Repository navigation
Handle floating promises and enable noFloatingPromises - #1488
cristianrgreco wants to merge 5 commits into
Conversation
Enable Biome's nursery/noFloatingPromises rule at error level and fix everything it reports: - Await archiver's finalize() together with putArchive() so a finalize rejection reaches the caller instead of becoming an unhandled rejection. Apply the same fix to the three copy*ToContainer methods on StartedGenericContainer, which the rule cannot see because it cannot infer archiver()'s return type. - Make the NATS pub/sub test assert on the messages it received after draining, so a wrong or missing message fails the test. - Make smoke-test.js exit non-zero explicitly when it fails. - Mark the Dockerfile pull-event observer as intentionally not awaited.
cristianrgreco
added
bug
✅ Deploy Preview for testcontainers-node ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The changes preserve the tested archive and asynchronous test outcomes; no material regression is established, so the PR is mergeable under normal checks. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the main changes: enabling Biome's noFloatingPromises rule and handling floating promises. |
| Description check | |
The description is detailed and directly explains the rule change, affected code, rationale, and verification results. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 5… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
…ses-de3b40 # Conflicts: # biome.json
A try/catch reads better than a trailing .catch(). The async function call itself is still an unawaited promise, so it is marked with void.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Wait for event delivery before asserting no pull. · generic-container-dockerfile.test.ts:89-90
packages/testcontainers/src/generic-container/generic-container-dockerfile.test.ts:89-90
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWait for event delivery before asserting no pull.
containerSpec.build()waits for the build response stream to end, butdockerPullEventPromiselistens on a separate Docker event stream. The test assertshasResolvedimmediately after the build completes. A pull event can reach the event stream after that assertion, so a pull regression can pass.Keep the observer active through a bounded post-build observation window, or use an equivalent timeout-aware event assertion before checking that no pull occurred.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
9ca9f9ef-078f-4120-9e76-f7d9bb2e1f66
📒 Files selected for processing (2)
biome.jsonpackages/testcontainers/smoke-test.js
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Docker events arrive on a separate stream from the build response, so a pull event could be delivered after the build finished and after the test had already asserted that none arrived. Keep observing for a short window after the build before asserting. Run the test apart from the file's other tests: the event stream is daemon-wide and a neighbouring test pulls the same image.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/testcontainers/src/generic-container/generic-container-dockerfile.test.ts (1)
92-93: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftSynchronize the no-pull assertion with event delivery.
build()resolves after the build response ends, but Docker deliversGET /eventsthrough a separate stream. Docker does not define ordering or a delivery deadline between these streams. A matching pull event may therefore arrive after the 500 ms delay, allowing this assertion to pass incorrectly.Use a completion signal for the relevant event, or assert the pull decision at its source.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
0307cb3b-80b0-45a6-806b-eb1384fa2bed
📒 Files selected for processing (1)
packages/testcontainers/src/generic-container/generic-container-dockerfile.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.
Watching the Docker event stream for the absence of a pull event cannot be made deterministic: events arrive on a separate stream with no delivery deadline, and the stream is daemon-wide. Build from a tag that only exists locally instead. The build succeeds when the image is not pulled and fails when a pull is attempted. This replaces the post-build observation window and lets the test run concurrently again.
This branch has not been deployed
1 participant
Summary
Enables Biome's
nursery/noFloatingPromisesrule aterrorlevel and fixes everything it reports. A floating promise is one that is never awaited, never given a rejection handler, and never explicitly ignored, so its errors are lost or become unhandled rejections.maingeneric-container.ts:209:archive.finalize()not handledawait Promise.all([archive.finalize(), putArchive(...)])(see below)nats-container.test.ts:21:(async () => { ... expect(...) })().then()nc.drain()smoke-test.js:3: top-level async IIFE not handledtry/catchinside the function that logs and exits with code 1. The call is markedvoid: atry/catchalone still leaves the un-awaited call flagged, and CommonJS has no top-level await.smoke-test.mjs(top-level await) andsmoke-test.jest.js(Jest awaits the test) don't need it.generic-container-dockerfile.test.ts:89:dockerPullEventPromise.then(...)void, with a comment explaining why:waitForDockerEventnever rejects and the test only records whether a pull happenedThe same
tar.finalize()pattern is also inStartedGenericContainer.copyFilesToContainer,copyDirectoriesToContainerandcopyContentToContainer. The rule doesn't flag them because Biome can't inferarchiver("tar")'s return type there, whilecreateArchiveToCopyToContainer()declares one. They get the same fix so all four call sites behave the same way.Why
Promise.alland notawait archive.finalize()archiver's
finalize()promise resolves when its internal tar module emitsendand rejects when that module emitserror(archiver/lib/core.js,Archiver.prototype.finalize). The module only reachesendonce its output has been read. The archiver stream buffers just 1 MiB (highWaterMark), and onlyputArchive()reads it. Awaitingfinalize()before callingputArchive()would therefore hang for any archive larger than the buffer. Running both concurrently keeps the existing ordering:finalize()is still called first andputArchive()consumes the stream.start()and thecopy*ToContainermethods now also see afinalize()rejection, andPromise.allhandles whichever promise loses the race, so neither rejection is left unhandled.Today the tar module's errors already reach
putArchive(): archiver re-emits them on its own stream, and docker-modem destroys the request on thaterror. So the user-visible change is that the duplicate rejection fromfinalize()is no longer left unhandled.Regression test for the archive fix: not feasible
Every archive failure I could trigger that makes
finalize()reject (a file that changes size betweenstatand read, which produces tar-stream'sSize mismatch) also throws an uncaught exception inside tar-stream. tar-stream emitserroron the tar entry stream and nothing listens for it. Vitest fails a run on that uncaught exception both before and after this change, so a test can't isolate the difference. The only way around it is to stub archiver's private internals, which would test the stub more than the behaviour.To show the difference, I ran a standalone script with the same wiring docker-modem uses: pipe the stream into the request, and destroy the request on the stream's
error. The copied file shrinks betweenstatand read:The remaining uncaught exception comes from archiver and tar-stream, not this repo's code. It's out of scope here; see the follow-up note at the end.
Rule level and nursery caveat
The rule is set to
error.lint:cirunsbiome ci --error-on-warnings, sowarnwould also fail CI, buterrormakesnpm run lintfail locally too, so authors see the problem before pushing. This is a nursery rule: Biome may change its behaviour, options or group between upgrades, including minor ones. A Biome bump may therefore need this config entry to be renamed or moved, or new findings fixed.#1485 also edits
linter.rulesinbiome.json;mainis merged into this branch with both rules enabled.Red → green
Lint rule
Before the fixes, with the rule enabled (
npx biome ci --error-on-warnings package.json packages):After the fixes:
npx biome lint --only=nursery/noFloatingPromises .over the whole repo goes fromFound 4 infos.(the rule's default severity) to no findings.NATS pub/sub test
The old test ran its assertion inside an un-awaited async loop, so it never affected the test result. I applied temporary mutations to each version of the test (not committed):
"WRONG"instead of the payloadUnhandled Rejection: AssertionError: expected 'WORLD' to deeply equal 'WRONG', not attributed to the testexpected [ 'WORLD' ] to deeply equal [ 'WRONG' ]nc.publish(...)expected [] to deeply equal [ 'WORLD' ]This test is the
natsPubsubsnippet included indocs/modules/nats.md, so the docs pick up the corrected example automatically.Verification
npm ci:package-lock.jsonunchangednpm run format: no changesnpm run lint: no changes, no diagnosticsnpm run check-compiles: exit 0npx biome ci --error-on-warnings package.json packages: exit 0, compared with exit 1 and 4 errors before the fixesnpx vitest run packages/testcontainers/src/generic-container/generic-container.test.ts -t copy: 13 passed. This covers copying files, directories, content and archives before start and into a started container, so it exercises all four changed call sites.npx vitest run packages/testcontainers/src/generic-container/generic-container-dockerfile.test.ts packages/modules/nats/src/nats-container.test.ts: 15 passednpm run build -w testcontainers && node packages/testcontainers/smoke-test.js: exit 0. WithDOCKER_HOST=tcp://127.0.0.1:1, it prints the error and exits 1.Why this is not breaking
No public API, types or options change. The only runtime change is in
GenericContainer.start()(when files, directories or content are copied) and inStartedGenericContainer.copyFilesToContainer,copyDirectoriesToContainerandcopyContentToContainer:putArchive()succeeds. By thenfinalize()has resolved too, so behaviour and timing don't change. The copy tests above confirm this.putArchive(). Now they may reject with the same error a little earlier, throughfinalize(), and no unhandled rejection is left behind.The other edits are to tests, the smoke-test script and lint config.
Follow-up (not in this PR)
While probing archiver I found that copying a file that becomes unreadable (
EACCES) or is deleted (ENOENT) betweenstatand read crashes the process with an uncaught exception from inside archiver (lazystream) instead of rejecting. A file that changes size does the same (tar-streamSize mismatch). This happens before and after this PR and is worth tracking separately.