fix(export): Guard _fail() against deleted row crash (TOCTOU) - #121
fix(export): Guard _fail() against deleted row crash (TOCTOU)#121Vigneshselvaraj1811 wants to merge 2 commits into
Conversation
_reconcile_one() catches all exceptions and calls _fail() to mark the migration Failed. _fail() then calls frappe.get_doc() — the same call that may have just failed if the row was deleted. If the row was removed between the cron fetching names and processing them, the second get_doc raises DoesNotExistError unhandled, aborting the entire reconcile_migrations loop and stalling every other in-flight migration. Fix: catch frappe.DoesNotExistError around the frappe.get_doc() call instead of a separate db.exists() check — atomic, no TOCTOU window.
Same bug as BUG-1 in migration.py (fixed in 0282b10). If the export row is deleted between fetching the list and processing it, frappe.get_doc() raises DoesNotExistError unhandled, aborting the entire reconcile loop. Closes frappe#120
Confidence Score: 4/5The export.py fix is straightforward and correct; the migration.py change looks fine in isolation but conflicts with the PR description's claim that it was already patched. Both changes are logically sound, but the migration.py hunk appears unexpectedly given the PR description — a quick author confirmation is worthwhile before merging. atlas/atlas/migration.py — verify the _fail() guard addition is intentional and not a stale or accidental include. Reviews (1): Last reviewed commit: "fix(export): Guard _fail() against delet..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _fail(name: str, message: str) -> None: | ||
| """Mark a migration Failed, recording the phase it failed at so retry() resumes | ||
| there. Best-effort and self-committing (it runs after a rollback).""" | ||
| doc = frappe.get_doc("Virtual Machine Migration", name) | ||
| try: | ||
| doc = frappe.get_doc("Virtual Machine Migration", name) | ||
| except frappe.DoesNotExistError: | ||
| # The row was deleted between the time we fetched the list of non-terminal | ||
| # migrations and now — nothing to fail. This is not an error (the operator | ||
| # intentionally removed it), so just return. | ||
| return | ||
| doc.db_set({"status": "Failed", "error_message": message[-2000:], "error_at_status": doc.status}) | ||
| # nosemgrep: frappe-manual-commit -- persist the failure so the next tick sees it | ||
| frappe.db.commit() |
There was a problem hiding this comment.
migration.py:1209" and references commit 0282b10, but the diff adds it here for the first time. Either the description is wrong, or this change is unintentional (e.g., a rebase that brought in an uncommitted patch). Worth confirming this is deliberate and not an accidental double-application.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Same bug as BUG-1 in
migration.py(fixed in 0282b10) but inexport.py._fail()atatlas/atlas/export.py:475callsfrappe.get_doc("Virtual Machine Image Export", name)without aDoesNotExistErrorguard. If the row is deleted between the reconcile loop fetching names and processing them, the unhandled exception aborts the entirefor name in namesloop, skipping all other in-flight exports.Fix: Wrap
frappe.get_doc()intry/except frappe.DoesNotExistErrorand return early, matching the pattern already applied tomigration.py:1209.Reference: See
llm/bugs-and-flows.mdBUG-1 for the original bug and its fix.