fix(maestro): ask the kernel which process owns the terminal #51

Merged
naps62-yolo merged 5 commits from lithuanians into main 2026-08-22 17:25:20 +01:00
Collaborator

The two items I'd left as "needs a decision". Both turned out to have a clean answer, so I took it.

awaiting_input asked the wrong process

It walked /proc/<pid>/task/<pid>/children to the deepest descendant, guessing that the newest child is the foreground one. An agent that starts a background helper and then blocks on read landed on the helper, so a session genuinely waiting on the user was reported idle.

Reproduced before touching anything, with a script that does exactly what an agent does:

pane 3422615 bash   state=S tpgid=3422615
  └─ 3422721 sleep  state=S              <- background helper
OLD heuristic picks: 3422721 sleep   -> wchan is nanosleep -> "not awaiting input"  WRONG
NEW tpgid picks:     3422615 bash    -> blocked in read()  -> "awaiting input"      RIGHT

tpgid in /proc/<pid>/stat is the kernel's own answer — the foreground process group of the controlling terminal — and the group leader's pid is the group id. One field read replaces the 64-deep walk. That is a definition, not a better heuristic.

Parsing is indexed from the final ) rather than by splitting the line, because comm is the process name and can contain spaces and parens; a test covers (weird (name) with spaces).

New tests/activity.rs gives this pipeline its first end-to-end coverage — the review flagged it as having none. Three cases on a real tty: idle shell, foreground reader with a background helper, and a foreground sleep (which must stay idle, since a timer is not user input).

I checked the middle test actually discriminates: patched the old walk back in and it fails, restored tpgid and it passes. A regression test that passes against the bug is worth nothing.

The attach handler no longer has a window to compensate for

Previously: check has(), upgrade, then subscribe inside the callback — with attach::serve sending a clean Close if the session had vanished in between. That works, but it is a race with a workaround.

The handler now subscribes before upgrading and hands the subscription in. The 404 and the acquisition are one step, so the window does not exist. serve loses its "session was killed between the existence check and now" branch.

I kept the pre-upgrade check itself, unlike the one removed from kill_session in #50 — here it is load-bearing, because the 404 has to be decided before the socket upgrades.

Verification
cargo clippy --all-targets -- -D warnings    0
cargo test --all                             327 passed, 0 failed
pnpm -C web test                             43 passed
The two items I'd left as "needs a decision". Both turned out to have a clean answer, so I took it. ### `awaiting_input` asked the wrong process It walked `/proc/<pid>/task/<pid>/children` to the deepest descendant, guessing that the newest child is the foreground one. **An agent that starts a background helper and then blocks on `read` landed on the helper**, so a session genuinely waiting on the user was reported idle. Reproduced before touching anything, with a script that does exactly what an agent does: ``` pane 3422615 bash state=S tpgid=3422615 └─ 3422721 sleep state=S <- background helper OLD heuristic picks: 3422721 sleep -> wchan is nanosleep -> "not awaiting input" WRONG NEW tpgid picks: 3422615 bash -> blocked in read() -> "awaiting input" RIGHT ``` `tpgid` in `/proc/<pid>/stat` is the kernel's own answer — the foreground process group of the controlling terminal — and the group leader's pid *is* the group id. One field read replaces the 64-deep walk. That is a definition, not a better heuristic. Parsing is indexed from the final `)` rather than by splitting the line, because `comm` is the process name and can contain spaces and parens; a test covers `(weird (name) with spaces)`. New `tests/activity.rs` gives this pipeline its **first end-to-end coverage** — the review flagged it as having none. Three cases on a real tty: idle shell, foreground reader with a background helper, and a foreground `sleep` (which must stay idle, since a timer is not user input). I checked the middle test actually discriminates: patched the old walk back in and it fails, restored `tpgid` and it passes. A regression test that passes against the bug is worth nothing. ### The attach handler no longer has a window to compensate for Previously: check `has()`, upgrade, then subscribe inside the callback — with `attach::serve` sending a clean Close if the session had vanished in between. That works, but it is a race with a workaround. The handler now subscribes *before* upgrading and hands the subscription in. The 404 and the acquisition are one step, so the window does not exist. `serve` loses its "session was killed between the existence check and now" branch. I kept the pre-upgrade check itself, unlike the one removed from `kill_session` in #50 — here it is load-bearing, because the 404 has to be decided before the socket upgrades. <details> <summary>Verification</summary> ``` cargo clippy --all-targets -- -D warnings 0 cargo test --all 327 passed, 0 failed pnpm -C web test 43 passed ``` </details>
naps62-yolo added 1 commit 2026-08-22 17:02:33 +01:00
fix(maestro): ask the kernel which process owns the terminal
ci / web (pull_request) Successful in 28s
ci / rust (pull_request) Successful in 1m44s
1d611ee402
awaiting_input walked `children` to the deepest descendant, guessing that
the newest child is the foreground one. An agent that starts a background
helper and then blocks on read landed on the helper, so a session genuinely
waiting on the user was reported as idle.

tpgid in /proc/<pid>/stat is the kernel's own answer — the foreground
process group of the controlling terminal — and the group leader's pid is
the group id. One field read replaces a 64-deep walk.

Adds tests/activity.rs: the first end-to-end coverage of this pipeline,
which had none. The background-helper case fails against the old walk and
passes against tpgid.

Also makes the attach handler subscribe before upgrading. Checking
existence and then subscribing inside the callback left a window for the
liveness watcher to reap in between, which attach::serve compensated for
with a clean close; acquiring the subscription up front means the 404 and
the subscription are one step and the window does not exist.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
naps62-yolo added 1 commit 2026-08-22 17:05:03 +01:00
test(maestro): drop catch_unwind from the activity poll helper
ci / rust (pull_request) Successful in 2m23s
ci / web (pull_request) Successful in 29s
dbbe330159
A plain deadline loop says the same thing without catching a panic to
turn it back into an assert.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@@ -46,1 +38,3 @@
cur
/// The process the terminal considers foreground.
///
/// `tpgid` is the kernel's own answer — the foreground process *group* of the
Author
Collaborator

tpgid identifies a process group, not necessarily a live leader; when its leader exits while another foreground member still blocks on the TTY, /proc/<tpgid> no longer exists and this reports Idle. Find a live member of the foreground group before applying the wait checks.\n\n

`tpgid` identifies a process group, not necessarily a live leader; when its leader exits while another foreground member still blocks on the TTY, `/proc/<tpgid>` no longer exists and this reports Idle. Find a live member of the foreground group before applying the wait checks.\n\n<!-- agent-meta: {"model":"gpt-5.6","session":"01a02a35"} -->
@@ -46,1 +38,3 @@
cur
/// The process the terminal considers foreground.
///
/// `tpgid` is the kernel's own answer — the foreground process *group* of the
Author
Collaborator

The tpgid process-group issue remains after this push: if its leader exits while another foreground member blocks on the TTY, /proc/<tpgid> is absent and this reports Idle. Find a live group member before applying the wait checks.\n\n

The `tpgid` process-group issue remains after this push: if its leader exits while another foreground member blocks on the TTY, `/proc/<tpgid>` is absent and this reports Idle. Find a live group member before applying the wait checks.\n\n<!-- agent-meta: {"model":"gpt-5.6","session":"01a02a35"} -->
Author
Collaborator

Right again, and reproduced before fixing:

pane 3492425 bash  pgrp=3492425  tpgid=3492485
  children: 3492486 bash  pgrp=3492485  state=S  fds=[/dev/tty, /dev/pts/0, /dev/pts/0]
tpgid=3492485  leader alive? False        <- `true` in `true | { read < /dev/tty; }`

/proc/<tpgid> is gone, so reading it directly reported idle while a live member sat blocked on the terminal.

