fix(disttae): retain unpublished S3 cleanup ownership

[XuPeng-SH]

What type of PR is this?

• BUG

Which issue(s) this PR fixes:

Fixes #29257.

What this PR does / why we need it:

CN S3 objects can be persisted before their names reach a transaction workspace. This PR retains cleanup ownership through local writers, Insert, MultiUpdate, remote DELETE, transaction dumps and transfers, tombstone transfer, clone, rollback, and CN teardown. Remote workers release ownership only after an ACK-capable coordinator has retained the names and issued contiguous ownership receipts.

• A per-CN ledger caps unpublished object-name tickets at 65,536. A sinker reserves a ticket immediately before object Sync; capacity exhaustion rejects the new write before upload. The receiving CN reserves its ticket before a remote ownership receipt. Confirmed workspace acceptance or successful/absent-object deletion releases the ticket. An ambiguous Sync or Delete retains it.
• Failed cleanup transfers a name-only owner or lightweight writer/flow shell to the transaction or CN retry worker. Sinker buffers, batches, free lists, and session mpools are released. A cleanup error does not overwrite an already-owned SQL error.
• Multi-batch progress: after a confirmed Delete batch, the owner releases exactly those names and tickets even if a later batch fails. A failed batch is retained in full because its partial effects are unknown. The next attempt starts at the remaining names. The TN sinker API keeps its full-snapshot count and error behavior.
• The single CN retry worker processes ready tasks immediately after success, rotates failed tasks, and waits one second after a failure. Queue metrics keep an immediate pending count and sample the oldest age at most once per second instead of scanning the backlog on every operation.
• Metrics expose tickets used/high-water, admission failures, pending retry tasks, oldest retry age by CN service, and process-level Delete failures.
• Partitioned MultiUpdate writers now stay on the coordinator because PartitionMultiUpdate has no remote instruction codec. Remote source scopes are grouped by CN before moving the writer; this preserves shuffle receiver ownership and writer parallelism without changing the wire protocol.

Validation

• A deterministic test failed on the original PR head 97d5905342: after a confirmed 1,000-key Delete batch followed by a failed one-key batch, the ledger still retained 1,001 tickets rather than 1. It now passes and checks physical deletion and capacity reopening using MemoryFS.
• New partition-writer placement tests failed against the previous PR head for both remote S3 and table writes. On this head, they prove local writer placement, remote source codec compatibility, parallel writers, and standalone grouped shuffle sources.
• Fault tests cover the workspace owner, detached writer and transfer-flow shells, ambiguous Delete responses, repeated cleanup, ticket release, queue rotation, enqueue-versus-close, and metric age. Direct remote ownership, rollback, and CN-close consumers passed.
• On exact head c7aa7b3bc6568a905483f3c4651ab65e877f5114, full -count=1 CGo tests passed for all 13 Go packages changed by the PR: cnservice, objectio/ioutil, pb/pipeline, sql/colexec, its deletion, insert, multi_update, and table_clone packages, sql/compile, util/metric/v2, disttae, vm/pipeline, and vm/process. The new main branch's arena-pool tests also passed.
• An isolated log/TN/two-CN deployment built from this exact head passed a partitioned UPDATE of 5,000 rows with optimizer_hints='execType=3'. EXPLAIN reported AP on multi-CN; the hint sets Outcnt=100,000,000, selecting the S3 branch. CN1 debug logs recorded RemoteRun to CN2 and NewS3Writer for all four physical partitions; COUNT=5,000, SUM(v)=5,000, MIN(v)=MAX(v)=1 verified each row updated once. A normal TP plan with 50,000 rows also passed.
• Focused queue race tests passed in one process at 100 repetitions for the immediate/concurrent cases, 29 for unresolved close, and 30 for failed-task rotation, selected from measured test durations. After rebase, full ioutil, colexec, and disttae race tests passed (disttae: 1,005 passed tests); focused remote cleanup/CN-close race tests passed. The faster worker exposed two old test assumptions about a one-second startup delay; explicit second-Delete barriers now verify handoff before physical deletion. Those exact remote and rollback tests passed 61 and 65 race repetitions respectively.
• Post-rebase gofmt, git diff --check, go vet, and repository-pinned golangci-lint passed for the changed package closure (0 issues). Repository molint exited 0 on the changed closure and again after the final test fix; its recover() diagnostics refer to unchanged code.
• The same eight-callback queue benchmark on the same host measured about 7.0 s with the old one-task-per-tick worker and 9.8 µs with immediate successful drain (-benchtime=1x). This isolates the prior ticker delay; it is not a production latency guarantee.

