fix(T-1031): Graceful server shutdown returns http.ErrServerClosed

[ArjenSchwarz]

Fixes Transit ticket T-1031: Graceful server shutdown returns http.ErrServerClosed

[claude]

Code Review

Overview

This PR fixes a cosmetic-but-meaningful bug where Start() on both HTTP servers returned http.ErrServerClosed after a graceful shutdown, rather than nil. The change is a 2-line fix applied consistently across both server packages.

---

The Fix Is Correct

http.ErrServerClosed is a sentinel value from net/http — Go's documentation explicitly states:

After Shutdown or Close, the returned error is ErrServerClosed.

It signals intentional server shutdown, not an actual failure. Both Start() methods already had the correct guard:

if err != nil && err != http.ErrServerClosed {
    return fmt.Errorf("server error: %w", err)
}

But then the old code leaked http.ErrServerClosed through as the return value, which was inconsistent with that intent. Returning nil is the right fix and makes the API contract unambiguous: nil = clean exit, non-nil = actual failure.

---

Caller Analysis

apsisweb caller (cmd/apsis/main.go):

errCh := make(chan error, 1)
go func() { errCh <- server.Start() }()
select {
case sig := <-sigCh:
    return server.Shutdown(ctx)   // Already returned; errCh is drained by buffer
case err := <-errCh:
    return err                     // Now gets nil instead of ErrServerClosed ✓
}

web caller (cmd/orbit/serve.go):

errChan := make(chan error, 1)
go func() { errChan <- server.Start() }()
select {
case sig := <-sigChan:
    return server.Shutdown(ctx)   // Already returned; errChan is drained by buffer
case err := <-errChan:
    return err                     // Now gets nil instead of ErrServerClosed ✓
}

Both callers would have previously propagated http.ErrServerClosed to the user as an error message on graceful exit (e.g., Error: http: Server closed). This is the bug being fixed.

---

Suggestions

1. Tighten existing tests to assert nil return (minor)

Both server test files currently accept http.ErrServerClosed as an acceptable Start() return value after graceful shutdown:

// internal/apsisweb/server_test.go and internal/web/server_test.go
if err != nil && err != http.ErrServerClosed {
    t.Errorf("unexpected server error: %v", err)
}

This condition passes for both nil and http.ErrServerClosed, so it doesn't verify the fix. To lock in the corrected behavior, these could be updated to:

if err != nil {
    t.Errorf("Start() after graceful shutdown should return nil, got: %v", err)
}

Not a blocker, but it would make the tests enforce the intended contract.

2. Pre-existing: double signal handling in internal/web/server.go (out of scope for this PR)

web.Server.Start() registers its own internal signal goroutine that calls Shutdown(), while cmd/orbit/serve.go independently registers the same signals and also calls server.Shutdown(). On SIGINT/SIGTERM, both fire and Shutdown() is called twice. Since http.Server.Shutdown() is safe to call multiple times this doesn't cause a crash, but the layered signal handling is confusing and worth a future cleanup.

---

Summary

• Correctness: ✅ Fix is correct and consistent across both server packages
• Logic: ✅ Caller analysis confirms no unintended behavior changes
• Test coverage: The fix works and existing tests continue to pass, though they could be tightened to assert nil specifically
• Security/performance: No concerns

LGTM with the optional test tightening as a follow-up.

[claude]

Code Review — PR #156: Graceful server shutdown returns http.ErrServerClosed

Note: This PR is already merged. Posting this review for the record.

Overview

Fixes a subtle but real bug in both HTTP server implementations: after a graceful shutdown, ListenAndServe() returns http.ErrServerClosed to signal intentional stop. The old code filtered this from the early-return path but then fell through to return err, inadvertently surfacing http.ErrServerClosed as a non-nil error to callers. The fix returns nil in both locations.

---

Correctness

The fix is correct and aligns with the Go standard library's documented recommendation: callers should treat http.ErrServerClosed as a signal that shutdown was intentional, not as an error condition. The three cases are now handled properly:

ListenAndServe() return value	Before	After
http.ErrServerClosed (graceful shutdown)	Returned as non-nil error to caller ❌	Returns nil ✅
Any other error	Wrapped and returned as error ✅	Unchanged ✅
nil (unusual)	Returned as nil ✅	Unchanged ✅

---

Code Quality

apsisweb/server.go — clean. The comment already says "Does not handle signals — the caller is responsible for calling Shutdown," which accurately describes the pattern. The fix is consistent with this design.

web/server.go — also correct. This server has its own signal handling built into Start(), so Shutdown() is invoked internally. Returning nil after the goroutine-driven graceful shutdown completes is the right behavior.

---

Minor Observations (non-blocking)

• web/server.go has a subtle race: the select { case <-shutdownComplete: default: } after the shutdown path is non-blocking. If ListenAndServe() returns before the goroutine finishes calling s.Shutdown(), the Start() method could return before the graceful drain is complete. This is pre-existing and out of scope for this fix, but worth noting for a follow-up.

• No tests added — the shutdown path is worth covering. An integration test that starts the server, calls Shutdown(), and asserts Start() returned nil would prevent regression. Low priority but nice to have.

---

Summary

Minimal, targeted, and correct. The fix removes one line of subtle confusion in each file with no added complexity. ✅