feature: Add BackgroundTask — cross-platform cancellable, progress-pollable worker (#552)#588
Conversation
…llable worker (#552) A primitive to run a plain side-effecting block on a background worker that can be cooperatively cancelled and progress-polled from another thread — no effect monad. Motivated by wvlet's DuckDB SqlConnector (run a query off-thread while polling duckdb_query_progress and supporting duckdb_interrupt). - wvlet.uni.control.BackgroundTask[A, P]: progress (poll snapshot), poll, await, cancel, isCancelled, isDone. Body receives a TaskContext with isCancelled / checkCancelled() / reportProgress(p) / onCancel(hook) (the hook is the escape hatch for interrupting an in-flight FFI call, e.g. duckdb_interrupt). - Shared state machine (Atomic flags + result + lock-coordinated cancel hooks); only the worker spawn + completion gate are per-platform (BackgroundTaskCompat): JVM/Native = daemon Thread + CountDownLatch; JS = inline run + no-op gate. - JS limitation: Node worker_threads run a separate JS realm and can't host an arbitrary Scala closure (see adr/2026-05-14), and Scala.js is single-threaded, so the body runs inline during start() — same API, no concurrency. The DuckDB target is JVM/Native. Documented on the API. Tests: cross-platform outcomes (completion/failure/progress/await, run inline on JS) + JVM & Native concurrency (live cancel via isCancelled, checkCancelled -> Failure(TaskCancelledException), onCancel fires, await blocks). JVM 9 / JS 4 / Native 9 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces BackgroundTask, a cross-platform, cooperatively cancellable, and progress-pollable background worker with platform-specific implementations for JVM, Native, and JS. Feedback on the changes highlights a critical issue where fatal exceptions (such as OutOfMemoryError) could cause await() to hang indefinitely because the completion gate is not signalled in a finally block. Additionally, it is recommended to clear registered onCancel hooks upon task completion to prevent potential memory leaks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Code Review
This pull request introduces BackgroundTask, a cross-platform, cooperatively cancellable, and progress-pollable background worker implementation for JVM, Native, and JS targets. While the JVM and Native platforms run tasks concurrently on daemon threads, the JS platform executes them inline. The feedback highlights two critical issues in the shared state machine: first, catching only NonFatal exceptions can cause await() to hang indefinitely if a fatal exception or InterruptedException occurs; second, failing to clear registered onCancel hooks upon task completion leads to memory leaks and potential post-completion execution of cancel hooks. Addressing these issues by wrapping the execution in a finally block to clean up hooks and signal the gate is highly recommended.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
… (review) - Wrap the worker body in try/finally so gate.signal() ALWAYS runs, and catch Throwable (not just NonFatal) so a fatal error / InterruptedException becomes a Failure result instead of hanging await() forever. (Recorded, not rethrown, so the JS inline path doesn't make start() throw.) - On completion, drain the onCancel hooks (set hooksDrained + clear) so a later cancel() can't fire them against a now-unrelated operation (e.g. duckdb_interrupt on a different query) and captured closures are released. - onCancel: when already drained, run a late hook immediately only if cancelled (drained by cancel()), not if drained by normal completion. Test: cancel after normal completion does not run onCancel hooks (cross-platform). JVM 10 / JS 5 / Native 10 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to #588 (`BackgroundTask`, issue #552). ## What 1. **`progressStream: Rx[P]`** on `BackgroundTask` — a push stream of progress updates that completes when the task finishes, for reactive consumers (a live CLI progress bar / UI) that want to *react* rather than poll. The poll snapshot (`progress`) stays. 2. **ADR** `adr/2026-06-20-background-task.md` capturing #588's non-obvious decisions, linked from `CLAUDE.md`. ## Design notes (two real footguns avoided) - Backed by an `RxVar[Option[P]]` used **only as the notification channel**; `progress` stays on the `AtomicReference` because `RxVar.get` reads a non-`@volatile` var outside any lock — not safe for cross-thread polling. `reportProgress` updates both. - Uses **`flatMap` (not `filter`)** to skip the initial replayed `None`: a *false* `filter` predicate emits `OnCompletion` downstream (`RxRunner` `FilterOp`), which would prematurely end the stream. `flatMap { Some(p) => Rx.single(p); None => Rx.empty }` skips the `None` instead. `RxVar` holds only the latest value, so an unconsumed stream doesn't accumulate (no buffering leak, unlike `Rx.queue`). - **JS**: the body runs inline during `start()`, so a later subscriber sees only the final value (not a live feed) — documented; use the poll snapshot on JS. ## Testing `progressStream pushes reported progress and completes` (JVM + Native; not cross-platform since JS has nothing live to observe). Full suites: **JVM 1651 / JS 1401 / Native 1413**. Plan: `plans/2026-06-20-background-task-progress-stream.md`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Closes #552.
Why
wvlet's
SqlConnector.QueryHandle(state/stats/cancel/await) is free for Trino (polls a remote server) but the DuckDB impl is a synchronous wrapper —cancelis a no-op, no progress. libduckdb exposesduckdb_query_progress/duckdb_interrupt/ a pending-result API, but driving them needs the query to run on a different thread than the one polling progress / cancelling. uni had no cross-platform primitive for that. This adds one.What
wvlet.uni.control.BackgroundTask[A, P]— run a plain side-effecting block on a background worker, no effect monad:The shared state machine (atomic flags + result + lock-coordinated cancel hooks) lives in
uni/src/main; only the worker spawn + completion gate are per-platform (BackgroundTaskCompat): JVM/Native = daemonThread+CountDownLatch; JS = inline run + no-op gate. ReusesResult,ThreadUtil.Design decision: JS is non-concurrent (documented)
The issue assumed Node
worker_threadscould host the work, but they run a separate JS realm fed a pre-written JS string (seeadr/2026-05-14-nodejs-sync-http.md) — an arbitrary Scala.js closure can't run on one, and Scala.js is single-threaded. So a true background worker exists only on JVM and Native (the DuckDB targets). The API is identical on all three, but on JS the body runs inline duringstart()(completes before it returns; no concurrency;cancel()after completion is a no-op). This was confirmed with the maintainer before implementing.Testing
uni/src/test, JS runs inline): completion →Success, body throw →Failure, progress reported,await/pollagree.isCancelled,checkCancelled→Failure(TaskCancelledException),onCancelfires (incl. registered-after-cancel),awaitblocks until completion.Plan:
plans/2026-06-20-background-task.md. Follow-ups noted there:Rx[P]progress stream,Thread.interrupt()on cancel, worker pooling/timeouts, and an ADR for the Node-worker_threads limitation.🤖 Generated with Claude Code