Skip to content

fix(jsonrpc): read response before waiting - #58

Merged
qiz029 merged 1 commit into
mainfrom
codex/fix-jsonrpc-stdout-race
Aug 5, 2026
Merged

fix(jsonrpc): read response before waiting#58
qiz029 merged 1 commit into
mainfrom
codex/fix-jsonrpc-stdout-race

Conversation

@qiz029

@qiz029 qiz029 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

What changed

  • Decode the complete JSON-RPC response before calling Cmd.Wait.
  • Preserve process termination and cleanup after the response is read.
  • Add a regression test using a fast provider that writes a large valid response and exits immediately.

Root cause

Provider.call started Cmd.Wait in a goroutine before decoding StdoutPipe. Go closes the pipe when Wait completes, so a fast-exiting provider could race the decoder and produce read |0: file already closed even though it emitted a valid response.

Impact

JSON-RPC providers no longer fail nondeterministically when they exit quickly after writing a response. Timeout behavior remains controlled by exec.CommandContext.

Validation

  • Regression test on old implementation: failed 9 of 10 runs with file already closed
  • Regression test after fix: 100 of 100 runs passed
  • Original CLI conformance test: 30 of 30 runs passed
  • go test ./... -count=1
  • go vet ./...
  • golangci-lint fmt --diff
  • scripts/check-production-complexity.sh
  • Product coverage: 78.1% (required 75%)

Local golangci-lint run was blocked by a broken local Go toolchain (go1.26.4 loading no packages; the cached Go 1.25 toolchain is missing unsafe). GitHub Actions provides the clean Go 1.25 lint result.

Copilot AI lite review requested due to automatic review settings August 5, 2026 20:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a JSON-RPC stdio race where calling Cmd.Wait() too early could close StdoutPipe while the response decoder was still reading, causing nondeterministic failures for fast-exiting providers. The fix ensures the full JSON-RPC response is decoded before waiting, and adds a regression test that reproduces the prior failure mode with a large, valid response.

Changes:

  • Reorder process lifecycle in Provider.call to decode stdout fully before invoking cmd.Wait().
  • Preserve cleanup behavior by still terminating the provider process after reading the response.
  • Add a regression test using a helper provider mode that emits a large response and exits immediately.

Reviewed changes

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

File Description
internal/adapters/jsonrpc/provider.go Decodes the JSON-RPC response before cmd.Wait() to avoid StdoutPipe being closed mid-decode.
internal/adapters/jsonrpc/provider_test.go Adds a regression test and helper mode producing a large fast response to validate the race fix.

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

@qiz029
qiz029 marked this pull request as ready for review August 5, 2026 21:06
@qiz029
qiz029 merged commit 1c2ee2b into main Aug 5, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants