Skip to content

feat(stack): stop lazy services after inactivity - #6579

Open
jgoux wants to merge 3 commits into
developfrom
feat/stack-idle-stop
Open

feat(stack): stop lazy services after inactivity#6579
jgoux wants to merge 3 commits into
developfrom
feat/stack-idle-stop

Conversation

@jgoux

@jgoux jgoux commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

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 idleTimeoutSeconds override or false to 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.

@jgoux
jgoux requested a review from a team as a code owner September 11, 2026 15:15
@jgoux jgoux self-assigned this Sep 11, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/stack/src/model/capabilities/auth.ts
Comment thread packages/stack/src/supervisor/Supervisor.ts Outdated
Comment thread packages/stack/src/supervisor/Supervisor.ts
Comment thread packages/stack/src/model/Compiler.ts
Comment thread packages/stack/src/state/StackState.ts
Comment thread packages/stack/src/supervisor/Supervisor.ts Outdated
Comment thread packages/stack/src/gateway/activity.integration.test.ts Outdated
Comment thread packages/stack/src/public/whole-stack.e2e.test.ts
Comment thread packages/stack/README.md Outdated
Comment thread packages/stack/src/supervisor/Supervisor.ts Outdated
@jgoux

jgoux commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

/ai-review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Comment thread packages/stack/src/supervisor/Supervisor.ts Outdated
Comment thread packages/stack/src/supervisor/supervisor.integration.test.ts Outdated
Comment thread packages/stack/src/supervisor/supervisor.integration.test.ts Outdated
Comment thread packages/stack/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant