Make target.loadedFromSource the sole cache-disposition signal; drop the Context mirror (#1576)#1590
Closed
kriszyp wants to merge 1 commit into
Closed
Make target.loadedFromSource the sole cache-disposition signal; drop the Context mirror (#1576)#1590kriszyp wants to merge 1 commit into
kriszyp wants to merge 1 commit into
Conversation
…osition signal (#1576) #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 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors how the cache disposition flag loadedFromSource is tracked and exposed. It removes the mirroring of loadedFromSource onto the request Context (removing it from the Context interface and the setLoadedFromSource function), and instead records it strictly on the RequestTarget object passed to the get operation. The corresponding unit tests have been updated to reflect this change. I have no feedback to provide as there are no review comments.
Contributor
|
Reviewed; no blockers found. |
kriszyp
added a commit
that referenced
this pull request
Jul 3, 2026
…mSource The OCSP and CRL verification paths reported the `cached` field (and the trace log) by calling `wasLoadedFromSource?.()` on the record returned by the caching table's `get()`. That plain frozen record has no resource methods, so the call was always `undefined` and `cached` was always `true` (always "cache hit"). `wasLoadedFromSource()` was removed from the Resource base class in #1576, and the Context mirror of the flag is removed in #1590. Observe cache disposition on the RequestTarget of the get instead (#1590 makes `target.loadedFromSource` the sole signal, marked on both hit and miss by the `_loadRecord` path that serves this instance-mode table). - ocsp / crl: pass a RequestTarget as the get id (empty ctor + `target.id = cacheKey` to avoid URL-parsing keys that may contain '/' or '?'); the cert data still travels in the context arg, which the source reads as requestContext. Read `target.loadedFromSource` after the get. CRL is aligned to the same 2-arg `get(target, context)` convention as OCSP. - certificateVerificationSource: read performCRLCheck/performOCSPCheck off the live module object (`.default`) with memoized, per-method dynamic imports. A CJS module's ESM-import namespace snapshots its named exports at first import, so a cached function reference would honor a stale implementation forever — e.g. a test double pinned past its restore, which leaked across test files. - crl: import the PKI.js Ed25519/Ed448 patch directly. CRL signature verification uses PKI.js but previously got the patch only as a side effect of loading the OCSP module; with the per-method import above, a CRL-only path no longer loads OCSP, so it must own its patch dependency (else Ed25519/Ed448 issuers degrade to unknown/failure). Adds unit tests covering fresh-fetch cached:false and cached-read cached:true for both OCSP and CRL. Verified the cert cache table marks target.loadedFromSource true on miss / false on hit despite being instance mode (loadAsInstance unset), and that loading only the CRL module applies the Ed25519 patch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 3, 2026
Member
Author
|
Closing — v5.1's #1575 (the base this PR reworks) was reverted after we caught it riding into an unintended v5.1.16 patch (it should not have shipped as a patch). The Context-mirror bug and the fix this PR describes both only ever existed on The net end-state this PR was going for (target-only — KrAIs |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1575 (already merged to
v5.1). Fixes #1576.Why
#1575 mirrored cache disposition onto the request Context so plain-id gets could observe it (#1571). But
loadedFromSourceis fundamentally a per-get()result: eachget()has its ownRequestTarget, whereas theContextis shared for the whole request lifecycle. Mirroring onto the shared Context makes it last-write-wins — an internal cachingget()inside a resource's ownget()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:231). And #1571's "unobservable on string-id gets" is really a usage question best answered by docs: observe disposition on theRequestTargetyou pass to the get. So the Context mirror is code papering over a documentation gap, and it introduces a semantic that doesn't hold up.What
Remove the Context mirror and standardize on
target.loadedFromSource:setLoadedFromSource()now writes only the target (keeping the primitive-id guard Fix loadedFromSource being unobservable on string-id gets on caching tables #1575 added); Context param and all context writes dropped.ensureLoaded()(it passedundefinedtarget — pure context write).Context.loadedFromSourcedeclaration (it predated Fix loadedFromSource being unobservable on string-id gets on caching tables #1575 as declared-but-never-assigned).Retained from #1575 (the parts that were genuinely right)
false-marking on cache HITS in theloadAsInstance=falsevalue path andensureLoaded(Table.ts~1227/~1240). Before Fix loadedFromSource being unobservable on string-id gets on caching tables #1575 those paths set the flag on source fetch but left itundefinedon cache hits, soREST.ts(if (loadedFromSource !== undefined)) skipped theAgeheader on hits. This is a realtarget-side fix, independent of the mirror.Testing
Caching unit suite converted to observe
target.loadedFromSource; 15/15 passing againstdist(incl. theloadAsInstance=falsehit/miss case and stale-while-revalidate). The plain-id "observable on context" test was removed (its premise — the mirror — is gone; target hit/miss is covered by the existingCan load cached datatest).Docs
HarperFast/documentation#563 reframed to make
target.loadedFromSourcecanonical and dropcontext.loadedFromSource.Note for downstream
The OCSP/CRL cert-verification code (
security/certificateVerification/*) does plain-id gets and previously read the (always-undefined)wasLoadedFromSource(). With the mirror gone, disposition there must be observed by passing aRequestTarget; tracked separately.Authored by KrAIs (Claude Opus 4.8), at Kris's direction.