Skip to content

feat(runtime-host): separate queued successor turns - #4130

Open
me2seeks wants to merge 3 commits into
apache:mainfrom
me2seeks:feat/4129-separate-next-turns
Open

feat(runtime-host): separate queued successor turns#4130
me2seeks wants to merge 3 commits into
apache:mainfrom
me2seeks:feat/4129-separate-next-turns

Conversation

@me2seeks

Copy link
Copy Markdown
Contributor

Summary

  • run each accepted next_turn message as its own FIFO successor Turn
  • keep unconsumed steering messages together ahead of explicit successor work
  • preserve queued messages across successor transitions and Runtime Host recovery
  • apply the same per-successor capacity contract to submit, update, and promote operations

Fixes#4129

Verification

  • npm run lint
  • npm run format:check
  • npm --workspace @maka/runtime-host run typecheck
  • npm --workspace @maka/runtime-host run build
  • 133 affected Runtime Host tests passed with a worktree-local TMPDIR

Full-workspace npm run typecheck reaches and passes Runtime Host, but currently fails in unrelated UI/Desktop files due to existing interface/build-output mismatches.

Review

Two independent reviews covered lifecycle/recovery correctness and design/simplicity. Both reviewers reported no blocking findings after fixes and re-review.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex assisted with code investigation, implementation, tests, and review preparation.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actionsgithub-actionsBot added the effort/M Under 500 readable lines label Aug 29, 2026

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for separating explicit next-turn messages into distinct successor Turns. That matches the user-visible queue semantics better than aggregating unrelated future messages, and the normal FIFO and capacity paths look coherent.

I left one inline P2 on recovery identity. After a restart, the remaining queued admissions retain their original identity in durable storage, while their restored in-memory entries are rewritten to the active successor identity. Editing or promoting one of those entries then fails with an identity conflict. The queue remains durable and the failure is recoverable, so I do not think this rises to P1, but preserving the original admission identity would make the recovery path consistent.

Reviewed with Codex and two independent @Reviewer agents. I verified the exact head, queue and successor state transitions, durable admission identity, restart behavior, and passing CI.

中文对照

谢谢你把明确的 next-turn 消息拆成各自独立的 successor Turn。与把无关的未来消息聚合到一起相比,这更符合用户看到的队列语义,正常路径下的 FIFO 和容量判断也基本自洽。

我在恢复身份边界留了一条 P2 行内评论。重启后,剩余队列项在持久化存储中仍保留原始身份,但恢复出的内存 entry 被改写成当前 successor 的身份,因此编辑或 promote 这些队列项时会发生身份冲突。队列本身仍然持久化,而且这个失败可以恢复,所以我认为它不到 P1;不过保留原始 admission identity 会让恢复路径保持一致。

本次审查使用了 Codex 和两个独立的 @Reviewer 子代理;我核对了精确 head、队列与 successor 状态转换、持久化 admission 身份、重启行为和 CI 结果。

Comment threadpackages/runtime-host/src/server/message-coordinator.ts Outdated
@Astro-Han

Copy link
Copy Markdown
Contributor

This is a synthesis of the independent review by @Haoqing_Reviewer at exact head ba41d1ccd2863dd264856b89a786df7e9d14bdbb (base 16d06f217de5d57e6825b58fdb7a0ffe668cf092, merge-ref d3937c1dc2641eb648ca2ff661d8d39fc72c2e2d clean). I verified the drift and the exact-head CI myself; the file:line findings below are from Haoqing's sealed review.

What I checked myself:

  • Confirmed head ba41d1c is still current (base 16d06f2, state OPEN/MERGEABLE, reviewDecision APPROVED but that approve is bound to older adcd42b6 and does not cover this head — stale, not terminal).
  • Checked exact-head CI: test SUCCESS (33263264872), windows_recovery SUCCESS (33263264968), label implied PASS; 2 required checks terminal green.

Findings from Haoqing's review:

Overall: NO-GO — 1×P1 blocker (worst P1)

  • [P1] packages/runtime-host/src/server/message-coordinator.ts:518-545 — queued entries retained across successor are not rebound to current execution ownership, causing lost retry or duplicate execution after restart.beginTerminalTransition() consumes only the queue head successor while commitNextRoot() increments generation without updating remaining entries. When the user promotes a remaining follow-up in a new Turn, the code writes the old turnId/runId back to durable admission and moves the old generation unchanged into steering (:1399-1412).

    Two reachable consequences with file:line证据:

    • nack path: provider nacks before persistence, :1903-1921 requires entry.generation === state.generation to re-queue; the carried entry belongs to the previous generation, so it is released directly and the user message disappears from the current queue.
    • restart duplication: provider has acked and the current Turn's immutable steering proof exists, but settlement only scans admissions whose durable identity equals the current run (:622-629) and recovery requires proof's turn/run to equal the old identity in admission (:686-699). The admission is therefore not deleted; the next Host restart re-delivers the already-delivered message as root, risking duplicate side effects.

    Both paths were reproduced with one-off regression tests: nack → expected re-queue but actual empty; ack + steering proof of current run → startRecoveredMessages receives the same message again after recovery. Tests were removed and the worktree has no source changes after verification. Fix direction: separate immutable submission identity from current execution ownership and update current generation on cross-successor/promote; add the two carried-follow-up promote regression tests (nack and restart).

Change scope checked

  • The PR splits each explicit next_turn into its own root, keeps steering prefix aggregation for the same finishing Turn, and adjusts recovery/capacity and related tests/docs. Review covered 6 changed files, full diff, and the root-turn-coordinator successor admission/handoff, Runtime ai-sdk-backend pull/ack/nack, and SQLite message admission identity/update/handoff paths.

Verification reported

  • Builds: core, storage, runtime, runtime-host — PASS.
  • message-coordinator.test.js: 51/51 PASS.
  • runtime-host full suite: 1348 passed / 1 failed / 9 skipped — the single failure is an unrelated managed-sandbox tool-result path check, reproducible in isolation and not on the PR path.
  • GitHub test / windows_recovery: PASS (exact-head, see above).
  • git diff --check and git show --check for both commits: PASS.

Self-check gaps noted by reviewer

  • Current plan is not yet correct, therefore not optimal; no deletable production code was found that would be safe to remove without losing the goal.
  • Missing the two cross-successor promote regression tests; other new tests for FIFO/recovery/capacity are valuable.
  • Ownership model needs a small deepening — rewriting durable turn/run to the current run would reintroduce the identity conflict fixed in the previous version.
  • Current head is therefore not merge-ready.
  • Undetermined: Windows/macOS behavior not verified locally (Windows hosted check passed); no schema change or migration was found.

Gate: Despite green CI and a stale APPROVED on adcd42b6, the exact head ba41d1c has an open P1 blocker (cross-successor ownership rebinding + two regression tests) and must not merge until fixed and re-reviewed at a new head.


Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@Astro-Han

Copy link
Copy Markdown
Contributor

This is a synthesis of the independent review by @Haoqing_Reviewer at exact head fe5a94ee2fb6bb17a5e28437d9b5b598d52867b9 (base 16d06f217de5d57e6825b58fdb7a0ffe668cf092, previous head ba41d1c). Delta ba41d1c..fe5a94ee is 2 files; I verified drift and exact-head checks myself — the file:line findings below are from Haoqing's sealed review.

What I checked myself:

  • Confirmed drift ba41d1c → fe5a94ee (message-coordinator.ts contention with main 8aed381e), state OPEN, mergeable UNKNOWN / CONFLICTING, mergeStateStatus UNKNOWN, reviewDecision APPROVED still bound to adcd42b6 (stale, does not cover fe5a94ee).
  • Checked exact-head checks: gh pr checks reports no checks on this branch; no terminal test/windows_recovery for fe5a94ee; statusCheckRollup=[].

Findings from Haoqing's sealed review:

Overall: NO-GO — 0×P1 open (previous P1 closed) but merge blocked by CONFLICTING + missing checks

  • [Closed] Previous P1 — cross-successor ownership rebinding (message-coordinator.ts:518-545, :1399-1412, :1903-1921, :622-629, :686-699) is fixed in fe5a94ee. The immutable submission identity is now separated from mutable execution generation and generation is updated on promote/successor handoff. Two new one-off regressions were added and both pass after the fix:

    • nack path: carried follow-up with old generation is now correctly re-queued instead of released
    • restart path: startRecoveredMessages no longer re-delivers a message already settled with an immutable steering proof
      The fix was verified with the expanded message-coordinator tests and a local reproduction that previously showed nack → empty and ack+proof → duplicate now passes. No schema change or migration was found, and the broader ba41d1c..fe5a94ee diff remains limited to the coordinator, admission storage uniqueness/cancellation, immutable proof and successor handoff.
  • [Gate still blocked] Content conflict with main.fe5a94ee conflicts with latest main 8aed381e in packages/runtime-host/src/server/message-coordinator.ts (also reported by GitHub as CONFLICTING). The old merge ref d3937c1d (for ba41d1c) cannot be used. Until the conflict is resolved and a new head is frozen, correctness of the final merged code (this fix + main's new message execution/proven-root handoff) cannot be verified.

Verification reported by reviewer

  • Builds core/storage/runtime/runtime-host: PASS
  • message-coordinator.test.js incremental two files + full targeted checks: PASS; git diff --check / commit --check: PASS; worktree clean
  • runtime-host full: 1350 passed / 1 failed / 9 skipped — the one failure is an unrelated execution-model-composition.test.ts managed-sandbox assertion, reproducible in isolation and not on PR path (local suite not fully green but not blocking this PR)
  • GitHub checks: none on fe5a94ee (see self-check above)

Undetermined

  • Final post-rebase correctness after resolving the message-coordinator.ts conflict
  • Hosted CI for the new head (test/windows_recovery)
  • Windows/macOS local behavior (hosted checks missing)

Gate: Previous P1 is considered closed based on the file:line evidence and two regressions, but head fe5a94ee is not merge-ready — it is CONFLICTING and has no terminal required checks. Author must rebase onto latest main, produce a new head, and re-freeze for review; task #400 stays in_review.


Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

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

Labels

effort/MUnder 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(runtime-host): run queued next-turn messages as separate turns

2 participants

@me2seeks@Astro-Han