fix(embedded-runner): recheck owned-writes before session takeover (#91236)#92202
Closed
DouglasCherry wants to merge 1 commit into
Closed
fix(embedded-runner): recheck owned-writes before session takeover (#91236)#92202DouglasCherry wants to merge 1 commit into
DouglasCherry wants to merge 1 commit into
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Long-running embedded agent turns abort with
EmbeddedAttemptSessionTakeoverError: session file changed while embedded prompt lock was released. In production this fires deterministically on multi-step runs dispatched through a wrapper lane — daily scheduled (cron-nested) jobs and delivery (main) turns — across multiple agents:Each failure is logged twice — once for the wrapper lane and once for
session:agent:<id>:…:run:<runId>— referencing the same session file and same runId. A directopenclaw agentrun (no wrapper lane) completes; only wrapper-lane runs fail. This is the same symptom as #84460 (closed not-planned).Root cause
A single run is driven by two same-process lanes (the wrapper lane and the session lane), both appending to one session JSONL. In
assertSessionFileFence()the fence samplesownedSessionFileWritesonce. Between a peer lane'sfswrite and itsrecordOwnedSessionFileWrite()call there is a window where the fence reads the new on-disk state but the owned-write record is still stale — so the peer lane's own append fails both the owned-write check andsessionFenceAdvanceIsBenign()(the appended tool-call lines aren'tisTranscriptOnlyOpenClawAssistantLine), and a takeover is wrongly declared.Fix
Before declaring a takeover, yield once (
setImmediate) and re-sampleownedSessionFileWrites/ the current fingerprint. If a same-process owned write now matches, the change is recognised as benign.Why this is safe:
ownedSessionFileWritesis per-process. A genuine foreign / cross-instance takeover never populates this process's map, so the re-check still won't find a matching owned write and the takeover is still raised. The change only affects the case where our own concurrent lane made the write — i.e. it narrows a false positive without weakening cross-instance protection. Worst case it adds a single microtask before an error that was about to throw anyway.Test
Added
does not declare a takeover for a concurrent same-process owned write (#91236)— the benign complement of the existingkeeps the session fence active after releasing for sessions_yield abort cleanuptest (which exercises a foreign append). Not executed locally — needs CI.Open questions for maintainers
setImmediatere-check be bounded/looped for very tight interleavings, or is a single yield sufficient given the owned-write record is synchronous after the write resolves?