1541022 - Write glean pings to a temporary file and then move them to final location

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Travis Long [:travis_]]

Glean.sendPings causes some async business to happen which ultimately causes a PingUploadWorker to be enqueued in WorkManager. It may be possible that quick, successive calls to sendPings() could cause problems since it replaces the existing worker.

Comment 1

[Alessio Placitelli [:Dexter]]

This doesn't seem to be a problem: our worker policy is now KEEP, so the worker doesn't get replaced or killed when calling sendPings, so having Synchronized there wouldn't help. However, the analysis helped uncover a few more issues:

(1) if we loose network connection, given that that's part of our worker constraints, the worker gets killed.

That's a documented behaviour of the WorkManager. I think that this can cause holes in our sequence numbers/data loss. Because:

• The worker calls PingStorageEngine.process

• in turn, this calls PingstorageEngine.processFile for each file in the storage directory. If upload is successful, this function deletes the file

• and, I think, we don't fail on network connectivity when uploading

This ends up deleting pings when network is down, while the worker is running. This is not highly likely to happen, but there's a chance that will happen.

(2) when writing pings, we might end up with partially written files to disk if the PingStorageEngine.process scans the directory while a write is happening.
As far as I can see, writing to disk writes to the final ping location, not to a temporary location and then moves the file there; no transactional-like behaviour.
There is a chance that the upload worker can see partial pings while scanning the directory, thus uploading corrupted data.

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Alessio Placitelli [:Dexter] from comment #1)

(2) when writing pings, we might end up with partially written files to disk if the PingStorageEngine.process scans the directory while a write is happening.
As far as I can see, writing to disk writes to the final ping location, not to a temporary location and then moves the file there; no transactional-like behaviour.
There is a chance that the upload worker can see partial pings while scanning the directory, thus uploading corrupted data.

Morphing this bug to handle the second case.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: GitHub Pull Request