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.

Goals and engineering principles

Maximize useful cleanup coverage at small, measured normal-path cost and proportionate implementation complexity; do not pursue unconditional elimination of every orphan object.

• A firm ownership contract for live-CN failures. Ordinary execution errors, cancellation and rollback must not silently discard the only object-name reference. Cleanup responsibility stays with a live owner until deletion succeeds or the next owner confirms takeover. This guarantees continuity of responsibility, not physical deletion by a deadline regardless of storage availability.
• Bounded best-effort recovery beyond that contract. Retry temporary Delete failures while retaining lightweight names rather than large writer buffers. Preserve the original SQL error and the transaction's business outcome after successful cleanup handoff. Process-crash recovery, permanently unavailable storage and discovery of historical orphans are outside this PR; do not add a durable journal, recovery protocol or full S3 sweeper merely to claim complete coverage.
• Protect the normal path. Keep bookkeeping proportional to produced objects, not data rows, and avoid redundant I/O, broad scans and additional synchronous waits. “No performance impact” is not assumed: CPU, allocations and retained memory must be measured, and any material regression must be fixed or explicitly justified.
• Keep the mechanism minimal. Reuse existing writer, workspace, transaction and CN lifecycle responsibilities. Add only the ownership handoff, safe deletion/retry and admission mechanisms needed to close demonstrated gaps; avoid parallel state machines, duplicate validation/storage/execution paths and generic cleanup frameworks. Added complexity must earn a clear correctness or robustness benefit.
• Make the outage tradeoff explicit. The 65,536-name admission ceiling is enforced before upload, not by dropping already-owned cleanup work. Exhaustion rejects affected new S3 uploads: this deliberately trades write availability during sustained cleanup failure for bounded retained state. It is not a zero-impact guarantee or a bound on S3 object bytes.
• Validate cost and behavior, not just CI status. Retain deterministic failure/recovery tests with accepted-object safety and cleanup ownership oracles. The required measurements of complete retained name/owner/queue state at capacity and comparable repeated-spill CPU/allocation cost remain acceptance evidence; a ledger-only or retry-ticker benchmark does not replace them. The capacity and paired-spill measurements below satisfy these evidence requirements; they do not imply zero overhead.

Current implementation and upgrade contract

Rebased onto main@37ba0712974d4a1ebe3fe8cd8e6a4daba7400f5e; current head 4cf6f8689c0e84f2b66609cf2bcc58cccce9c73d.

• Reuse the existing writer → workspace → transaction/CN lifecycle. One pending-name map now also records ticket ownership; a second per-owner charge map is unnecessary.
• Delete the unused MultiUpdate cleanup implementation and its mpool field; its replacement test drives the production MultiUpdate.Free → workspace callback → repeated failure/recovery path. Remove the redundant remote-DELETE file-service field.
• Resolve the rebase's actual protobuf field collision: main's protocol_version remains field 19, and the ownership receipt uses field 20. Independently encoded wire bytes verify that one cannot be mistaken for the other. Fresh generated output compares byte-for-byte with the checked-in binding.
• Preserve main's view-binding isolation contract through tenant/deadline/cancellation and independent cleanup-budget assertions, not a pointer-equality assertion invalidated by the derived execution context.
• Stop-the-cluster upgrade only, as confirmed by the design owner: stop old services and start all nodes on the new version before admitting work. No coordinator-first rolling-upgrade contract is claimed. Stored object formats and committed metadata are unchanged. Current-producer metadata formats and the distinction between credit ACKs and ownership receipts remain necessary within a single version.
• The design document describes only the final contract, limits and validation requirements. It does not retain the PR's design history.

The final correction commit is production −46 lines, documentation −193, tests/benchmarks +237 (net −2). The full PR still adds the ownership/admission/retry mechanism; this is not a claim that its overall diff is small. Each added benchmark addresses a previously missing proof, rather than increasing test count for its own sake.

Validation

Local validation applies to the final source tree; the commit itself made no source changes after testing. macOS arm64 / Apple M4 / Go 1.26.4, each worktree's own source-stamped native build.

