[semen-flamingo]

What

One case of a batch that waits on the engine for an offline machine, found in the review of what can change while it waits (#2880 shipped the wait): the schedule's script list changes. The waiting machine gets the schedule as the user sees it now: a script added to the schedule joins the batch and gets a QUEUED leaf under the same fire; a script that leaves the batch (removed from the schedule, deleted, archived) loses its leaf. The leaf was the promise of a batch the agent never received, and a leaf closed FAILED for it marked the whole run FAILED while every script that ran succeeded; the run shows what ran, as the path before the engine did (it made a new fire from the current schedule at the reconnect). Before, the batch was rebuilt from the fire's own scripts only, so a schedule edited from [A, B] to [C] still sent A and B when the machine came back, and C never.

retireScript is gone: a script delete or archive refreshes the waiting batches the way an edit does (the script is inactive, so it leaves the batch). A batch withdrawn as a whole (schedule deleted or archived, device unassigned, no script left) still closes through the engine with its reason, as before.

Reviewed and left as is: a repeating schedule firing again while the machine is still offline. The API keeps the reconnect window under the repeat interval, so the earlier batch has failed at its window end before the next fire; the same held for the reconnect sentinel.

Checked on the feature tenant of #4291 (tenant-4291-77)

Schedule made in the UI: scripts A and B, "run when the device comes back online" 30 min, one offline machine. The fire at 18:28:00Z held the batch on the engine (two QUEUED leaves). At 18:30:41Z the schedule was edited in the UI to script C only: the body was replaced (Delivery body replaced, Waiting batches refreshed … replaced=1), the leaves of A and B left the fire, a QUEUED leaf for C appeared under the same fire. The machine came back at 18:31:59Z, the sweep published at 18:32:08Z, the acknowledge for C closed the row DONE and moved the C leaf to RUNNING. (That check closed the A and B leaves FAILED, the head before the deletion; the re-check of this head is below.)

Where

ScheduleDeliveryRefresher (api-lib): runnableScriptsOf reads the schedule's current list; alignLeaves makes the leaves of a machine whose body was replaced equal to the batch (only those: a machine whose row is already acked, or on the route before the engine, keeps its leaves). ScriptService delete/archive call refreshForScript.

Tests

ScheduleDeliveryRefresherTest (+ script added, script removed, script deleted, row not replaced), ScriptServiceTest.

Change-Set: delivery-waiting-batch-follows-schedule

🤖 Generated with Claude Code

https://claude.ai/code/session_018qx6NqA97kwRxQ4hnD5jtu

Change set flamingo-stack/delivery-waiting-batch-follows-schedule: this pull request is the only one in it so far. Another pull request joins by naming this one in a Depends-On line, or by carrying the same Change-Set line.

• flamingo-stack/openframe-oss-lib#2939 (this pull request): fix(delivery): a waiting batch follows the schedule's script list CU-86akkb8f5 · open

Linked work

• …and 1 linked item not shown here

Linked by the Depends-On / Change-Set lines in these descriptions; this block is maintained by the hub.

[github-actions]

🦩 Flamingo Code Review

✅ Tests and checks passed on this commit (2 lanes).

2 findings: 🔴 1 action required · 🟠 1 recommended · 🔵 0 informational

Advisory: findings do not block the merge.

2 defects outside any rule

Files	Lines	🔴 Action required	🟠 Recommended	🔵 Informational
2	+144 / -28	1	1	0

🔴 Action required (1)

#	Where	Finding	Flagged by
1	ScheduleDeliveryRefresher.java:134	alignLeaves closes leaves of machines not in replacedMachines, orphaning their rows	Review

🟠 Recommended (1)

#	Where	Finding	Flagged by
2	ScheduleDeliveryRefresher.java:178	newLeaf ignores per-machine existing timestamps and always uses sample.getDispatchedAt()	Review

✅ What this change does well

• ScheduleDeliveryRefresherTest adds focused tests covering script-added, script-removed, and not-replaced scenarios for refreshForSchedule, matching the new alignLeaves branching.
• runnableScriptsOf is simplified to derive the schedule's scripts purely from the schedule's current order, removing the earlier fallback-to-fired-scripts logic that conflated two different sources of truth.
• The updated class-level comment in ScheduleDeliveryRefresher.java clearly documents the new leaf-following behavior for added/removed scripts, aiding future maintainers.

Inline comments: 2 new

5 possible problems checked and ruled out, 3 after reading more of the repository

• openframe-api-lib/src/main/java/com/openframe/api/service/rmm/schedule/ScheduleDeliveryRefresher.java:161 alignLeaves can violate the unique compound index by inserting a leaf that already exists for a different status/timestamp, risking a duplicate-key failure for the whole batch save: The unique index is on (tenantId, executionId, machineId, scriptId); leavesOfTheFire is loaded from exactly that scope and byScript is keyed by scriptId per machine from that same fresh load inside the single refreshExecution call, so newLeaf is only built when no existing leaf for that (machineId, scriptId) tuple was found moments earlier in the same request, the claimed race/duplicate-key…
• openframe-api-lib/src/main/java/com/openframe/api/service/rmm/schedule/ScheduleDeliveryRefresher.java:151 alignLeaves nests three levels deep (for machine → filter/forEach → for script → if/else-if chain): The nesting (for machine - for script - if/else-if) is two levels of loop plus a flat if/else chain, not clearly exceeding the stated two-level limit in a way distinct from ordinary business logic; largely stylistic.
• openframe-api-lib/src/main/java/com/openframe/api/service/rmm/schedule/ScheduleDeliveryRefresher.java:146 alignLeaves mixes three phases (close orphaned leaves, create/reopen/update per-script leaves, batch save) inline in one loop body: The method is already reasonably decomposed (newLeaf, reopen, isQueued extracted); the remaining loop body is acceptable complexity for business logic, not a case of visibly unseparated load/transform/persist phases.
• openframe-api-lib/src/main/java/com/openframe/api/service/rmm/schedule/ScheduleDeliveryRefresher.java:155 Compound stream-chain filter condition in alignLeaves left inline instead of a named predicate: OFJAVA-007 targets if-conditions, not stream filter predicates; this is a minor style nit stretched to fit the rule and not something a senior engineer would block on.
• openframe-api-lib/src/main/java/com/openframe/api/service/rmm/schedule/ScheduleDeliveryRefresher.java:155 Multi-line lambda body inside alignLeaves forEach: The lambda body is short (two statements) and the rule's intent targets genuinely large multi-statement blocks hiding logic; this is a stylistic nitpick a senior engineer would not insist on blocking.

🤖 Prompt for AI agents: fix every finding

Address these code-review findings on pull request #2939 of flamingo-stack/openframe-oss-lib, most important first.
For each one, make the minimal change that resolves it and do not refactor unrelated code.
If you can show a finding is wrong, skip it and say why.

1. [Action required] openframe-api-lib/src/main/java/com/openframe/api/service/rmm/schedule/ScheduleDeliveryRefresher.java:134 alignLeaves closes leaves of machines not in replacedMachines, orphaning their rows
   refreshExecution computes `replaced` as only the machines for which deliveryRefresher.refresh(...) returned true (line 134-137). alignLeaves is then called only with that `replaced` set (line 139), and inside alignLeaves the for-loop over `replacedMachines` (line 151) is the only place that looks at `leavesOfTheFire`. A machine that was NOT replaced (deliveryRefresher.refresh returned false, e.g. because the device is offline and the offline-behavior keeps the old row, or the machine simply isn't in `waitingMachines` for this executionId) never has its leaves touched by alignLeaves. That by itself is correct per the new test 'refreshForSchedule_rowNotReplaced_leavesUntouched'. However, leavesOfTheFire is fetched via `findByTenantIdAndExecutionId(tenantId, executionId)`, i.e. it contains leaves for ALL machines under this executionId, not just the waiting/queued ones. Machines whose leaf status is already a terminal state (e.g. RUNNING or already FAILED from a prior refresh) for scripts removed from the schedule are never marked FAILED with REMOVED_FROM_SCHEDULE, because the forEach at line 155-160 only considers machines in `replacedMachines`. So a schedule edit that removes a script will leave stale QUEUED leaves for any machine whose delivery-refresh call returned false (e.g. transient refresh failure, or a machine that is not currently in the `waitingMachines` set from a stale read) indefinitely in QUEUED state for a script that no longer exists in the schedule — contradicting the class-level invariant comment that 'a script removed from the schedule closes' its leaf for every machine, not only the replaced ones.
2. [Recommended] openframe-api-lib/src/main/java/com/openframe/api/service/rmm/schedule/ScheduleDeliveryRefresher.java:178 newLeaf ignores per-machine existing timestamps and always uses sample.getDispatchedAt()
   In alignLeaves, when a machine gets a brand-new leaf for a script that joined the schedule (line 163-164, newLeaf), the leaf's dispatchedAt is copied from `sample` -- the first element of the original queuedLeaves list for the whole execution (set at line 110, `ScriptExecution first = queuedLeaves.getFirst()`). That `first` leaf belongs to whichever machine happened to be first in the queued list, not to the `machineId` the new leaf is being created for. If different machines in the same executionId were dispatched at different times (e.g. staggered delivery, or one machine joined the schedule set later), the new leaf for machineId gets a dispatchedAt timestamp that belongs to a different machine's leaf, which can mislead the result-watchdog's timeout computation (timeoutSeconds is measured from dispatchedAt).

---

Next: commit a suggestion, or hand a comment's prompt to your coding agent. React 👍 on a finding that helped and 👎 on one that is wrong: the reviewer learns from both.

Review again. New commits are not reviewed until you ask:

• Review the new commits: only what was pushed since this review
• Review the whole diff again: everything, including what was already reviewed

Or comment @flamingo-review (new commits) or @flamingo-review full (everything). Add the flamingo-review-always label to review every push.

🧪 Tests and mutation

Tests per lane

Language	Project	Command	Status	Counts	Time
Java (Maven)	.	mvn -B -s ./.mvn/settings.xml test -pl openframe-api-lib,openframe-api-service-core -am	✅ passed	openframe-agent-key 4 · openframe-api-lib 603 · openframe-api-service-core 760 (42 skipped) · openframe-core 150 · openframe-core-crypto 8 · openframe-data-cassandra 20 · openframe-data-kafka 35 · openframe-data-loki 96 · openframe-data-mongo-sync 467 (198 skipped) · openframe-data-nats 163 (4 skipped) · openframe-data-pinot 69 · openframe-exception 4 · openframe-graphql-node 76 · openframe-graphql-relay 69 · openframe-machine-delivery 147 · openframe-notification-core 103 (22 skipped) · openframe-notification-mail 5 · openframe-presence 12 · openframe-security-core 38 · fleetmdm 51 · 0 failures	327.1 s

Mutation testing

This build has no PIT plugin, so each module is mutated from the command line with pit-adhoc.sh, changing nothing in the repository; no production class changed in openframe-api-service-core: nothing there to mutate.

Module	Classes	Whole classes	New lines only	Time
openframe-api-lib	ScheduleDeliveryRefresher	64 mutants, 0 killed (0%), 64 no coverage, line coverage 0%	23 mutants: 23 no coverage	44 s

New code, its covering tests and the result

New code	Covering tests	Result
refreshExecution in ScheduleDeliveryRefresher, line 139	none	no coverage (line 139)
lambda$alignLeaves$1 in ScheduleDeliveryRefresher, line 153	none	no coverage (line 153)
lambda$alignLeaves$2 in ScheduleDeliveryRefresher, line 154	none	no coverage (line 154)
lambda$alignLeaves$3 in ScheduleDeliveryRefresher, line 156	none	no coverage (line 156)
alignLeaves in ScheduleDeliveryRefresher, lines 157-168	none	no coverage (lines 157, 163, 165-166, 168)
lambda$alignLeaves$4 in ScheduleDeliveryRefresher, line 158	none	no coverage (line 158)
newLeaf in ScheduleDeliveryRefresher, line 179	none	no coverage (line 179)
reopen in ScheduleDeliveryRefresher, lines 196-200	none	no coverage (lines 196-200)
isQueued in ScheduleDeliveryRefresher, line 205	none	no coverage (line 205)
runnableScriptsOf in ScheduleDeliveryRefresher, lines 228-232	none	no coverage (lines 228, 232)

23 mutants on lines this change added are not caught (0 survived, 23 with no test reaching them): each is annotated on its line in the Flamingo Code Review / tests and mutation check.

Started 2026-11-02 18:05 UTC · updated 2026-11-02 18:07 UTC · workflow run

[semen-flamingo]

@flamingo-review

[andrii-flamingo]

You fix the main case, but:

A run could show FAILED when nothing actually failed.
Say a schedule has scripts A and B, the device is offline, and an admin removes B. When the device comes back, only A runs and it succeeds, which is correct. But B's record is closed as FAILED, and one failed record marks the whole run as FAILED.
In "current version" this run showed success.
Fix: delete B's record instead of closing it as FAILED.