From 92a13458390a0e70baf4e06b7da7d53de012d2ee Mon Sep 17 00:00:00 2001 From: semen-flamingo Date: Wed, 16 Sep 2026 14:46:42 +0200 Subject: [PATCH 1/5] feat(delivery): TOOL_INSTALLATION on the engine behind a per-type flag and agent-version gate Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01BnYUpYgKvuMw6hB5VZSimY --- .../DeviceOnlineDeliveryWakeListener.java | 49 +++++++ .../ScriptExecutionAcknowledgeListener.java | 22 ++- .../client/service/InstalledAgentService.java | 4 + .../DeviceOnlineDeliveryWakeListenerTest.java | 88 +++++++++++ ...criptExecutionAcknowledgeListenerTest.java | 115 +++++++++++++++ .../service/InstalledAgentServiceTest.java | 103 +++++++++++++ .../document/delivery/MachineDelivery.java | 1 + .../CustomMachineDeliveryRepository.java | 2 +- .../CustomMachineDeliveryRepositoryImpl.java | 6 +- ...stomMachineDeliveryRepositoryImplTest.java | 23 +++ openframe-data-nats/pom.xml | 4 + .../ToolInstallationDeliverySpec.java | 138 ++++++++++++++++++ .../nats/model/ToolInstallationMessage.java | 8 +- .../ScriptExecutionAcknowledgeMessage.java | 5 + .../ToolInstallationDeliverySpecTest.java | 103 +++++++++++++ .../delivery/config/DeliveryProperties.java | 20 ++- .../delivery/dispatch/AgentVersion.java | 42 ++++++ .../delivery/dispatch/DeliveryDispatcher.java | 14 +- .../delivery/dispatch/DeliveryGate.java | 39 +++++ .../delivery/dispatch/DeliveryRecorder.java | 8 +- .../delivery/spec/DeliveryPayload.java | 8 + .../delivery/spec/DeliveryRequest.java | 2 +- .../openframe/delivery/spec/DeliverySpec.java | 2 +- .../delivery/spec/DeliverySpecRegistry.java | 4 +- .../delivery/sweep/DeliveryCloser.java | 5 +- .../sweep/DeliverySweepScheduler.java | 2 +- .../delivery/sweep/DeliverySweepService.java | 11 +- .../sweep/DeliveryWatchdogService.java | 2 +- .../delivery/sweep/MachineOnlineStatus.java | 2 +- .../delivery/track/DeliveryTracker.java | 8 +- .../config/DeliveryPropertiesTest.java | 61 ++++++++ .../delivery/config/DeliveryTestPolicies.java | 1 - .../delivery/dispatch/AgentVersionTest.java | 32 ++++ .../dispatch/DeliveryDispatcherTest.java | 4 +- .../delivery/dispatch/DeliveryGateTest.java | 125 ++++++++++++++++ .../dispatch/DeliveryRecorderTest.java | 23 +-- .../spec/DeliverySpecRegistryTest.java | 6 +- .../openframe/delivery/spec/TestPayload.java | 4 +- .../delivery/track/DeliveryTrackerTest.java | 5 +- .../data/service/ToolInstallationService.java | 17 ++- 40 files changed, 1055 insertions(+), 63 deletions(-) create mode 100644 openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListener.java create mode 100644 openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListenerTest.java create mode 100644 openframe-client-core/src/test/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListenerTest.java create mode 100644 openframe-client-core/src/test/java/com/openframe/client/service/InstalledAgentServiceTest.java create mode 100644 openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java create mode 100644 openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpecTest.java create mode 100644 openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/AgentVersion.java create mode 100644 openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryGate.java create mode 100644 openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryPayload.java create mode 100644 openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/AgentVersionTest.java create mode 100644 openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryGateTest.java diff --git a/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListener.java b/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListener.java new file mode 100644 index 0000000000..0397a74644 --- /dev/null +++ b/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListener.java @@ -0,0 +1,49 @@ +package com.openframe.client.listener.delivery; + +import com.openframe.client.event.DeviceCameOnlineEvent; +import com.openframe.client.event.DeviceFirstConnectedEvent; +import com.openframe.data.document.device.Machine; +import com.openframe.delivery.track.DeliveryTracker; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.context.event.EventListener; +import org.springframework.stereotype.Component; + +import static com.openframe.data.document.device.DeviceStatus.ONLINE; + +@Slf4j +@Component +@RequiredArgsConstructor +public class DeviceOnlineDeliveryWakeListener { + + private final DeliveryTracker deliveryTracker; + + @EventListener + public void onDeviceFirstConnected(DeviceFirstConnectedEvent event) { + Machine machine = event.getMachine(); + if (isOnline(machine)) { + wake(machine); + } + } + + @EventListener + public void onDeviceCameOnline(DeviceCameOnlineEvent event) { + Machine machine = event.getMachine(); + wake(machine); + } + + // listeners of one event run in sequence on the publisher's thread: an exception here would also skip the + // SaaS DEVICE_REGISTERED listener; parked rows re-check on their own within max-retry-interval anyway + private void wake(Machine machine) { + String machineId = machine.getMachineId(); + try { + deliveryTracker.wake(machineId); + } catch (RuntimeException e) { + log.warn("Failed to wake parked deliveries, they re-check on their own: machineId={}", machineId, e); + } + } + + private static boolean isOnline(Machine machine) { + return machine.getStatus() == ONLINE; + } +} diff --git a/openframe-client-core/src/main/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListener.java b/openframe-client-core/src/main/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListener.java index 7a22c0d443..10fb187504 100644 --- a/openframe-client-core/src/main/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListener.java +++ b/openframe-client-core/src/main/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListener.java @@ -2,6 +2,8 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.openframe.client.service.rmm.ScriptExecutionAcknowledgeService; +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.delivery.track.DeliveryTracker; import com.openframe.data.nats.listener.AbstractJetStreamPushListener; import com.openframe.data.nats.rmm.model.ScriptExecutionAcknowledgeMessage; import io.nats.client.Connection; @@ -17,15 +19,18 @@ public class ScriptExecutionAcknowledgeListener extends AbstractJetStreamPushLis private final ObjectMapper objectMapper; private final ScriptExecutionAcknowledgeService acknowledgeService; + private final DeliveryTracker deliveryTracker; public ScriptExecutionAcknowledgeListener( Connection natsConnection, ObjectMapper objectMapper, - ScriptExecutionAcknowledgeService acknowledgeService + ScriptExecutionAcknowledgeService acknowledgeService, + DeliveryTracker deliveryTracker ) { super(natsConnection); this.objectMapper = objectMapper; this.acknowledgeService = acknowledgeService; + this.deliveryTracker = deliveryTracker; } @Override @@ -58,10 +63,23 @@ protected void handleMessage(Message message) { String payload = new String(message.getData(), StandardCharsets.UTF_8); try { ScriptExecutionAcknowledgeMessage ack = objectMapper.readValue(payload, ScriptExecutionAcknowledgeMessage.class); - acknowledgeService.acknowledge(ack); + if (isDeliveryAck(ack)) { + deliveryTracker.acknowledge(ack.getType(), ack.getTargetId(), ack.getMachineId(), ack.getDispatchId()); + } + if (isScriptAck(ack)) { + acknowledgeService.acknowledge(ack); + } message.ack(); } catch (Exception e) { log.error("Unexpected error processing execution ack: {}", payload, e); } } + + private static boolean isDeliveryAck(ScriptExecutionAcknowledgeMessage ack) { + return ack.getType() != null && ack.getTargetId() != null; + } + + private static boolean isScriptAck(ScriptExecutionAcknowledgeMessage ack) { + return ack.getType() == null || ack.getType() == DeliveryType.SCRIPT_SCHEDULE; + } } diff --git a/openframe-client-core/src/main/java/com/openframe/client/service/InstalledAgentService.java b/openframe-client-core/src/main/java/com/openframe/client/service/InstalledAgentService.java index 85d1511be4..e1cebf5c74 100644 --- a/openframe-client-core/src/main/java/com/openframe/client/service/InstalledAgentService.java +++ b/openframe-client-core/src/main/java/com/openframe/client/service/InstalledAgentService.java @@ -4,6 +4,8 @@ import com.openframe.client.exception.MachineNotFoundException; import com.openframe.data.document.installedagents.InstalledAgent; import com.openframe.data.document.tool.ConnectionStatus; +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.delivery.track.DeliveryTracker; import com.openframe.data.repository.device.MachineRepository; import com.openframe.data.repository.installedagents.InstalledAgentRepository; import lombok.RequiredArgsConstructor; @@ -21,6 +23,7 @@ public class InstalledAgentService { private final InstalledAgentRepository installedAgentRepository; private final MachineRepository machineRepository; + private final DeliveryTracker deliveryTracker; @Transactional public void addInstalledAgent(String machineId, String agentType, String version, boolean lastAttempt) { @@ -36,6 +39,7 @@ public void addInstalledAgent(String machineId, String agentType, String version installedAgent -> updateExistingInstalledAgent(installedAgent, version, machineId, agentType), () -> addNewInstalledAgent(machineId, agentType, version) ); + deliveryTracker.complete(DeliveryType.TOOL_INSTALLATION, agentType, machineId); } @Transactional diff --git a/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListenerTest.java b/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListenerTest.java new file mode 100644 index 0000000000..0c7272c0ff --- /dev/null +++ b/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListenerTest.java @@ -0,0 +1,88 @@ +package com.openframe.client.listener.delivery; + +import com.openframe.client.event.DeviceCameOnlineEvent; +import com.openframe.client.event.DeviceFirstConnectedEvent; +import com.openframe.data.document.device.DeviceStatus; +import com.openframe.data.document.device.Machine; +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.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; + +@ExtendWith(MockitoExtension.class) +class DeviceOnlineDeliveryWakeListenerTest { + + private static final String MACHINE_ID = "machine-1"; + + @Mock + private DeliveryTracker deliveryTracker; + + @InjectMocks + private DeviceOnlineDeliveryWakeListener listener; + + private Machine machine; + + @BeforeEach + void setUp() { + machine = new Machine(); + machine.setMachineId(MACHINE_ID); + } + + @Test + void onDeviceCameOnline_offlineToOnline_machineWoken() { + // setup + machine.setStatus(DeviceStatus.ONLINE); + DeviceCameOnlineEvent event = new DeviceCameOnlineEvent(this, machine); + + // execution + listener.onDeviceCameOnline(event); + + // verifications + verify(deliveryTracker).wake(MACHINE_ID); + } + + @Test + void onDeviceFirstConnected_pendingToOnline_machineWoken() { + // setup + machine.setStatus(DeviceStatus.ONLINE); + DeviceFirstConnectedEvent event = new DeviceFirstConnectedEvent(this, machine); + + // execution + listener.onDeviceFirstConnected(event); + + // verifications + verify(deliveryTracker).wake(MACHINE_ID); + } + + @Test + void onDeviceCameOnline_wakeFails_swallowedSoOtherListenersStillRun() { + // setup + machine.setStatus(DeviceStatus.ONLINE); + DeviceCameOnlineEvent event = new DeviceCameOnlineEvent(this, machine); + doThrow(new IllegalStateException("mongo down")).when(deliveryTracker).wake(MACHINE_ID); + + // execution + verifications + assertThatCode(() -> listener.onDeviceCameOnline(event)).doesNotThrowAnyException(); + } + + @Test + void onDeviceFirstConnected_pendingToOffline_nothingWoken() { + // setup + machine.setStatus(DeviceStatus.OFFLINE); + DeviceFirstConnectedEvent event = new DeviceFirstConnectedEvent(this, machine); + + // execution + listener.onDeviceFirstConnected(event); + + // verifications + verifyNoInteractions(deliveryTracker); + } +} diff --git a/openframe-client-core/src/test/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListenerTest.java b/openframe-client-core/src/test/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListenerTest.java new file mode 100644 index 0000000000..b30a7d4b3a --- /dev/null +++ b/openframe-client-core/src/test/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListenerTest.java @@ -0,0 +1,115 @@ +package com.openframe.client.listener.rmm; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.openframe.client.service.rmm.ScriptExecutionAcknowledgeService; +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.delivery.track.DeliveryTracker; +import com.openframe.data.nats.rmm.model.ScriptExecutionAcknowledgeMessage; +import io.nats.client.Connection; +import io.nats.client.Message; +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 static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; +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 ScriptExecutionAcknowledgeListenerTest { + + private static final String MACHINE_ID = "mach-42"; + private static final String EXECUTION_ID = "exec-1"; + private static final String TOOL_AGENT_ID = "tactical-agent"; + private static final String DISPATCH_ID = "d-1"; + private static final String LEGACY_SCRIPT_ACK = + "{\"executionId\":\"exec-1\",\"machineId\":\"mach-42\",\"scriptIds\":[\"s1\"]}"; + private static final String TOOL_INSTALLATION_ACK = + "{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"tactical-agent\",\"machineId\":\"mach-42\",\"dispatchId\":\"d-1\"}"; + private static final String SCRIPT_SCHEDULE_ACK = + "{\"type\":\"SCRIPT_SCHEDULE\",\"targetId\":\"exec-1\",\"executionId\":\"exec-1\",\"machineId\":\"mach-42\",\"scriptIds\":[\"s1\"]}"; + private static final String MALFORMED = "not json"; + + @Mock private Connection natsConnection; + @Mock private ScriptExecutionAcknowledgeService acknowledgeService; + @Mock private DeliveryTracker deliveryTracker; + @Mock private Message message; + + @Captor private ArgumentCaptor ackCaptor; + + private ScriptExecutionAcknowledgeListener listener; + + @BeforeEach + void setUp() { + listener = new ScriptExecutionAcknowledgeListener(natsConnection, new ObjectMapper(), acknowledgeService, deliveryTracker); + } + + @Test + void handleMessage_legacyScriptAck_scriptServiceOnly() { + // setup + stubPayload(LEGACY_SCRIPT_ACK); + + // execution + listener.handleMessage(message); + + // verifications + verify(acknowledgeService).acknowledge(ackCaptor.capture()); + assertThat(ackCaptor.getValue().getExecutionId()).isEqualTo(EXECUTION_ID); + verifyNoInteractions(deliveryTracker); + verify(message).ack(); + } + + @Test + void handleMessage_toolInstallationAck_trackerOnly() { + // setup + stubPayload(TOOL_INSTALLATION_ACK); + + // execution + listener.handleMessage(message); + + // verifications + verify(deliveryTracker).acknowledge(DeliveryType.TOOL_INSTALLATION, TOOL_AGENT_ID, MACHINE_ID, DISPATCH_ID); + verifyNoInteractions(acknowledgeService); + verify(message).ack(); + } + + @Test + void handleMessage_scriptScheduleAckWithType_trackerAndScriptService() { + // setup + stubPayload(SCRIPT_SCHEDULE_ACK); + + // execution + listener.handleMessage(message); + + // verifications + verify(deliveryTracker).acknowledge(DeliveryType.SCRIPT_SCHEDULE, EXECUTION_ID, MACHINE_ID, null); + verify(acknowledgeService).acknowledge(ackCaptor.capture()); + assertThat(ackCaptor.getValue().getExecutionId()).isEqualTo(EXECUTION_ID); + verify(message).ack(); + } + + @Test + void handleMessage_malformedPayload_leftUnacked() { + // setup + stubPayload(MALFORMED); + + // execution + listener.handleMessage(message); + + // verifications + verify(message, never()).ack(); + verifyNoInteractions(acknowledgeService); + verifyNoInteractions(deliveryTracker); + } + + private void stubPayload(String json) { + when(message.getData()).thenReturn(json.getBytes(UTF_8)); + } +} diff --git a/openframe-client-core/src/test/java/com/openframe/client/service/InstalledAgentServiceTest.java b/openframe-client-core/src/test/java/com/openframe/client/service/InstalledAgentServiceTest.java new file mode 100644 index 0000000000..98603f30f9 --- /dev/null +++ b/openframe-client-core/src/test/java/com/openframe/client/service/InstalledAgentServiceTest.java @@ -0,0 +1,103 @@ +package com.openframe.client.service; + +import com.openframe.client.exception.MachineNotFoundException; +import com.openframe.delivery.track.DeliveryTracker; +import com.openframe.data.document.device.Machine; +import com.openframe.data.document.installedagents.InstalledAgent; +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.data.document.tool.ConnectionStatus; +import com.openframe.data.repository.device.MachineRepository; +import com.openframe.data.repository.installedagents.InstalledAgentRepository; +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.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class InstalledAgentServiceTest { + + private static final String MACHINE_ID = "mach-42"; + private static final String AGENT_TYPE = "tactical-agent"; + private static final String OLD_VERSION = "1.0.0"; + private static final String VERSION = "1.2.3"; + + @Mock private InstalledAgentRepository installedAgentRepository; + @Mock private MachineRepository machineRepository; + @Mock private DeliveryTracker deliveryTracker; + + @Captor private ArgumentCaptor installedAgentCaptor; + + @InjectMocks private InstalledAgentService service; + + private Machine machine; + private InstalledAgent existing; + + @BeforeEach + void setUp() { + machine = new Machine(); + machine.setMachineId(MACHINE_ID); + existing = new InstalledAgent(); + existing.setMachineId(MACHINE_ID); + existing.setAgentType(AGENT_TYPE); + existing.setVersion(OLD_VERSION); + existing.setStatus(ConnectionStatus.DISCONNECTED); + } + + @Test + void addInstalledAgent_newAgent_savedAndDeliveryCompleted() { + // setup + when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); + when(installedAgentRepository.findByMachineIdAndAgentType(MACHINE_ID, AGENT_TYPE)).thenReturn(Optional.empty()); + + // execution + service.addInstalledAgent(MACHINE_ID, AGENT_TYPE, VERSION, false); + + // verifications + verify(installedAgentRepository).save(installedAgentCaptor.capture()); + assertThat(installedAgentCaptor.getValue().getVersion()).isEqualTo(VERSION); + assertThat(installedAgentCaptor.getValue().getStatus()).isEqualTo(ConnectionStatus.CONNECTED); + verify(deliveryTracker).complete(DeliveryType.TOOL_INSTALLATION, AGENT_TYPE, MACHINE_ID); + } + + @Test + void addInstalledAgent_existingAgent_versionUpdatedAndDeliveryCompleted() { + // setup + when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); + when(installedAgentRepository.findByMachineIdAndAgentType(MACHINE_ID, AGENT_TYPE)).thenReturn(Optional.of(existing)); + + // execution + service.addInstalledAgent(MACHINE_ID, AGENT_TYPE, VERSION, false); + + // verifications + assertThat(existing.getVersion()).isEqualTo(VERSION); + assertThat(existing.getStatus()).isEqualTo(ConnectionStatus.CONNECTED); + verify(installedAgentRepository).save(existing); + verify(deliveryTracker).complete(DeliveryType.TOOL_INSTALLATION, AGENT_TYPE, MACHINE_ID); + } + + @Test + void addInstalledAgent_unknownMachine_throwsAndDeliveryUntouched() { + // setup + when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.empty()); + + // execution + MachineNotFoundException ex = assertThrows(MachineNotFoundException.class, + () -> service.addInstalledAgent(MACHINE_ID, AGENT_TYPE, VERSION, false)); + + // verifications + assertThat(ex.getMessage()).contains(MACHINE_ID); + verifyNoInteractions(deliveryTracker); + } +} diff --git a/openframe-data-mongo-common/src/main/java/com/openframe/data/document/delivery/MachineDelivery.java b/openframe-data-mongo-common/src/main/java/com/openframe/data/document/delivery/MachineDelivery.java index cd334ce889..6db338f92d 100644 --- a/openframe-data-mongo-common/src/main/java/com/openframe/data/document/delivery/MachineDelivery.java +++ b/openframe-data-mongo-common/src/main/java/com/openframe/data/document/delivery/MachineDelivery.java @@ -32,6 +32,7 @@ public class MachineDelivery implements TenantScoped { private DeliveryStatus status; private int attempts; private int errors; + private String dispatchId; private String payloadJson; private Instant dispatchedAt; 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 2ee53b7173..86f9d99edb 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 @@ -22,7 +22,7 @@ public interface CustomMachineDeliveryRepository { boolean park(String id, Set from, Instant dispatchedAt, Instant dueAt); - boolean markAcked(String id, Set from, Instant ackedAt, Instant dueAt); + boolean markAcked(String id, String dispatchId, Set from, Instant ackedAt, Instant dueAt); boolean markDone(String id, Set from, 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 ee10d7ba62..8f7b495a5d 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 @@ -29,6 +29,7 @@ public class CustomMachineDeliveryRepositoryImpl extends TenantAwareRepositorySu private static final String FIELD_STATUS = "status"; private static final String FIELD_ATTEMPTS = "attempts"; private static final String FIELD_ERRORS = "errors"; + private static final String FIELD_DISPATCH_ID = "dispatchId"; private static final String FIELD_PAYLOAD_JSON = "payloadJson"; private static final String FIELD_DISPATCHED_AT = "dispatchedAt"; private static final String FIELD_DUE_AT = "dueAt"; @@ -99,13 +100,14 @@ public boolean park(String id, Set from, Instant dispatchedAt, I } @Override - public boolean markAcked(String id, Set from, Instant ackedAt, Instant dueAt) { + public boolean markAcked(String id, String dispatchId, Set from, Instant ackedAt, Instant dueAt) { + Criteria thisDispatch = stillIn(id, from).and(FIELD_DISPATCH_ID).is(dispatchId); Update update = new Update() .set(FIELD_STATUS, DeliveryStatus.ACKED) .set(FIELD_ACKED_AT, ackedAt) .set(FIELD_DUE_AT, dueAt) .set(FIELD_PARKED, false); - return updateOne(stillIn(id, from), update); + return updateOne(thisDispatch, update); } @Override 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 b4e9a7f28e..ddc51d1718 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 @@ -33,6 +33,7 @@ class CustomMachineDeliveryRepositoryImplTest { private static final String TENANT_ID = "tenant-1"; private static final int LIMIT = 500; private static final int ATTEMPTS = 1; + private static final String DISPATCH_ID = "d-1"; @Mock private TenantAwareMongoTemplate mongoTemplate; @Mock private MongoConverter converter; @@ -119,6 +120,28 @@ void markRepublished_rowMovedOn_false() { assertThat(republished).isFalse(); } + @Test + void markAcked_unackedRowOfThisDispatch_ackedAndTrue() { + // setup + UpdateResult oneRow = UpdateResult.acknowledged(1, 1L, null); + when(mongoTemplate.updateFirst(queryCaptor.capture(), updateCaptor.capture(), eq(MachineDelivery.class))).thenReturn(oneRow); + + // execution + boolean acked = repository.markAcked(ID, DISPATCH_ID, DeliveryStatus.UNACKED, now, now); + + // verifications + assertThat(acked).isTrue(); + assertThat(queryCaptor.getValue().getQueryObject().toString()) + .contains(ID) + .contains("PENDING") + .contains("dispatchId=" + DISPATCH_ID); + assertThat(updateCaptor.getValue().getUpdateObject().toString()) + .contains("ACKED") + .contains("ackedAt") + .contains("dueAt") + .contains("parked=false"); + } + @Test void postponeAfterError_pendingRow_errorCountedAndDueMoved() { // setup diff --git a/openframe-data-nats/pom.xml b/openframe-data-nats/pom.xml index d7ec7edd04..219bb25d05 100644 --- a/openframe-data-nats/pom.xml +++ b/openframe-data-nats/pom.xml @@ -28,6 +28,10 @@ com.openframe.oss openframe-data-mongo-sync + + com.openframe.oss + openframe-machine-delivery + org.springframework.boot 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 new file mode 100644 index 0000000000..2722dfaf3d --- /dev/null +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpec.java @@ -0,0 +1,138 @@ +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; +import com.openframe.data.document.toolagent.ToolAgentAssetSource; +import com.openframe.data.nats.mapper.DownloadConfigurationMapper; +import com.openframe.data.nats.mapper.LocalFilenameConfigurationMapper; +import com.openframe.data.nats.model.ToolInstallationMessage; +import com.openframe.data.nats.publisher.NatsMessagePublisher; +import com.openframe.delivery.spec.DeliveryRequest; +import com.openframe.delivery.spec.DeliverySeed; +import com.openframe.delivery.spec.DeliverySpec; +import lombok.AllArgsConstructor; +import lombok.Getter; +import lombok.RequiredArgsConstructor; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.stereotype.Component; + +import java.util.List; + +import static java.lang.String.format; +import static java.util.Objects.requireNonNullElse; + +@Component +@RequiredArgsConstructor +@ConditionalOnProperty("spring.cloud.stream.enabled") +public class ToolInstallationDeliverySpec implements DeliverySpec { + + private static final String SUBJECT_TEMPLATE = "machine.%s.tool-installation"; + + private final NatsMessagePublisher natsMessagePublisher; + private final DownloadConfigurationMapper downloadConfigurationMapper; + private final LocalFilenameConfigurationMapper localFilenameConfigurationMapper; + + @Getter + @AllArgsConstructor + public static class Seed implements DeliverySeed { + private final String machineId; + private final IntegratedToolAgent toolAgent; + private final IntegratedTool tool; + private final boolean reinstall; + + @Override + public DeliveryType type() { + return DeliveryType.TOOL_INSTALLATION; + } + } + + @Override + public DeliveryType getType() { + return DeliveryType.TOOL_INSTALLATION; + } + + @Override + public Class getPayloadClass() { + return ToolInstallationMessage.class; + } + + // targetId must equal the agentType the agent sends in installed-agent, or complete() never finds the row + @Override + public DeliveryRequest request(Seed 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()) + .payload(message) + .build(); + } + + @Override + public void publish(String machineId, ToolInstallationMessage payload) { + String subject = format(SUBJECT_TEMPLATE, machineId); + natsMessagePublisher.publish(subject, payload); + } + + @Override + public void onFailed(MachineDelivery delivery, DeliveryFailure failure) { + // intentionally empty: a failed install leaves nothing to compensate + } + + private ToolInstallationMessage buildMessage(IntegratedToolAgent toolAgent, IntegratedTool tool, boolean reinstall) { + String version = toolAgent.getVersion(); + ToolInstallationMessage message = new ToolInstallationMessage(); + message.setToolAgentId(toolAgent.getKey()); + message.setToolId(requireNonNullElse(toolAgent.getToolId(), "")); + message.setToolType(requireNonNullElse(tool.getToolType(), "")); + message.setVersion(version); + message.setSessionType(toolAgent.getSessionType()); + message.setDownloadConfigurations(downloadConfigurationMapper.map(toolAgent.getDownloadConfigurations(), version)); + message.setAssets(mapAssets(toolAgent.getAssets())); + message.setInstallationCommandArgs(toolAgent.getInstallationCommandArgs()); + message.setUninstallationCommandArgs(toolAgent.getUninstallationCommandArgs()); + message.setRunCommandArgs(toolAgent.getRunCommandArgs()); + message.setToolAgentIdCommandArgs(toolAgent.getAgentToolIdCommandArgs()); + message.setReinstall(reinstall); + return message; + } + + private List mapAssets(List assets) { + if (assets == null) { + return null; + } + return assets.stream() + .map(this::mapAsset) + .toList(); + } + + private ToolInstallationMessage.Asset mapAsset(ToolAgentAsset asset) { + String version = asset.getVersion(); + ToolInstallationMessage.Asset messageAsset = new ToolInstallationMessage.Asset(); + messageAsset.setId(asset.getId()); + messageAsset.setVersion(version); + messageAsset.setLocalFilenameConfiguration(localFilenameConfigurationMapper.map(asset.getLocalFilenameConfiguration())); + messageAsset.setDownloadConfigurations(downloadConfigurationMapper.map(asset.getDownloadConfigurations(), version)); + messageAsset.setSource(mapAssetSource(asset.getSource())); + messageAsset.setPath(asset.getPath()); + messageAsset.setExecutable(asset.isExecutable()); + return messageAsset; + } + + private static ToolInstallationMessage.AssetSource mapAssetSource(ToolAgentAssetSource source) { + if (source == null) { + return null; + } + return switch (source) { + case ARTIFACTORY -> ToolInstallationMessage.AssetSource.ARTIFACTORY; + case TOOL_API -> ToolInstallationMessage.AssetSource.TOOL_API; + case GITHUB -> ToolInstallationMessage.AssetSource.GITHUB; + }; + } +} diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/model/ToolInstallationMessage.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/model/ToolInstallationMessage.java index 8d60d93875..3fdcdbdf44 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/model/ToolInstallationMessage.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/model/ToolInstallationMessage.java @@ -1,5 +1,8 @@ package com.openframe.data.nats.model; +import com.fasterxml.jackson.annotation.JsonInclude; +import com.openframe.delivery.spec.DeliveryPayload; + import com.openframe.data.document.toolagent.SessionType; import lombok.Getter; import lombok.Setter; @@ -8,7 +11,10 @@ @Getter @Setter -public class ToolInstallationMessage { +public class ToolInstallationMessage implements DeliveryPayload { + + @JsonInclude(JsonInclude.Include.NON_NULL) + private String dispatchId; private String toolAgentId; private String toolId; diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/rmm/model/ScriptExecutionAcknowledgeMessage.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/rmm/model/ScriptExecutionAcknowledgeMessage.java index 9100972645..d446ac1a28 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/rmm/model/ScriptExecutionAcknowledgeMessage.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/rmm/model/ScriptExecutionAcknowledgeMessage.java @@ -1,5 +1,6 @@ package com.openframe.data.nats.rmm.model; +import com.openframe.data.document.delivery.DeliveryType; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import lombok.AllArgsConstructor; import lombok.Builder; @@ -19,4 +20,8 @@ public class ScriptExecutionAcknowledgeMessage { private String machineId; private String scheduleId; private List scriptIds; + + private DeliveryType type; + private String targetId; + private String dispatchId; } diff --git a/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpecTest.java b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpecTest.java new file mode 100644 index 0000000000..0f6c32a3cf --- /dev/null +++ b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpecTest.java @@ -0,0 +1,103 @@ +package com.openframe.data.nats.delivery; + +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.data.document.tool.IntegratedTool; +import com.openframe.data.document.toolagent.IntegratedToolAgent; +import com.openframe.data.nats.mapper.DownloadConfigurationMapper; +import com.openframe.data.nats.mapper.LocalFilenameConfigurationMapper; +import com.openframe.data.nats.model.ToolInstallationMessage; +import com.openframe.data.nats.publisher.NatsMessagePublisher; +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.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class ToolInstallationDeliverySpecTest { + + private static final String MACHINE_ID = "mach-42"; + private static final String TOOL_AGENT_KEY = "tactical-agent"; + private static final String TOOL_ID = "tactical"; + private static final String TOOL_TYPE = "TACTICAL"; + private static final String VERSION = "1.2.3"; + private static final List INSTALL_ARGS = List.of("--silent"); + + @Mock private NatsMessagePublisher natsMessagePublisher; + @Mock private DownloadConfigurationMapper downloadConfigurationMapper; + @Mock private LocalFilenameConfigurationMapper localFilenameConfigurationMapper; + + @InjectMocks private ToolInstallationDeliverySpec spec; + + private IntegratedToolAgent toolAgent; + private IntegratedTool tool; + + @BeforeEach + void setUp() { + toolAgent = new IntegratedToolAgent(); + toolAgent.setKey(TOOL_AGENT_KEY); + toolAgent.setToolId(TOOL_ID); + toolAgent.setVersion(VERSION); + toolAgent.setInstallationCommandArgs(INSTALL_ARGS); + tool = new IntegratedTool(); + tool.setToolType(TOOL_TYPE); + } + + @Test + void request_toolAgent_messageBuiltAndTargetIsAgentKey() { + // setup + when(downloadConfigurationMapper.map(null, VERSION)).thenReturn(List.of()); + ToolInstallationDeliverySpec.Seed seed = new ToolInstallationDeliverySpec.Seed(MACHINE_ID, toolAgent, tool, true); + + // execution + DeliveryRequest request = spec.request(seed); + + // verifications + assertThat(request.getType()).isEqualTo(DeliveryType.TOOL_INSTALLATION); + assertThat(request.getTargetId()).isEqualTo(TOOL_AGENT_KEY); + assertThat(request.getMachineId()).isEqualTo(MACHINE_ID); + ToolInstallationMessage message = request.getPayload(); + assertThat(message.getToolAgentId()).isEqualTo(TOOL_AGENT_KEY); + assertThat(message.getToolId()).isEqualTo(TOOL_ID); + assertThat(message.getToolType()).isEqualTo(TOOL_TYPE); + assertThat(message.getVersion()).isEqualTo(VERSION); + assertThat(message.getInstallationCommandArgs()).isEqualTo(INSTALL_ARGS); + assertThat(message.isReinstall()).isTrue(); + } + + @Test + void request_toolWithoutIdAndType_emptyStringsNotNulls() { + // setup + toolAgent.setToolId(null); + tool.setToolType(null); + when(downloadConfigurationMapper.map(null, VERSION)).thenReturn(List.of()); + ToolInstallationDeliverySpec.Seed seed = new ToolInstallationDeliverySpec.Seed(MACHINE_ID, toolAgent, tool, false); + + // execution + DeliveryRequest request = spec.request(seed); + + // verifications + assertThat(request.getPayload().getToolId()).isEmpty(); + assertThat(request.getPayload().getToolType()).isEmpty(); + } + + @Test + void publish_payload_sentToMachineSubject() { + // setup + ToolInstallationMessage message = new ToolInstallationMessage(); + + // execution + spec.publish(MACHINE_ID, message); + + // verifications + verify(natsMessagePublisher).publish("machine.mach-42.tool-installation", message); + } +} 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 bab1be0e42..812ccafbe1 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 @@ -13,7 +13,9 @@ import java.util.EnumMap; import java.util.Map; +import java.util.Optional; +import static java.lang.Boolean.FALSE; import static java.util.Objects.requireNonNullElse; @Getter @@ -23,9 +25,6 @@ @ConfigurationProperties(prefix = "openframe.delivery") public class DeliveryProperties { - @NotNull - private Boolean enabled; - @Valid @NotNull private Sweep sweep; @@ -37,8 +36,19 @@ public class DeliveryProperties { // deliberately not @Valid: a per-type entry lists only the fields it overrides private Map types = new EnumMap<>(DeliveryType.class); - public boolean isEnabled() { - return enabled; + // a type not listed here is off: every environment switches each type on explicitly + private Map enabled = new EnumMap<>(DeliveryType.class); + + // first agent version that acks the type; a type not listed here keeps every machine on the old path + private Map minAgentVersion = new EnumMap<>(DeliveryType.class); + + public boolean isEnabled(DeliveryType type) { + return enabled.getOrDefault(type, FALSE); + } + + public Optional minAgentVersion(DeliveryType type) { + String version = minAgentVersion.get(type); + return Optional.ofNullable(version); } public Policy resolve(DeliveryType type) { diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/AgentVersion.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/AgentVersion.java new file mode 100644 index 0000000000..8bf7b4105f --- /dev/null +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/AgentVersion.java @@ -0,0 +1,42 @@ +package com.openframe.delivery.dispatch; + +import lombok.experimental.UtilityClass; +import org.springframework.util.StringUtils; + +import java.util.Arrays; +import java.util.regex.Pattern; + +@UtilityClass +class AgentVersion { + + private final Pattern NON_DIGITS = Pattern.compile("\\D+"); + + boolean isAtLeast(String version, String minimum) { + long[] actual = numbers(version); + long[] required = numbers(minimum); + int length = Math.max(actual.length, required.length); + for (int i = 0; i < length; i++) { + long left = numberAt(actual, i); + long right = numberAt(required, i); + if (left != right) { + return left > right; + } + } + return true; + } + + private long[] numbers(String version) { + String[] tokens = NON_DIGITS.split(version); + return Arrays.stream(tokens) + .filter(StringUtils::hasText) + .mapToLong(Long::parseLong) + .toArray(); + } + + private long numberAt(long[] numbers, int index) { + if (index >= numbers.length) { + return 0; + } + return numbers[index]; + } +} 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 2b41424802..48629c4148 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 @@ -1,6 +1,7 @@ package com.openframe.delivery.dispatch; import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.delivery.spec.DeliveryPayload; import com.openframe.delivery.spec.DeliveryRequest; import com.openframe.delivery.spec.DeliverySeed; import com.openframe.delivery.spec.DeliverySpec; @@ -9,6 +10,8 @@ import lombok.extern.slf4j.Slf4j; import org.springframework.stereotype.Component; +import java.util.UUID; + @Slf4j @Component @RequiredArgsConstructor @@ -19,12 +22,15 @@ public class DeliveryDispatcher { public void dispatch(DeliverySeed seed) { DeliveryType type = seed.type(); - DeliverySpec spec = registry.require(type); - DeliveryRequest request = spec.request(seed); + DeliverySpec spec = registry.require(type); + DeliveryRequest request = spec.request(seed); + DeliveryPayload payload = request.getPayload(); + String dispatchId = UUID.randomUUID().toString(); + payload.setDispatchId(dispatchId); recorder.record(request); String machineId = request.getMachineId(); - Object payload = request.getPayload(); spec.publish(machineId, payload); - log.info("Delivery dispatched: type={} targetId={} machineId={}", type, request.getTargetId(), machineId); + log.info("Delivery dispatched: type={} targetId={} machineId={} dispatchId={}", + type, request.getTargetId(), machineId, dispatchId); } } diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryGate.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryGate.java new file mode 100644 index 0000000000..e1633a5bde --- /dev/null +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryGate.java @@ -0,0 +1,39 @@ +package com.openframe.delivery.dispatch; + +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.data.document.installedagents.InstalledAgent; +import com.openframe.data.repository.installedagents.InstalledAgentRepository; +import com.openframe.delivery.config.DeliveryProperties; +import lombok.RequiredArgsConstructor; +import org.springframework.stereotype.Component; +import org.springframework.util.StringUtils; + +import java.util.Optional; + +@Component +@RequiredArgsConstructor +public class DeliveryGate { + + private static final String OPENFRAME_CLIENT_AGENT_TYPE = "openframe-client"; + + private final DeliveryProperties properties; + private final InstalledAgentRepository installedAgentRepository; + + public boolean isOpen(DeliveryType type, String machineId) { + if (!properties.isEnabled(type)) { + return false; + } + Optional minAgentVersion = properties.minAgentVersion(type); + return minAgentVersion + .map(minimum -> isAgentAtLeast(machineId, minimum)) + .orElse(false); + } + + private boolean isAgentAtLeast(String machineId, String minimum) { + return installedAgentRepository.findByMachineIdAndAgentType(machineId, OPENFRAME_CLIENT_AGENT_TYPE) + .map(InstalledAgent::getVersion) + .filter(StringUtils::hasText) + .map(version -> AgentVersion.isAtLeast(version, minimum)) + .orElse(false); + } +} diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryRecorder.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryRecorder.java index f6b2e755c7..8e3af5b66f 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryRecorder.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryRecorder.java @@ -8,6 +8,7 @@ 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.DeliveryRequest; import com.openframe.delivery.track.DeliveryId; import lombok.RequiredArgsConstructor; @@ -26,9 +27,6 @@ public class DeliveryRecorder { private final ObjectMapper objectMapper; public void record(DeliveryRequest request) { - if (!properties.isEnabled()) { - return; - } MachineDelivery delivery = pendingRow(request); repository.upsertPending(delivery); log.info("Delivery recorded: type={} targetId={} machineId={}", @@ -41,7 +39,8 @@ private MachineDelivery pendingRow(DeliveryRequest request) { String targetId = request.getTargetId(); String machineId = request.getMachineId(); String id = DeliveryId.of(type, targetId, machineId); - Object payload = request.getPayload(); + DeliveryPayload payload = request.getPayload(); + String dispatchId = payload.getDispatchId(); String payloadJson = toJson(payload); Policy policy = properties.resolve(type); long ackThresholdSeconds = policy.getAckThresholdSeconds(); @@ -51,6 +50,7 @@ private MachineDelivery pendingRow(DeliveryRequest request) { .type(type) .targetId(targetId) .machineId(machineId) + .dispatchId(dispatchId) .status(DeliveryStatus.PENDING) .attempts(0) .errors(0) diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryPayload.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryPayload.java new file mode 100644 index 0000000000..962a3c4b02 --- /dev/null +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryPayload.java @@ -0,0 +1,8 @@ +package com.openframe.delivery.spec; + +public interface DeliveryPayload { + + String getDispatchId(); + + void setDispatchId(String dispatchId); +} diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryRequest.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryRequest.java index 469580feb3..04dbdcd930 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryRequest.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryRequest.java @@ -6,7 +6,7 @@ @Getter @Builder -public class DeliveryRequest

{ +public class DeliveryRequest

{ private final DeliveryType type; private final String targetId; private final 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 12c60cf908..1e461bf637 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 @@ -4,7 +4,7 @@ import com.openframe.data.document.delivery.DeliveryType; import com.openframe.data.document.delivery.MachineDelivery; -public interface DeliverySpec { +public interface DeliverySpec { DeliveryType getType(); diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpecRegistry.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpecRegistry.java index b6fa71e87b..11f85af26d 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpecRegistry.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliverySpecRegistry.java @@ -33,12 +33,12 @@ public DeliverySpecRegistry(ObjectProvider> specs) { } @SuppressWarnings("unchecked") - public Optional> find(DeliveryType type) { + public Optional> find(DeliveryType type) { DeliverySpec spec = (DeliverySpec) byType.get(type); return Optional.ofNullable(spec); } - public DeliverySpec require(DeliveryType type) { + public DeliverySpec require(DeliveryType type) { Optional> spec = find(type); return spec.orElseThrow(() -> new IllegalArgumentException("No spec registered for delivery type: " + type.name())); } diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryCloser.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryCloser.java index 6e065c6705..bd9cc05128 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryCloser.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryCloser.java @@ -8,6 +8,7 @@ import com.openframe.delivery.config.DeliveryProperties; import com.openframe.delivery.config.DeliveryProperties.Policy; 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; @@ -23,7 +24,7 @@ @Slf4j @Component @RequiredArgsConstructor -@ConditionalOnProperty(name = {"openframe.delivery.enabled", "openframe.delivery.sweep.enabled"}, havingValue = "true") +@ConditionalOnProperty(name = "openframe.delivery.sweep.enabled", havingValue = "true") public class DeliveryCloser { private final MachineDeliveryRepository repository; @@ -67,7 +68,7 @@ public void cancel(MachineDelivery delivery, Set from, String re private void notifySpec(MachineDelivery delivery, DeliveryFailure failure) { DeliveryType type = delivery.getType(); - Optional> spec = registry.find(type); + Optional> spec = registry.find(type); spec.ifPresentOrElse( registered -> registered.onFailed(delivery, failure), () -> log.warn("No spec registered for delivery type {}, onFailed skipped", type)); diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliverySweepScheduler.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliverySweepScheduler.java index 09881ff4b4..f6dd706ca5 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliverySweepScheduler.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliverySweepScheduler.java @@ -11,7 +11,7 @@ @Slf4j @Component @RequiredArgsConstructor -@ConditionalOnProperty(name = {"openframe.delivery.enabled", "openframe.delivery.sweep.enabled"}, havingValue = "true") +@ConditionalOnProperty(name = "openframe.delivery.sweep.enabled", havingValue = "true") public class DeliverySweepScheduler { private static final String PASS_RETRY = "retry"; 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 155242cb77..47c91e44ea 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 @@ -11,6 +11,7 @@ import com.openframe.delivery.config.DeliveryProperties; import com.openframe.delivery.config.DeliveryProperties.Policy; 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; @@ -28,7 +29,7 @@ @Slf4j @Service @RequiredArgsConstructor -@ConditionalOnProperty(name = {"openframe.delivery.enabled", "openframe.delivery.sweep.enabled"}, havingValue = "true") +@ConditionalOnProperty(name = "openframe.delivery.sweep.enabled", havingValue = "true") public class DeliverySweepService { private final MachineDeliveryRepository repository; @@ -102,9 +103,9 @@ private void parkSkipOrFailOffline(MachineDelivery delivery, Policy policy, Inst private void republish(MachineDelivery delivery, Policy policy, Instant now) { DeliveryType type = delivery.getType(); - DeliverySpec spec = registry.require(type); - Class payloadClass = spec.getPayloadClass(); - Object payload = readPayload(delivery, payloadClass); + DeliverySpec spec = registry.require(type); + Class payloadClass = spec.getPayloadClass(); + DeliveryPayload payload = readPayload(delivery, payloadClass); String machineId = delivery.getMachineId(); boolean published = publish(spec, machineId, payload); if (!published) { @@ -128,7 +129,7 @@ private void republish(MachineDelivery delivery, Policy policy, Instant now) { type, delivery.getTargetId(), machineId, attempt, dueAt); } - private static boolean publish(DeliverySpec spec, String machineId, Object payload) { + private static boolean publish(DeliverySpec spec, String machineId, DeliveryPayload payload) { try { spec.publish(machineId, payload); return true; diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryWatchdogService.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryWatchdogService.java index c7ddb6b752..266651d23c 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryWatchdogService.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryWatchdogService.java @@ -19,7 +19,7 @@ @Slf4j @Service @RequiredArgsConstructor -@ConditionalOnProperty(name = {"openframe.delivery.enabled", "openframe.delivery.sweep.enabled"}, havingValue = "true") +@ConditionalOnProperty(name = "openframe.delivery.sweep.enabled", havingValue = "true") public class DeliveryWatchdogService { private final MachineDeliveryRepository repository; 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 6e1e7fbb46..0375bba728 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 @@ -15,7 +15,7 @@ @Component @RequiredArgsConstructor -@ConditionalOnProperty(name = {"openframe.delivery.enabled", "openframe.delivery.sweep.enabled"}, havingValue = "true") +@ConditionalOnProperty(name = "openframe.delivery.sweep.enabled", havingValue = "true") public class MachineOnlineStatus { private static final Set GONE = EnumSet.of( 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 1d900a1146..f7fe0d44d2 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 @@ -19,15 +19,17 @@ public class DeliveryTracker { private final MachineDeliveryRepository repository; private final DeliveryProperties properties; - public void acknowledge(DeliveryType type, String targetId, String machineId) { + public void acknowledge(DeliveryType type, String targetId, String machineId, String dispatchId) { String id = DeliveryId.of(type, targetId, machineId); Instant now = Instant.now(); Policy policy = properties.resolve(type); long resultTimeoutSeconds = policy.getResultTimeoutSeconds(); Instant resultDueAt = now.plusSeconds(resultTimeoutSeconds); - boolean acked = repository.markAcked(id, DeliveryStatus.UNACKED, now, resultDueAt); + boolean acked = repository.markAcked(id, dispatchId, DeliveryStatus.UNACKED, now, resultDueAt); if (acked) { - log.info("Delivery ACKED: type={} targetId={} machineId={}", type, targetId, machineId); + log.info("Delivery ACKED: type={} targetId={} machineId={} dispatchId={}", type, targetId, machineId, dispatchId); + } else { + log.debug("Delivery ack ignored, no unacked row for this dispatch: id={} dispatchId={}", id, dispatchId); } } diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryPropertiesTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryPropertiesTest.java index 4bc81b1bdb..b6dee8b9db 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryPropertiesTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryPropertiesTest.java @@ -7,6 +7,7 @@ import org.junit.jupiter.api.Test; import java.util.Map; +import java.util.Optional; import static com.openframe.delivery.config.DeliveryTestPolicies.ACK_THRESHOLD; import static com.openframe.delivery.config.DeliveryTestPolicies.BACKOFF_MULTIPLIER; @@ -19,6 +20,7 @@ class DeliveryPropertiesTest { private static final int UNINSTALL_MAX_ATTEMPTS = 5; + private static final String MIN_AGENT_VERSION = "1.4.0"; private DeliveryProperties properties; @@ -73,4 +75,63 @@ void resolve_typeOverridesOfflineBehavior_overrideWins() { // verifications assertThat(resolved.getOfflineBehavior()).isEqualTo(DeliveryOfflineBehavior.SKIP); } + @Test + void isEnabled_typeNotListed_false() { + // setup + properties.setEnabled(Map.of()); + + // execution + boolean enabled = properties.isEnabled(DeliveryType.TOOL_INSTALLATION); + + // verifications + assertThat(enabled).isFalse(); + } + + @Test + void isEnabled_typeListedOff_false() { + // setup + properties.setEnabled(Map.of(DeliveryType.TOOL_INSTALLATION, false)); + + // execution + boolean enabled = properties.isEnabled(DeliveryType.TOOL_INSTALLATION); + + // verifications + assertThat(enabled).isFalse(); + } + + @Test + void minAgentVersion_typeNotListed_empty() { + // setup + properties.setMinAgentVersion(Map.of()); + + // execution + Optional minAgentVersion = properties.minAgentVersion(DeliveryType.TOOL_INSTALLATION); + + // verifications + assertThat(minAgentVersion).isEmpty(); + } + + @Test + void minAgentVersion_typeListed_versionReturned() { + // setup + properties.setMinAgentVersion(Map.of(DeliveryType.TOOL_INSTALLATION, MIN_AGENT_VERSION)); + + // execution + Optional minAgentVersion = properties.minAgentVersion(DeliveryType.TOOL_INSTALLATION); + + // verifications + assertThat(minAgentVersion).contains(MIN_AGENT_VERSION); + } + + @Test + void isEnabled_typeListedOn_true() { + // setup + properties.setEnabled(Map.of(DeliveryType.TOOL_INSTALLATION, true)); + + // execution + boolean enabled = properties.isEnabled(DeliveryType.TOOL_INSTALLATION); + + // verifications + assertThat(enabled).isTrue(); + } } diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryTestPolicies.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryTestPolicies.java index 161ffaab64..d97faf54e5 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryTestPolicies.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryTestPolicies.java @@ -31,7 +31,6 @@ public static DeliveryProperties properties() { Sweep sweep = new Sweep(); sweep.setBatchSize(BATCH_SIZE); DeliveryProperties properties = new DeliveryProperties(); - properties.setEnabled(true); properties.setDefaults(defaults); properties.setSweep(sweep); return properties; diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/AgentVersionTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/AgentVersionTest.java new file mode 100644 index 0000000000..18168d07d9 --- /dev/null +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/AgentVersionTest.java @@ -0,0 +1,32 @@ +package com.openframe.delivery.dispatch; + +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import static org.assertj.core.api.Assertions.assertThat; + +class AgentVersionTest { + + @ParameterizedTest + @CsvSource({ + "1.4.0, 1.4.0, true", + "1.4.1, 1.4.0, true", + "1.10.0, 1.9.9, true", + "1.4, 1.4.0, true", + "1.4.0.1, 1.4.0, true", + "v1.4.0, 1.4.0, true", + "2.0.0, 1.4.0, true", + "1.3.9, 1.4.0, false", + "0.9.99, 1.0.0, false", + "1.4, 1.4.1, false" + }) + void isAtLeast_versionAgainstMinimum_numericSegmentsDecide(String version, String minimum, boolean expected) { + // setup + + // execution + boolean atLeast = AgentVersion.isAtLeast(version, minimum); + + // verifications + assertThat(atLeast).isEqualTo(expected); + } +} 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 47c9c132de..01d3dfccf9 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 @@ -13,6 +13,7 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.verify; @@ -50,7 +51,7 @@ void setUp() { } @Test - void dispatch_seed_requestRecordedThenPublishedThroughSpec() { + void dispatch_seed_dispatchIdSetThenRecordedThenPublishedThroughSpec() { // setup doReturn(spec).when(registry).require(DeliveryType.TOOL_INSTALLATION); when(spec.request(seed)).thenReturn(request); @@ -59,6 +60,7 @@ void dispatch_seed_requestRecordedThenPublishedThroughSpec() { dispatcher.dispatch(seed); // verifications + assertThat(payload.getDispatchId()).isNotBlank(); verify(recorder).record(request); verify(spec).publish(MACHINE_ID, payload); } diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryGateTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryGateTest.java new file mode 100644 index 0000000000..8c51d5cfc5 --- /dev/null +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryGateTest.java @@ -0,0 +1,125 @@ +package com.openframe.delivery.dispatch; + +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.data.document.installedagents.InstalledAgent; +import com.openframe.data.repository.installedagents.InstalledAgentRepository; +import com.openframe.delivery.config.DeliveryProperties; +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.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.util.Map; +import java.util.Optional; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class DeliveryGateTest { + + private static final String MACHINE_ID = "mach-42"; + private static final String CLIENT_AGENT_TYPE = "openframe-client"; + private static final String MIN_VERSION = "1.4.0"; + + @Mock private InstalledAgentRepository installedAgentRepository; + + private DeliveryGate gate; + + private DeliveryProperties properties; + private InstalledAgent client; + + @BeforeEach + void setUp() { + properties = DeliveryTestPolicies.properties(); + properties.setEnabled(Map.of(DeliveryType.TOOL_INSTALLATION, true)); + properties.setMinAgentVersion(Map.of(DeliveryType.TOOL_INSTALLATION, MIN_VERSION)); + client = new InstalledAgent(); + client.setMachineId(MACHINE_ID); + client.setAgentType(CLIENT_AGENT_TYPE); + gate = new DeliveryGate(properties, installedAgentRepository); + } + + @Test + void isOpen_typeOff_falseWithoutLookup() { + // setup + properties.setEnabled(Map.of()); + + // execution + boolean open = gate.isOpen(DeliveryType.TOOL_INSTALLATION, MACHINE_ID); + + // verifications + assertThat(open).isFalse(); + verifyNoInteractions(installedAgentRepository); + } + + @Test + void isOpen_typeOnWithoutMinAgentVersion_falseWithoutLookup() { + // setup + properties.setMinAgentVersion(Map.of()); + + // execution + boolean open = gate.isOpen(DeliveryType.TOOL_INSTALLATION, MACHINE_ID); + + // verifications + assertThat(open).isFalse(); + verifyNoInteractions(installedAgentRepository); + } + + @Test + void isOpen_clientAgentRowMissing_false() { + // setup + when(installedAgentRepository.findByMachineIdAndAgentType(MACHINE_ID, CLIENT_AGENT_TYPE)).thenReturn(Optional.empty()); + + // execution + boolean open = gate.isOpen(DeliveryType.TOOL_INSTALLATION, MACHINE_ID); + + // verifications + assertThat(open).isFalse(); + } + + @Test + void isOpen_clientAgentVersionBlank_false() { + // setup + client.setVersion(" "); + when(installedAgentRepository.findByMachineIdAndAgentType(MACHINE_ID, CLIENT_AGENT_TYPE)).thenReturn(Optional.of(client)); + + // execution + boolean open = gate.isOpen(DeliveryType.TOOL_INSTALLATION, MACHINE_ID); + + // verifications + assertThat(open).isFalse(); + } + + @Test + void isOpen_clientAgentBelowMinimum_false() { + // setup + client.setVersion("1.3.9"); + when(installedAgentRepository.findByMachineIdAndAgentType(MACHINE_ID, CLIENT_AGENT_TYPE)).thenReturn(Optional.of(client)); + + // execution + boolean open = gate.isOpen(DeliveryType.TOOL_INSTALLATION, MACHINE_ID); + + // verifications + assertThat(open).isFalse(); + } + + @ParameterizedTest + @ValueSource(strings = {"1.4.0", "1.4.1", "1.10.0", "2.0.0"}) + void isOpen_clientAgentAtOrAboveMinimum_true(String version) { + // setup + client.setVersion(version); + when(installedAgentRepository.findByMachineIdAndAgentType(MACHINE_ID, CLIENT_AGENT_TYPE)).thenReturn(Optional.of(client)); + + // execution + boolean open = gate.isOpen(DeliveryType.TOOL_INSTALLATION, MACHINE_ID); + + // verifications + assertThat(open).isTrue(); + } +} diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryRecorderTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryRecorderTest.java index f1a5ee635c..2a5fa2d44c 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryRecorderTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryRecorderTest.java @@ -5,7 +5,6 @@ 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.DeliveryTestPolicies; import com.openframe.delivery.spec.DeliveryRequest; import com.openframe.delivery.spec.TestPayload; @@ -21,13 +20,13 @@ import static com.openframe.delivery.config.DeliveryTestPolicies.TTL; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; @ExtendWith(MockitoExtension.class) class DeliveryRecorderTest { private static final String MACHINE_ID = "mach-42"; private static final String VALUE = "issued"; + private static final String DISPATCH_ID = "d-1"; @Mock private MachineDeliveryRepository repository; @@ -35,21 +34,20 @@ class DeliveryRecorderTest { private DeliveryRecorder recorder; - private DeliveryProperties properties; private DeliveryRequest request; @BeforeEach void setUp() { TestPayload payload = new TestPayload(); payload.setValue(VALUE); + payload.setDispatchId(DISPATCH_ID); request = DeliveryRequest.builder() .type(DeliveryType.CLIENT_UNINSTALL) .targetId(MACHINE_ID) .machineId(MACHINE_ID) .payload(payload) .build(); - properties = DeliveryTestPolicies.properties(); - recorder = new DeliveryRecorder(repository, properties, new ObjectMapper()); + recorder = new DeliveryRecorder(repository, DeliveryTestPolicies.properties(), new ObjectMapper()); } @Test @@ -66,21 +64,10 @@ void record_request_pendingRowUpsertedDueAfterAckThreshold() { assertThat(saved.getType()).isEqualTo(DeliveryType.CLIENT_UNINSTALL); assertThat(saved.getStatus()).isEqualTo(DeliveryStatus.PENDING); assertThat(saved.getAttempts()).isZero(); - assertThat(saved.getPayloadJson()).contains(VALUE); + assertThat(saved.getDispatchId()).isEqualTo(DISPATCH_ID); + assertThat(saved.getPayloadJson()).contains(VALUE).contains(DISPATCH_ID); assertThat(saved.getDueAt()).isEqualTo(saved.getDispatchedAt().plusSeconds(ACK_THRESHOLD)); assertThat(saved.getExpiresAt()).isEqualTo(saved.getDispatchedAt().plusSeconds(TTL)); assertThat(saved.getErrors()).isZero(); } - - @Test - void record_engineDisabled_nothingWritten() { - // setup - properties.setEnabled(false); - - // execution - recorder.record(request); - - // verifications - verifyNoInteractions(repository); - } } diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/DeliverySpecRegistryTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/DeliverySpecRegistryTest.java index b638d80bf1..63628e3e73 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/DeliverySpecRegistryTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/DeliverySpecRegistryTest.java @@ -23,7 +23,7 @@ void require_registeredType_specReturned() { DeliverySpecRegistry registry = new DeliverySpecRegistry(specs); // execution - DeliverySpec resolved = registry.require(DeliveryType.TOOL_INSTALLATION); + DeliverySpec resolved = registry.require(DeliveryType.TOOL_INSTALLATION); // verifications assertThat(resolved).isSameAs(spec); @@ -77,7 +77,7 @@ void find_registeredType_specPresent() { DeliverySpecRegistry registry = new DeliverySpecRegistry(specs); // execution - Optional> found = registry.find(DeliveryType.TOOL_INSTALLATION); + Optional> found = registry.find(DeliveryType.TOOL_INSTALLATION); // verifications assertThat(found).get().isSameAs(spec); @@ -90,7 +90,7 @@ void find_unregisteredType_empty() { DeliverySpecRegistry registry = new DeliverySpecRegistry(specs); // execution - Optional> found = registry.find(DeliveryType.CLIENT_UNINSTALL); + Optional> found = registry.find(DeliveryType.CLIENT_UNINSTALL); // verifications assertThat(found).isEmpty(); diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestPayload.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestPayload.java index c344b62b92..e21b1910b8 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestPayload.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestPayload.java @@ -3,6 +3,8 @@ import lombok.Data; @Data -public class TestPayload { +public class TestPayload implements DeliveryPayload { + + private String dispatchId; private String value; } 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 4bb5e6e1aa..151cd1d23e 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 @@ -29,6 +29,7 @@ class DeliveryTrackerTest { private static final String TARGET_ID = "fleetmdm-agent"; private static final String DELIVERY_ID = DeliveryId.of(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID); private static final long TWO_ROWS = 2L; + private static final String DISPATCH_ID = "d-1"; @Mock private MachineDeliveryRepository repository; @@ -45,10 +46,10 @@ void setUp() { @Test void acknowledge_typedKey_unackedRowMarkedAckedWithResultDeadline() { // setup - when(repository.markAcked(eq(DELIVERY_ID), eq(DeliveryStatus.UNACKED), atCaptor.capture(), untilCaptor.capture())).thenReturn(true); + 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); + tracker.acknowledge(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID, DISPATCH_ID); // verifications Instant ackedAt = atCaptor.getValue(); diff --git a/openframe-tool-agent-nats-installation/src/main/java/com/openframe/data/service/ToolInstallationService.java b/openframe-tool-agent-nats-installation/src/main/java/com/openframe/data/service/ToolInstallationService.java index f23f850667..921657caed 100644 --- a/openframe-tool-agent-nats-installation/src/main/java/com/openframe/data/service/ToolInstallationService.java +++ b/openframe-tool-agent-nats-installation/src/main/java/com/openframe/data/service/ToolInstallationService.java @@ -2,7 +2,11 @@ import com.openframe.data.document.tool.IntegratedTool; import com.openframe.data.document.toolagent.IntegratedToolAgent; +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.data.nats.delivery.ToolInstallationDeliverySpec; import com.openframe.data.nats.publisher.ToolInstallationNatsPublisher; +import com.openframe.delivery.dispatch.DeliveryGate; +import com.openframe.delivery.dispatch.DeliveryDispatcher; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; @@ -21,6 +25,8 @@ public class ToolInstallationService { private final IntegratedToolService integratedToolService; private final ToolCommandParamsResolver toolCommandParamsResolver; private final ToolInstallationNatsPublisher toolInstallationNatsPublisher; + private final DeliveryGate deliveryGate; + private final DeliveryDispatcher deliveryDispatcher; public void process(String machineId, IntegratedToolAgent toolAgent) { process(machineId, toolAgent, false); @@ -41,7 +47,7 @@ public void process(String machineId, IntegratedToolAgent toolAgent, boolean rei List runCommandArgs = toolAgent.getRunCommandArgs(); toolAgent.setRunCommandArgs(toolCommandParamsResolver.process(toolId, runCommandArgs)); - toolInstallationNatsPublisher.publish(machineId, toolAgent, tool, reinstall); + publish(machineId, toolAgent, tool, reinstall); log.info("Published {} agent installation message for machine {}", toolId, machineId); } catch (Exception e) { // TODO: add fallback mechanism @@ -64,4 +70,13 @@ private IntegratedTool getIntegratedToolData(String toolId) { } } + + private void publish(String machineId, IntegratedToolAgent toolAgent, IntegratedTool tool, boolean reinstall) { + if (deliveryGate.isOpen(DeliveryType.TOOL_INSTALLATION, machineId)) { + ToolInstallationDeliverySpec.Seed seed = new ToolInstallationDeliverySpec.Seed(machineId, toolAgent, tool, reinstall); + deliveryDispatcher.dispatch(seed); + return; + } + toolInstallationNatsPublisher.publish(machineId, toolAgent, tool, reinstall); + } } From 620a04b1e37c6e705c0b25851e233f18c4e21814 Mon Sep 17 00:00:00 2001 From: semen-flamingo Date: Wed, 23 Sep 2026 13:04:44 +0200 Subject: [PATCH 2/5] refactor(delivery): generic result endpoint, no wake listener, no version gate Review follow-up (#2212): - machine.{id}.delivery.result carries ACKED/DONE/FAILED with dispatchId for every type; InstalledAgentService and ScriptExecutionAcknowledgeListener are back to main - offline rows are postponed to the next sweep tick instead of parked and woken by a device event - per-type flag only; the agent-version gate and AgentVersion are gone - ToolInstallationDeliverySeed is a top-level class Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01BnYUpYgKvuMw6hB5VZSimY --- .../delivery/DeliveryResultListener.java | 103 +++++++++++ .../DeviceOnlineDeliveryWakeListener.java | 49 ------ .../ScriptExecutionAcknowledgeListener.java | 22 +-- .../client/service/InstalledAgentService.java | 4 - .../delivery/DeliveryResultListenerTest.java | 163 ++++++++++++++++++ .../DeviceOnlineDeliveryWakeListenerTest.java | 88 ---------- ...criptExecutionAcknowledgeListenerTest.java | 115 ------------ .../service/InstalledAgentServiceTest.java | 103 ----------- .../document/delivery/DeliveryFailure.java | 3 +- .../document/delivery/MachineDelivery.java | 3 +- .../CustomMachineDeliveryRepository.java | 5 +- .../CustomMachineDeliveryRepositoryImpl.java | 40 ++--- ...stomMachineDeliveryRepositoryImplTest.java | 25 +-- .../data/nats/delivery/DeliveryResult.java | 7 + .../nats/delivery/DeliveryResultMessage.java | 23 +++ .../ToolInstallationDeliverySeed.java | 23 +++ .../ToolInstallationDeliverySpec.java | 21 +-- .../ScriptExecutionAcknowledgeMessage.java | 5 - .../ToolInstallationDeliverySpecTest.java | 4 +- .../delivery/config/DeliveryProperties.java | 12 +- .../delivery/dispatch/AgentVersion.java | 42 ----- .../delivery/dispatch/DeliveryGate.java | 39 ----- .../delivery/sweep/DeliverySweepService.java | 11 +- .../sweep/DeliveryWatchdogService.java | 1 + .../{sweep => track}/DeliveryCloser.java | 22 ++- .../delivery/track/DeliveryTracker.java | 22 +-- .../config/DeliveryPropertiesTest.java | 26 --- .../delivery/config/DeliveryTestPolicies.java | 2 + .../delivery/dispatch/AgentVersionTest.java | 32 ---- .../delivery/dispatch/DeliveryGateTest.java | 125 -------------- .../sweep/DeliverySweepServiceTest.java | 21 ++- .../sweep/DeliveryWatchdogServiceTest.java | 1 + .../{sweep => track}/DeliveryCloserTest.java | 33 +++- .../delivery/track/DeliveryTrackerTest.java | 16 +- .../NatsStreamConfigurationInitializer.java | 8 + .../data/service/ToolInstallationService.java | 10 +- 36 files changed, 467 insertions(+), 762 deletions(-) create mode 100644 openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeliveryResultListener.java delete mode 100644 openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListener.java create mode 100644 openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeliveryResultListenerTest.java delete mode 100644 openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListenerTest.java delete mode 100644 openframe-client-core/src/test/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListenerTest.java delete mode 100644 openframe-client-core/src/test/java/com/openframe/client/service/InstalledAgentServiceTest.java create mode 100644 openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/DeliveryResult.java create mode 100644 openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/DeliveryResultMessage.java create mode 100644 openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySeed.java delete mode 100644 openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/AgentVersion.java delete mode 100644 openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryGate.java rename openframe-machine-delivery/src/main/java/com/openframe/delivery/{sweep => track}/DeliveryCloser.java (76%) delete mode 100644 openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/AgentVersionTest.java delete mode 100644 openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryGateTest.java rename openframe-machine-delivery/src/test/java/com/openframe/delivery/{sweep => track}/DeliveryCloserTest.java (77%) 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 new file mode 100644 index 0000000000..83a35483e3 --- /dev/null +++ b/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeliveryResultListener.java @@ -0,0 +1,103 @@ +package com.openframe.client.listener.delivery; + +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.track.DeliveryTracker; +import io.nats.client.Connection; +import io.nats.client.Message; +import lombok.extern.slf4j.Slf4j; +import org.springframework.stereotype.Component; + +import java.nio.charset.StandardCharsets; + +import static org.springframework.util.StringUtils.hasText; + +@Slf4j +@Component +public class DeliveryResultListener extends AbstractJetStreamPushListener { + + private final ObjectMapper objectMapper; + private final NatsTopicMachineIdExtractor machineIdExtractor; + private final DeliveryTracker deliveryTracker; + + public DeliveryResultListener( + Connection natsConnection, + ObjectMapper objectMapper, + NatsTopicMachineIdExtractor machineIdExtractor, + DeliveryTracker deliveryTracker + ) { + super(natsConnection); + this.objectMapper = objectMapper; + this.machineIdExtractor = machineIdExtractor; + this.deliveryTracker = deliveryTracker; + } + + @Override + protected String getStreamName() { + return DeliveryResultMessage.STREAM; + } + + @Override + protected String getSubject() { + return DeliveryResultMessage.SUBJECT_FILTER; + } + + @Override + protected String getConsumerName() { + return "delivery-result-processor-v1"; + } + + @Override + protected String getDeliveryGroup() { + return "delivery-result"; + } + + @Override + protected String getDeliverySubject() { + return "machine.delivery.result.delivery"; + } + + @Override + protected void handleMessage(Message message) { + String payload = new String(message.getData(), StandardCharsets.UTF_8); + String subject = message.getSubject(); + try { + String machineId = machineIdExtractor.extract(subject); + DeliveryResultMessage report = objectMapper.readValue(payload, DeliveryResultMessage.class); + if (!isComplete(report)) { + log.warn("Delivery result without type, targetId, dispatchId or result dropped: machineId={} payload={}", machineId, payload); + message.ack(); + return; + } + apply(machineId, report); + message.ack(); + } catch (JsonProcessingException | IllegalArgumentException permanentlyBad) { + log.warn("Dropping malformed delivery result subject={} payload={}", subject, payload, permanentlyBad); + message.ack(); + } catch (Exception e) { + log.error("Unexpected error processing delivery result: {}", payload, e); + } + } + + private void apply(String machineId, DeliveryResultMessage report) { + DeliveryType type = report.getType(); + String targetId = report.getTargetId(); + String dispatchId = report.getDispatchId(); + switch (report.getResult()) { + case ACKED -> deliveryTracker.acknowledge(type, targetId, machineId, dispatchId); + case DONE -> deliveryTracker.complete(type, targetId, machineId, dispatchId); + case FAILED -> deliveryTracker.fail(type, targetId, machineId, dispatchId, report.getError()); + } + } + + private static boolean isComplete(DeliveryResultMessage report) { + return report.getType() != null + && hasText(report.getTargetId()) + && hasText(report.getDispatchId()) + && report.getResult() != null; + } +} diff --git a/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListener.java b/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListener.java deleted file mode 100644 index 0397a74644..0000000000 --- a/openframe-client-core/src/main/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListener.java +++ /dev/null @@ -1,49 +0,0 @@ -package com.openframe.client.listener.delivery; - -import com.openframe.client.event.DeviceCameOnlineEvent; -import com.openframe.client.event.DeviceFirstConnectedEvent; -import com.openframe.data.document.device.Machine; -import com.openframe.delivery.track.DeliveryTracker; -import lombok.RequiredArgsConstructor; -import lombok.extern.slf4j.Slf4j; -import org.springframework.context.event.EventListener; -import org.springframework.stereotype.Component; - -import static com.openframe.data.document.device.DeviceStatus.ONLINE; - -@Slf4j -@Component -@RequiredArgsConstructor -public class DeviceOnlineDeliveryWakeListener { - - private final DeliveryTracker deliveryTracker; - - @EventListener - public void onDeviceFirstConnected(DeviceFirstConnectedEvent event) { - Machine machine = event.getMachine(); - if (isOnline(machine)) { - wake(machine); - } - } - - @EventListener - public void onDeviceCameOnline(DeviceCameOnlineEvent event) { - Machine machine = event.getMachine(); - wake(machine); - } - - // listeners of one event run in sequence on the publisher's thread: an exception here would also skip the - // SaaS DEVICE_REGISTERED listener; parked rows re-check on their own within max-retry-interval anyway - private void wake(Machine machine) { - String machineId = machine.getMachineId(); - try { - deliveryTracker.wake(machineId); - } catch (RuntimeException e) { - log.warn("Failed to wake parked deliveries, they re-check on their own: machineId={}", machineId, e); - } - } - - private static boolean isOnline(Machine machine) { - return machine.getStatus() == ONLINE; - } -} diff --git a/openframe-client-core/src/main/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListener.java b/openframe-client-core/src/main/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListener.java index 10fb187504..7a22c0d443 100644 --- a/openframe-client-core/src/main/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListener.java +++ b/openframe-client-core/src/main/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListener.java @@ -2,8 +2,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.openframe.client.service.rmm.ScriptExecutionAcknowledgeService; -import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.delivery.track.DeliveryTracker; import com.openframe.data.nats.listener.AbstractJetStreamPushListener; import com.openframe.data.nats.rmm.model.ScriptExecutionAcknowledgeMessage; import io.nats.client.Connection; @@ -19,18 +17,15 @@ public class ScriptExecutionAcknowledgeListener extends AbstractJetStreamPushLis private final ObjectMapper objectMapper; private final ScriptExecutionAcknowledgeService acknowledgeService; - private final DeliveryTracker deliveryTracker; public ScriptExecutionAcknowledgeListener( Connection natsConnection, ObjectMapper objectMapper, - ScriptExecutionAcknowledgeService acknowledgeService, - DeliveryTracker deliveryTracker + ScriptExecutionAcknowledgeService acknowledgeService ) { super(natsConnection); this.objectMapper = objectMapper; this.acknowledgeService = acknowledgeService; - this.deliveryTracker = deliveryTracker; } @Override @@ -63,23 +58,10 @@ protected void handleMessage(Message message) { String payload = new String(message.getData(), StandardCharsets.UTF_8); try { ScriptExecutionAcknowledgeMessage ack = objectMapper.readValue(payload, ScriptExecutionAcknowledgeMessage.class); - if (isDeliveryAck(ack)) { - deliveryTracker.acknowledge(ack.getType(), ack.getTargetId(), ack.getMachineId(), ack.getDispatchId()); - } - if (isScriptAck(ack)) { - acknowledgeService.acknowledge(ack); - } + acknowledgeService.acknowledge(ack); message.ack(); } catch (Exception e) { log.error("Unexpected error processing execution ack: {}", payload, e); } } - - private static boolean isDeliveryAck(ScriptExecutionAcknowledgeMessage ack) { - return ack.getType() != null && ack.getTargetId() != null; - } - - private static boolean isScriptAck(ScriptExecutionAcknowledgeMessage ack) { - return ack.getType() == null || ack.getType() == DeliveryType.SCRIPT_SCHEDULE; - } } diff --git a/openframe-client-core/src/main/java/com/openframe/client/service/InstalledAgentService.java b/openframe-client-core/src/main/java/com/openframe/client/service/InstalledAgentService.java index e1cebf5c74..85d1511be4 100644 --- a/openframe-client-core/src/main/java/com/openframe/client/service/InstalledAgentService.java +++ b/openframe-client-core/src/main/java/com/openframe/client/service/InstalledAgentService.java @@ -4,8 +4,6 @@ import com.openframe.client.exception.MachineNotFoundException; import com.openframe.data.document.installedagents.InstalledAgent; import com.openframe.data.document.tool.ConnectionStatus; -import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.delivery.track.DeliveryTracker; import com.openframe.data.repository.device.MachineRepository; import com.openframe.data.repository.installedagents.InstalledAgentRepository; import lombok.RequiredArgsConstructor; @@ -23,7 +21,6 @@ public class InstalledAgentService { private final InstalledAgentRepository installedAgentRepository; private final MachineRepository machineRepository; - private final DeliveryTracker deliveryTracker; @Transactional public void addInstalledAgent(String machineId, String agentType, String version, boolean lastAttempt) { @@ -39,7 +36,6 @@ public void addInstalledAgent(String machineId, String agentType, String version installedAgent -> updateExistingInstalledAgent(installedAgent, version, machineId, agentType), () -> addNewInstalledAgent(machineId, agentType, version) ); - deliveryTracker.complete(DeliveryType.TOOL_INSTALLATION, agentType, machineId); } @Transactional 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 new file mode 100644 index 0000000000..15beeaca5c --- /dev/null +++ b/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeliveryResultListenerTest.java @@ -0,0 +1,163 @@ +package com.openframe.client.listener.delivery; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.openframe.client.service.NatsTopicMachineIdExtractor; +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.delivery.track.DeliveryTracker; +import io.nats.client.Connection; +import io.nats.client.Message; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.mockito.Mockito.doThrow; +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 DeliveryResultListenerTest { + + private static final String MACHINE_ID = "mach-42"; + 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 String ERROR = "download failed"; + private static final String ACKED = + "{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\",\"result\":\"ACKED\"}"; + private static final String DONE = + "{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\",\"result\":\"DONE\"}"; + private static final String FAILED = + "{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\",\"result\":\"FAILED\",\"error\":\"download failed\"}"; + private static final String WITHOUT_DISPATCH_ID = + "{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"result\":\"ACKED\"}"; + private static final String UNKNOWN_RESULT = + "{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\",\"result\":\"RETRYING\"}"; + private static final String UNKNOWN_TYPE = + "{\"type\":\"HOLOGRAM\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\",\"result\":\"ACKED\"}"; + private static final String MALFORMED = "not json"; + + @Mock private Connection natsConnection; + @Mock private DeliveryTracker deliveryTracker; + @Mock private Message message; + + private DeliveryResultListener listener; + + @BeforeEach + void setUp() { + listener = new DeliveryResultListener(natsConnection, new ObjectMapper(), new NatsTopicMachineIdExtractor(), deliveryTracker); + } + + @Test + void handleMessage_acked_trackerAcknowledgesDispatch() { + // setup + stubMessage(ACKED); + + // execution + listener.handleMessage(message); + + // verifications + verify(deliveryTracker).acknowledge(DeliveryType.TOOL_INSTALLATION, TOOL_AGENT_ID, MACHINE_ID, DISPATCH_ID); + verify(message).ack(); + } + + @Test + void handleMessage_done_trackerCompletesDispatch() { + // setup + stubMessage(DONE); + + // execution + listener.handleMessage(message); + + // verifications + verify(deliveryTracker).complete(DeliveryType.TOOL_INSTALLATION, TOOL_AGENT_ID, MACHINE_ID, DISPATCH_ID); + verify(message).ack(); + } + + @Test + void handleMessage_failed_trackerFailsDispatchWithError() { + // setup + stubMessage(FAILED); + + // execution + listener.handleMessage(message); + + // verifications + verify(deliveryTracker).fail(DeliveryType.TOOL_INSTALLATION, TOOL_AGENT_ID, MACHINE_ID, DISPATCH_ID, ERROR); + verify(message).ack(); + } + + @Test + void handleMessage_withoutDispatchId_droppedAndAcked() { + // setup + stubMessage(WITHOUT_DISPATCH_ID); + + // execution + listener.handleMessage(message); + + // verifications + verifyNoInteractions(deliveryTracker); + verify(message).ack(); + } + + @Test + void handleMessage_unknownResult_droppedAndAcked() { + // setup + stubMessage(UNKNOWN_RESULT); + + // execution + listener.handleMessage(message); + + // verifications + verifyNoInteractions(deliveryTracker); + verify(message).ack(); + } + + @Test + void handleMessage_unknownType_droppedAndAcked() { + // setup + stubMessage(UNKNOWN_TYPE); + + // execution + listener.handleMessage(message); + + // verifications + verifyNoInteractions(deliveryTracker); + verify(message).ack(); + } + + @Test + void handleMessage_malformedPayload_droppedAndAcked() { + // setup + stubMessage(MALFORMED); + + // execution + listener.handleMessage(message); + + // verifications + verifyNoInteractions(deliveryTracker); + verify(message).ack(); + } + + @Test + 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); + + // execution + listener.handleMessage(message); + + // verifications + verify(message, never()).ack(); + } + + private void stubMessage(String json) { + when(message.getSubject()).thenReturn(SUBJECT); + when(message.getData()).thenReturn(json.getBytes(UTF_8)); + } +} diff --git a/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListenerTest.java b/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListenerTest.java deleted file mode 100644 index 0c7272c0ff..0000000000 --- a/openframe-client-core/src/test/java/com/openframe/client/listener/delivery/DeviceOnlineDeliveryWakeListenerTest.java +++ /dev/null @@ -1,88 +0,0 @@ -package com.openframe.client.listener.delivery; - -import com.openframe.client.event.DeviceCameOnlineEvent; -import com.openframe.client.event.DeviceFirstConnectedEvent; -import com.openframe.data.document.device.DeviceStatus; -import com.openframe.data.document.device.Machine; -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.InjectMocks; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; - -import static org.assertj.core.api.Assertions.assertThatCode; -import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; - -@ExtendWith(MockitoExtension.class) -class DeviceOnlineDeliveryWakeListenerTest { - - private static final String MACHINE_ID = "machine-1"; - - @Mock - private DeliveryTracker deliveryTracker; - - @InjectMocks - private DeviceOnlineDeliveryWakeListener listener; - - private Machine machine; - - @BeforeEach - void setUp() { - machine = new Machine(); - machine.setMachineId(MACHINE_ID); - } - - @Test - void onDeviceCameOnline_offlineToOnline_machineWoken() { - // setup - machine.setStatus(DeviceStatus.ONLINE); - DeviceCameOnlineEvent event = new DeviceCameOnlineEvent(this, machine); - - // execution - listener.onDeviceCameOnline(event); - - // verifications - verify(deliveryTracker).wake(MACHINE_ID); - } - - @Test - void onDeviceFirstConnected_pendingToOnline_machineWoken() { - // setup - machine.setStatus(DeviceStatus.ONLINE); - DeviceFirstConnectedEvent event = new DeviceFirstConnectedEvent(this, machine); - - // execution - listener.onDeviceFirstConnected(event); - - // verifications - verify(deliveryTracker).wake(MACHINE_ID); - } - - @Test - void onDeviceCameOnline_wakeFails_swallowedSoOtherListenersStillRun() { - // setup - machine.setStatus(DeviceStatus.ONLINE); - DeviceCameOnlineEvent event = new DeviceCameOnlineEvent(this, machine); - doThrow(new IllegalStateException("mongo down")).when(deliveryTracker).wake(MACHINE_ID); - - // execution + verifications - assertThatCode(() -> listener.onDeviceCameOnline(event)).doesNotThrowAnyException(); - } - - @Test - void onDeviceFirstConnected_pendingToOffline_nothingWoken() { - // setup - machine.setStatus(DeviceStatus.OFFLINE); - DeviceFirstConnectedEvent event = new DeviceFirstConnectedEvent(this, machine); - - // execution - listener.onDeviceFirstConnected(event); - - // verifications - verifyNoInteractions(deliveryTracker); - } -} diff --git a/openframe-client-core/src/test/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListenerTest.java b/openframe-client-core/src/test/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListenerTest.java deleted file mode 100644 index b30a7d4b3a..0000000000 --- a/openframe-client-core/src/test/java/com/openframe/client/listener/rmm/ScriptExecutionAcknowledgeListenerTest.java +++ /dev/null @@ -1,115 +0,0 @@ -package com.openframe.client.listener.rmm; - -import com.fasterxml.jackson.databind.ObjectMapper; -import com.openframe.client.service.rmm.ScriptExecutionAcknowledgeService; -import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.delivery.track.DeliveryTracker; -import com.openframe.data.nats.rmm.model.ScriptExecutionAcknowledgeMessage; -import io.nats.client.Connection; -import io.nats.client.Message; -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 static java.nio.charset.StandardCharsets.UTF_8; -import static org.assertj.core.api.Assertions.assertThat; -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 ScriptExecutionAcknowledgeListenerTest { - - private static final String MACHINE_ID = "mach-42"; - private static final String EXECUTION_ID = "exec-1"; - private static final String TOOL_AGENT_ID = "tactical-agent"; - private static final String DISPATCH_ID = "d-1"; - private static final String LEGACY_SCRIPT_ACK = - "{\"executionId\":\"exec-1\",\"machineId\":\"mach-42\",\"scriptIds\":[\"s1\"]}"; - private static final String TOOL_INSTALLATION_ACK = - "{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"tactical-agent\",\"machineId\":\"mach-42\",\"dispatchId\":\"d-1\"}"; - private static final String SCRIPT_SCHEDULE_ACK = - "{\"type\":\"SCRIPT_SCHEDULE\",\"targetId\":\"exec-1\",\"executionId\":\"exec-1\",\"machineId\":\"mach-42\",\"scriptIds\":[\"s1\"]}"; - private static final String MALFORMED = "not json"; - - @Mock private Connection natsConnection; - @Mock private ScriptExecutionAcknowledgeService acknowledgeService; - @Mock private DeliveryTracker deliveryTracker; - @Mock private Message message; - - @Captor private ArgumentCaptor ackCaptor; - - private ScriptExecutionAcknowledgeListener listener; - - @BeforeEach - void setUp() { - listener = new ScriptExecutionAcknowledgeListener(natsConnection, new ObjectMapper(), acknowledgeService, deliveryTracker); - } - - @Test - void handleMessage_legacyScriptAck_scriptServiceOnly() { - // setup - stubPayload(LEGACY_SCRIPT_ACK); - - // execution - listener.handleMessage(message); - - // verifications - verify(acknowledgeService).acknowledge(ackCaptor.capture()); - assertThat(ackCaptor.getValue().getExecutionId()).isEqualTo(EXECUTION_ID); - verifyNoInteractions(deliveryTracker); - verify(message).ack(); - } - - @Test - void handleMessage_toolInstallationAck_trackerOnly() { - // setup - stubPayload(TOOL_INSTALLATION_ACK); - - // execution - listener.handleMessage(message); - - // verifications - verify(deliveryTracker).acknowledge(DeliveryType.TOOL_INSTALLATION, TOOL_AGENT_ID, MACHINE_ID, DISPATCH_ID); - verifyNoInteractions(acknowledgeService); - verify(message).ack(); - } - - @Test - void handleMessage_scriptScheduleAckWithType_trackerAndScriptService() { - // setup - stubPayload(SCRIPT_SCHEDULE_ACK); - - // execution - listener.handleMessage(message); - - // verifications - verify(deliveryTracker).acknowledge(DeliveryType.SCRIPT_SCHEDULE, EXECUTION_ID, MACHINE_ID, null); - verify(acknowledgeService).acknowledge(ackCaptor.capture()); - assertThat(ackCaptor.getValue().getExecutionId()).isEqualTo(EXECUTION_ID); - verify(message).ack(); - } - - @Test - void handleMessage_malformedPayload_leftUnacked() { - // setup - stubPayload(MALFORMED); - - // execution - listener.handleMessage(message); - - // verifications - verify(message, never()).ack(); - verifyNoInteractions(acknowledgeService); - verifyNoInteractions(deliveryTracker); - } - - private void stubPayload(String json) { - when(message.getData()).thenReturn(json.getBytes(UTF_8)); - } -} diff --git a/openframe-client-core/src/test/java/com/openframe/client/service/InstalledAgentServiceTest.java b/openframe-client-core/src/test/java/com/openframe/client/service/InstalledAgentServiceTest.java deleted file mode 100644 index 98603f30f9..0000000000 --- a/openframe-client-core/src/test/java/com/openframe/client/service/InstalledAgentServiceTest.java +++ /dev/null @@ -1,103 +0,0 @@ -package com.openframe.client.service; - -import com.openframe.client.exception.MachineNotFoundException; -import com.openframe.delivery.track.DeliveryTracker; -import com.openframe.data.document.device.Machine; -import com.openframe.data.document.installedagents.InstalledAgent; -import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.data.document.tool.ConnectionStatus; -import com.openframe.data.repository.device.MachineRepository; -import com.openframe.data.repository.installedagents.InstalledAgentRepository; -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.junit.jupiter.api.Assertions.assertThrows; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; -import static org.mockito.Mockito.when; - -@ExtendWith(MockitoExtension.class) -class InstalledAgentServiceTest { - - private static final String MACHINE_ID = "mach-42"; - private static final String AGENT_TYPE = "tactical-agent"; - private static final String OLD_VERSION = "1.0.0"; - private static final String VERSION = "1.2.3"; - - @Mock private InstalledAgentRepository installedAgentRepository; - @Mock private MachineRepository machineRepository; - @Mock private DeliveryTracker deliveryTracker; - - @Captor private ArgumentCaptor installedAgentCaptor; - - @InjectMocks private InstalledAgentService service; - - private Machine machine; - private InstalledAgent existing; - - @BeforeEach - void setUp() { - machine = new Machine(); - machine.setMachineId(MACHINE_ID); - existing = new InstalledAgent(); - existing.setMachineId(MACHINE_ID); - existing.setAgentType(AGENT_TYPE); - existing.setVersion(OLD_VERSION); - existing.setStatus(ConnectionStatus.DISCONNECTED); - } - - @Test - void addInstalledAgent_newAgent_savedAndDeliveryCompleted() { - // setup - when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); - when(installedAgentRepository.findByMachineIdAndAgentType(MACHINE_ID, AGENT_TYPE)).thenReturn(Optional.empty()); - - // execution - service.addInstalledAgent(MACHINE_ID, AGENT_TYPE, VERSION, false); - - // verifications - verify(installedAgentRepository).save(installedAgentCaptor.capture()); - assertThat(installedAgentCaptor.getValue().getVersion()).isEqualTo(VERSION); - assertThat(installedAgentCaptor.getValue().getStatus()).isEqualTo(ConnectionStatus.CONNECTED); - verify(deliveryTracker).complete(DeliveryType.TOOL_INSTALLATION, AGENT_TYPE, MACHINE_ID); - } - - @Test - void addInstalledAgent_existingAgent_versionUpdatedAndDeliveryCompleted() { - // setup - when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(machine)); - when(installedAgentRepository.findByMachineIdAndAgentType(MACHINE_ID, AGENT_TYPE)).thenReturn(Optional.of(existing)); - - // execution - service.addInstalledAgent(MACHINE_ID, AGENT_TYPE, VERSION, false); - - // verifications - assertThat(existing.getVersion()).isEqualTo(VERSION); - assertThat(existing.getStatus()).isEqualTo(ConnectionStatus.CONNECTED); - verify(installedAgentRepository).save(existing); - verify(deliveryTracker).complete(DeliveryType.TOOL_INSTALLATION, AGENT_TYPE, MACHINE_ID); - } - - @Test - void addInstalledAgent_unknownMachine_throwsAndDeliveryUntouched() { - // setup - when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.empty()); - - // execution - MachineNotFoundException ex = assertThrows(MachineNotFoundException.class, - () -> service.addInstalledAgent(MACHINE_ID, AGENT_TYPE, VERSION, false)); - - // verifications - assertThat(ex.getMessage()).contains(MACHINE_ID); - verifyNoInteractions(deliveryTracker); - } -} diff --git a/openframe-data-mongo-common/src/main/java/com/openframe/data/document/delivery/DeliveryFailure.java b/openframe-data-mongo-common/src/main/java/com/openframe/data/document/delivery/DeliveryFailure.java index b9afc5802f..0b81c0ac0a 100644 --- a/openframe-data-mongo-common/src/main/java/com/openframe/data/document/delivery/DeliveryFailure.java +++ b/openframe-data-mongo-common/src/main/java/com/openframe/data/document/delivery/DeliveryFailure.java @@ -4,5 +4,6 @@ public enum DeliveryFailure { EXHAUSTED, OFFLINE, TIMEOUT, - ERROR + ERROR, + AGENT_ERROR } diff --git a/openframe-data-mongo-common/src/main/java/com/openframe/data/document/delivery/MachineDelivery.java b/openframe-data-mongo-common/src/main/java/com/openframe/data/document/delivery/MachineDelivery.java index 6db338f92d..84b584fbaa 100644 --- a/openframe-data-mongo-common/src/main/java/com/openframe/data/document/delivery/MachineDelivery.java +++ b/openframe-data-mongo-common/src/main/java/com/openframe/data/document/delivery/MachineDelivery.java @@ -18,7 +18,6 @@ @AllArgsConstructor @Document(collection = "machine_delivery") @CompoundIndex(name = "machine_delivery_due", def = "{'tenantId': 1, 'status': 1, 'dueAt': 1}") -@CompoundIndex(name = "machine_delivery_machine", def = "{'tenantId': 1, 'machineId': 1}") public class MachineDelivery implements TenantScoped { @Id @@ -37,11 +36,11 @@ public class MachineDelivery implements TenantScoped { private Instant dispatchedAt; private Instant dueAt; - private boolean parked; private Instant ackedAt; private Instant finishedAt; private DeliveryFailure failure; + private String error; @Indexed(name = "machine_delivery_ttl", expireAfterSeconds = 0) private Instant expiresAt; 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 86f9d99edb..8b6c14da93 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 @@ -20,17 +20,16 @@ public interface CustomMachineDeliveryRepository { boolean postponeAfterError(String id, Set from, Instant dispatchedAt, Instant dueAt); - boolean park(String id, Set from, Instant dispatchedAt, Instant dueAt); boolean markAcked(String id, String dispatchId, Set from, Instant ackedAt, Instant dueAt); - boolean markDone(String id, Set from, Instant finishedAt, Instant expiresAt); + boolean markDone(String id, String dispatchId, 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); + boolean markFailed(String id, String dispatchId, Set from, DeliveryFailure failure, String error, Instant finishedAt, Instant expiresAt); - long wake(String machineId, Set from, Instant dueAt); } 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 8f7b495a5d..e6d379bd2d 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 @@ -25,7 +25,6 @@ public class CustomMachineDeliveryRepositoryImpl extends TenantAwareRepositorySu private static final String FIELD_ID = "_id"; private static final String FIELD_TENANT_ID = "tenantId"; - private static final String FIELD_MACHINE_ID = "machineId"; private static final String FIELD_STATUS = "status"; private static final String FIELD_ATTEMPTS = "attempts"; private static final String FIELD_ERRORS = "errors"; @@ -33,11 +32,11 @@ public class CustomMachineDeliveryRepositoryImpl extends TenantAwareRepositorySu private static final String FIELD_PAYLOAD_JSON = "payloadJson"; private static final String FIELD_DISPATCHED_AT = "dispatchedAt"; private static final String FIELD_DUE_AT = "dueAt"; - private static final String FIELD_PARKED = "parked"; private static final String FIELD_ACKED_AT = "ackedAt"; private static final String FIELD_FINISHED_AT = "finishedAt"; private static final String FIELD_EXPIRES_AT = "expiresAt"; private static final String FIELD_FAILURE = "failure"; + private static final String FIELD_ERROR = "error"; private static final Sort OLDEST_DUE_FIRST = Sort.by(FIELD_DUE_AT); @@ -91,29 +90,19 @@ public boolean postponeAfterError(String id, Set from, Instant d return updateOne(sameDispatch(id, from, dispatchedAt), update); } - @Override - public boolean park(String id, Set from, Instant dispatchedAt, Instant dueAt) { - Update update = new Update() - .set(FIELD_DUE_AT, dueAt) - .set(FIELD_PARKED, true); - return updateOne(sameDispatch(id, from, dispatchedAt), update); - } - @Override public boolean markAcked(String id, String dispatchId, Set from, Instant ackedAt, Instant dueAt) { - Criteria thisDispatch = stillIn(id, from).and(FIELD_DISPATCH_ID).is(dispatchId); Update update = new Update() .set(FIELD_STATUS, DeliveryStatus.ACKED) .set(FIELD_ACKED_AT, ackedAt) - .set(FIELD_DUE_AT, dueAt) - .set(FIELD_PARKED, false); - return updateOne(thisDispatch, update); + .set(FIELD_DUE_AT, dueAt); + return updateOne(thisDispatch(id, from, dispatchId), update); } @Override - public boolean markDone(String id, Set from, Instant finishedAt, Instant expiresAt) { + public boolean markDone(String id, String dispatchId, Set from, Instant finishedAt, Instant expiresAt) { Update update = closed(DeliveryStatus.DONE, finishedAt, expiresAt); - return updateOne(stillIn(id, from), update); + return updateOne(thisDispatch(id, from, dispatchId), update); } @Override @@ -136,16 +125,11 @@ public boolean markFailed(String id, Set from, Instant dispatche } @Override - public long wake(String machineId, Set from, Instant dueAt) { - Criteria parkedRowsOfMachine = Criteria.where(FIELD_MACHINE_ID).is(machineId) - .and(FIELD_STATUS).in(from) - .and(FIELD_PARKED).is(true); - Query query = new Query(parkedRowsOfMachine); - Update update = new Update() - .set(FIELD_DUE_AT, dueAt) - .set(FIELD_PARKED, false); - UpdateResult result = mongoTemplate.updateMulti(query, update, MachineDelivery.class); - return result.getModifiedCount(); + public boolean markFailed(String id, String dispatchId, Set from, DeliveryFailure failure, String error, Instant finishedAt, Instant expiresAt) { + Update update = closed(DeliveryStatus.FAILED, finishedAt, expiresAt) + .set(FIELD_FAILURE, failure) + .set(FIELD_ERROR, error); + return updateOne(thisDispatch(id, from, dispatchId), update); } private static void setField(Update update, String field, Object value) { @@ -171,6 +155,10 @@ private static Criteria sameDispatch(String id, Set from, Instan return stillIn(id, from).and(FIELD_DISPATCHED_AT).is(dispatchedAt); } + private static Criteria thisDispatch(String id, Set from, String dispatchId) { + return stillIn(id, from).and(FIELD_DISPATCH_ID).is(dispatchId); + } + private boolean updateOne(Criteria criteria, Update update) { Query query = new Query(criteria); UpdateResult result = mongoTemplate.updateFirst(query, update, MachineDelivery.class); 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 ddc51d1718..a22ef9d5c1 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 @@ -34,6 +34,7 @@ class CustomMachineDeliveryRepositoryImplTest { private static final int LIMIT = 500; private static final int ATTEMPTS = 1; private static final String DISPATCH_ID = "d-1"; + private static final String ERROR = "download failed"; @Mock private TenantAwareMongoTemplate mongoTemplate; @Mock private MongoConverter converter; @@ -138,8 +139,7 @@ void markAcked_unackedRowOfThisDispatch_ackedAndTrue() { assertThat(updateCaptor.getValue().getUpdateObject().toString()) .contains("ACKED") .contains("ackedAt") - .contains("dueAt") - .contains("parked=false"); + .contains("dueAt"); } @Test @@ -182,22 +182,25 @@ void markFailed_openRowSameDispatch_failureAndExpiryWrittenPayloadDropped() { } @Test - void wake_parkedRowsOfMachine_unparkedAndModifiedCountReturned() { + void markFailed_openRowOfThisDispatch_agentErrorWrittenPayloadDropped() { // setup - UpdateResult twoRows = UpdateResult.acknowledged(2, 2L, null); - when(mongoTemplate.updateMulti(queryCaptor.capture(), updateCaptor.capture(), eq(MachineDelivery.class))).thenReturn(twoRows); + UpdateResult oneRow = UpdateResult.acknowledged(1, 1L, null); + when(mongoTemplate.updateFirst(queryCaptor.capture(), updateCaptor.capture(), eq(MachineDelivery.class))).thenReturn(oneRow); // execution - long woken = repository.wake(MACHINE_ID, DeliveryStatus.UNACKED, now); + boolean failed = repository.markFailed(ID, DISPATCH_ID, DeliveryStatus.OPEN, DeliveryFailure.AGENT_ERROR, ERROR, now, now); // verifications - assertThat(woken).isEqualTo(2); + assertThat(failed).isTrue(); assertThat(queryCaptor.getValue().getQueryObject().toString()) - .contains(MACHINE_ID) + .contains(ID) + .contains("dispatchId=" + DISPATCH_ID) .contains("PENDING") - .contains("parked=true"); + .contains("ACKED"); assertThat(updateCaptor.getValue().getUpdateObject().toString()) - .contains("parked=false") - .contains("dueAt"); + .contains("FAILED") + .contains("AGENT_ERROR") + .contains("error=" + ERROR) + .contains("$unset"); } } diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/DeliveryResult.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/DeliveryResult.java new file mode 100644 index 0000000000..29922384eb --- /dev/null +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/DeliveryResult.java @@ -0,0 +1,7 @@ +package com.openframe.data.nats.delivery; + +public enum DeliveryResult { + ACKED, + DONE, + FAILED +} diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/DeliveryResultMessage.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/DeliveryResultMessage.java new file mode 100644 index 0000000000..c319c64591 --- /dev/null +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/DeliveryResultMessage.java @@ -0,0 +1,23 @@ +package com.openframe.data.nats.delivery; + +import com.fasterxml.jackson.annotation.JsonFormat; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.openframe.data.document.delivery.DeliveryType; +import lombok.Data; + +@Data +@JsonIgnoreProperties(ignoreUnknown = true) +public class DeliveryResultMessage { + + public static final String STREAM = "DELIVERY_RESULT"; + public static final String SUBJECT_FILTER = "machine.*.delivery.result"; + + // a value this server does not know yet reads as null and is dropped with a warning instead of poisoning the consumer + @JsonFormat(with = JsonFormat.Feature.READ_UNKNOWN_ENUM_VALUES_AS_NULL) + private DeliveryType type; + private String targetId; + private String dispatchId; + @JsonFormat(with = JsonFormat.Feature.READ_UNKNOWN_ENUM_VALUES_AS_NULL) + private DeliveryResult result; + private String error; +} 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 new file mode 100644 index 0000000000..058ba4907e --- /dev/null +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySeed.java @@ -0,0 +1,23 @@ +package com.openframe.data.nats.delivery; + +import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.data.document.tool.IntegratedTool; +import com.openframe.data.document.toolagent.IntegratedToolAgent; +import com.openframe.delivery.spec.DeliverySeed; +import lombok.AllArgsConstructor; +import lombok.Getter; + +@Getter +@AllArgsConstructor +public class ToolInstallationDeliverySeed implements DeliverySeed { + + private final String machineId; + private final IntegratedToolAgent toolAgent; + private final IntegratedTool tool; + private final boolean reinstall; + + @Override + public DeliveryType type() { + return DeliveryType.TOOL_INSTALLATION; + } +} 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 2722dfaf3d..8ec5c643b7 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 @@ -12,10 +12,7 @@ import com.openframe.data.nats.model.ToolInstallationMessage; import com.openframe.data.nats.publisher.NatsMessagePublisher; import com.openframe.delivery.spec.DeliveryRequest; -import com.openframe.delivery.spec.DeliverySeed; import com.openframe.delivery.spec.DeliverySpec; -import lombok.AllArgsConstructor; -import lombok.Getter; import lombok.RequiredArgsConstructor; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.stereotype.Component; @@ -28,7 +25,7 @@ @Component @RequiredArgsConstructor @ConditionalOnProperty("spring.cloud.stream.enabled") -public class ToolInstallationDeliverySpec implements DeliverySpec { +public class ToolInstallationDeliverySpec implements DeliverySpec { private static final String SUBJECT_TEMPLATE = "machine.%s.tool-installation"; @@ -36,20 +33,6 @@ public class ToolInstallationDeliverySpec implements DeliverySpec getPayloadClass() { // targetId must equal the agentType the agent sends in installed-agent, or complete() never finds the row @Override - public DeliveryRequest request(Seed seed) { + public DeliveryRequest request(ToolInstallationDeliverySeed seed) { IntegratedToolAgent toolAgent = seed.getToolAgent(); ToolInstallationMessage message = buildMessage(toolAgent, seed.getTool(), seed.isReinstall()); String targetId = toolAgent.getKey(); diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/rmm/model/ScriptExecutionAcknowledgeMessage.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/rmm/model/ScriptExecutionAcknowledgeMessage.java index d446ac1a28..9100972645 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/rmm/model/ScriptExecutionAcknowledgeMessage.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/rmm/model/ScriptExecutionAcknowledgeMessage.java @@ -1,6 +1,5 @@ package com.openframe.data.nats.rmm.model; -import com.openframe.data.document.delivery.DeliveryType; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import lombok.AllArgsConstructor; import lombok.Builder; @@ -20,8 +19,4 @@ public class ScriptExecutionAcknowledgeMessage { private String machineId; private String scheduleId; private List scriptIds; - - private DeliveryType type; - private String targetId; - private String dispatchId; } diff --git a/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpecTest.java b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpecTest.java index 0f6c32a3cf..782e522bc0 100644 --- a/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpecTest.java +++ b/openframe-data-nats/src/test/java/com/openframe/data/nats/delivery/ToolInstallationDeliverySpecTest.java @@ -55,7 +55,7 @@ void setUp() { void request_toolAgent_messageBuiltAndTargetIsAgentKey() { // setup when(downloadConfigurationMapper.map(null, VERSION)).thenReturn(List.of()); - ToolInstallationDeliverySpec.Seed seed = new ToolInstallationDeliverySpec.Seed(MACHINE_ID, toolAgent, tool, true); + ToolInstallationDeliverySeed seed = new ToolInstallationDeliverySeed(MACHINE_ID, toolAgent, tool, true); // execution DeliveryRequest request = spec.request(seed); @@ -79,7 +79,7 @@ void request_toolWithoutIdAndType_emptyStringsNotNulls() { toolAgent.setToolId(null); tool.setToolType(null); when(downloadConfigurationMapper.map(null, VERSION)).thenReturn(List.of()); - ToolInstallationDeliverySpec.Seed seed = new ToolInstallationDeliverySpec.Seed(MACHINE_ID, toolAgent, tool, false); + ToolInstallationDeliverySeed seed = new ToolInstallationDeliverySeed(MACHINE_ID, toolAgent, tool, false); // execution DeliveryRequest request = spec.request(seed); 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 812ccafbe1..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 @@ -13,7 +13,6 @@ import java.util.EnumMap; import java.util.Map; -import java.util.Optional; import static java.lang.Boolean.FALSE; import static java.util.Objects.requireNonNullElse; @@ -39,18 +38,10 @@ public class DeliveryProperties { // a type not listed here is off: every environment switches each type on explicitly private Map enabled = new EnumMap<>(DeliveryType.class); - // first agent version that acks the type; a type not listed here keeps every machine on the old path - private Map minAgentVersion = new EnumMap<>(DeliveryType.class); - public boolean isEnabled(DeliveryType type) { return enabled.getOrDefault(type, FALSE); } - public Optional minAgentVersion(DeliveryType type) { - String version = minAgentVersion.get(type); - return Optional.ofNullable(version); - } - public Policy resolve(DeliveryType type) { Policy override = types.get(type); if (override == null) { @@ -63,6 +54,9 @@ public Policy resolve(DeliveryType type) { @Setter public static class Sweep { + @NotNull + @Positive + private Long interval; @NotNull @Positive private Integer batchSize; diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/AgentVersion.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/AgentVersion.java deleted file mode 100644 index 8bf7b4105f..0000000000 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/AgentVersion.java +++ /dev/null @@ -1,42 +0,0 @@ -package com.openframe.delivery.dispatch; - -import lombok.experimental.UtilityClass; -import org.springframework.util.StringUtils; - -import java.util.Arrays; -import java.util.regex.Pattern; - -@UtilityClass -class AgentVersion { - - private final Pattern NON_DIGITS = Pattern.compile("\\D+"); - - boolean isAtLeast(String version, String minimum) { - long[] actual = numbers(version); - long[] required = numbers(minimum); - int length = Math.max(actual.length, required.length); - for (int i = 0; i < length; i++) { - long left = numberAt(actual, i); - long right = numberAt(required, i); - if (left != right) { - return left > right; - } - } - return true; - } - - private long[] numbers(String version) { - String[] tokens = NON_DIGITS.split(version); - return Arrays.stream(tokens) - .filter(StringUtils::hasText) - .mapToLong(Long::parseLong) - .toArray(); - } - - private long numberAt(long[] numbers, int index) { - if (index >= numbers.length) { - return 0; - } - return numbers[index]; - } -} diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryGate.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryGate.java deleted file mode 100644 index e1633a5bde..0000000000 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryGate.java +++ /dev/null @@ -1,39 +0,0 @@ -package com.openframe.delivery.dispatch; - -import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.data.document.installedagents.InstalledAgent; -import com.openframe.data.repository.installedagents.InstalledAgentRepository; -import com.openframe.delivery.config.DeliveryProperties; -import lombok.RequiredArgsConstructor; -import org.springframework.stereotype.Component; -import org.springframework.util.StringUtils; - -import java.util.Optional; - -@Component -@RequiredArgsConstructor -public class DeliveryGate { - - private static final String OPENFRAME_CLIENT_AGENT_TYPE = "openframe-client"; - - private final DeliveryProperties properties; - private final InstalledAgentRepository installedAgentRepository; - - public boolean isOpen(DeliveryType type, String machineId) { - if (!properties.isEnabled(type)) { - return false; - } - Optional minAgentVersion = properties.minAgentVersion(type); - return minAgentVersion - .map(minimum -> isAgentAtLeast(machineId, minimum)) - .orElse(false); - } - - private boolean isAgentAtLeast(String machineId, String minimum) { - return installedAgentRepository.findByMachineIdAndAgentType(machineId, OPENFRAME_CLIENT_AGENT_TYPE) - .map(InstalledAgent::getVersion) - .filter(StringUtils::hasText) - .map(version -> AgentVersion.isAtLeast(version, minimum)) - .orElse(false); - } -} 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 47c91e44ea..aa7ce270f9 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 @@ -9,7 +9,9 @@ import com.openframe.data.document.delivery.MachineDelivery; import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryProperties; +import com.openframe.delivery.track.DeliveryCloser; import com.openframe.delivery.config.DeliveryProperties.Policy; +import com.openframe.delivery.config.DeliveryProperties.Sweep; import com.openframe.delivery.metrics.DeliveryMetrics; import com.openframe.delivery.spec.DeliveryPayload; import com.openframe.delivery.spec.DeliverySeed; @@ -92,13 +94,14 @@ private void parkSkipOrFailOffline(MachineDelivery delivery, Policy policy, Inst closer.fail(delivery, DeliveryFailure.OFFLINE, DeliveryStatus.UNACKED, now); return; } - long recheckSeconds = policy.getMaxRetryIntervalSeconds(); - Instant recheckAt = now.plusSeconds(recheckSeconds); + Sweep sweep = properties.getSweep(); + long recheckMillis = sweep.getInterval(); + Instant recheckAt = now.plusMillis(recheckMillis); Instant dueAt = earliest(windowEnd, recheckAt); String id = delivery.getId(); Instant dispatchedAt = delivery.getDispatchedAt(); - repository.park(id, DeliveryStatus.UNACKED, dispatchedAt, dueAt); - log.debug("Delivery parked, machine not online: id={} dueAt={} windowEnd={}", id, dueAt, windowEnd); + repository.postpone(id, DeliveryStatus.UNACKED, dispatchedAt, dueAt); + log.debug("Delivery waits for the machine to come online: id={} dueAt={} windowEnd={}", id, dueAt, windowEnd); } private void republish(MachineDelivery delivery, Policy policy, Instant now) { diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryWatchdogService.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryWatchdogService.java index 266651d23c..da8dc168ed 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryWatchdogService.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryWatchdogService.java @@ -6,6 +6,7 @@ import com.openframe.data.document.delivery.MachineDelivery; import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryProperties; +import com.openframe.delivery.track.DeliveryCloser; import com.openframe.delivery.config.DeliveryProperties.Policy; import com.openframe.delivery.metrics.DeliveryMetrics; import lombok.RequiredArgsConstructor; diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryCloser.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryCloser.java similarity index 76% rename from openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryCloser.java rename to openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryCloser.java index bd9cc05128..b4e4c85bd2 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliveryCloser.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/track/DeliveryCloser.java @@ -1,4 +1,4 @@ -package com.openframe.delivery.sweep; +package com.openframe.delivery.track; import com.openframe.data.document.delivery.DeliveryFailure; import com.openframe.data.document.delivery.DeliveryStatus; @@ -14,7 +14,6 @@ import com.openframe.delivery.spec.DeliverySpecRegistry; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; -import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.stereotype.Component; import java.time.Instant; @@ -24,7 +23,6 @@ @Slf4j @Component @RequiredArgsConstructor -@ConditionalOnProperty(name = "openframe.delivery.sweep.enabled", havingValue = "true") public class DeliveryCloser { private final MachineDeliveryRepository repository; @@ -54,6 +52,20 @@ public void fail(MachineDelivery delivery, DeliveryFailure failure, Set from, String reason, Instant now) { DeliveryType type = delivery.getType(); Instant expiresAt = expiresAt(type, now); @@ -66,6 +78,10 @@ public void cancel(MachineDelivery delivery, 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); 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 f7fe0d44d2..9efa2068f7 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 @@ -18,6 +18,7 @@ public class DeliveryTracker { private final MachineDeliveryRepository repository; private final DeliveryProperties properties; + private final DeliveryCloser closer; public void acknowledge(DeliveryType type, String targetId, String machineId, String dispatchId) { String id = DeliveryId.of(type, targetId, machineId); @@ -33,16 +34,23 @@ public void acknowledge(DeliveryType type, String targetId, String machineId, St } } - public void complete(DeliveryType type, String targetId, String machineId) { + public void complete(DeliveryType type, String targetId, String machineId, String dispatchId) { 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); + boolean done = repository.markDone(id, dispatchId, DeliveryStatus.COMPLETABLE, now, expiresAt); if (done) { - log.info("Delivery DONE: type={} targetId={} machineId={}", type, targetId, machineId); + log.info("Delivery DONE: type={} targetId={} machineId={} dispatchId={}", type, targetId, machineId, dispatchId); + } else { + log.debug("Delivery completion ignored, no row for this dispatch: id={} dispatchId={}", id, dispatchId); } } + 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); + } + public void cancel(DeliveryType type, String targetId, String machineId) { String id = DeliveryId.of(type, targetId, machineId); Instant now = Instant.now(); @@ -53,14 +61,6 @@ public void cancel(DeliveryType type, String targetId, String machineId) { } } - public void wake(String machineId) { - Instant now = Instant.now(); - long woken = repository.wake(machineId, DeliveryStatus.UNACKED, now); - if (woken > 0) { - log.info("Delivery rows woken for retry: machineId={} count={}", machineId, woken); - } - } - 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/config/DeliveryPropertiesTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryPropertiesTest.java index b6dee8b9db..1ef4bfc446 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryPropertiesTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryPropertiesTest.java @@ -7,7 +7,6 @@ import org.junit.jupiter.api.Test; import java.util.Map; -import java.util.Optional; import static com.openframe.delivery.config.DeliveryTestPolicies.ACK_THRESHOLD; import static com.openframe.delivery.config.DeliveryTestPolicies.BACKOFF_MULTIPLIER; @@ -20,7 +19,6 @@ class DeliveryPropertiesTest { private static final int UNINSTALL_MAX_ATTEMPTS = 5; - private static final String MIN_AGENT_VERSION = "1.4.0"; private DeliveryProperties properties; @@ -99,30 +97,6 @@ void isEnabled_typeListedOff_false() { assertThat(enabled).isFalse(); } - @Test - void minAgentVersion_typeNotListed_empty() { - // setup - properties.setMinAgentVersion(Map.of()); - - // execution - Optional minAgentVersion = properties.minAgentVersion(DeliveryType.TOOL_INSTALLATION); - - // verifications - assertThat(minAgentVersion).isEmpty(); - } - - @Test - void minAgentVersion_typeListed_versionReturned() { - // setup - properties.setMinAgentVersion(Map.of(DeliveryType.TOOL_INSTALLATION, MIN_AGENT_VERSION)); - - // execution - Optional minAgentVersion = properties.minAgentVersion(DeliveryType.TOOL_INSTALLATION); - - // verifications - assertThat(minAgentVersion).contains(MIN_AGENT_VERSION); - } - @Test void isEnabled_typeListedOn_true() { // setup diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryTestPolicies.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryTestPolicies.java index d97faf54e5..ba4f55c48b 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryTestPolicies.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/config/DeliveryTestPolicies.java @@ -11,6 +11,7 @@ public final class DeliveryTestPolicies { public static final int BACKOFF_MULTIPLIER = 2; public static final long MAX_RETRY_INTERVAL = 300L; public static final int BATCH_SIZE = 500; + public static final long SWEEP_INTERVAL_MILLIS = 30_000L; public static final long RECONNECT_WINDOW = 86_400L; public static final long RESULT_TIMEOUT = 600L; public static final long TTL = 604_800L; @@ -29,6 +30,7 @@ public static DeliveryProperties properties() { defaults.setResultTimeoutSeconds(RESULT_TIMEOUT); defaults.setTtlSeconds(TTL); Sweep sweep = new Sweep(); + sweep.setInterval(SWEEP_INTERVAL_MILLIS); sweep.setBatchSize(BATCH_SIZE); DeliveryProperties properties = new DeliveryProperties(); properties.setDefaults(defaults); diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/AgentVersionTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/AgentVersionTest.java deleted file mode 100644 index 18168d07d9..0000000000 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/AgentVersionTest.java +++ /dev/null @@ -1,32 +0,0 @@ -package com.openframe.delivery.dispatch; - -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.CsvSource; - -import static org.assertj.core.api.Assertions.assertThat; - -class AgentVersionTest { - - @ParameterizedTest - @CsvSource({ - "1.4.0, 1.4.0, true", - "1.4.1, 1.4.0, true", - "1.10.0, 1.9.9, true", - "1.4, 1.4.0, true", - "1.4.0.1, 1.4.0, true", - "v1.4.0, 1.4.0, true", - "2.0.0, 1.4.0, true", - "1.3.9, 1.4.0, false", - "0.9.99, 1.0.0, false", - "1.4, 1.4.1, false" - }) - void isAtLeast_versionAgainstMinimum_numericSegmentsDecide(String version, String minimum, boolean expected) { - // setup - - // execution - boolean atLeast = AgentVersion.isAtLeast(version, minimum); - - // verifications - assertThat(atLeast).isEqualTo(expected); - } -} diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryGateTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryGateTest.java deleted file mode 100644 index 8c51d5cfc5..0000000000 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryGateTest.java +++ /dev/null @@ -1,125 +0,0 @@ -package com.openframe.delivery.dispatch; - -import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.data.document.installedagents.InstalledAgent; -import com.openframe.data.repository.installedagents.InstalledAgentRepository; -import com.openframe.delivery.config.DeliveryProperties; -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.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; - -import java.util.Map; -import java.util.Optional; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.verifyNoInteractions; -import static org.mockito.Mockito.when; - -@ExtendWith(MockitoExtension.class) -class DeliveryGateTest { - - private static final String MACHINE_ID = "mach-42"; - private static final String CLIENT_AGENT_TYPE = "openframe-client"; - private static final String MIN_VERSION = "1.4.0"; - - @Mock private InstalledAgentRepository installedAgentRepository; - - private DeliveryGate gate; - - private DeliveryProperties properties; - private InstalledAgent client; - - @BeforeEach - void setUp() { - properties = DeliveryTestPolicies.properties(); - properties.setEnabled(Map.of(DeliveryType.TOOL_INSTALLATION, true)); - properties.setMinAgentVersion(Map.of(DeliveryType.TOOL_INSTALLATION, MIN_VERSION)); - client = new InstalledAgent(); - client.setMachineId(MACHINE_ID); - client.setAgentType(CLIENT_AGENT_TYPE); - gate = new DeliveryGate(properties, installedAgentRepository); - } - - @Test - void isOpen_typeOff_falseWithoutLookup() { - // setup - properties.setEnabled(Map.of()); - - // execution - boolean open = gate.isOpen(DeliveryType.TOOL_INSTALLATION, MACHINE_ID); - - // verifications - assertThat(open).isFalse(); - verifyNoInteractions(installedAgentRepository); - } - - @Test - void isOpen_typeOnWithoutMinAgentVersion_falseWithoutLookup() { - // setup - properties.setMinAgentVersion(Map.of()); - - // execution - boolean open = gate.isOpen(DeliveryType.TOOL_INSTALLATION, MACHINE_ID); - - // verifications - assertThat(open).isFalse(); - verifyNoInteractions(installedAgentRepository); - } - - @Test - void isOpen_clientAgentRowMissing_false() { - // setup - when(installedAgentRepository.findByMachineIdAndAgentType(MACHINE_ID, CLIENT_AGENT_TYPE)).thenReturn(Optional.empty()); - - // execution - boolean open = gate.isOpen(DeliveryType.TOOL_INSTALLATION, MACHINE_ID); - - // verifications - assertThat(open).isFalse(); - } - - @Test - void isOpen_clientAgentVersionBlank_false() { - // setup - client.setVersion(" "); - when(installedAgentRepository.findByMachineIdAndAgentType(MACHINE_ID, CLIENT_AGENT_TYPE)).thenReturn(Optional.of(client)); - - // execution - boolean open = gate.isOpen(DeliveryType.TOOL_INSTALLATION, MACHINE_ID); - - // verifications - assertThat(open).isFalse(); - } - - @Test - void isOpen_clientAgentBelowMinimum_false() { - // setup - client.setVersion("1.3.9"); - when(installedAgentRepository.findByMachineIdAndAgentType(MACHINE_ID, CLIENT_AGENT_TYPE)).thenReturn(Optional.of(client)); - - // execution - boolean open = gate.isOpen(DeliveryType.TOOL_INSTALLATION, MACHINE_ID); - - // verifications - assertThat(open).isFalse(); - } - - @ParameterizedTest - @ValueSource(strings = {"1.4.0", "1.4.1", "1.10.0", "2.0.0"}) - void isOpen_clientAgentAtOrAboveMinimum_true(String version) { - // setup - client.setVersion(version); - when(installedAgentRepository.findByMachineIdAndAgentType(MACHINE_ID, CLIENT_AGENT_TYPE)).thenReturn(Optional.of(client)); - - // execution - boolean open = gate.isOpen(DeliveryType.TOOL_INSTALLATION, MACHINE_ID); - - // verifications - assertThat(open).isTrue(); - } -} 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 81cba4bf96..a61a916249 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 @@ -8,6 +8,7 @@ import com.openframe.data.document.delivery.MachineDelivery; import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryProperties; +import com.openframe.delivery.track.DeliveryCloser; import com.openframe.delivery.config.DeliveryTestPolicies; import com.openframe.delivery.metrics.DeliveryMetrics; import com.openframe.delivery.spec.DeliverySpec; @@ -33,6 +34,7 @@ import static com.openframe.delivery.config.DeliveryTestPolicies.MAX_ATTEMPTS; import static com.openframe.delivery.config.DeliveryTestPolicies.MAX_RETRY_INTERVAL; import static com.openframe.delivery.config.DeliveryTestPolicies.RECONNECT_WINDOW; +import static com.openframe.delivery.config.DeliveryTestPolicies.SWEEP_INTERVAL_MILLIS; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; @@ -52,7 +54,7 @@ class DeliverySweepServiceTest { private static final String PAYLOAD_JSON = "{\"value\":\"fleetmdm-agent\"}"; private static final String CORRUPT_JSON = "not-json"; private static final long TWO_DAYS_SECONDS = 172_800L; - private static final long ONE_MINUTE_SECONDS = 60L; + private static final long TEN_SECONDS = 10L; private static final long FIRST_RETRY_DELAY = ACK_THRESHOLD * BACKOFF_MULTIPLIER; private static final long CLOCK_SLACK_SECONDS = 5L; private static final int MANY_ATTEMPTS_ALLOWED = 10; @@ -216,7 +218,7 @@ void retryPending_onlineAttemptsExhausted_failedExhaustedOnlyIfStillUnacked() { } @Test - void retryPending_offlineFarFromWindowEnd_parkedUntilNextRecheck() { + void retryPending_offlineFarFromWindowEnd_postponedToNextSweep() { // setup Instant before = Instant.now(); stubDue(delivery); @@ -226,17 +228,18 @@ void retryPending_offlineFarFromWindowEnd_parkedUntilNextRecheck() { service.retryPending(); // verifications - verify(repository).park(eq(delivery.getId()), eq(DeliveryStatus.UNACKED), eq(dispatchedAt), dueAtCaptor.capture()); + verify(repository).postpone(eq(delivery.getId()), eq(DeliveryStatus.UNACKED), eq(dispatchedAt), dueAtCaptor.capture()); + Instant nextSweep = before.plusMillis(SWEEP_INTERVAL_MILLIS); assertThat(dueAtCaptor.getValue()) - .isAfterOrEqualTo(before.plusSeconds(MAX_RETRY_INTERVAL)) - .isBefore(before.plusSeconds(MAX_RETRY_INTERVAL + CLOCK_SLACK_SECONDS)); + .isAfterOrEqualTo(nextSweep) + .isBefore(nextSweep.plusSeconds(CLOCK_SLACK_SECONDS)); verifyNoInteractions(registry, closer, metrics); } @Test - void retryPending_offlineCloseToWindowEnd_parkedUntilWindowEnd() { + void retryPending_offlineCloseToWindowEnd_postponedToWindowEnd() { // setup - Instant recently = Instant.now().minusSeconds(RECONNECT_WINDOW - ONE_MINUTE_SECONDS); + Instant recently = Instant.now().minusSeconds(RECONNECT_WINDOW - TEN_SECONDS); delivery.setDispatchedAt(recently); stubDue(delivery); stubMachineNotOnline(); @@ -245,7 +248,7 @@ void retryPending_offlineCloseToWindowEnd_parkedUntilWindowEnd() { service.retryPending(); // verifications - verify(repository).park(delivery.getId(), DeliveryStatus.UNACKED, recently, recently.plusSeconds(RECONNECT_WINDOW)); + verify(repository).postpone(delivery.getId(), DeliveryStatus.UNACKED, recently, recently.plusSeconds(RECONNECT_WINDOW)); } @Test @@ -261,7 +264,7 @@ void retryPending_offlineReconnectWindowOver_failedOffline() { // verifications verify(closer).fail(eq(delivery), eq(DeliveryFailure.OFFLINE), eq(DeliveryStatus.UNACKED), any(Instant.class)); - verify(repository, never()).park(eq(delivery.getId()), eq(DeliveryStatus.UNACKED), eq(twoDaysAgo), any(Instant.class)); + verify(repository, never()).postpone(eq(delivery.getId()), eq(DeliveryStatus.UNACKED), eq(twoDaysAgo), any(Instant.class)); } @Test diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliveryWatchdogServiceTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliveryWatchdogServiceTest.java index dae8b5c9c4..a2f5daa077 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliveryWatchdogServiceTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliveryWatchdogServiceTest.java @@ -6,6 +6,7 @@ import com.openframe.data.document.delivery.MachineDelivery; import com.openframe.data.repository.delivery.MachineDeliveryRepository; import com.openframe.delivery.config.DeliveryProperties; +import com.openframe.delivery.track.DeliveryCloser; import com.openframe.delivery.config.DeliveryTestPolicies; import com.openframe.delivery.metrics.DeliveryMetrics; import org.junit.jupiter.api.BeforeEach; diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliveryCloserTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryCloserTest.java similarity index 77% rename from openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliveryCloserTest.java rename to openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryCloserTest.java index 760d75840e..a448ba4338 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/sweep/DeliveryCloserTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/track/DeliveryCloserTest.java @@ -1,4 +1,4 @@ -package com.openframe.delivery.sweep; +package com.openframe.delivery.track; import com.openframe.data.document.delivery.DeliveryFailure; import com.openframe.data.document.delivery.DeliveryStatus; @@ -33,6 +33,10 @@ class DeliveryCloserTest { private static final String DELIVERY_ID = "CLIENT_UNINSTALL:openframe-client:mach-42"; private static final String REASON = "machine gone"; + private static final String TARGET_ID = "openframe-client"; + private static final String MACHINE_ID = "mach-42"; + private static final String DISPATCH_ID = "d-1"; + private static final String ERROR = "download failed"; @Mock private MachineDeliveryRepository repository; @Mock private DeliverySpecRegistry registry; @@ -109,6 +113,33 @@ void fail_typeWithoutSpec_rowStillFailedSpecSkipped() { verifyNoInteractions(spec); } + @Test + void failReported_openRowOfThisDispatch_rowFailedMetricCountedSpecNotified() { + // 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); + } + + @Test + void failReported_rowOfAnotherDispatchOrClosed_nothingRecorded() { + // setup + when(repository.markFailed(DELIVERY_ID, DISPATCH_ID, DeliveryStatus.OPEN, DeliveryFailure.AGENT_ERROR, ERROR, now, now.plusSeconds(TTL))).thenReturn(false); + + // execution + closer.failReported(DeliveryType.CLIENT_UNINSTALL, TARGET_ID, MACHINE_ID, DISPATCH_ID, ERROR, now); + + // verifications + verifyNoInteractions(metrics, registry, spec); + } + @Test void cancel_rowStillUnackedSameDispatch_rowCancelledWithTtlExpiry() { // setup 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 151cd1d23e..c9adc32d52 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 @@ -28,10 +28,11 @@ class DeliveryTrackerTest { private static final String MACHINE_ID = "mach-42"; private static final String TARGET_ID = "fleetmdm-agent"; private static final String DELIVERY_ID = DeliveryId.of(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID); - private static final long TWO_ROWS = 2L; private static final String DISPATCH_ID = "d-1"; + private static final String ERROR = "download failed"; @Mock private MachineDeliveryRepository repository; + @Mock private DeliveryCloser closer; @Captor private ArgumentCaptor atCaptor; @Captor private ArgumentCaptor untilCaptor; @@ -40,7 +41,7 @@ class DeliveryTrackerTest { @BeforeEach void setUp() { - tracker = new DeliveryTracker(repository, DeliveryTestPolicies.properties()); + tracker = new DeliveryTracker(repository, DeliveryTestPolicies.properties(), closer); } @Test @@ -59,10 +60,10 @@ void acknowledge_typedKey_unackedRowMarkedAckedWithResultDeadline() { @Test void complete_typedKey_openOrFailedRowMarkedDoneWithTtlExpiry() { // setup - when(repository.markDone(eq(DELIVERY_ID), eq(DeliveryStatus.COMPLETABLE), atCaptor.capture(), untilCaptor.capture())).thenReturn(true); + 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); + tracker.complete(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID, DISPATCH_ID); // verifications Instant finishedAt = atCaptor.getValue(); @@ -83,14 +84,13 @@ void cancel_typedKey_openRowMarkedCancelledWithTtlExpiry() { } @Test - void wake_machineId_unackedRowsOfThatMachineWoken() { + void fail_agentReportedError_closerAsked() { // setup - when(repository.wake(eq(MACHINE_ID), eq(DeliveryStatus.UNACKED), any(Instant.class))).thenReturn(TWO_ROWS); // execution - tracker.wake(MACHINE_ID); + tracker.fail(DeliveryType.TOOL_INSTALLATION, TARGET_ID, MACHINE_ID, DISPATCH_ID, ERROR); // verifications - verify(repository).wake(eq(MACHINE_ID), eq(DeliveryStatus.UNACKED), any(Instant.class)); + verify(closer).failReported(eq(DeliveryType.TOOL_INSTALLATION), eq(TARGET_ID), eq(MACHINE_ID), eq(DISPATCH_ID), eq(ERROR), any(Instant.class)); } } diff --git a/openframe-management-service-core/src/main/java/com/openframe/management/initializer/NatsStreamConfigurationInitializer.java b/openframe-management-service-core/src/main/java/com/openframe/management/initializer/NatsStreamConfigurationInitializer.java index 0b4d087b48..4798f17ddf 100644 --- a/openframe-management-service-core/src/main/java/com/openframe/management/initializer/NatsStreamConfigurationInitializer.java +++ b/openframe-management-service-core/src/main/java/com/openframe/management/initializer/NatsStreamConfigurationInitializer.java @@ -1,5 +1,6 @@ package com.openframe.management.initializer; +import com.openframe.data.nats.delivery.DeliveryResultMessage; import com.openframe.data.nats.rmm.model.PackageManagerMissingMessage; import com.openframe.management.service.NatsStreamManagementService; import io.nats.client.api.RetentionPolicy; @@ -84,6 +85,13 @@ public class NatsStreamConfigurationInitializer implements ApplicationRunner { .storageType(StorageType.File) .retentionPolicy(RetentionPolicy.Limits) .build(), + StreamConfiguration.builder() + .name(DeliveryResultMessage.STREAM) + .subjects(List.of(DeliveryResultMessage.SUBJECT_FILTER)) + .storageType(StorageType.File) + .retentionPolicy(RetentionPolicy.Limits) + .maxAge(Duration.ofDays(1)) + .build(), StreamConfiguration.builder() .name(PackageManagerMissingMessage.STREAM) .subjects(List.of(PackageManagerMissingMessage.SUBJECT_FILTER)) diff --git a/openframe-tool-agent-nats-installation/src/main/java/com/openframe/data/service/ToolInstallationService.java b/openframe-tool-agent-nats-installation/src/main/java/com/openframe/data/service/ToolInstallationService.java index 921657caed..71d53fbf52 100644 --- a/openframe-tool-agent-nats-installation/src/main/java/com/openframe/data/service/ToolInstallationService.java +++ b/openframe-tool-agent-nats-installation/src/main/java/com/openframe/data/service/ToolInstallationService.java @@ -3,9 +3,9 @@ import com.openframe.data.document.tool.IntegratedTool; import com.openframe.data.document.toolagent.IntegratedToolAgent; import com.openframe.data.document.delivery.DeliveryType; -import com.openframe.data.nats.delivery.ToolInstallationDeliverySpec; +import com.openframe.data.nats.delivery.ToolInstallationDeliverySeed; import com.openframe.data.nats.publisher.ToolInstallationNatsPublisher; -import com.openframe.delivery.dispatch.DeliveryGate; +import com.openframe.delivery.config.DeliveryProperties; import com.openframe.delivery.dispatch.DeliveryDispatcher; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; @@ -25,7 +25,7 @@ public class ToolInstallationService { private final IntegratedToolService integratedToolService; private final ToolCommandParamsResolver toolCommandParamsResolver; private final ToolInstallationNatsPublisher toolInstallationNatsPublisher; - private final DeliveryGate deliveryGate; + private final DeliveryProperties deliveryProperties; private final DeliveryDispatcher deliveryDispatcher; public void process(String machineId, IntegratedToolAgent toolAgent) { @@ -72,8 +72,8 @@ private IntegratedTool getIntegratedToolData(String toolId) { private void publish(String machineId, IntegratedToolAgent toolAgent, IntegratedTool tool, boolean reinstall) { - if (deliveryGate.isOpen(DeliveryType.TOOL_INSTALLATION, machineId)) { - ToolInstallationDeliverySpec.Seed seed = new ToolInstallationDeliverySpec.Seed(machineId, toolAgent, tool, reinstall); + if (deliveryProperties.isEnabled(DeliveryType.TOOL_INSTALLATION)) { + ToolInstallationDeliverySeed seed = new ToolInstallationDeliverySeed(machineId, toolAgent, tool, reinstall); deliveryDispatcher.dispatch(seed); return; } From 80d401e60b7000e934dde8c5e4cbb5327dc1448b Mon Sep 17 00:00:00 2001 From: semen-flamingo Date: Wed, 23 Sep 2026 13:17:01 +0200 Subject: [PATCH 3/5] feat(delivery): rejected agent results are errors with a counter An incomplete, unknown or malformed delivery result is an agent contract violation: error log + openframe.delivery.result.rejected{reason} for alerting, then acked so it cannot poison the consumer. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01BnYUpYgKvuMw6hB5VZSimY --- .../delivery/DeliveryResultListener.java | 16 +++++++++++++--- .../delivery/DeliveryResultListenerTest.java | 16 +++++++++++----- .../delivery/metrics/DeliveryMetrics.java | 5 +++++ .../delivery/metrics/DeliveryMetricsTest.java | 13 +++++++++++++ 4 files changed, 42 insertions(+), 8 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 83a35483e3..1a428db101 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 @@ -6,6 +6,7 @@ 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; import com.openframe.delivery.track.DeliveryTracker; import io.nats.client.Connection; import io.nats.client.Message; @@ -20,20 +21,26 @@ @Component public class DeliveryResultListener extends AbstractJetStreamPushListener { + private static final String REJECTED_INCOMPLETE = "incomplete"; + private static final String REJECTED_MALFORMED = "malformed"; + private final ObjectMapper objectMapper; private final NatsTopicMachineIdExtractor machineIdExtractor; private final DeliveryTracker deliveryTracker; + private final DeliveryMetrics metrics; public DeliveryResultListener( Connection natsConnection, ObjectMapper objectMapper, NatsTopicMachineIdExtractor machineIdExtractor, - DeliveryTracker deliveryTracker + DeliveryTracker deliveryTracker, + DeliveryMetrics metrics ) { super(natsConnection); this.objectMapper = objectMapper; this.machineIdExtractor = machineIdExtractor; this.deliveryTracker = deliveryTracker; + this.metrics = metrics; } @Override @@ -69,14 +76,17 @@ protected void handleMessage(Message message) { String machineId = machineIdExtractor.extract(subject); DeliveryResultMessage report = objectMapper.readValue(payload, DeliveryResultMessage.class); if (!isComplete(report)) { - log.warn("Delivery result without type, targetId, dispatchId or result dropped: machineId={} payload={}", machineId, payload); + metrics.recordResultRejected(REJECTED_INCOMPLETE); + log.error("Delivery result rejected, agent violates the contract (type, targetId, dispatchId, result required or unknown): machineId={} payload={}", + machineId, payload); message.ack(); return; } apply(machineId, report); message.ack(); } catch (JsonProcessingException | IllegalArgumentException permanentlyBad) { - log.warn("Dropping malformed delivery result subject={} payload={}", subject, payload, permanentlyBad); + metrics.recordResultRejected(REJECTED_MALFORMED); + log.error("Delivery result rejected, malformed: subject={} payload={}", subject, payload, permanentlyBad); message.ack(); } catch (Exception e) { log.error("Unexpected error processing delivery result: {}", payload, e); 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 15beeaca5c..09030eaa6e 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 @@ -3,6 +3,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.openframe.client.service.NatsTopicMachineIdExtractor; import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.delivery.metrics.DeliveryMetrics; import com.openframe.delivery.track.DeliveryTracker; import io.nats.client.Connection; import io.nats.client.Message; @@ -43,13 +44,14 @@ class DeliveryResultListenerTest { @Mock private Connection natsConnection; @Mock private DeliveryTracker deliveryTracker; + @Mock private DeliveryMetrics metrics; @Mock private Message message; private DeliveryResultListener listener; @BeforeEach void setUp() { - listener = new DeliveryResultListener(natsConnection, new ObjectMapper(), new NatsTopicMachineIdExtractor(), deliveryTracker); + listener = new DeliveryResultListener(natsConnection, new ObjectMapper(), new NatsTopicMachineIdExtractor(), deliveryTracker, metrics); } @Test @@ -92,7 +94,7 @@ void handleMessage_failed_trackerFailsDispatchWithError() { } @Test - void handleMessage_withoutDispatchId_droppedAndAcked() { + void handleMessage_withoutDispatchId_rejectedCountedAndAcked() { // setup stubMessage(WITHOUT_DISPATCH_ID); @@ -101,11 +103,12 @@ void handleMessage_withoutDispatchId_droppedAndAcked() { // verifications verifyNoInteractions(deliveryTracker); + verify(metrics).recordResultRejected("incomplete"); verify(message).ack(); } @Test - void handleMessage_unknownResult_droppedAndAcked() { + void handleMessage_unknownResult_rejectedCountedAndAcked() { // setup stubMessage(UNKNOWN_RESULT); @@ -114,11 +117,12 @@ void handleMessage_unknownResult_droppedAndAcked() { // verifications verifyNoInteractions(deliveryTracker); + verify(metrics).recordResultRejected("incomplete"); verify(message).ack(); } @Test - void handleMessage_unknownType_droppedAndAcked() { + void handleMessage_unknownType_rejectedCountedAndAcked() { // setup stubMessage(UNKNOWN_TYPE); @@ -127,11 +131,12 @@ void handleMessage_unknownType_droppedAndAcked() { // verifications verifyNoInteractions(deliveryTracker); + verify(metrics).recordResultRejected("incomplete"); verify(message).ack(); } @Test - void handleMessage_malformedPayload_droppedAndAcked() { + void handleMessage_malformedPayload_rejectedCountedAndAcked() { // setup stubMessage(MALFORMED); @@ -140,6 +145,7 @@ void handleMessage_malformedPayload_droppedAndAcked() { // verifications verifyNoInteractions(deliveryTracker); + verify(metrics).recordResultRejected("malformed"); verify(message).ack(); } diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/metrics/DeliveryMetrics.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/metrics/DeliveryMetrics.java index fac9b0b27a..ee766fadb5 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/metrics/DeliveryMetrics.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/metrics/DeliveryMetrics.java @@ -17,6 +17,7 @@ public class DeliveryMetrics { private static final String FAILED_COUNTER = "openframe.delivery.failed"; private static final String PUBLISH_FAILED_COUNTER = "openframe.delivery.publish_failed"; private static final String ROW_ERROR_COUNTER = "openframe.delivery.sweep.row_errors"; + private static final String RESULT_REJECTED_COUNTER = "openframe.delivery.result.rejected"; private static final String SWEEP_TIMER = "openframe.delivery.sweep.duration"; private static final String TAG_TYPE = "type"; private static final String TAG_REASON = "reason"; @@ -38,6 +39,10 @@ public void recordFailed(DeliveryType type, DeliveryFailure failure) { meterRegistry.counter(FAILED_COUNTER, TAG_TYPE, typeTag, TAG_REASON, reasonTag).increment(); } + public void recordResultRejected(String reason) { + meterRegistry.counter(RESULT_REJECTED_COUNTER, TAG_REASON, reason).increment(); + } + public void recordPublishFailed(DeliveryType type) { String typeTag = tagValue(type.name()); meterRegistry.counter(PUBLISH_FAILED_COUNTER, TAG_TYPE, typeTag).increment(); diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/metrics/DeliveryMetricsTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/metrics/DeliveryMetricsTest.java index 8311adc353..9bf199b1db 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/metrics/DeliveryMetricsTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/metrics/DeliveryMetricsTest.java @@ -83,6 +83,19 @@ void recordRowError_called_counterIncremented() { assertThat(counter.count()).isEqualTo(1.0); } + @Test + void recordResultRejected_reason_counterTaggedWithReason() { + // setup + + // execution + metrics.recordResultRejected("incomplete"); + + // verifications + Counter counter = registry.find("openframe.delivery.result.rejected").tags("reason", "incomplete").counter(); + assertThat(counter).isNotNull(); + assertThat(counter.count()).isEqualTo(1.0); + } + @Test void recordFailed_typeAndReason_counterTaggedLowercase() { // setup From 4f9f11acd7a865be5b80e729c0f126b4099c9903 Mon Sep 17 00:00:00 2001 From: semen-flamingo Date: Wed, 23 Sep 2026 13:51:17 +0200 Subject: [PATCH 4/5] feat(delivery): stamp commands with a delivery block the agent copies back DeliveryRef {type, targetId, dispatchId} replaces the bare dispatchId in the command body; the result message carries the same block, so the agent reports without knowing types or targets. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01BnYUpYgKvuMw6hB5VZSimY --- .../delivery/DeliveryResultListener.java | 18 ++++++++----- .../delivery/DeliveryResultListenerTest.java | 27 ++++++++++++++----- .../nats/delivery/DeliveryResultMessage.java | 9 +++---- .../nats/model/ToolInstallationMessage.java | 3 ++- .../delivery/dispatch/DeliveryDispatcher.java | 5 +++- .../delivery/dispatch/DeliveryRecorder.java | 4 ++- .../delivery/spec/DeliveryPayload.java | 4 +-- .../openframe/delivery/spec/DeliveryRef.java | 19 +++++++++++++ .../dispatch/DeliveryDispatcherTest.java | 4 ++- .../dispatch/DeliveryRecorderTest.java | 3 ++- .../openframe/delivery/spec/TestPayload.java | 2 +- 11 files changed, 71 insertions(+), 27 deletions(-) create mode 100644 openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryRef.java 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 1a428db101..99a5657f5d 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 @@ -7,6 +7,7 @@ import com.openframe.data.nats.delivery.DeliveryResultMessage; import com.openframe.data.nats.listener.AbstractJetStreamPushListener; 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; @@ -77,7 +78,7 @@ protected void handleMessage(Message message) { DeliveryResultMessage report = objectMapper.readValue(payload, DeliveryResultMessage.class); if (!isComplete(report)) { metrics.recordResultRejected(REJECTED_INCOMPLETE); - log.error("Delivery result rejected, agent violates the contract (type, targetId, dispatchId, result required or unknown): machineId={} payload={}", + log.error("Delivery result rejected, agent violates the contract (delivery block incomplete or result unknown): machineId={} payload={}", machineId, payload); message.ack(); return; @@ -94,9 +95,10 @@ protected void handleMessage(Message message) { } private void apply(String machineId, DeliveryResultMessage report) { - DeliveryType type = report.getType(); - String targetId = report.getTargetId(); - String dispatchId = report.getDispatchId(); + 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.complete(type, targetId, machineId, dispatchId); @@ -105,9 +107,11 @@ private void apply(String machineId, DeliveryResultMessage report) { } private static boolean isComplete(DeliveryResultMessage report) { - return report.getType() != null - && hasText(report.getTargetId()) - && hasText(report.getDispatchId()) + DeliveryRef delivery = report.getDelivery(); + return delivery != null + && delivery.getType() != null + && hasText(delivery.getTargetId()) + && hasText(delivery.getDispatchId()) && report.getResult() != null; } } 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 09030eaa6e..1572ba422a 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 @@ -29,17 +29,18 @@ class DeliveryResultListenerTest { private static final String DISPATCH_ID = "d-1"; private static final String ERROR = "download failed"; private static final String ACKED = - "{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\",\"result\":\"ACKED\"}"; + "{\"delivery\":{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\"},\"result\":\"ACKED\"}"; private static final String DONE = - "{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\",\"result\":\"DONE\"}"; + "{\"delivery\":{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\"},\"result\":\"DONE\"}"; private static final String FAILED = - "{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\",\"result\":\"FAILED\",\"error\":\"download failed\"}"; + "{\"delivery\":{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\"},\"result\":\"FAILED\",\"error\":\"download failed\"}"; private static final String WITHOUT_DISPATCH_ID = - "{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"result\":\"ACKED\"}"; + "{\"delivery\":{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\"},\"result\":\"ACKED\"}"; private static final String UNKNOWN_RESULT = - "{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\",\"result\":\"RETRYING\"}"; + "{\"delivery\":{\"type\":\"TOOL_INSTALLATION\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\"},\"result\":\"RETRYING\"}"; private static final String UNKNOWN_TYPE = - "{\"type\":\"HOLOGRAM\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\",\"result\":\"ACKED\"}"; + "{\"delivery\":{\"type\":\"HOLOGRAM\",\"targetId\":\"fleetmdm-agent\",\"dispatchId\":\"d-1\"},\"result\":\"ACKED\"}"; + private static final String WITHOUT_DELIVERY = "{\"result\":\"ACKED\"}"; private static final String MALFORMED = "not json"; @Mock private Connection natsConnection; @@ -107,6 +108,20 @@ void handleMessage_withoutDispatchId_rejectedCountedAndAcked() { verify(message).ack(); } + @Test + void handleMessage_withoutDeliveryBlock_rejectedCountedAndAcked() { + // setup + stubMessage(WITHOUT_DELIVERY); + + // execution + listener.handleMessage(message); + + // verifications + verifyNoInteractions(deliveryTracker); + verify(metrics).recordResultRejected("incomplete"); + verify(message).ack(); + } + @Test void handleMessage_unknownResult_rejectedCountedAndAcked() { // setup diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/DeliveryResultMessage.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/DeliveryResultMessage.java index c319c64591..e0e3d20752 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/DeliveryResultMessage.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/delivery/DeliveryResultMessage.java @@ -2,7 +2,7 @@ import com.fasterxml.jackson.annotation.JsonFormat; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; -import com.openframe.data.document.delivery.DeliveryType; +import com.openframe.delivery.spec.DeliveryRef; import lombok.Data; @Data @@ -12,11 +12,8 @@ public class DeliveryResultMessage { public static final String STREAM = "DELIVERY_RESULT"; public static final String SUBJECT_FILTER = "machine.*.delivery.result"; - // a value this server does not know yet reads as null and is dropped with a warning instead of poisoning the consumer - @JsonFormat(with = JsonFormat.Feature.READ_UNKNOWN_ENUM_VALUES_AS_NULL) - private DeliveryType type; - private String targetId; - private String dispatchId; + // the `delivery` block of the command, copied back by the agent as is + private DeliveryRef delivery; @JsonFormat(with = JsonFormat.Feature.READ_UNKNOWN_ENUM_VALUES_AS_NULL) private DeliveryResult result; private String error; diff --git a/openframe-data-nats/src/main/java/com/openframe/data/nats/model/ToolInstallationMessage.java b/openframe-data-nats/src/main/java/com/openframe/data/nats/model/ToolInstallationMessage.java index 3fdcdbdf44..79475398c8 100644 --- a/openframe-data-nats/src/main/java/com/openframe/data/nats/model/ToolInstallationMessage.java +++ b/openframe-data-nats/src/main/java/com/openframe/data/nats/model/ToolInstallationMessage.java @@ -2,6 +2,7 @@ import com.fasterxml.jackson.annotation.JsonInclude; import com.openframe.delivery.spec.DeliveryPayload; +import com.openframe.delivery.spec.DeliveryRef; import com.openframe.data.document.toolagent.SessionType; import lombok.Getter; @@ -14,7 +15,7 @@ public class ToolInstallationMessage implements DeliveryPayload { @JsonInclude(JsonInclude.Include.NON_NULL) - private String dispatchId; + private DeliveryRef delivery; private String toolAgentId; private String toolId; 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 48629c4148..1561c28bd9 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 @@ -2,6 +2,7 @@ import com.openframe.data.document.delivery.DeliveryType; import com.openframe.delivery.spec.DeliveryPayload; +import com.openframe.delivery.spec.DeliveryRef; import com.openframe.delivery.spec.DeliveryRequest; import com.openframe.delivery.spec.DeliverySeed; import com.openframe.delivery.spec.DeliverySpec; @@ -26,7 +27,9 @@ public void dispatch(DeliverySeed seed) { DeliveryRequest request = spec.request(seed); DeliveryPayload payload = request.getPayload(); String dispatchId = UUID.randomUUID().toString(); - payload.setDispatchId(dispatchId); + String targetId = request.getTargetId(); + DeliveryRef delivery = new DeliveryRef(type, targetId, dispatchId); + payload.setDelivery(delivery); recorder.record(request); String machineId = request.getMachineId(); spec.publish(machineId, payload); diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryRecorder.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryRecorder.java index 8e3af5b66f..0bb54a30cf 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryRecorder.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryRecorder.java @@ -9,6 +9,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.DeliveryRequest; import com.openframe.delivery.track.DeliveryId; import lombok.RequiredArgsConstructor; @@ -40,7 +41,8 @@ private MachineDelivery pendingRow(DeliveryRequest request) { String machineId = request.getMachineId(); String id = DeliveryId.of(type, targetId, machineId); DeliveryPayload payload = request.getPayload(); - String dispatchId = payload.getDispatchId(); + DeliveryRef delivery = payload.getDelivery(); + String dispatchId = delivery.getDispatchId(); String payloadJson = toJson(payload); Policy policy = properties.resolve(type); long ackThresholdSeconds = policy.getAckThresholdSeconds(); diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryPayload.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryPayload.java index 962a3c4b02..ce1722720b 100644 --- a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryPayload.java +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryPayload.java @@ -2,7 +2,7 @@ public interface DeliveryPayload { - String getDispatchId(); + DeliveryRef getDelivery(); - void setDispatchId(String dispatchId); + void setDelivery(DeliveryRef delivery); } diff --git a/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryRef.java b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryRef.java new file mode 100644 index 0000000000..1f62b71d09 --- /dev/null +++ b/openframe-machine-delivery/src/main/java/com/openframe/delivery/spec/DeliveryRef.java @@ -0,0 +1,19 @@ +package com.openframe.delivery.spec; + +import com.fasterxml.jackson.annotation.JsonFormat; +import com.openframe.data.document.delivery.DeliveryType; +import lombok.AllArgsConstructor; +import lombok.Data; +import lombok.NoArgsConstructor; + +@Data +@NoArgsConstructor +@AllArgsConstructor +public class DeliveryRef { + + // a type this server does not know yet reads as null and the report is rejected instead of poisoning the consumer + @JsonFormat(with = JsonFormat.Feature.READ_UNKNOWN_ENUM_VALUES_AS_NULL) + private DeliveryType type; + private String targetId; + private String dispatchId; +} 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 01d3dfccf9..e6569755a7 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 @@ -60,7 +60,9 @@ void dispatch_seed_dispatchIdSetThenRecordedThenPublishedThroughSpec() { dispatcher.dispatch(seed); // verifications - assertThat(payload.getDispatchId()).isNotBlank(); + assertThat(payload.getDelivery().getType()).isEqualTo(DeliveryType.TOOL_INSTALLATION); + assertThat(payload.getDelivery().getTargetId()).isEqualTo(TARGET_ID); + assertThat(payload.getDelivery().getDispatchId()).isNotBlank(); verify(recorder).record(request); verify(spec).publish(MACHINE_ID, payload); } diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryRecorderTest.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryRecorderTest.java index 2a5fa2d44c..97dde8a894 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryRecorderTest.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/dispatch/DeliveryRecorderTest.java @@ -6,6 +6,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.DeliveryRequest; import com.openframe.delivery.spec.TestPayload; import org.junit.jupiter.api.BeforeEach; @@ -40,7 +41,7 @@ class DeliveryRecorderTest { void setUp() { TestPayload payload = new TestPayload(); payload.setValue(VALUE); - payload.setDispatchId(DISPATCH_ID); + payload.setDelivery(new DeliveryRef(DeliveryType.CLIENT_UNINSTALL, MACHINE_ID, DISPATCH_ID)); request = DeliveryRequest.builder() .type(DeliveryType.CLIENT_UNINSTALL) .targetId(MACHINE_ID) diff --git a/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestPayload.java b/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestPayload.java index e21b1910b8..9f6eeccc7a 100644 --- a/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestPayload.java +++ b/openframe-machine-delivery/src/test/java/com/openframe/delivery/spec/TestPayload.java @@ -5,6 +5,6 @@ @Data public class TestPayload implements DeliveryPayload { - private String dispatchId; + private DeliveryRef delivery; private String value; } From 5be6b4514da0f447415bbe7eb13f3d38a476fe59 Mon Sep 17 00:00:00 2001 From: semen-flamingo Date: Thu, 24 Sep 2026 12:22:21 +0200 Subject: [PATCH 5/5] fix(delivery): re-dispatch clears what the previous dispatch closed the row with Found on the feature tenant: a reinstall reopened the row as PENDING but kept failure=EXHAUSTED and finishedAt from the previous dispatch; null fields are not in the $set, so they are unset explicitly. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01SnaktPhqs3aeuuUHdXCAjX --- .../CustomMachineDeliveryRepositoryImpl.java | 9 +++++++-- ...stomMachineDeliveryRepositoryImplTest.java | 20 +++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) 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 e6d379bd2d..d524096044 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 @@ -55,12 +55,17 @@ public List findDue(DeliveryStatus status, Instant before, int return mongoTemplate.find(query, MachineDelivery.class); } - // $set per field, not a replacement document: a replacement is inserted without the tenant of the scoped filter + // $set per field, not a replacement document: a replacement is inserted without the tenant of the scoped filter; + // null fields are not written, so what a previous dispatch closed the row with has to be unset explicitly @Override public void upsertPending(MachineDelivery delivery) { Document document = new Document(); mongoTemplate.getConverter().write(delivery, document); - Update update = new Update().set(FIELD_TENANT_ID, tenantId()); + Update update = new Update().set(FIELD_TENANT_ID, tenantId()) + .unset(FIELD_ACKED_AT) + .unset(FIELD_FINISHED_AT) + .unset(FIELD_FAILURE) + .unset(FIELD_ERROR); document.forEach((field, value) -> setField(update, field, value)); String id = delivery.getId(); Query byId = new Query(Criteria.where(FIELD_ID).is(id)); 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 a22ef9d5c1..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 @@ -86,6 +86,26 @@ void upsertPending_row_setsEveryFieldAndTenantWithinScopedUpsert() { .contains("tenantId=" + TENANT_ID); } + @Test + void upsertPending_rowClosedByPreviousDispatch_closingFieldsUnset() { + // setup + MachineDelivery delivery = MachineDelivery.builder().id(ID).machineId(MACHINE_ID).build(); + when(mongoTemplate.getConverter()).thenReturn(converter); + when(mongoTemplate.tenantId()).thenReturn(TENANT_ID); + + // execution + repository.upsertPending(delivery); + + // verifications + verify(mongoTemplate).upsert(queryCaptor.capture(), updateCaptor.capture(), eq(MachineDelivery.class)); + assertThat(updateCaptor.getValue().getUpdateObject().toString()) + .contains("$unset") + .contains("ackedAt") + .contains("finishedAt") + .contains("failure") + .contains("error"); + } + @Test void markRepublished_sameDispatchAndAttemptStillPending_attemptCountedAndTrue() { // setup