Skip to content

fix(despawn): handle OS-terminal placements - #548

Open
w3lld1 wants to merge 2 commits into
fujibee:mainfrom
w3lld1:fix/os-terminal-despawn
Open

fix(despawn): handle OS-terminal placements#548
w3lld1 wants to merge 2 commits into
fujibee:mainfrom
w3lld1:fix/os-terminal-despawn

Conversation

@w3lld1

@w3lld1 w3lld1 commented Jul 29, 2026

Copy link
Copy Markdown

Summary

  • record the boot-shell PID for members launched through an OS terminal
  • terminate the recorded process tree during graceful or forced despawn and consistently remove the member registration
  • keep tmux and herdr placement handling unchanged

Validation

  • bats --print-output-on-failure --filter 'OS-terminal boot records|kills recorded OS-terminal PID|terminates OS-terminal descendants|graceful OS-terminal member|errors when there is no placement record|kills a herdr: placement|kills recorded placement and drops registration|graceful no-op when' tests/test_spawn.bats tests/test_despawn.bats (9 tests)
  • bash -n scripts/spawn.sh scripts/despawn.sh
  • git diff --check upstream/main...HEAD

The complete Bats suite is deferred to CI because it is intentionally sharded across Linux and macOS and takes substantially longer than the focused local validation.

Fixes #228

@w3lld1

w3lld1 commented Jul 29, 2026

Copy link
Copy Markdown
Author

Fixed the failing cross-platform checks in 6b920ff: the empty boot-prompt assertion now inspects only the generated launch line, and the descendant-order test no longer relies on macOS-missing mapfile. Both focused Bats cases pass, along with git diff --check.

@fujibee

fujibee commented Jul 31, 2026

Copy link
Copy Markdown
Owner

@w3lld1

Thanks for this — the gap is real and worth closing. A codex member started in an OS terminal has no watcher and no actas lock, so graceful despawn had nothing to act on and the member stayed up. Recording the boot PID as a placement and teaching both teardown paths to use it is the right shape, and the five tests cover the paths rather than the happy line.

We want this, and we would like to take it in the next cycle rather than this release. Review turned up three things that all sit on the same surface, so it seemed better to raise them together than to land and follow up.

The recorded PID is not checked for still being ours. kill_pid_tree validates that the value is numeric and then signals it and every descendant. A placement is written at spawn and read at despawn with an unbounded gap between, and the boot process can exit first, so by then the number may belong to something else entirely — and the recursion means an unrelated process tree goes with it. delivery.sh has the shape we use elsewhere for this: before killing a recorded pid it checks the command line still looks like ours, explicitly against recycling. Something equivalent is needed here. A stronger version records an identity at spawn time — a start token or generation — and requires it to match before signalling, since a command line can also be reused.

The numeric check itself is worth widening while you are there: it currently accepts 0, 00, leading zeros, and values above the platform's pid ceiling. kill 0 is the case that matters — it signals the caller's own process group rather than a single process. We hit this class in #505 and ended up bounding pid input before use; _agmsg_pid_valid in scripts/lib/instance-id.sh is already sourced in this path and does that part.

Teardown reports success without establishing it. kill_pid_tree sends TERM, waits for nothing, and swallows every failure; the caller then resets the registration, releases the lock, removes the spawn record, and prints status=ok. A CLI that ignores TERM survives with its ownership information deleted — which is worse than not having tried, because the next despawn has nothing left to act on. What we would want is TERM, a bounded wait, KILL if needed, a bounded confirmation, and removal of the registration and record only once the process is actually gone. If it cannot be confirmed, failing closed with a non-zero status and leaving the record intact is more useful than a false ok.

One test depends on the host's login shell. spawn: OS-terminal boot records its PID for despawn runs the generated boot script, which ends in exec "${SHELL:-/bin/bash}" -i. With SHELL=/bin/bash it passes; with SHELL=/bin/zsh it fails — and zsh is the macOS default, so it fails on a normal developer machine while CI stays green. Setting SHELL explicitly for that test, or handing it a fixture that exits immediately, makes it deterministic. Worth fixing here because it is the same surface the changes above need to be verified on.

None of this is a disagreement with the approach. If you would rather we took the safety changes ourselves and landed it with your commits, say so and we will — otherwise we will pick it up as soon as an updated branch is ready.

@w3lld1

w3lld1 commented Aug 1, 2026

Copy link
Copy Markdown
Author

Thanks for the detailed safety review. Please go ahead and take the PID-identity, bounded teardown, and deterministic-shell changes on this branch as you offered; preserving the existing commits is fine with me.

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.

despawn is ineffective for non-tmux (OS-terminal) spawns

2 participants