Swap worklet IPC from pipes to the in-process queue by bavulapati · Pull Request #94 · holepunchto/bare-kit

bavulapati · 2026-07-03T17:50:50Z

Bare Kit is in-process - worklets are threads - so the pipe transport with its async close on exit was the wrong tool, and the root of the EOF and win32 problems (holepunchto/bare#160, #92).

Bare.IPC becomes a bare-stream Duplex over the queue, and the worklet closes its end synchronously after bare_run, so the host reaches EOF on Bare.exit(), on Bare.IPC.end(), and on a host terminate, with no loop pump and nothing scheduled from the exit event. The host polls readiness instead of reading a pipe: apple keeps its dispatch source, linux and win32 now share one uv implementation in place of a hand-rolled epoll thread and a pipe loop, and android keeps its looper with the ipc owning the eventfd.

A worklet whose loop simply drains still does not reach EOF: Bare.IPC stays ref'd, so the loop never drains and bare_run never returns. That is unchanged from main, which does not EOF on that path either, so #83 stays open for it.

The two EOF tests now run on win32, which is what this was for.

Breaking: bare_ipc_get_incoming and bare_ipc_get_outgoing go with the pipes, along with the win32 exports, so this needs a major and a coordinated react-native-bare-kit. The worklet drops the bare-ipc and bare-pipe addons with them, so app code can no longer require either; nothing links them once the bootstrap builds its Duplex on bare-stream.

Split out of the original branch, which had grown to +1107/-843 across 27 files. The pieces that stand on their own went first:

Signal the queue's peer under its lock #105 - signal the queue's peer under its lock
Give the queue an owner rather than a caller's storage #108 - give the queue an owner rather than a caller's storage
Keep an IPC's worklet alive for as long as the IPC #107 - keep an IPC's worklet alive for as long as the IPC

This is what is left, and it cannot go smaller: shared/ipc.c drives every platform through one struct, so a swap that ports only apple would leave linux, win32 and android failing to build.

ipc.h and worklet.h now include queue.h, which adds it to the set of headers the desktop prebuild ships. Worth a look against docs/plans/2026-06-11-worklet-clean-c-api.md, which treats that set as closed.

kasperisager · 2026-07-03T18:43:53Z

+    bare_terminate(worklet->bare);
+    uv_async_send(&worklet->queue.async);
+    uv_sem_post(worklet->finished);
+    uv_thread_join(&worklet->thread);


We must not join the thread here, the function is never allowed to block; it has historically triggered ANR errors on Android.

That's the reason for the thread being detached, it's a strict requirement.

Reverted the join — understood, destroy must never block.

It was masking a teardown race I'd like your steer on. With the thread detached and destroy non-blocking, the shared worklet-ipc test (host terminate, then main returns and the process exits immediately) races the worklet's teardown against process exit: it dies non-zero, oscillating between a clean 72 and an abort by timing. This passed on the old pipe transport. The difference is the queue's post-park teardown: after bare_run the worklet closes its queue end and parks, then on destroy it unparks and runs bare_queue_destroy (a uv_close on the queue's async) before bare_teardown. If the process exits before that finishes, it goes down.

Every EOF path (loop drain / Bare.exit / IPC.end / host terminate -> host reads EOF) passes; it is specifically the detached-teardown-vs-immediate-process-exit window.

How would you like the worklet-side teardown shaped so it is safe without the host waiting? One option: close and flush the queue's async during bare_run as the worklet exits, so the post-park teardown only touches bare's own handles, as the pipe path did. Or something else you would prefer for the detached teardown.

What's the exact issue we hit when bare_teardown() doesn't finish prior to the process exiting?

Chased down your question - turns out it's the apple poll, not the queue/worklet teardown like I'd assumed.

The tell: only the tests using bare_ipc_poll race (worklet-ipc, worklet-ipc-large-write); the non-poll IPC tests and suspend/resume are all stable, and if I leak bare_ipc_poll_destroy's dispatch teardown, worklet-ipc goes 25/25 clean. The pipe baseline on main is 25/25 too, so it came in with the poll rewrite here.

What's happening: the worklet thread fires dispatch_source_merge_data(poll->source) (through the queue's on_signal_thread -> bare_ipc__poll_signal) right as the host thread cancels and releases that source in bare_ipc_poll_destroy - a use-after-free on the source across threads. The heap corruption then surfaces wherever: libmalloc "pointer being freed was not allocated" in bare_queue__free, a uv_mutex_lock abort on a trashed queue mutex, or a clean exit 72 at full speed (it's a Heisenbug - any instrumentation slows it into passing or a clean abort).

I took a swing at the obvious fix - serialize the merge_data against the null+release under the queue lock - but it's not enough on its own: the source's already-scheduled handler and the cancel/release ordering still race, so I backed it out rather than leave a half-fix.

The knot I keep hitting: the worklet only stops signalling once the queue's thread port is closed (the thread_closed guard), but teardown closes the poll before the port - so whatever we do, the worklet needs to stop touching the source before it's released.

I'd like your read on the shape before I build it. A few directions I can see - require the thread port be closed before poll_destroy so the guard kicks in first; give the ipc ownership of the source with a retain across the signal; or a signal path the worklet can't dangle on - but you may have a cleaner instinct. Happy to implement whichever we land on.

We need to dial down the Claude here and cut through the noise. The invariants that must be maintained are these:

Anything not owned by the worklet may be accessed right up until the moment the worklet is parked and waits for the termination.

Anything that must be accessed by the worklet after 1. must be owned by the worklet.

Anything not owned by the host may be accessed right up until the host signals the worklet to destroy.

Anything that must be accessed by the host after 3. must be owned by the host.

The design will follow from those invariants.

bavulapati · 2026-08-13T08:10:08Z

From the invariants: the dispatch source is host-owned, so the worklet must never touch it. Today it does, via on_signal_thread -> merge_data.

So the signal moves to the queue: the queue owns the primitive, the poll subscribes a callback, poll_destroy clears it under the queue lock. The worklet then only dereferences the queue, and releasing the poll can't free anything it is about to touch. No ordering needed between closing the port and destroying the poll.

I'll build that, get the apple suite green, then port the linux/android/win32 polls the same way.

Bare Kit is in-process - worklets are threads - so the pipe transport, with its async close on exit, was the wrong tool and the root of the EOF and win32 problems (holepunchto/bare#160, #92). Bare.IPC becomes a bare-stream Duplex over the queue, and the worklet closes its end synchronously after bare_run, so the host reaches EOF on every exit path with no loop pump and nothing scheduled from the exit event. The host polls readiness rather than reading a pipe: apple keeps its dispatch source, linux and win32 now share one uv implementation instead of a hand-rolled epoll thread and a pipe loop, and android keeps its looper with the ipc owning the eventfd. The two poll tests closed their loop while their poll was still armed. Pipes hid that: their EOF arrived asynchronously at thread teardown, while the queue closes synchronously, so the wake lands in the window and the callback posts to a closed loop - a segfault on linux-x64. The poll is retired first now. bare_ipc_get_incoming and bare_ipc_get_outgoing go with the pipes; the win32 exports follow. Both EOF tests now run on win32, which is what this was for.

bavulapati · 2026-08-14T15:49:37Z

Split this up first: #105, #107 and #108 have landed, so what is left here is the transport change on its own.

The design follows from your invariants. The worklet closes its end after it parks, so by 2 the queue is the worklet's - heap allocated, handed over at start, freed by the worklet thread. No reference counting; that was a mechanism I reached for before naming an owner.

The one thing it asks of hosts is that the ipc is destroyed before the worklet. Both headers say so now, and #107 is what makes our own bindings honour it.

Green on all platforms, and both EOF tests now run on win32 and pass - which was the point.

bavulapati · 2026-08-31T16:12:45Z

@kasperisager PTAL

kasperisager

Looks good! Just a small style alignment.

Co-authored-by: Kasper Isager Dalsgarð <kasperisager@hey.com>

kasperisager

There's an intermittent segfault in the tests.

kasperisager · 2026-09-17T14:08:34Z

The queue also has an intermittent test failure: https://github.com/holepunchto/bare-kit/actions/runs/35230115289/job/105231694082

The thread must stay detached: destroy is never allowed to block, and joining here has historically caused ANR errors on Android.

The signal callback read env and ipc_signal from the host's worklet through port->data, so a queue wakeup still in flight at teardown dereferenced storage the host had already released, crashing in js_open_handle_scope. The worklet thread now owns those two fields and frees them once the queue can no longer signal. Fixes the intermittent worklet-ipc segfault on darwin-x64 and win32-arm64.

A write to a closed peer was treated as a would-block and retried forever, so the write callback never fired, _final never ran, and the duplex wedged while holding a ref'd async. The native write now reports a closed peer as null, the way read already reports EOF, and the stream errors.

bavulapati · 2026-09-18T05:39:42Z

Chased the queue failure you linked. It is not the queue, and not this PR: it is liblog's ETW provider. Filed as holepunchto/liblog#4 with a fix in holepunchto/liblog#5.

src/win32.cc holds the provider in a thread_local with a non-trivial constructor, so a dynamic initializer runs on every thread start and builds a std::wstring. A thread starting while the process is exiting takes the CRT _Lockit lock after it has been torn down and faults on a NULL mutex. Windows supplies that thread itself: exit() resolves AppPolicyGetProcessTerminationMethod through LdrLoadDll, whose parallel loader spawns a worker. test/shared/queue is the one that shows it because it is the shortest binary we have, so it exits while the loader is still starting threads.

Faulting thread, with the main thread already inside exit():

ntdll!LdrInitializeThunk -> LdrpInitializeThread -> LdrpCallTlsInitializers
  -> __dyn_tls_init -> `dynamic initializer for 'log_provider'
    -> std::basic_string<wchar_t>::~basic_string -> std::_Lockit::_Lockit
      -> _Mtxlock        <-- NULL mutex, reads 0x24

A/B on one windows-11-arm runner, 3000 runs each side: 59/3000 with liblog as fetched, 0/3000 with the fix. For reference it is 0/3000 on win32-x64 and does not reproduce on darwin at all, including under guard malloc.

It reaches us via bare-system-logger, which pins liblog#edcfd8f, so landing it is liblog -> bare-system-logger -> bare -> here. The flake stays in CI until that works through.

Separately, the three commits above address your review here: the thread is detached again, the IPC signal state moved off the host's worklet, and a write to a closed peer now errors the stream instead of retrying forever. The second one turned out to be the worklet-ipc segfault on darwin-x64 and win32-arm64 — the signal callback reached worklet->env through port->data after the host had torn down, which is @tony-go's point on the intermediate struct. 4-5 per 400 runs before, 0 in 1600 after, and both jobs now pass in CI.

tony-go

Looks good % bare-system-logger / bare release

There is a lot of code, I could favour a npm version prerelease and some back and forth on our doctor apps.

The callback only runs once the reference exists, so make that ordering explicit rather than implicit, and give the closed-peer write longer to be retried so the test does not flake on a loaded runner. Co-authored-by: Tony Gorez <22824417+tony-go@users.noreply.github.com>

# Conflicts: # test/shared/worklet-ipc-large-write.c # test/shared/worklet-ipc.c

tony-go · 2026-09-21T08:41:01Z

+  if (worklet->thread != 0) {
+    // Stop the worklet if it is still running and wake its loop so the stop
+    // takes effect, then unblock it. The thread is detached and tears itself
+    // down; this function must never block (blocking here causes ANR on
+    // Android). bare_terminate is a no-op if the worklet already exited.


I think that we could avoid this kind of logic, by using ref counting.

The worklet will continue to create the queue and then unref it in the destroy method.

Any other thread would then ref/unref too.

For the last unref, the bare_queue_destroy is properly called.

The queue has one owner, and this branch is not a lifetime mechanism - it only asks whether anything ever took ownership.

Ownership follows @kasperisager's invariants from the review above: anything the worklet must touch after it parks has to be owned by the worklet, and the queue is that. It is created in init because the documented host order opens an ipc before start (new IPC(worklet) then worklet.start(...) in the README, and bare_ipc_init opens its end off worklet->queue), handed over at start, and freed by the worklet thread after bare_teardown. The host must retire its end first; that is documented on both bare_ipc_destroy and bare_worklet_destroy, and #107 is what makes our own bindings honour it.

The else is the case where no thread ever took it - bare_worklet_destroy straight after init (worklet-destroy-before-start), or a failed uv_thread_create. Note it frees worklet->state on the same condition, since that is allocated in init and handed over at start too, so refcounting the queue would not remove the branch.

Refcounting is where this started, and the invariants are what replaced it. The concrete problem: uv_async_t async is embedded in bare_queue_t and lives on the worklet's loop. Freeing the queue is only safe once bare_queue_shutdown_uv has closed that handle and the loop run inside bare_teardown has flushed the close, both of which can only happen on the worklet thread. A refcount hands the free to whoever drops last, so a host unref'ing late would free a uv handle the worklet's loop still owns. And bare_worklet_destroy must never block, so the host cannot wait to find out which it is. Naming a single owner is what fixes the thread that does the free without anyone waiting.

Note it frees worklet->state on the same condition, since that is allocated in init and handed over at start too, so refcounting the queue would not remove the branch.

Yeah but we would not have to handle the queue here.

The host must retire its end first; that is documented on both bare_ipc_destroy and bare_worklet_destroy, and #107 is what makes our own bindings honour it.

The contract is documented but [BareWorklet terminate] calls bare_worklet_destroy with the IPC still open, which is exactly what shared/worklet.h says not to do.

Closing the open IPC inside terminate, in both bindings, would make them honour what the headers already promise.

TLDR: the refcount isn't the real issue here. terminate() leaving a usable IPC pointing into a freed queue is.

tony-go · 2026-09-21T08:44:53Z

-  worklet->outgoing = -1;
+  worklet->queue = bare_queue_create();
+
+  if (worklet->queue == NULL) return -1;


I think we have a leak here because we don't clean bare_worklet_state_t and options...

Also we should take of look at places where bare_worklet_init function is called to check on the -1.

When we add a variant we should track them as far as we can.

Fixed in 6384389: nothing unwound when an allocation failed, so the state and the assets copy both leaked. Moving the options copy after the queue leaves a single failure path.

On the call sites: all three do assert(err == 0), and so does every other C call in both bindings - start, suspend, resume, terminate, push. Singling out init would leave the bindings inconsistent without closing the hole, since the rest still vanish under NDEBUG. Error propagation through the bindings is worth doing as its own change; happy to open an issue for it.

tony-go · 2026-09-21T08:55:56Z

+  poll->events = 0;
+  poll->cb = NULL;


I have the feeling that we don't treat the poll properties in the same way in each of our implementation.

For instance, in the Apple one don't reset the events to zero, but it he cb is atomic...

Aligned in 63d2cb4. Apple left events set and uv cleared the poll state by hand; both now call bare_ipc_poll_stop first, which is the shape android already had.

All that differs now is how each waits out a callback in flight: apple drains its serial queue, uv takes the running lock, android runs the callback on the thread that owns the poll.

tony-go · 2026-09-21T08:58:29Z

 const SystemLog = require('bare-system-logger')
 const Console = require('bare-console')
-const IPC = require('bare-ipc')
+const { Duplex } = require('bare-stream')


Let's remove bare-pic and add bare-stream from the package.json

Done in 775b862. bare-stream declared, bare-ipc dropped. bare-pipe went with it: it only resolved transitively through bare-ipc, and builtins.json listed both as addons.

So app code in a worklet can no longer require either, which I'll flag as breaking in the description.

Nothing unwound when an allocation failed, leaking the state and the assets copy. Move the options copy after the queue, so the only failure path left frees the state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Apple left the requested events set and uv cleared the poll state by hand. Both now stop the poll first, as android already did, so the three implementations differ only in how they wait out a callback in flight. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The worklet bootstrap builds its Duplex on bare-stream, which was never declared, and no longer requires bare-ipc or the bare-pipe it drags in. Breaking: app code in a worklet can no longer require bare-ipc or bare-pipe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The queue is the worklet's from the moment it is signalled to be destroyed, so a host holding an open IPC reads freed memory. Assert the documented order rather than letting it corrupt silently.

Both bindings called bare_worklet_destroy with the IPC still open, which is what the headers say not to do. Terminate now closes it first, and a closed IPC reads end-of-stream and refuses writes instead of reaching for a freed queue.

kasperisager requested changes Jul 3, 2026

View reviewed changes

This was referenced Aug 11, 2026

v2.4.0 #97

Merged

Surface IPC end-of-stream on win32 #92

Closed

bavulapati force-pushed the ipc-in-process-queue branch from bb1258c to 6100eea Compare August 13, 2026 17:34

This was referenced Aug 14, 2026

Signal the queue's peer under its lock #105

Merged

Give the queue an owner rather than a caller's storage #106

Closed

Keep an IPC's worklet alive for as long as the IPC #107

Merged

bavulapati changed the base branch from main to swap-base August 14, 2026 11:24

bavulapati force-pushed the ipc-in-process-queue branch from e2971b6 to f799d84 Compare August 14, 2026 11:25

bavulapati mentioned this pull request Aug 14, 2026

Give the queue an owner rather than a caller's storage #108

Merged

bavulapati force-pushed the ipc-in-process-queue branch from f799d84 to 378cc8a Compare August 14, 2026 13:16

bavulapati force-pushed the swap-base branch from 8323664 to 355c0af Compare August 14, 2026 13:21

bavulapati force-pushed the ipc-in-process-queue branch from 378cc8a to 32d33e4 Compare August 14, 2026 15:22

bavulapati changed the base branch from swap-base to main August 14, 2026 15:22

bavulapati closed this Aug 14, 2026

bavulapati reopened this Aug 14, 2026

bavulapati marked this pull request as ready for review August 14, 2026 15:38

bavulapati requested review from a team and kasperisager August 14, 2026 15:38

bavulapati added 3 commits September 2, 2026 12:33

Merge branch 'main' into ipc-in-process-queue

18fee9e

Merge branch 'main' into ipc-in-process-queue

8d6231a

Merge branch 'main' into ipc-in-process-queue

a6ea085

kasperisager requested changes Sep 11, 2026

View reviewed changes

Comment thread shared/worklet.c Outdated

bavulapati and others added 2 commits September 11, 2026 12:52

Update shared/worklet.c

2028251

Co-authored-by: Kasper Isager Dalsgarð <kasperisager@hey.com>

Merge branch 'main' into ipc-in-process-queue

b86767f

tony-go requested changes Sep 15, 2026

View reviewed changes

Comment thread shared/worklet.c Outdated

Comment thread shared/worklet.c Outdated

Merge branch 'main' into ipc-in-process-queue

f5ba60b

kasperisager requested changes Sep 16, 2026

View reviewed changes

bavulapati and others added 4 commits September 17, 2026 23:11

Detach the worklet thread again

4c445ca

The thread must stay detached: destroy is never allowed to block, and joining here has historically caused ANR errors on Android.

Merge branch 'main' into ipc-in-process-queue

d759bde

bavulapati requested review from kasperisager and tony-go September 18, 2026 05:56

tony-go reviewed Sep 18, 2026

View reviewed changes

Comment thread shared/worklet.c

Comment thread shared/worklet.c

Comment thread test/shared/worklet-ipc-write-after-close.c Outdated

bavulapati and others added 2 commits September 18, 2026 13:15

Merge branch 'main' into ipc-in-process-queue

80279f6

bavulapati force-pushed the ipc-in-process-queue branch from 203588a to eb07ca1 Compare September 18, 2026 07:55

bavulapati requested a review from tony-go September 18, 2026 08:02

bavulapati mentioned this pull request Sep 18, 2026

Retire the IPC poll before closing its loop #134

Merged

bavulapati added 2 commits September 18, 2026 19:13

Merge branch 'main' into ipc-in-process-queue

76badd2

Merge branch 'main' into ipc-in-process-queue

2cd7872

# Conflicts: # test/shared/worklet-ipc-large-write.c # test/shared/worklet-ipc.c

tony-go requested changes Sep 21, 2026

View reviewed changes

bavulapati and others added 5 commits September 22, 2026 16:43

Fix the worklet init error paths

6384389

Nothing unwound when an allocation failed, leaking the state and the assets copy. Move the options copy after the queue, so the only failure path left frees the state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Assert the host has retired its IPC before destroy

3513029

The queue is the worklet's from the moment it is signalled to be destroyed, so a host holding an open IPC reads freed memory. Assert the documented order rather than letting it corrupt silently.

Retire the IPC when a worklet is terminated

51f0443

Both bindings called bare_worklet_destroy with the IPC still open, which is what the headers say not to do. Terminate now closes it first, and a closed IPC reads end-of-stream and refuses writes instead of reaching for a freed queue.

Conversation

bavulapati commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bavulapati commented Aug 13, 2026

Uh oh!

bavulapati commented Aug 14, 2026

Uh oh!

bavulapati commented Aug 31, 2026

Uh oh!

kasperisager left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kasperisager left a comment

Choose a reason for hiding this comment

Uh oh!

kasperisager commented Sep 17, 2026

Uh oh!

bavulapati commented Sep 18, 2026

Uh oh!

tony-go left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bavulapati commented Jul 3, 2026 •

edited

Loading