Skip to content

Bound JSON-RPC output without reordering notifications and responses - #57

Merged
JaredP94 merged 2 commits into
mainfrom
fix/bounded-notifications
Nov 6, 2026
Merged

JaredP94 merged 2 commits into
mainfrom
fix/bounded-notifications

Conversation

@JaredP94

@JaredP94 JaredP94 commented Nov 6, 2026 •

Copy link
Copy Markdown
Owner

Description

A stopped JSON-RPC reader must not hold worker state locks or broker request dispatch indefinitely. Give each codec one bounded, ordered output writer; notifications, requests and responses retain their FIFO order and existing protocol-v2 envelopes. Overflow, write errors and deadlines kill the stream instead of silently dropping events on a live connection. Existing owners retain recovery policy.

The bound is 256 outstanding frames / 16 MiB (including the active frame), with a five-second deadline from admission, including queue time. Notify/Respond wait at most 10 ms for immediate completion, then report acceptance. The reproducible synthetic startup fixture measures 153 frames / 198,511 bytes and approximately 1.5 ms with a healthy reader; measurements and limitations are in services/shared/jsonrpc/README.md.

Scope

  • Codec FIFO, cancellation, short writes, terminal metadata-only diagnostic, bounded Flush/Drain/Close and peer failure propagation. Relay requests enter the same FIFO synchronously rather than using a separate goroutine chain.
  • Necessary lifecycle call sites only: twelve stdio adapters now provide interruptible closure and entry points defer codec Close; broker subprocess adapters close on failure, seven worker-handle kinds drain before control EOF within existing shutdown budgets, and the broker drains its shutdown acknowledgement before cancellation. TUI retains stdout draining through owned-process cleanup and observes codec failure immediately.
  • Tests migrate synchronous-output assumptions where required. Real lock-holding callers do not need business-logic or lock restructuring. No wire methods or payloads change.
  • Shared is a library. Every linked binary receives a patch bump relative to v0.9.0: all three proxies 1.5.0→1.5.1; node-info and engine-manager 1.4.0→1.4.1; scanner, manual-nodes and TUI 1.3.0→1.3.1; workload-manager 1.3.1→1.3.2; errors 1.1.1→1.1.2; node-settings 3.2.0→3.2.1; broker 2.5.0→2.5.1; cluster-manager 2.2.0→2.2.1; scheduler 1.1.0→1.1.1. Product/installer 3.0.0→3.0.1.

Validation

  • Six named-caller regressions demonstrated failure with the origin/main codec, then passed with this writer: pairing completion/rosterMu, peer metadata/stdinMu, error report, error clear, error replay/error mutex, public port-assignment rejection. Temporary compile-only lifecycle shims were used for the old codec; no shim remains in the change.
  • Codec tests cover stalled reader admission, concurrent notification/response ordering, relay ordering, frame/byte overflow, real five-second deadlines for net.Pipe and an inherited OS pipe, clean flush, bounded shutdown, flush barriers during later admissions, terminal logging without payloads, cancellation and goroutine leaks using goleak.
  • In all 16 non-cross-process Go modules: go vet ./..., GOOS=linux go vet ./..., GOOS=windows go vet ./..., go test -race -count=1 -timeout=5m ./..., and go test -count=1 -timeout=5m ./... passed on macOS arm64 / Go 1.27.3. The initial engine-manager race run exposed an asynchronous diagnostic/synchronous buffer assertion; explicit codec Close/join fixed that test, and the complete final run passed.
  • Shared ordering/barrier/diagnostic regressions plus the six named-caller regressions passed 20 race-enabled repetitions; engine-manager health regressions passed ten race-enabled repetitions.
  • The first Linux CI run exposed a fallback test-fixture latch also handling a later metadata RPC. The test-only follow-up distinguishes RPC kinds and echoes the actual selected port. Fifty race-enabled repetitions, the broker's full race and normal suites, and vet for all three targets passed after that correction.
  • gofmt -l services empty; git diff --cached --check clean; SPDX checker reports zero missing headers.
  • Worktree-local cd desktop && npm ci --no-audit --no-fund && npm run -s service-contracts:check passed; npm run -s licenses:check also passed. No shared/symlinked node_modules.
  • Cross-process tests were not run locally. Final CI run 42207474165, head 4cf7acde: five jobs passed (SPDX, Desktop, Linux Go modules, Linux cross-process, macOS cross-process). The sixth, Go modules (macos-15), never acquired a runner: runner_id: 0, no steps, and GitHub's annotation says “The job was not acquired by Runner of type hosted even after multiple attempts.” GitHub cancelled that job and marked the overall workflow failed; this is not six green jobs. No CI workflow changes or local substitute are claimed as hosted macOS verification.
  • Downloaded cross-process-ubuntu-latest from that final run using gh run download into a new empty directory, then ran python3 -I /tmp/pair-review/scripts/noskip.py <dir>/cross-process.jsonl: 122 passed, zero skips.

Risk

  • Notify/Respond success means acceptance rather than guaranteed physical delivery; clean owners must Drain/Close before EOF. A terminal stream can contain a partial final frame but is never reused.
  • Large/sustained bursts beyond the documented limits are terminal by design, not a coalescing or drop-and-continue policy.
  • Arbitrary io.Writers with ineffective Close cannot be made interruptible. Production stdio/IPC/subprocess transports expose effective closure. Windows is cross-vet checked locally, not runtime-certified by Unix tests.
  • node-info's separate applog Notifier remains unchanged; it uses Codec only for reading. Existing supervision/reconnect policy is unchanged.

Checklist

  • I have read the Contributing Guidelines.
  • Every commit is signed off with the configured author's DCO trailer.
  • New or existing tests cover the change.
  • Relevant documentation is updated.
  • I checked changed filenames, diff and commit messages for private data and generated artifacts.
  • I recorded final validation commands and results above.
  • I bumped affected components and described the user-visible change above.

A stalled stdio reader can otherwise keep Notify inside a worker state lock forever. Serialize notifications, requests and responses through one bounded FIFO, preserve protocol-v2 wire ordering, and terminate the stream on overflow, write failure or its admission-relative deadline.

Make production stdio closure interruptible and drain accepted output inside existing shutdown budgets. Add real lock-holder regressions, ordering and terminal-stream tests, inherited-pipe deadline coverage, goroutine leak checks, and a reproducible sizing fixture. Patch-bump every linked service binary from the released versions.

Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com>
The fallback ordering fixture also receives the asynchronous node/set-local-backend RPC after the ownership gate opens. Treating that metadata write as another fallback closed its latch twice on the Linux runner. Restrict the latch to set-port, make its signal idempotent, and echo the selected port instead of assuming one host-specific free port.

Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com>
@JaredP94
JaredP94 force-pushed the fix/bounded-notifications branch from 4cf7acd to 7416157 Compare November 6, 2026 11:23
@JaredP94
JaredP94 merged commit 25de84a into main Nov 6, 2026
11 of 12 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.

1 participant