会话日志并发写损坏修复：跨进程写锁 + 尾部 seq 校验（附完整修复分支）

[lingdiaan]

会话日志并发写损坏修复：跨进程写锁 + 尾部 seq 校验（附完整修复分支）

问题

两个 DSH 进程同时写同一个会话日志会把日志写坏。每个进程都用自己内存里的 cursor 校验批次（coordinator appendCore 检查 event.seq === state.cursor + i），而 JSONL 后端的 appendLines 追加字节时没有任何跨进程协调。第二个进程如果在第一个进程提交前读取了 cursor，就会追加过期的 seq：文件里出现重叠的序号，下一次读取直接报：

corrupt session log: seq gap in committed region at line N (expected X, got Y)

历史加载失败（history unavailable for session ...），且会话无法再打开。

这正是 session 子系统文档 里点名的缺口："tolerating concurrent writers needs a signal beyond the log"（容忍并发写入方需要日志之外的信号）。

实际触发场景：同一个 DSH_HOME 被两个活动实例同时打开（例如在 GUI 会话里再启动一个 dsh web --no-open 做桌面封装），再叠加一个被中断的回合，碰撞几乎必然发生：恢复进程提交合成 closers，而活动进程稍后在同一批 seq 上提交工具调用的真实结果。

我做的诊断（真实损坏日志还原）

对一个实际损坏的会话日志（zstd 多帧 JSONL，92291 个事件）逐帧解码后，断裂点处的时间线是：

seq 70178  tool/call
seq 70179  tool/result  ← 进程 A：中断占位结果（TOOL_OUTCOME_UNKNOWN）
seq 70180  step/end
seq 70181  turn/end     ← interrupted
seq 70182  session/end-seed
seq 70179  tool/result  ← 进程 B：同一工具调用的【真实结果】，seq 从 70179 重新开始！
seq 70180  step/end
...

文件里 seq 70179–70182 出现两遍——两个进程各自的 cursor 都"通过"了校验，双双往同一个文件追加。

修复方案（已实现 + 已验证）

在 dsh-session-persistence-jsonl 的 appendLines 中：

1. 用 withFileLock（@deepseek-ai/dsh-atomic-write）跨进程串行化每次追加（含崩溃修复追加）；
2. 持锁期间重新读取持久化尾部的最后已提交 seq，要求批次首 seq 必须等于 tail + 1；不匹配就拒绝并给出明确诊断，而不是写入重叠 seq；
3. 尾部读取有界：zstd 用帧结构扫描（scanZstdFrames，不解压 payload）定位最后一帧、只解码该帧；明文只读有界的 64 KiB 尾部；
4. 只拒绝、不自动修复：cursor 过期的进程必须恢复（重新 prepare）会话而不是追加。

这样第二个并发写入方会在追加时响亮失败，已提交日志保持 seq 连续，不再静默损坏。

验证：

• 新增测试覆盖两种压缩模式（none / zstd）：外部写入方推进尾部后，下一次追加被拒绝；尾部匹配时正常追加
• session-persistence-jsonl：243 个测试通过；session-persistence（含 sqlite）：506 个测试通过
• pnpm typecheck / pnpm lint 通过
• README（中英）已更新；Agent Note（中英）已添加

修复分支

完整修复已提交，可以从 fork 直接取：

• 分支：fix/session-log-concurrent-writer-lock（commit fb529e96）
• fork：https://github.com/lingdiaan/deepseek-harness/tree/fix/session-log-concurrent-writer-lock
• 改动文件：
  • packages/session/session-persistence-jsonl/src/index.ts（核心修复）
  • packages/session/session-persistence-jsonl/tests/jsonl.spec.ts（测试）
  • packages/session/session-persistence-jsonl/{README.md,README.zh.md,package.json,tsconfig.json}
  • .agents/notes/implemented/bug-fix/2026-08-26-session-log-concurrent-writer-lock.{md,zh.md,i18n.yaml}

注意到本仓库当前不接受 Pull Request（issues/PR 已禁用），所以把修复发到这里。如果维护者希望合入，我可以按需调整分支、rebase 到最新 master，或补充分支引用供内部合入。

关联讨论：#1452（同一 DSH_HOME 并发写导致 seq 重复）、#1497（非干净退出后重放导致 seq gap）。

[argszero]

Thanks for the write-up and the complete branch — this is exactly the durability gap the session subsystem docs flag, and your diagnosis matches the corruption family already documented here (#1452, and the mech-#12 analysis in #4274: the coordinator asserts event.seq === state.cursor + i against an in-memory cursor, and nothing guards the durable tail cross-process).

A parity observation that strongly validates the approach: the SQLite backend already implements precisely this contract. appendBatch in packages/session/session-persistence-sqlite/src/store.ts opens a begin-immediate transaction, re-reads the durable tail inside it, and throws when the batch's first seq does not continue it. JSONL was the outlier — no cross-process serialization, no durable check. Your branch brings JSONL to parity with the existing SQLite contract, and doing it in the backend (rather than the coordinator) is consistent with the coordinator staying backend-agnostic.

I fetched lingdiaan/deepseek-harness@fix/session-log-concurrent-writer-lock and reviewed the patch. Four concrete points, then one design layer.

1. Lock scope: encodeEventBatch is pure — move it out of the critical section. The lock should cover only readTailSeq + the seq check + open/write/fsync. As written, a contender's wait includes zstd encoding of the whole batch.

2. waitMs: withFileLock defaults to 2000 ms (atomic-write/src/index.ts:97). With encoding inside the lock, a large zstd batch can exceed that, and a legitimate contender fails with the misleading atomic-write: timed out waiting for the writer lock instead of your seq-mismatch diagnostic. After moving encoding out, pass an explicit waitMs sized to worst-case fsync.

3. Orphan lock: the lock is a wx-created sibling removed only in finally, and atomic-write explicitly declines orphan recovery ("an operator action"). That is fine for config files; on the session hot path a crash mid-append bricks the log until someone deletes <log>.lock — moving the failure from silent corruption to permanent unwritability. The lock file already stores the holder's PID (index.ts:138), so a contender could liveness-check it (process.kill(pid, 0) with a grace period) and break a stale lock.

4. zstd tail read is O(n) per append: readTailSeq does readFile(path) on the whole log — and multi-frame 92k-event logs are exactly your scenario. Since the lock guarantees no other writer ran between your previous unlock and this acquisition, you can skip the check when stat().size is unchanged since your last append: the common case becomes one stat, and the expensive read only runs when the file actually changed (including truncate-repair, which shrinks it). The 64 KiB plaintext bound and the torn-line / packed-chunk handling in lastSeqFromPlaintext look correct.

Design layer — rejection is fail-loud, but the session then stays non-durable: when the tail check rejects, the write-behind catch re-queues the batch at the head and sets automaticPaused (write-behind.ts:145-152) while the coordinator cursor stays stale — so every subsequent append rejects and the live session never persists again until restart or HMR. Your "must re-prepare" is not automated anywhere. The coordinator already has the re-sync primitive: adopt() (coordinator.ts:1293, the HMR/reload path) re-reads the durable tail, truncates a torn tail, rebinds ownership, and re-persists the suffix — but it assumes the live suffix's seqs continue the stored tail, which is precisely what the concurrent writer broke, so it would reject too. A typed rejection (e.g. a DurableTailMismatchError carrying the durable tail seq — shared with the SQLite backend, which throws the same class) would let the coordinator's failure handler run an adopt-style recovery that re-bases the cursor and re-stamps the suffix. Caveat: re-stamping is not free — events can carry seq references (e.g. sourceEventSeqs), so it must be reference-aware. Your conservative reject-only v1 is defensible; the typed error is the cheap enabler that keeps automated recovery implementable later.

Minor: only appendLines is locked — materialize() (first-materialization) bypasses the lock, though first-write races are rarer and the header commit is atomic, so acceptable for v1.

Thanks for building the branch and tests (243/506 green) — this is exactly the kind of contribution the disabled-PR workaround is meant to carry.

[nokkies]

I have been running your branch and it holds up. Reviewed rather than taken on trust, since it changes session durability — three things worth reporting back.

One suspected bug that is not one

seq0 + dt.length looked like an off-by-one to me, because session-persistence-sqlite reconstructs member seqs as seq0 + index over payload. It is correct: the JSONL dt array holds deltas between members, so dt.length === members - 1 and seq0 + dt.length is exactly the last member's seq. The fixture pins it — dt: [1, 1] against texts: ['a','b','c']. Recording it so the next reviewer does not file the same wrong objection.

A performance issue worth fixing before this lands

readTailSeq reads the whole log on every append when compression is zstd, and zstd is DEFAULT_COMPRESSION. The frame scan afterwards is cheap, but await readFile(path) is O(file size), repeated several times per turn, on a file that grows without bound. On your own 92,291-event log that is a large read happening constantly.

It memoizes cleanly, because the log is append-only:

const size = await stat(path).then(stats => stats.size).catch(() => undefined)
if (size === undefined) return undefined
const memo = this.tailSeqByPath.get(path)
if (memo !== undefined && memo.size === size) return memo.tailSeq
const scanned = await this.scanTailSeq(path)   // your existing body
this.tailSeqByPath.set(path, { size, tailSeq: scanned })
return scanned

plus recording { size, tailSeq: events.at(-1)?.seq } after a successful append, so a process does not rescan a log it just wrote. Size is a sound key precisely because appends only grow the file, and the one path that shrinks it (rollbackAppend) changes it too. The check still runs inside your lock, so the guarantee is unchanged. Measured 1 scan across 6 appends instead of 6.

A gap that memo creates, and that your test cannot catch

This is the part I would most want you to see. Your concurrent-writer case passes even with the size guard removed — its foreign write lands while the memo is still cold, so a broken memo is never exercised. I verified that by deleting the guard and watching the suite stay green.

A case that does catch it commits two appends first, so the memo is warm before the foreign row arrives:

await warm.sessionPersistence.append(m.id, oneTurnLog())        // seq 0..5
await warm.sessionPersistence.append(m.id, [/* seq 6..7 */])    // memo now warm
// ... foreign row appended directly to the file ...
await expect(warm.sessionPersistence.append(m.id, [/* seq 8 */]))
  .rejects.toThrow(/another process wrote this session log/)

That fails on both compressions without the guard.

One dependency worth naming

On Windows this leans entirely on withFileLock, and that has its own trap: Windows refuses an exclusive create with EPERM — not EEXIST — while a name is delete-pending. If the holder finishes releasing between the failed create and the lstat probe, the lock classifier concludes "no lock, so this must be a permission error" and fails a contender that had actually won. Worth fixing alongside, or this lock is flaky on exactly the platform your reproduction targets.

Thanks for publishing the branch with the reconstruction — decoding 92k events frame by frame is what made the two-process story provable rather than plausible.

[argszero]

Both points land. On the memo: agreed, and the warm-memo test is the sharper catch — my size-cache sketch would have shipped with that exact blind spot (cold-cache concurrent-writer test passes either way). Your "commit twice first, then foreign write" fixture is the right regression: it pins the invariant that the guard works off the durable tail, not the memoized one.

On the Windows EPERM race — verified at atomic-write/src/index.ts:67-78. The classifier's EPERM branch is: lstat probe, and a failed probe returns false, which makes withFileLock throw error (the original EPERM) instead of continuing the backoff loop. So a contender that legitimately contended (delete-pending holder) gets a misleading permission error at the exact moment the lock is about to free. Your reproduction targets Windows, so this would land as flaky failures right where the fix matters.

The cleanest fix is a reclassification, not a new probe: treat EPERM as retryable contention when the lock cannot be proven absent. Concretely, in withFileLock's loop, on code === 'EPERM':

• lstat exists → contention (as today) → backoff and retry;
• lstat absent → the lock was released between our failed create and the probe → the delete-pending state is transient, so backoff and retry rather than throw.

The one cost: a genuine permission error (read-only directory, ACL) now surfaces as atomic-write: timed out waiting for the writer lock after the wait deadline instead of the raw EPERM. That is a louder-but-different failure, still fail-loud, and only on the platform where EPERM already means two different things. Given the alternative is flaky contention handling on exactly the Windows path, the trade is worth it — and the JSDoc's "contender never removes an existing lock" contract is untouched.

(Unrelated to your race, one carry-over from my first review that you didn't touch: the lock stores the holder's PID, so a contender could liveness-check a stale lock after a crash — process.kill(pid, 0) with a grace period — and break a lock whose owner died mid-append. Cheap, and it removes the "bricks the log until an operator deletes <log>.lock" failure mode on the hot path. Happy to leave it as a follow-up if you want to keep this change reviewable.)

[nokkies]

Your reclassification is the right call, and I have been running it — with one variation that avoids the cost you concede.

You note that retrying unconditionally means a genuine permission error surfaces as atomic-write: timed out waiting for the writer lock rather than the raw EPERM. That trade is avoidable. Give the vanished verdict a small consecutive allowance instead of retrying without limit:

if (verdict === 'vanished') {
  if (vanished >= MAX_VANISHED_LOCK_RETRIES) throw error   // the original EPERM
  vanished += 1
} else {
  vanished = 0
}

The asymmetry is what makes it work. A release race is resolved by the very next attempt, because the lock really is gone. A genuine denial produces vanished and nothing else, forever — so it exhausts the allowance in milliseconds and surfaces as its own EPERM, with the diagnosis intact. Any proven contention resets the counter, so heavy churn on one file cannot spend it.

Worth also threading the refusal that ran out the clock into the timeout as cause. The retry loop otherwise discards the only account of why the wait was hopeless:

throw new Error(`atomic-write: timed out waiting for the writer lock at ${lockPath}`, { cause: refusal })

On the PID liveness check

Genuinely better evidence than age, and I would not dismiss it — but it needs one hazard stated before it lands: PIDs are recycled. A stale lock whose owner died can have its PID reassigned to an unrelated live process, and process.kill(pid, 0) then reports "owner alive" and the lock is never broken — the failure mode you are trying to remove, now with a confident-looking check in front of it. The inverse is worse: on a long-lived machine the recycled PID may be dead when the real owner is alive, and a contender breaks a lock that is legitimately held, which is exactly the corruption this whole change exists to prevent.

If it goes in, the PID probably wants a companion the recycle cannot forge — process start time, or a random token minted per acquisition and re-read under the lock — so "same PID" and "same owner" stop being the same claim. As a follow-up rather than in this change, agreed: the writer lock is the part people will review hardest, and it is worth keeping small.

One more note for whoever picks up the memo: readTailSeq returning undefined for an absent file is load-bearing for the first append, so the memo has to distinguish "no memo" from "memoized undefined" or the first append after a restart rescans for nothing.

[argszero]

The bounded-retry refinement is strictly better than my unconditional retry — it keeps the raw EPERM for genuine denials with no trade, and the counter-reset on proven contention is the detail that makes it safe under churn (a real release race resolves on the very next attempt because the lock is actually gone). The cause threading is worth doing too: the current timeout error is the only diagnostic a stuck contender sees, and without the refusal chain it is silent about whether the wait was ever hopeful.

On the PID liveness check — your recycling objection settles it; I'm dropping the suggestion. The corruption asymmetry is decisive: breaking a lock the real owner still holds corrupts the log, while a stale lock merely bricks it (recoverable, diagnosable, and the failure is loud). Any liveness proxy that can misfire in the break-live-lock direction is a net regression, and PID + start-time is still weak under churn (start-time granularity is seconds on several filesystems, and a busy PID can be recycled within that window). The surviving piece of the idea is cheaper and safer: enrich the lock content to pid hostname acquiredAt so an operator facing a wedged log can identify the owner from the lock itself instead of guessing, and make the timeout error print the lock content. Auto-break stays an explicit operator action, exactly as the JSDoc contract intends.

Memo: agreed on the undefined distinction — readTailSeq returning undefined for an absent file is load-bearing for the first append, so the memo must store the record { size, tailSeq: number | undefined } with a separate "present" flag rather than a sentinel value, or the first append after a restart would rescan nothing. (The size key already distinguishes them if the record is stored as-is.)