[DiegoTavares]

Patch another condition that may lead to double-booking on retried frames.

retryFrame released the frame (WAITING, proc link cleared) before killing RQD, so it was instantly booked onto a second host. FrameCompleteHandler then applied the killed run's report without checking the reporting proc still owned the frame, stopping the new run and freeing the frame again — every kill produced a report and every report freed the frame, sustaining a kill→report→free→rebook loop across hosts and silently discarding successful renders.

• FrameCompleteHandler: ownership fence — a report may only stop the frame while its proc still holds it; superseded reports divert to the stale path. The stale path no longer unbooks a proc re-dispatched the same frame, and unapplied reports (notably exit 0) are logged and counted.
• JobManagerSupport.retryFrame: kill and confirm dead via isFrameRunning polling (dispatcher.retry_frame_kill_budget_ms, shared across a bulk retry) before releasing; fail closed (frame stays RUNNING) when death cannot be confirmed. Gated by dispatcher.kill_running_frame_before_release_enabled.
• New counters: frame_complete_superseded, frame_complete_dropped, frame_retry_deferred, frame_zombie_render.

LLM usage disclosure

Claude Opus was used for investigating the root cause and creating unit tests.

Summary by CodeRabbit

• Bug Fixes

  • Prevented outdated frame-completion reports from stopping or releasing newer frame runs.
  • Improved handling of dropped, redirected, missing, and orphaned frame reports.
  • Retries now confirm renders have stopped before releasing frames, reducing duplicate or conflicting work.
  • Retries are deferred safely when termination cannot be confirmed within the configured time limit.

• Monitoring

  • Added metrics for superseded and dropped completions, deferred retries, and zombie renders, improving operational visibility.

[coderabbitai]

📝 Walkthrough

Walkthrough

The change adds metrics for stale completions, deferred retries, and zombie renders. Completion handling validates processor ownership. Running-frame retries can confirm render termination before release by using shared configurable deadlines.

Changes

Completion and retry safety

Layer / File(s)	Summary
Frame completion ownership fencing cuebot/src/main/java/com/imageworks/spcue/PrometheusMetricsCollector.java, cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java, cuebot/src/main/java/com/imageworks/spcue/dispatcher/HostReportHandler.java, cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerOwnershipTests.java	Completion handlers verify processor ownership before stopping or unbooking frames. Stale, orphaned, dropped, and zombie reports increment Prometheus metrics.
Kill-before-release retry flow cuebot/src/main/java/com/imageworks/spcue/service/JobManagerSupport.java, cuebot/src/main/resources/opencue.properties, cuebot/src/test/java/com/imageworks/spcue/test/service/JobManagerSupportRetryFrameTests.java	Running-frame retries kill renders and poll RQD until termination or a shared deadline. Failed confirmation defers retries and preserves RUNNING. Disabled kill-before-release retains the legacy order.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant RQD
  participant FrameCompleteHandler
  participant FrameDetail
  participant PrometheusMetricsCollector
  RQD->>FrameCompleteHandler: submit frame completion report
  FrameCompleteHandler->>FrameDetail: verify current processor ownership
  alt report is superseded or stale
    FrameCompleteHandler->>PrometheusMetricsCollector: increment dropped completion metric
    FrameCompleteHandler->>FrameDetail: preserve newer frame ownership
  else report is current
    FrameCompleteHandler->>FrameDetail: stop completed frame
  end

Loading

sequenceDiagram
  participant JobManagerSupport
  participant RQD
  participant FrameDetail
  participant PrometheusMetricsCollector
  JobManagerSupport->>RQD: kill running render
  JobManagerSupport->>RQD: poll render status
  alt render exits
    JobManagerSupport->>FrameDetail: release frame
  else deadline or confirmation failure
    JobManagerSupport->>PrometheusMetricsCollector: increment deferred retry metric
    JobManagerSupport->>FrameDetail: leave frame RUNNING
  end

Loading

Possibly related PRs

• AcademySoftwareFoundation/OpenCue#2441: Both PRs modify HostReportHandler#killUnverifiedRunningFrame and address zombie or stale frame fencing.
• AcademySoftwareFoundation/OpenCue#2471: This PR extends the same kill-before-release and deferred-retry behavior.
• AcademySoftwareFoundation/OpenCue#2473: Both PRs modify FrameCompleteHandler to handle stale and orphaned completion reports safely.

Suggested reviewers: lithorus, ramonfigueiredo

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.38% 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 primary change: adding safeguards to prevent double booking during retried frame execution.
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

🧪 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: 2

🧹 Nitpick comments (1)

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

441-451: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

The shared budget bounds sleeping, not RQD RPC time.

retryFrames shares one deadline across all frames, and killAndConfirmDead guarantees at least one isFrameRunning poll per frame even after the deadline passes. Each poll is a gRPC call to RQD with its own deadline (grpc.rqd_task_deadline=10 seconds). A bulk retry of N running frames therefore performs N kill calls plus at least N confirmation calls on the calling thread, and the wall-clock cost scales with N regardless of dispatcher.retry_frame_kill_budget_ms.

Consider skipping the confirmation poll once the shared deadline is spent and deferring those frames directly, or bounding the number of frames confirmed per request.

♻️ Proposed change: skip the poll when the shared budget is already spent

-        // do/while so the render gets at least one confirmation poll even when the shared budget
-        // was spent on earlier frames.
-        do {
+        if (System.currentTimeMillis() >= deadlineMs) {
+            // The shared budget is spent. Defer rather than spend another RQD round trip per
+            // remaining frame; the kill above is still best-effort delivered.
+            return false;
+        }
+        do {

Also applies to: 582-601

🤖 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/JobManagerSupport.java`
around lines 441 - 451, Update retryFrame and its killAndConfirmDead path to
check the shared killDeadlineMs before issuing any RQD confirmation poll. When
the deadline has expired after the kill request, skip isFrameRunning
confirmation and defer the frame directly; retain confirmation while budget
remains and preserve existing retryFrames behavior for frames processed within
the budget.

🤖 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 304-317: Update handleFrameCompleteReport to re-read the proc
through hostManager.getVirtualProc for every ownership decision, regardless of
frameDetail.state. After the fresh read, drop the stale report whenever
currentProc.frameId represents any newer assignment, including a different
frame, before redirecting or unbooking; retain the missing-proc early return.

In `@cuebot/src/main/java/com/imageworks/spcue/service/JobManagerSupport.java`:
- Around line 76-77: Add annotation processing for JobManagerSupport by enabling
context:annotation-config in applicationContext-service.xml, or explicitly wire
the Environment dependency through XML. Ensure the `@Autowired` env field in
JobManagerSupport is populated so configuration properties are applied.

---

Nitpick comments:
In `@cuebot/src/main/java/com/imageworks/spcue/service/JobManagerSupport.java`:
- Around line 441-451: Update retryFrame and its killAndConfirmDead path to
check the shared killDeadlineMs before issuing any RQD confirmation poll. When
the deadline has expired after the kill request, skip isFrameRunning
confirmation and defer the frame directly; retain confirmation while budget
remains and preserve existing retryFrames behavior for frames processed within
the budget.

🪄 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: 78a85e4c-ec12-4e67-a79a-5afa53ec283b

📥 Commits

Reviewing files that changed from the base of the PR and between 1f04fce and 3087348.

📒 Files selected for processing (7)

• cuebot/src/main/java/com/imageworks/spcue/PrometheusMetricsCollector.java
• cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java
• cuebot/src/main/java/com/imageworks/spcue/dispatcher/HostReportHandler.java
• cuebot/src/main/java/com/imageworks/spcue/service/JobManagerSupport.java
• cuebot/src/main/resources/opencue.properties
• cuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerOwnershipTests.java
• cuebot/src/test/java/com/imageworks/spcue/test/service/JobManagerSupportRetryFrameTests.java

[ramonfigueiredo]

Approved with some suggested changes

[coderabbitai]

Actionable comments posted: 1

🤖 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/test/java/com/imageworks/spcue/test/service/JobManagerSupportRetryFrameTests.java`:
- Around line 108-114: Update the test fixture setup in
JobManagerSupportRetryFrameTests to inject a PrometheusMetricsCollector mock
into the service under test. In the deferred-retry tests, including
exhausted-budget and unreachable-host scenarios, verify that
incrementFrameRetryDeferred() is called exactly once when kill confirmation
fails.

🪄 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: 5c0e0cf6-dd58-408a-9c0a-22c0e0c438b7

📥 Commits

Reviewing files that changed from the base of the PR and between 9673585 and 0920848.

📒 Files selected for processing (4)

• cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java
• cuebot/src/main/java/com/imageworks/spcue/service/JobManagerSupport.java
• cuebot/src/main/resources/opencue.properties
• cuebot/src/test/java/com/imageworks/spcue/test/service/JobManagerSupportRetryFrameTests.java

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

• cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java
• cuebot/src/main/resources/opencue.properties
• cuebot/src/main/java/com/imageworks/spcue/service/JobManagerSupport.java

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Test the frame_retry_deferred metric.

The fixture does not inject PrometheusMetricsCollector. The deferred-retry tests therefore cannot detect a missing incrementFrameRetryDeferred() call or missing collector wiring.

Inject a collector mock. Assert one increment when kill confirmation fails, including the exhausted-budget and unreachable-host 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
`@cuebot/src/test/java/com/imageworks/spcue/test/service/JobManagerSupportRetryFrameTests.java`
around lines 108 - 114, Update the test fixture setup in
JobManagerSupportRetryFrameTests to inject a PrometheusMetricsCollector mock
into the service under test. In the deferred-retry tests, including
exhausted-budget and unreachable-host scenarios, verify that
incrementFrameRetryDeferred() is called exactly once when kill confirmation
fails.