fix(supervise): make the response poll idempotent and slow it to 250ms
tracker-policy-pr / check-pr (pull_request) Successful in 15s
test / integration-docker (pull_request) Successful in 42s
test / unit (pull_request) Successful in 46s
lint / lint (push) Successful in 57s
test / integration-firecracker (pull_request) Successful in 3m22s
test / coverage (pull_request) Successful in 23s
test / publish-infra (pull_request) Has been skipped
tracker-policy-pr / check-pr (pull_request) Successful in 15s
test / integration-docker (pull_request) Successful in 42s
test / unit (pull_request) Successful in 46s
lint / lint (push) Successful in 57s
test / integration-firecracker (pull_request) Successful in 3m22s
test / coverage (pull_request) Successful in 23s
test / publish-infra (pull_request) Has been skipped
The control-plane supervise poll archived the proposal on the same call that returned the decision, so a dropped connection lost the operator's decision (the retry saw `unknown`). Poll no longer archives — a re-poll returns the same decision. Decided proposals still drop off the operator's pending list (a response row exists); their rows are reaped when the bottle is torn down or reconciled. Also raise the grace-window poll interval from 50ms to 250ms now that each poll is an HTTP round trip rather than a local file read. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -527,7 +527,7 @@ class TestDispatchSuperviseAgentRpc(unittest.TestCase):
|
||||
}))
|
||||
self.assertEqual(400, status)
|
||||
|
||||
def test_poll_pending_then_decided_then_archived(self) -> None:
|
||||
def test_poll_pending_then_decided_then_idempotent(self) -> None:
|
||||
rec = self._register()
|
||||
_, proposed = self._propose(rec)
|
||||
pid = proposed["proposal_id"]
|
||||
@@ -546,12 +546,12 @@ class TestDispatchSuperviseAgentRpc(unittest.TestCase):
|
||||
self.assertEqual("approved", decided["status"])
|
||||
self.assertEqual("ok", decided["notes"])
|
||||
|
||||
# The decided poll archived it: gone from pending, and a re-poll is
|
||||
# 'unknown' rather than replaying the decision forever.
|
||||
# Poll doesn't archive: it's gone from the operator's pending list, but a
|
||||
# re-poll returns the same decision so a dropped connection can't lose it.
|
||||
_, listing = dispatch(self.orch, "GET", "/supervise/proposals", b"")
|
||||
self.assertEqual([], listing["proposals"])
|
||||
_, again = self._poll(rec, pid)
|
||||
self.assertEqual("unknown", again["status"])
|
||||
self.assertEqual(decided, again)
|
||||
|
||||
def test_poll_cannot_read_another_bottles_proposal(self) -> None:
|
||||
rec_a = self._register("10.0.0.1", "a")
|
||||
|
||||
@@ -336,7 +336,7 @@ class TestOrchestratorSupervise(unittest.TestCase):
|
||||
self.assertEqual(
|
||||
{"status": "pending"}, self.orch.supervise_poll_response(bottle_id, pid))
|
||||
|
||||
def test_poll_returns_decision_and_archives(self) -> None:
|
||||
def test_poll_is_idempotent_and_leaves_no_pending(self) -> None:
|
||||
bottle_id = self._register("demo", "routes: []\n")
|
||||
pid = self.orch.supervise_queue_proposal(
|
||||
bottle_id, tool=TOOL_EGRESS_ALLOW,
|
||||
@@ -346,10 +346,34 @@ class TestOrchestratorSupervise(unittest.TestCase):
|
||||
decided = self.orch.supervise_poll_response(bottle_id, pid)
|
||||
self.assertEqual("approved", decided["status"])
|
||||
self.assertEqual("ok", decided["notes"])
|
||||
# Archived on read → gone from pending, and a re-poll is 'unknown'.
|
||||
# Decided proposal drops off the operator's pending list (a response row
|
||||
# exists), but poll does NOT archive — a re-poll returns the same
|
||||
# decision so a dropped connection can't lose it (issue #469 review).
|
||||
self.assertEqual([], self.orch.supervise_pending())
|
||||
self.assertEqual(decided, self.orch.supervise_poll_response(bottle_id, pid))
|
||||
|
||||
def test_teardown_reaps_the_bottles_proposals(self) -> None:
|
||||
rec = self.store.register("10.243.0.20", policy="routes: []\n")
|
||||
pid = self.orch.supervise_queue_proposal(
|
||||
rec.bottle_id, tool=TOOL_EGRESS_ALLOW,
|
||||
proposed_file="routes:\n - host: google.com\n", justification="j")
|
||||
self.orch.supervise_respond(
|
||||
pid, bottle_slug=rec.bottle_id, decision="approve", notes="ok")
|
||||
self.assertTrue(self.orch.teardown_bottle(rec.bottle_id))
|
||||
# The gone bottle's decided-but-unconsumed proposal is archived, so a
|
||||
# late poll returns 'unknown' rather than lingering forever.
|
||||
self.assertEqual(
|
||||
{"status": "unknown"}, self.orch.supervise_poll_response(bottle_id, pid))
|
||||
{"status": "unknown"}, self.orch.supervise_poll_response(rec.bottle_id, pid))
|
||||
|
||||
def test_reconcile_reaps_the_bottles_proposals(self) -> None:
|
||||
rec = self.store.register("10.243.0.21", policy="routes: []\n")
|
||||
pid = self.orch.supervise_queue_proposal(
|
||||
rec.bottle_id, tool=TOOL_EGRESS_ALLOW,
|
||||
proposed_file="routes:\n - host: google.com\n", justification="j")
|
||||
# No live source IPs -> the bottle is reaped (grace 0 so it's immediate).
|
||||
self.assertEqual([rec.bottle_id], self.orch.reconcile([], grace_seconds=0))
|
||||
self.assertEqual(
|
||||
{"status": "unknown"}, self.orch.supervise_poll_response(rec.bottle_id, pid))
|
||||
|
||||
def test_poll_unknown_for_other_bottle(self) -> None:
|
||||
bottle_id = self._register("demo", "routes: []\n")
|
||||
|
||||
@@ -108,7 +108,7 @@ class _FakeSuperviseResolver:
|
||||
except FileNotFoundError:
|
||||
return {"status": _sv.POLL_STATUS_UNKNOWN}
|
||||
return {"status": _sv.POLL_STATUS_PENDING}
|
||||
_sv.archive_proposal(self.bottle_id, proposal_id)
|
||||
# Idempotent: poll never archives (issue #469 review).
|
||||
return {
|
||||
"status": response.status, "notes": response.notes,
|
||||
"final_file": response.final_file,
|
||||
@@ -473,7 +473,7 @@ class TestHandleToolsCall(unittest.TestCase):
|
||||
self.assertEqual(ERR_INVALID_PARAMS, cm.exception.code)
|
||||
self.assertIn("unknown tool", cm.exception.message)
|
||||
|
||||
def test_archives_proposal_after_response(self):
|
||||
def test_decided_proposal_drops_off_pending(self):
|
||||
responder = self._respond_when_proposal_appears(_sv.STATUS_APPROVED)
|
||||
try:
|
||||
_tools_call(self.resolver, {
|
||||
@@ -485,7 +485,8 @@ class TestHandleToolsCall(unittest.TestCase):
|
||||
})
|
||||
finally:
|
||||
responder.join()
|
||||
# No pending proposals left after the decided poll archives it.
|
||||
# A decided proposal drops off the operator's pending list (a response
|
||||
# row exists) — poll itself no longer archives (issue #469 review).
|
||||
self.assertEqual([], _sv.list_pending_proposals("dev"))
|
||||
|
||||
def test_pending_response_times_out_without_archive(self):
|
||||
@@ -776,7 +777,7 @@ class TestNonBlockingSupervise(unittest.TestCase):
|
||||
|
||||
# --- check-proposal poll ---
|
||||
|
||||
def test_check_returns_approved_and_archives(self):
|
||||
def test_check_returns_approved_idempotently(self):
|
||||
p = self._seed_proposal()
|
||||
_sv.write_response("dev", _sv.Response(proposal_id=p.id, status=_sv.STATUS_APPROVED, notes="ok"))
|
||||
result = _check(self.resolver, {"arguments": {"proposal_id": p.id}})
|
||||
@@ -784,8 +785,10 @@ class TestNonBlockingSupervise(unittest.TestCase):
|
||||
text = result["content"][0]["text"] # type: ignore[index]
|
||||
self.assertIn("status: approved", text)
|
||||
self.assertIn("notes: ok", text)
|
||||
with self.assertRaises(FileNotFoundError): # archived on read
|
||||
_sv.read_proposal("dev", p.id)
|
||||
# Poll doesn't archive, so a re-check returns the same decision
|
||||
# (issue #469 review) rather than 'unknown'.
|
||||
again = _check(self.resolver, {"arguments": {"proposal_id": p.id}})
|
||||
self.assertEqual(result, again)
|
||||
|
||||
def test_check_rejected_sets_isError(self):
|
||||
p = self._seed_proposal()
|
||||
@@ -853,7 +856,7 @@ class TestNonBlockingSupervise(unittest.TestCase):
|
||||
poll = _check(self.resolver, {"arguments": {"proposal_id": pid}})
|
||||
self.assertFalse(poll["isError"])
|
||||
self.assertIn("status: approved", poll["content"][0]["text"]) # type: ignore[index]
|
||||
self.assertEqual([], _sv.list_pending_proposals("dev")) # resolved + archived
|
||||
self.assertEqual([], _sv.list_pending_proposals("dev")) # resolved (response exists)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
|
||||
Reference in New Issue
Block a user