Data loss: not all signature images are uploaded in offline-capable forms

[p2edwards]

Description

This is an intermittent bug that results in a loss of signature images.

If a form includes multiple image questions, at least one of which is appearance: signature, and the form is running in offline mode, it is likely that one or more of the signature image files get lost in the submission process.

To Reproduce

When running a form like this in offline mode:

type	name	label	appearance
start	start
end	end
image	sign_now	sign now!	signature
image	sign_again	sign again!	signature

The first or second signature image is likely to be missing, for each submission.

other factors:

• must use offline version of form to reproduce bug
• happens on 2.5.6 and 2.8.1 (about to test on 3.0.1)
• tried to sign both fields as quickly as possible
• having a non-signature appearance image in the form can also "crowd out" the signature file but regular images do not crowd each other out based on our testing so far.

Expected behavior Desired behavior is that no attachments are missing upon submission.

Browser and OS

• OS: Issue has been reproduced on macos and Linux.
• Browser: Issue has been reproduced in Chrome and Safari.

Additional context

I'm looking into this now, so I can add screenshots or improved description soon.

[joshuaberetta]

Additionally: https://community.kobotoolbox.org/t/issues-with-signatures-uploading-through-enketo/23735/4

[jnm]

Related: https://github.com/OpenClinica/enketo-express-oc/issues/158 and https://github.com/enketo/enketo-core/commit/7284ac9aea62669bcfe8e470a041d8d8eb809f69

• (possibly not, since I can still trigger the bug after reducing the DELAY from 1500 to 5 ms)

Likely the same problem as https://github.com/enketo/enketo-express/issues/198 (itself probably a continuation of https://github.com/enketo/enketo-express/issues/174).

Something is happening where the signature file is lost if the submit button is clicked before autosave successful appears in the console. We tried commenting out removeAutoSavedRecord() in case removing the auto-saved draft was also removing the same file used by the finalized submission, but that didn't help.

[eyelidlessness]

Thanks for reporting this! I'm looking into it. I've been able to reproduce this in the current version, and I think I understand what's happening.

[eyelidlessness]

So, I have a potential quick "fix", but I think it's worth discussing some options. First, here's what I've learned:

1. This issue likely affects offline mode as a whole. Any change made ~500ms before saving a record offline may be lost, depending on how that change is committed or accessed when saving. This is because of a delay here.
2. As noted in the comment, that delay was introduced to address #174.
3. However, the actual culprit in #174 is likely this, in enketo-core. That appears to be intended to debounce capturing the draw widget's call to the signature_pad library's toDataURL method, which is presumably expensive and can cause poor performance. That said...
4. If I understand the rest of the code around this correctly (and the copious logging output I produced to pretty much verify this), the second delay is only introduced when a sustained drawing event completes (e.g. mouseup or touchend events) not while continuously drawing (which does not perform an update at all), and is apparently optimizing only for multiple drawing strokes in quick succession.

Here are the options as I see them, roughly in order of value and risk (both high -> low):

Option 1: ensure all pending changes are committed before attempting to save anything

This is probably the "correct" fix, and almost certainly something we should do anyway in the fullness of time. My big (very big) concern is that it's a much bigger task, likely with many corner cases we may miss. And that greatly increases the probability of regressions, or even surfacing other bugs which may currently be hidden.

Option 2: remove the delays

I'm hesitant to say this with confidence (I don't know nearly enough about signature styles, other use cases for the draw widget, or performance constraints on (say) low-powered devices), but I suspect both delays can be removed. This doesn't 100% guarantee a solution, there's still technically a race condition. It's just a drastically shorter one that almost certainly would require automation to trigger.

My biggest concern here (besides my enumerated low confidences) is that there are likely other delays of this kind lurking, and may be hard to find and harder to evaluate whether they belong.

Option 3: replace the delays with explicit completion/readiness

This would involve:

• any applicable widgets and other form behavior to explicitly provide an interface to determine that they're updating (and/or do update) asynchronously, and a way (e.g. Promise) to signal that they have no such work pending. This has some of the same risk as option 2—finding applicable cases—but lower risk by allowing any such delay/asynchrony to remain but in a more reliable way.

Option 4: ~~every~~no one's favorite concurrency pattern, locking

This is the potential "fix" I have working (+- some potential edge cases, and currently without any test coverage). Essentially this would involve:

1. Initiate a Promise before the first delay.
2. Wait for that Promise to resolve before proceeding to save a record.
3. Resolve the Promise when updating the auto-saved record is complete (and when there are no other pending updates, as there may be several in quick succession).

This is probably the quickest and least impactful "fix", however... it's definitely incorrect. The delay in the draw widget is 1500ms, whereas the delay before saving a record is 500ms. It's a wonder my "fix" works at all, the only thing I can assume is the sum of the time it takes for me to mouse from draw widget to submit plus the various other IO and asynchrony delays is generally good enough that the draw widget wins the race.

---

My instinct is to go with option 4, but I wanted to make sure the other options are known and up for discussion. Thoughts anyone? @lognaturel @yanokwa @MartijnR?

[MartijnR]

Thanks!

Am also not a fan of the various delays. Sorry about that! :)

1. This issue likely affects offline mode as a whole. Any change made ~500ms before saving a record offline may be lost, depending on how that change is committed or accessed when saving.

Only autosaved records though, right?

It looks like this issue occurs on non-touchscreen devices only (because on touch-screen devices we have the .hide-canvas-btn to force an update). If so, could the 3rd option be implemented by letting the canvas blur event handler set a new promise? The widget superclass could have a updatePending method that defaults to return Promise.resolve(), but may have an overriding implementation in file widgets.

[jnm]

Hello @eyelidlessness, @MartijnR, did you all reach any consensus on the best way to proceed? The KoBo team could contribute work toward a fix, but I'd want the design direction to be clear beforehand. Thank you!

[eyelidlessness]

It's been a while since I looked at my potential fix, but from my recollection I'd think it's probably a good enough band-aid, anticipating a more thoughtful solution in the longer term. I'll look into testing and preparing my solution for a PR soon.

[jnm]

Thank you very much. We're also happy to help with testing and review; just let us know if we can assist in any way.

[lognaturel]

@jnm Want to make sure you saw @eyelidlessness’ review request! We’re hoping to get a release out soon, ideally with this fix.

[jnm]

:tada: Yes! I saw it. Thank you @eyelidlessness. Will review today or tomorrow.

[xref]

[jnm]

@joshuaberetta, after you mentioned that you were still seeing this bug on 3.1.0, I reproduced it as well (with a mouse, this time, not a touchscreen) and recorded a screencast: https://user-images.githubusercontent.com/2085013/165527413-64184ccf-e7b1-44d6-919c-77fc6e74686e.mp4

I think we should test on 3.1.1 just to make sure it's still broken there, and then also test the earliest release that included #373 to see whether this problem is a regression or just something I missed by not testing thoroughly enough during my review.

After that, let's reopen this issue if needed.

[joshuaberetta]

@lognaturel, I've tested this on 3.0.5, which was the earliest release with the fix, and with 4.1.1. In offline mode, I'm able to see the same intermittent loss of data, but also an additional issue which I didn't notice before — sometimes the signature image is blank (signature-10_49_13) even when an image file is sent. I tested on my laptop and phone and got the same behaviour. Although neither is reliably reproducible, it did appear as though the last signature question lost data more often than the first. Tested with the same form in the issue description: signature.xlsx. Can we reopen this issue?

[joshuaberetta]

Another case in the Kobo community: https://community.kobotoolbox.org/t/problems-with-loss-of-signature-images/33825