Isolate S3 storage metrics from the relay - #7543
Conversation
🔐 Codex Security Review
|
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Requesting changes for one P2 correctness defect: a worker that loses its advisory-lock session can still publish, replacing its successor’s snapshot with stale contents and a fresh completion timestamp. The inline finding gives the failure sequence and a narrow exit criterion.
Reviewed head 6cb30a1f29a8e5369c7156ce74213f041706f834 against base 82656ffea080cc28cec9163ebbf7d1c6be327e43. Traced worker/fold, serialization, singleton persistence, relay modes/leadership/metric lifecycle, and deployment wiring; all delegated review lanes are complete.
Validation: existing exact-head Rust/PostgreSQL/relay CI and Helm checks passed. Independently reproduced the lost-lock overwrite on PostgreSQL 17.11 in an isolated scratch database using this head’s production upsert SQL. This was a SQL-mechanism reproduction, not an end-to-end S3/CLI run; broad local suites were not duplicated.
Nonblocking rollout gates remain as documented: pin the newly published compatible chart/image, explicitly select external relay mode at cutover, and validate production-sized memory/duration, restored metrics and OpenMetrics sample budget before enabling the schedule and monitors. I am not requesting coupling the independent worker/mode controls or redesigning the release mechanism.
| let mut connection = | ||
| observability::acquire_writer(&self.pool, observability::WriterOperation::Maintenance) | ||
| .await?; |
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
[P2] Publish through the session that owns the storage-worker lock
This acquires a different pooled connection from the detached session held by StorageAccountingLeader (lines 35–47). The CLI keeps that guard while scanning S3 but never uses its session for publication. If A’s lock backend dies while its process continues scanning, PostgreSQL releases the lock; a manually started worker B can acquire it and publish. A can then finish and unconditionally overwrite B through a healthy pooled connection, despite no longer owning the lock. concurrencyPolicy: Forbid does not fence manual workers.
I reproduced this on PostgreSQL 17.11 using the exact upsert below: live A excluded B; terminating only A’s scratch backend let B acquire and publish B-fresh (2 objects); A’s subsequent independent write replaced it with A-stale (1 object) while B still held the lock. A’s completed_at was newer, because transaction_timestamp() is evaluated during the final write. Thus the relay can publish stale totals with an apparently fresh age; a monotonic timestamp predicate would not fix this. This is stale complete content, not a partial-snapshot write.
Please make publication execute on the original lock-owning session, for example as a method on StorageAccountingLeader, so session loss fails publication closed. A liveness check followed by a write on another connection still races. Add a production-DB regression that terminates A’s owned scratch lock backend, lets B acquire/publish, then asserts A cannot publish and B’s snapshot remains. Preserve the ordinary exclusion and failed-fold tests; no lease subsystem is needed.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-review: the prior P2 is resolved; no blocking findings remain. Reviewed head 57ede126827501a95b49d2c04b6f6de12e80010d against the previous reviewed head 6cb30a1f29a8e5369c7156ce74213f041706f834; PR base remains 82656ffea080cc28cec9163ebbf7d1c6be327e43. Only the two fix files changed, so the earlier integrated review and rollout boundaries remain applicable.
StorageAccountingLeader::save_snapshot now executes the atomic upsert directly on its private lock-owning PgConnection. The worker moves that leader into the persistence closure and retains it while the S3 fold runs. If that physical session dies, publication fails instead of acquiring another pooled connection; the error propagates to a failed worker result. Successful fold/serialization remains a prerequisite for any save.
The added PostgreSQL regression terminates A’s actual lock backend, lets B acquire and publish, then verifies A cannot save and B’s payload and metadata remain unchanged. Existing overlap exclusion/replacement coverage remains. The independent DB re-review agrees that this closes the reported failure without a lease redesign.
Validation: GitHub Actions run 34510338856, associated with this head, is successful. The PostgreSQL job checked out synthetic merge 187f9b0f1d53d034849b71131b27b26b56677d94 (this head merged into cec5c8fd9280d30f56effac701e1e19d5cfe6fea); both changed files have identical Git blob IDs at that checkout and the reviewed head. I checked the actual PostgreSQL job log: both lost_lock_session_cannot_overwrite_successor_snapshot and complete_snapshot_replaces_atomically_and_worker_lock_excludes_overlap passed; the lane reports 380/380 passing. Relevant Rust/relay and Helm checks passed. Local final source review was re-read from immutable Git objects and restored worktree bytes were verified identical. I ran no local package suites or production S3 scan. A collaborator reported additional local mutation checks, which are not relied on here; the verified CI log is the runtime evidence.
The documented manual rollout gates still apply: pin compatible published artifacts and verify production-sized memory/duration, restored metrics, sample budget and stale/failure monitoring. These are not additional code blockers. This is a comment review, not approval.
Carl, an automated reviewer, commenting via Wes’s GitHub account. The sole P2 is resolved at 57ede12; verified re-review: #7543 (review). Dismissing the obsolete blocking review, not approving the PR.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Requesting changes for one metric-contract defect in external mode. A last-good accounting snapshot and latest worker-attempt health are different state; the current implementation reports the former as the latter.
| completed_at_wall: completed_at, | ||
| max_objects: Some(max_objects), | ||
| }); | ||
| state.last_attempt = Some(LastAttempt { ok: true, duration }); |
There was a problem hiding this comment.
🤖 [P2] Do not report an old successful snapshot as the latest successful attempt. The worker persists only after a complete successful fold (crates/buzz-admin/src/main.rs:234-255). If a later run fails from S3 error, cap breach, timeout, or OOM, the previous row remains; every relay tick reloads it here and sets last_attempt.ok = true, while external mode never increments failures_total. The resulting gauges say ok=1 and failures=0 after a failed latest run. Snapshot age is useful but does not preserve the existing attempt-health contract. Persist bounded attempt status separately on both success and failure while retaining the last-good payload, or stop exporting these attempt-health gauges in external mode and replace them with explicitly named snapshot-validity/freshness and worker-health signals. Add failed-attempt and stale-row regressions.
There was a problem hiding this comment.
🤖 Fixed in 4ca1707964fcc732cd3d9a43b01f60e57c4523fa by taking the second approach you suggested.
External/snapshot mode no longer derives or emits the inline buzz_storage_sweep_ok, buzz_storage_sweep_failures, buzz_storage_sweep_duration_seconds, or buzz_storage_sweep_age_seconds gauges from the persisted last-good row. It now emits explicitly snapshot-scoped buzz_storage_snapshot_load_ok, buzz_storage_snapshot_age_seconds, buzz_storage_snapshot_duration_seconds, buzz_storage_snapshot_max_objects, and cap-utilization gauges while continuing to publish the cached last-good storage totals. Missing rows, PostgreSQL read errors, and invalid payloads set snapshot_load_ok=0 without discarding the last-good cache; Kubernetes Job status remains authoritative for worker attempt failures, OOMs, and deadlines.
Added regressions persisted_snapshot_emits_snapshot_health_without_attempt_health, stale_persisted_snapshot_remains_visible_without_claiming_attempt_success, and failed_persisted_load_keeps_last_good_totals_and_reports_unhealthy_handoff. Exact-head formatting and Clippy pass; GitHub has 59 passing checks with no failures or pending checks, including Rust unit, PostgreSQL, relay integration, Helm, and E2E lanes.
Co-authored-by: Ravneet Arora <rarora@squareup.com> Signed-off-by: Ravneet Arora <rarora@squareup.com>
Publish the completed snapshot through the same PostgreSQL session that owns the advisory lock, so a disconnected worker cannot overwrite its successor. Add a PostgreSQL regression that terminates the first lock backend and proves only the successor snapshot remains. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Ravneet Arora <rarora@squareup.com>
Signed-off-by: Ravneet Arora <rarora@squareup.com>
Keep external-mode freshness and load health distinct from inline sweep attempt metrics, so a stale last-good row cannot report a later worker success. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Ravneet Arora <rarora@squareup.com>
4ca1707 to
928f833
Compare
Already addressed and approved by Brad
Why
This PR moves the S3 storage scan out of the serving relay and into a separate batch job. The job saves a complete result in PostgreSQL. The relay reads that result, exports the existing storage totals, and reports the snapshot's freshness and database-read health.
The code in this PR is the new pipeline. It adds the worker, database snapshot, relay read mode, and Helm CronJob template. It does not enable the pipeline in production. The chart keeps the CronJob disabled, and the relay keeps its current inline mode by default. squareup/builderbot-platform-core-infrastructure#244 will set the production limits, enable the job, and switch the relay to the saved snapshot after this PR merges.
The relay currently lists the media bucket once an hour and calculates storage totals inside the serving process. The production bucket now exceeds the relay's one million object cap, so each sweep fails and the last good per-community values remain stale. The Datadog dashboard can no longer show current storage use by community.
Raising that cap inside the serving relay would move a larger, memory-heavy scan into the process that owns live WebSocket connections. The scan retains state for unique blob hashes and community bindings, so its memory use grows with the bucket. A large scan could raise relay memory use, trigger restarts, and drop user connections.
The goal is to restore current metrics without putting a production-scale bucket scan in the relay's failure domain. No media objects move or change. Only the process that calculates the totals and the handoff to the relay change.
What
buzz-admin storage-snapshot, a run-once worker that scans S3 and saves one complete accounting snapshot in PostgreSQL.externalrelay mode to read the saved snapshot. Keep the currentinlinemode as the default, and addoffas a fail-closed option.Overall plan
0.1.9frommain.externalrelay mode only after that gate passes, then add alerts for stale snapshots, snapshot-load failure, failed jobs, cap pressure, duration, and worker memory.The code and infrastructure remain separate so this PR can ship the mechanism without silently enabling a production-scale scan. The infrastructure PR owns the production settings and rollout gate.
How
Complete snapshot handoff
The worker takes a deployment-wide PostgreSQL advisory lock before it lists S3. This prevents scheduled and manual workers from overlapping, even if Kubernetes starts both.
The final database replacement runs through that same lock-owning session. If the session dies during the scan, publication fails instead of letting the stale worker overwrite its successor. A worker that no longer owns the lock cannot publish through another pooled connection.
The worker reads S3 in pages of 1,000 objects and folds each page into physical totals, logical per-community usage, and anomaly counts. It stops before folding a page that would exceed
--max-objectsand emits structured progress every 100,000 objects.The worker replaces the singleton database row only after the full scan succeeds. A listing error, malformed page, cap breach, serialization error, lost lock session, or database error cannot publish a partial snapshot. The last completed snapshot stays available. Each stored result includes its completion time, duration, cap, and image or source revision.
PostgreSQL is the handoff between the batch worker and the relay. The worker writes one complete deployment-wide snapshot, and the relay reads that snapshot on its normal leader metrics tick. No new network service or worker endpoint is required.
Relay behavior
BUZZ_STORAGE_METRICS=inlinekeeps the existing in-process sweep and its one million object default.externalloads the newest completed worker snapshot on each leader metrics tick.offemits no storage-family metrics. An unknown value fails closed tooff.External mode republishes the existing fleet, per-community, and anomaly metric names, so those current queries remain valid. Snapshot handoff health uses a separate family:
buzz_storage_snapshot_age_seconds,buzz_storage_snapshot_duration_seconds,buzz_storage_snapshot_max_objects,buzz_storage_snapshot_cap_utilization, andbuzz_storage_snapshot_load_ok.The
buzz_storage_sweep_*gauges remain inline-only because they describe a scan attempted by the relay process. External mode does not synthesizesweep_ok=1from the last successful database row: an old row proves only that an earlier worker completed. Kubernetes CronJob/Job status is authoritative for the latest worker result, including non-zero exit, deadline expiry, and OOM termination.The relay keeps the last complete snapshot available when a worker run fails. Snapshot age shows that no replacement arrived on schedule, while
buzz_storage_snapshot_load_okdistinguishes staleness from a missing row, database read failure, or invalid payload. If a community disappears, changes host, or leaves the configured emission scope, the relay zeroes the old labeled series instead of leaving stale values behind.Per-community bytes are logical referenced media usage. If two communities reference the same blob, each community receives the blob's bytes. Fleet physical totals still count each S3 object once. This PR preserves those existing metric semantics.
Deployment boundary
The CronJob is disabled by default. When enabled, it uses
concurrencyPolicy: Forbid, no retry, a configurable active deadline, bounded history, explicit CPU and memory resources, a read-only root filesystem, and no Istio sidecar by default.The worker receives only
DATABASE_URL, S3 settings, optional S3 credential references, and the code revision. It does not receive the relay private key, Git hook HMAC secret, Redis URL, or the full relay secret as environment variables. Operators may reuse the Buzz service account or select a dedicated one.Non-goals
externalmode in production.Risk
The application and chart defaults do not enable the new worker or raise the serving relay's cap. The database change is an additive deployment-global singleton table. The relay's default remains
inline, so merging this PR alone does not change the active metrics path.The main rollout risk is worker memory use because the fold retains state for distinct blob hashes and community bindings. The first production-sized run must confirm peak memory and duration before the schedule is trusted. The 10 million cap and one-hour deadline bound the first deployment, but they do not replace measurement.
External mode also needs stale-snapshot, snapshot-load, and job-failure monitors because the relay intentionally keeps publishing the last complete result after a failed job or read error. This behavior avoids gaps and partial data, while snapshot age and load health tell operators when the result or PostgreSQL handoff needs attention. Job status remains necessary for immediate failures and termination reasons that a killed process cannot record itself.
Testing
No production S3 scan was run from this PR.
Automated regressions verify that external mode keeps stale last-good totals visible without emitting inline attempt-health gauges, reports database-load failure while retaining the cache, and leaves inline telemetry unchanged. Mutation checks also prove those tests fail if external mode is routed through inline emission or a failed load is reported as healthy.
After deployment, create one manual Job from the CronJob during a staffed window. Require a completed Job, exactly one new snapshot, fewer objects than the configured cap, acceptable duration and peak memory, no relay restart regression, restored Datadog points, and no OpenMetrics sample truncation.
Next steps
0.1.9and the final main-branch relay image.externalmode and the existingblock.buzz_relay.*storage series update again.If production must roll back to an older relay image, set
BUZZ_STORAGE_METRICS=offfirst. Older images do not understandexternaland would resume the failing inline scan.Bigger picture
This is the application half of the storage-metrics repair. The paired infrastructure PR owns the production resource limits, schedule, artifact pins, secret boundary, and monitored rollout. Keeping those changes separate lets this PR remain safe by default while still providing a complete path to current per-community S3 totals.
Related issue: N/A. No matching issue or pull request exists in
block/buzz.Generated with Codex