fix: Server fails to start with IndexKeySpecsConflict on _Idempotency TTL index on MongoDB 8.0#10537
Conversation
… TTL index on MongoDB 8.0
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
📝 WalkthroughWalkthroughMongoStorageAdapter's ensureIndex now catches MongoDB error codes 85 (IndexOptionsConflict) and 86 (IndexKeySpecsConflict), logging a warning and resolving instead of throwing. A new spec test verifies that a pre-existing conflicting ttl index on _Idempotency no longer crashes startup. ChangesIndex conflict tolerance
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant DatabaseController
participant MongoStorageAdapter
participant MongoCollection
DatabaseController->>MongoStorageAdapter: ensureIndex(_Idempotency, ttl, ...)
MongoStorageAdapter->>MongoCollection: createIndex(spec)
MongoCollection-->>MongoStorageAdapter: error code 85/86 (conflict)
MongoStorageAdapter->>MongoStorageAdapter: log warning, keep existing index
MongoStorageAdapter-->>DatabaseController: resolve successfully
Related issues: Suggested labels: bug, mongodb Suggested reviewers: dblythy, mtrezza 🐰 A conflicting index once made servers crash, 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.1)src/Adapters/Storage/Mongo/MongoStorageAdapter.jsFile contains syntax errors that prevent linting: Line 3: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 160: classes can only implement interfaces in TypeScript files; Line 162: return types can only be used in TypeScript files; Line 163: return types can only be used in TypeScript files; Line 164: return types can only be used in TypeScript files; Line 165: return types can only be used in TypeScript files; Line 166: return types can only be used in TypeScript files; Line 167: return types can only be used in TypeScript files; Line 167: expected a semicolon to end the class property, but found none; Line 167: Expected an identifier, a string literal, a number literal, a private field name, or a computed name but instead found '; Line 167: type parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 167: Expected a class parameters but instead found ';'.; Line 168: ret ... [truncated 15553 characters] ... Illegal return statement outside of a function; Line 1279: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 1280: Illegal return statement outside of a function; Line 1290: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 1293: Illegal return statement outside of a function; Line 1296: expected Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #10537 +/- ##
=======================================
Coverage 92.66% 92.66%
=======================================
Files 193 193
Lines 16981 16984 +3
Branches 248 248
=======================================
+ Hits 15736 15739 +3
Misses 1224 1224
Partials 21 21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![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)
spec/MongoStorageAdapter.spec.js (1)
904-930: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTest leaves the
_Idempotencyttlindex permanently altered for later tests.The test replaces the
ttlindex withexpireAfterSeconds: 100, sparse: trueand, given the new "keep on conflict" behavior inensureIndex, no subsequent call in the suite will ever fix it back to the real config. This risks polluting state for other tests/spec files running in the same database.Consider restoring the original TTL index (or dropping the collection) in an
afterEach/finallyfor this test.Proposed cleanup
it('ensureIndex tolerates an existing conflicting index instead of crashing startup (`#10431`)', async () => { await reconfigureServer({ databaseAdapter: undefined, databaseURI, databaseOptions: {} }); const adapter = Config.get(Parse.applicationId).database.adapter; const collection = await adapter._adaptiveCollection('_Idempotency'); // ... await collection._mongoCollection.dropIndex('ttl').catch(() => {}); await collection._mongoCollection.createIndex( { expire: 1 }, { name: 'ttl', expireAfterSeconds: 100, sparse: true } ); - const schema = { + try { + const schema = { fields: { ...SchemaController.defaultColumns._Default, ...SchemaController.defaultColumns._Idempotency, }, - }; - await expectAsync( - adapter.ensureIndex('_Idempotency', schema, ['expire'], 'ttl', false, { ttl: 0 }) - ).toBeResolved(); - const indexes = await getIndexes('_Idempotency'); - expect(indexes.find(idx => idx.name === 'ttl')).toBeDefined(); + }; + await expectAsync( + adapter.ensureIndex('_Idempotency', schema, ['expire'], 'ttl', false, { ttl: 0 }) + ).toBeResolved(); + const indexes = await getIndexes('_Idempotency'); + expect(indexes.find(idx => idx.name === 'ttl')).toBeDefined(); + } finally { + await collection._mongoCollection.dropIndex('ttl').catch(() => {}); + } });🤖 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 `@spec/MongoStorageAdapter.spec.js` around lines 904 - 930, The _Idempotency ttl index is intentionally modified in this spec and now persists for later tests because ensureIndex keeps the conflicting index instead of recreating it. Update the test around adapter.ensureIndex and getIndexes to clean up after itself by restoring the original ttl index configuration or dropping the _Idempotency collection in a finally/afterEach block, so later specs don’t inherit the altered index state.
🤖 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 `@src/Adapters/Storage/Mongo/MongoStorageAdapter.js`:
- Around line 821-836: The current `ensureIndex` catch block in
`MongoStorageAdapter` treats every 85/86 conflict as success, which can mask
real index-definition changes. Update the logic in this `catch` path to first
compare the existing index spec against the requested one (using the surrounding
`indexName`/`className` index creation flow), and only ignore the error when
they are functionally equivalent. If the specs differ, either drop and recreate
the index or surface the error through `handleError` instead of returning
success.
---
Nitpick comments:
In `@spec/MongoStorageAdapter.spec.js`:
- Around line 904-930: The _Idempotency ttl index is intentionally modified in
this spec and now persists for later tests because ensureIndex keeps the
conflicting index instead of recreating it. Update the test around
adapter.ensureIndex and getIndexes to clean up after itself by restoring the
original ttl index configuration or dropping the _Idempotency collection in a
finally/afterEach block, so later specs don’t inherit the altered index state.
🪄 Autofix (Beta)
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
Run ID: e8093915-9b3a-4e17-b136-81c2c659b460
📒 Files selected for processing (2)
spec/MongoStorageAdapter.spec.jssrc/Adapters/Storage/Mongo/MongoStorageAdapter.js
| .catch(error => { | ||
| // MongoDB 8.0 enriches stored index specs with internal fields (e.g. `enableOrderedIndex`) | ||
| // that Parse Server never sets. Re-issuing an otherwise-identical createIndex on startup | ||
| // then fails with IndexKeySpecsConflict (86) / IndexOptionsConflict (85) even though a | ||
| // functionally-equivalent index already exists — which aborts server startup. Treat that | ||
| // as success (the existing index already serves its purpose), mirroring | ||
| // ensureAuthDataUniqueness. Logged so a genuine conflict is still visible. (#10431) | ||
| if (error.code === 85 || error.code === 86) { | ||
| logger.warn( | ||
| `Index "${indexName || ''}" on ${className} already exists with a conflicting ` + | ||
| `specification; keeping the existing index. (code ${error.code})` | ||
| ); | ||
| return; | ||
| } | ||
| return this.handleError(error); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all call sites of ensureIndex to assess blast radius of blanket-catching 85/86
rg -nP '\bensureIndex\s*\(' --type=js -C2Repository: parse-community/parse-server
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section and nearby helpers.
sed -n '760,880p' src/Adapters/Storage/Mongo/MongoStorageAdapter.js
printf '\n--- SEARCH: codes and related helpers ---\n'
rg -n "IndexOptionsConflict|IndexKeySpecsConflict|ensureAuthDataUniqueness|createIndex|dropIndex|partialFilterExpression|collation|unique" src/Adapters/Storage/Mongo/MongoStorageAdapter.jsRepository: parse-community/parse-server
Length of output: 7474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- ensureIndex call sites ---\n'
rg -n "\bensureIndex\s*\(" src
printf '\n--- related index management around createIndexesIfNeeded / createIndexes / dropIndex ---\n'
sed -n '1180,1295p' src/Adapters/Storage/Mongo/MongoStorageAdapter.js
printf '\n--- any other 85/86 handling ---\n'
rg -n "85|86|IndexOptionsConflict|IndexKeySpecsConflict" srcRepository: parse-community/parse-server
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- ensureIndex call sites ---'
rg -n "\bensureIndex\s*\(" src
echo
echo '--- index lifecycle code around createIndexesIfNeeded/createIndexes/dropIndex ---'
sed -n '1180,1295p' src/Adapters/Storage/Mongo/MongoStorageAdapter.js
echo
echo '--- tests and comments mentioning 85/86 or MongoDB 8.0 index conflicts ---'
rg -n "IndexOptionsConflict|IndexKeySpecsConflict|enableOrderedIndex|`#10431`|ensureAuthDataUniqueness|createIndex\(" test srcRepository: parse-community/parse-server
Length of output: 6507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- DatabaseController ensureIndex call sites and options ---'
sed -n '1970,2075p' src/Controllers/DatabaseController.js
echo
echo '--- MongoCollection helpers around createIndex / ensure sparse unique index ---'
sed -n '1,240p' src/Adapters/Storage/Mongo/MongoCollection.jsRepository: parse-community/parse-server
Length of output: 9541
Only ignore 85/86 for equivalent indexes This treats every ensureIndex conflict as success, so a real index-definition change can leave a stale index in place on startup. Compare the existing spec first, or drop/recreate when it differs.
🧰 Tools
🪛 Biome (2.5.1)
[error] 817-836: Illegal return statement outside of a function
(parse)
🤖 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/Adapters/Storage/Mongo/MongoStorageAdapter.js` around lines 821 - 836,
The current `ensureIndex` catch block in `MongoStorageAdapter` treats every
85/86 conflict as success, which can mask real index-definition changes. Update
the logic in this `catch` path to first compare the existing index spec against
the requested one (using the surrounding `indexName`/`className` index creation
flow), and only ignore the error when they are functionally equivalent. If the
specs differ, either drop and recreate the index or surface the error through
`handleError` instead of returning success.
1 participant
Closes #10431
Summary by CodeRabbit