Make the partition contract able to fail, and stop pinning a pid race - #171
Open
silverstein wants to merge 1 commit into
Open
silverstein wants to merge 1 commit into
silverstein wants to merge 1 commit into
Conversation
Two test defects, no product change. The partition contract compared each project's exclude list to the same arrays vite.config.mjs imports to build it. It constrained the config and never the registries, so a suite could run in no project at all, or in two, and the contract stayed green. Measured on the old test: renaming a serial-native entry so its suite ran nowhere passed 18; also listing test-runner-contract in serial-native so it ran twice passed 36 - it literally executed itself twice and reported success. serial-resource and source-identity turned out to be safe despite the same shape, because one has a literal oracle beside it and the other is derived from the import graph. serial-native had no oracle anywhere, which was the live hole, and the disjointness check only ran from serial-resource's side so source-identity against serial-native was never compared. Replaced with a test that walks the repository the way Vitest globs it, resolves each project's real claim from the built config, and requires every non-native suite to be claimed exactly once, native suites zero times, no project empty, and no include entry naming a file that does not exist. It mutates its own inputs in-test and asserts both a double-include and a renamed entry are reported, so the resolver cannot go vacuous. Added the missing serial-native oracle and full pairwise disjointness. Patterns that are neither a Vitest default nor a literal path are refused rather than silently resolving to nothing. The canvas latch test wrote a marker owned by deadPid() and asserted "latched". pdfjs-subprocess.js documents in its own comment that a reused pid makes the guard answer "concurrent" instead, deliberately, failing toward retryable. The test pinned one of two correct outcomes and the OS picked which. Holding the pid unreusable does not work and the reason is worth recording: the only POSIX way to reserve one is an unreaped zombie, and kill(pid, 0) succeeds on a zombie, so the product would read it as alive and answer "concurrent" - precisely the outcome to avoid. Instead deadPid() now probes before handing the number out, and the assertions re-run with a fresh pid only when the check failed and the owner is now alive, which is exactly when "concurrent" is correct. A real latch break fails on the first pass because its owner stays dead. The two force cases keep a bare deadPid() because force short-circuits before the latch is read. Both fixes cost nothing in detection: three product mutations against the latch guard produce identical red before and after. One mutation stays uncaught by either version - misreading another user's live process as dead - because it needs a process owned by a different uid. Pre-existing, recorded rather than hidden. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Two test defects. No product change.
1. The partition contract could not fail
test-runner-contract.test.jscompared each project's exclude list to the same arraysvite.config.mjsimports to build it. It constrained the config and never the registries.Measured against the old test:
serial-nativeentry → its suite runs in no projecttest-runner-contract→ it runs in two projectsdocling-macos-supervisorfromserial-nativeThe middle row is the one to look at: the old contract ran itself twice and reported green. I reproduced that independently — with the double-claim applied, the new test fails five ways and visibly executes itself twice while doing it.
Not all four projects were broken.
serial-resourceandsource-identityhave the same self-comparison shape but are safe — one has a literal oracle beside it, the other is derived from the import graph.serial-nativehad no oracle anywhere, which was the live hole, and the disjointness check only ran fromserial-resource's side, sosource-identity ∩ serial-nativewas never compared.The replacement walks the repository the way Vitest globs it, resolves each project's real claim from the built config, and requires every non-native suite claimed exactly once, native suites zero times, no project empty, and no
includeentry naming a file that does not exist. It mutates its own inputs in-test and asserts both a double-include and a renamed entry are reported, so the resolver itself cannot go vacuous. Adds the missingserial-nativeoracle and full pairwise disjointness. Patterns that are neither a Vitest default nor a literal path are refused rather than silently resolving to empty.2. A test pinned one of two documented outcomes
embedded-native-canvas-latch.test.jswrote a marker owned bydeadPid()and asserted"latched". Butpdfjs-subprocess.js:1105documents in its own comment that a reused pid makes the guard answer"concurrent"instead — deliberately, failing toward retryable. The test pinned one of two correct outcomes and the OS chose which.My suggested fix — hold the pid unreusable — does not work, and the reason is worth recording. The only POSIX way to reserve a pid is an unreaped zombie, and
kill(pid, 0)succeeds on a zombie, so the product would read it as alive and answer"concurrent"— precisely the outcome to avoid.Instead
deadPid()probes before handing the number out, and assertions re-run with a fresh pid only when the check failed and the owner is now alive — exactly when"concurrent"is correct. A genuine latch break fails on the first pass, because its owner stays dead. The twoforcecases keep a baredeadPid()sinceforceshort-circuits before the latch is read.Detection is unchanged: three product mutations against the guard produce identical red before and after.
Reported, not hidden
One mutation stays uncaught by either version —
processIsAlivemisreading another user's live process as dead. It needs a process owned by a different uid, which a test cannot reliably create without root. Pre-existing.Full gate: 2484 passed, the only failures being the three
source-identitysuites on the dirty-tree guard. Both changed suites green standalone (49 tests).server/untouched, mirrors verified identical, no oracle regeneration needed.🤖 Generated with Claude Code