Repository navigation
Conversation
When shell commands (builtins, Cmd, Pipeline, etc.) complete and are freed, they may still have pending chunks in the IOWriter queue. If a write callback fires after the command is freed, it would try to call onIOWriterChunk on freed memory, causing a use-after-free crash (typically at address 0x0). This fixes the issue by canceling all pending IOWriter chunks that reference a command before freeing it. The fix adds cancelPendingChunks() calls to: - Builtin.deinit() - for all shell builtins (echo, cd, ls, etc.) - Cmd.deinit() - for external commands and builtin wrappers - Pipeline.deinit() - for pipeline commands - CondExpr.deinit() - for conditional expressions - Subshell.deinit() - for subshell commands Also adds cancelChunksWithRawPtr() to IOWriter to support canceling chunks by raw pointer address, needed for the Builtin case where we have a generic pointer to the impl union field. Closes #26660 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Collaborator
Author
|
Updated 2:05 AM PT - Feb 1st, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 26662That installs a local version of the PR into your bun-26662 --bun |
Contributor
WalkthroughAdds raw-pointer-based cancellation to IOWriter and calls to cancel pending IOWriter chunks from deinit across builtins and several shell state types to avoid use-after-free; adds a concurrency regression test exercising many concurrent shell commands. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test/regression/issue/26660.test.ts`:
- Around line 26-39: Add a negative spawn case in the same test to exercise
error paths: spawn another process (using Bun.spawn like the existing proc) that
runs a short script which either throws or calls process.exit(1), then await its
stdout/stderr and proc.exited (use a different variable name than proc to avoid
clobbering), and assert that the returned exitCode is non‑zero and stderr
contains an error indicator (e.g., "Error" or the thrown message); ensure you
mirror the existing pattern (Bun.spawn, .stdout.text(), .stderr.text(), .exited)
so the failure path is covered alongside the current success assertions.
- Around line 10-32: Replace the tempDir-based single-file test with an inline
script using the -e flag: remove the tempDir call that creates "test.ts"
(tempDir("issue-26660", { "test.ts": `...` })) and instead pass the script
content directly to Bun via Bun.spawn's cmd by replacing [bunExe(), "run",
"test.ts"] with [bunExe(), "run", "-e", "<script>"] (keeping the same script
body used in the temp file). Update the Bun.spawn call that sets
cwd/env/stderr/stdout accordingly and apply the same refactor to the other two
tests that use tempDir for single-file scripts.
- Around line 36-37: Remove the forbidden stderr string-absence assertions by
deleting the two lines that assert expect(stderr).not.toContain("Segmentation
fault") and expect(stderr).not.toContain("panic") from the test (leave the rest
of the test logic intact); rely on the existing exitCode and expected stdout
assertions instead to determine success/failure and do not add any other
negative substring checks on stderr in this test (file:
test/regression/issue/26660.test.ts).
Comment on lines
+26
to
+39
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "run", "test.ts"], | ||
| cwd: String(dir), | ||
| env: bunEnv, | ||
| stderr: "pipe", | ||
| stdout: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stderr).not.toContain("Segmentation fault"); | ||
| expect(stderr).not.toContain("panic"); | ||
| expect(stdout).toContain("All shell commands completed successfully"); | ||
| expect(exitCode).toBe(0); |
Contributor
There was a problem hiding this comment.
Add an explicit failing-spawn case to cover error paths.
Right now only the success path is exercised. Add a small negative-case spawn (e.g., process.exit(1) or a throw) and assert non‑zero exitCode + stderr.
As per coding guidelines: Always check exit codes and test error scenarios when spawning processes in tests.
🤖 Prompt for AI Agents
In `@test/regression/issue/26660.test.ts` around lines 26 - 39, Add a negative
spawn case in the same test to exercise error paths: spawn another process
(using Bun.spawn like the existing proc) that runs a short script which either
throws or calls process.exit(1), then await its stdout/stderr and proc.exited
(use a different variable name than proc to avoid clobbering), and assert that
the returned exitCode is non‑zero and stderr contains an error indicator (e.g.,
"Error" or the thrown message); ensure you mirror the existing pattern
(Bun.spawn, .stdout.text(), .stderr.text(), .exited) so the failure path is
covered alongside the current success assertions.
- Replace tempDir-based tests with inline scripts using -e flag - Remove forbidden stderr string-absence assertions - Add error path test case for failing commands Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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
2 participants
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.
Summary
cancelPendingChunks()to all shell command types that use IOWriter (Builtin, Cmd, Pipeline, CondExpr, Subshell)cancelChunksWithRawPtr()to IOWriter to support canceling by raw pointer addressProblem
When shell commands complete and are freed, they may still have pending chunks in the IOWriter queue. If a write callback fires after the command is freed, it would try to call
onIOWriterChunkon freed memory, causing a use-after-free crash (typically segfault at address 0x0).This was reported in #26660 where OpenCode on Windows crashed with many concurrent shell commands. The previous fix in #26626 added
cleanup_stateto prevent double-frees during GC finalization, but didn't address the IOWriter pending chunks issue.Solution
Cancel all pending IOWriter chunks that reference a command before freeing it by:
Adding
cancelPendingChunks()calls todeinit()of:Builtin.zig- for all shell builtins (echo, cd, ls, etc.)Cmd.zig- for external commands and builtin wrappersPipeline.zig- for pipeline commandsCondExpr.zig- for conditional expressionsSubshell.zig- for subshell commandsAdding
cancelChunksWithRawPtr()to IOWriter to support canceling chunks by raw pointer address, needed for the Builtin case where we have a generic pointer to the impl union field.Test plan
test/regression/issue/26660.test.tsthat spawns many concurrent shell commandsCloses #26660
🤖 Generated with Claude Code