fix(escalation): wait full quorum timeout instead of resolving on first vote#3255
fix(escalation): wait full quorum timeout instead of resolving on first vote#3255SemTiOne wants to merge 10 commits into
Conversation
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
|
Welcome to the Agent Governance Toolkit! Thanks for your first pull request. |
🤖 AI Agent: contributor-guide — View details
Welcome, and thank you for your contribution! 🎉 Your detailed explanation of the issue and the targeted scope of changes is excellent. The regression test addition is also well-done. Before merging, please ensure:
For guidance, check out CONTRIBUTING.md. |
|
@microsoft-github-policy-service agree |
…st vote resolve() woke on the first quorum vote's event and ran the quorum check exactly once, falling through to default_action before later votes could arrive -- an M-of-N quorum could never be satisfied once M > 1. wait_for_quorum() now loops: it re-checks quorum after every vote and re-arms the event (inside the same lock approve()/deny() hold before calling set()) so a vote landing in that window is never lost. approve()/deny() previously only fired the wakeup event on the vote that first set req.decision; later votes were still recorded but never woke a blocked resolve(). Left as-is, a partial fix would still land on the correct decision but only after blocking for the entire timeout, defeating the point of the fix. Both now fire on every accepted vote. Fixes microsoft#3186 Signed-off-by: SemTiOne <emphyst80@gmail.com>
69c742b to
96548b1
Compare
github-actions
Bot
added
size/M
|
One extra regression I would want before this lands is the no-effect boundary around partial quorum. After the first allow vote, and before the quorum predicate is satisfied, no policy decision or action path should be able to observe allow, default allow, or a completed approval. The intermediate vote is evidence, not authorization. Authorization exists only after the action-bound quorum predicate closes. I would cover three assertions:
That keeps the invariant crisp: require_approval is fail-closed and no-effect until the retained approval evidence proves the required quorum for the bound action. |
|
Thanks for the feedback @rpelevin. Yeah, I verified this against Assertion 3 is already covered by |
|
Yes, a follow-up issue would be useful. I would scope it to one observable invariant: an incomplete quorum can be retained as evidence, but it must not make the request look authorized or remove it from pending work. The issue I would file is:
That keeps this separate from the waiter timing fix in this pull request: the current fix makes the wait path wake correctly; the follow-up makes the observable decision state fail closed until quorum is actually satisfied. |
imran-siddique
left a comment
imran-siddique
left a comment
There was a problem hiding this comment.
Reviewed the quorum fix in integrations/escalation.py and the regression test.
The core change is correct. resolve() now routes quorum through the new wait_for_quorum(), which loops re-checking _quorum_outcome() after each vote and re-arms the per-request Event with clear() while holding self._lock — the same lock approve()/deny() mutate the request under before they call set() outside it. That ordering means there is no lost wakeup and no deadlock: if a vote's set() lands between the unlock and the event.wait(), wait() returns immediately because the event is already set, and the loop re-evaluates. The companion fix to fire the event on every accepted vote (not just the one that first sets req.decision) is what makes the second-vote wakeup prompt instead of blocking to the deadline.
_quorum_outcome() as the single source of truth for both the wait predicate and the final decision (denials checked before approvals) means the waited result and the re-derived decision can't disagree, and the zero-vote path still leaves req.decision == PENDING and falls through to default_action correctly. The documented caveat that non-in-memory backends still poll once is accurate and reasonable.
The regression test is strong: it asserts both the ALLOW outcome and elapsed < 1.0s (votes at 0.1s/0.4s against a 2s timeout), so it discriminates the event-firing fix from a naive implementation that would merely block to the timeout and re-check. CI green. LGTM.
🤖 AI Agent: security-scanner — View details
No security issues found. |
🤖 AI Agent: docs-sync-checker — Docs Sync
Docs Sync
|
🤖 AI Agent: breaking-change-detector — API Compatibility
API Compatibility
|
🤖 AI Agent: test-generator — `agent-os/src/agent_os/integrations/escalation.py`
|
🤖 AI Agent: code-reviewer — Action Items:
TL;DR: 0 blockers, 1 warning. The PR addresses a critical bug in quorum handling, but the new
Action Items:
|
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
Spell-check changed files fails on wakeup (3 hits, all in lines this PR adds: a code comment and a test assertion message). Add wakeup to .cspell-repo-terms.txt or reword to wake-up.
Signed-off-by: SemTiOne <emphyst80@gmail.com>
|
Fixed. Reworded to wake-up. |
imran-siddique
left a comment
imran-siddique
left a comment
There was a problem hiding this comment.
The loop re-checks the quorum outcome after each vote and re-arms the event under the same lock approve/deny mutate, so an M-of-N quorum gets its full window and denies on timeout via the DENY default. The spelling finding is addressed and CI is green; the outstanding review looks stale.
4 participants
Description
EscalationHandler.resolve()woke on the first quorum vote's event and ran the quorum check exactly once, falling through todefault_actionbefore later votes could arrive. An M-of-N quorum with M > 1 could never be satisfied.InMemoryApprovalQueue.wait_for_quorum()now loops: it re-checks quorum after every vote and re-arms the event inside the same lockapprove()/deny()hold before callingset(), so a vote landing in that window is never lost.While reproducing this, found
approve()/deny()only fired the wakeup event on the vote that first setreq.decision. Later votes were recorded inreq.votesbut never woke a blockedresolve(). Left unfixed,wait_for_quorum()'s loop would still land on the correct decision, but only by blocking for the entiretimeout_secondsand re-checking votes at the deadline, which defeats the point of this fix. Both methods now fire on every accepted vote, not just the first.Scope is intentionally limited to
resolve()/wait_for_quorum()/_quorum_outcome()plus this event-firing fix inapprove()/deny(). It does not touch the vote-recording/dedup/empty-approver logic landed via #3126. That's unrelated to this bug and already inmain.Type of Change
Package(s) Affected
Checklist
Attribution & Prior Art
Prior art / related projects:
wait_for_quorum()'s loop follows the same re-check-predicate-and-re-arm pattern as Python'sthreading.Condition.wait_for(). Implemented by hand here instead of migrating toConditionto avoid touchingInMemoryApprovalQueue.approve()/deny()'s already-landed logic from #3126 beyond the minimal event-firing fix.AI Assistance
Claude (Anthropic) was used for root-cause investigation, drafting the fix, and test design. All changes were reviewed, verified against a live upstream checkout (not just the fork snapshot), and re-tested by reverting each individual fix in isolation to confirm the regression test actually discriminates the bug it targets, before submission.
IP, Patents, and Licensing
Related Issues
Fixes #3186