diff --git a/CLAUDE.md b/CLAUDE.md index 41c0334..f2cdba9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -590,6 +590,62 @@ Source layout (all under `src/`): `--list-networks`/`--list-volumes`/`ps` all confirmed clean afterward (only the pre-existing, unrelated `test-preexisting-net`/other real volumes remained, untouched). + + **Two real bugs found and fixed running this same test on the actual + Android target device, not assumed** (both in `is_network_in_use()`/ + its caller — `network_bridge.{h,cpp}`/`commands.cpp` — never in this + test file itself, which needed no changes beyond gaining better + diagnostics — see below): + 1. `is_network_in_use()` treated any nonzero exit from `ip -o link + show master ` as "the check itself failed" and failed + closed (assumed in use). But the device's own minimal `ip` build + exits **1**, not 0, for the "bridge exists, nothing attached" case + this project's dev machine reports as exit 0 with identical (empty) + output — confirmed by direct on-device inspection + (`nsenter --net=... -- ip -o link show master ` returned + empty stdout with exit 1 for a bridge that genuinely had nothing + attached). Fixed by splitting into two checks: first confirm the + bridge device itself is reachable at all (`ip link show `, + unfiltered — fail closed only if *that* fails or is empty), then + decide "in use" purely from whether the *filtered* membership + query's own output is non-empty, regardless of its exit code. + 2. Even with that fixed, the device still failed the same way: + `-d/--down` checked `is_network_in_use()` mere milliseconds after + `kill_session()` returned, and it was still correct — the tap + device genuinely hadn't been detached from the bridge yet. + Root cause: `kill_session()` only waits for the *sandboxed + process itself* (via its cgroup) to die; the separate, + independently scheduled daemonized process that started it + (running `run_mounted_container()`, blocked in its own `waitpid()` + on bwrap) still has its own post-exit cleanup left to run + (`stop_tap_relay()` among it) before a tap+relay join's host-side + device is actually removed from the bridge — confirmed in the + device's own debug log, where the kill and the "still in use" + check landed single-digit milliseconds apart. Fixed with a new + `network_becomes_unused()` (`commands.cpp`, `.cpp`-local), + retrying `is_network_in_use()` for up to 10s + (`nanosleep()`-based, matching this project's existing polling + style) instead of giving up on the very first still-attached + answer. + + **Diagnostics added while chasing this, kept permanently**: both + `-u`/`-d`'s own captured stdout (`CapturedStdout` otherwise silently + swallows `compose_up_command()`/`compose_down_command()`'s own + `fmt::print()`/`spdlog` output — spdlog's default sink is stdout, same + as this project's plain status lines) are now attached via Catch2's + `INFO()`, which only actually prints alongside a failing assertion in + the same scope — without this, the device failure would have had no + diagnostic trail at all. The port-forward reply check's own + `REQUIRE(reply.has_value())` was also softened to `CHECK` -- a flaky + reply on one run must never skip the `-d/--down` cleanup below it. + + **Verified end to end on the real Android target device itself** + (root, over SSH — see `reference_device_ssh_access.md`): after both + fixes, the full `-u`/`-d` lifecycle test passes cleanly (18 + assertions), and the entire `[integration][root][net]` suite (73 + assertions, 14 test cases, including this one) passes with nothing + regressed. `--list-networks`/`--list-containers`/`ps` all confirmed + clean afterward. - `tests/support/fixtures.{h,cpp}` — `g_test_app_config` (a plain `AppConfig` global, default-constructed) is set once by `run_self_tests()` (`self_test.cpp`, see above) from the current `-t` run's own effective,