Skip to content

Fix the write/write race on chunk.Owner - #290

Open
midwell wants to merge 1 commit into
omec-project:mainfrom
midwell:fix-drsm-owner-race
Open

Fix the write/write race on chunk.Owner#290
midwell wants to merge 1 commit into
omec-project:mainfrom
midwell:fix-drsm-owner-race

Conversation

@midwell

@midwell midwell commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

The race

Two goroutines write the owner of the same chunk with no synchronisation, and the second 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, because
    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.

Why it matters

The visible effect is a torn PodId — a PodName belonging to one pod paired with another
pod's PodIp — so the caller is pointed at the wrong pod. It is not a crash, which makes it
the quieter and more awkward of the two failure modes: concurrent map writes at least
announces itself.

This is demonstrable rather than theoretical. With the new locks removed, the assertion in
chunk_test.go catches it without the race detector, in 2 of 3 runs:

--- FAIL: TestChunkOwnerConcurrentAccess
    chunk_test.go:69: torn owner: PodName "pod-x" with PodIp "10.0.0.22", want "10.0.0.11"

The fix

Owner is guarded by a mutex on the chunk itself rather than on Drsm, because these are
chunk methods and GetOwner has no Drsm through which to reach a shared lock.

  • The mutex is a leaf: none of the four helpers calls anything else. The only nesting is
    globalChunkTblMutex then ownerMutex, in FindOwnerInt32ID, and nothing takes them in
    the other order.
  • GetOwner keeps its signature and returns a pointer to a copy, so callers holding the
    result are unaffected by later writes and no consumer needs to change.
  • setOwnerAddress deliberately leaves PodInstance alone, which is what claimChunk has
    always 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.go drives both writers against both readers and asserts the PodName/
PodIp pairing, so it fails on a torn read even in a non-race build. Mutation-verified: it
fails 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 InitDRSM against mongo 7.0 with the full goroutine set, a peer pod owning 8
chunks and a real keepalive deletion to fire the pod-down path. Under -race it reported
this Owner pair before the change and nothing after it, with liveness intact: no stall, all
8 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 do
touch adjacent lines in updates.go and conflict in exactly one place; I have merged them
locally and the resolution is one hunk (take setOwner from here and recordChunkOwner from
#289). Happy to rebase whichever lands second. With both applied, the harness above reports
zero races.

Not in this change

chunk.FreeIds and chunk.ScanIds are also raced, and it is a third distinct defect:
scanChunk appends to and truncates them with no lock (scan.go:37-43) while
AllocateIntID/ReleaseIntID touch the same slices under the package-level mutex
(chunk.go:112-123). It is reachable by design, since ReleaseInt32ID looks up scanChunks
specifically 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:

Read chunk.go:112 <-> prev write scan.go:43     (FreeIds)
Read chunk.go:120 <-> prev write scan.go:39     (ScanIds)

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.

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>
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.

1 participant