From 2c0dfd172ff8377b69e75ea00c43172d003d3977 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 10:38:16 +0200 Subject: [PATCH] Keep the process alive while a send waits for a link --- AGENTS.md | 6 +++++- src/link-gate.ts | 36 +++++++++++++++++++++++++++--------- src/send-sms.ts | 2 +- src/session.ts | 10 +++++----- test/session-extras.test.ts | 28 +++++++++++++++++++++++----- test/session.test.ts | 2 +- todo.md | 5 +++-- 7 files changed, 65 insertions(+), 24 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5d66444..ef3396c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -365,7 +365,11 @@ exactly 140. answer to how long one request may wait. It bounds the hold and the answer separately, and the wait for a `maxOutstanding` slot is bounded by nothing, so `responseTimeout` is not a deadline for the call; `SendOptions.signal` with `AbortSignal.timeout()` is, and both the gate and - `pending.wait()` honour it. + `pending.wait()` honour it. The hold's clock starts when the send is issued rather than when it + first finds the gate shut, so one budget covers every hold a single call makes — a send that spent + it queued behind the window is refused rather than held. The hold timer is the one timer here that + is not `unref()`'d: a held request is awaited with the socket already destroyed, so an unref'd one + lets a process whose only remaining work is that send exit without settling it. - **The gate decides whether a link can carry a request, and a bind is what makes it one.** Maintainer's call, 2026-09-01: `attach()` clears `closed` the moment a socket is handed over, one diff --git a/src/link-gate.ts b/src/link-gate.ts index 6e3bc9f..a86e1d0 100644 --- a/src/link-gate.ts +++ b/src/link-gate.ts @@ -1,6 +1,8 @@ +import type { SmppLog } from './log.ts'; import type { VoidResult } from './result.ts'; export type LinkGateOptions = { + log: SmppLog; now?: (() => number) | undefined; /** How long a request may wait for a link. 0 waits for as long as one may still arrive. */ timeout: number; @@ -20,11 +22,9 @@ function over(): Error { return new Error('Session is closed'); } -/** - * Where a request with no link to go out on waits for the next one. The owner reports what became of - * the link; nothing here reads back into the owner to find out. - */ +/** Where a request with no link to go out on waits for the next one. */ export class LinkGate { + private readonly log: SmppLog; private readonly now: () => number; private readonly timeout: number; private readonly waiting = new Set(); @@ -32,6 +32,7 @@ export class LinkGate { private up = true; constructor(options: LinkGateOptions) { + this.log = options.log; this.now = options.now ?? Date.now; this.timeout = options.timeout; } @@ -41,6 +42,11 @@ export class LinkGate { return this.up; } + /** Why the gate will never admit a request, or undefined while one may still get through. */ + refusal(): Error | undefined { + return this.up || this.returning ? undefined : over(); + } + /** When a hold starting now has to give up. 0 never does. */ deadline(): number { return this.timeout > 0 ? this.now() + this.timeout : 0; @@ -50,6 +56,11 @@ export class LinkGate { open(): void { this.up = true; this.returning = false; + + if (this.waiting.size > 0) { + this.log.verbose('linkGate - sending what was held for a link', { held: this.waiting.size }); + } + this.release({}); } @@ -65,7 +76,9 @@ export class LinkGate { wait(deadline: number, signal: AbortSignal | undefined): Promise { if (this.up) return Promise.resolve({}); - if (!this.returning) return Promise.resolve({ err: over() }); + const refused = this.refusal(); + + if (refused) return Promise.resolve({ err: refused }); if (signal?.aborted === true) return Promise.resolve({ err: aborted() }); @@ -77,6 +90,8 @@ export class LinkGate { } private hold(left: number, signal: AbortSignal | undefined): Promise { + this.log.verbose('linkGate - holding a request until a link is back', { timeout: left }); + return new Promise(resolve => { let timer: NodeJS.Timeout | undefined = undefined; const settle = (result: VoidResult): void => { @@ -86,15 +101,18 @@ export class LinkGate { this.waiting.delete(settle); resolve(result); }; + const giveUp = (): void => { + this.log.warn('linkGate - no link came back in time', { timeout: left }); + settle({ err: expired() }); + }; function onAbort(): void { settle({ err: aborted() }); } - if (left > 0) { - timer = setTimeout(() => { settle({ err: expired() }); }, left); - timer.unref(); - } + // Deliberately not unref()'d: a held request is awaited with a destroyed socket and no + // other handle, so an unref'd timer lets the process exit without ever settling it. + if (left > 0) timer = setTimeout(giveUp, left); signal?.addEventListener('abort', onAbort, { once: true }); this.waiting.add(settle); diff --git a/src/send-sms.ts b/src/send-sms.ts index 96689df..d329846 100644 --- a/src/send-sms.ts +++ b/src/send-sms.ts @@ -33,7 +33,7 @@ export type SendSmsResult = { err?: Error; pduObjs: PduObject[]; smsIds: string[]; - /** Segments the link dropped under. The peer may have taken them, so sending again may duplicate. */ + /** Segments that went out unanswered. The peer may have taken them, so sending again may duplicate. */ unanswered: number; }; diff --git a/src/session.ts b/src/session.ts index 8007073..247d62f 100644 --- a/src/session.ts +++ b/src/session.ts @@ -121,7 +121,7 @@ export class Session extends EventEmitter { max: defaults.maxDlrMerges, timeout: defaults.dlrMergeTimeout, }); - this.gate = new LinkGate({ timeout: options.responseTimeout ?? defaults.responseTimeout }); + this.gate = new LinkGate({ log: this.log, timeout: options.responseTimeout ?? defaults.responseTimeout }); this.incoming = new IncomingRequests({ dlrMerger: this.dlrMerger, log: this.log, @@ -174,7 +174,7 @@ export class Session extends EventEmitter { if (refused) return { err: refused }; - // A bind is what makes a link usable, so it cannot wait for one. The door unbind() uses too. + // A bind is what makes a link usable, so it cannot wait for one. if (bindCommands.includes(input.cmdName)) return (await this.attempt(input, options)).result; const deadline = this.gate.deadline(); @@ -189,7 +189,6 @@ export class Session extends EventEmitter { const attempt = await this.attempt(input, options).finally(() => { this.window.release(); }); // Nothing reached the socket, so the next link carries it instead of the caller resending. - // The gate's own answer, or a loop round one that admits everything spins on a dead socket. if (!attempt.retryOnNextLink || this.gate.isUp() || !this.retrying()) return attempt.result; } } @@ -206,7 +205,8 @@ export class Session extends EventEmitter { // Before the gate and the window, or an aborted call waits for what it will never use. if (options.signal?.aborted === true) return abortedBeforeSend(); - return undefined; + // A bind skips the gate below, so the answer it would have given is given here instead. + return bindCommands.includes(input.cmdName) ? this.gate.refusal() : undefined; } /** Read through a method: a drop can land while a send is awaiting. */ @@ -338,9 +338,9 @@ export class Session extends EventEmitter { } this.resetTimers(); + this.gate.open(); this.log.info('session - reconnected'); this.emit('reconnected'); - this.gate.open(); return {}; } diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 96af9b0..17b23c0 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -674,7 +674,7 @@ describe('sends across a reconnect', () => { smpp.on('session', peer => { peer.on('sms', () => undefined); }); - const { session } = await connect(t, smpp, { responseTimeout: 60 }); + const { session } = await connect(t, smpp, { responseTimeout: 200 }); assert.ok(session); @@ -729,7 +729,7 @@ describe('sends across a reconnect', () => { const smpp = await startServer(t); const { session } = await connect(t, smpp, { reconnect: { maxDelay: 10_000, minDelay: 10_000 }, - responseTimeout: 60, + responseTimeout: 200, }); assert.ok(session); @@ -818,7 +818,7 @@ describe('sends across a reconnect', () => { describe('LinkGate', () => { test('refuses a hold whose deadline has already passed', async () => { let now = 0; - const gate = new LinkGate({ now: () => now, timeout: 100 }); + const gate = new LinkGate({ log: silentLog, now: () => now, timeout: 100 }); const deadline = gate.deadline(); gate.shut(true); @@ -829,9 +829,27 @@ describe('LinkGate', () => { assert.match(held.err?.message ?? '', /did not come back in time/); }); + // A held send is awaited with the socket destroyed, so an unref'd timer here lets a process whose + // only remaining work is that send exit without ever settling it, losing the message silently. + test('holds on a timer that keeps the process alive', async () => { + const gate = new LinkGate({ log: silentLog, timeout: 10_000 }); + const timers = (): number => process.getActiveResourcesInfo().filter(name => name === 'Timeout').length; + + gate.shut(true); + + const before = timers(); + const held = gate.wait(gate.deadline(), undefined); + + assert.equal(timers(), before + 1, 'an unref\'d timer is not counted here, which is the point'); + + gate.open(); + + assert.deepEqual(await held, {}); + }); + // addEventListener never fires for a signal that already aborted, so it would wait out the timeout. test('gives up at once on a signal that was already aborted', async () => { - const gate = new LinkGate({ timeout: 100 }); + const gate = new LinkGate({ log: silentLog, timeout: 100 }); gate.shut(true); @@ -1073,7 +1091,7 @@ describe('graceful shutdown', () => { const result = await sent; assert.ok(result.err instanceof Error); - assert.match(result.err.message, /may have accepted/); + assert.match(result.err.message, /may have accepted.*Session closed before a response arrived/); assert.equal(result.unanswered, 1); }); diff --git a/test/session.test.ts b/test/session.test.ts index a8e382b..d7a23d3 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -236,7 +236,7 @@ describe('bind', () => { const sent = await session.send({ cmdName: 'enquire_link' }); assert.ok(sent.err instanceof Error); - assert.match(sent.err.message, /may have accepted/); + assert.match(sent.err.message, /may have accepted.*Session closed before a response arrived/); }); test('reports an unbind the peer left unanswered on a link that stays up', async t => { diff --git a/todo.md b/todo.md index a8370ae..d20940b 100644 --- a/todo.md +++ b/todo.md @@ -137,8 +137,9 @@ session message is a change to every call site. bind already need, with `Session` calling it when a link comes up or goes down instead of spreading `linkDown()` and `retrying()` across both sides. It would also close two smaller things — `LinkGate.deadline()` and `wait(deadline)` are a two-call protocol whose only failure - mode is calling `deadline()` inside the loop, which nothing catches; and `Session.linkDown()` - is read from both sides of the seam. Every one of these is unpublished, so it is a two-way + mode is calling `deadline()` inside the loop, which nothing catches; `Session.linkDown()` is + read from both sides of the seam; and `UnansweredError` sits in `pending-requests.ts`, which + never uses it, for the sole edge that makes `send-sms` import that module at all. Every one of these is unpublished, so it is a two-way door and belongs after 1.0.0. Raised by review, 2026-09-01. - [ ] **Does an intermediate delivery notification deserve to be a `dlr`?** `esm_class` message type `INTERMEDIATE_DELIVERY` (0x20) is classified as a message today, so a peer that reports