Skip to content

fix: bound session initialization with a progress deadline - #759

Open
zxch3n wants to merge 2 commits into
mainfrom
fix/bounded-session-initialization-deadline
Open

zxch3n wants to merge 2 commits into
mainfrom
fix/bounded-session-initialization-deadline

Conversation

@zxch3n

@zxch3n zxch3n commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Problem

A Session turn whose initialization dependency never returns has no timeout and no failure path. It stays in initializing and keeps writing a presence heartbeat every 10 seconds until something external clears it.

Verified in ~/.lody/logs/2026-09-16.log.1 — session 1b227b98:

04:58:53.133  trace-span start  dispatch.resolve_user
04:58:53.135  trace-span start  execution.visible_turn
04:58:53.136  presence heartbeat  status=initializing seq=1
04:58:53.138  ERROR  Failed to verify machine access (network): fetch failed
   ... 221 further heartbeats, all status=initializing, nothing else ...
06:49:32.563  Supervisor requested graceful shutdown
06:49:32.568  presence session entry cleared

1h50m39s, ended only by a supervisor shutdown — not self-healing. Session 8fc8fcee stalled identically from 05:02:43 and was cleared by the same shutdown.

dispatch.resolve_user never emitted its end span. The next attempt, in the restarted process, took 70061ms; the normal value is 1ms. resolveUserForRequest awaits that lookup unconditionally for any session with a project or parent.

Across every retained log (923 initializations, 09-09..16): p50 0s, p90 4s, p99 13s. Only four samples exceed 60s — the two stalls above, a third 31-minute stall, and one genuine 249s cold codex-acp start.

Fix

The watchdog measures silence, not duration: the clock runs from the last phase-or-detail change. managed-runtime publishes a rising download percentage, so a healthy transfer resets it continuously and gets unlimited legitimate wall-clock time; a wedged one still trips. Stages with no progress signal degrade to elapsed time.

Per-stage budgets, because the honest worst cases differ by orders of magnitude:

Stage Budget Why
initializing 180s 3× the 60s USER_PROFILE_TIMEOUT_MS, the only intentionally slow dependency there; worst healthy observation was 70s
acp / resuming / managed-runtime 900s 3.6× the worst healthy 249s ACP start
git-clone 1800s No progress signal and unbounded input; bounds a wedged transfer only

A flat timeout was rejected: any value tight enough to help the bookkeeping stage would kill a legitimate clone.

  • SessionActivePresenceController detects. It already owns per-stage timing for the 120s slow-stage report and is the only module allowed to publish session presence. It does not clear presence itself (loro/AGENTS.md reserves that for the owning Effect release) but stops its own heartbeat immediately, so the 10s wake-up of every presence subscriber ends at detection.
  • SessionExecutionService enforces. awaitInitializationStall races the turn body with Effect.raceFirst and never completes unless the watchdog fires, so a turn reaching running pays nothing. On a stall it goes through the existing recordKnownChatFailureAndHaltEffect path with session_init_failed, producing a visible chat failure rather than a silent return to idle.

Interrupting the fiber directly was rejected: the scope finalizer reads Cause.isInterrupted and would have reported the stall as a user cancellation. initializationStalled latches on the runtime to exclude it from that branch.

Garbage collection

SessionGCManager does not read presence directly, but isEligibleForCleanup calls hasActiveTurn, and MessageHandler.hasActiveTurn returns state.turn.phase !== 'idle' || hasSessionActivePresence(sessionId). A stalled turn satisfies both halves, so such a session was permanently uncollectable. No separate fix needed: failing the turn releases the runtime and the presence entry.

Tests

Two tests in session-execution-service.test.ts wire the real SessionActivePresenceController into the service, with an injected clock and only setInterval faked so Effect's scheduler still runs — no real sleeps, no mock-call-count assertions. They assert observable state: chat failure reason and message, user-turn status, final idle status, the presence clear event, that no further heartbeat is published however far the clock advances, and that hasActiveTurn returns to false. The second drives a managed-runtime download 450s past a 60s budget while reporting progress, then wedges it.

Ablation: disabling the watchdog, and separately removing the race while keeping the watchdog, each make both tests hang to the 30s vitest timeout — the production symptom.

Checks

  • pnpm check exit 0; pnpm format clean
  • pnpm run docs check"errors": []
  • Full CLI suite: 2867 passed, 4 skipped, 0 failed

Docs

  • Draft Spec specs/session-initialization-deadline.md (+ zh) — the guarantee is new externally-visible behavior
  • Agent Note 2026-09-16-bounded-session-initialization.md (+ zh)
  • apps/cli/src/lib/loro/AGENTS.md gains the bounded-initializing invariant (trimmed rationale elsewhere to stay under the 8 KiB gate)

Not validated

The 900s and 1800s budgets have never been reached by a real download or clone, so they are bounds rather than measurements. Only the initializing budget is calibrated against observed stalls.

🤖 Generated with Claude Code


Review follow-up (2b9aafc): detaching the wedged create

Codex flagged a P1 that was correct and, on verification, worse than reported — the retry path did not merely fail to self-heal, it stalled again for the full budget.

Failing the turn left the create in SessionManager.pendingSessionCreates. That map answers every createSession for a session id with the cached in-flight promise, and the entry is cleared only by that promise's own .finally() — which never runs for a create that never settles. requestSessionTerminate was no escape either: its pending-create branch did a bare await pendingCreate and hung with it.

Skipping finalizeCancelledTurnEffect on the stall path is still correct (it would mark the turn cancelled), but it also owned the pending-session release, and the stall is the first halt that can land while createSession is in flight. That release moved into a dedicated finalizeStalledInitializationEffect; wasCancelled returned to its original form now that branch ordering distinguishes the two.

  • abandonPendingSessionCreate detaches the entry so a retry starts genuinely new work.
  • The work cannot be cancelled (no abort signal through createSessionFromPreparationOrCold), so it is reaped: a Session it eventually yields is terminated rather than orphaned, and the registry entry is dropped only while it still points at that instance.
  • requestSessionTerminate races a 300s deadline through a sentinel rather than a rejection, so a genuine terminate() failure still propagates.

New tests: session-manager.test.ts covers the real dedupe map (naive retry gets the same wedged promise; abandoning lets a retry complete; a late-materializing create is terminated; teardown detaches on its deadline instead of hanging). session-execution-service.test.ts adds the end-to-end retry — wedge, fail, then the retry reaches agent.prompt and is recorded handled.

Ablation proves it: removing only the abandonPendingSessionCreate call fails the retry test on the leftover map entry; removing that assertion too makes the retry hang for the full 30s timeout.

Spec and Note updated in the same commit. pnpm check exit 0; full CLI suite 2872 passing.

A turn whose initialization dependency never returned stayed in
`initializing` forever with no timeout and no failure path, writing a
presence heartbeat every 10s until the daemon restarted. Two sessions did
this for 1h51m and 1h47m on 2026-09-16; the hung dependency was
`dispatch.resolve_user`, whose span never ended.

