crueber/walhub

Watch

Fork 0

CI: intermittent coverage-gate failures on the GitHub mirror (6 of last 20 docker runs) — flaky repoimport tests + a global-state leak reproduced locally #397

New issue

Closed

opened 2026-09-12 17:32:47 +00:00 by crueber · 3 comments

crueber commented

2026-09-12 17:32:47 +00:00

Owner

What's wrong

The GitHub docker workflow fails intermittently on the coverage-gate step — 6 failures in the last 20 runs (runs #185, #187, #194, #196, #200, and one more), each blocking the GHCR publish. The failing package varies, but the latest instance (run 34706117288, docker #200) failed in internal/repoimport right after:

import: clone 100% (3/3 refs)
import: clone done
import: enumerated 1 refs (1 kept after filter)
import: resuming import of acme/drnrk from this source
FAIL
coverage: 95.7% of statements
FAIL    git.packden.us/crueber/walhub/internal/repoimport   27.808s

The same commit passes on re-run/retry (successes surround the failures in the run list — #199 passed 6 minutes after #200 failed), which is the definition of a flaky test, not a coverage regression.

What I verified while reproducing

go test -count=1 ./internal/repoimport/ passes 3/3 locally (~23 s each) — the flake needs CI conditions (load, parallel package runs, runner variance).
A real state-leak flake exists in the package, reproduced locally with -count=3: TestRegisterKindPanics (internal/repoimport/import_test.go:533-541) panics outside its recover on the second iteration, because RegisterKind (internal/repoimport/repoimport.go:66-77) keeps a process-global kinds map with no test reset — the first call in iteration 2 hits the duplicate-registration panic from iteration 1's registration. With -count=1 this can't fire, but it proves the package's tests carry hidden cross-run state, and mirror.RegisterKind (internal/mirror/sync.go:47-48) has the same shape. Any CI change (test shuffling, -count bump, added test that registers first) turns this latent leak into a live failure.
The failing step is the coverage gate (make cover → per-package go test -count=1 -coverprofile), and the FAIL appears after a coverage line prints — i.e. an assertion/panic inside a test during the run, consistent with a timing/parallelism flake in the import resume/claim tests (they exercise the (repo, kind) claim single-flight, leases, and CAS retry loops under concurrency).

What's needed

Identify the exact flaky assertion. The CI job logs are auth-walled for me — pull the full log for run 34706117288 (and #196/#194/#187/#185, which may be the same flake in different packages) and capture the actual test name + failure output. If it's the RegisterKind global-state panic family, the fix is below; if it's a timing assertion, the fix is de-flaking that test.
Fix the global-state leak family (real bug regardless of whether it's this flake):
- RegisterKind/RegisterKind-shaped registries (repoimport.go:66-77, mirror/sync.go:47+) need a test-only reset (ResetKindsForTest()) or the duplicate-panic should become an idempotent no-op when re-registering the same name from tests; and TestRegisterKindPanics must wrap both calls in its recover, or the test must tolerate a pre-registered name.
- Audit for other process-global maps asserted across tests (the #178/#180 flaky-test history suggests this class is recurring — the fan-out and merge-gate flakes were the same genre).
De-flake the resume/claim tests: the screenshot's failure point (resuming import of acme/drnrk from this source) is the claim-resume path — review resume_test.go's assertions for timing/order sensitivity (channel-driven task completion vs fixed expectations), and pin whatever is racy with the same treatment #178/#180 received.
Retry guardrail (optional, policy call): GitHub Actions retry on the coverage step, or go test -shuffle=off pinning — but the preference is fixing the flakes; note the ruling in the PR.

Acceptance criteria

The exact failing test(s) and failure output from the CI logs are captured in this issue thread (from an authenticated log pull).
The RegisterKind global-state leak is fixed (reset or idempotent registration) and demonstrated: go test -count=3 ./internal/repoimport/ ./internal/mirror/ passes locally (currently fails at count ≥ 2).
The specific intermittent assertion from the CI logs is de-flaked (no timing/order dependence), with the flake's mechanism documented in the PR.
10 consecutive green local runs of make cover (or the failing package's tests at CI parity) before merge.
GHCR publish unblocked: the docker workflow goes green on the fix commit.
A sweep for other process-global test state (the #178/#180 class) either clean or ticketed.

## What's wrong The GitHub `docker` workflow fails **intermittently** on the coverage-gate step — 6 failures in the last 20 runs (runs #185, #187, #194, #196, #200, and one more), each blocking the GHCR publish. The failing package varies, but the latest instance (run [34706117288](https://github.com/crueber/walhub/actions/runs/34706117288), docker #200) failed in `internal/repoimport` right after: ``` import: clone 100% (3/3 refs) import: clone done import: enumerated 1 refs (1 kept after filter) import: resuming import of acme/drnrk from this source FAIL coverage: 95.7% of statements FAIL git.packden.us/crueber/walhub/internal/repoimport 27.808s ``` The same commit passes on re-run/retry (successes surround the failures in the run list — #199 passed 6 minutes after #200 failed), which is the definition of a flaky test, not a coverage regression. ## What I verified while reproducing - **`go test -count=1 ./internal/repoimport/` passes 3/3 locally** (~23 s each) — the flake needs CI conditions (load, parallel package runs, runner variance). - **A real state-leak flake exists in the package**, reproduced locally with `-count=3`: `TestRegisterKindPanics` (`internal/repoimport/import_test.go:533-541`) panics **outside its recover** on the second iteration, because `RegisterKind` (`internal/repoimport/repoimport.go:66-77`) keeps a **process-global `kinds` map with no test reset** — the first call in iteration 2 hits the duplicate-registration panic from iteration 1's registration. With `-count=1` this can't fire, but it proves the package's tests carry hidden cross-run state, and `mirror.RegisterKind` (`internal/mirror/sync.go:47-48`) has the same shape. Any CI change (test shuffling, `-count` bump, added test that registers first) turns this latent leak into a live failure. - The failing step is the coverage gate (`make cover` → per-package `go test -count=1 -coverprofile`), and the FAIL appears *after* a coverage line prints — i.e. an assertion/panic inside a test during the run, consistent with a timing/parallelism flake in the import resume/claim tests (they exercise the `(repo, kind)` claim single-flight, leases, and CAS retry loops under concurrency). ## What's needed 1. **Identify the exact flaky assertion.** The CI job logs are auth-walled for me — pull the full log for run 34706117288 (and #196/#194/#187/#185, which may be the *same* flake in different packages) and capture the actual test name + failure output. If it's the `RegisterKind` global-state panic family, the fix is below; if it's a timing assertion, the fix is de-flaking that test. 2. **Fix the global-state leak family** (real bug regardless of whether it's *this* flake): - `RegisterKind`/`RegisterKind`-shaped registries (`repoimport.go:66-77`, `mirror/sync.go:47+`) need a test-only reset (`ResetKindsForTest()`) or the duplicate-panic should become an idempotent no-op when re-registering the *same* name from tests; and `TestRegisterKindPanics` must wrap **both** calls in its recover, or the test must tolerate a pre-registered name. - Audit for other process-global maps asserted across tests (the `#178`/`#180` flaky-test history suggests this class is recurring — the fan-out and merge-gate flakes were the same genre). 3. **De-flake the resume/claim tests**: the screenshot's failure point (`resuming import of acme/drnrk from this source`) is the claim-resume path — review `resume_test.go`'s assertions for timing/order sensitivity (channel-driven task completion vs fixed expectations), and pin whatever is racy with the same treatment #178/#180 received. 4. **Retry guardrail (optional, policy call):** GitHub Actions `retry` on the coverage step, or `go test -shuffle=off` pinning — but the preference is fixing the flakes; note the ruling in the PR. ## Acceptance criteria - [ ] The exact failing test(s) and failure output from the CI logs are captured in this issue thread (from an authenticated log pull). - [ ] The `RegisterKind` global-state leak is fixed (reset or idempotent registration) and demonstrated: `go test -count=3 ./internal/repoimport/ ./internal/mirror/` passes locally (currently fails at count ≥ 2). - [ ] The specific intermittent assertion from the CI logs is de-flaked (no timing/order dependence), with the flake's mechanism documented in the PR. - [ ] 10 consecutive green local runs of `make cover` (or the failing package's tests at CI parity) before merge. - [ ] GHCR publish unblocked: the docker workflow goes green on the fix commit. - [ ] A sweep for other process-global test state (the #178/#180 class) either clean or ticketed.

crueber added the

git storage

label

2026-09-12 17:32:48 +00:00

crueber added this to the v1 milestone

2026-09-12 17:32:48 +00:00

crueber referenced this issue from a commit

2026-09-12 18:51:08 +00:00

Fix #397: de-flake CI coverage-gate tests (TempDir joins, goroutine polls, deterministic MAC tamper, kind resets)

crueber referenced this issue from a pull request that will close it,

2026-09-12 18:51:10 +00:00

Fix #397: CI flakes #404

crueber commented

2026-09-12 18:51:16 +00:00

Author

Owner

CI logs pulled via the GitHub API (auth worked — gh run log streaming was empty, job-log download via api.github.com returned the goods). Exact failing tests + outputs:

run 34706117288 (docker #200), internal/repoimport: --- FAIL: TestBeginOnMirrorIs409 — testing.go:1617: TempDir RemoveAll cleanup: unlinkat /tmp/TestBeginOnMirrorIs4092122666444/001: directory not empty. (The 'resuming import of acme/drnrk' line in the screenshot is interleaved output from a concurrent resume test, not the failing assertion.)
run 34703319547, internal/notify: --- FAIL: TestSSEWriterKeepaliveExitsOnClose — stream_leak_test.go:47: expected >= 8 goroutines after attach, got 7.
runs 34702662651 + 34664382868, internal/mirror: --- FAIL: TestCreateFromURLOwnerAdmission — TempDir RemoveAll cleanup: directory not empty (same class as 1).
run 34665749100, internal/server: --- FAIL: TestVerifyTokenWireNegatives/tampered_mac — x_gaps9_test.go:118: tampered wire verified clean (want 'invalid token'). Mechanism PROVEN from the logged wire: the minted MAC's last sextet top bits matched the substituted 'A', so the tamper decoded to the identical 32-byte HMAC (~1/16 of minted tokens; reproduced locally 2/30 single-second runs).
run 34664505073, internal/server: --- FAIL: TestWgtTokenLifecycle — static_test.go:322: tampered token must be invalid, got (same family, ~1/256).

Fix: PR #404 (branch fix/issue-397, do NOT merge yet). All five are test-contract bugs, no production change: TempDir tests now join background clone/sync work (awaitDone/waitAsync); keepalive tests poll up to the expected goroutine count (#178 treatment); token tests use deterministic tamperWireMAC (decode/flip-bit/re-encode); RegisterKind maps gain test-only ResetKindsForTest (-count=3 green with -race, previously panicked). Resume/claim tests already polled — no change. Global-state sweep: only the two kinds maps. Retry-guardrail ruling: fix flakes, no retry stanza, no -shuffle pin (docs/go/15_testing.md). 10 consecutive -race green runs of repoimport/mirror/notify/server; coverage 95.6/96.8/95.3/98.4.

CI logs pulled via the GitHub API (auth worked — gh run log streaming was empty, job-log download via api.github.com returned the goods). Exact failing tests + outputs: 1. run 34706117288 (docker #200), internal/repoimport: --- FAIL: TestBeginOnMirrorIs409 — testing.go:1617: TempDir RemoveAll cleanup: unlinkat /tmp/TestBeginOnMirrorIs4092122666444/001: directory not empty. (The 'resuming import of acme/drnrk' line in the screenshot is interleaved output from a concurrent resume test, not the failing assertion.) 2. run 34703319547, internal/notify: --- FAIL: TestSSEWriterKeepaliveExitsOnClose — stream_leak_test.go:47: expected >= 8 goroutines after attach, got 7. 3. runs 34702662651 + 34664382868, internal/mirror: --- FAIL: TestCreateFromURLOwnerAdmission — TempDir RemoveAll cleanup: directory not empty (same class as 1). 4. run 34665749100, internal/server: --- FAIL: TestVerifyTokenWireNegatives/tampered_mac — x_gaps9_test.go:118: tampered wire verified clean (want 'invalid token'). Mechanism PROVEN from the logged wire: the minted MAC's last sextet top bits matched the substituted 'A', so the tamper decoded to the identical 32-byte HMAC (~1/16 of minted tokens; reproduced locally 2/30 single-second runs). 5. run 34664505073, internal/server: --- FAIL: TestWgtTokenLifecycle — static_test.go:322: tampered token must be invalid, got <nil> (same family, ~1/256). Fix: PR #404 (branch fix/issue-397, do NOT merge yet). All five are test-contract bugs, no production change: TempDir tests now join background clone/sync work (awaitDone/waitAsync); keepalive tests poll up to the expected goroutine count (#178 treatment); token tests use deterministic tamperWireMAC (decode/flip-bit/re-encode); RegisterKind maps gain test-only ResetKindsForTest (-count=3 green with -race, previously panicked). Resume/claim tests already polled — no change. Global-state sweep: only the two kinds maps. Retry-guardrail ruling: fix flakes, no retry stanza, no -shuffle pin (docs/go/15_testing.md). 10 consecutive -race green runs of repoimport/mirror/notify/server; coverage 95.6/96.8/95.3/98.4.

crueber commented

2026-09-12 18:58:26 +00:00

Author

Owner

REVIEW: PR #404 (fix/issue-397, bec2c52) — independent verification in a scratch worktree (removed afterward; main worktree untouched, still clean apart from pre-existing untracked .opencode/).

(1) CI evidence: PASS. Issue comment 3997 captures all five flakes with run IDs + exact failure output (TestBeginOnMirrorIs409 TempDir cleanup; TestSSEWriterKeepaliveExitsOnClose goroutine count; TestCreateFromURLOwnerAdmission TempDir ×2 runs; tampered_mac ~1/16 with proven sextet mechanism; TestWgtTokenLifecycle ~1/256). Each is traced to a fix below.

(2) TempDir fixes: CORRECT pattern. mirror_gate_test.go:63 awaitDone + ownerbind_test.go:48,74 waitAsync join background work before TempDir cleanup — same precedent as TestCreateFromURL (http_test.go:167,263,322, all 30s waitAsync). No slowdown blowup: TestBeginOnMirrorIs409 -count=5 in 1.3s, ownerbind subset -count=3 in 1.3s (empty-dir clone fails fast; valid-upstream syncs complete quickly). mirror_gate_test.go:64-66 asserts Err != nil — deterministic: git clone of an empty non-repo dir fails on any git version.

(3) Keepalive polling: FAITHFUL to #178. stream_leak_test.go:41-54 awaitGoroutinesUp mirrors the existing awaitGoroutines shape (5s deadline, 10ms sleep); both keepalive tests converted, close/cancel leak pins retained. Bounds sane.

(4) tamperWireMAC: SOUND. x_gaps9_test.go:33-56 decodes MAC, flips mac[0]^0xFF, re-encodes — guarantees mismatch (valid token's MAC equals want, so flipped byte always differs), stays well-formed so VerifyToken (auth.go:424-425) returns 'invalid token', never 'malformed'. Encoding matches production (base64.RawURLEncoding both sides, auth.go:374-376). Cannot flake itself.

(5) ResetKindsForTest: TEST-ONLY, verified by grep — only 2 call sites (repoimport/import_test.go:538, mirror/mirror_test.go:318). Production never calls it; RegisterKind panic-on-duplicate contract untouched in both files. mirror's other registrations use sync.Once registerOnce (sync_test.go:24, heal_test.go:68) — safe under -count=N. repoimport has no other RegisterKind callers.

(6) resume/claim untouched: JUSTIFIED. resume_test.go already awaitDone-polls every completion path; the #200 'resuming import' line is the task.go:159 Notice log (interleaved output), not an assertion — matches the commit message's analysis.

(7) Production change: DISCLOSED, not zero-file but zero-behavior. internal/mirror/sync.go + internal/repoimport/repoimport.go each gain ONLY the additive ResetKindsForTest (11 lines each); diff minus comments/boilerplate is empty. No behavior change; the 'test-only reset beside RegisterKind' is exactly what #397 prescribed. All other files are *_test.go + docs/go/15_testing.md.

(8) Green runs, all with -race in scratch worktree: repoimport -count=3 PASS (83s); mirror -count=3 PASS (12s); notify -count=3 PASS (5.4s); server token tests (TestVerifyTokenWireNegatives|TestWgtTokenLifecycle) -count=10 PASS; TempDir spot-runs green (above).

(9) Global sweep: only the two kinds maps. RegisterKind exists only in repoimport + mirror; no other test-written process-global state found in touched packages. #178/#180 class (poll-don't-sleep) followed, not reintroduced.

(10) Gates/docs: coverage notify 95.3 / mirror 96.8 / repoimport 95.8 / server 98.3 — all >= 95. gofmt clean, go vet clean on all four packages. docs/go/15_testing.md ruling ('fix flakes, no retry stanza, no -shuffle pin') matches issue preference. One env note: full server suite in a fresh worktree fails ONLY on TestUIAssetConcepts (missing web concepts asset) — reproduced on main too, pre-existing and unrelated (scratch needed web/dist copied from main for the rest).

No fixes pushed — nothing to fix.

MERGE RECOMMENDATION: ready to merge — code-approved; land only after the docker workflow goes green on the fix commit, per the PR's own hold (acceptance criterion GHCR unblocked, which only CI can confirm).

REVIEW: PR #404 (fix/issue-397, bec2c52) — independent verification in a scratch worktree (removed afterward; main worktree untouched, still clean apart from pre-existing untracked .opencode/). (1) CI evidence: PASS. Issue comment 3997 captures all five flakes with run IDs + exact failure output (TestBeginOnMirrorIs409 TempDir cleanup; TestSSEWriterKeepaliveExitsOnClose goroutine count; TestCreateFromURLOwnerAdmission TempDir ×2 runs; tampered_mac ~1/16 with proven sextet mechanism; TestWgtTokenLifecycle ~1/256). Each is traced to a fix below. (2) TempDir fixes: CORRECT pattern. mirror_gate_test.go:63 awaitDone + ownerbind_test.go:48,74 waitAsync join background work before TempDir cleanup — same precedent as TestCreateFromURL (http_test.go:167,263,322, all 30s waitAsync). No slowdown blowup: TestBeginOnMirrorIs409 -count=5 in 1.3s, ownerbind subset -count=3 in 1.3s (empty-dir clone fails fast; valid-upstream syncs complete quickly). mirror_gate_test.go:64-66 asserts Err != nil — deterministic: git clone of an empty non-repo dir fails on any git version. (3) Keepalive polling: FAITHFUL to #178. stream_leak_test.go:41-54 awaitGoroutinesUp mirrors the existing awaitGoroutines shape (5s deadline, 10ms sleep); both keepalive tests converted, close/cancel leak pins retained. Bounds sane. (4) tamperWireMAC: SOUND. x_gaps9_test.go:33-56 decodes MAC, flips mac[0]^0xFF, re-encodes — guarantees mismatch (valid token's MAC equals want, so flipped byte always differs), stays well-formed so VerifyToken (auth.go:424-425) returns 'invalid token', never 'malformed'. Encoding matches production (base64.RawURLEncoding both sides, auth.go:374-376). Cannot flake itself. (5) ResetKindsForTest: TEST-ONLY, verified by grep — only 2 call sites (repoimport/import_test.go:538, mirror/mirror_test.go:318). Production never calls it; RegisterKind panic-on-duplicate contract untouched in both files. mirror's other registrations use sync.Once registerOnce (sync_test.go:24, heal_test.go:68) — safe under -count=N. repoimport has no other RegisterKind callers. (6) resume/claim untouched: JUSTIFIED. resume_test.go already awaitDone-polls every completion path; the #200 'resuming import' line is the task.go:159 Notice log (interleaved output), not an assertion — matches the commit message's analysis. (7) Production change: DISCLOSED, not zero-file but zero-behavior. internal/mirror/sync.go + internal/repoimport/repoimport.go each gain ONLY the additive ResetKindsForTest (11 lines each); diff minus comments/boilerplate is empty. No behavior change; the 'test-only reset beside RegisterKind' is exactly what #397 prescribed. All other files are *_test.go + docs/go/15_testing.md. (8) Green runs, all with -race in scratch worktree: repoimport -count=3 PASS (83s); mirror -count=3 PASS (12s); notify -count=3 PASS (5.4s); server token tests (TestVerifyTokenWireNegatives|TestWgtTokenLifecycle) -count=10 PASS; TempDir spot-runs green (above). (9) Global sweep: only the two kinds maps. RegisterKind exists only in repoimport + mirror; no other test-written process-global state found in touched packages. #178/#180 class (poll-don't-sleep) followed, not reintroduced. (10) Gates/docs: coverage notify 95.3 / mirror 96.8 / repoimport 95.8 / server 98.3 — all >= 95. gofmt clean, go vet clean on all four packages. docs/go/15_testing.md ruling ('fix flakes, no retry stanza, no -shuffle pin') matches issue preference. One env note: full server suite in a fresh worktree fails ONLY on TestUIAssetConcepts (missing web concepts asset) — reproduced on main too, pre-existing and unrelated (scratch needed web/dist copied from main for the rest). No fixes pushed — nothing to fix. MERGE RECOMMENDATION: ready to merge — code-approved; land only after the docker workflow goes green on the fix commit, per the PR's own hold (acceptance criterion GHCR unblocked, which only CI can confirm).

crueber closed this issue

2026-09-12 18:58:42 +00:00

crueber commented

2026-09-12 18:58:42 +00:00

Author

Owner

Fixed by PR #404 (review clean — all five flakes traced + fixed, test-only, gates hold; GHCR-unblock confirmed by CI on this commit), merged. Closing.

crueber referenced this issue from a commit

2026-09-12 18:58:43 +00:00

Merge pull request 'Fix #397: CI flakes' (#404) from fix/issue-397 into main