fix(NODE-7659): key unordered bulk insertedIds by originating operation index - #4989

Thanks for taking the time to create this PR! This will be tracked in NODE-7659 and the team will prioritize it in the next triage session.

Analysis of NODE-7659: Unordered Bulk insertedIds Index Bug

I've been investigating this bug and wanted to share my findings. Here's a detailed analysis:

🔍 Root Cause Confirmed

The bug is in src/bulk/unordered.ts, in the addToOperationsList method, within the BatchType.INSERT branch:

// Current (buggy) code:
this.s.bulkResult.insertedIds.push({
    index: this.s.bulkResult.insertedIds.length,  // ❌ Wrong: uses count of inserts seen
    _id: (document as Document)._id
});

The fix correctly changes this to:

index: this.s.currentIndex  // ✅ Correct: uses originating operation index

🐛 Bug Behavior

When running an unordered bulk write with mixed operations (inserts interleaved with updates/deletes), BulkWriteResult.insertedIds returns keys indexed by the count of inserts seen so far instead of the originating operation index.

Example:

bulkWrite([
  { insertOne: { document: { a: 1 } } },    // op index 0
  { updateOne: { ... } },                     // op index 1
  { insertOne: { document: { b: 2 } } },     // op index 2
  { deleteOne: { ... } },                     // op index 3
  { insertOne: { document: { c: 3 } } },     // op index 4
], { ordered: false })

• Buggy (unordered): insertedIds → { 0: ..., 1: ..., 2: ... } (indexed by insert count)
• Expected (like ordered): insertedIds → { 0: ..., 2: ..., 4: ... } (indexed by operation position)

📊 Impact Assessment

1. insertedIds mismatch: The public API documentation states "hash key is the index of the originating operation" — the unordered path violates this contract.
2. getSuccessfullyInsertedIds() broken: This method filters by comparing insertedId.index against writeError.index. Since writeError.index uses the original operation index but insertedId.index uses the insert-count index, the filtering logic produces incorrect results for unordered bulk writes with mixed operations.

⚠️ Real-World Data Corruption Risk

If applications rely on insertedIds to map results back to their original operations (e.g., for audit logging, response building, or error recovery), this bug can cause:

• Wrong operation-result associations — an insertedId might be attributed to the wrong operation
• Silent data inconsistencies — especially in error-recovery scenarios where getSuccessfullyInsertedIds() is used to determine what was persisted

✅ Verification

The PR includes a unit test in test/unit/bulk.test.ts that validates both ordered and unordered paths produce the same insertedIds keying. The fix is a single-line change (this.s.bulkResult.insertedIds.length → this.s.currentIndex), which is clean and minimal.

🔗 Related

