fix(agy): move carrier sweep off the startup path, enable on macOS - #1406
Merged
Conversation
Convert the carrier sweep's directory scan (readdir/lstat/readFile/rm) from sync fs calls to fs/promises, so it no longer blocks the event loop while it runs. The call site in runAgy.ts still awaits it in place, so this preserves the current blocking-before-hook-server ordering exactly -- only the mechanism changes. Also adds racing-safety tests proving a carrier created after the readdir() snapshot was taken (or written by this session's own prepareAgyHookCarrier() while a sweep is in flight) is never examined, since decoupling the call site (next commit) makes that interleaving possible for the first time.
sweepAgyHookCarriers is a backup path -- normal teardown already removes a carrier via cleanupAgyHookCarrier, so sweep only matters after a crash. There is no reason for it to delay this session's own startup (hook server, carrier prep, PTY spawn). Now that it is async (previous commit), fire it without awaiting it instead of blocking on it before startHookServer. It runs concurrently with this session's own prepareAgyHookCarrier() call further down; the previous commit's racing-safety tests prove that interleaving is safe. Still guaranteed to never throw or leave an unhandled rejection.
The upcoming macOS/Windows identity probes are async child-process calls (ioreg/sysctl, reg query), but writeOwnerMetadata is called synchronously from prepareAgyHookCarrier -- including from agyPtyLauncher.ts's respawn path, which must stay synchronous per its fail-closed contract. A warm cache lets that synchronous call read a value computed ahead of time instead of needing to await a probe. Adds computeLocalCarrierScopeAsync (platform dispatcher, Linux-only for now -- delegates to the exact same sync read computeLocalCarrierScope already does, so no platform's observable output changes here), warmCarrierScope (populates the cache once, including caching a failed probe so it is never retried), and resolveLocalCarrierScope (used by sweepAgyHookCarriers; bypasses the cache for any non-default probe so the many existing custom-probe tests keep forcing a fresh computation per call). writeOwnerMetadata reads the cache first and falls back to the existing synchronous Linux computation when the cache was never warmed -- unchanged behavior wherever nothing calls warmCarrierScope yet.
computeLocalCarrierScope was Linux-only, so sweepAgyHookCarriers was a no-op on macOS -- only crash leftovers there ever accumulated (normal teardown still works via cleanupAgyHookCarrier), but they accumulated forever. Adds a strong-identity scope for macOS: IOPlatformUUID (ioreg) + kern.bootsessionuuid (sysctl), the macOS analogue of Linux's boot_id -- NOT kern.boottime, which a live re-measurement showed drifts by over a second across 8 days on the same boot (recomputed from NTP-adjusted wall clock time under the hood). The probe runs by absolute path via execFile (never a shell, never PATH-dependent) with a 2s timeout; a failed or timed-out probe still resolves to undefined, preserving sweepAgyHookCarriers's existing "cannot identify -> preserve everything" contract unchanged. Windows is deliberately not enabled. MachineGuid -- the obvious candidate for a win32 scope -- is a machine identifier, not a PID-space identifier: it is written once at OS install time and is NOT regenerated by cloning a disk image (that is exactly what sysprep exists to fix). Two clones of the same image sharing a HAPI_HOME (SMB share, sync folder, shared VM folder) would compute the identical win32:<guid> scope while having completely independent PID spaces, so sweep could delete a live session's carrier and its --dangerously-skip-permissions approval bridge with it. No cheap per-boot alternative exists on Windows: no boot-id registry key, Get-CimInstance's LastBootUpTime measured 1.4-2.5s per call (an order of magnitude too slow for identity plumbing), and net statistics/systeminfo output is locale-dependent. computeLocalCarrierScopeAsync's docstring records the full reasoning and the bar for re-enabling it (a boot-scoped, not machine-scoped, identifier cheaper than CIM). Windows carriers keep accumulating exactly as before this change -- this is a purely additive capability for macOS. Wires warmCarrierScope into runAgy.ts's PTY setup: fired without awaiting it right after the sweep call (so the probe cost -- two child processes on macOS -- overlaps with hook server startup instead of adding to it), then awaited immediately before prepareAgyHookCarrier() so its synchronous owner.json write reads a warm cache. agyPtyLauncher.ts's respawn path needs no equivalent wiring: it always runs in the same, by-then-warm process.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
- None.
Summary
- Review mode: initial
- No high-confidence correctness, security, regression, data-loss, performance, or maintainability issues found in the added/modified lines.
- Residual risk: Linux automation exercises injected probes and captured macOS output, but not the real macOS
ioreg/sysctlsuccess path.
Testing
- Not run (automation); static review only. The repository
testcheck was pending at review time.
HAPI Bot
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Problem
Two follow-ups on the carrier sweeping that landed recently.
Sweeping runs on the session startup path.
sweepAgyHookCarriers()is called synchronously as the agy PTY session comes up, and every filesystem call inside it is synchronous too, so a full directory scan sits between the user starting a session and the session appearing. Sweeping is a backstop rather than a primary path: normal teardown already removes a session's own carrier throughcleanupAgyHookCarrier, and sweeping only matters when that never got to run — a crash, akill -9. There is no reason for it to hold up startup.Sweeping is inert on macOS.
computeLocalCarrierScope()derives its scope from/proc/sys/kernel/random/boot_idand/proc/self/ns/pid, so it returnsundefinedoff Linux and sweeping preserves everything. Preserving is the safe direction and was the right default to ship with, but it does mean crash leftovers accumulate on macOS with nothing to reclaim them.Solution
Move sweeping off the startup path. Sweeping is asynchronous now (
fs/promisesthroughout) and is no longer awaited before the session starts. The "never throws" contract is preserved, now covering rejections as well. Three independent properties make it safe to run alongside carrier creation, and each is pinned by a test: a carrier created after thereaddirsnapshot is not in the list at all; a carrier caught in the snapshot before itsowner.jsonexists reads as unowned and is preserved; and onceowner.jsonexists its PID is live, so the carrier is preserved.Give macOS a real scope. Scope identity branches on
process.platform, and each platform tags its scope string so two platforms can never compute the same value. Linux keepslinux:<boot_id>:<pid-ns-inode>unchanged; macOS getsdarwin:<IOPlatformUUID>:<kern.bootsessionuuid>.kern.bootsessionuuidis the macOS counterpart to Linux'sboot_id: a UUID the kernel mints during boot and also stamps into panic logs. Reading it costs onesysctl(measured 18-19 ms), andIOPlatformUUIDoneioreg(27-28 ms). It is worth saying why the boot timestamp is not used instead, since that is the more obvious choice.sysctl kern.boottimeis recomputed from the wall clock on each read, so NTP corrections move it: on a Mac that had been up for eight days without rebooting, the reported boot time drifted 1.3 seconds between two readings. A scope built on it would stop matching the scope already recorded in a carrier'sowner.json, and sweeping would silently stop recognising anything.Every probe keeps the existing failure posture. Each runs as a direct child process with an absolute path and a timeout, values are shape-checked before use, and anything unexpected — a missing binary, a non-zero exit, a timeout, an unparseable value — yields
undefined, which makes sweeping preserve everything, on any platform. There is still no weak fallback anywhere: hostname is not an identity.Because the macOS probes are child processes rather than
/procreads, the scope is computed once per process and memoised, and warmed asynchronously during startup so it overlaps with hook server startup rather than adding to it.Tests
Unit tests cover macOS scope computation (success, partial probe failure, total failure), that platforms without a strong identity preserve every carrier and never invoke a probe, that no two platforms can produce the same scope string, that the warm-up is awaited before a carrier is created, and the three race properties above.
ScopeProbeinjection drives the platform branches directly, so no module-wide mocking is needed.Verified live on Linux and macOS: a carrier whose owner PID has been reaped is removed, while carriers owned by a live PID, owned by a PID belonging to another user (
EPERM), missingowner.json, carrying a foreign scope, or owned by the running process itself are all preserved. The same run on Windows confirms it preserves everything. End to end with two concurrent agy PTY sessions against a hub, the second session's startup sweep reclaimed only the dead carrier and left the first session's live one alone, with its own carrier created while that sweep was still in flight.