[
https://issues.apache.org/jira/browse/UNOMI-979?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Serge Huber updated UNOMI-979:
------------------------------
Summary: Scheduled tasks can run twice at the same time when cluster nodes
disagree on the lock timeout (was: Scheduler lock-lease fix + test deflake)
> Scheduled tasks can run twice at the same time when cluster nodes disagree on
> the lock timeout
> ----------------------------------------------------------------------------------------------
>
> Key: UNOMI-979
> URL: https://issues.apache.org/jira/browse/UNOMI-979
> Project: Apache Unomi
> Issue Type: Improvement
> Components: unomi(-core)
> Affects Versions: unomi-3.1.0
> Reporter: Serge Huber
> Assignee: Serge Huber
> Priority: Major
> Fix For: unomi-3.1.0
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> A scheduler lock's renewal cadence is derived from its **owner's** configured
> lock timeout
> (`lockTimeout/3`, see `TaskExecutionManager#startLockRenewal`), but
> `TaskLockManager#isLockExpired`
> judged expiry against the **observer's** timeout. Any node whose configured
> timeout is shorter than a
> peer's renewal cadence therefore sees every renewal gap as an expired lock:
> its recovery pass marks
> the live execution CRASHED, clears the lock, and the next peer tick
> re-dispatches the task while the
> original execution is still running — the task runs twice.
> Reproduced deterministically (1s-timeout observer vs 10s-timeout workers;
> `LOCK-DIAG` traces show the
> watchdog's crash-mark at a 2.9s lock age followed by a second node
> dispatching while the first still
> executes). In production the same double execution follows from configuration
> drift between nodes or
> a rolling upgrade that changes `lockTimeout`.
> This was also the root cause of the sporadic scheduler CI failures on loaded
> runners:
> - `SchedulerServiceImplTest.testConcurrentLockAcquisition` — "Should have
> exactly one task executing at a time ==> expected: <1> but was: <2>"
> - `SchedulerServiceImplTest.testClusteringSupport` — "runOnAllNodes task
> should execute on every node including non-executors"
> (the test harness's `setUp()` scheduler ran with a 1s timeout alongside
> 10s-timeout test nodes —
> i.e. exactly the divergent configuration above, plus over-asserting tests;
> see below).
> ### The fix
> Record the lease with the lock: `ScheduledTask.lockLeaseMillis`, stamped from
> the owner's timeout on
> every acquire and renewal, cleared on release. `isLockExpired()` judges
> against the recorded lease.
> Compatibility: documents written before lease recording (or with a corrupt
> negative lease) carry no
> usable lease and fall back to the observer's timeout — the exact pre-change
> behaviour — so mixed
> clusters and existing locks behave identically. ES and OpenSearch
> `scheduledTask` mappings gain the
> field. `ScheduledTask` is now annotated
> `@JsonIgnoreProperties(ignoreUnknown=true)` so an older node
> can still read task documents written by a newer one during a rolling upgrade
> (Jackson's default
> rejects the first unknown field — a latent break for ANY future field
> addition, not just this one).
> `startLockRenewal` now warns when `lockTimeout` is at or below the minimum
> renewal interval — the one
> configuration where a node cannot keep its own lease alive.
> Same-config clusters (every normal deployment) are bit-for-bit unchanged:
> lease == every observer's
> timeout. Recovery of genuinely dead nodes still works and is now driven by
> the dead owner's lease —
> faster than before when the dead node ran a short timeout.
> ### Test hardening (same PR, second commit)
> - `setUp()` scheduler now uses the production-default lock timeout;
> multi-node tests that don't use it stop it first.
> - `testClusteringSupport` no longer demands one `runOnAllNodes` task execute
> on all three nodes — not a property the
> implementation promises (all nodes share the task's single schedule
> document; each period has one phase-dependent
> winner; no fairness). The regression it protected — non-executor nodes poll
> and run `runOnAllNodes` tasks — is pinned
> deterministically in a new single-node test.
> - Exact execution counts on still-firing periodic tasks → cancel-and-quiesce
> or lower bounds.
> - `Thread.sleep` policy applied and documented: positive assertions never
> wait on fixed sleeps (bounded polls /
> Mockito `timeout()` / test-released latches); deliberate quiet windows for
> negative assertions keep their sleeps.
> - Dead `configureDebugLogging()` removed (logback ignores slf4j-simple
> properties); `-DTEST_LOG_LEVEL=DEBUG` documented.
> ### Validation
> - New end-to-end regression tests are mutation-validated: the steal test
> fails with the expiry logic reverted
> ("expected: <RUNNING> but was: <CRASHED>"); the serialization
> forward-compat test fails without the annotation
> (`UnrecognizedPropertyException`).
> - Four scheduler suites (125 tests) run repeatedly under ~2.5× CPU
> oversubscription — the protocol that reproduced
> both original CI failures on demand — seven consecutive green runs.
> - Full `services` module: 805 tests green; ES/OS core, `rest`, `itests`
> compile clean.
> ### Follow-up worth its own ticket
> `runOnAllNodes` semantics: as implemented it means "any node, including
> non-executors, may run the task" (single shared
> schedule, one winner per period), not "each node runs it every period" as the
> name suggests. Worth deciding which
> semantic is intended and either implementing per-node schedules or
> renaming/documenting.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
