Skip to content

Kill the docker probe when its wait times out - #47

Merged
ptr727 merged 2 commits into
developfrom
verify-tests-kill-probe
Aug 3, 2026
Merged

Kill the docker probe when its wait times out#47
ptr727 merged 2 commits into
developfrom
verify-tests-kill-probe

Conversation

@ptr727

@ptr727 ptr727 commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Answers a suppressed Copilot finding raised on the develop to main promotion PR #45, on its head 0335970. That review carried no inline comment, so the PR read as having nothing outstanding.

The finding

PhotoCleanerTests/VerifyTaskTests.cs:502 - In ImmichImageAvailable(), if docker image inspect hangs and WaitForExit times out, the method returns false without terminating the child process. That can leak a running docker process into the test run and potentially affect subsequent tests.

Correct. The helper starts docker image inspect and returns false on a 60-second timeout. The using disposes the Process, but disposal frees the handles rather than ending the child, so the probe outlives the test that started it and keeps running alongside the rest of the suite.

Verified rather than assumed

Process.Dispose semantics are easy to state wrongly, so a standalone probe checked them instead. A child started the same way and left to Dispose alone was still present in /proc after the using block exited. The same child, killed first, was gone.

kill=False child pid <redacted> still running after dispose: True
kill=True  child pid <redacted> still running after dispose: False

The fix

Kill(entireProcessTree: true) runs before the false return. The InvalidOperationException catch covers the child exiting between the timeout expiring and the kill landing, which is a real race rather than defensive padding. A Win32Exception from the kill falls through to the existing catch on the enclosing block, which already returns false.

Verification

  • CSharpier Format, .NET Build, dotnet format style --verify-no-changes: clean, 0 of 57 files, 0 warnings.
  • dotnet husky run: pass.
  • dotnet test: 372 passed, 0 failed, 0 skipped. The zero matters here: the Immich image is present on this host, so the Docker-gated tests ran rather than skipping, which means this helper was actually exercised.

ImmichImageAvailable started `docker image inspect` and returned false on a
60-second timeout without ending the child. Disposing a Process frees its
handles only, so the probe outlived the test that started it and kept running
alongside the rest of the suite.

Confirmed the semantics with a standalone probe rather than assuming them: a
child started the same way and left to Dispose was still present in /proc after
the using block exited, and the same child was gone when Kill ran first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 43.37%. Comparing base (0335970) to head (0f49a28).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop      #47   +/-   ##
========================================
  Coverage    43.37%   43.37%           
========================================
  Files           28       28           
  Lines         3896     3896           
  Branches       306      306           
========================================
  Hits          1690     1690           
  Misses        2154     2154           
  Partials        52       52           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Updates the Docker preflight helper used by VerifyTaskTests to ensure a timed-out docker image inspect probe doesn’t leave a stray docker process running during the test suite.

Changes:

  • When WaitForExit(60_000) times out, the probe now attempts to Kill(entireProcessTree: true) before returning false.
  • Adds comments explaining why disposal alone is insufficient in this scenario.

Comment thread PhotoCleanerTests/VerifyTaskTests.cs
Kill signals termination rather than performing it, so the previous commit
still returned while the child was alive. A bounded WaitForExit after the kill
is what makes the child gone by the time the helper returns.

The early return also left the two ReadToEndAsync drains unobserved. The kill
closes their pipes, so a bounded WaitAll finishes them, and the AggregateException
catch observes a drain that faulted on the closing pipe.

Measured the asynchrony rather than reasoning about it: across five trials a
killed child was still present in /proc when Kill returned, and gone after the
bounded wait in every one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 3, 2026 19:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@ptr727
ptr727 merged commit 83f5101 into develop Aug 3, 2026
14 checks passed
@ptr727
ptr727 deleted the verify-tests-kill-probe branch August 3, 2026 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants