fix(guardian): order the post-recovery DB writes by blast radius (#446)

aastar/src/guardian/guardian.service.spec.ts

@@ -268,6 +268,65 @@ describe("GuardianService — recovery chain consistency (PR #434)", () => {
     });
   });
 
+  // The chain has already moved and there is no transaction across the two DB writes,
+  // so they are ordered by blast radius: a stale signerAddress means the DB names an
+  // owner who no longer controls the account AND never self-heals (a retry stops at
+  // findPendingRecovery). A stale request status is only untidy. See issue #446.
+  describe("post-confirmation DB writes (issue #446)", () => {
+    beforeEach(async () => {
+      mockGetChainId.mockResolvedValue(11155111);
+      service = await buildService(11155111);
+    });
+
+    it("writes the account's signerAddress before the request status", async () => {
+      const order: string[] = [];
+      mockUpdateAccountByAddress.mockImplementation(async () => void order.push("account"));
+      mockUpdateRecoveryRequest.mockImplementation(async () => void order.push("request"));
+
+      await service.executeRecovery(ACCOUNT);
+
+      expect(order).toEqual(["account", "request"]);
+    });
+
+    it("still reports success when only the bookkeeping write fails", async () => {
+      mockUpdateRecoveryRequest.mockRejectedValue(new Error("db down"));
+      const logged = jest.spyOn((service as any).logger, "error").mockImplementation(() => {});
+
+      // The recovery is on-chain and the account row already reflects it — telling the
+      // caller it failed would be a lie.
+      const result = await service.executeRecovery(ACCOUNT);
+
+      expect(result.txHash).toBe("0xtxhash");
+      expect(mockUpdateAccountByAddress).toHaveBeenCalledWith(ACCOUNT, {
+        signerAddress: NEW_OWNER,
+      });
+      // An operator has to reconcile by hand, so the log must carry enough to do it.
+      const msg = logged.mock.calls[0][0] as string;
+      expect(msg).toContain("0xtxhash");
+      expect(msg).toContain(ACCOUNT);
+      expect(msg).toContain(NEW_OWNER);
+      // chainId too — without it the log is ambiguous once more than one chain is in play.
+      expect(msg).toContain("chainId=11155111");
+    });
+
+    it("fails loudly, and leaves the request pending, when the account write fails", async () => {
+      mockUpdateAccountByAddress.mockRejectedValue(new Error("db down"));
+
+      await expect(service.executeRecovery(ACCOUNT)).rejects.toThrow(/db down/);
+      // Must NOT be marked executed: that is what makes the inconsistency unrecoverable.
+      expect(mockUpdateRecoveryRequest).not.toHaveBeenCalled();
+    });
+
+    it("reports one executedAt, not two clock reads that can disagree", async () => {
+      const result = await service.executeRecovery(ACCOUNT);
+
+      expect(mockUpdateRecoveryRequest).toHaveBeenCalledWith("req-1", {
+        status: "executed",
+        executedAt: result.executedAt,
+      });
+    });
+  });
+
   it("refuses to build a relay wallet when chainId is unusable", async () => {
     for (const bad of [undefined, 0, -1]) {
       service = await buildService(bad);

aastar/src/guardian/guardian.service.ts

@@ -418,8 +418,14 @@ export class GuardianService {
    *  4. Wait for the transaction to be mined and confirm success.
    *  5. Update the database only after on-chain success.
    *
-   * Any failure causes an exception; the database is NOT updated, so
-   * the recovery request stays in "pending" status and can be retried.
+   * Failures BEFORE the chain moves (1-4) throw, and the database is not touched, so
+   * the request stays "pending" and can be retried.
+   *
+   * Once the chain has moved, step 5's two writes are ordered by blast radius rather
+   * than wrapped in a transaction (there isn't one). A failed `signerAddress` write
+   * throws and leaves the request pending. A failed request-status write is
+   * deliberately swallowed and logged: the recovery really did happen and the account
+   * row already says so, so raising here would report the opposite. See issue #446.
    */
   async executeRecovery(accountAddress: string) {
     // ── 1. Off-chain checks ───────────────────────────────────────────────
@@ -519,10 +525,23 @@ export class GuardianService {
     }
 
     // ── 5. Update database only after on-chain success ─────────────────
-    await this.databaseService.updateRecoveryRequest(request.id, {
-      status: "executed",
-      executedAt: new Date().toISOString(),
-    });
+    // The chain has already moved and cannot be rolled back, and PersistenceAdapter
+    // has no transaction across its two writes (neither the json nor the postgres
+    // adapter offers one). So these two are ORDERED BY BLAST RADIUS rather than
+    // wrapped: whichever one we do second is the one that can be left behind.
+    //
+    // `signerAddress` is the security-relevant record — a stale one has the database
+    // naming an owner who no longer controls the account, and it self-heals never,
+    // because a retry stops at findPendingRecovery (the request is no longer pending).
+    // The request's `status` is bookkeeping: stale is untidy, not dangerous.
+    //
+    // This narrows the window; it does not close it. Nothing can, while the
+    // authoritative record is a chain that cannot join a database transaction —
+    // the real answer is reconciling the DB against on-chain state. See issue #446.
+    const executedAt = new Date().toISOString();
+    // Resolved out here, not inside the catch below: that block exists precisely to
+    // never throw, so nothing fallible belongs in it.
+    const chainId = this.getChainId();
 
     // Record what the chain actually did, not what the request asked for. The two
     // are equal by the check above; using the on-chain value keeps its checksummed
@@ -531,11 +550,28 @@ export class GuardianService {
       signerAddress: onChain.newOwner,
     });
 
+    try {
+      await this.databaseService.updateRecoveryRequest(request.id, {
+        status: "executed",
+        executedAt,
+      });
+    } catch (err) {
+      // Deliberately not rethrown. The recovery DID happen: it is on-chain and the
+      // account row already reflects it. Failing the call here would tell the caller
+      // the opposite. Log everything an operator needs to reconcile by hand instead.
+      this.logger.error(
+        `Recovery ${request.id} executed on-chain (chainId=${chainId}, tx=${txHash}, ` +
+          `account=${accountAddress} -> ${onChain.newOwner}) but marking the request ` +
+          `"executed" failed: ${(err as Error).message}. The account row is correct; only ` +
+          `the request status is stale — set it to "executed" manually.`
+      );
+    }
+
     return {
       message: "Account recovery executed successfully (on-chain + database updated)",
       accountAddress,
       newSignerAddress: onChain.newOwner,
-      executedAt: new Date().toISOString(),
+      executedAt,
       txHash,
     };
   }