Conversation
The test file was added in #33627 after prestart-map.mjs was last touched in #33622, so it is not in the map. That means: - scripts/runner.node.mjs does not push it toward the end of the shard - test/docker/coordinator.ts does not pre-warm postgres_plain for it When a shard's only postgres_plain test is this file (build #81338, debian 13 aarch64, shard [67/281]), it runs early and pays the full container cold-start (~18s) as its own wall time; the four tests themselves take ~80ms on a release build. Also flip the describe to concurrent: the four tests each own a max:1 SQL instance and share no state, so they can overlap their connect + warm-up round trip.
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 8:06 AM PT - Jul 26th, 2026
❌ @robobun, your commit ddb7553 has 2 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35912That installs a local version of the PR into your bun-35912 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
LGTM — straightforward test-infra change adding a missing prestart-map entry and enabling concurrent describe.
What was reviewed:
- Verified
describeWithContaineracceptsconcurrentand routes todescribe.concurrent(harness.ts:1099/1156); matches existing usage in the threesql-mysql*.test.tsfiles. - Confirmed each test owns its own
await using sql = new SQL({ max: 1 }), awaitscontainer.ready, and only issues statelessSELECTs — no shared state to make concurrency unsafe. - prestart-map entry matches the format and service name of its neighbors; a missing entry only means slower cold-start, so worst case is no-op.
Extended reasoning...
Overview
Two-line change across two test-only files: adds js/sql/postgres-prepared-pipeline-reorder → ["postgres_plain"] to test/docker/prestart-map.mjs, and flips the file's describeWithContainer to concurrent: true. No src/ changes.
Security risks
None. This touches CI test scheduling metadata and a test-runner concurrency flag. No user-facing code, no auth/crypto/permissions.
Level of scrutiny
Low. The prestart-map is a hand-maintained hint table whose header comment explicitly says a missing entry is "correct, just slower" — so a wrong entry can at worst pre-warm an unused container. The concurrent: true change is the only thing that could affect test correctness, and I verified: (1) the harness supports the option and it's already used identically in sql-mysql.test.ts, sql-mysql.helpers.test.ts, and sql-mysql.auth.test.ts; (2) each of the four tests creates and disposes its own max: 1 SQL instance via await using, awaits container.ready independently, and issues only parameterized SELECT statements against the shared postgres container — no DDL, no shared connection, no mutable module-level state. Running them concurrently cannot change what each test observes.
Other factors
The PR description shows 5/5 consecutive passing runs under describe.concurrent, and the map entry follows the exact format of the six neighboring js/sql/postgres-* entries. The known sibling gap (postgres-datestyle.test.ts) is explicitly called out as being handled in #35738. No outstanding reviewer comments.
|
Self-review complete, no surviving concerns. CI on ddb7553 (build #82338, 192/196 lanes done): every test lane ran the changed file green. The two red lanes are unrelated to this diff:
On debian 13 aarch64 (the 18s lane from build #81338) the changed file now reports 6.38s, and that number is inflated by the modified-tests-first ordering that runs it at the top of the shard only for this PR's build; on a normal main run the coordinator pre-warm plus docker-defer ordering put it at the end of the shard where Ready to merge. |
Problem
test/js/sql/postgres-prepared-pipeline-reorder.test.tsreported 18s wall time on debian 13 aarch64 in build #81338. The four tests in the file take ~80ms on a release build; the rest ispostgres_plaincontainer cold-start.The coordinator log for that shard shows why:
Other shards of the same build see
coordinator: postgres_plain ready (cached)and the file finishes in <1s.Cause
test/docker/prestart-map.mjsis read by two consumers:scripts/runner.node.mjssorts matching test files toward the end of the shard so container cold-start overlaps with the non-docker tests that run first.test/docker/coordinator.tspre-warms every mapped service at shard launch.This test file was added in #33627, right after the map was last touched in #33622, so it matches no prefix. In a shard where it is the only (or earliest-scheduled)
postgres_plaintest, the runner does not defer it and the coordinator does not pre-warm the container, so the file'sbeforeAllpays the full cold-start.Fix
js/sql/postgres-prepared-pipeline-reordertoprestart-map.mjs.describeWithContainertoconcurrent: true. Each test owns its ownmax: 1SQLinstance, issues onlySELECTwith parameters, and shares no state, so the four connect + warm-up round trips can overlap. This matches the existing pattern insql-mysql.test.ts/sql-mysql.helpers.test.ts/sql-mysql.auth.test.ts.The assertions are already strong (
toEqualon full row shapes, no exit-code-only checks, no "absence of panic" checks), so nothing to tighten there.postgres-datestyle.test.ts(#35112) has the same prestart-map gap; #35738 is already reworking that file to drop the container dependency entirely, so it is left alone here.Timing
Debug+ASAN, local, warm postgres via
BUN_TEST_SERVICE_postgres_plain(so the env-override path never paid the cold-start either; this measures only the in-file change):bun bd testwall timeCI (build #81338, debian 13 aarch64): 18s, all of it cold-start. With the map entry the coordinator pre-warms
postgres_plainat shard launch and the runner schedules this file after the non-docker tests, so the expected wall time is the same <1s every other mapped postgres file in that build already reports.Verification
This is a test-infrastructure change with no
src/modification; there is no fail-before state to demonstrate.[stamp-90s] gate passed · iteration 0 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
self-review · no surviving concerns
23 concerns were raised and did not survive verification.