Skip to content

fix(runtime): retry output-free provider finishes - #3777

Open
sunrioa wants to merge 2 commits into
apache:mainfrom
sunrioa:codex/fix-empty-provider-finish
Open

fix(runtime): retry output-free provider finishes#3777
sunrioa wants to merge 2 commits into
apache:mainfrom
sunrioa:codex/fix-empty-provider-finish

Conversation

@sunrioa

@sunrioa sunrioa commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Treat an output-free stop with unavailable usage as an incomplete provider response instead of a successful end_turn; retry it through the existing bounded recovery path and fail explicitly with provider_unavailable when recovery is exhausted.
  • Route the exact raw network_error finish reason through the existing bounded network retry policy.
  • Share one response-evidence and finish-boundary classification between model settlement and Backend side-effect gating, preventing retryable finishes from consuming a logical step, persisting an empty Assistant message, or replaying an already-settled tool.
  • Preserve authoritative usage from billed failed requests, fail aggregate metering closed when usage is incomplete, and keep valid zero-usage stops and text responses with unavailable usage unchanged.

Fixes #3772

Verification

  • npm --workspace @maka/runtime test — 3,047 passed, 13 skipped, 0 failed.
  • Focused ModelAdapter and AiSdkBackend test files — 216 passed, 0 failed.
  • Desktop suite — 1,506 passed, 0 failed.
  • Runtime Host suite — 1,144 passed, 0 failed.
  • Computer Use suite — 118 passed, 0 failed.
  • CLI suite — 453 passed, 0 failed.
  • Eval — Node 75 passed; Python 68 passed, 6 platform/feature skips, 0 failed.
  • npm run lint, npm run format:check, npm run build, and npm run typecheck — passed.
  • Desktop and UI Knip checks, ASF header check, release contract check, and git diff --check — passed.
  • CLI development package and offline installed-product smoke verification — passed.

Not run: a live OpenRouter/Ox Alpha reproduction because the provider-controlled empty finish is intermittent; deterministic Runtime fixtures cover the exact observed stop and network_error frames.

Review focus

  • A stop is recoverable only when it has no model-response evidence and no usable usage.
  • Only the exact raw network_error value enters the network retry path.
  • Retried physical requests do not become logical Runtime steps or replay settled tools.
  • Exact billed usage is retained, while incomplete usage prevents a misleading partial final total.

AI use

Select exactly one:

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

Tool(s) and scope: OpenAI Codex traced the provider-finish settlement, retry, persistence, and usage-accounting paths; implemented the Runtime changes and regression tests; ran local verification; and performed adversarial review of the final diff. I reviewed the resulting implementation, tests, and PR description and decided to submit the change.

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

@sunrioa
sunrioa marked this pull request as ready for review August 25, 2026 08:54

@Astro-Han Astro-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.

Test

@Astro-Han Astro-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.

I reviewed this head and found no blocking issues.

The fix correctly classifies output-free stop without usage as incomplete for bounded recovery, and routes network_error raw finishes to retryable network failure. Usage accounting retains failed-request usage and remains fail-closed. Two minor P3 observations remain non-blocking (evidence accumulation across steps, finish-without-step-finish usage fallback).

Checks on fe53c1529 are test: success.

简体中文该头未发现阻断,两处 P3 观察已记录。

Astro-Han
Astro-Han previously approved these changes Aug 25, 2026

@Astro-Han Astro-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.

APPROVE — fe53c15

@Astro-Han

Copy link
Copy Markdown
Contributor

Reviewed the production diff. The fix is right and the test weight is proportionate — 816 of the 985 added lines are tests, covering the branches that actually fork (metadata-only evidence, authoritative zero usage, exhausted-retry metering). Two non-blocking notes, both about where the new logic lives rather than whether it is correct.

P2 — finishReason === 'network_error' is one spelling of a documented class, not a provider quirk.

classifyModelFinishBoundary hardcodes a single provider spelling. That value is not OpenRouter-specific: Zhipu's API FAQ states that when an SSE call terminates abnormally, the reason is reported through finish_reason instead of an error code, and the same value appears from GLM endpoints and from Zen/OpenRouter alike. It already has siblings in the wild — sensitive for content policy (pydantic-ai#7678) and error from OpenRouter. LiteLLM and pydantic-ai both fix this in the finish-reason mapping layer (litellm#24373), not in downstream retry logic.

The owner here is chunkFinishReason (packages/runtime/src/model-adapter.ts:651), introduced in #2297. It deliberately un-maps the SDK's unified: 'other' back to the raw provider spelling, on the stated premise that "other with a provider spelling is an ordinary finished turn". network_error falsifies exactly that premise — the SDK's mapping is correct here and we override it. A spelling table at that seam would also cover sensitive for free, since settleModelStepOutcome already routes content-filter and error.

P3 — the Backend recomputes what the adapter has already computed.

The adapter's iterator already walks the translated stream and derives hasResponseEvidence and latestStepFinishHadUsableUsage. The Backend then walks the same events and derives both again, plus calls classifyModelFinishBoundary inline. The two copies use different fallbacks (?? usage !== undefined in the adapter, ?? true in the Backend). I traced it and the behaviour does not currently diverge, but only because an unrelated incompleteStreamRecovery disjunct catches the case.

