From dbf5c6c54d28144a371fa86fb3c5c17eaf8826a8 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Wed, 22 Jul 2026 04:43:16 +0000 Subject: [PATCH] feat(admin-config-tool): task 2.2 - xcecommon.schema Load controller.capnp once per process via a cached loader that resolves identically under editable and pinned installs. Missing or corrupted schema content converts a pycapnp uncaught-C++-abort into a clean SchemaNotFoundError via isolated subprocess pre-validation. --- .kiro/specs/admin-config-tool/tasks.md | 3 +- src/xceadmin/src/xcecommon/schema.py | 179 ++++++++++++++++ src/xceadmin/tests/test_schema.py | 270 +++++++++++++++++++++++++ 3 files changed, 451 insertions(+), 1 deletion(-) create mode 100644 src/xceadmin/src/xcecommon/schema.py create mode 100644 src/xceadmin/tests/test_schema.py diff --git a/.kiro/specs/admin-config-tool/tasks.md b/.kiro/specs/admin-config-tool/tasks.md index 3c8e0d7f..541910c7 100644 --- a/.kiro/specs/admin-config-tool/tasks.md +++ b/.kiro/specs/admin-config-tool/tasks.md @@ -31,7 +31,7 @@ - _Requirements: 4.1, 4.2, 4.3, 4.4, 4.5, 5.5_ - _Boundary: xcecommon.validation_ -- [ ] 2.2 (P) Load the cluster wire-protocol schema +- [x] 2.2 (P) Load the cluster wire-protocol schema - The wire-protocol schema loads once per process and exposes every message type xceadmin needs to build or decode - Loading works identically whether xceadmin was installed in editable or pinned mode - A missing or corrupted schema copy produces a clear, actionable error rather than a crash @@ -147,3 +147,4 @@ - **1.3**: Claude Code `PostToolUse` hooks receive tool input via stdin JSON (`tool_input.file_path`), not a `$FILE_PATH` env var — brief.md's ruff-hook sketch used the env-var form, which would have silently no-op'd. The actual hooks added to `.claude/settings.json` follow this repo's existing `.wolf/hooks/post-write.js` stdin-JSON convention instead. Any future hook work in this repo should follow the same pattern, not brief.md's original sketch. - **1.3**: `src/xceadmin/src/xcecommon/schema_files/` (synced capnp + JSON schema copies) is a build artifact, regenerated every `xce-build.sh xceadmin` run — never hand-edit it, never commit it (`src/xceadmin/.gitignore` excludes it). - **2.1**: `check_undefined_refs` deliberately does not parse resource names out of `res_req`/`gpu_req` free-form LSF-style expression strings (e.g. `"mem>1000 rusage[mem=1000]"`) — design.md's `xcecommon.validation` component scopes this check to queue→host-group and limit→group/`resname` only. `limits.yaml`'s `projects` field and `scaler.yaml` name collisions are likewise out of scope — no `projects.yaml` catalog exists among the 7 config types, and Requirement 4.3 only enumerates queue/host/resource/group/limit names. `src/configs/hosts.yaml` has 2 pre-existing undefined refs (resource `mg`, host `h3`) that now surface as warnings — expected, not a regression; fixing the example fixture is outside this task's boundary. +- **2.2**: The real `controller.capnp` names its reply struct `RequestReply`, not `Reply` as design.md's postcondition states — an inexact design pseudo-type, corrected empirically (verified directly against `src/schema/controller.capnp`, not assumed). pycapnp 2.0.0/capnproto 1.2.0 abort the whole process (uncaught C++ exception, not a Python exception) on a missing cross-file import or malformed `.capnp` content passed to `capnp.load()` — a bare `try/except` cannot catch this. `xcecommon.schema.load_schema()` works around it by pre-validating in an isolated subprocess before ever calling `capnp.load()` in the caller's own process, converting any abort into a clean `SchemaNotFoundError`. Any future code that calls `capnp.load()` directly (bypassing this loader) reintroduces the crash risk — always go through `xcecommon.schema.load_schema()`. diff --git a/src/xceadmin/src/xcecommon/schema.py b/src/xceadmin/src/xcecommon/schema.py new file mode 100644 index 00000000..6c6dc8f6 --- /dev/null +++ b/src/xceadmin/src/xcecommon/schema.py @@ -0,0 +1,179 @@ +"""Cap'n Proto wire-protocol schema loader for xcecommon/xceadmin (design.md, +``xcecommon.schema``; Requirements 5.1, 5.5, 7.4). + +This module has no dependency on ``xcecommon.{validation,mapping,wire}`` +(design.md, Dependency Direction) -- those depend on it, not the reverse. + +``load_schema()`` is the single load point for ``controller.capnp``: it is +called once per process (memoized below), and every other module imports +the returned module object rather than calling ``capnp.load()`` itself. +This matters beyond "avoid redundant work" -- ``capnp.load()`` returns a +*new* module object, with non-identical type objects, on every call ( +verified directly: two back-to-back ``capnp.load()`` calls against the +same file produce ``mod1 is not mod2`` and ``mod1.ClusterConfig is not +mod2.ClusterConfig``). Without a shared cache, two xcecommon modules each +calling ``capnp.load()`` independently would hold non-identical +``ClusterConfig``/``Request`` type objects for what is conceptually the +same schema. + +Crash-avoidance note (this is the reason ``load_schema()`` does more than +call ``capnp.load()`` directly): pycapnp 2.0.0 / capnproto 1.2.0 do **not** +raise a catchable Python exception for every schema-loading failure. A +missing top-level file raises an ordinary ``OSError`` -- but a missing +cross-file import (``c.capnp`` absent from the same directory) or +genuinely malformed ``.capnp`` syntax instead propagates as an uncaught +C++ ``kj::ExceptionImpl``, which crosses the Cython boundary uncaught and +calls ``std::terminate()`` -- aborting the *entire process* (confirmed +directly: ``capnp.load()`` against a corrupted file or a schema missing +its import terminates the interpreter with SIGABRT, not a Python +exception). The task's Observable bullet ("A missing or corrupted schema +copy produces a clear, actionable error rather than a crash") is +unimplementable by wrapping ``capnp.load()`` in a plain ``try/except`` -- +there is nothing in Python to catch. ``load_schema()`` instead: + +1. Pre-checks that the resolved path and its ``c.capnp`` sibling import + both exist as files, raising :class:`SchemaNotFoundError` (Requirement + 7.4) *before* ever calling ``capnp.load()`` -- this alone is sufficient + for the "not found" failure mode and never risks the abort. +2. For a schema file that exists but may be corrupted (wrong bytes, a bad + copy, truncated content), pre-validates it by attempting the exact same + ``capnp.load()`` call inside an isolated subprocess first. A crash + there is fully contained -- it takes down the throwaway subprocess, not + ours -- and its exit code tells us definitively whether the real, + in-process load below is safe to attempt. +""" + +from __future__ import annotations + +import functools +import importlib.resources +import os +import subprocess +import sys +from pathlib import Path +from typing import TYPE_CHECKING + +import capnp + +from xcecommon.errors import SchemaNotFoundError + +if TYPE_CHECKING: + from types import ModuleType + +SCHEMA_PATH_ENV_VAR = "XCEADMIN_SCHEMA_PATH" + +_C_CAPNP_FILENAME = "c.capnp" +_PRECHECK_TIMEOUT_SECONDS = 30 +_PRECHECK_SNIPPET = "import capnp, sys\ncapnp.load(sys.argv[1], imports=[sys.argv[2]])\n" + +_INSTALLATION_HINT = f"check your xceadmin installation (override with --schema-path or ${SCHEMA_PATH_ENV_VAR})" + + +def default_schema_path() -> str: + """Resolve the path to the bundled ``controller.capnp`` (Requirement + 5.5). + + ``XCEADMIN_SCHEMA_PATH``, if set, takes precedence -- this is the + env-var half of the "--schema-path (CLI) / XCEADMIN_SCHEMA_PATH (env + var) override the default for troubleshooting" mechanism (design.md); + the CLI-flag half is threaded through as the explicit ``schema_path`` + argument to :func:`load_schema` by a later task's ``xceadmin.cli``. + + Otherwise uses ``importlib.resources.files("xcecommon") / + "schema_files" / "controller.capnp"`` -- the same + ``importlib.resources`` mechanism ``xcecommon.validation. + default_schema_dir()`` uses for the JSON schemas, resolving correctly + under both ``pip install -e .`` (points into the checked-out + ``schema_files/`` copy) and ``pip install .`` (points into the + installed package's copy) identically. + """ + override = os.environ.get(SCHEMA_PATH_ENV_VAR) + if override: + return override + return str(importlib.resources.files("xcecommon") / "schema_files" / "controller.capnp") + + +def _require_schema_files_present(schema_path: Path) -> None: + """Raise :class:`SchemaNotFoundError` if ``schema_path`` or its + ``c.capnp`` sibling import is missing -- *before* calling + ``capnp.load()`` at all, since a missing import is one of the failure + modes that otherwise aborts the process (see module docstring). + """ + if not schema_path.is_file(): + raise SchemaNotFoundError( + f"controller.capnp schema not found at {schema_path} -- {_INSTALLATION_HINT}", + path=str(schema_path), + ) + + c_capnp_path = schema_path.parent / _C_CAPNP_FILENAME + if not c_capnp_path.is_file(): + raise SchemaNotFoundError( + f"c.capnp import not found alongside {schema_path} " + f"(expected {c_capnp_path}) -- {_INSTALLATION_HINT}", + path=str(c_capnp_path), + ) + + +def _precheck_parses_cleanly(schema_path: Path, import_dir: Path) -> None: + """Attempt the real ``capnp.load()`` call inside an isolated + subprocess first, so a corrupted schema's process-abort lands on the + throwaway subprocess instead of ours (see module docstring). A clean + (returncode 0) subprocess exit means the real in-process load that + follows is safe. + + Best-effort: if the subprocess itself cannot even be started (e.g. no + working Python interpreter, which should not happen in practice since + we re-invoke ``sys.executable``), this silently falls through to the + real load rather than masking an unrelated environment problem behind + a misleading schema error. + """ + try: + result = subprocess.run( + [sys.executable, "-c", _PRECHECK_SNIPPET, str(schema_path), str(import_dir)], + capture_output=True, + text=True, + timeout=_PRECHECK_TIMEOUT_SECONDS, + ) + except (OSError, subprocess.TimeoutExpired): + return + + if result.returncode != 0: + detail = (result.stderr or result.stdout or "unknown schema parse error").strip() + raise SchemaNotFoundError( + f"controller.capnp schema at {schema_path} is corrupted or cannot be " + f"parsed: {detail} -- {_INSTALLATION_HINT}", + path=str(schema_path), + ) + + +@functools.lru_cache(maxsize=None) +def _load_schema_cached(schema_path: str) -> "ModuleType": + path = Path(schema_path) + _require_schema_files_present(path) + _precheck_parses_cleanly(path, path.parent) + return capnp.load(str(path), imports=[str(path.parent)]) + + +def load_schema(schema_path: str | None = None) -> "ModuleType": + """Load ``controller.capnp`` (``default_schema_path()`` if + ``schema_path`` is ``None``), returning the capnp-generated module + exposing ``.ClusterConfig``, ``.Request``, ``.RequestReply`` (the + reply-message struct -- design.md's Service Interface calls this + ``.Reply``, but the real schema names it ``RequestReply``; verified + directly against ``capnp.load()`` output, not assumed), + ``.UpgradeConfigRequest``/``.UpgradeConfigReply``, + ``.ProbeConfigTagRequest``/``.ProbeConfigTagReply``, and every other + top-level type ``controller.capnp`` declares (Requirement 5.1). + + Memoized per resolved path -- repeated calls with the same + ``schema_path`` (including repeated implicit-default calls) return the + exact same module object, never re-loading (design.md: "Single load + point ... called once per process"). + + Raises :class:`SchemaNotFoundError` if the resolved path or its + ``c.capnp`` import is missing, or if the schema content is corrupted + and cannot be parsed (Requirement 7.4) -- never lets a raw capnp + exception or process abort escape. + """ + resolved_path = schema_path if schema_path is not None else default_schema_path() + return _load_schema_cached(resolved_path) diff --git a/src/xceadmin/tests/test_schema.py b/src/xceadmin/tests/test_schema.py new file mode 100644 index 00000000..328a7ae2 --- /dev/null +++ b/src/xceadmin/tests/test_schema.py @@ -0,0 +1,270 @@ +"""Tests for xcecommon.schema (task 2.2). + +Covers Requirements 5.1, 5.5, 7.4: loading the bundled ``controller.capnp`` +wire-protocol schema once per process and exposing every message type +xceadmin needs to build or decode, resolving identically under both +editable and pinned (non-editable) installs, and producing a clear, +actionable error -- never a crash -- when the bundled schema copy is +missing or corrupted. + +Fixture strategy mirrors task 2.1's ``test_validation.py``: the "happy +path" tests exercise the *real* bundled schema (``default_schema_path()``) +so we prove the actual shipped ``controller.capnp``/``c.capnp`` pair loads +and exposes real top-level types, not just a synthetic fixture. The +missing/corrupted-schema error-path tests build small throwaway ``.capnp`` +files under ``tmp_path`` instead, since those failure modes must not touch +the real bundled copy. + +Design-vs-reality note (see schema.py module docstring for the full +explanation): design.md's Service Interface postcondition lists a +``Reply`` top-level type, but the real ``controller.capnp`` names this +struct ``RequestReply`` (``RequestReply`` is the reply *message*; +``request.body.replymsg`` embeds it). Verified directly against +``capnp.load()`` output rather than assumed -- see +``test_load_schema_exposes_expected_top_level_types`` below, which asserts +against the real name. +""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +import pytest + +from xcecommon.errors import SchemaNotFoundError +from xcecommon.schema import ( + SCHEMA_PATH_ENV_VAR, + default_schema_path, + load_schema, +) + +# --------------------------------------------------------------------------- +# default_schema_path() -- resolution under the active install +# --------------------------------------------------------------------------- + + +def test_default_schema_path_resolves_to_real_bundled_schema() -> None: + """default_schema_path() must resolve to a real, existing + controller.capnp, with c.capnp sitting alongside it (Requirement 5.5). + This is the assertion that matters identically whether xceadmin is + installed editable or pinned -- see module docstring and the task's + Observable bullet; the *editable-vs-pinned* half of that guarantee is + proven by running this same test suite under both + `xce-build.sh xceadmin` and `xce-build.sh xceadmin release` profiles + (see TESTS_RUN in the task status report), not by anything expressible + inside a single test process. + """ + schema_path = Path(default_schema_path()) + + assert schema_path.is_file() + assert schema_path.name == "controller.capnp" + assert (schema_path.parent / "c.capnp").is_file() + + +def test_default_schema_path_env_var_override(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + """XCEADMIN_SCHEMA_PATH overrides the packaged default, per design.md's + '--schema-path (CLI) / XCEADMIN_SCHEMA_PATH (env var) override the + default for troubleshooting' (7.4).""" + custom = tmp_path / "custom.capnp" + monkeypatch.setenv(SCHEMA_PATH_ENV_VAR, str(custom)) + + assert default_schema_path() == str(custom) + + +def test_default_schema_path_no_env_var_uses_packaged_default( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.delenv(SCHEMA_PATH_ENV_VAR, raising=False) + + resolved = Path(default_schema_path()) + + assert resolved.is_file() + + +# --------------------------------------------------------------------------- +# load_schema() -- happy path against the real bundled schema +# --------------------------------------------------------------------------- + + +def test_load_schema_default_exposes_expected_top_level_types() -> None: + """load_schema() with no argument must load the real bundled schema + and expose every top-level type xceadmin needs to build or decode + (Requirement 5.1): ClusterConfig plus its nested list-section types, + Request, the real reply struct name RequestReply (not "Reply" -- see + module docstring), and the Upgrade/ProbeConfigTag request/reply pairs + used by config push / --dry-run.""" + mod = load_schema() + + for type_name in ( + "ClusterConfig", + "LimitConfig", + "HostConfig", + "ScalerConfig", + "Resource", + "QueueConf", + "GroupConfig", + "Request", + "RequestReply", + "UpgradeConfigRequest", + "UpgradeConfigReply", + "ProbeConfigTagRequest", + "ProbeConfigTagReply", + "OPCode", + ): + assert hasattr(mod, type_name), f"loaded schema module missing {type_name}" + + +def test_load_schema_default_types_are_usable_messages() -> None: + """The exposed types must be real, constructible capnp struct types -- + not just names -- proving the schema actually parsed and linked + correctly against the real bundled controller.capnp/c.capnp pair.""" + mod = load_schema() + + cluster_config = mod.ClusterConfig.new_message() + cluster_config.clusterName = "test-cluster" + assert cluster_config.clusterName == "test-cluster" + + assert "upgradeConfig" in mod.OPCode.schema.enumerants + + +def test_load_schema_caches_default_load_returns_same_module_object() -> None: + """'Single load point ... called once per process; other modules + import the returned module object, never re-load' (design.md, + xcecommon.schema Responsibilities & Constraints). capnp.load() itself + returns a *new* module object (and new type objects) on every call -- + verified directly -- so callers that didn't go through a shared cache + would silently get non-identical ClusterConfig/Request type objects + across modules. load_schema() must paper over that.""" + first = load_schema() + second = load_schema() + + assert first is second + assert first.ClusterConfig is second.ClusterConfig + + +def test_load_schema_explicit_path_matching_default_is_usable() -> None: + """An explicit schema_path pointing at the same real bundled file + (the --schema-path override path, at the xcecommon.schema layer) + loads just as well as the implicit default.""" + mod = load_schema(default_schema_path()) + + assert hasattr(mod, "ClusterConfig") + + +# --------------------------------------------------------------------------- +# load_schema() -- missing schema (Requirement 7.4) +# --------------------------------------------------------------------------- + + +def test_load_schema_missing_file_raises_schema_not_found_error(tmp_path: Path) -> None: + """A nonexistent controller.capnp path must raise SchemaNotFoundError + with the missing path attached (Requirement 7.4) -- and must never + reach capnp.load() itself: an absent file passed to capnp.load() + (without the c.capnp sibling present either) is a second, compounding + failure mode covered separately below.""" + missing = tmp_path / "does-not-exist.capnp" + + with pytest.raises(SchemaNotFoundError) as exc_info: + load_schema(str(missing)) + + assert exc_info.value.path == str(missing) + assert str(missing) in exc_info.value.message + + +def test_load_schema_missing_c_capnp_import_raises_schema_not_found_error( + tmp_path: Path, +) -> None: + """controller.capnp present but its c.capnp sibling import missing -- + e.g. a broken/partial installation -- must raise SchemaNotFoundError, + not the uncatchable process abort pycapnp/kj otherwise produces for a + failed import (verified empirically, see module docstring). This is + the 'installation is broken' reading of the task's 'corrupted schema + copy' bullet.""" + real_schema = Path(default_schema_path()) + broken = tmp_path / "controller.capnp" + broken.write_text(real_schema.read_text(encoding="utf-8"), encoding="utf-8") + # Deliberately do NOT copy c.capnp alongside it. + + with pytest.raises(SchemaNotFoundError) as exc_info: + load_schema(str(broken)) + + assert "c.capnp" in exc_info.value.message + + +def test_load_schema_env_var_override_missing_raises_schema_not_found_error( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + missing = tmp_path / "nope.capnp" + monkeypatch.setenv(SCHEMA_PATH_ENV_VAR, str(missing)) + + with pytest.raises(SchemaNotFoundError): + load_schema() + + +# --------------------------------------------------------------------------- +# load_schema() -- corrupted (present but invalid) schema content +# --------------------------------------------------------------------------- + + +def test_load_schema_corrupted_content_raises_schema_not_found_error_not_crash( + tmp_path: Path, +) -> None: + """A present-but-syntactically-invalid controller.capnp must raise a + clear SchemaNotFoundError, not crash the process. + + This is the test that would fail loudly (by taking the whole pytest + process down with it, exit via SIGABRT) if load_schema() called + capnp.load() directly on unvalidated content: pycapnp 2.0.0 / + capnproto 1.2.0 propagate a genuine capnp parse error as an uncaught + C++ 'kj::ExceptionImpl', which calls std::terminate() before it ever + reaches Python's exception machinery -- confirmed by direct + experimentation, not assumed. load_schema() must pre-validate in an + isolated subprocess before ever loading in-process.""" + broken = tmp_path / "controller.capnp" + broken.write_text("this is not { valid capnp syntax at all ]]]\n", encoding="utf-8") + real_schema = Path(default_schema_path()) + (tmp_path / "c.capnp").write_text( + (real_schema.parent / "c.capnp").read_text(encoding="utf-8"), encoding="utf-8" + ) + + with pytest.raises(SchemaNotFoundError) as exc_info: + load_schema(str(broken)) + + assert str(broken) in exc_info.value.message + + +def test_load_schema_corrupted_content_subprocess_never_aborts(tmp_path: Path) -> None: + """Belt-and-suspenders proof, fully isolated from pytest's own + process: run load_schema() against a corrupted schema inside a + throwaway subprocess and assert it exits with an ordinary Python + exception (returncode 1 from an uncaught SchemaNotFoundError formatted + by the interpreter), never a negative returncode (== killed by a + signal, e.g. -6/SIGABRT from the raw capnp.load() abort this task + exists to prevent).""" + broken = tmp_path / "controller.capnp" + broken.write_text("also not valid capnp {{{\n", encoding="utf-8") + real_schema = Path(default_schema_path()) + (tmp_path / "c.capnp").write_text( + (real_schema.parent / "c.capnp").read_text(encoding="utf-8"), encoding="utf-8" + ) + + snippet = ( + "from xcecommon.schema import load_schema\n" + "import sys\n" + "load_schema(sys.argv[1])\n" + ) + result = subprocess.run( + [sys.executable, "-c", snippet, str(broken)], + capture_output=True, + text=True, + timeout=30, + ) + + assert result.returncode >= 0, ( + f"load_schema() let a raw process abort escape (returncode=" + f"{result.returncode}); stderr={result.stderr!r}" + ) + assert result.returncode != 0 + assert "SchemaNotFoundError" in result.stderr