Repository navigation
fix(cli): 大输出写完 stdout 再退出，避免 ask/hook/relay 截断 - #1026
Conversation
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
DeepColds
left a comment
DeepColds
left a comment
There was a problem hiding this comment.
结论：通过，未发现阻塞问题。
- 修复点与根因匹配：以
stdout.write的 completion callback 作为完整写入边界，避免大于 pipe 高水位时process.exit(0)截断尾部。 - 先完成 stdout 写入，再断开 PM2 RPC 并退出；未改变只读 observer 与 fail-closed 语义。
- 改动范围集中在
jlist输出路径，不影响 status/logs、CLI adapter、会话后端或飞书链路。
本地验证：
pnpm build：通过。pnpm exec vitest run --project unit test/stdout-flush.test.ts test/desktop/desktop-pm2-apps.test.ts test/pm2-jlist.test.ts test/shutdown-supervisor-contract.test.ts：4 个文件、48 个测试通过。- 额外以子进程 pipe 写入 4 MiB payload：实际收到 4,194,304 / 4,194,304 bytes，退出码 0。
完整 pnpm test 也已执行；本环境中有 6 个与本 PR 无关的既有/环境敏感失败，其中 PM2 cgroup、进程名和 redirect 环境项可在当前 master 复现，不影响本次结论。
|
感谢这个修复 🙏 根因分析非常准确，修法也是最小改动。我在本地做了独立验证，确认这是个真实的生产 bug，先说结论：方向、根因、修法都没问题，只有新增测试的有效性想和你商量一下。 复现与修复效果都已坐实因为是 fork PR 不跑 CI，我在 Linux / Node 22 上复刻了
我还把因果链闭合到了用户看到的那句报错：把真实 payload 截到 146176 字节喂进真的 回归面也确认干净： 另外顺手枚举了全仓同类写法，可以确认没有第二个未修实例： 一个补充：影响面可能比描述里更广描述提到「在 macOS 上首次稳定复现」。我实测 Linux 下同样必中（上表数据就是 Linux 跑的）。因为 daemon 生产环境跑在 Linux，这其实是生产路径上的真实 bug，不只是 mac 本地开发的困扰 —— 这点或许值得在描述里改得更准，也更能说明这个修复的价值。 主要建议：
|
|
补充两点更正与更精确的信息（复审交叉后得到，其中一条是修正我上一条评论里的说法）： 1. 更正：真实截断阈值是 ~146 KiB，不是我上面说的 64 KiB我上一条评论按「超过 pipe 高水位 64 KiB 就会截断」来描述，这不够准确。我做了逐档实测（Linux / Node 22.21.1，
两个反直觉的点：
这不改变这个 PR 的结论（jlist 实测 474 KB ~ 878 KB，远超阈值，必中），但对写测试有实际影响：如果只把 payload 顶到「刚过 64 KiB」，测试会假绿（因为那个区间其实是安全的）。所以测试必须确保输出稳稳越过 ~146 KiB。 我上一条给的写法（6 进程 × 12 个 4000 字节 env）实测约 474 KB，是安全的；不过下面这个等价写法更简洁，也同样越过真实阈值（实测 474,752 字节），双向验证过有牙 —— 建议直接用这版： // 加进 test/pm2-readonly-cli.test.ts 的 describe 内（需补 writeFileSync import）
// 6 × 30KB env → jlist 实测 474,752 字节，稳稳越过 ~146 KiB 的同步冲刷阈值。
it('delivers a jlist past the synchronous flush ceiling without truncation', () => {
const home = tempHome();
const pm2Home = join(home, '.botmux', 'pm2');
mkdirSync(pm2Home, { recursive: true });
expect(spawnSync(process.execPath, [PM2_PATH, 'status'], {
env: { ...process.env, PM2_HOME: pm2Home },
stdio: 'ignore',
timeout: 10_000,
}).status).toBe(0);
const idleScript = join(home, 'idle.js');
writeFileSync(idleScript, 'setInterval(() => {}, 1000);\n');
for (let i = 0; i < 6; i++) {
const started = spawnSync(process.execPath, [PM2_PATH, 'start', idleScript, '--name', `flush-probe-${i}`], {
env: { ...process.env, PM2_HOME: pm2Home, BOTMUX_FLUSH_PROBE: 'x'.repeat(30_000) },
stdio: 'ignore',
timeout: 20_000,
});
expect(started.status).toBe(0);
}
const parsed = JSON.parse(captureReadonlyPm2Jlist({
pkgRoot: dirname(dirname(CLI_PATH)),
home: pm2Home,
})) as unknown[];
expect(parsed).toHaveLength(6);
}, 90_000);双向验证：打上修复 ✅ 通过（约 2.2s）；撤掉修复 ❌ 死在 2. 更正：同模式还有第二处（不阻断本 PR，建议另开 follow-up）我上一条说「没有发现第二个未修实例」，这句话说过头了，交叉复审时被指出还有一处：
不过按上面测出的真实阈值，触发需要用户粘贴 ~14 万字符以上，比我原本估计的更低频。所以：不阻断这个 PR，建议单独记个 follow-up issue 即可。 另外顺带说明，仓库里其余同形状的站点我都核过是安全的： 同样是自动评审的补充意见，最终以维护者审阅为准。核心修复的结论不变：真 bug、修法正确、值得合入，只是测试希望能真正锁住回归 🙏 |
|
已根据评论更新并推送 bfe372f。
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
bfe372f to
03c4d58
Compare
|
已基于最新 master（8c489709）完成 rebase 并强制更新分支。 冲突来自上游删除旧的 pm2-readonly-cli 生命周期测试；该测试绑定了已不再采用的 cgroup 生命周期假设。此次没有恢复旧测试，而是将本 PR 的大输出回归覆盖迁为独立的 test/pm2-readonly-jlist.test.ts，仅覆盖当前仍被桌面和插件调用的 read-only jlist helper。 已完成相关测试与 pnpm build；PR 描述中的测试路径和结果也已同步更新。 |
|
已基于最新 本次补充修复了复审中确认的同类 CLI 输出截断： 已实际运行：
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20cc0bd426
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| afterEach(() => { | ||
| for (const home of homes.splice(0)) { | ||
| const pm2Home = join(home, '.botmux', 'pm2'); | ||
| spawnSync(process.execPath, [PM2_PATH, 'kill'], { |
There was a problem hiding this comment.
The new regression test invokes PM2 directly with spawnSync(process.execPath, ...) here and again for status and start, bypassing the repository’s mandatory runtime-aware subprocess abstraction. This leaves the test and its teardown outside the supported Node/Bun launch contract, so they can fail or exercise different behavior when the suite runs under Bun; use spawnSyncTsScript from test/helpers/ts-runner.ts for all three invocations.
AGENTS.md reference: AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
20cc0bd to
a29634b
Compare
deepcoldy
changed the title
|
感谢原作者的根因分析与修复 🙏 已 rebase 到最新 master 并推送
验证： |
stdout 接 pipe 时 write() 是异步的，紧跟显式 process.exit() 会丢掉未排空 的尾部（实测父进程只收到约 146 KiB）。PM2 只读 jlist 的同类问题已在 master 修复，本提交补齐其余仍会输出大文本的退出路径： - 新增 src/cli/stdout-flush.ts 的 writeAndFlush：以 write 完成回调为边界 - botmux ask：写完结果再 ack；写失败时不 ack、exit 1，daemon 仍可回收回执 - botmux hook：directive 内嵌用户文字作答，同样等写完再 exit 0 - sandbox relay：send 与 dispatch 两处统一走 mirrorRelayResponseAndExit， stdout/stderr 均写完再镜像退出码 - 测试：writeAndFlush 子进程 2 MiB 管道回归；ask --json 50 万字符 comment 端到端回归；PM2 只读 helper 大注册表（>200 KB）回归，钉住 master 的修复。 PM2 用真实 Node 启动（bun 下 execPath 不是 node） Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a29634b to
f6b0e1a
Compare
#1026 引入的 test/pm2-readonly-jlist.test.ts 在 CI 上冷启动真实 pm2 God 时确定性失败（warmup `pm2 status` exit 1，node/bun 两腿首跑及 rerun 均复现，10403 其它用例全过；本地无法复现）。 仓库测试惯例不冷启动真实 God（legacy-pm2-reaper 用 fake、desktop-pm2-apps 用 mock spawn）；pm2 已非直接依赖、该只读 helper 属待删遗留桌面路径。删除该用例（-79，零生产改动）：编排逻辑仍由 desktop-pm2-apps 覆盖，「写完再退出」通用契约由 stdout-flush 2 MiB 管道回归与 ask 50 万字符端到端回归守住（均 CI 绿）。 #1875 自身 CI 14/14 全绿（test 三分片 + bun-test）。 Co-Authored-By: Claude Code <noreply@anthropic.com>
#1026 修了 sandbox relay 的 stdout/stderr 截断（mirrorRelayResponseAndExit）， 但 relay 调用点没有大载荷回归测试。本例起 mock host watcher 回 300KB stdout + 200KB stderr + exit 7，断言子进程逐字节送达且退出码透传。走 dispatch relay 路径（send/dispatch 共用 mirrorRelayResponseAndExit，一例盖共享逻辑）。 双向验证：修复版通过；撤掉 mirror 修复（改回 write-then-exit）立刻变红。
|
🚀 Released in v3.43.0 |
3 participants
原因（原作者背景）
Node 的 pipe 写入是异步的；调用
write()后立即退出，可能丢失尚未排空的尾部字节。PM2 registry 和ask --json的comment都可超过该边界，后者已用真实子进程复现。该 pipe 竞态在 macOS 与 Linux 上均存在（实测越过阈值后父进程恒定只收到 146,176 字节，position 146176即该 bug 的指纹）。维护者跟进
本 PR 最初修复的 PM2 只读
jlist截断，已由 master 的 #956 以write(payload, cb)形式落地。因此本次 rebase 到最新 master 时pm2-readonly-client.ts取 master 版本，PR 核心改为补齐其余「write 后直接 exit」的大输出路径，标题随之调整。改了什么
src/cli/stdout-flush.ts的writeAndFlush：以 write 完成回调为「写完」边界；botmux ask：等结果写完再 ack；写失败时不 ack、exit 1，daemon 仍可回收回执；botmux hook：claude/dsh/opencode 的 hook directive 会内嵌用户文字作答（长度无上限），改为写完再 exit 0（写失败仍 exit 0，不阻塞 CLI）；mirrorRelayResponseAndExit，stdout/stderr 均写完再镜像原退出码（send 默认 0、dispatch 默认 1，与原行为一致）；cli.ts其余 stdout/stderr 写入已排查：要么输出很小，要么只设process.exitCode（自然排空），要么已是write(payload, () => exit)，无需改动。影响面
只改 CLI 子命令（ask / hook / sandbox relay）的输出完成时机；协议、输出内容、退出码不变（新增：ask 写 stdout 失败时 exit 1 且不 ack）。不涉及 daemon、CLI 适配器、会话后端、飞书事件链路。
测试
test/stdout-flush.test.ts（新增）：经test/helpers/ts-runner.ts起子进程，经 pipe 写 2 MiB 后立刻process.exit，父进程延后读取制造背压，断言字节完整；test/ask-cli.test.ts：真实 CLI 子进程 + fake daemon，50 万字符 comment 的ask --json端到端回归；test/pm2-readonly-jlist.test.ts：按 desktop 的真实调用形态直接起pm2-readonly-client.ts jlist走 pipe（原依赖的src/cli/pm2-readonly.ts已在 master 删除），6 进程 × 30 KB env、输出 >200 KB，钉住 master 已有修复；PM2 用resolveNodeExecutable()起真实 Node（bun 下process.execPath是 bun）。实际运行：
bun run build：通过；npx vitest run --project unit12 个相关文件（stdout-flush、ask-cli、pm2-readonly-jlist、sandbox-relay-watcher、cli-send-hook-context、ask-hook-{claude,codex,dsh,opencode}、ask-resume-contract、desktop/desktop-pm2-apps、sandbox-session-data-dir）：158 通过、3 跳过；bun test（bun 1.4.2）三个新增/改动的测试文件：1 / 1 / 17 通过；expected 146176 to be greater than 200000）；让writeAndFlush不等回调 → stdout-flush 红（146176 / 2097152）、ask 红（292352 字节）。未做：全量
bun run test；未做飞书 live 验证（改动仅限 CLI 子命令输出边界，由上述子进程测试覆盖）。🤖 Generated with Claude Code