Repository navigation
Conversation
The runner now reads which docker-compose services a test file asks for from the source of that file (scripts/docker-services.ts). It uses the answer for the docker-last sort and the parallel bucket gate, and it passes the services of the shard's files to the coordinator, which starts them at launch. The hand-maintained prefix map (test/docker/prestart-map.mjs) is removed. scripts/update-parallel-allowlist.mjs uses the same rule. test/docker/service-names.mts holds the service names, and ServiceName is the type of that list.
|
Updated 10:54 AM PT - Oct 11th, 2026
✅ @robobun, your commit 052411ecdcc004fd2de07336341c6bdf0e3617d2 passed in 🧪 To try this PR locally: bunx bun-pr 44993That installs a local version of the PR into your bun-44993 --bun |
|
Status: ready for review. CI is green on 052411e (build 124593).
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 19 seconds. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| try { | ||
| source = readFileSync(`${testsDir}/${path}`, "utf8"); | ||
| } catch {} |
There was a problem hiding this comment.
🟡 (optional) A docker-backed test file that cannot be read for any reason other than not existing is silently treated as non-docker, so the generator can allowlist it and the runner puts it in the parallel batch. The empty catch {} at scripts/docker-services.ts:127 maps every readFileSync error (EACCES, EISDIR, EMFILE, EIO) to "asks for nothing". Fix: treat only ENOENT as the benign no-request case and rethrow (or at least log) every other error, for both callers (runner.node.ts needsDockerService and update-parallel-allowlist.mjs needsDockerService). On the base branch the allowlist generator read the file without a try and failed loudly on any unreadable file.
Why this was flagged
A test file listed by getTests (scripts/runner.node.ts:2501) or listBunTestFiles (scripts/update-parallel-allowlist.mjs:53) exists but readFileSync fails with an errno other than ENOENT, for example a permission problem in the checkout or an EMFILE/EIO on the agent. requestsOf at scripts/docker-services.ts:125-128 catches every error, returns none (backed: false) and caches that answer for the process. For a file that really calls describeWithContainer this makes needsDockerService false: scripts/runner.node.ts:1326 lets it into the bun test --parallel bucket and scripts/runner.node.ts:744 never passes its service to the coordinator, so the container starts inside the batch, the exact cost the PR removes. In scripts/update-parallel-allowlist.mjs:159 the same answer writes the file into test/parallel-allowlist.json as eligible, a persistent artifact. On the base branch the generator read the file with a bare readFileSync (base scripts/update-parallel-allowlist.mjs:162) and crashed on any read error, and the runner used a path prefix map that needed no read.
Verification: Triggering condition: readFileSync on a listed test file fails with an errno other than ENOENT in the runner or allowlist-generator process. At scripts/docker-services.ts:125-129 every error becomes none (backed: false, line 115) and is cached for the process. Regression vs base: the base generator's usesContainer had no catch, so a read error aborted the script loudly.
| describe.skipIf(!isLinux || !nodeExe())("scripts/runner.node.ts", () => { | ||
| /** Runs the runner over `testPaths` with stand-ins for bun, docker and who, and reports what it started. */ | ||
| async function runRunner(testPaths: string[]) { | ||
| using dir = tempDir("docker-services-runner", { | ||
| "docker": `#!/bin/sh\n[ "$*" = "compose version" ]\n`, | ||
| // A CI runner stays alive while somebody is logged in. | ||
| "who": `#!/bin/sh\n`, | ||
| // Stands in for the bun under test. It answers the runner's probe, records how the runner starts | ||
| // the coordinator and which test files it runs, and fails the ci-remap-server install so that | ||
| // the runner skips that server. | ||
| "bun": `#!/bin/sh | ||
| case "$1" in | ||
| --print) echo "$0" ;; | ||
| */coordinator.ts) printf '%s\\n' "$@" > "$0.coordinator" && echo COORDINATOR_READY && exec cat > /dev/null ;; | ||
| install) exit 1 ;; | ||
| test) for arg; do file=$arg; done; echo "$file" >> "$0.tests" ;; | ||
| esac | ||
| `, | ||
| }); | ||
| const bin = String(dir); | ||
| for (const name of ["docker", "who", "bun"]) chmodSync(join(bin, name), 0o755); | ||
| // The job this test runs in can be a Buildkite job. The runner under test is not one. | ||
| const env = Object.fromEntries( | ||
| Object.entries(bunEnv).filter(([name]) => !/^(BUILDKITE|GITHUB_|EXPECTED_PLATFORM_)/.test(name)), | ||
| ); | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| nodeExe()!, |
There was a problem hiding this comment.
🟡 (optional) Contributors on Linux whose node predates default type stripping get two failing tests from bun test test/internal/docker-services.test.ts instead of a skip. test/internal/docker-services.test.ts:322 gates the runner suite only on !nodeExe(), and test/internal/docker-services.test.ts:349 then runs that node on scripts/runner.node.ts, a TypeScript file; Node 22.x below 22.18 and anything older exits with ERR_UNKNOWN_FILE_EXTENSION and the exit object mismatches. Fix: skip the suite when the found node cannot load a .ts module (probe node --print on a tiny .ts fixture or check process.versions.node against the strip-types floor), or run the runner under the same mechanism CI uses.
Why this was flagged
The suite at test/internal/docker-services.test.ts:322 runs whenever isLinux and nodeExe() returns a path. runRunner at :347-362 spawns that node on join(testsDir, "..", "scripts", "runner.node.ts"). Node loads a .ts entry only with type stripping, which is on by default from 22.18 and 23.6; on an older node the process exits 1 with ERR_UNKNOWN_FILE_EXTENSION before writing bun.coordinator or bun.tests, so :384 and :392 compare {coordinatorArgv: [], ran: [], exit: {exitCode: 1, ...}} against the expected objects and fail. Population: local contributors on Linux distributions that ship Node 18/20/22-LTS-early (Debian/Ubuntu system packages), which is the common case outside the CI images that pin 26.3.0 (scripts/build/ci-images/spec.ts:102). The base has no such test. REVIEW.md requires a skipIf when a system dependency is unavailable; here the dependency is 'a node that strips types', and the guard only checks presence. Remedy: extend the skip condition to the capability, not the binary.
Verification: Triggers when a Linux contributor's node on PATH predates default type stripping. test/internal/docker-services.test.ts:322 gates the suite only on !nodeExe(), which has no version check. Line 349 then spawns nodeExe()! on scripts/runner.node.ts without --experimental-strip-types. Such a node exits 1 with ERR_UNKNOWN_FILE_EXTENSION, so both toEqual assertions at 384 and 392 fail rather than skip.
| let services: ServiceName[] | undefined; | ||
| for (const specifier of importSpecifiers(source)) { | ||
| if (!specifier.startsWith("./") && !specifier.startsWith("../")) continue; | ||
| const imported = posix.join(posix.dirname(path), specifier).replace(/\.[cm]?[jt]sx?$/, ""); |
There was a problem hiding this comment.
🟡 (optional) A valkey test that imports its helper through the js/* tsconfig alias is not docker-backed for the runner, so its redis container is not prestarted and the file waits for a cold start inside itself, or inside the parallel batch once the generator allowlists it. scripts/docker-services.ts:60 skips every specifier that does not begin with ./ or ../, but test/tsconfig.json defines "js/*": ["./js/*"] and bun resolves import ... from "js/valkey/test-utils" the same as the relative form. Fix: resolve helper imports through every spelling bun accepts, at least the tsconfig paths aliases, or match on the helper basename plus test-utils path suffix rather than relative-only specifiers. [also at: scripts/docker-services.ts:42 - nit: a test that imports the docker helper with a spelling other than docker/index or docker/index.ts is not seen as docker-backed, so it can enter the parallel batch. importsDockerIndex at scripts/docker-services.ts:42 accepts only (?:\.ts)?, while the helper lookup at line 60 strips any \.[cm]?[jt]sx?, and the bare directory import ../../docker that Bun resolves to index.ts matches neither.]
Why this was flagged
A test under test/js/valkey/ that writes import { createClient } from "js/valkey/test-utils" (the js/* alias is declared in test/tsconfig.json paths) reaches scripts/runner.node.ts:573 through needsDockerService. servicesOfImportedHelpers at scripts/docker-services.ts:60 drops the specifier because it starts with neither ./ nor ../, so requestsIn returns backed=false. On the base branch the js/valkey/ prefix row in test/docker/prestart-map.mjs made every file under that directory docker-backed regardless of import spelling, prestarted redis_unified and kept the file out of the bucket. After this change the file is sorted among non-docker files, redis_unified is not on the coordinator's argv (runner.node.ts:744), and the file pays the cold compose up wait in its own beforeAll; after the next scripts/update-parallel-allowlist.mjs run it can be allowlisted and wait inside the bun test --parallel batch. Remedy: resolve tsconfig path aliases (or match the helper by path suffix) in servicesOfImportedHelpers.
Verification: nit — triggers only when a future test file imports the valkey helper through the js/* tsconfig alias; no such file exists in the tree today. scripts/docker-services.ts:59 discards any non-relative specifier before the helperServices lookup at :60-63. On the base branch the js/valkey/ prefix row in test/docker/prestart-map.mjs covered any file under js/valkey/ regardless of import spelling.
| // Only a debug build needs more than the default: there the coordinator takes several seconds to start. | ||
| // No CI lane runs one. | ||
| const timeout = isDebug ? 30_000 : undefined; |
There was a problem hiding this comment.
🟡 nit (optional): the three coordinator tests pass a per-test timeout, which test/CLAUDE.md forbids ("Do not set a timeout on tests. Bun already has timeouts"). test/internal/docker-services.test.ts:261 sets timeout = isDebug ? 30_000 : undefined and hands it to test.concurrent at lines 281, 294 and 317. Fix: drop the timeout argument and the timeout constant; if a debug build is genuinely too slow for the default, shrink the work the test does (CI already passes its own --timeout via scripts/runner.node.ts:2172) rather than widening the ceiling.
Why this was flagged
test/internal/docker-services.test.ts:261 defines const timeout = isDebug ? 30_000 : undefined; and passes it as the third argument of test.concurrent at lines 281, 294 and 317. test/CLAUDE.md line 120 says in a CRITICAL rule not to set a timeout on tests, and the root CLAUDE.md line 105 says a per-test timeout is only for a rare outlier. The base branch has no such test file, so the convention slip is introduced here. A maintainer reading the file sees a hand-set 30 s ceiling on three subprocess tests, which is the pattern the repo rules reject; no safeguard catches it because no lint checks test timeouts.
Verification: test/internal/docker-services.test.ts:261 const timeout = isDebug ? 30_000 : undefined; is passed as the third argument to test.concurrent at lines 281, 294 and 317. test/CLAUDE.md:120 states "CRITICAL: Do not set a timeout on tests. Bun already has timeouts.". Triggering condition: any debug-build local run of this file; the base branch has no such file, so the slip is new.
Problem
test/docker/prestart-map.mjsis a hand-written prefix list. It decides which containers a shard starts at launch, which files run last and which stay out of thebun test --parallelbatch. It is wrong for 11 of the 14 docker-backed files added since it landed.coordinator: ensuring postgres_plainmid-run, 12.9 to 30 s), 29 inside the batch. Only Linux agent time pays: 3.2%.Fix
scripts/docker-services.tsreads what a test file asks for from its source. The runner uses that for all three decisions. The map is deleted.test/internal/docker-services.test.ts(4 of 15 fail on main's wiring). CI of this PR (build 124593): no container starts inside a test file, launch starts 444 to 309.Background
docker composecommand. Tests reach it over a unix socket.Downsides
bun testfiles once: median 108 reads, about 41 ms per Linux shard.Notes
Measured in the CI of this PR
The numbers come from the 140 Linux test job logs of two builds: main build 124219 and build 124593 of this PR. A launch start is a
coordinator: ensuring <service>line in the first 2 s afterCOORDINATOR_READY. A start on request is the same line later in the log.coordinator: ensuring mariadb_plainis at launch on each of the 7 shards that runsql-mysql-row-decode-error.test.ts.coordinator: ensuring postgres_plainis at launch on each of the 7 shards that runpostgres-bind-encode-throw.test.tsand on each of the 7 that runsql-begin-commit-rolled-back.test.ts. For the 11 files, 77 of 77 runs have every service they ask for started at launch.docker infofails there), sodocker compose updid not run there. Before the CI run, a static replay of the logs of build 124219 gave launch starts / first start on request / never requested of 444 / 56 / 231 for main, 503 / 0 / 234 for main with 11 more map rows, and 308 / 0 / 39 for this PR.Per-file wall time, before (build 124219) and after (build 124593)
sql-mysql-row-decode-error.test.tspostgres-bind-encode-throw.test.tssql-begin-commit-rolled-back.test.tsThe other 8 of the 11 files, slowest lane before and after:
postgres-bytea-bind18.38 s and 0.48 s,postgres-datestyle20.41 s and 0.31 s,postgres-listen-notify19.06 s and 5.36 s (56 tests of its own work),postgres-prepared-pipeline-reorder14.91 s and 0.72 s,postgres-row-decode-error18.03 s and 0.43 s,sql-empty-column-name21.79 s and 0.79 s,sql-reserve-abort17.44 s and 0.30 s,sql-statement-cache-hash-collision31.14 s and 1.16 s.In build 124219 on debian x64 shard 7, the log has
coordinator: ensuring mariadb_plaininside the first file andcoordinator: mariadb_plain ready13.5 s later. The map starts three mysql services for that file and it asks formariadb_plain. The other two files match no row, so they run inside thebun test --parallelbatch. On debian x64 shard 1 the batch of 71 files waits about 15 s for onepostgres_plaincontainer. In build 124593 the six files that were in the batch run after the serial files.What the rule reads
describeWithContainer(), importstest/docker/index.ts, or imports a helper module thathelperServiceslists. Today that list has one entry,test/js/valkey/test-utils.ts, which 11 valkey test files import.image: "<service>"in adescribeWithContainer()file, or the first argument of a call in a file that importstest/docker/index.ts.mysql:8andmysql:9count asmysql_plainandmysql_native_password, becausedescribeWithContainer()chooses between them at run time.test/docker/service-names.mts.ServiceNameintest/docker/index.tsis now the type of that list. A test asserts that the list equals theservices:keys ofdocker-compose.yml.bun testfiles, and it names at least one service for each of them.describeWithContainer()stays out of the batch, as in main's allowlist generator (ci: re-land the per-file parallel allowlist with a denylist of every file that has failed in the batch #39373).What changes for files that did not have the problem
postgres-bind-encode-throw,postgres-bytea-bind,postgres-datestyle,postgres-listen-notify,postgres-prepared-pipeline-reorder,postgres-row-decode-error,sql-begin-commit-rolled-back,sql-empty-column-name,sql-reserve-abort,sql-statement-cache-hash-collision(all intest/js/sql).sql-mysql*files get the one service they ask for and not three.sql-mysql-row-decode-errorgetsmariadb_plain.postgres-bind-encode-throw,postgres-bytea-bind,postgres-row-decode-error,sql-begin-commit-rolled-back,sql-empty-column-name,sql-reserve-abort. No file enters it today.js/sql/postgres-binary-numeric-digit-range,js/sql/sql-mysql-{auth-short-nonce, clean-reentry, columns-realloc-oom, duplicate-auth-switch, prepare-ok-zero-statement-id, tls-plaintext-injection},js/valkey/{reliability/resp-nesting-depth, valkey-gc, valkey-incremental-scan, valkey-tls-verify},js/web/websocket/websocket-proxy-{close-reentrancy, tunnel-client-leak, tunnel-upgrade-leak}.test/parallel-allowlist.json(generated 2026-08-17) excludes 10 of them and does not listjs/valkeyorjs/valkey/reliability.scripts/update-parallel-allowlist.mjsnow uses the same rule. At the next regeneration the 14 qualify by the normal rules (fast on every lane, no flaky annotation, not in the denylist). None of them has run in the batch, and the CI of this PR does not show that change. This PR does not regenerate the snapshot. It changes one phrase of its_meta.rule.Cost
bun testfile of the shard, memoised. Over the 140 shard lists of build 124219: median 108 reads (96 to 122), median 1.73 MB, 0 failed opens, 15,316 reads per build. Main: 0 reads.straceis not in the container, so a small ptrace counter made this count.COORDINATOR_READYcomes 50 ms after spawn on main and here (release build). Its argv for that shard is 1 service name (14 bytes). It was 301 test paths (15,259 bytes).postgres_plainhere.Tests
test/internal/docker-services.test.tshas 15 tests. It takes about 1 s on a release build and 9 to 15 s on a debug build, where three coordinators start at the same time (4.5 to 6.6 s each). Those three tests take a 30 s timeout on a debug build only. CI passes--timeout=90000.test/docker/coordinator.ts. Two runscripts/runner.node.tswith stand-ins forbun,dockerandwhoonPATH. ci: serialize docker service startup through a per-shard coordinator #32033 removed a coordinator test that used a docker shim. These five are kept because they need no docker, and they are the only tests that reach the spawn line of the runner and the launch loop of the coordinator. If a maintainer prefers the ci: serialize docker service startup through a per-shard coordinator #32033 decision, remove all five together.test/internal/source-lints/,test/internal/parallel-allowlist.test.ts,test/internal/runner-junit.test.ts,test/internal/ci-slowest-tests.test.ts,tsc -p scripts/tsconfig.json, and the new file throughscripts/runner.node.tswith exception-check and leak validation on.Open PRs that edit or import the deleted map
test/docker/prestart-map.mjs: test(docker): add postgres-prepared-pipeline-reorder to prestart-map #35912, test(sql): prestart postgres for the LISTEN/NOTIFY file, run its suites concurrently, assert exact errors #40193, test(sql): prestart the containers for the statement-cache hash-collision file, store the colliding queries, assert rows and prepare counts #40537, test(sql): prestart postgres for sql-reserve-abort, run its tests concurrently, assert results #41104, test(sql): prestart mariadb for sql-empty-column-name, one mock child per adapter, assert whole rows #41176, test(sql): prestart postgres for postgres-bytea-bind, run its tests concurrently, assert the bytes read back #42587, test(docker): prestart postgres for postgres-row-decode-error #44043, sql: bind every safe integer number as int8 or BIGINT #43293, sql(postgres): decode float arrays in exponent form and json[] numbers past int32 #43336, sql: make MySQLError and PostgresError in native code #44118, sql: dial a closed pool slot again for the callers still queued on the pool #44938, install: fold the open bun install / pm / pack+publish PRs into one branch (171 PRs) #39403. After this merges each gets a modify/delete conflict. The resolution isgit rm test/docker/prestart-map.mjs. For each of the 12, the rule reads from the test file exactly the services of the row.test/internal/docker-compose-services.test.ts,scripts/ci-slowest-tests.ts). They get no conflict and then fail at the import. They needisDockerBackedordockerServicesForfromscripts/docker-services.ts.Not in this PR
isDockerEnabled(), which returns false on Linux arm64 (test/harness.ts). test: resolve this harness TODO #25212 removes that return. A rule that skips those files misses 11 requests in the replay, so the rule does not look at that guard.test/expected-durations.jsonstill counts the container waits in the time of these files until its next refresh.Scope
scripts/update-parallel-allowlist.mjsalso reads the deleted map, so it moves to the rule. The rule is inscripts/because the runner is the one that evaluates it. Two tsconfig entries for the deleted file are removed. No file undersrc/and no sql test file changes.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/internal/docker-services.test.ts