sync: fix "send on closed channel" panic in responsesWorker at teardown

[GrapeBaBa]

Problem

(*DefaultClient).responsesWorker panics with send on closed channel and crashes the process at teardown:

panic: send on closed channel
goroutine N [running]:
github.com/testground/sdk-go/sync.(*DefaultClient).responsesWorker(...)
    .../sync/client_conn.go:43
created by github.com/testground/sdk-go/sync.newClient
    .../sync/client.go:118

I hit this reliably while running the testground daemon for a multi-instance test: at run teardown (right after all containers are complete / deleting containers) the daemon's sync client panics, the daemon process dies, and every subsequent testground run then fails with dial tcp [::1]:8042: connect: connection refused — the run reports outcome ... canceled.

Root cause — a close-vs-send race on the handler channel

In sync/client_conn.go:

• responsesWorker() looks up the per-request handler channel under handlersMu, releases the lock, then sends ch <- res unlocked.
• makeRequest()'s ctx-cancellation goroutine, on teardown, takes handlersMu, then close(c.handlers[req.ID]), delete(...), unlocks.

If a response arrives for a request whose handler is closed in the window between responsesWorker releasing the lock and performing the send, responsesWorker sends on a now-closed channel → panic. It's probabilistic (more in-flight requests / teardown timing → more likely); low-traffic runs often dodge it, busy ones hit it every time.

A third goroutine closing the channel that responsesWorker is the sole sender of is the underlying problem — in Go the sender should own the close, and there must be no send after close.

Note: wrapping the send in a select { case ch <- res: case <-done: } does not fix this — a send case on a closed channel is still "ready" and may be chosen, so it still panics. The send must never see a closed channel.

Fix — never close the handler channel from the cancel goroutine

Make responsesWorker the channel's only writer and never close ch from a third goroutine; use a per-request done channel for teardown signalling, and unblock one-shot receivers via the context instead of via channel-close.

• Each handler is now a pendingRequest{ ch, done }. The cancel goroutine deletes the handler and close(done) — it never closes ch.

• responsesWorker delivers with select { case h.ch <- res: case <-h.done: }: it can never panic on send-to-closed (ch is never closed), and never blocks forever (done is closed when the request/client context fires — every caller wraps the request in a cancellable context with defer cancel()).

• Because ch is no longer closed, the one-shot receivers (publish, SignalEntry, and the Barrier goroutine) unblock via context through a small helper:

  func (c *DefaultClient) awaitResponse(ctx context.Context, ch chan *sync.Response) (*sync.Response, error) {
      select {
      case res := <-ch:
          return res, nil
      case <-ctx.Done():
          return nil, ctx.Err()
      case <-c.ctx.Done():
          return nil, errors.New("client closed before getting response")
      }
  }

• subscribe already selects on c.ctx.Done() / ctx.Done() / resCh, so it still terminates via its contexts (the resCh close branch simply becomes unreachable); streaming delivery still works through the send-vs-done select.

Verification

• go build ./... and go vet ./sync/ pass; gofmt clean on all changed files.
• Reviewed for the four failure modes: (1) grep close( confirms ch is never closed anywhere (only done); (2) done is always eventually closed for every registered handler, so responsesWorker never blocks; (3) every one-shot receiver unblocks on both the request ctx and the client ctx (no hang); (4) subscribe still terminates and streams.
• The equivalent minimal fix (a recover() around the racy send) was additionally runtime-confirmed: with it, a 3-instance multi-impl testground run that previously crashed the daemon at teardown completed cleanly (outcome = success) across repeated runs. This PR is the cleaner, structural form of that same fix.

Alternative

If you'd prefer the minimal change, a one-line recover() around the send also stops the crash without the refactor — happy to switch to that if that's more in line with how you'd like to fix it.