Repository navigation
ci: test on push to main + nightly; fix registry assertion order-dependence - #915
Merged
Merged
Conversation
…ence 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
- 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
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
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
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
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the shutdown tests in test/shutdown.test.ts to use getRegistryStatus instead of registryStatus, querying specific database paths to make assertions more robust. It also adds explanatory comments in vitest.config.ts regarding the use of the 'forks' pool in alternate runtimes. There are no review comments, so I have no feedback to provide.
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 2ef5967 |
cb1kenobi
approved these changes
Oct 9, 2026
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
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.
⊙ Problem
.github/workflows/pr.yml'sTestworkflow only triggers onpull_request, so a merge tomaingets no test run (stress.ymlalready runs onpush). Separately,test/shutdown.test.tsasserted onregistryStatus()'s global array length — order-dependent, since the native registry is process-global and the whole suite runs withfileParallelism: false(one shared process). The Deno-test retry (~line 237) retried only on Windows, with no stated reason, unlikebun-test's own Windows-only retry which cites a specific tracked race.vitest.config.ts'sforks-pool choice for Bun/Deno had no comment explaining the tradeoff.💡 Solution
push: branches: [main]and a nightlyscheduletopr.yml.cancel-in-progress: trueon the shared${{ github.ref }}group meant two merges (or a push racing the nightly) silently dropped one run — GitHub always replaces an older queued run in a group when a newer one joins, independent ofcancel-in-progress(that setting only governs a running run). PR runs keep one group per ref (a new commit should supersede the old run);push/scheduleruns now get a group unique togithub.run_idso they never collide.d5329d8dcommit ("Rerun tests if they fail on Windows") — it applied the same blanket Windows-only retry to the Node-coverage, Bun, and Deno jobs with zero stated rationale; onlybun-testlater got a specific, cited reason (Env-cleanup hooks can miss a descriptor mid-close/mid-reopen (registry reachability + dangling entry reference) #695). There's no equivalent Deno-specific flake on record. Kept it Windows-only, now with a one-line comment saying so, rather than silently doubling this job's failure tolerance on Linux/macOS to "fix" an asymmetry that was never justified either way.test/shutdown.test.ts: both tests now look up the test's owndb.pathingetRegistryStatus()(.find()/.some()) instead of asserting the whole array's length — the same pattern already used inregistry-status.test.ts,destroy.test.ts,secondary.test.ts, etc. Also switched off the deprecatedregistryStatus()alias (src/load-binding.ts, removed in open PR Remove the deprecated registryStatus() alias in favor of getRegistryStatus() #900) togetRegistryStatus().vitest.config.ts: documented why Bun/Deno useforks(sidestepsthreads-poolworker_threadsflakiness) and the cost (--expose-gcdoesn't propagate — Deno sees noglobalThis.gc, perAGENTS.md).🔧 Changes
.github/workflows/pr.yml—push/scheduletriggers, run-id-scoped concurrency group, Windows-only Deno retry (documented), and a correctedbun-testcomment (see below).test/shutdown.test.ts— path-scopedgetRegistryStatus()assertions.vitest.config.ts—forks-pool rationale comment.Unplanned, in-scope fix: fixing the
vitest.config.tscomment surfaced a real contradiction withbun-test's pre-existing comment, which claimed Bun runs every test file's DB churn in one process. Installed Bun locally and ran the same two-fileprocess.pidcheck against it — PIDs differed across files, same as under Node and Deno: Bun is not single-process either. The pre-existing comment's premise was wrong; corrected it to point atworker_threadschurn within a run (the actual mechanism behind #695) instead. The retry behavior itself is unchanged.⚖️ Alternatives
::warning::on an otherwise-green run isn't seen by anyone. Reverted to Windows-only.cancel-in-progress: ${{ github.event_name == 'pull_request' }}alone, same shared group (the second attempt) — looked sufficient but only stops cancelling a running run; GitHub still drops an older queued run in the same group regardless of that setting. Needed therun_id-based group split too.✅ Verification
pnpm check(type-check + lint + fmt) clean.pnpm test— 1180 passed, 10 skipped, 0 failed (full suite, includingtest/shutdown.test.tsandtest/registry-status.test.ts).forks-pool comment: ran a throwaway two-fileprocess.pidcheck under plain Node (forced--pool=forks) and under Bun (bun --bun ...) — both spawn a fresh process per test file, serialized byfileParallelism: falseto one at a time.shutdown.test.ts's original global-length assertions restored (uncommitted, local-only) — still 1180/10/0. No current leak..github/workflows/pr.ymlparses as valid YAML (yaml.safe_load).No numbered GitHub issue was filed for this; it was worked from a dispatch task (
rocksdb-js-ci-on-main-and-test-hygiene).Related PRs: #900 overlaps, #742 overlaps, 5 others independent
Origin — the dispatch brief this PR was written from
Run the Test workflow on main; registry-assertion and retry hygiene
Dispatch: task
rocksdb-js-ci-on-main-and-test-hygiene· queued by unknown · ran by claude/sonnet/xhigh · worker kzyp-xps-1Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=5; full=2 @ 8574288
Review-Attention: study ~10m (sensitive: pr.yml; decisions: main-push-full-matrix, nightly-failure-routing, path-scoped-shutdown-assertion) @ 8574288