Skip to content

fix(terminal): fix compound-background rewrite bugs (syntax error + unhandled paren subshell) - #68935

Closed
x7peeps wants to merge 1 commit into
NousResearch:mainfrom
x7peeps:fix/issue-68915-worker-deadlock-server-background
Closed

x7peeps wants to merge 1 commit into
NousResearch:mainfrom
x7peeps:fix/issue-68915-worker-deadlock-server-background

Conversation

@x7peeps

@x7peeps x7peeps commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

背景

修复 #68915: Worker deadlock when agent backgrounds a server via shell &

当前 main 上已有的 _rewrite_compound_background 存在两个问题:

  1. 语法错误:A && B & C 被重写为 A && { B & } C,bash 报 syntax error near unexpected token
  2. 括号子 shell 未处理:(A && B) & 被故意跳过,但它和 A && B & 有同样的子 shell-wait + pipe-hold deadlock 问题

根因分析

  1. bash 中 & 是语句终结符,后面紧跟的命令需要 ; 或换行分隔。原重写器没有处理 & 后面有同命令的情况,导致输出 A && { B & } C 是无效的 bash 语法。
  2. 括号子 shell (A && B) & 同样会 fork 一个 subshell 来等待 B 完成,B 如果是长期运行的服务进程就会 deadlock。

修复方式

  1. 当 & 后面同一行有后续命令时,在 brace group 后插入 ; 分隔符:A && { B & }; C
  2. 新增括号子 shell 扫描:将 (A && B) & 重写为 { A && B & }
  3. 修正多个重写应用顺序(从后往前),避免索引失效导致输出被破坏

验证

  • 所有 39 个回归测试通过(33 原有 + 6 新增)
  • 所有重写输出经 bash -n 验证语法正确
  • 遵循项目 AGENTS.md 贡献规范
  • 无破坏性变更

Closes #68915

…nhandled paren subshell)

Fix NousResearch#68915

Root cause:
1. The rewriter produced 'A && { B & } C' for 'A && B & C', which is a bash
   syntax error because bash requires a ';' or newline before the next command
   after '&'.
2. Parenthesised subshells '(A && B) &' were deliberately skipped but have
   the same subshell-wait + pipe-hold deadlock bug class.

Fix:
1. When the '&' background operator is followed by a command on the same line,
   insert a ';' separator: 'A && { B & }; C'.
2. Rewrite '(A && B) &' to '{ A && B & }' (same brace-group strategy).
3. Process all rewrites back-to-front with correct sort order to avoid stale
   indices corrupting the output string.

Tests: added TestTrailingCommand (3 tests) and updated
TestQuotingAndParens (3 tests) for parenthesised subshell handling.
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround tool/terminal Terminal execution and process management labels Jul 21, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related to #41368 and its duplicate #42278: this PR includes the same separator repair but additionally covers parenthesized compound subshells implicated by #68915. These are overlapping but materially different scopes; maintainer consolidation is needed.

@PRATHAMESH75

Copy link
Copy Markdown
Contributor

Reviewed against #68915. I checked out the rewriter in isolation and ran every output through bash -n.

This is a correct fix for two genuine gaps, not the whole issue — and that's the right scope.

  • The exact repro command in the issue (cd /app && node server.js &>/tmp/server.log &) is already rewritten correctly on main → cd /app && { node server.js &>/tmp/server.log & }. So the headline deadlock for that command is not what this PR fixes.
  • What main gets wrong, and this PR fixes:
    • A && B & C → main emits A && { B & } C, which is invalid bash (bash -n: syntax error near unexpected token 'C'). The rewriter was corrupting any compound-background command that had a trailing statement on the same line. This PR emits A && { B & }; C ✅.
    • (A && B) & → main leaves it untouched, so the subshell-wait deadlock is still live. This PR rewrites it to { A && B & } ✅.
  • I confirmed the PR's outputs are syntactically valid and behavior-preserving across the cases I care about: the already-working redirect repro, A || B & C, (A && B) & C, (A) & (correctly left alone), echo hi & echo bye (left alone), and the newline case. All pass bash -n.

CI: the one red required check (Python tests / Run tests slice 7/8) is unrelated to this PR — it's tests/tools/test_mcp_tool_session_expired.py::test_session_expired_retry_waits_for_new_session (an MCP retry-count assertion), which touches none of the code here. Looks flaky/pre-existing; a re-run should clear it.

Minor (non-blocking): in the merge/dedup step, the overlap key reads item[3] for both kinds, but for a paren tuple item[3] is close_pos, not amp_pos (that's item[4]). It's currently harmless — a &&/|| inside (...) is skipped by the brace scanner (paren_depth > 0), so a brace and paren rewrite can't target the same operator, and the dedup never actually fires. Worth either fixing the key or dropping the dedup as dead code, for the next reader.

Scope note for maintainers: this closes the parse-time rewriter gaps (expected-behavior option 1 in the issue). The defense-in-depth pieces the issue also lists — a stuck-process timeout in the main conversation loop (option 2/3) — remain separate follow-ups and aren't in scope here.

Nice, focused fix with real bash -n-backed regression tests.

@x7peeps

x7peeps commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Closed as duplicate per maintainer feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 High — major feature broken, no workaround tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Worker deadlocks when agent backgrounds a server via shell & (orphaned subshell holds stdout pipe open)

3 participants