Repository navigation
fix(dispatch): reconcile the claim, alert on a run of failures, keep the exit code - #206
Merged
Merged
Conversation
…the exit code Three defects on the dispatch failure path, all with the same symptom: the only evidence was an absent message, and an absent message reads exactly like an uneventful day. Between 2026-10-17 06:00 and 2026-10-18 06:05 PDT every dispatch this relay made exited non-zero -- 163 of them, across every repo in scope and both lanes -- and nothing told anybody. Both units stayed active (running) and the daily sweeps kept firing on schedule. 1. A dispatch that ends without finishing now reconciles its own claim comment on the issue, naming the session id and the exit code, and says whether a pull request exists. Posting the claim before the worker starts is deliberate and unchanged -- it is what stops two workers taking one issue. What was missing was the other end of it: on zzsites#853 the claim stood on the beta tester's own ticket for eighteen hours after the worker had exited 1. 2. A run of failed dispatches (3 by default, configurable), or a sweep in which every repo failed, now sends one message. Not one per repo, and not again until a dispatch succeeds -- the latch lives in the state database so it survives a restart, and so does the counter: an in-memory run length resets on every restart, which would exempt precisely the relay that is broken and being restarted. #200 is the opposite failure on this same deployment, where one approval was re-asked thirteen times. 3. sessions.claude_exit_code is populated on the failure path and a scrubbed, capped tail of the worker's final output is kept beside it. The row for the failing session held None while the log line for the same session recorded exit_code 1, and nothing kept the output at all, so the only durable evidence of why 163 dispatches failed was that each took about two seconds. Redaction runs before the cap: truncating first can store half a token, and a prefix of a secret is still a secret. The outcome is reported from the dispatcher rather than from each pipeline, so a lane added later is covered by construction. make_agent_dispatcher takes the hook as a required keyword -- an optional parameter whose default is the pre-fix behaviour is a silent opt-out that reads as an ordinary call. Found while wiring point 2: failure_text was imported inside `if is_task:` in handle_issue and called in the shared failure branch, so a DEV-lane failure raised NameError and the surrounding except turned it into a console line. The "Failed on #N" message could therefore never be sent for a dev issue -- the lane zzsites#853 was dispatched on -- while a task failure sent it fine. The block is now announce_issue_outcome, where a fake transport can watch it. Closes #204
…numerated make_agent_dispatcher's new required keyword found both by itself, which is the point of making it required rather than optional. And the announcement delegation moved one hop: handle_issue calls announce_issue_outcome, which picks the branch. The structural check now accepts either name and still refuses a second announcement call, while the behavioural claim that it reaches announce_dev_success lives in tests/test_issue_outcome_announcement.py against a fake transport.
Both were the same shape: the right topic, one hop from the claim. The redaction-order test composed redact_secrets and tail itself, so swapping the order inside worker_output_tail -- the arrangement that actually ships -- left it green. A second test drives worker_output_tail directly. And the claim_posted guard was covered by a lock-contention test that returns before the flag is assigned, so flipping its initial value to True changed nothing. The new test makes get_issue raise, which enters the try, skips the claim block, and lands in the except -- the one place that reads the flag.
… write Four defects from a review round, each reproduced before it was fixed. A read opened a write transaction. _ensure_dispatch_health_row did an uncommitted INSERT and was called from the read accessors, so a bare dispatch_failure_count() took a RESERVED lock for the life of the connection's transaction. Measured: that call on one connection made an ordinary INSERT INTO sessions on a second one fail with "database is locked". The poller and the bridge both open state.db, so a read-shaped call could have wedged the live relay's whole database -- worse than the alert it was part of. The row is created in the migration and by the write paths, which commit; the reads read. The latch was claimed before delivery, so one failed send silenced the whole outage. Measured: channel absent for the first failure and healthy for the next fifty, zero messages ever delivered. The claim is now the right to ATTEMPT one message and is handed back when delivery fails. The counter is not reset, so the next failure retries rather than restarting the countdown. A duplicate is the safe direction; silence cost two days. The increment, the threshold comparison and the claim are one BEGIN IMMEDIATE transaction. As three statements, a success committed by a second poller could land between the read and the claim, announcing a relay that had just recovered. Redaction missed every namespaced environment variable. An underscore is a word character, so \bsecret\b does not match inside AWS_SECRET_ACCESS_KEY -- nor GITHUB_TOKEN, MY_API_KEY or X_PASSWORD, all of which stored their values verbatim while the bare password= case passed. The same word-boundary fault had already been fixed on the token prefixes and left here. The key is matched with a prefix now, bounded by the assignment operator that must follow. The accepted cost -- secretary= is over-redacted -- has a test of its own, so it is a decision rather than a surprise. And record_sweep said "must equal" while comparing failed < total, so total=1, failed=2 produced an alert reading "all 1 of 1 repos failed".
The second round verified all four of the first round's fixes and found ten more. Six were real and are fixed here; the rest are recorded in the pull request body as known limits with the reason. An OSError out of communicate() reported no outcome at all. Only the spawn failure, the timeout and a normal exit did, so a run of pipe errors would have been this ticket's own defect in a narrower window: dispatches failing with nothing counting them. The child is killed on that path too, for the reason the cancellation arm already does it. BEGIN IMMEDIATE ran before the try, so if a transaction was already open on the connection it raised and nothing rolled back, leaving the lock held. It is inside the try now, behind a helper that commits a pending transaction rather than discarding somebody's data to tidy up a lock, and the rollback only runs when there is a transaction to undo. The latch release was unowned. Two pollers share the row, so A's failed delivery could clear B's newer claim and let one outage be announced twice. The claim now hands back the stamp it wrote and the release is conditional on it. The residual limit -- a whole-second stamp cannot separate two claims inside one second -- is stated in the code rather than papered over, because what it costs is a duplicate message. The latch release also now runs when the send RAISES, not only when it returns False. This function's whole job is that an undelivered alert does not keep the latch, and that must not rest on another function's promise not to raise. The send is bounded. It is awaited from inside the dispatcher, after the child has exited and while the pipeline still holds the repo lock, so an accepted-then-stalled socket would stall the serial poll cycle to deliver a notification. The exception path passed exit_code=None unconditionally, under a comment claiming no exit code was available. False whenever the worker already ran: an exception out of verification, review or cleanup arrives after the exit code is in hand, and the comment then said "not recorded" about a number it was holding. The claim flag is set BEFORE the comment call. gh can create the comment and lose the response, and the two mistakes are not symmetrical: a claim left standing is the eighteen-hour defect, while a reconciliation posted when the claim never landed still says only that nothing is working on the issue. record_session_outcome scrubs what it is given. Redaction is idempotent, so the second pass costs nothing and the invariant holds at the one place every writer goes through. The docstring arguing the opposite was backwards -- the writer is the chokepoint, and it is the callers that can be added without one. And the widened key pattern was eating metrics: token_count=8192 and credential_cache_size=4096 were being redacted, which is the opposite of why the output is kept at all. Metric suffixes are exempt. That enumeration is of metric names, not credential names -- being incomplete there costs a redaction rather than a leak.
…r one live A mutation sweep left two survivors, and neither was an untested behaviour -- both were code that could not change an outcome. The trailing `or None` on the stored output tail was dead: `scrubbed` is already None exactly when the input was falsy, and redact_secrets never maps a non-empty string to an empty one. And the early return in release_dispatch_alert could not fire, because `WHERE alerted_at = NULL` never matches in SQL -- so deleting the branch changed nothing. A guard whose removal changes no outcome is a sentence pretending to be a guard, and the next person cites it as coverage. It reports the caller bug it detects now, which is a real signal and a branch a test can hold. The SQL condition that does the protecting is pinned separately, so the two mechanisms are not resting on each other.
…t again
A third round verified all nine of the second round's fixes and found
more. The redaction key rule had reached its third patch, so the
discriminator changed instead of the list growing:
1. \bsecret\b missed AWS_SECRET_ACCESS_KEY -- an underscore is a word
character. Boundaries dropped.
2. The widened rule then ate token_count=8192. A metric-suffix
exemption was added.
3. That exemption leaked API_KEY_COUNT=hunter2, because the suffix
was the whole test.
The question the rule is actually answering is whether a value is a
measurement or a credential, and a measurement is a NUMBER. The
exemption now needs both halves: a metric-shaped name AND a numeric
value. So the name list can only ever add a redaction, never remove
one, and the credential side stays open-ended -- which is the half that
must not be an enumeration. The value minimum also drops from four
characters to one: password=x and TOKEN=abc were reaching the database
unchanged, and a short secret is still a secret.
The alert claim is a uuid in its own column, not the alerted_at second.
Comparing a whole-second timestamp cannot separate two claims made
inside one second, and during an outage where dispatches fail in about
two seconds each that window is not remote -- so one poller's failed
delivery could clear another's claim and the same outage be announced
twice. This was recorded as a known limit a round earlier; it is cheap
to close properly because the table is new in this change.
record_session_outcome rolls back on a failed write. A "database is
locked" out of the UPDATE or the commit left the transaction open, and
the caller swallows the exception, so this connection would have held a
write lock on a database the bridge also uses. The same fault the read
path had, arriving on an error path instead of a happy one.
The communicate-failure kill is guarded: the child can exit between the
returncode check and the signal, and a bare ProcessLookupError would
have replaced the original failure and skipped the outcome report,
losing both the cause and the count.
Decoding uses errors="replace". Invalid UTF-8 from a child raised
UnicodeDecodeError after the guarded communicate() and before the
outcome report, so that dispatch was neither persisted nor counted.
Replacing the bytes removes the failure mode rather than catching it.
_begin_immediate logs when it finds a transaction already open. The
docstring claimed it did and it did not, which made the sentence false
and the branch invisible -- a caller leaving a transaction open is a bug
somewhere else, and committing it quietly is how that stays unfound.
The reconciliation comment says "supersedes any claim above" rather than
"the claim above is out of date". The flag that triggers it is
deliberately optimistic about a claim existing, so the wording must not
assert that one is visible.
And four tests were strengthened where a reviewer could name a no-op
that would pass them: the stalled-send test now proves the send was
entered, the healthy-relay test lands failures first so its assertions
are not true by default, the cap test proves enforcement and not only
preservation, and the structural delegation guard accepts one name
rather than either.
The locked-write tests hold the write lock from a second connection rather than patching commit, which sqlite3 refuses anyway -- and that is the better outcome, because it reproduces the condition the daemon actually meets: two processes on one file. A control asserts the pair fails because of the lock and not because the write is broken. Also covered: a child that exits between the returncode check and the kill, undecodable output from a child, short credential values, a credential name ending in a metric suffix, and the open-transaction branch whose docstring promised a log.
The token-uniqueness test made two claims back to back and asserted they differed, which is a streak rather than a mechanism: with a one-second timestamp token it passes whenever the two calls straddle a second boundary. A mutation sweep reported it surviving on one run and failing on another, which is the signal. Freezing time.time makes "inside the same second" true by construction. The per-failure path gets its own test for the same property -- nothing else can observe it, because a timestamp token still releases correctly against itself, so that mutation survived until this test existed.
A cold reviewer reading the diff with no history of the previous rounds found the defect those rounds could not: **a restart during delivery left the outage silent for its remaining duration.** The claim was committed before the send was awaited, and the send window is up to 15 s. During a wedged-bridge outage the poller is inside that window most of the time, so a `systemctl restart` -- the first thing an operator does -- almost certainly lands there. Reproduced: cancel mid-delivery, then 59 further failures against a healthy channel delivered zero messages. That is the ticket's own defect, inside the fix for it, reachable by the most likely operator action. Claiming the right to send and having sent are two facts. The claim is now a LEASE and delivery is the durable one, which replaced the explicit release with a single mechanism that covers three things the release did not: a process killed mid-send, a send that merely fails, and the retry rate. During an outage at most one attempt is made per lease window rather than one per failed dispatch. A retraction now cancels the claim. `already_claimed` tested for the marker alone, and the claim comment is deliberately never deleted -- so on the retry its own closing line invites, no fresh claim was posted and the issue's last relay comment stayed "nothing is working on this issue" while a worker ran, and after it succeeded too, because success is announced on chat and not on the issue. For the beta tester, whose only surface is the issue, that is the eighteen-hour defect mirrored. The sweep and the counter now share one definition of failure. The sweep counted PIPELINE failures, which include an agent that exits 0 and writes FAILED; the counter counts a non-zero exit. A sweep of those would have sent "every repo failed, check the agent credential, this will not be repeated until a dispatch succeeds" -- when every dispatch did succeed by the counter's definition, so the next sweep would clear the latch and send it again. The daily repeat the module exists to avoid, with a closing sentence untrue of its own input. `_begin_immediate` joins a caller's open transaction instead of committing it. The premise for committing -- "every writer in this class commits immediately after its execute" -- is false: `execute` is public, callers commit separately, and the outcome hook runs on the pipeline's own connection mid-pipeline. Publishing somebody's partial write is not recoverable; losing a counter increment to a rollback delays an alert by one failure and is not silent. And the redaction rule, on its fourth correction, lost its name list entirely. The fourth correction is the argument for deleting the mechanism: the list still had no `max_tokens`, `input_tokens` or `total_tokens` -- the exact numbers in the API error the alert tells the operator to go and read. A credential in these shapes is never a bare number, so a NUMERIC VALUE is a measurement and needs no list; the only exception is a key whose secret genuinely is numeric, which is a short list on the side where being wrong costs one redaction. Also fixed, all verified by running the regexes: JSON and escaped-JSON credentials were passing through unredacted because a quote sits between the key and the separator, and that is the dominant shape of this output; `Token` as an auth scheme is case-insensitively the English word, so "the token provided was rejected" became "the token [redacted] was rejected"; and `sk-` matched inside `disk-usage-...`, so that one pattern takes a lookbehind while the distinctive prefixes must not, since a lookbehind there is what reintroduces the glued-token leak. A test now drives the real pipeline with the hook wired and reads `claude_exit_code` and `output_tail` back, which shows two orderings no hand-inserted row could: that the session row exists when the hook fires, and that the pipeline's own end-of-run UPDATE does not clear what the hook wrote.
The redaction tests now assert the shapes a worker's stdout actually takes -- JSON and escaped JSON -- rather than the bare key=value form the first three versions of that module were believed correct on. Both directions are pinned: the numbers in an API error survive, a numeric secret does not; the word "token" in prose survives, a Bearer credential does not; `sk-` inside `disk-usage` survives, a real `sk-` key does not. Plus claim_is_live's ordering, a retry after a retraction posting a fresh claim through the real run_dev_issue, and the sweep agreeing with the counter on what "failed" means -- with a control each way, since "never alert on a sweep" would otherwise pass the negative half.
A mutation of the second last_exit_code assignment survived: the existing test fails on the first run with no resume, so the resumed path was never exercised. This one blocks with exit 3, answers, resumes with exit 9, and then raises late -- so the comment must name 9 and not 3.
A verify round on the head confirmed all eight of the cold review's findings fixed, and found that two of those fixes had defects of their own. record_session_outcome and mark_dispatch_alert_delivered committed unconditionally, which publishes a caller's open transaction -- exactly the fault _begin_immediate had just been corrected for, reintroduced through the writers beside it. The outcome hook runs mid-pipeline on the pipeline's own connection, so that caller is real. All three now join rather than commit, and _ensure_dispatch_health_row captures the flag BEFORE its INSERT, because the INSERT opens the transaction the flag is asking about -- a guard that cannot fire, inside the fix for a guard that fired too often. `proc.returncode or 0` told the hook 0 for a status that was never obtained. DispatchOutcome.ok is exit_code == 0, so that collapse reported a dispatch nobody could read as healthy, reset the failure run and cleared the alert -- "I could not see" becoming "there was nothing to see", in the one expression the whole alert rests on. SessionResult keeps its `or 0`, whose readers predate this change; the hook is told None. A credential inside a URL has no key, so the key/value rule could not see it: postgres://user:pw@host and a token in a clone URL both reached the database intact. The scheme and the user are kept, because those are what name the failure. And the alert's closing line said "this will not be repeated until a dispatch succeeds", which stopped being true when the latch became a lease -- a failed delivery is retried once the lease expires. It now says what the code does. The two negative tests the round named are paired with a positive assertion: both passed for an implementation that never called anything, which is the opposite defect wearing the shape of success.
A mutation of record_session_outcome's join flag survived: the only coverage drove record_dispatch_failure, so the writer beside it was untested. This one leaves a caller mid-write, calls the writer, and asserts the caller's transaction is still theirs -- with the control that our own transaction is still committed when there is no caller.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #204.
Three defects on the dispatch failure path. They had one symptom in common,
which is why none of them was noticed: the only evidence was an absent
message, and an absent message is indistinguishable from an uneventful day.
Between 2026-10-17 06:00 and 2026-10-18 06:05 PDT every dispatch this relay
made exited non-zero — 163 of them, across every repo in scope and both the
secopsanddevpipelines — and nothing told anybody. Both systemd unitsstayed
active (running)and the daily sweeps kept firing on schedule.1. A dispatch that ends without finishing reconciles its own claim
On
AInvirion/zzsites#853the claim comment went up two seconds before theworker started, the worker exited 1 four seconds later, and the beta tester's
own ticket said a pull request was coming for eighteen hours. It was noticed
only because a person went looking for the pull request.
Posting the claim first is deliberate and unchanged — it is what stops two
workers taking one issue. What was missing was the other end of it. A run that
ends without finishing now follows its claim with a comment naming the session
id and the exit code, says whether a pull request exists, and ends in what the
reader should do. The claim comment is not deleted: that would lose the record
that the issue was taken, and
format_issue_commentsdrops both markers soneither becomes agent input.
It does not comment when it never touched the issue, when the run
succeeded, or when the run is BLOCKED — a blocked run is waiting on an operator
and its claim is still true.
On
agent:working: the acceptance criteria ask for the label to be removedif it was applied. ctrlrelay has no label-writing code at all — no
gh issue edit, no--add-label, nothing ingithub.pybeyond readinglabelsfor routing.agent:workingis the fleet's own convention for humanand session claims, never applied by this tool, so there is nothing to remove.
Building a removal call for a label we never add would be dead code, so the
criterion is satisfied vacuously and this is the note saying so rather than a
silent omission.
2. A run of failed dispatches raises one alert
Three consecutive failed dispatches (configurable:
alerts.consecutive_dispatch_failures,default 3), or a sweep in which every repo failed, sends one message.
failing repo, and 88 of those is how an operator learns to skim the channel.
this same deployment — an expired question re-asked unchanged every morning,
one approval asked thirteen times — and a daily "still broken" is that bug
with a different payload.
in the lifetime of a database, and the next outage is silent again. A success
clears it, which is also the only event that means the relay recovered.
restarts, and an in-memory run length resets on every restart — which would
exempt precisely the relay that is broken and being restarted, and would
re-fire the alert on every boot.
0 of 0 failedsatisfies "every repofailed" arithmetically; nothing ran is not the same fact as everything failed.
so the relay works.
The outcome is reported from the dispatcher, not from each pipeline. Three
lanes spawn workers and resumes and fix rounds spawn more, so recording at the
call sites means a lane added later is silently exempt — which is how this
outage went unreported in the first place.
make_agent_dispatchertakes thehook as a required keyword: an optional parameter whose default is the
pre-fix behaviour is a silent opt-out that reads as an ordinary call, and making
it required handed the enumeration of call sites to the interpreter. It found
both of them.
3. The exit code and the worker's output are kept
sessions.claude_exit_codewas declared from the start and never written on thefailure path — the row for the failing session held
Nonewhile the log linefor the same session recorded
exit_code 1. Nothing kept the output either,so the only durable evidence of why 163 dispatches failed was that each took
about two seconds: enough to eliminate causes, not to read one.
Both are now recorded for every spawn, with a new
sessions.output_tailcolumn.The output is scrubbed and capped first, and two ordering decisions carry the
correctness:
half, and half a token no longer matches the pattern that would have caught
it, so the fragment is stored verbatim. A prefix of a secret is still a
secret.
lines.
A fourth defect, found while wiring the second
failure_textwas imported insideif is_task:inhandle_issueand called inthe shared failure branch, so a dev-lane failure raised
NameError: name 'failure_text' is not defined, which the surroundingexcept Exceptionturned into a yellow console line. The❌ Failed on #Nmessage could therefore never be sent for a dev issue — the lane
zzsites#853was dispatched on — while a task failure sent it fine, which iswhy the branch read as working.
Same fault class as the ticket: the only symptom was an absent message. The
block is now
announce_issue_outcome, at module level where a fake transportcan watch it, and the name is bound on every path by construction.
What was measured, and how
Suite:
uv run --extra dev pytest tests/— 1316 passed, 0 skipped, on532f283, run from this worktree on alkaturbo, clean tree before and after. Itreconciles against a 1316 collect, so nothing failed to import.
pytest-xdistis not a dependency of this project, so the run is sequential; there is no
-n autoresult to compare against.Mutation sweep: 44 mutations, all red, twice in a row. The sweep puts each
fault back and names the tests that must fail. It halts if a mutation did not
change the file, and if the named tests did not run at all — a node-id typo
reports "no tests ran" and reads exactly like a guard confirming itself.
Four guards were found to be untestable or dead by that sweep rather than by
reading, and each was repaired rather than tested around:
redact_secretsandtailitself, soswapping the order inside
worker_output_tail— the arrangement that ships —left it green.
claim_postedguard was covered by a lock-contention test that returnsbefore the flag is even assigned.
or Noneon the stored output tail could not change an outcome;deleted rather than tested.
release_dispatch_alertcould not fire, becauseWHERE alerted_at = NULLnever matches in SQL. It reports the caller bug itdetects now, which is a branch a test can hold.
The sweep itself was a faulty instrument once, and that is worth recording.
Two mutations replace
uuid.uuid4().hexwithstr(int(time.time()))indifferent functions of one file, so both produce a file of identical size —
and Python invalidates a
.pycon (mtime, size). When the second write landedin the same second as the first, the cached bytecode from the first mutation was
reused: the file on disk was mutated and the code that ran was not. Measured:
that mutation reported RED three times out of three in isolation and SURVIVED
when run immediately after its neighbour. The sweep now drops every
.pycandruns with
PYTHONDONTWRITEBYTECODE=1, and is deterministic across two fullruns. The usual check — "did the file change" — answers yes in that case, which
is what makes it worth writing down.
One test was flaky by construction for a related reason and was rewritten: the
claim-token uniqueness test made two claims back to back and asserted they
differed, which is a streak rather than a mechanism — with a timestamp token
it passes whenever the two calls straddle a second boundary. The clock is frozen
now, so "inside the same second" is true by construction.
The claim retraction was proven against a real GitHub issue, not by reading
the code. A scratch issue was created, the real claim comment posted through
GitHubCLI,reconcile_claim_on_failurerun against it, and the result readback from the forge rather than from the function's return value: two
comments on the issue, exactly one carrying the retraction marker, naming the
session id and exit code 1. The scratch issue was closed immediately.
The migration was proven against a copy of the live database — the real
schema with 3,730
sessionsrows.output_tail,dispatch_healthandalert_claim_tokenare added, no rows are lost, and reopening is a no-op. Asecond connection can still write afterwards, which is the check that matters
here: the poller and the bridge both open
state.db.The live config loads unchanged. 86 repos, and
alertstakes its defaults,so nothing needs editing in
~/.config/ctrlrelay/orchestrator.yamlfor this towork.
Known limits, stated rather than left to be discovered
reconcile_claim_on_failurerunson a path that is already failing, so it swallows
gherrors rather thanreplacing the run's real failure with a different one. A failure is logged as
dev.claim.retraction_failed. There is no retry; retry machinery is largerthan the defect.
call, because
ghcan create the comment and lose the response. So a runwhose claim genuinely never landed still posts a reconciliation — which says
only that nothing is working on the issue, and is true either way. The wording
is "supersedes any claim above" rather than "the claim above", so it does not
assert that a claim is visible. The two mistakes are not symmetrical: a claim
left standing is the eighteen-hour defect.
The connect is bounded at 10 s and the send at 5 s, and a failed delivery hands
the latch back so the next failure retries. That is latency added to work that
is already failing, which is the cheap direction; the alternative is a backoff
whose own state is a second thing that can go wrong silently.
persist_dispatch_outcomedoes not cap what a future caller hands it.record_session_outcomescrubs at the chokepoint, which is the invariant thatmatters, but it does not truncate. Today's one caller truncates first.
test_unreviewed_pr_announcement.pyshowshandle_issuehas not grown its ownannouncement; it cannot show the call runs.
handle_issueis a closure insidea Typer command, which is why the behaviour moved out to where a fake transport
can watch it. That limit is pre-existing and is stated in the test's docstring.
latch is now claimed by uuid and released only by its owner, and the failure
count, comparison and claim are one
BEGIN IMMEDIATEtransaction, so theworst case is a duplicate message rather than a missed one. Nothing here makes
running two pollers against one database a good idea.
Review
Three bounded rounds plus one cold reviewer, each on a named sha, bounded to
git diff origin/main...HEADafter a fetch, from a scratch directory holdingonly the diff, with round output written outside it, reasoning effort set
explicitly, and the verdict line checked at the end of the output rather than
anywhere in it.
61b56ae823dd250e4411abc08a28cd00dbeRound 1 found the alert latch was claimed before delivery, which is this
ticket's own defect reproduced inside the fix for it: measured with the channel
absent for the first failure and healthy for the next fifty, zero messages
were ever delivered. The latch is the right to attempt one message now, and is
handed back when delivery fails.
Round 1 also reported a two-poller race in the health counter. Trying to
reproduce it found something worse that nobody had reported: a read-looking
method (
dispatch_failure_count) was leaving an uncommitted write transactionopen, so a bare count on one connection made an ordinary
INSERT INTO sessionson a second one fail with
database is locked. The poller and the bridge bothopen
state.db, so a read-shaped call could have wedged the live relay's wholedatabase. The race as literally described was not reachable — SQLite serialised
it — but the mechanism it pointed at was real.
Round 3's redaction finding was the rule's third patch, so the discriminator
changed instead of the list growing again. The question the rule actually
answers is whether a value is a measurement or a credential, and a measurement
is a number; the exemption now needs a metric-shaped name and a numeric
value. That ordering matters: the name list can only ever add a redaction,
never remove one, so the credential side stays open-ended — which is the half
that must not be an enumeration.
The last column climbing from 0 to 4 to 7 is why round 4 is a different
model reading the diff cold rather than a fourth round of the same reviewer.
It earned its place immediately: it found the one BLOCKING defect three warm
rounds had not.
A restart during alert delivery left the outage silent for its remaining
duration. The claim was committed before the send was awaited, and the send
window is up to 15 s — during a wedged-bridge outage the poller is inside that
window most of the time, so a
systemctl restart, the first thing an operatordoes, almost certainly lands there. Reproduced: cancel mid-delivery, then 59
further failures against a healthy channel delivered zero messages. The
ticket's own defect, inside the fix for it, reachable by the most likely
operator action — and the arm's own justification ("there is no next failure to
retry on") was false for a restart rather than a retirement.
The repair collapsed two mechanisms into one: a claim is a lease, and only a
recorded delivery suppresses a repeat. That covers three things the explicit
release did not — a process killed mid-send, a send that merely fails, and the
retry rate, which is now one attempt per lease window rather than one per
failed dispatch.
Round 4 also found that the retraction invited a retry which posted no fresh
claim, so the issue's last relay comment stayed "nothing is working on this
issue" while a worker ran — the eighteen-hour defect mirrored — and that the
sweep and the counter held two different definitions of "failed" behind one
latch, which would have sent a message untrue of its own input and repeated it
every sweep.
Round 5 verified all eight of those and found that two of them had defects of
their own: the new writers committed unconditionally, reintroducing through
record_session_outcomethe exact fault_begin_immediatehad just beencorrected for; and
proc.returncode or 0told the hook0for a status neverobtained, so a dispatch nobody could read counted as healthy and cleared the
alert.
Round 5's own findings were mostly in round 4's fixes, which is the
documented signal to stop, and the loop is at its five-round cap. What is
left unfixed is in the known-limits list above, each with the reason. Two of
those the last round rated BLOCKING and I did not:
for up to ~15 s. Making it fire-and-forget is now safe under the lease
(an unfinished attempt records nothing and ages out), and it is a change to
the concurrency model at round five on a branch that has had five of them.
It belongs in its own ticket, and the cost it carries meanwhile is latency on
work that is already failing.
and then fails to be recorded may send a second message. That is deliberate:
silence cost two days and a duplicate costs a line in a channel. The
reviewer's other half — "lost if no later dispatch fails" — is weaker than
it reads: if no later dispatch fails, the relay recovered, and there is
nothing left to announce.
Round 5's sha is
cd00dbeand the head is532f283. The delta is twocommits: one source fix it asked for (
e6685c3) and the test for the onemutation that survived afterwards. The 44-mutation sweep and the full suite
were both re-run on the head.