Skip to content

fix: eliminate parallel Rust test flakes#2434

Open
pimlock wants to merge 3 commits into
mainfrom
fix/flaky-unit-tests/pimlock
Open

fix: eliminate parallel Rust test flakes#2434
pimlock wants to merge 3 commits into
mainfrom
fix/flaky-unit-tests/pimlock

Conversation

@pimlock

@pimlock pimlock commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Eliminate three recurring parallel Rust unit-test failure modes seen in Actions run 29951858582 and nearby runs. The changes make listener and tracing assertions deterministic and harden Linux proxy process-identity resolution against unrelated connections that reuse the same ephemeral port.

Related Issue

N/A — follow-up to the linked CI investigation.

Changes

  • Inject the SSH forward-listener probe in its failure-path unit test instead of releasing a real ephemeral port and racing another test for it.
  • Rebuild tracing callsite interest after installing each test-local subscriber so parallel callsite registration cannot leave capture disabled.
  • Match the complete local-to-remote TCP tuple, including IPv4-mapped IPv6 normalization, before resolving a proxy peer's socket inode from /proc/net/tcp{,6}.
  • Represent the ordered workload-to-proxy endpoints with WorkloadProxyTcpConnection and use ProcTcpAddressFamily::{Ipv4, Ipv6} instead of a boolean when selecting and decoding procfs tables.
  • Add deterministic tuple-collision coverage and update the sandbox architecture invariant.

Failure details

SSH forward-listener failure

Failure: wait_for_forward_listener_rejects_missing_listener asked the OS for an ephemeral port, closed that listener, and then expected a connection to the released port to fail. Under parallel test load, another process could claim the port between the close and the probe, so the test's premise was not stable.

Fix: The listener-wait loop now accepts an internal probe function. Production still uses the real TCP probe, while the renamed failure-path test injects a deterministic connection refused result and verifies that the diagnostic retains the underlying probe error. The existing real-listener success test continues to cover the integration path.

Tracing capture failures

Failures: evaluation_log_emits_structured_log_annotations and request_id_appears_in_trace_span intermittently observed an empty capture buffer. A tracing callsite can be registered by another parallel test while no interested subscriber is active, leaving its process-wide interest cache disabled even after these tests install a thread-local subscriber.

Fix: Each test installs its capture subscriber for the narrow emission scope and rebuilds tracing's callsite interest cache inside that scope before emitting the event or span. This keeps the tests isolated while ensuring the callsite is enabled for the subscriber that performs the assertion.

Linux proxy process-identity failure

How identity resolution works

Linux does not attach the originating client PID to the proxy's accepted TCP socket. OpenShell therefore resolves it indirectly:

  1. Read /proc/<entrypoint-pid>/net/tcp and tcp6. These files expose the active TCP table for the entrypoint's network namespace, not only sockets owned by that PID.
  2. Find the row for the accepted connection and read its socket inode.
  3. Scan /proc/<pid>/fd for every process holding socket:[inode].
  4. Resolve and integrity-check each owner's executable and ancestors, then use that identity for the network policy decision.

What failed

resolve_process_identity_hashes_live_exe_after_hot_swap sometimes resolved the test harness PID instead of the child process on a busy runner. The old /proc/net/tcp{,6} lookup matched only the workload peer's ephemeral port and returned the first ESTABLISHED row with that port.

A port alone does not identify a TCP connection. For example, these are distinct connections even though both use local port 50000:

wanted:    10.200.0.2:50000 -> 10.200.0.1:3128  inode 111  agent PID
unrelated:    127.0.0.1:50000 -> 127.0.0.1:8080  inode 222  test PID

If the old lookup selected inode 222, the later inode-to-PID scan worked as designed but resolved the wrong connection. The result was wrong table row -> wrong inode -> wrong PID -> wrong binary identity; Linux itself did not return an incorrect PID.

The hot-swap setup was incidental. That test creates a real Bash child with a real TCP connection so it can verify live executable hashing after the on-disk binary is replaced. Parallel CI placed many unrelated sockets in the same host network namespace, making the under-specified lookup much easier to expose.

Production relevance

This is a real production correctness issue, although it is less likely in an isolated sandbox network namespace than in the shared CI namespace. It can occur whenever another active connection in the same namespace has the same numeric local port but differs by local address, remote address, remote port, or address family.

The most likely production effect was a false denial or incorrect audit attribution because the request could be evaluated as the wrong binary. Because binary identity participates in authorization, using an unrelated allowed binary's identity could theoretically produce a false allow as well. This PR does not claim a demonstrated production bypass, but the old port-only association was too weak for a security-sensitive policy boundary.

Fix

Identity resolution now wraps both endpoints obtained from the accepted socket in a WorkloadProxyTcpConnection and passes that concrete type through the CONNECT and forward-proxy paths. Naming the workload and proxy endpoints makes their ordering explicit at construction and prevents adjacent SocketAddr arguments from being accidentally reversed.

The procfs lookup matches the complete workload-to-proxy tuple:

(peer IP, peer port) -> (proxy IP, proxy port)

It checks both IPv4 and IPv6 tables and normalizes IPv4-mapped IPv6 addresses before comparison. Table selection and address decoding use the explicit ProcTcpAddressFamily::{Ipv4, Ipv6} enum rather than an ipv6 boolean, so the family is readable and type-checked throughout the parser.

A synthetic regression test places two entries on the same local port and verifies that the remote endpoint selects the correct inode; the live hot-swap and shared-owner tests now also use the named full tuple.

Compatibility and failure behavior

Valid connections in OpenShell's supported proxy topologies should continue to resolve. The workload receives an explicit proxy URL and connects directly either to the host-side veth address (10.200.0.1:<port>) or to the sidecar/loopback proxy. There is no endpoint translation between the client socket recorded in /proc and the accepted proxy socket, so the two views should contain the same reversed endpoints. IPv4-mapped IPv6 is handled explicitly.

The intentional behavior change is that a lookup which previously "succeeded" only because it found an unrelated same-port socket will no longer resolve that socket. If no exact tuple exists, identity lookup fails and policy evaluation denies the request rather than falling back to the unsafe port-only match.

A custom or future topology that transparently NATs or rewrites either endpoint between the workload and proxy could therefore fail closed until the resolver is taught how to correlate that topology. OpenShell's current veth, loopback, and sidecar paths do not perform such rewriting, so this is not expected to regress supported deployments. The additional address parsing is negligible compared with the existing /proc scan and executable hashing.

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (not applicable; no deployment or external sandbox lifecycle change)

Additional stress coverage:

  • 200 parallel iterations of the OpenShell CLI unit-test binary
  • 500 parallel iterations of the gateway-interceptors unit-test binary
  • 500 parallel iterations of the server multiplex tests
  • Full unit suites for all four affected crates
  • Full supervisor-network suite after review updates (970 tests)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
@pimlock pimlock added the test:e2e Requires end-to-end coverage label Jul 22, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied for 45d470a. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.


/// Parse `/proc/<pid>/net/tcp` (and `/proc/<pid>/net/tcp6`) to find the socket
/// inode for a given local port.
/// inode for a complete local-to-remote TCP tuple.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Question: Should the (pid, peer, proxy) tuple be a concrete type?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think a small concrete type helps here. I pushed a proposal branch with the change split out so it is easier to inspect: https://github.com/NVIDIA/OpenShell/tree/codex/main-refactor-then-pr2434

Shape:

  • 509e0942 introduces WorkloadProxyTcpConnection::new(workload, proxy) plus Display for the workload -> proxy ordering.
  • ef20b59c reapplies this PR’s fix on top, preserving Piotr as the author, and uses the named connection for the full tuple lookup.

This keeps the ordering explicit at the proxy call sites (workload_addr, proxy_addr) while making the procfs lookup signatures harder to mix up.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is great, let me pull your change into this PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Applied your change, thanks!

elezar and others added 2 commits July 23, 2026 10:25
Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants