Repository navigation
fix(ui-server): record a Claude memory save in the journal's observation order - #2888
Merged
Merged
Conversation
…ion order Since journal commits moved to the write worker (#2814, #2830), persistClaudeMemory awaits several commits before it records the saved note, and nothing ordered it against the turn's finish or a committing observation: - finish could remove the turn from `active` while a save was still committing, so recordClaudeMemory refused the already-saved note with "memory observation requires an active turn" and the turn ended partial with the note unconfirmed; - a save requested after finish passed the up-front `active` check, wrote the external note, and only then was refused; - an observation reads a turn's sequence before its commit and stores the result after, so a memory record in between reused an observed sequence (and its steering_change dedupe key). persistClaudeMemory and recordClaudeMemory now run through the journal's serialize order beside observations and finish. A save requested before finish is recorded before the turn ends; one requested after is refused before it saves; a record takes the next free sequence. The refusal for an inactive turn is unchanged. Co-Authored-By: Claude Opus 5.5 <[email protected]>
… changeset The memory target type alias leaked into the ui-server API report; spell the target inline as before. Citations follow the moved lines. Co-Authored-By: Claude Opus 5.5 <[email protected]>
… turn The Claude backend awaits copy-out before its turn ends (releaseTerminal -> finishWorker -> worker.finish), and persistClaudeMemory may return a promise since #2830. The journal runtime fixture still called persist as if it were synchronous and ended the turn at once (#2889). Co-Authored-By: Claude Opus 5.5 <[email protected]>
…started no runs) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…bservation-044 # Conflicts: # docs/decisions/governed-writable-workers.md # docs/decisions/managed-worktrees.md # docs/plans/storage-lifecycle.md
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.
Closes #2893
Closes #2889
Fixes the main-red regression from #2830. The failing cell is
turn-journal-runtime.test.ts"persisted external memory uses the existing reader and authenticated recovery".What changed
This PR fixes two things.
persistClaudeMemorymay return a promise. The fixture still calledmemoryState.persist(...)as if it were synchronous and then calledfinishat once. In production, the Claude backend awaits copy-out before its turn ends:releaseTerminal/emitTerminal→finishWorker→worker.finish()→completions, which includestate.persist(packages/ui-backend-claude/src/turn-runner.ts,worker-spawn.ts). The ui-react: the journal runtime test finishes its memory turn before the awaited memory record, failing two cells #2889 hypothesis is therefore correct about the trigger. This is a test-ordering bug, and production's Claude path does not strand a save atfinish. Awaiting does not weaken the test. It now models the production order exactly.persistClaudeMemoryandrecordClaudeMemorynow run in the journal'sserializeorder, beside observations andfinish(packages/ui-server/src/brain/turn-journal.ts:514-544, plus the internalrecordClaudeMemory).The product defect: memory records reuse an observed sequence
observe()readsturn.sequencebefore itsrecordObservationcommit and assigns the result after it. Before #2830 those steps were synchronous. Now a commit sits between them. ArecordClaudeMemorythat lands in that window takes++turn.sequencefrom the stale base. It reuses an observed change's sequence and the${turnId}:${sequence}steering_change dedupe key, so the journal UI andversion(…, sequence, side)can resolve the wrong change.Production reaches this window. A Claude memory save is recorded at worker exit, while the 15 s scope timer or another active turn's
toolEndedobservation can be committing. Revert recovery is a second caller (turn-revert.ts,const target).Journal-API hardening (same change, not reachable through the Claude backend)
The same ordering also makes the journal API safe for a caller that does not await:
finishruns is recorded before the turn ends.finishis refused beforesave()writes the external note. Before, it passed the up-frontactivecheck, wrote the note, and was refused only at the record step.The refusal for an inactive turn and its message are unchanged.
Why this shape
recordClaudeMemoryaccept inactive turns. This would hide genuinely late observations, so I did not do it.serialize. It is the order thatbegin,finishand observations already share. Memory persistence never waits on a held operation: its writes go throughcall/retain, nottail. So it cannot deadlock. The publicrecordClaudeMemoryserializes the internal function. Persistence, which is already serialized, calls the internal function directly.turn-journal.ts.bindSession,bindModel,toolStarted/toolEnded,coverPaths,finishReserved,retainBefore,observeUserPolicy,attachWorktreeandobserveare either synchronous before their first await or already ordered. The Activity recorder has its ownorderedWritesorder.docs/decisions/sqlite-write-worker.md. Citations moved by the edit are refreshed. The public signatures are unchanged: the API report is identical, and noCONTRACT:change was needed.Proof
Started from
origin/maind9f4405. Mergedorigin/main5322d30 (citation-only conflicts, re-pointed). Final head a10edb3.Failing-first. The three new tests in
packages/ui-server/tests/claude-memory-journal.test.tswere run on unfixedorigin/main, and again onb192ca0c0itself in a temporary detached worktree. All three fail for the claimed reason in both runs:a memory record made while an observation of its turn commits takes the next free sequencefails withMEMORY_RECORD_NEVER_REUSES_AN_OBSERVED_SEQUENCE. It received[["claude-memory://counsel.md",1],["harbour.md",1]]. This is the product defect.a memory save in flight when its turn finishes is recorded before the turn endsfails with… memory observation requires an active turnatrecordClaudeMemory←persistClaudeMemory. This is the main-red stack.a memory save requested after its turn's finish is refused before it savesfails withReceived message: "… memory observation requires an active turn". The late save had passed the guard and written the note.The runtime cell. Before either change,
-t 'persisted external memory'failed 2/2 locally. With only the test await, on the unfixedorigin/mainjournal, it passes 2/2. With only the journal fix, on the unawaited test, it passes 2/2. With both, the full file passes 24/24.Mutations (restored after each run):
persistClaudeMemoryunserialized, as(async …)(). The in-flight test fails withmemory observation requires an active turn, and the late-save test fails withtoThrow(expected).recordClaudeMemorynot serialized. The sequence test fails withMEMORY_RECORD_NEVER_REUSES_AN_OBSERVED_SEQUENCE.Final head a10edb3 (after merging main):
bun run test packages/ui-server: 3955 tests in 331 files, exit 0.bun run test ./tests: 1954 tests in 177 files, exit 0.turn-journal-runtime.test.ts: 24/24.bun run check:pr --base origin/main: exit 0.CIandcontractboth success.tsc --noEmit,bun run lint, and the citation, api-surface and memory tests (105/105) are clean.Before the merge:
bun run test packages/ui-server: 3939 pass, 0 fail (on ebe204d; later commits change only the inline type, docs, the changeset and the ui-react test).claude-memory-journal.test.ts: 9/9.tests/api-surface.test.tsplus memory tests: 61/61.decision-citations: 44/44.ui-backend-claudememory tests (provider-denied-memory,worker-save-refused) plus the ui-react runtime file: 33/33.bun run test ./testson c8e7c17: exit 0, 1954 tests in 177 files.Penelope/Odysseus … admissionappear only as output from an inner planned-failure fixture, and they appear the same way on main.api-report/ui-server.txt, because my type alias leaked into the report. Fixed in c8e7c17 by spelling the type inline.policy-write-boundary"claude governed". This failure is intermittent on cleanorigin/maintoo: 1 of 2 isolated runs failed there, while 2 isolated runs on this head passed. Filed as tests: policy-write-boundary claude governed native Write intermittently refused as not governed #2899.tsc --noEmitandbun run lintare clean.bun run check:pr --base origin/main: exit 0 on c8e7c17 and on the final head 8de2ab3.Checklist
docs/reflect the change, or nothing documented moved.docs/decisions/provider-history.md): the provider-history guard passes, or nothing on a turn's path changed.Anything left open
policy-write-boundaryflake. It is pre-existing and needs a reproduction.docs/process/evidence/test-wait-review-partition.jsoncitesturn-journal.ts:297-301for the 15 000 ms scope interval. That citation was already off onmain; the interval is atbegin, now lines 367-370. It is evidence data rather than adocs/citation, so I left it unchanged.🤖 Generated with Claude Code