fix(dual-output): prevent FIFO blocking on startup when no reader connected - #4894
Conversation
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hi @chiga0, thanks for the PR! Before we dive into the code, the PR body needs to follow the PR template — it helps reviewers assess the change quickly.
The current body uses ## Summary and ## Test plan, but the template requires these sections:
## What this PR does— describe the change in prose## Why it's needed— motivation / user-facing problem## Reviewer Test Plan— with sub-sections: How to verify, Evidence (Before & After), Tested on (OS table), Environment## Risk & Scope— main risk, what's out of scope, breaking changes## Linked Issues—Fixes #4727<details><summary>中文说明</summary>— full Chinese translation
A couple of other things worth flagging upfront:
- Diff size: 451 changed files with 134K additions is very large for what's described as a FIFO-blocking fix. Could you help reviewers understand the scope? If there are unrelated changes mixed in, consider splitting into separate PRs.
- The underlying bug (#4727) is real and worth fixing — the motivation is clear. Just need the presentation to match the project's review process.
Please update the PR body to match the template and we'll get this reviewed. 🙏
中文说明
你好 @chiga0,感谢提交 PR!在开始代码审查之前,PR 正文需要按照 PR 模板 来填写。
当前正文使用了 ## Summary 和 ## Test plan,但模板要求的部分是:## What this PR does、## Why it's needed、## Reviewer Test Plan(含子-sections)、## Risk & Scope、## Linked Issues、以及中文翻译部分。
另外需要说明的是:451 个文件、134K 行新增对于一个 FIFO 阻塞修复来说变更量非常大。如果有不相关的改动,建议拆分成多个 PR。底层 bug (#4727) 是真实存在的,修复动机没有问题,只需要让 PR 格式符合项目规范即可。
请更新 PR 正文后我们会尽快审查。🙏
— Qwen Code · qwen3.7-max
…nected DualOutputBridge's ENXIO fallback used a blocking createWriteStream on FIFOs, causing the TUI to hang indefinitely when launched with `--json-file <fifo>` before a reader connects (issue #4727). Fix: use O_RDWR | O_NONBLOCK for the FIFO fallback path. This POSIX trick satisfies the kernel's "at least one reader" requirement without blocking. A buffer high-water-mark (1 MB) self-disables the bridge if no consumer ever drains the pipe. Also updates Quick start docs to recommend regular files as the default, with FIFOs documented as an advanced option that now works without ordering constraints. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
9698b32 to
55c3f72
Compare
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing: Test (windows-latest, Node 22.x). — qwen3.7-max via Qwen Code /review
|
Qwen Code review did not complete successfully: Qwen review timed out after 55 minutes. See workflow logs. |
Local runtime verification report (Linux)Built this branch ( Unit tests
E2E matrix (real TUI in tmux, real prompts via
|
| # | Scenario | Result |
|---|---|---|
| 1 | Baseline, FIFO w/o reader: startup | TUI starts fine (see note 1 below) |
| 2 | Baseline, FIFO w/o reader: quit | ❌ process wedges indefinitely (>90 s until killed); attaching a reader releases it but the reader receives 0 bytes — all buffered events lost |
| 3 | PR, FIFO w/o reader: startup | ✅ TUI up in <3 s; /proc/<pid>/fdinfo shows flags 02104002 = O_RDWR + O_NONBLOCK + O_CLOEXEC, exactly as implemented |
| 4 | PR, FIFO w/o reader: quit | ✅ process exits ~1 s after double Ctrl+C |
| 5 | PR, prompt answered with no reader, reader attaches later | ✅ full backlog replayed from the kernel pipe: session_start → user → 78 stream deltas → assistant → session_end (89 events, 22.6 KB), then EOF |
| 6 | PR, pre-connected reader | ✅ live streaming (26.6 KB); killing the reader mid-session → EPIPE → consumer disconnected, disabling in debug log, TUI keeps answering subsequent prompts |
| 7 | PR, regular file output | ✅ unchanged: session_start on launch, session_end appended on quit |
| 8 | PR, FIFO w/o reader + sustained output (long counting prompt) | ✅ bridge self-disables once the kernel pipe fills, fd closed, TUI stays fully responsive (answered the next prompt) |
| 9 | PR, #4727 exact repro (mkfifo both files) |
TUI starts ✅, output FIFO works ✅; input FIFO remains non-functional — echo >> input.fifo blocks the caller's shell (no reader) and the TUI never sees the command. Matches the new doc note that --input-file requires a regular file |
Observations (none blocking; for the record)
- The "before" symptom differs from the PR description, at least on Linux. The old blocking
open()runs on the libuv threadpool, not the main loop, so the TUI actually did start; the real damage was (a)shutdown()wedging the process forever and (b) total event loss once a reader finally attached (rows 1–2). The fix is just as necessary — only the failure mechanism is different from what the description says. macOS not re-verified here. - In practice self-disable triggers at kernel-pipe capacity (64 KB on Linux), not at the 1 MB guard. Sustained writes hit
EAGAINon the non-blocking fd → streamerror→ disable. The 1 MBwritableLengthguard can only fire for a >1 MB burst queued within one tick, so it is a backstop; the docs' "auto-disables once the internal buffer exceeds 1 MB" is approximate. End state is identical (bridge off, TUI unaffected), so this is a doc nuance, author's call. - The
EAGAINpath logsDualOutput stream error: SystemError [ERR_SYSTEM_ERROR]: A system error occurred: undefined returned undefined (undefined)— caught and handled correctly, but the message is unhelpful. Special-casingEAGAINalongsideEPIPEfor a clean WARN would be a nice follow-up. Cosmetic. - Since Dual Output模式运行TUI无响应 #4727's exact recipe still cannot inject prompts (row 9), consider a follow-up startup warning when
--input-filepoints to a FIFO — today the user'secho >>just silently blocks their shell.
CI context
The failing Test (windows-latest) job is pre-existing on main, not from this PR: the same two scripts/tests/dev.test.js launcher tests fail on main's latest run (27210101989). All package suites pass on Windows in this PR's run (7520 + 10394 tests), and the new FIFO tests correctly skip where mkfifo is unavailable.
Conclusion
Fix verified end-to-end on Linux: it eliminates a real process-wedge-at-exit + total-event-loss failure mode, late-reader delivery works through the kernel pipe buffer, and no regressions were found in the regular-file, pre-connected-reader, or reader-disconnect paths. Observations above are doc/cosmetic only. LGTM from the runtime-verification side.
e48d5a4 to
22e6fe1
Compare
|
Qwen Code review did not complete successfully: Qwen review timed out after 55 minutes. See workflow logs. |
wenshao
left a comment
There was a problem hiding this comment.
No review findings — all previous issues addressed in this commit. Downgraded from Approve to Comment: CI failing: review-pr, Test (windows-latest, Node 22.x). — qwen3.7-max via Qwen Code /review
22e6fe1 to
959b9fd
Compare
959b9fd to
6eefe0e
Compare
|
Qwen Code review did not complete successfully: Qwen review timed out after 55 minutes. See workflow logs. |
6eefe0e to
6dcbc43
Compare
wenshao
left a comment
There was a problem hiding this comment.
Downgraded from Approve to Comment: CI still running.
Independent runtime verification (Linux) — recommend merge, with notesVerified this PR by building two real esbuild bundles from Verdict: the fix is real and I recommend merging. Every scenario improves or stays equal, no regressions found. Two calibration notes below on the PR's mechanism narrative and one test, for the record. Runtime A/B matrix (
|
| Scenario | BEFORE (main) | AFTER (PR) |
|---|---|---|
| Launch with no reader connected | ✅ TUI renders in 0.94s (see note 1) | ✅ TUI renders in 0.94s |
| Reader attaches late, while TUI alive | ✅ receives session_start (401 B) |
✅ receives session_start (kernel pipe buffer) |
/quit after reader came & went |
✅ exits 0.16s | ✅ exits 0.16s |
/quit with no reader ever connected |
❌ process hangs indefinitely (>12s, reproduced twice; exits only once a reader attaches — and then the queued events are lost, reader gets 0 bytes/EOF) | ✅ exits cleanly in 0.16s |
| Pre-connected reader, full lifecycle | ✅ (unchanged code path) | ✅ start + end events, 628 B incl. session_end delivered on quit |
| Regular file (control) | ✅ | ✅ |
Note 1 — the BEFORE failure mode is exit-hang, not startup-hang (Linux)
On this Linux box the PR's headline symptom ("TUI hangs indefinitely on startup, frozen event loop") does not reproduce on current main: createWriteStream(path, {flags:'w'}) performs its blocking open(2) on the libuv threadpool, so the event loop keeps running and the TUI renders in the same 0.94s as a regular file (v0.17.0's DualOutputBridge is identical to main's, so this isn't a version gap). A standalone repro script confirms it: constructor returns at 0ms, ticks keep firing, OPEN/flush complete the moment a reader attaches.
What main actually suffers from — reproduced and shown in the matrix — is arguably worse for users:
- Exit hang: with no reader ever connected,
/quitleaves the process alive indefinitely (shutdown()awaits'close'on a stream whoseopen()never completed; the pending threadpool open also refs the loop). The user's terminal is stuck until theykillor some reader opens the FIFO. - Data loss at quit: if a reader attaches after
/quit, the process exits but the queuedsession_start/session_endnever reach it (0 bytes, clean EOF). - Events buffer in user-space memory without bound until a reader appears (no kernel backpressure, no cap).
The fix genuinely cures all three (O_RDWR fd is open from construction → end() flushes & closes immediately; events sit in the kernel pipe buffer instead). The author's macOS observation ("createWriteStream on FIFO hangs >3s until killed") matches the standalone script behavior — the pending open keeps the process alive — which I suspect was read as a startup freeze. If macOS genuinely fails to render the TUI pre-fix I'd love a correction, but threadpool semantics are the same there. Suggested tweak (optional): commit title/body could say "prevent FIFO exit-hang and event loss when no reader is connected" — same fix, accurate symptom. The hung-exit defect is also exactly what the new does not block when opened without a reader test pins (see below), so the test suite and the real defect agree even if the prose doesn't.
Safety net probe (built AFTER artifact, real FIFO, no reader)
Pumping 8 KB events through the built DualOutputBridge: after ~64 KB (= Linux kernel pipe capacity) the write fails and the bridge self-disables cleanly; shutdown() resolves in 1ms; process exits. Two findings:
- The error surfaced is
code === 'ERR_SYSTEM_ERROR'— the PR's new handler branch for "EAGAIN on a full non-blocking FIFO" is correct as written (nice catch; I verified the claim rather than trusting it). - In the FIFO scenario the EAGAIN path always fires long before the 1 MB
writableLengthguard (peak observed: 65 KB). The guard is still a sensible second line of defense for slow-draining targets; just noting the unit tests rightly have to synthesizewritableLengthbecause the real FIFO path can't reach it.
#4727 end-to-end status (this PR says "Fixes #4727")
The issue's exact repro uses FIFOs for both --json-file and --input-file. Verified with a scripted mock OpenAI provider:
- Both-FIFO launch (issue's setup): TUI now starts fine ✅, output FIFO delivers
session_start✅ — butecho '{"type":"submit",...}' >> input.fifoblocks forever (rc=124 after 5s timeout) and the TUI never receives the command (0 provider requests). This matches the docs note this PR adds (RemoteInputWatcherpollsstat.size, which is always 0 for FIFOs) — the input half of Dual Output模式运行TUI无响应 #4727 is documented as unsupported, not fixed. - Docs-recommended setup (FIFO output + regular-file input): full flow works — remote submit → TUI processes → mock reply renders → FIFO reader receives the complete
session_start → user → stream_event×6 → assistant → session_endsequence (3150 B),session_endincluded thanks to this fix.
Since merging will auto-close #4727, consider replying there with the recommended configuration (or keep a follow-up open for FIFO input support, e.g. a streaming reader instead of stat.size polling).
Unit tests & revert-proof
- AFTER tree: 17/18 pass. The one failure,
throws actionable error when FIFO lacks read permission, is environment-specific: it always fails when run as root (root'sCAP_DAC_OVERRIDEignores thechmod 000, the open succeeds, nothing throws). Verified: same open throwsEACCESasnobody. GitHub runners (non-root) will pass, but root dev boxes/containers won't — suggestit.skipIf(process.getuid?.() === 0). - Related: with
chmod 000, the firstO_WRONLYopen already failsEACCES, which the constructor re-throws raw — the test passes only because Node's raw message also matches/permission denied/. The PR's new friendly message is only reachable for a writable-but-unreadable FIFO (verified:chmod 002+ non-owner → exact new message). Changing the test tochmod 200would pin the new branch it was written for (root-skip still needed). - Revert-proof (old impl + new tests): 5/18 fail — the 3 overflow-guard tests, plus
does not block when opened without a readerfailing by 10s timeout inafterEach→shutdown(), i.e. the test catches the real exit-hang defect.delivers events to a reader that connects after constructionpasses on the old impl too (late delivery already worked; it's regression protection rather than fix-pinning).
Static
prettier --check ✅ eslint ✅ tsc (full package build) ✅. Minor: the diff removes ~15 unrelated blank lines throughout the file, which inflates review noise — harmless, but worth avoiding.
测试环境: Linux 6.12 / node 22, 真实 esbuild bundle A/B(BEFORE=main, AFTER=main+PR), tmux 驱动真实 TUI + mkfifo + mock provider。
结论: 推荐合并。修复真实有效——Linux 实测 main 的缺陷形态是退出挂死 + quit 时事件丢失(TUI 启动其实不挂,渲染 0.94s 与普通文件相同;createWriteStream 的 open 在线程池里不冻结事件循环),本 PR 三个缺陷全部治愈,无回归。注意 "Fixes #4727" 仅覆盖输出半边:双 FIFO 场景下 --input-file 仍不工作(echo 卡死,watcher 依赖 stat.size——已在文档中声明为限制),合并后 issue 自动关闭时建议在 issue 里给出推荐配置。permission 单测在 root 环境必失败(建议 skipIf(uid===0),并把 chmod 000 改为 chmod 200 以真正覆盖新增的友好报错分支)。
6dcbc43 to
1faf624
Compare
1faf624 to
0fb3233
Compare
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Suggestion] docs/users/features/dual-output.md:179 — Line 179 still says "The bridge opens FIFOs with O_NONBLOCK and falls back to blocking mode on ENXIO" — describing the pre-PR behavior. The new FIFO section at line 230 correctly says the bridge uses O_RDWR | O_NONBLOCK. These two statements contradict each other within the same document.
Suggested fix: update line 179 to: "The bridge opens FIFOs with O_NONBLOCK; if no reader is connected yet (ENXIO), it retries with O_RDWR | O_NONBLOCK so startup never blocks."
— qwen3.7-max via Qwen Code /review
0fb3233 to
a7e6944
Compare
DragonnZhang
left a comment
There was a problem hiding this comment.
Reviewed at a7e6944. The FIFO non-blocking open fallback (O_RDWR | O_NONBLOCK on ENXIO) is a correct POSIX pattern. The buffer overflow guard (1MB writableLength threshold + self-disable) properly prevents unbounded memory growth when no reader drains the pipe. The shutdown fix checking stream.destroyed in addition to stream.closed correctly resolves the promise after destroy() without waiting for the async 'close' event. Error handling is comprehensive — all stream errors disable the bridge, and all public write methods follow the double-check active pattern around disableIfBufferOverflowed(). Tests cover the key scenarios including FIFO open-without-reader, late-connecting reader, permission errors, and buffer overflow. No high-confidence issues found.
…oy, tests - Rename isBufferOverflowing() to guardActive() (command-query separation) - Apply buffer guard to all write methods, not just processEvent - Call stream.destroy() on overflow so FIFO consumers get EOF - Handle destroyed stream in shutdown() to prevent hanging - Add test: bridge disables on buffer overflow + stream is destroyed - Fix doc: --input-file requires regular file (not FIFO), stat.size=0 Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
a7e6944 to
04894ac
Compare
Re-verification after the review-feedback force-push (
|
Review-feedback item (04894acb4) |
Verified |
|---|---|
skipIf(process.getuid?.() === 0) on the FIFO suite |
✅ As root the FIFO suite now skips instead of failing. Confirmed the root cause directly: as root the O_RDWR retry succeeds (CAP_DAC_OVERRIDE ignores perms) so nothing throws — exactly why the skip is required. |
EACCES test switched to chmod 0200 (write-only) |
✅ As a non-root owner of a 0200 FIFO: O_WRONLY → ENXIO, then O_RDWR → EACCES → the friendly permission denied opening FIFO for read-write branch is reached. The test now pins the branch it was written for. |
ERR_SYSTEM_ERROR gets its own clean WARN |
✅ Present in the error handler; the ugly SystemError […] undefined returned undefined line from before is gone. |
Doc: --input-file requires a regular file (FIFO stat.size=0) |
✅ Added; Quick Start now uses regular files, FIFO moved to an advanced section. Matches the #4727 input-half limitation I reported. |
isBufferOverflowing() → disableIfBufferOverflowed() (command/query), applied to all write methods |
✅ Present on all 10 emit/process methods. |
stream.destroy() on overflow so consumers get EOF |
✅ New behavior — verified end-to-end (below). |
shutdown() handles a destroyed stream (`closed |
Unit tests & revert-proof
- AFTER (
vitest run src/dualOutput/, root): 16 passed | 3 skipped. The 4 new/updated cases pass: disables at 1 MB, destroys the stream on overflow so consumers receive EOF, shutdown resolves immediately after overflow, disables on ERR_SYSTEM_ERROR. The 3 FIFO cases skip (root) by design. - Revert-proof (merge-base
DualOutputBridge.ts+ this PR’s test): 3 fail — the three overflow-guard tests (1 MB / destroy→EOF / shutdown-after-destroy) — so they genuinely pin new behavior. - Minor, for the record: “disables on ERR_SYSTEM_ERROR” also passes on the old source, because the pre-existing handler already set
active=falseon any stream error. The newERR_SYSTEM_ERRORbranch only improves the log message, so that one test is regression coverage rather than fix-pinning. Not a problem.
New overflow→destroy→EOF behavior (driven against the real source)
- 1 MB guard fires →
stream.destroy()→ connected reader gets EOF:isConnected=false,stream.destroyed=true, reader received EOF = true (gotsession_startthen end). The headline new behavior works. - Which trigger fires depends on write size/cadence (both end in a clean self-disable and a reader EOF, since a stream
errorauto-destroys too):- small/slow writes (4–8 KB): disable via
ERR_SYSTEM_ERROR(EAGAIN) at ~34–65 KB (kernel pipe capacity); - large bursts (64 KB): queue into Node’s buffer past 1 MB (peak observed 1,051,200 B) → the 1 MB guard fires.
- This refines my earlier note: the 1 MB guard is not merely theoretical — it is reachable for bursty producers; the
ERR_SYSTEM_ERRORpath covers the small-write case. Both are handled correctly.
- small/slow writes (4–8 KB): disable via
Runtime A/B (real TUI in tmux, --json-file <FIFO>, no reader connected)
| Scenario | BEFORE (main) | AFTER (PR) |
|---|---|---|
| Launch, no reader | ✅ TUI renders in 309 ms | ✅ TUI renders in 309 ms |
/quit, no reader ever connected |
❌ process hangs >20 s (never exits; quit-summary frozen on screen) | ✅ clean exit in 256 ms |
| Hung process + reader attaches afterward | process exits, but reader gets 0 bytes — buffered events lost | n/a |
| Reader attaches while TUI alive | — | ✅ receives backlog from kernel pipe (404 B, session_start) |
| no-reader FIFO fd | — | lrwx = O_RDWR (flags 02104002 = `O_RDWR |
This is the same exit-hang + event-loss failure mode I documented before, reproduced on the current bundles — the refactor preserves the fix. (As noted previously, on Linux the startup itself doesn’t hang on main — createWriteStream’s blocking open() runs on the libuv threadpool — so the real cure is for the exit hang and event loss, which the new build still delivers.)
Static & CI
prettier --check✅,eslint✅,tsc --noEmit(cli) ✅ no dualOutput errors.- CI on
04894acb4: Lint ✅, CodeQL ✅, Test ubuntu/macOS/Windows all ✅ (the earlier Windows-launcher flake is no longer present).
One tiny doc nuance (non-blocking)
The doc says the bridge “auto-disables once the internal buffer exceeds 1 MB.” For slow/small producers it actually self-disables earlier, at kernel-pipe capacity (~64 KB) via the ERR_SYSTEM_ERROR path; the 1 MB guard is the path for bursty producers. End state (bridge off, TUI unaffected, reader EOF) is identical, so this is wording only — author’s call.
中文说明(复核报告)
针对评审反馈后的强推提交(04894acb4,“address review feedback — guardActive, stream.destroy, tests”)做了完整的本地运行时复核。该提交在我前两次报告之后才合入。
环境:Linux 6.12 / Node v22.22.2。从当前 origin/main(dc6edcd52)构建了两个真实 esbuild bundle —— BEFORE(main) 与 AFTER(main+本 PR),用 tmux 驱动真实 TUI,并补充单测 / revert-proof / 静态检查 / OS 级探针。两个 bundle 已证明不同(O_RDWR+EACCES 文案标记:AFTER=1/1,BEFORE=0/0)。
结论:跟进提交正确地处理了评审意见(包含我提出的几项),此前所有问题均已解决,且这次重构没有破坏核心修复。建议合并。
评审反馈项逐条核对(均✅):
skipIf(getuid===0):root 下 FIFO 套件现在“跳过”而非失败;根因已直接验证——root 因 CAP_DAC_OVERRIDE 会让O_RDWR重试成功,因此不会抛错,所以这个 skip 是必须的。- EACCES 测试改用
chmod 0200(只写):以非 root 的属主身份验证:O_WRONLY→ENXIO,再O_RDWR→EACCES,命中友好报错分支permission denied opening FIFO for read-write,正好覆盖到新分支。 ERR_SYSTEM_ERROR独立 WARN:已加入;之前那条难看的SystemError […] undefined returned undefined不再出现。- 文档:
--input-file必须用普通文件(FIFO 的stat.size恒为 0):已补充,Quick Start 改用普通文件,FIFO 移到高级章节。对应我之前报告的#4727输入侧限制。 isBufferOverflowing()→disableIfBufferOverflowed()(命令/查询分离),且应用到所有写方法:全部 10 个 emit/process 方法均已加上。- 溢出时
stream.destroy()让消费者收到 EOF:新行为,已端到端验证。 shutdown()处理已销毁的流(closed || destroyed):溢出销毁后shutdown()立即 resolve。
单测 & revert-proof:
- AFTER:16 通过 | 3 跳过(FIFO 套件 root 下按设计跳过)。四个新增/更新用例全部通过(1MB 禁用 / 销毁流→EOF / 溢出后 shutdown 立即返回 / ERR_SYSTEM_ERROR 禁用)。
- revert-proof(旧源码 + 本 PR 测试):3 个溢出守卫用例失败 → 确实锁定了新行为。
- 小提示:
disables on ERR_SYSTEM_ERROR在旧源码上也通过,因为旧的错误处理本就对任意流错误置active=false;新分支只是改善日志文案。该用例属回归保护而非锁定新分支,无碍。
新的“溢出→销毁→EOF”行为(对真实源码驱动):
- 1MB 守卫触发 →
stream.destroy()→ 已连接的 reader 收到 EOF:isConnected=false、destroyed=true、reader 收到 EOF。 - 触发路径取决于写入大小/节奏(两条路径都会干净自禁用,且 reader 都能收到 EOF,因为流 error 会自动 destroy):
- 小/慢写入(4–8KB):在 ~34–65KB(内核管道容量)经
ERR_SYSTEM_ERROR(EAGAIN) 禁用; - 大块突发(64KB):在 Node 缓冲里堆积超过 1MB(实测峰值 1,051,200 B)→ 1MB 守卫触发。
- 这修正了我之前的说法:1MB 守卫并非纯理论,突发写入场景下确实可达;小写入则由
ERR_SYSTEM_ERROR分支兜底。两者都处理正确。
- 小/慢写入(4–8KB):在 ~34–65KB(内核管道容量)经
运行时 A/B(真实 TUI,--json-file <FIFO>,无 reader):
- 启动无 reader:BEFORE/AFTER 均 309ms 正常渲染(Linux 上启动本身不挂)。
/quit且从未有 reader:BEFORE 挂死 >20s(永不退出,退出统计框卡屏);AFTER 256ms 干净退出。- 挂死后再接入 reader:BEFORE 进程退出但 reader 收到 0 字节(事件丢失)。
- TUI 存活时接入 reader:AFTER 从内核管道收到积压(404B,
session_start)。 - 无 reader 的 FIFO fd:
lrwx= O_RDWR(flags02104002)。
即此前记录的“退出挂死 + 事件丢失”形态,在当前 bundle 上复现,重构后修复依旧有效。
静态 & CI:prettier ✅、eslint ✅、tsc ✅(无 dualOutput 类型错误);04894acb4 的 CI:Lint ✅、CodeQL ✅、ubuntu/macOS/Windows 测试全 ✅(之前的 Windows launcher 抖动已消失)。
一个小的文档措辞(不阻塞):文档称“内部缓冲超过 1MB 即自禁用”。对慢/小生产者,实际会更早(~64KB 内核管道)经 ERR_SYSTEM_ERROR 路径自禁用;1MB 守卫面向突发生产者。最终状态一致(bridge 关闭、TUI 不受影响、reader 收到 EOF),属措辞问题,作者酌定。
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ — all required sections filled in properly, bilingual description included. On direction: this fixes a real, user-reported problem (#4727) — FIFO-based dual output wedges the process on exit when no reader is connected. Solidly within scope; dual-output is a first-class feature and FIFO is a natural use case for low-latency event streaming. The fix is targeted and doesn't expand scope beyond what the bug requires. On approach: the POSIX Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 方向:修复了真实用户问题(#4727)——FIFO 双输出在无 reader 时退出挂死。完全在项目范围内;dual-output 是一等特性,FIFO 是低延迟事件流的自然选择。修复目标明确,没有扩大范围。 方案:POSIX 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code reviewThe implementation is clean and focused. I walked through the three changed files: DualOutputBridge.ts — The ENXIO retry path is correct: DualOutputBridge.test.ts — 19 tests, all passing. The 4 new overflow-guard tests pin the new behavior (verified via revert-proof: 3/4 fail on old code). The 3 FIFO tests correctly skip when running as root ( dual-output.md — Quick Start switched to regular files, FIFO documented as an advanced option with the Real-scenario testing (tmux, bundled builds)Both BEFORE (main, Before (main v0.18.0)TUI starts fine (blocking After (this PR)Unit testsSummary
中文说明代码审查实现干净、聚焦。三个改动文件: DualOutputBridge.ts — ENXIO 重试路径正确:先 测试 — 19 个测试全部通过。4 个新的溢出守卫测试锁定了新行为(revert-proof 验证:旧代码上 3/4 失败)。FIFO 测试在 root 下正确跳过。 文档 — Quick Start 改用普通文件,FIFO 作为高级选项, 真实场景测试
— Qwen Code · qwen3.7-max |
|
Stepping back: this is a textbook bug fix. The problem is real (process wedges on exit when a FIFO has no reader), the root cause is well-understood ( The implementation matches what I'd propose independently. The scope is tight — no speculative features, no unnecessary abstractions. The buffer-overflow guard is a sensible safety net for the The tmux before/above confirms it: main hangs on No reservations. Shipping this. 中文说明这是一个教科书式的 bug 修复。问题真实存在(FIFO 无 reader 时退出挂死),根因清晰( 实现与独立提案一致。范围紧凑——无投机特性、无过度抽象。缓冲区溢出守卫是 tmux 前后对比确认:main 在 没有顾虑,推荐合并。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
…nected (#4894) * fix(dual-output): prevent FIFO blocking on startup when no reader connected DualOutputBridge's ENXIO fallback used a blocking createWriteStream on FIFOs, causing the TUI to hang indefinitely when launched with `--json-file <fifo>` before a reader connects (issue #4727). Fix: use O_RDWR | O_NONBLOCK for the FIFO fallback path. This POSIX trick satisfies the kernel's "at least one reader" requirement without blocking. A buffer high-water-mark (1 MB) self-disables the bridge if no consumer ever drains the pipe. Also updates Quick start docs to recommend regular files as the default, with FIFOs documented as an advanced option that now works without ordering constraints. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(dual-output): address review feedback — guardActive, stream.destroy, tests - Rename isBufferOverflowing() to guardActive() (command-query separation) - Apply buffer guard to all write methods, not just processEvent - Call stream.destroy() on overflow so FIFO consumers get EOF - Handle destroyed stream in shutdown() to prevent hanging - Add test: bridge disables on buffer overflow + stream is destroyed - Fix doc: --input-file requires regular file (not FIFO), stat.size=0 Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
What this PR does
Fixes the FIFO (named pipe) startup blocking issue in
DualOutputBridge. When--json-filepoints to a FIFO with no reader connected, the bridge now opens it withO_RDWR | O_NONBLOCKinstead of falling back to a blockingcreateWriteStream. Also adds a 1 MB buffer high-water-mark safety net that auto-disables the bridge if no consumer ever drains. Updates dual-output docs to recommend regular files in the Quick Start and document FIFOs as an advanced option.Why it's needed
Users launching
qwen --json-file <fifo> --input-file <fifo>withmkfifo-created FIFOs found the TUI hanging indefinitely (#4727). The root cause:DualOutputBridge's ENXIO fallback usedcreateWriteStream(path, {flags:'w'})which callsopen(O_WRONLY)on a FIFO — this blocks in the kernel until a reader connects, freezing the Node.js event loop and preventing the TUI from ever starting.Reviewer Test Plan
How to verify
Evidence (Before & After)
Before:
qwen --json-file <fifo>without a pre-connected reader blocks indefinitely (confirmed via Node.js reproduction —createWriteStreamon FIFO hangs >3s until killed).After: Constructor completes in ~3ms using
O_RDWR | O_NONBLOCK. Events buffer in the kernel pipe until a reader attaches.Tested on
Environment
Local runtime, manual Node.js scripts simulating
DualOutputBridgeconstructor logic.Risk & Scope
O_RDWR, EPIPE no longer fires when the reader disconnects (process is its own reader). Bridge self-disables via buffer overflow (1 MB limit) instead — slightly delayed but acceptable since the alternative was a completely broken startup.O_WRONLY | O_NONBLOCKattempt succeeds as before).Linked Issues
Fixes #4727
中文说明
修复
DualOutputBridge在 FIFO(命名管道)上启动阻塞的问题。当--json-file指向一个没有 reader 连接的 FIFO 时,bridge 现在使用O_RDWR | O_NONBLOCK打开,而不是 fallback 到阻塞的createWriteStream。根因:
open(O_WRONLY)在 FIFO 上会阻塞直到有 reader 连接,冻结了 Node.js 事件循环,TUI 无法启动。修复方案使用 POSIX 标准技巧:
O_RDWR使进程同时作为 reader 和 writer,内核的 "至少一个 reader" 条件自动满足,open 立即返回。额外增加了 1MB buffer 上限安全网,防止无 consumer 时内存无限增长。同时更新了 dual-output 文档,Quick Start 改为推荐普通文件,FIFO 作为高级低延迟选项单独说明。