From 28a22323cd3ee9534f850f6aa5e8d8e17a4a550f Mon Sep 17 00:00:00 2001 From: Helmut Michael Juskewycz Date: Tue, 11 Aug 2026 20:25:54 +0200 Subject: [PATCH 1/6] fix(store-postgres): commit self-owned writes explicitly Refs #111. --- AGENTS.md | 2 + CHANGELOG.md | 6 + docs/transactions.md | 7 +- threadmill-store-postgres/README.md | 5 +- .../store/postgres/PostgresJobStore.java | 235 ++++++++---------- .../PostgresJobStoreRegressionTest.java | 83 +++++++ 6 files changed, 209 insertions(+), 129 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9d980e9..40b3268 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -236,6 +236,7 @@ This section is the project's memory: the load-bearing decisions worth knowing b - **Per-state counts come from `threadmill_job_counts`, maintained by an AFTER INSERT/UPDATE/DELETE trigger — and the counter rows are sharded (V2).** One row per state serialized every concurrent writer on that row's lock: a 16-producer stress run collapsed to ~13 jobs/s with `pg_stat_activity` full of `Lock:transactionid` waits on the counter updates (claims hold the row for their whole long transaction). Each state now has 16 shard rows; the trigger updates the shard picked by `pg_backend_pid() % 16`, and `countsByState()` reads `SUM(count) GROUP BY state`. Individual shard rows may legitimately go negative — only the sum is meaningful. Same stress shape after sharding: ~162 jobs/s. Never write a `COUNT(*)`-over-`threadmill_jobs` query; the `perStateCountsReadFromCounterTableNotFromJobsTable` test uses `EXPLAIN` to assert the plan does not touch the jobs table. - **Migration runner bootstraps `threadmill_schema_history` itself** (`CREATE IF NOT EXISTS`). Migration SQL must not recreate it. Add new migrations as `V__.sql` under `src/main/resources/com/hemju/threadmill/store/postgres/migrations/` and register them in `MigrationRunner.SHIPPED_MIGRATIONS` (the runner does not classpath-scan; the explicit list is intentional for native-image compatibility). - **Deadlocks on busy queue tables are normal.** Every write goes through `DeadlockRetry.run(...)` (recognises SQLSTATE `40P01` / `40001`, exponential backoff with jitter). +- **Self-owned Postgres writes never rely on the pool's `autoCommit` default.** Operations that open their own connection route through `PostgresJobStore.ownedTransaction`, which starts, commits or rolls back, and restores the connection's prior mode. This applies even to single-statement heartbeat, lease, mutex, queue-pause, retention, recurring-definition, and check-in writes: a pool configured with `autoCommit=false` otherwise accepts the statement and silently rolls it back on release. Helpers that receive a caller's `Connection` stay boundary-agnostic; their caller owns the transaction. - **Testcontainers ≥ 2.0.** Module names use the `testcontainers-` prefix. `PostgreSQLContainer` lives in `org.testcontainers.postgresql` and is non-generic. - **The host owns the connection pool.** The store accepts a `javax.sql.DataSource`; it does not create or close one. - **Spring Boot Postgres schema handling is explicit.** Auto-configured Postgres stores run `threadmill.store.postgres.schema-mode=migrate` by default before constructing `PostgresJobStore`. `validate` is for externally-applied DDL, `none` skips schema handling, and `drop-and-migrate` requires `threadmill.store.postgres.allow-destructive-schema-reset=true` because it destroys Threadmill job data. @@ -490,6 +491,7 @@ Every hard-won failure mode that has come up during development, and the test th | Timestamp used as the nudge CAS identity erases a newer same-instant acceptance (Redis millis collide); materializer acts on a task definition listed before its mutex (stale handler/payload materialized, new definition's nudge consumed; disable raced past); a failed clear loses the run instead of over-delivering | `AbstractJobStoreContractTest.nudgeAcceptancesWithIdenticalTimestampsAreDistinguishable` + `SchedulingTest.materializerReloadsTheDefinitionUnderTheTaskMutexBeforeActing` + `materializerRechecksEnabledUnderTheTaskMutexBeforeActing` + `aFailedNudgeClearProducesAnExtraRunNeverALostOne` | | Nudge origin invisible to read-level dashboard users (metadata redaction) or unbounded metric tag cardinality from user-controlled origin metadata | `DashboardApiServiceTest.redactedJobSummariesStillCarryTheCronOrigin` + `ThreadmillMetricsTest.recurringRunsCounterTagsTheTriggerOriginWithBoundedCardinality` | | Spring nudge deferred to afterCommit still routed through the joining store boundary — Spring has committed but not unbound its resources, so the write runs in a fresh transaction on the caller's connection that nobody commits (silently rolled back on an `autoCommit=false` pool such as Hikari with JPA defaults) | `SpringPostgresTransactionBoundaryTest.nudgeIsCommittedEvenWhenThePoolHandsOutNonAutoCommitConnections` (fails against the joining routing, passes with `PostgresJobStore.ownedTransaction`) | +| Postgres self-owned single-statement writes silently roll back when the pool hands out `autoCommit=false` connections (issue #111) | `PostgresJobStoreRegressionTest.selfOwnedWritesCommitWhenConnectionsDefaultToNonAutoCommit` | | Per-transaction nudge storm: the documented "nudge once per work item" pattern registering one synchronisation and one store round trip per call; a `REQUIRES_NEW` inner transaction inheriting the outer's batch through a bound resource | `SpringPostgresTransactionBoundaryTest.repeatedNudgesOfOneTaskInATransactionCollapseToASingleWrite` (batch scoped to the suspend-aware synchronisation list, not `bindResource`) | | Recurring-runs meter counting retry attempts instead of instances, inflating the nudge-versus-schedule ratio operators diagnose with; a non-flip dashboard edit clearing a pending nudge | `ThreadmillMetricsTest.recurringRunsCounterCountsInstancesNotRetryAttempts` + `DashboardApiServiceTest.updateRecurringWithoutAnEnabledFlipLeavesAPendingNudgeIntact` | | Nudge guarantees hold in unit tests but break under sustained load, node churn, or master handover (no soak coverage of the wake-driven path at all) | `nudge-pump` soak scenario + `InvariantChecks.nudgeRunAfterWake` / `outboxDrainedByLaterRun` (red paths: `InvariantViolationTest.nudgeRunAfterWakeFiresWhenNoRunStartsAfterTheNudge` + `nudgeRunAfterWakeFiresWhenTheNudgeGoesUnservedTooLong` + `outboxDrainedByLaterRunFiresWhenARowSurvivesAWholeRun`; green paths: `nudgeRunAfterWakeAcceptsARunThatStartsAfterTheNudge` + `outboxDrainedByLaterRunToleratesRowsAppendedAfterTheRunStarted` + `NudgePumpSmokeTest`) | diff --git a/CHANGELOG.md b/CHANGELOG.md index 04fe4d4..e270cc7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +- Fixed Postgres self-owned writes being silently rolled back when the host + `DataSource` hands out connections with `autoCommit=false` (issue #111). + Queue pauses, execution and node heartbeats, maintenance leases, retention, + mutexes, recurring definitions/state, and the expired-dedup fallback now use + explicit Threadmill-owned transactions, restoring the connection's prior + auto-commit mode after commit or rollback. - Recurring tasks can declare claim-time exclusivity (issue #110). A `CronTask` gains an `exclusive` flag, surfaced as `@Recurring(exclusive = true)` and as an `exclusive` parameter on diff --git a/docs/transactions.md b/docs/transactions.md index 5847cc0..61862bf 100644 --- a/docs/transactions.md +++ b/docs/transactions.md @@ -258,9 +258,10 @@ closing the crash window between the caller's commit and the nudge write, and that window is an explicit non-goal: the backstop schedule bounds the worst-case latency, and a lost nudge costs one schedule period, never a run. -On the hot path the write is a microseconds-held autocommit update, and an -in-JVM per-task coalescer additionally bounds the store write rate to about -one round trip regardless of producer rate. +On the hot path the write is a short Threadmill-owned transaction that commits +independently of the pool's `autoCommit` default. An in-JVM per-task coalescer +additionally bounds the store write rate to about one round trip regardless of +producer rate. ## Connection-pool sharing diff --git a/threadmill-store-postgres/README.md b/threadmill-store-postgres/README.md index 0c41e4a..53eb948 100644 --- a/threadmill-store-postgres/README.md +++ b/threadmill-store-postgres/README.md @@ -145,7 +145,10 @@ is not a production upgrade strategy. ## Connection pool The host owns the pool. `PostgresJobStore` accepts a `javax.sql.DataSource`; -it does not create or close one. Recommended floor for the pool size: +it does not create or close one. Threadmill does not require the pool to default +to `autoCommit=true`: every self-owned write uses an explicit transaction and +restores the connection's previous mode before returning it. Recommended floor +for the pool size: `workerCount + claimBatchSize + headroom` so claim and maintenance never contend with handler-side queries. diff --git a/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/PostgresJobStore.java b/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/PostgresJobStore.java index 54e2187..58c980b 100644 --- a/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/PostgresJobStore.java +++ b/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/PostgresJobStore.java @@ -66,7 +66,8 @@ * contending workers never collide and never wait. *
  • Every write is wrapped in {@link DeadlockRetry} — deadlocks on a * busy queue table are normal and the right response is bounded retry - * with jittered backoff.
    • + * with jittered backoff. Self-owned writes use an explicit transaction, + * independent of the {@link DataSource}'s default auto-commit mode. *
  • Per-state counts come from {@code threadmill_job_counts}, maintained * row-by-row by a trigger; a naive {@code COUNT(*)} would contend * with the claim path.
    • @@ -85,7 +86,7 @@ public final class PostgresJobStore implements JobStore { private final DataSource dataSource; private final PostgresTransactionBoundary transactionBoundary; - /** Boundary for writes that must never join an external transaction; see {@link #ownedTransaction}. */ + /** Boundary for self-owned writes that must never join an external transaction. */ private final PostgresTransactionBoundary owningBoundary; private final JobSerializer serializer; @@ -1084,12 +1085,11 @@ private record WorkflowKey(String concurrencyKey, UUID workflowRootId) {} public void pauseQueue(String queue, String reason) { Names.requireName("queue", queue); try { - DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - PreparedStatement ps = conn.prepareStatement("INSERT INTO threadmill_queue_pauses " - + "(queue, paused_at, paused_by) VALUES (?, ?, ?) " - + "ON CONFLICT (queue) DO UPDATE SET paused_at = EXCLUDED.paused_at, " - + "paused_by = EXCLUDED.paused_by")) { + ownedTransaction(conn -> { + try (PreparedStatement ps = conn.prepareStatement("INSERT INTO threadmill_queue_pauses " + + "(queue, paused_at, paused_by) VALUES (?, ?, ?) " + + "ON CONFLICT (queue) DO UPDATE SET paused_at = EXCLUDED.paused_at, " + + "paused_by = EXCLUDED.paused_by")) { ps.setString(1, queue); ps.setTimestamp(2, Timestamp.from(Instant.now())); ps.setString(3, reason); @@ -1106,10 +1106,9 @@ public void pauseQueue(String queue, String reason) { public void resumeQueue(String queue) { Names.requireName("queue", queue); try { - DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - PreparedStatement ps = - conn.prepareStatement("DELETE FROM threadmill_queue_pauses WHERE queue = ?")) { + ownedTransaction(conn -> { + try (PreparedStatement ps = + conn.prepareStatement("DELETE FROM threadmill_queue_pauses WHERE queue = ?")) { ps.setString(1, queue); ps.executeUpdate(); return null; @@ -1157,11 +1156,9 @@ private boolean isQueuePaused(String queue) { @Override public void touchOwnerHeartbeat(NodeId nodeId, Instant now) { try { - DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - PreparedStatement ps = - conn.prepareStatement("UPDATE threadmill_jobs SET owner_heartbeat_at = ? " - + "WHERE state = 'PROCESSING' AND owner_node_id = ?")) { + ownedTransaction(conn -> { + try (PreparedStatement ps = conn.prepareStatement("UPDATE threadmill_jobs SET owner_heartbeat_at = ? " + + "WHERE state = 'PROCESSING' AND owner_node_id = ?")) { ps.setTimestamp(1, Timestamp.from(now)); ps.setObject(2, nodeId.asUuid()); ps.executeUpdate(); @@ -1180,16 +1177,15 @@ public boolean saveExecutionUpdate(Job job, NodeId nodeId) { JobSnapshot snapshot = withVersion(job, job.version()); String body = serializer.serializeJob(snapshot, capabilities); try { - return DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - // The version guard rejects a zombie flush from a previous - // attempt: a claim bumps version while a check-in does not, - // so an attempt-N flush whose job was orphan-reclaimed, - // retried, and re-claimed (as attempt N+1) by the same node - // no longer matches the live row's version and is dropped. - PreparedStatement ps = conn.prepareStatement("UPDATE threadmill_jobs SET " - + "owner_heartbeat_at = ?, last_checkin_at = ?, body = ? " - + "WHERE id = ? AND state = 'PROCESSING' AND owner_node_id = ? AND version = ?")) { + return ownedTransaction(conn -> { + // The version guard rejects a zombie flush from a previous + // attempt: a claim bumps version while a check-in does not, + // so an attempt-N flush whose job was orphan-reclaimed, + // retried, and re-claimed (as attempt N+1) by the same node + // no longer matches the live row's version and is dropped. + try (PreparedStatement ps = conn.prepareStatement("UPDATE threadmill_jobs SET " + + "owner_heartbeat_at = ?, last_checkin_at = ?, body = ? " + + "WHERE id = ? AND state = 'PROCESSING' AND owner_node_id = ? AND version = ?")) { Instant heartbeat = snapshot.lastCheckinAt() == null ? snapshot.ownerHeartbeatAt() : snapshot.lastCheckinAt(); setNullableTimestamp(ps, 1, heartbeat); @@ -1209,11 +1205,10 @@ public boolean saveExecutionUpdate(Job job, NodeId nodeId) { @Override public void recordNodeHeartbeat(NodeId nodeId, Instant now) { try { - DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - PreparedStatement ps = conn.prepareStatement( - "INSERT INTO threadmill_nodes (id, last_heartbeat_at) VALUES (?, ?) " - + "ON CONFLICT (id) DO UPDATE SET last_heartbeat_at = EXCLUDED.last_heartbeat_at")) { + ownedTransaction(conn -> { + try (PreparedStatement ps = + conn.prepareStatement("INSERT INTO threadmill_nodes (id, last_heartbeat_at) VALUES (?, ?) " + + "ON CONFLICT (id) DO UPDATE SET last_heartbeat_at = EXCLUDED.last_heartbeat_at")) { ps.setObject(1, nodeId.asUuid()); ps.setTimestamp(2, Timestamp.from(now)); ps.executeUpdate(); @@ -1245,15 +1240,14 @@ public boolean acquireOrRenewMaintenanceLease(NodeId nodeId, Duration leaseDurat Objects.requireNonNull(nodeId, "nodeId"); Mutexes.requirePositive(leaseDuration); try { - return DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - PreparedStatement ps = - conn.prepareStatement("INSERT INTO threadmill_leases (name, holder, expires_at) " - + "VALUES (?, ?, clock_timestamp() + (? * interval '1 millisecond')) " - + "ON CONFLICT (name) DO UPDATE " - + "SET holder = EXCLUDED.holder, expires_at = EXCLUDED.expires_at " - + "WHERE threadmill_leases.holder = EXCLUDED.holder " - + "OR threadmill_leases.expires_at <= clock_timestamp()")) { + return ownedTransaction(conn -> { + try (PreparedStatement ps = + conn.prepareStatement("INSERT INTO threadmill_leases (name, holder, expires_at) " + + "VALUES (?, ?, clock_timestamp() + (? * interval '1 millisecond')) " + + "ON CONFLICT (name) DO UPDATE " + + "SET holder = EXCLUDED.holder, expires_at = EXCLUDED.expires_at " + + "WHERE threadmill_leases.holder = EXCLUDED.holder " + + "OR threadmill_leases.expires_at <= clock_timestamp()")) { ps.setString(1, MAINTENANCE_LEASE); ps.setObject(2, nodeId.asUuid()); ps.setLong(3, leaseDuration.toMillis()); @@ -1269,10 +1263,9 @@ public boolean acquireOrRenewMaintenanceLease(NodeId nodeId, Duration leaseDurat public void releaseMaintenanceLease(NodeId nodeId) { Objects.requireNonNull(nodeId, "nodeId"); try { - DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - PreparedStatement ps = - conn.prepareStatement("DELETE FROM threadmill_leases WHERE name = ? AND holder = ?")) { + ownedTransaction(conn -> { + try (PreparedStatement ps = + conn.prepareStatement("DELETE FROM threadmill_leases WHERE name = ? AND holder = ?")) { ps.setString(1, MAINTENANCE_LEASE); ps.setObject(2, nodeId.asUuid()); ps.executeUpdate(); @@ -1458,10 +1451,9 @@ public List listNodeHeartbeats() { public long deleteNodeHeartbeatsOlderThan(Instant cutoff) { Objects.requireNonNull(cutoff, "cutoff"); try { - return DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - PreparedStatement ps = - conn.prepareStatement("DELETE FROM threadmill_nodes WHERE last_heartbeat_at <= ?")) { + return ownedTransaction(conn -> { + try (PreparedStatement ps = + conn.prepareStatement("DELETE FROM threadmill_nodes WHERE last_heartbeat_at <= ?")) { ps.setTimestamp(1, Timestamp.from(cutoff)); return (long) ps.executeUpdate(); } @@ -1476,14 +1468,13 @@ public long deleteExpiredDedupKeys(Instant now, int max) { Objects.requireNonNull(now, "now"); if (max <= 0) return 0L; try { - return DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - PreparedStatement ps = - conn.prepareStatement("DELETE FROM threadmill_dedup_keys d WHERE d.ctid IN (" - + "SELECT d2.ctid FROM threadmill_dedup_keys d2 " - + "LEFT JOIN threadmill_jobs j ON j.id = d2.job_id " - + "WHERE d2.expires_at <= ? AND (j.id IS NULL OR j.state IN ('SUCCEEDED','FAILED','DELETED','QUARANTINED')) " - + "LIMIT ?)")) { + return ownedTransaction(conn -> { + try (PreparedStatement ps = + conn.prepareStatement("DELETE FROM threadmill_dedup_keys d WHERE d.ctid IN (" + + "SELECT d2.ctid FROM threadmill_dedup_keys d2 " + + "LEFT JOIN threadmill_jobs j ON j.id = d2.job_id " + + "WHERE d2.expires_at <= ? AND (j.id IS NULL OR j.state IN ('SUCCEEDED','FAILED','DELETED','QUARANTINED')) " + + "LIMIT ?)")) { ps.setTimestamp(1, Timestamp.from(now)); ps.setInt(2, max); return (long) ps.executeUpdate(); @@ -1508,19 +1499,18 @@ public List findByHandlerSignature(String handlerType, int max) { @Override public long deleteFinishedOlderThan(Instant cutoff, JobState state, int max) { try { - return DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - // Skip a terminal job that still has an unexpired dedup - // row: the FK is ON DELETE CASCADE, so deleting it here - // would drop a live dedup key and silently cap the dedup - // TTL at the retention age. Keep the job until its dedup - // expires; the next sweep then removes both. - PreparedStatement ps = conn.prepareStatement( - "DELETE FROM threadmill_jobs WHERE id IN (" + "SELECT j.id FROM threadmill_jobs j " - + "WHERE j.state = ? AND j.current_state_at <= ? " - + "AND NOT EXISTS (SELECT 1 FROM threadmill_dedup_keys d " - + "WHERE d.job_id = j.id AND d.expires_at > clock_timestamp()) " - + "LIMIT ?)")) { + return ownedTransaction(conn -> { + // Skip a terminal job that still has an unexpired dedup + // row: the FK is ON DELETE CASCADE, so deleting it here + // would drop a live dedup key and silently cap the dedup + // TTL at the retention age. Keep the job until its dedup + // expires; the next sweep then removes both. + try (PreparedStatement ps = conn.prepareStatement( + "DELETE FROM threadmill_jobs WHERE id IN (" + "SELECT j.id FROM threadmill_jobs j " + + "WHERE j.state = ? AND j.current_state_at <= ? " + + "AND NOT EXISTS (SELECT 1 FROM threadmill_dedup_keys d " + + "WHERE d.job_id = j.id AND d.expires_at > clock_timestamp()) " + + "LIMIT ?)")) { ps.setString(1, state.name()); ps.setTimestamp(2, Timestamp.from(cutoff)); ps.setInt(3, Math.max(0, max)); @@ -1562,18 +1552,17 @@ public boolean tryAcquireMutex(String name, String holder, Duration leaseDuratio Objects.requireNonNull(holder, "holder"); Mutexes.requirePositive(leaseDuration); try { - return DeadlockRetry.run(() -> { + return ownedTransaction(conn -> { // Lease expiry uses server-side time (clock_timestamp()) for // both write and compare, like the maintenance lease: a node // whose clock runs ahead must not be able to steal a mutex // whose lease is unexpired by server time. - try (Connection conn = dataSource.getConnection(); - PreparedStatement ps = conn.prepareStatement("INSERT INTO threadmill_mutexes " - + "(name, holder, expires_at) " - + "VALUES (?, ?, clock_timestamp() + (? * interval '1 millisecond')) " - + "ON CONFLICT (name) DO UPDATE SET holder = EXCLUDED.holder, expires_at = EXCLUDED.expires_at " - + "WHERE threadmill_mutexes.expires_at <= clock_timestamp() " - + "OR threadmill_mutexes.holder = EXCLUDED.holder")) { + try (PreparedStatement ps = conn.prepareStatement("INSERT INTO threadmill_mutexes " + + "(name, holder, expires_at) " + + "VALUES (?, ?, clock_timestamp() + (? * interval '1 millisecond')) " + + "ON CONFLICT (name) DO UPDATE SET holder = EXCLUDED.holder, expires_at = EXCLUDED.expires_at " + + "WHERE threadmill_mutexes.expires_at <= clock_timestamp() " + + "OR threadmill_mutexes.holder = EXCLUDED.holder")) { ps.setString(1, name); ps.setString(2, holder); ps.setLong(3, leaseDuration.toMillis()); @@ -1666,10 +1655,9 @@ private static boolean isReplaceableState(String state) { public void releaseMutex(String name, String holder) { Names.requireName("mutex", name); try { - DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - PreparedStatement ps = - conn.prepareStatement("DELETE FROM threadmill_mutexes WHERE name = ? AND holder = ?")) { + ownedTransaction(conn -> { + try (PreparedStatement ps = + conn.prepareStatement("DELETE FROM threadmill_mutexes WHERE name = ? AND holder = ?")) { ps.setString(1, name); ps.setString(2, holder); ps.executeUpdate(); @@ -1701,24 +1689,23 @@ public void upsertCronTask(CronTask task) { throw new IllegalStateException("Unknown trigger kind: " + trigger); } try { - DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - PreparedStatement ps = conn.prepareStatement( - "INSERT INTO threadmill_cron_tasks (name, trigger_kind, trigger_value, handler_signature, " - + "payload_type_tag, payload_serialized, queue, priority, timeout_seconds, " - + "max_attempts, exclusive, missed_run_policy, time_zone, enabled) " - + "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) " - + "ON CONFLICT (name) DO UPDATE SET " - + "trigger_kind = EXCLUDED.trigger_kind, trigger_value = EXCLUDED.trigger_value, " - + "handler_signature = EXCLUDED.handler_signature, " - + "payload_type_tag = EXCLUDED.payload_type_tag, " - + "payload_serialized = EXCLUDED.payload_serialized, " - + "queue = EXCLUDED.queue, priority = EXCLUDED.priority, " - + "timeout_seconds = EXCLUDED.timeout_seconds, " - + "max_attempts = EXCLUDED.max_attempts, " - + "exclusive = EXCLUDED.exclusive, " - + "missed_run_policy = EXCLUDED.missed_run_policy, " - + "time_zone = EXCLUDED.time_zone, enabled = EXCLUDED.enabled")) { + ownedTransaction(conn -> { + try (PreparedStatement ps = conn.prepareStatement( + "INSERT INTO threadmill_cron_tasks (name, trigger_kind, trigger_value, handler_signature, " + + "payload_type_tag, payload_serialized, queue, priority, timeout_seconds, " + + "max_attempts, exclusive, missed_run_policy, time_zone, enabled) " + + "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) " + + "ON CONFLICT (name) DO UPDATE SET " + + "trigger_kind = EXCLUDED.trigger_kind, trigger_value = EXCLUDED.trigger_value, " + + "handler_signature = EXCLUDED.handler_signature, " + + "payload_type_tag = EXCLUDED.payload_type_tag, " + + "payload_serialized = EXCLUDED.payload_serialized, " + + "queue = EXCLUDED.queue, priority = EXCLUDED.priority, " + + "timeout_seconds = EXCLUDED.timeout_seconds, " + + "max_attempts = EXCLUDED.max_attempts, " + + "exclusive = EXCLUDED.exclusive, " + + "missed_run_policy = EXCLUDED.missed_run_policy, " + + "time_zone = EXCLUDED.time_zone, enabled = EXCLUDED.enabled")) { ps.setString(1, task.name()); ps.setString(2, kind); ps.setString(3, value); @@ -1788,10 +1775,8 @@ public List listCronTasks() { @Override public void deleteCronTask(String name) { try { - DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - PreparedStatement ps = - conn.prepareStatement("DELETE FROM threadmill_cron_tasks WHERE name = ?")) { + ownedTransaction(conn -> { + try (PreparedStatement ps = conn.prepareStatement("DELETE FROM threadmill_cron_tasks WHERE name = ?")) { ps.setString(1, name); ps.executeUpdate(); return null; @@ -1807,11 +1792,10 @@ public void recordCronTaskOwnership(String namespace, String taskName) { Names.requireName("cronTaskNamespace", namespace); Names.requireName("cronTask", taskName); try { - DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - PreparedStatement ps = conn.prepareStatement( - "INSERT INTO threadmill_cron_task_ownership (namespace, task_name) VALUES (?, ?) " - + "ON CONFLICT (namespace, task_name) DO NOTHING")) { + ownedTransaction(conn -> { + try (PreparedStatement ps = conn.prepareStatement( + "INSERT INTO threadmill_cron_task_ownership (namespace, task_name) VALUES (?, ?) " + + "ON CONFLICT (namespace, task_name) DO NOTHING")) { ps.setString(1, namespace); ps.setString(2, taskName); ps.executeUpdate(); @@ -1846,15 +1830,14 @@ public Set listCronTaskNamesOwnedBy(String namespace) { public void upsertCronTaskState(CronTaskScheduleState state) { Objects.requireNonNull(state, "state"); try { - DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection(); - PreparedStatement ps = conn.prepareStatement( - "INSERT INTO threadmill_cron_task_state (task_name, last_run_at, last_run_job_id, " - + "next_run_at, in_flight_job_id, timing_fingerprint) VALUES (?, ?, ?, ?, ?, ?) " - + "ON CONFLICT (task_name) DO UPDATE SET " - + "last_run_at = EXCLUDED.last_run_at, last_run_job_id = EXCLUDED.last_run_job_id, " - + "next_run_at = EXCLUDED.next_run_at, in_flight_job_id = EXCLUDED.in_flight_job_id, " - + "timing_fingerprint = EXCLUDED.timing_fingerprint")) { + ownedTransaction(conn -> { + try (PreparedStatement ps = conn.prepareStatement( + "INSERT INTO threadmill_cron_task_state (task_name, last_run_at, last_run_job_id, " + + "next_run_at, in_flight_job_id, timing_fingerprint) VALUES (?, ?, ?, ?, ?, ?) " + + "ON CONFLICT (task_name) DO UPDATE SET " + + "last_run_at = EXCLUDED.last_run_at, last_run_job_id = EXCLUDED.last_run_job_id, " + + "next_run_at = EXCLUDED.next_run_at, in_flight_job_id = EXCLUDED.in_flight_job_id, " + + "timing_fingerprint = EXCLUDED.timing_fingerprint")) { ps.setString(1, state.taskName()); setNullableTimestamp(ps, 2, state.lastRunAt()); setNullableUuid(ps, 3, state.lastRunJobId()); @@ -1980,11 +1963,15 @@ public void clearCronNudge(String taskName, long observedRevision) { } /** - * Run one write in a transaction Threadmill owns, never the caller's. + * Run one self-owned write in a transaction Threadmill commits, never the caller's. + * + *

    Operations routed here already own their connection rather than joining + * an external transaction. The explicit boundary is required even for a + * single statement because a host pool may hand out connections with + * {@code autoCommit=false}; closing such a connection does not commit it. * - *

    The nudge operations are the only cron writes that must refuse to - * join an external transaction, and the reason is subtle enough to be - * worth stating: the Spring layer defers nudges to {@code afterCommit}, + *

    The nudge operations have an additional reason they must refuse to join: + * the Spring layer defers nudges to {@code afterCommit}, * and at that point Spring has committed the caller's transaction but has * not yet unbound its resources — so a joining boundary would hand back * the caller's connection, execute the nudge in a fresh transaction on it, @@ -2080,9 +2067,7 @@ private Optional findActiveDedup(Connection conn, String queue, String de } private Optional findActiveDedup(String queue, String dedupKey, Instant now) throws SQLException { - try (Connection conn = dataSource.getConnection()) { - return findActiveDedup(conn, queue, dedupKey, now); - } + return ownedTransaction(conn -> findActiveDedup(conn, queue, dedupKey, now)); } private JobSnapshot snapshotForInsert(Connection conn, Job job, long version) throws SQLException { diff --git a/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/PostgresJobStoreRegressionTest.java b/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/PostgresJobStoreRegressionTest.java index d3a05c1..dbb8540 100644 --- a/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/PostgresJobStoreRegressionTest.java +++ b/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/PostgresJobStoreRegressionTest.java @@ -3,6 +3,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import java.io.PrintWriter; import java.sql.Connection; import java.sql.PreparedStatement; import java.sql.ResultSet; @@ -22,6 +23,7 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; +import java.util.logging.Logger; import javax.sql.DataSource; @@ -914,6 +916,29 @@ void cronTaskDefinitionAndScheduleStateRoundTrip() { assertThat(store.findCronTaskState(task.name())).isEmpty(); } + @Test + void selfOwnedWritesCommitWhenConnectionsDefaultToNonAutoCommit() { + var writer = new PostgresJobStore(new NonAutoCommitDataSource(dataSource)); + var observer = store(); + + writer.pauseQueue("low-priority", "maintenance"); + assertThat(observer.listPausedQueues()).contains("low-priority"); + + var task = sampleCronTask("non-auto-commit-task"); + writer.upsertCronTask(task); + assertThat(observer.findCronTask(task.name())).contains(task); + + var nodeId = NodeId.newId(); + var heartbeat = Instant.parse("2026-08-11T12:00:00Z"); + writer.recordNodeHeartbeat(nodeId, heartbeat); + assertThat(observer.readNodeHeartbeat(nodeId)).contains(heartbeat); + + assertThat(writer.tryAcquireMutex("non-auto-commit-mutex", "writer", Duration.ofMinutes(1))) + .isTrue(); + assertThat(observer.tryAcquireMutex("non-auto-commit-mutex", "observer", Duration.ofMinutes(1))) + .isFalse(); + } + @Test void mutexLeaseIsExclusiveReentrantAndExpires() throws InterruptedException { JobStore store = store(); @@ -956,6 +981,64 @@ private static Job awaitingChildOf(Job parent, int index) { .build(); } + /** A pool-alike whose connections arrive with {@code autoCommit=false}. */ + private static final class NonAutoCommitDataSource implements DataSource { + private final DataSource delegate; + + private NonAutoCommitDataSource(DataSource delegate) { + this.delegate = delegate; + } + + @Override + public Connection getConnection() throws SQLException { + var connection = delegate.getConnection(); + connection.setAutoCommit(false); + return connection; + } + + @Override + public Connection getConnection(String username, String password) throws SQLException { + var connection = delegate.getConnection(username, password); + connection.setAutoCommit(false); + return connection; + } + + @Override + public PrintWriter getLogWriter() throws SQLException { + return delegate.getLogWriter(); + } + + @Override + public void setLogWriter(PrintWriter out) throws SQLException { + delegate.setLogWriter(out); + } + + @Override + public void setLoginTimeout(int seconds) throws SQLException { + delegate.setLoginTimeout(seconds); + } + + @Override + public int getLoginTimeout() throws SQLException { + return delegate.getLoginTimeout(); + } + + @Override + public Logger getParentLogger() { + return Logger.getLogger("test"); + } + + @Override + public T unwrap(Class iface) throws SQLException { + return delegate.unwrap(iface); + } + + @Override + public boolean isWrapperFor(Class iface) throws SQLException { + return delegate.isWrapperFor(iface); + } + } + private static void dropSchemaObjects() throws SQLException { try (Connection conn = dataSource.getConnection(); Statement st = conn.createStatement()) { From a21a36a6a9f67db7a5c01838bf96e65b0d140932 Mon Sep 17 00:00:00 2001 From: Helmut Michael Juskewycz Date: Tue, 11 Aug 2026 20:34:14 +0200 Subject: [PATCH 2/6] fix(schedule): repair stale timing fingerprints Refs #112. --- AGENTS.md | 2 + CHANGELOG.md | 5 + .../core/schedule/CronTaskScheduleState.java | 8 +- .../core/schedule/RecurringMaterializer.java | 33 ++++++- .../api/DashboardApiServiceTest.java | 7 +- .../store/memory/SchedulingTest.java | 97 ++++++++++++++++--- 6 files changed, 131 insertions(+), 21 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 40b3268..b594c8e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -285,6 +285,7 @@ This section is the project's memory: the load-bearing decisions worth knowing b ### Scheduling - **Identity vs schedule-state for recurring.** `CronTask` is identity; `CronTaskScheduleState` is bookkeeping. `Scheduler.upsertCron` preserves the schedule state — including an overdue `nextRunAt` and an interval trigger's phase — while the re-registered schedule is unchanged (no disabled→enabled flip), so restart-missed firings stay observable and `MissedRunPolicy` decides their fate; it recomputes `nextRunAt` from now only on a real timing edit, so a freshly edited cron never fires stale times (issue #105: the original unconditional recompute — the pre-release catch-up-storm fix — wiped restart-missed firings before `CATCH_UP` could ever see them; storm safety now lives in the materializer, where `DROP` collapses the backlog to one run and `CATCH_UP` is capped per tick with carry-over). The unchanged-schedule decision reads `CronTaskScheduleState.timingFingerprint` — a canonical string of the trigger (plus zone for cron triggers only; an interval's zone is documented-ignored and deliberately excluded) written atomically with `nextRunAt` in the state record. Never decide preserve-vs-recompute by comparing the stored task definition against the new one: the task and state are two separate store writes, and a crash between them would pair a new trigger with old timing undetectably — the fingerprint makes the retry detect the mismatch and recompute (null fingerprint = legacy row = recompute once). `CronExpression` carries source-based value equality; never regress it to reference identity. `inFlightJobId` is preserved on every path. Dashboard `updateRecurring` intentionally recomputes timing on every edit — an operator editing a task expects it to schedule forward from the edit. +- **A materializer-observed timing-fingerprint mismatch completes the edit without firing the old schedule (issue #112).** In the under-mutex reload, the durable `CronTask` definition is authoritative: `RecurringMaterializer` recomputes `nextRunAt` forward from that tick, stamps the current fingerprint, preserves last-run / in-flight bookkeeping and any nudge, and produces no scheduled instance from the stale timing. Preserving overdue state applies only while its fingerprint matches the current definition; a mismatch is the crashed-edit signature, so materializing once would run a trigger the user already replaced, while merely skipping would leave the task dormant until a future re-registration. For `CATCH_UP`, the obsolete trigger's backlog is intentionally discarded and catch-up resumes normally from the repaired current-trigger timing. A pending nudge remains independent demand and may materialize immediately against the repaired definition. - **`DROP` recovery is phase-exact and nominally stamped.** `RecurringMaterializer` collapses a missed backlog into one instance for the most recent *nominal* fire (`latestFireAtOrBefore`: computed arithmetically for intervals so a tiny interval with a huge backlog cannot spin the maintenance thread; fire-by-fire for cron), stamps `CRON_FIRE_TIME_META` with that nominal time, and advances `nextRunAt` from the nominal fire — so an interval's phase never drifts (due 06:00, recovered 07:00 → next 12:00, not 13:00). Never materialize the DROP recovery at `now` or advance the schedule from `now`. - **Recurring tasks carry their per-instance overrides on the definition.** `CronTask.timeout` (nullable = engine global, whole seconds, rejected below 1s) and `CronTask.maxAttempts` (nullable = `RetryInterceptor` defaults, rejected below 1) are stamped onto every materialized instance as `JobRunner.META_TIMEOUT_SECONDS` / `RetryInterceptor.META_MAX_ATTEMPTS` — by `RecurringMaterializer.materialize` and by the dashboard's manual trigger — so `@Job(timeout)` and `@Job(maxAttempts)` on a `@Recurring` handler behave identically to the enqueue path (issue #84: both used to be silently ignored and every recurring instance ran under the engine defaults; the annotation attribute was named `maxRetries` before v0.1.4 — issue #104 renamed it because the value always counted total attempts). Postgres persists them as `threadmill_cron_tasks.timeout_seconds` / `max_attempts` (V2 migration); Redis as hash fields whose upsert has overwrite semantics so an override-less re-registration clears them. The dashboard's `updateRecurring` rebuilds the `CronTask` field-by-field and must keep preserving all three (see the exclusivity note below). - **Recurring exclusivity is claim-time admission, not a materializer check.** `CronTask.exclusive` (issue #110) makes `RecurringMaterializer.materialize` **and** the dashboard's manual trigger stamp `concurrencyKey = CronTask.concurrencyKeyFor(name)` (`recurring:`, truncated on a code-point boundary with a stable hash suffix past the 256-UTF-8-byte cap) with `ConcurrencyMode.EXCLUSIVE`. The key is derived, never user-supplied, so the `recurring:` namespace cannot collide with an application's own keys. This is deliberately stronger than the pile-up guard: the guard only decides what to materialize on the maintenance leader, while admission is enforced by every store on every node, so it also covers a manual trigger racing a scheduled instance and the retry-handoff window. It does **not** cover reclaim — the terminal failure save releases the slot — and that limitation is documented on the feature, on `@Recurring(exclusive)`, and in `docs/transactions.md`. Persisted as a Postgres column (V6) and a Redis hash field with the same overwrite-on-re-upsert semantics as the timeout/attempt overrides. Both Spring registration paths carry it: the namespaced `reconcileRecurring` path via `taskFor`, and the un-namespaced path via `Scheduler.defineRecurring` — the latter was missed on the first cut and caught by `ThreadmillAutoConfigurationTest`. @@ -484,6 +485,7 @@ Every hard-won failure mode that has come up during development, and the test th | Shutdown requeue moved off the handler's worker thread, letting a peer claim a job while user code still runs on the draining node (issue #110 item 1) | `ProcessingNodeTest.shutdownRequeueIsNeverPublishedWhileTheHandlerIsStillRunning` | | Startup re-registration resets `next_run_at`, so `CATCH_UP` never recovers restart-missed firings; interval phase re-based on every restart (issue #105) | `SchedulingTest.restartReRegistrationPreservesOverdueNextRunSoCatchUpRecoversMissedFires` + `restartReRegistrationWithDropCollapsesMissedFiresIntoASingleRun` + `reRegistrationWithAChangedTriggerRecomputesNextRunFromNow` + `reRegistrationWithAChangedZoneRecomputesNextRun` + `unchangedReRegistrationPreservesTheIntervalPhase` + `reEnablingATaskRestartsTimingInsteadOfCatchingUpTheDisabledPeriod` + `CronExpressionTest.equalityIsValueBasedOnTheSourceExpression` + `AbstractJobStoreContractTest.cronTaskTriggerRoundTripsToEqualValues` | | Crash between the separate cron-task and schedule-state writes pairs a new trigger with old timing that the retry then preserves forever; a store dropping the fingerprint silently reverts restart recovery | `SchedulingTest.stateWriteFailureDuringTriggerEditCannotPairNewTriggerWithOldTiming` + `AbstractJobStoreContractTest.cronTaskStateTimingFingerprintRoundTrips` | +| Materializer under-mutex reload observes a crashed timing edit but applies `CATCH_UP` to the obsolete trigger's overdue state (issue #112) | `SchedulingTest.materializerRepairsMismatchedTimingFingerprintWithoutCatchingUpTheOldSchedule` | | `DROP` recovery re-bases the schedule on the recovery wall-clock (interval phase drifts by the downtime; `cronFireTime()` lies about the firing it represents); an interval's documented-ignored zone counts as a schedule edit across nodes with different system zones | `SchedulingTest.restartReRegistrationWithDropCollapsesMissedFiresIntoASingleRun` (exact nominal-fire and phase assertions) + `differentSystemZonesDoNotResetAnIntervalSchedule` | | Nudge treated as satisfied by an in-flight run that read its inputs before the nudge committed (silent work loss until the backstop); nudge bursts producing 1:1 runs; a nudged run shifting the schedule (issue #108) | `SchedulingTest.nudgeDuringAnInFlightRunProducesExactlyOneFollowUpAfterCompletion` + `nudgeMaterializesPromptlyWithoutTouchingTheSchedule` + `nudgeCoalescesIntoADueScheduledFire` + `nudgeInstanceTakesThePileUpGuardSoAScheduledFireWaits` + `AbstractJobStoreContractTest.clearCronNudgeOnlyClearsTheObservedValue` | | Blanket schedule-state upsert (re-registration, materializer bookkeeping, dashboard edit) clobbers a concurrently accepted nudge; a nudge racing task removal resurrects schedule state; a disabled task runs from a nudge | `AbstractJobStoreContractTest.nudgeRoundTripsAndSurvivesBlanketStateUpserts` + `nudgeOnUnknownTaskIsRejectedWithoutResurrectingState` + `nudgeOnDisabledTaskIsRejected` + `SchedulingTest.nudgeDisabledTaskFailsLoudlyAndDoesNotRun` + `reEnablingATaskClearsAPendingNudgeFromBeforeThePause` + `DashboardApiServiceTest.updateRecurringEnabledFlipClearsAPendingNudge` | diff --git a/CHANGELOG.md b/CHANGELOG.md index e270cc7..a5865c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,11 @@ mutexes, recurring definitions/state, and the expired-dedup fallback now use explicit Threadmill-owned transactions, restoring the connection's prior auto-commit mode after commit or rollback. +- The recurring materializer now repairs a timing-fingerprint mismatch found + during its under-mutex definition reload (issue #112). The current task + definition wins: Threadmill preserves run bookkeeping and pending nudge + demand, recomputes timing forward from the tick, and does not fire or + `CATCH_UP` the obsolete trigger's overdue backlog. - Recurring tasks can declare claim-time exclusivity (issue #110). A `CronTask` gains an `exclusive` flag, surfaced as `@Recurring(exclusive = true)` and as an `exclusive` parameter on diff --git a/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/CronTaskScheduleState.java b/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/CronTaskScheduleState.java index b9dbc16..5524413 100644 --- a/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/CronTaskScheduleState.java +++ b/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/CronTaskScheduleState.java @@ -25,7 +25,8 @@ * or {@code null} for rows written before the * fingerprint existed. Written atomically with * {@code nextRunAt}, it lets {@code Scheduler.upsertCron} - * decide preserve-vs-recompute from this record alone — + * and {@link RecurringMaterializer} decide + * preserve-vs-recompute from this record alone — * a crash between the separate task and state writes can * never pair a new trigger with old timing undetectably * @param nudgeRequestedAt when an on-demand materialization ("nudge") was most @@ -84,8 +85,9 @@ public CronTaskScheduleState( /** * Convenience constructor with no timing fingerprint. A null fingerprint - * is always safe — the next re-registration recomputes the schedule - * instead of preserving it — but production writers that recompute + * is always safe — the next re-registration or materializer tick that is + * about to act recomputes the schedule instead of preserving it — but + * production writers that recompute * {@code nextRunAt} from a task should stamp * {@link #timingFingerprintOf(CronTask)} so unchanged re-registrations * can preserve overdue state. diff --git a/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/RecurringMaterializer.java b/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/RecurringMaterializer.java index 53112b9..7423c2f 100644 --- a/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/RecurringMaterializer.java +++ b/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/RecurringMaterializer.java @@ -35,6 +35,13 @@ * with carry-over. That cap — not re-registration — is the catch-up-storm * defense. * + *

    If the under-mutex definition reload finds that the schedule state's + * timing fingerprint belongs to a different definition, the definition wins: + * timing is recomputed forward from the current tick and the stale schedule + * produces no firing. This completes a crashed timing edit without running a + * trigger the user already replaced. {@code CATCH_UP} resumes normally from + * the repaired timing; it never catches up the obsolete trigger's backlog. + * *

    If a previously-materialised instance is still un-terminal, no new * instance is created until that one finishes. This guard prevents * pile-up under long-running recurring work. @@ -147,6 +154,29 @@ private void tickOneLocked(CronTask listed, Instant now) { CronTask task = store.findCronTask(listed.name()).orElse(null); if (task == null || !task.enabled()) return; + String fingerprint = CronTaskScheduleState.timingFingerprintOf(task); + if (!fingerprint.equals(state.timingFingerprint())) { + // A timing edit writes the definition before its schedule state. + // Seeing a mismatch here is the crash signature for that window. + // Finish the edit by scheduling forward from this tick: firing + // the stale timing would run a trigger the user already replaced, + // while merely skipping would leave the task dormant until some + // future re-registration happened to repair it. + Instant next = task.trigger().nextAfter(now, task.zone()); + state = new CronTaskScheduleState( + task.name(), + state.lastRunAt(), + state.lastRunJobId(), + next, + state.inFlightJobId(), + fingerprint, + state.nudgeRequestedAt(), + state.nudgeRevision()); + store.upsertCronTaskState(state); + due = false; + if (nudge == null) return; + } + // Pile-up guard: an in-flight instance that is still going to run // blocks the next materialization. if (state.inFlightJobId() != null) { @@ -166,7 +196,7 @@ private void tickOneLocked(CronTask listed, Instant now) { // represents no schedule tick), only the nudge origin marker. JobId id = materializeNudge(task); store.upsertCronTaskState(new CronTaskScheduleState( - task.name(), now, id.asUuid(), state.nextRunAt(), id.asUuid(), state.timingFingerprint())); + task.name(), now, id.asUuid(), state.nextRunAt(), id.asUuid(), fingerprint)); // Clear AFTER materializing (a crash between the two costs one // extra run, never a lost one — see the failure-semantics note in // the class Javadoc), and only the observed revision — a nudge @@ -176,7 +206,6 @@ private void tickOneLocked(CronTask listed, Instant now) { return; } - String fingerprint = CronTaskScheduleState.timingFingerprintOf(task); if (task.missedRunPolicy() == CronTask.MissedRunPolicy.CATCH_UP) { // Materialize every fire from nextRunAt up to and including now, // capped per tick so an unbounded backlog cannot occupy the diff --git a/threadmill-dashboard-api/src/test/java/com/hemju/threadmill/dashboard/api/DashboardApiServiceTest.java b/threadmill-dashboard-api/src/test/java/com/hemju/threadmill/dashboard/api/DashboardApiServiceTest.java index 03fe643..532f576 100644 --- a/threadmill-dashboard-api/src/test/java/com/hemju/threadmill/dashboard/api/DashboardApiServiceTest.java +++ b/threadmill-dashboard-api/src/test/java/com/hemju/threadmill/dashboard/api/DashboardApiServiceTest.java @@ -82,8 +82,8 @@ void manualTriggerDoesNotStealThePileUpGuardFromARunningScheduledInstance() { // A scheduled instance materialized earlier is still PROCESSING. var materializer = new RecurringMaterializer(store); - store.upsertCronTaskState( - CronTaskScheduleState.initial("report", Instant.now().minusSeconds(60))); + store.upsertCronTaskState(CronTaskScheduleState.initial( + "report", Instant.now().minusSeconds(60), CronTaskScheduleState.timingFingerprintOf(task))); materializer.tick(Instant.now()); var scheduledInstance = store.claimReady(NodeId.newId(), "default", 1, Instant.now()).getFirst(); @@ -107,7 +107,8 @@ void manualTriggerDoesNotStealThePileUpGuardFromARunningScheduledInstance() { afterTrigger.lastRunAt(), afterTrigger.lastRunJobId(), Instant.now().minusSeconds(1), - afterTrigger.inFlightJobId())); + afterTrigger.inFlightJobId(), + afterTrigger.timingFingerprint())); materializer.tick(Instant.now()); assertThat(store.findByHandlerSignature("com.example.ReportHandler", 10)) .hasSize(2); // scheduled instance + manual trigger, nothing else diff --git a/threadmill-store-memory/src/test/java/com/hemju/threadmill/store/memory/SchedulingTest.java b/threadmill-store-memory/src/test/java/com/hemju/threadmill/store/memory/SchedulingTest.java index dc59a31..acc50b8 100644 --- a/threadmill-store-memory/src/test/java/com/hemju/threadmill/store/memory/SchedulingTest.java +++ b/threadmill-store-memory/src/test/java/com/hemju/threadmill/store/memory/SchedulingTest.java @@ -224,7 +224,12 @@ void catchUpPolicyMaterializesEveryMissedFire() { // Backdate next run by ~1 second; expect ~10 catch-up runs in the first tick. var existing = store.findCronTaskState("catchup").orElseThrow(); store.upsertCronTaskState(new CronTaskScheduleState( - existing.taskName(), null, null, Instant.now().minus(Duration.ofSeconds(1)), null)); + existing.taskName(), + null, + null, + Instant.now().minus(Duration.ofSeconds(1)), + null, + existing.timingFingerprint())); node = ProcessingNode.builder(store).config(fastConfig()).build(); node.start(); await().atMost(Duration.ofSeconds(5)).until(() -> RecorderHandler.RECORD.size() >= 5); @@ -244,7 +249,12 @@ void dropPolicyDoesNotCauseACatchUpStorm() { // Backdate next run far into the past. var existing = store.findCronTaskState("ping").orElseThrow(); store.upsertCronTaskState(new CronTaskScheduleState( - existing.taskName(), null, null, Instant.now().minus(Duration.ofSeconds(60)), null)); + existing.taskName(), + null, + null, + Instant.now().minus(Duration.ofSeconds(60)), + null, + existing.timingFingerprint())); node = ProcessingNode.builder(store).config(fastConfig()).build(); node.start(); @@ -271,7 +281,8 @@ void catchUpInstancesCarryDistinctNominalFireTimes() { CronTask.MissedRunPolicy.CATCH_UP); var existing = store.findCronTaskState("stamped").orElseThrow(); Instant base = Instant.now().minusMillis(350); - store.upsertCronTaskState(new CronTaskScheduleState(existing.taskName(), null, null, base, null)); + store.upsertCronTaskState( + new CronTaskScheduleState(existing.taskName(), null, null, base, null, existing.timingFingerprint())); new RecurringMaterializer(store).tick(Instant.now()); @@ -295,7 +306,7 @@ void materializerSkipsATaskWhoseStateMutexIsHeld() { scheduler.defineIntervalTask("locked", Duration.ofMillis(100), new HelloPayload("tick"), RecorderHandler.class); var existing = store.findCronTaskState("locked").orElseThrow(); store.upsertCronTaskState(new CronTaskScheduleState( - existing.taskName(), null, null, Instant.now().minusSeconds(1), null)); + existing.taskName(), null, null, Instant.now().minusSeconds(1), null, existing.timingFingerprint())); // Another holder (e.g. an upsertCron on a different node) owns the // task's schedule-state mutex: the tick must skip, not clobber. @@ -320,7 +331,7 @@ void upsertCronWaitsForTheTaskMutexAndPreservesInFlightTracking() { "guarded", Duration.ofMillis(100), new HelloPayload("tick"), RecorderHandler.class); var existing = store.findCronTaskState("guarded").orElseThrow(); store.upsertCronTaskState(new CronTaskScheduleState( - existing.taskName(), null, null, Instant.now().minusSeconds(1), null)); + existing.taskName(), null, null, Instant.now().minusSeconds(1), null, existing.timingFingerprint())); new RecurringMaterializer(store).tick(Instant.now()); var inFlight = store.findCronTaskState("guarded").orElseThrow().inFlightJobId(); assertThat(inFlight).isNotNull(); @@ -695,7 +706,7 @@ void exclusiveRecurringInstancesAreSerializedByClaimTimeAdmission() { task.missedRunPolicy()); var initial = store.findCronTaskState("nightly-sweep").orElseThrow(); store.upsertCronTaskState(new CronTaskScheduleState( - initial.taskName(), null, null, Instant.now().minusSeconds(1), null)); + initial.taskName(), null, null, Instant.now().minusSeconds(1), null, initial.timingFingerprint())); var materializer = new RecurringMaterializer(store); materializer.tick(Instant.now()); @@ -726,7 +737,7 @@ void nonExclusiveRecurringInstancesCarryNoConcurrencyKey() { scheduler.defineIntervalTask("loose", Duration.ofMinutes(5), new HelloPayload("tick"), RecorderHandler.class); var initial = store.findCronTaskState("loose").orElseThrow(); store.upsertCronTaskState(new CronTaskScheduleState( - initial.taskName(), null, null, Instant.now().minusSeconds(1), null)); + initial.taskName(), null, null, Instant.now().minusSeconds(1), null, initial.timingFingerprint())); new RecurringMaterializer(store).tick(Instant.now()); var state = store.findCronTaskState("loose").orElseThrow(); @@ -801,7 +812,7 @@ private UUID failInstanceOfTaskWithBudget(String task, int maxAttempts, Instant CronTask.MissedRunPolicy.DROP); var initial = store.findCronTaskState(task).orElseThrow(); store.upsertCronTaskState(new CronTaskScheduleState( - initial.taskName(), null, null, Instant.now().minusSeconds(1), null)); + initial.taskName(), null, null, Instant.now().minusSeconds(1), null, initial.timingFingerprint())); var materializer = new RecurringMaterializer(store); materializer.tick(Instant.now()); var state = store.findCronTaskState(task).orElseThrow(); @@ -841,7 +852,12 @@ void catchUpBacklogIsCappedPerTickWithCarryOver() { var existing = store.findCronTaskState("burst").orElseThrow(); // ~150 missed intervals — more than one tick's materialization cap. store.upsertCronTaskState(new CronTaskScheduleState( - existing.taskName(), null, null, Instant.now().minus(Duration.ofSeconds(15)), null)); + existing.taskName(), + null, + null, + Instant.now().minus(Duration.ofSeconds(15)), + null, + existing.timingFingerprint())); Instant now = Instant.now(); new RecurringMaterializer(store).tick(now); @@ -859,7 +875,7 @@ void recurringInstancesCarryTheirCronTaskName() { scheduler.defineIntervalTask("linked", Duration.ofMillis(100), new HelloPayload("tick"), RecorderHandler.class); var existing = store.findCronTaskState("linked").orElseThrow(); store.upsertCronTaskState(new CronTaskScheduleState( - existing.taskName(), null, null, Instant.now().minusSeconds(1), null)); + existing.taskName(), null, null, Instant.now().minusSeconds(1), null, existing.timingFingerprint())); new RecurringMaterializer(store).tick(Instant.now()); @@ -887,7 +903,7 @@ void recurringInstancesCarryTheTaskTimeoutAsPerJobTimeoutMetadata() { CronTask.MissedRunPolicy.DROP); var existing = store.findCronTaskState("timed").orElseThrow(); store.upsertCronTaskState(new CronTaskScheduleState( - existing.taskName(), null, null, Instant.now().minusSeconds(1), null)); + existing.taskName(), null, null, Instant.now().minusSeconds(1), null, existing.timingFingerprint())); new RecurringMaterializer(store).tick(Instant.now()); @@ -903,7 +919,7 @@ void recurringInstancesWithoutATaskTimeoutCarryNoTimeoutMetadata() { "untimed", Duration.ofMillis(100), new HelloPayload("tick"), RecorderHandler.class); var existing = store.findCronTaskState("untimed").orElseThrow(); store.upsertCronTaskState(new CronTaskScheduleState( - existing.taskName(), null, null, Instant.now().minusSeconds(1), null)); + existing.taskName(), null, null, Instant.now().minusSeconds(1), null, existing.timingFingerprint())); new RecurringMaterializer(store).tick(Instant.now()); @@ -975,7 +991,7 @@ void schedulerRestartDoesNotDoubleEnqueueACronTask() { // Backdate next_run_at so the materializer treats the task as due. var existing = store.findCronTaskState("no-double-enqueue").orElseThrow(); store.upsertCronTaskState(new CronTaskScheduleState( - existing.taskName(), null, null, Instant.now().minusSeconds(1), null)); + existing.taskName(), null, null, Instant.now().minusSeconds(1), null, existing.timingFingerprint())); var materializer = new RecurringMaterializer(store); materializer.tick(Instant.now()); @@ -1219,6 +1235,61 @@ void reEnablingATaskClearsAPendingNudgeFromBeforeThePause() { .isEmpty(); } + @Test + void materializerRepairsMismatchedTimingFingerprintWithoutCatchingUpTheOldSchedule() { + // Crashed timing edit: the new definition landed, but the separate + // state write did not. The under-mutex reload must finish that edit + // instead of applying CATCH_UP to the obsolete trigger's backlog. + scheduler.defineIntervalTask( + "crashed-edit", + Duration.ofMinutes(5), + new HelloPayload("tick"), + RecorderHandler.class, + "default", + 0, + CronTask.MissedRunPolicy.CATCH_UP); + var oldTask = store.findCronTask("crashed-edit").orElseThrow(); + var repairAt = Instant.parse("2026-08-11T12:00:00Z"); + var lastRunAt = repairAt.minus(Duration.ofDays(2)); + var lastRunJobId = UUID.randomUUID(); + var inFlightJobId = UUID.randomUUID(); + store.upsertCronTaskState(new CronTaskScheduleState( + oldTask.name(), + lastRunAt, + lastRunJobId, + repairAt.minus(Duration.ofDays(1)), + inFlightJobId, + CronTaskScheduleState.timingFingerprintOf(oldTask))); + + var editedTask = new CronTask( + oldTask.name(), + new CronTask.Trigger.Interval(Duration.ofHours(6)), + oldTask.handlerType(), + oldTask.payloadArgument(), + oldTask.queue(), + oldTask.priority(), + oldTask.timeout(), + oldTask.maxAttempts(), + oldTask.exclusive(), + CronTask.MissedRunPolicy.CATCH_UP, + oldTask.zone(), + true); + store.upsertCronTask(editedTask); + + new RecurringMaterializer(store).tick(repairAt); + + assertThat(store.findByHandlerSignature(RecorderHandler.class.getName(), 100)) + .as("the obsolete trigger's overdue CATCH_UP backlog must not fire") + .isEmpty(); + assertThat(store.findCronTaskState("crashed-edit").orElseThrow()).satisfies(repaired -> { + assertThat(repaired.nextRunAt()).isEqualTo(repairAt.plus(Duration.ofHours(6))); + assertThat(repaired.timingFingerprint()).isEqualTo(CronTaskScheduleState.timingFingerprintOf(editedTask)); + assertThat(repaired.lastRunAt()).isEqualTo(lastRunAt); + assertThat(repaired.lastRunJobId()).isEqualTo(lastRunJobId); + assertThat(repaired.inFlightJobId()).isEqualTo(inFlightJobId); + }); + } + @Test void materializerReloadsTheDefinitionUnderTheTaskMutexBeforeActing() { // tick() snapshots the task list BEFORE tickOne takes the per-task From 29bf7edb47a00e34a2ae5602b414f0240c65099d Mon Sep 17 00:00:00 2001 From: Helmut Michael Juskewycz Date: Tue, 11 Aug 2026 20:48:57 +0200 Subject: [PATCH 3/6] test(simulation): add process-separated nudge churn Refs #114 --- AGENTS.md | 6 +- CHANGELOG.md | 6 + threadmill-simulation/README.md | 34 + threadmill-simulation/build.gradle.kts | 25 + .../nudge/NudgeSimulationHandler.java | 53 ++ .../simulation/nudge/NudgeSimulationMain.java | 732 ++++++++++++++++++ .../nudge/NudgeSimulationPayload.java | 17 + .../nudge/NudgeSimulationStores.java | 307 ++++++++ .../nudge/NudgeSimulationTrace.java | 57 ++ 9 files changed, 1235 insertions(+), 2 deletions(-) create mode 100644 threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationHandler.java create mode 100644 threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationMain.java create mode 100644 threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationPayload.java create mode 100644 threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationStores.java create mode 100644 threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationTrace.java diff --git a/AGENTS.md b/AGENTS.md index b594c8e..ece417e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -48,7 +48,7 @@ Modules are physically separate. No storage implementation and no UI may ever be | `threadmill-dashboard-ui` | n/a | Static React/Tailwind/shadcn operations console packaged as reusable classpath assets. | | `threadmill-dashboard-spring` | `com.hemju.threadmill.dashboard.spring` | Spring MVC/Security dashboard adapter: JSON endpoints, redaction, authorization, audit hooks, operator actions, and optional UI mounting. | | `threadmill-soak` | `com.hemju.threadmill.soak` | Fixed soak regression suite plus tunable sustained load/performance harnesses. Tagged regression tests are excluded from `check`; harness artifacts live under `build/soak/`. | -| `threadmill-simulation` | `com.hemju.threadmill.simulation` | Correctness simulations: short fixed invariant verifier for all three stores, plus worker-churn simulations against shared Postgres / Redis. JSON-lines traces live under `build/simulation/`. | +| `threadmill-simulation` | `com.hemju.threadmill.simulation` | Correctness simulations: short fixed invariant verifier for all three stores, worker-churn simulations against shared Postgres / Redis, and process-separated nudge failover / producer-crash simulations. JSON-lines traces live under `build/simulation/`. | | `threadmill-example` | `com.example.threadmill` | Compiled user-facing examples only: in-memory getting-started, manual Postgres / Redis workers, and submitter examples. | --- @@ -149,6 +149,7 @@ The build uses Gradle (≥ 9.5) and the project's Java 25 toolchain. - `./gradlew :threadmill-soak:soakEndurance` — production-readiness endurance run: one harness JVM per backend (Postgres + Redis) in parallel; defaults 8h × 50 jobs/s × 3 nodes with node churn every 10m. Pair with `threadmill-soak/docker-compose.endurance.yml` via `-PpostgresUrl` / `-PredisUrl`. - `./gradlew :threadmill-simulation:simulate` — run the short correctness simulation against all three backends. - `./gradlew :threadmill-simulation:simulateWorkerChurnPostgres` / `simulateWorkerChurnRedis` — run worker-process churn simulations against shared local datastores. +- `./gradlew :threadmill-simulation:simulateNudgePostgres` / `simulateNudgeRedis` — run the fixed process-separated nudge simulation with a hard-killed maintenance leader and a producer killed between its durable work write and nudge. `simulateNudge` runs both real backends. - `./gradlew :threadmill-example:run` — compile and run the public getting-started example. - `./gradlew productionCheck` — release-candidate validation: clean, check, Javadoc, real store tests, soak, dependency scan hook, example, and artifact inspection. - Integration tests for the real backends use **Testcontainers** and need a working container runtime (Docker / Podman / Colima / OrbStack). @@ -497,6 +498,7 @@ Every hard-won failure mode that has come up during development, and the test th | Per-transaction nudge storm: the documented "nudge once per work item" pattern registering one synchronisation and one store round trip per call; a `REQUIRES_NEW` inner transaction inheriting the outer's batch through a bound resource | `SpringPostgresTransactionBoundaryTest.repeatedNudgesOfOneTaskInATransactionCollapseToASingleWrite` (batch scoped to the suspend-aware synchronisation list, not `bindResource`) | | Recurring-runs meter counting retry attempts instead of instances, inflating the nudge-versus-schedule ratio operators diagnose with; a non-flip dashboard edit clearing a pending nudge | `ThreadmillMetricsTest.recurringRunsCounterCountsInstancesNotRetryAttempts` + `DashboardApiServiceTest.updateRecurringWithoutAnEnabledFlipLeavesAPendingNudgeIntact` | | Nudge guarantees hold in unit tests but break under sustained load, node churn, or master handover (no soak coverage of the wake-driven path at all) | `nudge-pump` soak scenario + `InvariantChecks.nudgeRunAfterWake` / `outboxDrainedByLaterRun` (red paths: `InvariantViolationTest.nudgeRunAfterWakeFiresWhenNoRunStartsAfterTheNudge` + `nudgeRunAfterWakeFiresWhenTheNudgeGoesUnservedTooLong` + `outboxDrainedByLaterRunFiresWhenARowSurvivesAWholeRun`; green paths: `nudgeRunAfterWakeAcceptsARunThatStartsAfterTheNudge` + `outboxDrainedByLaterRunToleratesRowsAppendedAfterTheRunStarted` + `NudgePumpSmokeTest`) | +| Accepted nudge is lost when the maintenance leader dies before its next tick; producer crash between durable work and nudge leaves work stranded past the recurring backstop (issue #114) | `:threadmill-simulation:simulateNudgePostgres` + `simulateNudgeRedis` (`NudgeSimulationMain` runs leader, standby, and producers in separate JVMs; its trace verifier pins hard-kill ordering, process identity, standby ownership, `nudge` / `schedule` origin, and drain) | ### Postgres-layer improvements (engagement notes) @@ -556,7 +558,7 @@ These are deliberately additive and design-compatible with the current model — - **Landed:** `ThreadmillAutoConfiguration` carries `@AutoConfigureAfter` for `DataSourceAutoConfiguration` and `RedisAutoConfiguration`. The `JobStore` bean resolves by precedence (explicit Redis config → Postgres if a `DataSource` is present and the Postgres store is on the classpath → explicitly enabled in-memory development store); startup fails when no store is configured. `ThreadmillLifecycle` uses Spring's maximum/default `SmartLifecycle` phase, which starts lower phases first and stops higher phases first. Remote-wake subscription and shutdown are owned by that same lifecycle so ordering relative to the node is deterministic. - **Deferred:** Actuator integration (`HealthIndicator`, `MeterBinder`, `/actuator/threadmill` endpoint) is held back from v1 because Spring Boot 4.0's actuator surface is still being reorganised — health and Micrometer integration moved out of the main `spring-boot-actuator` artifact during the milestone series and the final shape isn't pinned yet. Re-attempt after SB4 GA. Spring AOT `RuntimeHints` for native image is deferred for the same reason — it needs a stable actuator target first. A `threadmill-example/spring-boot-4/` sample app is deferred to the same follow-up. (Spring Boot 3 is intentionally not supported and there is no SB3 sample app planned.) After-commit enqueue is already default-on (postgres-improvements Phase 5). - **Task 3 of the v1-readiness finishing pass — per-module READMEs and full docs — landed.** All 14 module READMEs (`threadmill-core`, the three stores, `threadmill-spring-boot`, `threadmill-test-support`, `threadmill-metrics`, `threadmill-tracing`, the three dashboard modules, `threadmill-soak`, `threadmill-simulation`, `threadmill-example`) and the restructured `docs/` tree exist: `index`, `getting-started`, `quickstart` (Spring), `architecture`, `handlers`, `transactions` (deep dive — atomic boundaries per backend, handler-is-not-in-our-transaction, at-least-once + idempotency, outbox pattern), `backend-execution-model`, `configuration`, `concurrency`, `queue-topology`, `long-running-jobs`, `deduplication`, `wake-driven-pollers` (the nudge pattern: handler shape, choosing the backstop interval, what coalescing means for handler code — recurring tasks otherwise had no usage page), `operations`, `troubleshooting`, `migration`, `postgres-schema`, `redis-topologies`, `release-checklist`, plus the JobRunr / Quartz comparison pages. The Postgres README carries the full schema; the Redis README carries the full key layout and Lua script inventory. Runnable examples are compiled files under `threadmill-example/src/main/java/com/example/threadmill/`; doc snippets are maintained by hand (there is no compiled `threadmill-example/snippets/` directory). The bar is "an AI agent can use Threadmill to replace an existing job/scheduler system without reading source code." -- **Task 4 of the v1-readiness finishing pass — `threadmill-simulation` module — landed.** New module, separate from `threadmill-soak` (load/performance) and `threadmill-example` (teaching). The short correctness simulation runs 50 projects with `Import` (EXCLUSIVE) and `Export` (SHARED) jobs, 400 jobs over the run (small enough to finish in seconds), random failure injection (5% exception, 0.5% hang), mid-run pause/resume, half-via-`insertAll` bulk-enqueue sample. Records JSON-lines traces under `build/simulation/`; `TraceVerifier` asserts at-least-once, concurrency exclusion (EXCLUSIVE-vs-anything, SHARED-vs-EXCLUSIVE), lock pairing, and pause-obeyed. Gradle entry points: `:threadmill-simulation:simulate` (all three backends), `simulateMemory`, `simulatePostgres`, `simulateRedis`. The Gradle task fails (non-zero exit) when any backend doesn't drain or produces a verifier violation. The worker-churn simulation lives under `com.hemju.threadmill.simulation.workerchurn` and runs through `simulateWorkerChurnPostgres` / `simulateWorkerChurnRedis` against shared local datastores, writing traces to `build/simulation/worker-churn--.jsonl` by default. +- **Task 4 of the v1-readiness finishing pass — `threadmill-simulation` module — landed.** New module, separate from `threadmill-soak` (load/performance) and `threadmill-example` (teaching). The short correctness simulation runs 50 projects with `Import` (EXCLUSIVE) and `Export` (SHARED) jobs, 400 jobs over the run (small enough to finish in seconds), random failure injection (5% exception, 0.5% hang), mid-run pause/resume, half-via-`insertAll` bulk-enqueue sample. Records JSON-lines traces under `build/simulation/`; `TraceVerifier` asserts at-least-once, concurrency exclusion (EXCLUSIVE-vs-anything, SHARED-vs-EXCLUSIVE), lock pairing, and pause-obeyed. Gradle entry points: `:threadmill-simulation:simulate` (all three backends), `simulateMemory`, `simulatePostgres`, `simulateRedis`. The Gradle task fails (non-zero exit) when any backend doesn't drain or produces a verifier violation. The worker-churn simulation lives under `com.hemju.threadmill.simulation.workerchurn` and runs through `simulateWorkerChurnPostgres` / `simulateWorkerChurnRedis` against shared local datastores, writing traces to `build/simulation/worker-churn--.jsonl` by default. The fixed process-separated nudge simulation under `com.hemju.threadmill.simulation.nudge` runs through `simulateNudgePostgres` / `simulateNudgeRedis`: a supervisor hard-kills the maintenance leader after an accepted nudge and requires a standby-process nudge run, then hard-kills a producer after its durable work write and requires a schedule-origin backstop drain. Its per-run directory contains the verified cross-process trace plus every child JVM log. - **Batches.** The child relationship is already in the model; a `BatchCompletionInterceptor` over multiple children is the natural shape. - **External-trigger jobs.** The `PROCESSED` state is reserved; this needs an external-signal API plus an escape-hatch timeout. - **Rate limiters.** A store-side token-bucket primitive; Redis can use the standard Lua bucket pattern. diff --git a/CHANGELOG.md b/CHANGELOG.md index a5865c9..8d21226 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,12 @@ definition wins: Threadmill preserves run bookkeeping and pending nudge demand, recomputes timing forward from the tick, and does not fire or `CATCH_UP` the obsolete trigger's overdue backlog. +- Added fixed process-separated nudge simulations for Postgres and Redis + (issue #114). They hard-kill a maintenance leader after an accepted nudge + and prove the standby serves it, then hard-kill a producer after its durable + work write but before its nudge and prove the regular recurring backstop + drains the row. Cross-process JSON-lines traces record process ids, + leadership, trigger origins, and the verified event ordering. - Recurring tasks can declare claim-time exclusivity (issue #110). A `CronTask` gains an `exclusive` flag, surfaced as `@Recurring(exclusive = true)` and as an `exclusive` parameter on diff --git a/threadmill-simulation/README.md b/threadmill-simulation/README.md index 4fe79ed..d7b274f 100644 --- a/threadmill-simulation/README.md +++ b/threadmill-simulation/README.md @@ -145,6 +145,40 @@ attempts, and periodic store snapshots. It is a production-behaviour trace generator, not a replacement for the contract tests. The task exits non-zero when the queue does not drain within `--drain-timeout`. +## Process-Separated Nudge Simulation + +This fixed simulation starts real Postgres and Redis datastores with +Testcontainers, then runs the maintenance leader, standby, and producers in +separate JVMs. Run both backends: + +```bash +./gradlew :threadmill-simulation:simulateNudge +``` + +Or select one: + +```bash +./gradlew :threadmill-simulation:simulateNudgePostgres +./gradlew :threadmill-simulation:simulateNudgeRedis +``` + +The leader registers an exclusive eight-second recurring outbox pump. The +supervisor then verifies two crash windows: + +- A producer commits durable work and an accepted nudge. The supervisor + hard-kills the maintenance leader before its next materializer tick; the + standby must acquire the expired maintenance lease and run the pump with + `origin=nudge`. +- A second producer commits durable work, stops immediately before the nudge, + and is hard-killed. No nudge may appear for that row; the regular recurring + fire must run with `origin=schedule` and drain it. + +The verifier reads a cross-process JSON-lines trace and proves the event +ordering, distinct process ids, standby ownership, trigger origins, and final +drain. Artifacts live under +`build/simulation/nudge-cross-node--/`, including +`trace.jsonl` and one output log per child JVM. + ## Why a separate module `threadmill-soak` is about sustained load and operational performance diff --git a/threadmill-simulation/build.gradle.kts b/threadmill-simulation/build.gradle.kts index 6eccd70..f071f0e 100644 --- a/threadmill-simulation/build.gradle.kts +++ b/threadmill-simulation/build.gradle.kts @@ -24,6 +24,7 @@ dependencies { // Per-backend simulation tasks. The base `simulate` runs all three sequentially. val simulationMainClass = "com.hemju.threadmill.simulation.SimulationMain" val workerChurnMainClass = "com.hemju.threadmill.simulation.workerchurn.WorkerChurnSimulatorMain" +val nudgeSimulationMainClass = "com.hemju.threadmill.simulation.nudge.NudgeSimulationMain" tasks.register("simulateMemory") { group = "verification" @@ -71,3 +72,27 @@ tasks.register("simulateWorkerChurnRedis") { mainClass.set(workerChurnMainClass) args = listOf("--backend", "redis") } + +tasks.register("simulateNudgePostgres") { + group = "verification" + description = + "Run the process-separated leader-kill and producer-kill nudge simulation against PostgreSQL." + classpath = sourceSets["main"].runtimeClasspath + mainClass.set(nudgeSimulationMainClass) + args = listOf("--backend", "postgres") +} + +tasks.register("simulateNudgeRedis") { + group = "verification" + description = + "Run the process-separated leader-kill and producer-kill nudge simulation against Redis." + classpath = sourceSets["main"].runtimeClasspath + mainClass.set(nudgeSimulationMainClass) + args = listOf("--backend", "redis") +} + +tasks.register("simulateNudge") { + group = "verification" + description = "Run the process-separated nudge simulation against PostgreSQL and Redis." + dependsOn("simulateNudgePostgres", "simulateNudgeRedis") +} diff --git a/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationHandler.java b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationHandler.java new file mode 100644 index 0000000..66f5991 --- /dev/null +++ b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationHandler.java @@ -0,0 +1,53 @@ +package com.hemju.threadmill.simulation.nudge; + +import java.nio.file.Path; +import java.util.Map; + +import com.hemju.threadmill.core.handler.JobExecutionContext; +import com.hemju.threadmill.core.handler.JobHandler; + +/** Recurring outbox-pump handler used by the process-separated nudge simulation. */ +public final class NudgeSimulationHandler implements JobHandler { + + @Override + public void run(NudgeSimulationPayload payload, JobExecutionContext context) { + var trace = Path.of(payload.traceFile); + var pid = ProcessHandle.current().pid(); + var jobId = context.jobId().toString(); + var origin = context.cronOrigin().orElse("unknown"); + NudgeSimulationTrace.append( + trace, + "pump-run-start", + Map.of( + "runId", payload.runId, + "pid", pid, + "nodeId", context.nodeId().toString(), + "jobId", jobId, + "origin", origin)); + + try (var workStore = NudgeSimulationStores.openProcessWorkStore(payload.runId)) { + for (var sequence : workStore.drain()) { + NudgeSimulationTrace.append( + trace, + "work-drained", + Map.of( + "runId", payload.runId, + "sequence", sequence, + "pid", pid, + "nodeId", context.nodeId().toString(), + "jobId", jobId, + "origin", origin)); + } + } + + NudgeSimulationTrace.append( + trace, + "pump-run-finish", + Map.of( + "runId", payload.runId, + "pid", pid, + "nodeId", context.nodeId().toString(), + "jobId", jobId, + "origin", origin)); + } +} diff --git a/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationMain.java b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationMain.java new file mode 100644 index 0000000..fd593d1 --- /dev/null +++ b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationMain.java @@ -0,0 +1,732 @@ +package com.hemju.threadmill.simulation.nudge; + +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.time.Duration; +import java.time.Instant; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Objects; +import java.util.UUID; +import java.util.concurrent.TimeUnit; +import java.util.function.BooleanSupplier; +import java.util.function.Predicate; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; + +import com.hemju.threadmill.core.JobId; +import com.hemju.threadmill.core.JobState; +import com.hemju.threadmill.core.NodeId; +import com.hemju.threadmill.core.engine.ProcessingNode; +import com.hemju.threadmill.core.engine.ProcessingNodeConfig; +import com.hemju.threadmill.core.engine.QueueLane; +import com.hemju.threadmill.core.schedule.CronTask; +import com.hemju.threadmill.core.schedule.Scheduler; +import com.hemju.threadmill.core.serialization.JsonJobSerializer; +import com.hemju.threadmill.core.store.JobStore; +import com.hemju.threadmill.simulation.nudge.NudgeSimulationStores.Backend; +import com.hemju.threadmill.simulation.nudge.NudgeSimulationStores.ConnectionInfo; + +/** + * Process-separated nudge correctness simulation for Postgres and Redis. + * + *

    A leader JVM registers a recurring outbox pump, a standby JVM competes + * for maintenance leadership, and producer JVMs write durable work. The + * supervisor hard-kills the leader after one accepted nudge and hard-kills a + * second producer after its work write but before its nudge. The trace + * verifier requires the standby to serve the accepted nudge and the regular + * schedule to drain the producer's crash-window row. + */ +public final class NudgeSimulationMain { + + private static final ObjectMapper JSON = new ObjectMapper(); + private static final Duration BACKSTOP_INTERVAL = Duration.ofSeconds(8); + private static final Duration LEADER_MAINTENANCE_POLL = Duration.ofSeconds(10); + private static final Duration STANDBY_MAINTENANCE_POLL = Duration.ofMillis(100); + private static final Duration PROCESS_START_TIMEOUT = Duration.ofSeconds(10); + private static final Duration FAILOVER_TIMEOUT = Duration.ofSeconds(8); + private static final Duration BACKSTOP_TIMEOUT = Duration.ofSeconds(16); + + private NudgeSimulationMain() {} + + static void main(String[] args) throws Exception { + var options = Options.parse(args); + switch (options.role) { + case SUPERVISOR -> runSupervisor(options); + case NODE -> runNode(options); + case PRODUCER -> runProducer(options); + } + } + + private static void runSupervisor(Options options) throws Exception { + var backendName = options.backend.name().toLowerCase(Locale.ROOT); + var runId = UUID.randomUUID().toString(); + var timestamp = Instant.now().toString().replace(':', '-'); + var outputDirectory = Path.of("build", "simulation", "nudge-cross-node-" + timestamp + "-" + backendName) + .toAbsolutePath() + .normalize(); + Files.createDirectories(outputDirectory); + var trace = outputDirectory.resolve("trace.jsonl"); + var taskName = "nudge-simulation-" + runId; + var queue = "nudge-simulation-" + runId; + NudgeSimulationTrace.append( + trace, + "simulation-start", + Map.of( + "backend", backendName, + "runId", runId, + "taskName", taskName, + "queue", queue, + "backstopMillis", BACKSTOP_INTERVAL.toMillis())); + + ManagedProcess leader = null; + ManagedProcess standby = null; + ManagedProcess crashProducer = null; + try (var fixture = NudgeSimulationStores.startBackend(options.backend); + var storeHandle = NudgeSimulationStores.openJobStore(fixture.connectionInfo()); + var workStore = NudgeSimulationStores.openWorkStore(fixture.connectionInfo(), runId)) { + workStore.prepare(); + + var leaderReadyFile = outputDirectory.resolve("leader.ready.json"); + leader = startNode( + fixture.connectionInfo(), + outputDirectory, + trace, + runId, + taskName, + queue, + "leader", + true, + LEADER_MAINTENANCE_POLL, + leaderReadyFile); + var leaderReady = awaitReady(leader, leaderReadyFile, PROCESS_START_TIMEOUT); + await( + "leader to acquire the maintenance lease", + FAILOVER_TIMEOUT, + () -> storeHandle + .store() + .readMaintenanceLeaseOwner() + .filter(leaderReady.nodeId()::equals) + .isPresent()); + // Node.start() creates the maintenance thread asynchronously. Let + // its immediate no-nudge tick finish so the leader is sleeping on + // the deliberately long poll when the producer commits. + Thread.sleep(500); + + var standbyReadyFile = outputDirectory.resolve("standby.ready.json"); + standby = startNode( + fixture.connectionInfo(), + outputDirectory, + trace, + runId, + taskName, + queue, + "standby", + false, + STANDBY_MAINTENANCE_POLL, + standbyReadyFile); + var standbyReady = awaitReady(standby, standbyReadyFile, PROCESS_START_TIMEOUT); + require( + storeHandle + .store() + .readMaintenanceLeaseOwner() + .filter(leaderReady.nodeId()::equals) + .isPresent(), + "standby unexpectedly displaced the live leader"); + + var acceptedProducer = startProducer( + fixture.connectionInfo(), + outputDirectory, + trace, + runId, + taskName, + queue, + 1, + ProducerMode.NUDGE, + outputDirectory.resolve("producer-accepted.ready")); + awaitSuccess(acceptedProducer, PROCESS_START_TIMEOUT); + var observedNudge = storeHandle.store().findCronTaskState(taskName).orElseThrow(); + require(observedNudge.nudgeRequestedAt() != null, "accepted nudge was consumed before the leader kill"); + require(observedNudge.nudgeRevision() != null, "accepted nudge has no revision"); + + var killedLeaderPid = leader.pid(); + leader.destroyForcibly(); + NudgeSimulationTrace.append( + trace, + "leader-hard-killed", + Map.of( + "runId", + runId, + "pid", + killedLeaderPid, + "nodeId", + leaderReady.nodeId().toString(), + "observedNudgeRevision", + observedNudge.nudgeRevision())); + awaitExit(leader, PROCESS_START_TIMEOUT); + leader = null; + + await( + "standby to acquire the expired maintenance lease", + FAILOVER_TIMEOUT, + () -> storeHandle + .store() + .readMaintenanceLeaseOwner() + .filter(standbyReady.nodeId()::equals) + .isPresent()); + NudgeSimulationTrace.append( + trace, + "maintenance-elected", + Map.of( + "runId", runId, + "pid", standbyReady.pid(), + "nodeId", standbyReady.nodeId().toString())); + await("accepted nudge work to drain", FAILOVER_TIMEOUT, () -> !workStore.isPending(1)); + awaitRecurringInstanceSuccess(storeHandle.store(), taskName, FAILOVER_TIMEOUT); + + var crashReadyFile = outputDirectory.resolve("producer-crash.ready.json"); + crashProducer = startProducer( + fixture.connectionInfo(), + outputDirectory, + trace, + runId, + taskName, + queue, + 2, + ProducerMode.WAIT_BEFORE_NUDGE, + crashReadyFile); + var crashReady = awaitReady(crashProducer, crashReadyFile, PROCESS_START_TIMEOUT); + require(workStore.isPending(2), "crash-window producer did not persist its work row"); + crashProducer.destroyForcibly(); + NudgeSimulationTrace.append( + trace, + "producer-hard-killed-before-nudge", + Map.of("runId", runId, "sequence", 2, "pid", crashReady.pid())); + awaitExit(crashProducer, PROCESS_START_TIMEOUT); + crashProducer = null; + + await( + "crash-window work to drain on the backstop schedule", + BACKSTOP_TIMEOUT, + () -> !workStore.isPending(2)); + awaitRecurringInstanceSuccess(storeHandle.store(), taskName, FAILOVER_TIMEOUT); + + stopGracefully(standby, PROCESS_START_TIMEOUT); + standby = null; + verifyTrace(trace, standbyReady.pid()); + NudgeSimulationTrace.append( + trace, + "verification-passed", + Map.of("backend", backendName, "runId", runId, "trace", trace.toString())); + System.out.println("process-separated nudge simulation passed; trace: " + trace); + } finally { + destroyIfAlive(crashProducer); + destroyIfAlive(standby); + destroyIfAlive(leader); + } + } + + private static void runNode(Options options) throws Exception { + var connectionInfo = options.connectionInfo(); + NudgeSimulationStores.configureProcess(connectionInfo); + try (var storeHandle = NudgeSimulationStores.openJobStore(connectionInfo)) { + if (options.registerTask) { + var scheduler = new Scheduler(storeHandle.store(), new JsonJobSerializer()); + scheduler.defineRecurring( + options.taskName, + new CronTask.Trigger.Interval(BACKSTOP_INTERVAL), + new NudgeSimulationPayload(options.runId, options.traceFile.toString()), + NudgeSimulationHandler.class.getName(), + options.queue, + 0, + null, + null, + true, + CronTask.MissedRunPolicy.DROP); + } + + var config = ProcessingNodeConfig.builder() + .workerCount(1) + .pollInterval(Duration.ofMillis(50)) + .claimHeartbeat(Duration.ofMillis(250)) + .heartbeatTimeout(Duration.ofSeconds(2)) + .maintenanceLeaseDuration(Duration.ofMillis(1500)) + .maintenancePollInterval(options.maintenancePoll) + .jobTimeout(Duration.ofSeconds(10)) + .shutdownGracePeriod(Duration.ofSeconds(2)) + .claimBatchSize(1) + .build(); + var node = ProcessingNode.builder(storeHandle.store()) + .config(config) + .lane(new QueueLane(options.queue, 1)) + .build(); + Runtime.getRuntime() + .addShutdownHook(new Thread( + () -> { + NudgeSimulationTrace.append( + options.traceFile, + "node-stop", + Map.of( + "runId", + options.runId, + "label", + options.label, + "pid", + ProcessHandle.current().pid(), + "nodeId", + node.nodeId().toString())); + node.close(); + }, + "threadmill-nudge-simulation-shutdown")); + node.start(); + var pid = ProcessHandle.current().pid(); + NudgeSimulationTrace.append( + options.traceFile, + "node-start", + Map.of( + "runId", + options.runId, + "label", + options.label, + "pid", + pid, + "nodeId", + node.nodeId().toString(), + "registeredTask", + options.registerTask, + "maintenancePollMillis", + options.maintenancePoll.toMillis())); + writeReady(options.readyFile, node.nodeId(), pid); + Thread.currentThread().join(); + } + } + + private static void runProducer(Options options) throws Exception { + var connectionInfo = options.connectionInfo(); + try (var storeHandle = NudgeSimulationStores.openJobStore(connectionInfo); + var workStore = NudgeSimulationStores.openWorkStore(connectionInfo, options.runId)) { + var pid = ProcessHandle.current().pid(); + workStore.record(options.sequence); + NudgeSimulationTrace.append( + options.traceFile, + "work-recorded", + Map.of("runId", options.runId, "sequence", options.sequence, "pid", pid)); + writeReady(options.readyFile, null, pid); + if (options.producerMode == ProducerMode.WAIT_BEFORE_NUDGE) { + Thread.currentThread().join(); + return; + } + + var scheduler = new Scheduler(storeHandle.store(), new JsonJobSerializer()); + scheduler.nudgeRecurring(options.taskName); + var revision = storeHandle + .store() + .findCronTaskState(options.taskName) + .map(state -> state.nudgeRevision()) + .orElseThrow(); + NudgeSimulationTrace.append( + options.traceFile, + "nudge-accepted", + Map.of("runId", options.runId, "sequence", options.sequence, "pid", pid, "revision", revision)); + } + } + + private static ManagedProcess startNode( + ConnectionInfo connectionInfo, + Path outputDirectory, + Path trace, + String runId, + String taskName, + String queue, + String label, + boolean registerTask, + Duration maintenancePoll, + Path readyFile) + throws IOException { + var arguments = new ArrayList(); + arguments.add("--role"); + arguments.add("node"); + arguments.add("--run-id"); + arguments.add(runId); + arguments.add("--trace"); + arguments.add(trace.toString()); + arguments.add("--task"); + arguments.add(taskName); + arguments.add("--queue"); + arguments.add(queue); + arguments.add("--label"); + arguments.add(label); + arguments.add("--register-task"); + arguments.add(Boolean.toString(registerTask)); + arguments.add("--maintenance-poll"); + arguments.add(maintenancePoll.toMillis() + "ms"); + arguments.add("--ready"); + arguments.add(readyFile.toString()); + return startProcess(connectionInfo, outputDirectory, label, arguments); + } + + private static ManagedProcess startProducer( + ConnectionInfo connectionInfo, + Path outputDirectory, + Path trace, + String runId, + String taskName, + String queue, + int sequence, + ProducerMode mode, + Path readyFile) + throws IOException { + var label = "producer-" + sequence; + var arguments = new ArrayList(); + arguments.add("--role"); + arguments.add("producer"); + arguments.add("--run-id"); + arguments.add(runId); + arguments.add("--trace"); + arguments.add(trace.toString()); + arguments.add("--task"); + arguments.add(taskName); + arguments.add("--queue"); + arguments.add(queue); + arguments.add("--sequence"); + arguments.add(Integer.toString(sequence)); + arguments.add("--producer-mode"); + arguments.add(mode.name().toLowerCase(Locale.ROOT)); + arguments.add("--ready"); + arguments.add(readyFile.toString()); + return startProcess(connectionInfo, outputDirectory, label, arguments); + } + + private static ManagedProcess startProcess( + ConnectionInfo connectionInfo, Path outputDirectory, String label, List arguments) + throws IOException { + var command = new ArrayList(); + command.add(Path.of(System.getProperty("java.home"), "bin", "java").toString()); + command.add("-cp"); + command.add(System.getProperty("java.class.path")); + command.add(NudgeSimulationMain.class.getName()); + connectionInfo.appendArguments(command); + command.addAll(arguments); + File log = outputDirectory.resolve(label + ".out.log").toFile(); + var process = new ProcessBuilder(command) + .redirectOutput(log) + .redirectErrorStream(true) + .start(); + return new ManagedProcess(label, process, log.toPath()); + } + + private static Ready awaitReady(ManagedProcess process, Path readyFile, Duration timeout) throws Exception { + await( + process.label() + " to become ready", + timeout, + () -> Files.isRegularFile(readyFile) || !process.isAlive()); + if (!Files.isRegularFile(readyFile)) { + throw new IllegalStateException(process.label() + " exited before ready; output: " + process.logFile()); + } + var document = JSON.readTree(Files.readString(readyFile, StandardCharsets.UTF_8)); + var nodeId = document.hasNonNull("nodeId") + ? NodeId.of(UUID.fromString(document.get("nodeId").asText())) + : null; + return new Ready(nodeId, document.get("pid").asLong()); + } + + private static void writeReady(Path readyFile, NodeId nodeId, long pid) { + try { + var fields = new LinkedHashMap(); + fields.put("pid", pid); + fields.put("nodeId", nodeId == null ? null : nodeId.toString()); + Files.writeString(readyFile, JSON.writeValueAsString(fields), StandardCharsets.UTF_8); + } catch (IOException e) { + throw new IllegalStateException("failed to write process-ready marker: " + readyFile, e); + } + } + + private static void awaitRecurringInstanceSuccess(JobStore store, String taskName, Duration timeout) + throws Exception { + await( + "recurring instance to finish", + timeout, + () -> store.findCronTaskState(taskName) + .map(state -> state.inFlightJobId()) + .map(JobId::of) + .flatMap(store::findById) + .map(job -> job.currentState() == JobState.SUCCEEDED) + .orElse(false)); + } + + private static void await(String description, Duration timeout, BooleanSupplier condition) throws Exception { + var deadline = System.nanoTime() + timeout.toNanos(); + while (System.nanoTime() < deadline) { + if (condition.getAsBoolean()) return; + Thread.sleep(25); + } + throw new IllegalStateException("timed out waiting for " + description + " after " + timeout); + } + + private static void awaitSuccess(ManagedProcess process, Duration timeout) throws Exception { + awaitExit(process, timeout); + if (process.exitValue() != 0) { + throw new IllegalStateException( + process.label() + " exited " + process.exitValue() + "; output: " + process.logFile()); + } + } + + private static void awaitExit(ManagedProcess process, Duration timeout) throws Exception { + if (!process.waitFor(timeout)) { + throw new IllegalStateException(process.label() + " did not exit within " + timeout); + } + } + + private static void stopGracefully(ManagedProcess process, Duration timeout) throws Exception { + if (process == null || !process.isAlive()) return; + process.destroy(); + if (!process.waitFor(timeout)) { + process.destroyForcibly(); + awaitExit(process, timeout); + } + } + + private static void destroyIfAlive(ManagedProcess process) { + if (process == null || !process.isAlive()) return; + process.destroyForcibly(); + try { + process.waitFor(Duration.ofSeconds(5)); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + + private static void verifyTrace(Path trace, long standbyPid) throws IOException { + var events = new ArrayList(); + for (var line : Files.readAllLines(trace, StandardCharsets.UTF_8)) { + if (!line.isBlank()) events.add(JSON.readTree(line)); + } + + var acceptIndexes = indexesOf(events, "nudge-accepted"); + require(!acceptIndexes.isEmpty(), "trace has no accepted nudge"); + for (var acceptIndex : acceptIndexes) { + var producerPid = events.get(acceptIndex).get("pid").asLong(); + require( + findAfter( + events, + acceptIndex, + "pump-run-start", + event -> event.get("pid").asLong() != producerPid) + >= 0, + "accepted nudge was not followed by a run in another OS process"); + } + + var accepted = requireEvent(events, "nudge-accepted", 1); + var leaderKilled = requireEvent(events, "leader-hard-killed", null); + var elected = requireEvent(events, "maintenance-elected", null); + var firstDrain = requireEvent(events, "work-drained", 1); + var firstRun = requireRun(events, firstDrain.event().get("jobId").asText()); + require( + accepted.index() < leaderKilled.index() + && leaderKilled.index() < firstRun.index() + && firstRun.index() < firstDrain.index(), + "accepted-nudge leader-kill ordering is not proven by the trace"); + require(leaderKilled.index() < elected.index(), "standby election was observed before the leader kill"); + require(elected.event().get("pid").asLong() == standbyPid, "maintenance ownership did not move to standby"); + require(firstRun.event().get("pid").asLong() == standbyPid, "accepted nudge was not served by the standby"); + require( + "nudge".equals(firstRun.event().get("origin").asText()), + "accepted nudge was served only by the backstop"); + + var secondRecorded = requireEvent(events, "work-recorded", 2); + var producerKilled = requireEvent(events, "producer-hard-killed-before-nudge", 2); + var secondDrain = requireEvent(events, "work-drained", 2); + var secondRun = requireRun(events, secondDrain.event().get("jobId").asText()); + require( + secondRecorded.index() < producerKilled.index() + && producerKilled.index() < secondRun.index() + && secondRun.index() < secondDrain.index(), + "producer crash-window ordering is not proven by the trace"); + require( + events.stream() + .noneMatch(event -> + "nudge-accepted".equals(event.path("event").asText()) + && event.path("sequence").asInt(-1) == 2), + "the hard-killed producer accepted a nudge unexpectedly"); + require( + "schedule".equals(secondRun.event().get("origin").asText()), + "producer crash-window row was not drained by the backstop schedule"); + } + + private static List indexesOf(List events, String eventName) { + var indexes = new ArrayList(); + for (int i = 0; i < events.size(); i++) { + if (eventName.equals(events.get(i).path("event").asText())) indexes.add(i); + } + return indexes; + } + + private static int findAfter( + List events, int afterIndex, String eventName, Predicate predicate) { + for (int i = afterIndex + 1; i < events.size(); i++) { + var event = events.get(i); + if (eventName.equals(event.path("event").asText()) && predicate.test(event)) return i; + } + return -1; + } + + private static IndexedEvent requireEvent(List events, String eventName, Integer sequence) { + for (int i = 0; i < events.size(); i++) { + var event = events.get(i); + if (!eventName.equals(event.path("event").asText())) continue; + if (sequence == null || event.path("sequence").asInt(-1) == sequence) { + return new IndexedEvent(i, event); + } + } + throw new IllegalStateException("trace has no " + eventName + (sequence == null ? "" : " for " + sequence)); + } + + private static IndexedEvent requireRun(List events, String jobId) { + for (int i = 0; i < events.size(); i++) { + var event = events.get(i); + if ("pump-run-start".equals(event.path("event").asText()) + && jobId.equals(event.path("jobId").asText())) { + return new IndexedEvent(i, event); + } + } + throw new IllegalStateException("trace has no pump run for job " + jobId); + } + + private static void require(boolean condition, String message) { + if (!condition) throw new IllegalStateException(message); + } + + private enum Role { + SUPERVISOR, + NODE, + PRODUCER + } + + private enum ProducerMode { + NUDGE, + WAIT_BEFORE_NUDGE + } + + private record Ready(NodeId nodeId, long pid) {} + + private record IndexedEvent(int index, JsonNode event) {} + + private record ManagedProcess(String label, Process process, Path logFile) { + long pid() { + return process.pid(); + } + + boolean isAlive() { + return process.isAlive(); + } + + void destroy() { + process.destroy(); + } + + void destroyForcibly() { + process.destroyForcibly(); + } + + boolean waitFor(Duration timeout) throws InterruptedException { + return process.waitFor(timeout.toMillis(), TimeUnit.MILLISECONDS); + } + + int exitValue() { + return process.exitValue(); + } + } + + private static final class Options { + private Role role = Role.SUPERVISOR; + private Backend backend = Backend.POSTGRES; + private String jdbcUrl; + private String databaseUser; + private String databasePassword; + private String redisUri; + private String runId; + private Path traceFile; + private String taskName; + private String queue; + private String label; + private boolean registerTask; + private Duration maintenancePoll; + private Path readyFile; + private int sequence; + private ProducerMode producerMode; + + private static Options parse(String[] args) { + var options = new Options(); + for (int i = 0; i < args.length; i += 2) { + var key = args[i]; + var value = i + 1 < args.length ? args[i + 1] : ""; + switch (key) { + case "--role" -> options.role = Role.valueOf(value.toUpperCase(Locale.ROOT)); + case "--backend" -> options.backend = Backend.parse(value); + case "--jdbc-url" -> options.jdbcUrl = value; + case "--db-user" -> options.databaseUser = value; + case "--db-password" -> options.databasePassword = value; + case "--redis-uri" -> options.redisUri = value; + case "--run-id" -> options.runId = value; + case "--trace" -> options.traceFile = Path.of(value); + case "--task" -> options.taskName = value; + case "--queue" -> options.queue = value; + case "--label" -> options.label = value; + case "--register-task" -> options.registerTask = Boolean.parseBoolean(value); + case "--maintenance-poll" -> options.maintenancePoll = parseDuration(value); + case "--ready" -> options.readyFile = Path.of(value); + case "--sequence" -> options.sequence = Integer.parseInt(value); + case "--producer-mode" -> + options.producerMode = ProducerMode.valueOf(value.toUpperCase(Locale.ROOT)); + default -> throw new IllegalArgumentException("unknown nudge simulation argument: " + key); + } + } + options.validate(); + return options; + } + + private void validate() { + if (role == Role.SUPERVISOR) return; + Objects.requireNonNull(runId, "--run-id"); + Objects.requireNonNull(traceFile, "--trace"); + Objects.requireNonNull(taskName, "--task"); + Objects.requireNonNull(queue, "--queue"); + Objects.requireNonNull(readyFile, "--ready"); + connectionInfo(); + if (role == Role.NODE) { + Objects.requireNonNull(label, "--label"); + Objects.requireNonNull(maintenancePoll, "--maintenance-poll"); + } else { + if (sequence <= 0) throw new IllegalArgumentException("--sequence must be positive"); + Objects.requireNonNull(producerMode, "--producer-mode"); + } + } + + private ConnectionInfo connectionInfo() { + return switch (backend) { + case POSTGRES -> + new ConnectionInfo( + backend, + Objects.requireNonNull(jdbcUrl, "--jdbc-url"), + Objects.requireNonNull(databaseUser, "--db-user"), + Objects.requireNonNull(databasePassword, "--db-password"), + null); + case REDIS -> + new ConnectionInfo(backend, null, null, null, Objects.requireNonNull(redisUri, "--redis-uri")); + }; + } + + private static Duration parseDuration(String value) { + if (value.endsWith("ms")) return Duration.ofMillis(Long.parseLong(value.substring(0, value.length() - 2))); + if (value.endsWith("s")) return Duration.ofSeconds(Long.parseLong(value.substring(0, value.length() - 1))); + return Duration.parse(value); + } + } +} diff --git a/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationPayload.java b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationPayload.java new file mode 100644 index 0000000..92b82b4 --- /dev/null +++ b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationPayload.java @@ -0,0 +1,17 @@ +package com.hemju.threadmill.simulation.nudge; + +import com.hemju.threadmill.core.handler.JobPayload; + +/** Payload for the process-separated nudge pump simulation. */ +public final class NudgeSimulationPayload implements JobPayload { + + public String runId; + public String traceFile; + + public NudgeSimulationPayload() {} + + public NudgeSimulationPayload(String runId, String traceFile) { + this.runId = runId; + this.traceFile = traceFile; + } +} diff --git a/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationStores.java b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationStores.java new file mode 100644 index 0000000..21f7d1f --- /dev/null +++ b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationStores.java @@ -0,0 +1,307 @@ +package com.hemju.threadmill.simulation.nudge; + +import java.sql.Connection; +import java.sql.SQLException; +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; +import java.util.Objects; + +import javax.sql.DataSource; + +import io.lettuce.core.RedisClient; +import io.lettuce.core.RedisURI; +import io.lettuce.core.ScriptOutputType; +import org.postgresql.ds.PGSimpleDataSource; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.containers.wait.strategy.Wait; +import org.testcontainers.postgresql.PostgreSQLContainer; +import org.testcontainers.utility.DockerImageName; + +import com.hemju.threadmill.core.store.JobStore; +import com.hemju.threadmill.store.postgres.MigrationRunner; +import com.hemju.threadmill.store.postgres.PostgresJobStore; +import com.hemju.threadmill.store.redis.RedisJobStore; + +/** Real-datastore fixtures and work-row adapters for the nudge simulation. */ +final class NudgeSimulationStores { + + private static final String POSTGRES_IMAGE = "postgres:18-alpine"; + private static final String REDIS_IMAGE = "redis:7-alpine"; + private static final String REDIS_DRAIN_SCRIPT = "local values = redis.call('SMEMBERS', KEYS[1]); " + + "if #values > 0 then redis.call('DEL', KEYS[1]); end; return values"; + private static volatile ConnectionInfo processConnectionInfo; + + private NudgeSimulationStores() {} + + enum Backend { + POSTGRES, + REDIS; + + static Backend parse(String value) { + return switch (value.toLowerCase(Locale.ROOT)) { + case "postgres", "postgresql", "pg" -> POSTGRES; + case "redis" -> REDIS; + default -> throw new IllegalArgumentException("backend must be postgres or redis, got: " + value); + }; + } + } + + record ConnectionInfo( + Backend backend, String jdbcUrl, String databaseUser, String databasePassword, String redisUri) { + + void appendArguments(List command) { + command.add("--backend"); + command.add(backend.name().toLowerCase(Locale.ROOT)); + if (backend == Backend.POSTGRES) { + command.add("--jdbc-url"); + command.add(jdbcUrl); + command.add("--db-user"); + command.add(databaseUser); + command.add("--db-password"); + command.add(databasePassword); + } else { + command.add("--redis-uri"); + command.add(redisUri); + } + } + } + + record BackendFixture(ConnectionInfo connectionInfo, AutoCloseable closeAction) implements AutoCloseable { + @Override + public void close() { + closeQuietly(closeAction, "backend fixture"); + } + } + + record JobStoreHandle(JobStore store, AutoCloseable closeAction) implements AutoCloseable { + @Override + public void close() { + closeQuietly(closeAction, "job store"); + } + } + + interface WorkStore extends AutoCloseable { + void prepare(); + + void record(int sequence); + + List drain(); + + boolean isPending(int sequence); + + @Override + void close(); + } + + @SuppressWarnings("resource") + static BackendFixture startBackend(Backend backend) { + return switch (backend) { + case POSTGRES -> { + var container = new PostgreSQLContainer(DockerImageName.parse(POSTGRES_IMAGE)) + .withDatabaseName("threadmill") + .withUsername("threadmill") + .withPassword("threadmill"); + container.start(); + var info = new ConnectionInfo( + backend, container.getJdbcUrl(), container.getUsername(), container.getPassword(), null); + new MigrationRunner(postgresDataSource(info)).migrate(); + yield new BackendFixture(info, container::stop); + } + case REDIS -> { + var container = new GenericContainer<>(DockerImageName.parse(REDIS_IMAGE)) + .withExposedPorts(6379) + .withCommand("redis-server", "--appendonly", "yes", "--maxmemory-policy", "noeviction") + .waitingFor(Wait.forListeningPort()); + container.start(); + var uri = "redis://" + container.getHost() + ":" + container.getMappedPort(6379); + yield new BackendFixture(new ConnectionInfo(backend, null, null, null, uri), container::stop); + } + }; + } + + static JobStoreHandle openJobStore(ConnectionInfo connectionInfo) { + return switch (connectionInfo.backend()) { + case POSTGRES -> new JobStoreHandle(new PostgresJobStore(postgresDataSource(connectionInfo)), null); + case REDIS -> { + var store = new RedisJobStore(RedisURI.create(connectionInfo.redisUri())); + yield new JobStoreHandle(store, store::close); + } + }; + } + + static WorkStore openWorkStore(ConnectionInfo connectionInfo, String runId) { + return switch (connectionInfo.backend()) { + case POSTGRES -> new PostgresWorkStore(postgresDataSource(connectionInfo), runId); + case REDIS -> new RedisWorkStore(RedisURI.create(connectionInfo.redisUri()), runId); + }; + } + + static void configureProcess(ConnectionInfo connectionInfo) { + processConnectionInfo = Objects.requireNonNull(connectionInfo, "connectionInfo"); + } + + static WorkStore openProcessWorkStore(String runId) { + var connectionInfo = processConnectionInfo; + if (connectionInfo == null) { + throw new IllegalStateException("nudge simulation process connection was not configured"); + } + return openWorkStore(connectionInfo, runId); + } + + private static DataSource postgresDataSource(ConnectionInfo connectionInfo) { + var dataSource = new PGSimpleDataSource(); + dataSource.setUrl(connectionInfo.jdbcUrl()); + dataSource.setUser(connectionInfo.databaseUser()); + dataSource.setPassword(connectionInfo.databasePassword()); + return dataSource; + } + + private static void closeQuietly(AutoCloseable closeAction, String description) { + if (closeAction == null) return; + try { + closeAction.close(); + } catch (Exception e) { + throw new IllegalStateException("failed to close nudge simulation " + description, e); + } + } + + private static final class PostgresWorkStore implements WorkStore { + private final DataSource dataSource; + private final String runId; + + private PostgresWorkStore(DataSource dataSource, String runId) { + this.dataSource = dataSource; + this.runId = runId; + } + + @Override + public void prepare() { + transaction(connection -> { + try (var statement = connection.createStatement()) { + statement.executeUpdate("CREATE TABLE IF NOT EXISTS threadmill_simulation_nudge_work (" + + "run_id text NOT NULL, sequence integer NOT NULL, " + + "recorded_at timestamptz NOT NULL DEFAULT clock_timestamp(), drained_at timestamptz, " + + "PRIMARY KEY (run_id, sequence))"); + } + return null; + }); + } + + @Override + public void record(int sequence) { + transaction(connection -> { + try (var statement = connection.prepareStatement( + "INSERT INTO threadmill_simulation_nudge_work (run_id, sequence) VALUES (?, ?)")) { + statement.setString(1, runId); + statement.setInt(2, sequence); + statement.executeUpdate(); + } + return null; + }); + } + + @Override + public List drain() { + return transaction(connection -> { + var drained = new ArrayList(); + try (var statement = connection.prepareStatement( + "UPDATE threadmill_simulation_nudge_work SET drained_at = clock_timestamp() " + + "WHERE run_id = ? AND drained_at IS NULL RETURNING sequence")) { + statement.setString(1, runId); + try (var result = statement.executeQuery()) { + while (result.next()) drained.add(result.getInt(1)); + } + } + return List.copyOf(drained); + }); + } + + @Override + public boolean isPending(int sequence) { + try (var connection = dataSource.getConnection(); + var statement = connection.prepareStatement("SELECT 1 FROM threadmill_simulation_nudge_work " + + "WHERE run_id = ? AND sequence = ? AND drained_at IS NULL")) { + statement.setString(1, runId); + statement.setInt(2, sequence); + try (var result = statement.executeQuery()) { + return result.next(); + } + } catch (SQLException e) { + throw new IllegalStateException("failed to inspect Postgres nudge simulation work", e); + } + } + + @Override + public void close() {} + + private T transaction(SqlWork work) { + try (var connection = dataSource.getConnection()) { + var previousAutoCommit = connection.getAutoCommit(); + connection.setAutoCommit(false); + try { + var result = work.execute(connection); + connection.commit(); + return result; + } catch (RuntimeException | SQLException e) { + connection.rollback(); + throw e; + } finally { + connection.setAutoCommit(previousAutoCommit); + } + } catch (SQLException e) { + throw new IllegalStateException("Postgres nudge simulation work transaction failed", e); + } + } + } + + @FunctionalInterface + private interface SqlWork { + T execute(Connection connection) throws SQLException; + } + + private static final class RedisWorkStore implements WorkStore { + private final RedisClient client; + private final String key; + + private RedisWorkStore(RedisURI redisUri, String runId) { + this.client = RedisClient.create(redisUri); + this.key = "{threadmill}:simulation:nudge:" + runId + ":pending"; + } + + @Override + public void prepare() { + try (var connection = client.connect()) { + connection.sync().del(key); + } + } + + @Override + public void record(int sequence) { + try (var connection = client.connect()) { + connection.sync().sadd(key, Integer.toString(sequence)); + } + } + + @Override + public List drain() { + try (var connection = client.connect()) { + List values = + connection.sync().eval(REDIS_DRAIN_SCRIPT, ScriptOutputType.MULTI, new String[] {key}); + return values.stream().map(Integer::valueOf).sorted().toList(); + } + } + + @Override + public boolean isPending(int sequence) { + try (var connection = client.connect()) { + return connection.sync().sismember(key, Integer.toString(sequence)); + } + } + + @Override + public void close() { + client.shutdown(); + } + } +} diff --git a/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationTrace.java b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationTrace.java new file mode 100644 index 0000000..bcb05da --- /dev/null +++ b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationTrace.java @@ -0,0 +1,57 @@ +package com.hemju.threadmill.simulation.nudge; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.channels.FileChannel; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.time.Instant; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; + +/** Cross-process JSON-lines trace used by the nudge correctness simulation. */ +final class NudgeSimulationTrace { + + private static final ObjectMapper JSON = new ObjectMapper(); + private static final ConcurrentHashMap PROCESS_LOCKS = new ConcurrentHashMap<>(); + + private NudgeSimulationTrace() {} + + static void append(Path path, String event, Map fields) { + var absolute = path.toAbsolutePath().normalize(); + var processLock = PROCESS_LOCKS.computeIfAbsent(absolute, ignored -> new Object()); + synchronized (processLock) { + appendLocked(absolute, event, fields); + } + } + + private static void appendLocked(Path path, String event, Map fields) { + try { + var parent = path.getParent(); + if (parent != null) Files.createDirectories(parent); + var bytes = line(event, fields).getBytes(StandardCharsets.UTF_8); + try (var channel = FileChannel.open( + path, StandardOpenOption.CREATE, StandardOpenOption.WRITE, StandardOpenOption.APPEND); + var lock = channel.lock()) { + if (!lock.isValid()) throw new IllegalStateException("trace lock is not valid: " + path); + channel.write(ByteBuffer.wrap(bytes)); + } + } catch (IOException e) { + throw new IllegalStateException("failed to append nudge simulation trace: " + path, e); + } + } + + private static String line(String event, Map fields) throws JsonProcessingException { + var document = new LinkedHashMap(); + document.put("timestamp", Instant.now().toString()); + document.put("event", event); + document.putAll(fields); + return JSON.writeValueAsString(document) + '\n'; + } +} From 54c9e12ec265a23bed66b9343e954b4625d7edd7 Mon Sep 17 00:00:00 2001 From: Helmut Michael Juskewycz Date: Tue, 11 Aug 2026 20:54:13 +0200 Subject: [PATCH 4/6] docs: reject recurring lifecycle generations Refs #113 --- AGENTS.md | 3 ++- CHANGELOG.md | 7 +++++++ .../hemju/threadmill/store/memory/InMemoryJobStore.java | 5 +++-- .../hemju/threadmill/store/postgres/PostgresJobStore.java | 5 +++-- .../com/hemju/threadmill/store/redis/RedisJobStore.java | 5 +++-- 5 files changed, 18 insertions(+), 7 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ece417e..ed86625 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -294,7 +294,8 @@ This section is the project's memory: the load-bearing decisions worth knowing b - **The shutdown requeue ordering is guaranteed by where `recordFailure` is called, and that is now pinned.** `JobRunner.run`'s catch block runs on the handler's own worker thread, so the `FAILED` save and `RetryInterceptor`'s `SCHEDULED` save are both emitted strictly after `handler.run` returned or threw — no peer can claim the job while user code is still executing. `ProcessingNode.close()` also drains (`workerPool.shutdown()` + `awaitTermination(grace)`) *before* `shutdownNow()`, and keeps maintenance alive through the drain so heartbeats stay fresh. Issue #110 item 1 proposed joining the handler thread before requeueing on the assumption this window was open; it is not, and for an uncooperative handler the proposal would have been worse (`recordFailure` is never reached, so nothing is requeued today). `ProcessingNodeTest.shutdownRequeueIsNeverPublishedWhileTheHandlerIsStillRunning` pins the ordering — never move the requeue onto another thread. - **Missed-run policy is a contract.** `DROP` (default) materialises only the latest fire on a tick. `CATCH_UP` (opt-in) materialises every missed fire. The choice is per-task. Tests cover both modes. - **Pile-up guard.** `RecurringMaterializer` refuses to materialise the next instance while the previous instance's `inFlightJobId` points to a non-terminal job. -- **Nudge = durable flag consumed by the materializer, never a bypass lane (issue #108).** `Scheduler.nudgeRecurring(name)` records `nudge_requested_at` plus a store-generated `nudge_revision` on the schedule state (one cell per task, so bursts coalesce structurally); the materializer's per-task tick observes it alongside `next_run_at`, materialises one instance through the normal machinery (pile-up guard applies; `next_run_at` untouched — cron grid and interval phase preserved), and clears it with a **compare-and-clear on the observed REVISION** — never the timestamp, whose finite store precision (Redis keeps epoch millis) can collide and let a clear erase a newer same-instant acceptance. The revision is strictly monotonic and never reset, so cleared identities cannot be reused. Ordering is materialize-then-clear: the coalescing bound ("current + one follow-up") is failure-free — a crash between them costs one extra run, never a lost one. A tick that materialises a due scheduled fire also satisfies an observed nudge (that instance starts after the nudge committed). **The materializer reloads the task definition under the mutex before acting** (`tick()` lists tasks before the per-task mutex, so an edit/disable can commit in between; materializing from the listed snapshot would insert the stale handler/payload and consume a nudge made against the new definition) — the reload happens only when a materialization is imminent so idle ticks stay at one state read. The nudge cells are written ONLY by `requestCronNudge` / `clearCronNudge` — `upsertCronTaskState` deliberately preserves them on every backend (Postgres: columns absent from the upsert; Redis: the DEL+HSET overwrite script carries both fields across; in-memory: merge) so blanket state writes cannot clobber a concurrent nudge. Acceptance is atomic with the existence + enabled check: Redis does it in one Lua script, Postgres in one `INSERT … SELECT FROM threadmill_cron_tasks WHERE enabled ON CONFLICT DO UPDATE` statement (error-free by construction — an FK-violation catch would poison a `join_transaction` caller's already-aborted host transaction), and the in-memory store under a single cron-lifecycle lock (separate-map removals allowed a delete/re-register ABA to strand an ACCEPTED nudge). Unknown task → `UNKNOWN_TASK` (a nudge racing removal cannot resurrect state); disabled task → `DISABLED`. **Enabled flips are ordered for crash-safety**: disabling persists the disabled task first, then clears (a crash leaves the nudge on a disabled task, which the materializer's enabled recheck refuses to run); re-enabling clears + recomputes state while still disabled and flips enabled LAST, so a crash mid-sequence re-detects the flip on retry and stale pre-pause demand can never become executable. The dashboard's `updateRecurring` decides the flip from a read taken INSIDE the task mutex (a pre-mutex snapshot could see a concurrent enable as still-disabled and wrongly clear a legitimate post-enable nudge). No transient signaling exists: latency is bounded by `maintenancePollInterval` (default 1 s). Producer-side, `NudgeCoalescer` single-flights nudge writes per task per scheduler instance — joiners share a follow-up write that *starts after they arrived* (never the in-flight write, whose commit could predate their own triggering commit), and follow-up generations run on a dedicated virtual thread so no caller is retained past its own covering write; **Spring nudges are after-commit in every enqueue mode, `join_transaction` included** — the one write that deliberately does not join the caller's transaction (`DeferredNudge`): coalescing is one cell per task, so a joined nudge holds that row's write lock for the whole business transaction and serializes every concurrent producer of that task, silently (correct at low rate, collapsing under load), to buy only the closing of a crash window the design explicitly does not need closed. Rollback semantics are identical either way. Spring callers address the task by handler class (`nudgeRecurring(OutboxPump.class)`): a `@Recurring` task's identity defaults to the fully-qualified class name, so the string overload breaks on renames; the string form stays for core-registered tasks where the caller owns the name. Dashboard `triggerRecurring` stays the separate operator force lane; all three materialization paths stamp `threadmill.cron.origin` (`schedule` / `nudge` / `manual`), surfaced via `JobSummary.cronOrigin` (visible on redacted reads — closed value set), a badge in the React console, and the cardinality-clamped `threadmill.jobs.recurring.runs{origin=…}` counter; nudged instances carry no `CRON_FIRE_TIME_META`. +- **Nudge = durable flag consumed by the materializer, never a bypass lane (issue #108).** `Scheduler.nudgeRecurring(name)` records `nudge_requested_at` plus a store-generated `nudge_revision` on the schedule state (one cell per task, so bursts coalesce structurally); the materializer's per-task tick observes it alongside `next_run_at`, materialises one instance through the normal machinery (pile-up guard applies; `next_run_at` untouched — cron grid and interval phase preserved), and clears it with a **compare-and-clear on the observed REVISION** — never the timestamp, whose finite store precision (Redis keeps epoch millis) can collide and let a clear erase a newer same-instant acceptance. The revision is strictly monotonic and never reset for the lifetime of the schedule-state row, so cleared identities cannot be reused while that task identity exists; delete plus same-name re-registration starts a new row at one (see the lifecycle-generation decision below). Ordering is materialize-then-clear: the coalescing bound ("current + one follow-up") is failure-free — a crash between them costs one extra run, never a lost one. A tick that materialises a due scheduled fire also satisfies an observed nudge (that instance starts after the nudge committed). **The materializer reloads the task definition under the mutex before acting** (`tick()` lists tasks before the per-task mutex, so an edit/disable can commit in between; materializing from the listed snapshot would insert the stale handler/payload and consume a nudge made against the new definition) — the reload happens only when a materialization is imminent so idle ticks stay at one state read. The nudge cells are written ONLY by `requestCronNudge` / `clearCronNudge` — `upsertCronTaskState` deliberately preserves them on every backend (Postgres: columns absent from the upsert; Redis: the DEL+HSET overwrite script carries both fields across; in-memory: merge) so blanket state writes cannot clobber a concurrent nudge. Acceptance is atomic with the existence + enabled check: Redis does it in one Lua script, Postgres in one `INSERT … SELECT FROM threadmill_cron_tasks WHERE enabled ON CONFLICT DO UPDATE` statement (error-free by construction — an FK-violation catch would poison a `join_transaction` caller's already-aborted host transaction), and the in-memory store under a single cron-lifecycle lock (separate-map removals allowed a delete/re-register ABA to strand an ACCEPTED nudge). Unknown task → `UNKNOWN_TASK` (a nudge racing removal cannot resurrect state); disabled task → `DISABLED`. **Enabled flips are ordered for crash-safety**: disabling persists the disabled task first, then clears (a crash leaves the nudge on a disabled task, which the materializer's enabled recheck refuses to run); re-enabling clears + recomputes state while still disabled and flips enabled LAST, so a crash mid-sequence re-detects the flip on retry and stale pre-pause demand can never become executable. The dashboard's `updateRecurring` decides the flip from a read taken INSIDE the task mutex (a pre-mutex snapshot could see a concurrent enable as still-disabled and wrongly clear a legitimate post-enable nudge). No transient signaling exists: latency is bounded by `maintenancePollInterval` (default 1 s). Producer-side, `NudgeCoalescer` single-flights nudge writes per task per scheduler instance — joiners share a follow-up write that *starts after they arrived* (never the in-flight write, whose commit could predate their own triggering commit), and follow-up generations run on a dedicated virtual thread so no caller is retained past its own covering write; **Spring nudges are after-commit in every enqueue mode, `join_transaction` included** — the one write that deliberately does not join the caller's transaction (`DeferredNudge`): coalescing is one cell per task, so a joined nudge holds that row's write lock for the whole business transaction and serializes every concurrent producer of that task, silently (correct at low rate, collapsing under load), to buy only the closing of a crash window the design explicitly does not need closed. Rollback semantics are identical either way. Spring callers address the task by handler class (`nudgeRecurring(OutboxPump.class)`): a `@Recurring` task's identity defaults to the fully-qualified class name, so the string overload breaks on renames; the string form stays for core-registered tasks where the caller owns the name. Dashboard `triggerRecurring` stays the separate operator force lane; all three materialization paths stamp `threadmill.cron.origin` (`schedule` / `nudge` / `manual`), surfaced via `JobSummary.cronOrigin` (visible on redacted reads — closed value set), a badge in the React console, and the cardinality-clamped `threadmill.jobs.recurring.runs{origin=…}` counter; nudged instances carry no `CRON_FIRE_TIME_META`. +- **Per-task lifecycle generations are deliberately not part of the store SPI (issue #113).** The process-separated Postgres / Redis simulations from issue #114 reach maintenance-lease expiry and takeover after a hard-killed leader: an accepted nudge survives durably and the standby serves it, while a producer killed before its nudge is recovered by the regular schedule. A hard-killed materializer cannot later issue a stale clear. The remaining revision-reuse ABA requires a materially narrower sequence: a materializer reads an old revision, becomes suspended for longer than the 30-second task-mutex lease without dying, the task is deleted and re-registered under the same name, a new acceptance reaches the reused revision, and that same old materializer resumes and clears it. That could suppress the new nudge's latency/run-after-accept guarantee until the regular backstop, but it does not erase the producer's durable work. Preventing it needs a per-name generation or revision high-water record that survives deletion (and therefore needs an explicit, effectively unbounded retention policy), plus storage/migration work and a changed compare-and-clear contract for every `JobStore` implementation. It still would not replace the load-bearing enabled-flip or task/state deletion orderings: those must remain correct to prevent disabled execution and orphan state independently. The added mechanism would therefore protect and diagnose only this lease-expiry + same-name-reuse edge, below the bar for another cross-backend SPI mechanism. Revisit only with evidence of a suspended-then-resumed materializer reaching this sequence or with a broader durable-identity requirement; until then, use a new task name instead of delete-and-immediate-reuse where that risk is unacceptable. - **Recurring ownership reconciliation is namespace-scoped.** `Scheduler.reconcileRecurring(namespace, desiredTasks)` upserts the desired definitions and deletes only tasks previously recorded as owned by that namespace. Spring annotation-driven recurring defaults the namespace from `threadmill.spring.recurring-namespace`, then `spring.application.name`; without either, startup only upserts discovered tasks and leaves stale cleanup manual. - **Per-queue lanes are the starvation fix.** A `ProcessingNode` builder can declare multiple `QueueLane(name, workers)` entries; each gets its own `Dispatcher` with its own `Semaphore`. The `Scheduler.SYSTEM_QUEUE = "system"` constant is the canonical home for recurring / system jobs that must not be starved. - **Queue-family lanes discover active queues.** `ProcessingNode.Builder.lane(pattern, workers, QueueWeights)` creates one shared-capacity lane for queues matching anchored `*` / `?` patterns. Weights are resolved once per discovery cadence, zero pauses a queue, and empty queues remain in the working set until `queueFamilyRetentionAfterEmpty` to avoid bursty rediscovery churn. diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d21226..0be5ec3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,13 @@ work write but before its nudge and prove the regular recurring backstop drains the row. Cross-process JSON-lines traces record process ids, leadership, trigger origins, and the verified event ordering. +- Recorded the issue #113 decision not to add per-task lifecycle generations + to the `JobStore` SPI. Issue #114 proves hard-kill handoff and backstop + recovery, while the remaining revision-reuse ABA additionally requires an + old materializer to resume after outliving its mutex lease and a same-name + delete/re-registration. A persistent generation would not replace the + existing lifecycle orderings and does not justify its cross-backend storage, + retention, migration, and SPI cost without evidence that sequence occurs. - Recurring tasks can declare claim-time exclusivity (issue #110). A `CronTask` gains an `exclusive` flag, surfaced as `@Recurring(exclusive = true)` and as an `exclusive` parameter on diff --git a/threadmill-store-memory/src/main/java/com/hemju/threadmill/store/memory/InMemoryJobStore.java b/threadmill-store-memory/src/main/java/com/hemju/threadmill/store/memory/InMemoryJobStore.java index e2439ef..cd98bf7 100644 --- a/threadmill-store-memory/src/main/java/com/hemju/threadmill/store/memory/InMemoryJobStore.java +++ b/threadmill-store-memory/src/main/java/com/hemju/threadmill/store/memory/InMemoryJobStore.java @@ -847,8 +847,9 @@ public NudgeOutcome requestCronNudge(String taskName, Instant requestedAt) { public void clearCronNudge(String taskName, long observedRevision) { Names.requireName("cronTask", taskName); synchronized (cronLifecycleLock) { - // Clear the pending flag only; the revision is never reset, so a - // later acceptance can never reuse a cleared identity. + // Clear the pending flag only; the revision is never reset while + // this state row exists, so a later acceptance in the same task + // lifecycle cannot reuse a cleared identity. cronTaskStates.computeIfPresent( taskName, (name, prev) -> prev.nudgeRevision() != null diff --git a/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/PostgresJobStore.java b/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/PostgresJobStore.java index 58c980b..c1c9884 100644 --- a/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/PostgresJobStore.java +++ b/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/PostgresJobStore.java @@ -1892,8 +1892,9 @@ public NudgeOutcome requestCronNudge(String taskName, Instant requestedAt) { // and the no-state-row-yet window between upsertCron's two // writes. Sourcing the INSERT from the task row enforces // existence + enabled in the same statement. The revision - // advances on every acceptance and never resets, giving - // compare-and-clear a collision-free identity. + // advances on every acceptance and never resets while the + // schedule-state row exists, giving compare-and-clear a + // collision-free identity within one task lifecycle. for (int attempt = 0; attempt < 3; attempt++) { try (PreparedStatement ps = conn.prepareStatement( "INSERT INTO threadmill_cron_task_state (task_name, nudge_requested_at, nudge_revision) " diff --git a/threadmill-store-redis/src/main/java/com/hemju/threadmill/store/redis/RedisJobStore.java b/threadmill-store-redis/src/main/java/com/hemju/threadmill/store/redis/RedisJobStore.java index 9e850cc..1199bd8 100644 --- a/threadmill-store-redis/src/main/java/com/hemju/threadmill/store/redis/RedisJobStore.java +++ b/threadmill-store-redis/src/main/java/com/hemju/threadmill/store/redis/RedisJobStore.java @@ -1909,8 +1909,9 @@ public NudgeOutcome requestCronNudge(String taskName, Instant requestedAt) { // cluster slot): the existence + enabled check and the nudge write // cannot race a concurrent deleteCronTask, so a nudge can never // resurrect schedule state for a removed task. HINCRBY generates the - // strictly monotonic, never-reset revision that compare-and-clear - // uses as its collision-free identity. Always-string return + // strictly monotonic revision that compare-and-clear uses as its + // collision-free identity while this task's state hash exists. + // Always-string return // -> ScriptOutputType.VALUE per the Lua return-value conventions. try { String outcome = evalScript( From 02f5f758055957fd870345a315676761dc7a13a5 Mon Sep 17 00:00:00 2001 From: Helmut Michael Juskewycz Date: Wed, 12 Aug 2026 09:01:36 +0200 Subject: [PATCH 5/6] fix: address PR review findings --- AGENTS.md | 17 +- CHANGELOG.md | 27 +- build.gradle.kts | 8 +- .../core/schedule/CronTaskScheduleState.java | 10 +- .../core/schedule/RecurringMaterializer.java | 46 +- threadmill-simulation/README.md | 10 +- .../simulation/nudge/NudgeSimulationMain.java | 108 ++-- .../nudge/NudgeSimulationStores.java | 68 +-- .../nudge/NudgeSimulationTrace.java | 3 +- .../workerchurn/WorkerChurnTraceLog.java | 5 +- .../store/memory/SchedulingTest.java | 196 ++++++++ threadmill-store-postgres/README.md | 7 +- .../store/postgres/MigrationRunner.java | 85 ++-- .../store/postgres/PostgresJobStore.java | 469 ++++++++---------- .../postgres/NonAutoCommitDataSource.java | 93 ++++ .../PostgresJobStoreContractTest.java | 9 +- .../PostgresJobStoreRegressionTest.java | 96 ++-- .../threadmill/store/redis/RedisJobStore.java | 5 +- 18 files changed, 775 insertions(+), 487 deletions(-) create mode 100644 threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/NonAutoCommitDataSource.java diff --git a/AGENTS.md b/AGENTS.md index ed86625..53d8218 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -151,7 +151,7 @@ The build uses Gradle (≥ 9.5) and the project's Java 25 toolchain. - `./gradlew :threadmill-simulation:simulateWorkerChurnPostgres` / `simulateWorkerChurnRedis` — run worker-process churn simulations against shared local datastores. - `./gradlew :threadmill-simulation:simulateNudgePostgres` / `simulateNudgeRedis` — run the fixed process-separated nudge simulation with a hard-killed maintenance leader and a producer killed between its durable work write and nudge. `simulateNudge` runs both real backends. - `./gradlew :threadmill-example:run` — compile and run the public getting-started example. -- `./gradlew productionCheck` — release-candidate validation: clean, check, Javadoc, real store tests, soak, dependency scan hook, example, and artifact inspection. +- `./gradlew productionCheck` — release-candidate validation: clean, check, Javadoc, real store tests, soak, short correctness and process-separated nudge simulations, dependency scan hook, example, and artifact inspection. - Integration tests for the real backends use **Testcontainers** and need a working container runtime (Docker / Podman / Colima / OrbStack). The Gradle wrapper is committed; new clones run `./gradlew` without a system Gradle install. @@ -237,7 +237,7 @@ This section is the project's memory: the load-bearing decisions worth knowing b - **Per-state counts come from `threadmill_job_counts`, maintained by an AFTER INSERT/UPDATE/DELETE trigger — and the counter rows are sharded (V2).** One row per state serialized every concurrent writer on that row's lock: a 16-producer stress run collapsed to ~13 jobs/s with `pg_stat_activity` full of `Lock:transactionid` waits on the counter updates (claims hold the row for their whole long transaction). Each state now has 16 shard rows; the trigger updates the shard picked by `pg_backend_pid() % 16`, and `countsByState()` reads `SUM(count) GROUP BY state`. Individual shard rows may legitimately go negative — only the sum is meaningful. Same stress shape after sharding: ~162 jobs/s. Never write a `COUNT(*)`-over-`threadmill_jobs` query; the `perStateCountsReadFromCounterTableNotFromJobsTable` test uses `EXPLAIN` to assert the plan does not touch the jobs table. - **Migration runner bootstraps `threadmill_schema_history` itself** (`CREATE IF NOT EXISTS`). Migration SQL must not recreate it. Add new migrations as `V__.sql` under `src/main/resources/com/hemju/threadmill/store/postgres/migrations/` and register them in `MigrationRunner.SHIPPED_MIGRATIONS` (the runner does not classpath-scan; the explicit list is intentional for native-image compatibility). - **Deadlocks on busy queue tables are normal.** Every write goes through `DeadlockRetry.run(...)` (recognises SQLSTATE `40P01` / `40001`, exponential backoff with jitter). -- **Self-owned Postgres writes never rely on the pool's `autoCommit` default.** Operations that open their own connection route through `PostgresJobStore.ownedTransaction`, which starts, commits or rolls back, and restores the connection's prior mode. This applies even to single-statement heartbeat, lease, mutex, queue-pause, retention, recurring-definition, and check-in writes: a pool configured with `autoCommit=false` otherwise accepts the statement and silently rolls it back on release. Helpers that receive a caller's `Connection` stay boundary-agnostic; their caller owns the transaction. +- **Self-owned Postgres writes never rely on the pool's `autoCommit` default.** Every operation that opens its own write connection routes through `PostgresJobStore.ownedTransaction`, which starts, commits or rolls back, and restores the connection's prior mode; this includes the multi-statement `saveAtomic`, `softDelete`, `claimReady`, and `replaceJob` paths as well as single-statement heartbeat, lease, mutex, queue-pause, retention, recurring-definition, and check-in writes. `MigrationRunner` uses an equivalent explicit boundary for history bootstrap, each migration, and destructive schema reset. A pool configured with `autoCommit=false` otherwise accepts the statements and silently rolls them back on release; forcing `autoCommit=true` on return is the mirror-image defect. The full Postgres store contract runs through a mode-guarding non-auto-commit `DataSource`. Helpers that receive a caller's `Connection` stay boundary-agnostic; their caller owns the transaction. - **Testcontainers ≥ 2.0.** Module names use the `testcontainers-` prefix. `PostgreSQLContainer` lives in `org.testcontainers.postgresql` and is non-generic. - **The host owns the connection pool.** The store accepts a `javax.sql.DataSource`; it does not create or close one. - **Spring Boot Postgres schema handling is explicit.** Auto-configured Postgres stores run `threadmill.store.postgres.schema-mode=migrate` by default before constructing `PostgresJobStore`. `validate` is for externally-applied DDL, `none` skips schema handling, and `drop-and-migrate` requires `threadmill.store.postgres.allow-destructive-schema-reset=true` because it destroys Threadmill job data. @@ -285,8 +285,8 @@ This section is the project's memory: the load-bearing decisions worth knowing b ### Scheduling -- **Identity vs schedule-state for recurring.** `CronTask` is identity; `CronTaskScheduleState` is bookkeeping. `Scheduler.upsertCron` preserves the schedule state — including an overdue `nextRunAt` and an interval trigger's phase — while the re-registered schedule is unchanged (no disabled→enabled flip), so restart-missed firings stay observable and `MissedRunPolicy` decides their fate; it recomputes `nextRunAt` from now only on a real timing edit, so a freshly edited cron never fires stale times (issue #105: the original unconditional recompute — the pre-release catch-up-storm fix — wiped restart-missed firings before `CATCH_UP` could ever see them; storm safety now lives in the materializer, where `DROP` collapses the backlog to one run and `CATCH_UP` is capped per tick with carry-over). The unchanged-schedule decision reads `CronTaskScheduleState.timingFingerprint` — a canonical string of the trigger (plus zone for cron triggers only; an interval's zone is documented-ignored and deliberately excluded) written atomically with `nextRunAt` in the state record. Never decide preserve-vs-recompute by comparing the stored task definition against the new one: the task and state are two separate store writes, and a crash between them would pair a new trigger with old timing undetectably — the fingerprint makes the retry detect the mismatch and recompute (null fingerprint = legacy row = recompute once). `CronExpression` carries source-based value equality; never regress it to reference identity. `inFlightJobId` is preserved on every path. Dashboard `updateRecurring` intentionally recomputes timing on every edit — an operator editing a task expects it to schedule forward from the edit. -- **A materializer-observed timing-fingerprint mismatch completes the edit without firing the old schedule (issue #112).** In the under-mutex reload, the durable `CronTask` definition is authoritative: `RecurringMaterializer` recomputes `nextRunAt` forward from that tick, stamps the current fingerprint, preserves last-run / in-flight bookkeeping and any nudge, and produces no scheduled instance from the stale timing. Preserving overdue state applies only while its fingerprint matches the current definition; a mismatch is the crashed-edit signature, so materializing once would run a trigger the user already replaced, while merely skipping would leave the task dormant until a future re-registration. For `CATCH_UP`, the obsolete trigger's backlog is intentionally discarded and catch-up resumes normally from the repaired current-trigger timing. A pending nudge remains independent demand and may materialize immediately against the repaired definition. +- **Identity vs schedule-state for recurring.** `CronTask` is identity; `CronTaskScheduleState` is bookkeeping. `Scheduler.upsertCron` preserves the schedule state — including an overdue `nextRunAt` and an interval trigger's phase — while the re-registered schedule is unchanged (no disabled→enabled flip), so restart-missed firings stay observable and `MissedRunPolicy` decides their fate; it recomputes `nextRunAt` from now only on a real timing edit, so a freshly edited cron never fires stale times (issue #105: the original unconditional recompute — the pre-release catch-up-storm fix — wiped restart-missed firings before `CATCH_UP` could ever see them; storm safety now lives in the materializer, where `DROP` collapses the backlog to one run and `CATCH_UP` is capped per tick with carry-over). The unchanged-schedule decision reads `CronTaskScheduleState.timingFingerprint` — a canonical string of the trigger (plus zone for cron triggers only; an interval's zone is documented-ignored and deliberately excluded) written atomically with `nextRunAt` in the state record. Never decide preserve-vs-recompute by comparing the stored task definition against the new one: the task and state are two separate store writes, and a crash between them would pair a new trigger with old timing undetectably — the fingerprint makes the retry detect a non-null mismatch and recompute. A null fingerprint marks a legacy/public-API row rather than proving an edit: the materializer adopts the current fingerprint without moving a non-null `nextRunAt`, or initializes timing from the tick only when `nextRunAt` is also null. `CronExpression` carries source-based value equality; never regress it to reference identity. `inFlightJobId` is preserved on every path. Dashboard `updateRecurring` intentionally recomputes timing on every edit — an operator editing a task expects it to schedule forward from the edit. +- **A materializer-observed timing-fingerprint mismatch completes the edit without firing the old schedule (issue #112).** A mismatch between the listed definition and schedule state is itself a reason to enter the under-mutex reload, even when stale `nextRunAt` is still in the future and no nudge exists; otherwise replacing a weekly trigger with a one-minute interval could remain dormant until the obsolete weekly fire. In the reload, the durable `CronTask` definition is authoritative: `RecurringMaterializer` recomputes `nextRunAt` forward from that tick, stamps the current fingerprint, preserves last-run / in-flight bookkeeping and any nudge, and produces no scheduled instance from the stale timing. Preserving overdue state applies only while its fingerprint matches the current definition; a mismatch is the crashed-edit signature, so materializing once would run a trigger the user already replaced, while merely skipping would leave the task dormant until a future re-registration. For `CATCH_UP`, the obsolete trigger's backlog is intentionally discarded and catch-up resumes normally from the repaired current-trigger timing. A pending nudge remains independent demand and may materialize immediately against the repaired definition. - **`DROP` recovery is phase-exact and nominally stamped.** `RecurringMaterializer` collapses a missed backlog into one instance for the most recent *nominal* fire (`latestFireAtOrBefore`: computed arithmetically for intervals so a tiny interval with a huge backlog cannot spin the maintenance thread; fire-by-fire for cron), stamps `CRON_FIRE_TIME_META` with that nominal time, and advances `nextRunAt` from the nominal fire — so an interval's phase never drifts (due 06:00, recovered 07:00 → next 12:00, not 13:00). Never materialize the DROP recovery at `now` or advance the schedule from `now`. - **Recurring tasks carry their per-instance overrides on the definition.** `CronTask.timeout` (nullable = engine global, whole seconds, rejected below 1s) and `CronTask.maxAttempts` (nullable = `RetryInterceptor` defaults, rejected below 1) are stamped onto every materialized instance as `JobRunner.META_TIMEOUT_SECONDS` / `RetryInterceptor.META_MAX_ATTEMPTS` — by `RecurringMaterializer.materialize` and by the dashboard's manual trigger — so `@Job(timeout)` and `@Job(maxAttempts)` on a `@Recurring` handler behave identically to the enqueue path (issue #84: both used to be silently ignored and every recurring instance ran under the engine defaults; the annotation attribute was named `maxRetries` before v0.1.4 — issue #104 renamed it because the value always counted total attempts). Postgres persists them as `threadmill_cron_tasks.timeout_seconds` / `max_attempts` (V2 migration); Redis as hash fields whose upsert has overwrite semantics so an override-less re-registration clears them. The dashboard's `updateRecurring` rebuilds the `CronTask` field-by-field and must keep preserving all three (see the exclusivity note below). - **Recurring exclusivity is claim-time admission, not a materializer check.** `CronTask.exclusive` (issue #110) makes `RecurringMaterializer.materialize` **and** the dashboard's manual trigger stamp `concurrencyKey = CronTask.concurrencyKeyFor(name)` (`recurring:`, truncated on a code-point boundary with a stable hash suffix past the 256-UTF-8-byte cap) with `ConcurrencyMode.EXCLUSIVE`. The key is derived, never user-supplied, so the `recurring:` namespace cannot collide with an application's own keys. This is deliberately stronger than the pile-up guard: the guard only decides what to materialize on the maintenance leader, while admission is enforced by every store on every node, so it also covers a manual trigger racing a scheduled instance and the retry-handoff window. It does **not** cover reclaim — the terminal failure save releases the slot — and that limitation is documented on the feature, on `@Recurring(exclusive)`, and in `docs/transactions.md`. Persisted as a Postgres column (V6) and a Redis hash field with the same overwrite-on-re-upsert semantics as the timeout/attempt overrides. Both Spring registration paths carry it: the namespaced `reconcileRecurring` path via `taskFor`, and the un-namespaced path via `Scheduler.defineRecurring` — the latter was missed on the first cut and caught by `ThreadmillAutoConfigurationTest`. @@ -294,8 +294,8 @@ This section is the project's memory: the load-bearing decisions worth knowing b - **The shutdown requeue ordering is guaranteed by where `recordFailure` is called, and that is now pinned.** `JobRunner.run`'s catch block runs on the handler's own worker thread, so the `FAILED` save and `RetryInterceptor`'s `SCHEDULED` save are both emitted strictly after `handler.run` returned or threw — no peer can claim the job while user code is still executing. `ProcessingNode.close()` also drains (`workerPool.shutdown()` + `awaitTermination(grace)`) *before* `shutdownNow()`, and keeps maintenance alive through the drain so heartbeats stay fresh. Issue #110 item 1 proposed joining the handler thread before requeueing on the assumption this window was open; it is not, and for an uncooperative handler the proposal would have been worse (`recordFailure` is never reached, so nothing is requeued today). `ProcessingNodeTest.shutdownRequeueIsNeverPublishedWhileTheHandlerIsStillRunning` pins the ordering — never move the requeue onto another thread. - **Missed-run policy is a contract.** `DROP` (default) materialises only the latest fire on a tick. `CATCH_UP` (opt-in) materialises every missed fire. The choice is per-task. Tests cover both modes. - **Pile-up guard.** `RecurringMaterializer` refuses to materialise the next instance while the previous instance's `inFlightJobId` points to a non-terminal job. -- **Nudge = durable flag consumed by the materializer, never a bypass lane (issue #108).** `Scheduler.nudgeRecurring(name)` records `nudge_requested_at` plus a store-generated `nudge_revision` on the schedule state (one cell per task, so bursts coalesce structurally); the materializer's per-task tick observes it alongside `next_run_at`, materialises one instance through the normal machinery (pile-up guard applies; `next_run_at` untouched — cron grid and interval phase preserved), and clears it with a **compare-and-clear on the observed REVISION** — never the timestamp, whose finite store precision (Redis keeps epoch millis) can collide and let a clear erase a newer same-instant acceptance. The revision is strictly monotonic and never reset for the lifetime of the schedule-state row, so cleared identities cannot be reused while that task identity exists; delete plus same-name re-registration starts a new row at one (see the lifecycle-generation decision below). Ordering is materialize-then-clear: the coalescing bound ("current + one follow-up") is failure-free — a crash between them costs one extra run, never a lost one. A tick that materialises a due scheduled fire also satisfies an observed nudge (that instance starts after the nudge committed). **The materializer reloads the task definition under the mutex before acting** (`tick()` lists tasks before the per-task mutex, so an edit/disable can commit in between; materializing from the listed snapshot would insert the stale handler/payload and consume a nudge made against the new definition) — the reload happens only when a materialization is imminent so idle ticks stay at one state read. The nudge cells are written ONLY by `requestCronNudge` / `clearCronNudge` — `upsertCronTaskState` deliberately preserves them on every backend (Postgres: columns absent from the upsert; Redis: the DEL+HSET overwrite script carries both fields across; in-memory: merge) so blanket state writes cannot clobber a concurrent nudge. Acceptance is atomic with the existence + enabled check: Redis does it in one Lua script, Postgres in one `INSERT … SELECT FROM threadmill_cron_tasks WHERE enabled ON CONFLICT DO UPDATE` statement (error-free by construction — an FK-violation catch would poison a `join_transaction` caller's already-aborted host transaction), and the in-memory store under a single cron-lifecycle lock (separate-map removals allowed a delete/re-register ABA to strand an ACCEPTED nudge). Unknown task → `UNKNOWN_TASK` (a nudge racing removal cannot resurrect state); disabled task → `DISABLED`. **Enabled flips are ordered for crash-safety**: disabling persists the disabled task first, then clears (a crash leaves the nudge on a disabled task, which the materializer's enabled recheck refuses to run); re-enabling clears + recomputes state while still disabled and flips enabled LAST, so a crash mid-sequence re-detects the flip on retry and stale pre-pause demand can never become executable. The dashboard's `updateRecurring` decides the flip from a read taken INSIDE the task mutex (a pre-mutex snapshot could see a concurrent enable as still-disabled and wrongly clear a legitimate post-enable nudge). No transient signaling exists: latency is bounded by `maintenancePollInterval` (default 1 s). Producer-side, `NudgeCoalescer` single-flights nudge writes per task per scheduler instance — joiners share a follow-up write that *starts after they arrived* (never the in-flight write, whose commit could predate their own triggering commit), and follow-up generations run on a dedicated virtual thread so no caller is retained past its own covering write; **Spring nudges are after-commit in every enqueue mode, `join_transaction` included** — the one write that deliberately does not join the caller's transaction (`DeferredNudge`): coalescing is one cell per task, so a joined nudge holds that row's write lock for the whole business transaction and serializes every concurrent producer of that task, silently (correct at low rate, collapsing under load), to buy only the closing of a crash window the design explicitly does not need closed. Rollback semantics are identical either way. Spring callers address the task by handler class (`nudgeRecurring(OutboxPump.class)`): a `@Recurring` task's identity defaults to the fully-qualified class name, so the string overload breaks on renames; the string form stays for core-registered tasks where the caller owns the name. Dashboard `triggerRecurring` stays the separate operator force lane; all three materialization paths stamp `threadmill.cron.origin` (`schedule` / `nudge` / `manual`), surfaced via `JobSummary.cronOrigin` (visible on redacted reads — closed value set), a badge in the React console, and the cardinality-clamped `threadmill.jobs.recurring.runs{origin=…}` counter; nudged instances carry no `CRON_FIRE_TIME_META`. -- **Per-task lifecycle generations are deliberately not part of the store SPI (issue #113).** The process-separated Postgres / Redis simulations from issue #114 reach maintenance-lease expiry and takeover after a hard-killed leader: an accepted nudge survives durably and the standby serves it, while a producer killed before its nudge is recovered by the regular schedule. A hard-killed materializer cannot later issue a stale clear. The remaining revision-reuse ABA requires a materially narrower sequence: a materializer reads an old revision, becomes suspended for longer than the 30-second task-mutex lease without dying, the task is deleted and re-registered under the same name, a new acceptance reaches the reused revision, and that same old materializer resumes and clears it. That could suppress the new nudge's latency/run-after-accept guarantee until the regular backstop, but it does not erase the producer's durable work. Preventing it needs a per-name generation or revision high-water record that survives deletion (and therefore needs an explicit, effectively unbounded retention policy), plus storage/migration work and a changed compare-and-clear contract for every `JobStore` implementation. It still would not replace the load-bearing enabled-flip or task/state deletion orderings: those must remain correct to prevent disabled execution and orphan state independently. The added mechanism would therefore protect and diagnose only this lease-expiry + same-name-reuse edge, below the bar for another cross-backend SPI mechanism. Revisit only with evidence of a suspended-then-resumed materializer reaching this sequence or with a broader durable-identity requirement; until then, use a new task name instead of delete-and-immediate-reuse where that risk is unacceptable. +- **Nudge = durable flag consumed by the materializer, never a bypass lane (issue #108).** `Scheduler.nudgeRecurring(name)` records `nudge_requested_at` plus a store-generated `nudge_revision` on the schedule state (one cell per task, so bursts coalesce structurally); the materializer's per-task tick observes it alongside `next_run_at`, materialises one instance through the normal machinery (pile-up guard applies; `next_run_at` untouched — cron grid and interval phase preserved), and clears it with a **compare-and-clear on the observed REVISION** — never the timestamp, whose finite store precision (Redis keeps epoch millis) can collide and let a clear erase a newer same-instant acceptance. The revision is strictly monotonic and never reset for the lifetime of the schedule-state row, so cleared identities cannot be reused while that task identity exists; delete plus same-name re-registration starts a new row at one (see the lifecycle-generation decision below). Ordering is materialize-then-clear: the coalescing bound ("current + one follow-up") is failure-free — a crash between them costs one extra run, never a lost one. A tick that materialises a due scheduled fire also satisfies an observed nudge (that instance starts after the nudge committed). **The materializer reloads the task definition under the mutex before acting** (`tick()` lists tasks before the per-task mutex, so an edit/disable can commit in between; materializing from the listed snapshot would insert the stale handler/payload and consume a nudge made against the new definition) — the reload happens when materialization is imminent or the listed definition already proves the timing state stale, so ordinary idle ticks stay at one state read while crashed future timing edits self-heal. The nudge cells are written ONLY by `requestCronNudge` / `clearCronNudge` — `upsertCronTaskState` deliberately preserves them on every backend (Postgres: columns absent from the upsert; Redis: the DEL+HSET overwrite script carries both fields across; in-memory: merge) so blanket state writes cannot clobber a concurrent nudge. Acceptance is atomic with the existence + enabled check: Redis does it in one Lua script, Postgres in one `INSERT … SELECT FROM threadmill_cron_tasks WHERE enabled ON CONFLICT DO UPDATE` statement (error-free by construction — an FK-violation catch would poison a `join_transaction` caller's already-aborted host transaction), and the in-memory store under a single cron-lifecycle lock (separate-map removals allowed a delete/re-register ABA to strand an ACCEPTED nudge). Unknown task → `UNKNOWN_TASK` (a nudge racing removal cannot resurrect state); disabled task → `DISABLED`. **Enabled flips are ordered for crash-safety**: disabling persists the disabled task first, then clears (a crash leaves the nudge on a disabled task, which the materializer's enabled recheck refuses to run); re-enabling clears + recomputes state while still disabled and flips enabled LAST, so a crash mid-sequence re-detects the flip on retry and stale pre-pause demand can never become executable. The dashboard's `updateRecurring` decides the flip from a read taken INSIDE the task mutex (a pre-mutex snapshot could see a concurrent enable as still-disabled and wrongly clear a legitimate post-enable nudge). No transient signaling exists: latency is bounded by `maintenancePollInterval` (default 1 s). Producer-side, `NudgeCoalescer` single-flights nudge writes per task per scheduler instance — joiners share a follow-up write that *starts after they arrived* (never the in-flight write, whose commit could predate their own triggering commit), and follow-up generations run on a dedicated virtual thread so no caller is retained past its own covering write; **Spring nudges are after-commit in every enqueue mode, `join_transaction` included** — the one write that deliberately does not join the caller's transaction (`DeferredNudge`): coalescing is one cell per task, so a joined nudge holds that row's write lock for the whole business transaction and serializes every concurrent producer of that task, silently (correct at low rate, collapsing under load), to buy only the closing of a crash window the design explicitly does not need closed. Rollback semantics are identical either way. Spring callers address the task by handler class (`nudgeRecurring(OutboxPump.class)`): a `@Recurring` task's identity defaults to the fully-qualified class name, so the string overload breaks on renames; the string form stays for core-registered tasks where the caller owns the name. Dashboard `triggerRecurring` stays the separate operator force lane; all three materialization paths stamp `threadmill.cron.origin` (`schedule` / `nudge` / `manual`), surfaced via `JobSummary.cronOrigin` (visible on redacted reads — closed value set), a badge in the React console, and the cardinality-clamped `threadmill.jobs.recurring.runs{origin=…}` counter; nudged instances carry no `CRON_FIRE_TIME_META`. +- **Per-task lifecycle generations are deliberately not part of the store SPI (issue #113).** The process-separated Postgres / Redis simulations from issue #114 reach maintenance-lease expiry and takeover after a hard-killed leader: an accepted nudge survives durably and the standby serves it, while a producer killed before its nudge is recovered by the regular schedule. A hard-killed materializer cannot later issue a stale clear. The remaining revision-reuse ABA requires a materially narrower sequence: a materializer reads an old revision, becomes suspended for longer than the 30-second task-mutex lease without dying, the task is deleted and re-registered under the same name, a new acceptance reaches the reused revision, and that same old materializer resumes and clears it. That could suppress the new nudge's latency/run-after-accept guarantee until the regular backstop, but it does not erase the producer's durable work. Preventing it needs durable lifecycle identity that survives deletion — for example a store-global monotonic creation sequence or fresh durable nonce copied into task and state (constant storage), or a per-name high-water record with a retention policy — plus storage/migration work and a changed compare-and-clear contract for every `JobStore` implementation. It still would not replace the load-bearing enabled-flip or task/state deletion orderings: those must remain correct to prevent disabled execution and orphan state independently. The added mechanism would therefore protect and diagnose only this lease-expiry + same-name-reuse edge, below the bar for another cross-backend SPI mechanism. Revisit only with evidence of a suspended-then-resumed materializer reaching this sequence or with a broader durable-identity requirement; until then, use a new task name instead of delete-and-immediate-reuse where that risk is unacceptable. - **Recurring ownership reconciliation is namespace-scoped.** `Scheduler.reconcileRecurring(namespace, desiredTasks)` upserts the desired definitions and deletes only tasks previously recorded as owned by that namespace. Spring annotation-driven recurring defaults the namespace from `threadmill.spring.recurring-namespace`, then `spring.application.name`; without either, startup only upserts discovered tasks and leaves stale cleanup manual. - **Per-queue lanes are the starvation fix.** A `ProcessingNode` builder can declare multiple `QueueLane(name, workers)` entries; each gets its own `Dispatcher` with its own `Semaphore`. The `Scheduler.SYSTEM_QUEUE = "system"` constant is the canonical home for recurring / system jobs that must not be starved. - **Queue-family lanes discover active queues.** `ProcessingNode.Builder.lane(pattern, workers, QueueWeights)` creates one shared-capacity lane for queues matching anchored `*` / `?` patterns. Weights are resolved once per discovery cadence, zero pauses a queue, and empty queues remain in the working set until `queueFamilyRetentionAfterEmpty` to avoid bursty rediscovery churn. @@ -487,7 +487,7 @@ Every hard-won failure mode that has come up during development, and the test th | Shutdown requeue moved off the handler's worker thread, letting a peer claim a job while user code still runs on the draining node (issue #110 item 1) | `ProcessingNodeTest.shutdownRequeueIsNeverPublishedWhileTheHandlerIsStillRunning` | | Startup re-registration resets `next_run_at`, so `CATCH_UP` never recovers restart-missed firings; interval phase re-based on every restart (issue #105) | `SchedulingTest.restartReRegistrationPreservesOverdueNextRunSoCatchUpRecoversMissedFires` + `restartReRegistrationWithDropCollapsesMissedFiresIntoASingleRun` + `reRegistrationWithAChangedTriggerRecomputesNextRunFromNow` + `reRegistrationWithAChangedZoneRecomputesNextRun` + `unchangedReRegistrationPreservesTheIntervalPhase` + `reEnablingATaskRestartsTimingInsteadOfCatchingUpTheDisabledPeriod` + `CronExpressionTest.equalityIsValueBasedOnTheSourceExpression` + `AbstractJobStoreContractTest.cronTaskTriggerRoundTripsToEqualValues` | | Crash between the separate cron-task and schedule-state writes pairs a new trigger with old timing that the retry then preserves forever; a store dropping the fingerprint silently reverts restart recovery | `SchedulingTest.stateWriteFailureDuringTriggerEditCannotPairNewTriggerWithOldTiming` + `AbstractJobStoreContractTest.cronTaskStateTimingFingerprintRoundTrips` | -| Materializer under-mutex reload observes a crashed timing edit but applies `CATCH_UP` to the obsolete trigger's overdue state (issue #112) | `SchedulingTest.materializerRepairsMismatchedTimingFingerprintWithoutCatchingUpTheOldSchedule` | +| Materializer under-mutex reload observes a crashed timing edit but applies `CATCH_UP` / `DROP` to the obsolete trigger, skips repair until an obsolete future fire, or loses a coincident nudge; a nudge-created state with no timing stays uninitialized; a legacy null fingerprint drops or moves a valid fire (issue #112) | `SchedulingTest.materializerRepairsMismatchedTimingFingerprintWithoutCatchingUpTheOldSchedule` + `materializerRepairsMismatchedTimingFingerprintBeforeFutureStaleFire` + `materializerRepairsMismatchedTimingAndServesThePendingNudge` + `pendingNudgeInitializesAMissingScheduleState` + `materializerRepairSuppressesTheObsoleteDropFire` + `materializerAdoptsLegacyNullFingerprintsWithoutDroppingOrMovingTiming` | | `DROP` recovery re-bases the schedule on the recovery wall-clock (interval phase drifts by the downtime; `cronFireTime()` lies about the firing it represents); an interval's documented-ignored zone counts as a schedule edit across nodes with different system zones | `SchedulingTest.restartReRegistrationWithDropCollapsesMissedFiresIntoASingleRun` (exact nominal-fire and phase assertions) + `differentSystemZonesDoNotResetAnIntervalSchedule` | | Nudge treated as satisfied by an in-flight run that read its inputs before the nudge committed (silent work loss until the backstop); nudge bursts producing 1:1 runs; a nudged run shifting the schedule (issue #108) | `SchedulingTest.nudgeDuringAnInFlightRunProducesExactlyOneFollowUpAfterCompletion` + `nudgeMaterializesPromptlyWithoutTouchingTheSchedule` + `nudgeCoalescesIntoADueScheduledFire` + `nudgeInstanceTakesThePileUpGuardSoAScheduledFireWaits` + `AbstractJobStoreContractTest.clearCronNudgeOnlyClearsTheObservedValue` | | Blanket schedule-state upsert (re-registration, materializer bookkeeping, dashboard edit) clobbers a concurrently accepted nudge; a nudge racing task removal resurrects schedule state; a disabled task runs from a nudge | `AbstractJobStoreContractTest.nudgeRoundTripsAndSurvivesBlanketStateUpserts` + `nudgeOnUnknownTaskIsRejectedWithoutResurrectingState` + `nudgeOnDisabledTaskIsRejected` + `SchedulingTest.nudgeDisabledTaskFailsLoudlyAndDoesNotRun` + `reEnablingATaskClearsAPendingNudgeFromBeforeThePause` + `DashboardApiServiceTest.updateRecurringEnabledFlipClearsAPendingNudge` | @@ -495,7 +495,7 @@ Every hard-won failure mode that has come up during development, and the test th | Timestamp used as the nudge CAS identity erases a newer same-instant acceptance (Redis millis collide); materializer acts on a task definition listed before its mutex (stale handler/payload materialized, new definition's nudge consumed; disable raced past); a failed clear loses the run instead of over-delivering | `AbstractJobStoreContractTest.nudgeAcceptancesWithIdenticalTimestampsAreDistinguishable` + `SchedulingTest.materializerReloadsTheDefinitionUnderTheTaskMutexBeforeActing` + `materializerRechecksEnabledUnderTheTaskMutexBeforeActing` + `aFailedNudgeClearProducesAnExtraRunNeverALostOne` | | Nudge origin invisible to read-level dashboard users (metadata redaction) or unbounded metric tag cardinality from user-controlled origin metadata | `DashboardApiServiceTest.redactedJobSummariesStillCarryTheCronOrigin` + `ThreadmillMetricsTest.recurringRunsCounterTagsTheTriggerOriginWithBoundedCardinality` | | Spring nudge deferred to afterCommit still routed through the joining store boundary — Spring has committed but not unbound its resources, so the write runs in a fresh transaction on the caller's connection that nobody commits (silently rolled back on an `autoCommit=false` pool such as Hikari with JPA defaults) | `SpringPostgresTransactionBoundaryTest.nudgeIsCommittedEvenWhenThePoolHandsOutNonAutoCommitConnections` (fails against the joining routing, passes with `PostgresJobStore.ownedTransaction`) | -| Postgres self-owned single-statement writes silently roll back when the pool hands out `autoCommit=false` connections (issue #111) | `PostgresJobStoreRegressionTest.selfOwnedWritesCommitWhenConnectionsDefaultToNonAutoCommit` | +| Postgres self-owned writes or migration DDL silently roll back when the pool hands out `autoCommit=false` connections; a multi-statement write returns the connection with auto-commit forced on (issue #111) | `PostgresJobStoreRegressionTest.selfOwnedWritesCommitWhenConnectionsDefaultToNonAutoCommit` + `migrationBootstrapCommitsWhenConnectionsDefaultToNonAutoCommit` + `schemaDropAndRemigrateCommitWhenConnectionsDefaultToNonAutoCommit`; the full `PostgresJobStoreContractTest` runs through `NonAutoCommitDataSource`, which rejects a connection returned in the wrong mode | | Per-transaction nudge storm: the documented "nudge once per work item" pattern registering one synchronisation and one store round trip per call; a `REQUIRES_NEW` inner transaction inheriting the outer's batch through a bound resource | `SpringPostgresTransactionBoundaryTest.repeatedNudgesOfOneTaskInATransactionCollapseToASingleWrite` (batch scoped to the suspend-aware synchronisation list, not `bindResource`) | | Recurring-runs meter counting retry attempts instead of instances, inflating the nudge-versus-schedule ratio operators diagnose with; a non-flip dashboard edit clearing a pending nudge | `ThreadmillMetricsTest.recurringRunsCounterCountsInstancesNotRetryAttempts` + `DashboardApiServiceTest.updateRecurringWithoutAnEnabledFlipLeavesAPendingNudgeIntact` | | Nudge guarantees hold in unit tests but break under sustained load, node churn, or master handover (no soak coverage of the wake-driven path at all) | `nudge-pump` soak scenario + `InvariantChecks.nudgeRunAfterWake` / `outboxDrainedByLaterRun` (red paths: `InvariantViolationTest.nudgeRunAfterWakeFiresWhenNoRunStartsAfterTheNudge` + `nudgeRunAfterWakeFiresWhenTheNudgeGoesUnservedTooLong` + `outboxDrainedByLaterRunFiresWhenARowSurvivesAWholeRun`; green paths: `nudgeRunAfterWakeAcceptsARunThatStartsAfterTheNudge` + `outboxDrainedByLaterRunToleratesRowsAppendedAfterTheRunStarted` + `NudgePumpSmokeTest`) | @@ -560,6 +560,7 @@ These are deliberately additive and design-compatible with the current model — - **Deferred:** Actuator integration (`HealthIndicator`, `MeterBinder`, `/actuator/threadmill` endpoint) is held back from v1 because Spring Boot 4.0's actuator surface is still being reorganised — health and Micrometer integration moved out of the main `spring-boot-actuator` artifact during the milestone series and the final shape isn't pinned yet. Re-attempt after SB4 GA. Spring AOT `RuntimeHints` for native image is deferred for the same reason — it needs a stable actuator target first. A `threadmill-example/spring-boot-4/` sample app is deferred to the same follow-up. (Spring Boot 3 is intentionally not supported and there is no SB3 sample app planned.) After-commit enqueue is already default-on (postgres-improvements Phase 5). - **Task 3 of the v1-readiness finishing pass — per-module READMEs and full docs — landed.** All 14 module READMEs (`threadmill-core`, the three stores, `threadmill-spring-boot`, `threadmill-test-support`, `threadmill-metrics`, `threadmill-tracing`, the three dashboard modules, `threadmill-soak`, `threadmill-simulation`, `threadmill-example`) and the restructured `docs/` tree exist: `index`, `getting-started`, `quickstart` (Spring), `architecture`, `handlers`, `transactions` (deep dive — atomic boundaries per backend, handler-is-not-in-our-transaction, at-least-once + idempotency, outbox pattern), `backend-execution-model`, `configuration`, `concurrency`, `queue-topology`, `long-running-jobs`, `deduplication`, `wake-driven-pollers` (the nudge pattern: handler shape, choosing the backstop interval, what coalescing means for handler code — recurring tasks otherwise had no usage page), `operations`, `troubleshooting`, `migration`, `postgres-schema`, `redis-topologies`, `release-checklist`, plus the JobRunr / Quartz comparison pages. The Postgres README carries the full schema; the Redis README carries the full key layout and Lua script inventory. Runnable examples are compiled files under `threadmill-example/src/main/java/com/example/threadmill/`; doc snippets are maintained by hand (there is no compiled `threadmill-example/snippets/` directory). The bar is "an AI agent can use Threadmill to replace an existing job/scheduler system without reading source code." - **Task 4 of the v1-readiness finishing pass — `threadmill-simulation` module — landed.** New module, separate from `threadmill-soak` (load/performance) and `threadmill-example` (teaching). The short correctness simulation runs 50 projects with `Import` (EXCLUSIVE) and `Export` (SHARED) jobs, 400 jobs over the run (small enough to finish in seconds), random failure injection (5% exception, 0.5% hang), mid-run pause/resume, half-via-`insertAll` bulk-enqueue sample. Records JSON-lines traces under `build/simulation/`; `TraceVerifier` asserts at-least-once, concurrency exclusion (EXCLUSIVE-vs-anything, SHARED-vs-EXCLUSIVE), lock pairing, and pause-obeyed. Gradle entry points: `:threadmill-simulation:simulate` (all three backends), `simulateMemory`, `simulatePostgres`, `simulateRedis`. The Gradle task fails (non-zero exit) when any backend doesn't drain or produces a verifier violation. The worker-churn simulation lives under `com.hemju.threadmill.simulation.workerchurn` and runs through `simulateWorkerChurnPostgres` / `simulateWorkerChurnRedis` against shared local datastores, writing traces to `build/simulation/worker-churn--.jsonl` by default. The fixed process-separated nudge simulation under `com.hemju.threadmill.simulation.nudge` runs through `simulateNudgePostgres` / `simulateNudgeRedis`: a supervisor hard-kills the maintenance leader after an accepted nudge and requires a standby-process nudge run, then hard-kills a producer after its durable work write and requires a schedule-origin backstop drain. Its per-run directory contains the verified cross-process trace plus every child JVM log. +- **The process-separated nudge simulation budgets its two phases independently.** Failover starts with a one-minute recurring interval and a five-minute leader poll so the two 10-second process-start allowances plus lease takeover cannot race either scheduled backstop or leader consumption. After the standby serves the accepted nudge, the supervisor performs a real timing edit to an eight-second interval for the producer-crash backstop. Ready markers publish by temp-file atomic move; every cross-process trace write drains its buffer under the file lock. Simulation work stays outside the engine namespace (`nudge_simulation_work` / `threadmill-simulation:*`) and each work-store instance keeps one datastore connection for its lifetime. - **Batches.** The child relationship is already in the model; a `BatchCompletionInterceptor` over multiple children is the natural shape. - **External-trigger jobs.** The `PROCESSED` state is reserved; this needs an external-signal API plus an escape-hatch timeout. - **Rate limiters.** A store-side token-bucket primitive; Redis can use the standard Lua bucket pattern. diff --git a/CHANGELOG.md b/CHANGELOG.md index 0be5ec3..29b55a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,25 +7,41 @@ Queue pauses, execution and node heartbeats, maintenance leases, retention, mutexes, recurring definitions/state, and the expired-dedup fallback now use explicit Threadmill-owned transactions, restoring the connection's prior - auto-commit mode after commit or rollback. + auto-commit mode after commit or rollback. `saveAtomic`, `softDelete`, + `claimReady`, and `replaceJob` now use that same boundary instead of forcing + auto-commit on return. Migration history bootstrap and destructive schema + reset also commit explicitly. The expired-dedup fallback now keeps its + `SELECT … FOR UPDATE` lock through the delete, closing a lost-update window + that also existed on auto-commit pools. - The recurring materializer now repairs a timing-fingerprint mismatch found during its under-mutex definition reload (issue #112). The current task definition wins: Threadmill preserves run bookkeeping and pending nudge demand, recomputes timing forward from the tick, and does not fire or - `CATCH_UP` the obsolete trigger's overdue backlog. + `CATCH_UP` the obsolete trigger's overdue backlog. A listed fingerprint + mismatch now triggers that repair even when the stale `nextRunAt` is still in + the future, so a faster replacement schedule cannot remain dormant until the + obsolete fire time. Legacy null fingerprints are adopted without dropping or + moving an existing fire, and detected crashed edits emit an operator-visible + warning. - Added fixed process-separated nudge simulations for Postgres and Redis (issue #114). They hard-kill a maintenance leader after an accepted nudge and prove the standby serves it, then hard-kill a producer after its durable work write but before its nudge and prove the regular recurring backstop drains the row. Cross-process JSON-lines traces record process ids, - leadership, trigger origins, and the verified event ordering. + leadership, trigger origins, and the verified event ordering. Ready markers + are atomically published and trace writes drain their buffers fully; the + fixed real-backend simulation is part of `productionCheck`. The failover + phase uses a one-minute backstop and five-minute leader poll, then edits the + task to an eight-second backstop for the producer-crash phase, eliminating + process-start timing races. Simulation work uses its own datastore namespace + and long-lived per-process connections. - Recorded the issue #113 decision not to add per-task lifecycle generations to the `JobStore` SPI. Issue #114 proves hard-kill handoff and backstop recovery, while the remaining revision-reuse ABA additionally requires an old materializer to resume after outliving its mutex lease and a same-name delete/re-registration. A persistent generation would not replace the existing lifecycle orderings and does not justify its cross-backend storage, - retention, migration, and SPI cost without evidence that sequence occurs. + migration, and SPI cost without evidence that sequence occurs. - Recurring tasks can declare claim-time exclusivity (issue #110). A `CronTask` gains an `exclusive` flag, surfaced as `@Recurring(exclusive = true)` and as an `exclusive` parameter on @@ -161,7 +177,8 @@ trigger timing its `next_run_at` was computed from, written atomically with the state row (additive Postgres migration `V4__cron_state_timing_fingerprint.sql`; a Redis hash field; legacy rows - read as null and simply recompute once). The unchanged-schedule decision + read as null and adopt the current fingerprint without moving their recorded + timing). The unchanged-schedule decision reads this fingerprint rather than comparing stored task definitions, so a crash between the separate task-definition and state writes can never pair a new trigger with old timing undetectably — the retry detects the diff --git a/build.gradle.kts b/build.gradle.kts index 58ff9af..79de406 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -228,6 +228,9 @@ tasks.register("productionCheck") { // The correctness simulation is the gate that caught the C1 // in-memory concurrency bypass — a release candidate must run it. ":threadmill-simulation:simulate", + // The process-separated nudge simulation pins maintenance-leader + // hard-kill handoff and the documented producer crash window. + ":threadmill-simulation:simulateNudge", ) dependsOn(":threadmill-example:run") } @@ -240,6 +243,9 @@ tasks subprojects { tasks - .matching { it.name in setOf("jar", "test", "javadoc", "soak", "run") } + .matching { + it.name in setOf("jar", "test", "javadoc", "soak", "run") || + it.name.startsWith("simulate") + } .configureEach { mustRunAfter(cleanTask) } } diff --git a/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/CronTaskScheduleState.java b/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/CronTaskScheduleState.java index 5524413..2a56553 100644 --- a/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/CronTaskScheduleState.java +++ b/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/CronTaskScheduleState.java @@ -84,11 +84,11 @@ public CronTaskScheduleState( } /** - * Convenience constructor with no timing fingerprint. A null fingerprint - * is always safe — the next re-registration or materializer tick that is - * about to act recomputes the schedule instead of preserving it — but - * production writers that recompute - * {@code nextRunAt} from a task should stamp + * Convenience constructor with no timing fingerprint. A materializer tick + * adopts the current task's fingerprint without moving a non-null + * {@code nextRunAt}; if timing is also null, it initializes the next fire + * from that tick. Production writers that compute {@code nextRunAt} from a + * task should stamp * {@link #timingFingerprintOf(CronTask)} so unchanged re-registrations * can preserve overdue state. */ diff --git a/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/RecurringMaterializer.java b/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/RecurringMaterializer.java index 7423c2f..7034b04 100644 --- a/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/RecurringMaterializer.java +++ b/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/RecurringMaterializer.java @@ -141,7 +141,9 @@ private void tickOneLocked(CronTask listed, Instant now) { var state = stateOpt.get(); Long nudge = state.nudgeRequestedAt() == null ? null : state.nudgeRevision(); boolean due = state.nextRunAt() != null && !state.nextRunAt().isAfter(now); - if (!due && nudge == null) return; + boolean listedFingerprintMismatch = + !CronTaskScheduleState.timingFingerprintOf(listed).equals(state.timingFingerprint()); + if (!due && nudge == null && !listedFingerprintMismatch) return; // About to act — reload the definition now that we hold the task // mutex. The listed object was snapshotted by tick() BEFORE the @@ -149,20 +151,25 @@ private void tickOneLocked(CronTask listed, Instant now) { // and materializing from the stale object would insert the old // handler/payload (and, for a nudge, consume a request that was made // against the new definition). The reload is deliberately done only - // when a materialization is imminent, so idle ticks stay at one - // state read per task. + // when a materialization is imminent or the listed definition already + // proves the timing state is stale, so ordinary idle ticks stay at one + // state read per task while future stale schedules self-heal promptly. CronTask task = store.findCronTask(listed.name()).orElse(null); if (task == null || !task.enabled()) return; String fingerprint = CronTaskScheduleState.timingFingerprintOf(task); + boolean timingStateChanged = false; if (!fingerprint.equals(state.timingFingerprint())) { - // A timing edit writes the definition before its schedule state. - // Seeing a mismatch here is the crash signature for that window. - // Finish the edit by scheduling forward from this tick: firing - // the stale timing would run a trigger the user already replaced, - // while merely skipping would leave the task dormant until some - // future re-registration happened to repair it. - Instant next = task.trigger().nextAfter(now, task.zone()); + String previousFingerprint = state.timingFingerprint(); + boolean legacyTiming = previousFingerprint == null && state.nextRunAt() != null; + // A non-null mismatch is the crash signature for a timing edit + // that wrote the definition before its schedule state. Finish it + // by scheduling forward from this tick: firing the stale timing + // would run a trigger the user already replaced. A legacy null + // fingerprint does not prove an edit, so adopt the fingerprint + // without dropping or moving an already-recorded firing. + Instant next = legacyTiming ? state.nextRunAt() : task.trigger().nextAfter(now, task.zone()); + if (!legacyTiming) due = false; state = new CronTaskScheduleState( task.name(), state.lastRunAt(), @@ -170,11 +177,23 @@ private void tickOneLocked(CronTask listed, Instant now) { next, state.inFlightJobId(), fingerprint, + // These cells are carried in the in-memory record only; + // upsertCronTaskState deliberately never writes them. state.nudgeRequestedAt(), state.nudgeRevision()); - store.upsertCronTaskState(state); - due = false; - if (nudge == null) return; + timingStateChanged = true; + if (previousFingerprint != null) { + LOG.warn( + "Repairing stale recurring timing for task {} from fingerprint {} to {}; next run at {}", + task.name(), + previousFingerprint, + fingerprint, + next); + } + if (nudge == null && !due) { + store.upsertCronTaskState(state); + return; + } } // Pile-up guard: an in-flight instance that is still going to run @@ -182,6 +201,7 @@ private void tickOneLocked(CronTask listed, Instant now) { if (state.inFlightJobId() != null) { Job inFlight = store.findById(JobId.of(state.inFlightJobId())).orElse(null); if (inFlight != null && blocksNextMaterialization(inFlight, now)) { + if (timingStateChanged) store.upsertCronTaskState(state); // Still running — leave the next_run_at where it is so we revisit on the next tick. return; } diff --git a/threadmill-simulation/README.md b/threadmill-simulation/README.md index d7b274f..3581ddd 100644 --- a/threadmill-simulation/README.md +++ b/threadmill-simulation/README.md @@ -155,6 +155,9 @@ separate JVMs. Run both backends: ./gradlew :threadmill-simulation:simulateNudge ``` +The combined real-backend task is also part of the root `productionCheck` +release-candidate gate so the crash paths cannot silently rot. + Or select one: ```bash @@ -162,8 +165,11 @@ Or select one: ./gradlew :threadmill-simulation:simulateNudgeRedis ``` -The leader registers an exclusive eight-second recurring outbox pump. The -supervisor then verifies two crash windows: +The leader first registers an exclusive one-minute recurring outbox pump, with +its own maintenance poll set to five minutes, so process startup and lease +handoff cannot race the backstop or the leader's next tick. After failover is +proved, the supervisor edits the backstop to eight seconds for the producer +crash phase. It verifies two crash windows: - A producer commits durable work and an accepted nudge. The supervisor hard-kills the maintenance leader before its next materializer tick; the diff --git a/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationMain.java b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationMain.java index fd593d1..97a45b3 100644 --- a/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationMain.java +++ b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationMain.java @@ -5,6 +5,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardCopyOption; import java.time.Duration; import java.time.Instant; import java.util.ArrayList; @@ -16,7 +17,6 @@ import java.util.UUID; import java.util.concurrent.TimeUnit; import java.util.function.BooleanSupplier; -import java.util.function.Predicate; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; @@ -47,8 +47,9 @@ public final class NudgeSimulationMain { private static final ObjectMapper JSON = new ObjectMapper(); - private static final Duration BACKSTOP_INTERVAL = Duration.ofSeconds(8); - private static final Duration LEADER_MAINTENANCE_POLL = Duration.ofSeconds(10); + private static final Duration FAILOVER_BACKSTOP_INTERVAL = Duration.ofMinutes(1); + private static final Duration CRASH_BACKSTOP_INTERVAL = Duration.ofSeconds(8); + private static final Duration LEADER_MAINTENANCE_POLL = Duration.ofMinutes(5); private static final Duration STANDBY_MAINTENANCE_POLL = Duration.ofMillis(100); private static final Duration PROCESS_START_TIMEOUT = Duration.ofSeconds(10); private static final Duration FAILOVER_TIMEOUT = Duration.ofSeconds(8); @@ -84,7 +85,7 @@ private static void runSupervisor(Options options) throws Exception { "runId", runId, "taskName", taskName, "queue", queue, - "backstopMillis", BACKSTOP_INTERVAL.toMillis())); + "backstopMillis", FAILOVER_BACKSTOP_INTERVAL.toMillis())); ManagedProcess leader = null; ManagedProcess standby = null; @@ -191,6 +192,15 @@ private static void runSupervisor(Options options) throws Exception { await("accepted nudge work to drain", FAILOVER_TIMEOUT, () -> !workStore.isPending(1)); awaitRecurringInstanceSuccess(storeHandle.store(), taskName, FAILOVER_TIMEOUT); + defineTask(storeHandle.store(), runId, trace, taskName, queue, CRASH_BACKSTOP_INTERVAL); + NudgeSimulationTrace.append( + trace, + "backstop-shortened", + Map.of( + "runId", runId, + "taskName", taskName, + "backstopMillis", CRASH_BACKSTOP_INTERVAL.toMillis())); + var crashReadyFile = outputDirectory.resolve("producer-crash.ready.json"); crashProducer = startProducer( fixture.connectionInfo(), @@ -238,18 +248,13 @@ private static void runNode(Options options) throws Exception { NudgeSimulationStores.configureProcess(connectionInfo); try (var storeHandle = NudgeSimulationStores.openJobStore(connectionInfo)) { if (options.registerTask) { - var scheduler = new Scheduler(storeHandle.store(), new JsonJobSerializer()); - scheduler.defineRecurring( + defineTask( + storeHandle.store(), + options.runId, + options.traceFile, options.taskName, - new CronTask.Trigger.Interval(BACKSTOP_INTERVAL), - new NudgeSimulationPayload(options.runId, options.traceFile.toString()), - NudgeSimulationHandler.class.getName(), options.queue, - 0, - null, - null, - true, - CronTask.MissedRunPolicy.DROP); + FAILOVER_BACKSTOP_INTERVAL); } var config = ProcessingNodeConfig.builder() @@ -308,6 +313,22 @@ private static void runNode(Options options) throws Exception { } } + private static void defineTask( + JobStore store, String runId, Path trace, String taskName, String queue, Duration interval) { + var scheduler = new Scheduler(store, new JsonJobSerializer()); + scheduler.defineRecurring( + taskName, + new CronTask.Trigger.Interval(interval), + new NudgeSimulationPayload(runId, trace.toString()), + NudgeSimulationHandler.class.getName(), + queue, + 0, + null, + null, + true, + CronTask.MissedRunPolicy.DROP); + } + private static void runProducer(Options options) throws Exception { var connectionInfo = options.connectionInfo(); try (var storeHandle = NudgeSimulationStores.openJobStore(connectionInfo); @@ -432,18 +453,27 @@ private static Ready awaitReady(ManagedProcess process, Path readyFile, Duration } var document = JSON.readTree(Files.readString(readyFile, StandardCharsets.UTF_8)); var nodeId = document.hasNonNull("nodeId") - ? NodeId.of(UUID.fromString(document.get("nodeId").asText())) + ? NodeId.of(UUID.fromString(document.path("nodeId").asText())) : null; - return new Ready(nodeId, document.get("pid").asLong()); + long pid = document.path("pid").asLong(-1); + require(pid > 0, process.label() + " published a ready marker without a valid pid"); + return new Ready(nodeId, pid); } private static void writeReady(Path readyFile, NodeId nodeId, long pid) { + var temporaryFile = readyFile.resolveSibling(readyFile.getFileName() + ".tmp-" + pid); try { var fields = new LinkedHashMap(); fields.put("pid", pid); fields.put("nodeId", nodeId == null ? null : nodeId.toString()); - Files.writeString(readyFile, JSON.writeValueAsString(fields), StandardCharsets.UTF_8); + Files.writeString(temporaryFile, JSON.writeValueAsString(fields), StandardCharsets.UTF_8); + Files.move(temporaryFile, readyFile, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING); } catch (IOException e) { + try { + Files.deleteIfExists(temporaryFile); + } catch (IOException cleanupFailure) { + e.addSuppressed(cleanupFailure); + } throw new IllegalStateException("failed to write process-ready marker: " + readyFile, e); } } @@ -509,41 +539,24 @@ private static void verifyTrace(Path trace, long standbyPid) throws IOException if (!line.isBlank()) events.add(JSON.readTree(line)); } - var acceptIndexes = indexesOf(events, "nudge-accepted"); - require(!acceptIndexes.isEmpty(), "trace has no accepted nudge"); - for (var acceptIndex : acceptIndexes) { - var producerPid = events.get(acceptIndex).get("pid").asLong(); - require( - findAfter( - events, - acceptIndex, - "pump-run-start", - event -> event.get("pid").asLong() != producerPid) - >= 0, - "accepted nudge was not followed by a run in another OS process"); - } - var accepted = requireEvent(events, "nudge-accepted", 1); var leaderKilled = requireEvent(events, "leader-hard-killed", null); - var elected = requireEvent(events, "maintenance-elected", null); var firstDrain = requireEvent(events, "work-drained", 1); - var firstRun = requireRun(events, firstDrain.event().get("jobId").asText()); + var firstRun = requireRun(events, firstDrain.event().path("jobId").asText()); require( accepted.index() < leaderKilled.index() && leaderKilled.index() < firstRun.index() && firstRun.index() < firstDrain.index(), "accepted-nudge leader-kill ordering is not proven by the trace"); - require(leaderKilled.index() < elected.index(), "standby election was observed before the leader kill"); - require(elected.event().get("pid").asLong() == standbyPid, "maintenance ownership did not move to standby"); - require(firstRun.event().get("pid").asLong() == standbyPid, "accepted nudge was not served by the standby"); + require(firstRun.event().path("pid").asLong() == standbyPid, "accepted nudge was not served by the standby"); require( - "nudge".equals(firstRun.event().get("origin").asText()), + "nudge".equals(firstRun.event().path("origin").asText()), "accepted nudge was served only by the backstop"); var secondRecorded = requireEvent(events, "work-recorded", 2); var producerKilled = requireEvent(events, "producer-hard-killed-before-nudge", 2); var secondDrain = requireEvent(events, "work-drained", 2); - var secondRun = requireRun(events, secondDrain.event().get("jobId").asText()); + var secondRun = requireRun(events, secondDrain.event().path("jobId").asText()); require( secondRecorded.index() < producerKilled.index() && producerKilled.index() < secondRun.index() @@ -556,27 +569,10 @@ private static void verifyTrace(Path trace, long standbyPid) throws IOException && event.path("sequence").asInt(-1) == 2), "the hard-killed producer accepted a nudge unexpectedly"); require( - "schedule".equals(secondRun.event().get("origin").asText()), + "schedule".equals(secondRun.event().path("origin").asText()), "producer crash-window row was not drained by the backstop schedule"); } - private static List indexesOf(List events, String eventName) { - var indexes = new ArrayList(); - for (int i = 0; i < events.size(); i++) { - if (eventName.equals(events.get(i).path("event").asText())) indexes.add(i); - } - return indexes; - } - - private static int findAfter( - List events, int afterIndex, String eventName, Predicate predicate) { - for (int i = afterIndex + 1; i < events.size(); i++) { - var event = events.get(i); - if (eventName.equals(event.path("event").asText()) && predicate.test(event)) return i; - } - return -1; - } - private static IndexedEvent requireEvent(List events, String eventName, Integer sequence) { for (int i = 0; i < events.size(); i++) { var event = events.get(i); diff --git a/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationStores.java b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationStores.java index 21f7d1f..c685e02 100644 --- a/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationStores.java +++ b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationStores.java @@ -12,6 +12,7 @@ import io.lettuce.core.RedisClient; import io.lettuce.core.RedisURI; import io.lettuce.core.ScriptOutputType; +import io.lettuce.core.api.StatefulRedisConnection; import org.postgresql.ds.PGSimpleDataSource; import org.testcontainers.containers.GenericContainer; import org.testcontainers.containers.wait.strategy.Wait; @@ -70,14 +71,14 @@ void appendArguments(List command) { record BackendFixture(ConnectionInfo connectionInfo, AutoCloseable closeAction) implements AutoCloseable { @Override public void close() { - closeQuietly(closeAction, "backend fixture"); + closeOrFail(closeAction, "backend fixture"); } } record JobStoreHandle(JobStore store, AutoCloseable closeAction) implements AutoCloseable { @Override public void close() { - closeQuietly(closeAction, "job store"); + closeOrFail(closeAction, "job store"); } } @@ -157,7 +158,7 @@ private static DataSource postgresDataSource(ConnectionInfo connectionInfo) { return dataSource; } - private static void closeQuietly(AutoCloseable closeAction, String description) { + private static void closeOrFail(AutoCloseable closeAction, String description) { if (closeAction == null) return; try { closeAction.close(); @@ -167,11 +168,15 @@ private static void closeQuietly(AutoCloseable closeAction, String description) } private static final class PostgresWorkStore implements WorkStore { - private final DataSource dataSource; + private final Connection connection; private final String runId; private PostgresWorkStore(DataSource dataSource, String runId) { - this.dataSource = dataSource; + try { + this.connection = dataSource.getConnection(); + } catch (SQLException e) { + throw new IllegalStateException("failed to open Postgres nudge simulation work connection", e); + } this.runId = runId; } @@ -179,7 +184,7 @@ private PostgresWorkStore(DataSource dataSource, String runId) { public void prepare() { transaction(connection -> { try (var statement = connection.createStatement()) { - statement.executeUpdate("CREATE TABLE IF NOT EXISTS threadmill_simulation_nudge_work (" + statement.executeUpdate("CREATE TABLE IF NOT EXISTS nudge_simulation_work (" + "run_id text NOT NULL, sequence integer NOT NULL, " + "recorded_at timestamptz NOT NULL DEFAULT clock_timestamp(), drained_at timestamptz, " + "PRIMARY KEY (run_id, sequence))"); @@ -192,7 +197,7 @@ public void prepare() { public void record(int sequence) { transaction(connection -> { try (var statement = connection.prepareStatement( - "INSERT INTO threadmill_simulation_nudge_work (run_id, sequence) VALUES (?, ?)")) { + "INSERT INTO nudge_simulation_work (run_id, sequence) VALUES (?, ?)")) { statement.setString(1, runId); statement.setInt(2, sequence); statement.executeUpdate(); @@ -205,8 +210,8 @@ public void record(int sequence) { public List drain() { return transaction(connection -> { var drained = new ArrayList(); - try (var statement = connection.prepareStatement( - "UPDATE threadmill_simulation_nudge_work SET drained_at = clock_timestamp() " + try (var statement = + connection.prepareStatement("UPDATE nudge_simulation_work SET drained_at = clock_timestamp() " + "WHERE run_id = ? AND drained_at IS NULL RETURNING sequence")) { statement.setString(1, runId); try (var result = statement.executeQuery()) { @@ -219,9 +224,8 @@ public List drain() { @Override public boolean isPending(int sequence) { - try (var connection = dataSource.getConnection(); - var statement = connection.prepareStatement("SELECT 1 FROM threadmill_simulation_nudge_work " - + "WHERE run_id = ? AND sequence = ? AND drained_at IS NULL")) { + try (var statement = connection.prepareStatement("SELECT 1 FROM nudge_simulation_work " + + "WHERE run_id = ? AND sequence = ? AND drained_at IS NULL")) { statement.setString(1, runId); statement.setInt(2, sequence); try (var result = statement.executeQuery()) { @@ -233,10 +237,16 @@ public boolean isPending(int sequence) { } @Override - public void close() {} + public void close() { + try { + connection.close(); + } catch (SQLException e) { + throw new IllegalStateException("failed to close Postgres nudge simulation work connection", e); + } + } private T transaction(SqlWork work) { - try (var connection = dataSource.getConnection()) { + try { var previousAutoCommit = connection.getAutoCommit(); connection.setAutoCommit(false); try { @@ -262,46 +272,44 @@ private interface SqlWork { private static final class RedisWorkStore implements WorkStore { private final RedisClient client; + private final StatefulRedisConnection connection; private final String key; private RedisWorkStore(RedisURI redisUri, String runId) { this.client = RedisClient.create(redisUri); - this.key = "{threadmill}:simulation:nudge:" + runId + ":pending"; + this.connection = client.connect(); + this.key = "threadmill-simulation:nudge:" + runId + ":pending"; } @Override public void prepare() { - try (var connection = client.connect()) { - connection.sync().del(key); - } + connection.sync().del(key); } @Override public void record(int sequence) { - try (var connection = client.connect()) { - connection.sync().sadd(key, Integer.toString(sequence)); - } + connection.sync().sadd(key, Integer.toString(sequence)); } @Override public List drain() { - try (var connection = client.connect()) { - List values = - connection.sync().eval(REDIS_DRAIN_SCRIPT, ScriptOutputType.MULTI, new String[] {key}); - return values.stream().map(Integer::valueOf).sorted().toList(); - } + List values = + connection.sync().eval(REDIS_DRAIN_SCRIPT, ScriptOutputType.MULTI, new String[] {key}); + return values.stream().map(Integer::valueOf).sorted().toList(); } @Override public boolean isPending(int sequence) { - try (var connection = client.connect()) { - return connection.sync().sismember(key, Integer.toString(sequence)); - } + return connection.sync().sismember(key, Integer.toString(sequence)); } @Override public void close() { - client.shutdown(); + try { + connection.close(); + } finally { + client.shutdown(); + } } } } diff --git a/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationTrace.java b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationTrace.java index bcb05da..27c4d76 100644 --- a/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationTrace.java +++ b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/nudge/NudgeSimulationTrace.java @@ -40,7 +40,8 @@ private static void appendLocked(Path path, String event, Map fields) path, StandardOpenOption.CREATE, StandardOpenOption.WRITE, StandardOpenOption.APPEND); var lock = channel.lock()) { if (!lock.isValid()) throw new IllegalStateException("trace lock is not valid: " + path); - channel.write(ByteBuffer.wrap(bytes)); + var buffer = ByteBuffer.wrap(bytes); + while (buffer.hasRemaining()) channel.write(buffer); } } catch (IOException e) { throw new IllegalStateException("failed to append nudge simulation trace: " + path, e); diff --git a/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/workerchurn/WorkerChurnTraceLog.java b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/workerchurn/WorkerChurnTraceLog.java index 0ac994c..3188bfa 100644 --- a/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/workerchurn/WorkerChurnTraceLog.java +++ b/threadmill-simulation/src/main/java/com/hemju/threadmill/simulation/workerchurn/WorkerChurnTraceLog.java @@ -36,7 +36,8 @@ private static void appendLocked(Path path, String event, Map fields) path, StandardOpenOption.CREATE, StandardOpenOption.WRITE, StandardOpenOption.APPEND); FileLock lock = channel.lock()) { if (!lock.isValid()) throw new IllegalStateException("trace lock is not valid: " + path); - channel.write(ByteBuffer.wrap(line.getBytes(StandardCharsets.UTF_8))); + var buffer = ByteBuffer.wrap(line.getBytes(StandardCharsets.UTF_8)); + while (buffer.hasRemaining()) channel.write(buffer); } } catch (IOException e) { throw new IllegalStateException("failed to append worker-churn trace: " + path, e); @@ -45,7 +46,7 @@ private static void appendLocked(Path path, String event, Map fields) private static String line(String event, Map fields) { var out = new StringBuilder(256); - out.append("{\"ts\":\"") + out.append("{\"timestamp\":\"") .append(Instant.now()) .append("\",\"event\":\"") .append(escape(event)) diff --git a/threadmill-store-memory/src/test/java/com/hemju/threadmill/store/memory/SchedulingTest.java b/threadmill-store-memory/src/test/java/com/hemju/threadmill/store/memory/SchedulingTest.java index acc50b8..be4eafa 100644 --- a/threadmill-store-memory/src/test/java/com/hemju/threadmill/store/memory/SchedulingTest.java +++ b/threadmill-store-memory/src/test/java/com/hemju/threadmill/store/memory/SchedulingTest.java @@ -1290,6 +1290,202 @@ void materializerRepairsMismatchedTimingFingerprintWithoutCatchingUpTheOldSchedu }); } + @Test + void materializerRepairsMismatchedTimingFingerprintBeforeFutureStaleFire() { + scheduler.defineIntervalTask( + "future-crashed-edit", + Duration.ofDays(7), + new HelloPayload("tick"), + RecorderHandler.class, + "default", + 0, + CronTask.MissedRunPolicy.DROP); + var oldTask = store.findCronTask("future-crashed-edit").orElseThrow(); + var repairAt = Instant.parse("2026-08-12T06:00:00Z"); + var staleFutureFire = repairAt.plus(Duration.ofDays(7)); + store.upsertCronTaskState(new CronTaskScheduleState( + oldTask.name(), null, null, staleFutureFire, null, CronTaskScheduleState.timingFingerprintOf(oldTask))); + + var editedTask = new CronTask( + oldTask.name(), + new CronTask.Trigger.Interval(Duration.ofMinutes(1)), + oldTask.handlerType(), + oldTask.payloadArgument(), + oldTask.queue(), + oldTask.priority(), + oldTask.timeout(), + oldTask.maxAttempts(), + oldTask.exclusive(), + oldTask.missedRunPolicy(), + oldTask.zone(), + true); + store.upsertCronTask(editedTask); + + new RecurringMaterializer(store).tick(repairAt); + + assertThat(store.findByHandlerSignature(RecorderHandler.class.getName(), 10)) + .as("repairing future stale timing must not materialize an instance") + .isEmpty(); + assertThat(store.findCronTaskState(oldTask.name()).orElseThrow()).satisfies(repaired -> { + assertThat(repaired.nextRunAt()).isEqualTo(repairAt.plus(Duration.ofMinutes(1))); + assertThat(repaired.nextRunAt()).isBefore(staleFutureFire); + assertThat(repaired.timingFingerprint()).isEqualTo(CronTaskScheduleState.timingFingerprintOf(editedTask)); + }); + } + + @Test + void materializerRepairsMismatchedTimingAndServesThePendingNudge() { + scheduler.defineIntervalTask( + "nudged-crashed-edit", + Duration.ofDays(7), + new HelloPayload("tick"), + RecorderHandler.class, + "default", + 0, + CronTask.MissedRunPolicy.DROP); + var oldTask = store.findCronTask("nudged-crashed-edit").orElseThrow(); + var repairAt = Instant.parse("2026-08-12T07:00:00Z"); + store.upsertCronTaskState(new CronTaskScheduleState( + oldTask.name(), + null, + null, + repairAt.plus(Duration.ofDays(7)), + null, + CronTaskScheduleState.timingFingerprintOf(oldTask))); + scheduler.nudgeRecurring(oldTask.name()); + + var editedTask = new CronTask( + oldTask.name(), + new CronTask.Trigger.Interval(Duration.ofMinutes(1)), + oldTask.handlerType(), + oldTask.payloadArgument(), + oldTask.queue(), + oldTask.priority(), + oldTask.timeout(), + oldTask.maxAttempts(), + oldTask.exclusive(), + oldTask.missedRunPolicy(), + oldTask.zone(), + true); + store.upsertCronTask(editedTask); + + new RecurringMaterializer(store).tick(repairAt); + + var instances = store.findByHandlerSignature(RecorderHandler.class.getName(), 10); + assertThat(instances).hasSize(1); + assertThat(instances.getFirst().metadata().get(JobExecutionContext.CRON_ORIGIN_META)) + .contains(JobExecutionContext.CRON_ORIGIN_NUDGE); + assertThat(store.findCronTaskState(oldTask.name()).orElseThrow()).satisfies(repaired -> { + assertThat(repaired.nextRunAt()).isEqualTo(repairAt.plus(Duration.ofMinutes(1))); + assertThat(repaired.timingFingerprint()).isEqualTo(CronTaskScheduleState.timingFingerprintOf(editedTask)); + assertThat(repaired.nudgeRequestedAt()).isNull(); + assertThat(repaired.inFlightJobId()) + .isEqualTo(instances.getFirst().id().asUuid()); + }); + } + + @Test + void pendingNudgeInitializesAMissingScheduleState() { + scheduler.defineIntervalTask( + "nudge-created-state", + Duration.ofHours(2), + new HelloPayload("tick"), + RecorderHandler.class, + "default", + 0, + CronTask.MissedRunPolicy.DROP); + var task = store.findCronTask("nudge-created-state").orElseThrow(); + store.deleteCronTask(task.name()); + store.upsertCronTask(task); + scheduler.nudgeRecurring(task.name()); + assertThat(store.findCronTaskState(task.name()).orElseThrow()).satisfies(created -> { + assertThat(created.nextRunAt()).isNull(); + assertThat(created.timingFingerprint()).isNull(); + assertThat(created.nudgeRequestedAt()).isNotNull(); + }); + var now = Instant.parse("2026-08-12T07:30:00Z"); + + new RecurringMaterializer(store).tick(now); + + var instances = store.findByHandlerSignature(RecorderHandler.class.getName(), 10); + assertThat(instances).hasSize(1); + assertThat(instances.getFirst().metadata().get(JobExecutionContext.CRON_ORIGIN_META)) + .contains(JobExecutionContext.CRON_ORIGIN_NUDGE); + assertThat(store.findCronTaskState(task.name()).orElseThrow()).satisfies(initialized -> { + assertThat(initialized.nextRunAt()).isEqualTo(now.plus(Duration.ofHours(2))); + assertThat(initialized.timingFingerprint()).isEqualTo(CronTaskScheduleState.timingFingerprintOf(task)); + assertThat(initialized.nudgeRequestedAt()).isNull(); + }); + } + + @Test + void materializerRepairSuppressesTheObsoleteDropFire() { + scheduler.defineIntervalTask( + "drop-crashed-edit", + Duration.ofMinutes(5), + new HelloPayload("tick"), + RecorderHandler.class, + "default", + 0, + CronTask.MissedRunPolicy.DROP); + var oldTask = store.findCronTask("drop-crashed-edit").orElseThrow(); + var repairAt = Instant.parse("2026-08-12T08:00:00Z"); + store.upsertCronTaskState(new CronTaskScheduleState( + oldTask.name(), + null, + null, + repairAt.minus(Duration.ofDays(1)), + null, + CronTaskScheduleState.timingFingerprintOf(oldTask))); + var editedTask = new CronTask( + oldTask.name(), + new CronTask.Trigger.Interval(Duration.ofHours(6)), + oldTask.handlerType(), + oldTask.payloadArgument(), + oldTask.queue(), + oldTask.priority(), + oldTask.timeout(), + oldTask.maxAttempts(), + oldTask.exclusive(), + oldTask.missedRunPolicy(), + oldTask.zone(), + true); + store.upsertCronTask(editedTask); + + new RecurringMaterializer(store).tick(repairAt); + + assertThat(store.findByHandlerSignature(RecorderHandler.class.getName(), 10)) + .as("the obsolete trigger's collapsed DROP fire must not run") + .isEmpty(); + assertThat(store.findCronTaskState(oldTask.name()).orElseThrow().nextRunAt()) + .isEqualTo(repairAt.plus(Duration.ofHours(6))); + } + + @Test + void materializerAdoptsLegacyNullFingerprintsWithoutDroppingOrMovingTiming() { + scheduler.defineIntervalTask("legacy-due", Duration.ofHours(1), new HelloPayload("due"), RecorderHandler.class); + scheduler.defineIntervalTask( + "legacy-future", Duration.ofHours(1), new HelloPayload("future"), RecorderHandler.class); + var now = Instant.parse("2026-08-12T09:00:00Z"); + var dueAt = now.minus(Duration.ofMinutes(5)); + var futureAt = now.plus(Duration.ofMinutes(30)); + store.upsertCronTaskState(new CronTaskScheduleState("legacy-due", null, null, dueAt, null)); + store.upsertCronTaskState(new CronTaskScheduleState("legacy-future", null, null, futureAt, null)); + + new RecurringMaterializer(store).tick(now); + + var instances = store.findByHandlerSignature(RecorderHandler.class.getName(), 10); + assertThat(instances).hasSize(1); + assertThat(instances.getFirst().metadata().get(JobExecutionContext.CRON_ORIGIN_META)) + .contains(JobExecutionContext.CRON_ORIGIN_SCHEDULE); + assertThat(store.findCronTaskState("legacy-future").orElseThrow()).satisfies(adopted -> { + assertThat(adopted.nextRunAt()).isEqualTo(futureAt); + assertThat(adopted.timingFingerprint()) + .isEqualTo(CronTaskScheduleState.timingFingerprintOf( + store.findCronTask("legacy-future").orElseThrow())); + }); + } + @Test void materializerReloadsTheDefinitionUnderTheTaskMutexBeforeActing() { // tick() snapshots the task list BEFORE tickOne takes the per-task diff --git a/threadmill-store-postgres/README.md b/threadmill-store-postgres/README.md index 53eb948..08161f8 100644 --- a/threadmill-store-postgres/README.md +++ b/threadmill-store-postgres/README.md @@ -146,9 +146,10 @@ is not a production upgrade strategy. The host owns the pool. `PostgresJobStore` accepts a `javax.sql.DataSource`; it does not create or close one. Threadmill does not require the pool to default -to `autoCommit=true`: every self-owned write uses an explicit transaction and -restores the connection's previous mode before returning it. Recommended floor -for the pool size: +to `autoCommit=true`: every self-owned write uses the common owning transaction +boundary, commits explicitly, and restores the connection's previous mode. +Migration history bootstrap, individual migrations, and destructive schema +reset use equivalent explicit boundaries. Recommended floor for the pool size: `workerCount + claimBatchSize + headroom` so claim and maintenance never contend with handler-side queries. diff --git a/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/MigrationRunner.java b/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/MigrationRunner.java index 8a55eea..bddf9ca 100644 --- a/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/MigrationRunner.java +++ b/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/MigrationRunner.java @@ -221,13 +221,18 @@ private static String checksum(String sql) { public void dropThreadmillObjects() { try (Connection conn = dataSource.getConnection()) { acquireMigrationLock(conn); - try (Statement st = conn.createStatement()) { - for (String table : THREADMILL_TABLES) { - st.execute("DROP TABLE IF EXISTS " + table + " CASCADE"); - } - for (String function : THREADMILL_FUNCTIONS) { - st.execute("DROP FUNCTION IF EXISTS " + function + " CASCADE"); - } + try { + inTransaction(conn, transaction -> { + try (Statement st = transaction.createStatement()) { + for (String table : THREADMILL_TABLES) { + st.execute("DROP TABLE IF EXISTS " + table + " CASCADE"); + } + for (String function : THREADMILL_FUNCTIONS) { + st.execute("DROP FUNCTION IF EXISTS " + function + " CASCADE"); + } + } + return null; + }); } finally { releaseMigrationLock(conn); } @@ -237,39 +242,55 @@ public void dropThreadmillObjects() { } private void applyOne(Connection conn, Migration m) throws SQLException { - boolean priorAutoCommit = conn.getAutoCommit(); - conn.setAutoCommit(false); - try (Statement st = conn.createStatement()) { - st.execute(m.sql()); - try (PreparedStatement ps = conn.prepareStatement( - "INSERT INTO threadmill_schema_history (version, description, checksum) VALUES (?, ?, ?)")) { - ps.setInt(1, m.version()); - ps.setString(2, m.description()); - ps.setString(3, checksum(m.sql())); - ps.executeUpdate(); + try { + inTransaction(conn, transaction -> { + try (Statement st = transaction.createStatement()) { + st.execute(m.sql()); + try (PreparedStatement ps = transaction.prepareStatement( + "INSERT INTO threadmill_schema_history (version, description, checksum) VALUES (?, ?, ?)")) { + ps.setInt(1, m.version()); + ps.setString(2, m.description()); + ps.setString(3, checksum(m.sql())); + ps.executeUpdate(); + } + } + return null; + }); + } catch (SQLException e) { + throw new MigrationException("Migration " + m.fileName() + " failed", e); + } + } + + private void ensureHistoryTable(Connection conn) throws SQLException { + inTransaction(conn, transaction -> { + try (Statement st = transaction.createStatement()) { + st.execute(historyTableSql()); + // Backfill the checksum column for history tables created before it + // existed; older rows keep a NULL checksum (validate skips those). + st.execute("ALTER TABLE threadmill_schema_history ADD COLUMN IF NOT EXISTS checksum TEXT"); } + return null; + }); + } + + private static T inTransaction(Connection conn, PostgresConnectionWork work) throws SQLException { + boolean previousAutoCommit = conn.getAutoCommit(); + conn.setAutoCommit(false); + try { + T result = work.execute(conn); conn.commit(); - } catch (SQLException e) { - // Preserve the original failure even if the rollback itself fails - // (e.g. the connection died), so the operator still sees which - // migration and statement failed. + return result; + } catch (RuntimeException | SQLException e) { + // Preserve the original failure even if rollback also fails (for + // example because the connection died mid-DDL). try { conn.rollback(); } catch (SQLException rollbackError) { e.addSuppressed(rollbackError); } - throw new MigrationException("Migration " + m.fileName() + " failed", e); + throw e; } finally { - conn.setAutoCommit(priorAutoCommit); - } - } - - private void ensureHistoryTable(Connection conn) throws SQLException { - try (Statement st = conn.createStatement()) { - st.execute(historyTableSql()); - // Backfill the checksum column for history tables created before it - // existed; older rows keep a NULL checksum (validate skips those). - st.execute("ALTER TABLE threadmill_schema_history ADD COLUMN IF NOT EXISTS checksum TEXT"); + conn.setAutoCommit(previousAutoCommit); } } diff --git a/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/PostgresJobStore.java b/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/PostgresJobStore.java index c1c9884..2cca981 100644 --- a/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/PostgresJobStore.java +++ b/threadmill-store-postgres/src/main/java/com/hemju/threadmill/store/postgres/PostgresJobStore.java @@ -411,70 +411,55 @@ public void saveAtomic(Job job, long expectedVersion) { boolean saved; try { - saved = DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection()) { - conn.setAutoCommit(false); - try { - JobSnapshot oldSnapshot; - try (PreparedStatement ps = conn.prepareStatement( - "SELECT body, version FROM threadmill_jobs WHERE id = ? FOR UPDATE")) { - ps.setObject(1, snapshot.id().asUuid()); - try (ResultSet rs = ps.executeQuery()) { - if (!rs.next()) { - conn.commit(); - return false; - } - if (rs.getLong(2) != expectedVersion) { - conn.commit(); - return false; - } - oldSnapshot = serializer - .deserializeJob(rs.getString(1)) - .snapshot(); - } - } - if (oldSnapshot.concurrencyKey() != null) { - lockConcurrencyGroup(conn, oldSnapshot.concurrencyKey()); + saved = ownedTransaction(conn -> { + JobSnapshot oldSnapshot; + try (PreparedStatement ps = + conn.prepareStatement("SELECT body, version FROM threadmill_jobs WHERE id = ? FOR UPDATE")) { + ps.setObject(1, snapshot.id().asUuid()); + try (ResultSet rs = ps.executeQuery()) { + if (!rs.next()) { + return false; } - adjustWorkflowHoldOnTransition(conn, oldSnapshot, snapshot.currentState()); - try (PreparedStatement ps = conn.prepareStatement("UPDATE threadmill_jobs SET " - + "state = ?, queue = ?, priority = ?, handler_signature = ?, " - + "scheduled_at = ?, owner_node_id = ?, owner_heartbeat_at = ?, last_checkin_at = ?, " - + "current_state_at = ?, version = ?, body = ?, " - + "concurrency_key = ?, concurrency_mode = ?, workflow_root_id = ?, parent_job_id = ? " - + "WHERE id = ? AND version = ?")) { - ps.setString(1, snapshot.currentState().name()); - ps.setString(2, snapshot.queue()); - ps.setInt(3, snapshot.priority()); - ps.setString(4, snapshot.spec().handlerType()); - setNullableTimestamp(ps, 5, snapshot.scheduledFor()); - setNullableUuid( - ps, - 6, - snapshot.ownerNodeId() == null - ? null - : snapshot.ownerNodeId().asUuid()); - setNullableTimestamp(ps, 7, snapshot.ownerHeartbeatAt()); - setNullableTimestamp(ps, 8, snapshot.lastCheckinAt()); - ps.setTimestamp(9, Timestamp.from(currentStateAt)); - ps.setLong(10, nextVersion); - ps.setString(11, body); - setNullableConcurrency(ps, 12, snapshot.concurrencyKey(), snapshot.concurrencyMode()); - ps.setObject(14, snapshot.workflowRootId().asUuid()); - setNullableParentJobId(ps, 15, snapshot); - ps.setObject(16, snapshot.id().asUuid()); - ps.setLong(17, expectedVersion); - int rows = ps.executeUpdate(); - conn.commit(); - return rows > 0; + if (rs.getLong(2) != expectedVersion) { + return false; } - } catch (RuntimeException | SQLException e) { - conn.rollback(); - throw e; - } finally { - conn.setAutoCommit(true); + oldSnapshot = serializer.deserializeJob(rs.getString(1)).snapshot(); } } + if (oldSnapshot.concurrencyKey() != null) { + lockConcurrencyGroup(conn, oldSnapshot.concurrencyKey()); + } + adjustWorkflowHoldOnTransition(conn, oldSnapshot, snapshot.currentState()); + try (PreparedStatement ps = conn.prepareStatement("UPDATE threadmill_jobs SET " + + "state = ?, queue = ?, priority = ?, handler_signature = ?, " + + "scheduled_at = ?, owner_node_id = ?, owner_heartbeat_at = ?, last_checkin_at = ?, " + + "current_state_at = ?, version = ?, body = ?, " + + "concurrency_key = ?, concurrency_mode = ?, workflow_root_id = ?, parent_job_id = ? " + + "WHERE id = ? AND version = ?")) { + ps.setString(1, snapshot.currentState().name()); + ps.setString(2, snapshot.queue()); + ps.setInt(3, snapshot.priority()); + ps.setString(4, snapshot.spec().handlerType()); + setNullableTimestamp(ps, 5, snapshot.scheduledFor()); + setNullableUuid( + ps, + 6, + snapshot.ownerNodeId() == null + ? null + : snapshot.ownerNodeId().asUuid()); + setNullableTimestamp(ps, 7, snapshot.ownerHeartbeatAt()); + setNullableTimestamp(ps, 8, snapshot.lastCheckinAt()); + ps.setTimestamp(9, Timestamp.from(currentStateAt)); + ps.setLong(10, nextVersion); + ps.setString(11, body); + setNullableConcurrency(ps, 12, snapshot.concurrencyKey(), snapshot.concurrencyMode()); + ps.setObject(14, snapshot.workflowRootId().asUuid()); + setNullableParentJobId(ps, 15, snapshot); + ps.setObject(16, snapshot.id().asUuid()); + ps.setLong(17, expectedVersion); + int rows = ps.executeUpdate(); + return rows > 0; + } }); } catch (SQLException e) { throw new JdbcException("saveAtomic failed", e); @@ -488,58 +473,45 @@ public void saveAtomic(Job job, long expectedVersion) { @Override public boolean softDelete(JobId id) { try { - return DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection()) { - conn.setAutoCommit(false); - try { - String body; - long version; - try (PreparedStatement ps = conn.prepareStatement( - "SELECT body, version FROM threadmill_jobs WHERE id = ? FOR UPDATE")) { - ps.setObject(1, id.asUuid()); - try (ResultSet rs = ps.executeQuery()) { - if (!rs.next()) { - conn.commit(); - return false; - } - body = rs.getString(1); - version = rs.getLong(2); - } - } - Job j = serializer.deserializeJob(body); - if (j.currentState() == JobState.DELETED) { - conn.commit(); + return ownedTransaction(conn -> { + String body; + long version; + try (PreparedStatement ps = + conn.prepareStatement("SELECT body, version FROM threadmill_jobs WHERE id = ? FOR UPDATE")) { + ps.setObject(1, id.asUuid()); + try (ResultSet rs = ps.executeQuery()) { + if (!rs.next()) { return false; } - JobSnapshot oldSnapshot = j.snapshot(); - if (oldSnapshot.concurrencyKey() != null) { - lockConcurrencyGroup(conn, oldSnapshot.concurrencyKey()); - } - j.transitionTo(JobState.DELETED, Instant.now(), "user.delete", null); - long nextVersion = version + 1; - JobSnapshot snapshot = withVersion(j, nextVersion); - String newBody = serializer.serializeJob(snapshot, capabilities); - Instant currentStateAt = lastTransitionTime(snapshot, JobState.DELETED); - adjustWorkflowHoldOnTransition(conn, oldSnapshot, JobState.DELETED); - try (PreparedStatement ps = conn.prepareStatement( - "UPDATE threadmill_jobs SET state = ?, version = ?, body = ?, current_state_at = ? " - + "WHERE id = ?")) { - ps.setString(1, JobState.DELETED.name()); - ps.setLong(2, nextVersion); - ps.setString(3, newBody); - ps.setTimestamp(4, Timestamp.from(currentStateAt)); - ps.setObject(5, id.asUuid()); - ps.executeUpdate(); - } - conn.commit(); - return true; - } catch (RuntimeException | SQLException e) { - conn.rollback(); - throw e; - } finally { - conn.setAutoCommit(true); + body = rs.getString(1); + version = rs.getLong(2); } } + Job j = serializer.deserializeJob(body); + if (j.currentState() == JobState.DELETED) { + return false; + } + JobSnapshot oldSnapshot = j.snapshot(); + if (oldSnapshot.concurrencyKey() != null) { + lockConcurrencyGroup(conn, oldSnapshot.concurrencyKey()); + } + j.transitionTo(JobState.DELETED, Instant.now(), "user.delete", null); + long nextVersion = version + 1; + JobSnapshot snapshot = withVersion(j, nextVersion); + String newBody = serializer.serializeJob(snapshot, capabilities); + Instant currentStateAt = lastTransitionTime(snapshot, JobState.DELETED); + adjustWorkflowHoldOnTransition(conn, oldSnapshot, JobState.DELETED); + try (PreparedStatement ps = conn.prepareStatement( + "UPDATE threadmill_jobs SET state = ?, version = ?, body = ?, current_state_at = ? " + + "WHERE id = ?")) { + ps.setString(1, JobState.DELETED.name()); + ps.setLong(2, nextVersion); + ps.setString(3, newBody); + ps.setTimestamp(4, Timestamp.from(currentStateAt)); + ps.setObject(5, id.asUuid()); + ps.executeUpdate(); + } + return true; }); } catch (SQLException e) { throw new JdbcException("softDelete failed", e); @@ -558,106 +530,95 @@ public List claimReady(NodeId nodeId, String queue, int max, Instant heartb int cap = Math.min(max, capabilities.maxClaimBatch()); try { - return DeadlockRetry.run(() -> { + return ownedTransaction(conn -> { List result = new ArrayList<>(); - try (Connection conn = dataSource.getConnection()) { - conn.setAutoCommit(false); - try { - // 1. Gather candidates with cost bounded by CLAIMABLE work, not - // by backlog depth: the unkeyed lane pages its dedicated partial - // index, and the keyed lane enumerates distinct pending keys with - // bounded index probes, drops keys whose group counters show an - // EXCLUSIVE in flight, and fetches each remaining key's earliest - // pending heads. The historical single (priority, id) pager - // walked — and FOR UPDATE locked — every concurrency-blocked row - // ahead of claimable work, so claim cost grew linearly with - // backlog (39/s -> 3/s over a 90-minute overload run). - - // 2. For each, deserialize, transition to PROCESSING, re-serialize, and UPDATE the row. - // Version-matched as defense-in-depth: correctness rests on the - // FOR UPDATE SKIP LOCKED row lock from lockClaimCandidates, but if - // a future refactor ever fetches candidates without it, this turns - // a silent double-claim into a loud failure. - try (PreparedStatement ps = conn.prepareStatement( - "UPDATE threadmill_jobs SET state = 'PROCESSING', owner_node_id = ?, " - + "owner_heartbeat_at = ?, last_checkin_at = NULL, current_state_at = ?, version = ?, body = ? " - + "WHERE id = ? AND version = ?")) { - var alreadyBatched = new HashSet(); - while (result.size() < cap) { - List pending = lockClaimCandidates(conn, queue, cap - result.size()); - // Rows batched for UPDATE in an earlier pass are still - // ENQUEUED in the database (the batch executes after the - // loop) and locked by US, so SKIP LOCKED does not hide - // them from our own re-gather. - pending = pending.stream() - .filter(p -> !alreadyBatched.contains(p.id)) - .toList(); - if (pending.isEmpty()) { - break; - } - List claimable = claimableCandidates(conn, pending, cap - result.size()); - Map bodies = fetchBodies(conn, claimable); - int quarantined = 0; - int before = result.size(); - for (var p : claimable) { - if (result.size() >= cap) break; - Job j; - try { - j = serializer.deserializeJob(bodies.get(p.id)); - } catch (RuntimeException corrupt) { - // An undeserializable body (e.g. a wire form a rollback - // can't read, or external corruption) must not fail the - // whole claim and wedge the queue. Quarantine it via a - // body-independent scalar update so it leaves the - // ENQUEUED claim path, and continue with the rest. - quarantineUnreadable(conn, p.id, p.version, heartbeatAt); - quarantined++; - continue; - } - acquireWorkflowHold(conn, j.snapshot()); - j.transitionTo(JobState.PROCESSING, heartbeatAt, "engine.claim", null); - j.assignOwner(nodeId, heartbeatAt); - j.incrementAttempts(); - long nextVersion = p.version + 1; - JobSnapshot snap = withVersion(j, nextVersion); - String newBody = serializer.serializeJob(snap, capabilities); - ps.setObject(1, nodeId.asUuid()); - ps.setTimestamp(2, Timestamp.from(heartbeatAt)); - ps.setTimestamp(3, Timestamp.from(heartbeatAt)); - ps.setLong(4, nextVersion); - ps.setString(5, newBody); - ps.setObject(6, p.id); - ps.setLong(7, p.version); - ps.addBatch(); - alreadyBatched.add(p.id); - result.add(serializer.deserializeJob(newBody)); - } - // No claims and no quarantines means every gathered head is - // concurrency-inadmissible right now — a further pass would - // gather the same heads again. Quarantines are progress: the - // poison left ENQUEUED, so the next pass sees its successor. - if (result.size() == before && quarantined == 0) { - break; - } - } - int[] updated = ps.executeBatch(); - for (int count : updated) { - if (count != 1) { - throw new IllegalStateException( - "Claim UPDATE matched " + count + " rows — the row lock taken by " - + "readClaimPage no longer guarantees claim exclusivity"); - } + // 1. Gather candidates with cost bounded by CLAIMABLE work, not + // by backlog depth: the unkeyed lane pages its dedicated partial + // index, and the keyed lane enumerates distinct pending keys with + // bounded index probes, drops keys whose group counters show an + // EXCLUSIVE in flight, and fetches each remaining key's earliest + // pending heads. The historical single (priority, id) pager + // walked — and FOR UPDATE locked — every concurrency-blocked row + // ahead of claimable work, so claim cost grew linearly with + // backlog (39/s -> 3/s over a 90-minute overload run). + + // 2. For each, deserialize, transition to PROCESSING, re-serialize, and UPDATE the row. + // Version-matched as defense-in-depth: correctness rests on the + // FOR UPDATE SKIP LOCKED row lock from lockClaimCandidates, but if + // a future refactor ever fetches candidates without it, this turns + // a silent double-claim into a loud failure. + try (PreparedStatement ps = + conn.prepareStatement("UPDATE threadmill_jobs SET state = 'PROCESSING', owner_node_id = ?, " + + "owner_heartbeat_at = ?, last_checkin_at = NULL, current_state_at = ?, version = ?, body = ? " + + "WHERE id = ? AND version = ?")) { + var alreadyBatched = new HashSet(); + while (result.size() < cap) { + List pending = lockClaimCandidates(conn, queue, cap - result.size()); + // Rows batched for UPDATE in an earlier pass are still + // ENQUEUED in the database (the batch executes after the + // loop) and locked by US, so SKIP LOCKED does not hide + // them from our own re-gather. + pending = pending.stream() + .filter(p -> !alreadyBatched.contains(p.id)) + .toList(); + if (pending.isEmpty()) { + break; + } + List claimable = claimableCandidates(conn, pending, cap - result.size()); + Map bodies = fetchBodies(conn, claimable); + int quarantined = 0; + int before = result.size(); + for (var p : claimable) { + if (result.size() >= cap) break; + Job j; + try { + j = serializer.deserializeJob(bodies.get(p.id)); + } catch (RuntimeException corrupt) { + // An undeserializable body (e.g. a wire form a rollback + // can't read, or external corruption) must not fail the + // whole claim and wedge the queue. Quarantine it via a + // body-independent scalar update so it leaves the + // ENQUEUED claim path, and continue with the rest. + quarantineUnreadable(conn, p.id, p.version, heartbeatAt); + quarantined++; + continue; } + acquireWorkflowHold(conn, j.snapshot()); + j.transitionTo(JobState.PROCESSING, heartbeatAt, "engine.claim", null); + j.assignOwner(nodeId, heartbeatAt); + j.incrementAttempts(); + long nextVersion = p.version + 1; + JobSnapshot snap = withVersion(j, nextVersion); + String newBody = serializer.serializeJob(snap, capabilities); + ps.setObject(1, nodeId.asUuid()); + ps.setTimestamp(2, Timestamp.from(heartbeatAt)); + ps.setTimestamp(3, Timestamp.from(heartbeatAt)); + ps.setLong(4, nextVersion); + ps.setString(5, newBody); + ps.setObject(6, p.id); + ps.setLong(7, p.version); + ps.addBatch(); + alreadyBatched.add(p.id); + result.add(serializer.deserializeJob(newBody)); + } + // No claims and no quarantines means every gathered head is + // concurrency-inadmissible right now — a further pass would + // gather the same heads again. Quarantines are progress: the + // poison left ENQUEUED, so the next pass sees its successor. + if (result.size() == before && quarantined == 0) { + break; + } + } + int[] updated = ps.executeBatch(); + for (int count : updated) { + if (count != 1) { + throw new IllegalStateException( + "Claim UPDATE matched " + count + " rows — the row lock taken by " + + "readClaimPage no longer guarantees claim exclusivity"); } - conn.commit(); - return result; - } catch (RuntimeException | SQLException e) { - conn.rollback(); - throw e; - } finally { - conn.setAutoCommit(true); } } + return result; }); } catch (SQLException e) { throw new JdbcException("claimReady failed", e); @@ -1579,68 +1540,54 @@ public boolean replaceJob(JobId id, long expectedVersion, JobReplacement replace Objects.requireNonNull(id, "id"); Objects.requireNonNull(replacement, "replacement"); try { - return DeadlockRetry.run(() -> { - try (Connection conn = dataSource.getConnection()) { - conn.setAutoCommit(false); - try { - String body; - long version; - String state; - try (PreparedStatement ps = conn.prepareStatement( - "SELECT body, version, state FROM threadmill_jobs WHERE id = ? FOR UPDATE")) { - ps.setObject(1, id.asUuid()); - try (ResultSet rs = ps.executeQuery()) { - if (!rs.next()) { - conn.commit(); - return false; - } - body = rs.getString(1); - version = rs.getLong(2); - state = rs.getString(3); - } - } - if (version != expectedVersion) { - conn.commit(); - throw new StaleJobException(id, expectedVersion); - } - if (!isReplaceableState(state)) { - conn.commit(); + return ownedTransaction(conn -> { + String body; + long version; + String state; + try (PreparedStatement ps = conn.prepareStatement( + "SELECT body, version, state FROM threadmill_jobs WHERE id = ? FOR UPDATE")) { + ps.setObject(1, id.asUuid()); + try (ResultSet rs = ps.executeQuery()) { + if (!rs.next()) { return false; } - Job current = serializer.deserializeJob(body); - Job replaced = JobReplacements.apply(current, replacement); - long nextVersion = version + 1; - JobSnapshot snap = withVersion(replaced, nextVersion); - String newBody = serializer.serializeJob(snap, capabilities); - Instant currentStateAt = lastTransitionTime(snap, snap.currentState()); - try (PreparedStatement ps = conn.prepareStatement("UPDATE threadmill_jobs SET " - + "queue = ?, priority = ?, handler_signature = ?, scheduled_at = ?, " - + "current_state_at = ?, version = ?, body = ?, " - + "concurrency_key = ?, concurrency_mode = ?, workflow_root_id = ?, parent_job_id = ? " - + "WHERE id = ? AND version = ?")) { - ps.setString(1, snap.queue()); - ps.setInt(2, snap.priority()); - ps.setString(3, snap.spec().handlerType()); - setNullableTimestamp(ps, 4, snap.scheduledFor()); - ps.setTimestamp(5, Timestamp.from(currentStateAt)); - ps.setLong(6, nextVersion); - ps.setString(7, newBody); - setNullableConcurrency(ps, 8, snap.concurrencyKey(), snap.concurrencyMode()); - ps.setObject(10, snap.workflowRootId().asUuid()); - setNullableParentJobId(ps, 11, snap); - ps.setObject(12, id.asUuid()); - ps.setLong(13, expectedVersion); - int rows = ps.executeUpdate(); - conn.commit(); - return rows > 0; - } - } catch (RuntimeException | SQLException e) { - conn.rollback(); - throw e; - } finally { - conn.setAutoCommit(true); + body = rs.getString(1); + version = rs.getLong(2); + state = rs.getString(3); } } + if (version != expectedVersion) { + throw new StaleJobException(id, expectedVersion); + } + if (!isReplaceableState(state)) { + return false; + } + Job current = serializer.deserializeJob(body); + Job replaced = JobReplacements.apply(current, replacement); + long nextVersion = version + 1; + JobSnapshot snap = withVersion(replaced, nextVersion); + String newBody = serializer.serializeJob(snap, capabilities); + Instant currentStateAt = lastTransitionTime(snap, snap.currentState()); + try (PreparedStatement ps = conn.prepareStatement("UPDATE threadmill_jobs SET " + + "queue = ?, priority = ?, handler_signature = ?, scheduled_at = ?, " + + "current_state_at = ?, version = ?, body = ?, " + + "concurrency_key = ?, concurrency_mode = ?, workflow_root_id = ?, parent_job_id = ? " + + "WHERE id = ? AND version = ?")) { + ps.setString(1, snap.queue()); + ps.setInt(2, snap.priority()); + ps.setString(3, snap.spec().handlerType()); + setNullableTimestamp(ps, 4, snap.scheduledFor()); + ps.setTimestamp(5, Timestamp.from(currentStateAt)); + ps.setLong(6, nextVersion); + ps.setString(7, newBody); + setNullableConcurrency(ps, 8, snap.concurrencyKey(), snap.concurrencyMode()); + ps.setObject(10, snap.workflowRootId().asUuid()); + setNullableParentJobId(ps, 11, snap); + ps.setObject(12, id.asUuid()); + ps.setLong(13, expectedVersion); + int rows = ps.executeUpdate(); + return rows > 0; + } }); } catch (SQLException e) { throw new JdbcException("replaceJob failed", e); diff --git a/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/NonAutoCommitDataSource.java b/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/NonAutoCommitDataSource.java new file mode 100644 index 0000000..aa7a29a --- /dev/null +++ b/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/NonAutoCommitDataSource.java @@ -0,0 +1,93 @@ +package com.hemju.threadmill.store.postgres; + +import java.io.PrintWriter; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Proxy; +import java.sql.Connection; +import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; +import java.util.logging.Logger; + +import javax.sql.DataSource; + +/** Pool-alike test fixture whose borrowed connections start and must end with auto-commit disabled. */ +final class NonAutoCommitDataSource implements DataSource { + + private final DataSource delegate; + + NonAutoCommitDataSource(DataSource delegate) { + this.delegate = delegate; + } + + @Override + public Connection getConnection() throws SQLException { + return guarded(delegate.getConnection()); + } + + @Override + public Connection getConnection(String username, String password) throws SQLException { + return guarded(delegate.getConnection(username, password)); + } + + private static Connection guarded(Connection connection) throws SQLException { + connection.setAutoCommit(false); + return (Connection) Proxy.newProxyInstance( + NonAutoCommitDataSource.class.getClassLoader(), + new Class[] {Connection.class}, + (proxy, method, arguments) -> { + if (method.getName().equals("close") && method.getParameterCount() == 0) { + SQLException modeFailure = connection.getAutoCommit() + ? new SQLException("borrowed connection was not restored to autoCommit=false") + : null; + try { + connection.close(); + } catch (SQLException closeFailure) { + if (modeFailure == null) throw closeFailure; + modeFailure.addSuppressed(closeFailure); + } + if (modeFailure != null) throw modeFailure; + return null; + } + try { + return method.invoke(connection, arguments); + } catch (InvocationTargetException e) { + throw e.getCause(); + } + }); + } + + @Override + public PrintWriter getLogWriter() throws SQLException { + return delegate.getLogWriter(); + } + + @Override + public void setLogWriter(PrintWriter out) throws SQLException { + delegate.setLogWriter(out); + } + + @Override + public void setLoginTimeout(int seconds) throws SQLException { + delegate.setLoginTimeout(seconds); + } + + @Override + public int getLoginTimeout() throws SQLException { + return delegate.getLoginTimeout(); + } + + @Override + public Logger getParentLogger() throws SQLFeatureNotSupportedException { + return delegate.getParentLogger(); + } + + @Override + public T unwrap(Class iface) throws SQLException { + return delegate.unwrap(iface); + } + + @Override + public boolean isWrapperFor(Class iface) throws SQLException { + return delegate.isWrapperFor(iface); + } +} diff --git a/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/PostgresJobStoreContractTest.java b/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/PostgresJobStoreContractTest.java index 9fd6195..065d129 100644 --- a/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/PostgresJobStoreContractTest.java +++ b/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/PostgresJobStoreContractTest.java @@ -17,8 +17,9 @@ /** * Runs the {@link AbstractJobStoreContractTest} against real PostgreSQL via - * Testcontainers. The exact same 20 tests that the in-memory store passes - * must also pass here — that is the contract. + * Testcontainers. The exact same contract suite that the in-memory store + * passes must also pass here — with every borrowed connection starting at + * {@code autoCommit=false} and asserting that the store restores that mode. */ class PostgresJobStoreContractTest extends AbstractJobStoreContractTest { @@ -56,7 +57,7 @@ void truncateBetweenTests() throws Exception { Statement st = conn.createStatement()) { st.execute("TRUNCATE threadmill_jobs, threadmill_nodes, threadmill_metadata, " + "threadmill_cron_tasks, threadmill_mutexes, threadmill_leases, " - + "threadmill_dedup_keys, threadmill_concurrency_groups, " + + "threadmill_dedup_keys, threadmill_queue_pauses, threadmill_concurrency_groups, " + "threadmill_concurrency_workflow_holds RESTART IDENTITY CASCADE"); // The counts table is kept in sync by triggers, but TRUNCATE bypasses them — reset counts manually. st.execute("UPDATE threadmill_job_counts SET count = 0"); @@ -65,6 +66,6 @@ void truncateBetweenTests() throws Exception { @Override protected JobStore createStore() { - return new PostgresJobStore(dataSource); + return new PostgresJobStore(new NonAutoCommitDataSource(dataSource)); } } diff --git a/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/PostgresJobStoreRegressionTest.java b/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/PostgresJobStoreRegressionTest.java index dbb8540..4936f03 100644 --- a/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/PostgresJobStoreRegressionTest.java +++ b/threadmill-store-postgres/src/test/java/com/hemju/threadmill/store/postgres/PostgresJobStoreRegressionTest.java @@ -3,7 +3,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; -import java.io.PrintWriter; import java.sql.Connection; import java.sql.PreparedStatement; import java.sql.ResultSet; @@ -23,7 +22,6 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; -import java.util.logging.Logger; import javax.sql.DataSource; @@ -94,7 +92,7 @@ void migrate() throws SQLException { Statement st = conn.createStatement()) { st.execute("TRUNCATE threadmill_jobs, threadmill_nodes, threadmill_metadata, " + "threadmill_cron_tasks, threadmill_mutexes, threadmill_leases, " - + "threadmill_dedup_keys, threadmill_concurrency_groups, " + + "threadmill_dedup_keys, threadmill_queue_pauses, threadmill_concurrency_groups, " + "threadmill_concurrency_workflow_holds RESTART IDENTITY CASCADE"); st.execute("UPDATE threadmill_job_counts SET count = 0"); } @@ -672,6 +670,38 @@ void dropThreadmillObjectsAllowsCleanReinitialize() throws SQLException { new MigrationRunner(dataSource).validate(); } + @Test + void migrationBootstrapCommitsWhenConnectionsDefaultToNonAutoCommit() throws SQLException { + try (Connection conn = dataSource.getConnection(); + Statement st = conn.createStatement()) { + st.execute("ALTER TABLE threadmill_schema_history DROP COLUMN checksum"); + } + + new MigrationRunner(new NonAutoCommitDataSource(dataSource)).migrate(); + + try (Connection conn = dataSource.getConnection(); + Statement st = conn.createStatement(); + ResultSet rs = st.executeQuery("SELECT checksum FROM threadmill_schema_history LIMIT 1")) { + assertThat(rs.next()).isTrue(); + } + } + + @Test + void schemaDropAndRemigrateCommitWhenConnectionsDefaultToNonAutoCommit() throws SQLException { + var runner = new MigrationRunner(new NonAutoCommitDataSource(dataSource)); + runner.dropThreadmillObjects(); + + try (Connection conn = dataSource.getConnection(); + Statement st = conn.createStatement(); + ResultSet rs = st.executeQuery("SELECT to_regclass('threadmill_jobs')")) { + assertThat(rs.next()).isTrue(); + assertThat(rs.getString(1)).isNull(); + } + + runner.migrate(); + new MigrationRunner(dataSource).validate(); + } + @Test void concurrentCleanSchemaMigrationsAreSerialized() throws Exception { dropSchemaObjects(); @@ -937,6 +967,8 @@ void selfOwnedWritesCommitWhenConnectionsDefaultToNonAutoCommit() { .isTrue(); assertThat(observer.tryAcquireMutex("non-auto-commit-mutex", "observer", Duration.ofMinutes(1))) .isFalse(); + + writer.resumeQueue("low-priority"); } @Test @@ -981,64 +1013,6 @@ private static Job awaitingChildOf(Job parent, int index) { .build(); } - /** A pool-alike whose connections arrive with {@code autoCommit=false}. */ - private static final class NonAutoCommitDataSource implements DataSource { - private final DataSource delegate; - - private NonAutoCommitDataSource(DataSource delegate) { - this.delegate = delegate; - } - - @Override - public Connection getConnection() throws SQLException { - var connection = delegate.getConnection(); - connection.setAutoCommit(false); - return connection; - } - - @Override - public Connection getConnection(String username, String password) throws SQLException { - var connection = delegate.getConnection(username, password); - connection.setAutoCommit(false); - return connection; - } - - @Override - public PrintWriter getLogWriter() throws SQLException { - return delegate.getLogWriter(); - } - - @Override - public void setLogWriter(PrintWriter out) throws SQLException { - delegate.setLogWriter(out); - } - - @Override - public void setLoginTimeout(int seconds) throws SQLException { - delegate.setLoginTimeout(seconds); - } - - @Override - public int getLoginTimeout() throws SQLException { - return delegate.getLoginTimeout(); - } - - @Override - public Logger getParentLogger() { - return Logger.getLogger("test"); - } - - @Override - public T unwrap(Class iface) throws SQLException { - return delegate.unwrap(iface); - } - - @Override - public boolean isWrapperFor(Class iface) throws SQLException { - return delegate.isWrapperFor(iface); - } - } - private static void dropSchemaObjects() throws SQLException { try (Connection conn = dataSource.getConnection(); Statement st = conn.createStatement()) { diff --git a/threadmill-store-redis/src/main/java/com/hemju/threadmill/store/redis/RedisJobStore.java b/threadmill-store-redis/src/main/java/com/hemju/threadmill/store/redis/RedisJobStore.java index 1199bd8..9180db8 100644 --- a/threadmill-store-redis/src/main/java/com/hemju/threadmill/store/redis/RedisJobStore.java +++ b/threadmill-store-redis/src/main/java/com/hemju/threadmill/store/redis/RedisJobStore.java @@ -1910,9 +1910,8 @@ public NudgeOutcome requestCronNudge(String taskName, Instant requestedAt) { // cannot race a concurrent deleteCronTask, so a nudge can never // resurrect schedule state for a removed task. HINCRBY generates the // strictly monotonic revision that compare-and-clear uses as its - // collision-free identity while this task's state hash exists. - // Always-string return - // -> ScriptOutputType.VALUE per the Lua return-value conventions. + // collision-free identity while this task's state hash exists. The + // script always returns a string, hence ScriptOutputType.VALUE. try { String outcome = evalScript( """ From 17d99e66d514dd691f9ce778c5c1b4c66a2a8655 Mon Sep 17 00:00:00 2001 From: Helmut Michael Juskewycz Date: Wed, 12 Aug 2026 11:20:10 +0200 Subject: [PATCH 6/6] fix(core): prevent spurious nudge from stale listing --- .../core/schedule/RecurringMaterializer.java | 5 +++++ .../store/memory/SchedulingTest.java | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/RecurringMaterializer.java b/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/RecurringMaterializer.java index 7034b04..1beb17f 100644 --- a/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/RecurringMaterializer.java +++ b/threadmill-core/src/main/java/com/hemju/threadmill/core/schedule/RecurringMaterializer.java @@ -196,6 +196,11 @@ private void tickOneLocked(CronTask listed, Instant now) { } } + // The listed definition may simply be stale relative to an + // authoritative definition and state that already agree. In that + // case no scheduled firing or nudge is owed this tick. + if (!due && nudge == null) return; + // Pile-up guard: an in-flight instance that is still going to run // blocks the next materialization. if (state.inFlightJobId() != null) { diff --git a/threadmill-store-memory/src/test/java/com/hemju/threadmill/store/memory/SchedulingTest.java b/threadmill-store-memory/src/test/java/com/hemju/threadmill/store/memory/SchedulingTest.java index be4eafa..25091cc 100644 --- a/threadmill-store-memory/src/test/java/com/hemju/threadmill/store/memory/SchedulingTest.java +++ b/threadmill-store-memory/src/test/java/com/hemju/threadmill/store/memory/SchedulingTest.java @@ -1526,6 +1526,27 @@ public List listCronTasks() { .isNull(); } + @Test + void staleListingWhoseDefinitionAlreadyAgreesWithStateMaterializesNothing() { + scheduler.defineIntervalTask("racy", Duration.ofMinutes(5), new HelloPayload("tick"), RecorderHandler.class); + var stale = store.findCronTask("racy").orElseThrow(); + scheduler.defineIntervalTask("racy", Duration.ofMinutes(7), new HelloPayload("tick"), RecorderHandler.class); + var committedState = store.findCronTaskState("racy").orElseThrow(); + + var staleListing = new ForwardingJobStore(store) { + @Override + public List listCronTasks() { + return List.of(stale); + } + }; + new RecurringMaterializer(staleListing).tick(committedState.nextRunAt().minusNanos(1)); + + assertThat(store.findByHandlerSignature(RecorderHandler.class.getName(), 10)) + .as("a stale listing must not invent a nudge after the authoritative definition and state agree") + .isEmpty(); + assertThat(store.findCronTaskState("racy")).contains(committedState); + } + @Test void materializerRechecksEnabledUnderTheTaskMutexBeforeActing() { // The same list-then-mutex window, for the enabled bit: a disable