Skip to content

Make target.loadedFromSource the sole cache-disposition signal (net of v5.1 #1575 + #1590)#1626

Open
kriszyp wants to merge 3 commits into
mainfrom
kris/loadedfromsource-target-only-main
Open

Make target.loadedFromSource the sole cache-disposition signal (net of v5.1 #1575 + #1590)#1626
kriszyp wants to merge 3 commits into
mainfrom
kris/loadedfromsource-target-only-main

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 6, 2026

Copy link
Copy Markdown
Member

Brings the net end-state of the loadedFromSource cache-disposition work to main/5.2. That work landed only on v5.1 as two PRs — #1575 (initial fix) and #1590 (rework) — and was never on main. This is the single clean end-state, not a replay of #1575-then-#1590.

Why

Cache disposition (loadedFromSource: served from source vs. cache) is fundamentally a per-get() result — each get() has its own RequestTarget. #1575 additionally mirrored disposition onto the request Context, which is shared for the whole request lifecycle, making 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. #1590 dropped that mirror and standardized on target.loadedFromSource, which server/REST.ts already reads.

What changed

  • target.loadedFromSource is the sole signal. New setLoadedFromSource(target, bool) helper in Table.ts writes only the RequestTarget, with a primitive-id guard (typeof target === 'object') — instance-API calls can pass a primitive id that can't hold the flag. No Context writes anywhere.
  • Age header now emitted on cache hits. The loadAsInstance=false value path and the ensureLoaded branch (Table.ts ~1229/~1242) previously set the flag only on source fetch, leaving it undefined on hits — so REST.ts (if (loadedFromSource !== undefined)) skipped the Age header on hits. They now mark false on hits.
  • Removed dead code: the wasLoadedFromSource() method (Resource.ts / ResourceInterface.ts) and the Context.loadedFromSource declaration (ResourceInterface.ts), both now unused.

Where to look

  • resources/Table.ts is the substance — 7 small hunks routing all disposition writes through the guarded helper plus the two new cache-hit markings. The get() path is hot; the helper is a trivial branch, no added async.
  • Downstream note (open, tracked separately): security/certificateVerification/{ocsp,crl}Verification.ts still call cacheEntry.wasLoadedFromSource?.(). Removing the method is behavior-preserving there — it always returned undefined/void, and the optional-chaining call now yields undefined, i.e. the same "cache hit" branch as before. To actually observe disposition there, a RequestTarget must be passed to the get; that's out of scope here (matches Make target.loadedFromSource the sole cache-disposition signal; drop the Context mirror (#1576) #1590's downstream note). This is the one spot worth a reviewer's eye.

Testing

Ported the v5.1 caching unit coverage to unitTests/resources/caching.test.js: a loadAsInstance=false hit/miss case and a stale-while-revalidate loadedFromSource=false assertion. 15/15 caching tests pass against dist.

Generated by KrAIs (Claude Opus 4.8), at Kris's direction.

…v5.1 #1575 + #1590)

Make target.loadedFromSource the sole cache-disposition signal.

- setLoadedFromSource() writes only the RequestTarget, guarding against
  primitive-id instance-API targets; no Context mirror.
- Mark disposition false on cache hits in the loadAsInstance=false value path
  and ensureLoaded branch so REST emits the Age header on hits.
- Remove the unused wasLoadedFromSource() method and Context.loadedFromSource
  declaration.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors how the cache disposition flag loadedFromSource is set and tracked. It removes the unused wasLoadedFromSource method from Resource and ResourceInterface, and introduces a helper function setLoadedFromSource in Table.ts to safely set the flag on the RequestTarget object, preventing issues when the target is a primitive ID. Additionally, new unit tests are added to verify this behavior under different caching configurations. I have no feedback to provide.

@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

Non-blocking suggestionsecurity/certificateVerification/ocspVerification.ts and crlVerification.ts both call cacheEntry.wasLoadedFromSource?.() (lines 88–94 and 253–259 respectively). Now that wasLoadedFromSource() is removed from Resource, the optional-chain call silently returns undefined, so cached: !wasLoadedFromSource is always true in those return objects — a silent telemetry/tracing regression. The PR description correctly acknowledges this and says it’s tracked separately. When that follow-up lands, the fix is to pass a RequestTarget to the .get() call and read target.loadedFromSource instead. No action needed on this PR.

@kriszyp

kriszyp commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

CI note: the red Unit Test job (v22/v24/v26) is the pre-existing subscriptionReplay.test.js:1048 flake introduced by #1345 ("expected at least 1 event for id=1, got 0") — already red on main across all Node versions, unrelated to this change and tracked/fixed by #1625. The resources unit suite that actually exercises this change is fully green locally (1009 passing, incl. the caching suite 15/15). Everything else on this PR (builds, all integration shards, format, lint) is green.

Posted by KrAIs (Claude Opus 4.8).

Comment thread resources/Table.ts
…king (#1576)

Addresses cb1kenobi's review note on PR #1626 (two low-risk coverage gaps):

- loadAsInstance=false onlyIfCached cache-hit marks target.loadedFromSource=false
  (the value-path twin of the loadAsInstance=true instance path; previously untested).
- primitive-id instance-API getResource() no longer throws when recording cache
  disposition — locks in the `typeof target === 'object'` guard in setLoadedFromSource.
  Verified the test fails without the guard ("Cannot create property 'loadedFromSource'
  on number '41'").

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Ethan-Arrowood

Copy link
Copy Markdown
Member

seems like a rebase is needed to know if this can land cleanly. Rerequest review once rebased. Thanks!

sent with Claude Fable 5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants