Skip to content

test: SharedState collective save-failure rollback (PRPUNDIT-8, PRPUNDIT-9) - #1324

Merged
chaojhou merged 6 commits into
AMD-AGI:mainfrom
jiagaoxiang:testgap/PRPUNDIT-9
Sep 1, 2026
Merged

test: SharedState collective save-failure rollback (PRPUNDIT-8, PRPUNDIT-9)#1324
chaojhou merged 6 commits into
AMD-AGI:mainfrom
jiagaoxiang:testgap/PRPUNDIT-9

Conversation

@jiagaoxiang

@jiagaoxiang jiagaoxiang commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Forces SharedState.save to fail during record_collective and record_collective_integration.
  • Asserts in-memory last_collective / collective_attempts restore and on-disk state.json is unchanged.

Covers test gaps PRPUNDIT-8 and PRPUNDIT-9 (merged from #1323 into this PR).

Test plan

  • PYTHONPATH=src pytest src/hyperloom/inference_optimizer/tests/test_collective_lane.py::test_record_collective_rolls_back_when_save_fails src/hyperloom/inference_optimizer/tests/test_collective_lane.py::test_record_collective_integration_rolls_back_when_save_fails

…IT-8)

Force SharedState.save to fail and assert in-memory last_collective/collective_attempts and state.json are unchanged.
Force SharedState.save to fail and assert in-memory last_collective/collective_attempts and state.json are unchanged.
@jiagaoxiang
jiagaoxiang requested a review from a team as a code owner August 28, 2026 07:13
Keep both SharedState save-failure rollback tests in one file so the pair can land without a conflict.
@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

Merged #1323 into this branch and resolved the test_collective_lane.py conflict. This PR now has both record_collective and record_collective_integration save-failure tests. Safe to merge after or instead of #1323.

@jiagaoxiang jiagaoxiang changed the title test: roll back record_collective when save fails (PRPUNDIT-9) test: SharedState collective save-failure rollback (PRPUNDIT-8, PRPUNDIT-9) Aug 28, 2026

@zoroyihan7 zoroyihan7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with an independent verification pass: ran the touched test file on this branch and mutation-tested the production code to confirm the new tests actually catch regressions (not just pass).

Mutations tried and caught: deleting the restore lines in record_collective and in record_collective_integration each turns the corresponding new test red, so both producers are pinned separately.

No regressions in the surrounding suite (pre-existing failures verified identical on origin/main). LGTM.

@zoroyihan7
zoroyihan7 enabled auto-merge (squash) August 31, 2026 07:41
@zoroyihan7

Copy link
Copy Markdown
Contributor

/retest

(e2e failed on the Dispatch + monitor single-GPU smoke run step, which is failing on unrelated PRs too — retriggering. Auto-merge is armed, so this will land on its own once e2e is green.)

@zoroyihan7 zoroyihan7 added the skip-e2e-test It's a PR that doesn't need to be e2e tested label Aug 31, 2026
@zoroyihan7 zoroyihan7 added the retest Re-run E2E smoke without a new commit (runs once) label Sep 1, 2026
@chaojhou
chaojhou disabled auto-merge September 1, 2026 09:41
@chaojhou
chaojhou merged commit 724ecc6 into AMD-AGI:main Sep 1, 2026
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

retest Re-run E2E smoke without a new commit (runs once) skip-e2e-test It's a PR that doesn't need to be e2e tested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants