Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 3 additions & 1 deletion src/runtime/webcore/ReadableStream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,9 @@ impl ReadableStream {
event_loop: core::cell::Cell::new(jsc::EventLoopHandle::init(
cx.vm().as_mut().event_loop().cast(),
)),
start_offset: Some(blob.offset.get() as usize),
// pread only for a slice. An unsliced Blob reads from the fd's current offset.
start_offset: (blob.offset.get() != 0)
.then_some(blob.offset.get() as usize),
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
max_size: if blob.size.get() != webcore::blob::MAX_SIZE {
Some(blob.size.get() as usize)
} else {
Expand Down
45 changes: 45 additions & 0 deletions test/js/node/process/process-stdin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -665,3 +665,48 @@ test("process.stdin over an anonymous pipe delivers each byte exactly once", asy
expect(stdout).toBe(`${total} ${expected}`);
expect(err).toBeNull();
});

// A regular-file stdin is an open file description shared with the parent
// shell. The parent may already have consumed a prefix, and a sibling that
// runs after Bun continues from where Bun stopped. So the stream must read
// from the current offset with read(2), not pread from 0.
describe.skipIf(isWindows)("regular-file stdin inherited at an offset", () => {
const input = "HEADER-already-consumed\nbody1\nbody2\n";

async function runAfterRead(js: string, tail = "") {
using dir = tempDir("stdin-file-offset", { "in.txt": input });
await using proc = Bun.spawn({
cmd: ["sh", "-c", `{ read hdr; "$BUN" -e ${JSON.stringify(js)}; ${tail} } < in.txt`],
Comment on lines +673 to +679

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.

🔴 The Windows half of this fix ships with no test: the whole describe is skipIf(isWindows), so the libuv offset -1 path the PR relies on is never exercised and a regression there stays green. The block only skips Windows because it drives the scenario through sh, read, cat, wc and tr (test/js/node/process/process-stdin.test.ts:679, :706). Fix: make the four cases run on every platform without a shell: open in.txt with fs.openSync, consume the header with fs.readSync(fd, buf) (position null advances the shared offset), spawn bun with Bun.spawn({ stdin: fd }), and after exited assert fs.readSync(fd, buf) returns 0 in place of cat | wc -c.

Why this was flagged

On Windows an unsliced file Blob now takes the start_offset: None branch at src/runtime/webcore/ReadableStream.rs:548, which reaches WindowsBufferedReader with USE_PREAD unset and passes -1 to uv_fs_read at src/io/PipeReader.rs:1590 and :1669; on the base branch that reader always got offset 0. Nothing checks this: test/js/node/process/process-stdin.test.ts:673 is describe.skipIf(isWindows), and the PR's own evidence block says the tests were deferred to CI, which skips them on Windows. The shell dependency is the only reason for the skip; Bun.spawn accepts stdin: <fd> (src/runtime/api/bun/spawn/stdio.rs:441-489 maps a number to Stdio::Fd), which on POSIX dup2s the fd and on Windows inherits the handle, both sharing the file position with the parent, so fs.openSync + fs.readSync in the parent reproduces read hdr and a post-exit fs.readSync returning 0 reproduces the cat | wc -c check. The same rewrite also removes the BSD-vs-GNU tool differences that already needed a tr -d ' ' patch on this block.

Verification: Trigger is any Windows CI run, so a Windows-only regression of this change can never turn a test red. src/runtime/webcore/ReadableStream.rs:548-549 changes start_offset to None with no platform gate, so the Windows reader now passes -1 to uv_fs_read instead of _offset = 0 as on the base. The new block at test/js/node/process/process-stdin.test.ts:673 is describe.skipIf(isWindows)(...).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed: the Windows path has no coverage in this PR, and I cannot run Windows.

I wrote the shell-free version you describe. The parent opens in.txt with fs.openSync, consumes the header with fs.readSync, passes the fd as the child's stdin, and calls fs.readSync again after the child exits to see what the next reader gets. It has three tests (the old fourth test is now the left: 0 check in the first two). On Linux all three fail on main 4b02e10 (stdout has the header, 12 bytes are left for the next reader) and pass with this change.

It is not pushed. CI on this head is green (build 122398, 181 of 181 jobs), and a push starts a new run. If a maintainer wants it before the merge, I push it. If not, I open it as a follow-up PR after the merge, so that the Windows lanes run it.

Patch
diff --git a/test/js/node/process/process-stdin.test.ts b/test/js/node/process/process-stdin.test.ts
index f6d52989c5..2ad6921570 100644
--- a/test/js/node/process/process-stdin.test.ts
+++ b/test/js/node/process/process-stdin.test.ts
@@ -1,6 +1,8 @@
 import { describe, expect, test } from "bun:test";
 import { bunEnv, bunExe, isASAN, isDebug, isWindows, tempDir } from "harness";
 import { exec } from "node:child_process";
+import { closeSync, openSync, readSync } from "node:fs";
+import { join } from "node:path";
 
 test.concurrent("pipe does the right thing", async () => {
   // Note: Bun.spawnSync uses memfd_create on Linux for pipe, which means we see
@@ -666,47 +668,46 @@ test("process.stdin over an anonymous pipe delivers each byte exactly once", asy
   expect(err).toBeNull();
 });
 
-// A regular-file stdin is an open file description shared with the parent
-// shell. The parent may already have consumed a prefix, and a sibling that
-// runs after Bun continues from where Bun stopped. So the stream must read
-// from the current offset with read(2), not pread from 0.
-describe.skipIf(isWindows)("regular-file stdin inherited at an offset", () => {
-  const input = "HEADER-already-consumed\nbody1\nbody2\n";
-
-  async function runAfterRead(js: string, tail = "") {
-    using dir = tempDir("stdin-file-offset", { "in.txt": input });
-    await using proc = Bun.spawn({
-      cmd: ["sh", "-c", `{ read hdr; "$BUN" -e ${JSON.stringify(js)}; ${tail} } < in.txt`],
-      cwd: String(dir),
-      env: { ...bunEnv, BUN: bunExe() },
-      stdout: "pipe",
-      stderr: "pipe",
-    });
-    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
-    return { stdout, stderr, exitCode };
+// A regular-file stdin is an open file description shared with the parent,
+// like `{ read header; bun body.js; } < file`. The stream must read from the
+// current offset and advance it for whoever reads next, not pread from 0.
+describe("regular-file stdin inherited at an offset", () => {
+  const header = "HEADER-already-consumed\n";
+  const body = "body1\nbody2\n";
+
+  // `left` is what the next reader of the same description still gets.
+  async function runAfterHeader(js: string) {
+    using dir = tempDir("stdin-file-offset", { "in.txt": header + body });
+    const fd = openSync(join(String(dir), "in.txt"), "r");
+    try {
+      expect(readSync(fd, Buffer.alloc(header.length))).toBe(header.length);
+      await using proc = Bun.spawn({
+        cmd: [bunExe(), "-e", js],
+        env: bunEnv,
+        stdin: fd,
+        stdout: "pipe",
+        stderr: "pipe",
+      });
+      const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+      return { stdout, stderr, exitCode, left: readSync(fd, Buffer.alloc(64)) };
+    } finally {
+      closeSync(fd);
+    }
   }
 
-  test.concurrent("process.stdin 'data' starts after the line the shell consumed", async () => {
+  test.concurrent("process.stdin reads the rest and leaves the offset at the end", async () => {
     const js = `let s = ""; process.stdin.on("data", d => (s += d)).on("end", () => process.stdout.write(JSON.stringify(s)));`;
-    expect(await runAfterRead(js)).toEqual({ stdout: JSON.stringify("body1\nbody2\n"), stderr: "", exitCode: 0 });
+    expect(await runAfterHeader(js)).toEqual({ stdout: JSON.stringify(body), stderr: "", exitCode: 0, left: 0 });
   });
 
-  test.concurrent("Bun.stdin.stream() starts after the line the shell consumed", async () => {
-    const js = `process.stdout.write(JSON.stringify(await new Response(Bun.stdin.stream()).text()));`;
-    expect(await runAfterRead(js)).toEqual({ stdout: JSON.stringify("body1\nbody2\n"), stderr: "", exitCode: 0 });
+  test.concurrent("Bun.stdin.stream() reads the rest and leaves the offset at the end", async () => {
+    const js = `let s = ""; for await (const chunk of Bun.stdin.stream()) s += Buffer.from(chunk).toString(); process.stdout.write(JSON.stringify(s));`;
+    expect(await runAfterHeader(js)).toEqual({ stdout: JSON.stringify(body), stderr: "", exitCode: 0, left: 0 });
   });
 
-  test.concurrent("for await (const line of console) starts after the line the shell consumed", async () => {
+  test.concurrent("for await (const line of console) starts after the consumed header", async () => {
     const js = `for await (const line of console) { process.stdout.write(JSON.stringify(line)); break; }`;
-    expect(await runAfterRead(js)).toEqual({ stdout: JSON.stringify("body1"), stderr: "", exitCode: 0 });
-  });
-
-  test.concurrent("process.stdin advances the shared offset for the next reader", async () => {
-    const js = `let n = 0; process.stdin.on("data", d => (n += d.length)).on("end", () => process.stdout.write("bun:" + n + "\\n"));`;
-    expect(await runAfterRead(js, `echo "cat:$(cat | wc -c | tr -d ' ')";`)).toEqual({
-      stdout: "bun:12\ncat:0\n",
-      stderr: "",
-      exitCode: 0,
-    });
+    const { stdout, stderr, exitCode } = await runAfterHeader(js);
+    expect({ stdout, stderr, exitCode }).toEqual({ stdout: JSON.stringify("body1"), stderr: "", exitCode: 0 });
   });
 });

cwd: String(dir),
env: { ...bunEnv, BUN: bunExe() },
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
return { stdout, stderr, exitCode };
}

test.concurrent("process.stdin 'data' starts after the line the shell consumed", async () => {
const js = `let s = ""; process.stdin.on("data", d => (s += d)).on("end", () => process.stdout.write(JSON.stringify(s)));`;
expect(await runAfterRead(js)).toEqual({ stdout: JSON.stringify("body1\nbody2\n"), stderr: "", exitCode: 0 });
});

test.concurrent("Bun.stdin.stream() starts after the line the shell consumed", async () => {
const js = `process.stdout.write(JSON.stringify(await new Response(Bun.stdin.stream()).text()));`;
expect(await runAfterRead(js)).toEqual({ stdout: JSON.stringify("body1\nbody2\n"), stderr: "", exitCode: 0 });
});

test.concurrent("for await (const line of console) starts after the line the shell consumed", async () => {
const js = `for await (const line of console) { process.stdout.write(JSON.stringify(line)); break; }`;
expect(await runAfterRead(js)).toEqual({ stdout: JSON.stringify("body1"), stderr: "", exitCode: 0 });
});

test.concurrent("process.stdin advances the shared offset for the next reader", async () => {
const js = `let n = 0; process.stdin.on("data", d => (n += d.length)).on("end", () => process.stdout.write("bun:" + n + "\\n"));`;
expect(await runAfterRead(js, `echo "cat:$(cat | wc -c | tr -d ' ')";`)).toEqual({
stdout: "bun:12\ncat:0\n",
stderr: "",
exitCode: 0,
});
});
});
Loading