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 {