test(runner): cover BootCanary teardown-error-join contract (RIG-3219) - #860
Open
rigel-mintaka wants to merge 3 commits into
Open
test(runner): cover BootCanary teardown-error-join contract (RIG-3219)#860rigel-mintaka wants to merge 3 commits into
rigel-mintaka wants to merge 3 commits into
Conversation
|
❌ This pull request was removed from the merge queue because it failed tests. PR #887 was used for testing. See more details here.
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 |
`BootCanary`'s documented contract is that the always-run Remove teardown's error is joined into its return, never discarded — it gates Runner startup, so a silently-swallowed teardown failure would report a canary that leaked a live VMM+virtiofsd as a clean boot, inverting the fail-closed posture on the exact path the gate protects. The `#847` review loop found that contract had zero regression coverage: swallowing both `errors.Join` blocks in `microvm_preflight.go` left the entire hermetic suite green. The named-return + `errors.Join` idiom is precisely what a "tidy the error path" refactor flattens to a plain `defer m.Remove(...)`, and error-swallow discipline is CI-enforced repo law, so the join needs a test that keeps it correct. `TestBootCanaryTeardownErrorJoined` drives an otherwise-successful canary (boot + echo + nonce all pass) with a guest whose `Shutdown` fails, and asserts `BootCanary` returns a non-nil error carrying the teardown failure while the report is still assembled from what ran before teardown. A `shutdownErr` field on `canaryFakeVM` (threaded through `canaryLaunchRecorder`, mirroring the existing `pssErr` knob) is returned from `Shutdown`, so `Remove`'s own `errors.Join` surfaces it into the named return. Test-only; `microvm_preflight.go` is unchanged (the production code was already correct). Mutation-verified: gating off the teardown join reddens only this test and leaves the other ten green; restored to green. Stacked on `#847` (RIG-3148) because it exercises the `BootCanary` teardown code that PR introduces. Spec-impact: none. Refs RIG-3219 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
changed the title
rigel-mintaka
force-pushed
the
compass-runner/rig-3219-teardown-join-test
branch
from
September 4, 2026 16:16
c8feab2 to
17a9319
Compare
|
Compass engineering docs preview: https://compass-runner-rig-3219-tear.compass-eng-docs.pages.dev Deployed from |
…rkspace-cleanup leg (RIG-3219) Fold the two floor findings from #860's review, both test-only. - The existing teardown-join test drives an otherwise-successful canary, so `err` is nil at the join and `errors.Join(nil, x)` is observationally identical to a plain overwrite — the exact "tidy the error path" refactor the test claims to catch slips through. Add TestBootCanaryBootAndTeardownErrorsBothJoined: fail BOTH the boot chain (echo exec refused) and teardown (Shutdown refused), assert the returned error carries each. Reddens under join->overwrite, and only it. - The sibling throwaway-workspace os.RemoveAll join had no regression coverage; the doc comment wrongly claimed it needed a production temp-dir seam. It is reachable through the existing launch seam: cfg.FSSharedDir is the minted workspace. Add an onLaunch hook on canaryLaunchRecorder and TestBootCanaryWorkspaceCleanupErrorJoined, which locks a subtree (0555) so os.RemoveAll fails EACCES and asserts BootCanary surfaces it. Root-skipped per go/server/socket_test.go. Correct the doc comment to point at the new test. Both mutation-verified: gate off either join and only the matching new test reddens; production code (microvm_preflight.go) unchanged. Refs RIG-3219 Co-authored-by: Matt Wilkinson <matt@rigel.build>
Round-2 review of #860 found the symmetric twin of round-1's teardown-join gap: the throwaway-workspace RemoveAll join was covered against DROPPING the join but not against FLATTENING it to a plain overwrite. TestBootCanaryWorkspaceCleanupErrorJoined drives an otherwise-successful canary, so `err` is nil when that defer runs and `errors.Join(nil, x)` is observationally identical to `err = x` — the overwrite mutant survived. Since that defer is registered first it runs last (LIFO), so an overwrite there would discard both the boot diagnostic and the teardown-session error the sibling join preserves. - Add TestBootCanaryWorkspaceCleanupAndBootErrorsBothJoined: fail the boot chain (exec refused) AND the workspace RemoveAll (EACCES on a planted 0555 subtree) and assert the returned error carries both substrings. Mutation- verified: flattening the join at microvm_preflight.go:327 to a plain overwrite reddens ONLY this test; the drop-the-join sibling stays green. - Extract the 0555-subtree plant into a shared plantLockedSubtree helper so both workspace tests share one mechanism. - Correct the parity claim in TestBootCanaryTeardownErrorJoined's comment to name both sibling workspace tests. - Document the onLaunch-under-r.mu constraint on canaryLaunchRecorder, and pin the empty session table on the workspace-cleanup path with a rationale for the deliberately-absent temp-leak assertion. Test-only; the two production files are byte-identical to main. Refs RIG-3219 Co-authored-by: Matt Wilkinson <matt@rigel.build>
mattwilkinsonn
approved these changes
Sep 5, 2026
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.
BootCanary's documented contract is that the always-run Remove teardown's error is joined into its return, never discarded — it gates Runner startup, so a silently-swallowed teardown failure would report a canary that leaked a live VMM+virtiofsd as a clean boot, inverting the fail-closed posture on the exact path the gate protects. The#847review loop found that contract had zero regression coverage: swallowing botherrors.Joinblocks inmicrovm_preflight.goleft the entire hermetic suite green. The named-return +errors.Joinidiom is precisely what a "tidy the error path" refactor flattens to a plaindefer m.Remove(...), and error-swallow discipline is CI-enforced repo law, so the join needs a test that keeps it correct.TestBootCanaryTeardownErrorJoineddrives an otherwise-successful canary (boot + echo + nonce all pass) with a guest whoseShutdownfails, and assertsBootCanaryreturns a non-nil error carrying the teardown failure while the report is still assembled from what ran before teardown. AshutdownErrfield oncanaryFakeVM(threaded throughcanaryLaunchRecorder, mirroring the existingpssErrknob) is returned fromShutdown, soRemove's ownerrors.Joinsurfaces it into the named return.Test-only;
microvm_preflight.gois unchanged (the production code was already correct). Mutation-verified: gating off the teardown join reddens only this test and leaves the other ten green; restored to green.Stacked on
#847(RIG-3148) because it exercises theBootCanaryteardown code that PR introduces.Spec-impact: none. Refs RIG-3219
Co-authored-by: Matt Wilkinson matt@rigel.build