feat(relay): fence and drain file and git response streams on shutdown

[OrcaWin]

	Files	Added	Deleted	Net
Test	4	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​442	0	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​442
Prod	7	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​226	$\color{#cf222e}{\Huge{\mathbf{−}}}$​66	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​160

ELI5

Before the relay shuts down, it now waits for in-flight file and git response streams to finish and close their file handles. Once shutdown starts, new streams are refused. This ports the stream-shutdown slice of #16741. Nothing in production starts a drain yet.

What Changed

• RelayStreamRegistry:
  • disposeAll blocks new streams and waits for every close and admitted operation.
  • It fails with relay_file_stream_shutdown_incomplete if a handle stays open.
• GitResponseStreamRegistry:
  • Fences new streams, and adds disposeAllAndWait for in-flight pumps.
  • An aborted pump no longer sends git.responseEnd.
• Terminal-frame slots: moved unchanged into fs-stream-terminal-frame-slots.ts. Each slot now holds a registry operation.
• disposeOwnedProcesses: now waits for both stream drains. If either fails, shutdown is postponed instead of exiting with a handle still open.
• Porting notes are in the commit bodies:
  • Taken verbatim: the shutdown tests and the fs-handler changes.
  • Adapted: no Promise.withResolvers, since the relay targets node18; type guards instead of casts.
• Wire: no changes.

Why

Design D9, T1: this lets a later shutdown or reset (T3) drain safely without leaking file handles.

Linked Issue

Part of the Node runtime plan (Phase 3). Stacked on #24181.

Visual Proof

N/A. There's no user-visible change yet.

Testing

• 37 stream tests pass.

• Added in review: coverage for the stream drains in disposeOwnedProcesses.

• The full relay suite is green apart from 3 PTY tests that time out under full-suite load; they pass on their own.

• Known behaviour, same as feat(orcad): consolidate Bun runtime and SSH/Remote migration #16741: a connected client whose socket stops draining can keep shutdown open.

• I manually tested these changes locally

• Automated tests added/updated, or explained why not below

Agent skill upstream boundary

• Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

[greptile-apps]

Confidence Score: 3/5

[High risk] Adds graceful shutdown coordination to relay request and stream handling.

The PR does not appear safe to merge while shutdown can remain blocked by an unsettled socket write or be deferred after a handle has successfully closed.

Findings

1.  Shutdown can wait forever ▶
2.  Recovered close still fails shutdown ▶

Summary

The PR fences new file and git response streams during shutdown and waits for their pumps, file handles, and outstanding publications to settle.

• Adds dispatcher work admission and producer-publication settlement tracking.
• Adds shutdown tests for stream drains and owned-process disposal.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Shutdown begins] --> B[Fence new streams]
  B --> C[Drain git response pumps]
  B --> D[Close file handles and await stream operations]
  C --> E[Dispose remaining owned processes]
  D --> E
  E --> F{All drains complete?}
  F -->|Yes| G[Continue shutdown]
  F -->|No| H[Defer shutdown]

Loading

_{Reviews (3) · Last reviewed commit: "test(relay): cover the stream drains in ..."}

[greptile-apps]

Shutdown can wait forever When shutdown starts while a client socket has stopped draining, a file terminal frame or git chunk write can remain unsettled. These awaits block until the stream drains finish, but the dispatcher writer is closed only afterward. The close that would settle the write is therefore never reached, so shutdown cannot complete.

[greptile-apps]

Recovered close still fails shutdown If a handle close fails during disposal but an in-flight pump retries and closes it before its operation finishes, failures still contains the first rejection. Disposal reports relay_file_stream_shutdown_incomplete even though no handles remain, causing the lifecycle to defer shutdown until another attempt.

[pullfrog]

✅ No new issues found.

Reviewed changes — the stream-shutdown slice of the Node runtime plan (Design D9/T1), reviewed at 5da66b8 against 04c9331.

• RelayStreamRegistry drain and fence. Adds a disposed flag, beginOperation()/finish() refcounting, a coalesced async release (EBADF tolerated, other close failures retained for retry), and a disposeAll() that aborts every stream, awaits each close and every admitted operation, and rejects with relay_file_stream_shutdown_incomplete while a handle remains.
• File-stream pump accounting. readRelayFileStreamMetadata wraps metadata prep in one operation and the detached pump in another; terminal-frame slot reservation moved verbatim into fs-stream-terminal-frame-slots.ts, where each slot is itself a registry operation so shutdown also waits for undelivered terminal frames.
• GitResponseStreamRegistry drain and fence. Fences new streams via disposed, tracks in-flight pumps in pendingPumps, adds disposeAllAndWait, and suppresses git.responseEnd/git.responseError once a pump is aborted or stale.
• disposeOwnedProcesses. Kicks off both registry drains, aggregates their failures, and rejects relay_owned_process_shutdown_incomplete so the grace lifecycle defers shutdown and retries instead of exiting with a file handle still open.
• Tests. New shutdown suites for both registries and for disposeOwnedProcesses, plus an fs-handler case for an undelivered terminal frame. All 33 tests in the four touched files pass.

I traced the hang/leak candidates in this change independently: disposed is set synchronously before the first await, so the operations/pendingPumps snapshots are always complete; the register fence and the post-register beginOperation share a gap-free synchronous window, so no slot or descriptor can be acquired without being drained; and the slot-release closure is idempotent and reached on every internal path. The only path that can hold a drain open is a sink that never settles — the "client socket stops draining" case the PR documents. Wire is unchanged.

ℹ️ Shutdown deferral has no upper bound

disposeOwnedProcesses now rejects when a drain cannot finish, and relay-grace-lifecycle.ts only re-arms a retry while readSocketClientCount() === 0. If a client that stops draining keeps a control write or a bulk notifyBulk unsettled, no retry is armed and the daemon lingers indefinitely. This is the behaviour the PR documents as known, so no change is requested here — flagging only so the boundary is explicit.

Technical details

# Deferred shutdown retry is client-count-gated

## Affected sites
- `src/relay/relay-runtime-services.ts:126` — `disposeOwnedProcesses` now rejects `relay_owned_process_shutdown_incomplete` when a registry drain fails.
- `src/relay/relay-grace-lifecycle.ts:132-140` — on rejection it clears `shutdownInFlight` and re-arms a `shutdown-deferred` grace only when `readSocketClientCount() === 0`.
- `src/relay/relay-grace-branch.ts:121-138` — the `shutdown-deferred` branch is bounded by `idleRelayGraceMs`, so retries do continue while the relay is clientless.

## Required outcome
None if indefinite deferral is the intended posture. Otherwise, cap the retries (or force exit after N) so a permanently unstuck sink cannot keep the daemon alive.

## Open questions for the human
- Is a bound on deferral wanted, or is unbounded deferral intended until the T2 lifecycle port revisits this?

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used

📚 Code guidelines (1)

AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 42263dc1-ef25-4662-8b0f-52d31190cc0d

📥 Commits

Reviewing files that changed from the base of the PR and between 0b812bd and 3977d5b.

📒 Files selected for processing (11)

• src/relay/fs-handler-file-read.ts
• src/relay/fs-handler-stream.test.ts
• src/relay/fs-handler-utils.ts
• src/relay/fs-handler.ts
• src/relay/fs-stream-registry-shutdown.test.ts
• src/relay/fs-stream-registry.ts
• src/relay/fs-stream-terminal-frame-slots.ts
• src/relay/git-response-stream-shutdown.test.ts
• src/relay/git-response-stream.ts
• src/relay/relay-runtime-owned-process-shutdown.test.ts
• src/relay/relay-runtime-services.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

---

📝 Walkthrough

Walkthrough

File and Git response stream registries now track active work and fence new streams during shutdown. File stream setup and cleanup release handles through the registry, and terminal-frame slots remain tracked until released. Runtime-owned process cleanup awaits both stream drains and reports their failures together.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 3977d

The stream drains have no identified blocking defect. Disconnected clients release pending terminal frames; a connected client that stops draining can intentionally delay shutdown. Mergeable subject to normal checks.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 3977d

The change improves cleanup but already affects production shutdown. A live client with stalled output can postpone graceful shutdown and therefore delay restart or rollback. Disconnect releases pending writes, and the supported impact is limited to one relay process.

Retained concerns

• Medium · reliability · inferred: Graceful shutdown now waits for client-backed stream writes before closing the dispatcher that can cancel those writes. A live, non-draining client can therefore postpone daemon termination while stream admission is already permanently fenced and other cleanup has begun. This affects shutdown and rollback containment within that daemon; disconnect resolves the dependency, and external forced-termination limits remain unestablished.

Security review details

Security Blast Radius

• inferred — A client-held publication can affect graceful shutdown of the whole owning daemon, including streams from other attached clients. The supported outcome is delayed termination and restart, not additional filesystem access, credential access, or cross-daemon authority.

Security Findings and Attack Paths

• inferred — A client able to request streamed output can leave a write unsettled while its transport remains live. If graceful shutdown then begins, pump abortion does not cancel that write, and the new drain waits before reaching dispatcher closure. This is a shutdown-containment concern supported by static control flow, not a demonstrated runtime exploit.

Trust Boundaries and Controls

• observed — Socket admission continues through the existing endpoint handshake; configurations requiring a credential refuse connections before credential publication. Stream ACK and abort ownership checks remain in the Git registry. The changed shutdown coordination does not add a dispatcher shutdown route.

Resilience and Maintainability Implications

• observed — Disconnect and writer closure settle queued and in-flight publications, so ordinary peer loss is not an orphaned reservation. Disposal also wakes ACK waiters, and Git completion tracking settles on pump fulfillment or rejection. These controls mitigate disconnected-client stalls but do not themselves cancel a pending write on a still-live transport.

Hardening Proposals

• proposed — Define a bounded graceful-publication phase followed by explicit transport cancellation that settles outstanding callbacks before joining the remaining stream work. Preserve failed file-handle ownership and incomplete-shutdown reporting rather than treating a timeout as successful cleanup.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 11 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: shutdown fencing and draining for relay file and Git response streams.
Description check	✅ Passed	The description covers the required ELI5, change summary, rationale, issue reference, visual proof, testing, and upstream-boundary sections. It provides meaningful test results and known limitations. …
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}