Skip to content

fix(cli): Keep transient runtime record I/O retryable - #9362

Merged
doudouOUC merged 1 commit into
QwenLM:mainfrom
doudouOUC:agent/conversation-runtime-record-io-retry
Aug 17, 2026
Merged

fix(cli): Keep transient runtime record I/O retryable#9362
doudouOUC merged 1 commit into
QwenLM:mainfrom
doudouOUC:agent/conversation-runtime-record-io-retry

Conversation

@doudouOUC

Copy link
Copy Markdown
Collaborator

What this PR does

This PR preserves fail-closed integrity checks while distinguishing transient ownership-record I/O failures from invalid persisted state. Resource-exhaustion and read I/O errors can now surface as retryable unavailability and recover on a later acquisition, while symlink loops, file identity changes, malformed JSON, and invalid record shapes remain terminal.

Why it's needed

The secure record readers introduced by #9181 treated every open and read failure as evidence of corruption. A one-off EMFILE or EIO therefore latched conversation_runtime_ownership_compromised for the daemon lifetime even when the record was valid, preventing Conversations and Live recovery until restart. This follows up on two Critical review findings that arrived immediately before #9181 was merged.

Reviewer Test Plan

How to verify

Exercise ownership acquisition with an existing dead owner record and a stale Live discovery record. For each record type, inject EMFILE during open and EIO during file-handle read: the first acquisition should return conversation_runtime_unavailable with retryable: true, and the second should reclaim successfully after the injected fault clears. Injecting ELOOP during open should still return conversation_runtime_ownership_compromised with retryable: false. Existing malformed-record, unsafe-file, and identity-change cases should remain terminal and must not overwrite state.

Local validation passed 42/42 targeted tests together with the repository build, typecheck, lint, formatting, and diff checks. An independent probe reproduced all four permanent-latch paths on the base commit and confirmed recovery with this change.

Evidence (Before & After)

N/A (non-UI daemon reliability fix).

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

Node.js 22-compatible repository toolchain on macOS; local npm workspace build, typecheck, lint, and targeted Vitest tests.

Risk & Scope

  • Main risk or tradeoff: Reclassifying errors too broadly could weaken fail-closed handling; this is mitigated by keeping ELOOP, lock compromise, permission and identity changes, malformed content, and provisional ownership failures terminal.
  • Not validated / out of scope: Kernel-level fault injection on Windows and Linux; repository CI is expected to provide cross-platform coverage.
  • Breaking changes / migration notes: None. Persisted record formats, ownership ordering, routes, and public APIs are unchanged.

Linked Issues

Follow-up to #9181. Addresses the owner-record Critical finding and the Live discovery Critical finding.

中文说明

本 PR 的改动

本 PR 在保留完整性校验 fail-closed 语义的同时,区分所有权记录的瞬时 I/O 失败与无效持久化状态。资源耗尽和读取 I/O 错误现在会表现为可重试的暂时不可用,并可在后续 acquisition 中恢复;符号链接循环、文件身份变化、JSON 损坏和记录结构无效仍保持终态。

为什么需要此改动

#9181 引入的安全记录读取逻辑把所有 open 和 read 失败都视为状态损坏。即使记录本身有效,一次瞬时 EMFILEEIO 也会在整个守护进程生命周期内锁存 conversation_runtime_ownership_compromised,导致 Conversations 和 Live 必须重启才能恢复。本修复跟进 #9181 合并前最后出现的两条 Critical 评审意见。

审查者测试计划

验证方式

分别使用已有的失效 owner 记录和过期 Live discovery 记录执行所有权 acquisition。对每种记录,在 open 阶段注入 EMFILE,在文件句柄读取阶段注入 EIO:第一次 acquisition 应返回 conversation_runtime_unavailableretryable: true,故障清除后的第二次 acquisition 应成功回收记录。open 阶段注入 ELOOP 时仍应返回 conversation_runtime_ownership_compromisedretryable: false。现有的记录损坏、不安全文件和身份变化场景仍应保持终态,且不得覆盖原状态。

本地验证已通过 42/42 个定向测试,以及仓库 build、typecheck、lint、格式和 diff 检查。独立探针在基线提交上复现了全部四条永久锁存路径,并确认本改动可以恢复。

证据(改动前后)

N/A(非 UI 的守护进程可靠性修复)。

测试平台

操作系统 状态
🍏 macOS ✅ 已测试
🪟 Windows ⚠️ 未测试
🐧 Linux ⚠️ 未测试

环境(可选)

