[ldelvoye]

resolves INFRENG-977

Runs the preprod size comparison task without holding a database transaction across the file downloads, the diff and the result uploads.

The task used to wrap the whole run in one transaction, open for up to its 120 s deadline, with a savepoint per write inside it. It now computes and uploads every result first and records the rows in one short transaction at the end, so a run is still all-or-nothing: a failure or a deadline leaves no rows, as before. Worth weighing in review: two runs that overlap on the same pair now both compute the diff where the second used to block on the first's transaction, and the one that loses deletes its own result file. The manual and monitor paths claim their row with a conditional update for the same reason.

[linear-code]

INFRENG-977

[ldelvoye]

bot: diagram the transaction scope before and after this change

[ldelvoye]

🌱 Before this change the whole automatic run sat inside one transaction. Now the downloads, diffs and uploads run with no transaction open, and only the row writes share one short transaction at the end.

Before, run_size_analysis_comparisons wrapped the whole comparison loop in transaction.atomic. Every comparison's get_or_create, _mark_processing and _record_success ran inside it, and _record_success included the File create and putfile. Each of those nested atomics became a savepoint, and the outer transaction stayed open for up to the task's 120 s deadline.

After, comparison_run.py splits the run into three phases:

• comparison_run.py:40-48: _prepare_size_analysis_comparison runs once per comparison in autocommit. It reads the existing row, downloads both analysis files, runs compare_size_analysis and uploads the result under a provisional name.
• comparison_run.py:55-72: one short transaction.atomic runs bulk_create(ignore_conflicts=True) and then, per row in sorted order, select_for_update, renames the file and sets SUCCESS or FAILED. It then dispatches the follow-up tasks before the commit. It opens no savepoints.
• comparison_run.py:73-75: the finally runs after the transaction and deletes any result file that no committed row references.

The manual and monitor path, run_size_analysis_comparison, never ran inside an enclosing transaction. Its PROCESSING claim is now a conditional UPDATE in autocommit (comparison_run.py:315-320), and the upload runs outside any transaction. Only the SUCCESS write keeps its own short atomic (comparison_run.py:337-340).

The head has moved to 278bf73 since the review, but that commit only adds a test. comparison_run.py is unchanged, so the line numbers above still hold.

_{This message was generated by a bot; every message tagged 🌱 is from it. Reply with bot: to ask it something.}

[jamieQ]

Changes generally make sense, though personally I find it somewhat harder to reason about now 😅. I guess that's the nature of distributed systems though... Left a number of thoughts for your consideration. Also, as a slight aside, the "success criteria" in the linked ticket doesn't seem like it totally matches the implementation (e.g. pending rows created before side effects & don't roll back completed work).

[jamieQ]

Not sure if it's too much of an edge case to be worth dealing with, but it seems like if the task raised an exception after we create the file but before it's saved in the result_files array in the caller we'd forget to clean it up. Something similar maybe exists for the run_size_analysis_comparison flow too.

[ldelvoye]

Definitely an edge case and you're right run_size_analysis_comparison has that gap. Adding a sweep job that deletes orphaned comparison files is the proper way to do it IMO. I think it's fine to ship for now, the loss is just tiny data growth for now.