Repository navigation
fix(agent-work): narrow stale sweep lease release to the marked claim epoch (#1317) - #1319
Conversation
![its-saffron[bot]](https://avatars.githubusercontent.com/in/3901073?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: glm-5.3-flash (openai) — fallback (primary failed) · pr-reviewer-action v3.6.0
Findings (2 info)
| Severity | Location | Finding |
|---|---|---|
| Info | src/lib/stale-work.ts:178 |
The live-sibling guard defers the lease delete even for an already-expired lease; this is intentional and documented, but a follow-up could release expired leases regardless of siblings. |
| Info | docs/ownership-reconciliation.md:146 |
The label half of the sibling hazard (stale sibling still strips the shared agent/<name> label) remains unfenced and is correctly documented as a gap. |
Recommendation: approve. The diff implements exactly the correlation rule the PR body and issue PR 1317 describe: the sweep now counts live sibling AgentWork rows for the pair (excluding the swept row, active states only) and, absent one, deletes the lease only when expired or renewed (or created) before the row's snapshot lastHeartbeatAt, with strict lt comparisons. The stale-marker commit still precedes the external label release and staleClaimReleasePending retry semantics are unchanged (claim 5 held; the retry test pins identical epoch bounds across passes). Claims 1–4 held against the diff: the sibling guard, expiry branch, pair scoping, no-issue path, and never-renewed createdAt fallback are all exercised by new tests whose harness evaluates the actual where clauses rather than asserting mocks. The conservative tradeoff (a renewed lease surviving up to lease TTL) and the unfenced label half are documented in docs/ownership-reconciliation.md §3.5 and declared out of scope, consistent with the issue's lease-scoped ask. CI on the head commit is green (Tests, Typecheck, Lint, Database integration/migrations, Build).
joryirving
left a comment
joryirving
left a comment
There was a problem hiding this comment.
REQUEST CHANGES on exact head af53f6c. The new lease-delete fencing addresses the narrow claim-epoch bug from #1317: the live-sibling count and conditional renewal/expiry comparisons stop an older AgentWork row from deleting the current same-agent lease. The SQL predicate and its test harness look consistent. All five workflows pass.
Blocking cross-boundary regression: the sweep still removes the current owner's GitHub claim after it intentionally preserves that owner's lease. In src/lib/stale-work.ts, the new transaction can return {marked: true, leasesReleased: 0} because a live sibling owns the current lease or its renewal is newer than the swept stale row. However, sweepStaleWork unconditionally proceeds to releaseIssueClaim({agentName: work.agentName, allowMissingAgent:true, preserveStatus:true}) for the stale row. A newer claim by the same agent uses the same agent/<name> label, so releaseIssueClaim cannot distinguish the old from the live incarnation and strips the live worker's label, even though its valid lease was deliberately retained. This leaves the issue displayed unowned while the new worker still has a lease; rediscovery/reclaim may be blocked until lease TTL. Before this patch the pair-wide delete (incorrectly) cleared the lease along with the label; this patch fixes one side but introduces an especially inconsistent label/lease state.
Please make the external claim-release decision respect the fenced claim epoch and the live-sibling/retained-lease result. At a minimum, do not remove the shared agent label or write claim_released evidence when the sweep has not established authority to release that epoch; account for a new claim between the DB mark and external GitHub write as well. Add an integration regression with an old stale BLOCKED row + fresh same-agent IN_PROGRESS row and renewed lease: sweep must retain both the live lease and GitHub owner label, and must not record the live claim as released. The PR notes this label-side hazard as out of scope, but it is part of the same operation whose lease-release behavior this change alters. I'd resolve both sides before merging.
| } | ||
| } | ||
| return true; | ||
| return { marked: true, leasesReleased: 0 }; |
There was a problem hiding this comment.
This returns a successful stale mark even when no lease was deleted because the current claim belongs to a live sibling/newer epoch. The outer loop still calls releaseIssueClaim unconditionally, removing the same-agent agent/<name> label of that retained live claim, producing unowned GitHub state + a blocking live lease. The label release must be fenced to the same verified epoch, and a two-active-row regression should assert that both lease and label survive.
|
Coordination note from the #1300 run — the two PRs now overlap. PR #1300 (issue #1251) was blocked by a human review on the same hazard this PR addresses, and the fix for it landed there rather than being deferred:
Two things are worth flagging for whoever reconciles them:
The claim-epoch correlation in this PR is the part #1300 does not provide, and it is what closes the residual hole that #1300's own adversarial review flagged: a renewed claim with no live sibling row (a I am not proposing a resolution — merging, closing or reordering these two PRs is the maintainer's call. This note is only so the overlap is not discovered at merge time. |
Resolve sweepStaleWork against the #1251/#1259 rewrite on main. Main's mechanism (per-issue mutation fence + live-sibling deferral + claim-epoch CAS delete on {claimToken, expiredAt, lastRenewalObservedAt}) supersedes this PR's renewedAt-vs-heartbeat heuristic for the same #1317 hazard, so the release logic follows main. Kept from this PR: the leasesReleased report accounting (now counting leases consumed by the CAS-fenced delete), #1317 regression tests retargeted to the merged mechanism, and a docs section 3.5 rewrite describing the fenced design.
Merge conflict resolved (main → branch, cb7a999)
Kept from main (release logic):
Kept from this PR:
The branch's old heuristic tests and Validation (local, cb7a999)
|
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: glm-5.3-flash (openai) — fallback (primary failed) · pr-reviewer-action v3.7.0
Findings (1 minor, 1 info)
| Severity | Location | Finding |
|---|---|---|
| Info | src/lib/stale-work.ts:318 |
The pathological branch reports `leasesDeleted` for a row that is then retained — intentional per the added comment, but worth an operator-docs mention if retention-with-consumption ever shows up in practice. |
| Minor | src/lib/stale-work.ts |
This returns a successful stale mark even when no lease was deleted because the current claim belongs to a live sibling/newer epoch. The outer loop still calls `releaseIssueClaim` unconditionally, removing the same-agent `agent/<name>` label of that retained live claim, producing unowned GitHub st |
Approve. The head (cb7a999) merges main's PR 1300 fenced sweep mechanism and re-points this PR's PR 1317 work at it: leasesReleased accounting over the claim-epoch CAS delete, five new accounting tests, and a rewritten docs §3.5. The human change request's label/lease inconsistency is resolved at head: the merged live-sibling guard defers the entire release ("no lease delete, no label removal, no release", docs/ownership-reconciliation.md), and the new test pins the PR 1317 BLOCKED-sibling scenario with claimRetained: 1, zero delete attempts, and the live epoch's lease intact. Claims: 1–3 and 5 held (tests + docs); claim 4's renewedAt-vs-lastHeartbeatAt heuristic is superseded at head by the CAS delete — superseded by design, not violated. CI on cb7a999 is green (lint, typecheck, tests, migrations).
Unresolved Review Threads
PRRT_kwDOSZt6uM6uYRk1: open — fixed without evidence citing current code
Outstanding Human Change Requests
- @joryirving's change request (af53f6c, head moved since) is not shown addressed at this head; it needs the reviewer's own re-review.
…tention path Address review: the field doc said a retention is 'never' counted, but the claim_epoch_moved path deliberately reports a CAS-consumed deletion when a fresh lease still stands on the pair. Documentation only.
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: glm-5.3-flash (openai) — fallback (primary failed) · pr-reviewer-action v3.7.0
Findings (1 info)
| Severity | Location | Finding |
|---|---|---|
| Info | src/lib/stale-work.ts:339 |
The pathological path (CAS consumed a lease AND a recheck still found one) reports leasesDeleted alongside a retention — intentional and documented, but worth an operator-facing note if it ever fires. |
Recommendation: approve. The human change request (label release stripping a retained live claim) is resolved at head: the merged mechanism's live-sibling deferral now defers entirely — no lease delete, no label removal, no release — documented in docs/ownership-reconciliation.md (§3.5) and pinned by the claimRetained test that asserts leaseDeleteMany is never called and released is 0. The PR 1317 lease-epoch hazard is closed structurally (live-sibling guard + claim-epoch CAS delete), with leasesReleased accounting covered by five new tests. CI (tests, typecheck, lint, build, migrations) is green on the head commit. Claims in the PR body held against the diff and tests; the earlier renewedAt-vs-lastHeartbeatAt heuristic was superseded by the merged CAS mechanism, which the docs now describe. Must-check list is empty.
Outstanding Human Change Requests
- @joryirving's change request (af53f6c, head moved since) judged addressed at this head:
The requested label fencing landed via the #1300 merge (cb7a999): live-sibling deferral now skips releaseIssueClaim entirely (docs/ownership-reconciliation.md §3.5 'no lease delete, no label removal, no release'), and the two-active-row regression the reviewer asked for exists at src/lib/stale-work…
joryirving
left a comment
joryirving
left a comment
There was a problem hiding this comment.
Blocker-only re-review on exact head 3ec6511b7314830d3bb087c034bdfb339fb388e9: APPROVE. The earlier blocker is resolved by the #1300 mechanism now in this branch: a live sibling/advanced claim epoch causes full claim retention, so the sweep does not delete the live lease, strip the shared agent/<name> label, or emit a false release. This PR's remaining delta is primarily leasesReleased accounting over the already-fenced CAS delete plus focused regression coverage and updated ownership docs. The accounting is observational and does not weaken the fencing/release semantics. All exact-head workflows pass.
2 participants
Closes #1317.
What
sweepStaleWorkdeleted the lease by the(agentName, issueId)pair when it marked anAgentWorkrow stale. BecausestartAgentWorkonly releasesCLAIMED/IN_PROGRESSrows, aBLOCKEDsibling row survives a re-claim — so sweeping that stale sibling deleted the lease of a different, live claim epoch of the same agent on the same issue.Correlation rule (decided + documented)
The lease is a singleton per
(agentName, issueId)thatupsertLeaserenews at every claim, and worker heartbeats never touch it — so the lease's last renewal is the fingerprint of the newest claim epoch. The sweep now releases the lease only when:agentWork.countfor the same pair, excluding the swept row, counting onlyCLAIMED/IN_PROGRESS/BLOCKED). A heartbeat can land on a stale sibling after a newer claim renewed the lease, so the epoch rule alone is not enough; with a live sibling present the sweep defers entirely.lastHeartbeatAt(renewedAt, orcreatedAtwhen never renewed) — i.e. it belongs to the marked epoch, not a newer claim.Comparisons are strict
lt: a lease that outlives its epoch is retained until lease TTL reaps it (self-healing), while releasing a live epoch's lease could not self-heal. The snapshotlastHeartbeatAtis stable across a retry of the sameSTALErow, keeping the narrowed delete idempotent. The stale-marker commit still precedes the external label release, andstaleClaimReleasePendingretry semantics are unchanged.StaleWorkReportnow carriesleasesReleasedso operators can see when the narrowed rule deferred a release.Known conservative tradeoff (documented, not a regression of safety)
A lease renewed after the swept row's last heartbeat survives the sweep even if its epoch is actually dead (e.g. a worker that starts work before claiming), leaving the issue lease-blocked for up to the lease TTL (30 min default) while the board already shows it unowned. The old pair-delete had the inverse (worse) failure. Documented in
docs/ownership-reconciliation.md§3.5.Out of scope (follow-ups)
agent/<name>label when a live epoch of the same agent holds the issue (lease half is now fenced). Noted in the docs gap paragraph; the issue's ask is lease-scoped.checkpointAgentWork's unscopedfindFirstcan route a heartbeat to a sibling row (pre-existing; interacts with the above).Validation (local, at af53f6c)
npm run test— 4727 passed, 24 skipped, 1 todo, 0 failures (full suite)npm run typecheck— cleannpm run lint— 0 errors, 1 pre-existing warning in unrelatedsrc/app/login/page.tsxsrc/lib/stale-work.test.ts(9 tests incl. 6 new The stale-work sweep deletes the lease for the whole (agent, issue) pair when any sibling row is stale #1317 cases),agent-work,sweep route,issue-claim,lease— all greenReview
Two independent adversarial review passes (initial + delta re-review): no blockers. Harness-fidelity and coverage gaps flagged in review were fixed in this change (faithful Prisma-semantics lease/count evaluators in the test harness; sibling-guard, expiry-branch, pair-scope, no-issue, and retry-idempotency tests).
CI on this PR is pending at time of writing — not yet verified green.