Skip to content

Commit 94df3e3

Browse files
rustyconoverclaude
andcommitted
Stop import vgi from creating the storage database
`Function.storage` is a lazy descriptor, meant to build the default SQLite store on first use. `TableFunctionBase.__init_subclass__` defeated that: it detected abstract classes with a plain getattr() over dir(cls), which invoked the descriptor, which created vgi_storage.db under the user's state directory. vgi defines subclasses of its own (TableInOutGenerator), so a bare `import vgi` did it, and failed with PermissionError wherever that directory was not writable. That is how it surfaced: a worker image running as a non-root user whose home belonged to root died before it could serve /health. The scan now reads each attribute with inspect.getattr_static, which returns the raw class attribute without invoking descriptors. It answers the same question, because classmethod, staticmethod and property objects all expose the __isabstractmethod__ of what they wrap. tests/test_import_side_effects.py runs the import in a subprocess with HOME and the XDG directories pointed at an empty temporary directory, and checks that importing vgi and defining a function leaves it empty, that the import succeeds with a read-only home, and that storage is still created on first use. It also checks abstract detection for classmethods, staticmethods and properties against a concrete-class control. The two import tests fail without this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent dc2812d commit 94df3e3

2 files changed

Lines changed: 208 additions & 2 deletions

File tree

‎tests/test_import_side_effects.py‎

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
# Copyright 2026 Query Farm LLC - https://query.farm
2+
3+
"""Importing vgi, and defining functions, must not touch the filesystem.
4+
5+
``Function.storage`` resolves the default SQLite store lazily, on first access.
6+
``TableFunctionBase.__init_subclass__`` used to defeat that: it detected
7+
abstract classes with a plain ``getattr`` over ``dir(cls)``, which invoked the
8+
storage descriptor, which created ``vgi_storage.db`` under the user's state
9+
directory. Because vgi defines subclasses of its own, a bare ``import vgi`` did
10+
it — and failed outright wherever that directory was not writable, such as a
11+
container running as a non-root user whose home belongs to root.
12+
13+
The import checks run in a subprocess with ``HOME`` and the XDG directories
14+
pointed at an empty temporary directory, because the storage descriptor caches
15+
its backend per process and the test process has usually resolved it already.
16+
"""
17+
18+
import os
19+
import subprocess
20+
import sys
21+
from abc import abstractmethod
22+
from pathlib import Path
23+
24+
import pytest
25+
26+
from vgi.table_function import TableFunctionGenerator
27+
28+
#: Imports every function base class and defines a concrete table function, so
29+
#: both vgi's own class definitions and a worker's go through __init_subclass__.
30+
_IMPORT_AND_DEFINE = """
31+
import pyarrow as pa
32+
import vgi
33+
import vgi.aggregate_function
34+
import vgi.scalar_function
35+
import vgi.table_in_out_function
36+
from vgi.invocation import BindResponse
37+
from vgi.table_function import TableFunctionGenerator
38+
39+
40+
class Concrete(TableFunctionGenerator[None, None]):
41+
@classmethod
42+
def on_bind(cls, params):
43+
return BindResponse(output_schema=pa.schema([("n", pa.int64())]))
44+
45+
@classmethod
46+
def process(cls, params, state, out):
47+
out.finish()
48+
"""
49+
50+
51+
def _isolated_env(home: Path) -> dict[str, str]:
52+
"""Environment whose every per-user directory lives under ``home``.
53+
54+
``VGI_*`` variables are dropped so the default file-backed SQLite store is
55+
the one in play; a test run configured with ``VGI_WORKER_SHARED_STORAGE``
56+
set to ``memory`` would otherwise pass without the fix.
57+
"""
58+
env = {k: v for k, v in os.environ.items() if not k.startswith(("VGI_", "XDG_"))}
59+
env["HOME"] = str(home)
60+
for name in ("XDG_STATE_HOME", "XDG_DATA_HOME", "XDG_CACHE_HOME", "XDG_CONFIG_HOME"):
61+
env[name] = str(home / name.lower())
62+
return env
63+
64+
65+
def _run(code: str, home: Path) -> subprocess.CompletedProcess[str]:
66+
"""Run ``code`` in a fresh interpreter with per-user directories under ``home``."""
67+
return subprocess.run(
68+
[sys.executable, "-c", code],
69+
env=_isolated_env(home),
70+
capture_output=True,
71+
text=True,
72+
timeout=120,
73+
check=False,
74+
)
75+
76+
77+
def _files_under(root: Path) -> list[str]:
78+
"""Every file below ``root``, relative to it."""
79+
return sorted(str(p.relative_to(root)) for p in root.rglob("*") if p.is_file())
80+
81+
82+
class TestImportWritesNothing:
83+
"""The storage store is created when it is used, not when vgi is imported."""
84+
85+
def test_import_and_define_create_no_files(self, tmp_path: Path) -> None:
86+
"""Importing vgi and defining a function leaves the home directory empty."""
87+
result = _run(_IMPORT_AND_DEFINE, tmp_path)
88+
assert result.returncode == 0, result.stderr
89+
assert _files_under(tmp_path) == []
90+
91+
@pytest.mark.skipif(
92+
sys.platform == "win32" or os.geteuid() == 0,
93+
reason="a read-only directory does not stop root, and POSIX modes do not apply on Windows",
94+
)
95+
def test_import_succeeds_when_home_is_not_writable(self, tmp_path: Path) -> None:
96+
"""A read-only home — a container's non-root user, say — does not stop the import."""
97+
home = tmp_path / "home"
98+
home.mkdir()
99+
home.chmod(0o555)
100+
try:
101+
result = _run(_IMPORT_AND_DEFINE, home)
102+
finally:
103+
home.chmod(0o755)
104+
assert result.returncode == 0, result.stderr
105+
106+
def test_storage_is_still_created_on_first_use(self, tmp_path: Path) -> None:
107+
"""Laziness, not removal: touching Function.storage still creates the store."""
108+
result = _run("from vgi.function import Function\nFunction.storage", tmp_path)
109+
assert result.returncode == 0, result.stderr
110+
assert any(name.endswith("vgi_storage.db") for name in _files_under(tmp_path))
111+
112+
113+
class _NotADataclass:
114+
"""An argument type a concrete function would be rejected for."""
115+
116+
117+
class TestAbstractDetection:
118+
"""Reading attributes statically must still recognize every kind of abstract member.
119+
120+
An abstract class skips validation of its argument type; a concrete one is
121+
rejected at class creation when that type is not a dataclass. So a class
122+
with an invalid argument type is defined successfully only if it is seen as
123+
abstract.
124+
"""
125+
126+
def test_a_concrete_class_is_validated(self) -> None:
127+
"""Control: without an abstract member, the invalid argument type is rejected."""
128+
with pytest.raises(TypeError, match="must be a dataclass"):
129+
130+
class Concrete(TableFunctionGenerator[_NotADataclass, None]): # type: ignore[type-var]
131+
@classmethod
132+
def on_bind(cls, params): # type: ignore[no-untyped-def]
133+
raise NotImplementedError
134+
135+
@classmethod
136+
def process(cls, params, state, out): # type: ignore[no-untyped-def]
137+
raise NotImplementedError
138+
139+
def test_an_abstract_classmethod_marks_the_class_abstract(self) -> None:
140+
"""An inherited-concrete class that adds an abstract classmethod skips validation."""
141+
142+
class WithAbstractClassmethod(TableFunctionGenerator[_NotADataclass, None]): # type: ignore[type-var]
143+
@classmethod
144+
def on_bind(cls, params): # type: ignore[no-untyped-def]
145+
raise NotImplementedError
146+
147+
@classmethod
148+
def process(cls, params, state, out): # type: ignore[no-untyped-def]
149+
raise NotImplementedError
150+
151+
@classmethod
152+
@abstractmethod
153+
def extra(cls) -> None:
154+
"""Left for a subclass to implement."""
155+
156+
assert WithAbstractClassmethod._setting_params == {}
157+
158+
def test_an_abstract_staticmethod_marks_the_class_abstract(self) -> None:
159+
"""An abstract staticmethod is recognized through its raw attribute too."""
160+
161+
class WithAbstractStaticmethod(TableFunctionGenerator[_NotADataclass, None]): # type: ignore[type-var]
162+
@classmethod
163+
def on_bind(cls, params): # type: ignore[no-untyped-def]
164+
raise NotImplementedError
165+
166+
@classmethod
167+
def process(cls, params, state, out): # type: ignore[no-untyped-def]
168+
raise NotImplementedError
169+
170+
@staticmethod
171+
@abstractmethod
172+
def extra() -> None:
173+
"""Left for a subclass to implement."""
174+
175+
assert WithAbstractStaticmethod._setting_params == {}
176+
177+
def test_an_abstract_property_marks_the_class_abstract(self) -> None:
178+
"""An abstract property is recognized through its raw attribute too."""
179+
180+
class WithAbstractProperty(TableFunctionGenerator[_NotADataclass, None]): # type: ignore[type-var]
181+
@classmethod
182+
def on_bind(cls, params): # type: ignore[no-untyped-def]
183+
raise NotImplementedError
184+
185+
@classmethod
186+
def process(cls, params, state, out): # type: ignore[no-untyped-def]
187+
raise NotImplementedError
188+
189+
@property
190+
@abstractmethod
191+
def extra(self) -> int:
192+
"""Left for a subclass to implement."""
193+
194+
assert WithAbstractProperty._setting_params == {}

