From de33cee23257c729e32794c7692eb2bd0057f650 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Fri, 3 Jul 2026 07:09:23 -0600 Subject: [PATCH] refactor(resources): make target.loadedFromSource the sole cache-disposition signal (#1576) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1575 mirrored cache disposition onto the request Context so plain-id gets could observe it (#1571). But loadedFromSource is fundamentally a per-get result: each get() has its own RequestTarget, whereas the Context is shared for the whole request lifecycle. Mirroring onto the shared Context makes it last-write-wins — an internal caching get() inside a resource's own get() overwrites the entry get's disposition before the caller reads it. The REST handler already reads the precise per-get value from target.loadedFromSource (server/REST.ts), and #1571's gap is really a usage question answered by docs: observe disposition on the RequestTarget you pass to the get. This removes the Context mirror and standardizes on target.loadedFromSource: - setLoadedFromSource() now writes only the target (keeping the primitive-id guard #1575 added); drop the Context param and all context writes. - Remove the context-only marking in ensureLoaded() (undefined target). - Remove the now-unused Context.loadedFromSource declaration (it predated #1575 as declared-but-never-assigned). Retained from #1575 (the parts that were genuinely right): the per-get false-marking on cache HITS in the loadAsInstance=false value path and ensureLoaded (Table.ts ~1227/~1240) — before #1575 those paths set the flag on source fetch but left it undefined on cache hits, so REST skipped the Age header on hits. And the primitive-target guard. Tests updated to observe target.loadedFromSource; full caching suite passes (15/15) against dist. Docs: HarperFast/documentation#563. Co-Authored-By: Claude Opus 4.8 --- resources/RequestTarget.ts | 3 +- resources/ResourceInterface.ts | 6 ---- resources/Table.ts | 24 ++++++---------- unitTests/resources/caching.test.js | 44 ++++++++++------------------- 4 files changed, 26 insertions(+), 51 deletions(-) diff --git a/resources/RequestTarget.ts b/resources/RequestTarget.ts index 21e68e802d..aac5b10839 100644 --- a/resources/RequestTarget.ts +++ b/resources/RequestTarget.ts @@ -56,7 +56,8 @@ export class RequestTarget extends URLSearchParams { declare previousResidency?: string[]; // Action tracking - /** Cache disposition of a get on a caching table; also mirrored onto the request Context. */ + /** Cache disposition of this get on a caching table: true if loaded from the source, false if + * served from cache. Set per-get; read it on the RequestTarget you passed to the get. */ declare loadedFromSource?: boolean; declare createdNewId?: string; diff --git a/resources/ResourceInterface.ts b/resources/ResourceInterface.ts index 1c3abdd76e..4ce2515d2e 100644 --- a/resources/ResourceInterface.ts +++ b/resources/ResourceInterface.ts @@ -94,12 +94,6 @@ export interface Context { sourceApply?: boolean; originatingOperation?: OperationFunctionName; previousResidency?: string[]; - /** Cache disposition of the most recent get on a caching table in this context: true if the get - * fetched from source — including when a source error fell back to a stale record (staleIfError); - * false if served from cache — including stale-while-revalidate responses (the source fetch - * continues in the background) and waits on another request's in-flight source fetch. - * Subsequent gets in the same context overwrite it. */ - loadedFromSource?: boolean; nodeName?: string; resourceCache?: Map; _freezeRecords?: boolean; // until v5, we conditionally freeze records for back-compat diff --git a/resources/Table.ts b/resources/Table.ts index 5ef1c3a5d4..c4c7b5d90e 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -727,7 +727,7 @@ export function makeTable(options) { // return 504 (rather than 404) if there is no content and the cache-control header // dictates not to go to source if (!this.doesExist()) throw new ServerError('Entry is not cached', 504); - if (hasSourceGet) setLoadedFromSource(target, request, false); // mark it as cached + if (hasSourceGet) setLoadedFromSource(target, false); // mark it as cached } else if (resourceOptions?.ensureLoaded) { const loadingFromSource = ensureLoadedFromSource( (this.constructor as any).source, @@ -743,7 +743,7 @@ export function makeTable(options) { TableResource._updateResource(this, entry); return this; }); - } else if (hasSourceGet) setLoadedFromSource(target, request, false); // mark it as cached + } else if (hasSourceGet) setLoadedFromSource(target, false); // mark it as cached } return this; } @@ -776,7 +776,7 @@ export function makeTable(options) { this.#record = entry.value; this.#version = entry.version; }); - } else if (hasSourceGet) setLoadedFromSource(undefined, this.getContext(), false); // mark it as cached + } } // #section: lifecycle-admin static getNewId(): any { @@ -1224,7 +1224,7 @@ export function makeTable(options) { // return 504 (rather than 404) if there is no content and the cache-control header // dictates not to go to source if (!entry?.value) throw new ServerError('Entry is not cached', 504); - if (hasSourceGet) setLoadedFromSource(target, context, false); // mark it as cached + if (hasSourceGet) setLoadedFromSource(target, false); // mark it as cached } else if (ensureLoaded) { const loadingFromSource = ensureLoadedFromSource( constructor.source, @@ -1237,7 +1237,7 @@ export function makeTable(options) { if (loadingFromSource) { txn?.disregardReadTxn(); // this could take some time, so don't keep the transaction open if possible return loadingFromSource.then((entry) => entry?.value); - } else if (hasSourceGet) setLoadedFromSource(target, context, false); // mark it as cached + } else if (hasSourceGet) setLoadedFromSource(target, false); // mark it as cached } return entry?.value; }); @@ -4555,16 +4555,10 @@ export function makeTable(options) { } } - function setLoadedFromSource( - target: RequestTarget | undefined, - context: Context | undefined, - loadedFromSource: boolean - ) { - // mirror the flag onto the context: callers that pass a plain id get an internal - // RequestTarget they never see, so the context is their only way to observe cache disposition (#1571) + function setLoadedFromSource(target: RequestTarget | undefined, loadedFromSource: boolean) { + // cache disposition is a per-get result, recorded on the RequestTarget of the get (#1576) // target may be a primitive id on instance-API calls, which can't hold the flag if (target && typeof target === 'object') target.loadedFromSource = loadedFromSource; - if (context) context.loadedFromSource = loadedFromSource; } function ensureLoadedFromSource(source: typeof TableResource, id, entry, context, resource?, target?) { if (context?.onlyIfCached) { @@ -4798,7 +4792,7 @@ export function makeTable(options) { whenResolved(getFromSource(source, id, primaryStore.getEntry(id), context, target)); else { // served from cache after waiting for another request to resolve - setLoadedFromSource(target, context, false); + setLoadedFromSource(target, false); whenResolved(entry); } }; @@ -4813,7 +4807,7 @@ export function makeTable(options) { }); } // lock acquired — this request will actually load from source - setLoadedFromSource(target, context, true); + setLoadedFromSource(target, true); const existingRecord = existingEntry?.value; // it is important to remember that this is _NOT_ part of the current transaction; nothing is changing diff --git a/unitTests/resources/caching.test.js b/unitTests/resources/caching.test.js index 1cbaf9cc9f..4193323964 100644 --- a/unitTests/resources/caching.test.js +++ b/unitTests/resources/caching.test.js @@ -133,39 +133,24 @@ describe('Caching', () => { assert.equal(target23.loadedFromSource, true); }); - it('loadedFromSource is observable on the context with a plain id', async function () { - // with a plain id, the static get dispatch mints an internal RequestTarget the caller - // never sees, so the flag must be mirrored onto the context (#1571) - CachingTable.setTTLExpiration(30); - await CachingTable.invalidate(31); - let context = {}; - let result = await CachingTable.get(31, context); - assert.equal(result.id, 31); - assert.equal(context.loadedFromSource, true); - context = {}; - result = await CachingTable.get(31, context); - assert.equal(result.id, 31); - assert.equal(context.loadedFromSource, false); - context = { onlyIfCached: true }; - result = await CachingTable.get(31, context); - assert.equal(result.id, 31); - assert.equal(context.loadedFromSource, false); - }); - - it('loadedFromSource is observable on the context with loadAsInstance = false', async function () { + it('loadedFromSource is observable on the target with loadAsInstance = false (#1576)', async function () { + // disposition is recorded on the RequestTarget of the get; verify the loadAsInstance=false + // value path marks it on both cache miss (true) and cache hit (false) const previousLoadAsInstance = CachingTable.loadAsInstance; try { CachingTable.loadAsInstance = false; CachingTable.setTTLExpiration(30); await CachingTable.invalidate(32); - let context = {}; - let result = await CachingTable.get(32, context); + let target = new RequestTarget(); + target.id = 32; + let result = await CachingTable.get(target); assert.equal(result.id, 32); - assert.equal(context.loadedFromSource, true); - context = {}; - result = await CachingTable.get(32, context); + assert.equal(target.loadedFromSource, true); + target = new RequestTarget(); + target.id = 32; + result = await CachingTable.get(target); assert.equal(result.id, 32); - assert.equal(context.loadedFromSource, false); + assert.equal(target.loadedFromSource, false); } finally { CachingTable.loadAsInstance = previousLoadAsInstance; } @@ -266,10 +251,11 @@ describe('Caching', () => { events = []; await new Promise((resolve) => setTimeout(resolve, 10)); // should be stale but not evicted - const swrContext = {}; - let result = await CachingTableStaleWhileRevalidate.get(23, swrContext); + const swrTarget = new RequestTarget(); + swrTarget.id = 23; + let result = await CachingTableStaleWhileRevalidate.get(swrTarget); assert(result); // should exist in database even though it is stale - assert.equal(swrContext.loadedFromSource, false); // stale value served from cache while revalidating + assert.equal(swrTarget.loadedFromSource, false); // stale value served from cache while revalidating assert.equal(sourceRequests, 1); // the source request should be started assert.equal(sourceResponses, 0); // the source request should not be completed yet // the source request should be completed