fix(datastore): bulk-diff hydrate, arm commitSeq fast path, skip root migration #270
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/datastore-pull-push-perf-2033-1931-2091"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Three fixes applied symmetrically to both
@swamp/s3-datastoreand@swamp/gcs-datastore:#1931 — commitSeq fast path never arms after pull: On the v2 shard-first path with no 404 cleanup and no monolithic index ETag, neither existing branch wrote
commitSeqto the sidecar. Added a newelse ifbranch that persistscommitSeqafter a clean v2 pull so the fast path arms on the next call. Eliminates P50 4.5s pull overhead on every method call.#2091 — preparePush reads root
.datastore-index.jsonunder managedConfig+namespace:migrateRootDataToNamespaceunconditionally read the root monolithic index, hitting AccessDenied on scoped IAM policies. When v2 shard assembly succeeds, the migration is now skipped entirely anddataKeyMigrated=trueis set — the v2 shard index is authoritative. Eliminates 5–15s retry backoff per push on namespace-scoped IAM setups.#2033 — Cold hydrate issues one round-trip per file: Added a bulk
listAllObjectscall before thepullChangedwalk loop. Index entries absent from the listing are pruned during the walk instead of discovering 404s per-file during download — O(files/1000) list calls instead of O(files) getObject calls. On the reporter's 45,713-file namespace, this cuts ~45,512 wasted round-trips to ~46 list pages. The listing is wrapped in try-catch: on failure it falls back to the pre-change per-file behavior.Bumps both manifests to
2026.09.10.2.Test plan
New regression tests
pullChanged on v2 shard-first writes commitSeq to sidecar, next pull is fast-pathedpreparePush skips root migration when v2 shard index exists.datastore-index.jsonnever read,dataKeyMigratedsetpullChanged falls back to per-file behavior when listing failspullChanged prunes stale entries via listing without per-file 404Closes #2033, #1931, #2091
🤖 Generated with Claude Code
Code Review
Blocking Issues
None.
Suggestions
Documentation misplacement in
gcs_cache_sync.ts(lines 74–108): The large JSDoc block describingisInternalCacheFile's exclusion criteria is orphaned — it appears beforeisInsideNamespaceDirrather than directly beforeisInternalCacheFileat line 153. Two consecutive JSDoc comments appear with no function between them (theisInsideNamespaceDirJSDoc at lines 109–115 immediately follows), leavingisInternalCacheFileundocumented at its definition site. This is likely an ordering side-effect of placingisInsideNamespaceDirbeforeisInternalCacheFilein the GCS file (S3 has the reverse ordering). Consider movingisInternalCacheFile(and its JSDoc) to directly precedeisInsideNamespaceDir, matching the S3 layout, or placing the big JSDoc immediately before the function it describes.Dead code in full-walk orphan detection (gcs
pushChangedline ~2417,preparePushline ~2850; same in S3 counterparts): Inside the guardif (this.dirtyPathsOverflowed && !this.lazyPullActive && this.index), there is an unreachable checkif (this.lazyPullActive && isLazySkippable(key)) continue;. Since!this.lazyPullActiveis already required by the outer condition, the innerthis.lazyPullActiveis alwaysfalseand thecontinuenever executes. This is harmless but adds noise; removing the inner check would clarify intent.Test gap: scoped walk +
lazyPullActive = true: The existing lazy-hydration deletion guard test (del3) exercises the bulk invalidation path (markDirty()with norelPath, which setsbulkInvalidated = true, bypassing the scoped walk entirely). There is no test verifying that the scoped walk directory-deletion path (markDirty({ relPath: "data/..." })pointing to a directory that still exists) correctly skips orphan deletion whenlazyPullActive = true. The code guardif (!this.lazyPullActive && this.index)is correct, but a complementary test would protect against future regression in this specific combination.Adversarial Review
Critical / High
None found. The production code changes are logically correct.
Medium
Listing-failure fallback tests override the wrong method — tests don't test what they claim
S3
s3_cache_sync_test.ts:8501-8502and GCSgcs_cache_sync_test.ts:7783:Both "swamp-club#2033: pullChanged falls back to per-file behavior when listing fails" tests override
listObjects:But the production code calls
listAllObjects(S3 at line 1901, GCS at line 1808). The mock'slistAllObjectsmethod is defined separately and is NOT affected by this override. The listing succeeds normally,remoteKeysis populated, and the test passes coincidentally — NOT because the fallback path was exercised.Breaking example: If someone introduced a bug in the
catch {}fallback block (e.g., accidentally re-throwing, or settingremoteKeysto an empty set instead of leaving itnull), this test would not catch it.Suggested fix: Change both tests to override
listAllObjectsinstead oflistObjects:Low
mock.storage.set("data/file/v1/raw", ...)additions ats3_cache_sync_test.ts:4867andgcs_cache_sync_test.ts:4532are necessary — without them, the new bulk-diff listing would prune the entry before the download phase ever sees it, preventing the injected auth error from firing. The fix is correct but worth noting: this test now exercises both the listing path (which must NOT prune the file) and the download error path (which must throw). If the listing logic had a bug that incorrectly pruned an existing file, this test would no longer reach the auth-error assertion and would pass for the wrong reason. Consider adding a comment explaining why the storage seed is needed.Verdict
PASS — The production code is correct across all three fixes (#2033 bulk-diff listing, #1931 commitSeq fast-path arming, #2091 root-migration skip for v2 shards). The bulk-diff optimization handles failure gracefully (null remoteKeys → no pruning), namespace stripping is correct, and the v2 sidecar writes place commitSeq correctly. The test for listing-failure fallback overrides the wrong mock method (Medium), but the production fallback code itself is sound. The manifest version bumps are appropriate.
Code Review
Blocking Issues
swamp-club#2033listing-fallback tests override the wrong method name in both test files.gcs_cache_sync_test.ts(new test "swamp-club#2033: GCS pullChanged falls back to per-file behavior when listing fails") and its S3 mirror overridelistObjects:But the production code calls
this.gcs.listAllObjects(...), notlistObjects. The mock haslistAllObjectsdefined; overridinglistObjectsadds an unreachable property. ThelistAllObjectscall inpullChangedstill succeeds,remoteKeysis populated normally, and the test assertspulled === 1— which passes, but because the file was pulled via the normal path, not the intended fallback path. The fallback code undercatchis never exercised. The test name is wrong and the scenario it claims to cover is not actually tested.Fix: override
listAllObjectsinstead oflistObjectson the mock:(Same correction applies to the S3 mirror test using
S3OperationError.)Suggestions
formatBatchFailureternary style differs between GCS and S3. GCS uses a three-way ternary (op === "pull" ? "from" : op === "delete" ? "from" : "to") while S3 uses a simpler two-way form (op === "push" ? "to" : "from"). Both produce identical results, but consistency between the two mirrored implementations would make future diffs easier to read.Code Review: bulk-diff hydrate, arm commitSeq fast path, skip root migration (#2033, #1931, #2091)
Summary
This PR adds three features to both GCS and S3 datastore sync services:
pullChangednow lists all remote objects upfront to prune stale index entries in bulk, avoiding O(files) per-file 404 round-trips.commitSeqso the next pull can short-circuit via the fast path.pushChanged/preparePushskip the root-to-namespace data key migration when v2 shard-first indexing is already in place.Test coverage includes new tests for all three features plus a fix to an existing error-propagation test that would silently pass under the new bulk-diff listing.
617 insertions, 47 deletions across 6 files.
Findings
1. Pull-side shard writeback without distributed lock — amplified blast radius
gcs_cache_sync.ts:1978–2041,s3_cache_sync.ts:2113–2175The 404 shard writeback block (introduced by #2063) calls
writeShard()andwritePartitionMeta()duringpullChanged(), which runs without the distributed lock. Both methods are unconditional PUTs (gcs_cache_sync.ts:880–881,s3_cache_sync.ts:947–948) — no generation/ETag preconditions.This PR amplifies the existing race by routing all listing-pruned entries (potentially many) through
pull404PartitionKeysinto the same unlocked writeback path. Previously, only per-file download-time 404s fed this set (a handful at most). With the bulk-diff listing, a pull against a remote that had a recent GC or compaction could prune hundreds of entries and trigger shard rewrites for many partitions.Race scenario:
commitSeqto N+1._meta.jsonwithcommitSeq = M+1where M < N+1.discoverIndexFromBucket(new machine or empty cache).The shard writeback path itself is pre-existing (#2063), but the increased volume of entries flowing through it materially increases the probability and blast radius of this race.
Recommendation: Consider gating the shard writeback on a short lock acquisition (with a fast timeout — if the lock is held, skip the writeback like the existing "non-fatal" fallback), or at minimum use conditional writes (GCS generation-match / S3 If-Match) to detect concurrent modifications and abort the writeback if the shard was modified since it was read.
2. Full-bucket listing without sub-prefix scoping
gcs_cache_sync.ts:1808–1811,s3_cache_sync.ts:1901–1904The bulk-diff calls
listAllObjects(undefined, signal), which lists all objects under the bucket's configured prefix. Other listing calls in the codebase scope to a sub-prefix (e.g.,discoverIndexFromBucketscopes to${namespace}/at line 1552,migrateMonolithToShardsscopes to_index/at line 1124).The comment explains the rationale: root-level files from pre-namespace pushes must be captured. However, for large shared-prefix deployments (multi-namespace datastores with hundreds of models), this unscoped listing could:
Set<string>in memory.pullChangedcall, even when most listed objects are irrelevant.The graceful fallback on listing failure mitigates hard failures but not the performance degradation when the listing succeeds but is large.
Recommendation: Consider scoping to the namespace prefix (falling back to unscoped only for the non-namespaced case), or listing with namespace prefix first and falling back to an unscoped listing only when the index contains entries that don't match the namespace listing. Alternatively, add a configurable size threshold: if the listing returns more than N entries, discard it and fall back to per-file behavior.
3. Partial failure in shard writeback leaves inconsistent local state
gcs_cache_sync.ts:1986–2041,s3_cache_sync.ts:2121–2175The shard writeback block is wrapped in a single
try/catchthat covers multiple sequential operations:writeShard()(per partition),writePartitionMeta(),atomicWriteTextFile(), andwriteSyncState(). If an early operation succeeds but a later one fails:writePartitionMetasucceeds (remote commitSeq bumped) butatomicWriteTextFilefails → remote and local index diverge. Next boot reads stale local index, but the remote shards have the updated commitSeq.atomicWriteTextFilesucceeds butwriteSyncStatefails → local index is updated but sidecar doesn't record the new commitSeq. Next boot takes the slow path but finds consistent state.The first case is self-healing: the next
pullChangedwill re-read remote shards (commitSeq mismatch forces slow path) and re-prune the stale entries. But the recovery costs an extra slow-path cycle and the intermediate state could confuse concurrent operations.Recommendation: Consider writing the sidecar and local index before the remote shard writeback, so local state is always at least as up-to-date as remote state. Alternatively, break the try/catch into separate blocks so partial remote writes can be logged distinctly.
4. Test fix for error-propagation test is correct but subtle
gcs_cache_sync_test.ts:4532,s3_cache_sync_test.ts:4867The existing "still throws on non-NotFound errors" test is updated to add
mock.storage.set("data/file/v1/raw", ...). This is necessary because the bulk-diff listing now prunes entries not found in remote storage before the download loop runs. Without seeding the file, the listing would prune the entry and the overriddengetObject(which throws an auth error) would never be reached — the test would pass vacuously without exercising the error-propagation path.The fix is correct. The subtlety is that this test's validity now depends on the bulk-diff listing being exhaustive — if a future change makes the listing skip certain entries, this test could regress silently. A brief inline comment explaining why the file is seeded would aid future maintainers.
5. Empty-string sidecar fields are consistent but semantically ambiguous
gcs_cache_sync.ts:2048–2049,s3_cache_sync.ts:2182–2183The new
else if (v2CommitSeq !== null)block writesremoteIndexGeneration: ""(GCS) /remoteIndexETag: ""(S3) to the sidecar. An empty string is truthy in JavaScript, which could matter if any fast-path check usesif (sidecar.remoteIndexGeneration)rather than strict comparison.This matches the existing pattern in the #2063 shard writeback block directly above, so the fast-path code must already handle empty strings correctly. Consistent but worth noting for future readers.
Not Flagged (Examined and Clean)
assertSafePathis correctly applied to all local file paths derived from index entries.Object.entries()creates a snapshot before the loop, sodelete this.index.entries[rel]during iteration is safe.throwIfAborted(signal)is called at batch boundaries;signalis forwarded to all remote operations.generationvsETag,NotFoundErrorclass vs error name checks).finallyblocks.2026.09.10.2, consistent._meta.jsonwrite failure on empty buckets is documented and self-healing (retries on next run). No data loss risk since the bucket is empty.formatBatchFailureS3 preposition: S3 uses"from"for both pull and delete (vs GCS which distinguishes all three ops). Pre-existing difference, not introduced by this PR.Verdict
No critical or high-severity findings. Two medium findings (unlocked shard writeback amplification, full-bucket listing scope) and three low findings. The medium findings represent defense-in-depth improvements rather than likely-to-hit bugs in normal operation — the existing self-healing mechanisms (slow-path fallback, re-discovery) provide recovery paths for the identified races.
Code Review
Blocking Issues
Missing GCS tests for swamp-club#2033 (bulk-diff listing): The bulk-diff remote listing feature was added to
gcs_cache_sync.ts(newlistAllObjectscall inpullChanged,pull404PartitionKeyspre-population from the listing, listing fallback on error). The S3 test file has two corresponding tests for this exact feature:"swamp-club#2033: pullChanged falls back to per-file behavior when listing fails"— verifies the graceful degradation path whenlistAllObjectsthrows"swamp-club#2033: pullChanged prunes stale entries via listing without per-file 404"— verifies that index entries absent from the listing are pruned without issuing agetObjectper fileNeither test has a GCS counterpart in
gcs_cache_sync_test.ts. The project's testing rules require "new functionality in vault/ or datastore/ extensions should have corresponding tests." The feature is present in the GCS implementation (identical logic to S3) but lacks coverage.Suggestions
Test fix for
"pullChanged: still throws on non-NotFound errors"(both S3 and GCS, already applied correctly): Seedingdata/file/v1/rawintomock.storageensures the new listing code doesn't prune it before the overriddengetObjectcan fire the auth error. The fix is correct; just noting it's load-bearing for the test's intended behavior.No-op pruning when listing fails: The listing call inside
pullChangeduses no prefix (undefined), which for a namespaced repo results in a full-bucket listing. If another namespace shares the same bucket and has a root-level object with the same relative path as a namespaced object (data/file.yamlat root vsmy-ns/data/file.yaml), the root entry would adddata/file.yamltoremoteKeys, preventing a legitimate prune of a missing namespaced object. The inline comment explains this is intentional to preserve the pre-namespace fallback behavior, andpullFilehandles the 404 correctly. This is fine as-is, but worth noting for future reviewers.Adversarial Code Review
PR Summary
This PR implements three improvements to both GCS and S3 datastore sync backends:
listAllObjectsto prune stale index entries before the download loop, reducing per-file 404 round-trips from O(files) to O(files/1000) list calls.commitSeqto the sidecar so subsequent pulls can short-circuit via the fast path.migrateRootDataToNamespacecall entirely (data is already namespaced) and marksdataKeyMigrated = truein the sidecar.Additionally:
2026.09.10.2.Findings
MEDIUM-1: Unbounded remote listing in bulk-diff may spike memory on large buckets
File:
gcs_cache_sync.ts(lines 1808-1824),s3_cache_sync.ts(lines 1901-1917)Dimension: Resource Management
The bulk-diff listing calls
listAllObjects(undefined, signal)which fetches every object under the configured bucket prefix. For repositories with millions of objects (e.g., long-lived multi-model datastores), this will:Set<string>— potentially hundreds of MB of string data.The catch-all fallback is correctly wired: on listing failure,
remoteKeysstaysnulland the code falls back to per-file 404 behavior. TheAbortSignalis passed through, providing external timeout cancellation.Mitigating factors: The listing is bounded by the datastore's configured prefix, and extremely large datastores already perform full listings in other code paths (e.g.,
repairNamespaceContamination,migrateRootDataToNamespace). This is a conscious trade-off per the comment.Recommendation: Consider adding a high-water mark — if the listing exceeds N entries (e.g., 500K), abort and fall back to per-file 404 behavior. This would bound memory without changing the happy path.
MEDIUM-2: Bulk-diff listing includes all namespaces, not just the active one
File:
gcs_cache_sync.ts(lines 1808-1824),s3_cache_sync.ts(lines 1901-1917)Dimension: Resource Management / Data Integrity
The listing passes
undefinedas the sub-prefix, meaning it returns objects from all namespaces in the bucket. The code strips the current namespace prefix from matching keys and adds non-matching keys as-is. For multi-namespace shared datastores, this means:Setcontains keys from all namespaces (unnecessary extra memory).This is documented as intentional ("root-level files from pre-namespace pushes are captured"), so the correctness impact is nil. The only cost is the larger-than-necessary listing.
Recommendation: No action needed for correctness. If performance matters, a namespace-scoped listing with a root-fallback listing would be more efficient, but adds complexity.
LOW-1: S3 mock
listAllObjectsdoes not acceptsignalparameterFile:
s3_cache_sync_test.ts(line 162)Dimension: API Contract Violations (test-only)
The S3 mock's
listAllObjects(subPrefix?: string)omits thesignalparameter that the realS3Client.listAllObjects(subPrefix?, signal?)accepts. JavaScript silently ignores extra arguments, so this doesn't cause test failures, but the mock doesn't honor abort signals. The GCS mock correctly includessignalin its signature (line 180).Recommendation: Add
signal?: AbortSignalto the S3 mock'slistAllObjectsfor parity with the GCS mock and the real client.LOW-2:
pull404PartitionKeysset now accumulates from two sources — comment could clarifyFile:
gcs_cache_sync.ts(line 1833),s3_cache_sync.ts(line 1927)Dimension: Logic & Correctness
The
pull404PartitionKeysset was moved from after the download loop to before the index walk. It now collects partition keys from two sources: (1) entries pruned by the bulk-diff listing, and (2) entries that 404 during download. The later shard-cleanup code consumes this set identically regardless of source. This is correct behavior — both sources represent entries whose remote object is gone — but the moved declaration and dual-source accumulation could surprise a future reader.Recommendation: A one-line comment at the declaration explaining the dual-source accumulation would help.
INFO-1: Test fix for "non-NotFound errors" is correct and necessary
File:
gcs_cache_sync_test.ts(line 4532),s3_cache_sync_test.ts(line 4867)Dimension: Logic & Correctness
Both test files add
mock.storage.set("data/file/v1/raw", ...)to seed the file in mock storage. Without this, the new bulk-diff listing would see the file is absent from the remote and prune it from the index, preventing thegetObjectoverride (which throws a 403) from ever firing. The fix ensures the listing includes the file so the download path is exercised. Correct and well-motivated.INFO-2: GCS and S3 implementations are structurally identical — consistent changes
Dimension: Logic & Correctness
All three features (#2033, #1931, #2091) are applied symmetrically to both backends with appropriate backend-specific differences:
remoteIndexGeneration: ""vs S3 usesremoteIndexETag: ""this.gcs.listAllObjects()vs S3 callsthis.s3.listAllObjects()GcsOperationErrorvsS3OperationError)The structural symmetry is well maintained. No divergence found.
INFO-3: Migration skip logic in
pushChangedandpreparePushis correctDimension: Logic & Correctness
The
assembled/prepAssembledvariables are assigned fromassembleIndexFromShardsorassembleDirtyShardsOnlyearlier in each method. When truthy, the v2 shard-first index was successfully assembled, meaning data is already namespace-scoped. SkippingmigrateRootDataToNamespaceand markingdataKeyMigrated = trueis the correct behavior — migration is a v1-to-v2 concern, not a v2-to-v2 concern. The else branches preserve the original migration logic unchanged.Dimension Summary
assertSafePathusage is unchanged.Object.entries()iteration is safe (snapshot).pull404PartitionKeysdual-source accumulation is correct. No new concurrency hazards.listAllObjectsmay spike memory on very large buckets. Mitigated by prefix scoping and existing precedent.signalparam onlistAllObjects.Verdict
PASS — No critical or high-severity findings. The two medium findings are resource-management concerns with clear mitigating factors and no correctness impact. The changes are well-structured, symmetrically applied, and thoroughly tested.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.