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 no longer cause 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.
  • Added validation that callback-thrown errors propagate correctly without duplicate callback execution.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 4e2c0208-a447-4350-a35c-94a8ab82b5a2

📥 Commits

Reviewing files that changed from the base of the PR and between 0a02686 and 437a42a.

📒 Files selected for processing (1)

• src/types.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/types.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 437a4

Callback exceptions now propagate after a single callback invocation, preventing duplicate completion handling. 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 primary change: preventing a callback from being invoked twice after an unhandled exception.
Linked Issues check	✅ Passed	The changes satisfy issue #527. The callback now runs outside the try block, callback exceptions propagate normally, operation errors invoke the callback once, and the regression test covers the publi…
Out of Scope Changes check	✅ Passed	The source change and regression test directly support issue #527. No unrelated code changes are identified.
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?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.35%. Comparing base (f5c4d22) to head (71ccb63).
⚠️ Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #528      +/-   ##
==========================================
+ Coverage   75.95%   77.35%   +1.39%     
==========================================
  Files           9        9              
  Lines        1048     1073      +25     
  Branches      273      275       +2     
==========================================
+ Hits          796      830      +34     
+ Misses        144      137       -7     
+ Partials      108      106       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[markstos]

I agree this is a bug and agree with the fix. Test coverage also looks good.

The added comments are unnecessary, though.

In most cases, comments that describe bugs that the code no longer has are not useful. Git history is useful to reviewing prior states of the code and history of code changes.