fix(test): productmetrics hermeticity — stop asserting against the host's metrics preferences - #125
Merged
Merged
Conversation
…it for exit TestInitFromWithoutHostedPreservesTemplate (and four siblings) run the real init bootstrap, which starts a real dolt sql-server for the new city — and never stopped it. The package-level leak guard then failed the ENTIRE cmd/gc job. DETERMINISTICALLY: reproduced in isolation at load 9, every run. This single test blocked three consecutive pre-push runs on 2026-08-06 while masquerading as load-dependent infrastructure flakiness. Each city-creating test now registers cleanupManagedDoltForTest, which calls the production stopManagedDoltProcess(cityPath, "") — SIGTERM, bounded grace, SIGKILL, and a WAIT for actual process exit. Port "" resolves the managed PID from the city's own runtime layout, and a city whose init never reached the dolt bootstrap reports HadPID=false, so registering before the init runs is safe everywhere. Red first: the exact single-test invocation failed on the leak guard before this change and passes after it. Full cmd/gc package green (554s).
…preferences
TestOpenProductionAndPreparationAreLazyAndNonCreating asserts Status ==
(pending-notice, preference-unset) — which presupposes no environment-level
metrics opt-out. On any machine where the operator has set
GC_DISABLE_USAGE_METRICS=1 or DO_NOT_TRACK=1 (a perfectly reasonable default
for a dev or fleet box), the service correctly reports
(environment-disabled, gc-disable-usage-metrics) and the test fails — it is
asserting against the HOST'S PREFERENCES, not the code. Reproduced: the
package is red on this fleet host (which exports GC_DISABLE_USAGE_METRICS=1)
and green after the scrub; red/green verified both ways. TestMain unsets both
kill switches, checked and fatal on failure; tests that exercise the switches
set them explicitly via t.Setenv, which still works after the scrub.
DELIBERATELY NOT INCLUDED: a fix for the one observed CI flake of
TestRecordOnceFreshQuotaBootstrapNeverReplacesDestinationRace ("RecordOnce =
dropped, want stored replay", one occurrence; main's CI otherwise green). A
frozen-clock fix was built on the 50ms-decision-budget theory and probe-tested
before shipping: rename-only delay passes both with AND without it; every-step
delay fails both with and without it. It demonstrably changed nothing, the
true mechanism involves a real-time source other than deps.now that was not
identified, and an unproven "fix" would bury the signal. Probe matrix recorded
here for whoever picks it up when it next fires.
(The init-test dolt teardown fix is cherry-picked alongside because the local
pre-push gate cannot pass on any branch lacking it — it is the same patch as
on gc/vp-ia76 and will merge trivially with PR #124.)
…the scrub removed The teardown helper is deleted in favour of the package's hermetic idiom (GC_BEADS=file + GC_DOLT=skip) on the one local-bootstrap test: no spawn, no leak, nothing to reap. Hosted siblings verifiably need the bd provider (--dolt-host validation) and spawn no local server. Red-first: reverting the idiom brings the leak-guard failure back on the exact single-test run. The TestMain scrub's comment was false — kill-switch tests inject a fake getenv, not t.Setenv — so the accidental real-env coverage the scrub removed (an opted-out dev box going red) is restored deliberately: TestKillSwitchesDisableThroughTheRealEnvironment sets each switch in the REAL environment and drives OpenProduction's default deps end to end. Red-first: stubbing the default getenv wiring fails 4 tests. Darwin trap documented in the test: the trust inspection rejects a home with a symlinked ancestor, so TMPDIR (/var -> /private/var) must be canonicalized. Also: scrub uses the package env constants (literal drift hazard); the unreachable os.Unsetenv panic guard is an ignore with rationale at the line.
bourgois
added a commit
that referenced
this pull request
Aug 11, 2026
Three findings from the PR #124 review, all verified before fixing: * THE ONCE-GUARD COULD BE BURNED BY A FALSE ALARM. warnSessionStorageUnsupported was once-per-process, and the first (benign) alarm for beadPolicyGraphStore consumed it — so a later, genuinely incapable chain of a DIFFERENT store type wrote sessions to the committed table with no warning at all: a silent failure inside the warning that exists to prevent silent failure. Now once per store TYPE (bounded noise, never mutes a new offender). * THE FALSE ALARM ITSELF. beadPolicyStore applies the session storage policy in its own Create (policyForCreate -> createWithStoragePolicy) but deliberately does not forward CreateWithStorage, so the front door's capability probe misread it as incapable. It now declares AppliesBeadStoragePolicy(), and the front door recognizes the structural marker and creates quietly through it — verified equivalent on the live fleet (27 sessions -> wisps, 0 -> issues, all via this wrapper). * STALE DOC: createSessionBead's comment still described CachingStore's OLD silently-degrading fallback, which commit 2 of this PR replaced with class-stamping. Reworded to past tense. Also assessed, no change: the new error returns on CachingStore's incapable fallback (both-flags-preset, unknown class) fire only on contradictory input the capable path already rejected — refusing loudly beats persisting garbage. A fourth finding in the original round-2 commit (deduplicating the init-test teardown onto cleanupManagedDoltTestCity) is dropped: PR #125 fixed the same init-test leak on main with the hermetic GC_BEADS/GC_DOLT idiom, and cmd/gc/init_from_hosted_dolt_test.go now matches main exactly. Mutations, each confirmed applied: dropping the self-applying branch -> 1 failure; reverting to a process-wide guard -> 1 failure. Suites: session, beads, and the original leak single-test all green.
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.
What
Two test-quality fixes that make
mainbuildable and pushable from any developer or fleet machine:1.
internal/productmetricshermeticity.TestOpenProductionAndPreparationAreLazyAndNonCreatingassertsStatus == (pending-notice, preference-unset)— which presupposes no environment-level metrics opt-out. On any machine withGC_DISABLE_USAGE_METRICS=1orDO_NOT_TRACK=1(a reasonable default for a dev or fleet box), the service correctly reports(environment-disabled, gc-disable-usage-metrics)and the test fails: it asserts against the host's preferences, not the code. ATestMainnow scrubs both kill switches before any test runs (checked, fatal on failure). Tests exercising the switches set them viat.Setenv, which still works after the scrub. Red→green verified both directions on a host with the variable set.2. Init tests stop the dolt server they start. Same patch as in PR #124 (included because the local pre-push gate cannot pass on any branch without it; merges trivially with #124): the five city-creating init tests spawned a real managed dolt sql-server via the real bootstrap and never stopped it, deterministically failing the package leak guard.
Deliberately not included
A fix for the one observed CI flake of
TestRecordOnceFreshQuotaBootstrapNeverReplacesDestinationRace("RecordOnce = dropped, want stored replay", one occurrence;main's CI otherwise green). A frozen-clock fix was built on the 50ms decision-budget theory and probe-tested before shipping — it failed its own verification: rename-only injected delay passes both with and without it; every-step delay fails both with and without it (and takes a sibling subtest down, "load quota: absent"). The true mechanism involves a real-time source other thandeps.nowthat was not identified. The probe matrix is in the commit message for whoever picks it up when it next fires; shipping an unproven fix would only bury the signal.Base note
Branched from the ls-remote-verified
mainsha (7a34a0f30), not local tracking refs — this shared clone's refs are mutated by concurrently-working fleet agents.