[IAmJSD]

Summary

handleScheduledHTTP serves the /api/scheduled/{task} route that GCP Cloud Scheduler and Azure Logic Apps call. It ran the task on r.Context(). When the scheduler reaches its attempt deadline (Cloud Scheduler attempt_deadline = "320s", Logic Apps HTTP action 120s), it drops the connection. Go then cancels the request context, and the task is cancelled with it. A task cancelled partway can lose the write that records an external call that already happened, or leave a row stuck in running. The purchase execution path itself is already detached (executeAndFinalize uses detachedTimeout). The main exposure is therefore ensureDB and the tasks that are not purchases, such as collect_recommendations and ri_exchange_reshape, plus the money sweeps once #712 schedules them over HTTP.

A second problem on the same route: http.Server.WriteTimeout is 30s. It does not cancel the handler context (I checked this locally). But a task that runs longer than 30s finishes and then can't write its response, so the scheduler sees EOF and retries.

Fix

• Detached, bounded task context (internal/server/http.go, internal/server/scheduled_tasks.go). Once the method, path and task-name checks pass, ensureDB and HandleScheduledTask run on ctxutil.DetachedWithTimeout(r.Context(), scheduledTaskHTTPTimeout). This context keeps the request's values, ignores the client disconnect, and has its own timeout.
• 300s bound. This matches the Cloud Run request_timeout configured in Terraform: cloud_run_request_timeout = 300 in terraform/environments/gcp/github-{dev,staging,prod}.tfvars and dev.tfvars.example, and default = 300 in terraform/environments/gcp/variables.tf. The module runs with cpu_idle = var.cpu_throttling, which defaults to true in terraform/modules/compute/gcp/cloud-run/variables.tf and is not overridden by any environment. So once 300s have passed, Cloud Run considers the request finished, throttles CPU and may scale the instance in. A longer bound would promise time the platform doesn't give.
  • The 300s also covers the purchase manager's own detached budget (4m execution plus 30s terminal save).
  • Azure Container Apps' ingress gives up at 240s, but the container keeps its CPU, so the same 300s bound applies there.
  • TestScheduledTaskTimeoutFitsCloudRunRequestTimeout checks the bound against those Terraform values.
  • Before this PR, the bound in practice was the scheduler disconnect (120s on Azure, at most 300s on Cloud Run), so 300s does not shorten anything.
• Write deadline. extendScheduledWriteDeadline uses http.ResponseController to raise the write deadline for this route only, to 300s plus 30s. Other routes keep the 30s WriteTimeout.
• Shutdown path for the detached task.
  • scheduledTaskTracker records in-flight scheduled tasks.
  • On SIGTERM, StartHTTPServer drains for 25s, i.e. shutdownGracePeriod minus one sixth.
  • It then cancels any scheduled task still running and waits, for the rest of the 30s grace, for those tasks to return before main closes the database.
  • The whole sequence stays inside 30s, which matches Container Apps' default 30s termination grace.
  • Cloud Run sends SIGKILL 10s after SIGTERM, so on Cloud Run a task still running at that point is killed outright. The per-task advisory lock and the row CAS keep the next run safe.
  • A task that starts after shutdown begins gets a context that is already cancelled.
• Lambda is not affected. EventBridge calls handleLambdaScheduledEvent, which keeps the invocation context and its Lambda deadline.

Shared helper with #897

internal/ctxutil/detached.go and detached_test.go are copied byte-identical from #897 (fork/fix/issue-790 at d4fd03d; blob hashes match), so the two PRs merge cleanly in either order. Moving purchase.detachedTimeout onto it is left for a follow-up.

Not in this PR: turning off scheduler retries for the money sweeps. Those sweeps are not scheduled over HTTP yet; #712 adds them, and the retry config belongs with those Terraform resources. An overlapping retry is already skipped by the per-task advisory lock (already_running).

Closes #784

Verification

Run locally on macOS (darwin/arm64) with Go 1.26.9, using mocks only. No real cloud calls, purchases or deploys.

Tests in internal/server/http_scheduled_context_test.go:

• TestHandleScheduledHTTP_RequestCancelDoesNotCancelTask cancels the request context while a mock task is running. It checks that the task context is not cancelled, that the request-scoped value survives, that the deadline is at most scheduledTaskHTTPTimeout, and that the response is 200.
• TestScheduledHTTP_ResponseOutlivesServerWriteTimeout uses a real CreateHTTPServer server with WriteTimeout set to 100ms and a task that takes 400ms. It checks that the client receives a 200.
• TestStartHTTPServerCancelsScheduledTaskOnShutdown runs a real StartHTTPServer, sends SIGTERM while a scheduled task is in flight, and checks that the task saw the cancellation before StartHTTPServer returned.
• TestScheduledTaskTimeoutFitsCloudRunRequestTimeout checks that the bound is at most every cloud_run_request_timeout in terraform/environments/gcp.

Fail-before evidence:

• First commit (f714189), original pre-fix code:

  --- FAIL: TestHandleScheduledHTTP_RequestCancelDoesNotCancelTask: Expected no error, got: context canceled
  --- FAIL: TestScheduledHTTP_ResponseOutlivesServerWriteTimeout: Post ".../api/scheduled/process_scheduled_purchases": EOF
  

• Second commit, new tests run against f714189's code:

  --- FAIL: TestStartHTTPServerCancelsScheduledTaskOnShutdown: StartHTTPServer returned while the detached scheduled task was still running
  --- FAIL: TestScheduledTaskTimeoutFitsCloudRunRequestTimeout: ../../terraform/environments/gcp/dev.tfvars.example: scheduledTaskHTTPTimeout 15m0s exceeds cloud_run_request_timeout 5m0s
  

• Value assertion, mutation probe (start(context.Background(), ...)): Expected request-scoped, got <nil>.

Pass-after evidence at fb8e792:

• go test -race ./internal/server/ -run '...4 tests above...|TestStartHTTPServer' -count=1 -v: all PASS, no races.
• go build ./...: ok
• go vet ./internal/server/... ./internal/ctxutil/... ./cmd/server/...: ok
• AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./internal/server/... ./internal/ctxutil/... ./cmd/server/... -count=1: all ok. The env override isolates two existing tests from a broken local ~/.aws/config; that is unrelated to this change.
• golangci-lint run ./internal/server/... ./internal/ctxutil/... ./cmd/server/... (v2.10.1, as pinned in the Makefile): 0 issues

Not covered: nothing was run against a real Cloud Scheduler or Logic Apps calling Cloud Run or Container Apps. The Cloud Run CPU-throttling and SIGKILL timing come from the platform docs and were not observed.

Note: internal/server/http.go was 510 lines before this PR and is 536 now, over the 500-line guideline. The new logic lives in scheduled_tasks.go.

Labels: the triage labels from #784 have been mirrored by a maintainer (the author can't set labels).

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: e5c19442-59f4-4648-9a4a-97bb30f7f6bd

📥 Commits

Reviewing files that changed from the base of the PR and between e003025 and fb8e792.

📒 Files selected for processing: 6

• internal/ctxutil/detached.go
• internal/ctxutil/detached_test.go
• internal/server/app.go
• internal/server/http.go
• internal/server/http_scheduled_context_test.go
• internal/server/scheduled_tasks.go

+---------------------------------------------------------------------------------------------------------------------+
| Use a project glossary. Create and maintain a single source of all the specific terms and vocabulary for a project. |
+---------------------------------------------------------------------------------------------------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review of f714189: draft: blocks ready

Scope: 2 files, +143/-2 (under the 400-line cap, http.go stays under 500 lines). The core change is sound: context.WithTimeout(context.WithoutCancel(r.Context()), 15m) keeps request values, drops cancellation, and stays bounded. The findings below are what keeps it from ready.

Findings

1. actionable, internal/server/http.go:363 (and StartHTTPServer, http.go:172-205): no shutdown path for the detached task. Before this PR a forced server.Shutdown timeout closed connections and the request context was cancelled, so a task unwound before app.Close. Now the task context only ends at the 15m timeout. Scenario: SIGTERM mid-sweep, Shutdown waits 30s (shutdownGracePeriod), returns, the process exits or closes the DB under a running task for up to 14.5 more minutes. Fix: derive the task context from an app-lifetime context that StartHTTPServer cancels after the grace period (WithoutCancel of the request, then cancel on that shutdown signal via context.AfterFunc or a merged cancel), or state in the PR why a hard kill is the accepted outcome and add a test for the chosen behavior.
2. actionable, internal/server/http.go:310: 15m task bound is not aligned with the hosting limits the PR names. Cloud Run request_timeout defaults to 300s, so the platform considers the request finished at 5m while the handler may run 10m longer (the PR body admits CPU throttling / scale-in risk). Either set the bound from the platform limit, or show the Terraform request_timeout / cpu_idle settings for the scheduled services make 15m safe. Also scheduledResponseGrace/WriteDeadline of 15m30s can never matter on Cloud Run past 300s.
3. nit, internal/server/http_scheduled_context_test.go:15-72: the test proves cancellation is dropped and a deadline exists, but not that request-scoped values survive (the PR claims "Request-scoped values are kept"). Add a context.WithValue key to the request context and assert the task sees it; a swap to context.Background() would currently pass.
4. nit, PR metadata: no labels. Closing issue #784 carries urgency/this-sprint, triaged, priority/p1, severity/high, impact/all-users, effort/m, type/bug; none are on the PR. Mirror them when it leaves draft.
5. nit, scope/duplication: purchase.detachedTimeout (internal/purchase/manager.go:237) and the new inline expression in #897 (detachedRIExchangeContext, internal/api/ri_exchange_detached.go) are the same one-liner as this call site. Three copies now. #897's body says it is kept local deliberately; agree on one owner before both merge.

Overlap with #897

Both use WithoutCancel + WithTimeout, different layers: #895 wraps the whole HTTP scheduled task (15m); #897 detaches the user-triggered ExecuteExchange handler (4m exec, 30s terminal save) in internal/api. No file overlap (internal/server vs internal/api). The scheduled ri_exchange_reshape task is covered by #895 at the outer level, and by #897 if it routes through the same handler code: the inner 4m budget then nests inside the 15m, which is compatible. The existing purchase path already detaches (executeAndFinalize via detachedTimeout), so #895 mainly protects ensureDB and non-purchase tasks (collect_recommendations, ri_exchange_reshape). Note that, so the PR body's "could abort a sweep mid-purchase" is overstated for purchases.

What I attacked

• Ran both new tests with -race at the SHA: pass. Full go test ./internal/server/... ./cmd/server/... with AWS_CONFIG_FILE/AWS_SHARED_CREDENTIALS_FILE=/dev/null: 591 pass.
• Mutation probes (each reverted): (a) r.Context() instead of WithoutCancel: RequestCancelDoesNotCancelTask fails ("context canceled"); (b) WithCancel(WithoutCancel(...)), no deadline: same test fails on the deadline assertion; (c) remove the extendScheduledWriteDeadline(w) call: ResponseOutlivesServerWriteTimeout fails (client EOF). No probe covers value propagation (finding 3).
• Fail-before/pass-after: the PR's own transcript shows both tests failing against pre-fix code; mutation (a) and (c) reproduce the same failures independently, which is equivalent evidence.
• Goroutine leaks: no new goroutine is spawned; the work stays on the handler goroutine and defer cancel() runs, so no unbounded goroutine. The bound is the 15m timeout only (finding 1/2).
• ResponseController reaches the real conn through the securityHeaders wrapper (second test goes through CreateHTTPServer and passes). ErrNotSupported is swallowed deliberately; other errors are logged.
• Retry overlap: a detached task outlives the scheduler attempt, so the retry runs concurrently; relies on the per-task advisory lock (already_running), unchanged by this PR.

Gate status (origin/main Review gate)

• Independent review naming SHA: this comment (draft, so not a merge approval).
• Findings resolved: 2 actionable open.
• CI on SHA: no workflow runs reported by gh pr checks, only CodeRabbit (skipped, draft). Not satisfied.
• Local real-path verification evidence on the PR: present in the body (mocks, local macOS, fail/pass output); no real-account verification, correctly labelled as mocks.
• Verdict/SHA/evidence recorded on the PR: this comment.

Blocks ready

1. Decide and implement the shutdown behavior for the detached task (finding 1) with a test.
2. Reconcile the 15m bound with Cloud Run/Container Apps request limits or document and test the choice (finding 2).
3. Add a context-value propagation assertion (finding 3).
4. Mirror the issue labels; CI must run and be green on the final SHA; re-review after any new commit.
5. Coordinate with #897 on a single detached-context helper (finding 5).

[cristim]

Clarification of the review above (f714189), after maintainer calibration. Only these block ready:

1. Shutdown behaviour of the detached task (internal/server/http.go:363). Either cancel the task when the shutdown grace expires, or document that the process hard-kills it. Either is acceptable.
2. The 15m bound vs Cloud Run's default 300s request timeout (internal/server/http.go:310). Either document the Cloud Run behaviour or adjust the bound.
3. Labels, now mirrored from fix(server): HTTP-triggered scheduled tasks run on the request context and can be cancelled mid-purchase #784.

Non-blocking: finding 3 (a test that request values survive) is a nit. Finding 5 (one shared detached-context helper with #897 and purchase.detachedTimeout) is a follow-up, not a merge condition.

[IAmJSD]

Reply to the independent review of f714189. New head: fb8e792. This is a new commit, not a force-push.

1. Shutdown path (actionable): fixed. The task is now cancelled when the drain window ends.

   • scheduledTaskTracker (internal/server/scheduled_tasks.go) records each detached task.
   • On SIGTERM, StartHTTPServer drains for 5/6 of shutdownGracePeriod (25s of 30s).
   • It then cancels any scheduled task still running and waits for those tasks to return within the remaining 5s, before main runs app.Close.
   • A task that starts after shutdown begins gets a context that is already cancelled.
   • Why cancel rather than accept a hard kill: the whole sequence stays inside 30s, which matches Container Apps' default termination grace. The task therefore unwinds through its own ctx checks instead of having the DB closed under it.
   • On Cloud Run, SIGKILL arrives 10s after SIGTERM, so a task still running there is hard-killed. The advisory lock and the row CAS keep the next run safe. The PR body documents this.
   • Test: TestStartHTTPServerCancelsScheduledTaskOnShutdown runs a real StartHTTPServer, sends SIGTERM with a task in flight, and checks that the task saw the cancellation before StartHTTPServer returned.
     • Against f714189's code it fails: StartHTTPServer returned while the detached scheduled task was still running.
     • At fb8e792 it passes, including under -race.
   • The existing TestStartHTTPServer* shutdown tests still pass.

2. 15m bound vs hosting limits (actionable): aligned. The bound is now 300s.

   • Terraform sets cloud_run_request_timeout = 300 in terraform/environments/gcp/github-dev.tfvars, github-staging.tfvars, github-prod.tfvars and dev.tfvars.example, and default = 300 in terraform/environments/gcp/variables.tf.
   • The Cloud Run module sets cpu_idle = var.cpu_throttling, which defaults to true in terraform/modules/compute/gcp/cloud-run/variables.tf and is not overridden. So 15m could never have been honoured.
   • 300s covers the purchase manager's detached 4m+30s budget. Azure Container Apps ingress gives up at 240s, but CPU stays allocated, so the same 300s applies there.
   • Before this PR, the bound in practice was the scheduler disconnect (at most 300s), so nothing gets shorter.
   • The write deadline is now 300s + 30s.
   • TestScheduledTaskTimeoutFitsCloudRunRequestTimeout parses those Terraform files and fails if the bound exceeds any of them. Against the 15m bound it failed with: dev.tfvars.example: scheduledTaskHTTPTimeout 15m0s exceeds cloud_run_request_timeout 5m0s.

3. Request-value propagation (nit): added. TestHandleScheduledHTTP_RequestCancelDoesNotCancelTask now puts a context.WithValue key on the request and asserts the task sees it. Mutation probe start(context.Background(), ...): Expected request-scoped, got <nil>. Reverted, it passes.

4. Labels: these are already mirrored from fix(server): HTTP-triggered scheduled tasks run on the request context and can be cancelled mid-purchase #784 (urgency/this-sprint, triaged, priority/p1, severity/high, impact/all-users, effort/m, type/bug). I made no change.

5. Shared helper: done.

   • internal/ctxutil/detached.go and detached_test.go are copied byte-identical from fix(ri-exchange): run ExecuteExchange outside the request context #897 (fork/fix/issue-790 d4fd03d). The blob hashes 67daf366… and ffeaacf9… match.
   • The scheduled-task call site uses ctxutil.DetachedWithTimeout, so the two PRs merge cleanly in either order.
   • Moving purchase.detachedTimeout onto the helper is left as a follow-up.
   • I also toned down the "mid-purchase" wording in the PR body, as noted under Overlap: executeAndFinalize already detaches.

Verification at fb8e792 (local macOS, Go 1.26.9, mocks only, no cloud calls):

• go build ./...: ok
• go vet ./internal/server/... ./internal/ctxutil/... ./cmd/server/...: ok
• AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./internal/server/... ./internal/ctxutil/... ./cmd/server/... -count=1: all ok
• go test -race on the four scheduled tests plus TestStartHTTPServer*: all PASS
• golangci-lint v2.10.1: 0 issues

Remaining gap: nothing was tested against a real Cloud Scheduler or Logic Apps deployment. The Cloud Run throttling and SIGKILL timing come from the platform docs.

[cristim]

Independent delta review of fb8e792 (previous: f714189): draft: blocks ready (CI only)

OLD_SHA is an ancestor of NEW_SHA. Delta is 6 files, +313/-26. Base has not moved (merge-base is origin/main e003025), PR is mergeable.

Previous findings

#	Finding	Status	Evidence
1	Shutdown of detached task (blocking per clarification)	RESOLVED	internal/server/http.go:203-215 drains for 5/6 of the grace period, then app.scheduledTasks.stop(shutdownCtx) cancels in-flight tasks and waits for them before StartHTTPServer returns; hard-kill on Cloud Run (SIGKILL at 10s) is documented at http.go:162-168 and in the PR body. Proof below.
2	15m bound vs Cloud Run 300s (blocking per clarification)	RESOLVED	internal/server/scheduled_tasks.go:26 sets 300s, with the reasoning in the comment. cloud_run_request_timeout is 300 in terraform/environments/gcp/variables.tf:226, the three github-*.tfvars and dev.tfvars.example; cpu_idle = var.cpu_throttling at terraform/modules/compute/gcp/cloud-run/main.tf:142. Pinned by a test; with the bound set back to 15m it fails on dev.tfvars.example.
3	Value-propagation test (nit)	RESOLVED	http_scheduled_context_test.go:29-79 now asserts the request-scoped value; the author's context.Background() probe is a valid kill.
4	Labels (blocking per clarification)	RESOLVED	PR and #784 both carry: effort/m, impact/all-users, priority/p1, severity/high, triaged, type/bug, urgency/this-sprint.
5	Shared helper (follow-up)	PARTIALLY RESOLVED, non-blocking	internal/ctxutil/detached.go is used by the scheduled call site; purchase.detachedTimeout still has its own copy (author flags it as a follow-up). Agreed.

New findings (all NON-BLOCKING)

1. nit, internal/server/http.go: file is now 536 lines (origin/main: 510, already over; this PR adds 26). The project guideline is 500. Moving the shutdown block into scheduled_tasks.go would fix it; not a merge condition.
2. nit, PR size: 431 lines against the 400 guideline, of which 56 are the ctxutil copy shared with #897. Acceptable given the overlap.
3. nit, http.go:203-215 on Cloud Run: SIGKILL comes 10s after SIGTERM, but cancellation only fires at 25s. So on Cloud Run the cancel path never runs for a task that outlasts the drain; the outcome is the hard kill the PR documents. Cancelling earlier (for example when the drain starts) would let tasks unwind inside the 10s. Behavior matches what was accepted, so not blocking.
4. nit, TestScheduledTaskTimeoutFitsCloudRunRequestTimeout only covers terraform/environments/gcp. Azure Container Apps limits are reasoned in prose only. Fine, since the 240s Azure ingress limit does not stop the task.

No races, leaks or double-cancel problems found:

• start: wg.Add(1) is under mu and stopped is checked under the same lock, so it cannot race stop's Wait. A task started after stop gets a context that is already cancelled and a no-op done.
• The watcher goroutine in start exits on stopCh or ctx.Done(); done() cancels the context, so it cannot outlive the task. Double cancel() is harmless.
• stop is idempotent (stopped guard, channel closed once). If the wait times out, the helper goroutine blocked in Wait ends when the tasks return; the process is exiting anyway.
• Application now holds a mutex and WaitGroup by value; go vet (copylocks) is clean on internal/server and ctxutil.
• Shutdown waits for the in-flight scheduled request during the drain, so the request is not truncated before the cancel; the post-cancel response write on a closing connection is harmless.

What I attacked

• Tests at NEW_SHA with -race: ./internal/server/... ./internal/ctxutil/... ./cmd/server/... 594 pass (AWS_CONFIG_FILE/AWS_SHARED_CREDENTIALS_FILE=/dev/null). -run 'TestStartHTTPServer|Scheduled' -count=20 -race: 1400 pass, no flakes.
• Mutation probes on the shutdown test (each reverted, tree clean afterwards):
  • skip the stop call: fails ("StartHTTPServer returned while the detached scheduled task was still running");
  • stop does not close stopCh: fails with the same message;
  • stop skips wg.Wait: fails, same message;
  • drop the drain shortening (Shutdown uses the full grace period): fails;
  • set the bound to 15m: TestScheduledTaskTimeoutFitsCloudRunRequestTimeout fails.
• The shutdown test is not vacuous: without stop, Shutdown times out at 500ms without cancelling the detached context, so the unwound channel stays empty and the test fails. Fail-before/pass-after is therefore reproduced by the first two probes, which are equivalent to the old code's behavior.

Gate status (origin/main Review gate)

• Independent review naming SHA: this comment (draft, so not a merge approval).
• Findings resolved: all findings that the clarification marks as blocking are resolved in code; the rest are nits or follow-ups.
• CI on SHA: NOT satisfied. Both runs are action_required (fork PR, needs maintainer approval): pre-commit 38823111211 and CI - Build & Test 38823111244. Nothing has run yet.
• Local real-path verification evidence on the PR: present in the PR reply and body (mocks, local macOS, fail/pass output). No real Cloud Scheduler / Logic Apps run; the PR labels this gap correctly.
• Verdict, SHA and evidence recorded on the PR: this comment.

Blocks ready

1. Approve the two action_required workflow runs and get CI green on fb8e792 (or the final SHA). Any new commit needs a re-review.

[cristim]

Gate record for fb8e792 (origin/main CLAUDE.md "Review gate"):

• Independent delta review of fb8e792 (previous f714189): both blocking items are resolved (shutdown behaviour; 300s bound matching Cloud Run). No code findings. Non-blocking: http.go is 536 lines, and the shared-helper consolidation is a follow-up.
• CI on fb8e792: CI Success pass, Run pre-commit hooks pass.
• Local verification (macOS, mocks):
  • -race on internal/server, internal/ctxutil and cmd/server: 594 tests pass. The shutdown and scheduled tests were run 20 times.
  • Four mutation probes on the shutdown path fail the shutdown test. Setting the bound back to 15m fails the Terraform pin test.
  • No real Cloud Run run.
• CR skipped: draft and quota-limited; covered by the independent review plus local verification.
• Labels mirrored from fix(server): HTTP-triggered scheduled tasks run on the request context and can be cancelled mid-purchase #784.

Marking ready (owner-authorized) and merging at fb8e792 with --match-head-commit.