fix(s3-datastore): implement absence-on-disk deletion in pushChanged (#797) #70

Merged

stack72 merged 2 commits from fix/797-pushchanged-absence-deletion into main 2026-06-24 15:15:26 +00:00

Conversation 4 Commits 2 Files changed 4 +311 -17

stack72 commented 2026-06-24 14:36:53 +00:00

Owner

Copy link

Summary

• Scoped walk: when markDirty(relPath) signals a path and it's absent locally, pushChanged now collects matching index entries and issues batched S3 DeleteObject calls instead of silently ignoring the absence
• Bulk walk: when per-path dirty tracking overflows (>200 paths), the full walk compares the index against local files and deletes remote entries with no local counterpart — guarded by a new dirtyPathsOverflowed flag so that no-path markDirty() (modification signal) does not delete remote-only entries from other writers (preserves swamp-club#30)
• Both paths guarded by !lazyPullActive to avoid deleting un-hydrated content
• formatBatchFailure extended with "delete" operation type
• Return value of pushChanged includes deleted count (pushed + deleted)
• README cache-write contract section updated to document deletion behavior
• Manifest bumped to 2026.06.24.1

Closes swamp-club#797

Test plan

• New test: scoped walk deletion — markDirty(relPath) + delete local file + pushChanged asserts S3 DELETE issued and index entry removed
• New test: bulk walk deletion — overflow 201 dirty paths + pushChanged asserts S3 DELETE for absent indexed files
• New test: no-path markDirty() does NOT delete remote-only entries (swamp-club#30 regression guard)
• New test: lazy pull guard — lazyPullActive=true + absent file must NOT trigger S3 DELETE
• All 95 tests pass (91 existing + 4 new), 0 regressions
• deno check, deno lint, deno fmt --check clean
• deno install --frozen passes

🤖 Generated with Claude Code

## Summary - **Scoped walk**: when `markDirty(relPath)` signals a path and it's absent locally, `pushChanged` now collects matching index entries and issues batched S3 `DeleteObject` calls instead of silently ignoring the absence - **Bulk walk**: when per-path dirty tracking overflows (>200 paths), the full walk compares the index against local files and deletes remote entries with no local counterpart — guarded by a new `dirtyPathsOverflowed` flag so that no-path `markDirty()` (modification signal) does not delete remote-only entries from other writers (preserves swamp-club#30) - Both paths guarded by `!lazyPullActive` to avoid deleting un-hydrated content - `formatBatchFailure` extended with `"delete"` operation type - Return value of `pushChanged` includes deleted count (pushed + deleted) - README cache-write contract section updated to document deletion behavior - Manifest bumped to `2026.06.24.1` Closes swamp-club#797 ## Test plan - [x] New test: scoped walk deletion — `markDirty(relPath)` + delete local file + `pushChanged` asserts S3 DELETE issued and index entry removed - [x] New test: bulk walk deletion — overflow 201 dirty paths + `pushChanged` asserts S3 DELETE for absent indexed files - [x] New test: no-path `markDirty()` does NOT delete remote-only entries (swamp-club#30 regression guard) - [x] New test: lazy pull guard — `lazyPullActive=true` + absent file must NOT trigger S3 DELETE - [x] All 95 tests pass (91 existing + 4 new), 0 regressions - [x] `deno check`, `deno lint`, `deno fmt --check` clean - [x] `deno install --frozen` passes 🤖 Generated with [Claude Code](https://claude.ai/claude-code)

stack72 added 1 commit 2026-06-24 14:36:53 +00:00

fix(s3-datastore): implement absence-on-disk deletion in pushChanged (swamp-club#797) ... c0aef5e41e

pushChanged now issues S3 DELETEs for locally-absent files that were
signaled via markDirty(relPath), implementing the markDirty contract
rule #2 (absence-on-disk = delete).

Scoped walk: when Deno.stat throws for a dirty path, matching index
entries are collected for S3 deletion instead of being silently ignored.

Bulk walk: when per-path dirty tracking overflows (>200 paths), the
full walk compares the index against local files and deletes entries
with no local counterpart. A new dirtyPathsOverflowed flag distinguishes
this from no-path markDirty (which is a modification signal, not a
deletion signal), preserving the swamp-club#30 guarantee that
remote-only entries are not deleted on a fresh reader push.

Both paths are guarded by !lazyPullActive to avoid deleting un-hydrated
content.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-06-24 14:51:12 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

1. s3_cache_sync.ts:1306-1321 — Scoped-walk catch block swallows non-ENOENT errors, treating permission errors and I/O failures as "file absent" and triggering remote deletion.

   The scoped-walk branch does Deno.stat(absPath) inside a try, and the catch block unconditionally interprets any exception as "path is absent locally", which feeds entries into toDelete. But Deno.stat can throw for reasons other than file absence: PermissionDenied, Interrupted, FilesystemLoop, or even transient NFS/FUSE errors. On any of these, the code would queue the remote S3 objects for deletion despite the local files still existing.

   Breaking example: A cache directory on an NFS mount briefly becomes unresponsive. Deno.stat throws Deno.errors.Busy (or a generic OS error). The catch block treats every index entry matching that dirty path as absent and deletes them from S3, destroying data.

   Severity: HIGH — This is a data-loss path. A transient I/O error during Deno.stat would cause permanent remote deletion.

   Suggested fix: Check the error type before entering the deletion branch. Only treat Deno.errors.NotFound as absence; re-throw (or skip the path without deleting) for all other error types:

   } catch (err) {
     if (!(err instanceof Deno.errors.NotFound)) {
       // Non-absence error — do not assume deletion intent.
       // Either re-throw or skip this dirtyPath silently.
       continue;
     }
     // Path is genuinely absent — proceed with deletion logic
     if (!this.lazyPullActive && this.index) {
       ...
     }
   }
   

2. s3_cache_sync.ts:1427 — Index writeback condition (pushed > 0 || this.indexMutated) does not account for deleted > 0 alone when indexMutated is false.

   Trace the case: scoped walk finds 0 files to push but >0 files to delete. The delete loop at lines 1385–1416 sets this.indexMutated = true when deletion succeeds (line 1405). So the writeback condition pushed > 0 || this.indexMutated evaluates to 0 > 0 || true → true, and the writeback fires. This is correct in the current code because the delete loop mutates this.indexMutated.

   However, if a future refactoring moves the delete this.index.entries[batch[j]] call or drops the this.indexMutated = true assignment, the writeback would silently skip, leaving S3 objects deleted but the remote index still referencing them. The condition would read more clearly as (pushed > 0 || deleted > 0 || this.indexMutated). This is noted for robustness but does not block since the current code is correct.

   Severity: LOW — The current code works; this is a maintainability concern only.

Medium

1. s3_cache_sync.ts:1509 — return pushed + deleted counts delete-failures as neither pushed nor deleted, silently under-reporting.

   If some deletes succeed and then the delete-failure check at line 1414 throws, the function never reaches line 1509 — the error propagates. That's fine. But consider a subtle scenario: if pushFile for one file in toPush fails (caught by Promise.allSettled, added to failures), then the push-failures check at line 1381 throws before the delete loop runs. Any files that should have been deleted are silently skipped with no error message mentioning them.

   Breaking example: A batch of 5 files to push and 3 to delete. File 2 fails with 403. The function throws at line 1382 with a push-failure message listing only the push files. The 3 deletions never execute and are not mentioned in the error. The caller has no signal that deletions were skipped.

   Severity: MEDIUM — The error message is misleading about the scope of what failed, but the operation is already in a failure state. A second push would retry.

2. s3_cache_sync.ts:1310-1319 — Scoped-walk deletion prefix matching could match unrelated entries with overlapping path prefixes.

   The prefix construction at line 1311-1312 appends "/" if not already present, then checks rel.startsWith(prefix). Consider: dirty path is data/model, prefix becomes data/model/. An index entry data/model-v2/file.yaml does NOT start with data/model/, so it's safe. But if dirty path is data/mo, prefix becomes data/mo/, and data/model/file.yaml DOES match.

   In practice, markDirty is called with paths like data/<kind>/<type>/<id> which are structured enough to avoid collisions. But the code has no structural validation — it relies on callers never passing a truncated prefix.

   Severity: MEDIUM — Unlikely with well-formed caller paths, but no guard prevents it.

3. s3_cache_sync_test.ts:4191-4200 — Test "pushChanged scoped walk deletes S3 objects" removes both the file and its parent directory before calling pushChanged, but doesn't verify what happens if only the file is removed (directory still exists).

   When the directory data/model/id/v1 still exists but is empty, Deno.stat succeeds (it's a directory), the code enters the stat.isDirectory branch, walk finds no files, and no deletion occurs — the remote S3 object is never deleted even though the file is gone. This is a real gap: if a caller deletes a file but not the parent directory, the scoped walk won't detect the deletion.

   Severity: MEDIUM — Edge case that could leave orphaned S3 objects when only files (not directories) are deleted.

Low

1. s3_cache_sync.ts:1392-1397 — Delete batches reuse this.pushConcurrency rather than having a separate delete concurrency.

   S3 DeleteObject is much cheaper than PutObject. Using the push concurrency (default 25) for deletes is conservative but leaves performance on the table for large deletion sets. Not a bug, just a minor inefficiency.

   Severity: LOW

2. s3_cache_sync.ts:434 — dirtyPathsOverflowed?: boolean is optional in DatastoreSyncStateV2 but required in markSynced's written object (line 674).

   The field is always written as dirtyPathsOverflowed: false by markSynced, so a read of a v2 sidecar without the field (written by the previous version) gets undefined, which !!undefined at line 533 coerces to false. This is fine — the optional-to-required transition is handled correctly by the !! coercion. The only concern is that during a rolling upgrade, old writers produce sidecars without this field and new readers silently ignore it. This is safe behavior.

   Severity: LOW

Verdict

FAIL — Finding #1 (HIGH) is a data-loss risk. The scoped-walk catch block at lines 1306-1321 conflates all Deno.stat errors with "file not found", meaning any non-ENOENT stat failure (permissions, I/O, NFS timeout) on a dirty path would trigger remote S3 deletion of existing data. This should be narrowed to only trigger deletion on Deno.errors.NotFound.

## Adversarial Review ### Critical / High 1. **`s3_cache_sync.ts:1306-1321` — Scoped-walk catch block swallows non-ENOENT errors, treating permission errors and I/O failures as "file absent" and triggering remote deletion.** The scoped-walk branch does `Deno.stat(absPath)` inside a `try`, and the `catch` block unconditionally interprets *any* exception as "path is absent locally", which feeds entries into `toDelete`. But `Deno.stat` can throw for reasons other than file absence: `PermissionDenied`, `Interrupted`, `FilesystemLoop`, or even transient NFS/FUSE errors. On any of these, the code would queue the remote S3 objects for deletion despite the local files still existing. **Breaking example:** A cache directory on an NFS mount briefly becomes unresponsive. `Deno.stat` throws `Deno.errors.Busy` (or a generic OS error). The catch block treats every index entry matching that dirty path as absent and deletes them from S3, destroying data. **Severity: HIGH** — This is a data-loss path. A transient I/O error during `Deno.stat` would cause permanent remote deletion. **Suggested fix:** Check the error type before entering the deletion branch. Only treat `Deno.errors.NotFound` as absence; re-throw (or skip the path without deleting) for all other error types: ```typescript } catch (err) { if (!(err instanceof Deno.errors.NotFound)) { // Non-absence error — do not assume deletion intent. // Either re-throw or skip this dirtyPath silently. continue; } // Path is genuinely absent — proceed with deletion logic if (!this.lazyPullActive && this.index) { ... } } ``` 2. **`s3_cache_sync.ts:1427` — Index writeback condition `(pushed > 0 || this.indexMutated)` does not account for `deleted > 0` alone when `indexMutated` is false.** Trace the case: scoped walk finds 0 files to push but >0 files to delete. The delete loop at lines 1385–1416 sets `this.indexMutated = true` when deletion succeeds (line 1405). So the writeback condition `pushed > 0 || this.indexMutated` evaluates to `0 > 0 || true` → `true`, and the writeback fires. This is **correct** in the current code because the delete loop mutates `this.indexMutated`. However, if a future refactoring moves the `delete this.index.entries[batch[j]]` call or drops the `this.indexMutated = true` assignment, the writeback would silently skip, leaving S3 objects deleted but the remote index still referencing them. The condition would read more clearly as `(pushed > 0 || deleted > 0 || this.indexMutated)`. This is noted for robustness but does **not** block since the current code is correct. **Severity: LOW** — The current code works; this is a maintainability concern only. ### Medium 1. **`s3_cache_sync.ts:1509` — `return pushed + deleted` counts delete-failures as neither pushed nor deleted, silently under-reporting.** If *some* deletes succeed and then the delete-failure check at line 1414 throws, the function never reaches line 1509 — the error propagates. That's fine. But consider a subtle scenario: if `pushFile` for one file in `toPush` fails (caught by `Promise.allSettled`, added to `failures`), then the push-failures check at line 1381 throws *before* the delete loop runs. Any files that should have been deleted are silently skipped with no error message mentioning them. **Breaking example:** A batch of 5 files to push and 3 to delete. File 2 fails with 403. The function throws at line 1382 with a push-failure message listing only the push files. The 3 deletions never execute and are not mentioned in the error. The caller has no signal that deletions were skipped. **Severity: MEDIUM** — The error message is misleading about the scope of what failed, but the operation is already in a failure state. A second push would retry. 2. **`s3_cache_sync.ts:1310-1319` — Scoped-walk deletion prefix matching could match unrelated entries with overlapping path prefixes.** The prefix construction at line 1311-1312 appends "/" if not already present, then checks `rel.startsWith(prefix)`. Consider: dirty path is `data/model`, prefix becomes `data/model/`. An index entry `data/model-v2/file.yaml` does NOT start with `data/model/`, so it's safe. But if dirty path is `data/mo`, prefix becomes `data/mo/`, and `data/model/file.yaml` DOES match. In practice, `markDirty` is called with paths like `data/<kind>/<type>/<id>` which are structured enough to avoid collisions. But the code has no structural validation — it relies on callers never passing a truncated prefix. **Severity: MEDIUM** — Unlikely with well-formed caller paths, but no guard prevents it. 3. **`s3_cache_sync_test.ts:4191-4200` — Test "pushChanged scoped walk deletes S3 objects" removes both the file and its parent directory before calling `pushChanged`, but doesn't verify what happens if only the file is removed (directory still exists).** When the directory `data/model/id/v1` still exists but is empty, `Deno.stat` succeeds (it's a directory), the code enters the `stat.isDirectory` branch, `walk` finds no files, and no deletion occurs — the remote S3 object is never deleted even though the file is gone. This is a real gap: if a caller deletes a file but not the parent directory, the scoped walk won't detect the deletion. **Severity: MEDIUM** — Edge case that could leave orphaned S3 objects when only files (not directories) are deleted. ### Low 1. **`s3_cache_sync.ts:1392-1397` — Delete batches reuse `this.pushConcurrency` rather than having a separate delete concurrency.** S3 DeleteObject is much cheaper than PutObject. Using the push concurrency (default 25) for deletes is conservative but leaves performance on the table for large deletion sets. Not a bug, just a minor inefficiency. **Severity: LOW** 2. **`s3_cache_sync.ts:434` — `dirtyPathsOverflowed?: boolean` is optional in `DatastoreSyncStateV2` but required in `markSynced`'s written object (line 674).** The field is always written as `dirtyPathsOverflowed: false` by `markSynced`, so a read of a v2 sidecar without the field (written by the previous version) gets `undefined`, which `!!undefined` at line 533 coerces to `false`. This is fine — the optional-to-required transition is handled correctly by the `!!` coercion. The only concern is that during a rolling upgrade, old writers produce sidecars without this field and new readers silently ignore it. This is safe behavior. **Severity: LOW** ### Verdict **FAIL** — Finding #1 (HIGH) is a data-loss risk. The scoped-walk catch block at lines 1306-1321 conflates all `Deno.stat` errors with "file not found", meaning any non-ENOENT stat failure (permissions, I/O, NFS timeout) on a dirty path would trigger remote S3 deletion of existing data. This should be narrowed to only trigger deletion on `Deno.errors.NotFound`.

stack72 commented 2026-06-24 14:51:43 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. formatBatchFailure preposition logic can be simplified (s3_cache_sync.ts:399).
   op === "pull" ? "from" : op === "delete" ? "from" : "to" has a dead middle branch — both "pull" and "delete" map to "from". Simpler and equivalent: op === "push" ? "to" : "from". No behaviour change, just easier to read.

2. Mock deleteObject omits the signal parameter (s3_cache_sync_test.ts:106).
   The real S3Client.deleteObject signature is (key: string, signal?: AbortSignal): Promise<void>. The mock only declares (key: string). This compiles fine because signal is optional, but making the signatures match improves fidelity for future tests that pass a signal.

3. Redundant Deno.mkdir in two new tests (s3_cache_sync_test.ts:4246, 4268, 4290, 4311).
   Deno.makeTempDir() already creates the directory; calling await Deno.mkdir(cachePath, { recursive: true }) immediately after is a no-op. Removing it declutters the setup.

4. Awkward typeof + cast assertions (s3_cache_sync_test.ts:4216, 4256).
   assertEquals(typeof synced, "number"); assert((synced as number) >= 1) can be collapsed to assertGreater(synced as number, 0) (or assertGreaterOrEqual(..., 1)). These are pre-existing patterns elsewhere in the file, so not worth blocking on, but the new tests could set a cleaner precedent.

---

Overall the implementation is well-structured. The dirtyPathsOverflowed flag correctly distinguishes overflow-triggered bulk walks (where remote-only entries are safe to delete) from no-relPath markDirty() calls (where they are not). The lazy-pull guard is applied consistently in both the scoped and bulk walk paths. State is persisted and restored correctly across restarts. The four new tests cover the key cases: scoped-walk deletion, overflow-triggered bulk deletion, no-relPath non-deletion, and lazy-pull suppression.

## Code Review ### Blocking Issues None. ### Suggestions 1. **`formatBatchFailure` preposition logic can be simplified** (`s3_cache_sync.ts:399`). `op === "pull" ? "from" : op === "delete" ? "from" : "to"` has a dead middle branch — both `"pull"` and `"delete"` map to `"from"`. Simpler and equivalent: `op === "push" ? "to" : "from"`. No behaviour change, just easier to read. 2. **Mock `deleteObject` omits the `signal` parameter** (`s3_cache_sync_test.ts:106`). The real `S3Client.deleteObject` signature is `(key: string, signal?: AbortSignal): Promise<void>`. The mock only declares `(key: string)`. This compiles fine because `signal` is optional, but making the signatures match improves fidelity for future tests that pass a signal. 3. **Redundant `Deno.mkdir` in two new tests** (`s3_cache_sync_test.ts:4246`, `4268`, `4290`, `4311`). `Deno.makeTempDir()` already creates the directory; calling `await Deno.mkdir(cachePath, { recursive: true })` immediately after is a no-op. Removing it declutters the setup. 4. **Awkward `typeof` + cast assertions** (`s3_cache_sync_test.ts:4216`, `4256`). `assertEquals(typeof synced, "number"); assert((synced as number) >= 1)` can be collapsed to `assertGreater(synced as number, 0)` (or `assertGreaterOrEqual(..., 1)`). These are pre-existing patterns elsewhere in the file, so not worth blocking on, but the new tests could set a cleaner precedent. --- Overall the implementation is well-structured. The `dirtyPathsOverflowed` flag correctly distinguishes overflow-triggered bulk walks (where remote-only entries are safe to delete) from no-relPath `markDirty()` calls (where they are not). The lazy-pull guard is applied consistently in both the scoped and bulk walk paths. State is persisted and restored correctly across restarts. The four new tests cover the key cases: scoped-walk deletion, overflow-triggered bulk deletion, no-relPath non-deletion, and lazy-pull suppression.

stack72 added 1 commit 2026-06-24 14:56:10 +00:00

fix(s3-datastore): harden deletion path against non-NotFound errors ... 10fdae799d

Address CI review findings:

- HIGH: Scoped-walk catch block now only treats Deno.errors.NotFound as
  absence. Permission errors, I/O failures, and transient NFS errors
  skip the dirty path without triggering deletion.

- MEDIUM: Scoped-walk directory branch now detects files deleted within
  a still-existing directory by comparing walked local files against
  index entries under the dirty path prefix.

- LOW: Writeback condition explicitly includes `deleted > 0` for
  clarity alongside the existing indexMutated guard.

- Cleanup: simplified formatBatchFailure preposition logic, added
  signal parameter to mock deleteObject, removed redundant Deno.mkdir
  calls, simplified test assertions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-06-24 15:06:22 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

None found.

Medium

1. Partial delete failure leaves remote index referencing deleted objects — s3_cache_sync.ts:1436-1438

   If some deleteObject calls in the batch succeed but others fail (e.g., transient 5xx on the Nth key), the code throws at line 1437 before the index writeback at line 1449. Successfully-deleted objects are already gone from S3, but the remote index still references them (the in-memory index was updated at line 1426, but never written back). A subsequent pullChanged by another client would attempt to getObject on the deleted keys and fail with 404.

   Breaking scenario: Batch of 5 deletes — keys 1-3 succeed, key 4 gets a persistent 5xx after retries. Keys 1-3 are gone from S3, but the remote index still lists all 5. Another client's pullChanged tries to download key 1 and gets a 404 → batch failure → entire pull aborted.

   Mitigating factors: S3 DeleteObject is highly reliable and the retry logic handles transient failures. A persistent partial failure requires a pathological scenario (e.g., per-key IAM deny, which is unusual). The existing push path has the same structural issue for uploads (partial push success + throw before writeback), so this is consistent with the pre-existing contract. Still, the consequence for deletes is worse — orphaned pushes are benign; dangling index references break pulls.

   Suggested fix: Consider writing back the index (with successfully-deleted entries removed) before throwing the delete-failure error, or at minimum document the partial-failure semantics so callers know to retry pushChanged after a delete batch error.

2. Per-path deletion intent lost when pushFile forces bulk invalidation — s3_cache_sync.ts:1345-1373

   Scenario: (a) markDirty({ relPath: "data/x" }) adds data/x to dirtyPaths, (b) pushFile("data/y") calls markDirty() (no relPath), setting bulkInvalidated = true without setting dirtyPathsOverflowed, (c) user deletes data/x locally, (d) pushChanged() takes the bulk walk (because bulkInvalidated is true), but skips deletion detection (because dirtyPathsOverflowed is false). The local deletion of data/x is never propagated to S3.

   Breaking scenario: A command lifecycle that both marks specific paths dirty for deletion AND calls pushFile for a different file. The dirty-path deletion intent from step (a) is silently discarded.

   Mitigating factors: This requires interleaving markDirty({ relPath }) with pushFile in the same service instance, which may not occur in current core usage. The remote object is preserved (not data loss), and the next explicit sync could catch it.

   Suggested fix: Either set dirtyPathsOverflowed = true when bulkInvalidated is set while dirtyPaths is non-empty, or document this interaction as a known limitation.

Low

1. Duplicate entries in toDelete from overlapping dirty paths — s3_cache_sync.ts:1283-1344

   If dirtyPaths contains both "data/model" and "data/model/id/v1", and the file data/model/id/v1/raw was deleted, both dirty-path iterations add it to toDelete. This produces two deleteObject calls for the same key — wasteful but not harmful since S3 delete is idempotent. The deleted counter is also inflated, which inflates the return value.

   Suggested fix: Use a Set<string> for toDelete instead of an array, or deduplicate before the delete loop.

2. pushChanged return value semantic change — s3_cache_sync.ts:1531

   The return changed from pushed (upload count) to pushed + deleted. Any caller interpreting the return specifically as "number of files uploaded" would get a higher-than-expected number. The existing tests use >= 1 or === 0 checks, so they pass regardless. The semantic shift is arguably correct ("sync actions performed"), but it's an observable contract change.

3. formatBatchFailure preposition for delete: "from" — s3_cache_sync.ts:399

   When op === "delete", the ternary op === "push" ? "to" : "from" produces "Failed to delete 3 file(s) from S3". Grammatically fine and natural, but if more operations are added later, the binary ternary would need revisiting. Not a bug today.

Verdict

PASS — The core deletion logic is sound: scoped-walk absence detection, bulk-walk overflow detection, lazy-pull suppression, and the dirtyPathsOverflowed flag all interact correctly for the primary use cases. The new tests cover the key scenarios well, including the regression guard for no-relPath markDirty and the lazy-pull suppression path. The medium findings are edge cases in uncommon interaction patterns, not blocking issues.

## Adversarial Review ### Critical / High None found. ### Medium 1. **Partial delete failure leaves remote index referencing deleted objects** — `s3_cache_sync.ts:1436-1438` If some `deleteObject` calls in the batch succeed but others fail (e.g., transient 5xx on the Nth key), the code throws at line 1437 *before* the index writeback at line 1449. Successfully-deleted objects are already gone from S3, but the remote index still references them (the in-memory index was updated at line 1426, but never written back). A subsequent `pullChanged` by another client would attempt to `getObject` on the deleted keys and fail with 404. **Breaking scenario:** Batch of 5 deletes — keys 1-3 succeed, key 4 gets a persistent 5xx after retries. Keys 1-3 are gone from S3, but the remote index still lists all 5. Another client's `pullChanged` tries to download key 1 and gets a 404 → batch failure → entire pull aborted. **Mitigating factors:** S3 `DeleteObject` is highly reliable and the retry logic handles transient failures. A persistent partial failure requires a pathological scenario (e.g., per-key IAM deny, which is unusual). The existing push path has the same structural issue for uploads (partial push success + throw before writeback), so this is consistent with the pre-existing contract. Still, the consequence for deletes is worse — orphaned pushes are benign; dangling index references break pulls. **Suggested fix:** Consider writing back the index (with successfully-deleted entries removed) *before* throwing the delete-failure error, or at minimum document the partial-failure semantics so callers know to retry `pushChanged` after a delete batch error. 2. **Per-path deletion intent lost when `pushFile` forces bulk invalidation** — `s3_cache_sync.ts:1345-1373` Scenario: (a) `markDirty({ relPath: "data/x" })` adds `data/x` to `dirtyPaths`, (b) `pushFile("data/y")` calls `markDirty()` (no relPath), setting `bulkInvalidated = true` without setting `dirtyPathsOverflowed`, (c) user deletes `data/x` locally, (d) `pushChanged()` takes the bulk walk (because `bulkInvalidated` is true), but skips deletion detection (because `dirtyPathsOverflowed` is false). The local deletion of `data/x` is never propagated to S3. **Breaking scenario:** A command lifecycle that both marks specific paths dirty for deletion AND calls `pushFile` for a different file. The dirty-path deletion intent from step (a) is silently discarded. **Mitigating factors:** This requires interleaving `markDirty({ relPath })` with `pushFile` in the same service instance, which may not occur in current core usage. The remote object is preserved (not data loss), and the next explicit sync could catch it. **Suggested fix:** Either set `dirtyPathsOverflowed = true` when `bulkInvalidated` is set while `dirtyPaths` is non-empty, or document this interaction as a known limitation. ### Low 1. **Duplicate entries in `toDelete` from overlapping dirty paths** — `s3_cache_sync.ts:1283-1344` If `dirtyPaths` contains both `"data/model"` and `"data/model/id/v1"`, and the file `data/model/id/v1/raw` was deleted, both dirty-path iterations add it to `toDelete`. This produces two `deleteObject` calls for the same key — wasteful but not harmful since S3 delete is idempotent. The `deleted` counter is also inflated, which inflates the return value. **Suggested fix:** Use a `Set<string>` for `toDelete` instead of an array, or deduplicate before the delete loop. 2. **`pushChanged` return value semantic change** — `s3_cache_sync.ts:1531` The return changed from `pushed` (upload count) to `pushed + deleted`. Any caller interpreting the return specifically as "number of files uploaded" would get a higher-than-expected number. The existing tests use `>= 1` or `=== 0` checks, so they pass regardless. The semantic shift is arguably correct ("sync actions performed"), but it's an observable contract change. 3. **`formatBatchFailure` preposition for delete: "from"** — `s3_cache_sync.ts:399` When `op === "delete"`, the ternary `op === "push" ? "to" : "from"` produces `"Failed to delete 3 file(s) from S3"`. Grammatically fine and natural, but if more operations are added later, the binary ternary would need revisiting. Not a bug today. ### Verdict **PASS** — The core deletion logic is sound: scoped-walk absence detection, bulk-walk overflow detection, lazy-pull suppression, and the `dirtyPathsOverflowed` flag all interact correctly for the primary use cases. The new tests cover the key scenarios well, including the regression guard for no-relPath `markDirty` and the lazy-pull suppression path. The medium findings are edge cases in uncommon interaction patterns, not blocking issues.

stack72 commented 2026-06-24 15:12:32 +00:00

Author

Owner

Copy link

Code Review

This PR implements absence-on-disk deletion in pushChanged and persists dirtyPathsOverflowed across restarts so the bulk-delete path survives crashes. The logic is sound and the new tests cover the key scenarios.

Blocking Issues

None.

Suggestions

1. toDelete array can contain duplicates when overlapping dirty paths both turn up absent

When a caller marks both a parent and child directory dirty and neither exists on disk, both dirty-path iterations hit the NotFound catch branch and collect the same index entries. Duplicate keys in toDelete cause s3.deleteObject to be called twice for the same key.

AWS S3 is harmless here: DeleteObject always returns 204 even for already-gone keys. However, the README lists MinIO, DigitalOcean Spaces, and R2 as supported backends. Some S3-compatible stores return 404 when deleting a non-existent key, which would land in deleteFailures and throw a spurious batch error.

Fix: use a Set for toDelete rather than an array, or dedup before the delete loop.

2. (Pre-existing) sanitizeResources: false without inline explanation on DEF-2 integration tests

Tests at lines 1145, 1175, 1205, and 1383 set sanitizeResources: false with no accompanying comment explaining why (CLAUDE.md requires one). The explanation appears near line 2553, not adjacent to these tests. The new deletion tests do not repeat this gap, so this is worth a follow-up cleanup rather than a blocker.

## Code Review This PR implements absence-on-disk deletion in pushChanged and persists dirtyPathsOverflowed across restarts so the bulk-delete path survives crashes. The logic is sound and the new tests cover the key scenarios. ### Blocking Issues None. ### Suggestions **1. toDelete array can contain duplicates when overlapping dirty paths both turn up absent** When a caller marks both a parent and child directory dirty and neither exists on disk, both dirty-path iterations hit the NotFound catch branch and collect the same index entries. Duplicate keys in toDelete cause s3.deleteObject to be called twice for the same key. AWS S3 is harmless here: DeleteObject always returns 204 even for already-gone keys. However, the README lists MinIO, DigitalOcean Spaces, and R2 as supported backends. Some S3-compatible stores return 404 when deleting a non-existent key, which would land in deleteFailures and throw a spurious batch error. Fix: use a Set for toDelete rather than an array, or dedup before the delete loop. **2. (Pre-existing) sanitizeResources: false without inline explanation on DEF-2 integration tests** Tests at lines 1145, 1175, 1205, and 1383 set sanitizeResources: false with no accompanying comment explaining why (CLAUDE.md requires one). The explanation appears near line 2553, not adjacent to these tests. The new deletion tests do not repeat this gap, so this is worth a follow-up cleanup rather than a blocker.

stack72 merged commit bc39f7ede9 into main 2026-06-24 15:15:26 +00:00

stack72 deleted branch fix/797-pushchanged-absence-deletion 2026-06-24 15:15:27 +00:00

stack72 referenced this pull request from a commit 2026-06-24 15:15:27 +00:00

fix(s3-datastore): implement absence-on-disk deletion in pushChanged (#797) (#70)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

swamp-club/swamp-extensions!70

No description provided.