v0.50.0.1 fix(store): preserve argv boundaries in pglite-lock holder identity (#5072) - #5073
Draft
Masashi-Ono0611 wants to merge 12 commits into
Draft
Conversation
…robe
pglite-lock's isPidReusedByOtherProgram had its own local readProcessArgs
helper that only tried `ps` then `/proc/<pid>/cmdline` — both absent on
Windows. On win32 this always returned null ("unknowable"), and the
fail-safe design treats that as "not reused" (still alive), so a dead
gbrain holder whose PID got recycled by an unrelated Windows process was
never reaped: the PGLite data-dir lock stayed wedged until manual cleanup.
autopilot-lock.ts's readProcessCommand already grew a win32 branch (via
Get-CimInstance over powershell, garrytan#4563) with a DI-friendly ProcessCommandProbeDeps
type. Delegate to it instead of maintaining a second, Windows-blind copy of
the same probe. isPidReusedByOtherProgram now takes an optional deps param
(threaded straight to readProcessCommand) and is exported so tests can
exercise the win32 branch directly via dependency injection, without
requiring a real Windows host (Windows has neither `ps` nor `/proc`, so the
existing spawn-based PID-reuse tests are unconditionally skipped there and
could never have covered this).
autopilot-lock.ts itself is unchanged — only reused.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kthq1tdcmzggtc8if3jHtW
- Use a PID guaranteed distinct from process.pid instead of a hardcoded literal that could theoretically collide with the test runner's own PID. - The same-process short-circuit test now asserts zero execFile calls, since the probe internally catches exceptions and a thrown mock alone would not have failed the test if (wrongly) invoked. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kthq1tdcmzggtc8if3jHtW
Adds a control test alongside the existing win32 CIM-detection test: same pid/recordedCommand/live-holder scenario, but with the probe deps shaped like pglite-lock's own (now-deleted) pre-fix readProcessArgs — try `ps`, then /proc, no win32 branch — which is exactly what ran unconditionally before this fix, on every platform including real Windows (where neither API exists). It asserts `false` (the old false-negative), directly contrasting with the win32-branch test's `true` for the identical inputs. This gives an assertion-level, same-call-shape before/after comparison that the mechanical check-test-discriminates.sh script's whole-file revert cannot produce here (reverting pglite-lock.ts also removes the newly exported isPidReusedByOtherProgram itself, so the script's revert-and-rerun hits an unresolved import rather than a wrong runtime answer). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kthq1tdcmzggtc8if3jHtW
Without an explicit platform, the control test would default to the real host OS's process.platform. On a win32 CI runner that would select the Get-CimInstance branch (which then fails via the mocked execFile for an unrelated reason — a PowerShell error, not the absence of ps/proc), rather than reproducing the pre-fix probe's actual shape. Pin platform: 'linux' so the test deterministically exercises the /proc-then-ps branch regardless of which OS runs the suite (Codex review nit). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kthq1tdcmzggtc8if3jHtW
The test named "CommandLine quoted by CIM" supplied a fixture with no literal quotes, so it didn't exercise what its name claimed. Add real quotes around the executable path, matching how Win32_Process.CommandLine quotes paths containing spaces (Codex review nit). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kthq1tdcmzggtc8if3jHtW
Adds preFixReadProcessArgs, a byte-for-byte control-flow snapshot of the deleted src/core/pglite-lock.ts readProcessArgs (try ps, then /proc, nothing else — the only injection accommodation is taking the two OS calls as parameters instead of calling execFileSync/readFileSync directly, since the deleted code had no injection seam at all). Calling it directly with mocks simulating a real Windows host (neither ps nor /proc exist there) returns null for the same recycled-PID scenario the win32-branch test resolves — the literal restored old probe, still broken, side by side with the real fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kthq1tdcmzggtc8if3jHtW
The throwing readCmdlineFile mock proved the win32 branch tolerates a /proc attempt failing, not that it never attempts one — the probe's own try/catch around that branch would silently swallow an unintended call. Track calls explicitly and assert zero (Codex review nit). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kthq1tdcmzggtc8if3jHtW
The execFile mock in "recycled PID is detected via Get-CimInstance" previously returned useful data regardless of which command was invoked, so it couldn't discriminate "the win32 CIM branch ran" from "some other exec call happened to run and got the same canned answer" — on a real Windows host, `ps` is not an installed binary and would fail, so a mock that always succeeds understates what the fix actually requires. Manually verified (not part of this commit): temporarily forcing the probe to ignore deps.platform (simulating the pre-fix code path, which never reaches the win32 branch) makes this exact test fail with "expected true, received false" — 37 pass / 1 fail — and restoring the one-line change returns to 38 pass / 0 fail. This is the assertion-level, same-test, red/green discrimination for the actual production code path (as opposed to the deps-only or reconstructed-function controls already in the suite). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kthq1tdcmzggtc8if3jHtW
…ux pid_ns branch isPidReusedByOtherProgram's Linux-specific pid_ns/boot_id gating checked process.platform directly instead of deps?.platform, so the win32-simulation test (deps.platform: 'win32') actually exercised the Linux branch when run on a Linux CI runner (process.platform is really 'linux' there) — reading real /proc markers and returning false before ever reaching the injected win32 probe. Reproduced on this machine by forcing process.platform to 'linux' via Object.defineProperty: reused was false (expected true) and the win32 execFile mock was never called. After using (deps?.platform ?? process.platform), reused is true and the mock is called exactly once, matching the assertion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kthq1tdcmzggtc8if3jHtW
…lens finding) isPidReusedByOtherProgram's pre-existing Linux-specific pid_ns/boot_id gate (unrelated to this PR's change) checks (deps?.platform ?? process.platform) === 'linux'. The control test pinned deps.platform to 'linux' to force the /proc-then-ps fallback path, but with a null recordedPidNs/recordedBootId that gate returns false immediately on a real Linux CI runner -- before readCmdlineFile/execFile are ever called. The assertion (reused === false) still passed, for the wrong reason, silently on Linux CI. Reproduced: forcing process.platform to 'linux' via Object.defineProperty and running the test showed cmdlineCalls/execCalls both 0 even though the assertion passed. Switched platform to 'darwin' (clears both the win32 CIM branch and the Linux namespace gate) and added toHaveLength(1) assertions on both injected probes so a future regression fails loudly instead of silently passing. Verified green on macOS and under a simulated-Linux preload, with the probes now demonstrably reached in both cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kthq1tdcmzggtc8if3jHtW
…n path comparison Scope-narrows the previous approach (delegating every platform through autopilot-lock's shared readProcessCommand) after a second maintainer-lens review flagged it as high blast-radius for reordering the non-Windows ps-then-/proc probe as a side effect. - Restore a local readProcessArgs wrapper: win32 delegates to the shared readProcessCommand (autopilot-lock's garrytan#4563 CIM branch, unmodified); every other platform keeps the exact original ps-then-/proc order, now DI-testable via the same ProcessCommandProbeDeps. - Harden isPidReusedByOtherProgram's holder-comparison for win32: case-fold and split on both `/` and `\` only on win32 (NTFS is case-insensitive and uses backslashes). Before this PR, Windows always hit `cmdline === null` and never reached this comparison at all, so its case-sensitive, `/`-only logic had never been exercised against real Windows command lines — a gap identified via masa-codex design consultation. Without the fix, a live legitimate Windows holder reported with different case or backslash separators would be misclassified as "a different program" and staged for reaping (false-steal) — the same class of bug that already happened once on non-Windows (see the existing "False-steal hardening" comment). - Add regression tests proving both false-steal scenarios (case-only drift, backslash-path drift) fail without the hardening and pass with it, plus two tests proving the non-Windows probe order (ps first, /proc fallback) is unchanged. Removed the now-moot byte-for-byte reconstruction of the deleted readProcessArgs — the real function is restored to production and exercised directly instead. Verification: bun run typecheck clean, bun run verify 54/54 green, bun test test/pglite-lock.test.ts: 42 pass / 3 skip / 0 fail. Discrimination test: reverted src/core/pglite-lock.ts to the pre-narrowing HEAD, ran test/pglite-lock.test.ts → 38 pass / 4 fail (the 4 new regression tests). Restored → all pass. Positive-control verified: temporarily reverted just the comparison hardening (case-fold + backslash split) and confirmed both new false-steal tests fail (reused=true) against the un-hardened logic, then restored and confirmed green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kthq1tdcmzggtc8if3jHtW
…ated with win32 hardening (garrytan#5072) Stacked on garrytan#5065 (fix/pglite-lock-windows-process-probe). Integrates masa-codex's garrytan#5072 fix (recordedCommand's whitespace-joined string cannot round-trip argv boundaries when a script path itself contains whitespace, causing a false-steal) with garrytan#5065's win32-specific case-fold/backslash hardening, which the original garrytan#5072 patch — built against pre-garrytan#5065 master — did not have. - New locks record `argv: string[]` alongside the legacy `command: string`. - `isPidReusedByOtherProgram` prefers the structured `argv` comparison (`recordedArgvProvesPidReuse`) when present; locks written by older gbrain versions (no `argv` field) fall back to the legacy whitespace-joined comparison unchanged. - `recordedArgvProvesPidReuse` never re-splits `argv[0]` on whitespace (the garrytan#5072 fix), AND applies garrytan#5065's win32 case-fold + dual-separator (`/` and `\`) hardening — without this, a newly-written lock would have reintroduced the exact case-only/backslash-only false-steal garrytan#5065 closed for legacy locks, since ALL new locks always carry `argv`. Verification: bun run typecheck clean, bun run verify 54/54 green, bun test test/pglite-lock.test.ts: 58 pass / 3 skip / 0 fail (30 new assertions: masa-codex's original whitespace/malformed-argv regression tests, ported onto this branch, plus 5 new win32 case-fold/separator tests for the argv path that the original garrytan#5072 patch did not cover). Positive-control: temporarily reverted recordedArgvProvesPidReuse's case-fold + separator handling and confirmed the 2 new win32 false-steal tests fail (misclassify a live holder as reused) against the un-hardened comparison, then restored and confirmed green. Automated discrimination test (scripts/check-test-discriminates.sh) hits the same limitation noted on garrytan#5065: reverting to a point before recordedArgvProvesPidReuse existed produces an import SyntaxError rather than a logic-based test failure, since the symbol is newly exported. The manual positive-control above is the substantive evidence for this specific fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kthq1tdcmzggtc8if3jHtW
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
1 participant
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.
Stacked on #5065 (
fix/pglite-lock-windows-process-probe) — this PR's base branch isgarrytan:fix/pglite-lock-windows-process-probe, notmaster. It builds directly on #5065's win32 hardening rather than duplicating or conflicting with it. Please review/merge #5065 first; this PR's diff (once #5065 lands) will shrink to just the commit below.Problem (#5072)
isPidReusedByOtherPrograminsrc/core/pglite-lock.tscompares a lock's recorded holder command against the live process's command line by re-splitting a single space-joined string (recordedCommand, stored asprocess.argv.slice(1).join(' ')). When the recorded command contains a path with whitespace (a Windows username, a macOS "Full Name" home directory, etc.), the token-boundary recovery breaks, and a still-live, legitimate lock holder can be misclassified as "a different program" and staged for reaping (a false-steal). Full repro and root cause: #5072.Fix, integrated with #5065's win32 hardening
This PR was originally implemented independently against pre-#5065
master(by an external reviewer/collaborator, masa-codex), then integrated here on top of #5065's branch after review surfaced that the two changes touch the exact same function and would otherwise conflict/regress each other:argv: string[](the unmodifiedprocess.argv.slice(1)) alongside the legacycommand: string.commandis kept for legacy readers and human-readable display.isPidReusedByOtherProgramprefers the structuredargvcomparison (recordedArgvProvesPidReuse) when a lock has anargvfield. Locks written by older gbrain versions (noargvfield) fall back to the legacy whitespace-joined comparison, byte-for-byte unchanged.recordedArgvProvesPidReusenever re-splitsargv[0]on whitespace (the core pglite-lock: recorded-command token-boundary comparison misclassifies a live holder when the path contains whitespace (false-steal) #5072 fix) — and applies fix(store): pglite-lock reuses autopilot-lock's win32-aware process probe #5065's win32 case-fold + dual-separator (/and\) hardening. Without this second part, a newly-written lock (which always carriesargv) would have reintroduced the exact case-only/backslash-only false-steal that fix(store): pglite-lock reuses autopilot-lock's win32-aware process probe #5065 closed for legacy locks — the original pglite-lock: recorded-command token-boundary comparison misclassifies a live holder when the path contains whitespace (false-steal) #5072 patch, built before fix(store): pglite-lock reuses autopilot-lock's win32-aware process probe #5065 existed, did not have this.recordedArgv(not an array, empty, non-string element, or a blank first element) reads as unknowable, not evidence of reuse — same fail-safe bias as every other check in this file.Tests
test/pglite-lock.test.ts:argvfail-safe tests — plus a puredescribe('structured lock argv comparison (#5072)')block covering the same cases without process-spawning (so it also runs in sandboxes that deny process inspection).recordedArgvProvesPidReusemirroring fix(store): pglite-lock reuses autopilot-lock's win32-aware process probe #5065's hardening — case-only drift, backslash-only drift, a same-case/same-path baseline, a genuinely-different-program precision check, and a non-win32 control confirming case sensitivity is unchanged off Windows.bun test test/pglite-lock.test.ts: 58 pass, 3 skip (pre-existing spawn-based tests that self-skip off-Windows), 0 fail.bun run typecheckandbun run verify(54 checks) both clean.Positive-control: temporarily reverted
recordedArgvProvesPidReuse's case-fold + separator handling and confirmed the 2 new win32 false-steal tests fail (misclassify a live holder as reused) against the un-hardened comparison, then restored and confirmed green.Discrimination test caveat:
scripts/check-test-discriminates.shhits the same limitation already noted on #5065 — reverting to a point beforerecordedArgvProvesPidReuseexisted produces an importSyntaxError(the symbol is newly exported), not a logic-based test failure. The positive-control above is the substantive evidence for this specific fix, not the automated discrimination script's numeric output.What I could not verify
Same Windows-hardware caveat as #5065: all win32-path verification (both #5065's and this PR's) is against dependency-injected/pure-function test doubles, not a live win32 process table.