feat: Implement API fallback system#3343
Conversation
There was a problem hiding this comment.
Why is ReVanced API a repository now when it previously wasnt? And why is that not reflected in the package? besides we already have a ReVancedAPI file so why did you make a new one
What is the need of all these new files?
The cache implementation seems too hacky, don't we use something like Ktor already? Why is cache manual?
If you're going to be using agentic tools for your contributions, the bare minimum is expected, leaving large amounts of duplicate dead code and such hacky implementations is unacceptable
I only used it for file structuring because when i had it integrated into the exsiting functions the files became in thousands of lines ( which i remember getting cooked for it in revanced-website migration ). Besides, this is not it just so you know. I have like..1 week for refinements that i will look at sometime ! for the naming, i put it in network/api because its more of an HTTP cordinator than a normal domain repo. It doesnt really work like the other ones that sit on top of "Room" or "DataStore" and keeping the retry stuff close to HttpService felt cleaner to me. But if you’d rather keep everything under domain/repository/ for consistency ( That specific aspect i was also scrutinized for by no one exept you a long time ago ), I can move it and rename the class, should Axel or Ohan want! If you read the pdf file in the chat that i had then you will see why the caching was manual, it just requires some bravery to read it :) |
|
There is no reason to write manual http caching when Ktor has a caching plugin that is well tested and functions fine |
Great. Say that to both Axel and OsumAtrix. |
|
osumatrix and axel are not the sole authorative members of revanced, infact regarding revanced manager osumatrix has no authority regarding the codebase, so fix your attitude regarding the "pdf", not a single part of it implies custom manual caching 
If I'm mistaken, then feel free to respond with it the same pdf that you drafted, also says the following: 
which applies to both issues I raised regarding the cache and the file structure
Justifications for such things are expected, comments like that going forward isn't going to be tolerated |
Thanks for the info. It was conversational, not on paper. The chat got long but it should still be there. |
mostafaNazari702
force-pushed
the
feat/api-resilience-fallback-system
branch
2 times, most recently
from
ecd7ba7 to
d6b658b
Compare
|
I will admit that I missed we were using OkHttp cache (which is apparently broken) instead of the Ktor plugin (which should be tested). That was a mistake on my part, but if that cache is insufficient then caching in a wrapper repository is still the correct implementation. There is no requirement to write unit tests here. Nothing in manager is currently unit tested. |
Huh? No, the tests are only for me and my implementation, when i am done and want to submit i will remove it. |
|
Oh. I thought you were complaining about having to add them. |
That comment that has three points is just a reminder for myself.Not a message to anyone. |
|
I truly hope that i did not misinterpret "infront of ReVancedAPI". |
Retry, primary/backup endpoint fallback, and stale-readable response cache for API calls. cache freshness is derived from Cache-Control's "max-age" (defaults to 5 minutes shall it not be present), stale entries remain readable indefinitly so they can be served when no endpoint is reachable. no invasive dialog/popup while a patching job is in flight to avoid restart-mid-patch. inlined the ReVancedAPI wrapper.
mostafaNazari702
force-pushed
the
feat/api-resilience-fallback-system
branch
from
d6b658b to
a146676
Compare
mostafaNazari702
changed the title
mostafaNazari702
changed the title
| } | ||
|
|
||
| suspend fun getPatchesUpdate() = request<ReVancedAsset>("patches${prefs.usePatchesPrereleases.prereleaseString()}") | ||
| suspend fun probePrimary(): Boolean = withContext(Dispatchers.IO) { |
There was a problem hiding this comment.
But is the fallback system supporting nested fallbacks?
Primary -> Fallback A -> Fallback B
If currently on B, it should probe A. if on A, it should probe Primary.
If on B, but A down and Primary online, it should be able to restore to Primary, even though A is down.
There was a problem hiding this comment.
the system is intentionally just primary + one backup which is the scope we agreed on. right now /about only exposes a single fallback URL so supporting something like Primary => A => B would need API changes as well before we could even represent that chain.
on the main concern though , restoring back to primary even if an intermediate fallback is down, that's already how it works. probePrimary() talks directly to the primary endpoint and recovery only depends on whether the primary is reachable. The backup's health isn't part of that decision. so if we're currently on the backup and the primary comes back online, we will show the restore prompt regardless of what's happening with the backup.
he re-check happens on app launch (per the spec), not continuously during a session so we don't interrupt someone while they're using the app.
if we ever want true N-level fallback chains the implementation already routes through an ordered endpoints() list, so extending it wouldn't be difficult. since that's outside the current scope and would also require /about changes, I'd rather treat it as a separate discussion than fold it into this PR.
I hope this clears it for you.
There was a problem hiding this comment.
Single backup is all we need in my opinion. The way to increase reliability would be sever side measures.
There was a problem hiding this comment.
The fallback can be down too, the functionality has already been discussed on our discord
There was a problem hiding this comment.
the system is intentionally just primary + one backup which is the scope we agreed on. right now /about only exposes a single fallback URL so supporting something like Primary => A => B would need API changes as well before we could even represent that chain.
on the main concern though , restoring back to primary even if an intermediate fallback is down, that's already how it works. probePrimary() talks directly to the primary endpoint and recovery only depends on whether the primary is reachable. The backup's health isn't part of that decision. so if we're currently on the backup and the primary comes back online, we will show the restore prompt regardless of what's happening with the backup.
he re-check happens on app launch (per the spec), not continuously during a session so we don't interrupt someone while they're using the app.
if we ever want true N-level fallback chains the implementation already routes through an ordered endpoints() list, so extending it wouldn't be difficult. since that's outside the current scope and would also require /about changes, I'd rather treat it as a separate discussion than fold it into this PR.
I hope this clears it for you.
The N chain fallback was discussed as in scope of the PR for the same reason as mentioned in my previous comment. It's essential for highest availability. Otherwise if you just probe primary, backup A could be on, but primary and B off, and it would never recover
There was a problem hiding this comment.
The fallback can be down too, the functionality has already been discussed on our discord
Not to go against you but i don't remember that. Not to imply that we did not have that convo but i have put a ton of time into the website and forgot. I do however remember you telling me that:
Primary down? => Backup.
Backup down as well AND we have cached data? => We go to offline mode and use the cache instead so functionality persists even in offline mode/unresponsive API mode.
No cached data and primary/backup down? => No choice but to tell the user "Not available" but we of course also check whether it is true endpoint unavailability or just user being offline for whatever reason.
There was a problem hiding this comment.
See https://discord.com/channels/952946952348270622/1485653081038393415/1501176573100036167 for the explanation on the functionality
|
removed dead retry plugin and added a clarifying comment * |
|
Dropped the recovery notification card, restart dialog, restartApp(), the when a session is on the backup a background loop probes the primary every 5 minutes and switches back silently once it is reachable. |
| import kotlinx.coroutines.flow.StateFlow | ||
| import kotlinx.coroutines.flow.asStateFlow | ||
|
|
||
| class EndpointState( |
There was a problem hiding this comment.
I don't think this class works for chained fallbacks and it is also overcomplicated. You simply need one persistence for a schema:
API(url: String, fallback: String?, restore: API?)
If current API is down -> fallback
If restore != null, in an interval probe all restore APIs recursively, the earliest wins.
There was a problem hiding this comment.
Reworked EndpointState into the priority-chain setup you described. Endpoints are now represented as "ApiEndpoint(url, fallback)" and the behavior matches what we discussed on Discord, if i rememeber correctly.
Requests always try the active endpoint first. if that one is down we walk the chain from the top and pick the first reachable endpoint, so higher-priority endpoints always win. for example if we are currently on C and it dies while A or B are available we will move back up to A/B instead of falling further down to D.
I also added the recovery logic for backup endpoints. while we are running on a backup, every 5 minutes we check whether any higher-priority endpoints have come back. If they have, we switch to the highest priority one that's reachable. over time that means we naturally climb back up the chain (e.g C then to B tehn to A) until we're back on the primary then the recovery loop stops doing anything.
I didnt keep an explicit "restore" link on the endpoint node. A doubly-linked immutable structure (A.fallback = B and B.restore = A) is awkward to construct with vals so the node only stores the fallback direction. when we need restore candidates we derive them by walking the chain from the head. Same behavior, just simpler to build and maintain ig.
Also worth noting that /about only exposes a single fallback today, so in practice we are still running with two tiers. The code just no longer assumes that's the limit, so if we add more fallback levels later, it should work without any further changes.
the literal "restore" field can be added back if you'd rather keep the structure closer to the original proposal, since we kinda....shifted a lot during the...journey?
Changed EndpointState to support a proper priority chain instead of just primary/fallback. Requests now try the active endpoint first and then walk down the chain as needed, always preferring higher-priority endpoints before dropping to lower-priority ones. Also added a small recovery loop: when we're on a backup endpoint, every 5 minutes we check whether a higher-priority one is available again and switch back automatically. After switching, update checks are triggered again. Also bundled a few review fixes into the same commit: "api_cache => http_cache", use forEach, remove redundant URL normalization, use a Duration for the interval, and only keep the reconnect loop running while we're actually on a backup endpoint.
6 participants
Implemented an API client layer that retries failed requests up to three times and automatically fails over to a backup endpoint ( hardcoded in the code ) when the primary is unavailable. A file system cache provides limited functionality in fully offline/degraded mode. session fail-over state persists across app restarts with an optional non-blocking UI prompt to restore primary endpoint usage once it recovers.