refactor(relay): exercise operator listener delivery over HTTP - #7934
jsibbison-square wants to merge 1 commit into
Conversation
🔐 Codex Security Review
|
Signed-off-by: Implementor <691cca7a870db1dad6990d5938d1b4a2a2ca9647ca81290b536c17b06b2e473f@buzz.block.builderlab.xyz>
4afaf55 to
7526485
Compare
TheSentinel454
left a comment
There was a problem hiding this comment.
🤖 Non-blocking review notes. Removing the single-implementation traits and testing the real reqwest + NIP-98 path against a local axum receiver is a clear improvement, and it fits the repo's postgres_tests convention. The six new tests pass in the PostgreSQL lane at this SHA. Three small P2s are inline.
Follow-up for a separate PR, not this one: point 2 from the #7793 review (claim only rows this pod can route, listener_pubkey = ANY($n)) would remove release_unroutable_delivery, the release branch, and unroutable_delivery_releases_claim_without_http.
| fail_permanently(db, &delivery, &format!("HTTP {status}")).await; | ||
| } | ||
| Err(error) => retry_or_fail(store, &delivery, error.to_string()).await, | ||
| Err(error) => retry_or_fail(db, &delivery, error.to_string()).await, |
There was a problem hiding this comment.
🤖 P2: this transport-error branch no longer has a test. The old exhausted-attempts test used a transport error ("connection reset"), but exhausted_attempt_is_terminal now uses a 503, so it covers the HTTP-status branch instead. One cheap way to cover it: bind a TcpListener, take its address, drop the listener, and route the delivery there. The connection is refused, so you can assert the row is retried (attempt < max) or removed (attempt = max).
| } | ||
| // The worker claims from the deployment-global outbox, so these tests | ||
| // must not claim each other's fixture rows in parallel. | ||
| static DB_TEST_LOCK: Mutex<()> = Mutex::const_new(()); |
There was a problem hiding this comment.
🤖 P2: this lock only helps under plain cargo test against one shared database. The PostgreSQL CI lane runs each test in its own process against its own database, so the lock does nothing there. On a shared dev database, claim_operator_listener_deliveries claims the 10 oldest due rows across the whole deployment, ordered by next_attempt_at. If 10 or more older due rows are left over, the fixture row is never claimed and the test fails with a confusing message. Suggest saying that in the comment, or pushing the leftover rows' next_attempt_at into the future in the fixture.
| let url: url::Url = format!("http://{}{}", headers["host"].to_str().unwrap(), uri) | ||
| .parse() | ||
| .unwrap(); | ||
| nostr::nips::nip98::verify_auth_header( |
There was a problem hiding this comment.
🤖 P2 (optional): if verification fails, the unwrap() here panics inside the handler. That drops the connection, reqwest sees a transport error, and the delivery is retried. The tests still fail because each one asserts received.len(), but the failure message points away from the real cause (bad auth). Returning 401 and recording the verification error on Listener would make a failure here self-explanatory.
TheSentinel454
left a comment
There was a problem hiding this comment.
🤖 Approving at 752648577cc0. The notes in the previous review are non-blocking.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
One P2 regression-test blocker inline; I found no production-behavior regression in the trait removal. Please restore a falsifiable no-redirect assertion and show that removing Policy::none() makes that test fail.
Reviewed head 752648577cc01255b60c439182e671af640a807b against base c4c86006f2d67b2ea9055780129ee7ce95c54e0a. Existing CI passed, including all six replacement tests in the PostgreSQL lane. A targeted standalone HTTP reproduction using this head’s receiver/signing setup and pinned dependencies confirmed the blind spot; the full PostgreSQL suite was not rerun locally. An independent reviewer reproduced the same result.
| let (target_url, target, target_server) = | ||
| start_listener(AxumStatusCode::OK, None).await; | ||
| let (source_url, source, source_server) = | ||
| start_listener(AxumStatusCode::FOUND, Some(target_url.to_string())).await; | ||
| let fixture = Fixture::new(1).await; | ||
| fixture.deliver(&fixture.routes(source_url)).await; | ||
| assert_eq!(source.received.lock().await.len(), 1); | ||
| assert!(target.received.lock().await.is_empty()); |
There was a problem hiding this comment.
[P2] Count redirect-target requests before method routing/authentication
This replacement passes even if .redirect(Policy::none()) is removed. Both endpoints use start_listener, which only routes POST /mentions. Following the source’s 302 changes the request to GET, so the target returns 405 before receive records it. The worker treats that 405 as terminal and deletes the outbox row: source count 1, target recorded count 0, and absent row all still satisfy this test, despite contacting the target. The removed TCP-level test detected that contact.
I reproduced the HTTP portion with this head’s receiver and reqwest 0.13.4 / axum 0.8.9, adding a request-observing middleware:
redirects disabled: 302, source_received=1, target_received=0, target_wire_requests=[]
redirects enabled: 405, source_received=1, target_received=0, target_wire_requests=[GET]
The unchanged worker classifies both statuses as terminal. This loses the regression guard on the configured-endpoint boundary.
Give the target an all-method counter that runs before routing/authentication (or count accepted connections), and assert that counter remains zero. Keep NIP-98 verification on the source. Verify that removing the no-redirect policy fails the test. Merely switching to 307 is insufficient if target counting still happens after authorization verification.
Summary
DeliveryStoreandDeliveryTransporttraits and their mocks.Follow-up to address review point 4 on #7793.
Validation
cargo test -p buzz-relay --lib operator_listener::tests:: -- --include-ignoredwith the repository's configured local database: 7 passed.. ./bin/activate-hermit && just ci: exit 1. Formatting and clippy passed;buzz-acptests failed on three environment-sensitive cases (session_new_forwards_complete_git_block_without_duplicate_names,allowed_respond_to_full_path_unset_allows_all,test_session_policy_default_is_channel).. ./bin/activate-hermit && just test: exit 1. The same threebuzz-acpcases failed, plusbuzz-db'sp0_pool_acquisitions_use_typed_operation_pairs_without_othersource check and two workspace integration tests (oauth_missing_token_uses_configured_model_then_retries_discovery,non_auth_discovery_failure_uses_configured_model_without_caching_fallback). These failures also appear in the prior follow-up's logs.