Merge branch 'master' into double_booking_03

Signed-off-by: Diego Tavares <dtavares@imageworks.com>

Author: DiegoTavares

VERSION.in

@@ -1 +1 @@
-1.26
+1.27

cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupport.java

@@ -93,6 +93,14 @@ public interface DispatchSupport {
      */
     static final AtomicLong clearedProcs = new AtomicLong(0);
 
+    /**
+     * A lost proc whose release was deferred because the kill-before-release could not confirm the
+     * frame was stopped and the host was not confirmed dead (likely a flapping host). The proc and
+     * its RUNNING frame are left intact to avoid double-booking until the host is confirmed DOWN or
+     * the frame completes naturally.
+     */
+    static final AtomicLong deferredReleaseProcs = new AtomicLong(0);
+
     /**
      * Long for counting dispatch errors
      */
@@ -180,8 +188,10 @@ public interface DispatchSupport {
      * @param proc
      * @param reason
      * @param exitStatus
+     * @return true if the proc was actually released (unbooked and its frame reset); false if the
+     *         release was deferred to avoid double-booking a possibly-still-rendering host
      */
-    void lostProc(VirtualProc proc, String reason, int exitStatus);
+    boolean lostProc(VirtualProc proc, String reason, int exitStatus);
 
     /**
      * Unbooks a proc with no message

cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupportService.java

@@ -519,29 +519,65 @@ public void unbookProc(VirtualProc proc, String reason) {
 
     @Override
     @Transactional(propagation = Propagation.NOT_SUPPORTED)
-    public void lostProc(VirtualProc proc, String reason, int exitStatus) {
-        long numCleared = clearedProcs.incrementAndGet();
-
+    public boolean lostProc(VirtualProc proc, String reason, int exitStatus) {
         /*
          * Kill the frame on RQD before releasing it back to a bookable state. Without this, a host
          * that is merely flapping (transient unreachable/DOWN report, GC pause, dropped report)
          * keeps rendering the frame while the dispatcher resets it to WAITING and re-books it
-         * elsewhere, silently double-booking the frame onto two hosts. Killing first means a
-         * reachable host stops the frame before it becomes re-bookable; a genuinely dead host times
-         * out (caught below), and release is then safe because no live RQD remains.
-         * EXIT_STATUS_FAILED_KILL is skipped because the caller (JobManagerSupport.kill) already
-         * attempted this exact kill and it threw.
+         * elsewhere, silently double-booking the frame onto two hosts.
          */
-        if (proc.frameId != null && exitStatus != Dispatcher.EXIT_STATUS_FAILED_KILL
-                && env.getProperty("dispatcher.kill_running_frame_before_release_enabled",
-                        Boolean.class, true)) {
+        boolean killBeforeReleaseEnabled = env.getProperty(
+                "dispatcher.kill_running_frame_before_release_enabled", Boolean.class, true);
+
+        /*
+         * Whether the running frame is known to be stopped on RQD after this point.
+         * EXIT_STATUS_FAILED_KILL means the caller (JobManagerSupport.kill) already attempted this
+         * exact kill and it threw, so the frame is NOT known-stopped and we must not re-kill.
+         */
+        boolean frameKilled;
+        if (exitStatus == Dispatcher.EXIT_STATUS_FAILED_KILL) {
+            frameKilled = false;
+        } else if (proc.frameId != null && killBeforeReleaseEnabled) {
             try {
                 rqdClient.killFrame(proc, "kill-before-release: " + reason);
+                frameKilled = true;
             } catch (Exception e) {
                 logger.info("kill-before-release failed for " + proc.getName() + ", " + e);
+                frameKilled = false;
             }
+        } else {
+            // Feature disabled or no frame to kill: preserve legacy release behavior.
+            frameKilled = true;
         }
 
+        /*
+         * Flapping and genuinely-dead hosts are indistinguishable at kill time. If the kill could
+         * not confirm the frame is stopped and the host is not confirmed dead, releasing the frame
+         * now would re-book it onto a second host while this RQD keeps rendering. In that case we
+         * DEFER the release: the proc row and RUNNING frame are left intact (preserving the
+         * host<->frame link, which is otherwise lost once the proc is deleted) so the frame is
+         * reclaimed later, once the host is confirmed DOWN (clearDownProcs) or the frame completes
+         * naturally. A genuinely dead host (marked DOWN, or no longer Up) leaves no live RQD, so
+         * release is safe. See design/frame_double_booking_v2.md.
+         */
+        boolean deferReleaseEnabled = env.getProperty(
+                "dispatcher.defer_release_on_failed_kill_enabled", Boolean.class, true);
+        if (deferReleaseEnabled && !frameKilled && proc.frameId != null) {
+            boolean hostConfirmedDead =
+                    exitStatus == Dispatcher.EXIT_STATUS_DOWN_HOST || !hostDao.isHostUp(proc);
+            if (!hostConfirmedDead) {
+                DispatchSupport.deferredReleaseProcs.incrementAndGet();
+                logger.warn("Deferring release of lost proc " + proc.getName()
+                        + ": kill-before-release could not confirm the frame stopped and the host "
+                        + "is not confirmed dead. Leaving frame " + proc.frameId
+                        + " RUNNING to avoid double-booking. reason=" + reason);
+                return false;
+            }
+        }
+
+        // Count the clear only now that the proc is actually being released; deferrals above
+        // return early and must not inflate this counter.
+        long numCleared = clearedProcs.incrementAndGet();
         unbookProc(proc, "proc " + proc.getName() + " is #" + numCleared + " cleared: " + reason);
 
         if (proc.frameId != null) {
@@ -575,6 +611,7 @@ public void lostProc(VirtualProc proc, String reason, int exitStatus) {
         } else {
             logger.info("Frame ID is NULL, not updating Frame state");
         }
+        return true;
     }
 
     @Override

cuebot/src/main/java/com/imageworks/spcue/servant/CueStatic.java

@@ -69,6 +69,7 @@ public void getSystemStats(CueGetSystemStatsRequest request,
                 .setKilledOffenderProcs(DispatchSupport.killedOffenderProcs.get())
                 .setKilledOomProcs(DispatchSupport.killedOomProcs.get())
                 .setClearedProcs(DispatchSupport.clearedProcs.get())
+                .setDeferredReleaseProcs(DispatchSupport.deferredReleaseProcs.get())
                 .setBookingRetries(DispatchSupport.bookingRetries.get())
                 .setBookingErrors(DispatchSupport.bookingErrors.get())
                 .setBookedProcs(DispatchSupport.bookedProcs.get())

cuebot/src/main/java/com/imageworks/spcue/service/MaintenanceManagerSupport.java

@@ -158,15 +158,17 @@ private void clearOrphanedProcs() {
         List<VirtualProc> procs = procDao.findOrphanedVirtualProcs(100);
         for (VirtualProc proc : procs) {
             try {
-                dispatchSupport.lostProc(proc, "Removed by maintenance, orphaned",
-                        Dispatcher.EXIT_STATUS_FRAME_ORPHAN);
-
-                Sentry.configureScope(scope -> {
-                    scope.setExtra("frame_id", proc.getFrameId());
-                    scope.setExtra("host_id", proc.getHostId());
-                    scope.setExtra("name", proc.getName());
-                    Sentry.captureMessage("Manager cleaning orphan procs");
-                });
+                // Only report a cleanup when the proc was actually released; lostProc may defer the
+                // release (leaving the proc intact) to avoid double-booking a flapping host.
+                if (dispatchSupport.lostProc(proc, "Removed by maintenance, orphaned",
+                        Dispatcher.EXIT_STATUS_FRAME_ORPHAN)) {
+                    Sentry.withScope(scope -> {
+                        scope.setExtra("frame_id", proc.getFrameId());
+                        scope.setExtra("host_id", proc.getHostId());
+                        scope.setExtra("name", proc.getName());
+                        Sentry.captureMessage("Manager cleaning orphan procs");
+                    });
+                }
             } catch (Exception e) {
                 logger.info("failed to clear orphaned proc: " + proc.getName() + " " + e);
             }

cuebot/src/main/resources/opencue.properties

@@ -151,6 +151,16 @@ dispatcher.frame_kill_retry_limit=3
 # host simply times out and release proceeds. Default = true.
 dispatcher.kill_running_frame_before_release_enabled=true
 
+# Companion to the setting above. Flapping and genuinely-dead hosts are indistinguishable at kill
+# time: when the kill-before-release cannot confirm the frame stopped (the RQD kill threw, or the
+# kill already failed upstream) and the host is not confirmed dead (not marked DOWN and still Up),
+# releasing the frame would re-book it onto a second host while the original RQD keeps rendering.
+# When true, the release is deferred in that case: the proc and its RUNNING frame are left intact
+# (preserving the host<->frame link) and reclaimed later once the host is confirmed DOWN or the
+# frame completes naturally. Set to false to restore the prior always-release behavior. Default =
+# true.
+dispatcher.defer_release_on_failed_kill_enabled=true
+
 # When a reported running frame's resource_id maps to a proc that now owns a different frame, the
 # frame is demonstrably owned elsewhere: kill it immediately instead of waiting for the grace and
 # isOrphan windows. Set to false to disable this immediate kill; such mismatched frames are then

cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/DispatchSupportServiceLostProcTests.java

@@ -21,9 +21,12 @@
 import org.springframework.core.env.Environment;
 import org.springframework.test.util.ReflectionTestUtils;
 
+import com.imageworks.spcue.FrameDetail;
 import com.imageworks.spcue.FrameInterface;
+import com.imageworks.spcue.HostInterface;
 import com.imageworks.spcue.VirtualProc;
 import com.imageworks.spcue.dao.FrameDao;
+import com.imageworks.spcue.dao.HostDao;
 import com.imageworks.spcue.dao.JobDao;
 import com.imageworks.spcue.dao.LayerDao;
 import com.imageworks.spcue.dao.ProcDao;
@@ -56,11 +59,14 @@ public class DispatchSupportServiceLostProcTests {
 
     private static final String KILL_BEFORE_RELEASE_PROPERTY =
             "dispatcher.kill_running_frame_before_release_enabled";
+    private static final String DEFER_RELEASE_PROPERTY =
+            "dispatcher.defer_release_on_failed_kill_enabled";
 
     private DispatchSupportService dispatchSupport;
     private RqdClient rqdClient;
     private ProcDao procDao;
     private FrameDao frameDao;
+    private HostDao hostDao;
     private Environment env;
     private VirtualProc proc;
 
@@ -70,19 +76,26 @@ public void setup() {
         rqdClient = mock(RqdClient.class);
         procDao = mock(ProcDao.class);
         frameDao = mock(FrameDao.class);
+        hostDao = mock(HostDao.class);
         env = mock(Environment.class);
 
         dispatchSupport.setRqdClient(rqdClient);
         dispatchSupport.setProcDao(procDao);
         dispatchSupport.setFrameDao(frameDao);
+        dispatchSupport.setHostDao(hostDao);
         dispatchSupport.setShowDao(mock(ShowDao.class));
         dispatchSupport.setJobDao(mock(JobDao.class));
         dispatchSupport.setLayerDao(mock(LayerDao.class));
         ReflectionTestUtils.setField(dispatchSupport, "env", env);
 
-        // Kill-before-release enabled by default.
+        // Kill-before-release and defer-release both enabled by default.
         when(env.getProperty(eq(KILL_BEFORE_RELEASE_PROPERTY), eq(Boolean.class), eq(true)))
                 .thenReturn(true);
+        when(env.getProperty(eq(DEFER_RELEASE_PROPERTY), eq(Boolean.class), eq(true)))
+                .thenReturn(true);
+
+        // Default: the host still looks Up in the DB (the dangerous flapping case).
+        when(hostDao.isHostUp(any(HostInterface.class))).thenReturn(true);
 
         proc = new VirtualProc();
         proc.id = "00000000-0000-0000-0000-000000000001";
@@ -102,27 +115,111 @@ public void killsRqdBeforeReleasingProc() {
         InOrder inOrder = inOrder(rqdClient, procDao);
         inOrder.verify(rqdClient, times(1)).killFrame(eq(proc), anyString());
         inOrder.verify(procDao, times(1)).deleteVirtualProc(proc);
+        verify(frameDao, times(1)).updateFrameStopped(any(FrameInterface.class),
+                eq(FrameState.WAITING), anyInt());
     }
 
     @Test
     public void skipsRqdKillForFailedKillExitStatus() {
         // The failed-kill path already attempted this exact kill, so lostProc must not re-issue it.
+        // Since that kill demonstrably failed and the host still looks Up, the release is deferred
+        // to avoid double-booking a possibly-still-rendering host.
+        dispatchSupport.lostProc(proc, "failed kill", Dispatcher.EXIT_STATUS_FAILED_KILL);
+
+        verify(rqdClient, never()).killFrame(any(VirtualProc.class), anyString());
+        verify(procDao, never()).deleteVirtualProc(any(VirtualProc.class));
+        verify(frameDao, never()).updateFrameStopped(any(FrameInterface.class),
+                any(FrameState.class), anyInt());
+    }
+
+    @Test
+    public void releasesForFailedKillWhenHostConfirmedDownByDb() {
+        // FAILED_KILL but the host is no longer Up in the DB: no live RQD remains, release
+        // proceeds.
+        when(hostDao.isHostUp(any(HostInterface.class))).thenReturn(false);
+
         dispatchSupport.lostProc(proc, "failed kill", Dispatcher.EXIT_STATUS_FAILED_KILL);
 
         verify(rqdClient, never()).killFrame(any(VirtualProc.class), anyString());
         verify(procDao, times(1)).deleteVirtualProc(proc);
+        verify(frameDao, times(1)).updateFrameStopped(any(FrameInterface.class),
+                eq(FrameState.WAITING), anyInt());
     }
 
     @Test
-    public void releasesProcWhenRqdKillThrows() {
-        // A genuinely dead/unreachable host makes the kill throw; release must still proceed.
+    public void releasesProcWhenRqdKillThrowsForDownHost() {
+        // A host reported DOWN is confirmed dead: the kill may throw, but release must still
+        // proceed.
         doThrow(new RuntimeException("host unreachable")).when(rqdClient)
                 .killFrame(any(VirtualProc.class), anyString());
 
         dispatchSupport.lostProc(proc, "down host", Dispatcher.EXIT_STATUS_DOWN_HOST);
 
         verify(rqdClient, times(1)).killFrame(eq(proc), anyString());
         verify(procDao, times(1)).deleteVirtualProc(proc);
+        verify(frameDao, times(1)).updateFrameStopped(any(FrameInterface.class),
+                eq(FrameState.WAITING), anyInt());
+    }
+
+    @Test
+    public void updatesFrameHostDownWhenFrameNotRunningForDownHost() {
+        // Down host whose frame is no longer RUNNING but already DEAD: updateFrameStopped reports
+        // nothing to stop, so the down-host fallback must reset it via updateFrameHostDown.
+        when(frameDao.updateFrameStopped(any(FrameInterface.class), any(FrameState.class),
+                anyInt())).thenReturn(false);
+        FrameDetail deadFrame = new FrameDetail();
+        deadFrame.state = FrameState.DEAD;
+        when(frameDao.getFrameDetail(any(FrameInterface.class))).thenReturn(deadFrame);
+
+        dispatchSupport.lostProc(proc, "down host", Dispatcher.EXIT_STATUS_DOWN_HOST);
+
+        verify(procDao, times(1)).deleteVirtualProc(proc);
+        verify(frameDao, times(1)).updateFrameHostDown(any(FrameInterface.class));
+    }
+
+    @Test
+    public void defersReleaseWhenKillThrowsAndHostStillUp() {
+        // The flapping-host case: the kill throws and the host still looks Up. Releasing now would
+        // re-book the frame onto a second host while this RQD keeps rendering, so we must defer.
+        doThrow(new RuntimeException("transient unreachable")).when(rqdClient)
+                .killFrame(any(VirtualProc.class), anyString());
+
+        dispatchSupport.lostProc(proc, "orphaned", Dispatcher.EXIT_STATUS_FRAME_ORPHAN);
+
+        verify(rqdClient, times(1)).killFrame(eq(proc), anyString());
+        verify(procDao, never()).deleteVirtualProc(any(VirtualProc.class));
+        verify(frameDao, never()).updateFrameStopped(any(FrameInterface.class),
+                any(FrameState.class), anyInt());
+    }
+
+    @Test
+    public void releasesWhenKillThrowsAndHostNotUp() {
+        // The kill throws but the host is no longer Up: confirmed dead, so release is safe.
+        doThrow(new RuntimeException("host unreachable")).when(rqdClient)
+                .killFrame(any(VirtualProc.class), anyString());
+        when(hostDao.isHostUp(any(HostInterface.class))).thenReturn(false);
+
+        dispatchSupport.lostProc(proc, "orphaned", Dispatcher.EXIT_STATUS_FRAME_ORPHAN);
+
+        verify(rqdClient, times(1)).killFrame(eq(proc), anyString());
+        verify(procDao, times(1)).deleteVirtualProc(proc);
+        verify(frameDao, times(1)).updateFrameStopped(any(FrameInterface.class),
+                eq(FrameState.WAITING), anyInt());
+    }
+
+    @Test
+    public void releasesWhenDeferDisabledByPropertyEvenIfKillThrows() {
+        // With defer disabled, behavior falls back to the prior always-release semantics.
+        when(env.getProperty(eq(DEFER_RELEASE_PROPERTY), eq(Boolean.class), eq(true)))
+                .thenReturn(false);
+        doThrow(new RuntimeException("transient unreachable")).when(rqdClient)
+                .killFrame(any(VirtualProc.class), anyString());
+
+        dispatchSupport.lostProc(proc, "orphaned", Dispatcher.EXIT_STATUS_FRAME_ORPHAN);
+
+        verify(procDao, times(1)).deleteVirtualProc(proc);
+        verify(frameDao, times(1)).updateFrameStopped(any(FrameInterface.class),
+                eq(FrameState.WAITING), anyInt());
     }
 
     @Test
@@ -134,5 +231,7 @@ public void skipsRqdKillWhenDisabledByProperty() {
 
         verify(rqdClient, never()).killFrame(any(VirtualProc.class), anyString());
         verify(procDao, times(1)).deleteVirtualProc(proc);
+        verify(frameDao, times(1)).updateFrameStopped(any(FrameInterface.class),
+                eq(FrameState.WAITING), anyInt());
     }
 }

cuebot/src/test/resources/opencue.properties

@@ -66,6 +66,9 @@ dispatcher.frame_kill_retry_limit=3
 
 # Kill the frame on RQD before releasing a lost/down/orphaned proc back to a bookable state.
 dispatcher.kill_running_frame_before_release_enabled=true
+# Defer releasing a lost proc when the kill could not confirm the frame stopped and the host is not
+# confirmed dead, to avoid double-booking a flapping host's frame.
+dispatcher.defer_release_on_failed_kill_enabled=true
 # Immediately kill a reported frame whose resource_id maps to a proc on a different frame.
 dispatcher.frame_verification_strict_fencing_enabled=true
 # Grace seconds shielding a reported frame whose resource_id maps to no proc (propagation window).

cueweb/.env.example

@@ -75,7 +75,7 @@ NEXTAUTH_SECRET=canbeanything
 #   CUEWEB_AUTHZ_ENABLED   master switch; opt-in. Set to true to enable the
 #                          gate. Default: disabled (pure pass-through).
 #   CUEWEB_ALLOWED_GROUPS  groups allowed to use CueWeb at all
-#   CUEWEB_ADMIN_GROUPS    groups allowed to use the CueCommander admin pages
+#   CUEWEB_ADMIN_GROUPS    groups allowed on the whole CueCommander section, CueSubmit and Manage facilities
 #   CUEWEB_GROUPS_CLAIM    JWT/OIDC claim that holds the user's groups (default: groups)
 CUEWEB_AUTHZ_ENABLED=false
 CUEWEB_ALLOWED_GROUPS=