macOS 上兼容 Node.js 22 的仓库工具链;本地 npm workspace build、typecheck、lint 和定向 Vitest 测试。

风险与范围

  • 主要风险或权衡:错误分类放宽过度可能削弱 fail-closed 处理;本改动继续将 ELOOP、锁损坏、权限和身份变化、内容损坏以及 provisional 所有权阶段的失败视为终态,以降低该风险。
  • 未验证或不在范围内:未在 Windows 和 Linux 上执行内核级故障注入;预期由仓库 CI 提供跨平台覆盖。
  • 破坏性变更或迁移说明:无。持久化记录格式、所有权顺序、路由和公共 API 均未改变。

关联问题

本 PR 是 #9181 的后续修复,处理 owner 记录的 Critical 评审意见Live discovery 的 Critical 评审意见

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

Built the PR base vs this PR head 385e697, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

⚠️ The PR-base baseline could not be built this run, so the before/after diff was skipped. Re-run to compare.

Qwen Code · serve A/B

@doudouOUC
doudouOUC marked this pull request as ready for review August 17, 2026 15:58
@doudouOUC
doudouOUC enabled auto-merge August 17, 2026 15:58
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

  • Template: complete ✓ — all required sections present, bilingual body.
  • Problem: real and documented. This follows up two Critical review findings on feat(daemon): Isolate the Conversations runtime boundary #9181 (the owner-record fs.open catch and the Live discovery fs.open catch), each backed by a probe witness: EMFILE injected at the record open latches conversation_runtime_ownership_compromised (retryable: false) and keeps failing after the fault clears. I confirmed the latch in the base code — readOwnerRecord / readExistingRecord wrap every open/read failure as UnsafeOwnershipStateError / LiveDiscoveryStateError, which mapAcquireError escalates into a latched terminal error, while the sibling handle.stat() path already propagates raw and maps to retryable unavailability.
  • Direction: aligned — fail-closed integrity should not poison a daemon for its lifetime over one transient errno, and the existing boot retry loop (publicationRetryablescheduleLiveDiscoveryRetry()) is exactly the machinery this reclassification plugs into.
  • Size: not a core-module path (packages/cli/src/serve/**); 22 production lines (ownership 6+2, discovery 9+5) plus 133 test lines. No threshold concerns.
  • Approach: minimal and on-point — keep ELOOP (symlink traversal against O_NOFOLLOW) and malformed JSON terminal, rethrow other open/read errnos raw so they map to retryable unavailability, and lift readFile out of the JSON.parse try so read-I/O failures and parse failures classify separately. Every edit serves the stated goal; nothing to cut.
  • Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths. One focus for code review: the retryable window must not extend past the point where state is mutated, so provisional/commit-phase failures need to stay terminal.

Moving on to code review. 🔍

中文说明

感谢贡献!

  • 模板:完整 ✓——各必填小节齐全,正文双语。
  • 问题:真实且有据。本 PR 跟进 feat(daemon): Isolate the Conversations runtime boundary #9181 上的两条 Critical 评审意见(owner 记录与 Live discovery 的 fs.open catch),每条都有探针证据:在记录 open 处注入 EMFILE 会锁存 conversation_runtime_ownership_compromisedretryable: false),故障清除后仍持续失败。我已在基线代码中确认该锁存——readOwnerRecord / readExistingRecord 把所有 open/read 失败包装为 UnsafeOwnershipStateError / LiveDiscoveryStateError,经 mapAcquireError 升级为锁存的终态错误;而相邻的 handle.stat() 路径本来就原样向上传播并映射为可重试的暂时不可用。
  • 方向:对齐——fail-closed 完整性不应让一次瞬时 errno 毒害守护进程的整个生命周期;现有 boot 重试循环(publicationRetryablescheduleLiveDiscoveryRetry())正是本次错误重分类所接入的机制。
  • 规模:未触及核心模块路径(packages/cli/src/serve/**);生产代码 22 行(ownership 6+2、discovery 9+5)+ 测试 133 行,无任何阈值顾虑。
  • 方案:最小且聚焦——ELOOP(违反 O_NOFOLLOW 的符号链接遍历)与 JSON 损坏保持终态,其余 open/read errno 原样抛出、映射为可重试的暂时不可用;把 readFile 移出 JSON.parse 的 try,使读 I/O 失败与解析失败分开分类。每处改动都服务于既定目标,无可裁剪。
  • 风险:无升级风险信号——改动文件均不命中与回滚相关的高风险路径。代码审查阶段的一个关注点:可重试窗口不能越过状态变更点,provisional/提交阶段的失败必须保持终态。

进入代码审查 🔍

Qwen Code · qwen3.8-max

Reviewed at 385e697d3583ed19c1718862e9598bcede987358 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Code review

My independent proposal for this problem was exactly what this PR does: classify errno at the two record readers' fs.open/readFile sites, keep the integrity signals terminal (ELOOP against O_NOFOLLOW, malformed JSON, shape/identity violations), rethrow everything else raw so the existing mapping turns it into retryable conversation_runtime_unavailable, and let the daemon's existing retry loop do the recovery. The PR matches that baseline, including the two subtle bits — lifting readFile out of the JSON.parse try so read-I/O and parse failures classify separately, and finally passing the cause into LiveDiscoveryStateError instead of dropping it.

What I verified by tracing consumers, since this is security-adjacent fail-closed code:

  • Fail-closed integrity holds. Every shape/identity/permission check still runs before any state mutation, and both reclassified failure paths abort before anything is written. Post-commit failures stay terminal: mapAcquireError still compromises on state === 'provisional', so the retryable window ends exactly where mutation begins — including the confirmation read in the Live handoff after the owner record is committed.
  • No raw errno escapes to a route. mapAcquireError's default arm maps anything unrecognized to retryable conversation_runtime_unavailable, and on the serve side the publication catch-alls (attemptPendingLiveDiscovery's per-target catch and the boot .catch) handle non-LiveDiscoveryOwnerActiveError errors with warn-and-retry regardless of error class — scheduleLiveDiscoveryRetry() re-attempts the pending base dirs, which is the recovery path this reclassification feeds.
  • The release path improves too. A transient open failure during releaseOnce previously latched compromised for the daemon lifetime; it now surfaces as retryable unavailability. Nothing is mutated before that read, so retrying release is safe.
  • Tests pin the change. The four recovery cases (EMFILE at open / EIO at readFile × ownership / Live discovery) assert the retryable classification and a successful reclaim on the second acquisition; the two ELOOP cases assert terminal compromise. Against the base code all four recovery cases would see conversation_runtime_ownership_compromised, so the suite does not pass identically without the diff. Mock wiring uses vi.hoisted() per the repo's test guidance. No over-abstraction; 133 test lines for 22 production lines is the right ratio.

No critical findings. Not verified statically: kernel-level EMFILE/EIO behavior on Windows/Linux (the injection here is mocked at the node:fs/promises boundary, which is platform-agnostic, but the author tested on macOS only — author's claim, not independently re-run).

CI evidence

The PR's own CI at the reviewed commit — quoted from the check-runs API:

Final CI results for 385e697 (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Live Host (macos-latest) ✅ success
macos-latest / Java 21 ✅ success
Real daemon E2E / Java 11 ✅ success
Secret scan (TruffleHog) ✅ success
Serve A/B (ubuntu-latest, Node 22.x) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
ubuntu-latest / Java 11 ✅ success
ubuntu-latest / Java 17 ✅ success
ubuntu-latest / Java 21 ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
windows-latest / Java 21 ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

All five red checks failed in setup, before any PR code ran: four died downloading actions/checkout and Live Host died in "Install desktop dependencies", each with codeload.github.com answering 429 (Too Many Requests) (e.g. Failed to download archive '…actions/checkout…' after 3 attempts, GET https://codeload.github.com/WhiskeySockets/eslint-config/legacy.tar.gz/299e838 - 429) — a rate-limit burst in the 15:14–15:32 UTC window, not this PR. The two skipped unit legs are normal here: merged #9346 shows the identical pattern (ubuntu runs, macOS/Windows skipped). The suite that exercises this change — Test (ubuntu-latest, Node 22.x) including the new fault-injection cases — is green, as is Serve A/B.

Sandboxed verification would settle the one gap CI cannot: the central claim here is behavioural (a transient EMFILE/EIO no longer latches a terminal compromise, and a later acquisition reclaims), and the unit suite proves it only through mocked errno injection on one OS. A maintainer can sponsor @qwen-code /verify — the author is a fork contributor, so this runs as a sponsored run (the triggering comment approves the head it runs against, and it carries a pre-execution risk screen plus a full workspace wipe) — for A/B proof that the new tests fail on the base build and pass on this one. Read the resulting report with the same skepticism as a fork's own CI logs: the code under verification is adversarial input.

Real-scenario tmux: N/A — this changes daemon-internal error classification that only surfaces under injected EMFILE/EIO during record reads; there is no user-visible TUI behavior to drive (the PR's own evidence section says the same), and static-review rules bar executing PR code locally. The fault-injection unit suite above is the executable evidence carrier.

中文说明

代码审查

我对该问题的独立方案与本 PR 完全一致:在两个记录读取器的 fs.open/readFile 处按 errno 分类——完整性信号(O_NOFOLLOW 下的 ELOOP、JSON 损坏、形状/身份违规)保持终态,其余原样抛出,由现有映射转为可重试的 conversation_runtime_unavailable,再由守护进程已有的重试循环完成恢复。PR 连两处细节也做对了:把 readFile 移出 JSON.parse 的 try(读 I/O 与解析错误分开分类),并把 cause 传入 LiveDiscoveryStateError(不再丢弃)。

这是安全相关的 fail-closed 代码,我逐一追踪了消费方:

  • fail-closed 完整性保持:所有形状/身份/权限校验仍在任何状态变更前执行;两条被重分类的失败路径都在写入前中止。提交后的失败仍是终态:mapAcquireErrorstate === 'provisional' 时依旧 compromise——可重试窗口恰好在状态变更点结束,Live handoff 在 owner 记录提交后的确认读也一样。
  • 没有原始 errno 泄漏到路由mapAcquireError 的默认分支把未识别错误映射为可重试的 conversation_runtime_unavailable;serve 侧的发布兜底(attemptPendingLiveDiscovery 的逐目标 catch 与 boot .catch)对非 LiveDiscoveryOwnerActiveError 错误本来就是警告+重试,scheduleLiveDiscoveryRetry() 会重试未完成的 base dir——正是本次重分类接入的恢复路径。
  • release 路径同样改善releaseOnce 中的瞬时 open 失败过去会为守护进程生命周期锁存 compromised,现在表现为可重试的暂时不可用;该读取之前没有任何变更,重试 release 是安全的。
  • 测试钉住了改动:四个恢复用例(open 注 EMFILE / readFile 注 EIO × ownership / Live discovery)同时断言可重试分类与第二次 acquisition 成功回收;两个 ELOOP 用例断言终态。在基线代码上四个恢复用例都会得到 conversation_runtime_ownership_compromised,因此该测试套件在无此 diff 时不会同样通过。mock 按仓库规范使用 vi.hoisted()。无过度抽象;22 行生产代码配 133 行测试比例恰当。

无 Critical 发现。静态未验证:Windows/Linux 上的内核级 EMFILE/EIO 行为(此处注入发生在 node:fs/promises 边界、与平台无关,但作者仅在 macOS 上测试——此为作者自述,未独立复跑)。

CI 证据

上表引自 reviewed commit 的 check-runs API。五个红色检查全部在 setup 阶段失败、未运行任何 PR 代码:四个在下载 actions/checkout 时失败、Live Host 在 "Install desktop dependencies" 步骤失败,日志均为 codeload.github.com 返回 429 (Too Many Requests)(如 Failed to download archive '…actions/checkout…' after 3 attemptsGET …/eslint-config/legacy.tar.gz - 429)——15:14–15:32 UTC 时段的限流突发,与本 PR 无关。两个被跳过的单元测试腿是仓库常态:已合并的 #9346 呈相同模式(ubuntu 运行、macOS/Windows 跳过)。真正覆盖本改动的套件——Test (ubuntu-latest, Node 22.x)(含新增故障注入用例)——为绿色,Serve A/B 亦为绿色。

沙箱验证可补上 CI 无法覆盖的一环:本 PR 的核心主张是行为性的(瞬时 EMFILE/EIO 不再锁存终态、后续 acquisition 可回收),而单元测试仅通过单一 OS 上的 mock errno 注入证明。维护者可发起 @qwen-code /verify——作者是 fork 贡献者,因此以 sponsored run 方式执行(触发评论即批准其运行的 head,并带执行前风险筛查与完整工作区清理)——以获得新测试在基线构建上失败、在本 PR 上通过的 A/B 证据。请对产出的报告保持与 fork CI 日志同等的怀疑:被验证的代码本身是对抗性输入。

真实场景 tmux 测试:N/A——本改动是守护进程内部的错误分类,仅在记录读取时注入 EMFILE/EIO 才会显现;没有可操作的 TUI 用户可见行为(PR 的证据一节同样写 N/A),且静态审查规则禁止在本地执行 PR 代码。上述故障注入单元测试即为可执行的证据载体。

Qwen Code · qwen3.8-max

Reviewed at 385e697d3583ed19c1718862e9598bcede987358 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean, minimal, well-pinned fix for a documented latch bug; the two remaining notes are non-blocking (infra-red CI needs a re-run before merge, and errno behavior beyond the mocked injection is unexercised).

Stepping back: this is the follow-up those two Critical findings on #9181 deserved. The probe witnesses there showed a transient EMFILE latching conversation_runtime_ownership_compromised for the daemon lifetime; this PR closes exactly that window and nothing more. The approach matches my independent proposal, the 22-line production diff carries nothing speculative, and the fail-closed boundary landed precisely where it belongs — everything before the first state mutation is retryable, everything from provisional onwards stays terminal. The tests assert recovery, not just classification: the second-acquisition reclaim is the part a lazy fix would have skipped, and it is covered for both record types and both injection points. If I were maintaining this in six months, the ELOOP-stays-terminal / everything-else-rethrows split would read immediately.

Two honest reservations, neither blocking. First, the five red CI checks all died in setup on codeload.github.com 429s, so the merge gate needs a green re-run before this can land regardless of approval. Second, the errno behavior is proven by mocked injection on ubuntu plus the author's macOS run (their claim); kernel-level Windows/Linux behavior rides on the classification logic being platform-agnostic — which it is, since the only platform-dependent line (O_NOFOLLOW at open) is not the reclassified part. A sponsored @qwen-code /verify remains available if a maintainer wants A/B load-bearing proof beyond the unit suite.

Approving, pinned to the reviewed commit.

中文说明

置信度:4/5 —— 针对已记录的锁存 bug 的干净、最小、测试钉扎良好的修复;剩余两点保留意见均非阻塞(CI 因基础设施飘红需重跑才能合并;mock 注入之外的 errno 行为未实测)。

退一步看:这正是 #9181 那两条 Critical 意见应得的后续修复。当时的探针证据显示一次瞬时 EMFILE 会在守护进程整个生命周期内锁存 conversation_runtime_ownership_compromised;本 PR 恰好关上这扇窗,且没有扩大任何其他窗口。方案与我的独立提议一致,22 行生产 diff 没有任何投机性内容,fail-closed 边界落在正确位置——首次状态变更之前的一切可重试,provisional 及以后的一切保持终态。测试断言的是恢复而不仅是分类:第二次 acquisition 成功回收是偷懒修复会跳过的部分,而两种记录类型、两个注入点均有用例覆盖。六个月后再维护这段代码,"ELOOP 保持终态 / 其余原样抛出"的划分一目了然。

两点诚实的保留意见,均不阻塞。其一,五个红色 CI 检查全部在 setup 阶段因 codeload.github.com 429 失败,无论是否批准,合并门禁都需要一次绿色重跑。其二,errno 行为目前由 ubuntu 上的 mock 注入与作者的 macOS 本地运行(其自述)证明;内核级 Windows/Linux 行为依赖分类逻辑与平台无关——事实如此,因为唯一平台相关的一行(open 时的 O_NOFOLLOW)并非被重分类的部分。若维护者想要单元测试之外的 A/B 承载性证据,仍可发起 sponsored @qwen-code /verify

按 reviewed commit 批准。

Qwen Code · qwen3.8-max

Reviewed at 385e697d3583ed19c1718862e9598bcede987358 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, looks ready to ship. ✅

@doudouOUC doudouOUC self-assigned this Aug 17, 2026

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the full diff at 385e697d and ran the targeted suite locally: 27 passed, 1 skipped (platform-gated, pre-existing).

What I checked:

  • Error mapping is correct end-to-end: rethrown open/read errors (EMFILE/EIO) fall through mapAcquireError to conversationRuntimeUnavailableError (retryable), while ELOOP, malformed JSON, and identity changes still map to compromise (terminal). Same shape in readExistingRecord via LiveDiscoveryStateError.
  • Moving handle.readFile outside the JSON.parse try is the right split — parse failures stay terminal, I/O failures become retryable.
  • Tests inject at both open and readFile layers for both record types and verify recovery on the second acquisition, plus ELOOP staying terminal.

LGTM.

@doudouOUC
doudouOUC added this pull request to the merge queue Aug 17, 2026
Merged via the queue into QwenLM:main with commit 84522c1 Aug 17, 2026
111 of 117 checks passed
@doudouOUC
doudouOUC deleted the agent/conversation-runtime-record-io-retry branch August 17, 2026 17:49
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.14.

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.

3 participants