fix(report-failure): warn on a failed row append instead of aborting - #947
Conversation
The append to an open `tend-outage` tracker was bare, so a transient 5xx from `gh issue comment` aborted the step under `set -e` and lost the row. The read above already degrades to a warning, and the rate-limit caller guards its equivalent write for the same reason; this call site was the one left bare. The create branch keeps the opposite policy on purpose — with no tracker open a failed create leaves no record of the outage at all, so it still has to redden the step.
tend-agent
left a comment
tend-agent
left a comment
There was a problem hiding this comment.
The guard itself is right and the test pins it: reverting just the shell change makes test_report_failure_survives_a_failed_append_to_the_open_tracker fail with returncode 1 and the row dropped, and the full file passes (41) with it. One inaccuracy in the new comment block — the closing cross-reference describes rate-limit-preflight.sh as drawing the same fatal/non-fatal line between its two writes, but that caller guards both: its create is wrapped in if ! PAUSE=$(... | run_issue_create_and_reconcile ...), and the comment there says so explicitly ("set -e would take the script down on a failed create and the annotation below — this run's only trace — would never be reached, which is the outcome this path exists to avoid"). The asymmetry in this file is real, but it isn't mirrored there, so a reader who follows the pointer finds the opposite policy on the create.
…ard comment The comment claimed the rate-limit caller draws the same fatal/non-fatal line between its two writes. It does not — it guards both, because a failed create there must still reach the annotation naming what to close. Here the create is the last statement, so the red step is all that is left.
b3c88f4 to
437c748
Compare
2 participants
Found by
review-reviewersanalysingmax-sixty/worktrunk(run 31510773013). Evidence log: https://gist.github.com/a88c03f4d0c3fb1791060ff3dd97d1c4What happened
Worktrunk's Claude subscription hit its weekly limit during the window, and two runs failed with
You've hit your weekly limit · resets 2am (UTC)(visible in each session JSONL, zero tokens billed on the first):tend-mentionclaude -pexit 1tend-reviewon #3791claude -pexit 1The quota exhaustion itself isn't a tend defect. What the second run exposed is:
Report failureran, hit a GitHub 502, and exited 1 instead of degrading, so the row was never appended. #3800 still readsupdated_at: 2026-08-11T15:00:53Zwith zero comments and a single row — it reports one stranded run when there were two, and the run it omits is the one whose review of #3791's new HEAD never happened.Log evidence for the attribution
From run 31505266914's
Report failurestep (13.8 s, ending in failure):The three earlier
ghcalls in the script are each ruled out, which leaves the append:run_issue_ensure_labelis2>/dev/null || true, so it can neither emit that stderr nor abort.run_issue_canonicalis read throughif ! EXISTING=$(...), whose failure path prints::warning::Could not read this repo's tend-outage issues...and exits 0. No::warning::appears anywhere in the run log, so the read succeeded.$EXISTINGwas therefore #3800, and control reached the baregh issue comment— the only unguarded write left.Root cause
report-failure.shguards its read and leaves its append bare:rate-limit-preflight.sh, the sibling caller of the samelib/run-issue.sh, already guards the identical call — added ine5f0f9b, whose comment reasons about exactly this:That argument transfers verbatim;
report-failure.shwas simply never given the same treatment. This is the common write path, not a corner: once a tracker is open, every later failure in the same incident appends through it — a previous outage cluster put 8 rows on worktrunk#3780 this way, all through this one call.The fix
Wrap the append, warn, let the step end clean. Deliberately not symmetric — the create branch keeps its abort, because
test_report_failure_propagates_a_failed_createalready fixes that policy and the reasoning still holds: with no tracker open, a failed create leaves no record of the outage anywhere, so reddening the step is the only surviving signal. An append has a tracker that already carries the incident. The new test's docstring names the asymmetry so it doesn't get "tidied" later.test_report_failure_survives_a_failed_append_to_the_open_trackerreproduces the production failure: without the change it fails withreturncode 1and the row dropped; with it, exit 0 plus the warning. Full file passes (41 tests).Gate assessment
e5f0f9bis the project's already-accepted ruling that this exact mechanism on this exact call is a defect, applied to one of the two call sites. The remaining site has now fired. Failure is structural — given a 5xx on the append, the abort is deterministic, not a model behaviour that might go differently on a replay.ifwrapper plus a warning line, mirroring an existing guard byte-for-byte. It removes an inconsistency between two callers rather than introducing new policy.Not addressed here
The stranded
tend-reviewon worktrunk#3791 has no retry path — the run failed before stamping the commit, and nothing re-fires until the next push, so that HEAD stays unreviewed. #816 raised both halves of this ("nothing re-runs the trigger it names… leaves the PR silently un-reviewed forever") and was closed COMPLETED on 2026-08-07; the naming half did ship, via the nightly enricher. On this evidence the re-run half looks still live, but that's one observation, so it goes in the evidence log to accumulate rather than reopening anything here.The two do compound, which is worth flagging: the tracker is the list a maintainer would re-run from, so a dropped row makes a stranded run correspondingly harder to find. That's the argument for this one-line guard, not for widening the PR.