Skip to content

refactor(relay): remove database usage-metric queries - #9040

Merged
TheSentinel454 merged 7 commits into
mainfrom
elrond/remove-relay-usage-queries
Nov 5, 2026
Merged

TheSentinel454 merged 7 commits into
mainfrom
elrond/remove-relay-usage-queries

Conversation

@TheSentinel454

@TheSentinel454 TheSentinel454 commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The relay no longer runs database aggregate queries to emit usage gauges. Fleet and per-community counts of users, channels, relay members, workflows, git repos, communities, messages, and active users/channels are no longer relay metrics, and neither are the snapshot health gauges that came with them.

Removed

  • Gauges: buzz_total_users, buzz_total_channels, buzz_total_relay_members, buzz_total_workflows, buzz_total_git_repos, buzz_total_active_users, buzz_total_messages, buzz_total_active_channels, buzz_communities_estimated / buzz_communities_total, and every DB-derived buzz_community_* series (users, channels, messages, relay members, workflows, git repos, active users, active channels).
  • Per-community mode: BUZZ_USAGE_METRICS_PER_COMMUNITY=all and everything it fed: the per-community in-memory gauges, buzz_community_storage_bytes / buzz_community_storage_objects, buzz_storage_unmapped_community_bytes, the community → host map query, and the emission-scope bookkeeping that cleared series for renamed or removed communities. No deployment set it. If one still does, the relay ignores it.
  • Health signals: buzz_usage_snapshot_available, buzz_usage_snapshot_age_seconds, buzz_usage_query_skipped_total, and the usage_fleet_* paths of buzz_db_route_decision.
  • Code and config:
    • buzz-db/src/store/usage.rs, including its aggregate queries.
    • The relay's FleetUsageSchedule and its cadence/deadline/retry machinery.
    • The replica-only telemetry route (Db::route_usage_read).
    • BUZZ_USAGE_METRICS_REPLICA_MAX_AGE_MS and DbConfig::usage_metrics_replica_max_age_ms. If a deployment still sets the env var, the relay now ignores it; startup doesn't fail.
  • Reader maintenance pool operation: ReaderOperation::Maintenance and its close_on_drop release path existed only for those telemetry reads, so they're removed too. The pool-acquire contract drops from twelve to eleven valid pairs, which lowers the per-pod series ceiling from 216 to 198.
  • Dead code: Db::active_community_hosts had no callers.

Kept

  • Leader: the usage poller's leader lock, plus the leader's invite reaping and fleet storage-snapshot gauges. The session advisory lock (SessionLeader, Db::try_lock_session_leader) moves to buzz-db/src/store/leader_lock.rs, since storage-usage delivery uses it too and it has nothing to do with usage queries.
  • In-memory gauges: pod-local connection, subscription, and online-user gauges. They read process state, not the database.
  • Writer maintenance pool operation: WriterOperation::Maintenance stays. Dozens of writer-side maintenance paths across buzz-db use it (reapers, retention, the leader lock). The reader variant only ever served the telemetry reads.
  • Storage-usage delivery: unchanged. It reads the storage snapshot for billing, not for metrics.

docs/usage-metrics.md, docs/storage-accounting.md, .env.example, and the Helm chart README are updated to match.

What got simpler

This is net deletion (~3.6k lines). One telemetry subsystem is gone: two collection families with independent cadences, availability and age semantics, relay-side deadlines, a dedicated replica routing policy, two config knobs, a per-community emission mode, and a pool-operation variant. Leader election now does only leader work, and it lives in its own module.

Operational note

Dashboards and monitors that read the removed series will go to no data once this rolls out.

Testing

  • cargo fmt --check, workspace cargo clippy --all-targets -D warnings.
  • buzz-db lib, observability_source, and doc tests.
  • buzz-relay lib tests.
  • The Postgres lane for buzz-db (lib + tests) and buzz-relay (lib).

All of these run on a remote workstation at the PR head.

🤖 Generated with Claude Code

Comment thread crates/buzz-db/src/store/usage.rs Resolved
@TheSentinel454
TheSentinel454 marked this pull request as ready for review November 4, 2026 03:59
@TheSentinel454
TheSentinel454 requested a review from a team as a code owner November 4, 2026 03:59
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Nov 4, 2026
@github-actions

github-actions Bot commented Nov 4, 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: af54258f19ac32667c027f889652bdeb479c8e13...83e93a3d36d8ff7cf7777e60dfcdd553b8fafd83
  • 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 regressions found 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, as requested.

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

@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.

🤖 No blocking findings at 18220712.

I compared what this PR keeps against main (069cb9a0), and it all holds:

  • SessionLeader and try_lock_session_leader moved to store/leader_lock.rs unchanged: same pg_try_advisory_lock($1), writer Maintenance acquire, detach(), and 5-second liveness ping, with the same lock key. The observability_source no-bypass check now covers the new file.
  • The usage tick still emits the three pod-local gauges on every pod, then elects the leader, reaps invites with the 30-day cutoff, and runs the storage tick. Only the fleet DB queries are gone. storage_usage_delivery.rs and store/storage_accounting.rs are byte-identical to main.
  • Nothing reads BUZZ_USAGE_METRICS_REPLICA_MAX_AGE_MS or BUZZ_USAGE_METRICS_PER_COMMUNITY anymore, so a junk value no longer fails startup. No retired series, env var, route_usage_read, or ReaderOperation::Maintenance is left anywhere outside negative test assertions.
  • The 11-pair / 198-series pool-acquire contract agrees across the enum, pair list, constants, tests, and the Helm README.

I also ran the head live: two relays against a throwaway Postgres/Redis/S3, with both removed env vars set (one to a junk value). Both started. /metrics had none of the removed families. The connection, subscription, and online-user gauges went 0→1→0 across an authenticated client's REQ/CLOSE/disconnect. Fleet storage gauges appeared from a worker snapshot without a restart. Invite reaping deleted a 31-day-expired invite and kept a 29-day one. Exactly one relay held the advisory lock, and after SIGTERM to the leader the other took it over and emitted the same storage totals.

Two small things:

  1. crates/buzz-relay/src/storage_sweep.rs:143-146: emit_storage_metrics's mode parameter and its Disabled early return can't fire. Its only production caller (run_storage_metrics_tick, :85) already returns early on Disabled, and every test passes Snapshot. The parameter can go.
  2. docs/storage-accounting.md:54: the rewritten paragraph has one ~110-column line among hard-wrapped neighbors; worth rewrapping.

@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.

🤖 Two small nits inline, neither blocking. The removal itself looks complete: nothing left in the tree references the removed gauges, env vars, route_usage_read, ReaderOperation::Maintenance, or the usage store module, and the only conflict with main is the pub use store::{...} list in crates/buzz-db/src/lib.rs (main added community_auth), which needs a rebase before CI runs again.

Comment thread crates/buzz-relay/src/storage_sweep.rs Resolved
Comment thread docs/storage-accounting.md Resolved
TheSentinel454 and others added 7 commits November 4, 2026 17:46
The relay no longer runs aggregate usage queries to emit fleet and
per-community gauges (users, channels, relay members, workflows, git repos,
communities, messages, active users/channels) or their snapshot health
gauges. The replica-only telemetry route, its freshness budget
(BUZZ_USAGE_METRICS_REPLICA_MAX_AGE_MS), and the reader maintenance pool
operation that existed only for it are removed with them.

The usage poller keeps its leader lock, invite reaping, storage snapshot
gauges, and pod-local in-memory gauges.

Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: Luke Tornquist <[email protected]>
Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: Luke Tornquist <[email protected]>
Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: Luke Tornquist <[email protected]>
No deployment sets BUZZ_USAGE_METRICS_PER_COMMUNITY, so the per-community
storage gauges, the community host map, and the emission-scope plumbing
are dead weight. Remove them; the relay now emits fleet storage totals
and pod-local in-memory gauges only.

With the host map gone, store/usage.rs held only the session advisory
leader lock. Move it to store/leader_lock.rs and delete usage.rs.

Signed-off-by: Luke Tornquist <[email protected]>
Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
The operation-attribution bypass scan still referenced the deleted
usage.rs source.

Signed-off-by: Luke Tornquist <[email protected]>
Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
The host map was its last user.

Signed-off-by: Luke Tornquist <[email protected]>
Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Its only production caller already returns early when storage metrics
are disabled. Also rewrap an over-long line in storage-accounting.md.

Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: Luke Tornquist <[email protected]>
@TheSentinel454
TheSentinel454 force-pushed the elrond/remove-relay-usage-queries branch from 1822071 to 83e93a3 Compare November 4, 2026 22:47
@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 Nov 4, 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.

🤖 Both nits are fixed in 83e93a3, and the rebase onto main only changes the pub use store::{...} list, where it keeps community_auth. LGTM.

@TheSentinel454
TheSentinel454 merged commit 506a588 into main Nov 5, 2026
93 checks passed
@TheSentinel454
TheSentinel454 deleted the elrond/remove-relay-usage-queries branch November 5, 2026 03:00
matt2e added a commit that referenced this pull request Nov 5, 2026
#9097)

## Summary

`StorageAccountingLeader` holds a session-level advisory lock on a
detached connection. Dropping it only closes the socket, and PostgreSQL
frees the lock when the backend exits, which can be after the next lock
attempt. `release(self)` now unlocks on the leader's own session before
closing it. The relay storage-sweep test releases its worker between
iterations instead of dropping it, which fixes the
`worker_publication_activates_reader_without_restart_or_mode_change`
flake. The follow-up commit makes `release` report success only when the
unlock actually ran on the session that held the lease.

### fix(db): unlock the storage-accounting lease before its session
closes

#### Problem

The PostgreSQL lane's
`storage_sweep::postgres_tests::worker_publication_activates_reader_without_restart_or_mode_change`
failed with "called `Option::unwrap()` on a `None` value" at
crates/buzz-relay/src/storage_sweep/postgres_tests.rs:66:74 after 5.53
s, on the unrelated branch elrond/cli-credential-chain:
-
https://github.com/block/buzz/actions/runs/41730818977/job/126253029211

#### Cause

Line 66 is `db.try_lock_storage_accounting().await.unwrap().unwrap()`
inside the loop over three metrics modes. `Ok(None)` means
`pg_try_advisory_lock` returned false. A passing run takes about 15 s,
roughly 5 s per iteration, so the failure is the start of iteration 2.
Each test process gets its own database and advisory locks are
per-database, so the holder is iteration 1's own worker.

`StorageAccountingLeader` holds a session-level advisory lock on a
detached connection. The test ended each iteration with `drop(worker)`,
which only closes the socket. PostgreSQL frees session locks during
backend exit, after it notices EOF, so the lock can outlive the drop on
a loaded server. Iteration 2's lock attempt raced that exit. The
db-crate test `lost_lock_session_cannot_overwrite_successor_snapshot`
already polls for this release. The relay test didn't.

#### Fix

Add `StorageAccountingLeader::release(self)`. It runs
`pg_advisory_unlock` on the leader's own session and then closes it, so
the lease is free when it returns. This follows the unlock-then-close
pattern in `migration.rs`. The relay test now releases the worker at the
end of each iteration instead of dropping it. No assertions, timeouts or
retries change.

buzz-admin's `storage-snapshot` is left as is. It runs once per pod, and
its CronJob uses `concurrencyPolicy: Forbid`, `backoffLimit: 0` and
`restartPolicy: Never`, so the next lock attempt comes from a later
scheduled run, not from the same process.

`released_lease_admits_successor_before_old_session_exits` covers the
new method. A closing backend drops its temp tables before it frees
session locks. So the test locks a temp table owned by the leader's
session from another transaction, which keeps the old backend alive
after release. It then asserts that a fresh `try_lock` succeeds at once
and that the old backend has not exited.

Open PRs #9040 (edits this relay test and keeps `drop(worker)`) and
#9043 (changes the `sqlx` import in storage_accounting.rs) will see
small textual conflicts.

#### Validation

Local Linux workstation, throwaway `postgres:16-alpine` (CI uses
`postgres:16`), desired-state schema applied as in the PostgreSQL lane:

- Adversarial ordering, using temporary, uncommitted changes: the
leader's connection was made public and the relay test held the worker
backend's exit for 1 s with the same temp-table lock. Without the fix:
15/15 failed at postgres_tests.rs:66:74 after 5.1-5.3 s, the CI
signature. With the fix: 10/10 passed.
- Natural contention: Postgres, the test processes and busy loops were
pinned to 4 CPUs. With 8 parallel runs and 4 busy loops, the unmodified
test passed 24/24 without the fix. With 16 parallel runs and 16 busy
loops, it passed 48/48 without the fix and 48/48 with it. Load alone did
not open the window locally.
- New db test: 50/50 passed. With the unlock removed from `release`,
leaving only `close`, it fails with "a released lease must admit a
successor at once". All four storage_accounting Postgres tests pass.
- `cargo fmt --all -- --check`, `cargo clippy -p buzz-db -p buzz-relay
-p buzz-admin --all-targets -- -D warnings`, `just file-size-check` and
`git diff --check` are clean.

A green run doesn't prove the flake is gone. Hosted CI covers broad
validation.

**Rebased onto origin/main `64d12e297` (this branch):** the relay test,
run in a fresh database cloned from the desired-state template each
time, passed 8/8 at the branch head, which includes this commit. The
full rebased results are under the next commit.

#### Rebase note

#9040, which this commit's message lists as an open PR, has since merged
to main. It removed the per-community storage metric, including the
`buzz_community_storage_bytes` assertion just above `drop(worker)` in
`crates/buzz-relay/src/storage_sweep/postgres_tests.rs`. The cherry-pick
onto current main conflicted on that hunk. The resolution keeps main's
version of the test and changes only `drop(worker)` to
`worker.release().await.expect("release worker lease")` with its
comment. Nothing else in the commit changed, and no semantic adjustment
was needed. The commit message is unchanged, so it still describes #9040
as open.

### fix(db): report a storage-accounting release only once its unlock
succeeded

#### Problem

Second review of the branch (nit) on 51c2721 ("fix(db): unlock the
storage-accounting lease before its session closes"),
`StorageAccountingLeader::release` in storage_accounting.rs. That commit
cites the unlock-then-close pattern in runtime/migration.rs but handles
errors differently:

- A failed unlock returned early with `?`, skipping the graceful close.
The dropped connection then closed only the socket.
- A failed close after a successful unlock was returned as an error,
although the lease was already free.
- `pg_advisory_unlock` returning false, which means this session didn't
hold the lock, was ignored and reported as success.

So the doc comment's "Once this returns, the lease is already free" did
not hold for every `Ok`.

#### Cause

`release` propagated the unlock's error with `?` before the close,
propagated the close's error after a successful unlock, and ignored the
boolean that `pg_advisory_unlock` returns. So a failed unlock skipped
the graceful close, a failed close was reported although the lease was
already free, and an unlock that returned false because this session no
longer held the lock was reported as success.

#### Fix

Follow migration.rs's `with_schema_destruction_lock`. Run the unlock as
`query_scalar::<bool>` and keep its result, always attempt the close and
ignore its error, then return the unlock's error, or `InvalidData` if
the unlock returned false. The doc comment now says that `Ok` means the
unlock ran on the lease's own session, so the lease is free even if the
close failed, and that an error means the unlock failed or the session
no longer held the lease.

`released_lease_admits_successor_before_old_session_exits` gains a final
step: it unlocks the successor's lease on the successor's own session,
then expects `release` to fail. Its successor check is now an `expect`
with the same message.

#### Validation

Local Linux workstation, the `postgres:16` container used earlier on
this branch, `BUZZ_TEST_DATABASE_URL` pointing at it:
- `cargo test -p buzz-db --lib storage_accounting -- --ignored`: 4/4
storage_accounting Postgres tests pass.
- With the unlock's boolean ignored, as before this change, the new step
fails with "releasing a lease the session lost must fail".
- `cargo fmt -p buzz-db -- --check`, `cargo clippy -p buzz-db
--all-targets -- -D warnings` and `git diff --check` are clean.

**Rebased onto origin/main `64d12e297` (this branch, both commits):**
Linux workstation, private `postgres:16` container with the
desired-state schema applied as in the PostgreSQL lane (pgschema plus
the reconcile script).

- `cargo test -p buzz-db`: 191 lib, 21 `observability_source` and 5 doc
tests passed. The 674 Postgres-backed tests are `#[ignore]`d there.
- `cargo test -p buzz-db --lib storage_accounting -- --ignored
--test-threads=1`: 4/4 passed.
`released_lease_admits_successor_before_old_session_exits` alone: 20/20
passed.
-
`storage_sweep::postgres_tests::worker_publication_activates_reader_without_restart_or_mode_change`,
each run in a fresh database cloned from the desired-state template: 8/8
passed.
- `cargo clippy -p buzz-db --all-targets -- -D warnings`, `cargo clippy
-p buzz-relay --all-targets -- -D warnings`, `cargo fmt -p buzz-db -p
buzz-relay -- --check` and `git diff --check` are clean.

## Coordination

- #9043 (draft, "feat(relay): serve storage usage as an operator pull
feed") edits `crates/buzz-db/src/store/storage_accounting.rs`. `git
merge-tree` against its current head shows two small textual conflicts.
First, both sides rewrite the `sqlx` import to the same `use
sqlx::{postgres::PgConnection, Connection};`, and #9043 also drops `use
uuid::Uuid;`. Second, #9043 replaces
`usage_delivery_reads_snapshot_time_and_community_states` at the point
where this branch adds
`released_lease_admits_successor_before_old_session_exits`. Keeping both
sides resolves each one.
- #7899 also touches `storage_accounting.rs`. It already conflicts with
main, and this branch adds no new conflicted file to it.

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

---------

Signed-off-by: mtoohey <[email protected]>
Co-authored-by: Claude Opus 5.5 <[email protected]>

This branch was previously deployed

1 inactive deployment
codex-review — 83e93a3d Deployed Nov 4, 2026 by TheSentinel454 via Run Codex Security Review #12687
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.

2 participants