Skip to content

ipc: address post-review feedback on unified JSON-RPC layer#5

Merged
darkliquid merged 5 commits intofeat/protojsonfrom
copilot/sub-pr-3-again
Mar 15, 2026
Merged

ipc: address post-review feedback on unified JSON-RPC layer#5
darkliquid merged 5 commits intofeat/protojsonfrom
copilot/sub-pr-3-again

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 15, 2026

Fixes a set of correctness, robustness, and forward-compatibility issues flagged in review of the IPC unification PR.

Changes

server.go

  • Naming: rename cwconn in closeActiveConnections range loop — the map key is net.Conn, not *connWriter
  • Doc: drop "asynchronously" from BroadcastEvent godoc — it writes synchronously in the caller's goroutine
  • Forward-compat: use protojson.UnmarshalOptions{DiscardUnknown: true} so unknown fields from newer clients don't cause dropped frames

client.go

  • Connection-drop safety: add drainReqs(), deferred at the top of readLoop, which closes all outstanding response channels on EOF/scanner error — prevents Call() from blocking indefinitely after a connection drop
  • Forward-compat: same DiscardUnknown: true treatment as server side
  • Call(): handle closed-channel receive (resp, ok := <-ch) and return "ipc: connection closed" instead of a nil-dereference

Tests (extra_test.go)

  • TestBroadcastEvent_Delivery — connects a raw net.Conn, broadcasts an IndexUpdatedEvent, reads and unmarshals the frame end-to-end
  • TestBroadcastEvent_NoInterleave — races a broadcast against a blocked handler to assert response and event writes don't corrupt each other
  • TestClient_ConnectionDrop — forcibly closes the underlying connection while a Call() is in-flight and asserts it returns an error promptly

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

…tion-drop cleanup, and broadcast tests

Co-authored-by: darkliquid <31256+darkliquid@users.noreply.github.com>
Copilot AI changed the title [WIP] Unify IPC JSON-RPC for Quickshell and daemon ipc: address post-review feedback on unified JSON-RPC layer Mar 15, 2026
Copilot AI requested a review from darkliquid March 15, 2026 17:55
@darkliquid darkliquid marked this pull request as ready for review March 15, 2026 17:56
Copilot AI review requested due to automatic review settings March 15, 2026 17:56
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the internal IPC unified JSON-RPC framing to be more forward-compatible and robust to connection drops, and adds regression tests around event broadcasting and client disconnect behavior.

Changes:

  • Use protojson.UnmarshalOptions{DiscardUnknown: true} on both server and client to tolerate unknown fields from newer peers.
  • Ensure in-flight Client.Call() unblocks promptly when the connection drops by draining outstanding request channels on read loop exit.
  • Add end-to-end tests for BroadcastEvent delivery/interleaving and for client connection-drop behavior.

Reviewed changes

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

File Description
internal/ipc/server.go Adjusts naming/docs and makes envelope unmarshalling forward-compatible by discarding unknown fields.
internal/ipc/client.go Adds request-draining on read loop exit, forward-compatible unmarshalling, and safer Call() channel receive handling.
internal/ipc/extra_test.go Adds new tests covering broadcast delivery/interleaving and client behavior on connection drop.

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

Comment thread internal/ipc/extra_test.go
Comment thread internal/ipc/extra_test.go Outdated
Comment thread internal/ipc/extra_test.go Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment thread internal/ipc/extra_test.go Outdated
Comment thread internal/ipc/extra_test.go Outdated
@darkliquid darkliquid merged commit 6c17ef0 into feat/protojson Mar 15, 2026
7 of 8 checks passed
darkliquid added a commit that referenced this pull request Mar 15, 2026
* feat: unify IPC JSON-RPC

* fix: add missing js

* fix: add missing files

* ipc: enforce max frame size on write path (#4)

* Initial plan

* fix: add max frame size check in WriteEnvelope

Co-authored-by: darkliquid <31256+darkliquid@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: darkliquid <31256+darkliquid@users.noreply.github.com>

* ipc: address post-review feedback on unified JSON-RPC layer (#5)

* Initial plan

* fix: address all IPC review feedback - naming, forward-compat, connection-drop cleanup, and broadcast tests

Co-authored-by: darkliquid <31256+darkliquid@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* Update internal/ipc/extra_test.go

* Update internal/ipc/extra_test.go

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: darkliquid <31256+darkliquid@users.noreply.github.com>
Co-authored-by: Andrew Montgomery <darkliquid@darkliquid.co.uk>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* fix: eliminate race condition in TestBroadcastEvent_Delivery (#6)

* Initial plan

* fix: resolve race condition in TestBroadcastEvent_Delivery

Co-authored-by: darkliquid <31256+darkliquid@users.noreply.github.com>

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: darkliquid <31256+darkliquid@users.noreply.github.com>
Co-authored-by: Andrew Montgomery <darkliquid@darkliquid.co.uk>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: darkliquid <31256+darkliquid@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.

3 participants