fix(session-lock): allow reentrant acquire from inner transcript writers#96000
fix(session-lock): allow reentrant acquire from inner transcript writers#96000t2wei wants to merge 1 commit into
Conversation
Three mid-attempt transcript writers (compact, tool-result-truncation,
transcript-rewrite) acquired the session write lock without
`allowReentrant: true`. The outer embedded-attempt lock (acquired by
`createEmbeddedAttemptSessionLockController`) is non-reentrant by design,
so same-pid contention deadlocks for 60s when one of the inner writers
is invoked mid-turn.
The deadlock fires reliably in scenarios where a large tool result
triggers context overflow precheck in run.ts, which calls
`truncateOversizedToolResultsInSession` or compaction while the attempt
still holds the outer lock. Symptom in the journal:
```
[diagnostic] lane task error: lane=main durationMs=~290s
error="SessionWriteLockTimeoutError: session file locked (timeout 60000ms):
pid=<same gateway pid> alive=true ageMs=~60s
/path/<session>.jsonl.lock"
Embedded agent failed before reply: session file locked ...
```
`transcript-append` already uses `allowReentrant: true` for the same
reason — applying the same fix to the other three inner writers.
|
Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 5:46 PM ET / 21:46 UTC. Summary PR surface: Source +13. Total +13 across 3 files. Reproducibility: yes. at source level. Current main holds the embedded attempt session lock without reentry, the three inner writer paths still acquire the same session lock without Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the broad Do we have a high-confidence way to reproduce the issue? Yes, at source level. Current main holds the embedded attempt session lock without reentry, the three inner writer paths still acquire the same session lock without Is this the best way to solve the issue? Mostly yes, but not merge-ready as submitted. Adding Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 3811001d2783. Label changesLabel justifications:
Evidence reviewedPR surface: Source +13. Total +13 across 3 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Additional live reproduction from a Feishu direct session on 2026-06-29. Environment:
Observed user-facing error at 2026-06-29 14:46 GMT+8: Important detail: A few minutes later the same lock path still existed. Its metadata was: {
"pid": 346578,
"createdAt": "2026-06-29T06:51:09.800Z",
"maxHoldMs": 300000,
"starttime": 131306136
}Operational context from
This looks like same-process/live-gateway session lock contention or unreleased/reentrant lock behavior rather than stale dead-PID cleanup. This may be relevant to this PR because the observed owner is the same live gateway PID and the symptom is the exact 60s |
clawsweeper
Bot
added
the
merge-risk: 🚨 session-state
|
This pull request has been automatically marked as stale due to inactivity. |
|
Closing due to inactivity. |
2 participants
What Problem This Solves
Three mid-attempt transcript writers —
compact.ts,tool-result-truncation.ts,transcript-rewrite.ts— acquire the session write lock withoutallowReentrant: true. The outer embedded-attempt lock (acquired bycreateEmbeddedAttemptSessionLockController) is non-reentrant by design, so same-pid contention deadlocks for 60 seconds when one of the inner writers is invoked mid-turn.Why This Change Was Made
Reliable repro: in a long-running session where the agent produces a large tool result that triggers context overflow precheck in
run.ts, the precheck callstruncateOversizedToolResultsInSessionorcompactEmbeddedAgentSessionDirectwhile the attempt still holds the outer lock. The inner writer hits the file lock manager, sees the lock held by the same PID, and waits the full `DEFAULT_SESSION_WRITE_LOCK_ACQUIRE_TIMEOUT_MS` (60s) before throwing.src/config/sessions/transcript-append.tsalready uses `allowReentrant: true` for the same reason (writes session events during the attempt's turn). This PR extends the same fix to the other three inner writers that share the same locking pattern.User Impact
Without this fix, agents running parallel
sessions_spawnwith substantial tool outputs hit `SessionWriteLockTimeoutError` mid-turn:```text
[diagnostic] lane task error: lane=main durationMs=~290s
error="SessionWriteLockTimeoutError: session file locked (timeout 60000ms):
pid= alive=true ageMs=~60s
/path/.jsonl.lock"
Embedded agent failed before reply: session file locked ...
```
The agent fails to deliver its reply (the followup reply path falls back to other channels), turn is wasted, and the user sees a stalled response after ~5 minutes.
Evidence
alive=true, same gateway PID).SessionWriteLockTimeoutErrorevents in the inner-writer path.pnpm tsgopasses locally.transcript-append.ts:325(`allowReentrant: true` with a comment explaining the same reasoning).Test plan
pnpm tsgo)sessions_spawnwith large tool results — no longer deadlocksExisting tests in
src/agents/session-write-lock.test.tsalready cover reentrant acquire semantics (lines around 192/197/221/226), so the lock primitive itself is exercised.🤖 AI-assisted PR. Diagnosis + patch produced with Claude Code; locked behavior verified against production journals.