[Cuebot] Double booking 5 - #2513
Draft
DiegoTavares wants to merge 8 commits into
Draft
Conversation
getKillClearance wrote the attempt counter back to killRequestCounterCache before checking the retry limit. Since put() restarts the cache's expireAfterWrite clock and a still-running frame is re-detected on every host report (~10s), every denied attempt refreshed the entry, so it could never expire: after one exhausted burst of 3 failed kills, the host+frame pair was never killed again for as long as the zombie kept reporting. A transient kill failure (RQD busy behind its monitor-loop mutex, a dropped RPC, a deadline) therefore left a double-booked render running to completion. The double-booking log crawl shows this class directly: 259 overlapping runs in the last two weeks where the superseded render survived 20+ minutes (p90 2.7h) after the frame was rebooked, most finishing SUCCESS hours later. Only write the counter on granted attempts. Once the limit is hit the entry is left untouched, expiring FRAME_KILL_CACHE_EXPIRE_AFTER_WRITE minutes after the last granted kill, after which a fresh kill budget is available -- converting "give up forever" into "back off for 3 minutes". Unit tests pin the cooldown with a fake ticker; the refresh-forever regression is covered explicitly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E2kQu2ZFrvhTLAdjTau2hQ
…t delivered lostProc treated a successful killFrame RPC as proof the render was stopped and released the booking immediately. RQD acknowledges a kill as soon as the SIGTERM is delivered to the process group -- the render stays alive (still writing output) until it honors the signal, which takes ~20s for a well-behaved render flushing its log, and indefinitely for one that traps SIGTERM (rust RQD ships with force_kill_after_timeout=false, so there is no SIGKILL escalation). Releasing on the ACK re-books the frame within seconds while the superseded render is still running: the double-booking log crawl attributes ~3,200 overlapping runs in the last two weeks to this window (booking freed mid-run, superseded log closing ~20s after the replacement started). - RqdClient.killFrame now returns whether the frame is CONFIRMED not running: true only on RQD's NOT_FOUND answer (nothing to kill), false when the kill was delivered to a live render. - lostProc releases immediately only on confirmed-stopped. A delivered kill defers the release instead: the proc and RUNNING frame are left intact so the run's own FrameCompleteReport finalizes it through the normal ownership-fenced path. The existing lost_proc_max_defer_ms bound still applies (frame parked DEAD, never WAITING, if the report never arrives). - A delivered kill is positive proof of a live RQD and render, so it also disables the host-DOWN / host-not-Up / rebooted-since-dispatch release escape hatches for that proc: a host marked DOWN that still answers its kill RPC is partitioned or report-starved, not dead. Unit tests cover the ACK-vs-NOT_FOUND return in RqdClientGrpc against a real localhost gRPC server, and the new defer/confirm/bound resolutions in lostProc. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E2kQu2ZFrvhTLAdjTau2hQ
lostProc released a proc the moment its host was marked DOWN (EXIT_STATUS_DOWN_HOST or !isHostUp): the kill's failure was ignored and the RUNNING frame went straight back to WAITING. But DOWN only means "no processed host report for 300s" -- a network partition or a Cuebot-side report-ingest stall produces exactly the same mark while every render on the host keeps running. clearDownProcs then rebooked all of them at once, which the double-booking log crawl shows as the multi-minute-gap double-render class (~3.5k rows post-Aug-11; partition-length overlaps, first render killed only after the partition healed). A DOWN host now goes through the same fail-closed deferral as any unconfirmed kill, and the lost_proc_max_defer_ms bound decides by evidence when it expires: - Host down/not-Up AND silent for the whole bound: a live-but-partitioned host would have resumed reporting, so it is treated as genuinely dead and the frame returns to WAITING (auto-retry preserved, delayed by the bound). - Host still Up (or a delivered kill whose complete report never came): frame parked DEAD (manual retry), as before. Kill retries against an unreachable host stay cheap after the first attempt: the cached gRPC channel is in TRANSIENT_FAILURE and fails fast until its backoff reconnects. clearDownProcs now skips its Sentry report for deferred procs, and only counts actual releases. The trade-off is that frames on a genuinely dead host re-queue after lost_proc_max_defer_ms (20 min default) instead of ~5 minutes; partitions shorter than the bound no longer double-render at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E2kQu2ZFrvhTLAdjTau2hQ
updateFrameHostDown (UPDATE_FRAME_REASON) reset a frame to WAITING with no state fence -- only a no-proc-attached guard. Its caller checks the frame is DEAD first, but between that read and the update the frame can be finalized by another actor (eaten, completed after a late report), and the unfenced write would resurrect it for a fresh re-run of an already finished frame. Add str_state='DEAD' to the WHERE clause so the resurrection can only ever flip the state it was designed for. DAO test covers both the resurrect and refuse directions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E2kQu2ZFrvhTLAdjTau2hQ
Four defects let a "successfully killed" frame keep rendering after
Cuebot released or re-booked it, producing double-renders that the Aug
2026 log crawl shows surviving from 20 minutes to hours:
- No SIGKILL escalation by default: force_kill_after_timeout was false,
so the kill monitor logged "gave up" after 20 minutes and a
SIGTERM-trapping render survived its kill indefinitely. Escalation is
now on by default, with a 5 minute timeout and 1 minute checks.
- The monitor countdown only decremented while limit >= interval, so a
timeout that is not a multiple of the interval never reached zero and
escalation never fired. The decrement is now unconditional.
- killpg ESRCH ("no such process") was reported as a failed kill even
though the target being gone is exactly the state the kill was meant
to reach. Cuebot then counted it against its kill-retry budget and
eventually stopped killing the zombie. ESRCH now counts as success in
kill_session/force_kill_session/force_kill on both platforms.
- Killing a frame in a terminal state (Finished/FailedBeforeStart)
returned FAILED_PRECONDITION, which Cuebot treats as an unconfirmed
kill and defers on. It now returns NOT_FOUND ("nothing running to
kill"), the strongest not-running proof Cuebot already understands.
get_pid_to_kill now returns a typed KillOutcome instead of a stringly
Result, and kill_all_running_frames skips unkillable frames instead of
aborting the sweep on the first one (a single not-yet-started frame
previously left every other frame alive and propagated an error into the
NIMBY lock path).
Unit tests cover all four get_pid_to_kill dispositions and the ESRCH
mapping.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E2kQu2ZFrvhTLAdjTau2hQ
A kill for a frame in Created state (inserted into the running-frames cache, process not spawned yet -- a window that stretches to tens of seconds when log setup blocks on NFS) was refused with FAILED_PRECONDITION and forgotten: no kill_reason was recorded, so the frame proceeded to launch and render as if nothing happened. Cuebot kills landing in that window (verification kills, kill-before-release, orphan reclaims) were silently dropped, leaving a render alive on a booking Cuebot believed it had killed. The kill is now recorded on the Created state as a pending kill and the RPC is acknowledged (KillOutcome::Scheduled): start() carries the reason into the Running state, and run_inner terminates the process group immediately after spawn, before the render does any real work. The frame then finishes through its normal report path with exit_signal 15. kill_all_running_frames counts scheduled kills as handled instead of skipping the frame. Tests cover the pending-kill recording, the carry-over into Running, and an end-to-end run where a launch-window kill terminates the spawned process immediately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E2kQu2ZFrvhTLAdjTau2hQ
When the cached /proc scan failed to see a frame's pid for 15 seconds, monitor_running_frames declared the frame finished (exit_code=1, exit_signal=19), removed it from the cache and queued a FrameCompleteReport -- without ever touching the process. A stalled or incomplete scan (the monitor loop blocks on NFS stats and the same mutex serves launches and kills) produces exactly that picture while the render is alive: Cuebot receives the completion, re-books the frame, and the untouched render keeps running as an RQD-originated double-render that even the verification kill cannot see (the frame is no longer in the cache, so isFrameRunning answers NOT_FOUND). The dangling expiry now probes the pid directly with kill(pid, 0) -- independent of the cached scan. A live process is force-killed and kept in the cache; the completion is only reported once a later sweep confirms the pid is actually gone. Tests cover the pid probe for both a live process and a reaped child. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E2kQu2ZFrvhTLAdjTau2hQ
The monitor loop propagated errors from collect_and_send_host_report and handle_nimby_state_change with `?`, terminating the loop on the first failure: one report send that exhausts its gRPC retry ladder (Cuebot restart windows, network blips), or one NIMBY kill_all error, and the host silently stops reporting forever while its frames keep rendering. Cuebot then marks the host DOWN after 5 minutes and re-books every running frame -- a whole-host double-render factory, invisible until someone notices the host went quiet (main() only logs a JoinError). Both failures are now logged and retried on the next 5s tick instead of killing the loop. The NIMBY state transition keeps its previous state on failure so the transition is re-attempted, and kill_all_running_frames no longer aborts on individual frames since the previous commits. The existing integration suite covers the loop's happy path; the change is error containment only, with no behavior change on success. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E2kQu2ZFrvhTLAdjTau2hQ
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
Collaborator
Author
|
Most fixes on this PR have a strong tradeoff that needs to be evaluated. I'm keeping it on standby for now. Planning to get back to it next week. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Handle lingering double booking conditions: