Repository navigation
Add waitUntil, fail-open and abort to the Rust io-context crate - #8088
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8088 +/- ##
==========================================
+ Coverage 68.77% 68.84% +0.06%
==========================================
Files 732 733 +1
Lines 192040 192545 +505
Branches 29905 29997 +92
==========================================
+ Hits 132076 132557 +481
+ Misses 42416 42357 -59
- Partials 17548 17631 +83 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
73517e9 to
0a14de1
Compare
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
LGTM. Calling awaitJs() and exceptionToKj() rather than reimplementing them seems like the right call, and the parity tests are very thorough. A few comments inline. This needs a rebase onto the current base, which has await_io_and_run() and test-bridge.c++ already.
C++ jsg::Lock::terminateExecutionNow() requests a termination, has V8 deliver it by running V8 code, then unwinds with JsExceptionThrown, so no more JavaScript runs after the call: not the rest of the calling function, nor a catch or finally block. Lock::terminate_execution() only requests a termination, which V8 delivers at its next interrupt check, possibly after the caller's JavaScript has gone on. Lock::terminate_execution_now() does what the C++ does up to the unwinding, and returns the error to unwind with instead: a pending error, which a #[jsg_method] returns, setting no return value and throwing nothing. It is #[must_use]. Code that goes on regardless still cannot run JavaScript or replace the termination, since calls into JavaScript refuse to run, and the throw_* methods throw nothing, while execution is terminating. Tests call it from a method inside a JavaScript try/catch, and check that neither the catch block nor the code after it runs.
C++ JSG converts a jsg::Promise<void> argument, and so a kj::Promise<void> one such as ctx.waitUntil() takes, unlike other promise arguments: a promise is taken as is, a thenable is followed only if the unwrap_custom_thenables compatibility flag is on (checked for with `in`, then followed through a resolver), and any other value, a thenable included when the flag is off, becomes a promise resolved with undefined. FromJS for Local<Promise> follows every thenable, whatever the flag. jsg::v8::VoidPromise is a FromJS type that converts as C++ does for Promise<void>, so a Rust method that takes one converts its argument when C++ would, before the method runs, and an omitted argument fails with the same error, naming the type Promise. Tests cover promises, other values, thenables with and without the flag, the order in which a Proxy sees `has` and `get`, a has trap and a then getter that throw, and an omitted argument.
ExecutionContext::abort() created the exception it aborts with when given no reason itself. IoContext::abortWithoutReasonException() now creates it, so that another implementation of ctx.abort() aborts with an identical exception, down to the file and line it records, which appear in logs.
These are the IoContext operations behind ctx.waitUntil(), ctx.passThroughOnException() and ctx.abort(), for a Rust ExecutionContext to match the C++ one exactly. - IoContext::add_wait_until() waits for a promise as part of the request, through IoContext::awaitJs() and addWaitUntil(), as the C++ ExecutionContext::waitUntil() does once JSG has converted its argument; a VoidPromise argument converts it as JSG does. - IoContext::set_fail_open() calls IoContext::setFailOpen(). - IoContext::abort() aborts the IoContext with the reason, encoded by jsg::Lock::exceptionToKj(), or with the default exception, then terminates execution with Lock::terminate_execution_now() and returns its error for the method to return. ctx.abort() terminates right after aborting, so abort() does too, and no caller can forget to. state.abort() in a Durable Object aborts with an exception of its own, after breaking storage, so it will need another entry point. Matching awaitJs() and exceptionToKj() exactly means calling them, so the two shims behind add_wait_until() and abort() use C++ JSG, which Rust JSG's shims otherwise must not. A section of ffi.c++ explains the invariant they rely on. Every use, including getCurrentLock(), which the allowlist test now counts, is in the C++ JSG allowlist as a reviewed exception, so a new shim can't use C++ JSG without raising a count. When C++ JSG throws JsExceptionThrown, as when a getter of the abort reason throws, the shim returns false, and Rust passes the pending exception on with Error::pending(). request-ops-test runs each scenario in a real worker, once on the C++ ExecutionContext and once on a Rust resource that implements the three methods with the crate, and checks that JavaScript, the client and the logs observe the same, including the file and line of the exception the request fails with. Scenarios cover waitUntil() keeping the request alive after the response, with a control without it; a rejected waitUntil() promise, or followed thenable, logged as an uncaught exception; other values and thenables with and without unwrap_custom_thenables; an omitted argument; a has trap that throws; fail-open; abort() without a reason, with an Error, a string, null or a retryable object, and with a reason whose getter throws, after which no JavaScript runs; and calls outside a request. It also checks that the shims reject a stale IoContext id, and that add_wait_until() and abort() do nothing once execution is terminating.
These run on both the C++ ExecutionContext and the Rust one, like the other request-ops-test scenarios: - A rejected waitUntil() promise is handled: no unhandledrejection. - If logging a rejection runs a `stack` getter that throws, the getter's exception rejects unhandled, rather than an internal error, and the wait fails with "Promise will never complete.". - A promise that can never settle has the request canceled as hung. - A promise that another request settles, while the first still runs: without handle_cross_request_promise_resolution the reaction fails in the settling request, as an unhandled rejection, and the wait fails with "Promise will never complete."; with it, the reaction runs in the first request, and the wait completes. The worker records unhandled rejections as events, and scenarios take their compatibility flags as a Flags struct instead of a bool. add_wait_until()'s documentation now says where reactions run and how the wait fails when the promise settles in another request or is garbage-collected.
abort() skipped only the reason case once execution was terminating. Without a reason it still aborted the IoContext and called terminate_execution_now() again, whose JSON::Stringify enters V8 while terminating, which V8 DCHECKs against; C++ ctx.abort() can never get there. It now does nothing while terminating either way, as add_wait_until() does, and request-ops-test checks the no-reason case too.
ffi::add_wait_until() and ffi::abort() map the shims' false, an exception left pending, to Error::pending() themselves, so IoContext::add_wait_until() and abort() no longer branch on a bool. Also wraps abort()'s doc comment at 100 columns.
An ordinary error would throw a JavaScript exception that a catch or finally block could run after. In a callback from V8, catch_panic turns the panic into an internal error and a termination; only code entered through callback_scope! without a handler would abort.
59fc0eb to
3d65e44
Compare
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
4f8b6a77.
This review, of 3d65e44d
💬 Approved with comments — The change is solid and its C++ parity tests are thorough. One WARNING: the public terminate_execution_now() shim enters V8 even when a termination is already pending. No current caller reaches that path.
Findings: 1 warning (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #8088.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Code Quality] `isolate_terminate_execution_now` runs `JSON::Stringify` in the entered context even when a termination is already pending. This breaks the repository rule against entering V8 while terminating, and any Rust caller other than the guarded `IoContext::abort()` would hit a V8 debug check.
- **File:** src/rust/jsg/ffi.c++:1735
📋 Code Quality: ⚠️ 1 warning
terminate_execution_now() enters V8 again when it is called while execution is already terminating.
Recommendation: 📝 Minor Comments
- [WARNING]
isolate_terminate_execution_nowrunsJSON::Stringifyin the entered context even when a termination is already pending. This breaks the repository rule against entering V8 while terminating, and any Rust caller other than the guardedIoContext::abort()would hit a V8 debug check. (src/rust/jsg/ffi.c++:1735)
View full review
The VoidPromise conversion, the io-context shims, the allowlist counts and the parity tests all match the C++ code they mirror. The inline comments from the earlier approval have been addressed in later commits:
abort()now does nothing while terminating (2062df8).- The shim wrappers return
jsg::Result<()>(fc52c43). - The long doc line is wrapped.
- The panic in
terminate_execution_now()now has a documented reason (3d65e44).
The approver also asked for a rebase onto the current base. That is still outstanding.
isolate_terminate_execution_now is not safe while already terminating (src/rust/jsg/ffi.c++:1728-1737)
- The shim always calls
v8::JSON::Stringify(context, ...), even when a termination is already pending. The repository rules forbid calling a V8 API that takes a context in that state: V8 fails a debug check, or in a release build runs on with the termination pending. Lock::terminate_execution_now()is public. As the pull request description notes, Rust can run again after catching a termination. OnlyIoContext::abort()guards against this, withis_execution_terminating(). Any other caller, such as a future Durable Objectstate.abort(), would hit the debug check.- Fix: return early from the shim when
isolate->IsExecutionTerminating()is already true. This also makesabort()'s guard a second layer of defence rather than the only one. A test that callsterminate_execution_now()twice would cover it.
✅ All other checks passed
🔒 Security: ✅ LGTM
No security issues found.
- The C++ shims check that the
IoContextis current throughcurrentWithId(). They catchJsExceptionThrownand leave the exception pending. add_wait_until()andabort()return early while execution is terminating.- The two shims' use of C++ JSG is counted in
cpp-jsg-allowlist.txt.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md issues.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
No documentation issues.
Reviewed commit: 3d65e44d
isolate_terminate_execution_now() ran JSON::Stringify, which takes the entered context, even when execution was already terminating. V8 DCHECKs against entering through PrepareForExecutionScope then, and the shim rules in ffi.h forbid it. Lock::terminate_execution_now() is public and Rust can call it after catching a termination; only IoContext::abort() guarded against that. The shim now returns at once when the termination is already pending. With an ordinary exception pending, the shim still enters V8, and says why: PrepareForExecutionScope clears the exception and the termination takes its place, as in jsg::Lock::terminateExecutionNow(). Not entering would leave the termination undelivered, so JavaScript could catch the exception and run on. jsg-test now calls terminate_execution_now() twice, and after throwing an ordinary exception, and checks that the termination reaches the harness without any catch block or later code running.
There was a problem hiding this comment.
Superseded by the review of
4217e1c5.
This review, of 4f8b6a77
✅ Approved — The only earlier finding, entering V8 while already terminating, is fixed and tested. No new issues found.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
Both shims check the IoContext id with currentWithId() before they touch C++ JSG. They catch JsExceptionThrown and leave the exception pending. Neither shim enters V8 while execution is terminating.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md issues.
📋 Code Quality: ✅ LGTM
isolate_terminate_execution_now now returns early when IsExecutionTerminating() is true (src/rust/jsg/ffi.c++:1731). The ordinary-exception case still enters V8 so that the termination replaces the exception, as C++ terminateExecutionNow() does, and a comment there explains this. The terminate_execution_now() tests cover calling it twice and calling it with an ordinary exception pending. The allowlist counts for the io-context shims match the file.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
No documentation issues.
Reviewed commit: 4f8b6a77
There was a problem hiding this comment.
Superseded by the review of
43c731c7.
This review, of 4217e1c5
✅ Approved — The only earlier finding, entering V8 while already terminating, is fixed and tested. No new issues found.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
Both shims check the IoContext id with currentWithId() before they touch C++ JSG. They catch JsExceptionThrown and leave the exception pending. Neither shim enters V8 while execution is terminating.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md issues.
📋 Code Quality: ✅ LGTM
isolate_terminate_execution_now now returns early when IsExecutionTerminating() is true (src/rust/jsg/ffi.c++:1731). The ordinary-exception case still enters V8 so that the termination replaces the exception, as C++ terminateExecutionNow() does, and a comment there explains this. The terminate_execution_now() tests cover calling it twice and calling it with an ordinary exception pending. The allowlist counts for the io-context shims match the file.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
No documentation issues.
Reviewed commit: 4217e1c5 — same changes as 4f8b6a77, so this is that commit's review (no new review was run; line numbers are as of 4f8b6a77)
There was a problem hiding this comment.
✅ Approved — The only earlier finding, entering V8 while already terminating, is fixed and tested. No new issues found.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
Both shims check the IoContext id with currentWithId() before they touch C++ JSG. They catch JsExceptionThrown and leave the exception pending. Neither shim enters V8 while execution is terminating.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md issues.
📋 Code Quality: ✅ LGTM
isolate_terminate_execution_now now returns early when IsExecutionTerminating() is true (src/rust/jsg/ffi.c++:1731). The ordinary-exception case still enters V8 so that the termination replaces the exception, as C++ terminateExecutionNow() does, and a comment there explains this. The terminate_execution_now() tests cover calling it twice and calling it with an ordinary exception pending. The allowlist counts for the io-context shims match the file.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
No documentation issues.
Reviewed commit: 43c731c7 — same changes as 4f8b6a77, so this is that commit's review (no new review was run; line numbers are as of 4f8b6a77)
# Conflicts: # src/workerd/io/io-context/ffi.c++ # src/workerd/io/io-context/ffi.h # src/workerd/io/io-context/ffi.rs # src/workerd/io/io-context/lib.rs
There was a problem hiding this comment.
Superseded by the review of
ea975690.
This review, of f9736d47
✅ Approved — No issues found. The changes since the last approved review are the merge with main, and they are consistent with the rest of the pull request. The points raised in the earlier discussion are addressed.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
- IoContext check: both shims check the
IoContextid withcurrentWithId()before they use C++ JSG. - Exceptions: they catch
JsExceptionThrownand leave the exception pending. - Termination: no path enters V8 while execution is terminating.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md issues.
📋 Code Quality: ✅ LGTM
- Allowlist counts: the counts in
cpp-jsg-allowlist.txtmatchio-context/ffi.c++with comments stripped, which is how the allowlist test counts them. - APIs used exist: the C++ callees (
exceptionToKj,awaitJs,getCurrentLock) and the Rust helpers (into_ffi_maybe,Error::pending) exist and match how the shims call them. - Build: the
BUILD.bazeldependencies are consistent. - Termination: both termination guards return before touching V8, and tests cover them.
⚡ Performance: ✅ LGTM
No performance concerns. The new operations do a constant amount of work per call.
📝 Documentation: ✅ LGTM
No documentation issues.
Reviewed commit: f9736d47
main requires a TypeInfo implementation for every Rust JSG method parameter type, so VoidPromise now describes Promise<void>, as C++ describes jsg::Promise<void>.
There was a problem hiding this comment.
💬 Approved with comments — The code changes are sound and well tested. One WARNING: the Rust contributor guidance in src/rust/AGENTS.md still says no other C++ JSG shim exceptions are allowed, which this pull request now contradicts.
Findings: 1 warning
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #8088.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [AGENTS.md] src/rust/AGENTS.md:50 still says `io_context_await_io_and_run()` is the only reviewed C++ JSG shim exception and must not be extended. This pull request adds `io_context_add_wait_until()` and `io_context_abort()` as reviewed exceptions, so update the guidance to list them and the conditions that make them sound.
- **File:** src/rust/AGENTS.md:50
📖 AGENTS.md: ⚠️ 1 warning
src/rust/AGENTS.md still says no other C++ JSG shim exceptions are allowed
Recommendation: 📝 Minor Comments
- [WARNING] src/rust/AGENTS.md:50 still says
io_context_await_io_and_run()is the only reviewed C++ JSG shim exception and must not be extended. This pull request addsio_context_add_wait_until()andio_context_abort()as reviewed exceptions, so update the guidance to list them and the conditions that make them sound. (src/rust/AGENTS.md:50)
View full review
src/rust/AGENTS.md:50 says that the only reviewed exception to the no-C++-JSG rule for shims is io_context_await_io_and_run(), and says "don't extend it". This pull request adds two more reviewed exceptions in src/workerd/io/io-context/ffi.c++:
io_context_add_wait_until(), which usesawaitJs()io_context_abort(), which usesexceptionToKj()
Their rationale is documented in src/rust/jsg/README.md and they are counted in the allowlist. Please update the AGENTS.md line to list these exceptions and the conditions under which they are sound, so the guidance no longer contradicts the code.
✅ All other checks passed
🔒 Security: ✅ LGTM
- Both IoContext shims check that the IoContext is current with
currentWithId()before they use C++ JSG. - They catch
JsExceptionThrownand leave the exception pending. - No path enters V8 while execution is terminating.
- The new uses of C++ JSG are counted in
cpp-jsg-allowlist.txt.
📋 Code Quality: ✅ LGTM
No issues found.
- The
VoidPromiseTypeInfo entry inrtti.rsand its export fromv8.rsmatch how C++ describesjsg::Promise<void>. - The
jsg::Promise<void>construction in theio_context_add_wait_untilshim matches the constructor injsg/promise.h. - Both termination guards return before touching V8, and
request_ops_while_terminatingtests them.
⚡ Performance: ✅ LGTM
No performance concerns. Each new operation does a fixed amount of work per call.
📝 Documentation: ✅ LGTM
No documentation issues.
Reviewed commit: ea975690
These are the IoContext operations behind ctx.waitUntil(),
ctx.passThroughOnException() and ctx.abort(), so that a Rust
ExecutionContext can behave exactly like the C++ one.
after the response, through IoContext::awaitJs() and addWaitUntil().
A rejection is logged as an uncaught exception, as in C++.
jsg::Lock::exceptionToKj(), or with the default exception, then
terminates execution and returns the error for the method to return.
ctx.abort() always terminates right after aborting, so abort() does
too, and no caller can forget to; it is #[must_use]. The default
exception is now created by IoContext::abortWithoutReasonException()
for both implementations, so the logged file and line match.
Once execution is terminating, add_wait_until() and abort() do nothing
and return a pending error. JavaScript can't call them then, but Rust
can after catching a termination.
Two additions to Rust JSG support these:
terminateExecutionNow(). It requests termination and runs V8 code so
that it is delivered at once, then returns a pending error instead of
unwinding with JsExceptionThrown. A #[jsg_method] that returns it
throws nothing, and no more JavaScript runs, including catch and
finally blocks. If execution is already terminating, it doesn't enter
V8 again; a pending ordinary exception is replaced by the
termination. It panics if V8 doesn't deliver the termination, as C++
asserts, since an ordinary error would let a catch or finally block
run.
C++ JSG converts jsg::Promise (and so kj::Promise, which
ctx.waitUntil() takes): a promise is taken as is, a thenable is
followed only if unwrap_custom_thenables is on, and anything else
becomes a promise resolved with undefined. FromJS for Local
follows every thenable whatever the flag, so it doesn't match.
Rust JSG's shims must not use C++ JSG, but io_context_add_wait_until()
and io_context_abort() call IoContext::awaitJs() and
jsg::Lock::exceptionToKj() as a reviewed exception. Reimplementing them
over V8 would drift from C++ in ways tests are unlikely to catch:
rejection logging, the onAbort join, the tunneled-exception encoding,
and the retryable/overloaded details. This is sound because the shims
run only after currentWithId() has checked that the IoContext is
current. They then run synchronously under the isolate lock the
caller's jsg::Lock stands for, with handles from the caller's
HandleScope. The "C++ JSG" section of io-context/ffi.c++ spells this
out. Every use is counted in cpp-jsg-allowlist.txt, and the allowlist
test now also counts getCurrentLock(), so a new shim can't reach C++
JSG without raising a count. When C++ JSG throws JsExceptionThrown, the
shim returns false and Rust passes the pending exception on with
Error::pending().
request-ops-test runs each scenario in a real worker twice, on the C++
ExecutionContext and on a test-only Rust resource built on the crate.
It checks that JavaScript, the client and the logs observe the same
things: status, events, RequestObserver failures (including the
exception's file and line), fail-open state, subrequests and logged
uncaught exceptions. Scenarios cover:
with and without handle_cross_request_promise_resolution
terminating
jsg-test covers terminate_execution_now() and VoidPromise.
DurableObjectState can reuse add_wait_until() but not abort():
state.abort() aborts with its own exception and breaks storage first,
so it will need a lower-level entry point.