fix(okhttp): keep the wrapped EventListener per Call

[markushi]

📜 Description

SentryOkHttpEventListener held the wrapped EventListener in a single mutable field that
callStart overwrote for each call. It is now kept in a per-Call map, the same pattern the class
already uses for eventMap. No public API change.

💡 Motivation and Context

OkHttp uses one listener instance for all calls, thus concurrent calls were all delegated to the
listener made for the call that started last. This breaks the EventListener.Factory contract and
loses the terminal callEnd/callFailed of every overlapping call.

• resolves: SentryOkHttpEventListener breaks EventListener.Factory contract #5967
• resolves: JAVA-695

💚 How did you test it?

Added unit tests.

📝 Checklist

• I added GH Issue ID & Linear ID
• I added tests to verify the changes.
• No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
• I updated the docs if needed.
• I updated the wizard if needed.
• Review from the native team if needed.
• No breaking change or entry added to the changelog.
• No breaking change for hybrid SDKs or communicated to hybrid SDKs.
• Public API changes reviewed by another Mobile SDK team member or implemented according to the develop docs spec.

🔮 Next steps

[linear-code]

JAVA-695

[sentry]

📲 Install Builds

Android

🔗 App Name	App ID	Version	Configuration
SDK Size	io.sentry.tests.size	8.54.0 (1)	release

⚙️ sentry-android Build Distribution Settings

[0xadam-brown]

Thanks for this 💯 !

One comment worth addressing; otherwise looking good.

[0xadam-brown]

Excellent! One tweak more to satisfy the EventListener.Factory contract, and I think we'll be there 🥇

[0xadam-brown]

We're close! (and thanks for the great updates)

We still need to preserve the contract of EventListener.Factory that ensures only one EventListener instance is produced per Call lifecycle. Ie, the listener returned by the factory for a given call needs to be the listener that captures i) all of that call's lifecycle and ii) no other call's lifecycle.

We've fixed (ii), but we're still violating (i) in the case of cancelation because we're creating an extra listener for early and late cancel() invocations.

Possible solution

Thoughts about using a weak per-call map for the wrapped listener instead? Something like a WeakHashMap<Call, EventListener> guarded by synchronized, with a getOrCreateOriginalEventListener(call) helper used by both callStart and canceled().

That'd^^ let us avoid removing entries on callEnd / callFailed, and completed calls would be gc'd as soon as the Call instance is unreachable.

[markushi]

Fixed in 728129d. You were right that creating an extra listener was the wrong trade — and a WeakHashMap turned out not to be usable here, for a reason our own code demonstrates.

Why not weak keys. WeakHashMap holds values by ordinary strong references, and its javadoc warns that a value which strongly refers to its own key prevents the key from being discarded. That is exactly our shape: a factory is handed the Call, so listeners that keep it are the normal case (our own RecordingListener(val ownCall: Call) does it). Worse, the sibling eventMap already has this cycle inside the SDK: SentryOkHttpEvent holds a Response, Response.exchange is an Exchange, and Exchange.call is the RealCall. So a weak map would stop collecting as soon as a response is recorded. It is also unsynchronized, and getTable() calls expungeStaleEntries(), so even reads would need the monitor on a path OkHttp explicitly allows to run concurrently.

What we do instead. OkHttp stores the listener on the call itself (RealCall:73, client.eventListenerFactory.create(this)), so its lifetime is the Call object's lifetime. Ours is the callStart()..callEnd() window. canceled() is the one event that escapes that window, so it is the one that needed handling — and Call.isExecuted() tells the two edges apart without any storage of our own:

• not executed — the cancel precedes callStart(). computeIfAbsent creates and binds the listener, and callStart() then reuses it, so one listener sees the whole lifecycle. It is self-cleaning: getResponseWithInterceptorChain does if (canceled) throw IOException("Canceled"), so a pre-canceled call that is later executed still runs callStart → callFailed and the entry is removed.
• executed, entry present — in flight, delegated to the bound listener.
• executed, entry absent — the terminal event already passed. Ignored. Call.cancel() is documented as "Requests that are already complete cannot be canceled", so there is nothing to report, and fabricating a second listener would both break the contract and leak the entry.

A stored "terminal" flag would have worked too, but it needs a per-Call marker that no later event can ever remove — the same unbounded growth, just with a smaller value behind a Call key. isExecuted() is that flag, maintained by OkHttp, for free.

Two supporting changes: getOrCreateEventListener uses ConcurrentHashMap.computeIfAbsent rather than a get-then-put, so a cancel racing callStart() cannot make the factory produce two listeners for one call; and the constructors that wrap a single EventListener now keep it in a field. That instance is shared across calls by definition — it is precisely what OkHttp's own EventListener.asFactory() does — so it exists independently of the window and receives every cancel, which restores the pre-PR behaviour you flagged with no leak and no contract question.

Residual gap, stated plainly: a call that is canceled before it starts and then never executed keeps its map entry, because no terminal event ever arrives. That is far narrower than the late-cancel leak it replaces, and bounding it would need the weak keys that do not work here.

[0xadam-brown]

Thanks for the new tests 💯

Bonus points if our cancellation tests can assert the stronger factory-contract invariant 👍

(Right now cancel before callStart is delegated and cancel after callEnd is delegated prove that some listener receives canceled(), rather than that a single listener receives all lifecycle callbacks.)

[markushi]

Done in 728129d. The cancellation tests now assert the single-listener invariant rather than "some listener got it".

The blocker was that these tests drive the listener by hand with client.newCall(...), so Call.isExecuted() is always false and the post-terminal branch was unreachable. Added a Fixture.mockCall(path, isExecuted) helper so each test states the call state it is exercising.

• cancel before callStart binds the listener that callStart then reuses — asserts fixture.listeners has size 1 and that the one listener receives canceled, callStart, dnsStart, callEnd in order.
• cancel after the terminal event is ignored — size 1, receiving exactly callStart, callEnd; no second listener is created and no stray canceled is delivered.
• cancel after a failed call is ignored — same, via callFailed.
• cancel during a call is delegated to the listener of that call — callStart, canceled, callFailed all on one listener.
• a single wrapped listener receives cancels outside of the call window — covers the fixed-instance constructors, which keep delegating cancels at any time.

The hasSize(1) assertions are the ones carrying the factory contract: they fail if we ever invoke the factory more than once for a call.

[sentry]

Bug: The canceled() method incorrectly uses call.isExecuted() to detect post-terminal calls, causing cancel events to be dropped if they occur after enqueue() but before callStart().
_{Severity: MEDIUM}

Suggested Fix

The logic in the canceled() method should be adjusted to not rely solely on call.isExecuted() to determine if a call is post-terminal. The check if (call.isExecuted()) should be removed or modified to correctly handle the state where a call has been enqueued but callStart has not yet been invoked, ensuring the listener is created and the canceled event is properly delegated.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpEventListener.kt#L412

Potential issue: In `SentryOkHttpEventListener`, when using a factory-based listener,
there is a race condition in the `canceled()` method. The logic checks
`call.isExecuted()` to determine if a call is in a terminal state. However, OkHttp's
`isExecuted()` returns `true` as soon as a call is enqueued, which can occur before the
`callStart()` event is fired. If a call is canceled during this window (after
`enqueue()` but before `callStart()`), the `canceled()` method will incorrectly assume
the call is complete and will not delegate the event. This results in the `cancel` event
being lost, potentially leading to inaccurate transaction data where the associated span
is not marked as canceled.

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

^{Reviewed by Cursor Bugbot for commit 728129d. Configure here.}

[cursor]

Cancel before start leaks listeners

Medium Severity

canceled() now inserts a factory-created listener into originalEventListenerMap when isExecuted() is false. That entry is only removed in callEnd or callFailed, so a Call that is canceled and never started stays in the map for the life of the SentryOkHttpEventListener.

Additional Locations (1)

• sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpEventListener.kt#L436-L440

 

^{Reviewed by Cursor Bugbot for commit 728129d. Configure here.}

[cursor]

Start race drops canceled callback

Medium Severity

Call.isExecuted() becomes true at the start of enqueue/execute, before callStart inserts the listener. A concurrent canceled() can then see an empty map and a true isExecuted() flag and drop the event, so the listener created moments later never receives canceled().

 

^{Reviewed by Cursor Bugbot for commit 728129d. Configure here.}