From a15972066b4b4d0c4600baba87cb894678af4fd9 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Fri, 1 May 2026 22:00:04 -0700 Subject: [PATCH] harness(phase-2-followup): fix assert_status mislabel + honest race comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review nits from PR #2493 that don't affect correctness but matter for honesty in the harness's own self-documentation: 1. tenant-isolation.sh F3/F4 used assert_status for non-HTTP values. LEAKED_INTO_ALPHA/BETA are jq-derived counts, not HTTP codes — but the assertion ran through assert_status, which formats the result as "(HTTP 0)". Anyone reading the test output would believe these assertions involved an HTTP call. Adds a plain `assert` helper matching per-tenant-independence.sh's pattern, and uses it on the two count comparisons. 2. per-tenant-independence.sh Phase F over-claimed coverage. The comment said the concurrent-INSERT race catches "shared-pool corruption" + "lib/pq prepared-statement cache collision". Both are real failure modes — but neither can fire across tenants in THIS topology, because each tenant owns its own DATABASE_URL and its own postgres-{alpha,beta} container. The comment now lists only what the test actually catches (redis cross-keyspace bleed, shared cp-stub state corruption, cf-proxy buffer mixup) and notes that a future shared-Postgres variant is the right place for the lib/pq cache assertion. No behavioural change — both replays still pass 13/13 + 12/12, all six replays pass on a clean run-all-replays.sh boot. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../harness/replays/per-tenant-independence.sh | 15 +++++++++++---- tests/harness/replays/tenant-isolation.sh | 18 ++++++++++++++++-- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/tests/harness/replays/per-tenant-independence.sh b/tests/harness/replays/per-tenant-independence.sh index 86e8759a..e80a663a 100755 --- a/tests/harness/replays/per-tenant-independence.sh +++ b/tests/harness/replays/per-tenant-independence.sh @@ -120,10 +120,17 @@ assert "E3: postgres-alpha has zero beta-named workspaces" "0" "$ALPHA_HAS_BETA" assert "E4: postgres-beta has zero alpha-named workspaces" "0" "$BETA_HAS_ALPHA" # ─── Phase F: concurrent INSERT race ─────────────────────────────────── -# Both tenants take turns inserting 10 rows concurrently. Race shape -# catches: shared-connection-pool corruption, lib/pq prepared-statement -# cache collision (org-wide hazard per memory), redis cross-keyspace -# bleed. Each side must end with EXACTLY +10 rows from its own writes. +# Both tenants insert 10 rows concurrently. Race shape catches the +# failure modes that CAN cross tenants in this topology: +# - redis cross-keyspace bleed (shared redis container). +# - shared-cp-stub state corruption (single Go process serves both). +# - cf-proxy buffer mixup under simultaneous in-flight writes. +# Does NOT catch lib/pq prepared-statement cache collision or shared +# *sql.DB pool poisoning — each tenant has its own DATABASE_URL and +# its own postgres-{alpha,beta} container, so there is no shared pool +# to corrupt. A future replay variant on a single shared Postgres +# would be the right place to assert that failure mode. +# Each side must end with EXACTLY +10 rows from its own writes. echo "" echo "[replay] F. concurrent insert race — 10 rows per tenant in parallel" diff --git a/tests/harness/replays/tenant-isolation.sh b/tests/harness/replays/tenant-isolation.sh index 48887c6f..13e4ddf3 100755 --- a/tests/harness/replays/tenant-isolation.sh +++ b/tests/harness/replays/tenant-isolation.sh @@ -61,6 +61,20 @@ assert_status() { fi } +# Plain equality check — for non-HTTP values (counts, names, etc.). +# Distinct from assert_status so output reads naturally instead of +# claiming "(HTTP 0)" for what is really a count. +assert() { + local desc="$1" expected="$2" actual="$3" + if [ "$expected" = "$actual" ]; then + printf " PASS %s\n" "$desc" + PASS=$((PASS + 1)) + else + printf " FAIL %s\n expected: %s\n got : %s\n" "$desc" "$expected" "$actual" >&2 + FAIL=$((FAIL + 1)) + fi +} + # ─── Phase A: positive controls ──────────────────────────────────────── echo "[replay] A. positive controls — each tenant accepts its own valid creds" @@ -148,11 +162,11 @@ fi # Cross-check: neither tenant's list contains the other's workspace ids. LEAKED_INTO_ALPHA=$(echo "$ALPHA_LIST" | jq -r --arg b1 "$BETA_PARENT_ID" --arg b2 "$BETA_CHILD_ID" \ '[.[] | select(.id == $b1 or .id == $b2)] | length') -assert_status "F3: alpha list contains zero beta workspace ids" "0" "$LEAKED_INTO_ALPHA" +assert "F3: alpha list contains zero beta workspace ids" "0" "$LEAKED_INTO_ALPHA" LEAKED_INTO_BETA=$(echo "$BETA_LIST" | jq -r --arg a1 "$ALPHA_PARENT_ID" --arg a2 "$ALPHA_CHILD_ID" \ '[.[] | select(.id == $a1 or .id == $a2)] | length') -assert_status "F4: beta list contains zero alpha workspace ids" "0" "$LEAKED_INTO_BETA" +assert "F4: beta list contains zero alpha workspace ids" "0" "$LEAKED_INTO_BETA" # ─── Phase G: /health is allowlisted (sanity) ────────────────────────── echo ""