Document the two real device bugs found running the compose test on-device
Both fixes (is_network_in_use()'s ip-exit-code misread, and the kill_session()-vs-async-cleanup race) plus the INFO()-based diagnostics that made finding them possible are now documented in test_compose_orchestrator.cpp's own CLAUDE.md entry, alongside confirmation that the full [integration][root][net] suite (73 assertions, 14 cases) passes cleanly on the real Android target device. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz
This commit is contained in:
@@ -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 <bridge>` 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 <bridge>` 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 <bridge>`,
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user