Issue #958#976
Open
Suji2007hub wants to merge 2 commits into
Open
Conversation
👷 Deploy request for docmagic1 pending review.Visit the deploys page to approve it
|
👷 Deploy request for docmagic-muneer pending review.Visit the deploys page to approve it
|
Contributor
|
@Suji2007hub is attempting to deploy a commit to the muneerali199's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR adds graceful HTTP server shutdown functionality to the Go backend, including two reference server implementations with signal handling, configurable timeouts, comprehensive tests, and documentation. The primary implementation is in ChangesGo Backend Graceful Shutdown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
backend/go/cmd/server/main_test.go (1)
139-143: ⚡ Quick winAccepting
nilfromServemay hide unexpected behavior.Similar to the previous test, this accepts
nilas a valid return value fromsrv.Serve(). According to Go'shttp.Serverdocumentation,Servealways returns a non-nil error and should returnhttp.ErrServerClosedafterShutdownis called.♻️ Proposed fix to strictly check for ErrServerClosed
if serveErr := <-serverErr; serveErr != http.ErrServerClosed { - if serveErr != nil { - t.Fatalf("Serve error: %v", serveErr) - } + t.Fatalf("expected ErrServerClosed, got: %v", serveErr) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/go/cmd/server/main_test.go` around lines 139 - 143, The test currently allows a nil value from srv.Serve by only failing when serveErr != http.ErrServerClosed && serveErr != nil; change the check to strictly require serveErr == http.ErrServerClosed by replacing that conditional with a single check that fails when serveErr != http.ErrServerClosed (e.g., using the serverErr channel value), referencing srv.Serve and http.ErrServerClosed so the test fails on any unexpected non-ErrServerClosed value instead of silently accepting nil.backend/go/cmd/server/main.go (1)
29-38: ⚡ Quick winPotential race condition between server startup and signal handler setup.
If
ListenAndServefails immediately (e.g., port already in use), the goroutine will callos.Exit(1)at line 33, potentially before the signal handler is registered at line 38. While this might be acceptable behavior (the program should exit if the server can't start), it creates a race condition where the program might exit without the signal handler ever being installed.Consider one of these approaches:
- Use a channel to signal successful server startup before setting up the signal handler
- Set up the signal handler before starting the server
- Document this race as intended behavior if it's acceptable
♻️ Proposed fix to synchronize startup
func run() { srv := &http.Server{ Addr: ":8080", Handler: routes(), ReadTimeout: 15 * time.Second, WriteTimeout: 15 * time.Second, IdleTimeout: 60 * time.Second, } + // Set up signal handler before starting server to avoid race + sigChan := make(chan os.Signal, 1) + signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) + go func() { log.Println("Server starting on :8080") if err := srv.ListenAndServe(); err != nil && err != http.ErrServerClosed { log.Printf("ListenAndServe error: %v", err) os.Exit(1) } }() - sigChan := make(chan os.Signal, 1) - signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) - sig := <-sigChan log.Printf("Signal: %v - initiating shutdown", sig)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/go/cmd/server/main.go` around lines 29 - 38, There is a race between starting the server goroutine (srv.ListenAndServe) and registering the signal handler (sigChan/signal.Notify); fix by installing the signal handler first and synchronizing server startup failures via an error channel: create a startupErrChan (chan error), call signal.Notify(sigChan, ...) before launching the goroutine, have the goroutine send any ListenAndServe error into startupErrChan (instead of calling os.Exit), then in main select on startupErrChan and sigChan to handle immediate startup failures (call os.Exit(1) on error) or graceful shutdown on signals; reference srv.ListenAndServe, sigChan, signal.Notify and os.Exit in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/go.mod`:
- Line 3: The go directive in backend/go.mod currently sets an EOL Go version
("go 1.21"); update that directive to a supported release (e.g., "go 1.26" or
"go 1.26.4") to restore security updates, then run module housekeeping (e.g., go
mod tidy) and update any CI/build configs that pin Go (GitHub Actions,
Dockerfiles, build scripts) to the same new version so builds remain consistent;
locate the directive by opening backend/go.mod and change the line containing
"go 1.21".
In `@backend/go/cmd/server/main_test.go`:
- Around line 119-129: The test has a race: the goroutine calling client.Get may
not have actually sent the request before srv.Shutdown is invoked (symbols:
client.Get, srv.Shutdown, ln.Addr(), shutdownTimeout, shutdownStart), so
synchronize the request start before calling Shutdown; for example, create a
start signal (channel or sync.WaitGroup) that the goroutine closes/signals
immediately before performing client.Get (or that the server handler signals
when it begins handling), wait for that signal in the main test goroutine, then
call srv.Shutdown(ctx) and record shutdownStart — this guarantees the request is
in-flight when Shutdown is invoked.
- Around line 82-87: The current tests accept a nil error from the serverErr
channel which can mask unexpected Serve failures; update both checks (the server
err read in TestGracefulShutdown and the analogous check in
TestGracefulShutdownTimeout) to treat only http.ErrServerClosed as the expected
outcome and fail the test for any non-nil, non-http.ErrServerClosed error by
calling t.Fatalf with the actual error; locate the serverErr receive expressions
and replace the nil-accepting branch with a strict comparison against
http.ErrServerClosed and an explicit failure for other errors.
In `@backend/go/cmd/server/main.go`:
- Around line 72-83: The handler registered with mux.HandleFunc (the /api/echo
closure) directly interpolates the msg query param using fmt.Fprintf, causing
JSON injection; change it to build a proper Go value (e.g., a struct or map like
map[string]string{"echo": msg}) and write it with encoding/json
(json.NewEncoder(w).Encode or json.Marshal + w.Write) after setting
Content-Type, so the msg is correctly escaped/encoded; replace the fmt.Fprintf
call in the handler with the JSON encoder approach to eliminate injection.
In `@backend/main.go`:
- Around line 33-35: The signal channel created as sigChan is registered with
signal.Notify but never unregistered, risking a notifier leak during shutdown;
after the call to signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) add a
deferred call to signal.Stop(sigChan) so the runtime stops delivering signals to
that channel when shutdown begins (i.e. place defer signal.Stop(sigChan)
immediately after the signal.Notify invocation that references sigChan).
In `@backend/README.md`:
- Around line 23-25: The README.md contains fenced code blocks showing log
output that lack language identifiers (MD040); update each fenced block that
contains log lines like "2026/06/06 10:23:45 [INFO] Starting server on :8080"
and the subsequent shutdown log block ("[INFO] Received signal: interrupt..." /
"[INFO] Server shut down gracefully...") to include a language tag such as text
(i.e., replace ``` with ```text) so markdownlint passes and the code fences are
explicit.
- Around line 23-38: Update the README examples to match backend/main.go: change
the log example to the actual logger format used by the server (include the
date/time and “[INFO]” prefix as emitted by the code), correct the server
address to use the inline Addr ":8080" used in main.go, replace the placeholder
shutdown timeout in the docs with the actual shutdownTimeout constant value, and
fix the Windows shutdown command by removing the invalid "/SIGTERM" flag
(suggest using taskkill /PID <pid> /T or PowerShell Stop-Process -Id <pid> for
Windows); apply the same edits to the repeated section (lines 66-78).
---
Nitpick comments:
In `@backend/go/cmd/server/main_test.go`:
- Around line 139-143: The test currently allows a nil value from srv.Serve by
only failing when serveErr != http.ErrServerClosed && serveErr != nil; change
the check to strictly require serveErr == http.ErrServerClosed by replacing that
conditional with a single check that fails when serveErr != http.ErrServerClosed
(e.g., using the serverErr channel value), referencing srv.Serve and
http.ErrServerClosed so the test fails on any unexpected non-ErrServerClosed
value instead of silently accepting nil.
In `@backend/go/cmd/server/main.go`:
- Around line 29-38: There is a race between starting the server goroutine
(srv.ListenAndServe) and registering the signal handler (sigChan/signal.Notify);
fix by installing the signal handler first and synchronizing server startup
failures via an error channel: create a startupErrChan (chan error), call
signal.Notify(sigChan, ...) before launching the goroutine, have the goroutine
send any ListenAndServe error into startupErrChan (instead of calling os.Exit),
then in main select on startupErrChan and sigChan to handle immediate startup
failures (call os.Exit(1) on error) or graceful shutdown on signals; reference
srv.ListenAndServe, sigChan, signal.Notify and os.Exit in your changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f67b2ca-32a3-4ad7-9912-de0406a9a776
📒 Files selected for processing (6)
TODO.mdbackend/README.mdbackend/go.modbackend/go/cmd/server/main.gobackend/go/cmd/server/main_test.gobackend/main.go
Muneerali199
added
type:docs
Owner
|
@Suji2007hub please rebase on latest main to resolve conflicts |
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.
Summary by CodeRabbit
New Features
Documentation
Tests