Skip to content
This repository was archived by the owner on Jan 28, 2025. It is now read-only.

Fix passing etag arg as last modified header, prefer last modified if present as message deduplication id #2486

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nskarakoc
Copy link

ISR enabled pages does not always get fresh content because of SQS message deduplication id set to ETag of the object (previously generated static page HTML file) stored in S3. Here is the scenario where revalidate option is set to 1:

  1. Open the page, see regeneration happened and page content updated in S3 (assuming previously generated content was different or 5-min elapsed)
  2. Update page content (i.e. modify the DB records that are used to build the page)
  3. Wait for revalidate seconds
  4. Open the same page again, see regeneration is being triggered by the default Lambda, but is not picked by regeneration Lambda as SQS message deduplication id (ETag of page HTML file in S3) is exactly same as previous message.

So in this case we have to wait for 5 minutes which is SQS message deduplication interval. This PR will attempt to use last modified header instead, so that even if the page content is same, it's last modified date will be different (i.e. newer) therefore regeneration will happen.

@slsnextbot
Copy link
Collaborator

Handler Size Report

No changes to handler sizes.

Base Handler Sizes (kB) (commit e6367b5)

{
    "Lambda": {
        "Default Lambda": {
            "Standard": 1578,
            "Minified": 692
        },
        "Image Lambda": {
            "Standard": 1543,
            "Minified": 831
        }
    },
    "Lambda@Edge": {
        "Default Lambda": {
            "Standard": 1588,
            "Minified": 698
        },
        "Default Lambda V2": {
            "Standard": 1580,
            "Minified": 694
        },
        "API Lambda": {
            "Standard": 634,
            "Minified": 318
        },
        "Image Lambda": {
            "Standard": 1551,
            "Minified": 835
        },
        "Regeneration Lambda": {
            "Standard": 1233,
            "Minified": 566
        },
        "Regeneration Lambda V2": {
            "Standard": 1307,
            "Minified": 596
        }
    }
}

New Handler Sizes (kB) (commit 28f5880)

{
    "Lambda": {
        "Default Lambda": {
            "Standard": 1578,
            "Minified": 692
        },
        "Image Lambda": {
            "Standard": 1543,
            "Minified": 831
        }
    },
    "Lambda@Edge": {
        "Default Lambda": {
            "Standard": 1588,
            "Minified": 698
        },
        "Default Lambda V2": {
            "Standard": 1580,
            "Minified": 694
        },
        "API Lambda": {
            "Standard": 634,
            "Minified": 318
        },
        "Image Lambda": {
            "Standard": 1551,
            "Minified": 835
        },
        "Regeneration Lambda": {
            "Standard": 1233,
            "Minified": 566
        },
        "Regeneration Lambda V2": {
            "Standard": 1307,
            "Minified": 596
        }
    }
}

@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #2486 (28f5880) into master (e6367b5) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2486      +/-   ##
==========================================
+ Coverage   83.85%   83.90%   +0.04%     
==========================================
  Files         102      102              
  Lines        3717     3716       -1     
  Branches     1191     1190       -1     
==========================================
+ Hits         3117     3118       +1     
+ Misses        588      586       -2     
  Partials       12       12              
Impacted Files Coverage Δ
...ackages/libs/lambda-at-edge/src/default-handler.ts 83.68% <ø> (ø)
...ambda-at-edge/src/lib/triggerStaticRegeneration.ts 100.00% <100.00%> (+4.34%) ⬆️
packages/libs/core/src/images/imageOptimizer.ts 84.51% <0.00%> (+0.41%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@nskarakoc
Copy link
Author

@dphang @J3tto could you please take a look at this PR? Thanks!

@chrisneal
Copy link
Contributor

@dphang @J3tto Have you had a chance to look into this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants