fix(datastores): pullChanged skips and prunes dangling index entries (#867) #83

Merged

stack72 merged 3 commits from worktree-867 into main 2026-06-29 23:00:42 +00:00

Conversation 4 Commits 3 Files changed 6 +269 -26

stack72 commented 2026-06-29 22:11:09 +00:00

Owner

Copy link

Summary

• pullChanged() in both S3 and GCS datastore extensions now treats NotFound/NoSuchKey errors during download as recoverable dangling index entries — removes them from the index, writes the cleaned index back to the remote, and continues pulling remaining files
• Non-404 errors (auth, network, 5xx) remain fatal — no change to error handling for genuine failures
• Fixes the scenario from swamp-club #867 where all write-path commands (gc, sync, model method run, compact) were blocked because the pre-sync pull aborted on 9,956 dangling index entries with no built-in recovery

Changes

• datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts — detect NotFound/NoSuchKey in download batch loop (matching hydrateFile() precedent), prune entry, set indexMutated, expand local rewrite condition to (pulled > 0 || indexMutated), push cleaned index back to S3
• datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts — identical fix using NotFoundError class
• 2 new tests per extension: dangling entry pruning + non-404 errors remain fatal
• Manifest version bump to 2026.06.30.1 for both extensions

Test plan

• Unit tests: 99 S3 tests pass, 96 GCS tests pass (0 failures)
• Type check, lint, fmt all clean
• E2E: MinIO — injected index with 3 dangling entries, swamp datastore sync pulled 2 real files with 0 errors, remote index self-healed, follow-up sync clean
• E2E: fake-gcs-server — injected index with 2 dangling entries, same result
• Both e2e tests used swamp extension source add pointing to the modified local extension

## Summary - **pullChanged()** in both S3 and GCS datastore extensions now treats NotFound/NoSuchKey errors during download as recoverable dangling index entries — removes them from the index, writes the cleaned index back to the remote, and continues pulling remaining files - Non-404 errors (auth, network, 5xx) remain fatal — no change to error handling for genuine failures - Fixes the scenario from swamp-club #867 where all write-path commands (`gc`, `sync`, `model method run`, `compact`) were blocked because the pre-sync pull aborted on 9,956 dangling index entries with no built-in recovery ## Changes - `datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts` — detect NotFound/NoSuchKey in download batch loop (matching `hydrateFile()` precedent), prune entry, set `indexMutated`, expand local rewrite condition to `(pulled > 0 || indexMutated)`, push cleaned index back to S3 - `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts` — identical fix using `NotFoundError` class - 2 new tests per extension: dangling entry pruning + non-404 errors remain fatal - Manifest version bump to 2026.06.30.1 for both extensions ## Test plan - [x] Unit tests: 99 S3 tests pass, 96 GCS tests pass (0 failures) - [x] Type check, lint, fmt all clean - [x] E2E: MinIO — injected index with 3 dangling entries, `swamp datastore sync` pulled 2 real files with 0 errors, remote index self-healed, follow-up sync clean - [x] E2E: fake-gcs-server — injected index with 2 dangling entries, same result - [x] Both e2e tests used `swamp extension source add` pointing to the modified local extension

stack72 added 1 commit 2026-06-29 22:11:09 +00:00

fix(datastores): pullChanged skips and prunes dangling index entries instead of aborting (#867) ... cc4c13c60f

When the remote index references S3/GCS objects that no longer exist
(e.g. after a gc that deleted objects but left stale index entries),
pullChanged now treats NotFound errors as recoverable: removes the
dangling entry from the index, writes the cleaned index back to the
remote, and continues pulling remaining files. Non-404 errors remain
fatal. Fixes the scenario where all write-path commands (gc, sync,
model method run, compact) were blocked with no built-in recovery.

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

stack72 commented 2026-06-29 22:23:50 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. Stale fingerprint passed to markSynced after remote index writeback defeats fast path (both files, identical bug)

   GCS — gcs_cache_sync.ts:1079-1086: When indexMutated is true from pruning dangling entries, the new code writes the cleaned index to GCS via putObject, but discards the PUT response. It then calls markSynced(indexGeneration) at line 1086 with the old generation captured from the original pullIndex GET. The putObject changed the remote index's generation — so the sidecar now records a stale fingerprint. The next tryFastPullChanged HEADs the remote, sees the new generation, mismatches against the sidecar's old generation, and falls through to the slow path every time.

   S3 — s3_cache_sync.ts:1198-1205: Same bug. The PUT response's ETag is discarded and markSynced(indexETag) at line 1205 records the pre-PUT ETag.

   Contrast with pushChanged: The existing pushChanged writeback (GCS line 1352, S3 line 1485) correctly captures putResult.generation/putResult.etag from the PUT response and passes it to markSynced. The new pullChanged writeback doesn't follow this pattern.

   Breaking example: A repo with 1 dangling index entry out of 4000. pullChanged prunes it and writes back the cleaned index. markSynced records the old generation. Every subsequent pullChanged misses the fast path and does a full 1+ MB index GET + 4000-stat walk — exactly the performance bug the fast path was built to avoid (lab/164). This repeats on every sync until something else (e.g., pushChanged) writes a new sidecar with the correct fingerprint.

   Existing code documents this exact invariant — markSynced's comment (GCS line 587-594, S3 line 647-649) says: "remoteIndexGeneration MUST come from the same GET response that delivered the bytes we verified the local cache against, OR from our own putObject response for the bytes we just wrote." The new code violates the second clause.

   Suggested fix (GCS):

   if (this.indexMutated) {
     const indexData = new TextEncoder().encode(
       JSON.stringify(this.index, null, 2),
     );
     const putResult = await retryWithBackoff(
       () => this.gcs.putObject(this.indexKey(), indexData, signal),
       { signal },
     );
     this.indexMutated = false;
     indexGeneration = putResult?.generation ?? indexGeneration;
   }
   

   (And the same for S3, capturing putResult?.etag.)

Low

1. Broad catch block now silently swallows remote index PUT failures, not just sidecar writes

   GCS — gcs_cache_sync.ts:1087-1089, S3 — s3_cache_sync.ts:1206-1211: The catch block was originally documented as "Non-fatal: sidecar update is opportunistic" — protecting only the markSynced sidecar write. The new code adds a putObject (remote index writeback) inside the same try block, so a persistent PUT failure (e.g., credential revocation mid-sync) is silently swallowed. The cleaned index is written locally but the remote retains the dangling entry. Self-heals on the next pullChanged (re-prune, re-try PUT), so this is non-blocking. The comment is now slightly misleading about what it protects.

2. indexMutated flag leaks across pullIndex boundaries

   Neither pullIndex nor its callers reset indexMutated to false when a fresh remote index replaces this.index. If a pullChanged writeback fails (caught by the broad catch), indexMutated stays true. A subsequent pushChanged then calls pullIndex(forceRemote: true), which replaces this.index entirely, but indexMutated is still true from the prior operation. This triggers a redundant writeback of the just-fetched-and-unmodified remote index. No data loss — just a wasted PUT. Pre-existing design; the PR doesn't introduce it but does add a new path that can trigger it.

Verdict

PASS — The core logic (prune dangling NotFound entries, write back cleaned index) is correct and well-tested. The stale-fingerprint issue (Medium #1) defeats the fast-path optimization after a prune but doesn't cause data loss or incorrect behavior — the slow path always produces correct results. The fix is a one-line capture of the PUT response. Tests are thorough and cover both the happy path and the error-propagation path.

## Adversarial Review ### Medium 1. **Stale fingerprint passed to `markSynced` after remote index writeback defeats fast path (both files, identical bug)** **GCS** — `gcs_cache_sync.ts:1079-1086`: When `indexMutated` is true from pruning dangling entries, the new code writes the cleaned index to GCS via `putObject`, but discards the PUT response. It then calls `markSynced(indexGeneration)` at line 1086 with the **old** generation captured from the original `pullIndex` GET. The `putObject` changed the remote index's generation — so the sidecar now records a stale fingerprint. The next `tryFastPullChanged` HEADs the remote, sees the new generation, mismatches against the sidecar's old generation, and falls through to the slow path every time. **S3** — `s3_cache_sync.ts:1198-1205`: Same bug. The PUT response's ETag is discarded and `markSynced(indexETag)` at line 1205 records the pre-PUT ETag. **Contrast with `pushChanged`**: The existing `pushChanged` writeback (GCS line 1352, S3 line 1485) correctly captures `putResult.generation`/`putResult.etag` from the PUT response and passes it to `markSynced`. The new `pullChanged` writeback doesn't follow this pattern. **Breaking example**: A repo with 1 dangling index entry out of 4000. `pullChanged` prunes it and writes back the cleaned index. `markSynced` records the old generation. Every subsequent `pullChanged` misses the fast path and does a full 1+ MB index GET + 4000-stat walk — exactly the performance bug the fast path was built to avoid (lab/164). This repeats on every sync until something else (e.g., `pushChanged`) writes a new sidecar with the correct fingerprint. **Existing code documents this exact invariant** — `markSynced`'s comment (GCS line 587-594, S3 line 647-649) says: *"`remoteIndexGeneration` MUST come from the same GET response that delivered the bytes we verified the local cache against, OR from our own putObject response for the bytes we just wrote."* The new code violates the second clause. **Suggested fix** (GCS): ```typescript if (this.indexMutated) { const indexData = new TextEncoder().encode( JSON.stringify(this.index, null, 2), ); const putResult = await retryWithBackoff( () => this.gcs.putObject(this.indexKey(), indexData, signal), { signal }, ); this.indexMutated = false; indexGeneration = putResult?.generation ?? indexGeneration; } ``` (And the same for S3, capturing `putResult?.etag`.) ### Low 1. **Broad `catch` block now silently swallows remote index PUT failures, not just sidecar writes** **GCS** — `gcs_cache_sync.ts:1087-1089`, **S3** — `s3_cache_sync.ts:1206-1211`: The `catch` block was originally documented as "Non-fatal: sidecar update is opportunistic" — protecting only the `markSynced` sidecar write. The new code adds a `putObject` (remote index writeback) inside the same try block, so a persistent PUT failure (e.g., credential revocation mid-sync) is silently swallowed. The cleaned index is written locally but the remote retains the dangling entry. Self-heals on the next `pullChanged` (re-prune, re-try PUT), so this is non-blocking. The comment is now slightly misleading about what it protects. 2. **`indexMutated` flag leaks across `pullIndex` boundaries** Neither `pullIndex` nor its callers reset `indexMutated` to `false` when a fresh remote index replaces `this.index`. If a `pullChanged` writeback fails (caught by the broad `catch`), `indexMutated` stays `true`. A subsequent `pushChanged` then calls `pullIndex(forceRemote: true)`, which replaces `this.index` entirely, but `indexMutated` is still `true` from the prior operation. This triggers a redundant writeback of the just-fetched-and-unmodified remote index. No data loss — just a wasted PUT. Pre-existing design; the PR doesn't introduce it but does add a new path that can trigger it. ### Verdict **PASS** — The core logic (prune dangling NotFound entries, write back cleaned index) is correct and well-tested. The stale-fingerprint issue (Medium #1) defeats the fast-path optimization after a prune but doesn't cause data loss or incorrect behavior — the slow path always produces correct results. The fix is a one-line capture of the PUT response. Tests are thorough and cover both the happy path and the error-propagation path.

stack72 commented 2026-06-29 22:26:12 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

1. GCS bulk-walk deletion gate uses bulkInvalidated instead of an overflow-specific flag, causing a data-loss divergence from S3 (gcs_cache_sync.ts:1267)

   In pushChanged's full-walk branch, GCS gates remote-object deletion on this.bulkInvalidated:

   // gcs_cache_sync.ts:1267
   if (this.bulkInvalidated && this.index) {
     for (const key of Object.keys(this.index.entries)) {
       if (isInternalCacheFile(key)) continue;
       if (localFiles.has(key)) continue;
       if (this.lazyPullActive && isLazySkippable(key)) continue;
       toDelete.push(key);
     }
   }
   

   bulkInvalidated is set to true by markDirty() called with no relPath — and pushFile calls exactly that:

   // gcs_cache_sync.ts:1119
   async pushFile(relativePath: string, signal?: AbortSignal): Promise<void> {
     await this.markDirty();  // ← no relPath → sets bulkInvalidated = true in sidecar
     ...
   }
   

   This creates a silent data-loss scenario across process boundaries:

   • Process A calls service.pushFile("data/foo.yaml") → sidecar written with bulkInvalidated: true.
   • Process B (fresh GcsCacheSyncService) calls pushChanged() → loads sidecar → bulkInvalidated = true → full walk → all remote-only GCS objects not present locally are deleted.

   The S3 sibling implementation explicitly avoids this by using a separate dirtyPathsOverflowed field (only set when the 200-path cap overflows) as the deletion gate, and its code comments state directly:

   "Only when per-path dirty tracking overflowed — a no-path markDirty() is a modification signal, not a deletion signal."
   (s3_cache_sync.ts:1392)

   GCS has no dirtyPathsOverflowed field. The GCS test "pushChanged: bulk walk deletes orphaned index entries after markDirty()" explicitly validates deletions triggered by a no-relPath markDirty(), which confirms the divergence is present but the cross-process pushFile → pushChanged danger is untested (the test uses the same service instance, so toPush/toDelete are computed before bulkInvalidated is set by the upload batch).

   Fix: mirror S3's design — add a dirtyPathsOverflowed field to DatastoreSyncStateV2 in GCS, set it only on cap overflow in markDirty, and gate bulk-walk deletions on dirtyPathsOverflowed (not bulkInvalidated). Update markSynced to clear it. This brings GCS semantics in line with S3's documented intent.

Suggestions

1. writePartitionedIndex in GCS uses Array<Promise<unknown>> instead of Promise<void>[] (gcs_cache_sync.ts:1502): The S3 sibling chains .then(() => {}) before .catch(() => {}) to produce Promise<void>[]. GCS only chains .catch(() => {}) on a Promise<GcsWriteResult>, giving Array<Promise<GcsWriteResult | undefined>>. The runtime behaviour (all errors swallowed, Promise.allSettled used) is identical, but the type annotation is less precise than S3's.

2. Combined try-catch in pullChanged swallows atomicWriteTextFile errors (gcs_cache_sync.ts:1069–1090): The single try block wraps both the local index rewrite and markSynced. A disk-full or permission error during atomicWriteTextFile(this.indexPath, ...) is silently swallowed with the comment "Non-fatal: sidecar update is opportunistic." The local index rewrite is more than just the sidecar — losing it means the next process's slow-path walk pulls incorrect localMtime values, causing unnecessary re-uploads (swamp-club #222 scenario). The same pattern exists in S3, so this isn't new to this PR, but it's worth splitting the try blocks in both implementations: let the atomicWriteTextFile propagate (it's load-bearing), and only wrap markSynced in the opportunistic catch.

## Code Review ### Blocking Issues 1. **GCS bulk-walk deletion gate uses `bulkInvalidated` instead of an overflow-specific flag, causing a data-loss divergence from S3 (`gcs_cache_sync.ts:1267`)** In `pushChanged`'s full-walk branch, GCS gates remote-object deletion on `this.bulkInvalidated`: ```typescript // gcs_cache_sync.ts:1267 if (this.bulkInvalidated && this.index) { for (const key of Object.keys(this.index.entries)) { if (isInternalCacheFile(key)) continue; if (localFiles.has(key)) continue; if (this.lazyPullActive && isLazySkippable(key)) continue; toDelete.push(key); } } ``` `bulkInvalidated` is set to `true` by `markDirty()` called with **no `relPath`** — and `pushFile` calls exactly that: ```typescript // gcs_cache_sync.ts:1119 async pushFile(relativePath: string, signal?: AbortSignal): Promise<void> { await this.markDirty(); // ← no relPath → sets bulkInvalidated = true in sidecar ... } ``` This creates a silent data-loss scenario across process boundaries: - Process A calls `service.pushFile("data/foo.yaml")` → sidecar written with `bulkInvalidated: true`. - Process B (fresh `GcsCacheSyncService`) calls `pushChanged()` → loads sidecar → `bulkInvalidated = true` → full walk → **all remote-only GCS objects not present locally are deleted**. The S3 sibling implementation explicitly avoids this by using a separate `dirtyPathsOverflowed` field (only set when the 200-path cap overflows) as the deletion gate, and its code comments state directly: > "Only when per-path dirty tracking overflowed — a no-path markDirty() is a modification signal, not a deletion signal." > (`s3_cache_sync.ts:1392`) GCS has no `dirtyPathsOverflowed` field. The GCS test `"pushChanged: bulk walk deletes orphaned index entries after markDirty()"` explicitly validates deletions triggered by a no-relPath `markDirty()`, which confirms the divergence is present but the cross-process `pushFile → pushChanged` danger is untested (the test uses the same service instance, so `toPush`/`toDelete` are computed before `bulkInvalidated` is set by the upload batch). **Fix**: mirror S3's design — add a `dirtyPathsOverflowed` field to `DatastoreSyncStateV2` in GCS, set it only on cap overflow in `markDirty`, and gate bulk-walk deletions on `dirtyPathsOverflowed` (not `bulkInvalidated`). Update `markSynced` to clear it. This brings GCS semantics in line with S3's documented intent. ### Suggestions 1. **`writePartitionedIndex` in GCS uses `Array<Promise<unknown>>` instead of `Promise<void>[]` (`gcs_cache_sync.ts:1502`)**: The S3 sibling chains `.then(() => {})` before `.catch(() => {})` to produce `Promise<void>[]`. GCS only chains `.catch(() => {})` on a `Promise<GcsWriteResult>`, giving `Array<Promise<GcsWriteResult | undefined>>`. The runtime behaviour (all errors swallowed, `Promise.allSettled` used) is identical, but the type annotation is less precise than S3's. 2. **Combined try-catch in `pullChanged` swallows `atomicWriteTextFile` errors (`gcs_cache_sync.ts:1069–1090`)**: The single try block wraps both the local index rewrite *and* `markSynced`. A disk-full or permission error during `atomicWriteTextFile(this.indexPath, ...)` is silently swallowed with the comment "Non-fatal: sidecar update is opportunistic." The local index rewrite is more than just the sidecar — losing it means the next process's slow-path walk pulls incorrect `localMtime` values, causing unnecessary re-uploads (swamp-club #222 scenario). The same pattern exists in S3, so this isn't new to this PR, but it's worth splitting the try blocks in both implementations: let the `atomicWriteTextFile` propagate (it's load-bearing), and only wrap `markSynced` in the opportunistic catch.

stack72 added 1 commit 2026-06-29 22:29:49 +00:00

fix(datastores): capture PUT response fingerprint after dangling-entry writeback ... 60a9a71cec

The initial implementation discarded the putObject response when writing
the cleaned index back to S3/GCS, then passed the stale pre-PUT
ETag/generation to markSynced. This defeated the fast-path optimization
on every subsequent sync until something else rewrote the sidecar.
Now captures putResult.etag (S3) / putResult.generation (GCS) and
updates the fingerprint before markSynced, matching the pushChanged
writeback pattern.

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

stack72 added 1 commit 2026-06-29 22:36:47 +00:00

fix(gcs): gate bulk-walk deletion on dirtyPathsOverflowed, not bulkInvalidated ... 9c25861e75

Mirrors S3's design: a no-relPath markDirty() is a modification signal,
not a deletion signal. Previously GCS gated orphan deletion on
bulkInvalidated, which is set by pushFile's no-relPath markDirty call.
A cross-process pushFile → pushChanged sequence could delete remote-only
objects. Now only dirty-path cap overflow triggers deletion, matching
S3's documented intent.

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

stack72 commented 2026-06-29 22:46:23 +00:00

Author

Owner

Copy link

Code Review

Three fixes bundled in this PR:

1. GCS: dirtyPathsOverflowed field added to GCS sidecar state + orphan-scan gate changed from bulkInvalidated to dirtyPathsOverflowed && !lazyPullActive
2. Both: pullChanged now skips and prunes dangling index entries (404 on pull → delete from index, not abort)
3. Both: pullChanged captures the PUT response fingerprint after a dangling-entry writeback instead of using the stale pre-scrub generation/ETag

All three fixes are correct and well-motivated. Testing rules followed throughout (in-memory mocks, local HTTP servers via Deno.serve({ port: 0 }); env vars restored in finally; sanitizeResources: false on TCP-connection tests with an explanatory comment). No any types in hand-written code; named exports only; no model files touched; version bumps in manifest.yaml are consistent with the changes.

Blocking Issues

None.

Suggestions

1. Dead inner guard in gcs_cache_sync.ts line 1281 — The PR changed the outer condition from this.bulkInvalidated to this.dirtyPathsOverflowed && !this.lazyPullActive, making the pre-existing inner guard unreachable:

   // outer: dirtyPathsOverflowed && !this.lazyPullActive && this.index
   if (this.lazyPullActive && isLazySkippable(key)) continue; // always false
   

   Because the outer condition already requires !this.lazyPullActive, the inner this.lazyPullActive && … branch can never execute. The comment above it ("Without this check, a bulk push after a lazy pull would delete every un-hydrated raw file from the remote") is now misleading — the outer !this.lazyPullActive is what enforces that invariant. The S3 sibling (which already had the correct outer condition) has no inner guard. Consider removing the inner if and its comment to match S3.

2. del3 GCS test comment could explain why the test actually works — The test "pushChanged: lazy hydration guard prevents deletion of un-hydrated files" calls markDirty() (no relPath), which sets bulkInvalidated = true but leaves dirtyPathsOverflowed = false. So the orphan-scan block (dirtyPathsOverflowed && !lazyPullActive) never executes — the test passes because dirtyPathsOverflowed is false, not because lazyPullActive guards the inner check. A short inline note would prevent a future reader from concluding the inner guard is load-bearing.

## Code Review Three fixes bundled in this PR: 1. **GCS**: `dirtyPathsOverflowed` field added to GCS sidecar state + orphan-scan gate changed from `bulkInvalidated` to `dirtyPathsOverflowed && !lazyPullActive` 2. **Both**: `pullChanged` now skips and prunes dangling index entries (404 on pull → delete from index, not abort) 3. **Both**: `pullChanged` captures the PUT response fingerprint after a dangling-entry writeback instead of using the stale pre-scrub generation/ETag All three fixes are correct and well-motivated. Testing rules followed throughout (in-memory mocks, local HTTP servers via `Deno.serve({ port: 0 })`; env vars restored in `finally`; `sanitizeResources: false` on TCP-connection tests with an explanatory comment). No `any` types in hand-written code; named exports only; no model files touched; version bumps in `manifest.yaml` are consistent with the changes. ### Blocking Issues None. ### Suggestions 1. **Dead inner guard in `gcs_cache_sync.ts` line 1281** — The PR changed the outer condition from `this.bulkInvalidated` to `this.dirtyPathsOverflowed && !this.lazyPullActive`, making the pre-existing inner guard unreachable: ```typescript // outer: dirtyPathsOverflowed && !this.lazyPullActive && this.index if (this.lazyPullActive && isLazySkippable(key)) continue; // always false ``` Because the outer condition already requires `!this.lazyPullActive`, the inner `this.lazyPullActive && …` branch can never execute. The comment above it ("Without this check, a bulk push after a lazy pull would delete every un-hydrated raw file from the remote") is now misleading — the outer `!this.lazyPullActive` is what enforces that invariant. The S3 sibling (which already had the correct outer condition) has no inner guard. Consider removing the inner `if` and its comment to match S3. 2. **`del3` GCS test comment could explain why the test actually works** — The test `"pushChanged: lazy hydration guard prevents deletion of un-hydrated files"` calls `markDirty()` (no relPath), which sets `bulkInvalidated = true` but leaves `dirtyPathsOverflowed = false`. So the orphan-scan block (`dirtyPathsOverflowed && !lazyPullActive`) never executes — the test passes because `dirtyPathsOverflowed` is false, not because `lazyPullActive` guards the inner check. A short inline note would prevent a future reader from concluding the inner guard is load-bearing.

stack72 commented 2026-06-29 22:49:46 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. Dead code: isLazySkippable guard is unreachable — gcs_cache_sync.ts:1281

   The PR changed the outer condition at line 1274 from this.bulkInvalidated && this.index to this.dirtyPathsOverflowed && !this.lazyPullActive && this.index. The inner guard at line 1281:

   if (this.lazyPullActive && isLazySkippable(key)) continue;
   

   is now unreachable because the outer !this.lazyPullActive guarantees this.lazyPullActive is false inside the block. Before the PR this guard was live (the old outer condition didn't check lazyPullActive).

   Impact: No behavioral bug — the outer guard is strictly more conservative (skips ALL orphan deletion when lazy pull is active, vs. the old code which only skipped isLazySkippable keys). But the dead branch is misleading to future readers who may think the isLazySkippable filter is the intended behavior boundary.

   Suggested fix: Remove the dead if (this.lazyPullActive && isLazySkippable(key)) continue; line at 1281, or add a comment that the outer guard supersedes it.

Low

2. Scoped/partitioned pull doesn't persist dangling-entry cleanup — gcs_cache_sync.ts:1075, s3_cache_sync.ts:1187

   When pullChanged uses the partitioned index path (models filter), indexGeneration/indexETag is null. The writeback block (if (indexGeneration) / if (indexETag)) is skipped entirely, so dangling entries pruned from the in-memory index are never written back to local disk or remote. The same dangling entry will be re-encountered on every scoped pull without self-healing — only a full (non-scoped) pull persists the cleanup.

   This follows the pre-existing pattern (the pulled > 0 writeback also gated on the same condition before the PR), so it's consistent. Self-heals on the next full pull.

3. Double serialization in index writeback — gcs_cache_sync.ts:1080+1084, s3_cache_sync.ts:1191+1196

   JSON.stringify(this.index, null, 2) is called twice in quick succession — once for atomicWriteTextFile (local) and again for TextEncoder.encode (remote upload). Both produce identical output from the same in-memory object. For large indexes (the code comments reference 1.37 MB), serializing twice is wasteful. A single const indexJson = JSON.stringify(...) with new TextEncoder().encode(indexJson) for the upload and indexJson for the file write would halve the serialization cost. Not a correctness issue.

Verdict

PASS — The changes are logically sound.

The behavioral fix (gating bulk-walk orphan deletion on dirtyPathsOverflowed instead of bulkInvalidated) correctly distinguishes between "too many dirty paths to track individually" (overflow → orphan deletion appropriate) and "bare markDirty() from pushFile or explicit bulk signal" (modification intent → orphan deletion inappropriate). This closes the data-loss vector where a reader-side repo with a sparse local cache would have its remote files deleted by a pushChanged triggered via pushFile.

The dangling-entry pruning in pullChanged correctly identifies NotFoundError (GCS) / NoSuchKey/NotFound (S3) vs. other errors, prunes the in-memory index, writes the cleaned index back to both local and remote, and updates the sidecar generation/ETag from the PUT response. Error-path tests cover the non-404 case. The retryWithBackoff correctly does not retry NotFound errors (non-retryable), so the error surfaces without masking.

Test coverage is good: happy-path dangling pruning, non-NotFound error propagation, and the behavioral change for bare markDirty() are all tested for both GCS and S3.

## Adversarial Review ### Medium 1. **Dead code: `isLazySkippable` guard is unreachable** — `gcs_cache_sync.ts:1281` The PR changed the outer condition at line 1274 from `this.bulkInvalidated && this.index` to `this.dirtyPathsOverflowed && !this.lazyPullActive && this.index`. The inner guard at line 1281: ```ts if (this.lazyPullActive && isLazySkippable(key)) continue; ``` is now unreachable because the outer `!this.lazyPullActive` guarantees `this.lazyPullActive` is `false` inside the block. Before the PR this guard was live (the old outer condition didn't check `lazyPullActive`). **Impact:** No behavioral bug — the outer guard is strictly more conservative (skips ALL orphan deletion when lazy pull is active, vs. the old code which only skipped `isLazySkippable` keys). But the dead branch is misleading to future readers who may think the `isLazySkippable` filter is the intended behavior boundary. **Suggested fix:** Remove the dead `if (this.lazyPullActive && isLazySkippable(key)) continue;` line at 1281, or add a comment that the outer guard supersedes it. ### Low 2. **Scoped/partitioned pull doesn't persist dangling-entry cleanup** — `gcs_cache_sync.ts:1075`, `s3_cache_sync.ts:1187` When `pullChanged` uses the partitioned index path (models filter), `indexGeneration`/`indexETag` is `null`. The writeback block (`if (indexGeneration)` / `if (indexETag)`) is skipped entirely, so dangling entries pruned from the in-memory index are never written back to local disk or remote. The same dangling entry will be re-encountered on every scoped pull without self-healing — only a full (non-scoped) pull persists the cleanup. This follows the pre-existing pattern (the `pulled > 0` writeback also gated on the same condition before the PR), so it's consistent. Self-heals on the next full pull. 3. **Double serialization in index writeback** — `gcs_cache_sync.ts:1080+1084`, `s3_cache_sync.ts:1191+1196` `JSON.stringify(this.index, null, 2)` is called twice in quick succession — once for `atomicWriteTextFile` (local) and again for `TextEncoder.encode` (remote upload). Both produce identical output from the same in-memory object. For large indexes (the code comments reference 1.37 MB), serializing twice is wasteful. A single `const indexJson = JSON.stringify(...)` with `new TextEncoder().encode(indexJson)` for the upload and `indexJson` for the file write would halve the serialization cost. Not a correctness issue. ### Verdict **PASS** — The changes are logically sound. The behavioral fix (gating bulk-walk orphan deletion on `dirtyPathsOverflowed` instead of `bulkInvalidated`) correctly distinguishes between "too many dirty paths to track individually" (overflow → orphan deletion appropriate) and "bare `markDirty()` from `pushFile` or explicit bulk signal" (modification intent → orphan deletion inappropriate). This closes the data-loss vector where a reader-side repo with a sparse local cache would have its remote files deleted by a `pushChanged` triggered via `pushFile`. The dangling-entry pruning in `pullChanged` correctly identifies `NotFoundError` (GCS) / `NoSuchKey`/`NotFound` (S3) vs. other errors, prunes the in-memory index, writes the cleaned index back to both local and remote, and updates the sidecar generation/ETag from the PUT response. Error-path tests cover the non-404 case. The `retryWithBackoff` correctly does not retry NotFound errors (non-retryable), so the error surfaces without masking. Test coverage is good: happy-path dangling pruning, non-NotFound error propagation, and the behavioral change for bare `markDirty()` are all tested for both GCS and S3.

stack72 merged commit 9cf74da27b into main 2026-06-29 23:00:42 +00:00

stack72 deleted branch worktree-867 2026-06-29 23:00:42 +00:00

stack72 referenced this pull request from a commit 2026-06-29 23:00:44 +00:00

fix(datastores): pullChanged skips and prunes dangling index entries (#867) (#83)

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

No description provided.