Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/js/node/http2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2466,8 +2466,20 @@ class ServerHttp2Stream extends Http2Stream {
constructor(streamId, session, headers) {
super(streamId, session, headers);
}
pushStream() {
throw $ERR_HTTP2_PUSH_DISABLED();
pushStream(headers, options, callback) {
// Argument shuffle: pushStream(headers, callback) is valid.
if (typeof options === "function") {
callback = options;
options = undefined;
}
validateFunction(callback, "callback");
// Bun does not currently implement sending PUSH_PROMISE frames. Node
// throws ERR_HTTP2_PUSH_DISABLED synchronously only when the peer
// explicitly disabled push; otherwise runtime errors are delivered via
// the error-first callback on the next tick. Route the "not supported"
// case through the callback so user code written against Node's
// callback contract doesn't crash inside the 'stream' event handler.
process.nextTick(callback, $ERR_HTTP2_PUSH_DISABLED());
}

respondWithFile(path, headers, options) {
Expand Down
216 changes: 216 additions & 0 deletions test/regression/issue/30942.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
// https://github.com/oven-sh/bun/issues/30942
//
// `ServerHttp2Stream.pushStream` used to throw `ERR_HTTP2_PUSH_DISABLED`
// synchronously from inside the 'stream' event handler, which killed user
// code following Node's `(err, pushStream) => {}` callback pattern. Node
// routes the same error through the callback instead (async). The stub
// now matches that contract so callers can observe the error and recover
// until real PUSH_PROMISE support lands (see #28713).
Comment on lines +1 to +8
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.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Condense the file header to the two-line regression test pattern.

The multi-line explanation (lines 3-8) restates bug history and context rather than documenting test design rationale. As per coding guidelines, reduce to one brief follow-up line.

📝 Suggested revision
 // https://github.com/oven-sh/bun/issues/30942
-//
-// `ServerHttp2Stream.pushStream` used to throw `ERR_HTTP2_PUSH_DISABLED`
-// synchronously from inside the 'stream' event handler, which killed user
-// code following Node's `(err, pushStream) => {}` callback pattern. Node
-// routes the same error through the callback instead (async). The stub
-// now matches that contract so callers can observe the error and recover
-// until real PUSH_PROMISE support lands (see `#28713`).
+// pushStream should route ERR_HTTP2_PUSH_DISABLED via callback, not throw synchronously

As per coding guidelines for regression tests.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// https://github.com/oven-sh/bun/issues/30942
//
// `ServerHttp2Stream.pushStream` used to throw `ERR_HTTP2_PUSH_DISABLED`
// synchronously from inside the 'stream' event handler, which killed user
// code following Node's `(err, pushStream) => {}` callback pattern. Node
// routes the same error through the callback instead (async). The stub
// now matches that contract so callers can observe the error and recover
// until real PUSH_PROMISE support lands (see #28713).
// https://github.com/oven-sh/bun/issues/30942
// pushStream should route ERR_HTTP2_PUSH_DISABLED via callback, not throw synchronously
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/regression/issue/30942.test.ts` around lines 1 - 8, Replace the
multi-line file header in test/regression/issue/30942.test.ts with the two-line
regression test pattern: keep the issue URL comment and a single brief follow-up
line describing the test intent, removing the extra historical/context lines
(the current explanatory block after the URL). Ensure the header precedes the
test and uses the same comment style as other regression tests so the file
follows the repository's standard test header format.


import { expect, test } from "bun:test";
import http2 from "node:http2";

Check warning on line 11 in test/regression/issue/30942.test.ts

View check run for this annotation

Claude / Claude Code Review

Test file misplaced in test/regression/ and contradicts PR description

Per repo conventions (root `CLAUDE.md:66`, `test/CLAUDE.md:153`), `test/regression/issue/` is reserved for true regressions (worked in a previous release, then broke) — `pushStream()` was always a throwing stub, so these five tests belong in the existing `test/js/node/http2/node-http2.test.js` instead. The PR description and gate commands also claim the tests live in `test/js/node/http2/node-http2.test.js`, which contradicts the actual diff and suggests the verification shown may not have exerci
Comment on lines +1 to +11
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.

🟡 Per repo conventions (root CLAUDE.md:66, test/CLAUDE.md:153), test/regression/issue/ is reserved for true regressions (worked in a previous release, then broke) — pushStream() was always a throwing stub, so these five tests belong in the existing test/js/node/http2/node-http2.test.js instead. The PR description and gate commands also claim the tests live in test/js/node/http2/node-http2.test.js, which contradicts the actual diff and suggests the verification shown may not have exercised this file.

Extended reasoning...

What's wrong

The new test file is created at test/regression/issue/30942.test.ts, but the repo's contribution guidelines explicitly reserve that directory for regressions only:

  • Root CLAUDE.md:66: "test/regression/issue/${issueNumber}.test.ts is reserved for bugs that have a GitHub issue number and are true regressions (worked in a previous release, then broke). An issue number alone is not enough — if the behavior was never correct, it's not a regression and the test belongs in the existing file for that module."
  • test/CLAUDE.md:152-153: "Add tests to the existing test file for the code you're changing — do not create a new file." (followed by the same regression-only rule)

ServerHttp2Stream.pushStream() was always a synchronous-throw stub in Bun — the PR description itself says it "was a stub". It never worked correctly in any prior release, so #30942 is a missing-feature/compat bug, not a regression.

Description ↔ diff mismatch

The PR description's Verification section claims:

Five new tests in test/js/node/http2/node-http2.test.js

and shows gate commands like:

bun bd test test/js/node/http2/node-http2.test.js -t "createPushResponse|pushStream"

But the diff doesn't touch test/js/node/http2/node-http2.test.js at all — it creates test/regression/issue/30942.test.ts instead. So either the description is stale, or the gate commands shown were run against a file that doesn't contain these tests (in which case the "5 fail → 5 pass" output didn't actually exercise this code).

Step-by-step

  1. Before this PR, pushStream() was throw $ERR_HTTP2_PUSH_DISABLED() — never worked, in any Bun release.
  2. Therefore http2 pushStream throws synchronously instead of returning error in callback #30942 is not a regression; it's behavior that was never correct.
  3. Per CLAUDE.md:66 / test/CLAUDE.md:153, non-regression tests must go in the existing module test file, not test/regression/issue/.
  4. The existing module test file is test/js/node/http2/node-http2.test.js, which already exists and is where the PR description claims the tests were added.
  5. → The file is in the wrong location and the description/diff disagree.

Why existing checks don't catch this

There's no lint or CI check enforcing the test/regression/ placement rule — it's a documented convention only. CodeRabbit's "Files selected for processing" list even shows test/js/node/http2/node-http2.test.js (matching the description), not the file actually added, so the tooling appears to have been confused by the same mismatch.

Impact

Organizational only — the tests themselves are fine and will run in CI from either location. But it violates an explicit, twice-documented repo policy, and the description/diff contradiction means a reviewer can't trust the stated gate output without re-running it.

Fix

Move the five tests into test/js/node/http2/node-http2.test.js (as the PR description already claims), delete test/regression/issue/30942.test.ts, and re-run the gate against the correct file.


test("pushStream delivers ERR_HTTP2_PUSH_DISABLED through the callback, not a sync throw", async () => {
const server = http2.createServer();
const { promise, resolve, reject } = Promise.withResolvers<void>();

server.on("stream", stream => {
try {
stream.pushStream({ ":path": "/x.js" }, (err, pushStream) => {
try {
expect(err).toBeInstanceOf(Error);
expect((err as any).code).toBe("ERR_HTTP2_PUSH_DISABLED");
expect(err!.message).toBe("HTTP/2 client has disabled push streams");
expect(pushStream).toBeUndefined();
stream.respond({ ":status": 200 });
stream.end("ok");
} catch (e) {
reject(e);
}
});
} catch (e) {
reject(new Error(`pushStream threw synchronously: ${(e as any)?.code ?? (e as Error).message}`));
}
});

server.listen(0, () => {
const port = (server.address() as any).port;
const client = http2.connect(`http://localhost:${port}`);
const req = client.request({ ":path": "/" });
let body = "";
req.setEncoding("utf8");
req.on("data", chunk => {
body += chunk;
});
req.on("end", () => {
try {
expect(body).toBe("ok");
resolve();
} catch (e) {
reject(e);
} finally {
client.close();
server.close();
}
});
req.on("error", reject);
req.end();
});

await promise;
});

test("pushStream callback fires asynchronously (next tick), not synchronously", async () => {
const server = http2.createServer();
const { promise, resolve, reject } = Promise.withResolvers<void>();

server.on("stream", stream => {
let callbackInvoked = false;
try {
stream.pushStream({ ":path": "/x.js" }, err => {
callbackInvoked = true;
try {
expect((err as any)?.code).toBe("ERR_HTTP2_PUSH_DISABLED");
} catch (e) {
reject(e);
}
});
if (callbackInvoked) {
reject(new Error("pushStream callback fired synchronously"));
return;
}
Comment on lines +68 to +81
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.

🟡 Tests 2 and 3 will still pass if pushStream is changed to never invoke the callback at all — stream.respond()/end() run synchronously after pushStream() returns and the test resolves on the client's req.on('end'), so a no-op implementation goes undetected. Test 2's title says the callback "fires asynchronously (next tick)" but only checks the negative (not-synchronous) half; consider asserting callbackInvoked === true inside req.on('end') before resolve() (and similarly gating test 3's resolve() on the callback having fired). Tests 1 and 5 already cover the positive case suite-wide, so this is just a robustness nit.

Extended reasoning...

What's wrong

In test 2 ("pushStream callback fires asynchronously (next tick), not synchronously", lines 63–105) the server handler does:

let callbackInvoked = false;
stream.pushStream({ ":path": "/x.js" }, err => {
  callbackInvoked = true;
  expect((err as any)?.code).toBe("ERR_HTTP2_PUSH_DISABLED");
});
if (callbackInvoked) reject(new Error("pushStream callback fired synchronously"));
stream.respond({ ":status": 200 });
stream.end("ok");

and the client side does:

req.on("end", () => { client.close(); server.close(); resolve(); });

So the test resolves as soon as the response round-trips. The pushStream callback is never required to fire for resolve() to be reached. Test 3 ("accepts the (headers, callback) shape", lines 107–143) has the same shape: respond()/end() are outside the callback and resolve() is on req.on('end').

Step-by-step proof

Suppose pushStream were changed to:

pushStream(headers, options, callback) {
  if (typeof options === "function") { callback = options; options = undefined; }
  validateFunction(callback, "callback");
  // forget to schedule callback — no-op
}

Walk test 2:

  1. pushStream(...) returns without throwing → outer try does not reject.
  2. callbackInvoked is still false → the "fired synchronously" check passes.
  3. stream.respond() / stream.end("ok") run synchronously.
  4. Client receives response, req.on('end') fires → resolve().
  5. The callback never runs, so the expect((err as any)?.code).toBe(...) assertion is never evaluated.
  6. Test passes despite the implementation never invoking the callback.

Walk test 3 the same way: the callback body (the only place the ERR_HTTP2_PUSH_DISABLED assertion lives) never executes; respond()/end() still run; req.on('end')resolve(); test passes.

So test 2's title ("callback fires asynchronously") is only half-verified — it proves the callback does not fire synchronously, but never proves it does fire on the next tick. Test 3 never confirms the 2-arg overload actually reaches the callback.

Why existing code doesn't prevent it

Tests 1 and 5 are immune because stream.respond()/stream.end() (or res.end()) live inside the pushStream/createPushResponse callback there — if the callback never fired, the request would hang and the test would time out. So the suite as a whole would catch a no-op regression. The gap is only at the individual-test level: tests 2 and 3 are weaker than their titles imply, and someone reading test 2 in isolation would assume it covers the positive case.

Impact

Test-quality nit, not a runtime bug. The implementation in this PR is correct (it does call process.nextTick(callback, err)), and suite-level coverage exists via tests 1 & 5. But if a future refactor accidentally dropped the process.nextTick call, tests 2 and 3 would not flag it on their own.

Fix

Trivial — in test 2, assert the positive half before resolving:

req.on("end", () => {
  client.close();
  server.close();
  if (!callbackInvoked) return reject(new Error("pushStream callback never fired"));
  resolve();
});

(hoist callbackInvoked so it's visible in the client closure, or set a second flag). For test 3, either move resolve() into the callback after the err assertion, or track a callbackInvoked flag and check it in req.on('end') the same way.

} catch (e) {
reject(new Error(`pushStream threw synchronously: ${(e as any)?.code ?? (e as Error).message}`));
return;
}
stream.respond({ ":status": 200 });
stream.end("ok");
});

server.listen(0, () => {
const port = (server.address() as any).port;
const client = http2.connect(`http://localhost:${port}`);
const req = client.request({ ":path": "/" });
req.resume();
req.on("end", () => {
client.close();
server.close();
resolve();
});
req.on("error", reject);
req.end();
});

await promise;
});

test("pushStream accepts the (headers, callback) shape with options omitted", async () => {
const server = http2.createServer();
const { promise, resolve, reject } = Promise.withResolvers<void>();

server.on("stream", stream => {
try {
stream.pushStream({ ":path": "/x.js" }, (err, pushStream) => {
try {
expect((err as any)?.code).toBe("ERR_HTTP2_PUSH_DISABLED");
expect(pushStream).toBeUndefined();
} catch (e) {
reject(e);
}
});
stream.respond({ ":status": 200 });
stream.end("ok");
} catch (e) {
reject(e);
}
});

server.listen(0, () => {
const port = (server.address() as any).port;
const client = http2.connect(`http://localhost:${port}`);
const req = client.request({ ":path": "/" });
req.resume();
req.on("end", () => {
client.close();
server.close();
resolve();
});
req.on("error", reject);
req.end();
});

await promise;
});

test("pushStream rejects a non-function callback synchronously (ERR_INVALID_ARG_TYPE)", async () => {
const server = http2.createServer();
const { promise, resolve, reject } = Promise.withResolvers<void>();

server.on("stream", stream => {
try {
expect(() => (stream as any).pushStream({ ":path": "/x.js" }, "not-a-function")).toThrow(
expect.objectContaining({ code: "ERR_INVALID_ARG_TYPE" }),
);
expect(() => (stream as any).pushStream({ ":path": "/x.js" })).toThrow(
expect.objectContaining({ code: "ERR_INVALID_ARG_TYPE" }),
);
stream.respond({ ":status": 200 });
stream.end("ok");
resolve();
} catch (e) {
reject(e);
}
});

server.listen(0, () => {
const port = (server.address() as any).port;
const client = http2.connect(`http://localhost:${port}`);
const req = client.request({ ":path": "/" });
req.resume();
req.on("end", () => {
client.close();
server.close();
});
req.on("error", () => {});
req.end();
});

await promise;
});

test("Http2ServerResponse.createPushResponse delivers the error via its callback", async () => {
const server = http2.createServer((req, res) => {
res.createPushResponse({ ":path": "/x.js" }, (err, pushRes) => {
expect((err as any)?.code).toBe("ERR_HTTP2_PUSH_DISABLED");
expect(pushRes).toBeUndefined();
res.end("ok");
});
});
const { promise, resolve, reject } = Promise.withResolvers<void>();

server.listen(0, () => {
const port = (server.address() as any).port;
const client = http2.connect(`http://localhost:${port}`);
const req = client.request({ ":path": "/" });
let body = "";
req.setEncoding("utf8");
req.on("data", chunk => {
body += chunk;
});
req.on("end", () => {
try {
expect(body).toBe("ok");
resolve();
} catch (e) {
reject(e);
} finally {
client.close();
server.close();
}
});
req.on("error", reject);
req.end();
});

await promise;
});
Loading