Skip to content

fix(db): bound every statement and the #p mention scan - #8461

Merged
TheSentinel454 merged 7 commits into
mainfrom
elrond/db-statement-deadlines
Oct 21, 2026
Merged

TheSentinel454 merged 7 commits into
mainfrom
elrond/db-statement-deadlines

Conversation

@TheSentinel454

@TheSentinel454 TheSentinel454 commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Problem

An agent #p + #h + since REQ (the require_mention subscription in buzz-acp) ran for over 45 minutes on a production writer and held AccessShare on every events partition. There were two causes:

  1. The mention side of the #p join was unbounded. build_query_events_sql joined event_mentions on (community_id, event_id) only. The planner picked a nested loop whose outer side was every mention of the pubkey, ever, and whose inner side range-scanned the channel index from since on each partition. Cost ≈ total mentions × channel rows since since.
  2. Nothing cancelled it. The writer statement_timeout defaulted to 0. Only #e reads had a deadline (the 20s cap from 🤖 perf: bound long-thread aux reads and make query deadlines terminal #7854), and read-replica sessions had none at all.

pg_stat_statements shows this shape has a multi-thousand-second max going back months, so it is chronic and was not introduced by a recent change.

Change

#p mention bound (store/event.rs, push_mention_join, shared by the page SELECT and COUNT)

  • Join on the full event key, AND e.created_at = m.event_created_at. Mention rows copy the event's created_at at insert (migration 0007 already relies on this equality), so the join can prune events partitions by time.
  • Repeat since/until on m.event_created_at, so the outer scan is a range on the existing idx_event_mentions_pubkey_created. until is a superset bound for the keyset cursor; the exact (created_at, id) cursor test stays on e.
  • Repeat kinds on m.event_kind when the filter has kinds. Every production mention writer binds the event kind (insert_mentions in store/runtime/mod.rs, and the replaceable-event path in store/replaceable.rs, back to the first Postgres schema), and no migration inserts mention rows, so this drops no rows written by the relay. Rows imported out of band with a NULL event_kind would no longer match a kinds-filtered #p read; a read-only production count on 2026-10-20 found 0 such rows on both production reader databases. The pushdown makes idx_event_mentions_pubkey_kind_created available to the planner for the require_mention kind-9 shape; its use has not been observed (see below). An E2E fixture with a hand-set NULL event_kind row confirmed the caveat: base returns 8 events, head 7.
  • The feed's mentions and needs-action queries (store/feed.rs, build_mentions_query / build_needs_action_query) join event_mentions the same way, so each joined row probes one events partition instead of all of them. Their ORDER BY m.event_created_at ... LIMIT already kept them from running away; this removes the per-row cross-partition probe.
  • Observed plan (E2E at 9cb6638b1, before the kinds pushdown; 60,000 historical mentions + 2,500 recent decoys, stock planner, EXPLAIN (ANALYZE, BUFFERS) of the captured production SQL): the mention side is a bounded range on idx_event_mentions_pubkey_created (13 rows), then a hash join with a scan of the recent events partition. Base scanned 2,509 recent events and probed event_mentions_pkey 2,509 times. Head: 1.1 ms / 319 shared hits; base: 8.5 ms / 10,049 shared hits. Not measured against the production replica. A fresh EXPLAIN (ANALYZE, BUFFERS) replay at a7065c367, after the pushdown, ran on a smaller fixture where the planner chose a sequential mention scan, so it neither confirms nor replaces these numbers.

Reads: 20s deadline on every generic event read

  • query_events_on and count_events_on always run under the transaction-local 20s deadline that #e reads used (fetch_with_read_deadline, formerly fetch_with_e_tag_deadline). It only tightens: shorter operator or caller caps are kept.
  • Read-replica sessions get a 20s session default through after_connect, using the same tighten-only SQL.
  • Cost: every generic REQ history read and COUNT now pays BEGIN + set_config + ROLLBACK (three extra round trips; before, only #e reads did). In the local E2E the end-to-end history read was still faster at head (median 4.1 ms vs 10.2 ms at base, 9 samples each, warm cache), so the round trips are dominated by the bounded plan; this does not isolate them or establish production p99. The round trips also lengthen how long each such read holds its writer connection; that hold time has not been measured against production pool sizing.
  • A replica read cancelled at its deadline (57014) is returned, not re-run on the writer. Usually the cancel means expensive work the writer would only repeat. It can also mean the replica waited on a lock only it holds (a replayed ACCESS EXCLUSIVE from primary DDL), when the writer would have answered quickly; that miss is the accepted price of keeping runaway reads off the primary. The rationale now lives once on DbError::is_statement_cancelled instead of in a dozen copied comments. The generic routed reads and ReadSession already did this; the eight routed reads with their own SQL (feed mentions, needs-action and activity; thread replies; channel window; relay membership; thread window and its auxiliary scan) now do too. A cancelled auxiliary scan also keeps its replica session instead of degrading it to the writer for the rest of the request. Other replica failures (e.g. hot-standby recovery conflicts) still fall back to the writer.
  • Paths with their own tighter SET LOCAL (artifact 2–5s, thread window 4s, personal reads, usage 5–15s, serving lease 1.5s) are unchanged.

Writes: every writer session is bounded

  • The default statement_timeout changes from 0 to 120s (DEFAULT_STATEMENT_TIMEOUT_MS).
  • BUZZ_DB_STATEMENT_TIMEOUT_MS still overrides it. 0 or any value above 300s installs 300s (MAX_STATEMENT_TIMEOUT_MS), so a session can no longer disable it, and a clamped value logs a warn! when the pool is built. Only the operator-approved, fenced destruction paths below lift it, per transaction.

Long-running writer paths, audited

Path Behavior
Migrations / schema destruction (with_exclusive_schema_destruction_lock) Unchanged: these lift lock and statement timeouts on their own detached connection
Partition maintenance Unchanged: own 5s statement_timeout
Partition audit (buzz-admin) Unchanged: 30s
Community deletion inventory, purge and logical verification (inventory_schema whole-tenant counts, purge_postgres whole-community DELETE per table in one tx, verify_postgres_logically_deleted whole-tenant EXISTS per table right after the purge) Lifted (statement_timeout = 0) transaction-locally by lift_statement_timeout, like migrations; lock and idle timeouts still bound them. Inventory counts each table in its own lifted transaction, so each table's lock and snapshot are released as soon as its count finishes, as the autocommit counts did. A cancelled purge would retry from scratch and eventually block, so a cap here would turn large-tenant deletion into an operator override
Audit appends (buzz-audit, audit pool inherits the writer cap) Waits on the audit-chain advisory lock. With BUZZ_DB_LOCK_TIMEOUT_MS=0 (or at least the statement cap), statement_timeout (57014) cancels the wait instead of lock_timeout (55P03). The relay's audit worker now retries both with the same backoff instead of dropping the entry on 57014; admission refusal stays terminal

On HTTP routes a statement timeout already maps to the distinct retryable 503 (api/mod.rs, statement_timeout_maps_to_distinct_503). A cancelled REQ fails its history read instead of hanging.

Tests

  • p_tag_query_bounds_mention_side_by_time (unit): page and COUNT SQL carry the created_at join and the since/until/kinds mention bounds, and the bounds are absent when the filter has none.
  • p_tag_read_with_mention_bounds_keeps_results (Postgres): through the production mention insert, unbounded, since, window and keyset-cursor reads return exactly the mentioned rows, and COUNT matches. Every bound sits exactly on a mentioned row's second and the cursor page includes a same-second sibling after the cursor id, so an off-by-one on either mention-side bound fails it.
  • plain_read_is_cancelled_at_deadline_when_timeout_disabled / count_events_cancels_at_read_deadline_when_timeout_disabled (Postgres): the production #p + #h + since shape and a plain COUNT are cancelled at 20s with the session timeout disabled. Previously these covered #e only.
  • writer_statement_timeout_is_always_bounded (unit) and session_statement_timeouts_bound_writer_and_reader_sessions (Postgres): through Db::new, writer sessions install 2min / 5min (for 0 and for 600s) / 45s, and replica sessions install 20s.
  • deletion_whole_tenant_stages_outlive_the_writer_statement_timeout (Postgres): on a scratch desired-state database with a 500ms writer cap, the real inventory_schema, purge_postgres and verify_postgres_logically_deleted each wait behind a held ACCESS EXCLUSIVE lock on events for over 1s and still complete, a plain statement is cancelled (57014), and pooled sessions still read 500ms afterwards. A lifted transaction reads statement_timeout = 0, so a bounded lift (e.g. '300000') fails it, as does removing any of the three lift_statement_timeout calls.
  • mentions_query_is_tenant_scoped_and_joins_mentions_by_composite_key / needs_action_query_is_tenant_scoped_and_joins_mentions_by_composite_key (unit): both feed queries carry the e.created_at = m.event_created_at join.
  • replica_statement_cancel_propagates_without_writer_rerun (Postgres): with the replica locked past its cap, each of the six routed reads above returns 57014 instead of the writer's copy. Removing the cancel arm in any of them fails it.
  • thread_window_statement_cancel_propagates_without_writer_rerun (Postgres): a thread-window read parked behind a replica lock and cancelled (57014; the window's own 1s lock_timeout would otherwise fire first) returns the cancel instead of the unlocked writer's window, and an auxiliary scan timed out on a replica session returns 57014 and leaves the session on the replica. Removing either guard fails it.
  • audit_worker_retries_statement_timeout_until_original_entry_is_appended_once (Postgres, buzz-relay): the production audit worker, with lock_timeout disabled and a 100ms statement cap, keeps the entry through repeated 57014 cancels while the chain lock is held and appends it exactly once after release. Treating only 55P03 as retryable fails it.
  • The existing writer and audit-pool GUC tests now expect 5min for a disabled statement timeout.

After deploy, watch

  • buzz_db_route_decision{decision="replica",reason="statement_cancelled"}: replica reads that now fail instead of falling back to the writer, including the replica-only lock case above.
  • buzz_db_pool_acquire_wait_seconds and buzz_db_pool_waiters on the writer pool: every generic REQ history read and COUNT now holds its connection for three extra round trips.

Database checklist (docs/database.md § Review checklist)

This PR changes schema-dependent buzz-db queries (the #p join and its m.event_kind pushdown). It does not change the schema.

  • Phase: none. No schema change, so this is not an expand, backfill or contract step. Nothing before or after it is required.
  • Triggers, functions, procedures, rules, foreign keys: none added.
  • Row mutations / backfill: no migration, and no row is mutated or backfilled. The one data-shape dependency is mention rows with a NULL event_kind: a kinds-filtered #p read no longer matches them (see the kinds bullet under Change). Relay writers always bind event_kind, so only rows imported out of band are affected. A read-only production count on 2026-10-20 found 0 such rows on both production reader databases.
  • Contract migration: N/A.
  • Rule 4 locations: none touched. No table, index or desired-state file changes, so no parity test applies. The new bounds use the existing idx_event_mentions_pubkey_created and idx_event_mentions_pubkey_kind_created.
  • Deployed relay vs. new schema: there is no new schema. The currently deployed relay and this relay run against the same tables.
  • Lock and statement timeouts: this PR is about them. Generic reads get a 20s deadline, replica sessions a 20s default, and writer sessions a 120s default capped at 300s. Community-deletion transactions lift statement_timeout per transaction; migrations and schema destruction are unchanged (see "Long-running writer paths, audited"). No index builds and no operator prebuild step.

What got simpler

  • One read-deadline path instead of an #e-only branch in query_events_on and count_events_on.
  • The #p join SQL that was duplicated between SELECT and COUNT now lives in one push_mention_join.
  • The deletion tests share one scratch desired-state database helper instead of an inline copy.
  • "Is this query bounded?" now has one answer: yes, unless it runs in a migration, schema destruction, or community-deletion transaction.

Identified debt (not in this PR)

  • buzz-acp re-sends the same filter after a timeout CLOSED (no backoff or narrowing). With the #p bound the require_mention read should finish well inside 20s, so this is a follow-up, not a blocker.

@TheSentinel454
TheSentinel454 marked this pull request as ready for review October 19, 2026 17:57
@TheSentinel454
TheSentinel454 requested a review from a team as a code owner October 19, 2026 17:57
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 19, 2026
@github-actions

github-actions Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

  • Exact PR diff: d97f6b4c6ed616be0c3f1eb0e13d1eaa9e338a66...0fbd0593b09a95212e93b9988936b7032e5d5fc8
  • Model: gpt-6.1-sol

💡 Click "edited" above to see earlier reviews for this PR.


Review Summary

Overall Risk: NONE

No concrete security, correctness, or reliability findings were identified in the authorized PR range.

Findings

No concrete security, correctness, or reliability findings were identified.

Notes

  • Review used read-only source and history inspection. Builds and tests were not run.

Generated by Codex Security Review |
Requested by: @TheSentinel454 |
Workflow run

@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 19, 2026
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 19, 2026

@wpfleger96 wpfleger96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 Automated review of 9cb6638b1ad9d28f2a0819a6c54f19d2e9ad5ed0 (base dc4cdc4990877d400a435b481a416a73f17247bb): two blocking issues.

Thanks for this. The #p change holds up. Joining on e.created_at = m.event_created_at and repeating since/until on the mention side returns the same rows for since-only, until-only, both, page and COUNT, and the cursor second is still kept whole on m while e does the exact (created_at, id) tie-break. Every production writer copies the signed event second into the mention row. The tighten-only read deadline is correct on both the transaction path and the replica after_connect, and the deletion lifts are transaction-local with no leak into pooled connections. All seven new tests passed by name in the hosted Postgres and unit jobs, and removing the mention bounds or the 20s read deadline makes them fail.

Both blockers are consumers of the new caps that react to 57014 the wrong way.

Blocking: audit entries can now be dropped

The audit pool inherits the writer cap (crates/buzz-relay/src/main.rs:42-51). AuditService::log waits on the community's audit-chain advisory lock (crates/buzz-audit/src/service.rs:58-61). With BUZZ_DB_LOCK_TIMEOUT_MS=0, which is supported, or any lock timeout longer than the statement timeout, an append that waits more than 120s now ends in 57014. The worker only retries 55P03 (is_lock_timeout, crates/buzz-relay/src/state.rs:2420-2451). Every other error is logged and the entry is gone. Before this PR the wait had no cap and the append went through once the lock was released.

Fix: treat a cancelled statement (57014) as a rolled-back, retryable append with the same backoff, and keep admission refusal terminal. Add a regression through the production worker: short statement cap, lock timeout disabled, hold the audit-chain lock past the cap, release it, and assert the original entry is appended. Please don't remove the cap from the audit pool to fix this.

Blocking: a replica read cancelled at 20s now reruns on the writer

Replica sessions now get a 20s statement_timeout (crates/buzz-db/src/runtime/mod.rs:991-999). Six routed reads rerun on the writer after any replica error, including 57014:

  • feed mentions, needs-action and activity: crates/buzz-db/src/store/feed.rs:382-393, 467-478, 542-552
  • thread replies and channel window: crates/buzz-db/src/store/thread.rs:932-952, 1109-1135
  • relay membership: crates/buzz-db/src/store/relay_members.rs:991-994

These run their own SQL rather than the generic event read, so the writer rerun gets the writer's 120s cap per statement, or up to 300s if configured, and no 20s read deadline. Before this PR a slow replica read either finished on the replica or kept running there. The writer never saw it. Now the read is cancelled at 20s and the same query starts on the primary, at the moment we already know it's expensive. ReadSession::query_events (runtime/mod.rs:355-360) and the routed event reads in store/event.rs already return a cancelled statement as-is.

Fix: check is_statement_cancelled() before the fallback in all six branches and return the error, keeping the fallback for other replica failures. Add a routed-path test that forces a replica cancellation and asserts the writer isn't queried. If these reads should also be capped at 20s when they're routed to the writer first, apply the same tighten-only read deadline there.

Optional

  • deletion_transactions_lift_the_writer_statement_timeout (crates/buzz-db/src/store/deletion.rs:7391-7435) calls lift_statement_timeout directly and never calls inventory_schema or purge_postgres. Deleting the production call at deletion.rs:2250 leaves the whole buzz-db Postgres suite green. The runtime code is right, so this doesn't block. But that lift is what keeps a large-tenant purge from being cancelled and retried forever, so it should have a test that runs both production paths under a short writer cap and fails when either lift is removed. The PR description's test bullet for it should also say it exercises the helper.
  • inventory_schema now runs every row count in one read transaction (deletion.rs:1237-1255), so one snapshot is held on the writer for the whole inventory instead of one per count. That's worth a sentence in the description for large tenants.
  • Every generic read on the writer now pays BEGIN, set_config and ROLLBACK, which used to happen only for #e. That's three extra round trips per REQ history read routed to the writer. That's probably fine, but it's not mentioned anywhere.
  • The first commit's body says the purge raises itself to the 300s ceiling. The second commit replaced that with a lift to 0. If the squash keeps commit bodies, drop that line.
  • The description says every query is bounded except migrations, schema destruction and deletion. The audit wait and replica fallback above are the cases where that bound changes behavior. Worth a line once they're fixed.
  • The description doesn't include buzz-review-completed.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Star Lord’s automated source review via Wes’s GitHub account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/operator behavior) reviews.

Two low-severity P3 findings inline. These are non-blocking review comments; humans decide merge readiness. The mention bounds preserve timestamp/keyset semantics, and the read deadline preserves shorter caps and restores transaction-local settings.

Reviewed head 9cb6638b1ad9d28f2a0819a6c54f19d2e9ad5ed0 against base/merge-base dc4cdc4990877d400a435b481a416a73f17247bb. Source-only: no tests, SQL, app execution, query plans, load or latency measurements. One CI snapshot showed 56 successful and 28 skipped checks; it does not establish runtime correctness. The deletion timeout test exercises the helper directly, so it does not by itself protect the inventory/purge call sites. Public description and discussion inspected; no attached images to inspect.

Minimalism 9/10; elegance 9/10; correctness 8/10 due to the two concrete retry/lock-lifetime issues below. No approval or request-changes verdict is submitted.

Comment thread crates/buzz-db/src/store/event.rs
Comment thread crates/buzz-db/src/store/deletion.rs Outdated
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 20, 2026
@TheSentinel454

Copy link
Copy Markdown
Contributor Author

@wpfleger96 thanks. Both blockers are fixed in a7065c367:

  • Audit entries on 57014. The relay's audit worker now treats 57014 like 55P03: the append was rolled back, so the entry is kept and retried with the same backoff. Admission refusal stays terminal, and the audit pool keeps the cap. New regression audit_worker_retries_statement_timeout_until_original_entry_is_appended_once drives the production worker with lock_timeout disabled and a 100ms statement cap, holds the audit-chain lock past it, releases it, and asserts the original entry is appended exactly once.
  • Replica cancel rerunning on the writer. All six branches (feed mentions, needs-action and activity; thread replies; channel window; relay membership) now return a cancelled statement instead of falling back. Other replica failures still fall back. replica_statement_cancel_propagates_without_writer_rerun now covers all six: with the replica locked past its cap, each returns 57014, not the writer's copy of the row.

Optional notes:

  • The deletion lift test now runs the real inventory_schema and purge_postgres under a 500ms writer cap behind a held events lock (deletion_inventory_and_purge_outlive_the_writer_statement_timeout). Removing either lift call fails it. The description's test bullet is updated.
  • Inventory now counts each table in its own transaction, so there's no whole-inventory snapshot to describe.
  • The description now covers the three extra round trips per generic read, with the local E2E numbers. It also covers the audit and replica-fallback changes, and has a squash note to drop the first commit's "raises itself to the 300s ceiling" line.
  • buzz-review-completed is left off on purpose while the PR is a draft.

@github-actions github-actions Bot added codex-security-review-current The posted Codex security review matches its recorded range. and removed codex-security-review-current The posted Codex security review matches its recorded range. labels Oct 20, 2026
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 20, 2026

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Star Lord’s automated source review via Wes’s GitHub account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/operator recovery) reviews.

No new actionable findings in this follow-up. The per-table inventory transactions resolve the earlier lock-lifetime finding. The revised audit worker retains and retries 57014 failures, the six replica paths propagate cancellation without a writer rerun, and the mention-kind bound matches the production writer. The regression tests now exercise the actual inventory/purge and audit-worker paths. The previously reported ACP timeout-retry debt remains explicitly deferred in the description; this review does not declare it fixed.

Operational tradeoff: SQLSTATE 57014 does not distinguish expensive work from a replica-local lock wait. Under the stated no-rerun policy, even a membership point lookup can therefore fail while the writer could answer. We retained that as policy context, not a new change request.

Reviewed head 03202cab3d4dc2ec4e9de8f79c91677fd8de5753 against base/merge-base dc4cdc4990877d400a435b481a416a73f17247bb, including the endpoint delta from previously reviewed 9cb6638b1ad9d28f2a0819a6c54f19d2e9ad5ed0. Source-only: no tests, SQL, EXPLAIN, application execution, or independent CI validation. Production latency, replica behavior, and historical out-of-band mention-row integrity remain unverified. Public description and discussion inspected; no attached images. Minimalism/elegance/correctness: 9/9/9 for the reviewed follow-up. This is a non-blocking COMMENT, not approval or merge authorization.

@wpfleger96 wpfleger96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 Automated review of 03202cab3d4dc2ec4e9de8f79c91677fd8de5753 (base dc4cdc4990877d400a435b481a416a73f17247bb): no blocking issues. Both blockers from the last review are fixed, and so is the deletion test.

  • Audit entries on a cancelled statement. The worker now retries 57014 with the same backoff as 55P03, and admission refusal, pool, constraint and hash errors stay terminal (is_retryable_append_failure, crates/buzz-relay/src/state.rs:2450). audit_worker_retries_statement_timeout_until_original_entry_is_appended_once goes through the real worker with a 100ms cap and lock timeout off, holds the audit-chain lock through a retry, and checks for exactly one row. Dropping the 57014 arm makes it fail with "audit worker never retried".
  • Replica cancels rerunning on the writer. All six reads now return the cancel and keep the writer fallback for other errors. replica_statement_cancel_propagates_without_writer_rerun uses separate writer and replica databases, so a writer rerun returns rows and fails the test. Removing the guard in query_feed_mentions_routed, get_thread_replies or is_relay_member fails it with that path named.
  • Deletion lift. Inventory now lifts and counts each table in its own transaction, so locks and snapshots are released per table. deletion_inventory_and_purge_outlive_the_writer_statement_timeout runs the real inventory_schema and purge_postgres behind a held events lock past a 500ms cap. Removing either lift fails it with 57014.

All new and changed tests passed by name at this head in the hosted unit job and Postgres job.

Optional

  • Two replica fallbacks still rerun a cancelled read on the writer: get_thread_window_with_session (crates/buzz-db/src/store/thread_window.rs:251-260) and ReadSession::thread_window_aux (thread_window.rs:448-460, which also switches the session to the writer for good). Both predate this PR and both sides are already capped at 4s by SET LOCAL, so they don't bring back the 20s-to-120s escalation, and the description correctly scopes the fix to the six reads. A follow-up with the same guard and a test would make the rule hold for every routed read.
  • event_mentions.event_kind is nullable (migrations/0001_initial_schema.sql:292), and the kinds pushdown is the first reader that depends on it being set. The description already says the production NULL count wasn't checked. Worth running SELECT count(*) FROM event_mentions WHERE event_kind IS NULL before merge.
  • The first commit's body still says the purge "raises itself to the 300s ceiling". The lift is to 0 now, so drop that line if the squash keeps commit bodies.
  • .env.example:73 leaves # Generic event alone on its line.
  • The description doesn't include buzz-review-completed.

@github-actions github-actions Bot added codex-security-review-current The posted Codex security review matches its recorded range. and removed codex-security-review-current The posted Codex security review matches its recorded range. labels Oct 20, 2026
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 20, 2026
@TheSentinel454
TheSentinel454 force-pushed the elrond/db-statement-deadlines branch from dc3e2b4 to e9495a7 Compare October 20, 2026 22:21
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 20, 2026
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 20, 2026

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Star Lord’s automated source review via Wes’s GitHub account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/operator behavior) reviews.

No new actionable code findings in this follow-up. The two thread-window cancellation guards propagate SQLSTATE 57014 without retrying on the writer. The HTTP bridge ends the request with a retryable 503 before emitting authoritative page bounds; the cancelled auxiliary transaction is dropped when that request ends. The new regression test exercises both production database paths. The earlier four feature patches are preserved across the rebase apart from upstream context; this review concentrates on the added guards, test and integration.

Optional description cleanup: remove the “Startup d_tag backfill” row and its “Identified debt” bullet. Upstream #8468 (c99872f), already in this base, removed that startup step. Those two statements now describe code absent from the reviewed tree. No code change is requested for this note.

The previously reported ACP timeout re-subscribe debt remains explicitly deferred; this review does not declare it fixed. The existing no-rerun policy still treats replica-local cancellations the same as expensive work.

Reviewed head e9495a7c478ad2b81758ca7169842efb616203e8 against base 93114b4279a619613a6df8ef740472216e1d4cfb, comparing the feature series with previously reviewed 03202cab3d4dc2ec4e9de8f79c91677fd8de5753. Source-only: no tests, SQL, apps, latency measurements or independent CI verification. Database cancellation, deployed client retry/focus behavior and production performance remain unverified. Public description/discussion inspected; no attached images. Minimalism/elegance/correctness: 9/9/9 for the follow-up. Non-blocking COMMENT only; no approval or merge authorization.

@wpfleger96 wpfleger96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 Automated review of e9495a7c478ad2b81758ca7169842efb616203e8 (base 93114b4279a619613a6df8ef740472216e1d4cfb): no blocking issues. Since the last review (03202cab), this head adds the thread-window cancel guards and rebases onto current main. The other four commits are patch-identical apart from one line of .env.example context from main.

  • Thread-window cancels. get_thread_window_with_session (crates/buzz-db/src/store/thread_window.rs:256-259) and ReadSession::thread_window_aux (thread_window.rs:461) now return 57014 instead of rerunning on the writer, and the aux path keeps its replica session. Other replica errors still fall back. thread_window_statement_cancel_propagates_without_writer_rerun covers each guard on its own: with only the window guard removed it fails at runtime/tests.rs:3958, and with only the aux guard removed it fails at :3997. Both mutations were run against a disposable Postgres and failed on those assertions. The test also passed 3 of 3 runs alongside the two existing replica-cancel tests, so no flake turned up.
  • Rebase. Main added no routed replica read in dc4cdc4990..93114b42. Every route that falls back to the writer still catches 57014 first, including the relay-membership read that moved (store/relay_members.rs:1229-1244). Main's writer changes (partition catch-all advance with its own 5s cap, the NIP-43 reconcile moved off startup, the NIP-43 list written in the membership transaction) add nothing that needs more than the 120s cap. Main only changed tests in deletion.rs, so inventory and purge still run entirely under lift_statement_timeout.

The full buzz-db Postgres suite passed locally at this head (566/566). Hosted CI is green, including PostgreSQL Tests and Security.

Optional

  • The "Startup d_tag backfill" row in the long-running writer paths table and the matching "Identified debt" bullet describe code #8468 removed. backfill_d_tags no longer exists at this head.
  • .env.example:61 is a 104-character line after the "0 disables the lock and idle timeouts" edit; the rest of the block wraps at 80.
  • The first commit's body still says the deletion transaction "raises itself to the 300s ceiling". The lift is to 0, so drop that line if the squash keeps commit bodies.
  • The description doesn't include buzz-review-completed.

Elrond added 6 commits October 21, 2026 14:41
An agent #p + #h + since REQ ran for 45+ minutes on bb-public: the
event_mentions side of the join was unbounded (every mention of the
pubkey ever) and nothing cancelled it, because the writer
statement_timeout defaulted to 0 and only #e reads had a deadline.

- #p reads/counts join event_mentions on the full event key
  (created_at too) and repeat since/until on m.event_created_at, so the
  outer scan is a range on idx_event_mentions_pubkey_created.
- Every generic event read and COUNT runs under the 20s
  transaction-local deadline that #e reads already had; read-replica
  sessions default to 20s.
- Writer sessions default to statement_timeout 120s; 0 or values above
  300s install 300s, so no writer session is unbounded. Migrations and
  schema destruction keep their own lifted session.

Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
…und test

Community-deletion inventory counts and the purge transaction run
whole-tenant statements that scale with the tenant. Under the always-on
writer cap a large tenant's purge would be cancelled (57014, transient),
retried from scratch, and eventually blocked. Like migrations and schema
destruction these paths are operator-approved and fenced, so they now
disable statement_timeout transaction-locally; lock and idle timeouts
still bound them.

The #p mention-bound test now puts since/until exactly on mentioned
rows' seconds and adds a same-second sibling after the cursor id, so an
off-by-one on the mention-side bounds fails it.

Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
…writer

With the writer statement cap always on, two paths turned a 57014 into
the wrong outcome:

- The audit worker retried only lock_timeout (55P03). Under
  BUZZ_DB_LOCK_TIMEOUT_MS=0 (or >= the statement cap) the advisory-lock
  wait is cancelled by statement_timeout instead and the entry was
  dropped. 57014 is now retried with the same backoff; admission refusal
  stays terminal.
- Six replica reads with their own SQL (feed mentions/needs-action/
  activity, thread replies, channel window, relay membership) fell back
  to the writer on any replica error, so a 20s replica cancel re-ran the
  same expensive read on the writer. They now return the cancel, like
  the generic routed reads and ReadSession already do. Recovery
  conflicts still fall back.

Community-deletion inventory now counts each table in its own lifted
transaction, releasing each table's lock and snapshot as soon as its
count finishes. The lift test now drives the real inventory_schema and
purge_postgres under a 500ms writer cap behind a held events lock, so
removing either lift call fails it.

The #p mention join also repeats the kinds filter on event_mentions
(every mention insert copies the event kind), letting the planner use
idx_event_mentions_pubkey_kind_created. A clamped
BUZZ_DB_STATEMENT_TIMEOUT_MS (0 or above the 5-minute maximum) now logs
a warning at pool build.

Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Restore the SHOW statement_timeout = '0' oracle inside a lifted
transaction so a bounded lift (e.g. '300000') goes red, and stop the
audit retry warning from claiming every 57014 was a lock wait.

Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
get_thread_window_with_session and ReadSession::thread_window_aux re-ran
a cancelled replica statement (57014) on the writer, and the aux path
also degraded the session to the writer for the rest of the request.
Both now return the cancel, matching the other routed reads; other
replica failures still fall back.

thread_window_statement_cancel_propagates_without_writer_rerun locks
the replica's events table and cancels the parked window read, then
times out an aux scan on a proved replica session. The writer copy is
unlocked, so dropping either guard makes the test pass a writer result
and fail.

Also rewrap the .env.example statement_timeout comment.

Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
@TheSentinel454
TheSentinel454 force-pushed the elrond/db-statement-deadlines branch from e9495a7 to e004959 Compare October 21, 2026 21:41
@github-actions github-actions Bot added codex-security-review-current The posted Codex security review matches its recorded range. and removed codex-security-review-current The posted Codex security review matches its recorded range. labels Oct 21, 2026

@wpfleger96 wpfleger96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 approving at e0049592. the #p bound and the always-on writer cap both look right to me. a few nonblocking notes:

  • verify_postgres_logically_deleted (store/deletion.rs) still runs under the 120s writer cap. it runs EXISTS … WHERE community_id = $1 on every tenant table right after the purge, when the community_id index entries are still dead tuples the first scan has to visit, so on the largest tenants it could outrun 120s. retries get through eventually, but it's the same whole-tenant pattern inventory and purge exempt. could it call lift_statement_timeout(&mut tx) too?
  • build_mentions_query and build_needs_action_query (store/feed.rs) still join event_mentions on (community_id, event_id) only. they sort by m.event_created_at with a LIMIT so they can't run away the same way, but each joined row still probes every events partition. adding AND e.created_at = m.event_created_at would give them the same pruning, one line each.
  • the replica cancel arms say a cancel "would be just as slow on the writer". that doesn't hold when the replica is waiting on a lock only it holds (a replayed ACCESS EXCLUSIVE from primary DDL, which is also how the tests produce 57014). then the user gets an error where the writer would have answered quickly. I'd keep the change since replica reads had no limit before, just worth watching the statement_cancelled route reason after rollout.
  • every REQ history read now holds its writer connection for three extra round trips, so worth keeping an eye on writer pool-acquire wait after deploy.

@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 21, 2026
…eplica-cancel trade-off

- verify_postgres_logically_deleted runs one whole-tenant EXISTS per table
  right after the purge; lift its statement timeout like inventory and purge,
  and extend the deletion lift test through it.
- Feed mentions and needs-action join event_mentions on the event's
  created_at too, so each probe prunes to one events partition (same
  invariant as the #p read).
- Replace the copied "would be just as slow on the writer" comments with one
  accurate rationale on DbError::is_statement_cancelled, including the
  replica-only lock case.

Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 21, 2026
@TheSentinel454
TheSentinel454 force-pushed the elrond/db-statement-deadlines branch from f43696d to 0fbd059 Compare October 21, 2026 22:06
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 21, 2026
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 21, 2026

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Star Lord’s automated source review via Wes’s GitHub account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/operator behavior) reviews.

No new actionable findings in this follow-up. The feed timestamp joins preserve results for supported mention writers while supplying the partition key. Logical-deletion verification now receives the same transaction-local timeout exemption as inventory and purge; catalog, lease and fence checks remain, and the regression test exercises the production verification method. The clarified replica-cancellation tradeoff matches the existing no-writer-rerun policy.

The previously reported ACP timeout re-subscribe debt remains explicitly deferred; this review does not declare it fixed. The exemption intentionally leaves statement execution unbounded in these operator-approved deletion transactions; configured lock and idle timeouts address their respective waits, not total execution time.

Reviewed head 0fbd0593b09a95212e93b9988936b7032e5d5fc8 against base d97f6b4c6ed616be0c3f1eb0e13d1eaa9e338a66 (merge-base 93114b4279a619613a6df8ef740472216e1d4cfb), concentrating on the actual endpoint delta from previously reviewed e9495a7c478ad2b81758ca7169842efb616203e8. Source-only: no tests, SQL/EXPLAIN, app execution, CI validation or performance measurements. Partition-pruning plans, production latency/lock behavior, historical out-of-band mention integrity and native error/retry focus remain unverified. Public description and discussion inspected; no attached images. Minimalism/elegance/correctness: 9/9/9 for this follow-up. Non-blocking COMMENT only; no approval or merge authorization.

@TheSentinel454
TheSentinel454 merged commit 2f2f6e2 into main Oct 21, 2026
84 checks passed
@TheSentinel454
TheSentinel454 deleted the elrond/db-statement-deadlines branch October 21, 2026 23:35
dr0uleau added a commit that referenced this pull request Oct 22, 2026
…pshots

* origin/main:
  fix(db): bound every statement and the #p mention scan (#8461)

Signed-off-by: Daniel Rouleau <drouleau@squareup.com>

This branch was successfully deployed

1 active deployment
codex-review — 0fbd0593 Deployed Oct 21, 2026 by TheSentinel454 via Run Codex Security Review #9412
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex-security-review-current The posted Codex security review matches its recorded range.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants