Add per-issue Redis locks for distributed agent dedup

[nforro]

• Introduce a per-issue Redis lock module using SET NX PX with a heartbeat loop and Lua-based release/extend, so two workers never process the same Jira issue concurrently
• Wire the lock into the triage agent (both task processing and fetcher-side pre-check) and into the rebase, backport, and rebuild agents, each with its own lock prefix
• Lower default poll_timeout from 30s to 5s for faster graceful shutdown across all agents
• Update all 6 agent deployments: terminationGracePeriodSeconds: 45 → 90, strategy: Recreate → RollingUpdate (maxSurge: 1, maxUnavailable: 0)

[qodo-for-packit]

PR Summary by Qodo

Add per-issue Redis locks to deduplicate distributed agent work

🐞 Bug fix ✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Add a Redis-backed per-issue lock with heartbeat to prevent concurrent Jira processing.
• Wire locking into triage (worker + fetcher pre-check) and rebase/backport/rebuild with distinct
 prefixes.
• Improve shutdown and rollout behavior via shorter BRPOP polling and updated OpenShift deployment
 strategy.

Diagram

graph TD
  openshift["OpenShift Deploy"] --> fetcher["Jira Fetcher"] --> jira{{"Jira API"}}
  fetcher --> redis[("Redis")] --> triage["Triage Agent"] --> jira
  triage --> lockmod["issue_lock.py"] --> redis
  openshift --> other["Rebase/Backport/Rebuild"] --> lockmod

  subgraph Legend
    direction LR
    _svc["Service/Module"] ~~~ _db[("Redis")] ~~~ _ext{{"External"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Per-issue EXISTS/MGET check instead of SCAN in fetcher

• ➕ Avoids keyspace scanning and scales better with many lock keys
• ➕ Naturally bounded by the number of candidate Jira issues in a sweep
• ➖ Requires additional per-issue Redis calls (best done via pipeline)
• ➖ Slightly more code complexity than a single SCAN loop

2. Use Redis Streams consumer groups for queue-level dedup

• ➕ Built-in consumer group semantics reduce duplicate delivery
• ➕ More observable processing state via stream pending entries
• ➖ Larger architectural change vs current list-based queues
• ➖ Still doesn’t fully prevent concurrent work across multiple queues/agents without per-issue coordination

3. Rely solely on Jira labels as the dedup anchor

• ➕ No Redis lock lifecycle to maintain
• ➕ Single source of truth for in-progress/terminal state
• ➖ Higher TOCTOU risk (enqueue/consume races) and slower coordination
• ➖ More Jira write/read load and weaker guarantees during retries/outages

Recommendation: The PR’s approach (tokened Redis lock with TTL + heartbeat + atomic Lua release/extend) is a standard, low-latency way to prevent concurrent processing and fits the existing Redis-centric architecture. The main potential follow-up is optimizing the fetcher’s lock avoidance: SCAN is acceptable at current scale, but a pipelined EXISTS/MGET for just the candidate issues can be more predictable if lock key volume grows.

Files changed (17) +651 / -68

Enhancement (1) +170 / -0

issue_lock.pyAdd Redis per-issue lock primitive with TTL heartbeat and safe release +170/-0

Add Redis per-issue lock primitive with TTL heartbeat and safe release

• Introduces a new lock module using SET NX PX acquisition with a unique token per holder. Adds Lua-based compare-and-delete for release and compare-and-PEXPIRE for extension, plus a heartbeat loop at TTL/3; the context manager yields None when already locked to support guard-clause callers.

ymir/common/issue_lock.py

Bug fix (5) +66 / -40

backport_agent.pyGuard backport processing with per-issue Redis lock +11/-0

Guard backport processing with per-issue Redis lock

• Wraps backport task execution in an async issue_lock context using the lock:backport: prefix. Drops duplicate tasks when another worker already holds the lock and refactors the main logic into a locked helper function.

ymir/agents/backport_agent.py

rebase_agent.pyGuard rebase processing with per-issue Redis lock +11/-0

Guard rebase processing with per-issue Redis lock

• Wraps rebase task execution in an async issue_lock context using the lock:rebase: prefix. Drops duplicate tasks when the lock is already held and moves core processing into a helper executed only under lock ownership.

ymir/agents/rebase_agent.py

rebuild_agent.pyGuard rebuild processing with per-issue Redis lock +15/-3

Guard rebuild processing with per-issue Redis lock

• Adds issue_lock usage with the lock:rebuild: prefix to ensure only one rebuild worker processes a Jira issue at a time. Refactors variable extraction and main logic into a locked helper to avoid TOCTOU around parsing vs execution.

ymir/agents/rebuild_agent.py

triage_agent.pyAcquire per-issue lock before triage execution +12/-37

Acquire per-issue lock before triage execution

• Adds issue_lock acquisition around triage processing to prevent two pods from working the same Jira issue concurrently. Refactors the main processing body into a locked helper and adjusts comments around dedup/label semantics to reflect the new primary guard.

ymir/agents/triage_agent.py

jira_issue_fetcher.pySkip enqueueing triage tasks for issues with active triage locks +17/-0

Skip enqueueing triage tasks for issues with active triage locks

• Imports the triage lock prefix and adds a Redis SCAN-based helper to enumerate currently locked issues. Merges locked keys into the existing de-dup set before enqueuing, reducing duplicate triage scheduling during concurrent worker activity.

ymir/jira_issue_fetcher/jira_issue_fetcher.py

Tests (2) +371 / -0

test_triage_agent.pyAdd triage agent unit coverage for lock-held vs lock-acquired paths +61/-0

Add triage agent unit coverage for lock-held vs lock-acquired paths

• Introduces async context manager helpers to simulate lock acquisition outcomes. Adds tests asserting that triage drops duplicates when locked and proceeds to run_workflow when the lock is acquired, while patching issue_lock in existing tests.

ymir/agents/tests/unit/test_triage_agent.py

test_issue_lock.pyAdd unit tests for lock acquire/release/extend, heartbeat, and context behavior +310/-0

Add unit tests for lock acquire/release/extend, heartbeat, and context behavior

• Adds a FakeRedis test double that simulates the required Redis commands and Lua scripts. Covers success/failure paths for acquire/release/extend, heartbeat behavior under lock loss and transient Redis errors, cancellation cleanup, and prefix isolation semantics.

ymir/common/tests/unit/test_issue_lock.py

Other (9) +44 / -28

.secrets.baselineRefresh secrets baseline metadata after manifest line shifts +8/-8

Refresh secrets baseline metadata after manifest line shifts

• Updates stored line numbers for existing findings due to YAML edits and refreshes the generated timestamp. No new secrets are introduced; this is bookkeeping to keep detection stable.

.secrets.baseline

deployment-backport-agent-c10s.ymlSwitch backport agent rollout to RollingUpdate and extend grace period +5/-3

Switch backport agent rollout to RollingUpdate and extend grace period

• Changes deployment strategy from Recreate to RollingUpdate with maxSurge=1/maxUnavailable=0. Increases terminationGracePeriodSeconds from 45s to 90s to allow cleaner shutdown.

openshift/deployment-backport-agent-c10s.yml

deployment-backport-agent-c9s.ymlSwitch backport agent rollout to RollingUpdate and extend grace period +5/-3

Switch backport agent rollout to RollingUpdate and extend grace period

• Changes deployment strategy from Recreate to RollingUpdate with maxSurge=1/maxUnavailable=0. Increases terminationGracePeriodSeconds from 45s to 90s to allow cleaner shutdown.

openshift/deployment-backport-agent-c9s.yml

deployment-rebase-agent-c10s.ymlSwitch rebase agent rollout to RollingUpdate and extend grace period +5/-3

Switch rebase agent rollout to RollingUpdate and extend grace period

• Changes deployment strategy from Recreate to RollingUpdate with maxSurge=1/maxUnavailable=0. Increases terminationGracePeriodSeconds from 45s to 90s to reduce forced termination during blocking polls.

openshift/deployment-rebase-agent-c10s.yml

deployment-rebase-agent-c9s.ymlSwitch rebase agent rollout to RollingUpdate and extend grace period +5/-3

Switch rebase agent rollout to RollingUpdate and extend grace period

• Changes deployment strategy from Recreate to RollingUpdate with maxSurge=1/maxUnavailable=0. Increases terminationGracePeriodSeconds from 45s to 90s to reduce forced termination during blocking polls.

openshift/deployment-rebase-agent-c9s.yml

deployment-rebuild-agent-c10s.ymlSwitch rebuild agent rollout to RollingUpdate and extend grace period +5/-2

Switch rebuild agent rollout to RollingUpdate and extend grace period

• Changes deployment strategy from Recreate to RollingUpdate with maxSurge=1/maxUnavailable=0. Increases terminationGracePeriodSeconds from 45s to 90s to allow in-flight work to stop more gracefully.

openshift/deployment-rebuild-agent-c10s.yml

deployment-rebuild-agent-c9s.ymlSwitch rebuild agent rollout to RollingUpdate and extend grace period +5/-2

Switch rebuild agent rollout to RollingUpdate and extend grace period

• Changes deployment strategy from Recreate to RollingUpdate with maxSurge=1/maxUnavailable=0. Increases terminationGracePeriodSeconds from 45s to 90s to allow in-flight work to stop more gracefully.

openshift/deployment-rebuild-agent-c9s.yml

deployment-triage-agent.ymlSwitch triage agent rollout to RollingUpdate and extend grace period +5/-3

Switch triage agent rollout to RollingUpdate and extend grace period

• Changes deployment strategy from Recreate to RollingUpdate with maxSurge=1/maxUnavailable=0. Increases terminationGracePeriodSeconds from 45s to 90s to better match reduced poll timeouts and shutdown handling.

openshift/deployment-triage-agent.yml

base_utils.pyReduce task loop Redis poll timeout for faster shutdown +1/-1

Reduce task loop Redis poll timeout for faster shutdown

• Lowers run_task_loop default poll_timeout from 30s to 5s, reducing worst-case delay before observing shutdown events when using blocking BRPOP. This aligns better with extended termination grace periods and improves responsiveness across agents.

ymir/common/base_utils.py

[qodo-for-packit]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 7 rules

1. Locked skip strands trigger labels ✓ Resolved 🐞 Bug ≡ Correctness

Description

In jira_issue_fetcher.push_issues_to_queue(), adding locked issue keys into existing_keys causes the
enqueue loop to skip locked issues before the ymir_todo/ymir_retry_needed consume-and-flip logic
runs. This can leave trigger labels present while the issue later gains other ymir_* labels, after
which future sweeps mark it as existing and never consume/enqueue the requested rerun.

Code

ymir/jira_issue_fetcher/jira_issue_fetcher.py[R550-553]

+            locked_keys = await self._get_locked_issue_keys(redis_conn)
+            existing_keys |= locked_keys
+            if locked_keys:
+                logger.info("Found %d locked issues, will skip them", len(locked_keys))

Relevance

●●● Strong

Repo has repeatedly fixed dedup/label-race bugs in fetcher/triage flows (e.g., PR #459, PR #540).

PR-#459
PR-#540

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR merges locked_keys into existing_keys, and the enqueue loop skips any issue in
existing_keys before reaching the label consumption block. Separately, issues with any ymir_*
labels (including ymir_todo) are marked as existing, so a stuck trigger label can persist without
ever being consumed.

ymir/jira_issue_fetcher/jira_issue_fetcher.py[527-554]
ymir/jira_issue_fetcher/jira_issue_fetcher.py[659-676]
ymir/jira_issue_fetcher/jira_issue_fetcher.py[687-705]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`push_issues_to_queue()` currently unions `locked_keys` into `existing_keys` early, which makes the main enqueue loop short-circuit for locked issues *before* the code that consumes trigger labels (`ymir_todo` / `ymir_retry_needed`) and flips them to `ymir_triage_in_progress`.

This leaves trigger labels unconsumed during the locked window, and once the issue accumulates other `ymir_*` labels it is classified as “existing” and will be skipped on subsequent sweeps, effectively making the maintainer trigger a no-op.

### Issue Context
You want: (a) do not enqueue a second triage task while a lock is held, but (b) still consume/flip the trigger label so it doesn’t remain stuck and misleading.

### Fix Focus Areas
- ymir/jira_issue_fetcher/jira_issue_fetcher.py[527-737]

### Concrete fix options
1) **Reorder logic:** compute `label_to_consume` and perform the Jira label flip *before* the `if issue_key in existing_keys ... continue` check.
2) **Special-case locked issues with triggers:** if `issue_key` is locked and has `ymir_todo`/`ymir_retry_needed`, perform the flip (consume trigger) but **do not** push to Redis.
3) **Don’t merge `locked_keys` into `existing_keys`:** instead check `if issue_key in locked_keys` only at the point of deciding to enqueue, after trigger consumption has run.

Add/adjust unit tests to cover: locked + ymir_todo and locked + ymir_retry_needed => trigger label is consumed but no Redis push occurs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Inconsistent issue-key canonicalization ✓ Resolved 🐞 Bug ☼ Reliability

Description

Queue-derived existing issue keys are uppercased, but lock keys are constructed and decoded without
case normalization. If any producer supplies a non-canonical (lower/mixed-case) Jira key, the
fetcher may fail to recognize an active lock for the same logical issue and enqueue duplicate work.

Code

ymir/common/issue_lock.py[R42-43]

+def _lock_key(issue_key: str, prefix: str = LOCK_KEY_PREFIX) -> str:
+    return f"{prefix}{issue_key}"

Relevance

●●● Strong

Team previously standardized Jira issue key casing (uppercasing) to ensure reliable
dedup/comparisons in PR #129.

PR-#129

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The fetcher normalizes existing queue-derived issue keys to uppercase, while lock discovery returns
decoded keys without .upper(). The lock module constructs Redis key names from the issue key
verbatim, so any case mismatch persists into Redis and breaks equality checks.

ymir/jira_issue_fetcher/jira_issue_fetcher.py[481-506]
ymir/jira_issue_fetcher/jira_issue_fetcher.py[527-537]
ymir/common/issue_lock.py[42-44]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The system currently mixes canonicalization strategies:
- Existing queue items are normalized to `issue_key.upper()`.
- Redis lock keys are created from `issue_key` verbatim and decoded verbatim.

