diff --git a/tests/integration/test_network_join_scenarios.cpp b/tests/integration/test_network_join_scenarios.cpp index aeaec0b..b6839de 100644 --- a/tests/integration/test_network_join_scenarios.cpp +++ b/tests/integration/test_network_join_scenarios.cpp @@ -137,7 +137,9 @@ private: // *second* container can be started concurrently to interact with it (e.g. // ping it) while it's still running. Waits for a "READY" marker on the // child's own stdout before returning, printed only once the child's own -// script has confirmed its eth actually exists -- join_networks() +// script has confirmed its eth actually has an assigned address (not +// merely exists -- see wait_for_eth0_then()'s own doc comment below for why +// that distinction matters) -- join_networks() // (network_join.cpp) runs *concurrently with*, not before, the sandboxed // command starting (bwrap execs straight into it, with no hook point in // between), so a container that used its interface immediately could @@ -163,8 +165,8 @@ public: args.network_specs = networks; args.hostname_flag = hostname; args.command = {"sh", "-c", - fmt::format("for i in $(seq 1 20); do ip link show eth0 >/dev/null 2>&1 && break; " - "sleep 0.5; done; echo READY; sleep {}", + fmt::format("for i in $(seq 1 20); do ip -4 addr show eth0 2>/dev/null | " + "grep -q 'inet ' && break; sleep 0.5; done; echo READY; sleep {}", lifetime_seconds)}; AppConfig config = reload_config(); dispatch_command(args, "/nonexistent/unused-config.yaml", config); @@ -224,25 +226,51 @@ private: bool ready_ = false; }; -// Prefixes `command_after_eth0` with the same "wait for eth0 to actually -// exist" poll BackgroundPeer's own script above uses, wrapped between the +// Prefixes `command_after_eth0` with the same "wait for eth0 to actually be +// usable" poll BackgroundPeer's own script above uses, wrapped between the // usual BEGIN/END-TEST-OUTPUT markers. Needed for *any* sandboxed command // that uses its network interface at all, not just BackgroundPeer's own -- // join_networks() (network_join.cpp) runs concurrently with, not before, // the sandboxed command starting (bwrap execs straight into it, no hook // point in between), so a command that used eth0 immediately could -// otherwise race its own join. Confirmed by testing, not assumed: an -// earlier version of this file's own tests pinged immediately, and the -// container's own near-instant "ping, fail, exit" (no eth0 yet) sometimes -// raced ahead of join_one_network()'s own veth-move step, which then failed -// outright trying to move a veth into an already-exited container's pid -// ("Invalid netns value") -- the exact documented limitation -// network_join.{h,cpp}'s own CLAUDE.md entry already describes for a -// very-short-lived sandboxed command. +// otherwise race its own join. Confirmed by testing, not assumed, in two +// stages: +// +// 1. An earlier version of this file's own tests pinged immediately, and +// the container's own near-instant "ping, fail, exit" (no eth0 yet) +// sometimes raced ahead of join_one_network()'s own veth-move step, +// which then failed outright trying to move a veth into an +// already-exited container's pid ("Invalid netns value") -- the exact +// documented limitation network_join.{h,cpp}'s own CLAUDE.md entry +// already describes for a very-short-lived sandboxed command. Fixed by +// polling for the interface's own *existence* first (`ip link show +// eth0`). +// +// 2. That alone still wasn't enough: an interface can become visible (the +// device exists, already moved/created) *before* join_one_network()'s +// own later `ip addr add`/`ip link set ... up` steps for it have run. +// A script that only waited for existence could still start pinging +// (getting "Network unreachable", no address yet) and exit almost +// immediately -- and since this sandboxed process is the pid/net +// namespace's own sole occupant (`--unshare-pid`/`--unshare-net`), its +// exit destroys that namespace outright, which then made join_one_ +// network()'s own *remaining* steps for that same network -- or, in one +// observed case, the entirely separate per-session DNS resolver's own +// nsenter call (network_dns.cpp's start_dns_resolver(), also entering +// this same namespace) -- fail with "No such file or directory" against +// a namespace that had already collapsed underneath them. Confirmed via +// a temporary production-code diagnostic that the DNS resolver's own +// namespace lookup was never itself stale (proc_exists was always true +// right up to its own nsenter call), narrowing the cause to the +// sandboxed script's own premature exit, not a namespace-resolution bug. +// Fixed by polling for an actually *assigned address* on eth0 (`ip -4 +// addr show eth0 | grep -q 'inet '`) instead of mere existence -- this +// only becomes true once join_one_network()'s full sequence for that +// interface has completed, so the script no longer outruns its own join. std::string wait_for_eth0_then(const std::string& command_after_eth0) { return fmt::format( - "echo BEGIN-TEST-OUTPUT; for i in $(seq 1 20); do ip link show eth0 >/dev/null 2>&1 && break; " - "sleep 0.5; done; {}; echo END-TEST-OUTPUT", + "echo BEGIN-TEST-OUTPUT; for i in $(seq 1 20); do ip -4 addr show eth0 2>/dev/null | " + "grep -q 'inet ' && break; sleep 0.5; done; {}; echo END-TEST-OUTPUT", command_after_eth0); }