feat(relay): add early startup lifecycle logs - #7258
Conversation
🔐 Codex Security Review
|
1f7db9b to
3f5b95b
Compare
Record bounded, secret-safe lifecycle events for the relay's earliest startup phases. Keep the contract log-only so pre-exporter failures retain their real event time. Co-authored-by: Ravneet Arora <rarora@squareup.com> Signed-off-by: Ravneet Arora <rarora@squareup.com>
3f5b95b to
8faf752
Compare
|
P1 — post-bind metrics exporter failure remains detached and silent At exact head This also leaves the stated contract of #7238 incomplete: that issue explicitly calls for retaining supervision of the exporter lifecycle, while this PR says Please either:
The existing successful-start and occupied-port tests cover pre-bind behavior, but do not exercise a post-bind exporter exit. |
|
🤖 Thanks — confirmed. Dropping Tokio's I took option 2 to keep this PR within its logs-only early-startup scope:
No production code or staged image changed as part of this scope correction. |
wpfleger96
left a comment
There was a problem hiding this comment.
One blocking correctness gap remains. The bounded schema, secret-safe reasons, early stderr sink, exact terminal accounting, and child-process coverage are otherwise solid.
Quality scores:
- Minimalism: 9/10 — the schema and typed classifications are tightly bounded.
- Elegance: 9/10 — phase ownership and aggregate accounting are clear and fit the startup path.
- Correctness: 8/10 — bind/build/conflict handling is covered, but the exporter lifecycle required by #7238 is still detached.
CI is green at exact head 8faf7526822a119efa035e58b2b3c59aa67fc81d.
| .map_err(|_error| MetricsInstallError::RecorderConflict)?; | ||
| describe_readiness_metrics(); | ||
| describe_db_pool_metrics(); | ||
| tokio::spawn(exporter); |
There was a problem hiding this comment.
IMPORTANT — Correctness: This still drops the exporter JoinHandle immediately after reporting metrics_bind/succeeded. If this future later returns Err or panics, the relay keeps running with a dead metrics endpoint and no in-process failure record. That leaves #7238’s explicit exporter-supervision requirement incomplete. Please retain and supervise the task, surface an unexpected exit with a bounded secret-safe reason, and add a production-seam regression that forces a post-bind exit. If supervision is intentionally deferred, this PR should stop closing #7238 and link a follow-up issue instead.
There was a problem hiding this comment.
Independent live verification at the same exact head confirms this failure mode through the production API seam:
- Built
buzz-relayat8faf7526822a119efa035e58b2b3c59aa67fc81d(release artifact SHA-256d98e1dead1086cd126437c195e2d5b971f742e6d53523679af80ea7a1a9b7c40). - Started
BootTracker, ran requiredMetricsBind, calledmetrics::try_install(), and successfully scraped the actual bound HTTP exporter. - Dropped the exporter’s Tokio runtime to force a post-bind task exit while leaving the process alive. The endpoint then refused connections, but the process remained alive.
- Lifecycle evidence remained limited to
metrics_bindand aggregateprocess_telemetryterminal records withstatus: "succeeded"; no terminal or degradation record reported the exporter’s disappearance.
Observed probe result:
PROBE bound_and_scrapeable=true port=65429
PROBE child_alive_after_exporter_exit=true connect_after_runtime_drop=Err(... ConnectionRefused ...)
This independently confirms that the detached JoinHandle leaves stale, materially false process-local lifecycle evidence after metrics loss. External scrape alerts can detect absence, but they do not supervise, restart, terminate, or correct lifecycle state. The IMPORTANT correctness severity and required fix above stand. The probe worktree, listener, and process were cleaned up afterward.
There was a problem hiding this comment.
🤖 Thanks for the independent live reproduction. I agree the post-bind exporter terminal is currently unobserved.
This PR is taking the deferral option from the original comment. The current PR body no longer closes #7238; it explicitly scopes #7258 to the logs-only early-startup slice and links #7284. #7284 tracks retained exporter ownership, bounded secret-safe runtime-failure reporting, owned drain/teardown under an independent deadline, and the post-bind production-seam regressions.
The existing metrics_bind and process_telemetry terminals remain immutable startup receipts; #7284 must report the later runtime failure without emitting a second startup terminal. No production code or staged image changed for this scope correction.
Please re-review the corrected scope when you can.
There was a problem hiding this comment.
I re-reviewed the corrected scope. Removing the closing reference to #7238 and explicitly tracking post-bind exporter supervision in #7284 satisfies the deferral option from my original finding. The detached exporter remains a real pre-existing runtime gap, but it is no longer an unfulfilled claim of this logs-only PR, so this blocker is resolved.
wpfleger96
left a comment
There was a problem hiding this comment.
The scope correction resolves my requested change: this PR now explicitly delivers only the logs-only early-startup slice, does not close #7238, and tracks post-bind exporter supervision in #7284. The bounded schema, secret-safe reasons, early stderr sink, exact startup terminal accounting, and child-process coverage meet that narrower contract.
Quality scores:
- Minimalism: 9/10 — the fixed schema and typed classifications stay tightly bounded.
- Elegance: 9/10 — phase ownership and aggregate accounting are clear and fit the startup path.
- Correctness: 9/10 — the implementation and tests cover the stated logs-only startup contract; the independently reproduced post-bind runtime gap is explicitly deferred to #7284 rather than claimed here.
All applicable CI checks are green at exact head 8faf7526822a119efa035e58b2b3c59aa67fc81d.
…rcement * origin/main: feat(relay): add early startup lifecycle logs (#7258) docs(nip-fi): add Blossom kind-24242 media possession-proof exception (#7278) fix(desktop): wrap message tables within the available pane (#7279) Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
…ssion-deny * origin/main: feat(relay): add early startup lifecycle logs (#7258) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…-enforcement * origin/main: feat(relay): add early startup lifecycle logs (#7258) Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
* origin/main: chore(release): release Buzz Desktop version 0.5.21 (#7301) fix(scripts): copy global-agent-config.json in buzz-adopt-prod-agents (#7303) feat(relay): add early startup lifecycle logs (#7258) docs(nip-fi): add Blossom kind-24242 media possession-proof exception (#7278) fix(desktop): wrap message tables within the available pane (#7279) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
Why
Early relay failures can currently appear as a container restart without a trustworthy in-process account of whether crypto, structured logging, configuration, relay identity, or the metrics listener failed. Most of those steps happen before the Prometheus exporter exists, so their chronology belongs in logs rather than metrics.
Implements the logs-only early-startup slice of #7238. Post-bind Prometheus exporter supervision is tracked separately in #7284.
What changed
crypto_init,tracing_init,config_load,key_load,metrics_bind, and the aggregateprocess_telemetryphase;metrics_bindcan be classified without logging raw values, while preserving the existing publicmetrics::installAPI;This PR adds no startup metric families and no dashboard contract. Existing application metrics remain unchanged.
Verification
Exact head:
8faf7526822a119efa035e58b2b3c59aa67fc81dcargo fmt --all -- --checkcargo clippy -p buzz-relay --all-targets -- -D warningscrates/buzz-relay/src/api/media.rs:1145withSqlx(PoolTimedOut)because local PostgreSQL is unavailableAll exact-head GitHub CI gates are green, including lint, unit tests, PostgreSQL, relay/backend/desktop integration, both Linux server cross-compiles, Windows/macOS builds, and security checks.
Staging verification
dev-sha-8faf7526822a119efa035e58b2b3c59aa67fc81d-run-33708188952-1sha256:26cad28266a6bb0b0e7081eb6091d374e5489f8bb78c475a4a65737dee86cc67buzz-d68764bc7has two Ready pods with zero restartsprocess_telemetry/terminal/succeededat 3 msbuzz_startup_phase_terminalorbuzz_startup_phase_duration_secondsfamiliesThe experimental Row 7 was removed from the Buzz Startup & Rollout Safety dashboard. This logs-only PR deliberately adds no replacement dashboard row.
Update Sep 3, 12:26 ET: Clarified the review boundary: this PR does not close the broader #7238. Later exporter-task termination is pre-existing runtime behavior and is now explicitly tracked in #7284; no production code or staged image changed in this update.
Generated with Codex