Fixed KnexKvStore returning null for missing keys

[sagzy]

Summary

Fixes #1961 — follows between self-hosted instances failing with 401 Failed to verify the request signature.

KnexKvStore.get() returned null for missing keys, but Fedify's KvStore contract requires undefined when a key does not exist — null is a legitimate stored value. Since Fedify introduced persistent negative key caching (v2.1+), its KvKeyCache strictly distinguishes the two:

• undefined → cache miss → fetch the sender's key and verify
• null → "this key is known to be unavailable" → skip fetching for 10 minutes

Because our MySQL-backed store reported every missing key as null, every signing key that had never been cached was treated as a cached negative entry, so Fedify never fetched the sender's key and inbound HTTP signature verification failed with 401 for any new peer. This matches the reporter's logs exactly — on a freshly truncated key_value table, Fedify logs Entry ... found in cache, but it is unavailable, which is only possible if the store misreports misses as negatives:

DBG fedify·sig·key: Entry '...#main-key' found in cache, but it is unavailable.
DBG fedify·sig·key: Entry '...#main-key' found in cache, but no fetch failure details are available.
DBG fedify·sig·http: Failed to fetch key: '...'
WRN fedify·federation·http: 'POST' '/.ghost/activitypub/inbox/index': 401

("no fetch failure details are available" because there never was a fetch.)

Verification

Running Fedify's actual KvKeyCache against both store behaviours:

old KnexKvStore (missing -> null):      treated as cached NEGATIVE -> Fedify never fetches, verification 401s
fixed KnexKvStore (missing -> undefined): cache MISS -> Fedify fetches the key (correct)

Also fixed

Reading back a stored JSON null (Fedify persists null to mark failed key lookups, with a TTL) previously threw TypeError in Object.hasOwn(row.value, ...). It now correctly round-trips as null.

[coderabbitai]

Walkthrough

KnexKvStore.get() now returns undefined for missing and expired keys. Expired-row cleanup only deletes rows whose expiration remains past due, preventing deletion of concurrently refreshed entries. Integration tests were updated for unset, deleted, and expired keys while continuing to verify that explicitly stored null values remain null.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	It fixes the missing-key/null bug, but does not address the authorized-fetch hang/deadlock or Fedify upgrade requests in #1961.	Address the signed fetch/deadlock behavior or document the supported workaround, and consider the Fedify upgrade path noted in #1961.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	The edits stay within the KV store bug fix and corresponding tests, with no unrelated changes apparent.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title accurately summarizes the main change: missing-key handling in KnexKvStore now returns undefined instead of null.
Description check	✅ Passed	The description is clearly about the same KvStore fix and its Fedify signature-verification impact.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue-1961-review-a7c9ed

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

src/knex.kvstore.ts (1)

56-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extract new Date() to a variable for consistency.

Using a single Date instance for both the row.expires check and the conditional deletion ensures the condition uses the exact same timestamp, avoiding microsecond timing discrepancies between the evaluation of the if-statement and the query execution.

♻️ Proposed refactor

-        if (row.expires !== null && row.expires <= new Date()) {
+        const now = new Date();
+        if (row.expires !== null && row.expires <= now) {
             this.logging.debug(
                 `KnexKvStore: Deleting expired key ${key}`,
                 keyInfo,
             );
             // Only delete if still expired - a concurrent set() may have
             // refreshed the row between our read and this delete
             await this.knex(this.table)
                 .where(query)
-                .where('expires', '<=', new Date())
+                .where('expires', '<=', now)
                 .del();
             return undefined;
         }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/knex.kvstore.ts` around lines 56 - 67, In the expiration handling of the
key lookup method, extract a single current timestamp before the row.expires
check and reuse that Date value for both the if condition and the conditional
delete query. Keep the existing concurrency protection and return behavior
unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/knex.kvstore.ts`:
- Around line 56-67: In the expiration handling of the key lookup method,
extract a single current timestamp before the row.expires check and reuse that
Date value for both the if condition and the conditional delete query. Keep the
existing concurrency protection and return behavior unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: feb62b20-59d9-45c3-bcad-2279df088a9a

📥 Commits

Reviewing files that changed from the base of the PR and between 0d75f3c and a649dca.

📒 Files selected for processing (2)

• src/knex.kvstore.integration.test.ts
• src/knex.kvstore.ts