fix(mesh): retain echo receive across housekeeping ticks - #7140
Conversation
Keep the pending reliable receive and Redis validation alive until a frame arrives or shutdown actually drains the stream. Drop its mutable borrow before the existing Goodbye/finish path. Cover consumed-frame pool contention, drain before and after community latch, EOF, sequential echoes, peer Goodbye, and stale lease rejection. Signed-off-by: Logan Johnson <loganj@squareup.com>
🔐 Codex Security Review
|
|
@buzz-security-review a02372a |
Desktop smoke failure: existing animation-sampling raceThe required Desktop aggregate remains red because smoke shard 3 failed The failure mechanism is identifiable:
On macOS arm64/Chromium 148.0.7778.96, with an isolated E2E build/private port and no relay, the exact unmodified single test passed once. A separate diagnostic recorded a running channel transition from transparent to Suggested separate test-only follow-up, not applied to this two-file mesh PR: await channel.hover();
+ await waitForAnimations(page);
const channelHoverColor = await channel.evaluate(A copy with this stabilization passed 3/3, no retries, against the same unchanged build. No production CSS change was needed. A lucky rerun can also compare transient values, so these local passes do not clear the required remote check; no remote rerun was dispatched. Maintainer action: choose a policy-compliant disposition for the red Desktop gate, preferably a separate test stabilization, then establish a valid green gate for the relevant revision. This PR remains draft; human/codeowner/last-push review and the choice versus overlapping #2745 remain outstanding. Independently, the existing repository-wide local |
) ## Summary - Reject kind:20001 presence events with `OK false` / `error: presence storage unavailable` when Redis SET or DEL fails, before publishing, local fan-out, or local-event marking. - Preserve the producer contract needed by snapshot-confirming consumers: delivered live presence must follow successful mutation of the Redis state read by snapshots. - Classify those backend rejections with the existing `IngestError` taxonomy so a presence storage outage counts as `buzz_events_rejected_total{transport="ws",reason="error"}`, not client `reason="invalid"`; genuine client-input refusals (verification failure, membership gates) stay `invalid`, and every wire message is an unchanged fixed sanitized string (review follow-up, no protocol wording change). - Add actual `handle_event` integration coverage for rejected online/offline transitions, healthy online→offline accepted/stored/fanned-out behavior, and the rejection-counter routing on storage failure with an invalid-signature control. This is standalone on main; it does not depend on the mobile implementation. Deploy this relay prerequisite before relying on block#7526's snapshot-confirmation policy. Existing pubsub-failure-after-successful-storage behavior and disconnect TTL cleanup are deliberately unchanged. A storage error may be an ambiguous write outcome, not a rollback guarantee; the rejected event is not published by this handler. Clients may retry the generic `error:` rejection. Desktop's 60s heartbeat retries non-offline presence, not every explicit offline transition. ### Related issue Addresses the relay prerequisite identified in [block#7526 review 5157607827](block#7526 (review)). Searched open presence/storage PRs; no duplicate relay storage-error rejection fix found. block#7382/block#7383/block#7526 heads and bases are unchanged. ### Testing Exact head: `389174df29cc02d0f885c03209eff661d8bb2ec0` (+380/-13; 393 total), one commit `389174df2` on top of the reviewed `c031d6eb1` (DCO-signed; base `bfc384855889432df4a333a0edf3080f332ee169` unchanged). - PASS: `cargo fmt --all -- --check`, `cargo clippy -p buzz-relay --all-targets -- -D warnings`, `git diff --check`, `just file-size-check`, PostgreSQL discovery validation — all run at the exact final head with a clean tree before and after. - PASS: documented native `scripts/postgres-test-run.sh -p buzz-relay --lib --tests`: **89/89** actual integration tests, including the four presence cases (online/offline storage rejection, healthy online→offline, and the new rejection-classification case). Owned PostgreSQL 17/Redis on isolated loopback ports, schema plus reconciliation applied; no shared development database. - PASS: explicit `cargo test -p buzz-relay presence_storage -- --ignored --nocapture`: **4/4**, not skipped. - Full isolated relay crate suite at the final head (`cargo nextest run -p buzz-relay --lib --tests`): **1062 run: 1062 passed, 94 skipped**. The previously failing `api::mesh_demo::tests::demo_join_forwarded_arm_round_trips_echo` passed in this run (1.5s); it is a known timing-sensitive main baseline failure tracked open in block#7140 and untouched by this PR, so this single passing run is reported as-is and does not claim environmental clearance or close block#7140. No full-suite-green claim is made beyond this run. - Mobile is untouched; block#7526's existing 2090-test/format/analyze evidence remains scoped to its unchanged head. Its separate Desktop Smoke E2E (2) failure remains red; no CI retries requested. [Production-seam regression coverage](https://github.com/block/buzz/blob/389174df29cc02d0f885c03209eff661d8bb2ec0/crates/buzz-relay/src/handlers/event.rs#L1491-L1803): the metric case drives real `handle_event` traffic against a genuinely dead Redis endpoint with a seeded active PostgreSQL community and a registered presence watcher, asserts the storage rejection counts `reason="error"` while a tampered-signature control through the same dispatcher arm stays `reason="invalid"`, and re-asserts the rejected ACK, no fan-out, and no local-event marker. Counter assertions use a thread-local recorder guard held across `.await` points (the buzz-db counter-test convention) inside the per-process nextest postgres-ci lane, so no parallel test can race the counter snapshot. No UI change or screenshot. Local logs and reproducible service/gate scripts are retained under `WORK_LOGS/MOBILE_FEEDBACK_PRESENCE_20260909/relay_prerequisite/metric_correction/` in the engineering workspace. This PR is a review candidate, not merge clearance. Causal checks: restoring only the pre-fix production mutation block makes both original rejection tests fail (`OK true` instead of `false`); healthy success still passes. Reverting only the typed classification (mapping the ephemeral `Internal` arm back to `invalid`) makes the new metric regression fail with the outage counted as `[("ws","invalid",2)]` instead of `[("ws","error",1),("ws","invalid",1)]`. The unchanged mesh echo case also failed 504/200 with the main-production block restored in the prior run, supporting its separation from this change without claiming environmental clearance. Candidate source restored byte-for-byte after each mutation. Repository-wide `just ci` was not rerun; the scoped relay gates above are the new evidence. --------- Signed-off-by: Logan Johnson <loganj@squareup.com>
1 participant
🤖
Summary
The relay's mesh echo probe can time out even after receiving the message it should echo. Slow session validation makes this more likely: a periodic shutdown check can discard a message that has already been read. This change keeps that pending receive alive until validation finishes or the stream actually shuts down.
The fix is limited to the echo consumer. It is independent of Multiverse and contains none of its feature changes.
A single pending
recv_validated()operation now survives non-draining 100ms housekeeping ticks. That protects both partially read transport frames and messages awaiting Redis, the shared session directory. The invariant is that housekeeping never discards an in-flight receive. Actual shutdown may discard it, but never echoes unvalidated data. The pending receive releases its mutable stream borrow before the existing Goodbye/finish path runs. Session ownership and community validation, EOF/error handling, and tick timing remain unchanged; this does not add drain priority over already-ready data or change transport backpressure behavior.Related issue
Related: #2458. Existing PR #2745 addresses the same cancellation mechanism with broader transport/API and test changes; this is a smaller consumer-only alternative, not a claim that the earlier work is merged or superseded. Maintainers should choose which approach to carry forward.
Testing
On standalone head
a02372ad5636856af41a219b87a64d3be3f98768, with isolated PostgreSQL/Redis and Rust 1.95.0:buzz-relaypackage, run serially: 997 library + 13 binary tests passed; 92 library + 1 binary ignored. The four new ignored-by-default Redis tests were explicitly run above.Repository-wide local
just cicompleted successfully (exit 0), including workspace/Tauri checks and tests, desktop checks/tests/build, web checks/build, and 2,015 mobile tests. Thejust testwrapper was not run because it overwrites inherited service URLs and invokes default-service startup on this shared host; package tests instead used explicit isolated resources. Worker-only Redis/database and local diagnostic listeners have been cleaned up.Remote CI completed with the required Desktop aggregate red: smoke shard 3 failed the profile/channel hover CSS assertion on all three built-in attempts. Diagnosis and separate test-only proposal: the test captures an intermediate channel animation color (.03/.027), then compares it to the profile's settled .04. Both selectors use the same 4% hover token. On the base-identical desktop tree, an isolated diagnostic confirmed equal settled colors in five samples; a copy with a pre-sample animation wait passed 3/3. These local results do not clear the required remote failure, and no frontend change is bundled here. Maintainer disposition of this existing test race is still required; no remote rerun was dispatched.
All other executed required checks passed, including Rust Lint, Unit Tests, Security, Windows Rust, macOS build, both Desktop E2E Integration shards/aggregate, Backend Integration, Desktop E2E Relay, Relay E2E, DCO, and Desktop Release Candidate. Remote Mobile and Web were path-filter skipped (local checks/build/tests above ran). Advisory exact-range security review run 33432941909 is still reported in progress as of 2026-08-31 20:36 UTC (over 43 minutes, no current-range result posted), separately from the passed required Security check. Workflow-owner follow-through is needed; no advisory approval or absence of findings is inferred. This remains a draft, not an all-green or approved PR; human/codeowner/last-push review is still required.
The regression's negative control uses the exact original consumer and the same test: it fails because the receive is recreated four times instead of once. This is reused evidence from the unchanged source/test pair, not a fresh full-main baseline build.
The separate real-QUIC echo fixture tests loopback transport and session routing. Controlled-transport drain tests are not QUIC fault injection. Neither proves HTTP authorization, model execution, Desktop compute parity, or packaging readiness.