fix(issue-5): bind the auto-completion decision atomically to its attempt
Two findings from round 2, both reproduced before being accepted. R2 -- attribution was compared before the terminal write and nothing tied the two together. A job could complete, be re-dispatched and claimed again in that window, and evidence from the first attempt then satisfied a status check and finished the second one. Reproduced with real transitions: committed, with the registry already on iteration 2. This corrects my own earlier reasoning, and I want to be plain about it. Last round I argued that the attempt guard could live entirely in the evidence layer because that is where attempt identity lives. That holds for comparing evidence to a dispatch, and it does not hold for atomicity: a precondition can only be atomic with the commit if the transaction can read it under its own lock. So the identity now lives in the record. It is an opaque dispatch token, not the iteration counter. That distinction is the part of the earlier rebuttal that still stands: iteration already means the loop round, and comparing a dispatch attempt against it produces a guard that is vacuous when they agree and wrong when they diverge. A token has no competing meaning. It is minted when an attempt begins -- claim and redispatch -- and the gate passes the token it decided against, so any intervening re-dispatch refuses the commit. R4 -- the intent identity was being emitted but nothing consumed it. A replayed notification is re-published with a fresh wire sequence, so the monotonic guard cannot recognise it; to that guard a retry looks new. The subscriber now deduplicates on the stable intent identity, leaving the sequence guard as the rule for ordinary events. Across a restart the disk fallback resolves terminal status and a duplicate terminal is refused as an idempotent ack, so a replay after restart cannot double-apply either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrDNJkHmm18uu7n7JwV5Qd
This commit is contained in:
1 parent
d87674679d
commit
01b465f50c
5 files changed
+246
-6
No files matched your search
@@ -28,6 +28,7 @@ from __future__ import annotations
|
||||
|
||||
import logging
|
||||
import os
|
||||
import uuid
|
||||
from typing import Any, Dict, List, Optional
|
||||
|
||||
from lib_py.delegation import mqtt_common, notifier
|
||||
@@ -71,6 +72,20 @@ BOOKKEEPING_FIELDS = ("pending_notification",)
|
||||
# Distinguishes "no expectation" from "expect the field to be None".
|
||||
_UNSET = object()
|
||||
|
||||
# An opaque identity for one dispatch attempt, minted when an attempt begins and
|
||||
# stored in the record itself. It lives here, not only in the shadow store,
|
||||
# because a precondition can only be atomic with the commit if the transaction
|
||||
# can read it under its own lock. Comparing the shadow store before the call
|
||||
# leaves a window in which the job is completed, re-dispatched and claimed again,
|
||||
# after which stale evidence satisfies a status check and completes a new
|
||||
# attempt. An opaque token is used rather than the iteration counter because
|
||||
# iteration already means the loop round, and conflating the two produces a guard
|
||||
# that is either vacuous or wrong.
|
||||
DISPATCH_TOKEN_FIELD = "dispatch_token"
|
||||
|
||||
# The requests that begin a new attempt, and therefore mint a new token.
|
||||
ATTEMPT_STARTING_REQUESTS = ("claim", "redispatch")
|
||||
|
||||
# Only these sources may move a record out of a terminal state. An agent can
|
||||
# never walk back its own terminal; that is the whole point of the CAS table.
|
||||
OPERATOR_SOURCES = ("registry_cli", "operator")
|
||||
@@ -128,7 +143,8 @@ class _Decision(object):
|
||||
|
||||
|
||||
def _decide(record: Dict[str, Any], request: str, expected_status, expected_attempt,
|
||||
source="unknown", fields=None, expected_notification=_UNSET) -> _Decision:
|
||||
source="unknown", fields=None, expected_notification=_UNSET,
|
||||
expected_dispatch_token=None) -> _Decision:
|
||||
"""Pure CAS decision. No I/O, so it is safe to call under the lock.
|
||||
|
||||
Implements the target table in docs/contracts/issue-5/state-transitions.md §3.
|
||||
@@ -162,6 +178,13 @@ def _decide(record: Dict[str, Any], request: str, expected_status, expected_atte
|
||||
"stale attempt: expected {!r}, record has {!r}".format(
|
||||
expected_attempt, actual))
|
||||
|
||||
if expected_dispatch_token is not None:
|
||||
actual_token = record.get(DISPATCH_TOKEN_FIELD)
|
||||
if actual_token != expected_dispatch_token:
|
||||
return _Decision(False, current,
|
||||
"dispatch token changed: the attempt this decision was "
|
||||
"made for is no longer the current one")
|
||||
|
||||
if expected_status is not None and current != expected_status:
|
||||
return _Decision(False, current,
|
||||
"precondition failed: expected status {!r}, found {!r}".format(
|
||||
@@ -243,7 +266,8 @@ def _notification(job_id, kind, **fields) -> Dict[str, Any]:
|
||||
|
||||
def transition(job_id, request="merge", *, registry_dir=None, expected_status=None,
|
||||
expected_attempt=None, fields=None, source="unknown",
|
||||
notify=None, expected_notification=_UNSET) -> TransitionResult:
|
||||
notify=None, expected_notification=_UNSET,
|
||||
expected_dispatch_token=None) -> TransitionResult:
|
||||
"""Decide and commit one state transition. The only writer of job status.
|
||||
|
||||
Returns a result whose `notifications` have ALREADY been flushed; the field
|
||||
@@ -258,7 +282,8 @@ def transition(job_id, request="merge", *, registry_dir=None, expected_status=No
|
||||
from_status = record.get("status")
|
||||
decision = _decide(record, request, expected_status, expected_attempt,
|
||||
source=source, fields=fields,
|
||||
expected_notification=expected_notification)
|
||||
expected_notification=expected_notification,
|
||||
expected_dispatch_token=expected_dispatch_token)
|
||||
|
||||
notifications: List[Dict[str, Any]] = []
|
||||
if decision.commit:
|
||||
@@ -269,6 +294,10 @@ def transition(job_id, request="merge", *, registry_dir=None, expected_status=No
|
||||
# A monotonic counter so a late flush can still be ordered correctly
|
||||
# once the audit mirror happens outside the lock.
|
||||
record["transition_seq"] = int(record.get("transition_seq", 0) or 0) + 1
|
||||
if request in ATTEMPT_STARTING_REQUESTS:
|
||||
# A new attempt begins here, so anything decided about the
|
||||
# previous one stops being committable from this moment on.
|
||||
record[DISPATCH_TOKEN_FIELD] = uuid.uuid4().hex
|
||||
if notify is not None:
|
||||
# One atomic commit records both the new state and the fact that
|
||||
# the world has not been told yet. If the process dies before the
|
||||
|
||||
@@ -100,6 +100,17 @@ class _Watcher:
|
||||
self.expected = set(expected_job_ids)
|
||||
self.tokens = expected_tokens # job_id -> expected auth_token (or None)
|
||||
self.last_seq = dict(expected_seqs)
|
||||
# Intent identities already accepted, per job. A replayed notification is
|
||||
# re-published with a *fresh* wire sequence, so the monotonic sequence
|
||||
# guard below cannot recognise it: to that guard a retry looks like a new
|
||||
# event. The intent identity is what stays stable across a retry, so
|
||||
# deduplication has to happen on that.
|
||||
#
|
||||
# This is per-process state. Across a restart the disk fallback resolves
|
||||
# terminal status from the registry, and a duplicate terminal is refused
|
||||
# by the transition boundary as an idempotent ack, so a replay after
|
||||
# restart cannot double-apply either.
|
||||
self.seen_intents = {}
|
||||
|
||||
def on_message(self, _client, _userdata, msg) -> None:
|
||||
# --- defensive parsing -------------------------------------------
|
||||
@@ -129,6 +140,15 @@ class _Watcher:
|
||||
if seq is None or not isinstance(seq, int):
|
||||
logger.warning("drop event for job %s: missing or invalid seq", jid)
|
||||
return
|
||||
intent = (payload.get("data") or {}).get("notification_intent")
|
||||
if isinstance(intent, dict):
|
||||
identity = (intent.get("seq"), intent.get("event"), intent.get("timestamp"))
|
||||
if identity in self.seen_intents.setdefault(jid, set()):
|
||||
logger.info("drop event for job %s: notification intent %s already accepted",
|
||||
jid, identity)
|
||||
return
|
||||
self.seen_intents[jid].add(identity)
|
||||
|
||||
if seq <= self.last_seq.get(jid, 0):
|
||||
logger.warning("drop event for job %s: seq %d is not monotonically increasing (last %d)",
|
||||
jid, seq, self.last_seq.get(jid, 0))
|
||||
|
||||
@@ -181,6 +181,21 @@ def evaluate(job_id, scenario, registry_dir=None, workspace=None,
|
||||
if mismatch:
|
||||
return GateResult(False, "hold", mismatch)
|
||||
|
||||
# The evidence must belong to the attempt that is currently running, and the
|
||||
# commit must be refused if that stops being true while this runs. Comparing
|
||||
# here alone is not enough: the job can complete, be re-dispatched and
|
||||
# claimed again before the terminal write, after which a status check alone
|
||||
# would let stale evidence finish a new attempt.
|
||||
from lib_py.delegation import engine as _engine
|
||||
|
||||
token = record.get(_engine.DISPATCH_TOKEN_FIELD)
|
||||
if not token:
|
||||
return GateResult(False, "hold",
|
||||
"job has no dispatch token to bind this decision to")
|
||||
if dispatch.get("dispatch_token") != token:
|
||||
return GateResult(False, "hold",
|
||||
"evidence belongs to a superseded dispatch of this job")
|
||||
|
||||
verdict = finalize.verify_manifest(
|
||||
report_dir, job_id, expected_attempt=dispatch.get("attempt_id"),
|
||||
expected_artifacts=record.get("expected_artifacts") or [])
|
||||
@@ -206,7 +221,7 @@ def evaluate(job_id, scenario, registry_dir=None, workspace=None,
|
||||
# carries a loop-iteration counter, which is a different thing.
|
||||
result = engine.transition(
|
||||
job_id, "completed", registry_dir=registry_dir,
|
||||
expected_status="running", source=SOURCE,
|
||||
expected_status="running", expected_dispatch_token=token, source=SOURCE,
|
||||
fields={"completed_by": SOURCE, "auto_complete_reason": decision.reason},
|
||||
notify="completed",
|
||||
)
|
||||
|
||||
@@ -50,7 +50,7 @@ def _atomic_write(path: Path, text: str) -> None:
|
||||
|
||||
|
||||
def record_dispatch(job_id, attempt_id, agent_session_uuid, pane_process_generation,
|
||||
dispatch_ack=True, workspace=None) -> Dict[str, Any]:
|
||||
dispatch_ack=True, workspace=None, dispatch_token=None) -> Dict[str, Any]:
|
||||
"""Persist the four identifiers at dispatch time.
|
||||
|
||||
The execution anchor starts false on purpose: dispatch being acknowledged is
|
||||
@@ -63,6 +63,10 @@ def record_dispatch(job_id, attempt_id, agent_session_uuid, pane_process_generat
|
||||
"pane_process_generation": pane_process_generation,
|
||||
"dispatch_ack": bool(dispatch_ack),
|
||||
"execution_anchor_observed": False,
|
||||
# The record's token for the attempt this evidence belongs to. Carrying
|
||||
# it here is what lets the gate prove the evidence and the commit are
|
||||
# about the same attempt.
|
||||
"dispatch_token": dispatch_token,
|
||||
}
|
||||
_atomic_write(attribution_path(job_id, workspace),
|
||||
json.dumps(context, ensure_ascii=False, indent=2) + "\n")
|
||||
|
||||
Reference in new issue
Block a user