Skip to content

fix: guard should_replace against losing completed_at on same-run - #81

Open
cgoea wants to merge 1 commit into
developfrom
users/cgoea/should_replace_completed_at_guard
Open

fix: guard should_replace against losing completed_at on same-run#81
cgoea wants to merge 1 commit into
developfrom
users/cgoea/should_replace_completed_at_guard

Conversation

@cgoea

@cgoea cgoea commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Motivation

is_terminal() treats a terminal status (failure/success/...) as sufficient on its own, independent of completed_at. a job's own conclusion is known the instant it finishes, before every sibling job in its cell/run has necessarily finished too. Two same-run/attempt snapshots of the same cell or leaf can therefore both be "terminal" while differing in how much they actually know.

Technical Details

  • Added the same guard to both Variant.should_replace and RunLeaf.should_replace in therock_status_document.py.
  • This only tightens the guard for the specific same-run/attempt, both-terminal-by-status case.
  • No caller changes required: both should_replace implementations are already the sole gate consulted by `upsert_leaf'.

Test Plan

  • Added test_same_attempt_terminal_with_completed_at_beats_terminal_without: an existing terminal RunLeaf with completed_at set must not be replaced by a same-run/attempt terminal update with completed_at=None.
  • Added test_same_attempt_terminal_without_completed_at_replaced_by_one_with.
  • Added test_variant_same_attempt_terminal_with_completed_at_not_downgraded: same guard, Variant.should_replace path.
  • Added test_merge_does_not_lose_completed_at_from_later_incomplete_snapshot.
  • Ran the full scripts/receive_therock/tests/ suite to confirm no existing test relied on a same-run/attempt terminal update being able to clear completed_at.

Test Result

All tests pass.

@cgoea
cgoea requested review from a team and HereThereBeDragons August 14, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant