Keep counting a frame a tail call replaced while its trampoline runs

AGENTS.md

@@ -256,6 +256,7 @@ Each of these cost a real integrator or a real bug.
 - **Cancellation is the one thing that does span host calls, because the amortized check *cadence* is engine state.** It is the sibling of the entry above and resolves the other way. A budget must reset per run; a *cadence* must not. `Engine._amortizedConstraintCountdown` therefore lives on the engine and is deliberately **not** rewound by `ResetConstraints()`, so the statements of one short call carry the countdown forward into the next and detection latency stays bounded at `EvaluationContext.AmortizedConstraintCheckInterval` (64) statements *for the engine*, not for one entry. It used to live on `EvaluationContext` and restart at 64 in the constructor, and since every top-level entry builds a fresh context, a callee shorter than 64 statements never reached an amortized check **at all** — which silently defeated `CancellationConstraint.Reset()` being a no-op, the whole point of which is that cancellation survives across entries. A one-statement predicate invoked after `cts.Cancel()` now throws within 64 calls instead of never. Two consequences worth holding on to: this bounds *latency*, never a budget — `TimeoutInterval` is still re-armed per entry and still does not fire across a host loop — and a nested re-entry shares the countdown rather than restarting it, which is correct for a cadence and is what stops a host callback re-entering the engine from resetting the poll interval on every iteration. Any constraint that declares `IsAmortizable => true` inherits all of this, including a user-derived one.
 - **The handler-tree caches engage only on the *second* evaluation of a given script on a given engine.** A host that builds a fresh engine per operation never reaches them, by design. This concerns that cross-run carry-over only — caches which are not engine-scoped still pay back, and CLR member resolution in particular now survives across engines through the shared `TypeResolver` described above, which is the main reason a fresh engine per operation costs less than it used to. `Engine.Advanced.RestoreGlobalSnapshot` deliberately **preserves** these caches (`_scriptStatementLists`, `_functionDefinitions`, `_evaluatedScripts`) while resetting the globals — reaching them from a host that used to need a fresh engine per evaluation is the entire point of that API, and `Jint.Tests/Runtime/GlobalSnapshotInternalsTests.cs` pins it.
 - **A warmed member-read site retains its last receiver.** The prototype-method inline cache stores the receiver, its direct prototype and the resolved descriptor on the handler node (`JintMemberExpression._cachedProtoReceiver`), and only replaces that entry when the same site later caches a *different* receiver — a miss that does not qualify for caching leaves the old one in place. Handler trees are engine-owned and survive between evaluations on a re-running engine, so a pooled engine can hold one host object alive per warmed call site until its next run. Nothing clears these: `Engine.Advanced.ResetCallStack` releases call frames only. A host whose receivers wrap large native state that must not outlive a run should drop the engine rather than pool it. **A warmed *call* site retains its last callee the same way, and one of the two caches is heavier than it looks.** `JintCallExpression._fastCallee` (the built-in fast-call lane) usually holds a realm intrinsic, which the realm roots anyway — but the field is written for whatever `Function` an eligible site dispatched, before and regardless of the shape verdict, so a site with at most two arguments and no spread calling an *interpreted* function pins that too. `_regCallee` (the register-argument lane for interpreted callees) holds a `ScriptFunction` by construction, and a `ScriptFunction` holds the environment it closed over — so a warmed site pins one closure instance, and transitively whatever that closure captured, for the engine's lifetime. It is the same bounded, one-entry-per-site hazard, with the same remedy: drop the engine rather than pool it when the retained graph matters. The bound is per site, not per callee: a polymorphic site overwrites its entry rather than accumulating. The sibling `_regProbedCallee` records what the probe last examined so a rejected callee is not re-probed on every dispatch, and it retains that reference too — for an accepted callee it is the same object as `_regCallee`, so the ceiling stays one live callee per site either way. **Which sites are in the retaining set is decided at build time**, by `_fastArgsEligible` and `_regLaneEligible`: a site with five arguments, or with a spread, can arm neither lane and so records nothing at all. Note also that a site only enters the set once its handler tree outlives the evaluation, i.e. from the second run of the same program on the same engine — a host re-parsing its source every time never warms one, and a `WeakReference` test written against `Execute(string)` therefore proves nothing.
+- **`MaxRecursionDepth` counts one function's occurrences, not stack depth — and a tail call's displaced frame keeps counting.** `JintCallStack._statistics` is a multiset of the live stack keyed by `JintFunctionDefinition` identity (`CallStackElementComparer`), so the limit asks "how many times is *this* function on the stack", never "how deep is the stack". Two consequences. A recursion whose every level is a function created for that level — `eval`, `new Function`, a host re-running a script through a callback — repeats no definition and is invisible to the limit however deep it goes; `Options.Constraints.StackOverflowGuard` is the only thing that covers that shape, and no version of Jint has covered it any other way. And `JintCallStack.ReplaceTop` deliberately does **not** discount the frame a proper tail call replaced: the frame is gone, but the activation is not over while `ScriptFunction.ContinueTailCalls` is still on the native stack, so the occurrence is retained and handed back through `ReleaseTailRetention` in that trampoline's `finally`. Discounting it (which is what the code did through 4.16.0) made the limit blind to a recursion that leaves and re-enters the trampoline by a non-tail route — a getter, `new`, a coercion, a Proxy trap, a host callback — because every re-entry pushed the displaced function again at depth zero while the native stack kept growing, and `LimitRecursion` then ended in a process-killing stack overflow instead of a `RecursionDepthOverflowException`. Pinned from both sides in `Jint.Tests.PublicInterface/HostTailCallTests.cs` and `Jint.Tests/Runtime/TailCallOptimizationTests.cs`; the release half has its own test, because without it a loop over one *completed* tail delegation accumulates against the limit.
 - **Saturated sentinels register nothing.** `MaxStatements(int.MaxValue)`, `LimitMemory(long.MaxValue)` and `TimeoutInterval(TimeSpan.MaxValue)` — and any non-positive value, including `MaxStatements()`'s own parameter default — produce exactly the same engine as never calling the method, and additionally *remove* any previously registered constraint of that kind. A host spelling "effectively unlimited" that way has no limit, not a very large one.
 - **`RestoreGlobalSnapshot` bumps version counters, it never restores them.** `_propertiesVersion`, `GlobalEnvironment._lexicalMutations`, `Engine._envBindingInjectionEpoch` and `EventLoop.Generation` are what every inline cache — and, for the last one, every queued job — validates against, so putting a counter *back* could make an entry built before the capture compare equal again and be revalidated against state it never saw. Anything added to the restore path obeys the same rule. The API's other half is its honesty: it reverts the global *binding table*, and explicitly not intrinsic/prototype mutations, object graphs behind restored bindings, host CLR state (including `Engine.Advanced.HostDefined`, which a pooled engine keeps across a restore and the host swaps per request itself), `Symbol.for`, or the module registry — it is a configuration-reuse primitive, not an isolation boundary, and the non-guarantees are pinned as surviving in `Jint.Tests.PublicInterface/GlobalSnapshotTests.cs`. Since bare identifiers resolve through the global's whole prototype chain, surviving intrinsic pollution is now a surviving **binding** as well as a surviving property: `Object.prototype.leaked = 1` in one cycle makes `leaked` resolvable as a free identifier in the next, where before it was readable only as `globalThis.leaked` and `typeof leaked` answered `"undefined"`. The global's own `[[Prototype]]` is a different matter and *is* captured and restored.
 - **Discarding the event loop is a fence, not a flush.** `EventLoop.Clear()` can only throw away what is already queued, and the case that matters is the one where nothing is: a fire-and-forget async function suspended on a CLR `Task` enqueues its settle whenever that task happens to complete, which can be after a restore — and the resumed body then writes the previous cycle's data into the restored globals, a cross-cycle channel the fresh-engine-per-evaluation pattern never had. The fix is a generation captured at promise **registration** (engine thread), carried in the `EventLoopJob`, and checked at **dequeue** (engine thread): both ends are ordered by the single-thread contract, where a check inside the settle closure would race the restore. Any new enqueue path must stamp the registering cycle's generation, not the current one, whenever the two can differ. The consequence for hosts is real and documented: a promise registered before a restore never settles into the engine afterwards.

Jint.Tests.PublicInterface/HostTailCallTests.cs

@@ -50,4 +50,67 @@ function second() {
 
         result.Should().Be(42);
     }
