fix(disttae): retain unpublished S3 cleanup ownership by XuPeng-SH · Pull Request #29290 · matrixorigin/matrixone

XuPeng-SH · 2026-09-23T11:03:24Z

What type of PR is this?

BUG

Which issue(s) this PR fixes:

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 · 2026-09-23T11:03:29Z

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 · 2026-10-12T03:17:03Z

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