fix(gateway): amortise Windows ACL checks into one PowerShell process - #91
alphastorm wants to merge 8 commits into
Conversation
Closes #90. The Windows daemon startup path performed 11 ACL operations before the loopback listener could bind, and each one spawned its own PowerShell process. A single minimal Get-Acl spawn measured 1854ms on a 2-vCPU Windows Server 2025 host, so the ACL work alone cost about 20.4 seconds against a 15 second readiness budget. Install could not succeed there, and it failed exactly as arithmetic predicts: the Scheduled Task ran, the daemon process started and stayed alive, and the listener was never bound before install gave up and rolled back. The fix is at the cause. One long-lived PowerShell helper now serves every ACL request over a newline-delimited JSON loop, so 11 processes become 1. The helper is unref'd, never holds the event loop open, and exits by itself when our stdin closes. The readiness budget is additionally derived from the measured cost of that one spawn rather than being a bare constant, with a hard ceiling so a genuinely broken install still fails in bounded time. Security semantics are preserved and in two places improved. The validation logic is unchanged: protected ACL, owner is the current user, exactly the current-user and S-1-5-18 entries, AccessAllowed, mask and flags identical. ErrorActionPreference=Stop makes a non-terminating Get-Acl error a structured per-path failure rather than an opaque exit code, and a reply whose id does not match its request kills the helper and throws, so a batched failure can never be attributed to the wrong path. Non-win32 behaviour is unchanged; those functions still early-return. Four tests pin the contracts that must not regress: one run uses a single PowerShell process, a foreign principal is rejected with the offending path named, an unresolvable directory is rejected naming the one that failed, and an unreadable token ACL is fatal rather than being mistaken for a missing token. The real proof is the Windows CI job plus a future run on a 2-vCPU host; Windows could not be executed locally on macOS.
Two mitigations from independent review of this branch. A helper that accepted a request and never answered blocked that ACL check forever. During install the readiness budget bounded it, but a directly-run serve had no such bound, so the wait is now capped and reported as a timeout rather than as a hang with no diagnostic. Helper stderr was discarded, which meant the most likely failure of this design - a helper that never reaches its reply loop because the script is malformed, powershell.exe is missing, or execution policy blocks it - was reported only as "exited before replying". stderr is now captured and joined to the failure it explains. The read happens on its own task, so the catch yields once before sampling it; without that the informative part is lost to a race, which is exactly the case this reporting exists for. Reporting lives in one place. readWindowsAclReply throws plain messages and performWindowsAclRequest decides how a failure is presented, including the cause text that the previous wrapper dropped from the message an operator actually reads. A new test pins it: a helper that dies during start-up surfaces its own stderr. Mutation-proven, 16 pass to 15 pass and 1 fail when the capture is removed. The fake gained a stderr channel and an early-exit mode, because without them no test could reach the start-up path at all. Two other review items were checked and closed without code. The claim that the ported predicate stopped requiring exactly two ACEs is refuted: main already carries the same `rules.length >= allowedSids.size && rules.length <= 2` at config.ts:118-119 and already prints currentIsSystem, so the branch changed neither. The question of whether unref() lets the daemon exit while a reply is pending was answered by experiment on Bun 1.3.14: with the child unref'd and a stdout read outstanding as the only pending work, the read settled after the child's full delay rather than the process exiting.
Independent review outcomeRan a focused read-only security review (Claude Opus, trust-boundary lane per the critical-review skill's routing). Class assessed as reusable internal path — not production, revertible by reverting this PR — so one focused reviewer rather than a full council. Four items returned. Dispositions:
R1 being refuted is worth stating plainly: the reviewer was appropriately uncertain and told me exactly how to check, and the check disproved it. That is the process working, not a wasted review. Residual risk, statedU3 stands and is the honest gap: Local verification: |
The previous commit read helper stderr with a standing background task. On Windows that hung `bun test apps/gateway/test/config.test.ts` for 23 minutes until the job's 25 minute ceiling cancelled it. The cause is the fact the unref experiment in that same commit had already established and I read only half of: a pending read keeps the process alive even though the child is unref'd. That is the property that makes a reply safe to await, and it is equally the property that makes an open-ended read of a pipe nobody writes to fatal. The stderr read never reached EOF, so the test runner could never exit. stderr is now drained only on the failure path, and only after the helper has been killed so the pipe reaches EOF promptly. A 250ms race remains as a backstop rather than as the mechanism. No read is outstanding at rest. The test fake now hands back a real ReadableStream instead of an async generator, so it exercises the same shape Bun.spawn returns rather than a convenient stand-in that could not have surfaced this. Still 16 pass locally and the stderr test is still mutation-proven. The honest caveat is unchanged: only the Windows job can confirm this, because the hang it fixes was not reproducible on macOS.
The helper hung bun test on Windows and no local platform can reproduce it, since macOS early-returns from every Windows path. This runs the exact spawn with the exact script text against real powershell.exe and reports what happens, so the next change is driven by an observation. Every wait is bounded. A probe that reports no reply in 10s is a result; an unbounded one is another 25 minute cancellation that teaches nothing. Temporary, delete once #90 is closed.
The Windows checkout rewrites the working tree to CRLF, so the probe's regex anchored on ";\n" matched nothing and the job failed before it could measure anything.
The measured result is that the helper starts cleanly, writes nothing to stderr, and never answers, so [Console]::In.ReadLine() is not receiving our piped line under -Command. This compares that baseline against -File, the documented way to run a script that reads its own stdin.
All three spawn forms replied, so the earlier no-reply reading was a 10s bound shorter than a cold module load, not a broken stdin protocol. The actual failure is Get-Acl reporting that its module could not be loaded. main strips PSModulePath identically, so this isolates whether the stripping or the newly added ErrorActionPreference=Stop is what makes it fatal.
It answered its question. Findings are recorded on PR #91.
Parked as draft — not on the alpha critical pathThe alpha is scoped to a single qualified platform (Linux), which the ledger explicitly permits, so Windows is not advertised and #90 does not gate GO. Parking this rather than continuing to debug it on a 3-minute CI loop. What the probe establishedRan a temporary Windows-runner probe (now deleted) against the real
The helper works. Stripping Two conclusions I published and then had to withdrawWorth recording, because the pattern matters more than the result.
Both came from acting on one measurement without checking whether the measurement's bound was sound. What is actually still unknownWhy Next step when this resumesGet a Windows host with a 10-second edit-test loop rather than a CI push cycle. The Vultr recipe in
|
1 participant
Closes #90.
The arithmetic
The Windows daemon startup path performs 11 ACL operations before the loopback listener can bind, and each spawned its own PowerShell process. A single minimal
Get-Aclspawn measured 1854 ms on a 2-vCPU Windows Server 2025 host:Even the best sample exceeds the budget. Install could not succeed on that host, and it failed exactly as predicted: the Scheduled Task ran, the daemon process started and stayed alive ~18 s, and the listener was never bound before install gave up and rolled back.
The 11 operations were counted by replaying
runServe's real prelude withplatformforced to win32 andBun.spawnfaked — measured, not traced.The fix is at the cause
One long-lived PowerShell helper serves every ACL request over a newline-delimited JSON loop: 11 processes → 1. It is
unref()ed so it never holds the daemon's event loop open, and exits by itself when our stdin closes.The readiness budget is additionally derived from the measured cost of that one spawn rather than a bare constant, with a hard ceiling so a genuinely broken install still fails in bounded time. The batching is the fix; the budget change is defence in depth, not the remedy.
Security semantics preserved, and improved in two places
Validation logic is unchanged: protected ACL, owner is the current user, exactly the current-user and
S-1-5-18entries,AccessAllowed, identical mask (2032127) and flags.Two strict improvements:
$ErrorActionPreference='Stop'turns a non-terminatingGet-Aclerror into a structured per-path failure instead of an opaque exit code.Non-win32 behaviour is byte-identical; those functions still early-return.
Tests
Four new tests pin contracts that must not regress, driven by a protocol-faithful fake:
icacls /grant *S-1-1-0:F)That last one blocks a real downgrade: silently treating an unreadable ACL as "no token yet" would mint a new token beside an unverifiable one.
Verified locally:
bun test apps/gateway/test/config.test.ts→ 15 pass / 0 fail, 46 assertions.bun run typecheckclean across all four workspaces.Threat-model impact
Touches the code that enforces private-path ACLs on Windows, so it is security-relevant by construction. The enforced property is unchanged; what changes is process topology and error attribution. The batching risk — one failure being blamed on the wrong path — is closed explicitly by request/reply id matching rather than assumed away. Paths no longer travel through the helper's environment, and the request wire is restricted to printable ASCII so a stdin code page cannot mangle a path.
Windows cannot be executed locally on macOS, so the real proof is the
windows-service-lifecycleCI job plus a future run on a 2-vCPU host — the environment where the original failure was measured.