Skip to content

[v0.1.x-branch] Backport #966: waved: fix mailbox-compat test tempdir cleanup flake under -race - #975

Merged
Roasbeef merged 1 commit into
v0.1.x-branchfrom
backport-966-to-v0.1.x-branch
Jul 17, 2026
Merged

[v0.1.x-branch] Backport #966: waved: fix mailbox-compat test tempdir cleanup flake under -race#975
Roasbeef merged 1 commit into
v0.1.x-branchfrom
backport-966-to-v0.1.x-branch

Conversation

@github-actions

Copy link
Copy Markdown

Backport of #966


Problem

The unit-race job intermittently fails with:

--- FAIL: TestStartMailboxIngressConcurrentIncompatible
    testing.go:1464: TempDir RemoveAll cleanup: unlinkat .../001: directory not empty

(seen e.g. in the run for #895, but the test and code are on main and the
flake is not reorg-related). Despite running under -race, this is not a
data-race report — Go's t.TempDir() calls t.Errorf when its RemoveAll
fails, which fails the test.

Root cause

TestStartMailboxIngressConcurrentIncompatible and
TestStartMailboxIngressAlreadyIncompatible start the durable serverconn
egress actor with StartEgress() but tore it down with a deferred,
fire-and-forget runtime.Stop(). DurableActor.Stop() only cancels the
actor context and returns immediately (a.stopOnce.Do(func(){ a.cancel() }));
only StopAndWait/Wait block on a.done.

So the teardown ordering was:

  1. defer Stop() — signals cancel, returns instantly
  2. t.Cleanup: DB.Close()
  3. t.Cleanup: os.RemoveAll(tempdir)

The egress goroutine kept issuing queries against the shared SQLite handle
during steps 2–3. An in-flight connection re-materializes the WAL/-shm
sidecar files, so RemoveAll's final rmdir races and fails with
directory not empty.

Fix

Switch both tests to StopAndWait(context.Background()) in the deferred
teardown so the egress goroutine has fully drained before the DB is closed
and the temp dir removed. Test-only change.

Verification

  • Before: -count=300 -cpu=4 -race failed dozens of times.
  • After: passes cleanly across 1000+ iterations.
  • go vet, make fmt-changed, make lint-changed-local (0 issues) all pass.

Note (separate, pre-existing)

While stressing this I observed a distinct, much rarer data race originating
at baselib/actor/durable_actor.go:508 (the egress worker goroutine) — ~1 in
several thousand iterations, identical on main and the reorg branches and
untouched by this change. It is orthogonal to the reported cleanup flake and
worth a separate investigation; I could not reliably reproduce it to capture a
full trace.

TestStartMailboxIngressConcurrentIncompatible (and its sibling) start
the durable serverconn egress actor with StartEgress but tore it down
with a deferred, fire-and-forget runtime.Stop(). Stop() only cancels the
actor context and returns immediately, so the egress goroutine could
keep issuing queries against the shared SQLite handle while the
t.Cleanup chain closed the DB and removed the temp dir. An in-flight
connection re-materializes the WAL/-shm sidecar files, so the tempdir
RemoveAll raced and intermittently failed with "directory not empty"
under -race.

Switch both tests to StopAndWait so the egress goroutine has fully
exited before cleanup runs. Reproduced reliably at -count=300 -cpu=4
before the change; passes cleanly across 1000+ iterations after.

(cherry picked from commit c112d2e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants