[ngxson]

Overview

As explained in #30242 (comment)

All recursive function in the code base must now come with a hard limit. code-review skill checks for it.

Additional information

Component	Limit constant	Value	Defined in	Bounds
libcommon	COMMON_RECURSIVE_LIMIT	128	common/common.h	JSON schema builder (build_node), schema regex pattern nesting, PEG parse (re-entries of each rule), glob_match (number of wildcards)
jinja	JINJA_RECURSIVE_LIMIT	128	common/jinja/utils.h	input JSON (from_json), nested macro and caller() calls
grammar	GRAMMAR_RECURSIVE_LIMIT	256	src/llama-grammar.h	nested ( groups (parse_sequence), rule chains (detect_left_recursion)
server decision	RECURSIVE_LIMIT	128	tools/server/server-decision.cpp	the five decision_* helpers (replace_text, sort_keys, kev_render, d1omni_escape, py_dump)

Requirements

• I have read and agree with the contributing guidelines
• AI usage disclosure: yes

[ggerganov]

The llama-grammar changes are OK

[aldehir]

Slight problem, on the MSVC Debug build, the 128 limit is still too much for some of these.

Test	Debug	Release
test-peg-parser	crash (deep JSON)	pass
test-jinja	crash (macros.endless macro recursion)	crash (same test)
test-grammar-integration	pass	pass
test-json-schema-to-grammar	pass	pass
test-server-component	doesn't compile	not built

test-peg-parser crashes because the node carries a lot, and test-jinja crashes because the thrown exception balloons the stack.

Build	Stack	test-peg-parser	test-jinja
MSVC Debug	1 MB	crash	crash
MSVC Debug	8 MB	pass	pass
MSVC Release	1 MB	pass	crash
MSVC Release	8 MB	pass	pass
clang Release	1 MB	pass	crash
clang Release	8 MB	pass	pass

I recommend we increase the total stack size for Windows builds from 1MB to 8MB to match glibc. This might not be as straightforward.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index d091a1411..767fdc229 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -80,6 +80,13 @@ option(BUILD_SHARED_LIBS "build shared libraries" ${BUILD_SHARED_LIBS_DEFAULT})
 
 if (WIN32)
     add_compile_definitions(_CRT_SECURE_NO_WARNINGS)
+
+    # match the 8 MB default stack on Linux
+    if (MINGW)
+        add_link_options(LINKER:--stack,8388608)
+    else()
+        add_link_options(LINKER:/STACK:8388608)
+    endif()
 endif()
 
 if (MSVC)

[aldehir]

I don't think the changes here are needed in this file if the AST is already bounded by the parser. Just more work when we're given safe input.

[ServeurpersoCom]

I still have some tests to run...

I found a side effect: some vocabs have very long tokens, like GLM-4.7-Flash with a single token for 512 spaces. With the new limit in the grammar code, any token of 256 chars or more is now always blocked when a grammar is active, while master allows it. The limit counts the chars of a vocab token here, not request input, so it probably needs to follow the longest token of the vocab instead.

[ServeurpersoCom]

Pushed a fix for the long token issue: instead of a limit following the vocab, the rejection is now a loop, so there is no limit at all and the masks match master again. LGTM now!

[ServeurpersoCom]

Follow-up: the levels now carry indices into the candidates instead of copies, which makes the code simpler too. This PR made grammar sampling 3 to 10% slower than master, it is now 3 to 15% faster than master (GLM-4.7, Laguna, Qwen3.6 and gemma-4 with the json, list and spaces grammars), with the same masks.

[aldehir]

I think @ggerganov might need to review the new grammar changes.

[ggerganov]

Better move it in the .cpp file.

[ngxson]

Nice, thanks. OK to merge when CI passes @aldehir ?

[aldehir]

@ngxson The MSVC Windows debug builds still crash on test-peg-parser, and both the debug/release builds crash on test-jinja. Here is a change for common/jinja/runtime.cpp that will stop the crash:

Throw exception outside of catch

diff --git a/common/jinja/runtime.cpp b/common/jinja/runtime.cpp
index c2c5385d0..17b8a6f5d 100644
--- a/common/jinja/runtime.cpp
+++ b/common/jinja/runtime.cpp
@@ -3,6 +3,7 @@
 #include "value.h"
 #include "utils.h"
 
+#include <exception>
 #include <string>
 #include <vector>
 #include <memory>
@@ -52,32 +53,40 @@ static void ensure_key_type_allowed(const value & val) {
 
 // execute with error handling
 value statement::execute(context & ctx) const {
+    std::exception_ptr pending;
+    std::string message;
     try {
         return execute_impl(ctx);
     } catch (const continue_statement::signal & /* ex */) {
-        throw;
+        pending = std::current_exception();
     } catch (const break_statement::signal & /* ex */) {
-        throw;
+        pending = std::current_exception();
     } catch (const rethrown_exception & /* ex */) {
-        throw;
+        pending = std::current_exception();
     } catch (const not_implemented_exception & /* ex */) {
-        throw;
+        pending = std::current_exception();
     } catch (const std::exception & e) {
         const std::string & source = *ctx.src;
         if (source.empty()) {
             std::ostringstream oss;
             oss << "\nError executing " << type() << " at position " << pos << ": " << e.what();
-            throw rethrown_exception(oss.str());
+            message = oss.str();
         } else {
             std::ostringstream oss;
             oss << "\n------------\n";
             oss << "While executing " << type() << " at " << get_line_col(source, pos) << " in source:\n";
             oss << peak_source(source, pos) << "\n";
             oss << "Error: " << e.what();
-            // throw as another exception to avoid repeated formatting
-            throw rethrown_exception(oss.str());
+            message = oss.str();
         }
     }
+    // MSVC runs a catch block on top of the frames it unwinds, so throwing
+    // from inside one at every level stacks the handlers; throw from here
+    if (pending) {
+        std::rethrow_exception(pending);
+    }
+    // throw as another exception to avoid repeated formatting
+    throw rethrown_exception(message);
 }
 
 value identifier::execute_impl(context & ctx) const {

I'll leave that to your discretion, but basically throwing inside a catch block blows up the stack on Windows.

I'm lowering the recursion limit in the peg parser, since there is no need for it to be this high. A true fix is to parse using trampolines, but that's around a 1000-line fix.

[ServeurpersoCom]

@aldehir you were right about the cause. I reproduced it on Windows and measured how much stack 120 nested macro calls need:

• current code: 272 KB normally, but 2.7 MB when an error is thrown, so it crashes
• your patch: 272 KB in both cases
• my version below: 208 KB in both cases (400 KB in Debug instead of 656 KB)

Same idea as yours, the only difference is that the error formatting moves out of the catch block, so its locals no longer weigh on every level. With it, the 128 macro limit fits in the default stack everywhere (1 MB on Windows, 512 KB for macOS threads), and test-jinja passes on Windows in Release and Debug. Note that CI stays green either way, Windows only runs the main label so test-jinja never runs there.

Your 64 limit for the PEG parser makes sense for now: one JSON level costs about 3.6 KB in Release and 9.3 KB in Debug. I'll look into making the PEG parser non-recursive with an explicit stack, like the grammar rejection in this PR, so it no longer depends on the stack size at all. If it works out well, I'll open it as a follow-up.

@ngxson your call for the jinja fix: I can push it here, or open it as a follow-up if you prefer to merge first.

diff --git a/common/jinja/runtime.cpp b/common/jinja/runtime.cpp
index c2c5385d0a..b3ec0c01a7 100644
--- a/common/jinja/runtime.cpp
+++ b/common/jinja/runtime.cpp
@@ -3,6 +3,7 @@
 #include "value.h"
 #include "utils.h"

+#include <exception>
 #include <string>
 #include <vector>
 #include <memory>
@@ -50,34 +51,40 @@ static void ensure_key_type_allowed(const value & val) {
     }
 }

-// execute with error handling
+// the error of a statement, located in the source
+static std::string error_message(const statement & stmt, const context & ctx, const std::exception & e) {
+    const std::string & source = *ctx.src;
+    std::ostringstream oss;
+    if (source.empty()) {
+        oss << "\nError executing " << stmt.type() << " at position " << stmt.pos << ": " << e.what();
+    } else {
+        oss << "\n------------\n";
+        oss << "While executing " << stmt.type() << " at " << get_line_col(source, stmt.pos) << " in source:\n";
+        oss << peak_source(source, stmt.pos) << "\n";
+        oss << "Error: " << e.what();
+    }
+    return oss.str();
+}
+
+// execute with error handling: an error is wrapped once with its location, the signals and the
+// wrapped errors pass through. The exception leaves after the catch block: MSVC runs a catch block
+// on top of the frames it unwinds, so a throw from inside one at every level would stack them
 value statement::execute(context & ctx) const {
+    std::exception_ptr ex;
     try {
         return execute_impl(ctx);
-    } catch (const continue_statement::signal & /* ex */) {
-        throw;
-    } catch (const break_statement::signal & /* ex */) {
-        throw;
-    } catch (const rethrown_exception & /* ex */) {
-        throw;
-    } catch (const not_implemented_exception & /* ex */) {
-        throw;
+    } catch (const continue_statement::signal &) {
+        ex = std::current_exception();
+    } catch (const break_statement::signal &) {
+        ex = std::current_exception();
+    } catch (const rethrown_exception &) {
+        ex = std::current_exception();
+    } catch (const not_implemented_exception &) {
+        ex = std::current_exception();
     } catch (const std::exception & e) {
-        const std::string & source = *ctx.src;
-        if (source.empty()) {
-            std::ostringstream oss;
-            oss << "\nError executing " << type() << " at position " << pos << ": " << e.what();
-            throw rethrown_exception(oss.str());
-        } else {
-            std::ostringstream oss;
-            oss << "\n------------\n";
-            oss << "While executing " << type() << " at " << get_line_col(source, pos) << " in source:\n";
-            oss << peak_source(source, pos) << "\n";
-            oss << "Error: " << e.what();
-            // throw as another exception to avoid repeated formatting
-            throw rethrown_exception(oss.str());
-        }
+        ex = std::make_exception_ptr(rethrown_exception(error_message(*this, ctx, e)));
     }
+    std::rethrow_exception(ex);
 }

 value identifier::execute_impl(context & ctx) const {

[aldehir]

I'll look into making the PEG parser non-recursive with an explicit stack

No need to spin your wheels on this, I already have a working implementation from when I was tackling incremental parsing. It's more of a question of whether or not it's worth the complexity for higher recursion limits if model outputs are typically very shallow.

[ngxson]

@aldehir @ServeurpersoCom I applied both suggestions