Skip to content

Cache.get retains zero-TTL entries, allowing expired results to evict valid entries #8483

Description

@joaoeira

What version of Effect is running?

effect@4.0.0-rc.117 (latest published v4 RC when checked), also reproduced on 4.0.0-rc.112.

Reproduction tested with Bun 1.2.21 and Node.js v24.7.0 on macOS arm64.

What steps can reproduce the bug?

Cache.get retains completed zero-TTL lookups as immediately expired entries. They still occupy capacity and can evict valid entries on a later insertion, even though subsequent reads would not reuse their results.

In a new directory:

npm init -y
npm install --save-exact effect@4.0.0-rc.117

Save as repro.ts and run bun repro.ts (or node repro.ts with Node.js v24.7.0):

import { Cache, Duration, Effect, Exit } from "effect"

const program = Effect.gen(function* () {
  const cache = yield* Cache.makeWith(
    (key: string) =>
      key === "bad" ? Effect.fail("unavailable") : Effect.succeed(key),
    {
      capacity: 2,
      timeToLive: (exit) =>
        Exit.isFailure(exit) ? Duration.zero : "1 hour",
    },
  )

  yield* Cache.get(cache, "a")
  yield* Effect.exit(Cache.get(cache, "bad"))

  // The failed lookup has completed. Do not enumerate keys here:
  // iteration would remove the expired entry and mask the eviction.
  yield* Cache.get(cache, "b")

  console.log("active keys:", [...(yield* Cache.keys(cache))])
})

Effect.runPromise(program)

What is the expected behavior?

active keys: [ "a", "b" ]

The completed zero-TTL failure should be removed rather than remain as a capacity-consuming entry, consistent with zero-TTL handling in Cache.set and Cache.refresh.

This report concerns retention after lookup completion, not whether pending lookups should count toward capacity.

What do you see instead?

active keys: [ "b" ]

The expired "bad" entry occupies a slot. Inserting "b" evicts the older, still-valid "a" entry. Enumerating keys subsequently removes "bad", leaving only "b".

Additional information

Unreachable zero-TTL branch

In Cache.get's lookup observer:

const ttl = self.timeToLive(exit, key)
if (Duration.isFinite(ttl)) {
  entry.expiresAt = fiber.getRef(effect.ClockRef).currentTimeMillisUnsafe() + Duration.toMillis(ttl)
} else if (Duration.isZero(ttl)) {
  MutableHashMap.remove(self.map, key)
}

Duration.isFinite(Duration.zero) is true, so the removal branch cannot run. Cache.set and Cache.refresh check zero first instead.

The same code remains on main at 81a0b3aaae80b1f6918d9a5935946acefe4c92ee, Cache.ts:434–457.

Why simply swapping the branches is unsafe

I tested an isolated rc.117 copy with only these two branches swapped. This introduces two problems:

  1. Synchronous completion: EntryImpl forks immediately, and get registers the observer before inserting the entry. addObserver invokes the callback immediately for a completed fiber. Removing the key at this point does not prevent the subsequent insertion. The new entry retains expiresAt === undefined, making the zero-TTL success or failure non-expiring until removed or evicted. In a fail-once-then-succeed lookup, subsequent get calls consequently keep returning the initial failure without attempting recovery.
  2. Replacement ownership: for a suspended lookup, a newer value can be installed under the same key using Cache.set before the lookup completes. An unconditional zero-TTL removal then deletes that newer replacement. Removal needs to check that the entry being removed is still the relevant one.

A fix needs to account for completion both before and after insertion, and for replacement ownership.

Verification and scope

  • Tested synchronous and Deferred-suspended lookups, both successes and failures, with zero TTL.
  • Verified repeated get calls recompute on the unmodified implementation rather than reuse zero-TTL results.
  • Verified valid-entry eviction after completed zero-TTL lookups.
  • Verified zero-TTL removal by Cache.set and Cache.refresh; refresh coverage included existing/missing keys and synchronous/suspended success/failure results.
  • Ran 24 diagnostic tests against each of unmodified rc.112, unmodified rc.117, and the isolated branch-swap experiment. Suspended cases used Deferred synchronization and time-based checks used TestClock.

Cache.size explicitly documents that it counts expired entries, so a size/keys discrepancy alone is not the claimed contract violation. Also, Cache.keys returns a lazy iterable whose iteration removes expired entries; enumerating keys between the failed lookup and the next insertion masks this particular eviction scenario.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions