Lower the Node floor to 24.4.0 - #11
Merged
Merged
Conversation
The package required Node 24.15.0 with no recorded reason. Measuring the runtime shows the real constraint is one SQLite option. The SQLite adapter builds DatabaseSync with readBigInts, which it needs to read 64-bit integers without losing precision. Node 24.0.0 through 24.3.x accept the option and ignore it, returning Number instead of BigInt. That is silent, so the suite is what catches it: the stale-process effect recovery test and the SQLite transaction retry test both fail on 24.3.0 and pass on 24.4.0. Node 24.15.0 is where node:sqlite stops being experimental and stops printing a warning on stderr. That is a reason to prefer it, not a reason to require it, so the docs now say both. A floor job runs the default suite, the build, and the recovery demo on 24.4.0, and the publish job waits for it, so the declared floor stays measured rather than assumed.
Greptile SummaryThe PR lowers the supported Node.js version to 24.4.0 and documents why 24.15.0 remains preferable.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "ci: smoke the packaged artifact on the f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+60
to
+62
| Node.js 24.4.0 or newer is required. Node.js 24.15 or newer is preferred, | ||
| because `node:sqlite` prints an experimental warning before it. The `0.13.1` | ||
| release includes a packaged quickstart: |
There was a problem hiding this comment.
When a user on Node 24.4.0–24.14.x runs the documented quickstart with npm engine-strict enabled, the command installs solid-objects@0.13.1, whose engine constraint remains >=24.15.0, causing npm to reject the installation despite the new stated minimum.
Prompt To Fix With AI
This is a comment left during a code review.
Path: README.md
Line: 60-62
Comment:
**Pinned quickstart requires newer Node**
When a user on Node 24.4.0–24.14.x runs the documented quickstart with npm engine-strict enabled, the command installs `solid-objects@0.13.1`, whose engine constraint remains `>=24.15.0`, causing npm to reject the installation despite the new stated minimum.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+38
to
+41
| - run: pnpm install --frozen-lockfile | ||
| - run: pnpm run test | ||
| - run: pnpm run build | ||
| - run: pnpm run test:recovery |
There was a problem hiding this comment.
The floor job does not run test:package, so it does not continuously validate clean installation and execution of the generated package at Node 24.4.0; a floor-specific packaging or executable incompatibility can therefore pass the publish gate.
Suggested change
| - run: pnpm install --frozen-lockfile | |
| - run: pnpm run test | |
| - run: pnpm run build | |
| - run: pnpm run test:recovery | |
| - run: pnpm install --frozen-lockfile | |
| - run: pnpm run test | |
| - run: pnpm run build | |
| - run: pnpm run test:package | |
| - run: pnpm run test:recovery |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/ci.yml
Line: 38-41
Comment:
**Floor skips packaged artifact smoke**
The floor job does not run `test:package`, so it does not continuously validate clean installation and execution of the generated package at Node 24.4.0; a floor-specific packaging or executable incompatibility can therefore pass the publish gate.
```suggestion
- run: pnpm install --frozen-lockfile
- run: pnpm run test
- run: pnpm run build
- run: pnpm run test:package
- run: pnpm run test:recovery
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Bumps package.json, src/version.ts, the process metadata test literal, the README quickstart references, and dates the changelog section for the Node floor change.
Greptile noted the floor job proved the source tree but never the artifact a consumer installs. A packaging or executable problem specific to the floor could pass the publish gate. The job now runs test:package too.
Owner
Author
|
@greptileai review The two issues from the last review are addressed:
|
main released 0.13.2 while this branch was open, and this branch had claimed that number for the Node floor change. Both the changelog and the README quickstart conflicted on it. The released 0.13.2 section stays as main wrote it, and the floor entries move to a new 0.13.3 section. 0.13.3 also matches the Ruby gem, whose release carries the same date. The rest of the version references follow the release procedure: package.json, src/version.ts, the process metadata test literal, the README quickstart, docs/parity.md, and the example tag in docs/releasing.md.
The failure-recovery demo asserted the serialization control file held exactly four events. A worker that loses its lease mid-operation leaves the replacement to execute the same message again, so a slower machine produces six, and the demo failed for the at-least-once behaviour the library documents and the crash and fencing proofs rely on. It failed five times on GitHub runners, on Node 24.15.0 as well as the lower floor, and passed on rerun, so the assertion was reporting the runner's speed rather than a durability property. The proof now asserts what the runtime guarantees: executions never overlap, every start has a matching finish, and exactly the two sent messages ran. The committed state check is unchanged, and it is the one that would catch a lost or doubled write. assertSerializedExecution moves into its own module so it can be tested at the lowest layer. The unit tests cover the retry, overlap, unfinished, and lost-message cases, and each rejection case fails when the rule is removed. The lease timing stays as it was. proveCrashRecovery and proveFencing depend on a short lease, so raising it to hide the retry would weaken them.
The first rule demanded strict alternation across the whole log, which rejects the case it was written to accept. The attempt that loses its lease is the one that was slow, so it is still running when the replacement starts, and both write to the control file. Overlap is now allowed only when a message ran more than once, which is the only thing that produces a stale owner. Without a retry the proof still demands strict serialization. The rule tracks open attempts per message, so a message may be open twice. The demo reports executions, retried, and overlapped instead of a hardcoded overlap: false, so a run says what it actually saw. Found by Greptile on the first commit. The stale-overlap case is now a test, and it fails against the alternation rule.
Treating any retry as a licence to overlap let a real violation through: once one message retried, two different messages could run at the same time and the proof still passed. A retry excuses only the attempt it superseded. The rule now tracks which attempt of each message is the last one started, and asserts that at most one message has its surviving attempt open at any moment. Earlier attempts may overlap anything, because their writes are fenced. Found by Greptile on the previous commit. The case is now a test, and removing the assertion fails it along with the no-retry overlap case.
The floor job runs the recovery demo, and the demo's serialization proof asserted an execution count that a slow runner turns into a retry. It failed three times on this branch for behaviour the library documents. Pulls in the fix so the floor job proves the floor rather than the runner's speed. The changelog entry joins 0.13.3, which is the release this branch dates.
Two attempts of the same message can both be writing to the control file at once: the superseded attempt keeps running until it notices the lost lease, and its finish can land after the replacement's start. The previous rule paired a start with whichever finish came next, so that late finish read as the replacement's, and a second message could then overlap a replacement that was in fact still running. Each event now carries the message's attempt and the writing process, so a start pairs only with the finish of that same execution. The proof still asserts one thing about the surviving attempts, that none of them overlap another message's surviving attempt, and lets a superseded attempt overlap anything, because its write is fenced out and the committed state is the assertion that proves it. Found by Greptile on the previous commit, and reproduced as a test: a superseded attempt that finishes after its replacement starts, followed by a second message that starts while the replacement is still running. Removing the overlap assertion fails that case and the no-retry case.
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
1 participant
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.
engines.nodewas>=24.15.0from the first release commit, with no recordedreason. This measures the real floor and moves it to
24.4.0.What sets the floor
The SQLite adapter constructs
DatabaseSyncwithreadBigInts: true, which itneeds to read 64-bit integers without losing precision.
readBigIntson the constructornode:sqliteNumberBigIntBigIntProbe:
The option is accepted and ignored below 24.4.0, so the failure is silent. The
existing suite catches it. On Node 24.3.0:
The same suite on Node 24.4.0:
The experimental warning
Node.js 24.15.0 is the first release where
node:sqliteis no longerexperimental. Between 24.4.0 and 24.14.x the module works but prints
ExperimentalWarning: SQLite is an experimental featureon stderr, and its APIcan change. The README,
CONTRIBUTING.md, anddocs/support.mdstate thefloor and the reason to prefer 24.15.0. Nothing suppresses the warning.
Change
engines.nodeis>=24.4.0.floorCI job runs the default suite, the build, and the recovery demo on24.4.0. The publish job waits for it, so the declared floor stays measured
rather than assumed. The other jobs stay on 24.15.0.
CONTRIBUTING.md,docs/support.md, andCHANGELOG.mdrecord thefloor, the
readBigIntsreason, and the warning boundary.No source file changes.
Validation
On Node 24.4.0:
format:check,check,test,build,pack:check,test:package, andtest:recoveryall pass. On Node 24.18.0:format:check,check,test, andbuildall pass.Going below 24.4.0 is possible by calling
statement.setReadBigInts(true)perprepared statement, which exists on 24.0.0. That was left out on purpose,
because it changes the durable storage adapter for four releases of extra
reach.