fix(janitor): restore inbound /peer/denied expiry notification

Task 9's first pass replaced inbound expiry's signed /peer/denied POST with a plain delete, citing symmetric independent expiry as the design. The spec at §4.10 explicitly said 'inbound row cleanup unchanged' — that was scope creep, not a fix. Inbound expiry now preserves the pre-branch behavior verbatim: signed POST to remote, delete only on success, retry on failure. Outbound expiry's fanout logic (the actual Task 9 scope) is unchanged.
2026-04-26 22:22:34 +02:00
parent 86ac54b913
commit 4064d822cd
2 changed files with 168 additions and 25 deletions
@@ -102,11 +102,18 @@ describe('cleanupExpiredApprovalRequests', () => {
      createdAt: past - 500,
    }).run();

+    // Outbound expiry must NOT make any network call. Spy on fetch to assert.
+    const fetchSpy = vi.spyOn(globalThis, 'fetch').mockResolvedValue(
+      new Response(null, { status: 200 }),
+    );
+
    const { cleanupExpiredApprovalRequests } = await import('./storageJanitor.js');
    const before = Date.now();
-    const cleaned = cleanupExpiredApprovalRequests();
+    const cleaned = await cleanupExpiredApprovalRequests();
    const after = Date.now();

+    expect(fetchSpy).not.toHaveBeenCalled();
+
    expect(cleaned).toBe(1);

    // Parent deleted.
@@ -142,9 +149,10 @@ describe('cleanupExpiredApprovalRequests', () => {
    expect(bobNotif!.readAt).toBeNull();
  });

-  // Janitor expiry test B: inbound row with past expiresAt → janitor deletes
-  // it without writing any notifications.
-  it('inbound expired row: deletes outright with NO notifications written', async () => {
+  // Janitor expiry test B1: inbound row with past expiresAt + successful
+  // /peer/denied POST → janitor sends signed denial, then deletes the row.
+  // No local notifications are written for inbound.
+  it('inbound expired row: sends signed /peer/denied to origin and deletes on success', async () => {
    const past = Date.now() - 1_000;
    testDb.insert(schema.peerApprovalRequests).values({
      id: 'req-in',
@@ -157,16 +165,102 @@ describe('cleanupExpiredApprovalRequests', () => {
      approvalToken: null,
    }).run();

+    const fetchSpy = vi.spyOn(globalThis, 'fetch').mockResolvedValue(
+      new Response(null, { status: 200 }),
+    );
+
    const { cleanupExpiredApprovalRequests } = await import('./storageJanitor.js');
-    const cleaned = cleanupExpiredApprovalRequests();
+    const cleaned = await cleanupExpiredApprovalRequests();

    expect(cleaned).toBe(1);

+    // Exactly one signed POST to the origin's /peer/denied endpoint.
+    expect(fetchSpy).toHaveBeenCalledTimes(1);
+    const [url, init] = fetchSpy.mock.calls[0]!;
+    expect(url).toBe('https://orbit.example/api/federation/peer/denied');
+    expect(init?.method).toBe('POST');
+
+    const headers = init?.headers as Record<string, string>;
+    expect(headers['X-Federation-Signature']).toMatch(/^sha256=[0-9a-f]+$/);
+    expect(headers['X-Federation-Origin']).toBe('https://local.example');
+    expect(headers['X-Federation-Timestamp']).toMatch(/^\d+$/);
+    expect(headers['X-Federation-Nonce']).toBeDefined();
+    expect(headers['Content-Type']).toBe('application/json');
+
+    const body = JSON.parse(init?.body as string);
+    expect(body.origin).toBe('https://local.example');
+    expect(body.reason).toBe('expired');
+    expect(typeof body.message).toBe('string');
+    expect(body.message.length).toBeGreaterThan(0);
+
    // Parent deleted.
    expect(testDb.select().from(schema.peerApprovalRequests)
      .where(eq(schema.peerApprovalRequests.id, 'req-in')).get()).toBeUndefined();

-    // No notifications written for inbound.
+    // No local notifications written for inbound.
+    expect(testDb.select().from(schema.peerApprovalNotifications).all()).toHaveLength(0);
+  });
+
+  // Janitor expiry test B2: inbound row with past expiresAt + failed POST
+  // (network error or non-2xx) → janitor keeps the row for retry on the
+  // next cycle. No local notifications written.
+  it('inbound expired row: keeps row for retry when /peer/denied POST fails', async () => {
+    const past = Date.now() - 1_000;
+    testDb.insert(schema.peerApprovalRequests).values({
+      id: 'req-in-fail',
+      origin: 'https://orbit.example',
+      direction: 'inbound',
+      instanceName: 'Orbit',
+      hmacSecret: 'shared-secret',
+      requestedAt: past - 1000,
+      expiresAt: past,
+      approvalToken: null,
+    }).run();
+
+    // Simulate a 503 (non-ok response) — the row must remain.
+    const fetchSpy = vi.spyOn(globalThis, 'fetch').mockResolvedValue(
+      new Response(null, { status: 503 }),
+    );
+
+    const { cleanupExpiredApprovalRequests } = await import('./storageJanitor.js');
+    const cleaned = await cleanupExpiredApprovalRequests();
+
+    expect(cleaned).toBe(0);
+    expect(fetchSpy).toHaveBeenCalledTimes(1);
+
+    // Row still present — eligible for retry next cycle.
+    expect(testDb.select().from(schema.peerApprovalRequests)
+      .where(eq(schema.peerApprovalRequests.id, 'req-in-fail')).get()).toBeDefined();
+
+    // No local notifications written.
+    expect(testDb.select().from(schema.peerApprovalNotifications).all()).toHaveLength(0);
+  });
+
+  // Network-error variant of B2: thrown error (e.g. AbortError, DNS failure)
+  // is caught and treated as not-sent. Row must remain for retry.
+  it('inbound expired row: keeps row when fetch throws a network error', async () => {
+    const past = Date.now() - 1_000;
+    testDb.insert(schema.peerApprovalRequests).values({
+      id: 'req-in-throw',
+      origin: 'https://orbit.example',
+      direction: 'inbound',
+      instanceName: 'Orbit',
+      hmacSecret: 'shared-secret',
+      requestedAt: past - 1000,
+      expiresAt: past,
+      approvalToken: null,
+    }).run();
+
+    const fetchSpy = vi.spyOn(globalThis, 'fetch').mockRejectedValue(new Error('ECONNREFUSED'));
+
+    const { cleanupExpiredApprovalRequests } = await import('./storageJanitor.js');
+    const cleaned = await cleanupExpiredApprovalRequests();
+
+    expect(cleaned).toBe(0);
+    expect(fetchSpy).toHaveBeenCalledTimes(1);
+
+    expect(testDb.select().from(schema.peerApprovalRequests)
+      .where(eq(schema.peerApprovalRequests.id, 'req-in-throw')).get()).toBeDefined();
    expect(testDb.select().from(schema.peerApprovalNotifications).all()).toHaveLength(0);
  });

@@ -193,7 +287,7 @@ describe('cleanupExpiredApprovalRequests', () => {
    }).run();

    const { cleanupExpiredApprovalRequests } = await import('./storageJanitor.js');
-    const cleaned = cleanupExpiredApprovalRequests();
+    const cleaned = await cleanupExpiredApprovalRequests();

    expect(cleaned).toBe(0);

@@ -438,17 +438,18 @@ export function cleanupFederationFileQueue(): number {
 * Outbound rows: before deletion, fan out kind='expired' notifications to
 * each subscriber so the original requester(s) can see the terminal state on
 * next page load. Subscriber rows cascade-delete via FK when the parent row
- * is removed.
+ * is removed. No network call — the remote was never told about our queued
+ * outbound request to begin with.
 *
- * Inbound rows: deleted outright. Both sides expire independently (their
- * own janitor will fan out kind='expired' notifications to their subscribers
- * on their schedule); no cross-instance network notification is required.
+ * Inbound rows: send a signed POST /peer/denied with reason='expired' to
+ * the requesting origin so it can update its UI. Only delete the row if the
+ * POST succeeded; otherwise keep it and retry on the next janitor cycle.
 *
- * No WebSocket broadcast is emitted on expiry — offline users see the
+ * No WebSocket broadcast is emitted on local expiry — offline users see the
 * notification on next GET /api/federation/peering-notifications, matching
 * the persistence guarantee of the notifications table.
 */
-export function cleanupExpiredApprovalRequests(): number {
+export async function cleanupExpiredApprovalRequests(): Promise<number> {
  const db = getDb();
  const expiredRequests = db
    .select()
@@ -458,11 +459,15 @@ export function cleanupExpiredApprovalRequests(): number {

  if (expiredRequests.length === 0) return 0;

+  const { getOurOrigin, buildFederationHeaders } = await import('./federationAuth.js');
+  const ourOrigin = getOurOrigin();
  const now = Date.now();
  let deletedCount = 0;
+
  for (const req of expiredRequests) {
    if (req.direction === 'outbound') {
-      // Fan out kind='expired' notifications to subscribers before delete.
+      // Outbound expiry: fan out kind='expired' notifications to subscribers,
+      // then cascade-delete. No network call.
      const subs = db
        .select()
        .from(schema.peerApprovalSubscribers)
@@ -482,13 +487,54 @@ export function cleanupExpiredApprovalRequests(): number {
          })
          .run();
      }
+      db.delete(schema.peerApprovalRequests)
+        .where(eq(schema.peerApprovalRequests.id, req.id))
+        .run();
+      deletedCount++;
+      continue;
+    }
+
+    // Inbound expiry: signed /peer/denied POST to origin; only delete on
+    // success. Preserved verbatim from pre-branch behavior.
+    if (!req.hmacSecret) {
+      // CHECK constraint guarantees inbound rows have hmac_secret; defensive guard.
+      console.warn(
+        `[storage-janitor] Inbound approval-request ${req.id} missing hmac_secret — skipping expiry notification`,
+      );
+      continue;
+    }
+
+    const denialBody = JSON.stringify({
+      origin: ourOrigin,
+      reason: 'expired' as const,
+      message: 'Request expired — no response from admin within 30 days',
+    });
+    const headers = buildFederationHeaders(denialBody, req.hmacSecret, ourOrigin);
+
+    let sent = false;
+    try {
+      const response = await fetch(`${req.origin}/api/federation/peer/denied`, {
+        method: 'POST',
+        headers,
+        body: denialBody,
+        signal: AbortSignal.timeout(10_000),
+      });
+      sent = response.ok;
+    } catch {
+      // Network error — will retry next cycle
+    }
+
+    if (sent) {
+      // No rejected peer record for expiry — origin can re-request
+      db.delete(schema.peerApprovalRequests)
+        .where(eq(schema.peerApprovalRequests.id, req.id))
+        .run();
+      deletedCount++;
+    } else {
+      console.warn(
+        `[storage-janitor] Failed to send expiry denial to ${req.origin} — will retry next cycle`,
+      );
    }
-    // Cascade-delete the parent (subscribers go via FK cascade for outbound;
-    // inbound has no subscribers to clear).
-    db.delete(schema.peerApprovalRequests)
-      .where(eq(schema.peerApprovalRequests.id, req.id))
-      .run();
-    deletedCount++;
  }

  if (deletedCount > 0) {
@@ -643,7 +689,7 @@ export function cleanupSoftDeletedDmChannels(): number {
 *  - Stale file queue entries
 *  - Soft-deleted DM channels past grace period
 */
-export function runFederationJanitor(): void {
+export async function runFederationJanitor(): Promise<void> {
  try {
    const outbox = cleanupFederationOutbox();
    const mutLog = cleanupFederationMutationLog();
@@ -657,11 +703,14 @@ export function runFederationJanitor(): void {
      );
    }

-    // Expire approval requests (outbound rows fan out kind='expired'
-    // notifications to subscribers before delete; inbound rows are deleted
-    // outright). Then sweep read peering notifications older than 30 days.
+    // Expire approval requests:
+    //   - Outbound rows fan out kind='expired' notifications to subscribers
+    //     before cascade-delete (local DB only).
+    //   - Inbound rows send a signed /peer/denied POST to the requesting
+    //     origin; only delete on success, otherwise retry next cycle.
+    // Then sweep read peering notifications older than 30 days.
    try {
-      const approvalExpired = cleanupExpiredApprovalRequests();
+      const approvalExpired = await cleanupExpiredApprovalRequests();
      if (approvalExpired > 0) {
        console.log(`[storage-janitor] Expired ${approvalExpired} peer approval request(s)`);
      }