-
Notifications
You must be signed in to change notification settings - Fork 39
fix(hosting): never serve stale placeholder config.json after deploy #174
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
base: main
Are you sure you want to change the base?
Changes from all commits
ad95732
1705342
c152936
a695afb
a2a83c7
700eb48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@aws-blocks/core": patch | ||
| --- | ||
|
|
||
| Never serve a stale placeholder `config.json` after a deploy. The build-time placeholder (`{"_placeholder":true}`) was uploaded with the 1-year mutable cache-control (`public, s-maxage=31536000, max-age=0, must-revalidate`), and the post-deploy CloudFront invalidation targeted `/.blocks-sandbox/*` — which never matches the real cache key, because the skew-protection viewer-request function rewrites the URI to `/builds/<buildId>/.blocks-sandbox/config.json` before the cache lookup. An edge that cached the placeholder during the deploy window could keep serving it for up to a year, making `getApiUrl()` throw and every client API call fail. The placeholder is now registered as a no-cache path (`no-cache, no-store, must-revalidate`) so edges never cache it long-term, and the config deployment now also invalidates the post-rewrite key `/builds/<buildId>/.blocks-sandbox/*`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ | |
| FrameworkType, | ||
| } from '@aws-blocks/hosting'; | ||
| import { BLOCKS_RPC_PREFIX, BLOCKS_AUTH_PREFIX } from './constants.js'; | ||
| import { BLOCKS_SANDBOX_DIR } from './common/constants.js'; | ||
| import { registerConfig } from './cdk/config-registry.js'; | ||
| import { getRegisteredRoutes } from './raw-route.js'; | ||
|
|
||
|
|
@@ -415,7 +416,7 @@ | |
| ...(apiUrl ? { BLOCKS_API_URL: apiUrl } : {}), | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| throw new Error( | ||
| `Frontend build failed: ${props.buildCommand}\n` + | ||
| `Ensure the build command is correct and all dependencies are installed.`, | ||
|
|
@@ -499,6 +500,19 @@ | |
| JSON.stringify({ _placeholder: true }), | ||
| ); | ||
|
|
||
| // ── 5a. Mark config.json as a no-cache path ───────────────────── | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: the 8-line comment narrates the full #173 bug history that's already in the commit message. Keep the non-obvious invariant inline (config.json is mutable and must not inherit the content-hashed-asset s-maxage) and move the forensics to the commit body.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: the 8-line comment narrates the full #173 bug history that's already in the commit message. Keep the non-obvious invariant inline (config.json is mutable and must not inherit the content-hashed-asset s-maxage) and move the forensics to the commit body.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: the 8-line comment narrates the full #173 bug history that's already in the commit message. Keep the non-obvious invariant inline (config.json is mutable and must not inherit the content-hashed-asset s-maxage) and move the forensics to the commit body. |
||
| // config.json is a fixed-name, mutable runtime-config file: it must | ||
| // NOT inherit the content-hashed mutable-asset cache-control | ||
| // (`s-maxage=31536000`) applied to `/assets/<hash>.js`. Registering it | ||
| // as a no-cache path uploads the build-time placeholder with | ||
| // `no-cache, no-store, must-revalidate` so an edge never caches it | ||
| // long-term; the real config is deployed in step 8 with `max-age=60`. | ||
| const configNoCachePath = `${BLOCKS_SANDBOX_DIR}/config.json`; | ||
| const existingNoCachePaths = manifest.staticAssets.noCachePaths ?? []; | ||
| manifest.staticAssets.noCachePaths = existingNoCachePaths.includes(configNoCachePath) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The noCachePaths registration runs unconditionally (no api guard), which is correct — but both new tests always pass MOCK_API. A static-only test (no api prop) would guard against a future regression where the registration is accidentally moved inside the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a static-only test (no
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The noCachePaths registration runs unconditionally (no api guard), which is correct — but both new tests always pass MOCK_API. A static-only test (no api prop) would guard against a future regression where the registration is accidentally moved inside the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The noCachePaths registration runs unconditionally (no api guard), which is correct — but both new tests always pass MOCK_API. A static-only test (no api prop) would guard against a future regression where the registration is accidentally moved inside the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The noCachePaths registration runs unconditionally (no api guard), which is correct — but both new tests always pass MOCK_API. A static-only test (no api prop) would guard against a future regression where the registration is accidentally moved inside the |
||
| ? existingNoCachePaths | ||
| : [...existingNoCachePaths, configNoCachePath]; | ||
|
|
||
| // ── 5b. Inject static route for .blocks-sandbox config ────────── | ||
| // Insert a static route for /.blocks-sandbox/* so CloudFront | ||
| // serves config.json from S3 instead of routing to compute. | ||
|
|
@@ -596,7 +610,18 @@ | |
| destinationKeyPrefix: `builds/${buildId}/.blocks-sandbox`, | ||
| prune: false, | ||
| distribution: hosting.distribution, | ||
| distributionPaths: ['/.blocks-sandbox/*'], | ||
| // The skew-protection viewer-request CloudFront function rewrites the | ||
| // URI to `/builds/<buildId>/.blocks-sandbox/config.json` BEFORE the | ||
| // cache lookup, so the real edge cache key lives under `/builds/<id>/`. | ||
| // Invalidating only `/.blocks-sandbox/*` never matches that key and is | ||
| // a no-op for config.json. Invalidate the post-rewrite key too. (The | ||
| // primary guard against staleness is step 5a's no-cache placeholder; | ||
| // this is defense-in-depth so a post-deploy invalidation actually | ||
| // clears any edge entry at its real key.) | ||
| distributionPaths: [ | ||
| `/builds/${buildId}/.blocks-sandbox/*`, | ||
| '/.blocks-sandbox/*', | ||
| ], | ||
| cacheControl: [s3deploy.CacheControl.fromString('public, max-age=60, must-revalidate')], | ||
| }); | ||
|
|
||
|
|
@@ -718,7 +743,7 @@ | |
| let behaviorPattern: string; | ||
| const paramIndex = route.path.indexOf('/{'); | ||
| if (paramIndex !== -1) { | ||
| behaviorPattern = route.path.substring(0, paramIndex) + '/*'; | ||
| } else { | ||
| behaviorPattern = route.path; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the 8-line comment narrates the full #173 bug history that's already in the commit message. Keep the non-obvious invariant inline (config.json is mutable and must not inherit the content-hashed-asset s-maxage) and move the forensics to the commit body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done — trimmed the step-5a comment to the non-obvious invariant (config.json is mutable and must not inherit the content-hashed-asset
s-maxage). The bug forensics stay in the fix commit body. Fixed in a695afb.