Skip to content

fix(checkpoint): reply idle to a prepare whose checkpoint ended while it waited - #4660

Merged
ananthsub merged 1 commit into
mainfrom
ananthsub/checkpoint-prepare-stale-reply
Oct 16, 2026
Merged

ananthsub merged 1 commit into
mainfrom
ananthsub/checkpoint-prepare-stale-reply

Conversation

@ananthsub

Copy link
Copy Markdown
Contributor

What changed and why

A participant's prepare waits for the participant to become ready before it replies.
While it waits, its checkpoint can end.
For example, the controller goes quiet, the checkpoint's lease expires, and the participant reopens admission.
The controller can then prepare a second checkpoint, which reaches prepared before the first prepare wakes.

When the first prepare woke, ParticipantControlPlane.prepare in nemo_gym/_checkpoint/control.py replied with self.phase, the phase of whichever checkpoint was open by then.
In the example above, the first prepare reported prepared for a checkpoint that had already ended.

The participant's state stayed safe.
The lease expiry had fenced the first checkpoint's ID, so a commit for it is refused as stale_checkpoint.
Only the reply was wrong, and a controller reading it could believe its own checkpoint was prepared.

A prepare whose checkpoint is no longer the open one now replies idle, together with the current readiness report.
This follows the existing reply for a prepare that a resume interrupts, which already reported idle.
The controller in nemo_gym/_checkpoint/coordination.py treats any phase other than prepared as not prepared.
Once a resume or lease expiry has ended the checkpoint, a later prepare or commit for it is refused as stale_checkpoint.

A stale_checkpoint refusal was the other candidate reply.
It was not chosen because the reply to a prepare interrupted by a resume, which is the same situation, is already idle, and a unit test pins that reply.
Using one reply for both keeps the contract the same for every way a checkpoint can end while a prepare waits.

A prepare that wakes after a concurrent prepare of the same checkpoint finished it still reports that checkpoint's phase, because that phase belongs to its own checkpoint.

Issue

None needed.
This is a small fix for a race found by a test in the checkpoint stack's fault tests (#4197).

Validation

A new unit test in tests/unit_tests/test_checkpoint_control.py holds the first prepare at a gate while it waits for readiness.
While it is held, the lease expires, admission reopens, and a second checkpoint is prepared.
The test then releases the first prepare and checks three things: the second prepare reports prepared, the first prepare reports idle rather than the second checkpoint's phase, and a commit for the first checkpoint is refused as stale_checkpoint.
The test uses events rather than fixed sleeps, apart from waiting for the lease to expire.
On main without the fix, it fails because the first prepare reports prepared.

Commands, run with PYTHONPATH set to the worktree:

python -m pytest tests/unit_tests/test_checkpoint_*.py -n 16
python -m pytest tests/unit_tests -n 16 -m "not sandbox"
ruff check nemo_gym/_checkpoint/control.py tests/unit_tests/test_checkpoint_control.py
ruff format nemo_gym/_checkpoint/control.py tests/unit_tests/test_checkpoint_control.py
pre-commit run --files nemo_gym/_checkpoint/control.py tests/unit_tests/test_checkpoint_control.py

Rollout evidence

N/A.
The change affects only the control-plane reply to a prepare whose checkpoint ended while it waited, and a deterministic unit test covers that case.
No environment, agent, or verifier behavior changes.

Compatibility

The reply to /ng-control/v1/checkpoint/prepare changes only in this race: it reports idle where it used to report another checkpoint's phase.
The reply shape is unchanged.
A controller that checked for prepared now correctly sees its checkpoint as not prepared.

… it waited

A prepare that waits for the participant to become ready can wake after its checkpoint has ended.
For example, its lease expires and admission reopens, then the controller prepares a second checkpoint.
When the first prepare woke, it replied with the participant's current phase,
which by then belonged to the second checkpoint.
So the first prepare could report `prepared` for a checkpoint that no longer existed.

The participant's state stayed safe.
The lease expiry had already fenced the first checkpoint's ID,
so a commit for it is refused as `stale_checkpoint`.
Only the reply was misleading,
and a controller reading it could believe its own checkpoint was prepared.

A prepare whose checkpoint is no longer the open one now replies `idle`, with the current readiness report.
This matches the reply of a prepare that a resume interrupts, which already reported `idle`.
A controller treats any phase other than `prepared` as not prepared.
Once a resume or a lease expiry has ended the checkpoint, a later prepare or commit for it is refused as `stale_checkpoint`.
A prepare that wakes after a concurrent prepare of the same checkpoint finished still reports that checkpoint's phase.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>

@macandro96 macandro96 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.

good catch! lgtm

@ananthsub
ananthsub added this pull request to the merge queue Oct 16, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/38833982337

Merged via the queue into main with commit 98ffb64 Oct 16, 2026
49 of 54 checks passed
@ananthsub
ananthsub deleted the ananthsub/checkpoint-prepare-stale-reply branch October 16, 2026 01:27

This branch was previously deployed

1 inactive deployment
public — 9140e7c2 Deployed Oct 15, 2026 by copy-pr-bot[bot] via release / finalize / notify #5588
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants