Repository navigation
Conversation
…tatus() BREAKING CHANGE: the `registryStatus` export is gone; use `getRegistryStatus()`. #840 added `getRegistryStatus()` as the canonical name and kept `registryStatus` as a deprecated alias tagged for removal in v3.0.0. This performs that removal. The private native export is renamed to match. It is reachable in practice -- each platform package publishes its `.node` as `main`/`exports` -- so it is a second, undocumented breaking surface rather than a purely internal rename. `scripts/test-binding.mjs` now calls `getRegistryStatus()` and asserts the alias is absent, per bundle format, so a platform binary that did not pick up the rename fails the release build instead of shipping. It forks for the CJS check because the two bundles patch the same native prototypes and cannot both load in one process (pre-existing, unrelated to this change). Downstream: harper and harper-pro still import `registryStatus` in production code and must migrate before taking 3.0. Their `typeof` feature-detect does not protect them -- a missing named ESM export fails at module load. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cross-model review nit: the comment claimed both formats run in a child. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pre-push self-check: spawnSync had no timeout, so a wedged child would hang the release job indefinitely. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📊 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 00d5f28 |
kriszyp
left a comment
There was a problem hiding this comment.
Sounds good. Will you file the harper and harper-pro naming update PRs?
🤖 Reviewed with Codex
| * in v3.0.0. | ||
| */ | ||
| export const registryStatus: () => RegistryStatus = getRegistryStatus; | ||
| export const getRegistryStatus: () => RegistryStatus = binding.getRegistryStatus; |
There was a problem hiding this comment.
Could we retain the deprecated registryStatus re-export until v3? Its previous JSDoc promised removal in v3.0.0, but package.json:3 still declares 2.10.0. A v2 consumer importing { registryStatus } now fails ESM module linking or TypeScript compilation; a CommonJS caller receives undefined. The existing getRegistryStatus export already supports migration. Please keep the alias for v2, or land its removal with the v3 package and matching native-package version change.
There was a problem hiding this comment.
Agreed, and thank you — this is the right thing to hold the line on. One correction that makes the case stronger than stated: getRegistryStatus has never been published, so it does not yet support migration.
I unpacked the tarball to check: @harperfast/rocksdb-js@2.10.0 exports registryStatus only — zero occurrences of getRegistryStatus. #887 added it, but merged after the 2.10.0 release, so it exists in main and in no released artifact.
That makes "keep the alias for v2" not just the safer option but a prerequisite: until a 2.x release carries both names, the harper / harper-pro migration PRs have no installable rocksdb-js version to compile against, so they cannot merge ahead of this one.
Two orderings work, and the PR body carries this as a ❓ Your call: line for Chris:
- Cut a 2.x release from today's
main(it already has both names) → migrateharper/harper-pro→ merge this → tag 3.0.0. - Hold this branch unmerged until the 3.0 cut, and migrate downstream against the interim release from (1) anyway.
Either way this stays in Draft and must not land on a v2 tag.
On the native-package version: that part is already automatic. scripts/bump-versions.mjs rewrites every optionalDependencies entry prefixed with the package name to the exact new version, and .github/workflows/publish.yml derives that version from the release tag — so the renamed native export ships pinned to the same tag as the JS bundle, with no separate version change needed. Worth calling out in the 3.0 release notes regardless, since each platform package exposes its .node as main/exports and the native registryStatus property is renamed too.
🤖 Generated by Anthropic Claude (Opus 5); posted via @cb1kenobi.
The migration is drafted and ready to go — Chris's call on whether I open it as one issue spanning both repos or a PR per repo. For scope, the surface is wider than issue #840 recorded (it listed only
One detail worth knowing before that work starts: The change itself is mechanical ( 🤖 Generated by Anthropic Claude (Opus 5); posted via @cb1kenobi. |
…ndence (#915) * ci: test on push to main + nightly; fix registryStatus() order-dependence pr.yml's Test workflow ran on pull_request only, so a merge to main had no test signal (stress.yml already runs on push). Add push: branches: [main] and a nightly schedule. Make the Deno-test retry OS-consistent: it retried failures only on Windows, unlike bun-test's documented Windows-only retry (a specific, cited race). No equivalent Deno-specific root cause is documented, so retry once on every OS instead. test/shutdown.test.ts asserted on registryStatus()'s global array length, which is order-dependent — the native registry is process-global and vitest runs with fileParallelism: false, so another test file's open database shows up in the same array. Filter by the test's own db.path, matching the pattern already used in registry-status.test.ts, destroy.test.ts, secondary.test.ts, etc. Document why vitest.config.ts picks the forks pool for Bun/Deno and what it costs (process-spawn overhead, no --expose-gc propagation). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FF7XCwDWipqGApc3eX32wb Dispatch-Task: rocksdb-js-ci-on-main-and-test-hygiene * Address pre-push review: concurrency, retry visibility, deprecated alias - cancel-in-progress now only applies to pull_request runs. push and schedule share the refs/heads/main group, so leaving it unconditional meant two quick merges (or a push racing the nightly run) cancelled one of them -- silently dropping the per-commit signal this change exists to add. - The Deno retry now logs a ::warning:: on the first failed attempt before retrying, so a rising flake rate is visible instead of being silently absorbed by the retry on every OS. - shutdown.test.ts now calls getRegistryStatus() instead of the deprecated registryStatus() alias (src/load-binding.ts marks it for removal in v3.0.0, and open PR #900 removes it outright). - Fixed the new vitest.config.ts comment: fileParallelism: false forces maxWorkers to 1, so forks here is one process for the whole run, not one per test file (matches bun-test's existing comment on the same behavior). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FF7XCwDWipqGApc3eX32wb Dispatch-Task: rocksdb-js-ci-on-main-and-test-hygiene * Fix concurrency group dropping queued main runs; revert Deno retry scope Review round 2 found two real problems with the round-1 fixes: - cancel-in-progress only stops a RUNNING run from being cancelled. GitHub always drops an older QUEUED run when a newer one joins the same concurrency group, regardless of that setting -- so two merges (or a push racing the nightly run) still silently dropped one run's signal. push/schedule now get a group keyed on github.run_id instead of the shared main ref, so they never collide with each other or with a PR's group. - The unconditional Deno retry-with-warning (added in the prior commit) doubles this job's failure tolerance on Linux/macOS from p to roughly p^2 with no evidence a comparable flake exists there, and the warning sits on a run that passes and that nobody opens -- it doesn't actually fix the visibility problem it was meant to fix. Reverted to the original Windows-only retry, now with a comment explaining why: it's covering general Windows-runner flakiness (matching bun-test and the Node coverage job above), not a Deno-specific race the way bun-test's #695 is, so there's no basis for extending it to Linux/macOS. Also fixed the vitest.config.ts comment from the prior commit: it's wrong that fileParallelism collapses forks into one process for the whole run. Verified with a throwaway two-file process.pid check that each file still gets its own forked process; fileParallelism only serializes them to one at a time. bun-test's existing comment about a single Bun process is a separate, Bun-specific claim this doesn't contradict or confirm. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FF7XCwDWipqGApc3eX32wb Dispatch-Task: rocksdb-js-ci-on-main-and-test-hygiene * Trim comment noise from the closing review round Shortened the nightly, concurrency, and Deno-retry comments to state the decision instead of narrating it or arguing with a reviewer, and dropped vitest.config.ts's "verified via process.pid" aside and its claim about a Bun-specific exception (the other file's single-process comment is about Bun's own run, not something this diff can confirm or contradict). Also ran the full suite once with shutdown.test.ts's original global-length assertions restored (not committed) to check the decision ledger's open question about a possible existing registry leak: 1180 passed / 10 skipped / 0 failed, so there's no current leak for the path-scoped assertions to hide. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FF7XCwDWipqGApc3eX32wb Dispatch-Task: rocksdb-js-ci-on-main-and-test-hygiene * Fix bun-test.yml's comment: Bun is not single-process either Review flagged a real contradiction: vitest.config.ts's corrected comment says forks gives each test file its own child process, while bun-test's pre-existing comment claimed Bun runs every file's DB churn in one process. Installed Bun locally and ran the same two-file process.pid check against it (bun --bun ... vitest run): PIDs differed across files, same as under Node and Deno -- Bun is not single-process either, so the pre-existing comment's premise was wrong. The retry itself is unaffected (#695 is real and still the reason for the extra Windows attempts); only the inaccurate "single-process, every file's DB churn in one process" explanation is corrected to point at worker_threads churn within a run instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FF7XCwDWipqGApc3eX32wb Dispatch-Task: rocksdb-js-ci-on-main-and-test-hygiene --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
DO NOT MERGE UNTIL V3
⊙ Problem
@harperfast/rocksdb-jsexports the same registry-inspection function under two names. #840 establishedgetRegistryStatus()as the canonical one — matching the sibling sampling gettergetWriteBufferManagerStats()— and #887 added it while keepingregistryStatusas a deprecated alias tagged "This alias will be removed in v3.0.0." This PR performs that removal, so 3.0 ships one name per capability.💡 Solution
Delete the alias, point the canonical export straight at the native function, and migrate every in-repo caller. The function object, signature, and return shape are unchanged — 3.0 only drops the second name, so each migrated call site is textually identical in effect.
This is breaking on two surfaces, not one. The obvious one is the public TypeScript export. The second is the native export
binding.registryStatus, renamed here to match: each platform package publishes its.nodeasmain/exports, sorequire('@harperfast/rocksdb-js-darwin-arm64').registryStatusresolved for anyone who reached for it. Undocumented, but reachable, and the 3.0 release notes should name both.Downstream migration is required before Harper can take 3.0.
harperandharper-proeach importregistryStatusin four production modules (resources/databases.ts,resources/branchDatabase.ts,resources/longLivedTransactions.ts,dataLayer/rocksdbBackup.ts). Thetypeof registryStatus === 'function'guard inlongLivedTransactions.tsdoes not soften this: a named ESM import the module no longer exports fails at module load, so the guard never runs. SincegetRegistryStatushas existed in-tree since #887, that migration can land and ship independently of the 3.0 bump.⚖️ Alternatives
@deprecated ... will be removed in v3.0.0unperformed, so 3.0 ships a deprecation whose stated deadline has passed. This is the option the request rules out.load-binding.ts; the TS export is the semver-governed one the deprecation tag actually named.TransactionLog,NativeDatabase,NativeTransactionare all identity re-exports ofbinding.*), and a rule with an allowlist is a second list to keep in step. The targeted absence assertion intest/registry-status.test.tsgets the regression check without the false positives, which is what shipped.binding.getRegistryStatus ?? binding.registryStatus— rejected: exactoptionalDependenciespins keep JS and native in step through a normal install, so the skew it tolerates is not reachable, and it would mask a genuinely mispackaged binary.🔧 Changes
The removal —
src/load-binding.tsdrops the alias and bindsgetRegistryStatusto the renamed native export;src/index.tsdrops it from the public export list;src/binding/database/db_registry.cpprenames both the N-API function name and the exported property.Call-site migration — purely mechanical, one substitution per file. Test-seam names (
ROCKSDB_JS_REGISTRY_STATUS_COLUMNS_DELAY_MS,registryStatusColumnsDelayMsFlag) and fixture filenames are deliberately untouched — they name a seam, not the API.backup-stream.test.ts,backup.test.ts,checkpoint.test.ts,destroy.test.ts,secondary.test.ts,shutdown.test.ts,transaction-log.test.ts,transaction-orphan-gc.test.ts,registry-status-worker.mtsfork-close-failure.mts,fork-commit-admission-shutdown.mts,fork-compact-cancel-async.mts,fork-compact-cancel-destroy.mts,fork-compact-cancel-sync.mts,fork-count-destroy-race.mts,fork-destroy-open.mts,fork-flush-failure.mts,fork-gc-close-failure.mts,fork-iterator-next-race.mts,fork-legacy-commit-close.mts,fork-lifecycle-timeout-two-descriptors.mts,fork-lifecycle-timeout.mts,fork-open-attach-destroy.mts,fork-quarantined-exit.mts,fork-registry-status-close-race.mts,fork-registry-status-column-race.mts,fork-shutdown-failure.mts,fork-shutdown-retry.mts,transaction-orphan-dependents.mtspnpm test, so type-check is what catches it):db-instances.stress.test.tsTests —
test/registry-status.test.tsreplaces the alias-identity assertion with an absence assertion ('registryStatus' in rocksdbisfalse) and keeps the open-database behavior check.Release gate —
scripts/test-binding.mjsran per-platform in the publish matrix but only logged the version. It now callsgetRegistryStatus()and asserts the alias is gone, in both ESM and CJS, so a platform binary that did not pick up the native rename fails the release build instead of shipping. It checks CJS in a bounded child process because the two bundles patch the same process-global native prototypes and cannot both load in one process — pre-existing onmain, not introduced here.Prose — updated only where the old name now names nothing:
AGENTS.md(3 references), and comments indb_registry.h,transaction_handle.h,backup.cpp,test_seam.h.README.mdneeded no change: #887 already documentedgetRegistryStatus()only.Not changed:
package.jsonstays at 2.10.0..github/workflows/publish.ymlderives the version from the release tag and runsscripts/bump-versions.mjsin CI, so 3.0.0 is set at release time, not here.✅ Verification
End-to-end route: the extended release-matrix smoke check, which exercises the shipped artifact rather than source.
node scripts/test-binding.mjsagainst a freshpnpm build—esm: getRegistryStatus() ok, registryStatus alias absent/cjs: .... Verified it actually fails closed: run against a staledist, it reportedesm: getRegistryStatus is not a functionand exited non-zero.pnpm test— 74 files, 1103 passed / 10 skipped / 0 failed.pnpm test:native— 302 tests, 299 passed / 3 skipped / 0 failed.pnpm test:stress(excluded frompnpm test, and contains a migrated file) — 5 files / 9 tests passed. One earlier run showed an unrelated flake intransaction-log-mmap.stresswhile the machine was also running the native build and three review legs (310s vs 10s wall time); it passes in isolation and on a quiet machine, and touches nothing in this diff.pnpm check(tsc + oxlint + oxfmt) clean.tsccoversstress-test, so a missed migration there would have failed type-check.registryStatusonly, which is what the release-sequencing question above rests on.CI on the merge commit (
29bc159f, aftermainwas merged in for #896) is green across the full matrix: Node 22/24/26 × Linux/macOS/Windows, Bun and Deno × 3, native C++ tests × 3, stress and benchmarks.One flake, not caused by this change:
secondary.test.ts > should survive a live writer as a secondary where readOnly racesfailed once on Node 26 / Windows withERR_CONCURRENT_COMPACTION(Access is deniedon a.blob), and passed on a plain re-run with no code change. This PR's only edits to that file are three identifier substitutions on lines 1, 440 and 470; the failure is at line 724, inside a secondary open/read/close cycle racing a live writer worker. On Windows that classifier maps a sharing violation onto the same code as the POSIX missing-file race (#812 covers the read-only side of that race) — worth its own look, but out of scope here.Not verified here: installation through a published optional platform package. That is a release-time check, and the smoke gate above is what covers it in the publish matrix.
🤖 Generated by Anthropic Claude (Opus 5); posted via @cb1kenobi.
Related PRs: #887 overlaps (added the alias this removes; merged), #545 independent, #742 independent, #767 independent, #842 independent, #890 independent, #897 independent, #382 independent, #480 independent
Complexity: easy
Review-Coverage: authored=claude; ran=gemini,codex,cursor-grok; adjudicated=domain; declined=cursor-composer,cursor-kimi,cursor-muse; rounds=1; full=1 @ 29bc159
Review-Attention: study ~9m (critical: transaction_handle.h; decisions: native-export-rename, removal-timing, smoke-check-scope) @ 29bc159