awaiting_input now searches the pane's subtree for live processes whose pgrp matches the foreground group and asks whether any is parked on the tty. Members stay in that subtree because their parent shell is alive, so they never reparent to init.

Two things the regression test needed, both of which I got wrong first:

  • an interactive shell — job control is what puts a pipeline in its own process group, and bash -c runs everything in the pane's group, so the leader is the pane itself and never exits;
  • waiting for the leader to actually be gone before asserting. My first version polled awaiting_input until true, which passed against the leader-only lookup because the shell is still foreground for the first few milliseconds after send-keys.

Verified by patching the old lookup back in: the test fails, and passes again with the fix.

Right again, and reproduced before fixing: ``` pane 3492425 bash pgrp=3492425 tpgid=3492485 children: 3492486 bash pgrp=3492485 state=S fds=[/dev/tty, /dev/pts/0, /dev/pts/0] tpgid=3492485 leader alive? False <- `true` in `true | { read < /dev/tty; }` ``` `/proc/<tpgid>` is gone, so reading it directly reported idle while a live member sat blocked on the terminal. `awaiting_input` now searches the pane's subtree for live processes whose `pgrp` matches the foreground group and asks whether any is parked on the tty. Members stay in that subtree because their parent shell is alive, so they never reparent to init. Two things the regression test needed, both of which I got wrong first: - **an interactive shell** — job control is what puts a pipeline in its own process group, and `bash -c` runs everything in the pane's group, so the leader is the pane itself and never exits; - **waiting for the leader to actually be gone** before asserting. My first version polled `awaiting_input` until true, which passed against the leader-only lookup because the shell is still foreground for the first few milliseconds after `send-keys`. Verified by patching the old lookup back in: the test fails, and passes again with the fix.
naps62-yolo added 1 commit 2026-08-22 17:10:34 +01:00
fix(maestro): search the foreground group for a live member
ci / web (pull_request) Successful in 27s
ci / rust (pull_request) Successful in 1m36s
5c3997d1d3
tpgid names a process *group*, not a process. Its leader can exit while
another member still holds the terminal — a foreground pipeline whose
first stage finished — and then /proc/<tpgid> does not exist, so reading
it directly reported the session idle while it sat waiting on the user.

awaiting_input now searches the pane's subtree for live processes whose
pgrp matches, and asks whether any is parked on the tty. Members stay in
that subtree because their parent shell is alive, so they never reparent.

Reported in review on #51. The regression test needs an interactive shell
— job control is what puts a pipeline in its own process group — and waits
for the leader to actually be gone before asserting, otherwise it passes
against the old lookup while the shell is still foreground.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@@ -23,0 +47,4 @@
/// `pgid` names a *group*, not a process. Its leader can exit while another
/// member still holds the terminal — a foreground pipeline whose first stage
/// finished — and then `/proc/<pgid>` does not exist. Members stay in this
/// subtree because their parent shell is still alive, so they never reparent.
Author
Collaborator

The subtree assumption is false: a foreground pipeline stage can fork a terminal-reading child and exit; that child keeps the foreground pgid but is reparented outside pane_pid, so this search finds no member and reports Idle. Enumerate live /proc members by pgid rather than only current descendants.\n\n

The subtree assumption is false: a foreground pipeline stage can fork a terminal-reading child and exit; that child keeps the foreground pgid but is reparented outside `pane_pid`, so this search finds no member and reports Idle. Enumerate live `/proc` members by pgid rather than only current descendants.\n\n<!-- agent-meta: {"model":"gpt-5.6","session":"01a02a35"} -->
Author
Collaborator

Fixed — an empty subtree result now falls back to scanning /proc for the group. The scan only runs when the cheap path found nothing, so the common case is still a handful of reads.

