|
| 1 | +# Design: atomic `set_if_absent` for plugin storage |
| 2 | + |
| 3 | +**Status:** Draft / proposal |
| 4 | +**Author:** (via QA harness review of `pacs-integration-service#202`) |
| 5 | +**Affected repos:** `envoy-integrations-sdk-nodejs` (client), `envoy-web` (platform storage service) |
| 6 | + |
| 7 | +## Summary |
| 8 | + |
| 9 | +Add a race-free "write only if the key is not already set" primitive to plugin storage: |
| 10 | + |
| 11 | +- **SDK:** `storage.setIfAbsent(key, value, { ttlSeconds? })` and a matching pipeline command |
| 12 | + `set_if_absent`. |
| 13 | +- **Platform (`envoy-web`):** a new `set_if_absent` action in `Platform::PluginStorageService` |
| 14 | + that `INSERT`s and relies on the existing unique index — the DB row *is* the lock. Returns the |
| 15 | + item on a win, `null` when the key is already held. |
| 16 | + |
| 17 | +The result: exactly one of N concurrent invocations (across pods) wins the write. This gives |
| 18 | +integrations a correct mutual-exclusion / claim primitive that today's API cannot express. |
| 19 | + |
| 20 | +## Motivation |
| 21 | + |
| 22 | +`pacs-integration-service#202` (By The Bay / Habitap) provisions a visitor credential from two |
| 23 | +independent code paths that can run concurrently for the same visit: |
| 24 | + |
| 25 | +1. the `invite_created` webhook (for near-term invites), and |
| 26 | +2. the `registration_complete_email` **app-extension render** (create-if-missing). |
| 27 | + |
| 28 | +Both do `getVisitor → (null) → createVisitor`. `getVisitor` is a *read*; the record that would make |
| 29 | +it idempotent is only written **after** the external `POST`s complete. So two invocations that read |
| 30 | +during that window both see "missing" and both create — producing **duplicate Habitap events**, and |
| 31 | +because both write the same storage key afterward, the first event is orphaned (storage no longer |
| 32 | +references it). The two paths often fire at the same instant (invite creation) and may land on |
| 33 | +**different pods**, so in-process de-duplication cannot help. |
| 34 | + |
| 35 | +This is the classic check-then-act (TOCTOU) race. It cannot be fixed with a better *read*: the |
| 36 | +guarantee has to live on the *write*. |
| 37 | + |
| 38 | +## Why existing primitives don't solve it |
| 39 | + |
| 40 | +The full storage surface today is `get`, `set`, `set_unique`, `set_unique_num`, `unset`, `list` |
| 41 | +(confirmed in SDK `2.5.2` and `envoy-web`'s `PluginStorageService`). |
| 42 | + |
| 43 | +- `set` is an **unconditional** overwrite (last-write-wins) — no contention signal. |
| 44 | +- `set_unique` / `set_unique_num` guarantee a unique *generated value* and **overwrite the key**. |
| 45 | + The uniqueness is on the value, not on key ownership, so they cannot elect a single winner for a |
| 46 | + known key. |
| 47 | +- A user-land `get`-then-`set` "lock" reintroduces the exact TOCTOU race and is not cross-pod safe. |
| 48 | +- There is no `ttl`/`expire`, and no lock/mutex utility anywhere in the SDK. |
| 49 | + |
| 50 | +Importantly, the backend **already performs atomic conditional writes**: `set_unique` depends on a |
| 51 | +`RecordNotUnique` rescue against a unique index (`PluginStorageUniqValue`), and `plugin_storage_items` |
| 52 | +already has unique indexes on `(key, plugin_install_id) WHERE archived_at IS NULL` and |
| 53 | +`(plugin_id, key)`. `set_if_absent` is a *simpler* use of the same mechanism. |
| 54 | + |
| 55 | +## Proposed API (SDK) |
| 56 | + |
| 57 | +```ts |
| 58 | +// EnvoyPluginStorage |
| 59 | +setIfAbsent<Value>(key: string, value: Value, options?: { ttlSeconds?: number }): |
| 60 | + Promise<EnvoyStorageItem<Value> | { key: string; value: undefined }>; |
| 61 | +``` |
| 62 | + |
| 63 | +- Resolves to the **stored item** when this call wrote it (claim won). |
| 64 | +- Resolves to `{ key, value: undefined }` when the key already existed (claim lost) — mirroring the |
| 65 | + existing `get`-miss / `setUnique`-exhaustion convention, so no new result shape is introduced. |
| 66 | +- `ttlSeconds` (optional) gives the write an expiry so a crashed holder cannot wedge the key. |
| 67 | + |
| 68 | +Pipeline form, for batching a claim with a follow-up read in one round-trip: |
| 69 | + |
| 70 | +```ts |
| 71 | +storage.pipeline() |
| 72 | + .setIfAbsent(`provision-lock:${visitId}`, { at: now }, { ttlSeconds: 120 }) |
| 73 | + .get(`visit:${visitId}`) |
| 74 | + .execute(); |
| 75 | +``` |
| 76 | + |
| 77 | +## Wire protocol (`POST /api/v2/plugin-services/storage`) |
| 78 | + |
| 79 | +**Request** (claim): |
| 80 | + |
| 81 | +```json |
| 82 | +{ |
| 83 | + "install_id": "inst_abc123", |
| 84 | + "commands": [ |
| 85 | + { |
| 86 | + "action": "set_if_absent", |
| 87 | + "key": "provision-lock:invite-222", |
| 88 | + "value": { "claimedAt": 1720370000000 }, |
| 89 | + "ttlSeconds": 120 |
| 90 | + } |
| 91 | + ] |
| 92 | +} |
| 93 | +``` |
| 94 | + |
| 95 | +**Response — won** (server wrote the row): |
| 96 | + |
| 97 | +```json |
| 98 | +{ "data": [ { "key": "provision-lock:invite-222", "value": { "claimedAt": 1720370000000 } } ] } |
| 99 | +``` |
| 100 | + |
| 101 | +**Response — lost** (key already held): |
| 102 | + |
| 103 | +```json |
| 104 | +{ "data": [ null ] } |
| 105 | +``` |
| 106 | + |
| 107 | +No controller or strong-params changes are required: `StorageController#pipeline` passes command |
| 108 | +hashes through untouched, and results serialize exactly like `get`/`set` (item or `null`). |
| 109 | + |
| 110 | +## Platform implementation (`envoy-web`) |
| 111 | + |
| 112 | +Add one dispatch arm and one method to `Platform::PluginStorageService`. Unlike `set`, it must **not** |
| 113 | +`unset` first — the whole point is to fail when the key exists: |
| 114 | + |
| 115 | +```ruby |
| 116 | +when 'set_if_absent' |
| 117 | + do_storage_item_set_if_absent(command[:key], command[:value]) |
| 118 | +``` |
| 119 | + |
| 120 | +```ruby |
| 121 | +# Atomically create an item only if the key is not already set for this scope. |
| 122 | +# Relies on the unique index on (key, plugin_install_id) / (plugin_id, key): the INSERT either |
| 123 | +# wins or raises RecordNotUnique, which we treat as "already claimed" and return nil. Race-free |
| 124 | +# across concurrent requests and pods without an explicit lock — the DB is the lock. Unlike `set`, |
| 125 | +# it never overwrites an existing value. |
| 126 | +def do_storage_item_set_if_absent(key, value = nil) |
| 127 | + plugin_storage_items.create!({ key: key, value: value }) |
| 128 | +rescue ::ActiveRecord::RecordNotUnique, ::ActiveRecord::RecordInvalid |
| 129 | + nil |
| 130 | +end |
| 131 | +``` |
| 132 | + |
| 133 | +This is correct on day one **without TTL**; TTL is a follow-up (below). |
| 134 | + |
| 135 | +## Consumer usage (the fix in `pacs-integration-service`) |
| 136 | + |
| 137 | +```ts |
| 138 | +const lock = await pluginClient.storage.setIfAbsent( |
| 139 | + `provision-lock:${visit.id}`, { at: Date.now() }, { ttlSeconds: 120 }, |
| 140 | +); |
| 141 | +if (lock.value === undefined) { |
| 142 | + return res.sendIgnored('Provisioning already in progress'); // app-ext: poll getVisitor, then render |
| 143 | +} |
| 144 | +// sole writer for this visit.id — safe to create |
| 145 | +let userId = await pluginClient.getVisitor(visit); |
| 146 | +if (!userId) userId = await pluginClient.createVisitor(visit); |
| 147 | +// permanent idempotency marker stays `visit:{id}`; the lock only guards the create window. |
| 148 | +``` |
| 149 | + |
| 150 | +The permanent idempotency key remains `visit:{visit.id}`. The claim only serializes the create |
| 151 | +window; once the record is written, `getVisitor` short-circuits all future calls. |
| 152 | + |
| 153 | +## TTL follow-up (optional, recommended for locks) |
| 154 | + |
| 155 | +Without an expiry, a holder that crashes **after** claiming but **before** writing the durable record |
| 156 | +wedges the key. Options: |
| 157 | + |
| 158 | +1. **Consumer-side release** — `unset` the lock in a `finally`. Covers everything except hard pod |
| 159 | + death, which the "recoverable create" pattern (persist the external id immediately after the first |
| 160 | + external write) already de-fangs by making a re-create resumable rather than duplicative. |
| 161 | +2. **Server-side TTL** — add `expires_at` to `plugin_storage_items`, have `set_if_absent` treat an |
| 162 | + expired row as absent (delete-then-insert within the rescue, or a partial-unique-index + |
| 163 | + sweeper). This makes the lock self-healing and is the clean long-term answer, at the cost of a |
| 164 | + migration. |
| 165 | + |
| 166 | +The core PR ships option (1)-compatible behavior (no schema change); (2) can follow once the owning |
| 167 | +team weighs the migration. |
| 168 | + |
| 169 | +## Rollout |
| 170 | + |
| 171 | +1. Land the platform `set_if_absent` action in `envoy-web` (backward-compatible; new action only). |
| 172 | +2. Release the SDK with `setIfAbsent` (additive; no breaking changes). |
| 173 | +3. Adopt in `pacs-integration-service` for BTB provisioning; other integrations can use it for any |
| 174 | + claim/mutex need. |
| 175 | + |
| 176 | +Steps 1 and 2 are independent and safe to land in either order; the SDK method is inert until the |
| 177 | +platform understands the action. |
0 commit comments