Skip to content

GH-818 Review 3 into master#762

Closed
dnwiebe wants to merge 31 commits intomasterfrom
origin/GH-818-review3
Closed

GH-818 Review 3 into master#762
dnwiebe wants to merge 31 commits intomasterfrom
origin/GH-818-review3

Conversation

@dnwiebe
Copy link
Collaborator

@dnwiebe dnwiebe commented Mar 17, 2026

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors routing and proxy request handling to remove the legacy “return route id” concept, make the target host explicit (via a new Host type), and improve test/debug ergonomics around stream keys and protocol packet introspection.

Changes:

  • Introduces sub_lib::host::Host and threads it through route queries/responses and proxy request payload creation.
  • Removes return-route-id plumbing from Route/Neighborhood/tests and deletes related message types/subscriptions.
  • Enhances proxy protocol packs with describe_packet() and extends TtlHashMap with a retire callback + remove() API.

Reviewed changes

Copilot reviewed 35 out of 37 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
node/src/test_utils/recorder.rs Removes recording support for deprecated proxy-server return-route message subscription.
node/src/test_utils/mod.rs Updates route/test helpers to include host/TLS context; removes return-route-id helper; adjusts message type helper signature.
node/src/sub_lib/ttl_hashmap.rs Adds retire-closure behavior and remove(); expands unit tests accordingly.
node/src/sub_lib/stream_key.rs Makes “meaningless” stream keys random; adds a test-only constructor for deterministic keys.
node/src/sub_lib/stream_handler_pool.rs Renames TransmitDataMsg.sequence_number to sequence_number_opt.
node/src/sub_lib/sequence_buffer.rs Adapts to TransmitDataMsg.sequence_number_opt rename.
node/src/sub_lib/route.rs Removes return-route-id encoding/decoding and related debug/test logic.
node/src/sub_lib/proxy_server.rs Makes ClientRequestPayload_0v1.target_hostname mandatory and removes AddReturnRouteMessage + subscription.
node/src/sub_lib/neighborhood.rs Replaces optional hostname with Host in route queries and stores host in route responses; adds ExpectedService accessors; removes return-route-id from ExpectedServices.
node/src/sub_lib/mod.rs Exposes new host module.
node/src/sub_lib/migrations/client_request_payload.rs Updates payload migration to treat target_hostname as required (non-optional).
node/src/sub_lib/http_packet_framer.rs Fixes logger name to match the component.
node/src/sub_lib/host.rs Adds new Host type with Display and tests.
node/src/sub_lib/hopper.rs Updates tests to pass explicit StreamKey into meaningless message helper.
node/src/sub_lib/dispatcher.rs Renames fields to *_opt for clarity and updates debug output + tests.
node/src/stream_reader.rs Renames reception_port/sequence_number fields to *_opt and propagates into messages/logging/tests.
node/src/stream_handler_pool.rs Updates logic/tests for sequence_number_opt and reception_port_opt naming.
node/src/proxy_server/tls_protocol_pack.rs Moves Host import to sub_lib, adds describe_packet() and detailed TLS packet descriptions + tests.
node/src/proxy_server/server_impersonator_tls.rs Updates DNS-failure response signature to take a required server name.
node/src/proxy_server/server_impersonator_http.rs Updates DNS-failure response signature to take a required server name and removes “unspecified” variant test.
node/src/proxy_server/protocol_pack.rs Removes duplicate Host struct, adds describe_packet() to ProtocolPack, and updates from_ibcd() to use reception_port_opt.
node/src/proxy_server/http_protocol_pack.rs Moves Host import to sub_lib, adds describe_packet() with human-readable request/response summaries + tests.
node/src/proxy_server/client_request_payload_factory.rs Accepts optional host from history, requires a resolved host, and uses protocol packs’ packet descriptions in error messages; updates/extends tests.
node/src/proxy_client/stream_reader.rs Updates tests to avoid assuming deterministic “meaningless” stream keys.
node/src/proxy_client/stream_handler_pool.rs Removes optional hostname handling; updates logic/tests to assume target_hostname is always present.
node/src/proxy_client/stream_establisher.rs Updates tests for mandatory hostname and non-deterministic meaningless stream keys.
node/src/proxy_client/mod.rs Updates tests for mandatory hostname and updated route helper signatures.
node/src/neighborhood/mod.rs Threads Host through route building/undesirability logic and updates many tests for removed return-route-id and new route length expectations.
node/src/hopper/routing_service.rs Updates tests for new route helper signatures and renamed *_opt fields.
node/src/hopper/mod.rs Updates tests for new route helper signatures and make_meaningless_message_type(stream_key) signature.
node/src/hopper/live_cores_package.rs Updates tests for removed return-route-id behavior and explicit stream keys in meaningless message creation.
node/src/hopper/consuming_service.rs Updates tests for renamed *_opt fields and explicit stream keys in meaningless message creation.
node/src/dispatcher.rs Updates tests for renamed *_opt fields and sequence_number_opt.
multinode_integration_tests/tests/self_test.rs Updates integration tests for explicit stream keys in meaningless message creation.
multinode_integration_tests/tests/connection_termination_test.rs Removes return-route-id extraction/usage and updates request/report helpers accordingly.
.gitignore Ignores Copilot-related IDE migration artifacts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

} else {
error!(logger, "{}", e);
}
error!(logger, "{}", e);
@dnwiebe dnwiebe closed this Mar 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants