DialogQueue.enqueue is not robust to a synchronous settle (latent FIFO/queue corruption) #29

New issue

Open

opened 2026-07-04 12:11:10 +00:00 by momsse · 0 comments

momsse commented

2026-07-04 12:11:10 +00:00

Owner

Migré depuis viziertronic/octant#72 — ouvert le 2026-06-23 par @momsse.

Summary

DialogQueue.enqueue (apps/backoffice/src/lib/client/common/components/dialog-queue.ts) has a latent ordering bug: if a makeRequest callback ever invokes settle synchronously (before returning the Request), the queue state is corrupted. It is not reachable with today's callers, so we are deliberately deferring the fix — this issue records the context and what a proper fix needs.

The defect

enqueue<Result>(
  makeRequest: (settle: (result: Result) => void) => Request,
): Promise<Result> {
  return new Promise((resolve) => {
    let requestHasSettled = false
    const request = makeRequest((result) => {     // ← if settle() is called synchronously here…
      if (requestHasSettled) return
      requestHasSettled = true
      this.activateNextRequest()                   // …this runs while this.activeRequest is still null
      resolve(result)
    })
    if (this.activeRequest === null) {             // …and this guard only runs after makeRequest returns
      this.activeRequest = request
      this.onActiveRequestChange(request)
      return
    }
    this.pendingRequests = [...this.pendingRequests, request]
  })
}

If settle fires synchronously inside makeRequest, activateNextRequest() runs before this.activeRequest is assigned: it shifts from an empty pendingRequests, sets activeRequest = null, and notifies null. Control then returns to the this.activeRequest === null guard, which is true, so the already-settled request is installed as the active dialog and shown — permanently, since requestHasSettled blocks its resolve callback from ever firing again.

Why we are not fixing it now

Not reachable today. The only caller, DialogsProvider (dialogs.tsx), always builds and returns the Request object synchronously and never calls settle from inside makeRequest (settle is wired to user interaction / dialog dismissal). So the corrupting path cannot be hit by current code.
The obvious quick fix is wrong. A review suggested simply reordering to resolve(result) before activateNextRequest(). That regresses a real, supported scenario: a .then(() => dialogs.confirm(…)) chain. Resolving first runs the chained enqueue (microtask) before the queue advances, so the chained dialog is appended to pendingRequests and then a different pending request is promoted — breaking the FIFO ordering that currently holds. So this is not a one-line reorder; it needs a deliberate redesign + tests.

What a proper fix needs

Make enqueue robust to a synchronous settle: decide activation based on whether the settling request is the active one, not on a guard that runs after makeRequest returns. Options: assign activeRequest/enqueue before invoking makeRequest, or have the settle callback no-op its activateNextRequest() when the request was never activated.
Preserve FIFO for chained dialogs (.then(() => dialogs.confirm())) — the new active dialog must be promoted before the resolve continuation enqueues the next one (current behaviour), or chained requests must be ordered deterministically by design.
Add tests to dialog-queue.test.ts covering: (a) synchronous settle inside makeRequest, and (b) a chained confirm-in-.then preserving order — both currently missing.

Context

Surfaced during review of PR #65 (split/13-backoffice). Latent only; no user-visible impact with the current DialogsProvider.

> _Migré depuis [viziertronic/octant#72](https://github.com/viziertronic/octant/issues/72) — ouvert le 2026-06-23 par @momsse._ ## Summary `DialogQueue.enqueue` (`apps/backoffice/src/lib/client/common/components/dialog-queue.ts`) has a **latent ordering bug**: if a `makeRequest` callback ever invokes `settle` **synchronously** (before returning the `Request`), the queue state is corrupted. It is not reachable with today's callers, so we are deliberately deferring the fix — this issue records the context and what a proper fix needs. ## The defect ```ts enqueue<Result>( makeRequest: (settle: (result: Result) => void) => Request, ): Promise<Result> { return new Promise((resolve) => { let requestHasSettled = false const request = makeRequest((result) => { // ← if settle() is called synchronously here… if (requestHasSettled) return requestHasSettled = true this.activateNextRequest() // …this runs while this.activeRequest is still null resolve(result) }) if (this.activeRequest === null) { // …and this guard only runs after makeRequest returns this.activeRequest = request this.onActiveRequestChange(request) return } this.pendingRequests = [...this.pendingRequests, request] }) } ``` If `settle` fires synchronously inside `makeRequest`, `activateNextRequest()` runs before `this.activeRequest` is assigned: it shifts from an empty `pendingRequests`, sets `activeRequest = null`, and notifies `null`. Control then returns to the `this.activeRequest === null` guard, which is `true`, so the **already-settled** request is installed as the active dialog and shown — permanently, since `requestHasSettled` blocks its resolve callback from ever firing again. ## Why we are not fixing it now 1. **Not reachable today.** The only caller, `DialogsProvider` (`dialogs.tsx`), always builds and returns the `Request` object synchronously and never calls `settle` from inside `makeRequest` (settle is wired to user interaction / dialog dismissal). So the corrupting path cannot be hit by current code. 2. **The obvious quick fix is wrong.** A review suggested simply reordering to `resolve(result)` before `activateNextRequest()`. That regresses a real, supported scenario: a `.then(() => dialogs.confirm(…))` chain. Resolving first runs the chained `enqueue` (microtask) **before** the queue advances, so the chained dialog is appended to `pendingRequests` and then a *different* pending request is promoted — breaking the FIFO ordering that currently holds. So this is not a one-line reorder; it needs a deliberate redesign + tests. ## What a proper fix needs - Make `enqueue` robust to a synchronous `settle`: decide activation based on **whether the settling request is the active one**, not on a guard that runs after `makeRequest` returns. Options: assign `activeRequest`/enqueue *before* invoking `makeRequest`, or have the settle callback no-op its `activateNextRequest()` when the request was never activated. - Preserve FIFO for chained dialogs (`.then(() => dialogs.confirm())`) — the new active dialog must be promoted before the resolve continuation enqueues the next one (current behaviour), or chained requests must be ordered deterministically by design. - Add tests to `dialog-queue.test.ts` covering: (a) synchronous settle inside `makeRequest`, and (b) a chained `confirm`-in-`.then` preserving order — both currently missing. ## Context - Surfaced during review of PR #65 (`split/13-backoffice`). Latent only; no user-visible impact with the current `DialogsProvider`.