Async entries: one failure channel, decided by the failure and not by a thread race

[lahma]

Closes #3241.

Engine.EvaluateAsync could surface a constraint failure synchronously — out of the call itself — rather than through the returned task, decided by a thread race the host cannot see. A host writing var t = engine.EvaluateAsync(src); … await t; reasonably wraps the await, not the call, and under load its budget failure bypassed that handler entirely.

This has blocked CI on two unrelated PRs today.

The finding that reframed the issue

Probing every failure mode of every *Async entry showed the family was already split — by nothing more principled than which methods happened to be declared async.

ExecuteAsync and Modules.ImportAsync were, so their whole body reached the task. EvaluateAsync and InvokeAsync were not, so the identical failure on the identical script erupted from the call. Nobody chose that.

That is why the issue's direction 2 — "leave the behaviour and document it" — was not viable: there was no single behaviour to document.

The rule

A usage error is thrown out of the call; everything the operation does is delivered through the Task.

Synchronous — usage errors only. A null argument, a Prepared<Script> that did not come from PrepareScript, and the concurrent-use InvalidOperationException. All three mean the operation never started, so there is no evaluation for a task to describe. This is the carve-out TAP states explicitly ("TAP methods throw exceptions out of the method call only in response to a usage error"), it is already what Advanced.WaitForScheduledWorkAsync does, and HostEngineConcurrencyTests.ConcurrentAsyncEntryIsRejectedWithoutInterruptingTheOwner already pinned it for EvaluateAsync.

Task — everything else. Parse errors, script throws, every constraint, promise rejections. The operation started and failed, and the host that wrapped its await must see it.

Two judgement calls worth naming:

• Parse errors move to the task, though they never raced. Leaving them synchronous would force a host to write both a try around the call and one around the await — the worst of both worlds. A syntax error is work the operation did, not an argument check.
• Argument validation had to be made synchronous, not merely left so. The string overloads had no null check at all — code.Length NRE'd inside the parser, i.e. in the body. Under a naive "wrap the body" fix that NRE would have moved to the task and quietly contradicted the rule.

Every entry now validates → reserves → hands off to a private async body owning the release in a finally. A 17-line INVARIANT comment records why the split exists and says plainly: do not move a reservation into a body, and do not hoist work out of one.

Sibling audit

sibling	outcome
EvaluateAsync ×2, InvokeAsync ×2	fixed — shared the shape exactly
ExecuteAsync, Modules.ImportAsync	admission hoisted to synchronous, for the rule
JsValue.UnwrapIfPromiseAsync	fixed — body now on the task
Advanced.WaitForScheduledWorkAsync	already conformed; it was the in-repo precedent
Modules.StartImport	returns an operation, not a Task — n/a
CallAsync	does not exist; the issue named it speculatively

Public behaviour changes — all deliberate, all listed

1. EvaluateAsync and InvokeAsync now fault the task for parse errors, script throws and every constraint failure. This is the fix.
2. ExecuteAsync and Modules.ImportAsync now throw the concurrent-use InvalidOperationException synchronously, where they used to fault the task. This is the direction that makes a host wrap more, so it deserves calling out. It was chosen because the alternative left the family split, and because three of the five entries already behaved this way with a pinned test and a documented rationale — the minority moved to the majority, not the reverse. No test depended on the old behaviour.
3. EvaluateAsync(null) / ExecuteAsync(null) / InvokeAsync(null, …) now throw ArgumentNullException where they threw NullReferenceException from inside the parser. Strictly an improvement, but it is a type change.

No signature changed anywhere. ExecuteAsync/ImportAsync losing the async keyword is an implementation detail.

Evidence

Deterministic, not just statistical — a host function that awaits the pool thread's task before returning, so the callback's charge always lands before the engine thread reaches the post-script check:

	pre-fix	post-fix
forced-ordering case	synchronous throw, 100%	faulted task, 100%

Repetition of the issue's own scenario, 6000 runs per process:

	sync throws	task faults
pre-fix, idle machine	1 / 6000	5999
pre-fix, loaded machine	5 / 6000	5995
post-fix	0 / 6000	6000

The load sensitivity is the point: the window widens under contention, which is why CI hits it and a laptop usually does not.

One finding worth recording from building the repro: a first attempt using a bare new Thread(...).Join() had the callback's own memory check trip unhandled on that thread and kill the xUnit test host outright. Task.Run captures it instead, which is why the shipped test uses Task.Run + Wait().

The constraint is unchanged

TheTaskCarriesTheSameExceptionTheSynchronousEntryThrows runs one script through Evaluate and through EvaluateAsync and asserts same type and Message string-equal, then asserts the script did not run past the trip point. TheBudgetStillFiresWhenTheFailureOnlyReachesTheTask writes the exact host shape from the issue — call unwrapped, catch around the await only — and asserts the budget still fires.

Type and message identity confirmed across the move for all five constraint kinds: TimeoutException, StatementsCountOverflowException, MemoryLimitExceededException, RecursionDepthOverflowException, ExecutionCanceledException.

No catch clause was added anywhere — the old catch { Release; throw; } became finally { Release; }, which is propagation-identical. Nothing can be swallowed that could not be before.

The named test now asserts the channel

TransferredHostCallbackCountsAllocationsOnItsThread had its EvaluateAsync(…) call outside Record.ExceptionAsync, which is what let the raw exception escape. It now wraps the call in Should().NotThrow("a budget failure belongs on the returned task, whichever thread trips it") and then awaits — so it fails loudly on a channel regression rather than passing by luck.

Verification

Jint.Tests 10342 (net10.0) / 7206 (net472), identical with JINT_HOST_CONTRACT_VERIFICATION=1. Jint.Tests.PublicInterface 2427 / 1899, and 2431 / 1903 verifying. New HostAsyncFailureChannelTests: 15 of 20 cases fail against the unfixed engine, 20/20 pass with the fix.

test262: 102,495 passed / 0 failed on both this branch and an upstream/main baseline run on the same machine — identical, no movement.

Documentation: an AGENTS.md contract-table row and a Gotchas bullet stating the rule and the two "never do this" mechanics, plus a new README embedder section, Where failures arrive, with the catch-around-the-await example.

Left undone

• Advanced.WebApi.InvokeFetchHandlerAsync / CopyReadableStreamAsync are async throughout, so everything — including their own ArgumentNullException — reaches the task. Consistent-but-stricter and never racy, so a defensible stopping point rather than this defect. Left because Fetch: the iterator attributes, the disturbed-body lock, and WebIDL's record order #3247 was landing in that area; the follow-up is one hoist of a null check.
• The synchronous Evaluate/Execute/Invoke still NRE on a null script. Only the async entries gained validation, because only they have a before-body/after-body distinction to keep true.
• One unreachable edge: if EnterHostCall itself threw, its InvalidOperationException would now arrive via the task. It can only throw if another thread claimed _ownerThreadId between the successful reservation and the claim, which the reservation's own double-check makes effectively unreachable.

🤖 Generated with Claude Code