test(runtime): stop the isolation sweep failing toward PASS (RIG-3818) - #1265
Open
rigel-mintaka wants to merge 4 commits into
Open
rigel-mintaka wants to merge 4 commits into
rigel-mintaka wants to merge 4 commits into
Conversation
The batched sweep suppressed awk's stderr and had no guard for an unopenable input. gawk treats that as fatal: it aborts the invocation, skips END, and exits 2 — so one unreadable file loses the other 199 in the batch, and the result is indistinguishable from genuine confinement. On a security assertion that is a failure toward PASS. Measured with the two awkProg strings taken verbatim from main and from this change, over a batch whose middle file is unreadable and whose last file holds the needle: main exits 2, finds nothing, and emits a fatal: diagnostic the old 2>/dev/null hid; this version exits 0 and finds it. The [[ -f && -r ]] guard filters the statically-unreadable case, so the reachable exposure is its TOCTOU window — checked during accumulation, opened up to 200 files later on a box with two live microVMs writing. Also fixes a test that did not exercise the property it names. Bash globs lexicographically, so over unpadded f1..f425 the "in the final batch" row landed at glob index 362, inside a mid-loop flush; neither row ever reached the trailing flush. Zero-padding to f%03d puts it at index 424, in the trailing flush, as the row claims. And moves two Linux-only quota probes out of the unix-tagged file. Off Linux they reach the refusal stub, so TestReadVolumeQuotaOnRealPath fails outright and TestReadVolumeQuotaAbsentPath passes for the wrong reason — its error comes from the stub, not the absent path. Latent today: no CI lane runs Go unit tests on darwin. RIG-3818
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Compass engineering docs preview: https://compass-runner-3818-sweep-fa.compass-eng-docs.pages.dev Deployed from |
…to (RIG-3818) Three comment corrections from #1265 review. No code change: stripping comments from both revisions leaves both files byte-identical. The BEGINFILE claim was wrong twice. "losing the other 199 paths" is the best case stated as the rule -- gawk aborts AT the bad input, so the loss is positional (bad file at 1/100/200 of a 200-file batch leaves 0/99/199 scanned). And the guard covers OPEN errors only: a file that opens and then fails to READ still aborts at rc=2 with the needle unfound, so the comment implied a hole was closed that is not. Filed as RIG-3862 with the residue and the accumulator bug behind it -- a fatal rc=2 reads as a clean negative to scan(), which also drops a hit already printed before the abort. The padding comment cited glob index 362 as absolute; it is locale-specific (en_US.UTF-8 gives 360, and moves f1.txt from 0 to 110). That is why two measurements of mine disagreed: both were right in different collations. The comment now states the invariant -- padded ranks are 1 and 425 in ANY collation -- with the guest C-locale number as an aside. And microvm_quota_test.go still promised coverage for the two probes this PR moved out of it: its only remaining readVolumeQuota mention was that promise. Replaced with a pointer to their new home. RIG-3818
Round-2 review fold. Comment text only: AST-stripping both revisions of both files (go/parser, dropping f.Comments and every Doc/Comment field, re-printed from the AST) leaves them byte-identical at 20812B and 7392B, with the build constraints unchanged. "drops ... any hit already printed" was false, and backwards in the direction that matters. gawk flushes stdout on its fatal path, so the match line survives the abort -- and guestSh returns stdout, which is what the cross-tenant row's strings.Contains discriminator reads. The hit text is the one channel that still works; what is lost is the batch's contribution to the exit status, because awk exits 2, the if-awk-then-found=0 never fires, and exit $found reports 1. Measured through the real accumulator: needle at file 1, read-fail mid-batch -> stdout carries the hit, FINAL-EXIT=1. The correction also brings the inline comment back to the 4-line cap, and "the second reason the scan below" now resolves its own direction rather than pointing at a comment the reader has not met. The quota header dropped its count: four tests exercise readVolumeQuota in the linux file, not the two this PR moved, so a bare pointer cannot go stale. It also regains the enforcement thread the trim lost, with the gate stated accurately -- requireQuotaFS wants an operator-provided quota'd filesystem, not root. RIG-3818
Round-3 review fold; the round returned all-clear at the gating floor, these are its two LOWs. Comment text only. The guard is gawk-specific -- BEGINFILE is a GNU extension and silently no-ops elsewhere -- and the round-2 trim had dropped the citation grounding that. Restoring it turned up a worse problem: BOTH the earlier version and the reviewer cited guest-image/default.nix, and that file contains no gawk at all. It only ASSERTS bin/awk exists in the assembled tree (its userlandContract). The awk is shipped by the agent image, agent-image/toolchain.nix, for the egress arm. Neither of us had checked the citation against main. Also names the gate that actually binds on the enforcement pointer: the microvm build tag stops that leg long before the quota gate does, so a reader was told a quota gate was sufficient to run it. RIG-3818
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.
Three test-quality defects in the microVM isolation and quota suites, all live on
main. RIG-3818.1. The batched isolation sweep could fail toward PASS
sweepScriptsuppressed awk's stderr and had no guard for an unopenable input. gawk makes that fatal: it aborts the invocation at that path, skipsEND, and exits 2 — so one bad path dropped every later path in the batch, and with stderr suppressed the result was indistinguishable from genuine confinement. On a security assertion that is a failure toward PASS.Measured with the two
awkProgstrings taken verbatim frommainand from this branch, over a batch whose middle file is unopenable and whose last file holds the needle:Fix:
BEGINFILE { if (ERRNO) nextfile }, and both2>/dev/nullsuppressions removed. The loop's suppression was hiding nothing (measured: zero stderr over a hostile tree of a chmod-000 directory, a dangling symlink, a symlink loop and a FIFO); awk's was hiding the only signal that separates a real negative from a dead probe.Known limit, stated rather than implied: the guard covers OPEN errors only. An input that opens and then fails to READ still aborts. Tracked with the deterministic test and the accumulator bug behind it in RIG-3862 —
scan()isif awk ...; then found=0; fi, so a fatal rc=2 reads as a clean negative.2. The cross-batch test did not exercise the property it names
sweepBatchSizeis 200 and the test plants 425 files. Bash globs lexicographically, so unpaddedf425.txtsorted to glob index 362 — inside a mid-loop flush — meaning the row named "in the final batch" never reached the trailing flush it exists to test. Zero-padding tof%03dputs it at index 424, in the 25-file trailing remainder.The padded ordering is collation-independent; the unpadded index is not (
en_US.UTF-8gives 360, and movesf1.txtfrom 0 to 110), which is the strongest argument for the padding.3. A Linux-only test was tagged
unixTestReadVolumeQuotaOnRealPathandTestReadVolumeQuotaAbsentPathmoved to the//go:build linuxfile. Off Linux both reach the!linuxrefusal stub, so the first fails outright and the second passed for the wrong reason — its error came from the stub, not from the absent path. Latent today: no CI lane runs Go unit tests on darwin.Verification
gofmtclean;go vetrc=0 untagged and-tags microvmgo test ./internal/runtime/ -count=1 -race -shuffle=on→ ok 22.2s-run, not a suite summarygolangci-lint0 issues untagged; at the wider--build-tags microvm ./internal/runtime/...scope exactly one pre-existingerrorsastypeissue in a file this PR does not touch (RIG-3842)GOOS=darwin go vetandgo build ./internal/...rc=0 — defect 3's whole pointThe KVM-gated legs need
COMPASS_TEST_GUEST_KERNELand hard-fail rather than skip in this environment, pre-existing at the base. So defects 1 and 2 were verified by driving the verbatim-extracted awk program and the real accumulator loop under the same gawk the guest ships, not by a guest run.Review
Three rounds. Rounds 1 and 2 each found a comment asserting a mechanism the code does not perform — round 2's was in the text written to fix round 1's. Round 3 returned all-clear, and also caught a citation that did not resolve: both the comment and the reviewer cited
guest-image/default.nixfor the guest's gawk, which only assertsbin/awkexists; the binary comes fromagent-image/toolchain.nix. Now cited correctly.