Skip to content

Conversation

@sumeruchat
Copy link
Contributor

@sumeruchat sumeruchat commented Nov 18, 2025

Ticket: https://iterable.atlassian.net/browse/MOB-11490

Issue

GreenFi, 2ULaundry, and Hello Heart reported that iOS deep links were failing intermittently. Users would tap SMS/email links but the app wouldn't open to the correct screen. This was blocking all iOS deep linking for these clients.

What Was Happening

When a user taps an Iterable deep link (like https://links.greenfi.com/a/N2Nbu), the SDK needs to:

  1. Make a request to the shortened link
  2. Get the 303 redirect response with attribution data
  3. Extract the final destination URL and campaign info
  4. Open the app to that URL

The problem was in step 3. Here's what was going wrong:

The SDK intercepts the redirect to capture attribution cookies, then calls completionHandler(nil) to prevent actually following the redirect (we only want one hop). However, when the redirect is cancelled, URLSession sometimes returns an error - either NSURLErrorTimedOut (-1001) or NSURLErrorCancelled (-999).

The original code checked for errors FIRST:

if let error = error {
    // Fail immediately ❌
    fulfill.resolve(with: (nil, nil))
}

But by this point, the redirect delegate had ALREADY successfully captured the destination URL! We were throwing away perfectly good data just because URLSession reported an error from cancelling the redirect.

Why It Was Intermittent

This was a race condition based on network timing:

  • Fast networks/good conditions: Request completes quickly → timeout error → deep link fails ❌
  • Slow networks/cached responses: Request completes before timeout → no error → deep link works ✅

So ironically, clients with better network conditions were MORE likely to experience failures.

The Fix

We reversed the logic to check if we captured the redirect URL FIRST, before checking for errors:

if self.deepLinkLocation != nil {
    // We got the data we need, use it! ✅
    fulfill.resolve(with: (self.deepLinkLocation, attributionInfo))
} else if let error = error {
    // Only fail if we truly didn't get the redirect data
    fulfill.resolve(with: (nil, nil))
}

We also fixed two missing completionHandler(nil) calls in early return paths that could cause URLSession tasks to hang.

Why This Works

The redirect delegate is called SYNCHRONOUSLY when the 303 response arrives, so deepLinkLocation is always set before the completion handler runs. By checking for this data first, we use it regardless of any subsequent timeout/cancellation errors from URLSession.

This matches what we saw in 2ULaundry's successful cases - they were getting the location and opening links correctly, just without attribution data (separate cookie parsing issue).

Impact

  • ✅ Fixes deep linking for all clients using /a/* URL patterns (all Iterable email/SMS links)
  • ✅ Resolves timeout errors (-1001)
  • ✅ Resolves cancellation errors (-999)
  • ✅ Eliminates network timing race conditions
  • ✅ Prevents hanging URLSession tasks

@sumeruchat sumeruchat changed the title Fix iOS deep link resolution failures for /a/* links SDK-45] Fix iOS deep link resolution failures for greenFi Nov 18, 2025
@sumeruchat sumeruchat changed the title SDK-45] Fix iOS deep link resolution failures for greenFi [SDK-45] Fix iOS deep link resolution failures for greenFi Nov 18, 2025
@codecov
Copy link

codecov bot commented Nov 18, 2025

Codecov Report

❌ Patch coverage is 41.17647% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.39%. Comparing base (6e2fdec) to head (462e301).
⚠️ Report is 899 commits behind head on master.

Files with missing lines Patch % Lines
swift-sdk/Internal/DeepLinkManager.swift 46.66% 8 Missing ⚠️
swift-sdk/Internal/Network/NetworkSession.swift 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #974       +/-   ##
===========================================
- Coverage   85.19%   69.39%   -15.81%     
===========================================
  Files          91      109       +18     
  Lines        6301     8929     +2628     
===========================================
+ Hits         5368     6196      +828     
- Misses        933     2733     +1800     

☔ View full report in Codecov by Sentry.
📢 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.

@joaodordio joaodordio self-requested a review November 19, 2025 16:49
Comment on lines +117 to +123
completionHandler(nil)
return
}

guard let url = response.url else {
delegate?.onRedirect(deepLinkLocation: deepLinkLocation, campaignId: campaignId, templateId: templateId, messageId: messageId)
completionHandler(nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the nil completionHandler here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats just a bug that was there the completionhandler wasnt fired if there was no info in the headers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see! So, to double check, what we want is (as per the documentation) to pass nil on the completionHandler so we let "the body of the redirection response to be delivered as the payload of this request" correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically completion handler should be always fired before returning here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we pass nil on the completionHandler to not follow any further redirects the redirection is passed in the delegate callbacks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but effectively yes when the conpletionHandler(nil) is fired it would finish the redirection flow

@davidtruong
Copy link
Contributor

@sumeruchat Is this issue happening specifically with shortened urls with SMS only or with any re-written url?

@sumeruchat
Copy link
Contributor Author

@davidtruong It would happen a /a url in particular that would follow the redirect but if iOS returned an error of any kind it wouldnt use the redirect url

@davidtruong
Copy link
Contributor

@davidtruong It would happen a /a url in particular that would follow the redirect but if iOS returned an error of any kind it wouldnt use the redirect url

Do we know why a NSURLErrorCancelled would get called for the original request?

Comment on lines +72 to +76
// Check if we successfully captured redirect data FIRST
// The delegate callback happens before the error, so deepLinkLocation
// may be set even if we get NSURLErrorTimedOut (-1001) or
// NSURLErrorCancelled (-999) from cancelling the redirect
if self.deepLinkLocation != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that the deepLinkLocation is updated on the onRedirect delegate method. It's just weird that we are conditioning the logic of this makeDataRequest completionHandler be dependent on something happening elsewhere.

Logically I think this makes sense, but is there a way for us to better document this in code or use a different architecture to ensure it makes sense to disregard an error on a networkRequest completionHandler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well i wouldnt recommend refactoring but logically it means that we have a redirect url we use it no matter what the http connection or tcp connection returns. Its more robust than before

@joaodordio
Copy link
Member

⚠️ We should probably do a small Bug Bash to ensure these changes are covered in various scenarios before allowing this to go into GA.

@sumeruchat
Copy link
Contributor Author

@davidtruong So basically I was thinking that passing nil to the completionHandler in willPerformHTTPRedirection might return a cancelled error or its also possible that the request times out due to the server being slow to responr or bad network and it doesnt use the URL it already has received.

@sumeruchat
Copy link
Contributor Author

@davidtruong Another bug we fixed is that if we dont call the completionHandler at all before return in the guard condition it might hang and not complete the request at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants