Fix double callback invoke on unhandled exception

[adamjmcgrath]

Reported and diagnosed by @adamjmcgrath in #527: when a caller's callback throws, computeSignature(xml, cb) invokes it a second time, passing the callback's own error back as err.

Callback invoked false true
Callback invoked true false     <- should not happen
Error: Error Thrown

Cause

createOptionalCallbackFunction invoked the callback inside the try, so an exception thrown by the callback landed in the catch and was handed straight back to it:

try {
  const result = syncVersion(...args);
  possibleCallback(null, result);   // throws here...
} catch (err) {
  possibleCallback(err ...);        // ...and is caught here, invoking the callback again
}

Present since the helper was introduced in #343, so every release from v4.0.0 onward.

Fix

Narrow the try to cover only syncVersion. Once the callback is outside it, its exceptions cannot re-enter the catch:

let result: T;
try {
  result = syncVersion(...args);
} catch (err) {
  possibleCallback(err instanceof Error ? err : new Error("Unknown error"));
  return;
}
possibleCallback(null, result);

The return is enforced by the compiler rather than by discipline — without it result is not definitely assigned and tsc --strict rejects the file, so the error path cannot regress into a double call.

Why not process.nextTick

The original version of this PR deferred the success callback with process.nextTick. That also stops the double invocation, but it changes when the callback runs, and computeSignature(xml, cb) was effectively synchronous. Measured on the same input, the one line differing:

	callback invocations	getSignedXml() immediately after
before	2	780
process.nextTick	1	0
this fix	1	780

An existing caller reading getSignedXml() after computeSignature(xml, cb) would get an empty string rather than an exception — a silent breaking change to a semver-bound public API, in a library where the failure surfaces downstream as an unsigned document. Narrowing the try fixes the reported bug with byte-identical timing instead.

De-Zalgoing these callbacks is still worth doing, but as a deliberate major rather than inside a bug fix. Tracked for 7.0 in #546, alongside #545.

Tests

One regression test driving the public computeSignature(xml, callback) path from the issue. It was watched failing first against the unfixed helper, for the reported reason:

AssertionError: expected [ null, 'Error Thrown' ] to deeply equal [ null ]

It is fully synchronous, so it no longer removes and restores the process uncaughtException listeners — the earlier version leaked mocha's handler for the rest of the run whenever it failed.

npm run build, npm test (219 passing) and npm run lint all clean.

fixes #527

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Error-first callbacks now receive successful results synchronously.
  • Callback errors are handled without causing the callback to be invoked a second time.
  • Error callbacks return immediately after being called, providing more predictable completion behavior.

• Tests

  • Added coverage confirming synchronous XML signature computation invokes the callback exactly once with no error.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 8e510a42-f236-4369-9e6b-6fea04bfb3df

📥 Commits

Reviewing files that changed from the base of the PR and between 60f665b and b97e5da.

📒 Files selected for processing (2)

• src/types.ts
• test/types-tests.spec.ts

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

---

📝 Walkthrough

Walkthrough

This change prevents createOptionalCallbackFunction from invoking a throwing callback twice. It adds a SignedXml.computeSignature regression test that verifies one callback invocation with a null error before the callback exception propagates.

Changes

Callback exception handling

Layer / File(s)	Summary
Callback wrapper and regression coverage src/types.ts, test/types-tests.spec.ts	The callback wrapper invokes the success callback outside the catch path and returns after error handling. The regression test verifies that computeSignature invokes the callback once with null before propagating the callback exception.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to b97e5

This change prevents a throwing computeSignature callback from being invoked a second time while preserving synchronous error propagation. The covered behavior is ready to merge.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: preventing a callback from being invoked twice when the callback throws.
Linked Issues check	✅ Passed	The changes satisfy issue #527. The callback now runs outside the try block, callback errors propagate normally, operation errors invoke the callback once, and the regression test covers the public co…
Out of Scope Changes check	✅ Passed	The changed implementation and regression test directly support issue #527. No unrelated code changes are shown.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cjbarth]

@adamjmcgrath , I've made some changes. What do you think?