rust/jsg: typed jsg::Function and function-call FFI safety fixes

[guybedford]

This adds typed JS function handles to the Rust JSG layer, allowing Rust-implemented APIs to store and invoke user-provided JavaScript callbacks.

• jsg::Function<Args, R> — a persistent, GC-traced handle to a JS function with a typed Rust call signature, the counterpart of unwrapping a C++ jsg::Function<Ret(Args...)>. Implements FromJS/ToJS/Traced, so it can be taken directly as a #[jsg_method] parameter and stored in resource fields (participating in cycle collection). Arguments convert via ToJS tuples and returns via FromJS; call_with_receiver passes an explicit this.
• FromJS for () supports discard-result callbacks, mirroring C++ jsg::Function<void(...)>.

Two safety fixes on the existing call path:

• local_function_call wrapped fn->Call in jsg::check(), so a JS exception thrown by the callee unwound across the nounwind FFI frame, aborting the process. The shim is now fallible, tunneling callee exceptions into a jsg::Error that preserves the JS error type and message.
• Global<T> was Send: dropping one on a foreign thread would call global_reset without the isolate lock and corrupt the V8 heap. A raw-pointer phantom now makes it !Send + !Sync.

Tests cover the throw paths (both Local<Function>::call and the typed wrapper), error type and message preservation, non-function rejection, explicit receivers, unit signatures, and clone independence.

[ask-bonk]

ResolveMessage: Cannot find module '@opencode-ai/plugin' from '/home/runner/work/workerd/workerd/.opencode/tools/ci-report.ts'

github run

[ask-bonk]

@guybedford Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

[jasnell]

jasnell note: tried a new review skill to see how it would go... not overly impressed but the findings are worthwhile. Look for my notes interspersed

AI Reviewed the full diff against the C++ implementation.

Verdict

The two safety fixes are real and worth landing. The Function type itself is a solid JS-handle wrapper, but it is not the Rust equivalent of jsg::Function — it is the equivalent of jsg::Function's JsImpl arm only. That gap plus the receiver-semantics divergence are API-shape decisions that are much cheaper to fix now than later. The termination-tunneling mechanism also has a user-triggerable path to killing the isolate.

---

1. Blocking: Function can only hold a JS function

C++ jsg::Function is kj::OneOf<Ref<NativeFunction>, JsImpl> (src/workerd/jsg/function.h:287), and the native arm is load-bearing, not incidental:

• api/streams/encoding.c++:116-168 — TextDecoderStream/TextEncoderStream build a Transformer struct whose transform/flush fields are jsg::Functions constructed from C++ lambdas (JSG_VISITABLE_LAMBDA), then hand that struct to the JS-facing TransformStream.
• api/performance.c++:322 — timerify(js, jsg::Function<void()>) -> jsg::Function<void()>: takes a JS function, returns a native one. This requires ToJS to lazily create a v8::Function (FunctionWrapper::wrap → getOrCreateHandle → attachOpaqueWrapper, function.h:331, :224).
• api/streams/transform.c++:17-21, api/basics.c++:708 ("we create a jsg::Function, wrap that in a v8::Function").

jasnell note: not needed for this PR specifically but it will need to be addressed in some way at some point

Neither of those shapes is expressible with this PR: ToJS (function.rs:632) can only hand back a handle that already exists.

The problem isn't just the missing feature, it's that the current surface makes adding it a breaking change:

• as_local() (function.rs:553) returns Local<Function> unconditionally. With a native arm it must either lazily create the wrapper (needs a per-signature trampoline) or become fallible, cf. tryGetHandle() vs getOrCreateHandle().
• clone(&self, lock) maps to addRef, which for the native arm is a refcount bump on shared state, not a new persistent handle.
• The native arm needs GC visitation of captured state (hasPublicVisitForGc / WrappableFunctionImpl::jsgVisitForGc, function.h:66-70) and the "hold our own reference for the duration of the call" hazard documented at function.h:169-178 — a captured-state design that ignores those will need reworking.

Concretely, pick one:

• (a) Name it what it is now — JsFunction / FunctionHandle — and reserve Function for the OneOf. Cheapest, no future breakage, and stops readers assuming C++ parity from the name and the doc comment ("the Rust counterpart of ... jsg::Function<Ret(Args...)>", function.rs:521).
• (b) Introduce the enum now with only the JS variant populated, make as_local private or Option-returning, and land the native trampoline as a follow-up.

Either way the README section (README.md:344) should say explicitly that this is JS-functions-only today, since it currently reads as a full port.

2. Blocking-ish: default receiver diverges from C++

C++ binds this to the parent object, falling back to context->Global() (function.h:444-446); struct fields pass the struct object as parentObject (struct.h:155-156). This PR always calls with undefined (function.rs:579) and stores no receiver at all — so C++'s setReceiver / JsImpl::receiver has no counterpart.

jasnell note: this one will likely need to be addressed here

That is observable, and it breaks exactly the pattern the new #[jsg_struct] test is modelling:

const options = { state: 1, handler() { return this.state; } };  // works in C++, TypeError in Rust

WHATWG streams require underlying-source/transformer methods to be invoked with the object as this, and workerd gets that for free from parentObject. This can't be fixed inside Function alone — FromJS::from_js(lock, value) has no parent-object parameter — which is why it's worth settling before more code depends on the current signature. At minimum: store an optional receiver in Function, and document the divergence loudly.

3. Termination tunneling: catch (JsExceptionThrown&) is too broad

checkTunneled maps any escaping JsExceptionThrown to TERMINATED_DESCRIPTION. But the JSG_CATCH block itself can throw one: createTunneledException → IsolateBase::unwrapException → ExceptionWrapper::tryUnwrap does check(object->Get(context, "overloaded"_kj)) (jsg/value.h:1432). So:

jasnell note: this one also likely needs to be looked at now

throw { get overloaded() { throw 1 } }   // or a Proxy with a throwing get trap

thrown from a callback invoked through Function::call produces is_termination() == true, and Lock::throw_exception then calls terminate_execution(). That's guest-reachable isolate termination from an ordinary user-level throw.

Worse, JsgCatchScope::catchException has already destroyed its TryCatch (jsg.c++:703) by the time the handler body runs, so that second exception is left pending on the isolate with no TryCatch in the frame — the dangling-pending-exception hazard the existing comment in local_array_iterate warns about.

Fix: don't infer termination from the exception type. Check it (prior art: io/worker.c++:200, api/streams/standard.c++:731):

} catch (::workerd::jsg::JsExceptionThrown&) {
  if (isolate->IsExecutionTerminating()) { /* Terminated */ }
  else { /* pending exception, no TryCatch: capture or Reset it, report internal error */ }
}

That also lets you drop the "we conflate !HasCaught() with termination" caveat, and it addresses the TODO(cleanup) at jsg.h:2854.

4. Encode termination as an exception detail, not a magic string

The house rule is to attach a structured fact to an exception rather than encoding it in the message and matching on a substring later. JSG already does this (TUNNELED_EXCEPTION_DETAIL_ID, JS_EXCEPTION_METADATA_DETAIL_ID — jsg/util.c++:445,457,507), and workerd-cxx exposes KjException::details() on the Rust side.

A dedicated detail id makes the whole "unforgeable because guest errors use jsg. not jsg-internal." argument (ffi.c++ comment, lib.rs:762) unnecessary, removes the "Terminated"-as-error-type-name overload in the tunneling grammar, and removes a string parse from a security-relevant path.

jasnell note: also worth looking at. might not need to be done immediately tho

5. The throw path should be terminate-safe regardless of the flag

Lock::throw_exception's is_termination short-circuit only protects errors that keep the flag. A natural .map_err(|e| Error::new_error(format!("callback failed: {e}"))) drops it, and then:

• err.to_local() → exception_create_from_bytes (ffi.c++:1121) → check(String::NewFromUtf8(...))
• or throw_internal_error → makeInternalError

Both are declared infallible in the bridge (v8.rs:558, :566-568), and NewFromUtf8 returns empty while terminating → JsExceptionThrown across a nounwind frame → abort. Pre-existing, but this PR is what makes the situation reachable from ordinary Rust code, so it's the right place for a one-line guard: have throw_exception / throw_internal_error bail out when IsExecutionTerminating(). That makes the class safe without relying on flag propagation through every From / map_err.

