867902 - Delete saved payload from disk

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

Up until bug 860094 we saved the most recently-uloaded FHR payload to disk. We should consider adding some code to delete the payload from disk so it doesn't linger forever.

Comment 1

[Stefan Mirea [:smirea]]

Attachment: Removing old saved FHR payload from disk;

Mainly changed "healthreport.jsm" to include information about the legacy location of the "last_payload.json" and "last_payload.json.tmp" files and delete them if they exit. A boolean flag "removedOutdatedLastpayload" has been added to HealthReportState as this code is only meant to run once to delete the untracked payload files. The only doccumented issue was that, upon initialization, the state of "removedOutdatedLastpayload" is not immediately saved to a file and instead it waits for a future ".save()". This was done so it would not directly conflict with the "application re-upgrade" decision taken on healthreport.jsm:150

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 763863 [details] [diff] [review]
Removing old saved FHR payload from disk;

Review of attachment 763863 [details] [diff] [review]:
-----------------------------------------------------------------

This is mostly good. Next revision should hopefully get r+.

::: services/healthreport/healthreporter.jsm
@@ +339,5 @@
>      return Promise.resolve();
>    },
>  
>    _onStateInitialized: function () {
> +    return Task.spawn(function () {

Nit: Name functions that comprise tasks.

@@ +344,5 @@
> +      if (!this._state._s.removedOutdatedLastpayload) {
> +        yield this.deleteOldLastPayload();
> +        this._state._s.removedOutdatedLastpayload = true;
> +        // normally we should save this to a file but it directly conflicts with
> +        //  line 150 so for now this is commented out

Line numbers change. Reference the function or feature, not the line number.

Also, all source comments should be sentence like. They should start with a capital letter and end with a period.

@@ +359,4 @@
>  
> +      Metrics.Storage(this._dbName).then(this._onStorageCreated.bind(this),
> +                                         this._onInitError.bind(this));
> +    }.bind(this));

Let's think about the flow control here.

As written, you don't need the 2nd function here: you can just move the Services.obs and friends after the if (removeOutdatedLastpayload) in the main task body.

Now, onto more important matters.

What happens if this.deleteOldLastPayload() errors? The promise will be rejected and Task.jsm will turn this into an exception. That exception won't be caught and FHR will fail to initialize. Moreover, it's likely that said error will recur every time the application starts. In other words, FHR has been crippled and there is likely nothing the user can do to fix it. Not good.

@@ +364,5 @@
> +
> +
> +  /**
> +   * Removes the outdated lastpaylaod.json and lastpayload.json.tmp files
> +   * @see {@link https://bugzilla.mozilla.org/show_bug.cgi?id=867902} Bugzilla #867902

You don't need the link back to the bug.

@@ +367,5 @@
> +   * Removes the outdated lastpaylaod.json and lastpayload.json.tmp files
> +   * @see {@link https://bugzilla.mozilla.org/show_bug.cgi?id=867902} Bugzilla #867902
> +   * @return a promise for when all the files have been deleted
> +   */
> +  deleteOldLastPayload: function () {

This should probably be _ prefixed since it isn't a public API.

@@ +373,5 @@
> +    return Task.spawn(function remove_all_files () {
> +      for (let path of paths) {
> +        if (yield OS.File.exists(path)) {
> +          OS.File.remove(path);
> +        }

Consider catching exceptions.

::: services/healthreport/tests/xpcshell/test_healthreporter.js
@@ +299,5 @@
> +  reporter._shutdown();
> +  yield shutdownServer(server);
> +  try {
> +    yield create_files();
> +    [reporter, server] = yield getReporterAndServer("remove-old-lastpayload");

I thought we had magic in the tests to not reuse the state directory from multiple HealthReporter instances. If that's the case, this test may not actually do anything useful!

@@ +316,5 @@
> +
> +  } finally {
> +    reporter._shutdown();
> +    yield shutdownServer(server);
> +  }

You need a test somewhere that the state file has been updated saying the old files have been removed

Comment 3

[Stefan Mirea [:smirea]]

Attachment: Removing old saved FHR payload from disk;

Comment 4

[Stefan Mirea [:smirea]]

Attachment: Removing old saved FHR payload from disk;

updated with requested changes. Also moved to getJustReporter() from getReporterAndServer()

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 763915 [details] [diff] [review]
Removing old saved FHR payload from disk;

Review of attachment 763915 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there!

::: services/healthreport/healthreporter.jsm
@@ +339,5 @@
>      return Promise.resolve();
>    },
>  
>    _onStateInitialized: function () {
> +    return Task.spawn(function delte_outdated_payload() {

I don't think you need the return. But meh.

Also, typo in the function name.

@@ +347,5 @@
> +          this._state._s.removedOutdatedLastpayload = true;
> +          // Normally we should save this to a file but it directly conflicts with
> +          //  the "application re-upgrade" decision in HealthReporterState::init()
> +          //  which specifically does not save the state to a file.
> +          // yield this._state.save();

This is a complicated issue.

As things are currently implemented, if the client sees a state file with a version newer than what it supports, it internally stores a fresh state but holds off flushing this fresh state to disk. Only if a major change (e.g. new document ID generated during upload) occurs will the reset state be flushed to disk. The thinking is that if a client downgrades, we should hold off flushing and losing data in hopes that the client is reupgraded and can recover the lost data. If this happens before an upload or other major change, we're good.

So, what should we do about this one-time check for payload deletion? If the client is downgraded, we will incur deletion (because the old flag was lost). No big deal. And, if we save here, we will *always* incur a state save to update the "was deleted" flag. So, our optimistic "don't save unless there is something to save" code will be undermined. If a client downgrades, FHR client state will be lost.

But, if we don't save the state file, the payload deletion code will run until the state file is saved with the flag saying not to delete the files. If upload is disabled, the state file may never be saved because there is no new state to record! I suppose if we put more state in the file (such as last daily collection time - which we should do), this may partially solve the problem.

There is really no good answer here.

Richard: what do you think?

@@ +374,5 @@
> +   * @return a promise for when all the files have been deleted
> +   */
> +  _deleteOldLastPayload: function () {
> +    let paths = [this._state._lastPayloadPath, this._state._lastPayloadPath + ".tmp"];
> +    return Task.spawn(function remove_all_files () {

removeAllFiles

::: services/healthreport/tests/xpcshell/test_healthreporter.js
@@ +288,5 @@
> +add_task(function test_remove_old_lastpayload() {
> +  let reporter = getJustReporter("remove-old-lastpayload");
> +  let lastPayloadPath = reporter._state._lastPayloadPath;
> +  let paths = [lastPayloadPath, lastPayloadPath + ".tmp"];
> +  let create_files = function create_files() {

let createFiles = function () {

We use camelCase everywhere except where we don't :)

And, you don't need to name a function if that function is assigned to an object value or variable. All the tools will inherit the "left hand" symbol as the function's name.

@@ +317,5 @@
> +      do_check_true(yield OS.File.exists(path));
> +    }
> +  } finally {
> +    reporter._shutdown();
> +  }

Nice test!

Comment 6

[Gregory Szorc [:gps]]

See my question in review in comment #5. I'm just looking for a 2nd opinion here.

Comment 7

[Richard Newman [:rnewman]]

I drafted and discarded a comment suggesting that we don't track one-time deletion. Delete on upload, or delete on a version number transition. 

I don't regard excessive deletes as a problem. Better to accurately track other state; cleanup is best-effort. 

Did I understand your question?

Comment 8

[Stefan Mirea [:smirea]]

Attachment: Removing old saved FHR payload from disk;

added requested changes

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 763924 [details] [diff] [review]
Removing old saved FHR payload from disk;

Review of attachment 763924 [details] [diff] [review]:
-----------------------------------------------------------------

With rnewman's blessing, this looks good. We can fine tune it later.

::: services/healthreport/healthreporter.jsm
@@ +339,5 @@
>      return Promise.resolve();
>    },
>  
>    _onStateInitialized: function () {
> +    return Task.spawn(function deleteOutdatedPayload() {

Nit: This function does more than delete an outdated payload, so rename appropriately.

@@ +347,5 @@
> +          this._state._s.removedOutdatedLastpayload = true;
> +          // Normally we should save this to a file but it directly conflicts with
> +          //  the "application re-upgrade" decision in HealthReporterState::init()
> +          //  which specifically does not save the state to a file.
> +          // yield this._state.save();

Please remove the commented "yield" line. It serves no purpose.

Comment 10

[Stefan Mirea [:smirea]]

Attachment: Removing old saved FHR payload from disk;

Comment 11

[Stefan Mirea [:smirea]]

Attachment: Removing old saved FHR payload from disk;

Implemented requested updates

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca5fe9958e90

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ca5fe9958e90