fix(upstream): prevent goroutine logging after test completion in Docker recovery #122
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ker recovery Fixed a race condition where Docker recovery monitor goroutines would log after tests completed, causing panic: "Log in goroutine after test has completed". The issue occurred when: 1. Tests create a Runtime with upstream manager 2. Docker recovery monitor starts a goroutine 3. Test completes and calls Close(), canceling context 4. Goroutine wakes from timer and logs without checking context Changes: - Added context check in handleDockerUnavailable after timer fires - Added context checks in reconnection goroutine before logging - Ensures all goroutines respect context cancellation Fixes test failures in: - TestUpdateListenAddressValidation (internal/runtime) - All runtime tests with race detection 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
📦 Build Artifacts for PR #122Version: Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 19045236081 --repo smart-mcp-proxy/mcpproxy-go
# Or download a specific artifact:
gh run download 19045236081 --name dmg-darwin-arm64
|
Dumbris
added a commit
that referenced
this pull request
Nov 3, 2025
…n Windows Fixed Windows test failures by adding context checks before logging in startDockerRecoveryMonitor. This prevents goroutines from logging after test completion. The issue occurred when: 1. Tests create a Runtime with upstream manager 2. Docker recovery monitor checks availability and finds Docker unavailable 3. Test completes and calls Close(), canceling context 4. Goroutine logs warning synchronously without checking context cancellation Changes: - Add context check before logging in initial Docker availability check - Add context check before logging in periodic monitoring ticker - Ensures goroutines respect context cancellation on all platforms Fixes Windows test failure: - Unit Tests (windows-latest, 1.23.10) https://github.com/smart-mcp-proxy/mcpproxy-go/actions/runs/19045548056/job/54392774796 Complements PR #122 which fixed the same issue on macOS/Linux. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Dumbris
added a commit
that referenced
this pull request
Nov 3, 2025
…ecovery retry Fixed another goroutine leak where Docker recovery retry logging occurred after test completion, causing panic: "Log in goroutine after test has completed". The issue occurred in handleDockerUnavailable when: 1. Timer fires and checkDockerAvailability is called 2. Docker check fails (returns error) 3. Test completes and context is cancelled 4. Goroutine logs "Docker recovery retry" without checking context This completes the fix for Docker recovery monitor logging issues by adding context checks before ALL logging statements. Changes: - Add context check before "Docker recovery retry" log (line 1896) - Ensures goroutine exits cleanly if context was cancelled during check Fixes test failures: - End-to-End Tests (macos-latest, 1.23.10) https://github.com/smart-mcp-proxy/mcpproxy-go/actions/runs/19046210469 Complements PR #122 and PR #123. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Dumbris
added a commit
that referenced
this pull request
Nov 3, 2025
…ellation Fixed OAuth event monitor goroutine leak where logging occurred after test completion, causing panic: "Log in goroutine after test has completed". The issue occurred in startOAuthEventMonitor when: 1. Ticker fires (every 5 seconds) 2. processOAuthEvents() is called 3. Test completes and context is cancelled 4. processOAuthEvents() logs debug message without checking context This is similar to the Docker recovery monitor issues fixed in PR #122, #123, #124. Changes: - Add context check after ticker fires, before calling processOAuthEvents() - Return immediately if context was cancelled during ticker wait - Prevents goroutine from logging after test cleanup Fixes test failures: - Unit Tests (windows-latest, 1.23.10) https://github.com/smart-mcp-proxy/mcpproxy-go/actions/runs/19046681617/job/54396553672 Error: ``` panic: Log in goroutine after TestUpdateListenAddressValidation has completed: 2025-11-03T19:28:45.714Z DEBUG processOAuthEvents: checking for OAuth completion events... goroutine 144 [running]: mcpproxy-go/internal/upstream.(*Manager).processOAuthEvents(0xc0006b64d0) D:/a/mcpproxy-go/mcpproxy-go/internal/upstream/manager.go:1493 +0x9e mcpproxy-go/internal/upstream.(*Manager).startOAuthEventMonitor(0xc0006b64d0, {0x1414d55f8, 0xc000600a50}) D:/a/mcpproxy-go/mcpproxy-go/internal/upstream/manager.go:1474 +0x1e8 ``` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Dumbris
added a commit
that referenced
this pull request
Nov 4, 2025
…utdown ## Problem After PR #129, tests were still failing with "panic: Log in goroutine after test completed" on macOS. The `ctx.Err()` checks prevented the race between checking context and logging, but goroutines could still be running when tests completed because there was no synchronization mechanism. ## Root Cause The Manager struct had `shutdownCtx` and `shutdownCancel` for signaling shutdown, but **no `sync.WaitGroup` to track background goroutines**. When `ShutdownAll()` or `DisconnectAll()` was called: 1. Context was cancelled 2. Function returned immediately 3. Background goroutines (OAuth monitor, Docker recovery) were still running 4. Test completed and marked `testing.T` as done 5. Goroutines tried to log → panic ## Solution Added proper goroutine lifecycle management: 1. **Manager.shutdownWg** field - Tracks all background goroutines 2. **Goroutine launches** - Call `shutdownWg.Add(1)` before spawning: - `startOAuthEventMonitor` (in NewManager) - `startDockerRecoveryMonitor` (in NewManager and dynamic restart) 3. **Goroutine exits** - Call `defer shutdownWg.Done()` in both monitor functions 4. **Shutdown waiting** - Call `shutdownWg.Wait()` in: - `ShutdownAll()` - with 3-second timeout - `DisconnectAll()` - blocking wait ## Changes - `Manager` struct: Added `shutdownWg sync.WaitGroup` - `NewManager()`: Added `Add(1)` before launching monitors - `startOAuthEventMonitor()`: Added `defer Done()` at start - `startDockerRecoveryMonitor()`: Added `defer Done()` at start - Dynamic recovery restart: Added `Add(1)` before `go startDockerRecoveryMonitor()` - `ShutdownAll()`: Cancel context, wait for goroutines with timeout - `DisconnectAll()`: Cancel context, wait for goroutines ## Testing ```bash go test ./internal/runtime -run TestUpdateListenAddressValidation -v -race -count=5 # Result: 5/5 passed, no race conditions go test ./internal/upstream ./internal/runtime -v -race -count=2 # Result: All tests passed ``` ## Related - Fixes persistent issue from PR #122, #126, #127, #129 - Complements the `ctx.Err()` checks added in PR #129 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixed a race condition where Docker recovery monitor goroutines would log after tests completed, causing a panic: "Log in goroutine after test has completed".
Root Cause
The issue occurred when:
handleDockerUnavailable)Close(), canceling the contextChanges
File:
internal/upstream/manager.goAdded context check after timer fires (
handleDockerUnavailable:1823-1828)time.Afterfires, immediately check if context was cancelledAdded context checks in reconnection goroutine (
handleDockerUnavailable:1851-1865)ForceReconnectAllTest Results
✅ All tests pass locally with race detection:
go test -v -race -timeout 2m ./internal/runtime/...✅ Specific failing test now passes:
go test -v -race -run TestUpdateListenAddressValidation ./internal/runtime/...Fixes
Related Issues
Fixes errors from:
🤖 Generated with Claude Code