Skip to content

fix(runtime): deliver fire-and-forget notify() when the script throws - #133

Merged
ralyodio merged 1 commit into
moshcoder:mainfrom
clawedassistant26:fix/runtime-drain-notify-on-throw
Jul 31, 2026
Merged

fix(runtime): deliver fire-and-forget notify() when the script throws#133
ralyodio merged 1 commit into
moshcoder:mainfrom
clawedassistant26:fix/runtime-drain-notify-on-throw

Conversation

@clawedassistant26

Copy link
Copy Markdown
Contributor

The bug

runScript collects un-awaited async verbs in pending and drains them after fn(scope) returns:

await fn(scope);
// Let any un-awaited async verbs (fire-and-forget notify) finish delivering.
if (pending.length) await Promise.allSettled(pending);

If the script throws, that drain never runs. bin/moshcode.mjs catches the throw and calls process.exit(1), which kills the in-flight POST /api/approvals — so the notification is silently dropped.

The dropped one is the failure ping, which is the one you most want:

notify("build failed");
nope();            // ReferenceError

This violates the file's own stated contract at src/runtime.mjs:58:

pending collects promises returned by async verbs the script did NOT await (e.g. a bare notify("done")), so runScript can drain them before returning — otherwise the process could exit before a fire-and-forget notification lands.

That is precisely what happens; the guard just doesn't cover the throwing path.

Reproduced end to end on unmodified main (8e183ad)

No mocking — notify() is pointed at a real local HTTP server via the documented MOSHCODE_API / MOSHCODE_API_KEY env overrides, which records every request it actually receives.

$ MOSHCODE_API=http://127.0.0.1:8787 MOSHCODE_API_KEY=testkey \
    node bin/moshcode.mjs run fail.mosh
🎸 moshcode — running moshscript

  🔔 notify()  → build failed

nope is not defined

$ cat received.log
(empty — the notification was never delivered)

The 🔔 notify() → build failed line still prints, so the run looks like it notified. Deterministic, 5/5 runs, exit=1 delivered=0. The control (same script without nope()) delivers 1/1.

After the fix, the same command is exit=1 delivered=1, the error still prints, and the approval link now prints too.

The fix

Drain in a finally block. Promise.allSettled never rejects, so this cannot mask or replace the script's own error.

12 insertions / 3 deletions, 6 of the additions comment.

Tests

New test/runtime-drain-on-throw.test.mjs, 10 tests. The fake verb resolves on a macrotask (setTimeout), not a microtask — an un-drained promise chained purely off microtasks can still land by accident, which would let these tests pass for the wrong reason. A timer cannot.

4 are the bug (script throws; a verb throws; all three of several queued notifies deliver, not just the first; notifies queued inside a while (alive) loop).

6 pass both ways and deliberately assert the opposite direction, so the fix cannot buy delivery by swallowing errors: the script's error survives the drain unchanged; a rejecting fire-and-forget verb does not mask it; success-path delivery unchanged; { iterations, stopped } unchanged; a throwing script with nothing queued still rejects; an awaited notify() still returns its value.

Fail-before via git checkout -- src/runtime.mjs: 4 fail / 6 pass unpatched, 10/10 patched.
Full suite 464 → 474, 0 fail (baseline measured by holding the new file out, not assumed).

Scoped out

  • src/tui.mjs:356 has the same shape but recovers instead of exiting (catch → prints, keeps the REPL alive), so the event loop stays open and the POST does land. It benefits from this fix but was not broken.
  • Nothing here changes when a drain is skipped for time — there is still no timeout on the drain, so a hung fire-and-forget verb will hang exit. That is a design call about how long to wait, so I left it alone.

runScript drained un-awaited async verbs only after fn(scope) returned, so a
script that threw skipped the drain entirely. The CLI catches that throw and
calls process.exit(1), which kills the in-flight POST — dropping exactly the
notification the operator most wants:

  notify("build failed"); nope();

printed the "notify() -> build failed" line but never delivered it.

Drain in a finally block instead. Promise.allSettled never rejects, so the
script's own error still propagates unchanged.
@ralyodio
ralyodio merged commit 2d6cf46 into moshcoder:main Jul 31, 2026
3 checks passed
@ralyodio ralyodio mentioned this pull request Jul 31, 2026
ralyodio added a commit that referenced this pull request Jul 31, 2026
Cuts a release off main so the fixes merged after v0.13.1 actually reach
installs. v0.13.1 shipped before #135, so re-running the installer still
handed you a moshcode that reported turso as missing.

Included since v0.13.1:
- #135 fix(tools): find turso in ~/.turso instead of reporting it missing
- #134 fix(tui): stop styling the agent-view notice as an error
- #133 fix(runtime): deliver fire-and-forget notify() when the script throws

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants