Conversation
WalkthroughThe changes introduce enhancements to signal handling in the simple responder, centralize the process graceful stop timeout as a configurable field, and improve test modularity and coverage. New command-line flags and helper functions are added, and a new test verifies forced process termination behavior. Minor adjustments are made to logging levels in tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SimpleResponder
participant OS
User->>SimpleResponder: Start with flags (e.g., --ignore-sig-term)
SimpleResponder->>OS: Register signal handlers
OS-->>SimpleResponder: Send SIGINT/SIGTERM/other signals
SimpleResponder->>SimpleResponder: Loop: handle signals
alt SIGINT (first)
SimpleResponder->>SimpleResponder: Log received SIGINT
else SIGINT (second)
SimpleResponder->>SimpleResponder: Shutdown and exit
else SIGTERM and ignore-sig-term=true
SimpleResponder->>SimpleResponder: Ignore SIGTERM
else SIGTERM and ignore-sig-term=false
SimpleResponder->>SimpleResponder: Shutdown and exit
else Other signal
SimpleResponder->>SimpleResponder: Immediate shutdown
end
sequenceDiagram
participant Test
participant Proxy
participant UpstreamProcess
Test->>Proxy: Start process with ignore SIGTERM
Proxy->>UpstreamProcess: Launch command
Test->>Proxy: Send HTTP request
Proxy->>UpstreamProcess: Forward request
Test->>Proxy: Call StopImmediately (short timeout)
Proxy->>UpstreamProcess: Send SIGTERM
UpstreamProcess-->>Proxy: Ignore SIGTERM
Proxy->>UpstreamProcess: After timeout, send SIGKILL
UpstreamProcess-->>Proxy: Terminated by SIGKILL
Proxy->>Test: Responds with partial output and error
Suggested labels
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Makefile (1)
22-27: Flagging test cache disable may lengthen CI runsAdding
-count=1guarantees fresh test execution, which is useful when tests rely on side effects (random ports, file‐system, etc.).
Be aware it also disables Go’s build cache completely, so local/CI cycles will be slower. Consider either:
- Leaving the flag off by default and letting devs run
make test-no-cachewhen needed, or- Guarding it behind an environment toggle (
NO_CACHE=1 make test).No change required if deterministic re-runs outweigh the extra minutes.
misc/simple-responder/simple-responder.go (2)
29-30: Flag description nitpick
ignore-sig-termis clear, but the flag comment could mention why you might need it—e.g. “simulate a mis-behaving child that ignores SIGTERM”. This helps future readers/tests.
209-214: Name shadowing & missing context importInside the run loop you redeclare
signal := <-sigChan, shadowing the importedos/signalpackage. Although legal, it hampers readability and tooling autocomplete.- signal := <-sigChan + sig := <-sigChanAlso, if you adopt the graceful-shutdown suggestion below you’ll need to
import "context".
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Makefile(1 hunks)misc/simple-responder/simple-responder.go(3 hunks)proxy/helpers_test.go(1 hunks)proxy/process.go(4 hunks)proxy/process_test.go(1 hunks)proxy/proxymanager_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
proxy/helpers_test.go (1)
proxy/config.go (1)
ModelConfig(17-29)
🔇 Additional comments (11)
proxy/proxymanager_test.go (1)
337-344:LogLevel: "warn"change looks goodRaising the verbosity from
"error"to"warn"should surface flaky behaviour without flooding output. 👍misc/simple-responder/simple-responder.go (1)
195-198: Nice touch: printing PID only when not silentConditional PID output is handy for tests that need the PID yet keeps logs quiet in CI when
--silentis set. 👍proxy/process.go (4)
70-72: Great addition: Process graceful stop timeout field.Adding a dedicated field for the graceful stop timeout enhances configurability and testing flexibility. This change properly centralizes a previously hardcoded value.
99-100: LGTM: Proper initialization of the graceful stop timeout.The default value of 5 seconds maintains backward compatibility with the previous implementation.
357-357: LGTM: Using the configurable timeout in StopImmediately.Correctly uses the new field instead of the hardcoded timeout value.
370-370: LGTM: Using the configurable timeout in Shutdown.Correctly uses the new field instead of the hardcoded timeout value.
proxy/process_test.go (3)
397-445: Excellent test coverage for the SIGKILL behavior.This test thoroughly verifies the behavior when a process ignores SIGTERM and must be terminated with SIGKILL:
- It uses the
--ignore-sig-termflag to create a process that won't respond to SIGTERM- Sets a short timeout to make testing faster
- Verifies the process state transitions correctly
- Checks that HTTP responses are handled properly during forced termination
The test provides good coverage for issue #125 mentioned in the PR objectives.
417-417: LGTM: Setting a shorter timeout for faster testing.Appropriately reduces the timeout for testing purposes without affecting production behavior.
433-435: Good assertion for verifying the interrupted response.This assertion confirms that the response was interrupted by the SIGKILL signal, validating the end-to-end behavior of the forced termination.
proxy/helpers_test.go (2)
51-59: Good refactoring: Extracting port allocation logic.Extracting the port allocation logic into a dedicated function improves code organization and maintainability. The mutex handling ensures thread safety for concurrent tests.
61-63: Clean implementation: Using the new port allocation function.The refactored implementation properly delegates to the new
getTestPort()function and then togetTestSimpleResponderConfigPort, improving modularity.
| countSigInt := 0 | ||
|
|
||
| runloop: | ||
| for { | ||
| signal := <-sigChan | ||
| switch signal { | ||
| case syscall.SIGINT: | ||
| countSigInt++ | ||
| if countSigInt > 1 { | ||
| break runloop | ||
| } else { | ||
| log.Println("Recieved SIGINT, send another SIGINT to shutdown") | ||
| } | ||
| case syscall.SIGTERM: | ||
| if *ignoreSigTerm { | ||
| log.Println("Ignoring SIGTERM") | ||
| } else { | ||
| log.Println("Recieved SIGTERM, shutting down") | ||
| break runloop | ||
| } | ||
| default: | ||
| break runloop | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Graceful HTTP shutdown missing
Breaking the loop logs “shutting down” but never closes the http.Server, so existing connections may hang until the process is killed. A small addition ensures a tidy exit:
@@
default:
break runloop
}
}
log.Println("simple-responder shutting down")
+
+// attempt graceful shutdown of the HTTP server
+ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+defer cancel()
+if err := srv.Shutdown(ctx); err != nil {
+ log.Printf("server shutdown error: %v\n", err)
+}Remember to add import "context" at the top.
* Add test for SIGKILL of process * silent TestProxyManager_RunningEndpoint debug output * Ref mostlygeek#125
1 participant
No functionality changes. Improving testing for stop timeout and the usage of SIGKILL (#125)
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Other