-
Notifications
You must be signed in to change notification settings - Fork 40
Fix the macOS coreutils portability bugs in transcript streaming and gc #2416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+249
−83
Merged
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
a0d3e19
Stop depending on GNU coreutils that macOS does not ship
weishi-imbue d3a4114
Merge remote-tracking branch 'origin/main' into mngr/timeout-tac
weishi-imbue 9bbf489
Fix CI, remove per-line forks, and verify the impact by execution
weishi-imbue cb5c240
Do not report a failed tmux wait-for as a successful submission
weishi-imbue f73b435
Harden the macOS portability fixes after review
weishi-imbue a9c4541
Merge remote-tracking branch 'origin/main' into mngr/timeout-tac
weishi-imbue e861ac4
Test the signal-only script's exit code and reframe the watchdog comm…
weishi-imbue b3bca1b
Collapse get_directory_size to a single du -sk call
weishi-imbue d35b939
Drop transport details from the get_directory_size docstring
weishi-imbue eb9621d
Document two unstated contracts in mngr_transcript_lib.sh
weishi-imbue c6a99d4
Correct the transcript-streamer perf claim in the changelog
weishi-imbue 15a356f
Merge remote-tracking branch 'origin/main' into mngr/timeout-tac
weishi-imbue 8f56cd9
Bound the tmux waiter with perl's alarm instead of rewriting the subm…
weishi-imbue 56a8cdf
Drop comparative and historical narration from the new comments
weishi-imbue cbf8642
Flatten the timeout shim into a single constant named _timeout
weishi-imbue cfd91f9
Fix the perl-fallback tests, which only passed on a host without timeout
weishi-imbue d604aa5
Merge origin/main into mngr/timeout-tac
weishi-imbue a1f2907
Record the portable-shell technique in the style guide
weishi-imbue File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| Fixed four places where mngr shelled out to GNU-coreutils binaries and flags that stock macOS does not ship, each of which ran on the user's own machine whenever an agent used the `local` provider. All four were verified failing on macOS 26.4.1 with a stock `PATH`, then verified fixed. | ||
|
|
||
| `mngr message` to a local codex or antigravity agent raised `SendMessageError("Timeout waiting for message submission signal")` on every send. The submission path bounded its `tmux wait-for` with `timeout(1)`, which exits 127 (`timeout: command not found`) on macOS -- immediately, while reporting that it had waited the full timeout. It is now bounded by a `sleep`-then-`kill` watchdog. Reproducing `timeout(1)` by hand takes more than `sleep` and `kill`: a `tmux wait-for` client exits 0 whether it was signalled or killed, so the watchdog marks a file *before* killing and that marker, not the wait status, decides the exit code (the way `timeout` returns 124); the waiter must be the tmux client itself rather than a wrapper subshell, or the kill hits the subshell and orphans the client; and every background job redirects stdout, because a job that inherits it holds the caller's stdout open until it exits, which would stall each submission for the full deadline even when the signal lands immediately. For Claude agents (which also watch an acceptance marker) the same bug degraded silently instead: the `timeout` failure was swallowed inside a backgrounded subshell, so the hook path never confirmed and only the transcript marker could -- meaning `/clear` and `/compact`, which fire the hook but never enqueue a model turn, always burned the full timeout. | ||
|
|
||
| Raw-transcript streaming re-emitted already-emitted lines on macOS. `mngr_transcript_reconcile_offset` reverse-scanned the session file with `tac` to find the last emitted line; with `tac` absent the scan read nothing and the offset reset to 0. It now scans forward and tracks the last match, which needs no reverse at all. This also fixes a latent off-by-one: the old code combined `wc -l` (which does not count an unterminated final line) with `tac` (which emits it), so a session file whose last line was still being appended -- the normal state of a live transcript -- reconciled one line too low. | ||
|
|
||
| Transcript-streamer startup also got much faster. Both `mngr_transcript_build_id_set` and `mngr_transcript_reconcile_offset` used to extract each line's correlation field through a command substitution, which forks a subshell per line; they now match it with an inline bash regex and fork nothing. The dominant cost was `build_id_set`, which scans the whole already-emitted output: on a 50,000-line file it drops from about 32s to 0.7s on macOS, and about 14s to 0.6s on Linux (three trials each; forks are cheaper on Linux). `reconcile_offset` is subtler -- removing `tac` also removed its early exit on the first match from the end, so the new forward scan always reads the whole file (about 0.7s at 50,000 lines) instead of sometimes stopping early. But because it no longer forks, its worst case -- a match near the start, which made the old reverse scan fork through the entire file at about 12s -- is now that same bounded 0.7s. Combined startup on a 50,000-line pair is about 1.5s, down from tens of seconds. The inline match uses bash's built-in `[[ =~ ]]`, so unlike the `tac` it replaced it needs no external binary and behaves identically on macOS and Linux. | ||
|
|
||
| `mngr gc` mis-measured orphaned work directories on macOS. `du -sb` has no BSD equivalent (`-b` is rejected), and because the command was piped into `cut`, the pipeline exit status was `cut`'s, so the failure looked like success with empty output and every orphan was reported as 0 bytes. `stat -c %Y` is likewise rejected by BSD `stat`, so each orphan's `created_at` fell back to `datetime.now()` and looked brand-new, meaning age-based collection never ran on macOS. | ||
|
|
||
| Both `gc` call sites now go through the host interface instead of hand-rolled shell. `OuterHostInterface` gains a concrete `get_directory_size`, alongside the existing `path_exists` and `get_file_mtime`: it runs POSIX `test -d ... && du -sk` on the host -- locally via subprocess, remotely over SSH -- so callers never branch on `is_local`. Sizes are whole kibibytes, since `-k` is the only `du` block size POSIX defines, and a path that is not a directory reports 0. The trailing slash resolves a symlinked path to its target directory, and `du` exiting non-zero is tolerated because it still prints a correct total after skipping an unreadable subdirectory (the piped-into-`cut` form this replaced happened to mask that, and dropping the pipe would otherwise have turned a working directory into a reported 0 bytes). The exact `du -sk <dir>/` invocation was verified on stock macOS (BSD `du`) and on Linux (GNU and busybox `du`) to agree on the load-bearing cases: a hard-linked inode counted once, a nested symlinked directory not followed, and a top-level symlink resolved. A single `du` call rather than a Python reimplementation for local hosts follows the same rule as the `timeout` fix above -- call the tool instead of reimplementing it. | ||
|
|
||
| A submission whose `tmux wait-for` fails outright -- no tmux server, a dead session -- is no longer reported as successfully submitted. `timeout(1)` passed the client's exit status through when the client exited on its own, and only overrode it on expiry; the hand-rolled watchdog now does the same, using the deadline marker to detect expiry and the wait status to detect an outright failure. If the watchdog cannot create its marker file at all, the submission fails rather than silently treating every subsequent timeout as a success. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.