fix: harden TN replica lifecycle#25721
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? |
29f3576 to
5e5ad3d
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
[P1] Make the cancellation/lifecycle regressions independent of runner speed
The production changes look sound, but two new regressions encode correctness as a wall-clock performance assertion: TestRetryWaitHonorsContext requires a 20 ms deadline to return within 500 ms, and TestReplicaCreateRetryStopsPromptly requires Stop to finish within 500 ms. A loaded or paused CI worker can violate either bound even when cancellation is correct, which makes these tests flaky; several related lifecycle checks also use only 1-second terminal guards.
Please establish the causal order explicitly (for example: signal that the first retry/storage-create attempt has entered, then cancel/stop), use a retry interval long enough that the old implementation cannot accidentally complete, run the operation in a goroutine, and use a generous timeout only as a final CI hang guard. Assert the terminal result and call count/state rather than elapsed duration. This preserves the bug-class coverage without turning scheduler speed into part of the functional contract.
fce6db3 to
de43121
Compare
|
Addressed the cancellation-test review in
Validation passed: targeted race tests with Please re-review when convenient. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed the current head de43121d9. The previous cancellation-test concern is resolved: the retry test now uses testing/synctest, replica creation and Clock-SI tests establish explicit blocking points, async-commit cancellation asserts the terminal call count, and the 30-second values are terminal hang guards rather than scheduler-speed assertions.
I also rechecked the unchanged production diff for correctness, performance, and unhappy paths (call leasing/drain, shutdown ordering, retry cancellation, sender/storage ownership, and logtail allocation bounds). Local validation passed for all five affected packages, focused race tests with -count=10, full affected-package race tests, go build, and go vet. The PR merges cleanly with the latest main. All completed CI checks are green; coverage was still running when this review was submitted.
Merge Queue Status
This pull request spent 2 hours 16 minutes 50 seconds in the queue, including 1 hour 11 minutes 48 seconds running CI. Required conditions to merge
|
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #25668
Fixes #25678
Fixes #25694
Fixes #25696
#25695is no longer part of this PR: its replica-start wait cancellation path was implemented by #25724.What this PR does / why we need it:
This PR was rebuilt on the current
mainafter #25724 replaced the TN replica creation/cancellation model. It keeps and completes the lifecycle gaps that remain after that merge:Committingretries now stop on their request/service context, so replica close cannot wait forever on those loops.The ownership review also found related concrete gaps on the same paths:
TxnServiceborrows the store-wide sender; only the store now closes it, after all replicas are closed.StorageMEMno longer creates an unused logservice client, whileStorageMEMKVcloses its owned client exactly once.Validation:
make buildgo vet ./pkg/tnservice ./pkg/txn/service ./pkg/txn/storage/mem ./pkg/txn/storage/tae ./pkg/vm/engine/tae/logtail/servicego test -race -count=1 ./pkg/tnservice ./pkg/txn/service ./pkg/txn/storage/mem ./pkg/txn/storage/tae ./pkg/vm/engine/tae/logtail/serviceetc/launch-multi-cn: recovery completed, both CNs performed cross-CN SQL read/write, and graceful shutdown reachedShutdown complete