Repository navigation
Recover tasks lost to shutdown and stuck in-flight labels - #675
Conversation
PR Summary by QodoRecover in-flight Redis tasks on SIGTERM; re-triage issues stuck with stale in-flight labels
AI Description
Diagram
High-Level Assessment
Files changed (19)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a graceful shutdown mechanism for Ymir agents and a safety net for stale in-flight Jira labels. Specifically, it introduces signal handlers to catch SIGTERM/SIGINT, allowing the task loop to stop pulling new tasks, cancel active tasks, and safely re-push their payloads back to Redis. Additionally, the Jira issue fetcher is updated to detect and recover abandoned in-flight labels by flipping them to a retry-needed state. The review feedback is highly constructive, highlighting potential issues with task leaks during outer cancellation in _race_shutdown, platform compatibility on Windows for signal handlers, and the need for robust exception handling when re-pushing tasks to Redis during shutdown.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Code Review by Qodo
Context used✅ Compliance rules (platform):
7 rules 1.
|
![qodo-for-packit[bot]](https://avatars.githubusercontent.com/in/4066991?s=60&v=4){kind=small}
TomasTomecek
left a comment
TomasTomecek
left a comment
There was a problem hiding this comment.
I just reviewed it with Cursor (Composer) and Sonnet 5 and both agree it's very well done and most edge cases are sorted out. One unsolved that stood out to me is:
mr_consolidation has no recovery path for its own stuck state (moderate, undocumented)
When a consolidation task is cancelled, process_task deliberately leaves the :active hash field in place rather than calling complete_job() — correctly matching pre-PR crash behavior. But pick_next_job's Lua
script refuses to promote a new pending job for that package/branch while :active is set (merge_queue.py:85), and nothing in this PR (or elsewhere) ever clears a stale :active entry — unlike
triage/backport/rebase/rebuild, which get the new Jira-label staleness sweep. So any redeploy that catches an in-flight consolidation job permanently blocks future consolidation for that package/branch until
someone manually HDELs the field. The PR's "Known limitation" section calls out the 24h Jira threshold false-positive risk but doesn't mention this — and this one is arguably worse (silent, permanent, no
self-healing), not just a threshold tuning problem. Worth a callout/follow-up ticket at minimum.
Hard for me to say if Sonnet is correct here.
Pods currently have no SIGTERM handler, so a redeployment silently drops whatever task was popped off a Redis queue but not yet finished processing. Track each in-flight task's (source_queue, payload); on shutdown, stop pulling new work, cancel what's still running, and RPUSH the original payloads back so nothing is lost. There's no grace period — task processing runs for minutes to hours (e.g. build polling), so waiting for it to finish naturally would rarely succeed and only delays recovery. Also races the semaphore acquire (not just the poll) against shutdown: without it, once every max_concurrent slot is held by a long-running task, sem.acquire() for the next iteration blocks forever waiting for a slot that never frees up naturally, and the loop never notices shutdown at all — exactly the scenario this exists to handle. Abandons (rather than cancels) an in-flight BRPOP/poll_fn call on shutdown, since redis-py never resets a connection on CancelledError and cancelling mid-read risks a stale response corrupting the next command drawn from the same pool (e.g. our own re-push RPUSH calls). Assisted-by: Claude (Cursor)
Each agent's queue-mode main() now creates a shutdown_event, installs the signal handler, and passes it through to run_task_loop so redeployments stop pulling new work and re-push whatever was in flight back to Redis instead of dropping it. mr_consolidation_agent's job queue is a Redis Hash (pick_next_job / complete_job), not a list run_task_loop can RPUSH back into, so re-push doesn't apply there — its poll_fn now returns a sentinel source_queue just to satisfy run_task_loop's generic bookkeeping. A cancelled mid-flight job is dropped the same way process_task's own finally already drops any other failure via complete_job; this is a pre-existing gap, not a regression, and is out of scope here. Assisted-by: Claude (Cursor)
SIGTERM handling (previous commits) recovers planned redeployments instantly, but a hard crash (SIGKILL, OOM, node failure) still leaves an issue's in-flight label (ymir_triage_in_progress / ymir_triaged_backport / _rebase / _rebuild) stuck forever, since the fetcher currently treats any Ymir label besides ymir_retry_needed as "already being handled" and skips the issue on every sweep. Add "updated" to the existing bulk Jira search fields (free — same paginated response already being fetched, no extra API calls) and use it to detect an in-flight label that's had no update in STALE_LABEL_THRESHOLD_HOURS (default 24h, conservative pending real task duration data from Phoenix traces) with no other Ymir label alongside it — the same "no terminal outcome" signal triage_agent's own dedup check already uses. Recovery flips the stuck label to ymir_retry_needed and reuses the existing retry-needed re-triage path, since the original Redis task payload is unrecoverable by this point and re-triage is the only generically correct fallback. Assisted-by: Claude (Cursor)
_race_shutdown abandons an in-flight BRPOP/poll_fn call on shutdown rather than cancelling it (redis-py never resets a connection on CancelledError, so cancelling mid-read risks corrupting the next command drawn from the pool). But it never accounted for that abandoned call's eventual result: if it resolved with a real task after shutdown had already started winding down, that task was popped from Redis and then silently dropped - never processed, never re-pushed. Have _race_shutdown hand the abandoned task back to the caller instead of firing-and-forgetting it, and resolve it in run_task_loop's shutdown tail before returning: await it (bounded by poll_timeout, the same cap BRPOP itself already uses, so this isn't a grace period for task processing) and RPUSH its result back if it turns out to be a real task rather than a natural timeout. Bump terminationGracePeriodSeconds from 30s to 45s for the affected agent deployments to give this bounded wait room within the shutdown window, alongside the (near-instant) active-task cancel-and-repush. Assisted-by: Claude (Cursor)
The stale-label recovery path flipped an abandoned-looking in-flight label straight to ymir_retry_needed and added the issue to remove_issues_for_retry, which bypasses the existing_keys dedup check in the push loop below - the same check every other path relies on to avoid double-queuing an issue. If the label only looked abandoned because its task is still legitimately sitting in a live Redis queue (the SIGTERM handler already re-pushed it, or a downstream queue is simply backed up - ymir_triaged_backport et al. persist for as long as the task waits its turn, not just during a crash window), this started a second, concurrent agent run on top of the one already queued. Check existing_keys before treating the label as recoverable: if the issue's payload is still found in a live queue, leave the label alone and let it drain naturally instead of flipping it. Assisted-by: Claude (Cursor)
- Wrap _race_shutdown's asyncio.wait in try/except CancelledError so both internal tasks are cleaned up if the caller is itself cancelled - Wrap each individual RPUSH in try/except during shutdown so a transient Redis failure for one task doesn't abort the rest - Move orphan-poll RPUSH into the try body (was in else, which isn't covered by the preceding except) - Filter already-done tasks out of the repush snapshot to avoid duplicating work that already ran Assisted-by: Claude (Cursor)
Before this PR, SIGTERM killed the process outright so the finally block never ran. Now that the shutdown handler delivers a real CancelledError, the finally: complete_job() call would silently HDEL the :active hash entry, losing the job. Catch CancelledError and re-raise without cleanup so the entry stays in :active (matching pre-PR behavior). A proper requeue/sweep is follow-up work. Assisted-by: Claude (Cursor)
6fdc9a4 to
b3303d9
Compare
I've added a commit which fixes the issue pointed out. but there is still one issue and that is the mr_consolidation tasks are not requed, they are just thrown away. the architecture of mr_consolidation differs to the other agents. I'm not sure if I want to expand the scope of this PR, so maybe I'll create a follow-up ticket for it? wdyt? |
When a consolidation task is cancelled by shutdown, the :active hash field is left in place to avoid silent data loss. But pick_next_job refuses to promote :pending while :active exists for the same package/branch, so without cleanup a redeploy permanently blocks future consolidation for that pair. Add sweep_stale_active_jobs() to merge_queue.py: on every poll cycle it scans :active entries and removes any whose activated_at is older than a configurable threshold (default 6h, env STALE_ACTIVE_THRESHOLD_HOURS). Staleness is measured from activated_at (set by pick_next_job at promotion time), not submitted_at (set at initial queueing), to avoid falsely sweeping jobs that waited in :pending behind a backlog. Deletion uses an atomic compare-and-delete Lua script so a concurrent complete_job + pick_next_job can't cause the sweep to remove a fresh entry. Assisted-by: Claude (Cursor)
b3303d9 to
4c24c9f
Compare
TomasTomecek
left a comment
TomasTomecek
left a comment
There was a problem hiding this comment.
LGTM, thanks!
agreed with opening a followup
2 participants
Pods currently have no SIGTERM handler, so a redeployment silently drops whatever task was mid-processing. Separately, a hard crash (OOM, SIGKILL, node failure) leaves an issue's in-flight Jira label stuck forever, since the fetcher treats any Ymir label besides
ymir_retry_neededas "already being handled." This PR adds two safety nets to cover both cases.Cooperative SIGTERM shutdown (
ymir/common/base_utils.py)run_task_loopnow accepts ashutdown_event. On shutdown it stops pulling new tasks, cancels whatever's still in flight, andRPUSHes the original(source_queue, payload)back to Redis so nothing is lost. No grace period — tasks run for minutes to hours, so waiting for them to finish naturally would rarely succeed and only delays recovery.max_concurrentslot is held by a long-running task, the loop would never notice shutdown fired.BRPOP/poll_fncall already in flight when shutdown fires is abandoned rather than cancelled (cancelling mid-read risks corrupting the connection for the next command drawn from the pool), but its eventual result is still awaited and re-pushed if it turns out to be a real task.triage_agent,backport_agent,rebase_agent,rebuild_agent,mr_consolidation_agent.terminationGracePeriodSecondsbumped 30s → 45s on the affected deployments to give this room.Stale in-flight-label safety net (
ymir/jira_issue_fetcher/jira_issue_fetcher.py)updatedto the existing bulk Jira search fields (free — no extra API calls).STALE_LABEL_THRESHOLD_HOURS(default 24h) and no other Ymir label alongside it is treated as abandoned, flipped toymir_retry_needed, and re-triaged from scratch.Known limitation
The 24h threshold can still false-positive for backport/rebase/rebuild tasks that are actively processing (not queued) for longer than that — e.g.
COPR_BUILD_TIMEOUT(3h) ×max_build_attempts(10 default) means a single legitimate task can run 30+ hours. Recommend raising the threshold (or adding a Redis heartbeat) as a follow-up before relying on this safety net for those stages.