From cb46bd0b5fefb406875eeb876bbe666c8c3d9de3 Mon Sep 17 00:00:00 2001
From: Mahima Singh <mahima.singh@perforce.com>
Date: Tue, 8 Sep 2026 15:14:07 +0530
Subject: [PATCH 1/6] Fix race condition destroying completed ondemand VM
 requests

ABS-issued multi-VM ondemand requests were occasionally being marked
'failed' and having their VMs destroyed even when all requested VMs
had already finished cloning. Root cause was two independent, racing
code paths with no shared lock:

- the ondemand_provisioner thread's periodic sweep
  (check_ondemand_requests_ready -> request_expired?), and
- the per-VM pending-check thread (_check_pending_vm ->
  move_pending_vm_to_ready), spawned fresh per VM per check cycle.

Both read/write the same vmpooler__odrequest__<id> status and the
vmpooler__provisioning__processing zset with only a per-VM mutex
(vm_mutex) protecting VM state, not the request as a whole.

This fixes it three ways:

1. In check_ondemand_request_ready, check vms_ready? before
   request_expired?, so a request that just became fully provisioned
   is marked ready instead of being killed by a same-tick expiry
   check.
2. Guard request_expired? against a nil score. redis.zscore returns
   nil once an entry is removed from the processing set; nil.to_i is
   0, which previously produced a multi-decade "elapsed" delta that
   trivially exceeded any configured ondemand_request_ttl.
3. Add a per-request_id mutex (request_mutex, mirroring the existing
   vm_mutex pattern) around the critical sections in both
   check_ondemand_request_ready and move_pending_vm_to_ready, so the
   expiry sweep and a VM's completion callback can no longer
   interleave on the same request.
---
 lib/vmpooler/pool_manager.rb   | 85 ++++++++++++++++++++++------------
 spec/unit/pool_manager_spec.rb | 27 ++++++++++-
 2 files changed, 81 insertions(+), 31 deletions(-)

diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index 0580d332..66835953 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -37,6 +37,7 @@ def initialize(config, logger, redis_connection_pool, metrics)
       @reconfigure_pool = Concurrent::Hash.new
 
       @vm_mutex = Concurrent::Hash.new
+      @request_mutex = Concurrent::Hash.new
 
       # Name generator for generating host names
       @name_generator = Spicy::Proton.new
@@ -242,29 +243,31 @@ def move_pending_vm_to_ready(vm, pool, redis, request_id = nil)
       finish = format('%<time>.2f', time: Time.now - Time.parse(clone_time))
 
       if request_id
-        ondemandrequest_hash = redis.hgetall("vmpooler__odrequest__#{request_id}")
-        case ondemandrequest_hash['status']
-        when 'failed'
-          move_vm_queue(pool, vm, 'pending', 'completed', redis, "moved to completed queue. '#{request_id}' could not be filled in time")
-          return nil
-        when 'deleted'
-          move_vm_queue(pool, vm, 'pending', 'completed', redis, "moved to completed queue. '#{request_id}' has been deleted")
-          return nil
-        end
-        pool_alias = redis.hget("vmpooler__vm__#{vm}", 'pool_alias')
+        request_mutex(request_id).synchronize do
+          ondemandrequest_hash = redis.hgetall("vmpooler__odrequest__#{request_id}")
+          case ondemandrequest_hash['status']
+          when 'failed'
+            move_vm_queue(pool, vm, 'pending', 'completed', redis, "moved to completed queue. '#{request_id}' could not be filled in time")
+            return nil
+          when 'deleted'
+            move_vm_queue(pool, vm, 'pending', 'completed', redis, "moved to completed queue. '#{request_id}' has been deleted")
+            return nil
+          end
+          pool_alias = redis.hget("vmpooler__vm__#{vm}", 'pool_alias')
 
-        redis.pipelined do |pipeline|
-          pipeline.hset("vmpooler__active__#{pool}", vm, Time.now.to_s)
-          pipeline.hset("vmpooler__vm__#{vm}", 'checkout', Time.now.to_s)
-          if ondemandrequest_hash['token:token']
-            pipeline.hset("vmpooler__vm__#{vm}", 'token:token', ondemandrequest_hash['token:token'])
-            pipeline.hset("vmpooler__vm__#{vm}", 'token:user', ondemandrequest_hash['token:user'])
-            pipeline.hset("vmpooler__vm__#{vm}", 'lifetime', $config[:config]['vm_lifetime_auth'].to_i)
+          redis.pipelined do |pipeline|
+            pipeline.hset("vmpooler__active__#{pool}", vm, Time.now.to_s)
+            pipeline.hset("vmpooler__vm__#{vm}", 'checkout', Time.now.to_s)
+            if ondemandrequest_hash['token:token']
+              pipeline.hset("vmpooler__vm__#{vm}", 'token:token', ondemandrequest_hash['token:token'])
+              pipeline.hset("vmpooler__vm__#{vm}", 'token:user', ondemandrequest_hash['token:user'])
+              pipeline.hset("vmpooler__vm__#{vm}", 'lifetime', $config[:config]['vm_lifetime_auth'].to_i)
+            end
+            pipeline.sadd("vmpooler__#{request_id}__#{pool_alias}__#{pool}", vm)
           end
-          pipeline.sadd("vmpooler__#{request_id}__#{pool_alias}__#{pool}", vm)
+          move_vm_queue(pool, vm, 'pending', 'running', redis)
+          check_ondemand_request_ready(request_id, redis)
         end
-        move_vm_queue(pool, vm, 'pending', 'running', redis)
-        check_ondemand_request_ready(request_id, redis)
       else
         redis.smove("vmpooler__pending__#{pool}", "vmpooler__ready__#{pool}", vm)
       end
@@ -1782,6 +1785,14 @@ def vm_mutex(vmname)
       @vm_mutex[vmname] || @vm_mutex[vmname] = Mutex.new
     end
 
+    def request_mutex(request_id)
+      @request_mutex[request_id] || @request_mutex[request_id] = Mutex.new
+    end
+
+    def dereference_request_mutex(request_id)
+      true if @request_mutex.delete(request_id)
+    end
+
     def dereference_mutex(vmname)
       true if @vm_mutex.delete(vmname)
     end
@@ -2386,21 +2397,34 @@ def check_ondemand_requests_ready(redis)
     end
 
     def check_ondemand_request_ready(request_id, redis, score = nil)
-      # default expiration is one month to ensure the data does not stay in redis forever
-      default_expiration = 259_200_0
-      processing_key = 'vmpooler__provisioning__processing'
-      ondemand_hash_key = "vmpooler__odrequest__#{request_id}"
-      score ||= redis.zscore(processing_key, request_id)
-      return if request_expired?(request_id, score, redis)
+      request_mutex(request_id).synchronize do
+        # default expiration is one month to ensure the data does not stay in redis forever
+        default_expiration = 259_200_0
+        processing_key = 'vmpooler__provisioning__processing'
+        ondemand_hash_key = "vmpooler__odrequest__#{request_id}"
 
-      return unless vms_ready?(request_id, redis)
+        # Check readiness before expiry: a request that just became fully
+        # provisioned must not be killed by a concurrent expiry sweep landing
+        # on the same tick.
+        if vms_ready?(request_id, redis)
+          redis.hset(ondemand_hash_key, 'status', 'ready')
+          redis.expire(ondemand_hash_key, default_expiration)
+          redis.zrem(processing_key, request_id)
+          dereference_request_mutex(request_id)
+          next
+        end
 
-      redis.hset(ondemand_hash_key, 'status', 'ready')
-      redis.expire(ondemand_hash_key, default_expiration)
-      redis.zrem(processing_key, request_id)
+        score ||= redis.zscore(processing_key, request_id)
+        request_expired?(request_id, score, redis)
+      end
     end
 
     def request_expired?(request_id, score, redis)
+      # A missing score means there is nothing in the processing set to
+      # expire (e.g. the request already completed) - treat as not expired
+      # rather than letting `nil.to_i` (0) produce a runaway delta.
+      return false if score.nil?
+
       delta = Time.now.to_i - score.to_i
       ondemand_request_ttl = $config[:config]['ondemand_request_ttl']
       return false unless delta > ondemand_request_ttl * 60
@@ -2413,6 +2437,7 @@ def request_expired?(request_id, score, redis)
         pipeline.expire("vmpooler__odrequest__#{request_id}", expiration_ttl)
       end
       remove_vms_for_failed_request(request_id, expiration_ttl, redis)
+      dereference_request_mutex(request_id)
       true
     end
 
diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 1b2ccef2..dd176092 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -5428,7 +5428,9 @@
       before(:each) do
         expect(subject).to receive(:vms_ready?).and_return(true)
         redis_connection_pool.with do |redis|
-          expect(redis).to receive(:zscore).and_return(score)
+          # readiness is checked before expiry, so a ready request must not
+          # need to look up its score at all
+          allow(redis).to receive(:zscore).and_return(score)
         end
       end
 
@@ -5468,12 +5470,25 @@
     context 'when a request has taken too long to be filled' do
       it 'should return true for request_expired?' do
         redis_connection_pool.with do |redis|
+          expect(subject).to receive(:vms_ready?).and_return(false)
           expect(redis).to receive(:zscore).and_return(score)
           expect(subject).to receive(:request_expired?).with(request_id, Float, redis).and_return(true)
           subject.check_ondemand_request_ready(request_id, redis)
         end
       end
     end
