Repository navigation
Conversation
`lemuria unlock` reported "No applications are locked by this PR" on a PR whose applications were demonstrably still locked, leaving them held and blocking every other PR until the 7-day lock TTL expired. ListByPR resolves a PR's locks solely through the `lemuria:pr-locks:` index set. That index was given a TTL once, on first acquisition, and never refreshed. The lock keys themselves are rewritten on every replan, which resets their TTL. A PR that stays open and replans therefore keeps its locks alive indefinitely while the index counts down from the original acquisition and eventually expires — after which the locks are orphaned: still held, still blocking, but invisible to unlock, sync and rollback alike. Observed on argocd-gitops#49: three live lock keys, no index key, and two `lemuria unlock` comments that both no-oped. Refresh the index TTL wherever the lock is written, via a single indexLock helper, and give the index a longer TTL than the locks it points at so it can never expire first. The Expire error is no longer discarded. Repairing the write path alone would strand locks already orphaned, so treat the index as a fast path rather than the source of truth: when it comes back missing entries, SCAN the lock keys for that PR, fold what turns up back into the index, and log it. A lock the index has lost is precisely the lock that most needs releasing. Tests reproduce the production sequence — acquire, replan at day 6, expire the index at day 8 — and cover the recovery and TTL-refresh paths.
There was a problem hiding this comment.
🟡 Changes recommended
ListByPR currently performs a full Redis SCAN on every call, which can introduce significant operational load and should be gated to the “index missing” scenario (or another narrow trigger) before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes Redis lock “index drift” where the PR→locks index (lemuria:pr-locks:<repo>:<pr>) could expire while the underlying lock keys were kept alive by replans, making locks invisible to unlock/sync/rollback even though they still blocked other PRs.
Changes:
- Refreshes the PR index TTL on every lock write via a shared
indexLockhelper and sets the index TTL longer than the lock TTL. - Adds a scan-based recovery path in
ListByPRto find locks by PR even if the index is missing, and repairs the index when recovered locks are found. - Adds miniredis-based tests reproducing the production drift sequence and validating recovery + TTL refresh behavior.
File summaries
| File | Description |
|---|---|
| internal/lock/redis.go | Refresh PR index TTL on lock writes; add scan fallback to recover locks when the index is missing. |
| internal/lock/index_drift_test.go | New regression tests reproducing index TTL drift and verifying recovery/refresh behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| recovered, err := m.scanLocksByPR(ctx, repo, prNumber, seen) | ||
| if err != nil { | ||
| // The index results are still usable; degrade rather than fail the command. | ||
| slog.Warn("could not scan for locks missing from the PR index", | ||
| "repo", repo, "pr", prNumber, "error", err) | ||
| return locks, nil | ||
| } | ||
|
|
||
| for _, lock := range recovered { | ||
| slog.Warn("recovered lock missing from PR index", | ||
| "app", lock.Application, "repo", repo, "pr", prNumber) | ||
| if err := m.indexLock(ctx, repo, prNumber, lock.Application); err != nil { | ||
| slog.Warn("could not repair PR index", | ||
| "app", lock.Application, "repo", repo, "pr", prNumber, "error", err) | ||
| } | ||
| locks = append(locks, lock) | ||
| } | ||
|
|
||
| return locks, nil |
|
CI note: all four failing jobs are pre-existing on Backend (
Every other package passed in CI, including the one this PR changes: Frontend ( Happy to fix the embed/CI ordering separately (either commit a |
Follow-up to #86. That fix reclaimed locks whose PR had closed; this one fixes locks that go missing while their PR is still open.
Symptom
On argocd-gitops#49,
lemuria unlockreplied:while the plan comment showed three applications
🔒 Locked by this PR. Both unlock attempts no-oped, and the applications stayed blocked for every other PR.Cause
ListByPRresolves a PR's locks solely through thelemuria:pr-locks:<repo>:<pr>index set. That index is given a TTL once, on first acquisition, and never refreshed. The lock keys themselves are rewritten on every replan, which resets their TTL.So a PR that stays open and replans keeps its locks alive indefinitely, while the index keeps counting down from the original acquisition and eventually expires. After that the locks are orphaned: still held, still blocking, but invisible to
unlock,syncandrollbackalike — until the 7-day lock TTL finally runs out.Confirmed against production Redis: three live lock keys for PR #49 (
vault,external-secrets,traefik-internal-gateway) with ~6h of TTL remaining, and nopr-locks:...:49key.DELon the index returned0, confirming it had already expired. PRs #55/#57 had intact indexes, which is why only #49 misbehaved.This is also why the auto-sync restore path can silently skip applications: it is driven by the same index.
Fix
Stop the drift. Refresh the index TTL wherever the lock is written, through a single
indexLockhelper, and give the index a longer TTL than the locks it points at so it can never expire first. TheExpireerror is no longer discarded.Recover what already drifted. Repairing only the write path would leave existing orphaned locks stuck, so the index is now a fast path rather than the source of truth: when it returns nothing for a PR,
SCANthe lock keys, fold anything found back into the index, and log it. A lock the index has lost is exactly the lock that most needs releasing. A scan failure degrades to the index result instead of failing the command.Tests
internal/lock/index_drift_test.goreproduces the production sequence with miniredisFastForward— acquire, replan at day 6, expire the index at day 8 — and covers the recovery and TTL-refresh paths. All three fail onmain:Full unit suite passes. The pre-existing
e2efailure (401 against a live Argo CD) is unrelated and reproduces on a cleanmain.Note
Production locks for PR #49 were cleared manually to unblock work; no migration is needed, since the scan fallback recovers this state on its own.