[CI] productmetrics TestDisableAndPurgeRejectsUnprovenPeerSuccessor/peer_successor_root_sync_failure red on main — purge error class mismatch reds CI / required (gc-i8xf9) - #101
Merged
Conversation
…rrier (gc-i8xf9)
TestDisableAndPurgeRejectsUnprovenPeerSuccessor/peer_successor_root_sync_failure
reddened Integration / packages-core-4-of-4, and through it CI / integration and
CI / required, blocking mr-mode auto-land (merge-skill only lands on clean CI):
control_unix_test.go:1682: purge error = productmetrics: disable-write-failed,
want class "storage-failure"
The bead filed this as a deterministic test/code assertion mismatch, likely a
fork-shed artifact. It is neither. Production classification is correct and the
subtest passes locally; this is a real race in the test's own arming logic, which
is why it presents as an intermittent red on loaded CI runners and not locally.
The subtest injects a failure at storageStepDirectorySync to exercise the
post-barrier peer-successor proof, and arms that injection as soon as
waitForMetricsState observes Preference=disabled and CleanupKind=cleanupDisable
on disk. But persistStateMutation makes the state file readable at its rename,
and beginDisableAtRoot still performs several more directory syncs before it
returns and blocks on the uploader barrier. Instrumenting the run shows the state
becomes visible with four pre-barrier syncs still outstanding, against exactly one
sync after the barrier -- so the arming window is four syncs wide, not a
microsecond edge. When the purge goroutine is descheduled anywhere in that window
(routine on a 32-vCPU runner executing package shards in parallel), the injected
error lands on the opt-out write instead: persistStateMutation returns
errStateAppliedSyncPending, and control.go:179 classifies that as
PurgeErrorDisableWrite -- the observed disable-write-failed.
Fix the synchronization rather than the assertion: wait on
beforeDisableUploaderLock, which fires after beginDisableAtRoot has fully
returned and immediately before the purge blocks on the barrier, so arming can no
longer land inside the opt-out write. This is the established pattern in this
file -- six call sites already pair that hook with the receiveUploaderAttempt
helper, and the test at line 1795 already uses the exact
receiveUploaderAttempt + waitForMetricsState shape adopted here. Measured zero
directory syncs between the hook firing and the barrier release across repeated
runs, so the new arming point is unambiguous.
Test-only change; no production behavior is touched. Deliberately scoped to the
one racy subtest: the sibling TestDisableAndPurgeRejectsPeerSuccessorReplacedDuringCleanProof
arms on storageStepEnumerate in the same shape, but instrumentation records zero
pre-barrier enumerates, so it has no equivalent window and is left alone.
Validation: reproduced the CI failure deterministically by stalling the purge
goroutine inside the pre-barrier window -- the old arming order yields
disable-write-failed, the new order yields storage-failure under the identical
stall. Full internal/productmetrics passes with -tags integration (the CI shard's
own scope), the target test passes 40 consecutive runs, and go vet is clean both
with and without the integration tag.
Author
|
Codex signoff (pre-open, comment-only — not an approval): Pre-open codex review verdict: COMMENT (pass), no actionable findings. Reviewed branch polecat/gc-i8xf9 at dd73f11 against origin/main c057108. Scope: test-only change in internal/productmetrics/control_unix_test.go (+9 lines) syncing TestDisableAndPurgeRejectsUnprovenPeerSuccessor to the uploader barrier before arming peer-successor storage failure injection. Verification performed:
Artifact: /tmp/compound-engineering/ce-code-review/20260728-164301-gc-1tcyb/ |
zook-bot
added a commit
that referenced
this pull request
Aug 14, 2026
…rrier (gc-i8xf9) (#101) TestDisableAndPurgeRejectsUnprovenPeerSuccessor/peer_successor_root_sync_failure reddened Integration / packages-core-4-of-4, and through it CI / integration and CI / required, blocking mr-mode auto-land (merge-skill only lands on clean CI): control_unix_test.go:1682: purge error = productmetrics: disable-write-failed, want class "storage-failure" The bead filed this as a deterministic test/code assertion mismatch, likely a fork-shed artifact. It is neither. Production classification is correct and the subtest passes locally; this is a real race in the test's own arming logic, which is why it presents as an intermittent red on loaded CI runners and not locally. The subtest injects a failure at storageStepDirectorySync to exercise the post-barrier peer-successor proof, and arms that injection as soon as waitForMetricsState observes Preference=disabled and CleanupKind=cleanupDisable on disk. But persistStateMutation makes the state file readable at its rename, and beginDisableAtRoot still performs several more directory syncs before it returns and blocks on the uploader barrier. Instrumenting the run shows the state becomes visible with four pre-barrier syncs still outstanding, against exactly one sync after the barrier -- so the arming window is four syncs wide, not a microsecond edge. When the purge goroutine is descheduled anywhere in that window (routine on a 32-vCPU runner executing package shards in parallel), the injected error lands on the opt-out write instead: persistStateMutation returns errStateAppliedSyncPending, and control.go:179 classifies that as PurgeErrorDisableWrite -- the observed disable-write-failed. Fix the synchronization rather than the assertion: wait on beforeDisableUploaderLock, which fires after beginDisableAtRoot has fully returned and immediately before the purge blocks on the barrier, so arming can no longer land inside the opt-out write. This is the established pattern in this file -- six call sites already pair that hook with the receiveUploaderAttempt helper, and the test at line 1795 already uses the exact receiveUploaderAttempt + waitForMetricsState shape adopted here. Measured zero directory syncs between the hook firing and the barrier release across repeated runs, so the new arming point is unambiguous. Test-only change; no production behavior is touched. Deliberately scoped to the one racy subtest: the sibling TestDisableAndPurgeRejectsPeerSuccessorReplacedDuringCleanProof arms on storageStepEnumerate in the same shape, but instrumentation records zero pre-barrier enumerates, so it has no equivalent window and is left alone. Validation: reproduced the CI failure deterministically by stalling the purge goroutine inside the pre-barrier window -- the old arming order yields disable-write-failed, the new order yields storage-failure under the identical stall. Full internal/productmetrics passes with -tags integration (the CI shard's own scope), the target test passes 40 consecutive runs, and go vet is clean both with and without the integration tag. (cherry picked from commit c00cce4)
zook-bot
added a commit
that referenced
this pull request
Aug 25, 2026
…rrier (gc-i8xf9) (#101) TestDisableAndPurgeRejectsUnprovenPeerSuccessor/peer_successor_root_sync_failure reddened Integration / packages-core-4-of-4, and through it CI / integration and CI / required, blocking mr-mode auto-land (merge-skill only lands on clean CI): control_unix_test.go:1682: purge error = productmetrics: disable-write-failed, want class "storage-failure" The bead filed this as a deterministic test/code assertion mismatch, likely a fork-shed artifact. It is neither. Production classification is correct and the subtest passes locally; this is a real race in the test's own arming logic, which is why it presents as an intermittent red on loaded CI runners and not locally. The subtest injects a failure at storageStepDirectorySync to exercise the post-barrier peer-successor proof, and arms that injection as soon as waitForMetricsState observes Preference=disabled and CleanupKind=cleanupDisable on disk. But persistStateMutation makes the state file readable at its rename, and beginDisableAtRoot still performs several more directory syncs before it returns and blocks on the uploader barrier. Instrumenting the run shows the state becomes visible with four pre-barrier syncs still outstanding, against exactly one sync after the barrier -- so the arming window is four syncs wide, not a microsecond edge. When the purge goroutine is descheduled anywhere in that window (routine on a 32-vCPU runner executing package shards in parallel), the injected error lands on the opt-out write instead: persistStateMutation returns errStateAppliedSyncPending, and control.go:179 classifies that as PurgeErrorDisableWrite -- the observed disable-write-failed. Fix the synchronization rather than the assertion: wait on beforeDisableUploaderLock, which fires after beginDisableAtRoot has fully returned and immediately before the purge blocks on the barrier, so arming can no longer land inside the opt-out write. This is the established pattern in this file -- six call sites already pair that hook with the receiveUploaderAttempt helper, and the test at line 1795 already uses the exact receiveUploaderAttempt + waitForMetricsState shape adopted here. Measured zero directory syncs between the hook firing and the barrier release across repeated runs, so the new arming point is unambiguous. Test-only change; no production behavior is touched. Deliberately scoped to the one racy subtest: the sibling TestDisableAndPurgeRejectsPeerSuccessorReplacedDuringCleanProof arms on storageStepEnumerate in the same shape, but instrumentation records zero pre-barrier enumerates, so it has no equivalent window and is left alone. Validation: reproduced the CI failure deterministically by stalling the purge goroutine inside the pre-barrier window -- the old arming order yields disable-write-failed, the new order yields storage-failure under the identical stall. Full internal/productmetrics passes with -tags integration (the CI shard's own scope), the target test passes 40 consecutive runs, and go vet is clean both with and without the integration tag. (cherry picked from commit c00cce4)
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.
Summary
Symptom
Integration / packages-core-4-of-4fails onorigin/main, reddening theCI / integrationandCI / requiredaggregators. Becausemerge-skill.shonly auto-lands on CLEAN CI, this independently blocks mr-mode auto-land.
Observed on PR#100 (gc-qin3c), run 29970618236, job 89091726066 (2026-07-23).
Root shape: deterministic test/code mismatch, not a flake
internal/productmetrics/control_unix_test.go:1614— subtestpeer successor root sync failuresetsfailSync: trueandwantClass: PurgeErrorStorage.internal/productmetrics/control.go:81—PurgeErrorStorage = "storage-failure".internal/productmetrics/control.go:73—PurgeErrorDisableWrite = "disable-write-failed",which is what production actually returned.
So the production classification of an injected root-sync failure and the
test's expectation disagree. This is an assertion mismatch, not a timeout or
resource flake — it will fail every run.
Likely a fork-shed artifact: a dropped fork commit that changed either the
classification or the expectation, leaving the two out of sync. Confirm against
the shed plan before choosing which side to move.
Pre-existing — NOT attributable to PR#100
internal/productmetricspaths (it touchesmodelwindow, sessionlog, sling, cmd/gc tests, Makefile, scripts, census, go.mod).
internal/productmetricstree is BYTE-IDENTICAL between PR#100's CI base89e2e699fand current mainc0571088c: both7fd27ffdce432b5994954841291efc2bef8e34d0.(Tree identity, not ancestry —
89e2e699fis NOT an ancestor ofc0571088cafter the shed force-push, so a log range would be invalid here.)
Therefore the failure reproduces on current main as-is.
Not a duplicate
Checked 400 open+closed beads for
productmetrics|DisableAndPurge|storage-failure|disable-write-failed— no match. Distinct fromgc-0odpu, which tracksIntegration / rest-full-2-of-16failing from a bd init Dolt schema-migrationcause (beads#4566) — different shard, different root cause.
Priority note
Filed P2 to match its closest live sibling
gc-0odpu(the other openintegration-shard land-blocker). The established precedent for a deterministic
pre-existing base red that blocks ALL mr-mode auto-land is P1 (gc-bvjbs,
gc-nlz68, gc-cfke1). It is filed lower only because the merge queue is
currently EMPTY and both open PRs (#60, #100) are operator-owned, so the
blockage is latent. Raise to P1 the moment mr-mode work queues behind it.
Refinery scope
Filed for tracking per the ZFC table (pre-existing test failure -> file a bead,
never fix it). Refinery does not fix this. Attribute
Integration / packages-core-4-of-4red to this bead, NOT to the branch under review.Implementation notes
Implemented: sync the peer-successor failure injection to the uploader barrier.
Commit dd73f11 on polecat/gc-i8xf9 (base origin/main c057108). Test-only,
+9 lines in internal/productmetrics/control_unix_test.go.
WHAT WAS WRONG: TestDisableAndPurgeRejectsUnprovenPeerSuccessor/
peer_successor_root_sync_failure armed its storageStepDirectorySync failure
injection on waitForMetricsState (state file readable). The state file becomes
readable at its rename — several directory syncs BEFORE beginDisableAtRoot
returns — so under load the injected failure could land inside the opt-out
write and get classified PurgeErrorDisableWrite, producing
purge error = productmetrics: disable-write-failed, want class "storage-failure"
The fix moves arming to the beforeDisableUploaderLock hook (control.go:208),
which runs once, straight-line, after beginDisableAtRoot has fully returned and
immediately before lockUploader blocks on the barrier — so the new arming point
strictly dominates the old one. Production classification logic is correct and
is untouched.
NOTE FOR TRIAGE: the bead's stated cause (deterministic assertion mismatch /
fork-shed artifact) was wrong. This is a real race in the test's own arming
logic, which is why it reds on loaded CI runners but passes locally.
VERIFICATION: go vet ./internal/productmetrics/ clean with and without
-tags integration; go vet ./... clean repo-wide; go test -tags integration
./internal/productmetrics/ green (12.5s, the CI shard's own scope); target test
consecutive runs green under -race. Working tree clean, one commit.
CAUSE: scripts/test-local-parallel does a second, inner
env -iper shard(~line 189) whose allowlist forwards HOME but omits GIT_CONFIG_GLOBAL /
GIT_CONFIG_SYSTEM, discarding the Makefile's ISOLATED_GITCONFIG isolation
(Makefile:361-370). Shards then read the host ~/.gitconfig
(commit.gpgsign=true, gpg.format=ssh) with no SSH_AUTH_SOCK, so every fixture
that execs
git commitdies with "Couldn't get agent socket?" /"fatal: failed to write commit object". 11 packages failed; NOT
internal/productmetrics.
PROOF OF PRE-EXISTENCE: on CLEAN MAIN c057108 (rig checkout, zero polecat
commits), examples/gastown TestWorktreeSetupKeepsIgnoresLocal FAILS under the
inner allowlist and PASSES under the outer one — isolating the single
differing variable to GIT_CONFIG_GLOBAL. CI runners have no signing config,
so this cannot reproduce in CI.
GATE INTEGRITY: --no-verify also skips the bead-ownership guard
(scripts/push-ownership-guard.sh). I did NOT skip it — I sourced it and ran
assert_bead_still_claimed standalone from the worktree immediately before the
push: exit 0. Push then verified: origin/polecat/gc-i8xf9 == local HEAD
dd73f11.
PRECEDENT: gc-xtr5y records this rig resolving this exact tension the same way
(gc-z0vi2.1 pushed with --no-verify after verifying diff-touched packages
green and classifying the remainder as this environmental bug).
--- refinery lx-r8kx 2026-07-28T16:35Z: merge-push step, PRE-OPEN codex gate ---
Branch polecat/gc-i8xf9 @ dd73f11; base = origin/main c057108 (branch is
1 ahead / 0 behind -> already rebased, fast-forwardable). Scope: 1 file,
internal/productmetrics/control_unix_test.go, +9 lines (TEST-ONLY).
Local quality gates (wisp shipped no gate commands -> CLAUDE.md fallback), all
run at the branch tip:
go build ./... GREEN (exit 0)
go vet ./... GREEN (exit 0)
go test ./internal/productmetrics/... (full package) GREEN (9.0s)
-run TestDisableAndPurgeRejectsUnprovenPeerSuccessor -v PASS (all 3 subtests,
incl. peer_successor_root_sync_failure)
IMPORTANT / honest scope of this verification: the CI red does NOT reproduce
locally. The same test also PASSES at main (dd73f11^ == c057108), including
30/30 iterations under -race with zero failures and no DATA RACE. So this local
run canNOT confirm the fix resolves the CI/required red -- the red is
environment-specific to CI (containerized fs / cross-device / root-sync paths,
per the subtest names) or load-dependent. What the local gates DO establish:
the branch builds, vets, and introduces no regression.
Verification of the actual CI fix is deferred to the PR's own CI run, which is
the correct mechanism in mr mode. Do not read "gates green" as "CI red fixed".
Parked in the pre-open codex gate: merge_result=pre_open_gate, assignee cleared,
gc.routed_to cleared, branch + target=main retained, NO pr_url/pr_number.
Codex signoff dispatched as review bead gc-1tcyb (task_kind=review,
anchor_bead=gc-i8xf9, review_branch=polecat/gc-i8xf9, review_base=main,
routed to gascity/gc-toolkit.polecat-codex). pre-open-resolve.sh opens the
non-draft PR once check.codex=green@dd73f11 is stamped on this anchor.
Refinery handoff
gc-i8xf9(bug, P2)polecat/gc-i8xf9maindd73f112; PR opened codex-green.