From 0cfe46e039bd27ab869a76a0b44eb23726147365 Mon Sep 17 00:00:00 2001 From: Andy Aylward Date: Mon, 14 Sep 2026 10:51:54 -0400 Subject: [PATCH 1/3] Refuse a blank owner, and assemble the claim where the owner lives Both queues refuse an empty owner with InvalidArgument before touching the row, and V004 adds CHECK (owner_id <> '') to both request tables, nulling existing blanks on the deploy that adds it. A blank is an owner no fence matches, so a row claimed under one neither completes nor retires. PlatformAdmission::Claim returns the job and its slot; ClaimOne moves the owner into the Claim after the queue's last read of it. --- domains/games/apis/one_d4/BUILD.bazel | 2 + .../migrations/V004__owner_never_blank.sql | 29 ++++ .../games/apis/one_d4/migrations/manifest.txt | 1 + .../db/OwnerNeverBlankConstraintTest.java | 161 ++++++++++++++++++ domains/games/apis/one_d4_worker/BUILD.bazel | 6 + domains/games/apis/one_d4_worker/pg_queue.cc | 6 + .../games/apis/one_d4_worker/pg_queue_test.cc | 62 +++++++ domains/games/apis/one_d4_worker/poller.cc | 33 ++-- domains/games/apis/one_d4_worker/poller.h | 18 +- .../apis/one_d4_worker/reanalysis_queue.cc | 3 + .../one_d4_worker/reanalysis_queue_test.cc | 12 ++ 11 files changed, 316 insertions(+), 17 deletions(-) create mode 100644 domains/games/apis/one_d4/migrations/V004__owner_never_blank.sql create mode 100644 domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/OwnerNeverBlankConstraintTest.java diff --git a/domains/games/apis/one_d4/BUILD.bazel b/domains/games/apis/one_d4/BUILD.bazel index 4cf4e6a83..c5e570f89 100644 --- a/domains/games/apis/one_d4/BUILD.bazel +++ b/domains/games/apis/one_d4/BUILD.bazel @@ -68,6 +68,7 @@ filegroup( "migrations/V001__initial_schema.sql", "migrations/V002__player_titles.sql", "migrations/V003__platform_canonical.sql", + "migrations/V004__owner_never_blank.sql", ], ) @@ -559,6 +560,7 @@ java_test_suite( "src/test/java/com/muchq/games/one_d4/db/MigrationFilesTest.java", "src/test/java/com/muchq/games/one_d4/db/MigrationRunnerTest.java", "src/test/java/com/muchq/games/one_d4/db/MigrationTest.java", + "src/test/java/com/muchq/games/one_d4/db/OwnerNeverBlankConstraintTest.java", "src/test/java/com/muchq/games/one_d4/db/PgTestUrlsTest.java", "src/test/java/com/muchq/games/one_d4/db/PlatformCanonicalConstraintTest.java", "src/test/java/com/muchq/games/one_d4/db/ReanalysisRequestDaoTest.java", diff --git a/domains/games/apis/one_d4/migrations/V004__owner_never_blank.sql b/domains/games/apis/one_d4/migrations/V004__owner_never_blank.sql new file mode 100644 index 000000000..25406044b --- /dev/null +++ b/domains/games/apis/one_d4/migrations/V004__owner_never_blank.sql @@ -0,0 +1,29 @@ +-- owner_id names the claimant every fence keys on: a heartbeat, a progress +-- write and a terminal write all match on it, and a claim spends an attempt +-- only when the owner it presents differs from the one stored. A blank is a +-- claimant nobody's fence matches and one a re-claim under a blank spends +-- nothing on — so it is refused on both request tables, whoever the writer +-- is. NULL stays what it is: no owner. +-- +-- Blanks are nulled on the one execution that adds the constraint, so a +-- database holding one converges rather than failing the deploy. A nulled +-- owner with a lapsed lease is exactly what ClaimNext takes next. + +DO $$ +DECLARE + target text; + constraint_name text; +BEGIN + FOREACH target IN ARRAY ARRAY['indexing_requests', 'reanalysis_requests'] + LOOP + constraint_name := target || '_owner_never_blank'; + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conrelid = target::regclass AND conname = constraint_name + ) THEN + EXECUTE format('UPDATE %I SET owner_id = NULL WHERE owner_id = ''''', target); + EXECUTE format('ALTER TABLE %I ADD CONSTRAINT %I CHECK (owner_id <> '''')', + target, constraint_name); + END IF; + END LOOP; +END $$; diff --git a/domains/games/apis/one_d4/migrations/manifest.txt b/domains/games/apis/one_d4/migrations/manifest.txt index 09147839d..57f231ec4 100644 --- a/domains/games/apis/one_d4/migrations/manifest.txt +++ b/domains/games/apis/one_d4/migrations/manifest.txt @@ -7,3 +7,4 @@ V001__initial_schema V002__player_titles V003__platform_canonical +V004__owner_never_blank diff --git a/domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/OwnerNeverBlankConstraintTest.java b/domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/OwnerNeverBlankConstraintTest.java new file mode 100644 index 000000000..3ca55d7b4 --- /dev/null +++ b/domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/OwnerNeverBlankConstraintTest.java @@ -0,0 +1,161 @@ +package com.muchq.games.one_d4.db; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.sql.Connection; +import java.sql.SQLException; +import java.util.List; +import java.util.UUID; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * A blank owner is refused by the request tables, whoever the writer is. + * + *

Every fence keys on {@code owner_id}: a heartbeat, a progress write and a terminal write all + * match on it, and a claim spends an attempt only when the owner it presents differs from the one + * stored. A row claimed under {@code ''} is one no worker's fence matches and one a re-claim under + * {@code ''} does — so it never completes and never retires. {@code NULL} stays what it is: no + * owner. + */ +public class OwnerNeverBlankConstraintTest { + + private static final List REQUEST_TABLES = + List.of("indexing_requests", "reanalysis_requests"); + + private TestDb testDb; + + @BeforeEach + public void setUp() { + testDb = TestDb.create("ownerneverblank"); + } + + @Test + public void everyRequestTableRefusesABlankOwner() throws Exception { + for (String table : REQUEST_TABLES) { + UUID id = insertRequest(table); + assertThatThrownBy(() -> setOwner(table, id, "")) + .as("%s should refuse a blank owner", table) + .isInstanceOf(SQLException.class) + .hasMessageContaining("owner_never_blank"); + } + } + + /** The control: no owner and a named owner are both still storable. */ + @Test + public void aNullOrNamedOwnerIsStillAccepted() throws Exception { + for (String table : REQUEST_TABLES) { + UUID id = insertRequest(table); + assertThatCode(() -> setOwner(table, id, "cpp/host/1/9a1f")) + .as("%s should accept a named owner", table) + .doesNotThrowAnyException(); + assertThatCode(() -> setOwner(table, id, null)) + .as("%s should accept no owner", table) + .doesNotThrowAnyException(); + } + } + + /** The constraints are named, so {@code Migration.verify()} notices a database missing them. */ + @Test + public void theConstraintsAreVisibleToBootVerification() { + assertThat(constraintNames()) + .containsExactlyInAnyOrder( + "indexing_requests_owner_never_blank", "reanalysis_requests_owner_never_blank"); + } + + /** + * The normalise half. A database holding a blank before the constraint converges instead of + * failing the deploy: the step nulls blanks only on the execution that adds the constraint. + */ + @Test + public void rerunningTheMigrationClearsABlankOwnerStoredBeforeTheConstraint() throws Exception { + exec("ALTER TABLE indexing_requests DROP CONSTRAINT indexing_requests_owner_never_blank"); + UUID id = insertRequest("indexing_requests"); + setOwner("indexing_requests", id, ""); + + new Migration(testDb.dataSource()).run(); + + assertThat(ownerOf("indexing_requests", id)).isNull(); + assertThat(constraintNames()).contains("indexing_requests_owner_never_blank"); + } + + /** Re-run safe, like every step here: the file executes on every deploy. */ + @Test + public void theStepIsIdempotent() { + new Migration(testDb.dataSource()).run(); + new Migration(testDb.dataSource()).run(); + + assertThat(constraintNames()).hasSize(REQUEST_TABLES.size()); + } + + /** A pending row on `table`. At most one reanalysis row per test: its live index allows one. */ + private UUID insertRequest(String table) throws SQLException { + UUID id = UUID.randomUUID(); + String sql = + switch (table) { + case "indexing_requests" -> + "INSERT INTO indexing_requests (id, player, platform, start_month, end_month)" + + " VALUES (?, ?, 'CHESS_COM', '2024-01', '2024-01')"; + case "reanalysis_requests" -> "INSERT INTO reanalysis_requests (id) VALUES (?)"; + default -> throw new IllegalArgumentException(table); + }; + try (Connection conn = testDb.dataSource().getConnection(); + var stmt = conn.prepareStatement(sql)) { + stmt.setObject(1, id); + if (table.equals("indexing_requests")) { + stmt.setString(2, UUID.randomUUID().toString()); + } + stmt.executeUpdate(); + } + return id; + } + + private void setOwner(String table, UUID id, String owner) throws SQLException { + try (Connection conn = testDb.dataSource().getConnection(); + var stmt = conn.prepareStatement("UPDATE " + table + " SET owner_id = ? WHERE id = ?")) { + stmt.setString(1, owner); + stmt.setObject(2, id); + stmt.executeUpdate(); + } + } + + private String ownerOf(String table, UUID id) throws SQLException { + try (Connection conn = testDb.dataSource().getConnection(); + var stmt = conn.prepareStatement("SELECT owner_id FROM " + table + " WHERE id = ?")) { + stmt.setObject(1, id); + try (var rs = stmt.executeQuery()) { + assertThat(rs.next()).as("row %s should still exist", id).isTrue(); + return rs.getString(1); + } + } + } + + private void exec(String sql) throws SQLException { + try (Connection conn = testDb.dataSource().getConnection(); + var stmt = conn.createStatement()) { + stmt.execute(sql); + } + } + + private List constraintNames() { + try (Connection conn = testDb.dataSource().getConnection(); + var stmt = + conn.prepareStatement( + "SELECT conname FROM pg_constraint c JOIN pg_namespace n ON n.oid =" + + " c.connamespace WHERE n.nspname = ? AND conname LIKE" + + " '%owner_never_blank'")) { + stmt.setString(1, testDb.schema()); + var names = new java.util.ArrayList(); + try (var rs = stmt.executeQuery()) { + while (rs.next()) { + names.add(rs.getString(1)); + } + } + return names; + } catch (SQLException e) { + throw new RuntimeException(e); + } + } +} diff --git a/domains/games/apis/one_d4_worker/BUILD.bazel b/domains/games/apis/one_d4_worker/BUILD.bazel index a9ee6aebd..1b116316e 100644 --- a/domains/games/apis/one_d4_worker/BUILD.bazel +++ b/domains/games/apis/one_d4_worker/BUILD.bazel @@ -281,10 +281,16 @@ cc_test( ], tags = ["requires-postgres"], deps = [ + ":index_pool", + ":metrics", ":migration_files", ":pg_queue", ":pg_test_db", + ":poller", + "//domains/platform/libs/futility/otel:capturing_metrics_recorder", "//domains/platform/libs/pg", + "@com_google_absl//absl/status", + "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_absl//absl/time", "@googletest//:gtest_main", diff --git a/domains/games/apis/one_d4_worker/pg_queue.cc b/domains/games/apis/one_d4_worker/pg_queue.cc index efa7d8987..348eb60c2 100644 --- a/domains/games/apis/one_d4_worker/pg_queue.cc +++ b/domains/games/apis/one_d4_worker/pg_queue.cc @@ -50,6 +50,12 @@ std::string TextArray(absl::Span values) { absl::StatusOr> PgQueue::ClaimNext( std::string_view owner, absl::Duration lease, absl::Span at_capacity) { + // Every write after this one is fenced on the owner, and a re-claim + // under the id already on the row spends no attempt. A blank would be + // an owner nobody's fence matches and nobody's re-claim spends — so it + // is refused here, before the row is touched, and the schema refuses it + // too (V004). + if (owner.empty()) return absl::InvalidArgumentError("a claim needs an owner to fence on"); // One conditional UPDATE, so two workers racing for the same row cannot // both win: the row lock decides, and the loser's WHERE no longer // matches. FOR UPDATE SKIP LOCKED picks the candidate without the two of diff --git a/domains/games/apis/one_d4_worker/pg_queue_test.cc b/domains/games/apis/one_d4_worker/pg_queue_test.cc index 090c99cbc..579cb76ac 100644 --- a/domains/games/apis/one_d4_worker/pg_queue_test.cc +++ b/domains/games/apis/one_d4_worker/pg_queue_test.cc @@ -3,6 +3,7 @@ #include #include +#include #include #include @@ -10,8 +11,13 @@ #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" +#include "absl/time/clock.h" +#include "domains/games/apis/one_d4_worker/index_pool.h" +#include "domains/games/apis/one_d4_worker/metrics.h" #include "domains/games/apis/one_d4_worker/migration_files.h" #include "domains/games/apis/one_d4_worker/pg_test_db.h" +#include "domains/games/apis/one_d4_worker/poller.h" +#include "domains/platform/libs/futility/otel/capturing_metrics_recorder.h" #include "domains/platform/libs/pg/pg.h" namespace one_d4_worker { @@ -422,5 +428,61 @@ TEST_F(PgQueueTest, ProgressIsRefusedOnceTheLeaseHasExpired) { EXPECT_EQ(Column(Id(1), "games_indexed"), "0"); } +// The production wiring over the production statements: a pool of owned +// connections, the platform gate in front of the claim, a heartbeat and a +// progress report inside the run, and the terminal write after it. Every one +// of those is fenced on the id the row was claimed under, so this is where a +// claim made under any other id shows up — as a lease nobody took. +TEST_F(PgQueueTest, ARunTakenThroughTheGateIsFencedAllTheWayToCompleted) { + InsertOn(Id(1), "alireza", "LICHESS"); + + std::atomic kept{false}; + std::atomic reported{false}; + std::atomic runs{0}; + const Poller::Run run = [&](const Claim&, LeaseKeeper& lease) -> absl::StatusOr { + kept = lease.Keep(); + reported = lease.Report(7); + ++runs; + RunReport report; + report.games_indexed = 7; + return report; + }; + + PlatformAdmission admission({{"LICHESS", 1}}); + Poller::Options poller; + poller.owner = "cpp/test/1"; + poller.lease = absl::Minutes(5); + poller.admission = &admission; + futility::otel::CapturingMetricsRecorder recorder; + WorkerMetrics metrics(recorder); + IndexPool::Options pool_options; + pool_options.slots = 2; + pool_options.idle_wait = absl::Milliseconds(10); + IndexPool pool([this] { return NewOwnedPgQueue(conninfo_, kMaxAttempts); }, run, poller, metrics, + pool_options); + + pool.Run([&] { return runs.load() >= 1; }, [](absl::Duration wait) { absl::SleepFor(wait); }); + + EXPECT_TRUE(kept.load()) << "the heartbeat did not match the row the claim wrote"; + EXPECT_TRUE(reported.load()); + EXPECT_EQ(Column(Id(1), "status"), "COMPLETED"); + EXPECT_EQ(Column(Id(1), "games_indexed"), "7"); + EXPECT_EQ(Column(Id(1), "owner_id"), "(null)"); + EXPECT_EQ(Column(Id(1), "attempts"), "1"); +} + +// An owner is what every later write is fenced on, so a blank one is refused +// before the row is touched rather than written and then matched by nobody. +TEST_F(PgQueueTest, RefusesToClaimUnderABlankOwner) { + Insert(Id(1), "hikaru"); + + const auto claimed = queue_->ClaimNext("", absl::Minutes(5)); + + EXPECT_EQ(claimed.status().code(), absl::StatusCode::kInvalidArgument) << claimed.status(); + EXPECT_EQ(Column(Id(1), "status"), "PENDING"); + EXPECT_EQ(Column(Id(1), "owner_id"), "(null)"); + EXPECT_EQ(Column(Id(1), "attempts"), "0"); +} + } // namespace } // namespace one_d4_worker diff --git a/domains/games/apis/one_d4_worker/poller.cc b/domains/games/apis/one_d4_worker/poller.cc index 1767f9e7d..f5a55bbf6 100644 --- a/domains/games/apis/one_d4_worker/poller.cc +++ b/domains/games/apis/one_d4_worker/poller.cc @@ -121,9 +121,9 @@ std::string_view ToString(RunOutcome outcome) { Poller::Poller(IndexQueue& queue, Run run, Options options) : queue_(queue), run_(std::move(run)), options_(std::move(options)) {} -absl::StatusOr> PlatformAdmission::Claim( - absl::FunctionRef>(absl::Span)> claim, - std::string_view owner) { +absl::StatusOr> PlatformAdmission::Claim( + absl::FunctionRef>(absl::Span)> + claim) { const absl::MutexLock lock(mu_); std::vector at_capacity; for (const auto& [platform, limit] : limits_) { @@ -143,8 +143,7 @@ absl::StatusOr> PlatformAdmission::Claim( // Owns nothing; the deleter is the whole point. It runs when the last // copy of the claim is destroyed, wherever that happens to be. PlatformSlot slot(nullptr, [this, platform](void*) { Release(platform); }); - return one_d4_worker::Claim{ - .job = **claimed, .owner = std::string(owner), .slot = std::move(slot)}; + return Admitted{.job = **claimed, .slot = std::move(slot)}; } void PlatformAdmission::Release(const std::string& platform) { @@ -160,12 +159,24 @@ absl::StatusOr> Poller::ClaimOne() { const auto take = [&](absl::Span at_capacity) { return queue_.ClaimNext(owner, options_.lease, at_capacity); }; - if (options_.admission != nullptr) return options_.admission->Claim(take, owner); - - const absl::StatusOr> claimed = take({}); - if (!claimed.ok()) return claimed.status(); - if (!claimed->has_value()) return std::nullopt; - return Claim{.job = **claimed, .owner = std::move(owner), .slot = nullptr}; + // Either way the queue reads `owner` in place and it is moved once, here, + // after the last read — the Claim is assembled by the one function that + // holds the string. + absl::StatusOr> admitted; + if (options_.admission != nullptr) { + admitted = options_.admission->Claim(take); + } else { + absl::StatusOr> claimed = take({}); + if (!claimed.ok()) return claimed.status(); + admitted = claimed->has_value() + ? std::optional(Admitted{.job = std::move(**claimed), .slot = nullptr}) + : std::nullopt; + } + if (!admitted.ok()) return admitted.status(); + if (!admitted->has_value()) return std::nullopt; + return Claim{.job = std::move((*admitted)->job), + .owner = std::move(owner), + .slot = std::move((*admitted)->slot)}; } absl::StatusOr Poller::RunClaimed(const Claim& claim) { diff --git a/domains/games/apis/one_d4_worker/poller.h b/domains/games/apis/one_d4_worker/poller.h index 08a9db58a..49ab90447 100644 --- a/domains/games/apis/one_d4_worker/poller.h +++ b/domains/games/apis/one_d4_worker/poller.h @@ -78,6 +78,13 @@ class PlatformAdmission; /// one until the process restarted. using PlatformSlot = std::shared_ptr; +/// What the gate let through: a job, and the place it holds in its +/// platform's cap. +struct Admitted { + IndexJob job; + PlatformSlot slot; +}; + /// A claim, and the id it is fenced on. struct Claim { IndexJob job; @@ -127,13 +134,12 @@ class PlatformAdmission { /// every other slot, and every slot finishing a run, for up to the /// statement timeout. Accepted because the alternative races the cap. /// - /// `owner` is the id `claim` must claim the row under. A view and not a - /// value, so a `std::move` at the call site cannot empty the string - /// `claim` reads. - absl::StatusOr> Claim( + /// Returns the job and the place it holds, not a Claim: the owner is the + /// caller's, minted before the claim and moved into the Claim after it, + /// and nothing here ever needs to see it. + absl::StatusOr> Claim( absl::FunctionRef>(absl::Span)> - claim, - std::string_view owner); + claim); /// Gives a finished run's place back. Called by the slot the claim /// carries rather than by hand. diff --git a/domains/games/apis/one_d4_worker/reanalysis_queue.cc b/domains/games/apis/one_d4_worker/reanalysis_queue.cc index 53d58c3e6..9725a0242 100644 --- a/domains/games/apis/one_d4_worker/reanalysis_queue.cc +++ b/domains/games/apis/one_d4_worker/reanalysis_queue.cc @@ -30,6 +30,9 @@ int ToInt(const std::optional& value) { absl::StatusOr> PgReanalysisQueue::ClaimNext(std::string_view owner, absl::Duration lease) { + // Same refusal PgQueue makes, for the same reason: every fence keys on + // the owner, and a blank is one nobody's matches. + if (owner.empty()) return absl::InvalidArgumentError("a claim needs an owner to fence on"); // Retire what the budget has exhausted before looking for work. Merely // not claiming it is not enough: a row left PROCESSING holds the // single-live slot forever — never claimable, never history, and every diff --git a/domains/games/apis/one_d4_worker/reanalysis_queue_test.cc b/domains/games/apis/one_d4_worker/reanalysis_queue_test.cc index a125a286d..58f98f2aa 100644 --- a/domains/games/apis/one_d4_worker/reanalysis_queue_test.cc +++ b/domains/games/apis/one_d4_worker/reanalysis_queue_test.cc @@ -357,5 +357,17 @@ TEST_F(ReanalysisQueueTest, AnOwnedQueueFailsUnderTheRightOwner) { EXPECT_EQ(Column(id, "error_message"), "pg went away"); } +// The same refusal PgQueue makes, for the same reason: a blank is an owner no +// heartbeat matches, and a re-claim under it spends no attempt. +TEST_F(ReanalysisQueueTest, RefusesToClaimUnderABlankOwner) { + const std::string id = Enqueue(); + + const auto claimed = queue_->ClaimNext("", absl::Minutes(5)); + + EXPECT_EQ(claimed.status().code(), absl::StatusCode::kInvalidArgument) << claimed.status(); + EXPECT_EQ(Column(id, "status"), "PENDING"); + EXPECT_EQ(Column(id, "attempts"), "0"); +} + } // namespace } // namespace one_d4_worker From a6b51cd3cd20fe09146e5d47f2b9b4984d9ccb05 Mon Sep 17 00:00:00 2001 From: Andy Aylward Date: Mon, 14 Sep 2026 11:01:38 -0400 Subject: [PATCH 2/3] Bound the pool e2e, and note the V004 check in the design doc A run that never completes now fails the pool test at its assertion inside ten seconds rather than as a timeout on the whole target. --- .../src/main/java/com/muchq/games/one_d4/docs/DESIGN.md | 2 +- domains/games/apis/one_d4_worker/pg_queue_test.cc | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/docs/DESIGN.md b/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/docs/DESIGN.md index d3918cb22..7d1bb1d92 100644 --- a/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/docs/DESIGN.md +++ b/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/docs/DESIGN.md @@ -165,7 +165,7 @@ com.muchq.indexer/ | error_message | TEXT | Populated on FAILED | | games_indexed | INT | Running count during processing| | exclude_bullet | BOOLEAN | Part of the live-request key | -| owner_id | VARCHAR(128) | Who holds the lease; the fencing token every write is conditioned on. The Java worker claims as a process, the C++ worker mints one per run so several of its runs can be in flight at once | +| owner_id | VARCHAR(128) | Who holds the lease; the fencing token every write is conditioned on, NULL or never blank (V004 CHECK). The Java worker claims as a process, the C++ worker mints one per run so several of its runs can be in flight at once | | lease_expires_at | TIMESTAMP | Renewed every 75s while the owner is alive. Past this the request is reclaimable. Deliberately survives a terminal write, as the record of when a worker last held the row | | skip_cache | BOOLEAN | Persisted so a worker on any instance honours what the submitter asked for | | attempts | INT | Claims so far. Bounds the requeue loop for a request that keeps killing its worker | diff --git a/domains/games/apis/one_d4_worker/pg_queue_test.cc b/domains/games/apis/one_d4_worker/pg_queue_test.cc index 579cb76ac..1d5d1ebdb 100644 --- a/domains/games/apis/one_d4_worker/pg_queue_test.cc +++ b/domains/games/apis/one_d4_worker/pg_queue_test.cc @@ -461,8 +461,13 @@ TEST_F(PgQueueTest, ARunTakenThroughTheGateIsFencedAllTheWayToCompleted) { IndexPool pool([this] { return NewOwnedPgQueue(conninfo_, kMaxAttempts); }, run, poller, metrics, pool_options); - pool.Run([&] { return runs.load() >= 1; }, [](absl::Duration wait) { absl::SleepFor(wait); }); + // Bounded, so a run that never completes fails here rather than as a + // timeout on the whole target with the reason buried in the log. + const absl::Time deadline = absl::Now() + absl::Seconds(10); + pool.Run([&] { return runs.load() >= 1 || absl::Now() > deadline; }, + [](absl::Duration wait) { absl::SleepFor(wait); }); + ASSERT_EQ(runs.load(), 1) << "no run completed before the deadline"; EXPECT_TRUE(kept.load()) << "the heartbeat did not match the row the claim wrote"; EXPECT_TRUE(reported.load()); EXPECT_EQ(Column(Id(1), "status"), "COMPLETED"); From 111139f6d4ebc82afd86b038c47807545b8b4207 Mon Sep 17 00:00:00 2001 From: Andy Aylward Date: Mon, 14 Sep 2026 13:22:25 -0400 Subject: [PATCH 3/3] Heal test covers both request tables --- .../one_d4/db/OwnerNeverBlankConstraintTest.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/OwnerNeverBlankConstraintTest.java b/domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/OwnerNeverBlankConstraintTest.java index 3ca55d7b4..104d3a1d0 100644 --- a/domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/OwnerNeverBlankConstraintTest.java +++ b/domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/OwnerNeverBlankConstraintTest.java @@ -71,14 +71,16 @@ public void theConstraintsAreVisibleToBootVerification() { */ @Test public void rerunningTheMigrationClearsABlankOwnerStoredBeforeTheConstraint() throws Exception { - exec("ALTER TABLE indexing_requests DROP CONSTRAINT indexing_requests_owner_never_blank"); - UUID id = insertRequest("indexing_requests"); - setOwner("indexing_requests", id, ""); + for (String table : REQUEST_TABLES) { + exec("ALTER TABLE " + table + " DROP CONSTRAINT " + table + "_owner_never_blank"); + UUID id = insertRequest(table); + setOwner(table, id, ""); - new Migration(testDb.dataSource()).run(); + new Migration(testDb.dataSource()).run(); - assertThat(ownerOf("indexing_requests", id)).isNull(); - assertThat(constraintNames()).contains("indexing_requests_owner_never_blank"); + assertThat(ownerOf(table, id)).as("%s blank owner after re-run", table).isNull(); + assertThat(constraintNames()).contains(table + "_owner_never_blank"); + } } /** Re-run safe, like every step here: the file executes on every deploy. */