Fix the write/write race on chunk.Owner - #290
Open
midwell wants to merge 1 commit into
Open
Conversation
Two goroutines write the owner of the same chunk with no synchronisation, and the
second one is triggered by the first:
- claimChunk records this pod once the claim succeeds (claim.go:46-47);
- iterateChangeStream records the owner from the update that the claim just
made, since it is watching that collection (updates.go:183-185).
The same fields are read from two more places: scanChunk checks whether it still
owns the chunk (scan.go:18), and FindOwnerInt32ID hands the owner to the caller.
That last path leaks the race past the package boundary. GetOwner returned
&c.Owner, so although FindOwnerInt32ID holds globalChunkTblMutex while calling
it, the caller dereferences the pointer after that lock has been released, while
both writers are still running. The visible effect is a torn PodId: a PodName
belonging to one pod paired with another pod's PodIp, which points the caller at
the wrong pod. The chunk_test.go assertion reproduces exactly that with the new
locks removed, without needing the race detector:
torn owner: PodName "pod-x" with PodIp "10.0.0.22", want "10.0.0.11"
Owner is now guarded by a mutex on the chunk itself rather than on Drsm, because
these are chunk methods and GetOwner has no Drsm to reach a shared lock through.
The mutex is a leaf: none of the four helpers calls anything else, so the only
nesting is globalChunkTblMutex then ownerMutex in FindOwnerInt32ID, and nothing
takes them the other way round.
GetOwner keeps its signature and returns a pointer to a copy, so callers holding
the result are unaffected by later writes and no consumer has to change.
setOwnerAddress deliberately leaves PodInstance alone, which is what claimChunk
has always done - the change-stream update that the claim triggers is what fills
it in. That is preserved rather than fixed here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
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
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.
The race
Two goroutines write the owner of the same chunk with no synchronisation, and the second is
triggered by the first:
claimChunkrecords this pod once the claim succeeds (claim.go:46-47);iterateChangeStreamrecords the owner from the update that the claim just made, becauseit is watching that collection (
updates.go:183-185).The same fields are read from two more places:
scanChunkchecks whether it still owns thechunk (
scan.go:18), andFindOwnerInt32IDhands the owner to the caller.That last path leaks the race past the package boundary.
GetOwnerreturned&c.Owner, soalthough
FindOwnerInt32IDholdsglobalChunkTblMutexwhile calling it, the callerdereferences the pointer after that lock has been released, while both writers are still
running.
Why it matters
The visible effect is a torn
PodId— aPodNamebelonging to one pod paired with anotherpod's
PodIp— so the caller is pointed at the wrong pod. It is not a crash, which makes itthe quieter and more awkward of the two failure modes:
concurrent map writesat leastannounces itself.
This is demonstrable rather than theoretical. With the new locks removed, the assertion in
chunk_test.gocatches it without the race detector, in 2 of 3 runs:The fix
Owneris guarded by a mutex on thechunkitself rather than onDrsm, because these arechunkmethods andGetOwnerhas noDrsmthrough which to reach a shared lock.globalChunkTblMutexthenownerMutex, inFindOwnerInt32ID, and nothing takes them inthe other order.
GetOwnerkeeps its signature and returns a pointer to a copy, so callers holding theresult are unaffected by later writes and no consumer needs to change.
setOwnerAddressdeliberately leavesPodInstancealone, which is whatclaimChunkhasalways done — the change-stream update that the claim triggers is what supplies it. That
behaviour is preserved here rather than quietly changed.
Testing
drsm/chunk_test.godrives both writers against both readers and asserts thePodName/PodIppairing, so it fails on a torn read even in a non-race build. Mutation-verified: itfails with the locks stripped and passes with them in.
A throwaway harness (not included — it needs a MongoDB replica set, which CI has no service
for) also ran
InitDRSMagainst mongo 7.0 with the full goroutine set, a peer pod owning 8chunks and a real keepalive deletion to fire the pod-down path. Under
-raceit reportedthis
Ownerpair before the change and nothing after it, with liveness intact: no stall, all8 peer chunks reclaimed, ~5.5M allocate/release round-trips.
Relationship to #289
Branched from
main, so it is independent of #289 and can be reviewed on its own. The two dotouch adjacent lines in
updates.goand conflict in exactly one place; I have merged themlocally and the resolution is one hunk (take
setOwnerfrom here andrecordChunkOwnerfrom#289). Happy to rebase whichever lands second. With both applied, the harness above reports
zero races.
Not in this change
chunk.FreeIdsandchunk.ScanIdsare also raced, and it is a third distinct defect:scanChunkappends to and truncates them with no lock (scan.go:37-43) whileAllocateIntID/ReleaseIntIDtouch the same slices under the package-levelmutex(
chunk.go:112-123). It is reachable by design, sinceReleaseInt32IDlooks upscanChunksspecifically so an id belonging to a chunk still being scanned can be released. Confirmed
live with the harness above once the releases were spread across the run so they overlapped
scanChunk's 5s ticks:A torn slice header there means a duplicated or lost id, i.e. handing out an identifier that
is still in use. I would rather send that as its own change than widen this one, but say if
you would prefer it folded in.