|
| 1 | +--- |
| 2 | +"@objectstack/service-automation": patch |
| 3 | +--- |
| 4 | + |
| 5 | +fix(service-automation): evict a suspension consumed by another replica, so the run listings stop reporting phantoms (#15832) |
| 6 | + |
| 7 | +`AutomationEngine` had exactly one eviction site for its `suspendedRuns` |
| 8 | +map, inside `forgetSuspendedRun` — and that runs in whichever process |
| 9 | +**consumes** the suspension. In a multi-replica deployment that is routinely |
| 10 | +not the process that parked it: replica A parks a run, replica B resumes it, |
| 11 | +and nothing ever removes A's entry. There is no invalidation channel from B |
| 12 | +to A. |
| 13 | + |
| 14 | +The card that found this located the leak on `resumeInternal`'s |
| 15 | +`claim.kind === 'lost'` branch, which returns before that choke point. That |
| 16 | +branch does leak, but it is not the common shape: the **no-race** variant |
| 17 | +leaks identically — A parks, only B ever resumes, A never attempts a claim |
| 18 | +and there is no `'lost'` anywhere in the sequence — so an eviction hung on |
| 19 | +`'lost'` alone would have left the ordinary deployment untouched. |
| 20 | + |
| 21 | +The retained snapshot was **not only memory**. Two readers handed it back: |
| 22 | +`listSuspendedRuns()` (synchronous, cache-only, and the one listing on the |
| 23 | +`AutomationService` spec contract) and `listSuspendedRunsDurable()` (which |
| 24 | +deliberately appends map entries the durable list lacks). Once the other |
| 25 | +replica **completed** the run, both reported a phantom — a finished run |
| 26 | +listed as suspended, whose `getSuspendedScreen()` answers `null`, so a |
| 27 | +consumer that listed and then opened got an entry it could not act on. |
| 28 | + |
| 29 | +An entry is now dropped whenever this process holds a store-authoritative, |
| 30 | +per-id "no row" answer for it: the strict loader's store miss (which reaches |
| 31 | +`resume`, `hasSuspendedRun`, `cancelRun` and `getSuspendedScreen`), a lost |
| 32 | +advance claim, and a bounded per-id reconcile for the map-only entries of |
| 33 | +`listSuspendedRunsDurable()`. |
| 34 | + |
| 35 | +**Nothing here moves the cache-only listing's contract.** The fix only ever |
| 36 | +*removes* entries. The spec says `listSuspendedRuns()` lists "the currently |
| 37 | +suspended (paused) runs awaiting a resume"; the engine's own docblock adds |
| 38 | +only that it may OMIT runs (those parked in a previous process lifetime), |
| 39 | +because it reads the cache alone. Under-reporting is therefore already |
| 40 | +inside the declared latitude, and over-reporting was never inside the |
| 41 | +promise. Neither listing becomes store-backed, and `listSuspendedRuns()` |
| 42 | +stays synchronous. |
| 43 | + |
| 44 | +Three shapes are deliberately **never** evicted, each pinned by a control: |
| 45 | +no store attached (the map IS the authority); a run whose durable save |
| 46 | +failed (`cacheOnlySuspensions` — the store was never handed the row, so its |
| 47 | +silence says nothing about it); and a store read that THROWS (an outage |
| 48 | +means the run's existence is unknown, not gone). A failed `list()` |
| 49 | +enumeration likewise triggers no per-id reconcile — during an outage that |
| 50 | +would ask about every live run in the process. |
| 51 | + |
| 52 | +**Residual, stated rather than implied.** Eviction is demand-driven: a |
| 53 | +phantom is cleared when this process next obtains the per-id answer for that |
| 54 | +run — any `resume` / `hasSuspendedRun` / `getSuspendedScreen`, or a |
| 55 | +`listSuspendedRunsDurable()` reconcile. A process that never looks at the |
| 56 | +run again keeps the entry until it does. With no invalidation channel |
| 57 | +between replicas, closing that last gap needs either a background sweep or a |
| 58 | +store-backed listing, and both are decisions above this change; the boundary |
| 59 | +is pinned by a `RESIDUAL` test rather than left to be discovered. |
| 60 | + |
| 61 | +Note 2 of the same card — the `'unsupported'` branch deciding on the shape of |
| 62 | +a value the conditional delete has **already** been issued to obtain — is |
| 63 | +**not** addressed here: its honest fix is a declared return contract for the |
| 64 | +engine's multi-row delete, which lands in another package. |
0 commit comments