From 38868b21286e7d0a69be77c3c76fa4e937aa6d2e Mon Sep 17 00:00:00 2001 From: Silo Server Migration Date: Mon, 25 May 2026 00:16:41 -0400 Subject: [PATCH] feat(requests): add cancel endpoint and tighten state guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Owners can now POST /requests/{id}/cancel to withdraw a pending request; admins can cancel any active request that has not entered the fulfillment pipeline. The route is mounted on both the user group (with profile required) and the admin group. The cancelled outcome was already reserved in the migration's CHECK constraint but was unreachable from any handler. Decline now also rejects approved requests — between Approve setting StatusApproved and the reconciler picking the request up, an admin could declare the request declined while submission was about to fire. The reconciler's outcome filter would skip the request, but the narrow window meant external state could diverge from Silo's view. Refuse decline once a request is approved; callers should wait for completion or use the failed/retry path. Reconcile now emits a slog.WarnContext at the per-request failure site with request id, media type, tmdb id, status, and integration kind. Aggregated counters in ReconcileResult are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/api/handlers/requests.go | 24 +++++++ internal/api/router.go | 2 + internal/requests/service.go | 40 ++++++++++++ internal/requests/service_test.go | 100 ++++++++++++++++++++++++++++++ 4 files changed, 166 insertions(+) diff --git a/internal/api/handlers/requests.go b/internal/api/handlers/requests.go index 872ba7c6..69d93046 100644 --- a/internal/api/handlers/requests.go +++ b/internal/api/handlers/requests.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "io" "net/http" "strconv" "strings" @@ -26,6 +27,7 @@ type RequestService interface { GetRequest(ctx context.Context, viewer mediarequests.Viewer, id string) (*mediarequests.Request, error) Approve(ctx context.Context, viewer mediarequests.Viewer, id string) (*mediarequests.Request, error) Decline(ctx context.Context, viewer mediarequests.Viewer, id, reason string) (*mediarequests.Request, error) + Cancel(ctx context.Context, viewer mediarequests.Viewer, id, reason string) (*mediarequests.Request, error) Retry(ctx context.Context, viewer mediarequests.Viewer, id string) (*mediarequests.Request, error) GetSettings(ctx context.Context, viewer mediarequests.Viewer) (mediarequests.Settings, error) UpdateSettings(ctx context.Context, viewer mediarequests.Viewer, settings mediarequests.Settings) (mediarequests.Settings, error) @@ -325,6 +327,28 @@ func (h *RequestsHandler) HandleDecline(w http.ResponseWriter, r *http.Request) writeJSON(w, http.StatusOK, req) } +func (h *RequestsHandler) HandleCancel(w http.ResponseWriter, r *http.Request) { + viewer, ok := requestViewer(w, r, false) + if !ok { + return + } + var body struct { + Reason string `json:"reason"` + } + if r.Body != nil { + if err := json.NewDecoder(r.Body).Decode(&body); err != nil && !errors.Is(err, io.EOF) { + writeError(w, http.StatusBadRequest, "bad_request", "Invalid request body") + return + } + } + req, err := h.service.Cancel(r.Context(), viewer, chi.URLParam(r, "id"), body.Reason) + if err != nil { + writeRequestServiceError(w, err) + return + } + writeJSON(w, http.StatusOK, req) +} + func (h *RequestsHandler) HandleRetry(w http.ResponseWriter, r *http.Request) { viewer, ok := requestViewer(w, r, false) if !ok { diff --git a/internal/api/router.go b/internal/api/router.go index cbc68d85..20df1d83 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -1407,6 +1407,7 @@ func NewRouter(deps Dependencies) chi.Router { r.Post("/", requestHandler.HandleCreate) r.Get("/mine", requestHandler.HandleListMine) r.Get("/{id}", requestHandler.HandleGet) + r.Post("/{id}/cancel", requestHandler.HandleCancel) }) } @@ -1909,6 +1910,7 @@ func NewRouter(deps Dependencies) chi.Router { r.Get("/requests", requestHandler.HandleAdminList) r.Post("/requests/{id}/approve", requestHandler.HandleApprove) r.Post("/requests/{id}/decline", requestHandler.HandleDecline) + r.Post("/requests/{id}/cancel", requestHandler.HandleCancel) r.Post("/requests/{id}/retry", requestHandler.HandleRetry) r.Get("/request-settings", requestHandler.HandleGetSettings) r.Put("/request-settings", requestHandler.HandleUpdateSettings) diff --git a/internal/requests/service.go b/internal/requests/service.go index 974d9e35..49b19f79 100644 --- a/internal/requests/service.go +++ b/internal/requests/service.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "log/slog" "strings" "time" @@ -373,7 +374,10 @@ func (s *Service) Decline(ctx context.Context, viewer Viewer, id, reason string) if err != nil { return nil, err } + // Approved requests are pending submission by the reconciler; declining + // while submission may be in flight risks a divergent external state. if req.Outcome != OutcomeActive || + req.Status == StatusApproved || req.Status == StatusCompleted || req.Status == StatusQueued || req.Status == StatusDownloading || @@ -384,6 +388,34 @@ func (s *Service) Decline(ctx context.Context, viewer Viewer, id, reason string) return s.store.SetOutcome(ctx, req.ID, OutcomeDeclined, viewer, reason) } +// Cancel withdraws a request that has not yet been submitted to a downstream +// integration. Owners can cancel their own pending requests; admins can cancel +// any active request that has not entered the fulfillment pipeline. Requests +// already approved, queued, downloading, or completed cannot be cancelled — +// callers should decline (admin) or wait for completion in those cases. +func (s *Service) Cancel(ctx context.Context, viewer Viewer, id, reason string) (*Request, error) { + if viewer.UserID == 0 { + return nil, ErrForbidden + } + req, err := s.store.GetRequest(ctx, strings.TrimSpace(id)) + if err != nil { + return nil, err + } + if !viewer.IsAdmin && req.RequestedByUserID != viewer.UserID { + return nil, ErrForbidden + } + if req.Outcome != OutcomeActive || + req.Status == StatusApproved || + req.Status == StatusCompleted || + req.Status == StatusQueued || + req.Status == StatusDownloading || + strings.TrimSpace(req.ExternalID) != "" || + strings.TrimSpace(req.IntegrationKind) != "" { + return nil, ErrInvalidState + } + return s.store.SetOutcome(ctx, req.ID, OutcomeCancelled, viewer, reason) +} + func (s *Service) Retry(ctx context.Context, viewer Viewer, id string) (*Request, error) { if !viewer.IsAdmin { return nil, ErrForbidden @@ -425,6 +457,14 @@ func (s *Service) ReconcileRequests(ctx context.Context, limit int) (ReconcileRe } change, err := s.reconcileRequest(ctx, *req) if err != nil { + slog.WarnContext(ctx, "request reconcile failed", + "request_id", req.ID, + "media_type", req.MediaType, + "tmdb_id", req.TMDBID, + "status", req.Status, + "integration_kind", req.IntegrationKind, + "err", err, + ) result.Errors++ continue } diff --git a/internal/requests/service_test.go b/internal/requests/service_test.go index 7efe6fb8..472ea8f6 100644 --- a/internal/requests/service_test.go +++ b/internal/requests/service_test.go @@ -649,6 +649,106 @@ func TestReconcileRequestsMarksDownloadingFromAdapter(t *testing.T) { } } +func TestCancelOwnerCanWithdrawPendingRequest(t *testing.T) { + store := newFakeStore() + store.requests["req-mine"] = &Request{ + ID: "req-mine", + MediaType: MediaTypeMovie, + TMDBID: 550, + Status: StatusPending, + Outcome: OutcomeActive, + RequestedByUserID: 7, + } + service := newTestService(store) + + req, err := service.Cancel(context.Background(), Viewer{UserID: 7, ProfileID: "profile-1"}, "req-mine", "no longer want") + if err != nil { + t.Fatalf("Cancel returned error: %v", err) + } + if req.Outcome != OutcomeCancelled { + t.Fatalf("Outcome = %q, want cancelled", req.Outcome) + } +} + +func TestCancelNonOwnerForbidden(t *testing.T) { + store := newFakeStore() + store.requests["req-someone-else"] = &Request{ + ID: "req-someone-else", + MediaType: MediaTypeMovie, + TMDBID: 550, + Status: StatusPending, + Outcome: OutcomeActive, + RequestedByUserID: 7, + } + service := newTestService(store) + + _, err := service.Cancel(context.Background(), Viewer{UserID: 8, ProfileID: "profile-2"}, "req-someone-else", "") + if !errors.Is(err, ErrForbidden) { + t.Fatalf("err = %v, want ErrForbidden", err) + } +} + +func TestCancelAdminCanCancelAnyPending(t *testing.T) { + store := newFakeStore() + store.requests["req-other"] = &Request{ + ID: "req-other", + MediaType: MediaTypeMovie, + TMDBID: 550, + Status: StatusPending, + Outcome: OutcomeActive, + RequestedByUserID: 7, + } + service := newTestService(store) + + _, err := service.Cancel(context.Background(), Viewer{UserID: 99, IsAdmin: true}, "req-other", "house cleaning") + if err != nil { + t.Fatalf("admin Cancel returned error: %v", err) + } +} + +func TestCancelRejectsRequestsAlreadyInFulfillment(t *testing.T) { + cases := []struct { + name string + req Request + }{ + {"approved", Request{Status: StatusApproved, Outcome: OutcomeActive}}, + {"queued", Request{Status: StatusQueued, Outcome: OutcomeActive, IntegrationKind: "radarr", ExternalID: "42"}}, + {"downloading", Request{Status: StatusDownloading, Outcome: OutcomeActive, IntegrationKind: "radarr", ExternalID: "42"}}, + {"completed", Request{Status: StatusCompleted, Outcome: OutcomeActive}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + store := newFakeStore() + tc.req.ID = "req-x" + tc.req.RequestedByUserID = 7 + store.requests["req-x"] = &tc.req + service := newTestService(store) + + _, err := service.Cancel(context.Background(), Viewer{UserID: 7, ProfileID: "profile-1"}, "req-x", "") + if !errors.Is(err, ErrInvalidState) { + t.Fatalf("err = %v, want ErrInvalidState for %s", err, tc.name) + } + }) + } +} + +func TestDeclineRejectsApprovedRequests(t *testing.T) { + store := newFakeStore() + store.requests["req-approved"] = &Request{ + ID: "req-approved", + MediaType: MediaTypeMovie, + TMDBID: 550, + Status: StatusApproved, + Outcome: OutcomeActive, + } + service := newTestService(store) + + _, err := service.Decline(context.Background(), Viewer{UserID: 1, IsAdmin: true}, "req-approved", "changed mind") + if !errors.Is(err, ErrInvalidState) { + t.Fatalf("err = %v, want ErrInvalidState (approved is owned by the reconciler)", err) + } +} + func TestDeclineRejectsQueuedRequests(t *testing.T) { store := newFakeStore() store.requests["req-1"] = &Request{