+
+    /// <summary>
+    /// A recursion that leaves the tail-call trampoline through a non-tail route — here a property
+    /// getter, but <c>new</c>, a coercion, a Proxy trap and a host callback are the same shape — and
+    /// re-enters it, must still be stopped by <c>LimitRecursion</c>. The native stack grows on every
+    /// pass because <c>calc</c>'s read of <c>entity.calc</c> is not in tail position, so nothing else
+    /// can stop it: without the limit firing the host process dies on a stack overflow no
+    /// <c>catch</c> can see.
+    /// <para>
+    /// The host re-declaring <c>calc</c> on every pass is load-bearing rather than incidental. The
+    /// limit counts occurrences of one function <em>definition</em> on the call stack, so a stable
+    /// <c>calc</c> would be counted and would stop this on its own; a freshly parsed one is counted
+    /// once per level, which leaves the getter as the only function that repeats — and the getter is
+    /// exactly the frame the tail call replaces.
+    /// </para>
+    /// </summary>
+    [Fact]
+    public void RecursionLimitStillFiresWhenATailCallIsOnThePath()
+    {
+        var engine = new Engine(options => options
+            .LimitRecursion(20)
+            .TimeoutInterval(TimeSpan.FromSeconds(10)));
+
+        engine.SetValue("load", new Action(() => engine.Execute("function calc() { return entity.calc; }")));
+        engine.Execute("""
+            "use strict";
+            var entity = {
+                get calc() {
+                    load();
+                    return calc();
+                }
+            };
+            """);
+
+        Invoking(() => engine.Evaluate("entity.calc"))
+            .Should().ThrowExactly<RecursionDepthOverflowException>();
+    }
+
+    /// <summary>
+    /// The counterpart to the test above: a tail call that <em>completes</em> must give its
+    /// displaced caller back to the recursion budget, or a loop calling one bounded tail delegation
+    /// would accumulate against the limit and fail on its second iteration.
+    /// </summary>
+    [Fact]
+    public void CompletedTailDelegationDoesNotAccumulateAgainstTheLimit()
+    {
+        var result = new Engine(options => options.LimitRecursion(0)).Evaluate("""
+            "use strict";
+            function leaf() {
+                return 1;
+            }
+            function helper() {
+                return leaf();
+            }
+            var total = 0;
+            for (var i = 0; i < 100; i++) {
+                total += helper();
+            }
+            total;
+            """);
+
+        result.Should().Be(100);
+    }
 }

Jint.Tests/Runtime/TailCallOptimizationTests.cs

@@ -491,6 +491,63 @@ function third() {
         engine.CallStack.Count.Should().Be(0);
     }
 
+    /// <summary>
+    /// A tail call replaces its caller's frame, but the caller's activation is not over while the
+    /// trampoline runs — and here the tail-call target leaves it again through a getter, which is not
+    /// a tail position and so really does grow the native stack. The limit has to keep seeing the
+    /// displaced getter across those re-entries; when it did not, this ran until the process died.
+    /// <para>
+    /// Each <c>calc</c> is deliberately a definition of its own. The limit counts occurrences of one
+    /// function definition, so a stable <c>calc</c> would be counted and would stop the recursion by
+    /// itself, leaving the defect untested.
+    /// </para>
+    /// </summary>
+    [Fact]
+    public void RecursionLimitFiresWhenATailCallReEntersThroughAGetter()
+    {
+        var engine = new Engine(options => options.LimitRecursion(20));
+
+        Invoking(() => engine.Evaluate("""
+            "use strict";
+            var version = 0;
+            var entity = {
+                get calc() {
+                    (0, eval)("function calc() { return entity.calc; } //" + version++);
+                    return calc();
+                }
+            };
+            entity.calc;
+            """)).Should().ThrowExactly<RecursionDepthOverflowException>();
+
+        engine.CallStack.Count.Should().Be(0);
+    }
+
+    /// <summary>
+    /// The depth statistic must come back to zero after the limit fires, not only the frame stack. A
+    /// retention the trampoline failed to hand back is invisible in <c>CallStack.Count</c> and shows up
+    /// only later, as a second run of the same functions overflowing before it has recursed at all.
+    /// </summary>
+    [Fact]
+    public void RecursionLimitFailureLeavesTheDepthStatisticBalanced()
+    {
+        var engine = new Engine(options => options.LimitRecursion(0));
+        engine.Execute("""
+            "use strict";
+            var stop = false;
+            function first() {
+                return stop ? 42 : second();
+            }
+            function second() {
+                return first();
+            }
+            """);
+
+        Invoking(() => engine.Evaluate("first()")).Should().ThrowExactly<RecursionDepthOverflowException>();
+        engine.CallStack.Count.Should().Be(0);
+
+        engine.Evaluate("stop = true; first();").Should().Be(42);
+    }
+
     [Fact]
     public void DebugModeKeepsTailCallerFrame()
     {