Fix init-rocksdb skipping updates for pinned prerelease/revision versions - #734
Open
cb1kenobi wants to merge 3 commits into
Open
Fix init-rocksdb skipping updates for pinned prerelease/revision versions#734cb1kenobi wants to merge 3 commits into
cb1kenobi wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the RocksDB version checking logic by extracting it into a dedicated helper module (version-check.ts) and adding comprehensive unit tests. The feedback highlights potential runtime crashes when passing unvalidated version strings to semver.eq and semver.lte, recommending validation using semver.valid and suggesting corresponding test cases to ensure robust error handling.
Contributor
📊 Benchmark Resultsget-sync.bench.tsgetSync() > random keys - small key size (100 records)
getSync() > sequential keys - small key size (100 records)
ranges.bench.tsgetRange() > small range (100 records, 50 range)
realistic-load.bench.tsRealistic write load with workers > write variable records with transaction log
transaction-log.bench.tsTransaction log > read 100 iterators while write log with 100 byte records
Transaction log > read one entry from random position from log with 1000 100 byte records
worker-put-sync.bench.tsputSync() > random keys - small key size (100 records, 10 workers)
worker-transaction-log.bench.tsTransaction log with workers > write log with 100 byte records
Results from commit a6276f3 |
cb1kenobi
marked this pull request as ready for review
July 29, 2026 16:38
…ions Pinning a downstream revision such as `11.1.2-1` in package.json while `11.1.2` was installed left the old version in place. semver sorts a prerelease/revision suffix *below* the bare release (`11.1.2-1 < 11.1.2`), so the "don't downgrade" guard that protects the `latest` path refused to install the explicitly-pinned version, thinking it was a downgrade. Split the decision into two intent-named predicates (new version-check.ts): - `installedSatisfiesPin` — an exact pin is already installed (full version match incl. suffix, compatible runtime). - `prebuildIsRedundant` — the downgrade guard, now applied ONLY for `latest` (or unset). An explicit pin always installs exactly what was requested. This also removes a latent crash: the old first check ran `semver.eq(current, 'latest')`, which throws on a non-version string when `ROCKSDB_VERSION=latest` and a prebuild is already installed. Extracted as pure functions so the version-selection logic is unit tested without network or process.exit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Guard installedSatisfiesPin / prebuildIsRedundant with semver.valid before comparing: semver.eq/lte throw TypeError on a non-semver string (a corrupted rocksdb.json or a garbage ROCKSDB_VERSION). Invalid input now returns false — falling through to a (re)download or a clear "prebuild not found" error rather than crashing. Adds tests covering invalid-input handling for both. Addresses gemini-code-assist review feedback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The RocksDB prebuild may carry a downstream revision suffix (e.g. 11.1.2-1) that package.json pins but RocksDB's own version.h cannot encode (GetRocksVersionAsString reports 11.1.2). Compare RocksDBVersion.MatchesPackagePin against the base MAJOR.MINOR.PATCH, stripping any -N suffix, so a revision pin does not spuriously fail the native version check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cb1kenobi
force-pushed
the
fix/prerelease-version-check
branch
from
July 29, 2026 18:30
2f80721 to
c886f2b
Compare
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.
Summary
Pinning a downstream revision such as
11.1.2-1inpackage.json(rocksdb.version) while11.1.2was already installed left the old version in place instead of downloading the pinned one.Root cause: semver sorts a prerelease/revision suffix below the bare release —
11.1.2-1 < 11.1.2. The second check inscripts/init-rocksdb/main.tsis a "don't downgrade" guard (soROCKSDB_VERSION=latestwon't clobber a newer manually-pinned build). With a pinned11.1.2-1, that guard sawsemver.lte('11.1.2-1', '11.1.2') === trueand skipped the download, treating the requested version as a downgrade. Note the first check (semver.eq) was already correct — it's not where the skip happened.What changed
Split the version decision into two intent-named pure predicates in a new
scripts/init-rocksdb/version-check.ts:installedSatisfiesPin— the exact pin is already installed (full version match including the suffix, compatible runtime).prebuildIsRedundant— the downgrade guard, now applied only when resolvinglatest(or an unset version). An explicit pin always installs exactly what was requested.main.tsnow uses these instead of inlinesemvercalls.Where to look
scripts/init-rocksdb/version-check.ts— the new logic and the reasoning (see the doc comments on why a pin bypasses the downgrade guard).11.1.2→11.1.2-1now downloads. Thelatestpath is unchanged.Also fixed (latent)
The old first check ran
semver.eq(currentVersion.version, 'latest'), which throwsInvalid VersionwhenROCKSDB_VERSION=latestand a prebuild is already installed. Gating it on "is an exact pin" avoids that.Tests
test/init-rocksdb-version-check.test.ts— 12 cases covering pin detection, exact-match/mismatch (incl. the11.1.2vs11.1.2-1regression), runtime matching, thelatestdowngrade guard, and an end-to-end assertion that pinning11.1.2-1over an installed11.1.2proceeds to download. Extracted as pure functions specifically so this is testable without network orprocess.exit.Notes for the reviewer
getPrebuildsorts release tags withsemver.rcompare, so if bothv11.1.2andv11.1.2-1exist, thelatestdefault still picks11.1.2over the "prerelease"-1; (2) theversion.hfallback inget-current-version.tscan't represent a-Nsuffix (only MAJOR.MINOR.PATCH), so an old prebuild withoutrocksdb.jsonreports the bare version.latest-vs-pin semantics.🤖 Generated with Claude Code (model: Claude Opus 4.8)