This makes lock-based dedup fragile if an issue key ever enters the system in non-uppercase form.

### Issue Context
Even if Jira keys are *usually* uppercase, canonicalization should be enforced at the boundaries (lock creation + lock discovery) to ensure dedup logic is robust and future-proof.

### Fix Focus Areas
- ymir/common/issue_lock.py[42-60]
- ymir/jira_issue_fetcher/jira_issue_fetcher.py[527-537]

### Suggested fix
- Canonicalize in one place and apply everywhere, e.g.:
 - In `_lock_key(...)`: use `issue_key = issue_key.upper()` before formatting.
 - In `_get_locked_issue_keys(...)`: after decoding, also `.upper()`.
 - (Optional) enforce uppercase in the Pydantic model for triage input / task creation to make the invariant explicit.

Update/extend tests to ensure lock acquisition + fetcher discovery work when given lowercase inputs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. rebase-agent-c10s uses RollingUpdate ✗ Dismissed 📘 Rule violation ☼ Reliability

Description

Several single-replica Deployments (rebase-agent-c10s, rebase-agent-c9s, rebuild-agent-c10s,
rebuild-agent-c9s, and triage-agent) are configured with replicas: 1 but use `strategy.type:
RollingUpdate`. This violates the single-replica rollout requirement and may result in downtime or
concurrent processing during updates.

Code

openshift/deployment-rebase-agent-c10s.yml[R13-16]

+    type: RollingUpdate
+    rollingUpdate:
+      maxSurge: 1
+      maxUnavailable: 0

Relevance

●●● Strong

PR #487 explicitly switched single-replica deployments from RollingUpdate to Recreate to avoid
deadlocks/downtime.

PR-#487

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1592 requires Deployments fixed at replicas: 1 to use strategy.type: Recreate.
The referenced updated manifests show the combination of replicas: 1 alongside `strategy.type:
RollingUpdate` for these Deployments, directly contradicting the compliance requirement and
demonstrating the misconfiguration.

Rule 1592: Single-replica OpenShift Deployments Must Use Recreate Strategy
openshift/deployment-rebase-agent-c10s.yml[5-16]
openshift/deployment-rebase-agent-c9s.yml[5-16]
openshift/deployment-rebuild-agent-c10s.yml[5-16]
openshift/deployment-rebuild-agent-c9s.yml[5-16]
openshift/deployment-triage-agent.yml[5-16]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Multiple OpenShift Deployment manifests configure a single replica (`replicas: 1`) while using `strategy.type: RollingUpdate`, which is disallowed for single-replica workloads.

## Issue Context
PR Compliance ID 1592 requires `strategy.type: Recreate` when a Deployment is fixed at a single replica to avoid downtime or concurrent processing during updates.

