fix: cleanup empty folder failure from concurrent upload in file driver

[ferhatelmas]

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

There is a TOCTOU race while cleaning up empty directories in file driver.
Between check and deletion, a concurrent write can write a new version in the folder and error.

What is the new behavior?

Use an atomic delete which does nothing if folder isn't empty.

[Copilot]

Pull request overview

This PR addresses a TOCTOU race in the file storage backend’s empty-directory cleanup logic by switching from recursive removal to an atomic directory removal that fails when the directory is no longer empty, preventing concurrent uploads from being deleted during cleanup.

Changes:

• Replace recursive directory deletion (removePath) with fsp.rmdir for atomic “only-if-empty” directory removal.
• Add tests covering (1) a concurrent file appearing after an “empty” observation and (2) recursive cleanup up to the storage root.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/storage/backend/file.ts	Switches empty-directory cleanup deletion to fsp.rmdir to avoid deleting concurrently-added files.
src/storage/backend/file.test.ts	Adds targeted regression tests for concurrent repopulation and recursive cleanup behavior.

---

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

[coveralls]

Coverage Report for CI Build 31016675660

Coverage increased (+0.03%) to 80.554%

Details

• Coverage increased (+0.03%) from the base build.
• Patch coverage: 6 of 6 lines across 1 file are fully covered (100%).
• 17 coverage regressions across 1 file.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

17 previously-covered lines in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
src/internal/http/agent.ts	17	32.56%

---

Coverage Stats

Relevant Lines:	12663
Covered Lines:	10619
Line Coverage:	83.86%
Relevant Branches:	7516
Covered Branches:	5636
Branch Coverage:	74.99%
Branches in Coverage %:	Yes
Coverage Strength:	434.43 hits per line

---

💛 - Coveralls

[claude]

LGTM — this narrows the TOCTOU window in empty-directory cleanup by swapping a forced recursive removePath for a plain fs.rmdir, which atomically fails if a concurrent upload repopulated the directory (previously it could delete files written mid-race).

What was reviewed: the cleanupEmptyDirectories recursion (bounds check against storage root, parent-dir walk) and the new race-simulation tests, which cover both preservation of a concurrently-written file and correct recursive removal up to the root. Also considered the residual window between isEmptyDirectory and rmdir where a concurrent ensureFile could still hit ENOENT on directory creation/open — this is a narrower, more benign failure (a retryable upload error) than the deleted-data bug being fixed, and is not introduced by this change.

Extended reasoning...

Overview

The PR changes exactly one line of production logic in src/storage/backend/file.ts: cleanupEmptyDirectories now calls fsp.rmdir(dirPath) instead of removePath(dirPath) (which was fs.rm(..., { recursive: true, force: true })). The old code had a real bug: after confirming a directory was empty, a forced recursive delete would blow away any file written into that directory by a concurrent upload between the check and the delete, causing silent data loss. fs.rmdir on a non-empty directory throws ENOTEMPTY instead of deleting anything, so the race window collapses to "cleanup silently no-ops" rather than "cleanup destroys a fresh upload." The PR also adds a dedicated test file section with a subclassed FileBackend that deterministically injects a write between the empty-check and the delete, verifying both the race-safety and that recursive parent cleanup still works.

Security risks

None. This is a local filesystem cleanup path with no auth/crypto/permission implications, and the change only makes deletion stricter (less likely to remove data), not more permissive.

Level of scrutiny

Concurrency-sensitive fix in a production storage backend, but the change itself is small, targeted, and directly replaces a dangerous forced-recursive-delete with a safe atomic primitive. The surrounding cleanup call is already best-effort (wrapped in a try/catch that swallows errors so as not to affect the main delete/upload operation), so a rare residual ENOTEMPTY/ENOENT is not novel or newly introduced — it is strictly less risky than the prior behavior.

Other factors

New tests specifically simulate the race the PR description calls out and assert the fix holds, which is good evidence the change does what it claims. No CODEOWNER-restricted paths, no design decisions requiring product input, and the diff is minimal and mechanical (test additions plus a one-line production change).

[ferhatelmas]

@claude review always

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/storage/backend/file.ts:685

• cleanupEmptyDirectories returns early when pathExists(dirPath) is false, so if the directory was deleted before this call starts (or before the existence check), parent directories will not be cleaned even though they may now be empty. This undermines the intent of continuing parent cleanup for concurrent removals. Also, fsp.rmdir is deprecated; the codebase already uses fs.rm elsewhere (removePath).

        try {
          // Fails atomically if a concurrent upload repopulated the directory.
          await fsp.rmdir(dirPath)
        } catch (error) {
          // If another cleanup removed this directory, its parents may still need cleanup.

src/storage/backend/file.test.ts:353

• The test uses fsp.rmdir, which is deprecated and can emit deprecation warnings on newer Node versions. Prefer fsp.rm with recursive: false for removing an empty directory.

      if (isEmpty && this.removeBeforeRmdir) {
        this.removeBeforeRmdir = false
        await fsp.rmdir(dirPath)
      }

[claude]

LGTM — the atomic fsp.rmdir correctly fixes the TOCTOU race described in the PR, and the new tests exercise the concurrent-write and concurrent-cleanup scenarios well. The one inline finding is a narrower, pre-existing race that this change already narrows rather than introduces, so it doesn't need to block this fix.

Extended reasoning...

Overview

This PR changes cleanupEmptyDirectories in src/storage/backend/file.ts to use an atomic fsp.rmdir(dirPath) instead of the previous check-then-removePath sequence, closing the TOCTOU window where a concurrent upload could write a new version into a directory just before it was force-removed. It also adds explicit ENOENT handling so that if a concurrent cleanup already removed the directory, the recursion still continues up to parent directories instead of aborting silently. Three new tests in file.test.ts cover: (1) a file written after isEmptyDirectory observes empty is preserved, (2) normal recursive cleanup up to the storage root, and (3) recursion continuing correctly when the target directory was removed concurrently.

Security risks

None. This is a filesystem-only local-disk backend concurrency fix; no user input, auth, or crypto paths are touched.

Level of scrutiny

Correctness-focused, moderate scrutiny is warranted since this is concurrency-sensitive code, but the change itself is small, self-contained, and well-tested. The bug-hunting pass found one residual race (a narrower TOCTOU window between a concurrent ensureFile's mkdir and open calls) but confirmed it is pre-existing (the old removePath-based implementation had the identical gap) and that this PR narrows rather than widens or introduces it — the bug report itself notes it shouldn't block this PR.

Other factors

The Copilot review comment on this PR raised a concern about ENOENT aborting parent-directory cleanup recursion, but the current code already handles this explicitly (the catch block only returns early for non-ENOENT errors, so ENOENT falls through to the recursive parent-cleanup call) — this is also verified by the new 'continues cleaning parents when the target directory was removed concurrently' test. Test coverage for the new logic is solid and the coveralls report shows no regressions.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/storage/backend/file.ts:672

• The commented-out debug statement references e.message, but the surrounding catch clause does not bind an error variable. If someone uncomments this log for troubleshooting, it will fail to compile.

    } catch {
      // Ignore errors during cleanup to not affect main operations
      // Could be permission issues, concurrent access, directory not empty due to race conditions, etc.
      // Optional: Log for debugging purposes (uncomment if needed)
      // console.debug('Directory cleanup failed:', dirPath, e.message)

[claude]

No new bugs found in this pass. This is a concurrency fix to production file-storage cleanup logic (TOCTOU race handling), so I'd like a human to sign off on the atomic-rmdir approach and its ENOENT/ENOTEMPTY branching before merge.

What was reviewed: the rewritten cleanupEmptyDirectories atomic-rmdir logic and its recursive parent-cleanup behavior, the three new regression tests (repopulated directory, recursive cleanup to root, already-absent target directory), and the removal of the old isEmptyDirectory TOCTOU check. Copilot's concern about ENOENT aborting parent cleanup is addressed in the current code (explicit code !== 'ENOENT' branch) and covered by the 'continues cleaning parents when the target directory is already absent' test.

Extended reasoning...

Overview

This PR replaces a check-then-act (isEmptyDirectory + removePath) TOCTOU pattern in FileBackend.cleanupEmptyDirectories with an atomic fsp.rmdir, which naturally fails with ENOTEMPTY if a concurrent upload has repopulated the directory. It also adds explicit ENOENT handling so that if the directory was already removed by a concurrent cleanup, parent-directory cleanup still proceeds recursively. Three new tests cover: a directory repopulated by a concurrent upload (preserved), full recursive cleanup up to the storage root, and cleanup continuing past an already-absent target directory. Scope is limited to src/storage/backend/file.ts and its test file.

Security risks

None identified. This is local filesystem cleanup logic with no new external inputs, auth, or crypto surface. Path resolution still goes through the existing resolveSecurePath/traversal-protection code, which this PR does not touch.

Level of scrutiny

Medium-high. While the diff itself is small and the intent (atomic rmdir) is a sound general pattern, it modifies concurrency-sensitive logic in the file storage backend's delete/cleanup path — a production-critical code path where subtle interleavings are easy to miss and hard to test exhaustively. A prior automated pass on this PR (visible in the timeline) identified a narrower, pre-existing race in a related upload/cleanup interleaving; that finding was judged non-blocking (pre-existing, not worsened by this change), but its existence is itself a signal that this area rewards a careful human look rather than a rubber stamp.

Other factors

Test coverage for the new logic is solid (3 new targeted tests, 100% patch coverage per Coveralls) and directly exercises the scenario called out by Copilot's review comment. No outstanding unaddressed review comments — the ENOENT-handling concern from Copilot appears to already be reflected in the code. Given the combination of production-critical concurrency logic and the subtlety already surfaced during review, I'd rather a human confirm the approach than approve automatically.