Standardize failed outbound delivery recovery#25
Conversation
Failed SMS, iMessage and email sends now wake the agent so it can retry on the same thread. The delivery-failure events weren't subscribed before, so the handlers never ran and recovery never replied on the channel that failed. - subscribe to the hard failure events (ignore delivery_unconfirmed telemetry) - correlate a failure back to the original conversation and thread - reply on the failed channel by default; the agent can reword to retry, switch channels with its tools, or reply [SILENT] to stay quiet - dedup repeat webhooks and cap retries so a dead channel can't loop Fixes inkbox-ai#19
dimavrem22
left a comment
dimavrem22
left a comment
There was a problem hiding this comment.
Really nice PR — id-keyed outbound correlation, dedup, per-contact cap and the on-channel [SILENT] recovery flow all hang together well, and the 14 new tests pass locally. Before merging, three things (two behavioral, one mechanical). Inline comments below cover the first two + a minor; the third is here:
Merge conflict — needs a careful rebase, not a mechanical one. The PR is CONFLICTING: main advanced to #24 ("…external webhook injection…"), which rewrote the exact _handle_webhook / event-dispatch code this PR refactors into _dispatch_event. The new failure-event routing has to be re-reconciled against #24's webhook-injection dispatch, so please rebase and re-verify the routing before this goes in.
Heads up: the sibling PR openclaw-plugin#31 carries the identical run_recovery / route-pinning logic, so the two inline findings below apply there too — worth fixing them together.
(Flagging as a comment rather than a formal block — your call on severity.)
| None | ||
| """ | ||
| await self._queue.put( | ||
| _Turn(text=prompt, recovery=True, route_mode=mode, route_meta=dict(meta or {})) |
There was a problem hiding this comment.
🔴 Recovery turns are no longer protected from interruption (regression).
This enqueues a recovery turn with recovery=True + route_mode but no future. The interrupt guard at line 406 keys on future is None:
running_normal = self._current_turn is not None and self._current_turn.future is NoneThe old recovery path (run_consult) set a future, so recovery turns were treated as capture turns and never interrupted. Now future is None is True, so a recovery turn is classified as normal and gets interrupted by any concurrent inbound — directly contradicting the comment three lines below the guard ("a capture turn … delivery-failure recovery … runs to completion") and the _Turn docstring.
Failure mode: the human texts while the agent is mid-recovery → the recovery turn is aborted and the failed message is never recovered, defeating the PR's purpose.
Suggested fix at line 406:
running_normal = (self._current_turn is not None
and self._current_turn.future is None
and self._current_turn.route_mode is None)and update the now-stale _Turn docstring (it still describes only two turn kinds — that stale taxonomy is what this bug slipped through).
| # spoke. Applied here (not at enqueue) so an interleaved inbound can't | ||
| # steal the routing before this turn actually runs. | ||
| if turn.route_mode is not None: | ||
| self.mode = turn.route_mode |
There was a problem hiding this comment.
🟠 self.mode / self.reply_meta are clobbered and never restored.
Routing is a single mutable field on the session, and _reply reads it at send time (line 879: self.send_fn(self.chat_id, text, self.mode, self.reply_meta)). The recovery turn pins it here but nothing restores it, so the next turn that runs without an intervening handle_inbound inherits the recovery channel/thread.
Concrete path: an inbound arrives after a recovery turn is enqueued but before it runs (no interrupt fires) → it queues behind → the recovery pins mode=sms, then the queued normal reply (to, say, an email) sends over SMS with recovery=True meta. A cross-channel misroute.
Suggested fix: carry the route on the _Turn through to the send so _reply uses the turn's route instead of shared session state, or save/restore self.mode/self.reply_meta around the recovery send (restoring after _deliver_reply, since the recovery's own auto-send needs the pinned values).
| ) | ||
| return web.json_response({"ok": True, "unresolved": True}) | ||
|
|
||
| if not self._recovery_allowed(session_key): |
There was a problem hiding this comment.
Minor: _recovery_allowed(session_key) increments the per-contact counter as a side effect, but the if self.sessions is None: guard is a few lines below (1330). So when sessions is None a recovery slot is burned on a no-op. Move the self.sessions is None check above the _recovery_allowed call.
2 participants
Fixes #19.
What was wrong
Outbound SMS, iMessage and email can fail after the agent has gone idle,
via async delivery webhooks. Two things meant recovery never actually
happened: the failure events were never subscribed (so the existing
handlers never ran), and the wake-up used a side-effect turn, so the
agent's reply was never sent back on the channel that failed.
What this does
imessage.delivery_failed, message.bounced/failed); delivery_unconfirmed
stays off the wake path since it's telemetry, not a hard failure
original session and thread, with a webhook contact/thread fallback
agent can reword to retry, switch channels with its tools, or reply
exactly [SILENT] to stay quiet
fallback) and cap recovery per contact so a dead channel can't loop
Tests