Repository navigation
fix(compaction): synchronize SetOnCompactionComplete with running jobs - #496
Conversation
#351) SetOnCompactionComplete wrote Manager.onCompactionComplete without holding m.mu, while a completing compaction job read the same field — a data race under the Go memory model. The window is real in our wiring: main.go starts the compaction schedulers before wiring the callback, so a job finishing in that gap races the write. The setter now takes m.mu, and CompactPartition copies the callback while it already holds m.mu for the metrics update, still invoking it outside the lock (unchanged semantics). Adds a -race regression test verified to fail with the setter's lock removed.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves a potential data race by synchronizing the registration of the post-compaction cache-invalidation callback with running compaction jobs. Specifically, SetOnCompactionComplete now acquires the manager's mutex before writing the callback, and CompactPartition copies the callback to a local variable while holding the mutex before invoking it. A concurrent test has also been added to verify this behavior. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Code Review
This pull request resolves a data race issue (#351) by synchronizing the registration of the post-compaction cache-invalidation callback with running compaction jobs. Specifically, SetOnCompactionComplete now acquires the manager's mutex lock, and CompactPartition copies the callback to a local variable while holding the lock before executing it. A concurrent test has also been added to verify this behavior and prevent regressions. There are no review comments to address, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
1 participant
Closes #351.
Summary
SetOnCompactionCompletewroteManager.onCompactionCompleteunsynchronized while a completing compaction job could read it concurrently — a data race under the Go memory model. The window is real:main.gostarts the compaction schedulers (~line 1146/1174) before wiring the callback (~line 2067).m.mu;CompactPartitioncopies the callback while it already holdsm.mufor the metrics/history update, then invokes it outside the lock as before — no behavior change, no new lock ordering (the setter's only caller holds no locks, and the callback body never re-enters the Manager).TestSetOnCompactionCompleteConcurrent, a-raceregression test verified to fail when the setter's lock is removed.RELEASE_NOTES_2026.06.2.md(Bug fixes).Test plan
go build ./cmd/... ./internal/...go vet ./internal/compaction/andgofmt -lclean on touched filesgo test -race ./internal/compaction/ -count=1passes-racewith the setter's lock removed, PASS with the fix🤖 Generated with Claude Code