feat(delivery): TOOL_INSTALLATION — first delivery type end to end (PR 2 of the plan)

[semen-flamingo]

First delivery type on the engine merged in #2200: TOOL_INSTALLATION end to end, behind a per-type flag with a real rollback. Four commits on top of main (the last three are review follow-ups), 42 files.

The flag

openframe.delivery.enabled.TOOL_INSTALLATION is checked at the call site, ToolInstallationService.publish. A type not listed in that map is off: there is no defaults-level switch, so nothing can turn every type on at once.

if (deliveryProperties.isEnabled(DeliveryType.TOOL_INSTALLATION)) {
    deliveryDispatcher.dispatch(new ToolInstallationDeliverySeed(machineId, toolAgent, tool, reinstall));   // row + core publish
    return;
}
toolInstallationNatsPublisher.publish(machineId, toolAgent, tool, reinstall);   // yesterday's code, untouched: JetStream publish, no row

• off (every environment until the rollout PR): the old publisher runs exactly as on main today — same class, same js.publish, no row, no retries. Flipping the flag back off is the rollback for this type; nothing else has to move.
• on: the dispatcher records the row and publishes through the spec with core publish on the same subject (the stream still stores it, so agents that still hold a durable keep receiving it); the sweep on client retries; the agent's results on machine.{id}.delivery.result close the row.

Rollout order: the agent release that reports delivery results goes to the fleet first, the flag flips after. An agent that predates the release never reports, so with the flag on it would be re-sent to three times through its durable and the row would end FAILED/EXHAUSTED — bounded, visible, and only possible for a machine that was offline through the whole agent rollout and gets a tool command in its first minute back online. No version gate for that (was in an earlier revision, dropped on review).

The global openframe.delivery.enabled from #2200 is gone: one switch per type (enabled.<TYPE>, missing = off) and sweep.enabled for the sweep on client. Every next type gets the same call-site switch with its old code kept as the off-path until that type's cleanup.

openframe:
  delivery:
    sweep:
      enabled: true          # client only
      interval: 30000
      lock-at-most-for: 2m
      lock-at-least-for: 10s
      batch-size: 500
    enabled:
      TOOL_INSTALLATION: false   # per environment, in the rollout PR
    defaults:
      ack-threshold-seconds: 30
      max-attempts: 3
      backoff-multiplier: 2
      max-retry-interval-seconds: 300
      offline-behavior: RETRY_ON_RECONNECT
      reconnect-window-seconds: 86400
      result-timeout-seconds: 600
      ttl-seconds: 604800

What this PR adds

The spec. ToolInstallationDeliverySpec in openframe-data-nats (which now depends on openframe-machine-delivery) with its seed ToolInstallationDeliverySeed(machineId, toolAgent, tool, reinstall): request(seed) maps the payload with the two data-nats mappers and sets targetId = toolAgent.getKey(), publish sends on machine.{id}.tool-installation with core publish. It builds the message itself; the duplication with ToolInstallationNatsPublisher.buildMessage is deliberate and temporary — the old publisher stays byte-identical as the off-path and is deleted with this type's cleanup.

The delivery block. Every dispatch() mints a random dispatchId and stamps the command with one block the agent never has to interpret:

"delivery": { "type": "TOOL_INSTALLATION", "targetId": "fleetmdm-agent", "dispatchId": "…" }

