Skip to content

fix: harden TN replica lifecycle#25721

Merged
mergify[bot] merged 3 commits into
matrixorigin:mainfrom
aptend:fix/tn-lifecycle-hardening
Jul 16, 2026
Merged

fix: harden TN replica lifecycle#25721
mergify[bot] merged 3 commits into
matrixorigin:mainfrom
aptend:fix/tn-lifecycle-hardening

Conversation

@aptend

@aptend aptend commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

Fixes #25668
Fixes #25678
Fixes #25694
Fixes #25696

#25695 is 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 main after #25724 replaced the TN replica creation/cancellation model. It keeps and completes the lifecycle gaps that remain after that merge:

  • TN RPC retry waits observe request cancellation and re-check cancellation when the timer and context become ready together.
  • Each RPC/local dispatch now acquires a replica service lease. Replica removal first rejects new calls and cancels active call contexts, then drains active calls before closing or destroying storage. Service calls do not hold the lifecycle lock, so local 2PC nested dispatch remains reentrant.
  • Caller cancellation is checked before and after startup readiness and again while acquiring the lease, so an already-canceled request is never dispatched through the ready-channel double-ready window.
  • TN shutdown cancels replica lifecycles before draining the RPC server, then closes replica storage, the shared sender, and the remaining clients. This prevents both request-vs-destroy races and server-drain/lifecycle-cancel deadlocks.
  • Clock-SI reads waiting for a future snapshot and async Committing retries now stop on their request/service context, so replica close cannot wait forever on those loops.
  • Heartbeats report a replica only after service startup succeeds and stop reporting it once removal begins.
  • Logtail RPC message-size validation rejects values too small for the protobuf header and values above the 100 MiB RPC safety bound. Final option values are validated before TAE resources or segment buffers are created, without allocating the configured payload size.

The ownership review also found related concrete gaps on the same paths:

  • TxnService borrows the store-wide sender; only the store now closes it, after all replicas are closed.
  • The store closes both QueryService and QueryClient.
  • StorageMEM no longer creates an unused logservice client, while StorageMEMKV closes its owned client exactly once.
  • Replica close is idempotent, protecting TAE's non-idempotent DB close when shutdown and removal meet.

Validation:

  • make build
  • go vet ./pkg/tnservice ./pkg/txn/service ./pkg/txn/storage/mem ./pkg/txn/storage/tae ./pkg/vm/engine/tae/logtail/service
  • go test -race -count=1 ./pkg/tnservice ./pkg/txn/service ./pkg/txn/storage/mem ./pkg/txn/storage/tae ./pkg/vm/engine/tae/logtail/service
  • local non-Docker etc/launch-multi-cn: recovery completed, both CNs performed cross-CN SQL read/write, and graceful shutdown reached Shutdown complete

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@mergify mergify Bot added the kind/bug Something isn't working label Jul 14, 2026
@matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Jul 14, 2026
@aptend
aptend marked this pull request as draft July 14, 2026 11:06
@matrix-meow matrix-meow added size/L Denotes a PR that changes [500,999] lines and removed size/M Denotes a PR that changes [100,499] lines labels Jul 14, 2026
@aptend
aptend force-pushed the fix/tn-lifecycle-hardening branch from 29f3576 to 5e5ad3d Compare July 15, 2026 06:20

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@aptend

aptend commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the cancellation-test review in de43121d9.

  • TestRetryWaitHonorsContext now uses testing/synctest: it deterministically proves the request is blocked in a one-hour retry wait, cancels the context, then asserts completion and exactly one dispatch without any real-time performance assertion.
  • TestReplicaCreateRetryStopsOnStoreStop now waits for the first storage-create attempt, uses a one-hour retry interval, runs Stop asynchronously, and asserts termination plus exactly one create attempt.
  • Related new lifecycle tests now use 30-second timeouts only as terminal hang guards.
  • The clock-wait and async-commit cancellation tests now establish their blocking points explicitly before cancel/close and assert causes/call counts.

Validation passed: targeted race tests with -count=10, full related-package race tests with -count=1, go vet, and make build.

Please re-review when convenient.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mergify

mergify Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-07-16 14:23 UTC · Rule: main · triggered by rule Automatic queue on approval for main
  • Checks passed · in-place
  • Merged2026-07-16 16:39 UTC · at 635392204e526441ae8f52bb976c8607b3720979 · squash

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
  • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
  • github-review-approved [🛡 GitHub branch protection]
  • github-review-decision = APPROVED [🛡 GitHub branch protection]
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Compose CI / multi cn e2e bvt test docker compose(PESSIMISTIC)
    • check-neutral = Matrixone Compose CI / multi cn e2e bvt test docker compose(PESSIMISTIC)
    • check-skipped = Matrixone Compose CI / multi cn e2e bvt test docker compose(PESSIMISTIC)
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Standlone CI / Multi-CN e2e BVT Test on Linux/x64(LAUNCH, PROXY)
    • check-neutral = Matrixone Standlone CI / Multi-CN e2e BVT Test on Linux/x64(LAUNCH, PROXY)
    • check-skipped = Matrixone Standlone CI / Multi-CN e2e BVT Test on Linux/x64(LAUNCH, PROXY)
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
    • check-neutral = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
    • check-skipped = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone CI / UT Test on Ubuntu/x86
    • check-neutral = Matrixone CI / UT Test on Ubuntu/x86
    • check-skipped = Matrixone CI / UT Test on Ubuntu/x86
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Utils CI / Coverage
    • check-neutral = Matrixone Utils CI / Coverage
    • check-skipped = Matrixone Utils CI / Coverage
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone CI / SCA Test on Linux/arm64
    • check-neutral = Matrixone CI / SCA Test on Linux/arm64
    • check-skipped = Matrixone CI / SCA Test on Linux/arm64

@mergify
mergify Bot merged commit 734fa4e into matrixorigin:main Jul 16, 2026
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working kind/test-ci size/L Denotes a PR that changes [500,999] lines

Projects

None yet

3 participants