Skip to content

fix(runtime-host): log why a transcript page read failed - #5573

Merged
Astro-Han merged 1 commit into
apache:mainfrom
MoonOld:fix/runtime-host-transcript-page-diagnostic
Sep 27, 2026
Merged

Astro-Han merged 1 commit into
apache:mainfrom
MoonOld:fix/runtime-host-transcript-page-diagnostic

Conversation

@MoonOld

@MoonOld MoonOld commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Summary

SessionContinuityCoordinator#readTranscriptPage collapses every error that is not a TranscriptPageRequestError into persistence_failed / "Session transcript is unavailable", and returns it as an operation outcome. Because the handler returns instead of throwing, the [runtime-host] unexpected <operation> failure log in operation-dispatcher.ts — the only caller of boundedFailureDiagnostic — never runs for this operation. The cause therefore reaches neither the Runtime Host log, the desktop diagnostic report, nor the Renderer, which falls back to generic copy ("任务内容暂时无法刷新,请稍后重试。").

This records the cause with boundedFailureDiagnostic before the generic outcome is returned. The outcome the caller receives is unchanged; the only new output is one [runtime-host] log line.

Refs #5572

Verification

  • npm --workspace @maka/runtime-host run typecheck — clean
  • npx biome check on both touched files — clean
  • node --test packages/runtime-host/dist/__tests__/session-continuity-coordinator.test.js — 47/47 pass
  • Fails without the change: with the source hunk reverted and the test kept, the new case fails with AssertionError (0 pass / 1 fail)

The new test injects a reader that fails only on the page path — bootstrap requests carry no position, page requests always do — then asserts the generic outcome is still returned and that console.error was called exactly once with the injected cause:

✔ a failed transcript page records the underlying cause before the generic outcome
ℹ tests 47 / pass 47 / fail 0

Not run: the full workspace test suite.

Review focus

Whether logging directly here is right, versus routing it through onPublicationFailure. The sibling bootstrap path (session-continuity-coordinator.ts:966) calls onPublicationFailure under a comment saying the failure "has to leave a trace here", but that hook is wired to context.requestDrain (execution-composition.ts:925), not a logger — so no trace is emitted there either. That path is deliberately untouched here; it is context for #5572, not part of this change.

AI use

Select exactly one:

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

Tool(s) and scope: Maka (a DeepSeek-backed agent running in Maka Desktop) drafted the log line and the regression test, and ran the verification commands above. The root-cause analysis that motivated the change was reviewed and corrected before this PR by the author.

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

SessionContinuityCoordinator#readTranscriptPage collapses every error that
is not a TranscriptPageRequestError into persistence_failed with "Session
transcript is unavailable", and returns it as an operation outcome. Because
the handler returns instead of throwing, the "[runtime-host] unexpected
<operation> failure" log in operation-dispatcher.ts - the only caller of
boundedFailureDiagnostic - never runs for this operation. The cause then
reaches neither the Runtime Host log, the desktop diagnostic report, nor
the Renderer, which falls back to generic copy.

Record the cause with boundedFailureDiagnostic before returning the
generic outcome. The outcome the caller receives is unchanged.

Refs apache#5572

Generated-by: Maka
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 21, 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.

Verified head cea20b2a5.

The diagnosis in the body is accurate: #readTranscriptPage's catch returns the generic outcome instead of throwing, so the unexpected <operation> failure log at operation-dispatcher.ts:376 never sees it. boundedFailureDiagnostic (secret redaction + 8KB bound) is the right payload, console.error with the [runtime-host] prefix matches the sibling call sites, and the client-visible outcome is unchanged. The test fails without the change, and the console.error swap is safe — this file runs its tests sequentially, so no foreign output can land in the captured array.

On your review-focus question — direct logging vs onPublicationFailure: logging directly is right, and the reason is bigger than "not a logger". context.requestDrain is HostKernel.#requestDrain (host-kernel.ts:356-363): it sets shutdownRequested, arms the shutdown deadline, and begins composition drain — it requests host shutdown, not a trace. So all seven onPublicationFailure call sites (the canonical refresh / transcript advance / graph-change / domain-change fan-outs, and the bootstrap open at :974) take the host down with zero log line when the publication pipeline throws. For the oversized-Turn case in #5572, the same error on the bootstrap path doesn't just lose its cause — it silently drains the Runtime Host.

That's pre-existing wiring, not this PR's scope — the minimal follow-up is wrapping the hook at the composition seam, e.g. onPublicationFailure: (error) => { console.error('[runtime-host] transcript publication failed:', boundedFailureDiagnostic(error)); context.requestDrain(); }, which gives all seven sites a trace without touching the drain semantics. (Whether a read-path failure should drain at all is a separate design question for #5572.) Worth its own PR, not a blocker here.

One nit, take or leave: session.transcript.page failed is a different shape from the dispatcher's unexpected <operation> failure — it greps the same, so no change needed.

中文

已核实 head cea20b2a5。

正文诊断准确:#readTranscriptPage 的 catch 返回通用 outcome 而不抛出,operation-dispatcher.ts:376 的 unexpected <operation> failure 日志确实永远看不到它。boundedFailureDiagnostic(密钥脱敏 + 8KB 上限)是对的载荷,console.error + [runtime-host] 前缀与既有调用点一致,客户端拿到的 outcome 不变。测试在没有改动时会失败;console.error 替换是安全的——该文件的测试串行执行,不会捕获到其他用例的输出。

关于你在 Review focus 里的问题——直接打日志还是走 onPublicationFailure:直接打是对的,但理由比"它不是 logger"更大。context.requestDrain 就是 HostKernel.#requestDrain(host-kernel.ts:356-363):它置 shutdownRequested、武装 shutdown deadline、开始 composition drain——这是请求 Host 关机,不是留痕。所以全部七处 onPublicationFailure 调用点(canonical refresh / transcript advance / graph-change / domain-change 扇出,以及 :974 的 bootstrap open)在发布管线抛错时都是零日志关掉 Host。对 #5572 的 oversized-Turn 场景来说,同一个错发生在 bootstrap 路径上不只是丢原因——还会静默把 Runtime Host 拖下线。

这是既有接线问题,不在本 PR 范围——最小后续是在组合缝处包一层,例如 onPublicationFailure: (error) => { console.error('[runtime-host] transcript publication failed:', boundedFailureDiagnostic(error)); context.requestDrain(); },一次给全部七处补上留痕而不动 drain 语义(读路径失败到底该不该 drain 是 #5572 里另一个设计问题)。值得单独开个 PR,不阻塞这里。

一个小 nit(可改可不改):session.transcript.page failed 与 dispatcher 的 unexpected <operation> failure 形状不同——grep 效果一样,不用动。

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

Approved at @Astro-Han's explicit request: a small, focused fix with no blocking findings in the automated review of this head and green CI.

@Astro-Han
Astro-Han merged commit 22c2a81 into apache:main Sep 27, 2026
2 checks passed
Shouly pushed a commit to Shouly/maka that referenced this pull request Sep 27, 2026
A transcript page that failed answered `persistence_failed` and a failed
subscription bootstrap answered with a generic outcome; neither left the
cause anywhere. Both now log it first, bounded and redacted (a capacity
refusal is expected and stays quiet). An incomplete RuntimeEvent projection
names its invocation and each hard diagnostic's code and ids instead of one
fixed sentence, and never the event content.

Lead: apache#5573 (22c2a81), apache#5600 (f897600), apache#5601 (23b4d8a).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Shouly pushed a commit to Shouly/maka that referenced this pull request Sep 27, 2026
Watermark bfb315a. Done: apache#5573/apache#5600/apache#5601, apache#5521, apache#4875, apache#5723, apache#5738,
apache#5742. Not applicable: apache#5737, apache#5593. Deferred: apache#5730. Consider: apache#5599,
apache#5120, apache#5693. Diverged: apache#5740. Skipped: ACP, WorkHub, upstream renderer and
packages/ui, one refactor.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants