feat(datastores): add credential preflight and timeout guards #72

Merged

stack72 merged 5 commits from feat/credential-preflight-timeouts into main 2026-06-24 22:45:56 +00:00

Conversation 8 Commits 5 Files changed 9 +607 -25

stack72 commented 2026-06-24 19:26:24 +00:00

Owner

Copy link

Summary

• S3: Adds a preflightCredentials() method (3s timeout via Promise.race) called once before the first pullChanged/pushChanged operation. Disables IMDS credential lookup when not in a container environment (AWS_EC2_METADATA_DISABLED) to avoid the 1s+ hang on developer laptops. Credential errors flow through existing classifyAwsCredentialError/formatAwsCredentialHint.
• GCS: Adds AbortSignal.timeout(2s) to all three token-exchange fetch calls (tokenFromServiceAccount, tokenFromUserCredentials, tokenFromMetadataServer) — previously unbounded. Adds preflightCredentials() method (5s timeout) on GcsClient called once before first sync op. Wraps token timeout errors in headers() as GcsOperationError.
• Both: Cache sync services (S3CacheSyncService, GcsCacheSyncService) call ensurePreflight() once at the start of pullChanged/pushChanged.

ADV-1 (timeout alignment): 2s per-call token timeout, 5s chain timeout — no conflict.
ADV-2 (IMDS): Used AWS_EC2_METADATA_DISABLED env var (process-wide but acceptable — S3 datastore is the sole AWS SDK consumer) rather than adding @aws-sdk/credential-providers dependency which risks bundler/Deno npm compatibility issues.

Test plan

• S3: preflightCredentials succeeds on accessible bucket
• S3: preflightCredentials times out on stalled server (within 3s budget)
• S3: preflightCredentials propagates 403 with auth hint
• GCS: preflightCredentials succeeds with fast tokenFn
• GCS: preflightCredentials no-ops in emulator mode (no tokenFn)
• GCS: preflightCredentials times out on slow credential source (within 5s budget)
• GCS: preflightCredentials propagates token refresh errors with session-expired hints
• All 39 existing S3 tests pass
• All 29 existing GCS tests pass
• deno check, deno lint, deno fmt, deno install --frozen clean on both extensions

🤖 Generated with Claude Code

## Summary - **S3**: Adds a `preflightCredentials()` method (3s timeout via `Promise.race`) called once before the first `pullChanged`/`pushChanged` operation. Disables IMDS credential lookup when not in a container environment (`AWS_EC2_METADATA_DISABLED`) to avoid the 1s+ hang on developer laptops. Credential errors flow through existing `classifyAwsCredentialError`/`formatAwsCredentialHint`. - **GCS**: Adds `AbortSignal.timeout(2s)` to all three token-exchange fetch calls (`tokenFromServiceAccount`, `tokenFromUserCredentials`, `tokenFromMetadataServer`) — previously unbounded. Adds `preflightCredentials()` method (5s timeout) on `GcsClient` called once before first sync op. Wraps token timeout errors in `headers()` as `GcsOperationError`. - **Both**: Cache sync services (`S3CacheSyncService`, `GcsCacheSyncService`) call `ensurePreflight()` once at the start of `pullChanged`/`pushChanged`. ADV-1 (timeout alignment): 2s per-call token timeout, 5s chain timeout — no conflict. ADV-2 (IMDS): Used `AWS_EC2_METADATA_DISABLED` env var (process-wide but acceptable — S3 datastore is the sole AWS SDK consumer) rather than adding `@aws-sdk/credential-providers` dependency which risks bundler/Deno npm compatibility issues. ## Test plan - [x] S3: `preflightCredentials` succeeds on accessible bucket - [x] S3: `preflightCredentials` times out on stalled server (within 3s budget) - [x] S3: `preflightCredentials` propagates 403 with auth hint - [x] GCS: `preflightCredentials` succeeds with fast tokenFn - [x] GCS: `preflightCredentials` no-ops in emulator mode (no tokenFn) - [x] GCS: `preflightCredentials` times out on slow credential source (within 5s budget) - [x] GCS: `preflightCredentials` propagates token refresh errors with session-expired hints - [x] All 39 existing S3 tests pass - [x] All 29 existing GCS tests pass - [x] `deno check`, `deno lint`, `deno fmt`, `deno install --frozen` clean on both extensions 🤖 Generated with [Claude Code](https://claude.com/claude-code)

stack72 commented 2026-06-24 20:35:53 +00:00

Author

Owner

Copy link

Code Review

Result: PASS — No blocking issues found.

---

Summary

This PR adds credential preflight verification (preflightCredentials()) to both GcsClient and S3Client, and wires it into GcsCacheSyncService and S3CacheSyncService via a one-shot ensurePreflight() guard. The intent is to surface credential failures fast (before the first real data operation) with a hard timeout.

---

CLAUDE.md Compliance

• No any types: Confirmed. All new code uses explicit types.
• Named exports only: Confirmed. TOKEN_FETCH_TIMEOUT_MS, ADC_CHAIN_TIMEOUT_MS, PREFLIGHT_TIMEOUT_MS are all named exports.
• Pinned npm deps: No new npm dependencies introduced.
• deno.lock committed: datastore/gcs/deno.lock is updated and committed.
• No hand-edited model/ files: Confirmed. No model/ files touched.

---

Testing Rules

• No live cloud services: Confirmed. All new tests use local HTTP servers (Deno.serve({ port: 0 })) or in-memory mock clients.
• sanitizeResources: false with comments: The timeout test in gcs_client_test.ts uses sanitizeOps: false with an explanatory comment. Correct.
• Env var restoration: AWS_EC2_METADATA_DISABLED is saved/restored in a finally block in s3_client_test.ts. Correct.
• New functionality has tests: preflightCredentials() is tested in both gcs_client_test.ts and s3_client_test.ts. Mock clients in both *_cache_sync_test.ts files are updated to include preflightCredentials().

---

Security

• No credential leaks: Token values never appear in logs or error messages.
• No command injection: No shell commands involved.
• No path traversal: File paths remain unchanged from existing validated patterns.
• Deno.env.set scope: The AWS_EC2_METADATA_DISABLED mutation in S3Client's constructor is a process-wide side effect. It is guarded by a check for existing values and container-credential env vars, and the rationale is documented in a comment. No security concern, but see suggestions below.

---

Correctness

• Timeout behavior: Both GCS and S3 preflight use Promise.race against a setTimeout-based rejection with clearTimeout in a finally block. Correct — no timer leaks on the fast path.
• One-shot guard: preflightDone flag ensures preflight runs at most once per service instance regardless of concurrent callers (since JS is single-threaded, no race on the flag itself).
• probe.catch(() => {}) in GCS preflight: Prevents unhandled-rejection noise if the timeout fires first and the probe later rejects. Correct.
• AbortError/TimeoutError wrapping in headers(): Wraps these errors into GcsOperationError with context. Correct.

---

Suggestions (non-blocking)

1. GCS preflight ignores the caller's AbortSignal (gcs_cache_sync.ts:ensurePreflight, gcs_client.ts:preflightCredentials): GcsCacheSyncService.ensurePreflight() accepts no signal and GcsClient.preflightCredentials() is also signal-less. If a caller aborts pullChanged/pushChanged within the 5 s ADC_CHAIN_TIMEOUT_MS window, the GCS preflight continues running. S3's counterpart forwards the signal. This is safe — ADC_CHAIN_TIMEOUT_MS provides a hard upper bound — but the asymmetry with S3 is worth noting for future maintainers.

2. Process-wide env var mutation in S3Client constructor (s3_client.ts): Deno.env.set("AWS_EC2_METADATA_DISABLED", "true") affects all code in the process that reads this var, including any future S3Client instances. The existing comment explains the intent (prevent IMDS hang off-cloud). Consider whether this belongs in a one-time module-level init rather than every constructor invocation, though the idempotent guard (if (!Deno.env.get(...))) means repeated construction is safe in practice.

3. GCS formatBatchFailure preposition ternary is slightly verbose (gcs_cache_sync.ts): op === "pull" ? "from" : op === "delete" ? "from" : "to" can be simplified to op === "push" ? "to" : "from" to match the S3 sibling's style. Functionally equivalent.

