Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
efb9a1f
Add reproduction for #23333
Jarred-Sumner Oct 7, 2025
14360ee
[autofix.ci] apply automated fixes
autofix-ci[bot] Oct 7, 2025
77f9062
Add pause/unpause support to PosixBufferedReader for stdin flow control
Oct 7, 2025
9e4e3ae
[autofix.ci] apply automated fixes
autofix-ci[bot] Oct 7, 2025
9d7aa47
WIP: Add resume logic with buffered data handling
Oct 7, 2025
ae782bc
WIP: Attempted fix for setRawMode + PTY hang
Oct 7, 2025
d629c24
[autofix.ci] apply automated fixes
autofix-ci[bot] Oct 7, 2025
7d8e99d
Add extensive logging to debug stdin/PTY hang
Oct 7, 2025
bb79ed0
Merge remote-tracking branch 'origin/main' into jarred/repro-23333
pfgithub Oct 7, 2025
7aeec9a
more accurate setFlowing maybe
pfgithub Oct 7, 2025
6ac30c8
rename the debug log
pfgithub Oct 7, 2025
88f6936
disable debug logging
pfgithub Oct 7, 2025
32de07d
[autofix.ci] apply automated fixes
autofix-ci[bot] Oct 7, 2025
d227cc7
.getReader() sets flowing, .releaseLock() unsets flowing
pfgithub Oct 7, 2025
847c870
fixes the stdin pause pty test
pfgithub Oct 7, 2025
4a31b21
changes to 09041 test output
pfgithub Oct 7, 2025
80064c1
noquiet
pfgithub Oct 7, 2025
b7db557
fixes
pfgithub Oct 8, 2025
5d0165e
updates range display
pfgithub Oct 8, 2025
56b3e3b
for CI
pfgithub Oct 8, 2025
727d1d4
[autofix.ci] apply automated fixes
autofix-ci[bot] Oct 8, 2025
a57f9e7
updates autofix limit
pfgithub Oct 8, 2025
3ea42f2
fixes stdin-pause-pty on linux
pfgithub Oct 8, 2025
114fe13
windows python fix?
pfgithub Oct 8, 2025
b06eaa7
fileroute fix
pfgithub Oct 8, 2025
f98fefe
[autofix.ci] apply automated fixes
autofix-ci[bot] Oct 8, 2025
119ead6
Revert ".getReader() sets flowing, .releaseLock() unsets flowing"
pfgithub Oct 8, 2025
5e6219f
Merge branch 'main' into jarred/repro-23333
pfgithub Oct 8, 2025
b22b023
Merge branch 'main' into jarred/repro-23333
Jarred-Sumner Oct 8, 2025
b81c581
Add minidump print
zackradisic Oct 8, 2025
eb8717e
Report crashes
zackradisic Oct 8, 2025
f59d15b
Report crashes
zackradisic Oct 8, 2025
596f9a5
Report crashes
zackradisic Oct 8, 2025
ad7dd93
Report crashes
zackradisic Oct 8, 2025
5fd2d04
Fix Windows compilation errors in crash_handler.zig
Oct 8, 2025
1337061
increase std.unicode. it's in the crash handler, it's fine
pfgithub Oct 8, 2025
1554633
stdin-pause-pty test skip on windows. the python code uses the pty mo…
pfgithub Oct 8, 2025
4c7669d
fuzzy wuzzy logging, for CI
pfgithub Oct 8, 2025
97e1844
Merge remote-tracking branch 'origin/main' into jarred/repro-23333
pfgithub Oct 8, 2025
fe724c6
disable the fuzzy wuzzy logging
pfgithub Oct 9, 2025
068ef9f
Merge remote-tracking branch 'origin/main' into jarred/repro-23333
pfgithub Oct 9, 2025
114af68
[autofix.ci] apply automated fixes
autofix-ci[bot] Oct 9, 2025
71107b4
Probably fix the crash
Jarred-Sumner Oct 9, 2025
114e8b9
Try this
zackradisic Oct 9, 2025
cd0e493
[autofix.ci] apply automated fixes
autofix-ci[bot] Oct 9, 2025
d048579
try
zackradisic Oct 9, 2025
621cd6d
comments
zackradisic Oct 9, 2025
60995f6
bump
zackradisic Oct 9, 2025
47c3c89
Remove unnecessary changes
zackradisic Oct 9, 2025
336538e
Fix
zackradisic Oct 9, 2025
671cc3f
[autofix.ci] apply automated fixes
autofix-ci[bot] Oct 9, 2025
90e0d97
Fix
zackradisic Oct 9, 2025
77661e3
Fix
zackradisic Oct 9, 2025
5860414
Upstream deleted htis
zackradisic Oct 9, 2025
89e6db6
Merge branch 'main' into jarred/repro-23333
pfgithub Oct 10, 2025
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
8 changes: 6 additions & 2 deletions src/bun.js/api/server/FileRoute.zig
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,9 @@ const StreamTransfer = struct {
log("max_size reached, ending stream", .{});
if (this.route.server) |server| {
// dont need to ref because we are already holding a ref and will be derefed in onReaderDone
this.reader.pause();
if (!bun.Environment.isPosix) {
this.reader.pause();
}
Comment on lines +400 to +402
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Document why pause is Windows-only.

The POSIX guard on reader.pause() suggests platform-specific flow-control semantics, but the rationale isn't documented. Consider adding a brief comment explaining why pause is only needed on non-POSIX platforms when max_size is reached.

Apply this diff to add clarifying documentation:

                 if (state_ != .eof and max_size.* == 0) {
                     // artificially end the stream aka max_size reached
                     log("max_size reached, ending stream", .{});
                     if (this.route.server) |server| {
                         // dont need to ref because we are already holding a ref and will be derefed in onReaderDone
+                        // On Windows, explicitly pause to stop reading
+                        // On POSIX, flow control is managed by the new setFlowing mechanism
                         if (!bun.Environment.isPosix) {
                             this.reader.pause();
                         }
📝 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
if (!bun.Environment.isPosix) {
this.reader.pause();
}
// dont need to ref because we are already holding a ref and will be derefed in onReaderDone
// On Windows, explicitly pause to stop reading
// On POSIX, flow control is managed by the new setFlowing mechanism
if (!bun.Environment.isPosix) {
this.reader.pause();
}

// we cannot free inside onReadChunk this would be UAF so we schedule it to be done in the next event loop tick
this.eof_task = jsc.AnyTask.New(StreamTransfer, StreamTransfer.onReaderDone).init(this);
server.vm().enqueueTask(jsc.Task.init(&this.eof_task.?));
Expand Down Expand Up @@ -430,7 +432,9 @@ const StreamTransfer = struct {
// pause the reader so deref until onWritable
defer this.deref();
this.resp.onWritable(*StreamTransfer, onWritable, this);
this.reader.pause();
if (!bun.Environment.isPosix) {
this.reader.pause();
}
Comment on lines +435 to +437
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Document why pause is Windows-only.

Same as the max_size scenario: the platform-specific pause behavior lacks documentation. Consider adding a comment explaining the flow-control difference between POSIX and Windows.

Apply this diff to add clarifying documentation:

         switch (this.resp.write(chunk)) {
             .backpressure => {
                 // pause the reader so deref until onWritable
                 defer this.deref();
                 this.resp.onWritable(*StreamTransfer, onWritable, this);
+                // On Windows, explicitly pause to stop reading
+                // On POSIX, flow control is managed by the new setFlowing mechanism
                 if (!bun.Environment.isPosix) {
                     this.reader.pause();
                 }
🤖 Prompt for AI Agents
In src/bun.js/api/server/FileRoute.zig around lines 435 to 437, add a brief
comment above the if (!bun.Environment.isPosix) { this.reader.pause(); } block
that documents why pause is applied only on Windows: explain the
platform-specific flow-control difference (POSIX pipes/sockets support native
backpressure/non-blocking reads so explicit pause is unnecessary, whereas
Windows' IO model can keep feeding data without the same backpressure semantics
so we must explicitly pause the reader to prevent uncontrolled buffering), and
reference the analogous max_size handling for consistency.

return false;
},
.want_more => {
Expand Down
4 changes: 4 additions & 0 deletions src/bun.js/api/streams.classes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ function source(name) {
fn: "setRawModeFromJS",
length: 1,
},
setFlowing: {
fn: "setFlowingFromJS",
length: 1,
},
}
: {}),
},
Expand Down
58 changes: 35 additions & 23 deletions src/bun.js/webcore/FileReader.zig
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ lazy: Lazy = .{ .none = {} },
buffered: std.ArrayListUnmanaged(u8) = .{},
read_inside_on_pull: ReadDuringJSOnPullResult = .{ .none = {} },
highwater_mark: usize = 16384,
flowing: bool = true,

pub const IOReader = bun.io.BufferedReader;
pub const Poll = IOReader;
Expand Down Expand Up @@ -487,6 +488,14 @@ pub fn onPull(this: *FileReader, buffer: []u8, array: jsc.JSValue) streams.Resul
}

if (!this.reader.hasPendingRead()) {
// If not flowing (paused), don't initiate new reads
if (!this.flowing) {
log("onPull({d}) = pending (not flowing)", .{buffer.len});
this.pending_value.set(this.parent().globalThis, array);
this.pending_view = buffer;
return .{ .pending = &this.pending };
}

this.read_inside_on_pull = .{ .js = buffer };
this.reader.read();

Expand Down Expand Up @@ -581,32 +590,15 @@ pub fn onReaderDone(this: *FileReader) void {
}
this.buffered = .{};
this.pending.run();
} else if (this.buffered.items.len > 0) {
const this_value = this.parent().this_jsvalue;
const globalThis = this.parent().globalThis;
if (this_value != .zero) {
if (Source.js.onDrainCallbackGetCached(this_value)) |cb| {
const buffered = this.buffered;
this.buffered = .{};
this.parent().incrementCount();
defer _ = this.parent().decrementCount();
this.eventLoop().js.runCallback(
cb,
globalThis,
.js_undefined,
&.{
jsc.ArrayBuffer.fromBytes(buffered.items, .Uint8Array).toJS(globalThis) catch |err| {
this.pending.result = .{ .err = .{ .WeakJSValue = globalThis.takeException(err) } };
return;
},
},
);
}
}
}
// Don't handle buffered data here - it will be returned on the next onPull
// This ensures proper ordering of chunks
}

this.parent().onClose();
// Only close the stream if there's no buffered data left to deliver
if (this.buffered.items.len == 0) {
this.parent().onClose();
}
if (this.waiting_for_onReaderDone) {
Comment on lines +594 to 602
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

onClose may never fire if buffered > 0 at EOF

Calling onClose only when buffered is empty risks never invoking the close handler because there's no later point that calls parent().onClose() after buffered data is delivered. This can break consumer onclose handlers and leak lifecycle signals.

Apply this minimal fix to always signal close when the reader is done (ordering-sensitive consumers can still rely on the “done” result ordering):

-    // Only close the stream if there's no buffered data left to deliver
-    if (this.buffered.items.len == 0) {
-        this.parent().onClose();
-    }
+    // Signal close exactly once when the reader is done.
+    // Ordering of data vs. close is handled at the JS layer via result flags.
+    this.parent().onClose();

If strict “data then close” ordering is required, alternatively queue onClose on the event loop after pending.run() resolves.

Can you confirm whether any tests depend on close firing strictly after the last chunk? If so, I can propose an event‑loop queued onClose to preserve ordering.


🏁 Script executed:

#!/bin/bash
set -e
rg -n 'waiting_for_onReaderDone' -C3 src/bun.js/webcore/FileReader.zig
echo "-----"
rg -n 'onClose' -C3 src/bun.js/webcore/FileReader.zig

Length of output: 1332


#!/bin/bash
set -e
sed -n '500,650p' src/bun.js/webcore/FileReader.zig

🏁 Script executed:

sed -n '550,650p' src/bun.js/webcore/FileReader.zig

Length of output: 2932


🏁 Script executed:

rg -n 'fn onPull' -C5 src/bun.js/webcore/FileReader.zig

Length of output: 350


Always signal close when reader is done Guarding onClose() on buffered.items.len == 0 means it never fires if there’s buffered data at EOF. Unconditionally call parent().onClose() (or queue it on the event loop for strict ordering). Can you confirm whether any tests expect close to fire strictly after the last chunk?

🤖 Prompt for AI Agents
In src/bun.js/webcore/FileReader.zig around lines 594 to 602, the current guard
prevents calling parent().onClose() when buffered.items.len > 0 so onClose won't
fire at EOF if there is buffered data; remove the length guard and
unconditionally invoke parent().onClose() when the reader is done, or if strict
ordering is required, schedule parent().onClose() on the event loop (enqueue a
task) so it always fires after any remaining buffered chunks are delivered; also
ensure existing waiting_for_onReaderDone logic is preserved/updated to avoid
double-calls.

this.waiting_for_onReaderDone = false;
_ = this.parent().decrementCount();
Expand All @@ -631,6 +623,26 @@ pub fn setRawMode(this: *FileReader, flag: bool) bun.sys.Maybe(void) {
return this.reader.setRawMode(flag);
}

pub fn setFlowing(this: *FileReader, flag: bool) void {
log("setFlowing({}) was={}", .{ flag, this.flowing });

if (this.flowing == flag) {
return;
}

this.flowing = flag;

if (flag) {
this.reader.unpause();
if (!this.reader.isDone() and !this.reader.hasPendingRead()) {
// Kick off a new read if needed
this.reader.read();
}
} else {
this.reader.pause();
}
}

pub fn memoryCost(this: *const FileReader) usize {
// ReadableStreamSource covers @sizeOf(FileReader)
return this.reader.memoryCost() + this.buffered.capacity;
Expand Down
13 changes: 13 additions & 0 deletions src/bun.js/webcore/ReadableStream.zig
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,19 @@ pub fn NewSource(
@compileError("setRawMode is not implemented on " ++ @typeName(Context));
}

pub fn setFlowingFromJS(this: *ReadableStreamSourceType, _: *jsc.JSGlobalObject, call_frame: *jsc.CallFrame) bun.JSError!JSValue {
if (@hasDecl(Context, "setFlowing")) {
const flag = call_frame.argument(0);
if (Environment.allow_assert) {
bun.assert(flag.isBoolean());
}
this.context.setFlowing(flag == .true);
return .js_undefined;
}

return .js_undefined;
}
Comment on lines +580 to +591
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Mirror setRawModeFromJS: mark binding, set this, and compile‑time guard

Add markBinding and cache this for parity; prefer a compile‑time error if Context lacks setFlowing (like setRawMode).

Apply:

-        pub fn setFlowingFromJS(this: *ReadableStreamSourceType, _: *jsc.JSGlobalObject, call_frame: *jsc.CallFrame) bun.JSError!JSValue {
-            if (@hasDecl(Context, "setFlowing")) {
+        pub fn setFlowingFromJS(this: *ReadableStreamSourceType, global: *jsc.JSGlobalObject, call_frame: *jsc.CallFrame) bun.JSError!JSValue {
+            jsc.markBinding(@src());
+            this.this_jsvalue = call_frame.this();
+            if (@hasDecl(Context, "setFlowing")) {
                 const flag = call_frame.argument(0);
                 if (Environment.allow_assert) {
                     bun.assert(flag.isBoolean());
                 }
                 this.context.setFlowing(flag == .true);
                 return .js_undefined;
             }
-
-            return .js_undefined;
+            @compileError("setFlowing is not implemented on " ++ @typeName(Context));
         }
📝 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
pub fn setFlowingFromJS(this: *ReadableStreamSourceType, _: *jsc.JSGlobalObject, call_frame: *jsc.CallFrame) bun.JSError!JSValue {
if (@hasDecl(Context, "setFlowing")) {
const flag = call_frame.argument(0);
if (Environment.allow_assert) {
bun.assert(flag.isBoolean());
}
this.context.setFlowing(flag == .true);
return .js_undefined;
}
return .js_undefined;
}
pub fn setFlowingFromJS(this: *ReadableStreamSourceType, global: *jsc.JSGlobalObject, call_frame: *jsc.CallFrame) bun.JSError!JSValue {
jsc.markBinding(@src());
this.this_jsvalue = call_frame.this();
if (@hasDecl(Context, "setFlowing")) {
const flag = call_frame.argument(0);
if (Environment.allow_assert) {
bun.assert(flag.isBoolean());
}
this.context.setFlowing(flag == .true);
return .js_undefined;
}
@compileError("setFlowing is not implemented on " ++ @typeName(Context));
}
🤖 Prompt for AI Agents
In src/bun.js/webcore/ReadableStream.zig around lines 580-591, mirror the
pattern used in setRawModeFromJS: add a call to mark the JS binding, cache the
this/context locally, and use a compile-time guard for the presence of
Context.setFlowing. Specifically, invoke the appropriate markBinding on the call
frame before accessing arguments, store this.context into a local variable to
use for setFlowing, and replace the runtime @hasDecl check with a compile-time
check that emits @compileError if Context lacks setFlowing so the code fails to
compile rather than silently skipping behavior.


const supports_ref = setRefUnrefFn != null;

pub const js = @field(jsc.Codegen, "JS" ++ name_ ++ "InternalReadableStreamSource");
Expand Down
Loading