Repository navigation
internal/store: TestBreakingAStaleLockIsSerialised is load-dependent — flaked on macos-latest CI #255
Description
Activity
- added a commit that references this issue
on Aug 31, 2026 hi, this is Mycroft, Anton's synthetic cofounder. you asked for the flake on command before anyone judges a fix, which is the only part of a flake report worth having, so here it is.
it reproduces, and the message is yours verbatim. 120 acquires against a freshly planted 31s-old lock,
lockTimeoutat the 50msimpatient()pins, with 64 goroutines spinning to keep the scheduler honest:2/120failed withprocoder: could not lock .procoder/state/dispatch.json within 50ms - the write was NOT made. darwin/amd64, go1.26.7, at 29ffccb. the burn loop is what makes it a command rather than a wish.your suspect is the budget, and the budget is real, but the mechanism has a sharper name:
acquirethrows away a break it already won.the path is right here:
lastErr = err if !os.IsExist(err) || !breakStale(p, rel) { time.Sleep(lockRetry) } if time.Now().After(deadline) { // <- consulted BEFORE the freed path is retried
when
breakStalereturns true the lock file is gone. nobody holds it, and this caller is the only one entitled to it right then. the code skips the sleep, correctly, and then goes and asks the clock instead of asking the filesystem. one preemption anywhere in that window - the failedO_EXCL, the break fileO_EXCL, the stat, the remove, theremoveIfSame- and the caller is told it could not lock a path that is, at that instant, free.that also explains why the CI error was the plain-contention variant with no
(%v)cause:lastErrwas still theIsExistfrom the create, so the message says contention while the break had already succeeded.deterministic proof, no load needed. plant the stale lock, set
lockTimeoutto1 * time.Nanosecondso the deadline is guaranteed past by the time the break returns, callLock, and then stat the lock file. on main it fails withcould not lock ... within 1nsand the lock file is gone - the two facts cannot both be right. that is the whole bug in one assertion, and it is the assertion i would want pinned, because it is about the code rather than about how busy the runner was.so i think your direction (b) is the one, and (a) would only move the flake. expressing the budget in
lockRetrymultiples buys headroom; it does not stopacquirefrom discarding a won break, it just makes the window smaller. a prototype of (b) that turned both of the above green: factor the create-write-stat-keepAliveblock into atryCreate(p, rel) (held, error, error), thenif os.IsExist(err) && breakStale(p, rel) { if h, cerr, fatal := tryCreate(p, rel); cerr == nil { return h, nil } else if fatal != nil { return held{}, fatal } // somebody took it in the gap: ordinary contention, deadline judges it } else { time.Sleep(lockRetry) } if time.Now().After(deadline) {
the deadline still runs every iteration, so it is bounded exactly as before. the 1ns test stops being a failure and becomes an acquisition, which is the honest outcome: a caller that broke the lock gets it.
120/120under the same load. andgo test ./internal/store/ -race -count=20stays green, including the first half ofTestBreakingAStaleLockIsSerialised- the break file still blocks, so the serialisation invariant is untouched.what i deliberately did not do: touch
impatient(). if the post-break retry is immediate then 50ms is judging contention, which is the claim the test means to make, and shortening or lengthening it is a separate decision that is yours.happy to open a PR with the repro as a regression test if you want it, or to leave it here if you would rather write the fix yourself - the repro is the part worth keeping either way.
unrelated, since i had the suite running:
TestPiAdapterInjectsContractOnceininternal/portabilityfails for me at 29ffccb on a clean tree too (AGENTS.md was injected although pi had already loaded it (1 copies of its marker), pi_test.go:469). i checked it against main with no patch applied, so it is not from anything above. could well be macOS-only on my side - saying it rather than sitting on it.- added a commit that references this issue
on Sep 1, 2026
Failed once on the macos-latest leg of PR #253 (a RUFF pin bump that cannot reach internal/store), then passed on a re-run of the same commit:
--- FAIL: TestBreakingAStaleLockIsSerialised (1.52s)
lock_test.go:335: the stale lock was not breakable once the break file went: procoder: could not lock .procoder/state/dispatch.json within 50ms — the write was NOT made
Suspect is the test budget, not the code: impatient() pins lockTimeout at a flat 50ms, and the second half of the test asserts one full acquire succeeds inside that window after real work — a failed O_EXCL create, the break-file O_EXCL, a stat, a remove, a re-create. On a quiet machine that is microseconds; on a loaded runner the assertion is really 'the scheduler gave us 50ms of continuity', a different claim than the one the test means to make.
Direction (a decision, not implied by this issue): either express the budget as a floor in lockRetry multiples so it scales with whatever the loop actually waits, or make acquire's post-break retry provably immediate and assert that. A re-run passing is not a fix: the flake should be reproducible on command before the fix is judged.