ModelStreamEvent is package-internal — only model-protocol.ts, model-adapter.ts and tests reference it, and nothing serializes or persists it. So the adapter can stamp the disposition onto the step-finish/finish event it yields and carry hasResponseEvidence on ModelStepOutcome. That removes attemptSawResponseEvidence, latestStepFinishHadUsableUsage, the inline ternary, and both @internal exports, and makes the fallback divergence unrepresentable.

Not raised as a change request here: classifyModelFinishBoundary and settleModelStepOutcome now maintain two separate finish-reason vocabularies. That is a wider refactor, filed separately as #3802.

Reviewed with help from Claude.

简体中文

看了生产代码部分。修复方向是对的,测试的分量也匹配——985 行新增里有 816 行是测试,覆盖的都是真会分叉的分支(只有 metadata 的证据、权威零 usage、重试耗尽后的计费)。两条意见,都不阻塞,说的是新逻辑该放在哪里,而不是它对不对。

P2 —— finishReason === 'network_error' 是一整类成文行为里的一个拼写,不是某家供应商的怪癖。

classifyModelFinishBoundary 把单个供应商拼写写死了。这个值并非 OpenRouter 特有:智谱 API FAQ 写明流式调用异常终止时,异常原因通过 finish_reason 返回而不是错误码,同一个值在 GLM 端点和 Zen/OpenRouter 上都会出现。它在野外已经有兄弟了——内容策略的 sensitivepydantic-ai#7678)和 OpenRouter 的 error。LiteLLM 和 pydantic-ai 都是在 finish-reason 映射层修的(litellm#24373),没有放进下游重试逻辑。

真正的所有者是 #2297 引入的 chunkFinishReasonpackages/runtime/src/model-adapter.ts:651)。它主动把 SDK 给的 unified: 'other' 换回供应商原始拼写,前提是注释里写的「other 带供应商拼写 = 一次普通结束」。network_error 恰好证伪这个前提——这里 SDK 的映射是对的,是我们覆盖了它。在那个缝上放一张拼写表还能顺带覆盖 sensitive,因为 settleModelStepOutcome 已经有 content-filtererror 的去处。

P3 —— Backend 重算了一遍 adapter 已经算出来的东西。

adapter 的迭代器已经走过完整的翻译流,算出了 hasResponseEvidencelatestStepFinishHadUsableUsage。Backend 拿到同一批事件后又各算了一遍,还额外内联调了一次 classifyModelFinishBoundary。两份的回退语义还不一样(adapter 是 ?? usage !== undefined,Backend 是 ?? true)。我推演过,目前行为不会分叉,但那是因为被一个不相干的 incompleteStreamRecovery 析取项接住了。

ModelStreamEvent 是包内契约——只有 model-protocol.tsmodel-adapter.ts 和测试引用,没有序列化也没有持久化。所以 adapter 可以把 disposition 直接盖在它 yield 的 step-finish/finish 事件上,并把 hasResponseEvidence 挂到 ModelStepOutcome 上。这样可以删掉 attemptSawResponseEvidencelatestStepFinishHadUsableUsage、那个内联三元表达式,以及两个 @internal 导出,并且让回退分叉在结构上无法表示。

这里不作为修改要求提出:classifyModelFinishBoundarysettleModelStepOutcome 现在各自维护一套 finish-reason 词表。那是更大范围的重构,已单独记为 #3802

本次 Review 有 Claude 协助。

@Astro-Han
Astro-Han dismissed their stale review August 25, 2026 13:56

Dismissing my earlier approval — follow-up review found two non-blocking items (P2/P3) worth settling first; details in the review comment above.

@sunrioa

sunrioa commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Addressed both review notes in 776d3f5.

  • Moved the documented provider finish-reason aliases to chunkFinishReason: network_error now maps to Maka’s canonical network-failure sentinel, sensitive maps to content-filter, error keeps its existing terminal policy, and unknown raw reasons still pass through.
  • Made Adapter-stamped finish disposition and request-level response evidence the single authority consumed by Backend, removing the duplicate Backend classifier, evidence accumulation, usage fallback, and internal helper exports.
  • Added regression coverage for both finish boundary kinds, finish-only total usage, per-step evidence reset versus request-level retry safety, and the provider aliases above.

Local verification is green, including the full affected workspace suites, release contracts, and the installed CLI package smoke.

For the broader finish-reason vocabulary consolidation tracked in #3802, this fix does not make further changes at this time.

简体中文

已在 776d3f5 中处理两条评审意见。

  • 将已知供应商结束原因的别名映射移至 chunkFinishReasonnetwork_error 现在映射为 Maka 的规范网络失败标识,sensitive 映射为 content-filtererror 保留现有的终止处理策略,未知的原始结束原因继续透传。
  • 让 Adapter 写入的结束处置结果和请求级响应证据成为 Backend 使用的唯一权威,删除 Backend 中重复的分类逻辑、证据累计、usage 回退及内部辅助函数导出。
  • 新增回归测试,覆盖两种结束边界、仅在最终结束事件中提供总 usage、逐步骤证据重置与请求级重试安全,以及上述供应商别名。

本地验证全部通过,包括受影响工作区的完整测试、发布契约检查和安装后的 CLI 包冒烟测试。

对于 #3802 中跟踪的更广泛 finish-reason 语义统一工作,本次修复工作暂不作进一步调整。

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.

Runtime treats output-free provider finish frames as successful completion

2 participants