## Code Review **Result: PASS** — No blocking issues found. --- ### Summary This PR adds credential preflight verification (`preflightCredentials()`) to both `GcsClient` and `S3Client`, and wires it into `GcsCacheSyncService` and `S3CacheSyncService` via a one-shot `ensurePreflight()` guard. The intent is to surface credential failures fast (before the first real data operation) with a hard timeout. --- ### CLAUDE.md Compliance - **No `any` types**: Confirmed. All new code uses explicit types. - **Named exports only**: Confirmed. `TOKEN_FETCH_TIMEOUT_MS`, `ADC_CHAIN_TIMEOUT_MS`, `PREFLIGHT_TIMEOUT_MS` are all named exports. - **Pinned npm deps**: No new npm dependencies introduced. - **`deno.lock` committed**: `datastore/gcs/deno.lock` is updated and committed. - **No hand-edited `model/` files**: Confirmed. No `model/` files touched. --- ### Testing Rules - **No live cloud services**: Confirmed. All new tests use local HTTP servers (`Deno.serve({ port: 0 })`) or in-memory mock clients. - **`sanitizeResources: false` with comments**: The timeout test in `gcs_client_test.ts` uses `sanitizeOps: false` with an explanatory comment. Correct. - **Env var restoration**: `AWS_EC2_METADATA_DISABLED` is saved/restored in a `finally` block in `s3_client_test.ts`. Correct. - **New functionality has tests**: `preflightCredentials()` is tested in both `gcs_client_test.ts` and `s3_client_test.ts`. Mock clients in both `*_cache_sync_test.ts` files are updated to include `preflightCredentials()`. --- ### Security - **No credential leaks**: Token values never appear in logs or error messages. - **No command injection**: No shell commands involved. - **No path traversal**: File paths remain unchanged from existing validated patterns. - **`Deno.env.set` scope**: The `AWS_EC2_METADATA_DISABLED` mutation in `S3Client`'s constructor is a process-wide side effect. It is guarded by a check for existing values and container-credential env vars, and the rationale is documented in a comment. No security concern, but see suggestions below. --- ### Correctness - **Timeout behavior**: Both GCS and S3 preflight use `Promise.race` against a `setTimeout`-based rejection with `clearTimeout` in a `finally` block. Correct — no timer leaks on the fast path. - **One-shot guard**: `preflightDone` flag ensures preflight runs at most once per service instance regardless of concurrent callers (since JS is single-threaded, no race on the flag itself). - **`probe.catch(() => {})` in GCS preflight**: Prevents unhandled-rejection noise if the timeout fires first and the probe later rejects. Correct. - **AbortError/TimeoutError wrapping in `headers()`**: Wraps these errors into `GcsOperationError` with context. Correct. --- ### Suggestions (non-blocking) 1. **GCS preflight ignores the caller's AbortSignal** (`gcs_cache_sync.ts:ensurePreflight`, `gcs_client.ts:preflightCredentials`): `GcsCacheSyncService.ensurePreflight()` accepts no signal and `GcsClient.preflightCredentials()` is also signal-less. If a caller aborts `pullChanged`/`pushChanged` within the 5 s `ADC_CHAIN_TIMEOUT_MS` window, the GCS preflight continues running. S3's counterpart forwards the signal. This is safe — `ADC_CHAIN_TIMEOUT_MS` provides a hard upper bound — but the asymmetry with S3 is worth noting for future maintainers. 2. **Process-wide env var mutation in `S3Client` constructor** (`s3_client.ts`): `Deno.env.set("AWS_EC2_METADATA_DISABLED", "true")` affects all code in the process that reads this var, including any future `S3Client` instances. The existing comment explains the intent (prevent IMDS hang off-cloud). Consider whether this belongs in a one-time module-level init rather than every constructor invocation, though the idempotent guard (`if (!Deno.env.get(...))`) means repeated construction is safe in practice. 3. **GCS `formatBatchFailure` preposition ternary is slightly verbose** (`gcs_cache_sync.ts`): `op === "pull" ? "from" : op === "delete" ? "from" : "to"` can be simplified to `op === "push" ? "to" : "from"` to match the S3 sibling's style. Functionally equivalent.

stack72 commented 2026-06-24 21:07:44 +00:00

Author

Owner

Copy link

Adversarial Code Review — GCS & S3 Datastore Sync

Summary

Reviewed 8 changed files across the GCS and S3 datastore cache sync implementations. The GCS scoped-walk error handling has two HIGH-severity issues that can cause silent remote data deletion under non-exotic conditions (transient filesystem errors, lazy hydration). The S3 implementation handles both cases correctly, suggesting the GCS code was ported but missed guards added later.

---

CRITICAL / HIGH

H-1 · GCS scoped walk bare catch schedules remote deletion on any filesystem error

File	datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1197
Dimension	Data Integrity / Error Handling
Severity	HIGH

What: The scoped walk's catch block at line 1197 is a bare catch with no error type discrimination. When Deno.stat() throws for any reason — permission denied, I/O timeout, NFS stale handle, disk error — the code falls through to line 1202 and schedules all matching index entries for remote deletion.

S3 comparison: The equivalent S3 code at s3_cache_sync.ts:1330-1336 correctly discriminates:

} catch (err) {
  if (!(err instanceof Deno.errors.NotFound)) {
    // Non-absence error — skip silently, next push retries
    continue;
  }
  // genuinely absent — schedule deletion

Breaking scenario:

1. NFS-mounted cache dir has a brief I/O hiccup during pushChanged.
2. Deno.stat() throws a generic Error (not Deno.errors.NotFound).
3. The catch block at line 1197 fires — all index entries matching the dirty path are collected into toDelete.
4. pushChanged deletes those entries from the remote GCS bucket.
5. Data is permanently lost from the remote store because of a transient local filesystem error.

Suggested fix:

} catch (err) {
  if (!(err instanceof Deno.errors.NotFound)) {
    continue;
  }
  // Path is genuinely absent — schedule matching entries for deletion

---

H-2 · GCS scoped walk catch block missing lazyPullActive guard — deletes un-hydrated files

File	datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1202
Dimension	Data Integrity / Logic
Severity	HIGH

What: When a dirty path is absent on disk and the catch block fires, GCS unconditionally schedules matching index entries for remote deletion (line 1202: if (this.index)). It does not check this.lazyPullActive. During lazy hydration, raw data files are intentionally not pulled to disk — they are absent by design, not because the user deleted them. The comment on line 1199-1201 explicitly acknowledges this tradeoff ("wins over the lazy hydration guard") but the S3 implementation chose the safer default.

S3 comparison: At s3_cache_sync.ts:1341:

if (!this.lazyPullActive && this.index) {

S3 blanket-skips remote deletion scheduling when lazyPullActive is true, which is the safe default: you cannot distinguish "absent because un-hydrated" from "absent because deleted" without additional state.

Breaking scenario:

1. Lazy pull is performed — raw data files remain on-disk only as metadata stubs.
2. An external trigger calls markDirty("data/models/raw") on the GCS sync service.
3. Next pushChanged enters the scoped walk. Deno.stat throws NotFound for the un-hydrated path.
4. The bare catch fires, and all index entries under data/models/ are scheduled for deletion.
5. Remote raw files are deleted from GCS — data loss.

Suggested fix: Add the lazyPullActive guard:

} catch (err) {
  if (!(err instanceof Deno.errors.NotFound)) {
    continue;
  }
  if (!this.lazyPullActive && this.index) {

---

MEDIUM

M-1 · GCS scoped walk skips within-directory deletion detection

File	datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1186-1196
Dimension	Logic / Data Integrity
Severity	MEDIUM

What: When a dirty path resolves to a directory, GCS walks it and pushes changed files (lines 1186-1195), but does NOT collect localFilesInDir or compare against the index to detect files that were deleted within that directory.

S3 comparison: At s3_cache_sync.ts:1305-1327, S3 collects localFilesInDir and then scans index entries under the directory prefix, scheduling deletion for any that are not found locally (with a lazyPullActive guard).

Impact: If a user deletes a single file inside a dirty directory, the GCS scoped walk will push the remaining files but silently fail to propagate the deletion to GCS. The deletion would eventually be caught on the next bulk-invalidated push, but the intermediate state is inconsistent. This is a correctness gap, not data loss — it errs on the side of keeping extra data.

Suggested fix: Add localFilesInDir tracking and index comparison, mirroring the S3 pattern.

---

M-2 · GCS bulk-walk deletion gate uses bulkInvalidated instead of dirtyPathsOverflowed

File	datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1232
Dimension	Logic
Severity	MEDIUM

What: The GCS bulk walk gates orphan deletion on this.bulkInvalidated (line 1232). bulkInvalidated is set by three triggers: (1) dirty path cap overflow, (2) path escaping cache dir, (3) no-relPath markDirty() call (legacy/pushFile path). The S3 version uses this.dirtyPathsOverflowed (line 1375), which is only set by trigger (1).

Behavioral difference: A markDirty() call without relPath (e.g., from pushFile or a legacy caller) sets bulkInvalidated = true without dirtyPathsOverflowed = true. In S3, this means the bulk walk will not delete orphans — the no-relPath call is treated as "something changed" not "deleted files should be cleaned up." In GCS, the same call enables orphan deletion.

GCS also has no dirtyPathsOverflowed field at all — it was never ported from S3.

Impact: A sequence of pushFile then markDirty() then pushChanged on GCS could delete remote files that are merely un-fetched locally, if the local cache is a subset of the remote. The risk is amplified for reader-side repos that only have partial content locally.

Suggested fix: Add dirtyPathsOverflowed state to GCS (mirroring S3's DatastoreSyncStateV2 field) and use it instead of bulkInvalidated for the bulk walk deletion gate. Also add the !this.lazyPullActive guard that S3 has.

---

LOW

L-1 · Module-level GCS token cache shared across all client instances

File	datastore/gcs/extensions/datastores/_lib/gcs_client.ts:243
Dimension	Resource Management
Severity	LOW

What: cachedToken at line 243 is a module-level let variable. All GcsClient instances in the same Deno isolate share this single cache slot. If two GcsClient instances are constructed with different credential sources (e.g., different service account key paths via GOOGLE_APPLICATION_CREDENTIALS), the second instance's getToken() call may reuse a token minted for the first instance's credentials, leading to 403 errors.

In practice: This is low severity because typical deployment has one credential per process. It becomes a problem only in multi-tenant scenarios or tests that construct clients with different credentials in the same process.

Suggested fix: Move cachedToken to an instance field on GcsClient.

---

Informational

I-1 · GCS markDirty does not set dirtyPathsOverflowed on cap overflow

Note that the GCS markDirty at line 557-560 sets bulkInvalidated = true on dirty path cap overflow but has no dirtyPathsOverflowed flag. This is the root cause of M-2 — the two concepts (bulk invalidation and overflow-triggered deletion) are conflated in GCS but correctly separated in S3.

I-2 · Both implementations use Deno.writeFile (non-atomic) in pullFile

Both GCS (gcs_cache_sync.ts) and S3 (s3_cache_sync.ts) pullFile methods use Deno.writeFile to write downloaded content to disk. This is not atomic — a crash mid-write leaves a partial file. Both implementations self-heal on next sync via fingerprint mismatch, so the window of inconsistency is small. Mentioning for completeness; not actionable unless crash-consistency requirements tighten.

---

Verdict

FAIL — 2 HIGH findings (H-1, H-2) require changes before merge. The GCS scoped walk error handling diverges from the S3 implementation in ways that can cause silent data deletion under recoverable error conditions and during lazy hydration.

# Adversarial Code Review — GCS & S3 Datastore Sync ## Summary Reviewed 8 changed files across the GCS and S3 datastore cache sync implementations. The GCS scoped-walk error handling has two HIGH-severity issues that can cause silent remote data deletion under non-exotic conditions (transient filesystem errors, lazy hydration). The S3 implementation handles both cases correctly, suggesting the GCS code was ported but missed guards added later. --- ## CRITICAL / HIGH ### H-1 · GCS scoped walk bare catch schedules remote deletion on any filesystem error | | | |---|---| | **File** | `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1197` | | **Dimension** | Data Integrity / Error Handling | | **Severity** | **HIGH** | **What:** The scoped walk's `catch` block at line 1197 is a bare `catch` with no error type discrimination. When `Deno.stat()` throws for *any* reason — permission denied, I/O timeout, NFS stale handle, disk error — the code falls through to line 1202 and schedules all matching index entries for remote deletion. **S3 comparison:** The equivalent S3 code at `s3_cache_sync.ts:1330-1336` correctly discriminates: } catch (err) { if (!(err instanceof Deno.errors.NotFound)) { // Non-absence error — skip silently, next push retries continue; } // genuinely absent — schedule deletion **Breaking scenario:** 1. NFS-mounted cache dir has a brief I/O hiccup during `pushChanged`. 2. `Deno.stat()` throws a generic `Error` (not `Deno.errors.NotFound`). 3. The catch block at line 1197 fires — all index entries matching the dirty path are collected into `toDelete`. 4. `pushChanged` deletes those entries from the remote GCS bucket. 5. Data is permanently lost from the remote store because of a transient local filesystem error. **Suggested fix:** } catch (err) { if (!(err instanceof Deno.errors.NotFound)) { continue; } // Path is genuinely absent — schedule matching entries for deletion --- ### H-2 · GCS scoped walk catch block missing lazyPullActive guard — deletes un-hydrated files | | | |---|---| | **File** | `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1202` | | **Dimension** | Data Integrity / Logic | | **Severity** | **HIGH** | **What:** When a dirty path is absent on disk and the catch block fires, GCS unconditionally schedules matching index entries for remote deletion (line 1202: `if (this.index)`). It does not check `this.lazyPullActive`. During lazy hydration, raw data files are intentionally *not* pulled to disk — they are absent by design, not because the user deleted them. The comment on line 1199-1201 explicitly acknowledges this tradeoff ("wins over the lazy hydration guard") but the S3 implementation chose the safer default. **S3 comparison:** At `s3_cache_sync.ts:1341`: if (!this.lazyPullActive && this.index) { S3 blanket-skips remote deletion scheduling when `lazyPullActive` is true, which is the safe default: you cannot distinguish "absent because un-hydrated" from "absent because deleted" without additional state. **Breaking scenario:** 1. Lazy pull is performed — raw data files remain on-disk only as metadata stubs. 2. An external trigger calls `markDirty("data/models/raw")` on the GCS sync service. 3. Next `pushChanged` enters the scoped walk. `Deno.stat` throws `NotFound` for the un-hydrated path. 4. The bare catch fires, and all index entries under `data/models/` are scheduled for deletion. 5. Remote raw files are deleted from GCS — data loss. **Suggested fix:** Add the `lazyPullActive` guard: } catch (err) { if (!(err instanceof Deno.errors.NotFound)) { continue; } if (!this.lazyPullActive && this.index) { --- ## MEDIUM ### M-1 · GCS scoped walk skips within-directory deletion detection | | | |---|---| | **File** | `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1186-1196` | | **Dimension** | Logic / Data Integrity | | **Severity** | **MEDIUM** | **What:** When a dirty path resolves to a directory, GCS walks it and pushes changed files (lines 1186-1195), but does NOT collect `localFilesInDir` or compare against the index to detect files that were deleted within that directory. **S3 comparison:** At `s3_cache_sync.ts:1305-1327`, S3 collects `localFilesInDir` and then scans index entries under the directory prefix, scheduling deletion for any that are not found locally (with a `lazyPullActive` guard). **Impact:** If a user deletes a single file inside a dirty directory, the GCS scoped walk will push the remaining files but silently fail to propagate the deletion to GCS. The deletion would eventually be caught on the next bulk-invalidated push, but the intermediate state is inconsistent. This is a correctness gap, not data loss — it errs on the side of keeping extra data. **Suggested fix:** Add `localFilesInDir` tracking and index comparison, mirroring the S3 pattern. --- ### M-2 · GCS bulk-walk deletion gate uses bulkInvalidated instead of dirtyPathsOverflowed | | | |---|---| | **File** | `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1232` | | **Dimension** | Logic | | **Severity** | **MEDIUM** | **What:** The GCS bulk walk gates orphan deletion on `this.bulkInvalidated` (line 1232). `bulkInvalidated` is set by three triggers: (1) dirty path cap overflow, (2) path escaping cache dir, (3) no-relPath `markDirty()` call (legacy/pushFile path). The S3 version uses `this.dirtyPathsOverflowed` (line 1375), which is only set by trigger (1). **Behavioral difference:** A `markDirty()` call without `relPath` (e.g., from `pushFile` or a legacy caller) sets `bulkInvalidated = true` without `dirtyPathsOverflowed = true`. In S3, this means the bulk walk will not delete orphans — the no-relPath call is treated as "something changed" not "deleted files should be cleaned up." In GCS, the same call enables orphan deletion. GCS also has no `dirtyPathsOverflowed` field at all — it was never ported from S3. **Impact:** A sequence of `pushFile` then `markDirty()` then `pushChanged` on GCS could delete remote files that are merely un-fetched locally, if the local cache is a subset of the remote. The risk is amplified for reader-side repos that only have partial content locally. **Suggested fix:** Add `dirtyPathsOverflowed` state to GCS (mirroring S3's `DatastoreSyncStateV2` field) and use it instead of `bulkInvalidated` for the bulk walk deletion gate. Also add the `!this.lazyPullActive` guard that S3 has. --- ## LOW ### L-1 · Module-level GCS token cache shared across all client instances | | | |---|---| | **File** | `datastore/gcs/extensions/datastores/_lib/gcs_client.ts:243` | | **Dimension** | Resource Management | | **Severity** | **LOW** | **What:** `cachedToken` at line 243 is a module-level `let` variable. All `GcsClient` instances in the same Deno isolate share this single cache slot. If two `GcsClient` instances are constructed with different credential sources (e.g., different service account key paths via `GOOGLE_APPLICATION_CREDENTIALS`), the second instance's `getToken()` call may reuse a token minted for the first instance's credentials, leading to 403 errors. **In practice:** This is low severity because typical deployment has one credential per process. It becomes a problem only in multi-tenant scenarios or tests that construct clients with different credentials in the same process. **Suggested fix:** Move `cachedToken` to an instance field on `GcsClient`. --- ## Informational ### I-1 · GCS markDirty does not set dirtyPathsOverflowed on cap overflow Note that the GCS `markDirty` at line 557-560 sets `bulkInvalidated = true` on dirty path cap overflow but has no `dirtyPathsOverflowed` flag. This is the root cause of M-2 — the two concepts (bulk invalidation and overflow-triggered deletion) are conflated in GCS but correctly separated in S3. ### I-2 · Both implementations use Deno.writeFile (non-atomic) in pullFile Both GCS (`gcs_cache_sync.ts`) and S3 (`s3_cache_sync.ts`) `pullFile` methods use `Deno.writeFile` to write downloaded content to disk. This is not atomic — a crash mid-write leaves a partial file. Both implementations self-heal on next sync via fingerprint mismatch, so the window of inconsistency is small. Mentioning for completeness; not actionable unless crash-consistency requirements tighten. --- ## Verdict **FAIL** — 2 HIGH findings (H-1, H-2) require changes before merge. The GCS scoped walk error handling diverges from the S3 implementation in ways that can cause silent data deletion under recoverable error conditions and during lazy hydration.

stack72 commented 2026-06-24 21:32:24 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

1. GCS pushChanged scoped walk: isDirectory branch missing per-file deletion detection (gcs_cache_sync.ts)

When the scoped walk processes a dirty path that resolves to a still-existing directory, the GCS implementation does NOT detect files that have been deleted from within that directory. The S3 sibling (s3_cache_sync.ts) correctly handles this with a two-phase approach:

Phase 1 (same as GCS): walk the directory, collect files to push via fileNeedsPush.
Phase 2 (MISSING from GCS): build a set of all local files found during the walk, then scan this.index.entries for any entry under the dirtyPath prefix that is NOT in the local set -- and add those orphans to toDelete.

Impact: If markDirty is called with a relPath pointing to a parent directory, and then one file inside that directory is deleted while the directory and other files survive, the GCS implementation silently leaves the orphaned object in GCS. The S3 implementation correctly detects and deletes it.

The existing del1 test ("pushChanged: scoped walk deletes absent dirty path from GCS and index") only covers the case where the ENTIRE marked directory is removed -- handled by the catch (err instanceof Deno.errors.NotFound) path. There is no test for partial deletion within a surviving directory, so this regression has no coverage.

Fix needed: Port the localFilesInDir set pattern from s3_cache_sync.ts isDirectory branch into gcs_cache_sync.ts. After the walk loop, check this.index.entries for keys starting with the dirtyPath prefix that are absent from localFilesInDir (guarded by lazyPullActive), and push those to toDelete. Also add a del5 test: mark a directory dirty, delete one file within it while leaving others, assert the deleted file is removed from GCS and from the index.

---

Suggestions

1. Document the semantic divergence between bulkInvalidated (GCS) and dirtyPathsOverflowed (S3) (gcs_cache_sync.ts bulk walk, s3_cache_sync.ts bulk walk)

S3's bulk walk deletes orphaned entries only when dirtyPathsOverflowed is true (path count exceeded DIRTY_PATHS_CAP). Its code comments explicitly state that a no-relPath markDirty() is a modification signal, not a deletion signal -- S3 does NOT delete orphans in that case. GCS uses bulkInvalidated as the gate, which is set by BOTH no-relPath markDirty() calls AND path overflow. This means calling markDirty() with no relPath triggers orphan deletion in GCS but not in S3. The GCS del2 test confirms this is intentional, but a short comment in the bulk-walk deletion block documenting the divergence from S3 semantics would help future maintainers.

2. Misleading comment in DEF-2 integration test (gcs_cache_sync_test.ts)

A comment describes "monkey-patching the module-level default via a dedicated assertion path" but the code simply calls retryWithBackoff directly with a custom config argument -- no patching occurs. The comment should be removed or corrected.

3. Simplify formatBatchFailure preposition ternary (gcs_cache_sync.ts)

The three-way ternary (op === "pull" ? "from" : op === "delete" ? "from" : "to") can be replaced with a clearer two-way form (op === "push" ? "to" : "from") to match the S3 sibling's style.

## Code Review ### Blocking Issues **1. GCS pushChanged scoped walk: isDirectory branch missing per-file deletion detection** (gcs_cache_sync.ts) When the scoped walk processes a dirty path that resolves to a still-existing directory, the GCS implementation does NOT detect files that have been deleted from within that directory. The S3 sibling (s3_cache_sync.ts) correctly handles this with a two-phase approach: Phase 1 (same as GCS): walk the directory, collect files to push via fileNeedsPush. Phase 2 (MISSING from GCS): build a set of all local files found during the walk, then scan this.index.entries for any entry under the dirtyPath prefix that is NOT in the local set -- and add those orphans to toDelete. **Impact**: If markDirty is called with a relPath pointing to a parent directory, and then one file inside that directory is deleted while the directory and other files survive, the GCS implementation silently leaves the orphaned object in GCS. The S3 implementation correctly detects and deletes it. The existing del1 test ("pushChanged: scoped walk deletes absent dirty path from GCS and index") only covers the case where the ENTIRE marked directory is removed -- handled by the catch (err instanceof Deno.errors.NotFound) path. There is no test for partial deletion within a surviving directory, so this regression has no coverage. **Fix needed**: Port the localFilesInDir set pattern from s3_cache_sync.ts isDirectory branch into gcs_cache_sync.ts. After the walk loop, check this.index.entries for keys starting with the dirtyPath prefix that are absent from localFilesInDir (guarded by lazyPullActive), and push those to toDelete. Also add a del5 test: mark a directory dirty, delete one file within it while leaving others, assert the deleted file is removed from GCS and from the index. --- ### Suggestions **1. Document the semantic divergence between bulkInvalidated (GCS) and dirtyPathsOverflowed (S3)** (gcs_cache_sync.ts bulk walk, s3_cache_sync.ts bulk walk) S3's bulk walk deletes orphaned entries only when dirtyPathsOverflowed is true (path count exceeded DIRTY_PATHS_CAP). Its code comments explicitly state that a no-relPath markDirty() is a modification signal, not a deletion signal -- S3 does NOT delete orphans in that case. GCS uses bulkInvalidated as the gate, which is set by BOTH no-relPath markDirty() calls AND path overflow. This means calling markDirty() with no relPath triggers orphan deletion in GCS but not in S3. The GCS del2 test confirms this is intentional, but a short comment in the bulk-walk deletion block documenting the divergence from S3 semantics would help future maintainers. **2. Misleading comment in DEF-2 integration test** (gcs_cache_sync_test.ts) A comment describes "monkey-patching the module-level default via a dedicated assertion path" but the code simply calls retryWithBackoff directly with a custom config argument -- no patching occurs. The comment should be removed or corrected. **3. Simplify formatBatchFailure preposition ternary** (gcs_cache_sync.ts) The three-way ternary (op === "pull" ? "from" : op === "delete" ? "from" : "to") can be replaced with a clearer two-way form (op === "push" ? "to" : "from") to match the S3 sibling's style.

stack72 commented 2026-06-24 21:59:23 +00:00

Author

Owner

Copy link

Adversarial Review

High

H-1: GCS scoped walk treats ALL stat errors as deletion intent (data loss risk)

File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1197-1209
Dimension: Logic and Correctness, Data Integrity
Severity: HIGH

The scoped dirty-path walk wraps Deno.stat() and the subsequent directory walk in a single try block with a bare catch. Any error — Deno.errors.PermissionDenied, Deno.errors.Interrupted, NFS timeout, or an unexpected runtime error from walk() — is silently interpreted as “file absent on disk” and schedules remote object deletion.

GCS (lines 1197-1209):

} catch {
  if (this.index) {
    for (const key of Object.keys(this.index.entries)) {
      if (isInternalCacheFile(key)) continue;
      if (key === dirtyPath || key.startsWith(dirtyPath + "/")) {
        toDelete.push(key);
      }
    }
  }
}

S3 correctly distinguishes (lines 1330-1336):

} catch (err) {
  if (!(err instanceof Deno.errors.NotFound)) {
    continue;
  }
}

Impact: A transient permission error or I/O failure during a scoped push could silently delete objects from GCS, causing irreversible data loss. This is a correctness divergence from the S3 backend that changes the safety contract of pushChanged.

Recommendation: Guard the catch block identically to S3: only enter the deletion path for Deno.errors.NotFound, and continue for all other errors.

---

H-2: GCS scoped walk missing lazyPullActive guard on deletion path

File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1202
Dimension: Data Integrity, Logic and Correctness
Severity: HIGH

In the bare-catch deletion branch, GCS checks only if (this.index) before scheduling remote deletions. S3 checks if (!this.lazyPullActive && this.index) (line 1341).

When lazy hydration is active, files that have not been pulled yet are absent on disk by design — they are un-hydrated, not deleted. Without the lazyPullActive guard, GCS will interpret these absent-but-un-hydrated files as deleted and remove them from the remote store.

Impact: A lazy-pull repo that runs pushChanged with a single dirty path will delete every un-hydrated file under that prefix from GCS.

Recommendation: Change line 1202 to if (!this.lazyPullActive && this.index), mirroring S3 line 1341.

---

Medium

M-1: GCS scoped directory walk does not detect within-directory deletions

File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1186-1196
Dimension: Logic and Correctness
Severity: MEDIUM

When a dirty path resolves to a directory that still exists, GCS walks it for files to push but never checks whether any indexed files under that directory were deleted locally. S3 (lines 1304-1328) builds a localFilesInDir set and cross-references the index to find orphans.

GCS has no equivalent logic. If a user deletes a file inside a dirty directory (without deleting the directory itself), GCS will never schedule it for remote deletion.

Impact: Deleted files persist as ghost objects in GCS and reappear on the next pull, making file deletion within directories ineffective.

Recommendation: Port the localFilesInDir pattern from S3 into the GCS isDirectory branch.

---

M-2: GCS ensurePreflight() does not accept or forward AbortSignal

File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:454-457
Dimension: API Contract, Cancellation
Severity: MEDIUM

GCS ensurePreflight() (line 454) takes no parameters and calls this.gcs.preflightCredentials() without a signal. S3 (line 497) accepts an optional AbortSignal and forwards it:

// GCS (line 454)
private async ensurePreflight(): Promise<void> {
  await this.gcs.preflightCredentials();
}
// S3 (line 497)
private async ensurePreflight(signal?: AbortSignal): Promise<void> {
  await this.s3.preflightCredentials(signal);
}

Impact: When a caller cancels a GCS pull/push via AbortSignal, the preflight credential check runs to completion (or its internal timeout) instead of aborting immediately.

Recommendation: Add signal?: AbortSignal to ensurePreflight and forward it to preflightCredentials. Update both call sites (lines 949 and 1151) to pass signal.

---

M-3: Divergent deletion semantics between bulkInvalidated (GCS) and dirtyPathsOverflowed (S3)

File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1232 vs datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts:1375
Dimension: Logic and Correctness
Severity: MEDIUM

In the full-walk branch, GCS guards deletion on this.bulkInvalidated (line 1232) while S3 guards on this.dirtyPathsOverflowed (line 1375). The GCS sync state schema (DatastoreSyncStateV2) does not include dirtyPathsOverflowed at all.

These are not the same condition. bulkInvalidated is set when the entire cache is marked dirty; dirtyPathsOverflowed is set when the per-path tracking set exceeds its cap and falls back to full walk.

Impact: Depending on how the core sets these flags, one backend may delete remote files in scenarios where the other does not, leading to subtle cross-backend behavior differences.

Recommendation: Align both backends on the same guard condition. If dirtyPathsOverflowed is the correct semantic, add it to the GCS sync state and use it instead of bulkInvalidated.

---

Low

L-1: isInternalCacheFile may not match namespace-prefixed internal files

File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:103-114
Dimension: Defensive Coding
Severity: LOW

isInternalCacheFile checks for exact filenames (.datastore-index.json, .push-queue.json, .datastore.lock, _index, etc.) and basename patterns (.lock, _catalog.db). However, when a namespace is bound via bindNamespace, the cache path itself changes but the relative paths passed to isInternalCacheFile may still include a namespace prefix. If a namespace prefix ever appears in the relative path, the exact-match checks would fail and internal files could be uploaded to the remote store.

Impact: Low risk currently — the namespace binding appears to change this.cachePath itself, so relative paths should not carry the prefix. But the function is brittle and would silently fail if the path-construction logic ever changes.

Recommendation: Add a defensive check that strips any leading namespace prefix before matching, or add a test that verifies internal files are never uploaded when a namespace is bound.

---

Verdict

BLOCKING — do not merge.

Required before merge:

1. Fix H-1: narrow the catch block at line 1197 to only act on Deno.errors.NotFound
2. Fix H-2: add !this.lazyPullActive guard at line 1202
3. Fix M-1: implement localFilesInDir cross-reference in the isDirectory branch
4. Fix M-2: add signal parameter to ensurePreflight and forward it
5. Add tests for all four fixes, particularly the deletion-path scenarios

## Adversarial Review ### High #### H-1: GCS scoped walk treats ALL stat errors as deletion intent (data loss risk) **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1197-1209` **Dimension:** Logic and Correctness, Data Integrity **Severity:** HIGH The scoped dirty-path walk wraps `Deno.stat()` and the subsequent directory walk in a single `try` block with a bare `catch`. Any error — `Deno.errors.PermissionDenied`, `Deno.errors.Interrupted`, NFS timeout, or an unexpected runtime error from `walk()` — is silently interpreted as “file absent on disk” and schedules remote object deletion. GCS (lines 1197-1209): ```typescript } catch { if (this.index) { for (const key of Object.keys(this.index.entries)) { if (isInternalCacheFile(key)) continue; if (key === dirtyPath || key.startsWith(dirtyPath + "/")) { toDelete.push(key); } } } } ``` S3 correctly distinguishes (lines 1330-1336): ```typescript } catch (err) { if (!(err instanceof Deno.errors.NotFound)) { continue; } } ``` **Impact:** A transient permission error or I/O failure during a scoped push could silently delete objects from GCS, causing irreversible data loss. This is a correctness divergence from the S3 backend that changes the safety contract of `pushChanged`. **Recommendation:** Guard the catch block identically to S3: only enter the deletion path for `Deno.errors.NotFound`, and `continue` for all other errors. --- #### H-2: GCS scoped walk missing lazyPullActive guard on deletion path **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1202` **Dimension:** Data Integrity, Logic and Correctness **Severity:** HIGH In the bare-catch deletion branch, GCS checks only `if (this.index)` before scheduling remote deletions. S3 checks `if (!this.lazyPullActive && this.index)` (line 1341). When lazy hydration is active, files that have not been pulled yet are absent on disk by design — they are un-hydrated, not deleted. Without the `lazyPullActive` guard, GCS will interpret these absent-but-un-hydrated files as deleted and remove them from the remote store. **Impact:** A lazy-pull repo that runs `pushChanged` with a single dirty path will delete every un-hydrated file under that prefix from GCS. **Recommendation:** Change line 1202 to `if (!this.lazyPullActive && this.index)`, mirroring S3 line 1341. --- ### Medium #### M-1: GCS scoped directory walk does not detect within-directory deletions **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1186-1196` **Dimension:** Logic and Correctness **Severity:** MEDIUM When a dirty path resolves to a directory that still exists, GCS walks it for files to push but never checks whether any indexed files under that directory were deleted locally. S3 (lines 1304-1328) builds a `localFilesInDir` set and cross-references the index to find orphans. GCS has no equivalent logic. If a user deletes a file inside a dirty directory (without deleting the directory itself), GCS will never schedule it for remote deletion. **Impact:** Deleted files persist as ghost objects in GCS and reappear on the next pull, making file deletion within directories ineffective. **Recommendation:** Port the `localFilesInDir` pattern from S3 into the GCS `isDirectory` branch. --- #### M-2: GCS `ensurePreflight()` does not accept or forward AbortSignal **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:454-457` **Dimension:** API Contract, Cancellation **Severity:** MEDIUM GCS `ensurePreflight()` (line 454) takes no parameters and calls `this.gcs.preflightCredentials()` without a signal. S3 (line 497) accepts an optional `AbortSignal` and forwards it: ```typescript // GCS (line 454) private async ensurePreflight(): Promise<void> { await this.gcs.preflightCredentials(); } // S3 (line 497) private async ensurePreflight(signal?: AbortSignal): Promise<void> { await this.s3.preflightCredentials(signal); } ``` **Impact:** When a caller cancels a GCS pull/push via `AbortSignal`, the preflight credential check runs to completion (or its internal timeout) instead of aborting immediately. **Recommendation:** Add `signal?: AbortSignal` to `ensurePreflight` and forward it to `preflightCredentials`. Update both call sites (lines 949 and 1151) to pass `signal`. --- #### M-3: Divergent deletion semantics between `bulkInvalidated` (GCS) and `dirtyPathsOverflowed` (S3) **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:1232` vs `datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts:1375` **Dimension:** Logic and Correctness **Severity:** MEDIUM In the full-walk branch, GCS guards deletion on `this.bulkInvalidated` (line 1232) while S3 guards on `this.dirtyPathsOverflowed` (line 1375). The GCS sync state schema (`DatastoreSyncStateV2`) does not include `dirtyPathsOverflowed` at all. These are not the same condition. `bulkInvalidated` is set when the entire cache is marked dirty; `dirtyPathsOverflowed` is set when the per-path tracking set exceeds its cap and falls back to full walk. **Impact:** Depending on how the core sets these flags, one backend may delete remote files in scenarios where the other does not, leading to subtle cross-backend behavior differences. **Recommendation:** Align both backends on the same guard condition. If `dirtyPathsOverflowed` is the correct semantic, add it to the GCS sync state and use it instead of `bulkInvalidated`. --- ### Low #### L-1: `isInternalCacheFile` may not match namespace-prefixed internal files **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:103-114` **Dimension:** Defensive Coding **Severity:** LOW `isInternalCacheFile` checks for exact filenames (`.datastore-index.json`, `.push-queue.json`, `.datastore.lock`, `_index`, etc.) and basename patterns (`.lock`, `_catalog.db`). However, when a namespace is bound via `bindNamespace`, the cache path itself changes but the relative paths passed to `isInternalCacheFile` may still include a namespace prefix. If a namespace prefix ever appears in the relative path, the exact-match checks would fail and internal files could be uploaded to the remote store. **Impact:** Low risk currently — the namespace binding appears to change `this.cachePath` itself, so relative paths should not carry the prefix. But the function is brittle and would silently fail if the path-construction logic ever changes. **Recommendation:** Add a defensive check that strips any leading namespace prefix before matching, or add a test that verifies internal files are never uploaded when a namespace is bound. --- ### Verdict BLOCKING — do not merge. **Required before merge:** 1. Fix H-1: narrow the catch block at line 1197 to only act on `Deno.errors.NotFound` 2. Fix H-2: add `!this.lazyPullActive` guard at line 1202 3. Fix M-1: implement `localFilesInDir` cross-reference in the `isDirectory` branch 4. Fix M-2: add `signal` parameter to `ensurePreflight` and forward it 5. Add tests for all four fixes, particularly the deletion-path scenarios

stack72 commented 2026-06-24 22:03:11 +00:00

Author

Owner

Copy link

Code Review

Result: APPROVED — no blocking issues found.

---

CLAUDE.md Compliance

• No any types in hand-written code. Test mocks use as unknown as GcsClient & {...} for controlled upcasting in test infrastructure, which is acceptable.
• Named exports only, no default exports across all changed files.
• No semver ranges in npm imports; @aws-sdk/client-s3@3.1046.0 is pinned to an exact version.
• deno.lock is present and committed for the GCS extension.
• No files under model/ were modified.

Testing Rules

• All integration tests use Deno.serve({ port: 0 }) (GCS/S3 client tests) or in-memory mock clients (createMockGcsClient) — no live cloud services.
• S3 tests restore AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY in finally blocks. GCS tests use an emulator-mode ADC bypass that requires no env var teardown.
• sanitizeResources: false is set on all tests that instantiate a real GcsClient or S3Client, each annotated with a comment explaining the TCP connection pool reason.
• The TOCTOU fix (swamp-club#168) is correctly tested: generation/ETag is captured from the same response that delivers the bytes, not from a subsequent metadata probe.

Security

• No hardcoded credentials or secrets.
• bodyPreview capture in s3_client.ts is capped at 256 bytes — intentional, documented.
• Path traversal is guarded on two surfaces:
  • assertSafePath(cachePath, relativePath) using path.normalize + startsWith for local cache writes.
  • fetchForeignContent rejects paths with .. segments or leading slashes before any local I/O.
• GCS token cache (cachedToken) is a module-level singleton; tests call clearTokenCache() in finally to prevent state leakage between test cases.

Correctness

• Generation-as-string comparison in GCS sync is correct: GCS object generations are int64 values that exceed JS safe integer range; comparing as strings avoids precision loss.
• isRetryableError in gcs_cache_sync.ts adds explicit non-retryable guards for NotFoundError and PreconditionFailedError not present in the S3 version — correct, since these indicate caller-side logic errors rather than transient infrastructure faults.
• preflightCredentials races the token probe against ADC_CHAIN_TIMEOUT_MS (5 s) with abort signal propagation — correctly cleans up on timeout.

Suggestions (non-blocking)

1. Undocumented behavioral divergence: markDirty() deletion semantics between GCS and S3

In gcs_cache_sync.ts, a bare markDirty() call (no relPath) sets bulkInvalidated = true, which in the full-walk push path triggers orphan deletion of local files not present in the remote index. In s3_cache_sync.ts, orphan deletion in the full walk is only triggered by dirtyPathsOverflowed (the 200-entry cap being exceeded via scoped markDirty({ relPath }) calls); a bare markDirty() is explicitly documented as "a modification signal, not a deletion signal."

This divergence is intentional — it is explicitly validated by the test "pushChanged: bulk walk deletes orphaned index entries after markDirty()" in gcs_cache_sync_test.ts. However, the public DatastoreSyncService interface carries no documentation of this per-provider difference, which creates a footgun for callers who assume cross-provider parity. Consider adding a JSDoc comment on markDirty() in GcsCacheSyncService noting that bulk invalidation also schedules orphan deletion.

2. Minor verbosity in formatBatchFailure preposition logic (gcs_cache_sync.ts)

// GCS (current)
const preposition = op === "pull" ? "from" : op === "delete" ? "from" : "to";

// S3 (equivalent, simpler)
const preposition = op === "push" ? "to" : "from";

Functionally identical. Low priority, but worth aligning for consistency between providers.

## Code Review **Result: APPROVED** — no blocking issues found. --- ### CLAUDE.md Compliance - No `any` types in hand-written code. Test mocks use `as unknown as GcsClient & {...}` for controlled upcasting in test infrastructure, which is acceptable. - Named exports only, no default exports across all changed files. - No semver ranges in npm imports; `@aws-sdk/client-s3@3.1046.0` is pinned to an exact version. - `deno.lock` is present and committed for the GCS extension. - No files under `model/` were modified. ### Testing Rules - All integration tests use `Deno.serve({ port: 0 })` (GCS/S3 client tests) or in-memory mock clients (`createMockGcsClient`) — no live cloud services. - S3 tests restore `AWS_ACCESS_KEY_ID` / `AWS_SECRET_ACCESS_KEY` in `finally` blocks. GCS tests use an emulator-mode ADC bypass that requires no env var teardown. - `sanitizeResources: false` is set on all tests that instantiate a real `GcsClient` or `S3Client`, each annotated with a comment explaining the TCP connection pool reason. - The TOCTOU fix (swamp-club#168) is correctly tested: generation/ETag is captured from the same response that delivers the bytes, not from a subsequent metadata probe. ### Security - No hardcoded credentials or secrets. - `bodyPreview` capture in `s3_client.ts` is capped at 256 bytes — intentional, documented. - Path traversal is guarded on two surfaces: - `assertSafePath(cachePath, relativePath)` using `path.normalize` + `startsWith` for local cache writes. - `fetchForeignContent` rejects paths with `..` segments or leading slashes before any local I/O. - GCS token cache (`cachedToken`) is a module-level singleton; tests call `clearTokenCache()` in `finally` to prevent state leakage between test cases. ### Correctness - Generation-as-string comparison in GCS sync is correct: GCS object generations are int64 values that exceed JS safe integer range; comparing as strings avoids precision loss. - `isRetryableError` in `gcs_cache_sync.ts` adds explicit non-retryable guards for `NotFoundError` and `PreconditionFailedError` not present in the S3 version — correct, since these indicate caller-side logic errors rather than transient infrastructure faults. - `preflightCredentials` races the token probe against `ADC_CHAIN_TIMEOUT_MS` (5 s) with abort signal propagation — correctly cleans up on timeout. ### Suggestions (non-blocking) **1. Undocumented behavioral divergence: `markDirty()` deletion semantics between GCS and S3** In `gcs_cache_sync.ts`, a bare `markDirty()` call (no `relPath`) sets `bulkInvalidated = true`, which in the full-walk push path triggers orphan deletion of local files not present in the remote index. In `s3_cache_sync.ts`, orphan deletion in the full walk is only triggered by `dirtyPathsOverflowed` (the 200-entry cap being exceeded via scoped `markDirty({ relPath })` calls); a bare `markDirty()` is explicitly documented as "a modification signal, not a deletion signal." This divergence is intentional — it is explicitly validated by the test `"pushChanged: bulk walk deletes orphaned index entries after markDirty()"` in `gcs_cache_sync_test.ts`. However, the public `DatastoreSyncService` interface carries no documentation of this per-provider difference, which creates a footgun for callers who assume cross-provider parity. Consider adding a JSDoc comment on `markDirty()` in `GcsCacheSyncService` noting that bulk invalidation also schedules orphan deletion. **2. Minor verbosity in `formatBatchFailure` preposition logic (gcs_cache_sync.ts)** ```typescript // GCS (current) const preposition = op === "pull" ? "from" : op === "delete" ? "from" : "to"; // S3 (equivalent, simpler) const preposition = op === "push" ? "to" : "from"; ``` Functionally identical. Low priority, but worth aligning for consistency between providers.

stack72 commented 2026-06-24 22:06:37 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

1. S3 IMDS disable in constructor breaks EC2 instance-profile credential resolution
   • File: datastore/s3/extensions/datastores/_lib/s3_client.ts:342-349
   • What's wrong: The constructor unconditionally sets AWS_EC2_METADATA_DISABLED=true when AWS_CONTAINER_CREDENTIALS_RELATIVE_URI and AWS_CONTAINER_CREDENTIALS_FULL_URI are absent. This correctly preserves ECS/EKS container credential sources but silently disables IMDS (Instance Metadata Service) on bare EC2 instances, where the instance profile attached via IAM role is the sole credential source.
   • Breaking example: Deploy swamp on an EC2 instance with an attached IAM role (no explicit AWS_PROFILE, AWS_ACCESS_KEY_ID, or container credential env vars). The S3Client constructor disables IMDS. preflightCredentials then fails with CredentialsProviderError, which classifyAwsCredentialError maps to "session-expired", producing the hint: "Datastore session expired: your AWS profile's SSO session is no longer valid. Run 'aws sso login' to refresh, then retry." The user's SSO session is fine — the code disabled their actual credential source and then blamed the wrong thing.
   • Suggested fix: Guard the IMDS disable behind a more specific heuristic. For example, only disable when a local credential source is configured (env vars, profile, SSO), or add a swamp-specific env var (SWAMP_DISABLE_IMDS=true) so the optimization is opt-in. Alternatively, accept the 1s IMDS timeout on local dev as the cost of not breaking EC2 deployments — the new PREFLIGHT_TIMEOUT_MS (3s) already provides a fast-failure path, and the IMDS 1s timeout fits within that budget.

Medium

2. GCS preflightCredentials: abort signal listener may silently miss an abort fired between the early check and addEventListener
   • File: datastore/gcs/extensions/datastores/_lib/gcs_client.ts:731-785
   • What's wrong: Line 731 checks signal?.aborted and throws early. Line 771 registers an addEventListener("abort", ...) listener. If the signal aborts in the microtask gap between these two points, the abort event has already dispatched — the listener never fires (per EventTarget spec, addEventListener does not replay past events). The abort racer promise never settles, and the Promise.race must wait for the probe or the 5s timeout to settle it.
   • Breaking example: Caller aborts its AbortController at T=0.5s after preflightCredentials begins. The early check passed (at T=0, not yet aborted). The listener was attached after the event fired. The token fetch continues. The race settles at T=5s (timeout) instead of T=0.5s (abort). The user waits 4.5 extra seconds.
   • Impact: Extremely rare micro-race window; bounded by the 5s timeout. Not a data-loss or crash scenario.
   • Suggested fix: After attaching the listener, re-check signal.aborted and manually reject if true. Or replace the listener-based approach with AbortSignal.any([signal, ...]) composed into the race structure.

Low

3. Background SDK/token work continues after preflight timeout
   • File: datastore/s3/extensions/datastores/_lib/s3_client.ts:548-581 and datastore/gcs/extensions/datastores/_lib/gcs_client.ts:729-792
   • What's wrong: Both S3 and GCS preflightCredentials use Promise.race with a timeout. When the timeout wins, the losing probe promise continues running in the background. For S3, the headBucket SDK call continues until the SDK's own requestTimeout (default 30s). For GCS, the getAccessToken chain continues until per-call timeouts resolve. The probe.catch(() => {}) swallows the eventual rejection.
   • Impact: Harmless in practice — the background work is bounded by per-request timeouts, produces no user-visible side effects, and token caching from a late-succeeding GCS probe actually provides graceful recovery on retry.

Verdict

FAIL — the S3 IMDS disable (finding #1) silently breaks a common AWS deployment pattern (EC2 instance profiles) and produces a misleading error message that points users at the wrong remediation. The GCS preflight, scoped-walk deletion fix, lazyPullActive guard, and error discrimination improvements are all well-engineered. The test coverage is thorough. Fix finding #1 and this is ready to merge.

## Adversarial Review ### Critical / High 1. **S3 IMDS disable in constructor breaks EC2 instance-profile credential resolution** - **File**: `datastore/s3/extensions/datastores/_lib/s3_client.ts:342-349` - **What's wrong**: The constructor unconditionally sets `AWS_EC2_METADATA_DISABLED=true` when `AWS_CONTAINER_CREDENTIALS_RELATIVE_URI` and `AWS_CONTAINER_CREDENTIALS_FULL_URI` are absent. This correctly preserves ECS/EKS container credential sources but silently disables IMDS (Instance Metadata Service) on bare EC2 instances, where the instance profile attached via IAM role is the *sole* credential source. - **Breaking example**: Deploy swamp on an EC2 instance with an attached IAM role (no explicit `AWS_PROFILE`, `AWS_ACCESS_KEY_ID`, or container credential env vars). The S3Client constructor disables IMDS. `preflightCredentials` then fails with `CredentialsProviderError`, which `classifyAwsCredentialError` maps to `"session-expired"`, producing the hint: *"Datastore session expired: your AWS profile's SSO session is no longer valid. Run 'aws sso login' to refresh, then retry."* The user's SSO session is fine — the code disabled their actual credential source and then blamed the wrong thing. - **Suggested fix**: Guard the IMDS disable behind a more specific heuristic. For example, only disable when a local credential source *is* configured (env vars, profile, SSO), or add a swamp-specific env var (`SWAMP_DISABLE_IMDS=true`) so the optimization is opt-in. Alternatively, accept the 1s IMDS timeout on local dev as the cost of not breaking EC2 deployments — the new `PREFLIGHT_TIMEOUT_MS` (3s) already provides a fast-failure path, and the IMDS 1s timeout fits within that budget. ### Medium 2. **GCS `preflightCredentials`: abort signal listener may silently miss an abort fired between the early check and `addEventListener`** - **File**: `datastore/gcs/extensions/datastores/_lib/gcs_client.ts:731-785` - **What's wrong**: Line 731 checks `signal?.aborted` and throws early. Line 771 registers an `addEventListener("abort", ...)` listener. If the signal aborts in the microtask gap between these two points, the abort event has already dispatched — the listener never fires (per EventTarget spec, `addEventListener` does not replay past events). The abort racer promise never settles, and the `Promise.race` must wait for the `probe` or the 5s timeout to settle it. - **Breaking example**: Caller aborts its AbortController at T=0.5s after `preflightCredentials` begins. The early check passed (at T=0, not yet aborted). The listener was attached after the event fired. The token fetch continues. The race settles at T=5s (timeout) instead of T=0.5s (abort). The user waits 4.5 extra seconds. - **Impact**: Extremely rare micro-race window; bounded by the 5s timeout. Not a data-loss or crash scenario. - **Suggested fix**: After attaching the listener, re-check `signal.aborted` and manually reject if true. Or replace the listener-based approach with `AbortSignal.any([signal, ...])` composed into the race structure. ### Low 3. **Background SDK/token work continues after preflight timeout** - **File**: `datastore/s3/extensions/datastores/_lib/s3_client.ts:548-581` and `datastore/gcs/extensions/datastores/_lib/gcs_client.ts:729-792` - **What's wrong**: Both S3 and GCS `preflightCredentials` use `Promise.race` with a timeout. When the timeout wins, the losing `probe` promise continues running in the background. For S3, the `headBucket` SDK call continues until the SDK's own `requestTimeout` (default 30s). For GCS, the `getAccessToken` chain continues until per-call timeouts resolve. The `probe.catch(() => {})` swallows the eventual rejection. - **Impact**: Harmless in practice — the background work is bounded by per-request timeouts, produces no user-visible side effects, and token caching from a late-succeeding GCS probe actually provides graceful recovery on retry. ### Verdict **FAIL** — the S3 IMDS disable (finding #1) silently breaks a common AWS deployment pattern (EC2 instance profiles) and produces a misleading error message that points users at the wrong remediation. The GCS preflight, scoped-walk deletion fix, `lazyPullActive` guard, and error discrimination improvements are all well-engineered. The test coverage is thorough. Fix finding #1 and this is ready to merge.

stack72 force-pushed feat/credential-preflight-timeouts from 269f2e4aab to de74f50205 2026-06-24 22:11:23 +00:00 Compare

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

Author

Owner

Copy link

Code Review

Blocking Issues

None identified.

Suggestions

1. GCS bulkInvalidated vs S3 dirtyPathsOverflowed for orphan scanning (gcs_cache_sync.ts)

   In the GCS implementation, orphan deletion in the bulk walk path is gated on this.bulkInvalidated. The S3 equivalent gates on this.dirtyPathsOverflowed — a distinct condition that fires only when the dirty-path cap overflows (not on every bulk invalidation). If a GCS pull is triggered by an explicit bulk invalidation (e.g. first-time pull, explicit invalidation signal) but the dirty-path cap was not exceeded, the GCS path will still scan for orphans while S3 would skip that scan. This asymmetry is likely intentional (GCS design treats all bulk invalidations as orphan-scan-eligible), but it would be worth a brief comment at the scan site explaining why bulkInvalidated is the right sentinel here rather than a cap-overflow flag. As written, a future reader may assume this is a bug.

2. Module-level cachedToken singleton in gcs_client.ts

   The cachedToken variable is module-level state shared across all GcsClient instances in the process. This is reasonable for ADC (which is process-scoped), and clearTokenCache() is exported for test hygiene. However, if swamp ever hosts multiple GCS datastores configured with different service-account credential files (GOOGLE_APPLICATION_CREDENTIALS), they will contend on the same cache and the last writer wins. A comment noting this design assumption ("ADC credentials are process-global; multi-account configurations are not supported") would prevent a future multi-tenant footgun.

3. preflightCredentials timer leak on success path (gcs_client.ts)

   Inside preflightCredentials, the Promise.race approach spawns a setTimeout-backed rejection that continues to live after the credential fetch succeeds. The timer is not cleared on the success path, which will cause a sanitizer warning in tests unless sanitizeOps: false is set. The existing tests may already suppress this with sanitizeResources: false (which does not cover op sanitization), but it's worth verifying or restructuring to clearTimeout on success (pattern: capture the timer id, wrap in try/finally). See the analogous fix applied to the GCS preflight timeout test (fix: suppress leaked-timer sanitizer).

4. S3 preflightDone not reset on credential rotation (s3_cache_sync.ts)

   ensurePreflight sets preflightDone = true permanently. If the underlying S3 client's credentials are rotated (e.g. the vault re-issues credentials mid-session), the preflight guard will not re-run. This matches the GCS behaviour and is presumably acceptable — noting it here since the GCS side has the same characteristic. A comment "preflight is one-shot per service instance; credential rotation requires a new instance" would make the intent explicit.

## Code Review ### Blocking Issues None identified. ### Suggestions 1. **GCS `bulkInvalidated` vs S3 `dirtyPathsOverflowed` for orphan scanning** (`gcs_cache_sync.ts`) In the GCS implementation, orphan deletion in the bulk walk path is gated on `this.bulkInvalidated`. The S3 equivalent gates on `this.dirtyPathsOverflowed` — a distinct condition that fires only when the dirty-path cap overflows (not on every bulk invalidation). If a GCS pull is triggered by an explicit bulk invalidation (e.g. first-time pull, explicit invalidation signal) but the dirty-path cap was *not* exceeded, the GCS path will still scan for orphans while S3 would skip that scan. This asymmetry is likely intentional (GCS design treats all bulk invalidations as orphan-scan-eligible), but it would be worth a brief comment at the scan site explaining why `bulkInvalidated` is the right sentinel here rather than a cap-overflow flag. As written, a future reader may assume this is a bug. 2. **Module-level `cachedToken` singleton in `gcs_client.ts`** The `cachedToken` variable is module-level state shared across all `GcsClient` instances in the process. This is reasonable for ADC (which is process-scoped), and `clearTokenCache()` is exported for test hygiene. However, if swamp ever hosts multiple GCS datastores configured with different service-account credential files (`GOOGLE_APPLICATION_CREDENTIALS`), they will contend on the same cache and the last writer wins. A comment noting this design assumption ("ADC credentials are process-global; multi-account configurations are not supported") would prevent a future multi-tenant footgun. 3. **`preflightCredentials` timer leak on success path** (`gcs_client.ts`) Inside `preflightCredentials`, the `Promise.race` approach spawns a `setTimeout`-backed rejection that continues to live after the credential fetch succeeds. The timer is not cleared on the success path, which will cause a sanitizer warning in tests unless `sanitizeOps: false` is set. The existing tests may already suppress this with `sanitizeResources: false` (which does not cover op sanitization), but it's worth verifying or restructuring to `clearTimeout` on success (pattern: capture the timer id, wrap in try/finally). See the analogous fix applied to the GCS preflight timeout test (`fix: suppress leaked-timer sanitizer`). 4. **S3 `preflightDone` not reset on credential rotation** (`s3_cache_sync.ts`) `ensurePreflight` sets `preflightDone = true` permanently. If the underlying S3 client's credentials are rotated (e.g. the vault re-issues credentials mid-session), the preflight guard will not re-run. This matches the GCS behaviour and is presumably acceptable — noting it here since the GCS side has the same characteristic. A comment "preflight is one-shot per service instance; credential rotation requires a new instance" would make the intent explicit.

stack72 commented 2026-06-24 22:24:02 +00:00

Author

Owner

Copy link

Adversarial Code Review — Credential Preflight & Timeout Guards

Scope: 9 files, +607/-25 lines across GCS and S3 datastore extensions.
Commits reviewed: 4a89e42d1..de74f5020 (5 commits)

---

Summary

This PR adds credential preflight checks (fail-fast before first I/O), timeout guards on token acquisition (GCS ADC chain), IMDS disabling for non-container AWS environments, improved error discrimination in the GCS scoped-walk catch block, per-file deletion detection inside surviving directories, and a lazy-hydration guard to prevent deleting un-pulled files. The changes are defensive hardening with thorough test coverage.

Verdict: PASS — no CRITICAL or HIGH findings.

---

Findings

MEDIUM-1: S3 preflightCredentials omits pre-abort check (asymmetry with GCS)

File: datastore/s3/extensions/datastores/_lib/s3_client.ts:548

The GCS implementation checks signal?.aborted at the top of preflightCredentials and throws immediately with a descriptive GcsOperationError (lines 731-739 in gcs_client.ts). The S3 implementation does not — it forwards signal into headBucket(), which forwards it through the SDK's send(), which will eventually throw an AbortError. The behavior is functionally correct (an already-aborted signal will cause the SDK call to fail), but the error path differs: the GCS path produces a descriptive "GCS credential preflight aborted" message, while S3 produces a generic SDK abort error that gets wrapped through wrapError. This is a consistency gap, not a bug.

Severity: MEDIUM — No data loss or security risk. The signal is honored in both paths. But the diagnostic quality on pre-aborted calls differs, which could confuse debugging.

Recommendation: Add a 3-line guard at the top of S3Client.preflightCredentials:

if (signal?.aborted) {
  throw new S3OperationError("S3 credential preflight aborted", { name: "AbortError", cause: signal.reason, httpStatusCode: undefined, code: undefined, requestId: undefined, bodyPreview: undefined });
}

---

MEDIUM-2: AWS_EC2_METADATA_DISABLED is a process-wide side effect in a constructor

File: datastore/s3/extensions/datastores/_lib/s3_client.ts:342-348

The constructor sets AWS_EC2_METADATA_DISABLED=true when no container credential env vars are detected. The inline comment correctly acknowledges this is process-wide and asserts "swamp's datastore is the sole AWS SDK consumer in the process." This is a reasonable assertion today.

Severity: MEDIUM — The env var is set unconditionally (not restored on teardown, not scoped to the client instance). If a second AWS SDK consumer is ever introduced in the same process that legitimately needs IMDS (e.g., a vault provider running on EC2), this will silently break it. The "only if not already set" guard (!Deno.env.get("AWS_EC2_METADATA_DISABLED")) is a good defensive check but doesn't address the multi-consumer scenario.

Recommendation: Acceptable as-is given the documented constraint. If multi-consumer becomes realistic, move to SDK-level configuration (credentials: fromNodeProviderChain({ ec2InstanceMetadata: false })) instead of environment mutation.

---

LOW-1: GCS preflightCredentials timer creates a clearTimeout-able timer without signal cleanup

File: datastore/gcs/extensions/datastores/_lib/gcs_client.ts:745-791

When signal is provided, the signal's abort listener (line 771) is registered with { once: true } — correct. However, if probe wins the Promise.race, the abort listener remains registered on the signal until the signal itself is GC'd or aborted. This is harmless for short-lived signals but could accumulate listeners if the same long-lived signal is passed to many sequential preflight calls.

Severity: LOW — Preflight is called once per sync service lifecycle (preflightDone flag gates it), so accumulation is not realistic.

---

LOW-2: S3 preflightCredentials lacks a signal-forwarded abort racer (asymmetry with GCS)

File: datastore/s3/extensions/datastores/_lib/s3_client.ts:548-581

The GCS preflightCredentials constructs a 3-way race: [probe, timeout, signalAbort]. The S3 version races only [probe, timeout], relying on signal forwarding through headBucket → run → send. This is correct because the SDK does honor the signal. But if headBucket hangs at the TCP layer (past the SDK's requestTimeout), the GCS version would abort via the signal racer while S3 would wait for the PREFLIGHT_TIMEOUT_MS timer. Since PREFLIGHT_TIMEOUT_MS (3s) is shorter than defaultRequestTimeoutMs (30s), this matters — the signal path is the faster escape.

Severity: LOW — The PREFLIGHT_TIMEOUT_MS timer provides a hard upper bound of 3s regardless, so worst-case responsiveness is bounded. The difference only manifests if the caller aborts between "now" and "3s from now", which is an edge case.

---

LOW-3: No test for pre-aborted signal in S3 preflightCredentials

File: datastore/s3/extensions/datastores/_lib/s3_client_test.ts

GCS tests include a test for calling preflightCredentials with an already-aborted signal. The S3 test suite does not have an equivalent. This is consistent with the code asymmetry noted in MEDIUM-1 but reduces coverage of the signal-forwarding path.

Severity: LOW — Covered implicitly by the SDK's own abort handling, but an explicit test would close the gap.

---

Positive Observations

1. probe.catch(() => {}) pattern (both gcs_client.ts:743, s3_client.ts:550): Correctly prevents unhandled promise rejection when the timeout or signal wins the race and the probe rejects later. This is a common source of bugs in Promise.race patterns — handled properly here.

2. Error discrimination in scoped walk (gcs_cache_sync.ts, commit ea1915f89): The old bare catch treated any stat error as "file absent → schedule deletion." Permission errors, NFS timeouts, or I/O failures would have caused silent remote deletions of files that still exist. The fix correctly narrows to Deno.errors.NotFound only and skips other errors for retry. This is a meaningful data-safety improvement.

3. Lazy hydration guard (gcs_cache_sync.ts, commit ea1915f89): if (!this.lazyPullActive && this.index) prevents the scoped walk from scheduling deletions for files that are absent because they haven't been hydrated yet (lazy pull). Without this, a push after a lazy pull would delete un-hydrated remote files. Matches the S3 implementation.

4. Per-file deletion detection (gcs_cache_sync.ts, commit de74f5020): The localFilesInDir set correctly detects files that existed in the index under a dirty directory but are absent on disk. The previous code only detected whole-directory deletions — individual file deletions inside surviving directories were missed. Good use of the index-scanning pattern already established for the non-directory catch path.

5. composeTokenSignal in GCS (gcs_client.ts): Cleanly separates per-call timeout (2s TOKEN_FETCH_TIMEOUT_MS) from chain-level timeout (5s ADC_CHAIN_TIMEOUT_MS) and external abort signal. Uses AbortSignal.any() for composition — correct and idiomatic.

6. Token error wrapping in headers() (gcs_client.ts): Converts raw TimeoutError/AbortError from the token path into GcsOperationError so the retry classifier in the sync layer can handle them uniformly. Previously these would have propagated as untyped errors.

7. Test coverage is thorough: preflight timeout, preflight success, pre-aborted signal (GCS), mock client conformance updates, scoped-walk deletion detection, lazy-pull guard, and sanitizer annotations are all present.

## Adversarial Code Review — Credential Preflight & Timeout Guards **Scope:** 9 files, +607/-25 lines across GCS and S3 datastore extensions. **Commits reviewed:** `4a89e42d1..de74f5020` (5 commits) --- ### Summary This PR adds credential preflight checks (fail-fast before first I/O), timeout guards on token acquisition (GCS ADC chain), IMDS disabling for non-container AWS environments, improved error discrimination in the GCS scoped-walk catch block, per-file deletion detection inside surviving directories, and a lazy-hydration guard to prevent deleting un-pulled files. The changes are defensive hardening with thorough test coverage. **Verdict: PASS — no CRITICAL or HIGH findings.** --- ### Findings #### MEDIUM-1: S3 `preflightCredentials` omits pre-abort check (asymmetry with GCS) **File:** `datastore/s3/extensions/datastores/_lib/s3_client.ts:548` The GCS implementation checks `signal?.aborted` at the top of `preflightCredentials` and throws immediately with a descriptive `GcsOperationError` (lines 731-739 in `gcs_client.ts`). The S3 implementation does not — it forwards `signal` into `headBucket()`, which forwards it through the SDK's `send()`, which will eventually throw an `AbortError`. The behavior is functionally correct (an already-aborted signal will cause the SDK call to fail), but the error path differs: the GCS path produces a descriptive `"GCS credential preflight aborted"` message, while S3 produces a generic SDK abort error that gets wrapped through `wrapError`. This is a consistency gap, not a bug. **Severity: MEDIUM** — No data loss or security risk. The signal is honored in both paths. But the diagnostic quality on pre-aborted calls differs, which could confuse debugging. **Recommendation:** Add a 3-line guard at the top of `S3Client.preflightCredentials`: ```typescript if (signal?.aborted) { throw new S3OperationError("S3 credential preflight aborted", { name: "AbortError", cause: signal.reason, httpStatusCode: undefined, code: undefined, requestId: undefined, bodyPreview: undefined }); } ``` --- #### MEDIUM-2: `AWS_EC2_METADATA_DISABLED` is a process-wide side effect in a constructor **File:** `datastore/s3/extensions/datastores/_lib/s3_client.ts:342-348` The constructor sets `AWS_EC2_METADATA_DISABLED=true` when no container credential env vars are detected. The inline comment correctly acknowledges this is process-wide and asserts "swamp's datastore is the sole AWS SDK consumer in the process." This is a reasonable assertion today. **Severity: MEDIUM** — The env var is set unconditionally (not restored on teardown, not scoped to the client instance). If a second AWS SDK consumer is ever introduced in the same process that legitimately needs IMDS (e.g., a vault provider running on EC2), this will silently break it. The "only if not already set" guard (`!Deno.env.get("AWS_EC2_METADATA_DISABLED")`) is a good defensive check but doesn't address the multi-consumer scenario. **Recommendation:** Acceptable as-is given the documented constraint. If multi-consumer becomes realistic, move to SDK-level configuration (`credentials: fromNodeProviderChain({ ec2InstanceMetadata: false })`) instead of environment mutation. --- #### LOW-1: GCS `preflightCredentials` timer creates a `clearTimeout`-able timer without signal cleanup **File:** `datastore/gcs/extensions/datastores/_lib/gcs_client.ts:745-791` When `signal` is provided, the signal's abort listener (line 771) is registered with `{ once: true }` — correct. However, if `probe` wins the `Promise.race`, the abort listener remains registered on the signal until the signal itself is GC'd or aborted. This is harmless for short-lived signals but could accumulate listeners if the same long-lived signal is passed to many sequential preflight calls. **Severity: LOW** — Preflight is called once per sync service lifecycle (`preflightDone` flag gates it), so accumulation is not realistic. --- #### LOW-2: S3 `preflightCredentials` lacks a signal-forwarded abort racer (asymmetry with GCS) **File:** `datastore/s3/extensions/datastores/_lib/s3_client.ts:548-581` The GCS `preflightCredentials` constructs a 3-way race: `[probe, timeout, signalAbort]`. The S3 version races only `[probe, timeout]`, relying on signal forwarding through `headBucket → run → send`. This is correct because the SDK *does* honor the signal. But if `headBucket` hangs at the TCP layer (past the SDK's `requestTimeout`), the GCS version would abort via the signal racer while S3 would wait for the `PREFLIGHT_TIMEOUT_MS` timer. Since `PREFLIGHT_TIMEOUT_MS` (3s) is shorter than `defaultRequestTimeoutMs` (30s), this matters — the signal path is the faster escape. **Severity: LOW** — The PREFLIGHT_TIMEOUT_MS timer provides a hard upper bound of 3s regardless, so worst-case responsiveness is bounded. The difference only manifests if the caller aborts between "now" and "3s from now", which is an edge case. --- #### LOW-3: No test for pre-aborted signal in S3 `preflightCredentials` **File:** `datastore/s3/extensions/datastores/_lib/s3_client_test.ts` GCS tests include a test for calling `preflightCredentials` with an already-aborted signal. The S3 test suite does not have an equivalent. This is consistent with the code asymmetry noted in MEDIUM-1 but reduces coverage of the signal-forwarding path. **Severity: LOW** — Covered implicitly by the SDK's own abort handling, but an explicit test would close the gap. --- ### Positive Observations 1. **`probe.catch(() => {})` pattern** (both `gcs_client.ts:743`, `s3_client.ts:550`): Correctly prevents unhandled promise rejection when the timeout or signal wins the race and the probe rejects later. This is a common source of bugs in Promise.race patterns — handled properly here. 2. **Error discrimination in scoped walk** (`gcs_cache_sync.ts`, commit `ea1915f89`): The old bare `catch` treated *any* stat error as "file absent → schedule deletion." Permission errors, NFS timeouts, or I/O failures would have caused silent remote deletions of files that still exist. The fix correctly narrows to `Deno.errors.NotFound` only and skips other errors for retry. This is a meaningful data-safety improvement. 3. **Lazy hydration guard** (`gcs_cache_sync.ts`, commit `ea1915f89`): `if (!this.lazyPullActive && this.index)` prevents the scoped walk from scheduling deletions for files that are absent because they haven't been hydrated yet (lazy pull). Without this, a push after a lazy pull would delete un-hydrated remote files. Matches the S3 implementation. 4. **Per-file deletion detection** (`gcs_cache_sync.ts`, commit `de74f5020`): The `localFilesInDir` set correctly detects files that existed in the index under a dirty directory but are absent on disk. The previous code only detected whole-directory deletions — individual file deletions inside surviving directories were missed. Good use of the index-scanning pattern already established for the non-directory catch path. 5. **`composeTokenSignal` in GCS** (`gcs_client.ts`): Cleanly separates per-call timeout (2s `TOKEN_FETCH_TIMEOUT_MS`) from chain-level timeout (5s `ADC_CHAIN_TIMEOUT_MS`) and external abort signal. Uses `AbortSignal.any()` for composition — correct and idiomatic. 6. **Token error wrapping in `headers()`** (`gcs_client.ts`): Converts raw `TimeoutError`/`AbortError` from the token path into `GcsOperationError` so the retry classifier in the sync layer can handle them uniformly. Previously these would have propagated as untyped errors. 7. **Test coverage** is thorough: preflight timeout, preflight success, pre-aborted signal (GCS), mock client conformance updates, scoped-walk deletion detection, lazy-pull guard, and sanitizer annotations are all present.

stack72 merged commit faf4c76f71 into main 2026-06-24 22:45:56 +00:00

stack72 deleted branch feat/credential-preflight-timeouts 2026-06-24 22:45:56 +00:00

stack72 referenced this pull request from a commit 2026-06-24 22:45:58 +00:00

feat(datastores): add credential preflight and timeout guards (#72)

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

No description provided.