Skip to content

Conversation

@wagenet
Copy link
Contributor

@wagenet wagenet commented Dec 22, 2025

Summary

  • Fix panic when oxlint stdout pipe buffer fills up
  • Add retry logic for WouldBlock errors with 1ms backoff
  • Handle Interrupted and BrokenPipe gracefully

When oxlint is spawned as a child process (e.g., via Node.js execa/child_process), stdout is connected to a pipe. If the parent process doesn't consume output fast enough, the pipe buffer fills (8KB on macOS, 64KB on Linux) and writes return WouldBlock (EAGAIN, error code 35 on macOS). Previously this caused a panic:

Os { code: 35, kind: WouldBlock, message: "Resource temporarily unavailable" }

This PR replaces write_all with write_all_retry that properly handles transient I/O errors by retrying.

Test plan

  • Added unit tests for write_all_retry and flush_retry
  • Tests cover: success, WouldBlock retry, BrokenPipe, other errors, partial writes, empty buffer

🤖 Generated with Claude Code

When oxlint is spawned as a child process (e.g., via Node.js execa),
stdout is connected to a pipe. If the parent doesn't consume output
fast enough, the pipe buffer fills and writes return WouldBlock (EAGAIN).
Previously this caused a panic.

Replace `write_all` with `write_all_retry` that properly handles:
- WouldBlock: sleep 1ms and retry (back-pressure)
- Interrupted: retry immediately
- BrokenPipe: return gracefully
- Partial writes: continue until all bytes written

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Copilot AI review requested due to automatic review settings December 22, 2025 00:11
@github-actions github-actions bot added the C-bug Category - Bug label Dec 22, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a panic in oxlint when writing to stdout pipes by implementing proper retry logic for transient I/O errors. When oxlint is used as a child process and the parent doesn't consume output fast enough, the pipe buffer fills and write operations return WouldBlock errors, which previously caused panics.

Key changes:

  • Replaced check_for_writer_error with write_all_retry and flush_retry functions that handle WouldBlock, Interrupted, and BrokenPipe errors
  • Added 1ms sleep backoff for WouldBlock retries to prevent busy-waiting
  • Implemented comprehensive unit tests with mock writers simulating various error conditions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

.write_all(err_str.as_bytes())
.or_else(Self::check_for_writer_error)
.unwrap();
Self::write_all_retry(writer, err_str.as_bytes()).unwrap();
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

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

The return value of write_all_retry (which indicates success or broken pipe via Ok(true) vs Ok(false)) is being ignored by calling unwrap() directly. Consider handling the Ok(false) case explicitly to distinguish between successful writes and broken pipe scenarios, or document why ignoring this distinction is acceptable.

Copilot uses AI. Check for mistakes.
.write_all(err_str.as_bytes())
.or_else(Self::check_for_writer_error)
.unwrap();
Self::write_all_retry(writer, err_str.as_bytes()).unwrap();
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

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

The return value of write_all_retry (which indicates success or broken pipe via Ok(true) vs Ok(false)) is being ignored by calling unwrap() directly. Consider handling the Ok(false) case explicitly to distinguish between successful writes and broken pipe scenarios, or document why ignoring this distinction is acceptable.

Copilot uses AI. Check for mistakes.
.write_all(finish_output.as_bytes())
.or_else(Self::check_for_writer_error)
.unwrap();
Self::write_all_retry(writer, finish_output.as_bytes()).unwrap();
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

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

The return value of write_all_retry (which indicates success or broken pipe via Ok(true) vs Ok(false)) is being ignored by calling unwrap() directly. Consider handling the Ok(false) case explicitly to distinguish between successful writes and broken pipe scenarios, or document why ignoring this distinction is acceptable.

Copilot uses AI. Check for mistakes.
}

writer.flush().or_else(Self::check_for_writer_error).unwrap();
Self::flush_retry(writer).unwrap();
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

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

The return value of flush_retry (which indicates success or broken pipe via Ok(true) vs Ok(false)) is being ignored by calling unwrap() directly. Consider handling the Ok(false) case explicitly to distinguish between successful flush and broken pipe scenarios, or document why ignoring this distinction is acceptable.

Suggested change
Self::flush_retry(writer).unwrap();
match Self::flush_retry(writer) {
Ok(true) => {
// Flushed successfully; nothing else to do.
}
Ok(false) => {
// Broken pipe / consumer gone: stop writing and return the result.
return result;
}
Err(e) => {
// Preserve previous behavior of panicking on unexpected I/O errors.
panic!("failed to flush diagnostics output: {e}");
}
}

Copilot uses AI. Check for mistakes.
@codspeed-hq
Copy link

codspeed-hq bot commented Dec 22, 2025

CodSpeed Performance Report

Merging #17238 will not alter performance

Comparing wagenet:handle-blocked-stdout (e5a2985) with main (e08fdf1)1

Summary

✅ 42 untouched
⏩ 3 skipped2

Footnotes

  1. No successful run was found on main (c14afd3) during the generation of this report, so e08fdf1 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

  2. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant