feat(retry): pace retries and classify errors instead of giving up (audit theme 7)

[passcod]

Closes cross-cutting theme 7 from the logic bug audit: retry logic that either hammers or gives up forever. See the pattern analysis.

The class

Every retry site needs two things: a back-off, so failure does not mean hammering, and a recovery path, so failure does not mean death. Each audited site had at most one. Two distinct shapes hide under it, and they need different treatment — which is why a single "retry this future" helper (the backoff-crate model) fits neither: tick-driven sites have no future to wrap, and relay loops need per-error classification mid-loop rather than whole-operation retry.

(a) Tick-driven retries — a per-key decision

The reconciler is the loop; what was missing is a per-key "should I attempt now?".

runtime/retry.rs generalises the semantics scheduler::should_back_off already implements and tests against the persisted operations log (r[history.operations.rate-limiting]) — capped exponential from a base delay, with a gap longer than the cap resetting the count — for callers whose cadence is the tick rather than a row in that log, so the two do not drift apart. It has no exhausted state by construction: past a threshold the caller escalates to an operator-visible fault and keeps attempting at the cap interval. A permanent give-up is only legitimate behind an expiry or an explicit operator action with a reset path — the TLS retry-block plus store::set_force_retry pair is the model.

• pull.rs (H3) retried a failed pull immediately on the next 5 s tick ("retry immediately", per the comment it deleted), then after 5 attempts set exhausted = true — which nothing could ever clear, because entries were removed from the map only on a successful pull, which could no longer be attempted. A registry that was briefly unreachable disabled actuation of the workload permanently, until the daemon restarted. PullState now holds a gate instead of an attempts counter and a terminal flag.
• issuance.rs (H10) dispatched Tailscale-discovered hostnames to run_tailscale before loading the unified compute_state decision, so Blocked and Debounced were never consulted. While tailscaled was down, every tick opened and finalised a failed tls_cert_attempts row — and the 1000-row cap in Snapshot::load then evicted other hostnames' last_attempt, dissolving their debounce too. The dispatch moves below the decision, and a check_retry_gate applies the two decisions that are about whether to attempt now rather than about ACME policy. The rule: a subsystem with a central decision function admits no dispatch before the decision.

(b) Long-lived loops — classification, and a reported exit

A back-off alone would not have prevented these; they are misclassification plus silent exit.

• udp_relay_task (H21) matched Ok(n) if n > 0 and hit _ => break for both Ok(0) — a legal zero-length datagram — and Err(_), which is how an ICMP port-unreachable surfaces as ECONNREFUSED on a connected socket. One transient error killed the relay forever while the forward stayed registered and listed as healthy. Now: zero-length relays, transient errors report and continue, only connection loss ends it — and the exit is announced, because nothing else observes this task.
• forward.rs (H15) did if client.send_datagram(pkt).is_err() { break }, so quinn's recoverable SendDatagramError::TooLarge — any datagram over path MTU, so EDNS responses and QUIC payloads routinely — was treated as ConnectionLost. The server relay already gets this right, so the two ends of one protocol disagreed. OiClient::send_datagram returns quinn's error unboxed now (one caller), oversize datagrams are dropped and counted, and the count appears in the exit summary.

Findings closed

Finding	Severity
H3 — image pull retries have no back-off and exhaustion is permanent	high
H10 — Tailscale issuance bypasses retry blocks and the failure debounce	high
H21 — UDP relay dies permanently on a transient socket error or zero-length datagram	high
H15 — UDP forward terminates permanently on a single oversized datagram	high

On the dependency question

The theme analysis flagged this as needing a decision, since the repo prefers small dependencies over reimplementation. The obvious crates do not fit: backoff is unmaintained, and exponential-backoff / backon model "retry this future or closure", which matches neither shape here. The repo already owns a tested implementation of exactly these semantics in should_back_off, so this is that shape extracted rather than written fresh — but say if you would rather take a dependency and I will swap it.

Enforcement

• Spec: r[actuate.image.retry] (successive failures spaced by an increasing bounded delay; a transient failure must not permanently disable actuation; the threshold is an escalation, not a terminal state, which is what allows the fault to clear on a later success) and i[forward.relay.resilience] (a relay distinguishes a condition affecting one datagram from one affecting the forward; both ends classify identically; a relay that has ended is not reported as active).
• Tests: seven on the gate — delay doubles and caps, a 99-failure streak stays capped rather than wrapping into a short delay, attempts are withheld until the delay elapses, a gap past the cap resets, no failure count makes a key permanently ineligible, success resets fully, and keys are paced independently.

Not in scope

Misclassification itself — someone still has to decide that Ok(0) is legal and TooLarge is per-packet, and a wrong call puts a transient error in the fatal arm regardless of machinery. Nor does this fix hangs upstream of a loop: the web event broker's back-off is correct but defeated by subscribe_events parking inside accept_uni, which is theme 1 (#137) — that PR is what makes this one's discipline reachable there. The 1000-row Snapshot::load attempt cap should become per-hostname-latest independently; this PR removes the flood that made it bite, not the cap.

Overlap with other themes

