Repository navigation
fix(relay): let one owner hold pty.ackData instead of relying on construction order - #15079
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 5 remain after this review. 📝 WalkthroughWalkthrough
Merge Risk: ⚪ Minimal · up to The change makes pty.ackData ownership explicit without changing wire behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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. Comment |
14652af to
b71ba9a
Compare
…truction order PtyHandler registered a no-op pty.ackData handler and SshPtyConsumerSessionAdapter registered the real sourceCredit.acknowledge for the same method. onNotification is a single slot, so only the adapter's survived — and only because relay.ts constructs it second. Reversing those two lines would have made every credit-mode delivery wedge permanently once the 256KB window emptied, with no error and no log. Delete the dead no-op, and make onNotification throw on a duplicate registration the way registerPtyDataPublicationAdmission already does for the admission slot. pty.ackData was the only double-registered method in the tree, so nothing else changes behavior. The existing test asserted only that pty.ackData appeared in the handler map, which stays true when the real handler is shadowed. It now asserts PtyHandler does not own the method.
b71ba9a to
0b9436a
Compare
|
Review-loop completion (PR #15079) Verdict: clean stop. The loop changed nothing beyond rebasing this branch onto current origin/main; the implementation and tests remain unchanged, so there was no justified code follow-up. Verification after the rebase:
Precedent leg: mature registries use sets/first-match behavior when multiple handlers are intentional, but APIs that define a single slot or unique provider identity reject duplicate registration loudly and return explicit ownership/disposal. For Orca's single-owner notification slot, rejecting a second registration and deleting the dead no-op in favor of the explicit owner matches that shape; silent last-write-wins would make behavior construction-order dependent. QA: N/A. This is relay-only with no UI surface, and a screenshot cannot demonstrate notification ownership or duplicate-registration rejection; no screenshots were manufactured. Known flake: src/relay/subprocess.test.ts — “uses configured grace after a detached relay has accepted a socket client” — spawns a real subprocess against a 10s timeout and can fail under machine load. It was confirmed as a flake by pristine-revert / restore / clean-run, not by this implementation. |
…truction order (stablyai#15079) PtyHandler registered a no-op pty.ackData handler and SshPtyConsumerSessionAdapter registered the real sourceCredit.acknowledge for the same method. onNotification is a single slot, so only the adapter's survived — and only because relay.ts constructs it second. Reversing those two lines would have made every credit-mode delivery wedge permanently once the 256KB window emptied, with no error and no log. Delete the dead no-op, and make onNotification throw on a duplicate registration the way registerPtyDataPublicationAdmission already does for the admission slot. pty.ackData was the only double-registered method in the tree, so nothing else changes behavior. The existing test asserted only that pty.ackData appeared in the handler map, which stays true when the real handler is shadowed. It now asserts PtyHandler does not own the method.
…truction order (stablyai#15079) PtyHandler registered a no-op pty.ackData handler and SshPtyConsumerSessionAdapter registered the real sourceCredit.acknowledge for the same method. onNotification is a single slot, so only the adapter's survived — and only because relay.ts constructs it second. Reversing those two lines would have made every credit-mode delivery wedge permanently once the 256KB window emptied, with no error and no log. Delete the dead no-op, and make onNotification throw on a duplicate registration the way registerPtyDataPublicationAdmission already does for the admission slot. pty.ackData was the only double-registered method in the tree, so nothing else changes behavior. The existing test asserted only that pty.ackData appeared in the handler map, which stays true when the real handler is shadowed. It now asserts PtyHandler does not own the method.
…truction order (stablyai#15079) PtyHandler registered a no-op pty.ackData handler and SshPtyConsumerSessionAdapter registered the real sourceCredit.acknowledge for the same method. onNotification is a single slot, so only the adapter's survived — and only because relay.ts constructs it second. Reversing those two lines would have made every credit-mode delivery wedge permanently once the 256KB window emptied, with no error and no log. Delete the dead no-op, and make onNotification throw on a duplicate registration the way registerPtyDataPublicationAdmission already does for the admission slot. pty.ackData was the only double-registered method in the tree, so nothing else changes behavior. The existing test asserted only that pty.ackData appeared in the handler map, which stays true when the real handler is shadowed. It now asserts PtyHandler does not own the method.
ELI5
Two different places both told the relay "call me when a
pty.ackDataarrives." Only one of them can win, because that registration is a single slot — and the winner is simply whichever registered last. Today the real handler wins by luck of construction order. The loser is a do-nothing stub. Swap two lines inrelay.tsand terminal output over SSH stops forever after 256 KB, with no error and no log.This deletes the stub and makes registering twice an immediate, loud error instead of a silent coin flip.
What Changed
pty.ackDataregistration inPtyHandler(src/relay/pty-handler.ts). The real handler lives inSshPtyConsumerSessionAdapterand callssourceCredit.acknowledge(...).RelayDispatcher.onNotificationnow throws on a duplicate registration, matching the single-owner guardregisterPtyDataPublicationAdmissionalready has (dispatcher.ts:229-241).dispatcher-notification-ownership.test.ts— adding it todispatcher.test.tspushed that file over the 800-line cap, and amax-linesdisable is not acceptable.Why
onNotificationisnotificationHandlers.set(method, handler)— one slot, last write wins.PtyHandlerregisters a no-op;SshPtyConsumerSessionAdapterregisters the real credit acknowledgement. It works only becauserelay.ts:699-705constructs the adapter second.If that order ever flips,
creditedEndSunever advances,reserveNextSendreturnsnullonce the 256 KB window is exhausted, and every credit-mode delivery wedges permanently. Silent, and hard to trace back.I verified
pty.ackDatais the only double-registered method in the tree, so making the guard strict changes no other behavior.Linked Issue
Fixes STA-4571
Visual Proof
N/A— no UI surface. This is relay handler registration; the observable difference is an error at construction time instead of a silently shadowed handler.Testing
pnpm tcexit 0.oxfmt+oxlintclean.Both guards mutation-verified:
dispatcher-notification-ownership.test.tspty.ackDatare-added toPtyHandlerpty-handler-spawn-admission.test.ts— "registers all expected handlers"The existing test could not have caught this.
pty-handler-spawn-admission.test.ts:82asserted onlyexpect(notifMethods).toContain('pty.ackData')— true whether the handler is real or a shadowed stub. It now assertsPtyHandlerdoes not own the method, which is the actual contract.One transient failure during development (
subprocess.test.ts, "uses configured grace after a detached relay has accepted a socket client") turned out to be a load-induced flake, not this change: it spawns a real subprocess against a 10s timeout while a mobile QA rig was running on the same machine. Confirmed by reverting to a pristine tree (passed), restoring the change (passed), and a clean full-suite run.Review
Agent skill upstream boundary
Notes
pty.ackDatabehaves exactly as it does today; only the dead registration is gone.pty.ackDatawas the sole duplicate across non-test source. Any future duplicate now fails loudly at construction rather than shipping a coin flip.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Author