fix: retry session init after transient failure instead of pinning sessionId (#411)

[ranxianglei]

Fixes #411

Problem (verified)

ensureSessionInitialized assigns state.sessionId = sessionId synchronously before its first await. If initialization then fails, every subsequent getOrCreate for that session hits the idempotency fast path (state.sessionId === sessionId) forever — the session silently runs on a fresh empty SessionState for the process lifetime (persisted compression blocks, model context limit, and nudge baselines never loaded).

Trigger analysis correction (from triage): on master, getSessionParentId (lib/state/utils.ts:72–82) swallows all host-API errors → returns undefined, and loadSessionState swallowed all errors including I/O failures → returned null. So a host-API hiccup alone cannot throw; the dominant real-world path is a transient FS read error (EACCES/EIO/EMFILE) being conflated with "file absent": init silently takes the fresh/fork branch, pins sessionId, and the session never retries. Secondary throw paths: saveSessionState queue rejection and sync rebuildCompressionState throws.

Fix (two parts — one part alone is insufficient)

1. lib/state/state.ts — ensureSessionInitialized wraps the extracted runSessionInitialization(...) in try/catch: on failure it clears state.sessionId = null and re-throws, so the next request retries initialization. The synchronous assignment is kept deliberately (it is the concurrency guard against racing resetters); resetSessionState at the top of each attempt wipes partial mutations before re-running.
2. lib/state/persistence.ts — loadSessionState no longer conflates I/O errors with absent/corrupt files:
   • ENOENT → resolves null silently (unchanged)
   • EISDIR/ENOTDIR → warn + null (unchanged, preserves behavior for corrupted layouts where the state path is a directory)
   • any other read error → re-throws (new: transient failures surface to the caller instead of silently taking the fresh branch)
   • JSON parse/validation errors → warn + null (unchanged)
   • The existsSync pre-check was removed: it returns false for unsearchable directories too, which masked permission loss as "file absent".

All sessionId consumers are null-safe (verified: hooks.ts guard, truncate-tools fallback, saveSessionState no-op). Compress-side callers (prepareSession, decompress) propagate the error to the tool result — an explicit tool error beats compressing on stale/missing persisted state.

Tests (red-first verified)

• tests/registry.test.ts: regression test seeds real persisted state (via createSessionState + saveSessionState, bypassing the registry under test), chmods the state file 0o000, asserts the first getOrCreate fails AND leaves sessionId === null, restores perms, asserts the second getOrCreate loads modelContextLimit. Fails against unfixed code (first run confirmed; an initial seeding-via-registry design flaw that masked the bug was caught during the red phase).
• tests/persistence.test.ts: unreadable file rejects with EACCES; unsearchable storage directory rejects with EACCES (the removed pre-check would have hidden this case). Both root-self-skipping, like the registry test.
• Full suite: 1277 pass / 0 fail (baseline 1274 + 3 new). Typecheck + build clean.

Follow-up (tracked in devlog WORKLOG.md)

PR #408's runSessionInitialization wrapper has the same missing failure handling and needs the same treatment when it lands (its "failed init coalesces" test may pin the old semantics).

Devlog: devlog/2026-09-16_init-failure-retry/

[github-actions]

📦 Built Plugin Artifact

Branch: 2026-09-16_init-failure-retry (0476d45)

Option A — Install from npm PR tag (recommended)

opencode plugin opencode-acp@pr-412 --global

Each push to this PR publishes a new version under the pr-412 npm tag.

Option B — Install from GitHub

opencode plugin "github:ranxianglei/opencode-acp#2026-09-16_init-failure-retry" --global

Option C — Download artifact

1. Download the artifact from the Actions run
2. Extract the tarball and install:

tar xzf opencode-acp-pr412.tgz
cp -r package/dist ~/.cache/opencode/packages/opencode-acp@latest/node_modules/opencode-acp/dist

3. Restart opencode to pick up changes.

---

This comment is automatically updated on each push.

[ranxianglei]

🤖 Powered by ework · qwen3.8-27b

PR Review — verified independently, two direct fixes pushed to the branch

Diff cleanliness: clean. Single fix commit 700c276 (+ author's 55e7fd7 try/finally test-hygiene follow-up), no unrelated files, no whitespace churn, package.json version untouched, devlog folder matches branch name with REQ.md + WORKLOG.md present.

What I verified (re-derived, not taken on trust)

Bug is real and correctly layered. On master, ensureSessionInitialized assigns state.sessionId = sessionId synchronously before its first await (lib/state/state.ts); any later failure leaves the idempotency fast path set forever. I also confirmed the trigger correction from triage holds: getSessionParentId swallows all host-API errors → undefined, and old loadSessionState swallowed all read errors → null — so the dominant real-world path is transient FS error conflated with "file absent", exactly as described. The two-part structure is genuinely necessary: resetting sessionId alone would rarely fire because the silent-null path never throws.

Fix logic verified line-by-line:

• lib/state/state.ts:306-324 — catch clears state.sessionId = null then re-throws; sync assignment deliberately kept as the concurrency guard; partial mutations wiped by resetSessionState() at retry start.
• lib/state/persistence.ts:286-301 — ENOENT → silent null; EISDIR/ENOTDIR → warn + null; other I/O → re-throw; parse/validation → warn + null (separate try). error?.code is safe for non-Error throws (optional chaining → falls through to verbatim re-throw). existsSync removal changes nothing besides the intended unsearchable-dir case (TOCTOU race resolved identically pre/post). Fork-recovery call site (state.ts:384) lands inside its existing try/catch → replay fallback, net behavior unchanged.
• Null-safety of all sessionId consumers confirmed by grep: saveSessionState no-op on empty id (persistence.ts:174), truncate-tools.ts:68 ?? "unknown", hooks.ts:455 guard; everything else is log fields. A failed-init state can therefore persist zero garbage — every save path is a no-op while sessionId === null.
• Compress-side propagation confirmed: prepareSession (lib/compress/pipeline.ts:70) and decompress (lib/compress/decompress.ts:58) both await ensureSessionInitialized uncaught → explicit tool error instead of compressing on stale/missing state. Correct tradeoff.
• No new concurrency hazard: the fast-path-without-await for an in-flight init is pre-existing and intentionally preserved; documented in the new JSDoc.

Tests — red-first reproduced myself. In a master worktree with the branch's test files dropped in: all 3 new tests FAIL against unfixed code (not ok 12/13/19), everything else passes. On the branch: typecheck clean, full suite 1278 pass / 0 fail, build clean, prettier clean on all touched files. Test design checks out per §5.6: real-source imports, side-effect assertions (modelContextLimit and sessionId), root-self-skip portable, chmod restore now in finally (author's 55e7fd7).

Duplicate screening: no other open issue/PR covers this — #412 is the sole PR referencing #411.

Independent second review (dual-review requirement)

A separate agent adversarially reviewed all six risk areas (partial-mutation window, concurrency, error classification, type safety, state integrity, test quality): all PASS, no blockers/majors. Findings: one minor follow-up candidate + nits.

Fixed directly on the branch (commit 0476d45, pushed)

1. Missing test (nit→fixed): the newly added EISDIR/ENOTDIR warn+null branch had no coverage. Added tests/persistence.test.ts "loadSessionState resolves null when the state path is a directory (Transient init failure permanently suppresses persisted-state load for that session #411)" (portable: POSIX EISDIR / Windows ENOTDIR both map to warn+null). Suite now 1278 pass / 0 fail locally.
2. Devlog sync (nit→fixed): REQ.md acceptance checkboxes were stale (implemented items still [ ]) — updated; WORKLOG records the follow-ups below.

Follow-ups (tracked in WORKLOG, none block merge)

• Write-only FS failure mode (minor): if reads succeed but the trailing save fails persistently (e.g., ENOSPC), every request re-runs full init and wipes accumulated in-memory compression work each cycle. Defensible fail-loud behavior today; candidate refinement: scope the sessionId reset to failures at-or-before loadSessionState so a successful load survives trailing-save outages.
• Author already recorded two deferred items (friendlier tool-error message for transient I/O; rate-limiting the per-request ERROR log under persistent failure).
• Pre-existing (not this PR): PR fix: serialize same-session state initialization and transforms (#404) #408's runSessionInitialization wrapper needs the same treatment when it lands.

Status & next step

CI on original tip 700c276: all green (pr-validation, test 22/24, e2e, build, artifact). My review commit 0476d45: pr-validation + build-artifact already green, full CI run in progress. Once it goes green, the PR is ready to merge — merge is a human-only operation; please click Merge yourself: #412

---

独立复核确认了该修复：会话初始化瞬时失败后 sessionId 被永久锁死、持久化状态再也无法加载的问题真实存在且根因分层正确（loadSessionState 把 I/O 错误吞成 null 是主路径，失败不清 sessionId 是放大器，两部分缺一不可）；我在未修复代码上复现了 3 个新测试全部红、分支上全量 1278 测试通过，并已在 PR 分支直接补了缺失的 EISDIR 布局保留测试和 devlog 状态同步（0476d45），双 agent 审查均通过、无阻塞项——CI 跑绿后即可合并。

[ranxianglei]

🤖 Powered by ework · qwen3.8-27b

CI confirmed green — ready for human merge ✅

Final tip 0476d45 (fix + try/finally test hygiene + my review-fix commit): all three workflows completed success — pr-validation, CI (test 22/24 + e2e + build), build-artifact. mergeable_state: clean, branch protection satisfied.

Nothing outstanding from the review side. When you're ready, please merge it yourself (human-only operation): #412

---

最终提交 0476d45 的全部 CI（含 e2e）已跑绿、分支保护检查全部通过，审查无遗留问题，可以合并。

[ranxianglei]

继续