fix(upstream): use parent context in Docker recovery timer checks #127
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
Complete the fix from PR #126 by using parent context consistently throughout handleDockerUnavailable. This eliminates the race condition where child context cancellation lags parent by nanoseconds. Changes: - Line 1841: Changed recoveryCtx.Done() -> ctx.Done() in main select - Line 1846: Changed recoveryCtx.Done() -> ctx.Done() after timer fires This ensures immediate shutdown detection when tests complete, preventing "Log in goroutine after test has completed" panics. Fixes the persistent test failure in TestUpdateListenAddressValidation on macOS. Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
📦 Build Artifacts for PR #127Version: Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 19059587472 --repo smart-mcp-proxy/mcpproxy-go
# Or download a specific artifact:
gh run download 19059587472 --name dmg-darwin-arm64
|
Dumbris
added a commit
that referenced
this pull request
Nov 4, 2025
…ecks Replace select-default pattern with ctx.Err() checks to eliminate TOCTOU race condition in Docker recovery monitoring goroutines. The select-default pattern creates a race window where context can be cancelled AFTER the check passes but BEFORE logging happens, causing "Log in goroutine after test has completed" panics. Using ctx.Err() provides atomic context cancellation checking without the race condition. Changes: - handleDockerUnavailable: Use ctx.Err() for all context checks before logging - startDockerRecoveryMonitor: Use ctx.Err() in initial check and periodic monitoring - Added ctx.Err() check before "Docker recovery successful" logging Fixes the persistent test failure in TestUpdateListenAddressValidation that survived PR #126 and PR #127. 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
Complete the fix from PR #126 by using parent context consistently throughout
handleDockerUnavailable. This eliminates the race condition where child context cancellation lags parent by nanoseconds.Root Cause
PR #126 fixed line 1905 to use parent
ctxinstead of childrecoveryCtx, but lines 1841 and 1846 still usedrecoveryCtx. This created a race condition:ctxgets cancelledrecoveryCtx.Done()at line 1846Changes
recoveryCtx.Done()→ctx.Done()in main select statementrecoveryCtx.Done()→ctx.Done()after timer firesTest Results
Ran
TestUpdateListenAddressValidation3 times locally - all passed without panic.Impact
Fixes persistent test failures on macOS in workflow runs:
Generated with Claude Code