From 2589a68155324528aa12c78e41df375dee1b7061 Mon Sep 17 00:00:00 2001 From: Alexey Dubovskoy Date: Mon, 18 May 2026 17:49:17 +0100 Subject: [PATCH 1/7] docs: spec for empty-folder cleanup on download Addresses issue #21: parent directories left behind on receiving devices when a synced file is deleted. Design walks parents upward from Chunker::delete using strict-empty semantics. --- .../2026-05-18-empty-folder-cleanup-design.md | 147 ++++++++++++++++++ 1 file changed, 147 insertions(+) create mode 100644 docs/superpowers/specs/2026-05-18-empty-folder-cleanup-design.md diff --git a/docs/superpowers/specs/2026-05-18-empty-folder-cleanup-design.md b/docs/superpowers/specs/2026-05-18-empty-folder-cleanup-design.md new file mode 100644 index 0000000..7a3b54b --- /dev/null +++ b/docs/superpowers/specs/2026-05-18-empty-folder-cleanup-design.md @@ -0,0 +1,147 @@ +# Empty Folder Cleanup on Download + +GitHub issue: [#21 — folder isn't deleted from device](https://github.com/cooklang/cooklang-sync/issues/21). + +## Problem + +When the user deletes a file on one device, the sync server propagates a delete +event. On every other device, the syncer removes the file but leaves the +parent directory in place. The result: empty folders accumulate on receiving +devices and surface in the UI as ghost folders the user cannot easily clean up +themselves (mobile file managers often hide empty-folder removal behind +multiple taps). + +The bug lives in `Chunker::delete` (`client/src/chunker.rs:147`) which already +carries a `// TODO delete folders up too if empty` marker. The function is +invoked by the download loop in `Syncer::check_download_once` +(`client/src/syncer.rs:309-315`) when a remote delete record arrives. + +## Goals + +- After a synced delete, remove any parent directories that become empty as a + result, up to (but never including) the storage root. +- Keep the change scoped to the function flagged by the existing TODO. +- Do not regress: a sibling file in the same directory must keep its parent. + +## Non-goals + +- Cleaning up empty folders that became empty by some other path (e.g. user + manually deleted files outside the sync flow). The indexer is responsible + for tracking the user's view of the directory tree; this fix is targeted at + the sync-driven case described in the issue. +- Treating OS-generated files (`.DS_Store`, `Thumbs.db`, `.localized`) as + "doesn't count". We use strict-empty semantics: a directory containing any + entry is left alone. On the actual sync targets (iOS, Android), those + files are not auto-created, so the strict rule is the right default. If + desktop users report leftover folders caused by `.DS_Store`, revisit. +- Bulk cleanup on upload, indexer, or startup. Out of scope. + +## Design + +### Behavior + +Modify `Chunker::delete(&mut self, path: &str)`: + +1. Remove the file as today (`fs::remove_file`). Propagate the existing error + as `SyncError::from_io_error`. +2. Walk parent directories upward starting from `full_path.parent()`: + - If the candidate directory equals `self.base_path`, stop. The storage + root is never removed. + - Otherwise, attempt `fs::remove_dir(candidate)`. `remove_dir` only + succeeds on empty directories, which is exactly our strict-empty rule. + - On success: continue with `candidate.parent()`. + - On any error: stop walking. Do not propagate. The dominant cause is + `ENOTEMPTY` (the directory has remaining siblings) which is the normal + terminating condition, not a failure. Other errors (permissions, a + concurrent process re-creating the directory, etc.) are not worth + failing a download cycle over; the parent will be retried on the next + delete that lands in that subtree. + +### Sketch + +```rust +pub async fn delete(&mut self, path: &str) -> Result<()> { + trace!("deleting {:?}", path); + let full_path = self.full_path(path); + + fs::remove_file(&full_path) + .await + .map_err(|e| SyncError::from_io_error(path, e))?; + + let mut parent = full_path.parent(); + while let Some(dir) = parent { + if dir == self.base_path { + break; + } + if fs::remove_dir(dir).await.is_err() { + break; + } + parent = dir.parent(); + } + + Ok(()) +} +``` + +The TODO comment on the prior body is removed in the same edit. + +### Why this approach + +Three options were considered: + +1. **Walk up inside `Chunker::delete`.** (chosen) Minimal, self-contained, + matches the existing TODO. Each delete pays one `remove_dir` per ancestor + level, which is one cheap syscall — ENOTEMPTY is fast. +2. **Post-pass in `check_download_once`.** Track directories touched during + the deletes loop, then sweep them once after the loop. Slightly less + syscalls when many files in the same folder are deleted in a single + batch, but adds plumbing (set of touched paths, second loop) for a + negligible saving. +3. **Full tree sweep at end of each download cycle.** Highest I/O cost, most + thorough. Overkill for a targeted fix. + +Option 1 wins on simplicity-per-risk and stays inside the unit the bug +report names. + +## Error handling + +| Failure mode | Behavior | +|-------------------------------------------------------|------------------------------------------------| +| `remove_file` fails | Propagate as today (`SyncError::from_io_error`)| +| `remove_dir` fails (ENOTEMPTY, permission, missing) | Stop walking; return `Ok(())` | +| Parent chain reaches `base_path` | Stop; never remove the storage root | +| `parent()` returns `None` | Stop (loop terminates) | + +The function still returns `Ok(())` whenever the file itself was removed, +regardless of cleanup outcome. This preserves the contract callers in +`syncer.rs` rely on — a delete event is "handled" once the file is gone. + +## Tests + +Add four unit tests to the existing `chunker.rs` test module. Each uses a +`TempDir` as the storage base, mirroring the style of +`delete_removes_file_from_storage_dir`. + +1. `delete_removes_empty_parent_directories` — write `a/b/c.cook` under a + fresh storage dir, call `delete("a/b/c.cook")`, assert both `a/b/` and + `a/` are gone. +2. `delete_stops_at_first_non_empty_ancestor` — write `a/b/c.cook` and + `a/sibling.cook`, call `delete("a/b/c.cook")`, assert `a/b/` is gone + but `a/` and `a/sibling.cook` survive. +3. `delete_does_not_remove_storage_root` — write a single file at the + storage root, call `delete` on it, assert the storage root itself + still exists. +4. `delete_leaves_sibling_files_in_same_directory` — write `a/x.cook` and + `a/y.cook`, call `delete("a/x.cook")`, assert `a/` and `a/y.cook` are + still present. + +No integration test is added; the change is purely local filesystem +behavior and the existing chunker tests cover the wiring. + +## Out of scope follow-ups + +- Indexer-side detection of folders that became empty through means other + than the sync flow. If user reports come in, the right place is a + separate indexer pass, not this function. +- Treating known OS-generated hidden files as "doesn't count" for + emptiness checks. Revisit if reported. From 4d276ec5d0c52be97832b76a27343dc7683a7fb3 Mon Sep 17 00:00:00 2001 From: Alexey Dubovskoy Date: Mon, 18 May 2026 17:54:10 +0100 Subject: [PATCH 2/7] docs: implementation plan for empty-folder cleanup on download --- .../plans/2026-05-18-empty-folder-cleanup.md | 345 ++++++++++++++++++ 1 file changed, 345 insertions(+) create mode 100644 docs/superpowers/plans/2026-05-18-empty-folder-cleanup.md diff --git a/docs/superpowers/plans/2026-05-18-empty-folder-cleanup.md b/docs/superpowers/plans/2026-05-18-empty-folder-cleanup.md new file mode 100644 index 0000000..8b626f9 --- /dev/null +++ b/docs/superpowers/plans/2026-05-18-empty-folder-cleanup.md @@ -0,0 +1,345 @@ +# Empty Folder Cleanup on Download Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** When the syncer downloads a delete event, also remove any parent directories that became empty as a result — fixing GitHub issue #21 where folders linger on receiving devices after their last file is deleted. + +**Architecture:** Single-function change to `Chunker::delete` in `client/src/chunker.rs`. After `fs::remove_file`, walk up `parent()` chain calling `fs::remove_dir` — which natively fails on non-empty directories, giving us strict-empty semantics. Stop at `self.base_path` (never touch the storage root) or on any error. + +**Tech Stack:** Rust, tokio (async fs), tempfile (test helper). + +**Spec:** `docs/superpowers/specs/2026-05-18-empty-folder-cleanup-design.md` + +--- + +## File Structure + +- Modify: `client/src/chunker.rs:147-157` — replace the body of `Chunker::delete` (keeping its signature and behavior on `remove_file` failure). +- Modify: `client/src/chunker.rs` test module (currently ends at line 648) — add four new `#[tokio::test]` functions, mirroring the style of `delete_removes_file_from_storage_dir` (lines 577-598). + +No new files. No caller changes — `syncer.rs:313-315` continues to call `chunker.delete(&d.path)` unchanged. + +--- + +## Task 1: Test — empty parent directories get removed + +**Files:** +- Modify (test): `client/src/chunker.rs` (append to `mod tests`) + +- [ ] **Step 1: Write the failing test** + +Append inside the `mod tests { ... }` block in `client/src/chunker.rs` (just before the closing `}` of the module, which is currently at the end of the file): + +```rust +#[tokio::test] +async fn delete_removes_empty_parent_directories() { + // Issue #21: when the syncer deletes the only file inside a + // nested directory, those now-empty parents must also be removed. + // Otherwise users see ghost folders accumulate on receiving devices. + let temp = tempfile::TempDir::new().unwrap(); + let cache = InMemoryCache::new(100, 10_000); + let mut chunker = Chunker::new(cache, temp.path().to_path_buf()); + + let nested_dir = temp.path().join("a").join("b"); + tokio::fs::create_dir_all(&nested_dir).await.unwrap(); + tokio::fs::write(nested_dir.join("c.cook"), b"eggs\n") + .await + .unwrap(); + + chunker + .delete("a/b/c.cook") + .await + .expect("delete should succeed"); + + assert!( + !temp.path().join("a/b/c.cook").exists(), + "file should be removed" + ); + assert!( + !temp.path().join("a/b").exists(), + "empty intermediate directory should be removed" + ); + assert!( + !temp.path().join("a").exists(), + "empty grandparent directory should be removed" + ); + assert!( + temp.path().exists(), + "storage root must never be removed" + ); +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test -p cooklang-sync-client --lib chunker::tests::delete_removes_empty_parent_directories` + +Expected: FAIL — the assertion `!temp.path().join("a/b").exists()` will fail because the current `Chunker::delete` only removes the file, leaving `a/b/` behind. + +- [ ] **Step 3: Implement the cleanup walk** + +In `client/src/chunker.rs`, replace the entire current body of `pub async fn delete` (lines 147-157): + +```rust +pub async fn delete(&mut self, path: &str) -> Result<()> { + trace!("deleting {:?}", path); + let full_path = self.full_path(path); + + fs::remove_file(&full_path) + .await + .map_err(|e| SyncError::from_io_error(path, e))?; + + // Walk parents upward and remove empty directories. `remove_dir` + // only succeeds on empty directories, which gives us strict-empty + // semantics without manually counting entries. Stop at the storage + // root (never remove it) and on any error (most commonly ENOTEMPTY + // when a sibling file is present — that's the normal terminating + // condition, not a failure to propagate). + let mut parent = full_path.parent(); + while let Some(dir) = parent { + if dir == self.base_path { + break; + } + if fs::remove_dir(dir).await.is_err() { + break; + } + parent = dir.parent(); + } + + Ok(()) +} +``` + +Note: the existing `// TODO delete folders up too if empty` comment from line 151 is removed by this replacement. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test -p cooklang-sync-client --lib chunker::tests::delete_removes_empty_parent_directories` + +Expected: PASS. + +- [ ] **Step 5: Run existing chunker tests to verify no regression** + +Run: `cargo test -p cooklang-sync-client --lib chunker::tests` + +Expected: all chunker tests pass, including the existing `delete_removes_file_from_storage_dir` and `test_chunker_delete`. + +- [ ] **Step 6: Commit** + +```bash +git add client/src/chunker.rs +git commit -m "$(cat <<'EOF' +fix(chunker): remove empty parent dirs on delete + +Closes #21. When the syncer downloads a delete event, walk up from +the removed file and remove empty parent directories using +fs::remove_dir (which natively rejects non-empty dirs, giving us +strict-empty semantics). Stop at the storage root and on any error +so a sibling file or permissions issue never fails the download. +EOF +)" +``` + +--- + +## Task 2: Test — stop at first non-empty ancestor + +**Files:** +- Modify (test): `client/src/chunker.rs` (append to `mod tests`) + +- [ ] **Step 1: Write the test** + +Append inside the `mod tests { ... }` block in `client/src/chunker.rs`: + +```rust +#[tokio::test] +async fn delete_stops_at_first_non_empty_ancestor() { + // If `a/` still has a sibling file after we delete `a/b/c.cook`, + // we must remove `a/b/` (now empty) but leave `a/` alone. + // remove_dir naturally enforces this via ENOTEMPTY; this test + // pins that behavior so a future refactor can't accidentally + // implement recursive deletion. + let temp = tempfile::TempDir::new().unwrap(); + let cache = InMemoryCache::new(100, 10_000); + let mut chunker = Chunker::new(cache, temp.path().to_path_buf()); + + let nested_dir = temp.path().join("a").join("b"); + tokio::fs::create_dir_all(&nested_dir).await.unwrap(); + tokio::fs::write(nested_dir.join("c.cook"), b"eggs\n") + .await + .unwrap(); + tokio::fs::write(temp.path().join("a").join("sibling.cook"), b"flour\n") + .await + .unwrap(); + + chunker + .delete("a/b/c.cook") + .await + .expect("delete should succeed"); + + assert!( + !temp.path().join("a/b/c.cook").exists(), + "target file should be removed" + ); + assert!( + !temp.path().join("a/b").exists(), + "empty intermediate directory should be removed" + ); + assert!( + temp.path().join("a").exists(), + "non-empty ancestor must be preserved" + ); + assert!( + temp.path().join("a/sibling.cook").exists(), + "sibling file must be preserved" + ); +} +``` + +- [ ] **Step 2: Run test to verify it passes** + +Run: `cargo test -p cooklang-sync-client --lib chunker::tests::delete_stops_at_first_non_empty_ancestor` + +Expected: PASS (Task 1 already implemented the behavior). + +- [ ] **Step 3: Commit** + +```bash +git add client/src/chunker.rs +git commit -m "test(chunker): pin stop-at-non-empty-ancestor behavior" +``` + +--- + +## Task 3: Test — storage root is never removed + +**Files:** +- Modify (test): `client/src/chunker.rs` (append to `mod tests`) + +- [ ] **Step 1: Write the test** + +Append inside the `mod tests { ... }` block in `client/src/chunker.rs`: + +```rust +#[tokio::test] +async fn delete_does_not_remove_storage_root() { + // Even when the very last file in the storage root is deleted, + // the root itself must survive — otherwise the next download + // cycle has nowhere to write to, and the indexer would crash + // walking a missing directory. + let temp = tempfile::TempDir::new().unwrap(); + let cache = InMemoryCache::new(100, 10_000); + let mut chunker = Chunker::new(cache, temp.path().to_path_buf()); + + tokio::fs::write(temp.path().join("only.cook"), b"sugar\n") + .await + .unwrap(); + + chunker + .delete("only.cook") + .await + .expect("delete should succeed"); + + assert!( + !temp.path().join("only.cook").exists(), + "file should be removed" + ); + assert!( + temp.path().exists(), + "storage root must never be removed" + ); + assert!( + temp.path().is_dir(), + "storage root must still be a directory" + ); +} +``` + +- [ ] **Step 2: Run test to verify it passes** + +Run: `cargo test -p cooklang-sync-client --lib chunker::tests::delete_does_not_remove_storage_root` + +Expected: PASS. + +- [ ] **Step 3: Commit** + +```bash +git add client/src/chunker.rs +git commit -m "test(chunker): pin storage root preservation on delete" +``` + +--- + +## Task 4: Test — sibling files in same directory are preserved + +**Files:** +- Modify (test): `client/src/chunker.rs` (append to `mod tests`) + +- [ ] **Step 1: Write the test** + +Append inside the `mod tests { ... }` block in `client/src/chunker.rs`: + +```rust +#[tokio::test] +async fn delete_leaves_sibling_files_in_same_directory() { + // Deleting `a/x.cook` when `a/y.cook` also exists must leave + // both `a/` and `a/y.cook` intact. This is the "obvious" case + // but worth pinning — a naive implementation that always + // removes the immediate parent would break it. + let temp = tempfile::TempDir::new().unwrap(); + let cache = InMemoryCache::new(100, 10_000); + let mut chunker = Chunker::new(cache, temp.path().to_path_buf()); + + let dir = temp.path().join("a"); + tokio::fs::create_dir_all(&dir).await.unwrap(); + tokio::fs::write(dir.join("x.cook"), b"salt\n").await.unwrap(); + tokio::fs::write(dir.join("y.cook"), b"pepper\n").await.unwrap(); + + chunker + .delete("a/x.cook") + .await + .expect("delete should succeed"); + + assert!( + !temp.path().join("a/x.cook").exists(), + "target file should be removed" + ); + assert!( + temp.path().join("a").exists(), + "directory with remaining files must be preserved" + ); + assert!( + temp.path().join("a/y.cook").exists(), + "sibling file must be preserved" + ); +} +``` + +- [ ] **Step 2: Run test to verify it passes** + +Run: `cargo test -p cooklang-sync-client --lib chunker::tests::delete_leaves_sibling_files_in_same_directory` + +Expected: PASS. + +- [ ] **Step 3: Run the entire client test suite as a final regression check** + +Run: `cargo test -p cooklang-sync-client` + +Expected: all tests pass. Pay attention to indexer tests, especially the dot-directory tests added in #20 — those should be unaffected since they don't exercise `Chunker::delete`. + +- [ ] **Step 4: Commit** + +```bash +git add client/src/chunker.rs +git commit -m "test(chunker): pin sibling preservation on delete" +``` + +--- + +## Notes for the implementer + +- The four tests are intentionally split across four tasks (one commit each) for clean review. Tasks 2–4 will pass immediately after Task 1's implementation lands; that's expected — they exist to pin behavior, not to drive new code. +- `fs` in `chunker.rs` is `tokio::fs` (already imported at line 5). `fs::remove_dir` returns `io::Result<()>`; we discard the error variant on purpose (see the comment in the implementation). +- `self.base_path` is a `PathBuf` constructed from whatever the caller passed to `Chunker::new`. `full_path()` builds the file path by cloning `base_path` and `push`-ing the relative `path`, so `parent()` walking from `full_path` will pass through exactly equal `PathBuf` values as it climbs — `dir == self.base_path` is a sound stop condition. +- Don't change `syncer.rs`. The call site at `syncer.rs:313-315` is correct as-is. +- Don't try to be clever with `tokio::fs::read_dir` to count entries before calling `remove_dir`. `remove_dir`'s built-in ENOTEMPTY check is atomic with the actual removal; a separate count introduces a race window. From 5943be135ebf2490fcfc12dda3be9f1bb7cd8938 Mon Sep 17 00:00:00 2001 From: Alexey Dubovskoy Date: Mon, 18 May 2026 17:58:21 +0100 Subject: [PATCH 3/7] fix(chunker): remove empty parent dirs on delete Closes #21. When the syncer downloads a delete event, walk up from the removed file and remove empty parent directories using fs::remove_dir (which natively rejects non-empty dirs, giving us strict-empty semantics). Stop at the storage root and on any error so a sibling file or permissions issue never fails the download. --- client/src/chunker.rs | 58 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/client/src/chunker.rs b/client/src/chunker.rs index 3b3ff1b..3ee593b 100644 --- a/client/src/chunker.rs +++ b/client/src/chunker.rs @@ -148,11 +148,27 @@ impl Chunker { trace!("deleting {:?}", path); let full_path = self.full_path(path); - // TODO delete folders up too if empty - fs::remove_file(full_path) + fs::remove_file(&full_path) .await .map_err(|e| SyncError::from_io_error(path, e))?; + // Walk parents upward and remove empty directories. `remove_dir` + // only succeeds on empty directories, which gives us strict-empty + // semantics without manually counting entries. Stop at the storage + // root (never remove it) and on any error (most commonly ENOTEMPTY + // when a sibling file is present — that's the normal terminating + // condition, not a failure to propagate). + let mut parent = full_path.parent(); + while let Some(dir) = parent { + if dir == self.base_path { + break; + } + if fs::remove_dir(dir).await.is_err() { + break; + } + parent = dir.parent(); + } + Ok(()) } @@ -646,4 +662,42 @@ mod tests { "three newline-terminated lines should yield three text chunks; got {hashes:?}" ); } + + #[tokio::test] + async fn delete_removes_empty_parent_directories() { + // Issue #21: when the syncer deletes the only file inside a + // nested directory, those now-empty parents must also be removed. + // Otherwise users see ghost folders accumulate on receiving devices. + let temp = tempfile::TempDir::new().unwrap(); + let cache = InMemoryCache::new(100, 10_000); + let mut chunker = Chunker::new(cache, temp.path().to_path_buf()); + + let nested_dir = temp.path().join("a").join("b"); + tokio::fs::create_dir_all(&nested_dir).await.unwrap(); + tokio::fs::write(nested_dir.join("c.cook"), b"eggs\n") + .await + .unwrap(); + + chunker + .delete("a/b/c.cook") + .await + .expect("delete should succeed"); + + assert!( + !temp.path().join("a/b/c.cook").exists(), + "file should be removed" + ); + assert!( + !temp.path().join("a/b").exists(), + "empty intermediate directory should be removed" + ); + assert!( + !temp.path().join("a").exists(), + "empty grandparent directory should be removed" + ); + assert!( + temp.path().exists(), + "storage root must never be removed" + ); + } } From f577c9725332920ab8b7dd10fd503bc00c6f6a03 Mon Sep 17 00:00:00 2001 From: Alexey Dubovskoy Date: Mon, 18 May 2026 18:04:08 +0100 Subject: [PATCH 4/7] test(chunker): pin stop-at-non-empty-ancestor behavior --- client/src/chunker.rs | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/client/src/chunker.rs b/client/src/chunker.rs index 3ee593b..a111b6c 100644 --- a/client/src/chunker.rs +++ b/client/src/chunker.rs @@ -700,4 +700,47 @@ mod tests { "storage root must never be removed" ); } + + #[tokio::test] + async fn delete_stops_at_first_non_empty_ancestor() { + // If `a/` still has a sibling file after we delete `a/b/c.cook`, + // we must remove `a/b/` (now empty) but leave `a/` alone. + // remove_dir naturally enforces this via ENOTEMPTY; this test + // pins that behavior so a future refactor can't accidentally + // implement recursive deletion. + let temp = tempfile::TempDir::new().unwrap(); + let cache = InMemoryCache::new(100, 10_000); + let mut chunker = Chunker::new(cache, temp.path().to_path_buf()); + + let nested_dir = temp.path().join("a").join("b"); + tokio::fs::create_dir_all(&nested_dir).await.unwrap(); + tokio::fs::write(nested_dir.join("c.cook"), b"eggs\n") + .await + .unwrap(); + tokio::fs::write(temp.path().join("a").join("sibling.cook"), b"flour\n") + .await + .unwrap(); + + chunker + .delete("a/b/c.cook") + .await + .expect("delete should succeed"); + + assert!( + !temp.path().join("a/b/c.cook").exists(), + "target file should be removed" + ); + assert!( + !temp.path().join("a/b").exists(), + "empty intermediate directory should be removed" + ); + assert!( + temp.path().join("a").exists(), + "non-empty ancestor must be preserved" + ); + assert!( + temp.path().join("a/sibling.cook").exists(), + "sibling file must be preserved" + ); + } } From c899f5a6c927102c243523efcf9e310a1cbfb3ef Mon Sep 17 00:00:00 2001 From: Alexey Dubovskoy Date: Mon, 18 May 2026 18:05:16 +0100 Subject: [PATCH 5/7] test(chunker): pin storage root preservation on delete --- client/src/chunker.rs | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/client/src/chunker.rs b/client/src/chunker.rs index a111b6c..154a3e0 100644 --- a/client/src/chunker.rs +++ b/client/src/chunker.rs @@ -743,4 +743,37 @@ mod tests { "sibling file must be preserved" ); } + + #[tokio::test] + async fn delete_does_not_remove_storage_root() { + // Even when the very last file in the storage root is deleted, + // the root itself must survive — otherwise the next download + // cycle has nowhere to write to, and the indexer would crash + // walking a missing directory. + let temp = tempfile::TempDir::new().unwrap(); + let cache = InMemoryCache::new(100, 10_000); + let mut chunker = Chunker::new(cache, temp.path().to_path_buf()); + + tokio::fs::write(temp.path().join("only.cook"), b"sugar\n") + .await + .unwrap(); + + chunker + .delete("only.cook") + .await + .expect("delete should succeed"); + + assert!( + !temp.path().join("only.cook").exists(), + "file should be removed" + ); + assert!( + temp.path().exists(), + "storage root must never be removed" + ); + assert!( + temp.path().is_dir(), + "storage root must still be a directory" + ); + } } From bc5b3c6616efc875aed0abaf9f604388ce1c0ba0 Mon Sep 17 00:00:00 2001 From: Alexey Dubovskoy Date: Mon, 18 May 2026 18:07:01 +0100 Subject: [PATCH 6/7] test(chunker): pin sibling preservation on delete --- client/src/chunker.rs | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/client/src/chunker.rs b/client/src/chunker.rs index 154a3e0..2b89db1 100644 --- a/client/src/chunker.rs +++ b/client/src/chunker.rs @@ -776,4 +776,38 @@ mod tests { "storage root must still be a directory" ); } + + #[tokio::test] + async fn delete_leaves_sibling_files_in_same_directory() { + // Deleting `a/x.cook` when `a/y.cook` also exists must leave + // both `a/` and `a/y.cook` intact. This is the "obvious" case + // but worth pinning — a naive implementation that always + // removes the immediate parent would break it. + let temp = tempfile::TempDir::new().unwrap(); + let cache = InMemoryCache::new(100, 10_000); + let mut chunker = Chunker::new(cache, temp.path().to_path_buf()); + + let dir = temp.path().join("a"); + tokio::fs::create_dir_all(&dir).await.unwrap(); + tokio::fs::write(dir.join("x.cook"), b"salt\n").await.unwrap(); + tokio::fs::write(dir.join("y.cook"), b"pepper\n").await.unwrap(); + + chunker + .delete("a/x.cook") + .await + .expect("delete should succeed"); + + assert!( + !temp.path().join("a/x.cook").exists(), + "target file should be removed" + ); + assert!( + temp.path().join("a").exists(), + "directory with remaining files must be preserved" + ); + assert!( + temp.path().join("a/y.cook").exists(), + "sibling file must be preserved" + ); + } } From b1206be67e8806311a8d077ef6bcc7d798984fe2 Mon Sep 17 00:00:00 2001 From: Alexey Dubovskoy Date: Mon, 18 May 2026 18:16:37 +0100 Subject: [PATCH 7/7] fix(chunker): document traversal-safety assumption and log skipped cleanups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review of #21 fix: the `dir == self.base_path` stop guard is lexical, which is safe under the current trusted-server contract (paths from WalkDir don't contain `..`) but worth documenting so a future weakening of that contract doesn't silently regress. Also surface skipped cleanups via trace! — the dominant cause is ENOTEMPTY (normal) but field reports of leftover folders need a signal beyond "directory still on disk" to diagnose. --- client/src/chunker.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/client/src/chunker.rs b/client/src/chunker.rs index 2b89db1..c33f4fd 100644 --- a/client/src/chunker.rs +++ b/client/src/chunker.rs @@ -158,12 +158,21 @@ impl Chunker { // root (never remove it) and on any error (most commonly ENOTEMPTY // when a sibling file is present — that's the normal terminating // condition, not a failure to propagate). + // + // SAFETY: `dir == self.base_path` is a lexical comparison. It is + // sound here because `full_path` is built by `base_path.clone()` + // + `push(path)` with no canonicalization on either side, and the + // server-provided `path` is trusted to not contain `..` segments + // (the indexer produces forward-slash relative paths via WalkDir, + // which never emits parent components). If that contract ever + // weakens, harden this guard with `starts_with` or a depth check. let mut parent = full_path.parent(); while let Some(dir) = parent { if dir == self.base_path { break; } - if fs::remove_dir(dir).await.is_err() { + if let Err(e) = fs::remove_dir(dir).await { + trace!("stopping empty-dir cleanup at {:?}: {}", dir, e); break; } parent = dir.parent();