mirror of
https://github.com/block/buzz.git
synced 2026-10-27 04:30:52 +00:00
bd73490418
## 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 #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](https://github.com/block/buzz/actions/runs/32789030203/job/97630827823): 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](https://github.com/block/buzz/actions/runs/32789030203/job/97626930420): 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 - Stacked on #6660 at exact head `561de54be4d9c2b622b7c2aa5b61bc3068f47e2a` - https://github.com/TheSentinel454/buzz/issues/20 - https://github.com/TheSentinel454/buzz/issues/20#issuecomment-5398526792 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 #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 #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 #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](https://github.com/block/buzz/actions/runs/32801679226/job/97667835080): 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`. --------- Signed-off-by: Luke Tornquist <tornquist@squareup.com> Signed-off-by: tornquist <tornquist@squareup.com>
74 lines
2.6 KiB
Bash
Executable File
74 lines
2.6 KiB
Bash
Executable File
#!/usr/bin/env bash
|
|
# Build one desired-state source database for a nextest PostgreSQL run. Each
|
|
# selected test clones it (or template0 for migration-owned tests). The outer
|
|
# postgres-test-run.sh process owns and removes the source database.
|
|
set -euo pipefail
|
|
|
|
: "${NEXTEST_ENV:?nextest must provide NEXTEST_ENV}"
|
|
: "${BUZZ_POSTGRES_ADMIN_URL:?set BUZZ_POSTGRES_ADMIN_URL to an administrator database URL}"
|
|
: "${BUZZ_POSTGRES_DESIRED_TEMPLATE:?postgres-test-run.sh must provide the desired-state database name}"
|
|
: "${PGHOST:?set PGHOST for pgschema}"
|
|
: "${PGPORT:?set PGPORT for pgschema}"
|
|
: "${PGUSER:?set PGUSER for pgschema}"
|
|
: "${PGPASSWORD:?set PGPASSWORD for pgschema}"
|
|
|
|
resolve_pg_command() {
|
|
local name="$1"
|
|
local candidate
|
|
if [[ -n "${PG_BIN_DIR:-}" ]]; then
|
|
candidate="${PG_BIN_DIR}/${name}"
|
|
else
|
|
candidate="$(command -v "$name" || true)"
|
|
fi
|
|
if [[ -z "$candidate" || ! -x "$candidate" ]]; then
|
|
echo "required PostgreSQL client is not executable: ${candidate:-$name}" >&2
|
|
exit 1
|
|
fi
|
|
printf '%s\n' "$candidate"
|
|
}
|
|
|
|
psql="$(resolve_pg_command psql)"
|
|
createdb="$(resolve_pg_command createdb)"
|
|
dropdb="$(resolve_pg_command dropdb)"
|
|
|
|
workspace_root="${NEXTEST_WORKSPACE_ROOT:-$(cd "$(dirname "$0")/.." && pwd)}"
|
|
template_database="$BUZZ_POSTGRES_DESIRED_TEMPLATE"
|
|
if [[ ! "$template_database" =~ ^buzz_nt_[0-9a-f]{20}_desired$ ]]; then
|
|
echo "refusing unsafe desired-state database name: $template_database" >&2
|
|
exit 1
|
|
fi
|
|
run_hash="${template_database#buzz_nt_}"
|
|
run_hash="${run_hash%_desired}"
|
|
|
|
"$dropdb" --if-exists --force \
|
|
--maintenance-db="$BUZZ_POSTGRES_ADMIN_URL" \
|
|
"$template_database" >/dev/null 2>&1
|
|
"$createdb" \
|
|
--maintenance-db="$BUZZ_POSTGRES_ADMIN_URL" \
|
|
--template=template0 \
|
|
"$template_database"
|
|
|
|
export PGDATABASE="$template_database"
|
|
export PGSCHEMA_PLAN_HOST="${PGSCHEMA_PLAN_HOST:-$PGHOST}"
|
|
export PGSCHEMA_PLAN_PORT="${PGSCHEMA_PLAN_PORT:-$PGPORT}"
|
|
export PGSCHEMA_PLAN_DB="$template_database"
|
|
export PGSCHEMA_PLAN_USER="${PGSCHEMA_PLAN_USER:-$PGUSER}"
|
|
export PGSCHEMA_PLAN_PASSWORD="${PGSCHEMA_PLAN_PASSWORD:-$PGPASSWORD}"
|
|
|
|
schema_log="${TMPDIR:-/tmp}/buzz-pgschema-${run_hash}.log"
|
|
if ! "$workspace_root/bin/pgschema" apply \
|
|
--file "$workspace_root/schema/schema.sql" \
|
|
--auto-approve >"$schema_log" 2>&1; then
|
|
cat "$schema_log" >&2
|
|
exit 1
|
|
fi
|
|
if ! "$psql" --dbname="$template_database" --set=ON_ERROR_STOP=1 \
|
|
--file="$workspace_root/scripts/reconcile-schema-after-pgschema.sql" \
|
|
>>"$schema_log" 2>&1; then
|
|
cat "$schema_log" >&2
|
|
exit 1
|
|
fi
|
|
rm -f "$schema_log"
|
|
|
|
printf 'BUZZ_POSTGRES_DESIRED_TEMPLATE=%s\n' "$template_database" >>"$NEXTEST_ENV"
|