refactor(app): make subsystem teardown coverage enforceable - #643
Conversation
App maintained two independent lists of what must be closed: the `closers` slice unwound on startup failure, and the hand-written order in `App.Shutdown`. Both are correct today and their orders deliberately differ — startup failure unwinds construction in strict reverse (nothing served traffic), while runtime shutdown quiesces before it flushes, closing providers first to stop the model refresh loop and the shared storage connection last, after every producer has flushed into it. Neither order derives from the other, so the runtime order stays spelled out by hand. What is removed is the drift risk of two lists: a subsystem added to one and forgotten in the other is released on a failed startup and leaked on every SIGTERM, with no test to catch it. Every subsystem now registers once during construction with the teardown path that owns it (App.Shutdown, the pre-drain prologue, or Server.Shutdown after in-flight requests finish). Startup failure unwinds that registry; shutdown keeps its explicit order, and tests assert the two cover each other in both directions. No behavior change: teardown order, log messages, and error text are unchanged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesSubsystem lifecycle teardown
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/app/subsystems.go`:
- Around line 132-148: Remove the generic reflection-based nil handling from
closerOf and update shutdownOrder to perform direct nil checks on each subsystem
field before registering or calling its Close method. Preserve safe shutdown for
uninitialized *Result values and nil storage.Storage interfaces, relying on the
existing nil-receiver behavior of Result.Close implementations such as
mcpgateway.Result.Close.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 263f2ec1-647d-475d-b30c-67e1079d19b7
📒 Files selected for processing (3)
internal/app/app.gointernal/app/subsystems.gointernal/app/subsystems_test.go
Confidence Score: 4/5Not safe to merge until duplicate subsystem registrations are rejected by lifecycle coverage. The reproduced failure is limited to subsystem lifecycle coverage, but it can permit a future duplicate registration to pass tests and execute resource teardown twice during startup rollback. Files Needing Attention:
 T-Rex LogsWhat T-Rex did
Comments Outside Diff (1)
Reviews (1): Last reviewed commit: "refactor(app): make subsystem teardown c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| registered := make(map[string]closerOwner, len(application.registered)) | ||
| for _, subsystem := range application.registered { | ||
| registered[subsystem.name] = subsystem.owner | ||
| } |
There was a problem hiding this comment.
The name-keyed map overwrites an earlier registration with the same name, so this test accepts one shutdown-order entry for storage even when the registry contains multiple shutdown-owned storage closers. unwind iterates the append-only registry and invokes every duplicate closer. Reject an already-present name before assigning it to the map so the test enforces the intended exactly-once registration invariant.
Artifacts
Original shutdown coverage test passed
- Ran the existing fully wired shutdown-order coverage test and captured its output; it passed before the focused duplicate-registration reproduction.
Focused duplicate-registration reproduction passed
- Ran the authored focused test that mirrors the coverage map check, registers duplicate storage closers, and verifies unwind invokes both; it passed, confirming the gap.
Authored duplicate-registration validation test source
- The exact focused Go test source used for execution, preserved after removing the temporary in-tree test file; it demonstrates the duplicate registrations and both-close assertion.
Captured authored validation test source
- Captured output of the command that printed the authored focused test source from the repository before cleanup; it records the executed test logic verbatim.
There was a problem hiding this comment.
Valid — fixed in b4a5d12.
The map key did collapse duplicates, so two ownedByShutdown registrations sharing a name satisfied one shutdownOrder slot while unwind walked the append-only slice and closed both.
Two checks now, since the invariant deserves its own test rather than only a side effect of the coverage map:
TestShutdownOrderHasNoUnregisteredEntriesrejects a repeated name before inserting it, so the coverage assertion can no longer be satisfied by a collapsed duplicate.TestEverySubsystemRegistersExactlyOnceasserts the registry-wide invariant directly, across every owner rather than just the shutdown-owned subset.
Both were mutation-verified against your exact scenario — a second subsystemStorage registration in New:
subsystems_test.go:90: subsystem "storage" is registered more than once, so the coverage check below cannot see the duplicate
subsystems_test.go:126: subsystem "storage" is registered 2 times; unwind would close it 2 times on startup failure
Worth noting for the record that no duplicate exists on main today: the one path registering subsystemUsage twice is the usageResult.Logger == nil branch, which return fail(...)s before reaching the second registration, so the two are mutually exclusive. The gap was in what the tests could detect, not in current behavior.
Generated by Claude Code
The coverage checks keyed the registry by subsystem name, so two registrations sharing a name collapsed into one map entry and satisfied a single shutdownOrder slot. unwind walks the append-only registry instead, and would close that resource twice on startup failure. Reject a repeated name while building the coverage map, and assert the exactly-once invariant the registry documents in its own test. Both fail on a duplicated storage registration.
Description
internal/appmaintained two independent lists of what must be closed:closersslice unwound byfailon startup failure, andApp.Shutdown.Both are correct today, and their orders deliberately differ:
providerscloses first (stopping the model refresh loop) while the sharedstorageconnection closes last, after every producer has flushed into it.Since neither order derives from the other, the runtime order stays spelled out by hand — that part is load-bearing and unchanged. What this PR removes is the drift risk of maintaining two lists: a subsystem added to one and forgotten in the other is released on a failed startup and leaked on every SIGTERM, and no test would catch it.
What changed
internal/app/subsystems.go: every subsystem registers once during construction (app.register) with the teardown path that owns it —ownedByShutdown,ownedByPrologue(long-lived streams closed before the HTTP drain, so they don't hold it open until its timeout), orownedByServer(response cache and response/conversation stores, released byServer.Shutdownonce no request is in flight).app.unwind);App.Shutdownkeeps its explicit order, moved toshutdownOrder().closerOfmoved alongside the registry (and an unrelated misplaced doc comment abovelogStartupInfofixed).Tests (
internal/app/subsystems_test.go)Coverage is asserted in both directions against a fully wired app (MCP, usage, budgets, and rate limits all enabled, so no subsystem is skipped by config):
ownedByShutdownregistration appears inshutdownOrder;shutdownOrderentry is registered, has that owner, and appears once;ownedByServer/ownedByProloguesubsystems are registered but stay out ofshutdownOrder;unwindcloses in reverse registration order, runs every closer even when one fails, and joins the errors.Both completeness checks were mutation-verified: removing
taggingfromshutdownOrderand adding an unregistered entry each fail with a specific message.User-visible impact: none. Teardown order, log messages, and error text are unchanged;
go build ./...,go vet ./internal/app/,gofmt, andgo test ./internal/app/... ./run/...all pass. (golangci-lintcould not run in this environment — the installed binary is built with go1.25 while the repo targets go1.26.5, which predates this change.)No provider behavior, configuration, or documented API is affected, so no docs update was needed.
AI Generated (optional)
Authored by Claude Code. Context: a review of the composition-root lifecycle in
internal/app— closure mechanics belong to each feature module (Result.Close), while closure ordering belongs to the composition root. This PR keeps that split and makes the coverage between the two teardown paths mechanically enforced instead of maintained by hand.Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Tests