Wait for metadata post-processing when closing a metadata-only transaction - #553
Merged
Conversation
CommitTx's fast path discards the caller's CommitTxRequest and fires the txm-owned commit_tx_req_ without waiting, returning true unconditionally. For a transaction whose only state is a metadata (catalog) read that is wrong on three counts: an abort reports success, the caller's request is never finished so CommitTx()'s wrapper reads an ErrorCode that was never set, and -- most importantly -- the catalog read locks are still held when CommitTx returns, because releasing them is part of the asynchronous post-processing the caller no longer waits for. A retry loop that closes and immediately reopens a transaction can then keep a catalog entry's reader count permanently non-zero, so a DDL write intent never upgrades. Excluding MetaDataReadSetSize() from the fast-path guard routes those transactions through the waiting path, so the locks are released before CommitTx returns. TestNodeSmoke covers both close modes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The read-local comment and 03-concurrency-control claimed the catalog entry is where DDL and DML meet. It is not: every caller that asks for a catalog read-for-write is a DDL path (create/drop collection, create/drop indexes, rename), while DML and queries read the catalog without the flag. The cycle the fail-fast breaks is DDL against DDL on the same table. The behaviour and the reason to fail fast are unchanged; only the description was wrong. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
liunyl
added a commit
to eloqdata/eloqdoc
that referenced
this pull request
Aug 12, 2026
eloqdata/tx_service#551 merged, so this repoints the submodule from that branch onto main plus the two commits that landed after the merge (eloqdata/tx_service#553): the metadata-only transaction close fix and the corrected catalog fail-fast rationale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 24 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe commit path now treats metadata reads as non-empty transactions. Smoke tests cover metadata-only commits in both close modes. Catalog locking comments and documentation describe DDL-specific intent handling and retry behavior. ChangesMetadata transaction commit handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tx_service/src/tx_execution.cpp (1)
1832-1847: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeparate the lock paths in this comment:
ReaderWriterCntl::AddWriterrejects a different existing writer withWriteConflictand returnsWritePendingonly when existing readers remain. It does not inspectCcProtocol. Attribute enqueue-versus-fail behavior toNonBlockingLock::AcquireWriteIntent, which enqueues forLockingand fails fast forOCC.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tx_service/src/tx_execution.cpp` around lines 1832 - 1847, Revise the comment around the catalog read-for-write lock acquisition to separate lock responsibilities: describe ReaderWriterCntl::AddWriter as rejecting conflicting writers with WriteConflict and returning WritePending only for remaining readers, without attributing behavior to CcProtocol. Attribute Locking’s enqueue-versus-OCC’s fail-fast behavior to NonBlockingLock::AcquireWriteIntent, and retain the deadlock rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tx_service/src/tx_execution.cpp`:
- Around line 565-568: Update the CommitTx fast-path guard to also verify that
locked_db_ contains no outstanding catalog-read locks before returning
immediately. Preserve the existing checks and add a concise comment documenting
that this boundary prevents read-only object-command transactions from bypassing
Reset() and leaving catalog locks unreleased.
---
Nitpick comments:
In `@tx_service/src/tx_execution.cpp`:
- Around line 1832-1847: Revise the comment around the catalog read-for-write
lock acquisition to separate lock responsibilities: describe
ReaderWriterCntl::AddWriter as rejecting conflicting writers with WriteConflict
and returning WritePending only for remaining readers, without attributing
behavior to CcProtocol. Attribute Locking’s enqueue-versus-OCC’s fail-fast
behavior to NonBlockingLock::AcquireWriteIntent, and retain the deadlock
rationale.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8dbfb811-737b-4ac9-8bb5-02e265907dde
📒 Files selected for processing (3)
docs/03-concurrency-control.mdtx_service/src/tx_execution.cpptx_service/tests/TestNodeSmoke-Test.cpp
Catalog read locks live in two places, and the fast path only accounted for one. Besides the metadata read set, Process(ObjectCommandOp) stores a catalog lock per database in locked_db_, released by ReleaseCatalogsRead() from Reset(). A transaction holding only those would still take the immediate path and return before they were released. Only EloqKV populates locked_db_ (object commands, FLAGS_cmd_read_catalog defaults on), and its auto-commit commands never reach CommitTx -- the already_committed branch of PostProcess(ObjectCommandOp) resets directly, which releases the locks synchronously. So this affects explicit multi-command transactions, where the extra wait is proportionate to the work they already do. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
Confirmed and fixed in c18dd39 — the guard now also requires every Two notes on the blast radius, since the change moves work off the immediate path:
|
liunyl
added a commit
to eloqdata/eloqdoc
that referenced
this pull request
Aug 12, 2026
eloqdata/tx_service#553 merged, so the submodule now tracks main (5a76896) rather than a feature branch: the metadata-only transaction close fix, the locked_db_ addition to the same guard, and the corrected catalog fail-fast rationale are all upstream. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
liunyl
added a commit
to eloqdata/eloqdoc
that referenced
this pull request
Aug 12, 2026
…e, cursor lifetime (#491) * fix: surface catalog DDL conflicts as retryable WriteConflictException Tx-layer conflict errors surfaced as Status{WriteConflict} became ExceptionFor<WriteConflict> at the first uassertStatusOK -- a sibling type of mongo::WriteConflictException that writeConflictRetry never catches, so command-level retry loops treated retryable DDL/DML conflicts as fatal. - New ThrowIfWriteConflict(TxErrorCode) throws the catchable type for the conflict group (WW/RW conflicts, gap lock, OCC validation failures, deadlock abort); TxErrorCodeToMongoStatus calls it first so the mapping cannot drift. - updateRecord/deleteRecord retry loops and insertRecord conflict handling route through it; transient READ_CATALOG_FAIL stays in-loop. - createTable/dropTable/updateTable UpsertResult::Failed branches throw the retryable type instead of returning InternalError (the collMod path's InternalError reached ~MultiIndexBlockImpl's cleanup, which fasserts on anything but WriteConflictException and killed the node). - eraseUnreadyTable registers a rollback restore inside a unit of work so an aborted index build retains its staged unready-index metadata for the in-process retry. - The RecoveryUnit swaps around separate-transaction UpsertTable calls are now exception-safe (guards restore the original unit; the in-unit flag is cleared before commitUnitOfWork, matching its early _inUnitOfWork clear). - Unit tests cover the exception type, the conflict-group membership, and that READ_CATALOG_FAIL is not thrown. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: reference-count cached Collections and pin holders across catalog refresh getCollection() rebuilds the cached Collection whenever the catalog version moves -- constantly under concurrent DDL -- while operations hold raw Collection* across coroutine yields and into RecoveryUnit onCommit callbacks, and EloqLockerNoop provides none of the exclusion upstream's lock manager gave this code. Destroy-on-refresh was a use-after-free, and the interim mitigation leaked every evicted Collection. - Database collection maps hold shared_ptr<Collection>. Eviction is a map erase; the object dies when its last holder finishes. - RecoveryUnit::pinResource() retains resources until the pooled OperationContext is recycled (onCreateOperationContext -> resetRecoveryUnit -> reset(), cleared last so commit/rollback callbacks never outlive their Collection). Pins deduplicate against the live set and follow stashed transactions. Every path handing out a raw Collection* pins first. - _collectionsMutex serialises the pure in-memory map operations -- background threads such as the TTLMonitor mutate the maps concurrently -- and is never held across a coroutine yield. - getCollection() on a vanished table now evicts the stale entry, so a re-created namespace cannot be served the old object. - collections() returns an operation-owned snapshot by value; the shared _collectionsView member and the forView mode are gone (map-iterator invalidation that shared_ptr entries cannot cure). - The capped-insert onCommit callbacks capture the CappedInsertNotifier instead of the Collection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: cursor lifetime across wire operations and concurrent DDL Three cursor-lifetime defects reachable from parked cursors (batchSize smaller than the result set) under concurrent DDL: 1. Globally-managed aggregation cursors never re-resolve the collection on getMore, so no per-operation pin covers the raw Collection* their executors embed; a catalog refresh could destroy it mid-drain. Collection is now enable_shared_from_this; ClientCursor holds a collection pin captured in runAggregate, and DocumentSourceCursor pins the collection each embedded executor was built over -- covering $lookup/$graphLookup foreign pipelines uniformly (their sub-pipelines outlive the outer operation under the $unwind optimization). 2. DocumentSourceCursor::cleanupExecutor() re-resolved the namespace via Database::getCollection() to find the CursorManager -- an API upstream chose because theirs cannot throw. EloqDoc's performs a transactional catalog read that throws under concurrent DDL, and Pipeline::dispose() turns any exception into std::terminate. Re-resolution could also return a rebuilt Collection whose manager never saw the executor. The captured manager (kept alive by the pin) is used instead; upstream's lock discipline around dispose is preserved. 3. EloqRecordStoreCursor::save() kept its scan open across wire operations, unlike saveUnpositioned() and the index cursor: a stashed cursor's next getMore drove an EloqCursor bound to the txm of the operation that opened the scan -- by then committed and recycled to another transaction, whose request queue accepted the zombie's stack-allocated batch request. Both cursor types now close the scan at save time, while the opening transaction is live, and lazily re-seek from the recorded resume key on the current transaction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * build: bump data_substrate to fix/catalog-write-intent-fail-fast Pulls the tx_service half of the DDL catalog work (eloqdata/tx_service PR #551): catalog write-intent fail-fast with acquire-all error propagation, CatalogCcMap End() guards, and the DataSync meta-lock deadlock fixes. The two halves were validated together by the 30-minute DDL fuzz described in the PRs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: abandon the snapshot before write-conflict backoff Under Eloq the abandoned snapshot is a transaction whose accumulated catalog/key intents are exactly what the conflict winner is waiting to drain. The upstream order -- sleep, then abandonSnapshot -- parks the loser on top of its locks for the whole 1-10 ms backoff window and leaves the winner only the instant between the abandon and the retry's re-acquisition, so contended DDL kept losing that race and retrying. Abandoning first hands the winner the full backoff window. For conflicts thrown inside a WriteUnitOfWork the transaction is already aborted during unwind, so the reorder only changes conflicts thrown outside one (the catalog read fail-fast path). The catch runs only at the outermost retry loop, outside any unit of work, so abandonSnapshot's !_inUnitOfWork invariant holds as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * build: bump data_substrate for metadata-only tx close fix Picks up eloqdata/tx_service b627ae7: CommitTx now waits for metadata post-processing when the transaction's only state is a catalog read, so its catalog read locks are released before the close returns. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: close a stashed cursor's scan at detach, not at every save Closing the tx scan in save() cost one scan close and re-seek per deleted document: DeleteStage calls child()->saveState() inside its per-document loop, so a 20k-document deleteMany went from 275 ms to 67.8 s over an index scan (246x) and from 86 ms to 11.3 s over a collection scan. TTL, which deletes through the same planner path, stopped keeping up entirely. detachFromOperationContext() is the point where the cursor actually leaves its operation -- the stash-across-wire-operations case the close exists for -- and the transaction that opened the scan is still live there, so moving the close preserves the guarantee that no stashed cursor keeps an EloqCursor bound to a txm that is about to be recycled. Intra-operation save/restore no longer touches the scan; the resume path is unchanged. Measured after the move: 281 ms and 123 ms, i.e. back to pre-change levels. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * build: bump data_substrate for corrected catalog fail-fast rationale Doc-only change in eloqdata/tx_service 10b2f37: the read-local comment and 03-concurrency-control described the catalog entry as where DDL and DML meet, but every catalog read-for-write comes from a DDL path. No behaviour change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * build: repoint data_substrate at the post-merge follow-up branch eloqdata/tx_service#551 merged, so this repoints the submodule from that branch onto main plus the two commits that landed after the merge (eloqdata/tx_service#553): the metadata-only transaction close fix and the corrected catalog fail-fast rationale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * build: bump data_substrate to tx_service main eloqdata/tx_service#553 merged, so the submodule now tracks main (5a76896) rather than a feature branch: the metadata-only transaction close fix, the locked_db_ addition to the same guard, and the corrected catalog fail-fast rationale are all upstream. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #551, which merged before these two commits landed on its branch.
1.
CommitTxmust not fire-and-forget a metadata-only transactionCommitTx's fast path discards the caller'sCommitTxRequestand fires the txm-ownedcommit_tx_req_without waiting, returningtrueunconditionally. For a transaction whose only state is a catalog (metadata) read that is wrong on three counts:trueeven whento_commit_is false;CommitTx()'s wrapper reads anErrorCodethat was never set;CommitTxreturns, because releasing them is part of the asynchronous post-processing the caller no longer waits for.The third is the one that bites. A command that closes and immediately reopens a transaction —
writeConflictRetrydoing exactly that, with no backoff at all for the first four attempts — can keep a catalog entry's reader count permanently non-zero, so a DDL write intent never upgrades and the retry loop cannot converge.Excluding
MetaDataReadSetSize()from the fast-path guard routes those transactions through the waiting path, so the locks are released beforeCommitTxreturns. The guard's other terms already cover the remaining state:ForwardWriteCnt() > 0impliesWriteSetSize() > 0(both increment sites iterate the write set), andcmd_set_is the object-command path that only EloqKV drives.Measured on EloqDoc's jstest suite (Debug, rocksdb, single node), before → after:
createIndexesmainTestNodeSmokegains coverage for both close modes of a metadata-only transaction.2. Corrected rationale for the catalog fail-fast (docs only)
The read-local comment and
03-concurrency-controlsaid the catalog entry is "where DDL and DML meet". It is not: every caller that asks for a catalog read-for-write is a DDL path (create/drop collection, create/drop indexes, rename), while DML and queries read the catalog without the flag. The cycle the fail-fast breaks is DDL against DDL on the same table. Behaviour is unchanged; only the description was wrong.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation