diff --git a/buckaroo/server/handlers.py b/buckaroo/server/handlers.py index 6a25b36ee..a788610fa 100644 --- a/buckaroo/server/handlers.py +++ b/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 diff --git a/tests/unit/server/test_load_expr.py b/tests/unit/server/test_load_expr.py index 86dd46331..c162d09e9 100644 --- a/tests/unit/server/test_load_expr.py +++ b/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