Skip to content

fix(allocator): retry assign_vm when SKIP LOCKED hides a transiently locked seat - #499

Merged
7174Andy merged 1 commit into
mainfrom
fix/assign-vm-skip-locked-false-empty
Sep 3, 2026
Merged

7174Andy merged 1 commit into
mainfrom
fix/assign-vm-skip-locked-false-empty

Conversation

@7174Andy

@7174Andy 7174Andy commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Under concurrent seat requests, assign_vm could return a false "no seats available" (HTTP 503) while a VM was still running, healthy, and unassigned.
  • Root cause: the atomic claim uses FOR UPDATE SKIP LOCKED, which skips any eligible row another transaction is briefly holding a lock on (a client heartbeat or other short update on the last free VM). An empty claim was treated as an empty pool.
  • Fix keeps the atomic claim and SKIP LOCKED, but distinguishes "genuinely empty" from "transiently locked" before giving up.

The bug, observed live

During a 30-VM workshop-scaling run, client VM 9 remained running, Healthy, and unassigned while seat request 12 received a 503. SKIP LOCKED had skipped VM 9's momentarily-locked row and the handler reported the pool empty.

Changes Made

  • packages/allocator/src/lablink_allocator_service/db/vms.pyassign_vm:
    • Factor the eligibility predicate into one clause shared by the claim and a new existence check.
    • On an empty claim, run SELECT EXISTS (... eligibility ...). If a free VM still exists, retry with bounded exponential backoff (10 attempts, 10 ms → 100 ms, ~0.65 s worst case); only raise ValueError (→ 503 no_seats) when the pool is confirmed empty.
    • Exhausted retries raise RuntimeError rather than a false empty-pool.
  • packages/allocator/tests/db/test_vms.py — real-Postgres regression test test_assign_vm_retries_temporarily_locked_eligible_vm: locks the only free VM from a second connection, releases the lock mid-retry, and asserts the assignment then succeeds on that exact host.

Testing

  • New regression test passes against real Postgres (postgres:16).
  • Full allocator suite: 981 passed (pytest --ignore=tests/terraform, real Postgres); existing concurrency guarantees (no-double-assignment, oversubscribed) still hold.
  • ruff check clean on both changed files.
  • Live validation on AWS (g4dn.xlarge, us-west-2), fixed allocator, concurrent claim bursts against a full pool:
    • N=5 → 5/5 claimed
    • N=10 → 10/10 claimed
    • N=30 → 30/30 claimed (the exact size that produced the original false 503)
    • N=60 → 60/60 claimed
    • Zero false 503s across 105 concurrent claims. Grant latency rises with N (0.22 s → 1.12 s median) — the expected contention cost of concurrent claims plus per-session VNC password rotation, not a fault.

Design Decisions

  • Retry rather than drop SKIP LOCKED. SKIP LOCKED is what prevents concurrent requesters from colliding on one row; removing it would reintroduce the double-assignment race. The lock that caused the false negative is transient, so a bounded retry is the minimal correct fix.
  • Confirm-empty before 503. The existence check uses the identical eligibility predicate as the claim, so "no row claimable" and "no row eligible" can't diverge.
  • Bounded, not unbounded. 10 attempts with capped backoff (~0.65 s worst case) keeps a genuinely empty pool fast to 503 and a contended pool from turning a student away; exhaustion is a distinct RuntimeError, not a silent empty-pool.

Related Issues

None filed; discovered during workshop-scaling measurement.

…locked seat

`assign_vm` claimed a seat with `FOR UPDATE SKIP LOCKED`, which skips any
eligible row another transaction holds a lock on — a client heartbeat or
other short update on the final free VM. When the claim returned no row the
code raised "no available VMs" and the caller returned 503 no_seats, even
though an eligible VM was still running, healthy, and unassigned. Observed
live at N=30: VM 9 stayed free and Healthy while seat request 12 got a 503.

Keep the atomic claim and SKIP LOCKED. On an empty claim, run an EXISTS
check with the identical eligibility clause: raise ValueError (-> 503) only
when the pool is genuinely empty, otherwise retry with bounded exponential
backoff (10 attempts, 10ms -> 100ms, ~0.65s worst case) so a transient lock
resolves instead of turning a student away. Exhausted retries raise
RuntimeError rather than a false empty-pool.

Add a real-Postgres regression test that locks the only free VM, releases
the lock mid-retry, and asserts the assignment then succeeds.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@7174Andy
7174Andy merged commit 5ad7d2a into main Sep 3, 2026
10 checks passed
@7174Andy
7174Andy deleted the fix/assign-vm-skip-locked-false-empty branch September 3, 2026 23:27
7174Andy added a commit that referenced this pull request Sep 14, 2026
data/quarantine/ held the pre-fix N=30 diagnostic run that exposed the
#499 bug. Nothing plots it and the paper does not discuss it; the story is
recorded in PR #499 and its regression test. data/ is now exactly the four
figure runs plus allocator-deploy.json. The README's "Excluded runs" note
now points at #499 instead of a directory.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7174Andy added a commit that referenced this pull request Sep 16, 2026
data/quarantine/ held the pre-fix N=30 diagnostic run that exposed the
#499 bug. Nothing plots it and the paper does not discuss it; the story is
recorded in PR #499 and its regression test. data/ is now exactly the four
figure runs plus allocator-deploy.json. The README's "Excluded runs" note
now points at #499 instead of a directory.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7174Andy added a commit that referenced this pull request Sep 16, 2026
data/quarantine/ held the pre-fix N=30 diagnostic run that exposed the
#499 bug. Nothing plots it and the paper does not discuss it; the story is
recorded in PR #499 and its regression test. data/ is now exactly the four
figure runs plus allocator-deploy.json. The README's "Excluded runs" note
now points at #499 instead of a directory.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7174Andy added a commit that referenced this pull request Sep 16, 2026
data/quarantine/ held the pre-fix N=30 diagnostic run that exposed the
#499 bug. Nothing plots it and the paper does not discuss it; the story is
recorded in PR #499 and its regression test. data/ is now exactly the four
figure runs plus allocator-deploy.json. The README's "Excluded runs" note
now points at #499 instead of a directory.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7174Andy added a commit that referenced this pull request Sep 17, 2026
data/quarantine/ held the pre-fix N=30 diagnostic run that exposed the
#499 bug. Nothing plots it and the paper does not discuss it; the story is
recorded in PR #499 and its regression test. data/ is now exactly the four
figure runs plus allocator-deploy.json. The README's "Excluded runs" note
now points at #499 instead of a directory.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7174Andy added a commit that referenced this pull request Sep 17, 2026
data/quarantine/ held the pre-fix N=30 diagnostic run that exposed the
#499 bug. Nothing plots it and the paper does not discuss it; the story is
recorded in PR #499 and its regression test. data/ is now exactly the four
figure runs plus allocator-deploy.json. The README's "Excluded runs" note
now points at #499 instead of a directory.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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