From 9e8c767d16daa855f3808ba650743831a0e85cf1 Mon Sep 17 00:00:00 2001 From: semen-flamingo Date: Tue, 29 Sep 2026 15:41:17 +0200 Subject: [PATCH 1/9] =?UTF-8?q?feat(delivery):=20CLIENT=5FUNINSTALL=20?= =?UTF-8?q?=E2=80=94=20second=20delivery=20type,=20machine=20leaves=20serv?= =?UTF-8?q?ice=20on=20the=20agent's=20ACK?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ClientUninstallDeliverySpec behind openframe.delivery.enabled.CLIENT_UNINSTALL; flag off keeps the old JetStream publisher and PENDING_DELETION-at-send byte for byte. On the new path PENDING_DELETION is set in spec.onAcked, so the sweep still sees the machine's real status while retrying; a failure after the ACK hands the machine back as OFFLINE. The agent's own POST /agent/uninstall closes the row through DeliveryTracker.done(seed). DeliverySpec gains targetId(seed) and onAcked; DeliverySeed gains machineId(); DeliveryTracker.complete is renamed to done to match the row status. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01SnaktPhqs3aeuuUHdXCAjX --- .../service/ForceClientUninstallService.java | 21 +++ .../ForceClientUninstallServiceTest.java | 106 ++++++++++++ .../delivery/DeliveryResultListener.java | 2 +- .../client/service/AgentUninstallService.java | 4 + .../delivery/DeliveryResultListenerTest.java | 2 +- .../service/AgentUninstallServiceTest.java | 78 +++++++++ .../CustomMachineDeliveryRepository.java | 2 + .../CustomMachineDeliveryRepositoryImpl.java | 6 + ...stomMachineDeliveryRepositoryImplTest.java | 21 +++ .../delivery/ClientUninstallDeliverySeed.java | 23 +++ .../delivery/ClientUninstallDeliverySpec.java | 94 +++++++++++ .../ToolInstallationDeliverySeed.java | 5 + .../ToolInstallationDeliverySpec.java | 19 ++- .../nats/model/ClientUninstallMessage.java | 8 +- .../ClientUninstallDeliverySpecTest.java | 152 ++++++++++++++++++ .../openframe/delivery/spec/DeliverySeed.java | 2 + .../openframe/delivery/spec/DeliverySpec.java | 4 + .../delivery/track/DeliveryTracker.java | 33 +++- .../com/openframe/delivery/spec/TestSeed.java | 5 + .../delivery/track/DeliveryTrackerTest.java | 59 ++++++- 20 files changed, 634 insertions(+), 12 deletions(-) create mode 100644 openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java create mode 100644 openframe-client-core/src/test/java/com/openframe/client/service/AgentUninstallServiceTest.java create mode 100644 openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySeed.java create mode 100644 openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java create mode 100644 openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java diff --git a/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java b/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java index ab5bb25613..b5e3febe57 100644 --- a/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java +++ b/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java @@ -4,9 +4,13 @@ import com.openframe.api.dto.force.response.ForceAgentStatus; import com.openframe.api.dto.force.response.ForceClientUninstallResponse; import com.openframe.api.dto.force.response.ForceClientUninstallResponseItem; +import com.openframe.data.document.delivery.DeliveryType; import com.openframe.data.document.device.Machine; +import com.openframe.data.nats.delivery.ClientUninstallDeliverySeed; import com.openframe.data.nats.publisher.ClientUninstallNatsPublisher; import com.openframe.data.repository.device.MachineRepository; +import com.openframe.delivery.config.DeliveryProperties; +import com.openframe.delivery.dispatch.DeliveryDispatcher; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.stereotype.Service; @@ -25,6 +29,8 @@ public class ForceClientUninstallService { private final ClientUninstallNatsPublisher clientUninstallNatsPublisher; private final MachineRepository machineRepository; + private final DeliveryProperties deliveryProperties; + private final DeliveryDispatcher deliveryDispatcher; public ForceClientUninstallResponse process(ForceClientUninstallRequest request) { List machineIds = request.getMachineIds(); @@ -57,6 +63,11 @@ private ForceClientUninstallResponseItem processMachine(String machineId) { return buildResponseItem(machineId, ForceAgentStatus.FAILED); } + if (deliveryProperties.isEnabled(DeliveryType.CLIENT_UNINSTALL)) { + dispatch(machine); + return buildResponseItem(machineId, ForceAgentStatus.PROCESSED); + } + clientUninstallNatsPublisher.publish(machineId); markPendingDeletion(machine); @@ -68,6 +79,16 @@ private ForceClientUninstallResponseItem processMachine(String machineId) { } } + // the machine leaves service on the agent's ACK (spec.onAcked), not here: the sweep needs its real status to retry + private void dispatch(Machine machine) { + String machineId = machine.getMachineId(); + if (machine.getStatus() == PENDING_DELETION) { + log.info("Client uninstall already in progress for machine {}", machineId); + return; + } + deliveryDispatcher.dispatch(new ClientUninstallDeliverySeed(machineId)); + } + private void markPendingDeletion(Machine machine) { if (machine.getStatus() == PENDING_DELETION) { return; diff --git a/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java b/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java new file mode 100644 index 0000000000..92bb4f8cf7 --- /dev/null +++ b/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java @@ -0,0 +1,106 @@ +package com.openframe.api.service; + +import com.openframe.api.dto.force.request.ForceClientUninstallRequest; +import com.openframe.api.dto.force.response.ForceAgentStatus; +import com.openframe.api.dto.force.response.ForceClientUninstallResponse; +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.data.document.device.DeviceStatus; +import com.openframe.data.document.device.Machine; +import com.openframe.data.nats.delivery.ClientUninstallDeliverySeed; +import com.openframe.data.nats.publisher.ClientUninstallNatsPublisher; +import com.openframe.data.repository.device.MachineRepository; +import com.openframe.delivery.config.DeliveryProperties; +import com.openframe.delivery.dispatch.DeliveryDispatcher; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.util.List; +import java.util.Optional; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class ForceClientUninstallServiceTest { + + private static final String MACHINE_ID = "mach-42"; + + @Mock private ClientUninstallNatsPublisher clientUninstallNatsPublisher; + @Mock private MachineRepository machineRepository; + @Mock private DeliveryProperties deliveryProperties; + @Mock private DeliveryDispatcher deliveryDispatcher; + + @Captor private ArgumentCaptor seedCaptor; + + @InjectMocks private ForceClientUninstallService service; + + private Machine machine; + private ForceClientUninstallRequest request; + + @BeforeEach + void setUp() { + machine = new Machine(); + machine.setMachineId(MACHINE_ID); + machine.setStatus(DeviceStatus.ONLINE); + request = new ForceClientUninstallRequest(); + request.setMachineIds(List.of(MACHINE_ID)); + when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); + } + + @Test + void process_flagOff_publishedToJetStreamAndMarkedPendingDeletion() { + // setup + when(deliveryProperties.isEnabled(DeliveryType.CLIENT_UNINSTALL)).thenReturn(false); + + // execution + ForceClientUninstallResponse response = service.process(request); + + // verifications + verify(clientUninstallNatsPublisher).publish(MACHINE_ID); + assertThat(machine.getStatus()).isEqualTo(DeviceStatus.PENDING_DELETION); + verify(machineRepository).save(machine); + verifyNoInteractions(deliveryDispatcher); + assertThat(response.getItems().get(0).getStatus()).isEqualTo(ForceAgentStatus.PROCESSED); + } + + @Test + void process_flagOn_dispatchedThroughEngineStatusUntouched() { + // setup + when(deliveryProperties.isEnabled(DeliveryType.CLIENT_UNINSTALL)).thenReturn(true); + + // execution + ForceClientUninstallResponse response = service.process(request); + + // verifications + verify(deliveryDispatcher).dispatch(seedCaptor.capture()); + assertThat(seedCaptor.getValue().getMachineId()).isEqualTo(MACHINE_ID); + assertThat(machine.getStatus()).isEqualTo(DeviceStatus.ONLINE); + verify(machineRepository, never()).save(any()); + verifyNoInteractions(clientUninstallNatsPublisher); + assertThat(response.getItems().get(0).getStatus()).isEqualTo(ForceAgentStatus.PROCESSED); + } + + @Test + void process_flagOnUninstallAlreadyAcknowledged_notDispatchedAgain() { + // setup + machine.setStatus(DeviceStatus.PENDING_DELETION); + when(deliveryProperties.isEnabled(DeliveryType.CLIENT_UNINSTALL)).thenReturn(true); + + // execution + ForceClientUninstallResponse response = service.process(request); + + // verifications + verifyNoInteractions(deliveryDispatcher, clientUninstallNatsPublisher); + assertThat(response.getItems().get(0).getStatus()).isEqualTo(ForceAgentStatus.PROCESSED); + } +} diff --git a/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeliveryResultListener.java b/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeliveryResultListener.java index 99a5657f5d..b6c2d3a7f8 100644 --- a/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeliveryResultListener.java +++ b/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeliveryResultListener.java @@ -101,7 +101,7 @@ private void apply(String machineId, DeliveryResultMessage report) { String dispatchId = delivery.getDispatchId(); switch (report.getResult()) { case ACKED -> deliveryTracker.acknowledge(type, targetId, machineId, dispatchId); - case DONE -> deliveryTracker.complete(type, targetId, machineId, dispatchId); + case DONE -> deliveryTracker.done(type, targetId, machineId, dispatchId); case FAILED -> deliveryTracker.fail(type, targetId, machineId, dispatchId, report.getError()); } } diff --git a/openframe-client-core/src/main/java/com/openframe/client/service/AgentUninstallService.java b/openframe-client-core/src/main/java/com/openframe/client/service/AgentUninstallService.java index d0b0231240..a57a88b1b3 100644 --- a/openframe-client-core/src/main/java/com/openframe/client/service/AgentUninstallService.java +++ b/openframe-client-core/src/main/java/com/openframe/client/service/AgentUninstallService.java @@ -4,8 +4,10 @@ import com.openframe.data.document.device.DeviceStatus; import com.openframe.data.document.device.Machine; import com.openframe.data.document.oauth.OAuthClient; +import com.openframe.data.nats.delivery.ClientUninstallDeliverySeed; import com.openframe.data.repository.device.MachineRepository; import com.openframe.data.repository.oauth.OAuthClientRepository; +import com.openframe.delivery.track.DeliveryTracker; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.stereotype.Service; @@ -22,6 +24,7 @@ public class AgentUninstallService { private final MachineRepository machineRepository; private final ToolConnectionService toolConnectionService; private final InstalledAgentService installedAgentService; + private final DeliveryTracker deliveryTracker; public void uninstall(String machineId, String clientSecret) { Optional client = oauthClientRepository.findByMachineId(machineId); @@ -48,6 +51,7 @@ public void uninstall(String machineId, String clientSecret) { toolConnectionService.disconnectAll(machineId); installedAgentService.disconnectAll(machineId); + deliveryTracker.done(new ClientUninstallDeliverySeed(machineId)); log.info("Machine {} deregistered on uninstall", machineId); } diff --git a/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeliveryResultListenerTest.java b/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeliveryResultListenerTest.java index 1572ba422a..9de2cfd673 100644 --- a/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeliveryResultListenerTest.java +++ b/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeliveryResultListenerTest.java @@ -77,7 +77,7 @@ void handleMessage_done_trackerCompletesDispatch() { listener.handleMessage(message); // verifications - verify(deliveryTracker).complete(DeliveryType.TOOL_INSTALLATION, TOOL_AGENT_ID, MACHINE_ID, DISPATCH_ID); + verify(deliveryTracker).done(DeliveryType.TOOL_INSTALLATION, TOOL_AGENT_ID, MACHINE_ID, DISPATCH_ID); verify(message).ack(); } diff --git a/openframe-client-core/src/test/java/com/openframe/client/service/AgentUninstallServiceTest.java b/openframe-client-core/src/test/java/com/openframe/client/service/AgentUninstallServiceTest.java new file mode 100644 index 0000000000..59747c99ab --- /dev/null +++ b/openframe-client-core/src/test/java/com/openframe/client/service/AgentUninstallServiceTest.java @@ -0,0 +1,78 @@ +package com.openframe.client.service; + +import com.openframe.client.service.validator.ClientSecretValidator; +import com.openframe.data.document.device.DeviceStatus; +import com.openframe.data.document.device.Machine; +import com.openframe.data.document.oauth.OAuthClient; +import com.openframe.data.nats.delivery.ClientUninstallDeliverySeed; +import com.openframe.data.repository.device.MachineRepository; +import com.openframe.data.repository.oauth.OAuthClientRepository; +import com.openframe.delivery.track.DeliveryTracker; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.util.Optional; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class AgentUninstallServiceTest { + + private static final String MACHINE_ID = "mach-42"; + private static final String CLIENT_SECRET = "secret"; + + @Mock private OAuthClientRepository oauthClientRepository; + @Mock private ClientSecretValidator clientSecretValidator; + @Mock private MachineRepository machineRepository; + @Mock private ToolConnectionService toolConnectionService; + @Mock private InstalledAgentService installedAgentService; + @Mock private DeliveryTracker deliveryTracker; + + @Captor private ArgumentCaptor seedCaptor; + + @InjectMocks private AgentUninstallService service; + + private Machine machine; + + @BeforeEach + void setUp() { + machine = new Machine(); + machine.setMachineId(MACHINE_ID); + machine.setStatus(DeviceStatus.PENDING_DELETION); + when(oauthClientRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(new OAuthClient())); + when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); + } + + @Test + void uninstall_machineInService_deletedAndDeliveryRowClosed() { + // execution + service.uninstall(MACHINE_ID, CLIENT_SECRET); + + // verifications + assertThat(machine.getStatus()).isEqualTo(DeviceStatus.DELETED); + verify(machineRepository).save(machine); + verify(deliveryTracker).done(seedCaptor.capture()); + assertThat(seedCaptor.getValue().machineId()).isEqualTo(MACHINE_ID); + } + + @Test + void uninstall_machineAlreadyDeleted_nothingTouched() { + // setup + machine.setStatus(DeviceStatus.DELETED); + + // execution + service.uninstall(MACHINE_ID, CLIENT_SECRET); + + // verifications + verifyNoInteractions(deliveryTracker, toolConnectionService, installedAgentService); + } +} diff --git a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/CustomMachineDeliveryRepository.java b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/CustomMachineDeliveryRepository.java index 8b6c14da93..b92e4b815b 100644 --- a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/CustomMachineDeliveryRepository.java +++ b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/CustomMachineDeliveryRepository.java @@ -25,6 +25,8 @@ public interface CustomMachineDeliveryRepository { boolean markDone(String id, String dispatchId, Set from, Instant finishedAt, Instant expiresAt); + boolean markDone(String id, Set from, Instant finishedAt, Instant expiresAt); + boolean markCancelled(String id, Set from, Instant finishedAt, Instant expiresAt); boolean markCancelled(String id, Set from, Instant dispatchedAt, Instant finishedAt, Instant expiresAt); diff --git a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java index d524096044..bf8abe1e6f 100644 --- a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java +++ b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java @@ -110,6 +110,12 @@ public boolean markDone(String id, String dispatchId, Set from, return updateOne(thisDispatch(id, from, dispatchId), update); } + @Override + public boolean markDone(String id, Set from, Instant finishedAt, Instant expiresAt) { + Update update = closed(DeliveryStatus.DONE, finishedAt, expiresAt); + return updateOne(stillIn(id, from), update); + } + @Override public boolean markCancelled(String id, Set from, Instant finishedAt, Instant expiresAt) { Update update = closed(DeliveryStatus.CANCELLED, finishedAt, expiresAt); diff --git a/openframe-data-mongo-sync/src/test/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImplTest.java b/openframe-data-mongo-sync/src/test/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImplTest.java index 022540e6b6..9f47cc9e00 100644 --- a/openframe-data-mongo-sync/src/test/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImplTest.java +++ b/openframe-data-mongo-sync/src/test/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImplTest.java @@ -162,6 +162,27 @@ void markAcked_unackedRowOfThisDispatch_ackedAndTrue() { .contains("dueAt"); } + @Test + void markDone_withoutDispatchId_completableRowClosedPayloadDropped() { + // setup + UpdateResult oneRow = UpdateResult.acknowledged(1, 1L, null); + when(mongoTemplate.updateFirst(queryCaptor.capture(), updateCaptor.capture(), eq(MachineDelivery.class))).thenReturn(oneRow); + + // execution + boolean done = repository.markDone(ID, DeliveryStatus.COMPLETABLE, now, now); + + // verifications + assertThat(done).isTrue(); + assertThat(queryCaptor.getValue().getQueryObject().toString()) + .contains(ID) + .contains("PENDING") + .contains("FAILED") + .doesNotContain("dispatchId"); + assertThat(updateCaptor.getValue().getUpdateObject().toString()) + .contains("DONE") + .contains("payloadJson"); + } + @Test void postponeAfterError_pendingRow_errorCountedAndDueMoved() { // setup diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySeed.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySeed.java new file mode 100644 index 0000000000..4c16797476 --- /dev/null +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySeed.java @@ -0,0 +1,23 @@ +package com.openframe.data.nats.delivery; + +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.delivery.spec.DeliverySeed; +import lombok.AllArgsConstructor; +import lombok.Getter; + +@Getter +@AllArgsConstructor +public class ClientUninstallDeliverySeed implements DeliverySeed { + + private final String machineId; + + @Override + public DeliveryType type() { + return DeliveryType.CLIENT_UNINSTALL; + } + + @Override + public String machineId() { + return machineId; + } +} diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java new file mode 100644 index 0000000000..60916c2e62 --- /dev/null +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java @@ -0,0 +1,94 @@ +package com.openframe.data.nats.delivery; + +import com.openframe.data.document.delivery.DeliveryFailure; +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.data.document.delivery.MachineDelivery; +import com.openframe.data.document.device.DeviceStatus; +import com.openframe.data.document.device.Machine; +import com.openframe.data.nats.model.ClientUninstallMessage; +import com.openframe.data.repository.device.MachineRepository; +import com.openframe.delivery.spec.DeliveryRequest; +import com.openframe.delivery.spec.DeliverySpec; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.stereotype.Component; + +import java.time.Instant; + +import static java.lang.String.format; + +@Slf4j +@Component +@RequiredArgsConstructor +@ConditionalOnProperty("spring.cloud.stream.enabled") +public class ClientUninstallDeliverySpec implements DeliverySpec { + + private static final String TARGET_ID = "openframe-client"; + + private static final String SUBJECT_TEMPLATE = "machine.%s.client-uninstall"; + + private final MachineRepository machineRepository; + + @Override + public DeliveryType getType() { + return DeliveryType.CLIENT_UNINSTALL; + } + + @Override + public Class getPayloadClass() { + return ClientUninstallMessage.class; + } + + @Override + public String targetId(ClientUninstallDeliverySeed seed) { + return TARGET_ID; + } + + @Override + public DeliveryRequest request(ClientUninstallDeliverySeed seed) { + ClientUninstallMessage message = new ClientUninstallMessage(); + message.setIssuedAt(Instant.now().toString()); + return DeliveryRequest.builder() + .type(DeliveryType.CLIENT_UNINSTALL) + .targetId(targetId(seed)) + .machineId(seed.machineId()) + .payload(message) + .build(); + } + + @Override + public String subject(String machineId) { + return format(SUBJECT_TEMPLATE, machineId); + } + + // PENDING_DELETION stops status updates for the machine, so it is set only once the command is on the machine + @Override + public void onAcked(MachineDelivery delivery) { + String machineId = delivery.getMachineId(); + machineRepository.findByMachineId(machineId).ifPresent(machine -> { + DeviceStatus status = machine.getStatus(); + if (status == DeviceStatus.PENDING_DELETION || status == DeviceStatus.DELETED) { + return; + } + machine.setStatus(DeviceStatus.PENDING_DELETION); + machineRepository.save(machine); + log.info("Machine {} marked PENDING_DELETION: uninstall acknowledged by the agent", machineId); + }); + } + + // a machine still in PENDING_DELETION here was never uninstalled: hand it back, the next heartbeat sets ONLINE + @Override + public void onFailed(MachineDelivery delivery, DeliveryFailure failure) { + String machineId = delivery.getMachineId(); + machineRepository.findByMachineId(machineId).ifPresent(machine -> { + if (machine.getStatus() != DeviceStatus.PENDING_DELETION) { + return; + } + machine.setStatus(DeviceStatus.OFFLINE); + machineRepository.save(machine); + log.error("Client uninstall failed after the agent acknowledged it, machine {} returned to OFFLINE: reason={} error={}", + machineId, failure, delivery.getError()); + }); + } +} diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySeed.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySeed.java index 058ba4907e..89598b9a39 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySeed.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySeed.java @@ -20,4 +20,9 @@ public class ToolInstallationDeliverySeed implements DeliverySeed { public DeliveryType type() { return DeliveryType.TOOL_INSTALLATION; } + + @Override + public String machineId() { + return machineId; + } } diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java index 1d2c4f26eb..11aae0ff94 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java @@ -41,16 +41,20 @@ public Class getPayloadClass() { return ToolInstallationMessage.class; } - // targetId must equal the agentType the agent sends in installed-agent, or complete() never finds the row + // must equal the agentType the agent sends in installed-agent, or done() never finds the row + @Override + public String targetId(ToolInstallationDeliverySeed seed) { + return seed.getToolAgent().getKey(); + } + @Override public DeliveryRequest request(ToolInstallationDeliverySeed seed) { IntegratedToolAgent toolAgent = seed.getToolAgent(); ToolInstallationMessage message = buildMessage(toolAgent, seed.getTool(), seed.isReinstall()); - String targetId = toolAgent.getKey(); return DeliveryRequest.builder() .type(DeliveryType.TOOL_INSTALLATION) - .targetId(targetId) - .machineId(seed.getMachineId()) + .targetId(targetId(seed)) + .machineId(seed.machineId()) .payload(message) .build(); } @@ -60,9 +64,14 @@ public String subject(String machineId) { return format(SUBJECT_TEMPLATE, machineId); } + @Override + public void onAcked(MachineDelivery delivery) { + // nothing to do: an install in progress changes nothing on the machine document + } + @Override public void onFailed(MachineDelivery delivery, DeliveryFailure failure) { - // intentionally empty: a failed install leaves nothing to compensate + // nothing to compensate: a failed install leaves the machine as it was } private ToolInstallationMessage buildMessage(IntegratedToolAgent toolAgent, IntegratedTool tool, boolean reinstall) { diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/model/ClientUninstallMessage.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/model/ClientUninstallMessage.java index 13ffcd8d57..f9f32e15a4 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/model/ClientUninstallMessage.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/model/ClientUninstallMessage.java @@ -1,9 +1,15 @@ package com.openframe.data.nats.model; +import com.fasterxml.jackson.annotation.JsonInclude; +import com.openframe.delivery.spec.DeliveryPayload; +import com.openframe.delivery.spec.DeliveryRef; import lombok.Data; @Data -public class ClientUninstallMessage { +public class ClientUninstallMessage implements DeliveryPayload { + + @JsonInclude(JsonInclude.Include.NON_NULL) + private DeliveryRef delivery; /** * When the command was issued (ISO-8601 instant). Lets the agent ignore diff --git a/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java new file mode 100644 index 0000000000..883e9b32cd --- /dev/null +++ b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java @@ -0,0 +1,152 @@ +package com.openframe.data.nats.delivery; + +import com.openframe.data.document.delivery.DeliveryFailure; +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.data.document.delivery.MachineDelivery; +import com.openframe.data.document.device.DeviceStatus; +import com.openframe.data.document.device.Machine; +import com.openframe.data.nats.model.ClientUninstallMessage; +import com.openframe.data.repository.device.MachineRepository; +import com.openframe.delivery.spec.DeliveryRequest; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.util.Optional; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class ClientUninstallDeliverySpecTest { + + private static final String MACHINE_ID = "mach-42"; + private static final String TARGET_ID = "openframe-client"; + + @Mock private MachineRepository machineRepository; + + @InjectMocks private ClientUninstallDeliverySpec spec; + + private Machine machine; + private MachineDelivery delivery; + + @BeforeEach + void setUp() { + machine = new Machine(); + machine.setMachineId(MACHINE_ID); + machine.setStatus(DeviceStatus.ONLINE); + delivery = MachineDelivery.builder() + .type(DeliveryType.CLIENT_UNINSTALL) + .targetId(TARGET_ID) + .machineId(MACHINE_ID) + .build(); + } + + @Test + void request_seed_messageStampedAndTargetIsTheClient() { + // setup + ClientUninstallDeliverySeed seed = new ClientUninstallDeliverySeed(MACHINE_ID); + + // execution + DeliveryRequest request = spec.request(seed); + + // verifications + assertThat(request.getType()).isEqualTo(DeliveryType.CLIENT_UNINSTALL); + assertThat(request.getTargetId()).isEqualTo(TARGET_ID); + assertThat(request.getMachineId()).isEqualTo(MACHINE_ID); + assertThat(request.getPayload().getIssuedAt()).isNotBlank(); + assertThat(request.getPayload().getDelivery()).isNull(); + } + + @Test + void targetId_anySeed_theClientItself() { + // execution + String targetId = spec.targetId(new ClientUninstallDeliverySeed(MACHINE_ID)); + + // verifications + assertThat(targetId).isEqualTo(TARGET_ID); + } + + @Test + void subject_machineId_machineClientUninstallSubject() { + // execution + String subject = spec.subject(MACHINE_ID); + + // verifications + assertThat(subject).isEqualTo("machine.mach-42.client-uninstall"); + } + + @Test + void onAcked_machineInService_markedPendingDeletion() { + // setup + when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); + + // execution + spec.onAcked(delivery); + + // verifications + assertThat(machine.getStatus()).isEqualTo(DeviceStatus.PENDING_DELETION); + verify(machineRepository).save(machine); + } + + @Test + void onAcked_machineAlreadyDeleted_untouched() { + // setup + machine.setStatus(DeviceStatus.DELETED); + when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); + + // execution + spec.onAcked(delivery); + + // verifications + assertThat(machine.getStatus()).isEqualTo(DeviceStatus.DELETED); + verify(machineRepository, never()).save(any()); + } + + @Test + void onFailed_afterAck_machineHandedBackAsOffline() { + // setup + machine.setStatus(DeviceStatus.PENDING_DELETION); + when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); + + // execution + spec.onFailed(delivery, DeliveryFailure.TIMEOUT); + + // verifications + assertThat(machine.getStatus()).isEqualTo(DeviceStatus.OFFLINE); + verify(machineRepository).save(machine); + } + + @Test + void onFailed_neverAcked_machineUntouched() { + // setup + when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); + + // execution + spec.onFailed(delivery, DeliveryFailure.EXHAUSTED); + + // verifications + assertThat(machine.getStatus()).isEqualTo(DeviceStatus.ONLINE); + verify(machineRepository, never()).save(any()); + } + + @Test + void onFailed_machineAlreadyDeleted_untouched() { + // setup + machine.setStatus(DeviceStatus.DELETED); + when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); + + // execution + spec.onFailed(delivery, DeliveryFailure.TIMEOUT); + + // verifications + assertThat(machine.getStatus()).isEqualTo(DeviceStatus.DELETED); + verify(machineRepository, never()).save(any()); + } +} diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySeed.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySeed.java index 85c907aa23..80c6198361 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySeed.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySeed.java @@ -5,4 +5,6 @@ public interface DeliverySeed { DeliveryType type(); + + String machineId(); } diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java index 159cc67e30..83b0bd3e4b 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java @@ -10,9 +10,13 @@ public interface DeliverySpec Class

getPayloadClass(); + String targetId(S seed); + DeliveryRequest

request(S seed); String subject(String machineId); + void onAcked(MachineDelivery delivery); + void onFailed(MachineDelivery delivery, DeliveryFailure failure); } diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java index 9efa2068f7..10cf845a15 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java @@ -2,14 +2,20 @@ import com.openframe.data.document.delivery.DeliveryStatus; import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.data.document.delivery.MachineDelivery; import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryProperties; import com.openframe.delivery.config.DeliveryProperties.Policy; +import com.openframe.delivery.spec.DeliveryPayload; +import com.openframe.delivery.spec.DeliverySeed; +import com.openframe.delivery.spec.DeliverySpec; +import com.openframe.delivery.spec.DeliverySpecRegistry; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.stereotype.Service; import java.time.Instant; +import java.util.Optional; @Slf4j @Service @@ -19,6 +25,7 @@ public class DeliveryTracker { private final MachineDeliveryRepository repository; private final DeliveryProperties properties; private final DeliveryCloser closer; + private final DeliverySpecRegistry registry; public void acknowledge(DeliveryType type, String targetId, String machineId, String dispatchId) { String id = DeliveryId.of(type, targetId, machineId); @@ -29,12 +36,13 @@ public void acknowledge(DeliveryType type, String targetId, String machineId, St boolean acked = repository.markAcked(id, dispatchId, DeliveryStatus.UNACKED, now, resultDueAt); if (acked) { log.info("Delivery ACKED: type={} targetId={} machineId={} dispatchId={}", type, targetId, machineId, dispatchId); + repository.findById(id).ifPresent(this::notifyAcked); } else { log.debug("Delivery ack ignored, no unacked row for this dispatch: id={} dispatchId={}", id, dispatchId); } } - public void complete(DeliveryType type, String targetId, String machineId, String dispatchId) { + public void done(DeliveryType type, String targetId, String machineId, String dispatchId) { String id = DeliveryId.of(type, targetId, machineId); Instant now = Instant.now(); Instant expiresAt = expiresAt(type, now); @@ -46,6 +54,23 @@ public void complete(DeliveryType type, String targetId, String machineId, Strin } } + // for a completion the server learns outside the result channel, e.g. the agent's own uninstall call + public void done(DeliverySeed seed) { + DeliveryType type = seed.type(); + DeliverySpec spec = registry.require(type); + String targetId = spec.targetId(seed); + String machineId = seed.machineId(); + String id = DeliveryId.of(type, targetId, machineId); + Instant now = Instant.now(); + Instant expiresAt = expiresAt(type, now); + boolean done = repository.markDone(id, DeliveryStatus.COMPLETABLE, now, expiresAt); + if (done) { + log.info("Delivery DONE: type={} targetId={} machineId={}", type, targetId, machineId); + } else { + log.debug("Delivery completion ignored, no completable row: id={}", id); + } + } + public void fail(DeliveryType type, String targetId, String machineId, String dispatchId, String error) { Instant now = Instant.now(); closer.failReported(type, targetId, machineId, dispatchId, error, now); @@ -61,6 +86,12 @@ public void cancel(DeliveryType type, String targetId, String machineId) { } } + private void notifyAcked(MachineDelivery delivery) { + DeliveryType type = delivery.getType(); + Optional> spec = registry.find(type); + spec.ifPresent(registered -> registered.onAcked(delivery)); + } + private Instant expiresAt(DeliveryType type, Instant now) { Policy policy = properties.resolve(type); long ttlSeconds = policy.getTtlSeconds(); diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestSeed.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestSeed.java index c6d8052f50..72128a2451 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestSeed.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestSeed.java @@ -14,4 +14,9 @@ public class TestSeed implements DeliverySeed { public DeliveryType type() { return DeliveryType.TOOL_INSTALLATION; } + + @Override + public String machineId() { + return machineId; + } } diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java index c9adc32d52..9c32f1f952 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java @@ -2,8 +2,13 @@ import com.openframe.data.document.delivery.DeliveryStatus; import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.data.document.delivery.MachineDelivery; import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryTestPolicies; +import com.openframe.delivery.spec.DeliverySpec; +import com.openframe.delivery.spec.DeliverySpecRegistry; +import com.openframe.delivery.spec.TestPayload; +import com.openframe.delivery.spec.TestSeed; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -13,13 +18,16 @@ import org.mockito.junit.jupiter.MockitoExtension; import java.time.Instant; +import java.util.Optional; import static com.openframe.delivery.config.DeliveryTestPolicies.RESULT_TIMEOUT; import static com.openframe.delivery.config.DeliveryTestPolicies.TTL; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) @@ -33,6 +41,8 @@ class DeliveryTrackerTest { @Mock private MachineDeliveryRepository repository; @Mock private DeliveryCloser closer; + @Mock private DeliverySpecRegistry registry; + @Mock private DeliverySpec spec; @Captor private ArgumentCaptor atCaptor; @Captor private ArgumentCaptor untilCaptor; @@ -41,7 +51,7 @@ class DeliveryTrackerTest { @BeforeEach void setUp() { - tracker = new DeliveryTracker(repository, DeliveryTestPolicies.properties(), closer); + tracker = new DeliveryTracker(repository, DeliveryTestPolicies.properties(), closer, registry); } @Test @@ -58,12 +68,55 @@ void acknowledge_typedKey_unackedRowMarkedAckedWithResultDeadline() { } @Test - void complete_typedKey_openOrFailedRowMarkedDoneWithTtlExpiry() { + void acknowledge_rowJustAcked_specToldWithTheRow() { + // setup + MachineDelivery row = MachineDelivery.builder().id(DELIVERY_ID).type(DeliveryType.TOOL_INSTALLATION).build(); + when(repository.markAcked(eq(DELIVERY_ID), eq(DISPATCH_ID), eq(DeliveryStatus.UNACKED), any(Instant.class), any(Instant.class))).thenReturn(true); + when(repository.findById(DELIVERY_ID)).thenReturn(Optional.of(row)); + doReturn(Optional.of(spec)).when(registry).find(DeliveryType.TOOL_INSTALLATION); + + // execution + tracker.acknowledge(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID, DISPATCH_ID); + + // verifications + verify(spec).onAcked(row); + } + + @Test + void acknowledge_staleDispatch_specNotTold() { + // setup + when(repository.markAcked(eq(DELIVERY_ID), eq(DISPATCH_ID), eq(DeliveryStatus.UNACKED), any(Instant.class), any(Instant.class))).thenReturn(false); + + // execution + tracker.acknowledge(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID, DISPATCH_ID); + + // verifications + verifyNoInteractions(registry); + } + + @Test + void done_seed_keyResolvedThroughSpecAndCompletableRowMarkedDone() { + // setup + TestSeed seed = new TestSeed(MACHINE_ID); + doReturn(spec).when(registry).require(DeliveryType.TOOL_INSTALLATION); + when(spec.targetId(seed)).thenReturn(TARGET_ID); + when(repository.markDone(eq(DELIVERY_ID), eq(DeliveryStatus.COMPLETABLE), atCaptor.capture(), untilCaptor.capture())).thenReturn(true); + + // execution + tracker.done(seed); + + // verifications + Instant finishedAt = atCaptor.getValue(); + assertThat(untilCaptor.getValue()).isEqualTo(finishedAt.plusSeconds(TTL)); + } + + @Test + void done_typedKey_openOrFailedRowMarkedDoneWithTtlExpiry() { // setup when(repository.markDone(eq(DELIVERY_ID), eq(DISPATCH_ID), eq(DeliveryStatus.COMPLETABLE), atCaptor.capture(), untilCaptor.capture())).thenReturn(true); // execution - tracker.complete(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID, DISPATCH_ID); + tracker.done(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID, DISPATCH_ID); // verifications Instant finishedAt = atCaptor.getValue(); From 8b2571d075524829bbc37078f217d8773a7c9114 Mon Sep 17 00:00:00 2001 From: semen-flamingo Date: Tue, 29 Sep 2026 16:47:38 +0200 Subject: [PATCH 2/9] refactor(delivery): tracker speaks in DeliveryRef and DeliverySeed, dead cancel removed Agent results reach the tracker as the DeliveryRef the agent copied back (acknowledge/done/fail), server-side completions as the DeliverySeed that was dispatched (done). DeliveryTracker.cancel and the repository overload only it used had no callers: the sweep cancels through DeliveryCloser. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01SnaktPhqs3aeuuUHdXCAjX --- .../delivery/DeliveryResultListener.java | 10 ++----- .../delivery/DeliveryResultListenerTest.java | 10 ++++--- .../CustomMachineDeliveryRepository.java | 2 -- .../CustomMachineDeliveryRepositoryImpl.java | 6 ---- .../delivery/track/DeliveryTracker.java | 28 +++++++++--------- .../delivery/track/DeliveryTrackerTest.java | 29 ++++++------------- 6 files changed, 31 insertions(+), 54 deletions(-) diff --git a/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeliveryResultListener.java b/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeliveryResultListener.java index b6c2d3a7f8..4469c7f259 100644 --- a/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeliveryResultListener.java +++ b/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeliveryResultListener.java @@ -3,7 +3,6 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.openframe.client.service.NatsTopicMachineIdExtractor; -import com.openframe.data.document.delivery.DeliveryType; import com.openframe.data.nats.delivery.DeliveryResultMessage; import com.openframe.data.nats.listener.AbstractJetStreamPushListener; import com.openframe.delivery.metrics.DeliveryMetrics; @@ -96,13 +95,10 @@ protected void handleMessage(Message message) { private void apply(String machineId, DeliveryResultMessage report) { DeliveryRef delivery = report.getDelivery(); - DeliveryType type = delivery.getType(); - String targetId = delivery.getTargetId(); - String dispatchId = delivery.getDispatchId(); switch (report.getResult()) { - case ACKED -> deliveryTracker.acknowledge(type, targetId, machineId, dispatchId); - case DONE -> deliveryTracker.done(type, targetId, machineId, dispatchId); - case FAILED -> deliveryTracker.fail(type, targetId, machineId, dispatchId, report.getError()); + case ACKED -> deliveryTracker.acknowledge(delivery, machineId); + case DONE -> deliveryTracker.done(delivery, machineId); + case FAILED -> deliveryTracker.fail(delivery, machineId, report.getError()); } } diff --git a/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeliveryResultListenerTest.java b/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeliveryResultListenerTest.java index 9de2cfd673..fb00df3b40 100644 --- a/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeliveryResultListenerTest.java +++ b/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeliveryResultListenerTest.java @@ -4,6 +4,7 @@ import com.openframe.client.service.NatsTopicMachineIdExtractor; import com.openframe.data.document.delivery.DeliveryType; import com.openframe.delivery.metrics.DeliveryMetrics; +import com.openframe.delivery.spec.DeliveryRef; import com.openframe.delivery.track.DeliveryTracker; import io.nats.client.Connection; import io.nats.client.Message; @@ -27,6 +28,7 @@ class DeliveryResultListenerTest { private static final String SUBJECT = "machine.mach-42.delivery.result"; private static final String TOOL_AGENT_ID = "fleetmdm-agent"; private static final String DISPATCH_ID = "d-1"; + private static final DeliveryRef REF = new DeliveryRef(DeliveryType.TOOL_INSTALLATION, TOOL_AGENT_ID, DISPATCH_ID); private static final String ERROR = "download failed"; private static final String ACKED = "{\"delivery\":{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\"},\"result\":\"ACKED\"}"; @@ -64,7 +66,7 @@ void handleMessage_acked_trackerAcknowledgesDispatch() { listener.handleMessage(message); // verifications - verify(deliveryTracker).acknowledge(DeliveryType.TOOL_INSTALLATION, TOOL_AGENT_ID, MACHINE_ID, DISPATCH_ID); + verify(deliveryTracker).acknowledge(REF, MACHINE_ID); verify(message).ack(); } @@ -77,7 +79,7 @@ void handleMessage_done_trackerCompletesDispatch() { listener.handleMessage(message); // verifications - verify(deliveryTracker).done(DeliveryType.TOOL_INSTALLATION, TOOL_AGENT_ID, MACHINE_ID, DISPATCH_ID); + verify(deliveryTracker).done(REF, MACHINE_ID); verify(message).ack(); } @@ -90,7 +92,7 @@ void handleMessage_failed_trackerFailsDispatchWithError() { listener.handleMessage(message); // verifications - verify(deliveryTracker).fail(DeliveryType.TOOL_INSTALLATION, TOOL_AGENT_ID, MACHINE_ID, DISPATCH_ID, ERROR); + verify(deliveryTracker).fail(REF, MACHINE_ID, ERROR); verify(message).ack(); } @@ -168,7 +170,7 @@ void handleMessage_malformedPayload_rejectedCountedAndAcked() { void handleMessage_trackerThrows_leftUnackedForRedelivery() { // setup stubMessage(ACKED); - doThrow(new IllegalStateException("mongo down")).when(deliveryTracker).acknowledge(DeliveryType.TOOL_INSTALLATION, TOOL_AGENT_ID, MACHINE_ID, DISPATCH_ID); + doThrow(new IllegalStateException("mongo down")).when(deliveryTracker).acknowledge(REF, MACHINE_ID); // execution listener.handleMessage(message); diff --git a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/CustomMachineDeliveryRepository.java b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/CustomMachineDeliveryRepository.java index b92e4b815b..4225a89ad6 100644 --- a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/CustomMachineDeliveryRepository.java +++ b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/CustomMachineDeliveryRepository.java @@ -27,8 +27,6 @@ public interface CustomMachineDeliveryRepository { boolean markDone(String id, Set from, Instant finishedAt, Instant expiresAt); - boolean markCancelled(String id, Set from, Instant finishedAt, Instant expiresAt); - boolean markCancelled(String id, Set from, Instant dispatchedAt, Instant finishedAt, Instant expiresAt); boolean markFailed(String id, Set from, Instant dispatchedAt, DeliveryFailure failure, Instant finishedAt, Instant expiresAt); diff --git a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java index bf8abe1e6f..6396c03825 100644 --- a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java +++ b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java @@ -116,12 +116,6 @@ public boolean markDone(String id, Set from, Instant finishedAt, return updateOne(stillIn(id, from), update); } - @Override - public boolean markCancelled(String id, Set from, Instant finishedAt, Instant expiresAt) { - Update update = closed(DeliveryStatus.CANCELLED, finishedAt, expiresAt); - return updateOne(stillIn(id, from), update); - } - @Override public boolean markCancelled(String id, Set from, Instant dispatchedAt, Instant finishedAt, Instant expiresAt) { Update update = closed(DeliveryStatus.CANCELLED, finishedAt, expiresAt); diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java index 10cf845a15..b006ecc2f5 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java @@ -7,6 +7,7 @@ import com.openframe.delivery.config.DeliveryProperties; import com.openframe.delivery.config.DeliveryProperties.Policy; import com.openframe.delivery.spec.DeliveryPayload; +import com.openframe.delivery.spec.DeliveryRef; import com.openframe.delivery.spec.DeliverySeed; import com.openframe.delivery.spec.DeliverySpec; import com.openframe.delivery.spec.DeliverySpecRegistry; @@ -27,7 +28,11 @@ public class DeliveryTracker { private final DeliveryCloser closer; private final DeliverySpecRegistry registry; - public void acknowledge(DeliveryType type, String targetId, String machineId, String dispatchId) { + // ref = the delivery block the agent copied back from the command; the row is looked up by that exact dispatch + public void acknowledge(DeliveryRef ref, String machineId) { + DeliveryType type = ref.getType(); + String targetId = ref.getTargetId(); + String dispatchId = ref.getDispatchId(); String id = DeliveryId.of(type, targetId, machineId); Instant now = Instant.now(); Policy policy = properties.resolve(type); @@ -42,7 +47,10 @@ public void acknowledge(DeliveryType type, String targetId, String machineId, St } } - public void done(DeliveryType type, String targetId, String machineId, String dispatchId) { + public void done(DeliveryRef ref, String machineId) { + DeliveryType type = ref.getType(); + String targetId = ref.getTargetId(); + String dispatchId = ref.getDispatchId(); String id = DeliveryId.of(type, targetId, machineId); Instant now = Instant.now(); Instant expiresAt = expiresAt(type, now); @@ -54,7 +62,7 @@ public void done(DeliveryType type, String targetId, String machineId, String di } } - // for a completion the server learns outside the result channel, e.g. the agent's own uninstall call + // seed = what we asked for; used when the server learns the outcome outside the result channel, e.g. the agent's own uninstall call public void done(DeliverySeed seed) { DeliveryType type = seed.type(); DeliverySpec spec = registry.require(type); @@ -71,19 +79,9 @@ public void done(DeliverySeed seed) { } } - public void fail(DeliveryType type, String targetId, String machineId, String dispatchId, String error) { + public void fail(DeliveryRef ref, String machineId, String error) { Instant now = Instant.now(); - closer.failReported(type, targetId, machineId, dispatchId, error, now); - } - - public void cancel(DeliveryType type, String targetId, String machineId) { - String id = DeliveryId.of(type, targetId, machineId); - Instant now = Instant.now(); - Instant expiresAt = expiresAt(type, now); - boolean cancelled = repository.markCancelled(id, DeliveryStatus.OPEN, now, expiresAt); - if (cancelled) { - log.info("Delivery CANCELLED: type={} targetId={} machineId={}", type, targetId, machineId); - } + closer.failReported(ref.getType(), ref.getTargetId(), machineId, ref.getDispatchId(), error, now); } private void notifyAcked(MachineDelivery delivery) { diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java index 9c32f1f952..126f0b2657 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java @@ -5,6 +5,7 @@ import com.openframe.data.document.delivery.MachineDelivery; import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryTestPolicies; +import com.openframe.delivery.spec.DeliveryRef; import com.openframe.delivery.spec.DeliverySpec; import com.openframe.delivery.spec.DeliverySpecRegistry; import com.openframe.delivery.spec.TestPayload; @@ -38,6 +39,7 @@ class DeliveryTrackerTest { private static final String DELIVERY_ID = DeliveryId.of(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID); private static final String DISPATCH_ID = "d-1"; private static final String ERROR = "download failed"; + private static final DeliveryRef REF = new DeliveryRef(DeliveryType.TOOL_INSTALLATION, TARGET_ID, DISPATCH_ID); @Mock private MachineDeliveryRepository repository; @Mock private DeliveryCloser closer; @@ -55,12 +57,12 @@ void setUp() { } @Test - void acknowledge_typedKey_unackedRowMarkedAckedWithResultDeadline() { + void acknowledge_ref_unackedRowMarkedAckedWithResultDeadline() { // setup when(repository.markAcked(eq(DELIVERY_ID), eq(DISPATCH_ID), eq(DeliveryStatus.UNACKED), atCaptor.capture(), untilCaptor.capture())).thenReturn(true); // execution - tracker.acknowledge(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID, DISPATCH_ID); + tracker.acknowledge(REF, MACHINE_ID); // verifications Instant ackedAt = atCaptor.getValue(); @@ -76,7 +78,7 @@ void acknowledge_rowJustAcked_specToldWithTheRow() { doReturn(Optional.of(spec)).when(registry).find(DeliveryType.TOOL_INSTALLATION); // execution - tracker.acknowledge(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID, DISPATCH_ID); + tracker.acknowledge(REF, MACHINE_ID); // verifications verify(spec).onAcked(row); @@ -88,7 +90,7 @@ void acknowledge_staleDispatch_specNotTold() { when(repository.markAcked(eq(DELIVERY_ID), eq(DISPATCH_ID), eq(DeliveryStatus.UNACKED), any(Instant.class), any(Instant.class))).thenReturn(false); // execution - tracker.acknowledge(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID, DISPATCH_ID); + tracker.acknowledge(REF, MACHINE_ID); // verifications verifyNoInteractions(registry); @@ -111,25 +113,12 @@ void done_seed_keyResolvedThroughSpecAndCompletableRowMarkedDone() { } @Test - void done_typedKey_openOrFailedRowMarkedDoneWithTtlExpiry() { + void done_ref_openOrFailedRowMarkedDoneWithTtlExpiry() { // setup when(repository.markDone(eq(DELIVERY_ID), eq(DISPATCH_ID), eq(DeliveryStatus.COMPLETABLE), atCaptor.capture(), untilCaptor.capture())).thenReturn(true); // execution - tracker.done(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID, DISPATCH_ID); - - // verifications - Instant finishedAt = atCaptor.getValue(); - assertThat(untilCaptor.getValue()).isEqualTo(finishedAt.plusSeconds(TTL)); - } - - @Test - void cancel_typedKey_openRowMarkedCancelledWithTtlExpiry() { - // setup - when(repository.markCancelled(eq(DELIVERY_ID), eq(DeliveryStatus.OPEN), atCaptor.capture(), untilCaptor.capture())).thenReturn(true); - - // execution - tracker.cancel(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID); + tracker.done(REF, MACHINE_ID); // verifications Instant finishedAt = atCaptor.getValue(); @@ -141,7 +130,7 @@ void fail_agentReportedError_closerAsked() { // setup // execution - tracker.fail(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID, DISPATCH_ID, ERROR); + tracker.fail(REF, MACHINE_ID, ERROR); // verifications verify(closer).failReported(eq(DeliveryType.TOOL_INSTALLATION), eq(TARGET_ID), eq(MACHINE_ID), eq(DISPATCH_ID), eq(ERROR), any(Instant.class)); From 85461055eb3046e66ccf855928e47701d2c45676 Mon Sep 17 00:00:00 2001 From: semen-flamingo Date: Fri, 2 Oct 2026 13:41:41 +0200 Subject: [PATCH 3/9] refactor(delivery): the spec decides whether a seed is dispatchable DeliverySpec.canDispatch(seed), asked by the dispatcher before a row is written. The "uninstall already acknowledged" rule moves from ForceClientUninstallService into ClientUninstallDeliverySpec, next to the onAcked/onFailed rules it belongs with; tool installation always dispatches. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01SnaktPhqs3aeuuUHdXCAjX --- .../service/ForceClientUninstallService.java | 13 ++-------- .../ForceClientUninstallServiceTest.java | 14 ----------- .../delivery/ClientUninstallDeliverySpec.java | 13 ++++++++++ .../ToolInstallationDeliverySpec.java | 5 ++++ .../ClientUninstallDeliverySpecTest.java | 25 +++++++++++++++++++ .../delivery/dispatch/DeliveryDispatcher.java | 4 +++ .../openframe/delivery/spec/DeliverySpec.java | 2 ++ .../dispatch/DeliveryDispatcherTest.java | 14 +++++++++++ 8 files changed, 65 insertions(+), 25 deletions(-) diff --git a/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java b/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java index b5e3febe57..2b157f9f03 100644 --- a/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java +++ b/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java @@ -63,8 +63,9 @@ private ForceClientUninstallResponseItem processMachine(String machineId) { return buildResponseItem(machineId, ForceAgentStatus.FAILED); } + // the machine leaves service on the agent's ACK (spec.onAcked), not here: the sweep needs its real status to retry if (deliveryProperties.isEnabled(DeliveryType.CLIENT_UNINSTALL)) { - dispatch(machine); + deliveryDispatcher.dispatch(new ClientUninstallDeliverySeed(machineId)); return buildResponseItem(machineId, ForceAgentStatus.PROCESSED); } @@ -79,16 +80,6 @@ private ForceClientUninstallResponseItem processMachine(String machineId) { } } - // the machine leaves service on the agent's ACK (spec.onAcked), not here: the sweep needs its real status to retry - private void dispatch(Machine machine) { - String machineId = machine.getMachineId(); - if (machine.getStatus() == PENDING_DELETION) { - log.info("Client uninstall already in progress for machine {}", machineId); - return; - } - deliveryDispatcher.dispatch(new ClientUninstallDeliverySeed(machineId)); - } - private void markPendingDeletion(Machine machine) { if (machine.getStatus() == PENDING_DELETION) { return; diff --git a/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java b/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java index 92bb4f8cf7..93b050f24d 100644 --- a/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java +++ b/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java @@ -89,18 +89,4 @@ void process_flagOn_dispatchedThroughEngineStatusUntouched() { verifyNoInteractions(clientUninstallNatsPublisher); assertThat(response.getItems().get(0).getStatus()).isEqualTo(ForceAgentStatus.PROCESSED); } - - @Test - void process_flagOnUninstallAlreadyAcknowledged_notDispatchedAgain() { - // setup - machine.setStatus(DeviceStatus.PENDING_DELETION); - when(deliveryProperties.isEnabled(DeliveryType.CLIENT_UNINSTALL)).thenReturn(true); - - // execution - ForceClientUninstallResponse response = service.process(request); - - // verifications - verifyNoInteractions(deliveryDispatcher, clientUninstallNatsPublisher); - assertThat(response.getItems().get(0).getStatus()).isEqualTo(ForceAgentStatus.PROCESSED); - } } diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java index 60916c2e62..87fb69fa3c 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java @@ -45,6 +45,19 @@ public String targetId(ClientUninstallDeliverySeed seed) { return TARGET_ID; } + // PENDING_DELETION means an earlier uninstall was acknowledged: its open row and the watchdog own the outcome + @Override + public boolean canDispatch(ClientUninstallDeliverySeed seed) { + String machineId = seed.machineId(); + boolean inProgress = machineRepository.findByMachineId(machineId) + .map(machine -> machine.getStatus() == DeviceStatus.PENDING_DELETION) + .orElse(false); + if (inProgress) { + log.info("Client uninstall already in progress for machine {}", machineId); + } + return !inProgress; + } + @Override public DeliveryRequest request(ClientUninstallDeliverySeed seed) { ClientUninstallMessage message = new ClientUninstallMessage(); diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java index 11aae0ff94..ebf527922e 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java @@ -47,6 +47,11 @@ public String targetId(ToolInstallationDeliverySeed seed) { return seed.getToolAgent().getKey(); } + @Override + public boolean canDispatch(ToolInstallationDeliverySeed seed) { + return true; + } + @Override public DeliveryRequest request(ToolInstallationDeliverySeed seed) { IntegratedToolAgent toolAgent = seed.getToolAgent(); diff --git a/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java index 883e9b32cd..688b0b1127 100644 --- a/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java +++ b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java @@ -73,6 +73,31 @@ void targetId_anySeed_theClientItself() { assertThat(targetId).isEqualTo(TARGET_ID); } + @Test + void canDispatch_machineInService_true() { + // setup + when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); + + // execution + boolean allowed = spec.canDispatch(new ClientUninstallDeliverySeed(MACHINE_ID)); + + // verifications + assertThat(allowed).isTrue(); + } + + @Test + void canDispatch_uninstallAlreadyAcknowledged_false() { + // setup + machine.setStatus(DeviceStatus.PENDING_DELETION); + when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); + + // execution + boolean allowed = spec.canDispatch(new ClientUninstallDeliverySeed(MACHINE_ID)); + + // verifications + assertThat(allowed).isFalse(); + } + @Test void subject_machineId_machineClientUninstallSubject() { // execution diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryDispatcher.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryDispatcher.java index 86916f8781..ec746a1b21 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryDispatcher.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryDispatcher.java @@ -27,6 +27,10 @@ public class DeliveryDispatcher { public void dispatch(DeliverySeed seed) { DeliveryType type = seed.type(); DeliverySpec spec = registry.require(type); + if (!spec.canDispatch(seed)) { + log.debug("Delivery declined by spec: type={} machineId={}", type, seed.machineId()); + return; + } DeliveryRequest request = spec.request(seed); DeliveryPayload payload = request.getPayload(); String dispatchId = UUID.randomUUID().toString(); diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java index 83b0bd3e4b..387b0a7de1 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java @@ -12,6 +12,8 @@ public interface DeliverySpec String targetId(S seed); + boolean canDispatch(S seed); + DeliveryRequest

request(S seed); String subject(String machineId); diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryDispatcherTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryDispatcherTest.java index 8be953d96c..9307bab4b6 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryDispatcherTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryDispatcherTest.java @@ -57,6 +57,7 @@ void setUp() { void dispatch_seed_dispatchIdSetThenRecordedThenPublishedToSpecSubject() { // setup doReturn(spec).when(registry).require(DeliveryType.TOOL_INSTALLATION); + when(spec.canDispatch(seed)).thenReturn(true); when(spec.request(seed)).thenReturn(request); when(spec.subject(MACHINE_ID)).thenReturn("machine.mach-42.test"); when(publisherProvider.getObject()).thenReturn(publisher); @@ -72,6 +73,19 @@ void dispatch_seed_dispatchIdSetThenRecordedThenPublishedToSpecSubject() { verify(publisher).publish("machine.mach-42.test", payload); } + @Test + void dispatch_specDeclines_nothingRecordedOrPublished() { + // setup + doReturn(spec).when(registry).require(DeliveryType.TOOL_INSTALLATION); + when(spec.canDispatch(seed)).thenReturn(false); + + // execution + dispatcher.dispatch(seed); + + // verifications + verifyNoInteractions(recorder, publisherProvider); + } + @Test void dispatch_unregisteredType_throwsWithoutRecording() { // setup From 66687e4894d42de186e31a21d1bbc40ac46e04a5 Mon Sep 17 00:00:00 2001 From: semen-flamingo Date: Fri, 2 Oct 2026 14:03:02 +0200 Subject: [PATCH 4/9] style(delivery): seed accessors are getters DeliverySeed.getType()/getMachineId() instead of type()/machineId(): Lombok @Getter satisfies them, the hand-written overrides go away, and the interface reads like DeliverySpec. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01SnaktPhqs3aeuuUHdXCAjX --- .../client/service/AgentUninstallServiceTest.java | 2 +- .../data/nats/delivery/ClientUninstallDeliverySeed.java | 7 +------ .../data/nats/delivery/ClientUninstallDeliverySpec.java | 4 ++-- .../data/nats/delivery/ToolInstallationDeliverySeed.java | 7 +------ .../data/nats/delivery/ToolInstallationDeliverySpec.java | 2 +- .../openframe/delivery/dispatch/DeliveryDispatcher.java | 4 ++-- .../java/com/openframe/delivery/spec/DeliverySeed.java | 4 ++-- .../java/com/openframe/delivery/track/DeliveryTracker.java | 4 ++-- .../test/java/com/openframe/delivery/spec/TestSeed.java | 7 +------ 9 files changed, 13 insertions(+), 28 deletions(-) diff --git a/openframe-client-core/src/test/java/com/openframe/client/service/AgentUninstallServiceTest.java b/openframe-client-core/src/test/java/com/openframe/client/service/AgentUninstallServiceTest.java index 59747c99ab..8b67b32c93 100644 --- a/openframe-client-core/src/test/java/com/openframe/client/service/AgentUninstallServiceTest.java +++ b/openframe-client-core/src/test/java/com/openframe/client/service/AgentUninstallServiceTest.java @@ -61,7 +61,7 @@ void uninstall_machineInService_deletedAndDeliveryRowClosed() { assertThat(machine.getStatus()).isEqualTo(DeviceStatus.DELETED); verify(machineRepository).save(machine); verify(deliveryTracker).done(seedCaptor.capture()); - assertThat(seedCaptor.getValue().machineId()).isEqualTo(MACHINE_ID); + assertThat(seedCaptor.getValue().getMachineId()).isEqualTo(MACHINE_ID); } @Test diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySeed.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySeed.java index 4c16797476..8fa687363d 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySeed.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySeed.java @@ -12,12 +12,7 @@ public class ClientUninstallDeliverySeed implements DeliverySeed { private final String machineId; @Override - public DeliveryType type() { + public DeliveryType getType() { return DeliveryType.CLIENT_UNINSTALL; } - - @Override - public String machineId() { - return machineId; - } } diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java index 87fb69fa3c..93f78cf217 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java @@ -48,7 +48,7 @@ public String targetId(ClientUninstallDeliverySeed seed) { // PENDING_DELETION means an earlier uninstall was acknowledged: its open row and the watchdog own the outcome @Override public boolean canDispatch(ClientUninstallDeliverySeed seed) { - String machineId = seed.machineId(); + String machineId = seed.getMachineId(); boolean inProgress = machineRepository.findByMachineId(machineId) .map(machine -> machine.getStatus() == DeviceStatus.PENDING_DELETION) .orElse(false); @@ -65,7 +65,7 @@ public DeliveryRequest request(ClientUninstallDeliverySe return DeliveryRequest.builder() .type(DeliveryType.CLIENT_UNINSTALL) .targetId(targetId(seed)) - .machineId(seed.machineId()) + .machineId(seed.getMachineId()) .payload(message) .build(); } diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySeed.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySeed.java index 89598b9a39..af8e7f8b1f 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySeed.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySeed.java @@ -17,12 +17,7 @@ public class ToolInstallationDeliverySeed implements DeliverySeed { private final boolean reinstall; @Override - public DeliveryType type() { + public DeliveryType getType() { return DeliveryType.TOOL_INSTALLATION; } - - @Override - public String machineId() { - return machineId; - } } diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java index ebf527922e..ca544d57ce 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java @@ -59,7 +59,7 @@ public DeliveryRequest request(ToolInstallationDelivery return DeliveryRequest.builder() .type(DeliveryType.TOOL_INSTALLATION) .targetId(targetId(seed)) - .machineId(seed.machineId()) + .machineId(seed.getMachineId()) .payload(message) .build(); } diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryDispatcher.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryDispatcher.java index ec746a1b21..05d656d4bd 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryDispatcher.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryDispatcher.java @@ -25,10 +25,10 @@ public class DeliveryDispatcher { private final ObjectProvider publisher; public void dispatch(DeliverySeed seed) { - DeliveryType type = seed.type(); + DeliveryType type = seed.getType(); DeliverySpec spec = registry.require(type); if (!spec.canDispatch(seed)) { - log.debug("Delivery declined by spec: type={} machineId={}", type, seed.machineId()); + log.debug("Delivery declined by spec: type={} machineId={}", type, seed.getMachineId()); return; } DeliveryRequest request = spec.request(seed); diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySeed.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySeed.java index 80c6198361..2e5f37d057 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySeed.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySeed.java @@ -4,7 +4,7 @@ public interface DeliverySeed { - DeliveryType type(); + DeliveryType getType(); - String machineId(); + String getMachineId(); } diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java index b006ecc2f5..9e15788fa5 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java @@ -64,10 +64,10 @@ public void done(DeliveryRef ref, String machineId) { // seed = what we asked for; used when the server learns the outcome outside the result channel, e.g. the agent's own uninstall call public void done(DeliverySeed seed) { - DeliveryType type = seed.type(); + DeliveryType type = seed.getType(); DeliverySpec spec = registry.require(type); String targetId = spec.targetId(seed); - String machineId = seed.machineId(); + String machineId = seed.getMachineId(); String id = DeliveryId.of(type, targetId, machineId); Instant now = Instant.now(); Instant expiresAt = expiresAt(type, now); diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestSeed.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestSeed.java index 72128a2451..481a84a4ba 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestSeed.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestSeed.java @@ -11,12 +11,7 @@ public class TestSeed implements DeliverySeed { private final String machineId; @Override - public DeliveryType type() { + public DeliveryType getType() { return DeliveryType.TOOL_INSTALLATION; } - - @Override - public String machineId() { - return machineId; - } } From db8f5d42a133c5f180aa11088e0e638777041235 Mon Sep 17 00:00:00 2001 From: semen-flamingo Date: Fri, 2 Oct 2026 14:09:28 +0200 Subject: [PATCH 5/9] refactor(delivery): the seed owns the whole row key DeliverySeed.getTargetId() next to getType() and getMachineId(); DeliverySpec.targetId(seed) goes away and DeliveryTracker.done(seed) no longer needs the registry to build the key. The stale installed-agent comment on the tool-installation target is dropped: completion comes through delivery.result since #2212. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01SnaktPhqs3aeuuUHdXCAjX --- .../data/nats/delivery/ClientUninstallDeliverySeed.java | 7 +++++++ .../data/nats/delivery/ClientUninstallDeliverySpec.java | 9 +-------- .../data/nats/delivery/ToolInstallationDeliverySeed.java | 5 +++++ .../data/nats/delivery/ToolInstallationDeliverySpec.java | 8 +------- .../nats/delivery/ClientUninstallDeliverySpecTest.java | 9 --------- .../java/com/openframe/delivery/spec/DeliverySeed.java | 2 ++ .../java/com/openframe/delivery/spec/DeliverySpec.java | 2 -- .../com/openframe/delivery/track/DeliveryTracker.java | 3 +-- .../test/java/com/openframe/delivery/spec/TestSeed.java | 5 +++++ .../openframe/delivery/track/DeliveryTrackerTest.java | 4 +--- 10 files changed, 23 insertions(+), 31 deletions(-) diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySeed.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySeed.java index 8fa687363d..8620339a2c 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySeed.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySeed.java @@ -9,10 +9,17 @@ @AllArgsConstructor public class ClientUninstallDeliverySeed implements DeliverySeed { + private static final String TARGET_ID = "openframe-client"; + private final String machineId; @Override public DeliveryType getType() { return DeliveryType.CLIENT_UNINSTALL; } + + @Override + public String getTargetId() { + return TARGET_ID; + } } diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java index 93f78cf217..618d97bfcc 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java @@ -24,8 +24,6 @@ @ConditionalOnProperty("spring.cloud.stream.enabled") public class ClientUninstallDeliverySpec implements DeliverySpec { - private static final String TARGET_ID = "openframe-client"; - private static final String SUBJECT_TEMPLATE = "machine.%s.client-uninstall"; private final MachineRepository machineRepository; @@ -40,11 +38,6 @@ public Class getPayloadClass() { return ClientUninstallMessage.class; } - @Override - public String targetId(ClientUninstallDeliverySeed seed) { - return TARGET_ID; - } - // PENDING_DELETION means an earlier uninstall was acknowledged: its open row and the watchdog own the outcome @Override public boolean canDispatch(ClientUninstallDeliverySeed seed) { @@ -64,7 +57,7 @@ public DeliveryRequest request(ClientUninstallDeliverySe message.setIssuedAt(Instant.now().toString()); return DeliveryRequest.builder() .type(DeliveryType.CLIENT_UNINSTALL) - .targetId(targetId(seed)) + .targetId(seed.getTargetId()) .machineId(seed.getMachineId()) .payload(message) .build(); diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySeed.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySeed.java index af8e7f8b1f..9522532cee 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySeed.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySeed.java @@ -20,4 +20,9 @@ public class ToolInstallationDeliverySeed implements DeliverySeed { public DeliveryType getType() { return DeliveryType.TOOL_INSTALLATION; } + + @Override + public String getTargetId() { + return toolAgent.getKey(); + } } diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java index ca544d57ce..92cd497375 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java @@ -41,12 +41,6 @@ public Class getPayloadClass() { return ToolInstallationMessage.class; } - // must equal the agentType the agent sends in installed-agent, or done() never finds the row - @Override - public String targetId(ToolInstallationDeliverySeed seed) { - return seed.getToolAgent().getKey(); - } - @Override public boolean canDispatch(ToolInstallationDeliverySeed seed) { return true; @@ -58,7 +52,7 @@ public DeliveryRequest request(ToolInstallationDelivery ToolInstallationMessage message = buildMessage(toolAgent, seed.getTool(), seed.isReinstall()); return DeliveryRequest.builder() .type(DeliveryType.TOOL_INSTALLATION) - .targetId(targetId(seed)) + .targetId(seed.getTargetId()) .machineId(seed.getMachineId()) .payload(message) .build(); diff --git a/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java index 688b0b1127..a4ee8b598a 100644 --- a/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java +++ b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java @@ -64,15 +64,6 @@ void request_seed_messageStampedAndTargetIsTheClient() { assertThat(request.getPayload().getDelivery()).isNull(); } - @Test - void targetId_anySeed_theClientItself() { - // execution - String targetId = spec.targetId(new ClientUninstallDeliverySeed(MACHINE_ID)); - - // verifications - assertThat(targetId).isEqualTo(TARGET_ID); - } - @Test void canDispatch_machineInService_true() { // setup diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySeed.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySeed.java index 2e5f37d057..7da02223f5 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySeed.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySeed.java @@ -6,5 +6,7 @@ public interface DeliverySeed { DeliveryType getType(); + String getTargetId(); + String getMachineId(); } diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java index 387b0a7de1..3abceaa16f 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java @@ -10,8 +10,6 @@ public interface DeliverySpec Class

getPayloadClass(); - String targetId(S seed); - boolean canDispatch(S seed); DeliveryRequest

request(S seed); diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java index 9e15788fa5..5d9f65982a 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java @@ -65,8 +65,7 @@ public void done(DeliveryRef ref, String machineId) { // seed = what we asked for; used when the server learns the outcome outside the result channel, e.g. the agent's own uninstall call public void done(DeliverySeed seed) { DeliveryType type = seed.getType(); - DeliverySpec spec = registry.require(type); - String targetId = spec.targetId(seed); + String targetId = seed.getTargetId(); String machineId = seed.getMachineId(); String id = DeliveryId.of(type, targetId, machineId); Instant now = Instant.now(); diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestSeed.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestSeed.java index 481a84a4ba..b6e9ee87e4 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestSeed.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestSeed.java @@ -14,4 +14,9 @@ public class TestSeed implements DeliverySeed { public DeliveryType getType() { return DeliveryType.TOOL_INSTALLATION; } + + @Override + public String getTargetId() { + return "fleetmdm-agent"; + } } diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java index 126f0b2657..ba15db4feb 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java @@ -97,11 +97,9 @@ void acknowledge_staleDispatch_specNotTold() { } @Test - void done_seed_keyResolvedThroughSpecAndCompletableRowMarkedDone() { + void done_seed_completableRowOfTheSeedKeyMarkedDone() { // setup TestSeed seed = new TestSeed(MACHINE_ID); - doReturn(spec).when(registry).require(DeliveryType.TOOL_INSTALLATION); - when(spec.targetId(seed)).thenReturn(TARGET_ID); when(repository.markDone(eq(DELIVERY_ID), eq(DeliveryStatus.COMPLETABLE), atCaptor.capture(), untilCaptor.capture())).thenReturn(true); // execution From b26c08e3259d02d9f3a0921284e29f934eaa8f4a Mon Sep 17 00:00:00 2001 From: semen-flamingo Date: Fri, 2 Oct 2026 16:18:06 +0200 Subject: [PATCH 6/9] refactor(delivery): the spec only encodes the command; the engine publishes events, the domain moves the machine Kirill: a spec that reads and writes machines does not fit an isolated delivery service. DeliverySpec loses canDispatch/onAcked/onFailed; DeliveryTracker and DeliveryCloser publish DeliveryAckedEvent and DeliveryFailedEvent instead of calling back into specs. What an uninstall means for the machine lives in client-service: ClientUninstallDeliveryListener drives two MachineStatusService transitions (deletion acknowledged -> PENDING_DELETION, deletion cancelled -> OFFLINE). The "already acknowledged" precondition is back in ForceClientUninstallService, which reads the status but never writes it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01SnaktPhqs3aeuuUHdXCAjX --- .../service/ForceClientUninstallService.java | 14 +- .../ForceClientUninstallServiceTest.java | 14 ++ .../ClientUninstallDeliveryListener.java | 40 ++++++ .../client/service/MachineStatusService.java | 26 ++++ .../ClientUninstallDeliveryListenerTest.java | 69 +++++++++ .../service/MachineStatusServiceTest.java | 65 +++++++++ .../delivery/ClientUninstallDeliverySpec.java | 54 ------- .../ToolInstallationDeliverySpec.java | 17 --- .../ClientUninstallDeliverySpecTest.java | 132 +----------------- .../delivery/dispatch/DeliveryDispatcher.java | 4 - .../delivery/event/DeliveryAckedEvent.java | 23 +++ .../delivery/event/DeliveryFailedEvent.java | 29 ++++ .../openframe/delivery/spec/DeliverySpec.java | 8 -- .../delivery/track/DeliveryCloser.java | 26 +--- .../delivery/track/DeliveryTracker.java | 17 +-- .../dispatch/DeliveryDispatcherTest.java | 14 -- .../delivery/track/DeliveryCloserTest.java | 62 ++++---- .../delivery/track/DeliveryTrackerTest.java | 35 ++--- 18 files changed, 332 insertions(+), 317 deletions(-) create mode 100644 openframe-client-core/src/main/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListener.java create mode 100644 openframe-client-core/src/test/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListenerTest.java create mode 100644 openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryAckedEvent.java create mode 100644 openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryFailedEvent.java diff --git a/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java b/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java index 2b157f9f03..a2b992a3b5 100644 --- a/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java +++ b/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java @@ -63,9 +63,8 @@ private ForceClientUninstallResponseItem processMachine(String machineId) { return buildResponseItem(machineId, ForceAgentStatus.FAILED); } - // the machine leaves service on the agent's ACK (spec.onAcked), not here: the sweep needs its real status to retry if (deliveryProperties.isEnabled(DeliveryType.CLIENT_UNINSTALL)) { - deliveryDispatcher.dispatch(new ClientUninstallDeliverySeed(machineId)); + dispatch(machine); return buildResponseItem(machineId, ForceAgentStatus.PROCESSED); } @@ -80,6 +79,17 @@ private ForceClientUninstallResponseItem processMachine(String machineId) { } } + // the machine leaves service when the agent acknowledges (client-service, on DeliveryAckedEvent), not here: the sweep + // needs its real status to retry. PENDING_DELETION therefore means an uninstall is already on the machine + private void dispatch(Machine machine) { + String machineId = machine.getMachineId(); + if (machine.getStatus() == PENDING_DELETION) { + log.info("Client uninstall already in progress for machine {}", machineId); + return; + } + deliveryDispatcher.dispatch(new ClientUninstallDeliverySeed(machineId)); + } + private void markPendingDeletion(Machine machine) { if (machine.getStatus() == PENDING_DELETION) { return; diff --git a/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java b/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java index 93b050f24d..92bb4f8cf7 100644 --- a/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java +++ b/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java @@ -89,4 +89,18 @@ void process_flagOn_dispatchedThroughEngineStatusUntouched() { verifyNoInteractions(clientUninstallNatsPublisher); assertThat(response.getItems().get(0).getStatus()).isEqualTo(ForceAgentStatus.PROCESSED); } + + @Test + void process_flagOnUninstallAlreadyAcknowledged_notDispatchedAgain() { + // setup + machine.setStatus(DeviceStatus.PENDING_DELETION); + when(deliveryProperties.isEnabled(DeliveryType.CLIENT_UNINSTALL)).thenReturn(true); + + // execution + ForceClientUninstallResponse response = service.process(request); + + // verifications + verifyNoInteractions(deliveryDispatcher, clientUninstallNatsPublisher); + assertThat(response.getItems().get(0).getStatus()).isEqualTo(ForceAgentStatus.PROCESSED); + } } diff --git a/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListener.java b/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListener.java new file mode 100644 index 0000000000..69734cd413 --- /dev/null +++ b/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListener.java @@ -0,0 +1,40 @@ +package com.openframe.client.listener.delivery; + +import com.openframe.client.service.MachineStatusService; +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.delivery.event.DeliveryAckedEvent; +import com.openframe.delivery.event.DeliveryFailedEvent; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.context.event.EventListener; +import org.springframework.stereotype.Component; + +// what a client uninstall delivery means for the machine document; the engine itself knows nothing about machines +@Slf4j +@Component +@RequiredArgsConstructor +public class ClientUninstallDeliveryListener { + + private final MachineStatusService machineStatusService; + + @EventListener + public void onAcked(DeliveryAckedEvent event) { + if (event.getType() != DeliveryType.CLIENT_UNINSTALL) { + return; + } + machineStatusService.markDeletionAcknowledged(event.getMachineId()); + } + + @EventListener + public void onFailed(DeliveryFailedEvent event) { + if (event.getType() != DeliveryType.CLIENT_UNINSTALL) { + return; + } + String machineId = event.getMachineId(); + boolean handedBack = machineStatusService.cancelPendingDeletion(machineId); + if (handedBack) { + log.error("Client uninstall failed after the agent acknowledged it, machine {} returned to OFFLINE: reason={} error={}", + machineId, event.getFailure(), event.getError()); + } + } +} diff --git a/openframe-client-core/src/main/java/com/openframe/client/service/MachineStatusService.java b/openframe-client-core/src/main/java/com/openframe/client/service/MachineStatusService.java index a433cb8c2e..4164eae36c 100644 --- a/openframe-client-core/src/main/java/com/openframe/client/service/MachineStatusService.java +++ b/openframe-client-core/src/main/java/com/openframe/client/service/MachineStatusService.java @@ -16,6 +16,7 @@ import static com.openframe.data.document.device.DeviceStatus.OFFLINE; import static com.openframe.data.document.device.DeviceStatus.ONLINE; import static com.openframe.data.document.device.DeviceStatus.PENDING; +import static com.openframe.data.document.device.DeviceStatus.PENDING_DELETION; @Service @RequiredArgsConstructor @@ -37,6 +38,31 @@ public void processHeartbeat(String machineId, Instant eventTimestamp) { update(machineId, ONLINE, eventTimestamp); } + // the agent holds its uninstall command: from here on status events are ignored until the machine is gone + public void markDeletionAcknowledged(String machineId) { + machineRepository.findByMachineId(machineId).ifPresent(machine -> { + if (isDeletionInProgress(machine)) { + return; + } + machine.setStatus(PENDING_DELETION); + machineRepository.save(machine); + log.info("Machine {} marked PENDING_DELETION: uninstall acknowledged by the agent", machineId); + }); + } + + // the uninstall did not happen: hand the machine back as OFFLINE, the next heartbeat sets ONLINE again + public boolean cancelPendingDeletion(String machineId) { + return machineRepository.findByMachineId(machineId) + .filter(machine -> machine.getStatus() == PENDING_DELETION) + .map(machine -> { + machine.setStatus(OFFLINE); + machineRepository.save(machine); + log.warn("Machine {} returned to OFFLINE: pending deletion cancelled", machineId); + return true; + }) + .orElse(false); + } + private void update(String machineId, DeviceStatus newStatus, Instant eventTimestamp) { log.debug("Received status update event to {} for machineId={} eventTimestamp={}", newStatus, machineId, eventTimestamp); diff --git a/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListenerTest.java b/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListenerTest.java new file mode 100644 index 0000000000..0bfe2c8892 --- /dev/null +++ b/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListenerTest.java @@ -0,0 +1,69 @@ +package com.openframe.client.listener.delivery; + +import com.openframe.client.service.MachineStatusService; +import com.openframe.data.document.delivery.DeliveryFailure; +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.delivery.event.DeliveryAckedEvent; +import com.openframe.delivery.event.DeliveryFailedEvent; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class ClientUninstallDeliveryListenerTest { + + private static final String MACHINE_ID = "mach-42"; + private static final String TARGET_ID = "openframe-client"; + private static final String DISPATCH_ID = "d-1"; + + @Mock private MachineStatusService machineStatusService; + + @InjectMocks private ClientUninstallDeliveryListener listener; + + @Test + void onAcked_clientUninstall_deletionMarkedAcknowledged() { + // execution + listener.onAcked(new DeliveryAckedEvent(this, DeliveryType.CLIENT_UNINSTALL, TARGET_ID, MACHINE_ID, DISPATCH_ID)); + + // verifications + verify(machineStatusService).markDeletionAcknowledged(MACHINE_ID); + } + + @Test + void onAcked_otherType_ignored() { + // execution + listener.onAcked(new DeliveryAckedEvent(this, DeliveryType.TOOL_INSTALLATION, "fleetmdm-agent", MACHINE_ID, DISPATCH_ID)); + + // verifications + verifyNoInteractions(machineStatusService); + } + + @Test + void onFailed_clientUninstall_pendingDeletionCancelled() { + // setup + when(machineStatusService.cancelPendingDeletion(MACHINE_ID)).thenReturn(true); + + // execution + listener.onFailed(new DeliveryFailedEvent(this, DeliveryType.CLIENT_UNINSTALL, TARGET_ID, MACHINE_ID, DISPATCH_ID, + DeliveryFailure.TIMEOUT, null)); + + // verifications + verify(machineStatusService).cancelPendingDeletion(MACHINE_ID); + } + + @Test + void onFailed_otherType_ignored() { + // execution + listener.onFailed(new DeliveryFailedEvent(this, DeliveryType.TOOL_INSTALLATION, "fleetmdm-agent", MACHINE_ID, DISPATCH_ID, + DeliveryFailure.EXHAUSTED, null)); + + // verifications + verifyNoInteractions(machineStatusService); + } +} diff --git a/openframe-client-core/src/test/java/com/openframe/client/service/MachineStatusServiceTest.java b/openframe-client-core/src/test/java/com/openframe/client/service/MachineStatusServiceTest.java index 8b1d92897a..545926676b 100644 --- a/openframe-client-core/src/test/java/com/openframe/client/service/MachineStatusServiceTest.java +++ b/openframe-client-core/src/test/java/com/openframe/client/service/MachineStatusServiceTest.java @@ -224,4 +224,69 @@ void pendingDeletion_touchesNothing() { verify(machineRepository, never()).save(any(Machine.class)); verify(machineRepository, never()).updateLastSeen(anyString(), any(Instant.class)); } + + @Test + void markDeletionAcknowledged_machineInService_pendingDeletion() { + // setup + Machine machine = new Machine(); + machine.setMachineId(MACHINE); + machine.setStatus(DeviceStatus.ONLINE); + when(machineRepository.findByMachineId(MACHINE)).thenReturn(Optional.of(machine)); + + // execution + service.markDeletionAcknowledged(MACHINE); + + // verifications + assertThat(machine.getStatus()).isEqualTo(DeviceStatus.PENDING_DELETION); + verify(machineRepository).save(machine); + } + + @Test + void markDeletionAcknowledged_machineAlreadyDeleted_untouched() { + // setup + Machine machine = new Machine(); + machine.setMachineId(MACHINE); + machine.setStatus(DeviceStatus.DELETED); + when(machineRepository.findByMachineId(MACHINE)).thenReturn(Optional.of(machine)); + + // execution + service.markDeletionAcknowledged(MACHINE); + + // verifications + assertThat(machine.getStatus()).isEqualTo(DeviceStatus.DELETED); + verify(machineRepository, never()).save(any()); + } + + @Test + void cancelPendingDeletion_machinePendingDeletion_offlineAndTrue() { + // setup + Machine machine = new Machine(); + machine.setMachineId(MACHINE); + machine.setStatus(DeviceStatus.PENDING_DELETION); + when(machineRepository.findByMachineId(MACHINE)).thenReturn(Optional.of(machine)); + + // execution + boolean handedBack = service.cancelPendingDeletion(MACHINE); + + // verifications + assertThat(handedBack).isTrue(); + assertThat(machine.getStatus()).isEqualTo(DeviceStatus.OFFLINE); + verify(machineRepository).save(machine); + } + + @Test + void cancelPendingDeletion_machineNotPendingDeletion_falseAndUntouched() { + // setup + Machine machine = new Machine(); + machine.setMachineId(MACHINE); + machine.setStatus(DeviceStatus.ONLINE); + when(machineRepository.findByMachineId(MACHINE)).thenReturn(Optional.of(machine)); + + // execution + boolean handedBack = service.cancelPendingDeletion(MACHINE); + + // verifications + assertThat(handedBack).isFalse(); + verify(machineRepository, never()).save(any()); + } } diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java index 618d97bfcc..f09f40a30a 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java @@ -1,16 +1,9 @@ package com.openframe.data.nats.delivery; -import com.openframe.data.document.delivery.DeliveryFailure; import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.data.document.delivery.MachineDelivery; -import com.openframe.data.document.device.DeviceStatus; -import com.openframe.data.document.device.Machine; import com.openframe.data.nats.model.ClientUninstallMessage; -import com.openframe.data.repository.device.MachineRepository; import com.openframe.delivery.spec.DeliveryRequest; import com.openframe.delivery.spec.DeliverySpec; -import lombok.RequiredArgsConstructor; -import lombok.extern.slf4j.Slf4j; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.stereotype.Component; @@ -18,16 +11,12 @@ import static java.lang.String.format; -@Slf4j @Component -@RequiredArgsConstructor @ConditionalOnProperty("spring.cloud.stream.enabled") public class ClientUninstallDeliverySpec implements DeliverySpec { private static final String SUBJECT_TEMPLATE = "machine.%s.client-uninstall"; - private final MachineRepository machineRepository; - @Override public DeliveryType getType() { return DeliveryType.CLIENT_UNINSTALL; @@ -38,19 +27,6 @@ public Class getPayloadClass() { return ClientUninstallMessage.class; } - // PENDING_DELETION means an earlier uninstall was acknowledged: its open row and the watchdog own the outcome - @Override - public boolean canDispatch(ClientUninstallDeliverySeed seed) { - String machineId = seed.getMachineId(); - boolean inProgress = machineRepository.findByMachineId(machineId) - .map(machine -> machine.getStatus() == DeviceStatus.PENDING_DELETION) - .orElse(false); - if (inProgress) { - log.info("Client uninstall already in progress for machine {}", machineId); - } - return !inProgress; - } - @Override public DeliveryRequest request(ClientUninstallDeliverySeed seed) { ClientUninstallMessage message = new ClientUninstallMessage(); @@ -67,34 +43,4 @@ public DeliveryRequest request(ClientUninstallDeliverySe public String subject(String machineId) { return format(SUBJECT_TEMPLATE, machineId); } - - // PENDING_DELETION stops status updates for the machine, so it is set only once the command is on the machine - @Override - public void onAcked(MachineDelivery delivery) { - String machineId = delivery.getMachineId(); - machineRepository.findByMachineId(machineId).ifPresent(machine -> { - DeviceStatus status = machine.getStatus(); - if (status == DeviceStatus.PENDING_DELETION || status == DeviceStatus.DELETED) { - return; - } - machine.setStatus(DeviceStatus.PENDING_DELETION); - machineRepository.save(machine); - log.info("Machine {} marked PENDING_DELETION: uninstall acknowledged by the agent", machineId); - }); - } - - // a machine still in PENDING_DELETION here was never uninstalled: hand it back, the next heartbeat sets ONLINE - @Override - public void onFailed(MachineDelivery delivery, DeliveryFailure failure) { - String machineId = delivery.getMachineId(); - machineRepository.findByMachineId(machineId).ifPresent(machine -> { - if (machine.getStatus() != DeviceStatus.PENDING_DELETION) { - return; - } - machine.setStatus(DeviceStatus.OFFLINE); - machineRepository.save(machine); - log.error("Client uninstall failed after the agent acknowledged it, machine {} returned to OFFLINE: reason={} error={}", - machineId, failure, delivery.getError()); - }); - } } diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java index 92cd497375..14a7233f24 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java @@ -1,8 +1,6 @@ package com.openframe.data.nats.delivery; -import com.openframe.data.document.delivery.DeliveryFailure; import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.data.document.delivery.MachineDelivery; import com.openframe.data.document.tool.IntegratedTool; import com.openframe.data.document.toolagent.IntegratedToolAgent; import com.openframe.data.document.toolagent.ToolAgentAsset; @@ -41,11 +39,6 @@ public Class getPayloadClass() { return ToolInstallationMessage.class; } - @Override - public boolean canDispatch(ToolInstallationDeliverySeed seed) { - return true; - } - @Override public DeliveryRequest request(ToolInstallationDeliverySeed seed) { IntegratedToolAgent toolAgent = seed.getToolAgent(); @@ -63,16 +56,6 @@ public String subject(String machineId) { return format(SUBJECT_TEMPLATE, machineId); } - @Override - public void onAcked(MachineDelivery delivery) { - // nothing to do: an install in progress changes nothing on the machine document - } - - @Override - public void onFailed(MachineDelivery delivery, DeliveryFailure failure) { - // nothing to compensate: a failed install leaves the machine as it was - } - private ToolInstallationMessage buildMessage(IntegratedToolAgent toolAgent, IntegratedTool tool, boolean reinstall) { String version = toolAgent.getVersion(); ToolInstallationMessage message = new ToolInstallationMessage(); diff --git a/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java index a4ee8b598a..43f4b89ea8 100644 --- a/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java +++ b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java @@ -1,52 +1,17 @@ package com.openframe.data.nats.delivery; -import com.openframe.data.document.delivery.DeliveryFailure; import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.data.document.delivery.MachineDelivery; -import com.openframe.data.document.device.DeviceStatus; -import com.openframe.data.document.device.Machine; import com.openframe.data.nats.model.ClientUninstallMessage; -import com.openframe.data.repository.device.MachineRepository; import com.openframe.delivery.spec.DeliveryRequest; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.InjectMocks; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; - -import java.util.Optional; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; -@ExtendWith(MockitoExtension.class) class ClientUninstallDeliverySpecTest { private static final String MACHINE_ID = "mach-42"; - private static final String TARGET_ID = "openframe-client"; - - @Mock private MachineRepository machineRepository; - - @InjectMocks private ClientUninstallDeliverySpec spec; - - private Machine machine; - private MachineDelivery delivery; - @BeforeEach - void setUp() { - machine = new Machine(); - machine.setMachineId(MACHINE_ID); - machine.setStatus(DeviceStatus.ONLINE); - delivery = MachineDelivery.builder() - .type(DeliveryType.CLIENT_UNINSTALL) - .targetId(TARGET_ID) - .machineId(MACHINE_ID) - .build(); - } + private final ClientUninstallDeliverySpec spec = new ClientUninstallDeliverySpec(); @Test void request_seed_messageStampedAndTargetIsTheClient() { @@ -58,37 +23,12 @@ void request_seed_messageStampedAndTargetIsTheClient() { // verifications assertThat(request.getType()).isEqualTo(DeliveryType.CLIENT_UNINSTALL); - assertThat(request.getTargetId()).isEqualTo(TARGET_ID); + assertThat(request.getTargetId()).isEqualTo("openframe-client"); assertThat(request.getMachineId()).isEqualTo(MACHINE_ID); assertThat(request.getPayload().getIssuedAt()).isNotBlank(); assertThat(request.getPayload().getDelivery()).isNull(); } - @Test - void canDispatch_machineInService_true() { - // setup - when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); - - // execution - boolean allowed = spec.canDispatch(new ClientUninstallDeliverySeed(MACHINE_ID)); - - // verifications - assertThat(allowed).isTrue(); - } - - @Test - void canDispatch_uninstallAlreadyAcknowledged_false() { - // setup - machine.setStatus(DeviceStatus.PENDING_DELETION); - when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); - - // execution - boolean allowed = spec.canDispatch(new ClientUninstallDeliverySeed(MACHINE_ID)); - - // verifications - assertThat(allowed).isFalse(); - } - @Test void subject_machineId_machineClientUninstallSubject() { // execution @@ -97,72 +37,4 @@ void subject_machineId_machineClientUninstallSubject() { // verifications assertThat(subject).isEqualTo("machine.mach-42.client-uninstall"); } - - @Test - void onAcked_machineInService_markedPendingDeletion() { - // setup - when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); - - // execution - spec.onAcked(delivery); - - // verifications - assertThat(machine.getStatus()).isEqualTo(DeviceStatus.PENDING_DELETION); - verify(machineRepository).save(machine); - } - - @Test - void onAcked_machineAlreadyDeleted_untouched() { - // setup - machine.setStatus(DeviceStatus.DELETED); - when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); - - // execution - spec.onAcked(delivery); - - // verifications - assertThat(machine.getStatus()).isEqualTo(DeviceStatus.DELETED); - verify(machineRepository, never()).save(any()); - } - - @Test - void onFailed_afterAck_machineHandedBackAsOffline() { - // setup - machine.setStatus(DeviceStatus.PENDING_DELETION); - when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); - - // execution - spec.onFailed(delivery, DeliveryFailure.TIMEOUT); - - // verifications - assertThat(machine.getStatus()).isEqualTo(DeviceStatus.OFFLINE); - verify(machineRepository).save(machine); - } - - @Test - void onFailed_neverAcked_machineUntouched() { - // setup - when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); - - // execution - spec.onFailed(delivery, DeliveryFailure.EXHAUSTED); - - // verifications - assertThat(machine.getStatus()).isEqualTo(DeviceStatus.ONLINE); - verify(machineRepository, never()).save(any()); - } - - @Test - void onFailed_machineAlreadyDeleted_untouched() { - // setup - machine.setStatus(DeviceStatus.DELETED); - when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); - - // execution - spec.onFailed(delivery, DeliveryFailure.TIMEOUT); - - // verifications - assertThat(machine.getStatus()).isEqualTo(DeviceStatus.DELETED); - verify(machineRepository, never()).save(any()); - } } diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryDispatcher.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryDispatcher.java index 05d656d4bd..9e9a5d646e 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryDispatcher.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryDispatcher.java @@ -27,10 +27,6 @@ public class DeliveryDispatcher { public void dispatch(DeliverySeed seed) { DeliveryType type = seed.getType(); DeliverySpec spec = registry.require(type); - if (!spec.canDispatch(seed)) { - log.debug("Delivery declined by spec: type={} machineId={}", type, seed.getMachineId()); - return; - } DeliveryRequest request = spec.request(seed); DeliveryPayload payload = request.getPayload(); String dispatchId = UUID.randomUUID().toString(); diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryAckedEvent.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryAckedEvent.java new file mode 100644 index 0000000000..346d7125cf --- /dev/null +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryAckedEvent.java @@ -0,0 +1,23 @@ +package com.openframe.delivery.event; + +import com.openframe.data.document.delivery.DeliveryType; +import lombok.Getter; +import org.springframework.context.ApplicationEvent; + +// the agent confirmed it holds the command; published once, on the real PENDING -> ACKED transition +@Getter +public class DeliveryAckedEvent extends ApplicationEvent { + + private final DeliveryType type; + private final String targetId; + private final String machineId; + private final String dispatchId; + + public DeliveryAckedEvent(Object source, DeliveryType type, String targetId, String machineId, String dispatchId) { + super(source); + this.type = type; + this.targetId = targetId; + this.machineId = machineId; + this.dispatchId = dispatchId; + } +} diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryFailedEvent.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryFailedEvent.java new file mode 100644 index 0000000000..db6a1290d4 --- /dev/null +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryFailedEvent.java @@ -0,0 +1,29 @@ +package com.openframe.delivery.event; + +import com.openframe.data.document.delivery.DeliveryFailure; +import com.openframe.data.document.delivery.DeliveryType; +import lombok.Getter; +import org.springframework.context.ApplicationEvent; + +// the row closed as FAILED: retries exhausted, machine never came back, no result after the ACK, or the agent reported an error +@Getter +public class DeliveryFailedEvent extends ApplicationEvent { + + private final DeliveryType type; + private final String targetId; + private final String machineId; + private final String dispatchId; + private final DeliveryFailure failure; + private final String error; + + public DeliveryFailedEvent(Object source, DeliveryType type, String targetId, String machineId, String dispatchId, + DeliveryFailure failure, String error) { + super(source); + this.type = type; + this.targetId = targetId; + this.machineId = machineId; + this.dispatchId = dispatchId; + this.failure = failure; + this.error = error; + } +} diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java index 3abceaa16f..3b6fff80b1 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java @@ -1,8 +1,6 @@ package com.openframe.delivery.spec; -import com.openframe.data.document.delivery.DeliveryFailure; import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.data.document.delivery.MachineDelivery; public interface DeliverySpec { @@ -10,13 +8,7 @@ public interface DeliverySpec Class

getPayloadClass(); - boolean canDispatch(S seed); - DeliveryRequest

request(S seed); String subject(String machineId); - - void onAcked(MachineDelivery delivery); - - void onFailed(MachineDelivery delivery, DeliveryFailure failure); } diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryCloser.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryCloser.java index b4e4c85bd2..3edc6f4845 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryCloser.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryCloser.java @@ -7,17 +7,14 @@ import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryProperties; import com.openframe.delivery.config.DeliveryProperties.Policy; +import com.openframe.delivery.event.DeliveryFailedEvent; import com.openframe.delivery.metrics.DeliveryMetrics; -import com.openframe.delivery.spec.DeliveryPayload; -import com.openframe.delivery.spec.DeliverySeed; -import com.openframe.delivery.spec.DeliverySpec; -import com.openframe.delivery.spec.DeliverySpecRegistry; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.springframework.context.ApplicationEventPublisher; import org.springframework.stereotype.Component; import java.time.Instant; -import java.util.Optional; import java.util.Set; @Slf4j @@ -26,7 +23,7 @@ public class DeliveryCloser { private final MachineDeliveryRepository repository; - private final DeliverySpecRegistry registry; + private final ApplicationEventPublisher events; private final DeliveryProperties properties; private final DeliveryMetrics metrics; @@ -47,7 +44,8 @@ public void fail(MachineDelivery delivery, DeliveryFailure failure, Set from, String re } } - private void notifyAgentError(MachineDelivery delivery) { - notifySpec(delivery, DeliveryFailure.AGENT_ERROR); - } - - private void notifySpec(MachineDelivery delivery, DeliveryFailure failure) { - DeliveryType type = delivery.getType(); - Optional> spec = registry.find(type); - spec.ifPresentOrElse( - registered -> registered.onFailed(delivery, failure), - () -> log.warn("No spec registered for delivery type {}, onFailed skipped", type)); - } - private Instant expiresAt(DeliveryType type, Instant now) { Policy policy = properties.resolve(type); long ttlSeconds = policy.getTtlSeconds(); diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java index 5d9f65982a..9928857bc3 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryTracker.java @@ -2,21 +2,18 @@ import com.openframe.data.document.delivery.DeliveryStatus; import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.data.document.delivery.MachineDelivery; import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryProperties; import com.openframe.delivery.config.DeliveryProperties.Policy; -import com.openframe.delivery.spec.DeliveryPayload; +import com.openframe.delivery.event.DeliveryAckedEvent; import com.openframe.delivery.spec.DeliveryRef; import com.openframe.delivery.spec.DeliverySeed; -import com.openframe.delivery.spec.DeliverySpec; -import com.openframe.delivery.spec.DeliverySpecRegistry; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.springframework.context.ApplicationEventPublisher; import org.springframework.stereotype.Service; import java.time.Instant; -import java.util.Optional; @Slf4j @Service @@ -26,7 +23,7 @@ public class DeliveryTracker { private final MachineDeliveryRepository repository; private final DeliveryProperties properties; private final DeliveryCloser closer; - private final DeliverySpecRegistry registry; + private final ApplicationEventPublisher events; // ref = the delivery block the agent copied back from the command; the row is looked up by that exact dispatch public void acknowledge(DeliveryRef ref, String machineId) { @@ -41,7 +38,7 @@ public void acknowledge(DeliveryRef ref, String machineId) { boolean acked = repository.markAcked(id, dispatchId, DeliveryStatus.UNACKED, now, resultDueAt); if (acked) { log.info("Delivery ACKED: type={} targetId={} machineId={} dispatchId={}", type, targetId, machineId, dispatchId); - repository.findById(id).ifPresent(this::notifyAcked); + events.publishEvent(new DeliveryAckedEvent(this, type, targetId, machineId, dispatchId)); } else { log.debug("Delivery ack ignored, no unacked row for this dispatch: id={} dispatchId={}", id, dispatchId); } @@ -83,12 +80,6 @@ public void fail(DeliveryRef ref, String machineId, String error) { closer.failReported(ref.getType(), ref.getTargetId(), machineId, ref.getDispatchId(), error, now); } - private void notifyAcked(MachineDelivery delivery) { - DeliveryType type = delivery.getType(); - Optional> spec = registry.find(type); - spec.ifPresent(registered -> registered.onAcked(delivery)); - } - private Instant expiresAt(DeliveryType type, Instant now) { Policy policy = properties.resolve(type); long ttlSeconds = policy.getTtlSeconds(); diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryDispatcherTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryDispatcherTest.java index 9307bab4b6..8be953d96c 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryDispatcherTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryDispatcherTest.java @@ -57,7 +57,6 @@ void setUp() { void dispatch_seed_dispatchIdSetThenRecordedThenPublishedToSpecSubject() { // setup doReturn(spec).when(registry).require(DeliveryType.TOOL_INSTALLATION); - when(spec.canDispatch(seed)).thenReturn(true); when(spec.request(seed)).thenReturn(request); when(spec.subject(MACHINE_ID)).thenReturn("machine.mach-42.test"); when(publisherProvider.getObject()).thenReturn(publisher); @@ -73,19 +72,6 @@ void dispatch_seed_dispatchIdSetThenRecordedThenPublishedToSpecSubject() { verify(publisher).publish("machine.mach-42.test", payload); } - @Test - void dispatch_specDeclines_nothingRecordedOrPublished() { - // setup - doReturn(spec).when(registry).require(DeliveryType.TOOL_INSTALLATION); - when(spec.canDispatch(seed)).thenReturn(false); - - // execution - dispatcher.dispatch(seed); - - // verifications - verifyNoInteractions(recorder, publisherProvider); - } - @Test void dispatch_unregisteredType_throwsWithoutRecording() { // setup diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryCloserTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryCloserTest.java index a448ba4338..0c3210f57c 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryCloserTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryCloserTest.java @@ -7,23 +7,21 @@ import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryProperties; import com.openframe.delivery.config.DeliveryTestPolicies; +import com.openframe.delivery.event.DeliveryFailedEvent; import com.openframe.delivery.metrics.DeliveryMetrics; -import com.openframe.delivery.spec.DeliverySpec; -import com.openframe.delivery.spec.DeliverySpecRegistry; -import com.openframe.delivery.spec.TestPayload; -import com.openframe.delivery.spec.TestSeed; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.context.ApplicationEventPublisher; import java.time.Instant; -import java.util.Optional; import static com.openframe.delivery.config.DeliveryTestPolicies.TTL; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -39,9 +37,10 @@ class DeliveryCloserTest { private static final String ERROR = "download failed"; @Mock private MachineDeliveryRepository repository; - @Mock private DeliverySpecRegistry registry; @Mock private DeliveryMetrics metrics; - @Mock private DeliverySpec spec; + @Mock private ApplicationEventPublisher events; + + @Captor private ArgumentCaptor failedCaptor; private DeliveryCloser closer; @@ -56,20 +55,22 @@ void setUp() { delivery = MachineDelivery.builder() .id(DELIVERY_ID) .type(DeliveryType.CLIENT_UNINSTALL) + .targetId(TARGET_ID) + .machineId(MACHINE_ID) + .dispatchId(DISPATCH_ID) .status(DeliveryStatus.PENDING) .attempts(5) .dispatchedAt(dispatchedAt) .build(); DeliveryProperties properties = DeliveryTestPolicies.properties(); - closer = new DeliveryCloser(repository, registry, properties, metrics); + closer = new DeliveryCloser(repository, events, properties, metrics); } @Test - void fail_rowStillUnacked_rowFailedMetricCountedSpecNotified() { + void fail_rowStillUnacked_rowFailedMetricCountedEventPublished() { // setup Instant expiresAt = now.plusSeconds(TTL); when(repository.markFailed(DELIVERY_ID, DeliveryStatus.UNACKED, dispatchedAt, DeliveryFailure.EXHAUSTED, now, expiresAt)).thenReturn(true); - doReturn(Optional.of(spec)).when(registry).find(DeliveryType.CLIENT_UNINSTALL); // execution closer.fail(delivery, DeliveryFailure.EXHAUSTED, DeliveryStatus.UNACKED, now); @@ -80,7 +81,12 @@ void fail_rowStillUnacked_rowFailedMetricCountedSpecNotified() { assertThat(delivery.getFinishedAt()).isEqualTo(now); assertThat(delivery.getExpiresAt()).isEqualTo(expiresAt); verify(metrics).recordFailed(DeliveryType.CLIENT_UNINSTALL, DeliveryFailure.EXHAUSTED); - verify(spec).onFailed(delivery, DeliveryFailure.EXHAUSTED); + verify(events).publishEvent(failedCaptor.capture()); + DeliveryFailedEvent event = failedCaptor.getValue(); + assertThat(event.getType()).isEqualTo(DeliveryType.CLIENT_UNINSTALL); + assertThat(event.getMachineId()).isEqualTo(MACHINE_ID); + assertThat(event.getDispatchId()).isEqualTo(DISPATCH_ID); + assertThat(event.getFailure()).isEqualTo(DeliveryFailure.EXHAUSTED); } @Test @@ -94,38 +100,24 @@ void fail_rowAckedMeanwhile_nothingRecorded() { // verifications assertThat(delivery.getStatus()).isEqualTo(DeliveryStatus.PENDING); - verifyNoInteractions(metrics, registry, spec); - } - - @Test - void fail_typeWithoutSpec_rowStillFailedSpecSkipped() { - // setup - Instant expiresAt = now.plusSeconds(TTL); - when(repository.markFailed(DELIVERY_ID, DeliveryStatus.AWAITING_RESULT, dispatchedAt, DeliveryFailure.TIMEOUT, now, expiresAt)).thenReturn(true); - when(registry.find(DeliveryType.CLIENT_UNINSTALL)).thenReturn(Optional.empty()); - - // execution - closer.fail(delivery, DeliveryFailure.TIMEOUT, DeliveryStatus.AWAITING_RESULT, now); - - // verifications - assertThat(delivery.getStatus()).isEqualTo(DeliveryStatus.FAILED); - verify(metrics).recordFailed(DeliveryType.CLIENT_UNINSTALL, DeliveryFailure.TIMEOUT); - verifyNoInteractions(spec); + verifyNoInteractions(metrics, events); } @Test - void failReported_openRowOfThisDispatch_rowFailedMetricCountedSpecNotified() { + void failReported_openRowOfThisDispatch_rowFailedMetricCountedEventCarriesTheError() { // setup when(repository.markFailed(DELIVERY_ID, DISPATCH_ID, DeliveryStatus.OPEN, DeliveryFailure.AGENT_ERROR, ERROR, now, now.plusSeconds(TTL))).thenReturn(true); - when(repository.findById(DELIVERY_ID)).thenReturn(Optional.of(delivery)); - doReturn(Optional.of(spec)).when(registry).find(DeliveryType.CLIENT_UNINSTALL); // execution closer.failReported(DeliveryType.CLIENT_UNINSTALL, TARGET_ID, MACHINE_ID, DISPATCH_ID, ERROR, now); // verifications verify(metrics).recordFailed(DeliveryType.CLIENT_UNINSTALL, DeliveryFailure.AGENT_ERROR); - verify(spec).onFailed(delivery, DeliveryFailure.AGENT_ERROR); + verify(events).publishEvent(failedCaptor.capture()); + DeliveryFailedEvent event = failedCaptor.getValue(); + assertThat(event.getFailure()).isEqualTo(DeliveryFailure.AGENT_ERROR); + assertThat(event.getError()).isEqualTo(ERROR); + assertThat(event.getMachineId()).isEqualTo(MACHINE_ID); } @Test @@ -137,7 +129,7 @@ void failReported_rowOfAnotherDispatchOrClosed_nothingRecorded() { closer.failReported(DeliveryType.CLIENT_UNINSTALL, TARGET_ID, MACHINE_ID, DISPATCH_ID, ERROR, now); // verifications - verifyNoInteractions(metrics, registry, spec); + verifyNoInteractions(metrics, events); } @Test @@ -151,6 +143,6 @@ void cancel_rowStillUnackedSameDispatch_rowCancelledWithTtlExpiry() { // verifications verify(repository).markCancelled(DELIVERY_ID, DeliveryStatus.UNACKED, dispatchedAt, now, expiresAt); - verifyNoInteractions(registry, metrics); + verifyNoInteractions(metrics, events); } } diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java index ba15db4feb..6aa72600dc 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java @@ -2,13 +2,10 @@ import com.openframe.data.document.delivery.DeliveryStatus; import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.data.document.delivery.MachineDelivery; import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryTestPolicies; +import com.openframe.delivery.event.DeliveryAckedEvent; import com.openframe.delivery.spec.DeliveryRef; -import com.openframe.delivery.spec.DeliverySpec; -import com.openframe.delivery.spec.DeliverySpecRegistry; -import com.openframe.delivery.spec.TestPayload; import com.openframe.delivery.spec.TestSeed; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -17,16 +14,15 @@ import org.mockito.Captor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.context.ApplicationEventPublisher; import java.time.Instant; -import java.util.Optional; import static com.openframe.delivery.config.DeliveryTestPolicies.RESULT_TIMEOUT; import static com.openframe.delivery.config.DeliveryTestPolicies.TTL; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -43,17 +39,17 @@ class DeliveryTrackerTest { @Mock private MachineDeliveryRepository repository; @Mock private DeliveryCloser closer; - @Mock private DeliverySpecRegistry registry; - @Mock private DeliverySpec spec; + @Mock private ApplicationEventPublisher events; @Captor private ArgumentCaptor atCaptor; @Captor private ArgumentCaptor untilCaptor; + @Captor private ArgumentCaptor ackedCaptor; private DeliveryTracker tracker; @BeforeEach void setUp() { - tracker = new DeliveryTracker(repository, DeliveryTestPolicies.properties(), closer, registry); + tracker = new DeliveryTracker(repository, DeliveryTestPolicies.properties(), closer, events); } @Test @@ -70,22 +66,24 @@ void acknowledge_ref_unackedRowMarkedAckedWithResultDeadline() { } @Test - void acknowledge_rowJustAcked_specToldWithTheRow() { + void acknowledge_rowJustAcked_eventPublishedWithTheKey() { // setup - MachineDelivery row = MachineDelivery.builder().id(DELIVERY_ID).type(DeliveryType.TOOL_INSTALLATION).build(); when(repository.markAcked(eq(DELIVERY_ID), eq(DISPATCH_ID), eq(DeliveryStatus.UNACKED), any(Instant.class), any(Instant.class))).thenReturn(true); - when(repository.findById(DELIVERY_ID)).thenReturn(Optional.of(row)); - doReturn(Optional.of(spec)).when(registry).find(DeliveryType.TOOL_INSTALLATION); // execution tracker.acknowledge(REF, MACHINE_ID); // verifications - verify(spec).onAcked(row); + verify(events).publishEvent(ackedCaptor.capture()); + DeliveryAckedEvent event = ackedCaptor.getValue(); + assertThat(event.getType()).isEqualTo(DeliveryType.TOOL_INSTALLATION); + assertThat(event.getTargetId()).isEqualTo(TARGET_ID); + assertThat(event.getMachineId()).isEqualTo(MACHINE_ID); + assertThat(event.getDispatchId()).isEqualTo(DISPATCH_ID); } @Test - void acknowledge_staleDispatch_specNotTold() { + void acknowledge_staleDispatch_noEvent() { // setup when(repository.markAcked(eq(DELIVERY_ID), eq(DISPATCH_ID), eq(DeliveryStatus.UNACKED), any(Instant.class), any(Instant.class))).thenReturn(false); @@ -93,17 +91,16 @@ void acknowledge_staleDispatch_specNotTold() { tracker.acknowledge(REF, MACHINE_ID); // verifications - verifyNoInteractions(registry); + verifyNoInteractions(events); } @Test void done_seed_completableRowOfTheSeedKeyMarkedDone() { // setup - TestSeed seed = new TestSeed(MACHINE_ID); when(repository.markDone(eq(DELIVERY_ID), eq(DeliveryStatus.COMPLETABLE), atCaptor.capture(), untilCaptor.capture())).thenReturn(true); // execution - tracker.done(seed); + tracker.done(new TestSeed(MACHINE_ID)); // verifications Instant finishedAt = atCaptor.getValue(); @@ -125,8 +122,6 @@ void done_ref_openOrFailedRowMarkedDoneWithTtlExpiry() { @Test void fail_agentReportedError_closerAsked() { - // setup - // execution tracker.fail(REF, MACHINE_ID, ERROR); From 1902d8cae69d9ffa959bf125eeace79ccfd1835a Mon Sep 17 00:00:00 2001 From: semen-flamingo Date: Mon, 5 Oct 2026 15:20:23 +0200 Subject: [PATCH 7/9] refactor(delivery): machine status stays with the business plane; delivery is confirmed only over delivery.result Semen: delivery does not manage machine statuses. PENDING_DELETION is set by the force endpoint at send time on both paths, DELETED by the agent's own uninstall call, exactly as today; the delivery events, the listener and the MachineStatusService transitions are gone. The row is closed only through the agent's delivery.result (ACKED, then DONE), so done(seed) and the dispatchId-less markDone are gone too and AgentUninstallService is back to main. Because PENDING_DELETION freezes the status, the heartbeat now keeps lastSeen fresh for such machines and the sweep counts them as online while lastSeen is within sweep.online-threshold-seconds (130). Co-Authored-By: Claude Fable 5.1 --- .../service/ForceClientUninstallService.java | 18 +---- .../ForceClientUninstallServiceTest.java | 22 +----- .../ClientUninstallDeliveryListener.java | 40 ---------- .../client/service/AgentUninstallService.java | 4 - .../client/service/MachineStatusService.java | 30 ++----- .../ClientUninstallDeliveryListenerTest.java | 69 ---------------- .../service/AgentUninstallServiceTest.java | 78 ------------------- .../service/MachineStatusServiceTest.java | 61 +++------------ .../CustomMachineDeliveryRepository.java | 2 - .../CustomMachineDeliveryRepositoryImpl.java | 6 -- .../repository/device/MachineRepository.java | 2 + ...stomMachineDeliveryRepositoryImplTest.java | 21 ----- .../delivery/config/DeliveryProperties.java | 2 + .../delivery/event/DeliveryAckedEvent.java | 23 ------ .../delivery/event/DeliveryFailedEvent.java | 29 ------- .../delivery/sweep/MachineOnlineStatus.java | 13 +++- .../delivery/track/DeliveryCloser.java | 6 -- .../delivery/track/DeliveryTracker.java | 21 ----- .../sweep/MachineOnlineStatusTest.java | 51 +++++++----- .../delivery/track/DeliveryCloserTest.java | 30 ++----- .../delivery/track/DeliveryTrackerTest.java | 50 +----------- 21 files changed, 76 insertions(+), 502 deletions(-) delete mode 100644 openframe-client-core/src/main/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListener.java delete mode 100644 openframe-client-core/src/test/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListenerTest.java delete mode 100644 openframe-client-core/src/test/java/com/openframe/client/service/AgentUninstallServiceTest.java delete mode 100644 openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryAckedEvent.java delete mode 100644 openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryFailedEvent.java diff --git a/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java b/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java index a2b992a3b5..b2e4bb926e 100644 --- a/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java +++ b/openframe-api-service-core/src/main/java/com/openframe/api/service/ForceClientUninstallService.java @@ -64,12 +64,11 @@ private ForceClientUninstallResponseItem processMachine(String machineId) { } if (deliveryProperties.isEnabled(DeliveryType.CLIENT_UNINSTALL)) { - dispatch(machine); - return buildResponseItem(machineId, ForceAgentStatus.PROCESSED); + deliveryDispatcher.dispatch(new ClientUninstallDeliverySeed(machineId)); + } else { + clientUninstallNatsPublisher.publish(machineId); } - clientUninstallNatsPublisher.publish(machineId); - markPendingDeletion(machine); return buildResponseItem(machineId, ForceAgentStatus.PROCESSED); @@ -79,17 +78,6 @@ private ForceClientUninstallResponseItem processMachine(String machineId) { } } - // the machine leaves service when the agent acknowledges (client-service, on DeliveryAckedEvent), not here: the sweep - // needs its real status to retry. PENDING_DELETION therefore means an uninstall is already on the machine - private void dispatch(Machine machine) { - String machineId = machine.getMachineId(); - if (machine.getStatus() == PENDING_DELETION) { - log.info("Client uninstall already in progress for machine {}", machineId); - return; - } - deliveryDispatcher.dispatch(new ClientUninstallDeliverySeed(machineId)); - } - private void markPendingDeletion(Machine machine) { if (machine.getStatus() == PENDING_DELETION) { return; diff --git a/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java b/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java index 92bb4f8cf7..ef5ddd5685 100644 --- a/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java +++ b/openframe-api-service-core/src/test/java/com/openframe/api/service/ForceClientUninstallServiceTest.java @@ -24,8 +24,6 @@ import java.util.Optional; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -74,7 +72,7 @@ void process_flagOff_publishedToJetStreamAndMarkedPendingDeletion() { } @Test - void process_flagOn_dispatchedThroughEngineStatusUntouched() { + void process_flagOn_dispatchedThroughEngineAndMarkedPendingDeletion() { // setup when(deliveryProperties.isEnabled(DeliveryType.CLIENT_UNINSTALL)).thenReturn(true); @@ -84,23 +82,9 @@ void process_flagOn_dispatchedThroughEngineStatusUntouched() { // verifications verify(deliveryDispatcher).dispatch(seedCaptor.capture()); assertThat(seedCaptor.getValue().getMachineId()).isEqualTo(MACHINE_ID); - assertThat(machine.getStatus()).isEqualTo(DeviceStatus.ONLINE); - verify(machineRepository, never()).save(any()); + assertThat(machine.getStatus()).isEqualTo(DeviceStatus.PENDING_DELETION); + verify(machineRepository).save(machine); verifyNoInteractions(clientUninstallNatsPublisher); assertThat(response.getItems().get(0).getStatus()).isEqualTo(ForceAgentStatus.PROCESSED); } - - @Test - void process_flagOnUninstallAlreadyAcknowledged_notDispatchedAgain() { - // setup - machine.setStatus(DeviceStatus.PENDING_DELETION); - when(deliveryProperties.isEnabled(DeliveryType.CLIENT_UNINSTALL)).thenReturn(true); - - // execution - ForceClientUninstallResponse response = service.process(request); - - // verifications - verifyNoInteractions(deliveryDispatcher, clientUninstallNatsPublisher); - assertThat(response.getItems().get(0).getStatus()).isEqualTo(ForceAgentStatus.PROCESSED); - } } diff --git a/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListener.java b/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListener.java deleted file mode 100644 index 69734cd413..0000000000 --- a/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListener.java +++ /dev/null @@ -1,40 +0,0 @@ -package com.openframe.client.listener.delivery; - -import com.openframe.client.service.MachineStatusService; -import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.delivery.event.DeliveryAckedEvent; -import com.openframe.delivery.event.DeliveryFailedEvent; -import lombok.RequiredArgsConstructor; -import lombok.extern.slf4j.Slf4j; -import org.springframework.context.event.EventListener; -import org.springframework.stereotype.Component; - -// what a client uninstall delivery means for the machine document; the engine itself knows nothing about machines -@Slf4j -@Component -@RequiredArgsConstructor -public class ClientUninstallDeliveryListener { - - private final MachineStatusService machineStatusService; - - @EventListener - public void onAcked(DeliveryAckedEvent event) { - if (event.getType() != DeliveryType.CLIENT_UNINSTALL) { - return; - } - machineStatusService.markDeletionAcknowledged(event.getMachineId()); - } - - @EventListener - public void onFailed(DeliveryFailedEvent event) { - if (event.getType() != DeliveryType.CLIENT_UNINSTALL) { - return; - } - String machineId = event.getMachineId(); - boolean handedBack = machineStatusService.cancelPendingDeletion(machineId); - if (handedBack) { - log.error("Client uninstall failed after the agent acknowledged it, machine {} returned to OFFLINE: reason={} error={}", - machineId, event.getFailure(), event.getError()); - } - } -} diff --git a/openframe-client-core/src/main/java/com/openframe/client/service/AgentUninstallService.java b/openframe-client-core/src/main/java/com/openframe/client/service/AgentUninstallService.java index a57a88b1b3..d0b0231240 100644 --- a/openframe-client-core/src/main/java/com/openframe/client/service/AgentUninstallService.java +++ b/openframe-client-core/src/main/java/com/openframe/client/service/AgentUninstallService.java @@ -4,10 +4,8 @@ import com.openframe.data.document.device.DeviceStatus; import com.openframe.data.document.device.Machine; import com.openframe.data.document.oauth.OAuthClient; -import com.openframe.data.nats.delivery.ClientUninstallDeliverySeed; import com.openframe.data.repository.device.MachineRepository; import com.openframe.data.repository.oauth.OAuthClientRepository; -import com.openframe.delivery.track.DeliveryTracker; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.stereotype.Service; @@ -24,7 +22,6 @@ public class AgentUninstallService { private final MachineRepository machineRepository; private final ToolConnectionService toolConnectionService; private final InstalledAgentService installedAgentService; - private final DeliveryTracker deliveryTracker; public void uninstall(String machineId, String clientSecret) { Optional client = oauthClientRepository.findByMachineId(machineId); @@ -51,7 +48,6 @@ public void uninstall(String machineId, String clientSecret) { toolConnectionService.disconnectAll(machineId); installedAgentService.disconnectAll(machineId); - deliveryTracker.done(new ClientUninstallDeliverySeed(machineId)); log.info("Machine {} deregistered on uninstall", machineId); } diff --git a/openframe-client-core/src/main/java/com/openframe/client/service/MachineStatusService.java b/openframe-client-core/src/main/java/com/openframe/client/service/MachineStatusService.java index 4164eae36c..81be81eb7b 100644 --- a/openframe-client-core/src/main/java/com/openframe/client/service/MachineStatusService.java +++ b/openframe-client-core/src/main/java/com/openframe/client/service/MachineStatusService.java @@ -38,31 +38,6 @@ public void processHeartbeat(String machineId, Instant eventTimestamp) { update(machineId, ONLINE, eventTimestamp); } - // the agent holds its uninstall command: from here on status events are ignored until the machine is gone - public void markDeletionAcknowledged(String machineId) { - machineRepository.findByMachineId(machineId).ifPresent(machine -> { - if (isDeletionInProgress(machine)) { - return; - } - machine.setStatus(PENDING_DELETION); - machineRepository.save(machine); - log.info("Machine {} marked PENDING_DELETION: uninstall acknowledged by the agent", machineId); - }); - } - - // the uninstall did not happen: hand the machine back as OFFLINE, the next heartbeat sets ONLINE again - public boolean cancelPendingDeletion(String machineId) { - return machineRepository.findByMachineId(machineId) - .filter(machine -> machine.getStatus() == PENDING_DELETION) - .map(machine -> { - machine.setStatus(OFFLINE); - machineRepository.save(machine); - log.warn("Machine {} returned to OFFLINE: pending deletion cancelled", machineId); - return true; - }) - .orElse(false); - } - private void update(String machineId, DeviceStatus newStatus, Instant eventTimestamp) { log.debug("Received status update event to {} for machineId={} eventTimestamp={}", newStatus, machineId, eventTimestamp); @@ -70,6 +45,11 @@ private void update(String machineId, DeviceStatus newStatus, Instant eventTimes .orElseThrow(() -> new MachineNotFoundException(machineId)); if (isDeletionInProgress(machine)) { + // the status is frozen until the agent is gone, but a fresh lastSeen still tells the delivery sweep the machine is reachable + if (machine.getStatus() == PENDING_DELETION && isEventNewer(eventTimestamp, machine.getLastSeen())) { + touchLastSeen(machine, eventTimestamp); + return; + } log.debug("Ignoring {} event for machineId={} in status {}", newStatus, machineId, machine.getStatus()); return; } diff --git a/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListenerTest.java b/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListenerTest.java deleted file mode 100644 index 0bfe2c8892..0000000000 --- a/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/ClientUninstallDeliveryListenerTest.java +++ /dev/null @@ -1,69 +0,0 @@ -package com.openframe.client.listener.delivery; - -import com.openframe.client.service.MachineStatusService; -import com.openframe.data.document.delivery.DeliveryFailure; -import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.delivery.event.DeliveryAckedEvent; -import com.openframe.delivery.event.DeliveryFailedEvent; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.InjectMocks; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; - -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; -import static org.mockito.Mockito.when; - -@ExtendWith(MockitoExtension.class) -class ClientUninstallDeliveryListenerTest { - - private static final String MACHINE_ID = "mach-42"; - private static final String TARGET_ID = "openframe-client"; - private static final String DISPATCH_ID = "d-1"; - - @Mock private MachineStatusService machineStatusService; - - @InjectMocks private ClientUninstallDeliveryListener listener; - - @Test - void onAcked_clientUninstall_deletionMarkedAcknowledged() { - // execution - listener.onAcked(new DeliveryAckedEvent(this, DeliveryType.CLIENT_UNINSTALL, TARGET_ID, MACHINE_ID, DISPATCH_ID)); - - // verifications - verify(machineStatusService).markDeletionAcknowledged(MACHINE_ID); - } - - @Test - void onAcked_otherType_ignored() { - // execution - listener.onAcked(new DeliveryAckedEvent(this, DeliveryType.TOOL_INSTALLATION, "fleetmdm-agent", MACHINE_ID, DISPATCH_ID)); - - // verifications - verifyNoInteractions(machineStatusService); - } - - @Test - void onFailed_clientUninstall_pendingDeletionCancelled() { - // setup - when(machineStatusService.cancelPendingDeletion(MACHINE_ID)).thenReturn(true); - - // execution - listener.onFailed(new DeliveryFailedEvent(this, DeliveryType.CLIENT_UNINSTALL, TARGET_ID, MACHINE_ID, DISPATCH_ID, - DeliveryFailure.TIMEOUT, null)); - - // verifications - verify(machineStatusService).cancelPendingDeletion(MACHINE_ID); - } - - @Test - void onFailed_otherType_ignored() { - // execution - listener.onFailed(new DeliveryFailedEvent(this, DeliveryType.TOOL_INSTALLATION, "fleetmdm-agent", MACHINE_ID, DISPATCH_ID, - DeliveryFailure.EXHAUSTED, null)); - - // verifications - verifyNoInteractions(machineStatusService); - } -} diff --git a/openframe-client-core/src/test/java/com/openframe/client/service/AgentUninstallServiceTest.java b/openframe-client-core/src/test/java/com/openframe/client/service/AgentUninstallServiceTest.java deleted file mode 100644 index 8b67b32c93..0000000000 --- a/openframe-client-core/src/test/java/com/openframe/client/service/AgentUninstallServiceTest.java +++ /dev/null @@ -1,78 +0,0 @@ -package com.openframe.client.service; - -import com.openframe.client.service.validator.ClientSecretValidator; -import com.openframe.data.document.device.DeviceStatus; -import com.openframe.data.document.device.Machine; -import com.openframe.data.document.oauth.OAuthClient; -import com.openframe.data.nats.delivery.ClientUninstallDeliverySeed; -import com.openframe.data.repository.device.MachineRepository; -import com.openframe.data.repository.oauth.OAuthClientRepository; -import com.openframe.delivery.track.DeliveryTracker; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.ArgumentCaptor; -import org.mockito.Captor; -import org.mockito.InjectMocks; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; - -import java.util.Optional; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; -import static org.mockito.Mockito.when; - -@ExtendWith(MockitoExtension.class) -class AgentUninstallServiceTest { - - private static final String MACHINE_ID = "mach-42"; - private static final String CLIENT_SECRET = "secret"; - - @Mock private OAuthClientRepository oauthClientRepository; - @Mock private ClientSecretValidator clientSecretValidator; - @Mock private MachineRepository machineRepository; - @Mock private ToolConnectionService toolConnectionService; - @Mock private InstalledAgentService installedAgentService; - @Mock private DeliveryTracker deliveryTracker; - - @Captor private ArgumentCaptor seedCaptor; - - @InjectMocks private AgentUninstallService service; - - private Machine machine; - - @BeforeEach - void setUp() { - machine = new Machine(); - machine.setMachineId(MACHINE_ID); - machine.setStatus(DeviceStatus.PENDING_DELETION); - when(oauthClientRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(new OAuthClient())); - when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); - } - - @Test - void uninstall_machineInService_deletedAndDeliveryRowClosed() { - // execution - service.uninstall(MACHINE_ID, CLIENT_SECRET); - - // verifications - assertThat(machine.getStatus()).isEqualTo(DeviceStatus.DELETED); - verify(machineRepository).save(machine); - verify(deliveryTracker).done(seedCaptor.capture()); - assertThat(seedCaptor.getValue().getMachineId()).isEqualTo(MACHINE_ID); - } - - @Test - void uninstall_machineAlreadyDeleted_nothingTouched() { - // setup - machine.setStatus(DeviceStatus.DELETED); - - // execution - service.uninstall(MACHINE_ID, CLIENT_SECRET); - - // verifications - verifyNoInteractions(deliveryTracker, toolConnectionService, installedAgentService); - } -} diff --git a/openframe-client-core/src/test/java/com/openframe/client/service/MachineStatusServiceTest.java b/openframe-client-core/src/test/java/com/openframe/client/service/MachineStatusServiceTest.java index 545926676b..9f64075e4f 100644 --- a/openframe-client-core/src/test/java/com/openframe/client/service/MachineStatusServiceTest.java +++ b/openframe-client-core/src/test/java/com/openframe/client/service/MachineStatusServiceTest.java @@ -215,78 +215,37 @@ void unknownMachine_throwsAndWritesNothing() { } @Test - @DisplayName("T11: a device being deleted is left alone — neither save() nor updateLastSeen") - void pendingDeletion_touchesNothing() { - machineIs(DeviceStatus.PENDING_DELETION); - - service.processHeartbeat(MACHINE, LATER); - - verify(machineRepository, never()).save(any(Machine.class)); - verify(machineRepository, never()).updateLastSeen(anyString(), any(Instant.class)); - } - - @Test - void markDeletionAcknowledged_machineInService_pendingDeletion() { + void processHeartbeat_machinePendingDeletion_lastSeenRefreshedStatusKept() { // setup Machine machine = new Machine(); machine.setMachineId(MACHINE); - machine.setStatus(DeviceStatus.ONLINE); + machine.setStatus(DeviceStatus.PENDING_DELETION); + machine.setLastSeen(SEEN); when(machineRepository.findByMachineId(MACHINE)).thenReturn(Optional.of(machine)); // execution - service.markDeletionAcknowledged(MACHINE); + service.processHeartbeat(MACHINE, LATER); // verifications + verify(machineRepository).updateLastSeen(MACHINE, LATER); + verify(machineRepository, never()).save(any()); assertThat(machine.getStatus()).isEqualTo(DeviceStatus.PENDING_DELETION); - verify(machineRepository).save(machine); } @Test - void markDeletionAcknowledged_machineAlreadyDeleted_untouched() { + void processHeartbeat_machineDeleted_ignored() { // setup Machine machine = new Machine(); machine.setMachineId(MACHINE); machine.setStatus(DeviceStatus.DELETED); + machine.setLastSeen(SEEN); when(machineRepository.findByMachineId(MACHINE)).thenReturn(Optional.of(machine)); // execution - service.markDeletionAcknowledged(MACHINE); - - // verifications - assertThat(machine.getStatus()).isEqualTo(DeviceStatus.DELETED); - verify(machineRepository, never()).save(any()); - } - - @Test - void cancelPendingDeletion_machinePendingDeletion_offlineAndTrue() { - // setup - Machine machine = new Machine(); - machine.setMachineId(MACHINE); - machine.setStatus(DeviceStatus.PENDING_DELETION); - when(machineRepository.findByMachineId(MACHINE)).thenReturn(Optional.of(machine)); - - // execution - boolean handedBack = service.cancelPendingDeletion(MACHINE); - - // verifications - assertThat(handedBack).isTrue(); - assertThat(machine.getStatus()).isEqualTo(DeviceStatus.OFFLINE); - verify(machineRepository).save(machine); - } - - @Test - void cancelPendingDeletion_machineNotPendingDeletion_falseAndUntouched() { - // setup - Machine machine = new Machine(); - machine.setMachineId(MACHINE); - machine.setStatus(DeviceStatus.ONLINE); - when(machineRepository.findByMachineId(MACHINE)).thenReturn(Optional.of(machine)); - - // execution - boolean handedBack = service.cancelPendingDeletion(MACHINE); + service.processHeartbeat(MACHINE, LATER); // verifications - assertThat(handedBack).isFalse(); + verify(machineRepository, never()).updateLastSeen(anyString(), any()); verify(machineRepository, never()).save(any()); } } diff --git a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/CustomMachineDeliveryRepository.java b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/CustomMachineDeliveryRepository.java index 4225a89ad6..50ed6ef145 100644 --- a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/CustomMachineDeliveryRepository.java +++ b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/CustomMachineDeliveryRepository.java @@ -25,8 +25,6 @@ public interface CustomMachineDeliveryRepository { boolean markDone(String id, String dispatchId, Set from, Instant finishedAt, Instant expiresAt); - boolean markDone(String id, Set from, Instant finishedAt, Instant expiresAt); - boolean markCancelled(String id, Set from, Instant dispatchedAt, Instant finishedAt, Instant expiresAt); boolean markFailed(String id, Set from, Instant dispatchedAt, DeliveryFailure failure, Instant finishedAt, Instant expiresAt); diff --git a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java index 6396c03825..faddca2802 100644 --- a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java +++ b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java @@ -110,12 +110,6 @@ public boolean markDone(String id, String dispatchId, Set from, return updateOne(thisDispatch(id, from, dispatchId), update); } - @Override - public boolean markDone(String id, Set from, Instant finishedAt, Instant expiresAt) { - Update update = closed(DeliveryStatus.DONE, finishedAt, expiresAt); - return updateOne(stillIn(id, from), update); - } - @Override public boolean markCancelled(String id, Set from, Instant dispatchedAt, Instant finishedAt, Instant expiresAt) { Update update = closed(DeliveryStatus.CANCELLED, finishedAt, expiresAt); diff --git a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/device/MachineRepository.java b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/device/MachineRepository.java index a9682dbfc1..dcf730a46e 100644 --- a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/device/MachineRepository.java +++ b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/device/MachineRepository.java @@ -35,6 +35,8 @@ public interface MachineRepository extends MongoRepository, Cus List findByMachineIdInAndStatus(Collection machineIds, DeviceStatus status); + List findByMachineIdInAndStatusAndLastSeenAfter(Collection machineIds, DeviceStatus status, Instant lastSeenAfter); + List findByMachineIdInAndStatusIn(Collection machineIds, Collection statuses); List findByStatusIn(Collection statuses); diff --git a/openframe-data-mongo-sync/src/test/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImplTest.java b/openframe-data-mongo-sync/src/test/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImplTest.java index 9f47cc9e00..022540e6b6 100644 --- a/openframe-data-mongo-sync/src/test/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImplTest.java +++ b/openframe-data-mongo-sync/src/test/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImplTest.java @@ -162,27 +162,6 @@ void markAcked_unackedRowOfThisDispatch_ackedAndTrue() { .contains("dueAt"); } - @Test - void markDone_withoutDispatchId_completableRowClosedPayloadDropped() { - // setup - UpdateResult oneRow = UpdateResult.acknowledged(1, 1L, null); - when(mongoTemplate.updateFirst(queryCaptor.capture(), updateCaptor.capture(), eq(MachineDelivery.class))).thenReturn(oneRow); - - // execution - boolean done = repository.markDone(ID, DeliveryStatus.COMPLETABLE, now, now); - - // verifications - assertThat(done).isTrue(); - assertThat(queryCaptor.getValue().getQueryObject().toString()) - .contains(ID) - .contains("PENDING") - .contains("FAILED") - .doesNotContain("dispatchId"); - assertThat(updateCaptor.getValue().getUpdateObject().toString()) - .contains("DONE") - .contains("payloadJson"); - } - @Test void postponeAfterError_pendingRow_errorCountedAndDueMoved() { // setup diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/config/DeliveryProperties.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/config/DeliveryProperties.java index 7d3482cc1b..07334f4385 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/config/DeliveryProperties.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/config/DeliveryProperties.java @@ -60,6 +60,8 @@ public static class Sweep { @NotNull @Positive private Integer batchSize; + // a PENDING_DELETION machine keeps its status until the agent is gone; it counts as online while its heartbeat is this fresh + private long onlineThresholdSeconds = 130; } @Getter diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryAckedEvent.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryAckedEvent.java deleted file mode 100644 index 346d7125cf..0000000000 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryAckedEvent.java +++ /dev/null @@ -1,23 +0,0 @@ -package com.openframe.delivery.event; - -import com.openframe.data.document.delivery.DeliveryType; -import lombok.Getter; -import org.springframework.context.ApplicationEvent; - -// the agent confirmed it holds the command; published once, on the real PENDING -> ACKED transition -@Getter -public class DeliveryAckedEvent extends ApplicationEvent { - - private final DeliveryType type; - private final String targetId; - private final String machineId; - private final String dispatchId; - - public DeliveryAckedEvent(Object source, DeliveryType type, String targetId, String machineId, String dispatchId) { - super(source); - this.type = type; - this.targetId = targetId; - this.machineId = machineId; - this.dispatchId = dispatchId; - } -} diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryFailedEvent.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryFailedEvent.java deleted file mode 100644 index db6a1290d4..0000000000 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/event/DeliveryFailedEvent.java +++ /dev/null @@ -1,29 +0,0 @@ -package com.openframe.delivery.event; - -import com.openframe.data.document.delivery.DeliveryFailure; -import com.openframe.data.document.delivery.DeliveryType; -import lombok.Getter; -import org.springframework.context.ApplicationEvent; - -// the row closed as FAILED: retries exhausted, machine never came back, no result after the ACK, or the agent reported an error -@Getter -public class DeliveryFailedEvent extends ApplicationEvent { - - private final DeliveryType type; - private final String targetId; - private final String machineId; - private final String dispatchId; - private final DeliveryFailure failure; - private final String error; - - public DeliveryFailedEvent(Object source, DeliveryType type, String targetId, String machineId, String dispatchId, - DeliveryFailure failure, String error) { - super(source); - this.type = type; - this.targetId = targetId; - this.machineId = machineId; - this.dispatchId = dispatchId; - this.failure = failure; - this.error = error; - } -} diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/MachineOnlineStatus.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/MachineOnlineStatus.java index 0375bba728..743fb981f7 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/MachineOnlineStatus.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/MachineOnlineStatus.java @@ -3,11 +3,14 @@ import com.openframe.data.document.device.DeviceStatus; import com.openframe.data.document.device.Machine; import com.openframe.data.repository.device.MachineRepository; +import com.openframe.delivery.config.DeliveryProperties; import lombok.RequiredArgsConstructor; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.stereotype.Component; +import java.time.Instant; import java.util.EnumSet; +import java.util.HashSet; import java.util.List; import java.util.Set; @@ -22,10 +25,18 @@ public class MachineOnlineStatus { DeviceStatus.DELETED, DeviceStatus.ARCHIVED, DeviceStatus.DECOMMISSIONED); private final MachineRepository machineRepository; + private final DeliveryProperties properties; + // PENDING_DELETION freezes the status, so for those machines presence is read from the heartbeat instead public Set online(Set machineIds) { List online = machineRepository.findByMachineIdInAndStatus(machineIds, DeviceStatus.ONLINE); - return ids(online); + long thresholdSeconds = properties.getSweep().getOnlineThresholdSeconds(); + Instant lastSeenAfter = Instant.now().minusSeconds(thresholdSeconds); + List leaving = machineRepository.findByMachineIdInAndStatusAndLastSeenAfter( + machineIds, DeviceStatus.PENDING_DELETION, lastSeenAfter); + Set ids = new HashSet<>(ids(online)); + ids.addAll(ids(leaving)); + return ids; } public Set gone(Set machineIds) { diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryCloser.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryCloser.java index 3edc6f4845..6976c83cdb 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryCloser.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryCloser.java @@ -7,11 +7,9 @@ import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryProperties; import com.openframe.delivery.config.DeliveryProperties.Policy; -import com.openframe.delivery.event.DeliveryFailedEvent; import com.openframe.delivery.metrics.DeliveryMetrics; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; -import org.springframework.context.ApplicationEventPublisher; import org.springframework.stereotype.Component; import java.time.Instant; @@ -23,7 +21,6 @@ public class DeliveryCloser { private final MachineDeliveryRepository repository; - private final ApplicationEventPublisher events; private final DeliveryProperties properties; private final DeliveryMetrics metrics; @@ -44,8 +41,6 @@ public void fail(MachineDelivery delivery, DeliveryFailure failure, Set GONE = EnumSet.of( - DeviceStatus.DELETED, DeviceStatus.ARCHIVED, DeviceStatus.DECOMMISSIONED); + private static final String LEAVING_ID = "mach-2"; + private static final String GONE_ID = "mach-3"; + private static final Set ALL = Set.of(ONLINE_ID, LEAVING_ID, GONE_ID); @Mock private MachineRepository machineRepository; - @InjectMocks private MachineOnlineStatus status; + @Captor private ArgumentCaptor lastSeenAfterCaptor; + + private MachineOnlineStatus status; + + @BeforeEach + void setUp() { + status = new MachineOnlineStatus(machineRepository, DeliveryTestPolicies.properties()); + } @Test - void online_mixedMachines_onlyOnlineIdsReturned() { + void online_onlineMachinesAndPendingDeletionWithFreshHeartbeat_bothCounted() { // setup - Set asked = Set.of(ONLINE_ID, OFFLINE_ID); - List found = List.of(machine(ONLINE_ID)); - when(machineRepository.findByMachineIdInAndStatus(asked, DeviceStatus.ONLINE)).thenReturn(found); + Instant before = Instant.now(); + long threshold = DeliveryTestPolicies.properties().getSweep().getOnlineThresholdSeconds(); + when(machineRepository.findByMachineIdInAndStatus(ALL, DeviceStatus.ONLINE)).thenReturn(List.of(machine(ONLINE_ID))); + when(machineRepository.findByMachineIdInAndStatusAndLastSeenAfter(eq(ALL), eq(DeviceStatus.PENDING_DELETION), lastSeenAfterCaptor.capture())) + .thenReturn(List.of(machine(LEAVING_ID))); // execution - Set online = status.online(asked); + Set online = status.online(ALL); // verifications - assertThat(online).containsExactly(ONLINE_ID); + assertThat(online).containsExactlyInAnyOrder(ONLINE_ID, LEAVING_ID); + assertThat(lastSeenAfterCaptor.getValue()) + .isBetween(before.minusSeconds(threshold + 5), before.minusSeconds(threshold).plusSeconds(5)); } @Test - void gone_mixedMachines_onlyRemovedIdsReturned() { + void gone_deletedArchivedDecommissioned_returned() { // setup - Set asked = Set.of(ONLINE_ID, DELETED_ID); - List found = List.of(machine(DELETED_ID)); - when(machineRepository.findByMachineIdInAndStatusIn(asked, GONE)).thenReturn(found); + when(machineRepository.findByMachineIdInAndStatusIn(eq(ALL), eq(Set.of(DeviceStatus.DELETED, DeviceStatus.ARCHIVED, DeviceStatus.DECOMMISSIONED)))) + .thenReturn(List.of(machine(GONE_ID))); // execution - Set gone = status.gone(asked); + Set gone = status.gone(ALL); // verifications - assertThat(gone).containsExactly(DELETED_ID); + assertThat(gone).containsExactly(GONE_ID); } private static Machine machine(String machineId) { diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryCloserTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryCloserTest.java index 0c3210f57c..8bd88368ec 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryCloserTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryCloserTest.java @@ -7,16 +7,12 @@ import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryProperties; import com.openframe.delivery.config.DeliveryTestPolicies; -import com.openframe.delivery.event.DeliveryFailedEvent; import com.openframe.delivery.metrics.DeliveryMetrics; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.ArgumentCaptor; -import org.mockito.Captor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import org.springframework.context.ApplicationEventPublisher; import java.time.Instant; @@ -38,9 +34,6 @@ class DeliveryCloserTest { @Mock private MachineDeliveryRepository repository; @Mock private DeliveryMetrics metrics; - @Mock private ApplicationEventPublisher events; - - @Captor private ArgumentCaptor failedCaptor; private DeliveryCloser closer; @@ -63,11 +56,11 @@ void setUp() { .dispatchedAt(dispatchedAt) .build(); DeliveryProperties properties = DeliveryTestPolicies.properties(); - closer = new DeliveryCloser(repository, events, properties, metrics); + closer = new DeliveryCloser(repository, properties, metrics); } @Test - void fail_rowStillUnacked_rowFailedMetricCountedEventPublished() { + void fail_rowStillUnacked_rowFailedMetricCounted() { // setup Instant expiresAt = now.plusSeconds(TTL); when(repository.markFailed(DELIVERY_ID, DeliveryStatus.UNACKED, dispatchedAt, DeliveryFailure.EXHAUSTED, now, expiresAt)).thenReturn(true); @@ -81,12 +74,6 @@ void fail_rowStillUnacked_rowFailedMetricCountedEventPublished() { assertThat(delivery.getFinishedAt()).isEqualTo(now); assertThat(delivery.getExpiresAt()).isEqualTo(expiresAt); verify(metrics).recordFailed(DeliveryType.CLIENT_UNINSTALL, DeliveryFailure.EXHAUSTED); - verify(events).publishEvent(failedCaptor.capture()); - DeliveryFailedEvent event = failedCaptor.getValue(); - assertThat(event.getType()).isEqualTo(DeliveryType.CLIENT_UNINSTALL); - assertThat(event.getMachineId()).isEqualTo(MACHINE_ID); - assertThat(event.getDispatchId()).isEqualTo(DISPATCH_ID); - assertThat(event.getFailure()).isEqualTo(DeliveryFailure.EXHAUSTED); } @Test @@ -100,11 +87,11 @@ void fail_rowAckedMeanwhile_nothingRecorded() { // verifications assertThat(delivery.getStatus()).isEqualTo(DeliveryStatus.PENDING); - verifyNoInteractions(metrics, events); + verifyNoInteractions(metrics); } @Test - void failReported_openRowOfThisDispatch_rowFailedMetricCountedEventCarriesTheError() { + void failReported_openRowOfThisDispatch_rowFailedMetricCounted() { // setup when(repository.markFailed(DELIVERY_ID, DISPATCH_ID, DeliveryStatus.OPEN, DeliveryFailure.AGENT_ERROR, ERROR, now, now.plusSeconds(TTL))).thenReturn(true); @@ -113,11 +100,6 @@ void failReported_openRowOfThisDispatch_rowFailedMetricCountedEventCarriesTheErr // verifications verify(metrics).recordFailed(DeliveryType.CLIENT_UNINSTALL, DeliveryFailure.AGENT_ERROR); - verify(events).publishEvent(failedCaptor.capture()); - DeliveryFailedEvent event = failedCaptor.getValue(); - assertThat(event.getFailure()).isEqualTo(DeliveryFailure.AGENT_ERROR); - assertThat(event.getError()).isEqualTo(ERROR); - assertThat(event.getMachineId()).isEqualTo(MACHINE_ID); } @Test @@ -129,7 +111,7 @@ void failReported_rowOfAnotherDispatchOrClosed_nothingRecorded() { closer.failReported(DeliveryType.CLIENT_UNINSTALL, TARGET_ID, MACHINE_ID, DISPATCH_ID, ERROR, now); // verifications - verifyNoInteractions(metrics, events); + verifyNoInteractions(metrics); } @Test @@ -143,6 +125,6 @@ void cancel_rowStillUnackedSameDispatch_rowCancelledWithTtlExpiry() { // verifications verify(repository).markCancelled(DELIVERY_ID, DeliveryStatus.UNACKED, dispatchedAt, now, expiresAt); - verifyNoInteractions(metrics, events); + verifyNoInteractions(metrics); } } diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java index 6aa72600dc..e7189e97d3 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryTrackerTest.java @@ -4,9 +4,7 @@ import com.openframe.data.document.delivery.DeliveryType; import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryTestPolicies; -import com.openframe.delivery.event.DeliveryAckedEvent; import com.openframe.delivery.spec.DeliveryRef; -import com.openframe.delivery.spec.TestSeed; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -14,7 +12,6 @@ import org.mockito.Captor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import org.springframework.context.ApplicationEventPublisher; import java.time.Instant; @@ -24,7 +21,6 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) @@ -39,17 +35,15 @@ class DeliveryTrackerTest { @Mock private MachineDeliveryRepository repository; @Mock private DeliveryCloser closer; - @Mock private ApplicationEventPublisher events; @Captor private ArgumentCaptor atCaptor; @Captor private ArgumentCaptor untilCaptor; - @Captor private ArgumentCaptor ackedCaptor; private DeliveryTracker tracker; @BeforeEach void setUp() { - tracker = new DeliveryTracker(repository, DeliveryTestPolicies.properties(), closer, events); + tracker = new DeliveryTracker(repository, DeliveryTestPolicies.properties(), closer); } @Test @@ -65,48 +59,6 @@ void acknowledge_ref_unackedRowMarkedAckedWithResultDeadline() { assertThat(untilCaptor.getValue()).isEqualTo(ackedAt.plusSeconds(RESULT_TIMEOUT)); } - @Test - void acknowledge_rowJustAcked_eventPublishedWithTheKey() { - // setup - when(repository.markAcked(eq(DELIVERY_ID), eq(DISPATCH_ID), eq(DeliveryStatus.UNACKED), any(Instant.class), any(Instant.class))).thenReturn(true); - - // execution - tracker.acknowledge(REF, MACHINE_ID); - - // verifications - verify(events).publishEvent(ackedCaptor.capture()); - DeliveryAckedEvent event = ackedCaptor.getValue(); - assertThat(event.getType()).isEqualTo(DeliveryType.TOOL_INSTALLATION); - assertThat(event.getTargetId()).isEqualTo(TARGET_ID); - assertThat(event.getMachineId()).isEqualTo(MACHINE_ID); - assertThat(event.getDispatchId()).isEqualTo(DISPATCH_ID); - } - - @Test - void acknowledge_staleDispatch_noEvent() { - // setup - when(repository.markAcked(eq(DELIVERY_ID), eq(DISPATCH_ID), eq(DeliveryStatus.UNACKED), any(Instant.class), any(Instant.class))).thenReturn(false); - - // execution - tracker.acknowledge(REF, MACHINE_ID); - - // verifications - verifyNoInteractions(events); - } - - @Test - void done_seed_completableRowOfTheSeedKeyMarkedDone() { - // setup - when(repository.markDone(eq(DELIVERY_ID), eq(DeliveryStatus.COMPLETABLE), atCaptor.capture(), untilCaptor.capture())).thenReturn(true); - - // execution - tracker.done(new TestSeed(MACHINE_ID)); - - // verifications - Instant finishedAt = atCaptor.getValue(); - assertThat(untilCaptor.getValue()).isEqualTo(finishedAt.plusSeconds(TTL)); - } - @Test void done_ref_openOrFailedRowMarkedDoneWithTtlExpiry() { // setup From 6f48f6b5360c80b944c324f3e6594a0ee794052d Mon Sep 17 00:00:00 2001 From: semen-flamingo Date: Mon, 5 Oct 2026 16:14:30 +0200 Subject: [PATCH 8/9] refactor(delivery): the spec says which machine statuses still take its command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Kirill: an uninstall is the only command a PENDING_DELETION machine should receive. DeliverySpec gets a default getDeliverableStatuses() — everything but DELETED, ARCHIVED, DECOMMISSIONED and PENDING_DELETION — and the uninstall spec adds PENDING_DELETION back. The sweep cancels a row whose machine is outside its type's set instead of using the hard-coded gone list, so an install for a machine already leaving is cancelled on the first tick rather than failing after the window. Presence of PENDING_DELETION machines comes from telemetryStatus (#2536); the lastSeen workaround in MachineStatusService is gone. Co-Authored-By: Claude Fable 5.1 --- .../client/service/MachineStatusService.java | 6 -- .../service/MachineStatusServiceTest.java | 34 ++--------- .../repository/device/MachineRepository.java | 2 - .../delivery/ClientUninstallDeliverySpec.java | 12 ++++ .../ClientUninstallDeliverySpecTest.java | 15 +++++ .../delivery/config/DeliveryProperties.java | 2 - .../openframe/delivery/spec/DeliverySpec.java | 13 ++++ .../delivery/sweep/DeliverySweepService.java | 19 +++--- .../delivery/sweep/MachineOnlineStatus.java | 32 +++------- .../sweep/DeliverySweepServiceTest.java | 60 +++++++++++++++---- .../sweep/MachineOnlineStatusTest.java | 45 +++++--------- 11 files changed, 128 insertions(+), 112 deletions(-) diff --git a/openframe-client-core/src/main/java/com/openframe/client/service/MachineStatusService.java b/openframe-client-core/src/main/java/com/openframe/client/service/MachineStatusService.java index 81be81eb7b..a433cb8c2e 100644 --- a/openframe-client-core/src/main/java/com/openframe/client/service/MachineStatusService.java +++ b/openframe-client-core/src/main/java/com/openframe/client/service/MachineStatusService.java @@ -16,7 +16,6 @@ import static com.openframe.data.document.device.DeviceStatus.OFFLINE; import static com.openframe.data.document.device.DeviceStatus.ONLINE; import static com.openframe.data.document.device.DeviceStatus.PENDING; -import static com.openframe.data.document.device.DeviceStatus.PENDING_DELETION; @Service @RequiredArgsConstructor @@ -45,11 +44,6 @@ private void update(String machineId, DeviceStatus newStatus, Instant eventTimes .orElseThrow(() -> new MachineNotFoundException(machineId)); if (isDeletionInProgress(machine)) { - // the status is frozen until the agent is gone, but a fresh lastSeen still tells the delivery sweep the machine is reachable - if (machine.getStatus() == PENDING_DELETION && isEventNewer(eventTimestamp, machine.getLastSeen())) { - touchLastSeen(machine, eventTimestamp); - return; - } log.debug("Ignoring {} event for machineId={} in status {}", newStatus, machineId, machine.getStatus()); return; } diff --git a/openframe-client-core/src/test/java/com/openframe/client/service/MachineStatusServiceTest.java b/openframe-client-core/src/test/java/com/openframe/client/service/MachineStatusServiceTest.java index 9f64075e4f..8b1d92897a 100644 --- a/openframe-client-core/src/test/java/com/openframe/client/service/MachineStatusServiceTest.java +++ b/openframe-client-core/src/test/java/com/openframe/client/service/MachineStatusServiceTest.java @@ -215,37 +215,13 @@ void unknownMachine_throwsAndWritesNothing() { } @Test - void processHeartbeat_machinePendingDeletion_lastSeenRefreshedStatusKept() { - // setup - Machine machine = new Machine(); - machine.setMachineId(MACHINE); - machine.setStatus(DeviceStatus.PENDING_DELETION); - machine.setLastSeen(SEEN); - when(machineRepository.findByMachineId(MACHINE)).thenReturn(Optional.of(machine)); - - // execution - service.processHeartbeat(MACHINE, LATER); - - // verifications - verify(machineRepository).updateLastSeen(MACHINE, LATER); - verify(machineRepository, never()).save(any()); - assertThat(machine.getStatus()).isEqualTo(DeviceStatus.PENDING_DELETION); - } + @DisplayName("T11: a device being deleted is left alone — neither save() nor updateLastSeen") + void pendingDeletion_touchesNothing() { + machineIs(DeviceStatus.PENDING_DELETION); - @Test - void processHeartbeat_machineDeleted_ignored() { - // setup - Machine machine = new Machine(); - machine.setMachineId(MACHINE); - machine.setStatus(DeviceStatus.DELETED); - machine.setLastSeen(SEEN); - when(machineRepository.findByMachineId(MACHINE)).thenReturn(Optional.of(machine)); - - // execution service.processHeartbeat(MACHINE, LATER); - // verifications - verify(machineRepository, never()).updateLastSeen(anyString(), any()); - verify(machineRepository, never()).save(any()); + verify(machineRepository, never()).save(any(Machine.class)); + verify(machineRepository, never()).updateLastSeen(anyString(), any(Instant.class)); } } diff --git a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/device/MachineRepository.java b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/device/MachineRepository.java index dcf730a46e..a9682dbfc1 100644 --- a/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/device/MachineRepository.java +++ b/openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/device/MachineRepository.java @@ -35,8 +35,6 @@ public interface MachineRepository extends MongoRepository, Cus List findByMachineIdInAndStatus(Collection machineIds, DeviceStatus status); - List findByMachineIdInAndStatusAndLastSeenAfter(Collection machineIds, DeviceStatus status, Instant lastSeenAfter); - List findByMachineIdInAndStatusIn(Collection machineIds, Collection statuses); List findByStatusIn(Collection statuses); diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java index f09f40a30a..7fa31f50b2 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java @@ -1,6 +1,7 @@ package com.openframe.data.nats.delivery; import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.data.document.device.DeviceStatus; import com.openframe.data.nats.model.ClientUninstallMessage; import com.openframe.delivery.spec.DeliveryRequest; import com.openframe.delivery.spec.DeliverySpec; @@ -8,6 +9,9 @@ import org.springframework.stereotype.Component; import java.time.Instant; +import java.util.Collections; +import java.util.EnumSet; +import java.util.Set; import static java.lang.String.format; @@ -16,6 +20,9 @@ public class ClientUninstallDeliverySpec implements DeliverySpec { private static final String SUBJECT_TEMPLATE = "machine.%s.client-uninstall"; + // the uninstall is what a PENDING_DELETION machine is waiting for + private static final Set LEAVING_TOO = Collections.unmodifiableSet(EnumSet.complementOf(EnumSet.of( + DeviceStatus.DELETED, DeviceStatus.ARCHIVED, DeviceStatus.DECOMMISSIONED))); @Override public DeliveryType getType() { @@ -43,4 +50,9 @@ public DeliveryRequest request(ClientUninstallDeliverySe public String subject(String machineId) { return format(SUBJECT_TEMPLATE, machineId); } + + @Override + public Set getDeliverableStatuses() { + return LEAVING_TOO; + } } diff --git a/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java index 43f4b89ea8..00f5e5f675 100644 --- a/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java +++ b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java @@ -1,10 +1,14 @@ package com.openframe.data.nats.delivery; import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.data.document.device.DeviceStatus; import com.openframe.data.nats.model.ClientUninstallMessage; import com.openframe.delivery.spec.DeliveryRequest; +import com.openframe.delivery.spec.DeliverySpec; import org.junit.jupiter.api.Test; +import java.util.Set; + import static org.assertj.core.api.Assertions.assertThat; class ClientUninstallDeliverySpecTest { @@ -29,6 +33,17 @@ void request_seed_messageStampedAndTargetIsTheClient() { assertThat(request.getPayload().getDelivery()).isNull(); } + @Test + void getDeliverableStatuses_machineMarkedForDeletion_stillReceivesTheUninstall() { + // execution + Set statuses = spec.getDeliverableStatuses(); + + // verifications + assertThat(statuses).contains(DeviceStatus.PENDING_DELETION, DeviceStatus.ONLINE, DeviceStatus.OFFLINE); + assertThat(statuses).doesNotContain(DeviceStatus.DELETED, DeviceStatus.ARCHIVED, DeviceStatus.DECOMMISSIONED); + assertThat(DeliverySpec.IN_SERVICE).doesNotContain(DeviceStatus.PENDING_DELETION); + } + @Test void subject_machineId_machineClientUninstallSubject() { // execution diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/config/DeliveryProperties.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/config/DeliveryProperties.java index 07334f4385..7d3482cc1b 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/config/DeliveryProperties.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/config/DeliveryProperties.java @@ -60,8 +60,6 @@ public static class Sweep { @NotNull @Positive private Integer batchSize; - // a PENDING_DELETION machine keeps its status until the agent is gone; it counts as online while its heartbeat is this fresh - private long onlineThresholdSeconds = 130; } @Getter diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java index 3b6fff80b1..78baea55a6 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java @@ -1,6 +1,11 @@ package com.openframe.delivery.spec; import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.data.document.device.DeviceStatus; + +import java.util.Collections; +import java.util.EnumSet; +import java.util.Set; public interface DeliverySpec { @@ -11,4 +16,12 @@ public interface DeliverySpec DeliveryRequest

request(S seed); String subject(String machineId); + + // a machine that is gone or on its way out gets no new commands; a type that must reach such a machine overrides this + Set IN_SERVICE = Collections.unmodifiableSet(EnumSet.complementOf(EnumSet.of( + DeviceStatus.DELETED, DeviceStatus.ARCHIVED, DeviceStatus.DECOMMISSIONED, DeviceStatus.PENDING_DELETION))); + + default Set getDeliverableStatuses() { + return IN_SERVICE; + } } diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliverySweepService.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliverySweepService.java index 33faadec19..b2aca42223 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliverySweepService.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliverySweepService.java @@ -7,6 +7,7 @@ import com.openframe.data.document.delivery.DeliveryStatus; import com.openframe.data.document.delivery.DeliveryType; import com.openframe.data.document.delivery.MachineDelivery; +import com.openframe.data.document.device.DeviceStatus; import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryProperties; import com.openframe.delivery.track.DeliveryCloser; @@ -25,6 +26,7 @@ import java.time.Instant; import java.util.List; +import java.util.Map; import java.util.Set; import static java.util.stream.Collectors.toSet; @@ -52,14 +54,14 @@ public void retryPending() { return; } Set machineIds = due.stream().map(MachineDelivery::getMachineId).collect(toSet()); - Set gone = machineOnlineStatus.gone(machineIds); + Map statuses = machineOnlineStatus.statuses(machineIds); Set online = machineOnlineStatus.online(machineIds); - due.forEach(delivery -> retryOne(delivery, gone, online, now)); + due.forEach(delivery -> retryOne(delivery, statuses, online, now)); } - private void retryOne(MachineDelivery delivery, Set gone, Set online, Instant now) { + private void retryOne(MachineDelivery delivery, Map statuses, Set online, Instant now) { try { - retryOrClose(delivery, gone, online, now); + retryOrClose(delivery, statuses, online, now); } catch (Exception e) { metrics.recordRowError(); countErrorAndBackOff(delivery, now); @@ -67,13 +69,14 @@ private void retryOne(MachineDelivery delivery, Set gone, Set on } } - private void retryOrClose(MachineDelivery delivery, Set gone, Set online, Instant now) { + private void retryOrClose(MachineDelivery delivery, Map statuses, Set online, Instant now) { String machineId = delivery.getMachineId(); - if (gone.contains(machineId)) { - closer.cancel(delivery, DeliveryStatus.UNACKED, "machine gone", now); + DeliveryType type = delivery.getType(); + DeviceStatus status = statuses.get(machineId); + if (status != null && !registry.require(type).getDeliverableStatuses().contains(status)) { + closer.cancel(delivery, DeliveryStatus.UNACKED, "machine in status " + status + " takes no " + type, now); return; } - DeliveryType type = delivery.getType(); Policy policy = properties.resolve(type); if (!online.contains(machineId)) { parkSkipOrFailOffline(delivery, policy, now); diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/MachineOnlineStatus.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/MachineOnlineStatus.java index 743fb981f7..7c1429526c 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/MachineOnlineStatus.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/MachineOnlineStatus.java @@ -3,17 +3,15 @@ import com.openframe.data.document.device.DeviceStatus; import com.openframe.data.document.device.Machine; import com.openframe.data.repository.device.MachineRepository; -import com.openframe.delivery.config.DeliveryProperties; import lombok.RequiredArgsConstructor; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.stereotype.Component; -import java.time.Instant; -import java.util.EnumSet; -import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; +import static java.util.stream.Collectors.toMap; import static java.util.stream.Collectors.toSet; @Component @@ -21,30 +19,18 @@ @ConditionalOnProperty(name = "openframe.delivery.sweep.enabled", havingValue = "true") public class MachineOnlineStatus { - private static final Set GONE = EnumSet.of( - DeviceStatus.DELETED, DeviceStatus.ARCHIVED, DeviceStatus.DECOMMISSIONED); - private final MachineRepository machineRepository; - private final DeliveryProperties properties; - // PENDING_DELETION freezes the status, so for those machines presence is read from the heartbeat instead public Set online(Set machineIds) { List online = machineRepository.findByMachineIdInAndStatus(machineIds, DeviceStatus.ONLINE); - long thresholdSeconds = properties.getSweep().getOnlineThresholdSeconds(); - Instant lastSeenAfter = Instant.now().minusSeconds(thresholdSeconds); - List leaving = machineRepository.findByMachineIdInAndStatusAndLastSeenAfter( - machineIds, DeviceStatus.PENDING_DELETION, lastSeenAfter); - Set ids = new HashSet<>(ids(online)); - ids.addAll(ids(leaving)); - return ids; - } - - public Set gone(Set machineIds) { - List gone = machineRepository.findByMachineIdInAndStatusIn(machineIds, GONE); - return ids(gone); + return online.stream().map(Machine::getMachineId).collect(toSet()); } - private static Set ids(List machines) { - return machines.stream().map(Machine::getMachineId).collect(toSet()); + // whether a machine may still receive a command is the spec's call; a machine the repository does not know stays absent + public Map statuses(Set machineIds) { + List machines = machineRepository.findByMachineIdIn(machineIds); + return machines.stream() + .filter(machine -> machine.getStatus() != null) + .collect(toMap(Machine::getMachineId, Machine::getStatus, (first, second) -> first)); } } diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliverySweepServiceTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliverySweepServiceTest.java index 80a1e4dc52..e540966cc9 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliverySweepServiceTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliverySweepServiceTest.java @@ -6,6 +6,7 @@ import com.openframe.data.document.delivery.DeliveryStatus; import com.openframe.data.document.delivery.DeliveryType; import com.openframe.data.document.delivery.MachineDelivery; +import com.openframe.data.document.device.DeviceStatus; import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryProperties; import com.openframe.delivery.dispatch.DeliveryPublisher; @@ -26,6 +27,8 @@ import org.mockito.junit.jupiter.MockitoExtension; import java.time.Instant; +import java.util.EnumSet; +import java.util.Map; import java.util.List; import java.util.Set; @@ -41,6 +44,7 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -217,7 +221,7 @@ void retryPending_onlineAttemptsExhausted_failedExhaustedOnlyIfStillUnacked() { // verifications verify(closer).fail(eq(delivery), eq(DeliveryFailure.EXHAUSTED), eq(DeliveryStatus.UNACKED), any(Instant.class)); - verifyNoInteractions(registry, metrics); + verifyNoInteractions(metrics); } @Test @@ -236,7 +240,7 @@ void retryPending_offlineFarFromWindowEnd_postponedToNextSweep() { assertThat(dueAtCaptor.getValue()) .isAfterOrEqualTo(nextSweep) .isBefore(nextSweep.plusSeconds(CLOCK_SLACK_SECONDS)); - verifyNoInteractions(registry, closer, metrics); + verifyNoInteractions(closer, metrics); } @Test @@ -283,7 +287,7 @@ void retryPending_offlineWithSkipBehavior_cancelledNotFailed() { // verifications verify(closer).cancel(eq(delivery), eq(DeliveryStatus.UNACKED), any(String.class), any(Instant.class)); verify(closer, never()).fail(eq(delivery), any(DeliveryFailure.class), eq(DeliveryStatus.UNACKED), any(Instant.class)); - verifyNoInteractions(registry, metrics); + verifyNoInteractions(metrics); } @Test @@ -297,7 +301,36 @@ void retryPending_machineDeleted_cancelled() { // verifications verify(closer).cancel(eq(delivery), eq(DeliveryStatus.UNACKED), any(String.class), any(Instant.class)); - verifyNoInteractions(registry, metrics); + verifyNoInteractions(metrics); + } + + @Test + void retryPending_machineLeavingAndTypeDoesNotReachIt_cancelled() { + // setup + stubDue(delivery); + stubMachine(DeviceStatus.PENDING_DELETION, false); + + // execution + service.retryPending(); + + // verifications + verify(closer).cancel(eq(delivery), eq(DeliveryStatus.UNACKED), any(String.class), any(Instant.class)); + verifyNoInteractions(metrics); + } + + @Test + void retryPending_machineLeavingAndTypeReachesIt_waitsForOnlineInstead() { + // setup + stubDue(delivery); + stubMachine(DeviceStatus.PENDING_DELETION, false); + when(spec.getDeliverableStatuses()).thenReturn(EnumSet.complementOf(EnumSet.of(DeviceStatus.DELETED))); + + // execution + service.retryPending(); + + // verifications + verify(closer, never()).cancel(eq(delivery), eq(DeliveryStatus.UNACKED), any(String.class), any(Instant.class)); + verify(repository).postpone(eq(delivery.getId()), eq(DeliveryStatus.UNACKED), eq(dispatchedAt), any(Instant.class)); } @Test @@ -306,8 +339,9 @@ void retryPending_oneRowCorrupt_corruptCountedAndPostponedOtherRepublished() { MachineDelivery corrupt = row(OTHER_MACHINE_ID, CORRUPT_JSON); stubDue(corrupt, delivery); Set both = Set.of(OTHER_MACHINE_ID, MACHINE_ID); - when(machineOnlineStatus.gone(both)).thenReturn(Set.of()); + when(machineOnlineStatus.statuses(both)).thenReturn(Map.of(OTHER_MACHINE_ID, DeviceStatus.ONLINE, MACHINE_ID, DeviceStatus.ONLINE)); when(machineOnlineStatus.online(both)).thenReturn(both); + when(spec.getDeliverableStatuses()).thenReturn(DeliverySpec.IN_SERVICE); stubSpec(); when(repository.markRepublished(eq(delivery.getId()), eq(DeliveryStatus.UNACKED), eq(dispatchedAt), eq(NO_ATTEMPTS), any(Instant.class))).thenReturn(true); @@ -355,18 +389,22 @@ private void stubDue(MachineDelivery... rows) { } private void stubMachineOnline() { - when(machineOnlineStatus.gone(Set.of(MACHINE_ID))).thenReturn(Set.of()); - when(machineOnlineStatus.online(Set.of(MACHINE_ID))).thenReturn(Set.of(MACHINE_ID)); + stubMachine(DeviceStatus.ONLINE, true); } private void stubMachineNotOnline() { - when(machineOnlineStatus.gone(Set.of(MACHINE_ID))).thenReturn(Set.of()); - when(machineOnlineStatus.online(Set.of(MACHINE_ID))).thenReturn(Set.of()); + stubMachine(DeviceStatus.OFFLINE, false); } private void stubMachineGone() { - when(machineOnlineStatus.gone(Set.of(MACHINE_ID))).thenReturn(Set.of(MACHINE_ID)); - when(machineOnlineStatus.online(Set.of(MACHINE_ID))).thenReturn(Set.of()); + stubMachine(DeviceStatus.DELETED, false); + } + + private void stubMachine(DeviceStatus status, boolean online) { + when(machineOnlineStatus.statuses(Set.of(MACHINE_ID))).thenReturn(Map.of(MACHINE_ID, status)); + when(machineOnlineStatus.online(Set.of(MACHINE_ID))).thenReturn(online ? Set.of(MACHINE_ID) : Set.of()); + lenient().doReturn(spec).when(registry).require(DeliveryType.TOOL_INSTALLATION); + lenient().when(spec.getDeliverableStatuses()).thenReturn(DeliverySpec.IN_SERVICE); } private void stubSpec() { diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/MachineOnlineStatusTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/MachineOnlineStatusTest.java index 53fd4a3765..90f73e3c4c 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/MachineOnlineStatusTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/MachineOnlineStatusTest.java @@ -3,21 +3,17 @@ import com.openframe.data.document.device.DeviceStatus; import com.openframe.data.document.device.Machine; import com.openframe.data.repository.device.MachineRepository; -import com.openframe.delivery.config.DeliveryTestPolicies; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.ArgumentCaptor; -import org.mockito.Captor; +import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import java.time.Instant; import java.util.List; +import java.util.Map; import java.util.Set; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) @@ -25,54 +21,41 @@ class MachineOnlineStatusTest { private static final String ONLINE_ID = "mach-1"; private static final String LEAVING_ID = "mach-2"; - private static final String GONE_ID = "mach-3"; - private static final Set ALL = Set.of(ONLINE_ID, LEAVING_ID, GONE_ID); + private static final String UNKNOWN_ID = "mach-3"; + private static final Set ALL = Set.of(ONLINE_ID, LEAVING_ID, UNKNOWN_ID); @Mock private MachineRepository machineRepository; - @Captor private ArgumentCaptor lastSeenAfterCaptor; - - private MachineOnlineStatus status; - - @BeforeEach - void setUp() { - status = new MachineOnlineStatus(machineRepository, DeliveryTestPolicies.properties()); - } + @InjectMocks private MachineOnlineStatus status; @Test - void online_onlineMachinesAndPendingDeletionWithFreshHeartbeat_bothCounted() { + void online_onlineMachines_returned() { // setup - Instant before = Instant.now(); - long threshold = DeliveryTestPolicies.properties().getSweep().getOnlineThresholdSeconds(); - when(machineRepository.findByMachineIdInAndStatus(ALL, DeviceStatus.ONLINE)).thenReturn(List.of(machine(ONLINE_ID))); - when(machineRepository.findByMachineIdInAndStatusAndLastSeenAfter(eq(ALL), eq(DeviceStatus.PENDING_DELETION), lastSeenAfterCaptor.capture())) - .thenReturn(List.of(machine(LEAVING_ID))); + when(machineRepository.findByMachineIdInAndStatus(ALL, DeviceStatus.ONLINE)).thenReturn(List.of(machine(ONLINE_ID, DeviceStatus.ONLINE))); // execution Set online = status.online(ALL); // verifications - assertThat(online).containsExactlyInAnyOrder(ONLINE_ID, LEAVING_ID); - assertThat(lastSeenAfterCaptor.getValue()) - .isBetween(before.minusSeconds(threshold + 5), before.minusSeconds(threshold).plusSeconds(5)); + assertThat(online).containsExactly(ONLINE_ID); } @Test - void gone_deletedArchivedDecommissioned_returned() { + void statuses_knownMachines_mappedUnknownAbsent() { // setup - when(machineRepository.findByMachineIdInAndStatusIn(eq(ALL), eq(Set.of(DeviceStatus.DELETED, DeviceStatus.ARCHIVED, DeviceStatus.DECOMMISSIONED)))) - .thenReturn(List.of(machine(GONE_ID))); + when(machineRepository.findByMachineIdIn(ALL)).thenReturn(List.of(machine(ONLINE_ID, DeviceStatus.ONLINE), machine(LEAVING_ID, DeviceStatus.PENDING_DELETION))); // execution - Set gone = status.gone(ALL); + Map statuses = status.statuses(ALL); // verifications - assertThat(gone).containsExactly(GONE_ID); + assertThat(statuses).containsOnly(Map.entry(ONLINE_ID, DeviceStatus.ONLINE), Map.entry(LEAVING_ID, DeviceStatus.PENDING_DELETION)); } - private static Machine machine(String machineId) { + private static Machine machine(String machineId, DeviceStatus deviceStatus) { Machine machine = new Machine(); machine.setMachineId(machineId); + machine.setStatus(deviceStatus); return machine; } } From f831ca0e25403b16b0dc06e05304da1f64578042 Mon Sep 17 00:00:00 2001 From: semen-flamingo Date: Mon, 5 Oct 2026 17:58:09 +0200 Subject: [PATCH 9/9] style(delivery): deliverable statuses as plain explicit sets No constants, no unmodifiable wrappers: the default is ONLINE, OFFLINE and PENDING (a machine that has not connected yet is still waiting for its registration installs), the uninstall adds PENDING_DELETION. Co-Authored-By: Claude Fable 5.1 --- .../data/nats/delivery/ClientUninstallDeliverySpec.java | 7 ++----- .../nats/delivery/ClientUninstallDeliverySpecTest.java | 2 -- .../java/com/openframe/delivery/spec/DeliverySpec.java | 8 ++------ .../delivery/sweep/DeliverySweepServiceTest.java | 4 ++-- 4 files changed, 6 insertions(+), 15 deletions(-) diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java index 7fa31f50b2..d33fe9bcf6 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpec.java @@ -9,7 +9,6 @@ import org.springframework.stereotype.Component; import java.time.Instant; -import java.util.Collections; import java.util.EnumSet; import java.util.Set; @@ -20,9 +19,6 @@ public class ClientUninstallDeliverySpec implements DeliverySpec { private static final String SUBJECT_TEMPLATE = "machine.%s.client-uninstall"; - // the uninstall is what a PENDING_DELETION machine is waiting for - private static final Set LEAVING_TOO = Collections.unmodifiableSet(EnumSet.complementOf(EnumSet.of( - DeviceStatus.DELETED, DeviceStatus.ARCHIVED, DeviceStatus.DECOMMISSIONED))); @Override public DeliveryType getType() { @@ -51,8 +47,9 @@ public String subject(String machineId) { return format(SUBJECT_TEMPLATE, machineId); } + // the uninstall is the one command a machine marked for deletion is still waiting for @Override public Set getDeliverableStatuses() { - return LEAVING_TOO; + return EnumSet.of(DeviceStatus.ONLINE, DeviceStatus.OFFLINE, DeviceStatus.PENDING, DeviceStatus.PENDING_DELETION); } } diff --git a/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java index 00f5e5f675..d9acdcc035 100644 --- a/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java +++ b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ClientUninstallDeliverySpecTest.java @@ -4,7 +4,6 @@ import com.openframe.data.document.device.DeviceStatus; import com.openframe.data.nats.model.ClientUninstallMessage; import com.openframe.delivery.spec.DeliveryRequest; -import com.openframe.delivery.spec.DeliverySpec; import org.junit.jupiter.api.Test; import java.util.Set; @@ -41,7 +40,6 @@ void getDeliverableStatuses_machineMarkedForDeletion_stillReceivesTheUninstall() // verifications assertThat(statuses).contains(DeviceStatus.PENDING_DELETION, DeviceStatus.ONLINE, DeviceStatus.OFFLINE); assertThat(statuses).doesNotContain(DeviceStatus.DELETED, DeviceStatus.ARCHIVED, DeviceStatus.DECOMMISSIONED); - assertThat(DeliverySpec.IN_SERVICE).doesNotContain(DeviceStatus.PENDING_DELETION); } @Test diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java index 78baea55a6..6931869b11 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpec.java @@ -3,7 +3,6 @@ import com.openframe.data.document.delivery.DeliveryType; import com.openframe.data.document.device.DeviceStatus; -import java.util.Collections; import java.util.EnumSet; import java.util.Set; @@ -17,11 +16,8 @@ public interface DeliverySpec String subject(String machineId); - // a machine that is gone or on its way out gets no new commands; a type that must reach such a machine overrides this - Set IN_SERVICE = Collections.unmodifiableSet(EnumSet.complementOf(EnumSet.of( - DeviceStatus.DELETED, DeviceStatus.ARCHIVED, DeviceStatus.DECOMMISSIONED, DeviceStatus.PENDING_DELETION))); - + // a machine that never connected is still waiting for its first commands; one that is gone or leaving gets none default Set getDeliverableStatuses() { - return IN_SERVICE; + return EnumSet.of(DeviceStatus.ONLINE, DeviceStatus.OFFLINE, DeviceStatus.PENDING); } } diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliverySweepServiceTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliverySweepServiceTest.java index e540966cc9..4a95df07fc 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliverySweepServiceTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliverySweepServiceTest.java @@ -341,7 +341,7 @@ void retryPending_oneRowCorrupt_corruptCountedAndPostponedOtherRepublished() { Set both = Set.of(OTHER_MACHINE_ID, MACHINE_ID); when(machineOnlineStatus.statuses(both)).thenReturn(Map.of(OTHER_MACHINE_ID, DeviceStatus.ONLINE, MACHINE_ID, DeviceStatus.ONLINE)); when(machineOnlineStatus.online(both)).thenReturn(both); - when(spec.getDeliverableStatuses()).thenReturn(DeliverySpec.IN_SERVICE); + when(spec.getDeliverableStatuses()).thenReturn(EnumSet.of(DeviceStatus.ONLINE, DeviceStatus.OFFLINE, DeviceStatus.PENDING)); stubSpec(); when(repository.markRepublished(eq(delivery.getId()), eq(DeliveryStatus.UNACKED), eq(dispatchedAt), eq(NO_ATTEMPTS), any(Instant.class))).thenReturn(true); @@ -404,7 +404,7 @@ private void stubMachine(DeviceStatus status, boolean online) { when(machineOnlineStatus.statuses(Set.of(MACHINE_ID))).thenReturn(Map.of(MACHINE_ID, status)); when(machineOnlineStatus.online(Set.of(MACHINE_ID))).thenReturn(online ? Set.of(MACHINE_ID) : Set.of()); lenient().doReturn(spec).when(registry).require(DeliveryType.TOOL_INSTALLATION); - lenient().when(spec.getDeliverableStatuses()).thenReturn(DeliverySpec.IN_SERVICE); + lenient().when(spec.getDeliverableStatuses()).thenReturn(EnumSet.of(DeviceStatus.ONLINE, DeviceStatus.OFFLINE, DeviceStatus.PENDING)); } private void stubSpec() {