Independent — sits on main. Related to #137 only in that direction of causality, with no shared files.

---

Generated by Claude Code

[Copilot]

🟡 Not ready to approve

RetryGate does not actually reset the consecutive-failure count after a gap beyond the cap, which breaks the stated semantics and can mis-drive escalation thresholds after a long quiet period.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR addresses audit theme 7 by introducing shared retry pacing for tick-driven retries and tightening UDP forward/relay error classification so transient/per-datagram failures don’t permanently disable long-lived behaviour. It also updates the specs to codify the required retry/backoff and relay resilience semantics.

Changes:

• Add RetryGate/RetryGates for capped exponential backoff without a terminal “exhausted” state, and use it to pace image-pull retries.
• Move Tailscale issuance dispatch behind the unified decision computation and apply the retry block + debounce gating consistently.
• Align UDP forward/relay handling to treat TooLarge/zero-length/transient socket errors as non-fatal, surface relay termination, and add dropped-datagram stats.

File summaries

File	Description
docs/spec/runtime.md	Adds spec item r[actuate.image.retry] describing bounded backoff + non-terminal escalation semantics for image pulls.
docs/spec/interface.md	Adds spec item i[forward.relay.resilience] defining per-datagram vs per-forward failure classification and termination reporting.
crates/protocol/src/client.rs	Changes OiClient::send_datagram to return quinn::SendDatagramError so callers can classify TooLarge vs ConnectionLost.
crates/ctl/src/forward.rs	Updates client-side UDP forwarding to drop/count oversize datagrams and only break on ConnectionLost.
crates/core/src/system/actuator/pull.rs	Replaces terminal pull exhaustion with a retry gate to pace pulls and allow recovery after extended failure.
crates/core/src/runtime/tls/issuance.rs	Ensures tailscale issuance respects the central decision (retry block + debounce) before dispatch.
crates/core/src/runtime/retry.rs	Introduces per-key retry pacing primitives and unit tests.
crates/core/src/runtime.rs	Exposes the new runtime::retry module.
crates/core/src/oi/forwards/session.rs	Makes UDP relay resilient to zero-length datagrams and transient recv errors; reports task termination to opener.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The image-pull stale-resubmission path can race between overlapping pull tasks and corrupt PullState (in_flight/gate), potentially reintroducing hammering or concurrent pulls despite the new pacing.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

crates/core/src/system/actuator/pull.rs:102

• When a pull is re-submitted due to staleness, the previous ("stale") tokio task can still complete later and mutate the shared PullState for the newer attempt. In particular, a late failure from the old task will set state.in_flight = false and record a failure on the new state's gate, which can cause concurrent pulls and defeat the retry pacing (the next tick can spawn again while the newer task is still running). Similarly, a late success from an older task can remove the map entry that a newer in-flight task expects to update.

To make stale resubmission safe, track an attempt generation/token in PullState (or compare against a captured started_at/attempt id) and, in the spawned task, only clear in_flight / update the gate / remove the entry if the token still matches the current state. Otherwise ignore the completion as belonging to an obsolete attempt.

                tokio::spawn(async move {
                    let result = driver.container.pull_image(&image_owned).await;
                    let mut map = pulling_map.lock();
                    if let Err(e) = result {
                        if let Some(state) = map.get_mut(&image_owned) {
                            state.in_flight = false;

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[github-code-quality]

Code Coverage Overview

Languages: TypeScript, Rust

TypeScript / code-coverage/vitest

The overall coverage in commit 84291bb in the claude/pr-115-theme-... branch remains at 66%, unchanged from commit b383126 in the main branch.

Rust / code-coverage/rust

The overall coverage in commit 84291bb in the claude/pr-115-theme-... branch remains at 59%, unchanged from commit b383126 in the main branch.

Show a code coverage summary of the most impacted files.

File	main b383126	claude/pr-115-theme-... 84291bb	+/-
crates/core/src...tls/issuance.rs	22%	21%	-1%
crates/ctl/src/forward.rs	20%	19%	-1%
crates/protocol/src/client.rs	37%	38%	+1%
crates/core/src...ctuator/pull.rs	0%	56%	+56%
crates/core/src...untime/retry.rs	0%	95%	+95%

---

_{Updated August 02, 2026 03:18 UTC}

[Copilot]

🟡 Not ready to approve

The image-pull stale-resubmission path can race with an older in-flight task and incorrectly clear in_flight, enabling unintended concurrent pull attempts.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

crates/core/src/system/actuator/pull.rs:106

• The stale-pull resubmission path can spawn a second pull task for the same image while the original task is still running. When the original task later finishes, it unconditionally sets state.in_flight = false (or removes the entry), which can incorrectly mark the newer attempt as not in flight and allow additional concurrent resubmissions on subsequent ticks. Capture an attempt identifier (e.g. the started_at Instant you set before spawning) and only mutate the map entry if it still refers to the same attempt.

                let driver = Arc::clone(&self.driver);
                let image_owned = image.to_owned();
                let pulling_map = Arc::clone(&self.pulling);
                tokio::spawn(async move {
                    let result = driver.container.pull_image(&image_owned).await;

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.