1897264 - Add a `SuggestStore.interruptEverything` API to interrupt ingestions

Status: RESOLVED FIXED

(Application Services :: Suggest, task)

Description

[Lina Butler [:lina]]

SuggestStore.interrupt() currently only interrupts reads. This is by design, so that apps can cancel ongoing queries as the user types, and free up the reader connection for the user's latest input.

We currently don't have an analogue for interrupting ingestions or writes, which is a problem if we're backgrounded on iOS (or the user closes Firefox, in the case of Desktop), and the OS tells us to wrap up our work. It's still not possible to interrupt network requests with Viaduct, but that's OK; we can still check and interrupt the ingestion after the network request completes.

I'm thinking that interruptEverything could be this analogue, and interrupt writes as well as reads.

In the worst case of an empty or rechunked database, if we're making 80 network requests for an ingestion, and the OS tells us to stop when we're in the middle of request 5, we can interrupt ingestion after request 5 finishes, without having to wait for the remaining 75 requests to finish.

On the iOS side, we can call interruptEverything from our background ingestion task's "expiration handler". That handler gets called whenever iOS wants to tell us to wrap up.

On Desktop, we can call interruptEverything at shutdown.

(I'm not tied to the interruptEverything name at all; I just wanted to make it something long and vaguely awkward, to gently discourage consumers from using it incorrectly 😅 Unless the app is shutting down, it shouldn't need to call interruptEverything).

Comment 1

[Ben Dean-Kawamura [:bdk]]

What do you think of using the shutdown function for this? That was designed for this use-case.

Calling shutdown would also interrupt all other components with interruption support. I think that's okay, maybe good. What do you think?

Comment 2

[Lina Butler [:lina]]

Oh, cool; that's a great system!

I think I'm a little nervous about making the Suggest background task handler interrupt all components with interruption support, though. iOS can schedule background tasks separately, with different time slices for each—e.g., I think it's possible for the sync1 and sync2 tasks to keep running even if ingest expires, or even for sync{1, 2} to run concurrently with ingest. I'm not sure that we'd want ingest's expiration handler to also shut down sync1 and sync2.

WDYT?

Comment 3

[Ben Dean-Kawamura [:bdk]]

That makes sense to me. Also shutdown causes all future operations to be interrupted as well, which doesn't seem right for this case. Sounds like we do want an interruptEverything method rather than shutdown.

Comment 4

[Ben Dean-Kawamura [:bdk]]

One more thought here, maybe it's better to define an interruptIngestion method that doesn't interrupt in-progress reads. For this use-case I think it's the same, but maybe some consumers will want to interrupt the ingestion task because it's taking too long, but don't want to interrupt pending reads.

Comment 5

[Lina Butler [:lina]]

That sounds good to me, too! The expiration handler can call both interrupt() and interruptIngestion(), and consumers that only want to interrupt the latter can do just that.

Comment 6

[BMO Github Automation]

Attachment: [mozilla/application-services] Bug 1897264 - interruption support for ingestion (#6246)

Comment 7

[BMO Github Automation]

Attachment: [mozilla/application-services] Bug 1897264 - interruption support for ingestion (backport #6246) (#6256)

Comment 8

[Lina Butler [:lina]]

Comment on attachment 9404497 [details] [review]
[mozilla/application-services] Bug 1897264 - interruption support for ingestion (backport #6246) (#6256)

Beta/Release Uplift Approval Request

• User impact if declined: Users enrolled in the Enhanced Cross-Platform Suggest experiment in Firefox for iOS could experience increased app relaunch times, due to the system force-stopping Firefox because the background task for downloading new suggestions can't be interrupted. This App Services PR adds the necessary API for interrupting downloads; there's a separate Firefox for iOS PR that adopts the API.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a new API that has automated test coverage, and has been landed and tested in Firefox for iOS 128.
• String changes made/needed: None
• Is Android affected?: No

Comment 9

[Pascal Chevrel:pascalc]

https://github.com/mozilla/application-services/commit/36ef28daeaf81423b40c24a10eb1d0ce8873d3e4

Comment 10

[Lina Butler [:lina]]

Thanks for merging the uplift PR, Pascal!

When you have some time, would you mind cutting an App Services 127.0.1 release with it, please?

Comment 11

[Pascal Chevrel:pascalc]

(In reply to Lina Butler [:lina] from comment #10)

Thanks for merging the uplift PR, Pascal!

When you have some time, would you mind cutting an App Services 127.0.1 release with it, please?

This is done https://github.com/mozilla/application-services/releases/tag/v127.0.1

Comment 12

[Pascal Chevrel:pascalc]

https://github.com/mozilla/application-services/commit/8bd7ba4ea4dd4454420ca1eb87558c3c06d72162 landed on main but thr bugzilla integration didn't update the bug flags and closed the bugs.
I am resolving the bug as fixed and updating the 128 flag.

Comment 13

[Mathew Hodson]

Vendored in bug 1899410

Comment 14

[Mathew Hodson]

This wasn't uplifted to Firefox 127