Fix races: relocate signal-cli tmp root, join bridge goroutines on Close#30
Merged
Conversation
Two races surfaced by the Go Race CI job after #28: - The per-run temp dirs lived under the Signal config dir, so removing that dir (Unpair's RemoveAll, t.TempDir cleanup) raced with a lingering invocation still creating its run dir inside and failed with "directory not empty". The run-tmp root now lives in an app-owned, per-uid directory under the system temp dir, which no teardown path removes wholesale. - Bridge.Close cancelled background contexts but never waited, so package state reused after Close (tests swapping the runSignalCLI stub between runs, serve restarting a bridge) raced with loops still draining. Background goroutines are now tracked in a WaitGroup; Close joins them, and Unpair joins before removing the config dir a draining poll may still be writing WAL/recovery files into. TestConnectEmitsSignalQRCodeAndStoresPairedAccount previously failed under -race roughly 1 in 20 runs; 30 consecutive runs now pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jun 13, 2026
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
The Go Race CI job on main failed after #28 (
TestConnectEmitsSignalQRCodeAndStoresPairedAccount: "TempDir RemoveAll cleanup: directory not empty"). Investigating reproduced two distinct races:Unpair→os.RemoveAll(configDir),t.TempDir()cleanup) races with a lingering invocation'sMkdirTemp. The root now lives atos.TempDir()/openmessage-signal-cli-<uid>— app-owned, per-user, outside every wholesale-removal path. All Fix libsignal temp dir leak from signal-cli subprocess churn #28 guarantees hold (per-run dir removed on process exit, startup + periodic sweeps, legacylibsignal*reaping,OPENMESSAGES_SIGNAL_TMP_SWEEP=0opt-out).Bridge.Closecancelled contexts but never waited for the link/receive/refresh/sweep/sync goroutines, so reusing package state after Close (tests swapping therunSignalCLIstub between-countruns; serve restarting a bridge) raced with loops still draining. Goroutines are now tracked in a WaitGroup;Closejoins, andUnpairjoins beforeRemoveAll(configDir)— a draining poll can still be writing WAL/recovery files inside it.Verification
go test ./internal/signallive -race -run TestConnectEmitsSignalQRCode -count=30: all pass (previously ~1/20 failure)go test ./internal/signallive -race -count=3,go vet ./..., fullgo test ./...: green🤖 Generated with Claude Code