fix(issue-5): address phase 2a round 2 findings (F-C, F-D, R1, R4)
Both reviewers sustained the round 1 fixes and found four more. Each was reproduced here before being accepted; none were rebutted. F-C (claude, blocking) -- my own round 1 fix caused this. Making refused writes exit non-zero also classified an idempotent ACK as a refusal, and the wrapper finalises an approved job with `status --set completed` on a record that is already completed. It runs under `set -e`, so the *success* path aborted with exit 4 before its validation hook and its final stdout line. Reproduced at the shell. An idempotent result is now distinguishable from a refusal and exits 0; a genuine conflict still exits 4. F-D (claude) -- routing the operator write around update_status dropped the status vocabulary check. Under the rollback flag an invalid status was written to the record with exit 0, where before it was refused with exit 1 and the record left alone. The check now runs first, in both modes. R1 (codex, blocking) -- the previous fix compared identifiers only when they happened to be present, so evidence carrying none at all was still approved on the dispatch record's own flags. That is the self-assertion the check exists to replace. Every identifier must now be supplied and agree, and a report manifest must associate with the attempt being judged. Identifiers are read from a collector envelope beside the payload rather than from inside it: a native Herdr payload may not carry MAM fields, so one appearing there is either fabricated or a contract violation, and either way it is not evidence. R4 (codex) -- a sequence number orders nothing unless the compare and the write are atomic. Two flushes could both read the old value, the newer land first, and the slower overwrite it. Reproduced deterministically. The mirror now serialises read-compare-write under a per-job audit lock, taken outside the registry transaction so a slow mirror still cannot lengthen it, and each write uses its own temp file instead of a shared name. All eight new regressions fail against the pre-fix code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrDNJkHmm18uu7n7JwV5Qd
This commit is contained in:
1 parent
3b134e3358
commit
cb575fa851
6 files changed
+298
-48
No files matched your search
@@ -577,25 +577,44 @@ def update_logged_status(job_id: str, status: str, logs_dir: Optional[str] = Non
|
||||
path = job_log_path(job_id, STATUS_FILENAME, logs_dir)
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
|
||||
incoming_seq = extras.get("transition_seq")
|
||||
if incoming_seq is not None and path.exists():
|
||||
try:
|
||||
existing = json.loads(path.read_text(encoding="utf-8"))
|
||||
existing_seq = existing.get("transition_seq")
|
||||
if existing_seq is not None and int(existing_seq) >= int(incoming_seq):
|
||||
logger.debug("skipping stale status mirror for %s (seq %s <= %s)",
|
||||
job_id, incoming_seq, existing_seq)
|
||||
return
|
||||
except Exception:
|
||||
pass # unreadable existing file: fall through and rewrite it
|
||||
# Compare and write under one per-job lock. A sequence number only orders
|
||||
# writers that cannot interleave: with a bare check-then-write, two
|
||||
# flushes can both read the old value, the newer one lands first, and the
|
||||
# slower one then overwrites it. The lock is deliberately its own file so
|
||||
# it is never confused with the registry lock, and it is taken here --
|
||||
# outside the registry transaction -- so a slow mirror still cannot
|
||||
# lengthen that transaction.
|
||||
lock_path = path.with_name("." + path.name + ".lock")
|
||||
with open(lock_path, "a+") as lock_fh:
|
||||
with _file_lock(lock_fh):
|
||||
incoming_seq = extras.get("transition_seq")
|
||||
if incoming_seq is not None and path.exists():
|
||||
try:
|
||||
existing = json.loads(path.read_text(encoding="utf-8"))
|
||||
existing_seq = existing.get("transition_seq")
|
||||
if existing_seq is not None and int(existing_seq) >= int(incoming_seq):
|
||||
logger.debug("skipping stale status mirror for %s (seq %s <= %s)",
|
||||
job_id, incoming_seq, existing_seq)
|
||||
return
|
||||
except Exception:
|
||||
pass # unreadable existing file: fall through and rewrite it
|
||||
|
||||
record: Dict[str, Any] = {"job_id": job_id, "status": status, "updated_at": _utcnow()}
|
||||
record.update(extras)
|
||||
tmp = path.with_name(path.name + ".tmp")
|
||||
with open(tmp, "w", encoding="utf-8") as fh:
|
||||
json.dump(record, fh, ensure_ascii=False, indent=2)
|
||||
fh.write("\n")
|
||||
os.replace(tmp, path)
|
||||
record: Dict[str, Any] = {"job_id": job_id, "status": status,
|
||||
"updated_at": _utcnow()}
|
||||
record.update(extras)
|
||||
# A per-write temp name: a shared one lets concurrent writers
|
||||
# clobber each other's half-written file before the rename.
|
||||
fd, tmp = tempfile.mkstemp(dir=str(path.parent),
|
||||
prefix="." + path.name + ".", suffix=".tmp")
|
||||
try:
|
||||
with os.fdopen(fd, "w", encoding="utf-8") as fh:
|
||||
json.dump(record, fh, ensure_ascii=False, indent=2)
|
||||
fh.write("\n")
|
||||
os.replace(tmp, path)
|
||||
except BaseException:
|
||||
if os.path.exists(tmp):
|
||||
os.unlink(tmp)
|
||||
raise
|
||||
except Exception as exc: # pragma: no cover - best effort
|
||||
logger.warning("update_logged_status failed for job %s: %s", job_id, exc)
|
||||
|
||||
|
||||
Reference in new issue
Block a user