fix(server): a background sync with no owner outlives the server that started it (GDK-270) (#26)

`handlePutSettings` ends by kicking a full sync when the mirrored scope
changes, and `startSyncJob` started it as `go s.runSyncJob(context.Background(),
…)`: no cancel, no WaitGroup, no handle. The comment above that line correctly
explained why the job cannot hang off the request context, and then handed its
lifetime to nobody.

The symptom CI reported was a test failing on a PR that changed one markdown
file: `TempDir RemoveAll cleanup: directory not empty`. The mirror is WAL, so a
connection that opens or closes there recreates -wal/-shm — and the detached
goroutine was still writing into a directory the test had already torn down.
It never reproduced on macOS by repetition, which is why it survived.

It is deterministic when you ask the right question. A goroutine that still
holds the pool is `database/sql` reporting `InUse > 0` after `Close`, and that
needs no race detector, no Linux and no repetition: the four scope-changing
settings PUTs fail in under a second. The four that fail are exactly the four
where `scopeChanged` is true; the siblings that do not change scope pass.

Structural: the server owns the lifetime. `newServer` builds a cancelable
`jobsCtx`, `startSyncJob` registers on a WaitGroup and refuses once the context
is cancelled, and `Handler.Shutdown`/`Close` cancel and wait within the same 3s
bound `cmd/gadak/serve.go` already uses for `http.Server`. Waiting for the
goroutine is not sufficient — `database/sql` rolls a cancelled Tx back from
`Tx.awaitDone`, which can still hold the connection — so the wait ends on the
pool going idle, which is the writer this bug is actually about.

Wired into every production path that closes a mirror, because a shutdown
method nobody calls is decoration: `cmd/gadak/serve.go`, `desktop/main.go`, and
`workspace.Registry.Close`, each ordered so the sync stops before the database
it writes to closes. `Entry.Handler` narrows from `http.Handler` to
`*server.Handler`: the entry owns that lifetime, and the old type hid it.

Recurrence: `quiesceFixtureDir` now asserts no connection is checked out once a
test is done, so this class fails in one second on any machine instead of
rarely on Linux; the existing "files came back" check stays, since it catches an
external writer a pool check cannot see. CI repeats `internal/server` and
`internal/workspace` under `-race`, not path-scoped.

FAIL-first, both halves lead-run. The fixture assertion on the unfixed tree:
four PUTs, `1 connection(s) still checked out after Close`, 0.66s. The
production wiring, with `Registry.Close` unwired: `1 connection(s) still checked
out after Registry.Close — the workspace's sync outlived its mirror`.

Debuggability: `store.PoolStats()` is what makes the assertion expressible, and
`Handler.SnapshotSync()` answers "what background work is running right now"
in-process, mirroring the existing GET /sync/progress/ rather than adding a
route.

Known and unchanged: the suite still attempts an outbound request to the
fixture's `x.atlassian.net`; the job is cancelled at teardown so it no longer
outlives the test, but removing the attempt is a separate change.

Co-authored-by: midagedev <midagedev@users.noreply.github.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Author: midagedev

.github/workflows/ci.yml

@@ -56,6 +56,18 @@ jobs:
       - name: Go tests
         run: go test ./...
 
+      # GDK-270: a startSyncJob goroutine that outlives the test only shows
+      # up when this package is repeated under the race detector. Not
+      # path-scoped — the writer can be introduced from any import of
+      # internal/server. internal/workspace is here too: it owns the same
+      # lifetime one layer up (Registry.Close stops each workspace's sync
+      # before its mirror). count=2 (not 4): the job already ran count=1
+      # above; locally count=4 -race is ~180s, and doubling the package
+      # under race is enough to catch "passes once" while staying modest
+      # in a 20-minute job.
+      - name: Server tests under race (GDK-270)
+        run: go test ./internal/server/ ./internal/workspace/ -count=2 -race
+
       - name: Frontend typecheck
         run: npm run typecheck
 

cmd/gadak/serve.go

@@ -265,6 +265,10 @@ func cmdServe(args []string) error {
 	}
 
 	api := newServeAPI(db, cfg)
+	// Registered after db.Close and before reg.Close, so the LIFO order is
+	// workspaces, then this handler, then the mirror: a background sync must
+	// stop before the database it writes to closes (GDK-270).
+	defer api.Close()
 	spa := serveSPAHandler(opts.static)
 
 	// Workspace mounts share this process's listener; each profile opens lazily.

desktop/main.go

@@ -86,6 +86,8 @@ func run() error {
 	} else {
 		api = server.NewWithCache(db, cfg, cache)
 	}
+	// After db.Close above, so LIFO stops the background sync first (GDK-270).
+	defer api.Close()
 
 	ui, ok := gadak.WebUI()
 	if !ok {

internal/server/onboarding.go

@@ -268,15 +268,28 @@ func (s *server) startSyncJob(cfg *config.Config, full bool) bool {
 		return s.syncKick(cfg, full)
 	}
 	s.syncMu.Lock()
+	if s.jobsCtx != nil && s.jobsCtx.Err() != nil {
+		s.syncMu.Unlock()
+		return false
+	}
 	if s.syncJob.Running {
 		s.syncMu.Unlock()
 		return false
 	}
 	s.syncJob = progressDoc{Running: true, Phase: "syncing", StartedAt: store.Now()}
+	s.jobsWG.Add(1)
 	s.syncMu.Unlock()
 
 	// The request is answered immediately, so the run cannot hang off r.Context().
-	go s.runSyncJob(context.Background(), cfg, full)
+	// Lifetime is the server's jobsCtx: Shutdown cancels it and waits (GDK-270).
+	ctx := s.jobsCtx
+	if ctx == nil {
+		ctx = context.Background()
+	}
+	go func() {
+		defer s.jobsWG.Done()
+		s.runSyncJob(ctx, cfg, full)
+	}()
 	return true
 }
 

internal/server/server.go

@@ -89,6 +89,15 @@ type server struct {
 	syncMu   sync.Mutex
 	syncJob  progressDoc
 	activity mirrorActivity
+
+	// jobsCtx is cancelled by Shutdown so a startSyncJob goroutine cannot
+	// outlive the server. jobsWG counts those goroutines; Close waits on it.
+	// Nothing else owned this lifetime, so a settings PUT in a test (or a
+	// serve process exiting) left the writer holding a WAL connection
+	// (GDK-270).
+	jobsCtx    context.Context
+	jobsCancel context.CancelFunc
+	jobsWG     sync.WaitGroup
 }
 
 // Handler is the HTTP API plus optional update-check control. It implements
@@ -133,6 +142,7 @@ func newServer(db *store.DB, cfg *config.Config, cache *attachcache.Cache, profi
 	}
 	s := &server{db: db, cache: cache, profile: profile}
 	s.cfg.Store(cfg)
+	s.jobsCtx, s.jobsCancel = context.WithCancel(context.Background())
 
 	// Every pattern is anchored with {$}: a trailing slash alone would make each
 	// literal endpoint a subtree that overlaps `{key}/detail/`, which ServeMux
@@ -252,9 +262,86 @@ func newServer(db *store.DB, cfg *config.Config, cache *attachcache.Cache, profi
 	mux.HandleFunc("/", handleNotFound)
 	h := &Handler{mux: mux, s: s}
 	h.guarded = GuardBrowser(mux)
+	if testRegisterHandler != nil {
+		testRegisterHandler(h)
+	}
 	return h
 }
 
+// testRegisterHandler is set from tests so fixture cleanup can Shutdown every
+// Handler that opened a given DB before closing it. Production is nil.
+var testRegisterHandler func(*Handler)
+
+// closeWait is how long Close waits for in-flight startSyncJob goroutines.
+// Same bound as the HTTP server's shutdown window in cmd/gadak/serve.go
+// (the 3-second context.WithTimeout around srv.Shutdown). Past this, Close
+// returns and the caller may close the database anyway; a still-running job
+// then fails the pool assertion (tests) or races WAL files (production).
+const closeWait = 3 * time.Second
+
+// Shutdown cancels background startSyncJob work and waits for those
+// goroutines to return, or until ctx is done. A timed-out wait returns
+// ctx.Err(); the job may still be running and still hold a database
+// connection. Idempotent.
+//
+// Returning from the job goroutine is not enough: database/sql rolls a
+// cancelled Tx back from a helper goroutine (Tx.awaitDone), and that
+// helper can still hold the pool connection after runSyncJob has
+// returned. Waiting for InUse==0 is waiting for that writer, which is
+// the WAL leak GDK-270 actually is.
+func (h *Handler) Shutdown(ctx context.Context) error {
+	if h == nil || h.s == nil {
+		return nil
+	}
+	if h.s.jobsCancel != nil {
+		h.s.jobsCancel()
+	}
+	done := make(chan struct{})
+	go func() {
+		h.s.jobsWG.Wait()
+		close(done)
+	}()
+	if ctx == nil {
+		ctx = context.Background()
+	}
+	select {
+	case <-done:
+	case <-ctx.Done():
+		return fmt.Errorf("background sync still running after shutdown bound: %w", ctx.Err())
+	}
+	return waitPoolIdle(ctx, h.s.db)
+}
+
+// waitPoolIdle waits until no connection is checked out, or ctx is done.
+func waitPoolIdle(ctx context.Context, db *store.DB) error {
+	if db == nil {
+		return nil
+	}
+	if db.PoolStats().InUse == 0 {
+		return nil
+	}
+	t := time.NewTicker(5 * time.Millisecond)
+	defer t.Stop()
+	for {
+		select {
+		case <-ctx.Done():
+			return fmt.Errorf("background sync returned but %d connection(s) still checked out: %w", db.PoolStats().InUse, ctx.Err())
+		case <-t.C:
+			if db.PoolStats().InUse == 0 {
+				return nil
+			}
+		}
+	}
+}
+
+// Close is Shutdown with a 3s bound — the same window cmd/gadak/serve.go
+// uses for http.Server.Shutdown.
+func (h *Handler) Close() error {
+	ctx, cancel := context.WithTimeout(context.Background(), closeWait)
+	defer cancel()
+	return h.Shutdown(ctx)
+}
+
 // StartUpdateCheck runs a GitHub release lookup immediately and every 24h.
 // Results feed latest_version / release_url on bootstrap and delta when the
 // running build is older. Records cacheDir even when disabled so a later
@@ -307,6 +394,17 @@ func (h *Handler) SnapshotUpdate() UpdateStatus {
 	return h.s.snapshotUpdate()
 }
 
+// SnapshotSync is the debug document for "what background work is running
+// right now": the same one-shot job + activity picture that
+// GET /api/v1/issues/sync/progress/ already returns. No new endpoint — that
+// GET already carries it; this is the in-process form, matching SnapshotUpdate.
+func (h *Handler) SnapshotSync() progressResponse {
+	if h == nil || h.s == nil {
+		return progressResponse{}
+	}
+	return h.s.syncProgressResponse()
+}
+
 // setUpdateInfo stores a release snapshot (tests and the background loop).
 func (s *server) setUpdateInfo(info selfupdate.Info, ok bool) {
 	s.updateMu.Lock()

internal/server/server_test.go

@@ -11,6 +11,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"sync"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -22,6 +23,42 @@ import (
 	"github.com/midagedev/gadak/internal/store"
 )
 
+// liveHandlers lets fixtureAt stop every Handler created against a DB before
+// that DB is closed. Production never sets testRegisterHandler.
+var (
+	liveMu       sync.Mutex
+	liveHandlers = map[*store.DB][]*Handler{}
+)
+
+func init() {
+	testRegisterHandler = registerLive
+}
+
+func registerLive(h *Handler) {
+	if h == nil || h.s == nil || h.s.db == nil {
+		return
+	}
+	liveMu.Lock()
+	liveHandlers[h.s.db] = append(liveHandlers[h.s.db], h)
+	liveMu.Unlock()
+}
+
+func shutdownLive(t *testing.T, db *store.DB) {
+	t.Helper()
+	if db == nil {
+		return
+	}
+	liveMu.Lock()
+	hs := liveHandlers[db]
+	delete(liveHandlers, db)
+	liveMu.Unlock()
+	for _, h := range hs {
+		if err := h.Close(); err != nil {
+			t.Errorf("Handler.Close: %v — a startSyncJob goroutine did not return within the shutdown bound (GDK-270)", err)
+		}
+	}
+}
+
 // testRequest is httptest.NewRequest with Host set for the loopback API guard.
 // httptest defaults Host to "example.com", which browserGuard correctly
 // rejects as a DNS-rebinding name; real clients send localhost or an IP.
@@ -49,8 +86,17 @@ func testRequest(method, target string, body io.Reader) *http.Request {
 // nothing to race on, and it turns "some file was in the way" into "this named
 // file appeared after teardown began", reported by the fixture that owns the
 // path instead of anonymously by the framework.
-func quiesceFixtureDir(t *testing.T, dir string) {
+func quiesceFixtureDir(t *testing.T, dir string, db *store.DB) {
 	t.Helper()
+	if db != nil {
+		stats := db.PoolStats()
+		if stats.InUse > 0 {
+			// A connection still checked out means something the test started
+			// is still running. Close cannot reclaim it, and under WAL that
+			// writer recreates journal files in this TempDir (GDK-270).
+			t.Errorf("%d connection(s) still checked out after Close — something the test started is still running (GDK-270); stats=%+v", stats.InUse, stats)
+		}
+	}
 	entries, err := os.ReadDir(dir)
 	if err != nil {
 		// Already gone, or unreadable: not this helper's business.
@@ -89,8 +135,11 @@ func fixtureAt(t *testing.T) (*store.DB, *config.Config, string) {
 		t.Fatalf("open: %v", err)
 	}
 	t.Cleanup(func() {
+		// Stop writers first: a startSyncJob goroutine that outlives Close
+		// holds a pool connection and recreates WAL files in dir (GDK-270).
+		shutdownLive(t, db)
 		db.Close()
-		quiesceFixtureDir(t, dir)
+		quiesceFixtureDir(t, dir, db)
 	})
 	if err := db.UpsertSource(context.Background(), store.Source{ID: "jira", Kind: "jira", BaseURL: "https://x.atlassian.net"}); err != nil {
 		t.Fatalf("source: %v", err)
@@ -1241,3 +1290,30 @@ func TestBootstrapSurvivesABrokenGroupQuery(t *testing.T) {
 		t.Fatal("issues went missing with the broken query")
 	}
 }
+
+func TestHandlerShutdownCancelsSyncJob(t *testing.T) {
+	db, cfg := fixture(t)
+	h := New(db, cfg)
+	if !h.s.startSyncJob(cfg, true) {
+		t.Fatal("startSyncJob refused")
+	}
+	if err := h.Close(); err != nil {
+		t.Fatalf("Close: %v", err)
+	}
+	if n := db.PoolStats().InUse; n != 0 {
+		t.Fatalf("%d connection(s) still checked out after Handler.Close — background sync did not return (GDK-270)", n)
+	}
+	if err := h.Close(); err != nil {
+		t.Fatalf("second Close: %v", err)
+	}
+}
+
+func TestSnapshotSyncMatchesProgressEndpoint(t *testing.T) {
+	db, cfg := fixture(t)
+	h := New(db, cfg)
+	httpDoc := decode[progressResponse](t, get(t, h, apiBase+"sync/progress/", nil))
+	snap := h.SnapshotSync()
+	if snap != httpDoc {
+		t.Fatalf("SnapshotSync %+v != GET sync/progress/ %+v", snap, httpDoc)
+	}
+}

internal/store/stats.go

@@ -0,0 +1,10 @@
+package store
+
+import "database/sql"
+
+// PoolStats reports the connection pool's state. A caller that has finished
+// with a mirror can assert nothing is still checked out: a connection that
+// outlives Close belongs to something still running, and under WAL it writes
+// journal files back into a directory the caller may already have removed
+// (GDK-270).
+func (db *DB) PoolStats() sql.DBStats { return db.sql.Stats() }

internal/workspace/workspace.go

@@ -31,7 +31,10 @@ var workspaceNameRe = regexp.MustCompile(`^[A-Za-z0-9_-]{1,64}$`)
 
 // Entry is one opened workspace mirror (handler + DB + config).
 type Entry struct {
-	Handler http.Handler
+	// Handler is the concrete server handler, not http.Handler: this entry
+	// owns its lifetime, and Close has to be able to stop the background sync
+	// before the mirror it writes to is closed (GDK-270).
+	Handler *server.Handler
 	DB      *store.DB
 	Cfg     *config.Config
 }
@@ -65,8 +68,16 @@ func (r *Registry) Close() {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 	for name, e := range r.entries {
-		if e != nil && e.DB != nil {
-			_ = e.DB.Close()
+		if e != nil {
+			// Stop this workspace's background sync before closing the mirror
+			// it writes to; a job that outlives the DB holds a WAL connection
+			// (GDK-270).
+			if e.Handler != nil {
+				_ = e.Handler.Close()
+			}
+			if e.DB != nil {
+				_ = e.DB.Close()
+			}
 		}
 		delete(r.entries, name)
 	}

internal/workspace/workspace_test.go

@@ -442,3 +442,44 @@ func TestWorkspaceConnectStartsWatch(t *testing.T) {
 		t.Fatalf("workspace connect did not start Watch: watching %v", reg.Watching())
 	}
 }
+
+// TestCloseStopsWorkspaceSyncBeforeTheMirror is the production half of
+// GDK-270. The server learned to own its background sync, but a Registry that
+// closed only e.DB would still leave a workspace's job writing into a mirror
+// that had just been closed — the same leak, one layer up. Driving a real
+// scope-changing settings PUT is what starts that job; the profile's Site is a
+// dead loopback port, so the sync fails fast without leaving this machine.
+func TestCloseStopsWorkspaceSyncBeforeTheMirror(t *testing.T) {
+	setupHome(t)
+	seedProfile(t, "", &config.Config{
+		Site: "http://127.0.0.1:1", Email: "a@example.invalid", Token: "test-token", Projects: []string{"AAA"},
+	})
+	seedProfile(t, "work", &config.Config{
+		Site: "http://127.0.0.1:1", Email: "b@example.invalid", Token: "test-token", Projects: []string{"BBB"},
+	})
+
+	reg := New()
+	e, err := reg.Get("work")
+	if err != nil {
+		t.Fatalf("Get: %v", err)
+	}
+	db := e.DB
+
+	spa := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })
+	h := reg.Handler(spa, "test-ver")
+	rec := httptest.NewRecorder()
+	body := strings.NewReader(`{"projects":["BBB","CCC"],"staleThresholdHours":72}`)
+	req := httptest.NewRequest(http.MethodPut, "/w/work/api/v1/issues/settings/", body)
+	req.Host = "127.0.0.1:7777"
+	req.Header.Set("Content-Type", "application/json")
+	h.ServeHTTP(rec, req)
+	if rec.Code != http.StatusOK {
+		t.Fatalf("PUT settings → %d %s", rec.Code, rec.Body.String())
+	}
+
+	reg.Close()
+
+	if n := db.PoolStats().InUse; n != 0 {
+		t.Fatalf("%d connection(s) still checked out after Registry.Close — the workspace's sync outlived its mirror (GDK-270)", n)
+	}
+}