fix(datastores): complete v2 shard-first migration for write-side sync paths (#1052) #97

Merged

stack72 merged 4 commits from fix/1052-v2-shard-first-write-paths into main 2026-07-09 20:43:40 +00:00

Conversation 6 Commits 4 Files changed 8 +1027 -93

stack72 commented 2026-07-09 19:21:40 +00:00

Owner

Copy link

Summary

• commitPush no-op: reads 13KB _meta.json instead of 13.9MB monolith on v2 repos. Drops from ~38s to ~3ms on the reporter's setup.
• pushChanged slow path: assembles index from shards instead of fetching monolith. Writeback writes only dirty shards + bumps commitSeq instead of re-uploading the full monolith.
• preparePush slow path: assembles index from shards instead of fetching monolith.
• All three paths clear dirty state (dirtyPaths, bulkInvalidated) before writing the sidecar — prevents stale paths leaking across service instances.
• V1 repos are completely unaffected — every change adds a v2 guard with v1 fallback.
• 6 new v2 benchmark scenarios against MinIO cover push, no-op push, two-phase no-op, two-phase push, cold pull, and no-op pull.

v1 vs v2 benchmark results (MinIO, 1000 files / 50 models)

Scenario	v1	v2
Push 1 modified / 1000	53ms	68ms
No-op push (fast path)	5ms	4ms
Two-phase no-op	—	5ms
Two-phase push 1 modified	—	48ms
Pull 1000 cold	458ms	492ms
No-op pull (fast path)	3ms	7ms

v2 slow paths are slightly slower on local MinIO (more S3 round trips for shard assembly) but massively faster on slow links where the monolith transfer dominates.

Test plan

• 204 unit tests pass (136 sync tests + 68 client/lock/conformance)
• 3 new unit tests verify monolith is never fetched on v2 repos
• 15 benchmark scenarios pass against MinIO (9 v1 + 6 v2)
• Integration test against MinIO: push v1 → migrate → v2 push/pull cycle → fresh pull verifies data integrity (11/11)
• deno check, deno lint, deno fmt --check, deno install --frozen all clean

Closes swamp-club#1052

## Summary - **commitPush no-op**: reads 13KB `_meta.json` instead of 13.9MB monolith on v2 repos. Drops from ~38s to ~3ms on the reporter's setup. - **pushChanged slow path**: assembles index from shards instead of fetching monolith. Writeback writes only dirty shards + bumps `commitSeq` instead of re-uploading the full monolith. - **preparePush slow path**: assembles index from shards instead of fetching monolith. - All three paths clear dirty state (`dirtyPaths`, `bulkInvalidated`) before writing the sidecar — prevents stale paths leaking across service instances. - V1 repos are completely unaffected — every change adds a v2 guard with v1 fallback. - 6 new v2 benchmark scenarios against MinIO cover push, no-op push, two-phase no-op, two-phase push, cold pull, and no-op pull. ## v1 vs v2 benchmark results (MinIO, 1000 files / 50 models) | Scenario | v1 | v2 | |---|---|---| | Push 1 modified / 1000 | 53ms | 68ms | | No-op push (fast path) | 5ms | 4ms | | Two-phase no-op | — | 5ms | | Two-phase push 1 modified | — | 48ms | | Pull 1000 cold | 458ms | 492ms | | No-op pull (fast path) | 3ms | 7ms | v2 slow paths are slightly slower on local MinIO (more S3 round trips for shard assembly) but massively faster on slow links where the monolith transfer dominates. ## Test plan - [x] 204 unit tests pass (136 sync tests + 68 client/lock/conformance) - [x] 3 new unit tests verify monolith is never fetched on v2 repos - [x] 15 benchmark scenarios pass against MinIO (9 v1 + 6 v2) - [x] Integration test against MinIO: push v1 → migrate → v2 push/pull cycle → fresh pull verifies data integrity (11/11) - [x] `deno check`, `deno lint`, `deno fmt --check`, `deno install --frozen` all clean Closes swamp-club#1052

stack72 added 1 commit 2026-07-09 19:21:41 +00:00

fix(datastores): complete v2 shard-first migration for write-side sync paths (#1052) ... 572da2d74c

Three sync operations still fetched the full monolithic index on v2 repos
instead of using the shard/commitSeq fast-path: commitPush no-op
verification, pushChanged slow path, and preparePush slow path.
Additionally, pushChanged wrote the monolith back unconditionally.

For each path, try assembleIndexFromShards() first and fall back to the
monolith only for v1 repos — the same pattern pullChanged already uses.
The pushChanged writeback on v2 now writes only dirty shards, bumps
commitSeq, and updates the sidecar with commitSeq instead of the
monolith ETag. Dirty state (dirtyPaths, bulkInvalidated) is cleared
before writing the sidecar to prevent stale paths leaking across
service instances.

Add 6 v2 benchmark scenarios against MinIO (push, no-op push,
two-phase no-op, two-phase push, cold pull, no-op pull) to catch
regressions in either index path.

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

stack72 commented 2026-07-09 19:32:05 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

1. s3_cache_sync.ts:2178-2196 — commitPush v2 no-op marks sidecar clean without verifying local cache completeness

   The v1 no-op path (line 2200-2207) calls localHasAllRemoteEntries() before marking the sidecar clean — this is a correctness guard against the swamp-club#1225 data-loss scenario where a fresh reader with an incomplete cache gets marked clean, causing the next pullChanged to fast-path past unfetched files.

   The new v2 no-op path (line 2184-2191) unconditionally clears dirtyPaths, sets localDirty: false, and writes the sidecar — without any localHasAllRemoteEntries() check.

   Breaking scenario: A fresh reader runs pullChanged (metadata-only or interrupted), then does a two-phase push with nothing dirty. preparePush returns pushed=0, deleted=0. commitPush enters the v2 no-op branch, writes a clean sidecar with commitSeq. The next pullChanged sees commitSeq matches remote _meta.json, fast-paths to 0, and the reader never downloads the missing files.

   This is the exact same data-loss class that swamp-club#1225 addressed for v1 — the v2 path skips the safety check.

   Severity: CRITICAL — silent data loss on the pull path for fresh readers on v2 repos.

   Suggested fix: Add the localHasAllRemoteEntries() guard before writing the sidecar in the v2 no-op commitPush path, mirroring what the pushChanged v2 paths already do (lines 1844 and 1895).

   if (noopMeta && noopMeta.version === 2) {
     try {
       if (await this.localHasAllRemoteEntries()) {
         this.dirtyPaths.clear();
         this.bulkInvalidated = false;
         this.dirtyPathsOverflowed = false;
         const sidecar = this.buildV2State({ localDirty: false });
         sidecar.commitSeq = (noopMeta as PartitionMetaV2).commitSeq;
         sidecar.remoteIndexETag = "";
         await this.writeSyncState(sidecar);
       }
     } catch { /* Non-fatal */ }
     tracePhase("commitPush", commitStart, "v2noop");
     return 0;
   }
   

Medium

1. s3_cache_sync.ts:1818-1835 — v2 writeback partition list derived from groupEntriesByPartition may diverge from shards on disk

   When building newMeta.partitions (line 1828-1831), the code derives the partition list from the in-memory index via groupEntriesByPartition(this.index.entries), not from the shards that were actually written. If dirtyPartitionKeys is non-empty, only those shards are re-written (via writePartitionedIndex with the dirty filter). But the _meta.json partition list reflects ALL in-memory entries — including partitions whose shards predate this push.

   This is consistent with the existing commitPushShardFirst behavior (which also rebuilds from survivingPartitions), so it's likely intentional. However, if a shard was deleted out-of-band (e.g., bucket lifecycle policy), _meta.json will reference a shard that no longer exists, causing assembleIndexFromShards to fall back to monolith on the next read. This is a degraded-performance scenario, not data loss, since the fallback is safe.

   Severity: MEDIUM — degraded performance (fallback to monolith) if shards are externally deleted, but no data loss.

2. s3_cache_sync.ts:1590-1610 — pushChanged shard assembly populates index but preparePush duplicates the logic without sharing v2CommitSeq

   Both pushChanged (line 1590-1610) and preparePush (line 1953-1970) have nearly identical shard-assembly blocks, but preparePush doesn't track v2CommitSeq — the InternalPushManifest doesn't carry it. This means commitPush has to re-read _meta.json (line 2223) even though preparePush already read it. Not a correctness issue since commitPush reads the authoritative remote value, but it's a missed optimization and a divergence point where future changes to one path could forget the other.

   Severity: MEDIUM — not a bug, but a maintenance risk from code duplication.

3. s3_cache_sync_test.ts:5655-5707 — Test #1052: commitPush no-op does not exercise the fresh-reader scenario that the Critical finding above describes

   The test seeds the file locally (seedFile) and runs pullChanged to populate the sidecar before calling preparePush/commitPush. This means localHasAllRemoteEntries() would pass even if it were checked, so the test wouldn't catch the missing guard. A test with an incomplete local cache (e.g., metadata-only pull, or missing files) would be needed to verify the safety check.

   Severity: MEDIUM — test gap directly related to the Critical finding.

Low

1. s3_bench.ts:508 — catch block in scenario 14 setup swallows all errors including permission/IO failures

   } catch { /* empty dir is fine */ }
   

   If Deno.readDir fails for a reason other than an empty/nonexistent directory (e.g., permission denied), the benchmark silently proceeds with stale data from a prior run, producing misleading results. In a benchmark context this is low-risk since it runs in temp dirs, but catch (e) { if (!(e instanceof Deno.errors.NotFound)) throw e; } would be more precise.

   Severity: LOW — benchmark-only, not production code.

2. s3_cache_sync.ts — repeated buildV2State + manual sidecar.commitSeq = X pattern (lines 1848-1851, 1899-1901, 2188-2190)

   The buildV2State helper doesn't accept commitSeq in its overrides, so every call site that needs to set it does sidecar.commitSeq = X after construction. If a future caller forgets this step, the sidecar will have commitSeq: undefined and the fast path will miss. This is a footgun but not a current bug.

   Severity: LOW — maintenance concern, no current breakage.

Verdict

FAIL — The commitPush v2 no-op path unconditionally marks the sidecar clean without verifying that the local cache actually has all remote entries. This is a regression of the swamp-club#1225 data-loss fix for v2 repos. The fix is a one-line guard (if (await this.localHasAllRemoteEntries())) wrapping the sidecar write, matching the pattern already used in pushChanged's v2 paths.

## Adversarial Review ### Critical / High 1. **`s3_cache_sync.ts:2178-2196` — `commitPush` v2 no-op marks sidecar clean without verifying local cache completeness** The v1 no-op path (line 2200-2207) calls `localHasAllRemoteEntries()` before marking the sidecar clean — this is a correctness guard against the swamp-club#1225 data-loss scenario where a fresh reader with an incomplete cache gets marked clean, causing the next `pullChanged` to fast-path past unfetched files. The new v2 no-op path (line 2184-2191) **unconditionally** clears `dirtyPaths`, sets `localDirty: false`, and writes the sidecar — without any `localHasAllRemoteEntries()` check. **Breaking scenario**: A fresh reader runs `pullChanged` (metadata-only or interrupted), then does a two-phase push with nothing dirty. `preparePush` returns `pushed=0, deleted=0`. `commitPush` enters the v2 no-op branch, writes a clean sidecar with `commitSeq`. The next `pullChanged` sees `commitSeq` matches remote `_meta.json`, fast-paths to 0, and the reader never downloads the missing files. This is the exact same data-loss class that swamp-club#1225 addressed for v1 — the v2 path skips the safety check. **Severity: CRITICAL** — silent data loss on the pull path for fresh readers on v2 repos. **Suggested fix**: Add the `localHasAllRemoteEntries()` guard before writing the sidecar in the v2 no-op `commitPush` path, mirroring what the `pushChanged` v2 paths already do (lines 1844 and 1895). ```typescript if (noopMeta && noopMeta.version === 2) { try { if (await this.localHasAllRemoteEntries()) { this.dirtyPaths.clear(); this.bulkInvalidated = false; this.dirtyPathsOverflowed = false; const sidecar = this.buildV2State({ localDirty: false }); sidecar.commitSeq = (noopMeta as PartitionMetaV2).commitSeq; sidecar.remoteIndexETag = ""; await this.writeSyncState(sidecar); } } catch { /* Non-fatal */ } tracePhase("commitPush", commitStart, "v2noop"); return 0; } ``` ### Medium 1. **`s3_cache_sync.ts:1818-1835` — v2 writeback partition list derived from `groupEntriesByPartition` may diverge from shards on disk** When building `newMeta.partitions` (line 1828-1831), the code derives the partition list from the in-memory index via `groupEntriesByPartition(this.index.entries)`, not from the shards that were actually written. If `dirtyPartitionKeys` is non-empty, only those shards are re-written (via `writePartitionedIndex` with the dirty filter). But the _meta.json partition list reflects ALL in-memory entries — including partitions whose shards predate this push. This is consistent with the existing `commitPushShardFirst` behavior (which also rebuilds from `survivingPartitions`), so it's likely intentional. However, if a shard was deleted out-of-band (e.g., bucket lifecycle policy), `_meta.json` will reference a shard that no longer exists, causing `assembleIndexFromShards` to fall back to monolith on the next read. This is a degraded-performance scenario, not data loss, since the fallback is safe. **Severity: MEDIUM** — degraded performance (fallback to monolith) if shards are externally deleted, but no data loss. 2. **`s3_cache_sync.ts:1590-1610` — `pushChanged` shard assembly populates index but `preparePush` duplicates the logic without sharing `v2CommitSeq`** Both `pushChanged` (line 1590-1610) and `preparePush` (line 1953-1970) have nearly identical shard-assembly blocks, but `preparePush` doesn't track `v2CommitSeq` — the `InternalPushManifest` doesn't carry it. This means `commitPush` has to re-read `_meta.json` (line 2223) even though `preparePush` already read it. Not a correctness issue since `commitPush` reads the authoritative remote value, but it's a missed optimization and a divergence point where future changes to one path could forget the other. **Severity: MEDIUM** — not a bug, but a maintenance risk from code duplication. 3. **`s3_cache_sync_test.ts:5655-5707` — Test `#1052: commitPush no-op` does not exercise the fresh-reader scenario that the Critical finding above describes** The test seeds the file locally (`seedFile`) and runs `pullChanged` to populate the sidecar before calling `preparePush/commitPush`. This means `localHasAllRemoteEntries()` would pass even if it were checked, so the test wouldn't catch the missing guard. A test with an *incomplete* local cache (e.g., metadata-only pull, or missing files) would be needed to verify the safety check. **Severity: MEDIUM** — test gap directly related to the Critical finding. ### Low 1. **`s3_bench.ts:508` — `catch` block in scenario 14 setup swallows all errors including permission/IO failures** ```typescript } catch { /* empty dir is fine */ } ``` If `Deno.readDir` fails for a reason other than an empty/nonexistent directory (e.g., permission denied), the benchmark silently proceeds with stale data from a prior run, producing misleading results. In a benchmark context this is low-risk since it runs in temp dirs, but `catch (e) { if (!(e instanceof Deno.errors.NotFound)) throw e; }` would be more precise. **Severity: LOW** — benchmark-only, not production code. 2. **`s3_cache_sync.ts` — repeated `buildV2State` + manual `sidecar.commitSeq = X` pattern (lines 1848-1851, 1899-1901, 2188-2190)** The `buildV2State` helper doesn't accept `commitSeq` in its overrides, so every call site that needs to set it does `sidecar.commitSeq = X` after construction. If a future caller forgets this step, the sidecar will have `commitSeq: undefined` and the fast path will miss. This is a footgun but not a current bug. **Severity: LOW** — maintenance concern, no current breakage. ### Verdict **FAIL** — The `commitPush` v2 no-op path unconditionally marks the sidecar clean without verifying that the local cache actually has all remote entries. This is a regression of the swamp-club#1225 data-loss fix for v2 repos. The fix is a one-line guard (`if (await this.localHasAllRemoteEntries())`) wrapping the sidecar write, matching the pattern already used in `pushChanged`'s v2 paths.

stack72 commented 2026-07-09 19:33:54 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

1. pushChanged v2 writeback path: shard write errors silently swallowed after retries (datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts, lines ~1821-1836)

   In the new v2 writeback branch of pushChanged (if (v2CommitSeq !== null)), the code calls writePartitionedIndex to write dirty shards. The JSDoc of writePartitionedIndex explicitly states:

   "Non-fatal — errors are swallowed because the monolith is still the source of truth in this code path."

   Concretely, inside writePartitionedIndex, shard PUTs are wrapped as:

   retryWithBackoff(...).then(() => {}).catch(() => {
     // Non-fatal: partition files are an optimization in this path.
   })
   

   and the results are collected via Promise.allSettled (which never rejects).

   In the v2 path, shards are the source of truth — the code comment on line ~1820 even says "shards are the source of truth (same as commitPushShardFirst)." If a shard write fails after 3 retries, writePartitionedIndex returns silently, writePartitionMeta still writes a new _meta.json with commitSeq + 1, and the sidecar records the new commitSeq. Every subsequent sync takes the fast path (commitSeq matches), permanently masking the stale shard.

   The parallel code path commitPushShardFirst avoids this by calling writeShard directly, which propagates errors:

   await this.writeShard(partKey, existing, signal);  // errors propagate
   

   The fix is to use writeShard directly in the v2 pushChanged writeback path (mirroring commitPushShardFirst) rather than reusing writePartitionedIndex, which was designed for the legacy v1 path where the monolith is the source of truth and shard writes are optional optimizations.

   No existing or new test covers the failure-after-retries case for this code path.

---

Suggestions

1. Redundant _meta.json GET inside writePartitionedIndex for v2 pushChanged — When pushChanged enters the v2 writeback path, assembleIndexFromShards has already read _meta.json. Then writePartitionedIndex reads it a second time internally (to check if (existing && existing.version === 2) return; before skipping the v1 meta write). Since the repo is confirmed v2 at this point, this is an extra S3 GET per writeback that could be avoided by not reusing writePartitionedIndex for the v2 path (which aligns with the fix for blocking issue #1 above).

2. Benchmark env vars set but not restored (datastore/benchmarks/s3_bench.ts, lines 72–80) — AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and AWS_REGION are set in main() but not saved/restored in the finally block. The withProgrammableServer helper in the test file properly saves and restores env vars; the benchmark could adopt the same pattern for consistency, especially if it is ever invoked within a broader test harness.

## Code Review ### Blocking Issues 1. **`pushChanged` v2 writeback path: shard write errors silently swallowed after retries** (`datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts`, lines ~1821-1836) In the new v2 writeback branch of `pushChanged` (`if (v2CommitSeq !== null)`), the code calls `writePartitionedIndex` to write dirty shards. The JSDoc of `writePartitionedIndex` explicitly states: > "Non-fatal — errors are swallowed because the monolith is still the source of truth in this code path." Concretely, inside `writePartitionedIndex`, shard PUTs are wrapped as: ```typescript retryWithBackoff(...).then(() => {}).catch(() => { // Non-fatal: partition files are an optimization in this path. }) ``` and the results are collected via `Promise.allSettled` (which never rejects). In the v2 path, shards **are** the source of truth — the code comment on line ~1820 even says "shards are the source of truth (same as commitPushShardFirst)." If a shard write fails after 3 retries, `writePartitionedIndex` returns silently, `writePartitionMeta` still writes a new `_meta.json` with `commitSeq + 1`, and the sidecar records the new `commitSeq`. Every subsequent sync takes the fast path (commitSeq matches), permanently masking the stale shard. The parallel code path `commitPushShardFirst` avoids this by calling `writeShard` directly, which propagates errors: ```typescript await this.writeShard(partKey, existing, signal); // errors propagate ``` The fix is to use `writeShard` directly in the v2 `pushChanged` writeback path (mirroring `commitPushShardFirst`) rather than reusing `writePartitionedIndex`, which was designed for the legacy v1 path where the monolith is the source of truth and shard writes are optional optimizations. No existing or new test covers the failure-after-retries case for this code path. --- ### Suggestions 1. **Redundant `_meta.json` GET inside `writePartitionedIndex` for v2 pushChanged** — When `pushChanged` enters the v2 writeback path, `assembleIndexFromShards` has already read `_meta.json`. Then `writePartitionedIndex` reads it a second time internally (to check `if (existing && existing.version === 2) return;` before skipping the v1 meta write). Since the repo is confirmed v2 at this point, this is an extra S3 GET per writeback that could be avoided by not reusing `writePartitionedIndex` for the v2 path (which aligns with the fix for blocking issue #1 above). 2. **Benchmark env vars set but not restored** (`datastore/benchmarks/s3_bench.ts`, lines 72–80) — `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY`, and `AWS_REGION` are set in `main()` but not saved/restored in the `finally` block. The `withProgrammableServer` helper in the test file properly saves and restores env vars; the benchmark could adopt the same pattern for consistency, especially if it is ever invoked within a broader test harness.

stack72 added 1 commit 2026-07-09 19:42:44 +00:00

fix: address PR review — cache completeness guard, shard write errors, fresh-reader test ... 38a1eed211

- commitPush v2 no-op: wrap sidecar write with localHasAllRemoteEntries
  guard so partial readers don't mark incomplete caches as clean
- pushChanged v2 writeback: replace writePartitionedIndex (which
  swallows shard write errors via Promise.allSettled) with direct
  writeShard calls that propagate failures — shards are source of truth
  in v2, silent write failures would leave permanent stale data
- Add test for fresh-reader scenario: incomplete cache + commitPush
  no-op must NOT write commitSeq to sidecar
- Narrow catch in benchmark to NotFound only

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

stack72 added 1 commit 2026-07-09 19:47:41 +00:00

fix(datastores): apply same v2 shard-first write-path fix to GCS extension ... b43c25f8a3

Same three unguarded monolith-fetch paths existed in gcs_cache_sync.ts:
commitPush no-op, pushChanged slow path, and preparePush slow path.
Apply the identical fix pattern from the S3 extension: try shard
assembly first, use direct writeShard calls (not writePartitionedIndex)
for v2 writeback, bump commitSeq, clear dirty state, and guard sidecar
writes with localHasAllRemoteEntries.

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

stack72 commented 2026-07-09 19:59:04 +00:00

Author

Owner

Copy link

Code Review: V2 Shard-First Write-Path Migration (swamp-club#1052)

Verdict: PASS

Summary

This PR completes the v2 shard-first migration for the write side of both S3 and GCS datastore sync. When assembleIndexFromShards() detects a v2 repo (one with _meta.json containing commitSeq), the write paths (pushChanged, preparePush, commitPush) now skip the monolithic index fetch/writeback and instead operate exclusively on partition shards. This eliminates redundant full-index round-trips that were the source of significant overhead (swamp-club#1034).

The changes are well-structured, symmetric across S3 and GCS, and the new test coverage targets the critical safety invariant (incomplete-cache guard on sidecar marking).

---

Findings

MEDIUM — Orphaned shard files when all entries in a partition are deleted (pushChanged v2 path)

S3: s3_cache_sync.ts:1826-1831
GCS: gcs_cache_sync.ts:1689-1694

In the v2 pushChanged writeback, dirty partitions are written only when entries && Object.keys(entries).length > 0. If all entries in a partition are deleted, the code skips the writeShard() call but never deletes the now-empty remote shard file.

The _meta.json partitions list is correctly rebuilt from allPartitions.keys() (line 1834/1697), so the empty partition disappears from the metadata — but the orphaned shard blob remains on S3/GCS storage indefinitely.

Compare with commitPushShardFirst() (S3 line 2283-2296) which properly handles this case:

if (Object.keys(existing).length === 0) {
  await retryWithBackoff(
    () => this.s3.deleteObject(this.shardKey(partKey), signal),
    { signal },
  );
  survivingPartitions.delete(partKey);
}

Impact: Storage leak — orphaned shard files accumulate over the lifetime of the repo. Not a correctness issue since _meta.json won't reference them and assembleIndexFromShards() only reads partitions listed in metadata. However, this creates unbounded storage waste proportional to model/workflow churn.

Suggested fix: Add an else branch in the pushChanged v2 path that mirrors commitPushShardFirst's behavior — delete the remote shard when entries are empty.

---

LOW — _meta.json partitions list correctly reflects index state (reviewed, no issue)

S3: s3_cache_sync.ts:1834
GCS: gcs_cache_sync.ts:1698

The new _meta.json is built from allPartitions.keys() where allPartitions = groupEntriesByPartition(this.index.entries). Since this.index has already been mutated by the preceding put/delete operations in pushChanged, this is sound — the partitions list will accurately reflect which shards have entries.

No action needed — noting this was reviewed and found correct.

---

LOW — Test coverage is well-targeted (reviewed, no issue)

S3: s3_cache_sync_test.ts (new seedV2Repo function and 4 test cases)

The seedV2Repo() helper correctly seeds the mock S3 with _meta.json (commitSeq, partitions list) and corresponding shard files. The tests properly verify:

1. commitPush v2 no-op uses _meta.json commitSeq, not monolith (avoids monolith GET)
2. pushChanged slow path on v2 uses shard assembly
3. preparePush slow path on v2 uses shard assembly
4. commitPush v2 no-op does NOT mark sidecar clean when cache is incomplete

The incomplete-cache safety test (#4) is particularly valuable — it guards against a subtle regression where localHasAllRemoteEntries() returning false correctly prevents the sidecar from being marked clean.

No action needed — noting the tests are well-targeted.

---

Dimensions Reviewed

Dimension	Assessment
Logic & Correctness	Sound. V2 detection via assembleIndexFromShards() returning a non-null commitSeq is the right gate. The fallback to v1 monolith paths when v2CommitSeq === null preserves backward compatibility.
Error Handling	Appropriate. Shard write failures propagate (intentionally not swallowed — comment at S3 line 1822 explains why). Sidecar update is wrapped in try/catch as non-fatal (consistent with existing pattern).
Security	No new attack surface. Path traversal guards (assertSafePath) are unchanged. No user-controlled input flows into S3/GCS keys without validation.
Concurrency	Lock discipline unchanged — all write-path code executes within the existing global lock. No new race conditions introduced.
Data Integrity	The localHasAllRemoteEntries() guard before marking sidecar clean (S3 line 1846, GCS line 1709) correctly prevents premature clean marking on incomplete caches. commitSeq is incremented before write, ensuring monotonic progression.
Resource Management	No new resource leaks beyond the orphaned shard issue noted above. Signal propagation through abort controllers is consistent.
API Contracts	Public API surface unchanged. Internal pushChanged, preparePush, commitPush behavior changes are transparent to callers.
S3/GCS Symmetry	The S3 and GCS implementations are structurally identical with appropriate provider-specific differences (ETag vs generation, s3.deleteObject vs gcs.deleteObject).
Benchmarks	New benchmark file s3_bench.ts covers all 15 scenarios including v2-specific paths. Well-structured with proper cleanup.

---

Conclusion

The PR is well-executed with one medium-severity storage hygiene issue (orphaned shards in pushChanged v2 path). This is not a correctness or data integrity problem — orphaned shards are invisible to index assembly since _meta.json won't reference them — but it should be addressed to prevent unbounded storage accumulation. The fix is straightforward and can be done as a follow-up.

All other dimensions are sound. The code is clean, the S3/GCS implementations are properly symmetric, and the test coverage is well-targeted at the critical safety invariants.

# Code Review: V2 Shard-First Write-Path Migration (swamp-club#1052) **Verdict: PASS** ## Summary This PR completes the v2 shard-first migration for the write side of both S3 and GCS datastore sync. When `assembleIndexFromShards()` detects a v2 repo (one with `_meta.json` containing `commitSeq`), the write paths (`pushChanged`, `preparePush`, `commitPush`) now skip the monolithic index fetch/writeback and instead operate exclusively on partition shards. This eliminates redundant full-index round-trips that were the source of significant overhead (swamp-club#1034). The changes are well-structured, symmetric across S3 and GCS, and the new test coverage targets the critical safety invariant (incomplete-cache guard on sidecar marking). --- ## Findings ### MEDIUM — Orphaned shard files when all entries in a partition are deleted (`pushChanged` v2 path) **S3:** `s3_cache_sync.ts:1826-1831` **GCS:** `gcs_cache_sync.ts:1689-1694` In the v2 `pushChanged` writeback, dirty partitions are written only when `entries && Object.keys(entries).length > 0`. If all entries in a partition are deleted, the code skips the `writeShard()` call but **never deletes the now-empty remote shard file**. The `_meta.json` partitions list is correctly rebuilt from `allPartitions.keys()` (line 1834/1697), so the empty partition disappears from the metadata — but the orphaned shard blob remains on S3/GCS storage indefinitely. Compare with `commitPushShardFirst()` (S3 line 2283-2296) which properly handles this case: ```typescript if (Object.keys(existing).length === 0) { await retryWithBackoff( () => this.s3.deleteObject(this.shardKey(partKey), signal), { signal }, ); survivingPartitions.delete(partKey); } ``` **Impact:** Storage leak — orphaned shard files accumulate over the lifetime of the repo. Not a correctness issue since `_meta.json` won't reference them and `assembleIndexFromShards()` only reads partitions listed in metadata. However, this creates unbounded storage waste proportional to model/workflow churn. **Suggested fix:** Add an `else` branch in the `pushChanged` v2 path that mirrors `commitPushShardFirst`'s behavior — delete the remote shard when entries are empty. --- ### LOW — `_meta.json` partitions list correctly reflects index state (reviewed, no issue) **S3:** `s3_cache_sync.ts:1834` **GCS:** `gcs_cache_sync.ts:1698` The new `_meta.json` is built from `allPartitions.keys()` where `allPartitions = groupEntriesByPartition(this.index.entries)`. Since `this.index` has already been mutated by the preceding put/delete operations in `pushChanged`, this is sound — the partitions list will accurately reflect which shards have entries. No action needed — noting this was reviewed and found correct. --- ### LOW — Test coverage is well-targeted (reviewed, no issue) **S3:** `s3_cache_sync_test.ts` (new `seedV2Repo` function and 4 test cases) The `seedV2Repo()` helper correctly seeds the mock S3 with `_meta.json` (commitSeq, partitions list) and corresponding shard files. The tests properly verify: 1. `commitPush` v2 no-op uses `_meta.json` commitSeq, not monolith (avoids monolith GET) 2. `pushChanged` slow path on v2 uses shard assembly 3. `preparePush` slow path on v2 uses shard assembly 4. `commitPush` v2 no-op does NOT mark sidecar clean when cache is incomplete The incomplete-cache safety test (#4) is particularly valuable — it guards against a subtle regression where `localHasAllRemoteEntries()` returning false correctly prevents the sidecar from being marked clean. No action needed — noting the tests are well-targeted. --- ## Dimensions Reviewed | Dimension | Assessment | |---|---| | **Logic & Correctness** | Sound. V2 detection via `assembleIndexFromShards()` returning a non-null `commitSeq` is the right gate. The fallback to v1 monolith paths when `v2CommitSeq === null` preserves backward compatibility. | | **Error Handling** | Appropriate. Shard write failures propagate (intentionally not swallowed — comment at S3 line 1822 explains why). Sidecar update is wrapped in try/catch as non-fatal (consistent with existing pattern). | | **Security** | No new attack surface. Path traversal guards (`assertSafePath`) are unchanged. No user-controlled input flows into S3/GCS keys without validation. | | **Concurrency** | Lock discipline unchanged — all write-path code executes within the existing global lock. No new race conditions introduced. | | **Data Integrity** | The `localHasAllRemoteEntries()` guard before marking sidecar clean (S3 line 1846, GCS line 1709) correctly prevents premature clean marking on incomplete caches. commitSeq is incremented before write, ensuring monotonic progression. | | **Resource Management** | No new resource leaks beyond the orphaned shard issue noted above. Signal propagation through abort controllers is consistent. | | **API Contracts** | Public API surface unchanged. Internal `pushChanged`, `preparePush`, `commitPush` behavior changes are transparent to callers. | | **S3/GCS Symmetry** | The S3 and GCS implementations are structurally identical with appropriate provider-specific differences (ETag vs generation, `s3.deleteObject` vs `gcs.deleteObject`). | | **Benchmarks** | New benchmark file `s3_bench.ts` covers all 15 scenarios including v2-specific paths. Well-structured with proper cleanup. | --- ## Conclusion The PR is well-executed with one medium-severity storage hygiene issue (orphaned shards in `pushChanged` v2 path). This is not a correctness or data integrity problem — orphaned shards are invisible to index assembly since `_meta.json` won't reference them — but it should be addressed to prevent unbounded storage accumulation. The fix is straightforward and can be done as a follow-up. All other dimensions are sound. The code is clean, the S3/GCS implementations are properly symmetric, and the test coverage is well-targeted at the critical safety invariants.

stack72 commented 2026-07-09 19:59:05 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

1. Missing tests for GCS pushChanged v2 shard-first write path (datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts)

   CLAUDE.md requires: "New functionality in vault/ or datastore/ extensions should have corresponding tests." This PR adds a v2 shard-first write path to GCS's pushChanged and preparePush (structurally identical to the S3 changes). The S3 extension received four explicit tests covering these paths (#1052: pushChanged slow path on v2 uses shard assembly, not monolith, #1052: preparePush slow path on v2 uses shard assembly, not monolith, etc.). The GCS extension's gcs_cache_sync_test.ts has existing tests for commitPush/preparePush in v2 mode, but no test verifies that pushChanged on a v2 repo (a) uses shard assembly instead of pullIndex(forceRemote) and (b) skips the monolithic index PUT in the writeback. Without this test, a regression that causes pushChanged to fall back to the monolith path on v2 repos would go undetected in GCS.

   Required: add a GCS test equivalent to the S3 #1052: pushChanged slow path on v2 uses shard assembly, not monolith test.

Suggestions

1. Orphaned shard files when a partition becomes empty via pushChanged v2 (s3_cache_sync.ts lines ~1826–1840, gcs_cache_sync.ts lines ~1682–1700)

   In the v2 writeback path of pushChanged, when all entries in a partition are deleted, the shard file (e.g. _index/data--t--m--1.json) is left as an orphan in the bucket. _meta.json is correctly updated to omit the empty partition, so correctness is unaffected, but the shard object accumulates as garbage. commitPushShardFirst handles this consistently by explicitly calling s3.deleteObject / gcs.deleteObject on the shard when Object.keys(existing).length === 0. Consider applying the same cleanup in pushChanged's v2 writeback, or add a comment documenting the intentional divergence.

2. No test coverage for v2 deletions via pushChanged (s3_cache_sync_test.ts)

   The four new #1052 tests cover v2 push (file modification) and no-op paths, but none verify that deleting a file from a v2 repo via pushChanged correctly updates the shards and _meta.json. The orphaned-shard behavior described above would not be caught by any existing test.

## Code Review ### Blocking Issues 1. **Missing tests for GCS `pushChanged` v2 shard-first write path** (`datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts`) CLAUDE.md requires: "New functionality in vault/ or datastore/ extensions should have corresponding tests." This PR adds a v2 shard-first write path to GCS's `pushChanged` and `preparePush` (structurally identical to the S3 changes). The S3 extension received four explicit tests covering these paths (`#1052: pushChanged slow path on v2 uses shard assembly, not monolith`, `#1052: preparePush slow path on v2 uses shard assembly, not monolith`, etc.). The GCS extension's `gcs_cache_sync_test.ts` has existing tests for `commitPush`/`preparePush` in v2 mode, but no test verifies that `pushChanged` on a v2 repo (a) uses shard assembly instead of `pullIndex(forceRemote)` and (b) skips the monolithic index PUT in the writeback. Without this test, a regression that causes `pushChanged` to fall back to the monolith path on v2 repos would go undetected in GCS. Required: add a GCS test equivalent to the S3 `#1052: pushChanged slow path on v2 uses shard assembly, not monolith` test. ### Suggestions 1. **Orphaned shard files when a partition becomes empty via `pushChanged` v2** (`s3_cache_sync.ts` lines ~1826–1840, `gcs_cache_sync.ts` lines ~1682–1700) In the v2 writeback path of `pushChanged`, when all entries in a partition are deleted, the shard file (e.g. `_index/data--t--m--1.json`) is left as an orphan in the bucket. `_meta.json` is correctly updated to omit the empty partition, so correctness is unaffected, but the shard object accumulates as garbage. `commitPushShardFirst` handles this consistently by explicitly calling `s3.deleteObject` / `gcs.deleteObject` on the shard when `Object.keys(existing).length === 0`. Consider applying the same cleanup in `pushChanged`'s v2 writeback, or add a comment documenting the intentional divergence. 2. **No test coverage for v2 deletions via `pushChanged`** (`s3_cache_sync_test.ts`) The four new `#1052` tests cover v2 push (file modification) and no-op paths, but none verify that deleting a file from a v2 repo via `pushChanged` correctly updates the shards and `_meta.json`. The orphaned-shard behavior described above would not be caught by any existing test.

stack72 added 1 commit 2026-07-09 20:03:53 +00:00

fix: add GCS v2 tests, clean up orphaned shards on empty partition deletion ... c10f5b5584

- Add 4 GCS tests mirroring the S3 #1052 tests: commitPush no-op,
  pushChanged slow path, preparePush slow path, and incomplete-cache
  guard — all verify shard assembly is used instead of monolith fetch
- Delete orphaned shard files when all entries in a partition are
  removed during v2 pushChanged writeback (both S3 and GCS), matching
  the existing cleanup in commitPushShardFirst

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

stack72 commented 2026-07-09 20:14:23 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. S3 test file is missing an explicit preparePush v2 slow-path test. The GCS test file has #1052: GCS preparePush slow path on v2 uses shard assembly, not monolith (gcs_cache_sync_test.ts line 5382), which directly asserts that preparePush on a v2 repo calls assembleIndexFromShards and never fetches the monolithic index. The S3 equivalent was also modified in this PR (s3_cache_sync.ts, the same assembleIndexFromShards call was added to preparePush), but there is no corresponding #1052: preparePush slow path on v2 test in s3_cache_sync_test.ts. The S3 path is exercised indirectly through the commitPush tests (which call preparePush first), but a direct mirror of the GCS test would close the coverage gap and follow the existing test symmetry pattern the two files maintain.

2. v2 pushChanged bumps commitSeq even when only indexMutated is set. When pushed === 0, deleted === 0, and only indexMutated is true (i.e., scrubIndex removed zombie entries from the in-memory index but no S3/GCS objects changed), the v2 writeback path in pushChanged still iterates over dirtyPartitionKeys (empty), then writes a new _meta.json with commitSeq + 1 and the unchanged partition list. No shard content changes, but the _meta.json PUT fires anyway. This was the same behavior as the v1 index writeback for the indexMutated-only case, so it's not a regression — it just means a zombie-scrub produces a gratuitous _meta.json write and a spurious commitSeq bump that will invalidate any peer's cached sidecar. Low priority; could be gated on dirtyPartitionKeys.size > 0 in the future.

## Code Review ### Blocking Issues None. ### Suggestions 1. **S3 test file is missing an explicit `preparePush` v2 slow-path test.** The GCS test file has `#1052: GCS preparePush slow path on v2 uses shard assembly, not monolith` (gcs_cache_sync_test.ts line 5382), which directly asserts that `preparePush` on a v2 repo calls `assembleIndexFromShards` and never fetches the monolithic index. The S3 equivalent was also modified in this PR (`s3_cache_sync.ts`, the same `assembleIndexFromShards` call was added to `preparePush`), but there is no corresponding `#1052: preparePush slow path on v2` test in `s3_cache_sync_test.ts`. The S3 path is exercised indirectly through the `commitPush` tests (which call `preparePush` first), but a direct mirror of the GCS test would close the coverage gap and follow the existing test symmetry pattern the two files maintain. 2. **v2 `pushChanged` bumps `commitSeq` even when only `indexMutated` is set.** When `pushed === 0`, `deleted === 0`, and only `indexMutated` is true (i.e., `scrubIndex` removed zombie entries from the in-memory index but no S3/GCS objects changed), the v2 writeback path in `pushChanged` still iterates over `dirtyPartitionKeys` (empty), then writes a new `_meta.json` with `commitSeq + 1` and the unchanged partition list. No shard content changes, but the `_meta.json` PUT fires anyway. This was the same behavior as the v1 index writeback for the `indexMutated`-only case, so it's not a regression — it just means a zombie-scrub produces a gratuitous `_meta.json` write and a spurious commitSeq bump that will invalidate any peer's cached sidecar. Low priority; could be gated on `dirtyPartitionKeys.size > 0` in the future.

stack72 commented 2026-07-09 20:14:50 +00:00

Author

Owner

Copy link

Adversarial Code Review — Verdict: PASS ✅

No CRITICAL or HIGH severity issues found.

---

Changed Files Reviewed

File	Lines	Status
datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts	~2591	Reviewed
datastore/gcs/extensions/datastores/_lib/gcs_cache_sync_test.ts	~2770	Reviewed
datastore/gcs/manifest.yaml	52	Reviewed
datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts	~2750	Reviewed
datastore/s3/extensions/datastores/_lib/s3_cache_sync_test.ts	~2750	Reviewed
datastore/s3/manifest.yaml	49	Reviewed
datastore/benchmarks/s3_bench.ts	587	Reviewed
datastore/benchmarks/README.md	100	Reviewed

---

Dimension Analysis

1. Logic

No issues found.
Both S3 and GCS implementations correctly mirror each other with appropriate cloud-specific adaptations. The two-phase push (preparePush/commitPush) correctly narrows the lock critical section. The shard-first v2 migration path is logically sound — migrateMonolithToShards correctly parses monolithic indexes and splits into per-model shards. The fast-path sidecar optimization correctly gates on sidecar completeness before trusting cached fingerprints, preventing stale reads.

2. Error Handling

No issues found.
isRetryableError correctly classifies transient vs permanent failures. retryWithBackoff implements exponential backoff with jitter and respects abort signals between retries. All async paths propagate AbortSignal correctly via throwIfAborted checks. The finally blocks in test fixtures properly restore environment variables and close resources.

3. Security

No issues found.
Path traversal is prevented by assertSafePath which rejects .. components, absolute paths, and null bytes in all local filesystem write paths. fetchForeignContent independently validates its paths. isInternalCacheFile prevents leaking internal state files (.swamp-*) to cloud storage during push. No credential material is hardcoded — both implementations delegate to their respective cloud credential chains (AWS default chain, GCS ADC).

4. Concurrency

No issues found.
The TOCTOU race condition identified in swamp-club #168 is correctly addressed — fingerprints are recorded from the GET response headers during download, not from a separate metadata call after the write. The localHasAllRemoteEntries() guard (#1225) prevents marking the sidecar as clean when the local cache is incomplete, avoiding a race where a partial pull would incorrectly arm the fast path for a subsequent sync.

5. Data Integrity

No issues found.
SHA-256 content hashing prevents redundant uploads by comparing local and remote content digests. commitSeq-based fast path correctly short-circuits when the remote sequence matches the local cached value. The shard merge logic in pullChanged correctly handles concurrent writes by re-reading shards if the merged index differs from expectations. Atomic file writes via Deno.writeFile with rename semantics protect local index persistence.

6. Resource Management

No issues found.
abortableSleep correctly cleans up its timer on abort. Test HTTP servers use Deno.serve({ port: 0 }) for automatic port allocation and are properly shut down in finally blocks. Tests that create SDK clients with connection pools correctly use sanitizeResources: false with explanatory comments, as required by project conventions.

7. API Contracts

No issues found.
Both implementations correctly satisfy the DatastoreSyncService interface. S3's deleteObject(key, signal?) and GCS's deleteObject(key, options?, signal?) signatures correctly match their respective client SDK conventions. normalizeETag (S3-only) correctly strips surrounding quotes from ETags. isMultipartETag (S3-only) correctly rejects multipart ETags (containing -N suffix) which have non-deterministic values.

8. Codegen Safety

Not applicable. No files under model/ are in the changed file set. The manifest version bumps are data-only changes.

---

Minor Observations (LOW severity)

#	Dimension	File	Observation	Severity
1	Error Handling	s3_cache_sync.ts	S3's isRetryableError does not explicitly check for PreconditionFailedError by name, unlike GCS which does. S3 handles this implicitly via HTTP 412 status code. This is functionally correct since S3 SDK surfaces conditional-write failures differently than the GCS client, but the asymmetry could cause confusion during future maintenance.	LOW
2	Logic	s3_cache_sync.ts, gcs_cache_sync.ts	The DIRTY_PATH_CAP (200 paths) overflow fallback to full directory walk is a reasonable safeguard, but the cap value is not configurable. For repositories with many small files, this threshold could be hit frequently, negating the benefit of per-path dirty tracking. Not a bug — a design trade-off worth monitoring.	LOW
3	Data Integrity	gcs_cache_sync.ts	GCS generation values are int64 integers compared as strings. This is correct for equality checks (the only comparison performed), but worth noting that lexicographic ordering would not match numeric ordering if ever used for range comparisons in the future.	LOW

---

Summary

The codebase is well-engineered with comprehensive defensive programming practices. The S3 and GCS implementations are structurally parallel with appropriate cloud-specific adaptations. Test coverage is thorough, including race condition regression tests, error injection via programmable HTTP servers, and integration tests with real SDK clients against local emulators. The two-phase sync design, shard-first indexing, and fingerprint-based fast path are all correctly implemented with appropriate guards against known failure modes.

No changes are required for merge.

## Adversarial Code Review — Verdict: PASS ✅ No **CRITICAL** or **HIGH** severity issues found. --- ### Changed Files Reviewed | File | Lines | Status | |------|-------|--------| | `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts` | ~2591 | Reviewed | | `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync_test.ts` | ~2770 | Reviewed | | `datastore/gcs/manifest.yaml` | 52 | Reviewed | | `datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts` | ~2750 | Reviewed | | `datastore/s3/extensions/datastores/_lib/s3_cache_sync_test.ts` | ~2750 | Reviewed | | `datastore/s3/manifest.yaml` | 49 | Reviewed | | `datastore/benchmarks/s3_bench.ts` | 587 | Reviewed | | `datastore/benchmarks/README.md` | 100 | Reviewed | --- ### Dimension Analysis #### 1. Logic **No issues found.** Both S3 and GCS implementations correctly mirror each other with appropriate cloud-specific adaptations. The two-phase push (`preparePush`/`commitPush`) correctly narrows the lock critical section. The shard-first v2 migration path is logically sound — `migrateMonolithToShards` correctly parses monolithic indexes and splits into per-model shards. The fast-path sidecar optimization correctly gates on sidecar completeness before trusting cached fingerprints, preventing stale reads. #### 2. Error Handling **No issues found.** `isRetryableError` correctly classifies transient vs permanent failures. `retryWithBackoff` implements exponential backoff with jitter and respects abort signals between retries. All async paths propagate `AbortSignal` correctly via `throwIfAborted` checks. The `finally` blocks in test fixtures properly restore environment variables and close resources. #### 3. Security **No issues found.** Path traversal is prevented by `assertSafePath` which rejects `..` components, absolute paths, and null bytes in all local filesystem write paths. `fetchForeignContent` independently validates its paths. `isInternalCacheFile` prevents leaking internal state files (`.swamp-*`) to cloud storage during push. No credential material is hardcoded — both implementations delegate to their respective cloud credential chains (AWS default chain, GCS ADC). #### 4. Concurrency **No issues found.** The TOCTOU race condition identified in swamp-club #168 is correctly addressed — fingerprints are recorded from the GET response headers during download, not from a separate metadata call after the write. The `localHasAllRemoteEntries()` guard (#1225) prevents marking the sidecar as clean when the local cache is incomplete, avoiding a race where a partial pull would incorrectly arm the fast path for a subsequent sync. #### 5. Data Integrity **No issues found.** SHA-256 content hashing prevents redundant uploads by comparing local and remote content digests. `commitSeq`-based fast path correctly short-circuits when the remote sequence matches the local cached value. The shard merge logic in `pullChanged` correctly handles concurrent writes by re-reading shards if the merged index differs from expectations. Atomic file writes via `Deno.writeFile` with rename semantics protect local index persistence. #### 6. Resource Management **No issues found.** `abortableSleep` correctly cleans up its timer on abort. Test HTTP servers use `Deno.serve({ port: 0 })` for automatic port allocation and are properly shut down in `finally` blocks. Tests that create SDK clients with connection pools correctly use `sanitizeResources: false` with explanatory comments, as required by project conventions. #### 7. API Contracts **No issues found.** Both implementations correctly satisfy the `DatastoreSyncService` interface. S3's `deleteObject(key, signal?)` and GCS's `deleteObject(key, options?, signal?)` signatures correctly match their respective client SDK conventions. `normalizeETag` (S3-only) correctly strips surrounding quotes from ETags. `isMultipartETag` (S3-only) correctly rejects multipart ETags (containing `-N` suffix) which have non-deterministic values. #### 8. Codegen Safety **Not applicable.** No files under `model/` are in the changed file set. The manifest version bumps are data-only changes. --- ### Minor Observations (LOW severity) | # | Dimension | File | Observation | Severity | |---|-----------|------|-------------|----------| | 1 | Error Handling | `s3_cache_sync.ts` | S3's `isRetryableError` does not explicitly check for `PreconditionFailedError` by name, unlike GCS which does. S3 handles this implicitly via HTTP 412 status code. This is functionally correct since S3 SDK surfaces conditional-write failures differently than the GCS client, but the asymmetry could cause confusion during future maintenance. | LOW | | 2 | Logic | `s3_cache_sync.ts`, `gcs_cache_sync.ts` | The `DIRTY_PATH_CAP` (200 paths) overflow fallback to full directory walk is a reasonable safeguard, but the cap value is not configurable. For repositories with many small files, this threshold could be hit frequently, negating the benefit of per-path dirty tracking. Not a bug — a design trade-off worth monitoring. | LOW | | 3 | Data Integrity | `gcs_cache_sync.ts` | GCS `generation` values are int64 integers compared as strings. This is correct for equality checks (the only comparison performed), but worth noting that lexicographic ordering would not match numeric ordering if ever used for range comparisons in the future. | LOW | --- ### Summary The codebase is well-engineered with comprehensive defensive programming practices. The S3 and GCS implementations are structurally parallel with appropriate cloud-specific adaptations. Test coverage is thorough, including race condition regression tests, error injection via programmable HTTP servers, and integration tests with real SDK clients against local emulators. The two-phase sync design, shard-first indexing, and fingerprint-based fast path are all correctly implemented with appropriate guards against known failure modes. No changes are required for merge.

stack72 merged commit 6bc0b0523c into main 2026-07-09 20:43:40 +00:00

stack72 deleted branch fix/1052-v2-shard-first-write-paths 2026-07-09 20:43:40 +00:00

stack72 referenced this pull request from a commit 2026-07-09 20:43:41 +00:00

fix(datastores): complete v2 shard-first migration for write-side sync paths (#1052) (#97)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

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

No description provided.