fix(gcs-datastore): add read-before-write conflict detection in registerNamespace (#567) #37

Merged

stack72 merged 4 commits from fix/567-gcs-namespace-conflict-detection into main 2026-06-05 23:34:54 +00:00

Conversation 6 Commits 4 Files changed 4 +263 -15

stack72 commented 2026-06-05 22:13:32 +00:00

Owner

Copy link

Summary

• Add explicit read-before-write conflict detection to GCS registerNamespace so it does not rely solely on ifGenerationMatch=0 precondition enforcement
• Fix pre-existing TS2322 type error in gcs_lock.ts (setInterval returns Timeout, not number)
• Bump manifest version to 2026.06.05.1

Root cause

registerNamespace used putObjectConditional (ifGenerationMatch=0) as its sole mechanism for detecting existing namespace manifests. When a GCS-compatible backend doesn't enforce this precondition (e.g., some versions of fake-gcs-server), the conditional write silently succeeds, if (created !== null) return short-circuits, and the repoId comparison is never reached. The manifest is overwritten with no conflict error.

Fix

Read the existing manifest before attempting to write. If it exists and belongs to a different repo, throw immediately. Keep putObjectConditional as a secondary atomicity guard for concurrent-write races, with a re-read fallback if the conditional write fails.

What changed

File	Change
datastore/gcs/extensions/datastores/gcs.ts	Import NotFoundError; rewrite registerNamespace to read-before-write with putObjectConditional as secondary atomicity guard
datastore/gcs/extensions/datastores/gcs_test.ts	Add mock server variant that ignores preconditions; 4 new tests covering precondition-bypass and prefix scenarios
datastore/gcs/extensions/datastores/_lib/gcs_lock.ts	Fix TS2322: heartbeatId type from number to ReturnType<typeof setInterval>
datastore/gcs/manifest.yaml	Version bump 2026.06.03.2 → 2026.06.05.1

Test plan

• 4 new tests: conflict detection with/without precondition enforcement, with/without prefix
• All 15 GCS datastore tests pass (11 existing + 4 new)
• deno check passes (including previously broken gcs_lock.ts)
• deno lint, deno fmt --check, deno install --frozen pass

Closes #567

## Summary - Add explicit read-before-write conflict detection to GCS `registerNamespace` so it does not rely solely on `ifGenerationMatch=0` precondition enforcement - Fix pre-existing TS2322 type error in `gcs_lock.ts` (`setInterval` returns `Timeout`, not `number`) - Bump manifest version to `2026.06.05.1` ## Root cause `registerNamespace` used `putObjectConditional` (`ifGenerationMatch=0`) as its sole mechanism for detecting existing namespace manifests. When a GCS-compatible backend doesn't enforce this precondition (e.g., some versions of fake-gcs-server), the conditional write silently succeeds, `if (created !== null) return` short-circuits, and the repoId comparison is never reached. The manifest is overwritten with no conflict error. ## Fix Read the existing manifest **before** attempting to write. If it exists and belongs to a different repo, throw immediately. Keep `putObjectConditional` as a secondary atomicity guard for concurrent-write races, with a re-read fallback if the conditional write fails. ## What changed | File | Change | |------|--------| | `datastore/gcs/extensions/datastores/gcs.ts` | Import `NotFoundError`; rewrite `registerNamespace` to read-before-write with `putObjectConditional` as secondary atomicity guard | | `datastore/gcs/extensions/datastores/gcs_test.ts` | Add mock server variant that ignores preconditions; 4 new tests covering precondition-bypass and prefix scenarios | | `datastore/gcs/extensions/datastores/_lib/gcs_lock.ts` | Fix TS2322: `heartbeatId` type from `number` to `ReturnType<typeof setInterval>` | | `datastore/gcs/manifest.yaml` | Version bump `2026.06.03.2` → `2026.06.05.1` | ## Test plan - [x] 4 new tests: conflict detection with/without precondition enforcement, with/without prefix - [x] All 15 GCS datastore tests pass (11 existing + 4 new) - [x] `deno check` passes (including previously broken `gcs_lock.ts`) - [x] `deno lint`, `deno fmt --check`, `deno install --frozen` pass Closes #567

stack72 added 1 commit 2026-06-05 22:13:32 +00:00

fix(gcs-datastore): add read-before-write conflict detection in registerNamespace (#567) ... 8f18d53ed9

registerNamespace relied solely on putObjectConditional (ifGenerationMatch=0)
to detect existing namespace manifests. If the backend does not enforce this
precondition, the conditional write silently succeeds and the conflict check
is never reached. Add an explicit read-before-write check that detects
conflicts regardless of backend precondition support.

Also fixes a pre-existing TS2322 type error in gcs_lock.ts (setInterval
returns Timeout, not number).

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

stack72 commented 2026-06-05 22:25:31 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. TOCTOU window on non-compliant backends still allows concurrent silent clobber — gcs.ts:168-201

   The read-before-write check closes the sequential gap on backends that ignore ifGenerationMatch=0 (e.g. fake-gcs-server), but the concurrent gap remains. Two processes racing registerNamespace with different repoId values on such a backend can both observe NotFoundError in the read step (line 169), then both proceed to putObjectConditional (line 200), which succeeds for both (backend ignores the precondition), and both return at line 201 without detecting the conflict — last writer wins silently.

   Example:

   • Process A: getObject("infra/.namespace.json") → 404 → falls through
   • Process B: getObject("infra/.namespace.json") → 404 → falls through
   • Process A: putObjectConditional → succeeds (writes repo-aaa) → returns
   • Process B: putObjectConditional → succeeds (backend ignores precondition, clobbers with repo-bbb) → returns
   • Result: namespace silently owned by repo-bbb; repo-aaa believes it registered successfully.

   This is pre-existing (the code before this PR had the same concurrent-race behavior on non-compliant backends), and the sequential case is the one this PR targets. Flagging as MEDIUM because the new read-before-write check may give a false sense of full protection — a comment noting the concurrent limitation would prevent someone from removing the putObjectConditional fallback later under the assumption the read-before-write alone is sufficient.

   Suggested fix: Add a brief comment near line 193 noting that the read-before-write covers the sequential case, while putObjectConditional remains the primary concurrency guard on compliant backends.

Low

1. Unconditional overwrite in the race-recovery path — gcs.ts:216

   After putObjectConditional returns null (object exists) and the ownership re-check confirms same repoId, the code does an unconditional putObject(key, body). If a third concurrent writer from a different repo wrote between the getObject at line 204 and the putObject at line 216, it would be silently overwritten. This is a very narrow window requiring three concurrent registrations and is pre-existing behavior (unchanged by this PR). Noting for completeness.

2. heartbeatId type change is a no-op in Deno — gcs_lock.ts:93

   ReturnType<typeof setInterval> resolves to number in Deno, so this is functionally identical. It's a correct portability improvement — no issue, just confirming it's inert.

Verdict

PASS — The core change is sound: adding a read-before-write guard in registerNamespace correctly catches the sequential conflict case on GCS-compatible backends that don't enforce generation preconditions. The putObjectConditional fallback remains as the primary atomic guard on compliant backends. The type fix in gcs_lock.ts is correct and trivial. Test coverage is good — the new createMockGcsServerNoPreconditions accurately simulates the target scenario, and tests cover conflict detection, idempotent re-registration, and prefix configurations for both compliant and non-compliant backends. The TOCTOU gap for concurrent writers on non-compliant backends is pre-existing and not worsened by this PR; those backends (fake-gcs-server) are development-only where concurrent registration races are unlikely.

## Adversarial Review ### Medium 1. **TOCTOU window on non-compliant backends still allows concurrent silent clobber** — `gcs.ts:168-201` The read-before-write check closes the *sequential* gap on backends that ignore `ifGenerationMatch=0` (e.g. fake-gcs-server), but the *concurrent* gap remains. Two processes racing `registerNamespace` with different `repoId` values on such a backend can both observe `NotFoundError` in the read step (line 169), then both proceed to `putObjectConditional` (line 200), which succeeds for both (backend ignores the precondition), and both return at line 201 without detecting the conflict — last writer wins silently. **Example:** - Process A: `getObject("infra/.namespace.json")` → 404 → falls through - Process B: `getObject("infra/.namespace.json")` → 404 → falls through - Process A: `putObjectConditional` → succeeds (writes repo-aaa) → returns - Process B: `putObjectConditional` → succeeds (backend ignores precondition, clobbers with repo-bbb) → returns - Result: namespace silently owned by repo-bbb; repo-aaa believes it registered successfully. This is **pre-existing** (the code before this PR had the same concurrent-race behavior on non-compliant backends), and the sequential case is the one this PR targets. Flagging as MEDIUM because the new read-before-write check may give a false sense of full protection — a comment noting the concurrent limitation would prevent someone from removing the `putObjectConditional` fallback later under the assumption the read-before-write alone is sufficient. **Suggested fix:** Add a brief comment near line 193 noting that the read-before-write covers the sequential case, while `putObjectConditional` remains the primary concurrency guard on compliant backends. ### Low 1. **Unconditional overwrite in the race-recovery path** — `gcs.ts:216` After `putObjectConditional` returns `null` (object exists) and the ownership re-check confirms same `repoId`, the code does an unconditional `putObject(key, body)`. If a *third* concurrent writer from a different repo wrote between the `getObject` at line 204 and the `putObject` at line 216, it would be silently overwritten. This is a very narrow window requiring three concurrent registrations and is pre-existing behavior (unchanged by this PR). Noting for completeness. 2. **`heartbeatId` type change is a no-op in Deno** — `gcs_lock.ts:93` `ReturnType<typeof setInterval>` resolves to `number` in Deno, so this is functionally identical. It's a correct portability improvement — no issue, just confirming it's inert. ### Verdict **PASS** — The core change is sound: adding a read-before-write guard in `registerNamespace` correctly catches the sequential conflict case on GCS-compatible backends that don't enforce generation preconditions. The `putObjectConditional` fallback remains as the primary atomic guard on compliant backends. The type fix in `gcs_lock.ts` is correct and trivial. Test coverage is good — the new `createMockGcsServerNoPreconditions` accurately simulates the target scenario, and tests cover conflict detection, idempotent re-registration, and prefix configurations for both compliant and non-compliant backends. The TOCTOU gap for concurrent writers on non-compliant backends is pre-existing and not worsened by this PR; those backends (fake-gcs-server) are development-only where concurrent registration races are unlikely.

stack72 commented 2026-06-05 22:26:27 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

1. gcs_lock.ts:226-232 — forceRelease has a TOCTOU race (unconditional delete)

   readLock() calls gcs.getObject() but discards the returned generation:

   private async readLock(): Promise<LockInfo | null> {
     try {
       const { data } = await this.gcs.getObject(this.lockKey);  // generation dropped
       return decodeLockInfo(data);
     } catch { return null; }
   }
   

   Then forceRelease does an unconditional delete after checking the nonce:

   await this.gcs.deleteObject(this.lockKey);  // no ifGenerationMatch!
   

   Between the read and the delete, the original lock can expire and a new holder can legitimately acquire the same key. The unconditional delete would then remove the new holder's valid lock. Every other delete in this file uses ifGenerationMatch to prevent exactly this. readLock() should also return the generation (or forceRelease should use a separate read that captures it), and the delete should use ifGenerationMatch: generation.

   Note: the existing gcs_lock_test.ts does cover forceRelease, but the mock doesn't exercise the concurrent-acquisition race, so it currently passes despite the bug.

Suggestions

1. gcs.ts:32 — jsr:@std/path@1 uses a major-version range, not an exact pin

   The test file uses jsr:@std/assert@1.0.19 (exact). For consistency with the project's "pin to exact versions" convention, pin @std/path to a full version (e.g., jsr:@std/path@1.0.9). The deno.lock file will resolve it to exact, but the specifier itself should be explicit.

2. gcs.ts:163 — namespace is interpolated into GCS key without sanitization

   const key = `${namespace}/.namespace.json`;
   

   If upstream code ever passes a namespace containing .. or embedded slashes, the resulting key escapes the intended namespace scope. The rest of the implementation is well-hardened; a guard assertion (e.g., rejecting namespaces containing /) or a brief invariant comment would make the assumed constraint explicit.

## Code Review ### Blocking Issues 1. **`gcs_lock.ts:226-232` — `forceRelease` has a TOCTOU race (unconditional delete)** `readLock()` calls `gcs.getObject()` but discards the returned `generation`: ```typescript private async readLock(): Promise<LockInfo | null> { try { const { data } = await this.gcs.getObject(this.lockKey); // generation dropped return decodeLockInfo(data); } catch { return null; } } ``` Then `forceRelease` does an unconditional delete after checking the nonce: ```typescript await this.gcs.deleteObject(this.lockKey); // no ifGenerationMatch! ``` Between the read and the delete, the original lock can expire and a new holder can legitimately acquire the same key. The unconditional delete would then remove the new holder's valid lock. Every other delete in this file uses `ifGenerationMatch` to prevent exactly this. `readLock()` should also return the generation (or `forceRelease` should use a separate read that captures it), and the delete should use `ifGenerationMatch: generation`. Note: the existing `gcs_lock_test.ts` does cover `forceRelease`, but the mock doesn't exercise the concurrent-acquisition race, so it currently passes despite the bug. ### Suggestions 1. **`gcs.ts:32` — `jsr:@std/path@1` uses a major-version range, not an exact pin** The test file uses `jsr:@std/assert@1.0.19` (exact). For consistency with the project's "pin to exact versions" convention, pin `@std/path` to a full version (e.g., `jsr:@std/path@1.0.9`). The `deno.lock` file will resolve it to exact, but the specifier itself should be explicit. 2. **`gcs.ts:163` — `namespace` is interpolated into GCS key without sanitization** ```typescript const key = `${namespace}/.namespace.json`; ``` If upstream code ever passes a namespace containing `..` or embedded slashes, the resulting key escapes the intended namespace scope. The rest of the implementation is well-hardened; a guard assertion (e.g., rejecting namespaces containing `/`) or a brief invariant comment would make the assumed constraint explicit.

stack72 added 1 commit 2026-06-05 22:32:21 +00:00

fix(gcs-datastore): address review findings — forceRelease TOCTOU, namespace guard, pin path ... 01f258098a

- Fix forceRelease TOCTOU: readLockWithGeneration captures the GCS
  generation; forceRelease uses ifGenerationMatch on the delete so a
  concurrent new holder's lock is not removed between the nonce check
  and the delete.
- Add namespace sanitization guard rejecting path separators, "..", and
  null bytes before interpolating into the GCS key.
- Pin jsr:@std/path to exact version 1.1.4.
- Add comment documenting the concurrent TOCTOU limitation on
  non-compliant backends.

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

stack72 commented 2026-06-05 22:41:13 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

1. forceRelease throws on expected race — violates Promise<boolean> contract (gcs_lock.ts:231–235)

   The deleteObject call is not wrapped in try/catch. When ifGenerationMatch fails (HTTP 412 Precondition Failed) because the lock generation changed between the readLockWithGeneration() call and the deleteObject() call, the method throws instead of returning false. This is a new failure mode introduced by this PR: the old unconditional deleteObject could only throw on network errors, whereas an ifGenerationMatch 412 is an expected race outcome that callers cannot distinguish from a real error.

   The method signature is Promise<boolean>, and callers reasonably expect false when "the force release didn't happen because the lock is gone or different." A 412 should map to return false, not throw. Compare release(), which already wraps deleteObject in try/catch with a console.warn fallback. forceRelease should do the same or catch and return false on 412.

   // Fix: wrap the delete and return false on any failure
   async forceRelease(expectedNonce: string): Promise<boolean> {
     const result = await this.readLockWithGeneration();
     if (!result || result.info.nonce !== expectedNonce) return false;
     try {
       await this.gcs.deleteObject(
         this.lockKey,
         result.generation ? { ifGenerationMatch: result.generation } : undefined,
       );
       return true;
     } catch {
       return false;
     }
   }
   

---

Suggestions

1. forceRelease unconditional-delete fallback (gcs_lock.ts:233): When result.generation is undefined, the code passes undefined as options, resulting in an unconditional deleteObject with no generation guard. The nonce check happened on the read, but between the read and the delete another process could have acquired a new lock (different nonce). The unconditional delete would then silently remove that process's lock and return true incorrectly. In practice real GCS always returns a generation, so this is low risk — but logging a warning or returning false in the !result.generation branch would make the code safer by design.

2. createMockGcsServer and createMockGcsServerNoPreconditions code duplication (gcs_test.ts:169–347): The two helpers share almost identical GET and bucket-check handlers; only the PUT handler differs. A single helper accepting a { enforcePreconditions: boolean } option would cut ~80 lines and reduce drift. Not a blocker, but the current duplication makes the mock harder to maintain as the API surface grows.

## Code Review ### Blocking Issues 1. **`forceRelease` throws on expected race — violates `Promise<boolean>` contract** (`gcs_lock.ts:231–235`) The `deleteObject` call is not wrapped in try/catch. When `ifGenerationMatch` fails (HTTP 412 Precondition Failed) because the lock generation changed between the `readLockWithGeneration()` call and the `deleteObject()` call, the method throws instead of returning `false`. This is a new failure mode introduced by this PR: the old unconditional `deleteObject` could only throw on network errors, whereas an `ifGenerationMatch` 412 is an **expected** race outcome that callers cannot distinguish from a real error. The method signature is `Promise<boolean>`, and callers reasonably expect `false` when "the force release didn't happen because the lock is gone or different." A 412 should map to `return false`, not `throw`. Compare `release()`, which already wraps `deleteObject` in try/catch with a `console.warn` fallback. `forceRelease` should do the same or catch and return `false` on 412. ```typescript // Fix: wrap the delete and return false on any failure async forceRelease(expectedNonce: string): Promise<boolean> { const result = await this.readLockWithGeneration(); if (!result || result.info.nonce !== expectedNonce) return false; try { await this.gcs.deleteObject( this.lockKey, result.generation ? { ifGenerationMatch: result.generation } : undefined, ); return true; } catch { return false; } } ``` --- ### Suggestions 1. **`forceRelease` unconditional-delete fallback** (`gcs_lock.ts:233`): When `result.generation` is `undefined`, the code passes `undefined` as options, resulting in an unconditional `deleteObject` with no generation guard. The nonce check happened on the read, but between the read and the delete another process could have acquired a new lock (different nonce). The unconditional delete would then silently remove that process's lock and return `true` incorrectly. In practice real GCS always returns a generation, so this is low risk — but logging a warning or returning `false` in the `!result.generation` branch would make the code safer by design. 2. **`createMockGcsServer` and `createMockGcsServerNoPreconditions` code duplication** (`gcs_test.ts:169–347`): The two helpers share almost identical GET and bucket-check handlers; only the PUT handler differs. A single helper accepting a `{ enforcePreconditions: boolean }` option would cut ~80 lines and reduce drift. Not a blocker, but the current duplication makes the mock harder to maintain as the API surface grows.

stack72 commented 2026-06-05 22:41:59 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. gcs_lock.ts:226-236 — forceRelease throws PreconditionFailedError instead of returning false when the lock holder heartbeats between read and delete.

   The TOCTOU fix correctly uses ifGenerationMatch on the delete, but deleteObject does not catch PreconditionFailedError (it only swallows NotFoundError). If the lock holder calls extend() between readLockWithGeneration() and deleteObject(), the generation changes, the conditional delete throws PreconditionFailedError, and the exception propagates to the caller. The method's return type is Promise<boolean> — an uncaught exception violates this contract.

   Breaking scenario: Admin inspects a stuck lock, gets the nonce, calls forceRelease(nonce). Between the read and delete, the lock holder's heartbeat fires and extends the lock (new generation). deleteObject throws PreconditionFailedError — the admin's tooling crashes with an unhandled exception instead of receiving false.

   Suggested fix:

   async forceRelease(expectedNonce: string): Promise<boolean> {
     const result = await this.readLockWithGeneration();
     if (!result || result.info.nonce !== expectedNonce) {
       return false;
     }
     try {
       await this.gcs.deleteObject(
         this.lockKey,
         result.generation ? { ifGenerationMatch: result.generation } : undefined,
       );
       return true;
     } catch (err) {
       if (err instanceof PreconditionFailedError) return false;
       throw err;
     }
   }
   

   This requires importing PreconditionFailedError from gcs_client.ts.

2. gcs_lock_test.ts:123-128 (unchanged file, but affects coverage of the changed code) — Mock getObject never returns generation, so the forceRelease test at line 293 never exercises the conditional delete path.

   The mock's getObject returns { data } without a generation field. readLockWithGeneration() will always see generation: undefined, falling back to the unconditional delete branch. The test passes, but it's testing the pre-fix behavior (unconditional delete), not the fix (conditional delete with generation). This means finding #1 above is completely untested.

   (Note: the mock file itself is not in the changed-files list, so this is flagged as a coverage gap in the PR's test changes, not a blocking issue in an unchanged file.)

Low

1. gcs.ts:163 — Empty-string namespace passes validation but produces a semantically broken registration.

   The regex /[/\\]|^\.\.?$|\.\.[\\/]/ does not match an empty string. registerNamespace("", repoId) succeeds, creating key /.namespace.json (leading slash). listNamespaces then filters it out via the if (ns) guard at line 240, so the namespace is registered but invisible. Unlikely in practice — callers probably validate upstream — but the validator could add namespace.length === 0 to the check for completeness.

2. gcs.ts:214 — Race-recovery path after putObjectConditional returns null does not handle NotFoundError from getObject.

   If putObjectConditional returns null (object existed) but the object is deleted before the re-check getObject call on line 214, NotFoundError propagates uncaught. The initial read-before-write path (line 173) handles NotFoundError gracefully, but this path does not. Extremely unlikely race — requires the competing writer to register and unregister between lines 211 and 214.

Verdict

PASS — The core fixes (forceRelease TOCTOU mitigation via generation-conditional delete, namespace path-traversal guard, read-before-write for non-compliant backends, and dependency pinning) are well-structured and the test additions cover the important cases. The medium findings are real contract/coverage gaps but do not cause data loss or security issues in production paths — forceRelease is an administrative escape hatch, and the generation mismatch scenario means the lock holder is actively heartbeating (i.e., not actually stuck), so throwing is arguably safer than silently deleting.

## Adversarial Review ### Medium 1. **`gcs_lock.ts:226-236` — `forceRelease` throws `PreconditionFailedError` instead of returning `false` when the lock holder heartbeats between read and delete.** The TOCTOU fix correctly uses `ifGenerationMatch` on the delete, but `deleteObject` does not catch `PreconditionFailedError` (it only swallows `NotFoundError`). If the lock holder calls `extend()` between `readLockWithGeneration()` and `deleteObject()`, the generation changes, the conditional delete throws `PreconditionFailedError`, and the exception propagates to the caller. The method's return type is `Promise<boolean>` — an uncaught exception violates this contract. **Breaking scenario:** Admin inspects a stuck lock, gets the nonce, calls `forceRelease(nonce)`. Between the read and delete, the lock holder's heartbeat fires and extends the lock (new generation). `deleteObject` throws `PreconditionFailedError` — the admin's tooling crashes with an unhandled exception instead of receiving `false`. **Suggested fix:** ```typescript async forceRelease(expectedNonce: string): Promise<boolean> { const result = await this.readLockWithGeneration(); if (!result || result.info.nonce !== expectedNonce) { return false; } try { await this.gcs.deleteObject( this.lockKey, result.generation ? { ifGenerationMatch: result.generation } : undefined, ); return true; } catch (err) { if (err instanceof PreconditionFailedError) return false; throw err; } } ``` This requires importing `PreconditionFailedError` from `gcs_client.ts`. 2. **`gcs_lock_test.ts:123-128` (unchanged file, but affects coverage of the changed code) — Mock `getObject` never returns `generation`, so the `forceRelease` test at line 293 never exercises the conditional delete path.** The mock's `getObject` returns `{ data }` without a `generation` field. `readLockWithGeneration()` will always see `generation: undefined`, falling back to the unconditional delete branch. The test passes, but it's testing the pre-fix behavior (unconditional delete), not the fix (conditional delete with generation). This means finding #1 above is completely untested. *(Note: the mock file itself is not in the changed-files list, so this is flagged as a coverage gap in the PR's test changes, not a blocking issue in an unchanged file.)* ### Low 1. **`gcs.ts:163` — Empty-string namespace passes validation but produces a semantically broken registration.** The regex `/[/\\]|^\.\.?$|\.\.[\\/]/` does not match an empty string. `registerNamespace("", repoId)` succeeds, creating key `/.namespace.json` (leading slash). `listNamespaces` then filters it out via the `if (ns)` guard at line 240, so the namespace is registered but invisible. Unlikely in practice — callers probably validate upstream — but the validator could add `namespace.length === 0` to the check for completeness. 2. **`gcs.ts:214` — Race-recovery path after `putObjectConditional` returns null does not handle `NotFoundError` from `getObject`.** If `putObjectConditional` returns null (object existed) but the object is deleted before the re-check `getObject` call on line 214, `NotFoundError` propagates uncaught. The initial read-before-write path (line 173) handles `NotFoundError` gracefully, but this path does not. Extremely unlikely race — requires the competing writer to register *and* unregister between lines 211 and 214. ### Verdict **PASS** — The core fixes (forceRelease TOCTOU mitigation via generation-conditional delete, namespace path-traversal guard, read-before-write for non-compliant backends, and dependency pinning) are well-structured and the test additions cover the important cases. The medium findings are real contract/coverage gaps but do not cause data loss or security issues in production paths — `forceRelease` is an administrative escape hatch, and the generation mismatch scenario means the lock holder is actively heartbeating (i.e., not actually stuck), so throwing is arguably safer than silently deleting.

stack72 added 1 commit 2026-06-05 22:52:44 +00:00

fix(gcs-datastore): wrap forceRelease delete in try/catch for 412 race ... e9e1cf7b33

ifGenerationMatch on deleteObject can throw PreconditionFailedError
when the lock generation changes between readLockWithGeneration and
the delete. This is an expected race outcome, not a real error —
return false instead of throwing to honor the Promise<boolean> contract.

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

stack72 added 1 commit 2026-06-05 23:02:05 +00:00

style(gcs-datastore): deno fmt forceRelease ternary ... 7ae9ab6667

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

stack72 commented 2026-06-05 23:25:46 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. Copyright header inconsistency (gcs.ts line 2 vs gcs_lock.ts line 2): gcs.ts uses "Elder Swamp Club, Inc." while gcs_lock.ts uses "System Initiative, Inc." Pick one entity name and use it consistently across the datastore files.

2. createLock / createVerifier / createSyncService each instantiate a fresh GcsClient (gcs.ts lines 129, 135, 141): Each GcsClient brings its own connection pool. For tight-loop callers this can add overhead. Sharing a single GcsClient at provider-construction time — or at least a lazy singleton — would be more efficient. Not a correctness problem, just a resource-use note.

3. while (true) in acquire() does not have an explicit return path for the timeout branch: the timeout is handled by throw new LockTimeoutError(...), so this is fine as written, but a future reader might expect a break or loop condition. Consider while (Date.now() - startTime < this.maxWaitMs) for clarity (the timeout read at the top of the loop and the final throw after the loop), though again — not a correctness issue.

4. gcs_test.ts mock server always returns generation: "1": the re-registration test path calls putObject after the read-before-write check, but the mock always returns the same static generation. This is adequate for correctness testing here, but it means the test cannot catch bugs in code paths that depend on the returned generation (the namespace registration code doesn't use it, so this is fine — just noting the mock fidelity limit).

## Code Review ### Blocking Issues None. ### Suggestions 1. **Copyright header inconsistency** (`gcs.ts` line 2 vs `gcs_lock.ts` line 2): `gcs.ts` uses "Elder Swamp Club, Inc." while `gcs_lock.ts` uses "System Initiative, Inc." Pick one entity name and use it consistently across the datastore files. 2. **`createLock` / `createVerifier` / `createSyncService` each instantiate a fresh `GcsClient`** (`gcs.ts` lines 129, 135, 141): Each `GcsClient` brings its own connection pool. For tight-loop callers this can add overhead. Sharing a single `GcsClient` at provider-construction time — or at least a lazy singleton — would be more efficient. Not a correctness problem, just a resource-use note. 3. **`while (true)` in `acquire()` does not have an explicit `return` path for the timeout branch**: the timeout is handled by `throw new LockTimeoutError(...)`, so this is fine as written, but a future reader might expect a `break` or loop condition. Consider `while (Date.now() - startTime < this.maxWaitMs)` for clarity (the timeout read at the top of the loop and the final `throw` after the loop), though again — not a correctness issue. 4. **`gcs_test.ts` mock server always returns `generation: "1"`**: the re-registration test path calls `putObject` after the read-before-write check, but the mock always returns the same static generation. This is adequate for correctness testing here, but it means the test cannot catch bugs in code paths that depend on the returned generation (the namespace registration code doesn't use it, so this is fine — just noting the mock fidelity limit).

stack72 commented 2026-06-05 23:30:40 +00:00

Author

Owner

Copy link

Adversarial Review

Reviewed files: codegen/designs/gcp.md, codegen/gcp/pipeline.ts, datastore/gcs/extensions/datastores/_lib/gcs_lock.ts, datastore/gcs/extensions/datastores/gcs.ts, datastore/gcs/extensions/datastores/gcs_test.ts, datastore/gcs/manifest.yaml. All model/ files skipped (auto-generated).

Critical / High

No critical or high severity issues found.

Medium

1. codegen/gcp/pipeline.ts:506-546 — Removed cross-version resource deduplication; nondeterministic processing order

   The old code sorted schema files (preferred before additional) and tracked seenResourceKeys to prevent duplicate resources across versions. The new code iterates Deno.readDir (no guaranteed ordering) and pushes all resources from all schema files without any deduplication.

   For the current ADDITIONAL_VERSIONS (only iam: ["v1"]), this is safe because IAM v1 and v2 have disjoint resource paths. However, if a future ADDITIONAL_VERSIONS entry has overlapping resource paths with the preferred version, both copies would be included in allResources, producing duplicate model files. Which copy "wins" on disk depends on Deno.readDir iteration order — nondeterministic.

   The ADDITIONAL_VERSION_RESOURCE_FILTER exists as a manual safeguard but is empty and unused. The CLAUDE.md instructs "Run generation a second time to verify idempotency — there should be zero new diffs on the second run." If Deno.readDir order varies between runs for a service spanning multiple schema files, resource ordering in the models array could change, potentially affecting manifest file listings.

   Suggested fix: Either sort the directory entries by name before processing (restoring deterministic order), or keep the dedup set as a safety net. A one-liner entries.sort((a, b) => a.name.localeCompare(b.name)) before the processing loop would suffice.

2. codegen/gcp/pipeline.ts:514-516 — Service filter false positive for APIs whose names share a prefix with ADDITIONAL_VERSIONS keys

   The new service filter uses serviceName.startsWith(\${base}-`)to detect additional-version files. If a GCP API existed with a name likeiam-admin, running deno task generate:gcp iamwould also processiam-admin.json, because "iam-admin".startsWith("iam-")is true. The old regexADDITIONAL_VERSION_FILENAME_RE = /^(.+)-(v\d+)$/ only matched version-like suffixes (-v1, -v2), so iam-adminwould not have been treated as an additional version ofiam`.

   No current GCP API names use hyphens so this is not actively broken, but it's a latent defect.

   Suggested fix: Use a more precise check, e.g. ADDITIONAL_VERSIONS[base]?.some(v => serviceName === \${base}-${v}`)instead ofstartsWith`.

Low

3. datastore/gcs/extensions/datastores/gcs.ts:163 — Empty namespace string passes validation

   The regex /[/\\]|^\.\.?$|\.\.[\\/]/ does not reject empty strings. An empty namespace would produce the object key /.namespace.json. While callers likely validate before reaching this code, the guard could be more defensive.

   Example: await provider.registerNamespace("/tmp/ds", "", "repo-aaa") succeeds silently and creates a leading-slash key.

4. datastore/gcs/extensions/datastores/_lib/gcs_lock.ts:232-236 — forceRelease falls back to unconditional delete when generation is undefined

   If getObject doesn't return a generation header (e.g., a non-standard GCS-compatible backend), result.generation is undefined and the ternary passes undefined options to deleteObject, performing an unconditional delete. This is the same as the old behavior so it's not a regression, but it defeats the purpose of the generation-based guard on that code path.

Verdict

PASS — The datastore changes (read-before-write namespace registration, generation-conditional forceRelease) are solid improvements with good test coverage. The codegen changes are a reasonable simplification from auto-discovery to an explicit version map. The medium concerns are about future-proofing and determinism, not current correctness.

## Adversarial Review *Reviewed files: `codegen/designs/gcp.md`, `codegen/gcp/pipeline.ts`, `datastore/gcs/extensions/datastores/_lib/gcs_lock.ts`, `datastore/gcs/extensions/datastores/gcs.ts`, `datastore/gcs/extensions/datastores/gcs_test.ts`, `datastore/gcs/manifest.yaml`. All `model/` files skipped (auto-generated).* ### Critical / High No critical or high severity issues found. ### Medium 1. **`codegen/gcp/pipeline.ts`:506-546 — Removed cross-version resource deduplication; nondeterministic processing order** The old code sorted schema files (preferred before additional) and tracked `seenResourceKeys` to prevent duplicate resources across versions. The new code iterates `Deno.readDir` (no guaranteed ordering) and pushes all resources from all schema files without any deduplication. For the current `ADDITIONAL_VERSIONS` (only `iam: ["v1"]`), this is safe because IAM v1 and v2 have disjoint resource paths. However, if a future `ADDITIONAL_VERSIONS` entry has overlapping resource paths with the preferred version, both copies would be included in `allResources`, producing duplicate model files. Which copy "wins" on disk depends on `Deno.readDir` iteration order — nondeterministic. The `ADDITIONAL_VERSION_RESOURCE_FILTER` exists as a manual safeguard but is empty and unused. The CLAUDE.md instructs "Run generation a second time to verify idempotency — there should be zero new diffs on the second run." If `Deno.readDir` order varies between runs for a service spanning multiple schema files, resource ordering in the models array could change, potentially affecting manifest file listings. **Suggested fix:** Either sort the directory entries by name before processing (restoring deterministic order), or keep the dedup set as a safety net. A one-liner `entries.sort((a, b) => a.name.localeCompare(b.name))` before the processing loop would suffice. 2. **`codegen/gcp/pipeline.ts`:514-516 — Service filter false positive for APIs whose names share a prefix with ADDITIONAL_VERSIONS keys** The new service filter uses `serviceName.startsWith(\`${base}-\`)` to detect additional-version files. If a GCP API existed with a name like `iam-admin`, running `deno task generate:gcp iam` would also process `iam-admin.json`, because `"iam-admin".startsWith("iam-")` is true. The old regex `ADDITIONAL_VERSION_FILENAME_RE = /^(.+)-(v\d+)$/` only matched version-like suffixes (`-v1`, `-v2`), so `iam-admin` would not have been treated as an additional version of `iam`. No current GCP API names use hyphens so this is not actively broken, but it's a latent defect. **Suggested fix:** Use a more precise check, e.g. `ADDITIONAL_VERSIONS[base]?.some(v => serviceName === \`${base}-${v}\`)` instead of `startsWith`. ### Low 3. **`datastore/gcs/extensions/datastores/gcs.ts`:163 — Empty namespace string passes validation** The regex `/[/\\]|^\.\.?$|\.\.[\\/]/` does not reject empty strings. An empty namespace would produce the object key `/.namespace.json`. While callers likely validate before reaching this code, the guard could be more defensive. **Example:** `await provider.registerNamespace("/tmp/ds", "", "repo-aaa")` succeeds silently and creates a leading-slash key. 4. **`datastore/gcs/extensions/datastores/_lib/gcs_lock.ts`:232-236 — `forceRelease` falls back to unconditional delete when generation is undefined** If `getObject` doesn't return a generation header (e.g., a non-standard GCS-compatible backend), `result.generation` is `undefined` and the ternary passes `undefined` options to `deleteObject`, performing an unconditional delete. This is the same as the old behavior so it's not a regression, but it defeats the purpose of the generation-based guard on that code path. ### Verdict **PASS** — The datastore changes (read-before-write namespace registration, generation-conditional `forceRelease`) are solid improvements with good test coverage. The codegen changes are a reasonable simplification from auto-discovery to an explicit version map. The medium concerns are about future-proofing and determinism, not current correctness.

stack72 merged commit cf51949201 into main 2026-06-05 23:34:54 +00:00

stack72 deleted branch fix/567-gcs-namespace-conflict-detection 2026-06-05 23:34:54 +00:00

stack72 referenced this pull request from a commit 2026-06-05 23:34:55 +00:00

fix(gcs-datastore): add read-before-write conflict detection in registerNamespace (#567) (#37)

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

No description provided.