Skip to content

fix(embedded-runner): recheck owned-writes before session takeover (#91236)#92202

Closed
DouglasCherry wants to merge 1 commit into
openclaw:mainfrom
DouglasCherry:fix/session-takeover-record-after-write-91236
Closed

fix(embedded-runner): recheck owned-writes before session takeover (#91236)#92202
DouglasCherry wants to merge 1 commit into
openclaw:mainfrom
DouglasCherry:fix/session-takeover-record-after-write-91236

Conversation

@DouglasCherry

Copy link
Copy Markdown

Status: DRAFT — proposed fix, needs validation. The fix and regression test were authored from source analysis against the harness conventions but have not been executed in my environment (deps not installed). Please treat this as a starting point for maintainer/CI review, not a verified patch. Refs #91236.

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:

lane=cron-nested durationMs=180340  agents/cashflow/sessions/<sid>.jsonl
lane=cron-nested durationMs=97280   agents/main/sessions/<sid>.jsonl
lane=cron-nested durationMs=82233   agents/collections/sessions/<sid>.jsonl
lane=cron-nested durationMs=54274   agents/credit/sessions/<sid>.jsonl

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 direct openclaw agent run (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 samples ownedSessionFileWrites once. Between a peer lane's fs write and its recordOwnedSessionFileWrite() 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 and sessionFenceAdvanceIsBenign() (the appended tool-call lines aren't isTranscriptOnlyOpenClawAssistantLine), and a takeover is wrongly declared.

Fix

Before declaring a takeover, yield once (setImmediate) and re-sample ownedSessionFileWrites / the current fingerprint. If a same-process owned write now matches, the change is recognised as benign.

Why this is safe: ownedSessionFileWrites is 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 existing keeps the session fence active after releasing for sessions_yield abort cleanup test (which exercises a foreign append). Not executed locally — needs CI.

Open questions for maintainers

  • Is the cleaner fix at the dispatch layer (ensure one run executes in exactly one lane, with the wrapper delegating to/awaiting the session lane) rather than in the fence? That would remove the concurrent-writer condition entirely.
  • Should the setImmediate re-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?

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling clownfish Tracked by Clownfish automation merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.