fix: address PR review feedback

- Document single-handler assumption and late/extra response
  behavior on request()
- Shorten RejectAction/postUpdateQueue comment
- Add dispatchDirectly() to bypass the action queue for actions
  that need immediate processing (e.g. progress notifications)
- Use dispatchDirectly() for handler responses and nested requests
- Remove client-action queue bypass from dispatch(); all actions
  are now enqueued to preserve sequential ordering
- Extract sendResponseToClient() hook in DefaultGLSPServer

Author: martin-fleck-at

packages/server/src/common/actions/action-dispatcher.ts

@@ -43,6 +43,8 @@ export const ActionDispatcher = Symbol('ActionDispatcher');
 export interface ActionDispatcher {
     /**
      * Processes the given action by dispatching it to all registered handlers.
+     * Actions are enqueued to preserve sequential ordering. Response actions
+     * produced by handlers are dispatched directly via {@link dispatchDirectly}.
      *
      * @param action The action that should be dispatched.
      * @returns A promise indicating when the action processing is complete.
@@ -58,6 +60,18 @@ export interface ActionDispatcher {
     dispatchAll(actions: Action[]): Promise<void>;
     dispatchAll(...actions: Action[]): Promise<void>;
 
+    /**
+     * Dispatches an action directly, bypassing the action queue. Use this for actions that
+     * need to be processed immediately, e.g. progress notifications sent from inside a handler.
+     *
+     * Actions dispatched this way are not sequenced with other queued actions. Callers are
+     * responsible for ensuring that concurrent execution is safe.
+     *
+     * @param action The action to dispatch directly.
+     * @returns A promise indicating when the action processing is complete.
+     */
+    dispatchDirectly(action: Action): Promise<void>;
+
     /**
      * Processes all given actions, by dispatching them to the corresponding handlers, after the next model update.
      * The given actions are queued until the next model update cycle has been completed i.e. an
@@ -74,9 +88,12 @@ export interface ActionDispatcher {
      * _not_ passed to the registered action handlers. Instead, it is the responsibility of the
      * caller of this method to handle the response properly.
      *
-     * If the request's `kind` is registered in `ClientActionKinds`, it is forwarded to the client
-     * via {@link ClientActionForwarder}. Otherwise it is dispatched locally through server-side
-     * handlers.
+     * The request is dispatched directly (bypassing the queue). If its `kind` is registered in
+     * `ClientActionKinds`, it is forwarded to the client via {@link ClientActionForwarder}.
+     * If server-side handlers are registered, they are also executed.
+     *
+     * Only the first matching response resolves the request. Any additional or late responses
+     * are dispatched as normal actions.
      *
      * The promise waits indefinitely until a response arrives or the dispatcher is disposed.
      * Use {@link requestUntil} if a timeout is needed.
@@ -137,20 +154,16 @@ export class DefaultActionDispatcher implements ActionDispatcher, Disposable {
     }
 
     dispatch(action: Action): Promise<void> {
-        // Fast-path: resolve pending requests before the queue to prevent deadlock
-        // when request() is awaited inside a queued handler and the response
-        // arrives via process() -> dispatch().
         if (this.interceptPendingResponse(action)) {
             return Promise.resolve();
         }
-
-        // Dont queue actions that are just delegated to the client
-        if (this.clientActionForwarder.shouldForwardToClient(action)) {
-            return this.doDispatch(action);
-        }
         return this.actionQueue.enqueue(() => this.doDispatch(action));
     }
 
+    dispatchDirectly(action: Action): Promise<void> {
+        return this.doDispatch(action);
+    }
+
     protected async doDispatch(action: Action): Promise<void> {
         // Intercept responses to pending requests produced by local handlers
         // (via dispatchResponses). This enables server→server requests.
@@ -183,12 +196,16 @@ export class DefaultActionDispatcher implements ActionDispatcher, Disposable {
         return responseActions.map(action => respond(request, action));
     }
 
+    /**
+     * Dispatches response actions produced by handlers. Responses are dispatched directly
+     * (bypassing the queue) but sequenced relative to each other via an internal response queue.
+     */
     protected dispatchResponses(actions: Action[]): Promise<void> {
         if (actions.length === 0) {
             return Promise.resolve();
         }
         const responseQueue = new PromiseQueue();
-        const responses = actions.map(action => responseQueue.enqueue(() => this.doDispatch(action)));
+        const responses = actions.map(action => responseQueue.enqueue(() => this.dispatchDirectly(action)));
         return Promise.all(responses).then(() => Promise.resolve());
     }
 
@@ -254,12 +271,10 @@ export class DefaultActionDispatcher implements ActionDispatcher, Disposable {
             this.timeouts.set(action.requestId, timeout);
         }
 
-        // When the queue is busy (request() called from inside a handler),
-        // bypass it via doDispatch() to avoid deadlock.
-        // When idle, go through dispatch() to preserve action ordering.
-        const dispatchPromise = this.actionQueue.isBusy
-            ? this.doDispatch(action)
-            : this.dispatch(action);
+        // Always dispatch directly to avoid deadlock when request() is called
+        // from inside a queued handler. The response is intercepted before
+        // normal dispatch, so queue ordering is not affected.
+        const dispatchPromise = this.dispatchDirectly(action);
 
         dispatchPromise.catch(error => {
             if (this.pendingRequests.delete(action.requestId)) {
@@ -301,8 +316,8 @@ export class DefaultActionDispatcher implements ActionDispatcher, Disposable {
      * by {@link ClientActionForwarder}. If no stale entry exists, the `responseId` is left intact
      * for normal forwarding.
      *
-     * Called from both `dispatch()` (for responses arriving from the client) and `doDispatch()`
-     * (for responses produced by local handlers).
+     * Called from `dispatch()` (for responses arriving from the client, before the queue) and
+     * from `doDispatch()` (for responses produced by local handlers via `dispatchDirectly`).
      */
     protected interceptPendingResponse(action: Action): boolean {
         if (!ResponseAction.hasValidResponseId(action)) {
@@ -317,16 +332,17 @@ export class DefaultActionDispatcher implements ActionDispatcher, Disposable {
                 clearTimeout(timeout);
                 this.timeouts.delete(action.responseId);
             }
-            // Drain post-update actions before resolving — in the intercept path
-            // there's no responses array, so dispatch them directly.
+            // Drain post-update actions. A RejectAction won't trigger a drain,
+            // so pending post-update actions remain queued until the next
+            // successful model update.
             const postUpdateActions = this.drainPostUpdateQueue(action);
             if (RejectAction.is(action)) {
                 deferred.reject(new Error(`${action.message}${action.detail ? ': ' + action.detail : ''}`));
             } else {
                 deferred.resolve(action);
             }
             if (postUpdateActions.length > 0) {
-                this.dispatchAll(postUpdateActions);
+                this.dispatchResponses(postUpdateActions);
             }
             return true;
         }

packages/server/src/common/features/model/model-submission-handler.ts

@@ -180,7 +180,7 @@ export class ModelSubmissionHandler {
     }
 
     protected async performLiveValidation(validator: ModelValidator): Promise<void> {
-        this.actionDispatcher.dispatch(StatusAction.create('Validate Model...'));
+        this.actionDispatcher.dispatchDirectly(StatusAction.create('Validate Model...'));
         const markers = await validator.validate([this.modelState.root], MarkersReason.LIVE);
         return this.actionDispatcher.dispatchAll(
             SetMarkersAction.create(markers, { reason: MarkersReason.LIVE }),

packages/server/src/common/features/model/request-model-action-handler.ts

@@ -81,12 +81,12 @@ export class RequestModelActionHandler implements ActionHandler {
     }
 
     protected reportModelLoading(message: string): ProgressMonitor | undefined {
-        this.actionDispatcher.dispatch(StatusAction.create(message, { severity: 'INFO' }));
+        this.actionDispatcher.dispatchDirectly(StatusAction.create(message, { severity: 'INFO' }));
         return this.progressService.start(message);
     }
 
     protected reportModelLoadingFinished(monitor?: ProgressMonitor): void {
-        this.actionDispatcher.dispatch(StatusAction.create('', { severity: 'NONE' }));
+        this.actionDispatcher.dispatchDirectly(StatusAction.create('', { severity: 'NONE' }));
         monitor?.end();
     }
 

packages/server/src/common/features/progress/progress-service.ts

@@ -65,10 +65,10 @@ export class DefaultProgressService implements ProgressService {
 
     start(title: string, options?: ProgressOptions): ProgressMonitor {
         const progressId = uuid.v4();
-        this.actionDispatcher.dispatch(StartProgressAction.create({ progressId, title, ...options }));
+        this.actionDispatcher.dispatchDirectly(StartProgressAction.create({ progressId, title, ...options }));
         return {
-            update: updateOptions => this.actionDispatcher.dispatch(UpdateProgressAction.create(progressId, updateOptions)),
-            end: () => this.actionDispatcher.dispatch(EndProgressAction.create(progressId))
+            update: updateOptions => this.actionDispatcher.dispatchDirectly(UpdateProgressAction.create(progressId, updateOptions)),
+            end: () => this.actionDispatcher.dispatchDirectly(EndProgressAction.create(progressId))
         };
     }
 }

packages/server/src/common/protocol/glsp-server.ts

@@ -171,7 +171,7 @@ export class DefaultGLSPServer implements GLSPServer {
                 ? await clientSession.actionDispatcher.requestUntil(action, action.timeout, true)
                 : await clientSession.actionDispatcher.request(action);
             if (response) {
-                this.sendToClient({ clientId, action: response });
+                this.sendResponseToClient(clientId, response);
             }
         } catch (error) {
             const detail = error instanceof GLSPServerError ? error.cause?.toString?.() : error?.toString?.();
@@ -180,10 +180,14 @@ export class DefaultGLSPServer implements GLSPServer {
                 `Failed to handle request '${action.kind}' (${action.requestId})`,
                 { responseId: action.requestId, detail }
             );
-            this.sendToClient({ clientId, action: reject });
+            this.sendResponseToClient(clientId, reject);
         }
     }
 
+    protected sendResponseToClient(clientId: string, response: ResponseAction): void {
+        this.sendToClient({ clientId, action: response });
+    }
+
     protected handleProcessError(message: ActionMessage, reason: any): void | PromiseLike<void> {
         let errorMsg = `Could not process action: '${message.action.kind}`;
         this.logger.error(errorMsg);

packages/server/src/common/test/mock-util.ts

@@ -135,6 +135,10 @@ export class StubActionDispatcher implements ActionDispatcher {
         return Promise.resolve();
     }
 
+    dispatchDirectly(action: Action): Promise<void> {
+        return Promise.resolve();
+    }
+
     dispatchAll(...actions: MaybeArray<Action>[]): Promise<void> {
         return Promise.resolve();
     }