Skip to content

[35/36] Stabilize gRPC demo server reliability - #70

Open
cjohnsto-nz wants to merge 6 commits into
supervisor/add-request-action-first-click-taskfrom
feature/oc-160-grpc-demo-server-reliability
Open

[35/36] Stabilize gRPC demo server reliability#70
cjohnsto-nz wants to merge 6 commits into
supervisor/add-request-action-first-click-taskfrom
feature/oc-160-grpc-demo-server-reliability

Conversation

@cjohnsto-nz

@cjohnsto-nz cjohnsto-nz commented Jun 15, 2026

Copy link
Copy Markdown
Owner

Stack PR for the Missio 0.8.0 OpenCollection review queue.

Scope

Completes OC-160 by making the separate local gRPC fixture discoverable, configurable, deterministic, and integration-tested across all seven packaged demo requests. The implementation keeps generic transport errors unbranded; demo-specific startup guidance remains in the demo README, fixture logs, folder, and request descriptions.

Review rework is included in 98cf520:

  • unavailable-target tests retain ownership of an ephemeral loopback listener for the entire assertion and reject connections deterministically, eliminating the reserve-close-reuse race
  • the gRPC fixture handles SIGINT and SIGTERM with bounded tryShutdown, then force-shuts down only if the graceful path exceeds two seconds
  • test teardown now waits for process closure and fails on timeout; Linux CI requires the fixture's graceful-shutdown confirmation

An aggregate npm run demo remains intentionally out of scope: OC-160 lists it as optional, the companion-process boundary is documented explicitly, and the integration test starts/stops the real gRPC process itself.

Verification

  • npm run compile
  • focused OC-160 suite: 3/3 tests passed
  • targeted gRPC/runtime/provider/tool/schema/validation suite: 57/57 tests passed
  • npm test: 25 files, 480 tests passed
  • node scripts/validate-collection.js examples/demo-api: 47/47 files passed
  • npm run build
  • GitHub Linux build and Socket security checks passed at 98cf520

Stack integrity

Pre-push three-way merge simulations passed for PR 69 -> 70, PR 70 -> 71, and PR 70 -> rework PR 72. Post-push GitHub reports PR 70, immediate child PR 71, and rework PR 72 as MERGEABLE / CLEAN.

@APKiwi

APKiwi commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Verdict: approve with nits. Key question for a reliability PR answered: this addresses the root cause, not a mask. The failure was that the gRPC fixture is a separate process nothing told users to start, and the fix is guidance (README, server.js pointer, docs strings) plus a real testability seam (MISSIO_GRPC_PORT with validation, readiness log from the bindAsync-assigned port so port 0 works) and genuine integration coverage. No retries, timeouts or sleeps anywhere. Note the src/ changes visible in the ledger cancel out across the PR boundary (839c15d fully reverted by 96b5040), so the net increment is docs + demo hardening + tests.

  • Minor: reserveClosedLocalPort (test/grpcSupport.test.ts:280) has a TOCTOU window, the OS could reassign the reserved port between close and the client connect, in which case the diagnostic tests wouldn't observe UNAVAILABLE. Very low probability on loopback, but it's a nondeterministic seam in a suite that exists to prove reliability.
  • Nit: grpc-server.js has no explicit SIGTERM/SIGINT tryShutdown, relies on default process termination to free the port. Meets the task's criterion only by default behavior.
  • Observation, not a change request: reliability still hinges on the user reading docs and starting a second process. An aggregate npm run demo that spawns both would be structurally more robust, the task listed it as optional.

Tests: strong. The smoke test spawns the real grpc-server.js on an ephemeral port, parses stdout for readiness, drives all 7 packaged demos through RequestExecutionService, asserts concrete bodies including the runtime summary, and kills the child in finally. It would fail if the fixture or the execution path regressed, a genuine integration test.

Deps: none added (test imports are Node builtins).

@cjohnsto-nz

Copy link
Copy Markdown
Owner Author

Addressed the actionable reliability findings in 98cf520.

  • The reserveClosedLocalPort race is gone. The failure tests now own an ephemeral loopback listener for the full assertion and immediately destroy accepted sockets. That guarantees the target remains unavailable without giving another process a window to claim the port.
  • grpc-server.js now handles SIGINT and SIGTERM with server.tryShutdown(), a two-second bounded fallback to forceShutdown(), and explicit stop diagnostics. Test teardown waits for the child to close and fails instead of silently continuing after a timeout. The GitHub Linux run also requires the graceful-stop confirmation, so the signal path is exercised on the platform where POSIX signals are delivered.
  • I did not add an aggregate npm run demo. OC-160 marks it optional; the separate-process architecture is now explicitly documented, and the integration suite starts, drives, and stops the real companion process. Adding a process supervisor would expand the product surface without closing an unmet acceptance criterion.

Verification: focused OC-160 3/3, targeted matrix 57/57, full suite 480/480, demo collection 47/47, compile and build passed. GitHub build/security checks pass. PR 69 -> 70, PR 70 -> 71, and direct integration with rework PR 72 are all clean, so no descendant implementation branch was modified.

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