Fix DDL catalog livelock, Find() End() crashes, and DataSync meta-lock deadlock - #551
Merged
Conversation
|
Warning Review limit reached
Next review available in: 46 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 (5)
WalkthroughChangesCatalog concurrency control
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 3
🧹 Nitpick comments (1)
tx_service/src/cc/local_cc_shards.cpp (1)
5490-5509: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the missing-limiter case in release builds.
DetachAllPendingTasksrelies only onassert(iter != task_limiters_.end()). If the limiter is absent in an NDEBUG build,iteris the end iterator.iter->second->pending_tasks_andtask_limiters_.erase(iter)then produce undefined behavior. The callers reach this function on the table-dropped path, where the limiter lifetime depends on other threads following the same contract.ClearAllPendingTasksandPopPendingTaskshare this pattern, so a fix here can be applied to them later.♻️ Proposed guard
std::deque<std::shared_ptr<DataSyncTask>> detached; std::lock_guard<std::mutex> task_limiter_lk(task_limiter_mux_); auto iter = task_limiters_.find(task_limiter_key); assert(iter != task_limiters_.end()); + if (iter == task_limiters_.end()) + { + return detached; + } detached.swap(iter->second->pending_tasks_); task_limiters_.erase(iter); return detached;🤖 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/cc/local_cc_shards.cpp` around lines 5490 - 5509, Update LocalCcShards::DetachAllPendingTasks to handle task_limiters_.find(task_limiter_key) returning task_limiters_.end() in release builds: return an empty detached queue before dereferencing or erasing the iterator, while preserving the existing behavior when the limiter exists.
🤖 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 `@DDL_CATALOG_LIVELOCK_PLAN.md`:
- Line 25: Update the four fenced code blocks in DDL_CATALOG_LIVELOCK_PLAN.md,
including the log and invariant excerpts, to specify the text language
identifier after each opening fence and eliminate the MD040 warnings.
- Line 3: Update the document header and Part I to mark the proposal as
historical and clearly identify the changes included in this PR. Reconcile Part
II and Section 13.8 with the recorded implemented build, final run, and DataSync
fixes, removing statements that claim DataSync was untouched. Apply the same
scope and behavior corrections to the corresponding docs/ design document and
stale nearby comments.
In `@docs/04-transaction-execution.md`:
- Around line 91-96: Reword the sentence after the lock-type table to clarify
that `DeduceLockType` returns `WriteIntent` for `ReadForWrite` regardless of the
selected protocol, rather than claiming both table rows use the same lock type.
Preserve the existing explanation of the differing failure status and lock-cycle
rationale.
---
Nitpick comments:
In `@tx_service/src/cc/local_cc_shards.cpp`:
- Around line 5490-5509: Update LocalCcShards::DetachAllPendingTasks to handle
task_limiters_.find(task_limiter_key) returning task_limiters_.end() in release
builds: return an empty detached queue before dereferencing or erasing the
iterator, while preserving the existing behavior when the limiter exists.
🪄 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: 5f67fe79-a83f-4e10-bdfa-f1d591674cf8
📒 Files selected for processing (14)
DDL_CATALOG_LIVELOCK_PLAN.mddocs/03-concurrency-control.mddocs/04-transaction-execution.mdtx_service/include/cc/catalog_cc_map.htx_service/include/cc/local_cc_shards.htx_service/include/error_messages.htx_service/include/tx_operation.htx_service/src/cc/local_cc_shards.cpptx_service/src/cc/non_blocking_lock.cpptx_service/src/tx_execution.cpptx_service/src/tx_operation.cpptx_service/tests/AcquireAllError-Test.cpptx_service/tests/CMakeLists.txttx_service/tests/NonBlockingLock-Test.cpp
| @@ -0,0 +1,1715 @@ | |||
| # Plan: break the catalog write-intent livelock between concurrent DDL and DML | |||
|
|
|||
| Status: proposed, not implemented. Revision 9, after adversarial review rounds | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the implementation status and scope consistent.
The header says proposed, not implemented, but Part II records an implemented build and a successful final run. Section 13.8 also says the DataSync machinery was not touched, while the same report records DataSync fixes and the PR objective includes them. Mark Part I as historical and state which changes belong to this PR.
As per coding guidelines, update stale nearby comments and the corresponding docs/ design document when behavior changes.
Also applies to: 1677-1689, 1691-1715
🤖 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 `@DDL_CATALOG_LIVELOCK_PLAN.md` at line 3, Update the document header and Part
I to mark the proposal as historical and clearly identify the changes included
in this PR. Reconcile Part II and Section 13.8 with the recorded implemented
build, final run, and DataSync fixes, removing statements that claim DataSync
was untouched. Apply the same scope and behavior corrections to the
corresponding docs/ design document and stale nearby comments.
Source: Coding guidelines
|
|
||
| Ground truth from run 3 (2026-08-04, node-a glog plus `db.currentOp`): | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to the fenced blocks.
markdownlint-cli2 reports MD040 at these four fences. Use text for the log and invariant excerpts.
Also applies to: 173-173, 1092-1092, 1119-1119
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@DDL_CATALOG_LIVELOCK_PLAN.md` at line 25, Update the four fenced code blocks
in DDL_CATALOG_LIVELOCK_PLAN.md, including the log and invariant excerpts, to
specify the text language identifier after each opening fence and eliminate the
MD040 warnings.
Source: Linters/SAST tools
Comment on lines
+91
to
+96
| | `is_for_write_` | protocol | lock | on conflict | | ||
| |---|---|---|---| | ||
| | `false` (catalog read) | `Locking` | `ReadLock` | blocks in the entry's queue | | ||
| | `true` (catalog read for write) | `OCC` | `WriteIntent` | **fails fast** → `ACQUIRE_KEY_LOCK_FAILED_FOR_WW_CONFLICT` | | ||
|
|
||
| The lock type is identical in both rows — `DeduceLockType` returns `WriteIntent` for `ReadForWrite` under every protocol. Only the failure status differs. Waiting on a catalog write intent closes a lock cycle against a DDL that is already past its prepare log and therefore cannot be aborted; see the rationale in [03](03-concurrency-control.md#5-nonblockinglock-non_blocking_lockh-non_blocking_lockcpp). `UpsertTableIndexOp::acquire_all_intent_op_` and `CatalogAcquireAllOp::acquire_all_intent_op_` already picked `OCC` for the same reason. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix contradictory claim about the lock-type table.
The table shows different lock values for the two rows (ReadLock for is_for_write_ = false, WriteIntent for is_for_write_ = true). The sentence right after the table says "The lock type is identical in both rows," which contradicts what the table shows. The intended point is that the lock type for the true row does not change across CC protocols (OCC/OccRead/Locking), not that the two rows share the same lock type. Reword the sentence to avoid misleading a reader.
📝 Proposed wording fix
-The lock type is identical in both rows — `DeduceLockType` returns `WriteIntent` for `ReadForWrite` under every protocol. Only the failure status differs. Waiting on a catalog write intent closes a lock cycle against a DDL that is already past its prepare log and therefore cannot be aborted; see the rationale in [03](03-concurrency-control.md#5-nonblockinglock-non_blocking_lockh-non_blocking_lockcpp). `UpsertTableIndexOp::acquire_all_intent_op_` and `CatalogAcquireAllOp::acquire_all_intent_op_` already picked `OCC` for the same reason.
+For the `is_for_write_ = true` row, the lock type does not change across protocols — `DeduceLockType` returns `WriteIntent` for `ReadForWrite` whether the protocol is `OCC`, `OccRead`, or `Locking`. Only the failure status differs between them. Waiting on a catalog write intent closes a lock cycle against a DDL that is already past its prepare log and therefore cannot be aborted; see the rationale in [03](03-concurrency-control.md#5-nonblockinglock-non_blocking_lockh-non_blocking_lockcpp). `UpsertTableIndexOp::acquire_all_intent_op_` and `CatalogAcquireAllOp::acquire_all_intent_op_` already picked `OCC` for the same reason.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | `is_for_write_` | protocol | lock | on conflict | | |
| |---|---|---|---| | |
| | `false` (catalog read) | `Locking` | `ReadLock` | blocks in the entry's queue | | |
| | `true` (catalog read for write) | `OCC` | `WriteIntent` | **fails fast** → `ACQUIRE_KEY_LOCK_FAILED_FOR_WW_CONFLICT` | | |
| The lock type is identical in both rows — `DeduceLockType` returns `WriteIntent` for `ReadForWrite` under every protocol. Only the failure status differs. Waiting on a catalog write intent closes a lock cycle against a DDL that is already past its prepare log and therefore cannot be aborted; see the rationale in [03](03-concurrency-control.md#5-nonblockinglock-non_blocking_lockh-non_blocking_lockcpp). `UpsertTableIndexOp::acquire_all_intent_op_` and `CatalogAcquireAllOp::acquire_all_intent_op_` already picked `OCC` for the same reason. | |
| | `is_for_write_` | protocol | lock | on conflict | | |
| |---|---|---|---| | |
| | `false` (catalog read) | `Locking` | `ReadLock` | blocks in the entry's queue | | |
| | `true` (catalog read for write) | `OCC` | `WriteIntent` | **fails fast** → `ACQUIRE_KEY_LOCK_FAILED_FOR_WW_CONFLICT` | | |
| For the `is_for_write_ = true` row, the lock type does not change across protocols — `DeduceLockType` returns `WriteIntent` for `ReadForWrite` whether the protocol is `OCC`, `OccRead`, or `Locking`. Only the failure status differs between them. Waiting on a catalog write intent closes a lock cycle against a DDL that is already past its prepare log and therefore cannot be aborted; see the rationale in [03](03-concurrency-control.md#5-nonblockinglock-non_blocking_lockh-non_blocking_lockcpp). `UpsertTableIndexOp::acquire_all_intent_op_` and `CatalogAcquireAllOp::acquire_all_intent_op_` already picked `OCC` for the same reason. |
🤖 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 `@docs/04-transaction-execution.md` around lines 91 - 96, Reword the sentence
after the lock-type table to clarify that `DeduceLockType` returns `WriteIntent`
for `ReadForWrite` regardless of the selected protocol, rather than claiming
both table rows use the same lock type. Preserve the existing explanation of the
differing failure status and lock-cycle rationale.
…e-all errors DDL (UpsertTable) acquires catalog write intents on every node while DML transactions' for-write catalog reads also take a write intent. Under the Locking protocol the loser enqueues, so concurrent createIndexes plus DML livelocked: each side waited for a queue the other side kept refilling (90 s retry exhaustion in the two-createIndexes reproduction). - Catalog reads pick their protocol by intent: for-write reads use OCC and fail fast instead of enqueueing; plain reads keep Locking. The blocked writer surfaces a conflict the caller can retry instead of parking. - CatalogAcquireAllOp failure branches record a representative error on the commit response instead of leaving it unset (which hung the waiter). AcquireAllOp::RepresentativeError() classifies infrastructure errors above conflicts, with first-in-index-order as the within-class tiebreak, and guards the stale tail of the reused hd_results_ vector. - error_messages.h gains IsConflictError() covering the retryable conflict codes; PostProcess(CatalogAcquireAllOp&) asserts a failure never reaches it with NO_ERROR (silent success would drop the writes). - Corrects the stale OccRead/OCC comment in non_blocking_lock.cpp. - Tests: AcquireAllError-Test (representative-error classification, stale tail, in-range reporting) and NonBlockingLock-Test additions (OCC WriteIntent does not enqueue; Locking does). Reproduction goes from 90 s of retry exhaustion to 0.12 s with one winner and one clean WriteConflict loser. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TemplateCcMap::Find() returns End() for an absent key, and End() is not a dereferenceable entry. Three CatalogCcMap::Execute paths (PostWriteAllCc, replay, InvalidateTableCacheCc) dereferenced the iterator unconditionally and crashed once DDL throughput made the absent-entry window reachable. Each site now maps End() to nullptr and takes the existing absent-entry branch; InvalidateTableCacheCc forwards to the next core or finishes, and its write-lock assertion moves inside the guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Live forensics on a wedged cluster showed a three-party deadlock: DataSyncForRangePartition held the global metadata shared lock while DataSyncTask::SetFinish()'s last-task path issued RPCs (LogAgent::UpdateCheckpointTs, BrocastPrimaryCkptTs) and parked the worker; a catalog writer (CreateDirtyCatalog/UpdateDirtyCatalog) blocked on the metadata write lock while occupying the cc shard's only processor slot; the frozen shard starved the node's brpc service, and the peers' [E1008] timeout storms amplified the freeze cluster-wide. All three park-capable sections now decide under the lock and act after releasing it: DataSyncForRangePartition and DataSyncForHashPartition record a PreCheck outcome and run SetFinish/SetError/SetScanTaskFinished/ PopPendingTask after unlock, and the hash forward-cache error branch unlocks before PostProcessHashPartitionDataSyncTask (whose SCAN_ERROR path reaches AbortTx()/Wait()). The table-dropped paths use new DetachAllPendingTasks(): the limiter and its pending deque are removed atomically while the metadata lock still excludes a concurrent re-creation of the same table, closing the window in which next-generation tasks could join the limiter and be erroneously killed by the deferred finalization. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Updates 03-concurrency-control and 04-transaction-execution for the OCC fail-fast catalog read-for-write path and the CatalogAcquireAllOp error propagation, per the docs maintenance rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
liunyl
force-pushed
the
fix/catalog-write-intent-fail-fast
branch
from
August 11, 2026 04:38
098a900 to
2392601
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| table_name.Engine(), | ||
| id); | ||
|
|
||
| std::deque<std::shared_ptr<DataSyncTask>> detached; |
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Add #include for deque<> [build/include_what_you_use] [4]
| LockOpStatus::Blocked); | ||
|
|
||
| REQUIRE(lock.FindQueueRequest(kWaiter)); | ||
| std::vector<txservice::TxNumber> queued = lock.GetBlockTxIds(0); |
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Add #include for vector<> [build/include_what_you_use] [4]
thweetkomputer
approved these changes
Aug 11, 2026
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>
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
2 participants
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.
What
Three concurrency fixes in the catalog/DDL path, found and validated by a 30-minute DDL fuzz (12 workers, createIndexes/dropIndexes/insert storms plus parked aggregation cursors,
ttlMonitorEnabled: true) against a 3-node EloqDoc cluster:createIndexes). For-write catalog reads now use OCC and fail fast;CatalogAcquireAllOpfailure branches set a representative error on the commit response instead of hanging the waiter (RepresentativeError()ranks infrastructure errors above conflicts and guards the reusedhd_results_stale tail). Reproduction: 90 s → 0.12 s, one winner, one cleanWriteConflictloser.CatalogCcMap::Find()returnsEnd()for absent keys — threeExecutepaths dereferenced it unconditionally (PostWriteAllCc, replay, InvalidateTableCacheCc) and crashed once DDL throughput made the window reachable. All three now take the absent-entry branch.meta_data_mux_. Live-process forensics on a wedged cluster (glibc rwlock owner extraction + stack scans of parked bthread stacks; full write-up inDDL_CATALOG_LIVELOCK_PLAN.md§13.8) showedDataSyncForRangePartitionholding the global metadata shared lock acrossSetFinish()'s checkpoint RPCs while a catalog writer blocked on the write lock from the cc shard's only processor slot — freezing the node and, via peer[E1008]retry storms, the cluster. All three park-capable sections (range, hash, hash forward-cache error branch) now decide under the lock and act after releasing it, and the newDetachAllPendingTasks()removes the limiter atomically under the metadata lock so a concurrent re-creation of the same table cannot have its next-generation tasks killed by the deferred finalization.Verification
AcquireAllError-Test(classification, stale tail, in-range reporting), extendedNonBlockingLock-Test(OCC WriteIntent does not enqueue; Locking does).DDL_CATALOG_LIVELOCK_PLAN.mdcarries the complete design, forensics, and follow-up list (notably:TransactionExecution::Executeaccepts requests from a caller bound to a recycled txm — defensive identity assert suggested;PinRangeSlicesdispatches store loads under the plain shared meta lock, an invariant hazard if the store handler ever blocks).Companion PR in eloqdoc carries the mongo-layer half (Collection lifetime pinning, cursor save/restore across transactions, dispose-path fix) and the submodule bump.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests