Record exceptions from rejected zone.js promises in span callbacks

[ayaangazali]

In spanWithSettings (packages/logfire-api/src/index.ts) a callback returning a native promise gets recordSpanException on rejection, which records the error and sets the span status to ERROR. The zone.js branch right below it, the one the "clunky detection" comment refers to, only attaches .finally(() => span.end()). So under zone.js, which is to say in Angular apps, a span whose callback rejects ends up looking successful: no exception event and no error status, even though the rejection still propagates to the caller.

This adds the rejection handler to that branch when the value also exposes then, reusing the same recordSpanException call as the native path. The returned chain is ignored on purpose, exactly as the native branch does, so result is still handed back untouched and the caller keeps seeing the rejection. I deliberately kept this scoped to the .finally branch rather than treating every thenable as a promise, because the existing "thenable callback result is returned untouched" test pins the behavior for values that only have then.

The new test builds a non-native object exposing then and finally the way a zone.js promise does, and it fails on current main with zero calls to recordException. Ran the repo preflight and pnpm run check, both green. Patch changeset for logfire included.

quick disclosure: worked through this with Claude Code and reviewed the diff myself. Freshman here, and I do not have a real Angular app to hand, so if the zone.js shape I mocked is off I would be glad to be corrected :)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b68e16bc-f931-4c37-ac7a-6e49a90c1a22

📥 Commits

Reviewing files that changed from the base of the PR and between 4a96d77 and 8acb8e8.

📒 Files selected for processing (2)

• packages/logfire-api/src/index.test.ts
• packages/logfire-api/src/index.ts

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• pydantic/logfire (manual)
• pydantic/pydantic-ai (manual)

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

• packages/logfire-api/src/index.ts

Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.

---

📝 Walkthrough

Walkthrough

The span API now observes rejections from zone.js-style thenables returned by callbacks. It records the exception and sets the span status to error without replacing the returned value. It safely handles throwing then getters and handlers, uses finally when needed, and prevents duplicate span completion. Tests cover these behaviors. A patch changeset documents the update.

Possibly related PRs

• pydantic/logfire-js#236: Modifies thenable detection and handling in packages/logfire-api/src/index.ts, including cached then invocation and throwing getter handling.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: recording exceptions from rejected Zone.js-style promises in span callbacks.
Description check	✅ Passed	The description directly explains the implementation, tests, scope, behavior, and validation for the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

🧹 Nitpick comments (1)

packages/logfire-api/src/index.test.ts (1)

841-868: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert rejection propagation explicitly.

The test verifies return identity, exception recording, status, and span completion, but it never proves that the caller still receives error. Add an assertion that awaiting the returned thenable rejects with error.

Suggested assertion

     expect(result).toBe(zonePromise)
+    await expect(Promise.resolve(result)).rejects.toBe(error)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/logfire-api/src/index.test.ts` around lines 841 - 868, Update the
test around the result returned by span('test', ...) to explicitly await the
returned thenable and assert that it rejects with the original error. Keep the
existing identity, exception recording, status, and completion assertions
unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/logfire-api/src/index.test.ts`:
- Around line 841-868: Update the test around the result returned by
span('test', ...) to explicitly await the returned thenable and assert that it
rejects with the original error. Keep the existing identity, exception
recording, status, and completion assertions unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2b8fa357-b21d-4e1f-9744-7e06bd205bbd

📥 Commits

Reviewing files that changed from the base of the PR and between 7f25681 and 68eefab.

📒 Files selected for processing (3)

• .changeset/thenable-exception-recording.md
• packages/logfire-api/src/index.test.ts
• packages/logfire-api/src/index.ts

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• pydantic/logfire (manual)
• pydantic/pydantic-ai (manual)

[petyosi]

This fixes a real telemetry gap, but I think the fallback should use one settlement subscription before we merge it.

Calling both .then(undefined, onRejected) and .finally(onFinally) creates two derived chains. The ignored promise returned by .finally() preserves the rejection, so it can report an unhandled rejection even when the caller handles the original promise. This behavior already exists in the fallback, but this change should avoid carrying it forward.

Please use a single .then(onFulfilled, onRejected) subscription for promise-like values in this branch:

• end the span in the fulfillment handler;
• record the exception and end the span in the rejection handler;
• continue to return the original result unchanged.

Keeping the existing finally check as the gate is reasonable if we want to avoid consuming arbitrary lazy thenables.

Please also make the regression test representative of the zone.js case. With zone.js patching global Promise, a native async function can return an intrinsic native promise for which result instanceof Promise is false, while finally is present. The test should verify that the original rejection reaches the caller, the exception and error status are recorded, the span ends once, and no extra unhandled rejection is produced.

[ayaangazali]

Good catch on the second chain, and you are right that it was mine to avoid rather than inherit. 4a96d77 switches to a single then(onFulfilled, onRejected) subscription: the fulfillment handler ends the span, the rejection handler records the exception and ends the span, and result is still returned untouched. The finally check stays as the gate so lazy thenables are left alone, and finally is now only called when then is absent.

The test is rebuilt around the shape you described. It replaces the global Promise with a stand-in so an intrinsic promise fails instanceof Promise while keeping then and finally, then asserts the original rejection still reaches the caller, the exception and ERROR status are recorded, the span ends exactly once, and an unhandledRejection listener stays empty.

I checked it fails for both regressions rather than just passing: against main it fails with zero recordException calls, and against the previous two-chain version it fails on the empty-unhandled assertion, which is the exact problem you flagged. Ran the repo preflight and pnpm run check, both green.

[ayaangazali]

Flagging for re-review, since this still shows as changes requested but the change you asked for landed on 2026-08-04.

The two derived chains are gone. Settlement is observed through a single then subscription, so the chain returned by finally is no longer left rejected and unhandled when the caller handles the original, and result is still returned untouched.

Rebased onto current main (817fca1) and re-verified there, since the original run predated the dependency refresh in #212. Build is green, and the mutation check still holds: dropping the rejection handler fails the zone.js test with expected "vi.fn()" to be called with arguments: [ Error: zone-oops ] and nothing else.

[petyosi]

One correctness regression remains in the new settlement path.

packages/logfire-api/src/index.ts:622: The new then lookup and the call below are both unguarded. A callback result with a working finally but a throwing then getter worked on main, which only called finally; this branch now makes span() throw after the callback succeeded and never ends the span. A then() method that throws synchronously has the same result.

Please protect both lookup and subscription, preserve the original result, ensure the span ends, and add regressions for both cases.

[ayaangazali]

You were right and my previous comment answered the wrong review. The 08-04 single-settlement change had landed, but the 08-10 point about the unguarded lookup was still open on the branch, and I read it as already covered. Fixed now in 8acb8e8.

then is read once into a local and only that reference is called, so a getter that returns a function on one read and throws on the next cannot reach the call. The two failure modes get different fallbacks, since they are not equivalent:

• Lookup throws. Nothing has been called yet, so it falls through to finally, which is exactly what main did for such a value. Ending the span immediately here would have replaced the leak with a wrong duration.
• The call throws. then may already have registered the handlers, so adding a finally subscription risks ending the span twice, and doing nothing risks never ending it. It ends the span there, behind a flag so a handler that already fired cannot end it again.

Two regressions, each verified by reverting the source:

• throwing then getter with a working finally: the result is returned unchanged, the span is still open, and it ends once when finally settles. On the previous commit this fails with then getter boom.
• then that invokes its fulfillment handler and then throws: the span ends exactly once. Dropping the flag alone fails this with two calls to end.

I left the finally call itself unwrapped. It is already known to be a function because it gates the branch, and a finally that throws when called behaved the same way on main, so guarding it would be handling a state this change does not introduce.

Green on 79bcac0. Happy to close this if the zone.js path is not worth carrying.

Built this with Claude Code's help and reviewed the diff myself.

[ayaangazali]

Rebased onto 8effa79 and simplified against what just landed.

#236 merged getThenMethod, which does exactly what this branch was hand-rolling in the zone.js path: guard the then read, and hand back the method so the call site cannot re-read it. This now uses that helper instead, which drops 18 lines and leaves one implementation of the rule in the file rather than two. ThenMethod gained an optional onRejected parameter, since this branch needs both handlers and that is the real shape of then. The resultWithFinally cast narrowed to just finally, which is all it gates on now.

Behaviour is unchanged from your last review. Both regressions still bite: replacing getThenMethod with a raw property read fails the throwing-getter test, and dropping the ended flag fails the ends-once test. 495 tests green on the new base, preflight and pnpm run check clean.

[petyosi]

Merged locally and landed on main as eca8805 — thanks for the thorough follow-through across the review rounds; both sets of requested changes were verified as addressed. Two follow-ups landed alongside it: 03b4d62 moves the test's unhandledRejection cleanup into a finally block, and cd89805 records the failure on the span when then throws before any settlement handler runs, so that path no longer closes with an OK status. (Squash-merged outside GitHub, so closing manually.)