Skip to content

refactor(runtime): remove the legacy child provider-retry path - #3110

Merged
Astro-Han merged 1 commit into
apache:mainfrom
chinawch007:refactor/remove-legacy-child-provider-retry
Aug 16, 2026
Merged

Astro-Han merged 1 commit into
apache:mainfrom
chinawch007:refactor/remove-legacy-child-provider-retry

Conversation

@chinawch007

Copy link
Copy Markdown
Contributor

Summary

Removes the legacy legacy_provider_retry admission lane for child provider retries so durable continuation is the only path, per #3035. Production Runtime Hosts always provide both the SQLite continuation authority and the safety inspector, so host behavior is unchanged.

  • ChildAgentRetryInput no longer carries an admissionMode; retryChildAgentWithExecution replaces the mismatch/fallback selection with a single fail-closed guard that rejects an incomplete continuation composition (authority or safety inspector missing) before any claim, Run creation, or provider dispatch.
  • Deleted planLegacyChildProviderRetry() (session-manager) and runLegacyProviderRetry() + LegacyProviderRetryRunOptions (runtime-runner); the durable claim + continuation-start block in startChildRetryClaimed and runAgentContinuation is now unconditional.
  • Provider rate-limit retry, continuation recovery, and linked-child behavior remain covered by the existing durable-path tests, unchanged.
  • Updated the two zh-CN architecture docs that documented the legacy lane.

Behavior change: a directly constructed SessionManager without a complete continuation composition no longer retries child provider failures — it fails closed. This is not a supported product surface (per #3035).

Fixes #3035

Verification

  • npm run format:check — clean (8 formatting nits auto-fixed and folded in)
  • npm run lint — 2304 files, no issues
  • npm run typecheck
  • npm --workspace @maka/runtime run build — clean
  • npm --workspace @maka/runtime run test:dist — 2983 tests, 2971 pass, 12 skip

AI use

Select exactly one:

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

Tool(s) and scope: ZCode (GLM) — analyzed the retry admission paths and issue #3035, implemented the removal across runtime/runtime-runner/session-manager and the regression test, ran the verification suites. Human contributor of record reviewed the change and is responsible for the submission.

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

Child provider retries now admit only through durable continuation. The
admission-mode selection in retryChildAgentWithExecution collapses into
a single fail-closed guard that rejects an incomplete continuation
composition before any claim or provider dispatch, and the legacy
planner and runner paths are deleted.

Generated-by: ZCode (GLM)
@coderabbitai

coderabbitai Bot commented Aug 16, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Summary

This PR removes the legacy_provider_retry admission path for child provider retries.

Child retries now use durable continuation admission only. The runtime rejects incomplete continuation composition before claims, Run creation, or provider dispatch. This prevents retries when continuation authority or safety inspection is unavailable.

The PR extends the existing durable continuation source of truth. It does not create a parallel retry path. The legacy planner, runner, fallback logic, runtime-event replay, and admission-mode selector were removed.

This is the smallest coherent solution for fail-closed behavior. The deleted legacy path and its dedicated tests can remain removed because existing coverage preserves provider rate-limit retries, continuation recovery, linked-child behavior, retry limits, backoff policy, root-turn semantics, and provider transports. The new regression test covers incomplete continuation configurations.

Validation

  • Added regression coverage for missing continuation authority and safety inspection.
  • Updated architecture documentation.
  • Focused Runtime and Runtime Host tests, type checking, and formatting validation are required by the objectives.
  • Final check results are unverified from the provided evidence.

Review-relevant risks

The public ChildAgentRetryInput.admissionMode field was removed. This can affect callers that construct this input directly and requires independent human review under repository policy.

Directly constructed SessionManager instances without continuation authority or safety inspection no longer retry child provider failures. This changes behavior outside supported Runtime Host construction and requires independent human review under repository policy.

No other protected-area effect was identified in the current diff. The person performing the merge must review the final diff, and a maintainer makes the final determination.

Walkthrough

Hosted child provider retries now use only durable continuation admission. Missing continuation authority or safety inspection causes failure before claim, run creation, or provider dispatch. The legacy retry planner, runner, and admission selector were removed.

Changes

Child retry admission

Layer / File(s) Summary
Require durable retry planning
packages/runtime/src/session-manager.ts, packages/runtime/src/__tests__/session-manager.test.ts
Child retries require continuation authority and safety inspection. They use durable continuation planning only. Tests verify incomplete configurations fail before retry creation or provider dispatch.
Enforce durable continuation execution
packages/runtime/src/runtime-kernel.ts
Continuation retries always revalidate safety, claim authority, begin continuation admission, and execute through runAdmittedRuntimeContinuation.
Remove legacy runner and document the contract
packages/runtime/src/runtime-runner.ts, docs/architecture/*.zh-CN.md
The legacy provider-retry helper and options are removed. Architecture documents describe the durable-only, fail-closed behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 83f28

The PR removes the unsupported legacy retry fallback and fails closed when continuation dependencies are incomplete; the supplied checks pass, and only minor cleanup comments remain. No actionable merge-blocking risk remains.

Suggested reviewers: astro-han

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies removal of the legacy child provider-retry path, which is the primary change.
Description check ✅ Passed The description includes the required summary, issue link, verification, AI-use declaration, and checklist, but its behavior checkbox conflicts with the stated behavior change.
Linked Issues check ✅ Passed The changes satisfy [#3035] by removing the legacy path, enforcing durable admission, failing closed on incomplete composition, and adding regression coverage.
Out of Scope Changes check ✅ Passed The code, test, and documentation changes directly support [#3035] and do not introduce unrelated scope.
Ai Use Disclosure ✅ Passed The PR selects generative tooling, names ZCode (GLM) and its scope, and its sole introduced commit has the matching standalone trailer Generated-by: ZCode (GLM).
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (3)
packages/runtime/src/runtime-kernel.ts (2)

1285-1369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared continuation claim and start-commit body.

Lines 1293-1369 now duplicate resumeContinuationClaimed lines 785-893 almost exactly: same continuationTargetRunHeaderForExecution call, same continuationClaimForExecution, same conflict error shape, and an identical commitContinuationStart event payload. With the legacy branch removed, there is no longer a reason for two copies. A future fix to the admission event payload would have to land twice, and a missed copy would weaken one lane silently.

Extract one private helper that takes the continuation, session header, user input, orchestration, tool mode, and tool-boundary protocol, and returns { claim, claimedRunHeader, commitContinuationStart }. The explicit durableAdmission type annotation then becomes unnecessary because the helper declares the shape.


1704-1712: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Delete the unreachable residue of the removed legacy branch in runAgentContinuation. Both sites still defend against states that the durable-only path can no longer produce, because the legacy beginOperation lane is gone.

  • packages/runtime/src/runtime-kernel.ts#L1704-L1712: make revalidateSafety a required parameter and delete the 'Durable continuation omitted final safety revalidation' guard; both call sites (line 925 and lines 1441-1445) already pass it.
  • packages/runtime/src/runtime-kernel.ts#L1774-L1789: replace the 'continuationStartAdmission' in begin test and the throwing IIFE with begin.continuationStartAdmission; beginContinuation always returns that property.
docs/architecture/runtime-resume-extraction-ledger.zh-CN.md (1)

353-366: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

删除重复陈述。

第 353-356 行已经完整说明:legacy_provider_retry lane 已移除,child provider retry 只走 durable continuation 准入,组合不完整即 fail closed。第 363-366 行再次重复同一结论与同一移除理由。第 362 行的首句(关于 hosted auto-resume 尚不可默认启用)是独立信息,应保留;其后的重复段落可以删除,避免两处描述日后各自漂移。

♻️ 建议修改
-- 在上述 composition/owner 测试完成前,不能把当前切片描述为 hosted auto-resume 已默认可用。
-  `legacy_provider_retry` 兼容 lane(仅维持升级前的 provider 429 重试能力,不具有 durable
-  continuation、跨进程 exactly-once 或 crash-resume 承诺)已在 host authority lifecycle
-  integration 完成 typed authority composition 后移除;child provider retry 现在只走
-  durable continuation 准入,组合不完整即 fail closed。
+- 在上述 composition/owner 测试完成前,不能把当前切片描述为 hosted auto-resume 已默认可用。

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 368b926e-0a5e-4826-97a0-c337f7bae5c4

📥 Commits

Reviewing files that changed from the base of the PR and between a032beb and 83f28e5.

📒 Files selected for processing (6)
  • docs/architecture/runtime-resume-extraction-ledger.zh-CN.md
  • docs/architecture/runtime-resume-phase3-phase4-workspace-checkpoint-design.zh-CN.md
  • packages/runtime/src/__tests__/session-manager.test.ts
  • packages/runtime/src/runtime-kernel.ts
  • packages/runtime/src/runtime-runner.ts
  • packages/runtime/src/session-manager.ts
💤 Files with no reviewable changes (1)
  • packages/runtime/src/runtime-runner.ts

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.

@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for the cleanup — this is a clean, well-scoped deletion. I reviewed it end-to-end (references, tests, and the production composition claim).

Conclusion: PASS from review. No P0/P1/P2. A few optional nits.

Verified:

  • admissionMode, legacy_provider_retry, planLegacyChildProviderRetry, runLegacyProviderRetry have zero references left in code (only historical mentions in the zh-CN docs).
  • The production composition (execution-composition.ts) indeed always provides both the SQLite continuation authority and the safety inspector, so host behavior is unchanged.
  • No dead tests: no existing test referenced the removed lane, and the new regression test (retryChildAgent fails closed without a complete continuation composition) genuinely fails on main — the old code would have taken the legacy lane and retried. A real red→green test.

Optional nits:

  • P3: the error message at session-manager.ts:4110 still says "refusing legacy fallback" — the legacy path is gone, so there's no fallback left to refuse. Suggest rewording.
  • P3: 'continuationStartAdmission' in begin in runtime-kernel.ts is now a tautology (the type always carries the field); the throw branch is unreachable. Can be simplified to begin.continuationStartAdmission.
  • P3: the PR checklist marks "behavior change: No", but the summary describes fail-closed behavior for directly-constructed incomplete compositions — worth marking Yes for accuracy.
  • P3: the description says retryChildAgentWithExecution "replaces" the old entry, but it already existed in main; the PR removes the admission-mode selection inside it.

AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on ollama-cloud/deepseek-v4-flash, read-only). The subagent verified zero references via git grep on the PR head, checked the production composition, and confirmed the new test is red on main; I re-verified the key claims. Failure-mode predictions are analysis, not observed behavior. Please weigh these findings with your own judgment.

中文摘要(AI 辅助审查)

结论:PASS,无 P0/P1/P2,可合并。删除彻底(legacy 相关符号全仓 0 残留),生产组合确实总是双全(SQLite continuation authority + safety inspector),无死测试;新回归测试在 main 上必红,是真回归测试。仅 4 个 P3 可选建议:更新 session-manager.ts:4110 的过时错误文案、简化 runtime-kernel.ts 中恒真的 in 检查、修正 checklist 的"行为变化: No"、修正 PR 描述中"取代旧入口"的措辞(该函数在 main 已存在)。

@Astro-Han
Astro-Han merged commit 7a98074 into apache:main Aug 16, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(runtime): remove the legacy child provider-retry path

2 participants