perf(interpreter): near-linear variable resolver (fix hang on large stacks)

modules/interpreter.py

@@ -16,6 +16,16 @@
 import re
 from pathlib import Path
 
+# find_replace_values() memo, keyed (value, module). Reset per
+# handle_metadata_vars() pass so nothing leaks across CLI runs.
+_FRV_CACHE: dict[tuple[str, str], str] = {}
+_FRV_STACK: set[tuple[str, str]] = set()
+# Cost cap: bound work per attribute so an unresolvable value bails out fast
+# instead of hanging (it would otherwise hit the depth>=50 UNKNOWN path anyway).
+_FRV_STATE: dict[str, int] = {"calls": 0}
+_FRV_MAX_CALLS = 50000
+_FRV_MAX_LEN = 20000
+
 
 def _coerce_output_value(val: Any) -> str:
     """Coerce a Terraform output value to a string for variable substitution."""
@@ -167,10 +177,15 @@ def handle_metadata_vars(tfdata: Dict[str, Any]) -> Dict[str, Any]:
     Returns:
         Updated tfdata with resolved metadata variables
     """
+    # Reset memo caches for this pass (shared across all resources/attrs).
+    _FRV_CACHE.clear()
+    _FRV_STACK.clear()
     # Loop through each resource's metadata attributes
     for resource, attr_list in tfdata["meta_data"].items():
         for key, orig_value in attr_list.items():
             value = str(orig_value)
+            _FRV_STATE["calls"] = 0  # per-attribute budget for the cost cap
+            _FRV_STATE["warned"] = 0  # warn at most once per attribute
             # Iteratively resolve all variable references.
             # Track seen values to detect cycles (e.g. local.A → local.B → local.A)
             # where find_replace_values keeps producing a different string each
@@ -549,25 +564,64 @@ def find_replace_values(
         )
         return "UNKNOWN"
 
-    # Regex string matching to create lists of different variable markers found
-    value = helpers.strip_var_curlies(str(varstring))
-    # Find all variable types using regex patterns
-    var_found_list = re.findall(r"var\.[A-Za-z0-9_\-]+", value)
-    data_found_list = re.findall(r"data\.[A-Za-z0-9_\-\.\[\]]+", value)
-    varobject_found_list = re.findall(r"var\.[A-Za-z0-9_\-]+\.[A-Za-z0-9_\-]+", value)
-    local_found_list = re.findall(r"local\.[A-Za-z0-9_\-\.\[\]]+", value)
-    modulevar_found_list = [
-        m.rstrip("[") for m in re.findall(r"module\.[A-Za-z0-9_\-\.\[\]]+", value)
-    ]
-    # Replace found variable strings with actual values in order
-    value = replace_data_values(data_found_list, value, tfdata)
-    value = replace_module_vars(
-        modulevar_found_list, value, module, tfdata, recursion_depth
-    )
-    value = replace_var_values(
-        var_found_list, varobject_found_list, value, module, tfdata
-    )
-    value = replace_local_values(local_found_list, value, module, tfdata)
+    # Memoize + guard re-entrant resolution FIRST, so cache hits and re-entrant
+    # calls are free and do NOT consume the cost budget below. replace_module_vars
+    # re-runs this on the WHOLE string once per nested ref -> O(refs**depth) and
+    # hangs on large stacks; memoizing (value, module) and short-circuiting
+    # re-entrant identical calls makes it ~linear. Resolution is a pure function
+    # of (value, module) here, so caching is safe.
+    _cache_key = (str(varstring), str(module))
+    if _cache_key in _FRV_CACHE:
+        return _FRV_CACHE[_cache_key]
+    if _cache_key in _FRV_STACK:  # already resolving this exact string; can't progress
+        return str(varstring)
+
+    # Cost cap: bound per-attribute work so a value the resolver can never fully
+    # resolve bails out in bounded time (it would otherwise reach the
+    # recursion_depth>=50 UNKNOWN path). Warn once per attribute if hit, so real
+    # stacks tripping the guard are visible.
+    _FRV_STATE["calls"] += 1
+    if _FRV_STATE["calls"] > _FRV_MAX_CALLS or len(str(varstring)) > _FRV_MAX_LEN:
+        if not _FRV_STATE.get("warned"):
+            click.echo(
+                click.style(
+                    f"   WARNING: variable resolver hit its cost cap "
+                    f"(calls>{_FRV_MAX_CALLS} or len>{_FRV_MAX_LEN}); leaving "
+                    f"'{str(varstring)[:60]}' partially resolved.",
+                    fg="yellow",
+                )
+            )
+            _FRV_STATE["warned"] = 1
+        return str(varstring)
+
+    _FRV_STACK.add(_cache_key)
+    try:
+        # Regex string matching to create lists of different variable markers
+        value = helpers.strip_var_curlies(str(varstring))
+        var_found_list = re.findall(r"var\.[A-Za-z0-9_\-]+", value)
+        data_found_list = re.findall(r"data\.[A-Za-z0-9_\-\.\[\]]+", value)
+        varobject_found_list = re.findall(
+            r"var\.[A-Za-z0-9_\-]+\.[A-Za-z0-9_\-]+", value
+        )
+        local_found_list = re.findall(r"local\.[A-Za-z0-9_\-\.\[\]]+", value)
+        modulevar_found_list = [
+            m.rstrip("[") for m in re.findall(r"module\.[A-Za-z0-9_\-\.\[\]]+", value)
+        ]
+        # Replace found variable strings with actual values in order
+        value = replace_data_values(data_found_list, value, tfdata)
+        value = replace_module_vars(
+            modulevar_found_list, value, module, tfdata, recursion_depth
+        )
+        value = replace_var_values(
+            var_found_list, varobject_found_list, value, module, tfdata
+        )
+        value = replace_local_values(local_found_list, value, module, tfdata)
+    finally:
+        # Always clear the in-progress marker, even on exception, so a raised
+        # error never leaves a stale entry that poisons later resolutions.
+        _FRV_STACK.discard(_cache_key)
+    value = value if isinstance(value, str) else str(value)
+    _FRV_CACHE[_cache_key] = value
     return value
 
 

tests/interpreter_perf_unit_test.py

@@ -0,0 +1,100 @@
+"""Tests for the near-linear variable resolver guards in modules.interpreter.
+
+Covers the memoization, re-entrancy guard, and bounded-cost cap added to
+find_replace_values (and its per-pass reset in handle_metadata_vars), plus a
+regression guard asserting a pathological, deeply-referential attribute resolves
+in bounded time rather than blowing up O(refs**depth).
+"""
+
+import os
+import sys
+import time
+import unittest
+from unittest.mock import patch
+
+parent_dir = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
+sys.path.append(parent_dir)
+
+import modules.interpreter as interp
+from modules.interpreter import find_replace_values, handle_metadata_vars
+
+
+class ResolverGuardTests(unittest.TestCase):
+    def setUp(self):
+        # Isolate module-level caches between tests.
+        interp._FRV_CACHE.clear()
+        interp._FRV_STACK.clear()
+        interp._FRV_STATE.clear()
+        interp._FRV_STATE["calls"] = 0
+
+    def test_normal_local_resolution_unchanged(self):
+        """A simple local reference still resolves to its value."""
+        tfdata = {"all_locals": {"main": {"foo": "bar"}}}
+        self.assertEqual(find_replace_values("${local.foo}", "main", tfdata).strip(), "bar")
+
+    def test_cache_hit_does_not_consume_budget(self):
+        """A cached (value, module) returns immediately without spending budget."""
+        interp._FRV_CACHE[("${local.foo}", "main")] = "cached"
+        interp._FRV_STATE["calls"] = 0
+        result = find_replace_values("${local.foo}", "main", {"all_locals": {}})
+        self.assertEqual(result, "cached")
+        self.assertEqual(interp._FRV_STATE["calls"], 0)  # budget untouched
+
+    def test_reentrant_call_returns_str_unchanged(self):
+        """A re-entrant identical (value, module) short-circuits and returns a str."""
+        interp._FRV_STACK.add(("${module.m.a}", "main"))
+        result = find_replace_values("${module.m.a}", "main", {})
+        self.assertEqual(result, "${module.m.a}")
+        self.assertIsInstance(result, str)
+
+    def test_stack_marker_removed_on_exception(self):
+        """An exception mid-resolution must not leave a stale _FRV_STACK entry."""
+        key = ("${local.x}", "main")
+        with patch.object(
+            interp.helpers, "strip_var_curlies", side_effect=RuntimeError("boom")
+        ):
+            with self.assertRaises(RuntimeError):
+                find_replace_values("${local.x}", "main", {"all_locals": {}})
+        self.assertNotIn(key, interp._FRV_STACK)
+
+    def test_cost_cap_calls_bounds_and_warns(self):
+        """Exceeding the per-attribute call budget returns the input as a str and warns."""
+        with patch.object(interp, "_FRV_MAX_CALLS", 0):
+            with patch.object(interp.click, "echo") as echo:
+                result = find_replace_values("${local.foo}", "main", {"all_locals": {}})
+        self.assertEqual(result, "${local.foo}")
+        self.assertIsInstance(result, str)
+        self.assertTrue(echo.called)  # a warning was emitted
+
+    def test_cost_cap_length_bounds(self):
+        """An over-length value bails out unchanged without processing."""
+        big = "${local." + "a" * 50 + "}"
+        with patch.object(interp, "_FRV_MAX_LEN", 5):
+            result = find_replace_values(big, "main", {"all_locals": {}})
+        self.assertEqual(result, big)
+
+    def test_deeply_referential_attribute_is_bounded(self):
+        """Regression guard: an attribute referencing many mutually-referencing
+        module outputs must resolve in bounded time (previously O(refs**depth))."""
+        n = 10
+        refs = " ".join("${module.m1.o%d}" % i for i in range(n))
+        outputs = []
+        for i in range(n):
+            others = " ".join(
+                "${module.m1.o%d}" % j for j in range(n) if j != i
+            )
+            outputs.append({f"o{i}": {"value": "${var.unset} " + others}})
+        tfdata = {
+            "meta_data": {"aws_thing.x": {"module": "main", "config": refs}},
+            "all_output": {";m1;/outputs.tf": outputs},
+            "all_locals": {"main": {}},
+            "variable_map": {"main": {}},
+            "module_source_dict": {},
+        }
+        start = time.time()
+        handle_metadata_vars(tfdata)
+        self.assertLess(time.time() - start, 10.0)  # seconds; patched runs in <1s
+
+
+if __name__ == "__main__":
+    unittest.main()