fix(vfsevents): fix CI-observed flake in s3events TestEnhancedMetadata - #342
Closed
funkyshu wants to merge 1 commit into
Closed
fix(vfsevents): fix CI-observed flake in s3events TestEnhancedMetadata#342funkyshu wants to merge 1 commit into
funkyshu wants to merge 1 commit into
Conversation
TestEnhancedMetadata only synchronized on the event handler firing (via an eventReceived channel), not on pollOnce's own completion. On CI (ubuntu, go1.26), the subtest could finish and its mock-expectation teardown could run before the background pollOnce goroutine made its DeleteMessage call, producing: FAIL: DeleteMessage(string,string) FAIL: 1 out of 2 expectation(s) were met. This is exactly the residual race risk flagged (but not fixed at the time, since it wasn't the primary target) during review of #339's fix for TestNonVersionedBucketMetadata. Fixed by switching TestEnhancedMetadata to the same completion-based synchronization (wait for pollOnce to fully return via an error channel) already used by TestNonVersionedBucketMetadata, extracting the shared wait/cancel/grace-period logic into a waitForPoll suite helper (mirroring the equivalent gcsevents helper from #341). Uses time.NewTimer + defer Stop() to proactively avoid the un-stopped time.After timer issue raised in review of #341. Verified: - go test -race -run TestS3WatcherTestSuite -count=300 ./watchers/s3events/... - go test -run TestS3WatcherTestSuite -count=1000 ./watchers/s3events/... (no -race, to stress test scheduling/timing rather than just data races) both pass cleanly with 0 failures.
Member
Author
|
Folding this into #341 per discussion — same underlying pattern, and consolidating avoids two overlapping open PRs against the same test files. |
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
Fixes a CI failure observed on ubuntu + go1.26 after #339 merged:
Root cause
TestEnhancedMetadataonly synchronized on the event handler firing (via aneventReceivedchannel), not onpollOnce's own completion. This meant the subtest could reach its assertions and finish (triggering mock-expectation teardown) before the backgroundpollOncegoroutine made itsDeleteMessagecall — a timing gap that's tight enough to usually pass locally but wide enough to occasionally lose the race on CI runners.This is exactly the residual race risk flagged (but intentionally not fixed at the time, since it wasn't the primary target) during review of #339's fix for the sibling
TestNonVersionedBucketMetadatatest.Fix
TestEnhancedMetadatanow uses the same completion-based synchronization asTestNonVersionedBucketMetadata— waits forpollOnceto fully return via an error channel, rather than only waiting for the handler to fire.TestNonVersionedBucketMetadata) into awaitForPollsuite helper, now used by both tests, mirroring the equivalentwaitForReceivehelper added togcsevents_test.goin fix(vfsevents): remove sleep-based races in vfsevents watcher tests #341.time.NewTimer+defer timer.Stop()throughout (rather thantime.After), proactively addressing the un-stopped-timer issue raised in review of fix(vfsevents): remove sleep-based races in vfsevents watcher tests #341.contrib/vfsevents/CHANGELOG.md.Test plan
go test -race -run TestS3WatcherTestSuite -count=300 ./contrib/vfsevents/watchers/s3events/...— clean.go test -run TestS3WatcherTestSuite -count=1000 ./contrib/vfsevents/watchers/s3events/...(no-race, to stress scheduling/timing rather than just data races) — clean.go build ./... && go vet ./...forcontrib/vfsevents— clean.golangci-lint run ./watchers/s3events/...— only a pre-existing, unrelatednolintlintfinding ins3events.go(not touched by this PR).Made with Cursor