fix(acp): interrupt idle reads on cancellation

[PierrunoYT]

Summary

• retain a closable ACP input so context cancellation can interrupt an idle blocking read
• arbitrate cancellation and terminal read completion exactly once, preserving EOF and genuine read errors when the read finishes first
• join the cancellation watcher before Serve returns
• cover active idle-read cancellation and terminal-error races for closable and non-closable readers

Validation

• go test ./internal/acp -count=1
• focused cancellation/error regressions with -count=100
• go test ./internal/cli -count=1
• go test ./... -count=1
• go vet ./...
• go run golang.org/x/vuln/cmd/govulncheck@v1.3.0 ./... with GOTOOLCHAIN=go1.26.5 — no vulnerabilities found
• gofmt -l internal/acp/jsonrpc.go internal/acp/jsonrpc_test.go
• git diff --check

The pinned golangci-lint command still reports the repository's 35 known baseline findings tracked by #743; none are in the files changed here.

Fixes #777

Summary by CodeRabbit

• Bug Fixes

  • Improved connection shutdown when serving is cancelled, including interrupting idle reads promptly.
  • Preserved terminal read errors instead of reporting cancellation when a connection ends unexpectedly.
  • Improved handling of pending responses during connection shutdown to prevent dropped results.

• Tests

  • Added coverage for cancellation during idle reads and terminal read errors.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4a2a16dc-8957-44c4-ac06-a9f7744f0970

📥 Commits

Reviewing files that changed from the base of the PR and between 3967d49 and 0dc92a9.

📒 Files selected for processing (2)

• internal/acp/jsonrpc.go
• internal/acp/jsonrpc_test.go

---

Walkthrough

Changes

ACP read shutdown

Layer / File(s)	Summary
Reader closer capture internal/acp/jsonrpc.go	Conn stores an optional inbound io.Closer, and NewConn records it when the reader supports closing.
Serve cancellation and termination internal/acp/jsonrpc.go, internal/acp/jsonrpc_test.go	Serve interrupts blocked reads on cancellation, coordinates cleanup, preserves terminal read errors, and tests closable and non-closable readers.
Estimated code review effort: 3 (Moderate)	~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Serve
  participant ContextWatcher
  participant ReaderCloser
  participant ReadLoop
  ContextWatcher->>Serve: observe ctx.Done()
  Serve->>ReaderCloser: close inbound reader
  ReaderCloser->>ReadLoop: unblock ReadBytes
  ReadLoop->>Serve: report interrupted termination
  Serve->>Serve: cancel in-flight work and await read loop

Loading

Suggested reviewers: copilot, gnanam1990

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: interrupting idle ACP reads when cancellation occurs.
Linked Issues check	✅ Passed	The implementation matches #777 by interrupting idle reads on cancellation, preserving terminal errors, and adding cancellation/race tests.
Out of Scope Changes check	✅ Passed	The changes stay focused on ACP serve cancellation behavior and regression tests, with no clear unrelated additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

This PR improves ACP server shutdown behavior by ensuring context cancellation can interrupt an otherwise-idle blocking NDJSON read, while preserving “real” terminal read outcomes (EOF / genuine read errors) when the read completes first.

Changes:

• Track an optional io.Closer for the input reader and close it on context cancellation to break out of idle ReadBytes blocking.
• Add single-arbitration logic between cancellation-triggered interruption and terminal read completion, and join the cancellation watcher before returning from Serve.
• Add tests covering idle-read cancellation and cancellation vs. terminal-read-error races for both closable and non-closable readers.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
internal/acp/jsonrpc.go	Retains a closable read transport and adds cancellation-driven read interruption with single-arbitration and watcher join.
internal/acp/jsonrpc_test.go	Adds regression tests for idle read cancellation and cancellation/error race behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P2] Preserve a terminal read error that races cancellation
  internal/acp/jsonrpc.go:165
  A real non-EOF ReadBytes failure can occur just as ctx is cancelled. If the watcher is scheduled first, it wins endRead(true) and sets readInterrupted; when the read loop subsequently handles the already-returned error, its endRead(false) is a no-op and line 175 returns nil. This silently reports a broken ACP transport as a clean shutdown, despite the stated terminal-error preservation. Record/read the terminal read outcome before allowing cancellation to claim the transport (and add a regression that races a non-EOF read failure with cancellation).