|
| 1 | +--- |
| 2 | +"@objectstack/service-cache": patch |
| 3 | +"@objectstack/service-job": patch |
| 4 | +--- |
| 5 | + |
| 6 | +fix(services): two published `.d.ts` JSDoc comments stop describing behaviour their code does not have — `MemoryCacheAdapter` eviction is FIFO, not LRU, and `recordRuns` is an on/off switch, not a retention cap (#9611) |
| 7 | + |
| 8 | +Both comments are emitted by `tsup` into each package's built `index.d.ts`, so they |
| 9 | +are **the editor tooltip an npm consumer sees** — the same "published documentation |
| 10 | +asserting behaviour the runtime does not have" class as #9517 and #9532, in a third |
| 11 | +channel that no gate reads. No runtime behaviour changes in either package; this is a |
| 12 | +patch because the corrected text only reaches consumers through a release. |
| 13 | + |
| 14 | +**1. `MemoryCacheAdapter` — "LRU-style eviction" was never LRU.** |
| 15 | + |
| 16 | +The class comment advertised `TTL-based expiry and LRU-style eviction`. The eviction |
| 17 | +path takes `this.store.keys().next().value` — the first key in `Map` insertion order — |
| 18 | +and `get()` returns `entry.value` without ever deleting and re-setting the key, so a |
| 19 | +read does not move an entry back. Nor does an overwrite: `Map.set` on a key already |
| 20 | +present keeps its original insertion slot. Eviction has therefore always been |
| 21 | +**oldest-inserted (FIFO)**, which is a materially different hit-rate profile from the |
| 22 | +one the tooltip promised anyone sizing a cache. |
| 23 | + |
| 24 | +The comment was corrected rather than the code, deliberately: `maxSize` defaults to `0` |
| 25 | +(unlimited), so the eviction path is off by default and nothing shipped is getting FIFO |
| 26 | +where it expected LRU, and there is no measured pull for LRU. Minting a real behaviour |
| 27 | +change to make a stale sentence true inverts the fix — the defect is that the |
| 28 | +documentation lies, not that the cache is wrong. (A real LRU already exists in the repo, |
| 29 | +`packages/metadata/src/utils/lru-cache.ts`, for the callers that need one.) |
| 30 | + |
| 31 | +Four tests now pin the corrected sentence so it stops being an unenforced claim. Each is |
| 32 | +written as a **discriminator against LRU**: it reads (or overwrites) the oldest entry |
| 33 | +before overflowing the cache and then asserts that entry was evicted anyway — a hot key |
| 34 | +dies on age, an untouched newer key survives. The pre-existing eviction tests could not |
| 35 | +tell the two policies apart, which is how the wrong comment sat green. |
| 36 | + |
| 37 | +**2. `DbJobAdapterOptions.recordRuns` — the comment described a different field.** |
| 38 | + |
| 39 | +`/** Soft cap on sys_job_run rows recorded per job (defaults to none — handled by |
| 40 | +retention jobs) */` made three claims and the code contradicts all three: the field is a |
| 41 | +`boolean`, not a count; it defaults to `true`, not "none" (`args.options?.recordRuns ?? |
| 42 | +true`); and it gates whether a `sys_job_run` row is written at all, rather than being |
| 43 | +trimmed later by retention. The sentence reads as if it belongs to the numeric |
| 44 | +`JobRunRetention` knob that ADR-0057 retired — a copy-paste that outlived its source. |
| 45 | + |
| 46 | +The consequence the new wording keeps in sight: **a reader who sets `recordRuns: false` |
| 47 | +expecting "no cap" gets run history switched off.** The replacement states the real |
| 48 | +meaning (one row per attempt, inserted at start and updated on settle, default `true`) |
| 49 | +and both things that are *not* affected by the flag — the `sys_job` row's own |
| 50 | +`last_status` / `run_count` / `failure_count` counters, which `bumpJob` updates |
| 51 | +regardless, and `replay()`, which writes its synthetic `trigger: 'replay'` row without |
| 52 | +consulting the flag at all. |
0 commit comments