fix(warehouse/slave): guard outputFileWritersMap and tableEventCountMap reads in uploadLoadFiles (#7002) - #7256
Open
kotwal-itpro wants to merge 1 commit into
Conversation
…ap reads in uploadLoadFiles (rudderlabs#7002) `uploadLoadFiles` read `jr.outputFileWritersMap` and `jr.tableEventCountMap` in the main goroutine and in a spawned upload goroutine without holding `outputFileWritersMapMu` / `tableEventCountMapMu`. The Go race detector flags these as unsynchronised concurrent reads. In practice `handlePendingStagingFile` populates both maps before this function is called, but the ordering is implicit and there is no formal synchronisation guarantee. Snapshot each map under its read lock into a local slice and a local map before starting the errgroup workers, then let the workers operate on those snapshots. The workers no longer touch the shared maps, which eliminates both the reported race and any future regression as more concurrent work is layered on top. Semantics are preserved: the same tables are uploaded, the same TotalRows values are reported, and the same output-length invariant is checked. Existing tests still pass and `go vet` is clean.
Author
|
Hi maintainers — checking in on this one too. It closes #7002 (data race in the warehouse-slave upload path). The fix snapshots the shared maps under the existing RWMutexes before the errgroup workers spawn, so no concurrent worker ever touches shared state directly. Happy to adjust the approach if you'd rather see the synchronization done differently. Thanks! |
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.
Fixes #7002.
Problem
In
warehouse/slave/worker_job.go,uploadLoadFilesreadsjr.outputFileWritersMapandjr.tableEventCountMapfrom both the main goroutine and a spawned upload goroutine without holding their respective mutexes (outputFileWritersMapMu/tableEventCountMapMu). The Go race detector flags this as unsynchronised concurrent access — the exact behaviour described in #7002.Sites affected on current
master:warehouse/slave/worker_job.go:415—len(jr.outputFileWritersMap)in the main goroutinewarehouse/slave/worker_job.go:423—for tableName, uploadFile := range jr.outputFileWritersMapin the spawned goroutinewarehouse/slave/worker_job.go:450—jr.tableEventCountMap[tableName]in the errgroup workerwarehouse/slave/worker_job.go:473,483,484— three morelen(jr.outputFileWritersMap)reads in the main goroutineIn practice both maps are fully populated by
handlePendingStagingFilebefore this method runs, but the ordering is implicit — there is no formal synchronisation guarantee, and layering more concurrent work on top would trip the race for real.Fix
Rather than sprinkling
RLock/RUnlockat each read site (which still leaves the errgroup workers touching the shared maps), snapshot both maps under their read locks into local slices/maps before starting the errgroup workers. The workers then run entirely over local data.pending []pendingUpload— snapshot ofoutputFileWritersMapunderoutputFileWritersMapMu.RLock()tableEventCounts map[string]int— snapshot oftableEventCountMapundertableEventCountMapMu.RLock()The workers no longer read the shared maps, which eliminates both the reported race and any future regression as more concurrent work is layered on the same maps.
Semantics preserved
TotalRowsvalues reported (viatableEventCounts[tableName]).len(output) != len(pending)).len(pending)).Verified locally
go build ./warehouse/slave/...— clean.go vet ./warehouse/slave/...— clean.go test -race -run "TestSlaveJobPayload|TestSlaveJob/writer_and_reader|TestSlaveJob/discards" ./warehouse/slave/— passes (the docker-dependentTestSlaveJob/upload_load_filesandTestSlaveJob/download_staging_filewere skipped locally due to a docker socket path mismatch on the reviewer's machine; CI will exercise them).