Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(service-worker): Don't stay locked in EXISTING_CLIENTS_ONLY if corrupted data #37453

Conversation

adrienverge
Copy link
Contributor

@adrienverge adrienverge commented Jun 5, 2020

Problem

After #31109 and #31865, it's still possible to get locked in state
EXISTING_CLIENTS_ONLY, without any possibility to get out (even by
pushing new updates on the server).
More specifically, if control doc /latest of ngsw:/:db:control once
gets a bad value, then the service worker will fail early, and won't be
able to overwrite /latest with new, valid values (the ones from future
updates).

For example, once in this state, URL /ngsw/state will show:

NGSW Debug Info:
Driver state: EXISTING_CLIENTS_ONLY (Degraded due to failed initialization: Invariant violated (initialize): latest hash 8b75… has no known manifest
Error: Invariant violated (initialize): latest hash 8b75… has no known manifest
    at Driver.<anonymous> (https://my.app/ngsw-worker.js:2302:27)
    at Generator.next (<anonymous>)
    at fulfilled (https://my.app/ngsw-worker.js:175:62))
Latest manifest hash: 8b75…
Last update check: 22s971u

... with hash 8b75… corresponding to no installed version.

Solution

Currently, when such a case happens, the service worker simply fails
with an assertion
. Because this failure happens early, and is not
handled, the service worker is not able to update /latest to new
installed app versions.

I propose to detect this corrupted case (a latest hash that doesn't
match any installed version) a few lines above, so that the service
worker can correctly call its already existing cleaning code.

This change successfully fixes the problem described above.

I tried to add unit tests, but the mock fixture in https://github.com/angular/angular/blob/d1ea1f4/packages/service-worker/worker/test/happy_spec.ts is too simple to include access to cache storage / ngsw:/:db:control / /latest. Spontaneously I'm not sure unit tests are needed for this locked-state-recovery fix. However if unit tests can be written, could you guide me on what to do?

Unit test written with the help of George Kalpakas. Thank you!

@pullapprove pullapprove bot requested a review from IgorMinar June 5, 2020 10:09
@gkalpak gkalpak requested review from gkalpak and removed request for IgorMinar June 5, 2020 20:28
@gkalpak gkalpak added area: service-worker Issues related to the @angular/service-worker package action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release type: bug/fix labels Jun 5, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jun 5, 2020
@googlebot

This comment has been minimized.

@googlebot googlebot added cla: no and removed cla: yes labels Jun 5, 2020
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, @adrienverge 💯

I have left a couple of minor comments. Testing this is indeed not very straight forward. I have pushed a commit with a test.

packages/service-worker/worker/src/driver.ts Outdated Show resolved Hide resolved
packages/service-worker/worker/src/driver.ts Outdated Show resolved Hide resolved
@gkalpak

This comment has been minimized.

@googlebot

This comment has been minimized.

@googlebot googlebot added cla: yes and removed cla: no labels Jun 5, 2020
@gkalpak gkalpak added cla: no action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer cla: yes labels Jun 5, 2020
@googlebot

This comment has been minimized.

@adrienverge adrienverge force-pushed the fix/service-worker-locked-in-existing-clients-only branch from 0fb00da to d2cba7e Compare June 6, 2020 07:17
@googlebot googlebot removed the cla: no label Jun 6, 2020
Copy link
Contributor Author

@adrienverge adrienverge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkalpak thanks a lot for helping me with a test! That's similar to what I started writing yesterday, but await scope.caches.open('ngsw:/:db:control') was the missing part 👍

I just squashed your fixup commit and reworded messages. I also updated to commit message to mention you.

packages/service-worker/worker/src/driver.ts Outdated Show resolved Hide resolved
packages/service-worker/worker/src/driver.ts Outdated Show resolved Hide resolved
@adrienverge adrienverge force-pushed the fix/service-worker-locked-in-existing-clients-only branch from d2cba7e to f265298 Compare June 6, 2020 07:50
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant ✨
Thank you, @adrienverge 👍

// Make sure latest manifest is correctly installed. If not (e.g. corrupted data),
// it could stay locked in EXISTING_CLIENTS_ONLY or SAFE_MODE state.
if (!this.versions.has(latest.latest) &&
Object.keys(manifests).indexOf(latest.latest) === -1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, why not:

Suggested change
Object.keys(manifests).indexOf(latest.latest) === -1) {
!manifests.hasOwnProperty(latest.latest)) {

😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better 👍

PR updated!

…rrupted data

**Problem**

After angular#31109 and angular#31865, it's still possible to get locked in state
`EXISTING_CLIENTS_ONLY`, without any possibility to get out (even by
pushing new updates on the server).
More specifically, if control doc `/latest` of `ngsw:/:db:control` once
gets a bad value, then the service worker will fail early, and won't be
able to overwrite `/latest` with new, valid values (the ones from future
updates).

For example, once in this state, URL `/ngsw/state` will show:

    NGSW Debug Info:
    Driver state: EXISTING_CLIENTS_ONLY (Degraded due to failed initialization: Invariant violated (initialize): latest hash 8b75… has no known manifest
    Error: Invariant violated (initialize): latest hash 8b75… has no known manifest
        at Driver.<anonymous> (https://my.app/ngsw-worker.js:2302:27)
        at Generator.next (<anonymous>)
        at fulfilled (https://my.app/ngsw-worker.js:175:62))
    Latest manifest hash: 8b75…
    Last update check: 22s971u

... with hash `8b75…` corresponding to no installed version.

**Solution**

Currently, when such a case happens, the service worker [simply fails
with an assertion][1]. Because this failure happens early, and is not
handled, the service worker is not able to update `/latest` to new
installed app versions.

I propose to detect this corrupted case (a `latest` hash that doesn't
match any installed version) a few lines above, so that the service
worker can correctly call its [already existing cleaning code][2].

[1]: https://github.com/angular/angular/blob/3569fdf/packages/service-worker/worker/src/driver.ts#L559-L563
[2]: https://github.com/angular/angular/blob/3569fdf/packages/service-worker/worker/src/driver.ts#L505-L519

This change successfully fixes the problem described above.

Unit test written with the help of George Kalpakas. Thank you!
@adrienverge adrienverge force-pushed the fix/service-worker-locked-in-existing-clients-only branch from f265298 to c84d6f7 Compare June 7, 2020 15:52
@gkalpak gkalpak added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jun 8, 2020
@atscott atscott closed this in d63ecf4 Jun 8, 2020
atscott pushed a commit that referenced this pull request Jun 8, 2020
…rrupted data (#37453)

**Problem**

After #31109 and #31865, it's still possible to get locked in state
`EXISTING_CLIENTS_ONLY`, without any possibility to get out (even by
pushing new updates on the server).
More specifically, if control doc `/latest` of `ngsw:/:db:control` once
gets a bad value, then the service worker will fail early, and won't be
able to overwrite `/latest` with new, valid values (the ones from future
updates).

For example, once in this state, URL `/ngsw/state` will show:

    NGSW Debug Info:
    Driver state: EXISTING_CLIENTS_ONLY (Degraded due to failed initialization: Invariant violated (initialize): latest hash 8b75… has no known manifest
    Error: Invariant violated (initialize): latest hash 8b75… has no known manifest
        at Driver.<anonymous> (https://my.app/ngsw-worker.js:2302:27)
        at Generator.next (<anonymous>)
        at fulfilled (https://my.app/ngsw-worker.js:175:62))
    Latest manifest hash: 8b75…
    Last update check: 22s971u

... with hash `8b75…` corresponding to no installed version.

**Solution**

Currently, when such a case happens, the service worker [simply fails
with an assertion][1]. Because this failure happens early, and is not
handled, the service worker is not able to update `/latest` to new
installed app versions.

I propose to detect this corrupted case (a `latest` hash that doesn't
match any installed version) a few lines above, so that the service
worker can correctly call its [already existing cleaning code][2].

[1]: https://github.com/angular/angular/blob/3569fdf/packages/service-worker/worker/src/driver.ts#L559-L563
[2]: https://github.com/angular/angular/blob/3569fdf/packages/service-worker/worker/src/driver.ts#L505-L519

This change successfully fixes the problem described above.

Unit test written with the help of George Kalpakas. Thank you!

PR Close #37453
ngwattcos pushed a commit to ngwattcos/angular that referenced this pull request Jun 25, 2020
…rrupted data (angular#37453)

**Problem**

After angular#31109 and angular#31865, it's still possible to get locked in state
`EXISTING_CLIENTS_ONLY`, without any possibility to get out (even by
pushing new updates on the server).
More specifically, if control doc `/latest` of `ngsw:/:db:control` once
gets a bad value, then the service worker will fail early, and won't be
able to overwrite `/latest` with new, valid values (the ones from future
updates).

For example, once in this state, URL `/ngsw/state` will show:

    NGSW Debug Info:
    Driver state: EXISTING_CLIENTS_ONLY (Degraded due to failed initialization: Invariant violated (initialize): latest hash 8b75… has no known manifest
    Error: Invariant violated (initialize): latest hash 8b75… has no known manifest
        at Driver.<anonymous> (https://my.app/ngsw-worker.js:2302:27)
        at Generator.next (<anonymous>)
        at fulfilled (https://my.app/ngsw-worker.js:175:62))
    Latest manifest hash: 8b75…
    Last update check: 22s971u

... with hash `8b75…` corresponding to no installed version.

**Solution**

Currently, when such a case happens, the service worker [simply fails
with an assertion][1]. Because this failure happens early, and is not
handled, the service worker is not able to update `/latest` to new
installed app versions.

I propose to detect this corrupted case (a `latest` hash that doesn't
match any installed version) a few lines above, so that the service
worker can correctly call its [already existing cleaning code][2].

[1]: https://github.com/angular/angular/blob/3569fdf/packages/service-worker/worker/src/driver.ts#L559-L563
[2]: https://github.com/angular/angular/blob/3569fdf/packages/service-worker/worker/src/driver.ts#L505-L519

This change successfully fixes the problem described above.

Unit test written with the help of George Kalpakas. Thank you!

PR Close angular#37453
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 9, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
…rrupted data (angular#37453)

**Problem**

After angular#31109 and angular#31865, it's still possible to get locked in state
`EXISTING_CLIENTS_ONLY`, without any possibility to get out (even by
pushing new updates on the server).
More specifically, if control doc `/latest` of `ngsw:/:db:control` once
gets a bad value, then the service worker will fail early, and won't be
able to overwrite `/latest` with new, valid values (the ones from future
updates).

For example, once in this state, URL `/ngsw/state` will show:

    NGSW Debug Info:
    Driver state: EXISTING_CLIENTS_ONLY (Degraded due to failed initialization: Invariant violated (initialize): latest hash 8b75… has no known manifest
    Error: Invariant violated (initialize): latest hash 8b75… has no known manifest
        at Driver.<anonymous> (https://my.app/ngsw-worker.js:2302:27)
        at Generator.next (<anonymous>)
        at fulfilled (https://my.app/ngsw-worker.js:175:62))
    Latest manifest hash: 8b75…
    Last update check: 22s971u

... with hash `8b75…` corresponding to no installed version.

**Solution**

Currently, when such a case happens, the service worker [simply fails
with an assertion][1]. Because this failure happens early, and is not
handled, the service worker is not able to update `/latest` to new
installed app versions.

I propose to detect this corrupted case (a `latest` hash that doesn't
match any installed version) a few lines above, so that the service
worker can correctly call its [already existing cleaning code][2].

[1]: https://github.com/angular/angular/blob/3569fdf/packages/service-worker/worker/src/driver.ts#L559-L563
[2]: https://github.com/angular/angular/blob/3569fdf/packages/service-worker/worker/src/driver.ts#L505-L519

This change successfully fixes the problem described above.

Unit test written with the help of George Kalpakas. Thank you!

PR Close angular#37453
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: service-worker Issues related to the @angular/service-worker package cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants