fix: harden txn client cancellation and cleanup

[XuPeng-SH]

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #25206

What this PR does / why we need it:

This PR hardens the txn client lifecycle for cancellation, client shutdown, and CN-failure recovery, fixing multiple issues that could cause goroutine leaks, hangs, or process crashes in distributed scenarios.

Changes

1. Replace sync.Cond with channel-based signaling (client.go)

• sync.Cond.Wait() cannot respond to context cancellation or client close, causing goroutines to hang indefinitely.
• Replaced with pausedC and closedC channels; all wait paths now use select to listen on ctx.Done() and closedC, ensuring every exit condition wakes blocked callers.
• Added closed flag with proper cleanup on Close() to unblock paused New() calls.

2. Decouple abort signal from timestamp (client.go)

• Old abortC carried time.Time with buffer=1; new abort events were dropped when the channel was full, causing the worker to operate on stale timestamps and miss transactions that should be aborted.
• Changed abortC to a wakeup-only chan struct{}; the latest abort observation is stored in atomic.Int64 with CAS to ensure monotonic advancement. The worker reads latestAbortAt on each wakeup, never missing an observation.

3. Convert panics to error returns (operator.go)

• checkResponseTxnStatusForReadWrite, checkResponseTxnStatusForCommit, checkResponseTxnStatusForRollback, and checkTxnError previously used panic on malformed TN responses, which crashes the entire CN process.
• All panic calls replaced with return moerr.NewInternalError(...), allowing callers to handle errors gracefully.

4. Add waiter cleanup on context cancellation (timestamp_waiter.go)

• When a GetTimestamp caller was canceled, the waiter remained in the linked list until an unrelated future timestamp notification happened to clean it up — a permanent leak if no further notifications arrived.
• Added removeWaiter to drop the waiter from the list immediately on cancellation and unref it.
• Added index field for O(1) swap-delete removal.

5. Context-aware openTxn and waitMarkAllActiveAbortedLocked

• openTxn now accepts context.Context and checks ctx.Err() at all wait points, preventing unbounded blocking.
• waitMarkAllActiveAbortedLocked now propagates context cancellation and client close signals.

6. Safe type assertion for ResumeLockService

• ResumeLockService type assertion changed from bare cast (panics on mismatch) to ok-pattern assertion.

Tests Added

• TestOpenTxnReturnsWhenPausedContextCanceled — verifies paused New() returns on context timeout.
• TestCloseUnblocksPausedNew — verifies Close() unblocks a paused New() call.
• TestOpenTxnReturnsWhenAbortMarkingContextCanceled — verifies openTxn returns on context cancel during abort marking.
• TestMarkAllActiveTxnAbortedRetainsLatestObservation — verifies CAS monotonic advancement and wakeup signal coalescing.
• TestGetTimestampCanceledWaitIsRemoved — verifies canceled waiters are removed from the list.
• TestCommitAndRollbackDoNotProceedAfterRunningSQLTimeout — verifies commit/rollback abort when running SQL times out.
• TestMalformedTNResponsesReturnErrors — verifies malformed responses return errors instead of panicking.

[qodo-code-review]

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 →

[XuPeng-SH]

Blocking

The latest head still leaves one admission-lifecycle hole in pkg/txn/client: a queued user txn that fails during snapshot acquisition can still be promoted afterward and leak a permanent active/user slot.

Concrete failure path:

1. With maxActiveTxn=1, txn A is the active user txn and txn B is queued in waitActiveTxns.
2. doCreateTxn for txn B enters snapshot acquisition before waitActive() (pkg/txn/client/client.go:502-544).
3. While B is still in snapshot wait, txn A closes and closeTxn claims B via claimWaitActiveOpsLocked, removes it from waitActiveTxns, and reserves users for it (pkg/txn/client/client.go:895-915, 936-959; pkg/txn/client/active_txn_waiter.go:72-79).
4. Then B's snapshot wait fails/cancels, so doCreateTxn calls closeAsAborted (pkg/txn/client/client.go:516-533; pkg/txn/client/operator.go:1910-1919).
5. If B's ClosedEvent runs in the dequeue->publish gap, closeTxn finds B in neither activeTxns nor waitActiveTxns, and waiter.canceled() is still false, so it does no cleanup (pkg/txn/client/client.go:918-924).
6. The promoter then publishes the already-closed txn into activeTxns via addActiveTxn (pkg/txn/client/client.go:258-266, 910-914). No later ClosedEvent remains to remove it.

Result: activeTxnCount and users both stay leaked. After that, future user txns can wedge behind maxActiveTxn even though the promoted txn is already closed.

I verified this from the code path and with a deterministic scratch repro in a throwaway worktree by pausing exactly between client.mu.Unlock() and addActiveTxn(): once the failed queued txn's ClosedEvent ran in that window, the promoter still published a closed operator and the slot remained leaked.

Suggested fix: snapshot-acquisition failure for a queued txn needs to participate in the same admission ownership state machine as wait-active cancellation. Either make the failure path mark the waiter non-promotable / non-owning before closeAsAborted(), or make promotion re-check closed/terminal state before reserving/publishing. Please also add a regression that combines queued snapshot failure with claimed promotion, since current tests cover those two edges only in isolation.

[XuPeng-SH]

Addressed the new admission blocker in 37797246e5.

The finding was valid. The previous state machine was scoped too narrowly to cancellation inside waitActive; snapshot acquisition runs after openTxn transfers ownership but before waitActive, so its failure could bypass the promotion barrier.

The fix is structural rather than a snapshot-specific branch:

• Immediately after openTxn succeeds, doCreateTxn installs one deferred failure owner.
• Every later error return (and panic stack-unwind) automatically calls abortCreatedTxn; only the final successful admission disarms it.
• activeTxnWaiter.abort is now the common barrier for every post-open failure phase:
  • queued: mark non-promotable and let ClosedEvent remove queue ownership;
  • promoting: record the failure and wait until active ownership is published;
  • admitted: proceed directly to active cleanup.
• The barrier's authoritative error is preserved, including a concurrent client-close result.

The deterministic regression holds B in snapshot acquisition, lets A's close claim/dequeue B, blocks B's active-map publication at its shard, then cancels B. It proves B cannot return or emit its sole ClosedEvent before publication and finally verifies activeTxnCount, users, queue/map ownership, and subsequent capacity reuse.

Reflection: the core state-machine direction was right, but its abstraction boundary and the review method were wrong. I modeled a waitActive cancellation gate instead of the complete post-open admission lifecycle, then derived the test from the reported interleaving rather than the semantic product of phase × failure × owner × generation. The generalized rule already existed in the review skill; this was an execution-discipline failure, so I did not add more duplicate skill text. The code now enforces the rule with one structural defer instead of relying on every present and future error branch to remember cleanup.

Fresh validation after the final edit:

• focused admission matrix under -race -count=50
• full pkg/txn/client ordinary and race suites
• ordinary tests for all 8 affected packages
• GOWORK=off go vet -mod=readonly for all 8 affected packages
• make build
• git diff --check

[XuPeng-SH]

Reviewed latest head 37797246e5 again.

The previous blocker around queued snapshot-acquisition failure racing with claimed promotion now looks fixed.

What changed in the right direction:

• doCreateTxn now has a single deferred admission-abort owner after openTxn, so every post-open failure path is forced through the same cleanup barrier instead of relying on ad hoc closeAsAborted calls.
• activeTxnWaiter.abort now covers both cancellation during waitActive() and failures that happen before waitActive() is entered, while still waiting for publication when promotion already owns the operator.
• The new regression TestQueuedSnapshotFailureDuringPromotionReleasesOwnership covers the exact interleaving that was missing before: queued snapshot failure after claim/dequeue but before publication.

I re-checked the ownership closure in client.go / active_txn_waiter.go and reran the targeted txn-client tests around canceled promotion, queued snapshot failure during promotion, canceled snapshot acquisition, and queued snapshot wait on close. I didn't find a new high-confidence blocker on this head.

[XuPeng-SH]

Double-checked the latest head 37797246. The previous production-code blockers are fixed: queued promotion now has a linearized ownership transfer, all post-openTxn failures cross the admission barrier before the sole ClosedEvent, legacy token 0 behavior is preserved, terminal calls stay sealed after close, and RunSQL generations remain isolated. I found no additional confirmed correctness, performance, leak, hang, or unbounded-growth blocker in the txn lifecycle code.

One reproducible blocker remains in the unrelated skill/tooling commit included in this PR:

[P2] The new mo-cgo-test wrapper cannot link CGo-transitive packages on Linux

On this head, with the repository CGo and third-party libraries freshly available, this documented command fails:

.agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -timeout=10m ./pkg/txn/client

