Skip to content

Two gate flakes: a whole-VM log assertion, and a plan pinned to one of two usable indexes - #932

Merged
mkreyman merged 5 commits into
masterfrom
fix/gate-flakes
Sep 28, 2026
Merged

mkreyman merged 5 commits into
masterfrom
fix/gate-flakes

Conversation

@mkreyman

@mkreyman mkreyman commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

Three different tests failed the commit gate once each on 2026-09-28, and each passed alone. This PR fixes the two whose cause is confirmed. The third is recorded here.

session_end_test (fixed):

  • The test asserted that capture_log output was "". But capture_log collects every process's logs.
  • refusal_test deliberately walks every advance error through the runner channel's logging catch-all. One of those :actor_lineage_required error lines landed inside this async test's capture window.
  • It now refutes only its own call's line, found by its unique tenant_id.

revoke_expired_dispatches_worker_test (fixed):

  • The test pinned the sweep's plan to dispatches_expires_at_active_index. dispatches_tenant_id_expires_at_index serves the same expires_at < condition, and the planner's choice between the two moves with the rows other tests leave behind.
  • The plan assertion now requires an index scan carrying the expires_at condition, and no Seq Scan.
  • That the partial index matches the sweep's predicate is asserted from its definition in pg_indexes.
  • I did not hide the other index, because dropping it inside a savepoint takes an exclusive lock on dispatches and would wait on every async test's open transaction.

signup_test "deletes the external secret when a later Multi step fails" (not fixed, cause unconfirmed):

  • It failed with 57014 canceling statement due to user request instead of StaleEntryError.
  • Its mock runs DELETE FROM tenants, which must check all 83 foreign keys that reference tenants. All 83 are indexed.
  • Leading hypothesis: a lock wait. An async test running DDL inside its sandbox transaction holds an exclusive lock on one of those child tables until it finishes, so the delete's FK check waits past the 15 s client timeout. Ten async test files run DDL.
  • What it would take: reproduce it under a full-suite run with log_lock_waits on, to name the holder. Then either make that test synchronous, or make this test fail its Multi without deleting the parent row.
  • What it blocks: nothing on its own. It fails a gate run at random, and a retry passes.

Evidence:

  • Mutations (bin/mutate.sh):
    • The :busy clause removed so the call logs: exit 0, caught.
    • The definition read from the other index: exit 0, caught.
    • Removing SET LOCAL enable_seqscan = off survives (exit 1). The shared test tables now hold enough rows that the planner uses an index unprompted, so that pre-existing refute is not load-bearing today.
  • Gate: 12002 tests, 0 failures.

Review round 1 found 9 issues. 8 are fixed and 1 is not taken.

Fixed:

  • Coverage restored: the plan test had stopped proving anything about the partial index. It now reads the partial index from pg_index: it must be valid and ready, keyed on expires_at, with the predicate (revoked_at IS NULL). An interrupted CONCURRENTLY build leaves the same definition text on an invalid index.
  • Plan assertion: the plan must use one of the two indexes that can serve the sweep, named explicitly, and never a Seq Scan.
  • Stale descriptions corrected: the moduledoc, the test name, and AC-32.1.2's wording now describe what is asserted. So does the migration comment, which had said the composite index "cannot" be used when the planner reads it whole on a small table.
  • session_end_test: the sibling test anchors on its own tenant_id and pins the log format that the :busy refutation depends on.
  • Regex: the redundant alternative is removed.

Not taken: moving the planner-choice check into an async: false scale test. This test now proves eligibility deterministically, and the migration's verification note records which index the planner picks at scale.

Mutations (mutate.sh, all exit 0):

  • pg_index read from the other index;
  • the allowed index names replaced;
  • the :busy clause removed;
  • the log's tenant_id= reworded.

Review round 2 found 9 issues, all fixed.

  • EXPLAIN check removed: it passed whenever either index existed, so it proved nothing about the partial index.
  • Tied to the real query: the worker's sweep query is now RevokeExpiredDispatchesWorker.expired_query/1. The test asserts that query's SQL carries revoked_at IS NULL and expires_at <.
  • Catalog check tightened: from pg_index, the index must be valid, have one key column expires_at, be a btree on dispatches, and have the predicate (revoked_at IS NULL).
  • :busy test: it refutes its own story and dispatch ids instead of depending on one log format.
  • Stale claims corrected: the migration comment, the story rationale and AC-32.1.2 no longer claim the composite index cannot be used. The AC now names the ~20k-seeded-row capture, not production.