DeliveryRef is that block; DeliveryPayload is the one-property interface every payload class implements (ToolInstallationMessage included, @JsonInclude(NON_NULL) so the old path's bytes do not change). The dispatcher sets it, the recorder stores the dispatchId on the row, the sweep re-sends the stored payloadJson — so every re-send of the same dispatch carries the same block and the agent can dedupe on it (re-report, do not reinstall).

One generic result endpoint for every type. The agent reports on machine.{id}.delivery.result (DeliveryResultMessage, JetStream stream DELIVERY_RESULT added to the stream initializer):

{ "delivery": { "type": "TOOL_INSTALLATION", "targetId": "fleetmdm-agent", "dispatchId": "…" }, "result": "ACKED" }
{ "delivery": { "type": "TOOL_INSTALLATION", "targetId": "fleetmdm-agent", "dispatchId": "…" }, "result": "DONE" }
{ "delivery": { "type": "TOOL_INSTALLATION", "targetId": "fleetmdm-agent", "dispatchId": "…" }, "result": "FAILED", "error": "…" }

The delivery block is the command's block copied back verbatim; the agent knows nothing about types or targets.

DeliveryResultListener in client-core routes to DeliveryTracker.acknowledge / complete / fail. Every transition is a conditional update that matches the row's current dispatchId: a late report for the previous dispatch of the same command (reinstall twice) cannot ack or close the new row, and a report without a dispatchId does nothing. FAILED closes the row with failure = AGENT_ERROR and the agent's error text, counts the metric and calls the spec's onFailed. A report without the delivery block, with a field missing in it, or with a type/result this server does not know (reads as null), is a contract violation by the agent: it is rejected with an error log and the counter openframe.delivery.result.rejected{reason=incomplete|malformed} (alert candidate), then acked so it cannot poison the consumer; a malformed payload goes the same way; a storage error leaves the message unacked for redelivery. No business listener is touched: InstalledAgentService and ScriptExecutionAcknowledgeListener are as on main.

Offline machines without an event listener. A row whose machine is not online is postponed to the next sweep tick (sweep.interval, capped at the reconnect window end) instead of being parked for max-retry-interval and woken by a device-online event. A machine that comes back is picked up within one tick; the parked flag, wake and the {tenantId, machineId} index are gone.

Engine changes riding along: DeliveryRef/DeliveryPayload + dispatchId on the row and in markAcked/markDone/markFailed; DeliveryCloser moved to track (unconditional: the tracker closes agent-reported failures through it, the sweep still closes exhausted/offline/timeout ones); DeliveryProperties.isEnabled(type) over the per-type enabled map; Sweep.interval bound as a property (same key the scheduler already reads); the recorder no longer checks a flag (dispatch is the new path); the sweep package is conditional on sweep.enabled alone.

Agent contract (for the agent team)

1. Subscribe to machine.{machineId}.tool-installation with a plain (core) subscription, no durable consumer.
2. On receipt publish machine.{machineId}.delivery.result right away: the command's delivery block copied as is plus result: ACKED. A command without a delivery block (old path) is not reported.
3. If the same delivery.dispatchId was already processed: report ACKED again, do not reinstall.
4. After the install: DONE; on failure: FAILED with error.
5. installed-agent keeps being sent as today (installed_agents bookkeeping); it no longer closes deliveries.

Tests

ToolInstallationDeliverySpecTest (request mapping, empty-string defaults, core publish), DeliveryResultListenerTest (ACKED/DONE/FAILED routing; missing block, missing dispatchId, unknown result, unknown type and malformed payload rejected and counted; storage error left unacked), DeliveryMetricsTest, DeliveryPropertiesTest (type not listed / listed off / listed on), DeliveryTrackerTest/DeliveryCloserTest (dispatchId on complete, agent-reported failure), DeliveryDispatcherTest/DeliveryRecorderTest (dispatchId minted and stored), DeliverySweepServiceTest (offline rows postponed to the next tick), CustomMachineDeliveryRepositoryImplTest (dispatchId criteria on ack and agent failure). ToolInstallationService has no unit test because openframe-tool-agent-nats-installation has no test infrastructure at all.

Verification

mvn -pl openframe-data-mongo-sync,openframe-machine-delivery,openframe-data-nats,openframe-client-core,openframe-api-service-core,openframe-management-service-core,openframe-tool-agent-nats-installation -am test -Dtest='Delivery*Test,*DeliverySpecTest,CustomMachineDeliveryRepositoryImplTest,MachineOnlineStatusTest,DeliveryResultListenerTest' — 112 delivery-related tests, build green on JDK 21.

Verified on a feature tenant (openframe-saas-tenant#3330, dev, 2026-09-24)

Real macOS agent of today's release (no delivery.result yet), enabled.TOOL_INSTALLATION: true:

Case	Result
Enrollment: 3 tools dispatched through the engine	3 rows PENDING, each with its own dispatchId, old publisher not called
Retries	re-sent after ~30 s, 60 s, 120 s; FAILED/EXHAUSTED after 3 attempts (expected: this agent never reports)
Old agent on repeated install commands	"already installed … skipping" — harmless
Reinstall (force endpoint)	same row reopened, new dispatchId, attempts from 0
Old agent on repeated reinstall commands	uninstalls and reinstalls on every retry (4× in one cycle) — see rollout rule below
Machine offline	row stays PENDING, attempts stay 0, re-checked every sweep tick (30 s); sent on the first tick after the machine is back ONLINE
Flag switched off	old JetStream publisher, message without the delivery block, row not touched
Re-dispatch of a closed row	bug found and fixed in this PR: the reopened row kept failure/finishedAt from the previous dispatch; upsertPending now unsets ackedAt, finishedAt, failure, error

Rollout rule this confirms: the agent release that reports on delivery.result (and dedupes by delivery.dispatchId) goes to the fleet first, the flag flips after. An agent that predates it gets every retry of a reinstall as a fresh reinstall.

Tenant side needed for the rollout (in openframe-saas-tenant#3330): openframe-data-nats in the root dependencyManagement — saas-lib declares it at saas-lib's own oss version one hop closer than the delivery path, and Maven's nearest-wins otherwise drops openframe-machine-delivery from saas-api (NoClassDefFoundError DeliverySeed); NATS device permissions machine.*.tool-installation (subscribe) and machine.*.delivery.result (publish).

🤖 Generated with Claude Code

https://claude.ai/code/session_01BnYUpYgKvuMw6hB5VZSimY

[github-actions]

🦩 Flamingo Code Review

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

Mode: advisory · Rules cited: OFJAVA-013 · 6 defect(s) outside any rule

Inline comments: 6 new

Findings without an inline anchor in this diff

• 🟠 [warn/recommended] openframe-client-core/src/main/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListener.java:62 — handleMessage swallows all exceptions and never acks nor nacks the message on failure
  When objectMapper.readValue throws (malformed JSON) or deliveryTracker.acknowledge/acknowledgeService.acknowledge throws, the catch block only logs and never calls message.ack() nor any explicit nack/term. Depending on the underlying JetStream push consumer's ack policy and redelivery settings, this may cause the message to be redelivered indefinitely (poison message) or silently dropped depending on ack wait timeout — this behavior existed before this diff for the malformed-JSON case, but the diff adds two new failure surfaces (deliveryTracker.acknowledge and acknowledgeService.acknowledge) that can now throw inside the same try block. A RuntimeException from deliveryTracker.acknowledge (e.g., if the DB is unreachable) will now abandon the ack, causing legacy script acknowledgment side effects to also be skipped and the message parked for redelivery, changing prior single-responsibility failure isolation.

      protected void handleMessage(Message message) {
          String payload = new String(message.getData(), StandardCharsets.UTF_8);
          try {
              ScriptExecutionAcknowledgeMessage ack = objectMapper.readValue(payload, ScriptExecutionAcknowledgeMessage.class);
              if (isDeliveryAck(ack)) {
                  deliveryTracker.acknowledge(ack.getType(), ack.getTargetId(), ack.getMachineId(), ack.getDispatchId());
              }
              if (isScriptAck(ack)) {
  

---

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-22 20:29 UTC · updated 2026-09-22 20:30 UTC · workflow run