From f85f0c90255b100903ce25d7890485304002e870 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 2 Aug 2026 00:07:36 +0700 Subject: [PATCH 1/2] fix(guardian): order the post-recovery DB writes by blast radius (#446) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After executeRecovery confirms on-chain, two DB writes follow with no transaction between them — PersistenceAdapter has no transaction concept, the postgres adapter injects seven independent repositories, and the json adapter is readFile/writeFile over two separate files. Previously the recovery-request status was written first, so a failure of the second write left the account row naming an owner who no longer controls the account — and left it that way permanently, because a retry stops at findPendingRecovery (the request is no longer pending). Swapped: `signerAddress` (security-relevant) lands first, the request status (bookkeeping) second and inside a try/catch that logs the tx hash, account and new owner rather than rethrowing. Rethrowing would tell the caller the recovery failed when it demonstrably did not — it is on-chain and the account row already reflects it. Worst case is now a stale "pending" status: untidy, not dangerous. Also collapsed two separate `new Date().toISOString()` calls into one `executedAt`, so the value persisted and the value returned can no longer disagree. This deliberately narrows the window rather than closing it, and the comment says so. Nothing closes it while the authoritative record is a chain that cannot join a database transaction: a crash between confirmation and the first write reproduces the same divergence. Adding transactions would not fix that either — it is a chain/DB dual-write problem, and the real answer is reconciling the DB against on-chain state. #446 now tracks that, not "add transactions". Tests (4 new, 74 total): write order asserted explicitly; a failing bookkeeping write still returns success and logs enough to reconcile by hand; a failing account write throws AND leaves the request pending (never marked executed — that is what makes the inconsistency unrecoverable); executedAt is one value, not two clock reads. Verified non-vacuous by mutation: restoring the old order fails 3, removing the catch fails 1. Gates: backend type-check + build + 74 tests + lint + format:check green. (aastar-frontend format:check flags only gitignored test-results/ artifacts, a known local-only false positive; CI's clean checkout does not see them.) Claude-Session: https://claude.ai/code/session_01BxmyQj2A82DfFXu97kKACk --- aastar/src/guardian/guardian.service.spec.ts | 57 ++++++++++++++++++++ aastar/src/guardian/guardian.service.ts | 37 +++++++++++-- 2 files changed, 89 insertions(+), 5 deletions(-) diff --git a/aastar/src/guardian/guardian.service.spec.ts b/aastar/src/guardian/guardian.service.spec.ts index 7cf7aa0..e16e688 100644 --- a/aastar/src/guardian/guardian.service.spec.ts +++ b/aastar/src/guardian/guardian.service.spec.ts @@ -268,6 +268,63 @@ 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); + }); + + 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..ff881d9 100644 --- a/aastar/src/guardian/guardian.service.ts +++ b/aastar/src/guardian/guardian.service.ts @@ -519,10 +519,20 @@ 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(); // 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 +541,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 (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, }; } From f25d1ded1518ba1d500461ab9626634790e4cfa4 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 2 Aug 2026 00:22:48 +0700 Subject: [PATCH 2/2] fix(guardian): correct the executeRecovery docstring, log chainId (review #449) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both from the #449 review. The docstring still claimed "Any failure causes an exception; the database is NOT updated" — which this PR's own change made false, since a failed bookkeeping write is now deliberately swallowed. A stale comment on a social-recovery path is worse than no comment, so it is corrected rather than deferred: failures before the chain moves throw and leave the request pending; after it moves, a failed signerAddress write throws while a failed status write is swallowed and logged. The reconciliation log now carries chainId. That log exists to be acted on by hand, and without it the entry is ambiguous the moment more than one chain is in play. Resolved into a local before the try, not called inside the catch — that block exists precisely to never throw, so nothing fallible belongs in it. Test extended to assert chainId is present; verified non-vacuous by mutation (removing it from the message fails 1). Gates: type-check + 74 tests + lint + format:check green. Claude-Session: https://claude.ai/code/session_01BxmyQj2A82DfFXu97kKACk --- aastar/src/guardian/guardian.service.spec.ts | 2 ++ aastar/src/guardian/guardian.service.ts | 21 ++++++++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/aastar/src/guardian/guardian.service.spec.ts b/aastar/src/guardian/guardian.service.spec.ts index e16e688..71c538a 100644 --- a/aastar/src/guardian/guardian.service.spec.ts +++ b/aastar/src/guardian/guardian.service.spec.ts @@ -305,6 +305,8 @@ describe("GuardianService — recovery chain consistency (PR #434)", () => { 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 () => { diff --git a/aastar/src/guardian/guardian.service.ts b/aastar/src/guardian/guardian.service.ts index ff881d9..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 ─────────────────────────────────────────────── @@ -533,6 +539,9 @@ export class GuardianService { // 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 @@ -551,10 +560,10 @@ export class GuardianService { // 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 (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.` + `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.` ); }