Initialization now carries a per-stage deadline clocked from the last
published phase/detail change, so stages that report progress (a
managed-runtime download's percentage) keep resetting it while a wedged
one still trips. On expiry the presence controller stops its heartbeat
and `notifyInitializationStalled` fails the turn through the existing
`session_init_failed` path, which records a visible chat failure and
returns the session to `idle`.

This also unpins garbage collection: `hasActiveTurn` reads active
presence, so a stalled turn made the session permanently uncollectable.

Model: claude-opus-5[1m]

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 710c77e634

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +3290 to +3293
!turnRuntime.initializationStalled &&
(turnRuntime.cancelRequested ||
self.isTurnCancelled(sessionId, turnRuntime.turnId) ||
wasInterrupted);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Cancel the pending create when initialization stalls

When the deadline fires while trackPendingSession is awaiting SessionManager.createSession—for example during a wedged managed-runtime install or ACP startup—the race interrupts only the Effect wrapper, while the underlying promise remains in SessionManager.pendingSessionCreates. This new condition then suppresses finalizeCancelledTurnEffect, so no cleanup is even scheduled; a retry calls createSession, receives the same hung promise from the deduplication map, and stalls again. The watchdog therefore does not provide the documented retry/self-healing behavior for these initialization stages; the stalled create must be abortable and removed from the manager, with any session that subsequently materializes terminated.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed and fixed in 2b9aafc — the finding was correct, and the retry path was worse than "does not self-heal": it stalled again for the full budget.

Verified all three links in the chain:

  1. pendingSessionCreates is keyed by session id and createSession returns the cached in-flight promise (session-manager.ts:638-641), so the retry does get the same wedged promise.
  2. The entry is cleared only by that promise's own .finally() (session-manager.ts:647-655), which never runs for a create that never settles.
  3. requestSessionTerminate's pending-create branch did a bare await termination derived from pendingCreate (session-manager.ts:738-753), so the cleanup hung too — there was no escape hatch even manually.

Fix

Keeping finalizeCancelledTurnEffect skipped is still correct (it would mark the user's turn cancelled, which is wrong for a stall), but you're right that it also owned the pending-session release, and the stall is the first halt that can land while createSession is in flight. So the release moved into a dedicated finalizeStalledInitializationEffect, and wasCancelled went back to its original form now that branch ordering distinguishes the two.

  • SessionManager.abandonPendingSessionCreate(sessionId, reason) detaches the entry so the next createSession starts genuinely new work.
  • The work itself can't be cancelled — there's no abort signal through createSessionFromPreparationOrCold — so it's reaped: if the abandoned create ever yields a Session, that Session is terminated. The registry entry is dropped only while it still points at that instance, so a completed retry's Session is never unregistered.
  • Nothing awaits the wedged promise. requestSessionTerminate now races its wait against a 300s deadline (slowest healthy ACP start observed was 249s) using a sentinel rather than a rejection, so a genuine terminate() failure still propagates — that preserved the existing rejects when a still-starting session cannot be terminated test.

Tests

session-manager.test.ts covers the contract against the real dedupe map: a naive retry is handed the same wedged promise; abandoning detaches it so a retry completes; a create that materializes after abandonment is terminated; a wedged create makes requestSessionTerminate detach on its deadline instead of hanging.

session-execution-service.test.ts adds the end-to-end case: first create wedges → turn fails → the retry reaches agent.prompt and is recorded handled.

Ablation on that last one is the direct proof of your finding: removing only the abandonPendingSessionCreate call fails it on the leftover map entry, and removing that assertion too makes the retry itself hang for the full 30s vitest timeout.

Spec and Note updated in the same commit — the Spec now states the retry guarantee, that detaching is not cancellation, and the orphan-termination and bounded-teardown guarantees. pnpm check exit 0; full CLI suite 2872 passing.

Failing a stalled turn left the create itself in
`SessionManager.pendingSessionCreates`. That map answers every
`createSession` for a session id with the cached in-flight promise, and
the entry is cleared only by the promise's own `finally`, so a wedged
create was handed straight back to the user's retry — which stalled
identically. The documented self-healing did not exist.
`requestSessionTerminate` was no escape: its pending-create branch did a
bare `await pendingCreate` and hung with it.

Skipping `finalizeCancelledTurnEffect` on the stall path is still right
(it would mark the turn cancelled), but that finalizer also owned the
pending-session release, and the stall is the first halt that can land
while `createSession` is in flight. The release moves to a dedicated
`finalizeStalledInitializationEffect`.

`abandonPendingSessionCreate` detaches the entry so a retry starts fresh.
The work itself cannot be cancelled, so it is reaped: a Session it
eventually produces is terminated rather than orphaned, and the registry
entry is dropped only while it still points at that instance.
`requestSessionTerminate` now races a 300s deadline through a sentinel,
so a genuine terminate failure still propagates.

Model: claude-opus-5[1m]

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2b9aafcf97

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

this.sessions.delete(sessionId);
}
try {
await session.terminate(true);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Detach the orphan before terminating it

When an abandoned create later resolves after a retry has installed a replacement under the same session ID, this call terminates an instance whose SessionManager listeners are still attached. Session.terminate() emits terminated, and registerSessionEvents then unconditionally deletes sessions[event.sessionId] and forwards the event to MessageHandler, so the orphan can unregister and finalize the healthy replacement despite the identity guard above. The fresh evidence in this revision is this new reaper path; call detachSession(session) before terminating it, as the existing failed-create cleanup does.

AGENTS.md reference: apps/cli/AGENTS.md:L74-L78

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant