fix(datastores): SHA-256 fallback in fileNeedsPush when mtime matches (#1307) #132

Merged

stack72 merged 2 commits from fix/1307-mtime-sha256-fallback into main 2026-07-22 20:47:18 +00:00

Conversation 2 Commits 2 Files changed 4 +224 -4

stack72 commented 2026-07-22 20:02:48 +00:00

Owner

Copy link

Summary

• Bug: fileNeedsPush in both S3 and GCS datastores returned false (skip push) on the mtime-match fast path without checking the SHA-256 hash. On filesystems with coarse mtime granularity (e.g. Linux tmpfs at 1s resolution), rapid same-size writes within the same second are invisible to the stat-only fast path, silently skipping the push.
• Fix: When size and mtime both match and a sha256 hash exists in the index entry, compute the local file's SHA-256 and compare before returning false. Legacy index entries without sha256 preserve the old fast-path behavior.
• Applied identically to both @swamp/s3-datastore and @swamp/gcs-datastore.

Changed files

• datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts — hash check in mtime-match branch + updated doc comment
• datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts — identical hash check in mtime-match branch
• datastore/s3/extensions/datastores/_lib/s3_cache_sync_test.ts — 2 new tests (detects change / skips unchanged when mtime matches)
• datastore/gcs/extensions/datastores/_lib/gcs_cache_sync_test.ts — 2 new tests (same pattern)

Performance note

The mtime-match branch was previously a pure stat-only fast path (zero file I/O). Now, files with a matching mtime+size that have a sha256 in the index get a readFile + SHA-256 computation. This only affects the slow path (after markDirty), not the tryFastPushChanged shortcut. The tradeoff is correctness over speed — silent data loss on coarse-grained filesystems is worse than extra reads.

Test plan

• S3: 150/150 tests pass (including 2 new #1307 tests)
• GCS: 145/145 tests pass (including 2 new #1307 tests)
• All 4 pre-existing SHA-256 tests still pass (no regression)
• deno check passes for both extensions

Closes swamp-club#1307

🤖 Generated with Claude Code

## Summary - **Bug**: `fileNeedsPush` in both S3 and GCS datastores returned `false` (skip push) on the mtime-match fast path without checking the SHA-256 hash. On filesystems with coarse mtime granularity (e.g. Linux tmpfs at 1s resolution), rapid same-size writes within the same second are invisible to the stat-only fast path, silently skipping the push. - **Fix**: When size and mtime both match and a `sha256` hash exists in the index entry, compute the local file's SHA-256 and compare before returning `false`. Legacy index entries without `sha256` preserve the old fast-path behavior. - Applied identically to both `@swamp/s3-datastore` and `@swamp/gcs-datastore`. ## Changed files - `datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts` — hash check in mtime-match branch + updated doc comment - `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts` — identical hash check in mtime-match branch - `datastore/s3/extensions/datastores/_lib/s3_cache_sync_test.ts` — 2 new tests (detects change / skips unchanged when mtime matches) - `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync_test.ts` — 2 new tests (same pattern) ## Performance note The mtime-match branch was previously a pure stat-only fast path (zero file I/O). Now, files with a matching mtime+size that have a `sha256` in the index get a `readFile` + SHA-256 computation. This only affects the slow path (after `markDirty`), not the `tryFastPushChanged` shortcut. The tradeoff is correctness over speed — silent data loss on coarse-grained filesystems is worse than extra reads. ## Test plan - [x] S3: 150/150 tests pass (including 2 new #1307 tests) - [x] GCS: 145/145 tests pass (including 2 new #1307 tests) - [x] All 4 pre-existing SHA-256 tests still pass (no regression) - [x] `deno check` passes for both extensions Closes swamp-club#1307 🤖 Generated with [Claude Code](https://claude.com/claude-code)

stack72 added 1 commit 2026-07-22 20:02:48 +00:00

fix(datastores): SHA-256 fallback in fileNeedsPush when mtime matches (#1307) ... 18cc3ba4d8

On filesystems with coarse mtime granularity (e.g. Linux tmpfs at 1s
resolution), rapid same-size writes within the same second produce
identical mtime values. fileNeedsPush previously returned false on the
mtime-match fast path without checking the content hash, silently
skipping the push.

Now, when size and mtime both match and a sha256 hash exists in the
index, the local file is hash-compared before the fast-path skip.
Legacy index entries without sha256 preserve the old behavior.

Applied identically to both S3 and GCS datastore extensions.

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

stack72 added 1 commit 2026-07-22 20:14:23 +00:00

style(datastores): fix deno fmt in #1307 test files ... 73652aeced

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

stack72 commented 2026-07-22 20:26:17 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. Performance regression on push walk hot path — s3_cache_sync.ts:2775 / gcs_cache_sync.ts:2644

   Before this PR, the "same size + same mtime" branch in fileNeedsPush was a stat-only fast path (return false — O(1) per file). After this PR, when existing.sha256 is present (which it is for every file that was ever pushed via pushFile, since pushFile always computes and stores sha256), the code reads the entire file and computes SHA-256 — upgrading the fast path to O(file_size) per file.

   Breaking example: A production datastore with 4,000 previously-pushed files calls pushChanged() when nothing has changed. Before: ~4,000 stat() calls (~50ms). After: ~4,000 Deno.readFile() + crypto.subtle.digest() calls. If files average 50 KB, that's ~200 MB of reads + hashing per sync — significant on network-attached or containerized filesystems. For data/*/raw content files (potentially MB-sized), the cost is worse.

   The sha256 propagates through the remote index to all clients (pull downloads the index containing sha256 from prior pushes), so every client pays this cost, not just the original pusher.

   This is a correctness/performance trade-off, not a bug — the fix addresses a real data-loss scenario (coarse mtime on tmpfs). But the performance impact on the common "no changes" path should be measured against production workloads before shipping, since the DEF-2 tracing that motivated the fast-path sidecar identified this exact walk as the bottleneck.

   Suggested mitigation: Consider gating the SHA-256 fallback behind a condition that only fires when the sidecar indicates actual dirty state (e.g., bulkInvalidated || dirtyPaths.size > 0), rather than unconditionally for every file on every walk. Unchanged files on a non-dirty walk can safely trust mtime.

2. Double read+hash for files that need pushing — s3_cache_sync.ts:2776-2781 / gcs_cache_sync.ts:2645-2650

   When fileNeedsPush returns true because the SHA-256 doesn't match, the file is subsequently pushed via pushFile, which reads the same file again (Deno.readFile) and recomputes SHA-256 (crypto.subtle.digest). The file content is read and hashed twice for every file that actually changed.

   Breaking example: A 100 MB raw content file with a coarse-mtime change: fileNeedsPush reads 100 MB + hashes → returns true → pushFile reads 100 MB + hashes again → uploads. That's 200 MB of reads and two SHA-256 passes over the same data.

   This isn't a correctness issue, but it's avoidable waste. fileNeedsPush could cache the computed hash or the read data for reuse by pushFile.

Low

1. Asymmetric doc comment update — s3_cache_sync.ts:2750-2756 vs gcs_cache_sync.ts:2626

   The S3 version updates the JSDoc for fileNeedsPush to document the new 4-branch detection logic. The GCS version has no JSDoc on fileNeedsPush at all. While neither file is wrong, the asymmetry between two files that are otherwise kept in lockstep could confuse future maintainers.

Verdict

PASS — The core logic change is correct: when the index carries a SHA-256 hash and mtime matches, comparing the hash catches same-size writes that coarse mtime granularity would otherwise miss. Both the "hash mismatch → push" and "hash match → skip" paths are tested. The code is consistent between S3 and GCS implementations. The performance trade-off (stat-only fast path → full file read + hash for every previously-pushed file on every push walk) is worth measuring against production workloads, but is not a correctness or safety blocker.

## Adversarial Review ### Medium 1. **Performance regression on push walk hot path** — `s3_cache_sync.ts:2775` / `gcs_cache_sync.ts:2644` Before this PR, the "same size + same mtime" branch in `fileNeedsPush` was a stat-only fast path (`return false` — O(1) per file). After this PR, when `existing.sha256` is present (which it is for every file that was ever pushed via `pushFile`, since `pushFile` always computes and stores sha256), the code reads the entire file and computes SHA-256 — upgrading the fast path to O(file_size) per file. **Breaking example**: A production datastore with 4,000 previously-pushed files calls `pushChanged()` when nothing has changed. Before: ~4,000 `stat()` calls (~50ms). After: ~4,000 `Deno.readFile()` + `crypto.subtle.digest()` calls. If files average 50 KB, that's ~200 MB of reads + hashing per sync — significant on network-attached or containerized filesystems. For `data/*/raw` content files (potentially MB-sized), the cost is worse. The sha256 propagates through the remote index to all clients (pull downloads the index containing sha256 from prior pushes), so every client pays this cost, not just the original pusher. This is a correctness/performance trade-off, not a bug — the fix addresses a real data-loss scenario (coarse mtime on tmpfs). But the performance impact on the common "no changes" path should be measured against production workloads before shipping, since the DEF-2 tracing that motivated the fast-path sidecar identified this exact walk as the bottleneck. **Suggested mitigation**: Consider gating the SHA-256 fallback behind a condition that only fires when the sidecar indicates actual dirty state (e.g., `bulkInvalidated || dirtyPaths.size > 0`), rather than unconditionally for every file on every walk. Unchanged files on a non-dirty walk can safely trust mtime. 2. **Double read+hash for files that need pushing** — `s3_cache_sync.ts:2776-2781` / `gcs_cache_sync.ts:2645-2650` When `fileNeedsPush` returns `true` because the SHA-256 doesn't match, the file is subsequently pushed via `pushFile`, which reads the same file again (`Deno.readFile`) and recomputes SHA-256 (`crypto.subtle.digest`). The file content is read and hashed twice for every file that actually changed. **Breaking example**: A 100 MB raw content file with a coarse-mtime change: `fileNeedsPush` reads 100 MB + hashes → returns true → `pushFile` reads 100 MB + hashes again → uploads. That's 200 MB of reads and two SHA-256 passes over the same data. This isn't a correctness issue, but it's avoidable waste. `fileNeedsPush` could cache the computed hash or the read data for reuse by `pushFile`. ### Low 1. **Asymmetric doc comment update** — `s3_cache_sync.ts:2750-2756` vs `gcs_cache_sync.ts:2626` The S3 version updates the JSDoc for `fileNeedsPush` to document the new 4-branch detection logic. The GCS version has no JSDoc on `fileNeedsPush` at all. While neither file is wrong, the asymmetry between two files that are otherwise kept in lockstep could confuse future maintainers. ### Verdict **PASS** — The core logic change is correct: when the index carries a SHA-256 hash and mtime matches, comparing the hash catches same-size writes that coarse mtime granularity would otherwise miss. Both the "hash mismatch → push" and "hash match → skip" paths are tested. The code is consistent between S3 and GCS implementations. The performance trade-off (stat-only fast path → full file read + hash for every previously-pushed file on every push walk) is worth measuring against production workloads, but is not a correctness or safety blocker.

stack72 commented 2026-07-22 20:26:17 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. gcs_cache_sync.ts: Orphaned JSDoc block (lines 69–103)

   The large JSDoc block starting at line 69 ("Returns true for files that live inside the cache directory but must NOT cross the sync boundary…") describes isInternalCacheFile semantics. In the S3 sibling file that comment sits directly above isInternalCacheFile. In the GCS file, isInsideNamespaceDir was moved above isInternalCacheFile, but its comment (lines 104–110) was correctly placed — while the big isInternalCacheFile comment was left orphaned between the two functions. The result is that isInternalCacheFile (line 148) has no JSDoc, and the long comment appears to document isInsideNamespaceDir instead. Not a runtime issue, but could confuse future readers. Consider moving the large block to sit immediately above isInternalCacheFile.

2. gcs_cache_sync.ts / s3_cache_sync.ts: preparePush dirtyPartitionKeys deduplication uses linear scan

   In preparePush (both files), dirty partition keys are deduplicated with !dirtyPartitionKeys.includes(key) against a plain array (O(n) per insert). pushChanged in the same files uses a Set<string> for this, which is O(1). On large deploys with thousands of files, the array approach could be measurably slower. Low priority, but worth aligning for consistency.

## Code Review ### Blocking Issues None. ### Suggestions 1. **`gcs_cache_sync.ts`: Orphaned JSDoc block (lines 69–103)** The large JSDoc block starting at line 69 ("Returns true for files that live inside the cache directory but must NOT cross the sync boundary…") describes `isInternalCacheFile` semantics. In the S3 sibling file that comment sits directly above `isInternalCacheFile`. In the GCS file, `isInsideNamespaceDir` was moved above `isInternalCacheFile`, but its comment (lines 104–110) was correctly placed — while the big `isInternalCacheFile` comment was left orphaned between the two functions. The result is that `isInternalCacheFile` (line 148) has no JSDoc, and the long comment appears to document `isInsideNamespaceDir` instead. Not a runtime issue, but could confuse future readers. Consider moving the large block to sit immediately above `isInternalCacheFile`. 2. **`gcs_cache_sync.ts` / `s3_cache_sync.ts`: `preparePush` `dirtyPartitionKeys` deduplication uses linear scan** In `preparePush` (both files), dirty partition keys are deduplicated with `!dirtyPartitionKeys.includes(key)` against a plain array (`O(n)` per insert). `pushChanged` in the same files uses a `Set<string>` for this, which is `O(1)`. On large deploys with thousands of files, the array approach could be measurably slower. Low priority, but worth aligning for consistency.

stack72 merged commit b96a132544 into main 2026-07-22 20:47:18 +00:00

stack72 deleted branch fix/1307-mtime-sha256-fallback 2026-07-22 20:47:18 +00:00

stack72 referenced this pull request from a commit 2026-07-22 20:47:19 +00:00

fix(datastores): SHA-256 fallback in fileNeedsPush when mtime matches (#1307) (#132)

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

No description provided.