:zap: Improve performance by reworking "upgrade middleware"

[ff137]

Relates to changes introduced here: #2922

---

Originally posted by @ff137 in #2922 (comment)

The upgrading middleware will now use a cache during upgrades and after the upgrade has been completed. Agents that have the upgrade middleware but never upgrade will still have a very minor performance hit, due to needing to query the is_upgrading record.

With regards to that "minor performance hit" -- I'm noticing that because upgrade_middleware is always added to the web application's set of middleware, it means that askar is queried for acapy_upgrading records every single time there is any incoming request.

I'm picking this up after testing with more debug logging (#3689)

And I can see with every /status/live or /status/ready check that we do, we get:

2025-04-30 10:52:50,958 acapy_agent.admin.server DEBUG Incoming request: GET /status/live
2025-04-30 10:52:50,963 acapy_agent.storage.askar DEBUG Fetching all records for acapy_upgrading with tag query None
2025-04-30 10:52:50,971 aiohttp.access INFO 127.0.0.6 [30/Apr/2025:10:52:50 +0000] "GET /status/live HTTP/1.1" 200 194 "-" "kube-probe/1.32"
2025-04-30 10:52:50,977 acapy_agent.admin.server DEBUG Incoming request: GET /status/ready
2025-04-30 10:52:50,982 acapy_agent.storage.askar DEBUG Fetching all records for acapy_upgrading with tag query None
2025-04-30 10:52:50,988 aiohttp.access INFO 127.0.0.6 [30/Apr/2025:10:52:50 +0000] "GET /status/ready HTTP/1.1" 200 194 "-" "kube-probe/1.32"

That means an extra, unnecessary askar session and connection is opened up every single API call.

I think this isn't just a minor performance hit, but really not the way that this should have been implemented.

Can we please revisit this, and see if there's a different way to handle the "upgrading" logic?

[jamshale]

I remember this a bit and couldn't think of a design that did what I wanted without this. The goal was to get the upgrade to happen much faster then it has.

I didn't think checking one db value was much of a performance hit considering it also looked to query other db values in all other middleware for every request. I think that's why I decided it was ok.

That being said if the performance is an issue maybe there's a way to use the cache as it does after an upgrade, before an upgrade. I can't remember the exact mechanics. This middleware is to prevent api calls messing with the db during an upgrade, and the mechanism was designed to be reused for other migrations in the future.

[ff137]

Hmm... Is the upgrade middleware only necessary if the base wallet type is askar? Since, then upgrades need to be catered for?

Edit: That might already be the case 👀 previous agent had askar wallet-type. Just upgraded to askar-anoncreds, and I don't see the middleware logs anymore. I didn't see where it's skipped in the code, or if the base wallet is added to already upgraded wallets. So, my bad!

Edit2: It seems that multitenant agents with askar-anoncreds type do keep the middleware though? i.e. they need to support subwallets that can be askar type?

[jamshale]

I can't remember exactly what I did. It was a bit more complicated then I thought it would be with multitenancy. The goal was to have an in-memory cache for it after the upgrade completed.

I can look at this again though. I'm not sure why I couldn't have the cache populated initially, and without looking at it I'm not sure what I did for clustered deployments either. I think I just figured it was good enough for now when I worked on more anoncreds stuff, but I agree I could be done better.

[jamshale]

I was looking at trying to fix this and remembered why I did this. The problem is because of multi-instance scenarios you have no way of knowing whether another instance received the api request to upgrade and it's critical that other instances are locked out of altering the records during the upgrade.

If there was any type of pub/sub with redis or database hooks with askar available in acapy I'd be able to avoid this db read. I can't think of anyway to avoid it currently. We could possibly be more selective to what endpoints the lock applied to. This would help the performance hit, but we decided early on that everything should be completely locked down during the upgrade.

As I said before, once a tenant (profile) is upgraded it won't have this performance hit because it is found in the anoncreds_wallets cache.

I'm happy to fix it if anyone can think of a way to communicate between instances in a more performant way then using the database.