Skip to content

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

Merged
zxch3n merged 4 commits into
mainfrom
fix/bounded-session-initialization-deadline
Sep 27, 2026
Merged

zxch3n merged 4 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.


Second review follow-up (f228fd3): detach the orphan before terminating it

The reaper terminated a late orphan with the manager's listeners still attached. Because createSessionInner registers the Session before managed-runtime resolution and createAgent, a retry's replacement is registered over the orphan under the same id; the orphan's terminated event then hit onTerminated, which deletes sessions[id] by id and forwards to MessageHandler — unregistering the healthy replacement and finalizing its live turn. The reaper now calls detachSession first, as the createAgent failure path already does.

Test uses real Sessions in production order and asserts the replacement stays registered and no terminated is emitted for that id; ablation fails each assertion independently.

Rebased onto main (2026-09-27). pnpm check exit 0; full CLI suite 3105 passing.

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

@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 👍 / 👎.

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 f228fd3 — correct finding, and my earlier identity guard could not have prevented it.

Why the guard failed: createSessionInner publishes the Session and attaches the manager listeners before managed-runtime resolution and createAgent run, so for both stages that can wedge, the orphan is registered first and the retry's replacement is registered over it under the same id. When the orphan later resolved, my if (sessions.get(id) === session) delete correctly skipped the replacement — but then terminate() emitted terminated, and the still-attached onTerminated deleted sessions[event.sessionId] by id and forwarded the event to MessageHandler. The guard ran before the listener and couldn't stop it. This is exactly the hazard the createAgent failure path already documents.

Fix: the reaper now calls this.detachSession(session) before terminate, matching that path. detachSession is instance-keyed, so the hand-written delete is gone. detachSession and sessionEventDetachers widened from Session to ISession, since the pending map yields the interface type.

This also covers requestSessionTerminate's timeout path, which reuses the same reaper: its caller has already been told terminated, so a second late event for that id could only hit a Session started there since.

Test (session-manager.test.ts, real Session instances in production order): orphan registers and wedges → retry registers a replacement and completes → orphan resolves. Asserts the replacement is still registered and the manager emitted no terminated for that id; the orphan's own terminated event is the awaited signal that it really died.

Ablation: restoring the old reaper body fails on the registry (getSession → null); with that assertion removed it still fails on the emitted terminated event. So each observable catches the defect independently.

Also rebased onto main (266 commits; only loro/AGENTS.md conflicted, resolved to main's version plus the one invariant line). Spec/Note updated — the Note's earlier claim that the guard kept the replacement registered was wrong and is corrected rather than appended to. pnpm check exit 0; full CLI suite 3105 passing.

zxch3n and others added 3 commits September 27, 2026 16:30
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>
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>
The reaper for an abandoned create terminated the late Session with the
manager's listeners still attached. `createSessionInner` publishes the
Session and registers those listeners before managed-runtime resolution
and `createAgent` run, so for both stages that can wedge the orphan is
registered first and a retry's replacement is registered over it under
the same id. When the orphan finally resolved, `terminate()` emitted
`terminated`; `onTerminated` deletes `sessions[event.sessionId]` by id,
not instance, and forwards the event to MessageHandler — unregistering
the healthy replacement and finalizing its live turn as "the agent
died". The identity-guarded delete ran before the listener and could not
prevent it.

Call `detachSession` before `terminate`, as the `createAgent` failure
path already does. It is instance-keyed, so the hand-written delete is
dropped; `detachSession` and its detacher map widen to `ISession`
because the pending map yields the interface type.

Model: claude-opus-5-5[1m]

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

@zxch3n zxch3n left a comment

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.

Reviewed head 9a80dcde88f61b4c60834ed3bd34d0012bc792a5 against current main eb2ecf5b7f843d2d30206a89c62c4092dbd03ab0. Verified the progress clock resets on phase/detail changes, the stall failure is not recorded as user cancellation, abandoned creates are detached for retry, late orphan Sessions are detached before termination, and pending-create teardown has a 300s bound. Rechecked the prior P1 fixes in the current code and tests. Current head CI (Tests, Static checks, Desktop E2E smoke, policy and labeling checks) is green. No P0/P1 found.

@zxch3n
zxch3n merged commit 3d16abd into main Sep 27, 2026
11 checks passed
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