From 981a14d636f6388669180fce36b2ebbfa9baf2bf Mon Sep 17 00:00:00 2001 From: Zach Matarasso Date: Tue, 7 Jul 2026 13:25:28 -0400 Subject: [PATCH] feat(storage): add atomic setIfAbsent primitive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a race-free "write only if the key is not already set" command to plugin storage: storage.setIfAbsent(key, value, { ttlSeconds? }) plus the pipeline set_if_absent command. Resolves to the stored item on a win, or { value: undefined } when the key already existed (claim lost) — mirroring the get-miss convention. This is the client half; the platform (envoy-web PluginStorageService) must learn the set_if_absent action for it to resolve. See docs/proposals/atomic-set-if-absent.md. Motivation: integrations that provision from two concurrent paths (e.g. an event webhook + an app-extension render, possibly on different pods) need a real mutual-exclusion primitive. Today's get-then-set is a TOCTOU race and set_unique overwrites. setIfAbsent lets exactly one caller win, resolved server-side against the existing unique index. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/proposals/atomic-set-if-absent.md | 177 ++++++++++++++++++ src/base/EnvoyPluginStoragePipeline.ts | 11 ++ src/internal/EnvoyStorageCommand.ts | 9 +- src/mocks/EnvoyPluginStoragePipelineMock.ts | 22 +++ src/sdk/EnvoyPluginStorage.ts | 12 ++ .../EnvoyPluginStoragePipelineMock.test.ts | 35 ++++ 6 files changed, 265 insertions(+), 1 deletion(-) create mode 100644 docs/proposals/atomic-set-if-absent.md create mode 100644 test/mocks/EnvoyPluginStoragePipelineMock.test.ts diff --git a/docs/proposals/atomic-set-if-absent.md b/docs/proposals/atomic-set-if-absent.md new file mode 100644 index 0000000..1ef19f3 --- /dev/null +++ b/docs/proposals/atomic-set-if-absent.md @@ -0,0 +1,177 @@ +# Design: atomic `set_if_absent` for plugin storage + +**Status:** Draft / proposal +**Author:** (via QA harness review of `pacs-integration-service#202`) +**Affected repos:** `envoy-integrations-sdk-nodejs` (client), `envoy-web` (platform storage service) + +## Summary + +Add a race-free "write only if the key is not already set" primitive to plugin storage: + +- **SDK:** `storage.setIfAbsent(key, value, { ttlSeconds? })` and a matching pipeline command + `set_if_absent`. +- **Platform (`envoy-web`):** a new `set_if_absent` action in `Platform::PluginStorageService` + that `INSERT`s and relies on the existing unique index — the DB row *is* the lock. Returns the + item on a win, `null` when the key is already held. + +The result: exactly one of N concurrent invocations (across pods) wins the write. This gives +integrations a correct mutual-exclusion / claim primitive that today's API cannot express. + +## Motivation + +`pacs-integration-service#202` (By The Bay / Habitap) provisions a visitor credential from two +independent code paths that can run concurrently for the same visit: + +1. the `invite_created` webhook (for near-term invites), and +2. the `registration_complete_email` **app-extension render** (create-if-missing). + +Both do `getVisitor → (null) → createVisitor`. `getVisitor` is a *read*; the record that would make +it idempotent is only written **after** the external `POST`s complete. So two invocations that read +during that window both see "missing" and both create — producing **duplicate Habitap events**, and +because both write the same storage key afterward, the first event is orphaned (storage no longer +references it). The two paths often fire at the same instant (invite creation) and may land on +**different pods**, so in-process de-duplication cannot help. + +This is the classic check-then-act (TOCTOU) race. It cannot be fixed with a better *read*: the +guarantee has to live on the *write*. + +## Why existing primitives don't solve it + +The full storage surface today is `get`, `set`, `set_unique`, `set_unique_num`, `unset`, `list` +(confirmed in SDK `2.5.2` and `envoy-web`'s `PluginStorageService`). + +- `set` is an **unconditional** overwrite (last-write-wins) — no contention signal. +- `set_unique` / `set_unique_num` guarantee a unique *generated value* and **overwrite the key**. + The uniqueness is on the value, not on key ownership, so they cannot elect a single winner for a + known key. +- A user-land `get`-then-`set` "lock" reintroduces the exact TOCTOU race and is not cross-pod safe. +- There is no `ttl`/`expire`, and no lock/mutex utility anywhere in the SDK. + +Importantly, the backend **already performs atomic conditional writes**: `set_unique` depends on a +`RecordNotUnique` rescue against a unique index (`PluginStorageUniqValue`), and `plugin_storage_items` +already has unique indexes on `(key, plugin_install_id) WHERE archived_at IS NULL` and +`(plugin_id, key)`. `set_if_absent` is a *simpler* use of the same mechanism. + +## Proposed API (SDK) + +```ts +// EnvoyPluginStorage +setIfAbsent(key: string, value: Value, options?: { ttlSeconds?: number }): + Promise | { key: string; value: undefined }>; +``` + +- Resolves to the **stored item** when this call wrote it (claim won). +- Resolves to `{ key, value: undefined }` when the key already existed (claim lost) — mirroring the + existing `get`-miss / `setUnique`-exhaustion convention, so no new result shape is introduced. +- `ttlSeconds` (optional) gives the write an expiry so a crashed holder cannot wedge the key. + +Pipeline form, for batching a claim with a follow-up read in one round-trip: + +```ts +storage.pipeline() + .setIfAbsent(`provision-lock:${visitId}`, { at: now }, { ttlSeconds: 120 }) + .get(`visit:${visitId}`) + .execute(); +``` + +## Wire protocol (`POST /api/v2/plugin-services/storage`) + +**Request** (claim): + +```json +{ + "install_id": "inst_abc123", + "commands": [ + { + "action": "set_if_absent", + "key": "provision-lock:invite-222", + "value": { "claimedAt": 1720370000000 }, + "ttlSeconds": 120 + } + ] +} +``` + +**Response — won** (server wrote the row): + +```json +{ "data": [ { "key": "provision-lock:invite-222", "value": { "claimedAt": 1720370000000 } } ] } +``` + +**Response — lost** (key already held): + +```json +{ "data": [ null ] } +``` + +No controller or strong-params changes are required: `StorageController#pipeline` passes command +hashes through untouched, and results serialize exactly like `get`/`set` (item or `null`). + +## Platform implementation (`envoy-web`) + +Add one dispatch arm and one method to `Platform::PluginStorageService`. Unlike `set`, it must **not** +`unset` first — the whole point is to fail when the key exists: + +```ruby +when 'set_if_absent' + do_storage_item_set_if_absent(command[:key], command[:value]) +``` + +```ruby +# Atomically create an item only if the key is not already set for this scope. +# Relies on the unique index on (key, plugin_install_id) / (plugin_id, key): the INSERT either +# wins or raises RecordNotUnique, which we treat as "already claimed" and return nil. Race-free +# across concurrent requests and pods without an explicit lock — the DB is the lock. Unlike `set`, +# it never overwrites an existing value. +def do_storage_item_set_if_absent(key, value = nil) + plugin_storage_items.create!({ key: key, value: value }) +rescue ::ActiveRecord::RecordNotUnique, ::ActiveRecord::RecordInvalid + nil +end +``` + +This is correct on day one **without TTL**; TTL is a follow-up (below). + +## Consumer usage (the fix in `pacs-integration-service`) + +```ts +const lock = await pluginClient.storage.setIfAbsent( + `provision-lock:${visit.id}`, { at: Date.now() }, { ttlSeconds: 120 }, +); +if (lock.value === undefined) { + return res.sendIgnored('Provisioning already in progress'); // app-ext: poll getVisitor, then render +} +// sole writer for this visit.id — safe to create +let userId = await pluginClient.getVisitor(visit); +if (!userId) userId = await pluginClient.createVisitor(visit); +// permanent idempotency marker stays `visit:{id}`; the lock only guards the create window. +``` + +The permanent idempotency key remains `visit:{visit.id}`. The claim only serializes the create +window; once the record is written, `getVisitor` short-circuits all future calls. + +## TTL follow-up (optional, recommended for locks) + +Without an expiry, a holder that crashes **after** claiming but **before** writing the durable record +wedges the key. Options: + +1. **Consumer-side release** — `unset` the lock in a `finally`. Covers everything except hard pod + death, which the "recoverable create" pattern (persist the external id immediately after the first + external write) already de-fangs by making a re-create resumable rather than duplicative. +2. **Server-side TTL** — add `expires_at` to `plugin_storage_items`, have `set_if_absent` treat an + expired row as absent (delete-then-insert within the rescue, or a partial-unique-index + + sweeper). This makes the lock self-healing and is the clean long-term answer, at the cost of a + migration. + +The core PR ships option (1)-compatible behavior (no schema change); (2) can follow once the owning +team weighs the migration. + +## Rollout + +1. Land the platform `set_if_absent` action in `envoy-web` (backward-compatible; new action only). +2. Release the SDK with `setIfAbsent` (additive; no breaking changes). +3. Adopt in `pacs-integration-service` for BTB provisioning; other integrations can use it for any + claim/mutex need. + +Steps 1 and 2 are independent and safe to land in either order; the SDK method is inert until the +platform understands the action. diff --git a/src/base/EnvoyPluginStoragePipeline.ts b/src/base/EnvoyPluginStoragePipeline.ts index e150d0d..478ddf3 100644 --- a/src/base/EnvoyPluginStoragePipeline.ts +++ b/src/base/EnvoyPluginStoragePipeline.ts @@ -58,6 +58,17 @@ export default class EnvoyPluginStoragePipeline { return this.addCommand({ action: 'set', key, value }); } + /** + * Atomically sets a value for a storage item only if the key is not already set. + * Resolved server-side against a unique index, so it is race-free across concurrent + * invocations and pods. The result is the item when this call wrote it (claim won), + * or null when the key already held a value (claim lost). Pass ttlSeconds to give the + * write an expiry, so a crashed holder cannot wedge the key. + */ + setIfAbsent(key: string, value: unknown, options: { ttlSeconds?: number } = {}): EnvoyPluginStoragePipeline { + return this.addCommand({ action: 'set_if_absent', key, value, ...options }); + } + /** * Sets a unique value for a storage item, * and returns that item. diff --git a/src/internal/EnvoyStorageCommand.ts b/src/internal/EnvoyStorageCommand.ts index ce92a6f..30dd62e 100644 --- a/src/internal/EnvoyStorageCommand.ts +++ b/src/internal/EnvoyStorageCommand.ts @@ -17,7 +17,7 @@ export interface EnvoyStorageSetUniqueNumOptions { } export interface EnvoyBaseStorageCommand { - action: 'get' | 'set' | 'set_unique' | 'set_unique_num' | 'unset'; + action: 'get' | 'set' | 'set_if_absent' | 'set_unique' | 'set_unique_num' | 'unset'; key: string; } @@ -30,6 +30,12 @@ export interface EnvoySetStorageCommand extends EnvoyBaseStorageCommand { value: unknown; } +export interface EnvoySetIfAbsentStorageCommand extends EnvoyBaseStorageCommand { + action: 'set_if_absent'; + value: unknown; + ttlSeconds?: number; +} + export interface EnvoySetUniqueStorageCommand extends EnvoyBaseStorageCommand, EnvoyStorageSetUniqueOptions { action: 'set_unique'; } @@ -53,6 +59,7 @@ export interface EnvoyListStorageCommand { type EnvoyStorageCommand = | EnvoyGetStorageCommand | EnvoySetStorageCommand + | EnvoySetIfAbsentStorageCommand | EnvoySetUniqueStorageCommand | EnvoySetUniqueNumStorageCommand | EnvoyUnsetStorageCommand diff --git a/src/mocks/EnvoyPluginStoragePipelineMock.ts b/src/mocks/EnvoyPluginStoragePipelineMock.ts index 9b408f2..b43db40 100644 --- a/src/mocks/EnvoyPluginStoragePipelineMock.ts +++ b/src/mocks/EnvoyPluginStoragePipelineMock.ts @@ -37,6 +37,13 @@ export default class EnvoyPluginStoragePipelineMock extends EnvoyPluginStoragePi const value = EnvoyPluginStoragePipelineMock.set(command.key, command.value, isGlobal); return EnvoyPluginStoragePipelineMock.itemFromKeyValue(command.key, value); } + case 'set_if_absent': { + const written = EnvoyPluginStoragePipelineMock.setIfAbsent(command.key, command.value, isGlobal); + if (written === null) { + return null; // key already existed → claim lost + } + return EnvoyPluginStoragePipelineMock.itemFromKeyValue(command.key, written); + } case 'set_unique': try { const value = EnvoyPluginStoragePipelineMock.setUnique( @@ -109,6 +116,21 @@ export default class EnvoyPluginStoragePipelineMock extends EnvoyPluginStoragePi return value; } + // + // Writes only when the key is absent. Returns the value on a win, or null when the key + // already holds a value (claim lost). The real backend enforces this atomically via a + // unique index; the mock is single-threaded so a plain existence check is equivalent. + // TTL is not modelled in the mock. + // + static setIfAbsent(key: string, value: Value, isGlobal = false): Value | null { + key = EnvoyPluginStoragePipelineMock.normalizeKey(key, isGlobal); + if (Object.keys(EnvoyPluginStoragePipelineMock.storage).includes(key)) { + return null; + } + EnvoyPluginStoragePipelineMock.storage[key] = value; + return value; + } + static setUnique(key: string, options = DEFAULT_UNIQUE_OPTIONS, isGlobal = false) { key = EnvoyPluginStoragePipelineMock.normalizeKey(key, isGlobal); const chars = options.chars && options.chars.length ? options.chars : UNIQUE_OPTIONS_DEFAULT_CHARS; diff --git a/src/sdk/EnvoyPluginStorage.ts b/src/sdk/EnvoyPluginStorage.ts index 2b0bd93..d806cc1 100644 --- a/src/sdk/EnvoyPluginStorage.ts +++ b/src/sdk/EnvoyPluginStorage.ts @@ -45,6 +45,18 @@ export default class EnvoyPluginStorage { return this.pipeline().set(key, value).executeSingle>(); } + /** + * Atomically sets a single {@link EnvoyStorageItem} only if the key is not already set. + * + * Wrapper for single pipeline setIfAbsent. Resolves to the stored item when this call + * won the write, or { value: undefined } when the key already existed. + */ + setIfAbsent(key: string, value: Value, options: { ttlSeconds?: number } = {}) { + return this.pipeline() + .setIfAbsent(key, value, options) + .executeSingle | { key: string; value: undefined }>(); + } + /** * Sets a single unique string {@link EnvoyStorageItem} from storage. * diff --git a/test/mocks/EnvoyPluginStoragePipelineMock.test.ts b/test/mocks/EnvoyPluginStoragePipelineMock.test.ts new file mode 100644 index 0000000..72c705b --- /dev/null +++ b/test/mocks/EnvoyPluginStoragePipelineMock.test.ts @@ -0,0 +1,35 @@ +import EnvoyPluginStoragePipelineMock from '../../src/mocks/EnvoyPluginStoragePipelineMock'; +import EnvoyPluginAPI from '../../src/sdk/EnvoyPluginAPI'; + +// execute() is overridden in the mock and never touches the API, so a bare stub is enough. +const fakeApi = {} as EnvoyPluginAPI; +const pipeline = () => new EnvoyPluginStoragePipelineMock(fakeApi, 'install-1'); + +describe('EnvoyPluginStoragePipelineMock setIfAbsent', () => { + beforeEach(() => EnvoyPluginStoragePipelineMock.reset()); + + it('writes and returns the item when the key is absent (claim won)', async () => { + const result = await pipeline().setIfAbsent('lock:a', { at: 1 }).executeSingle(); + expect(result).toEqual({ key: 'lock:a', value: { at: 1 } }); + }); + + it('returns null when the key already exists (claim lost)', async () => { + await pipeline().setIfAbsent('lock:a', { at: 1 }).executeSingle(); + const second = await pipeline().setIfAbsent('lock:a', { at: 2 }).executeSingle(); + expect(second).toBeNull(); + }); + + it('does not overwrite the existing value on a lost claim', async () => { + await pipeline().setIfAbsent('lock:a', { at: 1 }).executeSingle(); + await pipeline().setIfAbsent('lock:a', { at: 2 }).executeSingle(); + const current = await pipeline().get('lock:a').executeSingle(); + expect(current).toEqual({ key: 'lock:a', value: { at: 1 } }); + }); + + it('lets the key be reclaimed after it is unset (lock release)', async () => { + await pipeline().setIfAbsent('lock:a', { at: 1 }).executeSingle(); + await pipeline().unset('lock:a').executeSingle(); + const reclaim = await pipeline().setIfAbsent('lock:a', { at: 3 }).executeSingle(); + expect(reclaim).toEqual({ key: 'lock:a', value: { at: 3 } }); + }); +});