Skip to content

fix(process): honor cancellation at bounded file read completion - #8908

Merged
morluto merged 1 commit into
morluto:mainfrom
Afloat16:fix/bounded-file-read-cancellation
Nov 6, 2026
Merged

morluto merged 1 commit into
morluto:mainfrom
Afloat16:fix/bounded-file-read-cancellation

Conversation

@Afloat16

@Afloat16 Afloat16 commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Problem

readBoundedFileBytes returns an empty success for an already-aborted zero-byte expected read. It also returns success or overflow when cancellation arrives during the final asynchronous read, because cancellation is checked only before I/O.

Changes

Check the caller's abort signal after option validation and after each successful read, before interpreting EOF, expected-size completion, or overflow. Preserve the caller's exact abort reason and existing cancellationFailure mapping. The caller continues to own and close the file handle.

This fixes the shared file-read boundary without changing the admitted byte limit or expected-size read semantics.

Validation

  • tests/boundary/filesystem/boundedFileBytes.test.ts: 10 passed on Node 24.22.1 / Vitest 5.0.3, with a focused external test configuration.
  • Real temporary-file cases cover an already-aborted zero-byte read and cancellation during EOF, early EOF, expected-size completion, and overflow, alongside exact-limit and growing-file controls.
  • Re-running against the unmodified source: 5 failed, 5 passed.
  • Targeted TypeScript 5.9.3 checking, repository-pinned oxfmt 0.72.0 formatting, and git diff --check passed.

The full project checks and provider workflows were not run.

Author readiness

Human owner: @Afloat16

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-11-04T14:03:17.122390Z
We couldn’t complete this review. Comment @codex review to try again.
db28041 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

@greptile-apps

greptile-apps Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5 Tier: plus

[Medium impact] This PR appears safe to merge; no actionable issues were found.

Summary

readBoundedFileBytes now checks cancellation before zero-byte completion and after each successful read, before returning bytes or reporting overflow.

  • Bounded file reads stop when cancellation arrives during the read.

Reviews (1) · Last reviewed commit: "fix(process): honor cancellation at boun..." · Reviewed by Greptile

Afloat16 commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

Both CI and author readiness are now passing on current head db28041deff548c9afdefdbc1cc30f322863a009. The regression evidence and validation scope are recorded in the PR description. Could a maintainer review this for merge?

@morluto
morluto merged commit b153ba8 into morluto:main Nov 6, 2026
47 of 49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants