Repository navigation
fix(disttae): bound long-transaction workspace maintenance and close orphan-cleanup ownership - #30988
gouhongshen wants to merge 34 commits into
Conversation
06c583c to
a1aefd5
Compare
…iles The SCA license job of run 39994645622 failed on this PR: license-eye reported 12 files added by PR matrixorigin#30988 without a valid Apache-2.0 header. Nine files (the relay/admission sources, their tests and txn_issue25590_test.go) carried no header at all; the three r33/r34 disttae tests carried a header with a noncanonical license URL (http://www.apache.org/licenses-2.0 instead of .../LICENSE-2.0), one also reflowing the "either express or implied" clause. All 12 now carry the repository's canonical Matrix Origin Apache-2.0 header, byte-identical to sibling table_meta_reader_r32_test.go which passes the same check. Verified with the pinned CI tool (license-eye v0.4.0): the header check over the 12 files reproduces the CI failure on the pre-fix head (12 invalid, exit 1) and passes after the fix (12 valid, exit 0); the full-tree header check reports 0 invalid files (exit 0). Comment-only change: gofmt -l and parse checks over the 12 files are clean.
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 19358d2881c0cf83e0d7784ceea20ec767539356
Assessment: request_changes
One capacity-bound integration defect remains in RC tombstone transfer. Earlier lifecycle findings are resolved in the current source; 12 focused regressions passed.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1586-7a01-9000-d45e20995cf8 · slot 1 · generation 1
- P2
pkg/vm/engine/disttae/txn.go:3914: An RC transaction transfers persisted tombstones while the engine's unpublished-object capacity is full. TransferFlow.fillDefaults creates its sinker without the reservation hook, so transferTombstoneObjects persists replacements before reaching this record gate.
Impact: Recording rejects the replacement, but this fallback retains it beyond the configured bound. Further transfers can grow the backlog despite admission refusal. Bind the transfer sinker to the workspace's reservation and cleanup lifecycle before persistence, and add a saturation regression.
Evidence: transfer_util.go:224 constructs the unhooked sinker; transfer.go:111 persists before recording at line 144. The internal producer/gate probe observed retained=2 with limit=1 and a physical replacement file. The same producer with SinkerCapacityOption refused before persistence. See transfer-probe-runtime.log and checks/1793253799150507913-3.json.
Validation:
-
Isolation preflight and native prerequisite build passed; 193 frozen native inputs were verified.
-
12 focused lifecycle regressions passed: lifecycle-regressions.log, checks/1793253875329094165-3.json.
-
The capacity probe failed its invariant assertion; its hooked control passed. Initial fixture and linker setup failures are retained separately.
-
Final audit verified unchanged source, matching patch content after newline normalization, receipts, and coverage of all 62 discussion keys: validation-audit.json, checks/1793254057811392189-4.json.
Limitations:
-
The probe exercises the transfer producer and record gate internally; RC rowid lookup and an end-to-end SQL reproduction were not executed.
-
Full SQL, performance, race, and exhaustive test/document-history review were deferred after the integration blocker converged.
ConstructCNTombstoneObjectsTransferFlow built its sinker without the workspace's per-object admission hook, so the RC transfer persisted replacement tombstones before the DELETE record reached the choke: at a saturated bound the refused record fail-open-retained every persisted name and further transfers kept growing the retained backlog. The flow now constructs its sinker with the owning transaction's SinkerCapacityOption, so each replacement reserves one in-flight unit before it goes durable, saturation refuses the transfer with nothing half-persisted, the record converts the reservation, and an interrupted transfer settles through the existing statement-rollback/terminal reservation drains. issue matrixorigin#25590 round-59 review (P2 at pkg/vm/engine/disttae/txn.go:3914)
|
Fixed at pushed head Verified valid, exactly as your probe observed: Fix (one option argument through the existing
Saturation regression:
Validation: the focused four-leg regression passes ( |
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Replacement review at the new head a52b906 (one authored commit over the previously reviewed 19358d2; no merge commits in the increment). The complete new review surface since the previously reconciled head 7c75f3e is nine files: the round-58 gcAccepted fix (unchanged here), the test-only round-59 CI-recovery commit, and this increment (transfer_util.go +50/-6 plus one new 343-line regression file). Disposition of every prior blocker at this head: my round-58 P2 — compaction-superseded objects releasing admission capacity at commit while their accepted GC deletion was in flight — is fixed and re-verified (the engine-owned, name-keyed gcAccepted refcount's four transitions at types.go:1474/txn.go:1906/txn.go:2087/types.go:1529 are byte-identical in this increment). The mo-bot RC-transfer P2 raised at 19358d2 — the transfer's sinker built without the workspace admission hook so replacements persisted before the record gate and refused records fail-open-retained names beyond the bound — is addressed by this increment, and I independently verified the fix: ConstructCNTombstoneObjectsTransferFlow now passes txn.SinkerCapacityOption() through the new WithTransferSinkerOption, so fillDefaults constructs the default tombstone sinker with the pre-existing per-object admission hook; each replacement reserves one in-flight unit before it goes durable, a saturated ledger refuses the transfer with nothing half-persisted, the DELETE record converts the reservation into recorded ownership through the existing register choke, and a transfer interrupted between spill and record settles through the statement-rollback and terminal reservation drains with the physical file collected by the handoff and the queue delete. The default construction for other flows is unchanged (empty sinkerOpts), the flow is constructed fresh per table pass under the transaction lock so there is no persistent-sinker rebinding hazard, and the reservation machinery is the already-reviewed round-18/19/22/33 lifecycle. Earlier topics — rounds 1 through 57, aunjgr's transferred-fragment teardown P1 fixed at e2653d7, the round-57 per-workspace claim sets, and fengttt's approval as decision context — are fixed at prior heads and untouched by this increment. The four-leg regression TestIssue25590RCTombstoneTransferBoundToTheAdmissionLedger pins the pre-fix overshoot (retained=2 behind a one-unit bound with physical files), the saturated refusal across three rounds, the reserve/convert lifecycle, and the interrupted window; every fixture it references exists, and gofmt is clean on the changed files. Limitation: focused tests cannot be executed in this checkout because the documented build requires the thirdparties/install CGO headers (xxhash, roaring, usearch, jemalloc), which are absent, and the review constraints forbid installing dependencies; the package build fails at those headers identically on a pristine pre-change tree, so the failure is not attributed to this PR. Verification is therefore full-source analysis of the increment with complete history reconciliation, corroborated by the author's recorded focused and corpus runs (r59_focused.log, r59_final.log, vet and lint clean) and the earlier rounds' passing regression evidence. Conclusion: every blocking topic from the review history is fixed at this head, the increment correctly closes the last open finding without introducing any new defect, and no findings remain.
|
mo-bot review · Commit: Review linked issue or PR description changed before publication; a fresh contextual review is queued. |
|
mo-bot review · Commit: Review belongs to an older commit; it is not a review of the current head. |
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Request changes. The previously reported successful-transfer workspace leak is fixed: the transfer now calls delTransaction and the new tests cover mpool/segment retirement. I found one remaining failure-path violation of the approved lossless-cleanup invariant in the updated GC ownership path. Passing CI was observed; this finding is based on source and the deletion helper, not a separately run fault-injection test.
The round-60 review of PR matrixorigin#30988 head a52b906 (GC ownership path) found the accepted GC worker settling on an unproven deletion: DeleteUnpublishedObjects stops at its own first failing storage sub-batch and proves nothing about the rest, but the worker logged the error and then unconditionally retired the whole batch's ledger membership (the admission capacity and the r58 engine-owned claim) and the submitter's recorded-name copy - so on a storage outage the objects could still exist with the last enumerable owner dropped, and nothing would ever retry the lost names, violating the lossless ownership this PR's ledger promises (design I2). The failed batch now transfers to the engine's bounded retry queue in one queue-mutex section (requeueFailedGCUnpublishedPending): the batch's engine-owned gcAccepted references release inside the section, each name's owned membership moves whole into the queue's own slot (the capacity unit is held, never freed), and the recorded-name holder the worker captured registers per name so the queue's settle retires the recorded copy exactly at a proven deletion or not-found. Because that registration must survive a transaction that may already have ended, recordedOwners is re-keyed from *Transaction to the *txnRecordedUnpublished holder (the drain's retire needs only the holder; no queued obligation may retain a completed transaction's graph - round-40/41). Recovery is the existing drain ladder behind the GC tick; a name the failed call had already deleted returns not-found - the documented success - on the next retry. Regressions (all three red on the previous head exactly at this contract, green on the fix; logs under /tmp/issue25590-r60/): - TestIssue25590AcceptedGCWorkerFailureRequeuesObligationUntilProof: the review's whole-batch scenario - accepted worker fails, both names requeue with capacity held and recorded copies tracked; the recovery drain deletes both and retires ledger, copies and bound per name at proof. - TestIssue25590AcceptedGCWorkerMidBatchFailureRequeuesWholeBatch: the LocalFS partial-batch shape - the failing call's first file is already gone, the whole batch requeues, the retry tolerates the absent prefix and settles every name per file. - TestIssue25590FailedGCBatchObligationSurvivesTerminalWithoutRe tainingTxn: commit while the failed names sit queued - the obligation survives the terminal, owner entries detach with the recorded copies, the emptied holder settles nothing and the drain still proves the deletions. Design: the round-60 record in docs/design/20261021-issue-25590- workspace-scaling-and-cleanup-ownership.md (implementation follow-up inside the approved design; settle classes keep whole-membership retirement on proofs only).
The UT Coverage lane of run 40918631620 failed TestGCFiles with ants' ErrPoolOverload at the test's own park submit (workspace_test.go:2547): an engine-owned submitter - the failed accepted delete's requeue tail scheduling a background queue drain into the pool current at that instant - took the freshly reset 1-worker nonblocking pool's worker inside the window between ResetGCWorkerPool and the park Submit. The engine never promises an idle pool: the requeue tail, the enqueue-side drain and the one-minute GC scheduler tick all submit into whatever pool is current, asynchronously. The test now parks the worker on a pool the engine has not seen yet, so no engine submitter can race the park; after the handoff every engine submit faces a busy nonblocking pool and transfers its names to the queue losslessly per design. The two rounds are now told apart by distinct name classes: - round 0 uses parseable-but-missing names: the accepted submission's delete returns the not-found proof, settles and retires every claim, so the queue stays empty at every instant (the old require.Empty raced the failed worker's requeue of the unparseable zero names); - round 1 uses '#' poisoned names that path validation refuses forever, proving the refused-submission handoff and the drain-pass retention with distinguishable names (the old three names all rendered the identical zero string, so a partial tail passed undetected). ResetGCWorkerPool's worker join becomes the phase barrier: round 1 asserts an empty queue with zero live claims before any refused-round assertion, and the index-range GC now hands over exactly the round's own recorded write window.
|
mo-bot review · Commit: Review discussion changed before publication; a fresh contextual review is queued. |
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Reviewed head 921be87 against origin/main; the new surface since the previously reviewed head b13f6ff is a single test-only commit (pkg/vm/engine/test/workspace_test.go, TestGCFiles), with no merge commits in the range. Disposition of prior blocking topics: the mo-bot RC-transfer P2 raised at 19358d remains fixed at a52b906 and its files are untouched by this increment; the round-57/58 capacity and workspace-claim topics remain fixed; aunjgr's earlier remoterunServer P1 remains fixed at e2653d7 as he confirmed himself; and the one outstanding third-party verdict, aunjgr's CHANGES_REQUESTED at types.go:1508 (accepted GC worker settling on an unproven deletion), is fixed at b13f6ff, which I re-verified at this head: a failed DeleteUnpublishedObjects no longer settles; the whole batch transfers to the engine's bounded retry queue in one queue-mutex section, the capacity unit is held through the outage, settle happens only at proven deletion or not-found, recordedOwners is re-keyed to the txnRecordedUnpublished holder so a completed transaction's graph cannot be retained, and three r60 regressions pin the whole-batch, mid-batch-prefix, and terminal cases. The new test commit correctly closes the CI flake from run 40918631620: the worker is now parked before ResetGCWorkerPool hands the pool to the engine, so no engine submitter (requeue-tail drain, enqueue-side drain, one-minute GC tick) can take the fresh pool's only worker in the reset-to-park window; after handoff every engine submit faces a busy nonblocking pool and transfers names losslessly to the queue. I verified each premise from source: round-0 names are path-valid and LocalFS deleteSingle returns nil for an absent file (the not-found proof), so the accepted round settles deterministically and the queue stays empty; round-1 names end in '#', which fileservice's validatePath whitelist refuses forever, so the drain pass retains exactly the queued names and reports the storage error; ResetGCWorkerPool's ants ReleaseContext joins old workers, making the new phase-barrier assertions deterministic; the default engine GC pool is a 5-worker blocking pool so round-0 submissions are accepted; the per-round index-range windows match the one-entries-per-WriteFile layout of txn.writes; and the 42-byte name-window construction is distinct per round, guarded by an explicit require.Len. Focused tests could not be built in this checkout because the documented build requires thirdparties CGO headers (xxhash, roaring, usearch, jemalloc) that are absent and the review constraints forbid installing them; the failure is environment-only and identical on a pristine tree. Static verification that did run: gofmt is clean on the changed files and 'go build ./pkg/vm/engine/disttae/' compiles. CI at this head passed the UT Coverage lane (the previously failing lane), UT Test, shared build, and BVT/e2e lanes. Every blocker from my own prior reviews is fixed and verified, the remaining third-party review state is answered by a fix I independently validated, and the new increment introduces no defect; no blocking or non-blocking findings remain.
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 921be87c932825c4d30dff0af7b0dfcbeb2aefa6
Assessment: request_changes
Prior runtime concerns are resolved. One repository-required delivery cleanup remains; the latest approval does not address this artifact requirement.
Model: gpt-6-astra / xhigh
Session: 01a1869d-820c-7212-9788-aecfb86a4141 · slot 0 · generation 2
- P2
docs/design/20261021-issue-25590-workspace-scaling-and-cleanup-ownership.md:4657: The committed design includes extensive per-run CI/review narratives, although the preexisting repository review contract explicitly excludes review attestations and one-off proof narratives from tracked delivery.
Impact: Obsolete test-run details and transient evidence become maintained documentation, obscuring the operative design and violating the delivery-artifact requirement. This is a policy finding, not a demonstrated runtime failure.
Evidence: .agents/skills/mo-self-review/SKILL.md:65–74 establishes the requirement at both base and head. This section records CI jobs, reproduction results and /tmp logs for removed receiver-refusal/R15 tests. Remove or externally archive that chronology while retaining the current design, substantive decisions and compact approval trace.
Validation:
-
Static audit passed: checks/1793304223628960872-4.json matches review-audit.log and the clean frozen head.
-
All 74 discussion entries accounted for in discussion-assessment.json.
-
Reconciled current-head CI successes and supplied author validation; details retained in review-evidence.json.
Limitations:
-
No new Go tests were run for this source-established delivery finding.
-
CI status metadata and author reports were available; their raw execution logs were not supplied.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
My earlier failed-delete ownership finding is fixed: failed accepted batches now transfer to the retry queue without releasing capacity. One required delivery-artifact cleanup remains.
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 921be87c932825c4d30dff0af7b0dfcbeb2aefa6
Assessment: request_changes
The failed-delete ownership fix is present. The existing delivery-artifact finding remains; runtime approval does not resolve that requirement.
Model: gpt-6-astra / xhigh
Session: 01a1869d-999c-7481-b340-ae4a63e8cee4 · slot 1 · generation 2
- P2
docs/design/20261021-issue-25590-workspace-scaling-and-cleanup-ownership.md:4657: The committed design retains extensive per-run CI and review narratives, including a removed receiver-refusal test.
Impact: This violates the repository’s delivery-artifact requirement and obscures the operative design. Externalize transient chronology while retaining current invariants, meaningful decisions and a compact approval trace.
Evidence: .agents/skills/mo-self-review/SKILL.md:65–74 excludes review attestations and one-off proof narratives; the rule is identical at base and head. The CI records beginning at lines 4657 and 4718 remain. This confirms the existing policy finding, not a runtime defect.
Validation:
-
Static audit passed: checks/1793357253562126284-2.json matches review-audit.log and the clean frozen head.
-
All 78 discussion keys accounted for exactly once in discussion-assessment.json.
-
Reconciled current-head successful UT, SCA, coverage and multi-CN BVT metadata; receipt and log details retained in validation-ledger.json.
Limitations:
-
This contextual follow-up reused supplied same-head runtime coverage; no new Go tests or exhaustive runtime audit were performed.
-
Raw CI and author execution logs were not supplied.
…ed design record The 2026-10-30 delivery review of PR matrixorigin#30988 (P2 at the r15 record) enforced the repository's delivery-artifact contract (.agents/skills/mo-self-review/SKILL.md): per-run CI/review evidence - CI run and job identities, local reproduction log narrations, per-head red/green execution records and review attestations - is excluded from tracked delivery. The operative design text (§1-§9), every revision's findings, decisions and supersessions, and the compact approval trace are retained unchanged; each implementation round keeps its finding, its trigger analysis, its decision and its maintained regression coverage, and now references the executed evidence through the external archive and GitHub's CI artifacts. The complete pre-maintenance document is preserved byte-identical outside the tracked repository with the loop controller (snapshot at head 921be87, sha256 5276bf5f6da64802189ac7cfbd3b38830816d3752212a3fc69dd122c9e165e70). No design decision, invariant, or validation requirement changes.
|
@XuPeng-SH — delivery-cleanup round (P2 at Your finding re-checked against the contract first: Disposition at
Static verification for the artifact requirement: the diff is the single design document (332 insertions/885 deletions at review time; LF-only, no trailing whitespace or tabs), the PR head moved |
|
@aunjgr — thanks for both confirmations. You verified at head That cleanup is delivered at the freshly pushed head
The full disposition (with the post-edit audit: zero |
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: c7b40fabac7bef97c517f2323601328ced3256c0
Assessment: approve
Prior ownership, capacity, cleanup and lifecycle findings are resolved. The documentation cleanup closes the remaining delivery concern. No actionable unresolved findings remain.
Model: gpt-6-astra / xhigh
Session: 01a1869d-820c-7212-9788-aecfb86a4141 · slot 0 · generation 2
Validation:
-
Reviewed affected ownership paths, failure recovery, remote settlement, regression oracles and all 84 discussion entries; saved discussion-assessment.json.
-
Static audit passed: 73 changed paths and 191 hunks match frozen sources; source remains clean; 64 Go files pass gofmt parsing/format checks; new single-row SQL expectations match independent transaction arithmetic.
-
Receipt checks/1793503442746065635-3.json reconciled with review-audit.log and validation-reconciliation.json.
-
Frozen CI metadata reports successful build, UT, coverage, static analysis and two multi-CN BVT lanes.
Limitations:
-
No local Go tests, race tests, BVT or benchmarks were run. Raw CI and author execution logs were not supplied.
-
The documented crash-mid-queue orphan limitation remains within the accepted design scope.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
The previous accepted-worker deletion failure is now routed into the retry owner without falsely settling capacity. I traced the current queue/reservation/recorded-holder ownership, statement-boundary staging and remote adoption/ACK settlement paths. One verified failure-path test cleanup blocker remains inline. This review does not claim a new local runtime/race run or a complete fresh implementation PASS; green CI and the supplied validation are reused.
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: c7b40fabac7bef97c517f2323601328ced3256c0
Assessment: request_changes
New discussion changes the prior approval: one P2 test-cleanup defect is confirmed. Previously resolved production and design concerns remain closed.
Model: gpt-6-astra / xhigh
Session: 01a1869d-820c-7212-9788-aecfb86a4141 · slot 0 · generation 2
- P2
pkg/vm/engine/disttae/txn_issue25590_test.go:2339: After Rollback schedules orphan cleanup, a fatal assertion at lines 2344–2351 exits the test before closeDeleteGate runs.
Impact: The drain worker can remain blocked indefinitely, retaining fixture state beyond teardown. Pool release and context cancellation cannot unblock the private channel receive.
Evidence: Delete waits at line 1604 on <-gate without cancellation; the sole release is at line 2354, with no registered fallback cleanup. This confirms review_comments:4440766469 and violates the repository's assertion-failure cleanup contract. Register an idempotent release immediately and boundedly join the drain before pool/file-service teardown.
Validation:
-
Read and classified all 88 discussion entries; saved discussion-assessment.json.
-
Static audit confirmed the frozen head, unchanged source, matching patch and failure-path control flow: checks/1793613230783136383-3.json and review-audit.log.
-
Receipt, log and artifact reconciliation passed: checks/1793613275218976177-4.json and validation-reconciliation.json.
Limitations:
-
No new Go, race or BVT tests ran; the finding is established by source analysis.
-
Reused review 5726235176 for unchanged coverage. Its original artifacts and raw CI/author logs were unavailable; supplied CI successes are status metadata only.
…d cleanup TestIssue25590RollbackHandsOrphanCleanupToEngineGC released its gated, context-blind Delete only through the body's explicit call after the synchronous assertions, so a failing require exited through FailNow and leaked the parked GC worker and the fixture's storage - pool.Release cannot end a channel wait. gateDeletesAfter now closes through a sync.Once (idempotent, matching armDeleteBlock/armDeleteIgnoreCtxBlock), issue25590RegisterDeleteGateRelease registers the release immediately at the arming - t.Cleanup runs it first, ahead of the pool and fixture teardown, and joins the engine queue's drain boundedly after the release - and the explicit body release stays the ordering oracle. The new TestIssue25590DeleteGateCleanupReleasesParkedWorker ends its body at exactly the FailNow state so a teardown-order witness pins the failure path red/green; the round-62 design record carries the trail.
|
Combined disposition for the two (a) The 2026-11-02T09:55:22Z review (review id 5734755552, generation 2, head (b) The 1. The generation-1 frozen head is disconnected, pre-rebase. 2. The specific P2 text it quotes was mid-flight state, long superseded. At the frozen head, doc lines 2678–2681 carry "r11 is submitted for independent design review; this handler does not self-approve the design gate, and implementation proceeds only after the reviewer's PASS on r11." At the current head, the same subjects carry the recorded outcomes instead:
3. The approval artifacts exist and are identified. The r17 PASS artifact and the full |
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: fce13c950cc6dbb7a62ad578c616e23bcf8c5a10
Assessment: approve
The outstanding test-cleanup finding is resolved: gate release is idempotent, registered before fatal assertions, and exercised by a cleanup-only regression. Verified unchanged production and design coverage remains applicable. No actionable findings remain.
Model: gpt-6-astra / xhigh
Session: 01a1869d-820c-7212-9788-aecfb86a4141 · slot 0 · generation 2
Validation:
-
Verified frozen head, clean source, complete change map, and two-file increment since the prior approved head.
-
Three focused cleanup tests passed normally and under the race detector.
-
Both affected concurrency tests passed separate race-enabled runs of 100 repetitions each.
-
Isolation preflight, native-library provenance qualification, git diff --check, and formatting checks for 64 Go files passed.
-
Reconciled all 93 discussion entries. Commands, terminal receipts, logs, and source fingerprints are recorded in validation-evidence.json and final-validation-receipt.json.
Limitations:
-
Prior full-review coverage was reused after source comparison; its original raw artifacts were unavailable.
-
Frozen CI statuses were inspected without raw CI logs. Full-suite, cluster SQL, restart, and performance checks were not rerun for the test-only increment.
What type of PR is this?
Which issue(s) this PR fixes:
Related to #25590
What this PR does / why we need it:
Long explicit transactions repeatedly updating the same primary-key rows rescan superseded workspace history at statement boundaries and during reads. This PR keeps incremental per-table views and compacts superseded entries so maintenance follows the live working set. The issue's UPDATE, REPLACE and DELETE+INSERT cases are covered by focused workspace tests and SQL cases.
Persistence and compaction also need a complete ownership contract: every unpublished object must reserve bounded capacity and remain owned until publication or cleanup. The implementation centralizes that contract in the workspace/engine, retains failed cleanup work, and drains it with bounded batches/backoff and a single shutdown budget. Local writers and remote fragments share the same same-version admission contract running on one build; remote ownership transfer proves the coordinator's intake adoption through the run's own batch-flow sequence/ack/drain barrier, installed in both stream modes (negotiated windowed and no-window alike), and clone-borrowed objects retain their existing owner. No per-feature protocol version, peer-capability gate or legacy fallback exists anywhere in the diff (the design document's
r16/r17records record that removal). Main's #26796 key-index/envelope ownership and Engine.Close shutdown ordering are preserved.The versioned design carries the operative r17 design, the decision log, the per-round review records with their approval trace, and the per-invariant validation criteria (§7).
Delivered behavior
Validation
Focused deterministic regressions cover each contract in its owning packages (
pkg/vm/engine/disttae,pkg/sql/colexecwithmulti_update/insert/deletion,pkg/sql/compile,pkg/objectio/ioutil) and at the engine level (pkg/vm/engine/test), with-racelegs for every synchronization-bearing change and counterexample-red/green proofs run during review. The context-blind deletion barrier's own failure path is pinned the same way: its release and bounded drain join run through the test's registered cleanup (idempotent against the explicit ordering-oracle release), andTestIssue25590DeleteGateCleanupReleasesParkedWorkerproves the state a failing assertion leaves behind holds no parked worker. SQL-visible behavior is pinned bytest/distributed/cases/dml/{update,delete,replace}_workspace.*andupdate_remote_fragment_capability.{sql,result}; every generated result was inspected against the intended semantics. The maintained per-invariant coverage spine and its criteria are recorded in the design document's §7.Review and validation history
Each review round's finding, repair decision, and outcome is recorded in the design document's review records, with the compact approval trace in that document's header. This description previously carried the per-round execution chronicle as well; it was removed on 2026-10-30 after the delivery review's P2 on the same class in the design document (the repository's review contract excludes per-run evidence and review attestations from tracked delivery). The pre-cleanup design document and description are archived byte-identical outside the tracked repository with the loop controller; per-run CI evidence remains available through GitHub's run artifacts.
Known limitations
TestCrossTargetUpgradeRecoveryhang) — with reproduction records posted there; this PR's CI record is evaluated against them.