Implement Script Schedule execution#1480
Conversation
📝 WalkthroughWalkthroughScript schedules now support timing metadata, immediate and periodic execution, repeat-based cursor pagination, shared execution correlation, and result tracking across GraphQL, MongoDB, NATS, Kafka, and stream handlers. ChangesSchedule execution
Script result correlation
Saved-script GraphQL contract
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…n' into feature/script-schedule-execution
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@openframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptDispatchService.java`:
- Around line 178-199: Update ScriptDispatchService’s batch persistence and
publication flow around createBatch and publishScript to establish a retry-safe
outbox or idempotency boundary: persist each machine’s delivery state before
publishing, ensure retries do not duplicate rows or messages, and preserve the
existing executionId across partial failures. Ensure manual schedule cadence
advancement still occurs when publication partially fails, so retries can resume
pending deliveries without creating another execution.
- Around line 118-120: Update runSchedule in ScriptDispatchService to require
the retrieved schedule’s status to be ACTIVE immediately after
scriptScheduleService.get(scheduleId). Reject ARCHIVED and all other non-active
statuses before any validation, dispatch, or rescheduling side effects.
In
`@openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/.ScriptDataFetcher.md`:
- Around line 198-215: Replace the “Running a saved script” guidance to document
the existing runScript mutation rather than the runCommand flow. Use
ScriptDataFetcher.runScript and the script.graphqls runScript schema to show the
supported saved-script dispatch contract, including args and envVars, and remove
the claim that these fields are unavailable or require backend escalation.
In
`@openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/ScriptScheduleDataFetcher.java`:
- Line 69: Update the dependency wiring for ScriptScheduleDataFetcher so
deployments with spring.cloud.stream.enabled disabled can still start. Avoid
unconditional injection of ScriptDispatchService by moving stream-specific
mutation behavior into a conditional component or injecting an optional dispatch
abstraction, while preserving dispatch behavior when streaming is enabled.
- Around line 38-41: Update the affected authentication handling in
ScriptScheduleDataFetcher to inject and use AuthPrincipal, matching the existing
patterns in the class, instead of casting the security principal to Jwt. Apply
this to all referenced usages and remove the now-unused Jwt-based handling.
In
`@openframe-data-mongo-common/src/main/java/com/openframe/data/document/rmm/ScriptExecution.java`:
- Around line 25-27: Add a migration for the legacy script_executions unique
index that enforces (tenantId, executionId, machineId), and drop it during
rollout before relying on the updated
tenant_executionId_machineId_scriptId_unique definition. Ensure the migration
safely handles deployments where the legacy index is absent.
In
`@openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/ScriptScheduleRepository.java`:
- Line 34: Replace
ScriptScheduleRepository.findByStatusAndNextRunAtLessThanEqual with an ordered,
bounded, atomic claim operation. In ScriptScheduleScheduler, remove the
unsupported exactly-once guarantee and avoid relying on a fixed lease for an
unbounded sweep. In ScriptScheduleExecutionService, advance cadence only after
successful completion, persist a durable run/claim before dispatch, and publish
through an outbox or per-target delivery state using an idempotency key. Apply
these changes respectively in
openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/ScriptScheduleRepository.java:34-34,
openframe-management-service-core/src/main/java/com/openframe/management/scheduler/ScriptScheduleScheduler.java:23-24
and :35-38, and
openframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.java:87-95,
:99-115, and :141-174.
In
`@openframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.java`:
- Around line 191-195: Update the stale-schedule advancement in
ScriptScheduleExecutionService to calculate elapsed intervals from now and the
existing next timestamp, then advance by the required number of step durations
in one operation. Preserve the behavior of selecting schedule.getNextRunAt() or
now and ensure the resulting next value is strictly after now without iterating
once per interval.
In
`@openframe-stream-service-core/src/main/java/com/openframe/stream/handler/ScriptExecutionStatusUpdateHandler.java`:
- Around line 85-99: In ScriptExecutionStatusUpdateHandler.java lines 85-99,
replace the missing-scriptId early return with a fallback to
scriptExecutionRepository.findByMachineIdAndExecutionId(machineId, executionId),
applying the result through applyResult and preserving the existing warning when
no row is found; retain the three-field lookup for nonblank scriptId values. In
ScriptExecutionStatusUpdateHandlerTest.java lines 243-260, update the test to
verify a missing scriptId uses the two-field fallback lookup and saves the
updated execution row.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 23c545f1-81f5-455a-a3fd-f98735c70369
📒 Files selected for processing (29)
openframe-api-lib/src/main/java/com/openframe/api/dto/rmm/schedule/CreateScriptScheduleInput.javaopenframe-api-lib/src/main/java/com/openframe/api/dto/rmm/schedule/ScriptScheduleResponse.javaopenframe-api-lib/src/main/java/com/openframe/api/dto/rmm/schedule/UpdateScriptScheduleInput.javaopenframe-api-lib/src/main/java/com/openframe/api/mapper/ScriptScheduleMapper.javaopenframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptDispatchService.javaopenframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptExecutionService.javaopenframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptScheduleService.javaopenframe-api-service-core/src/main/java/com/openframe/api/datafetcher/.ScriptDataFetcher.mdopenframe-api-service-core/src/main/java/com/openframe/api/datafetcher/ScriptScheduleDataFetcher.javaopenframe-api-service-core/src/main/resources/schema/script-schedule.graphqlsopenframe-api-service-core/src/test/java/com/openframe/api/service/rmm/ScriptDispatchServiceTest.javaopenframe-api-service-core/src/test/java/com/openframe/api/service/rmm/ScriptExecutionServiceTest.javaopenframe-api-service-core/src/test/java/com/openframe/api/service/rmm/ScriptScheduleServiceTest.javaopenframe-client-core/src/main/java/com/openframe/client/service/RmmResultService.javaopenframe-client-core/src/test/java/com/openframe/client/service/RmmResultServiceTest.javaopenframe-data-kafka/src/main/java/com/openframe/kafka/model/RmmResultEvent.javaopenframe-data-mongo-common/src/main/java/com/openframe/data/document/rmm/ScriptExecution.javaopenframe-data-mongo-common/src/main/java/com/openframe/data/document/rmm/ScriptSchedule.javaopenframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/ScriptExecutionRepository.javaopenframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/ScriptScheduleRepository.javaopenframe-data-nats/src/main/java/com/openframe/data/nats/rmm/model/RmmResultMessage.javaopenframe-data-nats/src/main/java/com/openframe/data/nats/rmm/model/ScriptMessage.javaopenframe-data-nats/src/main/java/com/openframe/data/nats/rmm/model/ScriptResultMessage.javaopenframe-management-service-core/src/main/java/com/openframe/management/scheduler/ScriptScheduleScheduler.javaopenframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.javaopenframe-management-service-core/src/test/java/com/openframe/management/service/ScriptScheduleExecutionServiceTest.javaopenframe-stream-service-core/src/main/java/com/openframe/stream/deserializer/ScriptResultDeserializer.javaopenframe-stream-service-core/src/main/java/com/openframe/stream/handler/ScriptExecutionStatusUpdateHandler.javaopenframe-stream-service-core/src/test/java/com/openframe/stream/handler/ScriptExecutionStatusUpdateHandlerTest.java
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@openframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptDispatchService.java`:
- Around line 178-199: Update ScriptDispatchService’s batch persistence and
publication flow around createBatch and publishScript to establish a retry-safe
outbox or idempotency boundary: persist each machine’s delivery state before
publishing, ensure retries do not duplicate rows or messages, and preserve the
existing executionId across partial failures. Ensure manual schedule cadence
advancement still occurs when publication partially fails, so retries can resume
pending deliveries without creating another execution.
- Around line 118-120: Update runSchedule in ScriptDispatchService to require
the retrieved schedule’s status to be ACTIVE immediately after
scriptScheduleService.get(scheduleId). Reject ARCHIVED and all other non-active
statuses before any validation, dispatch, or rescheduling side effects.
In
`@openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/.ScriptDataFetcher.md`:
- Around line 198-215: Replace the “Running a saved script” guidance to document
the existing runScript mutation rather than the runCommand flow. Use
ScriptDataFetcher.runScript and the script.graphqls runScript schema to show the
supported saved-script dispatch contract, including args and envVars, and remove
the claim that these fields are unavailable or require backend escalation.
In
`@openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/ScriptScheduleDataFetcher.java`:
- Line 69: Update the dependency wiring for ScriptScheduleDataFetcher so
deployments with spring.cloud.stream.enabled disabled can still start. Avoid
unconditional injection of ScriptDispatchService by moving stream-specific
mutation behavior into a conditional component or injecting an optional dispatch
abstraction, while preserving dispatch behavior when streaming is enabled.
- Around line 38-41: Update the affected authentication handling in
ScriptScheduleDataFetcher to inject and use AuthPrincipal, matching the existing
patterns in the class, instead of casting the security principal to Jwt. Apply
this to all referenced usages and remove the now-unused Jwt-based handling.
In
`@openframe-data-mongo-common/src/main/java/com/openframe/data/document/rmm/ScriptExecution.java`:
- Around line 25-27: Add a migration for the legacy script_executions unique
index that enforces (tenantId, executionId, machineId), and drop it during
rollout before relying on the updated
tenant_executionId_machineId_scriptId_unique definition. Ensure the migration
safely handles deployments where the legacy index is absent.
In
`@openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/ScriptScheduleRepository.java`:
- Line 34: Replace
ScriptScheduleRepository.findByStatusAndNextRunAtLessThanEqual with an ordered,
bounded, atomic claim operation. In ScriptScheduleScheduler, remove the
unsupported exactly-once guarantee and avoid relying on a fixed lease for an
unbounded sweep. In ScriptScheduleExecutionService, advance cadence only after
successful completion, persist a durable run/claim before dispatch, and publish
through an outbox or per-target delivery state using an idempotency key. Apply
these changes respectively in
openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/ScriptScheduleRepository.java:34-34,
openframe-management-service-core/src/main/java/com/openframe/management/scheduler/ScriptScheduleScheduler.java:23-24
and :35-38, and
openframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.java:87-95,
:99-115, and :141-174.
In
`@openframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.java`:
- Around line 191-195: Update the stale-schedule advancement in
ScriptScheduleExecutionService to calculate elapsed intervals from now and the
existing next timestamp, then advance by the required number of step durations
in one operation. Preserve the behavior of selecting schedule.getNextRunAt() or
now and ensure the resulting next value is strictly after now without iterating
once per interval.
In
`@openframe-stream-service-core/src/main/java/com/openframe/stream/handler/ScriptExecutionStatusUpdateHandler.java`:
- Around line 85-99: In ScriptExecutionStatusUpdateHandler.java lines 85-99,
replace the missing-scriptId early return with a fallback to
scriptExecutionRepository.findByMachineIdAndExecutionId(machineId, executionId),
applying the result through applyResult and preserving the existing warning when
no row is found; retain the three-field lookup for nonblank scriptId values. In
ScriptExecutionStatusUpdateHandlerTest.java lines 243-260, update the test to
verify a missing scriptId uses the two-field fallback lookup and saves the
updated execution row.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 23c545f1-81f5-455a-a3fd-f98735c70369
📒 Files selected for processing (29)
openframe-api-lib/src/main/java/com/openframe/api/dto/rmm/schedule/CreateScriptScheduleInput.javaopenframe-api-lib/src/main/java/com/openframe/api/dto/rmm/schedule/ScriptScheduleResponse.javaopenframe-api-lib/src/main/java/com/openframe/api/dto/rmm/schedule/UpdateScriptScheduleInput.javaopenframe-api-lib/src/main/java/com/openframe/api/mapper/ScriptScheduleMapper.javaopenframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptDispatchService.javaopenframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptExecutionService.javaopenframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptScheduleService.javaopenframe-api-service-core/src/main/java/com/openframe/api/datafetcher/.ScriptDataFetcher.mdopenframe-api-service-core/src/main/java/com/openframe/api/datafetcher/ScriptScheduleDataFetcher.javaopenframe-api-service-core/src/main/resources/schema/script-schedule.graphqlsopenframe-api-service-core/src/test/java/com/openframe/api/service/rmm/ScriptDispatchServiceTest.javaopenframe-api-service-core/src/test/java/com/openframe/api/service/rmm/ScriptExecutionServiceTest.javaopenframe-api-service-core/src/test/java/com/openframe/api/service/rmm/ScriptScheduleServiceTest.javaopenframe-client-core/src/main/java/com/openframe/client/service/RmmResultService.javaopenframe-client-core/src/test/java/com/openframe/client/service/RmmResultServiceTest.javaopenframe-data-kafka/src/main/java/com/openframe/kafka/model/RmmResultEvent.javaopenframe-data-mongo-common/src/main/java/com/openframe/data/document/rmm/ScriptExecution.javaopenframe-data-mongo-common/src/main/java/com/openframe/data/document/rmm/ScriptSchedule.javaopenframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/ScriptExecutionRepository.javaopenframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/ScriptScheduleRepository.javaopenframe-data-nats/src/main/java/com/openframe/data/nats/rmm/model/RmmResultMessage.javaopenframe-data-nats/src/main/java/com/openframe/data/nats/rmm/model/ScriptMessage.javaopenframe-data-nats/src/main/java/com/openframe/data/nats/rmm/model/ScriptResultMessage.javaopenframe-management-service-core/src/main/java/com/openframe/management/scheduler/ScriptScheduleScheduler.javaopenframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.javaopenframe-management-service-core/src/test/java/com/openframe/management/service/ScriptScheduleExecutionServiceTest.javaopenframe-stream-service-core/src/main/java/com/openframe/stream/deserializer/ScriptResultDeserializer.javaopenframe-stream-service-core/src/main/java/com/openframe/stream/handler/ScriptExecutionStatusUpdateHandler.javaopenframe-stream-service-core/src/test/java/com/openframe/stream/handler/ScriptExecutionStatusUpdateHandlerTest.java
🛑 Comments failed to post (9)
openframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptDispatchService.java (2)
118-120: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject archived schedules before dispatching.
ScriptScheduleService.get()only hidesDELETEDrecords, so this path executes and reschedulesARCHIVEDschedules. Requireschedule.status == ACTIVEbefore any validation or side effects.Proposed fix
ScriptScheduleResponse schedule = scriptScheduleService.get(scheduleId); +if (!ScriptStatus.ACTIVE.name().equals(schedule.getStatus())) { + throw new BadRequestException("Schedule is not active: " + scheduleId); +}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public DispatchResponse runSchedule(String scheduleId, String initiatedBy) { // Tenant-scoped lookup; throws if the schedule is missing or soft-deleted. ScriptScheduleResponse schedule = scriptScheduleService.get(scheduleId); if (!ScriptStatus.ACTIVE.name().equals(schedule.getStatus())) { throw new BadRequestException("Schedule is not active: " + scheduleId); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptDispatchService.java` around lines 118 - 120, Update runSchedule in ScriptDispatchService to require the retrieved schedule’s status to be ACTIVE immediately after scriptScheduleService.get(scheduleId). Reject ARCHIVED and all other non-active statuses before any validation, dispatch, or rescheduling side effects.
178-199: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make row persistence and NATS publication retry-safe.
Rows are saved first, then messages are published one at a time. A publication failure leaves a partial batch; for manual schedules it also prevents Lines 162-165 from advancing cadence, allowing an automated duplicate fire or a retry with another execution ID. Use an outbox/idempotency boundary or persist per-message delivery state before publishing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptDispatchService.java` around lines 178 - 199, Update ScriptDispatchService’s batch persistence and publication flow around createBatch and publishScript to establish a retry-safe outbox or idempotency boundary: persist each machine’s delivery state before publishing, ensure retries do not duplicate rows or messages, and preserve the existing executionId across partial failures. Ensure manual schedule cadence advancement still occurs when publication partially fails, so retries can resume pending deliveries without creating another execution.openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/.ScriptDataFetcher.md (1)
198-215: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Document the existing
runScriptmutation instead of directing clients throughrunCommand.The backend already exposes
runScriptinopenframe-api-service-core/src/main/java/com/openframe/api/datafetcher/ScriptDataFetcher.java:136-146andopenframe-api-service-core/src/main/resources/schema/script.graphqls:43-77. This guidance is therefore incorrect and causes frontend clients to bypass the saved-script dispatch contract, including its supportedargsandenvVarsfields.Proposed correction
-There is no dedicated `runScript` mutation. To dispatch a saved script: +To dispatch a saved script, use the dedicated `runScript` mutation: -1. Fetch the script via `script(id)`. -2. Compose `RunCommandInput` from the fetched fields: +runScript({ + machineId, + scriptId, + privilegeLevel, + args, + timeoutSeconds, + envVars, +}) -`defaultArgs` and `envVars` from the script are NOT yet wired through this path — `runCommand` doesn't accept them in the current schema. If you need them, flag it back to the backend team. +The backend resolves the stored script body, shell, default arguments, timeout, and environment variables.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.### Running a saved script To dispatch a saved script, use the dedicated `runScript` mutation:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/.ScriptDataFetcher.md` around lines 198 - 215, Replace the “Running a saved script” guidance to document the existing runScript mutation rather than the runCommand flow. Use ScriptDataFetcher.runScript and the script.graphqls runScript schema to show the supported saved-script dispatch contract, including args and envVars, and remove the claim that these fields are unavailable or require backend escalation.openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/ScriptScheduleDataFetcher.java (2)
38-41: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Inject
AuthPrincipalinstead of casting the security principal toJwt.This class already demonstrates that the resolved principal is
AuthPrincipalon Lines 112-115 and 143-149. The explicit(Jwt)cast can therefore throwClassCastExceptionfor authenticated requests.Proposed fix
`@DgsMutation` -public DispatchResponse runScheduleJobNow(`@InputArgument` `@NotBlank` String scheduleId) { - return scriptDispatchService.runSchedule(decodeId(scheduleId), getCurrentUserId()); +public DispatchResponse runScheduleJobNow( + `@InputArgument` `@NotBlank` String scheduleId, + `@AuthenticationPrincipal` AuthPrincipal principal) { + return scriptDispatchService.runSchedule(decodeId(scheduleId), principal.getId()); } - -private String getCurrentUserId() { - Authentication auth = SecurityContextHolder.getContext().getAuthentication(); - return AuthPrincipal.fromJwt((Jwt) auth.getPrincipal()).getId(); -}Also applies to: 152-158, 230-234
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/ScriptScheduleDataFetcher.java` around lines 38 - 41, Update the affected authentication handling in ScriptScheduleDataFetcher to inject and use AuthPrincipal, matching the existing patterns in the class, instead of casting the security principal to Jwt. Apply this to all referenced usages and remove the now-unused Jwt-based handling.
69-69: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not unconditionally inject a conditional service.
ScriptDispatchServiceexists only whenspring.cloud.stream.enabledis enabled, while this data fetcher is always created. Stream-disabled deployments will fail application startup due to the missing constructor dependency. Isolate the mutation in a conditional component or inject an optional dispatch abstraction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/ScriptScheduleDataFetcher.java` at line 69, Update the dependency wiring for ScriptScheduleDataFetcher so deployments with spring.cloud.stream.enabled disabled can still start. Avoid unconditional injection of ScriptDispatchService by moving stream-specific mutation behavior into a conditional component or injecting an optional dispatch abstraction, while preserving dispatch behavior when streaming is enabled.openframe-data-mongo-common/src/main/java/com/openframe/data/document/rmm/ScriptExecution.java (1)
25-27: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -C3 \ 'tenant_executionId_machineId|tenant_executionId_machineId_scriptId|script_executions|dropIndex|auto-index-creation' \ .Repository: flamingo-stack/openframe-oss-lib
Length of output: 9809
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show the relevant document and index-management code with line numbers. sed -n '1,120p' openframe-data-mongo-common/src/main/java/com/openframe/data/document/rmm/ScriptExecution.java printf '\n---\n' sed -n '1,180p' openframe-data-mongo-sync/src/main/java/com/openframe/data/config/MongoIndexConfig.java printf '\n---\n' sed -n '1,140p' openframe-data-mongo-common/src/main/java/com/openframe/data/document/rmm/CommandExecution.java printf '\n=== search for ScriptExecution index migration / stale index handling ===\n' rg -n -C2 'ScriptExecution|script_executions|tenant_executionId_machineId_scriptId_unique|tenant_executionId_machineId_unique|dropStaleIndex|dropIndex|auto-index-creation' openframe-data-mongo-common openframe-data-mongo-sync openframe-api-service-core openframe-data-natsRepository: flamingo-stack/openframe-oss-lib
Length of output: 50389
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '=== exact index / collection references ===' rg -n --max-count 40 \ 'script_executions|tenant_executionId_machineId_unique|tenant_executionId_machineId_scriptId_unique|tenant_executionId_machineId|dropIndex\("script_executions"|dropStaleIndex\("script_executions"|ensureIndex\(' \ openframe-data-mongo-common openframe-data-mongo-sync openframe-api-service-core openframe-data-nats echo echo '=== files that mention MongoIndexConfig or index migration helpers ===' rg -n --max-count 40 \ 'MongoIndexConfig|stale index|dropStaleIndex|auto-index-creation|ensureIndex\(' \ openframe-data-mongo-sync openframe-data-mongo-commonRepository: flamingo-stack/openframe-oss-lib
Length of output: 4124
Add a migration for the legacy
script_executionsunique index. Existing deployments can keep the old(tenantId, executionId, machineId)constraint, so duplicate script rows for one shared execution will still fail until that index is dropped during rollout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openframe-data-mongo-common/src/main/java/com/openframe/data/document/rmm/ScriptExecution.java` around lines 25 - 27, Add a migration for the legacy script_executions unique index that enforces (tenantId, executionId, machineId), and drop it during rollout before relying on the updated tenant_executionId_machineId_scriptId_unique definition. Ensure the migration safely handles deployments where the legacy index is absent.openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/ScriptScheduleRepository.java (1)
34-34: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Schedule dispatch is neither atomically claimed nor idempotent.
The finite lock, unbounded query, and publish-before-cadence-save flow permit both duplicate and permanently partial schedule runs.
openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/ScriptScheduleRepository.java#L34-L34: replace the unbounded read with ordered, bounded, atomically claimed work.openframe-management-service-core/src/main/java/com/openframe/management/scheduler/ScriptScheduleScheduler.java#L23-L24: remove the exactly-once guarantee unless backed by durable dispatch idempotency.openframe-management-service-core/src/main/java/com/openframe/management/scheduler/ScriptScheduleScheduler.java#L35-L38: do not rely on a fixed maximum lease to cover an unbounded sweep.openframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.java#L87-L95: do not advance cadence unconditionally after partial or transient failure.openframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.java#L99-L115: persist a durable run/claim before external dispatch and complete cadence separately.openframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.java#L141-L174: publish through an outbox or retain per-target delivery state and an idempotency key.📍 Affects 3 files
openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/ScriptScheduleRepository.java#L34-L34(this comment)openframe-management-service-core/src/main/java/com/openframe/management/scheduler/ScriptScheduleScheduler.java#L23-L24openframe-management-service-core/src/main/java/com/openframe/management/scheduler/ScriptScheduleScheduler.java#L35-L38openframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.java#L87-L95openframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.java#L99-L115openframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.java#L141-L174🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/ScriptScheduleRepository.java` at line 34, Replace ScriptScheduleRepository.findByStatusAndNextRunAtLessThanEqual with an ordered, bounded, atomic claim operation. In ScriptScheduleScheduler, remove the unsupported exactly-once guarantee and avoid relying on a fixed lease for an unbounded sweep. In ScriptScheduleExecutionService, advance cadence only after successful completion, persist a durable run/claim before dispatch, and publish through an outbox or per-target delivery state using an idempotency key. Apply these changes respectively in openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/ScriptScheduleRepository.java:34-34, openframe-management-service-core/src/main/java/com/openframe/management/scheduler/ScriptScheduleScheduler.java:23-24 and :35-38, and openframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.java:87-95, :99-115, and :141-174.openframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.java (1)
191-195: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Advance stale cadence arithmetically rather than interval by interval.
startAtis client-controlled and may be years in the past; a 30-minute interval can therefore require millions of loop iterations. Calculate the number of elapsed intervals and jump directly to the next slot.Proposed approach
Duration step = Duration.ofMinutes(intervalMinutes); Instant next = schedule.getNextRunAt() != null ? schedule.getNextRunAt() : now; - while (!next.isAfter(now)) { - next = next.plus(step); - } + long stepSeconds = step.getSeconds(); + long elapsedSeconds = Duration.between(next, now).getSeconds(); + long steps = Math.floorDiv(elapsedSeconds, stepSeconds) + 1; + next = next.plusSeconds(Math.multiplyExact(steps, stepSeconds)); schedule.setNextRunAt(next);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Duration step = Duration.ofMinutes(intervalMinutes); Instant next = schedule.getNextRunAt() != null ? schedule.getNextRunAt() : now; long stepSeconds = step.getSeconds(); long elapsedSeconds = Duration.between(next, now).getSeconds(); long steps = Math.floorDiv(elapsedSeconds, stepSeconds) + 1; next = next.plusSeconds(Math.multiplyExact(steps, stepSeconds)); schedule.setNextRunAt(next);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openframe-management-service-core/src/main/java/com/openframe/management/service/ScriptScheduleExecutionService.java` around lines 191 - 195, Update the stale-schedule advancement in ScriptScheduleExecutionService to calculate elapsed intervals from now and the existing next timestamp, then advance by the required number of step durations in one operation. Preserve the behavior of selecting schedule.getNextRunAt() or now and ensure the resulting next value is strictly after now without iterating once per interval.openframe-stream-service-core/src/main/java/com/openframe/stream/handler/ScriptExecutionStatusUpdateHandler.java (1)
85-99: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Restore fallback correlation for legacy agents without
scriptId. Dropping results that lack ascriptIdbreaks backwards compatibility for agents predating this feature. For legacy standalone script executions, the(machineId, executionId)pair is still strictly unique.
openframe-stream-service-core/src/main/java/com/openframe/stream/handler/ScriptExecutionStatusUpdateHandler.java#L85-L99: Add a fallback toscriptExecutionRepository.findByMachineIdAndExecutionId(machineId, executionId)whenscriptIdis null or blank.openframe-stream-service-core/src/test/java/com/openframe/stream/handler/ScriptExecutionStatusUpdateHandlerTest.java#L243-L260: Update the test to verify that a missingscriptIdcorrectly triggers the two-field fallback lookup and saves the updated execution row.🛠️ Proposed fix
For
ScriptExecutionStatusUpdateHandler.java:- // The agent always echoes scriptId, so it is the exact correlation key: a schedule - // run shares one executionId across all its scripts, and (executionId, machineId) - // alone would match several rows. String scriptId = stringOrNull(after, FIELD_SCRIPT_ID); - if (scriptId == null || scriptId.isBlank()) { - log.warn("RMM result has no scriptId — cannot update Execution row (executionId={} machineId={})", - executionId, machineId); - return; - } - - scriptExecutionRepository.findByMachineIdAndExecutionIdAndScriptId(machineId, executionId, scriptId) - .ifPresentOrElse( - row -> applyResult(row, after), - () -> log.warn("No Execution row for executionId={} machineId={} scriptId={} — result arrived before dispatch persisted OR row was never created", - executionId, machineId, scriptId)); + Optional<ScriptExecution> rowOpt; + if (scriptId == null || scriptId.isBlank()) { + rowOpt = scriptExecutionRepository.findByMachineIdAndExecutionId(machineId, executionId); + } else { + rowOpt = scriptExecutionRepository.findByMachineIdAndExecutionIdAndScriptId(machineId, executionId, scriptId); + } + + rowOpt.ifPresentOrElse( + row -> applyResult(row, after), + () -> log.warn("No Execution row for executionId={} machineId={} scriptId={} — result arrived before dispatch persisted OR row was never created", + executionId, machineId, scriptId));For
ScriptExecutionStatusUpdateHandlerTest.java:- `@Test` - `@DisplayName`("handle: missing scriptId (broken/legacy message) → repo NOT touched, no exception — the agent always echoes scriptId") - void handle_missingScriptId_skipsQuietly() { - DeserializedDebeziumMessage message = new DeserializedDebeziumMessage(); - message.setTenantId(TENANT_ID); - ObjectNode after = mapper.createObjectNode() - .put("executionId", EXECUTION_ID) - .put("machineId", MACHINE_ID) - .put("exitCode", 0); - DebeziumMessage.Payload<com.fasterxml.jackson.databind.JsonNode> payload = new DebeziumMessage.Payload<>(); - payload.setAfter(after); - message.setPayload(payload); - - handler.handle(message, new IntegratedToolEnrichedData()); - - verify(scriptExecutionRepository, never()).findByMachineIdAndExecutionIdAndScriptId(any(), any(), any()); - verify(scriptExecutionRepository, never()).save(any()); - } + `@Test` + `@DisplayName`("handle: missing scriptId (legacy message) → falls back to (machineId, executionId) lookup") + void handle_missingScriptId_fallsBackToTwoFieldLookup() { + ScriptExecution row = runningRow(EXECUTION_ID); + when(scriptExecutionRepository.findByMachineIdAndExecutionId(MACHINE_ID, EXECUTION_ID)) + .thenReturn(Optional.of(row)); + + DeserializedDebeziumMessage message = new DeserializedDebeziumMessage(); + message.setTenantId(TENANT_ID); + ObjectNode after = mapper.createObjectNode() + .put("executionId", EXECUTION_ID) + .put("machineId", MACHINE_ID) + .put("exitCode", 0); + DebeziumMessage.Payload<com.fasterxml.jackson.databind.JsonNode> payload = new DebeziumMessage.Payload<>(); + payload.setAfter(after); + message.setPayload(payload); + + handler.handle(message, new IntegratedToolEnrichedData()); + + verify(scriptExecutionRepository).findByMachineIdAndExecutionId(MACHINE_ID, EXECUTION_ID); + verify(scriptExecutionRepository).save(any(ScriptExecution.class)); + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.String scriptId = stringOrNull(after, FIELD_SCRIPT_ID); Optional<ScriptExecution> rowOpt; if (scriptId == null || scriptId.isBlank()) { rowOpt = scriptExecutionRepository.findByMachineIdAndExecutionId(machineId, executionId); } else { rowOpt = scriptExecutionRepository.findByMachineIdAndExecutionIdAndScriptId(machineId, executionId, scriptId); } rowOpt.ifPresentOrElse( row -> applyResult(row, after), () -> log.warn("No Execution row for executionId={} machineId={} scriptId={} — result arrived before dispatch persisted OR row was never created", executionId, machineId, scriptId));`@Test` `@DisplayName`("handle: missing scriptId (legacy message) → falls back to (machineId, executionId) lookup") void handle_missingScriptId_fallsBackToTwoFieldLookup() { ScriptExecution row = runningRow(EXECUTION_ID); when(scriptExecutionRepository.findByMachineIdAndExecutionId(MACHINE_ID, EXECUTION_ID)) .thenReturn(Optional.of(row)); DeserializedDebeziumMessage message = new DeserializedDebeziumMessage(); message.setTenantId(TENANT_ID); ObjectNode after = mapper.createObjectNode() .put("executionId", EXECUTION_ID) .put("machineId", MACHINE_ID) .put("exitCode", 0); DebeziumMessage.Payload<com.fasterxml.jackson.databind.JsonNode> payload = new DebeziumMessage.Payload<>(); payload.setAfter(after); message.setPayload(payload); handler.handle(message, new IntegratedToolEnrichedData()); verify(scriptExecutionRepository).findByMachineIdAndExecutionId(MACHINE_ID, EXECUTION_ID); verify(scriptExecutionRepository).save(any(ScriptExecution.class)); }📍 Affects 2 files
openframe-stream-service-core/src/main/java/com/openframe/stream/handler/ScriptExecutionStatusUpdateHandler.java#L85-L99(this comment)openframe-stream-service-core/src/test/java/com/openframe/stream/handler/ScriptExecutionStatusUpdateHandlerTest.java#L243-L260🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openframe-stream-service-core/src/main/java/com/openframe/stream/handler/ScriptExecutionStatusUpdateHandler.java` around lines 85 - 99, In ScriptExecutionStatusUpdateHandler.java lines 85-99, replace the missing-scriptId early return with a fallback to scriptExecutionRepository.findByMachineIdAndExecutionId(machineId, executionId), applying the result through applyResult and preserving the existing warning when no row is found; retain the three-field lookup for nonblank scriptId values. In ScriptExecutionStatusUpdateHandlerTest.java lines 243-260, update the test to verify a missing scriptId uses the two-field fallback lookup and saves the updated execution row.
…n' into feature/script-schedule-execution
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openframe-api-service-core/src/test/java/com/openframe/api/service/rmm/ScriptExecutionServiceTest.java (1)
64-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant mock setup.
This stubbing is redundant because it is already set up in the
@BeforeEachmethod on line 58. It can be safely removed to keep the test body concise.♻️ Proposed refactor
- when(scriptExecutionRepository.save(any(ScriptExecution.class))).thenAnswer(inv -> inv.getArgument(0)); Instant before = Instant.now().minus(Duration.ofSeconds(1));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openframe-api-service-core/src/test/java/com/openframe/api/service/rmm/ScriptExecutionServiceTest.java` at line 64, Remove the duplicate scriptExecutionRepository.save stubbing from the affected test method, keeping the existing setup in the `@BeforeEach` method as the single source of this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@openframe-api-service-core/src/test/java/com/openframe/api/service/rmm/ScriptExecutionServiceTest.java`:
- Line 64: Remove the duplicate scriptExecutionRepository.save stubbing from the
affected test method, keeping the existing setup in the `@BeforeEach` method as
the single source of this behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 248e5d2c-13e0-4389-bbd3-9a20b5af696e
📒 Files selected for processing (5)
openframe-api-lib/src/main/java/com/openframe/api/dto/rmm/execution/ScriptExecutionResponse.javaopenframe-api-lib/src/main/java/com/openframe/api/mapper/ScriptExecutionMapper.javaopenframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptExecutionService.javaopenframe-api-service-core/src/main/resources/schema/script-execution.graphqlsopenframe-api-service-core/src/test/java/com/openframe/api/service/rmm/ScriptExecutionServiceTest.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptScheduleService.java (2)
184-188: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecompute cadence when
repeatchanges.A repeat-only update leaves
nextRunAtunchanged. For example, converting a completed one-shot schedule to recurring can leavenextRunAt == null, so it never executes; converting recurring to one-shot can retain a stale pending run. Recalculate timing whenever eitherstartAtorrepeatchanges, with tests for both transitions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptScheduleService.java` around lines 184 - 188, Update the schedule update logic around existing.getStartAt() and scheduleMapper.updateEntity(existing, input) to detect changes to both startAt and repeat, not just startAt. When either value changes, recompute nextRunAt from the updated schedule state so one-shot-to-recurring and recurring-to-one-shot transitions do not retain stale timing; add tests covering both repeat transitions.
236-245: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPrevent older manual runs from overwriting newer cadence state.
This read-modify-save is last-write-wins. Concurrent runs can save
runAt=t2first and then regresslastRunAt/nextRunAtwith an oldert1, potentially causing an early automated execution. Use an atomic conditional update or optimistic versioning so only the latest run advances cadence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptScheduleService.java` around lines 236 - 245, Update rescheduleAfterManualRun to prevent stale concurrent manual runs from overwriting newer cadence state. Use an atomic conditional repository update or optimistic version check that applies the lastRunAt/nextRunAt changes only when the stored state is not newer than runAt, while preserving the existing repeat-based next-run calculation.
🧹 Nitpick comments (1)
openframe-data-mongo-sync/src/test/java/com/openframe/data/integration/repository/rmm/ScriptScheduleRepositoryIT.java (1)
161-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover backward paging and the descending null boundary.
Add compound-cursor tests for backward traversal and forward DESC traversal from numeric repeats into the null tail. These exercise the currently uncovered direction flip and Lines 264-267 null branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openframe-data-mongo-sync/src/test/java/com/openframe/data/integration/repository/rmm/ScriptScheduleRepositoryIT.java` around lines 161 - 210, The existing repeat keyset integration tests cover forward ASC and DESC traversal but omit backward paging and the DESC numeric-to-null boundary. Add tests alongside findPageForTenant_repeatKeysetPagesCleanlyAcrossTies and findPageForTenant_repeatKeysetCrossesNullBoundaryAscending that validate backward traversal across tied repeat values and forward DESC traversal from numeric repeats into the null group, asserting every row appears exactly once in the expected compound-key order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@openframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptScheduleService.java`:
- Around line 184-188: Update the schedule update logic around
existing.getStartAt() and scheduleMapper.updateEntity(existing, input) to detect
changes to both startAt and repeat, not just startAt. When either value changes,
recompute nextRunAt from the updated schedule state so one-shot-to-recurring and
recurring-to-one-shot transitions do not retain stale timing; add tests covering
both repeat transitions.
- Around line 236-245: Update rescheduleAfterManualRun to prevent stale
concurrent manual runs from overwriting newer cadence state. Use an atomic
conditional repository update or optimistic version check that applies the
lastRunAt/nextRunAt changes only when the stored state is not newer than runAt,
while preserving the existing repeat-based next-run calculation.
---
Nitpick comments:
In
`@openframe-data-mongo-sync/src/test/java/com/openframe/data/integration/repository/rmm/ScriptScheduleRepositoryIT.java`:
- Around line 161-210: The existing repeat keyset integration tests cover
forward ASC and DESC traversal but omit backward paging and the DESC
numeric-to-null boundary. Add tests alongside
findPageForTenant_repeatKeysetPagesCleanlyAcrossTies and
findPageForTenant_repeatKeysetCrossesNullBoundaryAscending that validate
backward traversal across tied repeat values and forward DESC traversal from
numeric repeats into the null group, asserting every row appears exactly once in
the expected compound-key order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c9159e3d-b348-4319-bbff-7daca7cf95ad
📒 Files selected for processing (6)
openframe-api-lib/src/main/java/com/openframe/api/service/rmm/ScriptScheduleService.javaopenframe-api-service-core/src/main/resources/schema/script-schedule.graphqlsopenframe-api-service-core/src/test/java/com/openframe/api/service/rmm/ScriptScheduleServiceTest.javaopenframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/CustomScriptScheduleRepository.javaopenframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/CustomScriptScheduleRepositoryImpl.javaopenframe-data-mongo-sync/src/test/java/com/openframe/data/integration/repository/rmm/ScriptScheduleRepositoryIT.java
🚧 Files skipped from review as they are similar to previous changes (1)
- openframe-api-service-core/src/main/resources/schema/script-schedule.graphqls
Closes: https://app.clickup.com/t/9013925967/86ajhuaqw
Summary by CodeRabbit