diff --git a/.changeset/service-jsdoc-declared-equals-actual.md b/.changeset/service-jsdoc-declared-equals-actual.md new file mode 100644 index 0000000000..ed69200947 --- /dev/null +++ b/.changeset/service-jsdoc-declared-equals-actual.md @@ -0,0 +1,52 @@ +--- +"@objectstack/service-cache": patch +"@objectstack/service-job": patch +--- + +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) + +Both comments are emitted by `tsup` into each package's built `index.d.ts`, so they +are **the editor tooltip an npm consumer sees** — the same "published documentation +asserting behaviour the runtime does not have" class as #9517 and #9532, in a third +channel that no gate reads. No runtime behaviour changes in either package; this is a +patch because the corrected text only reaches consumers through a release. + +**1. `MemoryCacheAdapter` — "LRU-style eviction" was never LRU.** + +The class comment advertised `TTL-based expiry and LRU-style eviction`. The eviction +path takes `this.store.keys().next().value` — the first key in `Map` insertion order — +and `get()` returns `entry.value` without ever deleting and re-setting the key, so a +read does not move an entry back. Nor does an overwrite: `Map.set` on a key already +present keeps its original insertion slot. Eviction has therefore always been +**oldest-inserted (FIFO)**, which is a materially different hit-rate profile from the +one the tooltip promised anyone sizing a cache. + +The comment was corrected rather than the code, deliberately: `maxSize` defaults to `0` +(unlimited), so the eviction path is off by default and nothing shipped is getting FIFO +where it expected LRU, and there is no measured pull for LRU. Minting a real behaviour +change to make a stale sentence true inverts the fix — the defect is that the +documentation lies, not that the cache is wrong. (A real LRU already exists in the repo, +`packages/metadata/src/utils/lru-cache.ts`, for the callers that need one.) + +Four tests now pin the corrected sentence so it stops being an unenforced claim. Each is +written as a **discriminator against LRU**: it reads (or overwrites) the oldest entry +before overflowing the cache and then asserts that entry was evicted anyway — a hot key +dies on age, an untouched newer key survives. The pre-existing eviction tests could not +tell the two policies apart, which is how the wrong comment sat green. + +**2. `DbJobAdapterOptions.recordRuns` — the comment described a different field.** + +`/** Soft cap on sys_job_run rows recorded per job (defaults to none — handled by +retention jobs) */` made three claims and the code contradicts all three: the field is a +`boolean`, not a count; it defaults to `true`, not "none" (`args.options?.recordRuns ?? +true`); and it gates whether a `sys_job_run` row is written at all, rather than being +trimmed later by retention. The sentence reads as if it belongs to the numeric +`JobRunRetention` knob that ADR-0057 retired — a copy-paste that outlived its source. + +The consequence the new wording keeps in sight: **a reader who sets `recordRuns: false` +expecting "no cap" gets run history switched off.** The replacement states the real +meaning (one row per attempt, inserted at start and updated on settle, default `true`) +and both things that are *not* affected by the flag — the `sys_job` row's own +`last_status` / `run_count` / `failure_count` counters, which `bumpJob` updates +regardless, and `replay()`, which writes its synthetic `trigger: 'replay'` row without +consulting the flag at all. diff --git a/packages/services/service-cache/src/memory-cache-adapter.test.ts b/packages/services/service-cache/src/memory-cache-adapter.test.ts index 796dea8178..fbdac10f1d 100644 --- a/packages/services/service-cache/src/memory-cache-adapter.test.ts +++ b/packages/services/service-cache/src/memory-cache-adapter.test.ts @@ -105,3 +105,87 @@ describe('MemoryCacheAdapter', () => { expect(await cache.has('expiring')).toBe(false); }); }); + +// ─── eviction is insertion-order (FIFO), and the class JSDoc says so ───────── +// +// The class comment used to advertise "LRU-style eviction" while `get()` has +// never re-inserted the key it reads, so eviction has always been oldest- +// INSERTED, not least-recently-USED. The comment was corrected rather than the +// code; these tests are what stops that corrected sentence from being another +// unenforced claim. +// +// Every case below is written to be a DISCRIMINATOR: each one passes under the +// shipped FIFO store and fails under an LRU store, because each performs a +// read (or an overwrite) on the oldest entry and then asserts that the entry +// was evicted anyway. A test that merely fills the cache past `maxSize` without +// touching anything first cannot tell the two policies apart, which is how the +// pre-existing eviction tests above sat green over a comment they contradicted. +describe('MemoryCacheAdapter — insertion-order (FIFO) eviction', () => { + it('a read does NOT refresh eviction order — the read-hot oldest entry is still evicted', async () => { + const cache = new MemoryCacheAdapter({ maxSize: 2 }); + await cache.set('a', 1); + await cache.set('b', 2); + + // Read 'a' repeatedly: under LRU this promotes it to most-recently-used and + // makes 'b' the eviction candidate. Under FIFO it changes nothing at all. + expect(await cache.get('a')).toBe(1); + expect(await cache.get('a')).toBe(1); + expect(await cache.has('a')).toBe(true); + + await cache.set('c', 3); + + expect(await cache.has('a')).toBe(false); // oldest-inserted, evicted despite being hot + expect(await cache.get('b')).toBe(2); // untouched, survives — the inverse of LRU + expect(await cache.get('c')).toBe(3); + }); + + it('an overwrite does NOT refresh eviction order either — Map.set keeps the original slot', async () => { + const cache = new MemoryCacheAdapter({ maxSize: 2 }); + await cache.set('a', 1); + await cache.set('b', 2); + + // Overwriting an existing key never evicts (it is not a new key) and never + // moves the entry: `Map.set` on a present key keeps its insertion position. + await cache.set('a', 10); + expect(await cache.get('a')).toBe(10); + + await cache.set('c', 3); + + expect(await cache.has('a')).toBe(false); // freshly written, still the oldest slot + expect(await cache.get('b')).toBe(2); + expect(await cache.get('c')).toBe(3); + }); + + it('evicts strictly in insertion order across a longer run, reads notwithstanding', async () => { + const cache = new MemoryCacheAdapter({ maxSize: 3 }); + await cache.set('a', 1); + await cache.set('b', 2); + await cache.set('c', 3); + + // Make 'a' the hottest key in the cache and 'c' the coldest. + await cache.get('a'); + await cache.get('a'); + await cache.get('a'); + + await cache.set('d', 4); // evicts 'a' (first in), not 'c' (least recently used) + expect(await cache.has('a')).toBe(false); + expect(await cache.has('c')).toBe(true); + + await cache.set('e', 5); // evicts 'b' — the queue keeps advancing by age + expect(await cache.has('b')).toBe(false); + + expect(await cache.get('c')).toBe(3); + expect(await cache.get('d')).toBe(4); + expect(await cache.get('e')).toBe(5); + expect((await cache.stats()).keyCount).toBe(3); + }); + + it('leaves the eviction path off entirely at the default maxSize of 0 (unlimited)', async () => { + const cache = new MemoryCacheAdapter(); // maxSize defaults to 0 + for (let i = 0; i < 50; i++) await cache.set(`k${i}`, i); + + expect(await cache.get('k0')).toBe(0); // the very first key is still there + expect(await cache.get('k49')).toBe(49); + expect((await cache.stats()).keyCount).toBe(50); + }); +}); diff --git a/packages/services/service-cache/src/memory-cache-adapter.ts b/packages/services/service-cache/src/memory-cache-adapter.ts index 8713d7ab19..bb6dbe48b4 100644 --- a/packages/services/service-cache/src/memory-cache-adapter.ts +++ b/packages/services/service-cache/src/memory-cache-adapter.ts @@ -30,7 +30,13 @@ export interface MemoryCacheAdapterOptions { /** * In-memory cache adapter implementing ICacheService. * - * Uses a Map-backed store with TTL-based expiry and LRU-style eviction. + * Uses a Map-backed store with TTL-based expiry and **insertion-order (FIFO) + * eviction**: once `maxSize` is reached, a new key evicts the oldest-inserted + * entry. This is deliberately **not** LRU — neither reading an entry nor + * overwriting its value moves it back in the queue, so a hot key is evicted on + * age like any other (pinned by the FIFO tests in `memory-cache-adapter.test.ts`). + * `maxSize` defaults to `0` (unlimited), which leaves the eviction path off. + * * Suitable for single-process environments, development, and testing. */ export class MemoryCacheAdapter implements ICacheService { diff --git a/packages/services/service-job/src/db-job-adapter.ts b/packages/services/service-job/src/db-job-adapter.ts index 110e4df6c5..2f707493cb 100644 --- a/packages/services/service-job/src/db-job-adapter.ts +++ b/packages/services/service-job/src/db-job-adapter.ts @@ -31,7 +31,19 @@ export interface JobLoggerLike { export interface DbJobAdapterOptions { /** Maximum executions kept in memory per job (default 100) */ maxExecutions?: number; - /** Soft cap on sys_job_run rows recorded per job (defaults to none — handled by retention jobs) */ + /** + * Record each scheduled or triggered execution as a `sys_job_run` row — + * inserted at the start of every attempt and updated to its terminal status + * when that attempt settles. Default **`true`**. + * + * This is an on/off switch for run history, NOT a retention cap: setting it to + * `false` means no per-attempt rows are written at all, so `sys_job_run` holds + * nothing for these executions and `listExecutionsByStatus` has nothing to + * read. Two things are unaffected either way — the `sys_job` row's own + * `last_status` / `run_count` / `failure_count` counters, and + * {@link DbJobAdapter.replay}, which writes its synthetic `trigger: 'replay'` + * row regardless of this flag. + */ recordRuns?: boolean; }