ttr, caching and cache refresh bugs

[gkc]

Describe the bug

A number of bugs have crept in to the system over time with regards to the 'ttr' metadata

• In our onboarding code (in at_onboarding_cli.AtOnboardingServiceImpl, in at_client_mobile.AtClientService.persistKeys, and in at_java.OnboardingUtil.storePublicEncryptionKey) we are not setting a ttr in the metadata when we are storing the encryption public key.
  • ==> We should be setting it to -1 (cache indefinitely)
• AtMetadata.fromJson sets ttr to 0 if it sees a value of null. We should leave it as a null.
  • ttl and ttb are also set to 0 if the value in the JSON is null. We should leave it as a null.
• As a result of the previous two points, when a server does a proxy lookup of publickey@alice on another server, and the AtData (and its AtMetadata) is created from the response json, then the AtMetadata's ttr is zero
• ProxyLookupVerbHandler does not check ttr after doing a successful remote lookup - it just always caches. This is incorrect: if the value is null or 0 then the data should not be cached
  • As a result, currently we are (as intended) caching other atSigns' public encryption keys at cached:public:publickey@alice, and we are also caching other things like 'firstname.wavi@colin' which has a null ttr on @colin's atServer, and is cached on @garycasey's atServer with a ttr of 0
  • However, the atProtocol spec says we should not be caching, because the public key's ttr was originally set to null upon original creation, and is then being set to zero (by fromJson) when retrieved by a server-to-server proxy lookup; and both null and zero mean we should not cache
• LookupVerbHandler checks for cached values and, if not there, or if bypassCache is true in the lookup request, does a lookup on the remote server.
  • ==> However, LookupVerbHandler does not store anything in cache
  • ==> So how do we end up with shared data in cache? via auto-notifications, is the answer
• Cache refresh job does not work because we have cached values which have ttr of 0 which cause a null check error
• Minor nit: some variable names (and possibly code comments / docs?) suggest that ttr is in milliseconds (but ttr is a value in seconds - and is treated as such when generating the 'refreshAt' in the metadata in keystore updates)

Expected behavior

So to restate

• When storing publickey@alice, our various onboarding libraries are not setting ttr to -1
  • ==> at_client_mobile, at_onboarding_cli and at_java libraries need to be changed to set a value of -1 in the ttr. Separate tickets have been created for that work in the respective repos
  • meanwhile, we need to ensure that we can continue to cache public keys, as we do indeed wish to
  • ==> we will modify the server's cache manager code to check for publickey@atSign entries , and explicitly set their ttr to -1 if so
• ProxyLookupVerbHandler does not check ttr after doing a successful remote lookup - it just always caches. This is incorrect: if the value is null or 0 then the data should not be cached
  • ==> we will modify the server's cache manager code to check the ttr value and not cache if the value is null or zero (but with the exception of publickey@atSign as described above)
• LookupVerbHandler is not updating the cache
  • ==> we will make it update the cache, respecting the caching rules
• We have AtMetadata.fromJson bug
• We have cache refresh job failing
• We have code clarity issues (ttr is seconds, not milliseconds)
  • we will fix these as they are encountered

Additional context

We need to think about side-effects of fixing all of this

• I expect that some app code may have come to depend on automatic caching of public keys that shouldn't have been cached.

[VJag]

Agree with the analysis and the suggestions.

A couple of thoughts:

*. ttr of -1 indicates two things

1. It can be cached
2. The value that is cached need not be refreshed

So if the value that is going to be cached may probably change(Including public keys) then we should consider a non -1 value or come up with a different mechanism to update the cache.

*. We should probably find ways to optimize the ttr job along the lines of:

1. Optimal use of connections
2. A verb to know the keys that have changed from the value that has been cached
3. Batch lookup of the keys

*. Expose health of ttr job through stats verb

[gkc]

Thanks @VJag

Additionally as per our discussion, we will continue to cache public data whose ttr is null or 0 as we do now, but we will do so with a ttl of 24 hours so the cached copy will 'expire' after 24 hours.

[gkc]

Complete. PR requires review. Dropping SP to zero for next sprint

[gkc]

Closed by https://github.com/atsign-foundation/at_server/releases/tag/c3.0.28c