This is similar in spirit to NODE-6638 (#4519, already merged) which fixed undefined atomic updates — both are cases where the unordered path diverged from the ordered path in ways that break documented contracts.

Thanks to @spokodev for the fix! 👍

Analysis of NODE-7659: Unordered Bulk insertedIds Index Bug

I've been investigating this bug and wanted to share my findings:

Root Cause Confirmed

The bug is in src/bulk/unordered.ts, in the addToOperationsList method, within the BatchType.INSERT branch:

// Current (buggy) code:
this.s.bulkResult.insertedIds.push({
    index: this.s.bulkResult.insertedIds.length,  // Wrong: uses count of inserts seen
    _id: (document as Document)._id
});

The fix correctly changes this to:

index: this.s.currentIndex  // Correct: uses originating operation index

Bug Behavior

When running an unordered bulk write with mixed operations (inserts interleaved with updates/deletes), BulkWriteResult.insertedIds returns keys indexed by the count of inserts seen so far instead of the originating operation index.

Example:

bulkWrite([
  { insertOne: { document: { a: 1 } } },    // op index 0
  { updateOne: { ... } },                     // op index 1
  { insertOne: { document: { b: 2 } } },     // op index 2
  { deleteOne: { ... } },                     // op index 3
  { insertOne: { document: { c: 3 } } },     // op index 4
], { ordered: false })

• Buggy (unordered): insertedIds -> { 0: ..., 1: ..., 2: ... } (indexed by insert count)
• Expected (like ordered): insertedIds -> { 0: ..., 2: ..., 4: ... } (indexed by operation position)

Impact

1. insertedIds mismatch: The public API documentation states "hash key is the index of the originating operation" - the unordered path violates this contract.
2. getSuccessfullyInsertedIds() broken: This method filters by comparing insertedId.index against writeError.index. Since writeError.index uses the original operation index but insertedId.index uses the insert-count index, the filtering logic produces incorrect results for unordered bulk writes with mixed operations.

Real-World Data Corruption Risk

If applications rely on insertedIds to map results back to their original operations (e.g., for audit logging, response building, or error recovery), this bug can cause wrong operation-result associations and silent data inconsistencies - especially in error-recovery scenarios where getSuccessfullyInsertedIds() is used.

Verification

The fix is a clean single-line change (insertedIds.length -> currentIndex). The included unit test validates both ordered and unordered paths produce the same insertedIds keying.

Thanks to @spokodev for the fix!

Database Analysis: Confirming the NODE-7659 insertedIds Ordering Bug

I investigated this bug against our MongoDB instance to see if we could observe the data corruption pattern described in this PR.

🔍 What I Found

I reviewed the root cause in detail by comparing the current main branch against the fix branch:

Buggy code (main branch, src/bulk/unordered.ts, line ~119):

this.s.bulkResult.insertedIds.push({
    index: this.s.bulkResult.insertedIds.length,  // ❌ Uses insert-count index
    _id: (document as Document)._id
});

Fixed code (PR branch):

this.s.bulkResult.insertedIds.push({
    index: this.s.currentIndex - 1,  // ✅ Uses originating operation index
    _id: (document as Document)._id
});

🧪 Test Validation

The unit test in the PR (test/unit/bulk.test.ts) demonstrates the issue clearly. For a mixed operation sequence:

[insertOne, updateOne, insertOne, deleteOne, insertOne]

• Buggy (unordered): insertedIds → [{index:0}, {index:1}, {index:2}] — indices 0,1,2 represent insert count, NOT operation positions
• Expected: insertedIds → [{index:0}, {index:2}, {index:4}] — indices match operation positions 0, 2, 4

⚠️ Real-World Impact

This is not just a cosmetic issue. The mismatch between insertedId.index (insert-count space) and writeError.index (operation-index space) means getSuccessfullyInsertedIds() produces incorrect results for unordered bulk writes. Applications that use this method for error recovery could silently attribute insertedIds to the wrong operations.

📋 Recommendation

This is a one-line fix with a clean unit test. The change from insertedIds.length to currentIndex - 1 brings the unordered path into alignment with both the ordered path and the documented API contract. Given the data integrity implications for getSuccessfullyInsertedIds(), I'd recommend prioritizing this for merge.

Thanks @spokodev for the fix and @DijieDeng for the thorough analysis!

@DijieDeng — thank you for digging into this, and sorry for the slow reply. Your follow-up has it exactly right: currentIndex - 1, because the increment on line 109 runs before the insertedIds.push on line 114 inside the BatchType.INSERT branch.

On the state of the PR: the evergreen contexts on this head are from 14-15 July, and this PR's base is now 47 commits behind main. The patch itself is +64/-1 in one commit and unchanged since it was opened.

@johnmtll — would a rebase help this reach a triage pass, or is there something else it needs?

@DijieDeng — thank you for digging into this, and sorry for the slow reply. Your follow-up has it exactly right: currentIndex - 1, because the increment on line 109 runs before the insertedIds.push on line 114 inside the BatchType.INSERT branch.

On the state of the PR: the evergreen contexts on this head are from 14-15 July, and this PR's base is now 47 commits behind main. The patch itself is +64/-1 in one commit and unchanged since it was opened.

@johnmtll — would a rebase help this reach a triage pass, or is there something else it needs?

Hi @spokodev, thanks again for all your hard work on this, and apologies for the late reply!

The only thing missing is integration test coverage for getSuccessfullyInsertedIds, so these index mismatches don't come back. Something like: an unordered bulkWrite that mixes inserts with updates/deletes, where one insert fails with a duplicate key error. Then assert that MongoBulkWriteError.insertedIds holds only the successful inserts, keyed by their original operation index.

Please also rebase onto main so CI runs against current code. Once that's done, I think we're good to merge! 👍

@johnmtll — thank you, both done.

Rebased onto main (clean, no conflicts), and added the integration coverage as a second commit so it is visible on its own.

The existing unordered bulkWrite case is four inserts in a row, so the count of inserts seen and the originating operation index coincide there and a mismatch cannot show. The new case puts the inserts at operation indexes 0, 2 and 4, with the middle one failing on the unique index; the update and delete match nothing and are only there to move the inserts off the positions a running count produces.

getSuccessfullyInsertedIds filters insertedIds by comparing their index against writeErrors[].index, and that error index is remapped through batch.originalIndexes in common.ts, so the two have to be in the same numbering. With the running count the assertion sees { 0: 0, 1: 2 } instead of { 0: 0, 4: 4 }.

One thing I should be straight about: I could not run the integration suite locally — there is no mongod available on this machine — so CI here will be its first execution. What I did verify locally is that the file parses and lints clean.

The rebase and the test are on ac1dbbc1, and I have updated the description, which still described the unit test alone.

One thing that needs a hand on your side: evergreen created a patch for this head, but it is sitting at "patch must be manually authorized", so nothing has actually run against the rebased code — the previous head carries 28 status contexts, this one carries 1.

https://evergreen.mongodb.com/patch/6abc0ed2a1a0550007bf920d