Serge Huber created UNOMI-979:
---------------------------------
Summary: Scheduler lock-lease fix + test deflake
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
Fix For: unomi-3.1.0
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)
