diff --git a/.changeset/list-single-flight.md b/.changeset/list-single-flight.md new file mode 100644 index 0000000000..4aaa02320f --- /dev/null +++ b/.changeset/list-single-flight.md @@ -0,0 +1,55 @@ +--- +"@objectstack/metadata": patch +--- + +fix(metadata): `list()` reads are single-flight, so the "one loader hit per TTL window" promise finally holds for concurrent callers too (#5253) + +`MetadataManager.list()` was a bare "read the cache → walk the loaders → write +the cache" sequence. The cache is written only once a read has **finished**, so +it absorbed the caller that arrived second in *time* but never the caller that +arrived second in *flight*: every `list(type)` issued while the first read was +still walking the loaders missed, and each one walked every loader itself. The +`listCache` field comment states the guarantee the cache exists to provide — +"the loader is only hit once per TTL window" — and that guarantee held for +sequential callers only. + +That is not a rounding error on the path the cache was built for. The comment +names it: security/permission middleware calling `list('permission')` on the +request path while `DatabaseLoader`'s read sits inside a transaction that holds +SQLite's only connection, waiting out knex's `acquireConnectionTimeout` (60s). +Every concurrent request arriving during those 60s used to burn its own 60s, +because nothing had been written to the cache yet. The everyday version is +milder but constant: cold start, and the small burst of concurrent `list()` +calls that follows every invalidation point — `register()` / `unregister()`, a +cluster peer's write (#5109), a filesystem change (#5218) — each repeated the +full loader walk. + +Reads of one metadata type are now single-flight. A `list(type)` that finds a +read already running for that type joins it instead of starting a second +identical walk. + +- **Sharers share the outcome — as an explicit contract, not an accident.** + Every caller joining an in-flight read receives that read's exact result, + including when a loader was unreadable and the answer is known-partial. + `list()` is the best-effort listing seam and does not throw (the strict + counterparts remain `listForIndex()` and `loadDiagnosed()`), so a lost loader + is not an error to fail over from — it is the answer, and re-running the read + privately for a joiner would walk the same loaders against the same outage in + the same window. +- **#5184's degraded judgment is unchanged and is not bypassed.** A shared read + that lost a loader is still memoized `degraded: true` on the 2s TTL, never + laundered onto the 30s healthy TTL by having been shared, and every sharer + received that same partial set. +- **A write landing mid-read wins.** `invalidateListCache()` now retracts the + in-flight read as well as the finished entry. The retracted read keeps running + for the callers already waiting on it — they asked before the write — but it + loses the right to memoize its pre-write answer, so that answer cannot outlive + the write it predates; and a caller arriving after the write starts a fresh + read rather than joining a pre-write one. That second half is #5219 / #5229's + ordering bar restated for concurrency: a consumer woken by a metadata change + must not observe the event and pre-event state together. +- The in-flight map is self-cleaning — an entry is dropped when its read + settles, by that read only, so a fresh read that replaced it keeps its slot. + +Internal caching policy only — no change to the `IMetadataService` contract or to +any public export. Sequential callers behave exactly as before. diff --git a/packages/metadata/src/metadata-manager-list-single-flight.test.ts b/packages/metadata/src/metadata-manager-list-single-flight.test.ts new file mode 100644 index 0000000000..f260b90806 --- /dev/null +++ b/packages/metadata/src/metadata-manager-list-single-flight.test.ts @@ -0,0 +1,469 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5253 — `MetadataManager.list()` reads are single-flight per metadata type. + * + * `listCache` is written only once a read has FINISHED, so before #5253 it + * absorbed the caller that arrived second in *time* but never the caller that + * arrived second in *flight*: N `list('permission')` calls issued while the + * first one was still walking the loaders all missed and each walked every + * loader themselves. The `listCache` field comment promises "the loader is only + * hit once per TTL window"; that promise held for sequential callers only, and + * the scenario the comment is built for — security middleware calling + * `list('permission')` from inside a transaction while `DatabaseLoader` waits + * out knex's 60s `acquireConnectionTimeout` — is exactly where concurrency is + * most likely, i.e. 60s burned per caller instead of once for all of them. + * + * These tests pin the four halves of the fix: + * 1. the issue's repro (3 concurrent callers ⇒ 1 loader walk) and that + * sequential callers still behave per-TTL exactly as before; + * 2. the SHARED-OUTCOME contract — every sharer of one read gets that read's + * result, including when it is known-partial, and the memoized entry is + * still judged by #5184's `degraded` rules (2s TTL, not 30s); + * 3. the mid-read invalidation decision: **the invalidation wins**. A read + * that was in flight when `invalidateListCache()` fired keeps running for + * the callers already waiting on it, but loses the right to memoize its + * pre-write answer, and a caller arriving after the write starts a fresh + * read rather than joining a pre-write one (#5219 / #5229's bar: a + * consumer woken by an event must not observe the event and pre-event + * state together); + * 4. the in-flight map is self-cleaning — nothing accumulates, and a second + * wave after the cache lapses starts exactly one new read. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import type { + MetadataLoadOptions, + MetadataLoadResult, + MetadataLoaderContract, + MetadataStats, +} from '@objectstack/spec/system'; +import { MetadataManager } from './metadata-manager.js'; +import type { MetadataLoader } from './loaders/loader-interface.js'; + +// Stable logger mock — the degraded assertions read what was logged. +const logger = vi.hoisted(() => ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), +})); + +vi.mock('@objectstack/core', () => ({ + createLogger: () => logger, +})); + +/** Mirror of the manager's private `ListCacheEntry` (deliberately not exported). */ +type ListCacheEntry = { ts: number; items: unknown[]; degraded: boolean }; + +const peekEntry = (mgr: MetadataManager, type: string): ListCacheEntry | undefined => + (mgr as unknown as { listCache: Map }).listCache.get(type); + +/** Peek at the private in-flight map — internal by design, load-bearing for #5253. */ +const inflight = (mgr: MetadataManager): Map> => + (mgr as unknown as { inflightListReads: Map> }).inflightListReads; + +const ttls = () => { + const c = MetadataManager as unknown as { + LIST_CACHE_TTL_MS: number; + DEGRADED_LIST_CACHE_TTL_MS: number; + }; + return { healthy: c.LIST_CACHE_TTL_MS, degraded: c.DEGRADED_LIST_CACHE_TTL_MS }; +}; + +const names = (items: unknown[]): string[] => + (items as { name: string }[]).map((i) => i.name).sort(); + +/** Minimal read-only loader; only `loadMany` is exercised by `list()`. */ +abstract class TestLoader implements MetadataLoader { + abstract readonly contract: MetadataLoaderContract; + abstract loadMany(type: string, options?: MetadataLoadOptions): Promise; + async load(): Promise { + return { data: null }; + } + async exists(): Promise { + return false; + } + async stat(): Promise { + return null; + } + async list(): Promise { + return []; + } +} + +/** + * The issue's `SlowLoader`: `loadMany` resolves 100ms later. Timer-driven, so + * the repro reads exactly as filed. + */ +class SlowLoader extends TestLoader { + readonly contract: MetadataLoaderContract = { + name: 'slow', + protocol: 'memory:', + capabilities: { read: true, write: false, watch: false, list: true }, + }; + loadManyCalls = 0; + constructor(private readonly items: unknown[] = [{ name: 'from_loader' }]) { + super(); + } + async loadMany(): Promise { + this.loadManyCalls += 1; + await new Promise((resolve) => setTimeout(resolve, 100)); + return this.items as T[]; + } +} + +/** + * A loader whose reads park until the test lets them through, one at a time. + * Lets a test hold a read open across a `register()` — the mid-flight window + * #5253 is about — without depending on timer ordering. + */ +class GatedLoader extends TestLoader { + readonly contract: MetadataLoaderContract = { + name: 'gated', + protocol: 'memory:', + capabilities: { read: true, write: false, watch: false, list: true }, + }; + loadManyCalls = 0; + /** When true, a released read throws instead of answering (outage). */ + broken = false; + private parked: Array<() => void> = []; + + constructor(private readonly items: unknown[] = [{ name: 'from_loader' }]) { + super(); + } + + async loadMany(): Promise { + this.loadManyCalls += 1; + await new Promise((resolve) => this.parked.push(resolve)); + if (this.broken) throw Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }); + return this.items as T[]; + } + + get parkedCount(): number { + return this.parked.length; + } + + /** Let the oldest parked read proceed. */ + releaseNext(): void { + this.parked.shift()?.(); + } + + /** Let every currently parked read proceed. */ + releaseAll(): void { + const parked = this.parked; + this.parked = []; + for (const resolve of parked) resolve(); + } +} + +/** Give queued microtasks a chance to run without advancing fake time. */ +const flush = async (): Promise => { + for (let i = 0; i < 8; i++) await Promise.resolve(); +}; + +beforeEach(() => { + logger.error.mockClear(); + logger.info.mockClear(); + logger.warn.mockClear(); + vi.useFakeTimers(); +}); + +afterEach(() => { + vi.useRealTimers(); +}); + +describe('#5253 — concurrent list() calls share one loader walk', () => { + it('the issue repro: three concurrent list() calls hit the loader ONCE', async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + const slow = new SlowLoader(); + manager.registerLoader(slow); + + const all = Promise.all([ + manager.list('permission'), + manager.list('permission'), + manager.list('permission'), + ]); + await vi.advanceTimersByTimeAsync(100); + const [a, b, c] = await all; + + // Expected 1 (the `listCache` comment's "only hit once per TTL + // window"); before #5253 this was 3. + expect(slow.loadManyCalls).toBe(1); + expect(names(a)).toEqual(['from_loader']); + expect(names(b)).toEqual(['from_loader']); + expect(names(c)).toEqual(['from_loader']); + }); + + it('sharers receive the SAME result, not three equal copies assembled separately', async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + const gated = new GatedLoader(); + manager.registerLoader(gated); + + const reads = [manager.list('view'), manager.list('view'), manager.list('view')]; + await flush(); + expect(gated.loadManyCalls).toBe(1); + + gated.releaseAll(); + const [a, b, c] = await Promise.all(reads); + + expect(a).toBe(b); + expect(b).toBe(c); + // …and it is the very array memoized for the callers that come next. + expect(peekEntry(manager, 'view')!.items).toBe(a); + }); + + it('the in-flight slot is self-cleaning: empty once the read settles', async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + const gated = new GatedLoader(); + manager.registerLoader(gated); + + const reads = [manager.list('view'), manager.list('view')]; + expect(inflight(manager).size).toBe(1); + + gated.releaseAll(); + await Promise.all(reads); + + expect(inflight(manager).size).toBe(0); + expect(gated.loadManyCalls).toBe(1); + }); + + it('a second wave after settle is served by the cache, and after the TTL starts exactly ONE new read', async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + const gated = new GatedLoader(); + manager.registerLoader(gated); + + const first = [manager.list('view'), manager.list('view')]; + gated.releaseAll(); + await Promise.all(first); + expect(gated.loadManyCalls).toBe(1); + + // Within the TTL: the cache answers, nothing reaches the loader. + await manager.list('view'); + await manager.list('view'); + expect(gated.loadManyCalls).toBe(1); + + // Past the TTL: a fresh concurrent wave — one walk for the whole wave, + // not one per caller and not a stale memoized promise either. + vi.advanceTimersByTime(ttls().healthy + 1); + const second = [manager.list('view'), manager.list('view'), manager.list('view')]; + await flush(); + expect(gated.loadManyCalls).toBe(2); + gated.releaseAll(); + await Promise.all(second); + expect(gated.loadManyCalls).toBe(2); + }); + + it('sequential callers are unchanged: one walk per TTL window, as before', async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + const slow = new SlowLoader(); + manager.registerLoader(slow); + + const read = async () => { + const p = manager.list('view'); + await vi.advanceTimersByTimeAsync(100); + return p; + }; + + await read(); + expect(slow.loadManyCalls).toBe(1); + await manager.list('view'); + expect(slow.loadManyCalls).toBe(1); + + vi.advanceTimersByTime(ttls().healthy + 1); + await read(); + expect(slow.loadManyCalls).toBe(2); + }); + + it('different types do not share a read', async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + const gated = new GatedLoader(); + manager.registerLoader(gated); + + const reads = [manager.list('view'), manager.list('permission')]; + await flush(); + expect(gated.loadManyCalls).toBe(2); + expect(inflight(manager).size).toBe(2); + + gated.releaseAll(); + await Promise.all(reads); + expect(inflight(manager).size).toBe(0); + }); +}); + +describe('#5253 — sharing a DEGRADED read is an explicit contract', () => { + /** + * `list()` is best-effort and does not throw, so a lost loader is not an + * error to fail over from — it is the answer. Every sharer therefore gets + * the same known-partial set, and #5184's judgment still applies to what is + * memoized: single-flight must not become a back door onto the 30s TTL. + */ + it('all concurrent callers get the same partial set, memoized degraded on the short TTL', async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + const gated = new GatedLoader(); + gated.broken = true; + manager.registerLoader(gated); + manager.registerInMemory('permission', 'from_code', { name: 'from_code' }); + + const reads = [ + manager.list('permission'), + manager.list('permission'), + manager.list('permission'), + ]; + await flush(); + expect(gated.loadManyCalls).toBe(1); + + gated.releaseAll(); + const [a, b, c] = await Promise.all(reads); + + // One outage, one walk, one shared answer. + expect(gated.loadManyCalls).toBe(1); + expect(names(a)).toEqual(['from_code']); + expect(a).toBe(b); + expect(b).toBe(c); + // Said once, not once per sharer. + expect(logger.error).toHaveBeenCalledTimes(1); + + // [#5184] The entry is stored AS degraded — sharing did not launder it. + const entry = peekEntry(manager, 'permission')!; + expect(entry.degraded).toBe(true); + expect(entry.items).toBe(a); + }); + + it('the shared degraded entry expires on the 2s TTL, not the 30s one', async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + const gated = new GatedLoader(); + gated.broken = true; + manager.registerLoader(gated); + manager.registerInMemory('permission', 'from_code', { name: 'from_code' }); + + const reads = [manager.list('permission'), manager.list('permission')]; + gated.releaseAll(); + await Promise.all(reads); + expect(gated.loadManyCalls).toBe(1); + + // Inside the degraded window the burst is still absorbed… + await manager.list('permission'); + expect(gated.loadManyCalls).toBe(1); + + // …and just past it the loader is re-asked, exactly as for a solo read. + vi.advanceTimersByTime(ttls().degraded + 1); + gated.broken = false; + const retry = manager.list('permission'); + await flush(); + gated.releaseAll(); + expect(names(await retry)).toEqual(['from_code', 'from_loader']); + expect(gated.loadManyCalls).toBe(2); + expect(peekEntry(manager, 'permission')!.degraded).toBe(false); + }); +}); + +describe('#5253 — an invalidation that crosses an in-flight read WINS', () => { + /** + * The decision this pins (see the `inflightListReads` field comment): + * callers already waiting still receive the in-flight, pre-write answer — + * they asked before the write — but that answer is never memoized past the + * invalidation, and nobody who asks after the write joins it. + */ + const managerMidRead = async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + const gated = new GatedLoader(); + manager.registerLoader(gated); + // Read starts and parks inside the loader — the registry snapshot it + // took is already fixed at this point. + const firstRead = manager.list('permission'); + await flush(); + expect(gated.loadManyCalls).toBe(1); + // A write lands mid-read. + await manager.register('permission', 'from_write', { name: 'from_write' }); + return { manager, gated, firstRead }; + }; + + it('the waiting caller still gets the pre-write result — it is not restarted under them', async () => { + const { gated, firstRead } = await managerMidRead(); + + gated.releaseAll(); + // Assembled before the write landed, and delivered as such: one walk, + // no retry loop bolted onto a best-effort seam. + expect(names(await firstRead)).toEqual(['from_loader']); + expect(gated.loadManyCalls).toBe(1); + }); + + it('but that pre-write answer is NOT memoized: the next read observes the write', async () => { + const { manager, gated, firstRead } = await managerMidRead(); + + gated.releaseAll(); + await firstRead; + + // The invalidation wins: nothing was written back over it. + expect(peekEntry(manager, 'permission')).toBeUndefined(); + expect(inflight(manager).size).toBe(0); + + const next = manager.list('permission'); + await flush(); + gated.releaseAll(); + expect(names(await next)).toEqual(['from_loader', 'from_write']); + expect(gated.loadManyCalls).toBe(2); + expect(names(peekEntry(manager, 'permission')!.items)).toEqual(['from_loader', 'from_write']); + }); + + it('a caller arriving AFTER the write starts a fresh read instead of joining the pre-write one', async () => { + const { manager, gated, firstRead } = await managerMidRead(); + + // #5219 / #5229's bar: a consumer woken by the change must not be handed + // a read that began before it. + const afterWrite = manager.list('permission'); + await flush(); + expect(gated.loadManyCalls).toBe(2); + + gated.releaseAll(); + const [before, after] = await Promise.all([firstRead, afterWrite]); + expect(names(before)).toEqual(['from_loader']); + expect(names(after)).toEqual(['from_loader', 'from_write']); + // Only the post-write read gets to memoize. + expect(names(peekEntry(manager, 'permission')!.items)).toEqual(['from_loader', 'from_write']); + }); + + it('the retracted read settling does not evict the fresh read that replaced it', async () => { + const { manager, gated, firstRead } = await managerMidRead(); + + const afterWrite = manager.list('permission'); + await flush(); + expect(gated.loadManyCalls).toBe(2); + + // Settle ONLY the retracted read. + gated.releaseNext(); + await firstRead; + + // The fresh read still owns the slot… + expect(inflight(manager).size).toBe(1); + // …so a third caller joins it rather than starting a third walk. + const joiner = manager.list('permission'); + await flush(); + expect(gated.loadManyCalls).toBe(2); + + gated.releaseAll(); + const [after, joined] = await Promise.all([afterWrite, joiner]); + expect(after).toBe(joined); + expect(names(joined)).toEqual(['from_loader', 'from_write']); + expect(inflight(manager).size).toBe(0); + }); + + it('a foreign write (cluster peer / FS event) retracts the in-flight read the same way', async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + const gated = new GatedLoader(); + manager.registerLoader(gated); + + const firstRead = manager.list('permission'); + await flush(); + + // Same seam #5109 (cluster) and #5218 (filesystem) invalidate through. + (manager as unknown as { invalidateForForeignWrite(t: string, n?: string): void }) + .invalidateForForeignWrite('permission', 'from_peer'); + + gated.releaseAll(); + await firstRead; + + expect(peekEntry(manager, 'permission')).toBeUndefined(); + }); +}); diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index a190b2fb5b..3a6678a162 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/packages/metadata/src/metadata-manager.ts @@ -184,10 +184,13 @@ export class MetadataManager implements IMetadataService { // acquire a fresh knex connection while the transaction is still holding // SQLite's single connection — knex waits the full `acquireConnectionTimeout` // (60s) before returning []. The cache absorbs the repeated lookups so the - // loader is only hit once per TTL window — for SEQUENTIAL callers. Nothing is - // written until a read completes, so calls issued concurrently with the first - // one all miss and each walk every loader (#5253); read that sentence as a - // statement about repeated lookups, not a concurrency guarantee. + // loader is only hit once per TTL window — for CONCURRENT callers as well as + // sequential ones, since #5253. The cache on its own could only ever deliver + // the sequential half of that promise: nothing is written until a read + // completes, so everything issued before the first read returned used to miss + // and walk every loader — N callers, N × 60s on the very stall described + // above. The concurrent half is delivered by `inflightListReads` below, which + // is why the two fields are one policy and are documented together. // // [#5184] That hazard is NOT historical — it was re-verified on the current // driver stack before this policy was chosen. `DatabaseLoader._find()` still @@ -238,6 +241,57 @@ export class MetadataManager implements IMetadataService { */ private static readonly DEGRADED_LIST_CACHE_TTL_MS = 2_000; + /** + * [#5253] The `list()` read currently in flight for a metadata type — the + * concurrent half of the `listCache` policy above. + * + * `listCache` memoizes an answer only once a read has *finished*, so it can + * absorb the caller that arrives second in time but never the caller that + * arrives second in flight. Everything issued while the first read is still + * walking the loaders used to miss and start its own identical walk; on the + * knex/SQLite path the field comment above is built for, that is 60s burned + * per concurrent caller instead of once for all of them. A type is read once + * at a time: whoever finds a read already running joins it. + * + * **Sharers share the outcome. This is a contract, not an accident.** Every + * caller joining an in-flight read receives that read's exact result — the + * same array instance, and, when a loader was unreadable, the same + * known-partial set that gets memoized `degraded: true` on the short TTL. + * There is no per-caller retry: `list()` is the best-effort listing seam and + * does not throw (see {@link reportLoaderReadFailure}; the strict + * counterparts are `listForIndex()` and {@link loadDiagnosed}), so a lost + * loader is not an error to fail over from — it is the answer. Re-running the + * read privately for a joiner would walk the same loaders in the same window + * against the same outage, which is precisely what this map exists to + * prevent. Should the seam ever acquire a rejecting path, that rejection is + * shared by the same mechanism and for the same reason. + * + * **The registration is also the permission to cache.** An entry here says + * "this read still describes the current state". {@link invalidateListCache} + * retracts it, which is what makes a write landing mid-read safe in both + * directions: + * • the retracted read does NOT write its result into `listCache` when it + * settles, so an answer assembled before the write cannot outlive the + * write it predates (the invalidation wins — it is the later, better + * informed fact); + * • a `list()` issued after the invalidation starts a FRESH read instead of + * joining one that predates the write. + * That second point is the #5219 / #5229 ordering bar restated for + * concurrency: a consumer woken by a metadata change must not observe the + * event and pre-event state together, and handing a woken watcher an + * in-flight read that began before the event would be exactly that. + * Callers *already waiting* on the retracted read still receive its (now + * possibly stale) result — they asked before the write, and restarting the + * read under them would turn a write burst into an unbounded retry loop on + * the one path the cache exists to keep off the loaders. + * + * Self-cleaning: the entry is dropped when the read settles, by that read + * only, so a fresh read that already replaced it keeps its slot. Nothing + * accumulates — a wave of callers arriving after settle finds the cache the + * settle just wrote, and once that lapses it starts one new read. + */ + private readonly inflightListReads = new Map>(); + // [#5108] Loader names whose read failure has already been reported at // `error` by `list()`. AGENTS.md → "Degradation log levels": say it once, at // the first degradation — `list()` is hot enough that one line per failed @@ -605,7 +659,17 @@ export class MetadataManager implements IMetadataService { } /** - * List all metadata items of a given type + * List all metadata items of a given type. + * + * Best-effort by contract: a loader that cannot be read is reported once and + * skipped ({@link reportLoaderReadFailure}), so this resolves with what the + * reachable loaders hold rather than throwing. + * + * [#5253] Reads of one type are single-flight — concurrent callers join the + * read already running instead of each walking every loader. What they are + * promised, and what happens when a write lands mid-read, is the contract on + * `inflightListReads`; what is memoized afterwards is the contract on + * `listCache`. */ async list(type: string): Promise { // Short-TTL cache: see the field comment on `listCache` for what is @@ -616,6 +680,54 @@ export class MetadataManager implements IMetadataService { return cached.items; } + // [#5253] Cold cache, but not necessarily a cold read: join the walk + // already in progress for this type rather than starting an identical one. + const joined = this.inflightListReads.get(type); + if (joined) { + return joined; + } + + // Registering the read is what permits it to memoize its own result — + // `invalidateListCache()` retracts the registration, and both the cache + // write and the cleanup below act only while the slot is still ours. + const shared: Promise = this.readListUncached(type).then(({ items, degraded }) => { + // [#5184] The degraded verdict of a SHARED read is the degraded verdict + // of the read: sharing must not become a back door that lands a + // known-partial answer on the 30s healthy TTL. Every sharer received + // this same set, and it is memoized as what it is. + // [#5253] Skipped when an invalidation crossed this read, so a write + // that landed mid-read is never re-buried under the pre-write answer. + if (this.inflightListReads.get(type) === shared) { + this.cacheListResult(type, items, degraded); + } + return items; + }); + this.inflightListReads.set(type, shared); + + try { + return await shared; + } finally { + // Retract only our OWN registration: an invalidation may have already + // dropped it and a fresh read may own the slot now — deleting that one + // would let a third read start against the same window. + if (this.inflightListReads.get(type) === shared) { + this.inflightListReads.delete(type); + } + } + } + + /** + * Assemble the `list()` answer for `type` from the in-memory registry plus + * every loader, reporting (but not rethrowing) loaders that could not be + * read. + * + * The body {@link list} used to inline, extracted so the caching and + * single-flight bookkeeping around it has one thing to run at most once per + * type (#5253). Deliberately does NOT touch `listCache` itself: whether this + * result may be memoized depends on what happened to the read's registration + * while it ran, which only `list()` can see. + */ + private async readListUncached(type: string): Promise<{ items: unknown[]; degraded: boolean }> { const items = new Map(); // From in-memory registry @@ -647,9 +759,7 @@ export class MetadataManager implements IMetadataService { } } - const result = Array.from(items.values()); - this.cacheListResult(type, result, degraded); - return result; + return { items: Array.from(items.values()), degraded }; } /** @@ -743,9 +853,20 @@ export class MetadataManager implements IMetadataService { return Date.now() - cached.ts < ttl ? cached : undefined; } - /** Internal helper: drop the cached `list()` result for a type. */ + /** + * Internal helper: drop every memoized or in-progress `list()` answer for a + * type, so the next read observes the write that called this. + * + * [#5253] Retracting the in-flight read (not just the finished entry) is the + * whole mid-read story, and it is pinned by test: the read keeps running for + * the callers already waiting on it, but it loses the right to memoize its + * pre-write answer, and a caller arriving after this point gets a fresh read + * instead of joining a pre-write one. The reasoning — including why waiting + * callers are NOT restarted — is on the `inflightListReads` field. + */ private invalidateListCache(type: string): void { this.listCache.delete(type); + this.inflightListReads.delete(type); // [#5089] The endpoint index is a cache of the same stored set, so it goes // stale under exactly the same conditions. Hooking here (rather than only // on the watcher) is what covers the `{ notify: false }` writes — artifact