fix: fire sip:refer actionHook when the far end BYEs before any NOTIFY (#1258) (#1580)

* fix: fire sip:refer actionHook when the far end BYEs before any NOTIFY When a REFER is accepted with 202 the verb waits for a NOTIFY carrying the final status of the referred call, and only fired the actionHook from the 15s timeout callback. If the far end sends BYE straight after the 202 the call session kills the task, awaitTaskDone() resolves, and exec returned without ever performing the action. Perform the action once from exec on every exit path - before the session tears the requestor down - and de-duplicate it against the final-NOTIFY path. Fix #1258 Generated by Ora Studio Vibe coded by ousamabenyounes Co-Authored-By: Ora Agent <noreply@oratelecom.net> * fix: keep final_referred_call_status when a BYE races the final NOTIFY (#1580) A final NOTIFY (status >= 200) whose eventHook round trip is still in flight when the far end BYEs lost final_referred_call_status on the actionHook: kill() woke exec(), which memoised the action via _performReferAction({refer_status}) without the final status, so the NOTIFY path's later call carrying it was deduped away. On main the app received final_referred_call_status here. Record the final status synchronously in _handleNotify before the eventHook await, and include it from exec() so the memoised action carries it regardless of which exit path wins the race. Extract the 200 threshold into a named constant while touching those lines. Addresses davehorton's review on #1580. Generated by Ora Studio Vibe coded by ousamabenyounes Co-Authored-By: Ora Agent <noreply@oratelecom.net> --------- Co-authored-by: Ora Agent <noreply@oratelecom.net> Co-authored-by: Ben Younes <2910651+ousamabenyounes@users.noreply.github.com>
2026-10-03 17:54:12 +00:00 · 2026-09-09 15:14:51 +01:00
parent a4e7afac83
commit 83db903bc6
2 changed files with 278 additions and 15 deletions
@@ -1,7 +1,16 @@
 const Task = require('./task');
-const {TaskName, TaskPreconditions} = require('../utils/constants');
+const {TaskName, TaskPreconditions, KillReason} = require('../utils/constants');
 const {parseUri} = require('drachtio-srf');

+/* how long we wait for a NOTIFY carrying the final status of the referred call */
+const NOTIFY_TIMEOUT_MS = 15000;
+
+/* SIP status returned by a far end that accepted the REFER */
+const REFER_ACCEPTED = 202;
+
+/* lowest SIP status that counts as the final response of the referred call */
+const SIP_FINAL_RESPONSE_MIN = 200;
+
 /**
 * sends a sip REFER to transfer the existing call
 */
@@ -15,6 +24,8 @@ class TaskSipRefer extends Task {
    this.referredByDisplayName = this.data.referredByDisplayName;
    this.headers = this.data.headers || {};
    this.eventHook = this.data.eventHook;
+    this._actionPromise = null;
+    this._finalReferredCallStatus = null;
  }

  get name() { return TaskName.SipRefer; }
@@ -47,30 +58,36 @@ class TaskSipRefer extends Task {
      this.logger.info(`TaskSipRefer:exec - received ${this.referStatus} to REFER`);

      /* if we fail, fall through to next verb.  If success, we should get BYE from far end */
-      if (this.referStatus === 202) {
+      if (this.referStatus === REFER_ACCEPTED) {
        this._notifyTimer = setTimeout(() => {
-          this.logger.info('TaskSipRefer:exec - no NOTIFY received in 15 secs, exiting');
-          this.performAction({refer_status: this.referStatus})
-            .catch((err) => this.logger.error(err, 'TaskSipRefer:exec - error performing action'));
+          this.logger.info(`TaskSipRefer:exec - no NOTIFY received in ${NOTIFY_TIMEOUT_MS} ms, exiting`);
          this.notifyTaskDone();
-        }, 15000);
+        }, NOTIFY_TIMEOUT_MS);
        await this.awaitTaskDone();
        if (this._notifyTimer) {
          clearTimeout(this._notifyTimer);
          this._notifyTimer = null;
        }
      }
-      else {
-        await this.performAction({refer_status: this.referStatus});
-      }
+      /* the far end may send BYE before any NOTIFY arrives, which kills this task while we are
+         awaiting above; performing the action here means the actionHook runs on every exit path -
+         and while the requestor is still up - rather than only when the NOTIFY timer expires.
+         When a final NOTIFY was already parsed (even if its eventHook round trip had not returned
+         before the BYE woke us), _finalReferredCallStatus carries the status so the app still gets
+         final_referred_call_status here rather than losing it to the memoised action */
+      await this._performReferAction({
+        refer_status: this.referStatus,
+        ...(this._finalReferredCallStatus && {final_referred_call_status: this._finalReferredCallStatus})
+      });
    } catch (err) {
      this.logger.info({err}, 'TaskSipRefer:exec - error sending REFER');
    }
    this.referSpan?.end();
  }

-  async kill(cs) {
+  async kill(cs, reason) {
    super.kill(cs);
+    this.killReason = reason || KillReason.Hangup;
    const {dlg} = cs;
    dlg.off('notify', this.notifyHandler);
    this.notifyTaskDone();
@@ -87,24 +104,40 @@ class TaskSipRefer extends Task {
      if (arr) {
        const status = typeof arr[1] === 'string' ? parseInt(arr[1], 10) : arr[1];
        this.logger.debug(`TaskSipRefer:_handleNotify: call got status ${status}`);
+        /* record the final status synchronously, before the eventHook round trip below can be
+           interrupted by a BYE, so exec() can include it even if it wins the race to the action */
+        if (status >= SIP_FINAL_RESPONSE_MIN) this._finalReferredCallStatus = status;
        if (this.eventHook) {
          const b3 = this.getTracingPropagation();
          const httpHeaders = b3 && {b3};
          await cs.requestor.request('verb:hook', this.eventHook,
            {event: 'transfer-status', call_status: status}, httpHeaders);
        }
-        if (status >= 200) {
+        if (status >= SIP_FINAL_RESPONSE_MIN) {
          this.referSpan.setAttributes({'refer.finalNotify': status});
-          await this.performAction({refer_status: 202, final_referred_call_status: status})
-            .catch((err) => {
-              this.logger.error(err, 'TaskSipRefer:exec - error performing action finalNotify');
-            });
+          await this._performReferAction({refer_status: REFER_ACCEPTED, final_referred_call_status: status});
          this.notifyTaskDone();
        }
      }
    }
  }

+  /**
+   * fire the verb's actionHook exactly once, whichever exit path completes the task:
+   * a final NOTIFY, the NOTIFY timeout, or the task being killed by an early BYE
+   */
+  _performReferAction(results) {
+    if (!this._actionPromise) {
+      /* when the app replaced this verb with new commands, still notify it, but do not let the
+         hook response replace the application a second time (same contract as the dial verb) */
+      this._actionPromise = this.performAction(results, this.killReason !== KillReason.Replaced)
+        .catch((err) => this.logger.error({err}, 'TaskSipRefer:_performReferAction - error performing action'));
+    }
+    /* callers await the shared promise, so exec() cannot return - and let the session close the
+       requestor - while an actionHook started by another exit path is still in flight */
+    return this._actionPromise;
+  }
+
  _normalizeReferHeaders(cs, dlg) {
    let {referTo, referredBy, referredByDisplayName} = this;

@@ -0,0 +1,230 @@
+const test = require('node:test');
+const assert = require('node:assert');
+const Emitter = require('events');
+const {context} = require('@opentelemetry/api');
+const {KillReason} = require('../../lib/utils/constants');
+const proxyquire = require('proxyquire').noCallThru();
+
+const ACTION_HOOK = '/refer-action';
+const EVENT_HOOK = '/refer-event';
+const REFER_TO = '+15551234567';
+const REFER_ACCEPTED = 202;
+const REFER_DECLINED = 603;
+const FINAL_NOTIFY_STATUS = 200;
+const NOTIFY_TIMEOUT_MS = 15000;
+const FAKE_TRACE_ID = '0'.repeat(32);
+const FAKE_SPAN_ID = '0'.repeat(16);
+
+/* Task#startSpan pulls the tracer off the app module singleton; stub it so the verb can be
+   exercised without booting the feature server */
+const fakeSpan = {
+  setAttributes: () => {},
+  end: () => {},
+  spanContext: () => ({traceId: FAKE_TRACE_ID, spanId: FAKE_SPAN_ID})
+};
+const TaskSipRefer = proxyquire('../../lib/tasks/sip_refer', {
+  '../..': {
+    srf: {locals: {otel: {tracer: {startSpan: () => fakeSpan}}}},
+    '@global': true,
+    '@noCallThru': true
+  }
+});
+
+const noop = () => {};
+const logger = {info: noop, debug: noop, error: noop};
+
+/* minimal CallSession stand-in: a dialog that answers the REFER, and a requestor that
+   records every hook it is asked to fire */
+const makeCallSession = (referStatus) => {
+  const hookCalls = [];
+  const dlg = new Emitter();
+  dlg.local = {uri: 'sip:jambonz@example.com'};
+  dlg.remote = {uri: 'sip:carrier@10.10.10.10'};
+  dlg.request = async() => ({status: referStatus});
+
+  return {
+    hookCalls,
+    dlg,
+    replacedWith: [],
+    replaceApplication(tasks) {
+      this.replacedWith.push(tasks);
+    },
+    req: {callingNumber: '+15550000000', callingName: 'jambonz'},
+    callInfo: {toJSON: () => ({call_sid: 'call-sid-under-test'})},
+    requestor: {
+      request: async(type, hook, params) => {
+        hookCalls.push({type, hook, params});
+      }
+    }
+  };
+};
+
+const makeTask = (opts = {}) => {
+  const task = new TaskSipRefer(logger, {referTo: REFER_TO, actionHook: ACTION_HOOK, ...opts});
+  task.ctx = context.active();
+  return task;
+};
+
+/* let the REFER request/response round trip settle before driving the next event */
+const settle = () => new Promise((resolve) => setImmediate(resolve));
+
+const makeNotify = (status) => {
+  const req = {
+    get: (name) => ('Content-Type' === name ? 'message/sipfrag;version=2.0' : undefined),
+    body: `SIP/2.0 ${status} OK`
+  };
+  return {req, res: {send: () => {}}};
+};
+
+test('sip:refer actionHook fires when the far end sends BYE before any NOTIFY', async() => {
+  const cs = makeCallSession(REFER_ACCEPTED);
+  const task = makeTask();
+  const execPromise = task.exec(cs);
+  await settle();
+
+  /* the BYE tears the call session down, which kills the running task */
+  task.kill(cs);
+  await execPromise;
+
+  assert.strictEqual(cs.hookCalls.length, 1, 'actionHook should have been called exactly once');
+  assert.strictEqual(cs.hookCalls[0].hook, ACTION_HOOK);
+  assert.strictEqual(cs.hookCalls[0].params.refer_status, REFER_ACCEPTED);
+});
+
+test('sip:refer actionHook fires once, with the final status, when a NOTIFY arrives', async() => {
+  const cs = makeCallSession(REFER_ACCEPTED);
+  const task = makeTask();
+  const execPromise = task.exec(cs);
+  await settle();
+
+  const {req, res} = makeNotify(FINAL_NOTIFY_STATUS);
+  cs.dlg.emit('notify', req, res);
+  await execPromise;
+
+  assert.strictEqual(cs.hookCalls.length, 1, 'actionHook should have been called exactly once');
+  assert.strictEqual(cs.hookCalls[0].params.refer_status, REFER_ACCEPTED);
+  assert.strictEqual(cs.hookCalls[0].params.final_referred_call_status, FINAL_NOTIFY_STATUS);
+});
+
+test('sip:refer actionHook fires when the far end rejects the REFER', async() => {
+  const cs = makeCallSession(REFER_DECLINED);
+  const task = makeTask();
+
+  await task.exec(cs);
+
+  assert.strictEqual(cs.hookCalls.length, 1, 'actionHook should have been called exactly once');
+  assert.strictEqual(cs.hookCalls[0].params.refer_status, REFER_DECLINED);
+});
+
+test('a failing actionHook is logged, not thrown out of the verb', async() => {
+  const cs = makeCallSession(REFER_ACCEPTED);
+  cs.requestor.request = async() => {
+    throw new Error('actionHook unreachable');
+  };
+  const task = makeTask();
+  const execPromise = task.exec(cs);
+  await settle();
+
+  task.kill(cs);
+  await assert.doesNotReject(execPromise);
+});
+
+test('sip:refer actionHook fires when no NOTIFY arrives before the timeout', async(t) => {
+  t.mock.timers.enable({apis: ['setTimeout']});
+  const cs = makeCallSession(REFER_ACCEPTED);
+  const task = makeTask();
+  const execPromise = task.exec(cs);
+  await settle();
+
+  t.mock.timers.tick(NOTIFY_TIMEOUT_MS);
+  await execPromise;
+
+  assert.strictEqual(cs.hookCalls.length, 1, 'actionHook should have been called exactly once');
+  assert.strictEqual(cs.hookCalls[0].params.refer_status, REFER_ACCEPTED);
+});
+
+test('exec waits for an in-flight actionHook when a BYE races the final NOTIFY', async() => {
+  const cs = makeCallSession(REFER_ACCEPTED);
+  let releaseHook;
+  const hookGate = new Promise((resolve) => {
+    releaseHook = resolve;
+  });
+  const recordHook = cs.requestor.request;
+  cs.requestor.request = async(...args) => {
+    await hookGate;
+    return recordHook(...args);
+  };
+
+  const task = makeTask();
+  const execPromise = task.exec(cs);
+  await settle();
+
+  const {req, res} = makeNotify(FINAL_NOTIFY_STATUS);
+  cs.dlg.emit('notify', req, res);
+  await settle();
+
+  /* the BYE lands while the actionHook request started by the NOTIFY is still in flight */
+  task.kill(cs);
+  let execResolved = false;
+  execPromise.then(() => {
+    execResolved = true;
+  });
+  await settle();
+  assert.strictEqual(execResolved, false, 'exec must not resolve while the actionHook is in flight');
+
+  releaseHook();
+  await execPromise;
+  assert.strictEqual(cs.hookCalls.length, 1, 'actionHook should have been called exactly once');
+});
+
+test('the final NOTIFY status still reaches the actionHook when a BYE races the eventHook round trip', async() => {
+  const cs = makeCallSession(REFER_ACCEPTED);
+  let releaseEvent;
+  const eventGate = new Promise((resolve) => {
+    releaseEvent = resolve;
+  });
+  const recordHook = cs.requestor.request;
+  cs.requestor.request = async(type, hook, params, httpHeaders) => {
+    /* hold the transfer-status eventHook mid round trip so the BYE can race it */
+    if (EVENT_HOOK === hook) await eventGate;
+    return recordHook(type, hook, params, httpHeaders);
+  };
+
+  const task = makeTask({eventHook: EVENT_HOOK});
+  const execPromise = task.exec(cs);
+  await settle();
+
+  /* final NOTIFY arrives; _handleNotify parks awaiting the eventHook round trip */
+  const {req, res} = makeNotify(FINAL_NOTIFY_STATUS);
+  cs.dlg.emit('notify', req, res);
+  await settle();
+
+  /* the BYE lands while that eventHook request is still in flight, killing the task */
+  task.kill(cs);
+  await settle();
+
+  releaseEvent();
+  await execPromise;
+
+  const action = cs.hookCalls.find((c) => ACTION_HOOK === c.hook);
+  assert.ok(action, 'actionHook should have been called');
+  assert.strictEqual(action.params.refer_status, REFER_ACCEPTED);
+  assert.strictEqual(action.params.final_referred_call_status, FINAL_NOTIFY_STATUS,
+    'final_referred_call_status must survive a BYE racing the final NOTIFY eventHook');
+});
+
+test('an actionHook response replaces the application only when the verb was not itself replaced', async() => {
+  for (const [killReason, expectedReplacements] of [[undefined, 1], [KillReason.Replaced, 0]]) {
+    const cs = makeCallSession(REFER_ACCEPTED);
+    cs.requestor.request = async() => [{verb: 'hangup'}];
+    const task = makeTask();
+    const execPromise = task.exec(cs);
+    await settle();
+
+    task.kill(cs, killReason);
+    await execPromise;
+
+    assert.strictEqual(cs.replacedWith.length, expectedReplacements,
+      `killReason=${killReason} should produce ${expectedReplacements} application replacement(s)`);
+  }
+});