The linker reports unresolved roaring64_bitmap_* symbols from cgo/libmo.so. The wrapper always adds -lmo at mo-cgo-test:99, but its external link flags do not add -lroaring; pkg/txn/client does not import cgo/lib.go, so it does not inherit that package's CRoaring link directive. Adding -lroaring after the third-party -L path in external_flags makes the same test binary link and pass. Please either fix the wrapper/link contract and add a smoke test for a CGo-transitive package, or preferably split/remove commit b071dcf0 and the 11 unrelated .agents/.claude files from this transaction-lifecycle PR.

Verification with the corrected link graph:

• go test -count=1 ./pkg/txn/client: pass
• go test -race -count=1 ./pkg/txn/client: pass
• focused CDC/frontend/compile admission regressions: pass
• go vet ./pkg/txn/client: pass
• RunSQL enter/exit benchmark: ~80-95 ns/op serial, ~200 ns/op parallel, 0 allocs/op

GitHub does not allow the PR author to submit an Approve/Request Changes review on their own PR, so this comment records the current request-changes verdict. Once the tooling/scope item is removed or fixed, the txn code itself is ready from this review.

[LeftHandCold]

Re-reviewed the latest head. The previous txn lifecycle blockers are fixed: the terminal gate remains sealed after close, queued admission promotion now has a linearized owner, every post-open failure crosses the same admission barrier, and legacy token 0 behavior is preserved. The full txn/client suite, the full race suite, and the focused concurrency regressions pass locally.

One P2 remains in the tooling included in this PR: the new CGo wrapper's Linux external link contract omits CRoaring. Since that wrapper is documented as the supported way to test CGo-transitive packages, the included tooling should be fixed (or split out) before merge.

[LeftHandCold]

[P2] Please include libmo's CRoaring dependency in the Linux external link graph. This wrapper unconditionally injects -lmo, but a CGo-transitive target such as ./pkg/txn/client does not import cgo/lib.go and therefore does not inherit its -lroaring directive. On Linux the documented wrapper command consequently reaches the final link with unresolved roaring64_bitmap_* symbols from libmo.so. Adding -lroaring after -L$lib_dir makes that link succeed; alternatively, avoid forcing -lmo for packages that do not need it. Please also cover this with a Linux smoke test of a CGo-transitive package, since the current green CI does not execute the wrapper.

[XuPeng-SH]

Fixed in 6ba1a22f28. The wrapper now appends the authoritative cgo package CgoLDFLAGS after -lmo, closing both current and future ordered CPU native dependencies without maintaining a second hard-coded list.

Coverage is now part of the existing CPU Linux UT path through a minimal CGo-transitive fixture that references libmo without importing cgo/lib.go. A Docker Linux/arm64 rebuild reproduced the old roaring64_bitmap_* failure and passed with the new wrapper; macOS ordinary/race smoke and full pkg/txn/client ordinary/race tests also passed.

[aptend]

Re-reviewed latest head 37797246e5 with the self-review lifecycle gate and an additional independent review pass.

The previous blockers are closed: queued admission promotion is linearized, all post-openTxn failures cross the ownership barrier before ClosedEvent, legacy token 0 behavior is preserved, terminal calls remain sealed after close, and RunSQL generations remain isolated. Focused admission stress tests, the full pkg/txn/client race suite, frontend compatibility tests, vet, CGo wrapper smoke tests, and diff checks passed. I found no remaining high-confidence correctness, leak, hang, or unbounded-growth blocker.

I also verified the #25206 history: PR #25583 was closed because merged PR #25651 had already replaced the unbounded cleanCommitState retry with bounded, context-aware recovery. The issue remains open only because #25651 referenced #25650, so the existing issue association is not hiding an unfixed defect.

[aptend]

Approved after re-reviewing latest head 37797246e5. The previously requested lifecycle and admission fixes are complete, and I found no remaining merge-blocking issue.

[gouhongshen]

Request changes — independent re-review of head 3779724.

发现两个仍需合并前处理的 P2：

1. 两个导出 API 的签名被直接改写：pkg/cdc.EnterRunSql 从 func(...) func() 变为 func(...) (func(), error)，compile.MarkQueryRunning 从无返回值变为返回 error。PR 的 API-change 仍未勾选；仓外调用方会直接 source break。请保留旧 API，并通过新增 Try/WithError API 传递 admission error，或明确声明并按 API 变更流程处理。

2. 新增 CGo wrapper 的链接图不完整：它在第 99 行无条件注入 -lmo，但没有注入 -lroaring；而 cgo/Makefile 和 cgo/lib.go 都明确把 libmo 与 CRoaring 链接。Linux 上按该 wrapper 运行 CGo-transitive target 时，最终链接可能出现 roaring64_bitmap_* 未解析。请补齐 -lroaring 并检查对应前置库，同时同步 reference 命令，或避免对不需要 libmo 的 target 强制链接。

本地验证当前 head：pkg/txn/client 普通测试、race、go vet、git diff --check 均通过；cdc/compile/frontend 的 CGo 编译受本机缺失 roaring/usearch/xxhash headers 阻塞。

[gouhongshen]

[P2] Preserve the exported EnterRunSql API. This PR changes pkg/cdc.EnterRunSql from func(...) func() to func(...) (func(), error), which is a source-breaking change for external callers while API-change is not declared. Keep the old symbol and add an additive error-returning helper, or explicitly version/document the API break.

[XuPeng-SH]

Fixed in 6ba1a22f28 with an additive API. cdc.EnterRunSql retains the original func(...) func() type and preserves opaque legacy tokens, including zero. The CDC production path now uses the new TryEnterRunSql(...) (func(), error) helper for rejection-aware admission.

Compile-time signature assertions and a legacy zero-token cleanup regression lock the old contract; the sealed-transaction regression covers the new path. Focused race -count=50, full ordinary/race pkg/cdc, full pkg/frontend, vet, and make build passed.

[gouhongshen]

[P2] Preserve the exported MarkQueryRunning API. Its return type changed from no result to error, so external callers no longer compile. Please keep the existing signature and expose the rejection-aware path additively, or explicitly declare this API break.

[XuPeng-SH]

Fixed in 6ba1a22f28 with an additive API. MarkQueryRunning(c, txn) keeps its original no-result signature and legacy non-rejecting behavior. New internal callers that must observe admission rejection use TryMarkQueryRunning(c, txn) error. Both share one validate-then-publish implementation without per-call closures.

Compile-time signature assertions plus runtime tests cover the legacy token-0 path and the rejection path's rollback of process-running state. Focused race -count=50, full ordinary/race pkg/sql/compile, vet, and make build passed.

[gouhongshen]

[P2] Complete the libmo link graph. cgo/Makefile and cgo/lib.go link libmo with -lroaring, but this wrapper injects only -lmo. On Linux, the documented wrapper path for a CGo-transitive target can fail the final link with unresolved roaring64_bitmap_* symbols. Add -lroaring (and validate the library prerequisite), and keep the reference commands consistent.

[XuPeng-SH]

Fixed in 6ba1a22f28. The wrapper now closes the complete ordered libmo CPU dependency graph after -lmo. Rather than hard-coding only -lroaring, it queries the authoritative cgo package CgoLDFLAGS via go list, so future native dependency changes do not drift from the wrapper.

I also added a minimal pure-Go → CGo transitive fixture and wired it fail-closed into the existing CPU Linux UT path; GPU remains on its explicit CUDA/cuVS contract. Verified with fresh macOS ordinary/race smoke and a Docker Linux/arm64 rebuild: the old link graph reproducibly failed on roaring64_bitmap_*, while the new wrapper passed. Reference commands and both skill mirrors were updated consistently.

[XuPeng-SH]

Blocking

I did another deeper pass on the latest head 6ba1a22f28, and I found a new high-confidence lifecycle hole around deferred rollback + restart reuse.

Problem

RestartTxn can be admitted even when the deferred rollback after running-SQL timeout never successfully reached TN.

Concrete failure path

1. A public terminal call times out waiting for another running SQL, so scheduleRollbackAfterRunningSQL takes ownership (pkg/txn/client/operator.go:1944-1970).
2. That path always flips terminalCleanupDone = true in a deferred epilogue, regardless of whether tc.rollback(ctx) succeeded (operator.go:1956-1960).
3. Inside rollback, a send / handleError failure still goes through the deferred local close path (operator.go:1162-1185): the operator is marked closed and local unlock runs, but the rollback RPC itself was not confirmed.
4. claimRestart only checks closed plus terminalCleanupDone for this terminal-action path (operator.go:2176-2207). Once those two bits are set, restart is admitted.

So a rollback attempt is currently treated as equivalent to rollback completion.

Why this is a real bug, not just a suspicious pattern

I verified it with a deterministic scratch repro in a throwaway worktree:

• I forced the deferred rollback send path to fail by making the test sender return assert.AnError for the rollback request.
• After the running SQL drained, the old-generation operator became closed and terminalCleanupDone == true.
• client.RestartTxn(...) then succeeded immediately instead of returning ErrTxnClosed.

That means the current head really does allow operator reuse after a failed deferred rollback transport/send path.

Why this matters

At that point the old generation's remote rollback has not been confirmed, but the operator is already reusable for a new txn generation. Even if local cleanup ran, the old TN-side txn lifetime has not been durably resolved, so restart is running ahead of remote cleanup.

Suggested fix

Do not mark terminalCleanupDone on rollback attempt alone.

A restart should become admissible only after one of these is true:

1. the deferred rollback/unlock path completed successfully, or
2. rollback outcome was explicitly handed off to a durable unknown-result / background cleanup path whose contract allows safe restart before final remote cleanup.

At minimum, please add a regression that forces the deferred rollback send path to fail and proves RestartTxn is still rejected until that cleanup state is actually safe.

[XuPeng-SH]

Addressed the blocker in 4205dcd.

The finding was valid. The root invariant is not “the deferred callback returned”; it is “the previous operator generation has no unresolved terminal responsibility.” The previous terminalCleanupDone bit represented execution completion, so it could not distinguish a successful rollback from a failed workspace, TN, or unlock cleanup.

The fix is generation-wide rather than specific to the reported deferred-send case:

• Replaced the done bit with an explicit terminal outcome: none, pending, succeeded, or failed.
• RestartTxn now admits reuse only from succeeded. There is no durable/background cleanup handoff in this path, so pending and failed remain sealed.
• rollback now publishes pending before taking cleanup ownership, attempts workspace rollback, TN rollback, and unlock without short-circuiting later cleanup, and publishes succeeded only when the complete path returns successfully.
• The same outcome gate covers synchronous Commit, Rollback, and WriteAndCommit failures, closing the equivalent non-deferred reuse hole found during self-review.
• Commit unlock failure is handled according to transaction facts: it does not rewrite an already durable Commit into an error or roll back finalized workspace objects, but it does mark the generation non-reusable because lock cleanup is unresolved.
• Admission failures remain reusable only because they fail before user execution/TN work and already cross the admission ownership barrier.

Regression matrix added:

• deferred cleanup phase: pending, success, failed;
• failure layer: workspace, TN rollback send, unlock;
• synchronous terminal owner: Commit, Rollback, WriteAndCommit;
• durable Commit plus failed unlock: Commit remains successful and workspace finalizes, Restart is rejected;
• successful WriteAndCommit: Restart remains admitted;
• repeated Restart attempts and post-failure SQL admission remain rejected without changing the txn generation.

Fresh validation on the final commit:

• focused terminal/restart matrix under -race -count=50;
• full pkg/txn/client ordinary suite;
• full pkg/txn/client race suite;
• go build and go vet for pkg/txn/client with readonly module resolution;
• focused downstream pkg/frontend transaction/RunSQL tests;
• git diff --check.

Reflection: the earlier review boundary was still too local. It modeled the detached callback lifecycle, then used “done” as a proxy for generation safety. The correct review product is terminal owner × completion outcome × cleanup layer × reuse generation. Once reuse is the authority, success must be proven by the full old-generation closure, not inferred from local control-flow completion.

[XuPeng-SH]

Self-review PASS on 4205dcd051. I found no remaining merge-blocking correctness, concurrency, resource-lifecycle, compatibility, or performance issue.

The latest terminal-outcome gate closes the remaining restart gap: cleanup stays Pending while deferred rollback owns it, successful workspace/TN/lock cleanup publishes Succeeded, and any unresolved cleanup keeps the generation permanently non-restartable. I also rechecked the full ownership graph for max-active promotion/cancellation, client Close, timestamp waiter pool reuse, RunSQL generation boundaries, public terminal-call serialization, and downstream frontend/compile/CDC admission.

Validation completed:

• deterministic CGo-transitive Linux smoke passed with the wrapper resolving the authoritative -lusearch_c -lroaring dependency order;
• full pkg/txn/client ordinary and race suites passed;
• latest terminal-outcome regressions passed under -race -count=20;
• frontend, compile, and CDC rejection/legacy-compatibility regressions passed under -race -count=10;
• go vet ./pkg/txn/client and git diff --check passed;
• RunSQL enter/exit and timestamp no-wait hot paths remain zero-allocation.

Non-blocking metadata note: the PR body still says issue #25206, but that issue is an unrelated lock-allocator retry bug already closed as fixed by #25651. Please update the issue linkage before merge.

GitHub does not allow the PR author (XuPeng-SH, the currently authenticated account) to submit an APPROVED review on their own PR, so this comment records the self-review verdict; an external reviewer still needs to approve the latest head.