You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Commit 94aeb1f
Browse filesBrowse the repository at this point in the historyBrowse files
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
0 commit comments