test(lockservice): bound remote waiter snapshot setup#25860
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Follow-up on the failed CI run: the first panic was caused by the test waiter using a context without a deadline. Remote Lock crosses MORPC, and Future.init requires a deadline for non-one-way sends. Commit acb41d7 changes the waiter to an independent 30-second timeout, preserving separation from the setup budget while satisfying the RPC contract. The subsequent RWMutex fatal was cascading panic cleanup after that first panic, not evidence of the original lockservice hang. Focused, 20 independent-process repetitions, race (-count=20), full lockservice package, build, and vet all pass locally. |
…h-lockservice-tests # Conflicts: # pkg/lockservice/service_remote_test.go
|
Latest main is now merged at PR head 8123388. The conflict was limited to this test cleanup and was resolved by retaining the failure-safe lifecycle cleanup while incorporating the new main changes (including TxnIterFunc liveness and bounded helper coverage). Revalidation on the final merged head passed: focused test, race focused test with -count=5, full ./pkg/lockservice, go build, go vet, and git diff --check. New CI run: https://github.com/matrixorigin/matrixone/actions/runs/29639445063 |
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
No concrete regressions found in the actual diff; the focused test passes in CI.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review completed. The final head closes the waiter lifecycle and all newly introduced waits are bounded. Focused stress, race, full lockservice package, build, vet, latest-main merge validation, and CI passed. The pre-existing production retry issue is tracked separately in #25870 and does not block this test-only fix.
Merge Queue Status
This pull request spent 58 minutes 47 seconds in the queue, including 58 minutes 31 seconds running CI. Required conditions to merge
|
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #25859
What this PR does / why we need it:
TestFetchWhoWaitingMeUsesActiveRemoteWaiterSnapshots used its 10-second setup context for the background waiter and then called an unbounded test helper while waiting for that waiter to reach the owner queue. If setup or remote routing consumed the deadline, Lock returned without entering the queue and the helper spun until the package-level 40-minute timeout.
The test now gives the background waiter an explicit lifecycle, bounds both admission and post-release waits, and uses independent cleanup contexts. On every assertion-failure path it releases the holder, cancels the waiter, and unlocks a waiter that acquired the lock before cleanup.
A follow-up CI run exposed that the independent waiter initially used a context without a deadline. Remote Lock crosses MORPC, whose Future contract requires every non-one-way send to carry a deadline. The waiter now has an independent 30-second timeout: it remains decoupled from setup while satisfying the RPC contract and retaining a hard liveness bound. The later RWMutex fatal in that run was cascading panic cleanup after the deadline-contract panic, not the original lockservice hang.
This is a test-liveness repair; it does not change lockservice production behavior.
Related CI failure #25729 is already covered by the receiver lifecycle fix merged in #25820. Current main dispatch regression no longer contains the racy post-send assertion, so no duplicate issue or code change is needed here.
Tested with:
mo-cgo-test -v -count=1 -timeout=90s ./pkg/lockservice -run ^TestFetchWhoWaitingMeUsesActiveRemoteWaiterSnapshots$
20 independent process repetitions of the focused test
mo-cgo-test -race -count=20 -timeout=180s ./pkg/lockservice -run ^TestFetchWhoWaitingMeUsesActiveRemoteWaiterSnapshots$
mo-cgo-test -count=1 -timeout=300s ./pkg/lockservice
go build ./pkg/lockservice
go vet ./pkg/lockservice