[pkaminski]

Concurrent GETs previously shared the first caller's fully processed promise. A joining caller skipped its callbacks and inherited the first caller's recovery values and retry policy. This change shares individual fetches while each caller keeps its own onSend, timeout, response parsing, metadata, pagination, and error/retry decisions.

A timeout of zero from onSend or options.timeout now invokes onError with a TimeoutError before sending or joining, without invoking onReceive. It rejects by default; the handler may recover or explicitly request a retry within maxTries. The callback can override the options timeout; nullish returns retain it. Positive timeouts apply independently to every caller, including the caller that started the fetch. A timeout cannot abort a fetch while another caller still needs it; the last departing caller aborts an unfinished fetch. Completed cache hits remain immediately available.

onReceive(call, latency) belongs to the physical fetch and runs exactly once after its body finishes, transport fails, or transport aborts. It uses the initiating caller's callback even if that caller has already timed out while other callers keep the fetch alive. Individual caller timeouts do not invoke it while the transport continues. Quota cost is reported once, including zero/unknown GraphQL cost, and latency measures the full fetch after onSend. If the callback throws, the shared attempt rejects for every remaining waiter; each caller then applies its own onError and retry policy. The callback is not invoked again for its own exception. This needs no shared-callback flag or cost/timing claim bookkeeping.

Every waiting caller receives quota metadata when headers arrive, before the body finishes or fails. A caller joining after headers receives the original observation timestamp without overwriting equal-time or newer quota metadata. The transport's parsed response is reused by the initiating caller, so ordinary REST and GraphQL requests parse JSON once, including cost accounting. Joined callers parse separate copies; error objects and parsed JSON remain isolated before caller-specific handling. Raw response bodies, including ArrayBuffers, are shared without copying; callers must copy a buffer before mutation or transfer. Concurrent retries can share their next fetch, and matching headers and the current page URL are required for transport reuse. Shared responses count as cache hits whether callers recover, retry, or reject, including unhandled HTTP 5xx responses; these metrics measure response reuse, with each retry counted separately.

This also fixes the pre-existing rejected-promise cache bug: throwing callbacks and exhausted network failures could leave a rejected promise cached indefinitely. Rejected promises are no longer stored in the response cache. Pending entries count active callers, retain any previous completed cachedItem for conditional requests, and are removed after the last finishes unless replaced by a completed response. The README and cache-entry union include this retained item, reusing the completed-response type. Identity checks protect newer requests and responses from late cleanup or completion. Package version: 9.2.0; README and TypeScript declarations describe the new behavior. The named Options type export is preserved alongside the base branch's static metadata declarations.

Validation: all 381 Hubkit tests pass, including 72 Node/browser shared-request cases. They check JSON parse counts for unshared REST/GraphQL requests, isolated shared results, and completed-fetch accounting even when JSON parsing fails, plus independent preparation and timeout budgets, transport completion after the initiator times out, one callback on body/transport failure or final abort, GraphQL cost after the body finishes, and callback exceptions rejecting every remaining waiter while preserving independent recovery/retry decisions. Existing coverage checks error/data isolation, header-time quota metadata and late joiners, manual/automatic pagination, conditional 304s, buffer identity, cache statistics (including shared rejected 500s and exhausted retries), and fresh/cache-clear races. Zero-timeout tests cover recovery, explicit retry, retry exhaustion, and throwing handlers. yarn lint, git diff --check, and a strict TypeScript cache-entry consumer check pass.

Client compatibility: 115 existing client tests pass with this candidate Hubkit substituted. Exercising the real diff worker's concurrent fetchFile calls confirmed one shared fetch, correct charset metadata for both callers, and separate HTTP 403 errors without retries. No client code changes are needed.

Companion server change: https://github.com/Reviewable/reviewable-server/pull/1135.

---

This change is 

[pkaminski]

@pkaminski reviewed 5 files and all commit messages.
Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on pkaminski).

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-06T19:04:32.293063Z	8d5b7ca	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9edb2d8db0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Evict the cached promise after a pre-send timeout

With the default GET cache enabled, requestPromise is inserted into the cache while send() is suspended at this await, but this new outer-catch rejection never reaches the cache-deletion path in handleError. Consequently, a cache miss requested with timeout: 0 leaves a rejected promise under the cache key, and every later request for the same resource—even with a positive or omitted timeout—reuses that rejection without issuing a fetch until the entry is evicted. Remove the matching in-flight cache entry when rejecting here, or reject before inserting it.

Useful? React with 👍 / 👎.

[pkaminski]

Fixed. Rejected request promises now remove their cache entry only if it still contains that same promise. The timeout suite now runs with caching enabled and covers recovery after zero options, sync/async zero callbacks, and a throwing callback. It also verifies that a late rejection preserves both a newer in-flight promise and a newer completed response. The eight recovery cases failed before this fix; all 84 Hubkit tests now pass.

[chatgpt-codex-connector]

Accept documented nullish onSend results in the type

The README, implementation, and new tests explicitly support onSend returning undefined or null to retain options.timeout, but this signature still requires a number. With strict null checks, TypeScript users following the documented contract cannot provide () => null, async () => null, or a callback with no return; include nullish values in both the synchronous and promised result types.

Useful? React with 👍 / 👎.

[pkaminski]

Fixed the synchronous and promised return types using OnSendResult = number | null | void. A strict TypeScript consumer check now accepts numeric, null, undefined, no-return, async, and mixed-branch callbacks, while still rejecting strings and object results. The nullish/no-return cases failed against the previous declaration.

[devin-ai-integration]

Devin Review found 1 potential issue.

[devin-ai-integration]

🔴 Zero timeout poisons cached GET requests

When a cacheable GET has a zero timeout, send rejects but leaves its promise in the cache. Later GETs reuse that rejection without sending a request, even with a positive timeout.

Learn more

A cacheable GET stores its in-flight promise after starting send in request and caches it. The zero-timeout branch rejects outside the response error handler, so no handler deletes that promise. The next GET passes the cache's pending-promise check and returns the same rejected promise from request, even though its own timeout permits a send.

Example: A GET for /items with timeout: 0 rejects. A later GET for /items with timeout: 1000 and the same cache also rejects with TimeoutError without calling fetch.

Recommended fix: Remove a cached in-flight entry when its request promise rejects, including pre-send rejections. Guard cleanup against deleting an entry installed by a newer request, and test consecutive GETs with the default cache and different timeouts.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[pkaminski]

Fixed with identity-checked cleanup when the request promise rejects. A rejected request can remove only its own in-flight entry, so a newer request's pending promise or completed response survives. Added default-cache recovery and concurrent-replacement regressions for both Node and browser execution; all 84 tests pass.

[pkaminski]

@pkaminski+CODX made 3 comments and resolved 3 discussions.
Reviewable status: 0 of 5 files reviewed, all discussions resolved.

[pkaminski]

Fixed. Rejected request promises now remove their cache entry only if it still contains that same promise. The timeout suite now runs with caching enabled and covers recovery after zero options, sync/async zero callbacks, and a throwing callback. It also verifies that a late rejection preserves both a newer in-flight promise and a newer completed response. The eight recovery cases failed before this fix; all 84 Hubkit tests now pass.

[pkaminski]

Fixed with identity-checked cleanup when the request promise rejects. A rejected request can remove only its own in-flight entry, so a newer request's pending promise or completed response survives. Added default-cache recovery and concurrent-replacement regressions for both Node and browser execution; all 84 tests pass.

[pkaminski]

Fixed the synchronous and promised return types using OnSendResult = number | null | void. A strict TypeScript consumer check now accepts numeric, null, undefined, no-return, async, and mixed-branch callbacks, while still rejecting strings and object results. The nullish/no-return cases failed against the previous declaration.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f0e04660c2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Enforce zero timeouts on cached GETs

When the GET cache already contains an unexpired value or in-flight promise, request() returns through the cache fast path at lines 198–216 before send() reaches this check. Consequently, request('/x', {timeout: 0}) can resolve from cache—or even wait for an ongoing fetch—instead of immediately rejecting with the TimeoutError promised by the new public contract; apply the zero-timeout semantics to the cache path as well.

Useful? React with 👍 / 👎.

[pkaminski]

The timeout applies to each outbound HTTP attempt, so keeping the cache fast path is intentional. Positive timeouts already do not bound a cache hit or the wait for an existing in-flight request, and neither path invokes the second caller's onSend. A zero timeout follows that same scope. Checking it before the cache lookup would also bypass the documented ability for onSend to override an options timeout.

The README wording was too broad; clarified it in e5e3acf to explicitly cover cached responses and shared in-flight requests. Verified all eight combinations of Node/browser, completed/in-flight cache entries, and zero/positive timeouts: the cached result was reused with one fetch total and no callbacks for the second caller. For in-flight entries, the second caller remained pending beyond its positive timeout and resolved when the original request completed.

[pkaminski]

Revisited this: a completed cached response and waiting for an in-flight request should behave differently. Implemented a caller-local timeout for the latter in 7835d3f. Zero rejects immediately; a positive options.timeout races the shared promise, and its timer is cleared when either side settles. A wait timeout neither aborts the underlying fetch nor removes its cache entry, so the original caller and other waiters can still succeed.

onSend remains tied to actual sends, so sharing uses the joining caller's options timeout and invokes none of its send/receive/error callbacks. Completed cached responses remain available regardless of timeout. Updated the README and PR description accordingly.

All 96 tests and lint pass. Twelve new Node/browser regressions failed before this change and now cover explicit/default zero and positive timeouts, continued success for other callers using the same fetch, and timer cleanup after either shared fulfillment or rejection.

[pkaminski]

The final implementation now shares individual fetches instead of a caller's processed request promise. Every caller runs its own onSend, derives its timeout from the callback/options, and applies its own error/retry policy. Zero still rejects before sending or joining; positive timeouts follow normal per-caller error handling. Even the initiating caller can time out without aborting a fetch another caller still needs.

onReceive(call, shared) identifies shared attempts, which report zero incremental quota cost. Each caller receives its own metadata and parsed data; errors are copied before annotations or recovery handlers run. The server forwards the sharing flag and records shared wait latency separately. Completed cache hits retain their existing behavior.

All 132 Hubkit tests pass, including 36 new Node/browser cases covering the independent lifecycles and cache/transport races. The client needs no code changes: 92 existing tests passed with the candidate library, and real diff-worker fetchFile calls shared one fetch while retaining each caller's charset metadata and error policy. The PR description and README now describe this final behavior.

[pkaminski]

@pkaminski+CODX made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 5 files reviewed, all discussions resolved.

[pkaminski]

The timeout applies to each outbound HTTP attempt, so keeping the cache fast path is intentional. Positive timeouts already do not bound a cache hit or the wait for an existing in-flight request, and neither path invokes the second caller's onSend. A zero timeout follows that same scope. Checking it before the cache lookup would also bypass the documented ability for onSend to override an options timeout.

The README wording was too broad; clarified it in e5e3acf to explicitly cover cached responses and shared in-flight requests. Verified all eight combinations of Node/browser, completed/in-flight cache entries, and zero/positive timeouts: the cached result was reused with one fetch total and no callbacks for the second caller. For in-flight entries, the second caller remained pending beyond its positive timeout and resolved when the original request completed.

[pkaminski]

@pkaminski+CODX made 1 comment.
Reviewable status: 0 of 5 files reviewed, all discussions resolved.

[pkaminski]

Revisited this: a completed cached response and waiting for an in-flight request should behave differently. Implemented a caller-local timeout for the latter in 7835d3f. Zero rejects immediately; a positive options.timeout races the shared promise, and its timer is cleared when either side settles. A wait timeout neither aborts the underlying fetch nor removes its cache entry, so the original caller and other waiters can still succeed.

onSend remains tied to actual sends, so sharing uses the joining caller's options timeout and invokes none of its send/receive/error callbacks. Completed cached responses remain available regardless of timeout. Updated the README and PR description accordingly.

All 96 tests and lint pass. Twelve new Node/browser regressions failed before this change and now cover explicit/default zero and positive timeouts, continued success for other callers using the same fetch, and timer cleanup after either shared fulfillment or rejection.

[pkaminski]

