Round the Redis key–value store TTL up to whole seconds

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
 --------------

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]

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();
+    }
+  },
+);

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 {