[rqd/cuebot] Add a retry mechanism for FrameCompleteReport #2473
[rqd/cuebot] Add a retry mechanism for FrameCompleteReport #2473DiegoTavares wants to merge 3 commits into
Conversation
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 requeue, 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 exemple) could lead to a completed frame not having its state synchronized to Cuebot.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
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<FrameDetail> (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. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
abeec3f to
bd98c03
Compare
Signed-off-by: Diego Tavares <dtavares@imageworks.com>
There was a problem hiding this comment.
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 winExit-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 winFactor out the shared post-completion steps.
handleOrphanedPostFrameCompletestill duplicates the dependency, layer-complete, job-complete, and layer-optimization logic fromhandlePostFrameCompleteOperations; extracting that shared block would keep the two paths in sync. The orphaned path can keep skipping proc-dependent Kafka publishing, sincebuildFrameCompleteEventstill requires aVirtualProc.🤖 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 winNo visibility/backpressure on
pending_completionsduring extended Cuebot outages.Every
Errfromsend_frame_complete_reportis retried forever with no cap, exponential backoff, or metric on queue size/age (Lines 640-646). Cuebot'shandleFrameCompleteReportis designed so permanently-stale/dropped cases return normally (noErr) 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 inpending_completionsunbounded, with no signal to operators that a backlog is building. Consider emitting a periodic warning/metric keyed offpending_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
📒 Files selected for processing (10)
cuebot/src/main/java/com/imageworks/spcue/dao/FrameDao.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/FrameDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.javacuebot/src/main/java/com/imageworks/spcue/rqd/RqdClient.javacuebot/src/main/java/com/imageworks/spcue/rqd/RqdClientGrpc.javacuebot/src/main/java/com/imageworks/spcue/service/MaintenanceManagerSupport.javacuebot/src/main/resources/conf/spring/applicationContext-service.xmlcuebot/src/main/resources/opencue.propertiescuebot/src/test/java/com/imageworks/spcue/test/dao/postgres/FrameDaoTests.javarust/crates/rqd/src/system/machine.rs
| public void clearOrphanedFrames() { | ||
| long killBudgetMs = env.getProperty("maintenance.orphaned_frame_kill_budget_ms", Long.class, | ||
| DEFAULT_ORPHANED_FRAME_KILL_BUDGET_MS); | ||
| long phaseDeadlineMs = System.currentTimeMillis() + killBudgetMs; | ||
| int failedUnconfirmed = 0; | ||
|
|
||
| List<FrameDetail> frames = frameDao.getOrphanedFrames(); | ||
| for (FrameDetail frame : frames) { | ||
| try { | ||
| if (frameDao.updateFrameStopped(frame, FrameState.WAITING, | ||
| Dispatcher.EXIT_STATUS_FRAME_ORPHAN)) { | ||
| /* | ||
| * The proc row is already gone, so the host<->frame link is lost and RQD cannot | ||
| * be killed from here. Reaching this point means a zombie may have been | ||
| * produced upstream (a proc was deleted without confirming the frame stopped). | ||
| * Report it so the residual rate is observable after the kill-before-release | ||
| * and defer-release fixes. | ||
| */ | ||
| logger.warn("Reset orphaned frame " + frame.getName() + " (frameId=" | ||
| + frame.getFrameId() + ") to WAITING; its proc was already gone so RQD " | ||
| + "could not be killed. If RQD is still rendering it this is a " | ||
| + "double-booking risk."); | ||
| Sentry.withScope(scope -> { | ||
| scope.setExtra("frame_id", frame.getFrameId()); | ||
| scope.setExtra("frame_name", frame.getName()); | ||
| Sentry.captureMessage("Maintenance reset orphaned frame with no proc"); | ||
| }); | ||
| boolean dead; | ||
| if (System.currentTimeMillis() >= phaseDeadlineMs) { | ||
| dead = false; | ||
| } else { | ||
| dead = killAndConfirmDead(frame, phaseDeadlineMs); | ||
| } | ||
|
|
||
| if (dead) { | ||
| frameDao.updateFrameStopped(frame, FrameState.WAITING, | ||
| Dispatcher.EXIT_STATUS_FRAME_ORPHAN); | ||
| } else { | ||
| frameDao.updateFrameStopped(frame, FrameState.DEAD, | ||
| Dispatcher.EXIT_STATUS_FRAME_ORPHAN); | ||
| failedUnconfirmed++; | ||
| } | ||
| } catch (Exception e) { | ||
| logger.info("failed to clear orphaned frame: " + frame.getName() + " " + e); | ||
| } | ||
| } | ||
|
|
||
| if (failedUnconfirmed > 0) { | ||
| logger.warn(failedUnconfirmed + " orphaned frame(s) were marked DEAD because the " | ||
| + "original render could not be confirmed dead (kill budget exhausted or host " | ||
| + "unreachable); they need a manual retry. Marking DEAD avoids double-booking " | ||
| + "them."); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Frames processed after the kill budget is exhausted never get a kill attempt at all.
When phaseDeadlineMs has already passed, line 203 sets dead = false directly, skipping killAndConfirmDead entirely — so rqdClient.killFrame is never even called for that frame before it's marked DEAD. The actual render on the frame's last-known host is left running unmonitored forever (no proc row, no auto-retry), a real resource leak on the render host. Given a single slow/unreachable host earlier in the pass can consume most of the default 30s budget (kill + first status check each carry their own grpc deadline), this isn't just a theoretical edge case.
Consider still firing a best-effort, non-blocking kill for these frames before marking them DEAD, matching the "best effort" pattern already used inside killAndConfirmDead.
🔧 Proposed fix: best-effort kill even when budget is already exhausted
List<FrameDetail> frames = frameDao.getOrphanedFrames();
for (FrameDetail frame : frames) {
try {
boolean dead;
if (System.currentTimeMillis() >= phaseDeadlineMs) {
- dead = false;
+ bestEffortKill(frame);
+ dead = false;
} else {
dead = killAndConfirmDead(frame, phaseDeadlineMs);
}+ private void bestEffortKill(FrameDetail frame) {
+ if (frame.lastResource == null || frame.lastResource.isEmpty()) {
+ return;
+ }
+ String host = frame.lastResource.split("/")[0];
+ if (host.isEmpty()) {
+ return;
+ }
+ try {
+ rqdClient.killFrame(host, frame.getFrameId(),
+ "kill-before-reset: clearing orphaned frame (budget exhausted)");
+ } catch (Exception e) {
+ logger.info("best-effort kill failed for orphaned frame " + frame.getName() + ", " + e);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void clearOrphanedFrames() { | |
| long killBudgetMs = env.getProperty("maintenance.orphaned_frame_kill_budget_ms", Long.class, | |
| DEFAULT_ORPHANED_FRAME_KILL_BUDGET_MS); | |
| long phaseDeadlineMs = System.currentTimeMillis() + killBudgetMs; | |
| int failedUnconfirmed = 0; | |
| List<FrameDetail> frames = frameDao.getOrphanedFrames(); | |
| for (FrameDetail frame : frames) { | |
| try { | |
| if (frameDao.updateFrameStopped(frame, FrameState.WAITING, | |
| Dispatcher.EXIT_STATUS_FRAME_ORPHAN)) { | |
| /* | |
| * The proc row is already gone, so the host<->frame link is lost and RQD cannot | |
| * be killed from here. Reaching this point means a zombie may have been | |
| * produced upstream (a proc was deleted without confirming the frame stopped). | |
| * Report it so the residual rate is observable after the kill-before-release | |
| * and defer-release fixes. | |
| */ | |
| logger.warn("Reset orphaned frame " + frame.getName() + " (frameId=" | |
| + frame.getFrameId() + ") to WAITING; its proc was already gone so RQD " | |
| + "could not be killed. If RQD is still rendering it this is a " | |
| + "double-booking risk."); | |
| Sentry.withScope(scope -> { | |
| scope.setExtra("frame_id", frame.getFrameId()); | |
| scope.setExtra("frame_name", frame.getName()); | |
| Sentry.captureMessage("Maintenance reset orphaned frame with no proc"); | |
| }); | |
| boolean dead; | |
| if (System.currentTimeMillis() >= phaseDeadlineMs) { | |
| dead = false; | |
| } else { | |
| dead = killAndConfirmDead(frame, phaseDeadlineMs); | |
| } | |
| if (dead) { | |
| frameDao.updateFrameStopped(frame, FrameState.WAITING, | |
| Dispatcher.EXIT_STATUS_FRAME_ORPHAN); | |
| } else { | |
| frameDao.updateFrameStopped(frame, FrameState.DEAD, | |
| Dispatcher.EXIT_STATUS_FRAME_ORPHAN); | |
| failedUnconfirmed++; | |
| } | |
| } catch (Exception e) { | |
| logger.info("failed to clear orphaned frame: " + frame.getName() + " " + e); | |
| } | |
| } | |
| if (failedUnconfirmed > 0) { | |
| logger.warn(failedUnconfirmed + " orphaned frame(s) were marked DEAD because the " | |
| + "original render could not be confirmed dead (kill budget exhausted or host " | |
| + "unreachable); they need a manual retry. Marking DEAD avoids double-booking " | |
| + "them."); | |
| } | |
| } | |
| public void clearOrphanedFrames() { | |
| long killBudgetMs = env.getProperty("maintenance.orphaned_frame_kill_budget_ms", Long.class, | |
| DEFAULT_ORPHANED_FRAME_KILL_BUDGET_MS); | |
| long phaseDeadlineMs = System.currentTimeMillis() + killBudgetMs; | |
| int failedUnconfirmed = 0; | |
| List<FrameDetail> frames = frameDao.getOrphanedFrames(); | |
| for (FrameDetail frame : frames) { | |
| try { | |
| boolean dead; | |
| if (System.currentTimeMillis() >= phaseDeadlineMs) { | |
| bestEffortKill(frame); | |
| dead = false; | |
| } else { | |
| dead = killAndConfirmDead(frame, phaseDeadlineMs); | |
| } | |
| if (dead) { | |
| frameDao.updateFrameStopped(frame, FrameState.WAITING, | |
| Dispatcher.EXIT_STATUS_FRAME_ORPHAN); | |
| } else { | |
| frameDao.updateFrameStopped(frame, FrameState.DEAD, | |
| Dispatcher.EXIT_STATUS_FRAME_ORPHAN); | |
| failedUnconfirmed++; | |
| } | |
| } catch (Exception e) { | |
| logger.info("failed to clear orphaned frame: " + frame.getName() + " " + e); | |
| } | |
| } | |
| if (failedUnconfirmed > 0) { | |
| logger.warn(failedUnconfirmed + " orphaned frame(s) were marked DEAD because the " | |
| "original render could not be confirmed dead (kill budget exhausted or host " | |
| "unreachable); they need a manual retry. Marking DEAD avoids double-booking " | |
| "them."); | |
| } | |
| } | |
| private void bestEffortKill(FrameDetail frame) { | |
| if (frame.lastResource == null || frame.lastResource.isEmpty()) { | |
| return; | |
| } | |
| String host = frame.lastResource.split("/")[0]; | |
| if (host.isEmpty()) { | |
| return; | |
| } | |
| try { | |
| rqdClient.killFrame(host, frame.getFrameId(), | |
| "kill-before-reset: clearing orphaned frame (budget exhausted)"); | |
| } catch (Exception e) { | |
| logger.info("best-effort kill failed for orphaned frame " + frame.getName() + ", " + e); | |
| } | |
| } |
🤖 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 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.
RQD silently drops FrameCompleteReports
In rust/crates/rqd/src/system/machine.rs:
(line 405-420): the retain() closure returns false for FrameState::Finished, removing it, then
calls handle_finished_frames.
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.
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
Configuration