Repository navigation
feat(relay): serve storage usage as an operator pull feed - #9043
Draft
TheSentinel454 wants to merge 5 commits into
Draft
TheSentinel454 wants to merge 5 commits into
TheSentinel454 wants to merge 5 commits into
Conversation
Replace the push worker from #8714 with a cursor feed the operator polls. The storage-accounting worker now appends one storage_usage_history row per community per completed run (explicit zeros for serving communities it did not measure) in the same transaction that replaces the snapshot, and prunes rows past 400 days. GET /operator/storage-usage?after=<cursor>&limit=<n> pages those rows for operator-signed NIP-98 requests. The relay no longer calls out, elects a delivery leader, or keeps in-memory delivery state, and a run replaced before delivery is no longer lost: a consumer that falls behind catches up from its cursor. BUZZ_STORAGE_USAGE_URL and the delivery worker are removed. The history table is provenance-only (usage_community_id, no FK, operator-global); community deletion severs it. 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]>
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]>
A scan that captured a community before deletion could save after purge severed its history, attributing new rows to the tombstone, which the feed then served under the deleted community's host. save_snapshot now writes history only for communities that are not yet fenced, holding their rows FOR KEY SHARE until commit. The deletion fence takes FOR UPDATE on the community row before purge can run, so the fence either waits for the save (and purge severs its rows) or commits first (and the save re-checks the row and skips it). The feed also hides rows still attributed to a tombstone. Adds storage_usage_history to the desired-state vs migration schema parity test. 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]>
The community tombstone trigger only lets the deletion executor change deletion_state, so the test sets the executor GUCs the production fence sets before it updates the row. 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]>
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 has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the storage-usage push from #8714 with a pull feed. Instead of the relay calling a downstream billing endpoint, the operator pages a cursor feed served on the existing operator API.
Why change it
What this does
buzz-admin storage-snapshotnow appends onestorage_usage_historyrow per community in the same transaction that replaces the snapshot. Communities that are still serving but absent from the snapshot get an explicit zero row, the same zero-fill feat(relay): push per-community storage usage to kgoose #8714 had. The same transaction prunes rows older than 400 days. A worker that lost its lock session writes nothing.GET /operator/storage-usage?after=<cursor>&limit=<1..1000>pages those rows oldest first and returnsnext_cursor. Cursors grow in commit order because runs serialize on the worker lock. The operator stores its own cursor, retries on its own schedule, and catches up after an outage. The relay keeps no delivery state and makes no outbound calls.BUZZ_STORAGE_USAGE_URL, its lock key, its metrics, and the chart and env docs are gone.storage_usage_historyis a provenance-only table underdocs/database.md. Its column isusage_community_id, notcommunity_id. It has no FK and no trigger, is registered operator-global in all three places, and community deletion severs it. That replaces feat(storage): append per-community storage accounting history #7899's approach, which widenedcommunity_write_fence_excluded_table(), and Rule 1 now forbids that.What got simpler: there's one integration direction instead of two, no relay egress, no second leader election, no in-memory delivery state, and no lost-run gap. Net −620 lines.
Overlap: #9040 also edits
config.rs,main.rs,.env.exampleanddocs/storage-accounting.md. Whichever lands second needs a mechanical rebase.The receiving service changes from accepting POSTs to polling this feed. That change is drafted separately.
Related issue
Supersedes the transport in #8714. Builds on the history idea in #7899, reshaped for the current schema rules.
Testing
storage_accounting::postgres_tests::usage_history_feeds_each_run_with_zero_fill_paging_and_retentiondrives the productionsave_snapshotwrite path:deletion::postgres_tests::purge_postgres_succeeds_with_relay_admin_action_rowsnow also checks that purge severs the purged community's history rows and leaves a bystander's alone.api::operator::postgres_tests::storage_usage_feed_pages_history_for_operators_onlycovers:afterand for an out-of-rangelimit;ce8b7f21:cargo fmt --check, workspace clippy (--all-targets -D warnings) and the buzz-db Postgres lane (676 passed). The buzz-db and buzz-relay unit lanes and the buzz-relay and buzz-admin Postgres lanes passed at4aa105df; the only change since is a buzz-db test fix.ce8b7f21.Relay E2Eis red ontest_nip29_announce_channel_wire, which fails intermittently on main and on other branches too, not because of this change.🤖 Generated with Claude Code