fix(agents): bound abort-path session lock release; force-release on unsettled retained writes#90065
Open
matin wants to merge 1 commit into
Open
fix(agents): bound abort-path session lock release; force-release on unsettled retained writes#90065matin wants to merge 1 commit into
matin wants to merge 1 commit into
Conversation
…unsettled retained writes (#6) A turn timeout aborts the run, but releaseHeldLockForAbort delegated to the graceful fence release, whose waitForRetainedLockIdle is unbounded. A retained transcript write pinned behind a hung provider stream (one that ignores abort, e.g. a stalled streamGenerateContent) therefore held the session .jsonl lock until the maxHoldMs watchdog (5-30 min), and every turn in between failed with SessionWriteLockTimeoutError. releaseHeldLockForAbort now races the graceful path against the abort settle bound (OPENCLAW_EMBEDDED_ABORT_SETTLE_TIMEOUT_MS). When the bound expires the underlying file lock is force-released and the controller poisoned via takeoverDetected, so the aborted run cannot perform torn writes after losing ownership. Retained writes that settle within the bound keep the existing graceful fence semantics. Prod incident: tulgey#225 (membrane 2026-06-01..03). Completes the lock-leak family of openclaw#87278 / openclaw#88623 / openclaw#89811, which release on settled aborts but not on aborts a hung provider call never lets settle. Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
openclaw-barnacle
Bot
added
agents
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 9:34 AM ET / 13:34 UTC. Summary PR surface: Source +94, Tests +67. Total +161 across 2 files. Reproducibility: yes. at source level: current main's abort release still waits on retained idle without a bound, and a retained 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: Rebuild the bounded abort-release fix on current main, preserve the deferred-release controller behavior, clear or transfer drain ownership before returning from force-release, and add real or real-component proof for the hung-provider case. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main's abort release still waits on retained idle without a bound, and a retained Is this the best way to solve the issue? No, not as-is. Bounded abort release is the right owner layer for the reported failure, but the stuck-drain branch, dirty head, and missing real or real-component proof keep this from being the best mergeable solution. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 6cb82eaab865. Label changesLabel justifications:
Evidence reviewedPR surface: Source +94, Tests +67. Total +161 across 2 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
Review history (1 earlier review cycle)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
This was referenced Jun 19, 2026
|
This pull request has been automatically marked as stale due to inactivity. |
openclaw-barnacle
Bot
added
stale
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A turn timeout aborts the embedded run, but the abort-path session-lock release is unbounded. On a deployment running a release that already contains #87278 (release lock on timeout abort), every embedded-run timeout still turned into 5–30 minutes of total unresponsiveness: each subsequent turn failed instantly with
SessionWriteLockTimeoutError("Something went wrong while processing your request"), until themaxHoldMswatchdog finally reclaimed the session.jsonllock. Starting a fresh session only helped until that session hit its own timeout.Representative trace:
The trigger was a flaky provider endpoint that accepts the connection and then hangs the stream (
streamGenerateContentthat never settles even afterrunAbortController.abort()), but any provider hang reproduces it.The gap #87278 left
releaseHeldLockForAbortdelegates to the graceful fence release, whosewaitForRetainedLockIdleis unbounded. A retained-lock transcript write pinned behind a hung provider stream keeps the retained-use count nonzero forever, so the abort release never reacheslock.release(). #87278 / #88623 / #89811 all release on aborts that settle; nothing covered an abort that a hung provider call never lets settle.Fix
releaseHeldLockForAbortnow runs a bounded variant:OPENCLAW_EMBEDDED_ABORT_SETTLE_TIMEOUT_MS, default 2s / 250ms underOPENCLAW_TEST_FAST), overridable per controller viaabortReleaseTimeoutMs.takeoverDetected, so the aborted run cannot perform torn writes after losing ownership: its laterwithSessionWriteLockthrowsEmbeddedAttemptSessionTakeoverErrorand cleanup degrades to the noop lock — the same posture as a real takeover.heldLockDrainingleak;dispose()stays unblocked).Relationship to #89673
Complementary, not overlapping. #89673 is the cross-process, contender-side
maxHoldMsreclaim insession-write-lock.ts(let a contender reclaim a lock past the holder's own recordedmaxHoldMs). This PR is the in-process abort path inattempt.session-lock.ts: it bounds the damage window at the source so the holder releases promptly on abort, rather than relying on a contender or the watchdog to reclaim minutes later. Both can land; together they close the live-but-stuck-holder hole from both sides.Abort propagation (cancelling the provider fetch/stream itself) is the other complementary half; the signal plumbing already exists in
fetch-guard.ts. This PR bounds the damage window regardless of provider behavior — the chokepoint fix.Tests
Human-run on my own setup (
OPENCLAW_TEST_FAST=1 pnpm vitest run src/agents/embedded-agent-runner/run/attempt.session-lock.test.ts):force-releases the held lock when a retained write never settles— the repro: never-settling retained write, bounded abort release, lock released once, controller poisoned.abort release stays graceful when retained writes settle within the bound— no poisoning on the happy path.attempt.session-lock.test.tssuite: 90/90.node scripts/run-tsgo.mjs -p tsconfig.json --noEmit): clean.AI-assisted (Claude Code). The tests above and the typecheck were run locally against my own OpenClaw instance.
🤖 Generated with Claude Code