Skip to content

test: sign a payload with a fixed iat in jsonwebtoken/async_sign.test.js - #45855

Closed
dylan-conway wants to merge 1 commit into
mainfrom
claude/jsonwebtoken-async-sign-fixed-iat
Closed

dylan-conway wants to merge 1 commit into
mainfrom
claude/jsonwebtoken-async-sign-fixed-iat

Conversation

@dylan-conway

Copy link
Copy Markdown
Member

What does this PR do?

Fixes the flaky "should return the same result as singing synchronously" in test/js/third_party/jsonwebtoken/async_sign.test.js. It timed out and passed on a retry in builds 127615 (Alpine 3.23 x64), 127674 (Debian 13 x64 ASAN), 127693 (Windows 2019 x64), 127852 (Debian 13 x64) and 127926 (Ubuntu 25.04 aarch64):

✗ signing a token asynchronously > when signing a token > should return the same result as singing synchronously [90000.00ms]
  ^ this test timed out after 90000ms, before its done callback was called.
15 pass
1 skip
1 fail
27 expect() calls

A run that passes also has 27 expect() calls, so in those builds the callback did run, and all three of its expects with it.

The test signs { foo: "bar" } asynchronously and, in the callback, once more synchronously, and expects the same token. jwt.sign adds iat, the current time in seconds, to a payload that has none. If a second begins between the two calls, the tokens differ and expect(asyncToken).toEqual(syncToken) throws. That is inside the try of jws's SignStream.prototype.sign, whose catch emits 'error'. The listener for that is the same callback wrapped in lodash.once, which has been called already. So nothing is reported, done is never called, and the test waits for its timeout.

The payload now has an iat, so both calls sign the same thing whenever they run.

How did you verify your code works?

Linux x64, with binaries CI built from main's code.

With a preload that makes every Date.now() one second later than the one before, main's file fails as in CI (that test alone, 15 pass, 1 skip, 1 fail, 27 expect() calls), and with this PR the file passes (16 pass, 1 skip).

With the real clock, that test alone, 8 runs at a time:

main this PR
release 25 of 4000 fail 0 of 8000
ASAN, CI's settings 16 of 600 fail 0 of 1200

With a preload that only records the two timestamps, in 4000 runs of main's file the 14 runs where they differ are the 14 that fail.

The test still checks what it did: with the asynchronous path made to sign another payload (a preload that patches jws.createSign), and with a process.nextTick that never runs its callbacks, it fails.

@coderabbitai

coderabbitai Bot commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 998bb43c-9f93-4d1c-9b5c-2cac9073fd6d
📥 Commits

Reviewing files that changed from the base of the PR and between d0831bc and 5c8f0f7.

📒 Files selected for processing: 1
  • test/js/third_party/jsonwebtoken/async_sign.test.js
  • 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Oct 18, 2026

Copy link
Copy Markdown
Collaborator
Updated 9:08 PM PT - Oct 17th, 2026

@dylan-conway, your commit 5c8f0f7 is building: #128009

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

LGTM — pinning iat removes the only time-dependent input to the two sign() calls, and the assertion still compares the async and sync tokens as before. I also checked the other jsonwebtoken tests for a second sign-vs-sign comparison that could race the same way; this is the only one.

Extended reasoning...

The change touches one vendored third-party test file, test/js/third_party/jsonwebtoken/async_sign.test.js, replacing two inline payloads with a shared payload that carries a fixed iat claim (+4/-2). No runtime or native code and no security-sensitive surface is involved. jsonwebtoken honors a caller-supplied iat (it only stamps the current second when the payload has none) and copies the payload unless mutatePayload is set, so sharing the object between the async and sync calls is safe. The original property (async and sync sign agree) is still asserted, and the one inline finding is a pre-existing hang-on-failure nit in the done-callback style shared by the whole vendored file, not something this PR introduces.

@dylan-conway

Copy link
Copy Markdown
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants