[smahima27]

Problem

ABS-issued on-demand requests are intermittently marked 'failed' — and their VMs destroyed — even when the VMs had already finished provisioning.

Traced in production (vmpooler 3.9.1) across several occurrences. Representative case, a 4-VM request:

06:50:21  wild-glen         ready  (1/4)
06:50:24  vile-pilgrimage   ready  (2/4)
06:50:27  well-founding     ready  (3/4)
06:50:59  ⚠ request declared expired, all VMs moved to completed
06:51:02  hairy-recruit     ready  (4/4)  ← 2s after the batch was destroyed

Elapsed-to-failure across occurrences was 167s / 357s / 378s — nowhere near a stable TTL boundary, and vmpooler_running was well under abs_capacity_aware_scheduler_capacity_maximum throughout, so this is a race rather than genuine capacity or timeout exhaustion.

Root cause

Two unsynchronized code paths act on the same on-demand request:

• the ondemand_provisioner thread's periodic expiry sweep (check_ondemand_requests_ready → request_expired?)
• a per-VM pending check (_check_pending_vm → move_pending_vm_to_ready), spawned as a fresh Thread.new per VM per cycle

Both read/write vmpooler__odrequest__<id> and vmpooler__provisioning__processing. The only existing lock (vm_mutex) is per-VM, so nothing prevented these from interleaving on the same request_id.

Changes

1. Check readiness before expiry in check_ondemand_request_ready. Previously expiry was evaluated first, so a request that had just become fully provisioned could still be killed by a sweep landing on the same tick.
2. Guard request_expired? against a nil score. redis.zscore returns nil once a request leaves the processing set, and nil.to_i is 0, so delta = Time.now.to_i - 0 produced a multi-decade "elapsed" value that exceeded any configured TTL regardless of the request's real age.
3. Add per-request locking (request_mutex) around the critical sections in both check_ondemand_request_ready and move_pending_vm_to_ready. Split into a lock-acquiring entry point and a lock-free _locked variant, since move_pending_vm_to_ready already holds the lock when it reaches the readiness check and Ruby's Mutex is not reentrant.
4. Striped mutexes rather than a growable per-request_id map. A compute_if_absent + delete map risks handing two callers two different Mutex objects for the same request_id (one already holding a reference to the deleted entry, a later one creating a fresh entry), which would defeat the guarantee entirely. A fixed 256-entry array needs no creation or deletion.
5. Don't resurrect terminal requests. The ready branch now bails out on failed/deleted and drops the request from the processing set.
6. Switched pool_mutex / vm_mutex / request_mutex to atomic compute_if_absent (per review), which required moving the backing collections from Concurrent::Hash to Concurrent::Map — Concurrent::Hash is a plain Hash subclass under MRI and only gains atomic operations under JRuby's native backing, and this needs to work on both.

⚠️ Two things I'd like a maintainer decision on

1. request_mutex is an in-process Mutex, and the state it guards is not process-local.

bin/vmpooler runs api and manager as separately deployable modes, and they are deployed separately (the manager pod's args are ["manager"]). The ondemand delete endpoint mutates the very same state from the api process, non-atomically and unsynchronized:

# lib/vmpooler/api/v3.rb
backend.hset("vmpooler__odrequest__#{request_id}", 'status', 'deleted')
... backend.smove("vmpooler__running__#{pool}", "vmpooler__completed__#{pool}", vm)
... backend.del("vmpooler__#{request_id}__#{platform_alias}__#{pool}")

The race actually observed in production was manager-internal (sweep vs. VM completion threads), so this fix does address the reported bug. But it follows that (a) a class of cross-process races remains unaddressed, and (b) the fix silently stops working if the manager is ever scaled beyond one replica, and nothing in the code or chart enforces that. A fully general fix would need redis-level atomicity (Lua, WATCH/MULTI, or a redis lock). I've documented the limitation in the code but would rather you decide whether that's acceptable here or whether the replica count should be constrained.

2. Preferring readiness over expiry changes behaviour for late-completing requests.

Before: an expired request was failed and its VMs destroyed promptly. After: if all VMs are present, it is fulfilled regardless of age. For a request that overshoots its TTL and then completes, ABS has already given up (its own client-side timeout), so those VMs are marked ready for a request nobody is polling and only get reaped by normal vm_lifetime — so capacity lingers rather than being reclaimed immediately. I think not destroying VMs that actually succeeded is the better default, but it is a deliberate trade-off and could reasonably be bounded to a grace window instead of applied unconditionally.

Testing

• bundle exec rspec spec/unit/pool_manager_spec.rb spec/integration/api/v3/ondemandvm_spec.rb → 461 examples, 0 failures
• bundle exec rubocop lib/vmpooler/pool_manager.rb → no offenses
• Updated the existing tests that asserted the old (pre-fix) call ordering.
• Each new test was verified to actually fail against the unfixed code, not just pass against the fixed code:
  • deadlock regression → reproduces ThreadError: deadlock; recursive locking with the _locked split reverted
  • mutual exclusion → a deterministic two-thread test that pauses move_pending_vm_to_ready mid-critical-section and proves the sweep cannot enter its own; reports :entered instead of :blocked with the request_mutex wrap removed
  • terminal-status guard → all six tests fail with the guard removed
  • nil-score guard → covered at both the check_ondemand_request_ready and request_expired? levels

An earlier version of the concurrency test passed even with the lock entirely removed (the readiness reorder alone was carrying it); that's why the current one instruments the shared critical section directly rather than asserting on end state.

Also noted, not fixed here

vm_mutex / dereference_mutex has the same delete-then-recreate hazard described in change 4 above. Left alone deliberately — it's pre-existing and used throughout the VM lifecycle rather than just the on-demand path this PR touches, so it seemed better as a separate change.

[CLAassistant]

All committers have signed the CLA.

[joshcooper]

This pattern of creating mutexes is racy. It's possible for calls from two threads to interleave:

th1: @request_mutex[request_id] #=> nil
th2: @request_mutex[request_id] #=> nil
th1: @request_mutex[request_id] = Mutex.new
th2: @request_mutex[request_id] = Mutex.new

Threads 1 and 2 would then synchronize on different mutexes, breaking mutual exclusion, and thread 2 would overwrite thread 1's mutex in the hash.

Suggest using:

Suggested change

	@request_mutex[request_id] || @request_mutex[request_id] = Mutex.new
	@request_mutex.compute_if_absent(request_id) { Mutex.new }

Could you change pool_mutex and vm_mutex too?

[smahima27]

Good catch, thanks — you're right that this is exactly the kind of race this PR is meant to fix, and I'd introduced a new instance of it. Fixed in 283e57f: switched pool_mutex, vm_mutex, and request_mutex to compute_if_absent.

One adjustment: compute_if_absent isn't available on Concurrent::Hash under MRI — it's just a plain Hash subclass there and only gets real atomic operations under JRuby's native ConcurrentHashMap backing (confirmed this app's CI/spec suite runs under MRI while the deployed image runs JRuby, so it needs to work on both). Switched the backing collections (@reconfigure_pool, @vm_mutex, @request_mutex) from Concurrent::Hash to Concurrent::Map, which provides compute_if_absent consistently on both runtimes. Full spec suite (452 examples) still passes.

[Copilot]

🟡 Changes recommended

The on-demand completion path recursively acquires a non-reentrant mutex and raises ThreadError.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds request-level synchronization and safer expiry handling for on-demand VM provisioning.

Changes:

• Checks readiness before expiration and handles missing Redis scores.
• Adds per-request mutexes around provisioning transitions.
• Updates regression tests for readiness and expiry behavior.

File summaries

File	Description
lib/vmpooler/pool_manager.rb	Adds synchronization and revises request completion logic.
spec/unit/pool_manager_spec.rb	Tests readiness ordering and nil-score handling.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

---

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

[Copilot]

🟡 Changes recommended

The central concurrency fix lacks a deterministic test exercising the competing threads.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Mutex cleanup can create multiple active locks for one request, defeating the new synchronization guarantee.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[smahima27]

I went back over this PR as a reviewer rather than an author, and pushed f85ba68 for what came out of that. I've also rewritten the PR description, which was stale (it still described only the original three changes and cited the wrong test count).

Fixed in f85ba68:

• Terminal-status guard. The ready branch wrote status='ready' unconditionally. The delete endpoint sets 'deleted' without zrem'ing the request from the processing set, so the sweep keeps visiting it. Not currently exploitable — the delete also dels the fulfillment sets so vms_ready? is false — but that's load-bearing coincidence, and putting readiness ahead of expiry made the branch more reachable. Now bails out on failed/deleted and zrems, which also preserves the previous cleanup behaviour (such requests used to leave the processing set only by being rewritten to 'failed').
• Lock ordering documented. _check_pending_vm holds vm_mutex while calling move_pending_vm_to_ready, which takes request_mutex — so the order is vm_mutex -> request_mutex. No reverse path exists today, but remove_vms_for_failed_request destroys VMs without their vm_mutex, and adding that locking is a plausible future change that would deadlock. Recorded the constraint next to request_mutex.
• Scope limitation documented, plus the _locked method now carries its own "must hold the lock" warning, the stripe constant moved to the class constant block, and comments explaining what the concurrency test's timing window does and doesn't prove.

Two things I'd rather you decide than quietly decide myself (both now in the description):

1. request_mutex is an in-process Mutex, but the state it guards isn't process-local — the api process mutates request status and VM queues in the delete endpoint with no coordination. The race actually seen in production was manager-internal, so this does fix the reported bug, but it means a class of cross-process races remains, and the fix silently stops working if the manager is scaled past one replica. A fully general fix would need redis-level atomicity. Happy to pursue that instead if you'd prefer.
2. Checking readiness before expiry means a request that overshoots its TTL and then completes now gets fulfilled rather than failed, so its VMs linger until normal vm_lifetime reaping instead of being reclaimed immediately. I think that beats destroying VMs that succeeded, but it could be bounded to a grace window if you'd rather keep the old behaviour for genuinely stale requests.

Also flagging that vm_mutex/dereference_mutex has the same delete-then-recreate hazard I fixed for request_mutex in 8a20088. I left it alone since it's pre-existing and spans the whole VM lifecycle rather than just the on-demand path — happy to do it as a follow-up if useful.

Full suite: 461 examples, 0 failures; rubocop clean. Each new test verified to fail against the unfixed code.

[Copilot]

🟡 Changes recommended

Cross-process status resurrection remains possible, and the intended TTL semantics require resolution.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

lib/vmpooler/pool_manager.rb:2462

• The terminal-status guard and this write are separate Redis operations, so the API can set deleted after the guard/readiness checks and this line can overwrite it with ready after the API has already moved the VMs to completed. The in-process mutex cannot prevent that cross-process TOCTOU race (or the same race between manager replicas). Make the status transition conditional/atomic in Redis and coordinate the associated request-set/queue mutations so a terminal status cannot be resurrected.

        redis.hset(ondemand_hash_key, 'status', 'ready')

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced