Skip to content

fix(tmux-pipe-backend): fifo 拆除时不再重复关闭 ReadStream 已拥有的 fd - #1373

Open
deepcoldy wants to merge 2 commits into
masterfrom
fix/tmux-fifo-fd-close-wedge
Open

fix(tmux-pipe-backend): fifo 拆除时不再重复关闭 ReadStream 已拥有的 fd#1373
deepcoldy wants to merge 2 commits into
masterfrom
fix/tmux-fifo-fd-close-wedge

Conversation

@deepcoldy

Copy link
Copy Markdown
Owner

问题

teardownFifoReader()stream.destroy() 之后,又对同一个 fd 调了一次 fs.closeSync()

在 bun 下这会楔死整个进程:被 destroy 的 ReadStream 会留下一个线程池线程仍然阻塞在这个 fd 的 read(2)(fifo 是我们自己以 O_RDWR 持有的,永远不会 EOF)。此时把 fd 关掉,那个线程就再也无法被 join —— 同进程内下一次 execFileSync() 永不返回,连它自己的 timeout 都不会触发

楔死瞬间的进程形状(实测):

  • 主线程停在 epoll_pwait2
  • 持有 0 个 pipe fd
  • 挂着一个 [tmux: client] <defunct> 僵尸子进程

子进程早已退出,同步调用却收不回来——这也是为什么表面上看不出是谁在阻塞。

改法

只在没有 ReadStream 拥有该 fd 时才自己关闭。destroy() 本来就会关它 —— 这一点原注释里已经写明「EBADF here is the normal, expected outcome」,那行 closeSync 原本只是 in-flight 情况的兜底,而恰恰是 in-flight 时它会踩中上面的楔死。

影响面

teardownFifoReader()TmuxPipeBackend 唯一的 fifo 拆除路径,kill() / destroySession() / handlePaneExit() / 进程退出钩子都汇到这里,所以所有 tmux 会话的收尾都会走到

  • 生产:同一进程在拆掉一个 backend 之后继续跑同步子进程(pipe-panekill-session、下一个会话的 new-session)时会命中同样的楔死。worker 主线程被冻住,心跳/取消/新消息都处理不了。
  • 其它后端PtyBackend / TmuxBackend / zmx / zellij 不走 fifo 这条路径,不受影响。
  • Node 运行时:node 下 destroy() 后 fd 已是 EBADF、线程池不留驻留线程,行为不变(该文件在 vitest/node 下一直是 2/2 通过)。

验证

目标用例 test/tmux-startup-storm-recovery.test.ts(就是 #1369 里记录的「第 4 个 flake」,其 PR 描述把真因列为 open question):

运行时 修复前 修复后
bun 1.4.0 3/3 楔死(>90s 无结果行) 3/3 通过(6s)
bun 1.4.2(CI 钉的版本) 2/3 楔死 5/5 通过(6s)
vitest / node 2/2 通过(7s) 2/2 通过

1.4.0 是确定性楔死、1.4.2 是间歇 —— 与 CI 上这个文件「时红时绿」的 flake 形状一致。

反变异:把那一行还原成无条件 closeSync,1.4.2 立刻复现楔死(70s 外部超时,无结果行)⟹ 改动是「武装」的,不是惰性编辑。

定位过程(最小 A/B):单独 destroy() 绿、单独 closeSync(fd) 绿、两者相邻则楔死;不带 backend 的同形用例 190ms 通过。

回归:逐文件(one-file-per-process,按 scripts/run-bun-tests.mjs 的要求)跑 33 个 tmux/backend 相关用例文件 —— 4 个文件有失败,但在干净 master 上同样失败(各 0 pass,属既有基础设施问题),本改动零回归test/tmux-pipe-backend.test.ts 54/54 通过。

fd 泄漏核查(这是本改法唯一的真实风险——少关一次 fd):N=25 与 N=100 两档 delta 恒为 2 且无 fifo fd 残留,不随次数增长 ⟹ 是 bun 运行时基线开销,不是本改动漏关;node 下 delta=0。

bun run build 通过,tsc --noEmit 通过。

备注

这解答了 #1369 留下的 open question。之前我一度判成 Atomics.wait 挡住了 per-test timeout,那个结论是错的(实测楔死 >290s,38s 的算术产生不了;且按那个思路做的测试侧修法 3/3 仍楔死),已作废。

🤖 Generated with Claude Code

`teardownFifoReader()` 在 `stream.destroy()` 之后又对同一个 fd 调用
`fs.closeSync()`。在 bun 下,被 destroy 的 ReadStream 会留下一个线程池线程
仍然阻塞在这个 fd 的 read(2) 上;此时关闭该 fd 会让那个线程永远无法被 join,
于是**同进程内下一次 execFileSync() 永不返回**——连它自己的 timeout 都不会触发。

实测形状:主线程停在 epoll_pwait2、持有 0 个 pipe fd、挂着一个已退出的
`[tmux: client] <defunct>` 僵尸子进程。即子进程早已退出,同步调用却收不回来。

改为只在没有 ReadStream 拥有该 fd 时才自己关闭——destroy() 本来就会关它,
注释里原本也写明了「EBADF 才是正常结果」。

影响面:`teardownFifoReader` 是 TmuxPipeBackend 唯一的 fifo 拆除路径,
kill() / destroySession() / handlePaneExit() / 进程退出钩子都汇到这里,
因此所有 tmux 会话的收尾都会走到。生产上同一进程在拆除后继续跑同步子进程
(如 pipe-pane、kill-session)时会命中同样的楔死。

验证:
- test/tmux-startup-storm-recovery.test.ts:
  bun 1.4.0 修复前 3/3 楔死(>90s 无结果行) → 修复后 3/3 通过(6s)
  bun 1.4.2(CI 钉的版本) 修复前 2/3 楔死 → 修复后 5/5 通过(6s)
- 反变异:还原这一行,1.4.2 立刻复现楔死(70s 超时)
- 该文件在 vitest/node 下一直 2/2 通过(7s),佐证是 bun 运行时行为差异
- 逐文件跑 33 个 tmux/backend 相关用例文件:4 个文件有失败,
  但在干净 master 上同样失败(各 0 pass),属既有问题,非本改动引入
- fd 泄漏核查:N=25 与 N=100 两档 delta 恒为 2 且无 fifo fd 残留,
  不随次数增长 ⟹ 是 bun 运行时基线开销,不是本改动漏关
- bun run build 通过;tsc --noEmit 通过

Co-Authored-By: Claude Code <noreply@anthropic.com>
@deepcoldy

Copy link
Copy Markdown
Owner Author

Review (pi bot, automated). ⚠️ Same shared-identity caveat: this account is the PR author, so this is a comment, not an approval.

LGTM. Independently verified:

  1. Fix is correct and complete: fifoFd is closed in exactly one place (teardownFifoReader). The hadStream capture at function entry (before readStream = null) correctly reflects "did a stream own this fd" for all current paths — readStream is created once (spawn, line 406) and nulled only here.

  2. Both runners pass: vitest 54/54, bun test 54/54. bun run build + tsc clean.

  3. Mutation check (important coverage note): reverting the fix (master version, unconditional closeSync) → the unit test still passes 54/54. The unit test does NOT catch the wedge — the A/B probe (d=destroy alone / x=closeSync alone / dx=both wedge) is the real guard. This is expected (the wedge is a process-level thread-join behavior, invisible to per-test assertions), but it means the A/B probe must be kept in the repo as the regression guard, not just the unit test.

  4. Point A (hadStream robustness): not a blocker. Currently safe (only this function nulls readStream), but "current paths" not structural. A follow-up could track fd ownership explicitly (e.g., a fifoFdOwnedByStream flag set at stream creation) if you want a hard guarantee. Worth an issue, not a merge blocker.

  5. Point B (runtime behavior reliance): acceptable. The fix relies on measured Bun behavior (destroy closes the fd despite autoClose: false). The alternative (always closeSync) IS the bug. If a future Bun stops closing on destroy, the fd leaks (minor) rather than wedges (severe) — the right failure direction.

  6. Production defect: agreed. All four teardown entry points funnel here; a tmux session teardown followed by execFileSync in the same worker could wedge it. This is not test-environment amplification.

第 4 个 flake 的长期守卫是 tmux-startup-storm-recovery.test.ts:revert 修复
后它会楔死、被 720s wall SIGKILL 转红。守卫有效但慢且钝,而且依赖「楔死一定
会发生」——将来 bun 改了线程池行为、楔死不再出现,它会静默失效(revert 也绿)。

这里补一条不依赖楔死的直接断言:spy fs.closeSync,断言 teardownFifoReader()
对 ReadStream 拥有的 fd 只关一次(修复前是两次,第二次就是楔死的那一次)。

同时修掉一个既有隐患:本文件的 node:fs mock 没有 stub closeSync/writeSync,
而 openSync 被 stub 成恒返回 7 —— 7 在测试进程自己身上是个活 fd,kill 路径
的用例一直在对它真调 closeSync(7)/writeSync(7)。

第二条用例覆盖 spawn() 的 fail-closed 路径(wake fd 打开失败):那里由 spawn()
自己关 fd 并 unlink,不经过 teardown。注释写明了为什么不在这里断言 teardown 的
`!hadStream` 分支——该分支今天没有可达调用方,断言它等于什么都没断言。

验证:
- bun test 56/56、vitest 56/56、tsc --noEmit 0 错
- 反变异四枪,每枪只红对应那条用例:
  M1 teardown 改回无条件 closeSync  -> 红「does not close the fifo fd that the ReadStream owns」
  M3 删 spawn fail-closed 的 closeSync -> 红「leaks no fd on the spawn fail-closed path」
  M4 删 spawn fail-closed 的 unlinkSync -> 红同上
  (修复态 56/56 绿)

Co-Authored-By: Claude Code <noreply@anthropic.com>
@deepcoldy

Copy link
Copy Markdown
Owner Author

Re-review of c572d869b (fast direct test added). LGTM maintained.

The follow-up test is exactly right:

  1. Fast, direct, wedge-independent: mockedCloseSync spy + spawn (creates the ReadStream) + kill → asserts closeSync called exactly once (the wake fd only; the stream-owned read fd is not). 0.6ms, doesn't rely on the wedge happening. This plugs the "guard silently dies if bun stops wedging" hole.

  2. Mutation verified: reverting teardown to unconditional closeSync → the new test fails (1 failed). Teeth confirmed, not decoration.

  3. The fake-test catch is good discipline: your second test ("no stream still closes fd") had all-green mutations = inert, because the fail-closed close lives in spawn()'s catch, not teardown — so teardown's !hadStream branch has no reachable caller today. Removing it and covering the spawn fail-closed path directly (close + unlink, no leftovers) is the honest coverage. The comment explaining why the teardown branch isn't asserted is exactly the right note.

  4. The pre-existing mock fix (stub closeSync/writeSync so the stubbed-openSync-returned-7 live fd isn't really closed/written) is a genuine latent-bug fix, well done.

  5. Both runners: 56/56. tsc clean. Issue fix(tmux-pipe-backend): fifo fd 所有权改用显式标志,替代 hadStream 间接判据 #1374 tracks the hadStream structural concern.

This PR now has three layers of guard: the fast unit test (immediate), the storm-recovery suite (slow but real), and the production fix itself. Ready for 申晗's word.

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.

1 participant