feat(AppConfig): cache the config if local cache is available

[susnux]

Summary

• resolves perf(AppConfig): Cache loading the app config from the database #54157

Benchmark

Tested flow:

• login
• move from dashboard to files app
• upload a folder with 1 subfolder and each 3 files
• in personal settings change the language
• back in files share the uploaded folder to another user
• add a comment on the uploaded folder
• log out

Queries on the app config table:

• Without caching: 140 queries (+-5)
• With caching:
  • TTL of 3s: 25 queries (+-3)
  • TTL of 120s: 9 queries (+-1)

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[skjnldsv]

What about #53868? 😅

My PR has red CI, although that might be fixed by rebasing and restarting cypress.

first one to green CI wins! Ready, set, go! 🐎 🏁

[AndyScherzinger]

🚀

[come-nc]

I forgot to send my review comments -_-, they were stuck in pending.

[fancycode]

This seems to break a usecase in Talk (see nextcloud/spreed#16227).

Within one request to Talk:

1. Request A: Talk creates a random nonce and stores it using setAppValue.
2. Request A: Talk performs a request to the HPB service backend and includes the random nonce.
3. The HPB service performs a request to Talk (B) to authenticate the incoming request, includes the nonce.
4. Request B: Talk reads the nonce using getAppValue, compares to the value sent by the HPB service and returns 200 or 412.
5. The HPB service returns 200 or an error depending on the response of request B.
6. Request A: Talk evaluates the response from the HPB service.

In some cases, the value read in step 4 is empty, so the request can not be authenticated by the HPB service.

If I set cache_app_config to false, this doesn't happen. Also this did not happen for instances before Nextcloud 32.

Thanks for @SystemKeeper for tracking this down.

[nickvergessen]

The problem is setTypedValue only does $refreshCache = true when the lazy attribute changes. Which is not the case here.

I guess setting a value that is cached should $refreshCache as well?
Or is the intended case that Talk would call clearCache() manually?

[fancycode]

I guess setting a value that is cached should $refreshCache as well?

IMHO that's the right thing to do. Having to clear the cache manually is a functional change that might break other affected apps, too.

[susnux]

The problem is setTypedValue only does $refreshCache = true when the lazy attribute changes. Which is not the case here.

The local cache is also cleared for non-lazy writes:

server/lib/private/AppConfig.php

Line 906 in 503f43f

$this->clearLocalCache();

refreshCache

This is only for removing the class local dictionary and that's not needed for non lazy as they are just added to the dictionary anyways.

[juliusknorr]

How does this work with clustered setups? I also fear this could break in a lot more places if a config value is written on one webserver and then the old valuer is still read on another webserver. Should we recommend disabling this for clustered instances? Maybe then the default should be switched to false for the config.php option? This will be super hard to debug such issues in production scenarios. :/

[fancycode]

Good point.

FWIW, our setup is not clustered but running the FPM Docker image (non-AIO).

[susnux]

Maybe then the default should be switched to false for the config.php option?

This was discussed with product management but decided that it has great benefits for the majority and there are only some special cases.
One we discussed is having multiple application servers but for this it was decided we only support pinned sessions.
Meaning one user will have consistent behavior, sure another user could see something already changed but the TTL only a couple seconds so this should be safe just like the setting was changed a second later.

If for any reason pinned sessions are not possible (we do not recommend this) then this can manually disabled in the settings, but this only affects very edge cases.

[juliusknorr]

If for any reason pinned sessions are not possible (we do not recommend this)

We always suggest either sticky sessions or redis as session storage, unless this changed recently. We have quite a few big instances not using session stickyness.

I think this shoud at least be properly documented and also noted to admins in the upgrade manuals.