Mutations (mutate.sh, all exit 0):

  • revoked_at IS NULL dropped from the worker's query
  • its < widened to <=
  • pg_index read from the other index
  • the :busy clause removed
  • the permanent log's tenant_id= reworded

Review round 3 found 7 issues and no production bug. All 7 are fixed by rewriting the two tests, not by patching them.

  • Plan asserted again, deterministically: the new RevokeExpiredDispatchesPlanTest (async: false, so it runs after every async module) seeds a production-shaped table (20k revoked history, 50 live) and ANALYZEs it inside its sandbox transaction. It captures the query perform/1 actually issues and requires the default planner to use dispatches_expires_at_active_index.
  • The seed ratio is load-bearing: at a 10% live fraction the planner multiplies the two predicates as if independent, estimates ~1,800 rows and seq-scans. The mutation raising the live fraction to 10% turns this test red.
  • Worker unchanged from master: the public expired_query/1 is gone, and so is the SQL substring check that not/or mutations defeated.
  • session_end: each test reads only the error lines carrying a per-test request_id its own process sets, with no dependence on the level token. The busy test logs a canary in the same capture and requires it to be the only line.
  • Docs: AC-32.1.2 is back to its EXPLAIN wording and names the test. The migration comment no longer holds a stale copy of the query.

Mutations (mutate.sh, all exit 0):

  • worker predicate made non-implying (coalesce)
  • index name swapped
  • live fraction raised to 10%
  • busy clause logging an error
  • request_id marker unset
  • permanent refusal logged at warning

Gate: 12003 tests, 0 failures.

The review of that rewrite found 10 issues, and they share one cause: it asserted the planner's choice inside the shared test database. ANALYZE writes pg_class.reltuples in place, so the statistics it set outlived the sandbox rollback and skewed later plans. That is the same flake class this PR removes. The rewrite is withdrawn:

  • Plan-choice test removed. The natural-plan guard for AC-32.1.2 comes next as its own PR, running in the scale job on its own database. Issues 1, 2, 6 and 10 were about that test.
  • Log tests moved to a sync module. budget_escalation_refused/4's tests are now BudgetEscalationRefusedTest (async: false). Sync modules run alone, after every async module, so the whole-capture log == "" belongs to that one call again. This removes the marker helper and, with it, issues 3, 4 and 9.
  • Stale claims corrected (issues 7 and 8): the migration comment and the epic 35 README now say which index the planner picks is not asserted in the suite.
  • Not taken (issue 5): a shared helper for the other negative capture_log assertions across test/. Converting them is a sweep over files this PR doesn't touch, and nothing is blocked on it. The sync-module pattern here is the template for doing it.

Mutations (mutate.sh, all exit 0):

  • the busy clause logging an error
  • the permanent refusal logged at warning
  • the catalog read pointed at the composite index

… of two usable indexes

session_end_test asserted capture_log's output was empty. capture_log collects every
process's logs, so an async neighbour's error line (refusal_test walks every advance
error through the channel's logging catch-all) failed it at random. It now refutes only
this call's own line, found by its unique tenant_id.

revoke_expired_dispatches_worker_test asserted the sweep plans on
dispatches_expires_at_active_index. dispatches_tenant_id_expires_at_index serves the
same expires_at condition, and the planner's pick between them moves with the rows
other tests leave behind. The plan assertion now requires an index scan with the
expires_at condition and no Seq Scan, and the partial index's match to the sweep's
predicate is asserted from its definition in pg_indexes.

Mutations (bin/mutate.sh): the busy clause removed so the call logs (exit 0, caught);
the definition read from the other index (exit 0, caught). Removing SET LOCAL
enable_seqscan survives (exit 1): the shared test tables now hold enough rows that the
planner uses an index unprompted, so that pre-existing refute is not load-bearing today.
…rrect what describes it

- AC-32.1.2's guard now reads the partial index from pg_index: valid and ready (an
  interrupted CONCURRENTLY build leaves the same definition text but an INVALID index),
  keyed on expires_at, predicate (revoked_at IS NULL). The plan must use one of the two
  indexes that can serve expires_at <, by name, and never a Seq Scan.
- The moduledoc, the test name, the migration's comment (which said the composite index
  cannot be used; the planner reads it whole on a small table) and AC-32.1.2's wording
  now describe what is actually asserted.
- session_end_test's permanent-refusal sibling anchors on its own tenant_id too, and pins
  the log format the :busy refutation depends on.
- The plan regex no longer has a redundant alternative.

Not taken: moving plan CHOICE into an async: false scale test. The async test now
asserts eligibility deterministically; choice at scale is recorded in the migration.

Mutations (bin/mutate.sh, all exit 0): pg_index read from the other index (validity and
predicate fail); the allowed index names replaced; the :busy clause removed; the log's
tenant_id= reworded (the pinned format fails).
…ery, drop the EXPLAIN

- The EXPLAIN passed whenever either index existed (the composite can serve expires_at <
  too), so it proved nothing about the partial index. It is removed. The worker's sweep
  query is now RevokeExpiredDispatchesWorker.expired_query/1, and the test asserts its
  SQL carries revoked_at IS NULL and expires_at <, beside pg_index: valid, one key
  column expires_at, btree, on dispatches, predicate (revoked_at IS NULL).
- The :busy test refutes its own story and dispatch ids rather than one log format.
- The migration comment, the story's rationale and AC-32.1.2 no longer say the composite
  index cannot be used, and the AC names the ~20k-seeded-row capture, not production.

Mutations (bin/mutate.sh, all exit 0): revoked_at IS NULL dropped from the worker's
query; its < widened to <=; pg_index read from the other index; the :busy clause
removed; the permanent log's tenant_id= reworded.
…ry in a sync test

- The sweep query is private again. RevokeExpiredDispatchesPlanTest (async: false) seeds a
  production-shaped dispatches table (20k revoked history, 50 live), ANALYZEs it inside
  the sandbox transaction, captures the query perform/1 issues through repo telemetry and
  requires the default planner to use dispatches_expires_at_active_index. Sync modules run
  after every async one, so no neighbour's rows move the choice.
- The async worker test keeps only the pg_index shape check; the SQL substring check,
  which not and or mutations defeated, is gone.
- session_end tests read only error lines carrying a per-test request_id the test process
  sets. The busy test logs a canary in the same capture and requires it to be the only
  line, so an unreadable capture cannot pass it.
- AC-32.1.2 is back to its EXPLAIN wording and names the test. The migration comment no
  longer carries a stale copy of the query.

Mutations (bin/mutate.sh): worker predicate made non-implying (coalesce), exit 0; index
name swapped, exit 0; live fraction raised to 10 percent, exit 0; busy clause logs an
error, exit 0; request_id marker unset, exit 0; permanent refusal logged at warning,
exit 0.
…tions to a sync module

The rewrite's review showed the cause of its findings: asserting the planner's choice in the
shared test database. ANALYZE writes pg_class reltuples in place, so the stats it set
outlived the sandbox rollback and polluted later plans, the very flake class this PR
removes. So the plan test is gone from this PR. The natural-plan guard for AC-32.1.2 goes
into the scale job, which commits a seeded table on its own database, as its own change.

- budget_escalation_refused/4's tests move to BudgetEscalationRefusedTest, async: false.
  Sync modules run alone after every async one, so a whole-capture log == "" is this
  call's capture again, with no marker, format dependence or spawned-process gap.
- The worker test keeps the pg_index shape check. Its docs, the migration comment and the
  epic 35 README now say which index the planner chooses is not asserted in the suite.
- AC-32.1.2 keeps master's wording; the story's rationale keeps its SEEK correction.

Mutations (bin/mutate.sh, all exit 0): busy clause logs an error; permanent refusal logged
at warning; the catalog read pointed at the composite index.
@mkreyman
mkreyman enabled auto-merge (squash) September 28, 2026 18:17
@mkreyman
mkreyman merged commit 61de9df into master Sep 28, 2026
17 checks passed
@mkreyman
mkreyman deleted the fix/gate-flakes branch September 28, 2026 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant