Skip to content

fix(replication): propagate per-event expiresAt to record writes - #640

Merged
kriszyp merged 1 commit into
mainfrom
fix/replicated-record-expiration-eviction-main
May 21, 2026
Merged

fix(replication): propagate per-event expiresAt to record writes#640
kriszyp merged 1 commit into
mainfrom
fix/replicated-record-expiration-eviction-main

Conversation

@kriszyp

@kriszyp kriszyp commented May 20, 2026

Copy link
Copy Markdown
Member

Problem

Records received via replication were not being evicted by the cleanup scanner even when they carried an expiresAt value. Three issues (same as #639 which backports this to v5.0):

1. expiresAt missing from options passed to _writeUpdate

In a multi-record transaction batch, every event after the first is processed with txnInProgress (the first event) as context. Subsequent events had no expiresAt in their options, so _writeUpdate saw context.expiresAt from the first record applied to all records — or saw nothing if the first record had no expiration.

2. _writeUpdate didn't read expiresAt from options

// before
const expiresAt: number = context?.expiresAt ?? (expirationMs ? expirationMs + Date.now() : -1);

// after
const expiresAt: number = options?.expiresAt ?? context?.expiresAt ?? (expirationMs ? expirationMs + Date.now() : -1);

3. scheduleCleanup() not armed for replicated writes

// before
if (context.expiresAt) scheduleCleanup();

// after
if (expiresAt >= 0) scheduleCleanup();

See also


Signed-off by Claude

Three issues prevented replicated records from being evicted:

1. The `options` object passed to `_writeUpdate` for each replicated
   event did not include `expiresAt`. In a multi-record transaction batch
   every event after the first uses the first event as `context`, so
   `context.expiresAt` would silently apply the wrong expiration (or
   none) to subsequent records.

2. `_writeUpdate` read `expiresAt` only from `context`, so per-event
   options were ignored. Now: `options?.expiresAt ?? context?.expiresAt ?? fallback`.

3. `scheduleCleanup()` was only armed when `context.expiresAt` was
   truthy, missing replicated writes that carry expiration via options.
   Changed to `if (expiresAt >= 0)` to cover both paths.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread resources/Table.ts
if (options?.residencyId != undefined) residencyId = options.residencyId;
const expiresAt: number = context?.expiresAt ?? (expirationMs ? expirationMs + Date.now() : -1);
const expiresAt: number =
options?.expiresAt ?? context?.expiresAt ?? (expirationMs ? expirationMs + Date.now() : -1);

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.

Missing test for the new options.expiresAt path

What: options?.expiresAt ?? is a new first-priority branch. In the replication loop it's populated from event.expiresAt (line 331); for local writes the third arg to the static put call flows into context.expiresAt via the transactional() wrapper, so every existing test in caching.test.js exercises the context?.expiresAt fallback, not this new leg.

Why it matters: A regression to this path — e.g. silently reverting the resolution order — would not be caught by any existing test. This is specifically the path the PR claims to fix.

Suggested fix: Add a test that calls _writeUpdate with options.expiresAt set but no context.expiresAt, and asserts the stored entry carries the correct expiresAt value (and that the cleanup scanner is armed). The "Handles eviction-only config without expiration" shape in caching.test.js is a reasonable model.

@claude

claude Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

1 blocker: the new options?.expiresAt branch in _writeUpdate (line 1705) has no test coverage — existing tests flow expiry through context.expiresAt via the transactional() wrapper, so a regression to the replication path wouldn't be caught. Details in the inline comment.

@kriszyp
kriszyp merged commit 351318a into main May 21, 2026
38 checks passed
@kriszyp
kriszyp deleted the fix/replicated-record-expiration-eviction-main branch May 21, 2026 20:45
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.

2 participants