fix(NODE-7575): reset ObjectId state when building startup snapshot#884
fix(NODE-7575): reset ObjectId state when building startup snapshot#884addaleax wants to merge 1 commit intomongodb:mainfrom
Conversation
| this.resetState(); | ||
| // https://nodejs.org/api/v8.html#startup-snapshot-api | ||
| // @ts-expect-error Node.js types not present since this is an optional API | ||
| const { startupSnapshot } = globalThis?.process?.getBuiltinModule('v8') ?? {}; |
There was a problem hiding this comment.
Accessing the API this way was recommended here: nodejs/node#63138 (comment)
I would agree that this seems like a reasonable compromise
|
|
||
| /** @internal */ | ||
| private static resetState = (): void => { | ||
| this.index = Math.floor(Math.random() * 0x1000000); |
There was a problem hiding this comment.
Drive-by fix: Math.random() never returns 1, and 0xffffff should be a valid counter value
There was a problem hiding this comment.
Pull request overview
This PR addresses an ObjectId startup-snapshot regression by ensuring per-process ObjectId generation state (counter + process-unique bytes) is reinitialized when a Node.js startup snapshot is deserialized, and adds a regression test that validates ObjectId behavior across multiple snapshot runs.
Changes:
- Add ObjectId static initialization logic that registers a V8 startup-snapshot deserialize callback to reset ObjectId per-process state.
- Move
PROCESS_UNIQUEintoObjectIdstatic state and reset it alongside the counter seed. - Add a Node.js startup snapshot regression test that builds and runs a snapshot twice and compares generated ObjectIds.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/objectid.ts |
Adds ObjectId state reset + V8 startup snapshot deserialize callback wiring. |
test/node/startup_snapshot.test.ts |
Adds regression coverage for ObjectId uniqueness across startup snapshot deserializations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Less than 20 seconds between timestamps should be plenty of leeway | ||
| expect(Math.abs(parseInt(oid2.ts, 16) - parseInt(oid1.ts, 16))).to.be.lessThan(20); | ||
| // Distinct process unique values | ||
| expect(oid1.uniq).to.not.equal(oid2.uniq); | ||
| // Distinct counter values | ||
| expect(oid1.counter).to.not.equal(oid2.counter); |
There was a problem hiding this comment.
Yeah, this is one in 16 million CI runs that fails, I can live with that
Use the relevant Node.js API to ensure that different processes indeed have different counter and process-unique values in their ObjectId classes, as intended, even when using Node.js's startup snapshot feature.
Description
Summary of Changes
Use the relevant Node.js API to ensure that different processes indeed have different counter and process-unique values in their ObjectId classes, as intended, even when using Node.js's startup snapshot feature.
Notes for Reviewers
What is the motivation for this change?
Release Highlight
Release notes highlight
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript