Separate background fetch state from cache values

[Sean-Kenneth-Doherty]

Summary

• keep #valList value-only and store in-flight fetch promises in #fetchList
• move fetch abort controllers and returned-state tracking into side lists instead of decorating promises
• update fetch internals/tests for stale values, inflight fetches, and fetch-option TTL initialization

Fixes #287.

Tests

• npm run prepare
• npx oxlint src test
• npx prettier --check src/index.ts test/fetch.ts tap-snapshots/test/tracing.ts.test.cjs
• npx tap --statements=0 --branches=0 --lines=0 --functions=0 test/fetch.ts
• npx tap --timeout=120 --statements=0 --branches=0 --lines=0 --functions=0 test/tracing.ts
• npx tap --timeout=120 --reporter=terse

[Sean-Kenneth-Doherty]

Validation for head 3d2cc8e7e4ffdecec09f6440a753fd479d62a053:

• npm run prepare
• npx oxlint src test
• npx prettier --check src/index.ts test/fetch.ts tap-snapshots/test/tracing.ts.test.cjs
• npx tap --statements=0 --branches=0 --lines=0 --functions=0 test/fetch.ts -> 195 assertions passed
• npx tap --timeout=120 --statements=0 --branches=0 --lines=0 --functions=0 test/tracing.ts -> 1,818 assertions passed
• npx tap --timeout=120 --reporter=terse -> 27 suites / 19,615 assertions passed

I also checked the worktree after prepare and the test runs; no generated-file drift was left behind.

[isaacs]

This is an ok approach, but it has some problems that need to be fixed prior to landing.

In general, separating out the backgroundFetch add-on properties to separate lists is the goal. But there's some slop that I don't think got proper human attention. In addition to the agent's findings below, the test/avoid-memory-leak.ts test now runs so slowly that it times out, or finishes only after more than 25s or so. Prior to this commit, that test completes in 4s. Please make sure to run npm test locally, without disabling coverage or timeouts. Those are there for a reason.

I haven't done a comprehensive benchmark yet, but if the tests time out... something is probably not great in there 😅

tl;dr summary:

• Ensure that npm test runs in a reasonable amount of time, and does not cause new timeouts or take longer than tests run on main. (Several tests are designed specifically to exercise high-load use cases, so if something lags or leaks memory in those tests, that is a problem.)
• DRY the duplicated code in #set and #setBackgroundFetch.
• Remove the dead branch tests in pop().
• Clean up the race condition around returnedFetch.

Here's what Claude had to say about it, more verbose and detailed:

---

Issues

1. #returnedFetch is keyed by slot, not by the bf — narrow but real race

src/index.ts:1820-1825, :2554

The old __returned lived on the bf object. The new flag lives in this.#returnedFetch[index]. If a bf is returned to a caller, then the slot is reused by another #set or another fetch before the original bf settles, the closure-captured slot index now points at unrelated state. In fetchFail:

const returned = !!this.#returnedFetch?.[i]

#setBackgroundFetch resets returnedFetch[i] = 0 for the new occupant, so the original bf — which was returned — will read returned === 0 on rejection and silently resolve to undefined instead of throwing. The user awaiting the original promise gets the wrong outcome.

Fix: capture returned as a per-bf closure variable. #returnBackgroundFetch would still take (index, bf) but flip a local flag (e.g., via a WeakMap<bf, {returned: boolean}>, or just attach a Symbol-keyed property to the bf — symbol-keyed props don't pollute the public type). The slot-indexed array is the wrong granularity here.

2. onInsert('add') is silently dropped for fetch-initiated entries

src/index.ts:2243, :2605-2628

Previously, starting a fetch for a new key went through #set(k, bf, …) which fired onInsert(bf, k, 'add') — weird, because bf is a Promise, but it did fire. The inlined new-entry path in #backgroundFetch doesn't call onInsert at all. When the fetch later resolves, #set takes the update branch with oldVal === undefined, so it fires onInsert(v, k, 'replace').

Net effect: users observing inserts via onInsert will never see an 'add' reason for fetch-created entries — only 'replace'. That's a behavior change. The intuitive fix is to call onInsert(v, k, 'add') from #backgroundFetch's cb when we know the entry was newly created by the fetch path (i.e., track a wasNewEntry flag from the index === undefined branch and let the resolution code emit 'add' instead of 'replace').

3. Inlined new-entry creation in #backgroundFetch duplicates #set's addition path

src/index.ts:2605-2628

It's a near copy-paste of #set's addition logic (src/index.ts:2225-2244), minus onInsert, minus start, minus maxEntrySize, minus #requireSize. Most of those omissions are fine for a placeholder (size 0, no value yet), but the duplication is a maintenance hazard — any future fix to the addition path has to be applied here too. Worth extracting a #addEntry(k, v, options) helper used by both call sites; it would also let you keep onInsert('add') semantics centralized.

Minor: setItemTTL(index, ttl) drops a user-supplied fetchOpts.options.start. Probably intentional, but the asymmetry with #set deserves a one-line comment if so.

4. pop() has dead-equivalent branches

src/index.ts:2310-2318

if (fetching && val !== undefined) {
  return val
} else if (!fetching && val !== undefined) {
  return val
}

Both branches do the same thing. Either collapse to if (val !== undefined) return val and drop the fetching capture, or actually do something different in the two cases (presumably whatever distinction the author had in mind got lost). Not a bug, just confusing code.

5. #fetchList / #abortControllerList allocation via Array.from({length}).fill(undefined)

src/index.ts:1781-1789

Functionally fine, but new Array(this.#max) does the same with one allocation instead of two. Minor.

6. #clear does not reset #returnedFetch

src/index.ts:3183-3198

Since #setBackgroundFetch resets per-entry on every new fetch, this is currently harmless. But it's inconsistent with the explicit #fetchList?.fill(undefined) / #abortControllerList?.fill(undefined) calls right above it, and it would mask the issue from #1 in the rare case where clear() runs between return and rejection.

---

[isaacs]

This is likely at least part of the perf regression. On line 2335, you're doing this:

  const bf = this.#abortBackgroundFetch(head, new Error('evicted'))

Previously, the code only created an Error object if there is actually a backgroundFetch to abort.

  if (this.#hasFetchMethod && this.#isBackgroundFetch(v)) {
    v.__abortController.abort(new Error('evicted'))
  }

Creating that Error object is costly, and you're doing it on every delete, evict, etc., even if there's no background fetch in progress.