refactor!: remove deprecated handle_message from AntProtocol#41
refactor!: remove deprecated handle_message from AntProtocol#41
Conversation
The deprecated `handle_message` method always returned response bytes — even for response messages — which could create infinite ping-pong loops if callers blindly sent the result back. All call sites now use `try_handle_request`, which returns `Option<Bytes>` (`None` for response messages). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR removes the deprecated AntProtocol::handle_message API and standardizes all protocol handling call sites on try_handle_request, which returns None for response messages to avoid reply ping-pong loops.
Changes:
- Remove deprecated
handle_messagefromAntProtocoland rely ontry_handle_request: Result<Option<Bytes>>. - Update E2E protocol call sites to unwrap
Option<Bytes>and avoid replying to response messages. - Adjust a handler test to assert
Nonefor unexpected response message inputs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/e2e/testnet.rs | Switch local protocol handling and P2P message routing to try_handle_request, replying only on Some(response) |
| tests/e2e/security_attacks.rs | Update direct protocol PUT helper to unwrap Option and require a response |
| tests/e2e/data_types/chunk.rs | Update E2E chunk payment enforcement test flows to unwrap Option responses |
| src/storage/handler.rs | Remove deprecated handle_message, update tests to use try_handle_request, and adjust unexpected-response behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //! │ protocol_id() = "autonomi/ant/chunk/v1" │ | ||
| //! │ │ | ||
| //! │ handle_message(data) ──▶ decode ChunkMessage │ | ||
| //! │ try_handle_request(data) ──▶ decode ChunkMessage │ | ||
| //! │ │ │ | ||
| //! │ ┌─────────────────────────┼─────────────────┐ │ |
There was a problem hiding this comment.
The top-level architecture diagram still implies try_handle_request “return Ok(response_bytes)”, but the method now returns Result<Option<Bytes>> and may return Ok(None) for response messages. Please update the diagram text to reflect the Option return (e.g., Ok(Some(response_bytes)) / Ok(None)), so it doesn’t mislead readers.
| )) | ||
| })? | ||
| .map_err(|e| TestnetError::Storage(format!("Protocol error: {e}")))? | ||
| .ok_or_else(|| TestnetError::Storage("expected response".to_string()))?; |
There was a problem hiding this comment.
The new ok_or_else(|| TestnetError::Storage("expected response".to_string()))? error message is very generic and will make E2E failures harder to diagnose. Consider including context (e.g., request_id, message type, and/or that the protocol returned None which likely indicates an unexpected response message).
| .ok_or_else(|| TestnetError::Storage("expected response".to_string()))?; | |
| .ok_or_else(|| { | |
| TestnetError::Storage(format!( | |
| "Protocol returned no response bytes for PUT request (request_id={request_id}, node_index={})", | |
| self.index | |
| )) | |
| })?; |
| )) | ||
| })? | ||
| .map_err(|e| TestnetError::Retrieval(format!("Protocol error: {e}")))? | ||
| .ok_or_else(|| TestnetError::Retrieval("expected response".to_string()))?; |
There was a problem hiding this comment.
Same as store_chunk: the new "expected response" error is very generic for debugging E2E failures. Consider adding context (e.g., request_id/address being fetched and that try_handle_request returned None).
| .ok_or_else(|| TestnetError::Retrieval("expected response".to_string()))?; | |
| .ok_or_else(|| { | |
| TestnetError::Retrieval(format!( | |
| "expected response from try_handle_request for GET chunk {address:?} \ | |
| with request_id {request_id}, but got None" | |
| )) | |
| })?; |
- Update architecture diagram to reflect Option return type - Add request_id/node_index context to store_chunk error message - Add request_id/address context to get_chunk error message Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add unit and e2e tests covering the remaining Section 18 scenarios: Unit tests (32 new): - Quorum: #4 fail→abandoned, #16 timeout→inconclusive, #27 single-round dual-evidence, #28 dynamic threshold undersized, #33 batched per-key, #34 partial response unresolved, #42 quorum-derived paid-list auth - Admission: #5 unauthorized peer, #7 out-of-range rejected - Config: #18 invalid config rejected, #26 dynamic paid threshold - Scheduling: #8 dedup safety, #8 replica/paid collapse - Neighbor sync: #35 round-robin cooldown skip, #36 cycle completion, #38 snapshot stability mid-join, #39 unreachable removal + slot fill, #40 cooldown peer removed, #41 cycle termination guarantee, consecutive rounds, cycle preserves sync times - Pruning: #50 hysteresis prevents premature delete, #51 timestamp reset on heal, #52 paid/record timestamps independent, #23 entry removal - Audit: #19/#53 partial failure mixed responsibility, #54 all pass, #55 empty failure discard, #56 repair opportunity filter, response count validation, digest uses full record bytes - Types: #13 bootstrap drain, repair opportunity edge cases, terminal state variants - Bootstrap claims: #46 first-seen recorded, #49 cleared on normal E2e tests (4 new): - #2 fresh offer with empty PoP rejected - #5/#37 neighbor sync request returns response - #11 audit challenge multi-key (present + absent) - Fetch not-found for non-existent key Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add unit and e2e tests covering the remaining Section 18 scenarios: Unit tests (32 new): - Quorum: #4 fail→abandoned, #16 timeout→inconclusive, #27 single-round dual-evidence, #28 dynamic threshold undersized, #33 batched per-key, #34 partial response unresolved, #42 quorum-derived paid-list auth - Admission: #5 unauthorized peer, #7 out-of-range rejected - Config: #18 invalid config rejected, #26 dynamic paid threshold - Scheduling: #8 dedup safety, #8 replica/paid collapse - Neighbor sync: #35 round-robin cooldown skip, #36 cycle completion, #38 snapshot stability mid-join, #39 unreachable removal + slot fill, #40 cooldown peer removed, #41 cycle termination guarantee, consecutive rounds, cycle preserves sync times - Pruning: #50 hysteresis prevents premature delete, #51 timestamp reset on heal, #52 paid/record timestamps independent, #23 entry removal - Audit: #19/#53 partial failure mixed responsibility, #54 all pass, #55 empty failure discard, #56 repair opportunity filter, response count validation, digest uses full record bytes - Types: #13 bootstrap drain, repair opportunity edge cases, terminal state variants - Bootstrap claims: #46 first-seen recorded, #49 cleared on normal E2e tests (4 new): - #2 fresh offer with empty PoP rejected - #5/#37 neighbor sync request returns response - #11 audit challenge multi-key (present + absent) - Fetch not-found for non-existent key Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
handle_messagemethod fromAntProtocol, which always returned response bytes even for response messages, risking infinite ping-pong loopstry_handle_request, which returnsOption<Bytes>(Nonefor response messages)test_handle_unexpected_response_messageto assertNoneinstead of an error responseTest plan
cargo test)cargo test --lib storage::handler)cargo fmtclean🤖 Generated with Claude Code