Repository navigation
fix: settle the CI flakes on #2609 and the native record-timer harness races on #2505 - #2611
Merged
Merged
Conversation
… wall clock The many-small-rows test raced the 2 s per-call deadline in unoptimised CI builds. It moves into the engine's unit tests and runs under a long caller deadline, so it proves the operation and size limits admit the densest reply the cap allows. Production calls keep the 2 s ceiling. Refs #2609. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…uites The GovStack BReg suite starts bregctl dev, which pulls the image on first use, so a registry hiccup failed as a test. Refs #2609. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…sts a CI checkpoint stall The blind-index ceiling test failed in setup when the activation's managed-ACL catalog query hit the suite's 5 s statement timeout during another test's forced checkpoint. Every apply in the suite now uses one timeouts() helper with a 30 s statement bound. Refs #2609. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
A record_timer.worker.iteration_failed warning carried no cause, so a CI failure on it could not tell a lost peer race from an unavailable database. The warning now carries a closed, value-free `error` naming one of six classes. Refs #2505. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…fix two harness races Log mismatches now quote the offending WARN/ERROR lines, and a replica that exits or times out before readiness reports its log tail, both only after the forbidden-value check passes. A code expectation can be restricted to closed `error` values. The fairness journey's due window was shorter than its setup on a starved runner, so both replicas parked at their claims before refreshing the stale key; it now leaves fifteen seconds. Each replica may lose up to two stale refresh races to its peer, reported only as a concurrency conflict. The progress wait required every replica to be fresher than half a second in one sweep; it now latches each replica once it succeeds after release and names the held and last ages on timeout. Refs #2505. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2609. Refs #2505: two harness races are fixed and the diagnostics are in, but two of its CI symptoms still have no proven cause, so the issue should stay open until the next failure names one.
#2609
a_reply_of_many_small_rows_is_walked_to_the_endraced the 2 s per-call script deadline in an unoptimised build. The test moves into the engine's unit tests (registry-adapter-engine/src/scripts.rs) and runs through a privateinvoke_untilunder a long caller deadline, so it proves the operation and size limits admit the densest reply the cap allows. Production calls still go throughinvoke_thrown, which keeps the 2 s ceiling. Trade-off: no test now asserts that a cap-sized reply finishes within 2 s on production hardware; that is a performance property, not a correctness one, and a wall-clock assertion in a debug build could not prove it anyway.real_postgres_blind_index_reindex_refuses_more_rows_than_its_ceiling. The PostgreSQL server log from the failed run shows the cause: the activation's managed-ACL catalog query was cancelled by the test's 5 sstatement_timeoutwhile another test'sDROP DATABASEforced a checkpoint that spent 6.7 s writing. The query reads a few hundred catalog rows. Every apply inpostgres_migration.rsnow goes through onetimeouts()helper with a 30 s statement bound. No test in the suite proves an apply statement times out; the rehearsal timeout test uses the reviewed migration's own 200 ms declaration and is unaffected.real_breg_and_adapter_preserve_write_receipts_conditions_and_exact_recoveryfailed becausebregctl devcould not download the pinned ThunderID image. The "Registry Adapter PostgreSQL and recovery suites" job now pulls the image named inthunderid-version.jsonwith three attempts before the suites, so a registry hiccup fails as that step. Other jobs that startbregctl devare unchanged.#2505
Five symptoms, with their status:
record_timer.worker.iteration_failedwarning in the fairness runProduct change: the
record_timer.worker.iteration_failedwarning now carries a closed, value-freeerrorfield. It names one of six classes: runtime unavailable, concurrency conflict, commit unresolved, handler failure, field-encryption unavailable, or refused. The commit also adds a CHANGELOG entry and a sentence in the monitor docs. Log lines are quoted only after the existing forbidden-value check passes over every replica's log.Open product question, not changed here: in the refresh, refresh-control-cleared and cancel lanes a lost peer race logs a warning, but in the expire lane the same race returns quietly. Whether a lost race should warn at all is a separate call.
Not in this PR
migration.rsdiscards the underlying error (reconcile_runtime_acl(...).is_ok(), andFrom<tokio_postgres::Error>maps every database error toConnection), so an operator or a test sees a bareApplyFailed. The cause of item 2 was only recoverable from the server log. Carrying a value-free cause (the existingPostgresFailureshape) through that path is a product error-shape change; it should be its own change.govstack_consultation_served.rsa_page_at_the_declared_maximum_is_shaped_within_the_script_budgetasserts under the same 2 s wall-clock deadline and may flake the same way; it has not failed yet.registry-adapter-jobsigned_coordinator_event_native.rspostgres_source_reaches_native_coordinator_and_reviews_a_revised_rowfailed once on this PR's first CI run at its 30 s wall-clock wait for the native Run to finish ("the native Run did not finish as revised-reviewed"). This PR does not touch that file, it passed on re-run, and it has no earlier failure in the last 60 CI runs. It is the same class of fixed wall-clock bound.Verification
cargo fmt --check: clean.cargo clippy -D warnings: clean onregistry-adapter-engineandregistry-adapter(all targets), and onregistry-breg(all targets, default features).scriptsunit tests (5/5) andscript_budget(4/4).postgres_migrationagainst the CI PostGIS image: 61/61.iteration_failures_map_to_closed_value_free_causes: 1/1.actionlintandrelease/scripts/check-gates-inventory.py: pass.clippy --features postgres-test,tooling,schema --test postgres_migrationreports a pre-existingtype_complexityatpostgres_migration.rs:9033. CI does not gate on that combination.