fix(runtime-host): unify durable message lifecycle - #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.
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.
d526e8b to
e6ce9ca
Compare
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).
ca08e9a to
7d6a5fb
Compare
7d6a5fb to
1343cee
Compare
ARE404
left a comment
There was a problem hiding this comment.
Exact-head re-review at 1343cee25bddb40b84456d7f15f08d74d8ffc0f7. NO-GO: 3×P2, 1×P3.
The three P1 crash/recovery defects from my prior review are closed: promotion is now atomic with transcript materialization, Stop suppression waits for immutable terminal Stop proof, and queued successors execute under the explicit durable Session capability contract. The replacement design still has three independent product regressions, documented inline.
[P3 — documented standard] CONTRIBUTING.md requires before/after screenshots or a recording for UI changes. This PR changes observable CLI pending/transcript rendering and Desktop existing-session submission behavior, but the description explicitly omits media; focused tests do not satisfy that evidence rule.
git diff --check is clean. Exact-head dependency audit is SUCCESS; hosted test is still IN_PROGRESS, so I do not claim a terminal CI result.
中文摘要
本次复审锁定 1343cee2,结论为 NO-GO:3×P2、1×P3。旧 review 的三条 P1 均已关闭:promotion 与 transcript 写入已原子化,Stop 丢弃已由不可变终态事实约束,durable successor 也不再继承临时 client capability。
当前三条 P2 分别是:CLI 同一 steering 同时走 durable transcript 与 steering_message 两条渲染路径,导致重复显示;Quote Companion 在发送竞态中会把任意旧 turn 的首个事件误认成本次 turn,随后丢失真正 successor 的回复;follow-up promotion 未重新执行 queue/canonical projection 容量预检,边界大小输入可把 Host 推入 fail-stop。另有一条 P3:可观察 UI 行为已变化,但缺少 CONTRIBUTING 要求的前后截图或录屏。
当前 audit 已通过,test 仍在运行中。
M4n5ter
left a comment
There was a problem hiding this comment.
English
Review result: NO-GO (3 × P1)
The architectural direction is sound: this change genuinely removes the RuntimeKernel/CLI fallback queue authorities instead of merely moving them. However, the new unified durable authority still has three reachable crash cuts that violate the PR's no-loss/no-resurrection/restart guarantees. I reproduced each with a focused probe on this exact head.
Validation completed: Core, Storage, MCP, Runtime, and Runtime Host builds; 107 focused Runtime Host tests; 18 SQLite core-execution tests; and 5 focused AiSdk steering tests. All existing tests pass. The three added diagnostic probes fail as described in the inline comments; they were removed afterward and the worktree is clean. Hosted audit is green; hosted test was still in progress at the final freshness check.
Simplify audit: production-like changes are +1299/-1085 and test-like changes are +1443/-1929. Across the relevant RuntimeKernel and CLI files, +179/-1009 removes pendingFallback (16 → 0 references), takePendingFollowup (5 → 0), and confirmLiveTurn (21 → 0). Please preserve that single-authority direction while closing these crash cuts; reintroducing a fallback owner would be the wrong fix.
中文
审查结论:NO-GO(3 × P1)
架构方向成立:这次确实删除了 RuntimeKernel / CLI 的 fallback queue authority,不是简单搬目录。但新的统一 durable authority 仍有三个可达崩溃切点,分别破坏本 PR 承诺的“不丢消息 / 不复活 / 可重启恢复”。我在这个 exact head 上分别做了最小探针复现。
已完成验证:Core、Storage、MCP、Runtime、Runtime Host 构建;Runtime Host 相关 107 个测试;SQLite core-execution 18 个测试;AiSdk steering 定向 5 个测试,现有测试全部通过。三个临时诊断探针均按内联评论描述失败,随后已移除,worktree clean。最终刷新时 hosted audit 已绿,hosted test 仍在运行。
简化审计:production-like +1299/-1085,test-like +1443/-1929;相关 RuntimeKernel 与 CLI 文件合计 +179/-1009,并把 pendingFallback(16 → 0)、takePendingFollowup(5 → 0)、confirmLiveTurn(21 → 0)完全删掉。修复以下崩溃切点时应保留单一 authority 方向,不要重新引入 fallback owner。
jackwener
left a comment
There was a problem hiding this comment.
Kabi exact-head review under the standing WAWQAQ review authorization, scoped to 1343cee25bddb40b84456d7f15f08d74d8ffc0f7.
NO-GO — Kabi grading: 2×P1, 2×P2, 1×P3. Four defects were independently reproduced after a sealed provisional judgment. To avoid opening duplicate threads, their current-head anchors are: provider-echo recovery loss (P1), materialized-source/no-Run startup failure (P1), Stop-recovery identity resurrection (P2 in this line), and double CLI rendering of one steering identity (P2). The non-duplicate P3 is inline.
The architectural simplification itself is real: durable Host/Storage state is now the message authority, while the old CLI fallback/live-steering ownership has been removed. The repair should preserve that direction. A post-seal promotion-capacity candidate was withdrawn after boundary probing: submit preflight reserves the worst-case revision width and rejects larger candidates before promotion. The Quote Companion race was not included in this line's independent count because the final hook-level reproduction was not completed.
Verification on this head: Runtime Host 1134/1134, CLI 423/423, UI 223/223, Desktop 1339/1339; hosted test and audit are completed/success. Fresh gate: OPEN, non-draft, MERGEABLE/BLOCKED, six unresolved P0–P2 threads, and zero current or stale APPROVED reviews. COMMENT only; no approval or merge.
jackwener
left a comment
There was a problem hiding this comment.
Supplement to review 5005153696 on the unchanged exact head 1343cee25bddb40b84456d7f15f08d74d8ffc0f7: the Quote Companion hook-level probe completed before the stop instruction arrived and independently reproduced the existing P2 thread. The correct Kabi count is therefore NO-GO — 2×P1, 3×P2, 1×P3. The earlier sentence excluding this race from Kabi independent reproduction is superseded; all other findings and the promotion-capacity withdrawal remain unchanged.\n\nThe exact hook sequence was: settle turn-old; begin a second send and execute onBeforeSend while the Host result is pending (activeTurnId=null, in-flight true); deliver a late terminal event for turn-old, which claims the active ref; return turn-new, whose ID is then not installed because the ref is non-null; deliver turn-new text, which remains absent from the live projection; release the old settlement, after which the hook reports streaming false while the real Host turn remains active. The React hook plus fake Workbar production service seam reproduced this in 30.5 ms; temporary probe removed and worktree clean. COMMENT only; no approval or merge.
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
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
Generated-by: Maka
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
1343cee to
c6a899e
Compare
Generated-by: Codex
Summary
Queued messages were split across transcript rows, an in-memory Host queue, Client bindings, and recovery-time inference. A Host restart could therefore lose an accepted message, replay a discarded one, or change the capabilities under which it ran.
This change gives the lifecycle four explicit authorities:
Admission and transcript materialization now share one SQLite transaction. Promotion and reorder are durable-first. Recovery reads unresolved admissions and starts them through the normal successor-root path; it no longer scans the transcript or waits for the original Client. Retracted identities retain only their original placement and canonical content digest, so they cannot be resurrected while message contents are removed.
A queued successor executes under the durable Session contract. It does not inherit session-, turn-, call-, or initiating-Client bindings, and an explicit durable capability marker prevents online call-affine providers from being discovered later.
Stop is settled only by the immutable terminal Stop fact. If the Host dies before that fact, Stop was not confirmed and accepted messages recover; after the fact, they do not.
The parallel embedded RuntimeKernel queue and its dead admission path/tests are removed. The CLI and Desktop continue to render from canonical transcript/Host projections rather than owning fallback delivery state.
Schema migration
Verification
After rebasing onto current
main:git diff --checkRepository-wide tests and the full Desktop Playwright suite were not run locally; CI owns full coverage.
Screenshots are not included because this revision changes lifecycle and projection ownership without changing visual styling or layout. The observable transcript/queue behavior is covered by focused CLI, Desktop contract, and Host integration tests.
AI use
Tool(s) and scope: Maka and Codex contributed implementation, tests, architecture analysis, and delegated read-only reviews. Material commits include
Generated-bytrailers.Checklist
Does this PR entail a change in behavior?