feat(gcs-datastore): implement absence-on-disk deletion in pushChanged (swamp-club#798) #71

Merged

stack72 merged 1 commit from feat/gcs-pushchanged-absence-deletion into main 2026-06-24 15:16:04 +00:00

Conversation 2 Commits 1 Files changed 3 +337 -10

stack72 commented 2026-06-24 14:58:32 +00:00

Owner

Copy link

Summary

Implements markDirty contract rule #2 for the GCS datastore: when core calls markDirty(relPath) before removing a file, pushChanged now detects the absent dirty path and issues GCS deleteObject calls for matching index entries.

• Scoped walk: absent dirty paths trigger prefix-matched index entry collection for remote deletion
• Bulk walk: after iterating local files, orphaned index entries with no local counterpart are collected for deletion (only when bulkInvalidated)
• Both paths guarded by lazyPullActive + isLazySkippable to avoid deleting un-hydrated content
• Reader-side repos calling pushChanged without markDirty are unaffected (no dirty signal = no deletion)
• Mirrors the fix pattern from swamp-club#797 (S3 datastore), targeting GCS independently

Test plan

• New test: scoped-walk deletion — markDirty(relPath), remove file, pushChanged issues deleteObject and removes index entry
• New test: bulk-walk deletion — markDirty() without relPath, remove file, pushChanged detects orphan and deletes
• New test: lazy hydration guard — with lazyPullActive=true, absent un-hydrated files are NOT deleted
• New test: reader-side safety — without markDirty, no files are deleted
• Existing "preserves remote index entries for files absent from local cache" test still passes
• Full test suite: 150/150 pass
• deno check, deno lint, deno fmt --check, deno install --frozen all clean

Closes swamp-club#798

## Summary Implements markDirty contract rule #2 for the GCS datastore: when core calls `markDirty(relPath)` before removing a file, `pushChanged` now detects the absent dirty path and issues GCS `deleteObject` calls for matching index entries. - **Scoped walk**: absent dirty paths trigger prefix-matched index entry collection for remote deletion - **Bulk walk**: after iterating local files, orphaned index entries with no local counterpart are collected for deletion (only when `bulkInvalidated`) - Both paths guarded by `lazyPullActive` + `isLazySkippable` to avoid deleting un-hydrated content - Reader-side repos calling `pushChanged` without `markDirty` are unaffected (no dirty signal = no deletion) - Mirrors the fix pattern from swamp-club#797 (S3 datastore), targeting GCS independently ## Test plan - [x] New test: scoped-walk deletion — `markDirty(relPath)`, remove file, `pushChanged` issues `deleteObject` and removes index entry - [x] New test: bulk-walk deletion — `markDirty()` without relPath, remove file, `pushChanged` detects orphan and deletes - [x] New test: lazy hydration guard — with `lazyPullActive=true`, absent un-hydrated files are NOT deleted - [x] New test: reader-side safety — without `markDirty`, no files are deleted - [x] Existing "preserves remote index entries for files absent from local cache" test still passes - [x] Full test suite: 150/150 pass - [x] `deno check`, `deno lint`, `deno fmt --check`, `deno install --frozen` all clean Closes swamp-club#798

stack72 added 1 commit 2026-06-24 14:58:32 +00:00

feat(gcs-datastore): implement absence-on-disk deletion in pushChanged (swamp-club#798) ... d05235c83c

When core calls markDirty(relPath) before removing a file, pushChanged
now detects the absent dirty path and issues GCS deleteObject calls for
matching index entries, fulfilling markDirty contract rule #2.

Two code paths handle deletion:
- Scoped walk: absent dirty paths trigger prefix-matched index entry
  collection for remote deletion
- Bulk walk: after iterating local files, orphaned index entries with
  no local counterpart are collected for deletion (only when
  bulkInvalidated)

Both paths are guarded by lazyPullActive + isLazySkippable to avoid
deleting un-hydrated content. Reader-side repos that call pushChanged
without markDirty are unaffected — no dirty signal means no deletion.

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

stack72 commented 2026-06-24 15:10:27 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. Duplicate entries in toDelete from overlapping scoped dirty paths
   • File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1186-1199
   • What's wrong: In the scoped walk path, each dirty path independently scans this.index.entries and appends matching keys to toDelete. If two dirty paths overlap (e.g. data/model/alpha and data/model/alpha/v1), the same index key can be appended to toDelete multiple times.
   • Breaking example: markDirty({relPath: "data/model/alpha"}) then markDirty({relPath: "data/model/alpha/v1"}), then delete the directory. pushChanged builds toDelete = ["data/model/alpha/v1/raw", "data/model/alpha/v1/raw"]. The delete loop fires two deleteObject calls for the same key (the second is a GCS no-op since 404 is handled), increments deleted to 2 instead of 1, and the return value is inflated.
   • Impact: Wasted API call, inflated return value reported to callers. No data corruption — delete this.index.entries[key] on the second pass is a no-op, and deleteObject handles 404 gracefully.
   • Suggested fix: Deduplicate toDelete before the delete loop, or use a Set<string> and convert to array at the end:

     const toDeleteSet = new Set<string>();
     // ... in the loop:
     toDeleteSet.add(key);
     // ... after the loop:
     const toDelete = [...toDeleteSet];
     

Low

1. Return value semantics change

   • File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1351
   • What's wrong: pushChanged previously returned pushed (upload count); it now returns pushed + deleted. The DatastoreSyncService interface says "returns the count synced" which is ambiguous. Callers that log this as "N files pushed" will now report a count that includes deletions.
   • Impact: Cosmetic only — log messages might say "Pushed 5 files" when 3 were uploaded and 2 were deleted. No functional breakage since the interface contract doesn't distinguish.

2. Partial delete failure leaves stale index entries for already-deleted objects

   • File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1293-1296
   • What's wrong: If deletes partially fail (some succeed, some don't), the error is thrown before writeback. Successfully-deleted objects are gone from GCS but their entries remain in the remote index. A concurrent pullChanged from another client would try to download the missing objects, get 404s, and fail with a batch error.
   • Impact: Transient inconsistency during partial delete failure — an already-degraded infrastructure state. Self-heals on the next successful pushChanged. This is the same pattern that existed for partial push failures before this PR, so not a regression.

Verdict

PASS — The deletion implementation is well-designed with appropriate guards: reader-side safety (no markDirty = no deletes), lazy hydration protection (un-hydrated raw files excluded from bulk orphan scan), proper isInternalCacheFile filtering, retry-with-backoff on delete calls, and comprehensive test coverage for all paths. The medium finding (duplicate toDelete entries from overlapping dirty paths) is low-impact and self-correcting. The overall approach is sound and consistent with the existing push/pull patterns.

## Adversarial Review ### Medium 1. **Duplicate entries in `toDelete` from overlapping scoped dirty paths** - **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1186-1199` - **What's wrong:** In the scoped walk path, each dirty path independently scans `this.index.entries` and appends matching keys to `toDelete`. If two dirty paths overlap (e.g. `data/model/alpha` and `data/model/alpha/v1`), the same index key can be appended to `toDelete` multiple times. - **Breaking example:** `markDirty({relPath: "data/model/alpha"})` then `markDirty({relPath: "data/model/alpha/v1"})`, then delete the directory. `pushChanged` builds `toDelete = ["data/model/alpha/v1/raw", "data/model/alpha/v1/raw"]`. The delete loop fires two `deleteObject` calls for the same key (the second is a GCS no-op since 404 is handled), increments `deleted` to 2 instead of 1, and the return value is inflated. - **Impact:** Wasted API call, inflated return value reported to callers. No data corruption — `delete this.index.entries[key]` on the second pass is a no-op, and `deleteObject` handles 404 gracefully. - **Suggested fix:** Deduplicate `toDelete` before the delete loop, or use a `Set<string>` and convert to array at the end: ```typescript const toDeleteSet = new Set<string>(); // ... in the loop: toDeleteSet.add(key); // ... after the loop: const toDelete = [...toDeleteSet]; ``` ### Low 1. **Return value semantics change** - **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1351` - **What's wrong:** `pushChanged` previously returned `pushed` (upload count); it now returns `pushed + deleted`. The `DatastoreSyncService` interface says "returns the count synced" which is ambiguous. Callers that log this as "N files pushed" will now report a count that includes deletions. - **Impact:** Cosmetic only — log messages might say "Pushed 5 files" when 3 were uploaded and 2 were deleted. No functional breakage since the interface contract doesn't distinguish. 2. **Partial delete failure leaves stale index entries for already-deleted objects** - **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1293-1296` - **What's wrong:** If deletes partially fail (some succeed, some don't), the error is thrown before writeback. Successfully-deleted objects are gone from GCS but their entries remain in the remote index. A concurrent `pullChanged` from another client would try to download the missing objects, get 404s, and fail with a batch error. - **Impact:** Transient inconsistency during partial delete failure — an already-degraded infrastructure state. Self-heals on the next successful `pushChanged`. This is the same pattern that existed for partial push failures before this PR, so not a regression. ### Verdict **PASS** — The deletion implementation is well-designed with appropriate guards: reader-side safety (no `markDirty` = no deletes), lazy hydration protection (un-hydrated raw files excluded from bulk orphan scan), proper `isInternalCacheFile` filtering, retry-with-backoff on delete calls, and comprehensive test coverage for all paths. The medium finding (duplicate `toDelete` entries from overlapping dirty paths) is low-impact and self-correcting. The overall approach is sound and consistent with the existing push/pull patterns.

stack72 commented 2026-06-24 15:10:30 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. Scoped walk catch block catches all Deno.stat errors, not just NotFound (gcs_cache_sync.ts, the catch block in the scoped walk added by this PR): If Deno.stat(absPath) fails for a reason other than NotFound — for example a transient PermissionDenied while the cache directory is temporarily inaccessible — the code currently queues all matching index entries for remote deletion. This could delete GCS objects that actually exist locally but are momentarily unreadable. The fix is straightforward: check err instanceof Deno.errors.NotFound before populating toDelete:

   } catch (err) {
     if (!(err instanceof Deno.errors.NotFound)) throw err; // re-throw unexpected errors
     if (this.index) { ... }
   }
   

   The previous code silently swallowed all errors here (prior comment: "Path may have been deleted between markDirty and push"), so this is not a regression, but now that the catch block does real work the distinction matters.

2. deleteObject on GCS returns 404 for already-deleted objects; concurrent callers will fail (gcs_cache_sync.ts delete loop, ~line 1275): GCS DELETE returns HTTP 404 when the object does not exist, which the GCS client surfaces as NotFoundError. isRetryableError treats NotFoundError as terminal and non-retryable, so if two concurrent pushChanged calls race to delete the same set of dirty-then-absent paths, the second caller will collect NotFoundError entries in deleteFailures and throw a batch-delete error rather than succeeding silently. Silently ignoring NotFoundError inside the delete loop (or inside the retryWithBackoff wrapper) would make deletion idempotent:

   batch.map((key) =>
     retryWithBackoff(
       () => this.gcs.deleteObject(key, undefined, signal),
       { signal },
     ).catch((err) => {
       if (err instanceof NotFoundError) return; // already gone, idempotent
       throw err;
     })
   ),
   

3. No test exercises the delete-batch-failure message path: The formatBatchFailure function now handles "delete" as an operation type, and the mock's deleteFailures map supports injecting per-key failures. A test that injects deleteFailures for one or more keys and verifies err.message contains "Failed to delete" would complete the coverage added in DEF-2 for the push case.

## Code Review ### Blocking Issues None. ### Suggestions 1. **Scoped walk catch block catches all `Deno.stat` errors, not just `NotFound`** (`gcs_cache_sync.ts`, the catch block in the scoped walk added by this PR): If `Deno.stat(absPath)` fails for a reason other than `NotFound` — for example a transient `PermissionDenied` while the cache directory is temporarily inaccessible — the code currently queues all matching index entries for remote deletion. This could delete GCS objects that actually exist locally but are momentarily unreadable. The fix is straightforward: check `err instanceof Deno.errors.NotFound` before populating `toDelete`: ```typescript } catch (err) { if (!(err instanceof Deno.errors.NotFound)) throw err; // re-throw unexpected errors if (this.index) { ... } } ``` The previous code silently swallowed all errors here (prior comment: "Path may have been deleted between markDirty and push"), so this is not a regression, but now that the catch block does real work the distinction matters. 2. **`deleteObject` on GCS returns 404 for already-deleted objects; concurrent callers will fail** (`gcs_cache_sync.ts` delete loop, ~line 1275): GCS DELETE returns HTTP 404 when the object does not exist, which the GCS client surfaces as `NotFoundError`. `isRetryableError` treats `NotFoundError` as terminal and non-retryable, so if two concurrent `pushChanged` calls race to delete the same set of dirty-then-absent paths, the second caller will collect `NotFoundError` entries in `deleteFailures` and throw a batch-delete error rather than succeeding silently. Silently ignoring `NotFoundError` inside the delete loop (or inside the retryWithBackoff wrapper) would make deletion idempotent: ```typescript batch.map((key) => retryWithBackoff( () => this.gcs.deleteObject(key, undefined, signal), { signal }, ).catch((err) => { if (err instanceof NotFoundError) return; // already gone, idempotent throw err; }) ), ``` 3. **No test exercises the delete-batch-failure message path**: The `formatBatchFailure` function now handles `"delete"` as an operation type, and the mock's `deleteFailures` map supports injecting per-key failures. A test that injects `deleteFailures` for one or more keys and verifies `err.message` contains `"Failed to delete"` would complete the coverage added in DEF-2 for the push case.

stack72 merged commit c3d7ccdd6f into main 2026-06-24 15:16:04 +00:00

stack72 deleted branch feat/gcs-pushchanged-absence-deletion 2026-06-24 15:16:04 +00:00

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

feat(gcs-datastore): implement absence-on-disk deletion in pushChanged (swamp-club#798) (#71)

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!71

No description provided.