fix(sync): avoid send on closed channel in fileinfo watcher Close#1992
Open
anxkhn wants to merge 1 commit into
Open
fix(sync): avoid send on closed channel in fileinfo watcher Close#1992anxkhn wants to merge 1 commit into
anxkhn wants to merge 1 commit into
Conversation
The FILEINFO file watcher's timer goroutine is the sole sender on evChan and erChan, but Close() closed both channels without coordinating with that goroutine. If the ticker fired while Close ran (for example when filepath Sync returns on context cancel and its deferred watcher.Close() executes), the goroutine could be mid-send on evChan and hit "panic: send on closed channel", crashing flagd. This only affects the non-default fileinfo watcher; the default fsnotify watcher is unaffected. Give the watcher a done channel (guarded by a sync.Once) and a sync.WaitGroup. Close now signals the goroutine to stop and aborts any in-flight send, waits for the goroutine to exit, and only then closes the channels, so no send can race with the close. The stop signal is also added to the run loop's select and to every send in update so shutdown is prompt and cannot deadlock on a full buffer. Adds a regression test that drives the timer at a fast tick with a stat func that emits an event every tick, then calls Close with no reader draining the channels; it panics before this change and passes after. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
dosubot
Bot
added
the
size:M
|
📝 WalkthroughWalkthroughThe file watcher now coordinates timer goroutine shutdown with a completion signal, idempotent close protection, and a wait group before closing channels. Event and error sends can abort during shutdown, and a race test covers closing while timer events are being emitted. ChangesFile watcher shutdown
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/pkg/sync/file/fileinfo_watcher_test.go (1)
45-58: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the race setup deterministic.
The 512-slot buffer cannot be assumed to fill during a 20 ms sleep, so the test may never reach the documented blocked-send state. Use an unbuffered channel and signal when
statFuncis entered instead of sleeping.Proposed test setup
watcher := makeTestWatcher(t, map[string]fs.FileInfo{ "foo": &mockFileInfo{name: "foo", modTime: time.Now()}, }) +watcher.evChan = make(chan fsnotify.Event) +statCalled := make(chan struct{}) watcher.statFunc = func(_ string) (fs.FileInfo, error) { + close(statCalled) return &mockFileInfo{name: "foo", modTime: time.Now().Add(time.Hour)}, nil } watcher.run(context.Background(), time.Millisecond) -time.Sleep(20 * time.Millisecond) +<-statCalled🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/pkg/sync/file/fileinfo_watcher_test.go` around lines 45 - 58, Make Test_fileInfoWatcher_Close_racesWithTimer deterministic by replacing the watcher’s event channel with an unbuffered channel and adding a synchronization signal from statFunc when it is entered. Wait on that signal before invoking the close/race logic, rather than relying on the fixed 20 ms sleep, ensuring the goroutine reaches the blocked send state described by the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/pkg/sync/file/fileinfo_watcher_test.go`:
- Around line 60-70: Add a second call to fileInfoWatcher.Close() in the test
after the initial close succeeds, and assert it returns nil without panicking.
Keep the existing channel-closure assertions to verify repeated Close remains
idempotent.
In `@core/pkg/sync/file/fileinfo_watcher.go`:
- Around line 55-65: Make fileInfoWatcher.Close fully idempotent by moving the
done signal, f.wg.Wait(), and both channel closures inside the existing
f.stopOnce.Do callback; subsequent Close calls must perform no operations and
must not panic.
---
Nitpick comments:
In `@core/pkg/sync/file/fileinfo_watcher_test.go`:
- Around line 45-58: Make Test_fileInfoWatcher_Close_racesWithTimer
deterministic by replacing the watcher’s event channel with an unbuffered
channel and adding a synchronization signal from statFunc when it is entered.
Wait on that signal before invoking the close/race logic, rather than relying on
the fixed 20 ms sleep, ensuring the goroutine reaches the blocked send state
described by the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a36fd104-91af-49d3-9555-523d7e7c97f5
📒 Files selected for processing (2)
core/pkg/sync/file/fileinfo_watcher.gocore/pkg/sync/file/fileinfo_watcher_test.go
Comment on lines
+60
to
+70
| if err := watcher.Close(); err != nil { | ||
| t.Errorf("fileInfoWatcher.Close() error = %v", err) | ||
| } | ||
|
|
||
| // Close must have stopped the timer goroutine and closed both channels; | ||
| // draining the (buffered) events chan blocks forever unless it is closed | ||
| for range watcher.Events() { | ||
| } | ||
| if _, ok := <-watcher.Errors(); ok { | ||
| t.Error("fileInfoWatcher.Close() failed to close error chan") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cover repeated Close() calls.
The implementation claims idempotency, but this test only closes once and therefore misses the double-close panic at Lines 64-65 of fileinfo_watcher.go.
Proposed coverage
if err := watcher.Close(); err != nil {
t.Errorf("fileInfoWatcher.Close() error = %v", err)
}
+if err := watcher.Close(); err != nil {
+ t.Errorf("second fileInfoWatcher.Close() error = %v", err)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := watcher.Close(); err != nil { | |
| t.Errorf("fileInfoWatcher.Close() error = %v", err) | |
| } | |
| // Close must have stopped the timer goroutine and closed both channels; | |
| // draining the (buffered) events chan blocks forever unless it is closed | |
| for range watcher.Events() { | |
| } | |
| if _, ok := <-watcher.Errors(); ok { | |
| t.Error("fileInfoWatcher.Close() failed to close error chan") | |
| } | |
| if err := watcher.Close(); err != nil { | |
| t.Errorf("fileInfoWatcher.Close() error = %v", err) | |
| } | |
| if err := watcher.Close(); err != nil { | |
| t.Errorf("second fileInfoWatcher.Close() error = %v", err) | |
| } | |
| // Close must have stopped the timer goroutine and closed both channels; | |
| // draining the (buffered) events chan blocks forever unless it is closed | |
| for range watcher.Events() { | |
| } | |
| if _, ok := <-watcher.Errors(); ok { | |
| t.Error("fileInfoWatcher.Close() failed to close error chan") | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 66-67: Either remove or fill this block of code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/pkg/sync/file/fileinfo_watcher_test.go` around lines 60 - 70, Add a
second call to fileInfoWatcher.Close() in the test after the initial close
succeeds, and assert it returns nil without panicking. Keep the existing
channel-closure assertions to verify repeated Close remains idempotent.
Comment on lines
+55
to
65
| // Close stops the timer goroutine and closes the event and error channels | ||
| func (f *fileInfoWatcher) Close() error { | ||
| // close all channels and exit | ||
| // signal the timer goroutine to stop and abort any in-flight send, then wait | ||
| // for it to exit before closing the channels it sends on. Closing before the | ||
| // goroutine has stopped would race with its sends -> send on closed channel. | ||
| f.stopOnce.Do(func() { | ||
| close(f.done) | ||
| }) | ||
| f.wg.Wait() | ||
| close(f.evChan) | ||
| close(f.erChan) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Include channel closure inside stopOnce.
A second Close() still panics because Lines 64-65 execute on every call. Put signaling, waiting, and channel closure inside stopOnce.Do to make the complete operation idempotent.
Proposed fix
f.stopOnce.Do(func() {
close(f.done)
+ f.wg.Wait()
+ close(f.evChan)
+ close(f.erChan)
})
-f.wg.Wait()
-close(f.evChan)
-close(f.erChan)
return nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Close stops the timer goroutine and closes the event and error channels | |
| func (f *fileInfoWatcher) Close() error { | |
| // close all channels and exit | |
| // signal the timer goroutine to stop and abort any in-flight send, then wait | |
| // for it to exit before closing the channels it sends on. Closing before the | |
| // goroutine has stopped would race with its sends -> send on closed channel. | |
| f.stopOnce.Do(func() { | |
| close(f.done) | |
| }) | |
| f.wg.Wait() | |
| close(f.evChan) | |
| close(f.erChan) | |
| // Close stops the timer goroutine and closes the event and error channels | |
| func (f *fileInfoWatcher) Close() error { | |
| // signal the timer goroutine to stop and abort any in-flight send, then wait | |
| // for it to exit before closing the channels it sends on. Closing before the | |
| // goroutine has stopped would race with its sends -> send on closed channel. | |
| f.stopOnce.Do(func() { | |
| close(f.done) | |
| f.wg.Wait() | |
| close(f.evChan) | |
| close(f.erChan) | |
| }) | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/pkg/sync/file/fileinfo_watcher.go` around lines 55 - 65, Make
fileInfoWatcher.Close fully idempotent by moving the done signal, f.wg.Wait(),
and both channel closures inside the existing f.stopOnce.Do callback; subsequent
Close calls must perform no operations and must not panic.
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
1 participant
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
FILEINFOfile watcher (core/pkg/sync/file/fileinfo_watcher.go) can crash flagd withpanic: send on closed channelat shutdown.The watcher's timer goroutine is the sole sender on
evChananderChan, butClose()closed both channels without coordinating with that goroutine. If the ticker fires whileClose()runs, the goroutine can be mid-send onevChanand hit the panic. The realistic trigger isfilepathSync()returning on context cancel: itsdefer fs.watcher.Close()runs while the same context is stopping the timer goroutine, so a send can still be in flight exactly as the channel is closed.This only affects the non-default
fileinfowatcher (selected bywatchType == "fileinfo"); the defaultfsnotifywatcher is unaffected. The window is narrow, but the panic is unrecovered and takes the process down.Fix
Close()now stops the timer goroutine before it closes the channels the goroutine sends on:donechannel (closed once, guarded by async.Once) and async.WaitGrouptofileInfoWatcher.Close()signalsdone, waits for the goroutine to exit (wg.Wait()), and only then closesevChan/erChan, so no send can race with the close.doneis also added to the timer goroutine'sselectand to every send inupdate(), so an in-flight send is aborted promptly and shutdown cannot deadlock on a full, unread buffer.No happy-path behavior changes.
Changes
core/pkg/sync/file/fileinfo_watcher.go: adddone/stopOnce/wg; makeClose()stop-then-close; make the timer goroutine and the three sends (erChaninrun, bothevChansends inupdate) abortable ondone.core/pkg/sync/file/fileinfo_watcher_test.go: addTest_fileInfoWatcher_Close_racesWithTimer, which drives the timer at a fast tick with a stat func that emits an event every tick, then callsClose()with no reader draining the channels. It panics before this change and passes after.makeTestWatchergains thedonefield so the existing close test keeps compiling.Notes
make test-core(-race),golangci-lintv2.7.2, andgofmtall pass locally;core/pkg/sync/filecoverage is ~69.9% and the fullcore/pkg/...-racesuite has no regressions.Close/run/updatecoordination (keeping the field so it compiles) reproduces the exactpanic: send on closed channel; with the fix it is race-clean across repeated runs.