Repository navigation
Transient init failure permanently suppresses persisted-state load for that session #411
Description
来源: #404 / PR #408 分析时发现（dual review of PR #408, reviewer 2 minor-major finding）
Problem
ensureSessionInitialized assigns state.sessionId = sessionId synchronously before its first await. If initialization then fails with a transient error (host API hiccup during getSessionParentId / loadSessionState), the state object is left with sessionId set but no loaded persisted state.
Every subsequent getOrCreate call for that session hits the idempotency fast path (state.sessionId === sessionId) and returns immediately — re-initialization never happens again for the process lifetime. The session silently runs on a fresh empty SessionState: persisted compression blocks, model context limit, and nudge baselines are never loaded.
Evidence
- Code path:
lib/state/state.ts— synchronoussessionIdassignment precedes all awaits inrunSessionInitialization; wrapper's failure path only cleans the inflight map, not the fast-path condition. - This behavior is pre-existing (the original implementation had the same synchronous assignment) and was deliberately preserved by PR fix: serialize same-session state initialization and transforms #404's coalescing change (its test "failed init coalesces" pins current semantics). Flagged as minor-major by an independent reviewer because a one-time transient failure has process-lifetime consequences.
Impact
After any transient first-request init failure: compression blocks from previous runs are invisible to the model (no summaries injected, refs re-assigned from scratch, potential duplicate block allocation on next compress), context-limit thresholds run against undefined until the next system.transform, and nothing logs a loud signal beyond the single init error.
Suggested fix
On init failure, either reset state.sessionId = null (so the next request retries init) or track an explicit initFailed flag that the fast path checks. Add a regression test: fail init once (mock client rejects), assert the next getOrCreate retries and succeeds.
Activity
ranxianglei commentedon Sep 16, 2026
[bot] 🏷 Triage verdict: structural bug confirmed — fixed in PR #412. With one important correction to the trigger analysis.
What I verified
The core claim is exactly right:
ensureSessionInitializedassignsstate.sessionId = sessionIdsynchronously before its first await (lib/state/state.ts), so any later failure leaves the fast-path condition set permanently and re-initialization never happens for the process lifetime. Pre-existing, and indeed preserved by PR #408's coalescing change.Correction to the trigger analysis: on master, a host-API hiccup during
getSessionParentId/loadSessionStateactually cannot throw — both swallow all errors (getSessionParentIdcatches → returns undefined atlib/state/utils.ts:72–82;loadSessionStatecaught everything, including I/O failures → returned null + warn). So the dominant real-world path is not "init throws" but "transient FS read error conflated with file absent": EACCES/EIO/EMFILE on the state file →loadSessionStatenull → init silently takes the fresh/fork branch →sessionIdpinned → session runs on empty state forever. This matches the reported impact exactly. Secondary genuine throw paths do exist (saveSessionStatequeue rejection atlib/state/persistence.ts:169–232, syncrebuildCompressionStatethrows), but they fire after in-memory fields are already applied, so low impact.Why the suggested fix needed a second part
Your proposed fix (reset
state.sessionId = nullon failure) is necessary but insufficient alone: becauseloadSessionStatenever throws on I/O errors, the reset would rarely fire — the silent-null path keeps pinning the session. So the fix is two parts:lib/state/state.ts— wrap init in try/catch; on failure clearstate.sessionId = nulland re-throw so the next request retries. The sync assignment stays deliberately (it's the concurrency guard against racing resetters);resetSessionStateat retry start wipes partial mutations.lib/state/persistence.ts—loadSessionStatenow classifies read failures instead of swallowing them: ENOENT → silent null (unchanged); EISDIR/ENOTDIR → warn + null (unchanged); any other I/O error → re-throws; parse/validation errors → warn + null (unchanged). Also removed theexistsSyncpre-check, which returned false for unsearchable directories too — masking permission loss as "file absent".All
sessionIdconsumers verified null-safe; compress-side callers (prepareSession,decompress) surface the error to the tool result rather than compressing on stale state.Tests (red-first verified)
tests/registry.test.ts: seeds real persisted state, chmods the file 0o000, asserts firstgetOrCreatefails and leavessessionId === null, restores perms, asserts second call loadsmodelContextLimit. Confirmed failing against unfixed code. (Fun detail: my first seeding attempt went through the registry under test itself, which masked the bug via the fast path — caught during the red phase.)tests/persistence.test.ts: unreadable file rejects EACCES; unsearchable storage dir rejects EACCES. Root-self-skipping.devlog/2026-09-16_init-failure-retry/.Follow-up: PR #408's
runSessionInitializationwrapper has the same missing failure handling and its "failed init coalesces" test may pin the old semantics — needs the same treatment when that PR lands (tracked in the WORKLOG).📎 PR: #412
修复了会话初始化瞬时失败后 sessionId 被永久锁死、持久化状态再也无法加载的问题（根因是 loadSessionState 把 I/O 错误吞成 null、与文件缺失混同，加上失败后未重置 sessionId），采用"失败清空 sessionId 允许重试 + loadSessionState 对真实 I/O 错误上抛"的双层修复，回归测试已验证修复前必失败，可以合并。
test: restore state-file permissions in finally on assertion failure (#…
ranxianglei commentedon Sep 16, 2026
[bot] 🏷 Independent review of PR #412 complete — verdict: APPROVE. (First reviewer agent was unavailable due to a provider error; a general-purpose reviewer ran the full AGENTS.md §5.3/§5.6 checklist instead.)
Key things verified beyond my own implementation:
sessionId = nullclear runs synchronously inside the single in-flight init's rejection handler; a second concurrent caller fast-paths on the set flag exactly as on master, so no interleaved double-init is possible.origin/master— ENOENT/EISDIR/parse-error outcomes are equivalent (null); only log-message wording differs, arguably clearer.sessionId === nullandmodelContextLimit === 314159).Applied directly from review (commit
55e7fd7):try/finallyaround the failed-init assertions intests/registry.test.tsso the permission restore runs even when an assertion throws mid-test.Two deferred follow-ups (recorded in devlog WORKLOG.md, acceptable trade-offs for v1): raw I/O errors now surface to the model as tool errors from
prepareSession/decompress(intended — better than master's silent empty state; could get friendlier wording later), and while an FS condition persists each request logs one init ERROR (could be rate-limited later).PR #412 is ready for your merge — CI is running the checks.
独立评审通过（无新增竞态、行为保持、回归测试静态验证修复前必失败），已按评审意见在 PR 分支直接补了一个测试清理的小修复（try/finally 恢复文件权限），可以合并。