A federated recipient is "unknown" only when absent, not when the lookup fails

The deposit route told a peer which recipients it did not have, and the peer stops retrying those envelopes. The account lookup ran under try?, so any storage failure (a locked SQLite file, a dropped connection) came back as "unknown" and the peer gave up on a message this relay would have taken a moment later. Spotted while fixing the lock race in the previous commits, which is exactly the passing failure it turned into a lost message. The lookup is findLocalUser now, nil for no such account and a throw for anything else. A throw rolls the batch back and the route answers 500, so the peer tries again. Placement moved into Routes.placeDeposits so it can be tested against a database directly; the signed end-to-end deposit in FederationPeerTests still covers the route around it. Three tests: present is accepted, absent is unknown (the control), and a lookup against a dropped users table throws rather than answering. Falsified by putting try? back: the third fails, the other two pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-09-26 14:44:54 -04:00
parent 651eaccca4
commit 71f7a5d4e9
2 changed files with 137 additions and 37 deletions
@@ -7,12 +7,19 @@ extension Routes {

    /// Resolve an fnid to a local account, or 404.
    private func localUser(forFNID fnid: String, on db: any Database) async throws -> User {
-        guard let user = try await User.query(on: db).filter(\.$fnid == fnid).first() else {
+        guard let user = try await Self.findLocalUser(forFNID: fnid, on: db) else {
            throw WireError.notFound("no such identity here")
        }
        return user
    }

+    /// The local account for an fnid, or nil when there is none. A storage
+    /// failure throws: "no such account" and "could not look" are different
+    /// answers, and a peer is told only the first.
+    static func findLocalUser(forFNID fnid: String, on db: any Database) async throws -> User? {
+        try await User.query(on: db).filter(\.$fnid == fnid).first()
+    }
+
    private func verifyPeer(_ request: Request, body: Data, path: String, now: Int) async throws
        -> Federation.Peer
    {
@@ -58,42 +65,8 @@ extension Routes {
            throw WireError.rateLimited("peer \(peer.audience) is over its hourly budget here")
        }

-        let (accepted, unknown) = try await request.db.retryingTransaction {
-            db -> (Int, [String]) in
-            var accepted = 0
-            var unknown: [String] = []
-            for e in input.envelopes {
-                guard let user = try? await self.localUser(forFNID: e.to_fnid, on: db),
-                    let userID = user.id
-                else {
-                    // Named, not silently dropped: a peer holding an envelope
-                    // it cannot place should stop retrying it, and it can only
-                    // know that if we say so.
-                    unknown.append(e.to_fnid)
-                    continue
-                }
-                // The device must belong to that account.
-                guard let device = try await Device.find(e.to_device, on: db),
-                    device.userID == userID
-                else {
-                    unknown.append(e.to_fnid)
-                    continue
-                }
-
-                let row = Envelope()
-                row.toUser = userID
-                row.toDevice = e.to_device
-                // Sealed by construction across a federation boundary: we were
-                // not told who sent it and must not invent an attribution.
-                row.fromUser = 0
-                row.fromDevice = ""
-                row.payload = e.payload
-                row.envelopeID = e.envelope_id
-                row.delivered = false
-                try await row.create(on: db)
-                accepted += 1
-            }
-            return (accepted, unknown)
+        let (accepted, unknown) = try await request.db.retryingTransaction { db in
+            try await Self.placeDeposits(input.envelopes, on: db)
        }

        return try .json(
@@ -101,6 +74,55 @@ extension Routes {
                ok: true, accepted: accepted, unknown: unknown))
    }

+    /// Put a peer's envelopes in the local mailboxes they name, and say which
+    /// recipients this relay does not have.
+    ///
+    /// "Unknown" is an answer the peer acts on: it stops retrying that
+    /// envelope. So it is only ever said about a recipient that is really not
+    /// here. This used to look the account up with `try?`, which turned a
+    /// storage failure (a locked SQLite file, a dropped connection) into
+    /// "unknown", and the peer then gave up on a message this relay would have
+    /// taken a moment later. A failure throws now, the whole batch rolls back,
+    /// and the peer gets a 500 and tries again.
+    static func placeDeposits(
+        _ envelopes: [FederationDepositRequest.Item], on db: any Database
+    ) async throws -> (accepted: Int, unknown: [String]) {
+        var accepted = 0
+        var unknown: [String] = []
+        for e in envelopes {
+            guard let user = try await findLocalUser(forFNID: e.to_fnid, on: db),
+                let userID = user.id
+            else {
+                // Named, not silently dropped: a peer holding an envelope
+                // it cannot place should stop retrying it, and it can only
+                // know that if we say so.
+                unknown.append(e.to_fnid)
+                continue
+            }
+            // The device must belong to that account.
+            guard let device = try await Device.find(e.to_device, on: db),
+                device.userID == userID
+            else {
+                unknown.append(e.to_fnid)
+                continue
+            }
+
+            let row = Envelope()
+            row.toUser = userID
+            row.toDevice = e.to_device
+            // Sealed by construction across a federation boundary: we were
+            // not told who sent it and must not invent an attribution.
+            row.fromUser = 0
+            row.fromDevice = ""
+            row.payload = e.payload
+            row.envelopeID = e.envelope_id
+            row.delivered = false
+            try await row.create(on: db)
+            accepted += 1
+        }
+        return (accepted, unknown)
+    }
+
    /// A batch cap, so one request cannot be an unbounded write.
    static let maxFederatedBatch = 64

@@ -0,0 +1,78 @@
+import Fluent
+import Foundation
+import SQLKit
+import Testing
+import Vapor
+
+@testable import FlamenetRelay
+
+/// A federated deposit reports a recipient "unknown" only when this relay
+/// really does not have them.
+///
+/// The peer acts on that word: it stops retrying the envelope. The lookup used
+/// to run under `try?`, so a storage failure read as "unknown" and the peer
+/// gave up on a message that would have been accepted a moment later. Found
+/// while fixing the SQLite lock race (SQLiteBusyRetry.swift), which is exactly
+/// the kind of passing failure this turned into a lost message.
+@Suite("a federated recipient is unknown only when absent")
+struct FederatedUnknownMeansAbsentTests {
+
+    private func item(_ fnid: String, device: String = "bbbb-0002") -> FederationDepositRequest.Item {
+        FederationDepositRequest.Item(
+            to_fnid: fnid, to_device: device, payload: "from-afar", envelope_id: nil)
+    }
+
+    private func addBob(_ h: RelayHarness) async throws {
+        try await h.register(h.device(user: 2, id: "bbbb-0002"), on: h.db)
+        let user = User()
+        user.id = 2
+        user.username = "bob"
+        user.email = "bob@example.com"
+        user.passwordHash = "x"
+        user.displayName = "Bob"
+        user.chosenStatus = ChosenStatus.online.rawValue
+        user.fnid = "fnid:bob"
+        try await user.create(on: h.db)
+    }
+
+    @Test("a recipient who is here is accepted")
+    func presentIsAccepted() async throws {
+        try await withRelay { h in
+            try await addBob(h)
+            let placed = try await Routes.placeDeposits([item("fnid:bob")], on: h.db)
+            #expect(placed.accepted == 1)
+            #expect(placed.unknown.isEmpty)
+        }
+    }
+
+    /// The control for the test below: absent really is "unknown", so that
+    /// test cannot pass by never saying the word.
+    @Test("a recipient who is not here is unknown")
+    func absentIsUnknown() async throws {
+        try await withRelay { h in
+            try await addBob(h)
+            let placed = try await Routes.placeDeposits([item("fnid:nobody")], on: h.db)
+            #expect(placed.accepted == 0)
+            #expect(placed.unknown == ["fnid:nobody"])
+        }
+    }
+
+    @Test("a lookup that fails throws, and is not reported as unknown")
+    func aFailedLookupIsNotUnknown() async throws {
+        try await withRelay { h in
+            try await addBob(h)
+            // The account lookup now fails for storage reasons, not absence.
+            let sql = try #require(h.db as? any SQLDatabase)
+            try await sql.raw("DROP TABLE users").run()
+
+            do {
+                let placed = try await Routes.placeDeposits([item("fnid:bob")], on: h.db)
+                Issue.record(
+                    "a storage failure came back as an answer: unknown \(placed.unknown), "
+                        + "so the peer would stop retrying a message this relay could take")
+            } catch {
+                // Thrown, so the route answers 500 and the peer retries.
+            }
+        }
+    }
+}