diff --git a/aastar/src/guardian/guardian.service.spec.ts b/aastar/src/guardian/guardian.service.spec.ts index 7cf7aa0..71c538a 100644 --- a/aastar/src/guardian/guardian.service.spec.ts +++ b/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); diff --git a/aastar/src/guardian/guardian.service.ts b/aastar/src/guardian/guardian.service.ts index 1f992af..5bd7fa6 100644 --- a/aastar/src/guardian/guardian.service.ts +++ b/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, }; }