Skip to content

fix(relay): treat empty filter sets as match-nothing in SQL - #9342

Merged
brow merged 4 commits into
mainfrom
relay/a-tag-count-empty-sets
Nov 12, 2026
Merged

brow merged 4 commits into
mainfrom
relay/a-tag-count-empty-sets

Conversation

@brow

@brow brow commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator

Fix for a COUNT regression introduced by #9339 (now on main), found in its review.

With #9339, a filter with a nonempty #a takes the exact SQL COUNT path. But filter_to_query_params dropped an empty authors, ids or #e list and never represented an empty #p, #d or unpushed tag set. So {"kinds":[7], "#a":[A], "authors":[]} counted every reaction to A, while REQ (which post-filters) correctly returned nothing. Before #9339 that COUNT returned 0. The same gap already existed for filters without #a.

Any empty authors, ids or tag set now maps to the existing kinds: [] match-nothing sentinel, which every read (REQ, COUNT, /query, /count) answers without a query. An empty string value such as "#d": [""] is still a real constraint.

Why nothing gets worse: only filters that provably match nothing change, and they now skip the database instead of reading or counting.

Validation

Added tests:

  • Unit table over empty authors, ids, #e, #p, #d, #h, #k and #t, each with and without #a, plus "#d": [""]. It fails with the sentinel disabled.
  • Postgres table: WebSocket and HTTP COUNT are 0, and REQ and /query are empty, for each empty set beside a matching #a and for {kinds:[7], authors:[]}. The first commit contains only the tests.

Existing workspace unit tests pass (pre-push). The Postgres lanes run separately on an isolated cluster.

🤖 Generated with Claude Code

@brow