• Normal tests and full -race -count=1 tests passed for all 13 changed packages: cnservice, objectio/ioutil, pb/pipeline, sql/colexec, deletion, insert, multi_update, table_clone, sql/compile, util/metric/v2, disttae, vm/pipeline, vm/process.
• Focused race stress passed at 100 same-process repetitions each for admission concurrency, enqueue versus Close, confirmed Delete-batch progress, actual MultiUpdate failed-cleanup handoff, inline ACK ingress, shared teardown budgets/prepared reset, view context isolation, and workspace name deduplication. Failed-task rotation passed 30 repetitions, selected from its measured 1-second duration.
• Incremental gofmt, git diff --check, go vet and repository-pinned golangci-lint passed (0 issues). molint produced the same 12 recover diagnostics as the clean main baseline, with no new diagnostic. Native and mo-service builds passed.
• A fresh isolated LOG/TN/two-CN deployment passed the 5,000-row public-SQL matrix twice on the same instance: partitioned AP update, rollback, repeated prepared execution, nonpartitioned AP insert, and TP control. Count/sum/min/max were compared with independently specified expected values. Debug logs confirm CN1:39230 → CN2:39250 remote execution and S3 writer creation for all four physical partitions. A third run with debug logging also passed; owned services shut down normally.
• White-box tests additionally select ambiguous Sync/Delete, post-persistence registration failure, accepted-object preservation, partial confirmed Delete progress, missing/rejected handoff, cancellation/StopSending/gapped ACKs, prepared reuse, retry recovery and CN-close failure. SQL tests establish public reachability; fault-injected tests establish the unhappy-path ownership/deletion oracles.
• CI is not being waited on. These are local results, not a claim that latest-head CI or independent review has passed.

Full retained cleanup state at capacity

BenchmarkUnpublishedS3RetainedCapacity allocates all names after the baseline and uses production receive → real transaction-detached callback → CN retry queue paths at 65,536 names. Post-GC live bytes include admission, names, owners, callback storage, queue/timestamps and FS references. Shared FS caches, S3 payloads and unrelated in-flight queries are excluded. Every sample then drains and verifies zero remaining tickets.

Name bytes	Objects/task	Retained bytes/ticket	Total retained MiB	Before map consolidation, MiB
42	1	505.1	31.57	50.57
42	64	159.1	9.94	16.40
256	1	713.1	44.57	76.57
256	64	367.1	22.94	42.40

Final medians: -benchtime=3x -count=5; pre-consolidation source is rebased d42fca1d37 with the identical capacity harness, -benchtime=1x -count=3. Capacity is fixed per iteration, not throughput-scaled. The 1-object/task cases retain 38–42% less live memory after consolidation.

Operational sizing must exceed retained live bytes: 128 MiB/CN is an initial bookkeeping-only planning allowance for these measured shapes (~2.9× the largest measured live set), not an enforced RSS bound or a guarantee for every fallback shell/GC setting. Add ordinary writer buffers, file-service caches and workload memory separately. The 80%-ticket alert is a starting threshold, not proof that every CN has sufficient memory headroom.

Paired repeated-spill cost versus main

The exact same unpublished_s3_bench_test.go (SHA256 8df413b4ef19041040e1b2c2b39ef47ed5b8459dbd1f60b1c251999cded3c9e1) runs on verified main@37ba071297 and this PR. Each operation performs four real Write → Sync → metadata → acceptance/reset cycles, each of 1 or 128 MiB logical input. PR admission is enabled; main uses its existing path. Input construction and physical deletion of accepted objects are excluded on both revisions. MemoryFS retains real encoding/compression work but does not model cloud-S3/network latency.

One warmup per revision, then 3 ABBA rounds (6 samples/revision), 3 operations/sample, GOMAXPROCS=4; medians:

Input/cycle	main time/op	PR time/op	Δ time	main → PR B/op	main → PR allocs/op
1 MiB	5.820 ms	5.991 ms	+2.94%	6,367,930.5 → 6,370,189	33,492 → 33,530
128 MiB	749.986 ms	756.990 ms	+0.93%	82,246,605 → 82,246,059.5	19,687 → 19,741

Allocation-count overhead is 0.11% / 0.27%; bytes are +0.035% / approximately flat. Time ranges overlap (1 MiB: main 5.698–9.438 ms, PR 5.819–6.753; 128 MiB: main 742.795–896.385 ms, PR 743.626–826.469). This supports small measured bookkeeping cost for this workload, not zero overhead, statistical significance or an end-to-end SQL throughput guarantee.

Reproduce capacity on this head:

.agents/skills/mo-dev/scripts/mo-cgo-test -run '^$' \
  -bench '^BenchmarkUnpublishedS3RetainedCapacity$' -benchtime=3x -count=5 \
  ./pkg/vm/engine/disttae

For the paired run, copy the exact benchmark file unchanged into a disposable worktree at the base above and build native dependencies independently. Build each package's test binary with the CGo wrapper, warm each once, then alternate the binaries in ABBA order three times:

GOMAXPROCS=4 ./disttae.test -test.run '^$' \
  -test.bench '^BenchmarkCNS3RepeatedSpill$' -test.benchtime=3x -test.count=1

Review dispositions and boundaries

The previous mandatory total-retained-memory / repeated-spill evidence gap is addressed above. The ACK ingress comment now explicitly pins no I/O / no blocking wait. Previous primary-error preservation, valid commit/rollback outcomes, canceled cleanup, buffer retention, queue growth/drain and partition placement findings remain fixed and covered by the current checks. The mixed-version concern is now outside the explicitly agreed stop-upgrade scope. MultiUpdate double-decode optimization remains deferred without profiling; it is not claimed fixed.

Self-review traced ownership, waits and growth across all changed contracts; no remaining demonstrated blocker in the requested live-CN scope. This is not independent approval, and existing reviewers' request-changes states are not dismissed.

Remaining limits are intentional: names are not durable across process crashes; no historical-object sweep is added; context-aware FileService operations are required for deadline bounds; CN shutdown retains dependencies if bounded cleanup fails, and the top-level close remains one-shot. Sustained debt can reject new S3 uploads at the admission cap.

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

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

[aptend]

Deep review at exact head d8352ec, including the full ownership/ACK/teardown change map, previous reviews, all 12 unresolved inline threads and the author's replies, and the current approved bounded-admission design. I traced writer → workspace → remote receipt → transaction/CN retry, cancellation/shutdown waits, and ticket/queue growth. Focused tests for the affected colexec, multi_update, compile and disttae paths passed on this head with its own native dependencies; CI is green. I found no independently reproducible live-CN lost-owner or hang defect in the supported failure paths.

[P2][Required validation gate] docs/design/unpublished_s3_object_ownership.md:235-236 and :347-355 explicitly require benchmarked total retained bytes per ticket, operational headroom, and repeated-spill hot-path cost before implementation approval. The current PR body reports only the eight-callback retry-drain result. In pkg/sql/colexec/unpublished_s3_admission_test.go:286-305, BenchmarkUnpublishedS3AdmissionRetainedBytes allocates all object-name strings before the HeapAlloc baseline and then measures only the admission map; my exact-head run reports 106.7 ledger-bytes/ticket, which excludes the names, name-only owners, file-service references and retry closures that remain during a Delete outage. There is no comparable repeated-spill timing/allocation result. A concrete capacity scenario is 65,536 admitted objects during an S3 Delete outage (potentially 256-byte names plus owners/queue state); the present number cannot establish CN retained-memory headroom or whether the 80% alert point is safe. Repeated 128 MiB spill cycles likewise lack measured hot-path cost for the new admission/ownership transfer. Please attach reproducible exact-revision measurements covering the complete retained owner/queue state at the ticket cap and a before/after repeated-spill workload (commands, workload, memory/B-op and time). This is missing evidence required by the approved design, not a claim that a performance regression has already been demonstrated.

[aunjgr]

Reviewed the approved design and exact head d8352ec. I found no verified lost-owner defect to add to the existing ownership review, but a mandatory acceptance proof remains missing. docs/design/unpublished_s3_object_ownership.md:235-236 and :347-355 require retained bytes per ticket, operational headroom and repeated-spill hot-path cost before implementation approval. The current retained-bytes benchmark (pkg/sql/colexec/unpublished_s3_admission_test.go:286-304) allocates all names before its HeapAlloc baseline and measures ledger entries only; it excludes the name/owner/queue/file-service state held during a Delete outage. The PR reports an eight-callback retry-ticker microbenchmark, not repeated-spill time/allocations. Please supply exact-head measurements of the complete retained state at the 65,536-ticket cap and before/after repeated spill, or obtain a recorded design-owner change to this required gate. CI green does not measure this capacity contract.

[XuPeng-SH]

