Fix: Handle already-stopped containers gracefully in kill command by mensfeld · Pull Request #8 · mensfeld/code-on-incus

mensfeld · 2026-01-13T08:43:03Z

Summary

Fixes flaky test kill_all_no_containers by improving the kill command to handle already-stopped and nonexistent containers gracefully.

Problem

The kill command failed when encountering containers in unexpected states:

Tried to stop already-stopped containers → error
Treated nonexistent containers as "successfully killed"
Exit code 1 instead of expected 0 for cleanup operations

This caused the kill_all_no_containers test to be flaky when leftover containers (like coi-build) existed in stopped state.

Solution

Two improvements to internal/cli/kill.go:

Check container exists first (before any operations)
- Verify container exists using mgr.Exists()
- Show warning and skip if not found
- Prevents misleading "successfully killed" messages
Skip stop for already-stopped containers
- Check if container is running with mgr.Running()
- Only call Stop() if actually running
- Prevents "already stopped" errors from Incus

Testing

New integration test:

tests/kill/kill_stopped_container_gracefully.py
Launches container → stops it → kills stopped container
Verifies kill succeeds without errors

All 10 kill tests pass locally:

$ pytest tests/kill/ -v
...
tests/kill/kill_no_args.py::test_kill_no_args PASSED
tests/kill/kill_running_container.py::test_kill_running_container PASSED
tests/kill/kill_force_flag.py::test_kill_with_force_flag PASSED
tests/kill/kill_multiple_containers.py::test_kill_multiple_containers PASSED
tests/kill/kill_nonexistent_container.py::test_kill_nonexistent_container PASSED
tests/kill/kill_idempotent.py::test_kill_idempotent PASSED
tests/kill/kill_all_no_containers.py::test_kill_all_no_containers PASSED
tests/kill/kill_stopped_container_gracefully.py::test_kill_stopped_container_gracefully PASSED
tests/kill/kill_all_with_force.py::test_kill_all_with_force PASSED
tests/kill/kill_stopped_container.py::test_kill_stopped_container PASSED

============================= 10 passed in 53.10s ==============================

Fixed tests:

✅ kill_all_no_containers (previously flaky - failed with leftover stopped containers)
✅ kill_nonexistent_container (previously failed - incorrectly reported success)
✅ kill_stopped_container_gracefully (new test)

Files Changed

internal/cli/kill.go - Added existence check and running state check before operations
tests/kill/kill_stopped_container_gracefully.py - New integration test (85 lines)

Benefits

More robust handling of edge cases (stopped/nonexistent containers)
Eliminates flaky test failures during cleanup operations
Better error messages for users
No breaking changes to existing behavior

- Check if container exists before attempting to kill - Skip stop operation if container is already stopped - Add integration test for killing stopped containers - Fixes flaky test: kill_all_no_containers This prevents errors when kill command encounters stopped or nonexistent containers, which can happen during cleanup operations or when containers are in unexpected states.

After adding 10 tmux tests in PR #9, CI runs started experiencing flaky failures in unrelated shell/attach tests. The tests would timeout waiting for prompt, showing 'no sessions' output from tmux. Root cause: The tmux tests create sessions (inside containers) but leave the host tmux server in a state that affects subsequent tests. When running 192+ tests in sequence, this accumulated state causes race conditions in prompt detection. Solution: Kill the tmux server after each test cleanup to ensure pristine state. This prevents cross-test pollution without affecting test isolation since each test uses unique container names. Fixes flaky failures in: - tests/attach/auto_attach_single_session.py - tests/shell/ephemeral/new_session_with_resume_ephemeral.py

…upgrade Applies the max-effort /code-review findings on the prior commit: - Anchored port match (#1/#7): replace `grep ':4444'` / `grep 'ESTAB.*:4444'` substring checks with exact `ss` state+port filters (`ss -Htln 'sport = :PORT'`, `ss -Htn state established '( sport = :PORT or dport = :PORT )'` + `grep -q .`). The old substring matched ephemeral ports 44440-44449 and IPv6 4444 hextets, which could flip established=True on a connect flake and misfire the "detector miss" assertion. One idiom now (grep -q + returncode) for both predicates. - TimeoutExpired safety (#2): `_incus_bash` catches subprocess.TimeoutExpired and returns None ("not ready"); predicates guard for it. The establish gate no longer adds an uncaught-timeout hard-ERROR surface to the detection path. - Establish/poll split (#3/#4): Phase 1 retries only the CONNECT (bounded) and confirms ESTABLISHED; Phase 2 polls ONCE with the full 75s budget. Drops the sticky `established` misdiagnosis and stops connect-retries from shrinking the detection-latency window below the old 75s. - Skip-vs-fail consistency (#5): an in-loop server restart that fails now pytest.skips (environmental), matching the initial start. - Parameterize + reuse (#6): helpers take (container, port); the metadata sibling drops its condemned `time.sleep(2)` for the same wait-for-listening. - Efficiency (#8/#9/#10): server-start unified into the loop (no redundant back-to-back probe); establish-wait polls at 2s (monitor's interval); break on first establish (no redundant extra connects). Pre-existing, out of scope: _poll_network_threats returning on the first network event, and the `"4444" in json.dumps` substring assert.

…ect+detect) (#615) * test(monitoring): retry the TCP connect+detect cycle to fix a flaky detection test test_suspicious_tcp_port_detected intermittently failed with 'Expected network threat for TCP:4444, got none' under CI load. The suspicious connection is fire-and-forget, so a transiently failed connect() (server race / slow python3 startup) leaves no ESTABLISHED socket to detect, and the host-side /proc-net poller can also miss a single detection window. Re-issue the connection and poll again up to 3 times (fresh connect each attempt); any detection passes. No change to what the test asserts. Prior commits widened the poll window (40s->75s) and added a wait-for-listen; this addresses the residual fire-and-forget/poll-window race directly. * test(monitoring): verify ESTABLISHED before polling (review follow-up) Applies the /code-review findings on the retry hardening: - Verify the in-container connect() actually reached ESTABLISHED (ss ESTAB on :4444) before polling for detection. This distinguishes the two failure modes the blind retry conflated: a failed connect is retried; an ESTABLISHED-but- undetected connection now fails as a real detector miss with a precise message (findings #1, #5). - Re-ensure the port-4444 server is listening each attempt, restarting it if it died between attempts (finding #4). - Track the fire-and-forget connect processes and terminate them in `finally` so up to three `incus exec ... sleep(120)` sessions don't linger (finding #3). - Per-attempt poll trimmed to 30s (3x40s worst case unchanged from the retry version) so runtime doesn't regress (finding #2). Extracted small helpers (_start_http_server_4444 / _wait_port4444_listening / _port4444_listening / _port4444_established) shared by the initial setup and the retry loop. Test-only; no product code touched. * test(monitoring): fix defects found reviewing the ESTABLISHED-verify upgrade Applies the max-effort /code-review findings on the prior commit: - Anchored port match (#1/#7): replace `grep ':4444'` / `grep 'ESTAB.*:4444'` substring checks with exact `ss` state+port filters (`ss -Htln 'sport = :PORT'`, `ss -Htn state established '( sport = :PORT or dport = :PORT )'` + `grep -q .`). The old substring matched ephemeral ports 44440-44449 and IPv6 4444 hextets, which could flip established=True on a connect flake and misfire the "detector miss" assertion. One idiom now (grep -q + returncode) for both predicates. - TimeoutExpired safety (#2): `_incus_bash` catches subprocess.TimeoutExpired and returns None ("not ready"); predicates guard for it. The establish gate no longer adds an uncaught-timeout hard-ERROR surface to the detection path. - Establish/poll split (#3/#4): Phase 1 retries only the CONNECT (bounded) and confirms ESTABLISHED; Phase 2 polls ONCE with the full 75s budget. Drops the sticky `established` misdiagnosis and stops connect-retries from shrinking the detection-latency window below the old 75s. - Skip-vs-fail consistency (#5): an in-loop server restart that fails now pytest.skips (environmental), matching the initial start. - Parameterize + reuse (#6): helpers take (container, port); the metadata sibling drops its condemned `time.sleep(2)` for the same wait-for-listening. - Efficiency (#8/#9/#10): server-start unified into the loop (no redundant back-to-back probe); establish-wait polls at 2s (monitor's interval); break on first establish (no redundant extra connects). Pre-existing, out of scope: _poll_network_threats returning on the first network event, and the `"4444" in json.dumps` substring assert.

mensfeld force-pushed the fix/kill-stopped-containers branch 2 times, most recently from 0e93dc1 to b320df4 Compare January 13, 2026 08:58

mensfeld added 3 commits January 13, 2026 10:01

Merge branch 'master' into fix/kill-stopped-containers

400f92d

Merge branch 'master' into fix/kill-stopped-containers

7f4bafe

mensfeld merged commit 4f951f0 into master Jan 13, 2026
3 checks passed

mensfeld deleted the fix/kill-stopped-containers branch January 13, 2026 10:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Handle already-stopped containers gracefully in kill command#8

Fix: Handle already-stopped containers gracefully in kill command#8
mensfeld merged 4 commits into
masterfrom
fix/kill-stopped-containers

mensfeld commented Jan 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mensfeld commented Jan 13, 2026

Summary

Problem

Solution

Testing

Files Changed

Benefits

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant