Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 24 additions & 21 deletions docs/adr/009-ffi-bridge-reliability.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,27 +66,28 @@ different dispatch mechanism:
| Adapter | Dispatch model | Respond path |
|---------|----------------|--------------|
| Node | Callback via `ThreadsafeFunction` (push) | Synchronous NAPI call |
| Deno | mpsc queue + `nextRequest()` polling (pull) | Synchronous FFI symbol |
| Deno | Bounded mpsc queue + `UnsafeCallback` wake (push notification, synchronous drain) | Synchronous FFI symbol |
| Tauri | Tauri `Channel` event stream (push) | Typed invoke command |

The Node adapter has proven reliable under high concurrency (32-stream bursts,
5 MB bodies). The Deno adapter has not. Issues #119 and #122 document race
conditions that produce `unknown handle` and `sendChunk failed` errors under
the same load that Node handles cleanly.

The root causes are architectural, not incidental: the Deno adapter's
JSON-over-FFI + mpsc + polling design introduces latency between handle
allocation in Rust and handle consumption in JS. That latency window is where
every observed race occurs.
The original root causes were architectural, not incidental: Deno's former
JSON-over-FFI + async `nextRequest()` path introduced latency between handle
allocation in Rust and handle consumption in JS. Issue #122 replaced that path
with synchronous queue draining to avoid `spawn_blocking` starvation. Issue
#397 retained that safe drain path but replaced its hot idle polling with a
thread-safe callback that only notifies JS when work is ready.

This exploration asks what architectural changes would bring Deno (and future
adapters) to the same reliability level as Node and Tauri.

## Questions

1. Should the Deno adapter move from a polling (`nextRequest`) model to a
push model (e.g. a callback-based or channel-based mechanism), and what
would that look like given Deno's FFI constraints?
push-notified model while retaining synchronous queue draining?
2. Should `on_request` in the core avoid spawning detached tokio tasks, and
instead use a synchronous (non-spawn) sender so the adapter controls the
async boundary?
Expand Down Expand Up @@ -121,20 +122,23 @@ libuv event loop. The NAPI `rawRespond` call is synchronous from the JS
thread into the Rust slab. There is no intermediate queue, no polling latency,
and no detached task.

**Why Deno struggles:** The `on_request` callback spawns a detached tokio task
to `try_send` into an mpsc channel. JS polls with `nextRequest()`. Between
enqueue and dequeue, the Rust request task continues running — it may time out,
the drain may fire, or `stopServe` may remove the registry entry. By the time
JS calls `respond()`, the handle may already be gone.
**Why Deno struggled before #122:** The `on_request` callback used an async
`nextRequest()` dispatch through Deno's fixed `spawn_blocking` pool. Under
concurrent self-fetches, request delivery competed with fetch work and could
deadlock. Deno now uses a bounded queue, a request-ready callback carrying only
an opaque generation token, and synchronous `try_next_request` draining on the
JS thread. This preserves #122's deadlock avoidance without #397's idle poll.

**Why Tauri is safe:** Tauri's `Channel` is owned by the command lifetime.
Frontend and backend are separate processes, so there is no shared-runtime
deadlock risk. The channel is push-based with fail-closed semantics.

### Constraints

- Deno's FFI (`dlopen`) does not support passing closures or callbacks from
Rust to JS — hence the polling model.
- Deno's FFI supports `UnsafeCallback.threadSafe()` for calls originating on
foreign Tokio threads. The callback pointer must outlive every possible
native invocation and should be unrefed when it must not keep the process
alive.
- Respond must be synchronous in Deno to avoid deadlock when client and server
share the same single-threaded event loop (documented in #122).
- The core's `on_request` callback must not block the QUIC accept loop.
Expand All @@ -150,6 +154,7 @@ deadlock risk. The channel is push-based with fail-closed semantics.
| **C. InsertGuard ownership tokens** — Rust returns an owning guard with the handle; handle cannot be freed until guard is dropped | Explicit lifetime control | Adds RAII complexity across the FFI boundary; guards must be passed back |
| **D. Move Deno to napi-rs** — use `napi-rs` for Deno (via `deno_napi` compat) instead of raw FFI | Same proven dispatch as Node | Adds napi dependency; Deno FFI is the official path and napi compat is not guaranteed |
| **E. Tolerate stale handles** — make `respond`/`sendChunk`/`finishBody` return Ok for missing handles | No architectural change | Silently drops responses; masks real bugs |
| **F. Callback wake + synchronous drain** — retain the bounded queue and `try_next_request`, but invoke a thread-safe Deno callback with an opaque generation token after enqueue | Preserves #122 behavior, avoids borrowed payload pointers, sleeps when idle | Requires callback pointer lifetime and restart routing to be explicit |

## Implications

Expand All @@ -164,13 +169,11 @@ deadlock risk. The channel is push-based with fail-closed semantics.

## Next steps

- [ ] Prototype Option B (remove `tokio::spawn` in Deno dispatch, use direct
`try_send`) — this is the smallest change and may be sufficient.
- [ ] If Option B is insufficient, prototype Option A (oneshot acknowledgment)
on a branch and benchmark the per-request overhead.
- [ ] Run the regression test from #122 (32-stream burst × 5 iterations) on
each prototype to confirm zero stale-handle errors.
- [x] Keep direct bounded `try_send` delivery and synchronous queue draining.
- [x] Add Option F's callback wake without passing request payload pointers.
- [x] Run the regression test from #122 (32-stream burst × 5 iterations) to
confirm zero stale-handle errors.
- [ ] Audit whether any other adapter has a similar detached-task pattern that
could surface under future load.
- [ ] Document the chosen dispatch contract in [architecture.md](../architecture.md)
- [x] Document the chosen dispatch contract in [architecture.md](../architecture.md)
so future adapters (e.g. Python) follow the same pattern.
10 changes: 9 additions & 1 deletion docs/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ Core's `serve()` accepts an `on_request: Arc<dyn Fn(RequestPayload) + Send + Syn
| Adapter | Mechanism | Model |
|---------|-----------|-------|
| Node | `ThreadsafeFunction` | Push — callback into JS event loop |
| Deno | `mpsc` queue + `nextRequest()` | Pull — JS polls for requests |
| Deno | bounded `mpsc` queue + `UnsafeCallback.threadSafe()` | Push notification — JS synchronously drains queued requests |
| Tauri | Tauri `Channel` | Push — event emitted to frontend |

**Behavioral guarantees of `on_request`:**
Expand All @@ -113,6 +113,14 @@ Core's `serve()` accepts an `on_request: Arc<dyn Fn(RequestPayload) + Send + Syn

Each adapter is responsible for surfacing errors from its callback mechanism. The core does not observe whether the callback succeeded.

The Deno callback carries only an opaque serve-generation token. Request
payloads remain owned by the bounded Rust queue and cross FFI only when
`iroh_http_try_next_request` synchronously drains them on the JS thread. One
module-lifetime `UnsafeCallback` keeps its function pointer valid for every
endpoint and generation; it is unrefed so an idle server does not keep the
process alive. Generation-keyed wake signals prevent a late notification from
an old serve cycle from waking a restarted server.

#### Request-delivery seam (`RequestTransport`)

The shared part of each adapter's `on_request` closure lives once in `iroh-http-adapter` behind the `RequestTransport` seam (ADR-009, #315). Core already acquires the request-body reader / response-body writer, measures header bytes, and fires `on_request` with a `RequestPayload`; the only parts that were duplicated across the three bridges — the header reshape into `Vec<Vec<String>>` and the fail-closed undeliverable fallback — are owned by the adapter:
Expand Down
Loading
Loading