[DiegoTavares]

RQD silently drops FrameCompleteReports

In rust/crates/rqd/src/system/machine.rs:

1. monitor_running_frames evicts the finished frame from the cache before the report is confirmed
   (line 405-420): the retain() closure returns false for FrameState::Finished, removing it, then
   calls handle_finished_frames.
2. handle_finished_frames is fire-and-forget (line 573-588): if send_frame_complete_report returns
   Err, it just error!(...) and moves on. No re-queue, no retry on the next monitor cycle, no
   persistence. The frame is already gone from the cache. The completion is lost forever.
3. The retry layer cannot save it (report/retry/backoff_policy.rs:74-103): it inspects the HTTP
   status and retries only on transport errors or HTTP 500/502/503. But a gRPC application error —
   which is exactly what Cuebot returns when handleFrameCompleteReport throws
   RqdRetryReportException — arrives as HTTP 200 with a grpc-status trailer. The policy sees HTTP
   200 → Outcome::Return → not retried; send_frame_complete_report then surfaces it as Err
   (report_client.rs:226-231) → dropped.

With this a completed frame under a failing Rqd (caused by memory pressure or cpu eviction for
example) could lead to a completed frame not having its state synchronized to Cuebot.

An orphaned frame (RUNNING with no proc, because its proc was removed
while RQD was still rendering) was reset straight to WAITING by the
maintenance reaper. The dispatcher could then rebook it onto a second
host while the original RQD process was still alive, a double booking.

Redesign the reaper to kill-then-confirm-dead before clearing:

• Add RqdClient.isFrameRunning(host, frameId), backed by RQD's
  GetRunningFrameStatus. A NOT_FOUND response means the render has been
  reaped (confirmed dead); any other failure is raised as an
  RqdClientException so callers can tell "confirmed gone" from
  "could not reach".

• clearOrphanedFrames() now kills each orphan on its last-known host and
  polls until death is confirmed. Only then is the frame reset to
  WAITING (safe auto-retry). If death cannot be confirmed, the per-pass
  kill budget is exhausted or the host is unreachable, the frame is
  failed (DEAD) so it needs a manual retry instead of risking a rebook.
  Fail closed: a stuck frame is acceptable, a silent double booking is
  not.

• The whole pass is bounded by maintenance.orphaned_frame_kill_budget_ms
  (default 30s) so a batch of unreachable hosts cannot stall maintenance.

• getOrphanedFrames() now returns List (mapped via the
  existing FRAME_DETAIL_MAPPER) so the reaper reads the last host from
  lastResource directly, dropping a second per-frame DB lookup.

Also document why FrameCompleteHandler's orphan-finalize path
intentionally skips the layer min-memory bump (it is proc-specific and
there is no proc on that path).

Tests: add MaintenanceManagerOrphanFrameTests (DB-free) covering the
confirmed-dead -> WAITING, host-unreachable -> DEAD, budget-exhausted ->
DEAD, and never-ran -> WAITING branches; extend
FrameDaoTests.testgetOrphanedFrames to assert lastResource is populated.

LLM Usage disclosure

Claude Opus was used to suggest the fix for this issue

Summary by CodeRabbit

• Bug Fixes

  • Improved recovery of orphaned frames by stopping lingering renders and resetting confirmed-stopped frames for retry.
  • Frames that cannot be confirmed stopped are deferred or marked for manual review.
  • Improved handling of missing, delayed, stale, and duplicate completion reports.
  • Preserved memory-failure retry behavior across lost reports and worker restarts.
  • Finished render reports are retried when delivery temporarily fails.

• Configuration

  • Added scheduled orphaned-frame cleanup with configurable processing limits and termination-confirmation timeouts.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The PR adds bounded orphan-frame recovery, RQD liveness checks, proc-independent completion handling, expanded completion-state tests, and retryable RQD completion delivery.

Changes

Cuebot orphaned-frame recovery

Layer / File(s)	Summary
Orphaned frame detail contract cuebot/src/main/java/com/imageworks/spcue/dao/..., cuebot/src/test/java/com/imageworks/spcue/test/dao/...	getOrphanedFrames(int limit) returns FrameDetail objects with lastResource and applies a result limit.
RQD running-frame status API cuebot/src/main/java/com/imageworks/spcue/rqd/*	RqdClient exposes isFrameRunning(). The gRPC client distinguishes NOT_FOUND from communication failures.
Orphaned-frame kill and confirmation cuebot/src/main/java/com/imageworks/spcue/service/..., cuebot/src/main/resources/..., cuebot/src/test/java/com/imageworks/spcue/test/service/...	Maintenance kills orphaned frames, confirms termination within configured budgets, resets confirmed frames to WAITING, defers active frames, and marks expired or unreachable frames DEAD.
Proc-absent frame completion cuebot/src/main/java/com/imageworks/spcue/dispatcher/..., cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/*	Completion handling supports missing proc records, stale reports, proc-independent bookkeeping, centralized state resolution, telemetry, retry behavior, and proc lifecycle decisions.

RQD retryable completion delivery

Layer / File(s)	Summary
Pending completion enqueue and flush rust/crates/rqd/src/system/machine.rs	MachineMonitor stores finished frames in pending_completions, releases reservations once, removes acknowledged entries, and retries failed completion reports.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MaintenanceManagerSupport
  participant FrameDao
  participant RqdClientGrpc
  participant FrameCompleteHandler
  MaintenanceManagerSupport->>FrameDao: getOrphanedFrames(limit)
  FrameDao-->>MaintenanceManagerSupport: FrameDetail with lastResource
  MaintenanceManagerSupport->>RqdClientGrpc: killFrame and isFrameRunning
  RqdClientGrpc-->>MaintenanceManagerSupport: termination result
  MaintenanceManagerSupport->>FrameCompleteHandler: process orphaned completion
  FrameCompleteHandler->>FrameCompleteHandler: validate and finalize frame

Loading

sequenceDiagram
  participant MachineMonitor
  participant pending_completions
  participant Cuebot
  MachineMonitor->>pending_completions: enqueue finished frame and release cores
  pending_completions->>Cuebot: send completion report
  alt Cuebot acknowledges
    Cuebot-->>pending_completions: success
    pending_completions->>pending_completions: remove frame
  else delivery fails
    Cuebot-->>pending_completions: error
    pending_completions->>pending_completions: retry later
  end

Loading

Possibly related PRs

• AcademySoftwareFoundation/OpenCue#2403: Both PRs modify FrameCompleteHandler and its tests.
• AcademySoftwareFoundation/OpenCue#2471: Both PRs modify orphaned-frame and proc recovery in MaintenanceManagerSupport.
• AcademySoftwareFoundation/OpenCue#2495: Both PRs modify orphaned-frame cleanup and completion handling across Cuebot components.

Suggested reviewers: lithorus, ramonfigueiredo

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 69.05% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the retry handling added for FrameCompleteReport failures, which is a central objective of the pull request.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

⚔️ Resolve merge conflicts 💡

• Resolve merge conflict in branch double_booking_03

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java (2)

604-653: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exit-status retouch logic duplicated verbatim from handleFrameCompleteReport.

The int exitStatus = report.getExitStatus(); if (frameDetail.exitStatus == Dispatcher.EXIT_STATUS_MEMORY_FAILURE) { exitStatus = frameDetail.exitStatus; } block at Lines 650-653 is an exact copy of Lines 187-190. Extracting a small private static helper (e.g. resolveExitStatus(FrameCompleteReport, FrameDetail)) shared by both call sites would remove the duplication and prevent the two paths from silently drifting apart if this logic changes later.

♻️ Proposed refactor

+    private static int resolveExitStatus(FrameCompleteReport report, FrameDetail frameDetail) {
+        int exitStatus = report.getExitStatus();
+        if (frameDetail.exitStatus == Dispatcher.EXIT_STATUS_MEMORY_FAILURE) {
+            exitStatus = frameDetail.exitStatus;
+        }
+        return exitStatus;
+    }

Then replace both inline blocks with resolveExitStatus(report, frameDetail).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java`
around lines 604 - 653, Extract the duplicated exit-status selection from
handleFrameCompleteReport and finalizeOrphanedFrameComplete into a private
static helper such as resolveExitStatus(FrameCompleteReport, FrameDetail). Have
the helper return the report exit status unless FrameDetail.exitStatus is
Dispatcher.EXIT_STATUS_MEMORY_FAILURE, in which case return that stored status,
and replace both inline blocks with the helper call.

---

685-734: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Factor out the shared post-completion steps. handleOrphanedPostFrameComplete still duplicates the dependency, layer-complete, job-complete, and layer-optimization logic from handlePostFrameCompleteOperations; extracting that shared block would keep the two paths in sync. The orphaned path can keep skipping proc-dependent Kafka publishing, since buildFrameCompleteEvent still requires a VirtualProc.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java`
around lines 685 - 734, Extract the shared dependency satisfaction,
layer-completion, layer optimization, and job-completion logic from
handlePostFrameCompleteOperations and handleOrphanedPostFrameComplete into a
common helper, preserving each path’s existing state conditions and arguments.
Keep proc-dependent actions, including Kafka event publishing and memory
updates, in handlePostFrameCompleteOperations only; have the orphaned path
invoke the shared helper without requiring a VirtualProc.

rust/crates/rqd/src/system/machine.rs (1)

554-649: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

No visibility/backpressure on pending_completions during extended Cuebot outages.

Every Err from send_frame_complete_report is retried forever with no cap, exponential backoff, or metric on queue size/age (Lines 640-646). Cuebot's handleFrameCompleteReport is designed so permanently-stale/dropped cases return normally (no Err) rather than throw, so in the common case this queue should stay small — but during a genuine Cuebot outage, cores keep getting released and reused for new frames while old completions pile up in pending_completions unbounded, with no signal to operators that a backlog is building. Consider emitting a periodic warning/metric keyed off pending_completions.len() (and/or oldest-entry age) when non-empty across cycles, so a growing backlog is observable before it becomes a bigger memory or duplicate-delivery concern.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/crates/rqd/src/system/machine.rs` around lines 554 - 649, Add backlog
observability to flush_pending_completions by emitting a periodic warning or
metric when pending_completions remains non-empty across monitor cycles,
including its current size and, if available, oldest-entry age. Keep the
existing retry and at-least-once delivery behavior unchanged, and avoid logging
on every individual retry.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@cuebot/src/main/java/com/imageworks/spcue/service/MaintenanceManagerSupport.java`:
- Around line 192-227: Update clearOrphanedFrames so frames encountered after
phaseDeadlineMs still receive a best-effort, non-blocking kill attempt before
being marked DEAD. Reuse the existing kill mechanism or its non-blocking variant
rather than calling killAndConfirmDead, which may exceed the exhausted budget;
preserve the current failedUnconfirmed accounting and status updates.

---

Nitpick comments:
In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java`:
- Around line 604-653: Extract the duplicated exit-status selection from
handleFrameCompleteReport and finalizeOrphanedFrameComplete into a private
static helper such as resolveExitStatus(FrameCompleteReport, FrameDetail). Have
the helper return the report exit status unless FrameDetail.exitStatus is
Dispatcher.EXIT_STATUS_MEMORY_FAILURE, in which case return that stored status,
and replace both inline blocks with the helper call.
- Around line 685-734: Extract the shared dependency satisfaction,
layer-completion, layer optimization, and job-completion logic from
handlePostFrameCompleteOperations and handleOrphanedPostFrameComplete into a
common helper, preserving each path’s existing state conditions and arguments.
Keep proc-dependent actions, including Kafka event publishing and memory
updates, in handlePostFrameCompleteOperations only; have the orphaned path
invoke the shared helper without requiring a VirtualProc.

In `@rust/crates/rqd/src/system/machine.rs`:
- Around line 554-649: Add backlog observability to flush_pending_completions by
emitting a periodic warning or metric when pending_completions remains non-empty
across monitor cycles, including its current size and, if available,
oldest-entry age. Keep the existing retry and at-least-once delivery behavior
unchanged, and avoid logging on every individual retry.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6936da85-5488-4e29-9fc9-1304f8b9aadc

📥 Commits

Reviewing files that changed from the base of the PR and between aea9a96 and 1aa4fce.

📒 Files selected for processing (10)

• cuebot/src/main/java/com/imageworks/spcue/dao/FrameDao.java
• cuebot/src/main/java/com/imageworks/spcue/dao/postgres/FrameDaoJdbc.java
• cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java
• cuebot/src/main/java/com/imageworks/spcue/rqd/RqdClient.java
• cuebot/src/main/java/com/imageworks/spcue/rqd/RqdClientGrpc.java
• cuebot/src/main/java/com/imageworks/spcue/service/MaintenanceManagerSupport.java
• cuebot/src/main/resources/conf/spring/applicationContext-service.xml
• cuebot/src/main/resources/opencue.properties
• cuebot/src/test/java/com/imageworks/spcue/test/dao/postgres/FrameDaoTests.java
• rust/crates/rqd/src/system/machine.rs

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerTests.java (1)

624-661: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Make HandlerMocks install/restore inseparable.

frameCompleteHandler is a shared singleton bean in the Spring test context. install() and restore() are two separate calls, so a leaked restore contaminates every later test in the context. Every current call site pairs them correctly, so there is no defect today. The pattern is fragile: one statement added between install() and the try block would leak the mocks.

Make the pairing structural. Implement AutoCloseable so tests use try-with-resources, or add a runWith(Runnable) method that installs, runs, and restores in a finally block. This file already documents shared-state leakage risk for ShowDaoJdbc at Lines 528-533, so the same protection is consistent here.

♻️ Proposed refactor

-    private class HandlerMocks {
+    private class HandlerMocks implements AutoCloseable {
         final DispatchSupport origDispatchSupport = frameCompleteHandler.getDispatchSupport();
@@
         void restore() {
@@
             frameCompleteHandler.setRedirectManager(origRedirectManager);
         }
+
+        `@Override`
+        public void close() {
+            restore();
+        }
+
+        void runWith(Runnable body) {
+            install();
+            try {
+                body.run();
+            } finally {
+                restore();
+            }
+        }
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerTests.java`
around lines 624 - 661, Make HandlerMocks’ install/restore lifecycle structural
by implementing AutoCloseable (or providing an equivalent runWith(Runnable)
wrapper) so installation is always paired with restoration in a finally-backed
scope. Update every existing HandlerMocks call site in FrameCompleteHandlerTests
to use the new scoped pattern, preserving the original mock state after each
test.

cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java (1)

341-359: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Resolve LayerDetail once in publishLayerCompletedTelemetry.

The method calls jobManager.getLayerDetail(frame.getLayerId()) at Line 344 and again at Line 354. When both Prometheus and Kafka are enabled, this issues two identical reads per completed layer. Resolve the value lazily so it is read at most once and still skipped when both sinks are disabled.

♻️ Proposed refactor

     private void publishLayerCompletedTelemetry(DispatchFrame frame) {
+        boolean prometheusEnabled = prometheusMetrics != null;
+        boolean kafkaEnabled = kafkaEventPublisher != null && kafkaEventPublisher.isEnabled();
+        if (!prometheusEnabled && !kafkaEnabled) {
+            return;
+        }
+        LayerDetail layerDetail = jobManager.getLayerDetail(frame.getLayerId());
+
-        if (prometheusMetrics != null) {
+        if (prometheusEnabled) {
             ExecutionSummary layerSummary = jobManager.getExecutionSummary((LayerInterface) frame);
-            LayerDetail layerDetail = jobManager.getLayerDetail(frame.getLayerId());
             prometheusMetrics.recordLayerMaxRuntime(layerSummary.highFrameSec, frame.show,
                     frame.shot, layerDetail.type.toString());
             if (layerSummary.highMemoryKb > 0) {
                 prometheusMetrics.recordLayerMaxMemory(layerSummary.highMemoryKb * 1024L,
                         frame.show, frame.shot, layerDetail.type.toString());
             }
         }
 
-        if (kafkaEventPublisher != null && kafkaEventPublisher.isEnabled()) {
-            LayerDetail layerDetail = jobManager.getLayerDetail(frame.getLayerId());
+        if (kafkaEnabled) {
             LayerEvent layerEvent = monitoringEventBuilder.buildLayerEvent(
                     EventType.LAYER_COMPLETED, layerDetail, frame.getName(), frame.show);
             kafkaEventPublisher.publishLayerEvent(layerEvent);
         }
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java`
around lines 341 - 359, Update publishLayerCompletedTelemetry to resolve
LayerDetail lazily and at most once: reuse a single local value across the
Prometheus and Kafka branches, initializing it only when the corresponding sink
is enabled. Preserve the existing behavior of skipping the lookup when both
sinks are disabled.

rust/crates/rqd/src/system/machine.rs (1)

86-92: 🩺 Stability & Availability | 🔵 Trivial

Consider a bound and a metric for pending_completions.

pending_completions has no size or age bound. Entries leave the map only on a successful acknowledgment or on a non-terminal state. If Cuebot rejects one specific report permanently, that entry and its Arc<RunningFrame> stay for the process lifetime. The retained size is bounded by the host's frame concurrency, so this is not an outage risk, but it is silent.

The current warning at Lines 618-627 is log-only. Export the backlog size and oldest-entry age as gauges so an operator can alert on a stuck report instead of reading logs. Also consider a configurable maximum age after which an entry is dropped with an error, so a poison report cannot persist forever.
[operational]

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/crates/rqd/src/system/machine.rs` around lines 86 - 92, Update the
pending_completions monitoring around its insertion and retry logic to export
gauges for current backlog size and oldest pending entry age, using the existing
metrics mechanism and updating them as entries change. Also add a configurable
maximum age for pending completions; when an entry exceeds it, remove the entry
and emit an error, while preserving normal retry and acknowledgment behavior for
entries within the limit.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java`:
- Around line 727-738: The memory-failure retry exemption is inconsistent
because determineFrameState() and the maxRetries check ignore the Cuebot-stored
status from resolveExitStatus(). Pass frameDetail through determineFrameState()
and use the resolved exit status or corresponding frame detail when applying the
memory-failure exemption, so Cuebot-initiated memory kills past maxRetries
follow the same retry path as RQD-initiated OOMs.

In
`@cuebot/src/main/java/com/imageworks/spcue/service/MaintenanceManagerSupport.java`:
- Around line 203-208: Update the budget-exhaustion branch in the method
containing killFrameBestEffort so dead is assigned from that helper’s result
instead of being forced false, preserving WAITING when no render exists and null
is returned. Add a regression test covering an empty lastResource with a zero
kill budget and verify the frame remains retryable in WAITING.

In `@rust/crates/rqd/src/system/machine.rs`:
- Around line 603-627: Update the backlog warning logic around
pending_completions and oldest_age so FailedBeforeStart entries and
clock-backward cases still emit a warning when no age is available. Preserve the
age-based warning for entries with a valid end_time, and add a backlog-size
fallback using the existing backlog value when oldest_age is None.

---

Nitpick comments:
In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java`:
- Around line 341-359: Update publishLayerCompletedTelemetry to resolve
LayerDetail lazily and at most once: reuse a single local value across the
Prometheus and Kafka branches, initializing it only when the corresponding sink
is enabled. Preserve the existing behavior of skipping the lookup when both
sinks are disabled.

In
`@cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerTests.java`:
- Around line 624-661: Make HandlerMocks’ install/restore lifecycle structural
by implementing AutoCloseable (or providing an equivalent runWith(Runnable)
wrapper) so installation is always paired with restoration in a finally-backed
scope. Update every existing HandlerMocks call site in FrameCompleteHandlerTests
to use the new scoped pattern, preserving the original mock state after each
test.

In `@rust/crates/rqd/src/system/machine.rs`:
- Around line 86-92: Update the pending_completions monitoring around its
insertion and retry logic to export gauges for current backlog size and oldest
pending entry age, using the existing metrics mechanism and updating them as
entries change. Also add a configurable maximum age for pending completions;
when an entry exceeds it, remove the entry and emit an error, while preserving
normal retry and acknowledgment behavior for entries within the limit.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5313165b-5983-4520-9c8e-ceb4ba96a748

📥 Commits

Reviewing files that changed from the base of the PR and between 1aa4fce and b367950.

📒 Files selected for processing (8)

• cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java
• cuebot/src/main/java/com/imageworks/spcue/dispatcher/commands/DispatchRqdKillFrameMemory.java
• cuebot/src/main/java/com/imageworks/spcue/service/MaintenanceManagerSupport.java
• cuebot/src/main/resources/opencue.properties
• cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerFrameStateTests.java
• cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerTests.java
• cuebot/src/test/java/com/imageworks/spcue/test/service/MaintenanceManagerOrphanFrameTests.java
• rust/crates/rqd/src/system/machine.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• cuebot/src/main/resources/opencue.properties

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java (1)

381-384: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Clear FrameDetail.exitStatus when the frame retries.

updateFrameStarted sets the frame to RUNNING and ts_stopped = null, but it leaves int_exit_status untouched before FrameCompleteHandler.resolveExitStatus and isMemoryFailure(report, frameDetail) can pull it back from the stale memory-failure value. This lets a later successful report be treated as a memory failure and recorded as such. Add a regression test for a memory-killed frame that later succeeds.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java`
around lines 381 - 384, Clear FrameDetail.exitStatus when updateFrameStarted
retries a frame, before resolveExitStatus or isMemoryFailure can inspect it,
while preserving the RUNNING and ts_stopped reset behavior. Add a regression
test covering a memory-killed frame that retries and later completes
successfully without being recorded as a memory failure.

cuebot/src/main/java/com/imageworks/spcue/service/MaintenanceManagerSupport.java (1)

245-264: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Enforce the orphan cleanup budget on polling waits and RQD calls.

Thread.sleep(ORPHAN_KILL_POLL_INTERVAL_MS) waits the full 500 ms even when only a smaller remainder of the cleanup budget remains; sleep for the remaining budget or exit when the phase deadline has been crossed. The RQD stubs use rqdTaskDeadlineSeconds via withDeadlineAfter, so an RQD call may still block longer than the configured orphan-cleanup budget; cap RQD deadlines by the remaining cleanup budget instead.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@cuebot/src/main/java/com/imageworks/spcue/service/MaintenanceManagerSupport.java`
around lines 245 - 264, Update the orphan cleanup loop around
killFrameBestEffort and rqdClient.isFrameRunning to enforce phaseDeadlineMs:
compute the remaining budget before each RQD call and pass a deadline capped to
that remainder, and before Thread.sleep either exit when no budget remains or
sleep only for the smaller of ORPHAN_KILL_POLL_INTERVAL_MS and the remaining
time. Preserve the existing success and fail-closed handling.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java`:
- Around line 381-384: Clear FrameDetail.exitStatus when updateFrameStarted
retries a frame, before resolveExitStatus or isMemoryFailure can inspect it,
while preserving the RUNNING and ts_stopped reset behavior. Add a regression
test covering a memory-killed frame that retries and later completes
successfully without being recorded as a memory failure.

In
`@cuebot/src/main/java/com/imageworks/spcue/service/MaintenanceManagerSupport.java`:
- Around line 245-264: Update the orphan cleanup loop around killFrameBestEffort
and rqdClient.isFrameRunning to enforce phaseDeadlineMs: compute the remaining
budget before each RQD call and pass a deadline capped to that remainder, and
before Thread.sleep either exit when no budget remains or sleep only for the
smaller of ORPHAN_KILL_POLL_INTERVAL_MS and the remaining time. Preserve the
existing success and fail-closed handling.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a272c249-7df1-40bf-b27e-61c9ad998e05

📥 Commits

Reviewing files that changed from the base of the PR and between b367950 and 89b5a7e.

📒 Files selected for processing (6)

• cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java
• cuebot/src/main/java/com/imageworks/spcue/service/MaintenanceManagerSupport.java
• cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerFrameStateTests.java
• cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerTests.java
• cuebot/src/test/java/com/imageworks/spcue/test/service/MaintenanceManagerOrphanFrameTests.java
• rust/crates/rqd/src/system/machine.rs

🚧 Files skipped from review as they are similar to previous changes (4)

• cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerFrameStateTests.java
• cuebot/src/test/java/com/imageworks/spcue/test/service/MaintenanceManagerOrphanFrameTests.java
• rust/crates/rqd/src/system/machine.rs
• cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerTests.java

[ramonfigueiredo]

Thanks, @DiegoTavares .

I will partially approve it after you address the suggested changes.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

rust/crates/rqd/src/system/machine.rs (2)

574-583: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Retry failed core releases or clear the finished reservation.

release_cores() removes the booking only on success, and sanitize_reservations() keeps the failed reservation while the monitor reports it as active and it has not yet reached the 60s grace period. Queue the frame for Cuebot completion separately and retry release_cores() for failed finished frames, or run a sanitization path that removes completed reservations outside running_frames.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/crates/rqd/src/system/machine.rs` around lines 574 - 583, Update the
finished-frame handling around release_cores and pending_completions so a failed
core release is retried or the completed reservation is explicitly removed from
reservation tracking outside running_frames. Keep queuing the frame for Cuebot
completion independently, and ensure failed releases do not remain indefinitely
until the 60-second sanitization grace period.

---

696-707: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve run_time for queued frame completions.

send_frame_complete_report(..., 0) serializes zero into FrameCompleteReport.run_time, while frame runtime is used by Cuebot metrics and the Python RQD path reports the measured value. Capture the runtime when enqueueing or at flush time for FrameState::Finished; use 0 explicitly for FailedBeforeStart paths.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/crates/rqd/src/system/machine.rs` around lines 696 - 707, Update the
frame-completion reporting flow around send_frame_complete_report to pass the
measured run_time for queued FrameState::Finished completions instead of the
hardcoded 0, preserving it through enqueueing or flush. Keep 0 only for
FrameState::FailedBeforeStart reports, and ensure the serialized
FrameCompleteReport retains the runtime used by metrics.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@rust/crates/rqd/src/system/machine.rs`:
- Around line 574-583: Update the finished-frame handling around release_cores
and pending_completions so a failed core release is retried or the completed
reservation is explicitly removed from reservation tracking outside
running_frames. Keep queuing the frame for Cuebot completion independently, and
ensure failed releases do not remain indefinitely until the 60-second
sanitization grace period.
- Around line 696-707: Update the frame-completion reporting flow around
send_frame_complete_report to pass the measured run_time for queued
FrameState::Finished completions instead of the hardcoded 0, preserving it
through enqueueing or flush. Keep 0 only for FrameState::FailedBeforeStart
reports, and ensure the serialized FrameCompleteReport retains the runtime used
by metrics.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a0fa7e3a-94f8-4863-b823-44ec99a21f4e

📥 Commits

Reviewing files that changed from the base of the PR and between 89b5a7e and a6bc859.

📒 Files selected for processing (3)

• cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java
• cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerTests.java
• rust/crates/rqd/src/system/machine.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java
• cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerTests.java

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

cuebot/src/main/java/com/imageworks/spcue/service/MaintenanceManagerSupport.java (1)

371-427: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Bound every RQD request to the remaining sweep budget.

deleteOrphanedFrames() uses per-frame and pass deadlines, but rqdClient.killFrame(...) and rqdClient.isFrameRunning(...) always use the default grpc.rqd_task_deadline of 10s. A frame can therefore be stuck in a long sync RQD request after the per-frame deadline, and remaining frames can block on best-effort kills after the pass deadline. Enforce rqdTaskDeadline with no more than the remaining deadlineMs for both calls, and use a non-blocking dispatch path when the pass deadline is already reached.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@cuebot/src/main/java/com/imageworks/spcue/service/MaintenanceManagerSupport.java`
around lines 371 - 427, Update killFrameBestEffort and killAndConfirmDead to
bound both rqdClient.killFrame and rqdClient.isFrameRunning calls by
rqdTaskDeadline and the remaining deadlineMs, using the smaller duration. When
the pass or per-frame deadline has already expired, dispatch the best-effort
kill through the non-blocking path instead of issuing a blocking RQD request,
while preserving confirmation behavior and result handling.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cuebot/src/main/java/com/imageworks/spcue/dao/postgres/FrameDaoJdbc.java`:
- Around line 447-448: Update the query built in FrameDaoJdbc to add
deterministic ordering before LIMIT: sort by the oldest frame.ts_updated first,
then use frame.pk_frame as the stable tie-breaker, while preserving the existing
filtering and limit behavior.

In
`@cuebot/src/main/resources/conf/ddl/postgres/migrations/V46__Add_orphaned_frame_check_task_lock.sql`:
- Around line 5-6: The orphan-frame task-lock timeout is specified in
milliseconds instead of seconds. Update the INSERT for LOCK_ORPHANED_FRAME_CHECK
in
cuebot/src/main/resources/conf/ddl/postgres/migrations/V46__Add_orphaned_frame_check_task_lock.sql:5-6
and the matching seed-data row in
cuebot/src/main/resources/conf/ddl/postgres/seed_data.sql:90 to use the intended
seconds value, such as 30, while keeping both rows consistent.

---

Outside diff comments:
In
`@cuebot/src/main/java/com/imageworks/spcue/service/MaintenanceManagerSupport.java`:
- Around line 371-427: Update killFrameBestEffort and killAndConfirmDead to
bound both rqdClient.killFrame and rqdClient.isFrameRunning calls by
rqdTaskDeadline and the remaining deadlineMs, using the smaller duration. When
the pass or per-frame deadline has already expired, dispatch the best-effort
kill through the non-blocking path instead of issuing a blocking RQD request,
while preserving confirmation behavior and result handling.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f243808d-3160-4723-9b45-9e57990cdc2e

📥 Commits

Reviewing files that changed from the base of the PR and between a6bc859 and 879ac63.

📒 Files selected for processing (11)

• cuebot/src/main/java/com/imageworks/spcue/MaintenanceTask.java
• cuebot/src/main/java/com/imageworks/spcue/dao/FrameDao.java
• cuebot/src/main/java/com/imageworks/spcue/dao/postgres/FrameDaoJdbc.java
• cuebot/src/main/java/com/imageworks/spcue/service/MaintenanceManagerSupport.java
• cuebot/src/main/resources/conf/ddl/postgres/migrations/V46__Add_orphaned_frame_check_task_lock.sql
• cuebot/src/main/resources/conf/ddl/postgres/seed_data.sql
• cuebot/src/main/resources/conf/spring/applicationContext-service.xml
• cuebot/src/main/resources/opencue.properties
• cuebot/src/test/java/com/imageworks/spcue/test/dao/postgres/FrameDaoTests.java
• cuebot/src/test/java/com/imageworks/spcue/test/service/MaintenanceManagerOrphanFrameTests.java
• rust/crates/rqd/src/system/machine.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• cuebot/src/test/java/com/imageworks/spcue/test/dao/postgres/FrameDaoTests.java
• rust/crates/rqd/src/system/machine.rs