Skip to content

fix(miles-pipeline): honor R11-F1 ledger contract on failed releases - #33

Merged
taoluo merged 1 commit into
zhenyu/miles-mvp-e2efrom
joe/ledger-flag-retry
Aug 3, 2026
Merged

fix(miles-pipeline): honor R11-F1 ledger contract on failed releases#33
taoluo merged 1 commit into
zhenyu/miles-mvp-e2efrom
joe/ledger-flag-retry

Conversation

@JunzheJoe

Copy link
Copy Markdown
Member

What

Two fixes in the same scheduler-ledger leak family as #14, found during the F1-F12 code review (findings F8-SHUTDOWN-FLAGS and F8-INIT-RELEASE-HANG).

1. shutdown_hard cleared ledger flags even when the release RPC failed

_notify_release_cluster_gpus's docstring documents the R11-F1 contract: "shutdown_hard will retry the release if the flag stays True". But shutdown_hard itself cleared both _actor_train_allocated / _actor_infer_allocated unconditionally after the release loop — a transient RPC failure meant a second shutdown_hard / dispose call skipped the retry and the scheduler ledger leaked the allocation (peer pipelines starve).

Now: flags flip per-cluster, and only after a successful release; failures keep the flag set for retry.

2. Init-phase release failures fell through instead of failing fast

Phase A step 7 (release actor_train before requesting actor_infer) and phase B step 8 (release INIT actor_infer before the GENERATION re-request) only logged a warning on failure and continued. On a pool sized for one role at a time (the exact scenario the P1-7 comment describes), the next request_gpus — which blocks without timeout by design — would wait forever on GPUs this same pipeline still holds: a silent deadlock instead of an error.

Now: both sites raise; initialize_pipeline's except path runs shutdown_hard, which retries the release.

Tests

  • tests/test_miles_pipeline_shutdown_ledger.py
    • behavioral: release failure keeps the flag, the successful peer clears its own; after recovery a second shutdown_hard retries only the leaked cluster (ray stubbed via the sys.modules pattern from test_scheduler_apply_plan_invariants.py)
    • AST asserts for both init fail-fast raises (same pattern as test_miles_pipeline_after_training_cleanup.py from fix(rlix): always resume generation after finalize #14)
  • Full suite: 69 passed, 3 skipped (baseline 59 + 10 new across the review-fix PRs)

Two fixes in the same leak family as #14:

1. shutdown_hard cleared both ledger flags unconditionally even when a
   notify_release_gpus RPC failed, contradicting the documented R11-F1
   contract ('shutdown_hard will retry the release if the flag stays
   True'). A second shutdown_hard/dispose call would skip the retry and
   leak the server-side allocation. Flags now flip per-cluster and only
   after a successful release.

2. Init-phase release failures (phase A step 7, phase B step 8) only
   logged a warning and fell through. On a pool sized for one role at a
   time, phase B's request_gpus (no timeout by design) would then block
   forever on GPUs this pipeline still holds. Both sites now raise;
   initialize_pipeline's except path runs shutdown_hard, which retries
   the release.

Tests: behavioral shutdown_hard flag-retention/retry (ray stubbed, same
pattern as test_scheduler_apply_plan_invariants) + AST fail-fast asserts
(same pattern as test_miles_pipeline_after_training_cleanup).
@zhenyulincs

Copy link
Copy Markdown
Collaborator

Cross-checked this against our smoke-test history, since at first glance it seemed odd that none of the 4x5090 dual smokes ever surfaced either bug. Conclusion: both bugs are real contract violations, but their trigger conditions are extremely narrow — they live exclusively on failure paths our current runs never exercise.

Bug 1 (shutdown_hard clearing ledger flags) needs all of the following to line up:

  1. the release RPC to the scheduler fails transiently (10s timeout or exception) during shutdown_hard,
  2. a second shutdown_hard / dispose call arrives later to attempt the retry,
  3. the scheduler is still alive and serving a peer pipeline (otherwise the leaked ledger entry is moot — a scheduler restart clears all state under the fail-fast design),
  4. that peer pipeline actually wants those GPUs.

On a single-node Ray cluster, ray.get(..., timeout=10) against a healthy local scheduler actor essentially never fails, and when a pipeline dies in our smokes the whole job tears down together — so the "one pipeline leaks, a peer starves" window never opens.

Bug 2 (init-phase release fall-through) only fires when the phase A step7 / phase B step8 release RPC itself fails. Every smoke so far released successfully, so the silent-deadlock branch (warn → next no-timeout request_gpus waits forever on GPUs this same pipeline still holds) has never been walked.

None of this argues against the PR — it explains why runtime testing couldn't have caught it, which is exactly the class of bug this review pass exists for. The exposure becomes real in multi-node deployments (network blips, scheduler overload, actor restart windows), the success path is behavior-identical, and the failure branches now match the documented R11-F1 contract.

@taoluo

taoluo commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

LGTM. The fix matches the ledger contract and correctly converts the init-time silent deadlock into an explicit failure.

@taoluo
taoluo merged commit 218daf1 into zhenyu/miles-mvp-e2e Aug 3, 2026
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.

3 participants