fix: [sc-106112] Stop one undeliverable postback from blocking the rest of the spool

[mlataza]

Problem

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 one 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. No per-entry attempt counter existed, 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.

The loss was also misreported. It surfaced as reason=expired, which reads like stale-data cleanup rather than a delivery failure, so "workflow made changes but reported nothing" had no signal pointing at a cause.

Changes

Split the failure into two outcomes at the one place that can tell them apart. attemptPostback now classifies each attempt as deliveryDone (accepted, already fulfilled, or permanently rejected), deliveryUnreachable (transport error, or a connection that broke mid-response), or deliveryRetryEntry (the engine answered but would not take this request). In-line retries treat both failures alike, exactly 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.

Durable attempt budget. A rejected entry keeps its place in the queue with a persisted attempt counter and last error, so its budget survives an agent restart. Once spent, the entry is abandoned: removed under its own drop reason attempts_exhausted, counted separately, and surfaced with a best-effort AgentPostbackAbandoned:<post_id> 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.

Spacing, which was not in the ticket. Reviewing the change surfaced a regression it would otherwise have shipped: an engine failing wholesale answers 5xx for every entry, which at the HTTP layer is indistinguishable from it rejecting each one specifically. With a flapping connection, reconnects are minutes apart, so a plain per-cycle counter could spend an entry's whole budget in minutes and abandon results the engine would have accepted on recovery — worse than the behaviour being replaced, since today those results survive to the 24h age bound. Rejections therefore count at most once every 10 minutes, bounding the budget in wall-clock time rather than in reconnects (a result survives ≥40 min of a wholesale outage however often the agent reconnects). This never delays the pass-over itself: a rejected entry is skipped on every flush regardless; only the counting is spaced.

Backward compatibility. Entry files written by an older agent have no attempt counter and read as never-attempted, so an upgrade delivers them rather than discarding them as corrupt.

Verification

Falsification of both new guards (disposable detached worktrees; the author worktree was never mutated):

• Restored the pre-fix behaviour — a rejection stops the flush. TestSpool_PoisonedEntryDoesNotBlockHealthyEntries went red with exactly the ticket's symptom, expected every entry attempted in one flush, got [poison], alongside entry "b" was stranded behind the poisoned entry and the abandonment test. Restored, green.
• Removed the spacing guard so every rejection counts immediately. TestSpool_RapidRejectionsDoNotBurnTheAttemptBudget went red: entry abandoned after only 2 spaced attempts. Restored, green.

Tests against real HTTP servers, not stubbed deliverers. TestFlushPostbackSpool_PoisonedEntryDoesNotBlockOthers drives the ticket's step-3 scenario through the production stack: an engine that 503s exactly one post_id and 200s the rest. All three results reach the engine in a single flush, in order, and only the rejected entry stays spooled. TestAttemptPostback_TransportFailureIsUnreachable and TestFlushPostbackSpool_StopsEarlyWhenEngineUnreachable point at a closed port for a genuine connection-refused, confirming the early-stop path survives and drops nothing.

TestAttemptPostback_ClassifiesOutcomes pins the mapping directly rather than only through its consequences — collapsing "rejected this request" back into "unreachable" is the defect itself, so it is asserted at the source.

Unit coverage coming from the ACs: a poisoned entry does not block healthy entries; stranded entries are delivered rather than aged out; a transport failure still stops the flush early and consumes no attempt; an entry is abandoned after the max attempts with the distinct reason, counter and callback; the attempt counter survives a simulated restart; an old-format entry file is delivered rather than discarded; a corrupt entry is still discarded; and expired / capacity / attempts / corrupt drops are each counted under their own reason.

go test ./..., go vet ./..., golangci-lint run ./... all clean; GOOS=windows and GOOS=linux cross-builds clean.

Not probed live, and worth being explicit about why — unlike sc-106111, no live probe of the shipped binary was reachable here:

• The spool lives under the installation's data directory, which is root/SYSTEM-owned, so entries cannot be planted unprivileged.
• The flush only runs after a successful MQTT subscribe, which needs a real Azure IoT Hub.
• CreatePostbackRequest hardcodes https://, so a local stub engine would need a TLS certificate trusted by the host — which is also what stands between this ticket and an integration-workflow scenario. The stub-release fixture used by sc-106110 gets away with plain HTTP because the release URL is configurable; the postback scheme is not.

The closest evidence is the httptest-backed tests above: real sockets, real TCP, real transport errors, real production code path — but under go test, not a running agent. Residual risk is in the wiring between a live connection cycle and the flush, which this change does not alter (only the callback's return type), rather than in the flush logic itself.

Follow-up

An end-to-end integration scenario needs a TLS-terminating stub engine trusted by each runner (or a build-time-gated postback scheme override on the same pattern as releaseUrlOverrideFileStr). Worth its own ticket — it would unlock QA automation for this whole class of postback behaviour, not just this fix.