diff --git a/api/src/main/java/org/apache/unomi/api/EventInfo.java b/api/src/main/java/org/apache/unomi/api/EventInfo.java index f9d6410a55..1f67d6e41d 100644 --- a/api/src/main/java/org/apache/unomi/api/EventInfo.java +++ b/api/src/main/java/org/apache/unomi/api/EventInfo.java @@ -17,12 +17,16 @@ package org.apache.unomi.api; +import java.io.Serializable; + /** * One event type entry in {@link ServerInfo#getEventTypes()}. * Pairs the event type name with how many matching events exist on the server, * giving operators and clients a quick view of which event types are active. */ -public class EventInfo { +public class EventInfo implements Serializable { + + private static final long serialVersionUID = 1L; private String name; private Long occurences; diff --git a/api/src/main/java/org/apache/unomi/api/ServerInfo.java b/api/src/main/java/org/apache/unomi/api/ServerInfo.java index b2ce88d87f..5066f58882 100644 --- a/api/src/main/java/org/apache/unomi/api/ServerInfo.java +++ b/api/src/main/java/org/apache/unomi/api/ServerInfo.java @@ -17,6 +17,7 @@ package org.apache.unomi.api; +import java.io.Serializable; import java.util.ArrayList; import java.util.Date; import java.util.List; @@ -28,7 +29,9 @@ * operators, and other nodes can read version, build metadata, supported * {@link EventInfo} types, optional capability flags, and banner logo lines. */ -public class ServerInfo { +public class ServerInfo implements Serializable { + + private static final long serialVersionUID = 1L; /** @api.example unomi */ private String serverIdentifier; diff --git a/api/src/main/java/org/apache/unomi/api/services/SchedulerService.java b/api/src/main/java/org/apache/unomi/api/services/SchedulerService.java index 0b770b91ff..6ba545adb8 100644 --- a/api/src/main/java/org/apache/unomi/api/services/SchedulerService.java +++ b/api/src/main/java/org/apache/unomi/api/services/SchedulerService.java @@ -351,11 +351,22 @@ interface TaskBuilder { /** * Disallows parallel execution. * Task will use locking to ensure only one instance runs at a time. + * This is the default for {@link #newTask(String)}. * * @return this builder for method chaining */ TaskBuilder disallowParallelExecution(); + /** + * Allows parallel execution (disables exclusive cluster locking for this task). + * Use only when duplicate side effects across nodes are intentional; prefer the + * default exclusive behaviour for cluster-wide jobs. {@link #runOnAllNodes()} + * enables this automatically. + * + * @return this builder for method chaining + */ + TaskBuilder allowParallelExecution(); + /** * Sets the task executor. * diff --git a/clear-elasticsearch.sh b/clear-elasticsearch.sh index 3e3c23ea4b..0d92e9da23 100755 --- a/clear-elasticsearch.sh +++ b/clear-elasticsearch.sh @@ -48,13 +48,17 @@ if [ "${_IS_SOURCED}" = false ]; then exit 1 fi -# Clear Elasticsearch environment variables +# Clear Elasticsearch environment variables (must cover everything setup-elasticsearch.sh exports) unset UNOMI_ELASTICSEARCH_CLUSTERNAME unset UNOMI_ELASTICSEARCH_ADDRESSES unset UNOMI_ELASTICSEARCH_USERNAME unset UNOMI_ELASTICSEARCH_PASSWORD unset UNOMI_ELASTICSEARCH_SSL_ENABLE unset UNOMI_ELASTICSEARCH_SSL_TRUST_ALL_CERTIFICATES +# Also set by setup-elasticsearch.sh / setup-opensearch.sh +unset UNOMI_DISTRIBUTION + +unset _IS_SOURCED echo "Elasticsearch environment variables cleared." diff --git a/clear-opensearch.sh b/clear-opensearch.sh index e6789bff4b..61b6c5a121 100755 --- a/clear-opensearch.sh +++ b/clear-opensearch.sh @@ -48,13 +48,18 @@ if [ "${_IS_SOURCED}" = false ]; then exit 1 fi -# Clear OpenSearch environment variables +# Clear OpenSearch environment variables (must cover everything setup-opensearch.sh exports, +# plus common overrides users may have set manually) unset UNOMI_OPENSEARCH_CLUSTERNAME unset UNOMI_OPENSEARCH_ADDRESSES unset UNOMI_OPENSEARCH_USERNAME unset UNOMI_OPENSEARCH_PASSWORD unset UNOMI_OPENSEARCH_SSL_ENABLE unset UNOMI_OPENSEARCH_SSL_TRUST_ALL_CERTIFICATES +# Also set by setup-opensearch.sh / setup-elasticsearch.sh +unset UNOMI_DISTRIBUTION + +unset _IS_SOURCED echo "OpenSearch environment variables cleared." diff --git a/itests/src/test/java/org/apache/unomi/itests/BaseIT.java b/itests/src/test/java/org/apache/unomi/itests/BaseIT.java index 53a07bedde..8587f4f8a4 100644 --- a/itests/src/test/java/org/apache/unomi/itests/BaseIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/BaseIT.java @@ -669,6 +669,13 @@ public Option[] config() { editConfigurationFilePut("etc/custom.system.properties", "org.apache.unomi.opensearch.rollover.maxDocs", "300"), editConfigurationFilePut("etc/custom.system.properties", "org.apache.unomi.opensearch.minimalClusterState", "YELLOW"), editConfigurationFilePut("etc/custom.system.properties", "org.apache.unomi.migration.tenant.id", TEST_TENANT_ID), + // Default scheduler.thread.poolSize (5) is sized for the near-instant in-memory unit-test + // double, not a real ES/OS backend. Under real refresh/write latency, the checker, task + // executions, and lease-renewal heartbeats (see scheduler.adoc) compete for the same small + // pool; when it saturates, heartbeats starve, locks age out from under live executions, and + // crash recovery repeatedly (mis)reclaims them, logging "Lock verification failed... after + // CAS" every checker tick. Widen it for ITs so heartbeats/checker never starve. + editConfigurationFilePut("etc/custom.system.properties", "org.apache.unomi.scheduler.thread.poolSize", "10"), systemProperty("org.ops4j.pax.exam.rbc.rmi.port").value("1199"), systemProperty("org.apache.unomi.healthcheck.enabled").value("true"), @@ -747,6 +754,15 @@ public Option[] config() { karafOptions.add(editConfigurationFilePut("etc/org.ops4j.pax.logging.cfg", "log4j2.logger.paxStore.name", "org.ops4j.store")); karafOptions.add(editConfigurationFilePut("etc/org.ops4j.pax.logging.cfg", "log4j2.logger.paxStore.level", "WARN")); + // UNOMI-967 diagnostic: DEBUG-level "LOCK-DIAG"-tagged logging was added throughout the + // scheduler's lock acquisition/renewal/recovery paths (TaskLockManager, TaskRecoveryManager, + // SchedulerServiceImpl, TaskExecutionManager) to root-cause a live-IT-only bug where + // "Lock verification failed... after CAS" fires on every checker tick for the whole suite + // (a real ES backend, not the in-memory unit-test double). Bump this package to DEBUG for + // ITs only so that logging is actually captured. TODO: remove once root-caused and fixed. + karafOptions.add(editConfigurationFilePut("etc/org.ops4j.pax.logging.cfg", "log4j2.logger.schedulerDiag.name", "org.apache.unomi.services.impl.scheduler")); + karafOptions.add(editConfigurationFilePut("etc/org.ops4j.pax.logging.cfg", "log4j2.logger.schedulerDiag.level", "DEBUG")); + // Enable debug logging for Karaf Resolver to diagnose bundle refresh issues (default: disabled) boolean enableResolverDebug = Boolean.parseBoolean(System.getProperty(RESOLVER_DEBUG_PROPERTY, "false")); if (enableResolverDebug) { diff --git a/manual/src/main/asciidoc/scheduler.adoc b/manual/src/main/asciidoc/scheduler.adoc index 1f5976b420..786fdbc874 100644 --- a/manual/src/main/asciidoc/scheduler.adoc +++ b/manual/src/main/asciidoc/scheduler.adoc @@ -141,6 +141,383 @@ end @enduml ---- +==== Task states in detail + +Each persistent task document carries a `status` field. Transitions are validated by `TaskStateManager` (and a stricter check in `TaskValidationManager` for some API paths). + +[cols="1,3,2",options="header"] +|=== +|Status |Meaning |Typical next states + +|`SCHEDULED` +|Eligible to run when `nextScheduledExecution` is due (and dependencies are satisfied). +|`RUNNING`, `WAITING`, `CANCELLED`, `CRASHED` + +|`WAITING` +|Blocked on unmet `dependsOn` / `waitingOnTasks`, or waiting for a lock/prerequisite. +|`SCHEDULED`, `RUNNING`, `CANCELLED` + +|`RUNNING` +|An executor node prepared the task, acquired the lock (if exclusive), and is executing (or resuming). +|`COMPLETED`, `FAILED`, `CANCELLED`, `CRASHED` + +|`COMPLETED` +|Successful terminal state for the current attempt. One-shot tasks are disabled; periodic tasks usually return to `SCHEDULED`. +|`SCHEDULED` (periodic), `CANCELLED` (race window) + +|`FAILED` +|Attempt failed. May return to `SCHEDULED` for retry while `failureCount <= maxRetries`. +|`SCHEDULED`, terminal `FAILED` when retries exhausted (one-shot) + +|`CRASHED` +|A node failure was detected (expired lock while `RUNNING`, or local shutdown). Visible to the task checker for recovery. +|`RUNNING` (resume), `SCHEDULED` (restart) + +|`CANCELLED` +|Operator or API cancelled the task. Terminal for execution; must not be overwritten by a late `complete()` / `fail()`. +|_(terminal)_ +|=== + +[plantuml] +---- +@startuml +skinparam state { + BackgroundColor<> #EEE +} +state SCHEDULED +state WAITING +state RUNNING +state COMPLETED <> +state FAILED +state CRASHED +state CANCELLED <> + +[*] --> SCHEDULED : create / schedule +SCHEDULED --> RUNNING : prepare + lock +SCHEDULED --> WAITING : deps unmet +WAITING --> SCHEDULED : deps satisfied +WAITING --> RUNNING : prepare after unlock +RUNNING --> COMPLETED : callback.complete() +RUNNING --> FAILED : callback.fail() +RUNNING --> CRASHED : lock expired /\nnode crash / shutdown +RUNNING --> CANCELLED : cancel +FAILED --> SCHEDULED : auto-retry\n(failureCount <= maxRetries) +FAILED --> SCHEDULED : periodic\n(next period, failureCount reset) +CRASHED --> RUNNING : resume()\n(canResume + checkpoint) +CRASHED --> SCHEDULED : restart()\n(retry budget remains) +COMPLETED --> SCHEDULED : periodic reschedule +COMPLETED --> CANCELLED : cancel race window +SCHEDULED --> CANCELLED : cancel +WAITING --> CANCELLED : cancel +@enduml +---- + +==== Exclusive vs parallel execution + +[cols="1,3",options="header"] +|=== +|Builder option |Cluster behaviour + +|`disallowParallelExecution()` (recommended for cluster singletons) +|Distributed lock with optimistic concurrency (`seq_no` / `primary_term`) plus a fencing `lockVersion`. Only one node runs that task instance. + +|`allowParallelExecution()` (opt-in; builder default is `false` / exclusive) +|No exclusive OCC lock — only a non-exclusive `lockOwner` marker. **Multiple executor nodes may run the same due task.** Use only when duplicate side effects are safe. `runOnAllNodes()` enables this automatically. + +|`runOnAllNodes()` +|Every node (including non-executors that still run a checker for these tasks) should execute. Requires `allowParallelExecution=true`. Typical for per-node cache refresh. +|=== + +NOTE: `newTask(...)` defaults to exclusive locking (`allowParallelExecution=false`). Call `allowParallelExecution()` only when duplicates are intentional; `runOnAllNodes()` turns it on for you. + +==== How distributed locks work with Elasticsearch / OpenSearch + +Unomi does **not** use a separate lock service. The `scheduledTask` document *is* the lock. That matters because ES/OS is an asynchronous, eventually-consistent search engine — two nodes can briefly see different versions of the same document unless you design for it. + +===== What is stored on the document + +[cols="1,3",options="header"] +|=== +|Field |Role + +|`lockOwner` +|Node ID that currently holds the exclusive lock (or a non-exclusive marker when parallel is allowed). + +|`lockDate` +|When the lock was taken. Used with `scheduler.lockTimeout` to detect dead holders. + +|`systemMetadata.seq_no` / `primary_term` +|Elasticsearch/OpenSearch optimistic concurrency tokens. Compare-and-set rejects stale writers. + +|`systemMetadata.lockVersion` +|Monotonic fencing token incremented on each successful lock acquire. Survives brief visibility races. + +|`executingNodeId` +|Node currently inside the execution wrapper (set after successful prepare). Used to reclaim a premature `CRASHED` mark if this node is still alive. +|=== + +===== Acquire protocol (exclusive persistent tasks) + +[plantuml] +---- +@startuml +participant "Node A" as A +participant "Node B" as B +database "ES / OS\n(scheduledTask doc)" as ES + +A -> A : affinity window?\n(primary / backup / open field) +A -> ES : GET task (refresh) +A -> A : already locked &\nnot expired? → abort +A -> ES : index with seq_no/primary_term\n+ lockOwner=A + lockVersion++\n(compare-and-set) +alt CAS succeeds + ES --> A : ok + A -> A : sleep verificationDelay (~100ms) + A -> ES : GET again (refresh) + alt still owner + matching lockVersion + A -> A : lock acquired + else lost to peer + A -> A : conflict metric; abort\n(do not run) + end +else CAS conflict + ES --> A : version conflict + A -> A : abort +end + +B -> ES : concurrent CAS with stale seq_no +ES --> B : rejected +@enduml +---- + +Key properties: + +* **Compare-and-set** — two nodes cannot both win the lock write against the same `seq_no`. +* **WAIT_UNTIL / wait_for refresh** (configured for `scheduledTask`) — after a successful lock write, subsequent GETs on other nodes are much more likely to see it immediately. This is still not a distributed mutex primitive; the fencing token + verify step covers residual races. +* **Affinity windows** — reduce contention: the hashed primary node gets ~3s exclusivity, then staggered 500ms backup windows, then an *open field* so a dead primary that remains in `getActiveNodes()` cannot strand the task forever. +* **CRASHED bypasses affinity** — recovery must be able to resume immediately on a survivor. + +===== Lock lease renewal (heartbeating) + +A fixed lock timeout alone cannot distinguish *"the owner died"* from *"the execution is simply taking +longer than `scheduler.lockTimeout`"*. Without renewal, a long-running execution eventually looks expired +to its peers, and crash recovery can then steal the lock — through a perfectly valid compare-and-set — +from a node that is still working, producing a double execution. Guarding on `executingNodeId` instead +would create the opposite failure: only the owner ever clears that field, so a task whose owner really is +dead would be declined by every node forever. + +[plantuml] +---- +@startuml +participant "Node A\n(executing, alive)" as A +participant "Node B\n(recovery)" as B +database "ES / OS\n(scheduledTask doc)" as ES + +A -> ES : acquire lock,\nlockDate = T0 +A -> A : execute() keeps running\npast lockTimeout +B -> ES : GET task +B -> B : lockDate older than\nlockTimeout → "expired" +B -> ES : steal lock (valid CAS) +B -> B : execute() starts +note over A, B #FFDDDD + Without renewal both nodes + now run the task: + double execution +end note +@enduml +---- + +While a node executes a persistent exclusive task it therefore **renews its lease**: a periodic heartbeat +(every `max(100 ms, lockTimeout / 3)`) reloads the document with a realtime GET, verifies it still owns +`lockOwner`, and refreshes `lockDate` with a compare-and-set write. Lock expiry then only ever means the +owner **stopped renewing** — crashed, shut down, or unreachable — never merely "ran long". + +[plantuml] +---- +@startuml +participant "Node A\n(executing)" as A +participant "Node B\n(recovery)" as B +database "ES / OS\n(scheduledTask doc)" as ES + +loop every max(100 ms, lockTimeout / 3) + A -> ES : GET task (realtime) + A -> A : still lockOwner = A ? + A -> ES : CAS: lockDate = now +end +B -> ES : GET task +B -> B : lockDate fresh →\nnot expired → back off +== Node A crashes — heartbeat stops == +B -> ES : GET task (after lockTimeout) +B -> B : lockDate stale → recover\n(CAS acquire, resume/restart) +@enduml +---- + +The heartbeat fails closed on any conflict: if a peer legitimately took over (or the document changed +between the GET and the write), the CAS is rejected and the heartbeat backs off — it never resurrects a +lost lock. Renewal deliberately uses the shutdown-sensitive load/save paths, so heartbeats go inert the +moment shutdown begins and held locks age out normally for peers to recover. And just before a terminal +`COMPLETED` / `FAILED` write, the heartbeat is stopped (waiting out any in-flight tick) so a late renewal +cannot invalidate the terminal compare-and-set. + +===== Renewal timing limits with the default 1 s refresh interval + +Renewal is safe with the ES/OS default index refresh batching (`refresh_interval` = 1 s) because none of +its building blocks depend on search visibility: compare-and-set is enforced at write time on the primary +shard, and ownership checks use realtime GETs that read through the translog. Two operational bounds do +follow from that batching, though: + +* `scheduledTask` writes use the `wait_for` refresh policy, so **each heartbeat write may block up to + ~1 s** waiting for the next refresh cycle. The safety margin is roughly + `lockTimeout − (lockTimeout / 3 + 1 s + scheduling jitter)`: comfortable at the default + `scheduler.lockTimeout=10000`, but do **not** tune `scheduler.lockTimeout` below ~5 s in production or + the refresh wait consumes the whole margin. +* Heartbeats run on the **same scheduler thread pool** (`scheduler.thread.poolSize`) as task executions + and the task checker. If every pool thread is occupied by long-running blocking executors, heartbeats — + like the checker — can starve, the lock expires under a live owner, and the pre-renewal steal scenario + returns. + +[plantuml] +---- +@startuml +concise "Pool threads" as POOL +concise "Heartbeat" as HB +concise "Lock (as seen by peers)" as LOCK + +@0 +POOL is "long executors occupy every thread" +HB is running +LOCK is fresh + +@3 +HB is starved #FFDDDD + +@10 +LOCK is expired #FFDDDD + +@13 +POOL is "still busy" +LOCK is "stolen by peer → double execution" +@enduml +---- + +To stay clear of both bounds: + +* **Size `scheduler.thread.poolSize` above the number of concurrently executing long-running exclusive + tasks**, so a thread is always free for heartbeats and the checker. +* Or break long work into checkpointed steps with a `resume()`-capable executor, so no single execution + blocks a pool thread for long stretches. + +===== Release protocol (safe unlock) + +Unlocking reloads the store document and **re-validates** before clearing `lockOwner`: + +* If this node still owns the lock → clear and save. +* If the lock is expired (dead holder) → a recoverer may clear it. +* If a **peer holds a non-expired lock** → **do not clear** (prevents a late completer from wiping a stealer’s lock after expiry). + +Terminal completion/error paths clear the lock on the **same CAS write** as the status change, so unlock and status stay atomic from the store’s point of view. + +===== Terminal complete / fail under concurrency + +A node finishing work may be racing with cancel or with crash recovery. Before persisting `COMPLETED` / `FAILED` / retry `SCHEDULED`, the execution manager: + +. Reclaims local `CRASHED` → `RUNNING` only if `executingNodeId` is still this node. +. Reloads the store document. +. Aborts if store is `CANCELLED`, or a peer owns `executingNodeId` / a non-expired lock, or store status is no longer `RUNNING`/`CRASHED` (for reclaim). +. Persists with **compare-and-set** (`saveTaskWithRefresh`), including cleared lock fields. + +[plantuml] +---- +@startuml +participant "Node A\n(finishing)" as A +participant "Node B\n(recovery)" as B +database "ES / OS" as ES + +A -> ES : was RUNNING, lockOwner=A +B -> ES : lock expired → CRASHED\nlock cleared +B -> ES : acquire lock (CAS)\nresume/restart +A -> A : callback.complete() +A -> ES : GET latest +A -> A : peer lock / status\nchanged → skip persist +note right of A + Late complete must not + overwrite B's RUNNING + or CANCELLED. +end note +@enduml +---- + +==== Node affinity timeline + +For a due exclusive task with active nodes `[N0, N1, N2]` (sorted) and primary = `hash(taskId) % 3`: + +[cols="1,2,3",options="header"] +|=== +|Delay since `nextScheduledExecution` |Who may acquire |Purpose + +|`0 .. 3000ms` +|Primary only +|Reduce CAS storms; preferred owner runs first + +|`3000 .. 3500ms` +|1st backup (ring order after primary) +|Failover if primary is slow/dead + +|`3500 .. 4000ms` +|2nd backup +|Next staggered window (`500ms` each) + +|`>= 4000ms` (= `3000 + (n-1)*500`) +|Any active node (*open field*) +|Avoid permanent stranding when a dead primary remains in `getActiveNodes()` +|=== + +[plantuml] +---- +@startuml +participant "Due time" as T +participant "Primary" as P +participant "Backup 1" as B1 +participant "Backup 2" as B2 + +T -> P : 0–3s exclusive window +T -> B1 : 3.0–3.5s window +T -> B2 : 3.5–4.0s window +T -> P : >= 4s open field +T -> B1 : >= 4s open field +T -> B2 : >= 4s open field +note over P,B2 + CRASHED recovery skips affinity + and may acquire immediately. +end note +@enduml +---- + +==== Crash recovery + +Every checker tick (~1s) on a node runs recovery: + +. **Running tasks with expired locks** → mark `CRASHED`, clear dead lock, record history, then: + * `canResume` + checkpoint → keep `CRASHED` and call `resume()` + * else if `failureCount <= maxRetries` → reset to `SCHEDULED` and `execute()` + * else leave `CRASHED` +. **Locked `SCHEDULED` / `WAITING` with expired locks** → release lock; if deps allow, reschedule / dispatch. +. **Never** release locks for `RUNNING`/`CRASHED` in the locked-task pass (that races the resume acquire). + +`executingNodeId` lets a still-living node whose lock expired reclaim a false `CRASHED` when its callback fires (`reclaimIfPrematurelyCrashed`). + +==== In-memory (non-persistent) tasks + +`nonPersistent()` tasks live only in the creating node’s memory map. They use a simple in-process lock (no ES CAS). They are ideal for local cache refresh and are **not** cluster-coordinated. + +==== Operational checklist for clustering + +* Set a **stable unique** `scheduler.nodeId` per JVM. +* Put at least one `scheduler.executorNode=true` in the cluster for persistent work. +* Use `disallowParallelExecution()` for singleton side-effecting jobs. +* Keep `scheduler.lockTimeout` at ~5s or above (default 10s): lease renewal keeps long executions safe automatically, but the heartbeat needs headroom over the `wait_for` refresh cycle (see <<_renewal_timing_limits_with_the_default_1_s_refresh_interval,Renewal timing limits>>). +* Size `scheduler.thread.poolSize` above the number of concurrently executing long-running exclusive tasks, so lock heartbeats and the task checker never starve. +* Remember ES/OS visibility: Unomi uses wait-for refresh + OCC + fencing; do not assume search-after-index without those safeguards. +* On cancel: expect `CANCELLED` to win over a late `complete()` from an in-flight thread. + === Core concepts [cols="1,3",options="header"] @@ -166,7 +543,7 @@ end |When `true`, the task is eligible on every node (useful for per-node cache maintenance). |Lock -|Prevents duplicate execution of the same task (or task type when parallel execution is disallowed) across the cluster. +|For exclusive persistent tasks: document-level OCC lock (`lockOwner` + `seq_no`/`primary_term` + `lockVersion`) with verify delay and affinity windows. See the distributed-locks section above. Does **not** apply when `allowParallelExecution` is true. |Checkpoint / resume |Long tasks can save progress and be resumed after a `CRASHED` state via REST or shell. @@ -187,7 +564,7 @@ Tasks are `ScheduledTask` items (`itemType = scheduledTask`). Important fields: |`oneShot` |Run once then stop |`period` / `timeUnit` |Recurrence interval (0 means one-shot) |`fixedRate` |`true` = interval from start; `false` = fixed delay from end -|`allowParallelExecution` |`false` = exclusive lock per task type +|`allowParallelExecution` |Default `false` (exclusive OCC lock). `true` = no exclusivity (multi-node duplicates possible) |`maxRetries` / `retryDelay` |Automatic retry policy after failure |`dependsOn` |Task IDs that must complete first |`checkpointData` / `currentStep` |Progress for resumable work @@ -684,6 +1061,10 @@ long failures = schedulerService.getMetric("tasks.failed"); |Task `FAILED` after retries |Read `lastError` via `task-show` or REST; fix root cause; call retry |Task `CRASHED` |Node failure during run; use resume if checkpoints exist |Duplicate cache refresh tasks |Use `disallowParallelExecution()` and stable task names (see `AbstractMultiTypeCachingService`) +|Same persistent task runs on two nodes |Was `allowParallelExecution()` called explicitly (default is `false`/exclusive)? Use `disallowParallelExecution()`. Check `tasks.lock.conflicts` metrics. +|Task `CRASHED` then stuck |Is an executor registered? Is retry budget exhausted (`failureCount` vs `maxRetries`)? Does the executor implement `canResume`/`resume` when checkpoints exist? +|Lock timeout storms / false CRASHED |Usually a starved heartbeat: check that `scheduler.thread.poolSize` exceeds the number of concurrent long-running tasks. Otherwise increase `scheduler.lockTimeout` (keep ≥ ~5s), or checkpoint + resume for long work. Verify clocks are roughly NTP-aligned across nodes. +|Cancelled task becomes COMPLETED |Should not happen after Unomi 3.1 terminal CAS guards; if seen, capture `seq_no` history and node logs around `Skipping terminal transition`. |Tenant data wrong in background job |Missing `executeAsTenant` / security subject setup in executor |=== diff --git a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java index f9e457fc83..b572ab129d 100644 --- a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java +++ b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java @@ -33,6 +33,27 @@ */ public interface PersistenceService { + /** + * {@link Item#getSystemMetadata(String)} key holding this backend's sequence number for an + * item, populated by every {@code PersistenceService} implementation after a successful + * {@code load()}/{@code save()}. Paired with {@link #SYSTEM_METADATA_PRIMARY_TERM}, this + * value changes on every successful write and can be treated as an opaque, monotonically + * -changing fencing token — no application-level version counter is needed on top of it for + * compare-and-set or distributed-locking use cases (see {@link #save(Item, Boolean, Boolean)}). + * This is a persistence-implementation detail, not a generic {@link Item} concept, which is + * why it lives here rather than on {@link Item} itself. Every current and planned + * {@code PersistenceService} implementation (Elasticsearch, OpenSearch) uses this exact key + * name, by convention, so it is declared once here rather than duplicated per implementation. + */ + String SYSTEM_METADATA_SEQ_NO = "seq_no"; + + /** + * {@link Item#getSystemMetadata(String)} key holding this backend's primary term for an item. + * Must always be supplied together with {@link #SYSTEM_METADATA_SEQ_NO} as the + * compare-and-set precondition; the pair identifies a specific document generation. + */ + String SYSTEM_METADATA_PRIMARY_TERM = "primary_term"; + /** * Returns the unique name of this persistence backend implementation. * @@ -108,6 +129,23 @@ public interface PersistenceService { /** * Persists the specified Item in the context server. + *

+ * When {@code alwaysOverwrite} is {@code false}, this becomes a compare-and-set (CAS) write: + * the backend applies it only if the item's current {@link #SYSTEM_METADATA_SEQ_NO}/ + * {@link #SYSTEM_METADATA_PRIMARY_TERM} system metadata (typically populated by a prior + * {@code load()} or {@code save()} on the same item) still match the backend's current state + * for that document. A {@code true} return is itself authoritative proof the write applied — + * callers do not need a follow-up read to double-check, since every implementation applies + * this precondition atomically. On success, the item's system metadata is updated in place + * with the new post-write seq_no/primary_term, which callers may treat as an opaque, + * monotonically-changing fencing token (e.g. for distributed-locking use cases — see + * {@code TaskLockManager#acquireLock}). A {@code false} return means the precondition did not + * match (the caller's view was stale / lost a race); nothing was written, and the caller + * should reload and retry rather than treat it as an application error. + *

+ * When {@code alwaysOverwrite} is {@code true} (or {@code null}, which falls back to the + * implementation's configured default), the write is unconditional and always succeeds + * (barring backend errors), regardless of the item's system metadata. * * @param item the item to persist * @param useBatching whether to use batching or not for saving the item. If activating there may be a delay between diff --git a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java index 1c0235b707..bb37c19852 100644 --- a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java +++ b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java @@ -70,6 +70,18 @@ public class ConditionContextHelper { */ private static final Object RESOLUTION_ERROR = new Object(); + /** + * Expected-type names that {@link #isTypeCompatible(String, String)} can actually reason + * about. A mismatch against one of these is a genuine, actionable signal. Anything else is + * a custom/registry-only type (e.g. "comparisonOperator") whose real check lives in a + * {@link org.apache.unomi.api.services.ValueTypeValidator} that may simply not be + * registered in this context (plain unit tests with no OSGi container) — in that case we + * have no basis to call it a mismatch, so it must not be logged at the same severity as a + * confirmed one. + */ + private static final Set KNOWN_PRIMITIVE_TYPES = new HashSet<>(Arrays.asList( + "string", "integer", "long", "float", "double", "boolean", "date")); + private static final Map FOLD_MAPPING = new HashMap<>(); static { @@ -511,7 +523,16 @@ private static void validateSingleParameterValue( "Parameter '%s' in condition type '%s' resolved to type '%s' but expected '%s'", paramName, conditionTypeId, actualType, expectedType); - LOGGER.warn(message + " (value: {})", resolvedValue); + // Only a mismatch against a type isTypeCompatible() truly understands is a + // confirmed problem worth a WARN. expectedType naming a custom/registry-only type + // (e.g. "comparisonoperator") with no registered validator is inconclusive, not + // confirmed — see KNOWN_PRIMITIVE_TYPES javadoc. + if (KNOWN_PRIMITIVE_TYPES.contains(expectedType)) { + LOGGER.warn(message + " (value: {})", resolvedValue); + } else { + LOGGER.debug(message + " (value: {}); no ValueTypeValidator registered for '{}' " + + "to confirm this is a real mismatch", resolvedValue, expectedType); + } traceParameterTypeMismatch(tracerService, paramName, expectedType, actualType, resolvedValue, conditionTypeId); } diff --git a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/MergeProfilesOnPropertyAction.java b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/MergeProfilesOnPropertyAction.java index 83db5fee2a..a29b39f302 100644 --- a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/MergeProfilesOnPropertyAction.java +++ b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/MergeProfilesOnPropertyAction.java @@ -280,7 +280,12 @@ public void execute(ScheduledTask task, TaskExecutor.TaskStatusCallback callback // Register the executor schedulerService.registerTaskExecutor(mergeProfilesReassignDataExecutor); - // Create a one-shot task for async data reassignment + // Create a one-shot task for async data reassignment. Runs allowParallelExecution(): + // the underlying operation (bulk update-by-query-and-script / anonymizeBrowsingData) is + // idempotent, and as a one-shot task it gets no natural retry if a single exclusive-lock + // acquisition attempt is lost to a transient race - unlike a periodic task, which simply + // tries again next period. Exclusivity buys nothing here but adds a real risk of the + // rewrite silently never running. schedulerService.newTask(taskType) .withParameters(Map.of( "anonymousBrowsing", anonymousBrowsing, @@ -290,6 +295,7 @@ public void execute(ScheduledTask task, TaskExecutor.TaskStatusCallback callback )) .withInitialDelay(1000, TimeUnit.MILLISECONDS) .asOneShot() + .allowParallelExecution() .schedule(); } diff --git a/plugins/request/src/main/java/org/apache/unomi/plugins/request/useragent/UserAgent.java b/plugins/request/src/main/java/org/apache/unomi/plugins/request/useragent/UserAgent.java index 6980b3a311..bf0e71314f 100644 --- a/plugins/request/src/main/java/org/apache/unomi/plugins/request/useragent/UserAgent.java +++ b/plugins/request/src/main/java/org/apache/unomi/plugins/request/useragent/UserAgent.java @@ -101,6 +101,6 @@ public String toString() { sb.append("device.category: " + this.getDeviceCategory() + " \n}"); sb.append("device.brand: " + this.getDeviceBrand() + " \n}"); sb.append("device.name: " + this.getDeviceName() + " \n}"); - return super.toString(); + return sb.toString(); } } diff --git a/plugins/request/src/test/java/org/apache/unomi/plugins/request/actions/UserAgentDetectorTest.java b/plugins/request/src/test/java/org/apache/unomi/plugins/request/actions/UserAgentDetectorTest.java index 019fe92aed..8701e8d037 100644 --- a/plugins/request/src/test/java/org/apache/unomi/plugins/request/actions/UserAgentDetectorTest.java +++ b/plugins/request/src/test/java/org/apache/unomi/plugins/request/actions/UserAgentDetectorTest.java @@ -19,8 +19,10 @@ import java.util.ArrayList; import java.util.List; import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.Future; import org.apache.unomi.plugins.request.useragent.UserAgent; import org.apache.unomi.plugins.request.useragent.UserAgentDetectorServiceImpl; @@ -32,6 +34,9 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; + @FixMethodOrder(MethodSorters.NAME_ASCENDING) public class UserAgentDetectorTest { @@ -62,31 +67,67 @@ public void testFirstUserAgentDetection() { long end = System.currentTimeMillis(); LOGGER.info("Duration user agent parsing (in msec) > {}", end - start); LOGGER.info(agent.toString()); + + assertEquals("Mobile", agent.getOperatingSystemFamily()); + assertEquals("Android", agent.getOperatingSystemName()); + assertEquals("Chrome", agent.getUserAgentName()); + assertEquals("53.0.2785.124", agent.getUserAgentVersion()); + assertEquals("Phone", agent.getDeviceCategory()); + assertEquals("Google", agent.getDeviceBrand()); + assertEquals("Google Nexus 6", agent.getDeviceName()); } @Test - public void testUserAgentDetectionPerformance() throws InterruptedException { - int workerCount = 5000000; - ExecutorService executorService = Executors.newFixedThreadPool(3000); + public void testDesktopUserAgentDetection() { + String header = "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.124 Safari/537.36"; - for (int cpt = 1; cpt < 6; cpt++) { - LOGGER.info("Execution {}/5", cpt); - executeWorker(executorService, workerCount); - } + UserAgent agent = this.userAgentDetectorService.parseUserAgent(header); + + assertEquals("Desktop", agent.getOperatingSystemFamily()); + assertEquals("Windows NT", agent.getOperatingSystemName()); + assertEquals("Chrome", agent.getUserAgentName()); + assertEquals("91.0.4472.124", agent.getUserAgentVersion()); + assertEquals("Desktop", agent.getDeviceCategory()); } - private void executeWorker(ExecutorService executorService, int workerCount) throws InterruptedException { - List> callables = new ArrayList<>(workerCount); - long startTime = System.currentTimeMillis(); - for (int i = 0; i < workerCount; i++) { - callables.add(new AgentWorker(this.userAgentDetectorService)); + /** + * Concurrency smoke test: the analyzer is shared across threads in production (a single + * OSGi service instance handling concurrent requests), so this exercises parseUserAgent() + * under concurrent access and verifies every call returns a correctly parsed result - + * not just "didn't throw". Iteration/thread counts are intentionally small (this is a + * correctness-under-concurrency check, not a throughput benchmark); the original version + * ran 25,000,000 parses across a 3000-thread pool with no assertions at all, which made + * this test one of the slowest in the module while verifying nothing. + */ + @Test + public void testUserAgentDetectionUnderConcurrency() throws InterruptedException { + int workerCount = 200; + ExecutorService executorService = Executors.newFixedThreadPool(20); + try { + List> callables = new ArrayList<>(workerCount); + for (int i = 0; i < workerCount; i++) { + callables.add(new AgentWorker(this.userAgentDetectorService)); + } + long startTime = System.currentTimeMillis(); + List> results = executorService.invokeAll(callables); + long totalTime = System.currentTimeMillis() - startTime; + LOGGER.info("{} concurrent parses completed in {}ms", workerCount, totalTime); + + for (Future result : results) { + UserAgent agent = result.get(); + assertNotNull("Every concurrent parse must return a result", agent); + assertEquals("Chrome", agent.getUserAgentName()); + assertEquals("Android", agent.getOperatingSystemName()); + assertEquals("Google Nexus 6", agent.getDeviceName()); + } + } catch (ExecutionException e) { + throw new AssertionError("A concurrent parseUserAgent() call failed", e); + } finally { + executorService.shutdown(); } - executorService.invokeAll(callables); - long totalTime = System.currentTimeMillis() - startTime; - LOGGER.info("AgentWorker workers completed execution in {}ms", totalTime); } - private class AgentWorker implements Callable { + private static class AgentWorker implements Callable { String header = "Mozilla/5.0 (Linux; Android 7.0; Nexus 6 Build/NBD90Z) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.124 Mobile Safari/537.36"; UserAgentDetectorServiceImpl service; @@ -96,9 +137,8 @@ public AgentWorker(UserAgentDetectorServiceImpl userAgentDetectorService) { } @Override - public Object call() { - this.service.parseUserAgent(header); - return null; + public UserAgent call() { + return this.service.parseUserAgent(header); } } diff --git a/services/src/main/java/org/apache/unomi/services/impl/scheduler/PersistenceSchedulerProvider.java b/services/src/main/java/org/apache/unomi/services/impl/scheduler/PersistenceSchedulerProvider.java index bde2905ac2..77a3838073 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/scheduler/PersistenceSchedulerProvider.java +++ b/services/src/main/java/org/apache/unomi/services/impl/scheduler/PersistenceSchedulerProvider.java @@ -141,6 +141,13 @@ public void preDestroy() { List tasks = findTasksByLockOwner(nodeId); for (ScheduledTask task : tasks) { try { + // Do not unlock RUNNING/CRASHED here — that races SchedulerServiceImpl.preDestroy + // which marks this node's RUNNING work CRASHED. Unlocking first would let a peer + // steal still-live JVM work. Service preDestroy clears locks after the CRASH mark. + if (task.getStatus() == ScheduledTask.TaskStatus.RUNNING + || task.getStatus() == ScheduledTask.TaskStatus.CRASHED) { + continue; + } if (lockManager != null) { lockManager.releaseLock(task); } @@ -148,7 +155,7 @@ public void preDestroy() { LOGGER.debug("Error releasing lock for task {} during shutdown: {}", task.getItemId(), e.getMessage()); } } - LOGGER.debug("Task locks released"); + LOGGER.debug("Non-running task locks released"); } catch (Exception e) { // During shutdown, services may be unavailable - this is expected LOGGER.debug("Error finding locked tasks during shutdown (this is expected if services are shutting down): {}", e.getMessage()); @@ -188,7 +195,11 @@ public List findEnabledScheduledOrWaitingTasks() { statusCondition.setParameter("comparisonOperator", "in"); statusCondition.setParameter("propertyValues", Arrays.asList( ScheduledTask.TaskStatus.SCHEDULED, - ScheduledTask.TaskStatus.WAITING + ScheduledTask.TaskStatus.WAITING, + // CRASHED must stay visible: recovery may fail to dispatch immediately + // (e.g. previous node's dispatch claim still held), and without this the + // task would remain stranded forever. + ScheduledTask.TaskStatus.CRASHED )); Condition andCondition = new Condition(SchedulerProvider.BOOLEAN_CONDITION_TYPE); @@ -328,9 +339,13 @@ public boolean saveTask(ScheduledTask task) { if (task.isPersistent()) { try { - persistenceService.save(task); - LOGGER.debug("Saved task {} to persistence", task.getItemId()); - return true; + boolean saved = persistenceService.save(task); + if (saved) { + LOGGER.debug("Saved task {} to persistence", task.getItemId()); + } else { + LOGGER.debug("Persistence rejected save for task {} (likely OCC conflict)", task.getItemId()); + } + return saved; } catch (Exception e) { LOGGER.error("Error saving task {} to persistence", task.getItemId(), e); return false; @@ -341,6 +356,28 @@ public boolean saveTask(ScheduledTask task) { } } + @Override + public boolean saveTaskCompareAndSet(ScheduledTask task) { + if (task == null) { + return false; + } + if (!task.isPersistent()) { + LOGGER.error("Can't handle in-memory task compare-and-set !"); + return false; + } + try { + // alwaysOverwrite=false enables seq_no / primary_term conflict detection + boolean saved = persistenceService.save(task, false, false); + if (!saved) { + LOGGER.debug("Compare-and-set rejected for task {}", task.getItemId()); + } + return saved; + } catch (Exception e) { + LOGGER.error("Error compare-and-set saving task {} to persistence", task.getItemId(), e); + return false; + } + } + @Override public List getActiveNodes() { Set activeNodes = new HashSet<>(); diff --git a/services/src/main/java/org/apache/unomi/services/impl/scheduler/SchedulerProvider.java b/services/src/main/java/org/apache/unomi/services/impl/scheduler/SchedulerProvider.java index d2bc83bb63..32057cf60a 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/scheduler/SchedulerProvider.java +++ b/services/src/main/java/org/apache/unomi/services/impl/scheduler/SchedulerProvider.java @@ -123,6 +123,21 @@ public interface SchedulerProvider { */ boolean saveTask(ScheduledTask task); + /** + * Saves a task using optimistic concurrency (compare-and-set on {@code seq_no}/ + * {@code primary_term}). Used by distributed lock acquisition so two nodes cannot + * both believe they hold the lock after a concurrent write. + * + *

Callers: {@code SchedulerServiceImpl.saveTaskWithRefresh}; implemented by + * {@code PersistenceSchedulerProvider.saveTaskCompareAndSet}. + * + * @param task The task to save (must carry the expected seq_no / primary_term) + * @return true if the compare-and-set succeeded, false on version conflict + */ + default boolean saveTaskCompareAndSet(ScheduledTask task) { + return saveTask(task); + } + /** * Returns the list of currently active cluster nodes. * This is used for node affinity in the distributed locking mechanism. diff --git a/services/src/main/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceImpl.java index dc305c0b4d..3c98963fc3 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceImpl.java @@ -88,6 +88,8 @@ public class SchedulerServiceImpl implements SchedulerService { private final AtomicBoolean running = new AtomicBoolean(false); private final Map> waitingNonPersistentTasks = new ConcurrentHashMap<>(); private final AtomicBoolean checkTasksRunning = new AtomicBoolean(false); + /** Diagnostic-only monotonic tick counter to correlate LOCK-DIAG log lines across managers. */ + private final java.util.concurrent.atomic.AtomicLong diagTickCounter = new java.util.concurrent.atomic.AtomicLong(); // Manager instances - will be injected by Blueprint private TaskStateManager stateManager; @@ -611,6 +613,9 @@ public void setExecutionManager(TaskExecutionManager executionManager) { */ public void setRecoveryManager(TaskRecoveryManager recoveryManager) { this.recoveryManager = recoveryManager; + if (recoveryManager != null) { + recoveryManager.setExecutorNode(this.executorNode); + } } /** @@ -782,10 +787,12 @@ public void postConstruct() { executionManager.setSchedulerService(this); recoveryManager.setSchedulerService(this); + // Start the task checker on every node. Non-executor nodes only dispatch tasks with + // runOnAllNodes=true (filtered in shouldExecuteTask); without a checker those nodes + // would never re-poll recurring runOnAllNodes work after the initial schedule. + running.set(true); + executionManager.startTaskChecker(this::checkTasks); if (executorNode) { - running.set(true); - // Start task checking thread using the execution manager - executionManager.startTaskChecker(this::checkTasks); // Queue task purge initialization instead of calling directly queuePendingOperation(OperationType.INITIALIZE_TASK_PURGE, "Initialize task purge"); } @@ -888,8 +895,16 @@ public void preDestroy() { try { stateManager.updateTaskState(task, ScheduledTask.TaskStatus.CRASHED, "Interrupted by scheduler shutdown", nodeId); + // Clear lock in the same write so peers do not wait for lockTimeout + // and PersistenceSchedulerProvider.preDestroy need not unlock RUNNING. + task.setLockOwner(null); + task.setLockDate(null); if (task.isPersistent() && persistenceProvider != null) { - persistenceProvider.saveTask(task); + if (!persistenceProvider.saveTask(task)) { + LOGGER.warn("Failed to persist CRASHED state for task {} during shutdown; " + + "store may still show it RUNNING with a dead lock owner until lockTimeout", + task.getItemId()); + } } } catch (Exception e) { LOGGER.warn("Error marking task {} as crashed during shutdown: {}", task.getItemId(), e.getMessage()); @@ -897,7 +912,7 @@ public void preDestroy() { } } } catch (Exception e) { - LOGGER.debug("Error marking running tasks as crashed during shutdown: {}", e.getMessage()); + LOGGER.warn("Error marking running tasks as crashed during shutdown", e); } } @@ -934,7 +949,7 @@ public boolean isShutdownNow() { } void checkTasks() { - if (shutdownNow || !running.get() || checkTasksRunning.get() || !executorNode) { + if (shutdownNow || !running.get() || checkTasksRunning.get()) { return; } @@ -942,6 +957,9 @@ void checkTasks() { return; } + long tick = diagTickCounter.incrementAndGet(); + long diagStart = System.currentTimeMillis(); + LOGGER.debug("LOCK-DIAG node {} : checkTasks() tick #{} starting", nodeId, tick); try { // Skip task processing during shutdown if (shutdownNow) { @@ -964,12 +982,16 @@ void checkTasks() { tasks.addAll(persistentTasks); } } + LOGGER.debug("LOCK-DIAG node {} : checkTasks() tick #{} findEnabledScheduledOrWaitingTasks() " + + "(search-based) returned {} persistent task(s)", + nodeId, tick, tasks.size()); // Also check in-memory tasks List inMemoryTasks = nonPersistentTasks.values().stream() .filter(task -> task.isEnabled() && (task.getStatus() == ScheduledTask.TaskStatus.SCHEDULED || - task.getStatus() == ScheduledTask.TaskStatus.WAITING)) + task.getStatus() == ScheduledTask.TaskStatus.WAITING || + task.getStatus() == ScheduledTask.TaskStatus.CRASHED)) .collect(Collectors.toList()); // Add in-memory tasks to the list of tasks to check @@ -996,6 +1018,8 @@ void checkTasks() { } catch (Exception e) { LOGGER.error("Error checking tasks", e); } finally { + LOGGER.debug("LOCK-DIAG node {} : checkTasks() tick #{} finished in {} ms", + nodeId, tick, System.currentTimeMillis() - diagStart); checkTasksRunning.set(false); } } @@ -1033,19 +1057,39 @@ private Map> groupTasksByType(List ta private void processTaskGroup(String taskType, List tasks) { TaskExecutor executor = executorRegistry.getExecutor(taskType); if (executor == null) { + LOGGER.debug("LOCK-DIAG node {} : processTaskGroup(type={}) no executor registered, skipping {} task(s)", + nodeId, taskType, tasks.size()); return; } - // Check if any task of this type is running with a valid lock - boolean hasRunningTask = hasRunningTaskOfType(taskType); - if (!hasRunningTask) { - // Get the first task that should execute - for (ScheduledTask task : tasks) { - if (shouldExecuteTask(task)) { - // All tasks here are persistent since they come from persistence service query - executionManager.executeTask(task, executor); - break; - } + for (ScheduledTask task : tasks) { + if (shutdownNow) { + return; + } + boolean due = shouldExecuteTask(task); + LOGGER.debug("LOCK-DIAG [{}] node {} : processTaskGroup(type={}) shouldExecuteTask={}, status={}, " + + "lockOwner={}, allowParallelExecution={}", + task.getItemId(), nodeId, taskType, due, task.getStatus(), task.getLockOwner(), + task.isAllowParallelExecution()); + if (!due) { + continue; + } + // Type-level mutex is best-effort across nodes (TOCTOU vs peer checkers). + // Re-query immediately before dispatch to shrink the race; the hard guarantee + // for a single task instance remains the per-task OCC lock. + if (!task.isAllowParallelExecution() && hasRunningTaskOfType(taskType)) { + LOGGER.debug("LOCK-DIAG [{}] node {} : processTaskGroup(type={}) skipping - another task of " + + "this type is already RUNNING on this node", + task.getItemId(), nodeId, taskType); + continue; + } + LOGGER.debug("LOCK-DIAG [{}] node {} : processTaskGroup(type={}) dispatching via " + + "executionManager.executeTask()", + task.getItemId(), nodeId, taskType); + executionManager.executeTask(task, executor); + if (!task.isAllowParallelExecution()) { + // One exclusive task of this type at a time on this node + break; } } } @@ -1133,11 +1177,17 @@ private boolean shouldExecuteTask(ScheduledTask task) { } } - // For waiting tasks, they are already ordered by creation date + // WAITING tasks run once their dependencies are satisfied (checked above). + // They are ordered by creation date in sortTasksByPriority. if (task.getStatus() == ScheduledTask.TaskStatus.WAITING) { return true; } + // CRASHED tasks are due for recovery/resume immediately + if (task.getStatus() == ScheduledTask.TaskStatus.CRASHED) { + return true; + } + // For scheduled tasks, check execution timing if (task.getStatus() == ScheduledTask.TaskStatus.SCHEDULED) { return isTaskDueForExecution(task); @@ -1158,14 +1208,19 @@ private boolean isTaskDueForExecution(ScheduledTask task) { return true; // Execute immediately if no initial delay } - // For periodic tasks, check next scheduled execution - if (!task.isOneShot() && task.getPeriod() > 0) { - Date nextExecution = task.getNextScheduledExecution(); - return nextExecution != null && - System.currentTimeMillis() >= nextExecution.getTime(); - } - - return false; + // Already executed at least once: the task is due again when a next execution is + // scheduled and its time has been reached. This covers periodic tasks (next period) + // as well as pending retries of one-shot tasks (handleTaskError() moves the failed + // task back to SCHEDULED with nextScheduledExecution = failure time + retry delay). + // Including one-shot retries here makes the periodic task checker a safety net when + // the in-process retry dispatch is lost, e.g. because it fired while the previous + // execution was still releasing its dispatch claim, or because the node that + // scheduled the retry died. Completed one-shot tasks are never re-dispatched: + // completion clears nextScheduledExecution and disables the task, and tasks whose + // retries are exhausted stay in FAILED state which the checker does not select. + Date nextExecution = task.getNextScheduledExecution(); + return nextExecution != null && + System.currentTimeMillis() >= nextExecution.getTime(); } @Override @@ -1189,6 +1244,8 @@ private void scheduleTaskInternal(ScheduledTask task) { } Map existingTasks = new HashMap<>(); + // Include the task itself so cycle detection can walk its dependsOn graph + existingTasks.put(task.getItemId(), task); if (task.getDependsOn() != null) { for (String dependencyId : task.getDependsOn()) { ScheduledTask dependency = getTask(dependencyId); @@ -1200,6 +1257,21 @@ private void scheduleTaskInternal(ScheduledTask task) { validationManager.validateTask(task, existingTasks); + // Enforce dependencies: enter WAITING until all dependsOn tasks have COMPLETED + if (task.getDependsOn() != null && !task.getDependsOn().isEmpty()) { + task.setWaitingOnTasks(new HashSet<>(task.getDependsOn())); + if (!stateManager.canRescheduleTask(task, existingTasks)) { + stateManager.updateTaskState(task, TaskStatus.WAITING, null, nodeId); + if (!saveTask(task)) { + LOGGER.error("Failed to save waiting task: {}", task.getItemId()); + } + LOGGER.debug("Task {} waiting on dependencies {}", task.getItemId(), task.getDependsOn()); + return; + } + // Dependencies already satisfied — clear waiting set and proceed to schedule + task.setWaitingOnTasks(null); + } + // Store task if (!saveTask(task)) { LOGGER.error("Failed to save task: {}", task.getItemId()); @@ -1310,7 +1382,20 @@ public List getAllTasks() { @Override public ScheduledTask getTask(String taskId) { - if (shutdownNow) { + return getTask(taskId, false); + } + + /** + * Loads a task by id. + * + * @param taskId the task id + * @param allowDuringShutdown when true, load even if {@code shutdownNow} is set + * (needed so lock release can re-read store state without picking up a + * caller-mutated SCHEDULED/FAILED retry view) + * @return the task, or null if not found + */ + public ScheduledTask getTask(String taskId, boolean allowDuringShutdown) { + if (!allowDuringShutdown && shutdownNow) { return null; } @@ -1497,6 +1582,9 @@ public void setThreadPoolSize(int threadPoolSize) { */ public void setExecutorNode(boolean executorNode) { this.executorNode = executorNode; + if (recoveryManager != null) { + recoveryManager.setExecutorNode(executorNode); + } } /** @@ -1506,6 +1594,9 @@ public void setExecutorNode(boolean executorNode) { */ public void setLockTimeout(long lockTimeout) { this.lockTimeout = lockTimeout; + if (lockManager != null) { + lockManager.setLockTimeout(lockTimeout); + } } /** @@ -1544,9 +1635,9 @@ public static long getTimeDiffInSeconds(int hourInUtc, ZonedDateTime now) { @Override public void recoverCrashedTasks() { if (areServicesReady()) { - if (executorNode) { - recoveryManager.recoverCrashedTasks(); - } + // Recovery may mark CRASHED / clear locks on any node; dispatch is gated inside + // TaskRecoveryManager by executorNode || runOnAllNodes. + recoveryManager.recoverCrashedTasks(); } else { queuePendingOperation(OperationType.RECOVER_CRASHED_TASKS, "Recover crashed tasks"); } @@ -1601,9 +1692,11 @@ private void resumeTaskInternal(String taskId) { if (task != null && task.getStatus() == ScheduledTask.TaskStatus.CRASHED) { TaskExecutor executor = executorRegistry.getExecutor(task.getTaskType()); if (executor != null && executor.canResume(task)) { - stateManager.updateTaskState(task, ScheduledTask.TaskStatus.SCHEDULED, null, nodeId); + // Keep CRASHED so the execution wrapper invokes executor.resume() metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_RESUMED); - scheduleTaskInternal(task); + if (lockManager.acquireLock(task)) { + executionManager.executeTask(task, executor); + } } } } @@ -1709,7 +1802,21 @@ private boolean updateTaskInPersistence(ScheduledTask task) { */ @Override public boolean saveTask(ScheduledTask task) { - if (task == null || shutdownNow) { + return saveTask(task, false); + } + + /** + * Saves a task, optionally allowing persistence while {@code shutdownNow} is set. + * Lock release during {@link #preDestroy()} / {@link #simulateCrash()} must still + * reach the store — otherwise deep-copy persistence leaves a stale lock that blocks + * the next scheduler instance until lock timeout. + * + * @param task the task to save + * @param allowDuringShutdown when true, persist even if the scheduler is shutting down + * @return true if saved successfully + */ + public boolean saveTask(ScheduledTask task, boolean allowDuringShutdown) { + if (task == null || (!allowDuringShutdown && shutdownNow)) { return false; } @@ -1720,9 +1827,11 @@ public boolean saveTask(ScheduledTask task) { } try { - persistenceProvider.saveTask(task); - LOGGER.debug("Saved task {} to persistence", task.getItemId()); - return true; + boolean saved = persistenceProvider.saveTask(task); + if (saved) { + LOGGER.debug("Saved task {} to persistence", task.getItemId()); + } + return saved; } catch (Exception e) { LOGGER.error("Error saving task {} to persistence", task.getItemId(), e); return false; @@ -1736,12 +1845,14 @@ public boolean saveTask(ScheduledTask task) { @Override public ScheduledTask createRecurringTask(String taskType, long period, TimeUnit timeUnit, Runnable runnable, boolean persistent) { - return newTask(taskType) + TaskBuilder builder = newTask(taskType) .withPeriod(period, timeUnit) .withFixedRate() - .withSimpleExecutor(runnable) - .nonPersistent() - .schedule(); + .withSimpleExecutor(runnable); + if (!persistent) { + builder.nonPersistent(); + } + return builder.schedule(); } @Override @@ -1922,7 +2033,21 @@ public void refreshTasks() { * @return true if the operation was successful */ public boolean saveTaskWithRefresh(ScheduledTask task) { - if (task == null || shutdownNow) { + return saveTaskWithRefresh(task, false); + } + + /** + * Saves a task with immediate refresh, optionally allowing persistence while + * {@code shutdownNow} is set. Lock release during {@link #preDestroy()} / + * {@link #simulateCrash()} must still go through compare-and-set — a blind overwrite + * could clobber a lock a peer legitimately acquired between our read and this write. + * + * @param task the task to save + * @param allowDuringShutdown when true, persist even if the scheduler is shutting down + * @return true if the operation was successful + */ + public boolean saveTaskWithRefresh(ScheduledTask task, boolean allowDuringShutdown) { + if (task == null || (!allowDuringShutdown && shutdownNow)) { return false; } @@ -1933,16 +2058,16 @@ public boolean saveTaskWithRefresh(ScheduledTask task) { } try { - // Save with optimistic concurrency control - // Refresh is now handled automatically by the refresh policy - return persistenceProvider.saveTask(task); + // Compare-and-set on seq_no/primary_term so distributed lock acquisition + // fails closed when two nodes race. Refresh is handled by refresh policy. + return persistenceProvider.saveTaskCompareAndSet(task); } catch (Exception e) { LOGGER.error("Error saving task {}", task.getItemId(), e); return false; } } else { // For non-persistent tasks, just save normally - return saveTask(task); + return saveTask(task, allowDuringShutdown); } } @@ -2031,7 +2156,7 @@ public static class TaskBuilder implements SchedulerService.TaskBuilder { private TimeUnit timeUnit = TimeUnit.MILLISECONDS; private boolean fixedRate = true; private boolean oneShot = false; - private boolean allowParallelExecution = true; + private boolean allowParallelExecution = false; private TaskExecutor executor; private boolean persistent = true; private boolean runOnAllNodes = false; @@ -2089,6 +2214,12 @@ public TaskBuilder disallowParallelExecution() { return this; } + @Override + public TaskBuilder allowParallelExecution() { + this.allowParallelExecution = true; + return this; + } + @Override public TaskBuilder withExecutor(TaskExecutor executor) { this.executor = executor; @@ -2125,6 +2256,8 @@ public TaskBuilder nonPersistent() { @Override public TaskBuilder runOnAllNodes() { this.runOnAllNodes = true; + // Validation requires allowParallelExecution when runOnAllNodes is set. + this.allowParallelExecution = true; return this; } @@ -2174,6 +2307,13 @@ public ScheduledTask schedule() { schedulerService.registerTaskExecutor(executor); } + // period=0 means one-shot; TaskBuilder defaults oneShot=false, and createTask + // calls setOneShot after setPeriod which would otherwise clear the auto-flag + // set by ScheduledTask.setPeriod(0). + if (period == 0) { + oneShot = true; + } + // Check for existing system tasks of the same type if this is a system task if (systemTask) { List existingTasks = schedulerService.getTasksByType(taskType, 0, 1, null).getList(); diff --git a/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskExecutionManager.java b/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskExecutionManager.java index b0155ad8b0..27178469bb 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskExecutionManager.java +++ b/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskExecutionManager.java @@ -21,6 +21,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.util.ArrayList; import java.util.Date; import java.util.Map; import java.util.Set; @@ -34,6 +35,7 @@ public class TaskExecutionManager { private static final Logger LOGGER = LoggerFactory.getLogger(TaskExecutionManager.class); private static final int MIN_THREAD_POOL_SIZE = 4; private static final long TASK_CHECK_INTERVAL = 1000; // 1 second + private static final long MIN_LOCK_RENEWAL_INTERVAL_MS = 100; private String nodeId; private ScheduledExecutorService scheduler; @@ -43,6 +45,7 @@ public class TaskExecutionManager { private TaskMetricsManager metricsManager; private TaskHistoryManager historyManager; private final Map> executingTasksByType; + private final Map activeLockRenewals = new ConcurrentHashMap<>(); private final AtomicBoolean running = new AtomicBoolean(false); private ScheduledFuture taskCheckerFuture; private SchedulerServiceImpl schedulerService; @@ -231,9 +234,15 @@ public void executeTask(ScheduledTask task, TaskExecutor executor) { // so a second poll tick landing in that gap could otherwise see a stale non-RUNNING status and // dispatch the same task again. Set.add() is atomic, so only one caller can win this race. if (!executingSet.add(task.getItemId())) { + LOGGER.debug("LOCK-DIAG [{}] node {} : executeTask() dispatch claim REJECTED - task already " + + "claimed by an in-flight dispatch (this is the duplicate-dispatch guard working)", + task.getItemId(), nodeId); LOGGER.debug("Node {} : Task {} is already dispatched for execution, skipping duplicate dispatch", nodeId, task.getItemId()); return; } + LOGGER.debug("LOCK-DIAG [{}] node {} : executeTask() dispatch claim ACQUIRED, scheduling wrapper " + + "(caller thread={})", + task.getItemId(), nodeId, Thread.currentThread().getName()); TaskExecutor.TaskStatusCallback statusCallback = createStatusCallback(task); Runnable taskWrapper = createTaskWrapper(task, executor, statusCallback); @@ -261,26 +270,57 @@ public void executeTask(ScheduledTask task, TaskExecutor executor) { * @return true if the task is ready to run */ public boolean prepareForExecution(ScheduledTask task) { + LOGGER.debug("LOCK-DIAG [{}] node {} : prepareForExecution() starting on thread {}, status={}", + task.getItemId(), nodeId, Thread.currentThread().getName(), task.getStatus()); if (!task.isEnabled()) { LOGGER.debug("Task {} is disabled", task.getItemId()); return false; } - // Only execute tasks that are in SCHEDULED state (or CRASHED for recovery) + // SCHEDULED (normal), CRASHED (recovery/resume), WAITING (deps just satisfied) if (task.getStatus() != ScheduledTask.TaskStatus.SCHEDULED && - task.getStatus() != ScheduledTask.TaskStatus.CRASHED) { + task.getStatus() != ScheduledTask.TaskStatus.CRASHED && + task.getStatus() != ScheduledTask.TaskStatus.WAITING) { LOGGER.debug("Task {} not in executable state: {}", task.getItemId(), task.getStatus()); return false; } - // For persistent tasks, acquire lock before execution - if (task.isPersistent() && !lockManager.acquireLock(task)) { + // Don't start a SCHEDULED task before its due time. The same task can be dispatched + // both by its scheduled retry closure and by the periodic task checker; without this + // guard a duplicate dispatch arriving after an attempt just failed could run the next + // attempt immediately, ignoring the configured retry delay (or run a periodic task + // before its next period). A skipped dispatch is not lost: the task checker picks the + // task up again once its nextScheduledExecution time is actually reached. + if (task.getStatus() == ScheduledTask.TaskStatus.SCHEDULED) { + Date nextExecution = task.getNextScheduledExecution(); + if (nextExecution != null && System.currentTimeMillis() < nextExecution.getTime()) { + LOGGER.debug("Task {} not due yet (next execution at {}), skipping execution", + task.getItemId(), nextExecution); + return false; + } + } + + // Acquire lock for exclusive tasks (persistent distributed or in-memory). + // allowParallelExecution tasks get a non-exclusive lock marker inside acquireLock. + long lockAttemptStart = System.currentTimeMillis(); + boolean lockAcquired = lockManager.acquireLock(task); + LOGGER.debug("LOCK-DIAG [{}] node {} : prepareForExecution() acquireLock() returned {} in {} ms", + task.getItemId(), nodeId, lockAcquired, System.currentTimeMillis() - lockAttemptStart); + if (!lockAcquired) { LOGGER.debug("Could not acquire lock for task: {}", task.getItemId()); return false; } + // Clear waiting state when leaving WAITING for execution + if (task.getStatus() == ScheduledTask.TaskStatus.WAITING) { + task.setWaitingOnTasks(null); + task.setWaitingForTaskType(null); + } + stateManager.updateTaskState(task, ScheduledTask.TaskStatus.RUNNING, null, nodeId); schedulerService.saveTask(task); + LOGGER.debug("LOCK-DIAG [{}] node {} : prepareForExecution() succeeded, task now RUNNING", + task.getItemId(), nodeId); return true; } @@ -326,51 +366,55 @@ public void fail(String error) { private Runnable createTaskWrapper(ScheduledTask task, TaskExecutor executor, TaskExecutor.TaskStatusCallback statusCallback) { return () -> { - // Check shutdown flag first - if scheduler is shutting down, skip task execution - if (schedulerService != null && schedulerService.isShutdownNow()) { - LOGGER.debug("Node {} : Skipping task {} execution as scheduler is shutting down", nodeId, task != null ? task.getItemId() : "unknown"); - return; - } - - if (task == null) { - LOGGER.error("Node {} : Cannot execute null task", nodeId); - return; - } - if (executor == null) { - LOGGER.error("Node {} : Cannot execute null executor for task type : {}", nodeId, task.getTaskType()); - return; - } + // executeTask() has already atomically claimed taskId in executingTasksByType + // before scheduling this wrapper. Every exit path below - including early returns for + // shutdown and prepareForExecution() failing - must release that claim in the outer + // finally, or the task is permanently blocked from ever being dispatched again. + String taskId = task != null ? task.getItemId() : null; + String taskType = task != null ? task.getTaskType() : null; + boolean executingNodeIdSet = false; + try { + // Check shutdown flag first - if scheduler is shutting down, skip task execution + if (schedulerService != null && schedulerService.isShutdownNow()) { + LOGGER.debug("Node {} : Skipping task {} execution as scheduler is shutting down", + nodeId, taskId != null ? taskId : "unknown"); + return; + } - String taskId = task.getItemId(); - String taskType = task.getTaskType(); + if (task == null) { + LOGGER.error("Node {} : Cannot execute null task", nodeId); + return; + } + if (executor == null) { + LOGGER.error("Node {} : Cannot execute null executor for task type : {}", nodeId, taskType); + return; + } - if (taskType == null) { - LOGGER.error("Task type is null for task: {}", taskId); - return; - } + if (taskType == null) { + LOGGER.error("Task type is null for task: {}", taskId); + return; + } - // From here on, executeTask() has already atomically claimed taskId in executingTasksByType - // before scheduling this wrapper. Every exit path below - including the early returns for - // shutdown and prepareForExecution() failing (e.g. lock already held by the caller, as - // happens when TaskRecoveryManager.attemptTaskResumption()/attemptTaskRestart() acquire the - // lock themselves before calling executeTask()) - must release that claim in the outer - // finally below, or the task is permanently blocked from ever being dispatched again. - boolean executingNodeIdSet = false; - try { // Check shutdown again before preparing for execution if (schedulerService != null && schedulerService.isShutdownNow()) { LOGGER.debug("Node {} : Skipping task {} execution as scheduler is shutting down", nodeId, taskId); return; } + // Decide resume vs execute before prepareForExecution() flips CRASHED→RUNNING + boolean shouldResume = task.getStatus() == ScheduledTask.TaskStatus.CRASHED + && executor.canResume(task); + // Prepare task for execution (both persistent and in-memory) if (!prepareForExecution(task)) { return; } - // Final shutdown check before executing + // Final shutdown check before executing. prepareForExecution already set RUNNING + // and acquired the lock — roll that back so peers are not blocked until lock timeout. if (schedulerService != null && schedulerService.isShutdownNow()) { - LOGGER.debug("Node {} : Skipping task {} execution as scheduler is shutting down", nodeId, taskId); + LOGGER.debug("Node {} : Aborting prepared task {} — scheduler is shutting down", nodeId, taskId); + abortPreparedExecution(task); return; } @@ -379,9 +423,13 @@ private Runnable createTaskWrapper(ScheduledTask task, TaskExecutor executor, executingNodeIdSet = true; schedulerService.saveTask(task); + // Heartbeat the lock while we execute so peers only see it expire if this + // node actually dies (stops renewing), not just because execution ran long. + startLockRenewal(task); + long startTime = System.currentTimeMillis(); try { - if (task.getStatus() == ScheduledTask.TaskStatus.CRASHED && executor.canResume(task)) { + if (shouldResume) { executor.resume(task, statusCallback); } else { executor.execute(task, statusCallback); @@ -394,21 +442,39 @@ private Runnable createTaskWrapper(ScheduledTask task, TaskExecutor executor, } } catch (Exception e) { LOGGER.error("Unexpected error while executing task: " + taskId, e); - statusCallback.fail("Unexpected error: " + e.getMessage()); + if (statusCallback != null) { + statusCallback.fail("Unexpected error: " + e.getMessage()); + } } finally { + // Catch-all renewal stop for paths where no terminal callback ran (executor + // returned without complete()/fail()); terminal handlers already stopped it. + stopLockRenewal(taskId); + // Only clear/save executingNodeId if we actually set it above; otherwise we never // touched the task and a redundant save here could race a concurrent legitimate holder. + // Use CAS, not a blind overwrite: when our own terminal transition above was skipped + // because a peer already reclaimed/restarted this task (canCommitTerminalTransition + // returned false), task still holds our stale local view (status RUNNING). A blind + // save here would clobber the peer's newer state and revive an already-resolved race, + // letting the peer's restart re-dispatch and double-execute. CAS fails harmlessly + // instead, since the peer is now the authoritative owner. if (executingNodeIdSet) { task.setExecutingNodeId(null); - schedulerService.saveTask(task); + if (!schedulerService.saveTaskWithRefresh(task)) { + LOGGER.debug("Node {} : Could not clear executingNodeId for task {} — " + + "a peer likely reclaimed it first, which is expected", + nodeId, taskId); + } } // Always release the dispatch claim taken by executeTask(), regardless of which path - // above was taken. + // above was taken (including the outer shutdown early-return). try { - Set executingTasks = executingTasksByType.get(taskType); - if (executingTasks != null && taskId != null) { - executingTasks.remove(taskId); + if (taskType != null && taskId != null) { + Set executingTasks = executingTasksByType.get(taskType); + if (executingTasks != null) { + executingTasks.remove(taskId); + } } } catch (Exception e) { LOGGER.error("Error cleaning up task execution state: " + taskId, e); @@ -418,50 +484,247 @@ private Runnable createTaskWrapper(ScheduledTask task, TaskExecutor executor, } /** - * Handles task completion + * Rolls back a task that was prepared (RUNNING + lock) but must not execute because + * the scheduler is shutting down. Marks CRASHED and clears the lock so recovery on + * the next instance is immediate. */ - private void handleTaskCompletion(ScheduledTask task, long startTime) { - long executionTime = System.currentTimeMillis() - startTime; + private void abortPreparedExecution(ScheduledTask task) { + try { + stateManager.updateTaskState(task, ScheduledTask.TaskStatus.CRASHED, + "Interrupted by scheduler shutdown", nodeId); + task.setExecutingNodeId(null); + task.setLockOwner(null); + task.setLockDate(null); + schedulerService.saveTask(task, true); + } catch (Exception e) { + LOGGER.warn("Failed to abort prepared task {} during shutdown: {}", + task.getItemId(), e.getMessage()); + } + } - // Only transition to completed if still in RUNNING state - if (task.getStatus() == ScheduledTask.TaskStatus.RUNNING) { - stateManager.updateTaskState(task, ScheduledTask.TaskStatus.COMPLETED, null, nodeId); - task.setLastExecutionDate(new Date()); - task.setLastExecutedBy(nodeId); - task.setFailureCount(0); - task.setSuccessCount(task.getSuccessCount() + 1); + /** + * Handle for the periodic lock-renewal (heartbeat) scheduled while a task executes. + * The mutex serializes renewal ticks against {@link #stopLockRenewal(String)} so a + * terminal transition can guarantee no renewal write lands after its fresh OCC read. + */ + private static final class LockRenewalHandle { + private final Object mutex = new Object(); + private volatile boolean cancelled; + private volatile ScheduledFuture future; + } - historyManager.recordSuccess(task, executionTime); - metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_COMPLETED); - metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_EXECUTION_TIME, executionTime); - - // Handle task completion based on type - if (task.isOneShot()) { - task.setEnabled(false); - task.setNextScheduledExecution(null); // Clear next execution time - scheduledTasks.remove(task.getItemId()); - } else if (task.getPeriod() > 0) { - // For periodic tasks, calculate next execution time - stateManager.calculateNextExecutionTime(task); - // Only transition to SCHEDULED if next execution is set (task might be disabled) - if (task.getNextScheduledExecution() != null) { - stateManager.updateTaskState(task, ScheduledTask.TaskStatus.SCHEDULED, null, nodeId); + /** + * Starts periodic lock renewal (heartbeating) for a persistent exclusive task while it + * executes on this node, so its lock only expires when the owner actually stops renewing + * (crash, shutdown) rather than merely because the execution outlived the lock timeout. + * Without renewal, crash recovery on a peer can legitimately steal the lock from a node + * that is still executing and double-run the task. + */ + private void startLockRenewal(ScheduledTask task) { + if (!task.isPersistent() || task.isAllowParallelExecution()) { + return; + } + long interval = Math.max(MIN_LOCK_RENEWAL_INTERVAL_MS, lockManager.getLockTimeout() / 3); + LockRenewalHandle handle = new LockRenewalHandle(); + activeLockRenewals.put(task.getItemId(), handle); + try { + handle.future = scheduler.scheduleAtFixedRate(() -> { + synchronized (handle.mutex) { + if (handle.cancelled) { + return; + } + try { + lockManager.renewLock(task); + } catch (Exception e) { + LOGGER.debug("Lock renewal failed for task {}: {}", task.getItemId(), e.getMessage()); + } } + }, interval, interval, TimeUnit.MILLISECONDS); + } catch (RejectedExecutionException e) { + // Scheduler is shutting down; without renewal the lock simply ages out, which is + // exactly what peers need in order to recover the work. + activeLockRenewals.remove(task.getItemId(), handle); + } + } + + /** + * Stops lock renewal for a task, waiting out any in-flight renewal tick. Must be called + * before a terminal transition's fresh OCC read: a renewal write landing after that read + * would advance the store version and make the terminal compare-and-set fail, stranding + * a finished task as RUNNING in the store. + */ + private void stopLockRenewal(String taskId) { + if (taskId == null) { + return; + } + LockRenewalHandle handle = activeLockRenewals.remove(taskId); + if (handle == null) { + return; + } + synchronized (handle.mutex) { + handle.cancelled = true; + } + if (handle.future != null) { + handle.future.cancel(false); + } + } + + /** + * Reclaims a task that crash recovery prematurely marked as CRASHED while it was in fact + * still executing on this node (e.g. the executing thread stalled long enough for the + * task lock to expire). The executor invoking its status callback proves the execution is + * alive, so the CRASHED marker is wrong: without reclaiming, the callback would be + * silently ignored and a one-shot task would be stranded in CRASHED state forever + * (recovery refuses to restart one-shot tasks that already executed, and the task checker + * only selects SCHEDULED/WAITING tasks). + * + * The executingNodeId guard ensures we only reclaim executions this node actually owns: + * it is set by the task wrapper right after successful preparation and cleared when the + * wrapper finishes, and recovery preserves it when marking a task CRASHED. + */ + private void reclaimIfPrematurelyCrashed(ScheduledTask task) { + if (task.getStatus() == ScheduledTask.TaskStatus.CRASHED + && nodeId != null && nodeId.equals(task.getExecutingNodeId())) { + LOGGER.info("Task {} was marked CRASHED by recovery while still executing on node {}; reclaiming it", + task.getItemId(), nodeId); + stateManager.updateTaskState(task, ScheduledTask.TaskStatus.RUNNING, null, nodeId); + } + } + + /** + * Returns whether this node may commit a terminal completion/error transition. + * Reloads the store view so we do not overwrite CANCELLED or a peer that stole the + * lock / execution after our local RUNNING view became stale. + */ + private boolean canCommitTerminalTransition(ScheduledTask task) { + reclaimIfPrematurelyCrashed(task); + if (task.getStatus() != ScheduledTask.TaskStatus.RUNNING) { + return false; + } + if (!task.isPersistent() || schedulerService == null) { + return true; + } + + ScheduledTask latest = schedulerService.getTask(task.getItemId(), true); + if (latest == null) { + // Store unavailable / deleted: fall back to the in-memory RUNNING decision. + return true; + } + + if (latest.getStatus() == ScheduledTask.TaskStatus.CANCELLED) { + LOGGER.info("Skipping terminal transition for task {}: store status is CANCELLED", + task.getItemId()); + task.setStatus(ScheduledTask.TaskStatus.CANCELLED); + return false; + } + + String latestExecutor = latest.getExecutingNodeId(); + if (latestExecutor != null && !nodeId.equals(latestExecutor)) { + LOGGER.info("Skipping terminal transition for task {}: peer {} owns execution", + task.getItemId(), latestExecutor); + return false; + } + + String latestLockOwner = latest.getLockOwner(); + if (latestLockOwner != null && !nodeId.equals(latestLockOwner) + && !lockManager.isLockExpired(latest)) { + LOGGER.info("Skipping terminal transition for task {}: peer {} holds a non-expired lock", + task.getItemId(), latestLockOwner); + return false; + } + + // Allow RUNNING. Allow CRASHED only when this node still owns executingNodeId + // (premature-crash reclaim). Any other store status means cancel/recovery/peer + // already moved the document — a late complete/fail must not clobber it. + if (latest.getStatus() == ScheduledTask.TaskStatus.CRASHED) { + if (latestExecutor == null || !nodeId.equals(latestExecutor)) { + LOGGER.info("Skipping terminal transition for task {}: CRASHED without our executingNodeId", + task.getItemId()); + return false; } + } else if (latest.getStatus() != ScheduledTask.TaskStatus.RUNNING) { + LOGGER.info("Skipping terminal transition for task {}: store status is {}", + task.getItemId(), latest.getStatus()); + return false; + } + + // Carry OCC tokens from the fresh load so persistTerminalState can CAS. + TaskLockManager.copyOccMetadata(latest, task); + return true; + } - // Release lock for persistent tasks - if (task.isPersistent()) { - lockManager.releaseLock(task); + /** + * Persists a terminal task state. Persistent tasks use compare-and-set so a late + * complete/fail cannot clobber CANCELLED or a peer's RUNNING document. Lock fields are + * cleared on the same write to avoid a separate alwaysOverwrite unlock race. + */ + private boolean persistTerminalState(ScheduledTask task) { + task.setLockOwner(null); + task.setLockDate(null); + if (!task.isPersistent()) { + boolean saved = schedulerService.saveTask(task); + if (!saved) { + LOGGER.warn("Failed to persist terminal state for non-persistent task {} (status={})", + task.getItemId(), task.getStatus()); } + return saved; + } + boolean saved = schedulerService.saveTaskWithRefresh(task); + if (!saved) { + LOGGER.warn("Terminal persist lost OCC race for task {} (status={})", + task.getItemId(), task.getStatus()); + } + return saved; + } - // Clean up executing tasks set - Set executingTasks = executingTasksByType.get(task.getTaskType()); - if (executingTasks != null) { - executingTasks.remove(task.getItemId()); + /** + * Handles task completion + */ + private void handleTaskCompletion(ScheduledTask task, long startTime) { + long executionTime = System.currentTimeMillis() - startTime; + + // Stop heartbeating before the terminal fresh read below — a renewal landing after + // that read would advance the store version and fail the terminal compare-and-set. + stopLockRenewal(task.getItemId()); + + if (!canCommitTerminalTransition(task)) { + return; + } + + stateManager.updateTaskState(task, ScheduledTask.TaskStatus.COMPLETED, null, nodeId); + task.setLastExecutionDate(new Date()); + task.setLastExecutedBy(nodeId); + task.setFailureCount(0); + task.setSuccessCount(task.getSuccessCount() + 1); + + historyManager.recordSuccess(task, executionTime); + + // Handle task completion based on type + if (task.isOneShot()) { + task.setEnabled(false); + task.setNextScheduledExecution(null); // Clear next execution time + scheduledTasks.remove(task.getItemId()); + } else if (task.getPeriod() > 0) { + // For periodic tasks, calculate next execution time + stateManager.calculateNextExecutionTime(task); + // Only transition to SCHEDULED if next execution is set (task might be disabled) + if (task.getNextScheduledExecution() != null) { + stateManager.updateTaskState(task, ScheduledTask.TaskStatus.SCHEDULED, null, nodeId); } + } + + // Clean up executing tasks set + Set executingTasks = executingTasksByType.get(task.getTaskType()); + if (executingTasks != null) { + executingTasks.remove(task.getItemId()); + } - schedulerService.saveTask(task); + if (!persistTerminalState(task)) { + return; } + + metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_COMPLETED); + metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_EXECUTION_TIME, executionTime); } /** @@ -470,63 +733,72 @@ private void handleTaskCompletion(ScheduledTask task, long startTime) { private void handleTaskError(ScheduledTask task, String error, long startTime) { long executionTime = System.currentTimeMillis() - startTime; - // Only transition to failed if still in RUNNING state - if (task.getStatus() == ScheduledTask.TaskStatus.RUNNING) { - stateManager.updateTaskState(task, ScheduledTask.TaskStatus.FAILED, error, nodeId); - task.setFailureCount(task.getFailureCount() + 1); + // Stop heartbeating before the terminal fresh read below — a renewal landing after + // that read would advance the store version and fail the terminal compare-and-set. + stopLockRenewal(task.getItemId()); - historyManager.recordFailure(task, error); - metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_FAILED); - metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_EXECUTION_TIME, executionTime); + if (!canCommitTerminalTransition(task)) { + return; + } - // Check if we should retry - if (task.getFailureCount() <= task.getMaxRetries()) { - // Calculate next retry time - stateManager.calculateNextExecutionTime(task, true); - stateManager.updateTaskState(task, ScheduledTask.TaskStatus.SCHEDULED, null, nodeId); + stateManager.updateTaskState(task, ScheduledTask.TaskStatus.FAILED, error, nodeId); + task.setFailureCount(task.getFailureCount() + 1); - // Only schedule retry if scheduler is not shutting down - if (!scheduler.isShutdown() && !scheduler.isTerminated()) { - // Schedule retry - try { - Runnable retryTask = () -> { - TaskExecutor executor = executorRegistry.getExecutor(task.getTaskType()); - if (executor != null) { - executeTask(task, executor); - } - }; - // Use the configured retry delay directly rather than re-deriving it from - // nextScheduledExecution: that target was computed before the state/history/metrics - // bookkeeping above ran, so subtracting "now" here would silently erode the delay - // by however long that bookkeeping took (worse under slower/contended runners). - long retryDelay = task.getRetryDelay(); - scheduler.schedule(retryTask, retryDelay, TimeUnit.MILLISECONDS); - LOGGER.debug("Scheduled retry #{} for task {} in {} ms", - task.getFailureCount(), task.getItemId(), retryDelay); - } catch (RejectedExecutionException e) { - LOGGER.debug("Retry scheduling rejected for task {} as scheduler is shutting down", task.getItemId()); - } - } else { - LOGGER.debug("Not scheduling retry for task {} as scheduler is shutting down", task.getItemId()); - } - } else if (!task.isOneShot()) { - LOGGER.debug("Periodic task {} failed all retries but scheduling for next period in {} ms", task.getItemId(), task.getPeriod()); - schedulerService.saveTask(task); // persist failure state before going back to scheduled state - task.setLastExecutionDate(new Date()); - task.setLastExecutedBy(nodeId); - stateManager.calculateNextExecutionTime(task, false); - if (task.getNextScheduledExecution() != null) { - stateManager.updateTaskState(task, ScheduledTask.TaskStatus.SCHEDULED, null, nodeId); - } - } + historyManager.recordFailure(task, error); - // Release lock for persistent tasks - if (task.isPersistent()) { - lockManager.releaseLock(task); + boolean scheduleRetry = false; + // Check if we should retry + if (task.getFailureCount() <= task.getMaxRetries()) { + // Calculate next retry time + stateManager.calculateNextExecutionTime(task, true); + stateManager.updateTaskState(task, ScheduledTask.TaskStatus.SCHEDULED, null, nodeId); + scheduleRetry = true; + } else if (!task.isOneShot()) { + LOGGER.debug("Periodic task {} failed all retries but scheduling for next period in {} ms", task.getItemId(), task.getPeriod()); + task.setLastExecutionDate(new Date()); + task.setLastExecutedBy(nodeId); + // Reset failure count so the next period gets a fresh retry budget + // (matches ScheduledTask API docs and prevents immediate exhaustion on period 2). + task.setFailureCount(0); + stateManager.calculateNextExecutionTime(task, false); + if (task.getNextScheduledExecution() != null) { + stateManager.updateTaskState(task, ScheduledTask.TaskStatus.SCHEDULED, null, nodeId); } + } - schedulerService.saveTask(task); - scheduledTasks.remove(task.getItemId()); + scheduledTasks.remove(task.getItemId()); + + if (!persistTerminalState(task)) { + return; + } + + metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_FAILED); + metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_EXECUTION_TIME, executionTime); + + if (scheduleRetry) { + // Only schedule retry if scheduler is not shutting down + if (!scheduler.isShutdown() && !scheduler.isTerminated()) { + try { + Runnable retryTask = () -> { + TaskExecutor executor = executorRegistry.getExecutor(task.getTaskType()); + if (executor != null) { + executeTask(task, executor); + } + }; + // Use the configured retry delay directly rather than re-deriving it from + // nextScheduledExecution: that target was computed before the state/history/metrics + // bookkeeping above ran, so subtracting "now" here would silently erode the delay + // by however long that bookkeeping took (worse under slower/contended runners). + long retryDelay = task.getRetryDelay(); + scheduler.schedule(retryTask, retryDelay, TimeUnit.MILLISECONDS); + LOGGER.debug("Scheduled retry #{} for task {} in {} ms", + task.getFailureCount(), task.getItemId(), retryDelay); + } catch (RejectedExecutionException e) { + LOGGER.debug("Retry scheduling rejected for task {} as scheduler is shutting down", task.getItemId()); + } + } else { + LOGGER.debug("Not scheduling retry for task {} as scheduler is shutting down", task.getItemId()); + } } } @@ -555,6 +827,8 @@ private void updateTaskMetrics(ScheduledTask task, long startTime) { * @param taskId the task ID to cancel */ public void cancelTask(String taskId) { + stopLockRenewal(taskId); + ScheduledFuture future = scheduledTasks.remove(taskId); if (future != null) { future.cancel(true); @@ -572,6 +846,11 @@ public void cancelTask(String taskId) { public void shutdown() { stopTaskChecker(); + // Stop all lock heartbeats so held locks age out and peers can recover the work + for (String taskId : new ArrayList<>(activeLockRenewals.keySet())) { + stopLockRenewal(taskId); + } + // Cancel all scheduled and running tasks for (ScheduledFuture future : scheduledTasks.values()) { future.cancel(true); diff --git a/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskLockManager.java b/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskLockManager.java index 0605ddb0f5..978d97fa64 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskLockManager.java +++ b/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskLockManager.java @@ -18,10 +18,12 @@ import org.apache.unomi.api.tasks.ScheduledTask; import org.apache.unomi.api.conditions.ConditionType; +import org.apache.unomi.persistence.spi.PersistenceService; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.util.*; +import java.util.concurrent.ConcurrentHashMap; /** * Manages task locks to coordinate execution in a cluster environment. @@ -43,13 +45,23 @@ * if ClusterService is unavailable. *

  • Time Windows: Primary nodes get an exclusive time window to acquire locks, * after which backup nodes attempt in sequence.
    • - *
  • Optimistic Concurrency Control: Uses Elasticsearch's sequence numbers and - * primary terms to ensure only one update succeeds when multiple nodes attempt - * simultaneous updates.
    • - *
  • Fencing Tokens: Monotonically increasing version numbers prevent split-brain - * scenarios where multiple nodes believe they own a lock.
    • - *
  • Lock Verification: Double-checking after acquiring a lock ensures it's - * still valid after changes have propagated through the cluster.
    • + *
  • Optimistic Concurrency Control: Uses the persistence backend's native sequence + * numbers and primary terms ({@link PersistenceService#SYSTEM_METADATA_SEQ_NO}/ + * {@link PersistenceService#SYSTEM_METADATA_PRIMARY_TERM}) as the compare-and-set + * precondition, via {@code PersistenceService#save(Item, Boolean, Boolean)}. This is backend-agnostic: + * any persistence implementation that honors that CAS contract (both the Elasticsearch + * and OpenSearch backends do) works here without scheduler-specific changes.
    • + *
  • Fencing Tokens: No separate application-level version counter is maintained. + * The backend's own seq_no/primary_term pair already changes atomically on every + * successful write and serves directly as the fencing token — a successful CAS write + * is itself authoritative proof of exclusive acquisition, so no post-write re-read is + * performed. (An earlier version of this class re-verified acquisition with a delayed + * re-read compared against a custom "lockVersion" field; that added a race window of + * its own — a concurrent renewal/recovery write landing between the CAS and the re-read + * could flip the comparison and produce a false "lost the lock" verdict for an + * acquisition that had, in fact, already succeeded — without adding any real safety + * over trusting the CAS result directly, which {@link #renewLock} and {@link #releaseLock} + * always have.)
    • *
  • Explicit Refreshes: Forces immediate index refreshes to make lock * information visible more quickly to other nodes.
    • * @@ -63,10 +75,8 @@ */ public class TaskLockManager { private static final Logger LOGGER = LoggerFactory.getLogger(TaskLockManager.class); - private static final String SEQ_NO = "seq_no"; - private static final String PRIMARY_TERM = "primary_term"; - private static final String LOCK_VERSION = "lockVersion"; - private static final long VERIFICATION_DELAY_MS = 100; + private static final String SEQ_NO = PersistenceService.SYSTEM_METADATA_SEQ_NO; + private static final String PRIMARY_TERM = PersistenceService.SYSTEM_METADATA_PRIMARY_TERM; private static final long PRIMARY_NODE_WINDOW_MS = 3000; private static final long BACKUP_NODE_WINDOW_MS = 500; @@ -74,6 +84,8 @@ public class TaskLockManager { private long lockTimeout; private TaskMetricsManager metricsManager; private SchedulerServiceImpl schedulerService; + /** Per-task guards for in-memory exclusive acquire (shared across manager instances). */ + private static final ConcurrentHashMap IN_MEMORY_LOCK_GUARDS = new ConcurrentHashMap<>(); /** * Creates the manager for Blueprint dependency injection. @@ -100,6 +112,15 @@ public void setLockTimeout(long lockTimeout) { this.lockTimeout = lockTimeout; } + /** + * Returns the lock expiry timeout in milliseconds. + * + * @return the lock timeout + */ + public long getLockTimeout() { + return lockTimeout; + } + /** * Sets the task metrics manager. * @@ -159,19 +180,28 @@ public boolean acquireLock(ScheduledTask task) { * complex distributed locking. */ private boolean acquireInMemoryLock(ScheduledTask task) { - if (task.getLockOwner() != null && !nodeId.equals(task.getLockOwner())) { - if (!isLockExpired(task)) { + Object guard = IN_MEMORY_LOCK_GUARDS.computeIfAbsent(task.getItemId(), id -> new Object()); + synchronized (guard) { + // Re-load the node-local task so concurrent acquires see each other's lock writes. + ScheduledTask latest = schedulerService.getTask(task.getItemId()); + if (latest == null) { + latest = task; + } + if (latest.getLockOwner() != null && !nodeId.equals(latest.getLockOwner()) + && !isLockExpired(latest)) { return false; } - } - task.setLockOwner(nodeId); - task.setLockDate(new Date()); - metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_LOCK_ACQUIRED); + latest.setLockOwner(nodeId); + latest.setLockDate(new Date()); + task.setLockOwner(nodeId); + task.setLockDate(latest.getLockDate()); + metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_LOCK_ACQUIRED); - // For non-persistent tasks, we just update the in-memory map - schedulerService.saveTask(task); - return true; + // For non-persistent tasks, we just update the in-memory map + schedulerService.saveTask(latest); + return true; + } } /** @@ -180,8 +210,11 @@ private boolean acquireInMemoryLock(ScheduledTask task) { * acquire the lock at the same time. */ private boolean acquireDistributedLock(ScheduledTask task) { + long diagStart = System.currentTimeMillis(); // Step 1: Check if this node should handle this task based on affinity if (!shouldHandleTask(task)) { + LOGGER.debug("LOCK-DIAG [{}] node {} : shouldHandleTask()=false, not attempting acquisition", + task.getItemId(), nodeId); return false; } @@ -194,6 +227,10 @@ private boolean acquireDistributedLock(ScheduledTask task) { LOGGER.warn("Task {} not found when attempting to lock", task.getItemId()); return false; } + LOGGER.debug("LOCK-DIAG [{}] node {} : pre-CAS read - lockOwner={}, lockDate={}, seq_no={}, " + + "primary_term={}, status={}", + task.getItemId(), nodeId, latestTask.getLockOwner(), latestTask.getLockDate(), + latestTask.getSystemMetadata(SEQ_NO), latestTask.getSystemMetadata(PRIMARY_TERM), latestTask.getStatus()); // Step 4: Check if already locked by another node if (latestTask.getLockOwner() != null && @@ -203,7 +240,10 @@ private boolean acquireDistributedLock(ScheduledTask task) { return false; } - // Step 5: Use optimistic concurrency control with sequence numbers + // Step 5: Use optimistic concurrency control with sequence numbers as the CAS + // precondition. The backend rejects this write outright unless seq_no/primary_term + // still match what we just read, so a successful write below is itself authoritative + // proof of exclusive acquisition - no separate re-read-and-compare step is needed. task.setSystemMetadata(SEQ_NO, latestTask.getSystemMetadata(SEQ_NO)); task.setSystemMetadata(PRIMARY_TERM, latestTask.getSystemMetadata(PRIMARY_TERM)); @@ -211,63 +251,30 @@ private boolean acquireDistributedLock(ScheduledTask task) { task.setLockOwner(nodeId); task.setLockDate(new Date()); - // Step 7: Add a monotonically increasing fencing token - Long lockVersion = (Long) latestTask.getSystemMetadata(LOCK_VERSION); - long newLockVersion = (lockVersion == null) ? 1L : lockVersion + 1L; - task.setSystemMetadata(LOCK_VERSION, newLockVersion); + LOGGER.debug("LOCK-DIAG [{}] node {} : attempting CAS write - if_seq_no={}, if_primary_term={}, " + + "writing lockOwner={}", + task.getItemId(), nodeId, task.getSystemMetadata(SEQ_NO), task.getSystemMetadata(PRIMARY_TERM), nodeId); - // Step 8: Save with WAIT_UNTIL refresh policy + // Step 7: Save with WAIT_UNTIL refresh policy. The backend's own CAS result is + // authoritative: true means our precondition matched and the write applied atomically, + // so we now exclusively hold the lock. task's seq_no/primary_term are updated in place + // to the post-write values, which double as an opaque fencing token for this generation + // of the lock (see PersistenceService#SYSTEM_METADATA_SEQ_NO). boolean acquired = schedulerService.saveTaskWithRefresh(task); + LOGGER.debug("LOCK-DIAG [{}] node {} : CAS write result acquired={} in {} ms, post-write seq_no={}, " + + "post-write primary_term={}", + task.getItemId(), nodeId, acquired, System.currentTimeMillis() - diagStart, + task.getSystemMetadata(SEQ_NO), task.getSystemMetadata(PRIMARY_TERM)); + if (!acquired) { LOGGER.debug("Failed to acquire lock for task {} due to version conflict", task.getItemId()); metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_LOCK_CONFLICTS); return false; } - // Step 9: Double-check our lock after a delay to ensure it's still valid - try { - // Wait for a short time to allow any concurrent operations to complete - Thread.sleep(VERIFICATION_DELAY_MS); - - // Force refresh again to ensure we see the latest state - schedulerService.refreshTasks(); - - // Get the task again to verify our lock - ScheduledTask verifiedTask = schedulerService.getTask(task.getItemId()); - if (verifiedTask == null) { - LOGGER.warn("Task {} disappeared after locking", task.getItemId()); - metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_LOCK_CONFLICTS); - return false; - } - - // Verify we're still the lock owner - if (!nodeId.equals(verifiedTask.getLockOwner())) { - LOGGER.warn("Lost lock ownership for task {} to {}", - task.getItemId(), verifiedTask.getLockOwner()); - metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_LOCK_CONFLICTS); - return false; - } - - // Verify our fencing token is still the highest - Long currentToken = (Long) verifiedTask.getSystemMetadata(LOCK_VERSION); - if (currentToken == null || currentToken != newLockVersion) { - LOGGER.warn("Lock version mismatch for task {}: expected {} but found {}", - task.getItemId(), newLockVersion, currentToken); - metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_LOCK_CONFLICTS); - return false; - } - - // Lock successfully verified - LOGGER.debug("Successfully acquired and verified lock for task {}", task.getItemId()); - metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_LOCK_ACQUIRED); - return true; - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - // Attempt to release the lock since we're being interrupted - releaseLock(task); - return false; - } + metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_LOCK_ACQUIRED); + return true; } /** @@ -275,6 +282,14 @@ private boolean acquireDistributedLock(ScheduledTask task) { * This reduces contention by giving priority to a specific node for each task. */ private boolean shouldHandleTask(ScheduledTask task) { + // Crash recovery has already chosen this node to resume/restart. Affinity windows + // must not block that path — a dead primary often remains in getActiveNodes() long + // enough that the backup would otherwise wait out PRIMARY_NODE_WINDOW_MS and miss + // the immediate recoverCrashedTasks() dispatch (checkpoint resume tests / failover). + if (task.getStatus() == ScheduledTask.TaskStatus.CRASHED) { + return true; + } + // Check if this is a scheduled task Date scheduledTime = task.getNextScheduledExecution(); if (scheduledTime == null) { @@ -316,7 +331,17 @@ private boolean shouldHandleTask(ScheduledTask task) { // Calculate backup order (relative position after primary) int backupOrder = (ourIndex - primaryIndex + activeNodes.size()) % activeNodes.size(); - // Each backup node gets a time window based on their order + // Each backup node gets a staggered time window based on their order to reduce + // contention during normal operation. After every backup has had a window, open the + // field to any active node — otherwise a dead primary that remains in getActiveNodes() + // would permanently strand the task (backups would return false forever after their + // short 500ms slots closed). + int backupCount = activeNodes.size() - 1; + long openFieldStart = PRIMARY_NODE_WINDOW_MS + (backupCount * BACKUP_NODE_WINDOW_MS); + if (delayMs >= openFieldStart) { + return true; + } + long ourWindowStart = PRIMARY_NODE_WINDOW_MS + ((backupOrder - 1) * BACKUP_NODE_WINDOW_MS); long ourWindowEnd = ourWindowStart + BACKUP_NODE_WINDOW_MS; @@ -333,30 +358,172 @@ public boolean releaseLock(ScheduledTask task) { if (task == null) { return false; } + LOGGER.debug("LOCK-DIAG [{}] node {} : releaseLock() called - caller's view lockOwner={}, lockDate={}", + task.getItemId(), nodeId, task.getLockOwner(), task.getLockDate()); - // Only allow the lock owner to release the lock - if (task.getLockOwner() != null && !nodeId.equals(task.getLockOwner())) { + // Fast reject from the caller's view: only the lock owner may release a still-valid lock. + // Expired locks may be cleared by any recovering node so a dead owner's lock does not + // block failover — but that decision is re-validated against the fresh store view below. + if (task.getLockOwner() != null && !nodeId.equals(task.getLockOwner()) && !isLockExpired(task)) { LOGGER.warn("Node {} attempted to release a lock owned by {}", nodeId, task.getLockOwner()); return false; } try { + // Clear lock on a freshly loaded copy when possible. Callers such as + // handleTaskError() may have already mutated the in-memory task to + // SCHEDULED/FAILED for retry; persisting that object during shutdown + // would hide the RUNNING/CRASHED state peers need for recovery. + ScheduledTask toSave = task; + ScheduledTask latest = schedulerService.getTask(task.getItemId(), true); + if (latest != null) { + toSave = latest; + } + + // Re-validate against the store: a peer may have stolen the lock after our local + // view expired. Never wipe a non-expired foreign lock (that would unlock a live + // peer mid-execution and enable double-dispatch). + String latestOwner = toSave.getLockOwner(); + LOGGER.debug("LOCK-DIAG [{}] node {} : releaseLock() fresh store read - lockOwner={}, " + + "lockDate={}, seq_no={}, primary_term={}", + task.getItemId(), nodeId, latestOwner, toSave.getLockDate(), + toSave.getSystemMetadata(SEQ_NO), toSave.getSystemMetadata(PRIMARY_TERM)); + if (latestOwner == null) { + task.setLockOwner(null); + task.setLockDate(null); + LOGGER.debug("LOCK-DIAG [{}] node {} : releaseLock() no-op, store already unlocked", + task.getItemId(), nodeId); + return true; + } + boolean weOwnLatest = nodeId.equals(latestOwner); + boolean latestExpired = isLockExpired(toSave); + if (!weOwnLatest && !latestExpired) { + LOGGER.warn( + "Node {} not releasing task {}: store lock is owned by {} and has not expired", + nodeId, task.getItemId(), latestOwner); + return false; + } + + toSave.setLockOwner(null); + toSave.setLockDate(null); task.setLockOwner(null); task.setLockDate(null); - if (!schedulerService.saveTask(task)) { - LOGGER.error("Failed to release lock for task {}", task.getItemId()); + // Compare-and-set on the freshly loaded seq_no/primary_term, not a blind overwrite: + // a peer may win a legitimate CAS-based lock acquisition in the window between our + // read above and this write. A blind overwrite would silently clobber that peer's + // new lock; CAS instead fails closed and we report a lost race below. Allow persist + // during shutdown: preDestroy/simulateCrash set shutdownNow before releasing locks, + // and a no-op save would leave a stale lock in deep-copy stores. + if (!schedulerService.saveTaskWithRefresh(toSave, true)) { + LOGGER.warn("Failed to release lock for task {}: lost compare-and-set race, " + + "a peer likely re-acquired the lock", task.getItemId()); return false; } + LOGGER.debug("LOCK-DIAG [{}] node {} : releaseLock() CAS write succeeded, lock cleared", + task.getItemId(), nodeId); metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_LOCK_RELEASED); return true; } catch (Exception e) { - LOGGER.error("Error releasing lock for task {}: {}", task.getItemId(), e.getMessage()); + LOGGER.error("Error releasing lock for task {}", task.getItemId(), e); + return false; + } + } + + /** + * Renews (heartbeats) a held distributed lock by refreshing its {@code lockDate}, so that + * expiry only ever means "the owner stopped renewing" (crashed or unreachable), never + * merely "the execution outlived the timeout". A live owner that keeps renewing never + * looks expired to peers, which closes the window where crash recovery could steal the + * lock from a node that is still genuinely executing and double-run the task. + * + * Only persistent exclusive tasks need renewal: parallel-execution and non-persistent + * tasks return true without touching the store. Renewal deliberately uses the + * shutdown-sensitive load/save paths — once shutdown begins the lock must be allowed to + * age out so peers can recover the work. + * + * @param task the executing task whose lock should be renewed (caller must own the lock) + * @return true if the lock was renewed (or renewal is not applicable), false if ownership + * was lost or the compare-and-set write failed (benign: a peer took over) + */ + public boolean renewLock(ScheduledTask task) { + if (task == null) { + return false; + } + if (!task.isPersistent() || task.isAllowParallelExecution()) { + return true; + } + if (!nodeId.equals(task.getLockOwner())) { + LOGGER.debug("LOCK-DIAG [{}] node {} : renewLock() skipped - caller's view lockOwner={} != nodeId", + task.getItemId(), nodeId, task.getLockOwner()); + return false; + } + try { + ScheduledTask latest = schedulerService.getTask(task.getItemId()); + LOGGER.debug("LOCK-DIAG [{}] node {} : renewLock() fresh store read - lockOwner={}, lockDate={}, " + + "seq_no={}, primary_term={}", + task.getItemId(), nodeId, latest != null ? latest.getLockOwner() : "", + latest != null ? latest.getLockDate() : null, + latest != null ? latest.getSystemMetadata(SEQ_NO) : null, + latest != null ? latest.getSystemMetadata(PRIMARY_TERM) : null); + if (latest == null || !nodeId.equals(latest.getLockOwner())) { + LOGGER.debug("Not renewing lock for task {}: store owner is {}", + task.getItemId(), latest != null ? latest.getLockOwner() : null); + return false; + } + + latest.setLockDate(new Date()); + + // Compare-and-set on the fresh store view: if a peer stole the lock between the + // read above and this write, renewal fails closed instead of resurrecting our lock. + if (!schedulerService.saveTaskWithRefresh(latest)) { + LOGGER.debug("Lock renewal for task {} lost a compare-and-set race", task.getItemId()); + return false; + } + + // Sync the renewed date and post-save OCC tokens back onto the caller's task so + // the executing thread's later compare-and-set writes are checked against the + // store's current version, not the pre-renewal one. + task.setLockDate(latest.getLockDate()); + copyOccMetadata(latest, task); + LOGGER.debug("LOCK-DIAG [{}] node {} : renewLock() succeeded, new lockDate={}", + task.getItemId(), nodeId, latest.getLockDate()); + return true; + } catch (Exception e) { + LOGGER.warn("Error renewing lock for task {}", task.getItemId(), e); return false; } } + /** + * Copies OCC (seq_no/primary_term) fencing metadata from a freshly loaded task onto the + * task about to be persisted, so a subsequent compare-and-set save is checked against the + * store's current version rather than a stale in-memory one. {@code from} and {@code to} + * may be the same instance (normalizes both ES/OS key variants onto it). + * + * @param from the task holding the current OCC metadata (typically a fresh store read) + * @param to the task that will be persisted next + */ + public static void copyOccMetadata(ScheduledTask from, ScheduledTask to) { + Object seq = from.getSystemMetadata(SEQ_NO); + if (seq == null) { + seq = from.getSystemMetadata("_seq_no"); + } + Object term = from.getSystemMetadata(PRIMARY_TERM); + if (term == null) { + term = from.getSystemMetadata("_primary_term"); + } + if (seq != null) { + to.setSystemMetadata(SEQ_NO, seq); + to.setSystemMetadata("_seq_no", seq); + } + if (term != null) { + to.setSystemMetadata(PRIMARY_TERM, term); + to.setSystemMetadata("_primary_term", term); + } + } + /** * Checks if a task's lock has expired based on timeout. * @@ -365,10 +532,17 @@ public boolean releaseLock(ScheduledTask task) { */ public boolean isLockExpired(ScheduledTask task) { if (task == null || task.getLockDate() == null) { + LOGGER.debug("LOCK-DIAG isLockExpired() : task={}, lockDate=null -> expired=true", + task != null ? task.getItemId() : ""); return true; } - long lockAge = System.currentTimeMillis() - task.getLockDate().getTime(); - return lockAge > lockTimeout; + long now = System.currentTimeMillis(); + long lockAge = now - task.getLockDate().getTime(); + boolean expired = lockAge > lockTimeout; + LOGGER.debug("LOCK-DIAG isLockExpired() : task={}, lockDate={} ({}), now={}, lockAge={}ms, " + + "lockTimeout={}ms -> expired={}", + task.getItemId(), task.getLockDate(), task.getLockDate().getTime(), now, lockAge, lockTimeout, expired); + return expired; } } diff --git a/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskRecoveryManager.java b/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskRecoveryManager.java index 7d6da402e3..43b29e3685 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskRecoveryManager.java +++ b/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskRecoveryManager.java @@ -42,6 +42,7 @@ public class TaskRecoveryManager { private TaskExecutorRegistry executorRegistry; private SchedulerServiceImpl schedulerService; private volatile boolean shutdownNow = false; + private boolean executorNode = true; /** * Creates the manager for Blueprint dependency injection. @@ -113,6 +114,17 @@ public void setSchedulerService(SchedulerServiceImpl schedulerService) { this.schedulerService = schedulerService; } + /** + * Sets whether this node is an executor node. Non-executors may still mark + * crashed tasks and clear dead locks, but must not dispatch resume/restart + * unless the task is {@code runOnAllNodes}. + * + * @param executorNode true when this node runs cluster tasks + */ + public void setExecutorNode(boolean executorNode) { + this.executorNode = executorNode; + } + /** * Marks the manager as shutting down so recovery work is skipped. */ @@ -133,11 +145,16 @@ public void recoverCrashedTasks() { return; } + long diagStart = System.currentTimeMillis(); + LOGGER.debug("LOCK-DIAG node {} : recoverCrashedTasks() tick starting", nodeId); try { recoverRunningTasks(); recoverLockedTasks(); } catch (Exception e) { LOGGER.error("Node {} Error recovering crashed tasks", nodeId, e); + } finally { + LOGGER.debug("LOCK-DIAG node {} : recoverCrashedTasks() tick finished in {} ms", + nodeId, System.currentTimeMillis() - diagStart); } } @@ -148,6 +165,8 @@ private void recoverRunningTasks() { if (shutdownNow) return; List runningTasks = schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING); + LOGGER.debug("LOCK-DIAG node {} : recoverRunningTasks() search found {} RUNNING task(s): {}", + nodeId, runningTasks.size(), taskSummary(runningTasks)); for (ScheduledTask task : runningTasks) { if (shutdownNow) return; @@ -159,6 +178,27 @@ private void recoverRunningTasks() { } } + /** + * Diagnostic helper: summarizes a list of tasks (id/status/lockOwner/lockDate) for a single + * log line, so recovery-pass discoveries can be correlated against TaskLockManager's + * LOCK-DIAG output by task id and timestamp. + */ + private static String taskSummary(List tasks) { + if (tasks == null || tasks.isEmpty()) { + return "[]"; + } + StringBuilder sb = new StringBuilder("["); + for (ScheduledTask t : tasks) { + sb.append("{id=").append(t.getItemId()) + .append(", status=").append(t.getStatus()) + .append(", lockOwner=").append(t.getLockOwner()) + .append(", lockDate=").append(t.getLockDate()) + .append("} "); + } + sb.append("]"); + return sb.toString(); + } + /** * Recovers one crashed task by marking it crashed, recording history, and resuming or restarting it. * @@ -180,28 +220,69 @@ private void recoverCrashedTask(ScheduledTask task) { return; } - // First mark as crashed and release lock - String previousOwner = task.getLockOwner(); - if (task.getStatus() != ScheduledTask.TaskStatus.CRASHED) { - stateManager.updateTaskState(task, ScheduledTask.TaskStatus.CRASHED, + // Reload + CAS so two survivors do not both alwaysOverwrite CRASHED/history. + ScheduledTask latest = schedulerService.getTask(task.getItemId(), true); + if (latest == null) { + latest = task; + } + if (latest.getStatus() == ScheduledTask.TaskStatus.CANCELLED) { + return; + } + if (latest.getStatus() != ScheduledTask.TaskStatus.CRASHED + && latest.getStatus() != ScheduledTask.TaskStatus.RUNNING) { + LOGGER.debug("Node {} Skipping recovery of task {} : {} — store status is {}", + nodeId, latest.getTaskType(), latest.getItemId(), latest.getStatus()); + return; + } + if (latest.getStatus() == ScheduledTask.TaskStatus.RUNNING && !lockManager.isLockExpired(latest)) { + LOGGER.debug("Node {} Skipping recovery of task {} : {} — lock no longer expired", + nodeId, latest.getTaskType(), latest.getItemId()); + return; + } + + // Carry OCC tokens from the fresh load when present + TaskLockManager.copyOccMetadata(latest, latest); + + // Mark as crashed, then drop the dead owner's lock. prepareForExecution() / + // acquireLock() takes a fresh lock on resume/restart. Leaving an expired lock + // in place races recoverLockedTasks() in the same pass: it can overwrite the + // newly acquired lock (alwaysOverwrite save) and the resume dispatch fails + // verification with "Lost lock ownership". + String previousOwner = latest.getLockOwner(); + if (latest.getStatus() != ScheduledTask.TaskStatus.CRASHED) { + stateManager.updateTaskState(latest, ScheduledTask.TaskStatus.CRASHED, "Node failure detected: " + previousOwner, nodeId); } + latest.setLockOwner(null); + latest.setLockDate(null); // Record the crash in execution history - recordCrash(task, previousOwner); + recordCrash(latest, previousOwner); metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_CRASHED); - if (schedulerService.saveTask(task)) { - // If task has checkpoint data and can be resumed, try to resume it - TaskExecutor executor = executorRegistry.getExecutor(task.getTaskType()); - if (executor != null && executor.canResume(task)) { - attemptTaskResumption(task, executor); - } else { - // If task can't be resumed, try to restart it - if (shouldRestartTask(task)) { - attemptTaskRestart(task, executor); - } - } + boolean saved = latest.isPersistent() + ? schedulerService.saveTaskWithRefresh(latest) + : schedulerService.saveTask(latest); + if (!saved) { + LOGGER.debug("Node {} lost CRASH CAS race for task {} : {}", + nodeId, latest.getTaskType(), latest.getItemId()); + return; + } + + // Keep caller-visible fields in sync for tests that hold the original reference + task.setStatus(latest.getStatus()); + task.setLockOwner(null); + task.setLockDate(null); + task.setStatusDetails(latest.getStatusDetails()); + task.setCurrentStep(latest.getCurrentStep()); + task.setLastError(latest.getLastError()); + task.setCheckpointData(latest.getCheckpointData()); + + TaskExecutor executor = executorRegistry.getExecutor(latest.getTaskType()); + if (executor != null && executor.canResume(latest)) { + attemptTaskResumption(latest, executor); + } else if (shouldRestartTask(latest)) { + attemptTaskRestart(latest, executor); } } @@ -244,12 +325,27 @@ private void recordCrash(ScheduledTask task, String previousOwner) { * @param executor the executor for the task type */ private void attemptTaskResumption(ScheduledTask task, TaskExecutor executor) { + if (executor == null) { + LOGGER.warn("Node {} cannot resume task {} : {} — no executor registered", + nodeId, task.getTaskType(), task.getItemId()); + return; + } + if (!mayDispatchRecovery(task)) { + LOGGER.debug("Node {} marked task {} : {} CRASHED but not dispatching resume (non-executor)", + nodeId, task.getTaskType(), task.getItemId()); + return; + } LOGGER.info("Node {} resuming crashed task {} : {}", nodeId, task.getTaskType(), task.getItemId()); metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_RESUMED); - stateManager.resetTaskToScheduled(task); - if (lockManager.acquireLock(task)) { - executionManager.executeTask(task, executor); + // Keep CRASHED so the execution wrapper calls executor.resume(). Persist so the + // checker can retry if executeTask is a no-op (e.g. previous dispatch claim held). + // Do not pre-acquire the lock here — prepareForExecution owns locking. + if (!schedulerService.saveTask(task)) { + LOGGER.warn("Node {} failed to persist CRASHED state before resuming task {} : {} — " + + "checker safety-net retry may not trigger if the resume dispatch is a no-op", + nodeId, task.getTaskType(), task.getItemId()); } + executionManager.executeTask(task, executor); } /** @@ -259,11 +355,27 @@ private void attemptTaskResumption(ScheduledTask task, TaskExecutor executor) { * @param executor the executor for the task type */ private void attemptTaskRestart(ScheduledTask task, TaskExecutor executor) { + if (executor == null) { + LOGGER.warn("Node {} cannot restart task {} : {} — no executor registered", + nodeId, task.getTaskType(), task.getItemId()); + return; + } + if (!mayDispatchRecovery(task)) { + LOGGER.debug("Node {} marked task {} : {} CRASHED but not dispatching restart (non-executor)", + nodeId, task.getTaskType(), task.getItemId()); + return; + } LOGGER.info("Node {} restarting crashed task: {}", nodeId, task.getItemId()); stateManager.resetTaskToScheduled(task); - if (lockManager.acquireLock(task)) { - executionManager.executeTask(task, executor); + task.setNextScheduledExecution(new Date()); + // Persist SCHEDULED before best-effort dispatch so the checker is a safety net when + // executeTask no-ops (dispatch claim still held by a dying node's stalled wrapper). + if (!schedulerService.saveTask(task)) { + LOGGER.warn("Node {} failed to persist SCHEDULED state before restarting task {} : {} — " + + "checker safety-net retry may not trigger if the restart dispatch is a no-op", + nodeId, task.getTaskType(), task.getItemId()); } + executionManager.executeTask(task, executor); } /** @@ -271,9 +383,25 @@ private void attemptTaskRestart(ScheduledTask task, TaskExecutor executor) { */ private void recoverLockedTasks() { List lockedTasks = schedulerService.findLockedTasks(); + LOGGER.debug("LOCK-DIAG node {} : recoverLockedTasks() search (findLockedTasks, propertyCondition-based, " + + "NOT a real-time GET) found {} locked task(s): {}", + nodeId, lockedTasks.size(), taskSummary(lockedTasks)); for (ScheduledTask task : lockedTasks) { - if (lockManager.isLockExpired(task)) { + // RUNNING / CRASHED are owned by recoverRunningTasks (resume/restart). + // Releasing their locks here races the async dispatch that just acquired a new lock. + if (task.getStatus() == ScheduledTask.TaskStatus.RUNNING + || task.getStatus() == ScheduledTask.TaskStatus.CRASHED) { + LOGGER.debug("LOCK-DIAG [{}] node {} : recoverLockedTasks() skipping - status={} owned by " + + "recoverRunningTasks", + task.getItemId(), nodeId, task.getStatus()); + continue; + } + boolean expired = lockManager.isLockExpired(task); + LOGGER.debug("LOCK-DIAG [{}] node {} : recoverLockedTasks() candidate - status={}, lockOwner={}, " + + "lockDate={}, isLockExpired={}", + task.getItemId(), nodeId, task.getStatus(), task.getLockOwner(), task.getLockDate(), expired); + if (expired) { LOGGER.info("Node {} releasing expired lock for task: {}", nodeId, task.getItemId()); recoverLockedTask(task); } @@ -283,9 +411,20 @@ private void recoverLockedTasks() { /** * Releases an expired lock and reschedules the task when appropriate. * + *

    NOTE: {@code task} here comes from a SEARCH query ({@link SchedulerServiceImpl#findLockedTasks()}), + * not a real-time GET by ID. If the search index lags a moment behind a concurrent, legitimate + * lock acquisition/release cycle on this same task (elsewhere in {@code acquireDistributedLock()}), + * this method can act on a stale view: releasing a lock a live acquisition attempt just took, and/or + * re-dispatching a duplicate execution attempt via {@code executionManager.executeTask()} below, + * which independently races whatever the normal {@code checkTasks()} -> {@code processTaskGroup()} + * dispatch path is doing for the same task in the same or a nearby tick. + * * @param task the locked task to recover */ private void recoverLockedTask(ScheduledTask task) { + LOGGER.debug("LOCK-DIAG [{}] node {} : recoverLockedTask() - releasing lock owned by {} (search-view " + + "lockDate={}), then re-dispatching if status becomes SCHEDULED", + task.getItemId(), nodeId, task.getLockOwner(), task.getLockDate()); lockManager.releaseLock(task); // Check if task can be rescheduled @@ -295,16 +434,29 @@ private void recoverLockedTask(ScheduledTask task) { } if (schedulerService.saveTask(task)) { - // If task is now scheduled, try to execute it - if (task.getStatus() == ScheduledTask.TaskStatus.SCHEDULED) { + // If task is now scheduled, try to execute it (executor nodes / runOnAllNodes only) + if (task.getStatus() == ScheduledTask.TaskStatus.SCHEDULED && mayDispatchRecovery(task)) { TaskExecutor executor = executorRegistry.getExecutor(task.getTaskType()); if (executor != null) { + LOGGER.debug("LOCK-DIAG [{}] node {} : recoverLockedTask() re-dispatching via " + + "executionManager.executeTask() (recovery-path dispatch, bypasses the normal " + + "checkTasks()/processTaskGroup() dispatch claim ordering)", + task.getItemId(), nodeId); executionManager.executeTask(task, executor); } } } } + /** + * Whether this node may dispatch resume/restart after marking a task crashed. + * Non-executors may still persist CRASHED / clear locks, but only executors + * (or runOnAllNodes tasks) should run the work. + */ + private boolean mayDispatchRecovery(ScheduledTask task) { + return executorNode || task.isRunOnAllNodes(); + } + /** * Returns whether a crashed task should be restarted instead of abandoned. * @@ -312,17 +464,17 @@ private void recoverLockedTask(ScheduledTask task) { * @return {@code true} when the task should be restarted */ private boolean shouldRestartTask(ScheduledTask task) { - // Don't restart one-shot tasks that have already started - if (task.isOneShot() && task.getLastExecutionDate() != null) { - return false; - } - - // Check retry configuration - if (task.getMaxRetries() > 0 && task.getFailureCount() >= task.getMaxRetries()) { + if (!task.isEnabled()) { return false; } - return task.isEnabled(); + // Align with handleTaskError(): after a failure, failureCount is incremented and a + // retry is scheduled while failureCount <= maxRetries. A crash mid-attempt has not + // yet incremented failureCount for that attempt, so restart while still within budget. + // Importantly, do NOT abandon one-shots merely because lastExecutionDate is set — + // that field is written on every failure, and abandoning them stranded one-shots that + // crashed mid-retry with budget remaining. + return task.getFailureCount() <= task.getMaxRetries(); } /** diff --git a/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskStateManager.java b/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskStateManager.java index 07c118c50a..c8877bd725 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskStateManager.java +++ b/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskStateManager.java @@ -187,13 +187,20 @@ private Map getOrCreateStatusDetails(ScheduledTask task) { * @return true if all dependencies are completed */ public boolean canRescheduleTask(ScheduledTask task, Map dependencies) { - if (task.getWaitingOnTasks() == null || task.getWaitingOnTasks().isEmpty()) { + // Prefer the runtime waiting set; fall back to the configured dependsOn graph so + // dependencies are enforced even when waitingOnTasks was never populated. + Set required = task.getWaitingOnTasks(); + if (required == null || required.isEmpty()) { + required = task.getDependsOn(); + } + if (required == null || required.isEmpty()) { return true; } - for (String dependencyId : task.getWaitingOnTasks()) { - ScheduledTask dependency = dependencies.get(dependencyId); - if (dependency != null && dependency.getStatus() != TaskStatus.COMPLETED) { + for (String dependencyId : required) { + ScheduledTask dependency = dependencies != null ? dependencies.get(dependencyId) : null; + // Missing dependency is not satisfied — do not run until it exists and completes + if (dependency == null || dependency.getStatus() != TaskStatus.COMPLETED) { return false; } } diff --git a/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskValidationManager.java b/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskValidationManager.java index 8f1bf62d82..9f0741b40a 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskValidationManager.java +++ b/services/src/main/java/org/apache/unomi/services/impl/scheduler/TaskValidationManager.java @@ -92,6 +92,8 @@ private void validateDependency(String dependencyId, Map private void validateDependencyCycles(ScheduledTask task, Map existingTasks) { Set visited = new HashSet<>(); Set recursionStack = new HashSet<>(); + // Ensure the task under validation is present with its current dependsOn edge set + existingTasks.put(task.getItemId(), task); detectCycle(task.getItemId(), existingTasks, visited, recursionStack); } @@ -186,9 +188,10 @@ private boolean isValidTransition(ScheduledTask.TaskStatus from, ScheduledTask.T */ public void validateExecutionPrerequisites(ScheduledTask task, String nodeId) { if (task.getStatus() != ScheduledTask.TaskStatus.SCHEDULED && - task.getStatus() != ScheduledTask.TaskStatus.CRASHED) { + task.getStatus() != ScheduledTask.TaskStatus.CRASHED && + task.getStatus() != ScheduledTask.TaskStatus.WAITING) { throw new IllegalStateException( - "Task must be in SCHEDULED or CRASHED state to execute, current state: " + + "Task must be in SCHEDULED, CRASHED, or WAITING state to execute, current state: " + task.getStatus()); } diff --git a/services/src/test/java/org/apache/unomi/services/TestHelper.java b/services/src/test/java/org/apache/unomi/services/TestHelper.java index 80f9f9f3c6..9c06ddc495 100644 --- a/services/src/test/java/org/apache/unomi/services/TestHelper.java +++ b/services/src/test/java/org/apache/unomi/services/TestHelper.java @@ -322,6 +322,7 @@ public static SchedulerServiceImpl createSchedulerService( taskRecoveryManager.setMetricsManager(taskMetricsManager); taskRecoveryManager.setExecutionManager(taskExecutionManager); taskRecoveryManager.setExecutorRegistry(taskExecutorRegistry); + taskRecoveryManager.setExecutorNode(executorNode); // Task Validation Manager TaskValidationManager taskValidationManager = new TaskValidationManager(); @@ -447,6 +448,7 @@ public static SchedulerServiceImpl createSchedulerServiceWithoutPersistenceProvi taskRecoveryManager.setMetricsManager(taskMetricsManager); taskRecoveryManager.setExecutionManager(taskExecutionManager); taskRecoveryManager.setExecutorRegistry(taskExecutorRegistry); + taskRecoveryManager.setExecutorNode(executorNode); // Create scheduler service SchedulerServiceImpl schedulerService = new SchedulerServiceImpl(); diff --git a/services/src/test/java/org/apache/unomi/services/impl/InMemoryPersistenceServiceImpl.java b/services/src/test/java/org/apache/unomi/services/impl/InMemoryPersistenceServiceImpl.java index 3c2795b0fe..38b7fb1367 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/InMemoryPersistenceServiceImpl.java +++ b/services/src/test/java/org/apache/unomi/services/impl/InMemoryPersistenceServiceImpl.java @@ -32,7 +32,11 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.io.ObjectInputStream; +import java.io.ObjectOutputStream; import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; @@ -66,6 +70,8 @@ public class InMemoryPersistenceServiceImpl implements PersistenceService { private final Map sequenceNumbersByIndex = new ConcurrentHashMap<>(); private final Map primaryTermsByIndex = new ConcurrentHashMap<>(); private final Map fileLocks = new ConcurrentHashMap<>(); + /** Per-item-key mutex guarding save()'s read-check-write CAS sequence (see save()). */ + private final Map saveLocks = new ConcurrentHashMap<>(); private final ConditionEvaluatorDispatcher conditionEvaluatorDispatcher; private final ExecutionContextManager executionContextManager; private final CustomObjectMapper objectMapper; @@ -167,6 +173,7 @@ public InMemoryPersistenceServiceImpl(ExecutionContextManager executionContextMa this.defaultQueryLimit = defaultQueryLimit > 0 ? defaultQueryLimit : 10; // Initialize objectMapper even when file storage is disabled - it's needed for property mapping + // and file-backed persistence. Isolation copies use Java serialization (see deepCopyItem). this.objectMapper = new CustomObjectMapper(); if (prettyPrintJson) { this.objectMapper.enable(SerializationFeature.INDENT_OUTPUT); @@ -612,61 +619,80 @@ public boolean save(Item item, Boolean useBatching, Boolean alwaysOverwrite) { String indexName = getIndexName(item); String key = getKey(item.getItemId(), indexName); - Item existingItem = itemsById.get(key); - // Handle _seq_no and _primary_term fields using system metadata - // Check for optimistic concurrency control - if (existingItem != null) { - Object existingSeqNo = existingItem.getSystemMetadata("_seq_no"); - Object existingPrimaryTerm = existingItem.getSystemMetadata("_primary_term"); - - // If the item has _seq_no and _primary_term specified, check them against the existing item - Object requestedSeqNo = item.getSystemMetadata("_seq_no"); - Object requestedPrimaryTerm = item.getSystemMetadata("_primary_term"); - - if (requestedSeqNo != null && requestedPrimaryTerm != null) { - // If sequence numbers don't match the existing ones, it's a conflict - if (existingSeqNo != null && - ((Number) requestedSeqNo).longValue() != ((Number) existingSeqNo).longValue()) { - LOGGER.warn("Sequence number conflict detected for item {}: requested={}, current={}", - item.getItemId(), requestedSeqNo, existingSeqNo); - return false; - } + // The read (existingItem), the CAS precondition check, and the write (itemsById.put) + // below must happen as a single atomic unit per key - real ES/OS enforce this via + // per-document write ordering at the shard level. Without this lock, two threads can + // both read the same pre-write state, both pass the CAS check, and both "successfully" + // write - a real bug this exact double once had, masked for a long time because a + // since-removed caller-side post-write re-verification step (in TaskLockManager) + // happened to catch the resulting double-acquisition after the fact. Real ES/OS never + // needed that re-verification because they never had this race to begin with. + Object lock = saveLocks.computeIfAbsent(key, k -> new Object()); + synchronized (lock) { + Item existingItem = itemsById.get(key); + + // Optimistic concurrency control — matches ES/OpenSearch: only enforced when + // alwaysOverwrite is false. Accept both ES keys (seq_no / primary_term) used by + // TaskLockManager and legacy underscore-prefixed keys used by older unit tests. + // Default save(item) passes alwaysOverwrite=true and must overwrite (blind put), + // otherwise multi-node recovery races fail spuriously when a stale in-memory view + // still carries an older seq_no. + boolean overwrite = alwaysOverwrite == null || alwaysOverwrite; + if (existingItem != null && !overwrite) { + Object existingSeqNo = getSequenceMetadata(existingItem, true); + Object existingPrimaryTerm = getSequenceMetadata(existingItem, false); + + Object requestedSeqNo = getSequenceMetadata(item, true); + Object requestedPrimaryTerm = getSequenceMetadata(item, false); + + if (requestedSeqNo != null && requestedPrimaryTerm != null) { + if (existingSeqNo != null && + ((Number) requestedSeqNo).longValue() != ((Number) existingSeqNo).longValue()) { + LOGGER.warn("Sequence number conflict detected for item {}: requested={}, current={}", + item.getItemId(), requestedSeqNo, existingSeqNo); + return false; + } - // If primary terms don't match, it's a conflict - if (existingPrimaryTerm != null && - ((Number) requestedPrimaryTerm).longValue() != ((Number) existingPrimaryTerm).longValue()) { - LOGGER.warn("Primary term conflict detected for item {}: requested={}, current={}", - item.getItemId(), requestedPrimaryTerm, existingPrimaryTerm); - return false; + if (existingPrimaryTerm != null && + ((Number) requestedPrimaryTerm).longValue() != ((Number) existingPrimaryTerm).longValue()) { + LOGGER.warn("Primary term conflict detected for item {}: requested={}, current={}", + item.getItemId(), requestedPrimaryTerm, existingPrimaryTerm); + return false; + } } } - } - // Get sequence number for this item - Long currentSeqNo = getSequenceNumber(indexName, true); + // Get sequence number for this item + Long currentSeqNo = getSequenceNumber(indexName, true); - // Get primary term for this index - Long currentPrimaryTerm = getPrimaryTerm(indexName); + // Get primary term for this index + Long currentPrimaryTerm = getPrimaryTerm(indexName); - // Set the new sequence number and primary term on the item - item.setSystemMetadata("_seq_no", currentSeqNo); - item.setSystemMetadata("_primary_term", currentPrimaryTerm); + // Write both key styles so ES-aligned clients and legacy tests both see the values + item.setSystemMetadata("seq_no", currentSeqNo); + item.setSystemMetadata("primary_term", currentPrimaryTerm); + item.setSystemMetadata("_seq_no", currentSeqNo); + item.setSystemMetadata("_primary_term", currentPrimaryTerm); - // Handle item versioning (the existing version system) - if ((existingItem == null || existingItem.getVersion() == null) && (item.getVersion() == null)) { - // New item or item without version, set initial version - item.setVersion(1L); - } else { - // Existing item being updated, increment version - if (existingItem != null && existingItem.getVersion() != null) { - item.setVersion(existingItem.getVersion() + 1); + // Handle item versioning (the existing version system) + if ((existingItem == null || existingItem.getVersion() == null) && (item.getVersion() == null)) { + // New item or item without version, set initial version + item.setVersion(1L); } else { - item.setVersion(item.getVersion() + 1); + // Existing item being updated, increment version + if (existingItem != null && existingItem.getVersion() != null) { + item.setVersion(existingItem.getVersion() + 1); + } else { + item.setVersion(item.getVersion() + 1); + } } - } - itemsById.put(key, item); + // Store a deep copy so later mutations on the caller's instance (or a second node's + // local view) cannot silently change the persisted document — matching ES/OpenSearch + // deserialize-on-load semantics required for realistic multi-node scheduler tests. + itemsById.put(key, deepCopyItem(item)); + } if (fileStorageEnabled) { persistItem(item); @@ -862,8 +888,11 @@ private String getIndexName(Item item) { public T load(String itemId, Class clazz) { Item item = itemsById.get(getKey(itemId, getIndex(clazz))); if (item != null && clazz.isAssignableFrom(item.getClass()) && executionContextManager.getCurrentContext().getTenantId().equals(item.getTenantId())) { + // Return a deep copy so callers cannot mutate the stored document in place + // (matches Elasticsearch/OpenSearch load semantics for multi-node tests). + T copy = deepCopyItem((T) item); // Apply reverse tenant transformations after load (simulates Elasticsearch/OpenSearch behavior) - return (T) handleItemReverseTransformation(item); + return handleItemReverseTransformation(copy); } return null; } @@ -1499,14 +1528,15 @@ public boolean update(Item item, Date dateHint, Class clazz, Map source // Apply tenant transformations before save (simulates Elasticsearch/OpenSearch behavior) existingItem = handleItemTransformation(existingItem); - // Save updated item - itemsById.put(key, existingItem); + // Persist an isolated copy (same as save) so callers cannot mutate the store in place + itemsById.put(key, deepCopyItem(existingItem)); if (fileStorageEnabled) { persistItem(existingItem); } // Handle refresh policy per item type for updates (same as save) - // Request-based override (via system metadata) takes precedence over per-item-type policy + // Request-based override lives on the caller's item (API request metadata), not the + // stored document — use the request item when resolving refresh. if (simulateRefreshDelay) { String itemType = existingItem.getItemType(); if (existingItem instanceof CustomItem) { @@ -1516,7 +1546,7 @@ public boolean update(Item item, Date dateHint, Class clazz, Map source } } String indexName = getIndexName(existingItem); - RefreshPolicy refreshPolicy = getRefreshPolicy(itemType, existingItem); + RefreshPolicy refreshPolicy = getRefreshPolicy(itemType, item); switch (refreshPolicy) { case TRUE: @@ -2583,7 +2613,8 @@ private List filterItemsByClass(Class clazz) { // Filter out items that are not yet available for querying (refresh delay simulation) return isItemAvailableForQuery(itemKey, indexName); }) - .map(entry -> (T) entry.getValue()) + // Deep-copy each result so query callers get isolated instances (ES semantics) + .map(entry -> deepCopyItem((T) entry.getValue())) .collect(Collectors.toList()); } @@ -2900,4 +2931,46 @@ public Integer getDefaultQueryLimit() { public void setDefaultQueryLimit(Integer defaultQueryLimit) { this.defaultQueryLimit = defaultQueryLimit != null && defaultQueryLimit > 0 ? defaultQueryLimit : 10; } + + /** + * Reads sequence/primary-term metadata, accepting both ES keys ({@code seq_no}, + * {@code primary_term}) and legacy underscore-prefixed keys. + */ + private static Object getSequenceMetadata(Item item, boolean sequenceNumber) { + if (item == null) { + return null; + } + if (sequenceNumber) { + Object value = item.getSystemMetadata("seq_no"); + return value != null ? value : item.getSystemMetadata("_seq_no"); + } + Object value = item.getSystemMetadata("primary_term"); + return value != null ? value : item.getSystemMetadata("_primary_term"); + } + + /** + * Deep-copies an item via Java serialization so callers cannot mutate the stored + * document in place. Preserves field types (Integer vs Long), {@code @XmlTransient} + * scope, systemMetadata, and Date millis — unlike a Jackson round-trip, which drops + * or coerces several of those. Falls back to the original instance only if cloning fails. + */ + @SuppressWarnings("unchecked") + private T deepCopyItem(T item) { + if (item == null) { + return null; + } + try { + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + try (ObjectOutputStream oos = new ObjectOutputStream(baos)) { + oos.writeObject(item); + } + try (ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray()))) { + return (T) ois.readObject(); + } + } catch (Exception e) { + LOGGER.warn("Deep copy failed for item {} ({}), returning same instance: {}", + item.getItemId(), item.getClass().getSimpleName(), e.getMessage()); + return item; + } + } } diff --git a/services/src/test/java/org/apache/unomi/services/impl/InMemoryPersistenceServiceImplTest.java b/services/src/test/java/org/apache/unomi/services/impl/InMemoryPersistenceServiceImplTest.java index 54ef7064c7..9d1fc916b3 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/InMemoryPersistenceServiceImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/InMemoryPersistenceServiceImplTest.java @@ -4545,8 +4545,11 @@ void shouldRejectUpdateWithIncorrectSequenceNumber() { conflictItem.setSystemMetadata("_seq_no", initialSeqNo - 1); // Use wrong sequence number conflictItem.setSystemMetadata("_primary_term", initialPrimaryTerm); - // Try to save with incorrect sequence number, should fail - boolean saveResult = persistenceService.save(conflictItem); + // Try to save with incorrect sequence number, should fail. OCC is only enforced + // when alwaysOverwrite=false (matches ElasticSearchPersistenceServiceImpl/ + // OpenSearchPersistenceServiceImpl, where plain save(item) defaults to a blind + // overwrite and if_seq_no/if_primary_term are only sent via this explicit overload). + boolean saveResult = persistenceService.save(conflictItem, false, false); assertFalse(saveResult, "Save should fail with incorrect sequence number"); // Original item should still be there unchanged @@ -4574,8 +4577,11 @@ void shouldRejectUpdateWithIncorrectPrimaryTerm() { conflictItem.setSystemMetadata("_seq_no", initialSeqNo); conflictItem.setSystemMetadata("_primary_term", initialPrimaryTerm + 1); // Use wrong primary term - // Try to save with incorrect primary term, should fail - boolean saveResult = persistenceService.save(conflictItem); + // Try to save with incorrect primary term, should fail. OCC is only enforced + // when alwaysOverwrite=false (matches ElasticSearchPersistenceServiceImpl/ + // OpenSearchPersistenceServiceImpl, where plain save(item) defaults to a blind + // overwrite and if_seq_no/if_primary_term are only sent via this explicit overload). + boolean saveResult = persistenceService.save(conflictItem, false, false); assertFalse(saveResult, "Save should fail with incorrect primary term"); // Original item should still be there unchanged @@ -4603,8 +4609,10 @@ void shouldAllowUpdateWithCorrectSequenceNumberAndPrimaryTerm() { updateItem.setSystemMetadata("_seq_no", initialSeqNo); updateItem.setSystemMetadata("_primary_term", initialPrimaryTerm); - // Try to save with correct sequence number and primary term, should succeed - boolean saveResult = persistenceService.save(updateItem); + // Try to save with correct sequence number and primary term, should succeed. + // Use the CAS overload (matches saveTaskCompareAndSet) so this actually exercises + // the OCC-enforced path, not the default blind overwrite. + boolean saveResult = persistenceService.save(updateItem, false, false); assertTrue(saveResult, "Save should succeed with correct sequence number and primary term"); // Item should be updated with new name and incremented sequence number @@ -4613,6 +4621,35 @@ void shouldAllowUpdateWithCorrectSequenceNumberAndPrimaryTerm() { assertEquals(initialSeqNo + 1, ((Number) loaded.getSystemMetadata("_seq_no")).longValue()); assertEquals(initialPrimaryTerm, ((Number) loaded.getSystemMetadata("_primary_term")).longValue()); } + + @Test + void shouldBlindlyOverwriteRegardlessOfSequenceNumberByDefault() { + // Locks in the intentional blind-put contract of the default save() overload: + // it must succeed even with a stale/incorrect seq_no or primary_term, exactly + // matching ElasticSearchPersistenceServiceImpl/OpenSearchPersistenceServiceImpl, + // where plain save(item) defaults to alwaysOverwrite=true and never sends + // if_seq_no/if_primary_term. TaskLockManager and other legacy callers rely on + // this: only the explicit save(item, false, false) overload enforces OCC. If this + // test starts failing, do not "fix" it by making save(item) reject stale seq_no — + // that reintroduces the scheduler double-execution regression fixed in UNOMI-967. + TestMetadataItem item = new TestMetadataItem(); + item.setItemId("test-blind-overwrite"); + item.setName("Original Name"); + persistenceService.save(item); + Long initialSeqNo = ((Number) item.getSystemMetadata("_seq_no")).longValue(); + + TestMetadataItem staleWrite = new TestMetadataItem(); + staleWrite.setItemId(item.getItemId()); + staleWrite.setName("Blindly Overwritten Name"); + staleWrite.setSystemMetadata("_seq_no", initialSeqNo - 1); // stale on purpose + staleWrite.setSystemMetadata("_primary_term", 999L); // wrong on purpose + + boolean saveResult = persistenceService.save(staleWrite); + assertTrue(saveResult, "Default save(item) must blindly overwrite regardless of seq_no/primary_term"); + + TestMetadataItem loaded = persistenceService.load(item.getItemId(), TestMetadataItem.class); + assertEquals("Blindly Overwritten Name", loaded.getName()); + } } @Nested diff --git a/services/src/test/java/org/apache/unomi/services/impl/goals/GoalsServiceImplTest.java b/services/src/test/java/org/apache/unomi/services/impl/goals/GoalsServiceImplTest.java index c19e639736..c472c64ef9 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/goals/GoalsServiceImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/goals/GoalsServiceImplTest.java @@ -121,33 +121,42 @@ public void setUp() throws IOException { // Mock action type for goal rules - ActionType goalActionType = new ActionType() { - private Metadata metadata = new Metadata(); - @Override - public String getItemId() { - return "goalMatchedAction"; - } - @Override - public String getItemType() { - return "actionType"; - } - @Override - public Metadata getMetadata() { - return metadata; - } - @Override - public void setMetadata(Metadata metadata) { - this.metadata = metadata; - } - @Override - public Long getVersion() { - return 1L; - } - }; + ActionType goalActionType = new GoalMatchedActionType(); goalActionType.getMetadata().setId("goalMatchedAction"); definitionsService.setActionType(goalActionType); } + /** + * A named static nested class rather than an anonymous inner class: an anonymous class + * declared in an instance method implicitly captures a reference to the enclosing (non + * {@code Serializable}) test instance, which fails Java-serialization-based deep copy in + * {@code InMemoryPersistenceServiceImpl} and silently falls back to sharing the live + * instance instead of an isolated copy. + */ + private static final class GoalMatchedActionType extends ActionType { + private Metadata metadata = new Metadata(); + @Override + public String getItemId() { + return "goalMatchedAction"; + } + @Override + public String getItemType() { + return "actionType"; + } + @Override + public Metadata getMetadata() { + return metadata; + } + @Override + public void setMetadata(Metadata metadata) { + this.metadata = metadata; + } + @Override + public Long getVersion() { + return 1L; + } + } + @AfterEach public void tearDown() throws Exception { if (testEventAdmin != null) { diff --git a/services/src/test/java/org/apache/unomi/services/impl/scheduler/PersistenceSchedulerProviderTest.java b/services/src/test/java/org/apache/unomi/services/impl/scheduler/PersistenceSchedulerProviderTest.java new file mode 100644 index 0000000000..f191c210bb --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/impl/scheduler/PersistenceSchedulerProviderTest.java @@ -0,0 +1,270 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.unomi.services.impl.scheduler; + +import org.apache.unomi.api.ClusterNode; +import org.apache.unomi.api.PartialList; +import org.apache.unomi.api.conditions.Condition; +import org.apache.unomi.api.services.ClusterService; +import org.apache.unomi.api.tasks.ScheduledTask; +import org.apache.unomi.persistence.spi.PersistenceService; +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.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; + +import java.util.Arrays; +import java.util.Collections; +import java.util.Date; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +/** + * Unit tests for {@link PersistenceSchedulerProvider} (the only SchedulerProvider impl). + * Invoked by Surefire: {@code -Dtest=PersistenceSchedulerProviderTest}. No data-file I/O. + * Glob: no prior PersistenceSchedulerProviderTest. User asked for SchedulerProvider impl tests. + */ +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +public class PersistenceSchedulerProviderTest { + + @Mock private PersistenceService persistenceService; + @Mock private ClusterService clusterService; + @Mock private TaskLockManager lockManager; + + private PersistenceSchedulerProvider provider; + + @BeforeEach + public void setUp() { + provider = new PersistenceSchedulerProvider(); + provider.setPersistenceService(persistenceService); + provider.setNodeId("provider-node"); + provider.setExecutorNode(true); + provider.setCompletedTaskTtlDays(30); + provider.setLockManager(lockManager); + provider.setClusterService(clusterService); + } + + @Test + public void testSaveTaskNullAndNonPersistent() { + assertFalse(provider.saveTask(null)); + ScheduledTask mem = TaskTestFixtures.baseTask("m"); + mem.setPersistent(false); + assertFalse(provider.saveTask(mem)); + verify(persistenceService, never()).save(any()); + } + + @Test + public void testSaveTaskPropagatesBoolean() { + ScheduledTask task = TaskTestFixtures.baseTask("p"); + when(persistenceService.save(task)).thenReturn(true); + assertTrue(provider.saveTask(task)); + when(persistenceService.save(task)).thenReturn(false); + assertFalse(provider.saveTask(task)); + } + + @Test + public void testSaveTaskCompareAndSetUsesAlwaysOverwriteFalse() { + ScheduledTask task = TaskTestFixtures.baseTask("cas"); + when(persistenceService.save(task, false, false)).thenReturn(true); + assertTrue(provider.saveTaskCompareAndSet(task)); + verify(persistenceService).save(task, false, false); + assertFalse(provider.saveTaskCompareAndSet(null)); + ScheduledTask mem = TaskTestFixtures.baseTask("m"); + mem.setPersistent(false); + assertFalse(provider.saveTaskCompareAndSet(mem)); + } + + @Test + public void testFindTasksByLockOwnerNullPersistence() { + provider.setPersistenceService(null); + assertTrue(provider.findTasksByLockOwner("x").isEmpty()); + } + + @Test + public void testFindEnabledScheduledOrWaitingIncludesCrashed() { + ScheduledTask crashed = TaskTestFixtures.baseTask("c"); + crashed.setStatus(ScheduledTask.TaskStatus.CRASHED); + PartialList result = new PartialList<>( + Collections.singletonList(crashed), 0, 1, 1, PartialList.Relation.EQUAL); + when(persistenceService.query(any(Condition.class), anyString(), eq(ScheduledTask.class), eq(0), eq(-1))) + .thenReturn(result); + + List found = provider.findEnabledScheduledOrWaitingTasks(); + assertEquals(1, found.size()); + + ArgumentCaptor conditionCaptor = ArgumentCaptor.forClass(Condition.class); + verify(persistenceService).query(conditionCaptor.capture(), eq("creationDate:asc"), + eq(ScheduledTask.class), eq(0), eq(-1)); + Condition and = conditionCaptor.getValue(); + @SuppressWarnings("unchecked") + List subs = (List) and.getParameter("subConditions"); + Condition status = subs.get(1); + @SuppressWarnings("unchecked") + List values = (List) status.getParameter("propertyValues"); + assertTrue(values.contains(ScheduledTask.TaskStatus.CRASHED)); + assertTrue(values.contains(ScheduledTask.TaskStatus.SCHEDULED)); + assertTrue(values.contains(ScheduledTask.TaskStatus.WAITING)); + } + + @Test + public void testPurgeOldTasksSkippedOnNonExecutor() { + provider.setExecutorNode(false); + provider.purgeOldTasks(); + verify(persistenceService, never()).removeByQuery(any(), eq(ScheduledTask.class)); + } + + @Test + public void testPurgeOldTasksOnExecutor() { + provider.purgeOldTasks(); + verify(persistenceService).removeByQuery(any(Condition.class), eq(ScheduledTask.class)); + } + + @Test + public void testDeleteTaskNullIsNoOp() { + provider.deleteTask(null); + verify(persistenceService, never()).remove(anyString(), eq(ScheduledTask.class)); + } + + @Test + public void testGetActiveNodesIncludesSelfAndRecentClusterHeartbeats() { + ClusterNode fresh = new ClusterNode(); + fresh.setItemId("peer"); + fresh.setLastHeartbeat(System.currentTimeMillis()); + ClusterNode stale = new ClusterNode(); + stale.setItemId("stale"); + stale.setLastHeartbeat(System.currentTimeMillis() - (10 * 60 * 1000)); + when(clusterService.getClusterNodes()).thenReturn(Arrays.asList(fresh, stale)); + + List nodes = provider.getActiveNodes(); + assertTrue(nodes.contains("provider-node")); + assertTrue(nodes.contains("peer")); + assertFalse(nodes.contains("stale")); + } + + @Test + public void testGetActiveNodesFallbackWhenClusterUnavailable() { + when(clusterService.getClusterNodes()).thenThrow(new RuntimeException("down")); + ScheduledTask locked = TaskTestFixtures.baseTask("l"); + locked.setLockOwner("fallback-peer"); + locked.setLockDate(new Date()); + when(persistenceService.query(any(Condition.class), anyString(), eq(ScheduledTask.class))) + .thenReturn(Collections.singletonList(locked)); + + List nodes = provider.getActiveNodes(); + assertTrue(nodes.contains("provider-node")); + assertTrue(nodes.contains("fallback-peer")); + } + + @Test + public void testPreDestroyReleasesLocksOnNonRunningTasks() { + ScheduledTask locked = TaskTestFixtures.baseTask("l"); + locked.setLockOwner("provider-node"); + locked.setLockDate(new Date()); + locked.setStatus(ScheduledTask.TaskStatus.SCHEDULED); + PartialList result = new PartialList<>( + Collections.singletonList(locked), 0, 1, 1, PartialList.Relation.EQUAL); + when(persistenceService.query(any(Condition.class), isNull(), eq(ScheduledTask.class), eq(0), eq(-1))) + .thenReturn(result); + when(lockManager.releaseLock(locked)).thenReturn(true); + + provider.preDestroy(); + verify(lockManager).releaseLock(locked); + } + + @Test + public void testPreDestroySkipsRunningAndCrashedLocks() { + ScheduledTask running = TaskTestFixtures.runningTask("r", "provider-node"); + ScheduledTask crashed = TaskTestFixtures.baseTask("c"); + crashed.setStatus(ScheduledTask.TaskStatus.CRASHED); + crashed.setLockOwner("provider-node"); + crashed.setLockDate(new Date()); + PartialList result = new PartialList<>( + Arrays.asList(running, crashed), 0, 2, 2, PartialList.Relation.EQUAL); + when(persistenceService.query(any(Condition.class), isNull(), eq(ScheduledTask.class), eq(0), eq(-1))) + .thenReturn(result); + + provider.preDestroy(); + verify(lockManager, never()).releaseLock(any()); + } + + @Test + public void testQueryExceptionReturnsEmpty() { + when(persistenceService.query(any(Condition.class), any(), eq(ScheduledTask.class), anyInt(), anyInt())) + .thenThrow(new RuntimeException("boom")); + assertTrue(provider.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING).isEmpty()); + PartialList page = provider.getTasksByStatus(ScheduledTask.TaskStatus.SCHEDULED, 0, 10, null); + assertTrue(page.getList().isEmpty()); + } + + @Test + public void testGetTaskAndRefresh() { + ScheduledTask task = TaskTestFixtures.baseTask("g"); + when(persistenceService.load(task.getItemId(), ScheduledTask.class)).thenReturn(task); + assertEquals(task, provider.getTask(task.getItemId())); + provider.refreshTasks(); + verify(persistenceService).refreshIndex(ScheduledTask.class); + } + + @Test + public void testSaveMethodsReturnFalseOnPersistenceException() { + ScheduledTask task = TaskTestFixtures.baseTask("ex"); + when(persistenceService.save(task)).thenThrow(new RuntimeException("disk full")); + when(persistenceService.save(task, false, false)).thenThrow(new RuntimeException("conflict store")); + assertFalse(provider.saveTask(task)); + assertFalse(provider.saveTaskCompareAndSet(task)); + } + + @Test + public void testFindLockedTasksQueryExcludesRunningAndCrashed() { + PartialList result = new PartialList<>( + Collections.emptyList(), 0, 0, 0, PartialList.Relation.EQUAL); + when(persistenceService.query(any(Condition.class), isNull(), eq(ScheduledTask.class), eq(0), eq(-1))) + .thenReturn(result); + + assertTrue(provider.findLockedTasks().isEmpty()); + + ArgumentCaptor conditionCaptor = ArgumentCaptor.forClass(Condition.class); + verify(persistenceService).query(conditionCaptor.capture(), isNull(), + eq(ScheduledTask.class), eq(0), eq(-1)); + Condition and = conditionCaptor.getValue(); + @SuppressWarnings("unchecked") + List subs = (List) and.getParameter("subConditions"); + Condition status = subs.get(1); + @SuppressWarnings("unchecked") + List values = (List) status.getParameter("propertyValues"); + assertTrue(values.contains(ScheduledTask.TaskStatus.SCHEDULED)); + assertTrue(values.contains(ScheduledTask.TaskStatus.WAITING)); + assertFalse(values.contains(ScheduledTask.TaskStatus.RUNNING)); + assertFalse(values.contains(ScheduledTask.TaskStatus.CRASHED)); + } + + @Test + public void testPreDestroySkipsWhenPersistenceUnavailable() { + provider.setPersistenceService(null); + provider.preDestroy(); + verify(lockManager, never()).releaseLock(any()); + } +} diff --git a/services/src/test/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceClusterRaceTest.java b/services/src/test/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceClusterRaceTest.java new file mode 100644 index 0000000000..62ae062bd8 --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceClusterRaceTest.java @@ -0,0 +1,1186 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.services.impl.scheduler; + +import org.apache.unomi.api.tasks.ScheduledTask; +import org.apache.unomi.api.tasks.TaskExecutor; +import org.apache.unomi.persistence.spi.CustomObjectMapper; +import org.apache.unomi.persistence.spi.PersistenceService; +import org.apache.unomi.persistence.spi.conditions.evaluator.ConditionEvaluatorDispatcher; +import org.apache.unomi.services.TestHelper; +import org.apache.unomi.services.common.security.ExecutionContextManagerImpl; +import org.apache.unomi.services.common.security.KarafSecurityService; +import org.apache.unomi.services.impl.InMemoryPersistenceServiceImpl; +import org.apache.unomi.services.impl.TestConditionEvaluators; +import org.apache.unomi.services.impl.cluster.ClusterServiceImpl; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; +import org.osgi.framework.Bundle; +import org.osgi.framework.BundleContext; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Date; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Multi-node race and failure scenarios for the scheduler. + * + * These tests rely on InMemoryPersistenceServiceImpl deep-copy + OCC semantics so each + * node's load/query returns an isolated instance — matching Elasticsearch behaviour and + * exposing split-brain / lock / affinity bugs that shared-identity harnesses hide. + * + * Invoked by Surefire as part of the services module unit suite + * ({@code mvn -pl services test -Dtest=SchedulerServiceClusterRaceTest}). + */ +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +@Tag("ClusterTests") +public class SchedulerServiceClusterRaceTest { + private static final Logger LOGGER = LoggerFactory.getLogger(SchedulerServiceClusterRaceTest.class); + + private static final long TEST_TIMEOUT_MS = 20000; + private static final long SHORT_LOCK_TIMEOUT_MS = 800; + private static final int MAX_RETRIES = 10; + + private PersistenceService persistenceService; + private ExecutionContextManagerImpl executionContextManager; + private KarafSecurityService securityService; + private ClusterServiceImpl clusterService; + private final List nodes = new ArrayList<>(); + + @Mock + private BundleContext bundleContext; + + @BeforeEach + public void setUp() throws IOException { + CustomObjectMapper.getCustomInstance().registerBuiltInItemTypeClass(ScheduledTask.ITEM_TYPE, ScheduledTask.class); + securityService = TestHelper.createSecurityService(); + executionContextManager = TestHelper.createExecutionContextManager(securityService); + ConditionEvaluatorDispatcher conditionEvaluatorDispatcher = TestConditionEvaluators.createDispatcher(); + + Bundle bundle = mock(Bundle.class); + when(bundleContext.getBundle()).thenReturn(bundle); + when(bundle.getBundleContext()).thenReturn(bundleContext); + when(bundleContext.getBundle().findEntries(anyString(), anyString(), anyBoolean())).thenReturn(null); + when(bundleContext.getBundles()).thenReturn(new Bundle[0]); + + TestHelper.cleanDefaultStorageDirectory(MAX_RETRIES); + InMemoryPersistenceServiceImpl inMemory = + new InMemoryPersistenceServiceImpl(executionContextManager, conditionEvaluatorDispatcher); + inMemory.setRefreshPolicy(ScheduledTask.ITEM_TYPE, + InMemoryPersistenceServiceImpl.RefreshPolicy.WAIT_FOR); + persistenceService = inMemory; + clusterService = TestHelper.createClusterService(persistenceService, "cluster-race-seed", bundleContext); + } + + @AfterEach + public void tearDown() { + for (SchedulerServiceImpl node : nodes) { + try { + node.preDestroy(); + } catch (Exception e) { + LOGGER.warn("Error destroying node {}: {}", node.getNodeId(), e.getMessage()); + } + } + nodes.clear(); + } + + private SchedulerServiceImpl createNode(String nodeId, boolean executorNode, long lockTimeoutMs) { + SchedulerServiceImpl node = (SchedulerServiceImpl) TestHelper.createSchedulerService( + nodeId, persistenceService, executionContextManager, bundleContext, clusterService, + lockTimeoutMs, executorNode, true, 0); + node.setLockTimeout(lockTimeoutMs); + nodes.add(node); + return node; + } + + private void seedActiveNodes(String... nodeIds) { + for (String id : nodeIds) { + ScheduledTask marker = new ScheduledTask(); + marker.setItemId(id + "-active-marker"); + marker.setTaskType("cluster-active-marker"); + marker.setPersistent(true); + marker.setEnabled(false); + marker.setStatus(ScheduledTask.TaskStatus.COMPLETED); + marker.setOneShot(true); + marker.setCreationDate(new Date()); + marker.setLockOwner(id); + marker.setLockDate(new Date()); + persistenceService.save(marker); + } + persistenceService.refreshIndex(ScheduledTask.class); + persistenceService.refresh(); + } + + private ScheduledTask waitForStatus(SchedulerServiceImpl node, String taskId, + ScheduledTask.TaskStatus status, long timeoutMs) throws InterruptedException { + long deadline = System.currentTimeMillis() + timeoutMs; + ScheduledTask task = null; + while (System.currentTimeMillis() < deadline) { + task = node.getTask(taskId); + if (task != null && status.equals(task.getStatus())) { + return task; + } + Thread.sleep(50); + } + return task; + } + + /** + * Asserts that two nodes see the same logical task via independent deep-copied instances. + * Shared identity would mean the in-memory harness is leaking store references across nodes. + */ + private void assertDistinctTaskViews(SchedulerServiceImpl nodeA, SchedulerServiceImpl nodeB, String taskId) { + ScheduledTask a = nodeA.getTask(taskId); + ScheduledTask b = nodeB.getTask(taskId); + assertNotNull(a, "nodeA must see task " + taskId); + assertNotNull(b, "nodeB must see task " + taskId); + assertNotSame(a, b, "Nodes must not share the same ScheduledTask instance in memory"); + assertEquals(a.getItemId(), b.getItemId()); + assertEquals(a.getStatus(), b.getStatus()); + if (a.getCheckpointData() != null && b.getCheckpointData() != null) { + assertNotSame(a.getCheckpointData(), b.getCheckpointData(), + "checkpointData maps must be deep-copied per load"); + } + if (a.getStatusDetails() != null && b.getStatusDetails() != null) { + assertNotSame(a.getStatusDetails(), b.getStatusDetails(), + "statusDetails maps must be deep-copied per load"); + } + } + + private void expireLockInStore(String taskId) { + ScheduledTask stored = persistenceService.load(taskId, ScheduledTask.class); + assertNotNull(stored, "task must exist to expire lock: " + taskId); + stored.setLockDate(new Date(System.currentTimeMillis() - 60_000)); + assertTrue(persistenceService.save(stored)); + persistenceService.refreshIndex(ScheduledTask.class); + persistenceService.refresh(); + } + + private void registerOnAll(TaskExecutor executor, SchedulerServiceImpl... nodeArr) { + for (SchedulerServiceImpl node : nodeArr) { + node.registerTaskExecutor(executor); + } + } + + @Test + public void testInMemoryLoadReturnsIsolatedInstance() { + ScheduledTask original = new ScheduledTask(); + original.setItemId("isolate-1"); + original.setTaskType("isolate"); + original.setPersistent(true); + original.setEnabled(true); + original.setStatus(ScheduledTask.TaskStatus.SCHEDULED); + original.setOneShot(true); + original.setCreationDate(new Date()); + Map checkpoint = new HashMap<>(); + checkpoint.put("step", 1); + original.setCheckpointData(checkpoint); + Map details = new HashMap<>(); + details.put("note", "seed"); + original.setStatusDetails(details); + persistenceService.save(original); + + // Mutating the caller's instance after save must not rewrite the store (save stores a copy). + original.setStatus(ScheduledTask.TaskStatus.RUNNING); + original.getCheckpointData().put("step", 99); + + ScheduledTask loaded1 = persistenceService.load("isolate-1", ScheduledTask.class); + ScheduledTask loaded2 = persistenceService.load("isolate-1", ScheduledTask.class); + assertNotSame(loaded1, loaded2, "Each load must return a distinct instance"); + assertNotSame(original, loaded1, "Load must not return the caller's instance"); + assertEquals(ScheduledTask.TaskStatus.SCHEDULED, loaded1.getStatus(), + "Post-save mutation of caller must not affect stored document"); + assertEquals(1, ((Number) loaded1.getCheckpointData().get("step")).intValue()); + + loaded1.setStatus(ScheduledTask.TaskStatus.RUNNING); + loaded1.getCheckpointData().put("step", 42); + loaded1.getStatusDetails().put("note", "mutated"); + ScheduledTask loaded3 = persistenceService.load("isolate-1", ScheduledTask.class); + assertEquals(ScheduledTask.TaskStatus.SCHEDULED, loaded3.getStatus(), + "Mutating a loaded copy must not change the stored document"); + assertEquals(1, ((Number) loaded3.getCheckpointData().get("step")).intValue()); + assertEquals("seed", loaded3.getStatusDetails().get("note")); + assertNotSame(loaded1.getCheckpointData(), loaded3.getCheckpointData()); + assertNotSame(loaded1.getStatusDetails(), loaded3.getStatusDetails()); + } + + @Test + public void testOptimisticConcurrencyConflictRejectsStaleLockWrite() throws Exception { + SchedulerServiceImpl node1 = createNode("occ-node1", true, 5000); + createNode("occ-node2", true, 5000); + seedActiveNodes("occ-node1", "occ-node2"); + + CountDownLatch started = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + + node1.newTask("occ-lock-test") + .disallowParallelExecution() + .asOneShot() + .withSimpleExecutor(() -> { + started.countDown(); + try { + release.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }) + .schedule(); + + assertTrue(started.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + ScheduledTask locked = persistenceService.load( + persistenceService.query("taskType", "occ-lock-test", null, ScheduledTask.class).get(0).getItemId(), + ScheduledTask.class); + assertNotNull(locked.getLockOwner()); + Object staleSeq = locked.getSystemMetadata("seq_no"); + assertNotNull(staleSeq, "Stored task must expose ES-aligned seq_no"); + + ScheduledTask fresh = persistenceService.load(locked.getItemId(), ScheduledTask.class); + fresh.setCurrentStep("bump"); + assertTrue(persistenceService.save(fresh), "Fresh save should succeed"); + + locked.setLockOwner("occ-node2"); + locked.setSystemMetadata("seq_no", staleSeq); + locked.setSystemMetadata("_seq_no", staleSeq); + // alwaysOverwrite=false is required for OCC (matches ES/OpenSearch + lock CAS path) + assertFalse(persistenceService.save(locked, false, false), "Stale seq_no write must be rejected"); + release.countDown(); + } + + @Test + public void testExclusiveLockPreventsDualNodeDoubleExecution() throws Exception { + SchedulerServiceImpl node1 = createNode("lock-node1", true, 10000); + SchedulerServiceImpl node2 = createNode("lock-node2", true, 10000); + seedActiveNodes("lock-node1", "lock-node2"); + + CountDownLatch started = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + AtomicInteger executions = new AtomicInteger(0); + Set executors = ConcurrentHashMap.newKeySet(); + + TaskExecutor executor = new TaskExecutor() { + @Override + public String getTaskType() { + return "exclusive-dual-node"; + } + + @Override + public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + executions.incrementAndGet(); + executors.add(task.getExecutingNodeId()); + started.countDown(); + assertTrue(release.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + callback.complete(); + } + }; + node1.registerTaskExecutor(executor); + node2.registerTaskExecutor(executor); + + ScheduledTask task = node1.newTask("exclusive-dual-node") + .disallowParallelExecution() + .asOneShot() + .schedule(); + + assertTrue(started.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS), "One node should start the task"); + Thread.sleep(2500); + release.countDown(); + + ScheduledTask done = waitForStatus(node1, task.getItemId(), ScheduledTask.TaskStatus.COMPLETED, TEST_TIMEOUT_MS); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, done.getStatus()); + assertEquals(1, executions.get(), "Exclusive task must execute exactly once across the cluster"); + assertEquals(1, executors.size(), "Exactly one node should have executed"); + } + + @Test + public void testAffinityOpenFieldAfterBackupWindowsWhenPrimaryDead() throws Exception { + SchedulerServiceImpl backup1 = createNode("aff-backup1", true, 10000); + SchedulerServiceImpl backup2 = createNode("aff-backup2", true, 10000); + seedActiveNodes("aff-primary-dead", "aff-backup1", "aff-backup2"); + + CountDownLatch executed = new CountDownLatch(1); + AtomicReference executorNode = new AtomicReference<>(); + + TaskExecutor executor = new TaskExecutor() { + @Override + public String getTaskType() { + return "affinity-failover"; + } + + @Override + public void execute(ScheduledTask task, TaskStatusCallback callback) { + executorNode.set(task.getExecutingNodeId()); + executed.countDown(); + callback.complete(); + } + }; + backup1.registerTaskExecutor(executor); + backup2.registerTaskExecutor(executor); + + ScheduledTask task = backup1.newTask("affinity-failover") + .disallowParallelExecution() + .asOneShot() + .schedule(); + + assertTrue(executed.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "A backup node must acquire the task after affinity windows expire (open field)"); + assertTrue( + "aff-backup1".equals(executorNode.get()) || "aff-backup2".equals(executorNode.get()), + "Execution must be on a live backup, not the dead primary"); + + ScheduledTask done = waitForStatus(backup1, task.getItemId(), ScheduledTask.TaskStatus.COMPLETED, TEST_TIMEOUT_MS); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, done.getStatus()); + } + + @Test + public void testDeadNodeMidRetryIsRecoveredByPeer() throws Exception { + long lockTimeout = SHORT_LOCK_TIMEOUT_MS; + SchedulerServiceImpl node1 = createNode("retry-node1", true, lockTimeout); + SchedulerServiceImpl node2 = createNode("retry-node2", true, lockTimeout); + seedActiveNodes("retry-node1", "retry-node2"); + + CountDownLatch retryAttemptStarted = new CountDownLatch(1); + CountDownLatch completed = new CountDownLatch(1); + AtomicInteger attempts = new AtomicInteger(0); + + TaskExecutor executor = new TaskExecutor() { + @Override + public String getTaskType() { + return "dead-mid-retry"; + } + + @Override + public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + int n = attempts.incrementAndGet(); + if (n == 1) { + callback.fail("first failure"); + return; + } + if (n == 2) { + retryAttemptStarted.countDown(); + Thread.sleep(lockTimeout + 1500); + callback.complete(); + completed.countDown(); + return; + } + callback.complete(); + completed.countDown(); + } + }; + node1.registerTaskExecutor(executor); + node2.registerTaskExecutor(executor); + + ScheduledTask task = node1.newTask("dead-mid-retry") + .disallowParallelExecution() + .withMaxRetries(3) + .withRetryDelay(200, TimeUnit.MILLISECONDS) + .asOneShot() + .schedule(); + + assertTrue(retryAttemptStarted.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "Retry attempt should start"); + node1.simulateCrash(); + + assertTrue(completed.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "Peer node must complete the task after the owning node dies mid-retry"); + + ScheduledTask finalTask = waitForStatus(node2, task.getItemId(), ScheduledTask.TaskStatus.COMPLETED, TEST_TIMEOUT_MS); + assertNotNull(finalTask); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, finalTask.getStatus(), + "Task must reach COMPLETED, not stay stranded in CRASHED/SCHEDULED"); + assertTrue(attempts.get() >= 2, "At least the initial failure and a recovery/retry attempt"); + } + + @Test + public void testCheckpointResumeIsInvokedNotExecute() throws Exception { + // Long lock timeout so the peer does not steal the task via lock-expiry recovery + // while the executor is still inside execute() — that path can restart (SCHEDULED) + // instead of resume() if it races the checkpoint write. + long lockTimeoutMs = TEST_TIMEOUT_MS; + SchedulerServiceImpl node1 = createNode("resume-node1", true, lockTimeoutMs); + SchedulerServiceImpl node2 = createNode("resume-node2", true, lockTimeoutMs); + seedActiveNodes("resume-node1", "resume-node2"); + + CountDownLatch firstStarted = new CountDownLatch(1); + CountDownLatch holdExecution = new CountDownLatch(1); + CountDownLatch resumeCalled = new CountDownLatch(1); + AtomicInteger executeCount = new AtomicInteger(0); + AtomicInteger resumeCount = new AtomicInteger(0); + AtomicReference executingNodeId = new AtomicReference<>(); + + TaskExecutor executor = new TaskExecutor() { + @Override + public String getTaskType() { + return "checkpoint-resume"; + } + + @Override + public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + executeCount.incrementAndGet(); + executingNodeId.set(task.getExecutingNodeId()); + callback.checkpoint(Collections.singletonMap("step", 1)); + firstStarted.countDown(); + // Hold until the test crashes this node (interrupt) or releases the latch. + holdExecution.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS); + callback.complete(); + } + + @Override + public boolean canResume(ScheduledTask task) { + return task.getCheckpointData() != null; + } + + @Override + public void resume(ScheduledTask task, TaskStatusCallback callback) { + resumeCount.incrementAndGet(); + resumeCalled.countDown(); + callback.complete(); + } + }; + node1.registerTaskExecutor(executor); + node2.registerTaskExecutor(executor); + + ScheduledTask task = node1.newTask("checkpoint-resume") + .disallowParallelExecution() + .asOneShot() + .schedule(); + + assertTrue(firstStarted.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS), "First attempt should start"); + assertNotNull(executingNodeId.get(), "execute() must record the owning node"); + + // Crash the node that holds the in-process dispatch claim. Affinity may pick either + // node; crashing the peer leaves the claim held and blocks resume dispatch. + SchedulerServiceImpl crashed = "resume-node1".equals(executingNodeId.get()) ? node1 : node2; + SchedulerServiceImpl survivor = crashed == node1 ? node2 : node1; + crashed.simulateCrash(); + + // simulateCrash releases the lock; mark lock expired from the survivor's perspective + // by clearing any residual lock metadata, then drive recoverCrashedTasks(). + ScheduledTask afterCrash = persistenceService.load(task.getItemId(), ScheduledTask.class); + assertEquals(ScheduledTask.TaskStatus.RUNNING, afterCrash.getStatus(), + "Task must still be RUNNING after executor crash (not prematurely rescheduled)"); + assertNotNull(afterCrash.getCheckpointData(), "Checkpoint must be persisted before crash"); + + long deadline = System.currentTimeMillis() + TEST_TIMEOUT_MS; + while (resumeCount.get() == 0 && System.currentTimeMillis() < deadline) { + survivor.recoverCrashedTasks(); + if (resumeCalled.await(200, TimeUnit.MILLISECONDS)) { + break; + } + } + + assertTrue(resumeCount.get() > 0, + "Recovery must call resume(), not execute(), when canResume is true"); + assertEquals(1, resumeCount.get(), "resume() should be invoked exactly once"); + assertEquals(1, executeCount.get(), "execute() should only have run on the original node"); + + ScheduledTask done = waitForStatus(survivor, task.getItemId(), ScheduledTask.TaskStatus.COMPLETED, TEST_TIMEOUT_MS); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, done.getStatus()); + holdExecution.countDown(); + } + + @Test + public void testOneShotCrashedMidRetryIsRestartedWhenBudgetRemains() throws Exception { + SchedulerServiceImpl node = createNode("crash-budget-node", true, SHORT_LOCK_TIMEOUT_MS); + + CountDownLatch completed = new CountDownLatch(1); + AtomicInteger attempts = new AtomicInteger(0); + + TaskExecutor executor = new TaskExecutor() { + @Override + public String getTaskType() { + return "crash-budget"; + } + + @Override + public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + int n = attempts.incrementAndGet(); + if (n == 1) { + callback.fail("fail once so lastExecutionDate is set"); + return; + } + if (n == 2) { + Thread.sleep(SHORT_LOCK_TIMEOUT_MS + 2500); + callback.fail("stalled attempt"); + return; + } + completed.countDown(); + callback.complete(); + } + }; + node.registerTaskExecutor(executor); + + ScheduledTask task = node.newTask("crash-budget") + .withMaxRetries(3) + .withRetryDelay(200, TimeUnit.MILLISECONDS) + .asOneShot() + .schedule(); + + assertTrue(completed.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "One-shot with prior failure must be restarted after CRASHED when retry budget remains"); + ScheduledTask done = waitForStatus(node, task.getItemId(), ScheduledTask.TaskStatus.COMPLETED, TEST_TIMEOUT_MS); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, done.getStatus()); + assertTrue(attempts.get() >= 3, "Expected fail, crashed attempt, then successful restart"); + } + + @Test + public void testPeriodicFailureCountResetsBetweenPeriods() throws Exception { + SchedulerServiceImpl node = createNode("period-reset-node", true, 10000); + + CountDownLatch period2Retried = new CountDownLatch(1); + AtomicInteger attempts = new AtomicInteger(0); + AtomicInteger period2Attempts = new AtomicInteger(0); + + TaskExecutor executor = new TaskExecutor() { + @Override + public String getTaskType() { + return "period-reset"; + } + + @Override + public void execute(ScheduledTask task, TaskStatusCallback callback) { + int n = attempts.incrementAndGet(); + if (n <= 2) { + callback.fail("period1 failure #" + n); + return; + } + int p2 = period2Attempts.incrementAndGet(); + if (p2 == 1) { + callback.fail("period2 first failure"); + return; + } + period2Retried.countDown(); + callback.complete(); + } + }; + node.registerTaskExecutor(executor); + + node.newTask("period-reset") + .withMaxRetries(1) + .withRetryDelay(100, TimeUnit.MILLISECONDS) + .withPeriod(800, TimeUnit.MILLISECONDS) + .withFixedDelay() + .schedule(); + + assertTrue(period2Retried.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "Period 2 must get a fresh retry budget after period 1 exhausted retries"); + assertTrue(period2Attempts.get() >= 2, "Period 2 should have retried after first failure"); + } + + @Test + public void testDependencyBlocksUntilBothCompleteAcrossNodes() throws Exception { + SchedulerServiceImpl node1 = createNode("dep-node1", true, 10000); + SchedulerServiceImpl node2 = createNode("dep-node2", true, 10000); + seedActiveNodes("dep-node1", "dep-node2"); + + CountDownLatch dep1Started = new CountDownLatch(1); + CountDownLatch dep1Release = new CountDownLatch(1); + CountDownLatch dep2Done = new CountDownLatch(1); + CountDownLatch dependentDone = new CountDownLatch(1); + AtomicBoolean dependentRanEarly = new AtomicBoolean(false); + + TaskExecutor executor = new TaskExecutor() { + @Override + public String getTaskType() { + return "cross-node-dep"; + } + + @Override + public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + String name = String.valueOf(task.getParameters().get("name")); + if ("dependent".equals(name)) { + if (dep1Release.getCount() > 0) { + dependentRanEarly.set(true); + } + dependentDone.countDown(); + callback.complete(); + return; + } + if ("dep1".equals(name)) { + dep1Started.countDown(); + assertTrue(dep1Release.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + callback.complete(); + return; + } + dep2Done.countDown(); + callback.complete(); + } + }; + node1.registerTaskExecutor(executor); + node2.registerTaskExecutor(executor); + + ScheduledTask dep1 = node1.newTask("cross-node-dep") + .withParameters(Collections.singletonMap("name", "dep1")) + .disallowParallelExecution() + .asOneShot() + .schedule(); + assertTrue(dep1Started.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + + ScheduledTask dep2 = node2.newTask("cross-node-dep") + .withParameters(Collections.singletonMap("name", "dep2")) + .asOneShot() + .schedule(); + + ScheduledTask dependent = node1.newTask("cross-node-dep") + .withParameters(Collections.singletonMap("name", "dependent")) + .withDependencies(dep1.getItemId(), dep2.getItemId()) + .asOneShot() + .schedule(); + + ScheduledTask waiting = node1.getTask(dependent.getItemId()); + assertEquals(ScheduledTask.TaskStatus.WAITING, waiting.getStatus()); + assertFalse(dependentRanEarly.get()); + + dep1Release.countDown(); + assertTrue(dep2Done.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + assertTrue(dependentDone.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + assertFalse(dependentRanEarly.get(), "Dependent must not run while dep1 is still held"); + + ScheduledTask done = waitForStatus(node1, dependent.getItemId(), ScheduledTask.TaskStatus.COMPLETED, TEST_TIMEOUT_MS); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, done.getStatus()); + } + + @Test + public void testCircularDependencyIsRejected() { + SchedulerServiceImpl node = createNode("cycle-node", true, 10000); + + // Build a↔b entirely before either is scheduled so both edges are present in the + // validation map (avoids relying on post-hoc mutation of an already-completed task). + ScheduledTask a = node.createTask("cycle-type", null, 0, 0, TimeUnit.MILLISECONDS, + true, true, true, true); + ScheduledTask b = node.createTask("cycle-type", null, 0, 0, TimeUnit.MILLISECONDS, + true, true, true, true); + a.setDependsOn(new java.util.HashSet<>(Collections.singleton(b.getItemId()))); + b.setDependsOn(new java.util.HashSet<>(Collections.singleton(a.getItemId()))); + assertTrue(node.saveTask(a)); + assertTrue(node.saveTask(b)); + + ScheduledTask aReloaded = node.getTask(a.getItemId()); + assertNotNull(aReloaded.getDependsOn()); + assertTrue(aReloaded.getDependsOn().contains(b.getItemId()), + "dependsOn must survive persistence round-trip for cycle detection"); + + assertThrows(IllegalArgumentException.class, () -> node.scheduleTask(b), + "Circular dependsOn must be rejected"); + } + + @Test + public void testDualNodeGetTaskReturnsDistinctInstances() { + SchedulerServiceImpl node1 = createNode("iso-node1", true, 10000); + SchedulerServiceImpl node2 = createNode("iso-node2", true, 10000); + seedActiveNodes("iso-node1", "iso-node2"); + + ScheduledTask created = node1.newTask("dual-view-iso") + .asOneShot() + .schedule(); + + assertDistinctTaskViews(node1, node2, created.getItemId()); + + ScheduledTask from1 = node1.getTask(created.getItemId()); + ScheduledTask again1 = node1.getTask(created.getItemId()); + assertNotSame(from1, again1, "Repeated getTask on same node must also deep-copy"); + } + + @Test + public void testStaleLocalViewUnchangedAfterPeerSave() { + SchedulerServiceImpl node1 = createNode("stale-node1", true, 10000); + SchedulerServiceImpl node2 = createNode("stale-node2", true, 10000); + seedActiveNodes("stale-node1", "stale-node2"); + + ScheduledTask created = node1.newTask("stale-view") + .asOneShot() + .schedule(); + + ScheduledTask stale = node1.getTask(created.getItemId()); + ScheduledTask.TaskStatus before = stale.getStatus(); + + ScheduledTask peer = node2.getTask(created.getItemId()); + assertNotSame(stale, peer); + peer.setCurrentStep("peer-updated"); + peer.setStatus(ScheduledTask.TaskStatus.WAITING); + assertTrue(node2.saveTask(peer)); + + assertEquals(before, stale.getStatus(), + "Stale local reference must not auto-update when peer persists a different copy"); + assertNull(stale.getCurrentStep(), + "Stale local reference must not see peer field writes"); + + ScheduledTask fresh = node1.getTask(created.getItemId()); + assertNotSame(stale, fresh); + assertEquals(ScheduledTask.TaskStatus.WAITING, fresh.getStatus()); + assertEquals("peer-updated", fresh.getCurrentStep()); + } + + @Test + public void testFindTasksByStatusReturnsIsolatedCopiesPerNode() { + SchedulerServiceImpl node1 = createNode("find-node1", true, 10000); + SchedulerServiceImpl node2 = createNode("find-node2", true, 10000); + seedActiveNodes("find-node1", "find-node2"); + + ScheduledTask created = node1.newTask("find-iso") + .asOneShot() + .schedule(); + + List list1 = node1.findTasksByStatus(ScheduledTask.TaskStatus.SCHEDULED); + List list2 = node2.findTasksByStatus(ScheduledTask.TaskStatus.SCHEDULED); + ScheduledTask t1 = list1.stream().filter(t -> created.getItemId().equals(t.getItemId())).findFirst().orElse(null); + ScheduledTask t2 = list2.stream().filter(t -> created.getItemId().equals(t.getItemId())).findFirst().orElse(null); + assertNotNull(t1); + assertNotNull(t2); + assertNotSame(t1, t2, "findTasksByStatus must deep-copy per node/query"); + + t1.setStatus(ScheduledTask.TaskStatus.RUNNING); + List list2Again = node2.findTasksByStatus(ScheduledTask.TaskStatus.SCHEDULED); + ScheduledTask t2Again = list2Again.stream() + .filter(t -> created.getItemId().equals(t.getItemId())).findFirst().orElse(null); + assertNotNull(t2Again); + assertEquals(ScheduledTask.TaskStatus.SCHEDULED, t2Again.getStatus(), + "Mutating one node's query result must not alter the store or the peer's view"); + } + + @Test + public void testNestedMapsIsolatedAcrossNodesDuringCheckpoint() throws Exception { + SchedulerServiceImpl node1 = createNode("nest-node1", true, TEST_TIMEOUT_MS); + SchedulerServiceImpl node2 = createNode("nest-node2", true, TEST_TIMEOUT_MS); + seedActiveNodes("nest-node1", "nest-node2"); + + CountDownLatch checkpointed = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "nest-iso"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + Map cp = new HashMap<>(); + cp.put("step", 7); + cp.put("payload", "alpha"); + callback.checkpoint(cp); + checkpointed.countDown(); + assertTrue(release.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + callback.complete(); + } + }; + registerOnAll(executor, node1, node2); + + ScheduledTask task = node1.newTask("nest-iso") + .disallowParallelExecution() + .asOneShot() + .schedule(); + + assertTrue(checkpointed.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + assertDistinctTaskViews(node1, node2, task.getItemId()); + + ScheduledTask view1 = node1.getTask(task.getItemId()); + ScheduledTask view2 = node2.getTask(task.getItemId()); + assertNotNull(view1.getCheckpointData()); + assertNotSame(view1.getCheckpointData(), view2.getCheckpointData()); + view1.getCheckpointData().put("payload", "mutated-locally"); + ScheduledTask view2Fresh = node2.getTask(task.getItemId()); + assertEquals("alpha", view2Fresh.getCheckpointData().get("payload"), + "Peer must not observe nested-map mutations on another node's copy"); + + release.countDown(); + ScheduledTask done = waitForStatus(node1, task.getItemId(), ScheduledTask.TaskStatus.COMPLETED, TEST_TIMEOUT_MS); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, done.getStatus()); + } + + @Test + public void testDualSurvivorRecoverDoesNotDoubleResume() throws Exception { + long lockTimeout = SHORT_LOCK_TIMEOUT_MS; + SchedulerServiceImpl victim = createNode("dual-rec-victim", true, lockTimeout); + SchedulerServiceImpl survivor1 = createNode("dual-rec-s1", true, lockTimeout); + SchedulerServiceImpl survivor2 = createNode("dual-rec-s2", true, lockTimeout); + seedActiveNodes("dual-rec-victim", "dual-rec-s1", "dual-rec-s2"); + + CountDownLatch started = new CountDownLatch(1); + CountDownLatch hold = new CountDownLatch(1); + AtomicInteger executeCount = new AtomicInteger(); + AtomicInteger resumeCount = new AtomicInteger(); + AtomicReference owner = new AtomicReference<>(); + + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "dual-recover"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + executeCount.incrementAndGet(); + owner.set(task.getExecutingNodeId()); + callback.checkpoint(Collections.singletonMap("step", 1)); + started.countDown(); + hold.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS); + callback.complete(); + } + @Override public boolean canResume(ScheduledTask task) { + return task.getCheckpointData() != null; + } + @Override public void resume(ScheduledTask task, TaskStatusCallback callback) { + resumeCount.incrementAndGet(); + callback.complete(); + } + }; + registerOnAll(executor, victim, survivor1, survivor2); + + ScheduledTask task = victim.newTask("dual-recover") + .disallowParallelExecution() + .asOneShot() + .schedule(); + + assertTrue(started.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + assertNotNull(owner.get()); + + SchedulerServiceImpl crashed = + "dual-rec-victim".equals(owner.get()) ? victim + : ("dual-rec-s1".equals(owner.get()) ? survivor1 : survivor2); + List survivors = new ArrayList<>(); + for (SchedulerServiceImpl n : new SchedulerServiceImpl[]{victim, survivor1, survivor2}) { + if (n != crashed) { + survivors.add(n); + } + } + crashed.simulateCrash(); + + // Ensure lock is expired so both survivors' recover passes see it as crashed work. + expireLockInStore(task.getItemId()); + + CountDownLatch go = new CountDownLatch(1); + CountDownLatch done = new CountDownLatch(survivors.size()); + for (SchedulerServiceImpl s : survivors) { + Thread t = new Thread(() -> { + try { + go.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS); + s.recoverCrashedTasks(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } finally { + done.countDown(); + } + }, "recover-" + s.getNodeId()); + t.setDaemon(true); + t.start(); + } + go.countDown(); + assertTrue(done.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + + // Drive a few more recover passes in case the first concurrent pass lost the OCC race + long deadline = System.currentTimeMillis() + TEST_TIMEOUT_MS; + while (resumeCount.get() == 0 && System.currentTimeMillis() < deadline) { + for (SchedulerServiceImpl s : survivors) { + s.recoverCrashedTasks(); + } + Thread.sleep(100); + } + + assertEquals(1, executeCount.get(), "Only the original node should have called execute()"); + assertEquals(1, resumeCount.get(), "Exactly one survivor may resume after dual recover"); + ScheduledTask completed = waitForStatus(survivors.get(0), task.getItemId(), + ScheduledTask.TaskStatus.COMPLETED, TEST_TIMEOUT_MS); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, completed.getStatus()); + assertDistinctTaskViews(survivors.get(0), survivors.get(1), task.getItemId()); + hold.countDown(); + } + + @Test + public void testRunOnAllNodesExecutesOnEveryNodeWithIsolatedViews() throws Exception { + SchedulerServiceImpl node1 = createNode("all-node1", true, 10000); + SchedulerServiceImpl node2 = createNode("all-node2", true, 10000); + SchedulerServiceImpl node3 = createNode("all-node3", false, 10000); + seedActiveNodes("all-node1", "all-node2", "all-node3"); + + CountDownLatch allRan = new CountDownLatch(1); + Set nodesSeen = ConcurrentHashMap.newKeySet(); + Set identityHashes = ConcurrentHashMap.newKeySet(); + + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "run-all-iso"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + nodesSeen.add(task.getExecutingNodeId()); + identityHashes.add(System.identityHashCode(task)); + if (nodesSeen.size() >= 3) { + allRan.countDown(); + } + callback.complete(); + } + }; + registerOnAll(executor, node1, node2, node3); + + ScheduledTask task = node1.newTask("run-all-iso") + .runOnAllNodes() + .withPeriod(200, TimeUnit.MILLISECONDS) + .schedule(); + + assertTrue(allRan.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "runOnAllNodes must execute on every node including non-executors"); + assertTrue(nodesSeen.contains("all-node1")); + assertTrue(nodesSeen.contains("all-node2")); + assertTrue(nodesSeen.contains("all-node3")); + assertTrue(identityHashes.size() >= 3, + "Each node execution must use a distinct deep-copied task instance, got " + + identityHashes.size()); + + // Parallel-execution tasks give each node an independent, uncoordinated status write + // (prepareForExecution()'s RUNNING transition is a blind, non-CAS save for parallel + // tasks by design). With a 200ms period, reading status right after the latch fires can + // race an in-flight node's RUNNING->SCHEDULED transition. Cancel on every node first - + // idempotent and terminal - so all nodes converge on the same shared-store status before + // the isolation assertions below, which only care about per-node deep-copy isolation. + node1.cancelTask(task.getItemId()); + node2.cancelTask(task.getItemId()); + node3.cancelTask(task.getItemId()); + + assertDistinctTaskViews(node1, node2, task.getItemId()); + assertDistinctTaskViews(node2, node3, task.getItemId()); + } + + @Test + public void testLockStealAfterExpiryUsesOccAndSingleWinner() throws Exception { + long lockTimeout = SHORT_LOCK_TIMEOUT_MS; + SchedulerServiceImpl holder = createNode("steal-holder", true, lockTimeout); + SchedulerServiceImpl peer = createNode("steal-peer", true, lockTimeout); + seedActiveNodes("steal-holder", "steal-peer"); + + CountDownLatch started = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + AtomicInteger executions = new AtomicInteger(); + Set executors = ConcurrentHashMap.newKeySet(); + + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "lock-steal"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + executions.incrementAndGet(); + executors.add(task.getExecutingNodeId()); + started.countDown(); + assertTrue(release.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + callback.complete(); + } + }; + registerOnAll(executor, holder, peer); + + ScheduledTask task = holder.newTask("lock-steal") + .disallowParallelExecution() + .asOneShot() + .schedule(); + + assertTrue(started.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + ScheduledTask lockedView = peer.getTask(task.getItemId()); + assertNotNull(lockedView.getLockOwner()); + Object staleSeq = lockedView.getSystemMetadata("seq_no"); + assertNotNull(staleSeq); + + // Expire lock while holder is still in execute(); peer recovery/checker may steal. + expireLockInStore(task.getItemId()); + Thread.sleep(lockTimeout + 200); + peer.recoverCrashedTasks(); + + // Stale CAS from the pre-steal peer view must lose once the store moved forward. + ScheduledTask staleWrite = peer.getTask(task.getItemId()); + // Re-load a snapshot taken conceptually before steal: reuse staleSeq on a fresh deep copy + ScheduledTask casAttempt = persistenceService.load(task.getItemId(), ScheduledTask.class); + Object currentSeq = casAttempt.getSystemMetadata("seq_no"); + // Force a stale seq from the earlier view + casAttempt.setSystemMetadata("seq_no", staleSeq); + casAttempt.setSystemMetadata("_seq_no", staleSeq); + casAttempt.setLockOwner("steal-peer-stale"); + if (!staleSeq.equals(currentSeq)) { + assertFalse(persistenceService.save(casAttempt, false, false), + "OCC must reject stale seq_no after peer advanced the document"); + } + + release.countDown(); + // Regardless of reclaim/steal outcome, only one logical completion and distinct views. + long deadline = System.currentTimeMillis() + TEST_TIMEOUT_MS; + ScheduledTask finalTask = null; + while (System.currentTimeMillis() < deadline) { + finalTask = holder.getTask(task.getItemId()); + ScheduledTask peerView = peer.getTask(task.getItemId()); + assertNotSame(finalTask, peerView); + if (finalTask != null && ( + ScheduledTask.TaskStatus.COMPLETED.equals(finalTask.getStatus()) + || ScheduledTask.TaskStatus.CRASHED.equals(finalTask.getStatus()) + || ScheduledTask.TaskStatus.SCHEDULED.equals(finalTask.getStatus()) + || ScheduledTask.TaskStatus.FAILED.equals(finalTask.getStatus()))) { + // Keep waiting for COMPLETED when reclaim path wins + if (ScheduledTask.TaskStatus.COMPLETED.equals(finalTask.getStatus())) { + break; + } + } + Thread.sleep(50); + } + finalTask = waitForStatus(holder, task.getItemId(), ScheduledTask.TaskStatus.COMPLETED, TEST_TIMEOUT_MS); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, finalTask.getStatus()); + assertEquals(1, executors.size(), "Exactly one node should own execution under exclusive lock"); + assertDistinctTaskViews(holder, peer, task.getItemId()); + } + + @Test + public void testDualCheckersExclusiveTaskSingleExecutionWithDistinctViews() throws Exception { + SchedulerServiceImpl node1 = createNode("chk-node1", true, 10000); + SchedulerServiceImpl node2 = createNode("chk-node2", true, 10000); + seedActiveNodes("chk-node1", "chk-node2"); + + CountDownLatch started = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + AtomicInteger executions = new AtomicInteger(); + Set identityHashes = ConcurrentHashMap.newKeySet(); + + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "dual-checker"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + executions.incrementAndGet(); + identityHashes.add(System.identityHashCode(task)); + started.countDown(); + // While held, both nodes' checkers keep polling — sample identity isolation. + assertTrue(release.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + callback.complete(); + } + }; + registerOnAll(executor, node1, node2); + + ScheduledTask task = node1.newTask("dual-checker") + .disallowParallelExecution() + .asOneShot() + .schedule(); + + assertTrue(started.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + // Contended window: both nodes repeatedly load distinct copies + for (int i = 0; i < 20; i++) { + assertDistinctTaskViews(node1, node2, task.getItemId()); + Thread.sleep(25); + } + release.countDown(); + + ScheduledTask done = waitForStatus(node1, task.getItemId(), ScheduledTask.TaskStatus.COMPLETED, TEST_TIMEOUT_MS); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, done.getStatus()); + assertEquals(1, executions.get(), "Dual checkers must not double-execute exclusive task"); + assertEquals(1, identityHashes.size(), "Winning execution uses one in-process instance"); + assertDistinctTaskViews(node1, node2, task.getItemId()); + } + + @Test + public void testConcurrentShutdownRacingPeerRecoveryDoesNotDoubleDispatch() throws Exception { + // Gap flagged in the UNOMI-967 PR review: existing crash-recovery tests trigger + // recovery *after* a crash has already happened (simulateCrash() then recover()). + // This drives victim.preDestroy() (which marks RUNNING work CRASHED and clears its + // own lock) concurrently with peer.recoverCrashedTasks() racing for the same + // still-locked document, exercising the shutdown-vs-recovery interleaving directly. + long lockTimeout = SHORT_LOCK_TIMEOUT_MS; + SchedulerServiceImpl victim = createNode("shutdown-race-victim", true, lockTimeout); + SchedulerServiceImpl peer = createNode("shutdown-race-peer", true, lockTimeout); + seedActiveNodes("shutdown-race-victim", "shutdown-race-peer"); + + CountDownLatch started = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + AtomicInteger executeCount = new AtomicInteger(); + AtomicInteger resumeCount = new AtomicInteger(); + + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "shutdown-race"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + executeCount.incrementAndGet(); + callback.checkpoint(Collections.singletonMap("step", 1)); + started.countDown(); + release.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS); + callback.complete(); + } + @Override public boolean canResume(ScheduledTask task) { + return task.getCheckpointData() != null; + } + @Override public void resume(ScheduledTask task, TaskStatusCallback callback) { + resumeCount.incrementAndGet(); + callback.complete(); + } + }; + registerOnAll(executor, victim, peer); + + ScheduledTask task = victim.newTask("shutdown-race") + .disallowParallelExecution() + .asOneShot() + .schedule(); + + assertTrue(started.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + // Age the lock past lockTimeout so peer's recovery pass considers it eligible, + // at the same moment we drive the victim's own shutdown-triggered crash-mark. + expireLockInStore(task.getItemId()); + + AtomicReference victimError = new AtomicReference<>(); + AtomicReference peerError = new AtomicReference<>(); + CountDownLatch go = new CountDownLatch(1); + CountDownLatch done = new CountDownLatch(2); + Thread shutdownThread = new Thread(() -> { + try { + go.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS); + victim.preDestroy(); + } catch (Throwable t) { + victimError.set(t); + } finally { + done.countDown(); + } + }, "shutdown-race-victim-thread"); + Thread recoveryThread = new Thread(() -> { + try { + go.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS); + peer.recoverCrashedTasks(); + } catch (Throwable t) { + peerError.set(t); + } finally { + done.countDown(); + } + }, "shutdown-race-peer-thread"); + shutdownThread.start(); + recoveryThread.start(); + go.countDown(); + assertTrue(done.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + release.countDown(); + + assertNull(victimError.get(), "victim.preDestroy() must not throw when racing peer recovery"); + assertNull(peerError.get(), "peer.recoverCrashedTasks() must not throw when racing victim shutdown"); + + // Drive a few more recover passes in case the first concurrent pass lost the OCC race + // (victim's own shutdown save and peer's recovery save both target the same document). + long deadline = System.currentTimeMillis() + TEST_TIMEOUT_MS; + while (resumeCount.get() == 0 && System.currentTimeMillis() < deadline) { + peer.recoverCrashedTasks(); + Thread.sleep(100); + } + + assertEquals(1, executeCount.get(), + "Concurrent shutdown + peer recovery must not cause a second independent execute()"); + assertTrue(resumeCount.get() <= 1, + "Concurrent shutdown + peer recovery must not double-resume the same crashed work"); + } +} diff --git a/services/src/test/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceImplTest.java b/services/src/test/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceImplTest.java index 74bb68f12e..d3e91d56ce 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceImplTest.java @@ -387,34 +387,12 @@ public void execute(ScheduledTask task, TaskStatusCallback callback) { @Test @Tag("DependencyTests") public void testTaskDependencies() throws Exception { - // Test task dependencies and waiting behavior - CountDownLatch dep1Latch = new CountDownLatch(1); + CountDownLatch dep1Start = new CountDownLatch(1); + CountDownLatch dep1Release = new CountDownLatch(1); CountDownLatch dep2Latch = new CountDownLatch(1); - - // Create first dependency - ScheduledTask dep1 = schedulerService.newTask("dep-test") - .withSimpleExecutor(() -> dep1Latch.countDown()) - .schedule(); - - // Create second dependency - ScheduledTask dep2 = schedulerService.newTask("dep-test") - .withSimpleExecutor(() -> dep2Latch.countDown()) - .schedule(); - - // Create dependent task + CountDownLatch dependentLatch = new CountDownLatch(1); AtomicBoolean dependentExecuted = new AtomicBoolean(false); - ScheduledTask dependentTask = schedulerService.createTask( - "dep-test", - null, - 0, - 0, - TimeUnit.MILLISECONDS, - true, - true, - true, - true - ); - dependentTask.setDependsOn(new HashSet<>(Arrays.asList(dep1.getItemId(), dep2.getItemId()))); + AtomicReference dependentTaskId = new AtomicReference<>(); TaskExecutor executor = new TaskExecutor() { @Override @@ -423,26 +401,51 @@ public String getTaskType() { } @Override - public void execute(ScheduledTask task, TaskStatusCallback callback) { - if (task == dependentTask) { + public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + if (task.getItemId().equals(dependentTaskId.get())) { dependentExecuted.set(true); + dependentLatch.countDown(); + callback.complete(); + return; + } + if ("dep1".equals(task.getParameters().get("name"))) { + dep1Start.countDown(); + assertTrue(dep1Release.await(TEST_TIMEOUT, TEST_TIME_UNIT), "dep1 should be released"); + callback.complete(); + return; } + dep2Latch.countDown(); callback.complete(); } }; - schedulerService.registerTaskExecutor(executor); - schedulerService.scheduleTask(dependentTask); - // Wait for dependencies to complete - assertTrue( - dep1Latch.await(TEST_TIMEOUT, TEST_TIME_UNIT) && - dep2Latch.await(TEST_TIMEOUT, TEST_TIME_UNIT), - "Dependencies should complete"); + ScheduledTask dep1 = schedulerService.newTask("dep-test") + .withParameters(Collections.singletonMap("name", "dep1")) + .schedule(); + ScheduledTask dep2 = schedulerService.newTask("dep-test") + .withParameters(Collections.singletonMap("name", "dep2")) + .schedule(); - // Verify dependent task execution - Thread.sleep(100); // Give time for dependent task to execute - assertTrue(dependentExecuted.get(), "Dependent task should execute after dependencies"); + // Hold dep1 so the dependent cannot legally run yet + assertTrue(dep1Start.await(TEST_TIMEOUT, TEST_TIME_UNIT), "dep1 should start"); + + ScheduledTask dependentTask = schedulerService.newTask("dep-test") + .withParameters(Collections.singletonMap("name", "dependent")) + .withDependencies(dep1.getItemId(), dep2.getItemId()) + .schedule(); + dependentTaskId.set(dependentTask.getItemId()); + + ScheduledTask waiting = schedulerService.getTask(dependentTask.getItemId()); + assertEquals(ScheduledTask.TaskStatus.WAITING, waiting.getStatus(), + "Dependent must wait while dependencies are incomplete"); + assertFalse(dependentExecuted.get(), "Dependent must not run before dependencies complete"); + + dep1Release.countDown(); + assertTrue(dep2Latch.await(TEST_TIMEOUT, TEST_TIME_UNIT), "dep2 should complete"); + assertTrue(dependentLatch.await(TEST_TIMEOUT, TEST_TIME_UNIT), + "Dependent should run only after both dependencies complete"); + assertTrue(dependentExecuted.get(), "Dependent task should have executed"); } // Clustering support tests @@ -466,50 +469,74 @@ public void testClusteringSupport() throws Exception { // Also refresh all indexes to ensure tasks are available for querying (handles refresh delay) persistenceService.refresh(); - CountDownLatch executionLatch = new CountDownLatch(2); - Set executingNodes = ConcurrentHashMap.newKeySet(); + CountDownLatch exclusiveLatch = new CountDownLatch(1); + CountDownLatch allNodesLatch = new CountDownLatch(3); // one execution observed per node + Set exclusiveNodes = ConcurrentHashMap.newKeySet(); + Set allNodesNodes = ConcurrentHashMap.newKeySet(); - TaskExecutor clusterTestExecutor = new TaskExecutor() { + TaskExecutor exclusiveExecutor = new TaskExecutor() { @Override public String getTaskType() { - return "cluster-test"; + return "cluster-exclusive-test"; } @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { - executingNodes.add(task.getExecutingNodeId()); - executionLatch.countDown(); + exclusiveNodes.add(task.getExecutingNodeId()); + exclusiveLatch.countDown(); callback.complete(); } }; - // Register executor on all nodes - node1.registerTaskExecutor(clusterTestExecutor); - node2.registerTaskExecutor(clusterTestExecutor); - nonExecutorNode.registerTaskExecutor(clusterTestExecutor); + TaskExecutor allNodesExecutor = new TaskExecutor() { + @Override + public String getTaskType() { + return "cluster-all-nodes-test"; + } - // Create tasks with different distribution requirements - ScheduledTask normalTask = node1.newTask("cluster-test") + @Override + public void execute(ScheduledTask task, TaskStatusCallback callback) { + if (allNodesNodes.add(task.getExecutingNodeId())) { + allNodesLatch.countDown(); + } + callback.complete(); + } + }; + + node1.registerTaskExecutor(exclusiveExecutor); + node2.registerTaskExecutor(exclusiveExecutor); + nonExecutorNode.registerTaskExecutor(exclusiveExecutor); + node1.registerTaskExecutor(allNodesExecutor); + node2.registerTaskExecutor(allNodesExecutor); + nonExecutorNode.registerTaskExecutor(allNodesExecutor); + + // Non-runOnAllNodes: only executor nodes may run + node1.newTask("cluster-exclusive-test") .withPeriod(100, TimeUnit.MILLISECONDS) .schedule(); - ScheduledTask allNodesTask = node1.newTask("cluster-test") + // runOnAllNodes: every node including non-executors must poll and run + node1.newTask("cluster-all-nodes-test") .runOnAllNodes() .withPeriod(100, TimeUnit.MILLISECONDS) .schedule(); - // Wait for executions assertTrue( - executionLatch.await(TEST_TIMEOUT, TEST_TIME_UNIT), - "Tasks should execute on cluster nodes"); - - // Verify distribution + exclusiveLatch.await(TEST_TIMEOUT, TEST_TIME_UNIT), + "Exclusive cluster task should execute on an executor node"); assertTrue( - executingNodes.contains("node1") || executingNodes.contains("node2"), - "Task should execute on executor nodes"); + exclusiveNodes.contains("node1") || exclusiveNodes.contains("node2"), + "Exclusive task should execute on an executor node"); assertFalse( - executingNodes.contains("node3"), - "Task should not execute on non-executor node"); + exclusiveNodes.contains("node3"), + "Exclusive task must not execute on a non-executor node"); + + assertTrue( + allNodesLatch.await(TEST_TIMEOUT, TEST_TIME_UNIT), + "runOnAllNodes task should execute on every node including non-executors"); + assertTrue(allNodesNodes.contains("node1"), "runOnAllNodes should run on node1"); + assertTrue(allNodesNodes.contains("node2"), "runOnAllNodes should run on node2"); + assertTrue(allNodesNodes.contains("node3"), "runOnAllNodes should run on non-executor node3"); TaskExecutor clusterLockTestExecutor = new TaskExecutor() { @Override @@ -602,10 +629,16 @@ public boolean canResume(ScheduledTask task) { .disallowParallelExecution() .schedule(); - // Wait for the task to be scheduled, then simulate a node crash (RUNNING + expired lock) + // The task executes immediately on schedule() and its first natural attempt always + // throws (no "crashTime" yet), triggering handleTaskError()'s own async + // failureCount-increment-and-reschedule save. Wait for that save to land (failureCount + // >= 1) before overwriting the task below - otherwise this test's blind saveTask() can + // race that background write and lose, silently reverting the simulated-crash setup back + // to a plain retry-delayed SCHEDULED state (observed as a deterministic failure once + // lock acquisition got fast enough to no longer incidentally happen-after that save). ScheduledTask persistedTask = TestHelper.retryUntil( () -> schedulerService.getTask(task.getItemId()), - Objects::nonNull + t -> t != null && t.getFailureCount() >= 1 ); persistedTask.setStatus(ScheduledTask.TaskStatus.RUNNING); persistedTask.setLockOwner("dead-node"); @@ -922,6 +955,129 @@ public void execute(ScheduledTask task, TaskStatusCallback callback) { "Should have executed expected number of times"); } + /** + * Regression test for a lost one-shot retry dispatch. + * + * The retry delay countdown starts inside handleTaskError(), i.e. while the failing + * execution's wrapper is still running and still holds the task's dispatch claim in + * TaskExecutionManager. If the wrapper takes longer than the retry delay to finish + * (slow executor cleanup after callback.fail(), GC pause, loaded CI runner), the retry + * fires while the claim is still held and is silently dropped as a duplicate dispatch. + * Since one-shot retries used to be invisible to the periodic task checker, the task + * was then stranded in SCHEDULED state forever. The checker must act as a safety net + * and re-dispatch the task once its nextScheduledExecution time is due. + */ + @Test + @Tag("RetryTests") + public void testOneShotRetryRecoveredWhenRetryDispatchIsDropped() throws Exception { + final long retryDelay = 300; + CountDownLatch secondExecutionLatch = new CountDownLatch(1); + AtomicInteger executionCount = new AtomicInteger(0); + + TaskExecutor executor = new TaskExecutor() { + @Override + public String getTaskType() { + return "retry-claim-race-test"; + } + + @Override + public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + int count = executionCount.incrementAndGet(); + if (count == 1) { + callback.fail("Simulated failure #1"); + // Keep this execution's wrapper alive past the retry delay: the retry + // dispatch fires while the dispatch claim is still held and is dropped + // as a duplicate. Only the task checker safety net can then re-dispatch + // the task. + Thread.sleep(retryDelay + 700); + } else { + secondExecutionLatch.countDown(); + callback.complete(); + } + } + }; + + ScheduledTask task = schedulerService.newTask("retry-claim-race-test") + .withMaxRetries(1) + .withRetryDelay(retryDelay, TimeUnit.MILLISECONDS) + .withExecutor(executor) + .asOneShot() + .schedule(); + + assertTrue( + secondExecutionLatch.await(TEST_TIMEOUT, TEST_TIME_UNIT), + "Retry should still execute when the direct retry dispatch was dropped"); + + ScheduledTask finalTask = waitForTaskStatus(task.getItemId(), ScheduledTask.TaskStatus.COMPLETED, TEST_WAIT_TIMEOUT, 50); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, finalTask.getStatus(), "Task should complete after retry"); + assertEquals(2, executionCount.get(), "Task should have executed exactly twice"); + } + + /** + * Regression test for crash recovery eating a live execution. + * + * If an executing thread stalls longer than the lock timeout while the task is RUNNING, + * the task checker's crash recovery marks the task CRASHED. When the executor then + * reports failure, handleTaskError() used to silently ignore the callback (status no + * longer RUNNING), so no retry was scheduled - and recovery refuses to restart one-shot + * tasks that already executed, stranding the task in CRASHED state forever. The + * execution manager must recognize that the execution it owns is still alive, reclaim + * the task and process the failure (and its retry) normally. + */ + @Test + @Tag("RetryTests") + public void testOneShotRetryAfterRecoveryMarksLiveExecutionCrashed() throws Exception { + // setUp() only sets the lock timeout on the scheduler service; the lock manager + // created by TestHelper keeps its 10s default. Shorten it here so a stalled + // execution's lock actually expires within this test's stall window. + schedulerService.getLockManager().setLockTimeout(TEST_LOCK_TIMEOUT); + + CountDownLatch completionLatch = new CountDownLatch(1); + AtomicInteger executionCount = new AtomicInteger(0); + + TaskExecutor executor = new TaskExecutor() { + @Override + public String getTaskType() { + return "retry-crash-reclaim-test"; + } + + @Override + public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + int count = executionCount.incrementAndGet(); + if (count == 1) { + callback.fail("Simulated failure #1"); + } else if (count == 2) { + // Stall while RUNNING for longer than the lock timeout so that crash + // recovery marks this still-live execution as CRASHED before the + // failure is reported. + Thread.sleep(TEST_LOCK_TIMEOUT + 3000); + callback.fail("Simulated failure #2"); + } else { + completionLatch.countDown(); + callback.complete(); + } + } + }; + + ScheduledTask task = schedulerService.newTask("retry-crash-reclaim-test") + .withMaxRetries(TEST_MAX_RETRIES) + .withRetryDelay(TEST_RETRY_DELAY, TimeUnit.MILLISECONDS) + .withExecutor(executor) + .asOneShot() + .schedule(); + + assertTrue( + completionLatch.await(TEST_TIMEOUT, TEST_TIME_UNIT), + "Task should retry and complete even after recovery marked a live execution as crashed"); + + ScheduledTask finalTask = waitForTaskStatus(task.getItemId(), ScheduledTask.TaskStatus.COMPLETED, TEST_WAIT_TIMEOUT, 50); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, finalTask.getStatus(), "Task should complete after reclaimed failure"); + // At least: fail #1, stalled #2 (reclaimed), success. The checker may also dispatch a + // CRASHED recovery attempt in parallel with reclaim, so allow >3. + assertTrue(executionCount.get() >= 3, + "Task should have executed at least three times, was " + executionCount.get()); + } + @Test @Tag("RetryTests") public void testPeriodicTaskRetryScenarios() throws Exception { @@ -1762,6 +1918,32 @@ public void execute(ScheduledTask task, TaskStatusCallback callback) { } } + @Test + public void testNewTaskDefaultsToExclusiveLocking() { + ScheduledTask task = schedulerService.newTask("builder-default-exclusive") + .asOneShot() + .withSimpleExecutor(() -> {}) + .schedule(); + assertFalse(task.isAllowParallelExecution(), + "newTask should default to exclusive (allowParallelExecution=false)"); + + ScheduledTask parallel = schedulerService.newTask("builder-parallel-opt-in") + .allowParallelExecution() + .asOneShot() + .withSimpleExecutor(() -> {}) + .schedule(); + assertTrue(parallel.isAllowParallelExecution()); + + ScheduledTask allNodes = schedulerService.newTask("builder-run-all") + .runOnAllNodes() + .withPeriod(1, TimeUnit.HOURS) + .withSimpleExecutor(() -> {}) + .schedule(); + assertTrue(allNodes.isRunOnAllNodes()); + assertTrue(allNodes.isAllowParallelExecution(), + "runOnAllNodes must auto-enable allowParallelExecution"); + } + @Test public void testNodeAffinity() throws Exception { // Create test nodes with cluster service diff --git a/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskExecutionManagerTest.java b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskExecutionManagerTest.java new file mode 100644 index 0000000000..582ecb9383 --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskExecutionManagerTest.java @@ -0,0 +1,595 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.unomi.services.impl.scheduler; + +import org.apache.unomi.api.tasks.ScheduledTask; +import org.apache.unomi.api.tasks.TaskExecutor; +import org.junit.jupiter.api.AfterEach; +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 org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; + +import java.util.Collections; +import java.util.Date; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +/** + * Unit tests for {@link TaskExecutionManager}. + * Invoked by Surefire: {@code -Dtest=TaskExecutionManagerTest}. No data-file I/O. + * Glob: no prior TaskExecutionManagerTest. User asked for Task*Managers unit tests. + */ +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +public class TaskExecutionManagerTest { + + private static final String NODE = "exec-node"; + + @Mock private SchedulerServiceImpl schedulerService; + @Mock private TaskLockManager lockManager; + @Mock private TaskExecutorRegistry executorRegistry; + + private TaskStateManager stateManager; + private TaskMetricsManager metricsManager; + private TaskHistoryManager historyManager; + private TaskExecutionManager executionManager; + + @BeforeEach + public void setUp() { + stateManager = new TaskStateManager(); + metricsManager = new TaskMetricsManager(); + historyManager = new TaskHistoryManager(); + historyManager.setNodeId(NODE); + historyManager.setMetricsManager(metricsManager); + + executionManager = new TaskExecutionManager(); + executionManager.setNodeId(NODE); + executionManager.setThreadPoolSize(4); + executionManager.setStateManager(stateManager); + executionManager.setLockManager(lockManager); + executionManager.setMetricsManager(metricsManager); + executionManager.setHistoryManager(historyManager); + executionManager.setExecutorRegistry(executorRegistry); + executionManager.setSchedulerService(schedulerService); + executionManager.initialize(); + + when(schedulerService.isShutdownNow()).thenReturn(false); + when(schedulerService.saveTask(any(ScheduledTask.class))).thenReturn(true); + when(schedulerService.saveTask(any(ScheduledTask.class), anyBoolean())).thenReturn(true); + when(schedulerService.saveTaskWithRefresh(any(ScheduledTask.class))).thenReturn(true); + when(lockManager.acquireLock(any(ScheduledTask.class))).thenReturn(true); + when(lockManager.releaseLock(any(ScheduledTask.class))).thenReturn(true); + when(lockManager.isLockExpired(any(ScheduledTask.class))).thenReturn(false); + } + + @AfterEach + public void tearDown() { + executionManager.shutdown(); + } + + @Test + public void testPrepareForExecutionRejectsDisabledAndWrongStatus() { + ScheduledTask disabled = TaskTestFixtures.baseTask("p"); + disabled.setEnabled(false); + assertFalse(executionManager.prepareForExecution(disabled)); + + ScheduledTask completed = TaskTestFixtures.baseTask("p"); + completed.setStatus(ScheduledTask.TaskStatus.COMPLETED); + assertFalse(executionManager.prepareForExecution(completed)); + } + + @Test + public void testPrepareForExecutionRejectsNotDueScheduled() { + ScheduledTask task = TaskTestFixtures.baseTask("p"); + task.setNextScheduledExecution(new Date(System.currentTimeMillis() + 60_000)); + assertFalse(executionManager.prepareForExecution(task)); + verify(lockManager, never()).acquireLock(any()); + } + + @Test + public void testPrepareForExecutionAcquiresLockAndSetsRunning() { + ScheduledTask task = TaskTestFixtures.baseTask("p"); + task.setNextScheduledExecution(new Date(System.currentTimeMillis() - 1000)); + assertTrue(executionManager.prepareForExecution(task)); + assertEquals(ScheduledTask.TaskStatus.RUNNING, task.getStatus()); + verify(lockManager).acquireLock(task); + verify(schedulerService).saveTask(task); + } + + @Test + public void testPrepareForExecutionFailsWhenLockDenied() { + when(lockManager.acquireLock(any())).thenReturn(false); + ScheduledTask task = TaskTestFixtures.baseTask("p"); + assertFalse(executionManager.prepareForExecution(task)); + assertEquals(ScheduledTask.TaskStatus.SCHEDULED, task.getStatus()); + } + + @Test + public void testExecuteTaskDuplicateDispatchIsSkipped() throws Exception { + CountDownLatch started = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + AtomicInteger runs = new AtomicInteger(); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "dup"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + runs.incrementAndGet(); + started.countDown(); + release.await(5, TimeUnit.SECONDS); + callback.complete(); + } + }; + ScheduledTask task = TaskTestFixtures.baseTask("dup"); + executionManager.executeTask(task, executor); + assertTrue(started.await(5, TimeUnit.SECONDS)); + // Second dispatch while claim held + executionManager.executeTask(task, executor); + release.countDown(); + Thread.sleep(200); + assertEquals(1, runs.get()); + } + + @Test + public void testExecuteTaskInvokesResumeForCrashedWhenCanResume() throws Exception { + CountDownLatch resumed = new CountDownLatch(1); + AtomicInteger executeCount = new AtomicInteger(); + AtomicInteger resumeCount = new AtomicInteger(); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "resume"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + executeCount.incrementAndGet(); + callback.complete(); + } + @Override public boolean canResume(ScheduledTask task) { return true; } + @Override public void resume(ScheduledTask task, TaskStatusCallback callback) { + resumeCount.incrementAndGet(); + resumed.countDown(); + callback.complete(); + } + }; + ScheduledTask task = TaskTestFixtures.baseTask("resume"); + task.setStatus(ScheduledTask.TaskStatus.CRASHED); + task.setCheckpointData(Collections.singletonMap("step", 1)); + executionManager.executeTask(task, executor); + assertTrue(resumed.await(5, TimeUnit.SECONDS)); + assertEquals(1, resumeCount.get()); + assertEquals(0, executeCount.get()); + } + + @Test + public void testCancelTaskCancelsFutureAndAllowsEventualRedispatch() throws Exception { + CountDownLatch started = new CountDownLatch(1); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "cancel"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + started.countDown(); + try { + Thread.sleep(10_000); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + callback.fail("cancelled"); + return; + } + callback.complete(); + } + }; + ScheduledTask task = TaskTestFixtures.baseTask("cancel"); + executionManager.executeTask(task, executor); + assertTrue(started.await(5, TimeUnit.SECONDS)); + executionManager.cancelTask(task.getItemId()); + // Wrapper exits after interrupt and releases claim; then redispatch works + CountDownLatch started2 = new CountDownLatch(1); + TaskExecutor executor2 = new TaskExecutor() { + @Override public String getTaskType() { return "cancel"; } + @Override public void execute(ScheduledTask t, TaskStatusCallback callback) { + started2.countDown(); + callback.complete(); + } + }; + long deadline = System.currentTimeMillis() + 5000; + while (System.currentTimeMillis() < deadline && started2.getCount() > 0) { + executionManager.executeTask(task, executor2); + if (started2.await(100, TimeUnit.MILLISECONDS)) { + break; + } + } + assertEquals(0, started2.getCount(), "after cancel+wrapper cleanup, redispatch should succeed"); + } + + @Test + public void testHandleTaskErrorSchedulesRetryWithinBudget() throws Exception { + CountDownLatch failed = new CountDownLatch(1); + CountDownLatch retried = new CountDownLatch(1); + AtomicInteger attempts = new AtomicInteger(); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "retry-ok"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + int n = attempts.incrementAndGet(); + if (n == 1) { + callback.fail("transient"); + failed.countDown(); + } else { + callback.complete(); + retried.countDown(); + } + } + }; + when(executorRegistry.getExecutor("retry-ok")).thenReturn(executor); + ScheduledTask task = TaskTestFixtures.baseTask("retry-ok"); + task.setMaxRetries(3); + task.setRetryDelay(50); + task.setFailureCount(0); + + executionManager.executeTask(task, executor); + assertTrue(failed.await(5, TimeUnit.SECONDS)); + assertTrue(retried.await(5, TimeUnit.SECONDS)); + assertEquals(2, attempts.get()); + long deadline = System.currentTimeMillis() + 2000; + while (task.getStatus() != ScheduledTask.TaskStatus.COMPLETED + && System.currentTimeMillis() < deadline) { + Thread.sleep(20); + } + assertEquals(ScheduledTask.TaskStatus.COMPLETED, task.getStatus()); + verify(schedulerService, atLeast(1)).saveTaskWithRefresh(task); + } + + @Test + public void testHandleTaskErrorOneShotExhaustsRetriesStaysFailed() throws Exception { + CountDownLatch done = new CountDownLatch(1); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "exhaust"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + callback.fail("permanent"); + done.countDown(); + } + }; + ScheduledTask task = TaskTestFixtures.baseTask("exhaust"); + task.setMaxRetries(0); + task.setFailureCount(0); + task.setRetryDelay(10); + + executionManager.executeTask(task, executor); + assertTrue(done.await(5, TimeUnit.SECONDS)); + Thread.sleep(100); + assertEquals(ScheduledTask.TaskStatus.FAILED, task.getStatus()); + assertEquals(1, task.getFailureCount()); + assertTrue(task.isEnabled()); + verify(schedulerService, atLeastOnce()).saveTaskWithRefresh(task); + verify(executorRegistry, never()).getExecutor(anyString()); + } + + @Test + public void testHandleTaskErrorPeriodicResetsFailureCountAfterExhaustion() throws Exception { + CountDownLatch done = new CountDownLatch(1); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "period-fail"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + callback.fail("boom"); + done.countDown(); + } + }; + ScheduledTask task = TaskTestFixtures.periodicTask("period-fail", 60_000); + task.setMaxRetries(0); + task.setFailureCount(0); + task.setRetryDelay(10); + + executionManager.executeTask(task, executor); + assertTrue(done.await(5, TimeUnit.SECONDS)); + Thread.sleep(100); + assertEquals(0, task.getFailureCount()); + assertEquals(ScheduledTask.TaskStatus.SCHEDULED, task.getStatus()); + assertNotNull(task.getNextScheduledExecution()); + verify(schedulerService, atLeastOnce()).saveTaskWithRefresh(task); + } + + @Test + public void testHandleTaskErrorSkipsRetryScheduleDuringShutdown() throws Exception { + CountDownLatch inFlight = new CountDownLatch(1); + CountDownLatch allowFail = new CountDownLatch(1); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "shut-retry"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) throws Exception { + inFlight.countDown(); + allowFail.await(5, TimeUnit.SECONDS); + callback.fail("while-shutting-down"); + } + }; + ScheduledTask task = TaskTestFixtures.baseTask("shut-retry"); + task.setMaxRetries(3); + task.setRetryDelay(50); + when(executorRegistry.getExecutor("shut-retry")).thenReturn(executor); + + executionManager.executeTask(task, executor); + assertTrue(inFlight.await(5, TimeUnit.SECONDS)); + executionManager.shutdown(); + allowFail.countDown(); + Thread.sleep(200); + assertEquals(ScheduledTask.TaskStatus.SCHEDULED, task.getStatus()); + assertEquals(1, task.getFailureCount()); + // No second attempt — retry schedule skipped after scheduler shutdown + verify(executorRegistry, never()).getExecutor("shut-retry"); + } + + @Test + public void testHandleTaskCompletionOneShotDisablesAndClearsNext() throws Exception { + CountDownLatch done = new CountDownLatch(1); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "oneshot-ok"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + callback.complete(); + done.countDown(); + } + }; + ScheduledTask task = TaskTestFixtures.baseTask("oneshot-ok"); + task.setNextScheduledExecution(new Date()); + + executionManager.executeTask(task, executor); + assertTrue(done.await(5, TimeUnit.SECONDS)); + Thread.sleep(100); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, task.getStatus()); + assertFalse(task.isEnabled()); + assertNull(task.getNextScheduledExecution()); + verify(schedulerService, atLeastOnce()).saveTaskWithRefresh(task); + } + + @Test + public void testHandleTaskCompletionPeriodicReschedules() throws Exception { + CountDownLatch done = new CountDownLatch(1); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "period-ok"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + callback.complete(); + done.countDown(); + } + }; + ScheduledTask task = TaskTestFixtures.periodicTask("period-ok", 5_000); + long before = System.currentTimeMillis(); + + executionManager.executeTask(task, executor); + assertTrue(done.await(5, TimeUnit.SECONDS)); + Thread.sleep(100); + assertEquals(ScheduledTask.TaskStatus.SCHEDULED, task.getStatus()); + assertNotNull(task.getNextScheduledExecution()); + assertTrue(task.getNextScheduledExecution().getTime() >= before + 5_000); + } + + @Test + public void testHandleTaskCompletionPeriodZeroDoesNotReschedule() throws Exception { + CountDownLatch done = new CountDownLatch(1); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "period-zero"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + callback.complete(); + done.countDown(); + } + }; + ScheduledTask task = TaskTestFixtures.rawTask("period-zero"); + task.setOneShot(false); + TaskTestFixtures.setPeriodField(task, 0L); + + executionManager.executeTask(task, executor); + assertTrue(done.await(5, TimeUnit.SECONDS)); + Thread.sleep(100); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, task.getStatus()); + } + + @Test + public void testCompletionAndErrorIgnoredWhenNotRunning() throws Exception { + CountDownLatch done = new CountDownLatch(1); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "ignored-cb"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + task.setStatus(ScheduledTask.TaskStatus.CANCELLED); + callback.complete(); + callback.fail("should-ignore"); + done.countDown(); + } + }; + ScheduledTask task = TaskTestFixtures.baseTask("ignored-cb"); + long completedBefore = metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_COMPLETED); + + executionManager.executeTask(task, executor); + assertTrue(done.await(5, TimeUnit.SECONDS)); + Thread.sleep(100); + assertEquals(ScheduledTask.TaskStatus.CANCELLED, task.getStatus()); + assertEquals(completedBefore, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_COMPLETED)); + } + + @Test + public void testReclaimPrematureCrashBeforeCompletion() throws Exception { + CountDownLatch done = new CountDownLatch(1); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "reclaim"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + task.setStatus(ScheduledTask.TaskStatus.CRASHED); + // executingNodeId already set by wrapper to NODE + callback.complete(); + done.countDown(); + } + }; + ScheduledTask task = TaskTestFixtures.baseTask("reclaim"); + + executionManager.executeTask(task, executor); + assertTrue(done.await(5, TimeUnit.SECONDS)); + Thread.sleep(100); + assertEquals(ScheduledTask.TaskStatus.COMPLETED, task.getStatus()); + assertFalse(task.isEnabled()); + } + + @Test + public void testWrapperSkipsOnShutdownAndReleasesDispatchClaim() throws Exception { + when(schedulerService.isShutdownNow()).thenReturn(true); + CountDownLatch executed = new CountDownLatch(1); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "shut-skip"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + executed.countDown(); + callback.complete(); + } + }; + ScheduledTask task = TaskTestFixtures.baseTask("shut-skip"); + executionManager.executeTask(task, executor); + Thread.sleep(150); + assertEquals(1, executed.getCount()); + + when(schedulerService.isShutdownNow()).thenReturn(false); + CountDownLatch started = new CountDownLatch(1); + TaskExecutor executor2 = new TaskExecutor() { + @Override public String getTaskType() { return "shut-skip"; } + @Override public void execute(ScheduledTask t, TaskStatusCallback callback) { + started.countDown(); + callback.complete(); + } + }; + executionManager.executeTask(task, executor2); + assertTrue(started.await(5, TimeUnit.SECONDS), "dispatch claim must be released after shutdown skip"); + } + + @Test + public void testPrepareForExecutionClearsWaitingState() { + ScheduledTask task = TaskTestFixtures.baseTask("waiting"); + task.setStatus(ScheduledTask.TaskStatus.WAITING); + task.setWaitingOnTasks(new java.util.HashSet<>(Collections.singleton("dep"))); + task.setWaitingForTaskType("depType"); + task.setNextScheduledExecution(new Date(System.currentTimeMillis() - 1000)); + + assertTrue(executionManager.prepareForExecution(task)); + assertEquals(ScheduledTask.TaskStatus.RUNNING, task.getStatus()); + assertNull(task.getWaitingOnTasks()); + assertNull(task.getWaitingForTaskType()); + } + + @Test + public void testPrepareForExecutionCrashedIgnoresFutureNextScheduled() { + ScheduledTask task = TaskTestFixtures.baseTask("crash-due"); + task.setStatus(ScheduledTask.TaskStatus.CRASHED); + task.setNextScheduledExecution(new Date(System.currentTimeMillis() + 60_000)); + + assertTrue(executionManager.prepareForExecution(task)); + assertEquals(ScheduledTask.TaskStatus.RUNNING, task.getStatus()); + } + + @Test + public void testExecuteTaskSkipsWhenAlreadyRunningOrDisabled() throws Exception { + AtomicInteger runs = new AtomicInteger(); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "skip"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + runs.incrementAndGet(); + callback.complete(); + } + }; + ScheduledTask running = TaskTestFixtures.runningTask("skip", NODE); + executionManager.executeTask(running, executor); + Thread.sleep(100); + assertEquals(0, runs.get()); + + ScheduledTask disabled = TaskTestFixtures.baseTask("skip"); + disabled.setEnabled(false); + executionManager.executeTask(disabled, executor); + Thread.sleep(100); + assertEquals(0, runs.get()); + } + + @Test + public void testTerminalCompleteSkippedWhenCancelledInStore() throws Exception { + CountDownLatch done = new CountDownLatch(1); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "cancel-race"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + ScheduledTask cancelled = TaskTestFixtures.baseTask("cancel-race"); + cancelled.setItemId(task.getItemId()); + cancelled.setStatus(ScheduledTask.TaskStatus.CANCELLED); + when(schedulerService.getTask(eq(task.getItemId()), eq(true))).thenReturn(cancelled); + callback.complete(); + done.countDown(); + } + }; + ScheduledTask task = TaskTestFixtures.baseTask("cancel-race"); + executionManager.executeTask(task, executor); + assertTrue(done.await(5, TimeUnit.SECONDS)); + Thread.sleep(100); + assertEquals(ScheduledTask.TaskStatus.CANCELLED, task.getStatus()); + // persistTerminalState() is skipped (terminal transition correctly bailed out above), but + // the wrapper's cleanup still CAS-clears executingNodeId once; that write is expected to + // fail harmlessly against a real store since the document moved on to CANCELLED. + verify(schedulerService, times(1)).saveTaskWithRefresh(any()); + assertEquals(0, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_COMPLETED)); + } + + @Test + public void testTerminalCompleteSkippedWhenPeerHoldsLock() throws Exception { + CountDownLatch done = new CountDownLatch(1); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "peer-lock"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + ScheduledTask peer = TaskTestFixtures.runningTask("peer-lock", "peer-node"); + peer.setItemId(task.getItemId()); + peer.setLockDate(new Date()); + when(schedulerService.getTask(eq(task.getItemId()), eq(true))).thenReturn(peer); + when(lockManager.isLockExpired(peer)).thenReturn(false); + callback.complete(); + done.countDown(); + } + }; + ScheduledTask task = TaskTestFixtures.baseTask("peer-lock"); + executionManager.executeTask(task, executor); + assertTrue(done.await(5, TimeUnit.SECONDS)); + Thread.sleep(100); + assertEquals(ScheduledTask.TaskStatus.RUNNING, task.getStatus()); + // persistTerminalState() is skipped (peer holds the lock), but the wrapper's cleanup still + // CAS-clears executingNodeId once; that write is expected to fail harmlessly against a real + // store since the peer is the authoritative owner. + verify(schedulerService, times(1)).saveTaskWithRefresh(any()); + } + + @Test + public void testAbortPreparedExecutionOnShutdownReleasesLockAndMarksCrashed() throws Exception { + AtomicInteger prepares = new AtomicInteger(); + // First call: not shutting down so prepare runs; flip during prepare via answer on acquireLock + when(lockManager.acquireLock(any())).thenAnswer(inv -> { + prepares.incrementAndGet(); + when(schedulerService.isShutdownNow()).thenReturn(true); + return true; + }); + + CountDownLatch executed = new CountDownLatch(1); + TaskExecutor executor = new TaskExecutor() { + @Override public String getTaskType() { return "abort-prep"; } + @Override public void execute(ScheduledTask task, TaskStatusCallback callback) { + executed.countDown(); + callback.complete(); + } + }; + ScheduledTask task = TaskTestFixtures.baseTask("abort-prep"); + executionManager.executeTask(task, executor); + Thread.sleep(300); + assertEquals(1, executed.getCount(), "executor must not run after shutdown-abort"); + assertEquals(ScheduledTask.TaskStatus.CRASHED, task.getStatus()); + assertNull(task.getLockOwner()); + verify(schedulerService, atLeastOnce()).saveTask(any(ScheduledTask.class), eq(true)); + } +} diff --git a/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskHistoryManagerTest.java b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskHistoryManagerTest.java new file mode 100644 index 0000000000..ac6ec637ba --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskHistoryManagerTest.java @@ -0,0 +1,132 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.unomi.services.impl.scheduler; + +import org.apache.unomi.api.tasks.ScheduledTask; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * Unit tests for {@link TaskHistoryManager}. + * Invoked by Surefire: {@code -Dtest=TaskHistoryManagerTest}. No data-file I/O. + * Glob: no prior TaskHistoryManagerTest. User asked for Task*Managers unit tests. + */ +public class TaskHistoryManagerTest { + + private TaskHistoryManager historyManager; + private TaskMetricsManager metricsManager; + + @BeforeEach + public void setUp() { + metricsManager = new TaskMetricsManager(); + historyManager = new TaskHistoryManager(); + historyManager.setNodeId("history-node"); + historyManager.setMetricsManager(metricsManager); + } + + @Test + public void testRecordSuccessCreatesHistoryAndMetrics() { + ScheduledTask task = TaskTestFixtures.baseTask("hist"); + historyManager.recordSuccess(task, 123); + List> history = historyManager.getExecutionHistory(task); + assertEquals(1, history.size()); + assertEquals("SUCCESS", history.get(0).get("status")); + assertEquals("history-node", history.get(0).get("nodeId")); + assertEquals(123L, ((Number) history.get(0).get("executionTime")).longValue()); + assertEquals(1, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_COMPLETED)); + assertEquals(123, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_EXECUTION_TIME)); + } + + @Test + public void testRecordFailureCrashCancelResumeRetry() { + ScheduledTask task = TaskTestFixtures.baseTask("hist"); + historyManager.recordFailure(task, "boom"); + historyManager.recordCrash(task); + historyManager.recordCancellation(task); + historyManager.recordResume(task); + historyManager.recordRetry(task); + List> history = historyManager.getExecutionHistory(task); + assertEquals(5, history.size()); + assertEquals("FAILED", history.get(0).get("status")); + assertEquals("CRASHED", history.get(1).get("status")); + assertEquals("CANCELLED", history.get(2).get("status")); + assertEquals("RESUMED", history.get(3).get("status")); + assertEquals("RETRIED", history.get(4).get("status")); + assertEquals(1, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_FAILED)); + assertEquals(1, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_CRASHED)); + assertEquals(1, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_CANCELLED)); + assertEquals(1, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_RESUMED)); + assertEquals(1, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_RETRIED)); + } + + @Test + public void testHistoryCappedAtTenFifo() { + ScheduledTask task = TaskTestFixtures.baseTask("hist"); + for (int i = 0; i < 12; i++) { + historyManager.recordFailure(task, "err-" + i); + } + List> history = historyManager.getExecutionHistory(task); + assertEquals(10, history.size()); + assertEquals("err-2", history.get(0).get("error")); + assertEquals("err-11", history.get(9).get("error")); + } + + @Test + public void testUnmodifiableStatusDetailsAreCopied() { + ScheduledTask task = TaskTestFixtures.baseTask("hist"); + task.setStatusDetails(Collections.unmodifiableMap(new HashMap<>())); + assertDoesNotThrow(() -> historyManager.recordSuccess(task, 1)); + assertEquals(1, historyManager.getExecutionHistory(task).size()); + } + + @Test + public void testGetExecutionHistoryEmptyWhenMissing() { + ScheduledTask task = TaskTestFixtures.baseTask("hist"); + assertTrue(historyManager.getExecutionHistory(task).isEmpty()); + task.setStatusDetails(new HashMap<>()); + assertTrue(historyManager.getExecutionHistory(task).isEmpty()); + } + + @Test + public void testUnmodifiableExecutionHistoryListIsCopied() { + ScheduledTask task = TaskTestFixtures.baseTask("hist"); + Map first = new HashMap<>(); + first.put("status", "FAILED"); + first.put("error", "seed"); + Map details = new HashMap<>(); + details.put("executionHistory", Collections.unmodifiableList(Collections.singletonList(first))); + task.setStatusDetails(details); + + assertDoesNotThrow(() -> historyManager.recordSuccess(task, 5)); + List> history = historyManager.getExecutionHistory(task); + assertEquals(2, history.size()); + assertEquals("SUCCESS", history.get(1).get("status")); + + for (int i = 0; i < 12; i++) { + historyManager.recordFailure(task, "cap-" + i); + } + assertEquals(10, historyManager.getExecutionHistory(task).size()); + } +} diff --git a/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskLockManagerTest.java b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskLockManagerTest.java new file mode 100644 index 0000000000..2497838845 --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskLockManagerTest.java @@ -0,0 +1,506 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.unomi.services.impl.scheduler; + +import org.apache.unomi.api.tasks.ScheduledTask; +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 org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; + +import java.util.Arrays; +import java.util.Collections; +import java.util.Date; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +/** + * Unit tests for {@link TaskLockManager}. + * Invoked by Surefire: {@code -Dtest=TaskLockManagerTest}. No data-file I/O. + * Glob: existing TaskLockManagerTest. User: edge/breakage case coverage. + */ +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +public class TaskLockManagerTest { + + private static final String NODE = "lock-node"; + + @Mock + private SchedulerServiceImpl schedulerService; + + private TaskMetricsManager metricsManager; + private TaskLockManager lockManager; + + @BeforeEach + public void setUp() { + metricsManager = new TaskMetricsManager(); + lockManager = new TaskLockManager(); + lockManager.setNodeId(NODE); + lockManager.setLockTimeout(1000); + lockManager.setMetricsManager(metricsManager); + lockManager.setSchedulerService(schedulerService); + when(schedulerService.getActiveNodes()).thenReturn(Collections.singletonList(NODE)); + when(schedulerService.saveTask(any(ScheduledTask.class))).thenReturn(true); + when(schedulerService.saveTask(any(ScheduledTask.class), anyBoolean())).thenReturn(true); + when(schedulerService.saveTaskWithRefresh(any(ScheduledTask.class))).thenReturn(true); + when(schedulerService.saveTaskWithRefresh(any(ScheduledTask.class), anyBoolean())).thenReturn(true); + } + + @Test + public void testNullTaskGuards() { + assertFalse(lockManager.acquireLock(null)); + assertFalse(lockManager.releaseLock(null)); + assertTrue(lockManager.isLockExpired(null)); + } + + @Test + public void testAllowParallelAlwaysAcquiresMarker() { + ScheduledTask task = TaskTestFixtures.baseTask("parallel"); + task.setAllowParallelExecution(true); + assertTrue(lockManager.acquireLock(task)); + assertEquals(NODE, task.getLockOwner()); + assertNotNull(task.getLockDate()); + verify(schedulerService, never()).saveTaskWithRefresh(any()); + assertEquals(1, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_LOCK_ACQUIRED)); + } + + @Test + public void testInMemoryLockRejectsOtherValidOwner() { + ScheduledTask task = TaskTestFixtures.baseTask("mem"); + task.setPersistent(false); + task.setLockOwner("other"); + task.setLockDate(new Date()); + assertFalse(lockManager.acquireLock(task)); + } + + @Test + public void testInMemoryLockAllowsExpiredOwner() { + ScheduledTask task = TaskTestFixtures.baseTask("mem"); + task.setPersistent(false); + task.setLockOwner("other"); + task.setLockDate(new Date(System.currentTimeMillis() - 5000)); + assertTrue(lockManager.acquireLock(task)); + assertEquals(NODE, task.getLockOwner()); + verify(schedulerService).saveTask(task); + } + + @Test + public void testDistributedLockOccConflict() { + ScheduledTask task = TaskTestFixtures.baseTask("dist"); + task.setNextScheduledExecution(new Date(System.currentTimeMillis() - 10_000)); + task.setSystemMetadata("seq_no", 1L); + task.setSystemMetadata("primary_term", 1L); + when(schedulerService.getTask(task.getItemId())).thenReturn(task); + when(schedulerService.saveTaskWithRefresh(any(ScheduledTask.class))).thenReturn(false); + + assertFalse(lockManager.acquireLock(task)); + assertEquals(1, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_LOCK_CONFLICTS)); + } + + @Test + public void testDistributedLockSuccess() { + ScheduledTask task = TaskTestFixtures.baseTask("dist"); + task.setNextScheduledExecution(new Date(System.currentTimeMillis() - 10_000)); + ScheduledTask latest = TaskTestFixtures.baseTask("dist"); + latest.setItemId(task.getItemId()); + latest.setNextScheduledExecution(task.getNextScheduledExecution()); + latest.setSystemMetadata("seq_no", 3L); + latest.setSystemMetadata("primary_term", 1L); + when(schedulerService.getTask(task.getItemId())).thenReturn(latest); + when(schedulerService.saveTaskWithRefresh(any(ScheduledTask.class))).thenReturn(true); + + assertTrue(lockManager.acquireLock(task)); + assertEquals(NODE, task.getLockOwner()); + assertEquals(1, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_LOCK_ACQUIRED)); + } + + @Test + public void testDistributedLockSuccessDoesNotReVerify() { + // A successful CAS write is itself authoritative proof of acquisition: exactly one + // GET-by-id (the pre-CAS read) and one CAS write, no post-write re-read/verification. + ScheduledTask task = TaskTestFixtures.baseTask("dist-no-reverify"); + task.setNextScheduledExecution(new Date(System.currentTimeMillis() - 10_000)); + ScheduledTask latest = TaskTestFixtures.baseTask("dist-no-reverify"); + latest.setItemId(task.getItemId()); + latest.setNextScheduledExecution(task.getNextScheduledExecution()); + latest.setSystemMetadata("seq_no", 5L); + latest.setSystemMetadata("primary_term", 1L); + when(schedulerService.getTask(task.getItemId())).thenReturn(latest); + when(schedulerService.saveTaskWithRefresh(any(ScheduledTask.class))).thenReturn(true); + + assertTrue(lockManager.acquireLock(task)); + verify(schedulerService, times(1)).getTask(task.getItemId()); + verify(schedulerService, times(1)).saveTaskWithRefresh(any(ScheduledTask.class)); + } + + @Test + public void testCrashedTaskBypassesAffinity() { + ScheduledTask task = TaskTestFixtures.baseTask("crash"); + task.setStatus(ScheduledTask.TaskStatus.CRASHED); + task.setNextScheduledExecution(new Date()); // would otherwise be in primary window for other node + when(schedulerService.getActiveNodes()).thenReturn(Arrays.asList("other-node", NODE)); + ScheduledTask latest = TaskTestFixtures.baseTask("crash"); + latest.setItemId(task.getItemId()); + latest.setStatus(ScheduledTask.TaskStatus.CRASHED); + latest.setNextScheduledExecution(task.getNextScheduledExecution()); + latest.setSystemMetadata("seq_no", 1L); + latest.setSystemMetadata("primary_term", 1L); + when(schedulerService.getTask(task.getItemId())).thenReturn(latest); + when(schedulerService.saveTaskWithRefresh(any(ScheduledTask.class))).thenReturn(true); + assertTrue(lockManager.acquireLock(task)); + } + + @Test + public void testReleaseLockRejectsNonOwnerValidLock() { + ScheduledTask task = TaskTestFixtures.baseTask("rel"); + task.setLockOwner("other"); + task.setLockDate(new Date()); + assertFalse(lockManager.releaseLock(task)); + } + + @Test + public void testReleaseLockClearsExpiredUsingFreshLoad() { + ScheduledTask caller = TaskTestFixtures.baseTask("rel"); + caller.setStatus(ScheduledTask.TaskStatus.SCHEDULED); // mutated retry view + caller.setLockOwner("other"); + caller.setLockDate(new Date(System.currentTimeMillis() - 5000)); + + ScheduledTask stored = TaskTestFixtures.runningTask("rel", "other"); + stored.setItemId(caller.getItemId()); + when(schedulerService.getTask(eq(caller.getItemId()), eq(true))).thenReturn(stored); + + assertTrue(lockManager.releaseLock(caller)); + assertNull(stored.getLockOwner()); + assertEquals(ScheduledTask.TaskStatus.RUNNING, stored.getStatus()); + verify(schedulerService).saveTaskWithRefresh(stored, true); + assertEquals(1, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_LOCK_RELEASED)); + } + + @Test + public void testIsLockExpired() { + ScheduledTask task = TaskTestFixtures.baseTask("exp"); + assertTrue(lockManager.isLockExpired(task)); + task.setLockDate(new Date()); + assertFalse(lockManager.isLockExpired(task)); + task.setLockDate(new Date(System.currentTimeMillis() - 5000)); + assertTrue(lockManager.isLockExpired(task)); + } + + @Test + public void testIsLockExpiredFalseWhenAgeEqualsTimeout() { + ScheduledTask task = TaskTestFixtures.baseTask("exp"); + task.setLockDate(new Date(System.currentTimeMillis() - 1000)); + assertFalse(lockManager.isLockExpired(task)); + } + + @Test + public void testAffinityBlocksBackupDuringPrimaryWindow() { + List nodes = Arrays.asList("aaa-node", NODE, "zzz-node"); + when(schedulerService.getActiveNodes()).thenReturn(nodes); + String itemId = TaskTestFixtures.itemIdNotPrimaryFor(NODE, nodes); + ScheduledTask task = TaskTestFixtures.baseTask("aff"); + task.setItemId(itemId); + task.setNextScheduledExecution(new Date(System.currentTimeMillis() - 500)); + + assertFalse(lockManager.acquireLock(task)); + verify(schedulerService, never()).saveTaskWithRefresh(any()); + } + + @Test + public void testAffinityAllowsBackupInStaggeredWindow() { + List nodes = Arrays.asList("aaa-node", NODE, "zzz-node"); + when(schedulerService.getActiveNodes()).thenReturn(nodes); + String itemId = TaskTestFixtures.itemIdNotPrimaryFor(NODE, nodes); + int backupOrder = TaskTestFixtures.backupOrderFor(itemId, NODE, nodes); + assertTrue(backupOrder >= 1); + + long delayMs = 3000 + ((backupOrder - 1) * 500L) + 50; + ScheduledTask task = TaskTestFixtures.baseTask("aff"); + task.setItemId(itemId); + task.setNextScheduledExecution(new Date(System.currentTimeMillis() - delayMs)); + stubSuccessfulDistributedAcquire(task); + + assertTrue(lockManager.acquireLock(task)); + } + + @Test + public void testAffinityOpenFieldAfterAllBackupWindows() { + List nodes = Arrays.asList("aaa-node", NODE, "zzz-node"); + when(schedulerService.getActiveNodes()).thenReturn(nodes); + String itemId = TaskTestFixtures.itemIdNotPrimaryFor(NODE, nodes); + // openFieldStart = 3000 + 2*500 = 4000 + ScheduledTask task = TaskTestFixtures.baseTask("aff"); + task.setItemId(itemId); + task.setNextScheduledExecution(new Date(System.currentTimeMillis() - 4500)); + stubSuccessfulDistributedAcquire(task); + + assertTrue(lockManager.acquireLock(task)); + } + + @Test + public void testAffinityRejectsNodeNotInActiveList() { + List nodes = Arrays.asList("aaa-node", "bbb-node", "ccc-node"); + when(schedulerService.getActiveNodes()).thenReturn(nodes); + ScheduledTask task = TaskTestFixtures.baseTask("aff"); + task.setItemId("affinity-0"); + task.setNextScheduledExecution(new Date(System.currentTimeMillis() - 3500)); + + assertFalse(lockManager.acquireLock(task)); + verify(schedulerService, never()).saveTaskWithRefresh(any()); + } + + @Test + public void testDistributedLockRejectsValidForeignOwner() { + ScheduledTask task = TaskTestFixtures.baseTask("dist"); + task.setNextScheduledExecution(new Date(System.currentTimeMillis() - 10_000)); + ScheduledTask latest = TaskTestFixtures.baseTask("dist"); + latest.setItemId(task.getItemId()); + latest.setLockOwner("other"); + latest.setLockDate(new Date()); + latest.setSystemMetadata("seq_no", 1L); + latest.setSystemMetadata("primary_term", 1L); + when(schedulerService.getTask(task.getItemId())).thenReturn(latest); + + assertFalse(lockManager.acquireLock(task)); + verify(schedulerService, never()).saveTaskWithRefresh(any()); + } + + @Test + public void testDistributedLockFailsWhenTaskMissing() { + ScheduledTask task = TaskTestFixtures.baseTask("dist"); + task.setNextScheduledExecution(new Date(System.currentTimeMillis() - 10_000)); + when(schedulerService.getTask(task.getItemId())).thenReturn(null); + + assertFalse(lockManager.acquireLock(task)); + verify(schedulerService, never()).saveTaskWithRefresh(any()); + } + + @Test + public void testReleaseLockReturnsFalseWhenPersistFails() { + ScheduledTask task = TaskTestFixtures.runningTask("rel", NODE); + when(schedulerService.getTask(eq(task.getItemId()), eq(true))).thenReturn(task); + when(schedulerService.saveTaskWithRefresh(any(ScheduledTask.class), eq(true))).thenReturn(false); + + assertFalse(lockManager.releaseLock(task)); + assertEquals(0, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_LOCK_RELEASED)); + } + + + @Test + public void testReleaseLockDoesNotClearPeersFreshLock() { + // Caller still thinks the dead owner's expired lock is clearable... + ScheduledTask caller = TaskTestFixtures.baseTask("steal"); + caller.setLockOwner("dead-node"); + caller.setLockDate(new Date(System.currentTimeMillis() - 5000)); + + // ...but the store already has a fresh lock held by a peer stealer. + ScheduledTask peerHeld = TaskTestFixtures.runningTask("steal", "peer-node"); + peerHeld.setItemId(caller.getItemId()); + peerHeld.setLockDate(new Date()); + when(schedulerService.getTask(eq(caller.getItemId()), eq(true))).thenReturn(peerHeld); + + assertFalse(lockManager.releaseLock(caller)); + assertEquals("peer-node", peerHeld.getLockOwner()); + verify(schedulerService, never()).saveTaskWithRefresh(any(ScheduledTask.class), eq(true)); + assertEquals(0, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_LOCK_RELEASED)); + } + + + + @Test + public void testInMemoryExclusiveLockSerializesConcurrentAcquires() throws Exception { + ScheduledTask shared = TaskTestFixtures.baseTask("mem-race"); + shared.setPersistent(false); + when(schedulerService.getTask(shared.getItemId())).thenReturn(shared); + when(schedulerService.saveTask(any(ScheduledTask.class))).thenAnswer(inv -> { + ScheduledTask t = inv.getArgument(0); + shared.setLockOwner(t.getLockOwner()); + shared.setLockDate(t.getLockDate()); + return true; + }); + + // Distinct node IDs — same-node re-acquire is allowed; exclusivity is cross-owner. + TaskLockManager[] managers = new TaskLockManager[8]; + for (int i = 0; i < managers.length; i++) { + TaskLockManager m = new TaskLockManager(); + m.setNodeId("mem-node-" + i); + m.setLockTimeout(1000); + m.setMetricsManager(new TaskMetricsManager()); + m.setSchedulerService(schedulerService); + managers[i] = m; + } + + java.util.concurrent.atomic.AtomicInteger wins = new java.util.concurrent.atomic.AtomicInteger(); + CountDownLatch start = new CountDownLatch(1); + CountDownLatch done = new CountDownLatch(managers.length); + for (TaskLockManager m : managers) { + Thread t = new Thread(() -> { + try { + start.await(); + ScheduledTask view = TaskTestFixtures.baseTask("mem-race"); + view.setItemId(shared.getItemId()); + view.setPersistent(false); + if (m.acquireLock(view)) { + wins.incrementAndGet(); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } finally { + done.countDown(); + } + }); + t.start(); + } + start.countDown(); + assertTrue(done.await(5, TimeUnit.SECONDS)); + assertEquals(1, wins.get(), "Only one in-memory exclusive acquire should win across node ids"); + assertNotNull(shared.getLockOwner()); + } + + @Test + public void testDistributedLockLosesRaceToPeerCasWithoutClobbering() { + // A peer's CAS lands between our read and our own write attempt (the backend rejects + // our write because seq_no/primary_term no longer match what we read). There is no + // verification window to race in anymore - the backend's own CAS result is the single, + // atomic point of truth - so losing a race must fail closed immediately and must never + // attempt to touch (let alone clear) whatever the peer just wrote. + String taskId = "peer-race"; + ScheduledTask task = TaskTestFixtures.baseTask("peer-race"); + task.setItemId(taskId); + task.setNextScheduledExecution(new Date(System.currentTimeMillis() - 10_000)); + ScheduledTask latest = TaskTestFixtures.baseTask("peer-race"); + latest.setItemId(taskId); + latest.setNextScheduledExecution(task.getNextScheduledExecution()); + latest.setSystemMetadata("seq_no", 1L); + latest.setSystemMetadata("primary_term", 1L); + when(schedulerService.getTask(taskId)).thenReturn(latest); + // Backend rejects our write: precondition (seq_no=1) no longer matches - a peer moved it. + when(schedulerService.saveTaskWithRefresh(any(ScheduledTask.class))).thenReturn(false); + + assertFalse(lockManager.acquireLock(task)); + assertEquals(1, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_LOCK_CONFLICTS)); + // Losing a CAS race must never trigger a release call - releaseLock() always re-loads + // via getTask(id, true) first, so its absence proves we never attempted to touch the peer. + verify(schedulerService, never()).getTask(eq(taskId), anyBoolean()); + } + + @Test + public void testRenewLockSkipsNonExclusiveAndNonPersistentTasks() { + ScheduledTask parallel = TaskTestFixtures.baseTask("renew-parallel"); + parallel.setAllowParallelExecution(true); + assertTrue(lockManager.renewLock(parallel)); + + ScheduledTask inMemory = TaskTestFixtures.baseTask("renew-mem"); + inMemory.setPersistent(false); + assertTrue(lockManager.renewLock(inMemory)); + + verify(schedulerService, never()).getTask(anyString()); + verify(schedulerService, never()).saveTaskWithRefresh(any(ScheduledTask.class)); + } + + @Test + public void testRenewLockRefusesWhenNotLocalOwner() { + ScheduledTask task = TaskTestFixtures.baseTask("renew-foreign"); + task.setLockOwner("other-node"); + task.setLockDate(new Date()); + + assertFalse(lockManager.renewLock(task)); + verify(schedulerService, never()).saveTaskWithRefresh(any(ScheduledTask.class)); + } + + @Test + public void testRenewLockRefusesWhenStoreOwnerChanged() { + ScheduledTask task = TaskTestFixtures.baseTask("renew-stolen"); + task.setLockOwner(NODE); + task.setLockDate(new Date()); + ScheduledTask storeView = TaskTestFixtures.baseTask("renew-stolen"); + storeView.setItemId(task.getItemId()); + storeView.setLockOwner("peer-node"); + storeView.setLockDate(new Date()); + when(schedulerService.getTask(task.getItemId())).thenReturn(storeView); + + assertFalse(lockManager.renewLock(task)); + verify(schedulerService, never()).saveTaskWithRefresh(any(ScheduledTask.class)); + } + + @Test + public void testRenewLockRefreshesLockDateAndSyncsOccMetadata() { + Date staleDate = new Date(System.currentTimeMillis() - 60_000); + ScheduledTask task = TaskTestFixtures.baseTask("renew-ok"); + task.setLockOwner(NODE); + task.setLockDate(staleDate); + task.setSystemMetadata("seq_no", 3L); + task.setSystemMetadata("primary_term", 1L); + + ScheduledTask storeView = TaskTestFixtures.baseTask("renew-ok"); + storeView.setItemId(task.getItemId()); + storeView.setLockOwner(NODE); + storeView.setLockDate(staleDate); + storeView.setSystemMetadata("seq_no", 7L); + storeView.setSystemMetadata("primary_term", 2L); + when(schedulerService.getTask(task.getItemId())).thenReturn(storeView); + when(schedulerService.saveTaskWithRefresh(storeView)).thenAnswer(inv -> { + // Simulate the store handing back post-save OCC tokens on the saved instance + storeView.setSystemMetadata("seq_no", 8L); + return true; + }); + + assertTrue(lockManager.renewLock(task)); + assertTrue(task.getLockDate().after(staleDate), + "Renewal must refresh the caller's lockDate"); + assertEquals(8L, ((Number) task.getSystemMetadata("seq_no")).longValue(), + "Renewal must sync post-save seq_no back so later CAS writes succeed"); + assertEquals(2L, ((Number) task.getSystemMetadata("primary_term")).longValue()); + } + + @Test + public void testRenewLockFailsClosedWhenCasLost() { + Date originalDate = new Date(System.currentTimeMillis() - 500); + ScheduledTask task = TaskTestFixtures.baseTask("renew-cas"); + task.setLockOwner(NODE); + task.setLockDate(originalDate); + ScheduledTask storeView = TaskTestFixtures.baseTask("renew-cas"); + storeView.setItemId(task.getItemId()); + storeView.setLockOwner(NODE); + storeView.setLockDate(originalDate); + when(schedulerService.getTask(task.getItemId())).thenReturn(storeView); + when(schedulerService.saveTaskWithRefresh(storeView)).thenReturn(false); + + assertFalse(lockManager.renewLock(task)); + assertEquals(originalDate, task.getLockDate(), + "A lost renewal CAS must not touch the caller's lock state"); + } + + private void stubSuccessfulDistributedAcquire(ScheduledTask task) { + ScheduledTask latest = TaskTestFixtures.baseTask(task.getTaskType()); + latest.setItemId(task.getItemId()); + latest.setNextScheduledExecution(task.getNextScheduledExecution()); + latest.setSystemMetadata("seq_no", 1L); + latest.setSystemMetadata("primary_term", 1L); + when(schedulerService.getTask(task.getItemId())).thenReturn(latest); + when(schedulerService.saveTaskWithRefresh(any(ScheduledTask.class))).thenReturn(true); + } +} diff --git a/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskMetricsManagerTest.java b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskMetricsManagerTest.java new file mode 100644 index 0000000000..97515b70d2 --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskMetricsManagerTest.java @@ -0,0 +1,96 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.unomi.services.impl.scheduler; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * Unit tests for {@link TaskMetricsManager}. + * Invoked by Surefire: {@code -Dtest=TaskMetricsManagerTest}. No data-file I/O. + * Glob: no prior TaskMetricsManagerTest. User asked for Task*Managers unit tests. + */ +public class TaskMetricsManagerTest { + + private TaskMetricsManager metricsManager; + + @BeforeEach + public void setUp() { + metricsManager = new TaskMetricsManager(); + } + + @Test + public void testMissingMetricReturnsZero() { + assertEquals(0, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_COMPLETED)); + } + + @Test + public void testIncrementAndAdd() { + metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_COMPLETED); + metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_COMPLETED); + metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_EXECUTION_TIME, 42); + assertEquals(2, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_COMPLETED)); + assertEquals(42, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_EXECUTION_TIME)); + } + + @Test + public void testGetAllMetricsAndReset() { + metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_FAILED); + Map all = metricsManager.getAllMetrics(); + assertEquals(1L, all.get(TaskMetricsManager.METRIC_TASKS_FAILED)); + metricsManager.resetMetrics(); + assertEquals(0, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_FAILED)); + assertTrue(metricsManager.getAllMetrics().isEmpty()); + } + + @Test + public void testConcurrentIncrements() throws Exception { + int threads = 8; + int perThread = 100; + ExecutorService pool = Executors.newFixedThreadPool(threads); + CountDownLatch start = new CountDownLatch(1); + CountDownLatch done = new CountDownLatch(threads); + for (int i = 0; i < threads; i++) { + pool.submit(() -> { + try { + start.await(); + for (int j = 0; j < perThread; j++) { + metricsManager.updateMetric(TaskMetricsManager.METRIC_TASKS_LOCK_ACQUIRED); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } finally { + done.countDown(); + } + }); + } + start.countDown(); + assertTrue(done.await(5, TimeUnit.SECONDS)); + pool.shutdownNow(); + assertEquals(threads * perThread, + metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_LOCK_ACQUIRED)); + } +} diff --git a/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskRecoveryManagerTest.java b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskRecoveryManagerTest.java new file mode 100644 index 0000000000..3a1a11e708 --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskRecoveryManagerTest.java @@ -0,0 +1,361 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.unomi.services.impl.scheduler; + +import org.apache.unomi.api.tasks.ScheduledTask; +import org.apache.unomi.api.tasks.TaskExecutor; +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 org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; + +import java.util.Arrays; +import java.util.Collections; +import java.util.Date; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +/** + * Unit tests for {@link TaskRecoveryManager}. + * Invoked by Surefire: {@code -Dtest=TaskRecoveryManagerTest}. No data-file I/O. + * Glob: no prior TaskRecoveryManagerTest. User asked for Task*Managers unit tests. + */ +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +public class TaskRecoveryManagerTest { + + private static final String NODE = "recovery-node"; + + @Mock private SchedulerServiceImpl schedulerService; + @Mock private TaskLockManager lockManager; + @Mock private TaskExecutionManager executionManager; + @Mock private TaskExecutorRegistry executorRegistry; + @Mock private TaskExecutor resumeExecutor; + @Mock private TaskExecutor plainExecutor; + + private TaskStateManager stateManager; + private TaskMetricsManager metricsManager; + private TaskRecoveryManager recoveryManager; + + @BeforeEach + public void setUp() { + stateManager = new TaskStateManager(); + metricsManager = new TaskMetricsManager(); + recoveryManager = new TaskRecoveryManager(); + recoveryManager.setNodeId(NODE); + recoveryManager.setStateManager(stateManager); + recoveryManager.setLockManager(lockManager); + recoveryManager.setMetricsManager(metricsManager); + recoveryManager.setExecutionManager(executionManager); + recoveryManager.setExecutorRegistry(executorRegistry); + recoveryManager.setSchedulerService(schedulerService); + + when(schedulerService.saveTask(any(ScheduledTask.class))).thenReturn(true); + when(schedulerService.saveTaskWithRefresh(any(ScheduledTask.class))).thenReturn(true); + when(schedulerService.getTask(anyString(), anyBoolean())).thenAnswer(inv -> { + // Default: no separate store copy — recover against the in-memory instance. + return null; + }); + when(lockManager.isLockExpired(any(ScheduledTask.class))).thenReturn(true); + recoveryManager.setExecutorNode(true); + when(schedulerService.findLockedTasks()).thenReturn(Collections.emptyList()); + when(resumeExecutor.canResume(any())).thenReturn(true); + when(plainExecutor.canResume(any())).thenReturn(false); + } + + @Test + public void testPrepareForShutdownSkipsRecovery() { + recoveryManager.prepareForShutdown(); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.singletonList(TaskTestFixtures.runningTask("t", "dead"))); + recoveryManager.recoverCrashedTasks(); + verify(executionManager, never()).executeTask(any(), any()); + } + + @Test + public void testRecoverRunningExpiredResumesWhenCanResume() { + ScheduledTask task = TaskTestFixtures.runningTask("resume-type", "dead"); + task.setCheckpointData(Collections.singletonMap("step", 1)); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.singletonList(task)); + when(executorRegistry.getExecutor("resume-type")).thenReturn(resumeExecutor); + + recoveryManager.recoverCrashedTasks(); + + assertEquals(ScheduledTask.TaskStatus.CRASHED, task.getStatus()); + assertNull(task.getLockOwner()); + assertNotNull(task.getStatusDetails().get("crashTime")); + verify(executionManager).executeTask(same(task), same(resumeExecutor)); + assertEquals(1, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_CRASHED)); + assertEquals(1, metricsManager.getMetric(TaskMetricsManager.METRIC_TASKS_RESUMED)); + } + + @Test + public void testRecoverRunningExpiredRestartsWhenCannotResume() { + ScheduledTask task = TaskTestFixtures.runningTask("plain-type", "dead"); + task.setFailureCount(0); + task.setMaxRetries(3); + task.setLastExecutionDate(new Date()); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.singletonList(task)); + when(executorRegistry.getExecutor("plain-type")).thenReturn(plainExecutor); + + recoveryManager.recoverCrashedTasks(); + + assertEquals(ScheduledTask.TaskStatus.SCHEDULED, task.getStatus()); + assertNotNull(task.getNextScheduledExecution()); + verify(executionManager).executeTask(same(task), same(plainExecutor)); + } + + @Test + public void testRecoverSkipsRestartWhenRetryBudgetExhausted() { + ScheduledTask task = TaskTestFixtures.runningTask("plain-type", "dead"); + task.setFailureCount(4); + task.setMaxRetries(3); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.singletonList(task)); + when(executorRegistry.getExecutor("plain-type")).thenReturn(plainExecutor); + + recoveryManager.recoverCrashedTasks(); + + assertEquals(ScheduledTask.TaskStatus.CRASHED, task.getStatus()); + verify(executionManager, never()).executeTask(any(), any()); + } + + @Test + public void testRecoverLockedWaitingTaskReschedules() { + ScheduledTask waiting = TaskTestFixtures.baseTask("wait"); + waiting.setStatus(ScheduledTask.TaskStatus.WAITING); + waiting.setLockOwner("dead"); + waiting.setLockDate(new Date(System.currentTimeMillis() - 10_000)); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.emptyList()); + when(schedulerService.findLockedTasks()).thenReturn(Collections.singletonList(waiting)); + when(executorRegistry.getExecutor("wait")).thenReturn(plainExecutor); + when(lockManager.releaseLock(waiting)).thenAnswer(inv -> { + waiting.setLockOwner(null); + waiting.setLockDate(null); + return true; + }); + + recoveryManager.recoverCrashedTasks(); + + assertEquals(ScheduledTask.TaskStatus.SCHEDULED, waiting.getStatus()); + verify(executionManager).executeTask(same(waiting), same(plainExecutor)); + } + + @Test + public void testRecoverLockedSkipsRunningAndCrashed() { + ScheduledTask running = TaskTestFixtures.runningTask("r", "dead"); + ScheduledTask crashed = TaskTestFixtures.baseTask("c"); + crashed.setStatus(ScheduledTask.TaskStatus.CRASHED); + crashed.setLockOwner("dead"); + crashed.setLockDate(new Date(0)); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.emptyList()); + when(schedulerService.findLockedTasks()).thenReturn(Arrays.asList(running, crashed)); + + recoveryManager.recoverCrashedTasks(); + + verify(lockManager, never()).releaseLock(running); + verify(lockManager, never()).releaseLock(crashed); + } + + @Test + public void testRecoverDoesNotDispatchWhenSaveFails() { + ScheduledTask task = TaskTestFixtures.runningTask("plain-type", "dead"); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.singletonList(task)); + when(schedulerService.saveTask(any(ScheduledTask.class))).thenReturn(false); + when(schedulerService.saveTaskWithRefresh(any(ScheduledTask.class))).thenReturn(false); + when(executorRegistry.getExecutor("plain-type")).thenReturn(plainExecutor); + + recoveryManager.recoverCrashedTasks(); + + assertEquals(ScheduledTask.TaskStatus.CRASHED, task.getStatus()); + verify(executionManager, never()).executeTask(any(), any()); + } + + @Test + public void testRecoverDoesNotRestartDisabledTask() { + ScheduledTask task = TaskTestFixtures.runningTask("plain-type", "dead"); + task.setEnabled(false); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.singletonList(task)); + when(executorRegistry.getExecutor("plain-type")).thenReturn(plainExecutor); + + recoveryManager.recoverCrashedTasks(); + + assertEquals(ScheduledTask.TaskStatus.CRASHED, task.getStatus()); + verify(executionManager, never()).executeTask(any(), any()); + } + + @Test + public void testRecoverSkipsWhenNoExecutorRegistered() { + ScheduledTask task = TaskTestFixtures.runningTask("missing-type", "dead"); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.singletonList(task)); + when(executorRegistry.getExecutor("missing-type")).thenReturn(null); + + recoveryManager.recoverCrashedTasks(); + + assertEquals(ScheduledTask.TaskStatus.CRASHED, task.getStatus()); + verify(executionManager, never()).executeTask(any(), any()); + } + + @Test + public void testRecoverRestartsWhenFailureCountEqualsMaxRetries() { + ScheduledTask task = TaskTestFixtures.runningTask("plain-type", "dead"); + task.setFailureCount(3); + task.setMaxRetries(3); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.singletonList(task)); + when(executorRegistry.getExecutor("plain-type")).thenReturn(plainExecutor); + + recoveryManager.recoverCrashedTasks(); + + assertEquals(ScheduledTask.TaskStatus.SCHEDULED, task.getStatus()); + verify(executionManager).executeTask(same(task), same(plainExecutor)); + } + + @Test + public void testRecoverLockedWaitingKeepsWaitingWhenDepsUnmet() { + ScheduledTask waiting = TaskTestFixtures.baseTask("wait"); + waiting.setStatus(ScheduledTask.TaskStatus.WAITING); + waiting.setLockOwner("dead"); + waiting.setLockDate(new Date(System.currentTimeMillis() - 10_000)); + waiting.setDependsOn(new java.util.HashSet<>(Collections.singleton("dep-1"))); + ScheduledTask dep = TaskTestFixtures.baseTask("dep"); + dep.setItemId("dep-1"); + dep.setStatus(ScheduledTask.TaskStatus.RUNNING); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.emptyList()); + when(schedulerService.findLockedTasks()).thenReturn(Collections.singletonList(waiting)); + when(schedulerService.getTask("dep-1")).thenReturn(dep); + when(lockManager.releaseLock(waiting)).thenReturn(true); + + recoveryManager.recoverCrashedTasks(); + + assertEquals(ScheduledTask.TaskStatus.WAITING, waiting.getStatus()); + verify(executionManager, never()).executeTask(any(), any()); + } + + @Test + public void testRecoverAbortsIndividualTaskWhenShutdownFlips() { + ScheduledTask task = TaskTestFixtures.runningTask("plain-type", "dead"); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenAnswer(inv -> { + recoveryManager.prepareForShutdown(); + return Collections.singletonList(task); + }); + + recoveryManager.recoverCrashedTasks(); + + assertEquals(ScheduledTask.TaskStatus.RUNNING, task.getStatus()); + verify(executionManager, never()).executeTask(any(), any()); + verify(schedulerService, never()).saveTask(task); + } + + @Test + public void testRecoverLockedScheduledDispatchesExecute() { + ScheduledTask scheduled = TaskTestFixtures.baseTask("sched"); + scheduled.setStatus(ScheduledTask.TaskStatus.SCHEDULED); + scheduled.setLockOwner("dead"); + scheduled.setLockDate(new Date(System.currentTimeMillis() - 10_000)); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.emptyList()); + when(schedulerService.findLockedTasks()).thenReturn(Collections.singletonList(scheduled)); + when(executorRegistry.getExecutor("sched")).thenReturn(plainExecutor); + when(lockManager.releaseLock(scheduled)).thenReturn(true); + + recoveryManager.recoverCrashedTasks(); + + verify(lockManager).releaseLock(scheduled); + verify(executionManager).executeTask(same(scheduled), same(plainExecutor)); + } + + @Test + public void testNonExecutorMarksCrashedButDoesNotDispatch() { + recoveryManager.setExecutorNode(false); + ScheduledTask task = TaskTestFixtures.runningTask("plain-type", "dead"); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.singletonList(task)); + when(executorRegistry.getExecutor("plain-type")).thenReturn(plainExecutor); + + recoveryManager.recoverCrashedTasks(); + + assertEquals(ScheduledTask.TaskStatus.CRASHED, task.getStatus()); + verify(executionManager, never()).executeTask(any(), any()); + } + + @Test + public void testNonExecutorDispatchesRunOnAllNodesRecovery() { + recoveryManager.setExecutorNode(false); + ScheduledTask task = TaskTestFixtures.runningTask("plain-type", "dead"); + task.setRunOnAllNodes(true); + task.setAllowParallelExecution(true); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.singletonList(task)); + when(executorRegistry.getExecutor("plain-type")).thenReturn(plainExecutor); + + recoveryManager.recoverCrashedTasks(); + + verify(executionManager).executeTask(any(ScheduledTask.class), same(plainExecutor)); + } + + @Test + public void testRecoverCrashedCasConflictSkipsDispatch() { + ScheduledTask task = TaskTestFixtures.runningTask("plain-type", "dead"); + task.setSystemMetadata("seq_no", 1L); + task.setSystemMetadata("primary_term", 1L); + ScheduledTask storeView = TaskTestFixtures.runningTask("plain-type", "dead"); + storeView.setItemId(task.getItemId()); + storeView.setSystemMetadata("seq_no", 1L); + storeView.setSystemMetadata("primary_term", 1L); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.singletonList(task)); + when(schedulerService.getTask(eq(task.getItemId()), eq(true))).thenReturn(storeView); + when(schedulerService.saveTaskWithRefresh(any(ScheduledTask.class))).thenReturn(false); + when(executorRegistry.getExecutor("plain-type")).thenReturn(plainExecutor); + + recoveryManager.recoverCrashedTasks(); + + verify(executionManager, never()).executeTask(any(), any()); + } + + @Test + public void testRecoverSkipsWhenStoreNoLongerRunning() { + ScheduledTask task = TaskTestFixtures.runningTask("plain-type", "dead"); + ScheduledTask completed = TaskTestFixtures.baseTask("plain-type"); + completed.setItemId(task.getItemId()); + completed.setStatus(ScheduledTask.TaskStatus.COMPLETED); + when(schedulerService.findTasksByStatus(ScheduledTask.TaskStatus.RUNNING)) + .thenReturn(Collections.singletonList(task)); + when(schedulerService.getTask(eq(task.getItemId()), eq(true))).thenReturn(completed); + + recoveryManager.recoverCrashedTasks(); + + assertEquals(ScheduledTask.TaskStatus.RUNNING, task.getStatus()); + verify(executionManager, never()).executeTask(any(), any()); + } +} diff --git a/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskStateManagerTest.java b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskStateManagerTest.java new file mode 100644 index 0000000000..e2e80336ab --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskStateManagerTest.java @@ -0,0 +1,273 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.unomi.services.impl.scheduler; + +import org.apache.unomi.api.tasks.ScheduledTask; +import org.apache.unomi.api.tasks.ScheduledTask.TaskStatus; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.Collections; +import java.util.Date; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * Unit tests for {@link TaskStateManager}. + * Invoked by Surefire: {@code -Dtest=TaskStateManagerTest}. No data-file I/O. + * Glob: no prior TaskStateManagerTest. User asked for Task*Managers unit tests. + */ +public class TaskStateManagerTest { + + private TaskStateManager stateManager; + + @BeforeEach + public void setUp() { + stateManager = new TaskStateManager(); + } + + @Test + public void testValidTransitionsIncludeRunningToRunning() { + assertTrue(TaskStateManager.TaskTransition.isValidTransition(TaskStatus.RUNNING, TaskStatus.RUNNING)); + assertTrue(TaskStateManager.TaskTransition.isValidTransition(TaskStatus.SCHEDULED, TaskStatus.RUNNING)); + assertTrue(TaskStateManager.TaskTransition.isValidTransition(TaskStatus.RUNNING, TaskStatus.CRASHED)); + assertTrue(TaskStateManager.TaskTransition.isValidTransition(TaskStatus.COMPLETED, TaskStatus.SCHEDULED)); + } + + @Test + public void testCancelledToCrashedIsRejected() { + ScheduledTask task = TaskTestFixtures.baseTask("state"); + task.setStatus(TaskStatus.CANCELLED); + assertThrows(IllegalStateException.class, + () -> stateManager.updateTaskState(task, TaskStatus.CRASHED, "x", "node-1")); + } + + @Test + public void testUpdateToCompletedClearsLockAndSetsLastExecution() { + ScheduledTask task = TaskTestFixtures.runningTask("state", "node-1"); + task.setCurrentStep("step"); + stateManager.updateTaskState(task, TaskStatus.COMPLETED, null, "node-1"); + assertEquals(TaskStatus.COMPLETED, task.getStatus()); + assertNull(task.getLockOwner()); + assertNull(task.getLockDate()); + assertNull(task.getCurrentStep()); + assertNotNull(task.getLastExecutionDate()); + } + + @Test + public void testUpdateToCrashedPreservesCrashDetails() { + ScheduledTask task = TaskTestFixtures.runningTask("state", "dead-node"); + stateManager.updateTaskState(task, TaskStatus.CRASHED, "node died", "survivor"); + assertEquals(TaskStatus.CRASHED, task.getStatus()); + assertEquals("CRASHED", task.getCurrentStep()); + assertEquals("node died", task.getLastError()); + assertNotNull(task.getStatusDetails().get("crashTime")); + assertEquals("dead-node", task.getStatusDetails().get("crashedNode")); + } + + @Test + public void testUpdateToRunningSetsStatusDetails() { + ScheduledTask task = TaskTestFixtures.baseTask("state"); + stateManager.updateTaskState(task, TaskStatus.RUNNING, null, "node-1"); + assertEquals(TaskStatus.RUNNING, task.getStatus()); + assertEquals("node-1", task.getStatusDetails().get("executingNode")); + assertNotNull(task.getStatusDetails().get("startTime")); + } + + @Test + public void testCanRescheduleWithCompletedDependencies() { + ScheduledTask dep = TaskTestFixtures.baseTask("dep"); + dep.setStatus(TaskStatus.COMPLETED); + ScheduledTask task = TaskTestFixtures.baseTask("dependent"); + task.setDependsOn(new HashSet<>(Collections.singleton(dep.getItemId()))); + Map deps = new HashMap<>(); + deps.put(dep.getItemId(), dep); + assertTrue(stateManager.canRescheduleTask(task, deps)); + } + + @Test + public void testCanRescheduleFalseWhenDependencyMissingOrIncomplete() { + ScheduledTask task = TaskTestFixtures.baseTask("dependent"); + task.setDependsOn(new HashSet<>(Collections.singleton("missing"))); + assertFalse(stateManager.canRescheduleTask(task, new HashMap<>())); + + ScheduledTask dep = TaskTestFixtures.baseTask("dep"); + dep.setStatus(TaskStatus.RUNNING); + task.setDependsOn(new HashSet<>(Collections.singleton(dep.getItemId()))); + Map deps = Collections.singletonMap(dep.getItemId(), dep); + assertFalse(stateManager.canRescheduleTask(task, deps)); + } + + @Test + public void testCanReschedulePrefersWaitingOnTasks() { + ScheduledTask done = TaskTestFixtures.baseTask("done"); + done.setStatus(TaskStatus.COMPLETED); + ScheduledTask pending = TaskTestFixtures.baseTask("pending"); + pending.setStatus(TaskStatus.RUNNING); + + ScheduledTask task = TaskTestFixtures.baseTask("dependent"); + task.setDependsOn(new HashSet<>(Collections.singleton(done.getItemId()))); + task.setWaitingOnTasks(new HashSet<>(Collections.singleton(pending.getItemId()))); + + Map deps = new HashMap<>(); + deps.put(done.getItemId(), done); + deps.put(pending.getItemId(), pending); + assertFalse(stateManager.canRescheduleTask(task, deps)); + } + + @Test + public void testResetTaskToScheduledClearsWaiting() { + ScheduledTask task = TaskTestFixtures.baseTask("wait"); + task.setStatus(TaskStatus.WAITING); + task.setWaitingOnTasks(new HashSet<>(Collections.singleton("x"))); + task.setWaitingForTaskType("t"); + stateManager.resetTaskToScheduled(task); + assertEquals(TaskStatus.SCHEDULED, task.getStatus()); + assertNull(task.getWaitingOnTasks()); + assertNull(task.getWaitingForTaskType()); + } + + @Test + public void testCalculateNextExecutionRetryUsesMillisecondRetryDelay() { + ScheduledTask task = TaskTestFixtures.baseTask("retry"); + task.setRetryDelay(250); + long before = System.currentTimeMillis(); + stateManager.calculateNextExecutionTime(task, true); + long after = System.currentTimeMillis(); + long next = task.getNextScheduledExecution().getTime(); + assertTrue(next >= before + 250); + assertTrue(next <= after + 250); + } + + @Test + public void testCalculateNextExecutionOneShotSecondRunDisables() { + ScheduledTask task = TaskTestFixtures.baseTask("oneshot"); + task.setLastExecutionDate(new Date()); + stateManager.calculateNextExecutionTime(task, false); + assertNull(task.getNextScheduledExecution()); + assertFalse(task.isEnabled()); + } + + @Test + public void testCalculateNextExecutionFixedDelayFromNow() { + ScheduledTask task = TaskTestFixtures.periodicTask("periodic", 1000); + task.setLastExecutionDate(new Date()); + long before = System.currentTimeMillis(); + stateManager.calculateNextExecutionTime(task, false); + assertTrue(task.getNextScheduledExecution().getTime() >= before + 1000); + } + + @Test + public void testValidateTaskRejectsOneShotWithPeriod() { + ScheduledTask task = TaskTestFixtures.rawTask("bad"); + task.setOneShot(true); + TaskTestFixtures.setPeriodField(task, 5L); + task.setTimeUnit(TimeUnit.SECONDS); + assertThrows(IllegalArgumentException.class, + () -> stateManager.validateTask(task, new HashMap<>())); + } + + @Test + public void testCancelFromCompletedIsAllowed() { + ScheduledTask task = TaskTestFixtures.baseTask("cancel"); + task.setStatus(TaskStatus.COMPLETED); + assertDoesNotThrow(() -> stateManager.updateTaskState(task, TaskStatus.CANCELLED, null, "node-1")); + assertEquals(TaskStatus.CANCELLED, task.getStatus()); + } + + @Test + public void testUpdateToWaitingClearsLockInfo() { + ScheduledTask task = TaskTestFixtures.runningTask("wait", "node-1"); + stateManager.updateTaskState(task, TaskStatus.WAITING, null, "node-1"); + assertEquals(TaskStatus.WAITING, task.getStatus()); + assertNull(task.getLockOwner()); + assertNull(task.getLockDate()); + } + + @Test + public void testFixedRateCatchesUpPastIntervals() { + ScheduledTask task = TaskTestFixtures.periodicTask("fixed", 1000); + task.setFixedRate(true); + long now = System.currentTimeMillis(); + task.setLastExecutionDate(new Date(now - 3500)); + task.setNextScheduledExecution(new Date(now - 3500)); + stateManager.calculateNextExecutionTime(task, false); + assertTrue(task.getNextScheduledExecution().getTime() > now); + assertTrue(task.getNextScheduledExecution().getTime() <= now + 1000); + } + + @Test + public void testInitialDelayCreatesCreationDateWhenMissing() { + ScheduledTask task = TaskTestFixtures.baseTask("delay"); + task.setCreationDate(null); + task.setInitialDelay(200); + task.setTimeUnit(TimeUnit.MILLISECONDS); + long before = System.currentTimeMillis(); + stateManager.calculateNextExecutionTime(task, false); + assertNotNull(task.getCreationDate()); + assertTrue(task.getNextScheduledExecution().getTime() >= before + 200); + } + + @Test + public void testPeriodZeroDoesNotSetNextExecution() { + ScheduledTask task = TaskTestFixtures.rawTask("zero"); + task.setOneShot(false); + TaskTestFixtures.setPeriodField(task, 0L); + task.setLastExecutionDate(new Date()); + task.setNextScheduledExecution(null); + stateManager.calculateNextExecutionTime(task, false); + assertNull(task.getNextScheduledExecution()); + } + + @Test + public void testCanRescheduleFallsBackWhenWaitingOnEmpty() { + ScheduledTask dep = TaskTestFixtures.baseTask("dep"); + dep.setStatus(TaskStatus.COMPLETED); + ScheduledTask task = TaskTestFixtures.baseTask("dependent"); + task.setDependsOn(new HashSet<>(Collections.singleton(dep.getItemId()))); + task.setWaitingOnTasks(new HashSet<>()); + Map deps = new HashMap<>(); + deps.put(dep.getItemId(), dep); + assertTrue(stateManager.canRescheduleTask(task, deps)); + } + + @Test + public void testValidateTaskRejectsBlankDependencyAndNullTimeUnit() { + ScheduledTask blankDep = TaskTestFixtures.baseTask("blank"); + blankDep.setDependsOn(new HashSet<>(Collections.singleton(" "))); + assertThrows(IllegalArgumentException.class, + () -> stateManager.validateTask(blankDep, new HashMap<>())); + + ScheduledTask noUnit = TaskTestFixtures.rawTask("nounit"); + noUnit.setOneShot(false); + TaskTestFixtures.setPeriodField(noUnit, 5L); + noUnit.setTimeUnit(null); + assertThrows(IllegalArgumentException.class, + () -> stateManager.validateTask(noUnit, new HashMap<>())); + + ScheduledTask negRetries = TaskTestFixtures.baseTask("retries"); + negRetries.setMaxRetries(-1); + assertThrows(IllegalArgumentException.class, + () -> stateManager.validateTask(negRetries, new HashMap<>())); + } +} + diff --git a/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskTestFixtures.java b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskTestFixtures.java new file mode 100644 index 0000000000..a073780c9b --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskTestFixtures.java @@ -0,0 +1,132 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.services.impl.scheduler; + +import org.apache.unomi.api.tasks.ScheduledTask; + +import java.util.Date; +import java.util.UUID; +import java.util.concurrent.TimeUnit; + +/** + * Shared builders for scheduler manager unit tests. + * + * Callers (same package): TaskStateManagerTest, TaskValidationManagerTest, + * TaskHistoryManagerTest, TaskLockManagerTest, TaskRecoveryManagerTest, + * TaskExecutionManagerTest, PersistenceSchedulerProviderTest. + * + * No existing TaskTestFixtures (Glob confirmed 0 matches). No data-file I/O. + * + * User instruction: "Could you add unit tests for all the Task*Managers and for + * the SchedulerProvider implementations ?" + */ +final class TaskTestFixtures { + + private TaskTestFixtures() { + } + + static ScheduledTask baseTask(String type) { + ScheduledTask task = new ScheduledTask(); + task.setItemId(UUID.randomUUID().toString()); + task.setTaskType(type); + task.setStatus(ScheduledTask.TaskStatus.SCHEDULED); + task.setEnabled(true); + task.setPersistent(true); + task.setOneShot(true); + task.setAllowParallelExecution(false); + task.setMaxRetries(3); + task.setRetryDelay(500); + task.setCreationDate(new Date()); + task.setTimeUnit(TimeUnit.MILLISECONDS); + return task; + } + + static ScheduledTask runningTask(String type, String lockOwner) { + ScheduledTask task = baseTask(type); + task.setStatus(ScheduledTask.TaskStatus.RUNNING); + task.setLockOwner(lockOwner); + task.setLockDate(new Date(System.currentTimeMillis() - 10_000)); + task.setExecutingNodeId(lockOwner); + return task; + } + + static ScheduledTask rawTask(String type) { + ScheduledTask task = new ScheduledTask(); + task.setItemId(UUID.randomUUID().toString()); + task.setTaskType(type); + task.setStatus(ScheduledTask.TaskStatus.SCHEDULED); + task.setEnabled(true); + task.setPersistent(true); + task.setAllowParallelExecution(false); + task.setMaxRetries(3); + task.setRetryDelay(500); + task.setCreationDate(new Date()); + task.setTimeUnit(TimeUnit.MILLISECONDS); + task.setDependsOn(new java.util.HashSet<>()); + return task; + } + + static ScheduledTask periodicTask(String type, long periodMs) { + ScheduledTask task = baseTask(type); + task.setOneShot(false); + task.setPeriod(periodMs); + task.setTimeUnit(TimeUnit.MILLISECONDS); + task.setFixedRate(false); + return task; + } + + static void setPeriodField(ScheduledTask task, long period) { + try { + java.lang.reflect.Field field = ScheduledTask.class.getDeclaredField("period"); + field.setAccessible(true); + field.setLong(task, period); + } catch (ReflectiveOperationException e) { + throw new RuntimeException(e); + } + } + + /** + * Returns an itemId whose affinity primary is not {@code nodeId} for the given active set. + */ + static String itemIdNotPrimaryFor(String nodeId, java.util.List activeNodes) { + java.util.List sorted = new java.util.ArrayList<>(activeNodes); + java.util.Collections.sort(sorted); + for (int i = 0; i < 50_000; i++) { + String id = "affinity-" + i; + int primaryIndex = Math.abs(id.hashCode() % sorted.size()); + if (!sorted.get(primaryIndex).equals(nodeId)) { + return id; + } + } + throw new IllegalStateException("Could not find non-primary itemId for " + nodeId); + } + + /** + * Backup order of {@code nodeId} relative to the affinity primary of {@code itemId} + * (1 = first backup after primary). + */ + static int backupOrderFor(String itemId, String nodeId, java.util.List activeNodes) { + java.util.List sorted = new java.util.ArrayList<>(activeNodes); + java.util.Collections.sort(sorted); + int primaryIndex = Math.abs(itemId.hashCode() % sorted.size()); + int ourIndex = sorted.indexOf(nodeId); + if (ourIndex < 0) { + throw new IllegalArgumentException(nodeId + " not in active nodes"); + } + return (ourIndex - primaryIndex + sorted.size()) % sorted.size(); + } +} diff --git a/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskValidationManagerTest.java b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskValidationManagerTest.java new file mode 100644 index 0000000000..172b5bd38a --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/impl/scheduler/TaskValidationManagerTest.java @@ -0,0 +1,230 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.unomi.services.impl.scheduler; + +import org.apache.unomi.api.tasks.ScheduledTask; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * Unit tests for {@link TaskValidationManager}. + * Invoked by Surefire: {@code -Dtest=TaskValidationManagerTest}. No data-file I/O. + * Glob: no prior TaskValidationManagerTest. User asked for Task*Managers unit tests. + */ +public class TaskValidationManagerTest { + + private TaskValidationManager validationManager; + + @BeforeEach + public void setUp() { + validationManager = new TaskValidationManager(); + } + + @Test + public void testValidateTaskAcceptsValidOneShot() { + ScheduledTask task = TaskTestFixtures.baseTask("valid"); + assertDoesNotThrow(() -> validationManager.validateTask(task, new HashMap<>())); + } + + @Test + public void testRejectsNullOrEmptyTypeAndId() { + ScheduledTask task = TaskTestFixtures.baseTask("t"); + task.setTaskType(" "); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateTask(task, new HashMap<>())); + + ScheduledTask task2 = TaskTestFixtures.baseTask("t"); + task2.setItemId(""); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateTask(task2, new HashMap<>())); + } + + @Test + public void testRejectsNegativePeriodAndMissingTimeUnit() { + ScheduledTask task = TaskTestFixtures.rawTask("p"); + task.setOneShot(false); + setPeriodField(task, -1L); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateTask(task, new HashMap<>())); + + ScheduledTask delayed = TaskTestFixtures.rawTask("d"); + delayed.setOneShot(true); + delayed.setInitialDelay(5); + delayed.setTimeUnit(null); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateTask(delayed, new HashMap<>())); + } + + @Test + public void testRejectsOneShotWithPeriod() { + ScheduledTask task = TaskTestFixtures.rawTask("oneshot"); + task.setOneShot(true); + setPeriodField(task, 10L); + task.setTimeUnit(TimeUnit.SECONDS); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateTask(task, new HashMap<>())); + } + + private static void setPeriodField(ScheduledTask task, long period) { + TaskTestFixtures.setPeriodField(task, period); + } + + @Test + public void testRejectsMissingDependency() { + ScheduledTask task = TaskTestFixtures.baseTask("dep"); + task.setDependsOn(new HashSet<>(Collections.singleton("missing-id"))); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateTask(task, new HashMap<>())); + } + + @Test + public void testRejectsCircularDependency() { + ScheduledTask a = TaskTestFixtures.baseTask("a"); + ScheduledTask b = TaskTestFixtures.baseTask("b"); + a.setDependsOn(new HashSet<>(Collections.singleton(b.getItemId()))); + b.setDependsOn(new HashSet<>(Collections.singleton(a.getItemId()))); + Map existing = new HashMap<>(); + existing.put(a.getItemId(), a); + existing.put(b.getItemId(), b); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateTask(a, existing)); + } + + @Test + public void testRejectsRunOnAllNodesWithDisallowParallel() { + ScheduledTask task = TaskTestFixtures.periodicTask("all", 1000); + task.setRunOnAllNodes(true); + task.setAllowParallelExecution(false); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateTask(task, new HashMap<>())); + } + + @Test + public void testRejectsOneShotRunOnAllNodes() { + ScheduledTask task = TaskTestFixtures.baseTask("all"); + task.setRunOnAllNodes(true); + task.setAllowParallelExecution(true); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateTask(task, new HashMap<>())); + } + + @Test + public void testValidateStateTransitionStricterThanStateManager() { + ScheduledTask task = TaskTestFixtures.baseTask("v"); + task.setStatus(ScheduledTask.TaskStatus.SCHEDULED); + assertThrows(IllegalStateException.class, + () -> validationManager.validateStateTransition(task, ScheduledTask.TaskStatus.CANCELLED)); + assertDoesNotThrow( + () -> validationManager.validateStateTransition(task, ScheduledTask.TaskStatus.RUNNING)); + } + + @Test + public void testValidateExecutionPrerequisites() { + ScheduledTask task = TaskTestFixtures.baseTask("exec"); + assertDoesNotThrow(() -> validationManager.validateExecutionPrerequisites(task, "node-1")); + + task.setEnabled(false); + assertThrows(IllegalStateException.class, + () -> validationManager.validateExecutionPrerequisites(task, "node-1")); + + task.setEnabled(true); + task.setStatus(ScheduledTask.TaskStatus.COMPLETED); + assertThrows(IllegalStateException.class, + () -> validationManager.validateExecutionPrerequisites(task, "node-1")); + } + + @Test + public void testValidateExecutionPrerequisitesRejectsWrongLockOwner() { + ScheduledTask task = TaskTestFixtures.baseTask("exec"); + task.setLockOwner("other-node"); + assertThrows(IllegalStateException.class, + () -> validationManager.validateExecutionPrerequisites(task, "node-1")); + } + + @Test + public void testValidateRetryConfigurationRejectsNegatives() { + ScheduledTask task = TaskTestFixtures.baseTask("retry"); + task.setMaxRetries(-1); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateRetryConfiguration(task)); + task.setMaxRetries(1); + task.setRetryDelay(-5); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateRetryConfiguration(task)); + } + + @Test + public void testRejectsSelfAndTransitiveDependencyCycles() { + ScheduledTask self = TaskTestFixtures.baseTask("self"); + self.setDependsOn(new HashSet<>(Collections.singleton(self.getItemId()))); + Map existing = new HashMap<>(); + existing.put(self.getItemId(), self); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateTask(self, existing)); + + ScheduledTask a = TaskTestFixtures.baseTask("a"); + ScheduledTask b = TaskTestFixtures.baseTask("b"); + ScheduledTask c = TaskTestFixtures.baseTask("c"); + a.setDependsOn(new HashSet<>(Collections.singleton(b.getItemId()))); + b.setDependsOn(new HashSet<>(Collections.singleton(c.getItemId()))); + c.setDependsOn(new HashSet<>(Collections.singleton(a.getItemId()))); + Map cycle = new HashMap<>(); + cycle.put(a.getItemId(), a); + cycle.put(b.getItemId(), b); + cycle.put(c.getItemId(), c); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateTask(a, cycle)); + } + + @Test + public void testExecutionPrerequisitesAllowsWrongOwnerWhenRunOnAllNodes() { + ScheduledTask task = TaskTestFixtures.periodicTask("all", 1000); + task.setRunOnAllNodes(true); + task.setAllowParallelExecution(true); + task.setLockOwner("other-node"); + task.setStatus(ScheduledTask.TaskStatus.WAITING); + assertDoesNotThrow(() -> validationManager.validateExecutionPrerequisites(task, "node-1")); + + task.setStatus(ScheduledTask.TaskStatus.CRASHED); + assertDoesNotThrow(() -> validationManager.validateExecutionPrerequisites(task, "node-1")); + } + + @Test + public void testRejectsNegativeInitialDelay() { + ScheduledTask task = TaskTestFixtures.baseTask("delay"); + task.setInitialDelay(-1); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateTask(task, new HashMap<>())); + } + + @Test + public void testRejectsBlankDependencyId() { + ScheduledTask task = TaskTestFixtures.baseTask("dep"); + task.setDependsOn(new HashSet<>(Collections.singleton(""))); + assertThrows(IllegalArgumentException.class, + () -> validationManager.validateTask(task, new HashMap<>())); + } +} diff --git a/services/src/test/java/org/apache/unomi/services/impl/segments/SegmentServiceImplTest.java b/services/src/test/java/org/apache/unomi/services/impl/segments/SegmentServiceImplTest.java index 852fd8a3fd..c7564fa9d4 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/segments/SegmentServiceImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/segments/SegmentServiceImplTest.java @@ -881,6 +881,7 @@ public void testCustomEventConditionTypeWithBooleanParent() { Event event = createTestEvent(profile, "view"); event.setProperty("testProperty", "test"); eventService.send(event); + persistenceService.refresh(); // handles refresh delay, see sibling tests in this file // Force recalculation segmentService.recalculatePastEventConditions();