fix(datastores): namespace-scope controlPlaneStore keys, migrate root _control/, add lock namespace support (#1889) #242

Merged

stack72 merged 1 commit from worktree-1889 into main 2026-08-28 21:05:21 +00:00

Conversation 2 Commits 1 Files changed 13 +701 -22

stack72 commented 2026-08-28 20:34:19 +00:00

Owner

Copy link

Summary

• controlPlaneStore() binding: Methods now call ensureBound() — if a namespace was bound by pullChanged, control-plane operations use it; if unbound, solo mode binds undefined. Prevents silent root-level writes in namespaced deployments. list() evaluates controlPrefixPath() lazily instead of capturing a stale prefix at construction time.
• Control-plane key migration: New migrateRootControlPlaneToNamespace() copies orphaned root-level _control/ keys to {namespace}/_control/ on push, with hasNamespacedIndex guard (only runs for solo-to-namespace transitions). Tracked via controlPlaneKeysMigrated sync state flag.
• Lock namespace support: LockOptions.namespace prefixes the lock key under {namespace}/ so namespace-scoped IAM policies can reach it. Core adoption tracked in #1897.
• Error diagnostic: S3 wrapError now includes the S3 key in error messages so AccessDenied errors show which key was targeted.
• All changes applied symmetrically to S3 and GCS.

Core-side fixes for the root-level global lock derivation and undefined-namespace caller tracked in #1897.

Closes #1889, closes #1896.

Test plan

• S3: 267 tests pass (8 new: binding protocol, migration copy/delete, migration guard, list lazy prefix, lock namespace scoping)
• GCS: 259 tests pass (8 new: symmetric to S3)
• deno check, deno lint, deno fmt --check, deno install --frozen pass for both extensions
• CI matrix runs for datastore-s3 and datastore-gcs

🤖 Generated with Claude Code

## Summary - **controlPlaneStore() binding**: Methods now call `ensureBound()` — if a namespace was bound by `pullChanged`, control-plane operations use it; if unbound, solo mode binds `undefined`. Prevents silent root-level writes in namespaced deployments. `list()` evaluates `controlPrefixPath()` lazily instead of capturing a stale prefix at construction time. - **Control-plane key migration**: New `migrateRootControlPlaneToNamespace()` copies orphaned root-level `_control/` keys to `{namespace}/_control/` on push, with `hasNamespacedIndex` guard (only runs for solo-to-namespace transitions). Tracked via `controlPlaneKeysMigrated` sync state flag. - **Lock namespace support**: `LockOptions.namespace` prefixes the lock key under `{namespace}/` so namespace-scoped IAM policies can reach it. Core adoption tracked in #1897. - **Error diagnostic**: S3 `wrapError` now includes the S3 key in error messages so `AccessDenied` errors show which key was targeted. - All changes applied symmetrically to S3 and GCS. Core-side fixes for the root-level global lock derivation and undefined-namespace caller tracked in #1897. Closes #1889, closes #1896. ## Test plan - [x] S3: 267 tests pass (8 new: binding protocol, migration copy/delete, migration guard, list lazy prefix, lock namespace scoping) - [x] GCS: 259 tests pass (8 new: symmetric to S3) - [x] `deno check`, `deno lint`, `deno fmt --check`, `deno install --frozen` pass for both extensions - [ ] CI matrix runs for `datastore-s3` and `datastore-gcs` 🤖 Generated with [Claude Code](https://claude.com/claude-code)

stack72 added 1 commit 2026-08-28 20:34:19 +00:00

fix(datastores): namespace-scope controlPlaneStore keys, migrate root _control/, add lock namespace support (#1889) ... 4295a0e47f

Three independent fixes for namespaced datastore deployments where IAM
credentials are scoped to the namespace prefix:

1. controlPlaneStore() now participates in namespace binding via
   ensureBound() — if a namespace was bound by pullChanged, control-plane
   operations use it; if no binding exists, solo mode binds undefined.
   list() evaluates controlPrefixPath() lazily instead of capturing a
   stale prefix at construction time.

2. New migrateRootControlPlaneToNamespace() migrates orphaned root-level
   _control/ keys to {namespace}/_control/ on push, guarded by
   hasNamespacedIndex (only runs for solo-to-namespace transitions).
   Tracked via controlPlaneKeysMigrated sync state flag.

3. LockOptions.namespace prefixes the lock key under {namespace}/ so
   namespace-scoped IAM policies can reach it. S3 wrapError now includes
   the S3 key in error messages so AccessDenied errors show which key
   was targeted.

All changes applied symmetrically to S3 and GCS. Core-side fixes for
the root-level global lock derivation and undefined-namespace caller
tracked in swamp-club#1897.

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

stack72 commented 2026-08-28 20:42:22 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. GCS/S3 migrateRootControlPlaneToNamespace: guard silently swallows all errors, inconsistent with migrateRootDataToNamespace.

   The existing migrateRootDataToNamespace rethrows non-NotFoundErrors from the namespaced-index guard check. The new migrateRootControlPlaneToNamespace (both GCS and S3) uses a bare catch that swallows all errors and comments "Can't check — skip migration". A transient 5xx or network timeout during the guard check will silently skip migration — and once a subsequent push records controlPlaneKeysMigrated = true, the migration opportunity is permanently suppressed. The data-key variant correctly propagates transient errors so they surface as sync failures and are retried on the next call. The control-plane variant should do the same, or at minimum not set controlPlaneKeysMigrated = true when the guard errored rather than returned exists: false.

2. s3_client.ts: error message key includes the full S3 bucket-prefix path; span attribute uses the logical key.

   run() extracts inputKey from the SDK command's input.Key (the full S3 key including the configured prefix, e.g. swamp/data/@org/file.yaml) and passes it to wrapError. The span attribute correctly strips the prefix to record the logical key. The resulting error message shows key=swamp/data/@org/file.yaml, which may confuse users who only see logical keys. Minor cosmetic issue -- passing logicalKey to wrapError instead would be consistent with how the span is recorded.

3. preparePush migration path not covered by tests.

   Both S3 and GCS test the control-plane migration via pushChanged but not via preparePush (the two-phase path). Since both entry points call the same private method the gap is low-risk, but a test exercising preparePush + commitPush against a bucket with root _control/ keys would complete the coverage.

## Code Review ### Blocking Issues None. ### Suggestions 1. **GCS/S3 `migrateRootControlPlaneToNamespace`: guard silently swallows all errors, inconsistent with `migrateRootDataToNamespace`.** The existing `migrateRootDataToNamespace` rethrows non-`NotFoundError`s from the namespaced-index guard check. The new `migrateRootControlPlaneToNamespace` (both GCS and S3) uses a bare catch that swallows all errors and comments "Can't check — skip migration". A transient 5xx or network timeout during the guard check will silently skip migration — and once a subsequent push records `controlPlaneKeysMigrated = true`, the migration opportunity is permanently suppressed. The data-key variant correctly propagates transient errors so they surface as sync failures and are retried on the next call. The control-plane variant should do the same, or at minimum not set `controlPlaneKeysMigrated = true` when the guard errored rather than returned `exists: false`. 2. **`s3_client.ts`: error message key includes the full S3 bucket-prefix path; span attribute uses the logical key.** `run()` extracts `inputKey` from the SDK command's `input.Key` (the full S3 key including the configured prefix, e.g. `swamp/data/@org/file.yaml`) and passes it to `wrapError`. The span attribute correctly strips the prefix to record the logical key. The resulting error message shows `key=swamp/data/@org/file.yaml`, which may confuse users who only see logical keys. Minor cosmetic issue -- passing `logicalKey` to `wrapError` instead would be consistent with how the span is recorded. 3. **`preparePush` migration path not covered by tests.** Both S3 and GCS test the control-plane migration via `pushChanged` but not via `preparePush` (the two-phase path). Since both entry points call the same private method the gap is low-risk, but a test exercising `preparePush` + `commitPush` against a bucket with root `_control/` keys would complete the coverage.

stack72 commented 2026-08-28 20:46:59 +00:00

Author

Owner

Copy link

Adversarial Code Review

PR: Namespace-scope controlPlaneStore keys, migrate root _control/, add lock namespace support

Findings

1. GCS migration declares wrong type for listing entries

Severity: MEDIUM
Category: Logic / Correctness
File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:770

The migrateRootControlPlaneToNamespace method declares:

let listing: Array<{ key: string; size: number; lastModified?: Date }>;

But GcsClient.listAllObjects() returns Promise<GcsListEntry[]> where GcsListEntry is:

interface GcsListEntry {
  key: string;
  size: number;
  generation?: string;
  updated?: Date;  // NOT lastModified
}

The S3 version correctly uses lastModified which matches its client return type. The GCS version uses a field name (lastModified) that does not exist on the actual return type. The field is not accessed in the migration body (only key is used), so there is no runtime bug today. However:

• The type annotation is misleading -- a future developer would assume entry.lastModified is available.
• TypeScript structural subtyping silently accepts this because only required fields must match.
• The GCS-specific generation field is also dropped, preventing future use without re-typing.

Fix: Change the type annotation to match GcsListEntry or import the type directly.

---

2. Silent error swallowing on index existence check can skip needed migration

Severity: LOW
Category: Error Handling
File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:760-767
File: datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts:819-827

Both migration methods catch and ignore errors when checking if the namespaced index exists. If the HEAD/metadata call fails due to a transient error (network timeout, temporary auth failure, rate limit), migration is silently skipped. The controlPlaneKeysMigrated flag is never set to true, so migration will be re-attempted on the next pushChanged call -- making this self-healing.

The comment documents the intent. This is a reasonable defensive choice, but persistent transient failures will cause the migration to be silently re-attempted on every push without user-visible diagnostics.

Suggestion: Consider logging a warning on catch so operators can diagnose repeated skips.

---

3. Delete failures during control-plane migration leave orphaned source keys

Severity: LOW
Category: Data Integrity
File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:807-818
File: datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts:867-879

After successfully copying keys, both implementations delete the originals using Promise.allSettled. Failed deletes are silently ignored. When copied === total but some deletes fail, controlPlaneKeysMigrated is set to true and the orphaned root keys persist indefinitely.

This is acceptable because the namespaced copies are authoritative after migration. However, the data key migration logs partial failure warnings. The control-plane migration should match this pattern for consistency and observability.

---

4. Listing failure falsely marks migration as permanently complete

Severity: MEDIUM
Category: Logic / Correctness
File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:771-778
File: datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts:830-838

If listAllObjects("_control/") fails after all retry attempts, migration returns { copied: 0, total: 0 }. The caller then evaluates:

if (total === 0 || copied === total) {
  sidecar.controlPlaneKeysMigrated = true;
}

When listing fails: total = 0, copied = 0, so the condition is true and controlPlaneKeysMigrated = true. Migration is marked complete without ever verifying whether root keys exist.

This is a real logic gap: a listing failure (permissions, network) permanently marks migration as done, leaving root _control/ keys unscoped. The migration will never retry.

Fix: Return a sentinel that distinguishes "no keys found" from "listing failed", e.g. { copied: 0, total: -1 }, and guard: if (total >= 0 && (total === 0 || copied === total)).

---

5. Code pattern inconsistency: GCS vs S3 data-key migration sidecar ordering

Severity: LOW
Category: Logic / Correctness
File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:2032-2052
File: datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts:2160-2171

GCS data-key migration duplicates the sidecar write block across two branches (inside and outside if (copied > 0)), while S3 writes the sidecar once unconditionally before the early return check. Both are functionally equivalent but the divergence means a future fix to one may not be applied to the other.

---

6. ControlPlaneStore.list() prefix semantics undocumented

Severity: LOW
Category: API Contracts
File: datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:3286-3293
File: datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts:3459-3466

The list method strips cpPrefix (e.g., my-ns/_control/) from keys but does NOT strip the caller-provided prefix. If a caller does list("heartbeats/"), they get back ["heartbeats/inst-1", ...] not ["inst-1", ...].

This is consistent with typical list-by-prefix APIs and tests confirm the intended behavior. However, the ControlPlaneStore interface (interfaces.ts:117) does not document whether the prefix is stripped from results.

Suggestion: Add a JSDoc comment to the interface clarifying return key semantics.

---

7. Lock namespace tests verify key placement but not contention isolation

Severity: LOW
Category: Concurrency
File: datastore/gcs/extensions/datastores/_lib/gcs_lock_test.ts:476-503
File: datastore/s3/extensions/datastores/_lib/s3_lock_test.ts:248-275

The new namespace tests verify lock keys are placed under {namespace}/.datastore.lock, which is correct. However, they do not test that a lock in namespace A does not block acquisition in namespace B. Given that contention isolation is the primary purpose of namespace-scoped locking, a cross-namespace non-contention test would strengthen confidence.

---

Summary

#	Severity	Finding
1	MEDIUM	GCS migration type annotation uses lastModified instead of updated (copy-paste from S3)
2	LOW	Silent index-existence-check failure skips migration without diagnostics
3	LOW	Delete failures leave orphaned root keys when migration is marked complete
4	MEDIUM	Listing failure causes total=0, falsely marking migration as permanently complete
5	LOW	GCS/S3 data-key migration have divergent sidecar write patterns (functionally equivalent)
6	LOW	ControlPlaneStore.list() prefix semantics undocumented in interface
7	LOW	Lock namespace tests do not verify cross-namespace non-contention

No CRITICAL or HIGH severity findings.

Finding #4 has the most real-world impact potential: a transient listing failure during migration permanently marks migration as complete, leaving root _control/ keys unscoped. The window is narrow (requires listAllObjects to fail after all retries on a push where migration has not yet been marked done), but the consequence is silent data skew that will not self-heal.

Finding #1 is a clear copy-paste discrepancy from the S3 implementation that should be corrected to avoid misleading future readers, even though it has no runtime impact today.

The overall implementation is solid: the namespace guard prevents cross-tenant key migration, idempotent copy-then-delete handles partial failures gracefully, the controlPlaneKeysMigrated flag prevents unnecessary re-runs, and test coverage is thorough including both the happy path (migration runs) and the guard path (migration skips for fresh namespaces).

# Adversarial Code Review **PR: Namespace-scope controlPlaneStore keys, migrate root _control/, add lock namespace support** ## Findings ### 1. GCS migration declares wrong type for listing entries **Severity: MEDIUM** **Category: Logic / Correctness** **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:770` The `migrateRootControlPlaneToNamespace` method declares: ```typescript let listing: Array<{ key: string; size: number; lastModified?: Date }>; ``` But `GcsClient.listAllObjects()` returns `Promise<GcsListEntry[]>` where `GcsListEntry` is: ```typescript interface GcsListEntry { key: string; size: number; generation?: string; updated?: Date; // NOT lastModified } ``` The S3 version correctly uses `lastModified` which matches its client return type. The GCS version uses a field name (`lastModified`) that does not exist on the actual return type. The field is not accessed in the migration body (only `key` is used), so there is no runtime bug today. However: - The type annotation is misleading -- a future developer would assume `entry.lastModified` is available. - TypeScript structural subtyping silently accepts this because only required fields must match. - The GCS-specific `generation` field is also dropped, preventing future use without re-typing. **Fix:** Change the type annotation to match `GcsListEntry` or import the type directly. --- ### 2. Silent error swallowing on index existence check can skip needed migration **Severity: LOW** **Category: Error Handling** **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:760-767` **File:** `datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts:819-827` Both migration methods catch and ignore errors when checking if the namespaced index exists. If the HEAD/metadata call fails due to a transient error (network timeout, temporary auth failure, rate limit), migration is silently skipped. The `controlPlaneKeysMigrated` flag is never set to `true`, so migration will be re-attempted on the next `pushChanged` call -- making this self-healing. The comment documents the intent. This is a reasonable defensive choice, but persistent transient failures will cause the migration to be silently re-attempted on every push without user-visible diagnostics. **Suggestion:** Consider logging a warning on catch so operators can diagnose repeated skips. --- ### 3. Delete failures during control-plane migration leave orphaned source keys **Severity: LOW** **Category: Data Integrity** **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:807-818` **File:** `datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts:867-879` After successfully copying keys, both implementations delete the originals using `Promise.allSettled`. Failed deletes are silently ignored. When `copied === total` but some deletes fail, `controlPlaneKeysMigrated` is set to `true` and the orphaned root keys persist indefinitely. This is acceptable because the namespaced copies are authoritative after migration. However, the data key migration logs partial failure warnings. The control-plane migration should match this pattern for consistency and observability. --- ### 4. Listing failure falsely marks migration as permanently complete **Severity: MEDIUM** **Category: Logic / Correctness** **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:771-778` **File:** `datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts:830-838` If `listAllObjects("_control/")` fails after all retry attempts, migration returns `{ copied: 0, total: 0 }`. The caller then evaluates: ```typescript if (total === 0 || copied === total) { sidecar.controlPlaneKeysMigrated = true; } ``` When listing fails: `total = 0`, `copied = 0`, so the condition is `true` and `controlPlaneKeysMigrated = true`. Migration is marked complete without ever verifying whether root keys exist. This is a real logic gap: a listing failure (permissions, network) permanently marks migration as done, leaving root `_control/` keys unscoped. The migration will never retry. **Fix:** Return a sentinel that distinguishes "no keys found" from "listing failed", e.g. `{ copied: 0, total: -1 }`, and guard: `if (total >= 0 && (total === 0 || copied === total))`. --- ### 5. Code pattern inconsistency: GCS vs S3 data-key migration sidecar ordering **Severity: LOW** **Category: Logic / Correctness** **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:2032-2052` **File:** `datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts:2160-2171` GCS data-key migration duplicates the sidecar write block across two branches (inside and outside `if (copied > 0)`), while S3 writes the sidecar once unconditionally before the early return check. Both are functionally equivalent but the divergence means a future fix to one may not be applied to the other. --- ### 6. ControlPlaneStore.list() prefix semantics undocumented **Severity: LOW** **Category: API Contracts** **File:** `datastore/gcs/extensions/datastores/_lib/gcs_cache_sync.ts:3286-3293` **File:** `datastore/s3/extensions/datastores/_lib/s3_cache_sync.ts:3459-3466` The `list` method strips `cpPrefix` (e.g., `my-ns/_control/`) from keys but does NOT strip the caller-provided `prefix`. If a caller does `list("heartbeats/")`, they get back `["heartbeats/inst-1", ...]` not `["inst-1", ...]`. This is consistent with typical list-by-prefix APIs and tests confirm the intended behavior. However, the `ControlPlaneStore` interface (interfaces.ts:117) does not document whether the prefix is stripped from results. **Suggestion:** Add a JSDoc comment to the interface clarifying return key semantics. --- ### 7. Lock namespace tests verify key placement but not contention isolation **Severity: LOW** **Category: Concurrency** **File:** `datastore/gcs/extensions/datastores/_lib/gcs_lock_test.ts:476-503` **File:** `datastore/s3/extensions/datastores/_lib/s3_lock_test.ts:248-275` The new namespace tests verify lock keys are placed under `{namespace}/.datastore.lock`, which is correct. However, they do not test that a lock in namespace A does not block acquisition in namespace B. Given that contention isolation is the primary purpose of namespace-scoped locking, a cross-namespace non-contention test would strengthen confidence. --- ## Summary | # | Severity | Finding | |---|----------|---------| | 1 | MEDIUM | GCS migration type annotation uses `lastModified` instead of `updated` (copy-paste from S3) | | 2 | LOW | Silent index-existence-check failure skips migration without diagnostics | | 3 | LOW | Delete failures leave orphaned root keys when migration is marked complete | | 4 | MEDIUM | Listing failure causes total=0, falsely marking migration as permanently complete | | 5 | LOW | GCS/S3 data-key migration have divergent sidecar write patterns (functionally equivalent) | | 6 | LOW | ControlPlaneStore.list() prefix semantics undocumented in interface | | 7 | LOW | Lock namespace tests do not verify cross-namespace non-contention | **No CRITICAL or HIGH severity findings.** Finding #4 has the most real-world impact potential: a transient listing failure during migration permanently marks migration as complete, leaving root `_control/` keys unscoped. The window is narrow (requires `listAllObjects` to fail after all retries on a push where migration has not yet been marked done), but the consequence is silent data skew that will not self-heal. Finding #1 is a clear copy-paste discrepancy from the S3 implementation that should be corrected to avoid misleading future readers, even though it has no runtime impact today. The overall implementation is solid: the namespace guard prevents cross-tenant key migration, idempotent copy-then-delete handles partial failures gracefully, the `controlPlaneKeysMigrated` flag prevents unnecessary re-runs, and test coverage is thorough including both the happy path (migration runs) and the guard path (migration skips for fresh namespaces).

stack72 merged commit 8669b6ac8c into main 2026-08-28 21:05:21 +00:00

stack72 referenced this pull request from a commit 2026-08-28 21:05:22 +00:00

fix(datastores): namespace-scope controlPlaneStore keys, migrate root _control/, add lock namespace support (#1889) (#242)

stack72 deleted branch worktree-1889 2026-08-28 21:05:22 +00:00

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

No description provided.