Netlify Adapter: Deploy fails pointing to Lambda file size limitations in spite of proper size

regisphilibert commented 6 months ago

Astro Info

## Versions
Astro                    v4.5.0
Node                     v21.1.0
System                   macOS (arm64)
Package Manager          npm
Output                   hybrid
Adapter                  @astrojs/netlify 5.1.3
Integrations             none

Describe the Bug

When using SSR (setting output to hybrid and having an SSR route) the build will fail on Netlify during the "Deploy" stage with an error pointing to the function bundle file size when it is way below the (50mb zipped, 250mb unzipped) AWS Lambda limitation.

Deploy did not succeed with HTTP Error 400: [PUT /deploys/{deploy_id}/functions/{name}][400] uploadDeployFunction default  &{Code:400 Message:Failed to create function on AWS Lambda: invalid parameter for lambda creation: Unzipped size must be smaller than 262144000 bytes}

The error occurs when the whole deploy bundle (dist + .netlify) exceeds 250mb unzipped.

Should coder remove some files from the public directory for example to go below 250mb, the deploy will succeed.
Should coder keep the files but disable the hybrid output and remove the SSR route, the deploy will also succeed.

PS: Error above occured with less files (PDFs) above the limit. This following occurs with more files (images) above the limit:

Deploy did not succeed with HTTP Error 400: [PUT /deploys/{deploy_id}/functions/{name}][400] uploadDeployFunction default  &{Code:400 Message:could not parse form file: http: request body too large}

What's the expected result?

The deploy stage should succeeds as long as the bundled .netlify/function-internal/ssr directory remains below the Lambda limit and thus regardless of the /dist directory size.

Link to Minimal Reproducible Example

https://github.com/regisphilibert/astro-issue-repro-deploy-size

Participation

[ ] I am willing to submit a pull request for this issue.

regisphilibert commented 6 months ago

I struggled for naming this issue, I'm sure someone better understanding the problem will rename it proper :)

alexanderniebuhr commented 6 months ago

cc @Skn0tt

Skn0tt commented 6 months ago

Interesting, thanks for opening the issue! I'm currently on PTO, but will take a look at this when I come back in 2 weeks. If you feel this is more urgent, let me know and I can escalate internally.

Skn0tt commented 6 months ago

Okay, took a look at this. Thanks a lot for the reproduction repository, that was invaluable!

Astro is generating a chunks/pages/generic_PC-9mf2e.mjs file that contains this:

const imageConfig = {"service":{"entrypoint":"@astrojs/netlify/image-service.js","config":{}},"domains":[],"remotePatterns":[]};
const outDir = new URL("file:///Users/skn0tt/dev/tmp/astro-issue-repro-deploy-size/dist/");
new URL("_astro", outDir);
const getImage = async (options) => await getImage$1(options, imageConfig);

Netlify's bundler interprets the second line as a directory import, and adds the full directory to the Lambda. In this case, it's a false positive - outDir isn't referenced anywhere outside of this snippet. But that's close to impossible to reason via static analysis, so we get a false positive. This is better than a false negative, because most of these usages would need the contents of dist - but in this specific example, it bloats the Lambda.

Now i'm curious why Astro is generating this, especially since it's dead code. I'll dive into Astro internals to see if we can do anything here.

Skn0tt commented 6 months ago

It seems to me that this is a side-effect by https://github.com/withastro/astro/pull/10284/files#diff-038f9d38f16312162e9489c3adc3163fb507aa2133c82ec1581fab3c23659628R76. Before that change, assetsDir was unused if it wasn't imported, so it would be tree-shaken out before the Netlify bundler could pick it up. After the change, it needs to stay in because new URL might have side-effects so outDir needs to stay in the bundle, no matter if it's used. A hacky fix for this would be to mark the usage of new URL there as @pure - not sure if Vite picks that up, though. cc @Princesseuh

Skn0tt commented 6 months ago

diff --git a/packages/astro/src/assets/vite-plugin-assets.ts b/packages/astro/src/assets/vite-plugin-assets.ts
index db1a6e705..429180bee 100644
--- a/packages/astro/src/assets/vite-plugin-assets.ts
+++ b/packages/astro/src/assets/vite-plugin-assets.ts
@@ -68,14 +68,14 @@ export default function assets({
                    export { default as Picture } from "astro/components/Picture.astro";

                    export const imageConfig = ${JSON.stringify(settings.config.image)};
-                   export const outDir = new URL(${JSON.stringify(
+                   export const outDir = /* #__PURE__ */ new URL(${JSON.stringify(
                        new URL(
                            isServerLikeOutput(settings.config)
                                ? settings.config.build.client
                                : settings.config.outDir
                        )
                    )});
-                   export const assetsDir = new URL(${JSON.stringify(settings.config.build.assets)}, outDir);
+                   export const assetsDir = /* #__PURE__ */ new URL(${JSON.stringify(settings.config.build.assets)}, outDir);
                    export const getImage = async (options) => await getImageInternal(options, imageConfig);
                `;
                }

I tested this change locally, and it solved the issue. Happy to open a PR with this change, but I'd like to hear y'alls opinions first.

Princesseuh commented 6 months ago

Awesome work figuring that out, I would've quit programming probably. This changes makes sense to me, and I would approve it (with a comment on top, so we remember what happened)

I assume you've figured it out since, but just in case as an explanation, we need assetsDir and outDir for the Node image endpoint, so that it can know where to load images from (otherwise you'd end up with paths like example.com/_image?href=/something/else/whatever/var/www/project/dist/_astro/image.hash.png instead of being able to use a short path.)

Skn0tt commented 6 months ago

Figuring out arcane bundling errors is a big part of my job at Netlify, so I learned to be resilient about it over the years ;D But yeah this is definitely an interesting one! Thanks for double-checking my approach, I'll open a PR and tag you for a review.

we need assetsDir and outDir for the Node image endpoint

That makes sense, thanks for the explanation! When I inspect the output, it looks like outDir is an absolute path. That makes me think the output is not portable between systems, e.g. when you build in CI and then ship the code to your servers. Would it make more sense to make this path relative to import.meta.url?

Princesseuh commented 6 months ago

Hmm, I would guess so? What you say makes sense, I'm surprised no one complained yet. If you're willing to make that change while you're there, that'd be awesome!

Skn0tt commented 6 months ago

I tried implementing it, but i'm unsure how to do it. Initially, I thought it'd be a one-liner to get the relative path between outDir and the location where the file is written to, but it looks like that's not yet decided in the Vite Plugin stage: https://github.com/Skn0tt/astro/blob/e84999b6493b63363c8168502f37efa7e0b1fb9b/packages/astro/src/assets/vite-plugin-assets.ts#L77-L83

This feels like something that the Astro Core folks have more experience with. Maybe we can open up a new issue for it.

withastro / adapters