fix(guardian): order the post-recovery DB writes by blast radius (#446) - #449
Conversation
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
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
clestons
left a comment
clestons
left a comment
There was a problem hiding this comment.
Review — #449 (fix(guardian): order the post-recovery DB writes by blast radius, part of #446)
结论：APPROVE
跟进了 Codex PK 在 #441 review 里独立挖出的"两次 DB 写非原子"发现。这个 PR 的价值判断很诚实——它没有假装"加事务"能解决问题(PersistenceAdapter 压根没有事务概念，postgres 那边是 7 个独立 repository、json 那边是两个独立文件的 read-modify-write)，而是承认这本质是链+DB 双写问题，链是权威记录且没法参与数据库事务，事务顶多把"两次写之间"的窗口缩小到"链确认到第一次写之间"，缩不到 0。所以它把 #446 从"加事务"改写成"要做对账"，没有留一个误导后人的 issue。
具体改法：signerAddress(安全相关——写错了 DB 会记着一个已经不控制账户的 owner，而且原来的顺序会让这个错误永久不可恢复，因为重试逻辑靠 findPendingRecovery 找活着的请求,状态一旦被标成 executed 就再也摸不到)改成先写；request 状态(账务，记录用)后写，且包在 try/catch 里失败了只记日志不重新抛——理由也站得住：链上已经真的执行了、account 行也已经反映了，这时候抛错等于告诉调用方一个假消息。最坏情况从"永久记错 owner"降级成"request 状态卡在 pending，不整洁但不危险"。
验证
- 本地实跑：
npx jest74/74 全绿(新增 4 条：显式断言写入顺序["account","request"]；账务写失败仍返回成功且日志带 tx hash/account/new owner 够人工对账；account 写失败会抛错且 request 绝不会被标 executed；executedAt落库值和返回值是同一个而不是两次读钟)；npx tsc --noEmit干净。 - 我自己有个没能在这个仓库里完全确认的疑问：如果 account 写成功但 request 状态卡成永久 pending，之后如果有人(或某个重试逻辑)对同一账户再调一次
executeRecovery，会不会真的安全 no-op？——这取决于合约在执行成功后会不会清掉activeRecovery这个槽位(这部分在合约仓库,不在这次 diff 里)。Codex PK 帮忧看了 #441 已有的readActiveRecovery检查(:467-471)：如果合约清空了 active proposal,重试会在这道检查上被正确拦下,不会重复广播；如果没清空,#441 的 owner 一致性检查也还在。这条链路没有被这个 PR 破坏。
[Note] Low — 方法级注释有点跟不上代码了(Codex PK 发现)
executeRecovery 顶部的 docstring 还写着"Any failure causes an exception"(任何失败都会抛异常,request 停在 pending 可重试)——但现在账务写失败是故意吞掉不抛的，这句注释不完全准了。不影响功能，下次改这个方法时顺手更新一下。
PK Summary | Verification
- R1 DeepSeek(flash)：R1a+R1b 都跑了，这轮校准得不错——两边都正确识别出这是刻意的设计权衡而不是 bug，没有制造假警报。R1b 提了一条"日志里加上 chainId,免得多链场景对账混淆"，是个合理的低价值运维建议。
- Codex PK：独立 worktree。确认了写入顺序的取舍是对的(还专门去读了
postgres.adapter.ts/json.adapter.ts确认这个持久层真的没有事务 API,不是我们凭空断言)；回答了我的重试安全性疑问；挖出上面那条文档过期的小问题。
自评 — #449
- 轮数：R1 DeepSeek(R1a+R1b 真跑)+ 我独立分析(含提出重试安全性的开放问题)+ 本地实跑 74/74 测试+tsc 干净 + Codex PK(1 轮，专门去读了持久层适配器代码核实"真没事务"这个前提)。
- 机械证据：
npx jest74/74、npx tsc --noEmit干净。 - DeepSeek flash 评级：4/5 —— 这轮质量好，两边都没有为了显得有用而制造假发现,准确识别出这是刻意权衡。样本 14/20，跟 #441 一样，这类"nuanced tradeoff 判断"的场景 flash 表现明显比"读代码找 bug"的场景稳。
- 与 skill 设计一致，R1a+R1b 双通道 + Codex PK 都跑了。
R1 DeepSeek(R1a+R1b)真跑 + 独立分析(含提出并验证重试安全性问题)+ 本地实跑 74/74 测试+tsc 干净 + Codex PK 1 轮(核实持久层无事务的前提 + 挖出文档过期问题)。
…view #449) 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
|
APPROVE 之后追加了一个 commit（ 1. [Low] docstring 过期（Codex PK 发现） —— 修了，没留到下次。 这条注释的失真是这个 PR 自己造成的：
2. [R1b 建议] 日志加 chainId —— 采纳了。这条日志的存在意义就是给人工对账用，多链场景下没有 chainId 就是条有歧义的记录。 实现上有个细节： 测试补了 关于 review 里那个开放问题（account 写成功但 request 卡 pending 后，重复调 Gates：type-check + 74 tests + lint + format:check 全绿。 |
clestons
left a comment
clestons
left a comment
There was a problem hiding this comment.
Re-review — #449 (增量 diff: f85f0c90..f25d1de)
结论：APPROVE
直接跟进了我上一轮review留的两条 Low note：docstring 更新为准确描述新行为(区分"链上确认前失败"vs"确认后两次写按 blast radius 排序,第二次吞掉")；reconciliation 日志里加了 chainId,并且新测试断言了这一点。getChainId() 特意放在 catch 块外面,注释解释了原因("catch 块存在的意义就是绝不抛错,不该塞任何可能失败的东西进去")——这个位置选择是对的。
验证
- 本地实跑：
npx jest74/74 全绿、npx tsc --noEmit干净。
PK Summary
- R1 DeepSeek(flash)：跑了，1 条 finding("
getChainId()在 try/catch 外面，如果它抛错会打断流程")——我顺着调用链核实过：assertRpcChain()(内部第一步就调this.getChainId())在同一个函数里第 464 行已经先跑过一次并且成功了(不然函数根本到不了第 544 行)，而getChainId()是纯同步读 config，没有 I/O，同一次函数调用里第二次读一定拿到同样的结果——这条 finding 单独看有道理，但没考虑到函数前面已经验证过的执行路径，实际上不会触发。 - Codex PK：跳过。这是对上一轮已经深度审过的同一个方法的极小增量(21 行)，逻辑闭环我已经用调用链自己验证过，不需要再开一轮。
自评 — #449(二次)
- 轮数：R1 DeepSeek(真跑)+ 我独立分析(顺调用链验证 getChainId 的假阳性)+ 本地实跑 74/74 测试+tsc 干净。小增量 follow-up，未跑 Codex PK。
- 机械证据：
grep -n getChainId拉出全部调用点，确认assertRpcChain在新调用点之前已经用同一个函数验证过配置有效。 - DeepSeek flash 评级：2/5 —— 唯一一条 finding 技术上不算错,但没结合前文已经验证过的执行路径,属于"看起来合理但一追溯调用链就站不住"的类型。样本 18/20。
- 与 skill 设计一致，R1 没跳过。
R1 DeepSeek 真跑 + 独立分析(顺调用链验证假阳性)+ 本地实跑 74/74 测试+tsc 干净，小增量未跑 Codex PK。
2 participants
Part of #446（不 close —— 见下方「这个 PR 不解决什么」）
情况
executeRecovery链上确认之后跟着两次 DB 写，中间没有事务：PersistenceAdapter（47 行、约 30 个方法的扁平 CRUD 接口）没有任何事务概念PostgresAdapter注入 7 个独立Repository<T>，每次写各自一个隐式事务JsonAdapter是readFile→ 改 →writeFile(整个文件)，而且这两个操作分别写recovery-requests.json和accounts.json两个文件原顺序是 request 状态先写。第二次写失败 → account 行记着一个已经不控制该账户的 owner，而且永久如此：重试会在
findPendingRecovery直接返回 null（已非 pending），根本走不到修复逻辑。改法：按爆炸半径排序，而不是包事务
链已经动了、回滚不了，也没有事务可用。所以这两次写按"哪个被落下代价更大"排序：
signerAddress（安全相关）先写try/catch，记录 tx hash / account / new owner 而不重新抛出不抛出是有意的：恢复确实发生了 —— 链上完成，account 行也已反映。这里抛错等于告诉调用方相反的事实。最坏情况现在退化成「request 状态陈旧」：不整洁，但不危险。
顺带把两次独立的
new Date().toISOString()合并成一个executedAt，避免落库值与返回值差几毫秒。这个 PR 不解决什么（所以不 close #446）
它刻意只收窄窗口，不闭合，代码注释里也这么写了。链确认之后、第一次写之前崩溃，同样的分歧照样出现。
加事务也修不好 —— 这不是数据库原子性问题，是 chain + DB 的双写问题：链是权威记录，而链不可能参与数据库事务。事务只是把窗口从「两次写之间」缩到「确认与提交之间」。
真正的解法是对账：让 DB 从链派生（读路径以链上 owner 为准，或跑周期性 reconciler 扫 pending recovery 与链上实际状态）。#446 已改写成跟踪这件事，不再是「加事务」——免得留一个误导后来人的 issue。
测试（新增 4 个，后端共 74）
["account", "request"]executedAt变异验证非空测：顺序倒回去挂 3 个；去掉 catch 挂 1 个。
Gates
backend type-check + build + 74 tests + lint +
format:check全绿。（
aastar-frontend的format:check只报 gitignore 掉的test-results/产物 —— 已知的本地误报，CI 干净 checkout 看不到。）Claude-Session: https://claude.ai/code/session_01BxmyQj2A82DfFXu97kKACk