[Autoscaler] Request ID sharing causes instance starvation due to launch_errors dict collision and request_id reuse

[pjdurden]

Issue #65223 — Autoscaler v2: launch errors lost when a launch request spans node types

1. Root cause

Reconciler._handle_cloud_instance_allocation built its launch-error lookup keyed
only by the launch request id:

launch_errors: Dict[str, LaunchNodeError] = {
    error.request_id: error
    for error in cloud_provider_errors
    if isinstance(error, LaunchNodeError)
}

A single launch request is not one node type. Reconciler._handle_instances_launch
assigns one new_launch_request_id to every instance it moves to REQUESTED in that
iteration, across all node types, and CloudInstanceUpdater then groups those events by
launch_request_id and issues one launch(shape={type-1: n, type-2: m}, request_id=...)
call. Both providers fan that shape out into one LaunchNodeError per node type with the
same request_id:

• NodeProviderAdapter._launch_nodes_by_type (node_provider.py:494) — one error per
  node type, and per launch batch.
• KubeRayProvider._add_launch_errors (kuberay/cloud_provider.py:437) — explicitly
  loops for node_type, count in shape.items().

Because the dict was keyed on request_id alone, those sibling errors overwrote each
other and only the last one survived. The lookup at the call site then guarded with
launch_error.node_type == im_instance.instance_type, so any instance whose node type
was the overwritten one matched nothing and returned None — no update.

Consequence: for a launch request covering N node types that all fail, only one node
type transitions to ALLOCATION_FAILED. The others sit in REQUESTED until
request_status_timeout_s fires, blocking max_concurrent_launches capacity and, worse,
still being eligible for allocation in later reconcile passes — so a stale REQUESTED
instance can claim an unassigned cloud instance (Pod) that was launched for a different
request. That is the "instance starvation" reported in the issue.

2. The fix and why

Key the launch errors by (request_id, node_type) — the pair that actually identifies a
launch error — and look them up with the same composite key:

launch_errors: Dict[Tuple[str, NodeType], LaunchNodeError] = {
    (error.request_id, error.node_type): error
    for error in cloud_provider_errors
    if isinstance(error, LaunchNodeError)
}
...
launch_error = launch_errors.get(
    (im_instance.launch_request_id, im_instance.instance_type)
)
if launch_error:
    ...

The now-redundant launch_error.node_type == im_instance.instance_type guard is folded
into the key, so the matching semantics for the previously-surviving error are unchanged;
the only behavior difference is that errors that used to be dropped are now honored.

This is the minimal change: it stays inside the reconciler, keeps the isinstance
filter (the error list also carries TerminateNodeErrors, which have no node_type),
and does not touch request-id generation or the provider error plumbing.

Note on scope: the issue title also mentions "request_id reuse" (_handle_instances_launch
deliberately reuses an instance's existing launch_request_id on retry). That reuse is
intentional and is only harmful because of the collision above — once errors are keyed
per node type, a retried instance matches only errors for its own type. Changing the reuse
policy would alter _fill_autoscaling_state's pending-request grouping and retry
accounting, so it is left alone.

3. Files changed

• python/ray/autoscaler/v2/instance_manager/reconciler.py
  • _handle_cloud_instance_allocation: launch_errors keyed by
    (request_id, node_type), with a comment explaining why.
  • _try_resolve_pending_allocation: signature type updated, docstring updated,
    composite-key lookup replacing the key + node_type comparison.
• python/ray/autoscaler/v2/tests/test_reconciler.py
  • New TestReconciler.test_multiple_node_types_share_launch_request_id: two
    REQUESTED instances of different types sharing launch request l1, one
    LaunchNodeError per type; asserts both reach ALLOCATION_FAILED. This test fails
    on the unpatched reconciler (only i-2 fails; i-1 stays REQUESTED).

4. Risk / uncertainty

• Low blast radius: one dict key and one lookup, both local to allocation reconciliation.
• Behavior change is strictly "more instances get failed" for launch requests that
  previously lost errors. Instances that already transitioned correctly are unaffected —
  the old code required node_type to match anyway.
• LaunchNodeError.count is still ignored: if a shape of 10 of one type is split into
  two provider batches and only one batch errors, all 10 REQUESTED instances of that
  type fail. That pre-existing behavior is unchanged here; fixing it needs per-instance
  attribution the errors don't carry.
• Duplicate (request_id, node_type) errors (same type split across launch batches) still
  collapse to the last one. Harmless — the match is boolean; only details in the failure
  message could differ.
• The existing test_requested_instance_to_allocation_failed covers the single-error case
  and its expectations are unchanged.

5. How I verified it

• Traced the shared-request_id path end to end: _handle_instances_launch →
  CloudInstanceUpdater._launch_new_instances → NodeProviderAdapter._do_launch /
  KubeRayProvider._add_launch_errors → _handle_cloud_instance_allocation, confirming a
  single request_id legitimately produces multiple LaunchNodeErrors.

• Reproduced the collision in isolation (standalone script replicating both the old and
  new keying against two errors sharing request_id="l1" with types type-1/type-2):
  old keying resolves only i-2 to failed, new keying resolves both i-1 and i-2.
  This matches the reproduction in the issue.

• Byte-compiled both changed files (python -m py_compile) — clean.

• Checked the diff for line-length violations against Ray's 88-column limit — none.

• Not run: pytest python/ray/autoscaler/v2/tests/test_reconciler.py. Ray is not
  installed in this environment (no ray module in any available interpreter) and the
  test imports ray.core.generated.* protobufs, which require a source build. The new
  test must be executed by the human submitter before this is proposed upstream:

  pytest -q python/ray/autoscaler/v2/tests/test_reconciler.py \
      -k "allocation_failed or share_launch_request_id"

[gemini-code-assist]

Code Review

This pull request updates the reconciler to key launch errors by a tuple of (request_id, node_type) instead of just request_id. This prevents launch errors of different node types from the same request from overwriting each other. A corresponding unit test has been added to verify this behavior. There are no review comments, so I have no feedback to provide.

[Copilot]

Pull request overview

Fixes a correctness bug in Autoscaler v2’s allocation reconciliation where launch errors could be dropped when a single launch request spans multiple node types, causing some REQUESTED instances to never transition to ALLOCATION_FAILED.

Changes:

• Key launch errors by (request_id, node_type) instead of request_id alone to avoid overwriting sibling errors from the same launch request.
• Update _try_resolve_pending_allocation to perform a composite-key lookup and simplify the matching logic.
• Add a unit test covering multiple node types sharing the same launch_request_id and verifying both fail as expected.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
python/ray/autoscaler/v2/instance_manager/reconciler.py	Prevents launch error collisions by using a composite key (request_id, node_type) during allocation failure resolution.
python/ray/autoscaler/v2/tests/test_reconciler.py	Adds regression coverage ensuring multiple node types in the same launch request all transition to ALLOCATION_FAILED on launch failure.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[pjdurden]

rebased on current master (was 105 behind), applies clean.

verified the new case locally against a commit matched master wheel. the added test fails on master and passes with the change. the 4 other failures in that file are pre existing in my local setup, they fail identically on unmodified master, so 71 passed on master vs 72 here.

anyone free to take a look? its a small change, keying the launch error lookup by request id and node type instead of request id alone.