Fix DDL catalog livelock fallout: conflict typing, Collection lifetime, cursor lifetime - #491
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR changes collection caches from unique to shared ownership. It adds synchronized snapshots and lifetime pins. It updates cursor cleanup and Eloq cursor reuse. It propagates Eloq write conflicts as retryable exceptions, improves recovery cleanup, adds utility tests, and updates a submodule. ChangesCatalog lifetime and Eloq transaction fixes
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Aggregate
participant Collection
participant ClientCursor
participant DocumentSourceCursor
participant CursorManager
Aggregate->>Collection: resolve input collection
Aggregate->>ClientCursor: transfer collection pin
ClientCursor->>DocumentSourceCursor: retain executor and collection state
DocumentSourceCursor->>CursorManager: dispose through captured manager
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 8
🧹 Nitpick comments (3)
src/mongo/db/catalog/database_impl.cpp (1)
813-847: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDocument or handle the null-
RecoveryUniteviction path.If
opCtx->recoveryUnit()returns null at line 845,evictedis destroyed when this function returns. That destroys theCollectionimmediately, which is the exact hazard the pinning scheme prevents.getCollection()carries an explicit comment for the same condition; this function does not.Add the same note here, or make the absence of a
RecoveryUnitan invariant on this path so a future RU-less caller fails loudly instead of freeing a live object.🤖 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 `@src/mongo/db/catalog/database_impl.cpp` around lines 813 - 847, Handle the null-RecoveryUnit branch in DatabaseImpl::_clearCollectionCache: either document that immediate destruction is safe for RU-less callers, matching getCollection(), or enforce an invariant that a RecoveryUnit is required before evicted is released. Ensure future callers cannot silently destroy a live Collection without the pinning scheme.src/mongo/db/catalog/collection_impl.cpp (1)
406-423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the three identical capped-notifier commit callbacks.
The same lambda body and the same 12-line rationale comment appear at Lines 337-354, 406-423, and 506-523. Extract one private helper, for example
void CollectionImpl::_scheduleCappedNotifyOnCommit(OperationContext* opCtx), and keep the rationale in that single place. This prevents the three copies from drifting.♻️ Proposed helper
+void CollectionImpl::_scheduleCappedNotifyOnCommit(OperationContext* opCtx) { + // Capture the notifier, not the Collection: Eloq can refresh the cached Collection before + // RecoveryUnit callbacks run, so a captured `this` may already be freed. A shared_ptr copy + // keeps the notifier alive independently. Non-capped collections hold a null notifier. + opCtx->recoveryUnit()->onCommit([notifier = _cappedNotifier](boost::optional<Timestamp>) { + if (notifier) { + notifier->notifyAll(); + } + }); +}Then replace each of the three call sites with
_scheduleCappedNotifyOnCommit(opCtx);.Also applies to: 506-523
🤖 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 `@src/mongo/db/catalog/collection_impl.cpp` around lines 406 - 423, Extract the duplicated capped-notifier RecoveryUnit callback and its rationale into a private CollectionImpl helper, such as _scheduleCappedNotifyOnCommit(OperationContext* opCtx). Have the helper capture _cappedNotifier and perform the existing null-checked notifyAll() behavior, then replace the callback blocks at all three call sites with helper calls.src/mongo/db/clientcursor.h (1)
322-326: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
std::shared_ptr<const Collection>instead ofstd::shared_ptr<const void>.The
voiderasure removes all type checking on what gets pinned, andsrc/mongo/db/pipeline/document_source_cursor.hLine 219 already uses a typedstd::shared_ptr<Collection>for the same purpose. A forward declaration ofCollectionis enough here, so the typed form costs nothing and makes the two pins consistent.Note one consequence of the
constqualifier:std::shared_ptr<const void>does not convert to thestd::shared_ptr<void>parameter ofRecoveryUnit::pinResource(). That is fine today, because no code feeds this member there. Keep it in mind if the pin is ever forwarded to a recovery unit.🤖 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 `@src/mongo/db/clientcursor.h` around lines 322 - 326, Change ClientCursor::_collectionPin from std::shared_ptr<const void> to std::shared_ptr<const Collection>, adding or reusing a Collection forward declaration as needed. Keep its declaration before _exec and preserve the existing lifetime-pinning behavior; do not forward this const pointer to RecoveryUnit::pinResource().
🤖 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 all four fenced code blocks in DDL_CATALOG_LIVELOCK_PLAN.md to
include an explicit language tag: use text for log or trace output and text or
cpp for the transaction-flow snippet, resolving markdownlint MD040 without
changing the block contents.
- Around line 3-14: Update the document’s implementation status to reflect the
implemented fixes and successful Run 6, revise §13.8 to show its “not patched
here” DataSync disposition is superseded by §13.9, and add the exact eloqdoc
commit plus data_substrate gitlink used for Run 6.
- Around line 426-450: Audit the accepted-scope claims around read-local write
requests rather than treating them as unaffected: trace
EloqRecoveryUnit::readCatalog callers in eloq_record_store.cpp, EloqKV
forwarding in eloq_kv_engine.cpp, and the corresponding tx_service, EloqKV,
EloqSQL, and mixed-version paths. Verify whether each can set read_local_ with
is_for_write_ true and depend on blocking WriteIntent; update the scope decision
and affected-caller inventory to reflect the findings, or adjust the
implementation to preserve their required behavior.
In `@src/mongo/db/catalog/collection_impl.cpp`:
- Around line 337-354: Preserve the haveCappedWaiters() optimization in the
onCommit callback without capturing CollectionImpl or risking a dangling this
pointer. Capture or record waiter presence separately before registering the
callback, then only call notifier->notifyAll() when waiters were present and the
notifier remains valid; retain the existing null-notifier behavior.
In `@src/mongo/db/catalog/database_impl.cpp`:
- Around line 897-904: Guard the UUID optional in the version-mismatch branch
before calling UUIDCatalog::removeUUIDCatalogEntry. Reuse the established
conditional pattern from the nearby check in the same function, removing the
catalog entry only when found->uuid() is engaged, while preserving the
subsequent _clearCollectionCache call.
In `@src/mongo/db/catalog/database.h`:
- Around line 133-136: Update catalog::closeCatalog() and repairDatabase() to
operate on cached Collection instances rather than the fresh objects returned by
collections(), preserving _minVisibleSnapshot set through
setMinimumVisibleSnapshot(). Avoid rebuilding all collections for each
listCollections command; reuse the existing cached collection state or
explicitly transfer it when a snapshot is required.
In `@src/mongo/db/modules/eloq/src/eloq_record_store.cpp`:
- Around line 296-307: The insertRecord catalog-read path must retry transient
READ_CATALOG_FAIL results instead of immediately returning InternalError. Update
the flow around ru->readCatalog and ThrowIfWriteConflict to use the same bounded
retry behavior as deleteRecord() and updateRecord(), while preserving existing
handling for non-retryable errors. Add a regression test that produces
READ_CATALOG_FAIL once, then a successful catalog read, and verifies
insertRecord succeeds.
In `@src/mongo/db/pipeline/document_source_cursor.cpp`:
- Around line 255-273: Update the second cleanupExecutor overload to pass the
constructor-captured _cursorManager to _exec->dispose, matching the overload
shown here. Remove its call-time readLock.getCollection()->getCursorManager()
lookup while preserving the caller-provided read-lock discipline.
---
Nitpick comments:
In `@src/mongo/db/catalog/collection_impl.cpp`:
- Around line 406-423: Extract the duplicated capped-notifier RecoveryUnit
callback and its rationale into a private CollectionImpl helper, such as
_scheduleCappedNotifyOnCommit(OperationContext* opCtx). Have the helper capture
_cappedNotifier and perform the existing null-checked notifyAll() behavior, then
replace the callback blocks at all three call sites with helper calls.
In `@src/mongo/db/catalog/database_impl.cpp`:
- Around line 813-847: Handle the null-RecoveryUnit branch in
DatabaseImpl::_clearCollectionCache: either document that immediate destruction
is safe for RU-less callers, matching getCollection(), or enforce an invariant
that a RecoveryUnit is required before evicted is released. Ensure future
callers cannot silently destroy a live Collection without the pinning scheme.
In `@src/mongo/db/clientcursor.h`:
- Around line 322-326: Change ClientCursor::_collectionPin from
std::shared_ptr<const void> to std::shared_ptr<const Collection>, adding or
reusing a Collection forward declaration as needed. Keep its declaration before
_exec and preserve the existing lifetime-pinning behavior; do not forward this
const pointer to RecoveryUnit::pinResource().
🪄 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: d521966e-64e8-4201-98ef-a4c1f379b429
📒 Files selected for processing (24)
DDL_CATALOG_LIVELOCK_PLAN.mdsrc/mongo/db/catalog/collection.hsrc/mongo/db/catalog/collection_impl.cppsrc/mongo/db/catalog/database.hsrc/mongo/db/catalog/database_impl.cppsrc/mongo/db/catalog/database_impl.hsrc/mongo/db/catalog/index_catalog_entry_impl.cppsrc/mongo/db/clientcursor.cppsrc/mongo/db/clientcursor.hsrc/mongo/db/commands/run_aggregate.cppsrc/mongo/db/modules/eloq/SConscriptsrc/mongo/db/modules/eloq/data_substratesrc/mongo/db/modules/eloq/src/base/eloq_util.cppsrc/mongo/db/modules/eloq/src/base/eloq_util.hsrc/mongo/db/modules/eloq/src/base/eloq_util_test.cppsrc/mongo/db/modules/eloq/src/eloq_index.cppsrc/mongo/db/modules/eloq/src/eloq_record_store.cppsrc/mongo/db/modules/eloq/src/eloq_recovery_unit.cppsrc/mongo/db/pipeline/document_source_cursor.cppsrc/mongo/db/pipeline/document_source_cursor.hsrc/mongo/db/storage/kv/kv_collection_catalog_entry.cppsrc/mongo/db/storage/recovery_unit.htests/jstests/eloq_basic/multikey_catalog_commit_stress.jstests/jstests/eloq_basic/update_index_validation.js
Comment on lines
+3
to
+14
| Status: proposed, not implemented. Revision 9, after adversarial review rounds | ||
| 1-9 plus a live reproduction. | ||
|
|
||
| Baseline: tx_service `main` at `b146a514`, which already contains the | ||
| `NonBlockingLock` upgrade-at-head fix (eloqdata/tx_service#543, closing #541). | ||
|
|
||
| The change spans two repositories. Paths are labelled by repository and are | ||
| relative to that repository's root: | ||
|
|
||
| * **tx_service** — `eloqdata/tx_service`, consumed by EloqDoc as the | ||
| `src/mongo/db/modules/eloq/data_substrate` submodule. | ||
| * **eloqdoc** — `eloqdata/eloqdoc`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="DDL_CATALOG_LIVELOCK_PLAN.md"
if [ -f "$file" ]; then
printf '%s\n' '--- header ---'
sed -n '1,30p' "$file"
printf '%s\n' '--- final sections ---'
sed -n '1650,1710p' "$file"
printf '%s\n' '--- revision/status references ---'
rg -n -i 'Run 6|data_substrate|gitlink|eloqdoc revision|revision|implemented|not implemented|submitted binary|DataSync|clean run|zero wedges' "$file" | tail -n 160
else
printf 'MISSING: %s\n' "$file"
git ls-files | rg 'DDL_CATALOG_LIVELOCK_PLAN\.md$'
fiRepository: eloqdata/eloqdoc
Length of output: 7948
🏁 Script executed:
#!/bin/bash
set -eu
file="DDL_CATALOG_LIVELOCK_PLAN.md"
printf '%s\n' '--- implementation record ---'
sed -n '940,990p' "$file"
printf '%s\n' '--- Run 6 context ---'
sed -n '1688,1715p' "$file"
printf '%s\n' '--- all SHA-like values in document ---'
rg -o -n '\b[0-9a-f]{7,40}\b' "$file" | sort -u
printf '%s\n' '--- revision/run/build metadata ---'
rg -n -i 'revision|commit|sha|gitlink|submodule|HEAD|Run [0-9]|binary|build|implemented|submitted|final state' "$file" | sed -n '1,240p'
printf '%s\n' '--- repository revisions ---'
git rev-parse HEAD
git status --short
git ls-tree HEAD -- src/mongo/db/modules/eloq/data_substrate
printf '%s\n' '--- gitlink path candidates ---'
git ls-files -s | rg 'data_substrate|eloq'Repository: eloqdata/eloqdoc
Length of output: 21679
🏁 Script executed:
#!/bin/bash
set -eu
file="DDL_CATALOG_LIVELOCK_PLAN.md"
printf '%s\n' '--- bfa7ab3402 context ---'
rg -n -C 8 'bfa7ab3402' "$file" || true
printf '%s\n' '--- implementation/build context ---'
sed -n '990,1165p' "$file"
printf '%s\n' '--- Run 5 through Run 6 ---'
sed -n '1460,1715p' "$file"
printf '%s\n' '--- document commit metadata ---'
git show -s --format='HEAD=%H%nsubject=%s%ncommitter=%cI' HEAD
git log -n 5 --format='%H %s' -- DDL_CATALOG_LIVELOCK_PLAN.mdRepository: eloqdata/eloqdoc
Length of output: 26270
Reconcile implementation status and Run 6 provenance.
- Replace
Status: proposed, not implemented; Part II and §13.9 record implemented fixes and a successful Run 6. - Mark §13.8’s “not patched here” DataSync disposition as superseded; §13.9 includes those fixes in the tested binary.
- Record the exact eloqdoc commit and
data_substrategitlink used for Run 6. The current document does not identify them.
🤖 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` around lines 3 - 14, Update the document’s
implementation status to reflect the implemented fixes and successful Run 6,
revise §13.8 to show its “not patched here” DataSync disposition is superseded
by §13.9, and add the exact eloqdoc commit plus data_substrate gitlink used for
Run 6.
|
|
||
| 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
Specify languages for all fenced code blocks.
markdownlint-cli2 reports MD040 at these four fences. Use text for log and trace output, and text or cpp for the transaction-flow snippet.
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 all four fenced code blocks
in DDL_CATALOG_LIVELOCK_PLAN.md to include an explicit language tag: use text
for log or trace output and text or cpp for the transaction-flow snippet,
resolving markdownlint MD040 without changing the block contents.
Source: Linters/SAST tools
Comment on lines
+426
to
+450
| **Accepted scope decision.** This keys the behaviour on `is_for_write_` rather | ||
| than a new per-request protocol field. Review round 1 raised that this changes | ||
| a generic `read_local_ && is_for_write_` semantic shared with EloqKV and | ||
| EloqSQL, which have not been audited. The alternative was designed and then | ||
| rejected by the maintainer in favour of the simpler form. This is a deliberate | ||
| decision, not an oversight: | ||
|
|
||
| * Every in-repository `ReadLocal` caller passes `is_for_write=false` and is | ||
| therefore unaffected: all five `sequences.cpp` catalog reads, | ||
| `ReadLocalOperation`, `LockWriteRangeBucketsOp`, the scan-next range lock, | ||
| and `BatchReadOperation`. | ||
| * No recovery, replay, standby, or checkpoint path constructs an affected | ||
| request. | ||
| * The residual exposure is EloqKV and EloqSQL, which are out of scope here. | ||
|
|
||
| **Known gap: read-local scans.** `Process(ScanOpenOperation&)` | ||
| (`tx_service/src/tx_execution.cpp:2347-2363`) is a separate path that also | ||
| hardcodes `CcProtocol::Locking` and passes `is_for_write` through; the scanner | ||
| deduces `WriteIntent` under that protocol | ||
| (`tx_service/src/cc/local_cc_handler.cpp:1105-1112`). A read-local scan for | ||
| write would still take a blocking write intent and could reproduce this | ||
| livelock shape. EloqDoc does construct `ScanOpenTxRequest` | ||
| (`src/mongo/db/modules/eloq/src/eloq_cursor.h:91`, reset at | ||
| `eloq_cursor.cpp:73`) but passes `is_read_local=false`, so no read-local scan | ||
| exists there today. Left unchanged and recorded rather than fixed |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
git -C src/mongo/db/modules/eloq/data_substrate grep -n -E \
'ReadLocal|is_for_write_|read_local_|ScanOpenTxRequest' -- '*.h' '*.cc' '*.cpp'
rg -n -C 3 \
'ReadLocal|is_for_write_|read_local_|ScanOpenTxRequest' \
src/mongo/db/modules/eloqRepository: eloqdata/eloqdoc
Length of output: 154
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '== repository paths =='
git ls-files | rg '(^|/)(tx_execution\.cpp|local_cc_handler\.cpp|eloq_cursor\.(h|cpp)|.*(sequences|operation).*\.(h|cc|cpp)|DDL_CATALOG_LIVELOCK_PLAN\.md)$' | head -200
printf '%s\n' '== read-local symbols and request fields =='
rg -n -C 4 --hidden -g '!node_modules' -g '!build' -g '!dist' \
'ReadLocal|is_for_write_|read_local_|ScanOpenTxRequest' . | head -1200Repository: eloqdata/eloqdoc
Length of output: 11815
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '== submodules and repository metadata =='
git submodule status 2>/dev/null || true
git ls-files | rg '(^|/)(\.gitmodules|data_substrate|tx_service|EloqKV|EloqSQL)' || true
printf '%s\n' '== EloqDoc scan request construction and assignments =='
rg -n -C 8 \
'ScanOpenTxRequest|is_read_local_|is_for_write_|OpenTxScan' \
src/mongo/db/modules/eloq/src/eloq_cursor.cpp \
src/mongo/db/modules/eloq/src/eloq_cursor.h
printf '%s\n' '== all source request constructions in the checked-out tree =='
rg -n -C 3 --hidden -g '*.{h,hpp,cc,cpp,cxx}' \
'ReadTxRequest|ScanOpenTxRequest|read_local_|is_for_write_' . | head -1600Repository: eloqdata/eloqdoc
Length of output: 11907
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '== submodule definition and pinned revision =='
cat .gitmodules
git ls-tree HEAD src/mongo/db/modules/eloq/data_substrate
printf '%s\n' '== EloqDoc ReadTxRequest and BatchReadTxRequest arguments =='
sed -n '350,390p;405,435p;475,510p;560,595p' \
src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp
printf '%s\n' '== EloqDoc ScanOpenTxRequest initialization =='
sed -n '35,85p' src/mongo/db/modules/eloq/src/eloq_cursor.cpp
printf '%s\n' '== exact local request-field assignments in EloqDoc =='
rg -n '(^|[^[:alnum:]_])(read_local_|is_read_local_|is_for_write_)[[:space:]]*=' \
src/mongo/db/modules/eloq --glob '*.{h,hpp,cc,cpp,cxx}'Repository: eloqdata/eloqdoc
Length of output: 9093
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '== readCatalog call sites and arguments =='
rg -n -C 5 'readCatalog\s*\(' src/mongo/db --glob '*.{h,hpp,cc,cpp,cxx}' | head -1200
printf '%s\n' '== EloqRecoveryUnit read methods and parameter flow =='
rg -n -C 8 \
'readCatalog|batchReadCatalog|getKV|isForWrite|isForShare|readLocal' \
src/mongo/db/modules/eloq/src/eloq_recovery_unit.h \
src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp | head -1800
printf '%s\n' '== request constructor declarations in the available submodule index =='
git ls-tree -r HEAD src/mongo/db/modules/eloq/data_substrate | head -100Repository: eloqdata/eloqdoc
Length of output: 36655
Audit the affected catalog write callers before accepting this scope.
EloqRecoveryUnit::readCatalog() sets read_local_ to true and forwards isForWrite as is_for_write_. eloq_record_store.cpp calls it with true at lines 229, 296, and 359. eloq_kv_engine.cpp also forwards isForWrite at line 311. These callers contradict the claim that every in-repository caller passes false. Audit their dependence on blocking WriteIntent, plus the tx_service, EloqKV, EloqSQL, and mixed-version paths.
🤖 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` around lines 426 - 450, Audit the
accepted-scope claims around read-local write requests rather than treating them
as unaffected: trace EloqRecoveryUnit::readCatalog callers in
eloq_record_store.cpp, EloqKV forwarding in eloq_kv_engine.cpp, and the
corresponding tx_service, EloqKV, EloqSQL, and mixed-version paths. Verify
whether each can set read_local_ with is_for_write_ true and depend on blocking
WriteIntent; update the scope decision and affected-caller inventory to reflect
the findings, or adjust the implementation to preserve their required behavior.
Comment on lines
337
to
+354
| opCtx->recoveryUnit()->onCommit( | ||
| [this](boost::optional<Timestamp>) { notifyCappedWaitersIfNeeded(); }); | ||
| [notifier = _cappedNotifier](boost::optional<Timestamp>) { | ||
| // Capture the notifier, not the Collection. Eloq refreshes the cached Collection | ||
| // while committing catalog metadata, before RecoveryUnit callbacks run, so a | ||
| // captured `this` can already be freed here -- the same hazard the eloq guard in | ||
| // IndexCatalogEntryImpl::setMultikey was added for. Holding a shared_ptr copy keeps | ||
| // the notifier alive independently of the Collection. | ||
| // | ||
| // The haveCappedWaiters() short-circuit is dropped along with `this`: its use_count | ||
| // test cannot be expressed once the Collection may be gone, and it was only an | ||
| // optimisation. notifyAll() on a notifier nobody waits on takes one uncontended | ||
| // mutex and bumps a version that no waiter reads; on a killed notifier it is | ||
| // likewise harmless, because waiters exit on the _dead flag. Non-capped collections | ||
| // have a null notifier and do nothing at all. | ||
| if (notifier) { | ||
| notifier->notifyAll(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find remaining callers of the capped-waiter helpers.
rg -nP -C3 '\b(haveCappedWaiters|notifyCappedWaitersIfNeeded)\s*\(' --type=cpp --type=cpp-headerRepository: eloqdata/eloqdoc
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- helper definitions and callers ---'
rg -nP -C4 '\b(haveCappedWaiters|notifyCappedWaitersIfNeeded)\s*\(' --glob '*.cpp' --glob '*.h' --glob '*.hpp' --glob '*.inl' .
printf '%s\n' '--- definitions in collection implementation ---'
rg -n -C12 'haveCappedWaiters|notifyCappedWaitersIfNeeded' src/mongo/db/catalog/collection_impl.cpp src/mongo/db/catalog/collection_impl.hRepository: eloqdata/eloqdoc
Length of output: 31521
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- oplog caller context ---'
sed -n '1665,1720p' src/mongo/db/repl/oplog.cpp
printf '%s\n' '--- WiredTiger oplog manager caller context ---'
sed -n '165,265p' src/mongo/db/storage/wiredtiger/wiredtiger_oplog_manager.cpp
printf '%s\n' '--- record-store capped callback plumbing ---'
rg -n -C5 'setCappedCallback|getCappedInsertNotifier|CappedCallback|_cappedNotifier' \
src/mongo/db/catalog/collection_impl.cpp \
src/mongo/db/catalog/collection_impl.h \
src/mongo/db/storage \
src/mongo/db/repl
printf '%s\n' '--- notifier implementation and use_count-related behavior ---'
rg -n -C8 'class CappedInsertNotifier|notifyAll\(|_dead|waitUntil' src/mongo/dbRepository: eloqdata/eloqdoc
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- _localOplogCollection declarations and assignments ---'
rg -n -C4 '_localOplogCollection' src/mongo/db/repl/oplog.cpp src/mongo/db/repl/oplog.h
printf '%s\n' '--- production call sites only ---'
rg -nP '\b(haveCappedWaiters|notifyCappedWaitersIfNeeded)\s*\(' src/mongo \
--glob '!**/*test*' --glob '!src/mongo/db/catalog/collection_impl.cpp' \
--glob '!src/mongo/db/catalog/collection_impl.h' --glob '!src/mongo/db/catalog/collection.h'
printf '%s\n' '--- CollectionImpl notifier initialization and callback registration ---'
rg -n -C8 '_cappedNotifier|setCappedCallback|CappedCallback' \
src/mongo/db/catalog/collection_impl.cpp src/mongo/db/catalog/collection_impl.h
printf '%s\n' '--- CollectionImpl insert method signatures and callback regions ---'
sed -n '285,430p' src/mongo/db/catalog/collection_impl.cppRepository: eloqdata/eloqdoc
Length of output: 29648
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- WiredTiger oplog manager decision and notification flow ---'
sed -n '175,258p' src/mongo/db/storage/wiredtiger/wiredtiger_oplog_manager.cpp
printf '%s\n' '--- WiredTiger record-store callback methods ---'
sed -n '1265,1312p' src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp
printf '%s\n' '--- callback setup and wait lifecycle references ---'
rg -n -C6 'setCappedCallback|haveCappedWaiters\(\)|notifyCappedWaitersIfNeeded\(\)' \
src/mongo/db/storage/wiredtiger src/mongo/db/catalog/collection_impl.cpp \
--glob '!**/*test*'
printf '%s\n' '--- commit callback registration semantics ---'
rg -n -C6 'onCommit\(' src/mongo/db/recovery_unit.h src/mongo/db/storage/recovery_unit.h \
src/mongo/db/modules/eloq/srcRepository: eloqdata/eloqdoc
Length of output: 23084
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
collection = Path("src/mongo/db/catalog/collection_impl.cpp").read_text()
record_store = Path("src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp").read_text()
oplog_manager = Path("src/mongo/db/storage/wiredtiger/wiredtiger_oplog_manager.cpp").read_text()
oplog = Path("src/mongo/db/repl/oplog.cpp").read_text()
assert collection.count("[notifier = _cappedNotifier]") == 3
assert "return _cappedNotifier.use_count() > 1;" in collection
assert "if (haveCappedWaiters())" in collection
assert "return _cappedCallback && _cappedCallback->haveCappedWaiters();" in record_store
assert "oplogRecordStore->haveCappedWaiters()" in oplog_manager
assert "oplogRecordStore->notifyCappedWaitersIfNeeded();" in oplog_manager
assert "_localOplogCollection->notifyCappedWaitersIfNeeded();" in oplog
print("pending commit notifier copies are counted by CollectionImpl::haveCappedWaiters")
print("WiredTigerOplogManager reaches CollectionImpl::haveCappedWaiters through WiredTigerRecordStore")
print("the waiter check controls the oplog journal-delay loop")
print("the notification path also invokes CollectionImpl::haveCappedWaiters")
PYRepository: eloqdata/eloqdoc
Length of output: 453
Preserve the haveCappedWaiters() optimization
A pending commit callback makes CollectionImpl::haveCappedWaiters() return true without an await-data cursor. WiredTigerOplogManager uses this result to bypass its journal delay, which can cause unnecessary synchronization. Keep the waiter check in a lifetime-safe form, or track waiter presence separately from notifier ownership.
🤖 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 `@src/mongo/db/catalog/collection_impl.cpp` around lines 337 - 354, Preserve
the haveCappedWaiters() optimization in the onCommit callback without capturing
CollectionImpl or risking a dangling this pointer. Capture or record waiter
presence separately before registering the callback, then only call
notifier->notifyAll() when waiters were present and the notifier remains valid;
retain the existing null-notifier behavior.
Comment on lines
897
to
+904
| } else { | ||
| MONGO_LOG(1) << "nss: " << nss.toStringData() | ||
| << " version changed. old: " << found->catalogVersion() | ||
| << ", new: " << version; | ||
| auto& uuidCatalog = UUIDCatalog::get(opCtx); | ||
| uuidCatalog.removeUUIDCatalogEntry(found->uuid().get()); | ||
| _clearCollectionCache(opCtx, nss.ns(), "collection version changed", true); | ||
| _clearCollectionCache( | ||
| opCtx, nss.ns(), "collection version changed", true, found.get()); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the optional UUID before removeUUIDCatalogEntry.
Line 902 calls found->uuid().get() without checking that the optional is engaged. Line 887 in the same function checks it with if (auto uuid = found->uuid()), so a disengaged UUID is reachable on this code path too. A collection without a UUID makes this dereference a disengaged boost::optional.
The version-mismatch path is now taken constantly under concurrent DDL, which increases exposure.
🐛 Proposed fix
auto& uuidCatalog = UUIDCatalog::get(opCtx);
- uuidCatalog.removeUUIDCatalogEntry(found->uuid().get());
+ if (auto foundUuid = found->uuid()) {
+ uuidCatalog.removeUUIDCatalogEntry(foundUuid.get());
+ }
_clearCollectionCache(
opCtx, nss.ns(), "collection version changed", true, found.get());📝 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
| } else { | |
| MONGO_LOG(1) << "nss: " << nss.toStringData() | |
| << " version changed. old: " << found->catalogVersion() | |
| << ", new: " << version; | |
| auto& uuidCatalog = UUIDCatalog::get(opCtx); | |
| uuidCatalog.removeUUIDCatalogEntry(found->uuid().get()); | |
| _clearCollectionCache(opCtx, nss.ns(), "collection version changed", true); | |
| _clearCollectionCache( | |
| opCtx, nss.ns(), "collection version changed", true, found.get()); | |
| } else { | |
| MONGO_LOG(1) << "nss: " << nss.toStringData() | |
| << " version changed. old: " << found->catalogVersion() | |
| << ", new: " << version; | |
| auto& uuidCatalog = UUIDCatalog::get(opCtx); | |
| if (auto foundUuid = found->uuid()) { | |
| uuidCatalog.removeUUIDCatalogEntry(foundUuid.get()); | |
| } | |
| _clearCollectionCache( | |
| opCtx, nss.ns(), "collection version changed", true, found.get()); |
🤖 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 `@src/mongo/db/catalog/database_impl.cpp` around lines 897 - 904, Guard the
UUID optional in the version-mismatch branch before calling
UUIDCatalog::removeUUIDCatalogEntry. Reuse the established conditional pattern
from the nearby check in the same function, removing the catalog entry only when
found->uuid() is engaged, while preserving the subsequent _clearCollectionCache
call.
Comment on lines
296
to
+307
| auto [exist, errorCode] = ru->readCatalog(catalogKey, catalogRecord, true); | ||
| if (errorCode != txservice::TxErrorCode::NO_ERROR) { | ||
| if (errorCode == txservice::TxErrorCode::WRITE_WRITE_CONFLICT) { | ||
| MONGO_LOG(1) << "Eloq readCatalog error with write intent. Another transaction " | ||
| "may do DDL on the same table."; | ||
| return {ErrorCodes::WriteConflict, | ||
| "[Create Table] Another transaction may do DDL on the same table"}; | ||
| } else { | ||
| MONGO_LOG(0) << "Eloq readCatalog error with write intent." | ||
| << txservice::TxErrorMessage(errorCode); | ||
| return {ErrorCodes::InternalError, txservice::TxErrorMessage(errorCode)}; | ||
| } | ||
| // The whole conflict group has to leave as a WriteConflictException. Returning a | ||
| // Status carrying ErrorCodes::WriteConflict looks equivalent but is not: at the | ||
| // first uassertStatusOK it becomes ExceptionFor<ErrorCodes::WriteConflict>, a | ||
| // sibling type that no writeConflictRetry catches. The previous form also mapped | ||
| // DEAD_LOCK_ABORT and UPSERT_TABLE_ACQUIRE_WRITE_INTENT_FAIL to InternalError, | ||
| // which no caller retries at all. | ||
| ThrowIfWriteConflict(errorCode); | ||
| MONGO_LOG(0) << "Eloq readCatalog error with write intent." | ||
| << txservice::TxErrorMessage(errorCode); | ||
| return {ErrorCodes::InternalError, txservice::TxErrorMessage(errorCode)}; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Retry transient catalog reads before failing insertRecord.
readCatalog() runs once here. ThrowIfWriteConflict() returns for READ_CATALOG_FAIL. Lines 305-307 then return InternalError.
This makes collection creation fail when the catalog fetch is in progress. Match the bounded retry behavior in deleteRecord() and updateRecord(). Add a regression test that returns READ_CATALOG_FAIL before a successful catalog read.
🤖 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 `@src/mongo/db/modules/eloq/src/eloq_record_store.cpp` around lines 296 - 307,
The insertRecord catalog-read path must retry transient READ_CATALOG_FAIL
results instead of immediately returning InternalError. Update the flow around
ru->readCatalog and ThrowIfWriteConflict to use the same bounded retry behavior
as deleteRecord() and updateRecord(), while preserving existing handling for
non-retryable errors. Add a regression test that produces READ_CATALOG_FAIL
once, then a successful catalog read, and verifies insertRecord succeeds.
liunyl
force-pushed
the
fix/ddl-catalog-livelock
branch
from
August 11, 2026 04:42
50148e5 to
fc6fc42
Compare
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>
…g 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>
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>
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>
liunyl
force-pushed
the
fix/ddl-catalog-livelock
branch
from
August 11, 2026 04:56
fc6fc42 to
ac58d0b
Compare
thweetkomputer
approved these changes
Aug 11, 2026
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>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mongo/db/concurrency/write_conflict_exception.h (1)
94-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the retry-order documentation.
The function comment states that retry handling waits before cleanup. Lines 94-99 now call
abandonSnapshot()beforeWriteConflictException::logAndBackoff(). Update the comment to describe the new order.🤖 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 `@src/mongo/db/concurrency/write_conflict_exception.h` around lines 94 - 99, Update the retry-order documentation in the function comment surrounding abandonSnapshot() to state that the snapshot is abandoned before WriteConflictException::logAndBackoff(), rather than describing cleanup after the wait. Keep the existing explanation of releasing the loser’s locks before backoff aligned with this order.
🤖 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.
Nitpick comments:
In `@src/mongo/db/concurrency/write_conflict_exception.h`:
- Around line 94-99: Update the retry-order documentation in the function
comment surrounding abandonSnapshot() to state that the snapshot is abandoned
before WriteConflictException::logAndBackoff(), rather than describing cleanup
after the wait. Keep the existing explanation of releasing the loser’s locks
before backoff aligned with this order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2176b8ee-f106-4f9b-9d64-48931a0988f4
📒 Files selected for processing (1)
src/mongo/db/concurrency/write_conflict_exception.h
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>
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>
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>
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>
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>
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
The mongo-layer half of the DDL catalog work (tx_service half: eloqdata/tx_service#551, pulled in by the submodule bump commit). Three themes, one commit each:
Status{WriteConflict}becameExceptionFor<WriteConflict>— a sibling typewriteConflictRetrynever catches — so retryable DDL/DML conflicts were fatal to commands, and one collMod-pathInternalErrorreached aMultiIndexBlockcleanup fassert that killed the node.ThrowIfWriteConflict()centralizes the mapping (TxErrorCodeToMongoStatusroutes through it so they cannot drift); the catalog record-store retry loops, table create/drop/update paths, and the separate-transaction RecoveryUnit swaps are now exception-correct, with unit tests asserting the exception type, the conflict-group membership, and that transientREAD_CATALOG_FAILstays in-loop.getCollection()rebuilds cached Collections on every catalog version move while callers hold raw pointers across coroutine yields and into onCommit callbacks (EloqLockerNoopgives none of upstream's lock-manager exclusion). Destroy-on-refresh was a proven UAF; the interim mitigation leaked every evicted object. The maps now holdshared_ptr, every hand-out pins on the RecoveryUnit until the pooled OperationContext is recycled (pins follow stashed transactions), a mutex covers the pure map operations that background threads (TTLMonitor — two observed crashes) race, stale entries are evicted on vanished tables, andcollections()returns a by-value snapshot (the shared_collectionsViewandforViewmode are gone).Collection*(fixed withenable_shared_from_this+ pins at ClientCursor and DocumentSourceCursor, covering$lookup/$graphLookupforeign pipelines);cleanupExecutor()re-resolving the namespace on the no-throw disposal path (EloqDoc'sgetCollectionthrows under DDL —Pipeline::dispose()turns that intostd::terminate; observed node death) — it now uses the captured, pinned manager; andEloqRecordStoreCursor::save()keeping its scan open across wire operations, driving a committed-and-recycled txm from the next getMore (observed TxProcessor SIGSEGV on a freed stack-allocated request) — both cursor types now close at save and lazily re-seek on the current transaction, matching the pre-existingsaveUnpositioned()idiom.Verification
$lookupdrains across createIndexes, drop/recreate staleness, multikey, TTL sweep, and an explicit assertion that concurrent DDL kills collection-managed cursors withCursorNotFound(the designed semantic).ttlMonitorEnabled: true, with a parked-aggregation op (50%$lookup): the final run completed with zero crashes, zero cores, zero wedges (no operation ever exceeded 120 s), conflict-class 0.07%, canaries 29/29, per-minute completions never zero. Full run history, crash/wedge forensics, and review-gate record:DDL_CATALOG_LIVELOCK_PLAN.md§13.Notes
count()'s stats-basednumRecordslag (pre-existing, documented "should not rely on it").🤖 Generated with Claude Code
Summary by CodeRabbit
getMoreoperations.