feat(stack): stop lazy services after inactivity - #6579
Conversation
There was a problem hiding this comment.
Superseded by a newer AI review
🤖 AI Review
Both reviews completed. After deduplication, 10 findings are confirmed and 2 refuted. The most serious issue is backward compatibility: previously persisted stack definitions retain the v1 format tag but cannot pass the new exact-shape validation. Two idle-timer races and an abnormal-retirement consistency issue are also confirmed, while stack-wide fencing after cleanup failure and ignoring idle timeouts for eager capabilities are intentional safety/semantic behavior.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🔴 CRITICAL | packages/stack/src/state/StackState.ts:124 |
backward-compatibility |
claude+codex | Previously persisted v1 stack definitions are rejected after upgrade because idleTimeoutSeconds became a mandatory exact-shape key without a format bump or legacy normalization. |
| 🟡 MINOR | packages/stack/src/model/capabilities/auth.ts:398 |
test-coverage |
claude | The new 60-second defaults make the whole-stack E2E's simultaneous-ready assertion timing-dependent: early Auth and Realtime activations can retire before the scenario reaches its exact status and workload assertions. |
| 🟡 MINOR | packages/stack/src/supervisor/Supervisor.ts:402 |
error-handling |
claude | A defect or interruption during idle retirement escapes through an unobserved timer fiber after the capability has already been removed from active state, without logging or marking cleanup unproven. |
| 🟡 MINOR | packages/stack/src/supervisor/Supervisor.ts:479 |
concurrency |
claude | cancelIdleTimers updates timer and generation state outside the admission semaphore, allowing a concurrently armed timer to be installed with a stale generation and delay retirement by another timeout. |
| 🟡 MINOR | packages/stack/src/supervisor/Supervisor.ts:432 |
concurrency |
codex | A very short idle timer can complete before its bookkeeping entry is installed, leaving a completed fiber in idleTimers that permanently suppresses future arming for that capability. |
| ⚪ NIT | packages/stack/src/model/Compiler.ts:549 |
code-quality |
claude | validateIdleTimeouts contains unreachable finite/positive checks whose shared error message incorrectly describes those values as unsupported capabilities. |
| ⚪ NIT | packages/stack/src/supervisor/Supervisor.ts:333 |
readability |
claude | The first branch of canRetire is redundant because its condition already guarantees the final dependency expression returns true. |
| ⚪ NIT | packages/stack/src/gateway/activity.integration.test.ts:53 |
test-quality |
claude | The new gateway fixtures start listening before registering their error listener, violating the repository's required foreign-lifecycle ordering. |
| ⚪ NIT | packages/stack/src/public/whole-stack.e2e.test.ts:1508 |
test-quality |
claude | A REST request or assertion failure in the idle E2E waits for the full log-observation timeout in finally before surfacing the original failure. |
| ⚪ NIT | packages/stack/README.md:44 |
docs |
claude | The new README text contains a 132-character line inconsistent with the surrounding wrapping. |
Refuted findings (kept for transparency, not posted as review comments)
packages/stack/src/supervisor/Supervisor.ts:404(error-handling): An idle-cleanup failure unnecessarily fences the entire stack in the stopping phase instead of recovering the affected capability locally.
Refuted: This is a deliberate exact-cleanup safety fence. SessionLauncher.ts:69-78 shows that stop/remove failures can leave uncertain runtime ownership, while Supervisor.ts:195 and the dedicated integration test establish that such uncertainty must remain in stopping until explicit cleanup. Locally restoring activation would risk reusing an inconsistent launcher session.packages/stack/src/model/Compiler.ts:522(config-validation): Silently converting a numeric idleTimeoutSeconds to false for eager capabilities is inconsistent with rejecting that option on unsupported capabilities.
Refuted: The cases have different semantics: an eager capability is continuously active, so an idle timeout is inapplicable and materializes as false, while an unsupported capability cannot honor the option in any activation mode. The eager behavior is explicitly covered by the new integration test and does not contradict runtime behavior.
Stats
Claude findings: 11 · Codex findings: 2 · Confirmed: 10 · Refuted: 2 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
|
/ai-review |
There was a problem hiding this comment.
🤖 AI Review
Adjudicated 9 Claude findings and 1 Codex finding into 9 deduplicated entries. Four are confirmed: an unhandled defect path can leave an idle-stopped capability inconsistent, two tests depend on scheduler ordering, unused fixture hooks remain, and the architectural ADR is not updated. Five are refuted, including the merged persisted-state finding because trusted ADR 0017 explicitly defines this state as private and unreleased rather than a compatibility contract. No critical user-facing issue remains.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | packages/stack/src/supervisor/Supervisor.ts:396 |
error-handling |
claude | A defect during idle capability removal can leave the capability fenced out while its stale session entry prevents a later activation from restarting its workload. |
| 🟡 MINOR | packages/stack/src/supervisor/supervisor.integration.test.ts:955 |
test-quality |
claude | Racing Deferred.await against TestClock.adjust checks whether completion wins scheduler ordering rather than whether it occurs within a virtual-time interval. |
| ⚪ NIT | packages/stack/src/supervisor/supervisor.integration.test.ts:108 |
dead-code |
claude | The workloadStopFinished and workloadStopFinishedFor fixture options are wired into the driver but unused by every test. |
| ⚪ NIT | packages/stack/README.md:28 |
documentation |
claude | The new traffic-based automatic stopping architecture is documented in the package README but omitted from the managed-stack architecture ADR. |
Refuted findings (kept for transparency, not posted as review comments)
packages/stack/src/supervisor/Supervisor.ts:398(error-handling): A failed background idle stop unnecessarily fences the whole stack as stopping and silently makes it unusable until stop and start are run.
Refuted: The fence is the repository's documented fail-closed behavior, not an accidental availability state: trusted ADR 0017 states that unproven exact cleanup must remain stopping and be recoverable through retryable stop(). The failure is also not silent because Supervisor.ts:401-403 logs it and StatusProjection.ts:67 exposes lifecycle="stopping".packages/stack/src/state/StackState.ts:124(backward-compatibility): Making idleTimeoutSeconds mandatory causes definitions persisted by earlier builds to fail validation, constituting a supported-state compatibility break.
Refuted: The old documents would indeed fail the new shape check, but trusted ADR 0017 explicitly defines the managed document as private, unreleased state that is not a compatibility facade or independently versioned database schema. Thus this is not the claimed supported user-facing compatibility break, and critical severity is unwarranted.packages/stack/src/model/Compiler.ts:522(config-validation): Handling idleTimeoutSeconds is inconsistent because a positive timeout on an eager supported capability is normalized to false, while a positive timeout on an unsupported capability is rejected.
Refuted: These cases have different semantics. README.md:44 explicitly documents that eager capabilities never auto-stop, while an unsupported capability cannot auto-stop in any activation mode. idle-config.integration.test.ts:53-84 directly codifies both behaviors, and false consistently means idle stopping is disabled.packages/stack/src/public/whole-stack.e2e.test.ts:364(test-quality): When the timeout wins, waitForLogEntry leaves observation without a rejection handler and closing the iterator can cause an unhandled rejection.
Refuted: Promise.race installs fulfillment and rejection handlers on every input promise, including observation. A later observation rejection invokes the already-settled race's rejection handler and is therefore handled rather than becoming an unhandled rejection.packages/stack/src/supervisor/Supervisor.ts:426(effect-idiom): Using uninterruptibleMask without restore violates the repository's Effect convention and should be replaced with uninterruptible.
Refuted: Trusted CLAUDE.md specifically permits uninterruptibleMask for an acquisition-to-registration handoff, which is exactly this block. Its operations are non-blocking, so there is no blocking acquisition that must be wrapped with restore.
Stats
Claude findings: 9 · Codex findings: 1 · Confirmed: 4 · Refuted: 5 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
Lazy services currently remain running after their first request. Add traffic-based idle stopping for REST, Auth, Realtime, Studio, and the pooler, with a 60-second default and a per-service
idleTimeoutSecondsoverride orfalseto disable it.In-flight requests, open streams and sockets, and running dependants keep services active. After retirement, the next request starts the service again through the same listener while preserving stack data. Eager services remain running.