fix(holocron): bad modules could cause crashes and prevent restart #631

Matthew-Mallimo · 2021-12-08T21:36:03Z

Description

Updates holocron to v1.2.0, which includes checks to prevent bad modules from stopping module map polling.
Adds integration tests to an upcoming change to holocron

Motivation and Context

This new version of holocron will prevent one-app from crashing when a bad module is in the module map.

On one-app start, the server will log an error stating the bad module will be ignored until the next poll where the module succeeds to load.

If one-app is already running, it will log an error stating the bad module's name, and revert back to a working module.

How Has This Been Tested?

integration tests cover:

one-app start up with a bad module
- Should start up normally, just ignoring the bad module
one-app running with moduleA at 0.0.0, and moduleA 0.0.1 is deployed with problems
- one-app should continue to poll ignoring v0.0.1 until its healthy
one-app running when a new module is deployed with issues
- should continue to poll and ignore the broken module until its healthy

Types of Changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation (adding or updating documentation)
Dependency update
Security update

Checklist:

My change requires a change to the documentation and I have updated the documentation accordingly.
These changes should be applied to a maintenance branch.
This change requires cross browser checks.
Performance tests should be ran against the server prior to merging.
This change impacts caching for client browsers.
This change impacts HTTP headers.
This change adds additional environment variable requirements for One App users.
I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using One App?

…ling

github-actions · 2021-12-09T14:41:58Z

📊 Bundle Size Report

file name	size on disk	gzip
app.js	314.2KB	93.2KB
runtime.js	15.6KB	5.5KB
vendors.js	129.1KB	40KB
app~vendors.js	427.4KB	110.4KB
service-worker-client.js	17.1KB	5.3KB
legacy/app.js	334.8KB	98KB
legacy/runtime.js	15.6KB	5.5KB
legacy/vendors.js	189.4KB	51.7KB
legacy/app~vendors.js	449.1KB	116.4KB
legacy/service-worker-client.js	17.6KB	5.5KB

Generated by 🚫 dangerJS against 98e2774

__tests__/integration/one-app.spec.js

infoxicator · 2021-12-13T13:40:51Z

__tests__/integration/one-app.spec.js

+              version,
+            });
+            // not ideal but need to wait for app to poll;
+            await waitFor(5000);


Are we loading the same module 'cultured-frankie'; again? if so we should try and merge this test with the one above if you are able to get both error messages at the same time. This is to avoid overloading the integration tests and make them take longer unnecessarily

I was only able to get one regex matcher to work at a time within a single test case. Thats why I added another test case

What does the log look like? does it log both lines?
/Failed to get external react-intl from root module/
and
/There was an error loading module (?<moduleName>.*) at (?<url>.*). Reverting back to (?<workingModule>.*)?

If so we should try and find why the regex is not matching in one single case, loading the same module to the module map twice and that 5sec delay is expensive if we can avoid it 👍

prod-sample/sample-modules/unhealthy-frank/0.0.0/README.md

infoxicator · 2021-12-13T13:50:53Z

__tests__/integration/one-app.spec.js

+            // eslint-disable-next-line no-useless-escape
+            expect(workingUrl).toBe(`${testCdnUrl}/${gitSha}/${moduleName}/0.0.0/${moduleName}.node.js\"}`);
+          });
+          test('fails to get external `semver` for child module as an unsupplied `requiredExternal` for new module in mooduleMap', async () => {


have we not tested the same scenario above when requiring react-intl? the only difference I can see is unhealthy-frank tries to load a different external semver?

If this test is to test the scenario when 2 modules with broken required externals are deployed consecutively, then the afterEach in line 754 is resetting the module map anyway and removing cultured-frankie from the previous test

This test case test when a new module is added to the module map with an error. The previous test case tests when a module's version is updated with an issue

right so we are testing 1 case when the same module is updated (version bump) and it is broken and also when a brand new module which is broken is added for the first time? is that right?

infoxicator · 2021-12-13T13:52:57Z

prod-sample/sample-modules/unhealthy-frank/0.0.0/package.json

@@ -0,0 +1,31 @@
+{
+  "name": "unhealthy-frank",


I wonder if we can reuse any of the other franks rather than creating a brand new one? If we deploy the correct version of the root module that doesn't provide any externals, I think cultured-frankie@0.0.1 should be enough?

It was easier to add a new module that only had one version with an issue, than to manage an existing module with one working version and one broken one

I am just thinking that it is more work to maintain another frank but I guess now we removed the package-locks from them we shouldn't need to change them too much.

Is it achievable with the franks that we already got? if so maybe double check with the team to see what's their preference

maintenance aside, building another module would also be expensive and increase integration tests build time

An additional version of an existing module would have the same downside.

…d-sample readme

Matthew-Mallimo

left comments

Matthew-Mallimo · 2021-12-13T18:25:59Z

__tests__/integration/one-app.spec.js

+              version,
+            });
+            // not ideal but need to wait for app to poll;
+            await waitFor(5000);


I was only able to get one regex matcher to work at a time within a single test case. Thats why I added another test case

Matthew-Mallimo · 2021-12-13T18:26:55Z

__tests__/integration/one-app.spec.js

+            // eslint-disable-next-line no-useless-escape
+            expect(workingUrl).toBe(`${testCdnUrl}/${gitSha}/${moduleName}/0.0.0/${moduleName}.node.js\"}`);
+          });
+          test('fails to get external `semver` for child module as an unsupplied `requiredExternal` for new module in mooduleMap', async () => {


This test case test when a new module is added to the module map with an error. The previous test case tests when a module's version is updated with an issue

Matthew-Mallimo · 2021-12-13T18:28:19Z

prod-sample/sample-modules/unhealthy-frank/0.0.0/package.json

@@ -0,0 +1,31 @@
+{
+  "name": "unhealthy-frank",


It was easier to add a new module that only had one version with an issue, than to manage an existing module with one working version and one broken one

10xLaCroixDrinker · 2021-12-14T19:31:08Z

__tests__/integration/one-app.spec.js

+              version: modVersion,
+            });
+            // not ideal but need to wait for app to poll;
+            await waitFor(5000);


Why don't we set a lower max pall time for the integration tests?

5 seconds is hardcoded as the min - https://github.com/americanexpress/one-app/blob/main/src/server/utils/pollModuleMap.js#L35-L40

10xLaCroixDrinker · 2021-12-14T19:31:37Z

__tests__/integration/one-app.spec.js

+            await expect(requiredExternalsError).resolves.toMatch(revertErrorMatch);
+            expect(problemModule).toBe(modName);
+            expect(problemModuleUrl).toBe(`${testCdnUrl}/${gitSha}/${modName}/${modVersion}/${modName}.node.js`);
+            // eslint-disable-next-line no-useless-escape


Is this needed?

10xLaCroixDrinker · 2021-12-14T19:33:23Z

prod-sample/sample-modules/unhealthy-frank/0.0.0/package.json

@@ -0,0 +1,31 @@
+{
+  "name": "unhealthy-frank",


An additional version of an existing module would have the same downside.

prod-sample/sample-modules/unhealthy-frank/0.0.0/package.json

prod-sample/sample-modules/unhealthy-frank/0.0.0/src/components/UnhealthyFrank.jsx

prod-sample/sample-modules/unhealthy-frank/0.0.0/README.md

github-actions · 2022-01-14T00:47:28Z

This pull request is stale because it has been open 30 days with no activity.

…nexpress/one-app into test/holocronIntegration

10xLaCroixDrinker · 2022-01-27T17:08:28Z

__tests__/integration/one-app.spec.js

+              moduleName: modName,
+              version: modVersion,
+            });
+            // not ideal but need to wait for app to poll;


Please set a lower max poll time instead of waiting a full 5s

https://github.com/americanexpress/one-app/blob/main/src/server/utils/pollModuleMap.js#L35-L40

also for future reference here is where we set it - https://github.com/americanexpress/one-app/blob/main/prod-sample/one-app/base.env#L14

N/A

JAdshead · 2022-01-27T18:22:05Z

Approved from what you have but we will need to fix those other tests

test(integration): added integration test for new holocron error hand…

47caed4

…ling

github-actions bot added the test label Dec 8, 2021

chore(holocron): add holocron tgz

46d4c5d

feat(unhealthy): added unhealthy-frank and integration test

5c04d60

Matthew-Mallimo requested a review from a team December 10, 2021 14:44