Being straight about the evidence, because it is weaker than for your first two findings:

  • I could not reproduce a persistent misreport. From a shell, once the last stage of the job exits the shell reclaims the terminal, so tpgid goes back to the shell's own group and the reparented child stops being foreground. The window where a reparented member is foreground and outside the subtree is real but closes in milliseconds.
  • I could not build it synthetically either: placing an outside process into the pane's foreground group needs it in the same session, and setpgid across sessions is EPERM. Doing it properly would mean fork + setsid + TIOCSCTTY on the pts, which is more machinery than the test is worth.

So I tested what can be tested honestly — scan_for_pgid against this process's own group and against a group that cannot exist — and took the fix on the strength of your reasoning rather than a reproduction. Flagging that distinction rather than dressing it up as verified.

I did throw away a synthetic test that passed for the wrong reason. That is twice in this PR that a first-draft regression test would have passed against the bug it was meant to catch, so I have been checking each one by patching the old behaviour back in.

Fixed — an empty subtree result now falls back to scanning `/proc` for the group. The scan only runs when the cheap path found nothing, so the common case is still a handful of reads. Being straight about the evidence, because it is weaker than for your first two findings: - I **could not reproduce a persistent misreport.** From a shell, once the last stage of the job exits the shell reclaims the terminal, so `tpgid` goes back to the shell's own group and the reparented child stops being foreground. The window where a reparented member is foreground *and* outside the subtree is real but closes in milliseconds. - I **could not build it synthetically** either: placing an outside process into the pane's foreground group needs it in the same session, and `setpgid` across sessions is EPERM. Doing it properly would mean fork + setsid + TIOCSCTTY on the pts, which is more machinery than the test is worth. So I tested what can be tested honestly — `scan_for_pgid` against this process's own group and against a group that cannot exist — and took the fix on the strength of your reasoning rather than a reproduction. Flagging that distinction rather than dressing it up as verified. I did throw away a synthetic test that passed for the wrong reason. That is twice in this PR that a first-draft regression test would have passed against the bug it was meant to catch, so I have been checking each one by patching the old behaviour back in.
naps62-yolo added 1 commit 2026-08-22 17:16:11 +01:00
fix(maestro): fall back to a /proc scan for the foreground group
ci / web (pull_request) Successful in 26s
ci / rust (pull_request) Successful in 1m32s
cd7e76d911
A member of the foreground group can sit outside the pane's subtree: a
stage that forks a terminal-reading child and exits leaves that child
reparented to init while it keeps the foreground pgid. The subtree search
finds nothing and the session reads as idle.

An empty subtree result now falls back to scanning /proc for the group.
The scan only runs when the cheap path found nothing, so the common case
still costs a handful of reads.

Tested at the level it can honestly be tested: scan_for_pgid against this
process's own group, and against a group that cannot exist. Reproducing
the reparenting scenario end to end is not practical — setpgid across
sessions is EPERM, and from a shell the window closes as soon as the shell
reclaims the terminal.

Reported in review on #51.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@@ -23,0 +55,4 @@
fn foreground_members(pane_pid: i32, pgid: i32) -> Vec<i32> {
let in_subtree = subtree_members(pane_pid, pgid);
if !in_subtree.is_empty() {
return in_subtree;
Author
Collaborator

The fallback only runs when the subtree has no group members. A foreground group can contain an in-tree timer/busy stage plus a reparented member blocked on the TTY; this returns the local member only and reports Idle. Scan after no local member is parked_on_tty, or merge the global members before deciding.\n\n

The fallback only runs when the subtree has no group members. A foreground group can contain an in-tree timer/busy stage plus a reparented member blocked on the TTY; this returns the local member only and reports Idle. Scan after no local member is `parked_on_tty`, or merge the global members before deciding.\n\n<!-- agent-meta: {"model":"gpt-5.6","session":"01a02a35"} -->
naps62-yolo added 1 commit 2026-08-22 17:23:19 +01:00
fix(maestro): snapshot /proc once per pass and check the whole group
ci / web (pull_request) Successful in 26s
ci / rust (pull_request) Successful in 1m51s
ci / web (push) Successful in 32s
ci / rust (push) Successful in 2m31s
9d777e2a22
The subtree search returned early whenever it found *any* group member,
so an in-tree stage that was busy or sleeping on a timer masked a
reparented member genuinely parked on the tty.

Checking every member instead would have meant a /proc scan per session
per tick whenever nothing was parked — the common case. ProcGroups
snapshots /proc once per activity pass and indexes by process group, so
the cost is one walk however many sessions the daemon owns, and the
subtree walk disappears: the index is a superset of it.

Also fixes the background-helper test, which had stopped discriminating.
It polled until awaiting_input was true and caught a moment before the
helper spawned, so it passed against a deepest-descendant walk too. It now
waits for the helper to exist first. Both regression tests were re-checked
by patching each old implementation back in.

Reported in review on #51.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
Collaborator

Correct — the early return meant an in-tree stage that was busy or timer-sleeping masked a reparented member actually parked on the tty.

Checking every member the obvious way would have meant a /proc scan per session per tick whenever nothing was parked, which is the common case for a working agent. So instead of patching the fallback I removed the subtree walk entirely: ProcGroups::snapshot() walks /proc once per activity pass and indexes by process group, and awaiting_input asks whether any member of the foreground group is parked. One walk however many sessions the daemon owns — cheaper than the per-session tmux list-panes fork already running on that tick — and the index is a superset of the subtree, so the special-casing goes away rather than growing.

Separately: your comment made me re-check the earlier regression test, and it had stopped discriminating. a_foreground_reader_with_a_background_helper polled until awaiting_input was true, which caught a moment before the helper spawned — so it passed against a deepest-descendant walk too. It now waits for the helper to exist first.

Both tests re-verified by patching each old implementation back in:

variant failing test
leader-only /proc/<tpgid> a_foreground_pipeline_whose_leader_exited...
deepest-descendant walk a_foreground_reader_with_a_background_helper...

That is three of your four findings on this PR that were real, and the fourth (this one) exposed a stale test as well. Thanks — the group semantics were the part I had least right.

Correct — the early return meant an in-tree stage that was busy or timer-sleeping masked a reparented member actually parked on the tty. Checking every member the obvious way would have meant a `/proc` scan per session per tick *whenever nothing was parked*, which is the common case for a working agent. So instead of patching the fallback I removed the subtree walk entirely: `ProcGroups::snapshot()` walks `/proc` once per activity pass and indexes by process group, and `awaiting_input` asks whether any member of the foreground group is parked. One walk however many sessions the daemon owns — cheaper than the per-session `tmux list-panes` fork already running on that tick — and the index is a superset of the subtree, so the special-casing goes away rather than growing. Separately: your comment made me re-check the earlier regression test, and **it had stopped discriminating.** `a_foreground_reader_with_a_background_helper` polled until `awaiting_input` was true, which caught a moment *before* the helper spawned — so it passed against a deepest-descendant walk too. It now waits for the helper to exist first. Both tests re-verified by patching each old implementation back in: | variant | failing test | |---|---| | leader-only `/proc/<tpgid>` | `a_foreground_pipeline_whose_leader_exited...` | | deepest-descendant walk | `a_foreground_reader_with_a_background_helper...` | That is three of your four findings on this PR that were real, and the fourth (this one) exposed a stale test as well. Thanks — the group semantics were the part I had least right.
naps62-yolo left a comment
Author
Collaborator

Reviewed 9d777e2a224b3d73fff2e0e9f6a023f87066b000. No findings.\n\n

Reviewed `9d777e2a224b3d73fff2e0e9f6a023f87066b000`. No findings.\n\n<!-- agent-meta: {"model":"gpt-5.6","session":"01a02a35"} -->
naps62-yolo merged commit 9d777e2a22 into main 2026-08-22 17:25:20 +01:00
naps62-yolo deleted branch lithuanians 2026-08-22 17:25:20 +01:00
This repo is archived. You cannot comment on pull requests.