From fe95698b95e7687857d2421549366b8771c71e36 Mon Sep 17 00:00:00 2001
From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
Date: Tue, 18 Aug 2026 12:37:39 -0700
Subject: [PATCH] fix(relay): let one owner hold pty.ackData instead of relying
 on construction order (#15079)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 .../dispatcher-notification-ownership.test.ts  | 18 ++++++++++++++++++
 src/relay/dispatcher.ts                        |  7 +++++++
 src/relay/pty-handler-spawn-admission.test.ts  |  5 ++++-
 src/relay/pty-handler.ts                       |  3 ---
 4 files changed, 29 insertions(+), 4 deletions(-)
 create mode 100644 src/relay/dispatcher-notification-ownership.test.ts

diff --git a/src/relay/dispatcher-notification-ownership.test.ts b/src/relay/dispatcher-notification-ownership.test.ts
new file mode 100644
index 00000000000..e94fdcede62
--- /dev/null
+++ b/src/relay/dispatcher-notification-ownership.test.ts
@@ -0,0 +1,18 @@
+import { describe, expect, it, vi } from 'vitest'
+import { RelayDispatcher } from './dispatcher'
+
+describe('RelayDispatcher notification ownership', () => {
+  it('refuses a second handler for a method instead of silently shadowing the first', () => {
+    const dispatcher = new RelayDispatcher(() => {})
+    const owner = vi.fn()
+
+    dispatcher.onNotification('pty.ackData', owner)
+
+    // Why: the slot holds one handler, so a second registration used to win purely by
+    // construction order — that is how a no-op ack handler nearly disabled credit acks.
+    expect(() => dispatcher.onNotification('pty.ackData', vi.fn())).toThrow(/already registered/)
+
+    // Why: the constructor arms a keepalive interval that would outlive the test.
+    dispatcher.dispose()
+  })
+})
diff --git a/src/relay/dispatcher.ts b/src/relay/dispatcher.ts
index 1898e9fbf4e..00e1836d9d7 100644
--- a/src/relay/dispatcher.ts
+++ b/src/relay/dispatcher.ts
@@ -212,7 +212,14 @@ export class RelayDispatcher {
     this.requestHandlers.set(method, handler)
   }
 
+  // Why it throws: this is a single slot, so a second registration silently shadows the
+  // first and which one survives depends only on construction order. `pty.ackData` shipped
+  // that way — a no-op handler was saved from disabling credit acks purely by the adapter
+  // being constructed second (STA-4571). Fail loudly instead of encoding that ordering.
   onNotification(method: string, handler: NotificationHandler): void {
+    if (this.notificationHandlers.has(method)) {
+      throw new Error(`Notification handler for ${method} is already registered`)
+    }
     this.notificationHandlers.set(method, handler)
   }
 
diff --git a/src/relay/pty-handler-spawn-admission.test.ts b/src/relay/pty-handler-spawn-admission.test.ts
index c5938d91f57..aae35a2e166 100644
--- a/src/relay/pty-handler-spawn-admission.test.ts
+++ b/src/relay/pty-handler-spawn-admission.test.ts
@@ -79,7 +79,10 @@ describe('PtyHandler', () => {
     const notifMethods = Array.from(dispatcher._notificationHandlers.keys())
     expect(notifMethods).toContain('pty.data')
     expect(notifMethods).toContain('pty.resize')
-    expect(notifMethods).toContain('pty.ackData')
+    // Why not `toContain('pty.ackData')`: PtyHandler must NOT own that method. It used to
+    // register a no-op here, which survived only because the consumer session adapter was
+    // constructed later and overwrote it (STA-4571).
+    expect(notifMethods).not.toContain('pty.ackData')
   })
 
   it('rejects strict process inspection for a missing relay PTY', async () => {
diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts
index c7dfb0a533d..3f82038261d 100644
--- a/src/relay/pty-handler.ts
+++ b/src/relay/pty-handler.ts
@@ -903,9 +903,6 @@ export class PtyHandler {
 
     this.dispatcher.onNotification('pty.data', (p) => this.writeData(p))
     this.dispatcher.onNotification('pty.resize', (p) => this.resize(p))
-    this.dispatcher.onNotification('pty.ackData', (_p) => {
-      /* flow control ack -- not yet enforced */
-    })
   }
 
   private isLikelyInteractiveRedraw(data: string): boolean {