Self-review of 4cf6f8689c0e84f2b66609cf2bcc58cccce9c73d rebased onto main@37ba071297: no remaining demonstrated blocker in the agreed live-CN/stop-the-cluster-upgrade scope. This account authored the PR, so this is COMMENT, not independent APPROVE.

The outstanding performance evidence is now in the PR body: full 65,536-name retained owner/queue state (31.57–44.57 MiB for one object/task, including names allocated after the baseline), and identical paired four-cycle spill workloads versus main. 1/128 MiB cycles measured +2.94%/+0.93% median time, with overlapping sample ranges and +0.11%/+0.27% allocation counts; no zero-overhead claim. Consolidating the ownership/ticket maps reduces the measured one-object/task retained state by 38–42%.

Removed the test-only duplicate MultiUpdate cleanup path and redundant fields; tested actual Free → retry failure → recovery. Preserved main's protocol field 19 and assigned receipts field 20, with independent raw-wire oracles. ACK ingress now documents its no-I/O/no-blocking-wait contract. The new main view-binding test verifies semantic context isolation instead of derived-context pointer identity.

All 13 affected normal/race package suites, targeted same-process race stress, incremental vet/lint (0 issues), baseline-compared molint, regenerated-protobuf consistency, and a fresh two-CN public SQL matrix passed. SQL results were independently checked for partition AP update, rollback, prepared reuse, ordinary-table AP insert and TP control; logs prove remote execution and all four partition S3 writers. CI is not being waited on.

Ownership/wait/growth review closes the changed writer/workspace/receipt/transaction/retry boundaries under context-aware FileService operations. Stop-upgrade/data-format boundaries, crash durability exclusion, admission's availability tradeoff and one-shot failed CN Close remain explicit in the final design. Prior mixed-version availability is no longer a requirement per the user's instruction. The unprofiled double-decode suggestion remains deferred, not claimed fixed. Existing request-changes reviews are left for their reviewers to reassess against this evidence.

[XuPeng-SH]

Deep re-review of 4cf6f8689c0e84f2b66609cf2bcc58cccce9c73d against base/merge-base 37ba0712974d4a1ebe3fe8cd8e6a4daba7400f5e.

Technical conclusion: no remaining demonstrated blocking finding in the agreed live-CN / stop-the-cluster-upgrade scope. This is a self-authored COMMENT, not independent approval. I did not spawn subagents, change production code, dismiss others' reviews, or wait for CI. The existing CHANGES_REQUESTED state still needs the original reviewers' reassessment.

Design and engineering assessment

The original gap is real: object persistence precedes metadata registration, so ordinary transaction GC cannot discover every object when metadata copying/forwarding/registration fails. The current design connects existing writer, workspace and CN lifecycle owners instead of adding a second durable metadata store or a general GC framework. Registration, not metadata copying, retires cleanup responsibility. Remote credit ACKs alone cannot prove takeover, so the additional contiguous receipt state is necessary. Admission before Sync is what makes outage debt bounded; merely limiting the retry queue after upload would lose already-created objects.

The mechanism is substantial, not a small diff: excluding generated code, implementation is +2,601/-156, tests +4,260/-14, and documentation +171 lines. I assessed those independently. Name-only ownership, one admission ledger per CN, one retry worker, reuse of the existing flow mutex/sequence and one shared teardown budget have identifiable correctness/resource benefits. The final correction removes the unused MultiUpdate cleanup implementation, redundant fields and the separate charge map; the document describes the final contract rather than PR history. This does not mean every inherited helper is maximally minimal. The optional MultiUpdate double-decode optimization remains deferred pending profiling, not claimed fixed.

Adversarial closure and evidence

I rechecked source paths and the exact-head evidence, including adjacent controls rather than treating a green suite as the proof:

Closure	Counterexample / control checked
Writer and registration	Successful persistence followed by rejected registration and failed Delete versus successfully registered metadata. TestIssue29257DumpRegistrationFailureDeletesUnpublishedObjects uses real insert/delete dump paths and verifies physical file removal; workspace acceptance and repeated-spill controls preserve accepted files. TN's existing full-snapshot deletion-count contract remains unchanged.
Admission and partial progress	Exhaustion must reject before Sync; an ambiguous Delete batch must remain charged, whereas completely confirmed batches release only their own names. Admission, partial-progress and ambiguous-batch tests cover these different outcomes.
Remote handoff	Connector/Dispatch receive must retain before forwarding/ACK; missing, ordinary-credit, gapped, stopped or aborted streams must not retire the producer's owner. TestRemoteS3ReceiveTakeoverBeforeAck, finalizer/flow tests, and independently encoded field-19/field-20 wire tests select these boundaries. Inline valid ACK handling has no I/O or channel wait.
Terminal cleanup and reuse	Delete failure plus request cancellation must preserve the original SQL error and retain retryable state after workspace retirement. Actual MultiUpdate Free-to-callback recovery, clone fresh-context, rollback handoff and shared-budget/prepared-reuse tests cover the corresponding controls. Failed cleanup detaches buffers/table/request state rather than retaining the session mpool until storage recovery.
CN retry/shutdown	Concurrent enqueue/close, failed-task rotation, successful immediate draining and failed-close recovery are covered. The worker is joined before I/O dependencies close. Delete reaches the existing context-aware object-storage semaphore and storage API; no guarantee is claimed for an implementation that ignores cancellation.
Partition/public SQL	Writers remain on the coordinator because the partition wrapper has no remote instruction codec; remote source grouping and parallelism remain. The two-CN SQL evidence selects real remote execution and four partition writers, with independently specified count/sum/min/max oracles for AP update, rollback, prepared reuse, ordinary AP insert and TP control.

Two additional source hypotheses were rejected after following their real callers: (1) MultiUpdate's one-shot owner list is not an unbounded history of already-accepted spills: production accumulates their metadata until final output, pending names remain admission-charged, and Reset/Free clears or transfers the references; (2) normal S3 query execution does not lose the shared cleanup budget through BuildPipelineContext, because compile builds its budget-bearing query context first. The plain-context embedded-reader path is not an S3-producing write path.

Evidence reused from the unchanged exact head: all 13 affected normal/race package suites completed successfully; focused same-process race repetitions covered admission, enqueue/close, partial progress, actual Free, ACK ingress, context/reuse and deduplication; incremental vet/lint and baseline-compared molint passed; regenerated protobuf matches; public SQL passed repeatedly on the same two-CN instance. I checked the terminal race-package records and evidence scope. These are prior local executions on this source, not newly rerun tests or an assertion that pending CI passed.

Historical comment disposition

All 12 inline threads were inspected. All currently remain marked unresolved in GitHub; this table describes technical disposition, not an administrative resolution of another reviewer's thread.

Comment IDs	Disposition at this head
4090363760, 4090363768, 4090363776	Original remote/transfer/deletion SQL error is preserved when cleanup ownership is retained; cleanup errors are logged instead of replacing its moerr identity. A failed handoff is still surfaced.
4090363780, 4090363785	Commit/rollback can proceed after successful CN cleanup handoff; transient garbage deletion failure is not itself reported as transaction failure.
4090363782	Terminal cleanup detaches request cancellation and uses its bounded cleanup context.
4090363789	Failed writer cleanup releases sinker/batch/mpool-backed state and retains lightweight retry state.
4090363794	Unbounded-growth concern addressed by pre-upload admission and name limits; successful tasks drain immediately, failed tasks rotate with a one-second delay. Per-task exponential backoff was not implemented and is not necessary to claim this bounded-debt contract.
4090363801	ACK ingress explicitly documents the no-I/O/no-blocking-wait requirement, consistent with its current handler.
4090363764	Explicitly scoped to stop-the-cluster upgrade by the requester. Mixed-version write availability is not implemented or claimed; persisted data compatibility remains required.
4100624104	Partition case closed through valid coordinator-writer placement, not by inventing an unsupported remote wrapper codec. Placement tests and actual two-CN partition SQL cover it.
4103141771; reviews 5333838958 / 5350993661	Mandatory performance/capacity evidence is now supplied: names are allocated after the baseline and full retained owner/callback/queue state is measured at 65,536 tickets; identical real repeated-spill workloads are compared against verified main.

Performance and explicit limits

The paired 4-cycle MemoryFS workload reports median time +2.94% at 1 MiB/cycle and +0.93% at 128 MiB/cycle; allocation counts +0.11%/+0.27%, bytes +0.035%/approximately flat. Timing ranges overlap. This supports small measured bookkeeping cost for those shapes, not zero overhead, statistical significance or a cloud-S3/SQL-throughput guarantee.

Full retained state at 65,536 names measures 9.94–44.57 MiB across 42/256-byte names and 1/64 objects/task. The 128 MiB planning allowance is not a hard process-RSS bound; ordinary writer buffers, file-service caches, GC and other in-flight work remain additional costs. Sustained cleanup debt can reject new S3 uploads when admission fills. Process-crash durability and historical-orphan sweeping are deliberately not provided. A failed bounded CN Close retains dependencies/retries but does not automatically resume the one-shot top-level shutdown. These limitations are material and documented, not hidden behind the no-blocker conclusion.

[gouhongshen]

Codex automated review

Revalidated exact head 4cf6f86 against merge-base 37ba071. The current equivalent of previously approved 964b8f5 is c3c3e07; c3c3e07..HEAD contains ten authored non-merge commits. I reconciled current PR reviews, issue comments, and all twelve inline threads. My prior approval had no blockers. The other reviewers' performance/capacity evidence gate is addressed at this head: pkg/vm/engine/disttae/unpublished_s3_capacity_test.go:47 measures full post-GC retained state after names are allocated, pkg/vm/engine/disttae/unpublished_s3_bench_test.go:73 and the PR body provide paired main/PR measurements, and the benchmark SHA matches. Prior primary-error, commit/rollback, retry-shell, ACK-ingress, mixed-version, and partition-placement concerns are fixed or accepted within the documented live-CN/stop-cluster scope; none remains blocking. Local gofmt and diff checks pass. Focused mo-cgo tests could not start because this worktree lacks cgo/libmo.dylib and thirdparties/install; that is an environment limitation, while exact-head execution evidence is supplied in the PR.

[aunjgr]

Re-reviewed 4cf6f8689c0e84f2b66609cf2bcc58cccce9c73d. My prior required-performance-evidence finding is closed: the capacity benchmark allocates names after its baseline and retains the production receive -> transaction-detached callback -> CN retry graph at 65,536 tickets; the paired spill benchmark executes the same four real Write/Sync/accept/reset cycles on main and this PR. The reported bytes, allocations and timing describe their measured workload and limits accurately.

Invariant	Review closure
Ownership	The final map consolidation keeps pending names and their charge bit together; confirmed acceptance/Delete releases a charge once, while failed deletion remains owned.
Termination	Existing bounded teardown and retry ownership are retained; the deleted MultiUpdate cleanup method was superseded by the production detached retry path.
Growth	Admission remains before Sync, capped at 65,536 names; full retained-state measurements and the stated operational allowance close the earlier ledger-only proof gap.

I checked the final simplification, benchmarks, independent race/two-CN evidence and the revised design. The recorded owner decision limits upgrades to stopping the cluster and starting all services on the new version; it closes the former rolling-upgrade requirement. Current CI is green. No remaining blocker found in the documented live-CN scope; process-crash durability and unavailable-storage deletion deadlines remain outside that contract.

[XuPeng-SH]

mo-bot work · mo-bot-local · failed

Commit: 4cf6f8689c0e84f2b66609cf2bcc58cccce9c73d

Frozen conflict status contradicts the supplied revision pair. Refresh conflict evidence before authorizing repair or rebase. Earlier review concerns have corresponding source corrections and published measurement evidence.

[XuPeng-SH]

mo-bot work · mo-bot-local · failed

Commit: 4cf6f8689c0e84f2b66609cf2bcc58cccce9c73d

No source repair or rebase is justified, but no-op acceptance remains blocked by unverified mandatory validation and concrete local prerequisite gaps. Supply matching validation diagnostics or provision the recorded prerequisites, then execute the retained validation plan and separate final gate.

[XuPeng-SH]

mo-bot work · mo-bot-local · failed

Commit: 4cf6f8689c0e84f2b66609cf2bcc58cccce9c73d

The validation-only plan remains technically sound, with no source repair or rebase justified. No-op acceptance is withheld until required product validation is verified. The previous validation_complete=true flag was scoped too narrowly to design assessment and is corrected here.

[XuPeng-SH]

mo-bot work · mo-bot-local · failed

Commit: 4cf6f8689c0e84f2b66609cf2bcc58cccce9c73d

No new source repair is demonstrated. Acceptance remains blocked by missing required validation diagnostics. Supply matching terminal evidence or enable the recorded validation plan; another unchanged triage/design cycle cannot close this gap.