-
Notifications
You must be signed in to change notification settings - Fork 60
chore(ci): fix flaky membership test and enforce gofmt - #171
Merged
Merged
Conversation
...ed PRs TestBackgroundFetchRetriesAfterBackoffExpiry used two barriers that do not actually gate the state it asserts on, so it failed intermittently in CI on PRs that do not touch this package (#163, #166, #169). Both barriers were false: - fakeMemberAPI records a call on *entry*, but backgroundFetch writes lastFailed only after the API call returns. Backdating lastFailed after waitForCallCount(1) therefore raced the manager's own write and got clobbered, leaving the backoff live so the retry never fired: "timed out waiting for 2 API calls". - EnsureFresh calls pushSnapshot synchronously on the caller's goroutine, so waitForPush(sink, 2) was satisfied by the test's own second EnsureFresh rather than by the background fetch. The lastFailed assertion then ran while that fetch was still in flight: "lastFailed not cleared by a successful fetch". Replace both with waitUntil, which polls the manager state under its own lock. Add an optional post-call delay to fakeMemberAPI and use it here, so the slow-API interleaving that used to be a rare race is now the test's default path. Verified: with the 50ms delay the old test failed 5/5; the new one passes 20/20 under -race, and still fails as intended when `delete(m.lastFailed, channelID)` is removed from manager.go.
golangci-lint had no formatter enabled, so gofmt drift accumulated silently: 29 files were unformatted on main. The cost lands on review — contributors' editors reformat whatever they touch, so alignment churn rides along in feature diffs and has to be separated from real changes every time. The same `Edge *edge.Client` realignment shows up in four open PRs for exactly this reason, and two open PRs introduce new gofmt failures that CI currently cannot see. Enable the gofmt formatter and reformat the tree in one pass so the next diff that touches these files is only the change itself. No semantic changes: build, vet, and `go test ./... -race` (54 packages) all pass.
This was referenced Sep 3, 2026
This was referenced Sep 3, 2026
Yukaii
pushed a commit
to Yukaii/slk
that referenced
this pull request
Sep 4, 2026
TestUserResolver_BatchesMissesThroughEdge failed intermittently (~10% of runs locally, and it just failed CI on gammons#128) with: user_resolver_test.go: U001 was not cached from the edge batch: getting user: sql: no rows in result set Same root cause as the membership flake fixed in gammons#171: the test waited on a barrier that does not gate the state it asserts. fakeBatcher.UsersInfo records the batch and returns immediately. The resolver only *then* loops applyEdgeUser, which is what calls UpsertUserFromEdge. So `for len(batcher.calls()) == 0 { ... }` returned before any row existed, and the db.GetUser assertions raced the write. Wait for the applied result instead — both cache rows present and both UserResolvedMsg sent. applyEdgeUser writes the row and then sends, per user, so the message count is the later signal and implies every write has landed. Adds a waitUntil helper to the package and documents why a batcher call count must not be used as a barrier. Verified: 40/40 under -race (previously ~2 failures per 20), and it still fails as intended when UpsertUserFromEdge is stubbed out, so it has not been weakened into a no-op.
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.
Two CI-hygiene fixes found while triaging the open PR queue. Both were producing false signals that cost review time on unrelated PRs.
1. Flaky
TestBackgroundFetchRetriesAfterBackoffExpiryThis test was failing intermittently in CI on PRs that don't touch
internal/slack/membership— currently red on #163, #166 and #169, all of which are innocent.It used two barriers that don't gate the state it asserts on:
waitForCallCount—fakeMemberAPIrecords a call on entry, butbackgroundFetchwriteslastFailedonly after the API call returns (manager.go:183-185). So backdatinglastFailedafterwaitForCallCount(1)raced the manager's own write and got clobbered, leaving the backoff live so the retry never fired →timed out waiting for 2 API calls.waitForPush—EnsureFreshcallspushSnapshotsynchronously on the caller's goroutine (manager.go:91-92), sowaitForPush(sink, 2)was satisfied by the test's own secondEnsureFresh, not by the background fetch. The assertion then ran while that fetch was still in flight →lastFailed not cleared by a successful fetch.Replaced both with a
waitUntilhelper that polls manager state under its own lock, and added an optional post-call delay tofakeMemberAPIso the slow-API interleaving that used to be a rare race is now this test's default path.The product code was correct —
delete(m.lastFailed, channelID)atmanager.go:222is properly locked. This was purely a test bug.Verified:
-race.delete(m.lastFailed, channelID)is removed frommanager.go, so it hasn't been weakened into a no-op.2. gofmt enforcement
.golangci.ymlenabled no formatter, so drift accumulated silently — 29 files were unformatted on main.The cost shows up in review: contributors' editors reformat whatever they touch, so alignment churn rides along inside feature diffs. The same
Edge *edge.Clientrealignment currently appears in four separate open PRs (#161, #162, #167, #170) for exactly this reason, and two open PRs introduce new gofmt failures that CI can't currently see.Enables the
gofmtformatter and reformats the tree in one pass, so the next diff touching these files is only the actual change.Verification
go build ./...— passgo vet ./...— passgo test ./... -race— pass, 54 packages, 0 failuresRecommend merging this first, so the rest of the PR queue rebases onto a tree where CI tells the truth.