brow commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-12T02:59:01.640902Z d5ccf65 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: b5d4cf24c2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Base automatically changed from relay/push-a-tag-filters to main November 11, 2026 23:41
square-tomb and others added 2 commits November 11, 2026 15:42
An empty authors, ids or tag set matches nothing (NIP-01). Cover it in
the query conversion and over WebSocket and HTTP, beside a nonempty #a
and without one.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: Tom Brow <[email protected]>
filter_to_query_params dropped an empty authors, ids or #e list and
never represented an empty #p, #d or unpushed tag set. Now that #a
filters take the exact COUNT path, {kinds:[7], #a:[A], authors:[]}
counted the #a matches while REQ returned nothing. Map any empty set to
the existing kinds: [] sentinel, which every read answers without a
query.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: Tom Brow <[email protected]>
@brow
brow force-pushed the relay/a-tag-count-empty-sets branch from b5d4cf2 to 966b622 Compare November 11, 2026 23:50
square-tomb and others added 2 commits November 11, 2026 16:34
just test-unit picks buzz-relay req tests by exact name, so the new
table ran in no CI lane.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: Tom Brow <[email protected]>
@brow
brow marked this pull request as ready for review November 12, 2026 02:56
@brow
brow requested a review from a team as a code owner November 12, 2026 02:56
@brow
brow enabled auto-merge (squash) November 12, 2026 02:57
@brow
brow deployed to codex-review November 12, 2026 02:57 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Nov 12, 2026
@github-actions

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: 4c60be6a1e8dc2c7839f7c0294391fb93941ed9f...d5ccf659dd389a26fd05c9238af1e519b8a4f804
  • 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 in the authorized PR range. The empty-filter handling preserves existing community and authorization boundaries.

Findings

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

Notes

  • Review used read-only inspection of the diff, callers, database handling, and tests. Builds and tests were not executed.

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

@kazemek21 kazemek21 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Kyle reviewed this PR and the Nexus review-tracker notes and approved with no comments.

— Posted by Codex (gpt-6.1-sol) on behalf of kylez

@brow
brow merged commit e715fbb into main Nov 12, 2026
117 checks passed
@brow
brow deleted the relay/a-tag-count-empty-sets branch November 12, 2026 14:21
ravarora2 pushed a commit that referenced this pull request Nov 12, 2026
…small

* origin/main:
  fix(db): seek replaceable heads by channel (#9147)
  buzz-db: check events created_at floor in app writers (phase 1) (#8325)
  fix(mobile): keep a thread tail behind the keyboard unread (#9295)
  fix(mobile): restore header cues and per-channel identity check for #9369 (#9370)
  fix(relay): treat empty filter sets as match-nothing in SQL (#9342)
  buzz-cli: add --before and --before-id to messages search (#9382)
  feat(relay): agents cannot write in DMs except with their owner (#9371)
  fix(relay): bound each cross-node delivery attempt (#9305)
  feat(mobile): add iOS Home Screen shortcuts for New chat, New DM and the Bestie (#9360)
  perf(mobile): stop downloading whole videos for posterless stills (#9367)
  Enforce single-community relay mode (#9149)
  fix(relay): match #a tag filters in SQL before LIMIT (#9339)
  Add participant avatars to mobile conversation headers (#9369)
  fix(mobile): stop asking for an API token when the channel list is refused (#9327)
  chore(release): release Buzz Relay version 0.3.0 (#9362)
  Fix gallery video selection, thumbnails, and swipe dismissal (#9356)

Signed-off-by: dessert <7d616e7e143e5d408159614dccff1371350a1c1bd36c8de838a45ece5aca0aca@buzz.block.builderlab.xyz>
matt2e added a commit that referenced this pull request Nov 14, 2026
…ests a test-lane acquire budget (#9442)

## Summary
Two Rust tests failed on loaded CI runners because their timing budget
came from the wall clock or from a production timeout, not from the
behavior they assert. Each fix changes only test code or `cfg(test)`
code.

Commits (cherry-picked unchanged from a local flake-fix series):
- `test(cli): freeze the test clock in the assertion refresh tests`
- `test(db): give the pool release Postgres tests a test-lane acquire
budget`

### Root cause and fix per test


**`federated_identity::tests::short_and_long_assertions_refresh_at_the_earlier_point`**
and its twin in `desktop_provider` (`crates/buzz-cli`): failed in CI
with `assertion left == right failed: lifetime 300`, left 2, right 1, at
`federated_identity.rs:634`.

- CI sighting, 11 November 2026, "Rust / Unit Tests", run 43122921918
attempt 1 (on #9342; the rerun passed):
-
https://github.com/block/buzz/actions/runs/43122921918/job/130868244938
- Root cause: under `cfg(test)`, `cached_assertion::unix_now()` was the
wall clock plus a thread-local skew. The mock stamps `exp = unix_now() +
lifetime`, then `CachedAssertion::get` computes `refresh_at(unix_now(),
exp)` after the HTTP round trip, and the next call checks `unix_now() <
refresh`. The test needed those reads to agree to the second. When the
wall clock crossed a second boundary between them, the refresh point
moved 1 s earlier, `advance(refresh - 1)` landed exactly on it, and the
assertion was minted one step early.
- Fix: a thread-local `FROZEN` base beside `SKEW`, and
`test_clock::freeze(at)` to pin it. Under `cfg(test)`, `unix_now()`
returns `FROZEN.unwrap_or(real_now) + SKEW`, so `advance` and its
existing callers in `client.rs` behave as before. Both tests freeze the
clock at the top of their loop body. The mock adapter runs on the same
current-thread runtime, so `exp`, `refresh_at` and the check share one
"now". Production code paths are unchanged.
- Verification: each test run 1,440 times, 24 in parallel, before and
after. `federated_identity`: 4 failures before, 0 after.
`desktop_provider`: 1 failure before, 0 after.
- Not seen in CI: the `desktop_provider` twin. It passed in the job
above and is fixed here because it has the same race and failed locally.


**`runtime::postgres_tests::pool_release_postgres_tests::silence_after_release_hook_does_not_strand_sqlx_ping`**
(`crates/buzz-db`): panicked at `pool_release_postgres_tests.rs:204:10`
with ``called `Result::unwrap()` on an `Err` value: PoolTimedOut`` after
0.85 s.

- CI sighting, 10 November 2026, "PostgreSQL Domain / PostgreSQL Tests",
run 42851548618 attempt 1 (on #8317; the rerun passed):
-
https://github.com/block/buzz/actions/runs/42851548618/job/129969964659
- Root cause: the test built its pool from the production
`Db::read_pool_options`, whose 150 ms `READER_ACQUIRE_TIMEOUT` also
bounded `connect_with`'s eager first dial (and `fresh_query`'s). On a
busy runner the cold dial through `PausingProxy`, SCRAM and the reader
`after_connect` hook overran it, so the test panicked at the initial
`connect_with(...).unwrap()` before the release-hook recovery path it
asserts ever ran. Production is not exposed: the relay dials the reader
pool lazily, so that budget only governs request-time acquires, which
fall back to the writer by design.
- Fix: a 30 s `TEST_LANE_ACQUIRE_TIMEOUT` on both pool builders in this
file: the reader test's `PoolOptions`, and `writer_pool`'s `DbConfig`,
whose eagerly dialed 3 s default is the same exposure #9273 fixed for
buzz-admin. `test_before_acquire(false)`, `after_connect` and
`after_release` stay as in production, and recovery still completes at
sqlx's 5 s return deadline inside the test's 10 s `await_capacity`
budget. No assertions change.
- Verification, against a local Postgres: 10 rounds of
`pool_release_postgres_tests --ignored --test-threads=6` per binary.
Before, 2 of 10 rounds failed at `:204:10` with `PoolTimedOut`, the CI
signature. After, 0 of 10. Under 28 CPU busy loops, with before and
after runs interleaved: 6 of 10 failed before, 0 of 10 after.
- Not done here: `reader_pool_postgres_tests.rs` already carries its own
5 s test budget on `read_pool_options` for the same reason. This PR does
not unify the two constants.

One sighting of each failure was found in the CI runs from 7 to 12
November 2026 that were searched.

### Related issue
N/A (flake fixes). No existing issue was found, and no open PR touches
these tests or the four files changed here.

### Testing
Human testing is still required. This PR does not carry
`buzz-review-completed`; the author has not yet run these tests
themselves on this branch.

The repetition counts above were measured when the fixes were written,
before this branch was split out. They were not repeated on this branch.

#### Local checks on this branch
The branch is one commit behind `main`; that commit changes only mobile
and docs files, none of them touched here.

Ran and passed at push time, on a checkout of this branch's head
(`42b3faccc`): the organization push check, `branch-skew`,
`push-head-scope`, `file-size-check` and git-lfs.

Did not run:
- The `rust-tests` and `desktop-tauri-checks` pre-push lanes. They were
excluded from the push (`LEFTHOOK_EXCLUDE`) because the machine ran out
of disk part-way through them on an earlier attempt. That attempt did
not complete, so it proves nothing either way about this branch.
- The desktop and mobile pre-push lanes (`desktop-check`,
`desktop-typecheck`, `desktop-test`, `mobile-checks`). This diff touches
no desktop or mobile file.
- `cargo fmt --check`, `cargo clippy`, `just ci`, and the PostgreSQL
lane.

CI's Unit Tests and PostgreSQL Tests jobs are the gate for this branch.
A green run does not prove the flakes are gone: both failures need a
loaded runner.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Signed-off-by: mtoohey <[email protected]>
Signed-off-by: Matt Toohey <[email protected]>
Co-authored-by: Claude Fable 5.1 <[email protected]>

This branch was successfully deployed

1 active deployment
codex-review — d5ccf659 Deployed Nov 12, 2026 by brow via Run Codex Security Review #14548
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