From 2d8822c1a4406a8d39014f2f94c9bf285a04f5e6 Mon Sep 17 00:00:00 2001 From: Wanja Krah Date: Wed, 4 Nov 2026 21:54:20 +0100 Subject: [PATCH] S16 - some transactional hardening --- .../agiledancers/repository/Database.java | 8 +++ .../agiledancers/service/SchedulingRuns.java | 36 ++++++++-- .../service/SchedulingRunsTest.java | 65 +++++++++++++++++++ specs/S16-validation.md | 24 +++++++ .../S16_store-state-in-persistent-storage | 2 +- 5 files changed, 130 insertions(+), 5 deletions(-) rename specs/{in_review => implemented}/S16_store-state-in-persistent-storage (99%) diff --git a/agiledancers-be/src/main/java/nl/agilecc/agiledancers/repository/Database.java b/agiledancers-be/src/main/java/nl/agilecc/agiledancers/repository/Database.java index 8ce8149..8720acb 100644 --- a/agiledancers-be/src/main/java/nl/agilecc/agiledancers/repository/Database.java +++ b/agiledancers-be/src/main/java/nl/agilecc/agiledancers/repository/Database.java @@ -27,6 +27,12 @@ public class Database { this.manager = manager; } + /** + * Runs work in a new transaction, or joins the current JTA transaction. Callback registration is supported only + * inside transactions started by this instance. Commit callbacks run in registration order after durable commit; + * their exceptions propagate to the caller without rolling back committed work or running remaining callbacks. + * Rollback callbacks run before the original failure is rethrown; a callback exception replaces that failure. + */ public T transaction(final Callable work) throws Exception { try { if (manager.getStatus() != Status.STATUS_NO_TRANSACTION) { @@ -90,10 +96,12 @@ public class Database { } } + /** Registers a callback on this thread's transaction owned by this instance; see {@link #transaction(Callable)}. */ public void afterCommit(final Runnable action) { committed.get().add(action); } + /** Registers a rollback callback under the same ownership requirement as {@link #afterCommit(Runnable)}. */ public void afterRollback(final Runnable action) { rolledBack.get().add(action); } diff --git a/agiledancers-be/src/main/java/nl/agilecc/agiledancers/service/SchedulingRuns.java b/agiledancers-be/src/main/java/nl/agilecc/agiledancers/service/SchedulingRuns.java index 797f6f5..0bbb62b 100644 --- a/agiledancers-be/src/main/java/nl/agilecc/agiledancers/service/SchedulingRuns.java +++ b/agiledancers-be/src/main/java/nl/agilecc/agiledancers/service/SchedulingRuns.java @@ -18,6 +18,7 @@ import nl.agilecc.agiledancers.model.scheduling.SchedulingResult; import nl.agilecc.agiledancers.model.scheduling.SchedulingRun; import nl.agilecc.agiledancers.model.scheduling.SchedulingState; import nl.agilecc.agiledancers.model.scheduling.SchedulingSummary; +import nl.agilecc.agiledancers.repository.StorageException; import nl.agilecc.agiledancers.service.port.SchedulingEngine; /** Owns background tasks; the school monitor orders short commits and publication, never solver computation. */ @@ -173,15 +174,42 @@ public class SchedulingRuns { private void finish(final SchedulingRun completed) { try { - work(() -> { - save(completed); - return null; - }); + while (true) { + try { + work(() -> { + schools.show(completed.schoolId(), completed.showId()); + save(completed); + return null; + }); + return; + } catch (final StorageException unavailable) { + // Keep the run guard and terminal outcome until storage recovers; never publish an uncommitted + // status. + if (executor.isShutdown() || !waitForStorage()) { + return; + } + } catch (final ManagementException removed) { + if (removed.getKind() != ManagementExceptionKind.NOT_FOUND) { + throw removed; + } + return; + } + } } finally { schools.endScheduling(completed.showId()); } } + private boolean waitForStorage() { + try { + Thread.sleep(1000); + return true; + } catch (final InterruptedException stopping) { + Thread.currentThread().interrupt(); + return false; + } + } + private void save(final SchedulingRun run) { if (persistence == null) { publish(run); diff --git a/agiledancers-be/src/test/java/nl/agilecc/agiledancers/service/SchedulingRunsTest.java b/agiledancers-be/src/test/java/nl/agilecc/agiledancers/service/SchedulingRunsTest.java index a4d9ee1..049bfc7 100644 --- a/agiledancers-be/src/test/java/nl/agilecc/agiledancers/service/SchedulingRunsTest.java +++ b/agiledancers-be/src/test/java/nl/agilecc/agiledancers/service/SchedulingRunsTest.java @@ -136,6 +136,71 @@ class SchedulingRunsTest { schools.endScheduling(schools.beginScheduling(school, show.getId()).showId()); } + public @Test void databaseRecoveryPersistsPendingFailureWithoutRestart() throws Exception { + final Show show = show(); + final var entered = new CountDownLatch(1); + final var release = new CountDownLatch(1); + final var persistence = new OutagePersistence(); + final var runs = new SchedulingRuns(schools, blocking(entered, release)); + final java.lang.reflect.Field field = SchedulingRuns.class.getDeclaredField("persistence"); + field.setAccessible(true); + field.set(runs, persistence); + try { + final SchedulingRun started = runs.start(school, show.getId()); + assertTrue(entered.await(10, TimeUnit.SECONDS)); + persistence.available.set(false); + release.countDown(); + assertTrue(persistence.unavailable.await(10, TimeUnit.SECONDS)); + assertEquals(started, persistence.stored.get()); + assertEquals(started, runs.status(school, show.getId())); + assertThrows(ManagementException.class, () -> runs.start(school, show.getId())); + persistence.available.set(true); + final SchedulingRun completed = await(runs, show); + assertEquals(SchedulingState.SOLVER_FAILED, completed.state()); + assertEquals(started.runId(), completed.runId()); + assertEquals(completed, persistence.stored.get()); + schools.endScheduling(schools.beginScheduling(school, show.getId()).showId()); + } finally { + persistence.available.set(true); + release.countDown(); + runs.close(); + } + } + + private static class OutagePersistence extends SchedulingPersistence { + private final CountDownLatch unavailable = new CountDownLatch(2); + private final java.util.concurrent.atomic.AtomicBoolean available = + new java.util.concurrent.atomic.AtomicBoolean(true); + private final AtomicReference stored = new AtomicReference<>(); + + private OutagePersistence() { + super(null, null, null); + } + + public @Override T work(final java.util.concurrent.Callable action) { + if (!available.get()) { + unavailable.countDown(); + throw new nl.agilecc.agiledancers.repository.StorageException(new IllegalStateException("offline")); + } + try { + return action.call(); + } catch (final RuntimeException failure) { + throw failure; + } catch (final Exception failure) { + throw new IllegalStateException(failure); + } + } + + public @Override void accepted(final SchedulingRun run, final Runnable launch, final Runnable rollback) { + save(run, launch); + } + + public @Override void save(final SchedulingRun run, final Runnable publication) { + stored.set(run); + publication.run(); + } + } + private Show show() { final long group = schools.createGroup( school, new GroupDetails("Group", DanceGroupType.REGULAR, 0, 99, Set.of())) diff --git a/specs/S16-validation.md b/specs/S16-validation.md index 12eb71e..4979c80 100644 --- a/specs/S16-validation.md +++ b/specs/S16-validation.md @@ -154,3 +154,27 @@ Retained review signals in changed code: One active backend remains the supported deployment model. Global execution capacity, distributed workers, database backup/restore and recovery after database volume loss remain outside S16. S17 owns backup/restore. + +## Review follow-up: database recovery during calculation + +Terminal scheduling outcomes whose transaction fails remain pending in their worker. +The worker retries storage once per second without holding the school monitor between +attempts, retaining its run guard until the terminal status commits. Publication still +happens only after commit. Shutdown interrupts the wait; startup recovery remains +responsible for persisted Running statuses after a process restart. If the show was +removed during the outage, the pending outcome is discarded rather than recreating +its status. The finish method's cognitive complexity 12 and cyclomatic complexity 8 distinguish +retryable storage failures, removed shows and shutdown; these lifecycle decisions +stay in the run owner. SchedulingRuns fan-out is now 34 and its test fixture fan-out +35: the added storage exception, interruption handling and outage fixture belong to +this existing persistence lifecycle boundary. + +The regression test simulates unavailable storage during result persistence, verifies +that no terminal status is published prematurely and duplicate starts remain rejected, +then restores storage without restarting the backend and checks the stable run ID, +persisted failure status and released guard. Public transaction/callback documentation +now specifies transaction ownership, callback timing and exception behavior. + +Follow-up verification: clean Maven verify against PostgreSQL passes all 73 backend +tests and 19 standards-checker tests. Standards and the coverage gate pass; +backend line coverage is 1769/1841 (96.09%). diff --git a/specs/in_review/S16_store-state-in-persistent-storage b/specs/implemented/S16_store-state-in-persistent-storage similarity index 99% rename from specs/in_review/S16_store-state-in-persistent-storage rename to specs/implemented/S16_store-state-in-persistent-storage index 9b3a7e6..3fa8593 100644 --- a/specs/in_review/S16_store-state-in-persistent-storage +++ b/specs/implemented/S16_store-state-in-persistent-storage @@ -1,6 +1,6 @@ # S16 — Store state in persistent storage -Status: IN_REVIEW +Status: IMPLEMENTED Agreed after the grill-me interview; shared understanding confirmed.