ReviewOS

also looking at this

craft-native/craft

fix(bridge): correlate replies by request id, not by action name

#71
Merged chris wants to merge fix/bridge-request-id into main
9 files +1086 -91
11 days ago

Closes #66.

The bug

craft-bridge.js matched native's replies to the callers waiting for them by shifting a FIFO keyed on the action name. That guess is wrong whenever two calls settle out of the order they were made, and it fails silently — the caller resolves, with somebody else's payload.

The issue frames this as a narrow same-action race. It is wider than that. Six action names are served by more than one bridge:

actionbridges
getkeychain, tags
isAvailablebiometric, iap, speechRecognition
isEnabledautoLaunch, bluetooth, crashReporter
listserial, shortcuts
requestPermissionlocation, notification
startlocalServer, speechRecognition

Fifteen distinct (type, action) pairs collapse onto six queue keys. Promise.all([craft.keychain.get(…), craft.tags.get(…)]) is enough to reach it.

The error path was worse than the reply path. __craftBridgeError drained the entire queue for an action name, so craft.bluetooth.isEnabled() failing rejected an in-flight craft.autoLaunch.isEnabled() with bluetooth's error — a bridge the caller never touched.

The fix

The page puts an i on every message; native echoes it back; replies resolve by id.

Threading an id through the handlers would have meant touching 267 sendResultToJS call sites across 39 files, reached through five different handleMessage signatures and a layer of per-bridge helpers — and every future one would have to remember. Instead the dispatcher records the id in a new request_context.zig and bridge_error.zig reads it back when it formats the reply. macos.zig grows twelve lines. No handler changes at all.

Three decisions worth the review time:

A stack, not a slot. Dispatch nests — runModal spins a nested run loop that keeps delivering bridge messages, so a second call is served in the middle of the first. A slot would be clobbered by the inner call and the dialog's reply would go out wearing the inner call's id. That is worse than no id: the page resolves a stranger's promise and is certain it was right.

A message with no id pushes null, rather than skipping the push. An absent id has to shadow the enclosing one. If id-less messages skipped, one dispatched inside a call that does have an id would inherit it — the same misattribution by another route.

null for absent, never 0 or -1. Zero is a perfectly good id, and a sentinel is how this bug class returns. Likewise, a reply carrying an id the page does not recognise is dropped, not fallen back to the queue — falling back is the swap.

Also fixed

menubar.getState had a hand-rolled pending entry queued under a reply key native invented for it (menubarCollapse:getState), bypassing _req entirely — including its timeout, so a call native never answered held its resolver for the life of the page. The id makes the key meaningless, so it is an ordinary _req now. That puts it on getState alongside screenSharing — a seventh collision, which the ids handle.

How it's tested

By driving the real bridge, not by reading it.

  • 13 bun tests (bridge-request-id.test.ts) run the embedded craft-bridge.js in a synthetic window and check which promise settled with what. Against the version on main, 9 of the 13 fail; the 4 that pass are precisely the ones asserting the unchanged fallback. Two are inverted controls that reproduce the original swap.
  • 8 tests in injected_js_test.zig run it in a real JS engine via zig-js and answer it with the very string sendResultToJS emits — so the JS half and the native half cannot drift apart silently.
  • 20 Zig unit tests across request_context.zig and bridge_error.zig, including the nested-dialog case and the exact reply strings.

Every one of those was control-checked. Sabotaging the JS id branch fails the suite; making native emit null instead of the id fails it; renaming the menubar action fails it. Before this change, sabotaging the id branch changed nothing, because nothing exercised it.

Verified

zig build · zig build test · zig build test-js · x86_64-windows cross-build · zig fmt --check · tsc --noEmit · 506 bun tests (was 493) · pickier 38 warnings, unchanged from main.

Linux is unbuildable on this machine (no gtk+-3.0 / webkit2gtk-4.1); it compiles and fails at link, as it does on main.

9 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.