fix: persist steering at message admission - #3633
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the simplification at the core of this is sound, and most of the deleted CLI fallback tests genuinely remove an unreachable client-owned authority. Production has a single SessionManager construction that always supplies Host authority, and these packages are private, so that removal holds. Two items below.
[P1] Steering admission has no atomic recoverable proof across the durable-append boundary
Admission durably appends the UserMessage before it mutates the in-memory queue and before the submit receipt exists. A Host crash inside that window leaves neither a queue entry nor any recoverable admission proof, while the durable append has already landed.
Retry then consults only root-source receipts and provider-consumption events, and SQLite here permits duplicate message IDs. So the same cut produces two different bad outcomes depending on timing: a message the user saw accepted can be lost outright, or retry can append the same message identity a second time before any single delivery has occurred.
The fix needs one atomic, recoverable admission fact spanning the append and the queue/receipt update, rather than an ordering that is only correct when no fault occurs. Please add a restart/fault regression that cuts exactly at the append→queue/receipt boundary; an ordering assertion in the happy path cannot show this.
[P2] The CLI never renders the durable append, so a successful steering send becomes invisible
The CLI deletes the pending Steering row but does not render the durable append live. subscription.transcript_advanced has no consumer anywhere under packages/cli — I checked, there are zero references — and transcript reload only happens on a tool result, a reconnect, or a terminal state.
During a long-running step the consequence is that the user presses Enter, the text leaves the editor, and the message then appears nowhere at all until one of those events happens. From the user's side a successful send is indistinguishable from a dropped one.
The rewritten test asserts the pending row's absence but never asserts that a conversation entry replaces it, so it passes on exactly the broken behaviour.
One observation, not a blocker
The fallback QueueEnqueueOutcome representation and RuntimeKernel's complete embedded queue authority both survive this change. That is consistent with the deletions being safe today, but it means the second authority still exists in the tree even though nothing in production reaches it.
ran: Core → Storage → Runtime → Runtime Host → CLI builds, all green; Runtime Host 50/50, CLI 235/235, Runtime steering 39/39; deletion-reachability probes for the removed fallback tests; independent confirmation that packages/cli contains no transcript_advanced consumer.
did-not-run: the hosted exact-head run 32647365821 (job 97213551805) was still in progress at the time of writing, so no terminal CI result is claimed here. Neither finding depends on it.
|
Follow-up on the CI caveat in my review: the hosted run at |
bf1b2f6 to
07a11c7
Compare
jackwener
left a comment
There was a problem hiding this comment.
Reviewed on behalf of WAWQAQ under the standing Kabi review authorization, scoped to exact head 220bdebbae3d42c6e2c01f18f81fe80655fbadf5.
NO-GO: 2×P1. The old CLI visibility P2 is closed: subscription.transcript_advanced now reloads canonical stored messages and publishes a transcript replacement rather than a live-only echo. The large deletion is also a real simplification: it removes CLI fallback/live-steering ownership and leaves Host queue + durable transcript as the authority.
Exact-head hosted test is completed/success; the PR is OPEN, non-draft, and mergeable against current main. Local focused verification passed: message coordinator 40/40, Host message integration 4/4, Runtime steering recovery 8/8, CLI durable/transcript 23/23, Desktop contract 1/1. The two inline P1s still block approval. No merge performed.
| } | ||
| const durableAdmission = isCurrentEpoch | ||
| ? undefined | ||
| : await this.#durableProof.readSteeringAdmission(input.sessionId, input.messageId); |
There was a problem hiding this comment.
[P1] This recovery proof has no production caller that can present its key after the fault it is meant to recover. A crash after commitSteeringAdmission durably appends the user row but before the queue push/submit receipt causes the mutation request to end as outcome-unknown. The reconnecting client does not replay mutations; CLI and Desktop both generate messageId inside each new submission, so a user resend uses the new epoch and a new ID and never reaches this old-epoch/same-ID branch. A production-shaped fault probe therefore produced durable admissions [oldId, newId]: the transcript contains two identical user rows, while only newId enters the delivery queue. The added test manually reuses the old ID, proving a protocol capability that the clients do not retain or invoke. Please either make this durable fact self-rehydrate admission on Host recovery, or persist and automatically replay the exact original submit identity/payload from the client, with a reconnect fault test through the real client seam.
| continue; | ||
| } | ||
| const result = { disposition, queueRevision: candidateRevision + 1 } as const; | ||
| if (disposition === 'steering' && !durableAdmission) { |
There was a problem hiding this comment.
[P1] Durability is added only on the direct submit path. queue.entry.promote still moves a follow-up into state.steering and immediately makes it non-retractable without calling commitSteeringAdmission. The real Desktop path is the queue card's “Send now” action. If the user stops before the next provider step pulls it (or the Host dies), there is no transcript record that can recover the message. Commit the steering admission before exposing the promoted state, or keep the entry retractable until that commit succeeds; add a promote→Stop/crash regression.
| } | ||
|
|
||
| #discardQueuedSteering(state: SessionState): void { | ||
| const entries = state.steering; |
There was a problem hiding this comment.
[P1 — same promotion finding] This silent discard is the reachable loss sink for an undurable promoted entry. Stop returns snapshots only from #retractFollowups; #discardQueuedSteering releases entries without returning them. Diagnostic path: follow-up submit → promote succeeds with zero steering admissions → Stop yields retracted=[], steering=[], followup=[]. Because promotion created no durable row, the text is neither in the transcript nor restored to the draft.
220bdeb to
d526e8b
Compare
jackwener
left a comment
There was a problem hiding this comment.
Representative review action on behalf of @WAWQAQ; scope: fix-head re-review of the two previously reported Runtime Host steering-recovery findings; exact head d526e8bfd01762561bc80a45e979efbd0c5aeccf.
Result: NO-GO — 1×P1 remains.
The previous promote → Stop content-loss finding is closed: promotion now durably commits the steering admission before removing the follow-up entry, and a failed durability write leaves the entry retractable. Stop can still discard the live steering queue, but it no longer deletes the durable transcript row or fabricates provider delivery.
The automatic-recovery finding remains: the helper exists, but its only production call is behind a new-root admission path that the target crash state cannot reach. A production Host restart reproduction retains the durable user message but never delivers it to the model (durable row = 1, provider steering proof = 0, root admission chain = 1, where recovery requires a successor/root chain of 2). See the two inline comments for the wiring and mutation evidence.
Required repair boundary: do not fix this by replaying every durable steering row that lacks a provider proof. Crash-before-queue-commit and an intentional Stop-discard currently leave the same durable-row shape. Blind replay would resend a message after the user explicitly pressed Stop. The repair must durably distinguish recovery-eligible queue state from intentional discard (or provide an equivalent fence) while preserving Stop’s “no successor” contract.
Local verification on this head: focused coordinator tests 41/41, Runtime Host full suite green, and root build green. The PR is OPEN/non-draft/MERGEABLE; exact-head hosted test is completed/success. This review is COMMENT only; no approval or merge.
| try { | ||
| this.messages.reserveRootTurn(messageIdentity); | ||
| messageReserved = true; | ||
| await this.messages.recoverRootTurn(messageIdentity); |
There was a problem hiding this comment.
[P1] This is the only production call to recoverRootTurn, but it sits inside prepareAdmittedTurn, which is reached for recovery only by a rootReplayAdmission planned when no Run exists. The target crash is after the durable steering append and before queue commit, so an active Run necessarily already exists (commitSteeringAdmission rejects otherwise). Startup first terminalizes interrupted Runs, then recovery checks existing-Run admissions without re-entering this new-root path. Therefore the guard here cannot run for the state it is meant to repair. Reproduction: seed an existing Run plus one durable steering admission and no queue/provider proof, restart the real Host, and observe durable=1 / provider proof=0 / root chain=1 instead of the expected successor chain of 2. Existing guards do not help because control never reaches this call.
| state.phase = 'open'; | ||
| } | ||
|
|
||
| async recoverRootTurn(identity: RuntimeMessageRunIdentity): Promise<void> { |
There was a problem hiding this comment.
[P1 corroborating evidence] The helper’s internal live-ID and immutable-proof checks are reasonable, but the new unit test calls this method directly and does not prove startup reachability. Deleting the sole production call at root-turn-coordinator.ts:1993 left the complete Runtime Host suite green; focused coordinator/execution-host tests remained 45/45 and root-turn-coordinator tests 49/49. This mutation demonstrates that the shipped recovery behavior is untested and currently disconnected, not merely that one edge-case assertion is missing.
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Persist a Host-owned pending steering admission before exposing it to the active Run. Recover unresolved admissions after interrupted Runs are terminalized, and settle them when provider consumption, successor root admission, or an intentional stop makes replay unnecessary. Add a production startup regression for the crash between durable admission and the in-memory queue commit. Generated-by: Maka
d526e8b to
e6ce9ca
Compare
Keep the newly added item_completed fixture assertion on the current persisted message decoder so the storage workspace builds on main. Generated-by: Maka
Route ordinary messages through the Runtime Host admission API, stop creating renderer-owned empty live turns, and keep transcript publication within the active subscription. Preserve visible live tails across sends, return sparse transcripts to latest before submission, and remove redundant live/durable filtering and unconfirmed-arm bookkeeping. Generated-by: Codex
Update the real-framing execution test to cover the ordinary-message operation now used by Desktop instead of retaining the removed direct turn-start contract. Generated-by: Codex
Keep revision and Skill inputs on the turn-start control path while ordinary messages use Host admission. Rebind Side Chat to the Host-owned Turn identity from either the first event or admission response, and assert remount continuity without requiring an unobservable React intermediate paint. Generated-by: Codex
Generated-by: Codex
ARE404
left a comment
There was a problem hiding this comment.
Exact-head re-review at 064157d3f09039e7f9842be39d447db8f18a59c4. NO-GO: 3×P1, 1×P3.
The previously reported disconnected restart recovery is now wired at Host startup, and the normal promote path correctly waits for durable transcript admission before exposing steering. Successful Stop also now preserves the transcript row while suppressing delivery. The three inline P1s are new crash/recovery boundary defects in that replacement design.
[P3 — documented standard] This diff changes renderer/UI behavior and stories, but the PR description has no before/after screenshots or recording as required by CONTRIBUTING.md:85.
No other documented-standard violations. The admission-time transcript model also semantically supersedes #3316's synthetic in-flight projection; textual overlap there is not an additional correctness finding. git diff --check is clean, and the only expected hosted workflow for these paths is CI test, which is terminal SUCCESS (run 32668976841, job 97266724286).
| } | ||
| return failure('not_found', 'Message queue entry does not exist'); | ||
| } | ||
| const pending = await this.#receipts.commitPendingSteering({ |
There was a problem hiding this comment.
[P1] A failed promotion can later execute a message that the user successfully retracted. This call first creates the pending recovery record; if commitSteeringAdmission then rejects, the new regression at message-coordinator.test.ts:664-693 deliberately leaves the entry in followup. queue.retract removes that entry via #retractFollowups, but never settles this pending record, so recoverPendingAfterHostRestart() materializes and starts it after a restart. Repro: promote → inject transcript-append failure → retract the still-visible follow-up → restart; the supposedly retracted instruction becomes a recovered root. A normal retry cannot reliably heal the cut either: callers generate a fresh admittedAt, while the production SQLite store treats a timestamp change for the same session/message identity as Pending steering admission identity conflict (the in-memory fake silently returns the old record). Once this record is committed, either make the entry non-retractable/cancel the record atomically on every retract path, and make retries identity-stable.
| } | ||
| return; | ||
| } | ||
| await this.#receipts.settlePendingSteering( |
There was a problem hiding this comment.
[P1] This deletes the only restart-delivery record before the Stop fence is committed or durable. Every Stop caller awaits prepareStopFence() first; only afterward does declareStopFence() commit the in-memory queue fence and await claimRunClosure. A Host crash/failure in that window leaves the admitted user row visible in the transcript but no pending record, and startup terminalizes the interrupted run before pending-steering recovery, so the message is never delivered and no successor is created even though Stop itself never became durable. The successful-Stop behavior is fine; this is the pre-fence crash window. Couple pending settlement to a durable Stop/fence fact, or reverse the durable ordering so recovery can distinguish an accepted Stop from an interrupted attempt.
| content: aggregateMessageContent(pending.map((entry) => entry.modelContent)), | ||
| submittedContent: aggregateMessageContent(pending.map((entry) => entry.content)), | ||
| sources, | ||
| initiatingConnectionId: pending[0]!.initiatingConnectionId, |
There was a problem hiding this comment.
[P1] Restart recovery merges all pending steering for a session and binds the resulting root to only the first submitter's client capabilities. The uninterrupted handoff deliberately uses sameInitiatingClientPrefix() to split B-then-A follow-ups into separate roots, and root-turn-coordinator.test.ts verifies each root sees its own client's ephemeral tools. After a crash, two unconsumed admissions from B and A instead reach this single batch; startRecoveredSteering() calls bindConfirmedFollowup with B's ID, so A's instruction executes with B's turn-affine Host tools/capabilities. Partition recovery into the same contiguous initiating-client batches as normal terminal handoff, starting each batch under its own connection identity.
Summary
Steering was accepted into the Runtime Host queue immediately, but the transcript did not expose it until the provider pulled the queue at a later step boundary. That coupled user-visible conversation state to provider delivery state and made sends appear delayed.
This change makes successful steering admission durable before it enters the active Turn queue, so every client can render the accepted user message from the transcript immediately. Provider consumption remains separately ordered and exactly-once. Sent steering is non-retractable; an ordinary terminal race carries the same message into the successor without creating a second transcript row, while explicit Stop retains the message and suppresses an automatic successor.
The PR also removes parallel presentation and fallback paths that encoded conflicting ownership:
sessions:enqueuesteering_messageprojection from queuein_flightstateNet diff: 389 insertions and 903 deletions.
Verification
npm run format:check@maka/core,@maka/runtime,@maka/runtime-host,maka-agent, and Desktop main/preload/renderer@maka/runtimeworkspace suite: 2,989 passed, 13 skipped, 0 failed@maka/runtime-hostworkspace suite: 1,112 passed, 0 failedRepository-wide tests and the full Desktop Playwright suite were not run locally.
AI use
Select exactly one:
Tool(s) and scope: Maka authored implementation and tests, performed architecture analysis, and used delegated read-only scans to identify redundant steering paths. Each affected commit includes a
Generated-by: Makatrailer.Checklist
Does this PR entail a change in behavior?