Conversation
…oncurrently, assert the bytes read back Add the file to test/docker/prestart-map.mjs so the CI coordinator starts postgres_plain when the shard starts and the runner orders the file late. Without the entry the container cold start ran inside this file's beforeAll (18s on alpine 3.23 x64). Each test owns its connection, so the describe block is concurrent. Assertions: the position test checks the whole message. The round-trip test also reads the bytea value back and compares the bytes, and covers a Uint16Array. A rejected case that wrongly resolves now prints the rows.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughThe PostgreSQL ChangesPostgreSQL bytea test coverage
Priority: ➖ Normal Severity of issue fixed: High Merge Risk: ⚪ Minimal · up to The test and Docker prestart updates have no identified merge-blocking behavior risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The pull request does not implement the core requirements in issue Resolution Implement atomic writer rollback in
Comment |
|
Status: ready for review. The diff is green. CI is red only on tests that this PR does not touch. Reproduced from CI build 114889 (alpine 3.23 x64, shard 2). The file reported 18.46 s. The shard log has Locally Postgres already runs on 127.0.0.1, so there is no container start to measure. Interleaved medians: debug build 4.63 s before and 4.41 s after, release build 159 ms before and 130 ms after. CI on this PR, builds 115105 and 115109 (the second is a rerun of the same diff). Both are finished.
This PR changes one Postgres test file and adds one line to |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only tightening plus a one-line prestart-map entry, matching the sibling js/sql/* entries exactly.
What was reviewed:
describeWithContaineracceptsconcurrent, and each test opens its ownawait using sql = connect(), so concurrency is safe; the header comment cites #34732 for why a shared connection isn't hoisted.rejection()calls.then()on the lazy query thenable and surfaces{ resolved: rows }on unexpected success, so no assertion is vacuous; error checks still assert class +.code+ full message.- The
Uint16Arraycase is built over aUint8Arraybuffer, so the expected10111213is endianness-independent;stored("")and the explicit{ hex: null, bytes: null }cover empty and NULL correctly.
Extended reasoning...
Overview
Two files changed. test/docker/prestart-map.mjs gains one entry mapping js/sql/postgres-bytea-bind to postgres_plain, inserted alphabetically among identical sibling entries so the CI coordinator warms the container before the shard reaches this file. test/js/sql/postgres-bytea-bind.test.ts is refactored: concurrent: true on describeWithContainer, a rejection() helper replacing inline .then(() => null, e => e), toStartWith upgraded to an exact toBe on the full message, and the round-trip test now selects both encode(..., 'hex') and the raw ::bytea back, asserting a combined { hex, bytes } object and adding a Uint16Array variant.
Security risks
None. This is test code and a static string→string[] map consumed by the test runner. No user input, no auth/crypto/permissions paths, no runtime code touched.
Level of scrutiny
Low. The prestart-map change is a mechanical config addition following the exact pattern of a dozen neighboring lines. The test changes strengthen assertions (exact message, bytes read back, resolved-rows-on-failure diagnostic) and add a variant without removing any existing case — squarely what REVIEW.md asks for. I confirmed in test/harness.ts that describeWithContainer supports concurrent: boolean and routes to describe.concurrent, and that other test/js/sql/ files already use this option, so the pattern is established.
Other factors
Checked correctness details: rejection() invokes .then() so the lazy SQL query actually executes (avoiding the expect().rejects hang the PR notes); the Uint16Array is constructed over a byte buffer so the expected hex is host-endianness-independent; stored("") yields an empty Buffer and NULL is handled as its own branch. Each test still owns its connection via await using, so concurrent execution can't share a poisoned pool. No existing test was weakened or deleted. The timeline shows no outstanding third-party objections and the bug hunt exited on dry_streak with no findings.
|
Updated 4:34 AM PT - Sep 13th, 2026
❌ @robobun, your commit c6d9831 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 42587That installs a local version of the PR into your bun-42587 --bun |
|
The "Linked Issues check" warning in the bot summary above is a false positive. #34732 is a separate open PR. This PR only points to it, to explain why the tests do not share one connection. This PR is test-only and does not try to implement that change. One sentence in the description read like a closing keyword ( |
|
Order note from the review of #34732: that PR lands first. It adds tests to |
This branch has not been deployed
1 participant
Problem
test/js/sql/postgres-bytea-bind.test.tstook 18.46 s on alpine 3.23 x64 (build 114889). On the same lanesql-prepare-false.test.ts(eight tests, same container) took 183 ms.test/docker/prestart-map.mjs. Its shard loggedcoordinator: ensuring postgres_plainonly when this file asked, so the cold start ran inside the file'sbeforeAll.Fix
"js/sql/postgres-bytea-bind": ["postgres_plain"]to the map. The coordinator then starts the container at shard launch, and the runner runs the file after the non-docker files. Same pattern as 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 postgres for sql-reserve-abort, run its tests concurrently, assert results #41104.byteavalue back, compares the bytes, and covers aUint16Array. A rejected case that wrongly resolves prints the stored rows. No test was removed.bun bd test test/js/sql/postgres-bytea-bind.test.ts, 6 pass in 11 debug runs. Local Postgres already runs on 127.0.0.1, so local times contain no container start. Medians: debug 4.63 s before, 4.41 s after. Release 159 ms before, 130 ms after.Background
describeWithContainer(test/harness.ts) is a describe block whosebeforeAllasks the docker coordinator for a service. The first file to ask pays the container start.test/docker/prestart-map.mjsmaps test-path prefixes to services.test/docker/coordinator.tsstarts mapped services at shard launch.scripts/runner.node.mjsruns mapped files last, outside the parallel bucket.ERR_POSTGRES_CONNECTION_CLOSED(see open PR sql(postgres): discard a partial Bind when a parameter fails to encode #34732).Notes
CI attribution. Build 114889, job
01a097c4-9deb-426c-8da6-65829219437c(:alpine: 3.23 x64 - test-bun, shard 2). At launch the log hascoordinator: ensuring minioandcoordinator: ensuring redis_unified.coordinator: ensuring postgres_plainappears about 1200 lines later, andcoordinator: postgres_plain readyis the line before the file's header. The file is reported as[78/295] test/js/sql/postgres-bytea-bind.test.ts (18.46s). It ran inside the parallel bucket (Ran 1258 tests across 80 files. [47.99s]).Same build, same lane, for comparison:
sql-prepare-false.test.ts(shard 18)postgres_plain ready (cached)Ran 8 tests across 1 file. [183.00ms]postgres-multi-statement-fields.test.ts(shard 19)postgres_plain ready (cached)Ran 5 tests across 1 file. [103.00ms]postgres-bytea-bind.test.ts(shard 2)ensuring postgres_plainon requestsql-reserve-abort.test.ts(shard 17, #41104)ensuring postgres_plainon requestHow the entry removes the wait.
coordinator.tscallstestPath.startsWith(prefix)on each test path of the shard and starts every matched service when the shard starts.runner.node.mjsuses the same check inneedsDockerService. That check excludes the file fromisBucketCandidateand sorts it to the end of the serial list. The container start then overlaps with the tests that run first. I checked the new key against both path separator styles. It matches this file and does not matchpostgres-bool-bind.test.ts.The runner runs modified test files first in their shard. So in this PR's own CI run the file can still wait for the container. The gain shows on later builds.
test/parallel-allowlist.jsonwas generated on 2026-08-17, before this file existed, so the file is not inexcludeFiles. The generator excludes everydescribeWithContainercaller. The map entry gives the same result at run time.Local timing. Postgres already runs on 127.0.0.1 (
BUN_TEST_SERVICE_postgres_plainis set), so no number here contains a container start. The host had a load average near 100 on 16 cores, so absolute values are high. Runs are interleaved (before, after, before, after). Values are the runner's ownRan N teststime, median (minimum) of 9 rounds.bun bd test)USE_SYSTEM_BUN=1, 1.4.3-canary.1+b99371011)The debug number is all fixed cost. A
describeWithContainerfile with one test and no query took 3.0 to 3.2 s at a time when the old file took 2.9 to 3.0 s. On the release build that empty file takes 103 ms. Withoutconcurrent: truethe new file measures 4430 ms debug and 150 ms release. With it, the release run was faster in 9 of 9 rounds.Levers I measured and did not apply.
beforeAll. After a rejected bind on amax: 1pool,select 1fails withPostgresError: Connection closed. The same happens for an older case (an object whosetoStringthrows, bound totext) and on the 1.4.3 canary. sql(postgres): discard a partial Bind when a parameter fails to encode #34732 has the cause and a fix.Assertion details.
hexisencode(value, 'hex'), computed by the server, so it does not depend on how Bun decodes abytearesult.bytesis the same parameter selected back. It arrives as aBuffer. NULL gives{ hex: null, bytes: null }.Uint16Arrayis a view over the bytes10 11 12 13, so the expected value does not depend on endianness.expect(query).rejectsis not used. A Bun SQL query starts on.then()or.execute(). The matcher calls neither, and the test hangs past its timeout (related: event loop: never re-enter from inside a dispatch callback (de-block expect().resolves and HTMLRewriter.transform, then assert) #33261). Therejectionhelper calls.then().toMatchObject({ code, message })is not used. On acodemismatch its diff prints the error as[TypeError: message]and hides the receivedcode.Bufferexpected for NULL, a validBufferin the rejected table, a cut tail of the position message, a swappedUint16Arraybyte order.Open PRs #35912, #40193, #41104, #40537 and #41176 add their own lines to
prestart-map.mjs. This PR inserts at a different line. No other open PR touches this test file.[auto-merge] gate passed · iteration 1 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file