Design, rollout, and limits

The design document specifies the admission cap, ownership handoff, bounded cleanup and confirmed-batch refinement. Independent design and implementation reviews found no remaining concrete blocker for this revision.

Roll out ACK-capable coordinators before workers that require receipts; mixed-version remote writes remain fail-closed without that order. Configure alerts for ticket use above 80% for five minutes, sustained cleanup age above five minutes, and Delete failures.

The ledger is in memory and covers live-CN failures; a process crash can still lose names. CN shutdown keeps dependencies alive if its bounded cleanup expires, and the top-level close path is one-shot. The ten-minute terminal cleanup budget and crash durability require separate lifecycle/design work before making a stronger guarantee.

Latest-main rebase and CI

Rebased onto matrixorigin/main@7ce783464806490263c184f22aaf5d307225adef; head c7aa7b3bc6568a905483f3c4651ab65e877f5114. The 17 original PR commits were equivalent across the prior rebase; two subsequent commits fixed test timing and partitioned writer placement. Independent full-diff review found no concrete code blocker. CI on this head is pending; local verification above is complete.

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

GPT-6 High force review at exact head 3551641.\n\n[P2] Repeated multi-update spills retain each one-shot S3 writer and its sinker buffers until operator reset. sortAndSyncOneTable appends every writer to ownedWriters (pkg/sql/colexec/multi_update/s3writer_delegate.go:683); meanwhile cleanDeleteBatches releases the delegate throttler grant after each spill (lines 269-285). The accumulated writers are only closed by reset/free (lines 901-915, 963-1004), so subsequent spill cycles can retain completed writers’ freelists/arenas after their memory is no longer charged to the throttler. Keep the transaction cleanup ownership lightweight (e.g. retain persisted-object cleanup records separately) and release a successfully synced one-shot writer’s buffers promptly; add a multi-spill test that checks retained memory/bounds.\n\nReview scope note: I traced the changed ownership handoff and relevant cleanup paths, but did not run new tests; full async sink/I/O termination and all real multi-CN cancel/teardown combinations were not independently closed in this review. At review time UT, SCA, coverage and multi-CN BVT checks were still in progress. This is a self-authored PR, so this is a COMMENT rather than an approval/request-changes decision.

[gouhongshen]

Codex automated review

Reviewed the exact PR head and full diff. No new blocking or non-blocking findings beyond already-raised reviewer threads. The coordinator retains S3 ownership before forwarding, failed or missing ACKs keep the worker cleanup-owned, workspace name deduplication prevents double ownership, producer handoffs occur before writer release, and retry-queue shutdown/join is race-safe. Local focused test execution is unavailable because required CGO headers are missing; gofmt passes for all changed Go files.

[XuPeng-SH]

GPT-6 High review of exact head f583a5f. This is a COMMENT review because XuPeng-SH authored this PR. The technical assessment remains blocked by previously reported items still present at this head.

1. P2 — Cleanup failure still masks the primary SQL error. remoterunServer.go:249/252 joins handlerErr with cleanupErr, including after successful transfer to the CN retry worker. sendError -> EncodedMessageError -> ConvertGoError converts that joined value to an internal error instead of preserving the original MySQL error code. transfer.go:192 has the same pattern. Preserve the primary error identity and add an RPC regression for a business error combined with cleanup failure.

2. P2 — The outage cleanup backlog remains unbounded. RetryUnpublishedS3Cleanup appends failed stream/rollback tasks, and failed attempts retain/rotate callbacks. These callbacks keep object-name owners and potentially writer buffers until Delete succeeds. Continued work during persistent Delete failures therefore accumulates retained memory without admission control or durable transfer; one worker and per-attempt timeouts only bound concurrency/time, not backlog size.

3. P2 — The mandatory design decision remains open. The current ownership document explicitly says it is retrospective and not approved, and leaves mixed-version behavior, in-memory recovery and outage-backlog bounds for independent approval. Please record a decision for this exact revision and close the blocking resource/rollout questions. The receipt protocol and background retry worker trigger the repository design-first gate.

The earlier terminal local rollback handoff and clone retry-context fixes are present. CI run 35984658556 completed successfully for this exact head, including build, Linux UT, SCA, coverage and both active multi-CN BVT lanes. No new local tests were run. Full implementation approval is deferred by the open design gate; this comment does not claim complete implementation/lifecycle closure.

[XuPeng-SH]

Design decision record for the rebased head bbf324b30bdd9bc180eae8cf961c6ec0353b982f:

The requester, XuPeng-SH, explicitly approved the current design document docs/design/unpublished_s3_object_ownership.md (blob 528fd66d919b3f4bb9840cca1898362478fab4e4) and authorized pushing the validated rebase on 2026-09-24. The chosen outage boundary is upload-time admission: when the retained cleanup quota is full, related new S3 writes pause/fail admission rather than growing the backlog. This records the product/design decision; CI for the rebased head is still pending.

CI root cause fixed in this push: the remote S3 takeover test fixture now registers the CN admission service required by the bounded ownership path, while preserving the production fail-closed behavior. The merge with current main regenerated pipeline.pb.go so main's SessionInfo fields and this PR's ACK field coexist. Focused 18 takeover subcases, full pkg/sql/compile and pkg/sql/colexec/multi_update packages, targeted cnservice conflict tests, and go vet passed locally.

[XuPeng-SH]

Rebased-head CI triage at bbf324b30bdd9bc180eae8cf961c6ec0353b982f:

• Shared build passed. The previous deterministic TestRemoteS3ReceiveTakeoverBeforeAck fixture failure is fixed locally; all 18 subcases and the full affected compile and multi_update packages passed after rebase. The new Ubuntu UT job is still queued.
• Both multi-CN BVT jobs stopped during Compose's Quay MinIO/mc image pull (HTTP 401), before SQL. Shared infrastructure issue CI: multi-CN BVT blocked by unauthorized Quay MinIO images #29367 has a proposed fix in ci(compose): build verified MinIO releases locally #29370; its remote CI has already completed Compose startup and entered SQL BVT.
• UT Coverage stopped before running Go tests because its Java 8 setup step timed out downloading Adopt JDK. The independent CI issue is Coverage UT can time out downloading unused Adopt JDK 8 before tests CI#460; ci(coverage): skip unused Java installation CI#461 removes unused Java setup from Coverage. The separate Go package-budget defect remains tracked in CI: isolated UT Coverage package exceeds 10-minute deadline during cluster shutdown #29368 / ci(coverage): allow 20 minutes per Go test package CI#459.

These external failures do not validate or invalidate the rebased S3 ownership code. Full CI remains pending the shared CI fixes.

[XuPeng-SH]

Deep review of exact head 58ce2d4ddef543fd2503d00d95885dae5bf82ca4 against 47ec0605c1deaeec3eda1a7ffcba2cb79327fa7e. Changes needed (P2): the new remote S3 ownership classifier misses PartitionMultiUpdate when a partitioned UpdateWriteS3 scope runs on another CN. See the inline finding for the complete producer → receiver → ACK → worker-finalizer path. This is an availability regression for distributed partitioned S3 writes, and the current TestScopeS3Output covers only the unwrapped operator. Please classify the wrapper and test the actual metadata receipt before the ownership ACK.

