diff --git a/CHANGES.md b/CHANGES.md index 102e01106..50cfd46f6 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,21 @@ Version 2.0.28 To be released. +### @fedify/redis + + - Fixed `RedisKvStore.set()` failing when the `ttl` option was not a whole + number of seconds. The duration was handed to Redis `SETEX` unchanged, and + `SETEX` takes only whole seconds, so the write was rejected with + `ERR value is not an integer or out of range` instead of being stored with + a rounded expiry. The TTL is now rounded up to the next whole second. A + zero or negative duration, which `SETEX` also rejects, now stores the value + for one second, the shortest expiry that command can express. The + one-second granularity is `SETEX`'s rather than Redis's; `SET` with `PX` + supports millisecond expiries. [[#1028], [#1034] by Heewon Chae\] + +[#1028]: https://github.com/fedify-dev/fedify/issues/1028 +[#1034]: https://github.com/fedify-dev/fedify/issues/1034 + Version 2.0.27 -------------- diff --git a/changes.d/redis/ttl-whole-seconds.md b/changes.d/redis/ttl-whole-seconds.md new file mode 100644 index 000000000..841b5832e --- /dev/null +++ b/changes.d/redis/ttl-whole-seconds.md @@ -0,0 +1,9 @@ + - Fixed `RedisKvStore.set()` failing when the `ttl` option was not a whole + number of seconds. The duration was handed to Redis `SETEX` unchanged, and + `SETEX` takes only whole seconds, so the write was rejected with + `ERR value is not an integer or out of range` instead of being stored with + a rounded expiry. The TTL is now rounded up to the next whole second. A + zero or negative duration, which `SETEX` also rejects, now stores the value + for one second, the shortest expiry that command can express. The + one-second granularity is `SETEX`'s rather than Redis's; `SET` with `PX` + supports millisecond expiries. [[#1028], [#1034] by Heewon Chae] diff --git a/packages/redis/src/kv.test.ts b/packages/redis/src/kv.test.ts index 729854b1c..b63cf27b6 100644 --- a/packages/redis/src/kv.test.ts +++ b/packages/redis/src/kv.test.ts @@ -1,8 +1,13 @@ import { test } from "@fedify/fixture"; import { RedisKvStore } from "@fedify/redis/kv"; +import * as temporal from "@js-temporal/polyfill"; +import type { Redis as RedisClient, RedisKey } from "ioredis"; import { Redis } from "ioredis"; import assert from "node:assert/strict"; import process from "node:process"; +import { test as nodeTest } from "node:test"; + +const Temporal = globalThis.Temporal ?? temporal.Temporal; const redisUrl = process.env.REDIS_URL; const ignore = redisUrl == null; @@ -144,3 +149,191 @@ test("RedisKvStore.list() - empty prefix", { ignore }, async () => { redis.disconnect(); } }); + +// Regression tests for `RedisKvStore.set()` handing Redis `SETEX` a TTL that +// is not a whole number of seconds. +// +// `options.ttl.total("second")` was passed straight through, so any duration +// that is not an exact number of seconds — which `KvStoreSetOptions.ttl` +// accepts, since it is any `Temporal.Duration` — made the server reject the +// write with `ERR value is not an integer or out of range` rather than storing +// the value with a rounded expiry. A zero duration failed too, with `ERR +// invalid expire time in 'setex' command`. +// +// See: https://github.com/fedify-dev/fedify/issues/1028 + +/** + * A stand-in for the Redis client that records the arguments `set()` hands to + * `SETEX`. It exists so the conversion can be checked on every runtime, + * including the ones with no `REDIS_URL`; the end-to-end behaviour is covered + * by the `REDIS_URL`-gated test below. + */ +function recordingRedis(): { + setexCalls: { key: RedisKey; seconds: unknown }[]; + redis: RedisClient; +} { + const setexCalls: { key: RedisKey; seconds: unknown }[] = []; + const client = { + setex(key: RedisKey, seconds: unknown, _value: unknown): Promise<"OK"> { + setexCalls.push({ key, seconds }); + return Promise.resolve("OK"); + }, + }; + return { setexCalls, redis: client as unknown as RedisClient }; +} + +nodeTest("RedisKvStore.set() rounds a TTL up to whole seconds", async () => { + const cases: [Temporal.Duration, number, string][] = [ + [Temporal.Duration.from({ seconds: 1 }), 1, "a whole second is unchanged"], + [ + Temporal.Duration.from({ minutes: 5 }), + 300, + "whole seconds are unchanged", + ], + [Temporal.Duration.from({ milliseconds: 1500 }), 2, "1.5s rounds up"], + [ + Temporal.Duration.from({ milliseconds: 1400 }), + 2, + "1.4s rounds up, not to the nearest second", + ], + [ + Temporal.Duration.from({ milliseconds: 500 }), + 1, + "a sub-second TTL becomes the smallest expiry", + ], + [ + Temporal.Duration.from({ milliseconds: 1 }), + 1, + "a near-zero TTL stays at least 1", + ], + [Temporal.Duration.from({ seconds: 0 }), 1, "a zero TTL stays at least 1"], + [ + Temporal.Duration.from({ milliseconds: -1 }), + 1, + "a negative sub-second TTL stays at least 1", + ], + [ + Temporal.Duration.from({ seconds: -30 }), + 1, + "a negative TTL stays at least 1", + ], + [ + Temporal.Duration.from({ hours: -1 }), + 1, + "a large negative TTL stays at least 1", + ], + ]; + for (const [ttl, expected, why] of cases) { + const { setexCalls, redis } = recordingRedis(); + const store = new RedisKvStore(redis, { keyPrefix: "fedify_test::" }); + await store.set(["foo"], "bar", { ttl }); + assert.strictEqual(setexCalls.length, 1); + assert.strictEqual(setexCalls[0].seconds, expected, why); + assert( + Number.isInteger(setexCalls[0].seconds), + "SETEX only accepts whole seconds", + ); + assert( + (setexCalls[0].seconds as number) > 0, + "SETEX rejects a non-positive expiry", + ); + } +}); + +/** + * Asserts that a key written with a `seconds`-long expiry still has a + * plausible amount of that lifetime left. + * + * `PTTL` is read rather than `TTL` because `TTL` rounds the remaining + * lifetime to whole seconds: just over half a second after a `SETEX 1` write + * it reports `0` for a key that is still there, and `29` for one written with + * `SETEX 30`. Asserting an exact `TTL` therefore fails on a slow run even + * though the conversion is correct. + * + * `elapsedMs` must span everything from before the write to after the read, + * so it is an upper bound on how long the key has been alive, and the + * remaining lifetime cannot have fallen below `seconds * 1000 - elapsedMs`. + * The upper bound is what rules out a longer expiry than intended. + */ +function assertExpiresIn( + remainingMs: number, + seconds: number, + elapsedMs: number, + why: string, +): void { + assert( + remainingMs > 0, + `${why}: expected a live key with an expiry, but PTTL returned ` + + `${remainingMs}`, + ); + assert( + remainingMs <= seconds * 1000, + `${why}: expected at most ${seconds}s left, but PTTL returned ` + + `${remainingMs}ms`, + ); + assert( + remainingMs >= seconds * 1000 - elapsedMs, + `${why}: expected at least ${seconds * 1000 - elapsedMs}ms left ` + + `(${seconds}s minus the ${elapsedMs}ms the write and read took), but ` + + `PTTL returned ${remainingMs}ms`, + ); +} + +nodeTest( + "RedisKvStore.set() stores a sub-second TTL", + { skip: ignore }, + async () => { + if (ignore) return; // Bun does not support the skip option + const { redis, keyPrefix, store, cleanup } = getRedis(); + try { + // Before the fix this threw `ERR value is not an integer or out of + // range`. The expiry is read straight after the write, so only the two + // Redis commands sit inside the window the bounds have to tolerate. + let startedAt = Date.now(); + await store.set(["foo", "sub"], "bar", { + ttl: Temporal.Duration.from({ milliseconds: 500 }), + }); + let remaining = await redis.pttl(`${keyPrefix}foo::sub`); + assertExpiresIn( + remaining, + 1, + Date.now() - startedAt, + "a sub-second TTL should be stored as the smallest expiry SETEX accepts", + ); + assert.strictEqual(await store.get(["foo", "sub"]), "bar"); + + // A negative duration is stored for one second rather than rejected. + // `SETEX` refuses a non-positive expiry outright, so without the floor + // this throws `ERR invalid expire time in 'setex' command`. + startedAt = Date.now(); + await store.set(["foo", "negative"], "bar", { + ttl: Temporal.Duration.from({ seconds: -30 }), + }); + remaining = await redis.pttl(`${keyPrefix}foo::negative`); + assertExpiresIn( + remaining, + 1, + Date.now() - startedAt, + "a negative TTL should be stored as the smallest expiry SETEX accepts", + ); + assert.strictEqual(await store.get(["foo", "negative"]), "bar"); + + // A whole number of seconds keeps its value, so the rounding does not + // change what already worked. + startedAt = Date.now(); + await store.set(["foo", "whole"], "bar", { + ttl: Temporal.Duration.from({ seconds: 30 }), + }); + remaining = await redis.pttl(`${keyPrefix}foo::whole`); + assertExpiresIn( + remaining, + 30, + Date.now() - startedAt, + "a whole number of seconds should be stored unchanged", + ); + } finally { + await cleanup(); + redis.disconnect(); + } + }, +); diff --git a/packages/redis/src/kv.ts b/packages/redis/src/kv.ts index 758ee3bf4..012450764 100644 --- a/packages/redis/src/kv.ts +++ b/packages/redis/src/kv.ts @@ -8,6 +8,34 @@ import type { Cluster, Redis, RedisKey } from "ioredis"; import { Buffer } from "node:buffer"; import { type Codec, JsonCodec } from "./codec.ts"; +/** + * Turns a TTL into the whole number of seconds Redis `SETEX` requires. + * + * The one-second granularity is `SETEX`'s, not Redis's: Redis can express a + * millisecond expiry through `SET` with `PX`, and `SETEX` is simply the + * command this adapter uses. Within that command a duration which is not a + * whole number of seconds has to be approximated. + * + * It is rounded up rather than to the nearest second, because every other + * {@link KvStore} implementation keeps a value for at least as long as it was + * asked to, and expiring early is the direction that can change behaviour + * rather than just cost a refetch — a TTL used to suppress duplicate work + * would start letting duplicates through. + * + * The result is clamped to 1, the smallest expiry `SETEX` accepts. A + * sub-second duration therefore stores the value for one second instead of + * being rejected, and so do **zero and negative durations**, which `SETEX` + * rejects outright. That last part is a policy choice rather than a + * consequence of the rounding: the other {@link KvStore} implementations read + * a non-positive TTL as already expired, whereas this one keeps the value for + * the shortest lifetime the command can express. Storing it briefly is closer + * to the caller's request than failing the write, which is what happened + * before. + */ +function expirySeconds(ttl: Temporal.Duration): number { + return Math.max(1, Math.ceil(ttl.total("second"))); +} + /** * Options for {@link RedisKvStore} class. */ @@ -91,6 +119,15 @@ export class RedisKvStore implements KvStore { return this.#codec.decode(encodedValue) as T; } + /** + * {@inheritDoc KvStore.set} + * + * The `ttl` option is stored through Redis `SETEX`, which takes a whole + * number of seconds, so a duration with a finer resolution is rounded up to + * the next second. A zero or negative duration stores the value for one + * second, the shortest expiry the command can express, rather than failing + * the write or deleting the key. + */ async set( key: KvKey, value: unknown, @@ -101,7 +138,7 @@ export class RedisKvStore implements KvStore { if (options?.ttl != null) { await this.#redis.setex( serializedKey, - options.ttl.total("second"), + expirySeconds(options.ttl), encodedValue, ); } else {