[feat](extensions) Publish signed extension repository - #622
kaka11chen wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ae1b939e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _atomic_write(destination / _INDEX_FILE_NAME, index_bytes) | ||
| _atomic_write(destination / _SIGNATURE_FILE_NAME, _canonical_json_bytes(signature) + b"\n") |
There was a problem hiding this comment.
If publishing is interrupted after the first _atomic_write, repository.json contains the new generation while repository.sig.json still signs the old one. Every installer then rejects the repository, and rerunning the publisher cannot repair it because _read_existing_manifest rejects the mismatched pair before publishing. Serving the output directory directly also exposes this mismatch to concurrent readers, so the signed metadata needs a single atomic generation/pointer update.
Useful? React with 👍 / 👎.
| def to_dict(self) -> dict[str, object]: | ||
| return { | ||
| "artifacts": [artifact.to_dict() for artifact in self.artifacts], | ||
| "format_version": _REPOSITORY_FORMAT_VERSION, | ||
| "repository_id": self.repository_id, |
There was a problem hiding this comment.
If a repository host, mirror, or cache replays an older correctly signed index, the client has no signed generation or local checkpoint with which to detect the rollback. This can make newer releases disappear and, when extension_version is omitted, silently select an older previously approved extension; Ed25519 verification alone only proves who signed the stale state. Include a monotonic signed repository version and persist/enforce the highest observed value.
Useful? React with 👍 / 👎.
| vane_version = _validate_wheel_version(vane.__version__) | ||
| distribution_name = f"vane-extension-{name}" | ||
| distribution_root = f"vane_extension_{name}-{vane_version}" | ||
| wheel_tag = f"py3-none-{normalized_platform_tag}" | ||
| output_path = Path(output_directory).expanduser().resolve() / f"{distribution_root}-{wheel_tag}.whl" |
There was a problem hiding this comment.
When two extension releases are built for the same Vane version, name, and platform, both receive the identical wheel filename and distribution version because descriptor.extension_version is not represented here. The second build is therefore rejected as a conflicting existing wheel, and an artifact index cannot retain both independently distributed releases under distinct filenames, despite the repository supporting exact extension-version selection. Encode the extension release in a valid wheel version or build tag.
Useful? React with 👍 / 👎.
| descriptor = create_dynamic_extension_descriptor( | ||
| artifact_path, | ||
| name=name, | ||
| trust_identity=trust_identity, | ||
| ) |
There was a problem hiding this comment.
For an extension that depends on another dynamic extension, the supported wheel builder always calls create_dynamic_extension_descriptor without its dependencies argument, producing an empty dependency tuple. Publishing that wheel therefore gives the installer no closure to download, and DynamicExtensionResolver will attempt to resolve/load only the root artifact. Expose dependency descriptors through this API and CLI, or derive them from authoritative build metadata.
Useful? React with 👍 / 👎.
| response = urllib.request.urlopen(request, timeout=self._timeout_seconds) | ||
| except (urllib.error.URLError, OSError) as exception: | ||
| raise DynamicExtensionError( | ||
| "REPOSITORY_DOWNLOAD_FAILED", f"could not download {label} from configured repository" | ||
| ) from exception | ||
| response_url = response.geturl() | ||
| if not _url_is_within_repository(response_url, self._repository_url): |
There was a problem hiding this comment.
If the pinned repository returns a redirect to an external HTTPS host and that host redirects back under the repository URL, urllib.request.urlopen follows both hops automatically and response.geturl() reports only the final in-repository URL. The check therefore passes even though the client made an outbound request outside the pinned repository, violating the stated repository and network boundary. Use a redirect handler that validates each target before following it.
Useful? React with 👍 / 👎.
| "REPOSITORY_DESCRIPTOR_MISMATCH", | ||
| f"repository descriptor file does not match signed index for {entry.descriptor.identity}", | ||
| ) | ||
| _atomic_write(descriptor_cache_path, descriptor_bytes) |
There was a problem hiding this comment.
When installation cannot create or replace the cached descriptor—for example because the cache is read-only or full—this call reaches _atomic_write, whose hard-coded exception path raises REPOSITORY_PUBLISH_FAILED and says it could not write a repository index. Consumers consequently receive the wrong public error category during an install and cannot handle cache failures consistently with the artifact-write path. Give cache writes their own error code/message or parameterize the helper.
Useful? React with 👍 / 👎.
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| with path.open("xb") as output: | ||
| output.write(data) |
There was a problem hiding this comment.
If output.write(data) fails partway through because the process is interrupted or the filesystem runs out of space, the newly created immutable path remains as a truncated file. A later publish cannot recover: opening with xb raises FileExistsError, the byte comparison fails, and the publisher refuses to replace the partial file, requiring manual repository repair. Write to a sibling temporary file and atomically install it only after the complete contents are durable.
Useful? React with 👍 / 👎.
| existing_entry = entries.get(descriptor.identity) | ||
| if existing_entry is not None and existing_entry != entry: | ||
| _fail("REPOSITORY_PUBLISH_FAILED", f"repository identity changed paths: {descriptor.identity}") |
There was a problem hiding this comment.
When the same byte-identical artifact is packaged for another Vane runtime—particularly a stable C_STRUCT extension whose footer and binary can legitimately be reused—the new descriptor has the same name, extension version, and SHA-256 identity but a different vane_version or duckdb_source_id. This lookup treats it as the existing entry and then rejects the differing descriptor paths, so the append-only repository cannot publish that artifact for both supported runtimes. Key repository entries and dependency references by the complete immutable descriptor coordinate rather than only the artifact identity.
Useful? React with 👍 / 👎.
Summary
.duckdb_extensionbytes from independent platform wheels into an append-only, Ed25519-signed Vane repository index.extensionsextra, provide publish/install CLIs and development documentation, and keep Ray repository transport out of worker execution.This PR stacks on the platform-wheel contract in #620. Its GitHub base is #618's branch because #620 is fork-only and is not a branch in the upstream repository; it should merge after #620.
Related issue
close: #614
Documentation impact
AstroVela/vane-website.DEVELOPMENT.mdnow covers signed repository publishing, key pinning, local cache recovery, and the Ray network boundary.Validation
scripts/format root --changedandpre-commit run --files ...(Ruff, Mypy, YAML/TOML, private-key scan, and merge-conflict checks).CMAKE_BUILD_PARALLEL_LEVEL=16,VANE_NATIVE_BUILD_JOBS=16, shared vcpkg Arrow/OpenSSL paths, anduv pip install '.[extensions]' --no-build-isolation.scripts/run_installed_pytest.sh tests/fast/test_extension_repository.py tests/fast/test_package_metadata.py— 34 passed, 1 optional ADBC skip.scripts/run_release_tests.sh— completed through isolated non-Ray and real-Ray shards.Checklist
cryptographyis a compatible optional dependency declared in package metadata and the type-check hook.