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 through agent-check-pr
• AI_REVIEW_BASE_SHA=e3ef7708729a709e19ae119d8165143fbeeebc28 bash scripts/agent-check-pr
• Focused race suites selected by the canonical gate: ./internal/adapter/grpc and ./internal/bootstrap
• Regression sensitivity: restoring the background parent made the production Fx regression fail deterministically because shutdown did not cancel the job

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, cancellation, request joining, job joining, and staging-store closure ordering is internally consistent and covered by focused lifecycle tests. No actionable correctness defect was identified.

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 81.42857% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.48%. Comparing base (e3ef770) to head (8caf088).

Files with missing lines	Patch %	Lines
internal/adapter/grpc/server_restore_download.go	46.15%	4 Missing and 3 partials ⚠️
internal/adapter/grpc/server_restore.go	86.66%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##           release/v3.0    #1908      +/-   ##
================================================
+ Coverage         77.31%   77.48%   +0.16%     
================================================
  Files               458      458              
  Lines             48243    48308      +65     
================================================
+ Hits              37300    37430     +130     
+ Misses             7807     7730      -77     
- Partials           3136     3148      +12     

Flag	Coverage Δ
e2e	77.48% <81.42%> (+0.16%)	⬆️
scenario	77.48% <81.42%> (+0.16%)	⬆️
unit	77.48% <81.42%> (+0.16%)	⬆️

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.