Skip to content

Commit fc53ada

Browse files
committed
fix: [sc-106112] Stop one undeliverable postback from blocking the spool
The spool flush stopped at the first entry that failed, on the assumption that a failure meant the engine was unreachable. But attemptPostback returned done=false for any 5xx or unparseable body, not only for connectivity failures, so a single entry the engine consistently rejected blocked every entry behind it: the flush restarted from the same poisoned entry on every connection cycle, failed, and returned. There was no per-entry attempt counter, so it was retried forever, and the newer results stranded behind it were eventually discarded by the age check -- silently losing exactly the command results the spool was added in sc-97780 to protect, and logging the loss as "expired", which reads like stale-data cleanup rather than a delivery failure. Split the failure into two outcomes at the one place that can tell them apart. attemptPostback now classifies each attempt: deliveryDone (accepted, already fulfilled, or permanently rejected), deliveryUnreachable (a transport error, or a connection that broke mid-response), and deliveryRetryEntry (the engine answered but would not take this request). In-line retries treat both failures alike, as before. The flush does not: unreachable still stops it early -- attempting the rest is pointless and that optimization is worth keeping -- while a rejection passes over the entry and keeps delivering the ones behind it. A rejected entry keeps its place in the queue with a persisted attempt counter and last error, so its budget survives an agent restart, and is abandoned once the budget is spent: removed under its own drop reason, counted separately, and surfaced with a best-effort AgentPostbackAbandoned notification, following the precedent set for exhausted in-line retries. Drop reasons are now counted individually (expired / capacity / attempts_exhausted / corrupt) rather than sharing one number, because a spool shedding entries under pressure and one abandoning a result the engine refuses are diagnosed very differently. Rejections count against the budget at most once every 10 minutes. An engine failing wholesale answers 5xx for every entry, which at the HTTP layer is indistinguishable from it rejecting each one specifically; without spacing, a flapping connection could spend a budget in minutes and abandon results the engine would have accepted on recovery -- worse than the behaviour being replaced. Spacing bounds the budget in wall-clock time rather than in reconnects. It never delays the pass-over itself: a rejected entry is skipped on every flush regardless, and only the counting is spaced. Entry files written by an older agent have no attempt counter and simply read as never attempted, so an upgrade delivers them rather than discarding them.
1 parent 8428a0f commit fc53ada

6 files changed

Lines changed: 1101 additions & 101 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ Loaded plugins are supervised: a subprocess that exits or crashes is detected (b
8282
1. Agent connects to Azure IoT Hub via MQTT on topic `devices/{device_id}/messages/devicebound/#`. Every MQTT operation waits with a deadline rather than an open-ended `token.Wait()`, and the connect/subscribe waits are additionally interruptible by the service stop signal, so a broker that keeps the connection open but stops acknowledging control packets (a throttling hub, a half-open middlebox) produces a logged failure and a backed-off reconnect instead of a device that is connected but never subscribed and cannot be stopped. See the README's "Bounded MQTT Operations" section.
8383
2. Receives JSON messages containing either `commands` (shell scripts) or `get_installation` (system info requests)
8484
3. Executes commands using platform-appropriate interpreter (PowerShell on Windows, Bash on Unix). Stdout and stderr are each captured through an independently bounded writer (`max_output_bytes`, default 10 MiB per stream) so a verbose script cannot OOM the agent; output past the ceiling is discarded and the result is flagged `truncated` with both byte counts. See the README's "Bounding per-command output size" section.
85-
4. Posts results back to Rewst engine at `https://{rewst_engine_host}/webhooks/custom/action/{post_id}`
85+
4. Posts results back to Rewst engine at `https://{rewst_engine_host}/webhooks/custom/action/{post_id}`. Results that exhaust their in-line retry budget are spooled to disk and re-attempted per connection cycle. The flush distinguishes an unreachable engine (stop, retry everything later) from an entry the engine rejected (pass over it, keep delivering the rest), so one undeliverable result can no longer pin the queue until the entries behind it age out. A rejected entry carries a persisted attempt counter and is abandoned after a bounded number of rejections with its own drop reason and a plugin notification. See the README's "Command Result Delivery" section.
8686

8787
### Client System Deployment
8888

README.md

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,47 @@ The spool is bounded by count and age (the oldest/expired entries are evicted)
303303
so it cannot grow without limit, and the flush is bound to the connection cycle
304304
so it never blocks shutdown.
305305

306+
#### One undeliverable result never blocks the rest
307+
308+
A flush distinguishes two failures that look alike from a distance:
309+
310+
- **The engine is unreachable** — a transport error, or a connection that broke
311+
while the response was being read. Nothing behind this entry could be
312+
delivered either, so the flush stops and every remaining entry is retried on a
313+
later cycle. This costs the entry nothing: an outage is not the entry's fault
314+
and consumes none of its attempt budget.
315+
- **The engine rejected this entry** — it answered with a `5xx`, or with a body
316+
that could not be parsed. The engine is plainly up, so the flush **passes over
317+
this entry and keeps going**; every entry behind it still gets its attempt.
318+
319+
The second case used to be read as the first. One result the engine consistently
320+
rejected would pin the queue: the flush restarted from it every cycle, retried
321+
it forever with no attempt bound, and the healthy results behind it were never
322+
attempted until the age check discarded them — logged as `expired`, which reads
323+
like stale-data cleanup rather than the delivery failure it was.
324+
325+
An entry that is rejected carries a **persisted attempt counter and last error**,
326+
so its budget survives an agent restart. After **5 counted rejections** the entry
327+
is abandoned: removed with the distinct reason `attempts_exhausted`, counted
328+
separately, and surfaced with a best-effort `AgentPostbackAbandoned:<post_id>`
329+
plugin notification — never silently reported as stale.
330+
331+
Rejections are counted **at most once every 10 minutes**. An engine that is
332+
failing wholesale answers `5xx` for every entry, which at the HTTP layer is
333+
indistinguishable from it rejecting each one specifically; without that spacing,
334+
a flapping connection could spend an entry's whole budget in minutes and abandon
335+
a result the engine would have accepted on recovery. Spacing bounds the budget in
336+
time rather than in reconnects, so a result survives at least 40 minutes of a
337+
wholesale outage however often the agent reconnects. It never delays the pass-over
338+
itself — a rejected entry is skipped on every flush regardless; only the counting
339+
is spaced.
340+
341+
Drop reasons are counted separately (`expired`, `capacity`, `attempts_exhausted`,
342+
`corrupt`) so a spool shedding entries under pressure is distinguishable in
343+
diagnostics from one abandoning a result the engine refuses. Spool entry files
344+
written by an older agent have no attempt counter; they are read as
345+
never-attempted and delivered normally, not discarded.
346+
306347
The in-line retry budget is tunable per deployment:
307348

308349
| Config key | Default | Description |

0 commit comments

Comments
 (0)