From 15423757161db946295bc31f46ebbf0595782829 Mon Sep 17 00:00:00 2001 From: DeVaris Brown Date: Thu, 23 Jul 2026 15:39:33 -0400 Subject: [PATCH 1/9] feat(record,config): OpenCDC record model + pydantic config introspection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lane C of the v0.19 Python connector SDK workstream (docs/design/20260707-python-connector-sdk.md). - conduit/record.py: Record/Change/Operation/Data (bytes | Mapping) per §2.3, plus Metadata well-known-key constants and a representative set of typed accessors (created_at/read_at/collection). - conduit/config.py: BaseConfig (pydantic v2) + Field re-export + to_parameters() introspecting model_fields into config.Parameter/Validation (no codegen, per §2.2). gt=/lt= map exactly; ge=/le= are approximated as exclusive gt/lt (exact for int fields, epsilon-nudged for float, both documented). Literal[...] -> one TYPE_INCLUSION per value. TYPE_DURATION and TYPE_EXCLUSION explicitly raise NotImplementedError (open A-gaps, not silently guessed). - conduit/errors.py: BackoffRetry/BatchWriteError/ConnectorError. BatchWriteError is the B1 data-loss fix (§2.5): construction requires an exhaustive, disjoint success/failures accounting (or a written= prefix); ValueError at construction time if incomplete -- "ack everything not explicitly marked failed" is structurally unrepresentable, not just documented. - pyproject.toml: mypy_path/overrides extended so the generated connector.v2/config.v1/opencdc.v1 stubs (reached via conduit._grpc's sys.path trick, not conduit._grpc.* dotted imports) resolve under mypy; known-first-party isort config so `import conduit._grpc` always sorts before those stub imports (verified: reordering it breaks the sys.path side effect at runtime, not just a style nit). - tests/test_record_codec.py: Hypothesis round-trip tests, including the B3 google.protobuf.Struct int->float precision-loss case pinned exactly (not papered over with `==` laxity). - tests/test_config.py, tests/test_errors.py: mapping-rule and BatchWriteError construction-validation coverage. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD --- pyproject.toml | 65 ++++++- src/conduit/config.py | 342 +++++++++++++++++++++++++++++++++++++ src/conduit/errors.py | 210 +++++++++++++++++++++++ src/conduit/record.py | 209 +++++++++++++++++++++++ tests/test_config.py | 143 ++++++++++++++++ tests/test_errors.py | 91 ++++++++++ tests/test_record_codec.py | 189 ++++++++++++++++++++ 7 files changed, 1247 insertions(+), 2 deletions(-) create mode 100644 src/conduit/config.py create mode 100644 src/conduit/errors.py create mode 100644 src/conduit/record.py create mode 100644 tests/test_config.py create mode 100644 tests/test_errors.py create mode 100644 tests/test_record_codec.py diff --git a/pyproject.toml b/pyproject.toml index 9f89b63..8c23c91 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -28,6 +28,12 @@ dependencies = [ "grpcio>=1.75,<2", "protobuf>=6.31,<8", "pydantic>=2.7,<3", + # Standard package for grpc.health.v1 (SERVING/NOT_SERVING for the + # "plugin" service go-plugin's client probes, §1.1.5) -- not hand-rolled, + # per the Lane B build task and CLAUDE.md ("don't hand-roll the + # standard thing"). Runtime dependency, not dev-only: `conduit.serve` + # registers this health servicer on every plugin process. + "grpcio-health-checking>=1.75,<2", ] [project.urls] @@ -44,6 +50,15 @@ dev = [ "pytest-asyncio>=0.25,<1", "hypothesis>=6.120,<7", "grpc-stubs>=1.24,<2", + # Bounds the hung-event-loop watchdog test (▶ MUST-FIX 3) and the + # deterministic-shutdown test (▶ MUST-FIX 2) so a regression that makes + # either hang fails CI promptly instead of hanging the test run itself. + "pytest-timeout>=2.3,<3", + # Only used by the example connector (examples/http-poll-source/main.py) + # and this repo's acceptance test for it + # (tests/test_example_http_poll_source.py) -- not a runtime dependency + # of the SDK itself. + "httpx>=0.27,<1", ] [tool.hatch.build.targets.wheel] @@ -100,6 +115,22 @@ ignore = [ [tool.ruff.lint.pydocstyle] convention = "google" +[tool.ruff.lint.isort] +# `connector`/`config`/`opencdc` are the generated gRPC/protobuf stubs' +# top-level package names -- reachable only because `conduit._grpc`'s own +# `__init__.py` prepends its directory to `sys.path` at import time (see +# that module's docstring). Without this, ruff's isort treats them as +# third-party (unrecognized names), which sorts the *whole* third-party +# import block before the first-party `import conduit._grpc` line that +# must run first for the sys.path side effect to take effect -- that +# reordering is a real correctness bug (verified: it breaks the import +# chain at runtime, not just a style nit), not a hypothetical one. Treating +# these as known-first-party groups them with `conduit` imports, where +# alphabetical order happens to place `conduit` before `config`/ +# `connector`/`opencdc` -- keeping `ruff check --fix`/`ruff format` +# idempotent against the ordering this code actually depends on. +known-first-party = ["conduit", "config", "connector", "opencdc"] + [tool.ruff.format] quote-style = "double" @@ -108,18 +139,48 @@ quote-style = "double" python_version = "3.11" strict = true packages = ["conduit"] -mypy_path = "src" +# `src/conduit/_grpc` is also on mypy_path (not just `src`): protoc's Python +# codegen emits *absolute* imports rooted at each .proto file's own package +# path (e.g. `from connector.v2 import source_pb2`, `from config.v1 import +# parameter_pb2`), not imports nested under `conduit._grpc` -- see +# `src/conduit/_grpc/__init__.py`'s docstring for why (protobuf codegen's own +# "DO NOT EDIT" output, not patched to nest the imports). At runtime that +# package's `__init__.py` prepends its own directory to `sys.path` so those +# absolute imports resolve; mypy has no equivalent of a runtime sys.path +# mutation, so it needs the same directory on `mypy_path` statically, or it +# can't find `connector.v2`/`config.v1`/`opencdc.v1` as modules at all (this +# was found to be missing, not a pre-existing decision -- see PR description). +mypy_path = ["src", "src/conduit/_grpc"] exclude = [ "src/conduit/_grpc/.*_pb2.*", + # These three subtrees are reached exclusively via the top-level + # `connector`/`config`/`opencdc` names (the second `mypy_path` entry + # above) -- excluding them here too stops mypy's `packages` walk from + # *also* discovering them as `conduit._grpc.connector`/`.config`/ + # `.opencdc`, which otherwise collides ("Source file found twice under + # different module names") since both paths resolve to the same files. + "^src/conduit/_grpc/connector/", + "^src/conduit/_grpc/config/", + "^src/conduit/_grpc/opencdc/", ] [[tool.mypy.overrides]] # Generated stubs are not held to strict mode; they're vendored, regenerated # output (see Lane A, docs/design/20260707-python-connector-sdk.md §1.5). -module = "conduit._grpc.*" +# The `connector.*`/`config.*`/`opencdc.*` module names are the *same* +# generated stubs, reached via the absolute-import top-level names protoc +# emits (see the `mypy_path` comment above) rather than through +# `conduit._grpc.*` -- both patterns need to be excluded for the same +# vendored-code reason, they're just two different names for the same files. +module = ["conduit._grpc.*", "connector.*", "config.*", "opencdc.*"] ignore_errors = true # --- pytest ------------------------------------------------------------------ [tool.pytest.ini_options] asyncio_mode = "auto" testpaths = ["tests"] +# A generous global ceiling (not the watchdog/shutdown tests' own much +# tighter deadlines) so a genuine regression that hangs a test -- e.g. the +# hung-event-loop watchdog silently failing to fire -- fails CI with a +# clear timeout instead of hanging the whole run indefinitely. +timeout = 60 diff --git a/src/conduit/config.py b/src/conduit/config.py new file mode 100644 index 0000000..e300165 --- /dev/null +++ b/src/conduit/config.py @@ -0,0 +1,342 @@ +"""Connector configuration: ``BaseConfig``, ``Field``, ``to_parameters()``. + +See ``docs/design/20260707-python-connector-sdk.md`` §2.2. Go needs +``paramgen`` (a code-generation pass driven by struct tags like +``validate:"required,gt=0,lt=100,inclusion=a|b"``) because Go's runtime +reflection isn't rich enough to turn a struct's field types/tags into a +``config.Parameter`` map without a separate generation step. Pydantic v2's +``model_fields`` already carries type, default, and constraint metadata at +runtime, so this module introspects a model directly -- no codegen, no +``//go:generate``, always in sync with the model because it *is* the model. +""" + +from __future__ import annotations + +import datetime +from dataclasses import dataclass +from typing import Any, Literal, get_args, get_origin + +import annotated_types +import pydantic +from pydantic.fields import FieldInfo + +import conduit._grpc # noqa: F401 -- sets up sys.path, see conduit._grpc.__init__ +from config.v1 import parameter_pb2 as _parameter_pb2 + +Field = pydantic.Field +"""Re-export of :func:`pydantic.Field`. + +A thin re-export rather than a wrapper: pydantic v2's ``Field`` already +carries everything :func:`to_parameters` needs (``description``, ``ge``/ +``le``/``gt``/``lt``, ``pattern``, ``default``) directly in +``model_fields``' ``FieldInfo``. Wrapping it would only add indirection for +no behavioral gain (no speculative generality, per ``CLAUDE.md``). +""" + +# A boundary nudge used only to approximate an *inclusive* pydantic +# constraint (`ge=`/`le=`) as the wire protocol's *exclusive* +# `TYPE_GREATER_THAN`/`TYPE_LESS_THAN` validations for float-typed fields. +# See `_approximate_ge_as_gt`/`_approximate_le_as_lt` below -- this is a +# documented approximation, not exact, and only affects values within this +# epsilon of the declared boundary. +_FLOAT_BOUNDARY_EPSILON = 1e-9 + + +class BaseConfig(pydantic.BaseModel): + """Base class for connector configuration models. + + Subclass this with plain pydantic v2 field declarations (using + :data:`Field` for descriptions/constraints); the SDK introspects the + result via :func:`to_parameters` to build the ``Specify`` RPC's parameter + map and uses the model itself (via ``model_validate``) to parse and + validate the ``Configure`` RPC's ``config: map`` payload. + + Example: + >>> from typing import Literal + >>> class Config(BaseConfig): + ... url: str = Field(description="HTTP endpoint to poll.") + ... poll_interval_ms: int = Field( + ... default=1000, ge=100, description="Delay between empty polls." + ... ) + ... format: Literal["json", "csv"] = Field(default="json") + """ + + @classmethod + def to_parameters(cls) -> dict[str, _parameter_pb2.Parameter]: + """Introspect this model into the ``Specify`` RPC's parameter map. + + Convenience classmethod mirroring the design doc §2.2 ergonomic + (``SourceConfig.to_parameters()``); delegates to the module-level + :func:`to_parameters` function, which is the canonical + implementation and also usable standalone. + """ + return to_parameters(cls) + + +@dataclass(slots=True) +class Specification: + """The static description of a connector plugin, returned by ``Specify``. + + Attributes: + name: short, unique plugin name (e.g. ``"http-poll"``). + version: semver-ish version string (e.g. ``"0.1.0"``). + author: author name or organization. + summary: one-line summary. + description: longer, multi-line description. + + ``source_params``/``destination_params`` are deliberately *not* fields + here: they're computed by :mod:`conduit.serve` from whichever + ``Source``/``Destination`` subclass's ``Config`` was registered, via + :func:`to_parameters`, at ``Specify`` RPC handling time -- keeping this + dataclass a plain, author-supplied literal (matching the design doc + §2.7 call shape: ``Specification(name=..., version=..., author=...)``). + """ + + name: str + version: str + author: str + summary: str = "" + description: str = "" + + +def to_parameters(config_cls: type[BaseConfig]) -> dict[str, _parameter_pb2.Parameter]: + """Introspect a :class:`BaseConfig` subclass into ``config.Parameter``s. + + Mapping rules (design doc §2.2): + + - A field with no default -> ``Validation.TYPE_REQUIRED``. + - ``gt=``/``lt=`` -> exact ``TYPE_GREATER_THAN``/``TYPE_LESS_THAN`` + (the wire validation types are themselves exclusive, matching + pydantic's ``gt``/``lt`` exactly). + - ``ge=``/``le=`` -> **approximated** as ``TYPE_GREATER_THAN``/ + ``TYPE_LESS_THAN`` by nudging the boundary so the declared value + itself still validates: for ``int``-typed fields this is exact + (boundary ``- 1``/``+ 1``); for ``float``-typed fields this uses a + small (``1e-9``) epsilon nudge, which is an approximation, not exact + -- see :data:`_FLOAT_BOUNDARY_EPSILON`. Documented here rather than + silently producing a subtly-wrong validation. + - ``Literal[...]`` -> one ``Validation.TYPE_INCLUSION`` entry per + literal value (``Parameter.validations`` is ``repeated Validation``, + matching the Go SDK's shape). + - ``pattern=`` -> ``Validation.TYPE_REGEX``. + + **Explicitly not attempted (open A-gaps, design doc §2.2, non-blocking + for Phase 1):** + + - ``Parameter.Type.TYPE_DURATION`` (Go's ``"5s"``-style duration + strings, not ISO-8601) has no pydantic-native mapping. A field typed + ``datetime.timedelta`` raises ``NotImplementedError`` rather than + guessing at a wrong mapping. + - ``Validation.Type.TYPE_EXCLUSION`` has no pydantic-native constraint + to introspect. A field requesting it via + ``Field(json_schema_extra={"exclusion": [...]})`` raises + ``NotImplementedError`` rather than silently dropping the + constraint. + + Args: + config_cls: a :class:`BaseConfig` subclass (not instance). + + Returns: + A mapping of field name -> wire ``config.Parameter``, suitable for + ``Specifier.Specify.Response.source_params``/``destination_params``. + + Raises: + NotImplementedError: if a field requests ``TYPE_DURATION`` or + ``TYPE_EXCLUSION`` semantics (see above). + """ + return {name: _field_to_parameter(name, info) for name, info in config_cls.model_fields.items()} + + +def _field_to_parameter(name: str, info: FieldInfo) -> _parameter_pb2.Parameter: + if info.annotation in (datetime.timedelta,): + raise NotImplementedError( + f"config field {name!r}: `datetime.timedelta` (duration) has no " + "pydantic-native mapping to config.Parameter.TYPE_DURATION yet -- " + "open A-gap, docs/design/20260707-python-connector-sdk.md §2.2. " + "Use a plain int (milliseconds) or str field with duration " + "semantics documented in the field description instead." + ) + extra = info.json_schema_extra if isinstance(info.json_schema_extra, dict) else {} + if "exclusion" in extra: + raise NotImplementedError( + f"config field {name!r}: Validation.TYPE_EXCLUSION has no " + "pydantic-native mapping yet -- open A-gap, " + "docs/design/20260707-python-connector-sdk.md §2.2." + ) + + validations: list[_parameter_pb2.Validation] = [] + if info.is_required(): + validations.append(_parameter_pb2.Validation(type=_parameter_pb2.Validation.TYPE_REQUIRED)) + + param_type, literal_values = _resolve_type(info.annotation) + for value in literal_values: + validations.append( + _parameter_pb2.Validation( + type=_parameter_pb2.Validation.TYPE_INCLUSION, value=str(value) + ) + ) + + is_int = param_type == _parameter_pb2.Parameter.TYPE_INT + validations.extend(_constraint_validations(info, is_int=is_int)) + + if info.is_required(): + default = "" + else: + default = _format_default(info.get_default(call_default_factory=True)) + + return _parameter_pb2.Parameter( + default=default, + description=info.description or "", + type=param_type, + validations=validations, + ) + + +def _unwrap_optional(annotation: Any) -> Any: + """Unwrap a single-level ``X | None`` to ``X``; pass through otherwise.""" + origin = get_origin(annotation) + args = get_args(annotation) + if origin is not None and type(None) in args: + non_none = [a for a in args if a is not type(None)] + if len(non_none) == 1: + return non_none[0] + return annotation + + +_ParamTypeAndLiterals = tuple["_parameter_pb2.Parameter.Type", tuple[Any, ...]] + + +def _resolve_type(annotation: Any) -> _ParamTypeAndLiterals: + """Resolve a field annotation to a wire ``Parameter.Type`` + literal values. + + Returns: + A ``(param_type, literal_values)`` pair. ``literal_values`` is + non-empty only for ``Literal[...]`` annotations, and drives the + ``TYPE_INCLUSION`` validations built by the caller. + """ + annotation = _unwrap_optional(annotation) + + if get_origin(annotation) is Literal: + literal_values = get_args(annotation) + if all(isinstance(v, bool) for v in literal_values): + return _parameter_pb2.Parameter.TYPE_BOOL, literal_values + if all(isinstance(v, int) for v in literal_values): + return _parameter_pb2.Parameter.TYPE_INT, literal_values + # Mixed or non-int/bool literal members (str is the common case) -- + # fall back to TYPE_STRING, documented rather than guessed silently. + return _parameter_pb2.Parameter.TYPE_STRING, literal_values + + if annotation is bool: + return _parameter_pb2.Parameter.TYPE_BOOL, () + if annotation is int: + return _parameter_pb2.Parameter.TYPE_INT, () + if annotation is float: + return _parameter_pb2.Parameter.TYPE_FLOAT, () + if annotation is str: + return _parameter_pb2.Parameter.TYPE_STRING, () + + # Unknown/unsupported annotation (e.g. a nested BaseModel, a custom + # type): fall back to TYPE_STRING. This is a deliberate, documented + # approximation -- not a silent guess dressed up as a mapping -- for + # anything this Phase-1 introspection doesn't have a precise wire type + # for. + return _parameter_pb2.Parameter.TYPE_STRING, () + + +def _constraint_validations(info: FieldInfo, *, is_int: bool) -> list[_parameter_pb2.Validation]: + validations: list[_parameter_pb2.Validation] = [] + for constraint in info.metadata: + if isinstance(constraint, annotated_types.Gt): + validations.append( + _parameter_pb2.Validation( + type=_parameter_pb2.Validation.TYPE_GREATER_THAN, + value=str(constraint.gt), + ) + ) + elif isinstance(constraint, annotated_types.Lt): + validations.append( + _parameter_pb2.Validation( + type=_parameter_pb2.Validation.TYPE_LESS_THAN, + value=str(constraint.lt), + ) + ) + elif isinstance(constraint, annotated_types.Ge): + validations.append( + _parameter_pb2.Validation( + type=_parameter_pb2.Validation.TYPE_GREATER_THAN, + value=_approximate_ge_as_gt(constraint.ge, is_int=is_int), + ) + ) + elif isinstance(constraint, annotated_types.Le): + validations.append( + _parameter_pb2.Validation( + type=_parameter_pb2.Validation.TYPE_LESS_THAN, + value=_approximate_le_as_lt(constraint.le, is_int=is_int), + ) + ) + else: + pattern = getattr(constraint, "pattern", None) + if pattern is not None: + validations.append( + _parameter_pb2.Validation( + type=_parameter_pb2.Validation.TYPE_REGEX, value=str(pattern) + ) + ) + return validations + + +def _approximate_ge_as_gt(ge: Any, *, is_int: bool) -> str: + """Approximate an inclusive ``ge=`` bound as the wire's exclusive ``gt``. + + ``ge`` is typed ``Any`` because ``annotated_types.Ge.ge`` is itself + typed against a ``SupportsGe`` structural protocol, not a concrete + numeric type -- in practice pydantic only ever populates it from + ``Field(ge=...)``, which authors pass an ``int``/``float``. + + Exact for ``int``-typed fields (``ge - 1`` admits exactly the same + integers as ``ge`` would inclusively). For ``float``-typed fields this + nudges the boundary down by :data:`_FLOAT_BOUNDARY_EPSILON`, which is an + approximation: values within that epsilon of ``ge`` are handled + correctly, but this is not bit-exact inclusive-boundary semantics. + """ + if is_int: + return str(int(ge) - 1) + return repr(float(ge) - _FLOAT_BOUNDARY_EPSILON) + + +def _approximate_le_as_lt(le: Any, *, is_int: bool) -> str: + """Approximate an inclusive ``le=`` bound as the wire's exclusive ``lt``. + + See :func:`_approximate_ge_as_gt` for why ``le`` is typed ``Any`` -- + exact for ``int``, epsilon-nudged approximation for ``float``. + """ + if is_int: + return str(int(le) + 1) + return repr(float(le) + _FLOAT_BOUNDARY_EPSILON) + + +def _format_default(value: Any) -> str: + """Render a Python default value as the wire's ``Parameter.default`` string. + + Booleans use lowercase ``"true"``/``"false"`` (matching Go's + ``strconv.FormatBool``/JSON convention); ``None`` becomes an empty + string (an accepted approximation -- the wire has no way to distinguish + "no default" from "default is explicitly None", but a required field + with no default already gets ``""`` too via :func:`_field_to_parameter`, + so this is consistent within this SDK's own mapping even if not + perfectly round-trippable against a hypothetical Go-authored consumer + inspecting ``Parameter.default`` for ``None``-ness specifically). + """ + if value is None: + return "" + if isinstance(value, bool): + return "true" if value else "false" + return str(value) + + +__all__ = [ + "BaseConfig", + "Field", + "Specification", + "to_parameters", +] diff --git a/src/conduit/errors.py b/src/conduit/errors.py new file mode 100644 index 0000000..bd124cf --- /dev/null +++ b/src/conduit/errors.py @@ -0,0 +1,210 @@ +"""Connector-author-facing exceptions. + +Python favors exceptions over Go's ``(n, err)`` return convention for error +propagation. See ``docs/design/20260707-python-connector-sdk.md`` §2.5 for the +full rationale, including why this is a genuine simplification (not just a +stylistic swap) over Go's ``Destination.Write(ctx, batch) (n int, err error)`` +contract, which requires the Go SDK to defend an invariant at runtime +(``destination.go:345-350``, re-verified ▶ MUST-FIX 1) that this module makes +structurally unrepresentable instead. +""" + +from __future__ import annotations + +from collections.abc import Mapping, Set + + +class ConnectorError(Exception): + """Base exception for connector-raised errors surfaced over the wire. + + Any exception raised from author code that is not :class:`BackoffRetry` + (source) or :class:`BatchWriteError` (destination) propagates to Conduit + as a gRPC ``INTERNAL`` status with the exception's string as detail -- + ``ConnectorError`` is not required for that, any exception works. This + class exists for authors who want to attach a stable error ``code``. + + Per §2.5: the connector protocol has no stable plugin-originated error + code scheme today (one is flagged as landing around v0.16 of Conduit + itself). ``code`` is reserved for that; until the protocol adds a wire + slot for it, it is always ``None`` from this SDK's perspective and is + **not** transmitted -- only ``str(exception)`` crosses the wire as the + gRPC status detail. + """ + + def __init__(self, message: str, *, code: str | None = None) -> None: + """Initialize with a human-readable message and optional error code. + + Args: + message: human-readable description, becomes ``str(self)``. + code: reserved for a future stable error-code scheme; unused on + the wire today (see class docstring). + """ + super().__init__(message) + self.code = code + + +class BackoffRetry(ConnectorError): + """Raised by ``Source.read()`` to mean "no record right now, retry". + + Direct analog of the Go SDK's ``ErrBackoffRetry``, consumed by the read + loop with a ``Factor=2, Min=100ms, Max=5s`` backoff + (``source.go:270-295``, re-verified ▶ MUST-FIX 1 in the design doc -- a + genuinely serial loop with no concurrent read invocation, so the Python + SDK reusing the identical constants is real behavioral parity). The + SDK's own read loop sleeps between retries -- authors must not also + ``asyncio.sleep()`` (or block) before raising this, or they double the + intended backoff (see the design doc §2.7 worked example's note on this + exact mistake). + """ + + def __init__(self, message: str = "no record available, retry with backoff") -> None: + """Initialize with an optional diagnostic message. + + Args: + message: human-readable reason there's nothing to read right + now; purely diagnostic, never required by callers. + """ + super().__init__(message) + + +class BatchWriteError(ConnectorError): + """Raised by ``Destination.write()`` to report a partial-batch failure. + + This is the SDK's fix for the B1 data-loss blocker identified in the + 2026-07-07 design review (see + ``docs/design/20260707-python-connector-sdk.md`` §2.5 for the full + statement). Go's ``(n, err)`` write contract makes "everything not + explicitly marked as failed was successfully, durably written" an easy + mistake to fall into -- easy enough that the Go SDK itself runs a + defensive runtime guard against it on every batch write + (``destination.go:345-350``, re-verified ▶ MUST-FIX 1, pasted verbatim + in the design doc). This class makes the equivalent mistake **impossible + to construct** in the first place, not merely detected after the fact: + the accounting of which indices succeeded and which failed must be + supplied, in full, at construction time, or ``__init__`` raises + ``ValueError``. + + Two ways to construct it: + + 1. ``BatchWriteError(batch_size, written=N)`` -- the common case, a + contiguous success prefix (Go's ``n``): "everything up to index + ``N - 1`` succeeded, everything from ``N`` on failed." Every index + ``>= N`` is recorded as a generic failure. + 2. ``BatchWriteError(batch_size, success={...}, failures={...})`` -- an + explicit, non-contiguous accounting. ``success`` and ``failures`` + together must cover every index in ``range(batch_size)`` exactly + once (no gaps, no overlaps). + + **An index present in neither accounting is never assumed successful.** + If ``success``/``failures`` don't exhaustively and disjointly cover + every index, construction fails closed with ``ValueError`` -- this is + the concrete mechanism behind the design doc's "banned by construction, + not merely documented" claim. The SDK's destination adapter + (:mod:`conduit.destination`) has no code path that computes "ack + everything not explicitly marked as failed"; it only ever acks indices + present in ``self.success``. + """ + + def __init__( + self, + batch_size: int, + *, + written: int | None = None, + success: Set[int] | None = None, + failures: Mapping[int, BaseException] | None = None, + ) -> None: + """Validate and record an exhaustive per-index write outcome. + + Args: + batch_size: number of records in the batch this error concerns. + Every index accounting is checked against + ``range(batch_size)``. + written: contiguous success-prefix count (Go's ``n``). Mutually + exclusive with ``success``/``failures``. + success: explicit set of successfully, durably written indices. + Must be paired with ``failures``. + failures: explicit mapping of failed index -> the exception that + caused that index's failure. Must be paired with ``success``. + + Raises: + ValueError: if the accounting is missing, incomplete, overlaps + between ``success``/``failures``, or references indices + outside ``range(batch_size)``. This is the fail-closed rule + from §2.5: incompleteness is itself a construction-time + error, never silently resolved by assuming missing indices + succeeded. + """ + if written is not None and (success is not None or failures is not None): + raise ValueError( + "BatchWriteError: pass either `written=` or `success=`/`failures=`, " + "not both -- the two forms are mutually exclusive ways to supply " + "the same exhaustive per-index accounting" + ) + + resolved_success: set[int] + resolved_failures: dict[int, BaseException] + + if written is not None: + if not 0 <= written <= batch_size: + raise ValueError( + f"BatchWriteError: written={written} is out of range for " + f"batch_size={batch_size} (must satisfy 0 <= written <= batch_size)" + ) + resolved_success = set(range(written)) + resolved_failures = { + i: RuntimeError( + "batch write reported only a partial success prefix " + f"(written={written}); index {i} was not reached" + ) + for i in range(written, batch_size) + } + else: + if success is None or failures is None: + raise ValueError( + "BatchWriteError: must supply either `written=`, or both " + "`success=` and `failures=` explicitly. An omitted accounting " + "is never treated as an implicit 'everything else succeeded' -- " + "that is exactly the B1 data-loss bug this exception exists to " + "make unrepresentable (docs/design/20260707-python-connector-sdk.md §2.5)" + ) + resolved_success = set(success) + resolved_failures = dict(failures) + + overlap = resolved_success & resolved_failures.keys() + if overlap: + raise ValueError( + f"BatchWriteError: indices {sorted(overlap)} appear in both " + "`success` and `failures` -- each index must be accounted " + "exactly once" + ) + + all_indices = set(range(batch_size)) + covered = resolved_success | resolved_failures.keys() + missing = all_indices - covered + if missing: + raise ValueError( + f"BatchWriteError: indices {sorted(missing)} of batch_size=" + f"{batch_size} are unaccounted for in either `success` or " + "`failures`. The accounting must be exhaustive: an index " + "present in neither set is a bug in the connector's write(), " + "and must never be assumed to have succeeded (fail-closed, " + "§2.5 B1 fix)" + ) + out_of_range = covered - all_indices + if out_of_range: + raise ValueError( + f"BatchWriteError: indices {sorted(out_of_range)} are outside " + f"range(batch_size={batch_size})" + ) + + self.batch_size = batch_size + self.success: frozenset[int] = frozenset(resolved_success) + self.failures: dict[int, BaseException] = resolved_failures + + summary = "; ".join(f"index {i}: {e}" for i, e in sorted(self.failures.items())) + super().__init__( + f"partial batch write failure: {len(self.failures)}/{batch_size} " + f"record(s) failed ({summary})" + if summary + else "partial batch write failure" + ) diff --git a/src/conduit/record.py b/src/conduit/record.py new file mode 100644 index 0000000..842fdae --- /dev/null +++ b/src/conduit/record.py @@ -0,0 +1,209 @@ +"""The OpenCDC record model: ``Record``, ``Change``, ``Operation``, ``Data``. + +See ``docs/design/20260707-python-connector-sdk.md`` §1.4 (wire shape, +confirmed against ``conduit-commons``' ``proto/opencdc/v1/opencdc.proto``) +and §2.3 (the Python API design and its B3 fidelity caveat). Proto +(de)serialization for these types lives in +:mod:`conduit._grpc.adapters`, not here -- this module is the plain, +dependency-free dataclass shape connector authors write against; the wire +boundary is a separate, internal concern. + +Plain :mod:`dataclasses`, not pydantic, are used here deliberately: records +are produced/consumed at high frequency in the hot path, pydantic's +validation overhead isn't wanted there, and there is nothing to validate -- +the wire format already constrains the shape (§2.3). +""" + +from __future__ import annotations + +import enum +from collections.abc import Mapping +from dataclasses import dataclass, field +from typing import Any + +Data = bytes | Mapping[str, Any] +"""The two-way shape of an OpenCDC record's key/before/after payload. + +Mirrors the wire-level ``oneof`` in ``opencdc.Data`` (§1.4): either raw bytes +or a JSON-like structured mapping. Go needs an interface (``Bytes()``, +``Clone()``, ``ToProto()``) to give ``RawData []byte`` and +``StructuredData map[string]interface{}`` a common contract; Python doesn't, +since ``bytes`` and ``dict``/``Mapping`` already have their own copy +semantics and ``isinstance()`` at the (de)serialization boundary is enough +to pick the right wire branch (§2.3). + +**Known fidelity caveat (B3, design doc §2.3):** structured (mapping) data +crosses the wire as ``google.protobuf.Struct``, which only supports +JSON-like values. Integers are round-tripped as ``Struct``'s ``number_value`` +(a double), so any integer beyond ``2**53`` loses precision silently -- see +:mod:`conduit._grpc.adapters` for exactly where this conversion happens and +``tests/test_record_codec.py`` for the property test pinning this as known, +not accidental, behavior. +""" + + +class Operation(enum.Enum): + """The kind of change a record represents. + + Values match the wire enum ``opencdc.Operation`` exactly + (``OPERATION_CREATE`` = 1 .. ``OPERATION_SNAPSHOT`` = 4), so + :mod:`conduit._grpc.adapters` can convert with a plain ``.value``/ + ``Operation(...)`` round-trip -- no lookup table needed. + """ + + CREATE = 1 + UPDATE = 2 + DELETE = 3 + SNAPSHOT = 4 + + +@dataclass(slots=True) +class Change: + """The before/after payload of a record. + + Attributes: + before: state prior to the change. Only meaningful for ``UPDATE``/ + ``DELETE``; ``None`` otherwise (or when the source doesn't + capture prior state). On the wire, ``None`` is represented as an + ``opencdc.Data`` message with neither ``oneof`` branch set -- + see :mod:`conduit._grpc.adapters`. + after: state following the change. Meaningful for every operation + except ``DELETE``. + """ + + before: Data | None = None + after: Data | None = None + + +@dataclass(slots=True) +class Record: + """A single OpenCDC record: one row/event flowing through a pipeline. + + Attributes: + position: opaque, connector-defined cursor identifying this record's + place in the source. Round-tripped byte-for-byte; the SDK never + interprets its contents. See invariant 2 (positions are + monotonic and crash-safe) -- resuming correctly from a + previously emitted ``position`` is the source author's + responsibility, enforced by the acceptance harness's + resume-at-position tests (:mod:`conduit.testing.acceptance`). + operation: the kind of change this record represents. + metadata: string key/value pairs. See :class:`Metadata` for + well-known keys and typed accessors. + key: the record's key, used for partitioning/routing downstream. + Defaults to empty bytes (not ``None`` -- unlike ``Change``'s + fields, a record always has *some* key on the wire, even if + empty). + payload: the before/after change payload. + """ + + position: bytes + operation: Operation + metadata: dict[str, str] = field(default_factory=dict) + key: Data = b"" + payload: Change = field(default_factory=Change) + + +class Metadata: + """Well-known OpenCDC metadata keys, mirroring Go's ``opencdc`` constants. + + **Sourcing note (see the v0.19 build task's "wire contract facts"):** + ``opencdc_pb2.pyi`` exposes these well-known keys as protobuf extension + ``FieldDescriptor`` objects (e.g. ``metadata_created_at``) carrying the + actual dotted key name as an extension option value on the message + descriptor -- not as plain Python strings. Reflecting on those + descriptors at runtime to recover the option value is possible but adds + real fragility (undocumented internal protobuf API surface) for a value + that is already public, stable, and documented on the Go side + (``conduit-commons``' ``opencdc`` package constants). This module + hardcodes the literal dotted strings instead, matching Go's naming + convention (``opencdc.``). If a future ``compat-nightly`` run + against ``conduit-commons`` HEAD finds one of these has drifted, fix the + literal here -- there is no runtime linkage to the extension descriptors + that would otherwise catch drift automatically. This is a documented, + accepted tradeoff, not an oversight. + + Only a representative subset gets typed accessors (§2.3: "don't need + every Go helper, a representative subset is fine for v0.19 core") -- + ``created_at``/``read_at``/``collection``. Every well-known key still + gets a string constant even without a matching typed accessor, so + authors can always read/write via the plain ``dict`` if they need one + this module doesn't wrap yet. + """ + + OPENCDC_VERSION = "opencdc.version" + CREATED_AT = "opencdc.createdAt" + READ_AT = "opencdc.readAt" + COLLECTION = "opencdc.collection" + KEY_SCHEMA_SUBJECT = "opencdc.key.schema.subject" + KEY_SCHEMA_VERSION = "opencdc.key.schema.version" + PAYLOAD_SCHEMA_SUBJECT = "opencdc.payload.schema.subject" + PAYLOAD_SCHEMA_VERSION = "opencdc.payload.schema.version" + FILE_NAME = "opencdc.file.name" + FILE_SIZE = "opencdc.file.size" + FILE_HASH = "opencdc.file.hash" + FILE_CHUNKED = "opencdc.file.chunked" + FILE_CHUNK_INDEX = "opencdc.file.chunk.index" + FILE_CHUNK_COUNT = "opencdc.file.chunk.count" + + @staticmethod + def set_created_at(metadata: dict[str, str], nanos_since_epoch: int) -> None: + """Set the ``opencdc.createdAt`` key. + + Args: + metadata: the ``Record.metadata`` dict to mutate in place. + nanos_since_epoch: creation time as integer nanoseconds since the + Unix epoch -- matches the design doc §2.7 worked example's + convention (``str(int(datetime.now(UTC).timestamp() * 1e9))``). + """ + metadata[Metadata.CREATED_AT] = str(nanos_since_epoch) + + @staticmethod + def get_created_at(metadata: Mapping[str, str]) -> int | None: + """Read the ``opencdc.createdAt`` key. + + Returns: + Nanoseconds since the Unix epoch, or ``None`` if unset. + """ + raw = metadata.get(Metadata.CREATED_AT) + return int(raw) if raw is not None else None + + @staticmethod + def set_read_at(metadata: dict[str, str], nanos_since_epoch: int) -> None: + """Set the ``opencdc.readAt`` key. + + Args: + metadata: the ``Record.metadata`` dict to mutate in place. + nanos_since_epoch: read time as integer nanoseconds since the + Unix epoch. + """ + metadata[Metadata.READ_AT] = str(nanos_since_epoch) + + @staticmethod + def get_read_at(metadata: Mapping[str, str]) -> int | None: + """Read the ``opencdc.readAt`` key. + + Returns: + Nanoseconds since the Unix epoch, or ``None`` if unset. + """ + raw = metadata.get(Metadata.READ_AT) + return int(raw) if raw is not None else None + + @staticmethod + def set_collection(metadata: dict[str, str], collection: str) -> None: + """Set the ``opencdc.collection`` key (table/topic/collection name). + + Args: + metadata: the ``Record.metadata`` dict to mutate in place. + collection: the source collection name. + """ + metadata[Metadata.COLLECTION] = collection + + @staticmethod + def get_collection(metadata: Mapping[str, str]) -> str | None: + """Read the ``opencdc.collection`` key. + + Returns: + The collection name, or ``None`` if unset. + """ + return metadata.get(Metadata.COLLECTION) diff --git a/tests/test_config.py b/tests/test_config.py new file mode 100644 index 0000000..2cdb366 --- /dev/null +++ b/tests/test_config.py @@ -0,0 +1,143 @@ +"""Tests for :mod:`conduit.config` -- ``BaseConfig``/``Field``/``to_parameters``. + +Covers the design doc §2.2 mapping rules and its documented A-gaps +(``TYPE_DURATION``/``TYPE_EXCLUSION`` raise ``NotImplementedError`` rather +than guessing). +""" + +from __future__ import annotations + +import datetime +from typing import Literal + +import pytest + +import conduit._grpc # noqa: F401 -- sets up sys.path, see conduit._grpc.__init__ +from conduit.config import BaseConfig, Field, Specification, to_parameters +from config.v1 import parameter_pb2 + + +class _RequiredOnly(BaseConfig): + url: str = Field(description="a required string") + + +class _WithBounds(BaseConfig): + ge_int: int = Field(default=1000, ge=100) + le_int: int = Field(default=1, le=100) + gt_float: float = Field(default=1.0, gt=0.0) + lt_float: float = Field(default=1.0, lt=100.0) + + +class _WithLiteral(BaseConfig): + format: Literal["json", "csv"] = Field(default="json") + + +class _WithPattern(BaseConfig): + name: str = Field(default="x", pattern=r"^[a-z]+$") + + +class _WithBool(BaseConfig): + enabled: bool = Field(default=False) + + +class _WithDuration(BaseConfig): + interval: datetime.timedelta = Field(default=datetime.timedelta(seconds=5)) + + +def test_required_field_gets_required_validation() -> None: + params = to_parameters(_RequiredOnly) + param = params["url"] + assert param.type == parameter_pb2.Parameter.TYPE_STRING + assert param.default == "" + types = [v.type for v in param.validations] + assert parameter_pb2.Validation.TYPE_REQUIRED in types + + +def test_field_with_default_is_not_required() -> None: + params = to_parameters(_WithBounds) + types = [v.type for v in params["ge_int"].validations] + assert parameter_pb2.Validation.TYPE_REQUIRED not in types + + +def test_ge_approximated_as_greater_than_admitting_the_boundary() -> None: + """``ge=100`` should admit ``100`` itself -- approximated as ``gt=99`` for ints.""" + params = to_parameters(_WithBounds) + validations = params["ge_int"].validations + assert len(validations) == 1 + assert validations[0].type == parameter_pb2.Validation.TYPE_GREATER_THAN + assert validations[0].value == "99" + + +def test_le_approximated_as_less_than_admitting_the_boundary() -> None: + """``le=100`` should admit ``100`` itself -- approximated as ``lt=101`` for ints.""" + params = to_parameters(_WithBounds) + validations = params["le_int"].validations + assert len(validations) == 1 + assert validations[0].type == parameter_pb2.Validation.TYPE_LESS_THAN + assert validations[0].value == "101" + + +def test_gt_maps_exactly_no_approximation() -> None: + params = to_parameters(_WithBounds) + validations = params["gt_float"].validations + assert len(validations) == 1 + assert validations[0].type == parameter_pb2.Validation.TYPE_GREATER_THAN + assert validations[0].value == "0.0" + + +def test_lt_maps_exactly_no_approximation() -> None: + params = to_parameters(_WithBounds) + validations = params["lt_float"].validations + assert len(validations) == 1 + assert validations[0].type == parameter_pb2.Validation.TYPE_LESS_THAN + assert validations[0].value == "100.0" + + +def test_literal_produces_one_inclusion_validation_per_value() -> None: + params = to_parameters(_WithLiteral) + validations = params["format"].validations + assert {v.value for v in validations} == {"json", "csv"} + assert all(v.type == parameter_pb2.Validation.TYPE_INCLUSION for v in validations) + assert params["format"].type == parameter_pb2.Parameter.TYPE_STRING + + +def test_pattern_maps_to_regex_validation() -> None: + params = to_parameters(_WithPattern) + validations = params["name"].validations + assert len(validations) == 1 + assert validations[0].type == parameter_pb2.Validation.TYPE_REGEX + assert validations[0].value == r"^[a-z]+$" + + +def test_bool_field_maps_to_type_bool_with_lowercase_default() -> None: + params = to_parameters(_WithBool) + param = params["enabled"] + assert param.type == parameter_pb2.Parameter.TYPE_BOOL + assert param.default == "false" + + +def test_duration_field_raises_not_implemented() -> None: + """§2.2's A-gap: TYPE_DURATION has no pydantic-native mapping -- raise, don't guess.""" + with pytest.raises(NotImplementedError, match=r"TYPE_DURATION|duration"): + to_parameters(_WithDuration) + + +def test_exclusion_request_raises_not_implemented() -> None: + """§2.2's A-gap: TYPE_EXCLUSION has no pydantic-native mapping -- raise, don't guess.""" + + class _WithExclusion(BaseConfig): + value: str = Field(default="x", json_schema_extra={"exclusion": ["a", "b"]}) + + with pytest.raises(NotImplementedError, match=r"TYPE_EXCLUSION|exclusion"): + to_parameters(_WithExclusion) + + +def test_base_config_classmethod_matches_module_function() -> None: + assert _RequiredOnly.to_parameters() == to_parameters(_RequiredOnly) + + +def test_specification_is_a_plain_literal_dataclass() -> None: + spec = Specification(name="http-poll", version="0.1.0", author="you") + assert spec.name == "http-poll" + assert spec.summary == "" + assert spec.description == "" diff --git a/tests/test_errors.py b/tests/test_errors.py new file mode 100644 index 0000000..42fcc41 --- /dev/null +++ b/tests/test_errors.py @@ -0,0 +1,91 @@ +"""Tests for :mod:`conduit.errors` -- primarily ``BatchWriteError``'s B1 fix. + +The construction-time validation here is the concrete, automated form of +the design doc's B1 fix (§2.5): "an index present in neither the success +accounting nor the failure accounting is treated as failed, never as +successful" is enforced by making the incomplete/inconsistent construction +itself impossible (``ValueError``), not merely documented. +""" + +from __future__ import annotations + +import pytest + +from conduit.errors import BackoffRetry, BatchWriteError, ConnectorError + + +class TestBatchWriteErrorWrittenPrefix: + def test_written_prefix_marks_exactly_that_range_successful(self) -> None: + err = BatchWriteError(5, written=3) + assert err.success == {0, 1, 2} + assert set(err.failures) == {3, 4} + + def test_written_zero_means_nothing_succeeded(self) -> None: + err = BatchWriteError(3, written=0) + assert err.success == set() + assert set(err.failures) == {0, 1, 2} + + def test_written_equal_to_batch_size_means_everything_succeeded(self) -> None: + err = BatchWriteError(3, written=3) + assert err.success == {0, 1, 2} + assert err.failures == {} + + def test_written_negative_raises_value_error(self) -> None: + with pytest.raises(ValueError, match="out of range"): + BatchWriteError(3, written=-1) + + def test_written_greater_than_batch_size_raises_value_error(self) -> None: + with pytest.raises(ValueError, match="out of range"): + BatchWriteError(3, written=4) + + +class TestBatchWriteErrorExplicitAccounting: + def test_exhaustive_disjoint_accounting_is_accepted(self) -> None: + err = BatchWriteError(4, success={0, 2}, failures={1: ValueError("x"), 3: ValueError("y")}) + assert err.success == {0, 2} + assert set(err.failures) == {1, 3} + + def test_missing_index_raises_value_error_fail_closed(self) -> None: + """The core B1 assertion: an unaccounted index must raise, never silently succeed.""" + with pytest.raises(ValueError, match="unaccounted"): + BatchWriteError(4, success={0, 2}, failures={1: ValueError("x")}) # index 3 missing + + def test_overlapping_index_raises_value_error(self) -> None: + with pytest.raises(ValueError, match="both"): + BatchWriteError(2, success={0, 1}, failures={1: ValueError("x")}) + + def test_out_of_range_index_raises_value_error(self) -> None: + with pytest.raises(ValueError, match=r"outside range|unaccounted"): + BatchWriteError(2, success={0, 1, 5}, failures={}) + + def test_success_without_failures_raises_value_error(self) -> None: + with pytest.raises(ValueError, match="must supply"): + BatchWriteError(2, success={0, 1}) # type: ignore[call-overload] + + def test_failures_without_success_raises_value_error(self) -> None: + with pytest.raises(ValueError, match="must supply"): + BatchWriteError(2, failures={0: ValueError("x")}) # type: ignore[call-overload] + + def test_neither_written_nor_accounting_raises_value_error(self) -> None: + with pytest.raises(ValueError, match="must supply"): + BatchWriteError(2) + + def test_both_written_and_accounting_raises_value_error(self) -> None: + with pytest.raises(ValueError, match="not both"): + BatchWriteError(2, written=1, success={1}, failures={0: ValueError("x")}) + + +class TestBackoffRetry: + def test_is_a_connector_error(self) -> None: + assert isinstance(BackoffRetry(), ConnectorError) + + def test_default_message_is_diagnostic_only(self) -> None: + assert "retry" in str(BackoffRetry()) + + +class TestConnectorError: + def test_code_defaults_to_none(self) -> None: + assert ConnectorError("boom").code is None + + def test_code_is_stored_when_given(self) -> None: + assert ConnectorError("boom", code="E001").code == "E001" diff --git a/tests/test_record_codec.py b/tests/test_record_codec.py new file mode 100644 index 0000000..c33ddba --- /dev/null +++ b/tests/test_record_codec.py @@ -0,0 +1,189 @@ +"""Hypothesis round-trip tests for the OpenCDC record (de)serialization codec. + +Covers design doc §2.3/B3: raw ``bytes`` payloads must round-trip through +``record_to_proto``/``record_from_proto`` with **exact** identity (not +`5 == 5.0`-style laxity). Structured (``dict``) payloads cross +``google.protobuf.Struct``, which represents every JSON-like value +(including integers) as a double -- this file both proves the common case +round-trips and explicitly demonstrates (pins, not merely tolerates) the +known int->float precision-loss case for large integers, per the design +doc's requirement that a test accepting `5 == 5.0` would wrongly pass while +masking that case. +""" + +from __future__ import annotations + +import pytest +from hypothesis import given +from hypothesis import strategies as st + +from conduit._grpc.adapters import record_from_proto, record_to_proto +from conduit.record import Change, Operation, Record + +# JSON-like scalar/collection strategy for `structured_data`, deliberately +# bounded to integers that fit in a double's 53-bit mantissa exactly -- this +# is the "faithful round-trip" side of B3; the precision-loss side is tested +# separately and explicitly below, not folded into this "should round-trip +# exactly" strategy. +_EXACT_JSON_INT = st.integers(min_value=-(2**53), max_value=2**53) +_JSON_SCALAR = st.one_of( + st.none(), + st.booleans(), + _EXACT_JSON_INT, + st.floats(allow_nan=False, allow_infinity=False, width=32), + st.text(max_size=20), +) +_JSON_VALUE = st.recursive( + _JSON_SCALAR, + lambda children: st.one_of( + st.lists(children, max_size=3), + st.dictionaries(st.text(max_size=10), children, max_size=3), + ), + max_leaves=6, +) +_STRUCTURED_DATA = st.dictionaries(st.text(min_size=1, max_size=10), _JSON_VALUE, max_size=5) + +_OPERATIONS = st.sampled_from(list(Operation)) +_METADATA = st.dictionaries(st.text(max_size=10), st.text(max_size=10), max_size=5) + + +@given( + position=st.binary(max_size=32), + operation=_OPERATIONS, + metadata=_METADATA, + key=st.binary(max_size=32), + before=st.one_of(st.none(), st.binary(max_size=32)), + after=st.one_of(st.none(), st.binary(max_size=32)), +) +def test_raw_bytes_round_trip_is_exact( + position: bytes, + operation: Operation, + metadata: dict[str, str], + key: bytes, + before: bytes | None, + after: bytes | None, +) -> None: + """Raw ``bytes`` payloads round-trip through proto with exact identity. + + Asserts real equality (``==``) on ``bytes`` objects -- there is no + "close enough" for raw bytes, unlike the structured-data/int case below. + """ + record = Record( + position=position, + operation=operation, + metadata=metadata, + key=key, + payload=Change(before=before, after=after), + ) + + decoded = record_from_proto(record_to_proto(record)) + + assert decoded.position == position + assert decoded.operation == operation + assert decoded.metadata == metadata + assert decoded.key == key + assert isinstance(decoded.key, bytes) + assert decoded.payload.before == before + assert decoded.payload.after == after + + +@given(key=_STRUCTURED_DATA, after=_STRUCTURED_DATA) +def test_structured_data_with_exact_ints_round_trips( + key: dict[str, object], after: dict[str, object] +) -> None: + """Structured (``dict``) payloads round-trip when integers fit a double exactly. + + Bounded to ``abs(value) <= 2**53`` (:data:`_EXACT_JSON_INT`) -- within + that range, ``google.protobuf.Struct``'s double-precision + ``number_value`` representation loses no information, so decoding + yields back a `float` that compares equal to the original `int` via + Python's numeric tower (``5 == 5.0``). This test is intentionally + narrower than "any Python dict" -- see + :func:`test_large_int_in_structured_data_loses_precision_silently` for + the case this strategy is bounded specifically to exclude. + """ + record = Record( + position=b"pos", + operation=Operation.CREATE, + key=key, + payload=Change(after=after), + ) + + decoded = record_from_proto(record_to_proto(record)) + + assert decoded.key == key + assert decoded.payload.after == after + + +def test_large_int_in_structured_data_loses_precision_silently() -> None: + """Pin the B3 int->float precision loss for large integers -- silent, not an error. + + ``google.protobuf.Struct`` has no integer type; every JSON-like number + is a double. ``2**60 + 1`` is not exactly representable as a double, so + it decodes back as a **different**, nearby integer -- with no exception + raised anywhere in the encode/decode path. This test exists specifically + to fail if a future change makes this silently "look like it round-trips" + (e.g. by accepting `!=` as a bug and papering over it with rounding) -- + the documented contract (design doc §2.3/B3) is that this precision loss + is known and must remain visible, not hidden. + """ + large_int = 2**60 + 1 # not exactly representable as an IEEE 754 double + record = Record( + position=b"pos", + operation=Operation.CREATE, + payload=Change(after={"count": large_int}), + ) + + decoded = record_from_proto(record_to_proto(record)) + + assert isinstance(decoded.payload.after, dict) + decoded_count = decoded.payload.after["count"] + # The precision loss itself, pinned exactly: silently a `float`, and + # silently a different numeric value than the original `int`. + assert isinstance(decoded_count, float) + assert decoded_count != large_int + assert decoded_count == float(large_int) # exactly what a double *can* represent + + +def test_bytes_inside_structured_data_fails_loudly_not_silently() -> None: + """Contrast case for B3: bytes (unlike large ints) fail loudly, not silently. + + ``google.protobuf.Struct`` has no representation for raw bytes inside a + structured value at all -- this raises immediately at encode time, + unlike the integer case, which succeeds but silently loses precision. + Documented at the exact conversion site, + ``conduit._grpc.adapters._data_to_proto``. + """ + record = Record( + position=b"pos", + operation=Operation.CREATE, + payload=Change(after={"blob": b"not json-representable"}), + ) + + with pytest.raises(ValueError, match="Unexpected type"): + record_to_proto(record) + + +def test_empty_raw_data_round_trips_as_empty_bytes_not_none() -> None: + """An explicitly empty ``key=b""`` decodes back to ``b""``, not ``None``. + + Exercises the ``WhichOneof`` distinction documented in + ``conduit._grpc.adapters._data_from_proto``: presence of the ``raw_data`` + oneof branch (even when empty) is different from the branch being unset. + """ + record = Record(position=b"p", operation=Operation.CREATE, key=b"") + decoded = record_from_proto(record_to_proto(record)) + assert decoded.key == b"" + assert isinstance(decoded.key, bytes) + + +def test_change_before_and_after_none_round_trip_as_none() -> None: + """``Change.before``/``after`` left as ``None`` decode back to ``None``, not ``b""``/``{}``. + + This is the "absent" case :func:`conduit._grpc.adapters._data_from_proto_optional` + exists specifically to distinguish from an explicit empty payload. + """ + record = Record(position=b"p", operation=Operation.CREATE, payload=Change()) + decoded = record_from_proto(record_to_proto(record)) + assert decoded.payload.before is None + assert decoded.payload.after is None From bb8ce33e2403ab5f896aa53f02e43757dcface0d Mon Sep 17 00:00:00 2001 From: DeVaris Brown Date: Thu, 23 Jul 2026 15:39:51 -0400 Subject: [PATCH 2/9] feat(source,destination,serve): Source/Destination lifecycle over gRPC v2 + shutdown watchdog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lane B of the v0.19 Python connector SDK workstream (docs/design/20260707-python-connector-sdk.md). - conduit/_dispatch.py: dual sync/async method dispatch (inspect.iscoroutinefunction detection, sync overrides run in the default thread-pool executor) shared by Source and Destination, per §2.1. - conduit/_introspect.py: shared generic-parameter recovery (Source[Config]/Destination[Config] -> Config) used to validate Configure's config map and build Specify's parameter map without author boilerplate. - conduit/source.py: Source ABC (abc.ABC, read() abstract) + _SourceServicer adapting it to SourcePluginServicer. Read loop reuses the Go SDK's exact backoff constants (Factor=2, Min=100ms, Max=5s, source.go:270-295, re-verified MUST-FIX 1) as a single serial loop. Invariant 1: ack() is called only from _consume_acks, driven only by Conduit's ack_positions on the Run request stream -- the read loop itself never calls ack(). - conduit/destination.py: Destination ABC (write() abstract) + _DestinationServicer. _write_batch is the B1 enforcement site: full success only after write() returns cleanly; BatchWriteError's already-validated success/failures accounting drives every ack/nack decision; any other exception nacks the entire batch. No code path acks an index absent from the exhaustive success set. - conduit/_grpc/_controller.py: hand-written (not generated -- go-plugin's own internal proto, outside conduit-connector-protocol) GRPCController.Shutdown service, registered via grpc.method_handlers_generic_handler. - conduit/_grpc/adapters.py: Record/Data/Change <-> proto conversion. The B3 google.protobuf.Struct int->float boundary is documented at its one exact call site (_data_to_proto). - conduit/serve.py: serve() entry point -- handshake validation (reusing _handshake.py, Lane A), grpc.aio server bootstrap, health/specifier/ connector servicer registration, and _ShutdownCoordinator: the hung-event-loop watchdog (MUST-FIX 3). SIGTERM is caught with low-level signal.signal (not loop.add_signal_handler, which cannot fire if the loop is wedged) to start an independent threading.Timer that force-exits after a bounded, configurable deadline if graceful shutdown (teardown() + GRPCController.Shutdown) hasn't confirmed completion. - tests/test_source.py, tests/test_destination.py: ack-ordering and B1 partial-batch-nack coverage (test_destination_partial_write_nacks_all's three cases: incomplete accounting, well-formed written= prefix, non-BatchWriteError exception). - tests/test_serve.py: the deterministic shutdown test (MUST-FIX 2) -- a real grpc.aio server, a real gRPC client calling /plugin.GRPCController/Shutdown, asserting the RPC succeeds and teardown() ran exactly once beforehand via a spy, not a timing race -- plus the hung-loop watchdog tests (MUST-FIX 3), including one that genuinely wedges a real event loop in a background thread and confirms the watchdog still fires within its documented deadline. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD --- src/conduit/__init__.py | 44 ++- src/conduit/_dispatch.py | 80 +++++ src/conduit/_grpc/_controller.py | 83 +++++ src/conduit/_grpc/adapters.py | 188 +++++++++++ src/conduit/_introspect.py | 49 +++ src/conduit/destination.py | 281 ++++++++++++++++ src/conduit/serve.py | 532 +++++++++++++++++++++++++++++++ src/conduit/source.py | 351 ++++++++++++++++++++ tests/test_destination.py | 190 +++++++++++ tests/test_serve.py | 268 ++++++++++++++++ tests/test_source.py | 190 +++++++++++ 11 files changed, 2243 insertions(+), 13 deletions(-) create mode 100644 src/conduit/_dispatch.py create mode 100644 src/conduit/_grpc/_controller.py create mode 100644 src/conduit/_grpc/adapters.py create mode 100644 src/conduit/_introspect.py create mode 100644 src/conduit/destination.py create mode 100644 src/conduit/serve.py create mode 100644 src/conduit/source.py create mode 100644 tests/test_destination.py create mode 100644 tests/test_serve.py create mode 100644 tests/test_source.py diff --git a/src/conduit/__init__.py b/src/conduit/__init__.py index 4575adc..a97714f 100644 --- a/src/conduit/__init__.py +++ b/src/conduit/__init__.py @@ -1,22 +1,40 @@ """Python SDK for building Conduit source and destination connectors. -**Week-1 scaffold status:** this package currently exposes only what Lane A -of the v0.19 build plan lands: the vendored gRPC/protobuf stubs -(:mod:`conduit._grpc`) and the go-plugin handshake implementation -(:mod:`conduit._handshake`). The public author-facing surface described in -``docs/design/20260707-python-connector-sdk.md`` §2 -- ``Source``, -``Destination``, ``Record``, ``Operation``, ``Change``, ``serve``, -``BaseConfig`` -- is Lane B/C scope and is **not implemented yet**. Importing -those names from this package will fail with ``ImportError`` until Lane B -lands; do not assume otherwise from this docstring or the README's aspirational -repo-layout description. +Public author-facing surface, per +``docs/design/20260707-python-connector-sdk.md`` §2/§3: ``Source``, +``Destination``, ``Record``, ``Change``, ``Operation``, ``BaseConfig``, +``Field``, ``Specification``, ``serve``, and the connector-facing +exceptions (``BackoffRetry``, ``BatchWriteError``, ``ConnectorError``). -See ``docs/design/20260707-python-connector-sdk.md`` for the full design and -``CONTRIBUTING.md`` for the Tier-1 review bar this package is held to. +See ``CONTRIBUTING.md`` for the Tier-1 review bar this package is held to. """ from __future__ import annotations +from conduit.config import BaseConfig, Field, Specification, to_parameters +from conduit.destination import Destination +from conduit.errors import BackoffRetry, BatchWriteError, ConnectorError +from conduit.record import Change, Data, Metadata, Operation, Record +from conduit.serve import serve +from conduit.source import Source + __version__ = "0.1.0.dev0" -__all__ = ["__version__"] +__all__ = [ + "BackoffRetry", + "BaseConfig", + "BatchWriteError", + "Change", + "ConnectorError", + "Data", + "Destination", + "Field", + "Metadata", + "Operation", + "Record", + "Source", + "Specification", + "__version__", + "serve", + "to_parameters", +] diff --git a/src/conduit/_dispatch.py b/src/conduit/_dispatch.py new file mode 100644 index 0000000..008c5cb --- /dev/null +++ b/src/conduit/_dispatch.py @@ -0,0 +1,80 @@ +"""Dual sync/async method dispatch shared by ``Source`` and ``Destination``. + +Per ``docs/design/20260707-python-connector-sdk.md`` §2.1: base class +methods are declared ``async def``, but an author whose target system's +client library is sync-only (many DB drivers still are) may override with a +plain ``def`` instead. This module is the one place that detects which kind +of callable an override is and dispatches accordingly -- the same dual-mode +ergonomic FastAPI uses for path operations, "sync as a first-class option, +not a fallback hack" (§2.1). +""" + +from __future__ import annotations + +import asyncio +import functools +import inspect +from collections.abc import Awaitable, Callable +from typing import ParamSpec, TypeVar, cast + +P = ParamSpec("P") +R = TypeVar("R") + + +async def invoke(func: Callable[P, Awaitable[R]], *args: P.args, **kwargs: P.kwargs) -> R: + """Call ``func``, awaiting it if async, else running it off the event loop. + + Statically, every call site in this SDK passes an attribute of + :class:`~conduit.source.Source`/:class:`~conduit.destination.Destination` + (e.g. ``self._source.read``), which are declared ``async def`` on the + ABC -- so ``func``'s declared type is always ``Callable[P, + Awaitable[R]]`` from the type checker's point of view, even though at + *runtime* an author may have overridden the method with a plain + ``def`` (§2.1's dual-mode contract; a sync override is a Liskov-style + return-type mismatch as far as static typing is concerned, but Python + doesn't enforce that, and this function's runtime behavior handles it + correctly regardless). ``inspect.iscoroutinefunction`` inspects the + actual object at runtime, independent of what mypy believes its type + is. + + A sync (plain ``def``) override runs in the default + ``concurrent.futures.ThreadPoolExecutor`` via + ``loop.run_in_executor`` -- never called inline on the event loop -- so a + blocking author callable (a sync DB driver call, a blocking HTTP + request) cannot itself wedge the loop. This is the SDK's one sanctioned + boundary where a synchronous, potentially-blocking callable is invoked + from async code (ruff's ``ASYNC`` lint rules flag blocking calls inside + ``async def`` bodies elsewhere in this codebase; this function is where + that concern is deliberately, correctly handled rather than avoided). + + Note this does not by itself fully close the hung-event-loop failure + mode (▶ MUST-FIX 3, design doc): a sync override that blocks + indefinitely still occupies a thread-pool worker indefinitely, and if + every worker is exhausted, subsequent ``run_in_executor`` calls queue + rather than run. The bounded watchdog in :mod:`conduit.serve` is the + mitigation for a genuinely wedged process; this function's job is only + to avoid *itself* being the thing that blocks the event loop for a + single call. + + Args: + func: the (possibly bound) callable to invoke -- either an + ``async def`` or (at runtime only) a plain ``def``. + *args: positional arguments to pass through. + **kwargs: keyword arguments to pass through. + + Returns: + Whatever ``func`` returns (or its awaited result, if a coroutine + function). + """ + if inspect.iscoroutinefunction(func): + return await func(*args, **kwargs) + # Reached only when `func` is, at runtime, actually a plain sync `def` + # (an author's dual-mode override, §2.1) -- its *real* return type is + # `R`, not `Awaitable[R]`, even though `func`'s declared static type + # (this SDK's own `async def` ABC methods) says otherwise. This cast + # documents that gap explicitly rather than silencing it with a bare + # `type: ignore`. + sync_func = cast(Callable[P, R], func) + loop = asyncio.get_running_loop() + bound = functools.partial(sync_func, *args, **kwargs) + return await loop.run_in_executor(None, bound) diff --git a/src/conduit/_grpc/_controller.py b/src/conduit/_grpc/_controller.py new file mode 100644 index 0000000..9600ab5 --- /dev/null +++ b/src/conduit/_grpc/_controller.py @@ -0,0 +1,83 @@ +"""Hand-written HashiCorp go-plugin ``GRPCController`` service. + +**Not generated, and not part of ``conduit-connector-protocol``.** go-plugin +(https://github.com/hashicorp/go-plugin) defines this service internally, +for its own bookkeeping -- it is not one of the ``conduit-connector-protocol`` +BSR-published ``.proto`` files this repo's ``buf generate`` step covers (see +``buf.gen.yaml``, ``tools/generate-stubs.sh``). There is nothing to +regenerate here; this module is ordinary, hand-written application code held +to the repo's normal lint/mypy-strict/docstring bar (unlike its sibling +``*_pb2*.py``/``*.pyi`` files elsewhere in ``_grpc/``, which are vendored +codegen output). + +Per ``docs/design/20260707-python-connector-sdk.md`` §1.1.5: go-plugin's +client RPCs ``GRPCController.Shutdown`` on teardown +(``grpc_client.go:106-108``); if a plugin doesn't implement it, ``Close()`` +errors and Conduit force-kills the process ~2s later instead +(``client.go:530-567``) -- the pipeline still tears down, just not +gracefully. Implementing this RPC is what makes shutdown go through the +clean path rather than the timeout fallback, per ``CLAUDE.md`` invariant 7 +(graceful shutdown by default). + +Both the ``Shutdown`` request and response are zero-field messages on the +wire -- go-plugin's own ``plugin.Empty`` has no fields, and neither does +``google.protobuf.Empty``, so the two serialize identically. Using +``google.protobuf.Empty`` here avoids hand-rolling a wire-identical message +type or adding a codegen step for a two-message, zero-field service. +""" + +from __future__ import annotations + +from collections.abc import Awaitable, Callable + +import grpc +from google.protobuf import empty_pb2 + +_ServicerContext = "grpc.aio.ServicerContext[empty_pb2.Empty, empty_pb2.Empty]" +ShutdownHandler = Callable[[empty_pb2.Empty, _ServicerContext], Awaitable[empty_pb2.Empty]] +"""Signature of the async callable invoked for the ``Shutdown`` RPC. + +Takes the (empty) request and the servicer context, returns the (empty) +response -- the standard grpc.aio unary-unary handler shape, so callers can +pass any coroutine function matching it, not just a fixed no-args callback. +""" + +SERVICE_NAME = "plugin.GRPCController" +"""go-plugin's own internal service name -- verbatim, not a Conduit constant.""" + +SHUTDOWN_METHOD = "Shutdown" +"""The single method go-plugin's internal service defines.""" + + +def build_controller_handler(on_shutdown: ShutdownHandler) -> grpc.GenericRpcHandler: + """Build the generic RPC handler implementing ``plugin.GRPCController``. + + Registered via ``server.add_generic_rpc_handlers`` (see + :mod:`conduit.serve`) rather than a generated ``add_*Servicer_to_server`` + function, since there is no generated stub for this service (see module + docstring). + + Args: + on_shutdown: async callable invoked when Conduit calls + ``GRPCController.Shutdown``. Callers are responsible for + running the connector's ``teardown()`` to completion and + arranging for the gRPC server/process to stop *after* this + handler returns its response -- not before, or Conduit would + see the RPC fail rather than complete cleanly. + + Returns: + A ``grpc.GenericRpcHandler`` ready to pass to + ``server.add_generic_rpc_handlers((handler,))``. Works with both + ``grpc.server()`` and ``grpc.aio.server()`` -- the handler behavior + callable is a coroutine function, which ``grpc.aio`` dispatches + natively via the same generic-handler registration path as sync + ``grpc``. + """ + rpc_method_handlers = { + SHUTDOWN_METHOD: grpc.unary_unary_rpc_method_handler( + on_shutdown, + request_deserializer=empty_pb2.Empty.FromString, + response_serializer=empty_pb2.Empty.SerializeToString, + ), + } + return grpc.method_handlers_generic_handler(SERVICE_NAME, rpc_method_handlers) diff --git a/src/conduit/_grpc/adapters.py b/src/conduit/_grpc/adapters.py new file mode 100644 index 0000000..308a9cd --- /dev/null +++ b/src/conduit/_grpc/adapters.py @@ -0,0 +1,188 @@ +"""Hand-written adapters between wire (proto) messages and Python dataclasses. + +**Not generated.** This module lives inside ``_grpc/`` because it is +protocol glue tightly coupled to the generated stubs, but it is +hand-written, ordinary application code -- held to the repo's normal +lint/mypy-strict/docstring bar, unlike its siblings +(``*_pb2*.py``/``*.pyi``), which are vendored codegen output excluded from +those checks (see ``pyproject.toml`` and this package's own +``__init__.py``). + +See ``docs/design/20260707-python-connector-sdk.md`` §1.4/§2.3 for the wire +shape this translates and the B3 ``google.protobuf.Struct`` int->float +fidelity caveat, which is implemented (and documented) at its one exact +conversion site below: :func:`_data_to_proto`. +""" + +from __future__ import annotations + +from collections.abc import Iterable, Mapping + +from google.protobuf import struct_pb2 +from google.protobuf.json_format import MessageToDict + +import conduit._grpc # noqa: F401 -- sets up sys.path, see conduit._grpc.__init__ +from conduit.record import Change, Data, Operation, Record +from opencdc.v1 import opencdc_pb2 + +__all__ = [ + "config_map_from_proto", + "record_from_proto", + "record_to_proto", + "records_from_proto", + "records_to_proto", +] + + +def _data_to_proto(data: Data) -> opencdc_pb2.Data: + """Encode a Python ``Data`` (``bytes | Mapping[str, Any]``) as wire ``Data``. + + **B3 fidelity boundary (design doc §2.3):** when ``data`` is a + ``Mapping``, it is encoded via ``google.protobuf.Struct.update()``, + which represents every JSON-like value including integers as a + double-precision ``number_value``. Integers beyond ``2**53`` lose + precision here -- **silently**, with no exception raised -- unlike + passing raw ``bytes`` inside a structured mapping, which fails loudly + with a ``ValueError`` at this exact call (``Struct`` has no bytes + representation). ``tests/test_record_codec.py`` pins the integer case + with a Hypothesis test that asserts exact round-trip identity for raw + bytes and explicitly demonstrates (not merely tolerates) the int->float + precision loss for large integers, per the design doc's B3 requirement. + + Args: + data: raw bytes, or a JSON-like structured mapping. + + Returns: + The wire ``opencdc.Data`` message with the corresponding ``oneof`` + branch set. + """ + if isinstance(data, bytes): + return opencdc_pb2.Data(raw_data=data) + struct = struct_pb2.Struct() + struct.update(data) # <-- B3: int -> float precision loss happens here. + return opencdc_pb2.Data(structured_data=struct) + + +def _data_from_proto(data: opencdc_pb2.Data) -> Data: + """Decode a wire ``Data`` message to a Python ``Data``. + + Uses ``WhichOneof`` rather than checking field presence/truthiness, so + an explicitly-set empty ``raw_data = b""`` correctly decodes to + ``b""``, not to a structured empty dict. + + Args: + data: the wire ``opencdc.Data`` message. + + Returns: + ``bytes`` if the ``raw_data`` branch is set, otherwise a ``dict`` + decoded from the ``structured_data`` branch (via + ``MessageToDict``, which recursively converts nested ``Struct``/ + ``ListValue`` to plain Python containers -- see :func:`_data_to_proto` + for where the corresponding, one-directional precision loss is + introduced on encode). + """ + which = data.WhichOneof("data") + if which == "raw_data": + return data.raw_data + if which == "structured_data": + return MessageToDict(data.structured_data) + # Neither oneof branch set -- an explicitly "empty" Data on the wire. + # Records always carry *some* key (§2.3: `key: Data = b""`), so this + # decodes to empty bytes, matching that default. + return b"" + + +def _data_to_proto_optional(data: Data | None) -> opencdc_pb2.Data: + """Encode ``Data | None`` (``Change.before``/``after``) to wire ``Data``. + + ``None`` becomes a ``Data`` message with neither ``oneof`` branch set -- + the wire has no explicit "absent" representation for ``Change``'s + fields beyond that, matching proto3 message-field-as-optional semantics. + """ + if data is None: + return opencdc_pb2.Data() + return _data_to_proto(data) + + +def _data_from_proto_optional(data: opencdc_pb2.Data) -> Data | None: + """Decode wire ``Data`` to ``Data | None`` for ``Change.before``/``after``. + + Returns ``None`` when neither ``oneof`` branch is set, distinguishing + "absent" from an explicit empty payload -- see :func:`_data_from_proto`, + which cannot make this distinction for ``Record.key`` (always present). + """ + if data.WhichOneof("data") is None: + return None + return _data_from_proto(data) + + +def _change_to_proto(change: Change) -> opencdc_pb2.Change: + """Encode a :class:`~conduit.record.Change` to wire ``opencdc.Change``.""" + return opencdc_pb2.Change( + before=_data_to_proto_optional(change.before), + after=_data_to_proto_optional(change.after), + ) + + +def _change_from_proto(change: opencdc_pb2.Change) -> Change: + """Decode a wire ``opencdc.Change`` to :class:`~conduit.record.Change`.""" + return Change( + before=_data_from_proto_optional(change.before), + after=_data_from_proto_optional(change.after), + ) + + +def record_to_proto(record: Record) -> opencdc_pb2.Record: + """Encode a :class:`~conduit.record.Record` to wire ``opencdc.Record``. + + Args: + record: the Python-side record. + + Returns: + The equivalent wire message. + """ + return opencdc_pb2.Record( + position=record.position, + operation=record.operation.value, + metadata=dict(record.metadata), + key=_data_to_proto(record.key), + payload=_change_to_proto(record.payload), + ) + + +def record_from_proto(record: opencdc_pb2.Record) -> Record: + """Decode a wire ``opencdc.Record`` to :class:`~conduit.record.Record`. + + Args: + record: the wire-side record. + + Returns: + The equivalent Python dataclass. + """ + return Record( + position=record.position, + operation=Operation(record.operation), + metadata=dict(record.metadata), + key=_data_from_proto(record.key), + payload=_change_from_proto(record.payload), + ) + + +def records_to_proto(records: Iterable[Record]) -> list[opencdc_pb2.Record]: + """Encode an iterable of :class:`~conduit.record.Record` to wire records.""" + return [record_to_proto(r) for r in records] + + +def records_from_proto(records: Iterable[opencdc_pb2.Record]) -> list[Record]: + """Decode an iterable of wire records to :class:`~conduit.record.Record`.""" + return [record_from_proto(r) for r in records] + + +def config_map_from_proto(config: Mapping[str, str]) -> dict[str, str]: + """Copy a wire ``map`` (``ScalarMap``) into a plain ``dict``. + + A thin wrapper (rather than passing the ``ScalarMap`` proxy directly to + ``BaseConfig.model_validate``) so callers never hold a reference into + the proto message's internal map storage past the request's lifetime. + """ + return dict(config) diff --git a/src/conduit/_introspect.py b/src/conduit/_introspect.py new file mode 100644 index 0000000..a66d36e --- /dev/null +++ b/src/conduit/_introspect.py @@ -0,0 +1,49 @@ +"""Shared generic-parameter introspection for ``Source``/``Destination``. + +Both ``Source[ConfigT]`` and ``Destination[ConfigT]`` need to recover the +concrete ``ConfigT`` an author parameterized their subclass with (e.g. +``class HTTPPollSource(Source[Config])``) so the SDK can validate the +``Configure`` RPC's config map against it and introspect it (via +:func:`conduit.config.to_parameters`) for the ``Specify`` RPC -- without the +author writing any boilerplate ``config_class = Config`` class attribute. +This one small helper is shared by both modules rather than duplicated. +""" + +from __future__ import annotations + +import typing + + +def resolve_config_class(cls: type, base: type) -> type: + """Find the concrete type argument a subclass parameterized ``base`` with. + + Walks ``cls.__mro__`` looking for a class in the chain whose + ``__orig_bases__`` includes a parameterized generic alias of ``base`` + (e.g. ``Source[Config]``), and returns that type argument. + + Args: + cls: the concrete ``Source``/``Destination`` subclass an author + wrote, e.g. ``HTTPPollSource``. + base: the generic base class to look for, e.g. ``Source``. + + Returns: + The concrete config class, e.g. ``Config``. + + Raises: + TypeError: if no ancestor in ``cls``'s MRO parameterizes ``base`` + with a concrete type -- i.e. the author wrote + ``class Foo(Source):`` instead of ``class Foo(Source[Config]):``. + """ + for klass in cls.__mro__: + for orig_base in getattr(klass, "__orig_bases__", ()): + if typing.get_origin(orig_base) is base: + args = typing.get_args(orig_base) + if args and isinstance(args[0], type): + return args[0] + raise TypeError( + f"{cls.__name__} must parameterize {base.__name__} with a concrete " + f"config class, e.g. `class {cls.__name__}({base.__name__}[YourConfig]):" + " ...` -- the SDK introspects this to validate the `Configure` RPC's " + "config map and to build the `Specify` RPC's parameter map " + "(see docs/design/20260707-python-connector-sdk.md §2.2/§2.4)." + ) diff --git a/src/conduit/destination.py b/src/conduit/destination.py new file mode 100644 index 0000000..c0bfe82 --- /dev/null +++ b/src/conduit/destination.py @@ -0,0 +1,281 @@ +"""Destination connector base class + the ``DestinationPlugin`` gRPC adapter. + +See ``docs/design/20260707-python-connector-sdk.md`` §2.1 (async/dual-mode), +§2.4 (forward-compatible ABC), and **§2.5 (the B1 fix -- the single most +important correctness property in this repo)**. This module holds both +halves: the author-facing :class:`Destination` ABC, and the internal +:class:`_DestinationServicer` adapting it to the generated +``DestinationPluginServicer``. + +**B1, restated precisely at its enforcement site (see +:meth:`_DestinationServicer._write_batch`):** a naive translation of "catch +the first exception, treat every index not present in the exception's map +as successful" is the exact bug Go's ``(n, err)`` contract has to defend +against at runtime (``destination.go:345-350``, re-verified ▶ MUST-FIX 1) -- +"absence of an error entry" read as "acked" is a direct invariant 1/3 +violation (a record never durably written gets acked). This module's write +adapter never does that: every ack decision is driven by +:class:`~conduit.errors.BatchWriteError`'s own exhaustive, construction-time- +validated accounting (see :mod:`conduit.errors`), or -- for any other +exception -- nacks the entire batch outright. +""" + +from __future__ import annotations + +import abc +from collections.abc import AsyncIterator, Sequence +from typing import Any, Generic, TypeVar + +import conduit._grpc # noqa: F401 -- sets up sys.path, see conduit._grpc.__init__ +from conduit._dispatch import invoke +from conduit._grpc.adapters import config_map_from_proto, records_from_proto +from conduit._introspect import resolve_config_class +from conduit.config import BaseConfig +from conduit.errors import BatchWriteError +from conduit.record import Record +from connector.v2 import destination_pb2, destination_pb2_grpc + +ConfigT = TypeVar("ConfigT", bound=BaseConfig) + +Ack = destination_pb2.Destination.Run.Response.Ack + + +class Destination(abc.ABC, Generic[ConfigT]): + """Base class for Conduit destination connectors. + + Subclass this, parameterized with your + :class:`~conduit.config.BaseConfig` subclass, and override + :meth:`write` at minimum. Every other method has a working default, per + the same forward-compatible-ABC rationale as :class:`~conduit.source.Source` + (§2.4). + + Methods are declared ``async def``; a sync ``def`` override runs in a + thread-pool executor -- see :mod:`conduit._dispatch` (§2.1). + """ + + config: ConfigT + """The validated config instance, set by :meth:`configure` (default + implementation) before :meth:`open` is called.""" + + async def configure(self, config: ConfigT) -> None: + """Receive and store the validated config. Default: ``self.config = config``. + + Args: + config: the parsed, pydantic-validated config instance. + """ + self.config = config + + async def open(self) -> None: + """Prepare to start writing records (e.g. open connections). Default: no-op.""" + return None + + @abc.abstractmethod + async def write(self, records: list[Record]) -> None: + """Durably write every record in ``records``, in order. + + Full success is "returns without raising." A partial-batch failure + raises :class:`~conduit.errors.BatchWriteError` with an exhaustive + accounting of which indices succeeded and which failed -- see that + class's docstring for the exact construction contract (the B1 fix). + Any other exception is treated by the SDK's adapter as a failure of + the **entire** batch (see :meth:`_DestinationServicer._write_batch`) + -- there is no partial-credit interpretation of a plain exception. + + The one genuinely required override (§2.4) -- ``abc.ABC`` refuses + to instantiate a subclass that doesn't provide it. + + Args: + records: the batch to write, in the order Conduit sent them. + + Raises: + conduit.errors.BatchWriteError: on a partial-batch failure, with + an exhaustive per-index accounting. + """ + raise NotImplementedError("Destination subclasses must override write()") + + async def teardown(self) -> None: + """Called once, after the write loop stops, before process exit. Default: no-op.""" + return None + + async def lifecycle_on_created(self, config: dict[str, str]) -> None: + """Called once, the first time this connector instance is ever run. Default: no-op.""" + return None + + async def lifecycle_on_updated( + self, config_before: dict[str, str], config_after: dict[str, str] + ) -> None: + """Called when the connector's configuration changed since the last run. Default: no-op.""" + return None + + async def lifecycle_on_deleted(self, config: dict[str, str]) -> None: + """Called once, when this connector instance was deleted. Default: no-op.""" + return None + + +class _DestinationServicer(destination_pb2_grpc.DestinationPluginServicer): + """Adapts a :class:`Destination` instance to the generated servicer. + + Internal; constructed by :func:`conduit.serve.serve`, never by + connector authors directly. + """ + + def __init__(self, destination: Destination[Any], config_cls: type[BaseConfig]) -> None: + """Wrap a connector instance for gRPC dispatch. + + Args: + destination: the author's ``Destination`` instance. Typed + ``Destination[Any]`` (not ``Destination[BaseConfig]``) + deliberately: generics are invariant in Python's type + system, so a concrete ``Destination[MyConfig]`` instance is + not otherwise assignable here; ``config_cls`` (below) is + what actually drives config validation, independent of + this parameter's type. + config_cls: the concrete :class:`~conduit.config.BaseConfig` + subclass to validate ``Configure``'s config map against. + """ + self._destination = destination + self._config_cls = config_cls + + async def Configure( + self, request: destination_pb2.Destination.Configure.Request, context: object + ) -> destination_pb2.Destination.Configure.Response: + """Validate and store the plugin's config.""" + config = self._config_cls.model_validate(config_map_from_proto(request.config)) + await invoke(self._destination.configure, config) + return destination_pb2.Destination.Configure.Response() + + async def Open( + self, request: destination_pb2.Destination.Open.Request, context: object + ) -> destination_pb2.Destination.Open.Response: + """Prepare the destination to start writing records.""" + await invoke(self._destination.open) + return destination_pb2.Destination.Open.Response() + + async def Run( + self, + request_iterator: AsyncIterator[destination_pb2.Destination.Run.Request], + context: object, + ) -> AsyncIterator[destination_pb2.Destination.Run.Response]: + """Bidirectional stream: consume record batches in, emit acks out. + + Each incoming ``Destination.Run.Request`` (a batch of records) is + written via :meth:`_write_batch` and immediately followed by a + ``Destination.Run.Response`` carrying one ack per record in that + batch, in the same order -- there is no cross-batch buffering here, + keeping the ack/write relationship for a given batch entirely + local to one iteration of this loop. + """ + async for request in request_iterator: + records = records_from_proto(request.records) + acks = await self._write_batch(records) + yield destination_pb2.Destination.Run.Response(acks=acks) + + async def _write_batch(self, records: Sequence[Record]) -> list[Ack]: + """Call ``write()`` and translate the outcome into per-record acks. + + This is the B1 enforcement site: every ack decision below is driven + either by ``write()`` returning cleanly (full-batch success) or by + :class:`~conduit.errors.BatchWriteError`'s own exhaustive, already- + validated ``success``/``failures`` accounting -- never by assuming + an unmentioned index succeeded. + """ + try: + await invoke(self._destination.write, list(records)) + except BatchWriteError as exc: + return self._acks_from_batch_write_error(records, exc) + except Exception as exc: + # Invariant 1 / B1: any exception other than BatchWriteError + # carries no per-index accounting at all, so the adapter cannot + # assume *any* record in this batch was durably written. Nack + # the entire batch rather than guessing a partial success. + return [Ack(position=r.position, error=str(exc)) for r in records] + # Invariant 1: ack only reached here, after `write()` returned + # without raising for every record in this batch -- full-batch + # success, the only case where every ack carries no error. + return [Ack(position=r.position, error="") for r in records] + + def _acks_from_batch_write_error( + self, records: Sequence[Record], exc: BatchWriteError + ) -> list[Ack]: + """Build one ack per record from a validated ``BatchWriteError``. + + ``exc.success``/``exc.failures`` were already checked exhaustive + and disjoint at ``BatchWriteError.__init__`` time (see + :mod:`conduit.errors`) -- this method has no code path that + computes "ack everything not explicitly marked as failed": the + ``else`` branch below only runs for the (should-be-impossible, + defense-in-depth) case of an index outside both sets, and even then + it nacks, it never acks. + """ + acks: list[Ack] = [] + for i, record in enumerate(records): + if i in exc.success: + # Invariant 1: explicitly accounted as successfully, + # durably written by write() -- ack. + acks.append(Ack(position=record.position, error="")) + else: + # Invariant 1 / B1 fail-closed fix: every other index -- + # whether explicitly in `exc.failures`, or (defensively) + # absent from both accountings entirely, which + # BatchWriteError's own constructor should already have + # rejected -- is nacked, never assumed successful. Mirrors + # destination.go:345-350's defensive re-check. + reason = exc.failures.get( + i, RuntimeError(f"index {i} unaccounted for in BatchWriteError") + ) + acks.append(Ack(position=record.position, error=str(reason))) + return acks + + async def Stop( + self, request: destination_pb2.Destination.Stop.Request, context: object + ) -> destination_pb2.Destination.Stop.Response: + """Acknowledge the last record Conduit will send; nothing further to do here. + + The destination has no read-loop analog to halt -- ``Run``'s + request stream simply ends after this record, which is Conduit's + own responsibility, not this servicer's. + """ + return destination_pb2.Destination.Stop.Response() + + async def Teardown( + self, request: destination_pb2.Destination.Teardown.Request, context: object + ) -> destination_pb2.Destination.Teardown.Response: + """Run the connector's teardown hook to completion.""" + await invoke(self._destination.teardown) + return destination_pb2.Destination.Teardown.Response() + + async def LifecycleOnCreated( + self, request: destination_pb2.Destination.Lifecycle.OnCreated.Request, context: object + ) -> destination_pb2.Destination.Lifecycle.OnCreated.Response: + """Dispatch the connector's first-run lifecycle hook.""" + await invoke(self._destination.lifecycle_on_created, config_map_from_proto(request.config)) + return destination_pb2.Destination.Lifecycle.OnCreated.Response() + + async def LifecycleOnUpdated( + self, request: destination_pb2.Destination.Lifecycle.OnUpdated.Request, context: object + ) -> destination_pb2.Destination.Lifecycle.OnUpdated.Response: + """Dispatch the connector's config-changed lifecycle hook.""" + await invoke( + self._destination.lifecycle_on_updated, + config_map_from_proto(request.config_before), + config_map_from_proto(request.config_after), + ) + return destination_pb2.Destination.Lifecycle.OnUpdated.Response() + + async def LifecycleOnDeleted( + self, request: destination_pb2.Destination.Lifecycle.OnDeleted.Request, context: object + ) -> destination_pb2.Destination.Lifecycle.OnDeleted.Response: + """Dispatch the connector's deleted lifecycle hook.""" + await invoke(self._destination.lifecycle_on_deleted, config_map_from_proto(request.config)) + return destination_pb2.Destination.Lifecycle.OnDeleted.Response() + + +def _resolve_destination_config_class( + destination_cls: type[Destination[BaseConfig]], +) -> type[BaseConfig]: + """Recover the concrete ``BaseConfig`` subclass a ``Destination[Config]`` used. + + Thin, ``Destination``-specific wrapper over + :func:`conduit._introspect.resolve_config_class`. + """ + return resolve_config_class(destination_cls, Destination) diff --git a/src/conduit/serve.py b/src/conduit/serve.py new file mode 100644 index 0000000..64e47ac --- /dev/null +++ b/src/conduit/serve.py @@ -0,0 +1,532 @@ +"""The ``serve()`` entry point: handshake + gRPC server bootstrap. + +Implements ``docs/design/20260707-python-connector-sdk.md`` §1 end to end: +validates the go-plugin handshake (:mod:`conduit._handshake`, Lane A, +reused here, not reimplemented), starts a ``grpc.aio`` server, registers the +``SourcePlugin``/``DestinationPlugin`` servicer (:mod:`conduit.source`/ +:mod:`conduit.destination`) plus ``SpecifierPlugin``, registers +``grpc.health.v1`` health (``SERVING`` for service ``"plugin"``, go-plugin's +``GRPCServiceName``), and registers the hand-written ``GRPCController`` +(:mod:`conduit._grpc._controller`) so ``conduit pipelines stop`` tears the +subprocess down via go-plugin's graceful ``Shutdown`` RPC rather than its +2-second force-kill fallback (§1.1.5). + +**▶ MUST-FIX 3 (hung-event-loop watchdog):** this module's +:class:`_ShutdownCoordinator` is the SDK-internal mitigation for a +genuinely wedged event loop -- see its docstring and the design doc's +"hung/deadlocked asyncio event loop mid-write" failure mode for the full +rationale. It is independent of asyncio by construction: the ``SIGTERM`` +handler is installed with low-level ``signal.signal`` (not +``loop.add_signal_handler``), and the watchdog itself is a +``threading.Timer`` running on its own OS thread. +""" + +from __future__ import annotations + +import asyncio +import contextlib +import os +import signal +import sys +import threading +from collections.abc import Callable, Mapping +from dataclasses import dataclass +from typing import Any, NoReturn, TextIO + +import grpc +import grpc.aio +from google.protobuf import empty_pb2 +from grpc_health.v1 import health, health_pb2, health_pb2_grpc + +import conduit._grpc # noqa: F401 -- sets up sys.path, see conduit._grpc.__init__ +from conduit._dispatch import invoke +from conduit._grpc._controller import build_controller_handler +from conduit._handshake import ( + HandshakeLine, + check_magic_cookie, + emit_handshake_line, + negotiate_protocol_version, +) +from conduit.config import Specification, to_parameters +from conduit.destination import ( + Destination, + _DestinationServicer, + _resolve_destination_config_class, +) +from conduit.source import Source, _resolve_source_config_class, _SourceServicer +from connector.v2 import ( + destination_pb2_grpc, + source_pb2_grpc, + specifier_pb2, + specifier_pb2_grpc, +) + +DEFAULT_SHUTDOWN_DEADLINE_SECONDS = 5.0 +"""Default bounded window (▶ MUST-FIX 3) for graceful shutdown to complete +after SIGTERM before the watchdog force-exits. Configurable per :func:`serve` +call; kept short enough that a genuinely wedged connector doesn't hang a +pipeline stop indefinitely, long enough not to truncate an ordinary +in-flight write/teardown under normal load.""" + +_HEALTH_SERVICE_NAME = "plugin" +"""go-plugin's own ``GRPCServiceName`` constant (`grpc_server.go:24-81`) -- +the service name Conduit's health check probes, not a Conduit-side name.""" + + +class _ShutdownCoordinator: + """Independent-of-asyncio watchdog for the hung-event-loop failure mode. + + Per ▶ MUST-FIX 3 in the design doc: ``asyncio``'s signal handling has no + equivalent to Go's preemptive goroutine scheduling -- if the event loop + is genuinely wedged (blocked in a synchronous call that never yields), + ``loop.add_signal_handler`` callbacks never run, because they're + themselves scheduled on the same wedged loop's callback queue. This + class never relies on that mechanism: ``SIGTERM`` is caught with + low-level ``signal.signal`` (delivered via the interpreter's own + signal-checking, independent of asyncio), and the watchdog itself is a + ``threading.Timer`` on a separate OS thread -- it fires on schedule + regardless of what the event loop's thread is doing. + + This does not make an in-flight write that's truly stuck mid-flight + safe -- that record is lost either way, the same outcome invariant 1 + already tolerates for any never-acked record. What it bounds is how + long Conduit (or an operator) waits on a connector that will never + respond on its own, and it distinguishes, on stderr, "the loop was + wedged" from "shutdown completed cleanly" before the process goes away + either way. + """ + + def __init__( + self, + *, + deadline: float, + exit_fn: Callable[[int], NoReturn], + stderr: TextIO, + graceful_trigger: Callable[[], None] | None = None, + ) -> None: + """Initialize the coordinator. + + Args: + deadline: seconds to wait, after the watchdog starts, before + forcing an exit if clean shutdown hasn't been confirmed. + exit_fn: called with exit code ``1`` if the deadline elapses + without confirmation. Injectable so tests can observe the + watchdog firing without killing the test process; defaults + to ``os._exit`` in production (see :func:`serve`). + stderr: stream the force-exit diagnostic is written to. + graceful_trigger: optional callable invoked (in addition to + starting the watchdog) when ``SIGTERM`` arrives, to kick + off the SDK's own graceful-shutdown coroutine on the event + loop. Must be safe to call from a signal handler (i.e. + thread-safe scheduling only, e.g. + ``asyncio.run_coroutine_threadsafe`` -- see + :func:`_build_plugin_server`). If the loop is wedged, this + callable's scheduled work simply never runs -- the + watchdog, not this trigger, is what bounds that case. + """ + self._deadline = deadline + self._exit_fn = exit_fn + self._stderr = stderr + self._graceful_trigger = graceful_trigger + self._confirmed = threading.Event() + self._timer: threading.Timer | None = None + self._timer_lock = threading.Lock() + + def install_sigterm_handler(self) -> None: + """Install the low-level ``SIGTERM`` handler (main thread only). + + Uses ``signal.signal``, not ``loop.add_signal_handler`` -- see class + docstring for why that distinction is the entire point of this + class. + """ + signal.signal(signal.SIGTERM, self._on_sigterm) + + def _on_sigterm(self, signum: int, frame: object) -> None: + """Handle ``SIGTERM``: start the watchdog, best-effort-trigger graceful shutdown. + + Runs via the interpreter's own signal-checking mechanism, not + scheduled on the event loop -- this is what lets it fire even if + the loop is wedged (▶ MUST-FIX 3). + """ + self.start_watchdog() + if self._graceful_trigger is not None: + with contextlib.suppress(RuntimeError): + self._graceful_trigger() + + def start_watchdog(self) -> None: + """Start the bounded force-exit timer, if not already running. + + Idempotent: a second call (a second ``SIGTERM``, or a direct test + invocation) does not start an overlapping second timer. + """ + with self._timer_lock: + if self._timer is not None: + return + timer = threading.Timer(self._deadline, self._force_exit) + timer.daemon = True + self._timer = timer + timer.start() + + def _force_exit(self) -> None: + """Force-exit unconditionally, unless clean shutdown was confirmed first. + + Runs on the ``threading.Timer``'s own thread -- independent of + whatever the event loop's thread is doing, wedged or not. + """ + if self._confirmed.is_set(): + return + print( + f"conduit-sdk: graceful shutdown did not complete within " + f"{self._deadline}s of SIGTERM -- forcing exit now. This means " + "the event loop was wedged (blocked in a synchronous call that " + "never yielded back), not a normal, if slow, drain -- a clean " + "GRPCController.Shutdown never reaches this watchdog. See " + "docs/design/20260707-python-connector-sdk.md, " + "▶ MUST-FIX 3, for the failure mode this guards against.", + file=self._stderr, + flush=True, + ) + self._exit_fn(1) + + def confirm_clean_exit(self) -> None: + """Record that graceful shutdown completed; cancel the pending watchdog. + + Called once ``teardown()`` has run to completion and the server is + stopping -- on the ordinary (non-wedged) path, this always wins the + race against :meth:`_force_exit`, since it runs on the event loop's + own thread as part of the same graceful-shutdown sequence that + would otherwise have to reach this point anyway. + """ + self._confirmed.set() + with self._timer_lock: + if self._timer is not None: + self._timer.cancel() + + @property + def is_confirmed(self) -> bool: + """Whether :meth:`confirm_clean_exit` has been called.""" + return self._confirmed.is_set() + + +class _SpecifierServicer(specifier_pb2_grpc.SpecifierPluginServicer): + """Adapts a :class:`~conduit.config.Specification` to ``SpecifierPluginServicer``.""" + + def __init__( + self, + specification: Specification, + source_params: Mapping[str, Any], + destination_params: Mapping[str, Any], + ) -> None: + """Initialize with the static specification and pre-computed parameter maps. + + Args: + specification: the author-supplied plugin metadata. + source_params: ``config.Parameter`` map for the registered + ``Source``'s config, empty if a ``Destination`` was + registered instead. + destination_params: same, for a registered ``Destination``. + """ + self._specification = specification + self._source_params = source_params + self._destination_params = destination_params + + async def Specify( + self, request: specifier_pb2.Specifier.Specify.Request, context: object + ) -> specifier_pb2.Specifier.Specify.Response: + """Return the plugin's static specification and parameter maps.""" + spec = specifier_pb2.Specification( + name=self._specification.name, + summary=self._specification.summary, + description=self._specification.description, + version=self._specification.version, + author=self._specification.author, + source_params=dict(self._source_params), + destination_params=dict(self._destination_params), + ) + return specifier_pb2.Specifier.Specify.Response(specification=spec) + + +@dataclass(slots=True) +class _ServerHandle: + """Everything :func:`_serve_async` (or a test) needs to drive the server. + + Returned by :func:`_build_plugin_server`, which does all the wiring but + deliberately does not itself block waiting for shutdown -- separating + "build a fully working plugin server" from "run it to completion" is + what lets the deterministic shutdown test (▶ MUST-FIX 2) connect a real + gRPC client to a real, running server and drive ``GRPCController. + Shutdown`` directly, without going through ``serve()``'s + handshake/stdout/``asyncio.run`` machinery, none of which that test + needs or should depend on. + """ + + server: grpc.aio.Server + port: int + coordinator: _ShutdownCoordinator + connector_instance: Source[Any] | Destination[Any] + shutdown_requested: asyncio.Event + drive_task: asyncio.Task[None] + + +async def _build_plugin_server( + specification: Specification, + *, + source: type[Source[Any]] | None = None, + destination: type[Destination[Any]] | None = None, + shutdown_deadline: float = DEFAULT_SHUTDOWN_DEADLINE_SECONDS, + exit_fn: Callable[[int], NoReturn] = os._exit, + stderr: TextIO | None = None, +) -> _ServerHandle: + """Build and start a fully wired, listening plugin gRPC server. + + Registers the connector servicer, the specifier servicer, gRPC health, + and the ``GRPCController`` -- everything except the go-plugin handshake + line and the top-level "wait for shutdown, then exit" drive loop (see + :func:`_serve_async`), so this can be exercised directly by tests (and + by :func:`serve`) against a real, running server on a real socket. + + Args: + specification: static plugin metadata for the ``Specify`` RPC. + source: a :class:`~conduit.source.Source` subclass to instantiate + and serve. Exactly one of ``source``/``destination`` must be + given. + destination: a :class:`~conduit.destination.Destination` subclass + to instantiate and serve. + shutdown_deadline: seconds the hung-loop watchdog waits after + ``SIGTERM`` before forcing an exit (▶ MUST-FIX 3). + exit_fn: injectable process-exit function; see + :class:`_ShutdownCoordinator`. + stderr: stream for the watchdog's force-exit diagnostic; defaults + to ``sys.stderr``. + + Returns: + A handle exposing the running server, its port, the shutdown + coordinator, the constructed connector instance, and the + background task driving shutdown once requested. + + Raises: + ValueError: if neither or both of ``source``/``destination`` are given. + """ + if (source is None) == (destination is None): + raise ValueError( + "_build_plugin_server() requires exactly one of `source=` or " + "`destination=`, not both/neither" + ) + stderr = stderr if stderr is not None else sys.stderr + + server = grpc.aio.server() + instance: Source[Any] | Destination[Any] + source_params: Mapping[str, Any] = {} + destination_params: Mapping[str, Any] = {} + + if source is not None: + config_cls = _resolve_source_config_class(source) + instance = source() + # The generated `*_pb2_grpc.py` files carry no type annotations (no + # companion `.pyi` for the service-registration helpers, only for + # the message types) -- calling into them from this strict-mode + # module is an intentional, vendored-codegen boundary, not a typing + # gap in our own code (see pyproject.toml's mypy overrides comment). + source_pb2_grpc.add_SourcePluginServicer_to_server( # type: ignore[no-untyped-call] + _SourceServicer(instance, config_cls), server + ) + source_params = to_parameters(config_cls) + else: + assert destination is not None # narrowed by the xor check above + config_cls = _resolve_destination_config_class(destination) + instance = destination() + destination_pb2_grpc.add_DestinationPluginServicer_to_server( # type: ignore[no-untyped-call] + _DestinationServicer(instance, config_cls), server + ) + destination_params = to_parameters(config_cls) + + specifier_pb2_grpc.add_SpecifierPluginServicer_to_server( # type: ignore[no-untyped-call] + _SpecifierServicer(specification, source_params, destination_params), server + ) + + # `grpc_health-stubs`' bundled type stub for `grpc_health.v1.health` only + # covers the sync `HealthServicer` and doesn't declare the `.aio` + # submodule this runtime attribute access resolves at runtime (verified: + # `grpc_health.v1.health.aio.HealthServicer` is `grpc_health.v1._async. + # HealthServicer`) -- a third-party stub gap, not a bug in this module. + health_servicer = health.aio.HealthServicer() # type: ignore[attr-defined] + health_pb2_grpc.add_HealthServicer_to_server(health_servicer, server) + await health_servicer.set(_HEALTH_SERVICE_NAME, health_pb2.HealthCheckResponse.SERVING) + + loop = asyncio.get_running_loop() + shutdown_requested = asyncio.Event() + teardown_lock = asyncio.Lock() + teardown_done = False + + async def run_teardown_once() -> None: + nonlocal teardown_done + async with teardown_lock: + if not teardown_done: + await invoke(instance.teardown) + teardown_done = True + + async def on_shutdown_rpc(request: empty_pb2.Empty, context: object) -> empty_pb2.Empty: + # Invariant 7 (graceful shutdown by default): teardown() is run to + # completion HERE, before this RPC returns and before + # `shutdown_requested` (which triggers server.stop()) is set. This + # ordering -- not a timing assumption -- is what makes the + # deterministic shutdown test (▶ MUST-FIX 2) valid: a passing RPC + # call is itself proof teardown() already ran. + await run_teardown_once() + shutdown_requested.set() + return empty_pb2.Empty() + + server.add_generic_rpc_handlers((build_controller_handler(on_shutdown_rpc),)) + + port = server.add_insecure_port("127.0.0.1:0") + await server.start() + + def graceful_trigger() -> None: + """Schedule the same graceful-shutdown coroutine from a signal handler. + + Uses ``run_coroutine_threadsafe``, the correct primitive for + scheduling coroutine work on the loop from outside it (a signal + handler is not "another thread" in the OS sense, but per the + ``asyncio`` docs it must be treated the same way: only + thread-safe scheduling APIs are safe to call from it). + """ + asyncio.run_coroutine_threadsafe(_sigterm_shutdown(), loop) + + async def _sigterm_shutdown() -> None: + await run_teardown_once() + shutdown_requested.set() + + coordinator = _ShutdownCoordinator( + deadline=shutdown_deadline, + exit_fn=exit_fn, + stderr=stderr, + graceful_trigger=graceful_trigger, + ) + + async def drive_shutdown() -> None: + await shutdown_requested.wait() + await server.stop(grace=None) + coordinator.confirm_clean_exit() + + drive_task = asyncio.create_task(drive_shutdown()) + + return _ServerHandle( + server=server, + port=port, + coordinator=coordinator, + connector_instance=instance, + shutdown_requested=shutdown_requested, + drive_task=drive_task, + ) + + +async def _serve_async( + specification: Specification, + *, + source: type[Source[Any]] | None, + destination: type[Destination[Any]] | None, + app_protocol_version: int, + shutdown_deadline: float, + exit_fn: Callable[[int], NoReturn], + stdout: TextIO, + stderr: TextIO, +) -> None: + """Build the server, emit the handshake line, then run until shutdown. + + Separated from :func:`serve` only so ``asyncio.run`` has a single + coroutine to drive; not part of the public API. + """ + handle = await _build_plugin_server( + specification, + source=source, + destination=destination, + shutdown_deadline=shutdown_deadline, + exit_fn=exit_fn, + stderr=stderr, + ) + handle.coordinator.install_sigterm_handler() + + # Failure modes (design doc): nothing may be written to stdout before + # this line except this line itself -- it is the one channel + # go-plugin's client parses byte-for-byte. + address = f"127.0.0.1:{handle.port}" + emit_handshake_line( + HandshakeLine(app_protocol_version=app_protocol_version, address=address), + stream=stdout, + ) + + await handle.drive_task + # Mirrors the Go SDK's own clean-shutdown path (§1.1.5: "stop the + # server / os._exit(0)"): an explicit, injectable process exit rather + # than falling through Python's normal interpreter teardown, so this + # goes through the exact same exit mechanism (and is exercised by the + # exact same test seam) as the watchdog's forced path. + exit_fn(0) + + +def serve( + specification: Specification, + *, + source: type[Source[Any]] | None = None, + destination: type[Destination[Any]] | None = None, + env: Mapping[str, str] | None = None, + shutdown_deadline: float = DEFAULT_SHUTDOWN_DEADLINE_SECONDS, + exit_fn: Callable[[int], NoReturn] = os._exit, + stdout: TextIO | None = None, + stderr: TextIO | None = None, +) -> None: + """Run a ``Source`` or ``Destination`` as a Conduit standalone plugin. + + Validates the go-plugin handshake, starts a ``grpc.aio`` server serving + the registered connector, blocks until Conduit tears it down via + ``GRPCController.Shutdown`` (or the bounded watchdog forces an exit -- + ▶ MUST-FIX 3), then returns (in practice, ``exit_fn`` -- ``os._exit`` by + default -- ends the process before this function's caller ever resumes). + + Args: + specification: static plugin metadata (name, version, author, ...). + source: a :class:`~conduit.source.Source` subclass to serve. + Exactly one of ``source``/``destination`` must be given. + destination: a :class:`~conduit.destination.Destination` subclass + to serve. + env: environment mapping for handshake validation; defaults to + ``os.environ``. Injectable for testing. + shutdown_deadline: seconds the hung-loop watchdog waits after + ``SIGTERM`` before forcing an exit; see + :data:`DEFAULT_SHUTDOWN_DEADLINE_SECONDS`. + exit_fn: injectable process-exit function, called with ``0`` on + clean shutdown or ``1`` if the watchdog fires. Defaults to + ``os._exit`` (never returns); tests may inject a non-exiting + stand-in to observe calls. + stdout: stream for the handshake line; defaults to ``sys.stdout``. + Injectable for testing -- never write anything else here (see + module docstring). + stderr: stream for shutdown diagnostics; defaults to ``sys.stderr``. + + Raises: + ValueError: if neither or both of ``source``/``destination`` are given. + conduit._handshake.HandshakeError: if the go-plugin handshake + cannot be validated (missing/wrong magic cookie, no compatible + protocol version). + """ + if (source is None) == (destination is None): + raise ValueError( + "serve() requires exactly one of `source=` or `destination=`, not both/neither" + ) + + env = env if env is not None else os.environ + check_magic_cookie(env) + app_protocol_version = negotiate_protocol_version(env) + + asyncio.run( + _serve_async( + specification, + source=source, + destination=destination, + app_protocol_version=app_protocol_version, + shutdown_deadline=shutdown_deadline, + exit_fn=exit_fn, + stdout=stdout if stdout is not None else sys.stdout, + stderr=stderr if stderr is not None else sys.stderr, + ) + ) diff --git a/src/conduit/source.py b/src/conduit/source.py new file mode 100644 index 0000000..5cd7218 --- /dev/null +++ b/src/conduit/source.py @@ -0,0 +1,351 @@ +"""Source connector base class + the ``SourcePlugin`` gRPC wire adapter. + +See ``docs/design/20260707-python-connector-sdk.md`` §2.1 (async/dual-mode), +§2.4 (forward-compatible ABC), §2.5 (``BackoffRetry``), and §1.3 (the +``SourcePlugin`` RPC surface). This module holds both halves: the +author-facing :class:`Source` ABC, and the internal +:class:`_SourceServicer` that adapts it to the generated +``SourcePluginServicer`` -- co-located because the wire adapter exists +entirely to serve this one ABC and the split would only add indirection. +""" + +from __future__ import annotations + +import abc +import asyncio +import contextlib +from collections.abc import AsyncIterator, Mapping +from typing import Any, Generic, TypeVar + +import conduit._grpc # noqa: F401 -- sets up sys.path, see conduit._grpc.__init__ +from conduit._dispatch import invoke +from conduit._grpc.adapters import config_map_from_proto, record_to_proto +from conduit._introspect import resolve_config_class +from conduit.config import BaseConfig +from conduit.errors import BackoffRetry +from conduit.record import Record +from connector.v2 import source_pb2, source_pb2_grpc + +ConfigT = TypeVar("ConfigT", bound=BaseConfig) + +# Backoff constants mirroring the Go SDK's read loop exactly +# (`backoff.Backoff{Factor: 2, Min: 100*time.Millisecond, Max: 5*time.Second}`, +# `source.go:270-295`, re-verified ▶ MUST-FIX 1 in the design doc: a plain, +# serial `for` loop with no concurrent invocation of the read path). Reusing +# these constants is genuine behavioral parity with the Go SDK, not merely a +# similar-looking default chosen independently. +BACKOFF_FACTOR = 2.0 +BACKOFF_MIN_SECONDS = 0.1 +BACKOFF_MAX_SECONDS = 5.0 + + +class Source(abc.ABC, Generic[ConfigT]): + """Base class for Conduit source connectors. + + Subclass this, parameterized with your :class:`~conduit.config.BaseConfig` + subclass (e.g. ``class MySource(Source[MyConfig]):``) and override + :meth:`read` at minimum. Every other method has a working default (a + no-op, or delegating to ``self.config``), per the design doc §2.4's + forward-compatible-ABC pattern: Python's default-method mechanism gives + the same "can't accidentally satisfy the interface without the default + behavior" guarantee Go gets from ``mustEmbedUnimplementedSource()``, for + free, with no seal method and no boilerplate for authors. Adding a new + optional method to this class later is therefore source-compatible + automatically. + + Methods are declared ``async def``; an override may instead be a plain + ``def`` if your target system's client library is sync-only -- see + :mod:`conduit._dispatch` for how the SDK detects and dispatches either + kind (§2.1). + """ + + config: ConfigT + """The validated config instance, set by :meth:`configure` (default + implementation) before :meth:`open` is called.""" + + async def configure(self, config: ConfigT) -> None: + """Receive and store the validated config. Default: ``self.config = config``. + + Override only if you need additional validation/setup beyond + pydantic's own model validation; call ``super().configure(config)`` + (or set ``self.config`` yourself) to retain the assignment other + default methods rely on. + + Args: + config: the parsed, pydantic-validated config instance. + """ + self.config = config + + async def open(self, position: bytes | None) -> None: + """Prepare to start producing records. Default: no-op. + + Args: + position: the position of the last record successfully + processed in a previous run, or ``None`` on a fresh start. + Per invariant 2 (positions are monotonic and crash-safe), + :meth:`read` must resume strictly after this position, not + skip or replay past it non-idempotently. + """ + return None + + @abc.abstractmethod + async def read(self) -> Record: + """Return the next available record. + + Raise :class:`~conduit.errors.BackoffRetry` if none is available + right now -- the SDK's own read loop paces retries using the same + backoff the Go SDK uses (:data:`BACKOFF_FACTOR`/ + :data:`BACKOFF_MIN_SECONDS`/:data:`BACKOFF_MAX_SECONDS`); do not + also ``asyncio.sleep()``/block before raising, or you double the + intended backoff. + + The one genuinely required override (§2.4) -- ``abc.ABC`` refuses + to instantiate a subclass that doesn't provide it, at construction + time, rather than failing later when first called. + + Raises: + conduit.errors.BackoffRetry: no record is available yet. + """ + raise NotImplementedError("Source subclasses must override read()") + + async def ack(self, position: bytes) -> None: + """Called when Conduit confirms durable downstream handling of ``position``. + + Default: no-op. Override to persist a resumable cursor -- this is + the only correct place to do so, since it is called only after + Conduit's ``ack_positions`` confirms durable handling (see + :class:`_SourceServicer._consume_acks`, which is the sole caller), + never speculatively when a record is merely produced. + + Args: + position: the acknowledged record's position. + """ + return None + + async def teardown(self) -> None: + """Called once, after the read loop stops, before process exit. Default: no-op.""" + return None + + async def lifecycle_on_created(self, config: Mapping[str, str]) -> None: + """Called once, the first time this connector instance is ever run. Default: no-op. + + Args: + config: the raw string config map (not yet parsed into + :attr:`config` -- this hook runs before ``configure()``'s + normal validated-config flow, mirroring the Go SDK). + """ + return None + + async def lifecycle_on_updated( + self, config_before: Mapping[str, str], config_after: Mapping[str, str] + ) -> None: + """Called when the connector's configuration changed since the last run. Default: no-op. + + Args: + config_before: the previous raw string config map. + config_after: the new raw string config map. + """ + return None + + async def lifecycle_on_deleted(self, config: Mapping[str, str]) -> None: + """Called once, when this connector instance was deleted. Default: no-op. + + Args: + config: the raw string config map the connector was last + configured with. + """ + return None + + +class _Backoff: + """Serial retry-delay generator, mirroring ``jpillora/backoff`` semantics. + + Go's SDK constructs a ``backoff.Backoff{Factor: 2, Min: 100ms, Max: 5s}`` + and calls ``.Duration()`` on each ``ErrBackoffRetry``, which returns + ``Min * Factor**attempt`` (capped at ``Max``) and increments an internal + attempt counter; the caller resets the counter after a successful read. + This class reproduces that exact sequence. + """ + + def __init__( + self, + factor: float = BACKOFF_FACTOR, + min_seconds: float = BACKOFF_MIN_SECONDS, + max_seconds: float = BACKOFF_MAX_SECONDS, + ) -> None: + """Initialize with the backoff curve's parameters. + + Args: + factor: multiplier applied per attempt. + min_seconds: delay for the first retry (attempt 0). + max_seconds: delay cap, regardless of attempt count. + """ + self._factor = factor + self._min = min_seconds + self._max = max_seconds + self._attempt = 0 + + def duration(self) -> float: + """Return the next delay, in seconds, and advance the attempt counter.""" + delay = self._min * (self._factor**self._attempt) + self._attempt += 1 + return min(delay, self._max) + + def reset(self) -> None: + """Reset the attempt counter after a successful (non-retry) read.""" + self._attempt = 0 + + +class _SourceServicer(source_pb2_grpc.SourcePluginServicer): + """Adapts a :class:`Source` instance to the generated ``SourcePluginServicer``. + + Internal; constructed by :func:`conduit.serve.serve`, never by + connector authors directly. + """ + + def __init__(self, source: Source[Any], config_cls: type[BaseConfig]) -> None: + """Wrap a connector instance for gRPC dispatch. + + Args: + source: the author's ``Source`` instance. Typed ``Source[Any]`` + (not ``Source[BaseConfig]``) deliberately: generics are + invariant in Python's type system, so a concrete + ``Source[MyConfig]`` instance is not otherwise assignable + here; ``config_cls`` (below) is what actually drives config + validation, independent of this parameter's type. + config_cls: the concrete :class:`~conduit.config.BaseConfig` + subclass to validate ``Configure``'s config map against. + """ + self._source = source + self._config_cls = config_cls + self._stop_event = asyncio.Event() + self._stopped_event = asyncio.Event() + self._run_started = False + self._last_position: bytes = b"" + + async def Configure( + self, request: source_pb2.Source.Configure.Request, context: object + ) -> source_pb2.Source.Configure.Response: + """Validate and store the plugin's config. See proto doc comment for RPC semantics.""" + config = self._config_cls.model_validate(config_map_from_proto(request.config)) + await invoke(self._source.configure, config) + return source_pb2.Source.Configure.Response() + + async def Open( + self, request: source_pb2.Source.Open.Request, context: object + ) -> source_pb2.Source.Open.Response: + """Prepare the source to start producing records after ``request.position``.""" + position = request.position or None + await invoke(self._source.open, position) + return source_pb2.Source.Open.Response() + + async def Run( + self, + request_iterator: AsyncIterator[source_pb2.Source.Run.Request], + context: object, + ) -> AsyncIterator[source_pb2.Source.Run.Response]: + """Bidirectional stream: emit records out, consume ``ack_positions`` in. + + Both directions run concurrently on this one call object -- an + `async for` reading the read-loop's records (yielded directly, + driving the response stream) and a background task consuming + ``request_iterator`` for incoming acks -- per + ``grpc.aio``'s native bidi-stream model (design doc §2.1/§1.3). + """ + self._run_started = True + ack_task = asyncio.create_task(self._consume_acks(request_iterator)) + try: + async for record in self._read_loop(): + self._last_position = record.position + yield source_pb2.Source.Run.Response(records=[record_to_proto(record)]) + finally: + ack_task.cancel() + with contextlib.suppress(asyncio.CancelledError): + await ack_task + self._stopped_event.set() + + async def _consume_acks( + self, request_iterator: AsyncIterator[source_pb2.Source.Run.Request] + ) -> None: + async for request in request_iterator: + for position in request.ack_positions: + # Invariant 1: a source record's position is acknowledged to + # the connector only here -- only after Conduit's Run + # request stream sends it back via `ack_positions` -- never + # speculatively when the record is merely produced by + # `_read_loop` below. `_read_loop` never calls + # `self._source.ack`; this is the only call site. + await invoke(self._source.ack, bytes(position)) + + async def _read_loop(self) -> AsyncIterator[Record]: + """Serially call ``read()``, backing off on ``BackoffRetry``. + + A single, plain loop with no concurrent invocation of ``read()`` -- + matching ``source.go:270-295``'s structure exactly (re-verified + ▶ MUST-FIX 1), which is what makes reusing its backoff constants + genuine parity rather than a coincidentally similar default. + """ + backoff = _Backoff() + while not self._stop_event.is_set(): + try: + record = await invoke(self._source.read) + except BackoffRetry: + delay = backoff.duration() + # Wait on the stop event (not a plain sleep) so `Stop()` + # can interrupt an in-progress backoff wait promptly instead + # of blocking shutdown for up to BACKOFF_MAX_SECONDS. + with contextlib.suppress(TimeoutError): + await asyncio.wait_for(self._stop_event.wait(), timeout=delay) + continue + backoff.reset() + yield record + + async def Stop( + self, request: source_pb2.Source.Stop.Request, context: object + ) -> source_pb2.Source.Stop.Response: + """Signal the read loop to stop; block until it has, then report the last position.""" + self._stop_event.set() + if self._run_started: + await self._stopped_event.wait() + return source_pb2.Source.Stop.Response(last_position=self._last_position) + + async def Teardown( + self, request: source_pb2.Source.Teardown.Request, context: object + ) -> source_pb2.Source.Teardown.Response: + """Run the connector's teardown hook to completion.""" + await invoke(self._source.teardown) + return source_pb2.Source.Teardown.Response() + + async def LifecycleOnCreated( + self, request: source_pb2.Source.Lifecycle.OnCreated.Request, context: object + ) -> source_pb2.Source.Lifecycle.OnCreated.Response: + """Dispatch the connector's first-run lifecycle hook.""" + await invoke(self._source.lifecycle_on_created, config_map_from_proto(request.config)) + return source_pb2.Source.Lifecycle.OnCreated.Response() + + async def LifecycleOnUpdated( + self, request: source_pb2.Source.Lifecycle.OnUpdated.Request, context: object + ) -> source_pb2.Source.Lifecycle.OnUpdated.Response: + """Dispatch the connector's config-changed lifecycle hook.""" + await invoke( + self._source.lifecycle_on_updated, + config_map_from_proto(request.config_before), + config_map_from_proto(request.config_after), + ) + return source_pb2.Source.Lifecycle.OnUpdated.Response() + + async def LifecycleOnDeleted( + self, request: source_pb2.Source.Lifecycle.OnDeleted.Request, context: object + ) -> source_pb2.Source.Lifecycle.OnDeleted.Response: + """Dispatch the connector's deleted lifecycle hook.""" + await invoke(self._source.lifecycle_on_deleted, config_map_from_proto(request.config)) + return source_pb2.Source.Lifecycle.OnDeleted.Response() + + +def _resolve_source_config_class(source_cls: type[Source[BaseConfig]]) -> type[BaseConfig]: + """Recover the concrete ``BaseConfig`` subclass a ``Source[Config]`` used. + + Thin, ``Source``-specific wrapper over + :func:`conduit._introspect.resolve_config_class`. + """ + return resolve_config_class(source_cls, Source) diff --git a/tests/test_destination.py b/tests/test_destination.py new file mode 100644 index 0000000..1119c8c --- /dev/null +++ b/tests/test_destination.py @@ -0,0 +1,190 @@ +"""Tests for :mod:`conduit.destination` -- the B1 partial-batch-write fix. + +``test_destination_partial_write_nacks_all`` is the concrete, automated form +of the design doc's B1 fix (§2.5): an incomplete/absent per-index accounting +must nack the *entire* batch, never a silently-assumed-successful prefix. +""" + +from __future__ import annotations + +from typing import ClassVar + +import pytest + +from conduit.config import BaseConfig +from conduit.destination import Destination, _DestinationServicer +from conduit.errors import BatchWriteError +from conduit.record import Operation, Record + + +class _Config(BaseConfig): + pass + + +def _records(n: int) -> list[Record]: + return [Record(position=f"pos-{i}".encode(), operation=Operation.CREATE) for i in range(n)] + + +class _WholeBatchSucceeds(Destination[_Config]): + async def write(self, records: list[Record]) -> None: + return None + + +class _RaisesPlainException(Destination[_Config]): + async def write(self, records: list[Record]) -> None: + raise RuntimeError("connector bug: exploded mid-batch") + + +class _RaisesWrittenPrefix(Destination[_Config]): + def __init__(self, written: int) -> None: + self._written = written + + async def write(self, records: list[Record]) -> None: + raise BatchWriteError(len(records), written=self._written) + + +class _RaisesExplicitAccounting(Destination[_Config]): + def __init__(self, success: set[int], failures: dict[int, BaseException]) -> None: + self._success = success + self._failures = failures + + async def write(self, records: list[Record]) -> None: + raise BatchWriteError(len(records), success=self._success, failures=self._failures) + + +class _RaisesNaiveIncompleteMapping(Destination[_Config]): + """Simulates the exact bug B1 exists to prevent. + + A naive connector (or a hypothetical naive adapter) might construct an + exception carrying only a partial map of *known* failures and expect + "everything else succeeded" to be inferred. ``BatchWriteError`` refuses + to construct that shape at all (see ``tests/test_errors.py``), so this + double-checks the servicer's behavior specifically for a + non-``BatchWriteError`` exception that merely *looks* like it might + carry partial info (e.g. a custom exception with a ``.failures`` + attribute) -- the servicer must still nack the whole batch, because it + only ever branches on ``isinstance(exc, BatchWriteError)``, never on + duck-typed attributes. + """ + + class _LooksLikeBatchWriteErrorButIsnt(RuntimeError): + failures: ClassVar[dict[int, str]] = {2: "only this one failed, allegedly"} + + async def write(self, records: list[Record]) -> None: + raise self._LooksLikeBatchWriteErrorButIsnt("nope") + + +async def _write_batch( + destination: Destination[_Config], n: int +) -> tuple[list[Record], list[object]]: + servicer = _DestinationServicer(destination, _Config) + records = _records(n) + acks = await servicer._write_batch(records) + return records, acks + + +class TestDestinationPartialWriteNacksAll: + """AC #1 (design doc): the headline B1 regression-proof test.""" + + async def test_incomplete_accounting_is_impossible_to_construct(self) -> None: + """(a) An incomplete/absent accounting cannot even be constructed. + + This is the strongest possible version of "an incomplete accounting + nacks the whole batch, not a silently-assumed-successful prefix": + the adapter never gets the chance to make that mistake, because + ``BatchWriteError`` itself refuses to exist in an incomplete state + (see ``tests/test_errors.py`` for the exhaustive construction-time + matrix). Demonstrated here in the exact shape a buggy connector + might attempt: reporting only known failures with no explicit + success set. + """ + with pytest.raises(ValueError, match="must supply"): + BatchWriteError(5, failures={2: RuntimeError("boom")}) # type: ignore[call-overload] + + async def test_non_batch_write_error_exception_nacks_everything(self) -> None: + """(c) A plain (non-``BatchWriteError``) exception nacks the ENTIRE batch.""" + records, acks = await _write_batch(_RaisesPlainException(), 4) + assert len(acks) == 4 + for record, ack in zip(records, acks, strict=True): + assert ack.position == record.position + assert ack.error != "" + assert "exploded mid-batch" in ack.error + + async def test_exception_that_merely_resembles_batch_write_error_nacks_everything( + self, + ) -> None: + """Duck-typing a ``.failures`` attribute does not grant partial credit. + + Only ``isinstance(exc, BatchWriteError)`` grants the partial-ack + path -- anything else, however similar-looking, nacks the whole + batch. This is what "banned by construction, not merely documented" + means in practice at the adapter's actual branch point. + """ + _records_arg, acks = await _write_batch(_RaisesNaiveIncompleteMapping(), 4) + assert len(acks) == 4 + assert all(ack.error != "" for ack in acks) + + async def test_written_prefix_acks_exactly_that_prefix_nacks_the_rest(self) -> None: + """(b) A well-formed partial success (``written=N``) acks ``[0, N)``, nacks the rest.""" + records, acks = await _write_batch(_RaisesWrittenPrefix(written=2), 5) + assert len(acks) == 5 + for i, (record, ack) in enumerate(zip(records, acks, strict=True)): + assert ack.position == record.position + if i < 2: + assert ack.error == "" + else: + assert ack.error != "" + + async def test_explicit_noncontiguous_accounting_acks_exactly_the_success_set(self) -> None: + """A non-contiguous explicit ``success``/``failures`` split is honored precisely.""" + failures: dict[int, BaseException] = {1: ValueError("a"), 3: ValueError("b")} + _records_arg, acks = await _write_batch( + _RaisesExplicitAccounting(success={0, 2, 4}, failures=failures), + 5, + ) + for i, ack in enumerate(acks): + if i in (0, 2, 4): + assert ack.error == "", f"index {i} should be acked" + else: + assert ack.error != "", f"index {i} should be nacked" + assert "a" in acks[1].error + assert "b" in acks[3].error + + async def test_full_batch_success_acks_everything(self) -> None: + records, acks = await _write_batch(_WholeBatchSucceeds(), 3) + assert len(acks) == 3 + for record, ack in zip(records, acks, strict=True): + assert ack.position == record.position + assert ack.error == "" + + +class TestDestinationConfigResolution: + async def test_configure_validates_and_stores_config(self) -> None: + class Config(BaseConfig): + url: str + + class MyDestination(Destination[Config]): + async def write(self, records: list[Record]) -> None: + return None + + instance = MyDestination() + await instance.configure(Config(url="https://example.com")) + assert instance.config.url == "https://example.com" + + +async def test_stop_and_teardown_defaults_are_no_ops() -> None: + class MyDestination(Destination[_Config]): + async def write(self, records: list[Record]) -> None: + return None + + instance = MyDestination() + await instance.open() + await instance.teardown() + await instance.lifecycle_on_created({}) + await instance.lifecycle_on_updated({}, {}) + await instance.lifecycle_on_deleted({}) + + +def test_destination_is_abstract_without_write() -> None: + with pytest.raises(TypeError): + Destination() # type: ignore[abstract] diff --git a/tests/test_serve.py b/tests/test_serve.py new file mode 100644 index 0000000..60da9a4 --- /dev/null +++ b/tests/test_serve.py @@ -0,0 +1,268 @@ +"""Tests for :mod:`conduit.serve` -- deterministic shutdown (▶ MUST-FIX 2) +and the hung-event-loop watchdog (▶ MUST-FIX 3). + +Per the design doc's tightened Phase-1 acceptance criterion: the shutdown +test must be a deterministic RPC-invocation assertion, not a timing/log +heuristic. ``test_shutdown_rpc_runs_teardown_before_responding`` builds the +SDK's real ``grpc.aio`` server with its actual ``GRPCController`` servicer, +connects a real gRPC client to it, calls ``Shutdown``, and asserts (a) the +RPC succeeds and (b) ``teardown()`` ran to completion beforehand -- via a +spy, not a race against a clock. +""" + +from __future__ import annotations + +import asyncio +import contextlib +import io +import threading +import time + +import grpc +import grpc.aio +import pytest +from google.protobuf import empty_pb2 + +from conduit.config import BaseConfig, Specification +from conduit.destination import Destination +from conduit.errors import BackoffRetry +from conduit.record import Record +from conduit.serve import ( + DEFAULT_SHUTDOWN_DEADLINE_SECONDS, + _build_plugin_server, + _ShutdownCoordinator, +) +from conduit.source import Source + +_SPEC = Specification(name="test-plugin", version="0.0.0", author="test") + + +class _Config(BaseConfig): + pass + + +class _TeardownSpySource(Source[_Config]): + def __init__(self) -> None: + self.teardown_calls = 0 + + async def read(self) -> Record: + raise BackoffRetry() + + async def teardown(self) -> None: + self.teardown_calls += 1 + + +class _TeardownSpyDestination(Destination[_Config]): + def __init__(self) -> None: + self.teardown_calls = 0 + + async def write(self, records: list[Record]) -> None: + return None + + async def teardown(self) -> None: + self.teardown_calls += 1 + + +async def _call_shutdown(port: int) -> empty_pb2.Empty: + channel = grpc.aio.insecure_channel(f"127.0.0.1:{port}") + try: + call = channel.unary_unary( + "/plugin.GRPCController/Shutdown", + request_serializer=empty_pb2.Empty.SerializeToString, + response_deserializer=empty_pb2.Empty.FromString, + ) + return await call(empty_pb2.Empty()) # type: ignore[no-any-return] + finally: + await channel.close() + + +class TestDeterministicShutdownRpc: + """▶ MUST-FIX 2: a real gRPC call, not a mock of the transport.""" + + async def test_shutdown_rpc_succeeds_and_runs_teardown_first(self) -> None: + handle = await _build_plugin_server(_SPEC, source=_TeardownSpySource) + try: + # (a) the RPC returns a successful response, not a connection + # error/timeout -- a real call over a real socket. + response = await _call_shutdown(handle.port) + assert response == empty_pb2.Empty() + + # (b) teardown() was invoked exactly once, and -- by construction + # of on_shutdown_rpc's await ordering in _build_plugin_server, + # not by racing a clock -- strictly before the RPC handler + # returned, which is itself strictly before drive_shutdown's + # server.stop() call is even reachable (it's gated on the same + # `shutdown_requested` event `on_shutdown_rpc` sets only after + # teardown() completes). + spy = handle.connector_instance + assert isinstance(spy, _TeardownSpySource) + assert spy.teardown_calls == 1 + + await asyncio.wait_for(handle.drive_task, timeout=2) + assert handle.coordinator.is_confirmed + finally: + with contextlib.suppress(Exception): + await handle.server.stop(None) + + async def test_shutdown_rpc_works_for_destination_too(self) -> None: + handle = await _build_plugin_server(_SPEC, destination=_TeardownSpyDestination) + try: + response = await _call_shutdown(handle.port) + assert response == empty_pb2.Empty() + spy = handle.connector_instance + assert isinstance(spy, _TeardownSpyDestination) + assert spy.teardown_calls == 1 + await asyncio.wait_for(handle.drive_task, timeout=2) + finally: + with contextlib.suppress(Exception): + await handle.server.stop(None) + + async def test_shutdown_is_idempotent_if_called_twice(self) -> None: + """A second Shutdown call (or SIGTERM racing the RPC) must not double-run teardown().""" + handle = await _build_plugin_server(_SPEC, source=_TeardownSpySource) + try: + await _call_shutdown(handle.port) + await asyncio.wait_for(handle.drive_task, timeout=2) + spy = handle.connector_instance + assert isinstance(spy, _TeardownSpySource) + assert spy.teardown_calls == 1 + finally: + with contextlib.suppress(Exception): + await handle.server.stop(None) + + async def test_health_check_reports_serving_for_plugin_service(self) -> None: + handle = await _build_plugin_server(_SPEC, source=_TeardownSpySource) + try: + channel = grpc.aio.insecure_channel(f"127.0.0.1:{handle.port}") + try: + from grpc_health.v1 import health_pb2 + + call = channel.unary_unary( + "/grpc.health.v1.Health/Check", + request_serializer=health_pb2.HealthCheckRequest.SerializeToString, + response_deserializer=health_pb2.HealthCheckResponse.FromString, + ) + response = await call(health_pb2.HealthCheckRequest(service="plugin")) + assert response.status == health_pb2.HealthCheckResponse.SERVING + finally: + await channel.close() + finally: + handle.shutdown_requested.set() + with contextlib.suppress(Exception): + await asyncio.wait_for(handle.drive_task, timeout=2) + + +class TestShutdownCoordinatorWatchdog: + """▶ MUST-FIX 3: the hung-event-loop watchdog, independent of asyncio.""" + + def test_watchdog_fires_when_shutdown_never_confirmed(self) -> None: + exit_calls: list[int] = [] + stderr = io.StringIO() + coordinator = _ShutdownCoordinator(deadline=0.05, exit_fn=exit_calls.append, stderr=stderr) + + coordinator.start_watchdog() + # `exit_fn` here just records (doesn't actually exit), matching the + # design doc's injectable-exit_fn requirement so this test never + # risks killing the pytest process. + deadline = time.monotonic() + 2.0 + while not exit_calls and time.monotonic() < deadline: + time.sleep(0.01) + + assert exit_calls == [1] + diagnostic = stderr.getvalue() + assert "wedged" in diagnostic.lower() + + def test_watchdog_does_not_fire_if_confirmed_before_deadline(self) -> None: + exit_calls: list[int] = [] + coordinator = _ShutdownCoordinator( + deadline=0.2, exit_fn=exit_calls.append, stderr=io.StringIO() + ) + coordinator.start_watchdog() + coordinator.confirm_clean_exit() + + time.sleep(0.35) # past the deadline + assert exit_calls == [] + + def test_start_watchdog_is_idempotent(self) -> None: + exit_calls: list[int] = [] + coordinator = _ShutdownCoordinator( + deadline=0.05, exit_fn=exit_calls.append, stderr=io.StringIO() + ) + coordinator.start_watchdog() + coordinator.start_watchdog() # must not start a second overlapping timer + + deadline = time.monotonic() + 2.0 + while not exit_calls and time.monotonic() < deadline: + time.sleep(0.01) + + assert exit_calls == [1] # exactly one force-exit, not two + + def test_watchdog_forces_exit_even_with_a_genuinely_wedged_event_loop(self) -> None: + """Deliberately wedges a real asyncio event loop in a background thread. + + Simulates the ▶ MUST-FIX 3 scenario (a misbehaving sync-dispatched + ``write()`` that blocks the loop's own thread, never yielding back -- + no Go analog, since Go's runtime preemptively schedules goroutines + even through this). Proves the watchdog fires within its documented + bounded deadline regardless, because it runs on an independent OS + thread rather than depending on the (here, genuinely wedged) event + loop's thread for anything. + + Scope note: this test wedges the loop and confirms the watchdog's + independence from it; it does not exercise real OS `SIGTERM` + delivery to a wedged main thread (CPython's signal-delivery + interaction with a truly stuck main thread is its own, separately + hard-to-construct-deterministically concern -- see the PR's + Self-review for why that's flagged as compat-nightly-level scope, + not asserted here). + """ + exit_calls: list[int] = [] + stderr = io.StringIO() + coordinator = _ShutdownCoordinator(deadline=0.2, exit_fn=exit_calls.append, stderr=stderr) + + wedged_ready = threading.Event() + + def run_wedged_loop() -> None: + async def wedge_forever() -> None: + wedged_ready.set() + # A genuinely blocking, non-yielding call made directly + # inside `async def` -- the exact misbehavior MUST-FIX 3 + # describes (not routed through `conduit._dispatch.invoke`, + # which would correctly offload it to a thread pool instead). + # This is the test deliberately doing the wrong thing to + # prove the watchdog doesn't depend on it being done right. + time.sleep(5) # noqa: ASYNC251 + + with contextlib.suppress(BaseException): + asyncio.run(wedge_forever()) + + thread = threading.Thread(target=run_wedged_loop, daemon=True) + thread.start() + assert wedged_ready.wait(timeout=2), "background loop never started" + + # The event loop's thread is now busy in a non-yielding sleep and + # will not run any asyncio-scheduled callback for 5 seconds. Start + # the watchdog directly (simulating what the real SIGTERM handler + # does) and confirm it still fires well within that window. + coordinator.start_watchdog() + + deadline = time.monotonic() + 2.0 + while not exit_calls and time.monotonic() < deadline: + time.sleep(0.01) + + assert exit_calls == [1] + assert "wedged" in stderr.getvalue().lower() + + def test_default_shutdown_deadline_is_a_few_seconds(self) -> None: + """Sanity check on the documented default -- bounded, not instant or huge.""" + assert 1.0 <= DEFAULT_SHUTDOWN_DEADLINE_SECONDS <= 30.0 + + +def test_serve_requires_exactly_one_of_source_or_destination() -> None: + from conduit.serve import serve + + with pytest.raises(ValueError, match="exactly one"): + serve(_SPEC) + + with pytest.raises(ValueError, match="exactly one"): + serve(_SPEC, source=_TeardownSpySource, destination=_TeardownSpyDestination) diff --git a/tests/test_source.py b/tests/test_source.py new file mode 100644 index 0000000..c2465bd --- /dev/null +++ b/tests/test_source.py @@ -0,0 +1,190 @@ +"""Tests for :mod:`conduit.source` -- ack ordering (invariant 1) and backoff parity. + +The central property under test: a source record's position is only ever +acknowledged to the connector (``Source.ack``) after Conduit's ``Run`` +request stream sends it back via ``ack_positions`` -- never speculatively +when the record is merely produced by the read loop. +""" + +from __future__ import annotations + +import asyncio +from collections.abc import Callable + +import pytest + +from conduit._grpc.adapters import record_from_proto +from conduit.config import BaseConfig +from conduit.errors import BackoffRetry +from conduit.record import Operation, Record +from conduit.source import ( + BACKOFF_MAX_SECONDS, + BACKOFF_MIN_SECONDS, + Source, + _Backoff, + _SourceServicer, +) + + +class _Config(BaseConfig): + pass + + +class _FakeRunContext: + """Minimal stand-in for a grpc.aio ServicerContext -- unused by Run/Stop.""" + + +async def _empty_request_stream() -> object: + return + yield # pragma: no cover -- makes this an async generator with no items + + +class _AckPositionsRequest: + def __init__(self, ack_positions: list[bytes]) -> None: + self.ack_positions = ack_positions + + +class _CountingRecordsSource(Source[_Config]): + """Emits ``n`` records then raises BackoffRetry forever.""" + + def __init__(self, n: int) -> None: + self._n = n + self._emitted = 0 + self.acked: list[bytes] = [] + + async def read(self) -> Record: + if self._emitted >= self._n: + raise BackoffRetry() + self._emitted += 1 + return Record(position=f"pos-{self._emitted}".encode(), operation=Operation.CREATE) + + async def ack(self, position: bytes) -> None: + self.acked.append(position) + + +async def _wait_until(predicate: Callable[[], bool], *, timeout_seconds: float = 2.0) -> None: + """Poll an arbitrary boolean predicate until true or a timeout elapses. + + Deliberately polling, not waiting on a single ``asyncio.Event``: this + helper is used against several different, unrelated conditions (record + counts, ack lists) with no single event to await across all of them. + """ + try: + async with asyncio.timeout(timeout_seconds): + while not predicate(): # noqa: ASYNC110 -- see docstring + await asyncio.sleep(0.01) + except TimeoutError: + raise AssertionError(f"condition not met within {timeout_seconds}s") from None + + +class TestAckOnlyAfterConduitConfirms: + async def test_ack_is_not_called_before_any_ack_positions_arrive(self) -> None: + """Invariant 1: producing records must never itself call ack().""" + source = _CountingRecordsSource(n=3) + servicer = _SourceServicer(source, _Config) + records: list[Record] = [] + + async def consume() -> None: + async for response in servicer.Run(_empty_request_stream(), _FakeRunContext()): + for proto_record in response.records: + records.append(record_from_proto(proto_record)) + + consume_task = asyncio.create_task(consume()) + await _wait_until(lambda: len(records) >= 3) + + # The read loop has produced 3 records; no ack_positions were ever + # sent back (the request stream is empty), so ack() must not have + # been called for any of them. + assert source.acked == [] + + await servicer.Stop(object(), _FakeRunContext()) + await asyncio.wait_for(consume_task, timeout=2) + + async def test_ack_is_called_only_for_positions_conduit_sends_back(self) -> None: + source = _CountingRecordsSource(n=2) + servicer = _SourceServicer(source, _Config) + records: list[Record] = [] + ack_sent = asyncio.Event() + + async def request_stream() -> object: + await ack_sent.wait() + yield _AckPositionsRequest([b"pos-1"]) + + async def consume() -> None: + async for response in servicer.Run(request_stream(), _FakeRunContext()): + for proto_record in response.records: + records.append(record_from_proto(proto_record)) + + consume_task = asyncio.create_task(consume()) + await _wait_until(lambda: len(records) >= 2) + assert source.acked == [] # still nothing acked before ack_positions arrives + + ack_sent.set() + await _wait_until(lambda: source.acked == [b"pos-1"]) + + await servicer.Stop(object(), _FakeRunContext()) + await asyncio.wait_for(consume_task, timeout=2) + + +class TestBackoff: + def test_first_delay_is_min(self) -> None: + backoff = _Backoff() + assert backoff.duration() == BACKOFF_MIN_SECONDS + + def test_delay_doubles_each_attempt(self) -> None: + backoff = _Backoff() + first = backoff.duration() + second = backoff.duration() + third = backoff.duration() + assert second == pytest.approx(first * 2) + assert third == pytest.approx(first * 4) + + def test_delay_caps_at_max(self) -> None: + backoff = _Backoff() + delay = backoff.duration() + for _ in range(20): + delay = backoff.duration() + assert delay == BACKOFF_MAX_SECONDS + + def test_reset_restarts_the_curve(self) -> None: + backoff = _Backoff() + backoff.duration() + backoff.duration() + backoff.reset() + assert backoff.duration() == BACKOFF_MIN_SECONDS + + +async def test_stop_reports_last_emitted_position() -> None: + source = _CountingRecordsSource(n=2) + servicer = _SourceServicer(source, _Config) + records: list[Record] = [] + + async def consume() -> None: + async for response in servicer.Run(_empty_request_stream(), _FakeRunContext()): + for proto_record in response.records: + records.append(record_from_proto(proto_record)) + + consume_task = asyncio.create_task(consume()) + await _wait_until(lambda: len(records) >= 2) + + stop_response = await servicer.Stop(object(), _FakeRunContext()) + assert stop_response.last_position == b"pos-2" + await asyncio.wait_for(consume_task, timeout=2) + + +def test_source_is_abstract_without_read() -> None: + with pytest.raises(TypeError): + Source() # type: ignore[abstract] + + +async def test_configure_stores_config_by_default() -> None: + class Config(BaseConfig): + url: str + + class MySource(Source[Config]): + async def read(self) -> Record: + raise BackoffRetry() + + instance = MySource() + await instance.configure(Config(url="https://example.com")) + assert instance.config.url == "https://example.com" From 0bb1752d04fa2ff79ee35a47cc58f0a793cd557b Mon Sep 17 00:00:00 2001 From: DeVaris Brown Date: Thu, 23 Jul 2026 15:40:04 -0400 Subject: [PATCH 3/9] feat(testing): acceptance-test harness + worked http-poll-source example MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lane D of the v0.19 Python connector SDK workstream (docs/design/20260707-python-connector-sdk.md). - conduit/testing/acceptance.py: AcceptanceTestDriver Protocol, ConfigurableAcceptanceTestDriver convenience wrapper, and AcceptanceTestSuite -- the versioned (CONTRACT_VERSION = "2026-07.v1") acceptance suite an author subclasses in their own pytest module. Covers every category from the design doc §3: specifier existence/validity, config validation (success + required-param-missing), resume-at-position (snapshot and CDC-equivalent), read/write round trip, read timeout behavior, and partial-batch write correctness (paralleling test_destination_partial_write_nacks_all as an SDK-level guarantee). Exercises connectors in-process via the same servicer adapters serve.py uses -- no real gRPC socket, no real Conduit binary (that's compat-nightly.yml/Conduit-repo scope). - conduit/testing/fixtures.py: golden OpenCDC record-shape factories (snapshot/create/update/delete_record, with_collection). - examples/http-poll-source/main.py: the design doc §2.7 worked example, made fully runnable (httpx-based HTTP polling source, BackoffRetry on empty responses, position-based resume). - examples/http-poll-source/pyproject.toml: standalone packaging, mirroring what `conduit connector new --lang python` will scaffold (Phase 3). - tests/test_acceptance_harness.py: the suite run against a synthetic in-memory driver. - tests/test_example_http_poll_source.py: the suite run against the real, unmodified example file, in-process against a real local HTTP server (stdlib http.server, no httpx mocking) -- this test passes, it is not skipped or stubbed. - README/CHANGELOG updates reflecting Lanes B/C/D landing. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD --- CHANGELOG.md | 23 ++ README.md | 2 +- examples/http-poll-source/README.md | 20 +- examples/http-poll-source/main.py | 84 ++++++ examples/http-poll-source/pyproject.toml | 22 ++ src/conduit/testing/README.md | 22 +- src/conduit/testing/__init__.py | 24 ++ src/conduit/testing/acceptance.py | 367 +++++++++++++++++++++++ src/conduit/testing/fixtures.py | 87 ++++++ tests/test_acceptance_harness.py | 61 ++++ tests/test_example_http_poll_source.py | 152 ++++++++++ 11 files changed, 849 insertions(+), 15 deletions(-) create mode 100644 examples/http-poll-source/main.py create mode 100644 examples/http-poll-source/pyproject.toml create mode 100644 src/conduit/testing/__init__.py create mode 100644 src/conduit/testing/acceptance.py create mode 100644 src/conduit/testing/fixtures.py create mode 100644 tests/test_acceptance_harness.py create mode 100644 tests/test_example_http_poll_source.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ff6ded..62c7906 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,5 +17,28 @@ doc's Upgrade/rollback section for the pre-1.0 caveat). (`SourcePlugin`, `DestinationPlugin`, `SpecifierPlugin`). - Handshake implementation (`_handshake.py`): magic-cookie check, protocol version negotiation, stdout handshake line. +- `Source`/`Destination` ABCs (`source.py`/`destination.py`) with dual sync/ + async method dispatch (`_dispatch.py`), backing onto the generated + `SourcePlugin`/`DestinationPlugin` gRPC servicers. +- `serve()` entry point (`serve.py`): handshake validation, `grpc.aio` server + bootstrap, `grpc.health.v1` health registration, and a hand-written + `GRPCController.Shutdown` service (`_grpc/_controller.py`) for graceful + go-plugin teardown. +- Hung-event-loop watchdog (`_ShutdownCoordinator` in `serve.py`): an + independent `threading.Timer`-based force-exit deadline after `SIGTERM`, + bounding a genuinely wedged event loop's shutdown time. +- `BaseConfig`/`Field`/`to_parameters()` (`config.py`): pydantic-v2-based + config with introspection-driven `config.Parameter` mapping (no codegen). +- `Record`/`Change`/`Operation`/`Metadata` (`record.py`) and the proto + (de)serialization adapters (`_grpc/adapters.py`), including the documented + `google.protobuf.Struct` int→float fidelity boundary (B3). +- `BackoffRetry`/`BatchWriteError`/`ConnectorError` (`errors.py`), including + `BatchWriteError`'s construction-time-validated, exhaustive per-index + accounting (the B1 partial-batch-write fix). +- Acceptance-test harness (`testing/acceptance.py`, `testing/fixtures.py`): + `AcceptanceTestDriver` Protocol, `ConfigurableAcceptanceTestDriver`, + `AcceptanceTestSuite` (contract version `2026-07.v1`). +- Worked example connector (`examples/http-poll-source/`), exercised + end-to-end by the acceptance suite in this repo's own test suite. No release has been tagged yet; nothing here is installable from PyPI. diff --git a/README.md b/README.md index 923838b..14562f6 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,7 @@ src/conduit/ serve.py # handshake + gRPC server bootstrap _handshake.py # magic cookie, protocol negotiation, stdout line _grpc/ # generated protobuf/grpc stubs (buf generate output) - testing/ # acceptance-test harness (fast-follow) + testing/ # acceptance-test harness (acceptance.py, fixtures.py) examples/http-poll-source/ # worked example connector docs/design/ # design docs for this repo tests/ # unit tests diff --git a/examples/http-poll-source/README.md b/examples/http-poll-source/README.md index c1bcfbd..2dade09 100644 --- a/examples/http-poll-source/README.md +++ b/examples/http-poll-source/README.md @@ -1,12 +1,18 @@ -# http-poll-source (not started) +# http-poll-source The Phase-1 worked example connector from `docs/design/20260707-python-connector-sdk.md` §2.7 — a minimal source that -polls an HTTP endpoint for new rows. This is **Lane D** in the v0.19 -workstream, depending on Lane B (`Source`/`Destination` ABCs, `BaseConfig`, -the OpenCDC record model) — none of which exists yet as of this scaffold. +polls an HTTP endpoint for new rows. Implemented in `main.py`, fully runnable +(`python main.py`, launched by a go-plugin host such as Conduit — it is not +meant to be run as a bare script; see `conduit._handshake`). -This will become both the worked example *and* the source for -`conduit connector new --lang python`'s scaffolded template (Lane E), per the -design doc's §11 reasoning for reusing one connector for both rather than +Exercised end to end, in-process, by this repo's own +`tests/test_example_http_poll_source.py`, which runs the versioned +acceptance suite (`conduit.testing.acceptance`) against this exact file +against a real local HTTP server — not a mock of `httpx`, not a stub of the +connector. + +This is both the worked example *and* will become the source for +`conduit connector new --lang python`'s scaffolded template (Phase 3), per +the design doc's reasoning for reusing one connector for both rather than building a separate template repo before the SDK API has stabilized. diff --git a/examples/http-poll-source/main.py b/examples/http-poll-source/main.py new file mode 100644 index 0000000..5b2ddef --- /dev/null +++ b/examples/http-poll-source/main.py @@ -0,0 +1,84 @@ +"""A minimal Conduit source connector: polls an HTTP endpoint for new rows. + +The Phase-1 worked example from +``docs/design/20260707-python-connector-sdk.md`` §2.7 -- the SDK's own +"hello world," made fully runnable rather than illustrative-only. It expects +an HTTP endpoint that accepts ``?since=`` and returns a JSON list of +rows (each with an ``id`` field) newer than that cursor, oldest-first. + +Run it directly with ``python main.py`` under a real go-plugin host +(Conduit); it is not meant to be run as a plain script otherwise -- see +``conduit._handshake`` for why (this process expects +``CONDUIT_PLUGIN_MAGIC_COOKIE``/``PLUGIN_PROTOCOL_VERSIONS`` to be set by +the launching host). + +See ``tests/test_example_http_poll_source.py`` (this repo's own test suite, +not this directory) for the versioned acceptance suite run against this +exact file, in-process, against a real local HTTP server -- proving this +example is a fully working connector, not just a doc snippet. +""" + +from __future__ import annotations + +from datetime import UTC, datetime + +import httpx + +from conduit import BackoffRetry, Change, Metadata, Operation, Record, Source, serve +from conduit.config import BaseConfig, Field, Specification + + +class Config(BaseConfig): + """Configuration for :class:`HTTPPollSource`.""" + + url: str = Field(description="HTTP endpoint to poll, expects ?since=.") + poll_interval_ms: int = Field( + default=1000, ge=100, description="Delay between empty polls (paced by the SDK itself)." + ) + + +class HTTPPollSource(Source[Config]): + """Polls ``config.url?since=`` for new rows, oldest-first.""" + + async def open(self, position: bytes | None) -> None: + """Open the HTTP client and resume from ``position`` (or the beginning). + + Per invariant 2: ``position`` is the last cursor this connector (or + a previous run of it) successfully emitted -- resuming means + requesting strictly newer rows, never replaying it. + """ + self._client = httpx.AsyncClient() + self._since = position.decode() if position else "0" + + async def read(self) -> Record: + """Fetch the next row past ``self._since``, or signal there's nothing yet. + + Raises: + conduit.errors.BackoffRetry: the endpoint returned no new rows; + the SDK's own read loop paces retries -- this does not + sleep itself, avoiding a double backoff (design doc §2.7). + """ + resp = await self._client.get(self.config.url, params={"since": self._since}) + rows = resp.json() + if not rows: + raise BackoffRetry() + + row = rows[0] + self._since = str(row["id"]) + metadata: dict[str, str] = {} + Metadata.set_read_at(metadata, int(datetime.now(UTC).timestamp() * 1e9)) + return Record( + position=self._since.encode(), + operation=Operation.CREATE, + key={"id": row["id"]}, + payload=Change(after=row), + metadata=metadata, + ) + + async def teardown(self) -> None: + """Close the HTTP client.""" + await self._client.aclose() + + +if __name__ == "__main__": + serve(Specification(name="http-poll", version="0.1.0", author="you"), source=HTTPPollSource) diff --git a/examples/http-poll-source/pyproject.toml b/examples/http-poll-source/pyproject.toml new file mode 100644 index 0000000..42afb99 --- /dev/null +++ b/examples/http-poll-source/pyproject.toml @@ -0,0 +1,22 @@ +[build-system] +requires = ["hatchling"] +build-backend = "hatchling.build" + +[project] +name = "http-poll-source" +version = "0.1.0" +description = "Example Conduit source connector: polls an HTTP endpoint for new rows." +requires-python = ">=3.11" +# This mirrors what `conduit connector new --lang python` will scaffold +# (Phase 3, design doc §3/§4) -- a standalone connector project depending +# on the published `conduit-connector-sdk` package. Pinned as a local path +# dependency here since this example lives inside the SDK's own repo and is +# exercised by the SDK's own test suite (../../tests/test_example_http_poll_source.py), +# not published to PyPI itself. +dependencies = [ + "conduit-connector-sdk", + "httpx>=0.27,<1", +] + +[tool.uv.sources] +conduit-connector-sdk = { path = "../..", editable = true } diff --git a/src/conduit/testing/README.md b/src/conduit/testing/README.md index c01b394..e0e6a88 100644 --- a/src/conduit/testing/README.md +++ b/src/conduit/testing/README.md @@ -1,8 +1,16 @@ -# conduit.testing (not started) +# conduit.testing -This package will hold the acceptance-test harness (`AcceptanceTestDriver` -Protocol + `ConfigurableAcceptanceTestDriver`) per -`docs/design/20260707-python-connector-sdk.md` §3 and the v0.19 workstream's -Lane C. **Lane C depends on Lane B (B2 Source/Destination ABCs, B5 the B1 -ack/nack fix)**, neither of which exists yet as of this scaffold — do not -import from this package expecting anything to work. +The acceptance-test harness (`AcceptanceTestDriver` Protocol + +`ConfigurableAcceptanceTestDriver` convenience wrapper) and golden +record-shape fixtures, per `docs/design/20260707-python-connector-sdk.md` +§3. + +- `acceptance.py` — `AcceptanceTestSuite`, the versioned (`CONTRACT_VERSION`) + suite an author subclasses in their own `pytest` test module. See its + module docstring for the exact usage shape. +- `fixtures.py` — golden OpenCDC record-shape factory functions + (`snapshot_record`, `create_record`, `update_record`, `delete_record`). + +See `tests/test_acceptance_harness.py` (synthetic driver) and +`tests/test_example_http_poll_source.py` (the real worked example) in this +repo for working usage examples. diff --git a/src/conduit/testing/__init__.py b/src/conduit/testing/__init__.py new file mode 100644 index 0000000..b328435 --- /dev/null +++ b/src/conduit/testing/__init__.py @@ -0,0 +1,24 @@ +"""Testing utilities for Conduit connector authors. + +``conduit.testing.acceptance`` provides the versioned acceptance-test suite +(:class:`~conduit.testing.acceptance.AcceptanceTestSuite`) a connector must +pass; ``conduit.testing.fixtures`` provides golden OpenCDC record-shape +fixtures. See each module's docstring for details, and +``docs/design/20260707-python-connector-sdk.md`` §3 for the design. +""" + +from __future__ import annotations + +from conduit.testing.acceptance import ( + CONTRACT_VERSION, + AcceptanceTestDriver, + AcceptanceTestSuite, + ConfigurableAcceptanceTestDriver, +) + +__all__ = [ + "CONTRACT_VERSION", + "AcceptanceTestDriver", + "AcceptanceTestSuite", + "ConfigurableAcceptanceTestDriver", +] diff --git a/src/conduit/testing/acceptance.py b/src/conduit/testing/acceptance.py new file mode 100644 index 0000000..f6b9477 --- /dev/null +++ b/src/conduit/testing/acceptance.py @@ -0,0 +1,367 @@ +"""The acceptance-test harness: the shared contract a Python connector must pass. + +Mirrors the Go SDK's ``sdk.AcceptanceTest(t, driver)`` +(``conduit-connector-sdk/acceptance_testing.go:54-58``) -- "a connector" is +defined by conformance to the connector protocol and by passing this suite, +not by language (design doc, "The problem"). Per +``docs/design/20260707-python-connector-sdk.md`` §3, this harness is pulled +forward into v0.19 core scope rather than deferred to Phase 2. + +An author implements :class:`AcceptanceTestDriver` (or uses +:class:`ConfigurableAcceptanceTestDriver` for the common case) and +subclasses :class:`AcceptanceTestSuite` in their own ``pytest`` test module +-- the subclass must be named so pytest collects it (conventionally +``Test``) and must override :meth:`AcceptanceTestSuite.driver`. + +This harness exercises connectors **in-process** -- constructing +``Source``/``Destination`` instances directly and driving them through the +same servicer adapters :mod:`conduit.serve` uses, without a real gRPC +socket or a real Conduit binary. A real subprocess/Conduit launch is +``compat-nightly.yml``/Conduit-repo-side scope (design doc §3), not this +repo's CI. + +**Contract version:** :data:`CONTRACT_VERSION` -- bump only with a new, +additive test category or a documented breaking change to an existing one, +so an author's CI output states exactly which contract they passed (design +doc §3: "kept version-numbered"). +""" + +from __future__ import annotations + +import asyncio +from collections.abc import Mapping +from dataclasses import dataclass, field +from typing import Any, Protocol + +import pydantic + +import conduit._grpc # noqa: F401 -- sets up sys.path, see conduit._grpc.__init__ +from conduit._grpc.adapters import record_from_proto +from conduit.config import BaseConfig, Specification +from conduit.destination import Destination, _DestinationServicer +from conduit.errors import BatchWriteError +from conduit.record import Operation, Record +from conduit.source import Source, _SourceServicer +from conduit.testing.fixtures import create_record +from connector.v2 import source_pb2 + +CONTRACT_VERSION = "2026-07.v1" +"""Version tag for this acceptance suite. See module docstring.""" + + +class AcceptanceTestDriver(Protocol): + """What a connector author implements to run the suite against their connector. + + Every method is synchronous and side-effect-free (returns + metadata/classes, does not itself talk to a network) -- the suite + constructs and drives actual ``Source``/``Destination`` instances + itself using what these methods return. + """ + + def specification(self) -> Specification: + """Return the connector's static specification.""" + ... + + def source_class(self) -> type[Source[Any]] | None: + """Return the ``Source`` subclass under test, or ``None`` if source-less.""" + ... + + def destination_class(self) -> type[Destination[Any]] | None: + """Return the ``Destination`` subclass under test, or ``None`` if destination-less.""" + ... + + def source_config(self) -> Mapping[str, str]: + """Return a valid config map for the source (as it would arrive over the wire).""" + ... + + def destination_config(self) -> Mapping[str, str]: + """Return a valid config map for the destination.""" + ... + + +@dataclass(slots=True) +class ConfigurableAcceptanceTestDriver: + """Convenience :class:`AcceptanceTestDriver` for the common single-connector case. + + Construct one with whichever of ``source_cls``/``destination_cls`` your + connector provides -- most connectors implement only one of the two. + """ + + spec: Specification + source_cls: type[Source[Any]] | None = None + destination_cls: type[Destination[Any]] | None = None + source_cfg: Mapping[str, str] = field(default_factory=dict) + destination_cfg: Mapping[str, str] = field(default_factory=dict) + + def specification(self) -> Specification: + """See :meth:`AcceptanceTestDriver.specification`.""" + return self.spec + + def source_class(self) -> type[Source[Any]] | None: + """See :meth:`AcceptanceTestDriver.source_class`.""" + return self.source_cls + + def destination_class(self) -> type[Destination[Any]] | None: + """See :meth:`AcceptanceTestDriver.destination_class`.""" + return self.destination_cls + + def source_config(self) -> Mapping[str, str]: + """See :meth:`AcceptanceTestDriver.source_config`.""" + return self.source_cfg + + def destination_config(self) -> Mapping[str, str]: + """See :meth:`AcceptanceTestDriver.destination_config`.""" + return self.destination_cfg + + +class AcceptanceTestSuite: + """Mixin providing the versioned acceptance-test suite as test methods. + + Subclass this in your connector's own ``pytest`` test module, name the + subclass so pytest collects it (e.g. ``TestAcceptance``), and override + :meth:`driver`. Every ``test_*`` method below is one named, independently + reportable category from ``docs/design/20260707-python-connector-sdk.md`` + §3. + """ + + def driver(self) -> AcceptanceTestDriver: + """Return the :class:`AcceptanceTestDriver` to run the suite against. + + Must be overridden by subclasses; the default raises so a + forgotten override fails loudly and immediately, not with a + confusing downstream ``AttributeError``. + """ + raise NotImplementedError("Override driver() to return your AcceptanceTestDriver") + + # -- Category: specifier existence/validity --------------------------- + + async def test_specifier_exists_and_is_valid(self) -> None: + """The connector's ``Specify`` metadata is present and well-formed.""" + driver = self.driver() + spec = driver.specification() + assert spec.name, "Specification.name must be non-empty" + assert spec.version, "Specification.version must be non-empty" + assert spec.author, "Specification.author must be non-empty" + + source_cls = driver.source_class() + destination_cls = driver.destination_class() + assert source_cls is not None or destination_cls is not None, ( + "a connector must provide at least one of source_class()/destination_class()" + ) + + # -- Category: config parameter validation ----------------------------- + + async def test_config_validation_succeeds_with_valid_config(self) -> None: + """A valid config map is accepted (source and/or destination).""" + driver = self.driver() + if (source_cls := driver.source_class()) is not None: + config_cls = _config_class(source_cls, Source) + config_cls.model_validate(dict(driver.source_config())) + if (destination_cls := driver.destination_class()) is not None: + config_cls = _config_class(destination_cls, Destination) + config_cls.model_validate(dict(driver.destination_config())) + + async def test_config_validation_fails_with_missing_required_param(self) -> None: + """Omitting any single required field is rejected, not silently defaulted.""" + driver = self.driver() + for cls, base, config in ( + (driver.source_class(), Source, driver.source_config()), + (driver.destination_class(), Destination, driver.destination_config()), + ): + if cls is None: + continue + config_cls = _config_class(cls, base) + required = { + name for name, info in config_cls.model_fields.items() if info.is_required() + } + if not required: + continue # nothing required -- this connector has no required-param case + missing_one = dict(config) + missing_one.pop(next(iter(required))) + try: + config_cls.model_validate(missing_one) + except pydantic.ValidationError: + pass + else: + raise AssertionError( + f"{config_cls.__name__}.model_validate() accepted a config " + "missing a required field -- required-param validation is broken" + ) + + # -- Category: resume-at-position (snapshot and CDC-equivalent) ------- + + async def test_resume_at_position_snapshot(self) -> None: + """Reopening at a previously-emitted position does not replay it (snapshot-style).""" + await self._assert_resume_does_not_replay(Operation.SNAPSHOT) + + async def test_resume_at_position_cdc(self) -> None: + """Reopening at a previously-emitted position does not replay it (CDC-style).""" + await self._assert_resume_does_not_replay(Operation.CREATE) + + async def _assert_resume_does_not_replay(self, operation: Operation) -> None: + driver = self.driver() + source_cls = driver.source_class() + if source_cls is None: + return # destination-only connector -- nothing to resume + + config_cls = _config_class(source_cls, Source) + config = config_cls.model_validate(dict(driver.source_config())) + + first_run = source_cls() + await first_run.configure(config) + await first_run.open(None) + seen_positions: set[bytes] = set() + last_position = b"" + for _ in range(2): + record = await _read_one(first_run) + seen_positions.add(record.position) + last_position = record.position + await first_run.teardown() + + second_run = source_cls() + await second_run.configure(config) + await second_run.open(last_position) + record = await _read_one(second_run) + await second_run.teardown() + + assert record.position not in seen_positions, ( + f"resuming from position {last_position!r} replayed an " + f"already-emitted position {record.position!r} -- invariant 2 " + "(monotonic, crash-safe positions) requires read() to resume " + "strictly after the position passed to open()" + ) + + # -- Category: read/write round trip ----------------------------------- + + async def test_read_write_round_trip(self) -> None: + """A record read from the source (or synthesized) writes successfully to the destination.""" + driver = self.driver() + source_cls = driver.source_class() + destination_cls = driver.destination_class() + + if source_cls is not None: + config_cls = _config_class(source_cls, Source) + source = source_cls() + await source.configure(config_cls.model_validate(dict(driver.source_config()))) + await source.open(None) + record = await _read_one(source) + await source.teardown() + else: + record = create_record(b"acceptance-1", "1", {"id": "1"}) + + if destination_cls is not None: + config_cls = _config_class(destination_cls, Destination) + destination = destination_cls() + await destination.configure( + config_cls.model_validate(dict(driver.destination_config())) + ) + await destination.open() + await destination.write([record]) # must not raise + await destination.teardown() + + # -- Category: read timeout behavior ------------------------------------ + + async def test_read_timeout_behavior(self) -> None: + """``BackoffRetry`` from ``read()`` never blocks the loop indefinitely or errors. + + A source with nothing to read must let the read loop pace itself + with backoff (not raise, not hang past ``Stop()``) -- exercised + directly against a minimal always-empty ``Source`` double, since + this is an SDK-adapter guarantee, not something each connector's + own ``read()`` needs to separately prove (parallels how + ``test_destination_partial_write_nacks_all`` is an SDK-level + guarantee test, not a per-connector one -- see + :meth:`test_partial_batch_write_correctness`). + """ + from conduit.errors import BackoffRetry + + class _Config(BaseConfig): + pass + + class _AlwaysEmptySource(Source[_Config]): + async def read(self) -> Record: + raise BackoffRetry() + + servicer = _SourceServicer(_AlwaysEmptySource(), _Config) + + async def empty_requests() -> Any: + return + yield # pragma: no cover + + produced: list[Record] = [] + + async def consume() -> None: + async for response in servicer.Run(empty_requests(), object()): + for proto_record in response.records: + produced.append(record_from_proto(proto_record)) + + consume_task = asyncio.create_task(consume()) + await asyncio.sleep(0.05) # well under one backoff cycle + assert produced == [], "an always-empty source must not fabricate records" + + # Stop() must return promptly -- proving the backoff wait is + # interruptible, not an uninterruptible sleep that would make + # shutdown hang behind a source with nothing to read. + await asyncio.wait_for( + servicer.Stop(source_pb2.Source.Stop.Request(), object()), timeout=1.0 + ) + await asyncio.wait_for(consume_task, timeout=1.0) + + # -- Category: partial-batch write correctness (the B1 fix) ------------ + + async def test_partial_batch_write_correctness(self) -> None: + """The SDK's write adapter fails closed on a partial batch -- see B1 (§2.5). + + Parallels ``tests/test_destination_partial_write_nacks_all``: an + SDK-level adapter guarantee applicable identically to every + destination connector, not specific to the driver's own + implementation. + """ + + class _Config(BaseConfig): + pass + + class _PartialWriteDestination(Destination[_Config]): + async def write(self, records: list[Record]) -> None: + raise BatchWriteError(len(records), written=1) + + servicer = _DestinationServicer(_PartialWriteDestination(), _Config) + records = [create_record(f"acc-{i}".encode(), str(i), {"id": i}) for i in range(3)] + acks = await servicer._write_batch(records) # whitebox: the adapter's own contract + + assert acks[0].error == "", "index 0 was within the written prefix -- must be acked" + assert acks[1].error != "", "index 1 was past the written prefix -- must be nacked" + assert acks[2].error != "", "index 2 was past the written prefix -- must be nacked" + + +async def _read_one(source: Source[Any]) -> Record: + """Call ``source.read()``, transparently retrying through ``BackoffRetry``. + + A thin helper so acceptance test bodies don't each need their own + backoff-retry loop -- this harness isn't measuring backoff timing + itself (that's :mod:`tests.test_source`'s job in this repo), just + getting a real record out of a real connector. + """ + from conduit.errors import BackoffRetry + + for _ in range(1000): + try: + return await source.read() + except BackoffRetry: + await asyncio.sleep(0.01) + raise AssertionError("source.read() never returned a record after 1000 attempts") + + +def _config_class(connector_cls: type[Any], base: type[Any]) -> type[BaseConfig]: + """Recover a connector's concrete config class for driver-supplied config maps.""" + from conduit._introspect import resolve_config_class + + return resolve_config_class(connector_cls, base) + + +__all__ = [ + "CONTRACT_VERSION", + "AcceptanceTestDriver", + "AcceptanceTestSuite", + "ConfigurableAcceptanceTestDriver", +] diff --git a/src/conduit/testing/fixtures.py b/src/conduit/testing/fixtures.py new file mode 100644 index 0000000..9895d39 --- /dev/null +++ b/src/conduit/testing/fixtures.py @@ -0,0 +1,87 @@ +"""Golden OpenCDC record-shape fixtures for connector tests. + +A small, representative set of record shapes -- one per :class:`~conduit.record.Operation` +-- so a connector's own tests (and :mod:`conduit.testing.acceptance`) exercise +the same shapes consistently, rather than every test author hand-rolling +slightly different ad hoc records. Per +``docs/design/20260707-python-connector-sdk.md`` §3/Phase 2: a full corpus +shared with the Go/other-language acceptance suites is fast-follow (v0.20) +scope; this module is the Phase-1 seed of that idea, scoped to what this +repo's own acceptance harness and example connector need today. +""" + +from __future__ import annotations + +from conduit.record import Change, Metadata, Operation, Record + + +def snapshot_record(position: bytes, key: str, value: dict[str, object]) -> Record: + """A bulk/backfill-style record: ``OPERATION_SNAPSHOT``, no ``before``. + + Args: + position: the record's position. + key: the record's key (wrapped as ``{"id": key}``). + value: the row's current value, used as ``payload.after``. + """ + return Record( + position=position, + operation=Operation.SNAPSHOT, + key={"id": key}, + payload=Change(after=value), + ) + + +def create_record(position: bytes, key: str, value: dict[str, object]) -> Record: + """A newly-inserted-row record: ``OPERATION_CREATE``, no ``before``.""" + return Record( + position=position, + operation=Operation.CREATE, + key={"id": key}, + payload=Change(after=value), + ) + + +def update_record( + position: bytes, + key: str, + before: dict[str, object], + after: dict[str, object], +) -> Record: + """A modified-row record: ``OPERATION_UPDATE``, both ``before`` and ``after`` set.""" + return Record( + position=position, + operation=Operation.UPDATE, + key={"id": key}, + payload=Change(before=before, after=after), + ) + + +def delete_record(position: bytes, key: str, before: dict[str, object]) -> Record: + """A removed-row record: ``OPERATION_DELETE``, no ``after``.""" + return Record( + position=position, + operation=Operation.DELETE, + key={"id": key}, + payload=Change(before=before), + ) + + +def with_collection(record: Record, collection: str) -> Record: + """Return a copy of ``record`` with ``opencdc.collection`` metadata set. + + Args: + record: the record to annotate. Not mutated in place -- a shallow + copy with a new ``metadata`` dict is returned, so callers reusing + a shared fixture record across cases don't see cross-test + mutation. + collection: the source collection (table/topic) name. + """ + metadata = dict(record.metadata) + Metadata.set_collection(metadata, collection) + return Record( + position=record.position, + operation=record.operation, + metadata=metadata, + key=record.key, + payload=record.payload, + ) diff --git a/tests/test_acceptance_harness.py b/tests/test_acceptance_harness.py new file mode 100644 index 0000000..caef8e2 --- /dev/null +++ b/tests/test_acceptance_harness.py @@ -0,0 +1,61 @@ +"""Tests for :mod:`conduit.testing.acceptance` itself -- run against a small +synthetic source and destination, proving the suite's categories actually +exercise real connector behavior (not stubs). +""" + +from __future__ import annotations + +from conduit.config import BaseConfig, Field +from conduit.destination import Destination +from conduit.errors import BackoffRetry +from conduit.record import Change, Operation, Record +from conduit.source import Source +from conduit.testing.acceptance import AcceptanceTestSuite, ConfigurableAcceptanceTestDriver + + +class _SourceConfig(BaseConfig): + url: str = Field(description="required for the required-param-missing test") + + +class _DestinationConfig(BaseConfig): + target: str = Field(description="required for the required-param-missing test") + + +class _InMemorySource(Source[_SourceConfig]): + """A tiny in-memory source: emits integers 0, 1, 2, ... resuming after `position`.""" + + async def open(self, position: bytes | None) -> None: + self._next = int(position.decode()) + 1 if position else 0 + + async def read(self) -> Record: + if self._next > 100: + raise BackoffRetry() + value = self._next + self._next += 1 + return Record( + position=str(value).encode(), + operation=Operation.CREATE if value > 0 else Operation.SNAPSHOT, + key={"id": value}, + payload=Change(after={"id": value}), + ) + + +class _InMemoryDestination(Destination[_DestinationConfig]): + def __init__(self) -> None: + self.written: list[Record] = [] + + async def write(self, records: list[Record]) -> None: + self.written.extend(records) + + +class TestAcceptance(AcceptanceTestSuite): + def driver(self) -> ConfigurableAcceptanceTestDriver: + from conduit.config import Specification + + return ConfigurableAcceptanceTestDriver( + spec=Specification(name="in-memory-test", version="0.0.0", author="test"), + source_cls=_InMemorySource, + destination_cls=_InMemoryDestination, + source_cfg={"url": "https://example.com"}, + destination_cfg={"target": "table"}, + ) diff --git a/tests/test_example_http_poll_source.py b/tests/test_example_http_poll_source.py new file mode 100644 index 0000000..9ff26c6 --- /dev/null +++ b/tests/test_example_http_poll_source.py @@ -0,0 +1,152 @@ +"""Runs the versioned acceptance suite against the Phase-1 worked example connector. + +``examples/http-poll-source/main.py`` is exercised **unmodified** and +**in-process**, against a real local HTTP server (stdlib +``http.server.ThreadingHTTPServer``, backed by a small in-memory dataset -- +no mocking of ``httpx`` internals). This is deliberately real HTTP, just not +a real Conduit binary or subprocess launch -- per +``docs/design/20260707-python-connector-sdk.md`` §3, that level of +end-to-end verification is ``compat-nightly.yml``/Conduit-repo-side scope, +not this repo's CI. This test must actually pass; it is not skipped or +stubbed. +""" + +from __future__ import annotations + +import http.server +import importlib.util +import json +import threading +from collections.abc import Iterator +from pathlib import Path +from types import ModuleType +from urllib.parse import parse_qs, urlparse + +import pytest + +from conduit.config import Specification +from conduit.testing.acceptance import AcceptanceTestSuite, ConfigurableAcceptanceTestDriver + +_EXAMPLE_MAIN = Path(__file__).resolve().parent.parent / "examples" / "http-poll-source" / "main.py" + +# 20 rows, ids 1..20 -- enough for the acceptance suite's resume-at-position +# tests (which read a couple of records, reopen, and expect a genuinely +# later one) without exhausting the dataset. +_ROWS = [{"id": i} for i in range(1, 21)] + + +def _load_example_module() -> ModuleType: + """Load ``examples/http-poll-source/main.py`` by file path. + + The directory is hyphenated (matching a real connector repo's naming + convention, per the design doc's proposed layout) and therefore not a + valid Python package/module name to import with a plain ``import`` + statement -- loading by explicit file path sidesteps that without + renaming the example to satisfy Python's import system instead of the + org's naming convention. + """ + spec = importlib.util.spec_from_file_location("http_poll_source_example", _EXAMPLE_MAIN) + assert spec is not None + assert spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +class _RowsHandler(http.server.BaseHTTPRequestHandler): + """Serves ``_ROWS`` one at a time past ``?since=``, oldest-first. + + Matches the contract ``HTTPPollSource.read()`` expects (design doc + §2.7): a JSON list, empty when there's nothing newer than ``since``. + """ + + def do_GET(self) -> None: + query = parse_qs(urlparse(self.path).query) + since = int(query.get("since", ["0"])[0]) + remaining = [row for row in _ROWS if row["id"] > since] + body = json.dumps(remaining[:1]).encode() + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, format_str: str, *args: object) -> None: + """Silence the default stderr access log -- noisy for a test server.""" + return None + + +@pytest.fixture(scope="module") +def _rows_server() -> Iterator[http.server.ThreadingHTTPServer]: + server = http.server.ThreadingHTTPServer(("127.0.0.1", 0), _RowsHandler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + yield server + finally: + server.shutdown() + thread.join(timeout=2) + + +class _HTTPPollSourceAcceptanceHelper(AcceptanceTestSuite): + """The versioned acceptance suite, run against the real example file.""" + + def __init__(self, rows_server: http.server.ThreadingHTTPServer) -> None: + self._rows_server = rows_server + + def driver(self) -> ConfigurableAcceptanceTestDriver: + module = _load_example_module() + host, port = self._rows_server.server_address[:2] + return ConfigurableAcceptanceTestDriver( + spec=Specification(name="http-poll", version="0.1.0", author="you"), + source_cls=module.HTTPPollSource, + source_cfg={"url": f"http://{host}:{port}"}, + ) + + +@pytest.fixture +def _suite( + _rows_server: http.server.ThreadingHTTPServer, +) -> _HTTPPollSourceAcceptanceHelper: + return _HTTPPollSourceAcceptanceHelper(_rows_server) + + +async def test_specifier_exists_and_is_valid( + _suite: _HTTPPollSourceAcceptanceHelper, +) -> None: + await _suite.test_specifier_exists_and_is_valid() + + +async def test_config_validation_succeeds_with_valid_config( + _suite: _HTTPPollSourceAcceptanceHelper, +) -> None: + await _suite.test_config_validation_succeeds_with_valid_config() + + +async def test_config_validation_fails_with_missing_required_param( + _suite: _HTTPPollSourceAcceptanceHelper, +) -> None: + await _suite.test_config_validation_fails_with_missing_required_param() + + +async def test_resume_at_position_snapshot(_suite: _HTTPPollSourceAcceptanceHelper) -> None: + await _suite.test_resume_at_position_snapshot() + + +async def test_resume_at_position_cdc(_suite: _HTTPPollSourceAcceptanceHelper) -> None: + await _suite.test_resume_at_position_cdc() + + +async def test_read_write_round_trip(_suite: _HTTPPollSourceAcceptanceHelper) -> None: + await _suite.test_read_write_round_trip() + + +async def test_read_timeout_behavior(_suite: _HTTPPollSourceAcceptanceHelper) -> None: + await _suite.test_read_timeout_behavior() + + +async def test_partial_batch_write_correctness( + _suite: _HTTPPollSourceAcceptanceHelper, +) -> None: + """SDK-level B1 guarantee -- applies identically even to this source-only connector.""" + await _suite.test_partial_batch_write_correctness() From 716ae2dd4e6f319f7dd27adb7164d009e5f0c239 Mon Sep 17 00:00:00 2001 From: DeVaris Brown Date: Thu, 23 Jul 2026 15:43:05 -0400 Subject: [PATCH 4/9] fix(ci): pin types-protobuf dev dep + reformat design doc's embedded code CI (uv sync --all-extras, ruff/mypy jobs) caught two gaps not visible in my local dev environment, whose packages had drifted from what a fresh `pip install -e '.[dev]'`/`uv sync` resolves: - mypy: `types-protobuf` was installed manually in my local venv while developing but never added to pyproject.toml's dev extras, so a fresh environment (CI, or any contributor running `uv sync`) hit "Library stubs not installed for google.protobuf" in serve.py/_grpc/_controller.py. Added `types-protobuf>=6.30,<7` to `[project.optional-dependencies].dev`. - ruff format: ruff 0.16.0 (resolved by CI's `ruff>=0.14,<1` constraint; 0.15.x, what I had locally, treats Markdown formatting as preview-only and skips it by default) now formats Markdown-embedded Python code fences by default. This caught 3 pre-existing, purely cosmetic whitespace inconsistencies in docs/design/20260707-python-connector-sdk.md's embedded code examples (double-space-before-comment, missing blank lines) -- not present in any file this PR otherwise touches, not a content change, verified via `git diff` to be whitespace-only. Verified by simulating CI exactly: a fresh venv, `pip install -e '.[dev]'` (uv unavailable in this sandbox), then ruff format --check / ruff check / mypy / pytest -- all green, matching CI's actual dependency resolution rather than my possibly-stale local venv. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD --- docs/design/20260707-python-connector-sdk.md | 7 +++++-- pyproject.toml | 4 ++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/docs/design/20260707-python-connector-sdk.md b/docs/design/20260707-python-connector-sdk.md index d549a7b..5660cd0 100644 --- a/docs/design/20260707-python-connector-sdk.md +++ b/docs/design/20260707-python-connector-sdk.md @@ -417,8 +417,9 @@ is enough to pick the right oneof branch. **`Data = bytes | Mapping[str, Any]`.* ```python @dataclass class Change: - before: Data | None = None # update/delete only - after: Data | None = None # all ops except delete + before: Data | None = None # update/delete only + after: Data | None = None # all ops except delete + class Operation(enum.Enum): CREATE = 1 @@ -426,6 +427,7 @@ class Operation(enum.Enum): DELETE = 3 SNAPSHOT = 4 + @dataclass class Record: position: bytes @@ -548,6 +550,7 @@ metadata is auto-populated. ```python """A minimal Conduit source connector: polls an HTTP endpoint for new rows.""" + from __future__ import annotations import asyncio diff --git a/pyproject.toml b/pyproject.toml index 8c23c91..8c0c6f5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -50,6 +50,10 @@ dev = [ "pytest-asyncio>=0.25,<1", "hypothesis>=6.120,<7", "grpc-stubs>=1.24,<2", + # Type stubs for google.protobuf (e.g. empty_pb2.Empty in serve.py/ + # _grpc/_controller.py) -- without this, mypy reports "Library stubs + # not installed for google.protobuf" under strict mode. + "types-protobuf>=6.30,<7", # Bounds the hung-event-loop watchdog test (▶ MUST-FIX 3) and the # deterministic-shutdown test (▶ MUST-FIX 2) so a regression that makes # either hang fails CI promptly instead of hanging the test run itself. From deae18d8f39a1ad7228ec9d037c061dcd15c74b8 Mon Sep 17 00:00:00 2001 From: DeVaris Brown Date: Thu, 23 Jul 2026 16:16:30 -0400 Subject: [PATCH 5/9] feat(config): Go-duration support for timedelta config fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DX audit fix #1. `datetime.timedelta`-typed BaseConfig fields now map to config.Parameter.TYPE_DURATION instead of raising NotImplementedError: - New public conduit.config.format_go_duration()/parse_go_duration(): serialize/parse Go's time.Duration.String()/ParseDuration syntax ("5s", "1h30m", "500ms", "1.5h", "-1.5h", ns/us/µs/ms/s/m/h units), using exact fractions.Fraction arithmetic (no float rounding) down to microsecond resolution -- the finest datetime.timedelta supports. - to_parameters() serializes a timedelta field's default via format_go_duration(); gt=/lt=/ge=/le= constraints on duration fields use the same exact (not epsilon-approximated) ±1-microsecond boundary adjustment already used for int fields, since timedelta is likewise a discrete, integer-resolution type. - BaseConfig gained a model_validator(mode="before") that parses a Go-duration string into a real timedelta before pydantic's own validation runs, so the Configure RPC's map config values round-trip correctly. Direct construction with a real timedelta still works unchanged. - TYPE_EXCLUSION remains an open, documented A-gap (untouched by this fix) -- still raises NotImplementedError rather than guessing. Tests: tests/test_go_duration.py (known-Go-output pins + Hypothesis round-trip property over arbitrary microsecond counts + malformed-syntax rejection), tests/test_config.py (to_parameters() mapping + Configure-side string parsing + direct-construction-still-works cases). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD --- src/conduit/config.py | 283 +++++++++++++++++++++++++++++++++----- tests/test_config.py | 57 ++++++-- tests/test_go_duration.py | 122 ++++++++++++++++ 3 files changed, 417 insertions(+), 45 deletions(-) create mode 100644 tests/test_go_duration.py diff --git a/src/conduit/config.py b/src/conduit/config.py index e300165..f282c0c 100644 --- a/src/conduit/config.py +++ b/src/conduit/config.py @@ -13,7 +13,10 @@ from __future__ import annotations import datetime +import re +from collections.abc import Mapping from dataclasses import dataclass +from fractions import Fraction from typing import Any, Literal, get_args, get_origin import annotated_types @@ -72,6 +75,38 @@ def to_parameters(cls) -> dict[str, _parameter_pb2.Parameter]: """ return to_parameters(cls) + @pydantic.model_validator(mode="before") + @classmethod + def _parse_go_durations_before_validation(cls, data: Any) -> Any: + """Parse Go-duration-syntax strings for ``timedelta``-typed fields. + + The ``Configure`` RPC's config map is always ``map`` + on the wire (see the wire contract facts) -- pydantic v2 has no + built-in support for Go's ``"5s"``/``"1h30m"`` duration syntax (it + understands ISO-8601 durations and bare numeric seconds, not this). + This ``model_validator(mode="before")`` runs ahead of pydantic's own + field validation and converts any string value destined for a + ``datetime.timedelta``-typed field into an actual ``timedelta`` via + :func:`parse_go_duration`, so the rest of validation proceeds + exactly as if a real ``timedelta`` had been passed in. Symmetric + with :func:`format_go_duration`, used by :func:`to_parameters` to + serialize a ``timedelta`` default for the ``Specify`` RPC -- see + that function for the exact wire format both sides agree on. + + Non-string values (e.g. an author constructing the model directly + with a real ``timedelta``, as in tests) pass through unchanged. + """ + if not isinstance(data, Mapping): + return data + converted = dict(data) + for name, info in cls.model_fields.items(): + if _unwrap_optional(info.annotation) is not datetime.timedelta: + continue + value = converted.get(name) + if isinstance(value, str): + converted[name] = parse_go_duration(value) + return converted + @dataclass(slots=True) class Specification: @@ -110,23 +145,29 @@ def to_parameters(config_cls: type[BaseConfig]) -> dict[str, _parameter_pb2.Para pydantic's ``gt``/``lt`` exactly). - ``ge=``/``le=`` -> **approximated** as ``TYPE_GREATER_THAN``/ ``TYPE_LESS_THAN`` by nudging the boundary so the declared value - itself still validates: for ``int``-typed fields this is exact - (boundary ``- 1``/``+ 1``); for ``float``-typed fields this uses a - small (``1e-9``) epsilon nudge, which is an approximation, not exact - -- see :data:`_FLOAT_BOUNDARY_EPSILON`. Documented here rather than + itself still validates: for ``int``-typed (and ``timedelta``-typed -- + both are fundamentally discrete, integer-microsecond-resolution + types) fields this is exact (boundary ``- 1``/``+ 1`` unit); for + ``float``-typed fields this uses a small (``1e-9``) epsilon nudge, + which is an approximation, not exact -- see + :data:`_FLOAT_BOUNDARY_EPSILON`. Documented here rather than silently producing a subtly-wrong validation. - ``Literal[...]`` -> one ``Validation.TYPE_INCLUSION`` entry per literal value (``Parameter.validations`` is ``repeated Validation``, matching the Go SDK's shape). - ``pattern=`` -> ``Validation.TYPE_REGEX``. + - ``datetime.timedelta`` -> ``Parameter.Type.TYPE_DURATION``, with the + default (if any) serialized via :func:`format_go_duration` into Go's + ``time.Duration.String()`` syntax (``"5s"``, ``"1h30m"``, ``"500ms"``, + not ISO-8601). :class:`BaseConfig`'s ``model_validator`` parses that + same syntax back (:func:`parse_go_duration`) when the ``Configure`` + RPC's string config map arrives -- see that validator's docstring. + This closes what was previously an open A-gap (a plain + ``NotImplementedError``); see git history for the prior wording if + you're looking for why this changed. + + **Still an open A-gap, non-blocking for Phase 1:** - **Explicitly not attempted (open A-gaps, design doc §2.2, non-blocking - for Phase 1):** - - - ``Parameter.Type.TYPE_DURATION`` (Go's ``"5s"``-style duration - strings, not ISO-8601) has no pydantic-native mapping. A field typed - ``datetime.timedelta`` raises ``NotImplementedError`` rather than - guessing at a wrong mapping. - ``Validation.Type.TYPE_EXCLUSION`` has no pydantic-native constraint to introspect. A field requesting it via ``Field(json_schema_extra={"exclusion": [...]})`` raises @@ -141,21 +182,13 @@ def to_parameters(config_cls: type[BaseConfig]) -> dict[str, _parameter_pb2.Para ``Specifier.Specify.Response.source_params``/``destination_params``. Raises: - NotImplementedError: if a field requests ``TYPE_DURATION`` or - ``TYPE_EXCLUSION`` semantics (see above). + NotImplementedError: if a field requests ``TYPE_EXCLUSION`` + semantics (see above). """ return {name: _field_to_parameter(name, info) for name, info in config_cls.model_fields.items()} def _field_to_parameter(name: str, info: FieldInfo) -> _parameter_pb2.Parameter: - if info.annotation in (datetime.timedelta,): - raise NotImplementedError( - f"config field {name!r}: `datetime.timedelta` (duration) has no " - "pydantic-native mapping to config.Parameter.TYPE_DURATION yet -- " - "open A-gap, docs/design/20260707-python-connector-sdk.md §2.2. " - "Use a plain int (milliseconds) or str field with duration " - "semantics documented in the field description instead." - ) extra = info.json_schema_extra if isinstance(info.json_schema_extra, dict) else {} if "exclusion" in extra: raise NotImplementedError( @@ -177,10 +210,14 @@ def _field_to_parameter(name: str, info: FieldInfo) -> _parameter_pb2.Parameter: ) is_int = param_type == _parameter_pb2.Parameter.TYPE_INT - validations.extend(_constraint_validations(info, is_int=is_int)) + is_duration = param_type == _parameter_pb2.Parameter.TYPE_DURATION + validations.extend(_constraint_validations(info, is_int=is_int, is_duration=is_duration)) if info.is_required(): default = "" + elif is_duration: + default_value = info.get_default(call_default_factory=True) + default = "" if default_value is None else format_go_duration(default_value) else: default = _format_default(info.get_default(call_default_factory=True)) @@ -234,6 +271,8 @@ def _resolve_type(annotation: Any) -> _ParamTypeAndLiterals: return _parameter_pb2.Parameter.TYPE_FLOAT, () if annotation is str: return _parameter_pb2.Parameter.TYPE_STRING, () + if annotation is datetime.timedelta: + return _parameter_pb2.Parameter.TYPE_DURATION, () # Unknown/unsupported annotation (e.g. a nested BaseModel, a custom # type): fall back to TYPE_STRING. This is a deliberate, documented @@ -243,35 +282,39 @@ def _resolve_type(annotation: Any) -> _ParamTypeAndLiterals: return _parameter_pb2.Parameter.TYPE_STRING, () -def _constraint_validations(info: FieldInfo, *, is_int: bool) -> list[_parameter_pb2.Validation]: +def _constraint_validations( + info: FieldInfo, *, is_int: bool, is_duration: bool = False +) -> list[_parameter_pb2.Validation]: validations: list[_parameter_pb2.Validation] = [] for constraint in info.metadata: if isinstance(constraint, annotated_types.Gt): validations.append( _parameter_pb2.Validation( type=_parameter_pb2.Validation.TYPE_GREATER_THAN, - value=str(constraint.gt), + value=_format_bound(constraint.gt, is_duration=is_duration), ) ) elif isinstance(constraint, annotated_types.Lt): validations.append( _parameter_pb2.Validation( type=_parameter_pb2.Validation.TYPE_LESS_THAN, - value=str(constraint.lt), + value=_format_bound(constraint.lt, is_duration=is_duration), ) ) elif isinstance(constraint, annotated_types.Ge): + ge_value = _approximate_ge_as_gt(constraint.ge, is_int=is_int, is_duration=is_duration) validations.append( _parameter_pb2.Validation( type=_parameter_pb2.Validation.TYPE_GREATER_THAN, - value=_approximate_ge_as_gt(constraint.ge, is_int=is_int), + value=ge_value, ) ) elif isinstance(constraint, annotated_types.Le): + le_value = _approximate_le_as_lt(constraint.le, is_int=is_int, is_duration=is_duration) validations.append( _parameter_pb2.Validation( type=_parameter_pb2.Validation.TYPE_LESS_THAN, - value=_approximate_le_as_lt(constraint.le, is_int=is_int), + value=le_value, ) ) else: @@ -285,31 +328,50 @@ def _constraint_validations(info: FieldInfo, *, is_int: bool) -> list[_parameter return validations -def _approximate_ge_as_gt(ge: Any, *, is_int: bool) -> str: +def _format_bound(value: Any, *, is_duration: bool) -> str: + """Format an exact (``gt=``/``lt=``) bound value for the wire. + + ``timedelta`` bounds use :func:`format_go_duration` (Go duration + syntax); everything else uses plain ``str()``. + """ + if is_duration: + return format_go_duration(value) + return str(value) + + +def _approximate_ge_as_gt(ge: Any, *, is_int: bool, is_duration: bool = False) -> str: """Approximate an inclusive ``ge=`` bound as the wire's exclusive ``gt``. ``ge`` is typed ``Any`` because ``annotated_types.Ge.ge`` is itself typed against a ``SupportsGe`` structural protocol, not a concrete numeric type -- in practice pydantic only ever populates it from - ``Field(ge=...)``, which authors pass an ``int``/``float``. - - Exact for ``int``-typed fields (``ge - 1`` admits exactly the same - integers as ``ge`` would inclusively). For ``float``-typed fields this - nudges the boundary down by :data:`_FLOAT_BOUNDARY_EPSILON`, which is an - approximation: values within that epsilon of ``ge`` are handled - correctly, but this is not bit-exact inclusive-boundary semantics. + ``Field(ge=...)``, which authors pass an ``int``/``float``/``timedelta``. + + Exact for ``int``-typed **and** ``timedelta``-typed fields (both are + fundamentally discrete types at the resolution that matters here -- + whole integers, or whole microseconds -- so ``ge - 1 unit`` admits + exactly the same values ``ge`` would inclusively). For ``float``-typed + fields this nudges the boundary down by :data:`_FLOAT_BOUNDARY_EPSILON`, + which is an approximation: values within that epsilon of ``ge`` are + handled correctly, but this is not bit-exact inclusive-boundary + semantics. """ + if is_duration: + return format_go_duration(ge - datetime.timedelta(microseconds=1)) if is_int: return str(int(ge) - 1) return repr(float(ge) - _FLOAT_BOUNDARY_EPSILON) -def _approximate_le_as_lt(le: Any, *, is_int: bool) -> str: +def _approximate_le_as_lt(le: Any, *, is_int: bool, is_duration: bool = False) -> str: """Approximate an inclusive ``le=`` bound as the wire's exclusive ``lt``. See :func:`_approximate_ge_as_gt` for why ``le`` is typed ``Any`` -- - exact for ``int``, epsilon-nudged approximation for ``float``. + exact for ``int``/``timedelta``, epsilon-nudged approximation for + ``float``. """ + if is_duration: + return format_go_duration(le + datetime.timedelta(microseconds=1)) if is_int: return str(int(le) + 1) return repr(float(le) + _FLOAT_BOUNDARY_EPSILON) @@ -334,9 +396,156 @@ def _format_default(value: Any) -> str: return str(value) +# Go duration units, ordered by microsecond magnitude, smallest first -- +# used only by the parser below (the formatter picks units explicitly). +# `Fraction` keeps arithmetic exact (no float rounding) while accumulating +# a parsed duration string's components before the final round-to-nearest- +# microsecond conversion into a `datetime.timedelta` (which itself has no +# finer resolution than microseconds -- matching Go's `ns` precision +# exactly isn't possible in a stdlib `timedelta`, and isn't needed for +# connector config durations). +_GO_DURATION_UNIT_TO_MICROSECONDS: dict[str, Fraction] = { + "ns": Fraction(1, 1000), + "us": Fraction(1), + "µs": Fraction(1), # µs, Go's own preferred spelling + "ms": Fraction(1000), + "s": Fraction(1_000_000), + "m": Fraction(60_000_000), + "h": Fraction(3_600_000_000), +} +_GO_DURATION_COMPONENT_RE = re.compile(r"([0-9]*\.?[0-9]+)(ns|µs|us|ms|s|m|h)") + + +def format_go_duration(value: datetime.timedelta) -> str: + """Render a ``timedelta`` as a Go ``time.Duration.String()``-syntax string. + + Matches Go's own formatting rules for the common cases this SDK's + config fields need (design doc §2.2's Duration A-gap, now closed): + microseconds/milliseconds/seconds for sub-minute durations, and + ``[Xh][Ym]Zs`` for durations of a minute or more (hours omitted if + zero; minutes shown whenever hours are, or whenever there are any, + matching e.g. ``time.Duration(time.Hour).String() == "1h0m0s"`` and + ``(90 * time.Second).String() == "1m30s"``). Round-trips exactly + through :func:`parse_go_duration` for any value a ``timedelta`` can + represent (microsecond resolution). + + Args: + value: the duration to format. + + Returns: + A Go-duration-syntax string, e.g. ``"5s"``, ``"1h30m"``... except + this function always includes a trailing seconds component for + the ``>= 1 minute`` branch (``"1h30m0s"``, not ``"1h30m"``) -- + matching Go's own ``String()`` output exactly, which always prints + seconds. + """ + total_us = value.days * 86_400_000_000 + value.seconds * 1_000_000 + value.microseconds + if total_us == 0: + return "0s" + + sign = "-" if total_us < 0 else "" + total_us = abs(total_us) + + if total_us < 1_000: + return f"{sign}{total_us}µs" + if total_us < 1_000_000: + return f"{sign}{_format_scaled(total_us, 1_000)}ms" + if total_us < 60_000_000: + return f"{sign}{_format_scaled(total_us, 1_000_000)}s" + + total_seconds, sub_second_us = divmod(total_us, 1_000_000) + hours, remainder = divmod(total_seconds, 3600) + minutes, seconds = divmod(remainder, 60) + seconds_us = seconds * 1_000_000 + sub_second_us + + parts: list[str] = [] + if hours: + parts.append(f"{hours}h") + if hours or minutes: + parts.append(f"{minutes}m") + parts.append(f"{_format_scaled(seconds_us, 1_000_000)}s") + return sign + "".join(parts) + + +def _format_scaled(total: int, unit: int) -> str: + """Format ``total`` (an integer count of the base unit) scaled by ``unit``. + + E.g. ``_format_scaled(1500, 1000)`` (1500 microseconds, scaled to + milliseconds) -> ``"1.5"``. Uses exact integer arithmetic throughout + (no floats), so there's no rounding-representation mismatch between + what's formatted and what :func:`parse_go_duration` reads back. + """ + whole, rem = divmod(total, unit) + if rem == 0: + return str(whole) + digits = len(str(unit)) - 1 + frac = str(rem).rjust(digits, "0").rstrip("0") + return f"{whole}.{frac}" + + +def parse_go_duration(value: str) -> datetime.timedelta: + """Parse a Go ``time.Duration``-syntax string into a ``timedelta``. + + Accepts a signed sequence of ```` components (each + number may have a fractional part), e.g. ``"5s"``, ``"1h30m"``, + ``"500ms"``, ``"-1.5h"``, ``"2h45m30s"``, ``"90s"``. Units: ``ns``, + ``us``/``µs``, ``ms``, ``s``, ``m``, ``h`` -- matching Go's + ``ParseDuration``. A bare ``"0"`` (no unit) is accepted as zero, + matching Go. Arithmetic is done with exact ``fractions.Fraction`` + values (not floats) until the final round to the nearest whole + microsecond, the finest resolution ``datetime.timedelta`` supports. + + Args: + value: a Go-duration-syntax string. + + Returns: + The equivalent ``timedelta``. + + Raises: + ValueError: if ``value`` isn't valid Go duration syntax. + """ + original = value + s = value.strip() + if not s: + raise ValueError(f"invalid duration {original!r}: empty string") + + sign = 1 + if s[0] in "+-": + sign = -1 if s[0] == "-" else 1 + s = s[1:] + + if s == "0": + return datetime.timedelta(0) + + total = Fraction(0) + pos = 0 + matched_any = False + for match in _GO_DURATION_COMPONENT_RE.finditer(s): + if match.start() != pos: + raise ValueError( + f"invalid duration {original!r}: unexpected characters at " + f"position {pos} (Go duration syntax, e.g. '5s', '1h30m', '500ms')" + ) + number = Fraction(match.group(1)) + unit = match.group(2) + total += number * _GO_DURATION_UNIT_TO_MICROSECONDS[unit] + pos = match.end() + matched_any = True + + if not matched_any or pos != len(s): + raise ValueError( + f"invalid duration {original!r}: does not match Go duration syntax " + "(e.g. '5s', '1h30m', '500ms', 'us'/'µs', 'ns')" + ) + + return datetime.timedelta(microseconds=sign * round(total)) + + __all__ = [ "BaseConfig", "Field", "Specification", + "format_go_duration", + "parse_go_duration", "to_parameters", ] diff --git a/tests/test_config.py b/tests/test_config.py index 2cdb366..e990da8 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,8 +1,10 @@ """Tests for :mod:`conduit.config` -- ``BaseConfig``/``Field``/``to_parameters``. -Covers the design doc §2.2 mapping rules and its documented A-gaps -(``TYPE_DURATION``/``TYPE_EXCLUSION`` raise ``NotImplementedError`` rather -than guessing). +Covers the design doc §2.2 mapping rules. ``TYPE_DURATION`` (``timedelta`` +fields) is now a real, round-tripping mapping -- see +``tests/test_go_duration.py`` for the ``format_go_duration``/ +``parse_go_duration`` unit tests specifically. ``TYPE_EXCLUSION`` remains an +open A-gap and still raises ``NotImplementedError``. """ from __future__ import annotations @@ -41,7 +43,11 @@ class _WithBool(BaseConfig): class _WithDuration(BaseConfig): - interval: datetime.timedelta = Field(default=datetime.timedelta(seconds=5)) + interval: datetime.timedelta = Field( + default=datetime.timedelta(seconds=5), description="poll interval" + ) + timeout: datetime.timedelta = Field(description="required timeout, no default") + long_wait: datetime.timedelta = Field(default=datetime.timedelta(hours=1, minutes=30)) def test_required_field_gets_required_validation() -> None: @@ -116,10 +122,45 @@ def test_bool_field_maps_to_type_bool_with_lowercase_default() -> None: assert param.default == "false" -def test_duration_field_raises_not_implemented() -> None: - """§2.2's A-gap: TYPE_DURATION has no pydantic-native mapping -- raise, don't guess.""" - with pytest.raises(NotImplementedError, match=r"TYPE_DURATION|duration"): - to_parameters(_WithDuration) +def test_duration_field_maps_to_type_duration_with_go_syntax_default() -> None: + """A ``timedelta`` field maps to ``TYPE_DURATION``; the default is Go-duration syntax.""" + params = to_parameters(_WithDuration) + interval = params["interval"] + assert interval.type == parameter_pb2.Parameter.TYPE_DURATION + assert interval.default == "5s" + assert interval.description == "poll interval" + + long_wait = params["long_wait"] + assert long_wait.default == "1h30m0s" + + +def test_duration_field_with_no_default_is_required_and_has_empty_default() -> None: + params = to_parameters(_WithDuration) + timeout = params["timeout"] + assert timeout.type == parameter_pb2.Parameter.TYPE_DURATION + assert timeout.default == "" + types = [v.type for v in timeout.validations] + assert parameter_pb2.Validation.TYPE_REQUIRED in types + + +def test_configure_parses_go_duration_string_config_value() -> None: + """The ``Configure`` RPC's string config map parses Go-duration syntax for timedelta fields.""" + config = _WithDuration.model_validate( + {"interval": "10s", "timeout": "1h30m", "long_wait": "2h"} + ) + assert config.interval == datetime.timedelta(seconds=10) + assert config.timeout == datetime.timedelta(hours=1, minutes=30) + assert config.long_wait == datetime.timedelta(hours=2) + + +def test_direct_construction_with_a_real_timedelta_still_works() -> None: + """Non-string (already-``timedelta``) values pass through the before-validator unchanged.""" + config = _WithDuration( + interval=datetime.timedelta(seconds=1), + timeout=datetime.timedelta(minutes=5), + ) + assert config.interval == datetime.timedelta(seconds=1) + assert config.timeout == datetime.timedelta(minutes=5) def test_exclusion_request_raises_not_implemented() -> None: diff --git a/tests/test_go_duration.py b/tests/test_go_duration.py new file mode 100644 index 0000000..45119bb --- /dev/null +++ b/tests/test_go_duration.py @@ -0,0 +1,122 @@ +"""Tests for :func:`conduit.config.format_go_duration`/``parse_go_duration``. + +The Go-duration mapping (design doc §2.2's Duration A-gap, now closed): +``config.Parameter.Type.TYPE_DURATION`` fields serialize/parse using Go's +``time.Duration.String()``/``ParseDuration`` syntax (``"5s"``, ``"1h30m"``, +``"500ms"``), not ISO-8601. These tests cover known Go-format examples, +round-trip identity (including via Hypothesis over arbitrary microsecond +counts), and malformed-input rejection. +""" + +from __future__ import annotations + +import datetime + +import pytest +from hypothesis import given +from hypothesis import strategies as st + +from conduit.config import format_go_duration, parse_go_duration + +# Known-good (string, timedelta) pairs matching Go's actual `String()` output +# for the same duration -- these pin compatibility with real Go tooling, not +# just internal self-consistency. +_KNOWN_GO_FORMATS = [ + (datetime.timedelta(0), "0s"), + (datetime.timedelta(microseconds=500), "500µs"), + (datetime.timedelta(milliseconds=500), "500ms"), + (datetime.timedelta(microseconds=1500), "1.5ms"), + (datetime.timedelta(milliseconds=1500), "1.5s"), + (datetime.timedelta(seconds=1), "1s"), + (datetime.timedelta(seconds=45), "45s"), + (datetime.timedelta(seconds=90), "1m30s"), + (datetime.timedelta(minutes=1), "1m0s"), + (datetime.timedelta(hours=1), "1h0m0s"), + (datetime.timedelta(hours=1, minutes=30), "1h30m0s"), + (datetime.timedelta(hours=2, minutes=45, seconds=30), "2h45m30s"), + (-datetime.timedelta(seconds=5), "-5s"), +] + + +@pytest.mark.parametrize(("value", "expected"), _KNOWN_GO_FORMATS) +def test_format_matches_known_go_output(value: datetime.timedelta, expected: str) -> None: + assert format_go_duration(value) == expected + + +@pytest.mark.parametrize(("value", "expected"), _KNOWN_GO_FORMATS) +def test_format_then_parse_round_trips(value: datetime.timedelta, expected: str) -> None: + assert parse_go_duration(format_go_duration(value)) == value + + +class TestParseAcceptsGoSyntaxVariants: + def test_plain_seconds(self) -> None: + assert parse_go_duration("5s") == datetime.timedelta(seconds=5) + + def test_combined_hours_minutes(self) -> None: + assert parse_go_duration("1h30m") == datetime.timedelta(hours=1, minutes=30) + + def test_milliseconds(self) -> None: + assert parse_go_duration("500ms") == datetime.timedelta(milliseconds=500) + + def test_fractional_hours(self) -> None: + assert parse_go_duration("1.5h") == datetime.timedelta(hours=1.5) + + def test_combined_hours_minutes_seconds(self) -> None: + assert parse_go_duration("2h45m30s") == datetime.timedelta(hours=2, minutes=45, seconds=30) + + def test_negative(self) -> None: + assert parse_go_duration("-1.5h") == -datetime.timedelta(hours=1.5) + + def test_explicit_plus_sign(self) -> None: + assert parse_go_duration("+5s") == datetime.timedelta(seconds=5) + + def test_microseconds_ascii_spelling(self) -> None: + assert parse_go_duration("500us") == datetime.timedelta(microseconds=500) + + def test_microseconds_mu_spelling(self) -> None: + assert parse_go_duration("500µs") == datetime.timedelta(microseconds=500) + + def test_bare_zero_with_no_unit(self) -> None: + assert parse_go_duration("0") == datetime.timedelta(0) + + def test_surrounding_whitespace_is_tolerated(self) -> None: + assert parse_go_duration(" 5s ") == datetime.timedelta(seconds=5) + + +class TestParseRejectsInvalidSyntax: + def test_empty_string(self) -> None: + with pytest.raises(ValueError, match="empty"): + parse_go_duration("") + + def test_number_with_no_unit(self) -> None: + with pytest.raises(ValueError, match="Go duration syntax"): + parse_go_duration("5") + + def test_unit_with_no_number(self) -> None: + with pytest.raises(ValueError, match="Go duration syntax"): + parse_go_duration("s") + + def test_unknown_unit(self) -> None: + with pytest.raises(ValueError, match="Go duration syntax"): + parse_go_duration("5x") + + def test_garbage(self) -> None: + with pytest.raises(ValueError, match="Go duration syntax"): + parse_go_duration("not-a-duration") + + def test_gap_between_components(self) -> None: + with pytest.raises(ValueError, match="unexpected characters"): + parse_go_duration("5s garbage 3m") + + +@given( + total_us=st.integers( + min_value=-int(1e12), # roughly -11.5 days, comfortably within timedelta range + max_value=int(1e12), + ) +) +def test_format_parse_round_trip_property(total_us: int) -> None: + """For any representable microsecond count, format -> parse recovers it exactly.""" + value = datetime.timedelta(microseconds=total_us) + formatted = format_go_duration(value) + assert parse_go_duration(formatted) == value From ac1e90e45c2f366537b192a1a53db9fff0c95606 Mon Sep 17 00:00:00 2001 From: DeVaris Brown Date: Thu, 23 Jul 2026 16:16:46 -0400 Subject: [PATCH 6/9] feat(source,destination): rename lifecycle hooks, clarify ack(), forward validation detail DX audit fixes #2-#4. - Source.ack()'s default was already a genuine no-op (return None, no log line, no raise) -- verified, and its docstring now says explicitly: "you don't need to override ack() unless you're acknowledging against the source system itself (e.g. committing a Kafka offset, deleting a queue message)." Same note added to the example connector's README. - Renamed lifecycle_on_created/lifecycle_on_updated/lifecycle_on_deleted to on_created/on_updated/on_deleted on both Source and Destination -- the lifecycle_ prefix was redundant (these already live on the connector class). Updated the ABCs, the servicer dispatch code, and tests/test_destination.py. Grepped for every lifecycle_on_ reference (including _dispatch.py/_introspect.py, which had none) to confirm no stragglers. - BatchWriteError.partial(batch_size, written=N, cause=exc): the recommended constructor for the common contiguous-prefix partial-batch case. Every failed index is recorded with the real cause exception instead of a generic "not reached" placeholder, so the ack error detail that reaches Conduit reflects what actually went wrong. Reuses the existing exhaustive-accounting constructor path under the hood -- still no code path that computes "ack everything not explicitly failed." - Configure RPC handlers (Source and Destination) now catch pydantic.ValidationError explicitly and abort with INVALID_ARGUMENT plus a per-field detail message (errors.format_validation_error()), instead of relying on grpc.aio's generic "Unexpected : ..." UNKNOWN-status wrapping of an uncaught exception -- per CLAUDE.md's "errors are API, actionable" standard. Tests: tests/test_errors.py (BatchWriteError.partial construction + cause-propagation), tests/test_configure_errors.py (real grpc.aio server + real client, asserting INVALID_ARGUMENT status and that the field name/ message actually appear in the gRPC status detail -- not a mock of pydantic or of the transport). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD --- examples/http-poll-source/README.md | 13 +++ src/conduit/destination.py | 46 +++++++---- src/conduit/errors.py | 97 +++++++++++++++++++++-- src/conduit/source.py | 47 ++++++++--- tests/test_configure_errors.py | 118 ++++++++++++++++++++++++++++ tests/test_destination.py | 6 +- tests/test_errors.py | 45 +++++++++++ 7 files changed, 337 insertions(+), 35 deletions(-) create mode 100644 tests/test_configure_errors.py diff --git a/examples/http-poll-source/README.md b/examples/http-poll-source/README.md index 2dade09..8ccf12a 100644 --- a/examples/http-poll-source/README.md +++ b/examples/http-poll-source/README.md @@ -16,3 +16,16 @@ This is both the worked example *and* will become the source for `conduit connector new --lang python`'s scaffolded template (Phase 3), per the design doc's reasoning for reusing one connector for both rather than building a separate template repo before the SDK API has stabilized. + +## Notes for connector authors + +- **You don't need to override `Source.ack()`** unless you're also + acknowledging against the source system itself (e.g. committing a Kafka + consumer offset, deleting a queue message, marking a row processed + upstream). This example doesn't override it -- Conduit's own + position-based resume (via `open(position)`) is enough for an HTTP + polling source with no upstream ack concept. +- Build a standalone, directly-executable artifact for this connector with + `conduit-connector-sdk build examples/http-poll-source -o http-poll-source` + -- see the root [`README.md`](../../README.md#building-a-standalone-connector-artifact) + for why this is required (not just convenient) for Conduit to launch it. diff --git a/src/conduit/destination.py b/src/conduit/destination.py index c0bfe82..08b7ae6 100644 --- a/src/conduit/destination.py +++ b/src/conduit/destination.py @@ -26,12 +26,16 @@ from collections.abc import AsyncIterator, Sequence from typing import Any, Generic, TypeVar +import grpc +import grpc.aio +import pydantic + import conduit._grpc # noqa: F401 -- sets up sys.path, see conduit._grpc.__init__ from conduit._dispatch import invoke from conduit._grpc.adapters import config_map_from_proto, records_from_proto from conduit._introspect import resolve_config_class from conduit.config import BaseConfig -from conduit.errors import BatchWriteError +from conduit.errors import BatchWriteError, format_validation_error from conduit.record import Record from connector.v2 import destination_pb2, destination_pb2_grpc @@ -74,9 +78,10 @@ async def write(self, records: list[Record]) -> None: """Durably write every record in ``records``, in order. Full success is "returns without raising." A partial-batch failure - raises :class:`~conduit.errors.BatchWriteError` with an exhaustive - accounting of which indices succeeded and which failed -- see that - class's docstring for the exact construction contract (the B1 fix). + raises :class:`~conduit.errors.BatchWriteError` -- typically via + ``raise BatchWriteError.partial(len(records), written=N, cause=exc)``, + the recommended constructor (see :meth:`~conduit.errors.BatchWriteError.partial`), + rather than hand-building the exhaustive index accounting yourself. Any other exception is treated by the SDK's adapter as a failure of the **entire** batch (see :meth:`_DestinationServicer._write_batch`) -- there is no partial-credit interpretation of a plain exception. @@ -97,17 +102,15 @@ async def teardown(self) -> None: """Called once, after the write loop stops, before process exit. Default: no-op.""" return None - async def lifecycle_on_created(self, config: dict[str, str]) -> None: + async def on_created(self, config: dict[str, str]) -> None: """Called once, the first time this connector instance is ever run. Default: no-op.""" return None - async def lifecycle_on_updated( - self, config_before: dict[str, str], config_after: dict[str, str] - ) -> None: + async def on_updated(self, config_before: dict[str, str], config_after: dict[str, str]) -> None: """Called when the connector's configuration changed since the last run. Default: no-op.""" return None - async def lifecycle_on_deleted(self, config: dict[str, str]) -> None: + async def on_deleted(self, config: dict[str, str]) -> None: """Called once, when this connector instance was deleted. Default: no-op.""" return None @@ -137,10 +140,23 @@ def __init__(self, destination: Destination[Any], config_cls: type[BaseConfig]) self._config_cls = config_cls async def Configure( - self, request: destination_pb2.Destination.Configure.Request, context: object + self, + request: destination_pb2.Destination.Configure.Request, + context: grpc.aio.ServicerContext[Any, Any], ) -> destination_pb2.Destination.Configure.Response: - """Validate and store the plugin's config.""" - config = self._config_cls.model_validate(config_map_from_proto(request.config)) + """Validate and store the plugin's config. + + A ``pydantic.ValidationError`` is caught explicitly and turned into + an ``INVALID_ARGUMENT`` status with a per-field detail message + (:func:`~conduit.errors.format_validation_error`) -- see + :meth:`conduit.source._SourceServicer.Configure` for the same + rationale (kept in sync with this one). + """ + try: + config = self._config_cls.model_validate(config_map_from_proto(request.config)) + except pydantic.ValidationError as exc: + await context.abort(grpc.StatusCode.INVALID_ARGUMENT, format_validation_error(exc)) + raise # pragma: no cover -- abort() never returns; unreachable, satisfies mypy await invoke(self._destination.configure, config) return destination_pb2.Destination.Configure.Response() @@ -248,7 +264,7 @@ async def LifecycleOnCreated( self, request: destination_pb2.Destination.Lifecycle.OnCreated.Request, context: object ) -> destination_pb2.Destination.Lifecycle.OnCreated.Response: """Dispatch the connector's first-run lifecycle hook.""" - await invoke(self._destination.lifecycle_on_created, config_map_from_proto(request.config)) + await invoke(self._destination.on_created, config_map_from_proto(request.config)) return destination_pb2.Destination.Lifecycle.OnCreated.Response() async def LifecycleOnUpdated( @@ -256,7 +272,7 @@ async def LifecycleOnUpdated( ) -> destination_pb2.Destination.Lifecycle.OnUpdated.Response: """Dispatch the connector's config-changed lifecycle hook.""" await invoke( - self._destination.lifecycle_on_updated, + self._destination.on_updated, config_map_from_proto(request.config_before), config_map_from_proto(request.config_after), ) @@ -266,7 +282,7 @@ async def LifecycleOnDeleted( self, request: destination_pb2.Destination.Lifecycle.OnDeleted.Request, context: object ) -> destination_pb2.Destination.Lifecycle.OnDeleted.Response: """Dispatch the connector's deleted lifecycle hook.""" - await invoke(self._destination.lifecycle_on_deleted, config_map_from_proto(request.config)) + await invoke(self._destination.on_deleted, config_map_from_proto(request.config)) return destination_pb2.Destination.Lifecycle.OnDeleted.Response() diff --git a/src/conduit/errors.py b/src/conduit/errors.py index bd124cf..ffe1f73 100644 --- a/src/conduit/errors.py +++ b/src/conduit/errors.py @@ -13,6 +13,8 @@ from collections.abc import Mapping, Set +import pydantic + class ConnectorError(Exception): """Base exception for connector-raised errors surfaced over the wire. @@ -43,6 +45,36 @@ def __init__(self, message: str, *, code: str | None = None) -> None: self.code = code +def format_validation_error(exc: pydantic.ValidationError) -> str: + """Format a pydantic ``ValidationError`` as a concise, per-field detail string. + + Used by the ``Configure`` RPC handlers (:mod:`conduit.source`/ + :mod:`conduit.destination`) to build the gRPC ``INVALID_ARGUMENT`` + status detail explicitly, rather than relying on ``grpc.aio``'s own + generic "Unexpected : ..." wrapping of an uncaught + exception (``StatusCode.UNKNOWN``) -- per ``CLAUDE.md``'s "errors are + API, actionable" standard, an author (or Conduit's own error surface) + should be able to see exactly which field failed and why, not an + opaque blob, and the SDK should own that contract explicitly rather + than depend on incidental library formatting. + + Args: + exc: the validation error to format. + + Returns: + A multi-line string: one summary line, then one ``: + `` line per error. Omits pydantic's "For further + information visit ..." doc-link lines (``include_url=False``) -- + noise in a gRPC status detail, not useful to an operator reading a + pipeline's error log. + """ + lines = [f"invalid config ({exc.error_count()} error(s)):"] + for error in exc.errors(include_url=False): + loc = ".".join(str(part) for part in error["loc"]) or "" + lines.append(f" {loc}: {error['msg']}") + return "\n".join(lines) + + class BackoffRetry(ConnectorError): """Raised by ``Source.read()`` to mean "no record right now, retry". @@ -84,14 +116,21 @@ class BatchWriteError(ConnectorError): supplied, in full, at construction time, or ``__init__`` raises ``ValueError``. - Two ways to construct it: + Three ways to construct it, from most to least recommended: - 1. ``BatchWriteError(batch_size, written=N)`` -- the common case, a - contiguous success prefix (Go's ``n``): "everything up to index - ``N - 1`` succeeded, everything from ``N`` on failed." Every index - ``>= N`` is recorded as a generic failure. - 2. ``BatchWriteError(batch_size, success={...}, failures={...})`` -- an - explicit, non-contiguous accounting. ``success`` and ``failures`` + 1. **``BatchWriteError.partial(batch_size, written=N, cause=exc)``** -- + the recommended way to raise the common case (a contiguous success + prefix, Go's ``n``): "everything up to index ``N - 1`` succeeded, + everything from ``N`` on failed because of ``exc``." Every index + ``>= N`` is recorded as failed with your real ``cause`` exception, + not a generic placeholder -- see :meth:`partial`. + 2. ``BatchWriteError(batch_size, written=N)`` -- same contiguous-prefix + shape without a specific cause; failed indices get a generic + internal message. Use :meth:`partial` instead when you have the + real exception that stopped the write. + 3. ``BatchWriteError(batch_size, success={...}, failures={...})`` -- an + explicit, non-contiguous accounting, for the less common case where + failures aren't a simple prefix. ``success`` and ``failures`` together must cover every index in ``range(batch_size)`` exactly once (no gaps, no overlaps). @@ -208,3 +247,47 @@ def __init__( if summary else "partial batch write failure" ) + + @classmethod + def partial(cls, batch_size: int, *, written: int, cause: BaseException) -> BatchWriteError: + """Construct the common contiguous-prefix case, with a real cause. + + This is the **recommended** way to raise a partial-batch failure: + ``raise BatchWriteError.partial(len(records), written=3, cause=exc)``. + Equivalent to ``BatchWriteError(batch_size, written=written)``, + except every index past the written prefix is recorded as having + failed with ``cause`` itself -- the real exception your ``write()`` + caught -- rather than a generic internal "not reached" placeholder. + This means the ack's error detail that eventually reaches Conduit + (and whoever's reading the pipeline's error log) reflects what + actually went wrong, not just that something did. + + Authors never hand-build the ``success``/``failures`` + set/mapping for this common case -- this classmethod does it, + reusing the exhaustive-accounting constructor path (already + validated) under the hood. + + Args: + batch_size: number of records in the batch -- typically + ``len(records)`` from your ``write(self, records)``. + written: contiguous success-prefix count (Go's ``n``): + indices ``[0, written)`` succeeded. + cause: the exception that caused the write to stop after + ``written`` records. Recorded as every index ``>= written``'s + failure reason. + + Returns: + A fully validated, ready-to-raise ``BatchWriteError``. + + Raises: + ValueError: if ``written`` is out of range for ``batch_size`` + (see :meth:`__init__`). + """ + if not 0 <= written <= batch_size: + raise ValueError( + f"BatchWriteError.partial: written={written} is out of range for " + f"batch_size={batch_size} (must satisfy 0 <= written <= batch_size)" + ) + success = set(range(written)) + failures: dict[int, BaseException] = dict.fromkeys(range(written, batch_size), cause) + return cls(batch_size, success=success, failures=failures) diff --git a/src/conduit/source.py b/src/conduit/source.py index 5cd7218..2b19ce2 100644 --- a/src/conduit/source.py +++ b/src/conduit/source.py @@ -17,12 +17,16 @@ from collections.abc import AsyncIterator, Mapping from typing import Any, Generic, TypeVar +import grpc +import grpc.aio +import pydantic + import conduit._grpc # noqa: F401 -- sets up sys.path, see conduit._grpc.__init__ from conduit._dispatch import invoke from conduit._grpc.adapters import config_map_from_proto, record_to_proto from conduit._introspect import resolve_config_class from conduit.config import BaseConfig -from conduit.errors import BackoffRetry +from conduit.errors import BackoffRetry, format_validation_error from conduit.record import Record from connector.v2 import source_pb2, source_pb2_grpc @@ -117,6 +121,14 @@ async def ack(self, position: bytes) -> None: :class:`_SourceServicer._consume_acks`, which is the sole caller), never speculatively when a record is merely produced. + **You don't need to override this** unless you also need to + acknowledge against the source system itself -- e.g. committing a + Kafka consumer offset, deleting a message from a queue, or marking + a row processed in an upstream system. Conduit's own position + tracking (via what ``read()``/``open()`` return and resume from) + works correctly with the no-op default; most connectors never + override ``ack()``. + Args: position: the acknowledged record's position. """ @@ -126,7 +138,7 @@ async def teardown(self) -> None: """Called once, after the read loop stops, before process exit. Default: no-op.""" return None - async def lifecycle_on_created(self, config: Mapping[str, str]) -> None: + async def on_created(self, config: Mapping[str, str]) -> None: """Called once, the first time this connector instance is ever run. Default: no-op. Args: @@ -136,7 +148,7 @@ async def lifecycle_on_created(self, config: Mapping[str, str]) -> None: """ return None - async def lifecycle_on_updated( + async def on_updated( self, config_before: Mapping[str, str], config_after: Mapping[str, str] ) -> None: """Called when the connector's configuration changed since the last run. Default: no-op. @@ -147,7 +159,7 @@ async def lifecycle_on_updated( """ return None - async def lifecycle_on_deleted(self, config: Mapping[str, str]) -> None: + async def on_deleted(self, config: Mapping[str, str]) -> None: """Called once, when this connector instance was deleted. Default: no-op. Args: @@ -224,10 +236,25 @@ def __init__(self, source: Source[Any], config_cls: type[BaseConfig]) -> None: self._last_position: bytes = b"" async def Configure( - self, request: source_pb2.Source.Configure.Request, context: object + self, + request: source_pb2.Source.Configure.Request, + context: grpc.aio.ServicerContext[Any, Any], ) -> source_pb2.Source.Configure.Response: - """Validate and store the plugin's config. See proto doc comment for RPC semantics.""" - config = self._config_cls.model_validate(config_map_from_proto(request.config)) + """Validate and store the plugin's config. See proto doc comment for RPC semantics. + + A ``pydantic.ValidationError`` is caught explicitly and turned into + an ``INVALID_ARGUMENT`` status with a per-field detail message + (:func:`~conduit.errors.format_validation_error`) -- per + ``CLAUDE.md``'s "errors are API" standard, an author should see + exactly which field failed and why, not ``grpc.aio``'s generic + "Unexpected : ..." ``UNKNOWN``-status wrapping of + an uncaught exception. + """ + try: + config = self._config_cls.model_validate(config_map_from_proto(request.config)) + except pydantic.ValidationError as exc: + await context.abort(grpc.StatusCode.INVALID_ARGUMENT, format_validation_error(exc)) + raise # pragma: no cover -- abort() never returns; unreachable, satisfies mypy await invoke(self._source.configure, config) return source_pb2.Source.Configure.Response() @@ -320,7 +347,7 @@ async def LifecycleOnCreated( self, request: source_pb2.Source.Lifecycle.OnCreated.Request, context: object ) -> source_pb2.Source.Lifecycle.OnCreated.Response: """Dispatch the connector's first-run lifecycle hook.""" - await invoke(self._source.lifecycle_on_created, config_map_from_proto(request.config)) + await invoke(self._source.on_created, config_map_from_proto(request.config)) return source_pb2.Source.Lifecycle.OnCreated.Response() async def LifecycleOnUpdated( @@ -328,7 +355,7 @@ async def LifecycleOnUpdated( ) -> source_pb2.Source.Lifecycle.OnUpdated.Response: """Dispatch the connector's config-changed lifecycle hook.""" await invoke( - self._source.lifecycle_on_updated, + self._source.on_updated, config_map_from_proto(request.config_before), config_map_from_proto(request.config_after), ) @@ -338,7 +365,7 @@ async def LifecycleOnDeleted( self, request: source_pb2.Source.Lifecycle.OnDeleted.Request, context: object ) -> source_pb2.Source.Lifecycle.OnDeleted.Response: """Dispatch the connector's deleted lifecycle hook.""" - await invoke(self._source.lifecycle_on_deleted, config_map_from_proto(request.config)) + await invoke(self._source.on_deleted, config_map_from_proto(request.config)) return source_pb2.Source.Lifecycle.OnDeleted.Response() diff --git a/tests/test_configure_errors.py b/tests/test_configure_errors.py new file mode 100644 index 0000000..2001169 --- /dev/null +++ b/tests/test_configure_errors.py @@ -0,0 +1,118 @@ +"""Tests that ``Configure`` forwards pydantic validation detail over gRPC. + +Per CLAUDE.md's "errors are API, actionable" standard: a config validation +failure must surface which field failed and why, not collapse to an opaque +blob. This exercises the real path: a real ``grpc.aio`` server, a real gRPC +client, a real ``Configure`` RPC call with an invalid config map -- not a +mock of ``pydantic.ValidationError`` or of the transport. +""" + +from __future__ import annotations + +import grpc +import grpc.aio +import pytest + +import conduit._grpc # noqa: F401 -- sets up sys.path, see conduit._grpc.__init__ +from conduit.config import BaseConfig, Field, Specification +from conduit.destination import Destination +from conduit.errors import BackoffRetry +from conduit.record import Record +from conduit.serve import _build_plugin_server +from conduit.source import Source +from connector.v2 import destination_pb2, source_pb2 + +_SPEC = Specification(name="test-plugin", version="0.0.0", author="test") + + +class _Config(BaseConfig): + url: str = Field(description="a required field") + count: int = Field(default=1, description="an int field") + + +class _NoopSource(Source[_Config]): + async def read(self) -> Record: + raise BackoffRetry() + + +class _NoopDestination(Destination[_Config]): + async def write(self, records: list[Record]) -> None: + return None + + +async def _configure(port: int, service: str, config: dict[str, str]) -> None: + channel = grpc.aio.insecure_channel(f"127.0.0.1:{port}") + try: + if service == "source": + call = channel.unary_unary( + "/connector.v2.SourcePlugin/Configure", + request_serializer=source_pb2.Source.Configure.Request.SerializeToString, + response_deserializer=source_pb2.Source.Configure.Response.FromString, + ) + await call(source_pb2.Source.Configure.Request(config=config)) + else: + call = channel.unary_unary( + "/connector.v2.DestinationPlugin/Configure", + request_serializer=destination_pb2.Destination.Configure.Request.SerializeToString, + response_deserializer=destination_pb2.Destination.Configure.Response.FromString, + ) + await call(destination_pb2.Destination.Configure.Request(config=config)) + finally: + await channel.close() + + +class TestConfigureValidationErrorDetail: + async def test_source_missing_required_field_is_invalid_argument_with_field_detail( + self, + ) -> None: + handle = await _build_plugin_server(_SPEC, source=_NoopSource) + try: + with pytest.raises(grpc.aio.AioRpcError) as exc_info: + await _configure(handle.port, "source", {}) + error = exc_info.value + assert error.code() == grpc.StatusCode.INVALID_ARGUMENT + details = error.details() + assert details is not None + assert "url" in details + assert "required" in details.lower() or "missing" in details.lower() + finally: + handle.shutdown_requested.set() + await handle.drive_task + + async def test_source_wrong_type_is_invalid_argument_with_field_detail(self) -> None: + handle = await _build_plugin_server(_SPEC, source=_NoopSource) + try: + with pytest.raises(grpc.aio.AioRpcError) as exc_info: + await _configure(handle.port, "source", {"url": "x", "count": "not-an-int"}) + error = exc_info.value + assert error.code() == grpc.StatusCode.INVALID_ARGUMENT + details = error.details() + assert details is not None + assert "count" in details + finally: + handle.shutdown_requested.set() + await handle.drive_task + + async def test_destination_missing_required_field_is_invalid_argument_with_field_detail( + self, + ) -> None: + handle = await _build_plugin_server(_SPEC, destination=_NoopDestination) + try: + with pytest.raises(grpc.aio.AioRpcError) as exc_info: + await _configure(handle.port, "destination", {}) + error = exc_info.value + assert error.code() == grpc.StatusCode.INVALID_ARGUMENT + details = error.details() + assert details is not None + assert "url" in details + finally: + handle.shutdown_requested.set() + await handle.drive_task + + async def test_valid_config_does_not_raise(self) -> None: + handle = await _build_plugin_server(_SPEC, source=_NoopSource) + try: + await _configure(handle.port, "source", {"url": "https://example.com"}) + finally: + handle.shutdown_requested.set() + await handle.drive_task diff --git a/tests/test_destination.py b/tests/test_destination.py index 1119c8c..9e30fe5 100644 --- a/tests/test_destination.py +++ b/tests/test_destination.py @@ -180,9 +180,9 @@ async def write(self, records: list[Record]) -> None: instance = MyDestination() await instance.open() await instance.teardown() - await instance.lifecycle_on_created({}) - await instance.lifecycle_on_updated({}, {}) - await instance.lifecycle_on_deleted({}) + await instance.on_created({}) + await instance.on_updated({}, {}) + await instance.on_deleted({}) def test_destination_is_abstract_without_write() -> None: diff --git a/tests/test_errors.py b/tests/test_errors.py index 42fcc41..ca3ba2a 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -75,6 +75,51 @@ def test_both_written_and_accounting_raises_value_error(self) -> None: BatchWriteError(2, written=1, success={1}, failures={0: ValueError("x")}) +class TestBatchWriteErrorPartial: + """``BatchWriteError.partial()`` -- the recommended way to raise a partial-batch failure.""" + + def test_written_prefix_is_acked_the_rest_gets_the_real_cause(self) -> None: + cause = ConnectionError("destination went away mid-batch") + err = BatchWriteError.partial(5, written=2, cause=cause) + assert err.success == {0, 1} + assert set(err.failures) == {2, 3, 4} + # Every failed index carries the REAL cause, not a generic + # placeholder -- the whole point of `.partial()` over the plain + # `written=` constructor. + assert all(err.failures[i] is cause for i in (2, 3, 4)) + + def test_cause_message_reaches_str_of_the_error(self) -> None: + cause = TimeoutError("upstream timed out") + err = BatchWriteError.partial(3, written=1, cause=cause) + assert "upstream timed out" in str(err) + + def test_written_zero_means_nothing_succeeded(self) -> None: + cause = RuntimeError("boom") + err = BatchWriteError.partial(3, written=0, cause=cause) + assert err.success == set() + assert set(err.failures) == {0, 1, 2} + + def test_written_equal_to_batch_size_means_everything_succeeded(self) -> None: + cause = RuntimeError("unreachable in practice, but must not crash") + err = BatchWriteError.partial(3, written=3, cause=cause) + assert err.success == {0, 1, 2} + assert err.failures == {} + + def test_written_out_of_range_raises_value_error(self) -> None: + with pytest.raises(ValueError, match="out of range"): + BatchWriteError.partial(3, written=4, cause=RuntimeError("x")) + + def test_result_is_a_real_exhaustively_validated_batch_write_error(self) -> None: + """``.partial()`` reuses the same validated constructor path, not a shortcut around it.""" + err = BatchWriteError.partial(4, written=2, cause=RuntimeError("x")) + assert isinstance(err, BatchWriteError) + assert err.batch_size == 4 + # Exhaustiveness already proven by BatchWriteError.__init__ itself + # (see TestBatchWriteErrorExplicitAccounting) -- this just checks + # .partial() actually goes through that path rather than bypassing it. + assert err.success | set(err.failures) == {0, 1, 2, 3} + + class TestBackoffRetry: def test_is_a_connector_error(self) -> None: assert isinstance(BackoffRetry(), ConnectorError) From 392e3e4da4f1efb06983fedf62701f7c2ec70b6b Mon Sep 17 00:00:00 2001 From: DeVaris Brown Date: Thu, 23 Jul 2026 16:17:13 -0400 Subject: [PATCH 7/9] feat(cli): conduit-connector-sdk build + fix a real SIGTERM shutdown bug found by it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DX audit fix #5 (the #1 lever per the audit). New `conduit-connector-sdk` console script, `build` subcommand (_build.py/_cli.py): packages a connector project into one self-contained, directly-executable artifact. Closes the design doc §1.1.6 packaging gap -- Conduit execs a standalone connector subprocess with a clean environment (no inherited PATH), so a `pip install`-then-shebang-script connector cannot launch; this command produces a file whose shebang is an absolute interpreter path resolved at build time. Vendoring strategy: copies files from the *current environment's* already-installed distributions (importlib.metadata: file manifests + transitive Requires-Dist, marker-filtered for extras) rather than a fresh pip install -- this SDK isn't published to PyPI yet, so a fresh resolve would fail outright; this also means `build` needs no network access and runs in ~0.2-0.4s. conduit-connector-sdk itself is vendored by copying its actual installed location directly (works for editable dev installs too, where importlib.metadata's file manifest is just a .pth redirect). Not a plain zipapp: grpcio and pydantic's pydantic-core both ship compiled extensions, which zipimport cannot load from inside a zip archive. Found this the hard way -- an earlier version of this command raised BuildError on any compiled extension, which would have made this SDK's own core dependencies (grpcio, pydantic) impossible to vendor at all. Fixed by generating a small, dependency-free bootstrap __main__.py (the only thing zipimport ever runs directly) that extracts the real payload to a per-build cache directory on first run -- the same fundamental approach shiv/pex use -- then executes the connector's real entry point from those extracted, real files. Verified end-to-end: built the example connector, exec'd the artifact directly (not `python `), confirmed grpc/pydantic-core's .so files are present as real extracted files and the handshake completes correctly. Bug fix, found via that same end-to-end testing: sending a real SIGTERM to a running serve() process whose connector's teardown() raised (e.g. HTTPPollSource.teardown() accessing self._client before open() ever ran) silently hung for the full watchdog deadline instead of shutting down promptly. Root cause: _sigterm_shutdown()'s coroutine is scheduled via run_coroutine_threadsafe from a signal handler with its Future never awaited/checked, so an exception inside it was silently swallowed and shutdown_requested was never set. Fixed: shutdown_requested.set() now runs unconditionally in a finally block, with a clear stderr diagnostic distinguishing "teardown raised" from "loop genuinely wedged" -- a buggy teardown() no longer blocks the entire SIGTERM-triggered graceful path. Also hardened the example connector's own teardown() to guard against running before open() ever did. Tests: tests/test_build.py builds the real example connector and execs the resulting artifact directly as a subprocess (mirroring exactly how Conduit's dispenser launches a plugin) -- asserts a valid handshake line, an absolute-path shebang, compiled extensions present as real extracted files, cache reuse on a second launch, and (the regression test for the bug above) that a real SIGTERM triggers prompt graceful shutdown rather than hanging until the watchdog fires. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD --- CHANGELOG.md | 33 +++ README.md | 41 +++- examples/http-poll-source/main.py | 21 +- pyproject.toml | 15 ++ src/conduit/_build.py | 394 ++++++++++++++++++++++++++++++ src/conduit/_cli.py | 85 +++++++ src/conduit/serve.py | 36 ++- tests/test_build.py | 223 +++++++++++++++++ 8 files changed, 839 insertions(+), 9 deletions(-) create mode 100644 src/conduit/_build.py create mode 100644 src/conduit/_cli.py create mode 100644 tests/test_build.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 62c7906..3cbef3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,5 +40,38 @@ doc's Upgrade/rollback section for the pre-1.0 caveat). `AcceptanceTestSuite` (contract version `2026-07.v1`). - Worked example connector (`examples/http-poll-source/`), exercised end-to-end by the acceptance suite in this repo's own test suite. +- Go-duration config support (`config.py`): `datetime.timedelta` fields now + map to `config.Parameter.TYPE_DURATION`, serializing/parsing Go's + `time.Duration` string syntax (`"5s"`, `"1h30m"`, `"500ms"`) via the new + public `format_go_duration()`/`parse_go_duration()` functions. Closes the + previously-`NotImplementedError` Duration A-gap. +- `BatchWriteError.partial(batch_size, written=N, cause=exc)` (`errors.py`): + the recommended constructor for the common partial-batch-write case, + carrying the real exception that caused the failure instead of a generic + placeholder message. +- `Configure` RPC handlers now catch `pydantic.ValidationError` explicitly + and abort with `INVALID_ARGUMENT` plus a per-field detail message + (`errors.format_validation_error()`), rather than relying on `grpc.aio`'s + generic `UNKNOWN`-status wrapping of an uncaught exception. +- Lifecycle hook rename: `Source`/`Destination`'s `lifecycle_on_created`/ + `lifecycle_on_updated`/`lifecycle_on_deleted` are now `on_created`/ + `on_updated`/`on_deleted` (the `lifecycle_` prefix was redundant). +- `conduit-connector-sdk build` (`_build.py`/`_cli.py`, new console-script + entry point): packages a connector project into a single, directly + executable artifact with an absolute-interpreter-path shebang (no `PATH` + lookup at exec time, per design doc §1.1.6) and every third-party + dependency vendored in, including compiled-extension dependencies + (`grpcio`, `pydantic-core`) via an extract-on-first-run bootstrap. + +### Fixed + +- `serve()`'s SIGTERM-triggered graceful shutdown path (`_sigterm_shutdown` + in `serve.py`) no longer silently hangs until the hung-loop watchdog's + deadline if `teardown()` itself raises (e.g. a connector's `teardown()` + running before `Open` was ever called) — `shutdown_requested` is now set + unconditionally, with a clear diagnostic distinguishing "teardown raised" + from "loop genuinely wedged." Found via a real end-to-end SIGTERM test + while building `conduit-connector-sdk build`'s test coverage. The example + connector's own `teardown()` is also now defensive against this case. No release has been tagged yet; nothing here is installable from PyPI. diff --git a/README.md b/README.md index 14562f6..0741b73 100644 --- a/README.md +++ b/README.md @@ -48,13 +48,44 @@ src/conduit/ destination.py # Destination ABC serve.py # handshake + gRPC server bootstrap _handshake.py # magic cookie, protocol negotiation, stdout line - _grpc/ # generated protobuf/grpc stubs (buf generate output) - testing/ # acceptance-test harness (acceptance.py, fixtures.py) -examples/http-poll-source/ # worked example connector -docs/design/ # design docs for this repo -tests/ # unit tests + _build.py # `conduit-connector-sdk build` implementation + _cli.py # `conduit-connector-sdk` console-script entry point + _grpc/ # generated protobuf/grpc stubs (buf generate output) + testing/ # acceptance-test harness (acceptance.py, fixtures.py) +examples/http-poll-source/ # worked example connector +docs/design/ # design docs for this repo +tests/ # unit tests ``` +## Building a standalone connector artifact + +Conduit launches a standalone connector as a subprocess with a **clean +environment** — no inherited `PATH` (design doc §1.1.6). A `pip +install`-then-shebang-script connector (`#!/usr/bin/env python3`, or an +activated venv) cannot launch this way: there's no `PATH` for `env` to +search. `conduit-connector-sdk build` closes that gap: + +```shell +conduit-connector-sdk build examples/http-poll-source -o http-poll-source.pyz +./http-poll-source.pyz # directly executable — no `python` prefix, no venv activation +``` + +This produces one file with an **absolute** interpreter shebang (resolved +at build time, never looked up via `PATH`), bundling every third-party +dependency your connector needs — including compiled-extension +dependencies like `grpcio`/`pydantic`'s `pydantic-core`, which a plain +[`zipapp`](https://docs.python.org/3/library/zipapp.html) can't load +in-place: the artifact extracts itself to a per-build cache directory on +first run (the same fundamental approach `shiv`/`pex` use), then executes +your connector's real entry point from those extracted files. Later +launches of the same build reuse the cache. + +**Precondition:** run `build` from an environment where your connector's +own dependencies are already installed (however you installed them — pip, +uv, poetry) — it vendors from what's already resolved, not a fresh +`pip install`. See `conduit/_build.py`'s module docstring for the full +rationale and known limitations. + ## Contributing See [CONTRIBUTING.md](CONTRIBUTING.md). This SDK sits on Conduit's data path — diff --git a/examples/http-poll-source/main.py b/examples/http-poll-source/main.py index 5b2ddef..2fbff06 100644 --- a/examples/http-poll-source/main.py +++ b/examples/http-poll-source/main.py @@ -40,6 +40,11 @@ class Config(BaseConfig): class HTTPPollSource(Source[Config]): """Polls ``config.url?since=`` for new rows, oldest-first.""" + _client: httpx.AsyncClient | None = None + """``None`` until :meth:`open` runs. Checked in :meth:`teardown` -- see + that method's docstring for why this matters even though a normal + Conduit-driven lifecycle always calls ``Open`` before ``Teardown``.""" + async def open(self, position: bytes | None) -> None: """Open the HTTP client and resume from ``position`` (or the beginning). @@ -76,8 +81,20 @@ async def read(self) -> Record: ) async def teardown(self) -> None: - """Close the HTTP client.""" - await self._client.aclose() + """Close the HTTP client, if one was ever opened. + + Guards against ``teardown()`` running before ``open()`` ever did -- + e.g. SIGTERM arriving immediately after the subprocess starts, before + Conduit calls ``Configure``/``Open``. A normal Conduit-driven + lifecycle always calls ``Open`` before ``Teardown``, but SIGTERM can + arrive at any point (design doc's SIGTERM-mid-write failure mode), + and an unguarded ``self._client.aclose()`` here would raise + ``AttributeError`` in that case -- found via a real end-to-end + SIGTERM test while building ``conduit-connector-sdk build``'s test + coverage (see ``tests/test_build.py``). + """ + if self._client is not None: + await self._client.aclose() if __name__ == "__main__": diff --git a/pyproject.toml b/pyproject.toml index 8c0c6f5..ec7af35 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -34,8 +34,23 @@ dependencies = [ # standard thing"). Runtime dependency, not dev-only: `conduit.serve` # registers this health servicer on every plugin process. "grpcio-health-checking>=1.75,<2", + # Used by `conduit-connector-sdk build` (conduit/_build.py) to parse + # PEP 508 requirement strings and normalize distribution names when + # vendoring a connector's dependencies -- already a transitive + # dependency of pip/hatchling in practice, but a real, direct runtime + # dependency of this SDK's own `build` command, so declared explicitly + # rather than relied on implicitly. + "packaging>=23,<26", ] +[project.scripts] +# `build` packages a connector project into a self-contained, +# directly-executable artifact -- see conduit/_build.py's module +# docstring for why this is required (not just convenient) for Conduit to +# launch a standalone connector (design doc §1.1.6: Conduit execs with a +# clean environment, no inherited PATH). +conduit-connector-sdk = "conduit._cli:main" + [project.urls] Homepage = "https://github.com/ConduitIO/conduit-connector-sdk-python" Repository = "https://github.com/ConduitIO/conduit-connector-sdk-python" diff --git a/src/conduit/_build.py b/src/conduit/_build.py new file mode 100644 index 0000000..e49fb48 --- /dev/null +++ b/src/conduit/_build.py @@ -0,0 +1,394 @@ +"""``conduit-connector-sdk build``: package a connector into one executable file. + +Per ``docs/design/20260707-python-connector-sdk.md`` §1.1.6: Conduit execs a +standalone connector subprocess with a **clean environment** +(``cmd.Env = make([]string, 0)`` before appending its own vars, +``pconnector/client/client.go:45``) -- no inherited ``PATH``. A +``pip install``-then-shebang-script approach (relying on +``#!/usr/bin/env python3`` resolving via ``PATH``, or an activated venv) +cannot launch under Conduit for exactly this reason -- this was flagged in +the design doc's Risks & open questions as a packaging gap this repo's own +Lane D closes. This module is that fix: it builds a single-file artifact +whose embedded shebang is an **absolute** interpreter path resolved at +build time (never looked up via ``PATH`` at exec time), with every +third-party dependency the connector needs bundled directly into the +archive. + +**Vendoring strategy: vendor from the current environment, not a fresh +``pip install``.** Rather than re-resolving dependencies via a fresh +install (which would fail outright for this SDK itself -- not yet +published to PyPI -- and adds a network dependency to every build), this +vendors packages directly from the *current Python environment's* +already-installed distributions, using ``importlib.metadata`` to enumerate +each distribution's own files and transitive requirements. This is the +same fundamental assumption PEX/``shiv`` builds ultimately rely on (a +resolved environment) minus a redundant second dependency-resolution step. +Practical consequence: ``build`` must be run from an environment where the +connector's dependencies are already installed (however you installed them +-- pip, uv, poetry) -- this is a documented precondition, not a silent +requirement. + +**Why this isn't a plain ``zipapp`` (the extension-module problem, found +while testing this exact command):** ``zipimport``/``zipapp`` cannot load +compiled extension modules (``.so``/``.pyd``/``.dylib``) directly from +inside a zip archive. This SDK's own core dependencies are not all pure +Python -- ``grpcio`` and ``pydantic``'s ``pydantic-core`` both ship +compiled extensions -- so a naive zipapp containing them would fail at +import time. The fix, the same one ``shiv``/``pex`` use: the artifact is +still a single zip file with an executable shebang, but its ``__main__.py`` +is a small, dependency-free **bootstrap** that extracts the real payload +(everything vendored, including compiled extensions) to a per-build cache +directory on disk on first run, then executes the connector's real entry +point from those extracted, real files -- never via zipimport for the +parts that can't support it. Subsequent launches reuse the cached +extraction (keyed by a content hash of the archive), so the extraction +cost is paid once, not on every subprocess launch. +""" + +from __future__ import annotations + +import contextlib +import importlib.metadata +import shutil +import sys +import tempfile +import tomllib +import zipapp +from collections.abc import Iterator +from pathlib import Path + +from packaging.requirements import Requirement +from packaging.utils import canonicalize_name + +_CONDUIT_SDK_DIST_NAME = "conduit-connector-sdk" +_SKIP_TOP_LEVEL_SUFFIXES = (".dist-info", ".data") +_SKIP_SOURCE_NAMES = frozenset( + {"pyproject.toml", "uv.lock", "poetry.lock", "README.md", "__pycache__", ".git"} +) +_PAYLOAD_DIR_NAME = "_payload" + +# The bootstrap that becomes the archive's own top-level `__main__.py`. +# Deliberately stdlib-only (no `conduit` import) -- it runs *before* the +# real payload (including this SDK itself) has been extracted anywhere +# `sys.path` can see it. See module docstring for why this exists at all. +_BOOTSTRAP_SOURCE = '''\ +"""Auto-generated by `conduit-connector-sdk build` -- do not edit. + +Extracts this archive's vendored payload (which may contain compiled +extension modules `zipimport` cannot load in place -- grpcio and +pydantic-core, both core dependencies of the connector SDK, are exactly +this case) to a per-build cache directory on first run, then executes the +real connector entry point from there. Later launches of the same archive +(same content hash) reuse the cached extraction. +""" + +from __future__ import annotations + +import hashlib +import os +import runpy +import sys +import zipfile +from pathlib import Path + +_PAYLOAD_PREFIX = "_payload/" + + +def _cache_root() -> Path: + override = os.environ.get("CONDUIT_CONNECTOR_BUILD_CACHE_DIR") + if override: + return Path(override) + return Path.home() / ".cache" / "conduit-connector-sdk" / "builds" + + +def _extract_payload(archive_path: Path) -> Path: + # Known limitation, not addressed here: two processes launching this + # same archive for the very first time concurrently can race on this + # check-then-extract sequence. Since both would extract byte-identical + # content from the same source archive to the same destination paths, + # this is not a correctness risk in practice (no partial/torn file can + # result from two writers writing identical bytes), but it is not + # formally locked -- a follow-up could add a lockfile if this ever + # matters for a real deployment's concurrent-launch pattern. + digest = hashlib.sha256(archive_path.read_bytes()).hexdigest()[:16] + target = _cache_root() / digest + marker = target / ".extracted-ok" + if not marker.exists(): + target.mkdir(parents=True, exist_ok=True) + with zipfile.ZipFile(archive_path) as archive: + for name in archive.namelist(): + if name.startswith(_PAYLOAD_PREFIX) and not name.endswith("/"): + archive.extract(name, target) + marker.touch() + return target / "_payload" + + +def _main() -> None: + archive_path = Path(sys.argv[0]).resolve() + payload_dir = _extract_payload(archive_path) + sys.path.insert(0, str(payload_dir)) + runpy.run_path(str(payload_dir / "__main__.py"), run_name="__main__") + + +_main() +''' + + +class BuildError(Exception): + """Raised when :func:`build_connector_artifact` cannot produce an artifact. + + Every raise site here is meant to be actionable (``CLAUDE.md``'s + "errors are API" standard): each message says what's missing and, where + there's a clear next step, what to do about it. + """ + + +def build_connector_artifact( + project_dir: Path, + output_path: Path, + *, + entry_point: str = "main.py", + interpreter: str | None = None, +) -> Path: + """Build a self-contained, directly-executable artifact for a connector project. + + Args: + project_dir: path to the connector project (containing at least + ``entry_point`` and, if it declares third-party dependencies + beyond this SDK, a ``pyproject.toml`` with a + ``[project] dependencies`` list -- see module docstring's + vendoring-strategy note for what "declares" means in practice). + output_path: where to write the artifact. Parent directories are + created if needed. + entry_point: the connector's entry script within ``project_dir``, + default ``main.py`` -- becomes the extracted payload's own + ``__main__.py`` (see module docstring's bootstrap note), + preserving the ``if __name__ == "__main__":`` guard authors + already write (design doc §2.7's worked example) unmodified. + interpreter: absolute path to embed in the artifact's shebang. + Defaults to ``sys.executable`` -- already guaranteed absolute + by Python itself, satisfying the "no ``PATH`` lookup at exec + time" requirement with no extra effort in the common case of + building with the same interpreter you'll deploy with. + + Returns: + ``output_path``, for chaining. + + Raises: + BuildError: if ``entry_point`` doesn't exist under ``project_dir``, + or if a declared dependency isn't installed in the current + environment. + """ + project_dir = Path(project_dir).resolve() + output_path = Path(output_path).resolve() + entry_script = project_dir / entry_point + if not entry_script.is_file(): + raise BuildError(f"entry point {entry_script} does not exist") + + dependency_names = _project_dependency_names(project_dir / "pyproject.toml") + sdk_name = canonicalize_name(_CONDUIT_SDK_DIST_NAME) + if sdk_name in dependency_names: + # conduit-connector-sdk itself is vendored directly from this + # running interpreter's own installed copy (see + # _vendor_conduit_sdk) -- an editable dev install (this repo's own + # setup) has no real files `importlib.metadata` can enumerate for + # it (its `RECORD` only lists a `.pth` redirect), so *it* is + # special-cased. Its own runtime dependencies (grpcio, protobuf, + # pydantic, grpcio-health-checking, ...) are NOT special-cased, + # though -- those need vendoring like any other dependency, or the + # resulting artifact only happens to work when its embedded + # interpreter shebang points at an environment that already has + # them installed (verified: building without this line produces an + # artifact that "works" only by accident, shebang'd to the dev + # venv it was built in -- not actually self-contained). + dependency_names.discard(sdk_name) + dependency_names |= _direct_requirement_names(sdk_name) + + with _temporary_build_dir() as staging_dir: + payload_dir = staging_dir / _PAYLOAD_DIR_NAME + payload_dir.mkdir() + + _vendor_conduit_sdk(payload_dir) + for dist_name in _resolve_transitive_distributions(dependency_names): + _vendor_distribution(dist_name, payload_dir) + _copy_connector_source(project_dir, payload_dir, entry_script) + + (staging_dir / "__main__.py").write_text(_BOOTSTRAP_SOURCE) + + output_path.parent.mkdir(parents=True, exist_ok=True) + zipapp.create_archive( + staging_dir, + target=output_path, + interpreter=interpreter or sys.executable, + ) + + return output_path + + +@contextlib.contextmanager +def _temporary_build_dir() -> Iterator[Path]: + with tempfile.TemporaryDirectory(prefix="conduit-connector-sdk-build-") as tmp: + yield Path(tmp) + + +def _project_dependency_names(pyproject_path: Path) -> set[str]: + """Read ``[project] dependencies`` from a connector's ``pyproject.toml``. + + Returns an empty set if there's no ``pyproject.toml`` at all -- a + connector with no declared third-party dependencies (beyond this SDK) + is valid; :func:`_vendor_conduit_sdk` still runs regardless. + """ + if not pyproject_path.is_file(): + return set() + data = tomllib.loads(pyproject_path.read_text()) + raw_dependencies = data.get("project", {}).get("dependencies", []) + return {canonicalize_name(Requirement(spec).name) for spec in raw_dependencies} + + +def _resolve_transitive_distributions(root_names: set[str]) -> list[str]: + """Breadth-first resolve every distribution a set of root deps needs. + + Uses each already-installed distribution's own recorded + ``Requires-Dist`` metadata (``importlib.metadata.Distribution.requires``), + skipping extras-conditional requirements (evaluated with no extras + active) and anything already resolved. This is metadata-only: it + trusts the *installed* distribution's own recorded dependency graph, + not a fresh dependency resolution. + + Args: + root_names: canonicalized top-level dependency names. + + Returns: + Every distribution name (canonicalized), root and transitive, that + needs vendoring. + + Raises: + BuildError: if a required distribution isn't installed in the + current environment. + """ + resolved: list[str] = [] + seen: set[str] = set() + queue = list(root_names) + while queue: + name = queue.pop() + if name in seen: + continue + seen.add(name) + resolved.append(name) + for dep_name in _direct_requirement_names(name): + if dep_name not in seen: + queue.append(dep_name) + return resolved + + +def _direct_requirement_names(dist_name: str) -> set[str]: + """Return the canonicalized names of ``dist_name``'s own non-extra requirements. + + Skips extras-conditional requirements (evaluated with no extras + active -- nothing this command builds requests an extra). + + Args: + dist_name: canonicalized distribution name. + + Returns: + Canonicalized names of every unconditional runtime dependency. + + Raises: + BuildError: if ``dist_name`` isn't installed in the current + environment. + """ + try: + dist = importlib.metadata.distribution(dist_name) + except importlib.metadata.PackageNotFoundError as exc: + raise BuildError( + f"dependency {dist_name!r} is not installed in the current " + "environment -- `build` vendors from already-installed " + "distributions (see conduit._build's module docstring); " + f"install it here first (e.g. `pip install {dist_name}`)" + ) from exc + names: set[str] = set() + for requirement_str in dist.requires or (): + requirement = Requirement(requirement_str) + if requirement.marker is not None and not requirement.marker.evaluate({"extra": ""}): + continue # an optional extra this build doesn't request + names.add(canonicalize_name(requirement.name)) + return names + + +def _vendor_distribution(dist_name: str, payload_dir: Path) -> None: + """Copy one already-installed distribution's real files into ``payload_dir``. + + Compiled extension modules are copied like any other file here -- + unlike a plain zipapp, this artifact's bootstrap extracts the payload + to real files on disk before ever importing anything from it (see + module docstring), so a ``.so``/``.pyd``/``.dylib`` works fine. + + Args: + dist_name: canonicalized distribution name. + payload_dir: destination root (the extracted payload's future root). + + Raises: + BuildError: if the distribution has no file manifest (see + :func:`_resolve_transitive_distributions`'s docstring on + editable installs). + """ + dist = importlib.metadata.distribution(dist_name) + if dist.files is None: + raise BuildError( + f"distribution {dist_name!r} has no file manifest (RECORD) -- " + "cannot vendor it. Is it installed in editable/development " + "mode? Only conduit-connector-sdk itself is special-cased for " + "that (see _vendor_conduit_sdk); a real dependency needs a " + "normal (non-editable) install in this environment to be " + "vendored." + ) + for file in dist.files: + if str(file).startswith(".."): + continue # outside the distribution's own tree, e.g. a console-script shim + if any(part.endswith(_SKIP_TOP_LEVEL_SUFFIXES) for part in file.parts): + continue # *.dist-info / *.data housekeeping, not importable code + source = file.locate() + if not Path(source).is_file(): + continue + destination = payload_dir / file + destination.parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(source, destination) + + +def _vendor_conduit_sdk(payload_dir: Path) -> None: + """Copy this SDK's own package directory into ``payload_dir`` directly. + + Bypasses ``importlib.metadata`` entirely: works identically whether + this SDK is installed normally or (as in this repo's own dev setup) + in editable mode, since it just asks Python's own import system where + ``conduit`` actually lives, rather than reading install metadata. + """ + import conduit # local import: this is the package this module lives in + + conduit_dir = Path(conduit.__file__).resolve().parent + shutil.copytree(conduit_dir, payload_dir / "conduit", dirs_exist_ok=True) + + +def _copy_connector_source(project_dir: Path, payload_dir: Path, entry_script: Path) -> None: + """Copy a connector project's own source into ``payload_dir``. + + Skips packaging/VCS housekeeping (``pyproject.toml``, lockfiles, + ``README.md``, ``__pycache__``, ``.git``) -- everything else in + ``project_dir`` is assumed to be the connector's own source and is + copied verbatim, so a connector split across multiple modules (not + just a single ``main.py``) still works. + """ + for item in project_dir.iterdir(): + if item.name in _SKIP_SOURCE_NAMES or item.name.startswith("."): + continue + destination = payload_dir / item.name + if item.is_dir(): + shutil.copytree(item, destination, dirs_exist_ok=True) + else: + shutil.copy2(item, destination) + + # The bootstrap's `runpy.run_path` looks for `__main__.py` at the + # extracted payload's root (see _BOOTSTRAP_SOURCE) -- copy (not move, + # not a symlink: the zip needs a real file) the connector's own entry + # script there. + shutil.copy2(entry_script, payload_dir / "__main__.py") diff --git a/src/conduit/_cli.py b/src/conduit/_cli.py new file mode 100644 index 0000000..61c7989 --- /dev/null +++ b/src/conduit/_cli.py @@ -0,0 +1,85 @@ +"""``conduit-connector-sdk`` console-script entry point. + +Currently one subcommand, ``build`` (see :mod:`conduit._build`). Mirrors the +org-wide convention (Conduit's own Go CLI) of a single top-level command +with subcommands rather than a proliferation of separate scripts. +""" + +from __future__ import annotations + +import argparse +import sys +from pathlib import Path + +from conduit._build import BuildError, build_connector_artifact + + +def main(argv: list[str] | None = None) -> int: + """Entry point for the ``conduit-connector-sdk`` console script. + + Args: + argv: argument vector, excluding the program name; defaults to + ``sys.argv[1:]``. Injectable for testing. + + Returns: + Process exit code (``0`` on success). + """ + parser = _build_parser() + args = parser.parse_args(argv) + + if args.command == "build": + try: + output = build_connector_artifact( + args.project_dir, + args.output, + entry_point=args.entry_point, + interpreter=args.interpreter, + ) + except BuildError as exc: + print(f"conduit-connector-sdk build: error: {exc}", file=sys.stderr) + return 1 + print(f"built {output}") + return 0 + + parser.print_help(sys.stderr) + return 1 + + +def _build_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser(prog="conduit-connector-sdk") + subparsers = parser.add_subparsers(dest="command") + + build_parser = subparsers.add_parser( + "build", + help=( + "Build a self-contained, directly-executable connector artifact " + "(a zipapp with an absolute-interpreter-path shebang, bundling " + "every third-party dependency). Required for Conduit to launch " + "a standalone connector -- see " + "docs/design/20260707-python-connector-sdk.md §1.1.6." + ), + ) + build_parser.add_argument( + "project_dir", type=Path, help="path to the connector project directory" + ) + build_parser.add_argument( + "-o", "--output", type=Path, required=True, help="output artifact path" + ) + build_parser.add_argument( + "--entry-point", + default="main.py", + help="entry script within project_dir (default: main.py)", + ) + build_parser.add_argument( + "--interpreter", + default=None, + help=( + "absolute interpreter path to embed in the artifact's shebang " + "(default: the interpreter running this build command)" + ), + ) + return parser + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/src/conduit/serve.py b/src/conduit/serve.py index 64e47ac..6c9f532 100644 --- a/src/conduit/serve.py +++ b/src/conduit/serve.py @@ -393,8 +393,40 @@ def graceful_trigger() -> None: asyncio.run_coroutine_threadsafe(_sigterm_shutdown(), loop) async def _sigterm_shutdown() -> None: - await run_teardown_once() - shutdown_requested.set() + """Run teardown, then unblock ``drive_shutdown`` -- unconditionally. + + **Bug this fixes, found while building/testing the ``build`` CLI + command (item 5): if ``teardown()`` itself raised (e.g. a + connector's ``teardown()`` accessing a resource ``open()`` never + got a chance to set up, because SIGTERM arrived before Conduit + ever called ``Open``), the exception used to propagate out of this + coroutine. Since it runs via ``run_coroutine_threadsafe`` with its + returned ``Future`` never awaited/checked (a signal handler has + nothing to await), that exception was silently swallowed -- + ``shutdown_requested`` was never set, ``drive_shutdown`` waited + forever, and the hung-loop watchdog fired and force-exited with a + "wedged event loop" diagnostic that was actively misleading: the + loop was never wedged, a plain exception was just never observed. + ``shutdown_requested.set()`` in a ``finally`` block makes forward + progress on shutdown unconditional -- a buggy ``teardown()`` no + longer blocks the *entire* SIGTERM-triggered graceful path; it only + loses the (already-lost, since it raised) cleanup it was supposed + to do, and this prints a clear diagnostic distinguishing "teardown + raised" from "loop genuinely wedged" rather than reaching the + watchdog's generic message at all. + """ + try: + await run_teardown_once() + except Exception as exc: + print( + f"conduit-sdk: teardown() raised during SIGTERM-triggered " + f"shutdown: {exc!r} -- shutting down anyway rather than " + "hanging until the watchdog force-exits", + file=stderr, + flush=True, + ) + finally: + shutdown_requested.set() coordinator = _ShutdownCoordinator( deadline=shutdown_deadline, diff --git a/tests/test_build.py b/tests/test_build.py new file mode 100644 index 0000000..870ec5c --- /dev/null +++ b/tests/test_build.py @@ -0,0 +1,223 @@ +"""Tests for ``conduit-connector-sdk build`` (:mod:`conduit._build`/``_cli``). + +Builds the real worked example connector (``examples/http-poll-source``) +into a self-contained artifact and **execs the resulting file directly** +(never via ``python ``) -- matching exactly how Conduit's +dispenser launches a standalone connector subprocess (design doc §1.1.6: +a clean environment, no inherited ``PATH``, so a plain shebang script +resolved via ``PATH`` cannot work). This must actually pass, not be +skipped: it's the closest thing this repo's CI has to proving the +packaging story the design doc calls out as a hard launch gate. +""" + +from __future__ import annotations + +import os +import stat +import subprocess +import sys +from pathlib import Path + +import pytest + +from conduit._build import BuildError, build_connector_artifact +from conduit._handshake import MAGIC_COOKIE_KEY, MAGIC_COOKIE_VALUE, PROTOCOL_VERSIONS_ENV + +_EXAMPLE_PROJECT_DIR = Path(__file__).resolve().parent.parent / "examples" / "http-poll-source" + + +def _handshake_env(cache_dir: Path) -> dict[str, str]: + env = dict(os.environ) + env[MAGIC_COOKIE_KEY] = MAGIC_COOKIE_VALUE + env[PROTOCOL_VERSIONS_ENV] = "2" + # Isolate the extraction cache to this test run -- never touch the + # real user cache directory, and never let one test's extraction leak + # into another's. + env["CONDUIT_CONNECTOR_BUILD_CACHE_DIR"] = str(cache_dir) + return env + + +@pytest.fixture(scope="module") +def built_artifact(tmp_path_factory: pytest.TempPathFactory) -> Path: + """Build the real example connector once, shared across this module's tests.""" + output_dir = tmp_path_factory.mktemp("build-output") + output_path = output_dir / "http-poll-source.pyz" + build_connector_artifact(_EXAMPLE_PROJECT_DIR, output_path) + return output_path + + +class TestBuildConnectorArtifact: + def test_output_file_exists_and_is_executable(self, built_artifact: Path) -> None: + assert built_artifact.is_file() + mode = built_artifact.stat().st_mode + assert mode & stat.S_IXUSR, "artifact must have the executable bit set" + + def test_shebang_is_an_absolute_interpreter_path(self, built_artifact: Path) -> None: + """Design doc §1.1.6: Conduit execs with no inherited PATH -- the + + shebang must be an absolute path, never `#!/usr/bin/env python3` + (which would require PATH resolution at exec time). + """ + with built_artifact.open("rb") as f: + first_line = f.readline() + assert first_line.startswith(b"#!") + interpreter_path = first_line[2:].decode().strip() + assert Path(interpreter_path).is_absolute() + assert interpreter_path == sys.executable + + def test_directly_executed_artifact_prints_a_valid_handshake_line( + self, built_artifact: Path, tmp_path: Path + ) -> None: + """Exec the artifact PATH itself -- not `python ` -- exactly + + how Conduit's dispenser launches a standalone connector subprocess. + """ + cache_dir = tmp_path / "cache" + proc = subprocess.Popen( + [str(built_artifact)], # <-- the artifact itself is the executable + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env=_handshake_env(cache_dir), + text=True, + ) + try: + line = proc.stdout.readline() + parts = line.strip().split("|") + assert len(parts) == 6, f"malformed handshake line: {line!r}" + core_version, app_version, network, address, protocol, server_cert = parts + assert core_version == "1" + assert app_version == "2" + assert network == "tcp" + assert address # non-empty listen address + assert protocol == "grpc" + assert server_cert == "" + finally: + proc.terminate() + try: + proc.wait(timeout=5) + except subprocess.TimeoutExpired: + proc.kill() + proc.wait() + + def test_sigterm_triggers_prompt_graceful_shutdown( + self, built_artifact: Path, tmp_path: Path + ) -> None: + """A real SIGTERM to the real exec'd artifact exits promptly, not after + + the watchdog's multi-second deadline -- the regression this test + pins: found via this exact scenario while building this test + (`HTTPPollSource.teardown()` used to crash with `AttributeError` + when `SIGTERM` arrived before `Open` was ever called, silently + swallowing the exception and hanging until the watchdog fired; see + `conduit/serve.py`'s `_sigterm_shutdown` and the example's + `teardown()` guard). + """ + import signal + import time + + cache_dir = tmp_path / "cache" + proc = subprocess.Popen( + [str(built_artifact)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env=_handshake_env(cache_dir), + text=True, + ) + try: + proc.stdout.readline() # wait for the handshake line + start = time.monotonic() + proc.send_signal(signal.SIGTERM) + proc.wait(timeout=4) # well under the default 5s watchdog deadline + elapsed = time.monotonic() - start + assert proc.returncode == 0 + assert elapsed < 2.0, ( + f"exited after {elapsed:.2f}s -- expected a prompt graceful " + "shutdown, not a wait anywhere near the watchdog's deadline" + ) + finally: + if proc.poll() is None: + proc.kill() + proc.wait() + + def test_extraction_cache_is_reused_on_second_launch( + self, built_artifact: Path, tmp_path: Path + ) -> None: + """The payload is extracted once per distinct cache dir, not on every launch.""" + cache_dir = tmp_path / "cache" + env = _handshake_env(cache_dir) + + for _ in range(2): + proc = subprocess.Popen( + [str(built_artifact)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env=env, + text=True, + ) + try: + line = proc.stdout.readline() + assert line.strip().split("|")[4] == "grpc" + finally: + proc.terminate() + try: + proc.wait(timeout=5) + except subprocess.TimeoutExpired: + proc.kill() + proc.wait() + + marker_dirs = list(cache_dir.glob("*/.extracted-ok")) + assert len(marker_dirs) == 1, "expected exactly one cached extraction, reused twice" + + def test_extracted_payload_includes_compiled_extension_modules( + self, built_artifact: Path, tmp_path: Path + ) -> None: + """Pins that this is NOT a plain zipapp: grpcio's and pydantic-core's + + compiled extensions must be present as real extracted files, not + silently dropped or left unimportable inside the zip. + """ + cache_dir = tmp_path / "cache" + proc = subprocess.Popen( + [str(built_artifact)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env=_handshake_env(cache_dir), + text=True, + ) + try: + proc.stdout.readline() + finally: + proc.terminate() + try: + proc.wait(timeout=5) + except subprocess.TimeoutExpired: + proc.kill() + proc.wait() + + extracted = list(cache_dir.glob("*/_payload")) + assert len(extracted) == 1 + payload_dir = extracted[0] + so_files = list(payload_dir.rglob("*.so")) + list(payload_dir.rglob("*.dylib")) + assert so_files, "expected at least one compiled extension module to be vendored" + assert (payload_dir / "conduit").is_dir() + assert (payload_dir / "httpx").is_dir() + assert (payload_dir / "__main__.py").is_file() + + +class TestBuildConnectorArtifactErrors: + def test_missing_entry_point_raises_build_error(self, tmp_path: Path) -> None: + empty_project = tmp_path / "empty-project" + empty_project.mkdir() + with pytest.raises(BuildError, match="does not exist"): + build_connector_artifact(empty_project, tmp_path / "out.pyz") + + def test_uninstalled_dependency_raises_build_error(self, tmp_path: Path) -> None: + project = tmp_path / "project-with-bad-dep" + project.mkdir() + (project / "main.py").write_text("print('hi')\n") + (project / "pyproject.toml").write_text( + '[project]\nname = "x"\nversion = "0.1.0"\n' + 'dependencies = ["definitely-not-a-real-installed-package-xyz"]\n' + ) + with pytest.raises(BuildError, match="not installed"): + build_connector_artifact(project, tmp_path / "out.pyz") From 036cf02152861f527f859d348cc126f365d6056e Mon Sep 17 00:00:00 2001 From: DeVaris Brown Date: Thu, 23 Jul 2026 16:21:31 -0400 Subject: [PATCH 8/9] fix(tests): make test_build.py's exec-based tests Windows-aware CI's windows-latest jobs failed test_build.py: Windows has neither POSIX executable bits (os.chmod's execute flags are a no-op there for arbitrary extensions) nor shebang-based direct execution (CreateProcess dispatches by file extension, not by parsing a leading `#!` line -- confirmed via the actual CI failure: `OSError: [WinError 193] %1 is not a valid Win32 application` when invoking the bare .pyz path). This is exactly the "Windows subprocess launch specifics ... untested until the CI matrix actually runs it" risk the design doc already flagged as open, now surfaced for real. - The executable-bit assertion is skipped on Windows (nothing meaningful to assert there for a .pyz). - Every subprocess invocation goes through a new `_exec_argv()` helper: the bare artifact path on POSIX (the actual "no `python` prefix" claim this test suite is about), `[sys.executable, artifact]` on Windows, documented as a real, known platform difference rather than silently worked around. - `test_sigterm_triggers_prompt_graceful_shutdown` is skipped on Windows outright: `Popen.send_signal(SIGTERM)` maps to an unconditional `TerminateProcess()` there, not something `conduit.serve`'s SIGTERM handler ever observes, so the test would not exercise the graceful path it exists to pin. Windows-native graceful shutdown is out of scope for this fix (a real, separate feature involving different Windows IPC/ signal-equivalent mechanisms). - The compiled-extension-module check now also looks for `*.pyd` (Windows' extension suffix), not just `*.so`/`*.dylib`. Verified: all 8 tests in tests/test_build.py still pass locally (macOS/POSIX); the Windows-specific branches were validated against the exact CI failure output, not guessed at blind. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD --- tests/test_build.py | 63 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 56 insertions(+), 7 deletions(-) diff --git a/tests/test_build.py b/tests/test_build.py index 870ec5c..d621256 100644 --- a/tests/test_build.py +++ b/tests/test_build.py @@ -25,6 +25,9 @@ _EXAMPLE_PROJECT_DIR = Path(__file__).resolve().parent.parent / "examples" / "http-poll-source" +_IS_WINDOWS = sys.platform == "win32" +_COMPILED_EXTENSION_GLOBS = ("*.so", "*.dylib", "*.pyd") + def _handshake_env(cache_dir: Path) -> dict[str, str]: env = dict(os.environ) @@ -37,6 +40,33 @@ def _handshake_env(cache_dir: Path) -> dict[str, str]: return env +def _exec_argv(artifact: Path) -> list[str]: + """Build the subprocess argv that launches ``artifact``. + + On POSIX, the artifact's own absolute-interpreter shebang makes it + directly executable -- exactly how Conduit's dispenser launches a + plugin (design doc §1.1.6), so the argv is just the artifact path. + + Windows has no shebang-based direct execution at all (`CreateProcess` + dispatches by file extension, not by parsing a leading `#!` line -- + confirmed empirically: invoking the bare artifact path on + windows-latest CI raises `OSError: [WinError 193] %1 is not a valid + Win32 application`). This is a real, known platform gap the design + doc's Risks & open questions already flagged ("Windows subprocess + launch specifics ... untested until the CI matrix actually runs it") -- + now that it does, this is exactly that gap, not swept under the rug. + Solving Windows-native direct execution (e.g. via `.pyz`/`py.exe` file + association, which is a machine-configuration concern, not something + this build command controls) is out of scope for this fix; tests that + only need the artifact's *contents* to be correct explicitly invoke it + via `sys.executable` on Windows instead of asserting the (POSIX-only) + zero-prefix direct-exec property. + """ + if _IS_WINDOWS: + return [sys.executable, str(artifact)] + return [str(artifact)] + + @pytest.fixture(scope="module") def built_artifact(tmp_path_factory: pytest.TempPathFactory) -> Path: """Build the real example connector once, shared across this module's tests.""" @@ -49,6 +79,11 @@ def built_artifact(tmp_path_factory: pytest.TempPathFactory) -> Path: class TestBuildConnectorArtifact: def test_output_file_exists_and_is_executable(self, built_artifact: Path) -> None: assert built_artifact.is_file() + if _IS_WINDOWS: + # Windows has no POSIX executable-bit concept for arbitrary + # file extensions (`os.chmod`'s execute bits are a no-op for a + # `.pyz` file there) -- see `_exec_argv`'s docstring. + return mode = built_artifact.stat().st_mode assert mode & stat.S_IXUSR, "artifact must have the executable bit set" @@ -70,11 +105,12 @@ def test_directly_executed_artifact_prints_a_valid_handshake_line( ) -> None: """Exec the artifact PATH itself -- not `python ` -- exactly - how Conduit's dispenser launches a standalone connector subprocess. + how Conduit's dispenser launches a standalone connector subprocess + (POSIX only -- see `_exec_argv`'s docstring for the Windows gap). """ cache_dir = tmp_path / "cache" proc = subprocess.Popen( - [str(built_artifact)], # <-- the artifact itself is the executable + _exec_argv(built_artifact), stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=_handshake_env(cache_dir), @@ -99,6 +135,17 @@ def test_directly_executed_artifact_prints_a_valid_handshake_line( proc.kill() proc.wait() + @pytest.mark.skipif( + _IS_WINDOWS, + reason=( + "Windows has no real SIGTERM: subprocess.Popen.send_signal(SIGTERM) " + "maps to TerminateProcess() there -- an unconditional hard kill, not " + "something conduit.serve's SIGTERM handler ever sees -- so this test " + "would not exercise the graceful path it's meant to pin at all. " + "Windows-native graceful shutdown is a documented open risk in the " + "design doc ('Windows subprocess launch specifics'), not solved here." + ), + ) def test_sigterm_triggers_prompt_graceful_shutdown( self, built_artifact: Path, tmp_path: Path ) -> None: @@ -110,14 +157,14 @@ def test_sigterm_triggers_prompt_graceful_shutdown( when `SIGTERM` arrived before `Open` was ever called, silently swallowing the exception and hanging until the watchdog fired; see `conduit/serve.py`'s `_sigterm_shutdown` and the example's - `teardown()` guard). + `teardown()` guard). POSIX only -- see the `skipif` reason above. """ import signal import time cache_dir = tmp_path / "cache" proc = subprocess.Popen( - [str(built_artifact)], + _exec_argv(built_artifact), stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=_handshake_env(cache_dir), @@ -148,7 +195,7 @@ def test_extraction_cache_is_reused_on_second_launch( for _ in range(2): proc = subprocess.Popen( - [str(built_artifact)], + _exec_argv(built_artifact), stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env, @@ -178,7 +225,7 @@ def test_extracted_payload_includes_compiled_extension_modules( """ cache_dir = tmp_path / "cache" proc = subprocess.Popen( - [str(built_artifact)], + _exec_argv(built_artifact), stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=_handshake_env(cache_dir), @@ -197,7 +244,9 @@ def test_extracted_payload_includes_compiled_extension_modules( extracted = list(cache_dir.glob("*/_payload")) assert len(extracted) == 1 payload_dir = extracted[0] - so_files = list(payload_dir.rglob("*.so")) + list(payload_dir.rglob("*.dylib")) + so_files = [ + match for glob in _COMPILED_EXTENSION_GLOBS for match in payload_dir.rglob(glob) + ] assert so_files, "expected at least one compiled extension module to be vendored" assert (payload_dir / "conduit").is_dir() assert (payload_dir / "httpx").is_dir() From a8354f3b7702df177549c25659ced0c9532da741 Mon Sep 17 00:00:00 2001 From: DeVaris Brown Date: Thu, 23 Jul 2026 18:19:46 -0400 Subject: [PATCH 9/9] fix(serve): drain in-flight read/write before SIGTERM-triggered teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tier-1 review gap (invariant 7): serve.py's _sigterm_shutdown ran teardown() immediately on SIGTERM with no regard for an actively streaming Run() call, letting teardown() (e.g. closing a DB pool) race a live Source.read()/Destination.write(). Not a data-loss bug (a raced write nacks the whole batch and Conduit redelivers), but a graceful- shutdown gap on a Tier-1 SDK where graceful shutdown is the contract. - _SourceServicer.drain()/_DestinationServicer.drain() factor out the same stop-then-wait ordering Stop() already performs for the deterministic path (Conduit Stop RPC -> Run ends -> Teardown RPC), reusable from serve.py's SIGTERM handler. Destination's Run() gained the same _stop_event/_stopped_event/finally shape Source already had, so drain() waits for the whole Run() generator to finish (including its already-computed ack response), not just for write() to return. - _sigterm_shutdown awaits drain() before teardown(), bounded by the existing hung-loop watchdog deadline (unchanged) so a stuck connector still force-exits on schedule. - Fixed the design doc overclaim (Risks & open questions §3): it asserted the ordinary SIGTERM-mid-write case was "covered by the ordinary SIGTERM-mid-write test," but the only existing SIGTERM test fires before Open(). Added the actual TestSigtermDrainsInFlightOperation::test_sigterm_mid_read_drains_before_teardown and ::test_sigterm_mid_write_drains_before_teardown integration tests (real grpc.aio server + client, deterministic via direct _on_sigterm invocation) and pointed the doc at them. Gates: ruff format --check, ruff check, mypy --strict (18 source files), full pytest (165 passed). Generated _grpc/ dirs unchanged. Tier 1 (data path adjacent: SDK shutdown contract) -- does not merge without DeVaris sign-off. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD --- docs/design/20260707-python-connector-sdk.md | 19 +- src/conduit/destination.py | 72 ++++++- src/conduit/serve.py | 95 ++++++--- src/conduit/source.py | 22 +- tests/test_serve.py | 204 ++++++++++++++++++- 5 files changed, 374 insertions(+), 38 deletions(-) diff --git a/docs/design/20260707-python-connector-sdk.md b/docs/design/20260707-python-connector-sdk.md index 5660cd0..59a83ed 100644 --- a/docs/design/20260707-python-connector-sdk.md +++ b/docs/design/20260707-python-connector-sdk.md @@ -919,13 +919,18 @@ replacement for, the ordinary (non-wedged) SIGTERM-mid-write drain test. interaction with process-exit signal handling needs care. **This risk is now split into two distinct sub-cases, per ▶ MUST-FIX 3 above**: (a) the ordinary case — an event loop that's mid-`await` when SIGTERM arrives must - let in-flight writes drain before exit (covered by the ordinary - SIGTERM-mid-write test), and (b) the **hung-loop** case — SIGTERM arrives - while the loop is wedged and cannot process the signal handler at all, - which has no Go analog and needs the bounded watchdog mitigation and its - own dedicated test. Treating these as one risk understates (b), which is - the one that can genuinely hang a shutdown indefinitely without the - watchdog. + let in-flight reads/writes drain before `teardown()` runs, enforced by + `_SourceServicer.drain`/`_DestinationServicer.drain` (`conduit/source.py`, + `conduit/destination.py`) and wired into `conduit/serve.py`'s + `_sigterm_shutdown`; covered by + `tests/test_serve.py::TestSigtermDrainsInFlightOperation`'s + `test_sigterm_mid_write_drains_before_teardown` and + `test_sigterm_mid_read_drains_before_teardown` — and (b) the **hung-loop** + case — SIGTERM arrives while the loop is wedged and cannot process the + signal handler at all, which has no Go analog and needs the bounded + watchdog mitigation and its own dedicated test. Treating these as one risk + understates (b), which is the one that can genuinely hang a shutdown + indefinitely without the watchdog. 4. **Performance vs. Go is unknown and unclaimed.** No benchmark exists yet; per CLAUDE.md, no performance claim should be made about this SDK until a `benchi` run is committed to the repo. diff --git a/src/conduit/destination.py b/src/conduit/destination.py index 08b7ae6..480a18e 100644 --- a/src/conduit/destination.py +++ b/src/conduit/destination.py @@ -23,6 +23,7 @@ from __future__ import annotations import abc +import asyncio from collections.abc import AsyncIterator, Sequence from typing import Any, Generic, TypeVar @@ -138,6 +139,9 @@ def __init__(self, destination: Destination[Any], config_cls: type[BaseConfig]) """ self._destination = destination self._config_cls = config_cls + self._stop_event = asyncio.Event() + self._stopped_event = asyncio.Event() + self._run_started = False async def Configure( self, @@ -180,11 +184,35 @@ async def Run( batch, in the same order -- there is no cross-batch buffering here, keeping the ack/write relationship for a given batch entirely local to one iteration of this loop. + + The ``try``/``finally`` (setting ``_stopped_event``) mirrors + :meth:`conduit.source._SourceServicer.Run`'s structure exactly, for + the same reason: it's what lets :meth:`drain` block until this + generator has actually finished -- including yielding (to the + framework) whatever ack response was already computed for the + in-flight batch -- rather than only until ``write()`` itself + returns, which would let ``conduit.serve``'s SIGTERM path tear the + server down (``server.stop()``) before that already-earned ack had + a chance to reach Conduit. """ - async for request in request_iterator: - records = records_from_proto(request.records) - acks = await self._write_batch(records) - yield destination_pb2.Destination.Run.Response(acks=acks) + self._run_started = True + try: + async for request in request_iterator: + if self._stop_event.is_set(): + # A SIGTERM-triggered drain (see `drain`, below) asked + # the write loop to stop accepting new batches. + # Conduit's deterministic `Stop` RPC path gets this for + # free -- it simply stops sending requests after calling + # `Stop` -- but a SIGTERM can land mid-`Run` with more + # batches already queued on the stream, so this check is + # what makes the "no new write starts after the drain + # point" half of `drain`'s contract actually hold. + break + records = records_from_proto(request.records) + acks = await self._write_batch(records) + yield destination_pb2.Destination.Run.Response(acks=acks) + finally: + self._stopped_event.set() async def _write_batch(self, records: Sequence[Record]) -> list[Ack]: """Call ``write()`` and translate the outcome into per-record acks. @@ -253,6 +281,42 @@ async def Stop( """ return destination_pb2.Destination.Stop.Response() + async def drain(self) -> None: + """Stop accepting new write batches and await any write already in flight. + + Invariant 7 (graceful shutdown by default) enforcement site: called + by ``conduit.serve``'s SIGTERM handler before ``teardown()`` runs, so + an in-flight ``write()`` is never raced against ``teardown()`` + closing a resource (e.g. a DB pool) the write is still using. + + Mirrors :meth:`conduit.source._SourceServicer.drain`'s exact + stop-then-wait shape: sets the stop flag :meth:`Run` checks before + starting each new batch (see the enforcement site there), then -- + if ``Run`` was ever invoked -- awaits ``_stopped_event``, which + :meth:`Run`'s ``finally`` only sets once its generator has fully + finished. That is deliberately stronger than merely waiting for + ``write()`` to return: it also covers the already-computed ack + response for the in-flight batch being handed back to the ``grpc.aio`` + framework (the ``yield`` right after ``write()`` returns), so + ``conduit.serve``'s subsequent ``server.stop()`` doesn't abort a + response that was already earned. If ``Run`` was never invoked + (e.g. SIGTERM arrives before Conduit ever calls it), this returns + immediately -- there is no write loop to wait for. + + There is an unavoidable, narrow window between :meth:`Run` checking + ``_stop_event`` and this method setting it: a batch already pulled + off the request stream at that instant still starts its write. This + mirrors a window invariant 1/3 already tolerate elsewhere -- a write + already committed to starting is allowed to finish, never torn + mid-flight -- so this method stops *new* batches after the one + already in flight; it does not attempt mid-write cancellation, which + would itself violate invariant 1/3 (a torn write can't be safely + un-started). + """ + self._stop_event.set() + if self._run_started: + await self._stopped_event.wait() + async def Teardown( self, request: destination_pb2.Destination.Teardown.Request, context: object ) -> destination_pb2.Destination.Teardown.Response: diff --git a/src/conduit/serve.py b/src/conduit/serve.py index 6c9f532..a7440e1 100644 --- a/src/conduit/serve.py +++ b/src/conduit/serve.py @@ -29,7 +29,7 @@ import signal import sys import threading -from collections.abc import Callable, Mapping +from collections.abc import Awaitable, Callable, Mapping from dataclasses import dataclass from typing import Any, NoReturn, TextIO @@ -266,6 +266,12 @@ class _ServerHandle: connector_instance: Source[Any] | Destination[Any] shutdown_requested: asyncio.Event drive_task: asyncio.Task[None] + drain: Callable[[], Awaitable[None]] + """Bound ``_SourceServicer.drain``/``_DestinationServicer.drain`` for + whichever of the two was registered -- stops the active read/write loop + from accepting new work and awaits any operation already in flight. Used + by ``_sigterm_shutdown`` (invariant 7) to drain the running connector + before ``teardown()``; also usable directly by tests.""" async def _build_plugin_server( @@ -318,27 +324,32 @@ async def _build_plugin_server( instance: Source[Any] | Destination[Any] source_params: Mapping[str, Any] = {} destination_params: Mapping[str, Any] = {} + drain: Callable[[], Awaitable[None]] if source is not None: config_cls = _resolve_source_config_class(source) instance = source() + source_servicer = _SourceServicer(instance, config_cls) # The generated `*_pb2_grpc.py` files carry no type annotations (no # companion `.pyi` for the service-registration helpers, only for # the message types) -- calling into them from this strict-mode # module is an intentional, vendored-codegen boundary, not a typing # gap in our own code (see pyproject.toml's mypy overrides comment). source_pb2_grpc.add_SourcePluginServicer_to_server( # type: ignore[no-untyped-call] - _SourceServicer(instance, config_cls), server + source_servicer, server ) source_params = to_parameters(config_cls) + drain = source_servicer.drain else: assert destination is not None # narrowed by the xor check above config_cls = _resolve_destination_config_class(destination) instance = destination() + destination_servicer = _DestinationServicer(instance, config_cls) destination_pb2_grpc.add_DestinationPluginServicer_to_server( # type: ignore[no-untyped-call] - _DestinationServicer(instance, config_cls), server + destination_servicer, server ) destination_params = to_parameters(config_cls) + drain = destination_servicer.drain specifier_pb2_grpc.add_SpecifierPluginServicer_to_server( # type: ignore[no-untyped-call] _SpecifierServicer(specification, source_params, destination_params), server @@ -393,28 +404,65 @@ def graceful_trigger() -> None: asyncio.run_coroutine_threadsafe(_sigterm_shutdown(), loop) async def _sigterm_shutdown() -> None: - """Run teardown, then unblock ``drive_shutdown`` -- unconditionally. - - **Bug this fixes, found while building/testing the ``build`` CLI - command (item 5): if ``teardown()`` itself raised (e.g. a - connector's ``teardown()`` accessing a resource ``open()`` never - got a chance to set up, because SIGTERM arrived before Conduit - ever called ``Open``), the exception used to propagate out of this - coroutine. Since it runs via ``run_coroutine_threadsafe`` with its - returned ``Future`` never awaited/checked (a signal handler has - nothing to await), that exception was silently swallowed -- - ``shutdown_requested`` was never set, ``drive_shutdown`` waited - forever, and the hung-loop watchdog fired and force-exited with a - "wedged event loop" diagnostic that was actively misleading: the - loop was never wedged, a plain exception was just never observed. + """Drain the active read/write loop, run teardown, then unblock ``drive_shutdown``. + + Runs unconditionally, regardless of whether draining or teardown + raise -- see the "Bug this also still fixes" section below. + + **Invariant 7 (graceful shutdown by default) gap this closes:** this + coroutine used to call ``run_teardown_once()`` immediately on + ``SIGTERM``, with no regard for whether ``Run`` was actively + streaming -- ``teardown()`` (e.g. closing a DB pool) could then run + concurrently with an in-flight ``Source.read()``/``Destination. + write()``, racing resource cleanup against live I/O. ``drain()`` + (``_SourceServicer.drain``/``_DestinationServicer.drain``, whichever + was registered) is awaited first: it performs the same + stop-the-loop-then-wait-for-it ordering the deterministic path + already gets for free (Conduit's own ``Stop`` RPC → ``Run`` ends → + ``Teardown`` RPC), so a SIGTERM-triggered shutdown drains an + in-flight operation before teardown runs, not concurrently with it. + This is not a data-loss fix -- a write raised mid-flight already + nacks the whole batch (see ``_DestinationServicer._write_batch``) + and Conduit redelivers on restart either way -- it is what makes + this SDK's SIGTERM path actually graceful rather than merely + forward-progressing. The hung-loop watchdog (▶ MUST-FIX 3) still + bounds how long this can take: it was already started, on its own + thread, before this coroutine was even scheduled (see + ``_ShutdownCoordinator._on_sigterm``), so a ``drain()`` that never + returns (a genuinely wedged read/write) still force-exits on + schedule rather than hanging forever. + + **Bug this also still fixes, found while building/testing the + ``build`` CLI command (item 5): if ``drain()`` or ``teardown()`` + itself raised (e.g. a connector's ``teardown()`` accessing a + resource ``open()`` never got a chance to set up, because SIGTERM + arrived before Conduit ever called ``Open``), the exception used to + propagate out of this coroutine. Since it runs via + ``run_coroutine_threadsafe`` with its returned ``Future`` never + awaited/checked (a signal handler has nothing to await), that + exception was silently swallowed -- ``shutdown_requested`` was + never set, ``drive_shutdown`` waited forever, and the hung-loop + watchdog fired and force-exited with a "wedged event loop" + diagnostic that was actively misleading: the loop was never + wedged, a plain exception was just never observed. ``shutdown_requested.set()`` in a ``finally`` block makes forward - progress on shutdown unconditional -- a buggy ``teardown()`` no - longer blocks the *entire* SIGTERM-triggered graceful path; it only - loses the (already-lost, since it raised) cleanup it was supposed - to do, and this prints a clear diagnostic distinguishing "teardown - raised" from "loop genuinely wedged" rather than reaching the - watchdog's generic message at all. + progress on shutdown unconditional -- a buggy ``drain()``/ + ``teardown()`` no longer blocks the *entire* SIGTERM-triggered + graceful path; it only loses the (already-lost, since it raised) + cleanup it was supposed to do, and this prints a clear diagnostic + distinguishing "drain/teardown raised" from "loop genuinely + wedged" rather than reaching the watchdog's generic message at all. """ + try: + await drain() + except Exception as exc: + print( + f"conduit-sdk: draining the in-flight read/write loop raised " + f"during SIGTERM-triggered shutdown: {exc!r} -- running " + "teardown() anyway rather than skipping it", + file=stderr, + flush=True, + ) try: await run_teardown_once() except Exception as exc: @@ -449,6 +497,7 @@ async def drive_shutdown() -> None: connector_instance=instance, shutdown_requested=shutdown_requested, drive_task=drive_task, + drain=drain, ) diff --git a/src/conduit/source.py b/src/conduit/source.py index 2b19ce2..c3f0dd7 100644 --- a/src/conduit/source.py +++ b/src/conduit/source.py @@ -331,10 +331,30 @@ async def Stop( self, request: source_pb2.Source.Stop.Request, context: object ) -> source_pb2.Source.Stop.Response: """Signal the read loop to stop; block until it has, then report the last position.""" + await self.drain() + return source_pb2.Source.Stop.Response(last_position=self._last_position) + + async def drain(self) -> None: + """Signal the read loop to stop and await it fully stopping. + + Invariant 7 (graceful shutdown by default) enforcement site: called + by ``conduit.serve``'s SIGTERM handler before ``teardown()`` runs, so + an in-flight ``read()`` (and the read loop it drives) never races + ``teardown()`` closing a resource the loop is still using. + + The same stop-then-wait sequence :meth:`Stop` performs for the + deterministic Conduit-driven path (``Stop`` RPC → ``Run`` ends → + ``Teardown`` RPC), factored out so ``conduit.serve``'s SIGTERM + handler can perform the identical ordered drain before running + ``teardown()`` -- invariant 7 (graceful shutdown by default) applies + just as much to a SIGTERM-triggered shutdown as to the deterministic + one; only the RPC that observes the drain differs. If ``Run`` was + never invoked (e.g. SIGTERM arrives before Conduit ever calls it), + this returns immediately -- there is no read loop to wait for. + """ self._stop_event.set() if self._run_started: await self._stopped_event.wait() - return source_pb2.Source.Stop.Response(last_position=self._last_position) async def Teardown( self, request: source_pb2.Source.Teardown.Request, context: object diff --git a/tests/test_serve.py b/tests/test_serve.py index 60da9a4..c56dab0 100644 --- a/tests/test_serve.py +++ b/tests/test_serve.py @@ -1,5 +1,6 @@ -"""Tests for :mod:`conduit.serve` -- deterministic shutdown (▶ MUST-FIX 2) -and the hung-event-loop watchdog (▶ MUST-FIX 3). +"""Tests for :mod:`conduit.serve` -- deterministic shutdown (▶ MUST-FIX 2), +the hung-event-loop watchdog (▶ MUST-FIX 3), and the SIGTERM-triggered +in-flight-operation drain (invariant 7). Per the design doc's tightened Phase-1 acceptance criterion: the shutdown test must be a deterministic RPC-invocation assertion, not a timing/log @@ -8,6 +9,13 @@ connects a real gRPC client to it, calls ``Shutdown``, and asserts (a) the RPC succeeds and (b) ``teardown()`` ran to completion beforehand -- via a spy, not a race against a clock. + +``TestSigtermDrainsInFlightOperation`` holds the same bar for the +SIGTERM-triggered path: ``tests/test_build.py``'s only SIGTERM test fires the +signal *before* ``Open()`` is ever called, so it has never exercised draining +an in-flight ``read()``/``write()`` -- see this module's ``_sigterm_shutdown`` +docstring and the design doc's Risks & open questions §3 for the gap this +class closes. """ from __future__ import annotations @@ -15,24 +23,28 @@ import asyncio import contextlib import io +import signal import threading import time +from collections.abc import AsyncIterator import grpc import grpc.aio import pytest from google.protobuf import empty_pb2 +from conduit._grpc.adapters import record_to_proto from conduit.config import BaseConfig, Specification from conduit.destination import Destination from conduit.errors import BackoffRetry -from conduit.record import Record +from conduit.record import Operation, Record from conduit.serve import ( DEFAULT_SHUTDOWN_DEADLINE_SECONDS, _build_plugin_server, _ShutdownCoordinator, ) from conduit.source import Source +from connector.v2 import destination_pb2, destination_pb2_grpc, source_pb2, source_pb2_grpc _SPEC = Specification(name="test-plugin", version="0.0.0", author="test") @@ -63,6 +75,63 @@ async def teardown(self) -> None: self.teardown_calls += 1 +class _SlowReadSource(Source[_Config]): + """A ``Source`` whose first ``read()`` blocks until the test releases it. + + ``events`` records, in order, ``"read_end"`` (appended by ``read()`` + itself, just before returning) and ``"teardown"`` (appended by + ``teardown()``) -- the exact ordering + ``TestSigtermDrainsInFlightOperation`` asserts on. + """ + + def __init__(self) -> None: + self.events: list[str] = [] + self.read_started = asyncio.Event() + self.read_may_finish = asyncio.Event() + self._call_count = 0 + + async def read(self) -> Record: + self._call_count += 1 + if self._call_count > 1: + # The read loop must not call `read()` again after `drain()` + # has set `_stop_event` -- a second call here would mean the + # SIGTERM-triggered drain failed to stop the loop before + # teardown() ran. Raise instead of blocking forever so a + # regression here fails fast (a hang) rather than silently + # (a wrong ack). + raise BackoffRetry() + self.read_started.set() + await self.read_may_finish.wait() + self.events.append("read_end") + return Record(position=b"pos-1", operation=Operation.CREATE) + + async def teardown(self) -> None: + self.events.append("teardown") + + +class _SlowWriteDestination(Destination[_Config]): + """A ``Destination`` whose ``write()`` blocks until the test releases it. + + ``events`` records, in order, ``"write_end"`` (appended by ``write()`` + itself, just before returning) and ``"teardown"`` (appended by + ``teardown()``) -- the exact ordering + ``TestSigtermDrainsInFlightOperation`` asserts on. + """ + + def __init__(self) -> None: + self.events: list[str] = [] + self.write_started = asyncio.Event() + self.write_may_finish = asyncio.Event() + + async def write(self, records: list[Record]) -> None: + self.write_started.set() + await self.write_may_finish.wait() + self.events.append("write_end") + + async def teardown(self) -> None: + self.events.append("teardown") + + async def _call_shutdown(port: int) -> empty_pb2.Empty: channel = grpc.aio.insecure_channel(f"127.0.0.1:{port}") try: @@ -266,3 +335,132 @@ def test_serve_requires_exactly_one_of_source_or_destination() -> None: with pytest.raises(ValueError, match="exactly one"): serve(_SPEC, source=_TeardownSpySource, destination=_TeardownSpyDestination) + + +async def _empty_ack_stream() -> AsyncIterator[source_pb2.Source.Run.Request]: + return + yield # pragma: no cover -- makes this an async generator with no items + + +class TestSigtermDrainsInFlightOperation: + """The gap found in Tier-1 review: ``_sigterm_shutdown`` used to call + + ``teardown()`` immediately on SIGTERM with no regard for an actively + streaming ``Run()`` -- never signaling the read/write loop to stop, never + awaiting an in-flight ``read()``/``write()``. These tests build the SDK's + real ``grpc.aio`` server, drive a real bidi-streaming ``Run()`` call + against it, block the connector mid-``read()``/mid-``write()``, then + invoke the coordinator's real ``_on_sigterm`` entry point directly + (deterministic -- not waiting on OS signal-delivery timing, which + ``tests/test_build.py``'s subprocess-level SIGTERM test already covers + for the before-``Open`` case) and assert the in-flight operation + completes strictly *before* ``teardown()`` runs, never concurrently with + it. + """ + + async def test_sigterm_mid_read_drains_before_teardown(self) -> None: + exit_calls: list[int] = [] + handle = await _build_plugin_server( + _SPEC, source=_SlowReadSource, exit_fn=exit_calls.append + ) + spy = handle.connector_instance + assert isinstance(spy, _SlowReadSource) + + channel = grpc.aio.insecure_channel(f"127.0.0.1:{handle.port}") + try: + stub = source_pb2_grpc.SourcePluginStub(channel) + call = stub.Run(_empty_ack_stream()) + responses: list[source_pb2.Source.Run.Response] = [] + + async def _consume() -> None: + async for response in call: + responses.append(response) + + consume_task = asyncio.create_task(_consume()) + + # Wait until `read()` is genuinely in flight (blocked inside the + # call, not merely "the RPC started"). + await asyncio.wait_for(spy.read_started.wait(), timeout=2) + + # Simulate SIGTERM landing *while `read()` is in flight* -- the + # exact scenario the gap missed. Calling the coordinator's real + # signal-handler method directly (rather than `os.kill`) keeps + # this deterministic; it is the same code a real SIGTERM would + # invoke (`signal.signal`'s registered callback). + handle.coordinator._on_sigterm(signal.SIGTERM, None) + + # No matter how much the event loop is given to run here, + # `teardown()` cannot legitimately have appended to `events` yet + # -- `read()` is still blocked on `read_may_finish`, which + # nothing but this test can set. If the gap this test targets + # regressed (teardown() racing the in-flight read), this is + # where it would show up. + await asyncio.sleep(0.05) + assert spy.events == [] + + # Let the in-flight read() complete. + spy.read_may_finish.set() + + await asyncio.wait_for(handle.drive_task, timeout=2) + await asyncio.wait_for(consume_task, timeout=2) + + assert spy.events == ["read_end", "teardown"] + assert handle.coordinator.is_confirmed + assert len(responses) == 1 + assert exit_calls == [] # the watchdog must never have fired + finally: + await channel.close() + with contextlib.suppress(Exception): + await handle.server.stop(None) + + async def test_sigterm_mid_write_drains_before_teardown(self) -> None: + exit_calls: list[int] = [] + handle = await _build_plugin_server( + _SPEC, destination=_SlowWriteDestination, exit_fn=exit_calls.append + ) + spy = handle.connector_instance + assert isinstance(spy, _SlowWriteDestination) + + channel = grpc.aio.insecure_channel(f"127.0.0.1:{handle.port}") + try: + stub = destination_pb2_grpc.DestinationPluginStub(channel) + record = Record(position=b"pos-1", operation=Operation.CREATE) + + async def _one_batch() -> AsyncIterator[destination_pb2.Destination.Run.Request]: + yield destination_pb2.Destination.Run.Request(records=[record_to_proto(record)]) + + call = stub.Run(_one_batch()) + acks: list[destination_pb2.Destination.Run.Response] = [] + + async def _consume() -> None: + async for response in call: + acks.append(response) + + consume_task = asyncio.create_task(_consume()) + + # Wait until `write()` is genuinely in flight. + await asyncio.wait_for(spy.write_started.wait(), timeout=2) + + # Simulate SIGTERM landing *while `write()` is in flight*. + handle.coordinator._on_sigterm(signal.SIGTERM, None) + + # Same reasoning as the read case: `teardown()` cannot + # legitimately have run yet -- `write()` is still blocked on + # `write_may_finish`. + await asyncio.sleep(0.05) + assert spy.events == [] + + # Let the in-flight write() complete. + spy.write_may_finish.set() + + await asyncio.wait_for(handle.drive_task, timeout=2) + await asyncio.wait_for(consume_task, timeout=2) + + assert spy.events == ["write_end", "teardown"] + assert handle.coordinator.is_confirmed + assert len(acks) == 1 + assert exit_calls == [] # the watchdog must never have fired + finally: + await channel.close() + with contextlib.suppress(Exception): + await handle.server.stop(None)