fs: auto-close Dir async iterator on exit

[robobun]

What

Node's fs.Dir async iterator closes the directory handle when it finishes — naturally, via break, or via a throw. Bun's iterator never called close(), so the handle leaked until the Dir was GC'd and a subsequent dir.close() succeeded instead of throwing ERR_DIR_CLOSED.

Repro (fixed)

import { opendir } from "node:fs/promises";

const dir = await opendir(".");
for await (const entry of dir) {
  console.log(entry.name);
  break;
}
await dir.close();  // before: returns; after: throws ERR_DIR_CLOSED (matches Node)

Fix

Wrap the yield* in try/finally and call closeSync() in the finally block, swallowing ERR_DIR_CLOSED in case the user closed the handle manually — same shape as Node's lib/internal/fs/dir.js iterator.

Verification

test/regression/issue/28894.test.ts covers:

• close-on-break
• close-on-natural-completion
• close-on-thrown-error
• manual-close-inside-loop is a no-op (swallowed)

All four fail on USE_SYSTEM_BUN=1 (well, 3 — the fourth only asserts no throw) and pass on bun bd.

Fixes #28894

[robobun]

^{Updated 10:20 AM PT - Apr 5th, 2026}

❌ Your commit 3a925d57 has 4 failures in Build #43853 (All Failures):

• test/js/bun/webview/webview.test.ts - timeout on 🍎 14 aarch64
• test/js/bun/webview/webview.test.ts - timeout on 🍎 13 aarch64
• test/js/bun/webview/webview.test.ts - timeout on 🍎 13 x64
• test/js/bun/webview/webview.test.ts - timeout on 🍎 14 x64
• test/js/node/http/node-http-backpressure-max.test.ts - code 1 on 🍎 13 x64
• test/js/third_party/@azure/service-bus/azure-service-bus.test.ts - SIGILL on 🐧 13 x64-asan
• test/js/bun/webview/webview-chrome.test.ts - code 1 on 🍎 14 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 28895

That installs a local version of the PR into your bun-28895 executable, so you can run:

bun-28895 --bun

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f1ad9a8f-3c41-4d99-aad2-306a06ca326f

📥 Commits

Reviewing files that changed from the base of the PR and between 38a195a and 3a925d5.

📒 Files selected for processing (2)

• src/js/node/fs.ts
• test/regression/issue/28894.test.ts

---

Walkthrough

Reworks Dir's async *[Symbol.asyncIterator] to enforce deterministic directory-handle cleanup by wrapping readdir/iteration in a try/finally and unconditionally calling this.closeSync() in finally. The iterator now throws $ERR_DIR_CLOSED() if the handle is already closed and allows ERR_DIR_CLOSED from a manual close during iteration to propagate. Adds five regression tests covering iterator exit scenarios and handle lifecycle assertions.

Changes

Cohort / File(s)	Summary
Dir async iterator src/js/node/fs.ts	Replaced lazy/cached yield behavior with a try/finally around fs.readdir() / yield*, unconditionally calling this.closeSync() in finally. Now throws $ERR_DIR_CLOSED() if opened handle is already closed and lets ERR_DIR_CLOSED from manual close propagate out of the iterator.
Regression tests test/regression/issue/28894.test.ts	Added five bun:test regression cases validating node:fs/promises Dir async iterator lifecycle: natural completion, early break (auto-close), re-iteration after auto-close (rejects), thrown error inside loop (original error propagates, handle closed), and manual close inside loop (iterator rejects with Directory handle was closed).

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title 'fs: auto-close Dir async iterator on exit' clearly and concisely summarizes the main change: automatic cleanup of directory handles when async iteration ends.
Description check	✅ Passed	The PR description fully addresses both required template sections: it clearly explains what the fix does and includes comprehensive verification details showing how the code was tested.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/js/node/fs.ts`:
- Around line 1139-1141: In the iterator cleanup where this.closeSync() is
called, change the empty catch to capture the thrown error and only swallow it
when the error code equals 'ERR_DIR_CLOSED'; for any other error rethrow it.
Locate the call to this.closeSync() (the iterator cleanup block) and replace the
bare catch with one that inspects the caught err (e.g., err && err.code ===
'ERR_DIR_CLOSED') to ignore that specific case and rethrow otherwise.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ea04fea7-1119-4ca4-b97f-b15dd0d9ab6f

📥 Commits

Reviewing files that changed from the base of the PR and between 6ecf467 and 38a195a.

📒 Files selected for processing (2)

• src/js/node/fs.ts
• test/regression/issue/28894.test.ts

[claude]

Previous concerns addressed: guard added, dead imports removed, errors propagate correctly. LGTM.

Extended reasoning...

Overview

This PR fixes fs.Dir's async iterator to auto-close the directory handle on exit (natural, break, or throw), matching Node.js behavior. Changes are confined to the Dir class in src/js/node/fs.ts and a new regression test file.

Security risks

None. This is a Node.js compatibility fix for directory handle lifecycle — no auth, crypto, or permission-sensitive code paths involved.

Level of scrutiny

Low. The change is small, well-scoped, and follows an established pattern already used by read()/readSync() in the same class. The fix mirrors Node's own lib/internal/fs/dir.js implementation.

Prior review concerns (resolved)

My previous inline comments flagged two issues that have since been addressed in follow-up commits:

1. Missing closed-handle guard at the start of the async iterator — now present: if (this.#handle < 0) throw $ERR_DIR_CLOSED();
2. Dead imports in the test file — removed; the file now only imports from node:fs/promises

Additionally, coderabbitai's concern about silently swallowing all close errors is moot: the current code calls this.closeSync() directly in finally without any try/catch, so all errors propagate. A dedicated test verifies this behavior.

Other factors

Five test cases cover all relevant scenarios (break, natural completion, thrown error, second iteration, manual close inside loop). The implementation is consistent with the existing guard pattern in the class.

[robobun]

Heads up: this PR fixes auto-close on exit (#28894), but it does not cover the closely related #31879, where a `close()` between `next()` pulls should make the next pull throw `ERR_DIR_CLOSED`.

The reason is that the iterator still does `yield* entries` over a snapshot array and only checks `this.#handle` once, before the first pull. After the first `next()`, later pulls come straight from the materialized array without re-checking the handle. Verified against this exact build:

const dir = await fs.opendir(d);
const iter = dir[Symbol.asyncIterator]();
await iter.next();
await dir.close();
await iter.next(); // Node: throws ERR_DIR_CLOSED. With this PR: yields "c.txt"

The minimal change that covers both issues is to drive the loop through `this.read()` (which already throws `ERR_DIR_CLOSED` on a closed handle) instead of `yield*`:

async *[Symbol.asyncIterator]() {
  try {
    let entry;
    while ((entry = await this.read()) !== null) {
      yield entry;
    }
  } finally {
    if (this.#handle >= 0) this.close();
  }
}

#31830 already takes this approach (per-pull `read()` plus auto-close in `finally`) and ports the Node `opendir`/`Dir` tests that assert the close-between-pulls behavior, so that PR covers both #28894 and #31879.