test(lockservice): retry indeterminate txn timeout checks - #26587
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? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed head 90ef785. The retry matches the production state machine: a failed active-txn observation is indeterminate and retains ownership; only a later authoritative inactive result plus cannot-commit fencing permits removal. The loop remains serial and bounded, preserves the 10s failure oracle, and the deterministic unit now exercises the exact backend-create-timeout value. I traced the RPC/cleanup wait-for path and found no unbounded or duplicate-cleanup edge. Focused tests pass for 20 non-race and 10 race repetitions.
Merge Queue Status
This pull request spent 4 minutes 19 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #26587 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Tick the box to put this pull request back in the merge queue (same as
|
3 participants
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #26575
What this PR does / why we need it:
Root cause
Lockservice active-transaction recovery intentionally uses a separate MORPC
client with a 500ms backend-create budget. The failing CI log shows that cold
connection timing out after 500.241548ms. Production correctly treats that
observation as indeterminate: it retains the transaction and retries during a
later timeout scan.
TestCheckTxnTimeoutinstead required the transaction to beremoved by one scan, so scheduler delay in a valid recovery path failed the
test.
At the failing commit, that assertion also exited the topology callback before
the service and allocator cleanup statements. Their RPC goroutines remained
active and caused the later
TestIssue5176andTestUnlockInRollingRestartCNleak reports; the latter test's log contains theprevious test's service ID. Main has since merged #26555, which installs the
topology cleanup before the callback and closes it from a defer. This PR does
not duplicate that already-merged cleanup fix.
Changes
TestCheckTxnTimeoutassert the production contract: an indeterminatescan may retain the transaction, but subsequent scans must eventually remove
it after authoritative inactive and cannot-commit fence results.
morpc.ErrBackendCreateTimeoutin the existingdeterministic error-then-confirmed-inactive unit test.
The 500ms recovery policy remains unchanged. There are no production-code,
API, wire-format, or performance changes.
Verification
Environment: Go 1.26.4, Darwin/arm64, controlled MatrixOne CGo test wrapper.
-count=10: passTestValidTxnCheckErrorThenConfirmedInactive, non-race,-count=100: passTestCheckTxnTimeout: T=1.16s, N=25TestValidTxnCheckErrorThenConfirmedInactive: T=0s, N=100TestIssue5176: T=0.09s, N=100TestUnlockInRollingRestartCN: T=2.08s, N=14./pkg/lockservice, non-race: pass (113.574s)./pkg/lockservice, race: pass (121.847s)go list,go build, andgo vetfor./pkg/lockservice: passBVT is not applicable because the affected contract is an internal Go recovery
scanner and test-topology lifecycle, with no SQL-visible behavior.
Residual risk
The test retains its existing 10s outer bound and still fails if recovery never
obtains an authoritative result. It only permits the transient first-scan
timeout that production explicitly handles by retrying.