Relatedly, the comment at lib.rs:851 ("Idempotent if termination is already pending; covers the edge case where V8 cleared the flag") — if V8 cleared the flag, re-arming isn't idempotent, it's a new termination of an isolate that had finished unwinding. Say that, or gate it on IsExecutionTerminating().

6. Same bug class, left unfixed in the same file

local_function_call was one instance of a pattern. Still infallible-but-throwing, all reachable from guest JS via Proxy traps / accessors (pre-PR line numbers):

• local_object_set_property (ffi.c++:427) — check(obj->Set(...)), throwing setter → abort.
• local_array_get (:460), local_array_set (:466) — same.
• local_object_has_property (:431) — Has(...).FromJust() on a throwing has trap → CHECK crash.
• local_object_get_property (:437) — swallows the exception and returns kj::none, i.e. a throwing getter is reported as "property absent" and leaves the exception pending.

The invariant is already written down at v8.rs:508-512; these violate it. Fix here or file a follow-up referenced from the PR.

7. impl FromJS for () is a blanket footgun

C++ special-cases void at the call site (if constexpr (isVoid<Ret>()), function.h:95, :437); it deliberately does not define unwrap<void>. The blanket impl in wrappable.rs means () now accepts any JS value everywhere FromJS is used — including as a #[jsg_method] parameter or #[jsg_struct] field type, where it silently swallows anything. Prefer a sealed FunctionResult trait (or handle R = () inside Function::call) so the discard behavior is scoped to where it's meant.

8. Smaller items

• Missing bounds (function.rs:601, :611, :632): Function<i32, i32> compiles fine as a field or parameter and only fails at the call site. Bound the Type/FromJS/ToJS impls on Args: FunctionArgs, R: FromJS so bad signatures fail at the declaration.
• Two heap allocations per invocation: to_js_args returns a Vec (function.rs:661), then Local<Function>::call builds a second Vec of ffi handles (v8.rs:1467). C++ uses a stack LocalVector. Callbacks are per-chunk hot paths; ArrayVec/SmallVec or an iterator-based arg path would avoid both.
• No dynamic arity: FunctionArgs stops at 8 tuple elements and has no Vec<Local<Value>> impl; C++ has Arguments<Value> (function.h:449). Also consider sealing FunctionArgs — as a public trait it locks in the Vec return type.
• Error message: function.rs:617 hardcodes "expected function, got {}" while the type already implements Type; use Self::class_name() to match resource.rs:179.
• Lost error context: C++ reports return-value conversion failures with TypeErrorContext::callbackReturn() (function.h:437); this version surfaces a bare coercion error with no indication it came from a callback's return value.
• Global was already !Sync (it holds an UnsafeCell), so the comment at v8.rs:2905 overstates what changed. Note also that Local/Lock get thread-affinity from NonNull<Isolate> — worth a cross-reference so the next person reaches for the same mechanism.
• Error now carries two bools that aren't orthogonal (termination implies internal). An enum kind would express the invariant and drop the six is_termination: false literals.

9. Tests

Good coverage of the throw paths. Gaps:

• ToJS / ToLocalValue for Function are untested — nothing passes a stored Function back to JS.
• No cross-scope call. The "persistent — safe to store in a resource and call later" claim is only exercised within a single run_in_context; callback_cycle_collected spans two contexts but never calls. A store-in-scope-1 / call-in-scope-2 test is the one that actually validates the persistent handle.
• call_after_terminate_returns_termination_error leaves the harness isolate terminated with no way to clear it (there's no cancel_terminate_execution binding). Worth adding one so the test restores state.
• No test for callee-returns-wrong-type, >2 args, or Option<Function> / Nullable<Function>.
• Reentrancy: function_resource.rs:204-212 and the README snippet both take() the callback, call, then put it back — so a callback that re-enters invoke() sees "no callback set". If that's the intended contract, say so; otherwise the README should recommend RefCell + borrow rather than modelling the footgun.

What's good

• Making local_function_call fallible is a genuine bug fix, and hoisting the shared checkTunneled helper is the right refactor.
• The Global !Send fix is correct and consistent with Local/Lock.
• Cycle-collection and traced-mode tests for a stored callback are the right tests to have written, and the docs on Global's strong/traced duality are unusually clear.