diff --git a/CHANGELOG.md b/CHANGELOG.md index 884bca7..cc624df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,34 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 output when combined with `--json`. Credentials embedded in the stored URL are masked before display (the password placeholder is `xxxxx`). +### Fixed + +- **Gopher: `ctx.Done()` cancellation goroutine leak** — the goroutine + spawned in `Download` to close the socket on `ctx.Done()` previously + waited indefinitely on the parent context, leaking every time the + context outlived the function. Now bounded by a `defer`-closed + `done` channel so the goroutine exits on every return path. + +- **Gemini: `ctx.Done()` cancellation goroutine leak** — same bug, + same fix in `downloadWithRedirects`. + +- **Gopher: partial state lost on cancel-then-close** — when the server + closed the connection after `ctx` was cancelled, `downloadText` + treated the resulting EOF as a successful end-of-stream and never + persisted `.goget.meta`. Mirrors the equivalent Gemini behavior + (already present from the Gemini resume feature): if `ctx.Err() != nil`, + save partial state and return the context error. + +- **Gopher: `--resume` ignored when caller supplies `Writer`** — when + `--resume` was set together with a caller-supplied `Writer`, the + partial-state sidecar would track bytes that landed in the `Writer` + rather than `Output`. `--resume` is now silently disabled in that + case (with a `--verbose` warning), keeping metadata consistent with + the actual on-disk state. + +- **Gemini: `--resume` ignored when caller supplies `Writer`** — same + fix applied to `downloadWithRedirects`. + ### Testing - `cmd/goget/man_page_test.go` — new test file. Asserts the embedded @@ -85,6 +113,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `defineFlags` (so adding a new flag without documenting it now breaks the build). When `groff` is installed, also runs `groff -man -ww -Kutf-8 -z` over the embedded source and fails on any warning. +- Resume tests deterministic across the protocol stack + (`TestGeminiResume{SaveMetadata,Download}` and + `TestGopherResume{SaveMetadata,Download}`) — the four tests previously + relied on a fixed `time.Sleep` before calling `cancel()`, making them + flaky on slow CI schedulers. They now wait for a `ProgressCallback` + signal, which fires synchronously after the first `writer.Write` + lands on disk. +- Goroutine leak regression suite + (`TestGeminiCancelGoroutineExits`, `TestGopherCancelGoroutineExits`) + — runs 50 downloads back-to-back, settles the runtime via + `runtime.GC()`, and asserts `runtime.NumGoroutine()` returns to + baseline + 1 within a 2-second window. +- `TestGopherResumeInformation` — new regression test covering the + `typeInformation` text-mode path through resume, which differs from + `typeHTML`/`typeTextFile` because lines are passed through verbatim + (no metadata strip). +- `TestGopherResumeStaleMetadata` now asserts that an orphan + `.goget.meta` carried over from a different URL is deleted once a + successful fresh download completes, preventing it from being + picked up by a future `--resume` run. ## [1.0.0] — 2026-06-26 diff --git a/internal/protocol/gemini/gemini_test.go b/internal/protocol/gemini/gemini_test.go index ff2287c..234b26f 100644 --- a/internal/protocol/gemini/gemini_test.go +++ b/internal/protocol/gemini/gemini_test.go @@ -16,6 +16,7 @@ import ( "net" "net/url" "os" + "runtime" "strconv" "strings" "testing" @@ -450,3 +451,81 @@ func TestSaveGeminiResumeNoop(t *testing.T) { // Should not panic. }) } + +// TestGeminiCancelGoroutineExits verifies that the ctx-cancellation +// goroutine spawned by Gemini downloadWithRedirects does not outlive +// Download itself. The goroutine used to wait forever on <-ctx.Done() +// when the parent context was never cancelled; the done-channel bound +// must let every iteration return to the runtime goroutine pool. +func TestGeminiCancelGoroutineExits(t *testing.T) { + cfg := testTLSConfig() + listener, err := tls.Listen("tcp", "127.0.0.1:0", cfg) + if err != nil { + t.Fatal(err) + } + defer listener.Close() + port := listener.Addr().(*net.TCPAddr).Port + + go func() { + for { + conn, lerr := listener.Accept() + if lerr != nil { + return + } + go func(c net.Conn) { + defer c.Close() + r := bufio.NewReader(c) + r.ReadString('\n') + // Brief response that Gemini Download can chew + // through quickly, spawning the cancellation + // goroutine and returning. The goroutine must + // exit via the done channel on return. + c.Write([]byte("20 application/octet-stream\r\n")) + c.Write([]byte("hi")) + }(conn) + } + }() + + // Warm up to settle the runtime goroutine pool (timer, GC, etc.). + proto := NewProtocol() + proto.TLSInsecure = true + u, _ := url.Parse("gemini://127.0.0.1:" + strconv.Itoa(port) + "/warmup") + if _, dlErr := proto.Download(context.Background(), &core.DownloadRequest{ + URL: u, + Writer: &stringWriter{&strings.Builder{}}, + }); dlErr != nil { + t.Fatalf("warmup: %v", dlErr) + } + + runtime.GC() + time.Sleep(50 * time.Millisecond) + runtime.GC() + baseline := runtime.NumGoroutine() + + const iterations = 50 + for i := 0; i < iterations; i++ { + u, _ := url.Parse("gemini://127.0.0.1:" + strconv.Itoa(port) + "/x") + if _, dlErr := proto.Download(context.Background(), &core.DownloadRequest{ + URL: u, + Writer: &stringWriter{&strings.Builder{}}, + }); dlErr != nil { + t.Fatalf("download %d: %v", i, dlErr) + } + } + + // After every successful Download returns, its cancellation + // goroutine must have exited. Allow a brief settle window for + // the runtime to reclaim them. + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + runtime.GC() + time.Sleep(50 * time.Millisecond) + // One tolerance for any incidental stdlib goroutine that + // the Go runtime occasionally spins up between calls. + if runtime.NumGoroutine() <= baseline+1 { + return + } + } + t.Fatalf("goroutine leak: baseline=%d, after %d downloads = %d", + baseline, iterations, runtime.NumGoroutine()) +}