Skip to content

ci: run PostgreSQL tests in isolated lane - #6730

Open
TheSentinel454 wants to merge 17 commits into
mainfrom
codex/issue-20-postgres-ci
Open

ci: run PostgreSQL tests in isolated lane#6730
TheSentinel454 wants to merge 17 commits into
mainfrom
codex/issue-20-postgres-ci

Conversation

@TheSentinel454

@TheSentinel454 TheSentinel454 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Why

PR #6660 introduced four PostgreSQL-backed persistence tests and a focused CI selector, but an exact test-name list does not automatically cover future database tests. Broad ignored-test execution also exposed shared-schema races and ambiguity between desired-state and migration-applied schema expectations.

What

  • Establish a discoverable convention: PostgreSQL unit modules use postgres_tests, PostgreSQL integration binaries use a postgres_ prefix, and unrelated external-infrastructure tests use an external_infra_ prefix.
  • Add a dedicated nextest PostgreSQL profile and archive-backed CI job covering the relevant crates without enumerating test names.
  • Create a run-scoped desired-state source database and a unique PostgreSQL database per test process for parallel-safe isolation, with cleanup on success, failure, or interruption.
  • Route destructive migration and migration-parity tests to clean template0 databases while desired-state tests clone the desired-state database.
  • Document discovery, schema modes, required role privileges, and a bounded portable local runner.
  • Keep the existing infrastructure-free unit-test jobs unchanged.

This is intentionally limited to test and CI harness behavior. It does not move production database code, change persistence semantics, or implement issue #20's broader shared test-utility refactor.

Risk

Low production risk because all changes are confined to tests, CI configuration, documentation, and test harness scripts.

Remaining operational risks:

  • Cleanup retries dropdb --force five times across roughly four seconds. Exhaustion warns with the database name but deliberately does not mask the test result; individual PostgreSQL diagnostics are suppressed.
  • The seven-package boundary is duplicated between the runner and CI archive and must remain synchronized if PostgreSQL tests move to a new crate.
  • Portable SHA fallback branches were exercised on Linux; no macOS Blox workstation was needed for this Linux CI artifact.
  • GitHub's unchanged generic Unit Tests job currently fails while cold-linking buzz-voice because sherpa-onnx-c-api is absent. The branch changes no voice/build/toolchain inputs; the exact parent previously passed that job, and Blox passes the same infrastructure-free suite (1,450/1,450). A prior-head one-job retry and the final-head run both reproduced the hosted-runner failure.

Verification

Author Blox workstation 2020088:

  • cargo fmt --all -- --check
  • just clippy
  • just test-unit: 1,450 passed; PostgreSQL tests remain skipped in fast jobs
  • PR Centralize replaceable event persistence #6660 focused tests: 4/4 passed through the final runner
  • Full lane: 282/282 passed in 10.087s with a non-superuser role limited to CREATEDB, CREATEROLE, and pg_read_all_stats
    • 272 desired-state tests
    • 10 migration-applied tests
    • 6 explicitly filtered external-infrastructure tests
  • Cleanup fault injection: a deliberately underprivileged 282-test run produced the expected 277 passes and 5 failures, and the post-run catalog audit found zero lane databases after retry cleanup. The restored successful run also left zero lane databases.

GitHub exact final head 92c231e48f299ea2af23817763f5ae80ac013d68:

  • PostgreSQL Tests: 282/282 passed across 10 binaries in 98.886s; 1,240 skipped, including 6 via the profile filter
  • The shared relay/PostgreSQL archive predecessor completed successfully
  • Unit Tests: unrelated sherpa-onnx-c-api native-link failure described above
  • Independent exact-head review on separate Blox workstation 2022762: no substantive findings after all initial findings were addressed. The reviewer independently verified cleanup retry behavior, exit-status preservation, shell portability, syntax, and a clean exact-head worktree.

References

Generated with Codex.

Update — August 24, 2026 review follow-up

  • Centralized the repeated PostgreSQL test URL resolution in crate-local, test-only helpers without introducing the broader shared utility refactor from issue Add just goose for one-command agent setup + fix API token auth #20.
  • Restored descriptive hybrid/Redis test function names. Structural external_infra_*_tests modules now own exclusion, and the nextest filter only recognizes module path segments.
  • Added a three-second source guard that scans every Rust file and fails CI when an ignored PostgreSQL test would be omitted or an external-infrastructure test would be included. Fixture tests cover accepted modules/binaries and both failure modes.
  • Removed the two implementation-plan documents.

Performance profiling on Blox workstation 2027352 (same prebuilt archive and cargo-nextest 0.9.143 for every comparison):

  • Current per-test database model, 8 workers: 10.15–10.40s across three runs. The 283 createdb calls and 284 dropdb calls consumed 28.85–29.46 aggregate seconds, about 39–40% of aggregate test-process duration.
  • Reusing one database per worker without cleanup: 6.55–6.84s, about 35% faster, but all three runs failed because global matcher-queue tests inherited a quiesced community from an earlier test; the failing test varied with scheduling.
  • Reusing workers with TRUNCATE … RESTART IDENTITY CASCADE: all 282 tests passed, but total time regressed to 10.47–10.67s. The 265 truncate calls consumed 24.34–25.45 aggregate seconds, so truncation merely replaced most clone/drop cost.
  • Raising concurrency to 16 workers reduced one isolated-database run to 8.79s, but 24 workers exposed a cluster-global pg_stat_activity race. The profile remains at the proven-safe 8 workers.

Decision: retain unique per-test databases. Harness cleanup uses dropdb --if-exists --force; it drops the database rather than truncating tables or deleting rows.

Verification at local branch head 60086c1f6ee34a193c342255a5e8e6293b60e988:

  • Discovery inventory: exactly 282 intended tests; all 6 hybrid/Redis tests present and structurally excluded.
  • PR Centralize replaceable event persistence #6660 focused persistence tests: 4/4 passed.
  • Full PostgreSQL lane: 282/282 passed three times; 1,240 skipped each run; wall time 9.72–9.96s without profiling shims.
  • Non-ignored tests with a desired-state database: 1,234/1,234 passed; one mesh-demo timeout from the first run passed immediately in isolation.
  • cargo fmt --all -- --check and all-target/all-feature clippy for buzz-db, buzz-deletion, and buzz-relay passed.

Final independent re-review at exact head 42097c0136ab3fa8f0efe5720ae829c04575b9b6 on separate Blox workstation 2028455: no substantive residual findings. The reviewer independently exercised ordinary strings, zero/one/three-hash raw strings, line/block comments, and string-contained lookalikes, then reran the full 353-file scan, shell syntax, Python AST parsing, and diff check.

Independent review found that the source guard's initial regular expression could miss raw-string ignore reasons and treat a commented attribute as real. Final head 42097c0136ab3fa8f0efe5720ae829c04575b9b6 parses valid ordinary/raw Rust string literals only at attributes found in comment-sanitized source; regression fixtures cover both cases. The fixture suite, Python compilation, shell syntax, diff check, and full 353-file repository scan pass on Blox.

Update — August 25, 2026 restack

PR #6660 merged, so this follow-up was rebased from its former exact parent onto current origin/main (8d2d0ff5ad42733e9949442c4b6358d0ba87f9a8). The final candidate head is 05dcc2ab28948e3ab79bb44839a69f2ba44648a2; the PR no longer carries #6660's pre-squash history.

Fresh exact-head verification on Blox workstation 2028572:

  • Discovery guard passed across 353 Rust files; inventory remained exactly 282 intended PostgreSQL tests.
  • PR Centralize replaceable event persistence #6660 focused persistence tests: 4/4 passed.
  • Full PostgreSQL lane: 282/282 passed three times; 1,243 skipped each run; wall time 9.70–9.87s.
  • cargo fmt --all -- --check and all-target/all-feature clippy for buzz-db, buzz-deletion, and buzz-relay passed.

Update — August 25, 2026 final hosted verification

  • GitHub PostgreSQL Tests: 282/282 passed in 53.787s; 1,243 skipped.
  • The final guard scanned all 353 Rust files successfully before the hosted lane ran.
  • The shared relay/archive prerequisite and infrastructure-free Unit Tests job both passed at exact final head 42097c0136ab3fa8f0efe5720ae829c04575b9b6.

@TheSentinel454
TheSentinel454 force-pushed the codex/issue-20-postgres-ci branch from de8c835 to 0ffeede Compare August 24, 2026 22:40
@TheSentinel454
TheSentinel454 marked this pull request as ready for review August 24, 2026 23:14
@TheSentinel454
TheSentinel454 requested a review from a team as a code owner August 24, 2026 23:14
@TheSentinel454
TheSentinel454 marked this pull request as draft August 24, 2026 23:15
@TheSentinel454
TheSentinel454 force-pushed the codex/issue-20-postgres-ci branch from 0ffeede to 92c231e Compare August 24, 2026 23:22
@TheSentinel454
TheSentinel454 marked this pull request as ready for review August 24, 2026 23:45
Comment thread crates/buzz-db/src/usage.rs Outdated
Comment thread crates/buzz-deletion/src/lib.rs Outdated
Comment thread docs/plans/2026-08-24-postgres-ci-lane-design.md Outdated
Base automatically changed from codex/issue-4-replaceable-persistence to main August 25, 2026 00:42
@TheSentinel454
TheSentinel454 force-pushed the codex/issue-20-postgres-ci branch from 60086c1 to 05dcc2a Compare August 25, 2026 02:13

@TheSentinel454 TheSentinel454 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks are failing, please look into that.

Comment thread .github/workflows/ci.yml Outdated
Comment thread scripts/check-postgres-test-discovery.py
Comment thread .config/nextest.toml
Comment thread .github/workflows/ci.yml Outdated
Comment thread crates/buzz-db/src/store/channel_members.rs
Comment thread crates/buzz-db/src/store/channel_members.rs
Comment thread crates/buzz-db/src/channel.rs Outdated
Comment thread CONTRIBUTING.md
@TheSentinel454
TheSentinel454 force-pushed the codex/issue-20-postgres-ci branch from 42097c0 to e5ab1bb Compare August 25, 2026 14:42
@TheSentinel454

Copy link
Copy Markdown
Contributor Author

🤖 Follow-up on the earlier “Checks are failing” review note: I fixed the workflow environment scoping and the PostgreSQL schema/isolation issues exposed by the broad lane, then reran the exact pushed head. The PostgreSQL job passed 282/282 tests, and the complete GitHub CI workflow is green: https://github.com/block/buzz/actions/runs/32867749069. An independent exact-head Blox review also found no substantive issues.

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is 2c99ee7af5a20d239e95ec5368407887449795c9...8ab6bba4882e48c738e38b9e3b1b9a7ae193c20c.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review 8ab6bba4882e48c738e38b9e3b1b9a7ae193c20c to authorize a new review.
Any previous review applies only to its recorded range.

Signed-off-by: Luke Tornquist <tornquist@squareup.com>
Signed-off-by: Luke Tornquist <tornquist@squareup.com>
Signed-off-by: Luke Tornquist <tornquist@squareup.com>
Signed-off-by: Luke Tornquist <tornquist@squareup.com>
Signed-off-by: Luke Tornquist <tornquist@squareup.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: tornquist <tornquist@squareup.com>
@TheSentinel454
TheSentinel454 force-pushed the codex/issue-20-postgres-ci branch from 7809198 to 8ab6bba Compare August 28, 2026 21:53
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