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

More robust process handling for apps #418

Merged
merged 4 commits into from
Oct 23, 2024
Merged

More robust process handling for apps #418

merged 4 commits into from
Oct 23, 2024

Conversation

benjreinhart
Copy link
Contributor

@benjreinhart benjreinhart commented Oct 23, 2024

This PR:

  • Stores all app-related processes (npm install, vite dev server) in a more robust map. We can use this to synchronize calls and ensure we only have one process for each running at any given time.
  • Fixes the following bug: Preview server doesn't run because no node_modules yet. When this happens, the child process emits an error event (containing an error with code 'ENOENT') but no exit event. Since it doesn't emit exit, we don't call the onExit handler and therefore never remove the process from the process map. Subsequent calls to start the server won't work because the process map thinks there's an existing process for the vite server, but that process is dead and never ran. Because it never ran, the port is null and thus we see locahost:null as the URL for the dev server.

TODO

The following still needs to be addressed but will do so in subsequent PR(s)

  • We should either A) remove the npm install calls in apps.mts and let the client handle all installing or B) kick off npm install when the app is created on the backend and hook into the process map introduced in this PR ensuring we synchronize npm installs (sketch for this approach is here: npm install should always include sockets messages #412)
  • The client is a bit buggy with npm install calls, sometimes when the npm install toast prompts me, I install and after install finishes (even successfully), i immediately get prompted to install again. I can reproduce this by allowing non AI apps to be created (no prompt in create modal) and also remove the install call in apps.mts.
  • I occasionally still see npm install failing. When this happens, the app gets into a bad state. With this PR, if I re-run npm install on the client, the dev server and other things seem to start working again. But we need to figure out why npm installs fail sometimes

@benjreinhart benjreinhart merged commit d94d5cf into main Oct 23, 2024
2 checks passed
@benjreinhart benjreinhart deleted the processes branch October 23, 2024 18:50
@benjreinhart
Copy link
Contributor Author

benjreinhart commented Oct 23, 2024

Here is one instance of npm install failing:

792 info run [email protected] postinstall node_modules/esbuild node install.js
793 info run [email protected] postinstall { code: 1, signal: null }
794 verbose stack Error: command failed
794 verbose stack     at ChildProcess.<anonymous> (/Users/ben/.nvm/versions/node/v22.1.0/lib/node_modules/npm/node_modules/@npmcli/promise-spawn/lib/index.js:53:27)
794 verbose stack     at ChildProcess.emit (node:events:520:28)
794 verbose stack     at maybeClose (node:internal/child_process:1105:16)
794 verbose stack     at ChildProcess._handle.onexit (node:internal/child_process:305:5)
795 verbose pkgid [email protected]
796 verbose cwd /Users/ben/.srcbook/apps/ucubf5g149koikd6a5ohthnvnc
797 verbose Darwin 23.5.0
798 verbose node v22.1.0
799 verbose npm  v10.7.0
800 error code 1
801 error path /Users/ben/.srcbook/apps/ucubf5g149koikd6a5ohthnvnc/node_modules/esbuild
802 error command failed
803 error command sh -c node install.js
804 error node:internal/errors:984
804 error   const err = new Error(message);
804 error               ^
804 error
804 error Error: Command failed: /Users/ben/.nvm/versions/node/v22.1.0/bin/node /Users/ben/.srcbook/apps/ucubf5g149koikd6a5ohthnvnc/node_modules/esbuild/bin/esbuild --version
804 error /Users/ben/.srcbook/apps/ucubf5g149koikd6a5ohthnvnc/node_modules/esbuild/bin/esbuild:1
804 error ����^L
804 error
804 error
804 error SyntaxError: Invalid or unexpected token
804 error     at wrapSafe (node:internal/modules/cjs/loader:1389:18)
804 error     at Module._compile (node:internal/modules/cjs/loader:1425:20)
804 error     at Module._extensions..js (node:internal/modules/cjs/loader:1564:10)
804 error     at Module.load (node:internal/modules/cjs/loader:1287:32)
804 error     at Module._load (node:internal/modules/cjs/loader:1103:12)
804 error     at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:168:12)
804 error     at node:internal/main/run_main_module:30:49
804 error
804 error Node.js v22.1.0
804 error
804 error     at genericNodeError (node:internal/errors:984:15)
804 error     at wrappedFn (node:internal/errors:538:14)
804 error     at checkExecSyncError (node:child_process:889:11)
804 error     at Object.execFileSync (node:child_process:925:15)
804 error     at validateBinaryVersion (/Users/ben/.srcbook/apps/ucubf5g149koikd6a5ohthnvnc/node_modules/esbuild/install.js:99:28)
804 error     at /Users/ben/.srcbook/apps/ucubf5g149koikd6a5ohthnvnc/node_modules/esbuild/install.js:281:5 {
804 error   status: 1,
804 error   signal: null,
804 error   output: [
804 error     null,
804 error     Buffer(0) [Uint8Array] [],
804 error     Buffer(623) [Uint8Array] [
804 error        47,  85, 115, 101, 114, 115,  47,  98, 101, 110,  47,  46,
804 error       115, 114,  99,  98, 111, 111, 107,  47,  97, 112, 112, 115,
804 error        47, 117,  99, 117,  98, 102,  53, 103,  49,  52,  57, 107,
804 error       111, 105, 107, 100,  54,  97,  53, 111, 104, 116, 104, 110,
804 error       118, 110,  99,  47, 110, 111, 100, 101,  95, 109, 111, 100,
804 error       117, 108, 101, 115,  47, 101, 115,  98, 117, 105, 108, 100,
804 error        47,  98, 105, 110,  47, 101, 115,  98, 117, 105, 108, 100,
804 error        58,  49,  10, 239, 191, 189, 239, 191, 189, 239, 191, 189,
804 error       239, 191, 189,  12,
804 error       ... 523 more items
804 error     ]
804 error   ],
804 error   pid: 98013,
804 error   stdout: Buffer(0) [Uint8Array] [],
804 error   stderr: Buffer(623) [Uint8Array] [
804 error      47,  85, 115, 101, 114, 115,  47,  98, 101, 110,  47,  46,
804 error     115, 114,  99,  98, 111, 111, 107,  47,  97, 112, 112, 115,
804 error      47, 117,  99, 117,  98, 102,  53, 103,  49,  52,  57, 107,
804 error     111, 105, 107, 100,  54,  97,  53, 111, 104, 116, 104, 110,
804 error     118, 110,  99,  47, 110, 111, 100, 101,  95, 109, 111, 100,
804 error     117, 108, 101, 115,  47, 101, 115,  98, 117, 105, 108, 100,
804 error      47,  98, 105, 110,  47, 101, 115,  98, 117, 105, 108, 100,
804 error      58,  49,  10, 239, 191, 189, 239, 191, 189, 239, 191, 189,
804 error     239, 191, 189,  12,
804 error     ... 523 more items
804 error   ]
804 error }
804 error
804 error Node.js v22.1.0
805 verbose exit 1

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

Successfully merging this pull request may close these issues.

1 participant