feat(delivery): CLIENT_UNINSTALL — second delivery type, machine leaves service on the agent's ACK

[semen-flamingo]

Second type on the delivery engine after TOOL_INSTALLATION (#2212). Flag openframe.delivery.enabled.CLIENT_UNINSTALL, off by default: the old path (ClientUninstallNatsPublisher + PENDING_DELETION at send time) stays byte for byte.

Why the machine status moves

Today ForceClientUninstallService sets PENDING_DELETION right after publishing. From that moment MachineStatusService ignores the machine's heartbeats, so for the sweep it is never ONLINE and no retry would ever happen. On the new path:

Step	Who	What
dispatch	api, ForceClientUninstallService	row CLIENT_UNINSTALL:openframe-client:{machineId} + core publish on the same subject machine.{id}.client-uninstall; status untouched; a machine already in PENDING_DELETION/DELETED is skipped
retries	client, sweep	30/60/120 s, 3 attempts, offline waits up to 24 h — the machine still reports its real status
ACKED	spec.onAcked	PENDING_DELETION — the command is on the machine, retries are over
uninstall done	agent → POST /agent/uninstall → AgentUninstallService	DELETED as today + deliveryTracker.done(new ClientUninstallDeliverySeed(machineId)) closes the row DONE
FAILED from the agent, or 600 s after ACK without a result	spec.onFailed	machine still PENDING_DELETION → back to OFFLINE (next heartbeat sets ONLINE), ERROR log + delivery.failed metric; DELETED → nothing, the uninstall happened; never acked → nothing to revert

Engine changes

• DeliverySpec: targetId(seed) — one place of truth for the row key, request() uses it — and onAcked(row). Both hooks are abstract; ToolInstallationDeliverySpec implements them empty.
• DeliverySeed.machineId().
• DeliveryTracker now takes the two identities the callers actually hold: agent results come in as the DeliveryRef the agent copied back — acknowledge(ref, machineId), done(ref, machineId), fail(ref, machineId, error), CAS on dispatchId; a completion the server learns outside the result channel comes in as the DeliverySeed that was dispatched — done(seed), the key resolved through the spec, CAS on status only. complete is renamed to done to match the row status. acknowledge notifies spec.onAcked once, on the real PENDING → ACKED transition only.
• DeliveryTracker.cancel and the repository overload only it used are removed: no callers, the sweep cancels through DeliveryCloser.
• Repository gains markDone without dispatchId.
• ClientUninstallMessage implements DeliveryPayload; the delivery block is @JsonInclude(NON_NULL).

Agent contract for this type (Denys)

• On receipt: machine.{id}.delivery.result with the command's delivery block and result: ACKED.
• On failure: result: FAILED + error.
• Same dispatchId again → ACKED again, do not run the uninstall twice.
• DONE is optional: the existing POST /agent/uninstall closes the row.
• The CLIENT_UNINSTALL durable stays until the flag is on everywhere, same as for tool installation.

Tenant side (separate PR)

Device ACL: subscribe on machine.*.client-uninstall (today only .inbox is allowed); flag CLIENT_UNINSTALL: false everywhere until the agent release.

Tests

1 171 green locally: machine-delivery 52, data-mongo-sync 308, data-nats 85, client-core 242, api-service-core 484. New: ClientUninstallDeliverySpecTest, ForceClientUninstallServiceTest, AgentUninstallServiceTest, plus tracker/repository cases for onAcked and done(seed).

🤖 Generated with Claude Code

https://claude.ai/code/session_01SnaktPhqs3aeuuUHdXCAjX

[github-actions]

🦩 Flamingo Code Review

2 finding(s) — 1 action required · 0 recommended · 1 informational

Mode: advisory · Checks: missing-test ×1 · 1 defect(s) outside any rule

Inline comments: 1 new

Findings without an inline anchor in this diff

• 🔵 [info/informational] missing-test Tool-checked openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/CustomMachineDeliveryRepository.java:11 — CustomMachineDeliveryRepository is changed and no test references it
  No test file in this checkout (646 scanned) mentions CustomMachineDeliveryRepository. Add or extend a test before merging.

  public interface CustomMachineDeliveryRepository {
  

---

Need another pass? Commits pushed after this review are not reviewed automatically.

• Review the new commits — the commits added since this review
• Review the whole diff again — ignoring what was already reviewed

Prefer typing? Comment @flamingo-review, or @flamingo-review full. To review every push on this pull request, add the flamingo-review-always label.

React 👍/👎 on inline comments to teach the reviewer.

Started 2026-09-29 13:42 UTC · updated 2026-09-29 13:44 UTC · workflow run

[github-actions]

🦩 🔴 [error/action_required] markDone(id, from, finishedAt, expiresAt) never rejects a dispatchId-mismatched row, defeating fencing

The new markDone(String id, Set from, Instant finishedAt, Instant expiresAt) overload matches with stillIn(id, from), i.e. any row for this delivery id currently in one of the from statuses — it does not check dispatchId at all. The dispatchId-aware overload (markDone(id, dispatchId, from, finishedAt, expiresAt)) exists specifically to make sure a DONE result only closes the dispatch it actually belongs to (thisDispatch(id, from, dispatchId)); it prevents a stale/duplicate NATS message from an earlier dispatch attempt from completing a delivery row that has since been redispatched with a new dispatchId. The no-dispatchId overload added here is used by AgentUninstallService via DeliveryTracker#done(DeliverySeed) (client-uninstall, no dispatchId known at that call site), but it removes that fencing for that whole path: if the client-uninstall delivery was retried and re-dispatched (new dispatchId, machine came back online and NATS republished), a late/duplicate ACK-less completion signal (or the deregister endpoint being invoked twice) could close a row against the wrong/newer dispatch attempt, since the query only checks id + status, not which physical dispatch actually finished. This is a real behavioural gap introduced by the new overload versus the existing dispatchId-fenced sibling method.

Evidence

    @Override
    public boolean markDone(String id, Set<DeliveryStatus> from, Instant finishedAt, Instant expiresAt) {
        Update update = closed(DeliveryStatus.DONE, finishedAt, expiresAt);
        return updateOne(stillIn(id, from), update);
    }

🤖 Prompt for AI agents

In openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java around lines 113-117, address this code-review finding: markDone(id, from, finishedAt, expiresAt) never rejects a dispatchId-mismatched row, defeating fencing.
The new markDone(String id, Set<DeliveryStatus> from, Instant finishedAt, Instant expiresAt) overload matches with stillIn(id, from), i.e. any row for this delivery id currently in one of the `from` statuses — it does not check dispatchId at all. The dispatchId-aware overload (markDone(id, dispatchId, from, finishedAt, expiresAt)) exists specifically to make sure a DONE result only closes the dispatch it actually belongs to (thisDispatch(id, from, dispatchId)); it prevents a stale/duplicate NATS message from an earlier dispatch attempt from completing a delivery row that has since been redispatched with a new dispatchId. The no-dispatchId overload added here is used by AgentUninstallService via DeliveryTracker#done(DeliverySeed) (client-uninstall, no dispatchId known at that call site), but it removes that fencing for that whole path: if the client-uninstall delivery was retried and re-dispatched (new dispatchId, machine came back online and NATS republished), a late/duplicate ACK-less completion signal (or the deregister endpoint being invoked twice) could close a row against the wrong/newer dispatch attempt, since the query only checks id + status, not which physical dispatch actually finished. This is a real behavioural gap introduced by the new overload versus the existing dispatchId-fenced sibling method.
The flagged code:
```
    @Override
    public boolean markDone(String id, Set<DeliveryStatus> from, Instant finishedAt, Instant expiresAt) {
        Update update = closed(DeliveryStatus.DONE, finishedAt, expiresAt);
        return updateOne(stillIn(id, from), update);
    }
```
Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: judge 75 · author 45 — react 👍/👎 to teach the reviewer