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

Dev Server crashes if saving before the last rebuild complete #6919

Closed
1 task done
xHomu opened this issue Jul 21, 2023 · 29 comments
Closed
1 task done

Dev Server crashes if saving before the last rebuild complete #6919

xHomu opened this issue Jul 21, 2023 · 29 comments

Comments

@xHomu
Copy link
Contributor

xHomu commented Jul 21, 2023

What version of Remix are you using?

v1.19.0

Are all your remix dependencies & dev-dependencies using the same version?

  • Yes

Steps to Reproduce

This is somewhat inconsistent, since it depends in part how quickly your server rebuilds.

This most frequently occurs when working on our production apps with longer rebuild time (https://github.com/manawiki/starrail) and CTRL+S save rapidly before rebuilds are ready.

The error usually looks like this:

 info  rebuilding... (~ app/routes/_index.tsx)
 info  rebuilt (167ms)
 info  app server ready (9ms)
 info  hmr

 info  rebuilding... (~ app/routes/_index.tsx)
 info  rebuilt (291ms)
 info  rebuilding... (~ app/routes/_index.tsx)
 info  app server ready (37ms)
 info  hmr


/node_modules/@remix-run/server-runtime/dist/dev.js:26
      buildHash: build.assets.version
                              ^
TypeError: Cannot read properties of undefined (reading 'version')
    at broadcastDevReady (/node_modules/@remix-run/server-runtime/dist/dev.js:26:31)
    at FSWatcher.<anonymous> (file:///server.js:64:5)
 info  rebuilt (511ms)

I'll try to see if I can create a reproducible step with a simpler template.

Expected Behavior

Dev Server crashes gracefully.

Actual Behavior

Dev Server crash and force a manual restart.

@xHomu
Copy link
Contributor Author

xHomu commented Jul 22, 2023

Just happens occasionally.

image

@pcattori Happens most often editing a heavy page like this one if you want to use it to debug https://github.com/manawiki/core/blob/home-page/app/routes/_index/index.tsx

@pcattori
Copy link
Contributor

pcattori commented Jul 22, 2023

@xHomu could you patch if (build.assets === undefined) return; as the first line of broadcastDevReady in node_modules/@remix-run/server-runtime/dist/dev.js? I'm curious if just ignoring empty builds will fix the issue and not cause any rebuilds to be missed by the dev server.

You could do:

if (build.assets === undefined) {
  // console.log(JSON.stringify(build, undefined, 2)); // uncomment this line to debug  
  return;
}

and then you could try to see what builds that are missing assets look like

@ngbrown
Copy link
Contributor

ngbrown commented Jul 24, 2023

@xHomu This looks like it might be the same/similar as something I reported in #6831. I had a large server bundle size and would occasionally have file reading issues because the server was triggered by chokidar to re-read the bundle before the writing finished.

Does removing the createDevRequestHandler() handler option and removing --manual fix it (with the cost of the server restarting on every change). Or does adding the awaitWriteFinish option to chokidar.watch() fix it? That worked in my case.

@xHomu
Copy link
Contributor Author

xHomu commented Jul 24, 2023

@ngbrown restart on every change would be a big hit on the DX, so I'm gonna give the awaitWriteFinish option a try and see if it helps. Just .watch(BUILD_PATH, { ignoreInitial: true, awaitWriteFinish: true }) or do you have a specific setting for it that worked for you?

@ngbrown
Copy link
Contributor

ngbrown commented Jul 24, 2023

@xHomu The default timeout for awaitWriteFinish is 2 seconds, so I lowered the stabilityThreshold to 200 ms, and that was working for me. See the diff in #6831 (comment).

xHomu added a commit to manawiki/mana that referenced this issue Jul 24, 2023
@xHomu
Copy link
Contributor Author

xHomu commented Jul 24, 2023

@ngbrown no crushes in an afternoon after the change, a good sign!

   chokidar
      .watch(BUILD_PATH, {
         ignoreInitial: true,
         awaitWriteFinish: { stabilityThreshold: 200 },
      })
      ```

@xHomu
Copy link
Contributor Author

xHomu commented Jul 25, 2023

I made a PR #6922 to the Express template to include awaitWriteFinish. Is this enough to resolve #6831 and #6919, at least for Remix dev v2 server on Express?

@ngbrown
Copy link
Contributor

ngbrown commented Aug 8, 2023

@pcattori I thought of another way to handle this than using awaitWriteFinish on chokidar... Since chokidar in the server is only watching a single file, if the build saved to a temporary file, and then when saving was done, rename/move the file to the target file name, replacing the old one, it would be an almost atomic action, and reloading should take place without a hitch.

@pcattori
Copy link
Contributor

With #6772 , we are now writing metafile.server.json after writing the server build is finished. @xHomu do you want to try watching for that file instead of index.js to confirm if that fixes the issue?

@ngbrown agree that doing an atomic move is a good solution, but it adds some complexity to the compiler. Not a lot, but some, so trying to see if we can use metafile.server.json instead.

@pcattori pcattori reopened this Aug 15, 2023
@xHomu
Copy link
Contributor Author

xHomu commented Aug 15, 2023

Sure let me give that a shot today. Should I go ahead and close pr #6922 until we settle on best practices?

@pcattori
Copy link
Contributor

Sure let me give that a shot today. Should I go ahead and close pr #6922 until we settle on best practices?

Just made it into a Draft PR for now

xHomu referenced this issue in manawiki/mana Aug 16, 2023
@xHomu
Copy link
Contributor Author

xHomu commented Aug 16, 2023

@pcattori I pushed the WATCH_PATH change to mana.wiki and seems to be working at least as well as watching BUILD_PATH, will report back if we saw any instability manawiki/mana@2b81f4d#comments

so far so good

@58bits
Copy link

58bits commented Aug 17, 2023

I'm also seeing this. Will try the suggested fix above (watching metafile.server.json)

@xHomu
Copy link
Contributor Author

xHomu commented Aug 17, 2023

Mhm, experiencing some weird hmr not refreshing issues with one of the remix-v2-dev branches with metafile.server.json, looking into it. https://github.com/xHomu/remix-v2-server/tree/esm-server.js

@ngbrown
Copy link
Contributor

ngbrown commented Aug 17, 2023

Watching "./build/metafile.server.json" (and removing awaitWriteFinish) isn't working for me. The server re-imports, but on a manual refresh of the browser, the server is now trying to serve up the old missing chunks to the browser.

The HMR also seemed to stop working as the new chunks aren't loading.

I'm on Remix 1.19.2.

My reading of this is that the metadata is written out before the bytes in the outputFiles:

let compile = async () => {
let { outputFiles, metafile } = await compiler.rebuild();
writeMetafile(ctx, "metafile.server.json", metafile);
return outputFiles;
};

@xHomu
Copy link
Contributor Author

xHomu commented Aug 17, 2023

It's really strange, it works just fine on some of my repos, and not others.

✔️ mana.wiki (cjs/server.ts) https://github.com/manawiki/core
✔️ repay (cjs/server.ts) https://github.com/manawiki/repay

remix-v2-server:

My presumption that was this was particular to specific typescript/ esm combination turns out to be wrong, or else there's no reason why mana and repay works but the cjs/server.ts branch of remix-v2-server didn't.


@ngbrown Have you tried adding awaitWriteFinish back in and see if it do anything on your end? That seems to have helped, which indicates to me some weird race condition issue.

After adding awaitWriteFinish: { stabilityThreshold: 200 }, back in, all the branches hmr work as expected now.
@pcattori What do you think?

@ngbrown
Copy link
Contributor

ngbrown commented Aug 18, 2023

@xHomu Adding back in awaitWriteFinish and still watching metafile.server.json wouldn't prove anything because it would then be relying on the timing of 200 ms (or whatever the configuration is set to). for the index.js to finish being written. It would be mostly a coincidence if it worked.

@58bits
Copy link

58bits commented Aug 18, 2023

...not sure if this helps, but here are the file sizes of my remix build output...

image

This is under Windows 11 / WSL 2 / Ubuntu 22.04.3 LTS / Node.js v18.17.1

Watching metafile.server.js alone did not solve this for me, although I've not yet tried all suggestions above.

I still receive buildHash: build.assets.version undefined error fairly frequently.

However, this never happens under Mac OS (14" M1 MBP - Ventura 13.4.1 / Node.js v18.13.0)

@pcattori
Copy link
Contributor

My reading of this is that the metadata is written out before the bytes in the outputFiles:

let compile = async () => {
let { outputFiles, metafile } = await compiler.rebuild();
writeMetafile(ctx, "metafile.server.json", metafile);
return outputFiles;
};

Oh yea, good point. I forgot that we are manually writing the server build file later and not depending on esbuild to write it for us.

@pcattori
Copy link
Contributor

For now, let's rely on awaitWriteFinish. After v2 releases, I'll look into this some more.

@pcattori pcattori removed this from v2 Aug 28, 2023
@58bits
Copy link

58bits commented Aug 29, 2023

chokidar
      .watch(BUILD_PATH, {
         ignoreInitial: true,
         awaitWriteFinish: { stabilityThreshold: 200 },
      })

... solved this for me as well (and I was also seeing the SyntaxError: Unexpected end of input error described in #6831)

@pcattori
Copy link
Contributor

My new idea is to have a ./build/.versions file and to have the dev server write the version for the latest build as a newline at the end of that file after the server build has been completely written. Then the app server would watch the .versions file, grab the last line as the "expected version", and try to reimport the server build. If the server build's actual version matches the expected version we know we are in sync and can send a "ready" message to the dev server. If the versions don't match, try again: reread the last line of .versions, reimport the server build, and compare. Keep doing this until they match.

@pcattori
Copy link
Contributor

pcattori commented Aug 30, 2023

Ok version 2.0.0-pre.3 includes changes to write a version.txt file in the server build directory (not the public build directory) after the server build has been completely written.

Technically, there's still a chance of a race condition, but should be much rarer plus I have additional plans to fix that. But for now you should be able to watch version.txt for changes instead of directly watching the server build:

chokidar
- .watch(buildPath, { ignoreInitial: true })
+ .watch(path.resolve("build/version.txt"), { ignoreInitial: true })
  .on("add", handleServerUpdate)
  .on("change", handleServerUpdate);

@xHomu @ngbrown or anyone want to try it out?

@xHomu
Copy link
Contributor Author

xHomu commented Aug 30, 2023

Something came up atm, but I'll be able to give it a whirl tonight.

@xHomu
Copy link
Contributor Author

xHomu commented Aug 30, 2023

@pcattori happy to report back that all status looks green, and none of the repos I'm actively maintaining experience any of the previous downsides!

✔️ mana.wiki (cjs/server.ts) https://github.com/manawiki/core
✔️ repay (cjs/server.ts) https://github.com/manawiki/repay

remix-v2-server:

Also seems a tad faster too? Might just be happy thoughts.

Sample PR

@pcattori
Copy link
Contributor

pcattori commented Sep 7, 2023

After v2 releases, we can update the templates to watch for version.txt and then close this issue

xHomu added a commit to xHomu/weabdev that referenced this issue Sep 16, 2023
Update chokidar to resolve a rare race condition issue that resultant in dev server crashing. This PR remix-run/remix#7299 sets up a fix where we can watch the version.txt file for changes (rather than build/index.js directly) to avoid race condition.
kentcdodds pushed a commit to epicweb-dev/epic-stack that referenced this issue Sep 16, 2023
@pcattori
Copy link
Contributor

Fixed by #7469 and #7470

@chankruze
Copy link

It's still happening

image

"@remix-run/css-bundle": "^2.0.0",
"@remix-run/node": "^2.0.0",
"@remix-run/react": "^2.0.0",
"@remix-run/serve": "^2.0.0",
"@remix-run/dev": "^2.0.0",

@pcattori
Copy link
Contributor

pcattori commented Sep 21, 2023

@chankruze the fixes were merged after v2.0.0. they are available on nightly (npx upgrade-remix nightly) if you want to try them out. They'll be included in the next release.

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

Successfully merging a pull request may close this issue.

6 participants