one_d4: move the retention sweep to the C++ worker (#1424, #1356) - #1443
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
1d4-web | 9e3b528 | Commit Preview URL Branch Preview URL |
Aug 23 2026, 10:08 PM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
iili | 9e3b528 | Commit Preview URL Branch Preview URL |
Aug 23 2026, 10:08 PM |
79987d6 to
7af95d4
Compare
|
Review panel on 1f030ab — four lenses (correctness/control flow; SQL/resource; tests/docs/CI; altitude), each agent refuted its own findings, then I re-checked survivors against the deleted Java Headline: the move is at the right level and the policy-file / fail-closed startup story is sound. One real behavioral regression vs Java (settle+delete in one txn), plus several tests that claim to pin the new guarantees and do not. Ranked most-severe first in the inline comments. Agree fine to leave (author already disclosed): unreachable release Altitude, not blocking: |
| // The four tables the sweep touches, column for column what the migrations | ||
| // create; schema_contract_test is what keeps this copy honest. |
There was a problem hiding this comment.
Fold-in: this comment invents a gate. schema_contract_test’s data deps are pg_queue_test.cc / pg_game_sink_test.cc / … — it never reads retention_test.cc. These fixtures are minimal stubs (not migration mirrors), and the suite that now actually runs in CI can stay green against a drifted type/FK/cascade while production SQL fails.
Either add retention_test.cc to the contract’s watched sources (and make the fixtures match), or drop the claim and say what is actually pinned (own schema + the cascade the metric depends on).
| const absl::Status status = client.InTransaction([&](pg::Transaction& tx) -> absl::Status { | ||
| // Bounds every statement below. LOCAL, so it reverts with the transaction | ||
| // rather than leaking onto a pooled connection. | ||
| auto bound = tx.Exec(absl::StrCat("SET LOCAL statement_timeout = ", | ||
| absl::ToInt64Milliseconds(policy.statement_timeout))); | ||
| if (!bound.ok()) return bound.status(); | ||
|
|
||
| // The three arms run poisoned, stalled, released, and the order is load | ||
| // bearing twice over. | ||
| // | ||
| // Poisoned before stalled: a row whose attempts are spent is also unheld | ||
| // and may well be old, so it matches the stalled arm too. Both retire it; | ||
| // only one tells the user why. | ||
| // | ||
| // Stalled before released: releasing stamps updated_at, which would make | ||
| // the row look freshly touched and hide it from the staleness the stalled | ||
| // arm looks for — costing the user another whole window of silence. | ||
| auto poisoned = tx.Exec(absl::StrCat(R"( | ||
| UPDATE indexing_requests | ||
| SET status = 'FAILED', error_message = $2, dedupe_key = NULL, | ||
| updated_at = $1::timestamp, owner_id = NULL, lease_expires_at = NULL | ||
| WHERE status IN ('PENDING', 'PROCESSING') | ||
| AND attempts >= $3::int | ||
| AND )", | ||
| kUnheld, " RETURNING id"), | ||
| {at, kPoisonedMessage, attempts}); | ||
| if (!poisoned.ok()) return poisoned.status(); | ||
| report.poisoned = poisoned->rows(); | ||
|
|
||
| // The NOT EXISTS pair is what separates a backlog from an outage. Age | ||
| // alone cannot: one worker draining a deep queue leaves rows at the back | ||
| // untouched for as long as the backlog takes. So this fires only when no | ||
| // worker anywhere holds a live lease, and none has held one recently | ||
| // enough to still be working. | ||
| auto stalled = tx.Exec(absl::StrCat(R"( | ||
| UPDATE indexing_requests | ||
| SET status = 'FAILED', error_message = $2, dedupe_key = NULL, | ||
| updated_at = $1::timestamp, owner_id = NULL, lease_expires_at = NULL | ||
| WHERE status IN ('PENDING', 'PROCESSING') | ||
| AND )", | ||
| kUnheld, R"( | ||
| AND updated_at < $3::timestamp | ||
| AND NOT EXISTS ( | ||
| SELECT 1 FROM indexing_requests live | ||
| WHERE live.owner_id IS NOT NULL | ||
| AND live.lease_expires_at > $1::timestamp) | ||
| AND NOT EXISTS ( | ||
| SELECT 1 FROM indexing_requests recent | ||
| WHERE recent.id <> indexing_requests.id | ||
| AND recent.lease_expires_at >= $3::timestamp) | ||
| RETURNING id)"), | ||
| {at, kStalledMessage, stale_cutoff}); | ||
| if (!stalled.ok()) return stalled.status(); | ||
| report.stalled = stalled->rows(); | ||
|
|
||
| // Not a retirement: the owner is gone, the work is not. Another worker | ||
| // claims it next, and the user is told nothing about work about to run. | ||
| // Carries its own attempts guard, so a row at the limit cannot match here | ||
| // at all — the ordering above is not what keeps them apart. | ||
| auto released = tx.Exec(absl::StrCat(R"( | ||
| UPDATE indexing_requests | ||
| SET owner_id = NULL, updated_at = $1::timestamp | ||
| WHERE status IN ('PENDING', 'PROCESSING') | ||
| AND owner_id IS NOT NULL | ||
| AND lease_expires_at IS NOT NULL | ||
| AND lease_expires_at <= $1::timestamp | ||
| AND attempts < $2::int | ||
| RETURNING id)"), | ||
| {at, attempts}); | ||
| if (!released.ok()) return released.status(); | ||
| report.released = released->rows(); | ||
|
|
||
| // Games before requests: game_features.request_id is a foreign key onto | ||
| // indexing_requests(id), and the request delete skips any row a game still | ||
| // points at. Reversing these would not corrupt anything — it would leave a | ||
| // qualifying request for the next pass, once its games had gone. | ||
| // motif_occurrences goes with the games, by ON DELETE CASCADE. | ||
| const std::string games_cutoff = Stamp(now - policy.period); | ||
| auto games = | ||
| tx.Exec("DELETE FROM game_features WHERE indexed_at < $1::timestamp RETURNING game_url", | ||
| {games_cutoff}); | ||
| if (!games.ok()) return games.status(); | ||
| report.games_deleted = games->rows(); | ||
|
|
||
| auto periods = | ||
| tx.Exec("DELETE FROM indexed_periods WHERE fetched_at < $1::timestamp RETURNING id", | ||
| {games_cutoff}); | ||
| if (!periods.ok()) return periods.status(); | ||
| report.periods_deleted = periods->rows(); | ||
|
|
||
| auto requests = tx.Exec(R"( | ||
| DELETE FROM indexing_requests | ||
| WHERE created_at < $1::timestamp | ||
| AND status NOT IN ('PENDING', 'PROCESSING') | ||
| AND NOT EXISTS ( | ||
| SELECT 1 FROM game_features g WHERE g.request_id = indexing_requests.id) | ||
| RETURNING id)", | ||
| {Stamp(now - policy.request)}); | ||
| if (!requests.ok()) return requests.status(); | ||
| report.requests_deleted = requests->rows(); | ||
|
|
||
| return absl::OkStatus(); | ||
| }); |
There was a problem hiding this comment.
Fold-in: settle and deletes share one transaction. Java did not.
On main, RetentionWorker committed reclaimStale via inTransactionWithTimeout, then ran each deleteOlderThan separately. StatementTimeouts still documents that split: reclaim is one unit; deletes are idempotent and outside it.
Here, a game_features delete that hits statement_timeout / lock wait aborts the whole txn and undoes poisoned / stalled / released. Attempt-exhausted rows stay live with attempts >= 3, so ClaimNext will not take them — stuck until a later successful sweep (submit-path reclaim only covers the key being submitted).
Same txn also holds row locks from the release UPDATE across the deletes. Claim uses FOR UPDATE SKIP LOCKED, so those rows are invisible to other workers for the bulk of the delete phase — the opposite of “return to queue promptly.”
Commit settles, then delete (three statements, same timeout), matching the Java boundary the comments still describe.
| assertThat(new IndexerModule().retentionWindows()) | ||
| .as("the bean has to read a real window, or it initializes nothing") | ||
| .isEqualTo(Duration.ofDays(7)) | ||
| .isEqualTo(RetentionPolicy.PERIOD); |
There was a problem hiding this comment.
Fold-in: this does not prove the bean loads the policy.
A body of return Duration.ofDays(7); (no RetentionPolicy touch) still passes: both expecteds are value-equal, and RetentionPolicy.PERIOD is only evaluated as the assertion’s expected value in the test JVM. The javadoc says “a bean that returned a constant would be eager and prove nothing” — and that is exactly what survives.
Same class already uses constant-pool scans for similar traps (theModuleWiresNoExtraction…, readJdbcUrl_consultsNoFile). Pin that retentionWindows’s constant pool names RetentionPolicy / the resource, or that calling the bean is what initializes the class (e.g. clear + force after a bad classpath fixture). The fail-fast story depends on this.
| # Retention only: indexing and reanalysis run in the C++ one_d4_worker | ||
| # (#1389); this JVM's one background job is the chess-free hourly retention | ||
| # sweep. |
There was a problem hiding this comment.
Fold-in: :worker / RetentionWorker are gone; this JVM no longer owns the hourly sweep. Same stale “hourly retention tick / three settles and three deletes / re-runs in an hour” language is still in StatementTimeouts and IndexingRequestDao.reclaim — those paths are submit-path reclaim now, not that tick. Say so, or readers will wire timeouts to a loop that does not exist.
| for (std::string_view outcome : kSweepOutcomes) { | ||
| metrics_.DeclareCounter(kRetentionSweepsMetric, With("outcome", std::string(outcome))); | ||
| } | ||
| for (std::string_view table : kRetentionTables) { | ||
| metrics_.DeclareCounter(kRetentionRowsDeletedMetric, With("table", std::string(table))); | ||
| } | ||
| for (std::string_view arm : kSettleArms) { | ||
| metrics_.DeclareCounter(kRetentionRequestsSettledMetric, With("arm", std::string(arm))); | ||
| } | ||
| } | ||
|
|
||
| void WorkerMetrics::SweepFinished(std::string_view outcome, const SweepReport& report) { | ||
| metrics_.RecordCounter(kRetentionSweepsMetric, 1, With("outcome", std::string(outcome))); | ||
|
|
||
| // Zeroes are skipped rather than recorded: the series already exist from | ||
| // Declare, so adding nothing is what "nothing to delete" should look like. | ||
| const std::pair<std::string_view, int> deleted[] = { | ||
| {"game_features", report.games_deleted}, | ||
| {"indexed_periods", report.periods_deleted}, | ||
| {"indexing_requests", report.requests_deleted}, | ||
| }; | ||
| for (const auto& [table, rows] : deleted) { | ||
| if (rows > 0) { | ||
| metrics_.RecordCounter(kRetentionRowsDeletedMetric, rows, With("table", std::string(table))); | ||
| } | ||
| } | ||
|
|
||
| const std::pair<std::string_view, int> settled[] = { | ||
| {"poisoned", report.poisoned}, | ||
| {"stalled", report.stalled}, | ||
| {"released", report.released}, | ||
| }; | ||
| for (const auto& [arm, rows] : settled) { | ||
| if (rows > 0) { | ||
| metrics_.RecordCounter(kRetentionRequestsSettledMetric, rows, With("arm", std::string(arm))); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fold-in: Declare now creates the three retention series, and SweepFinished is what makes a dead sweep distinguishable from an idle one (#1356 / #1323). metrics_test.cc’s DeclaresItsSeriesBeforeAnythingHappens still only asserts index/reanalysis series; nothing in the suite records or declares retention_sweeps / retention_rows_deleted / retention_requests_settled.
Removing these loops (or breaking SweepFinished label wiring) stays green while Cleanup tiles stay empty until first event — the silent-failure mode this PR claims to close. Extend the declare test for outcomes/arms/tables, and add a SweepFinished recording pin.
| counterOver("Cleanup", "sweeps", "", | ||
| `retention_sweeps_total{service_name=~"one_d4(_worker)?",outcome="ok"}`, burstWindow), | ||
| // An alarm, not a volume: a sweep that cannot reach the database | ||
| // leaves rows uncollected and requests unsettled, and neither is | ||
| // visible in any other tile. | ||
| counterOver("Cleanup", "sweeps_failed", "", | ||
| `retention_sweeps_total{service_name=~"one_d4(_worker)?",outcome="error"}`, alarmWindow), | ||
| counterOver("Cleanup", "rows_deleted", "rows", | ||
| `retention_rows_deleted_total{service_name=~"one_d4(_worker)?"}`, burstWindow), | ||
| // Requeued work, which is ordinary — a worker died and another took | ||
| // its range. | ||
| counterOver("Cleanup", "requests_requeued", "", | ||
| `retention_requests_settled_total{service_name=~"one_d4(_worker)?",arm="released"}`, burstWindow), | ||
| // The two that end a request rather than moving it. Both mean a user | ||
| // got an answer they did not want, so both read on the alarm window: | ||
| // poisoned is a range that fails repeatedly, stalled is a fleet that | ||
| // was not running at all. | ||
| counterOver("Cleanup", "requests_poisoned", "", | ||
| `retention_requests_settled_total{service_name=~"one_d4(_worker)?",arm="poisoned"}`, alarmWindow), | ||
| counterOver("Cleanup", "requests_stalled", "", | ||
| `retention_requests_settled_total{service_name=~"one_d4(_worker)?",arm="stalled"}`, alarmWindow), |
There was a problem hiding this comment.
Fold-in: the closed name set now includes these instruments, but TestRegistry_OneD4QueriesUseTheWorkersVocabulary’s “typo selects nothing” block still only requires the index vocabulary (outcome="completed|failed|…", result="empty|cached"). It never requires outcome="ok"|"error" or arm="released"|"poisoned"|"stalled".
A selector typo on the Cleanup tiles above (arm="requeued" — the UI language here — or outcome="success") still passes CI and yields permanent zero tiles. Same class of false confidence the index-outcome pins were written for; extend that block for the retention label set.
|
Note: the summary above cites |
fde6daa to
cfde875
Compare
|
Recheck on
One new item from the split, inline below. Otherwise LGTM pending #6 + that. |
| # Retention only: indexing and reanalysis run in the C++ one_d4_worker | ||
| # (#1389); this JVM's one background job is the chess-free hourly retention | ||
| # sweep. |
There was a problem hiding this comment.
Still open from the earlier fold-in: :worker / RetentionWorker are gone. Drop or rewrite this — it still claims this JVM owns the hourly sweep.
| }); | ||
| if (!deleted.ok()) return deleted; |
There was a problem hiding this comment.
Fold-in (new, from the txn split): on delete failure this returns deleted and discards report, so worker_main does SweepFinished("error", {}) and the settles that did commit are never counted.
Before the split, empty-on-error was correct (nothing committed). Now a timeout mid-delete is exactly when reclaim mattered, and Cleanup under-counts poisoned/stalled/released while only sweeps_failed moves.
Return the populated report with a non-ok status (or record settles before attempting deletes) so outcome=error still carries the arms that stuck. AFailedDeleteKeepsTheSettlingThatAlreadyRan already sets up the case — extend it to assert the metrics side.
cfde875 to
be2e99b
Compare
|
Recheck on
All eight items from the panel + rechecks are addressed. LGTM. |
The sweep's guarantees are cross-worker facts — "no worker anywhere holds a live lease" — and this is the process that holds the leases. Settling and deleting are separate transactions. The arms must agree, but a failed delete must not roll them back: a poisoned row left below ClaimNext's attempts limit is never claimed and never answered. The windows move to retention_policy.json, which both processes read at startup. A missing or contradictory file fails the worker's start and the service's class initialization. RetentionWorker and :worker are deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ST7XLGWstRE4CfUF84ijKD
be2e99b to
9e3b528
Compare
|
Recheck on Delta is dependency hygiene only: Prior fold-ins still hold (txn split, error-path settle counts, BUILD ownership, IndexerModule/metrics/prom_proxy/schema pins). CI green. LGTM stands. |
Closes #1424. Rolls in #1356.
The hourly retention sweep moves from the Java service to the C++ worker, and the windows it runs on become a file both processes read at startup.
RetentionWorkerand the:workertarget are deleted in the same change rather than soaked alongside.Why the sweep belongs here
Its guarantees are cross-worker facts — "no worker anywhere holds a live lease" — and this is the process that holds the leases. It gets its own thread beside indexing and reanalysis, for the same reason reanalysis has one: it is a single connection doing occasional bulk deletes, and a slot waiting on a 120-second statement is a slot not indexing.
The three settle arms run poisoned → stalled → released, and the order is load-bearing twice over. A row whose attempts are spent is also unheld and probably old, so both retiring arms match it; only one tells the user why. And releasing stamps
updated_at, which would make a row look freshly touched and hide it from the staleness check for another whole window.Settling is one transaction, the deletes are another, and that boundary matters as much as the ordering. The arms have to agree, so they commit together. But a delete that trips the statement timeout or waits on a lock must not roll them back with it: a poisoned row that fails to reach
FAILEDkeepsattempts >= kMaxAttempts, whichClaimNextexcludes, so no worker will take it and the user is never told — it sits invisible until some later sweep gets all the way through. The deletes are idempotent, so their own rollback costs an hour and nothing else. Committing first also drops the releaseUPDATE's row locks before the delete phase; held across it, they would hide freshly-released rows from every other worker (ClaimNexttakes its candidateFOR UPDATE SKIP LOCKED) — the opposite of returning the work promptly. This is the boundary the Java drew, andStatementTimeoutsstill documents.The report comes back even when a delete fails, since settling already committed — reporting an empty one would under-count exactly the pass where reclaiming mattered.
Deletes run games → periods → requests so a request and its games clear in one pass.
motif_occurrencesgoes with the games by FK cascade, which is why it has no metric label.Why the windows are a file
#1424 suggested keeping them in
RetentionPolicy.javaand extending the existing cross-language check. That check was a C++ test grepping Java source text:Every value lived in three places, and the coupling was to Java's formatting rather than its values — rename the field and it breaks with nothing having changed. Moving the sweep would have taken this from two constants to six.
So
retention_policy.jsonis the source, and both readers load it at runtime rather than pinning constants to it: the worker out of its image's runfiles (viaargv[0]) before anything claims or deletes, the service off its classpath in a class initializer. Field validation is identical on both sides — present, integral, positive — and the worker additionally checks the relationships between windows and exits 1 on a violation.Two things took a second pass to get right.
canConvertToLongis a range check that accepts300.5and truncates it, so Java usesisIntegralNumber. And nothing in the Micronaut graph touchedRetentionPolicy— every reader is a method body — so a broken file would have booted a container that answers/health200 and then 500s every request;IndexerModule.retentionWindows()is a@Contextbean that forces the read at startup instead. That bean is pinned by a constant-pool scan rather than by its return value: a body ofreturn Duration.ofDays(7)initializes nothing and would satisfy any assertion comparing it againstRetentionPolicy.PERIOD, since the test JVM evaluates that expected value itself.ONE_D4_RETENTION_POLICYoverrides the path. That is a real hole in "one set of numbers" — an override is a policy no test sees, and the service has no equivalent — andworker_mainnames it as such rather than claiming otherwise. Happy to drop it if you'd rather not carry the hole; it exists only to point a worker at a different policy without a rebuild.Metrics
retention_sweeps_total{outcome},retention_rows_deleted_total{table},retention_requests_settled_total{arm}, declared at startup so rates have a baseline, plus a Cleanup tile group on the one_d4 tab. The sweep deletes in silence, so one that had stopped running looks exactly like one with nothing to do. Failures count underoutcome="error", so an unreachable database does not look idle.The tile selectors are checked against the labels the worker actually emits, in both directions. The worker emits
arm="released"; these tiles and the README call it Requeued, because that is what it means to a reader — and a selector written in the UI's language matches nothing, errors nowhere, and reads zero for as long as anyone believes it.Worth knowing before you read the diff
retention_testwas skipping in CI, and had been since it landed. Noenv_inherit = ["PG_TEST_DB_URL"]— bazel does not forward the ambient environment into the test sandbox, and CI sets the variable at job level rather than passing--test_env. All 14 sweep testsGTEST_SKIPped. I reported "13 tests / 0 skipped" for this suite earlier in this PR: that was true of my local run, which passed--test_envby hand, and not of the green CI behind it. They have now run in CI and passed.It was also breaking
pg_queue_test. It built its tables in thepublicschema, and itsgame_featuresforeign key ontoindexing_requestsmade that suite's unqualifiedDROP TABLE indexing_requestsfail outright — a real failure in a suite that changed nothing. Own schema now, aspg_game_sink_testalready had, pinned by a test.retention_test's fixture is minimal, not a mirror of the migrations. It declares the columns the sweep keys on and not the other fifteen ofgame_features, so it cannot be compared column-for-column the waypg_queue_test's is. What is pinned instead: every column the sweep names still exists, and every timestamp it compares against is still a naiveTIMESTAMP— one drifted toTIMESTAMPTZwould delete a different set of rows than these tests say it does.The release arm's
attempts <guard is unobservable, and I left it. The poisoned arm runs first with the same unheld predicate and retires those rows, so nothing reaching the release arm can be at the attempt limit. It is defence if the ordering ever changes, and it matches the Java it came from; I could not construct a reachable case to test it.deleteOlderThanon the three stores now has no Java caller. Removing it means reworking fakes across ~6 test files, and those DAO tests are what document the SQL semantics the C++ copy mirrors. Left in place with a comment — happy to strip them in a follow-up.No soak. One PR, so the Java sweep is gone on merge rather than after watching the two sets of counters agree. The deletes are idempotent and settling is settle-once, so a rollback is redeploying the previous image.
Verified
87 targets across one_d4, one_d4_worker and prom_proxy with Postgres up and 0 skips, from a cold cache. asan, ubsan and tsan all clean in CI. The worker exits 1 end to end on both a missing and a self-contradictory policy.
Mutation-checked: the loader invariants and both boundary directions, the poller wiring, the schema isolation, the packaged path, the settle/delete boundary, the partial report on a failed delete, the metrics declare loops, the Cleanup label vocabulary, and the startup bean. Several started as survivors and now have tests — a non-object document that failed for the wrong reason, an absent key that fell back silently, the recent-lease
NOT EXISTScomparison, and the eager-bean assertion that a constant return would have satisfied.:pollerand:workerreach neither nlohmann/json nor libpq:PollerOptionsFromlives in its own:poller_optionsandSweepReportin a dependency-free header, so the policy loader and the Postgres client stay out of the compile of everything that polls a queue or records a counter.Reviewed by panel and by cursor[bot]; every finding from both is addressed in the diff.
🤖 Generated with Claude Code
https://claude.ai/code/session_01ST7XLGWstRE4CfUF84ijKD