cmd/wol-proxy: tweak logs to show what is causing wake ups #356
Conversation
WalkthroughReplaces wol-proxy upstream health-check polling with a persistent SSE connection to Changes
Sequence DiagramsequenceDiagram
participant Client
participant Proxy as wol-proxy
participant Upstream
Note over Proxy,Upstream: SSE-based health monitoring (new)
rect rgb(220,240,220)
Proxy->>Upstream: GET /api/events (SSE headers)
Upstream-->>Proxy: 200 OK + persistent stream
Proxy->>Proxy: mark ready, reset failures
end
loop Monitor SSE stream
alt Receive event
Upstream-->>Proxy: SSE event line(s)
Proxy->>Proxy: scan buffer (keep-alive)
else Disconnect / error
Proxy->>Proxy: mark notready
Proxy->>Proxy: retry loop (delay + timeout)
Proxy->>Upstream: reconnect attempt
end
end
par Client requests
Client->>Proxy: request
alt Upstream ready
Proxy->>Upstream: forward request
Upstream-->>Client: response
else Upstream notready
alt Request to /api/events
Proxy-->>Client: 204 No Content / skip wake
else Other request
Proxy->>Proxy: log path + remote addr
Proxy->>Upstream: attempt wake
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (1)
Makefile (1)
93-93: Consider alignment with main app versioning approach.The date suffix provides basic versioning but introduces a few operational considerations:
- Build reproducibility: Builds of the same code on different days produce different filenames, which could complicate deployment automation or scripts that reference the binary by name.
- Inconsistency: The main application binaries (lines 48, 53-54) use ldflags with
${GIT_HASH}and${BUILD_DATE}for versioning metadata embedded in the binary, whilewol-proxyonly uses a filename suffix.If versioning is needed for
wol-proxy, consider either:
- Using ldflags like the main app for consistency
- Or accepting the simpler filename approach if wol-proxy has different deployment requirements
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(1 hunks)cmd/wol-proxy/wol-proxy.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Fix all staticcheck-reported issues in Go code
Files:
cmd/wol-proxy/wol-proxy.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-tests
- GitHub Check: run-tests
🔇 Additional comments (1)
cmd/wol-proxy/wol-proxy.go (1)
178-178: Excellent logging improvement for debugging wake-ups!This change directly addresses the PR objective by logging the request path and remote address when a magic packet is sent. This will help identify patterns in what requests are keeping the LLM machine awake.
The log fields provide actionable information:
"req": Shows which endpoint is being accessed"from": Shows which client is making the request
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
cmd/wol-proxy/wol-proxy.go (7)
97-100: Call stop() for signal.NotifyContext; consider graceful ShutdownNot calling the stop func leaks the signal notification. Prefer Shutdown with a timeout; keep Close only if you intentionally want a hard close.
- // graceful shutdown - ctx, _ := signal.NotifyContext(context.Background(), os.Interrupt) - <-ctx.Done() - server.Close() + // graceful shutdown + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt) + defer stop() + <-ctx.Done() + shutdownCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if err := server.Shutdown(shutdownCtx); err != nil { + slog.Warn("server shutdown error", "error", err) + }
145-148: Drop “Connection: keep-alive”; HTTP/2 ignores itUnnecessary and can confuse intermediaries. SSE will remain open without it.
- req.Header.Set("Connection", "keep-alive")
156-167: Use the existing waitDuration consistentlyTwo hardcoded 10s sleeps diverge from waitDuration.
- time.Sleep(10 * time.Second) + time.Sleep(waitDuration) @@ - time.Sleep(10 * time.Second) + time.Sleep(waitDuration)
176-190: Avoid stdout prints; check scanner.Err; keep logs structuredfmt.Print floods stdout and the trailing fmt.Println runs even when not debugging. Prefer slog and always report scanner.Err.
- events := 0 - if slog.Default().Enabled(context.Background(), slog.LevelDebug) { - fmt.Print("Events: ") - } - for scanner.Scan() { - if slog.Default().Enabled(context.Background(), slog.LevelDebug) { - // Just read the events to keep connection alive - // We don't need to process the event data - events++ - fmt.Printf("%d, ", events) - } - } - fmt.Println() + var events int + debug := slog.Default().Enabled(context.Background(), slog.LevelDebug) + for scanner.Scan() { + if debug { + events++ + if events%100 == 0 { + slog.Debug("SSE events observed", "count", events) + } + } + } + if err := scanner.Err(); err != nil { + slog.Warn("SSE scanner error", "error", err, "events", events) + }
219-224: Log requester when skipping wake-up for SSEAdd remote address (and optionally UA) so you can correlate who is holding the SSE and preventing wake-ups.
- slog.Debug("Skipping wake up", "req", path) + slog.Debug("Skipping wake up", "req", path, "from", r.RemoteAddr, "ua", r.UserAgent())
226-226: Enrich wake-up logs to identify sourcesInclude host, UA, and XFF to pinpoint what is causing wake-ups.
- slog.Info("upstream not ready, sending magic packet", "req", path, "from", r.RemoteAddr) + slog.Info("upstream not ready, sending magic packet", + "req", path, + "from", r.RemoteAddr, + "host", r.Host, + "ua", r.UserAgent(), + "xff", r.Header.Get("X-Forwarded-For"))
230-246: Stop the ticker in all exit pathsTicker leaks on timeout return. Defer Stop ensures cleanup.
- ticker := time.NewTicker(250 * time.Millisecond) + ticker := time.NewTicker(250 * time.Millisecond) + defer ticker.Stop() @@ - if p.getStatus() == ready { - ticker.Stop() + if p.getStatus() == ready {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/wol-proxy/wol-proxy.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Fix all staticcheck-reported issues in Go code
Files:
cmd/wol-proxy/wol-proxy.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-tests
- GitHub Check: run-tests
| eventsUrl := url.Scheme + "://" + url.Host + "/api/events" | ||
| client := &http.Client{ | ||
| Timeout: 0, // No timeout for SSE connection | ||
| } | ||
|
|
||
| slog.Debug("checking upstream status at", "url", checkUrl) | ||
| resp, err := client.Get(checkUrl) | ||
| waitDuration := 10 * time.Second | ||
|
|
||
| // drain the body | ||
| if err == nil && resp != nil { | ||
| _, _ = io.Copy(io.Discard, resp.Body) | ||
| _ = resp.Body.Close() | ||
| for { | ||
| slog.Debug("connecting to SSE endpoint", "url", eventsUrl) | ||
|
|
||
| req, err := http.NewRequest("GET", eventsUrl, nil) |
There was a problem hiding this comment.
Build SSE URL robustly; avoid shadowing net/url; fix initialism
String concat drops any upstream base path and the param name url shadows the net/url package; eventsUrl casing violates ST1003.
- eventsUrl := url.Scheme + "://" + url.Host + "/api/events"
+ // Build from upstream, preserving base path.
+ eventsURL, err := neturl.JoinPath(upstream.String(), "api", "events")
+ if err != nil {
+ slog.Warn("failed to build SSE URL", "error", err)
+ proxy.setStatus(notready)
+ proxy.incFail()
+ time.Sleep(waitDuration)
+ continue
+ }Supporting changes outside this hunk:
- Import alias:
neturl "net/url". - Rename newProxy param to avoid shadowing and use initialism:
func newProxy(upstream *url.URL) *proxyServer { - Update callsite:
proxy := newProxy(upstreamURL) - Rename variable to eventsURL.
Based on coding guidelines (staticcheck ST1003).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/wol-proxy/wol-proxy.go around lines 126-136, the code builds the SSE
endpoint by string concatenation, shadows the net/url package with a variable
named url, and uses non-API-compliant casing for eventsUrl; fix by: rename the
function parameter and local variable to avoid shadowing (e.g.,
newProxy(upstream *url.URL) or similar) and update its callsite, add or use the
neturl import alias if needed (neturl "net/url"), construct the SSE URL robustly
using URL methods (e.g., ResolveReference or URL.JoinPath with the upstream base
so the upstream path and query are preserved) and rename eventsUrl to eventsURL
to follow ST1003; ensure the http.NewRequest uses the computed eventsURL string.
| client := &http.Client{ | ||
| Timeout: 0, // No timeout for SSE connection | ||
| } | ||
|
|
||
| slog.Debug("checking upstream status at", "url", checkUrl) | ||
| resp, err := client.Get(checkUrl) | ||
| waitDuration := 10 * time.Second |
There was a problem hiding this comment.
SSE client can hang indefinitely on connect; set dial/TLS timeouts
Timeout=0 is correct for the stream, but the default Transport has no dial or TLS handshake timeouts, so a dead upstream can block forever.
- client := &http.Client{
- Timeout: 0, // No timeout for SSE connection
- }
+ tr := &http.Transport{
+ DialContext: (&net.Dialer{Timeout: 10 * time.Second, KeepAlive: 30 * time.Second}).DialContext,
+ TLSHandshakeTimeout: 10 * time.Second,
+ IdleConnTimeout: 90 * time.Second,
+ MaxIdleConns: 100,
+ }
+ client := &http.Client{Transport: tr, Timeout: 0} // no overall timeout; stream is long‑lived📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| client := &http.Client{ | |
| Timeout: 0, // No timeout for SSE connection | |
| } | |
| slog.Debug("checking upstream status at", "url", checkUrl) | |
| resp, err := client.Get(checkUrl) | |
| waitDuration := 10 * time.Second | |
| tr := &http.Transport{ | |
| DialContext: (&net.Dialer{Timeout: 10 * time.Second, KeepAlive: 30 * time.Second}).DialContext, | |
| TLSHandshakeTimeout: 10 * time.Second, | |
| IdleConnTimeout: 90 * time.Second, | |
| MaxIdleConns: 100, | |
| } | |
| client := &http.Client{Transport: tr, Timeout: 0} // no overall timeout; stream is long-lived | |
| waitDuration := 10 * time.Second |
🤖 Prompt for AI Agents
In cmd/wol-proxy/wol-proxy.go around lines 127 to 131, the HTTP client is
created with Timeout: 0 which is fine for an SSE stream but leaves the default
Transport with no dial or TLS handshake timeouts so a dead upstream can block
forever; replace the default transport with a custom http.Transport that sets a
net.Dialer with a reasonable Dial timeout (e.g., 10s) and TLSHandshakeTimeout
(and optionally ResponseHeaderTimeout/ExpectContinueTimeout) and assign that
Transport to the http.Client while keeping Timeout: 0 so the stream itself is
not terminated.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/wol-proxy/wol-proxy.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Fix all staticcheck-reported issues in Go code
Files:
cmd/wol-proxy/wol-proxy.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-tests
- GitHub Check: run-tests
🔇 Additional comments (2)
cmd/wol-proxy/wol-proxy.go (2)
4-4: LGTM!The new imports (
bufioandstrings) are necessary for SSE stream scanning and path-based request filtering.Also applies to: 17-17
226-233: LGTM! Logging improvements align with PR objectives.The path-aware logic correctly skips wake-up for SSE connections and the enhanced logging (path and remote address) will help identify what is causing wake-ups.
| // graceful shutdown | ||
| ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt) | ||
| defer stop() | ||
| ctx, _ := signal.NotifyContext(context.Background(), os.Interrupt) |
There was a problem hiding this comment.
Resource leak: cancel function must be called.
The cancel function returned by signal.NotifyContext should be called to release resources. Discarding it with _ causes a resource leak.
Apply this diff:
- ctx, _ := signal.NotifyContext(context.Background(), os.Interrupt)
+ ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt)
+ defer stop()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ctx, _ := signal.NotifyContext(context.Background(), os.Interrupt) | |
| ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt) | |
| defer stop() |
🤖 Prompt for AI Agents
In cmd/wol-proxy/wol-proxy.go around line 97, the cancel function returned by
signal.NotifyContext is discarded which causes a resource leak; change the call
to capture the cancel function (ctx, cancel := signal.NotifyContext(...)) and
ensure you call cancel() when done (e.g., immediately after creating ctx add
defer cancel() in main or the surrounding function) so resources are released.
| if err := server.Shutdown(shutdownCtx); err != nil { | ||
| slog.Error("server shutdown error", "error", err) | ||
| } | ||
| server.Close() |
There was a problem hiding this comment.
Immediate close terminates active connections abruptly.
server.Close() immediately closes all active connections without waiting for in-flight requests to complete. For graceful shutdown, use server.Shutdown(ctx) with a timeout context.
Apply this diff:
ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt)
+ defer stop()
<-ctx.Done()
- server.Close()
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ if err := server.Shutdown(shutdownCtx); err != nil {
+ slog.Error("error during shutdown", "error", err)
+ }🤖 Prompt for AI Agents
In cmd/wol-proxy/wol-proxy.go around line 99, replace the immediate
server.Close() call with a graceful shutdown: create a context with timeout
(e.g., context.WithTimeout(context.Background(), shutdownTimeout)), call
server.Shutdown(ctx) and handle its error, and defer cancel the context; if
Shutdown returns an error (or times out) fall back to server.Close() to force
close and log the outcome. Ensure shutdownTimeout is a sensible duration and
propagate/log any errors from Shutdown/Close.
| proxy.statusMutex.Lock() | ||
| proxy.failCount++ | ||
| proxy.statusMutex.Unlock() | ||
| time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
Use waitDuration consistently for retry delays.
These lines use hardcoded 10 * time.Second but line 131 defines waitDuration for this purpose. Use the variable consistently.
Apply this diff:
- time.Sleep(10 * time.Second)
+ time.Sleep(waitDuration)Apply to both lines 156 and 166.
Also applies to: 166-166
🤖 Prompt for AI Agents
In cmd/wol-proxy/wol-proxy.go around lines 156 and 166, the retry sleeps use a
hardcoded 10 * time.Second instead of the previously defined waitDuration;
replace those hardcoded sleep calls with time.Sleep(waitDuration) (or simply use
waitDuration where appropriate) so both retry delays consistently use the
waitDuration variable.
| if slog.Default().Enabled(context.Background(), slog.LevelDebug) { | ||
| fmt.Print("Events: ") | ||
| } | ||
| for scanner.Scan() { | ||
| if slog.Default().Enabled(context.Background(), slog.LevelDebug) { | ||
| // Just read the events to keep connection alive | ||
| // We don't need to process the event data | ||
| events++ | ||
| fmt.Printf("%d, ", events) | ||
| } | ||
| } | ||
| fmt.Println() |
There was a problem hiding this comment.
Replace fmt debug output with structured slog logging.
Using fmt.Print and fmt.Printf for debug output breaks structured logging and always prints to stdout. Line 193 also executes unconditionally, printing an empty line even when debug is disabled.
Apply this diff:
events := 0
- if slog.Default().Enabled(context.Background(), slog.LevelDebug) {
- fmt.Print("Events: ")
- }
for scanner.Scan() {
- if slog.Default().Enabled(context.Background(), slog.LevelDebug) {
- // Just read the events to keep connection alive
- // We don't need to process the event data
- events++
- fmt.Printf("%d, ", events)
- }
+ events++
}
- fmt.Println()
+ slog.Debug("SSE stream ended", "events_received", events)🤖 Prompt for AI Agents
In cmd/wol-proxy/wol-proxy.go around lines 182 to 193, replace the fmt debug
output with structured slog calls and avoid unconditional printing: remove
fmt.Print/Printf and the unconditional fmt.Println; inside the existing
slog.Default().Enabled(context.Background(), slog.LevelDebug) checks, increment
events and emit structured debug logs such as slog.Debug("events read",
slog.Int("count", events)) (or slog.DebugContext with ctx if available) so
output only appears when debug is enabled and uses structured attributes instead
of stdout prints.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
cmd/wol-proxy/wol-proxy.go (6)
97-97: Resource leak: cancel function still not called.This issue was previously flagged but remains unresolved. The cancel function from
signal.NotifyContextmust be called to release resources.
99-99: Immediate close still terminates connections abruptly.This issue was previously flagged but remains unresolved.
server.Close()immediately closes all active connections. Consider usingserver.Shutdown(ctx)with a timeout for graceful shutdown.
126-126: URL construction, shadowing, and casing issues remain unresolved.This issue was previously flagged but remains unresolved. The variable
eventsUrlshadows theurlpackage, uses string concatenation that drops any upstream base path, and violates ST1003 naming conventions.Based on coding guidelines.
127-129: SSE client can still hang indefinitely on connect.This issue was previously flagged but remains unresolved. While
Timeout: 0is correct for the stream itself, the default Transport has no dial or TLS handshake timeouts, so a dead upstream can block the connection attempt forever.
154-154: Hardcoded sleep duration instead of using waitDuration.This issue was previously flagged but remains unresolved. Use
waitDurationconsistently instead of hardcoding10 * time.Second.Also applies to line 164.
180-191: Replace fmt debug output with structured slog logging.This issue was previously flagged but remains unresolved. Using
fmt.Printandfmt.Printfbreaks structured logging and the unconditionalfmt.Println()at line 191 always executes even when debug is disabled.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/wol-proxy/wol-proxy.go(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Fix all staticcheck-reported issues in Go code
Files:
cmd/wol-proxy/wol-proxy.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-tests
- GitHub Check: run-tests
| if strings.HasPrefix(path, "/api/events") { | ||
| slog.Debug("Skipping wake up", "req", path) | ||
| w.WriteHeader(http.StatusNoContent) | ||
| return | ||
| } |
There was a problem hiding this comment.
Path prefix check may match unintended paths.
strings.HasPrefix(path, "/api/events") will match paths like "/api/eventstream", "/api/events2", or "/api/events_other" in addition to "/api/events" and "/api/events/...". If the intent is to only skip wake-up for the exact "/api/events" endpoint or paths under that namespace, consider a more precise check.
Apply this diff for an exact match or a path under /api/events/:
- if strings.HasPrefix(path, "/api/events") {
+ if path == "/api/events" || strings.HasPrefix(path, "/api/events/") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if strings.HasPrefix(path, "/api/events") { | |
| slog.Debug("Skipping wake up", "req", path) | |
| w.WriteHeader(http.StatusNoContent) | |
| return | |
| } | |
| if path == "/api/events" || strings.HasPrefix(path, "/api/events/") { | |
| slog.Debug("Skipping wake up", "req", path) | |
| w.WriteHeader(http.StatusNoContent) | |
| return | |
| } |
🤖 Prompt for AI Agents
In cmd/wol-proxy/wol-proxy.go around lines 223 to 227, the current
strings.HasPrefix(path, "/api/events") will also match unintended routes like
"/api/eventstream" or "/api/events2"; change the guard to only match the exact
"/api/events" or any path under that namespace by checking path == "/api/events"
|| strings.HasPrefix(path, "/api/events/") so only the endpoint and its subpaths
are skipped.
| func (p *proxyServer) incFail(num int) { | ||
| p.statusMutex.Lock() | ||
| defer p.statusMutex.Unlock() | ||
| p.failCount += num | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Simplify incFail API by removing unnecessary parameter.
The incFail method accepts a num int parameter but is always called with 1 at all four call sites (lines 140, 153, 163, 200). This adds unnecessary complexity without providing any benefit.
Apply this diff to simplify the API:
-func (p *proxyServer) incFail(num int) {
+func (p *proxyServer) incFail() {
p.statusMutex.Lock()
defer p.statusMutex.Unlock()
- p.failCount += num
+ p.failCount++
}Then update all call sites from proxy.incFail(1) to proxy.incFail().
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/wol-proxy/wol-proxy.go around lines 267 to 271, the incFail method
currently accepts a num int but callers always pass 1; change the method
signature to func (p *proxyServer) incFail() { p.statusMutex.Lock(); defer
p.statusMutex.Unlock(); p.failCount++ } and update the four call sites at lines
140, 153, 163, and 200 from proxy.incFail(1) to proxy.incFail() so the API is
simplified and all increments remain identical.
wol-proxy seems to be keeping my LLM box awake. Why? Improve logging to discover the cause and the fix.
Summary by CodeRabbit
New Features
Bug Fixes
Chores