Clear cached forms on app upgrade

[grzesiek2010]

Closes #6301

Why is this the best possible solution? Were any other approaches considered?

On slack we have discussed with @seadowg two options and we decided that this one is what we like more.
The second option was switching to using const instead of class names as keys to store data in HashMap. However, this would require adding backward compatibility and the code would be convoluted.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

I think it would be good to test two things:

1. The issue itself to make sure it does not exist anymore.
2. Testing how clearing the cache works on app upgrade. We store cached forms along with savepoints in the cache but only cached forms should be deleted (we of course need to keep savepoints).

I can't think of anything else that should be tested more carefully.

Here I'm attaching APKs for testing the upgrade:
https://drive.google.com/file/d/1YUZDF1QLzbmZCrKqeCiIDHZTLAn1zsZk/view?usp=sharing

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

• added or modified tests for any new or changed behavior
• run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
• added a comment above any new strings describing it for translators
• added any new strings with date formatting to DateFormatsTest
• verified that any code or assets from external sources are properly credited in comments and/or in the about file.
• verified that any new UI elements use theme colors. UI Components Style guidelines

[lognaturel]

Apologies for jumping in without much context! Quick question -- did you consider catching any errors loading from cache file and reloading directly from the form definition in that case? I could have sworn that was already happening but maybe not completely? Clearing the cache on app update feels like a nuclear option and could add significant time to form load for forms with complex logic for every release including small point releases and many that have no impact on the cache files.

I probably won't have a chance to engage in a full discussion on this now so feel free to proceed for now if the approach still feels right and we can come back to some of my questions.

[seadowg]

Apologies for jumping in without much context! Quick question -- did you consider catching any errors loading from cache file and reloading directly from the form definition in that case? I could have sworn that was already happening but maybe not completely?

You're correct that already happens, but it can't account for errors that happen later due to mismatches in the "serialized" code. The linked issue is about a crash that happens during finalization.

Clearing the cache on app update feels like a nuclear option and could add significant time to form load for forms with complex logic for every release including small point releases and many that have no impact on the cache files.

Do we have forms that demonstrate this? @getodk/testers maybe has a set of them? I think it's worth investigating that and if caching is giving us noticeable loading time improvements, we should make that part of at least regression testing. Right now I don't think there's any check that would prevent it from being completely removed (or not work some large percentage of the time). I'm going to add "needs testing" before review here for that.

[grzesiek2010]

If caching is still needed we can consider a hybrid solution. That would mean clearing cache but only once for the next release and switching to using const instead of class names as keys to store extras. If we clear cache once then we can rework how extras are stored without backward compatibility and the code should still be clean.

[seadowg]

Some more thoughts on this I've had on this:

• I'm thinking there was probably a similar problem with forms that had been parsed by Collect before we wrote any entity code as those cached FormDef objects wouldn't include the extras needed to deal saveto etc. This was probably rare though.
• Although it does seem harder due to the sensitivity of Externalizable's implementation, I'm not confident that we are entirely protected against similar "silent" or "crashes later" problems when making changes to JavaRosa crashes.
• I've noticed that it's very common for OOM crashes to be preceded by a failed read from the cache (logs here). This makes me wonder if our fallback mechanism is actually causing larger memory overheads due to the readExternal work followed by a full parse.

It's starting to look to me like our "extras" concept for FormDef might be incompatible with the way we've been doing form caching (keeping the cache until reading it fails).

I'm also thinking that the only benefit we have from the fallback approach as opposed to clearing on upgrade is that some upgrades won't end up clearing the cache (due to JavaRosa's rate of change being lower than Collect's). However, when there is a change that breaks the cache, the fallback mechanism is inherently worse as it requires form loads to do more work (and that extra work will happen for every form).

Given all that, I still think it's important for us to get a sense of the gains we're getting from caching currently, but I'm also now feeling like clearing the cache on upgrade is potentially a better overall solution - we're trading sometimes not needing to do full loads after upgrade for faster load times on upgrades that do break the cache, a ton of simplicity and much stronger protection from problems. If we really want to win back the cached load times on upgrades that don't break anything, we could introduce a versioning mechanic (like with Java Serializable). These are very hard to manage in practice though.

Thoughts?

[srujner]

Hey, we have checked the loading times of the forms after the "upgrade" with the version from "PR" (clearing cache) and the version from "Master" (not clearing cache) and there is no noticeable loading time improvements.

But we really only have 2-3 larger forms on which to check this and, to be 100% honest, we are not sure if any problems will arise in the future from clearing the cache after the upgrade.

From our perspective, it is difficult to make a single decision here whether we should cleared cache after upgrade or not.

[seadowg]

From our perspective, it is difficult to make a single decision here whether we should cleared cache after upgrade or not.

Thanks for digging in! That's helpful.

[seadowg]

@grzesiek2010 I'm thinking we go ahead with this for the moment. We can always replace it with a more targeted fix later if we feel the performance hit is too great.

[srujner]

Tested with Success!

Verified on device with Android 10, 13

Verified cases:

• Issue The app crashes on form finalization #6301 is no longer reproducing;
• General regression check on savepoints;
• Upgrading the app from older version;
• Checking how cache behaves before and after app upgrade;
• Don't keep activities enabled/disabled;
• Minimize the app, rotate the screen;
• Upgrade the app during sending big forms;

[WKobus]

Tested with Success!

Verified on device with Android 14