guardFailures recurses unbounded, so a deep result reports a successful call as a 500

[frenzzy]

Summary

guardFailures walks the result recursively and unbounded, so a deep-but-legal return value overflows the stack. The RangeError is caught outside encodeResult's own try, and the caller is told the call failed — after the function ran and returned successfully.

Tested against next @ fa137614, built from source, Node 24.19.

Reproduction

const deep = n => { let o = {}; const root = o; for (let i = 0; i < n; i++) { o.n = {}; o = o.n; } return root; };

let ran = 0;
registerServerFunction("deep", async () => { ran++; return deep(20000); });

fn ran = 1   status = 500   X-Server-Function-Error = Internal Server Error

The function executed, its result is a legal object graph, and the client receives a generic 500. Shallower depths (100 / 1 000 / 5 000) answer 200; a wide-but-shallow value (a 20 000-element array) also answers 200, so this is depth, not size.

This is the exact hazard the tree already documents

isJSONSafe was rewritten iteratively for this reason, and its comment in shared.ts says so — that a recursive walk overflows, and that the resulting RangeError "escaped into dispatch's catch as a phantom function error". guardFailures kept the recursion, so the same phantom error is still reachable by the other road.

It also arrives before the codec's own depth cap could produce a structured error: guardFailures runs on the codec road, which is exactly the road taken for values deeper than JSON_SAFE_DEPTH_LIMIT.

guardFailures already guards against this exact failure class — for a different cause

The strongest argument for fixing it is inside the function itself. Its comment on why it reads data properties only, rather than through getters:

reading through a getter would invoke it here as well as when the codec encodes, and a throwing one would escape into dispatch's catch to be reported as the function itself failing — the phantom error over a call that succeeded.

So "a phantom error over a call that succeeded" is a named, understood hazard that this function deliberately avoids by one route while its own recursion reopens by another. The fix is not introducing a new invariant; it is completing one the code already states.

Why it matters

This is the #3117 failure shape again: a successful, committed call reported as a failure. For a non-idempotent mutation that invites a retry of work that already happened. The generic message gives the author nothing to act on, and the value is not obviously pathological — a recursive tree, a linked structure, or an ORM graph with a deep association chain gets there without anyone intending it.

Reachability: ordinary application data, no attacker required. Depth ~10k+; the exact cliff is stack-state dependent (20 000 overflowed from one call site and survived from a shallower frame; 50 000 always breaks).

Note the thrown path is not affected the same way: guardFailures deliberately skips class instances, so a deep property hanging off a thrown Error is never walked.

Options

1. Make guardFailures iterative, the way isJSONSafe already was, for the same reason. Largest behavioural fidelity — the walk visits the same nodes in the same order, it just carries its own stack.
2. Bound the walk's depth and stop descending past the codec's own limit. Cheaper, and defensible: anything deeper is going to be refused by the codec anyway, so guarding it is wasted work. Changes what happens to a deep value that the codec would have accepted on the JSON road.
3. Move the RangeError inside encodeResult's try so it becomes a structured encode error rather than a phantom function error. Does not fix the overflow, but it converts a misattributed 500 into an honest one — worth doing regardless of 1 or 2, since it also covers whatever else can throw there.
4. Document a maximum result depth. Weakest: the limit would be discovered in production, and the current failure does not name it.

I'd suggest (1) — it matches the precedent set for isJSONSafe and removes the cliff rather than relocating it — with (3) alongside so misattribution cannot recur from another cause.

Happy to send a PR. The durable test is a depth sweep asserting a legal deep result round-trips, plus an assertion that the function ran, which is what distinguishes this from an ordinary encode failure.

[vansh2408]

/assign

[ryansolid]

Fixed in 1d2d1e5 with your (1) + (3), for the reasons you laid out — completing the invariant the code already states rather than adding a new one.

(1) guardFailures' container descent is now iterative: leaves and channel wrappers resolve immediately, synchronous containers (array/map/set/plain object) drive explicit frames on a heap stack. The seen/cyclic identity contract, the rebuild-stands (keepGuarded) semantics, visit order, and the data-properties-only policy all carry over unchanged — the channel callbacks were never part of the problem since they re-enter from fresh async stacks. Regression pins: a depth sweep at 20k and 200k asserting the function ran once and answered 200 with no error header (the cliff is gone, not relocated), plus a cycle-with-nested-rejection round-trip proving back-reference identity and sanitization survived the rewrite.

(3) encodeResult's codec road now wraps the synchronous half (the walk plus Response construction) and renames any residual throw to Server function result could not be encoded: … before rethrowing — dev keeps the cause, production sanitizes downstream, and nothing from that seam can be attributed to the function again. You were right that this is worth having independently: post-rewrite the remaining reachable throws there are user-hostile shapes like a throwing custom Symbol.iterator on a Map subclass, which no walk can avoid invoking.

Your observation that the function's own getter-policy comment named the exact hazard its recursion reopened was the whole review in one line. Thanks.