fix(server): a warm /load_expr POST with the session's own config skips the pipeline (#1037)

buckaroo/server/handlers.py

@@ -476,17 +476,6 @@ async def post(self):
                 "message": f"cache_dir must be an absolute path, got {cache_dir!r}"})
             return
 
-        # Config-bearing fields that change how the result is computed or
-        # rendered. If the caller passes any of these on a warm POST we must
-        # re-run the pipeline — returning cached metadata would silently
-        # ignore the new config. stats_tier and stats_delivery are not in this
-        # tuple: it tests truthiness, and a host that sends the pair on every
-        # POST would never get the warm exit (#944). They are compared with the
-        # session's stored pair below instead.
-        has_config = any(body.get(k) for k in (
-            "component_config", "column_config_overrides", "extra_grid_config",
-            "init_sd", "skip_stat_columns"))
-
         # Companion telemetry endpoint (#943): when present, the firstpull.*
         # spans POST themselves to the companion as session-correlated records.
         # Built before the warm-session early-exit so a warm re-POST still
@@ -498,8 +487,9 @@ async def post(self):
         # build_dir (and no new config was supplied), skip the expensive
         # pipeline and return cached metadata. Only fires when the caller
         # explicitly passes back a session_id from a prior response
-        # (UUID-generated ids are always new). Pass force_reload=true — or any
-        # config-bearing field — to bypass this and re-run the full pipeline.
+        # (UUID-generated ids are always new). Pass force_reload=true — or a
+        # config-bearing field that differs from the session's — to bypass
+        # this and re-run the full pipeline.
         sessions = self.application.settings["sessions"]
         existing = sessions.get(session_id)
         # cache_dir carries over like the rest of a loaded session: a re-POST
@@ -525,6 +515,22 @@ async def post(self):
         existing_kwargs = (existing.dataflow_kwargs or {}) if existing is not None else {}
         if data_id is None and existing is not None and existing.build_dir == build_dir:
             data_id = existing_kwargs.get("data_id")
+        # Config-bearing fields that change how the result is computed or
+        # rendered. A warm POST that carries one that differs from what the
+        # session holds must re-run the pipeline — returning cached metadata
+        # would silently ignore the new config. One equal to the session's
+        # changes nothing, so a client that sends its saved config on every
+        # open (#1037) still takes the early-exit. stats_tier and
+        # stats_delivery are not in this tuple: a host that sends the pair on
+        # every POST would never get the warm exit (#944). They are compared
+        # with the session's stored pair in the exit condition below instead.
+        held_config = dict(existing_kwargs)
+        if existing is not None:
+            held_config["component_config"] = existing.component_config
+        has_config = any(
+            body.get(k) and body[k] != held_config.get(k)
+            for k in ("component_config", "column_config_overrides",
+                "extra_grid_config", "init_sd", "skip_stat_columns"))
         # /load swaps a session to pandas without clearing build_dir, so the
         # backend is checked too — else its pandas metadata comes back here.
         if (not force_reload and not has_config and existing

tests/unit/server/test_load_expr.py

@@ -830,6 +830,20 @@ async def _load_and_pull(captured):
             shutil.rmtree(builds_root, ignore_errors=True)
 
 
+# The config-bearing /load_expr fields, each with a value and a different one.
+_CONFIG_VARIANTS = {
+    "column_config_overrides": (
+        {"name": {"displayer_args": {"displayer": "string", "max_length": 5000}}},
+        {"name": {"displayer_args": {"displayer": "string", "max_length": 6000}}}),
+    "extra_grid_config": ({"rowHeight": 70}, {"rowHeight": 80}),
+    "init_sd": (
+        {"idx": {"displayer_args": {"displayer": "string", "max_length": 200}}},
+        {"idx": {"displayer_args": {"displayer": "string", "max_length": 300}}}),
+    "skip_stat_columns": (["name"], ["idx"]),
+    "component_config": ({"search_debounce": 100}, {"search_debounce": 200}),
+}
+
+
 class TestLoadExprPerfFixes(tornado.testing.AsyncHTTPTestCase):
     """Tests for #896 (releasing xorq load state) and #899 (warm-session early-exit)."""
 
@@ -959,6 +973,76 @@ def counting_loader(bd, **kwargs):
         finally:
             shutil.rmtree(builds_root, ignore_errors=True)
 
+    @tornado.testing.gen_test
+    async def test_warm_session_with_same_config_skips_pipeline(self):
+        """#1037: a warm POST whose config fields equal the ones the session
+        holds takes the early-exit. A client that sends its saved config on
+        every open must not re-run expr_load, the dataflow and the metadata
+        each time. A field that differs still re-runs."""
+        builds_root = tempfile.mkdtemp()
+        try:
+            build_path = _build_expr_dir(builds_root)
+            original = xorq_loading.load_expr_build_dir
+            calls = []
+            def counting_loader(bd, **kwargs):
+                calls.append(bd)
+                return original(bd, **kwargs)
+
+            with patch.object(xorq_loading, "load_expr_build_dir", side_effect=counting_loader):
+                for field, (held, other) in _CONFIG_VARIANTS.items():
+                    sid = f"lx-same-config-{field}"
+                    calls.clear()
+                    body = {"session": sid, "build_dir": build_path}
+                    resp = await _post(self.get_http_port(), "/load_expr", {**body, field: held})
+                    self.assertEqual(resp.code, 200, resp.body)
+                    self.assertEqual(len(calls), 1)
+                    resp = await _post(self.get_http_port(), "/load_expr", {**body, field: held})
+                    self.assertEqual(resp.code, 200, resp.body)
+                    self.assertEqual(json.loads(resp.body)["rows"], 10)
+                    self.assertEqual(len(calls), 1, f"equal {field} must take the early-exit")
+                    resp = await _post(self.get_http_port(), "/load_expr", {**body, field: other})
+                    self.assertEqual(resp.code, 200, resp.body)
+                    self.assertEqual(len(calls), 2, f"changed {field} must re-run")
+        finally:
+            shutil.rmtree(builds_root, ignore_errors=True)
+
+    @tornado.testing.gen_test
+    async def test_warm_session_reruns_when_any_config_field_differs(self):
+        """#1037: an equal field doesn't hide a different one, a config the
+        session was not loaded with is new, and omitting config keeps the
+        session's rather than re-running."""
+        builds_root = tempfile.mkdtemp()
+        try:
+            build_path = _build_expr_dir(builds_root)
+            original = xorq_loading.load_expr_build_dir
+            calls = []
+            def counting_loader(bd, **kwargs):
+                calls.append(bd)
+                return original(bd, **kwargs)
+
+            overrides, _ = _CONFIG_VARIANTS["column_config_overrides"]
+            grid, other_grid = _CONFIG_VARIANTS["extra_grid_config"]
+            with patch.object(xorq_loading, "load_expr_build_dir", side_effect=counting_loader):
+                body = {"session": "lx-config-mixed", "build_dir": build_path}
+                await _post(self.get_http_port(), "/load_expr", body)
+                self.assertEqual(len(calls), 1)
+                await _post(self.get_http_port(), "/load_expr",
+                    {**body, "column_config_overrides": overrides})
+                self.assertEqual(len(calls), 2, "a config the session never held is new")
+
+                await _post(self.get_http_port(), "/load_expr",
+                    {**body, "column_config_overrides": overrides, "extra_grid_config": grid})
+                self.assertEqual(len(calls), 3)
+                await _post(self.get_http_port(), "/load_expr",
+                    {**body, "column_config_overrides": overrides, "extra_grid_config": other_grid})
+                self.assertEqual(len(calls), 4, "one differing field must re-run")
+
+                resp = await _post(self.get_http_port(), "/load_expr", body)
+                self.assertEqual(resp.code, 200, resp.body)
+                self.assertEqual(len(calls), 4, "omitted config must keep the session's")
+        finally:
+            shutil.rmtree(builds_root, ignore_errors=True)
+
     @tornado.testing.gen_test
     async def test_warm_exit_requires_a_xorq_session(self):
         """/load swaps a session to pandas but leaves its build_dir, so a later