feat(admin): distinct ACK outcomes + opt-in auto-retry (#4492, #4487)

[Yeraze]

Closes #4492, closes #4487.

They land together because #4492 is a prerequisite: a retry loop must not resend a command the node explicitly rejected, and today rejected and timed_out are indistinguishable from succeeded on status alone.

---

#4492 — distinct terminal states

rejected and timed_out split out of succeeded. Both previously settled as succeeded with the real outcome buried in result.ack, so a caller reading only status could not tell "the node confirmed it", "the node refused it" and "we never heard back" apart — even though those demand different treatment.

status	meaning	retried?
succeeded	destination ACKed	no
rejected	destination answered with a routing error	no — it already answered
timed_out	no answer in the ACK window	yes
failed	could not send at all	no

result.ack is still populated exactly as before, so existing consumers — including the frontend's favorite-ACK handling — keep working untouched.

#4487 — ACK-driven auto-retry

Only timed_out is retried. A rejection reached the node and was refused, so resending re-asks a question already answered; a failed operation never reached the radio. This needs no per-command verification logic, so it applies uniformly to every command kind — including ones with no readable state, like reboot.

Attempts resolve from a per-request retryAttempts override → the adminRetryAttempts setting → 1, clamped 1–10.

The default is deliberately 1, i.e. exactly the pre-#4487 behaviour. Every extra attempt is real airtime on a shared band, and silently multiplying every operator's radio traffic isn't a cost to impose by default — retrying is opt-in. Backoff is linear (5s/10s/15s) rather than exponential, because the ACK window is already 30s and doubling would push a third attempt minutes out, long past when the operator is still watching.

Two duplicated-terminal-list traps this surfaced

Both were live bugs, not test noise:

1. followAdminOperation recognised only succeeded/failed as terminal. The new states would have polled an already-settled operation until the 120s timeout.
2. waitForSettled in adminRoutes.asyncOperations.test.ts had the same hardcoded pair, and spun its whole budget once a rejection stopped settling as succeeded. It now calls the service's isTerminal() so it can't drift again.

resolveAdminRetryAttempts is awaited in the request path before the 202, so a settings-read failure is caught and falls back to a single attempt rather than failing the send.

Deliberately not done

AdminCommandsTab still reads result.ack rather than status. The issue lists it in scope, but the ack payload is unchanged, so the UI behaves identically — rewriting it would add risk without changing behaviour. Happy to follow up if you'd rather the migration be complete.

Test plan

• 5 tests for the new terminal states — rejection/timeout settle correctly, both carry a result rather than an error (the send worked; the node refused), all four outcomes terminal and immutable, and only timed_out is retryable.
• 15 tests for retry bounds — default, setting, override precedence, out-of-range rejection on both paths, exact bounds, linear backoff, and resilience when the settings read throws.
• Confirmed real regressions: 18 fail without the change.
• Full Vitest suite — success: true, 13007 passed, 0 failed.
• npx tsc --noEmit clean; npm run lint:ci no FAIL lines.
• docs/features/admin-commands.md Confirmation Semantics updated, as the issue asked.

Credit to @wilhel1812 for the original status model in #4483.

---

Generated by Claude Code

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: feat(admin): distinct ACK outcomes + opt-in auto-retry (#4492, #4487)

Tasks:

• Gather context and understand the PR
• Review adminOperationService.ts — core state machine changes
• Review adminRoutes.ts — retry logic and route changes
• Review test files for coverage and correctness
• Review api.ts frontend changes
• Review settings and docs changes
• Post final review

---

This is a well-structured, carefully-reasoned PR. The design decisions are sound and the implementation is clean. Here's a detailed review:

Code Quality & Design

adminOperationService.ts — The state machine changes are exactly right. Splitting rejected and timed_out out of succeeded makes status load-bearing for the retry logic, which was the prerequisite. The settle() no-op guard (if (!operation || isTerminal(operation.status)) return) correctly prevents late background updates from overwriting settled state. The TERMINAL_STATUSES constant as the authoritative list — consumed by both isTerminal() and exported for tests — is a good pattern that prevents the duplicated-list drift that the PR itself fixed.

adminRoutes.ts retry loop — The retry loop structure is clean:

for (let attempt = 1; ; attempt++) {
  const result = await execute(...);
  if (!ack)                  → succeed
  if (ack.acked)             → succeed  
  if (!ack.timedOut)         → reject   // node refused; no retry
  if (attempt >= maxAttempts) → timeOut
  // else: sleep + retry
}

One minor observation: the void (async () => {...})() detached-task pattern is already established in this codebase, and the catch block correctly handles TX_DISABLED errors. However, the outer catch (error: any) on line ~2010 uses error.message which could silently swallow non-Error objects — but this is a pre-existing pattern, not introduced here.

resolveAdminRetryAttempts — Well done:

• The override fast-path short-circuits before reading settings (correct priority ordering)
• The settings read is wrapped in try/catch so a DB failure degrades gracefully to 1 attempt rather than failing the entire send
• parseInt(raw, 10) with NaN guard correctly rejects non-numeric stored values
• Clamping to [1, 10] via the integer check is clean

adminRetryDelayMs — Math.min(attempt, 3) * 5_000 gives 5s/10s/15s/15s/... Linear is the right call for the reasons stated: the ACK window is 30s, and exponential would push a 3rd attempt far past when the operator is watching.

followAdminOperation fix in api.ts — This is a critical correctness fix. Before #4492, rejected and timed_out weren't in the terminal check, so the client would poll the operation for the full 120s before timing out on its side. The fix correctly adds them to the success branch (returning {success: true, status, ...result}) rather than the error branch — the right call since the send itself worked.

Potential Issues / Observations

1. attempts and maxAttempts merged into the result payload (adminRoutes.ts:1960)

const settled = { ...result, attempts: attempt, maxAttempts };

These fields propagate into operation.result and are returned to the client. If the frontend spreads result into UI state (which the PR says it does via AdminCommandsTab), adding these new fields is backward-compatible but may be unexpected. Worth confirming no consumer is confused by result.attempts appearing.

2. isRetryable is exported but not used inside the retry loop — The retry loop in adminRoutes.ts checks ack.timedOut directly rather than calling isRetryable(status). This is fine (the ack is more direct at that point in the flow), but it means isRetryable is only used in tests. If the retry decision ever needs to change, there are two places to update. Very minor.

3. AdminOperationSnapshot in api.ts is a local copy of server types — The status union type is duplicated between adminOperationService.ts and api.ts:164-172. The PR correctly keeps them in sync, but this is a drift surface. Worth a code comment pointing to the server type as the authoritative source (there's already a comment at line 160 doing this — good, just noting it).

4. The followAdminOperation overall timeout (120_000ms) may now be too short for operations with multiple retries. With adminRetryAttempts=10, worst case: 10 × 30s ACK window + 5+10+15+15+...s backoff = 300s+ on the mesh. A client with the 120s budget would give up well before the server settles. This is arguably acceptable since the client 504s gracefully and the operation is still readable via the poll endpoint, but it's worth documenting or adjusting the timeout for callers that pass a per-request retryAttempts override.

5. The AdminCommandResult interface (api.ts:145) doesn't include attempts/maxAttempts — These fields land in result but aren't typed. Minor: they're covered by [key: string]: unknown on the server-side AdminOperationResult, but the frontend AdminCommandResult interface won't show them in autocompletion.

Test Coverage

The test coverage is thorough and the approach is correct:

• adminOperationService.test.ts: The 5 new ACK-outcome tests cover all 4 terminal states, the result-vs-error distinction for rejected/timed_out, immutability, and the retryable predicate. isRetryable being tested here prevents future drift.
• adminRoutes.retry.test.ts: 15 tests covering bounds, precedence, out-of-range handling, exact bounds, linear backoff, and DB-failure resilience. The mockReset + mockResolvedValue(null) in beforeEach correctly prevents test contamination.
• adminRoutes.asyncOperations.test.ts: The waitForSettled fix (isTerminal instead of hardcoded list) is exactly right — the comment explaining the old trap is valuable. The existing coverage of rejection (errorReason: 34) correctly checks settled.result.ack.status isn't 'confirmed', though it doesn't assert settled.status === 'rejected' explicitly (minor gap).
• The PR reports 18 tests fail without the changes — a good validation that the tests are catching real regressions, not just passing trivially.

Security

No concerns. The operation ownership check at GET /operations/:id correctly treats null !== null as false (preventing two session-less principals from sharing visibility). The retry logic operates entirely server-side and doesn't introduce any new external inputs.

Performance

The sweep-on-access pattern (rather than a background timer) for the in-memory operation registry remains correct and unchanged. The retry delay await new Promise((resolve) => setTimeout(resolve, adminRetryDelayMs(attempt))) runs inside the detached async IIFE, so it doesn't block any other requests.

Summary

This is a solid, well-tested PR that correctly separates three previously-indistinguishable outcomes and adds a conservative, opt-in retry mechanism. The prerequisite relationship between the two issues is well-explained. The "deliberately not done" note about the UI migration is honest and the risk/reward assessment is correct.

The items worth a follow-up:

• Consider whether the followAdminOperation 120s client timeout needs to grow when retryAttempts > 1 is passed as a per-request override
• Consider typing attempts/maxAttempts in AdminCommandResult for discoverability

Neither blocks merging. LGTM.