@pkaminski partially reviewed 4 files and all commit messages.
Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on pkaminski).

[pkaminski]

@pkaminski+CODX made 1 comment.
Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on pkaminski).

[pkaminski]

The final implementation now shares individual fetches instead of a caller's processed request promise. Every caller runs its own onSend, derives its timeout from the callback/options, and applies its own error/retry policy. Zero still rejects before sending or joining; positive timeouts follow normal per-caller error handling. Even the initiating caller can time out without aborting a fetch another caller still needs.

onReceive(call, shared) identifies shared attempts, which report zero incremental quota cost. Each caller receives its own metadata and parsed data; errors are copied before annotations or recovery handlers run. The server forwards the sharing flag and records shared wait latency separately. Completed cache hits retain their existing behavior.

All 132 Hubkit tests pass, including 36 new Node/browser cases covering the independent lifecycles and cache/transport races. The client needs no code changes: 92 existing tests passed with the candidate library, and real diff-worker fetchFile calls shared one fetch while retaining each caller's charset metadata and error policy. The PR description and README now describe this final behavior.

[pkaminski]

@pkaminski reviewed 6 files and all commit messages, and made 4 comments.
Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on pkaminski).

---

README.md line 184 at r5 (raw file):

* `host`: The URL to prepend to all request paths; defaults to `https://api.github.com`.
* `graphHost`: The URL to use for all GraphQL requests; defaults to using the value of `host` which works fine for `github.com`, but you'll need to set a separate value when working with GitHub Enterprise.
* `timeout`: The timeout in milliseconds for each attempt, including waiting for a shared fetch; none by default.  Each caller runs its own `onSend` to override this timeout, then enforces the resulting budget independently.  Zero rejects immediately before sending or joining, without invoking `onReceive` or `onError`.  A positive timeout produces a `TimeoutError` and follows the caller's normal error/retry policy.  Timing out does not abort a fetch while another caller is waiting; a pending fetch is aborted when its last caller leaves.  An already completed cached response is returned regardless of the timeout.

I think a zero timeout should still invoke onError.

---

README.md line 190 at r5 (raw file):

You can use the (approximate) `size` field to help your cache determine when to evict items, but note that it tends to underestimate the actual size size of the object by 3-4x.  The
default cache is set to hold ~10MB of the measured bytes amount (so ~30-40MB of actual memory usage).
While requests are being processed, the cache can also contain internal `{pending: number, size: 100}` entries.  Concurrent callers share individual fetches, but run their own callbacks, response parsing, pagination, and error/retry policies.  Pending entries are removed when all their callers finish unless replaced by a completed response; rejected promises are never cached.

Why can't response parsing be shared?

---

hubkit.js line 573 at r5 (raw file):

                flights = inFlightRequests.get(cacheEntry);
                if (!flights) inFlightRequests.set(cacheEntry, flights = new Map());
                requestKey = computeCacheKey(path, options) + '\n' + JSON.stringify(config.headers);

Why recompute the cache key instead of reusing cacheKey here?

---

hubkit.js line 594 at r5 (raw file):

              // Each caller parses its own data: pagination and error handlers may mutate it.
              rawData = options.responseType === 'arraybuffer' && response.status < 400 ?
                response.rawData.slice(0) : response.rawData;

Do we actually mutate arraybuffer responses anywhere? If not, I'd rather prohibit doing so (in docs or by freezing it somehow) than make a defensive copy, since buffer-type responses are likely to be used precisely when there's a lot of data to fetch.

[pkaminski]

@pkaminski+CODX made 4 comments and resolved 4 discussions.
Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on pkaminski).

---

hubkit.js line 573 at r5 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

Why recompute the cache key instead of reusing cacheKey here?

cacheKey stays fixed for the complete result, but automatic pagination changes path and clears/updates options.body. Callers can be on different pages at the same time, so the transport key must identify the current page. Reusing the original key could make a delayed first-page caller join an active second-page fetch. Added an explanatory comment and a Node/browser regression that exercises exactly that overlap.

---

hubkit.js line 594 at r5 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

Do we actually mutate arraybuffer responses anywhere? If not, I'd rather prohibit doing so (in docs or by freezing it somehow) than make a defensive copy, since buffer-type responses are likely to be used precisely when there's a lot of data to fetch.

Hubkit doesn't mutate ArrayBuffer contents, and I found no ArrayBuffer consumers in the server/client source; the diff worker uses Blob. Removed the defensive copy and documented that callers must treat buffers as read-only, copying before mutation, resizing, or transfer/detachment. A Node/browser regression verifies that both callers receive the original buffer and the body is read only once.

---

README.md line 184 at r5 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

I think a zero timeout should still invoke onError.

Changed both zero-timeout paths to invoke onError with a TimeoutError. They still skip onReceive and never send or join a fetch. The default behavior rejects without an automatic network retry; an error handler can recover, throw, or explicitly return RETRY, bounded by maxTries. Added Node/browser coverage for those policies and updated the docs.

---

README.md line 190 at r5 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

Why can't response parsing be shared?

The body read is shared. JSON decoding could also be shared, but we'd then need a deep copy before caller-specific processing: pagination attaches next and edits GraphQL connection fields, and onError can mutate the decoded response. Parsing the shared text per caller gives each lifecycle its own JSON objects without adding separate cloning logic. I've clarified that distinction in the README; raw buffers now stay shared without copying.

[pkaminski]

@pkaminski reviewed 6 files and all commit messages.
Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on pkaminski).

[pkaminski]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9fee48b9f4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Attribute quota cost after the original caller times out

When the caller that created a flight times out while another caller is still waiting, its onReceive runs with no response or cost, but the surviving caller later has shared === true, so this line reports cost 0. The completed HTTP request's quota cost is therefore never reported to any onReceive callback, causing consumers that account for GitHub quota through this callback to undercount. Transfer cost attribution to one surviving caller when the original transport owner leaves.

Useful? React with 👍 / 👎.

[pkaminski]

Fixed by recording cost attribution on the shared flight. Its first response callback reports the quota cost, and later callbacks report zero. A timed-out caller never claims that cost, so a survivor can report it while retaining shared === true. The claim is made before invoking the callback so a throwing callback cannot cause duplicate attribution; a caller without onReceive also cannot consume the claim. The Node/browser regression failed before the fix and now passes with two surviving callers (costs 1 and 0).

Rebased onto current master and integrated its header-time quota observations for all shared callers. Late joiners retain the original header timestamp and cannot overwrite equal-time or newer quota metadata. All 361 Hubkit tests, lint, the 20 server integration tests, and 96 client compatibility tests pass against the candidate; the real diff-worker probe also passes.

[pkaminski]

@pkaminski+CODX made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 6 files reviewed, all discussions resolved.

[pkaminski]

Fixed by recording cost attribution on the shared flight. Its first response callback reports the quota cost, and later callbacks report zero. A timed-out caller never claims that cost, so a survivor can report it while retaining shared === true. The claim is made before invoking the callback so a throwing callback cannot cause duplicate attribution; a caller without onReceive also cannot consume the claim. The Node/browser regression failed before the fix and now passes with two surviving callers (costs 1 and 0).

Rebased onto current master and integrated its header-time quota observations for all shared callers. Late joiners retain the original header timestamp and cannot overwrite equal-time or newer quota metadata. All 361 Hubkit tests, lint, the 20 server integration tests, and 96 client compatibility tests pass against the candidate; the real diff-worker probe also passes.

[pkaminski]

@pkaminski reviewed 7 files and all commit messages.
Reviewable status: 0 of 8 files reviewed, all discussions resolved.

[pkaminski]

@pkaminski reviewed 7 files and all commit messages, and made 1 comment.
Reviewable status: 0 of 9 files reviewed, 1 unresolved discussion.

---

hubkit.js line 851 at r9 (raw file):

        const api = detectApi(config.url);
        const data = api === 'graph' ?
          parseResponseData(rawData, response.headers, options, response.status) : undefined;

For the majority case where a request is not shared, will we end up parsing the data twice? Is there a way to optimize that without introducing complex logic?

[pkaminski]

@pkaminski+CODX made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 9 files reviewed, all discussions resolved.

---

hubkit.js line 851 at r9 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

For the majority case where a request is not shared, will we end up parsing the data twice? Is there a way to optimize that without introducing complex logic?

Yes: GraphQL responses were parsed once to read the cost and again for the result. REST responses were only parsed once. The transport now returns its parsed data alongside the raw body, and the initiating caller reuses it. Only joined callers parse separate copies, preserving mutation isolation without another flag or ownership mechanism.

Added Node/browser tests that count JSON parses: one for an unshared REST or GraphQL request with onReceive, and two for two shared REST callers whose returned objects remain independent. All 373 Hubkit tests and lint pass.

[pkaminski]

@pkaminski reviewed 3 files and all commit messages.
Reviewable status: 0 of 9 files reviewed, all discussions resolved.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 93344371e7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Isolate quota-assignment failures per caller

When a shared GET includes quota headers and one waiter supplies metadata with a read-only property or throwing setter, this Object.assign throws inside the transport's try, so the valid HTTP response is converted into a network failure for every waiter (and may be retried repeatedly). A sibling caller with ordinary metadata therefore rejects even though only the other caller's metadata update failed; handle metadata-assignment errors per caller rather than rejecting the shared flight.

Useful? React with 👍 / 👎.

[pkaminski]

Given that all callers will be from the same app, and that supplying a broken metadata object is a bug that needs fixing anyway, I don't think it's worth the extra complexity to decrease the blast radius.

[pkaminski]

@pkaminski made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 9 files reviewed, all discussions resolved (waiting on snoack).

[pkaminski]

Given that all callers will be from the same app, and that supplying a broken metadata object is a bug that needs fixing anyway, I don't think it's worth the extra complexity to decrease the blast radius.

[snoack]

@snoack reviewed 9 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pkaminski).

---

hubkit.js line 229 at r10 (raw file):

          error.request = {method: options.method, url: path, headers};
          if (cacheable && res) {
            if (options.stats) options.stats.record(shared);

Two concurrent callers that both get a 500 record one hit and one miss, so hitRate improves as error fan-out grows. The README documents hits for errors handled by onError/ifNotFound/ifGone, and those two sites record deliberately. Was an unhandled 5xx meant to count as a hit as well?

---

index.d.ts line 72 at r10 (raw file):

  cache?: LRUCache<
    string,
    {pending: number, size: number} |

Pending entries also carry cachedItem (cacheEntry = {pending: 0, cachedItem, size: 100}), and the README's {pending: number, size: 100} description omits it too. Worth adding?

[pkaminski]

@pkaminski+CODX made 2 comments and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on pkaminski).

---

hubkit.js line 229 at r10 (raw file):

Previously, snoack (Sebastian Noack) wrote…

Two concurrent callers that both get a 500 record one hit and one miss, so hitRate improves as error fan-out grows. The README documents hits for errors handled by onError/ifNotFound/ifGone, and those two sites record deliberately. Was an unhandled 5xx meant to count as a hit as well?

Yes. The metric measures response reuse: two callers receiving one shared 500 avoided a second fetch, so the joining caller counts as a hit regardless of whether it recovers, retries, or rejects. The previous pending-promise implementation also recorded a hit when a joined promise rejected.

The README was too narrow, so I made that contract explicit, including per-attempt accounting on retries. Added Node/browser regressions for two callers rejecting a shared 500 both immediately and after exhausting retries. They assert one miss and one hit per physical attempt, with no rejected response retained in the cache. All 381 tests pass.

---

index.d.ts line 72 at r10 (raw file):

Previously, snoack (Sebastian Noack) wrote…

Pending entries also carry cachedItem (cacheEntry = {pending: 0, cachedItem, size: 100}), and the README's {pending: number, size: 100} description omits it too. Worth adding?

Added cachedItem to both the pending-entry declaration and README. The completed entry shape is factored into CachedResponse so it is shared by the two cache-entry variants. The docs explain that cachedItem retains the previous completed entry for conditional requests and 304 restoration.

A strict TypeScript consumer check covers writing completed/pending entries and reading entry.cachedItem?.status after narrowing the union.

[snoack]

@snoack reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on pkaminski).

[pkaminski]

@pkaminski reviewed 3 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on pkaminski).