[pradithya]

What

Locks whose holding PR is already merged/closed are now reclaimed on the next plan instead of blocking until the 7-day TTL.

Why

Locks are released by the closed pull_request webhook. That is the only release path, so a single lost delivery strands the lock permanently.

This happened on pradithya/argocd-gitops. The worker crash-looped on a Redis dial failure (connect: operation not permitted) across the window in which #45, #46 and #48 were merged. Their closed deliveries never got processed — the surviving asynq task set contains opened/synchronize for those PRs but no closed — so 16 applications stayed locked by merged PRs.

Every plan afterwards reported conflicts against PRs that no longer existed:

### Application: `seaweedfs`
⚠️ Locked by PR #45 (pradithya)

and lemuria sync then answered ⚠️ No applications are locked by this PR — plan had acquired nothing, so there was nothing to sync. The two messages read as contradictory and the PR was stuck with no self-service way out short of lemuria unlock on each merged PR.

Fix

1. Self-healing lock acquisition (internal/commands/plan.go)

On conflict, verify the holder via vcs.GetPR before reporting it. If that PR is closed, the lock is orphaned — force-unlock and retry once. Fail-safe in every uncertain case (nil holder, malformed repo, API error, failed release): keep the lock and report the conflict, because refusing to plan is always safer than stealing a lock from a genuinely active PR.

This complements rather than replaces the webhook: the webhook stays the fast path, and this is the reconciliation backstop for when it is missed.

2. Renderer no longer swallows the conflict (pkg/diff/renderer.go)

The new/deleted early-returns ran before the lock-status check. A deleted app that rendered no diff took the early return and printed:

### Application: `dev-platform-kyverno` 🗑️
➖ **Application will be deleted** when the Application CR is removed

with no indication that the lock was held by another PR and nothing had been planned. 9 of the 13 apps in the affected PR rendered exactly this. Hoisted the foreign-lock check above both branches and extracted it into isForeignLock.

Tests

• internal/commands/stale_lock_test.go — reclaim on merged holder, reclaim on closed-unmerged holder, no reclaim on open holder, no reclaim on PR-lookup error, conflict preserved when force-unlock fails, guards for nil/empty/malformed repo, plus splitRepoFullName table.
• pkg/diff/renderer_test.go — foreign lock reported for deleted and new apps, own lock still renders the deletion notice, isForeignLock table.

go build ./..., go vet ./... and the full go test ./internal/... ./pkg/... suite pass.

Verification against the live incident

Released the stale locks via lemuria unlock on the merged PRs, then re-ran lemuria plan on the blocked PR — it acquired all 13 locks and rendered a correct plan. With this change that manual step is no longer needed.

[codecov]

Codecov Report

❌ Patch coverage is 94.54545% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.14%. Comparing base (a5f9645) to head (5aad2ac).

Files with missing lines	Patch %	Lines
internal/commands/plan.go	93.75%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
+ Coverage   80.05%   80.14%   +0.09%     
==========================================
  Files          58       58              
  Lines        7686     7738      +52     
==========================================
+ Hits         6153     6202      +49     
- Misses       1190     1192       +2     
- Partials      343      344       +1     

Flag	Coverage Δ
unit	80.14% <94.54%> (+0.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

This PR adds a reconciliation backstop for Redis application locks that were stranded due to missed PR-closed webhooks, and fixes plan rendering so foreign-lock conflicts aren’t silently omitted for new/deleted apps that render no diffs.

Changes:

• Add stale-lock reclamation during plan lock acquisition by verifying the holding PR state via VCS and force-unlocking when the holder PR is closed.
• Hoist and refactor “foreign lock” detection in the plan renderer so lock conflicts are surfaced before new/deleted early-returns.
• Add focused unit tests covering stale-lock reclaim behavior and renderer foreign-lock behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
internal/commands/plan.go	Adds stale-lock reclamation during lock acquisition (VCS PR state check + force-unlock + retry).
internal/commands/stale_lock_test.go	New unit tests for stale-lock reclaim behavior and guard cases.
pkg/diff/renderer.go	Moves foreign-lock reporting ahead of new/deleted early returns; extracts isForeignLock.
pkg/diff/renderer_test.go	Adds tests ensuring foreign locks are still reported for new/deleted apps and validates isForeignLock.

Suppressed comments (2)

internal/commands/plan.go:521

• The docstring says "cross-repo holder" is treated as an uncertainty that prevents reclaim, but reclaimStaleLock currently doesn’t check for cross-repo holders (it just validates held.Repo is parseable). This comment is misleading; update it to match the actual guards (nil holder / malformed repo / PR lookup error / failed unlock).

// Returns true if the lock was released and the caller should retry.
// On any uncertainty (nil holder, cross-repo holder, API error) it returns
// false and leaves the lock intact — refusing to plan is always safer than
// stealing a lock from a genuinely active PR.

internal/commands/plan.go:559

• There’s a race window between verifying the holder PR is closed and calling ForceUnlock: the lock could be released (e.g. webhook) and re-acquired by a different PR, and ForceUnlock would then delete the new lock. Consider a best-effort guard by reading the current lock and confirming it’s still held by the same repo/PR before force-unlocking.

	if err := e.lock.ForceUnlock(ctx, application); err != nil {
		slog.Warn("failed to release stale lock",
			"app", application,
			"holder_pr", held.PRNumber,
			"error", err,
		)
		return false
	}

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

internal/commands/plan.go:525

• The doc comment says reclaimStaleLock returns false on a “cross-repo holder”, but the function does not (and cannot, given its current parameters) check whether the holder’s repo matches the current PR’s repo. This is misleading for future maintainers; either implement an explicit cross-repo check (by passing the current repo into the function) or adjust the comment to match the actual guard conditions.

// Returns true if the lock was released and the caller should retry.
// On any uncertainty (nil holder, cross-repo holder, API error) it returns
// false and leaves the lock intact — refusing to plan is always safer than
// stealing a lock from a genuinely active PR.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated no new comments.

Files not reviewed (1)

• web/package-lock.json: Generated file

Suppressed comments (3)

internal/commands/plan.go:525

• The function comment claims cross-repo holders are treated as an uncertainty, but the implementation does not check for cross-repo and will attempt to verify/reclaim locks regardless of repo. Please align the comment with the actual fail-safe conditions to avoid misleading future maintenance.

// Returns true if the lock was released and the caller should retry.
// On any uncertainty (nil holder, cross-repo holder, API error) it returns
// false and leaves the lock intact — refusing to plan is always safer than
// stealing a lock from a genuinely active PR.

internal/commands/plan.go:569

• splitRepoFullName’s comment says it splits an "owner/repo" identifier, but the function intentionally splits only on the first slash and allows additional slashes in the repo portion (as covered by the new tests). Clarifying this prevents incorrect reuse of the helper.

// splitRepoFullName splits an "owner/repo" identifier into its parts.
func splitRepoFullName(fullName string) (owner, repo string, ok bool) {

web/package-lock.json:6

• This PR’s description and code changes are focused on stale lock reclamation in the Go backend, but it also includes a full regeneration of web/package-lock.json (including a lockfileVersion bump). If this is not intentional, it should be reverted or moved to a separate PR to avoid unrelated dependency churn and noisy diffs.