Skip to content

fix(bash): address code-review findings on background shells - #237

Merged
yogthos merged 1 commit into
mainfrom
fix/background-shells-review-followups
May 30, 2026
Merged

fix(bash): address code-review findings on background shells#237
yogthos merged 1 commit into
mainfrom
fix/background-shells-review-followups

Conversation

@yogthos

@yogthos yogthos commented May 30, 2026

Copy link
Copy Markdown
Collaborator

Post-merge code review of #236 (background-shells rework) surfaced six findings; this fixes #1#5 and documents #6.

# Sev Fix
1 HIGH spawn_streaming_shell never disarmed its PgKillGuard, so on natural (and timeout) exit the guard's Drop fired a second killpg(SIGKILL) against an already-reaped — possibly OS-recycled — process-group id (could kill an unrelated group). Now disarmed before finish() on every non-aborted completion path, matching run_with_timeout. The guard still fires on the abort path, which is exactly how kill_shell/kill_all kill the group.
2 MED-HIGH Background shells (own process group) leaked past normal exitkill_all() only ran on session swap. Now also called when run_interactive returns (/quit, Ctrl+C/D, EOF) and at the headless session-end points.
3 MED register() eviction at capacity dropped a still-running entry's JoinHandle (tokio detaches on drop → orphaned, untracked process). Now evicts the oldest terminal entry preferentially, and aborts the handle if a running entry must be evicted as a last resort.
4 MED The background arg schema description still described the reverted push-once model ("output delivered automatically, do NOT poll, killed at the timeout") — which would stop the model from ever polling bash_output. Rewritten to the poll / unbounded / kill_shell model.
5 LOW Fixed a stale test doc-comment referencing the removed TaskKind::Shell.
6 LOW Documented the Windows process-tree-kill limitation on spawn_streaming_shell (Unix-only PgKillGuard; grandchildren best-effort on Windows).

Verified-clean in review and left as-is: the abort→SIGKILL chain on unix, drain-EOF-while-process-alive, the cap TOCTOU (bash is Sequential), global-store sharing, /tasks listing, and the status-bar call sites. The soft output-buffer cap and capturing trailing output after exit are intentional.

Tests

  • New: eviction aborts (not detaches) an evicted running shell; eviction prefers terminal entries over running ones.
  • Full feature-matrix suite green at -D warnings (2166 passed).

Post-merge review of #236:

- HIGH: spawn_streaming_shell never disarmed its PgKillGuard, so on natural
  (and timeout) exit the guard's Drop fired a second killpg(SIGKILL) against an
  already-reaped, possibly OS-recycled process-group id. Now disarmed before
  finish() on every non-aborted completion path (matching run_with_timeout);
  the guard still fires on the abort/kill path, which is how kill_shell works.

- MED-HIGH: background shells (own process group) leaked past normal exit —
  kill_all() only ran on session swap. Now also called when run_interactive
  returns (/quit, Ctrl+C/D, EOF) and at the headless session-end points.

- MED: register() eviction at capacity dropped a still-running entry's
  JoinHandle (tokio detaches on drop → orphaned untracked process). Now evicts
  the oldest TERMINAL entry preferentially, and aborts the handle if a running
  entry must be evicted as a last resort.

- MED: the  arg schema description still described the reverted
  push-once model ('output delivered automatically, do NOT poll, killed at the
  timeout'). Rewritten to the poll-bash_output / unbounded / kill_shell model.

- LOW: fixed a stale test doc-comment referencing the removed TaskKind::Shell;
  documented the Windows process-tree-kill limitation on spawn_streaming_shell.

Tests: eviction aborts (not detaches) an evicted running shell; eviction
prefers terminal entries. Full feature-matrix suite green at -D warnings
(2166 passed).
@yogthos
yogthos merged commit 20850da into main May 30, 2026
9 checks passed
@yogthos
yogthos deleted the fix/background-shells-review-followups branch May 30, 2026 03:20
allen-munsch pushed a commit to allen-munsch/dirge that referenced this pull request Jun 3, 2026
…ls-review-followups

fix(bash): address code-review findings on background shells
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