fix(sessions): restore owned-write fence gate for same-lane takeover race (#86584 regression)#44
Merged
Merged
Conversation
…race (openclaw#86584) Long agent turns (notably cron turns) on 2026.6.11-boon.* throw EmbeddedAttemptSessionTakeoverError on their OWN transcript append — the paired-lane / same-lane race (upstream openclaw#86572). On the gandalf fleet box this fires ~192x/day across 54 of 84 crons. Root cause: the 5.18→6.11 merge (#31) dropped the fork [openclaw#86584] fence patch on the assumption "upstream's fence work now covers it." It does not. 6.11 carries the owned-write machinery, but the message-persist path registers owned writes via refreshAfterOwnedSessionWrite(), which keys beforeWrite off the controller's STALE fenceFingerprint field rather than a fingerprint read immediately before the append. On a long turn whose two nested async lanes each append, that stale key diverges from the real pre-append on-disk state, so the run's own write is never recognized as owned and the peer lane's fence throws a takeover on it. Fix: register the owned write keyed on a FRESH pre-append fingerprint captured at the persist callsite, reusing 6.11's hardened publishOwnedSessionFileFenceSync trust gate. - session-tool-result-guard{,-wrapper}.ts: widen onMessagePersisted to (message, {beforeWriteSnapshot}); add beforeMessagePersist hook captured immediately before originalAppend. - attempt.session-lock.ts: new controller method publishOwnedPostMessageWrite — fence-active path delegates to publishOwnedSessionFileFenceSync keyed on the fresh snapshot; pre-fence path records the user-message write as trusted state (supersedes refreshAfterOwnedSessionWrite on this callsite). Export SessionFileFingerprint + readSessionFileFingerprintSync. - attempt.ts: wire beforeMessagePersist; call ONLY publishOwnedPostMessageWrite from onMessagePersisted. Dropping the stale-keyed refreshAfterOwnedSessionWrite here is required — calling it first could launder a real external takeover into a silently dropped reply (Codex review finding). Fail-closed: publishOwnedSessionFileFenceSync records nothing and does not advance the fence unless beforeWrite matches the active fence or a recorded trusted state, so a genuine external takeover still trips. Tests: 3 regression tests (own append absorbed; external-before-append still trips; exact production hook sequence not laundered). tsgo:core clean; 360 tests pass across session-lock + guard + write-lock suites. Codex-reviewed (2 rounds); v1 laundering bug found + fixed; verdict ship-ready. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…ersist wiring Addresses cubic-dev-ai P3: the production-sequence test called publishOwnedPostMessageWrite directly, duplicating the prior controller-level test — reintroducing refreshAfterOwnedSessionWrite() at the attempt.ts persist callsite would have left it green. Rewrite it as a guard/runner composition test: build a file-backed guardSessionManager wired exactly as runEmbeddedAttempt (beforeMessagePersist snapshot -> append -> onMessagePersisted -> publishOwnedPostMessageWrite), let a foreign writer mutate the transcript, then persist the lane's own message through the guard and assert the fence still trips. Mutation-tested: injecting controller.refreshAfterOwnedSessionWrite() into onMessagePersisted flips this test red (withSessionWriteLock resolves instead of throwing), proving it now guards the exact regression class the old test could not. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…tion test check:test-types (stricter than tsgo:core) requires AssistantMessage to carry api/provider/model/usage/stopReason. Populate them so the laundering-regression test typechecks under test/tsconfig/tsconfig.core.test.json. No behavior change — mutation test still flips red when refreshAfterOwnedSessionWrite() is reintroduced. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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
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.
Summary
Fixes a regression in
2026.6.11-boon.*where long agent turns (notably cron turns) throwEmbeddedAttemptSessionTakeoverErroron their own transcript append — the paired-lane / same-lane race (upstream openclaw#86572). On the gandalf fleet box this is firing ~192×/day across 54 of 84 crons.This restores the fork [openclaw#86584] fence mechanism that was dropped in the 5.18→6.11 merge (#31) on the assumption that "upstream's fence work now covers it." It does not: 6.11 carries the owned-write machinery (
transcript-write-context,publishOwnedSessionFileFenceSync) but the message-persist path registers owned writes viarefreshAfterOwnedSessionWrite(), which keys itsbeforeWriteoff the controller's stalefenceFingerprintfield rather than a fingerprint read immediately before the append. On a long turn whose two nested async lanes each append, that stale key diverges from the real pre-append on-disk state, so the run's own write is never recognized as owned and the peer lane's fence throws a takeover on it.The fix
Register the owned write keyed on a fresh pre-append fingerprint captured at the persist callsite, reusing 6.11's existing hardened
publishOwnedSessionFileFenceSynctrust gate:session-tool-result-guard{,.-wrapper}.ts: widenonMessagePersistedto(message, { beforeWriteSnapshot })and add abeforeMessagePersisthook captured immediately before the append.attempt.session-lock.ts: new controller methodpublishOwnedPostMessageWrite(beforeWrite)— on the fence-active path delegates topublishOwnedSessionFileFenceSynckeyed on the fresh snapshot; on the pre-fence path records the user-message write as trusted state (supersedingrefreshAfterOwnedSessionWriteon this callsite). ExportSessionFileFingerprint+readSessionFileFingerprintSync.attempt.ts: wirebeforeMessagePersist+ call onlypublishOwnedPostMessageWritefromonMessagePersisted(drops the stale-keyedrefreshAfterOwnedSessionWritehere — a Codex-review finding: calling it first could launder a real external takeover into a silently dropped reply).Fail-closed property (why this can't hide a real takeover)
publishOwnedSessionFileFenceSyncrecords nothing and does not advance the fence unlessbeforeWritematches the active fence or a recorded trusted state. If an external mutation lands before the lane's append, the freshly capturedbeforeWritereflects that untrusted state → no publish → the subsequent fence still trips the genuine takeover. Covered by a negative regression test.Tests
3 new regression tests in
attempt.session-lock.test.ts:publishOwnedPostMessageWritedoes not trip the fence;tsgo:coreclean; 360 tests pass across the session-lock + guard + write-lock suites.Follow-ups (out of scope, filed separately)
Two adjacent hardening gaps surfaced during review, deferred to keep this hotfix minimal/revertible:
installPromptSubmissionLockRelease's unconditionalfinally { reacquireAfterPrompt() }can mask a provider error with a takeover error (attribution bug).classifySessionFenceAdvance(async fence-assert path) reads only the appended region and does not re-validate the prefix — the prefix-launder guard from fork commitf11af41c18covers the rewrite path but not the advance path.🤖 Generated with Claude Code
Summary by cubic
Fixes a 6.11-boon regression where long agent turns threw session takeovers on their own transcript append. Restores the owned-write fence keyed on a fresh pre-append fingerprint to stop false takeovers while still catching real ones.
publishOwnedSessionFileFenceSync.beforeMessagePersistand widensonMessagePersistedto(message, { beforeWriteSnapshot }); updates the guard and wrapper to pass this snapshot.publishOwnedPostMessageWriteon the controller and uses it instead ofrefreshAfterOwnedSessionWriteat the persist site; handles pre-fence trusted-state; exportsSessionFileFingerprintandreadSessionFileFingerprintSync.attempt.tsto snapshot before append and call onlypublishOwnedPostMessageWriteto avoid laundering real takeovers.beforeMessagePersist → append → onMessagePersisted) and fully types the assistant message for stricter type checks, proving a genuine external write still trips.Written for commit 7e874f3. Summary will update on new commits.