Two separate disk reads for the same session entry

shouldSwitchToLiveModel reads the session store twice with skipCache: true: once here to check liveModelSwitchPending, and a second time inside resolveLiveSessionModelSelection (which also calls loadSessionStore). In practice the window is tiny, but a write arriving between the two reads could cause liveModelSwitchPending to be cleared on disk while resolveLiveSessionModelSelection picks up a different snapshot. Passing the already-loaded entry directly to avoid the redundant read would close this gap, though it would require small refactoring of resolveLiveSessionModelSelection.

This is a comment left during a code review.
Path: src/agents/live-model-switch.ts
Line: 156-183

Comment:
**Two separate disk reads for the same session entry**

`shouldSwitchToLiveModel` reads the session store twice with `skipCache: true`: once here to check `liveModelSwitchPending`, and a second time inside `resolveLiveSessionModelSelection` (which also calls `loadSessionStore`). In practice the window is tiny, but a write arriving between the two reads could cause `liveModelSwitchPending` to be cleared on disk while `resolveLiveSessionModelSelection` picks up a different snapshot. Passing the already-loaded `entry` directly to avoid the redundant read would close this gap, though it would require small refactoring of `resolveLiveSessionModelSelection`.

How can I resolve this? If you propose a fix, please make it concise.

Valid observation. The two disk reads are: (1) loadSessionStore to check liveModelSwitchPending, then (2) resolveLiveSessionModelSelection which calls loadSessionStore again internally. The race window is negligible (microseconds, same process, synchronous calls) and fixing it would require refactoring resolveLiveSessionModelSelection to accept a pre-loaded entry, which touches other callers. Keeping this as-is for now to minimize the surface area of the change.

Consume pending live-switch flag when selection already matches

shouldSwitchToLiveModel exits early when the persisted selection equals the current model, but it leaves liveModelSwitchPending set. That means a /model change made while idle (or during a turn that cannot restart) can leave a stale flag in sessions.json; later, any unrelated fallback attempt that differs from the persisted model can be treated as a fresh live-switch request and throw LiveSessionModelSwitchError, short-circuiting normal retry/compaction behavior. This branch should consume/clear the pending flag once the requested selection is already active.

Useful? React with 👍 / 👎.

Good catch — addressed in 440731475d. shouldSwitchToLiveModel now calls void clearLiveModelSwitchPending(...) (fire-and-forget) in the models-match branch before returning undefined. A dedicated test ("clears the stale liveModelSwitchPending flag when models already match") verifies the flag is removed from the session entry.

Catch fire-and-forget stale-flag clear failures

The stale-flag branch launches clearLiveModelSwitchPending(...) with void and no rejection handler, but that function awaits updateSessionStore, which can throw on write/lock/fs errors. Under Node 22, an unhandled rejection is fatal, so a transient session-store write failure here can terminate the process even though this path is only trying to clean up state. Please await this call or attach an explicit .catch(...) handler.

Useful? React with 👍 / 👎.

Fixed in 31492d8 — replaced void clearLiveModelSwitchPending(...) with clearLiveModelSwitchPending(...).catch(() => { /* best-effort */ }). This prevents fatal unhandled rejections under Node 22+ while keeping the fire-and-forget semantics (the stale-flag clear is non-critical — the worst case if it fails is one extra no-op re-evaluation on the next retry iteration).

Guard pending-flag clear against newer /model updates

requestedSelection is computed from a snapshot (shouldSwitchToLiveModel) and then clearLiveModelSwitchPending deletes liveModelSwitchPending unconditionally before throwing. Because /model writes that same flag in handleDirectiveOnly (sessionEntry.liveModelSwitchPending = true) and persists via updateSessionStore, a second /model command that lands between the read and this clear can have its new pending switch erased, while the runner still throws using the older selection. This drops the most recent user intent and can switch to the wrong model under rapid consecutive /model changes.

Useful? React with 👍 / 👎.

Valid theoretical concern. The race window is extremely narrow: shouldSwitchToLiveModel reads the flag and clearLiveModelSwitchPending clears it within the same synchronous call site in run.ts:621-641 — the only async gap is the updateSessionStore write itself. A second /model command arriving in that exact window would need to complete its own updateSessionStore write between our read and our clear.

In practice this requires sub-millisecond /model command timing during an active retry attempt, which is not a realistic user interaction pattern. A compare-and-swap guard (e.g., checking a generation counter before clearing) would add complexity disproportionate to the risk.

Noted as a potential follow-up if rapid /model switching becomes a supported UX pattern.

Resolve pending switch from selected override, not runtime model

shouldSwitchToLiveModel derives persisted via resolveLiveSessionModelSelection, which prefers runtime modelProvider/model over providerOverride/modelOverride. When /model is issued during a turn that cannot restart, liveModelSwitchPending remains set, and the run-completion path rewrites runtime model fields to the old model (updateSessionStoreAfterAgentRun in src/agents/command/session-store.ts). On the next turn, this code can interpret the old runtime model as the requested switch, so the pending flag is never consumed (or, on a retryable empty attempt, it throws LiveSessionModelSwitchError back to the old model), effectively dropping the user’s latest model choice.

Useful? React with 👍 / 👎.

This concern is about the behavior of resolveLiveSessionModelSelection (an existing function we don't modify), not the new flag mechanism. The function's preference order (runtime fields > override fields) is the existing upstream contract — changing it would be a separate refactor with its own risk surface.

In the specific scenario described: /model during an active run sets providerOverride/modelOverride AND liveModelSwitchPending = true. When the run completes and updateSessionStoreAfterAgentRun writes runtime fields, the override fields still reflect the user's choice. On the next turn, resolveLiveSessionModelSelection reads runtime fields first — but these were just written by the previous run's completion, and the override fields still carry the user's /model selection. The pending flag ensures the switch fires.

If the runtime fields happen to match the old model, the switch still works because shouldSwitchToLiveModel compares the resolved selection against the current candidate — they'll differ, and the switch throws correctly.

The edge case would require runtime fields to be overwritten to match the new model (making the switch look like a no-op) while overrides are cleared — but updateSessionStoreAfterAgentRun doesn't clear overrides, so this doesn't happen in practice.

Resolve pending switch from persisted override, not runtime

shouldSwitchToLiveModel computes the pending target via resolveLiveSessionModelSelection, which internally prefers modelProvider/model runtime fields over providerOverride/modelOverride. In the /model-during-active-run path, liveModelSwitchPending can carry into the next turn, and updateSessionStoreAfterAgentRun repopulates runtime fields with the old model from the previous run; this makes the pending switch resolve to the stale runtime model and can throw LiveSessionModelSwitchError back to the old selection, dropping the user’s requested switch. Fresh evidence in this commit is that pending-switch handling now routes through this new call site while the new flag is persisted from directive handling, so the stale runtime-vs-override conflict is now on the critical path.

Useful? React with 👍 / 👎.

Duplicate of the comment above — same concern about resolveLiveSessionModelSelection preferring runtime over override fields. See reply there. The resolution priority is the existing upstream contract; our PR doesn't modify it.