## Fix Focus Areas
- openshift/deployment-rebase-agent-c10s.yml[7-16]
- openshift/deployment-rebase-agent-c9s.yml[7-16]
- openshift/deployment-rebuild-agent-c10s.yml[7-16]
- openshift/deployment-rebuild-agent-c9s.yml[7-16]
- openshift/deployment-triage-agent.yml[7-16]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Lock expiry permits duplicate work ✗ Dismissed 🐞 Bug ☼ Reliability

Description

If the heartbeat can’t refresh the lock and ownership is lost, issue_lock stops heartbeating but
intentionally does not stop the running task, allowing another worker with the same lock prefix to
acquire the lock and run concurrently. This is especially risky for backport/rebase/rebuild workers
because the codebase only defines a durable Jira “in progress” label for triage, not for those
downstream operations.

Code

ymir/common/issue_lock.py[R109-114]

+            extended = await extend_issue_lock(redis_conn, issue_key, token, ttl_ms, prefix)
+            if not extended:
+                # Lock expired (Redis outage > TTL) and was re-acquired
+                # by another worker.  We stop heartbeating but do NOT
+                # cancel the task — see issue_lock() docstring.
+                logger.error(

Relevance

●● Moderate

No clear prior decision on cancelling work when Redis lock ownership is lost; PR #675 shows mixed
tradeoffs around cancellation.

PR-#675

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The heartbeat loop exits (without cancelling work) when extend_issue_lock returns false. The lock
module’s docstring names a triage-specific Jira label as the fallback, and the JiraLabels enum only
includes an in-progress label for triage, not for downstream operations.

ymir/common/issue_lock.py[105-120]
ymir/common/issue_lock.py[142-147]
ymir/common/constants.py[137-156]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`issue_lock()` intentionally allows work to continue after lock ownership is lost (e.g., Redis outage > TTL). This can re-enable concurrent same-operation processing once another worker acquires the lock.

### Issue Context
The docstring cites Jira’s triage in-progress label as a fallback dedup anchor, but downstream agents (backport/rebase/rebuild) do not have an equivalent durable in-progress label in the label set.

### Fix Focus Areas
- ymir/common/issue_lock.py[98-170]
- ymir/agents/backport_agent.py[873-886]
- ymir/agents/rebase_agent.py[465-478]
- ymir/agents/rebuild_agent.py[422-430]

### Suggested fix directions (choose one)
1) **Fail fast on lock loss (safer for downstream):** make `_heartbeat_loop` notify the context manager (e.g., via an `asyncio.Event`) and have `issue_lock()` raise an exception or cancel the protected block when ownership is lost.
2) **Per-agent durable Jira anchors:** introduce and write per-agent `*_in_progress` Jira labels (similar to `ymir_triage_in_progress`) so the system has a non-Redis dedup anchor during long outages.
3) **Increase TTL + add idempotency checks:** if you keep the current tradeoff, raise TTL substantially and add explicit idempotency/"already done" checks in downstream workflows before performing side effects.

Add a unit test that simulates `extend_issue_lock()` returning False and asserts the chosen behavior (abort vs continue).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit ccd095a

[majamassarini]

LGTM! I just don't understand the comment removal in triage_agent.py code. But I suppose you didn't like them.

[nforro]

I just don't understand the comment removal in triage_agent.py code. But I suppose you didn't like them.

Sorry, I thought they are no longer relevant and that's why Claude removed them. Reverted.

[jpodivin]

Why the comment removals, aren't they valid any more?

[jpodivin]

I've just notice the other review.