+
+    context 'when the request is not ready and the score has already been removed from processing' do
+      it 'does not treat a missing score as an infinitely old request' do
+        redis_connection_pool.with do |redis|
+          expect(subject).to receive(:vms_ready?).and_return(false)
+          expect(redis).to receive(:zscore).and_return(nil)
+          expect(subject).to receive(:request_expired?).with(request_id, nil, redis).and_call_original
+          expect(subject).to_not receive(:remove_vms_for_failed_request)
+          subject.check_ondemand_request_ready(request_id, redis)
+        end
+      end
+    end
   end
 
   describe '#request_expired?' do
@@ -5549,6 +5564,16 @@
         end
       end
     end
+
+    context 'with a nil score' do
+      it 'returns false without treating the missing score as an infinitely old request' do
+        redis_connection_pool.with do |redis|
+          expect(subject).to_not receive(:remove_vms_for_failed_request)
+          result = subject.request_expired?(request_id, nil, redis)
+          expect(result).to be false
+        end
+      end
+    end
   end
 
   describe '#remove_vms_for_failed_request)' do

From 283e57f115812843820fc198b760895ea5e6c934 Mon Sep 17 00:00:00 2001
From: Mahima Singh <mahima.singh@perforce.com>
Date: Wed, 9 Sep 2026 19:34:20 +0530
Subject: [PATCH 2/6] Fix racy mutex-creation pattern flagged in review

joshcooper pointed out that request_mutex (and the pre-existing
pool_mutex/vm_mutex it was modeled on) had a check-then-act race:

  @request_mutex[request_id] || @request_mutex[request_id] = Mutex.new

Two threads can both observe nil for the same key before either
writes, ending up synchronized on two different Mutex objects for
the same request_id/vm/pool - silently defeating mutual exclusion,
which is exactly the class of bug this PR exists to fix.

Switched pool_mutex, vm_mutex, and request_mutex to the atomic
compute_if_absent. That method isn't available on Concurrent::Hash
under MRI (it's just a Hash subclass there, only gaining real
atomic ops under JRuby's native backing), so also switched the
backing collections from Concurrent::Hash to Concurrent::Map, which
provides compute_if_absent consistently on both the JRuby runtime
this app deploys on and the MRI Ruby the test suite runs under.
---
 lib/vmpooler/pool_manager.rb | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index 66835953..ec83f2b1 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -34,10 +34,10 @@ def initialize(config, logger, redis_connection_pool, metrics)
       $threads = Concurrent::Hash.new
 
       # Pool mutex
-      @reconfigure_pool = Concurrent::Hash.new
+      @reconfigure_pool = Concurrent::Map.new
 
-      @vm_mutex = Concurrent::Hash.new
-      @request_mutex = Concurrent::Hash.new
+      @vm_mutex = Concurrent::Map.new
+      @request_mutex = Concurrent::Map.new
 
       # Name generator for generating host names
       @name_generator = Spicy::Proton.new
@@ -1778,15 +1778,15 @@ def check_pool(pool,
     end
 
     def pool_mutex(poolname)
-      @reconfigure_pool[poolname] || @reconfigure_pool[poolname] = Mutex.new
+      @reconfigure_pool.compute_if_absent(poolname) { Mutex.new }
     end
 
     def vm_mutex(vmname)
-      @vm_mutex[vmname] || @vm_mutex[vmname] = Mutex.new
+      @vm_mutex.compute_if_absent(vmname) { Mutex.new }
     end
 
     def request_mutex(request_id)
-      @request_mutex[request_id] || @request_mutex[request_id] = Mutex.new
+      @request_mutex.compute_if_absent(request_id) { Mutex.new }
     end
 
     def dereference_request_mutex(request_id)

From eccb2036d60c4a4c6292fa7f8737929834f9e3c3 Mon Sep 17 00:00:00 2001
From: Mahima Singh <mahima.singh@perforce.com>
Date: Wed, 9 Sep 2026 19:47:47 +0530
Subject: [PATCH 3/6] Fix recursive-locking deadlock flagged by Copilot review

move_pending_vm_to_ready already holds request_mutex(request_id) for
the whole request_id branch, then called check_ondemand_request_ready
at the end of that same branch - which immediately tried to
synchronize on the exact same mutex. Ruby's Mutex is not reentrant,
so this raised ThreadError: deadlock; recursive locking on every
single on-demand VM that reached the ready state, i.e. the happy
path for every successful on-demand provision.

Split check_ondemand_request_ready into:
- check_ondemand_request_ready: acquires request_mutex(request_id),
  used by callers that don't already hold it (the ondemand_provisioner
  sweep).
- check_ondemand_request_ready_locked: the actual logic, with no
  locking of its own, for callers that already hold the lock
  (move_pending_vm_to_ready).

Added a regression test that calls move_pending_vm_to_ready for real
(no stubbing of the ready-check) and asserts it completes without
raising - verified this test fails with ThreadError against the
prior shape of the code, and passes now.
---
 lib/vmpooler/pool_manager.rb   | 45 +++++++++++++++++++++-------------
 spec/unit/pool_manager_spec.rb | 31 +++++++++++++++++++++--
 2 files changed, 57 insertions(+), 19 deletions(-)

diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index ec83f2b1..fc1bbc30 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -266,7 +266,10 @@ def move_pending_vm_to_ready(vm, pool, redis, request_id = nil)
             pipeline.sadd("vmpooler__#{request_id}__#{pool_alias}__#{pool}", vm)
           end
           move_vm_queue(pool, vm, 'pending', 'running', redis)
-          check_ondemand_request_ready(request_id, redis)
+          # already holding request_mutex(request_id) from the top of this
+          # block, so call the lock-free variant directly to avoid deadlocking
+          # on Ruby's non-reentrant Mutex
+          check_ondemand_request_ready_locked(request_id, redis)
         end
       else
         redis.smove("vmpooler__pending__#{pool}", "vmpooler__ready__#{pool}", vm)
@@ -2396,27 +2399,35 @@ def check_ondemand_requests_ready(redis)
       in_progress_requests.length
     end
 
+    # Acquires the per-request lock before delegating. Callers that already
+    # hold request_mutex(request_id) (e.g. move_pending_vm_to_ready) must call
+    # check_ondemand_request_ready_locked directly instead - Ruby's Mutex is
+    # not reentrant, so re-synchronizing here would deadlock.
     def check_ondemand_request_ready(request_id, redis, score = nil)
       request_mutex(request_id).synchronize do
-        # default expiration is one month to ensure the data does not stay in redis forever
-        default_expiration = 259_200_0
-        processing_key = 'vmpooler__provisioning__processing'
-        ondemand_hash_key = "vmpooler__odrequest__#{request_id}"
+        check_ondemand_request_ready_locked(request_id, redis, score)
+      end
+    end
 
-        # Check readiness before expiry: a request that just became fully
-        # provisioned must not be killed by a concurrent expiry sweep landing
-        # on the same tick.
-        if vms_ready?(request_id, redis)
-          redis.hset(ondemand_hash_key, 'status', 'ready')
-          redis.expire(ondemand_hash_key, default_expiration)
-          redis.zrem(processing_key, request_id)
-          dereference_request_mutex(request_id)
-          next
-        end
+    def check_ondemand_request_ready_locked(request_id, redis, score = nil)
+      # default expiration is one month to ensure the data does not stay in redis forever
+      default_expiration = 259_200_0
+      processing_key = 'vmpooler__provisioning__processing'
+      ondemand_hash_key = "vmpooler__odrequest__#{request_id}"
 
-        score ||= redis.zscore(processing_key, request_id)
-        request_expired?(request_id, score, redis)
+      # Check readiness before expiry: a request that just became fully
+      # provisioned must not be killed by a concurrent expiry sweep landing
+      # on the same tick.
+      if vms_ready?(request_id, redis)
+        redis.hset(ondemand_hash_key, 'status', 'ready')
+        redis.expire(ondemand_hash_key, default_expiration)
+        redis.zrem(processing_key, request_id)
+        dereference_request_mutex(request_id)
+        return
       end
+
+      score ||= redis.zscore(processing_key, request_id)
+      request_expired?(request_id, score, redis)
     end
 
     def request_expired?(request_id, score, redis)
diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index dd176092..4d7247cb 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -541,7 +541,7 @@
     context 'with request_id' do
       context 'with a pending request' do
         before(:each) do
-          allow(subject).to receive(:check_ondemand_request_ready)
+          allow(subject).to receive(:check_ondemand_request_ready_locked)
           config[:config]['ondemand_request_ttl'] = 20
         end
 
@@ -566,6 +566,33 @@
         end
       end
 
+      context 'when this is the last vm needed to fulfill the request' do
+        let(:platform_alias) { pool }
+        let(:platforms_string) { "#{platform_alias}:#{pool}:1" }
+
+        before(:each) do
+          config[:config]['ondemand_request_ttl'] = 20
+          redis_connection_pool.with do |redis|
+            create_ondemand_request_for_test(request_id, current_time.to_i, platforms_string, redis)
+            create_ondemand_processing(request_id, current_time.to_i, redis)
+            redis.hset("vmpooler__vm__#{vm}", 'pool_alias', pool)
+          end
+        end
+
+        it 'does not deadlock marking the request ready (regression: move_pending_vm_to_ready already holds request_mutex)' do
+          redis_connection_pool.with do |redis|
+            expect { subject.move_pending_vm_to_ready(vm, pool, redis, request_id) }.to_not raise_error
+          end
+        end
+
+        it 'marks the request as ready' do
+          redis_connection_pool.with do |redis|
+            subject.move_pending_vm_to_ready(vm, pool, redis, request_id)
+            expect(redis.hget("vmpooler__odrequest__#{request_id}", 'status')).to eq('ready')
+          end
+        end
+      end
+
       context 'when the request has been marked as failed' do
         before(:each) do
           redis_connection_pool.with do |redis|
@@ -617,7 +644,7 @@
         let(:score) { current_time.to_i }
         before(:each) do
           config[:config]['ondemand_request_ttl'] = 20
-          allow(subject).to receive(:check_ondemand_request_ready)
+          allow(subject).to receive(:check_ondemand_request_ready_locked)
           redis_connection_pool.with do |redis|
             create_ondemand_request_for_test(request_id, score, platforms_string, redis, user, token)
           end

From c7b087397238bd819e45c67b3d2275630dd9d745 Mon Sep 17 00:00:00 2001
From: Mahima Singh <mahima.singh@perforce.com>
Date: Wed, 9 Sep 2026 20:32:23 +0530
Subject: [PATCH 4/6] Add deterministic two-thread test proving request_mutex
 enforces mutual exclusion between move_pending_vm_to_ready and the expiry
 sweep

The previous regression test only proved the recursive-locking
deadlock was gone; it passed even with the request_mutex wrap
removed entirely from move_pending_vm_to_ready, because the earlier
reordering fix (check vms_ready? before request_expired?) alone was
enough to save that specific scenario once the vm was already added
to the fulfillment set - the lock's own contribution wasn't actually
being exercised.

This test instruments both call sites' shared critical section
(check_ondemand_request_ready_locked) directly and, using Queue-based
thread coordination (no sleep/timing-based coordination for the
deterministic parts):

- pauses move_pending_vm_to_ready mid-critical-section (still holding
  request_mutex) right after it has added the vm to the fulfillment
  set but before marking the request ready
- starts the sweep concurrently and proves it cannot enter its own
  critical section within a generous 0.3s window while the lock is
  held (Timeout-bounded check, since Queue#pop's timeout: keyword
  isn't available on the jruby target this runs under in CI)
- tracks concurrent entries into the shared critical section and
  asserts they never exceed 1
- asserts the final state is correct (request ready, vm running, not
  destroyed)

Verified this test fails cleanly and quickly (no hang) when the
request_mutex wrap is temporarily removed from
move_pending_vm_to_ready - the blocked/entered assertion reports
:entered instead of :blocked - and passes with the fix restored.
---
 spec/unit/pool_manager_spec.rb | 100 +++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 4d7247cb..7a4c6c6a 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -1,5 +1,6 @@
 require 'spec_helper'
 require 'time'
+require 'timeout'
 require 'mock_redis'
 
 # Custom RSpec :Matchers
@@ -593,6 +594,105 @@
         end
       end
 
+      context 'when the expiry sweep runs concurrently with the last vm completing the request' do
+        # Deterministic two-thread test: proves request_mutex actually
+        # provides mutual exclusion between move_pending_vm_to_ready and the
+        # expiry sweep (check_ondemand_request_ready), rather than merely
+        # proving the recursive-lock deadlock is gone. Uses a single shared
+        # MockRedis instance directly (not redis_connection_pool, which is
+        # sized to 1 connection here and would otherwise block on pool
+        # checkout rather than on the mutex we're testing).
+        let(:platform_alias) { pool }
+        let(:platforms_string) { "#{platform_alias}:#{pool}:1" }
+        # older than the ttl configured below, so the sweep considers the
+        # request expired the instant it can inspect it
+        let(:expired_score) { (Time.now - 960).to_i }
+        let(:shared_redis) { MockRedis.new }
+
+        before(:each) do
+          config[:config]['ondemand_request_ttl'] = 5
+          create_ondemand_request_for_test(request_id, expired_score, platforms_string, shared_redis)
+          create_ondemand_processing(request_id, expired_score, shared_redis)
+          shared_redis.hset("vmpooler__vm__#{vm}", 'pool_alias', pool)
+          shared_redis.hset("vmpooler__vm__#{vm}", 'clone', Time.now.to_s)
+          shared_redis.sadd("vmpooler__pending__#{pool}", vm)
+        end
+
+        it 'blocks the sweep from entering its critical section until move_pending_vm_to_ready releases it, and never runs both concurrently' do
+          reached_move_pause = Queue.new
+          release_move_pending = Queue.new
+          entered_locked_check = Queue.new
+          bookkeeping_mutex = Mutex.new
+          active_count = 0
+          max_active = 0
+
+          # Pause move_pending_vm_to_ready only at its 'pending' -> 'running'
+          # transition (unique to this call site) while it still holds
+          # request_mutex, so we can deterministically run the sweep
+          # concurrently and prove it can't make progress until this releases.
+          allow(subject).to receive(:move_vm_queue).and_wrap_original do |original, *args|
+            if args[2] == 'pending' && args[3] == 'running'
+              reached_move_pause << true
+              release_move_pending.pop
+            end
+            original.call(*args)
+          end
+
+          # Instrument the actual critical section both callers eventually
+          # reach, so we can directly observe: (a) whether the sweep manages
+          # to enter it while move_pending_vm_to_ready is paused holding the
+          # lock, and (b) whether the two ever overlap.
+          original_locked_check = subject.method(:check_ondemand_request_ready_locked)
+          allow(subject).to receive(:check_ondemand_request_ready_locked) do |*args|
+            entered_locked_check << true
+            bookkeeping_mutex.synchronize do
+              active_count += 1
+              max_active = active_count if active_count > max_active
+            end
+            begin
+              original_locked_check.call(*args)
+            ensure
+              bookkeeping_mutex.synchronize { active_count -= 1 }
+            end
+          end
+
+          Timeout.timeout(5) do
+            move_thread = Thread.new do
+              subject.move_pending_vm_to_ready(vm, pool, shared_redis, request_id)
+            end
+
+            reached_move_pause.pop
+
+            sweep_thread = Thread.new do
+              subject.check_ondemand_request_ready(request_id, shared_redis, expired_score)
+            end
+
+            # While move_pending_vm_to_ready still holds request_mutex, the
+            # sweep must not be able to enter its own critical section yet -
+            # give it a generous 0.3s window (far longer than an in-memory
+            # mock-redis call sequence needs) to prove it's genuinely blocked,
+            # not just slow.
+            sweep_progress = Thread.new do
+              Timeout.timeout(0.3) { entered_locked_check.pop }
+              :entered
+            rescue Timeout::Error
+              :blocked
+            end
+            expect(sweep_progress.value).to eq(:blocked)
+
+            release_move_pending << true
+
+            move_thread.join
+            sweep_thread.join
+          end
+
+          expect(max_active).to eq(1)
+          expect(shared_redis.hget("vmpooler__odrequest__#{request_id}", 'status')).to eq('ready')
+          expect(shared_redis.sismember("vmpooler__completed__#{pool}", vm)).to be false
+          expect(shared_redis.sismember("vmpooler__running__#{pool}", vm)).to be true
+        end
+      end
+
       context 'when the request has been marked as failed' do
         before(:each) do
           redis_connection_pool.with do |redis|

From 8a20088b829fd0c3925aa57a98530758eca2bdb0 Mon Sep 17 00:00:00 2001
From: Mahima Singh <mahima.singh@perforce.com>
Date: Thu, 10 Sep 2026 12:20:49 +0530
Subject: [PATCH 5/6] Fix unsafe request_mutex cleanup flagged by Copilot
 review

request_mutex used the same compute_if_absent + delete pattern as
vm_mutex/pool_mutex, but deleting a request_id's entry once it goes
terminal (ready/failed) is unsafe here in a way it inherently isn't
for a bug-free vm_mutex/pool_mutex: a caller that already fetched the
old Mutex object (e.g. blocked on it, or about to synchronize) is
unaffected by the map entry being deleted, but a *later* caller for
the same request_id, finding no entry, gets handed a brand new Mutex
- letting both callers enter the request's critical section
concurrently. This directly reintroduces the class of race this PR
exists to fix, and is realistic here: a multi-VM request's sibling
VM completion threads and the expiry sweep are exactly the
concurrent callers this lock is meant to serialize.

Replaced the growable per-request_id map with a fixed-size array of
256 pre-allocated mutexes (request_id hashed to a stripe). Nothing is
ever created or destroyed per-request, so there's no map entry to go
stale and no deletion race is possible; unrelated request_ids
occasionally sharing a stripe just adds incidental contention, not a
correctness issue. Removed dereference_request_mutex entirely along
with both call sites.

Verified: full suite (455 examples) still passes, rubocop clean, and
the two-thread concurrency test still fails cleanly (:entered instead
of :blocked, no hang) when the request_mutex wrap is temporarily
removed from move_pending_vm_to_ready under this new implementation,
confirming it still actually enforces mutual exclusion.
---
 lib/vmpooler/pool_manager.rb | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index fc1bbc30..07ec86a9 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -37,7 +37,7 @@ def initialize(config, logger, redis_connection_pool, metrics)
       @reconfigure_pool = Concurrent::Map.new
 
       @vm_mutex = Concurrent::Map.new
-      @request_mutex = Concurrent::Map.new
+      @request_mutex_stripes = Array.new(REQUEST_MUTEX_STRIPE_COUNT) { Mutex.new }
 
       # Name generator for generating host names
       @name_generator = Spicy::Proton.new
@@ -1788,12 +1788,22 @@ def vm_mutex(vmname)
       @vm_mutex.compute_if_absent(vmname) { Mutex.new }
     end
 
-    def request_mutex(request_id)
-      @request_mutex.compute_if_absent(request_id) { Mutex.new }
-    end
+    # Striped rather than per-request_id: request_ids are one-shot and never
+    # revisited once terminal (ready/failed/deleted), so a growable
+    # compute_if_absent+delete map (like pool_mutex/vm_mutex) would leak
+    # forever if never deleted, or - if deleted - risks a caller that already
+    # fetched the old Mutex still being blocked/about to synchronize on it
+    # while a later caller, finding the entry gone, gets handed a brand new
+    # Mutex for the same request_id. That would let both callers enter the
+    # request's critical section concurrently, defeating the whole point of
+    # this lock (see puppetlabs/vmpooler#702 review discussion). A fixed-size
+    # array of pre-allocated mutexes needs no creation or deletion at all, so
+    # this can't happen; unrelated request_ids occasionally sharing a stripe
+    # just means incidental extra contention, not a correctness problem.
+    REQUEST_MUTEX_STRIPE_COUNT = 256
 
-    def dereference_request_mutex(request_id)
-      true if @request_mutex.delete(request_id)
+    def request_mutex(request_id)
+      @request_mutex_stripes[request_id.hash.abs % REQUEST_MUTEX_STRIPE_COUNT]
     end
 
     def dereference_mutex(vmname)
@@ -2422,7 +2432,6 @@ def check_ondemand_request_ready_locked(request_id, redis, score = nil)
         redis.hset(ondemand_hash_key, 'status', 'ready')
         redis.expire(ondemand_hash_key, default_expiration)
         redis.zrem(processing_key, request_id)
-        dereference_request_mutex(request_id)
         return
       end
 
@@ -2448,7 +2457,6 @@ def request_expired?(request_id, score, redis)
         pipeline.expire("vmpooler__odrequest__#{request_id}", expiration_ttl)
       end
       remove_vms_for_failed_request(request_id, expiration_ttl, redis)
-      dereference_request_mutex(request_id)
       true
     end
 

From f85ba682e913760e2e2c7512fbe80310b2be99dc Mon Sep 17 00:00:00 2001
From: Mahima Singh <mahima.singh@perforce.com>
Date: Thu, 10 Sep 2026 12:39:37 +0530
Subject: [PATCH 6/6] Address self-review findings: terminal-status guard, lock
 docs, nits

Reviewing my own diff with fresh eyes surfaced several things worth
fixing before this goes further:

- Guard against resurrecting a terminal request.
  check_ondemand_request_ready_locked's ready branch unconditionally
  wrote status='ready'. The ondemand delete endpoint sets 'deleted'
  *without* removing the request from the processing set, so the sweep
  keeps visiting it. It isn't currently exploitable (the delete also
  dels the fulfillment sets, so vms_ready? is false), but that's
  load-bearing coincidence and reordering readiness ahead of expiry
  made the branch more reachable. Now bails out early on
  failed/deleted and zrems the request so it stops being revisited -
  which also preserves the old cleanup behaviour, since previously
  such requests were only dropped from the set by being rewritten to
  'failed'.

- Document the lock ordering. _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. There's no reverse path today,
  but remove_vms_for_failed_request destroys VMs without their
  vm_mutex and "add vm_mutex there" is a plausible future change that
  would deadlock. Recorded the constraint.

- Document the scope limitation. request_mutex is an in-process Mutex:
  it does not coordinate with the api process (which mutates request
  status and VM queues in the delete endpoint) and provides no
  protection if the manager runs more than one replica.

- Warn on check_ondemand_request_ready_locked itself that it requires
  the lock; previously that note only lived on the other method.

- Move REQUEST_MUTEX_STRIPE_COUNT up to the class's constant block.

- Note in the concurrency test that its 0.3s window is deliberately
  one-sided (can under-report a regression, can never invent one), and
  that max_active only covers the _locked call while the :blocked
  assertion covers the rest of the critical section.

Verified the six new terminal-status tests all fail with the guard
removed. Full suite: 461 examples, 0 failures; rubocop clean.
---
 lib/vmpooler/pool_manager.rb   | 34 +++++++++++++++++++++++++--
 spec/unit/pool_manager_spec.rb | 43 ++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index 07ec86a9..ed2fe704 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -12,6 +12,11 @@ class PoolManager
     CHECK_LOOP_DELAY_MAX_DEFAULT = 60
     CHECK_LOOP_DELAY_DECAY_DEFAULT = 2.0
 
+    # Number of pre-allocated mutexes that ondemand request_ids are hashed
+    # across. See #request_mutex for why this is striped rather than a
+    # per-request_id map.
+    REQUEST_MUTEX_STRIPE_COUNT = 256
+
     def initialize(config, logger, redis_connection_pool, metrics)
       $config = config
 
@@ -1800,8 +1805,20 @@ def vm_mutex(vmname)
     # array of pre-allocated mutexes needs no creation or deletion at all, so
     # this can't happen; unrelated request_ids occasionally sharing a stripe
     # just means incidental extra contention, not a correctness problem.
-    REQUEST_MUTEX_STRIPE_COUNT = 256
-
+    #
+    # LOCK ORDERING: _check_pending_vm holds vm_mutex(vm) while calling
+    # move_pending_vm_to_ready, which acquires request_mutex(request_id) - so
+    # the established order is vm_mutex -> request_mutex. Anything acquiring
+    # request_mutex first (e.g. the ondemand_provisioner sweep) must not then
+    # acquire vm_mutex, or the two will deadlock. Note in particular that
+    # remove_vms_for_failed_request currently destroys VMs without taking
+    # their vm_mutex; adding that locking would need this order respected.
+    #
+    # SCOPE: this is an in-process Mutex, so it only serializes threads within
+    # a single manager process. It does not coordinate with the api process
+    # (which mutates request status and VM queues in the ondemand delete
+    # endpoint), and it provides no protection at all if the manager is run
+    # with more than one replica.
     def request_mutex(request_id)
       @request_mutex_stripes[request_id.hash.abs % REQUEST_MUTEX_STRIPE_COUNT]
     end
@@ -2419,12 +2436,25 @@ def check_ondemand_request_ready(request_id, redis, score = nil)
       end
     end
 
+    # MUST be called with request_mutex(request_id) already held - it does no
+    # locking of its own. Use check_ondemand_request_ready if you do not
+    # already hold the lock.
     def check_ondemand_request_ready_locked(request_id, redis, score = nil)
       # default expiration is one month to ensure the data does not stay in redis forever
       default_expiration = 259_200_0
       processing_key = 'vmpooler__provisioning__processing'
       ondemand_hash_key = "vmpooler__odrequest__#{request_id}"
 
+      # Never resurrect a request that already reached a terminal state. The
+      # ondemand delete endpoint sets 'deleted' (from the api process, which
+      # this lock cannot coordinate with) without removing the request from
+      # the processing set, so the sweep keeps visiting it; drop it here so we
+      # neither overwrite its status nor revisit it forever.
+      if %w[failed deleted].include?(redis.hget(ondemand_hash_key, 'status'))
+        redis.zrem(processing_key, request_id)
+        return
+      end
+
       # Check readiness before expiry: a request that just became fully
       # provisioned must not be killed by a concurrent expiry sweep landing
       # on the same tick.
diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 7a4c6c6a..eb897a65 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -672,6 +672,12 @@
             # give it a generous 0.3s window (far longer than an in-memory
             # mock-redis call sequence needs) to prove it's genuinely blocked,
             # not just slow.
+            #
+            # Note the flake direction is deliberately one-sided: a working
+            # lock can never spuriously fail this (the sweep is blocked
+            # indefinitely, not merely for longer than 0.3s), whereas a broken
+            # lock on a pathologically slow runner could occasionally escape
+            # detection. It can under-report a regression, never invent one.
             sweep_progress = Thread.new do
               Timeout.timeout(0.3) { entered_locked_check.pop }
               :entered
@@ -686,6 +692,10 @@
             sweep_thread.join
           end
 
+          # Covers only overlap of the check_ondemand_request_ready_locked
+          # call itself; the :blocked assertion above is what proves the sweep
+          # can't overlap the earlier part of move_pending_vm_to_ready's
+          # critical section (the status read and fulfillment-set write).
           expect(max_active).to eq(1)
           expect(shared_redis.hget("vmpooler__odrequest__#{request_id}", 'status')).to eq('ready')
           expect(shared_redis.sismember("vmpooler__completed__#{pool}", vm)).to be false
@@ -5605,6 +5615,39 @@
       end
     end
 
+    %w[failed deleted].each do |terminal_status|
+      context "when the request has already been marked as '#{terminal_status}'" do
+        before(:each) do
+          redis_connection_pool.with do |redis|
+            create_ondemand_processing(request_id, current_time.to_i, redis)
+            set_ondemand_request_status(request_id, terminal_status, redis)
+          end
+        end
+
+        it 'does not overwrite the status' do
+          redis_connection_pool.with do |redis|
+            subject.check_ondemand_request_ready(request_id, redis)
+            expect(redis.hget("vmpooler__odrequest__#{request_id}", 'status')).to eq(terminal_status)
+          end
+        end
+
+        it 'stops revisiting it by removing it from the processing set' do
+          redis_connection_pool.with do |redis|
+            subject.check_ondemand_request_ready(request_id, redis)
+            expect(redis.zscore('vmpooler__provisioning__processing', request_id)).to be nil
+          end
+        end
+
+        it 'does not evaluate readiness or expiry' do
+          redis_connection_pool.with do |redis|
+            expect(subject).to_not receive(:vms_ready?)
+            expect(subject).to_not receive(:request_expired?)
+            subject.check_ondemand_request_ready(request_id, redis)
+          end
+        end
+      end
+    end
+
     context 'when the request is not ready and the score has already been removed from processing' do
       it 'does not treat a missing score as an infinitely old request' do
         redis_connection_pool.with do |redis|