From cd5c859cf3cb46aa2e56df7689125fcc83c9791d Mon Sep 17 00:00:00 2001 From: Matthew Johnson Date: Fri, 4 Sep 2026 13:47:07 -0700 Subject: [PATCH 1/2] perf(interpreter): make variable resolver near-linear (fix hang on large stacks) find_replace_values re-resolves the entire attribute string once per nested reference (via replace_module_vars), which is O(refs**depth) and can hang for minutes / exhaust memory on large stacks whose attributes reference several module outputs (e.g. a jsonencode() secret built from other modules' endpoints). This adds, within a single handle_metadata_vars pass: - Memoization of (value, module) -> resolved result. - A re-entrancy guard that returns the string unchanged when the same (value, module) is already being resolved on the stack (that recursion cannot make progress and is the source of the exponential fan-out). - A per-attribute bounded-cost guard (call budget + working-string length) so a value the resolver could never fully resolve anyway (it would otherwise reach the existing recursion_depth>=50 'UNKNOWN' path) bails out in bounded time instead of after a hang. Normal attributes resolve in a handful of small calls and never hit the caps, so their resolved output is unchanged. --- modules/interpreter.py | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/modules/interpreter.py b/modules/interpreter.py index cb034a16..fb43f828 100644 --- a/modules/interpreter.py +++ b/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,14 @@ 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 # 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,6 +563,22 @@ def find_replace_values( ) return "UNKNOWN" + # Cost cap: bound per-attribute work (see module top). + _FRV_STATE["calls"] += 1 + if _FRV_STATE["calls"] > _FRV_MAX_CALLS or len(str(varstring)) > _FRV_MAX_LEN: + return str(varstring) + + # replace_module_vars re-runs this on the WHOLE string once per nested ref + # -> O(refs**depth), which hangs on large stacks. Memoize (value, module) and + # short-circuit re-entrant identical calls to make it ~linear. Resolution is + # a pure function of (value, module) here, so caching is safe. + _cache_key = (str(varstring), 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 varstring + _FRV_STACK.add(_cache_key) + # 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 @@ -568,6 +598,8 @@ def find_replace_values( var_found_list, varobject_found_list, value, module, tfdata ) value = replace_local_values(local_found_list, value, module, tfdata) + _FRV_STACK.discard(_cache_key) + _FRV_CACHE[_cache_key] = value return value From ac27cced6d2c004531cf91491258169a8f7c00a9 Mon Sep 17 00:00:00 2001 From: Matthew Johnson Date: Wed, 9 Sep 2026 11:00:07 -0700 Subject: [PATCH 2/2] address review: try/finally stack cleanup, str keys, cache-before-budget, cap warning, tests Per @patrickchugh's review on #212: - Clear the in-progress (value, module) marker in a try/finally so an exception never leaves a stale _FRV_STACK entry. - Use str(module) in the cache key; short-circuit paths always return str. - Check the memo cache (and re-entrancy) BEFORE spending the cost budget, so cached hits don't consume _FRV_STATE. - Warn (once per attribute) when the cost cap is hit, so stacks that trip the guard are visible. - Add tests/interpreter_perf_unit_test.py: normal resolution, cache-hit budget, re-entrancy, exception stack-cleanup, both cost caps, and a bounded-runtime regression guard for a deeply mutually-referential attribute. --- modules/interpreter.py | 84 ++++++++++++++--------- tests/interpreter_perf_unit_test.py | 100 ++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 31 deletions(-) create mode 100644 tests/interpreter_perf_unit_test.py diff --git a/modules/interpreter.py b/modules/interpreter.py index fb43f828..76ba648a 100644 --- a/modules/interpreter.py +++ b/modules/interpreter.py @@ -185,6 +185,7 @@ def handle_metadata_vars(tfdata: Dict[str, Any]) -> Dict[str, Any]: 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 @@ -563,42 +564,63 @@ def find_replace_values( ) return "UNKNOWN" - # Cost cap: bound per-attribute work (see module top). + # 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) - # replace_module_vars re-runs this on the WHOLE string once per nested ref - # -> O(refs**depth), which hangs on large stacks. Memoize (value, module) and - # short-circuit re-entrant identical calls to make it ~linear. Resolution is - # a pure function of (value, module) here, so caching is safe. - _cache_key = (str(varstring), 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 varstring _FRV_STACK.add(_cache_key) - - # 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) - _FRV_STACK.discard(_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 diff --git a/tests/interpreter_perf_unit_test.py b/tests/interpreter_perf_unit_test.py new file mode 100644 index 00000000..809ff6c1 --- /dev/null +++ b/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()