‎vgi/table_function.py‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
from __future__ import annotations
1010

11+
import inspect
1112
import uuid
1213
from abc import abstractmethod
1314
from collections.abc import Mapping
@@ -854,8 +855,19 @@ def __init_subclass__(cls) -> None:
854855
cls.FunctionArguments = type_args[0]
855856
break
856857

857-
# Skip validation for abstract base classes
858-
is_abstract = any(getattr(getattr(cls, name, None), "__isabstractmethod__", False) for name in dir(cls))
858+
# Skip validation for abstract base classes. Each attribute is read with
859+
# getattr_static, which returns the raw class attribute without invoking
860+
# descriptors: a plain getattr() here resolved the inherited
861+
# `Function.storage` descriptor, whose first access builds the default
862+
# SQLite store on disk. Because vgi defines subclasses of its own, that
863+
# made `import vgi` alone create a database under the user's state
864+
# directory, and fail outright where that directory is not writable (a
865+
# container's non-root user, say). The raw attribute answers the same
866+
# question: classmethod, staticmethod and property objects all expose
867+
# the `__isabstractmethod__` of what they wrap.
868+
is_abstract = any(
869+
getattr(inspect.getattr_static(cls, name, None), "__isabstractmethod__", False) for name in dir(cls)
870+
)
859871
if is_abstract:
860872
cls._setting_params = {}
861873
cls._secret_params = {}

0 commit comments

Comments
 (0)