Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…cannot park libuv workers
Plan-file readers (`readPlanFile`, `propose_plan`) opened the plan path with a
plain `open()`. A FIFO with no writer blocks that open in the kernel; on Node
each such read pins a libuv threadpool worker that no abort signal can release,
and a handful of concurrent plan reads exhaust the pool so unrelated fs/DNS/zlib
work stalls process-wide.
Add `ReadFileOptions { requireRegularFile?: boolean }` to `Runtime.readFile`.
Absent, every runtime behaves as before (FIFOs, devices and sockets keep their
native semantics). Present:
- Local (`LocalBaseRuntime`): `open(O_RDONLY|O_NONBLOCK)` → `fstat` on the SAME
descriptor → `handle.createReadStream({ autoClose: true })`; non-regular ⇒
close + `RuntimeError("… is not a regular file", "file_io")`. The type check
is on the acquired inode, never a stat→open pre-check. The pull/abort/cancel
wiring moves into `wrapNodeReadable`, shared by both paths (lazy acquisition
in start()); the default path is behavior-preserving — same
`fs.createReadStream(path)` source and semantics — but not byte-identical code.
- Exec-backed (`RemoteRuntime`, `DevcontainerRuntime` unmounted paths):
`buildRegularFileReadCommand` — advisory `[ -e ] && ! [ -f ]` fail-fast,
`exec 3<P`, `[ -f /dev/fd/3 ]`, then `cat <&3` from the acquired descriptor;
exit 65/66/67 with stderr surfacing through `readFileViaExec`. The precheck
does not close the check→open race; the same-descriptor check does.
Acquisition of a FIFO appearing after the precheck is bounded only by the
exec timeout/abort. Not executed on live SSH/Docker/Devcontainer targets.
- Devcontainer mounted paths forward options to `super`; `multiProjectRuntime`
forwards; `readFileString` passes through. `readPlanFile` (both paths) and
`propose_plan` opt in; no new `ReadPlanResult` field — a non-regular plan
reads as missing, which callers already map to their "no plan file" paths.
Size rationale (~750 changed lines incl. tests): the option must be honoured by
every runtime in the same change so it is never silently dropped, which rules
out splitting exec-backed runtimes behind the local fix; a smaller
`Readable.from(async generator)` source was tried and rejected because Bun
1.3.5 never runs the generator's finally after reader.cancel (one fd leaked per
cancelled read), while this wrapper passes the same fd-baseline tests on Node
and Bun. Tests are real-FS/real-bash/native-Node and were not trimmed.
Tests: `LocalBaseRuntime.readFileRegular.test.ts` (FIFO-no-writer typed error
≤2 s with fd baseline, directory, regular/symlink parity with the default
path, swap-to-FIFO after acquisition keeps the acquired inode, default FIFO
streaming unchanged, abort/cancel leak-free); `execFileIO.regularFile.test.ts`
(real bash: exit 0 content, FIFO±writer → 66, missing → 65);
`tests/ipc/planFileNonRegular.test.ts` (Jest/Node: six concurrent
`getPlanContent` on a writer-less FIFO settle with safe errors while `fs.stat`
and `workspace.list` keep answering; RED on the base). `tests/ipc/fifoRelease.ts`
drains FIFO readers until the owned read attempts settle so a RED run cannot
strand libuv workers or leave parked readers behind.
_Generated with `xum` • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh` • Cost: `$93.46`_
<!-- mux-attribution: model=coder:anthropic/claude-fable-5-1 thinking=xhigh costs=93.46 -->
Change-Id: Ia70fac44170f13fd3744a0202af4016e7053151d
Signed-off-by: Thomas Kosiewski <tk@coder.com>
25ea30d to
0474c6b
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0474c6b19f
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…lar file `AttachmentService.generatePlanFileReference` reads the canonical and legacy plan paths directly (not through `readPlanFile`), so it was left out of the `requireRegularFile` guard: a writer-less FIFO at either path blocked the read and, on Node, pinned a libuv threadpool worker on every post-compaction turn. Pass the existing option on both reads; canonical→legacy fallback semantics are unchanged (a non-regular file fails like a missing plan and falls through). Test (tests/ipc/planFileNonRegular.test.ts, real LocalRuntime): six overlapping `generatePlanFileReference` calls with FIFOs at BOTH the canonical and legacy paths settle to null while an unrelated fs.stat keeps answering; regular canonical and legacy-fallback controls return the expected reference. Both FIFOs are drained concurrently until the owned attempts settle, so a RED run fails on the assertion and leaves the pool usable for the next test. _Generated with `xum` • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh` • Cost: `$93.46`_ <!-- mux-attribution: model=coder:anthropic/claude-fable-5-1 thinking=xhigh costs=93.46 --> Signed-off-by: Thomas Kosiewski <tk@coder.com>
Blocked checkpoint — final head 0f9c8bd
Required CI is still red. Linux E2E job 106848326690 failed in The combined stack is paused: #4317 has four new open findings, and final-head remote UAT has not run because audit attribution is unresolved. No code change, duplicate review trigger, speculative CI rerun or merge is being issued here. The next bounded diagnostic pass must reproduce or explain the E2E failure before the required check can be treated as satisfied. Six external code/security assessments are recorded for this PR; the current-head approvals are preserved. Generated with |
This branch has not been deployed
1 participant
Summary
Prevent a writer-less FIFO at a plan path from parking Node's filesystem workers. A few blocked
open()calls can exhaust the shared libuv pool and stall unrelated filesystem work; aborting a stream does not release a kernel-blocked open.Plan readers now request a regular file. Local reads acquire with
O_NONBLOCK, check the acquired descriptor, and stream from that same descriptor. OrdinaryreadFilecalls retain their existing FIFO/device behavior. Thepropose_planschema and result shape are unchanged; non-regular plans use the existing safe error path.This runtime prerequisite sits above #4337 and below #4317. Native plan-review behavior and its feature-specific integration test stay in #4317.
Review order
ReadFileOptionspropagation through every runtime, including Devcontainer's mounted and exec-backed paths and the multi-project wrapper.The 845 changed lines keep the cross-runtime contract and its tests together. A smaller async-generator adapter was rejected because cancellation leaked file handles under the pinned Bun runtime.
Validation
The unchanged native-Node regression fails on the original production implementation, drains all six owned reads, and then passes the healthy-file control. Regular files, symlinks, default FIFO reads, cancellation, and acquired-inode behavior have focused coverage.
On
0f9c8bd11a6bd3f9bc2bab804e8015b19d00d845:make static-checkpassed (33s), all 98 focused runtime tests passed (4s), and all 20 native-Node integration cases passed (16s: 3 plan-file cases plus 17 payload cases for the combined prerequisite stack). Bun 1.3.5, Node 22.19.0, and frozen dependencies were used. The final regression first reproduced six overlapping post-compaction attachment reads blocking on canonical and legacy FIFOs; both paths now use the existing regular-file guard, while regular-file and legacy-fallback controls remain green. Combined remote UAT, final code/security reviews, and required CI remain merge gates.Platform limits
cat. A FIFO introduced between the precheck and open can still block remote acquisition until the existing exec timeout or abort.Generated with
xum• Model:coder:openai/gpt-6-astra• Thinking:xhigh• Cost:$836.94