fix: Keep the ABI reload cursor consistent when a reload fails - #350
Conversation
`load_new_abis` read the latest ABI id into `last_abi_id` before streaming and applying the rows. Any failure in between (connection drop, bad ABI json) left the cursor past rows that were never applied, so those ABIs were skipped for the whole life of the process. Only a restart recovered them. The cursor is now written after every fetched ABI was applied, so a reload that dies halfway is retried from the same id. A single ABI that cannot be turned into selectors is dropped and logged instead of aborting the reload. If it aborted, that row would stop the cursor on every later reload and no new ABI would ever load again. The catch is limited to the errors a malformed `abi_json` produces, so a real failure still aborts and keeps the cursor back. The cursor is no longer written on the "no new ABIs" path. There it could only stay the same or move backwards when rows were deleted, which contradicts the monotonic cursor the reload relies on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Felipe Alvarado (falvaradorodriguez)
left a comment
Felipe Alvarado (falvaradorodriguez)
left a comment
There was a problem hiding this comment.
Not from this PR, but worth noting here. The reload skips a bad ABI, init() can't: selectors_from_abis reads fn_abi["type"] directly. So the skipped row stays in the table and breaks a whole batch of 500 on the next restart, leaving /health/ready at 503. fn_abi.get("type") == "function" would fix both paths. Can go in another ticket.
Agree! #351 |
2 participants
Make sure these boxes are checked! 📦✅
./run_tests.sh(94 passed; ranalembic upgrade head+pytestagainst a local postgres because the compose ports were taken)pre-commit run -a(clean on the changed files;insert-licensealso wants to add SPDX headers to ~48 unrelated files that are already missing them onmain, left alone)What was wrong? 👾
Closes https://linear.app/safe-global/issue/PLA-1954
DataDecoderService.load_new_abisread the latest ABI id intolast_abi_idfirst, and streamed and applied the rows afterwards. Any failure in between (connection drop, bad ABI json) left the cursor past rows that were never applied, and the next reload started from the advanced cursor. Those ABIs were skipped for the whole life of the process.The process recovers on restart, which is why this is easy to miss: the loss is confined to one process lifetime.
Pre-existing behaviour, found while reviewing #342.
How was it fixed? 🎯
The latest id goes to a local, and
last_abi_idis written only after every fetched ABI was applied. A reload that dies halfway is retried from the same id. Re-applying an ABI is cheap:add_abionly writes selectors that are not already in the map.A single ABI that cannot be turned into selectors is dropped and logged instead of aborting the reload. If it aborted, that row would stop the cursor on every later reload and no new ABI would ever load again. The catch is limited to the errors a malformed
abi_jsonproduces (KeyError,TypeError,ValueError), so a DB or stream failure still aborts the reload and keeps the cursor back.The cursor is no longer written on the "no new ABIs" path. There it could only stay the same, or move backwards when rows were deleted, which contradicts the monotonic cursor the reload relies on.
Two tests, both verified to fail against the old code:
test_load_new_abis_keeps_cursor_when_reload_fails: the stream drops after the first ABI was applied. The cursor stays back and the next reload loads the ABI that was missed.test_load_new_abis_skips_a_malformed_abi: a row without thetypekey is dropped, the rest of the reload goes through, and it is not retried.Out of scope
init()builds the selector map through_generate_selectors_with_abis_from_abis, which batches up to 500 ABIs into oneselectors_from_abiscall with no per-ABI guard. The same malformed row that the reload now survives still aborts startup, so the protection does not survive a restart. Moving the guard down intoselectors_from_abiswould coverinit()andget_contract_abi_selectors_with_functionstoo. That is startup resilience, which belongs to PLA-1953, so I left it out. Happy to open a follow-up.