fix(session): hold a single-writer lock on session JSONL files

[jasonkneen]

Problem

SessionManager._persist (packages/coding-agent/src/core/session-manager.ts ~1016-1043) appends to the session JSONL with appendFileSync. The only exclusivity was openSync(..., "wx") on the very first flush (~1031). If two processes resume the same session (two terminals running --resume / --session <same file>, or a respawned subagent child racing its predecessor), both append. Their parentId chains interleave and the tree forks silently. That breaks the invariant that each durable record has exactly one writer.

Fix

• New packages/coding-agent/src/core/session-writer-lock.ts adds an advisory <sessionFile>.lock. It is created with O_EXCL (openSync(path, "wx", 0o600)) and holds {pid, hostname, startedAt}.
  • The existing repo helpers were not reused. proper-lockfile (settings/auth/trust) and the step-cron wx + mtime lock are short-lived mutexes with mtime staleness. A lock held for a whole session would need a keepalive timer. The conflict error also could not name the owning pid/host. The step-cron openSync(..., "wx") shape is kept, but mtime staleness is replaced with a pid liveness check.
  • A lock is stale when it is on the same host and process.kill(pid, 0) gives ESRCH. A stale lock is reclaimed with unlink and one more wx attempt. EPERM counts as alive. A lock from another host is always treated as held.
  • Locks are reference-counted per process. Two managers in the same process share the lock, for example the session selector's rename writing to the active session. When the transient one is disposed, the active one still owns the file.
  • On process.once("exit") the process makes a best-effort release of every lock it still holds, but only when the lock file still names this pid.
• SessionManager takes the lock lazily, right before its first write: at the top of _persist (both the append path and the first wx flush) and in _rewriteFile (empty-file init, migration rewrite, createBranchedSession). No lock is created for in-memory managers or for sessions that are only opened to read or list, such as export-html and the session picker.
• The lock is released in setSessionFile, newSession, createBranchedSession (before it switches to the new file) and the new SessionManager.dispose(). dispose() only releases the lock. The manager takes it again if it writes later.
• AgentSessionRuntime.teardownCurrent and dispose now call sessionManager.dispose(). A resume, fork or quit therefore frees the outgoing file for other processes instead of holding it until this process exits.
• interactive-mode rename-from-selector now calls mgr.dispose() after appendSessionInfo, so the transient manager does not keep the renamed file locked.
• Error on conflict: Session <file> is already open in another process (pid X on host Y). Close it or use --fork to continue in a new session. If that process is gone, delete <file>.lock.
• .jsonl.lock does not end in .jsonl, so list, listAll and findMostRecentSession already skip it. A test now pins that.

Test

• packages/coding-agent/test/session-manager/writer-lock.test.ts has 10 tests:
  • the lock contents
  • a live foreign pid (process.ppid) makes append throw the clear error and leaves the file unchanged
  • a foreign host is refused
  • a dead pid (from a finished spawnSync child) is reclaimed
  • the lock is released on newSession and setSessionFile
  • the lock is released on createBranchedSession
  • two managers in one process share the lock
  • a read-only open creates no lock
  • in-memory sessions create no lock
  • listing ignores .lock
• packages/coding-agent/test/agent-session-writer-lock.test.ts is an end-to-end check. AgentSession.prompt() on a resumed session that another process has locked rejects with the lock error. The model is never called and the file is unchanged. This shows the error reaches the interactive loop's showError and print mode's prompt rejection handling.
• Before the fix, 8 of 9 tests failed (manager.dispose is not a function, and expected false to be true for the lock file existing). After the fix, all pass.

Risk / behaviour change

• The conflict appears on first write, not at open. A second process can --resume a locked session and view it. The error shows when the user sends the first prompt. That prompt fails before any model call and nothing is persisted. The CLI flows were not redesigned to lock at open time, because SessionManager.open is also used read-only (export-html, the picker).
• A crash or hard kill (SIGKILL, power loss) leaves a .lock file behind. The next writer on the same host reclaims it automatically because the pid is dead. No SIGINT/SIGTERM handlers were added. A normal exit releases locks through the exit hook.
• Pid reuse: if the OS gives the dead owner's pid to an unrelated live process, the lock looks held. The error names the lock path so the user can delete it.
• Cross-host (shared or network home dirs): locks from another host are never reclaimed automatically. Delete the lock manually if that host is gone.
• An unreadable or corrupt lock file is treated as held, and the error names the path to delete.
• Same-process re-entrancy: in-process managers share the lock by reference count. Two managers in one process writing the same file can still interleave, as they could before. The lock protects only between processes.
• In-memory state on conflict: when a write throws, the entry has already been pushed onto the manager's in-memory fileEntries. The prompt fails, so this matches the existing behaviour of a failed append.
• Subagents: each child runs --mode rpc --session-id <subagent-uuid> with its own session id and file, so children never share the parent's file. A lane respawn resumes the same id. If the old child is somehow still alive, the new one now fails loudly instead of interleaving. A dead predecessor's lock is reclaimed.

Merge note: interaction between #168 (session writer lock) and #172 (subagent inherit settings)

When both fix/harness-session-writer-lock and fix/harness-subagent-inherit-settings are merged, replacing an idle keep-alive subagent lane (on a parent permission change) resumes its transcript right after the old child is told to stop. The old child may still hold the session writer lock, so the replacement can fail with "session is already open". Whichever merges second should await the old child's exit before spawning the replacement.

https://claude.ai/code/session_01GUdnnHEaDThHUATSwXBpV9

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate lock ownership, cleanup, and read-only mutation issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1

Open (3)

• Prevent read-only session loading from mutating JSONL files · New
• Do not delete locks when owner data is missing or malformed · New
• Dispose abandoned replacement session manager during lifecycle races · New

What changed in this PR

Adds lazy advisory single-writer locks for session JSONL files, including stale-lock recovery, lifecycle integration, and test coverage.

Changes:

• Adds PID/hostname ownership, reference counting, and exit cleanup.
• Integrates locking with persistence, session transitions, runtime teardown, and renaming.
• Adds unit and end-to-end conflict tests.

File	Summary and final findings
packages/​coding-agent/​test/​session-manager/​writer-lock.test.ts	Adds session-manager lock coverage.
packages/​coding-agent/​test/​agent-session-writer-lock.test.ts	Adds end-to-end conflict handling coverage.
packages/​coding-agent/​src/​core/​session-writer-lock.ts	Implements locking. Critical (3 votes): malformed or replaced locks may be removed without confirmed ownership. Moderate (1 vote): owner publication is non-atomic.
packages/​coding-agent/​src/​core/​session-manager.ts	Integrates lock lifecycle. Critical (1 vote): read-only loading can modify an unterminated file without a lock. Moderate (1 vote): export migration locks may remain held. Moderate (1 vote): failed branch replacement paths may abandon locks.
packages/​coding-agent/​src/​core/​agent-session-runtime.ts	Adds runtime cleanup. Moderate (1 vote): teardown callback errors can skip disposal. Moderate (1 vote): quit callback errors can skip disposal. Moderate (2 votes): lifecycle-race replacements can retain locks.
apps/​cli/​src/​ui/​interactive-mode.ts	Disposes transient rename managers.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.