fix(triggers): stop swallowing a failed trigger removal - #166
Merged
Merged
Conversation
Owner
Author
|
Review found two defects that contradict the invariant this PR documents. Both proven by execution, not on paper. A follow-up commit is being prepared on this branch rather than merging as is. 1. A throw before A trigger file whose body is the valid JSON This still an improvement over 2. The Reverting that block to its pre-PR form ( Also noted
Attacked, nothing found: the two-simultaneous-events race. The fix direction: make the invariant total — every exit, including by exception, writes a result and then removes the file — with "never replay" still winning if the result write itself fails. |
devsuitup
added a commit
that referenced
this pull request
Sep 3, 2026
Adversarial review of PR #166 found two ways processTriggerFile() could decide a trigger's fate without ever writing a result or deleting the file, contradicting the invariant this PR's own docs claimed: - A trigger body that parses as valid JSON but destructures badly (the bare value `null`, or a chain step that isn't an object) threw before any writeResult() call. The thrown promise rejection was caught in dispatch(), which retained the name and logged, but nothing was ever written and the file was never deleted -- silently unrunnable forever, even if a valid file later reused the same name. - A non-ENOENT lstat error (share-lock, EPERM on a network path) hit a bare `if (...) { log; retain; } return;` with no writeResult() call either, and had no test coverage: reverting it to the pre-PR `catch { return; }` left the suite fully green. Wrap the whole body of processTriggerFile() in one try/catch that falls back to a generic `{ok:false, error:"internal error: ..."}` result on anything unanticipated, and route the lstat catch through writeResult() the same way every other validation failure already does. writeResult() itself is unchanged: it still deletes the trigger unconditionally, even if the result write failed, and it still never throws -- so this keeps the "no unhandledRejection" property the PR was built to guarantee. With both paths now resolving through writeResult() on every exit, `retained` only means one thing left: the unlink itself failed. It no longer marks the case where an internal throw happened but nothing was actually left undone, since that case now writes a result and deletes the file like any other outcome. dispatch()'s own catch stays as a last-resort backstop for something escaping the new try (a broken logger, say) -- it should no longer fire for anything this function does today. Updated the "a throwing ctx" test to match: it now checks for a definitive result and a deleted trigger file (not a permanent block), and that a later attempt under the same name goes through. Added three tests for the specific shapes review found, each checked against its own mutation (the review's mutations for both defects, plus removing the new outer try/catch) to confirm they fail without the fix. Docs updated to describe the try/catch and the narrower meaning of retained -- the previous wording claimed a total invariant the code didn't actually have.
Every exit path in processTriggerFile already routed through writeResult,
which writes the result then unlinks the trigger, so no path skipped the
removal. What it did not handle was the removal failing: the unlink sat in
a bare `catch {}`, so a locked file, a permissions error or a non-regular
entry left the trigger on disk with no trace at all, indistinguishable from
one still waiting to be processed. Nothing then stopped a later filesystem
event on that name from replaying the command into the session.
Report a non-ENOENT unlink or lstat failure at error level and remember the
name in a `retained` set the watcher refuses to dispatch again, for the
lifetime of the process. ENOENT stays silent: it is the intended end state,
reached by the loser of a race between two events for the same file. Catch
a rejected processTriggerFile promise in dispatch() for the same reason —
it left an unhandled rejection and an entry in an unknown state.
Document the actual behaviour, including the deliberate gap where a failed
result write still deletes the trigger, and note that processed/ has no
retention policy.
Adversarial review of PR #166 found two ways processTriggerFile() could decide a trigger's fate without ever writing a result or deleting the file, contradicting the invariant this PR's own docs claimed: - A trigger body that parses as valid JSON but destructures badly (the bare value `null`, or a chain step that isn't an object) threw before any writeResult() call. The thrown promise rejection was caught in dispatch(), which retained the name and logged, but nothing was ever written and the file was never deleted -- silently unrunnable forever, even if a valid file later reused the same name. - A non-ENOENT lstat error (share-lock, EPERM on a network path) hit a bare `if (...) { log; retain; } return;` with no writeResult() call either, and had no test coverage: reverting it to the pre-PR `catch { return; }` left the suite fully green. Wrap the whole body of processTriggerFile() in one try/catch that falls back to a generic `{ok:false, error:"internal error: ..."}` result on anything unanticipated, and route the lstat catch through writeResult() the same way every other validation failure already does. writeResult() itself is unchanged: it still deletes the trigger unconditionally, even if the result write failed, and it still never throws -- so this keeps the "no unhandledRejection" property the PR was built to guarantee. With both paths now resolving through writeResult() on every exit, `retained` only means one thing left: the unlink itself failed. It no longer marks the case where an internal throw happened but nothing was actually left undone, since that case now writes a result and deletes the file like any other outcome. dispatch()'s own catch stays as a last-resort backstop for something escaping the new try (a broken logger, say) -- it should no longer fire for anything this function does today. Updated the "a throwing ctx" test to match: it now checks for a definitive result and a deleted trigger file (not a permanent block), and that a later attempt under the same name goes through. Added three tests for the specific shapes review found, each checked against its own mutation (the review's mutations for both defects, plus removing the new outer try/catch) to confirm they fail without the fix. Docs updated to describe the try/catch and the narrower meaning of retained -- the previous wording claimed a total invariant the code didn't actually have.
devsuitup
force-pushed
the
fix/trigger-file-cleanup
branch
from
September 4, 2026 00:17
c3678d5 to
00c6a8a
Compare
…aping Each of the four poll loops (waitForComposerFree, pollForBusyRise, waitForBusyFall, waitForIdle) called back into ctx from a recursive setTimeout tick, not just the synchronous first call. A throw from a later tick had nothing to catch it -- not the original Promise executor, not processTriggerFile's try/catch, not dispatch()'s .catch() -- and surfaced as an uncaughtException on the whole process. All four now share one pollLoop() helper that routes a throw from any tick to that promise's rejection. writeResult()'s own error logging could itself throw (a broken ctx.log is out of this module's control), which used to skip the unlink attempt and, on the unlink-failure path, run before onEntryRetained() -- meaning a logger failure could drop the guarantee it was meant to describe. Logging in writeResult() now goes through safeLogError(), which cannot throw, and onEntryRetained() runs before it. Added a dedicated internal:true field on the generic-catch result so a supervisor can tell an internal bug apart from a validation refusal without parsing the error string. Also covers, with a regression test, a case the code already handled correctly but nothing tested: an ENOENT unlink race must not populate retained, or a later legitimate reuse of the same trigger name is silently dropped.
…r-fate path The generic catch's own log line and dispatch()'s backstop log line were still raw ctx.log.error() calls. An unhandledRejection terminates the process by default under Node -- the same reasoning that justified fixing the deferred-poll-throw defect at the source rather than documenting it applies here: a broken ctx.log at either site could kill the app, and dispatch()'s own retained.add(filename) already runs before its log call, so nothing but the log itself needed guarding. Both now go through safeLogError(). Every other ctx.log.* call inside processTriggerFile() is deliberately left unwrapped: each one precedes a writeResult() return inside the same outer try, so a throw from any of them is already absorbed by these two now-guarded backstops -- wrapping every site individually would duplicate that protection without closing anything. Documented the boundary in .ai/contexts/trigger-watcher.md so the asymmetry reads as deliberate.
The pointer three lines above already covers it; repo rule caps in-code explanation at one line.
Merged
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
1 participant
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.
What this is not
A remote machine reported 18 trigger files processed but never removed. That machine runs a different transport — the harness's
tmux-watchagainst the same directory contract — so this PR fixes nothing that was observed there. It started as a census of this repo's transport, and the census came back clean.No exit path skips the removal. Every path that writes a result goes through the single
writeResult()closure (trigger-watcher.js:425), which writes the result and then unlinks the trigger. All 30-odd exits — validation refusals, absent session, dead target, idle timeout, composer never free, PTY write failure, and every chain abort — remove the entry. "Processed but left in place" cannot happen here by construction.The defect that is real
fs.unlinkSyncwas wrapped in a silentcatch {}. A removal that genuinely fails — locked file, permissions, a non-regular entry — left the trigger on disk with no trace at all, indistinguishable from one still waiting. Worse: nothing stopped a later filesystem event on that name from re-injecting the command into the session, on a trigger whose result had already been written.Reproducible here without mocks: create the entry as a directory, and it is refused (
trigger must be a regular file), its result is written, and the unlink fails.What changed
unlinkfailure is now logged at error rather than swallowed; ENOENT stays quiet, since that is the intended end state and the ordinary outcome of losing a race between two events. Same treatment forlstat.retainedset holds the names whose removal failed, and both the watch handler and the scan refuse to dispatch them again. The result is already written, so never looking at the residual entry again loses nothing. It only ever receives failures, so it does not grow in normal operation.processTriggerFilegets a.catch: no more unhandled rejection, and a throw marks the entry non-replayable — a throw leaves an unknown state, and replay is the one outcome that cannot be taken back.Tests
832 tests, 825 pass, 0 fail, 7 skipped (base 822). eslint 0 errors. Three inverse mutations, each verified individually red: restoring the silent catch, dropping the
retainedcheck from the handler, and removing the dispatch.catch. The middle one carries the invariant that matters — after an impossible removal, the same name reappearing as a valid trigger no longer reaches the PTY and no longer overwrites the original result.Noted, not fixed
processed/has no retention policy at all. The.result.jsonfiles accumulate without bound; nothing in the repo ever removes them. Documented, deliberately not implemented — that's a decision, not an oversight..tmpcan survive ifwriteFileSyncsucceeds andrenameSyncfails.