Fix zombie process when proxy stops due to health check failure

[gkatz2]

Summary

• Fix zombie process issue where the ToolHive proxy process continues running after health check failure triggers proxy shutdown
• Add IsRunning() method to the Proxy interface
• Update HTTPTransport.IsRunning() to check proxy state in addition to its own shutdown channel
• Add unit tests and e2e test for the fix

Problem

When health checks fail 3 consecutive times, TransparentProxy.Stop() is called which closes the proxy's shutdown channel. However, HTTPTransport.IsRunning() only checks its own shutdown channel, not the proxy's. This causes the runner's status monitor to keep returning true, preventing the process from exiting and blocking the self-healing restart mechanism.

Solution

HTTPTransport.IsRunning() now also calls proxy.IsRunning() to detect when the proxy has stopped itself due to health check failure. This enables the runner to detect the stopped state and trigger automatic restart.

Test plan

• Unit tests for IsRunning() behavior (pkg/transport/http_test.go)
• E2e test verifying runner detects proxy shutdown (test/e2e/health_check_zombie_test.go)
• Manual testing confirmed fix enables self-healing restart

Fixes #3542

[dmjb]

@gkatz2 Thanks for this change. If you can address the pieces of feedback I have here, I'm happy to merge.

[dmjb]

Looks like you may also need to run task gen locally to re-update some mocks.

[codecov]

Codecov Report

❌ Patch coverage is 41.37931% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.59%. Comparing base (ac82979) to head (48c4dff).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/transport/proxy/httpsse/http_proxy.go	0.00%	6 Missing ⚠️
pkg/transport/proxy/streamable/streamable_proxy.go	0.00%	6 Missing ⚠️
...g/transport/proxy/transparent/transparent_proxy.go	57.14%	2 Missing and 1 partial ⚠️
pkg/runner/runner.go	0.00%	1 Missing ⚠️
pkg/transport/stdio.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3543      +/-   ##
==========================================
+ Coverage   65.51%   65.59%   +0.07%     
==========================================
  Files         406      406              
  Lines       39996    40019      +23     
==========================================
+ Hits        26202    26249      +47     
+ Misses      11762    11736      -26     
- Partials     2032     2034       +2     

☔ View full report in Codecov by Sentry.
📢 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.

[gkatz2]

Thanks, @dmjb. Just pushed a commit for the mock generation issue. Looking at the code suggestion now.

[gkatz2]

Re: Context parameter in IsRunning

You're right - the context isn't used by any of the implementations. I added it for consistency with Start/Stop, but since IsRunning is just checking a channel state (instant, non-blocking), context serves no purpose here. Will remove it.

Re: Code restructure

I think the suggestion has an issue - using := inside the if block creates a new proxyRunning that shadows the outer one, meaning that although the code appears to be using the same instance of the proxyRunning variable throughout, there are actually two. The bug manifests when the proxy has stopped (e.g., due to health check failure) - IsRunning() returns false, nil, the inner proxyRunning is set to false, but when the if block exits, the outer proxyRunning (still true) is returned.

Here are two corrected versions:

Option A - declare at top, single return:

proxyRunning := true
var err error
if t.proxy != nil {
    proxyRunning, err = t.proxy.IsRunning(ctx)
    if err != nil {
        return false, err
    }
}
return proxyRunning, nil

Option B - early return:

if t.proxy != nil {
    proxyRunning, err := t.proxy.IsRunning(ctx)
    if err != nil {
        return false, err
    }
    return proxyRunning, nil
}
return true, nil

Let me know which you prefer and I'll update!

[gkatz2]

@dmjb - I wanted to get your thoughts on one aspect of this PR.

The latest commit adds a TOOLHIVE_HEALTH_CHECK_INTERVAL environment variable to the transparent proxy. This allows the E2E test to use a 1-second health check interval instead of the default 10 seconds, which reduces test runtime from 30+ seconds to ~12 seconds.

I realize this expands the scope a bit - adding production code to support faster E2E testing. A few options:

1. Keep it as-is: Being able to control the health check interval could be valuable for production use cases (e.g., adjusting for slow/unreliable backends, or tightening intervals for faster failure detection)
2. Remove it: I can revert that part and let the E2E test run slower (~30-35s). The test would still be reliable with the other fixes (status-based detection instead of log parsing)

Happy to go either direction based on your preference. What do you think?

[dmjb]

@gkatz2 I think the interval env var is a good idea, let's keep it 👍

[dmjb]

@gkatz2 Thanks for addressing the feedback. I'll merge when the build goes green.

[gkatz2]

@dmjb Implemented the code restructure per your suggestion. I added var err error since using = instead of := requires the variable to be pre-declared.

proxyRunning := true
var err error
if t.proxy != nil {
    proxyRunning, err = t.proxy.IsRunning()
    if err != nil {
        return false, err
    }
}
return proxyRunning, nil

CI is passing.

Update: Oops, didn't see you already merged it! Thanks.