Files
kandev/apps/backend/internal/agent/runtime/lifecycle/streams_test.go
T
João Salavisa de92557f46 test(backend): wire goleak goroutine-leak detection across 8 packages (#1139)
* chore(backend): add go.uber.org/goleak test dependency

Promotes the previously-indirect goleak v1.3.0 to a direct require so
per-package TestMain calls can use it for goroutine leak detection.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(backend/lifecycle): drain workspace-stream backoff on Manager.Stop

StreamManager.connectWorkspaceStream slept the full retry backoff in
time.Sleep between failed attempts. SessionTraceContext is deliberately
uncancellable (it carries a long-lived span), so if the Manager began
shutting down mid-backoff the goroutine outlived Stop() and stranded
until the timer fired.

Thread Manager.stopCh through the StreamManager constructor and add a
sleepOrStop helper that selects between the timer and the shutdown
signal. ReconnectAll's startup sleep gets the same treatment.

TestConnectWorkspaceStream_BackoffDrainsOnStop closes stopCh while a
failing-port retry is in flight and asserts the goroutine exits within
500ms — fails without the fix, passes with it.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(backend/github): close sqlite DB in newTestStore to drain connectionOpener

newTestStore opened *sqlx.DB via db.OpenSQLite but never registered a
Close on cleanup, leaving database/sql.(*DB).connectionOpener goroutines
parked across test boundaries. Add t.Cleanup(sqlxDB.Close); goleak's
per-package TestMain (added in a follow-up commit) is the regression
check.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* test(backend): wire goleak.VerifyTestMain across leak-prone packages

Per-package TestMain in:
- internal/integrations/healthpoll (auth-health ticker loop)
- internal/jira, internal/linear (auth + issue-watch loops)
- internal/github (PR / review / issue poller loops)
- internal/gateway/websocket (per-conn pumps + terminal bridge)
- internal/orchestrator (detached event-handler goroutines)
- internal/agent/runtime/lifecycle (remote-status loop, StreamManager
  helpers; ignores OTel batch-span processor and OTLP retry helper —
  initialised early in the process and only exit on shutdown)
- internal/agentctl/server/process (extends the existing fixture-binary
  TestMain so subprocess + PTY pump leaks surface there)

Catches regressions where a Start/Stop forgets to drain on WaitGroup,
or a detached goroutine forgets to honour ctx/stopCh cancellation.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* docs(backend): document goroutine ownership and leak-testing convention

New "Goroutine ownership and leak testing" subsection under Conventions
in apps/backend/AGENTS.md (CLAUDE.md symlinks to it). Covers:

- Lifecycle: Start/Stop + WaitGroup, both idempotent.
- Cancellation: every long wait selects on ctx.Done() or stopCh; never
  time.Sleep in retry loops (reference lifecycle.StreamManager.sleepOrStop).
- Detached helpers: must respect the owning type's shutdown signal.
- Leak testing: new packages spawning goroutines add goleak.VerifyTestMain;
  IgnoreTopFunction is reserved for genuine third-party background
  goroutines and must carry a comment explaining the suppression.

Lists the packages currently instrumented so future contributors can
mirror the pattern.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* chore(backend/lifecycle): drop dead OTel IgnoreTopFunction from goleak setup

Review follow-up. agentctl/tracing.initTracing only spawns the OTel SDK
provider when OTEL_EXPORTER_OTLP_ENDPOINT is set; in tests it isn't, so
the noop provider is used and no batchSpanProcessor / OTLP retry
goroutines ever exist. The IgnoreTopFunction calls were dead
suppressions and the comment ("called by NewManager via the agent
execution trace path") misdescribed when the SDK init runs. Drop the
ignores and correct AGENTS.md to stop citing OTel as the canonical
suppression example.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* test(backend/lifecycle): drop unneeded 50ms sleep from backoff-drain test

QA follow-up. The 50ms time.Sleep before close(stopCh) in
TestConnectWorkspaceStream_BackoffDrainsOnStop was meant to ensure the
goroutine had entered the backoff sleep before shutdown — but that
ordering doesn't affect the test outcome:

  - With the fix: sleepOrStop sees stopCh already closed and returns
    false on the first call, so the goroutine drains within one attempt
    regardless of when stopCh closes.
  - Without the fix: time.Sleep ignores stopCh, the goroutine burns
    its full 5-retry backoff (~31s), and the 500ms timeout fires.

Removing the sleep makes the test ~12x faster (15ms vs 200ms total) and
removes the only time.Sleep-based wait from the new tests, aligning
with the codebase's "channel-based sync, no sleep waits" convention.
Verified by 50x -race and 5x revert-and-fail.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(backend/github): drain HTTP idle conns before goleak inspects

CI surfaced a leaked goroutine on the Linux runner that local runs
missed: a single net/http.(*http2ClientConn).readLoop parked on a
TLS read, created by net/http.(*http2Transport).newClientConn. The
package's anonymousHTTPClient and PATClient's rewriteTransport both
route through http.DefaultTransport's connection pool, so an idle
HTTP/2 connection in that pool keeps the reader goroutine alive past
m.Run() — invisible on sandboxed local runs where outbound network
is blocked.

Replace github/goleak_test.go's plain VerifyTestMain with an
expanded TestMain that drains http.DefaultTransport's idle
connections before goleak.Find runs. This is the stdlib's
documented teardown for the connection pool, keeps the assertion
strict (no IgnoreTopFunction), and doesn't leak into production
code. Other in-scope packages don't need this — they don't use
http.DefaultTransport for HTTPS calls.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(backend): drain ws.Done on stopCh; always run goleak in github tests

- lifecycle/streams.go: replace bare <-ws.Done() with a select that also
  watches sm.stopCh so a connected goroutine drains when Manager.Stop()
  fires before the remote WS closes. A nil stopCh (isolated tests) blocks
  the nil channel case forever, degrading to plain <-ws.Done() semantics.
- github/goleak_test.go: remove the exitCode==0 guard around goleak.Find
  so leaks are always surfaced even when a test has already failed; original
  exit code is preserved (leaks only promote it to 1).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(backend/lifecycle): close ws on stopCh so agentctl WS loops drain

cubic flagged that the previous shutdown path observed stopCh but never
called ws.Close(), so the underlying WebSocket and its agentctl-side
read/write goroutines (workspace_stream.go:67-68) kept running past
Manager.Stop(). ClearWorkspaceStream only removes the WS from the
execution's tracking — it doesn't close the connection.

Call ws.Close() on the stopCh path. ws.Close is idempotent via
closeOnce, so it's safe even if the remote end already closed.

TestConnectWorkspaceStream_ClosesWSOnStop connects to a mock workspace
server that stays open until the client closes its side; after stopCh
fires, the test asserts ws.Done() closes within 500ms — which is only
true if connectWorkspaceStream invoked ws.Close(). Verified to fail
without the fix and pass with it.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* test(backend/lifecycle): use channel sync instead of sleep polling in ws-close test

cubic flagged that TestConnectWorkspaceStream_ClosesWSOnStop polled
execution.GetWorkspaceStream() in a 500ms sleep loop, which is brittle
under CI load. connectWorkspaceStream already accepts a ready
channel that closes after SetWorkspaceStream — pass it in and wait on
the channel instead.

synctest isn't viable here (real network I/O), so channel-based sync is
the testing convention per AGENTS.md.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
2026-05-29 10:23:18 +01:00

151 lines
5.1 KiB
Go

package lifecycle
import (
"testing"
"time"
agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl"
)
// TestConnectWorkspaceStream_IdempotentWhenAlreadyAttached is the regression
// test for the workspace-stream double-connect race. Two paths can call
// connectWorkspaceStream for the same execution (e.g. workspace-only ensure
// followed by full-launch promotion). The second call previously hit
// "workspace stream already connected" and burned 5 retries before logging
// a terminal ERROR. The fix short-circuits when a stream is already attached.
func TestConnectWorkspaceStream_IdempotentWhenAlreadyAttached(t *testing.T) {
sm := NewStreamManager(newTestLogger(), StreamCallbacks{}, nil, nil)
execution := &AgentExecution{ID: "exec-1", SessionID: "sess-1"}
// Pre-attach a non-nil workspace stream — simulates another goroutine
// having already connected before this call.
execution.SetWorkspaceStream(&agentctl.WorkspaceStream{})
ready := make(chan struct{})
done := make(chan struct{})
go func() {
sm.connectWorkspaceStream(execution, ready)
close(done)
}()
// Should return effectively immediately (well under the 1s first-retry
// backoff). 500ms gives ample headroom on slow CI without masking a
// regression that would burn through the full 5-retry loop.
select {
case <-done:
case <-time.After(500 * time.Millisecond):
t.Fatal("connectWorkspaceStream did not exit early when stream was already attached")
}
// ready must be closed (deferred signalReady runs even on early exit).
select {
case <-ready:
default:
t.Error("ready channel was not closed on early-exit path")
}
}
// TestConnectWorkspaceStream_BackoffDrainsOnStop is the regression test for
// the workspace-stream retry-backoff leak. Before the fix, connectWorkspaceStream
// slept the full backoff in time.Sleep between retries; if the test (or
// production Manager) tore down while a retry was in flight, the goroutine
// stranded until the backoff fired, surviving the Manager's Stop(). The fix
// selects on stopCh so the backoff drains immediately on shutdown.
//
// We trigger the failing path by pointing the client at a closed port —
// StreamWorkspace returns "connection refused" on every attempt, sending the
// loop into its retry backoff. Closing stopCh must release the goroutine in
// well under the (uncapped) backoff window.
func TestConnectWorkspaceStream_BackoffDrainsOnStop(t *testing.T) {
stopCh := make(chan struct{})
sm := NewStreamManager(newTestLogger(), StreamCallbacks{}, nil, stopCh)
log := newTestLogger()
badClient := agentctl.NewClient("127.0.0.1", 1, log) // port 1 is reserved
defer badClient.Close()
execution := &AgentExecution{
ID: "exec-backoff",
SessionID: "sess-backoff",
agentctl: badClient,
}
done := make(chan struct{})
go func() {
sm.connectWorkspaceStream(execution, nil)
close(done)
}()
close(stopCh)
select {
case <-done:
case <-time.After(500 * time.Millisecond):
t.Fatal("connectWorkspaceStream did not drain on stopCh close — backoff is leaking")
}
}
// TestConnectWorkspaceStream_ClosesWSOnStop is the regression test for the
// connected-stream shutdown leak. Once StreamWorkspace returns successfully
// the goroutine parks on `<-ws.Done()`. If the manager shuts down while the
// remote WS is still alive, simply observing stopCh isn't enough — the
// underlying WS connection (and the agentctl-side read/write loops it owns)
// keep running. The fix calls ws.Close() on the stopCh path so the WS
// actually tears down, and ws.Done() fires for any other observer.
//
// We exercise this by pointing a real agentctl client at a mock workspace
// stream server that stays open until the client closes the WS. After the
// goroutine connects, we close stopCh and assert ws.Done() fires within
// a short window — which is only true if connectWorkspaceStream called
// ws.Close() on its way out.
func TestConnectWorkspaceStream_ClosesWSOnStop(t *testing.T) {
mock := newMockAgentServer(t)
defer mock.Close()
stopCh := make(chan struct{})
sm := NewStreamManager(newSessionTestLogger(), StreamCallbacks{}, nil, stopCh)
client := createTestClient(t, mock.server.URL)
defer client.Close()
execution := &AgentExecution{
ID: "exec-close",
SessionID: "sess-close",
agentctl: client,
}
// ready closes after SetWorkspaceStream returns and signalReady fires —
// channel-based sync per the testing convention, no sleep polling.
ready := make(chan struct{})
done := make(chan struct{})
go func() {
sm.connectWorkspaceStream(execution, ready)
close(done)
}()
select {
case <-ready:
case <-time.After(500 * time.Millisecond):
t.Fatal("workspace stream never attached (ready channel did not close)")
}
ws := execution.GetWorkspaceStream()
if ws == nil {
t.Fatal("workspace stream attached but execution lost the reference")
}
close(stopCh)
select {
case <-ws.Done():
case <-time.After(500 * time.Millisecond):
t.Fatal("ws.Done() did not fire on stopCh close — ws.Close() was not called")
}
select {
case <-done:
case <-time.After(500 * time.Millisecond):
t.Fatal("connectWorkspaceStream goroutine did not drain on stopCh close")
}
}