Stop depending on GNU coreutils that macOS does not ship#2416
Stop depending on GNU coreutils that macOS does not ship#2416weishi-imbue wants to merge 12 commits into
Conversation
mngr shelled out to `timeout`, `tac`, `du -sb` and `stat -c %Y` in code that runs on the agent's host. For the `local` provider that host is the user's machine, and stock macOS ships a BSD userland with none of them. Each failure was reproduced on macOS 26.4.1 under a stock PATH before being fixed, and the fix re-verified against the same harness. `timeout` (agents/tui_utils.py): exits 127, so `mngr message` to a local codex or antigravity agent always raised SendMessageError, instantly, while claiming it had waited the full timeout. Claude agents degraded silently instead, since the 127 was swallowed inside a backgrounded subshell. Replacing `timeout(1)` needs more than sleep-and-kill. Measured on macOS: a `tmux wait-for` client exits 0 whether signalled or killed, so the exit status cannot distinguish them -- the watchdog therefore marks a file before killing, and that marker decides the exit code, the way timeout returns 124. The waiter must be the tmux client itself; killing a wrapper subshell leaves the client running forever. And background jobs must redirect stdout, since a job that inherits it holds the caller's stdout open until it exits, stalling every submission for the full deadline. `tac` (resources/mngr_transcript_lib.sh): the reverse scan read nothing, so the offset reset to 0 and the transcript re-emitted lines. Scanning forward and keeping the last match needs no reverse. This also drops a latent off-by-one: `wc -l` ignores an unterminated final line while `tac` emits it, so a live transcript being appended to reconciled one line low. `du -sb` / `stat -c %Y` (api/gc.py): both rejected by BSD. `du -sb ... | cut` made the pipeline exit status `cut`'s, so failure looked like success with empty output and every orphaned work dir was reported as 0 bytes; the `stat` failure made each orphan's created_at fall back to now(), so age-based collection never ran. Both sites now call the host interface, which gains a concrete `get_directory_size` beside `path_exists` and `get_file_mtime`. Regression tests poison `tac`/`timeout` on PATH, so a reintroduced dependency fails on Linux too rather than only on a user's mac. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tui_agent_test asserted `"mktemp" not in command` for the signal-only path as a proxy for "uses no sentinel file". The watchdog legitimately needs one: a `tmux wait-for` client exits 0 whether signalled or killed, so a marker file, not the wait status, must decide the exit code. Assert on the absence of a polling loop instead, which is what actually distinguishes the two paths. Remove the per-line subshell fork from both `mngr_transcript_reconcile_offset` and `mngr_transcript_build_id_set`. Each extracted the correlation field via a command substitution, forking once per line. Matching the bash regex inline forks nothing. Reconciling a 50k-line session file: 29.9s -> 1.5s. Without this the forward scan would have been a regression, since `tac` let the old reverse scan stop at the first match from the end -- measured 5.6s -> 11.0s at 10k lines before this change, and 5.4s -> 0.3s after. `get_directory_size` walked with `Path.rglob` + `lstat`, 5x slower than the `du -sb` it replaced (0.600s vs 0.124s over 38k entries). `os.walk` closes that to 1.5x (0.193s) for identical bytes. Also guard the remote branch with `test -d`, so a non-directory reports 0 on both sides rather than only locally. Add two real-tmux regression tests that pin the impact rather than infer it. Against the pre-fix tui_utils.py on macOS both fail with `SendMessageError: Timeout waiting for message submission signal`, though the hook fired: the signal-only path (what codex and antigravity call) and the marker path with a frozen marker (the shape of Claude's /clear and /compact, which fire the hook but never enqueue a model turn). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The watchdog dropped the waiter's exit status entirely, because a TERM'd `tmux wait-for` exits 0 and so cannot distinguish "signalled" from "killed". But that status is only meaningless when the watchdog did the killing. When `tmux wait-for` exits on its own it is meaningful, and non-zero means it failed outright -- no server, dead session, bad channel. Discarding it made both builders exit 0 in ~15ms against a dead tmux server, so `send_enter_via_tmux_wait_for_hook` reported a message as submitted when nothing had been. `timeout(1)` never had this problem: it forwards the child's status and only overrides it on expiry. The deadline marker now decides expiry and the wait status decides outright failure. In the marker variant, a waiter that dies without confirming no longer ends the poll -- the acceptance marker can still confirm, so the loop keeps watching it until the deadline, matching the previous behavior. Regression tests drive both built commands against a `tmux` shim that exits 1 and assert a non-zero result; both fail against the previous commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Guard against mktemp failure in the submission watchdog: an empty $tmo made the `[ ! -s "$tmo" ]` expiry check unconditionally true, so every timeout would have reported success. Also kill the Enter-sending subshell on exit. Make get_directory_size agree across its two branches. The remote branch tolerated no `du` exit status: `du` prints a correct total and exits 1 after skipping an unreadable subdirectory, and the previous `| cut -f1` pipeline masked that, so dropping the pipe would have reported 0 bytes for such a tree. It also needs a trailing slash to descend a symlink to a directory. The local branch summed apparent bytes while the remote summed disk blocks, which diverge by up to 4096x on a small-file tree; it now sums st_blocks and dedupes by inode, matching `du`. Walking with os.walk rather than Path.rglob makes it ~3x faster. Delete mngr_transcript_extract_field (no callers repo-wide) and _mngr_transcript_set_field_pattern (assigned a global, breaking the header's stated purity contract); the field ERE is now a module constant. Fix two stale claims in the send_enter_via_tmux_wait_for_hook docstring: the waiter no longer runs in the foreground, and a tmux wait-for signal with no registered waiter is not lost -- it latches.
…ents Add two @pytest.mark.tmux tests that run the exact script _build_signal_only_command emits against the test's isolated tmux server: it exits 0 once the hook fires the wait-for channel, and non-zero when the sleep-then-kill deadline passes with no hook. These assert on the generated script directly, complementing the higher-level send_enter_via_tmux_wait_for_hook tests. Reword the watchdog docstrings to lead with the portability constraint (the code runs on the agent's host, Linux or macOS) rather than singling out macOS, and tighten the marker-variant comment.
The local branch was a 15-line os.walk reimplementation of du (st_blocks*512, inode dedup, followlinks handling) that existed only to match the remote du. execute_idempotent_command runs the same shell path locally and remotely, so the whole is_local branch, the seen_inodes set, and _STAT_BLOCK_SIZE_BYTES are gone -- one du -sk call for both. This follows the same rule as the timeout fix: call the tool, do not reimplement it. Verified the exact `test -d P/ && du -sk P/` invocation on stock macOS (BSD du), Linux GNU du, and Linux busybox du: all agree that a hard-linked inode is charged once, a nested symlinked directory is not followed, a top-level symlink is resolved via the trailing slash, and a non-directory reports nothing. The old comment claimed the trailing slash "descends a symlinked directory, as os.walk does", which overstated it -- the slash resolves the top-level path only, coinciding with os.walk's top-arg rule by a different mechanism. Add the first tests for get_directory_size (it had none): a real local host over a tree with a hardlink and a nested symlink, plus the non-directory case.
How execute_idempotent_command reaches the host (subprocess vs SSH) is the transport layer's concern, not this method's, and the is_local mention was a leftover from the branch that no longer exists. Keep only the du contract that affects the returned number: KiB rounding, symlink resolution, and tolerating du's non-zero exit on an unreadable entry.
The field ERE interpolates FIELD literally via printf %s, so a field name with regex metacharacters would change the pattern; note that callers must pass a regex-safe field (uuid/id do). The reconcile loop's `while read` skips an unterminated final line, which is deliberate: it matches wc -l and the sed emit range, so a partially-written final record is deferred rather than miscounted. Note it so it is not "fixed" with a trailing-line catch that would re-break that consistency.
Benchmarked old vs new (3 trials each, macOS and Linux) instead of the single number the changelog carried. The real, scenario-independent win is build_id_set's fork elimination: ~32s->0.7s on macOS, ~14s->0.6s on Linux at 50k lines. reconcile_offset is subtler -- dropping tac also dropped its early-exit, so the new forward scan always reads the whole file (~0.7s), a small loss in the best case but a large win over the old reverse scan's ~12s worst case. The old "30s -> 1.5s, reconciling" line conflated build_id_set's cost with reconcile and compared old-build-only against new-build-plus-reconcile.
| return ( | ||
| f"bash -c '" | ||
| f'( sleep 0.1 && tmux send-keys -t "$1" Enter ) & ' | ||
| f'timeout {full_timeout} tmux wait-for "$0"' |
There was a problem hiding this comment.
idk, maybe we could use this alternative suggested by claude instead?
Perl one-liner (no install, works everywhere)
perl -e 'alarm shift; exec @ARGV' 10 ./slow-thingThis execs the command, so it replaces the perl process — signals and exit codes behave sensibly. It's the closest drop-in.
| # Clean up on every exit path: remove the sentinel file and reap any | ||
| # still-running background job (notably the hook waiter, which otherwise | ||
| # outlives a fast marker-win and would recreate "$sig" -- leaking the | ||
| # temp file -- when the hook finally fires). Runs exactly once on exit. | ||
| 'trap \'p="$(jobs -p)"; [ -n "$p" ] && kill $p 2>/dev/null; rm -f "$sig"\' EXIT; ' | ||
| 'tmo="$(mktemp)" || exit 1; ' | ||
| # Any of the three background jobs can outlive a fast marker-win. | ||
| 'trap \'kill "$waiter" "$watchdog" "$submit" 2>/dev/null; rm -f "$tmo"\' EXIT; ' | ||
| f'base="$({accept_marker_command})"; ' | ||
| # Register the hook waiter first (full timeout), sentinel on success. | ||
| f'( timeout {full_timeout} tmux wait-for "$1" >/dev/null 2>&1 && echo 1 > "$sig" ) & ' | ||
| # Register the hook waiter first, bounded by the watchdog. | ||
| 'tmux wait-for "$1" & waiter=$!; ' | ||
| f'( sleep {full_timeout}; echo 1 > "$tmo"; kill "$waiter" ) >/dev/null 2>&1 & watchdog=$!; ' | ||
| # Then submit, after a beat so the waiter is registered. | ||
| '( sleep 0.1 && tmux send-keys -t "$2" Enter ) & ' | ||
| '( sleep 0.1 && tmux send-keys -t "$2" Enter ) >/dev/null 2>&1 & submit=$!; ' | ||
| f'end="$(( $(date +%s) + {int(full_timeout) + 1} ))"; ' | ||
| "hook_pending=1; " | ||
| 'while [ "$(date +%s)" -lt "$end" ]; do ' | ||
| 'if [ -s "$sig" ]; then exit 0; fi; ' | ||
| 'if [ "$hook_pending" -eq 1 ] && ! kill -0 "$waiter" 2>/dev/null; then ' | ||
| 'wait "$waiter" 2>/dev/null && [ ! -s "$tmo" ] && exit 0; ' | ||
| "hook_pending=0; fi; " | ||
| f'cur="$({accept_marker_command})"; ' |
There was a problem hiding this comment.
I'd want a similar change here instead (the above to replace timeout, but only on osx)
these other changes are changes to very delicate code that doesn't seem worth messing with right now
|
lgtm except the timeout changes -- those are too complex for me to verify, and I'm very skeptical that they are still correct and it seems like we should be able to easily just replace with the other alternative if timeout is unavailable? PS: did you make issues for the "Filed separately (pre-existing, not macOS-specific)" issues? |
What
mngr shells out to
timeout,tac,du -sbandstat -c %Yfrom code that runs on the agent's host — which, under thelocalprovider, is the user's machine, where stock macOS has none of them. Each failure was reproduced on macOS 26.4.1 under a stockPATH, then re-verified fixed.timeouttui_utils.pymngr messageto a local codex/antigravity agent always raisedSendMessageErrorinstantly, claiming a full-timeout waittimeouttui_utils.py(marker)/clear+/compactburned the full timeouttacmngr_transcript_lib.shdu -sbgc.py×2| cut -f1masked the error exit → orphan dir reported as 0 bytesstat -c %Ygc.py×2created_atfell back tonow()→ age-based GC never ran on macOSRule applied throughout: reimplement a tool only when it isn't a shell one-liner; otherwise call the tool. So
timeoutbecomes a watchdog, buttacis deleted,du -sb→POSIXdu -sk, andstat -c %Yreuses an existing portable helper. (readlink -fandxargs -rwere checked — both fine on macOS, no change.)timeout→ sleep-and-kill watchdogNot as simple as it sounds; three measured facts:
tmux wait-forclient exits 0 whether signalled or killed, sowait's status can't tell success from timeout. The watchdog marks a file before killing; that marker decides the exit code (liketimeout's 124).$waitermust be the tmux client itself — killing a wrapper subshell orphans the client forever.Two
@pytest.mark.tmuxtests drive the generated script directly (hook → 0, deadline → non-zero).tac→ forward scan + fork eliminationForward scan keeping the last match is the same
O(n), needs no reverse, and fixes a latent off-by-one (old pairedwc -l, which drops an unterminated final line, withtac, which keeps it).The real speedup was orthogonal: both scanners pulled each field through
value=$(…), forking a subshell per line; they now match inline with[[ =~ ]](a builtin — verified on macOS bash 3.2 and Linux 5.2 — so no external binary at all).build_id_set, which must read the whole output, at 50k lines:~45×/~22× (3 trials each). This is where essentially all the win is, and it needs no early exit.
Why not stream backward?
reconcile_offsetgave uptac's early exit (now a constant ~0.72s@50k). It runs once per streamer startup, and in steady-state recovery the cut-off sits near the end of the file — so backward would early-exit (measured 0.72s→0.009s). Still not done:mapfile+ backwardtac/tail -rtacGNU,tail -rBSD) — the exact anti-pattern this PR removesWorth it only on a hot path. A cut-off-near-end distribution is necessary but not sufficient; a once-per-startup cold scan doesn't justify O(n) memory or an OS branch. If it ever goes per-poll,
mapfile(nevertail -r) is the move.gc.py→ the host interfacegc.pynow callsget_file_mtime(already portable) and a newget_directory_size— onedu -skcall, nois_localbranch (execute_idempotent_commandalready dispatches local/remote). An earlier draft reimplemented the local case as a 15-lineos.walk; that was itself adureimplementation and is gone. Thetest -d P/ && du -sk P/invocation was verified on macOS BSD, Linux GNU, and busyboxduto agree on hardlinks (counted once), nested symlinks (not followed), top-level symlink (resolved via the slash), and non-dirs (0).du's non-zero exit after an unreadable subdir is tolerated (it still prints the total).Tests
tac/timeoutshims that exit 127 onPATH, so a reintroduced dep fails on Linux CI too.get_directory_sizehad zero tests; added a real-local-host tree (hardlink + nested symlink) and the non-dir case.Still to land
Filed separately (pre-existing, not macOS-specific)
tmux wait-forsignals latch, contradicting the old docstring ("a signal with none registered is lost"). A late hook signal on the stablemngr-submit-<session>channel can make the next submit falsely report success. Docstring fixed; behavior not.wait-forclient whose server dies mid-wait exits 0, so a server death after registration but before the deadline reads as a successful send.timeout(1)passed the same 0 through — unchanged here.🤖 Generated with Claude Code