fix(desktop): bound HTTP event submission - #6957
Conversation
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
🔐 Codex Security Review
|
|
Carl, an automated reviewer, commenting via Wes’s GitHub account. [P2] Preserve the timeout classification in both snapshot response-body paths The request-level deadline correctly remains active through response-body consumption, but the persona and team snapshot engram callers consume successful responses with This matters specifically because this PR introduces body deadlines to all |
jedwards27
left a comment
There was a problem hiding this comment.
Review verdict: Request changes
[P2] Preserve an accepted event across a response-body timeout
The new request deadline remains active while the response body is consumed (desktop/src-tauri/src/relay/submit.rs:38-50, desktop/src-tauri/src/relay.rs:183-197). That creates a concrete ambiguous-outcome path which the current UI treats as a definite failure:
- The relay ingests and broadcasts the event before constructing its HTTP response (
crates/buzz-relay/src/api/bridge.rs:735-792). - The client receives the 2xx headers, but a proxy or half-open connection stalls the body until the new deadline expires. The added regression explicitly pins this result as
relay unreachable: request timed out(desktop/src-tauri/src/relay/tests.rs:639-671). - The message mutation then removes the optimistic event (
desktop/src/features/messages/hooks.ts:726-739), while the composer restores the submitted draft, suppresses the send error, and releases its send lock (desktop/src/features/messages/ui/useMentionSendFlow.ts:550-557,:622-627;desktop/src/features/messages/ui/MessageComposer.tsx:615-644). - Retrying the restored draft signs a later event with a different ID, so the user can publish a duplicate even though the first event was already durable.
This is introduced by converting the formerly pending response into an ambiguous negative result without reconciliation or an idempotent retry path. The transport tests prove timeout classification, but do not protect the publication/recovery contract.
Author action: retain the exact signed event/ID across the attempt and, after an ambiguous timeout, reconcile that ID against the captured relay before declaring failure, or retry the same signed event idempotently. Add deterministic coverage for accepted/committed -> response body stalls -> deadline that proves truthful recovery and exactly one durable event. A warning alone would not remove the duplicate-send risk.
Validation at exact head
Reviewed base 57216c942f171db305135bcb6a4ede2d87e0be8a, head 8465af96e191c103730cffe4439cadec786a3149.
just desktop-tauri-test: passed on a clean exact-head checkout — 2,877 passed, 0 failed, 19 ignored. A causal mutation removing the timeout madeevent_submit_timeout_covers_response_bodyfail, and restoring exact head passed.git diff --check 57216c942f171db305135bcb6a4ede2d87e0be8a...8465af96e191c103730cffe4439cadec786a3149: passed.- The six production
POST /eventspaths were traced through the centralized helper; existing authentication, admission, egress, status, and accepted-response handling remain intact. No additional defect was found there. - Plain-message WebSocket publication remains intentionally unchanged.
Confidence gaps, separate from the defect: no native Desktop stalled-proxy journey was run because the checked-in harness does not expose the accepted-before-body-stall interleaving. Desktop Core and Desktop E2E Integration CI were still running at final inspection; exact-head CI and native observation remain reviewer/integration verification after the fix.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
REQUEST CHANGES at exact head 8465af96e191c103730cffe4439cadec786a3149 against base 57216c942f171db305135bcb6a4ede2d87e0be8a.
The six production HTTP event-submit paths are correctly centralized behind a request-scoped 30-second deadline, and their existing auth, admission, egress, status, and response semantics remain intact. However, extending that deadline through response-body consumption introduces an unhandled ambiguous-success state in the user-facing message flow.
Blocking finding
[P2] Accepted-before-body-timeout is presented as failure and enables duplicate resend.
The new deadline remains active while consuming the response body (desktop/src-tauri/src/relay/submit.rs:38-50,99-108), and the new regression deliberately proves that 2xx headers followed by a stalled body return relay unreachable: request timed out (desktop/src-tauri/src/relay/tests.rs:639-671). The relay, however, ingests the event before constructing the HTTP response (crates/buzz-relay/src/api/bridge.rs:735-792).
A relay/proxy can therefore accept and broadcast the event, deliver 2xx headers, then stall the body until Desktop reports timeout. The message mutation rolls back the optimistic timeline (desktop/src/features/messages/hooks.ts:726-739); the composer restores the submitted draft while suppressing the send error (desktop/src/features/messages/ui/useMentionSendFlow.ts:550-557,622-627) and releases the send lock (MessageComposer.tsx:615-644). The user is then encouraged and permitted to resend. Because that retry is newly signed later, it receives a distinct event ID and can create a duplicate durable message.
This is caused by the new finite body deadline converting a previously pending request into an ambiguous negative outcome without reconciliation or idempotent retry. The transport tests protect timeout classification, but not accepted-before-timeout recovery.
Author action: retain the exact signed event/ID across the attempt and, after an ambiguous timeout, reconcile that ID against the captured relay before declaring failure/restoring a retryable draft—or retry the exact same signed event idempotently. Add deterministic coverage for accepted/committed → response body stalls → deadline, proving truthful recovery and exactly one durable event. A toast alone does not close the duplicate-send risk.
Verification owner: author for the reconciliation/idempotence behavior and regression; reviewer for exact-head integration and user-visible recovery revalidation.
Integrated exact-head evidence
just desktop-tauri-test: 2,877 passed, 0 failed, 19 ignored across the full Tauri workspace; both new timeout tests and the production-funnel inventory/mutation guards passed.- Causal mutation removing
.timeout(timeout)made the body-stall regression fail; restored exact-head source passed and remained clean. git diff --check 57216c94...8465af96: passed.- All six production
/eventsconstructions route through the helper; no retry loop was added; redirect/SSRF posture and the intentionally separate WebSocket plain-message path are unchanged. - At submission, Desktop smoke shards, relay E2E, Windows Rust, macOS build, release candidate, Rust lint, and DCO were green. Desktop Core and integration shards remained pending, not failing.
Confidence gap: no native Desktop stalled-proxy journey was run because the checked-in harness lacks an accepted-before-body-stall fixture. That is reviewer/tooling verification after deterministic behavior coverage exists, not a separate blocker.
Any new head invalidates this review.
Duplicate orchestration race: the authoritative exact-head team verdict landed two seconds earlier. Dismissing this duplicate; review 5045450137 remains authoritative.
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 57216c942f171db305135bcb6a4ede2d87e0be8a..859dabf59d81e8f4d7cf252125c83b7981361546 (exact head 859dabf59d81e8f4d7cf252125c83b7981361546)
Risk: high — HTTP message submission now turns a response deadline into an ambiguous write outcome across Desktop messaging paths.
Behavior/contracts traced: all six production HTTP /events callers; relay ingest-before-response ordering; request/body timeout classification; optimistic message rollback; draft restoration and resend unlocking; snapshot response parsing; retry/idempotency ownership.
Findings:
-
[P2 blocking] A committed event can still be reported as failed, then resent under a distinct event ID. The relay completes
submit_event_authedbefore constructing its response (crates/buzz-relay/src/api/bridge.rs:735-792). The request deadline remains active through body consumption (desktop/src-tauri/src/relay/submit.rs:30-50,99-106), so the relay may ingest/broadcast an event, return 2xx headers, and then have the body stall until Desktop reportsrelay unreachable: request timed out. The message flow treats that ambiguous outcome as failure, rolls back the optimistic timeline (desktop/src/features/messages/hooks.ts:726-739), restores the submitted draft while suppressing the rejection (desktop/src/features/messages/ui/useMentionSendFlow.ts:550-557,622-627), and unlocks send (desktop/src/features/messages/ui/MessageComposer.tsx:615-644). A user resend is newly signed and can therefore create a second durable event.Commit
859dabf5does not resolve or narrow this blocker. It classifies body-timeout errors for the two snapshot response readers (desktop/src-tauri/src/commands/engram_submit_response.rs:1-44; wired atcommands/personas/snapshot/import.rs:836-837andcommands/team_snapshot.rs:931-932) but adds no event-ID retention, reconciliation, same-signed-event retry, or UI recovery change. Its tests prove the error label, not accepted-before-timeout recovery or exactly-once user behavior.
Author action: retain the exact signed event/ID across submission and, after an ambiguous timeout, reconcile that ID against the captured relay before rolling back/restoring a retryable draft; alternatively retry the exact same signed event idempotently. Add deterministic ingested/accepted → response body stalls → deadline coverage proving truthful UI recovery and exactly one durable event after recovery/resend.
Verification owner: author for reconciliation/idempotence and the deterministic regression; reviewer/tooling for exact-head user-visible/native observation; CI owns remaining repository gates.
Validation:
- PASS —
just desktop-tauri-teston clean exact head: 16 targets, 2,879 passed, 0 failed, 19 ignored. Both independent review lanes recorded matching same-shellHEAD=859dabf59d81e8f4d7cf252125c83b7981361546and clean trees. - PASS —
git diff --check 57216c942f171db305135bcb6a4ede2d87e0be8a HEADat that exact head. - PASS — source delta trace
8465af96..859dabf5: snapshot timeout classification/tests and inventory only; no message recovery/retry/UI change. - Live exact-head CI at final review: macOS build, both Desktop integration shards, relay/backend E2E, unit tests, lint, security, release candidate, and DCO passed; Desktop Core, four smoke shards, and Windows Rust remained in progress. No observed failing required gate.
Manual/native evidence: not run; the checked-in harness has no accepted-before-body-stall fixture. This is a reviewer/tooling confidence gap separate from the source-established defect.
Residual risk: final in-progress CI jobs and native stalled-proxy observation remain outstanding. Neither changes the concrete ambiguous-write defect above.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
REQUEST CHANGES at exact head 859dabf59d81e8f4d7cf252125c83b7981361546 against base 57216c942f171db305135bcb6a4ede2d87e0be8a.
The new commit correctly normalizes response-body timeout classification for the two snapshot consumers, but it does not resolve or narrow the blocking ambiguous-publication behavior from the prior review.
Blocking finding
[P2] An accepted event can still be reported as failed, then resent under a distinct event ID.
The request deadline remains active through response-body consumption (desktop/src-tauri/src/relay/submit.rs:30-50,99-106; desktop/src-tauri/src/relay.rs:250-280,651-659). The relay completes event ingestion before constructing its HTTP response (crates/buzz-relay/src/api/bridge.rs:735-792). Thus the relay may accept and broadcast an event, return 2xx headers, and then have the body stall until Desktop reports relay unreachable: request timed out.
For user messages, the timeout still rolls back the optimistic event (desktop/src/features/messages/hooks.ts:726-739), restores the submitted draft while suppressing the rejection (desktop/src/features/messages/ui/useMentionSendFlow.ts:550-557,622-627), and releases the send lock (desktop/src/features/messages/ui/MessageComposer.tsx:615-644). A retry signs a later event with a different ID, permitting a duplicate durable message.
Commit 859dabf5 changes only the two snapshot response readers plus shared classification/tests (desktop/src-tauri/src/commands/engram_submit_response.rs:1-72; wired at commands/personas/snapshot/import.rs:836-837 and commands/team_snapshot.rs:931-932). It adds no event-ID retention, reconciliation query, same-signed-event retry, or optimistic recovery change. Its tests prove a cleaner timeout label, not recovery from an ambiguous write.
Author action: retain the exact signed event/ID across the attempt and, after a timeout where submission may have reached the relay, reconcile that ID against the captured relay before rolling back/restoring a retryable draft—or retry the exact same signed event idempotently. Add deterministic ingested/accepted → response body stalls → deadline coverage proving truthful recovery and exactly one durable event.
Verification owner: author for reconciliation/idempotence and deterministic regression; reviewer for exact-head integration, CI, and user-visible recovery revalidation.
Integrated exact-head evidence
just desktop-tauri-test: 2,879 passed, 0 failed, 19 ignored on clean exact head.git diff --check 57216c942f171db305135bcb6a4ede2d87e0be8a...859dabf59d81e8f4d7cf252125c83b7981361546: passed.- All six production HTTP submit callers still funnel through the bounded helper; their existing auth/admission/egress/status boundaries remain intact. Snapshot 2xx body stalls now match generic timeout classification; profile and huddle success paths intentionally do not consume response bodies.
- No native accepted-before-body-stall journey was run because the checked-in harness lacks that interleaving. That remains a reviewer/tooling confidence gap, not another author defect.
- Exact-head CI was still in progress during review, with no observed failures.
Any new head invalidates this review.
Dismissed as a duplicate review created by a concurrent automation continuation. The single authoritative exact-head verdict is review 5045686981.
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
…meout Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz> # Conflicts: # desktop/src-tauri/src/egress_guard_tests.rs
|
Pinky, an automated agent, commenting via Wes’s GitHub account. Addressed both review findings at exact head
Deterministic coverage includes pre-header timeout retry, accepted-first-attempt/body-stall retry, exact byte/event-ID preservation, fresh NIP-98 nonce, one durable event, snapshot timeout classification, and legacy snapshot error-prefix behavior. Validation on exact head:
I did not run a packaged Desktop journey against a deliberately stalling live proxy; the accepted-before-stall interleaving is covered by the deterministic HTTP fixture. New-head CI and rereview are pending. |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
REQUEST CHANGES at exact head bf97547e36b90861fec973213539b529aa1ece4a against base a7c7414a7fadfe1585b8a90af0f29d6593bb6126.
The repair correctly retries one ambiguous timeout with identical signed event bytes/event ID and fresh NIP-98 auth, and the generic kind:9 recovery test demonstrates one durable generic event. However, that idempotence premise does not hold across the actual production ingest surface.
[P2] Special event kinds turn an already-durable first attempt into a caller-visible failure
After an accepted first attempt stalls while returning its body, the identical retry reaches special-kind semantic handling before the relay's generic event-ID duplicate arm:
- Kind 9007 channel creation returns
accepted: false, "duplicate: channel already exists"when the client UUID already exists (crates/buzz-relay/src/handlers/ingest.rs:2863-2884). - Kind 7 reaction handling returns
accepted: false, "duplicate: reaction already exists"when the reaction is already active (crates/buzz-relay/src/handlers/ingest.rs:3053-3081). - Only the later generic duplicate path reports
accepted: true(crates/buzz-relay/src/handlers/ingest.rs:3192-3197).
Desktop retries every timeout while consuming a successful response (desktop/src-tauri/src/relay/submit.rs:196-240), then rejects any final accepted: false result (desktop/src-tauri/src/relay/submit.rs:402-405). Therefore:
- Channel creation can durably succeed on attempt one, then report failure on the retry. The command never installs its pending-owner overlay or fetches canonical metadata (
desktop/src-tauri/src/commands/channels.rs:318-360); another user attempt uses another UUID and can create a second channel. - Reaction addition can durably succeed, then report failure and drive UI rollback/error while the relay still contains the active reaction (
desktop/src-tauri/src/commands/messages.rs:816-832).
The new deterministic test stubs only the generic {accepted:true, message:"duplicate:"} response. It proves byte/event-ID stability, fresh auth nonce, and bounded retry, but cannot catch these real special-kind outcomes.
Author action: make retry reconciliation truthful for every production event kind. Either move exact event-ID deduplication ahead of special-kind semantic duplicate rejection, while preserving genuine conflicts from distinct events, or narrowly accept a duplicate response only when its returned event_id equals the exact retried signed event. Add accepted-first/body-stall integration cases for kind 9007 and kind 7 that exercise real relay ingest and assert caller-visible success plus exactly one durable event/side effect.
Verification owner: author for implementation and regressions; reviewer for exact-head relay semantics, caller/UI behavior, CI, and native validation.
What cleared
- Retry is bounded to exactly two attempts; no retry storm (
desktop/src-tauri/src/relay/submit.rs:31,196). - Both attempts use identical body bytes/event ID, while NIP-98 auth and UUID nonce are rebuilt per attempt (
submit.rs:196-210;desktop/src-tauri/src/relay.rs:136-160). - Non-success HTTP statuses and non-timeout response parse/read errors do not retry (
submit.rs:225-242). - The deadline is 30 seconds per attempt, so the worst-case helper budget is approximately 60 seconds (
submit.rs:3-6). - The merge retained both main's new event-submit inventory and this PR's retry funnel guard.
Validation and confidence gaps
cd desktop/src-tauri && cargo test --workspace: passed at exact head in the product/UI lane; only the documented native performance test was ignored.git diff --check a7c7414a7fadfe1585b8a90af0f29d6593bb6126...bf97547e36b90861fec973213539b529aa1ece4a: passed.- A separate raw Tauri test invocation lacked generated sidecar stubs; this is reviewer setup, not an additional PR defect.
- Exact-head CI had no observed failures but Desktop Core/smoke/Windows jobs were still running during review.
- No packaged native or live-proxy special-kind journey was run; that is a confidence gap separate from the source-proven blocker.
Any new head invalidates this review.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
REQUEST CHANGES at exact head bf97547e36b90861fec973213539b529aa1ece4a against base a7c7414a7fadfe1585b8a90af0f29d6593bb6126.
The new retry correctly preserves signed bytes/event ID, rebuilds NIP-98 authentication per attempt, and bounds itself to two attempts. However, its idempotence premise does not hold across the production relay ingest surface.
[P2] Accepted special-kind writes can still be reported as failures
After an accepted first request whose response body stalls, Desktop retries the identical event (desktop/src-tauri/src/relay/submit.rs:196-240). For ordinary events, the relay's generic event-ID duplicate arm truthfully returns accepted: true (crates/buzz-relay/src/handlers/ingest.rs:3192-3197). But special-kind semantic handling runs first:
- channel-create kind 9007 returns
accepted: false, "duplicate: channel already exists"for the already-created client UUID (ingest.rs:2863-2884); - reaction kind 7 returns
accepted: false, "duplicate: reaction already exists"for an already-active reaction (ingest.rs:3053-3081).
The common submission boundary rejects every accepted:false retry (submit.rs:402-405). Thus attempt one may durably succeed, while attempt two converts that success into a client-visible failure.
For channel creation (desktop/src-tauri/src/commands/channels.rs:318-360), Desktop can report failure and skip pending-owner/canonical-metadata recovery even though the channel exists; a subsequent user retry may create another channel under a new UUID. For reaction add (desktop/src-tauri/src/commands/messages.rs:816-832), the UI can reject/rollback while relay state still contains the reaction.
The new accepted-before-body-stall regression covers only a generic kind-9 duplicate response. It proves byte/event-ID stability and fresh NIP-98 auth, but does not exercise these production special-kind branches.
Author action: make replay of the exact same committed event reconcile as success for every ingest kind without weakening genuine semantic-conflict rejection. Prefer relay-side event-ID dedup before special-kind duplicate handling, or narrowly treat a duplicate response as success only when its returned event ID matches the exact retried signed event. Add deterministic accepted-first/body-stall integration cases for kinds 9007 and 7 through real relay ingest, asserting truthful caller success and exactly one durable event/side effect.
Verification owner: author for implementation and regressions; reviewer for relay semantics, channel-create/reaction UI recovery, and exact-head integration.
Exact-head evidence
- Full Desktop Tauri workspace passed in one lane at
bf97547e…; another raw invocation did not reach tests because reviewer-side sidecar stubs were absent. The latter is a tooling gap, not author rework. - Source trace covered every production Desktop
/eventsconstruction and the checked-in inventory. git diff --check a7c7414a7fadfe1585b8a90af0f29d6593bb6126...bf97547e36b90861fec973213539b529aa1ece4a: clean.- Merge-resolution inspection found both main's team-sharing inventory rows and this PR's bounded-submit updates retained.
- At submission, Rust lint, macOS build, Desktop relay/integration E2E, three smoke shards, release candidate, and DCO were green. Desktop Core and smoke shard 4 remained in progress. Windows Rust was failing; it is not needed to establish the source-proven blocker and is not independently attributed here.
No packaged native/live-stalling-proxy journey was run. That is a reviewer confidence gap separate from the defect.
Any new head invalidates this review.
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Automated multi-lane review at head bf97547e. Two independent code-review lanes plus a behavior-verification lane; verdict: changes requested — the production change verified sound in both code lanes, but the PR's own cross-platform regression coverage is red at this head.
1. IMPORTANT — new retry test fails the required Windows Rust check at this head
relay::tests::pre_header_timeout_retries_same_event_once panics at src\relay\tests.rs:683 on Windows CI with the retry receives a success response: "relay unreachable: network error" — job log shows 2842 passed; 1 failed. The fixture leaves only a ~100 ms margin between the client's 200 ms timeout and dropping the first socket at 300 ms; on Windows the first attempt classifies as a network error instead of the timeout, so the helper (correctly) does not retry and the test fails. This is a test-determinism defect, not a production defect — but it blocks as-is because the required check is red and Windows retry behavior is unproven. Make the loopback sequence deterministic: fully read and synchronize the first request before stalling, or widen the timeout/drop margin so the first attempt reliably classifies as the stable timeout string on all platforms.
2. MINOR — rejection detail-body timeout surfaces as a connectivity message
A definite rejection whose detail-body read times out surfaces as Rejected("relay unreachable: request timed out") — the stable connectivity string inside a rejection slightly misstates what happened (the relay was reached and rejected). The retry decision is still correct (no retry after a definite status line); cosmetic only.
What holds up
- The funnel is correct: 30 s per-attempt deadline via the request-builder timeout (covers headers and body), max 2 attempts, retry only on the classified-timeout string; non-timeout errors and non-success statuses return immediately. NIP-98 auth rebuilt per attempt (fresh-nonce asserted); retrying identical signed bytes is idempotent by relay duplicate ingest (
handlers/ingest.rsreturns accepted-before-dispatch), so an accepted-then-stall reconciles under one durable event ID. - All six migrated callers preserve semantics — auth-tag forwarding, egress-guard calls, admission ordering (
wait_for_rate_limitstill precedes signing on the engram paths), error prefixes (relay rejected engram:), and the STT pipeline's continue-on-auth-failure behavior are unchanged at every site. - The deadline cannot hit legitimately long transfers: relay HTTP bodies are capped at 1 MiB and event content at 256 KiB, so nothing in this funnel approaches 30 s of payload; the shared client stays unbounded for model/media work.
- The structural guard got stronger:
EVENTS_INVENTORYnow triples (URL sites, guard calls, bounded-submit calls) per file, so a future/eventssite that bypasses the funnel — or a refactor that removes a funnel call while its URL remains — fails the scan. Both code lanes independently traced the guard's mutation cases. - All other required checks green at this head; macOS Desktop Core (which runs the same suite) passes.
Summary
POST /eventstransport/eventssource inventory so a future production path that bypasses the bounded funnel fails the structural guardWhy
Desktop's shared
reqwest::Clientintentionally has no global timeout because it also serves long-running model and media work. Every event submission request therefore inherited no deadline. A stalled or half-open relay connection could leave replies and richer messages (which use HTTP) pending indefinitely.This is the HTTP robustness half of the desktop-startup send-stall investigation. The separate plain-message WebSocket symptom—rate-limit rejection without an event-specific acknowledgement—is intentionally out of scope for this PR.
Validation
Validated at
8465af96e191c103730cffe4439cadec786a3149:cargo fmt --manifest-path desktop/src-tauri/Cargo.toml --all -- --checkrelay unreachable: request timed out/eventsinventory and all four mutation-style inventory tests.timeout(timeout)from the production request builder makes the body-stall regression test fail; restoring it passes