fix(restore): join downloads during Fx shutdown

[gfyrag]

What changed

Restore-mode services now own asynchronous download jobs for the full Fx application lifetime. Shutdown rejects new restore RPC work, cancels and joins an active download, waits for admitted restore RPCs, then closes retained staging Pebble state.

RPC detachment remains intentional: cancelling the initiating StartDownload RPC does not cancel the background job; explicit CancelDownload remains the job-level cancellation API.

Why

The current service creates downloads from a background context and has no Fx stop hook. A deterministic production-module regression showed Fx shutdown returning while an S3 manifest request and restore job remained alive, leaving service-owned staging state without teardown ownership.

Product / operational motivation

Need: restore work may outlive its initiating RPC, but must never outlive the restore-mode application that owns it.
Current limitation: Fx shutdown neither cancels nor joins active download work and never closes retained staging Pebble state.
Requirement / constraint: once shutdown begins, no new job starts; active work receives cancellation and is joined; admitted RPCs finish; service-owned staging state closes last.
Evidence: internal/bootstrap/restore_download_lifecycle_test.go and internal/adapter/grpc/server_restore_lifecycle_test.go.
Durable repository evidence: docs/ops/backup-restore.md and docs/technical/architecture/subsystems/api/grpc-api.md.

Technical decision

Decision: add a restore-service lifetime context, an admission/stopping gate, active-request and active-job joins, and phased Fx stop hooks that order admission closure before transports and resource closure after gRPC draining.
Why now / why proportionate: this is the smallest ownership boundary matching the existing single-job service and Fx lifecycle.
Alternatives considered: tying work to the initiating RPC would break intentional asynchronous behavior; closing staging state directly from Fx would race active restore operations; a broader restore subsystem redesign is unnecessary.

Risk

MEDIUM: shutdown ordering and concurrency change in restore mode; deterministic synchronization tests and focused race validation cover the affected paths.

Validation

• bash scripts/agent-check (standalone, then repeated by the final PR gate)
• AI_REVIEW_BASE_SHA=afdd58395fd4689c624286893c2c618e38db8148 bash scripts/agent-check-pr with shared caches and reduced local build concurrency (GOMAXPROCS=4 GOFLAGS=-p=2)
• Focused race suites: ./internal/adapter/grpc and ./internal/bootstrap
• Production Fx regression: go test -race -tags s3 ./internal/bootstrap -run '^TestRestoreDownloadStopsWithFxApplication$' -count=1 -timeout 2m
• CI Tests explicitly runs the tagged Fx regression against the in-process S3 endpoint; no MinIO or Docker is required for this case
• Regression sensitivity: restoring the background parent made the production Fx regression fail deterministically because shutdown did not cancel the job
• Final diff and fresh review: no blocking findings or generated drift
• GitHub CI passed on 8797af37b73b3619e6408c396234f1f478146cf6, including the explicit S3 Fx lifecycle step

Architecture / behavior impact

No wire, storage-format, FSM, Raft, or compatibility change. Restore-mode shutdown now owns asynchronous work and retained staging-store teardown. Explicit cancellation still leaves the application available for another download.

Review focus

Please check lock/join ordering, the double admission check at job registration, and the phased Fx hook order around HTTP/gRPC shutdown and staging-store closure.

Known concerns

A download backend that ignores context cancellation can keep shutdown blocked. That is intentional fail-loud ownership behavior; returning would violate the application-lifetime invariant.

[NumaryBot]

✅ Approve — automated review

The shutdown admission gate, request/job joins, cancellation, and Fx hook ordering are consistent. No actionable correctness defect was found in the current diff.

No findings.

[shipfox-ai]

Both reviews were verified line-by-line against the diff and the code under test. The PR faithfully implements the requested restore-mode lifetime boundary: BeginShutdown closes admission and cancels the application-owned download context under the service mutex; Shutdown joins admitted RPCs and the active job without holding that mutex, then idempotently closes the retained staging store; the Fx hooks in module_restore.go are registered in the correct reverse-stop order around the gRPC/HTTP teardown. I independently re-checked the riskiest areas — the WaitGroup Add-vs-Wait race (closed by the shared mutex), the ordering of stagingStore publication versus job.done closure, the interaction between CancelDownload's bounded drain and Shutdown's join, and double-shutdown idempotency — and found no correctness, security, or compatibility defect. All retained findings are minor test-hygiene and maintainability items.

Recommendation: approve with comments.

Standards

1. Minor — unexplained error discard in test cleanup. internal/bootstrap/restore_download_lifecycle_test.go:80 contains _ = app.Stop(context.Background()) inside a t.Cleanup with no justification comment. AGENTS.md and docs/technical/contributing/conventions.md require an explicit justification comment for intentional discards. Because this cleanup runs even when earlier assertions fail, a real Fx shutdown error — the exact behavior this regression test exists to verify — would be silently swallowed and could mislead diagnosis. Either assert the error or add a concrete best-effort justification (e.g. // best-effort cleanup; failure already reported via stopDone when the test reached that point).

2. Minor — parallel-capable test is not parallelized. TestRestoreDownloadStopsWithFxApplication (internal/bootstrap/restore_download_lifecycle_test.go:27) omits t.Parallel(). AGENTS.md requires t.Parallel() "where supported by existing test conventions"; the bootstrap package uses it pervasively, and this test is fully isolated (own httptest backend, t.TempDir(), loopback listeners, own Fx app), so serialization is undocumented and unnecessary. Add t.Parallel() or document the shared resource that prevents it.

3. Low / judgement call — admission is enforced by per-handler convention, not by construction. The preamble if err := s.beginRequest(); err != nil { … }; defer s.endRequest() is repeated in all six restore handlers (internal/adapter/grpc/server_restore.go:271-274 and five more call sites; internal/adapter/grpc/server_restore_download.go:70-84, 127-130, 163-166), the "restore service is shutting down" message is duplicated between beginRequest and the StartDownloadBackup recheck, and the mock-storage/readerReady fixture is duplicated between startBlockedRestoreDownload and TestRestoreDownloadOutlivesInitiatingRPC in server_restore_lifecycle_test.go. The lifecycle invariant therefore depends on each future restore RPC remembering the preamble; centralizing admission in an interceptor or a shared wrapper (and an errShuttingDown sentinel plus a shared test fixture) would make omission impossible rather than merely unlikely. Not blocking.

Spec

1. Minor — the production-module regression test only executes under a build tag CI never passes for this package. internal/bootstrap/restore_download_lifecycle_test.go:1 carries //go:build s3, yet TestRestoreDownloadStopsWithFxApplication uses only an httptest server — no MinIO/Testcontainers, so it does not need the tag that per docs/technical/contributing/testing.md marks suites requiring MinIO. CI's unit job runs just test-coverage (plain go test ./..., no tags) and the e2e coverage jobs only target ./tests/e2e/..., so this deterministic Fx-level regression is neither compiled nor run by any CI gate; it executes only when a developer manually runs go test -tags s3 ./internal/bootstrap. The untagged in-process tests in internal/adapter/grpc/server_restore_lifecycle_test.go do cover the same invariant in the default suite, so coverage is not lost — but the PR's strongest evidence risks rotting unnoticed. Consider dropping the tag or moving the test so the default suite runs it.

No other spec findings: admission closure, the admit-then-stopping recheck at job registration, the no-mutex join, the staging-store-closes-last ordering, and the documented fail-loud choice to ignore the Fx deadline when a backend violates context cancellation all match the PR's claims.

Reviewed independently by GLM (glm-5.3-flash) and Codex (gpt-5.6-sol) via Shipfox; verified and synthesized by GLM.

[codecov]

Codecov Report

❌ Patch coverage is 88.70968% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.30%. Comparing base (afdd583) to head (8797af3).
⚠️ Report is 1 commits behind head on release/v3.0.

Files with missing lines	Patch %	Lines
internal/adapter/grpc/server_restore_download.go	69.23%	4 Missing ⚠️
internal/adapter/grpc/server_restore.go	92.68%	3 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##           release/v3.0    #1908   +/-   ##
=============================================
  Coverage         83.29%   83.30%           
=============================================
  Files               460      460           
  Lines             42399    42456   +57     
=============================================
+ Hits              35316    35367   +51     
- Misses             7078     7084    +6     
  Partials              5        5           

Flag	Coverage Δ
e2e	83.30% <88.70%> (+<0.01%)	⬆️
scenario	83.30% <88.70%> (+<0.01%)	⬆️
unit	83.30% <88.70%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[gfyrag]

Addressed the four items in the Shipfox review after rebasing onto afdd58395fd4689c624286893c2c618e38db8148:

1. Cleanup now asserts the app.Stop error in internal/bootstrap/restore_download_lifecycle_test.go.
2. The isolated Fx test now calls t.Parallel().
3. Kept admission at the service-method boundary. All six restore RPCs call beginRequest/endRequest; beginRequest and BeginShutdown synchronize the stopping flag and WaitGroup registration through the same mutex, and StartDownloadBackup rechecks at job registration. An interceptor alone would miss direct service calls, including the production-module regression. The proposed error sentinel/shared fixture are optional refactors without a correctness defect, so the focused implementation is retained.
4. The CI coverage gap is fixed: the Tests job now explicitly runs go test -race -tags s3 ./internal/bootstrap -run '^TestRestoreDownloadStopsWithFxApplication$' -count=1 -timeout 2m. The tag must remain: internal/infra/backup/s3.go requires it, and s3_disabled.go otherwise rejects S3 storage creation. The regression uses an in-process HTTP endpoint and needs no MinIO/Docker. The backend-start wait is also bounded so a setup failure reports its actual phase.

No unresolved inline threads or pending reviews were present. The S3 Fx regression and both affected package race suites passed locally. Canonical validation and the final candidate are recorded in the PR description and follow-up status.

Fix commit: 8797af37b73b3619e6408c396234f1f478146cf6. Final rebase base: afdd58395fd4689c624286893c2c618e38db8148; range-diff confirms the PR commits are unchanged by the subsequent HTTP/operator test-only base advances. After disk recovery, the complete exact-base agent-check-pr passed, including normalization, baseline and tooling race suites. The tagged Fx regression and both affected package race suites also passed. The worktree is clean. CI passed on this SHA, including the explicit S3 Fx lifecycle step. The remaining merge gate is the required human/CODEOWNER approval; no merge was performed.

[shipfox-ai]

This PR makes restore-mode download jobs children of the application lifetime and adds a phased Fx shutdown: admission closes and the job is canceled first (RestoreServiceServerImpl.BeginShutdown/beginRequest, internal/adapter/grpc/server_restore.go:187-230), admitted RPCs and the detached job are joined after the network servers stop (Shutdown, server_restore.go:232-244), and the retained staging Pebble store closes last, with hooks wired in internal/bootstrap/module_restore.go:73,139 in the correct LIFO order. I verified the concurrency independently: activeRequests.Add and the stopping flag share s.mu so no Add/Wait race exists; the job-registration recheck (server_restore_download.go:79-83) closes the admit-then-shutdown gap, and cancel propagation is gap-free regardless of where BeginShutdown interleaves, since the job context derives from lifetimeCtx; CancelDownload's bounded drain never holds s.mu, so it cannot deadlock against finishJob or Shutdown; repeated Shutdown is safe (closeStagingStore nils the handle, terminal job.done channels are closed). Both docs and the new -race -tags s3 CI step (docs/technical/contributing/testing.md:658-667, .github/workflows/main.yml:73-76) are mutually consistent and describe the actual shutdown sequence, satisfying AGENTS.md:90. Recommendation: approve with comments — no correctness or spec defects; the only retained findings are mild maintainability smells.

Standards

1. Minor — Duplicated admission preamble and shutdown sentinel across six handlers. The three-line beginRequest/endRequest guard is repeated verbatim at the entry of ValidateRestore, PreviewRestore, and FinalizeRestore (internal/adapter/grpc/server_restore.go:269-272,331-334,399-402) and StartDownloadBackup, GetDownloadStatus, and CancelDownload (internal/adapter/grpc/server_restore_download.go:70-73,127-130,163-166). The literal "restore service is shutting down" is additionally duplicated between beginRequest (server_restore.go:192) and the StartDownloadBackup recheck (server_restore_download.go:83). AGENTS.md:70 prefers DRY solutions; the concrete impact is that every future restore RPC must remember the preamble for the admission invariant to hold, and the sentinel string can drift. A gRPC interceptor (or a shared sentinel error) would centralize this; extraction is optional but recommended before more restore RPCs are added.
2. Minor — Duplicated blocked-reader test fixture. The storage.EXPECT().GetFile(...).DoAndReturn block that builds a cancellationBlockingReader and feeds a readerReady channel is inlined twice in internal/adapter/grpc/server_restore_lifecycle_test.go (startBlockedRestoreDownload at line 73 and TestRestoreDownloadOutlivesInitiatingRPC at line 158). The second site cannot reuse the helper only because it needs a cancelable RPC context; parameterizing the helper (e.g., accepting the start context and returning the cancel func) would remove the duplication.

No documented-standard violations found: no time.Sleep (tests synchronize on channels with bounded time.After fallbacks), t.Parallel() on every new test, NewMockStorage is the mockgen-generated mock (storage_generated_test.go) while cancellationBlockingReader is a legitimate hand-written io.ReadCloser fake, and documentation updates accompany the behavior change per AGENTS.md:82-90.

Spec

The Spec axis has no confirmed material finding — no missing or partial requirements, no scope creep, and no implementation defects. All four PR-body constraints are implemented and verified: (1) no new job starts after shutdown begins (beginRequest gate plus the server_restore_download.go:79-83 recheck, both serialized under s.mu with BeginShutdown); (2) active downloads are canceled and joined (server_restore_download.go:99 parents jobCtx to lifetimeCtx; Shutdown waits on job.done after finishJob publishes the staging store, so the join observes the published handle); (3) admitted RPCs finish (activeRequests.Wait() before any store close); (4) staging state closes last (closeStagingStore runs only after both joins; fx OnStop LIFO order is BeginShutdown → HTTP stop → gRPC stop → Shutdown → release bindings, matching module_restore.go and the updated docs). Explicit CancelDownload behavior is unchanged (job-only cancel, 5-second bounded drain, application stays alive). All evidence promised in the PR body exists in the diff, including the competing-cancel and repeated-shutdown tests.

One residual risk is explicitly acknowledged in the PR and is a deliberate trade-off, not a defect: Shutdown (server_restore.go:232-244) joins without bound and ignores the fx stop context, so a backend that violates the honored-cancellation contract (e.g., wedged S3 connection) will hang process shutdown visibly rather than silently claiming safe teardown — operators have no timeout escape short of SIGKILL. Also noted, non-blocking: the agent-check-pr validation box remains unchecked in the PR body ("pending after disk recovery"), a pending process item rather than a diff defect.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.