Cover the whole transport/codec matrix in the e2e tunnel test - #6
Conversation
`test_delay.rs` ran one case: UDP with `--codec`, on hardcoded ports read from a checked-in `tests/.env`, sequenced by three 200ms sleeps. The TCP case existed but was `#[ignore]`d. So three of the four real forwarding paths were never exercised, and the one that was could only run when no relay already held port 7666. Now every case builds its own relay, echo server, and tunnel: - Four cases — TCP and UDP, each with and without `--codec`. The codec flag is not cosmetic: the relay generates an AES key per stream and swaps `Normal*` for `Codec*` readers and writers (`pb-mapper-server/src/client.rs:226`), so these are four distinct paths through the forwarder. - The transport is a parameter in code, not `SERVER_TEST_TYPE` in a dotfile, which is what made the matrix impossible to express before. - Every port is `:0`. The relay takes a pre-bound listener via `run_server_on_listener`, so there is no window between reserving a port and binding it. `connect` binds inside the client, so its address is reserved by binding and dropping — with the matching protocol, since TCP and UDP have separate port spaces. - No sleeps. Registration waits on the relay's `Keys` status; forwarding waits on a probe payload making the full round trip. Both are true readiness rather than a guess at how long startup takes. The cases run concurrently, which needs every process-global read out of the path: `AuthRuntime::start` takes the key directly instead of reading the process credential, both tunnel ends use their pinned-credential entry points, and each relay gets its own auth state directory for the `auth.lock` flock. One `LazyLock` writes the process credential once, for the framing checksum that `NormalMessageWriter` falls back to. `tests/.env` and the `dotenvy` dev-dependency are gone with the last test that needed them. Test count goes 136 pass + 1 ignored to 139 pass + 0 ignored: the ignored TCP case is replaced by real coverage, and the single tunnel case became four. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4ffb0cfa4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Transport::Tcp => { | ||
| let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); | ||
| let addr = listener.local_addr().unwrap(); | ||
| drop(listener); |
There was a problem hiding this comment.
Dropping the temporary TCP listener here means the address is no longer reserved before run_client_side_cli_with_pinned_credential binds it; the UDP branch has the same gap. When another parallel test or local process acquires that ephemeral port, the client retries its bind forever and this test eventually fails its readiness deadline, contradicting the documented guarantee that concurrent cases cannot collide. Preserve ownership of the bound listener/socket through startup or add a bind-and-retry mechanism that selects a new address.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
The harness lived privately inside `test_delay.rs`, so a second test file wanting a full `server` + `register` + `connect` flow had two options: crowd into that file, or copy it. This makes it a crate any test can depend on. Two levels, because the cases need different things. `TunnelHarness::start` is the one-liner for "give me a working tunnel". `Relay` and `TunnelSpec` split beneath it for cases that must act on the relay first — a relay retains its `AuthRuntime` clone, which both lets a test issue, renew and revoke credentials without the admin wire protocol, and keeps the actor's command channel open, since dropping every clone cancels all leases. A real crate rather than `tests/common/mod.rs`: that module compiles separately into every test binary, and whatever a given binary does not use is reported as dead code — fatal under `-D warnings`. Traffic drivers come in framed and raw pairs. `NormalMessageReader` writes a checksum + length header, while the local side of a tunnel is byte transparent, so an echo server that prepends a tag byte shifts every frame header. Framed drivers need a transparent echo; tagged tunnels need the raw ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`regression.rs` drives the credential lifecycle at the hand-rolled frame level, which left the real entry points untested with anything but the administrator key. These run `register` and `connect` themselves, so a temporary credential goes through connection pooling, the heartbeat window, control-connection reconnect, the stream-establishment handshake, and `connect`'s startup status probe. Nine cases: the transport/codec matrix on a temporary credential, two credentials sharing one service name in different namespaces, renew keeping a live tunnel forwarding past its original TTL, expiry and revocation each closing one, and the administrator placing a service inside a tenant's namespace. The namespace case tags each echo server, so a leak cannot satisfy payload equality — without it both servers echo identically and the assertion proves nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Building a `PbMapperState` applies its stored `MSG_HEADER_KEY` to the process, and a state rooted at a fresh temporary directory has no stored key — so `temp_state` *clears* the process credential. The four tests share one binary, so a sibling constructing its state could wipe the credential that `a_failed_registration_releases_its_claim` had just set, and `register_service` reads it in phase 1, before the address parsing the case is actually asserting on. It failed with `InvalidArgument` instead of `InvalidAddress`. `temp_state` now takes `PROCESS_CREDENTIAL_TEST_LOCK` and hands the guard back, the way the tests under `crates/` already do. It was latent before — adding test load to the workspace run is what surfaced it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| Transport::Tcp => { | ||
| let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); | ||
| let addr = listener.local_addr().unwrap(); | ||
| drop(listener); |
There was a problem hiding this comment.
reserve_addr reserves a port by binding then immediately dropping the socket (same for the UDP branch at line 159). Between the drop and the later real bind performed by connect/register's echo server, the OS ephemeral-port allocator can hand that same port to another concurrent reserve_addr call (all four test_delay.rs cases run concurrently) or to an unrelated process. When that happens, the subsequent real bind fails with "address already in use" and the test fails/flakes intermittently — despite AGENTS.md's claim that "each case reserves its own loopback ports ... so cases run concurrently and never collide," which this TOCTOU window doesn't actually guarantee.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08b9271862
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async fn probe_udp(&self) -> Result<(), String> { | ||
| let socket = connected_udp_socket(self.tunnel_addr).await; | ||
| probe_udp_socket(&socket, &self.expected_echo(PROBE)).await |
There was a problem hiding this comment.
probe_udp_socket documents that the first datagram from each new source socket may be dropped while the relay creates that UDP stream, but this method creates a fresh socket for every attempt in wait_until_forwarding. On transports that consistently consume the first datagram during setup, every retry creates another uninitialized stream and all UDP tunnel tests time out despite a healthy tunnel. Keep one socket across readiness retries so subsequent probes exercise the established stream.
AGENTS.md reference: AGENTS.md:L54-L56
Useful? React with 👍 / 👎.
| let mut reply = Vec::with_capacity(n + 1); | ||
| reply.extend(tag); | ||
| reply.extend_from_slice(&buf[..n]); |
There was a problem hiding this comment.
TCP does not preserve write boundaries, so one payload written by run_raw_tcp_echo may be returned by multiple read calls here. Each chunk then gets another tag, while the driver expects exactly tag + payload; fragmented or coalesced traffic therefore corrupts the expected byte stream and makes the namespace-isolation test flaky. The tagged protocol needs explicit message boundaries, or a connection-level tagging scheme that does not treat each read as one request.
AGENTS.md reference: AGENTS.md:L57-L59
Useful? React with 👍 / 👎.
| pub fn init_test_env() { | ||
| static TEST_ENV: LazyLock<()> = LazyLock::new(|| { | ||
| init_tracing(); | ||
| set_process_msg_header_key(Some(TEST_ADMIN_KEY)).unwrap(); | ||
| }); |
There was a problem hiding this comment.
init_test_env writes the process-global credential without taking PROCESS_CREDENTIAL_TEST_LOCK. If this reusable testkit is used in a test target containing another credential-writing test, that test can hold the documented lock while this unlocked initializer concurrently overwrites its credential, producing authentication or checksum failures in unrelated cases. Acquire and retain the shared lock around this initialization rather than relying on LazyLock alone.
AGENTS.md reference: AGENTS.md:L60-L63
Useful? React with 👍 / 👎.
| while Instant::now() < deadline { | ||
| if self.probe_once().await.is_err() { | ||
| return; |
There was a problem hiding this comment.
When expiry or revocation causes the client listener/control connection to restart, a single probe can fail during that transient gap even if an authentication regression subsequently lets the client reconnect and resume forwarding. Returning on the first error therefore allows both lifecycle tests to pass without proving that the revoked or expired tunnel stays closed; require failures throughout a stability window or additionally verify that the credential remains rejected.
Useful? React with 👍 / 👎.
| let issued = relay.issue(MIN_TEMP_KEY_TTL, "renew").await; | ||
| let key_id = issued.metadata.key_id; | ||
| let credential = pb_mapper_core::checksum::parse_credential(&issued.credential).unwrap(); | ||
|
|
||
| let tunnel = relay | ||
| .start_tunnel(TunnelSpec::new(Transport::Tcp).credential(credential)) |
There was a problem hiding this comment.
This credential expires after 10 seconds, while start_tunnel performs registration and forwarding readiness phases that can each poll under load before the renewal is attempted. On a slow CI worker, the credential can expire during setup or immediately before renew, making the renewal test fail without exercising renewal at all. Use an initial TTL comfortably longer than the worst-case setup duration, or renew before spending the minimum-TTL interval.
Useful? React with 👍 / 👎.
The tagged echo server prepended its identifying byte to every reply, which on a stream means every `read` — and TCP preserves no write boundaries, so a payload arriving as two reads came back with a tag injected into its middle. Measured: correct through 4000-byte payloads, corrupt from 5000 up, four tags on 16 KiB. `run_raw_tcp_echo` generated at most 2000 bytes, so the bug sat under passing tests, waiting for whoever raised the size. A stream now carries the tag once, as the first byte of the connection. That is well defined however the traffic is chunked, and still identifies which echo server answered — all the tag is for. UDP keeps datagram boundaries, so it still tags every reply. The driver's payloads go to 20 KiB, well past the 8 KiB forwarding buffer, so this can no longer hold only because the sizes stayed small; a `const` assertion pins that. Two cases cover it, both verified to fail against the old server: one payload larger than a single read, and the driver and server agreeing over enough rounds to cross the threshold. Also reuse one socket across UDP probes. The relay keys UDP streams by source address, so a fresh socket per attempt meant `wait_until_not_forwarding` watched new streams fail to start rather than the established one going away. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two claims were stronger than the code. `reserve_addr` has to drop the socket before `connect` binds it — the bind happens inside the client and cannot be handed a pre-bound socket — so it picks a port rather than reserving one, and "never collide" was not something it could promise. What does hold is now written down, along with the advice to own a socket outright where a test can. `init_test_env` establishes the process credential for the whole binary and never writes again, which is why a `LazyLock` is the whole synchronisation and not an oversight. The rule that follows is what was missing: a target using this crate must not also set the credential itself, because there is no lock to coordinate with. A case wanting a different one passes it to `TunnelSpec::credential`, which touches nothing global. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
crates/pb-mapper-cli/tests/test_delay.rswas the repository's only end-to-endtest — it stands up a real
server+register+connectchain and pushespayloads through it. But it ran exactly one case: UDP with
--codec, on portshardcoded in a checked-in
tests/.env, sequenced by threesleep(200ms)calls.The TCP case next to it was
#[ignore = "run codec test enough"].Three of the four real forwarding paths were therefore never exercised, and the
one that was could only run on a machine where nothing already held port 7666.
This PR makes each case build its own relay, echo server, and tunnel.
What changed
--codec. The flag is notcosmetic: the relay generates a random AES key per stream and swaps
Normal*for
Codec*readers and writers (crates/pb-mapper-server/src/client.rs:226,:267), so these are four distinct paths through the forwarder.SERVER_TEST_TYPEin a dotfile.That indirection is what made the matrix impossible to express before — one
env var could only select one transport per run.
:0. The relay takes a pre-bound listener throughrun_server_on_listener, so there is no window between reserving a port andbinding it.
connectbinds inside the client, so its address is reserved bybinding and dropping — using the matching protocol, since TCP and UDP have
separate port spaces and a TCP reservation says nothing about UDP.
Keysstatus response;forwarding waits on a probe payload making the full round trip. Both are real
readiness rather than a guess at how long startup takes, and the forwarding
probe covers the relay,
register's control connection,connect's listener,and the echo server in one shot.
tests/.envand thedotenvydev-dependency are gone, along with theAGENTS.mdlines documenting the five fixture variables. No test needsenvironment setup any more.
Why the cases can run concurrently
They previously could not, because of process-global state. Every read of it is
now out of the path, using entry points that already existed:
AuthRuntime::starttakes the administrator key directly, instead ofAuthRuntime::from_processwhich mutates the process credential.*_with_pinned_credentialentry points.auth.lockexclusive flock.
LazyLockwrites the process credential once —NormalMessageWriterisconstructed with
checksum_key: Noneand falls back to it for framing. Allfour cases share the same key, so it is a single write with no race.
One UDP-specific detail worth noting: the tunnel keys UDP streams by source
address, so a fresh socket's first datagram can be dropped while the relay sets
that stream up. Any socket that goes on to assert payload equality warms itself
with a probe first.
Test coverage
Total goes from 136 pass + 1 ignored to 139 pass + 0 ignored. The
ignored TCP case is replaced by real coverage, and the single tunnel case became
four.
The four tunnel cases finish in 0.66s together. Checked for flakiness with 15
consecutive runs of the
test_delaytarget: 0 failures.🤖 Generated with Claude Code