infoxicator reviewed Dec 13, 2021

View reviewed changes

fix(integration): removed unnecessary moduleMap edit. Updated new pro…

d7b988b

…d-sample readme

Matthew-Mallimo commented Dec 13, 2021

View reviewed changes

Matthew-Mallimo requested a review from a team December 14, 2021 14:38

10xLaCroixDrinker reviewed Dec 14, 2021

View reviewed changes

github-actions bot added the stale-pr label Jan 14, 2022

10xLaCroixDrinker and others added 5 commits January 14, 2022 08:32

Merge branch 'main' into test/holocronIntegration

9dadb44

Merge branch 'main' into test/holocronIntegration

2e6e400

fix(holocron): update holocron to 1.2.0

221a0a5

Merge branch 'test/holocronIntegration' of https://github.com/america…

0674669

…nexpress/one-app into test/holocronIntegration

fix(tgz): remove holocron tgz

0ec0071

Matthew-Mallimo marked this pull request as ready for review January 27, 2022 16:23

Matthew-Mallimo requested a review from a team as a code owner January 27, 2022 16:23

Matthew-Mallimo changed the title ~~test(integration): added integration test for new holocron error~~ feat(holocron): upgrade holocron to 1.2.0 Jan 27, 2022

10xLaCroixDrinker changed the title ~~feat(holocron): upgrade holocron to 1.2.0~~ fix(holocron): bad modules could cause crashes and prevent restart Jan 27, 2022

10xLaCroixDrinker previously requested changes Jan 27, 2022

View reviewed changes

JAdshead previously approved these changes Jan 27, 2022

View reviewed changes

fix(integration): remove unhealthy-frank from module map

41089bb

Matthew-Mallimo dismissed JAdshead’s stale review via 41089bb January 27, 2022 19:10

10xLaCroixDrinker approved these changes Jan 27, 2022

View reviewed changes

10xLaCroixDrinker requested a review from a team January 27, 2022 19:30

Merge branch 'main' into test/holocronIntegration

98e2774

github-actions bot removed the stale-pr label Jan 28, 2022

bishnubista approved these changes Jan 28, 2022

View reviewed changes

10xLaCroixDrinker merged commit 3e53147 into main Jan 28, 2022

10xLaCroixDrinker deleted the test/holocronIntegration branch January 28, 2022 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(holocron): bad modules could cause crashes and prevent restart #631

fix(holocron): bad modules could cause crashes and prevent restart #631

Matthew-Mallimo commented Dec 8, 2021 •

edited

Loading

github-actions bot commented Dec 9, 2021 •

edited

Loading

infoxicator Dec 13, 2021

Matthew-Mallimo Dec 13, 2021

infoxicator Dec 14, 2021

infoxicator Dec 13, 2021

Matthew-Mallimo Dec 13, 2021

infoxicator Dec 14, 2021

infoxicator Dec 13, 2021

Matthew-Mallimo Dec 13, 2021

infoxicator Dec 14, 2021

infoxicator Dec 14, 2021

10xLaCroixDrinker Dec 14, 2021

Matthew-Mallimo left a comment

Matthew-Mallimo Dec 13, 2021

Matthew-Mallimo Dec 13, 2021

Matthew-Mallimo Dec 13, 2021

10xLaCroixDrinker Dec 14, 2021

JAdshead Jan 27, 2022

10xLaCroixDrinker Dec 14, 2021

10xLaCroixDrinker Dec 14, 2021

github-actions bot commented Jan 14, 2022

10xLaCroixDrinker Jan 27, 2022

JAdshead Jan 27, 2022

JAdshead Jan 27, 2022

JAdshead commented Jan 27, 2022

fix(holocron): bad modules could cause crashes and prevent restart #631

fix(holocron): bad modules could cause crashes and prevent restart #631

Conversation

Matthew-Mallimo commented Dec 8, 2021 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of Changes

Checklist:

What is the Impact to Developers Using One App?

github-actions bot commented Dec 9, 2021 • edited Loading

📊 Bundle Size Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matthew-Mallimo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAdshead commented Jan 27, 2022

Matthew-Mallimo commented Dec 8, 2021 •

edited

Loading

github-actions bot commented Dec 9, 2021 •

edited

Loading