Repository navigation
Conversation
The handleShutdown goroutine was reading c.ctx through the struct pointer without holding c.mux. A concurrent Start() call writes c.ctx under the lock, creating an unsynchronised read/write pair detected by -race on the restart-collector sub-test. Capture ctx := c.ctx inside Start() under the lock before launching the goroutines, then pass ctx by value to startMethod(ctx) and handleShutdown(ctx, stopCh). Update the startMethod signature and test fakes accordingly. Signed-off-by: Killian Daly <Killian.Daly@ibm.com>
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Layer / File(s) | Summary |
|---|---|
Pass context and serialize generations internal/nonkube/controller/collector_handler.go, internal/nonkube/controller/collector_handler_test.go, internal/nonkube/controller/collector_handler_generation_test.go |
Start captures the generation context and start method, then waits for the previous generation to finish. Startup and shutdown handling use the captured context. Tests cover context cancellation and repeated restarts. |
Wait for collector cleanup internal/nonkube/flow/collector.go |
siteCollector defers status ConfigMap deletion and waits for statusSync.Run to stop after context cancellation. StartCollector documents its blocking and cleanup behavior. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant CollectorHandler
participant StartCollector
participant StatusSync
CollectorHandler->>StartCollector: Start with captured generation context
StartCollector->>StatusSync: Run status synchronization
CollectorHandler->>StartCollector: Cancel generation context
StartCollector->>StatusSync: Wait for Run to stop
StartCollector->>CollectorHandler: Return after ConfigMap cleanup
Merge Risk: 
b228d
-
Merge Risk: ⚪ Minimal · up to
b228dCollector generations are serialized and cleanup waits for status synchronization. No merge-blocking issue was established.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: resolving a data race involving context handling in CollectorLifecycleHandler. Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 4 files.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
ef1f167c-8729-496b-b971-532601150fa9
-
internal/nonkube/controller/collector_handler.gointernal/nonkube/controller/collector_handler_test.go
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
-
If Stop() and a new Start() occur before a generation-1 startCollectorLite goroutine is scheduled, the goroutine receives an already-cancelled ctx. flow.StartCollector does not check ctx.Err() at entry: siteCollector adds the shared status ConfigMap and registers a cleanup goroutine that deletes it on ctx.Done(). Because ctx is already done, the deletion fires immediately, removing the ConfigMap that the new generation is actively using and preventing further status updates. Add an ctx.Err() guard at the top of startCollectorLite. If the context is already cancelled, log and return before siteCollector is ever called, so the cleanup goroutine is never registered and the shared ConfigMap is never deleted by a stale generation.
…t status Each collector generation writes the same network status ConfigMap file. Its removal was fire-and-forget: siteCollector returned right after spawning a goroutine that waited for ctx.Done() and then deleted the file. Nothing made the next generation wait for that delete, so an immediate Stop() followed by Start() could let the new generation write the file before the old generation's delete landed, removing the new generation's status and stopping status updates. Make the collector own its lifetime: - flow.StartCollector now blocks until ctx is canceled and the status sync has stopped, then removes the ConfigMap via defer. The delete also runs if setup fails after the ConfigMap was added. - CollectorLifecycleHandler.Start keeps the previous generation's done channel and makes the new generation wait for it before calling startMethod. Start still returns immediately and the wait happens in the goroutine, so c.mux is never held while waiting. The ctx.Err() guard in startCollectorLite is kept; it now also skips a generation that was canceled while queued behind its predecessor. Add a regression test that restarts the real collector back to back against a temporary data directory and asserts the current generation's ConfigMap is present. It fails without the change. Signed-off-by: Killian Daly <Killian.Daly@ibm.com>
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
432d89aa-b417-43d8-b50d-d7433082e4ff
-
internal/nonkube/controller/collector_handler.gointernal/nonkube/controller/collector_handler_generation_test.gointernal/nonkube/flow/collector.go
-
- internal/nonkube/controller/collector_handler.go
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
-
The restart regression test slept for 100ms and then checked that the status ConfigMap existed. If generation 1 had not finished by then, its file could satisfy the check and generation 1 could delete it after the test reported success, while a slow but correct generation 2 could make the test fail. Capture each generation's lastDone channel around the restart. Wait for generation 1 to finish (it removes its ConfigMap before returning), then poll for generation 2's file. After the final Stop, wait for generation 2 to finish and assert the file is gone before starting the next round. All waits have a timeout. Also add doc comments to the functions touched by this change and fix the comment in Start, which said ctx is captured before the lock is released; the lock is held at that point. Signed-off-by: Killian Daly <Killian.Daly@ibm.com>
dalyk24
force-pushed
the
contribution/collector-handler-race
branch
from
November 1, 2026 16:10
b228d01 to
33d9f09
Compare
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.
Summary
Resolves a data race on
CollectorLifecycleHandler.ctxbetween concurrent calls toStart()and asynchronous shutdown handling / collector startup.Problem
In
CollectorLifecycleHandler,Start(ctx context.Context, ...)assignsc.ctx = ctxunderc.mux. However,handleShutdown(andstartCollectorLite) running in background goroutines accessedc.ctxdirectly through the struct pointer without holdingc.mux.When
Start()is called again concurrently or during restart sequences (e.g. tested inTestCollectorHandler/restart-collector), Go's race detector reports a data race:Fix
CollectorLifecycleHandler.Start(), capturectx := c.ctxlocally underc.mux.ctxby value toc.startMethod(ctx)andc.handleShutdown(ctx, stopCh).CollectorLifecycleHandler.handleShutdownandstartMethodsignature to acceptctx context.Contextas a parameter instead of dereferencingc.ctx.collector_handler_test.goto match the signature.Testing
Verified with
go test -raceacross 100 consecutive runs:Changes
internal/nonkube/controller/collector_handler.go: Passctxby value tostartMethodandhandleShutdown, eliminating unprotected struct field reads.internal/nonkube/controller/collector_handler_test.go: Update mock/fake test handler to acceptctx context.Context.Summary by CodeRabbit