I checked the pre-Sync ticket boundary, failed/ambiguous Delete retention, confirmed-batch progress, name-only retry state, terminal rollback/remote cleanup, ACK ordering, and accepted-name deduplication. I found no second concrete blocker in those paths. The approved in-memory/crash and mixed-version rollout limits still apply. Exact-head CI is green (build, Linux UT/coverage/SCA, and active multi-CN BVT). Focused local race tests passed for colexec and disttae; the local compile race binary could not link because /tmp ran out of space, so I rely on the exact-head CI for that package. This account authored the PR, so this is a COMMENT review rather than a GitHub request-changes state.

[XuPeng-SH]

P2 — Include partitioned S3 producers in ownership classification. constructMultiUpdate returns *multi_update.PartitionMultiUpdate for partitioned targets (operator.go:1044-1052), and PartitionMultiUpdate.writeS3 delegates to raw.Call and returns its S3 metadata output (multi_update_partition.go:226-247). After a remote Connector/Dispatch is traversed, this switch sees the wrapper, not *MultiUpdate, and returns remoteS3None. The coordinator then neither retains the names nor sets field 19 on its ACK (remoterunClient.go:366-377); the worker's still-pending owner makes finalizeUnpublishedS3Objects return NotSupported and clean up, so distributed partitioned S3 writes fail even when S3 is healthy. Classify the wrapper when its raw action is UpdateWriteS3, and add a regression that drives the wrapped producer through receipt and ACK.

[XuPeng-SH]

Deep review — d8352ece2d3ee83810125581ddc90acc57769e8f

Technical assessment: one remaining P2 validation gap; no new demonstrated correctness blocker in the supported live-CN failure paths. This is a COMMENT review because the authenticated account is the PR author.

P2 — Complete the performance evidence required by the accepted design

The design's explicit proof requirement includes retained bytes per ticket and repeated-spill hot-path cost. The published validation reports the eight-callback retry-queue benchmark, but supplies neither of those results. The existing BenchmarkUnpublishedS3AdmissionRetainedBytes allocates object-name strings before the measurement baseline and measures only the admission map; it does not establish total retained cleanup-owner/queue memory at the 65,536-ticket cap. The repeated-spill UT proves that writer mpool buffers are released, but does not measure CPU/allocation overhead.

Please attach reproducible measurements for representative retained cleanup records at capacity and a comparable repeated-spill workload, identifying source revision, commands, workload, memory/allocations and timing. This requests the evidence already required by the design; it does not require a new queue or durable-journal design. No measured performance regression is asserted here. Until that evidence exists, the performance/capacity validation is incomplete.

Audit of all 11 inline comments

“Fixed” below describes current code; all 11 GitHub threads are still marked unresolved.

Prior comment	Current disposition
Remote cleanup masks the primary SQL error	Fixed. Successful retry handoff returns the original handler error unchanged; the regression test checks its RPC error code.
New worker / old coordinator	Addressed by a documented deployment constraint. Coordinator-first upgrade and draining old workers remain required. There is no scheduling capability gate; arbitrary mixed-version write availability is not provided.
Transfer cleanup masks the original error	Fixed. The original error survives while cleanup ownership is retained.
Remote DELETE error joining	Fixed. The failed writer is retained and the primary cause is returned.
Garbage cleanup fails a valid commit	Fixed. After successful CN cleanup handoff, Commit proceeds according to its business outcome.
Canceled rollback context	Fixed. Terminal cleanup detaches request cancellation/deadline and preserves the shared bounded teardown budget.
Successful cleanup handoff reported as rollback failure	Fixed. Successful queue handoff clears the unpublished cleanup error.
Full writers/buffers retained during S3 outage	Fixed in the traced production paths. Failed cleanup retains names/lightweight shells; writer, flow and MultiUpdate buffers are detached.
Unbounded backlog / one-task-per-tick drain	Root concern addressed with a different mechanism. Pre-upload tickets bound outstanding names; successful tasks drain immediately. Failed tasks rotate with a fixed delay. Per-task exponential backoff was not implemented.
Document that inline ACK processing must not block	Small documentation follow-up remains. The comment explains ordering, but does not pin the requested no-I/O/no-blocking-wait invariant. The normal valid ACK path currently updates flow state under a mutex and notifies waiters.
PartitionMultiUpdate remote ownership classification	Fixed through placement. Partition writers execute on the coordinator; remote sources are grouped by CN before wrapping, preserving shuffle ownership and parallel writers without adding a missing remote operator codec.

Earlier review-body findings about final rollback losing cleanup ownership, clone retries reusing an expired context, nested cleanup budgets, and repeated spills retaining complete writers are also addressed. The exact design approval is recorded here; I am not reopening that approved design choice. The MultiUpdate double-decode optimization remains explicitly deferred pending profiling.

Unhappy paths, scope and engineering judgment

Audit	Closure checked	Result / boundary
Ownership	reserve → ambiguous/successful upload → writer → workspace / remote receipt → registration or Delete → transaction/CN retry	No new lost-owner or accepted-object deletion path established. Confirmed Delete batches release only their own names/tickets; uncertain batches remain owned.
Liveness	cancellation, Reset/Free, stream removal, ACK gaps, rollback, enqueue versus Close	Shared terminal budgets and independent retry attempts are present. Dependency shutdown follows producer drain. This assumes file-service operations honor their context.
Growth	long S3 outage, repeated spills, retry rotation, metrics	The 65,536-name cap applies before upload; successful cleanup reopens capacity. Failed cleanup releases large buffers. Total retained-memory and hot-path measurements remain the P2 above.

The shared admission hook, name-only ownership and single retry worker are a reasonable scope for the original live-CN orphan problem. I found no evidence requiring more worker layers, a generic retry framework or durable journaling in this PR. Process-crash recovery remains outside this design, and shutdown retains dependencies if its cleanup budget expires; the top-level Close path remains one-shot. These limits must remain visible in rollout claims.

Evidence checked

• Reviewed the complete 58-file diff against merge-base fdfbea4fb7125d538d8675d725ace17811760023, including generated-code consumers and the changed test contracts.
• CI run 36102808708 is green for this PR head: Linux UT, arm64 SCA, shared build, coverage and both enabled multi-CN BVT lanes. Skipped platform/upgrade jobs are not treated as passes.
• Downloaded the Linux UT checkpoint artifact and job log: all 13 changed Go packages are explicitly selected in the successful -race -short -tags matrixone_test stage (exit status 0). This verifies the actual mode/selection, not only the job name.
• Verified the generated protobuf descriptor differs only by Message field 19; git diff --check passes. Author evidence at c7aa7b3 remains relevant to the unchanged reviewed package trees after the merge into d8352ec.
• A supplementary local benchmark attempt did not execute: the CGo wrapper rejected native-artifact provenance. It supplies no performance result and is not counted as a PR test failure. Existing exact-head CI evidence was reused rather than rerunning the same broad suites.

Answer to “all comments resolved?”: the prior concrete code defects are addressed in the reviewed scope; the compatibility choice is documented, two optimization/documentation suggestions remain deferred or unimplemented, and the required performance evidence is still incomplete. GitHub thread resolution also has not been updated.

[XuPeng-SH]

P2 — Supply the performance proof required here. The current published evidence reports the eight-callback queue-drain benchmark, but not retained bytes per cleanup ticket or repeated-spill hot-path cost. BenchmarkUnpublishedS3AdmissionRetainedBytes starts its baseline after allocating the name strings and measures only the admission map, omitting retained owner/queue state. The repeated-spill UT establishes mpool buffer release, not timing/allocation overhead. Please attach reproducible capacity-memory and comparable repeated-spill measurements (revision, commands, workload, time and allocations) before treating this explicit validation requirement as complete. No measured regression is asserted, and no additional retry framework is requested.