fix: wait for streaming heartbeat shutdown

[zzbybq]

Summary

• stop and join the keepalive heartbeat before the streaming handler returns
• prevent an in-flight heartbeat Flush from racing with net/http response finalization
• add a regression test proving shutdown waits for an active flush

Failure observed

On Windows, the proxy exited immediately after a streaming request completed:

panic: runtime error: invalid memory address or nil pointer dereference
bufio.(*Writer).Flush
net/http.(*response).Flush
internal/handlers.(*responseWriter).WriteKeepalive

heartbeatCancel() only signaled the goroutine; it did not wait for it to exit. If the ticker and cancellation were ready together, the goroutine could still enter WriteKeepalive while the handler returned and net/http finalized the response writer.

Validation

• go test ./internal/handlers passes
• go test ./... passes except the pre-existing internal/updater test, which currently expects a missing release asset: routatic-proxy_windows-amd64.exe
• git merge-tree origin/main HEAD reports a clean merge
• race detector was not available on the test machine because its Windows Go setup has no C compiler

[kilo-code-bot]

🔥 The Roast: This <-done waits forever for the goroutine to exit. If the in-flight WriteKeepalive flush is wedged on a stalled-but-still-open TCP connection (peer connected but not reading, kernel send buffer full), the underlying write can block until the OS TCP timeout — that's minutes, not milliseconds. Your handler now politely joins that wait, so a single stuck keepalive flush hangs the whole request goroutine and leaks it alongside the connection. You traded a Windows nil-pointer panic for a slow-motion goroutine leak. Progress?

🩹 The Fix: Bound the shutdown wait so a stuck flush can't hang the handler indefinitely, e.g. select { case <-done: case <-time.After(shutdownGracePeriod): }, and log at Debug/Warn if it times out so the leak is at least visible.

📏 Severity: suggestion

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[zzbybq]

Addressed in 4a0c78d. I kept the join (a bounded join that returns while Flush is still running would reintroduce the original response-writer finalization race) and instead bounded each keepalive write with a 5s http.ResponseController write deadline. A stalled Flush now returns, the heartbeat exits, and only then does shutdown complete. The deadline remains set after a write error so later writes fail instead of blocking on the same client. Added a regression test that verifies the deadline unblocks heartbeat shutdown.

[kilo-code-bot]

🔥 The Roast: Now that interval is caller-supplied, time.NewTicker(interval) will panic on a zero or negative duration — harder than a freshman's first prod deploy. The old code hardcoded 3 * time.Second, so it was happily immune. You've widened the blast radius to any future caller who passes a bad value.

🩹 The Fix: Guard the input (fall back to a sane default like 3 * time.Second when interval <= 0) or at minimum document that interval must be strictly positive. As written, one zero value takes the whole ticker goroutine down with a panic.

📏 Severity: nitpick

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[zzbybq]

Addressed in 4a0c78d. Non-positive heartbeat intervals now fall back to the 3s default; non-positive write timeouts likewise fall back to 5s. Added a regression test covering zero values.

[kilo-code-bot]

Code Review Roast 🔥

Verdict: 1 Issue Found | Recommendation: Address before merge

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	1
🤏 nitpick	0

Issue Details (click to expand)

File	Line	Roast
internal/handlers/messages.go	255	Unbounded <-done waits forever for the goroutine to exit; a stuck keepalive flush on a stalled connection can hang the request goroutine indefinitely

---

🏆 Best part: The write-deadline fix for keepalive flushes is exactly the right tool for the job — bounding a potentially-wedged Flush() with http.ResponseController.SetWriteDeadline instead of hoping the OS eventually notices. The regression tests (deadline unblocks stop, non-positive durations default) are also solid. I'm almost disappointed this is a follow-up review instead of a first pass.

💀 Worst part: The <-done at line 255 is still there. You added a 5-second write deadline to bound the flush, but the shutdown wait itself is still unbounded. If the deadline logic ever gets bypassed, you're back to the same goroutine leak.

📊 Overall: Like a homeowner who installed a security system but left the back door wide open — the new protection is real and well-tested, but the original vulnerability is still standing there waving hello.

Files Reviewed (2 files)

• internal/handlers/messages.go - 1 unresolved issue (1 resolved since last review)
• internal/handlers/messages_test.go - 0 issues

Fix these issues in Kilo Cloud

Previous Review Summary (commit a6abfa7)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit a6abfa7)

Verdict: 2 Issues Found | Recommendation: Address before merge

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	1
🤏 nitpick	1

Issue Details (click to expand)

File	Line	Roast
internal/handlers/messages.go	225	Unbounded <-done waits forever for the goroutine to exit; a stuck keepalive flush on a stalled connection can hang the request goroutine indefinitely
internal/handlers/messages.go	208	time.NewTicker(interval) now takes a caller-supplied value and will panic on zero/negative duration

---

🏆 Best part: The refactor into startKeepaliveHeartbeat is genuinely clean, and the TestKeepaliveHeartbeat_StopWaitsForInFlightFlush regression test (blocking-flush writer that proves shutdown waits on an in-flight flush) is exactly the kind of test this bug deserved. I'm almost disappointed it's this good.

💀 Worst part: The fix swaps the Windows nil-pointer panic for the possibility of the handler goroutine hanging forever on <-done if the in-flight flush blocks on a wedged connection — traded one failure mode for another.

📊 Overall: Like a surgeon who fixed the bleed but forgot the patient is still on the table — the core repair is correct and well-tested, just bound the shutdown wait so it can't stall.

Files Reviewed (2 files)

• internal/handlers/messages.go - 2 issues
• internal/handlers/messages_test.go - 0 issues

Fix these issues in Kilo Cloud

---

_{Reviewed by step-3.7-flash · Input: 156.2K · Output: 23.9K · Cached: 495.1K}

_{Review guidance: REVIEW.md from base branch main (truncated)}