fix(cn): cancel background registration and trace work on shutdown - #29380
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
There was a problem hiding this comment.
Deep review of 13d6822a3f2c18a881544bededea0d0e91b33791 against base/merge-base 426cf10b000b6a828e8c69f1af26082f26036ed3.
Result: changes required — one confirmed P1 regression. Submitted as COMMENT because this is my own PR. The earlier no-blocker conclusion missed the canceled CSV flush with queued events.
The inline finding shows that cancellation exits only the flush closure after its file is closed. The outer handler can then select queued work and reach a fatal write/flush on that closed file. The caller must propagate the terminal result and exit the handler from both flush call sites.
Review coverage:
| Closure | Ownership / waits / bounds checked | Result |
|---|---|---|
| CN cron registration | Startup/heartbeat return, task lock, stopper admission/join before storage close, bounded attempts, retry cancellation, system identity, duplicate metadata handling and generation guards | No additional blocker found |
| Trace background I/O | Watch, three filter refreshes, MO/S3 loads, retry delay, txn-error write; context flow into SQL/visibility waits and file-service operations | Cancellation propagation checked; public refresh entry points preserved |
| Trace CSV producer | Open → write → flush/close → load queue → cancellation; queued events and timer branch; final file cleanup | P1: handler resumes with a closed writer |
| Tests and cost | Existing normal/race/stress evidence, white-box phase barriers, no added cluster fixture or SQL hot-path work | Existing tests omit canceled flush with event backlog |
Validation: reused the recorded owning-package normal/race runs, six individual race stress runs and incremental static checks for the unchanged PR files. The new review probe used a Go overlay matching the current head, an observable barrier at the blocked load-queue send, one queued event, and normal context cancellation. A logger fatal hook captured the fatal as a panic rather than terminating the test process. TestReviewCanceledFlushWithBacklog failed: 15 of 32 shutdowns reached failed to flush csv file with file already closed (package 0.055s). This is a reachable select branch, not merely a timing suspicion. No production files or PR commits were changed by the review.
This is an ordinary lifecycle bug fix, so the feature-design-document gate does not apply. No wire/catalog/API change or new unbounded worker accumulation was found. Latest CI build is successful; UT, coverage, SCA and BVT are still in progress and are not counted as passes here.
| table: tableName, | ||
| }: | ||
| case <-ctx.Done(): | ||
| return |
There was a problem hiding this comment.
[P1] Propagate canceled flush to the outer event handler
When loadC is full (or its consumer has stopped), this cancellation branch returns only from flush, after f.Close() has already run. It skips both sum = 0 and open(), leaving the outer loop with a nonzero sum and a writer pointing at the closed file. If there is an event backlog or a ready ticker, the next select can choose that branch despite the canceled context; w.Write/w.Flush (or the next f.Close) then calls logger.Fatal, terminating the process during shutdown.
A white-box probe that waits until the closed-file load send is blocked, queues one event, and cancels reproduced Fatal: failed to flush csv file / file already closed in 15/32 runs on this head. Have flush return a terminal/canceled result and immediately return from handleEvent at both call sites; add coverage for cancellation with a blocked load queue and queued events. Merely having ctx.Done() in the outer select does not prioritize cancellation.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of 51e23129639d7bd76998ba2c20d65f07471bef8c against current base/merge-base 426cf10b000b6a828e8c69f1af26082f26036ed3.
Result: no blocking findings; recommend merge from code review. Submitted as COMMENT because the authenticated reviewer is also the PR author. The previous P1 in the earlier review is addressed.
Correctness and unhappy paths
Reviewed all 11 changed files and followed the affected owners, callers, storage operations, and shutdown ordering. This is an ordinary lifecycle bug fix; the feature-design-document gate does not apply.
| Closure | Review result |
|---|---|
| CN lineage-GC registration | Registration leaves the synchronous startup/heartbeat path and runs under the CN stopper. System identity, cron metadata and duplicate registration semantics are preserved. Transient failure retries; successful registration terminates the worker. Cancellation reaches the attempt and retry wait. Runner readiness/revocation guards prevent later retries after retirement. The stopper joins this worker before task storage closes. |
| Trace watch/filter/load/error I/O | Stopper context flows into the watch fetch, three filter refreshes, MO/S3 writes, and txn-error write. Existing operation deadlines remain. Load retry delay observes cancellation. Public refresh entry points retain their caller-visible interface. SQL executor commit/rollback paths receive the operation context; S3 file ownership remains protected by defer. |
| CSV flush cancellation | Both size-triggered and timer-triggered flush callers now exit when flush returns false. The closed file reference is cleared, and observed cancellation prevents reopening. A blocked load send therefore cannot return to queued events or another timer with a closed writer. The new test covers both triggers, with a pending event, controlled cancellation and a fatal-write oracle. |
| Ownership, waits and bounds | Q1: task/ticker/context/file cleanup has an identified owner. Q2: the changed background I/O, enqueue and retry waits have cancellation paths. Q3: one registration worker per runner, existing bounded queues, no per-retry goroutine growth. Existing best-effort trace shutdown semantics remain; this is not a lossless drain guarantee. |
Repeatability, performance and engineering quality
The tests use blocked fake storage/executors and explicit phase barriers to exercise failure and cancellation, with live registration/retry controls. The flush regression has both event and timer cases. The production change uses the existing stopper and small context-aware helpers; it adds no framework, cluster fixture, public configuration, wire format or catalog migration. No new foreground SQL I/O or hot-path worker allocation was found. Registration attempts retain a 30-second context deadline and a 10-second retry ticker; that deadline is not a universal bound on all CN shutdown work or OS I/O. No throughput improvement is claimed without a benchmark.
Evidence checked
- Reused the previously recorded owning-package normal/race results, six individual concurrency tests at
-race -count=100, and incremental vet/lint evidence for the semantically unchanged portions. For the flush follow-up, reused the focused normal/race-100 test, full trace package normal/race runs, and isolated shutdown coverage test recorded for this head. No unnecessary full-suite rerun was added by this review. - Independently checked the exact-head CI run: Required, Linux UT, Linux/arm64 SCA, UT coverage, merged coverage, PROXY BVT and PESSIMISTIC BVT passed. Skipped jobs are not counted as passes.
- Downloaded and inspected the UT checkpoint/summary and coverage incident report. The UT command explicitly selects both owning packages with
-race; its stage completed with status 0. The full isolated package also passed in the embedded race stage: 566.76 seconds.TestIssue28012And28013AcceptedCommitDuringStandaloneShutdownpassed in 80.07 seconds there and 21.62 seconds in coverage. The coverage producer exited 0 with no failed package/test event. git diff --checkpassed; the review worktree is clean. Remote head/base were rechecked before submission; the PR is mergeable.
The demonstrated cancellation gaps and prior closed-file regression are closed. This does not establish that the separate aggregate-runtime issue #29368 is solved: 566.76 seconds still leaves little margin against the former 600-second package budget, and the current CI uses a larger timeout. The PR body's statement that CI/full isolated validation is still pending is now superseded by the results above.
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #29371.
Related: PR #29290 (the CI shutdown trace) and #29368 (the isolated-package 10-minute timeout is a separate aggregate-runtime issue).
What this PR does / why we need it:
The shutdown stacks exposed two independent background-I/O cancellation gaps. CN task-runner initialization registered the lineage-GC cron synchronously with
context.Background()while handling startup or a heartbeat command. A stalled task-storage call could block command acknowledgement andStopper.Stop; the observed MySQL read timeout was 15 seconds. Separately, txn-trace's watch fetch, filter refresh, load writes, and txn-error write derived long timeouts fromcontext.Background(), so CN close could wait for those operations even after its stopper was canceled. The CI trace showed roughly two minutes spent closing CN; it does not establish that these calls caused the unrelated 10-minute package deadline.This change runs lineage-GC registration as a stopper-owned task. Each attempt has a 30-second deadline, transient errors retry, and stopper cancellation ends blocked attempts and retry waits. The system account context and registration metadata stay intact. Txn-trace background work now derives operation deadlines from its stopper context; load queue sends and retry waits also observe cancellation. Deterministic tests block the storage/executor calls and verify prompt shutdown, retry, and live registration. A follow-up fixes a flush cancellation path found during deep review: the trace event handler now exits after a canceled CSV load send from either event-size or timer flush, clears its reference to the closed file, and skips reopening when cancellation is observed. A deterministic test covers both triggers with a pending event and a blocked load send.
Validation on base
fdfbea4fb7(Go 1.26.4, Linux/amd64):make cgo— pass.mo-cgo-test -count=1 ./pkg/cnservice ./pkg/txn/trace— owning packages passed separately.mo-cgo-test -race -count=1 ./pkg/cnservice ./pkg/txn/trace— owning packages passed separately.-race -count=100runs — pass.mo-cgo-test -short -tags matrixone_test -count=1 -timeout=20m -covermode=set -coverpkg=./pkg/cnservice,./pkg/txn/trace,./pkg/tests/issues/isolated -run '^TestIssue28012And28013AcceptedCommitDuringStandaloneShutdown$' ./pkg/tests/issues/isolated— pass (110.124s).go vetand golangci-lint 2.5.0 (built with Go 1.26.4) forpkg/cnserviceandpkg/txn/trace,gofmt,git diff --check— pass; lint reported 0 issues.The first coverage retry was blocked by another test's fixed port 10060. It was rerun after that process released the port and passed.
Follow-up validation on main merge base
426cf10b000b(Go 1.26.4, Linux/amd64):-race -count=100— pass.pkg/txn/trace: normal and race runs — pass.TestIssue28012And28013AcceptedCommitDuringStandaloneShutdownisolated coverage run — pass (27.012s test execution).go vet, golangci-lint 2.5.0,gofmt, andgit diff --check— pass; lint reported 0 issues.51e2312963.CI and the full isolated package remain to run on this PR.
Risk: cron registration is now asynchronous; it can complete after command acknowledgement. It uses one stopper-owned worker per runner and is joined before task storage closes. Reverting this commit restores the previous behavior.