Repository navigation
test: profile until the sampler has run in the profile test of bun-jsc.test.ts - #45854
Closed
dylan-conway wants to merge 1 commit into
Closed
dylan-conway wants to merge 1 commit into
dylan-conway wants to merge 1 commit into
Conversation
Collaborator
|
Updated 9:08 PM PT - Oct 17th, 2026
@dylan-conway, your commit d533c7a is building: |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe repeated-call profiling test retries each profiling call until it returns a stack trace or five seconds pass. The test retains its existing result and nonempty-trace assertions. ChangesProfiling test
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly describes the main change: retrying profiling until the sampler captures a stack trace in the bun-jsc test. |
| Description check | |
The description includes both required sections. It explains the flaky test, the retry and timeout behavior, the failure mode, and detailed verification results. |
| 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. |
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- 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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked that the retry loop stays bounded (a 5 s inline deadline per call, since the synchronous loop cannot be preempted by the test timeout; 15 s worst case across the three calls) and that the original regression this test guards — a profiler that produces no samples at all after the first call — still fails, because every retry returns zero traces until the deadline expires and the unchanged toBeGreaterThan(0) assertion then trips.
Extended reasoning...
The change is confined to one test, test/js/bun/jsc/bun-jsc.test.ts, wrapping three profile() calls in a retry-until-sampled helper with a 5 s deadline; it touches no runtime or security-sensitive code. The inline finding about the retry masking intermittently-sampling profilers is the reason a human should weigh the de-flake tradeoff, so this is not an approval.
Member
Author
|
Folded into #45873. |
dylan-conway
added a commit
that referenced
this pull request
Oct 20, 2026
### What does this PR do? Fixes 20 causes of flaky tests, found in the annotations of the last 400 CI builds. Only files under `test/` change. No limit or deadline is raised, and no assertion is dropped except where the table says on which platform. Each row was first its own PR. That PR has the CI sightings, the cause and the numbers in full. | test file | what went wrong | what changes | write-up | |---|---|---|---| | `cli/install/bun-install-streaming-extract.test.ts` | the line taken for the error could be one of the verbose extract log | those lines are removed before the `error:` line is looked for | #45842 | | `cli/install/bun-install-streaming-extract.test.ts` | a body that is all there at the client's first read is not streamed, so `Streamed ` is not printed | the fixture is larger than one read of the client holds | #45844 | | `js/bun/shell/yield.test.ts` | Windows keeps the working directory of a killed process busy for a while (`EBUSY` on `rm`); the pid file exists before the pid is in it | the pid file is named by its full path and nothing has the directory as its cwd; the waiter waits for the file's content | #45846 | | `internal/harness-max-rss.test.ts` | a busy macOS machine no longer keeps the runner's 256 MiB resident when they are read | the two assertions on the runner's own RSS are for Linux, the one platform where a child inherits it | #45847 | | `js/web/fetch/body-clone.test.ts` | the FIFO case also wrote to a stdin that its child never reads (`EPIPE`) | that case gets no stdin | #45848 | | `js/first_party/ws/ws-server-compat-close.test.ts` | the count of sends that were written assumed the loopback takes bytes at once | it counts up to the last send that left nothing buffered | #45849 | | `js/bun/spawn/spawn-stdin-destroy.test.ts` | `sleep(80)` stood for "the child has exited" | `await child.exited` | #45850 | | `regression/issue/29181.test.ts` | file descriptors are closed on the work pool, so some are still open right after the last response | the count is polled until it is under the same limit, with a deadline | #45852 | | `js/web/fetch/fetch-keepalive.test.ts` | Windows Server 2019 delivers loopback bytes late while other loopback connections are busy, as macOS does | the five TCP cases are left out on `10.0.17763` as they are on macOS | #45853 | | `js/bun/jsc/bun-jsc.test.ts` | one run of 10 ms gets no sample when the sampler thread gets no CPU | `profile()` is repeated until it has a sample, with a deadline | #45854 | | `js/third_party/jsonwebtoken/async_sign.test.js` | two tokens signed on either side of a second differ in `iat` | the payload has a fixed `iat` | #45855 | | `js/bun/test/parallel/test-http-should-emit-events-in-the-right-order.ts` | the fixture asked a host on the internet | it asks a local server | #45856 | | `cli/run/transpiler-cache.test.ts` | a killed helper still had the temp directory as its cwd when the next test removed it (`EBUSY`, Windows) | the helper is held with `await using` | #45857 | | `js/web/fetch/fetch-abort-stream-body.test.ts` | `expect(p).rejects` in a socket callback runs a nested tick of the event loop, after which a concurrent test's `proc.stdout.text()` never settles | the promise is awaited | #45862 | | `js/bun/gc/gc-controller-cadence.test.ts` | the wait ended at `<= limit`, the assertion wants `< limit` | the wait goes on at `>= limit`, and the reading that ends it is the one asserted | #45863 | | `js/web/websocket/websocket-client.test.ts` | a limit of 1 s per test from 2023, now with a real closing handshake with a `node` process inside it | the limit is removed, the runner's applies | #45864 | | `js/node/tls/node-tls-connect.test.ts` | in TLS 1.3 the server's handshake ends after the client's, and the test waited for the client's | it waits for both | #45866 | | `js/node/tls/tls-shutdown-before-handshake-fixture.mjs` | the "refused" port was one `connect()` chose, which Linux can hand to the socket under test as its own: it connects to itself | `bind()` chooses the port | #45869 | | `js/bun/module-graph/module-graph-io.test.ts` | on macOS `fs.watch()` returns before anything is watched, and the case wrote once | it writes until the listener is called | #45870 | | `js/bun/module-graph/module-graph-io.test.ts` | the closed UDP port can be given to the socket under test, which then hears itself and is never refused | the port stays bound until that socket exists | #45871 | ### How did you verify your code works? Each change on its own, with release and ASAN binaries that CI built from main's code: the failure reproduced with main's file (under load, or with the losing order forced by a delayed loopback, a blocked thread, a stopped process or a narrow port range), the same conditions with the fix, and a negative test that shows the test still fails on what it guards. Linux x64 locally, Windows 11 x64 for the Windows rows. Some of the numbers: | | main | fix | |---|---|---| | `yield.test.ts`, Windows, 96 at a time | 2878 of 4000 fail | 0 of 4000 | | `transpiler-cache.test.ts`, Windows | 99 of 120 | 0 of 120 | | `bun-install-streaming-extract.test.ts` (body size), Windows | 326 of 6000 | 0 of 6000 | | `29181.test.ts` | 143 of 280 | 0 of 280 | | `bun-jsc.test.ts`, contended CPU | 111 of 200 | 0 of 200 | | `node-tls-connect.test.ts`, loopback delayed by 5 ms | 3 of 3 in each round | 0 | | `tls-shutdown-before-handshake-fixture.mjs`, 32 ports, node and Bun | 12 of 100 each | 0 of 100 each | All of it together, on Linux x64: every changed file passes, but for two tests that fail with main's files on my machine too ("a fifo in place of an entry is removed instead of opened" in `transpiler-cache.test.ts`, and "under node > a session that is set before the handshake starts is resumed" in `node-tls-connect.test.ts`, which runs under my `node` v26.7.0). Not run on the platform they are for: the macOS rows (`harness-max-rss`, `ws-server-compat-close`, the `fs.watch` case), which rest on the CI failures, the source and an emulation on Linux. The Server 2019 numbers of `fetch-keepalive` are from a CI run.
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.
What does this PR do?
Fixes the flaky "profile can be called multiple times" in
test/js/bun/jsc/bun-jsc.test.ts. It failed in the parallel batch and passed alone in builds 127669 (Ubuntu 25.04 x64) and 127946 (Debian 13 x64):The profiled function keeps the JS thread busy for 10 ms, "so the sampler thread is guaranteed time to fire". The sampler is a thread of its own, and with the machine's CPUs taken by the rest of the batch it may not get to run at all in those 10 ms. Then there is no trace, though nothing is wrong with the profiler.
Each of the three calls now profiles again until there is a trace, for up to 5 s. The limit is in the test because the loop never yields, so the runner's timeout cannot end it. A profiler that no longer samples after the first call, which is what the test is for, uses up the limit and fails on the same line as before.
How did you verify your code works?
Linux x64, with a release binary CI built from main's code (d0831bc). That test alone, on one CPU that it shares with busy loops, main's file and this PR's in alternating rounds of 50:
The test still catches the bug it came with (#25939): with Bun 1.3.2 it fails with
Expected: > 0, Received: 0, and with 1.3.8 it passes.The whole file passes (41 pass).