Harden the task queue against synchronously-throwing tasks - #749
Open
titouanmathis wants to merge 6 commits into
Open
Harden the task queue against synchronously-throwing tasks#749titouanmathis wants to merge 6 commits into
titouanmathis wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #749 +/- ##
==========================================
- Coverage 97.26% 97.20% -0.06%
==========================================
Files 169 169
Lines 4128 4156 +28
Branches 1145 1148 +3
==========================================
+ Hits 4015 4040 +25
- Misses 102 105 +3
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Export Size@studiometa/js-toolkit
Unchanged@studiometa/js-toolkit
|
Merging this PR will degrade performance by 31.52%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | all transforms |
299.5 µs | 437.4 µs | -31.52% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/queue-task-throw-isolation (5d6ec2f) with main (a67c3cf)
titouanmathis
added a commit
that referenced
this pull request
Aug 1, 2026
Address the review of #749: resolving the `add()` promise with `undefined` on a synchronous throw was wrong on two counts. It was inconsistent — `await queue.add(() => { throw e })` fulfilled while `await queue.add(async () => { throw e })` rejected, so a caller doing rollback in `catch` behaved differently solely because the task was declared `async`. And it lied at the lifecycle boundary — a synchronous throw in a queued `$mount` task made the internal `Promise.all` look successful, so the component set `$isMounted = true`, emitted `after-mounted` and resolved as if init had completed, silently swallowing an initialization failure. Make failures consistent and honest instead of papering over them: - `Queue.add()` now rejects with the task's error on a synchronous throw, exactly like an async task returning a rejected promise. The rejection is the single surfacing channel; the wrapper no longer re-throws, so `run()`'s `queueMicrotask` does not double-report. `run()` keeps its try/catch purely as defence in depth. - Fire-and-forget call sites attach an explicit `.catch()` routing the error to a log (`reportQueuedTaskError`) so a rejection never becomes an unhandled rejection: the two `addToQueue` calls in `mutationCallback` and the async-child `.then()` branch in `ChildrenManager`. - The awaited lifecycle boundaries in `Base.ts` (`$mount`, `$update`, `$destroy`, `$terminate`) wrap their queued work in try/catch: a failed task no longer falsely reports success (`$mount` leaves the component honestly unmounted and does not emit `after-mounted`; `$destroy` does not emit `after-destroyed`), surfaces the error through the log, and resolves instead of rejecting so fire-and-forget callers do not produce unhandled rejections. Also fixes two minor findings from the same review: - `scheduleFlush()` reset the `isScheduled` flag in a `finally`-like path so a throwing custom scheduler no longer wedges the queue permanently. - The "same batch" regression test now uses a manual waiter, enqueues every task and triggers a single flush, so it actually exercises same-batch drain isolation instead of flushing each `add()` on its own. Update the tests accordingly: a synchronously-throwing task's `add()` promise rejects (consistent with the async case) and is surfaced exactly once; a throwing queued lifecycle task does not mark the component mounted and does not produce an unhandled rejection; the scheduler-wedge case is covered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeZwHwo3d9rYsCxJUQqTep
titouanmathis
force-pushed
the
fix/queue-task-throw-isolation
branch
from
August 2, 2026 07:01
329e962 to
9dcd6a6
Compare
titouanmathis
changed the title
A task that threw synchronously used to (a) abort the rest of its batch and (b) leave the queue's scheduling flag stuck `true` forever, which permanently froze every queued lifecycle operation on the page. Its `add()` promise also stayed pending forever, so awaiters hung. - `Queue`/`SmartQueue`: run each task in `try/catch` so one throw can no longer abort the batch; reset the scheduling flag in `finally` and re-arm the flush. A throwing custom scheduler no longer wedges either. - `add()` now rejects on a synchronous throw — consistent with an async task that returns a rejected promise — instead of hanging or silently resolving, so a failure is never mistaken for success. - Handle the rejection at the call sites rather than changing behaviour by hiding it: the fire-and-forget queue calls (`mutationCallback`, the async-child hooks) route errors to a warn helper, and the lifecycle boundaries (`$mount`/`$update`/`$destroy`/`$terminate`) surface the error and no longer falsely mark a component as mounted. Adds regression tests for batch isolation, scheduler-throw recovery, reject consistency, exactly-once error reporting, and the not-falsely-mounted lifecycle behaviour. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeZwHwo3d9rYsCxJUQqTep
`Queue.ts` is the only writer to `this.tasks` package-wide, and after the `add()` fix the closure it pushes provably cannot throw. The extra guards added around it were therefore unreachable through the public API: - `Queue.flush()`'s try/finally - `Queue.run()`'s try/catch and `queueMicrotask` re-throw - both `SmartQueue` overrides, which now simply inherit the fix `scheduleFlush()`'s `isScheduled` reset stays: `waiter` is a public constructor parameter, so a throwing scheduler is reachable. The task it already pushed stays queued on purpose — it runs on the next flush that does get scheduled, and its promise never reached the caller, so rejecting it could only produce an unobservable rejection. Also fix `SmartQueue.spec.ts` leaving a rejection unobserved, which made a root-level `vitest run packages/tests/utils/SmartQueue.spec.ts` exit 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0114BLVFkrWWJAhmamS6NHB3
A failed queued lifecycle task was reported with a bare, ungated
`console.warn`, which no error monitor observes: not `window.onerror`,
not `unhandledrejection`, not the Sentry/Bugsnag defaults. Combined with
the lifecycle methods no longer rejecting, a userland exception had no
channel at all.
Re-throw from a microtask instead — the primitive the queue fix already
used — so the failure reaches the global error handler without wedging
anything. The thrown error names the instance `$id` and the failed
lifecycle, matching the diagnostics style used elsewhere, and carries the
original error as its `cause`.
In blocking mode `addToQueue` runs the task synchronously in the caller's
own stack, so no queue is involved and nothing can wedge: the error is
re-thrown there and the lifecycle promise keeps rejecting, which is what
`try { await createApp(App, { blocking: true }) } catch` relies on.
Adds coverage for the headline scenario (a failing component no longer
stops an unrelated one from mounting), for the `$update`, `$destroy` and
`$terminate` catches, and for blocking mode.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0114BLVFkrWWJAhmamS6NHB3
`$register()` feeds `$mount()` promises into `Promise.allSettled`, and those no longer reject when a queued mount task fails. `registerComponent` was therefore handing back a never-mounted instance as a successful registration, silently undoing the v3.8.0 skip-and-log behaviour. Check `$isMounted` on the fulfilled branch instead. The rejected branch stays: it still fires when the instance could not even be constructed, which no other channel reports. `registerComponents` is unaffected — `registerComponent` still rejects on a failed dynamic import, which is what its own rejected branch covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0114BLVFkrWWJAhmamS6NHB3
`Queue.add()` now rejects when its task fails, so document it and warn that a task which can fail must have its rejection observed — the page still showed fire-and-forget `queue.add(...)` only. Add an error-handling section to the instance methods reference covering the non-rejecting lifecycle methods, the global error channel, the events that are not emitted on failure, and the blocking-mode exception. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0114BLVFkrWWJAhmamS6NHB3
Match the pattern used in parent-resolution-residual.spec.ts so a failed assertion cannot leave `blocking` enabled for later tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0114BLVFkrWWJAhmamS6NHB3
titouanmathis
force-pushed
the
fix/queue-task-throw-isolation
branch
from
August 7, 2026 08:47
9dcd6a6 to
5d6ec2f
Compare
Code ReviewRisk: Low — The queue and lifecycle error-handling changes are internally consistent and safe to merge. The MR contains the synchronous task-throw isolation, scheduler recovery, lifecycle error reporting, and registration filtering described in the intent. It also adds documentation and regression coverage for queue behavior, lifecycle failures, blocking mode, and failed registrations. Review usage: 21,377 in (3,056 cached) / 875 out tokens — $0.0142 (openrouter/openai/gpt-5.6-luna, thinking: low) Reviewed by @weareikko/code-review v0.9.5 for commit 5d6ec2f. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A queued task that throws synchronously used to take the whole task queue down with it.
Queue.add()pushed() => resolve(task())ontothis.tasks. Whentask()threw, the throw escapedresolve(...), escapedrun()'swhileloop, and escapedflush()— which meant:this.tasksforever;isScheduledstayedtrue, so every laterscheduleFlush()early-returned and no flush was ever re-armed;add()promise never settled.Base/utils.tsholds a single module-globalSmartQueuebehindaddToQueue, and every lifecycle method routes its work through it. So one component with a throwingmounted()hook permanently killed mounting, updating, destroying and terminating for every component on the page.Reproduced against
main:await registerComponent(Boom)with a wiring failure never resolves.Fix
Contain the throw where the task runs —
Queue.add()wrapsresolve(task())in a try/catch and rejects the returned promise.run(),flush()andscheduleFlush()never see the throw, the batch keeps draining,isScheduledis reset normally, and a synchronous throw now settles the promise exactly like anasynctask whose promise rejects.SmartQueueinherits this unchanged and is not modified at all.scheduleFlush()also resetsisScheduledif thewaiteritself throws. Unlike the task path, this one is genuinely reachable:waiteris a public constructor parameter (new Queue(concurrency, waiter)). The task already pushed stays queued deliberately — it runs on the next flush that does get scheduled, and its promise never reached the caller (the throw pre-emptsadd()'sreturn), so rejecting it could only produce an unobservable rejection.Lifecycle methods no longer reject in queued mode.
$mount(),$update(),$destroy()and$terminate()catch a failed queued task and resolve. They run detached on the shared queue and are commonly called fire-and-forget — from the auto-mounting mutation observer, from$terminate()when an element leaves the DOM — so a rejection would have had no awaiter and would only have become an unhandled rejection.The error is not swallowed. It is re-thrown from a microtask, so it reaches
window.onerrorand any error monitor, wrapped in an error naming the instance$idand the failed lifecycle, carrying the original error as itscause.A failed lifecycle stops at the point of failure:
after-mountedandafter-destroyedare not emitted. What$isMountedends up as depends on where the failure happened —falseif the wiring failed, andtrueif themounted()hook itself threw, because$isMountedis set immediately before the hook runs. That is deliberate: the component is wired in that case, and$destroy()'s guard needs the flag to tear it down.Blocking mode keeps rejecting. With
features.get('blocking')set,addToQueueruns the task synchronously in the caller's own stack — no queue, nothing to wedge — so the error is re-thrown and the lifecycle promise rejects as it always has.try { await createApp(App, { blocking: true }) } catch {}still catches startup failures.registerComponent()still skips instances that failed to mount. Because$mount()no longer rejects,Promise.allSettledinregisterComponentwould have reported a never-mounted instance as a successful registration, silently undoing the v3.8.0 skip-and-log behaviour. It now checks$isMountedon the fulfilled branch. The rejected branch stays and is still reachable — it fires when the instance could not even be constructed, which no other channel reports.registerComponentsis unaffected:registerComponentstill rejects on a failed dynamic import.Scope
The fix is deliberately minimal. An earlier revision of this branch also added a try/finally in
Queue.flush(), a try/catch inQueue.run()and mirrored both inSmartQueue. Those were removed:Queue.add()is the only writer tothis.taskspackage-wide, and once its closure cannot throw, none of those guards is reachable through the public API.SmartQueue.tsis now byte-identical tomain, andQueue.tscarries about 14 lines of new code.Tests
after-mounted, and reports the error with the component identity and the originalcause.destroyed()/updated()/terminated()hooks: the lifecycle resolves,after-destroyedis not emitted, the error is reported.$mount()on a throwing component produces zero unhandled rejections and exactly one global report.$mount()rejects with the original error and nothing is deferred to a microtask.registerComponentskips a never-mounted instance.All lifecycle tests were checked to fail when the corresponding production change is reverted.
Also fixed
SmartQueue.spec.tsleaving a rejection unobserved, which made a root-levelvitest run packages/tests/utils/SmartQueue.spec.tsexit 1 (CI passed only because it runs frompackages/tests).Docs & changelog
packages/docs/utils/Queue.mddocuments thatadd()rejects on task failure and warns that a failure-capable task must have its rejection observed.packages/docs/api/instance-methods.mdgains an "Error handling" section for the lifecycle contract, the global error channel, and the blocking-mode exception.CHANGELOG.mdcovers the queue wedge,add()rejecting, the non-rejecting lifecycle methods, blocking mode, andregisterComponent.Related, not addressed here
$destroy()leaves the instance in the global storage (after-destroyedis the only path todeleteInstance), so the terminate scan can re-fire$terminate()on each mutation. This is pre-existing onmainfor async-rejecting teardown; this PR widens the reachable paths while fixing a strictly worse failure. Follow-up.$mount()leaves a stale entry in the element storage. Recoverable — the catch resets__isMounting— and strictly better thanmain, which hung. Follow-up.The
'terminated'marker in the mount scan, which an earlier revision of this description called a latent defect, is intended behaviour — see #750, which documents and pins it.Verification
Rebased onto
main(a67c3cfc). From the repo root, afternpm ci && npm run build:npm run test --workspace=@studiometa/js-toolkit-tests— 142 files, 1024 passed, 2 skippednpx vitest run packages/tests/utils/SmartQueue.spec.ts— 4 passed, exit 0npm run lint— 0 errors (1 pre-existingunicorn/no-useless-spreadwarning inautoload/loader.ts, frommain)npm run build— exit 0🤖 Generated with Claude Code
https://claude.ai/code/session_0114BLVFkrWWJAhmamS6NHB3