Ensure socket messages are always sent for all npm installs #419

1egoman · 2024-10-23T20:18:34Z

At a high level, this change should address the first two points in #418.

We had discussed previously the notion of whether making all npm installs client initiated would be an easy way to fix the first item. However, after getting more into this work, I opted to not do it this way. This was due to a mixture of not being able to come up with an elegant way to pass the state from the creation step to the actual apps interface to invoke the install, and that it seemed like it's likely folks could have multiple instances of a srcbook open and generally getting in the habit of broadcasting events so they all remain in sync no matter which instance caused an install to happen seemed like a good idea.

In more detail, this change:

Ensures that for every npm install invocation made by the app, all relevant socket events are broadcasted to all connected clients on the given channel. Previously, some calls sent everything, some sent a few events, and some sent none. At least in my anecdotal observations, a lot of complex and hard to reason about situation were coming up due to the server and client's state not being synced.
Adds a new message that gets sent when a npm install starts running. This means that all connected app instances will show a "loading" state, even if the npm install in question was initiated on the server or in another app instance.
When a user joins a channel initially, send a message to them giving them an initial npm install status, so that if an install is in flight, the newly connected app can display this.

…nishes

… are properly sequenced, and that the process gets deleted at the end

…ws about in flight npm installs

… install status

…son context up to date

…er dependencies are installed Previously, events were sent bespoke in each implementation. Many missed events and that led to cases where the client often had no idea the state of the server. Now, always send all events always.

1egoman · 2024-10-23T20:20:33Z

packages/api/apps/app.mts

 import { CreateAppSchemaType, CreateAppWithAiSchemaType } from './schemas.mjs';
 import { asc, desc, eq } from 'drizzle-orm';
-import { npmInstall } from '../exec.mjs';
+import { npmInstall, waitForProcessToComplete } from './processes.mjs';
 import { generateApp } from '../ai/generate.mjs';
 import { toValidPackageName } from '../apps/utils.mjs';


It seems like this npmInstall was missed during the migration previously - so I converted this to use the same common process-managed interface as everything else. In theory, this means that two npm install commands could still run in parallel and I want to just make 100% sure that will never happen.

This was intentionally not done in #418 and left as a todo item. I didn't do it in that PR because I wasn't sure if I was going to delete this code entirely (so only allow the client to initiate installs) or let both initiate installs. We opted for the latter here, though I think it's probably better for now make this all client driven. Doing so requires reworking the create flow though, so I think that can be done in a separate PR.

1egoman · 2024-10-23T20:21:04Z

packages/api/apps/app.mts

  const packagesToInstall = getPackagesToInstall(plan);

  if (packagesToInstall.length > 0) {
+    await waitForProcessToComplete(firstNpmInstallProcess);
+
    console.log('installing packages', packagesToInstall);
-    npmInstall({
-      cwd: pathToApp(app.externalId),
+    npmInstall(app.externalId, {


I introduced this waitForProcessToComplete function here to ensure that these npm installs during an ai app creation always happen sequentially. In practice, almost always this function should end up being a noop.

Instead of waitForProcessToComplete, a nicer API might be to have a wrapper around npmInstall that returns a promise so that callers like these can just await the install call.

+1 we never want two concurrent ones, so that guard can be abstracted away

1egoman · 2024-10-23T20:24:02Z

packages/api/apps/processes.mts

 export function npmInstall(
  appId: string,
-  options: Omit<Parameters<typeof execNpmInstall>[0], 'cwd'>,
+  options: Omit<Partial<Parameters<typeof execNpmInstall>[0]>, 'cwd'> & { onStart?: () => void },
 ) {
-  if (!processes.has(appId, 'npm:install')) {
-    processes.set(appId, {
-      type: 'npm:install',
-      process: execNpmInstall({ cwd: pathToApp(appId), ...options }),
-    });
+  const runningProcess = processes.get(appId, 'npm:install');
+  if (runningProcess) {
+    return runningProcess;
+  }
+
+  wss.broadcast(`app:${appId}`, 'deps:install:status', { status: 'installing' });
+  if (options.onStart) {
+    options.onStart();
  }

-  return processes.get(appId, 'npm:install');
+  const newlyStartedProcess: NpmInstallProcessType = {
+    type: 'npm:install',
+    process: execNpmInstall({
+      ...options,
+
+      cwd: pathToApp(appId),
+      stdout: (data) => {
+        wss.broadcast(`app:${appId}`, 'deps:install:log', {
+          log: { type: 'stdout', data: data.toString('utf8') },
+        });
+
+        if (options.stdout) {
+          options.stdout(data);
+        }
+      },
+      stderr: (data) => {
+        wss.broadcast(`app:${appId}`, 'deps:install:log', {
+          log: { type: 'stderr', data: data.toString('utf8') },
+        });
+
+        if (options.stderr) {
+          options.stderr(data);
+        }
+      },
+      onExit: (code) => {
+        // We must clean up this process so that we can run npm install again
+        deleteAppProcess(appId, 'npm:install');
+
+        wss.broadcast(`app:${appId}`, 'deps:install:status', {
+          status: code === 0 ? 'complete' : 'failed',
+          code,
+        });
+
+        if (code === 0) {
+          wss.broadcast(`app:${appId}`, 'deps:status:response', {
+            nodeModulesExists: true,
+          });
+        }
+      },
+    }),
+  };
+  processes.set(appId, newlyStartedProcess);
+
+  return newlyStartedProcess;
 }


This might overload this process abstraction a bit, but I moved all the socket message emitting logic here - this is what makes sure these always get emitted for all cases where npm install is run. Previously, implementations emitted socket messages piecemeal which meant sometimes important messages in certain situations were missed.

I could potentially make another level of abstraction here if desired, though I don't think in practice there would be a reason one would want to call npmInstall without the socket logic associated.

1egoman · 2024-10-23T20:24:43Z

packages/api/server/channels/app.mts

+    .on('file:updated', FileUpdatedPayloadSchema, onFileUpdated)
+    .onJoin((topic, conn) => {
+      // FIXME: there's almost certainly a better way to get the app id than this?
+      const appExternalId = topic.replace(/^app:/, '');
+
+      // When connecting, send back info about an in flight npm install if one exists
+      const npmInstallProcess = getAppProcess(appExternalId, "npm:install");
+      if (npmInstallProcess) {
+        conn.reply(`app:${appExternalId}`, 'deps:install:status', { status: 'installing' });
+      }
+    });


When a client joins the channel, send a message letting the client know if there is a npm install in progress.

benjreinhart

Going to approve and we can merge this now but I think we should follow up and remove these backend npm install calls in favor of the client. There may be good reasons to kick off npm installs from BE code in the future, but right now I can't think of those reasons and it seems simpler to implement the following flow:

create app -> immediate redirect to client -> client assumes 'normal' flow, checks for deps, installs if not there, etc.

benjreinhart · 2024-10-23T21:51:43Z

packages/api/server/ws-client.mts

@@ -214,7 +214,7 @@ export default class WebSocketServer {
    if (event === 'subscribe') {
      conn.subscriptions.push(topic);
      conn.reply(topic, 'subscribed', { id: payload.id });
-      channel.onJoinCallback(topic, conn.socket);
+      channel.onJoinCallback(topic, conn);


The special onJoinCallback was implemented as a quick hack before #332. However, now that #332 is in place, we should make the callback signature the same as all other handlers:

+type HandlerType = ( + payload: Record<string, any>, + context: MessageContextType, + conn: ConnectionContextType, +) => void; + /** * Channel is responsible for dispatching incoming messages for a given topic. * @@ -52,15 +58,11 @@ export class Channel { string, { schema: z.ZodTypeAny; - handler: ( - payload: Record<string, any>, - context: MessageContextType, - conn: ConnectionContextType, - ) => void; + handler: HandlerType; } > = {}; - onJoinCallback: (topic: string, ws: WebSocket) => void = () => {}; + onJoinCallback: HandlerType = () => {}; constructor(topic: string) { this.topic = topic; @@ -130,7 +132,7 @@ export class Channel { return this; } - onJoin(callback: (topic: string, ws: WebSocket) => void) { + onJoin(callback: HandlerType) { this.onJoinCallback = callback; return this; } @@ -214,7 +216,11 @@ export default class WebSocketServer { if (event === 'subscribe') { conn.subscriptions.push(topic); conn.reply(topic, 'subscribed', { id: payload.id }); - channel.onJoinCallback(topic, conn.socket); + channel.onJoinCallback( + payload, + { topic: match.topic, event: event, params: match.params }, + conn, + ); return; }

The code in server/channels/app.mts can then do:

.onJoin((_payload, context, conn) => { const appExternalId = (context as AppContextType).params.appId; // ... });

benjreinhart · 2024-10-23T21:55:28Z

packages/api/apps/app.mts

  const packagesToInstall = getPackagesToInstall(plan);

  if (packagesToInstall.length > 0) {
+    await waitForProcessToComplete(firstNpmInstallProcess);
+
    console.log('installing packages', packagesToInstall);
-    npmInstall({
-      cwd: pathToApp(app.externalId),
+    npmInstall(app.externalId, {


Instead of waitForProcessToComplete, a nicer API might be to have a wrapper around npmInstall that returns a promise so that callers like these can just await the install call.

…mplete

…r handler functions

1egoman added 6 commits October 23, 2024 16:07

feat: hide the installation toast when a server driven npm install fi…

4eda8e7

…nishes

feat: ensure the first and second npm install when creating an ai app…

3219380

… are properly sequenced, and that the process gets deleted at the end

feat: send more websocket messages to make sure the client always kno…

9bf94b4

…ws about in flight npm installs

feat: send a socket message on connection that relays the current npm…

df2229c

… install status

feat: always listen for deps:status messages and keep the usePackageJ…

fa9eaeb

…son context up to date

1egoman commented Oct 23, 2024

View reviewed changes

1egoman added 3 commits October 23, 2024 16:25

fix: add missed onExit handler

9fce181

fix: run npm run format

35728fd

fix: address linter warning

0a70c52

1egoman marked this pull request as ready for review October 23, 2024 20:50

benjreinhart approved these changes Oct 23, 2024

View reviewed changes

1egoman added 2 commits October 24, 2024 14:03

feat: wrap all returns coming out of npmInstall in waitForProcessToCo…

75dfe0d

…mplete

fix: modify wss.onJoin to take the standard set of parameters as othe…

d60ce72

…r handler functions

1egoman merged commit 48f0b91 into main Oct 24, 2024
2 checks passed

1egoman deleted the npm-install-socket-messages branch October 24, 2024 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure socket messages are always sent for all npm installs #419

Ensure socket messages are always sent for all npm installs #419

1egoman commented Oct 23, 2024 •

edited

Loading

1egoman Oct 23, 2024 •

edited

Loading

benjreinhart Oct 23, 2024

1egoman Oct 23, 2024 •

edited

Loading

benjreinhart Oct 23, 2024

nichochar Oct 24, 2024

1egoman Oct 23, 2024 •

edited

Loading

1egoman Oct 23, 2024

benjreinhart left a comment

benjreinhart Oct 23, 2024

benjreinhart Oct 23, 2024

Ensure socket messages are always sent for all npm installs #419

Ensure socket messages are always sent for all npm installs #419

Conversation

1egoman commented Oct 23, 2024 • edited Loading

1egoman Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

benjreinhart Oct 23, 2024

Choose a reason for hiding this comment

1egoman Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

benjreinhart Oct 23, 2024

Choose a reason for hiding this comment

nichochar Oct 24, 2024

Choose a reason for hiding this comment

1egoman Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

1egoman Oct 23, 2024

Choose a reason for hiding this comment

benjreinhart left a comment

Choose a reason for hiding this comment

benjreinhart Oct 23, 2024

Choose a reason for hiding this comment

benjreinhart Oct 23, 2024

Choose a reason for hiding this comment

1egoman commented Oct 23, 2024 •

edited

Loading

1egoman Oct 23, 2024 •

edited

Loading

1egoman Oct 23, 2024 •

edited

Loading

1egoman Oct 23, 2024 •

edited

Loading