Conversation
… hard shutdown deadline The signal handler goroutine was one-shot: it read one signal, armed a 10s timer, and returned when that timer won the select. signal.Notify stays registered for the life of the process, so once the goroutine was gone the Go runtime kept intercepting SIGINT/SIGTERM (their default disposition is disabled) and dropping them into a one-slot buffer nobody read. If graceful shutdown ran longer than 10s - exactly when an operator is hammering Ctrl+C - the daemon could only be killed with SIGKILL. The logic moves into runSignalHandler (signal_handler.go), behind an injected signal channel, clock and exit func so it is unit-testable. After the first signal it arms both windows and hands the process's fate to a small goroutine that reads only channels: a further signal exits immediately with code 1, and a new 60s hard deadline, measured from the first signal, exits with the new ExitCodeShutdownTimeout (6) so launchd / systemd / the tray's process monitor can restart a wedged daemon. That goroutine never logs, on purpose: the failure that wedges shutdown (a full disk, a tray that stopped draining the core's stderr pipe) is the same failure that wedges the log sink, so a forced exit that had to get past a log write first would not be forced at all. The reason is announced up front instead, with both exit codes in the fields, and the handler goroutine does the remaining reporting. For the same reason the goroutine is now started before runServer's own log line: signal.Notify is already registered by then. The 10s window is demoted to a one-shot reminder; its old "within 10 seconds" wording was a lie once the window never closed. This is also the first upper bound on shutdown in this path - srv.Shutdown() was previously unbounded - so a cleanup that legitimately takes longer than 60s is now cut short with exit 6. Accepted trade-off; 60s sits under systemd's 90s DefaultTimeoutStopSec. Spec 024 is unchanged: receivedSignal is still stored before cancel(), which a test pins, since runServer's ctx.Done() branch type-asserts it unchecked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
4da584b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8310e800.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-force-quit-deadline.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 35748535939 --repo smart-mcp-proxy/mcpproxy-go
|
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What
runServer's signal handler was one-shot: it read one signal, armed a 10s force-quit timer, and returned when that timer won the select.signal.Notifystays registered for the life of the process, so once that goroutine was gone the Go runtime kept intercepting SIGINT/SIGTERM (their default disposition is disabled) and dropping them into a one-slot buffer nobody read.If graceful shutdown exceeds 10s — the precise situation in which an operator is hammering Ctrl+C — the daemon could then only be killed with SIGKILL. On the normal path
cancel()finishes shutdown well inside the window, so this is a hung-shutdown bug, not an "after 10 seconds it always breaks" bug.How
The logic moves into
runSignalHandler(cmd/mcpproxy/signal_handler.go) behind an injected signal channel, clock and exit func. After the first signal it arms both windows and hands the process's fate to one small goroutine that reads only channels:ExitCodeShutdownTimeout(6)runServerreturns and the process exits 0That goroutine never logs, deliberately: the failure that wedges shutdown (a full disk, a tray that stopped draining the core's stderr pipe) is the same failure that wedges the log sink, so a forced exit that had to get past a log write first would not be forced at all. The reason is announced up front instead, with both exit codes as fields. For the same reason the goroutine is started immediately after
signal.Notify, before any logging. The 10s window is demoted to a one-shot reminder — its old "within 10 seconds" wording was a lie once the window never closed.Decisions taken per the Zero Interruption Policy: 60s for the hard deadline (under systemd's 90s
DefaultTimeoutStopSec) and 6 for the exit code (6-9 were free; 10-12 are the Spec 098 preflight band).classifyError/classifyStartupErrorare deliberately untouched — this path callsos.Exitdirectly and exit 6 is not a startup outcome. Downstream, the tray's process monitor andCoreError.fromExitCodeboth fall through to their general-error default, which is the desired "supervisor restarts it".Behavior change worth naming: there was previously no upper bound on shutdown here —
srv.Shutdown()blocks indefinitely — so a cleanup that legitimately takes longer than 60s is now cut short with exit 6. Accepted trade-off.Spec 024 is unchanged:
receivedSignalis still stored beforecancel(), which a test pins, since thectx.Done()branch type-asserts it unchecked.Verified
TestRunSignalHandler_SignalAfterGraceWindowStillHonoredfails against the old one-shot logic (the process was never asked to exit) and passes after. Every later hardening step has its own test that fails against the step before it.go test -race ./cmd/mcpproxy/ -count=5, injected channels only — no real sleeps, no timing assertions.cmd/mcpproxypackage with-race; both golangci-lint v2 passes (bare and--build-tags server) clean for the touched files.codex exec --model gpt-5.6-sol, 5 rounds, final verdict NO BLOCKING ISSUES. Rounds 1-4 each found a genuine gap in the deadline's independence from logging; all were accepted and fixed.Docs: exit code 6 added to the list in
CLAUDE.mdand to the tray's error-classification list indocs/architecture.md.docs/cli-management-commands.md's "Exit Codes" table is a different (subcommand) table and is left alone.🤖 Generated with Claude Code