Fix recurring nudge persistence and add failover simulations

[hemju]

Summary

• Make every Postgres write that owns its connection commit through the
  non-joining ownedTransaction boundary, including the expired-dedup
  conflict cleanup fallback.
• Repair a recurring timing-fingerprint mismatch under the task mutex by
  treating the current durable definition as authoritative, preserving
  bookkeeping and nudge demand while discarding the obsolete trigger's
  backlog.
• Add fixed, process-separated Postgres and Redis simulations for maintenance
  leader hard kill and producer crash between durable work and nudge.
• Record the decision not to add per-task lifecycle generations to the
  JobStore SPI unless evidence shows the suspended-then-resumed,
  lease-expiry, same-name-reuse ABA is occurring.

Why

Postgres self-owned writes previously relied on host pools returning
autoCommit=true; with a non-auto-commit connection, successful-looking
writes were silently rolled back when the connection closed. The recurring
materializer also lacked an explicit recovery policy when its under-mutex
reload paired a new durable task definition with stale timing state.

The new simulations exercise the two process boundaries that unit and soak
coverage did not reach. They prove that an accepted nudge survives a dead
maintenance leader and is served by the standby, while durable work committed
by a producer killed before its nudge is recovered by the regular recurring
backstop.

That evidence also bounds issue #113: SIGKILL removes the stale clearer.
Revision reuse additionally requires an old materializer to resume after
outliving its 30-second mutex lease and a same-name delete/re-registration. A
durable generation/high-water record would not replace the existing lifecycle
orderings and does not justify its cross-backend retention, migration, and SPI
cost without evidence of that sequence.

Impact

Hosts may safely supply Postgres connections with autoCommit=false for
Threadmill-owned operations. Crashed recurring timing edits now self-heal
deterministically without firing an obsolete schedule. Maintainers gain
repeatable real-backend evidence for nudge failover and the documented
producer crash window.

Validation

• ./gradlew check green at every one of the four issue commits.
• PostgresJobStoreRegressionTest.selfOwnedWritesCommitWhenConnectionsDefaultToNonAutoCommit
• SchedulingTest.materializerRepairsMismatchedTimingFingerprintWithoutCatchingUpTheOldSchedule
• ./gradlew :threadmill-simulation:simulateNudgePostgres
• ./gradlew :threadmill-simulation:simulateNudgeRedis
• Matched 15-second Postgres soak before/after the transaction fix: 752
  enqueued at 50 jobs/s in both runs.

Closes #111
Closes #112
Closes #114
Closes #113

[hemju]

Deep-dive review: I found one scheduling correctness blocker plus several consistency and maintainability improvements. The existing project checks, Javadoc, and both real-backend nudge simulations pass; the blocking future-nextRunAt fingerprint case was reproduced separately.

[hemju]

Blocking — repair mismatches before the due/nudge early return. This comparison is never reached when the stale state has a future nextRunAt and no nudge, because line 144 returns first. An interrupted edit from a weekly interval to a one-minute interval therefore remains on the old fingerprint and can stay dormant until the obsolete weekly fire. I reproduced that shape: the state remained interval:PT168H instead of repairing to interval:PT1M. Please make a fingerprint mismatch trigger the under-mutex reload/repair even when the stale schedule is not yet due, and add a regression with a future stale nextRunAt.

[hemju]

Please align this architectural claim with the remaining inline transactions. saveAtomic, softDelete, claimReady, and replaceJob still manage their own connections and unconditionally restore autoCommit=true rather than the previous value. They do commit correctly with an autoCommit=false pool, so issue #111's data-loss bug is fixed, but this statement (and the equivalent AGENTS.md wording) is broader than the implementation. Either route those four through the common owning boundary or narrow the documentation to the formerly auto-commit-dependent operations.

[hemju]

Suggestion — avoid presenting unbounded per-name retention as inherent. The decision not to add another cross-backend mechanism is reasonable given the narrow suspended-process sequence, but lifecycle identity could also use a store-global monotonically increasing creation sequence or a fresh durable lifecycle nonce copied into task/state; either is constant storage and survives same-name reuse. I would base the rejection primarily on reachability and SPI/migration cost, and describe unbounded per-name retention as one implementation option rather than a requirement.

[hemju]

Suggestion — wire this fixed correctness simulation into a release gate. Neither check nor the root productionCheck depends on simulateNudge, so this process-separation coverage can silently rot. Both real-backend tasks completed locally in about 23 seconds; adding :threadmill-simulation:simulateNudge to productionCheck would make the new guarantee part of release-candidate validation.

[hemju]

Make the ready marker atomic. The supervisor treats Files.isRegularFile(readyFile) as readiness and immediately parses it, while writeString creates the file before all JSON bytes are necessarily visible. A scheduler interleaving can expose an empty or partial marker and make the simulation fail nondeterministically. Write a sibling temp file and atomically move it into place, or retry parsing until the deadline.

[hemju]

Handle partial channel writes. FileChannel.write is allowed to consume fewer bytes than remain in the buffer. The records are small and local files usually complete in one call, but a short write would leave invalid JSONL and fail the verifier for the wrong reason. Keep the buffer and loop while hasRemaining() under the existing file lock.

[hemju]

Deep-dive review

Reviewed the full diff against db4b2ed plus the surrounding code each change depends on. spotlessCheck and the core / memory / dashboard-api suites are green locally.

Overall this is strong work and the reasoning is well captured in the docs. Four things I'd want resolved before merge, then a set of smaller items — all inline.

Would block

1. Issue #111 is only half fixed. MigrationRunner still runs DDL on whatever auto-commit mode the pool supplies — see the comment on threadmill-store-postgres/README.md. dropThreadmillObjects() (Spring schema-mode=drop-and-migrate) becomes a silent no-op, and the checksum column backfill never becomes durable on an already-migrated database, which permanently disables migration-drift detection for that installation.
2. Four self-owned writes still bypass ownedTransaction (saveAtomic, softDelete, claimReady, replaceJob) and force setAutoCommit(true) rather than restoring the borrowed mode — the mirror image of #111. The store README and AGENTS.md both state the stronger invariant as if it already held.
3. The #112 repair never fires for a task that isn't due, so a crashed edit that lengthens the stale time-to-next-fire leaves the new trigger dormant — exactly the outcome the new comment says it exists to prevent. The fix is free; tick() already holds the definition.
4. The nudge simulation has two wall-clock races it must win inside an 8s window while allowing up to 20s of process-start timeouts. Red rather than false-green, but with misleading messages on a slow machine.

Verified correct

Things I specifically checked that hold up: ownedTransaction retains DeadlockRetry, so none of the converted paths lost retry protection; nudge cells survive upsertCronTaskState on all three backends including the new repair write; the repair preserves lastRunAt / lastRunJobId / inFlightJobId; nextAfter is strictly-after so there is no repeat-repair loop; upsertCron's re-enable path writes state-then-task (inverted vs. the normal path) but the materializer's !task.enabled() recheck precedes the fingerprint check, so it cannot misfire in that window; the simulation's defineRecurring(..., true, DROP) does land on the exclusive overload; and the build file adds no dependencies, so no lockfile update is owed.

[hemju]

Blocking — this claim isn't true yet for the migration path.

MigrationRunner was not touched by this PR and still depends on the pool's auto-commit default. applyOne manages its own boundary correctly, but ensureHistoryTable(conn) (called at MigrationRunner:87, defined at :267) and dropThreadmillObjects() (:221) run DDL with no explicit transaction.

With autoCommit=false connections — the exact premise of this PR:

• migrate() on an already-migrated database: ALTER TABLE threadmill_schema_history ADD COLUMN IF NOT EXISTS checksum TEXT runs, no applyOne follows, so nothing ever commits and conn.close() discards it. Every startup repeats it. Because validateAppliedMigrations skips NULL checksums, migration-drift detection silently never engages for that installation.
• dropThreadmillObjects() — wired at ThreadmillPostgresAutoConfiguration:83 for schema-mode=drop-and-migrate — is a silent no-op: the DROPs roll back, then migrate() finds everything applied. The operator's destructive reset doesn't happen and nothing says so.
• Fresh-database migrate() only works by accident: applyOne's commit() sweeps up the earlier uncommitted CREATE TABLE.

Suggest giving both methods the same explicit boundary applyOne already has (or setting/restoring auto-commit once at the top of migrate() / dropThreadmillObjects()), and extending selfOwnedWritesCommitWhenConnectionsDefaultToNonAutoCommit to cover a migrate + drop-and-migrate cycle through NonAutoCommitDataSource.

[hemju]

Blocking — four self-owned writes still don't route through here.

saveAtomic (:416/:475), softDelete (:493/:540), claimReady (:564/:658) and replaceJob (:1584/:1641) all open their own connection and hand-roll the transaction, ending in finally { conn.setAutoCommit(true); } — forcing true rather than restoring the borrowed mode. That's the mirror image of #111: Threadmill hands the connection back flipped to auto-commit for whoever borrows it next. It's invisible under Hikari (which resets on return), and equally invisible for exactly the pools where #111 bit.

Best fix is to route those four through ownedTransaction. They're already non-joining, the semantics are identical, it deletes four duplicated blocks, and it makes "self-owned write" a single enforceable idiom instead of one a future contributor can silently opt out of by copy-pasting a neighbouring read.

The Javadoc here is accurate as written (it describes what's routed here). The over-broad claims are in the store README and in AGENTS.md.

[hemju]

"Operations that open their own connection route through PostgresJobStore.ownedTransaction" isn't true yet — saveAtomic, softDelete, claimReady and replaceJob open their own connection, hand-roll the boundary, and force setAutoCommit(true) instead of restoring the prior mode. Same over-statement as threadmill-store-postgres/README.md:149.

Either route those four through ownedTransaction (preferred — see the comment on PostgresJobStore) or scope this bullet to the single-statement writes it actually covers. Given AGENTS.md is the project's memory, I'd rather the code move to match the claim than the claim be narrowed.

[hemju]

Blocking — the repair only runs when the stale timing says a run is due.

tickOneLocked returns at line 144 (if (!due && nudge == null) return;) before this check ever executes. So a crashed edit is only repaired if the obsolete nextRunAt happens to be in the past.

Concrete miss: task is 0 3 1 1 * (yearly), operator edits it to */5 * * * *, crash between the task write and the state write. The stale nextRunAt is next January — never due — so the mismatch is never observed and the new every-5-minutes trigger stays dormant until something re-registers the task. That is precisely the "leave the task dormant until some future re-registration" outcome the comment below says this block exists to prevent; today it's prevented in the shorten-the-interval direction only.

The fix is free: tick() already holds the fully populated CronTask from listCronTasks(), so CronTaskScheduleState.timingFingerprintOf(listed) costs nothing. Use a listed-vs-state mismatch as an additional reason to proceed past the early return; the under-mutex reload still re-checks against the authoritative definition, so a stale listed only ever costs a wasted findCronTask — the "idle ticks stay at one state read" property is preserved for the common case.

[hemju]

The repair rewrites next_run_at and suppresses a firing with no operator-visible signal — in a class where the CATCH_UP cap logs debug and a failed tick logs warn. A LOG.warn naming the task, the old and new fingerprints, and the new nextRunAt would make a crashed edit diagnosable after the fact, and would surface the pathological case where a store fails to persist the fingerprint and every tick re-repairs the same task forever (silent permanent dormancy today).

Minor, same block: this is the only site in the file that uses the 8-arg constructor carrying the nudge cells; every other writer uses the 6-arg form because upsertCronTaskState never persists those fields anyway. Not wrong — arguably more faithful for the in-memory record — but worth a one-line comment so a future reader doesn't conclude the nudge is being written here.

Also: when repair and nudge coincide, this write is immediately followed by a second upsertCronTaskState in the nudge branch. Harmless, but two round trips where one would do.

[hemju]

This is now the third trace writer in the module — TraceWriter, WorkerChurnTraceLog, and this one. It and WorkerChurnTraceLog are near-identical (same PROCESS_LOCKS + FileChannel.lock() + appendLocked shape); the only substantive difference is Jackson vs. a hand-rolled escaper, and this version is clearly the better of the two.

They also disagree on the timestamp key — "ts" there, "timestamp" here — for no reason, inside one module whose whole output contract is JSON-lines traces an agent is expected to read cold.

Suggest promoting one shared cross-process writer (this implementation) into the parent package and deleting WorkerChurnTraceLog; failing that, at least align the key.

[hemju]

This puts simulation state inside the engine's own namespace. AGENTS.md reserves {threadmill}: for engine keys, and RedisJobStore.dropThreadmillKeys() matches {threadmill}:* — so the simulation's work rows sit inside the blast radius of a Threadmill development reset. Nothing in this run calls it, but the invariant "{threadmill}: is Threadmill's" is worth keeping clean; the cluster hash tag buys nothing here since the simulation runs against a standalone container.

Suggest {threadmill-simulation}:nudge:<runId>:pending or an untagged threadmill_simulation:....

Same point, lower stakes, for threadmill_simulation_nudge_work in PostgresWorkStore: it isn't in MigrationRunner.THREADMILL_TABLES so it's functionally safe, but it squats the product's documented table prefix.

[hemju]

closeQuietly throws IllegalStateException — the name promises the opposite of what it does, and "quietly" is a well-established convention for swallow-and-continue. Throwing is the right behaviour for a simulation fixture; just rename it (closeOrFail).

[hemju]

RedisWorkStore opens and closes a fresh client.connect() per operation, and PostgresWorkStore takes a fresh unpooled PGSimpleDataSource connection per call. The supervisor polls isPending every 25ms for up to 16s, so a single phase costs hundreds of TCP + auth round trips against the datastore under test.

Hold one connection per work store (Lettuce connections are thread-safe and long-lived by design). Same for PostgresWorkStore — or at minimum poll on a longer interval.

[hemju]

Nit: the reflow leaves // Always-string return stranded on its own line mid-sentence. Worth rewrapping the paragraph.

[hemju]

Re-review of 02f5f75

Went through the whole follow-up commit. Almost everything from the first pass is properly resolved, and several fixes went further than what I suggested — the NonAutoCommitDataSource proxy asserting mode restoration on close(), wiring it into the full PostgresJobStoreContractTest suite, simulateNudge joining productionCheck, the atomic ready-marker publish, and the while (buffer.hasRemaining()) partial-write fix in both trace writers (that last one was a latent bug neither of us flagged).

One new blocker, though: the fix for the "repair never fires when not due" finding introduced a regression. Details and a reproduction inline on RecurringMaterializer.java. Short version: listedFingerprintMismatch broke the invariant that reaching the if (!due) nudge branch implies nudge != null, so a stale listing can now materialize a spurious origin=nudge instance and then NPE. I ran it against the current commit — confirmed.

Resolved

First-pass finding	Status
MigrationRunner auto-commit dependency	Fixed — ensureHistoryTable / dropThreadmillObjects / applyOne share one inTransaction helper, plus two new regression tests
saveAtomic / softDelete / claimReady / replaceJob bypassing ownedTransaction	Fixed — all four routed through it, four duplicated blocks deleted
README / AGENTS.md over-statement	Fixed, and now true
Repair unreachable when not due	Fixed via the listed fingerprint — but see the inline comment
Null fingerprint costing a firing	Fixed — legacy nulls adopt the fingerprint without moving nextRunAt; Javadoc corrected
No log on repair	Fixed — LOG.warn, correctly suppressed for the legacy adopt path
Untested repair branches	Fixed — five new tests incl. the nudge path, the DROP variant, and the missing-state case
Contract-suite coverage of the invariant	Fixed, and stronger than suggested
threadmill_queue_pauses leak	Fixed — resumeQueue plus both TRUNCATE lists
Dedup FOR UPDATE window	Documented in the CHANGELOG
Simulation timing races	Fixed exactly as proposed — 1 min / 5 min for failover, then an 8s re-registration for the crash phase
Tautological trace assertions	Removed; .path() used throughout
{threadmill}: namespace squatting	Fixed — threadmill-simulation: and nudge_simulation_work
closeQuietly misnomer	Renamed
Per-call datastore connections	Fixed — long-lived connections both backends
RedisJobStore comment reflow	Fixed

Not addressed, and fine to leave: extracting a NudgeTraceVerifier and consolidating the three trace writers. The timestamp key is now aligned (timestamp in both), which was the part that actually mattered.

./gradlew spotlessCheck :threadmill-core:test :threadmill-store-memory:test :threadmill-dashboard-api:test :threadmill-simulation:compileJava is green on 02f5f75 — the regression below is not caught by any existing test.

[hemju]

Blocking — this fix broke the invariant that guarded the nudge branch.

Before this commit, if (!due && nudge == null) return; meant that reaching if (!due) at line 210 implied nudge != null. The repair block preserved it with its own if (nudge == null) return;. Adding listedFingerprintMismatch as a third way past the guard breaks that: a stale listing alone now admits a tick with neither a due fire nor a nudge, and the repair block doesn't catch it because it only runs when the reloaded fingerprint mismatches.

The reachable path is the exact race the reload exists to handle — a concurrent upsertCron with a changed trigger during a rolling deploy:

1. tick() lists tasks; listed holds fingerprint F1.
2. upsertCron on another node takes the mutex and commits both the task (F2) and the state (F2, nextRunAt = now + interval).
3. tickOne acquires the mutex; the post-mutex state read returns F2, so due = false and nudge = null.
4. listedFingerprintMismatch is !F1.equals(F2) → true → past the guard.
5. Reload returns F2, which matches state.timingFingerprint() → the repair block is skipped entirely, so due is never forced and nothing returns.
6. Falls into if (!due) → materializeNudge(task) → then store.clearCronNudge(task.name(), nudge) unboxes a null Long.

Two consequences, in this order:

• A spurious recurring instance is inserted with origin=nudge, and the state is overwritten with lastRunAt=now and inFlightJobId=<spurious job> — which then blocks the next legitimate materialization via the pile-up guard until it terminates.
• An NullPointerException at line 225, swallowed by tick()'s catch (Throwable) as Recurring tick failed for task … — i.e. logged after the damage is durable, and looking like a transient blip.

Reproduced against 02f5f75 with a throwaway test (in-memory store, ForwardingJobStore returning the stale listing, same technique as materializerReloadsTheDefinitionUnderTheTaskMutexBeforeActing):

scheduler.defineIntervalTask("racy", Duration.ofMinutes(5), ...);
CronTask stale = store.findCronTask("racy").orElseThrow();
scheduler.defineIntervalTask("racy", Duration.ofMinutes(7), ...);   // task + state both commit
new RecurringMaterializer(staleListing).tick(Instant.now());

Result:

jobs=1 origins=[nudge]
state=CronTaskScheduleState[..., nextRunAt=...+7m, inFlightJobId=019ff532-…, timingFingerprint=interval:PT7M, nudgeRequestedAt=null, nudgeRevision=null]

WARN RecurringMaterializer - Recurring tick failed for task racy
java.lang.NullPointerException: Cannot invoke "java.lang.Long.longValue()" because "nudge" is null
	at RecurringMaterializer.tickOneLocked(RecurringMaterializer.java:225)

Fix — one line, immediately after the repair block closes (line 197), i.e. once the authoritative fingerprint has had its say:

// The listed mismatch was a false alarm: the listing was stale relative to
// a definition+state pair that already agree. Nothing is owed this tick.
if (!due && nudge == null) return;

It has to sit after the repair block — putting it before would re-open the dormant-schedule case this commit just fixed. The three paths that reach line 210 then all satisfy due || nudge != null again.

Worth a named regression test too, since none of the five new tests covers it — they all arrange a mismatch that the reload confirms. Something like staleListingWhoseDefinitionAlreadyAgreesWithStateMaterializesNothing.