|
| 1 | +# ADR-007: Cross-container serialization for the dynamic-allocation absolute-set race |
| 2 | + |
| 3 | +## Context |
| 4 | + |
| 5 | +The dynamic (JIT) Semrush AI resource allocator (PR #2764, `SERENITY_DYNAMIC_ALLOCATION`, default |
| 6 | +OFF) does an **absolute set** of a sub-workspace's resource `total` — it reads the child's current |
| 7 | +totals, computes a new one, and transfers it (`ensureAiHeadroom` and `releaseAiSurplus`, both in |
| 8 | +`src/support/serenity/resource-manager.js`). Two operations that read the same child concurrently |
| 9 | +and then each write an absolute value can clobber one another: the later write wins with a value |
| 10 | +computed from a now-stale read. |
| 11 | + |
| 12 | +`src/support/serenity/resource-lock.js` (`withResourceLock`) serializes same-child mutations via an |
| 13 | +in-process promise chain with a `LOCK_TIMEOUT_MS` (10s) safety valve. This PR (LLMO-6191) also wraps |
| 14 | +`releaseAiSurplus`'s one production call site (`markets-subworkspace.js`, the model-update seam) in |
| 15 | +the same lock, so **both** `ensureAiHeadroom` and `releaseAiSurplus` now serialize against each other |
| 16 | +for the same child, within one warm Lambda container. That closes the same-container half of the |
| 17 | +race. Across separate warm containers — the normal case for a Lambda-per-request API service — there |
| 18 | +is **no serialization at all**, so the race is fully open cross-container. This ADR is about closing |
| 19 | +that remaining gap (or deciding, explicitly, not to yet). |
| 20 | + |
| 21 | +Constraints on the options below: no new cloud infrastructure may be provisioned unilaterally by this |
| 22 | +PR (no new DynamoDB table, no new Redis/ElastiCache instance — see the LLMO-6191 scope guidance). |
| 23 | +This repo has **zero existing DynamoDB usage** (no `@aws-sdk/client-dynamodb` dependency) and no |
| 24 | +distributed-lock/conditional-write primitive was found in `@adobe/spacecat-shared` either. |
| 25 | + |
| 26 | +### Corrected failure-mode analysis (staff-engineer review) |
| 27 | + |
| 28 | +The absolute-set mechanic that CAUSES the race is also what BOUNDS it — this matters for how risky |
| 29 | +"do nothing yet" actually is: |
| 30 | + |
| 31 | +- **ensure-vs-ensure** (two concurrent top-ups on the same child): each call computes its own |
| 32 | + `target = roundUpToBlock(...)`, ceiling-clamped (`resource-manager.js`, `ensureAiHeadroom`). The |
| 33 | + last writer wins with **its own legitimate, ceiling-bounded target** — the final total is never |
| 34 | + higher than `max(target_a, target_b)`. No ceiling breach, no unbounded over-write, no double-charge |
| 35 | + of the parent pool. Worst case: the loser's top-up is silently absorbed and the next metered op |
| 36 | + that finds itself still short just re-triggers `ensureAiHeadroom` — self-healing. |
| 37 | +- **ensure-vs-release** (an `ensureAiHeadroom` topping up a child racing a `releaseAiSurplus` lowering |
| 38 | + the same child — now closed IN-PROCESS by this PR's lock wrap, but still open cross-container): a |
| 39 | + stale `release` writing last can set `total` below the now-real `used` (a transient |
| 40 | + **oversubscribed child**, rejecting the in-flight metered write until the next `ensure` heals it — |
| 41 | + not silent data loss, but not "just re-tops-up" either). A stale `ensure` writing last after a |
| 42 | + `release` already lowered the child can draw more from the master than its own advisory pool check |
| 43 | + computed (bounded only by the master's terminal `422`, not by the advisory read). |
| 44 | + |
| 45 | +Neither interleaving causes data loss or a permanent ceiling breach. The corrected framing: **worst |
| 46 | +case is a transient oversubscription/over-draw window, self-healing on the next `ensureAiHeadroom`, |
| 47 | +never corruption or a stuck permanent state.** |
| 48 | + |
| 49 | +## Decision |
| 50 | + |
| 51 | +**Adopt Option C (accept the current risk band, observe, revisit on signal) for the initial ON |
| 52 | +rollout — conditional on the LLMO-6191 item-2 observability landing first**, not as an independent |
| 53 | +decision made in isolation. `TopUpLatencyMs`, `PoolFreeRatio`, and the `AllocationRejection`/ |
| 54 | +`ReleaseOutcome` metrics (`src/support/serenity/allocation-metrics.js`) are the concrete signal this |
| 55 | +ADR's "revisit if it manifests" promise depends on — this status is NOT "Accepted, ship and forget." |
| 56 | + |
| 57 | +Options considered: |
| 58 | + |
| 59 | +**A — DynamoDB conditional write (new infra).** A lock-holder row per child workspace id, written |
| 60 | +with a `ConditionExpression` (only succeeds if unheld or the holder's TTL expired), TTL as the |
| 61 | +auto-expiry safety valve mirroring `LOCK_TIMEOUT_MS`. Correct, well-understood pattern. Cost: a new |
| 62 | +table, IAM policy, and an operational surface (TTL tuning, hot-partition risk on a busy child) this |
| 63 | +repo does not have today. Requires provisioning review — out of scope for this PR per the ticket's |
| 64 | +scope guidance. |
| 65 | + |
| 66 | +**B — Push the fix upstream (Semrush optimistic concurrency).** If the Semrush transfer API accepted |
| 67 | +an `expected_total` (compare-and-swap) parameter, a stale writer would get a rejection instead of |
| 68 | +silently clobbering, and no distributed lock would be needed at all — the upstream call becomes |
| 69 | +self-serializing. **Unverified**: whether the live gateway supports this is an open question, not |
| 70 | +confirmed against the tenant. If it does, this is the strongest option (no new infra, and it fixes |
| 71 | +the root cause rather than working around it). Action item: check with the Semrush/Project-Engine |
| 72 | +API owners before ruling this out permanently. |
| 73 | + |
| 74 | +**C — Accept + observe (recommended for now).** Ship with the current risk band: same-container race |
| 75 | +closed (this PR), cross-container race open but bounded to a transient, self-healing oversubscription |
| 76 | +window (never corruption, never a permanent ceiling breach — see the analysis above). Watch the |
| 77 | +item-2 telemetry (`TopUpLatencyMs`, `PoolFreeRatio`, `AllocationRejection`, `ReleaseOutcome`) for |
| 78 | +repeated-oversubscription symptoms (e.g. a `workspaceBusy` spike shortly after a top-up, or a |
| 79 | +`ReleaseOutcome` pattern suggesting a release clobbered a subsequent ensure) as the live signal of |
| 80 | +whether this actually manifests at current traffic. Revisit with Option A, B, or D if it does. |
| 81 | + |
| 82 | +**D — SQS FIFO with `MessageGroupId = childWorkspaceId` (SRE review addition).** This repo already |
| 83 | +speaks SQS (`src/support/sqs.js`); a FIFO queue's per-group ordering IS a cross-container |
| 84 | +serialization primitive without provisioning new infrastructure CLASS (an existing AWS primitive, |
| 85 | +not a new one). It would reframe the hot path from synchronous-locked to enqueue-and-settle, which |
| 86 | +also retires the fail-fast/504 tension `ensureAiHeadroom`'s hot path currently has to work around. |
| 87 | +Real costs: async top-up latency (the request would have to wait for a queue round-trip, or the |
| 88 | +metered write would have to happen optimistically before the top-up is confirmed — a different |
| 89 | +correctness contract than today's), a new consumer worker, and reordering the synchronous gate |
| 90 | +contract every metered handler currently relies on (`createHeadroomGuard.ensure` is awaited |
| 91 | +in-request today). Worth a real look if Option C's telemetry shows the race manifesting, but a bigger |
| 92 | +shape change than A or B. |
| 93 | + |
| 94 | +**E — Single-flight coalescing (SRE review addition, NOT cross-container).** Not a serialization |
| 95 | +primitive on its own — de-duplicating concurrent identical top-up requests within a container reduces |
| 96 | +contention but does not touch the cross-container case. Cheap, but only a partial mitigation; already |
| 97 | +effectively achieved in-container by `withResourceLock`. |
| 98 | + |
| 99 | +## Status |
| 100 | + |
| 101 | +Accepted, contingent on the item-2 observability metrics being live and watched (see LLMO-6191 PR). |
| 102 | +In-PR mitigation shipped now: `releaseAiSurplus`'s one production call site is wrapped in |
| 103 | +`withResourceLock` alongside `ensureAiHeadroom`, closing the same-container ensure/release race — |
| 104 | +see `src/support/serenity/handlers/markets-subworkspace.js`. The cross-container gap remains open, |
| 105 | +by design, per Option C above. Revisit this ADR (promote to Option A, B, or D) if the item-2 signals |
| 106 | +show the race manifesting at real traffic, or if the Semrush API owners confirm Option B is |
| 107 | +available — check that before committing engineering time to Option A's new infrastructure. |
0 commit comments