Merging this PR will not alter performance

✅ 70 untouched benchmarks
⏩ 129 skipped benchmarks¹

_{Comparing maizatskyi/2026-04-08-exception-or-nothrow-move (38ce901) with main (121f7f9)}

@mikea Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

The review has been posted to PR #6536. Here's a summary of the findings:

1. [HIGH] Ref::operator=(Ref<U>&&) at src/workerd/jsg/jsg.h:1285 is marked noexcept but calls destroy() which transitively calls deferDestruction() containing KJ_REQUIRE assertions that throw on failure. This would cause std::terminate(). I posted a suggestion to remove noexcept from this operator.

2. [MEDIUM] AsyncGenerator move constructor at src/workerd/jsg/iterator.h:178-180 is marked noexcept but calls kj::rc<>() which heap-allocates. A failed allocation would call std::terminate().

3. [LOW] Ref move constructors have a noexcept-unsafe addStrongRef() fallback path, though this is in a code path already documented as UB.

github run

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.81%. Comparing base (121f7f9) to head (38ce901).
⚠️ Report is 2 commits behind head on main.

@@            Coverage Diff             @@
##             main    #6536      +/-   ##
==========================================
- Coverage   70.82%   70.81%   -0.01%     
==========================================
  Files         435      435              
  Lines      122654   122659       +5     
  Branches    19267    19267              
==========================================
- Hits        86869    86864       -5     
- Misses      24371    24384      +13     
+ Partials    11414    11411       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

NonModuleScript is stored in a kj::OneOf<jsg::NonModuleScript, kj::Path> at worker.c++:878. When the linked KJ PR lands and kj::OneOf requires noexcept-movable variants, NonModuleScript MUST be noexcept-movable. So it's correct to mark it noexcept now.

But wait — if v8::Global's move ctor isn't noexcept, this would be deleted. The same question applies to Data(Data&&) noexcept which also moves a v8::Global. Since CI passes, v8::Global's move ctor must in fact be noexcept in the version of V8 used here. In that case, GlobalToDelete is actually unnecessary — v8::Global<v8::Data> could stay in the OneOf directly.

Actually wait, I should check more carefully. The Data(Data&&) noexcept is user-defined, not defaulted. A user-defined noexcept function that calls a potentially-throwing function won't cause a compile error — it just calls std::terminate() if an exception escapes. Only defaulted functions are deleted when the noexcept doesn't match.

So the question for NonModuleScript is: is the defaulted move constructor deleted? If v8::Global<v8::UnboundScript>'s move ctor is NOT noexcept, then NonModuleScript(NonModuleScript&&) noexcept = default; would be deleted. But CI passes, which means either:

1. v8::Global's move IS noexcept, or
2. NonModuleScript's move is never actually invoked (it would be deleted silently)

For case (2), the kj::OneOf at worker.c++:878 would need to move NonModuleScript. Looking at kj::OneOf's current move ctor (not noexcept, no constraints), it doesn't need NonModuleScript's move ctor to exist at compile time unless the OneOf is actually move-constructed. If it IS move-constructed, then the template instantiation would fail. But kj::OneOf::moveFrom uses a placement-new expression, which would fail to compile if NonModuleScript's move is deleted... unless the compiler doesn't instantiate that template variant.

Actually, looking at kj::OneOf::moveVariantFrom<T>, it's a template method that's instantiated for ALL variants in the pack expansion. The line ctor(*reinterpret_cast<T*>(space), kj::mv(other.get<T>())) would be compiled even if T = NonModuleScript and its move ctor is deleted. So this would be a compile error.

Since CI passes, I can conclude that v8::Global<T>'s move constructor IS noexcept. This means GlobalToDelete is technically unnecessary — v8::Global<v8::Data> could remain directly in the OneOf. However, GlobalToDelete serves as a future-proofing measure in case V8 changes, and as documentation of intent. It's not wrong, just perhaps overly cautious.

Let me now do my final assessment and post the review.

After thorough analysis, my previous review findings were:

1. [HIGH] Ref::operator=(Ref<U>&&) noexcept — FALSE POSITIVE. The operator was never marked noexcept in this PR.
2. [MEDIUM] AsyncGenerator kj::rc allocation — Author correctly pointed out that workerd doesn't support throwing allocations (tcmalloc terminates on OOM).
3. [LOW] Ref addStrongRef in noexcept — Already acknowledged as borderline, only reachable in documented-UB scenarios.

On re-review, the code changes are all correct. The assertInvariant() removal from Data's move ctor is justified since KJ_IASSERT could technically throw, and the invariant trivially holds after the move operations anyway.

The GlobalToDelete wrapper is a reasonable defensive measure even though v8::Global appears to be noexcept-movable in the current V8 version.

All the trace.h changes are straightforward defaulted noexcept move constructors for types containing only KJ value types.

LGTM

github run