fix(communities): reuse the request clock for RequestedToJoinAt - #7792
Open
friofry wants to merge 4 commits into
Open
fix(communities): reuse the request clock for RequestedToJoinAt#7792friofry wants to merge 4 commits into
friofry wants to merge 4 commits into
Conversation
Member
Jenkins BuildsClick to see older builds (25)
|
alexjba
approved these changes
Sep 1, 2026
`SaveRequestToJoinAndCommunity` read the wall clock a second time instead of reusing the clock of the request it was saving, so a call straddling a second boundary left the in-memory community disagreeing with the value `persistence_mapping.go` reads back from the request row. Three tests asserted that equality and failed 3 runs in 5 locally (TestRequestAccessAgain, TestSyncCommunity_RequestToJoin, TestDeletePendingRequestAccessWithDeclinedState); they now pass 15 in 15. Also de-flake TestCommunityContactCodeAdvertisement, which read `response.Contacts[0]` from a batch that can carry more than one contact - RetrieveAll drains the batch, so picking the wrong one was not retryable. Skip five tests that flake for reasons not resolved here, tracked in #7791: - TestBlockedContactSyncing - the unblock sync reaches the paired device with Blocked still true, so this looks like a product defect, not a test race - TestAliceDidNotProcessOutdatedCommunityRequestToJoinResponse - the duplicate check needs the community clock and the request clock to land in the same second - TestPeerExchange, TestHandleSegmentMessages, TestReevaluateMemberTokenMasterRoleInOpenCommunity_ERC721 - green locally under repetition, no hypothesis yet - TestLocalDownloadWithContext_Cancellation - races cancellation against a download that may already have finished Refs #7791
friofry
force-pushed
the
claude/status-go-flaky-tests-61dee2
branch
from
September 1, 2026 13:10
83d9e07 to
c31e01e
Compare
…tions of it TestDownDebounceResetsAfterSilentRecovery failed in CI on the `expectNoNotification(ch, 10*time.Millisecond)` right after the silent recovery. Only two paths can emit there, and both require the 120ms timer armed by the preceding Down to have already fired - that is, more than 120ms of wall time elapsed across a wait the test budgets at 40ms. On a loaded agent a short `time.After` can overrun several times over, and every debounce test here left a margin under 2x. Assertions about when a Down lands are now made by measuring elapsed time (expectNotificationNoEarlierThan), which scheduling delays can only push later; assertions that a specific update emitted nothing use a zero-wait channel check (expectNoPendingNotification), since emission is synchronous inside Update; and assertions that a stopped timer stays stopped watch past the moment it would have fired (expectNoNotificationPast) instead of for a hand-tuned fraction of the debounce. The shared testDownDebounce is 1s, so the tests' own bookkeeping has a margin of 5x or more rather than 1.7x. Verified the tests still catch what they guard, by mutation: a no-op stopDownTimer fails TestDownDebounceResetsAfterSilentRecovery, firing the timer immediately fails four of them, and a Pause that neither stops the timer nor suppresses the emit fails TestPauseStopsPendingDownTimer. 40 concurrent runs at GOMAXPROCS=1 are green. Suite runtime goes from ~3s to ~7s.
…adline Folding the margin into a single `time.Until(deadline)+200ms` timeout meant a caller already past the deadline got a non-positive duration, and a select whose timeout has expired picks at random between the two ready cases - so a notification sitting in the buffer would be missed half the time and the helper would pass without observing anything. Drain-check first, then wait out whatever is left of the window, then always spend the margin as its own wait. Raised by Copilot on #7793.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7792 +/- ##
===========================================
- Coverage 63.80% 63.67% -0.13%
===========================================
Files 876 876
Lines 121449 121449
===========================================
- Hits 77491 77336 -155
- Misses 36084 36230 +146
- Partials 7874 7883 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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
3 participants
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.
TestRequestAccessAgain,TestSyncCommunity_RequestToJoin,TestDeletePendingRequestAccessWithDeclinedState.SaveRequestToJoinAndCommunityread the wall clock a second time instead of reusing the clock of the requestit was saving:
(similar to
persistence_mapping.go:137RequestedToJoinAt = requestToJoin.clock)TestCommunityContactCodeAdvertisementit readresponse.Contacts[0]from a batch thatcan carry more than one contact.
RetrieveAlldrains the batch, so picking the wrong one was not retryable;it now looks Bob up by ID.
Skipped tests are tracked in Fix flaky unit tests #7791