From ae7601f74cee299e420a0bb8639d83ed95234e31 Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Fri, 31 Jul 2026 11:18:07 -0300 Subject: [PATCH 01/15] Add client migration tests to CI workflows and Justfile - Fix C2 item from migration gaps review --- .github/workflows/post-release-tests.yml | 5 +++++ .github/workflows/unit-tests.yml | 3 +++ Justfile | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/.github/workflows/post-release-tests.yml b/.github/workflows/post-release-tests.yml index 2b71535e4d7..16229a3bd73 100644 --- a/.github/workflows/post-release-tests.yml +++ b/.github/workflows/post-release-tests.yml @@ -62,3 +62,8 @@ jobs: run: | source .venv/bin/activate pytest -n auto ./tests/unit + + - name: Run client migration tests + run: | + source .venv/bin/activate + pytest -n auto ./tests/migrations diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index bce442931be..41ae0f7fdb6 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -198,3 +198,6 @@ jobs: - name: Run migration tests run: just test-unit-migration + + - name: Run client migration tests + run: just test-client-migrations diff --git a/Justfile b/Justfile index 74fcb344912..3f664e39f6c 100644 --- a/Justfile +++ b/Justfile @@ -38,6 +38,10 @@ test-unit-migration: #!/bin/bash uv run pytest -n auto ./packages/syft-migration/tests +test-client-migrations: + #!/bin/bash + uv run pytest -n auto ./tests/migrations + test-unit-enclave: #!/bin/bash From c67d6e90579212a5eb97cd3b89f8ac140634e281 Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Tue, 4 Aug 2026 18:01:40 -0300 Subject: [PATCH 02/15] Implement version ordering for migration objects - Fix D1 item from migration gaps review --- .../src/syft_migration/identity.py | 15 +++ .../src/syft_migration/registry.py | 13 ++- .../src/syft_migration/schema.py | 9 +- .../tests/test_version_ordering.py | 103 ++++++++++++++++++ 4 files changed, 133 insertions(+), 7 deletions(-) create mode 100644 packages/syft-migration/tests/test_version_ordering.py diff --git a/packages/syft-migration/src/syft_migration/identity.py b/packages/syft-migration/src/syft_migration/identity.py index b9f2a2d9066..6ab95ab81d4 100644 --- a/packages/syft-migration/src/syft_migration/identity.py +++ b/packages/syft-migration/src/syft_migration/identity.py @@ -23,6 +23,21 @@ def _has_identity(cls: type[MigratableObject]) -> bool: return not (name_field.is_required() or version_field.is_required()) +def _version_order(version: str) -> int: + """Return the sort key of an object version. + + Object versions are incrementing integers held as strings. A string sort puts + ``"10"`` before ``"2"``, so every comparison must use this key. + """ + try: + return int(version) + except ValueError: + raise MigrationError( + f"Object version {version!r} is not an integer. Object versions are " + "incrementing integers, for example '1', '2', '3'." + ) from None + + def _identity(cls: type[MigratableObject]) -> tuple[str, str]: """Return (canonical_name, version) for a concrete subclass. diff --git a/packages/syft-migration/src/syft_migration/registry.py b/packages/syft-migration/src/syft_migration/registry.py index 7c81859ac77..b445dd48a51 100644 --- a/packages/syft-migration/src/syft_migration/registry.py +++ b/packages/syft-migration/src/syft_migration/registry.py @@ -3,7 +3,12 @@ from collections import deque from typing import TYPE_CHECKING, Callable -from syft_migration.identity import MigrationError, _has_identity, _identity +from syft_migration.identity import ( + MigrationError, + _has_identity, + _identity, + _version_order, +) from syft_migration.schema import ( PackageInfo, ProtocolSchema, @@ -49,6 +54,8 @@ def register_object_version(self, cls: type[MigratableObject]) -> None: if not _has_identity(cls): return canonical_name, version = _identity(cls) + # Reject a version that cannot be ordered, at class definition time. + _version_order(version) existing = self.objects.get(canonical_name, {}).get(version) if existing is not None and existing is not cls: raise MigrationError( @@ -72,7 +79,7 @@ def latest_version(self, canonical_name: str) -> str: versions = self.versions(canonical_name) if not versions: raise MigrationError(f"No versions registered for {canonical_name!r}") - return max(versions) + return max(versions, key=_version_order) # -- migrations -------------------------------------------------------- def register_migration( @@ -206,7 +213,7 @@ def compute_protocol_schema(self) -> ProtocolSchema: protocol_name=self.protocol_name, version=self.protocol_version, supported_versions={ - canonical_name: sorted(versions) + canonical_name: sorted(versions, key=_version_order) for canonical_name, versions in self.objects.items() }, current_object_schemas={ diff --git a/packages/syft-migration/src/syft_migration/schema.py b/packages/syft-migration/src/syft_migration/schema.py index b68bed5ccf4..0b07c1b8458 100644 --- a/packages/syft-migration/src/syft_migration/schema.py +++ b/packages/syft-migration/src/syft_migration/schema.py @@ -6,7 +6,7 @@ from pydantic import BaseModel -from syft_migration.identity import MigrationError, _identity +from syft_migration.identity import MigrationError, _identity, _version_order if TYPE_CHECKING: from syft_migration.base import MigratableObject @@ -45,13 +45,14 @@ def from_objects( versions = supported_versions.setdefault(canonical_name, []) if object_version not in versions: versions.append(object_version) - if object_version == max(versions): + if object_version == max(versions, key=_version_order): latest_classes[canonical_name] = klass return cls( protocol_name=protocol_name, version=version, supported_versions={ - name: sorted(versions) for name, versions in supported_versions.items() + name: sorted(versions, key=_version_order) + for name, versions in supported_versions.items() }, current_object_schemas={ name: klass.model_json_schema() @@ -64,7 +65,7 @@ def current_schema(self, canonical_name: str) -> str: versions = self.supported_versions.get(canonical_name) if not versions: raise MigrationError(f"Schema does not include object {canonical_name!r}") - return max(versions) + return max(versions, key=_version_order) def save(self, path: PathLike) -> None: Path(path).write_text(self.model_dump_json(indent=2)) diff --git a/packages/syft-migration/tests/test_version_ordering.py b/packages/syft-migration/tests/test_version_ordering.py new file mode 100644 index 00000000000..c9f3937aafc --- /dev/null +++ b/packages/syft-migration/tests/test_version_ordering.py @@ -0,0 +1,103 @@ +"""Object versions order by number, not as strings.""" + +import pytest + +from syft_migration import ( + MigratableObject, + MigrationError, + MigrationRegistry, + ProtocolSchema, +) + + +def _registry() -> MigrationRegistry: + return MigrationRegistry( + protocol_name="p", + package_name="pkg", + package_version="1.0.0", + protocol_version="1", + ) + + +def _two_digit_registry() -> tuple[ + MigrationRegistry, type[MigratableObject], type[MigratableObject] +]: + """A registry with version 2 and version 10 of the same object.""" + reg = _registry() + + class ThingV2(MigratableObject, registry=reg): + canonical_name: str = "thing" + version: str = "2" + + class ThingV10(MigratableObject, registry=reg): + canonical_name: str = "thing" + version: str = "10" + extra: int = 0 + + return reg, ThingV2, ThingV10 + + +def test_latest_version_orders_by_number(): + reg, _, _ = _two_digit_registry() + assert reg.latest_version(canonical_name="thing") == "10" + + +def test_computed_schema_freezes_the_highest_version(): + # find_schema_drift compares the frozen schema of the highest version. A + # string order freezes version 2 and leaves version 10 unguarded. + reg, _, thing_v10 = _two_digit_registry() + schema = reg.compute_protocol_schema() + assert schema.supported_versions == {"thing": ["2", "10"]} + assert schema.current_object_schemas["thing"] == thing_v10.model_json_schema() + + +def test_current_schema_orders_by_number(): + schema = ProtocolSchema( + protocol_name="p", + version="1", + supported_versions={"thing": ["2", "10"]}, + ) + assert schema.current_schema(canonical_name="thing") == "10" + + +@pytest.mark.parametrize("reverse", [False, True]) +def test_from_objects_picks_the_highest_version(reverse): + _, thing_v2, thing_v10 = _two_digit_registry() + classes = [thing_v10, thing_v2] if reverse else [thing_v2, thing_v10] + schema = ProtocolSchema.from_objects( + protocol_name="p", + version="1", + classes=classes, + ) + assert schema.supported_versions == {"thing": ["2", "10"]} + assert schema.current_object_schemas["thing"] == thing_v10.model_json_schema() + + +def test_upgradeable_path_targets_the_highest_version(): + reg, _, _ = _two_digit_registry() + + # Version 3 has no migration, so it cannot reach version 10. A string order + # makes version 3 the latest and reports the path as trivially available. + class ThingV3(MigratableObject, registry=reg): + canonical_name: str = "thing" + version: str = "3" + + reg.register_migration( + canonical_name="thing", + from_version="2", + to_version="10", + fn=lambda obj: obj, + ) + assert reg.has_upgradeable_path_to_latest(canonical_name="thing", from_version="2") + assert not reg.has_upgradeable_path_to_latest( + canonical_name="thing", from_version="3" + ) + + +def test_non_numeric_object_version_is_rejected(): + reg = _registry() + with pytest.raises(MigrationError): + + class ThingV1Patch(MigratableObject, registry=reg): + canonical_name: str = "thing" + version: str = "1.0" From b612d3f76e12507c239e10536ec0acb2522fc935 Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Tue, 4 Aug 2026 22:12:34 -0300 Subject: [PATCH 03/15] Publish before bumping and run the release artifact export in CD - Fix C1 item from migration gaps review - Pin dependents to the published version, not the bumped one, so a package released later in the same run does not need a version PyPI lacks --- .github/workflows/cd-monorepo.yml | 30 +++++-- .github/workflows/cd-syft-bg.yml | 24 ++--- .github/workflows/cd-syft-dataset.yml | 33 ++++--- .github/workflows/cd-syft-job.yml | 33 ++++--- .github/workflows/cd-syft-permissions.yml | 24 ++--- .github/workflows/cd-syft-perms.yml | 24 ++--- Justfile | 9 +- docs/release.md | 36 +++++++- .../scripts/export_release_artifact.py | 25 +++++- .../migrations/unit/test_history_artifacts.py | 17 +++- .../scripts/export_release_artifact.py | 28 +++++- .../migrations/unit/test_history_artifacts.py | 10 +++ .../src/syft_migration/registry.py | 22 +++++ .../tests/test_release_artifacts.py | 75 ++++++++++++++++ scripts/bump_version.py | 42 +++++++-- scripts/export_release_artifact.py | 37 ++++---- .../migrations/unit/test_history_artifacts.py | 10 +++ tests/unit/test_bump_version.py | 88 +++++++++++++++++++ 18 files changed, 465 insertions(+), 102 deletions(-) create mode 100644 tests/unit/test_bump_version.py diff --git a/.github/workflows/cd-monorepo.yml b/.github/workflows/cd-monorepo.yml index 977c7556024..b27ceb12881 100644 --- a/.github/workflows/cd-monorepo.yml +++ b/.github/workflows/cd-monorepo.yml @@ -146,22 +146,38 @@ jobs: git config user.name "github-actions[bot]" git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + # The branch already holds the version to release. The bump happens after + # the publish, so the version on the branch names an unreleased version + - name: Read the version to release + run: | + git pull + VERSION=$(python3 syft_client/version.py) + echo "VERSION=$VERSION" >> $GITHUB_ENV + echo "Releasing syft-client $VERSION" + + # Artifacts live inside the package, so they must be committed before the + # build. Re-running the export writes nothing when they already exist. + - name: Freeze release artifacts + run: | + just export-release-artifacts + git add syft_client/migrations/history + git diff --cached --quiet || \ + git commit -m "Freeze syft-client v${{ env.VERSION }} release artifacts" + - name: Upload to PyPI id: publish env: TWINE_USERNAME: __token__ TWINE_PASSWORD: ${{ secrets.PYPI_PASS_SYFT_CLIENT }} run: | - git pull - just bump-and-publish ${{ inputs.bump_type }} - VERSION=$(python3 syft_client/version.py) - echo "VERSION=$VERSION" >> $GITHUB_ENV - echo "version=$VERSION" >> $GITHUB_OUTPUT + just publish + echo "version=${{ env.VERSION }}" >> $GITHUB_OUTPUT - # bump and publish already does committing - - name: Push changes to syft-client repo + # The tag must name the published version, so it is created before the bump. + - name: Tag the release, then bump for the next one run: | git tag "syft-client/v${{ env.VERSION }}" + just bump ${{ inputs.bump_type }} git push origin --follow-tags post-release-tests: diff --git a/.github/workflows/cd-syft-bg.yml b/.github/workflows/cd-syft-bg.yml index 365c55e85f5..e30fdf4aa7d 100644 --- a/.github/workflows/cd-syft-bg.yml +++ b/.github/workflows/cd-syft-bg.yml @@ -43,16 +43,15 @@ jobs: git config user.name "github-actions[bot]" git config user.email "41898282+github-actions[bot]@users.noreply.github.com" - - name: Bump version + # The branch already holds the version to release. The bump happens after + # the publish, so the version on the branch always names an unreleased + # version while people develop. + - name: Read the version to release run: | git pull - pip install packaging - OUTPUT=$(python scripts/bump_version.py syft-bg ${{ inputs.bump_type }}) - VERSION=$(echo "$OUTPUT" | sed -n '1p') - MODIFIED=$(echo "$OUTPUT" | sed -n '2p') + VERSION=$(python -c "import tomllib; print(tomllib.load(open('packages/syft-bg/pyproject.toml','rb'))['project']['version'])") echo "VERSION=$VERSION" >> $GITHUB_ENV - echo "MODIFIED=$MODIFIED" >> $GITHUB_ENV - echo "Bumped syft-bg to $VERSION (modified: $MODIFIED)" + echo "Releasing syft-bg $VERSION" - name: Build package working-directory: packages/syft-bg @@ -65,9 +64,14 @@ jobs: TWINE_PASSWORD: ${{ secrets.PYPI_PASS_SYFT_BG }} run: uvx twine upload --verbose dist/* - - name: Commit and tag + # The tag must name the published version, so it is created before the bump. + - name: Tag the release, then bump for the next one run: | - git add ${{ env.MODIFIED }} - git commit -m "Release syft-bg v${{ env.VERSION }}" git tag "syft-bg/v${{ env.VERSION }}" + pip install packaging + OUTPUT=$(python scripts/bump_version.py syft-bg ${{ inputs.bump_type }} --dependents published) + NEXT=$(echo "$OUTPUT" | sed -n '1p') + MODIFIED=$(echo "$OUTPUT" | sed -n '2p') + git add $MODIFIED + git commit -m "Bump syft-bg to $NEXT for the next release" git push origin --follow-tags diff --git a/.github/workflows/cd-syft-dataset.yml b/.github/workflows/cd-syft-dataset.yml index 8b572684918..f1a88f369c0 100644 --- a/.github/workflows/cd-syft-dataset.yml +++ b/.github/workflows/cd-syft-dataset.yml @@ -43,16 +43,24 @@ jobs: git config user.name "github-actions[bot]" git config user.email "41898282+github-actions[bot]@users.noreply.github.com" - - name: Bump version + # The branch already holds the version to release. The bump happens after + # the publish, so the version on the branch always names an unreleased + # version while people develop. + - name: Read the version to release run: | git pull - pip install packaging - OUTPUT=$(python scripts/bump_version.py syft-dataset ${{ inputs.bump_type }}) - VERSION=$(echo "$OUTPUT" | sed -n '1p') - MODIFIED=$(echo "$OUTPUT" | sed -n '2p') + VERSION=$(python -c "import tomllib; print(tomllib.load(open('packages/syft-datasets/pyproject.toml','rb'))['project']['version'])") echo "VERSION=$VERSION" >> $GITHUB_ENV - echo "MODIFIED=$MODIFIED" >> $GITHUB_ENV - echo "Bumped syft-dataset to $VERSION (modified: $MODIFIED)" + echo "Releasing syft-dataset $VERSION" + + # Artifacts live inside the package, so they must be committed before the + # build. Re-running the export writes nothing when they already exist. + - name: Freeze release artifacts + run: | + uv run python packages/syft-datasets/scripts/export_release_artifact.py + git add packages/syft-datasets/src/syft_datasets/migrations/history + git diff --cached --quiet || \ + git commit -m "Freeze syft-dataset v${{ env.VERSION }} release artifacts" - name: Build package working-directory: packages/syft-datasets @@ -65,9 +73,14 @@ jobs: TWINE_PASSWORD: ${{ secrets.PYPI_PASS_SYFT_DATASET }} run: uvx twine upload --verbose dist/* - - name: Commit and tag + # The tag must name the published version, so it is created before the bump. + - name: Tag the release, then bump for the next one run: | - git add ${{ env.MODIFIED }} - git commit -m "Release syft-dataset v${{ env.VERSION }}" git tag "syft-dataset/v${{ env.VERSION }}" + pip install packaging + OUTPUT=$(python scripts/bump_version.py syft-dataset ${{ inputs.bump_type }} --dependents published) + NEXT=$(echo "$OUTPUT" | sed -n '1p') + MODIFIED=$(echo "$OUTPUT" | sed -n '2p') + git add $MODIFIED + git commit -m "Bump syft-dataset to $NEXT for the next release" git push origin --follow-tags diff --git a/.github/workflows/cd-syft-job.yml b/.github/workflows/cd-syft-job.yml index a9bdb706b2a..bba4fc0c342 100644 --- a/.github/workflows/cd-syft-job.yml +++ b/.github/workflows/cd-syft-job.yml @@ -43,16 +43,24 @@ jobs: git config user.name "github-actions[bot]" git config user.email "41898282+github-actions[bot]@users.noreply.github.com" - - name: Bump version + # The branch already holds the version to release. The bump happens after + # the publish, so the version on the branch always names an unreleased + # version while people develop. + - name: Read the version to release run: | git pull - pip install packaging - OUTPUT=$(python scripts/bump_version.py syft-job ${{ inputs.bump_type }}) - VERSION=$(echo "$OUTPUT" | sed -n '1p') - MODIFIED=$(echo "$OUTPUT" | sed -n '2p') + VERSION=$(python -c "import tomllib; print(tomllib.load(open('packages/syft-job/pyproject.toml','rb'))['project']['version'])") echo "VERSION=$VERSION" >> $GITHUB_ENV - echo "MODIFIED=$MODIFIED" >> $GITHUB_ENV - echo "Bumped syft-job to $VERSION (modified: $MODIFIED)" + echo "Releasing syft-job $VERSION" + + # Artifacts live inside the package, so they must be committed before the + # build. Re-running the export writes nothing when they already exist. + - name: Freeze release artifacts + run: | + uv run python packages/syft-job/scripts/export_release_artifact.py + git add packages/syft-job/src/syft_job/migrations/history + git diff --cached --quiet || \ + git commit -m "Freeze syft-job v${{ env.VERSION }} release artifacts" - name: Build package working-directory: packages/syft-job @@ -65,9 +73,14 @@ jobs: TWINE_PASSWORD: ${{ secrets.PYPI_PASS_SYFT_JOB }} run: uvx twine upload --verbose dist/* - - name: Commit and tag + # The tag must name the published version, so it is created before the bump. + - name: Tag the release, then bump for the next one run: | - git add ${{ env.MODIFIED }} - git commit -m "Release syft-job v${{ env.VERSION }}" git tag "syft-job/v${{ env.VERSION }}" + pip install packaging + OUTPUT=$(python scripts/bump_version.py syft-job ${{ inputs.bump_type }} --dependents published) + NEXT=$(echo "$OUTPUT" | sed -n '1p') + MODIFIED=$(echo "$OUTPUT" | sed -n '2p') + git add $MODIFIED + git commit -m "Bump syft-job to $NEXT for the next release" git push origin --follow-tags diff --git a/.github/workflows/cd-syft-permissions.yml b/.github/workflows/cd-syft-permissions.yml index f34b0c686d9..49e01b19977 100644 --- a/.github/workflows/cd-syft-permissions.yml +++ b/.github/workflows/cd-syft-permissions.yml @@ -43,16 +43,15 @@ jobs: git config user.name "github-actions[bot]" git config user.email "41898282+github-actions[bot]@users.noreply.github.com" - - name: Bump version + # The branch already holds the version to release. The bump happens after + # the publish, so the version on the branch always names an unreleased + # version while people develop. + - name: Read the version to release run: | git pull - pip install packaging - OUTPUT=$(python scripts/bump_version.py syft-permissions ${{ inputs.bump_type }}) - VERSION=$(echo "$OUTPUT" | sed -n '1p') - MODIFIED=$(echo "$OUTPUT" | sed -n '2p') + VERSION=$(python -c "import tomllib; print(tomllib.load(open('packages/syft-permissions/pyproject.toml','rb'))['project']['version'])") echo "VERSION=$VERSION" >> $GITHUB_ENV - echo "MODIFIED=$MODIFIED" >> $GITHUB_ENV - echo "Bumped syft-permissions to $VERSION (modified: $MODIFIED)" + echo "Releasing syft-permissions $VERSION" - name: Build package working-directory: packages/syft-permissions @@ -65,9 +64,14 @@ jobs: TWINE_PASSWORD: ${{ secrets.PYPI_PASS_SYFT_PERMISSIONS }} run: uvx twine upload --verbose dist/* - - name: Commit and tag + # The tag must name the published version, so it is created before the bump. + - name: Tag the release, then bump for the next one run: | - git add ${{ env.MODIFIED }} - git commit -m "Release syft-permissions v${{ env.VERSION }}" git tag "syft-permissions/v${{ env.VERSION }}" + pip install packaging + OUTPUT=$(python scripts/bump_version.py syft-permissions ${{ inputs.bump_type }} --dependents published) + NEXT=$(echo "$OUTPUT" | sed -n '1p') + MODIFIED=$(echo "$OUTPUT" | sed -n '2p') + git add $MODIFIED + git commit -m "Bump syft-permissions to $NEXT for the next release" git push origin --follow-tags diff --git a/.github/workflows/cd-syft-perms.yml b/.github/workflows/cd-syft-perms.yml index f094d99e176..c8ba7d34902 100644 --- a/.github/workflows/cd-syft-perms.yml +++ b/.github/workflows/cd-syft-perms.yml @@ -43,16 +43,15 @@ jobs: git config user.name "github-actions[bot]" git config user.email "41898282+github-actions[bot]@users.noreply.github.com" - - name: Bump version + # The branch already holds the version to release. The bump happens after + # the publish, so the version on the branch always names an unreleased + # version while people develop. + - name: Read the version to release run: | git pull - pip install packaging - OUTPUT=$(python scripts/bump_version.py syft-perms ${{ inputs.bump_type }}) - VERSION=$(echo "$OUTPUT" | sed -n '1p') - MODIFIED=$(echo "$OUTPUT" | sed -n '2p') + VERSION=$(python -c "import tomllib; print(tomllib.load(open('packages/syft-perms/pyproject.toml','rb'))['project']['version'])") echo "VERSION=$VERSION" >> $GITHUB_ENV - echo "MODIFIED=$MODIFIED" >> $GITHUB_ENV - echo "Bumped syft-perms to $VERSION (modified: $MODIFIED)" + echo "Releasing syft-perms $VERSION" - name: Build package working-directory: packages/syft-perms @@ -65,9 +64,14 @@ jobs: TWINE_PASSWORD: ${{ secrets.PYPI_PASS_SYFT_PERMS }} run: uvx twine upload --verbose dist/* - - name: Commit and tag + # The tag must name the published version, so it is created before the bump. + - name: Tag the release, then bump for the next one run: | - git add ${{ env.MODIFIED }} - git commit -m "Release syft-perms v${{ env.VERSION }}" git tag "syft-perms/v${{ env.VERSION }}" + pip install packaging + OUTPUT=$(python scripts/bump_version.py syft-perms ${{ inputs.bump_type }} --dependents published) + NEXT=$(echo "$OUTPUT" | sed -n '1p') + MODIFIED=$(echo "$OUTPUT" | sed -n '2p') + git add $MODIFIED + git commit -m "Bump syft-perms to $NEXT for the next release" git push origin --follow-tags diff --git a/Justfile b/Justfile index 3f664e39f6c..b8bae8d4b8c 100644 --- a/Justfile +++ b/Justfile @@ -10,7 +10,6 @@ _nc := '\033[0m' alias b := build alias p := publish -alias bp:= bump-and-publish # --------------------------------------------------------------------------------------------------------------------- @@ -140,12 +139,10 @@ publish: build uvx twine upload dist/* @echo "{{ _green }}Publish complete!{{ _nc }}" -# Bump version and publish to PyPI +# Export the frozen release artifacts for the current version [group('publish')] -bump-and-publish part="patch": - just bump {{ part }} - just publish - @echo "{{ _green }}Bump and publish complete!{{ _nc }}" +export-release-artifacts: + uv run python scripts/export_release_artifact.py # Launch Jupyter Lab jupyter: diff --git a/docs/release.md b/docs/release.md index 2c842757c5f..be10831da81 100644 --- a/docs/release.md +++ b/docs/release.md @@ -2,16 +2,46 @@ ## Overview -Releases are managed through dedicated release branches. The mono repo release job handles bumping versions and pushing tags for all individual packages automatically. +Releases are managed through dedicated release branches. The mono repo release job handles publishing, tagging and bumping versions for all individual packages automatically. + +## Version order + +A release publishes the version that is **already on the branch**. The release then tags that version. After the tag, the release job bumps the version for the next release. + +The version on a branch is always a version that is **not yet published**. Therefore one version string always refers to one build. + +Do not change a version by hand before a release. The release job makes the bump. ## Steps -1. **Create a release branch** from `main`, dont include the patch version in the semver, so we can hotfix patches on the same branch (e.g. `release/v0.1`). If you are patching, re-use the branch +1. **Create a release branch** from `main`, don't include the patch version in the semver, so we can hotfix patches on the same branch (e.g. `release/v0.1`). If you are patching, re-use the branch. 2. **Run the release workflow.** You can trigger frmo github UI from the Actions tab. In most cases, release the mono repo — this releases all individual packages (`syft-client`, `syft-job`, `syft-dataset`, etc.) in one go. You only need to release individual packages if they are changed, but we are not detecting that automatically currently. 3. **Integration tests are optional.** You can skip them during the release if needed. Unit tests should still pass. -4. **Versions are bumped **before releasing to pypi** and pushed automatically** by the release process — no manual version edits required. +4. **The release job publishes, tags, and then bumps the version.** No manual version edit is necessary. 5. Merge the release branch back into `main` to ensure all version bumps and hotfixes are carried forward. +## Release artifacts + +`syft-client`, `syft-job`, and `syft-dataset` each write a release artifact. The artifact records the object versions of that release. It also records the exact schema of each object version. + +The drift check compares the current models against these files. If an artifact is absent, the drift check has nothing to compare for that version. + +The artifacts are inside the package, so the release job runs the export before the build: + +``` +uv run python scripts/export_release_artifact.py # syft-client +uv run python packages/syft-job/scripts/export_release_artifact.py # syft-job +uv run python packages/syft-datasets/scripts/export_release_artifact.py # syft-dataset +``` + +A developer can also run an export in a pull request. The version on the branch is the version that the next release publishes. The artifact is therefore available for review before the release. + +An artifact is permanent. If an artifact for a version exists, a second export writes nothing and reports success. + +An export stops with an error if the protocol changed but the protocol version constant did not change. The error message gives the name of the constant to bump. + +The drift check has one known limit. A new protocol generation adds object versions, and no artifact freezes those versions until the release of that generation. The drift check therefore cannot see a change to them. Frequent releases keep this period short. + ## Hotfixes If a fix is needed after cutting the release branch, apply the hotfix directly to the release branch and re-release from there. diff --git a/packages/syft-datasets/scripts/export_release_artifact.py b/packages/syft-datasets/scripts/export_release_artifact.py index f6a97db3bbd..2e35ebbb16c 100644 --- a/packages/syft-datasets/scripts/export_release_artifact.py +++ b/packages/syft-datasets/scripts/export_release_artifact.py @@ -3,6 +3,9 @@ Run on EVERY release (uv run python scripts/export_release_artifact.py): always writes the package release info; additionally writes the protocol artifact when this release introduces a new protocol version. + +Artifacts are frozen once written. Running this again for the same version +writes nothing and succeeds, so a release can re-run it safely. """ import sys @@ -16,6 +19,14 @@ def main() -> None: # Import the models so every versioned object is registered. import syft_datasets # noqa: F401 + if dataset_registry.protocol_bump_missing(): + sys.exit( + "The dataset protocol changed since the released " + f"protocol-{dataset_registry.latest_released_protocol_version()}.json; " + "bump DATASET_PROTOCOL_VERSION in " + "syft_datasets/migrations/registry.py before releasing." + ) + if dataset_registry.protocol_changed_without_bump(): sys.exit( "The dataset protocol changed compared to the released " @@ -28,11 +39,17 @@ def main() -> None: PROTOCOLS_DIR.mkdir(parents=True, exist_ok=True) info_path = PACKAGE_ARTIFACTS_DIR / f"syft-dataset-{__version__}.json" - dataset_registry.compute_released_package_protocol_info().save(info_path) - print(f"Wrote {info_path}") - protocol_path = PROTOCOLS_DIR / f"protocol-{DATASET_PROTOCOL_VERSION}.json" - if not protocol_path.exists(): + + if info_path.exists(): + print(f"Package artifact already present: {info_path}") + else: + dataset_registry.compute_released_package_protocol_info().save(info_path) + print(f"Wrote {info_path}") + + if protocol_path.exists(): + print(f"Protocol artifact already present: {protocol_path}") + else: dataset_registry.compute_released_protocol().save(protocol_path) print(f"Wrote {protocol_path} (new protocol version)") diff --git a/packages/syft-datasets/tests/migrations/unit/test_history_artifacts.py b/packages/syft-datasets/tests/migrations/unit/test_history_artifacts.py index be7b2814160..d8d0d90e870 100644 --- a/packages/syft-datasets/tests/migrations/unit/test_history_artifacts.py +++ b/packages/syft-datasets/tests/migrations/unit/test_history_artifacts.py @@ -1,15 +1,14 @@ """The hardcoded release artifacts of past syft-dataset releases.""" +from syft_datasets.migrations import dataset_registry +from syft_datasets.migrations.history import PACKAGE_ARTIFACTS_DIR, PROTOCOLS_DIR +from syft_datasets.models import DatasetV1 from syft_migration import ( MigrationService, ReleasedPackageProtocolInfo, ReleasedProtocol, ) -from syft_datasets.migrations import dataset_registry -from syft_datasets.migrations.history import PACKAGE_ARTIFACTS_DIR, PROTOCOLS_DIR -from syft_datasets.models import DatasetV1 - def test_all_released_package_artifacts_load(): artifact_paths = sorted(PACKAGE_ARTIFACTS_DIR.glob("*.json")) @@ -79,6 +78,16 @@ def test_protocol_bumped_when_changed(): assert not dataset_registry.protocol_changed_without_bump() +def test_protocol_bump_not_missing(): + # Stays live between a protocol bump and the release that freezes it, which is + # exactly where test_protocol_bumped_when_changed goes quiet. + assert not dataset_registry.protocol_bump_missing(), ( + "The dataset protocol changed since the newest released protocol without a " + "bump. Bump DATASET_PROTOCOL_VERSION in " + "syft_datasets/migrations/registry.py, or revert the model change." + ) + + def test_historic_schemas_registered_on_import(): # syft_datasets/__init__ registers every artifact in migrations/history/. assert dataset_registry.package_version_history["0"].version == "0.1.20" diff --git a/packages/syft-job/scripts/export_release_artifact.py b/packages/syft-job/scripts/export_release_artifact.py index e4d054c8ce0..73df7e84997 100644 --- a/packages/syft-job/scripts/export_release_artifact.py +++ b/packages/syft-job/scripts/export_release_artifact.py @@ -3,6 +3,9 @@ Run on EVERY release (uv run python scripts/export_release_artifact.py): always writes the package release info; additionally writes the protocol artifact when this release introduces a new protocol version. + +Artifacts are frozen once written. Running this again for the same version +writes nothing and succeeds, so a release can re-run it safely. """ import sys @@ -16,6 +19,14 @@ def main() -> None: # Import the models so every versioned object is registered. import syft_job # noqa: F401 + if job_registry.protocol_bump_missing(): + sys.exit( + "The job protocol changed since the released " + f"protocol-{job_registry.latest_released_protocol_version()}.json; " + "bump JOB_PROTOCOL_VERSION in syft_job/migrations/registry.py " + "before releasing." + ) + if job_registry.protocol_changed_without_bump(): sys.exit( "The job protocol changed compared to the released " @@ -23,12 +34,21 @@ def main() -> None: "in syft_job/migrations/registry.py before releasing." ) - info_path = PACKAGE_ARTIFACTS_DIR / f"syft-job-{__version__}.json" - job_registry.compute_released_package_protocol_info().save(info_path) - print(f"Wrote {info_path}") + PACKAGE_ARTIFACTS_DIR.mkdir(parents=True, exist_ok=True) + PROTOCOLS_DIR.mkdir(parents=True, exist_ok=True) + info_path = PACKAGE_ARTIFACTS_DIR / f"syft-job-{__version__}.json" protocol_path = PROTOCOLS_DIR / f"protocol-{JOB_PROTOCOL_VERSION}.json" - if not protocol_path.exists(): + + if info_path.exists(): + print(f"Package artifact already present: {info_path}") + else: + job_registry.compute_released_package_protocol_info().save(info_path) + print(f"Wrote {info_path}") + + if protocol_path.exists(): + print(f"Protocol artifact already present: {protocol_path}") + else: job_registry.compute_released_protocol().save(protocol_path) print(f"Wrote {protocol_path} (new protocol version)") diff --git a/packages/syft-job/tests/migrations/unit/test_history_artifacts.py b/packages/syft-job/tests/migrations/unit/test_history_artifacts.py index 417bb7db4eb..fb759a576cb 100644 --- a/packages/syft-job/tests/migrations/unit/test_history_artifacts.py +++ b/packages/syft-job/tests/migrations/unit/test_history_artifacts.py @@ -87,6 +87,16 @@ def test_protocol_bumped_when_changed(): assert not job_registry.protocol_changed_without_bump() +def test_protocol_bump_not_missing(): + # Stays live between a protocol bump and the release that freezes it, which is + # exactly where test_protocol_bumped_when_changed goes quiet. + assert not job_registry.protocol_bump_missing(), ( + "The job protocol changed since the newest released protocol without a " + "bump. Bump JOB_PROTOCOL_VERSION in syft_job/migrations/registry.py, or " + "revert the model change." + ) + + def test_historic_schemas_registered_on_import(): # syft_job/__init__ registers every artifact in migrations/history/. assert job_registry.package_version_history["0"].version == "0.1.38" diff --git a/packages/syft-migration/src/syft_migration/registry.py b/packages/syft-migration/src/syft_migration/registry.py index b445dd48a51..e85339bed17 100644 --- a/packages/syft-migration/src/syft_migration/registry.py +++ b/packages/syft-migration/src/syft_migration/registry.py @@ -285,3 +285,25 @@ def protocol_changed_without_bump(self) -> bool: return False current = self.compute_protocol_schema() return released.supported_versions != current.supported_versions + + def latest_released_protocol_version(self) -> str | None: + """The newest protocol version with a frozen schema. None if there is none.""" + if not self.protocol_version_history: + return None + return max(self.protocol_version_history, key=_version_order) + + def protocol_bump_missing(self) -> bool: + """Whether the protocol changed since the newest RELEASED protocol + without a bump of the version constant. + + Only object versions are compared. A protocol change that alters the + on-disk layout, but adds no object version, is invisible here. + """ + latest = self.latest_released_protocol_version() + if latest is None: + return False + released = self.protocol_version_history[latest] + current = self.compute_protocol_schema() + if current.supported_versions == released.supported_versions: + return False + return _version_order(self.protocol_version) <= _version_order(latest) diff --git a/packages/syft-migration/tests/test_release_artifacts.py b/packages/syft-migration/tests/test_release_artifacts.py index 4fb572c3400..19326d73c25 100644 --- a/packages/syft-migration/tests/test_release_artifacts.py +++ b/packages/syft-migration/tests/test_release_artifacts.py @@ -152,3 +152,78 @@ class GadgetV2(MigratableObject, registry=reg): version: str = "2" assert reg.protocol_changed_without_bump() + + +def test_bump_missing_is_live_before_the_protocol_is_released(): + # protocol_changed_without_bump needs a frozen schema for the CURRENT protocol + # version, so it cannot see a change made after a bump. protocol_bump_missing + # compares against the newest released protocol instead. + reg = _fresh_registry(protocol_version="0") + + class WidgetV1(MigratableObject, registry=reg): + canonical_name: str = "widget" + version: str = "1" + + reg.register_released_protocol(released=reg.compute_released_protocol()) + assert reg.latest_released_protocol_version() == "0" + assert not reg.protocol_bump_missing() + + # Bump the protocol, then add an object version. Protocol 1 is not released, + # so the old guard goes quiet and the new one must not. + reg.protocol_version = "1" + + class WidgetV2(MigratableObject, registry=reg): + canonical_name: str = "widget" + version: str = "2" + + assert not reg.protocol_changed_without_bump() + assert not reg.protocol_bump_missing() + + class WidgetV3(MigratableObject, registry=reg): + canonical_name: str = "widget" + version: str = "3" + + # Still one bump ahead of the newest released protocol, so still clean. + assert not reg.protocol_bump_missing() + + # Roll the constant back onto the released protocol: the change is now unbumped. + reg.protocol_version = "0" + assert reg.protocol_bump_missing() + + +def test_bump_missing_compares_against_the_newest_released_protocol(): + reg = _fresh_registry(protocol_version="2") + + class PartV1(MigratableObject, registry=reg): + canonical_name: str = "part" + version: str = "1" + + # Freeze protocol 0 holding only version 1. + reg.register_released_protocol(released=reg.compute_released_protocol()) + protocol_0 = reg.protocol_version_history.pop("2") + protocol_0.version = "0" + reg.register_historic_protocol_schema(schema=protocol_0) + + class PartV2(MigratableObject, registry=reg): + canonical_name: str = "part" + version: str = "2" + + # Freeze protocol 10 holding both versions. A string sort would treat "2" as + # the newest released protocol and miss that the code matches protocol 10. + protocol_10 = reg.compute_released_protocol().protocol_schema + protocol_10.version = "10" + reg.register_historic_protocol_schema(schema=protocol_10) + + assert reg.latest_released_protocol_version() == "10" + assert not reg.protocol_bump_missing() + + +def test_bump_missing_is_false_without_history(): + reg = _fresh_registry() + + class BoltV1(MigratableObject, registry=reg): + canonical_name: str = "bolt" + version: str = "1" + + assert reg.latest_released_protocol_version() is None + assert not reg.protocol_bump_missing() diff --git a/scripts/bump_version.py b/scripts/bump_version.py index 0ad7467e959..4ddd90f3143 100644 --- a/scripts/bump_version.py +++ b/scripts/bump_version.py @@ -1,17 +1,32 @@ -"""Bump a package version and propagate the change to all dependents. +"""Bump the version of one package, and update the packages that depend on it. -Usage: python scripts/bump_version.py +Usage: + python scripts/bump_version.py + [--dependents {bumped,published}] -Output (two lines): - Line 1: new version - Line 2: space-separated list of all modified pyproject.toml files +The script writes the new version into the pyproject.toml of the package. It +then writes a version pin for the package into each pyproject.toml that depends +on it. + +The --dependents option selects the version for those pins: + +- published: the version that was in the file before this run. A release + publishes the version on the branch, and bumps the version after that. This + version is therefore the version on PyPI. Use this option for a release. +- bumped: the new version. PyPI does not have this version yet. Use this option + only if the script runs before the release. + +The script prints two lines: + +- Line 1: the new version. +- Line 2: the modified pyproject.toml files, separated by spaces. """ import argparse import re -import tomllib from pathlib import Path +import tomllib from packaging.version import Version REPO_ROOT = Path(__file__).resolve().parent.parent @@ -88,11 +103,24 @@ def main() -> None: ) parser.add_argument("package_name", help="Package name (e.g. syft-perms)") parser.add_argument("bump_type", choices=["major", "minor", "patch"]) + parser.add_argument( + "--dependents", + choices=["bumped", "published"], + default="bumped", + help=( + "Version for the dependent pins. 'bumped' is the new version. " + "'published' is the version that was in the file before this run, " + "which is the version a release publishes." + ), + ) args = parser.parse_args() target_path = find_target_pyproject(args.package_name) + with open(target_path, "rb") as f: + published_version = Version(tomllib.load(f)["project"]["version"]) new_version = update_target_version(target_path, args.bump_type) - modified_deps = update_dependents(args.package_name, new_version, target_path) + pinned = new_version if args.dependents == "bumped" else published_version + modified_deps = update_dependents(args.package_name, pinned, target_path) all_modified = [target_path] + modified_deps relative_paths = [str(p.relative_to(REPO_ROOT)) for p in all_modified] diff --git a/scripts/export_release_artifact.py b/scripts/export_release_artifact.py index b9ba8c7f997..0fd30eae299 100644 --- a/scripts/export_release_artifact.py +++ b/scripts/export_release_artifact.py @@ -3,6 +3,9 @@ Run on EVERY release (uv run python scripts/export_release_artifact.py): always writes the package release info; additionally writes the protocol artifact when this release introduces a new protocol version. + +Artifacts are frozen once written. Running this again for the same version +writes nothing and succeeds, so a release can re-run it safely. """ import sys @@ -19,6 +22,14 @@ def main() -> None: # Import the package so every versioned object is registered. import syft_client # noqa: F401 + if client_registry.protocol_bump_missing(): + sys.exit( + "The syft-client protocol changed since the released " + f"protocol-{client_registry.latest_released_protocol_version()}.json; " + "bump SYFT_CLIENT_PROTOCOL_VERSION in " + "syft_client/migrations/registry.py before releasing." + ) + if client_registry.protocol_changed_without_bump(): sys.exit( "The syft-client protocol changed compared to the released " @@ -27,29 +38,21 @@ def main() -> None: "before releasing." ) + PACKAGE_ARTIFACTS_DIR.mkdir(parents=True, exist_ok=True) + PROTOCOLS_DIR.mkdir(parents=True, exist_ok=True) + info_path = PACKAGE_ARTIFACTS_DIR / f"syft-client-{SYFT_CLIENT_VERSION}.json" protocol_path = PROTOCOLS_DIR / f"protocol-{SYFT_CLIENT_PROTOCOL_VERSION}.json" - need_info = not info_path.exists() - need_protocol = not protocol_path.exists() - # Single exit when there is nothing left to write - if not need_info and not need_protocol: - sys.exit( - f"Release artifacts already present:\n" - f" {info_path}\n" - f" {protocol_path}\n" - "They are frozen once written. Bump SYFT_CLIENT_VERSION (and " - "SYFT_CLIENT_PROTOCOL_VERSION if the protocol changed) before " - "exporting again." - ) - - if need_info: + if info_path.exists(): + print(f"Package artifact already present: {info_path}") + else: client_registry.compute_released_package_protocol_info().save(info_path) print(f"Wrote {info_path}") - else: - print(f"Package artifact already present: {info_path}") - if need_protocol: + if protocol_path.exists(): + print(f"Protocol artifact already present: {protocol_path}") + else: client_registry.compute_released_protocol().save(protocol_path) print(f"Wrote {protocol_path} (new protocol version)") diff --git a/tests/migrations/unit/test_history_artifacts.py b/tests/migrations/unit/test_history_artifacts.py index 6d9adcfeadb..98f507752f2 100644 --- a/tests/migrations/unit/test_history_artifacts.py +++ b/tests/migrations/unit/test_history_artifacts.py @@ -63,6 +63,16 @@ def test_protocol_not_changed_without_bump(): assert not client_registry.protocol_changed_without_bump() +def test_protocol_bump_not_missing(): + # Stays live between a protocol bump and the release that freezes it, which is + # exactly where test_protocol_not_changed_without_bump goes quiet. + assert not client_registry.protocol_bump_missing(), ( + "The client protocol changed since the newest released protocol without a " + "bump. Bump SYFT_CLIENT_PROTOCOL_VERSION in " + "syft_client/migrations/registry.py, or revert the model change." + ) + + def test_bump_guard_trips_on_protocol_change(): # A registry claiming the same protocol version as a released schema but # supporting different object versions must trip the guard. diff --git a/tests/unit/test_bump_version.py b/tests/unit/test_bump_version.py new file mode 100644 index 00000000000..2e6508f2864 --- /dev/null +++ b/tests/unit/test_bump_version.py @@ -0,0 +1,88 @@ +"""Check the version that bump_version.py writes into the pin of a dependent.""" + +import importlib.util +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +SCRIPT = REPO_ROOT / "scripts" / "bump_version.py" + +TARGET = """\ +[project] +name = "syft-thing" +version = "0.1.9" +dependencies = [] +""" + +DEPENDENT = """\ +[project] +name = "syft-other" +version = "0.2.0" +dependencies = [ + "syft-thing==0.1.9", +] + +[tool.uv.sources] +"syft-thing" = { workspace = true } +""" + + +@pytest.fixture +def fake_repo(tmp_path): + (tmp_path / "packages" / "syft-thing").mkdir(parents=True) + (tmp_path / "packages" / "syft-other").mkdir(parents=True) + (tmp_path / "packages" / "syft-thing" / "pyproject.toml").write_text(TARGET) + (tmp_path / "packages" / "syft-other" / "pyproject.toml").write_text(DEPENDENT) + return tmp_path + + +def _run(fake_repo, *args): + spec = importlib.util.spec_from_file_location("bump_version_under_test", SCRIPT) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + module.REPO_ROOT = fake_repo + argv = [str(SCRIPT), "syft-thing", "patch", *args] + old = sys.argv + sys.argv = argv + try: + module.main() + finally: + sys.argv = old + + +def _versions(fake_repo): + target = (fake_repo / "packages" / "syft-thing" / "pyproject.toml").read_text() + dependent = (fake_repo / "packages" / "syft-other" / "pyproject.toml").read_text() + source = next( + line for line in target.splitlines() if line.startswith("version") + ).split('"')[1] + pin = next(line for line in dependent.splitlines() if "syft-thing==" in line) + return source, pin.split("==")[1].split('"')[0] + + +def test_default_pins_dependents_to_the_bumped_version(fake_repo): + _run(fake_repo) + source, pin = _versions(fake_repo) + assert source == "0.1.10" + assert pin == "0.1.10" + + +def test_published_pins_dependents_to_the_version_just_released(fake_repo): + # A release publishes the version on the branch, then bumps the version. The + # monorepo releases a dependent later in the same run. The pin must therefore + # name a version that PyPI already has. + _run(fake_repo, "--dependents", "published") + source, pin = _versions(fake_repo) + assert source == "0.1.10" + assert pin == "0.1.9" + + +def test_dependent_pin_is_a_published_version_for_every_release_order(fake_repo): + # This test covers the monorepo order. syft-perms releases before syft-job. If + # the script pins a dependent to the new version, syft-job publishes a + # dependency that PyPI does not have. + _run(fake_repo, "--dependents", "published") + _, pin = _versions(fake_repo) + assert pin == "0.1.9", "a dependent must pin the version that the release published" From 2dec5498c0da77efc61cd615a87851f30fad30b4 Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Wed, 5 Aug 2026 20:20:46 -0300 Subject: [PATCH 04/15] Adopt the Drive folder of an earlier client version - Fix A2 item from migration gaps review, private folders only - Code quality fixes --- .../connections/drive/gdrive_transport.py | 332 ++++++++++++------ tests/unit/test_dataset_collection_listing.py | 68 ++++ tests/unit/test_versioned_folder_adopt.py | 134 +++++++ 3 files changed, 430 insertions(+), 104 deletions(-) create mode 100644 tests/unit/test_dataset_collection_listing.py create mode 100644 tests/unit/test_versioned_folder_adopt.py diff --git a/syft_client/sync/connections/drive/gdrive_transport.py b/syft_client/sync/connections/drive/gdrive_transport.py index 3cf50f2dd7c..0d30f020185 100644 --- a/syft_client/sync/connections/drive/gdrive_transport.py +++ b/syft_client/sync/connections/drive/gdrive_transport.py @@ -1,58 +1,58 @@ """Google Drive Files transport layer implementation""" -import logging import io import json -from pathlib import Path +import logging import pickle -from syft_client.sync.utils.syftbox_utils import check_env -from syft_client.version import SYFT_CLIENT_VERSION -from typing import Any, Dict, List, Optional, Tuple -from typing import TYPE_CHECKING -from pydantic import BaseModel +from pathlib import Path +from typing import TYPE_CHECKING, Any, Optional + +from google.oauth2.credentials import Credentials as GoogleCredentials from google_auth_httplib2 import AuthorizedHttp from googleapiclient.discovery import build from googleapiclient.http import MediaIoBaseDownload, MediaIoBaseUpload, build_http -from google.oauth2.credentials import Credentials as GoogleCredentials +from pydantic import BaseModel +from syft_datasets.dataset_manager import ( + DATASET_COLLECTION_PREFIX, + PRIVATE_DATASET_COLLECTION_PREFIX, +) +from syft_migration import MigrationError -from syft_client.sync.connections.drive.gdrive_utils import ( - gather_all_file_and_folder_ids_recursive, +from syft_client.sync.checkpoints.checkpoint import ( + CHECKPOINT_FILENAME_PREFIX, + INCREMENTAL_CHECKPOINT_PREFIX, + Checkpoint, + IncrementalCheckpoint, ) -from syft_client.sync.connections.drive.gdrive_retry import ( - execute_with_retries, - next_chunk_with_retries, - batch_execute_with_retries, +from syft_client.sync.checkpoints.rolling_state import ( + ROLLING_STATE_FILENAME_PREFIX, + RollingState, ) -from syft_client.sync.version.version_info import _parse_semver - from syft_client.sync.connections.base_connection import ( FileCollection, SyftboxPlatformConnection, ) -from syft_datasets.dataset_manager import ( - DATASET_COLLECTION_PREFIX, - PRIVATE_DATASET_COLLECTION_PREFIX, +from syft_client.sync.connections.drive.gdrive_retry import ( + batch_execute_with_retries, + execute_with_retries, + next_chunk_with_retries, +) +from syft_client.sync.connections.drive.gdrive_utils import ( + gather_all_file_and_folder_ids_recursive, ) +from syft_client.sync.environments.environment import Environment from syft_client.sync.events.file_change_event import ( - FileChangeEventsMessageFileName, FileChangeEventsMessage, + FileChangeEventsMessageFileName, ) from syft_client.sync.messages.proposed_filechange import ( - MessageFileName, FileNameParseError, + MessageFileName, ProposedFileChangesMessage, ) -from syft_client.sync.environments.environment import Environment -from syft_client.sync.checkpoints.checkpoint import ( - Checkpoint, - IncrementalCheckpoint, - CHECKPOINT_FILENAME_PREFIX, - INCREMENTAL_CHECKPOINT_PREFIX, -) -from syft_client.sync.checkpoints.rolling_state import ( - RollingState, - ROLLING_STATE_FILENAME_PREFIX, -) +from syft_client.sync.utils.syftbox_utils import check_env +from syft_client.sync.version.version_info import _parse_semver +from syft_client.version import SYFT_CLIENT_VERSION if TYPE_CHECKING: from syft_client.sync.connections.drive.grdrive_config import ( @@ -80,8 +80,8 @@ def build_drive_service( http = build_http() http.timeout = timeout if environment == Environment.COLAB: - from google.colab import auth as colab_auth import google.auth + from google.colab import auth as colab_auth colab_auth.authenticate_user() creds, _ = google.auth.default() @@ -250,6 +250,53 @@ def _filter_patch_compatible( return kept +# A folder id and name, with the version from the name. The version fields come +# first, so the default sort puts these in version order. +_VersionedFolder = tuple[int, int, int, str, str] + + +def _partition_by_version( + folders: list[tuple[str, str]], + current_version: str | None = None, +) -> tuple[list[tuple[str, str]], list[tuple[str, str]], list[tuple[str, str]]]: + """Split folders into (compatible, older, newer) by the version in the name. + + Compatible means the same major and minor as the current version. The function + drops a folder that has no version in its name. Each list starts at the lowest + version. + """ + if current_version is None: + current_version = SYFT_CLIENT_VERSION + try: + current = _parse_semver(current_version) + except ValueError: + return [], [], [] + + compatible: list[_VersionedFolder] = [] + older: list[_VersionedFolder] = [] + newer: list[_VersionedFolder] = [] + for fid, name in folders: + version_str = _extract_version_from_name(name) + if version_str is None: + continue + try: + found = _parse_semver(version_str) + except ValueError: + continue + entry = (*found, fid, name) + if found[:2] == current[:2]: + compatible.append(entry) + elif found < current: + older.append(entry) + else: + newer.append(entry) + + def _ordered(entries: list[_VersionedFolder]) -> list[tuple[str, str]]: + return [(fid, name) for *_, fid, name in sorted(entries)] + + return _ordered(compatible), _ordered(older), _ordered(newer) + + class GDriveConnection(SyftboxPlatformConnection): """Google Drive Files API transport layer""" @@ -272,21 +319,21 @@ class Config: _personal_syftbox_folder_id: str | None = None # peer_email -> folder_id (folders I created for peer's datasite) - peer_datasite_inbox_cache: Dict[str, str] = {} - peer_datasite_outbox_cache: Dict[str, str] = {} + peer_datasite_inbox_cache: dict[str, str] = {} + peer_datasite_outbox_cache: dict[str, str] = {} # peer_email -> folder_id (folders peer created for my datasite) - own_datasite_inbox_cache: Dict[str, str] = {} - own_datasite_outbox_cache: Dict[str, str] = {} + own_datasite_inbox_cache: dict[str, str] = {} + own_datasite_outbox_cache: dict[str, str] = {} # sender email -> archive folder id - archive_folder_id_cache: Dict[str, str] = {} + archive_folder_id_cache: dict[str, str] = {} # fname -> gdrive id - personal_syftbox_event_id_cache: Dict[str, str] = {} + personal_syftbox_event_id_cache: dict[str, str] = {} # tag -> dataset collection folder id - dataset_collection_folder_id_cache: Dict[str, str] = {} + dataset_collection_folder_id_cache: dict[str, str] = {} # Rolling state caches for single-API-call optimization _rolling_state_folder_id: str | None = None @@ -296,7 +343,7 @@ class Config: _encryption_bundles_folder_id: str | None = None # Cached SYFT_peers.json contents (None = not loaded yet). - _peers_json_cache: Dict[str, Dict[str, str]] | None = None + _peers_json_cache: dict[str, dict[str, str]] | None = None @classmethod def from_config(cls, config: "GdriveConnectionConfig") -> "GDriveConnection": @@ -470,7 +517,7 @@ def _get_peers_file_id(self) -> str | None: items = results.get("files", []) return items[0]["id"] if items else None - def _download_peers_json(self) -> Dict[str, Dict[str, str]]: + def _download_peers_json(self) -> dict[str, dict[str, str]]: """Fetch peers JSON from GDrive. Returns empty dict if not found.""" file_id = self._get_peers_file_id() if file_id is None: @@ -478,21 +525,25 @@ def _download_peers_json(self) -> Dict[str, Dict[str, str]]: try: file_data = self.download_file(file_id) - return json.loads(file_data.decode("utf-8")) except Exception as e: - print(f"Warning: Error reading peers file: {e}") + print(f"Warning: could not download the peers file: {e}") + return {} + try: + return json.loads(file_data.decode("utf-8")) + except ValueError as e: + print(f"Warning: could not read the peers file: {e}") return {} def _get_peers_json( self, force_download: bool = False - ) -> Dict[str, Dict[str, str]]: + ) -> dict[str, dict[str, str]]: """Return peers JSON, using the in-memory cache when available.""" if self._peers_json_cache is not None and not force_download: return self._peers_json_cache self._peers_json_cache = self._download_peers_json() return self._peers_json_cache - def _write_peers_json(self, peers_data: Dict[str, Dict[str, str]]): + def _write_peers_json(self, peers_data: dict[str, dict[str, str]]): """Write peers JSON to GDrive. Creates or updates the file.""" syftbox_folder_id = self.get_syftbox_folder_id() file_id = self._get_peers_file_id() @@ -542,7 +593,7 @@ def _update_peer_state( peers_data[peer_email] = existing self._write_peers_json(peers_data) - def get_peer_requests(self) -> List[str]: + def get_peer_requests(self) -> list[str]: """Get list of pending peer requests. Scans for syft_datasite_#version#{self}_* folders NOT owned by self — those are @@ -563,10 +614,12 @@ def get_peer_requests(self) -> List[str]: for f in results.get("files", []): try: folder = GdriveP2PFolder.from_name(f["name"]) - if folder.datasite_email == self.email: - all_folder_peers.add(folder.peer_email) - except (ValueError, Exception): + except ValueError: + # The query matches a name prefix, so a folder with another shape + # can appear here. continue + if folder.datasite_email == self.email: + all_folder_peers.add(folder.peer_email) peers_data = self._get_peers_json() pending_peers = [] @@ -609,7 +662,7 @@ def watcher_download_raw_events_from_outbox( def watcher_get_events_messages( self, peer_email: str, since_timestamp: float | None - ) -> List[FileChangeEventsMessage]: + ) -> list[FileChangeEventsMessage]: raw_list = self.watcher_download_raw_events_from_outbox( peer_email, since_timestamp ) @@ -617,7 +670,7 @@ def watcher_get_events_messages( def watcher_get_outbox_file_metadatas( self, peer_email: str, since_timestamp: float | None - ) -> List[Dict]: + ) -> list[dict]: """Get file metadata from peer's outbox folder without downloading.""" folder_id = self._get_peer_datasite_outbox_id(peer_email) if folder_id is None: @@ -667,7 +720,7 @@ def owner_download_raw_bytes_by_id(self, file_id: str) -> bytes: def owner_get_all_accepted_event_file_ids( self, since_timestamp: float | None = None - ) -> List[str]: + ) -> list[str]: personal_syftbox_folder_id = self.get_personal_syftbox_folder_id() file_metadatas = self.get_file_metadatas_from_folder( personal_syftbox_folder_id, since_timestamp=since_timestamp @@ -689,7 +742,7 @@ def owner_download_all_raw_events_from_syftbox(self) -> list[bytes]: try: file_data = self.download_file(gdrive_id) except Exception as e: - print(e) + print(f"Warning: could not download event {fname_obj.as_string()}: {e}") continue result.append(file_data) return result @@ -822,7 +875,10 @@ def get_personal_syftbox_folder_id(self) -> str: # '#{peer}#{type}#{email}'. Personal folder shape is exactly # '{version}#{email}', so require a single '#'. folders = [(fid, name) for fid, name in folders if name.count("#") == 1] - folder_id = self._expect_one(_filter_patch_compatible(folders)) + folder_id = self._find_or_adopt_versioned_folder( + folders, + current_name=GdrivePersonalSyftboxFolder(email=self.email).as_string(), + ) if folder_id: self._personal_syftbox_folder_id = folder_id return folder_id @@ -901,7 +957,7 @@ def get_file_metadatas_from_folder( folder_id: str, since_timestamp: float | None = None, page_size: int = 100, - ) -> List[Dict]: + ) -> list[dict]: """ Get file metadatas from folder with early termination. @@ -966,37 +1022,39 @@ def get_file_metadatas_from_folder( @staticmethod def _filter_valid_file_metadatas( - file_metadatas: List[Dict], - ) -> List[Dict]: + file_metadatas: list[dict], + ) -> list[dict]: res = [] for file_metadata in file_metadatas: fname = file_metadata["name"] try: - _ = FileChangeEventsMessageFileName.from_string(fname) - res.append(file_metadata) - except Exception: + FileChangeEventsMessageFileName.from_string(fname) + except ValueError: + # The folder holds other files, so a name that is not an event + # name is normal here. This method filters them out. continue + res.append(file_metadata) return res @staticmethod def _get_valid_events_from_file_metadatas( - file_metadatas: List[Dict], - ) -> List[FileChangeEventsMessageFileName]: + file_metadatas: list[dict], + ) -> list[FileChangeEventsMessageFileName]: res = [] for file_metadata in file_metadatas: fname = file_metadata["name"] try: message_filename = FileChangeEventsMessageFileName.from_string(fname) - res.append(message_filename) - except Exception: - print("Warning, invalid file name: ", fname) + except ValueError: + print(f"Warning: invalid event file name: {fname}") continue + res.append(message_filename) return res @staticmethod def _get_valid_messages_from_file_metadatas( - file_metadatas: List[Dict], - ) -> List[MessageFileName]: + file_metadatas: list[dict], + ) -> list[MessageFileName]: res = [] for file_metadata in file_metadatas: try: @@ -1180,7 +1238,7 @@ def reset_caches(self): self._encryption_bundles_folder_id = None self._peers_json_cache = None - def gather_all_file_and_folder_ids(self) -> List[str]: + def gather_all_file_and_folder_ids(self) -> list[str]: syftbox_folder_id = self.get_syftbox_folder_id() return gather_all_file_and_folder_ids_recursive( self.drive_service, syftbox_folder_id @@ -1188,7 +1246,7 @@ def gather_all_file_and_folder_ids(self) -> List[str]: def delete_multiple_files_by_ids( self, - file_ids: List[str], + file_ids: list[str], ignore_permissions_errors: bool = True, ignore_file_not_found: bool = True, ): @@ -1226,17 +1284,13 @@ def callback(request_id, response, exception): batch.add(self.drive_service.files().delete(fileId=file_id)) batch_execute_with_retries(batch) - def delete_file_by_id( - self, file_id: str, verbose: bool = False, raise_on_error: bool = False - ): + def delete_file_by_id(self, file_id: str, raise_on_error: bool = False): try: execute_with_retries(self.drive_service.files().delete(fileId=file_id)) except Exception as e: if raise_on_error: raise e - else: - if verbose: - print(f"Error deleting file: {file_id}") + print(f"Warning: could not delete file {file_id}: {e}") def delete_unversioned_state(self) -> None: """Delete non-versioned remote artifacts during upgrade. @@ -1350,7 +1404,7 @@ def find_orphaned_message_files(self) -> list[str]: return file_ids - def create_file_payload(self, data: Any) -> Tuple[MediaIoBaseUpload, str]: + def create_file_payload(self, data: Any) -> tuple[MediaIoBaseUpload, str]: """Create a file payload for the GDrive""" if isinstance(data, str): file_data = data.encode("utf-8") @@ -1447,6 +1501,54 @@ def _expect_one(self, folders: list[tuple[str, str]]) -> str | None: f"folder(s) on Drive (keeping the one with your data) and retry." ) + def _find_or_adopt_versioned_folder( + self, + folders: list[tuple[str, str]], + current_name: str, + current_version: str | None = None, + ) -> str | None: + """Return the id of a PRIVATE folder for this client version, or None. + + A private folder name holds the client version, so a minor upgrade looks + for a name that does not exist yet. This method renames the folder of the + highest earlier version to `current_name` and keeps the data. A new folder + would leave the data of the user on Drive and out of reach. + + Renames the folder, so the caller must own it and no peer may look it up by + name. A P2P folder fails both conditions: use `_expect_one` for those. + + Raises RuntimeError if only a folder from a later version exists, or if + more than one compatible folder exists. + """ + compatible, older, newer = _partition_by_version(folders, current_version) + if compatible: + return self._expect_one(compatible) + if newer: + names = [n for _, n in newer] + latest = _extract_version_from_name(names[-1]) + raise RuntimeError( + f"Found a folder from a later client version on Drive: {names}. " + f"This client is {current_version or SYFT_CLIENT_VERSION} and " + f"cannot read that data. Install syft-client {latest} or later." + ) + if not older: + return None + + folder_id, name = older[-1] + execute_with_retries( + self.drive_service.files().update( + fileId=folder_id, body={"name": current_name} + ) + ) + print(f"Adopted the folder of an earlier version: {name} -> {current_name}") + if len(older) > 1: + stale = [n for _, n in older[:-1]] + print( + f"Warning: {len(stale)} folder(s) of earlier versions stay on " + f"Drive: {stale}" + ) + return folder_id + def download_file(self, file_id: str) -> bytes: request = self.drive_service.files().get_media(fileId=file_id) @@ -1457,7 +1559,7 @@ def download_file(self, file_id: str) -> bytes: done = False while not done: - status, done = next_chunk_with_retries(downloader) + _, done = next_chunk_with_retries(downloader) message_data = file_buffer.getvalue() return message_data @@ -1541,10 +1643,9 @@ def _batch_add_permissions(self, file_id: str, users: list[str]) -> None: """Add reader permissions for multiple users in a single batch request.""" def callback(request_id, response, exception): - if exception: - # Ignore "already shared" errors - if "alreadyShared" not in str(exception): - raise exception + # Ignore "already shared" errors + if exception and "alreadyShared" not in str(exception): + raise exception BATCH_SIZE = 100 for i in range(0, len(users), BATCH_SIZE): @@ -1620,23 +1721,21 @@ def owner_list_all_dataset_collections_with_permissions( collections = [] for folder in results.get("files", []): - folder_id = folder["id"] try: folder_obj = DatasetCollectionFolder.from_name(folder["name"]) - has_anyone = ( - folder.get("appProperties", {}).get("syft_shared_with_any") - == "true" - ) - collections.append( - FileCollection( - folder_id=folder_id, - tag=folder_obj.tag, - content_hash=folder_obj.content_hash, - has_any_permission=has_anyone, - ) - ) - except Exception: + except ValueError: continue + has_anyone = ( + folder.get("appProperties", {}).get("syft_shared_with_any") == "true" + ) + collections.append( + FileCollection( + folder_id=folder["id"], + tag=folder_obj.tag, + content_hash=folder_obj.content_hash, + has_any_permission=has_anyone, + ) + ) return collections @@ -1705,7 +1804,7 @@ def watcher_download_dataset_collection( def watcher_get_dataset_collection_file_metadatas( self, tag: str, content_hash: str, owner_email: str - ) -> List[Dict]: + ) -> list[dict]: """Get file metadata from a dataset collection without downloading.""" folder_obj = DatasetCollectionFolder(tag=tag, content_hash=content_hash) folder_name = folder_obj.as_string() @@ -1822,7 +1921,7 @@ def owner_delete_private_dataset_collection(self, tag: str) -> None: def owner_get_private_collection_file_metadatas( self, tag: str, content_hash: str, owner_email: str - ) -> List[Dict]: + ) -> list[dict]: """Get file metadata from a private dataset collection without downloading.""" folder_obj = PrivateDatasetCollectionFolder(tag=tag, content_hash=content_hash) folder_name = folder_obj.as_string() @@ -1916,8 +2015,13 @@ def read_own_version_file(self) -> Optional["VersionInfo"]: try: file_data = self.download_file(file_id) + except Exception as e: + print(f"Warning: could not download the own version file: {e}") + return None + try: return VersionInfo.from_json(file_data.decode("utf-8")) - except Exception: + except (ValueError, MigrationError) as e: + print(f"Warning: could not read the own version file: {e}") return None def read_peer_version_file(self, peer_email: str) -> Optional["VersionInfo"]: @@ -1930,8 +2034,13 @@ def read_peer_version_file(self, peer_email: str) -> Optional["VersionInfo"]: try: file_data = self.download_file(file_id) + except Exception as e: + print(f"Warning: could not download the version file of {peer_email}: {e}") + return None + try: return VersionInfo.from_json(file_data.decode("utf-8")) - except Exception: + except (ValueError, MigrationError) as e: + print(f"Warning: could not read the version file of {peer_email}: {e}") return None def share_version_file_with_peer(self, peer_email: str) -> None: @@ -1961,7 +2070,9 @@ def _get_checkpoints_folder_id(self) -> str | None: name_contains=[f"{self.email}-", "-checkpoints"], parent_id=self.get_syftbox_folder_id(), ) - return self._expect_one(_filter_patch_compatible(folders)) + return self._find_or_adopt_versioned_folder( + folders, current_name=self._get_checkpoints_folder_name() + ) def _get_or_create_checkpoints_folder_id(self) -> str: """Get or create the checkpoints folder.""" @@ -2264,7 +2375,9 @@ def _get_rolling_state_folder_id(self, use_cache: bool = True) -> str | None: name_contains=[f"{self.email}-", "-rolling-state"], parent_id=self.get_syftbox_folder_id(), ) - folder_id = self._expect_one(_filter_patch_compatible(folders)) + folder_id = self._find_or_adopt_versioned_folder( + folders, current_name=self._get_rolling_state_folder_name() + ) if folder_id is not None: self._rolling_state_folder_id = folder_id return folder_id @@ -2296,7 +2409,13 @@ def upload_raw_rolling_state(self, filename: str, data: bytes) -> str: media_body=payload, ).execute() return self._rolling_state_file_id - except Exception: + except Exception as e: + # The cached file is gone or unreachable. Clear the cache and + # write a new file below. + print( + f"Warning: could not update rolling state " + f"{self._rolling_state_file_id}, writing a new file: {e}" + ) self._rolling_state_file_id = None folder_id = self._get_or_create_rolling_state_folder_id() @@ -2451,6 +2570,11 @@ def read_peer_encryption_bundle(self, peer_email: str) -> str | None: return None try: data = self.download_file(items[0]["id"]) + except Exception as e: + print(f"Warning: could not download the bundle of {peer_email}: {e}") + return None + try: return data.decode("utf-8") - except Exception: + except ValueError as e: + print(f"Warning: could not read the bundle of {peer_email}: {e}") return None diff --git a/tests/unit/test_dataset_collection_listing.py b/tests/unit/test_dataset_collection_listing.py new file mode 100644 index 00000000000..606f0850a95 --- /dev/null +++ b/tests/unit/test_dataset_collection_listing.py @@ -0,0 +1,68 @@ +"""owner_list_all_dataset_collections_with_permissions skips only bad names. + +The Drive query matches a name prefix, so another tool can return a folder that +this client cannot parse. The listing skips that folder. Every other failure is a +defect, so the listing must raise it. +""" + +from unittest.mock import Mock + +import pytest + +from syft_client.sync.connections.drive.gdrive_transport import ( + DATASET_COLLECTION_PREFIX, + GDriveConnection, +) + +VALID = f"{DATASET_COLLECTION_PREFIX}_mytag_abc123" +UNPARSEABLE = DATASET_COLLECTION_PREFIX + + +def _conn(files): + conn = GDriveConnection(email="alice@example.com", verbose=False) + conn.drive_service = Mock() + conn._syftbox_folder_id = "syftbox-id" + conn.drive_service.files().list().execute.return_value = {"files": files} + return conn + + +def test_a_valid_collection_is_returned(): + conn = _conn([{"id": "f1", "name": VALID, "appProperties": {}}]) + got = conn.owner_list_all_dataset_collections_with_permissions() + assert [(c.folder_id, c.tag, c.content_hash) for c in got] == [ + ("f1", "mytag", "abc123") + ] + assert got[0].has_any_permission is False + + +def test_the_any_permission_flag_comes_from_app_properties(): + conn = _conn( + [ + { + "id": "f1", + "name": VALID, + "appProperties": {"syft_shared_with_any": "true"}, + } + ] + ) + got = conn.owner_list_all_dataset_collections_with_permissions() + assert got[0].has_any_permission is True + + +def test_a_name_the_client_cannot_parse_is_skipped(): + conn = _conn( + [ + {"id": "bad", "name": UNPARSEABLE, "appProperties": {}}, + {"id": "f1", "name": VALID, "appProperties": {}}, + ] + ) + got = conn.owner_list_all_dataset_collections_with_permissions() + assert [c.folder_id for c in got] == ["f1"] + + +def test_a_missing_name_field_raises(): + # A blanket except turned this defect into a collection that disappears + # without a message. + conn = _conn([{"id": "f1", "appProperties": {}}]) + with pytest.raises(KeyError): + conn.owner_list_all_dataset_collections_with_permissions() diff --git a/tests/unit/test_versioned_folder_adopt.py b/tests/unit/test_versioned_folder_adopt.py new file mode 100644 index 00000000000..7dac544a0ed --- /dev/null +++ b/tests/unit/test_versioned_folder_adopt.py @@ -0,0 +1,134 @@ +"""A client adopts a private Drive folder from an earlier client version. + +A private folder name holds the client version. After a minor upgrade the name of +the current version does not exist yet. Without adoption the client creates a new +folder, and the datasite of the user stays on Drive out of reach. + +These tests cover the private folders only. The name of a P2P folder is a +rendezvous string that both peers compute, so a client must never rename one. +""" + +from unittest.mock import Mock + +import pytest + +from syft_client.sync.connections.drive.gdrive_transport import ( + GDriveConnection, + _partition_by_version, +) + +EMAIL = "alice@example.com" + + +def _conn(): + conn = GDriveConnection(email=EMAIL, verbose=False) + conn.drive_service = Mock() + return conn + + +def _renames(conn): + """Return the (fileId, new name) pairs the connection sent to Drive.""" + return [ + (kwargs["fileId"], kwargs["body"]["name"]) + for _, kwargs in conn.drive_service.files().update.call_args_list + if "body" in kwargs and "name" in kwargs.get("body", {}) + ] + + +# ---------- _partition_by_version ------------------------------------------- + + +def test_partition_splits_compatible_older_and_newer(): + folders = [ + ("old", f"0.1.9#{EMAIL}"), + ("same", f"0.2.5#{EMAIL}"), + ("new", f"0.3.0#{EMAIL}"), + ] + compatible, older, newer = _partition_by_version(folders, current_version="0.2.7") + assert compatible == [("same", f"0.2.5#{EMAIL}")] + assert older == [("old", f"0.1.9#{EMAIL}")] + assert newer == [("new", f"0.3.0#{EMAIL}")] + + +def test_partition_sorts_by_number_not_by_string(): + folders = [("a", f"0.1.9#{EMAIL}"), ("b", f"0.1.10#{EMAIL}")] + _, older, _ = _partition_by_version(folders, current_version="0.2.0") + assert [fid for fid, _ in older] == ["a", "b"] + + +def test_partition_drops_names_without_a_version(): + folders = [("a", f"0.1.9#{EMAIL}"), ("b", "no_version_here")] + _, older, _ = _partition_by_version(folders, current_version="0.2.0") + assert older == [("a", f"0.1.9#{EMAIL}")] + + +def test_partition_returns_empty_for_a_bad_current_version(): + folders = [("a", f"0.1.9#{EMAIL}")] + assert _partition_by_version(folders, current_version="garbage") == ([], [], []) + + +# ---------- adoption -------------------------------------------------------- + + +def test_a_compatible_folder_wins_and_nothing_is_renamed(): + conn = _conn() + folders = [("same", f"0.2.5#{EMAIL}"), ("old", f"0.1.9#{EMAIL}")] + got = conn._find_or_adopt_versioned_folder( + folders, current_name=f"0.2.7#{EMAIL}", current_version="0.2.7" + ) + assert got == "same" + assert _renames(conn) == [] + + +def test_an_older_folder_is_adopted_by_rename(): + conn = _conn() + folders = [("old", f"0.1.9#{EMAIL}")] + got = conn._find_or_adopt_versioned_folder( + folders, current_name=f"0.2.7#{EMAIL}", current_version="0.2.7" + ) + assert got == "old", "the client must keep the folder that holds the data" + assert _renames(conn) == [("old", f"0.2.7#{EMAIL}")] + + +def test_the_highest_older_folder_is_adopted(): + conn = _conn() + folders = [ + ("v1", f"0.1.9#{EMAIL}"), + ("v2", f"0.1.20#{EMAIL}"), + ("v0", f"0.0.4#{EMAIL}"), + ] + got = conn._find_or_adopt_versioned_folder( + folders, current_name=f"0.2.7#{EMAIL}", current_version="0.2.7" + ) + assert got == "v2" + assert _renames(conn) == [("v2", f"0.2.7#{EMAIL}")] + + +def test_a_newer_folder_stops_the_client(): + # A new folder here would hide data that this client cannot read. Report the + # version to install instead. + conn = _conn() + folders = [("new", f"0.3.0#{EMAIL}")] + with pytest.raises(RuntimeError, match="0.3.0"): + conn._find_or_adopt_versioned_folder( + folders, current_name=f"0.2.7#{EMAIL}", current_version="0.2.7" + ) + assert _renames(conn) == [] + + +def test_no_folder_returns_none_so_the_caller_creates_one(): + conn = _conn() + got = conn._find_or_adopt_versioned_folder( + [], current_name=f"0.2.7#{EMAIL}", current_version="0.2.7" + ) + assert got is None + assert _renames(conn) == [] + + +def test_two_compatible_folders_still_raise(): + conn = _conn() + folders = [("a", f"0.2.1#{EMAIL}"), ("b", f"0.2.2#{EMAIL}")] + with pytest.raises(RuntimeError): + conn._find_or_adopt_versioned_folder( + folders, current_name=f"0.2.7#{EMAIL}", current_version="0.2.7" + ) From 60c1a64f947fafe2f3f6d61ca7a830b4d42c5af0 Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Wed, 5 Aug 2026 21:36:41 -0300 Subject: [PATCH 05/15] Add a per-protocol minimum supported version - Fix A3 item from migration gaps review, floor mechanism only - Every floor starts at 0, so no peer is refused; protocol 1 has never shipped, so 0 is the only correct value today. --- .../src/syft_datasets/dataset_storage.py | 12 ++-- .../src/syft_datasets/migrations/registry.py | 6 ++ packages/syft-job/src/syft_job/job_storage.py | 12 ++-- .../src/syft_job/migrations/registry.py | 6 ++ .../src/syft_migration/registry.py | 30 +++++++++ .../src/syft_migration/schema.py | 3 + .../tests/test_protocol_floor.py | 67 +++++++++++++++++++ syft_client/migrations/registry.py | 6 ++ syft_client/sync/version/version_info.py | 1 + 9 files changed, 135 insertions(+), 8 deletions(-) create mode 100644 packages/syft-migration/tests/test_protocol_floor.py diff --git a/packages/syft-datasets/src/syft_datasets/dataset_storage.py b/packages/syft-datasets/src/syft_datasets/dataset_storage.py index fe06581ad3d..d768010afaa 100644 --- a/packages/syft-datasets/src/syft_datasets/dataset_storage.py +++ b/packages/syft-datasets/src/syft_datasets/dataset_storage.py @@ -138,13 +138,17 @@ def negotiated_protocol_version_for_peer( """The dataset protocol version to speak with ``peer_email``. Negotiated as the minimum of our own protocol version and the peer's, so - both sides use a version they can read. A peer without a known schema - raises by default; with ``raise_on_unknown=False`` it is assumed to run - the current protocol. + both sides use a version they can read. The result must also be at or + above the floor of each side, or the negotiation raises. A peer without a + known schema raises by default; with ``raise_on_unknown=False`` it is + assumed to run the current protocol. """ schema = self.peer_schemas.get(peer_email) if schema is not None: - return min(DATASET_PROTOCOL_VERSION, schema.version, key=int) + return self.registry.negotiate_protocol_version( + peer_version=schema.version, + peer_min=schema.min_supported_version, + ) if raise_on_unknown: raise MigrationError( f"No dataset protocol schema known for peer {peer_email!r}" diff --git a/packages/syft-datasets/src/syft_datasets/migrations/registry.py b/packages/syft-datasets/src/syft_datasets/migrations/registry.py index 661be107d98..396ad7984af 100644 --- a/packages/syft-datasets/src/syft_datasets/migrations/registry.py +++ b/packages/syft-datasets/src/syft_datasets/migrations/registry.py @@ -12,6 +12,11 @@ # syft_datasets folder (see config.protocol_dir_name). DATASET_PROTOCOL_VERSION = "1" +# Oldest dataset protocol this release still reads. "0" refuses no peer. Raise it +# only when the code drops support for a released protocol, because a peer below +# the floor cannot exchange datasets with this release. +MIN_SUPPORTED_DATASET_PROTOCOL_VERSION = "0" + # Package-local registry for all versioned syft-dataset objects. The current # protocol schema is computed from the objects registered into it. dataset_registry = MigrationRegistry( @@ -19,4 +24,5 @@ package_name=PACKAGE_NAME, package_version=__version__, protocol_version=DATASET_PROTOCOL_VERSION, + min_supported_protocol_version=MIN_SUPPORTED_DATASET_PROTOCOL_VERSION, ) diff --git a/packages/syft-job/src/syft_job/job_storage.py b/packages/syft-job/src/syft_job/job_storage.py index 5507841e628..aecc96484c6 100644 --- a/packages/syft-job/src/syft_job/job_storage.py +++ b/packages/syft-job/src/syft_job/job_storage.py @@ -72,13 +72,17 @@ def negotiated_protocol_version_for_peer( """The job protocol version to speak with ``peer_email``. Negotiated as the minimum of our own protocol version and the peer's, - so both sides use a version they can read. A peer without a known - schema raises by default; with ``raise_on_unknown=False`` it is assumed - to run the current protocol. + so both sides use a version they can read. The result must also be at or + above the floor of each side, or the negotiation raises. A peer without a + known schema raises by default; with ``raise_on_unknown=False`` it is + assumed to run the current protocol. """ schema = self.peer_schemas.get(peer_email) if schema is not None: - return min(JOB_PROTOCOL_VERSION, schema.version, key=int) + return self.registry.negotiate_protocol_version( + peer_version=schema.version, + peer_min=schema.min_supported_version, + ) if raise_on_unknown: raise MigrationError( f"No job protocol schema known for peer {peer_email!r}" diff --git a/packages/syft-job/src/syft_job/migrations/registry.py b/packages/syft-job/src/syft_job/migrations/registry.py index 30f527b2d69..039fd03b6f7 100644 --- a/packages/syft-job/src/syft_job/migrations/registry.py +++ b/packages/syft-job/src/syft_job/migrations/registry.py @@ -11,6 +11,11 @@ # jobs under a v segment after the peer email (see config.protocol_dir_name). JOB_PROTOCOL_VERSION = "1" +# Oldest job protocol this release still reads. "0" refuses no peer. Raise it +# only when the code drops support for a released protocol, because a peer below +# the floor cannot exchange jobs with this release. +MIN_SUPPORTED_JOB_PROTOCOL_VERSION = "0" + # Package-local registry for all versioned syft-job objects. The current # protocol schema is computed from the objects registered into it. job_registry = MigrationRegistry( @@ -18,4 +23,5 @@ package_name=PACKAGE_NAME, package_version=__version__, protocol_version=JOB_PROTOCOL_VERSION, + min_supported_protocol_version=MIN_SUPPORTED_JOB_PROTOCOL_VERSION, ) diff --git a/packages/syft-migration/src/syft_migration/registry.py b/packages/syft-migration/src/syft_migration/registry.py index e85339bed17..b7dcce22d03 100644 --- a/packages/syft-migration/src/syft_migration/registry.py +++ b/packages/syft-migration/src/syft_migration/registry.py @@ -32,11 +32,15 @@ def __init__( package_name: str, package_version: str, protocol_version: str, + min_supported_protocol_version: str = "0", ) -> None: self.protocol_name = protocol_name self.package_name = package_name self.package_version = package_version self.protocol_version = protocol_version + # The oldest protocol version this package still reads. Raise it only + # when the code drops support for a protocol that a release froze. + self.min_supported_protocol_version = min_supported_protocol_version # canonical_name -> {version: object_class} self.objects: dict[str, dict[str, type[MigratableObject]]] = {} # canonical_name -> {(from_version, to_version): migration_fn} @@ -212,6 +216,7 @@ def compute_protocol_schema(self) -> ProtocolSchema: return ProtocolSchema( protocol_name=self.protocol_name, version=self.protocol_version, + min_supported_version=self.min_supported_protocol_version, supported_versions={ canonical_name: sorted(versions, key=_version_order) for canonical_name, versions in self.objects.items() @@ -224,6 +229,31 @@ def compute_protocol_schema(self) -> ProtocolSchema: }, ) + def negotiate_protocol_version( + self, peer_version: str, peer_min: str | None = None + ) -> str: + """The protocol version to speak with a peer. + + Both sides speak the lower of the two current versions, because each side + must read what the other writes. That version must also be at or above + both floors. A peer that publishes no floor is treated as ``"0"``, which + refuses nothing. + + Raises MigrationError when no version satisfies both sides. + """ + chosen = min(self.protocol_version, peer_version, key=_version_order) + floor = max( + self.min_supported_protocol_version, peer_min or "0", key=_version_order + ) + if _version_order(chosen) < _version_order(floor): + raise MigrationError( + f"No usable {self.protocol_name} protocol version with this peer. " + f"This client speaks {self.protocol_version} and reads down to " + f"{self.min_supported_protocol_version}; the peer speaks " + f"{peer_version} and reads down to {peer_min or '0'}." + ) + return chosen + def compute_released_protocol(self) -> ReleasedProtocol: """The protocol artifact a release emits when the protocol changed.""" return ReleasedProtocol(protocol_schema=self.compute_protocol_schema()) diff --git a/packages/syft-migration/src/syft_migration/schema.py b/packages/syft-migration/src/syft_migration/schema.py index 0b07c1b8458..1230bb7e607 100644 --- a/packages/syft-migration/src/syft_migration/schema.py +++ b/packages/syft-migration/src/syft_migration/schema.py @@ -25,6 +25,9 @@ class ProtocolSchema(BaseModel): # Incrementing protocol version ("0", "1", ...); bumped when the on-disk / # on-the-wire layout of the protocol changes, independent of package versions. version: str + # The oldest protocol version this speaker still reads. A peer that predates + # this field says nothing, so "0" refuses nothing. + min_supported_version: str = "0" # canonical_name -> all supported versions supported_versions: dict[str, list[str]] = {} # canonical_name -> JSON schema of the protocol's current (latest) object diff --git a/packages/syft-migration/tests/test_protocol_floor.py b/packages/syft-migration/tests/test_protocol_floor.py new file mode 100644 index 00000000000..48daf6db75e --- /dev/null +++ b/packages/syft-migration/tests/test_protocol_floor.py @@ -0,0 +1,67 @@ +"""A protocol floor refuses a version that one of the two sides cannot read. + +Both sides publish a floor. Negotiation picks the lower current version, and that +version must be at or above both floors. A floor of "0" refuses nothing. +""" + +import pytest +from syft_migration import MigrationError, MigrationRegistry, ProtocolSchema + + +def _registry(protocol_version: str = "2", floor: str = "0") -> MigrationRegistry: + return MigrationRegistry( + protocol_name="p", + package_name="pkg", + package_version="1.0.0", + protocol_version=protocol_version, + min_supported_protocol_version=floor, + ) + + +def test_schema_floor_defaults_to_zero(): + # A peer that predates the floor field says nothing, so it refuses nothing. + schema = ProtocolSchema(protocol_name="p", version="1") + assert schema.min_supported_version == "0" + + +def test_registry_floor_defaults_to_zero(): + reg = MigrationRegistry( + protocol_name="p", + package_name="pkg", + package_version="1.0.0", + protocol_version="1", + ) + assert reg.min_supported_protocol_version == "0" + + +def test_negotiation_picks_the_lower_version(): + reg = _registry(protocol_version="2") + assert reg.negotiate_protocol_version(peer_version="1") == "1" + assert reg.negotiate_protocol_version(peer_version="3") == "2" + + +def test_negotiation_orders_by_number(): + reg = _registry(protocol_version="10") + assert reg.negotiate_protocol_version(peer_version="9") == "9" + + +def test_our_floor_refuses_an_older_peer(): + reg = _registry(protocol_version="2", floor="2") + with pytest.raises(MigrationError, match="1"): + reg.negotiate_protocol_version(peer_version="1") + + +def test_the_peer_floor_refuses_us(): + reg = _registry(protocol_version="2", floor="0") + with pytest.raises(MigrationError): + reg.negotiate_protocol_version(peer_version="3", peer_min="3") + + +def test_a_zero_floor_on_both_sides_refuses_nothing(): + reg = _registry(protocol_version="5", floor="0") + assert reg.negotiate_protocol_version(peer_version="0", peer_min="0") == "0" + + +def test_an_unknown_peer_floor_is_treated_as_zero(): + reg = _registry(protocol_version="2", floor="0") + assert reg.negotiate_protocol_version(peer_version="1", peer_min=None) == "1" diff --git a/syft_client/migrations/registry.py b/syft_client/migrations/registry.py index c071e4c6da8..e74077c855f 100644 --- a/syft_client/migrations/registry.py +++ b/syft_client/migrations/registry.py @@ -14,6 +14,11 @@ # fields on every versioned object. SYFT_CLIENT_PROTOCOL_VERSION = "1" +# Oldest syft-client protocol this release still reads. "0" refuses no peer. +# Raise it only when the code drops support for a released protocol, because a +# peer below the floor cannot exchange syft-client messages with this release. +MIN_SUPPORTED_SYFT_CLIENT_PROTOCOL_VERSION = "0" + # Package-local registry for all versioned syft-client objects. The current # protocol schema is computed from the objects registered into it. client_registry = MigrationRegistry( @@ -21,6 +26,7 @@ package_name=PACKAGE_NAME, package_version=SYFT_CLIENT_VERSION, protocol_version=SYFT_CLIENT_PROTOCOL_VERSION, + min_supported_protocol_version=MIN_SUPPORTED_SYFT_CLIENT_PROTOCOL_VERSION, ) # Shared service for loading/migrating syft-client objects. diff --git a/syft_client/sync/version/version_info.py b/syft_client/sync/version/version_info.py index 28f1453c67d..73c6bf27862 100644 --- a/syft_client/sync/version/version_info.py +++ b/syft_client/sync/version/version_info.py @@ -157,6 +157,7 @@ def _slim_schema_of(registry) -> ProtocolSchema: return ProtocolSchema( protocol_name=registry.protocol_name, version=registry.protocol_version, + min_supported_version=registry.min_supported_protocol_version, supported_versions={ canonical_name: sorted(versions) for canonical_name, versions in registry.objects.items() From 79f234866d3617601cdab139698721c19e5cf1ec Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Wed, 5 Aug 2026 22:54:39 -0300 Subject: [PATCH 06/15] Stop refusing a peer for a client version difference - Fix A3 item from migration gaps review, peer gate policy - A peer with UNKNOWN version is still skipped, because nothing can be negotiated without its version --- syft_client/sync/version/peer_manager.py | 47 +++++++++++----- tests/unit/test_version_negotiation.py | 68 ++++++++++++++++-------- 2 files changed, 79 insertions(+), 36 deletions(-) diff --git a/syft_client/sync/version/peer_manager.py b/syft_client/sync/version/peer_manager.py index fa635cb3e96..8756d421ba9 100644 --- a/syft_client/sync/version/peer_manager.py +++ b/syft_client/sync/version/peer_manager.py @@ -370,11 +370,16 @@ def get_peer_compatibility_status( """Build a PeerCompatibilityResult describing whether the caller should skip / raise / warn for this peer. - SAME → no skip, no warning. PATCH_DIFF → no skip, "patch differs" - warning. INCOMPATIBLE / UNKNOWN → skip unless effective - `force_ignore_peer_version or ignore_peer_version` (then proceed with - a "proceeding to {action}" warning). UNKNOWN's skip message includes - a "call client.sync()" hint. + SAME → no skip, no log. + + PATCH_DIFF → no skip and a "patch differs" log, or a skip when + `skip_peer_on_patch_version_diff` is set. + + INCOMPATIBLE → no skip; the client version difference is logged, and + each protocol decides separately through its floor. + + UNKNOWN → skip, unless effective `force_ignore_peer_version or + ignore_peer_version`; the message includes a "call client.sync()" hint. """ own_version = self.get_own_version() peer_version = self.get_peer_version(peer_email) @@ -422,14 +427,26 @@ def get_peer_compatibility_status( **common, ) - # UNKNOWN or INCOMPATIBLE - if status == CompatibilityStatus.UNKNOWN: - detail = ( - "version information not available " - "(if you are unsure if it is up to date, call client.sync())" + if status == CompatibilityStatus.INCOMPATIBLE: + # A different client version does not refuse the peer. What each side + # can exchange is decided per protocol by the floor published in + # VersionInfo (MigrationRegistry.negotiate_protocol_version), not by + # comparing package versions. + return PeerCompatibilityResult( + should_skip=False, + explanation_not_skip=( + f"Peer {peer_email}: " + f"{own_version.get_incompatibility_reason(peer_version)}." + ), + **common, ) - else: - detail = own_version.get_incompatibility_reason(peer_version) + + # UNKNOWN: the capabilities of the peer are not known, so there is no + # floor to check. Skipping stays the safe answer. + detail = ( + "version information not available " + "(if you are unsure if it is up to date, call client.sync())" + ) effective_ignore = self.force_ignore_peer_version or ignore_peer_version if effective_ignore: @@ -485,8 +502,10 @@ def warn_if_all_peers_incompatible(self, peer_emails: List[str]) -> None: ) if not any_compatible: warnings.warn( - f"All connected peers ({len(peer_emails)}) have incompatible versions. " - "You may not be able to submit jobs or load datasets until versions match." + f"All connected peers ({len(peer_emails)}) run a different client " + "version, or their version is unknown. A peer with an unknown " + "version cannot receive jobs or datasets; call client.sync() to " + "read the version of each peer." ) def shutdown(self) -> None: diff --git a/tests/unit/test_version_negotiation.py b/tests/unit/test_version_negotiation.py index 36bb593f4ac..339ca4c7e4d 100644 --- a/tests/unit/test_version_negotiation.py +++ b/tests/unit/test_version_negotiation.py @@ -5,7 +5,6 @@ import pytest from syft_client.sync.syftbox_manager import SyftboxManager from syft_client.sync.version.exceptions import ( - VersionMismatchError, VersionUnknownError, ) from syft_client.sync.version.peer_manager import CompatAction @@ -455,33 +454,36 @@ def test_explicit_true_on_ds_is_preserved(self): class TestForceAllowIncompatiblePeers: """Tests for force_ignore_peer_version and per-call ignore_peer_version.""" - def test_incompatible_peer_skipped_by_default(self): + def test_incompatible_peer_is_included_with_a_log(self, caplog): + # A different client version no longer refuses a peer. The protocol floor + # in VersionInfo decides what the two sides may exchange. ds_manager, do_manager = SyftboxManager.pair_with_mock_drive_service_connection( check_versions=True, ) _set_peer_version(do_manager, ds_manager.email, build_client_version("99.0.0")) - do_manager.peer_manager.suppress_version_warnings = True - compatible = do_manager.peer_manager.get_compatible_peer_emails_for_syncing( - [ds_manager.email] + with caplog.at_level(logging.INFO, logger="syft_client"): + compatible = do_manager.peer_manager.get_compatible_peer_emails_for_syncing( + [ds_manager.email] + ) + assert ds_manager.email in compatible + assert any( + "client version mismatch" in r.getMessage().lower() for r in caplog.records ) - assert ds_manager.email not in compatible - def test_force_allow_includes_incompatible_peer(self, caplog): + def test_force_allow_is_redundant_for_an_incompatible_peer(self): + # The flag overrode a refusal that no longer happens. The peer is included + # either way. ds_manager, do_manager = SyftboxManager.pair_with_mock_drive_service_connection( check_versions=True, ) _set_peer_version(do_manager, ds_manager.email, build_client_version("99.0.0")) do_manager.peer_manager.force_ignore_peer_version = True - with caplog.at_level(logging.INFO, logger="syft_client"): - compatible = do_manager.peer_manager.get_compatible_peer_emails_for_syncing( - [ds_manager.email] - ) - assert ds_manager.email in compatible - assert any( - "proceeding anyway" in r.getMessage().lower() for r in caplog.records + compatible = do_manager.peer_manager.get_compatible_peer_emails_for_syncing( + [ds_manager.email] ) + assert ds_manager.email in compatible def test_per_call_ignore_peer_version_includes_peer(self): ds_manager, do_manager = SyftboxManager.pair_with_mock_drive_service_connection( @@ -494,19 +496,22 @@ def test_per_call_ignore_peer_version_includes_peer(self): ) assert ds_manager.email in compatible - def test_per_call_ignore_peer_version_in_submit(self): + def test_submit_no_longer_raises_for_an_incompatible_peer(self): + # A client version difference does not stop a submission. Only an unknown + # peer version does (see test_job_submission_blocked_without_version). ds_manager, do_manager = SyftboxManager.pair_with_mock_drive_service_connection( check_versions=True, ) _set_peer_version(ds_manager, do_manager.email, build_client_version("99.0.0")) - with pytest.raises(VersionMismatchError): - result = ds_manager.peer_manager.get_peer_compatibility_status( - do_manager.email, action=CompatAction.SUBMIT - ) - result.raise_on_skip(operation="submit job") + result = ds_manager.peer_manager.get_peer_compatibility_status( + do_manager.email, action=CompatAction.SUBMIT + ) + assert result.status == CompatibilityStatus.INCOMPATIBLE + assert not result.should_skip + result.raise_on_skip(operation="submit job") - # With per-call override, should not raise + # The per-call override is redundant now, and still does not raise. result = ds_manager.peer_manager.get_peer_compatibility_status( do_manager.email, action=CompatAction.SUBMIT, @@ -531,12 +536,31 @@ def test_force_allow_in_submit(self): class TestVersionMismatchBehavior: """Tests for version mismatch behavior during operations.""" - def test_sync_skips_incompatible_peers(self): + def test_sync_keeps_an_incompatible_peer(self): ds_manager, do_manager = SyftboxManager.pair_with_mock_drive_service_connection( check_versions=True, ) _set_peer_version(do_manager, ds_manager.email, build_client_version("0.0.1")) + do_manager.peer_manager.suppress_version_warnings = True + compatible_peers = ( + do_manager.peer_manager.get_compatible_peer_emails_for_syncing( + [ds_manager.email] + ) + ) + assert ds_manager.email in compatible_peers + + def test_sync_still_skips_a_peer_of_unknown_version(self): + # The boundary of the policy: a known difference is allowed, an unknown + # peer is not. Nothing can be negotiated without the version of the peer. + ds_manager, do_manager = SyftboxManager.pair_with_mock_drive_service_connection( + check_versions=True, + ) + peer = do_manager.peer_manager.get_cached_peer(ds_manager.email) + assert peer is not None + peer.version = None + do_manager.peer_manager._loaded_peer_versions[ds_manager.email] = None + do_manager.peer_manager.suppress_version_warnings = True compatible_peers = ( do_manager.peer_manager.get_compatible_peer_emails_for_syncing( From 15790de6147cecf43f0e0bbba2be1a59db53afbe Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Thu, 6 Aug 2026 15:16:33 -0300 Subject: [PATCH 07/15] Find a P2P folder whatever client version is in its name - Fix A2b item from migration gaps review, completing A2 - A folder this client owns is reused after an upgrade, becasuse a peer that has not upgraded still looks for the old name - Delete _filter_patch_compatible, which has no caller left. --- .../connections/drive/gdrive_transport.py | 62 +++++++------ tests/unit/test_p2p_folder_lookup.py | 90 +++++++++++++++++++ tests/unit/test_version_mismatch_flow.py | 19 +++- tests/unit/test_versioned_folder_lookup.py | 65 +------------- 4 files changed, 141 insertions(+), 95 deletions(-) create mode 100644 tests/unit/test_p2p_folder_lookup.py diff --git a/syft_client/sync/connections/drive/gdrive_transport.py b/syft_client/sync/connections/drive/gdrive_transport.py index 0d30f020185..eaf56cb2580 100644 --- a/syft_client/sync/connections/drive/gdrive_transport.py +++ b/syft_client/sync/connections/drive/gdrive_transport.py @@ -221,38 +221,28 @@ def _extract_version_from_name(name: str) -> str | None: return None -def _filter_patch_compatible( - folders: list[tuple[str, str]], - current_version: str | None = None, -) -> list[tuple[str, str]]: - """Keep folders whose embedded version has matching major.minor. +# A folder id and name, with the version from the name. The version fields come +# first, so the default sort puts these in version order. +_VersionedFolder = tuple[int, int, int, str, str] + - `current_version` defaults to the module-level SYFT_CLIENT_VERSION at call - time (not import time) so tests that patch the version take effect. +def _sorted_by_version(folders: list[tuple[str, str]]) -> list[tuple[str, str]]: + """Folders from the lowest version to the highest. + + A name with no readable version sorts first, so a versioned folder always + wins when the caller takes the last entry. """ - if current_version is None: - current_version = SYFT_CLIENT_VERSION - try: - cur_major, cur_minor, _ = _parse_semver(current_version) - except ValueError: - return [] - kept: list[tuple[str, str]] = [] - for fid, name in folders: - version_str = _extract_version_from_name(name) + + def key(entry: tuple[str, str]) -> tuple[int, int, int]: + version_str = _extract_version_from_name(entry[1]) if version_str is None: - continue + return (-1, -1, -1) try: - major, minor, _ = _parse_semver(version_str) + return _parse_semver(version_str) except ValueError: - continue - if major == cur_major and minor == cur_minor: - kept.append((fid, name)) - return kept - + return (-1, -1, -1) -# A folder id and name, with the version from the name. The version fields come -# first, so the default sort puts these in version order. -_VersionedFolder = tuple[int, int, int, str, str] + return sorted(folders, key=key) def _partition_by_version( @@ -1123,8 +1113,21 @@ def _is_exact_match(name: str) -> bool: and folder.peer_email == peer_email ) - folders = [(fid, name) for fid, name in folders if _is_exact_match(name)] - return self._expect_one(_filter_patch_compatible(folders)) + # Ignore the version in the name. Each peer builds this name from its own + # client version, so a filter here hides the folder that the peer uses. + # After an upgrade the client therefore finds the old folder and writes to + # it. It makes no second folder, which an older peer would never look for. + candidates = _sorted_by_version( + [(fid, name) for fid, name in folders if _is_exact_match(name)] + ) + if not candidates: + return None + if len(candidates) > 1: + print( + f"Warning: {len(candidates)} P2P folders for {datasite_email} " + f"{folder_type} {peer_email}; using {candidates[-1][1]}" + ) + return candidates[-1][0] def _get_peer_datasite_inbox_id(self, peer_email: str) -> str | None: """Get folder: syft_datasite_{peer}_inbox_{self}, owned by self.""" @@ -1456,7 +1459,8 @@ def _find_folders( Thin wrapper over Drive's files.list -- handles query building and pagination, knows nothing about versions. Pair with - _filter_patch_compatible when the caller cares about version compat. + _partition_by_version or _sorted_by_version when the caller cares about + the version in the folder name. """ clauses = [f"mimeType='{GOOGLE_FOLDER_MIME_TYPE}'", "trashed=false"] for substr in name_contains: diff --git a/tests/unit/test_p2p_folder_lookup.py b/tests/unit/test_p2p_folder_lookup.py new file mode 100644 index 00000000000..0e8d79c7659 --- /dev/null +++ b/tests/unit/test_p2p_folder_lookup.py @@ -0,0 +1,90 @@ +"""P2P folder lookup accepts any client version in the folder name. + +A P2P folder name is a rendezvous string that both peers compute from their own +client version, so neither side may rename it (see the adopt path for private +folders). Lookup therefore has to tolerate the version instead. + +Reuse matters in both directions. A folder this client owns must be reused after +an upgrade, because a peer that still filters by name would not find a new one. +A folder the peer owns must be found whatever version the peer wrote into it. +""" + +from unittest.mock import Mock + +from syft_client.sync.connections.drive.gdrive_transport import GDriveConnection + +ME = "alice@example.com" +PEER = "bob@example.com" + + +def _name(version: str, datasite: str, folder_type: str, peer: str) -> str: + return f"syft_datasite#{version}#{datasite}#{folder_type}#{peer}" + + +def _conn(found): + conn = GDriveConnection(email=ME, verbose=False) + conn.drive_service = Mock() + conn._find_folders = Mock(return_value=found) + return conn + + +def _lookup(conn): + return conn._find_p2p_folder_id( + datasite_email=PEER, folder_type="inbox", peer_email=ME, owner_email=ME + ) + + +def test_a_folder_of_another_minor_version_is_found(): + # The old filter dropped this folder, so the client created a second one and + # the peer kept writing into the first. 0.2.0 differs in the minor from the + # current client version, which is what the filter used to reject. + old = _name("0.2.0", PEER, "inbox", ME) + assert _lookup(_conn([("old", old)])) == "old" + + +def test_a_folder_of_an_older_major_version_is_found(): + old = _name("0.0.9", PEER, "inbox", ME) + assert _lookup(_conn([("old", old)])) == "old" + + +def test_the_highest_version_wins_when_several_exist(): + folders = [ + ("v1", _name("0.1.117", PEER, "inbox", ME)), + ("v2", _name("0.2.0", PEER, "inbox", ME)), + ("v0", _name("0.0.9", PEER, "inbox", ME)), + ] + assert _lookup(_conn(folders)) == "v2" + + +def test_versions_order_by_number_not_by_string(): + folders = [ + ("nine", _name("0.1.9", PEER, "inbox", ME)), + ("ten", _name("0.1.10", PEER, "inbox", ME)), + ] + assert _lookup(_conn(folders)) == "ten" + + +def test_several_folders_no_longer_raise(): + folders = [ + ("a", _name("0.1.117", PEER, "inbox", ME)), + ("b", _name("0.1.118", PEER, "inbox", ME)), + ] + assert _lookup(_conn(folders)) is not None + + +def test_a_folder_of_another_peer_is_ignored(): + other = _name("0.1.117", PEER, "inbox", "carol@example.com") + assert _lookup(_conn([("other", other)])) is None + + +def test_a_folder_of_another_type_is_ignored(): + outbox = _name("0.1.117", PEER, "outbox", ME) + assert _lookup(_conn([("outbox", outbox)])) is None + + +def test_no_folder_returns_none(): + assert _lookup(_conn([])) is None + + +def test_a_name_that_does_not_parse_is_ignored(): + assert _lookup(_conn([("junk", "not_a_p2p_folder")])) is None diff --git a/tests/unit/test_version_mismatch_flow.py b/tests/unit/test_version_mismatch_flow.py index 471142d0e71..75f2d9b8fa1 100644 --- a/tests/unit/test_version_mismatch_flow.py +++ b/tests/unit/test_version_mismatch_flow.py @@ -2,7 +2,6 @@ from unittest.mock import patch -from syft_client.sync.utils.syftbox_utils import delete_local_syftbox from syft_client.sync.connections.drive.gdrive_transport import ( GDRIVE_P2P_FOLDER_DATASITE_PREFIX, GOOGLE_FOLDER_MIME_TYPE, @@ -12,6 +11,7 @@ MockDriveService, ) from syft_client.sync.syftbox_manager import SyftboxManager, SyftboxManagerConfig +from syft_client.sync.utils.syftbox_utils import delete_local_syftbox from syft_client.version import SYFT_CLIENT_VERSION from tests.unit.utils import create_test_project_folder, create_tmp_dataset_files @@ -240,12 +240,23 @@ def test_version_mismatch_and_backup_flow(): do_manager.load_peers() do_manager.approve_peer_request(ds_manager.email) - # Now new versioned P2P folders should exist + # The P2P folders of the old version are reused, not replaced. Both + # peers compute this folder name from their own client version, so a + # peer that has not upgraded still looks for the old name. A second + # folder under NEW_VERSION would hide the first one from that peer. do_p2p_new = _find_versioned_p2p_folders(do_conn_new, ds_email, NEW_VERSION) - assert len(do_p2p_new) > 0 + assert len(do_p2p_new) == 0 + do_p2p_old = _find_versioned_p2p_folders( + do_conn_new, ds_email, SYFT_CLIENT_VERSION + ) + assert len(do_p2p_old) > 0 ds_p2p_new = _find_versioned_p2p_folders(ds_conn_new, do_email, NEW_VERSION) - assert len(ds_p2p_new) > 0 + assert len(ds_p2p_new) == 0 + ds_p2p_old = _find_versioned_p2p_folders( + ds_conn_new, do_email, SYFT_CLIENT_VERSION + ) + assert len(ds_p2p_old) > 0 # -- Step 14: Re-upload dataset -- mock_path2, private_path2, readme_path2 = create_tmp_dataset_files() diff --git a/tests/unit/test_versioned_folder_lookup.py b/tests/unit/test_versioned_folder_lookup.py index f9dce12b8d2..0d3176cd7f2 100644 --- a/tests/unit/test_versioned_folder_lookup.py +++ b/tests/unit/test_versioned_folder_lookup.py @@ -2,15 +2,16 @@ These are pure functions -- no Drive mocks needed. They cover the path that replaced the four format-specific parsers from the original PR. + +Ordering and selection now live in _partition_by_version (adopt, private +folders) and _sorted_by_version (P2P lookup), each tested separately. """ from syft_client.sync.connections.drive.gdrive_transport import ( _extract_version_from_name, - _filter_patch_compatible, _looks_like_version, ) - # ---------- _looks_like_version --------------------------------------------- @@ -61,63 +62,3 @@ def test_extract_from_rolling_state_format(): def test_extract_returns_none_when_missing(): assert _extract_version_from_name("just_a_folder_name") is None - - -# ---------- _filter_patch_compatible ---------------------------------------- - - -def test_filter_keeps_same_patch(): - folders = [("id1", "0.1.114#alice@example.com")] - assert _filter_patch_compatible(folders, current_version="0.1.114") == folders - - -def test_filter_keeps_different_patch_same_minor(): - folders = [("id1", "0.1.114#alice@example.com")] - assert _filter_patch_compatible(folders, current_version="0.1.200") == folders - - -def test_filter_drops_minor_diff(): - folders = [ - ("id1", "0.1.114#alice@example.com"), - ("id2", "0.2.0#alice@example.com"), - ] - assert _filter_patch_compatible(folders, current_version="0.1.114") == [ - ("id1", "0.1.114#alice@example.com") - ] - - -def test_filter_drops_major_diff(): - folders = [ - ("id1", "0.1.114#alice@example.com"), - ("id2", "1.0.0#alice@example.com"), - ] - assert _filter_patch_compatible(folders, current_version="0.1.114") == [ - ("id1", "0.1.114#alice@example.com") - ] - - -def test_filter_drops_names_without_a_version(): - folders = [ - ("id1", "0.1.114#alice@example.com"), - ("id2", "no_version_here"), - ] - assert _filter_patch_compatible(folders, current_version="0.1.114") == [ - ("id1", "0.1.114#alice@example.com") - ] - - -def test_filter_covers_all_four_folder_formats(): - """All four formats syft-client uses should match when major.minor align.""" - folders = [ - ("id1", "0.1.114#alice@example.com"), - ("id2", "syft_datasite#0.1.115#alice@example.com#inbox#bob@example.com"), - ("id3", "alice@example.com-0.1.116-checkpoints"), - ("id4", "alice@example.com-0.1.117-rolling-state"), - ] - kept = _filter_patch_compatible(folders, current_version="0.1.200") - assert {fid for fid, _ in kept} == {"id1", "id2", "id3", "id4"} - - -def test_filter_returns_empty_for_bad_current_version(): - folders = [("id1", "0.1.114#alice@example.com")] - assert _filter_patch_compatible(folders, current_version="garbage") == [] From c2ffeaa407382c65123e4bdf6eb20c58607343f3 Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Thu, 6 Aug 2026 16:25:03 -0300 Subject: [PATCH 08/15] Close small migration gaps B2, B4, D2, A4 - B2: version the crypto key file; refuse an unknown later version because a private key cannot be rebuilt - B4: version the persisted caches; reset on an unknown later version, because the client rebuilds them. On-disk format becomes {"version", "entries"} - D2: freeze the VersionInfo V1 field set; every field V2 adds needs a default. - A4: delete the two unused version exception classes. --- syft_client/sync/peers/peer_store.py | 16 ++++ .../sync/sync/caches/persisted_dict.py | 34 ++++++++- syft_client/sync/version/__init__.py | 8 +- syft_client/sync/version/exceptions.py | 34 --------- .../unit/test_version_info_fields.py | 74 +++++++++++++++++++ tests/unit/test_crypto_keys_version.py | 51 +++++++++++++ tests/unit/test_persisted_dict.py | 8 +- tests/unit/test_persisted_dict_version.py | 65 ++++++++++++++++ 8 files changed, 242 insertions(+), 48 deletions(-) create mode 100644 tests/migrations/unit/test_version_info_fields.py create mode 100644 tests/unit/test_crypto_keys_version.py create mode 100644 tests/unit/test_persisted_dict_version.py diff --git a/syft_client/sync/peers/peer_store.py b/syft_client/sync/peers/peer_store.py index e95a1d7dc96..e349f0a745c 100644 --- a/syft_client/sync/peers/peer_store.py +++ b/syft_client/sync/peers/peer_store.py @@ -20,6 +20,11 @@ PRIVATE_DIR_NAME = "private" CRYPTO_KEYS_FILENAME = "crypto_keys.json" +# Format of the crypto key file. Raise it when the layout of the file changes, +# and add a read path for every earlier version. A file with no version predates +# the field and is version 0. +CRYPTO_KEYS_VERSION = 1 + def datasite_crypto_keys_path(syftbox_folder: Path | str, email: str) -> Path: """Per-datasite key file: ``//private/crypto_keys.json``.""" @@ -230,6 +235,7 @@ def decrypt_and_verify_for_self_if_needed(self, data: bytes) -> bytes: def save_keys(self, path: Path) -> None: keys = self._ensure_private_keys() data = { + "version": CRYPTO_KEYS_VERSION, "email": self.email, "keys_jwk": keys.to_jwks(), "peer_bundles": { @@ -245,6 +251,16 @@ def save_keys(self, path: Path) -> None: @classmethod def load_keys(cls, path: Path) -> "PeerStore": data = json.loads(Path(path).read_text()) + # A file with no version predates the field, and its layout is the one + # this client reads. A later version is refused: a user cannot rebuild a + # private key, so a wrong read loses the keys. + version = data.get("version", 0) + if version > CRYPTO_KEYS_VERSION: + raise ValueError( + f"The crypto key file at {path} has version {version}, and this " + f"client reads up to version {CRYPTO_KEYS_VERSION}. Install a " + "newer syft-client to use these keys." + ) store = cls(email=data["email"], use_encryption=True) store._private_keys = syc.SyftPrivateKeys.from_jwks(data["keys_jwk"]) for email, bundle_dict in data.get("peer_bundles", {}).items(): diff --git a/syft_client/sync/sync/caches/persisted_dict.py b/syft_client/sync/sync/caches/persisted_dict.py index 24064b4ce99..c3f412d54d8 100644 --- a/syft_client/sync/sync/caches/persisted_dict.py +++ b/syft_client/sync/sync/caches/persisted_dict.py @@ -31,6 +31,11 @@ import portalocker +# Format of the persisted file: {"version": N, "entries": {...}}. Raise it when +# the layout of an entry changes. A file with no version holds the entries at the +# top level and predates the field, so it is version 0. +PERSISTED_DICT_VERSION = 1 + class PersistedDict(dict): """Dict that persists to a JSON file. With path=None it's a plain in-memory dict.""" @@ -94,10 +99,28 @@ def _read_from_file(self) -> None: return try: data = json.loads(self._path.read_text()) - for k, v in data.items(): - super().__setitem__(self._key_deserializer(k), v) except (json.JSONDecodeError, OSError): - pass + return + entries = self._entries_of(data) + for k, v in entries.items(): + super().__setitem__(self._key_deserializer(k), v) + + @staticmethod + def _entries_of(data: Any) -> dict: + """The entries to load from a parsed file, empty when it cannot be read. + + The client rebuilds every cache that uses this class, so an unreadable + file costs a re-scan and nothing else. A file from a later version + therefore starts empty instead of stopping the client. + """ + if not isinstance(data, dict): + return {} + if "version" not in data or "entries" not in data: + # Written before the version field existed: entries at the top level. + return data + if data["version"] > PERSISTED_DICT_VERSION: + return {} + return data["entries"] def _write_to_file(self) -> None: if self._path is None: @@ -106,7 +129,10 @@ def _write_to_file(self) -> None: # Per-process unique tmp path: even with the file lock, this guards # against any path where two writers share a tmp filename. tmp = self._path.with_suffix(f".tmp.{os.getpid()}.{uuid4().hex}") - serialized = {self._key_serializer(k): v for k, v in super().items()} + serialized = { + "version": PERSISTED_DICT_VERSION, + "entries": {self._key_serializer(k): v for k, v in super().items()}, + } try: tmp.write_text(json.dumps(serialized)) tmp.replace(self._path) diff --git a/syft_client/sync/version/__init__.py b/syft_client/sync/version/__init__.py index 7a45def3162..80cddae4cb5 100644 --- a/syft_client/sync/version/__init__.py +++ b/syft_client/sync/version/__init__.py @@ -5,20 +5,16 @@ Import it directly: from syft_client.sync.version.peer_manager import PeerManager """ -from syft_client.sync.version.version_info import VersionInfo from syft_client.sync.version.exceptions import ( VersionError, VersionMismatchError, VersionUnknownError, - ClientVersionMismatchError, - ProtocolVersionMismatchError, ) +from syft_client.sync.version.version_info import VersionInfo __all__ = [ - "VersionInfo", "VersionError", + "VersionInfo", "VersionMismatchError", "VersionUnknownError", - "ClientVersionMismatchError", - "ProtocolVersionMismatchError", ] diff --git a/syft_client/sync/version/exceptions.py b/syft_client/sync/version/exceptions.py index a4f7cecc932..f09ce30834a 100644 --- a/syft_client/sync/version/exceptions.py +++ b/syft_client/sync/version/exceptions.py @@ -11,8 +11,6 @@ class VersionError(Exception): """Base exception for version-related errors.""" - pass - class VersionMismatchError(VersionError): """Raised when versions are incompatible between peers.""" @@ -63,35 +61,3 @@ def __init__(self, peer_email: str, operation: Optional[str] = None): ) super().__init__(message) - - -class ClientVersionMismatchError(VersionMismatchError): - """Raised specifically for client version mismatches.""" - - def __init__( - self, - peer_email: str, - local_version: "VersionInfo", - peer_version: "VersionInfo", - ): - reason = ( - f"Client version mismatch: local={local_version.syft_client_version}, " - f"peer={peer_version.syft_client_version}" - ) - super().__init__(peer_email, local_version, peer_version, reason) - - -class ProtocolVersionMismatchError(VersionMismatchError): - """Raised specifically for protocol version mismatches.""" - - def __init__( - self, - peer_email: str, - local_version: "VersionInfo", - peer_version: "VersionInfo", - ): - reason = ( - f"Protocol version mismatch: local={local_version.protocol_version}, " - f"peer={peer_version.protocol_version}" - ) - super().__init__(peer_email, local_version, peer_version, reason) diff --git a/tests/migrations/unit/test_version_info_fields.py b/tests/migrations/unit/test_version_info_fields.py new file mode 100644 index 00000000000..5b9e5a2a1d1 --- /dev/null +++ b/tests/migrations/unit/test_version_info_fields.py @@ -0,0 +1,74 @@ +"""VersionInfo may only grow, because it is the bootstrap channel. + +A peer reads SYFT_version.json before it knows anything else, so every supported +client must parse every newer file. Two rules follow, and neither is enforced by +the migration system: + +- A field of an older version must not disappear or change name. An older reader + requires it, and pydantic raises when it is absent. +- A field that a newer version adds must have a default. A newer reader must + still parse a file that an older client wrote without that field. + +Adding a field is safe on its own: pydantic ignores a field it does not know. +""" + +import syft_client # noqa: F401 -- imports models and registers history +from syft_client.sync.version.version_info import VersionInfoV1, VersionInfoV2 + +# Frozen on purpose. A change here means a change to the bootstrap file, so read +# the two rules above before editing this set. +V1_FIELDS = { + "canonical_name", + "version", + "syft_client_version", + "min_supported_syft_client_version", + "protocol_version", + "min_supported_protocol_version", + "syft_client_install_source", + "updated_at", + "attestation_token", +} + +V2_ADDS = {"protocol_schemas"} + + +def test_v1_fields_are_frozen(): + assert set(VersionInfoV1.model_fields) == V1_FIELDS, ( + "VersionInfoV1 changed. A client that speaks protocol 0 reads this " + "object, so a removed or renamed field stops that client from parsing " + "the version file of this one." + ) + + +def test_v2_keeps_every_v1_field(): + missing = V1_FIELDS - set(VersionInfoV2.model_fields) + assert not missing, ( + f"VersionInfoV2 dropped {sorted(missing)}. A reader of V1 requires these " + "fields, so V2 must keep them." + ) + + +def test_v2_adds_only_the_expected_fields(): + assert set(VersionInfoV2.model_fields) - V1_FIELDS == V2_ADDS + + +def test_fields_added_after_v1_have_a_default(): + # A file written by an older client carries none of these, so a reader of the + # newer version must supply a value. + for name in set(VersionInfoV2.model_fields) - V1_FIELDS: + assert not VersionInfoV2.model_fields[name].is_required(), ( + f"VersionInfoV2.{name} is required. A version file written before " + "this field existed would then fail to parse." + ) + + +def test_a_file_without_the_v2_fields_still_parses(): + written_by_an_older_client = VersionInfoV1( + syft_client_version="0.1.117", + min_supported_syft_client_version="0.1.93", + protocol_version="1.0.0", + min_supported_protocol_version="1.0.0", + ).model_dump(exclude={"canonical_name", "version"}) + + loaded = VersionInfoV2.model_validate(written_by_an_older_client) + assert loaded.protocol_schemas == {} diff --git a/tests/unit/test_crypto_keys_version.py b/tests/unit/test_crypto_keys_version.py new file mode 100644 index 00000000000..bf32ff607d8 --- /dev/null +++ b/tests/unit/test_crypto_keys_version.py @@ -0,0 +1,51 @@ +"""The crypto key file carries a version, and an unknown one stops the load. + +A user cannot rebuild a private key, so delete-and-rebuild is not a recovery +here. If a newer client wrote the file, this client must refuse it rather than +read it wrong and lose the keys. +""" + +import json + +import pytest +from syft_client.sync.peers.peer_store import CRYPTO_KEYS_VERSION, PeerStore + + +def _saved(tmp_path): + store = PeerStore(email="alice@example.com", use_encryption=True) + store.generate_keys() + path = tmp_path / "crypto_keys.json" + store.save_keys(path) + return path + + +def test_a_saved_file_carries_the_version(tmp_path): + data = json.loads(_saved(tmp_path).read_text()) + assert data["version"] == CRYPTO_KEYS_VERSION + + +def test_a_saved_file_loads_back(tmp_path): + path = _saved(tmp_path) + loaded = PeerStore.load_keys(path) + assert loaded.email == "alice@example.com" + + +def test_a_file_without_a_version_still_loads(tmp_path): + # Written before the version field existed. Those keys must keep working. + path = _saved(tmp_path) + data = json.loads(path.read_text()) + del data["version"] + path.write_text(json.dumps(data)) + + loaded = PeerStore.load_keys(path) + assert loaded.email == "alice@example.com" + + +def test_a_file_from_a_newer_client_is_refused(tmp_path): + path = _saved(tmp_path) + data = json.loads(path.read_text()) + data["version"] = CRYPTO_KEYS_VERSION + 1 + path.write_text(json.dumps(data)) + + with pytest.raises(ValueError, match=str(CRYPTO_KEYS_VERSION + 1)): + PeerStore.load_keys(path) diff --git a/tests/unit/test_persisted_dict.py b/tests/unit/test_persisted_dict.py index b8a72a576f8..650d11d3ecd 100644 --- a/tests/unit/test_persisted_dict.py +++ b/tests/unit/test_persisted_dict.py @@ -36,7 +36,7 @@ def writer(d: PersistedDict, prefix: str): assert errors == [], f"Concurrent writes raised: {errors!r}" # Every key from both writers must be present in the final on-disk state. - final = json.loads(target.read_text()) + final = json.loads(target.read_text())["entries"] expected = {f"a-{i}": i for i in range(iterations)} | { f"b-{i}": i for i in range(iterations) } @@ -60,7 +60,7 @@ def test_set_with_write_false_does_not_persist(tmp_path: Path): with d.exclusive_lock(): d._write_to_file() - assert json.loads(target.read_text()) == {"k": "v"} + assert json.loads(target.read_text())["entries"] == {"k": "v"} def test_batch_write_with_exclusive_lock(tmp_path: Path): @@ -86,7 +86,7 @@ def batch_write(d: PersistedDict, prefix: str, n: int): t1.join() t2.join() - final = json.loads(target.read_text()) + final = json.loads(target.read_text())["entries"] expected = {f"a-{i}": i for i in range(50)} | {f"b-{i}": i for i in range(50)} assert final == expected @@ -108,4 +108,4 @@ def test_contains_and_delete_with_flags(tmp_path: Path): d._write_to_file() # After the batch, on-disk state reflects the in-memory delete. - assert json.loads(target.read_text()) == {} + assert json.loads(target.read_text())["entries"] == {} diff --git a/tests/unit/test_persisted_dict_version.py b/tests/unit/test_persisted_dict_version.py new file mode 100644 index 00000000000..44aba4f97e3 --- /dev/null +++ b/tests/unit/test_persisted_dict_version.py @@ -0,0 +1,65 @@ +"""A persisted cache carries a version, and an unknown one resets the cache. + +The client can rebuild every one of these caches from the events and the files, +so an unreadable cache costs a re-scan and nothing else. An unknown version +therefore starts empty instead of stopping the client. +""" + +import json + +from syft_client.sync.sync.caches.persisted_dict import ( + PERSISTED_DICT_VERSION, + PersistedDict, +) + + +def _path(tmp_path): + return tmp_path / "cache.json" + + +def test_a_saved_file_carries_the_version(tmp_path): + d = PersistedDict(path=_path(tmp_path)) + d["a"] = "1" + data = json.loads(_path(tmp_path).read_text()) + assert data["version"] == PERSISTED_DICT_VERSION + assert data["entries"] == {"a": "1"} + + +def test_a_saved_file_loads_back(tmp_path): + d = PersistedDict(path=_path(tmp_path)) + d["a"] = "1" + assert PersistedDict(path=_path(tmp_path)).get("a") == "1" + + +def test_a_file_without_a_version_still_loads(tmp_path): + # Written before the version field existed: a bare map of entries. Reading it + # saves the user a full re-scan on the first run after an upgrade. + _path(tmp_path).write_text(json.dumps({"a": "1", "b": "2"})) + d = PersistedDict(path=_path(tmp_path)) + assert d.get("a") == "1" + assert d.get("b") == "2" + + +def test_a_file_from_a_newer_client_starts_empty(tmp_path): + _path(tmp_path).write_text( + json.dumps({"version": PERSISTED_DICT_VERSION + 1, "entries": {"a": "1"}}) + ) + d = PersistedDict(path=_path(tmp_path)) + assert d.get("a") is None + assert len(d) == 0 + + +def test_an_unreadable_file_starts_empty(tmp_path): + _path(tmp_path).write_text("{not json") + assert len(PersistedDict(path=_path(tmp_path))) == 0 + + +def test_a_reset_cache_can_be_written_again(tmp_path): + _path(tmp_path).write_text( + json.dumps({"version": PERSISTED_DICT_VERSION + 1, "entries": {"a": "1"}}) + ) + d = PersistedDict(path=_path(tmp_path)) + d["b"] = "2" + data = json.loads(_path(tmp_path).read_text()) + assert data["version"] == PERSISTED_DICT_VERSION + assert data["entries"] == {"b": "2"} From 5a3025b9b07735ccb64d9100fcc42289c5d8312b Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Thu, 6 Aug 2026 19:27:41 -0300 Subject: [PATCH 09/15] Version SYFT_peers.json and log unknown peer states - Fix B3 item from migration gaps review - Stamp the format version under a reserved _meta key, so older clients that treat every top-level key as a peer email skip it safely - Log and skip an unknown peer state instead of dropping the peer in silence; the writer keeps other entries, so the record is not erased on Drive --- .../sync/connections/connection_router.py | 33 ++++-- .../connections/drive/gdrive_transport.py | 13 +++ tests/unit/test_peers_json_version.py | 101 ++++++++++++++++++ 3 files changed, 140 insertions(+), 7 deletions(-) create mode 100644 tests/unit/test_peers_json_version.py diff --git a/syft_client/sync/connections/connection_router.py b/syft_client/sync/connections/connection_router.py index e165ed86e21..38c18eb5301 100644 --- a/syft_client/sync/connections/connection_router.py +++ b/syft_client/sync/connections/connection_router.py @@ -1,29 +1,38 @@ -from pydantic import BaseModel +import logging from typing import TYPE_CHECKING, List, Optional + +from pydantic import BaseModel + +from syft_client.sync.checkpoints.checkpoint import Checkpoint, IncrementalCheckpoint +from syft_client.sync.checkpoints.rolling_state import RollingState from syft_client.sync.connections.base_connection import ( ConnectionConfig, FileCollection, SyftboxPlatformConnection, ) -from syft_client.sync.connections.drive.gdrive_transport import GDriveConnection +from syft_client.sync.connections.drive.gdrive_transport import ( + PEERS_META_KEY, + GDriveConnection, +) from syft_client.sync.events.file_change_event import ( FileChangeEventsMessage, ) -from syft_client.sync.checkpoints.checkpoint import Checkpoint, IncrementalCheckpoint -from syft_client.sync.checkpoints.rolling_state import RollingState -from syft_client.sync.peers.peer_store import PeerStore from syft_client.sync.messages.proposed_filechange import ProposedFileChangesMessage -from syft_client.sync.platforms.gdrive_files_platform import GdriveFilesPlatform from syft_client.sync.peers.peer import Peer, PeerState +from syft_client.sync.peers.peer_store import PeerStore +from syft_client.sync.platforms.gdrive_files_platform import GdriveFilesPlatform from syft_client.sync.utils.print_utils import ( - print_peer_adding_to_platform, print_peer_added_to_platform, + print_peer_adding_to_platform, ) if TYPE_CHECKING: from syft_client.sync.version.version_info import VersionInfo +logger = logging.getLogger(__name__) + + class ConnectionRouter(BaseModel): connections: List[SyftboxPlatformConnection] @@ -194,9 +203,19 @@ def get_all_peers_from_json(self, force_download: bool = False) -> List[Peer]: peers_data = connection._get_peers_json(force_download=force_download) peers = [] for email, data in peers_data.items(): + if email == PEERS_META_KEY: + continue try: state = PeerState(data.get("state", "unknown")) except ValueError: + # A later client wrote a state that this client does not know. + # The writer changes one entry and keeps the rest, so the entry + # stays in the file. The peer returns after an upgrade. + logger.warning( + f"Skipping peer {email}: unknown state " + f"{data.get('state')!r}. Install a newer syft-client to see " + "this peer." + ) continue peer = Peer( email=email, diff --git a/syft_client/sync/connections/drive/gdrive_transport.py b/syft_client/sync/connections/drive/gdrive_transport.py index eaf56cb2580..e5476adb025 100644 --- a/syft_client/sync/connections/drive/gdrive_transport.py +++ b/syft_client/sync/connections/drive/gdrive_transport.py @@ -96,6 +96,15 @@ def build_drive_service( LEGACY_GDRIVE_OUTBOX_INBOX_FOLDER_PREFIX = "syft_outbox_inbox" # legacy prefix GDRIVE_P2P_FOLDER_DATASITE_PREFIX = "syft_datasite" SYFT_PEERS_FILE = "SYFT_peers.json" + +# SYFT_peers.json is a flat map of peer email to entry, so a version at the top +# level would look like a peer email. The version goes under this reserved key. +# A client written before the key reads a peer state from that entry and fails. +# The key therefore never appears as a peer. +PEERS_META_KEY = "_meta" +# Shape of one entry in SYFT_peers.json. Raise this when an entry changes. A file +# with no reserved entry was written before the version, and is version 0. +SYFT_PEERS_VERSION = 1 SYFT_VERSION_FILE = "SYFT_version.json" @@ -535,6 +544,10 @@ def _get_peers_json( def _write_peers_json(self, peers_data: dict[str, dict[str, str]]): """Write peers JSON to GDrive. Creates or updates the file.""" + peers_data = { + **peers_data, + PEERS_META_KEY: {"version": SYFT_PEERS_VERSION}, + } syftbox_folder_id = self.get_syftbox_folder_id() file_id = self._get_peers_file_id() diff --git a/tests/unit/test_peers_json_version.py b/tests/unit/test_peers_json_version.py new file mode 100644 index 00000000000..f8b09f9dc6b --- /dev/null +++ b/tests/unit/test_peers_json_version.py @@ -0,0 +1,101 @@ +"""SYFT_peers.json carries a version, and an unreadable peer state is logged. + +The file is a flat map of peer email to entry, so a version cannot go at the top +level: every existing client reads a top-level key as an email. The version lives +under a reserved key instead. An older client parses the state of that entry, +fails, and skips it, so the reserved key is invisible to a client that predates +it. + +The record itself is safe either way. The only writer is `_update_peer_state`, +which changes one entry of the raw map and writes the rest back, so a peer this +client cannot read is not erased for the other side. +""" + +import logging +from unittest.mock import Mock, patch + +from syft_client.sync.connections.drive.gdrive_transport import ( + PEERS_META_KEY, + SYFT_PEERS_VERSION, + GDriveConnection, +) +from syft_client.sync.peers.peer import PeerState + +PEER = "bob@example.com" + + +def _conn(peers_data): + conn = GDriveConnection(email="alice@example.com", verbose=False) + conn.drive_service = Mock() + conn._peers_json_cache = dict(peers_data) + return conn + + +def _router(conn): + router = Mock() + router.connection_for_send_message = Mock(return_value=conn) + from syft_client.sync.connections.connection_router import ConnectionRouter + + return ConnectionRouter.get_all_peers_from_json.__get__(router, ConnectionRouter) + + +def test_a_write_stamps_the_reserved_entry(): + conn = _conn({PEER: {"state": "accepted"}}) + with ( + patch.object(GDriveConnection, "_get_peers_file_id", return_value="file-id"), + patch.object( + GDriveConnection, "get_syftbox_folder_id", return_value="folder-id" + ), + patch.object( + GDriveConnection, "create_file_payload", return_value=(Mock(), None) + ), + ): + conn._write_peers_json({PEER: {"state": "accepted"}}) + + assert conn._peers_json_cache[PEERS_META_KEY] == {"version": SYFT_PEERS_VERSION} + assert conn._peers_json_cache[PEER] == {"state": "accepted"} + + +def test_the_reserved_entry_is_not_a_peer(): + conn = _conn( + { + PEERS_META_KEY: {"version": SYFT_PEERS_VERSION}, + PEER: {"state": "accepted"}, + } + ) + peers = _router(conn)() + assert [p.email for p in peers] == [PEER] + + +def test_a_known_state_loads(): + conn = _conn({PEER: {"state": "rejected"}}) + peers = _router(conn)() + assert peers[0].state == PeerState.REJECTED + + +def test_an_unknown_state_is_skipped_and_logged(caplog): + conn = _conn({PEER: {"state": "quarantined"}}) + with caplog.at_level(logging.WARNING, logger="syft_client"): + peers = _router(conn)() + assert peers == [] + assert any(PEER in r.getMessage() for r in caplog.records) + assert any("quarantined" in r.getMessage() for r in caplog.records) + + +def test_a_file_without_the_reserved_entry_still_loads(): + # Written before the reserved key existed. + conn = _conn({PEER: {"state": "accepted"}}) + peers = _router(conn)() + assert [p.email for p in peers] == [PEER] + + +def test_a_reserved_entry_from_a_newer_client_does_not_stop_the_read(caplog): + conn = _conn( + { + PEERS_META_KEY: {"version": SYFT_PEERS_VERSION + 1}, + PEER: {"state": "accepted"}, + } + ) + with caplog.at_level(logging.WARNING, logger="syft_client"): + peers = _router(conn)() + assert [p.email for p in peers] == [PEER] From 4f1c6e4e2f6929ba27ed15e230721d49720bbe6d Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Thu, 6 Aug 2026 20:06:13 -0300 Subject: [PATCH 10/15] Refuse a checkpoint or rolling state from a later client - Fix B1 item from migration gaps review; A2a already fixed the folder half - A later client can reshape a field while the object still parses, which gives a wrong restore silently. Ever load site already falls back to downloading all events, so refusing costs one slow cold start. --- syft_client/sync/checkpoints/checkpoint.py | 31 ++++++-- syft_client/sync/checkpoints/rolling_state.py | 30 ++++++-- .../sync/sync/datasite_owner_syncer.py | 61 ++++++++------- tests/unit/test_checkpoint_version.py | 77 +++++++++++++++++++ 4 files changed, 161 insertions(+), 38 deletions(-) create mode 100644 tests/unit/test_checkpoint_version.py diff --git a/syft_client/sync/checkpoints/checkpoint.py b/syft_client/sync/checkpoints/checkpoint.py index b2a1ae41c60..26d71711fd5 100644 --- a/syft_client/sync/checkpoints/checkpoint.py +++ b/syft_client/sync/checkpoints/checkpoint.py @@ -11,12 +11,14 @@ - After N incremental checkpoints: compact into single full Checkpoint """ -from typing import List, Dict, TYPE_CHECKING -from pydantic import BaseModel, Field from pathlib import Path +from typing import TYPE_CHECKING, Dict, List + +from pydantic import BaseModel, Field + from syft_client.sync.utils.syftbox_utils import ( - create_event_timestamp, compress_data, + create_event_timestamp, uncompress_data, ) @@ -28,6 +30,21 @@ INCREMENTAL_CHECKPOINT_PREFIX = "incremental_checkpoint" CHECKPOINT_VERSION = 1 + +def _check_version(version: int, kind: str) -> None: + """Refuse a checkpoint from a later client.""" + + # A later client can change what a field holds while the object still parses. + # The restore would then be wrong and silent. Every caller falls back to a + # download of all events, so a refusal costs one slow cold start. + + if version > CHECKPOINT_VERSION: + raise ValueError( + f"This {kind} has version {version}, and this client reads up to " + f"version {CHECKPOINT_VERSION}." + ) + + # Default compacting threshold: merge after this many incremental checkpoints DEFAULT_COMPACTING_THRESHOLD = 4 @@ -124,7 +141,9 @@ def as_compressed_data(self) -> bytes: def from_compressed_data(cls, data: bytes) -> "Checkpoint": """Load checkpoint from compressed data.""" uncompressed_data = uncompress_data(data) - return cls.model_validate_json(uncompressed_data) + checkpoint = cls.model_validate_json(uncompressed_data) + _check_version(checkpoint.version, "checkpoint") + return checkpoint class IncrementalCheckpoint(BaseModel): @@ -180,7 +199,9 @@ def as_compressed_data(self) -> bytes: def from_compressed_data(cls, data: bytes) -> "IncrementalCheckpoint": """Load from compressed data.""" uncompressed_data = uncompress_data(data) - return cls.model_validate_json(uncompressed_data) + checkpoint = cls.model_validate_json(uncompressed_data) + _check_version(checkpoint.version, "incremental checkpoint") + return checkpoint def compact_incremental_checkpoints( diff --git a/syft_client/sync/checkpoints/rolling_state.py b/syft_client/sync/checkpoints/rolling_state.py index cbbf6824895..31be37e60da 100644 --- a/syft_client/sync/checkpoints/rolling_state.py +++ b/syft_client/sync/checkpoints/rolling_state.py @@ -13,22 +13,36 @@ """ from typing import List + from pydantic import BaseModel, Field -from syft_client.sync.utils.syftbox_utils import ( - create_event_timestamp, - compress_data, - uncompress_data, -) + from syft_client.sync.events.file_change_event import ( FileChangeEvent, FileChangeEventsMessage, ) - +from syft_client.sync.utils.syftbox_utils import ( + compress_data, + create_event_timestamp, + uncompress_data, +) ROLLING_STATE_FILENAME_PREFIX = "rolling_state" ROLLING_STATE_VERSION = 1 +def raise_for_later_version(version: int) -> None: + """Refuse a rolling state from a later client.""" + + # A later client can change what a field holds while the object still + # parses. The restore would then be wrong and silent. Every caller falls + # back to a download of all events, so a refusal costs one slow cold start. + if version > ROLLING_STATE_VERSION: + raise ValueError( + f"This rolling state has version {version}, and this client reads up " + f"to version {ROLLING_STATE_VERSION}." + ) + + class RollingState(BaseModel): """ Rolling state keeps the latest state of each file since the last checkpoint. @@ -111,7 +125,9 @@ def as_compressed_data(self) -> bytes: def from_compressed_data(cls, data: bytes) -> "RollingState": """Load rolling state from compressed data.""" uncompressed_data = uncompress_data(data) - return cls.model_validate_json(uncompressed_data) + state = cls.model_validate_json(uncompressed_data) + raise_for_later_version(state.version) + return state @classmethod def filename_to_timestamp(cls, filename: str) -> float | None: diff --git a/syft_client/sync/sync/datasite_owner_syncer.py b/syft_client/sync/sync/datasite_owner_syncer.py index 81e6b25241b..538af2f79c5 100644 --- a/syft_client/sync/sync/datasite_owner_syncer.py +++ b/syft_client/sync/sync/datasite_owner_syncer.py @@ -1,38 +1,42 @@ import logging -from pathlib import Path -from uuid import uuid4 - -from pydantic import ConfigDict, Field, BaseModel, PrivateAttr from concurrent.futures import ThreadPoolExecutor +from pathlib import Path from queue import Queue from typing import List, Tuple -from syft_client.sync.events.file_change_event import ( - FileChangeEventsMessage, - FileChangeEventsMessageFileName, - FileChangeEvent, +from uuid import uuid4 + +from pydantic import BaseModel, ConfigDict, Field, PrivateAttr +from syft_perms import SyftPermContext + +from syft_client.sync.callback_mixin import BaseModelCallbackMixin +from syft_client.sync.checkpoints.checkpoint import ( + DEFAULT_COMPACTING_THRESHOLD, + Checkpoint, + CheckpointFile, + IncrementalCheckpoint, + compact_incremental_checkpoints, +) +from syft_client.sync.checkpoints.rolling_state import ( + RollingState, + raise_for_later_version, ) from syft_client.sync.connections.base_connection import ( ConnectionConfig, FileCollection, ) -from syft_client.sync.sync.caches.datasite_owner_cache import ( - DataSiteOwnerEventCacheConfig, -) from syft_client.sync.connections.connection_router import ConnectionRouter -from syft_client.sync.sync.caches.datasite_owner_cache import DataSiteOwnerEventCache -from syft_client.sync.callback_mixin import BaseModelCallbackMixin +from syft_client.sync.events.file_change_event import ( + FileChangeEvent, + FileChangeEventsMessage, + FileChangeEventsMessageFileName, +) from syft_client.sync.messages.proposed_filechange import ProposedFileChangesMessage -from syft_client.sync.utils.path_filters import is_normal_syncable_path -from syft_client.sync.checkpoints.checkpoint import ( - Checkpoint, - CheckpointFile, - IncrementalCheckpoint, - compact_incremental_checkpoints, - DEFAULT_COMPACTING_THRESHOLD, +from syft_client.sync.sync.caches.datasite_owner_cache import ( + DataSiteOwnerEventCache, + DataSiteOwnerEventCacheConfig, ) -from syft_client.sync.checkpoints.rolling_state import RollingState -from syft_perms import SyftPermContext from syft_client.sync.sync.constants import CACHE_DIR, ROLLING_STATE_FILENAME +from syft_client.sync.utils.path_filters import is_normal_syncable_path logger = logging.getLogger(__name__) @@ -124,9 +128,14 @@ def _load_rolling_state(self) -> None: if not path.exists(): return try: - self._rolling_state = RollingState.model_validate_json(path.read_text()) - except Exception: - pass + state = RollingState.model_validate_json(path.read_text()) + raise_for_later_version(state.version) + except (ValueError, OSError) as e: + # A later client wrote this file, or it is damaged. The caller falls + # back to a download of all events. + print(f"Warning: could not load the local rolling state: {e}") + return + self._rolling_state = state def _save_rolling_state(self) -> None: """Save rolling state to disk for cross-process consistency.""" @@ -537,8 +546,8 @@ def _create_resend_event(self, path: str) -> "FileChangeEvent | None": if content is None: return None from syft_client.sync.utils.syftbox_utils import ( - get_event_hash_from_content, create_event_timestamp, + get_event_hash_from_content, ) timestamp = create_event_timestamp() diff --git a/tests/unit/test_checkpoint_version.py b/tests/unit/test_checkpoint_version.py new file mode 100644 index 00000000000..38850d3c8d0 --- /dev/null +++ b/tests/unit/test_checkpoint_version.py @@ -0,0 +1,77 @@ +"""A checkpoint or rolling state from a later client is refused, not restored. + +Both models carry a `version` field that nothing read. A later client can change +what a field means while the object still parses, because pydantic accepts a +document that holds every field it knows. The restore would then be wrong and +silent. + +Refusing is cheap here. Every load site already falls back to a download of all +events when a checkpoint fails to load, so an unusable checkpoint costs one slow +cold start and nothing else. +""" + +import pytest +from syft_client.sync.checkpoints.checkpoint import ( + CHECKPOINT_VERSION, + Checkpoint, + IncrementalCheckpoint, +) +from syft_client.sync.checkpoints.rolling_state import ( + ROLLING_STATE_VERSION, + RollingState, +) + +EMAIL = "alice@example.com" + + +def _checkpoint(**kwargs) -> Checkpoint: + return Checkpoint(email=EMAIL, **kwargs) + + +def _incremental(**kwargs) -> IncrementalCheckpoint: + return IncrementalCheckpoint(email=EMAIL, sequence_number=1, **kwargs) + + +def _rolling(**kwargs) -> RollingState: + return RollingState(email=EMAIL, base_checkpoint_timestamp=1.0, **kwargs) + + +def test_a_checkpoint_round_trips(): + loaded = Checkpoint.from_compressed_data(_checkpoint().as_compressed_data()) + assert loaded.version == CHECKPOINT_VERSION + + +def test_an_incremental_checkpoint_round_trips(): + loaded = IncrementalCheckpoint.from_compressed_data( + _incremental().as_compressed_data() + ) + assert loaded.version == CHECKPOINT_VERSION + + +def test_a_rolling_state_round_trips(): + loaded = RollingState.from_compressed_data(_rolling().as_compressed_data()) + assert loaded.version == ROLLING_STATE_VERSION + + +def test_a_later_checkpoint_is_refused(): + data = _checkpoint(version=CHECKPOINT_VERSION + 1).as_compressed_data() + with pytest.raises(ValueError, match=str(CHECKPOINT_VERSION + 1)): + Checkpoint.from_compressed_data(data) + + +def test_a_later_incremental_checkpoint_is_refused(): + data = _incremental(version=CHECKPOINT_VERSION + 1).as_compressed_data() + with pytest.raises(ValueError, match=str(CHECKPOINT_VERSION + 1)): + IncrementalCheckpoint.from_compressed_data(data) + + +def test_a_later_rolling_state_is_refused(): + data = _rolling(version=ROLLING_STATE_VERSION + 1).as_compressed_data() + with pytest.raises(ValueError, match=str(ROLLING_STATE_VERSION + 1)): + RollingState.from_compressed_data(data) + + +def test_an_earlier_version_still_loads(): + # Version 0 predates the field. Those objects are the shape this client reads. + data = _checkpoint(version=0).as_compressed_data() + assert Checkpoint.from_compressed_data(data).version == 0 From f7bf139782d213aef3a431e79dca7efacd3d0b33 Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Thu, 6 Aug 2026 20:43:54 -0300 Subject: [PATCH 11/15] Test that a job negotiated down to protocol 0 arrives and reads - Fix C3 case 1 from migration gaps review - The existing tests assert the negotiated version only; removing the protocol-0 codec fails this test and leaves those passing --- .../p2p/test_job_protocol_skew_delivery.py | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 tests/migrations/p2p/test_job_protocol_skew_delivery.py diff --git a/tests/migrations/p2p/test_job_protocol_skew_delivery.py b/tests/migrations/p2p/test_job_protocol_skew_delivery.py new file mode 100644 index 00000000000..3d0b63ccb54 --- /dev/null +++ b/tests/migrations/p2p/test_job_protocol_skew_delivery.py @@ -0,0 +1,91 @@ +"""A job written for a protocol-0 peer reaches that peer and reads back. + +The other tests in this folder stop at the negotiated version. They assert which +protocol the two sides agree on, not that a job written at that protocol arrives +and reads. That seam is where the dataset transport broke: negotiation chose a +layout the delivery path could not carry. + +This test drives the whole path: the peer advertises job protocol 0, the sender +negotiates down, writes the flat layout, syncs, and the receiver finds and reads +the job through its own scan. +""" + +from pathlib import Path + +import pytest +from syft_client.sync.syftbox_manager import SyftboxManager +from syft_migration import ProtocolSchema + +from tests.unit.utils import create_test_project_folder + + +def _job_schema(protocol_version: str) -> ProtocolSchema: + # The slim form a peer advertises in its VersionInfo. + return ProtocolSchema( + protocol_name="syft-job", + version=protocol_version, + supported_versions={"JobState": ["1"], "JobSubmissionMetadata": ["1"]}, + ) + + +@pytest.fixture +def pair(): + return SyftboxManager.pair_with_mock_drive_service_connection( + use_in_memory_cache=False, + sync_automatically=False, + ) + + +def _submit(ds_manager, do_manager, job_name: str) -> Path: + project_dir = create_test_project_folder(with_pyproject=False) + ds_manager.submit_python_job( + user=do_manager.email, + code_path=str(project_dir), + job_name=job_name, + entrypoint="main.py", + ) + do_manager.sync() + return project_dir + + +def test_a_job_for_a_protocol0_peer_uses_the_flat_layout(pair): + ds_manager, do_manager = pair + # The DO advertises job protocol 0, as a client of 0.1.38 or earlier does. + ds_manager.peer_manager.live_peer_schemas("syft-job")[do_manager.email] = ( + _job_schema("0") + ) + + ref = ds_manager.job_client.manager.new_submission_ref(do_manager.email, "skew.job") + assert ref.protocol_version == "0" + assert "/v0/" not in str(ref) and "/v1/" not in str(ref), ( + "protocol 0 is the flat layout, so the path carries no v segment" + ) + + +def test_a_job_for_a_protocol0_peer_arrives_and_reads(pair): + ds_manager, do_manager = pair + ds_manager.peer_manager.live_peer_schemas("syft-job")[do_manager.email] = ( + _job_schema("0") + ) + + _submit(ds_manager, do_manager, "skew.job") + + # The receiver scans every layout it knows, so it finds the flat one. + assert [job.name for job in do_manager.jobs] == ["skew.job"] + found = do_manager.job_client.manager.find_submission_ref( + do_manager.email, "skew.job" + ) + assert found.protocol_version == "0" + + +def test_a_job_for_a_current_peer_still_uses_the_versioned_layout(pair): + # The control: without a protocol-0 peer the sender keeps the current layout, + # so the test above measures negotiation and not a broken default. + ds_manager, do_manager = pair + _submit(ds_manager, do_manager, "current.job") + + found = do_manager.job_client.manager.find_submission_ref( + do_manager.email, "current.job" + ) + assert found.protocol_version != "0" + assert [job.name for job in do_manager.jobs] == ["current.job"] From 28972a1af4185caf5a03420207451f0ebc79f9c8 Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Thu, 6 Aug 2026 21:38:12 -0300 Subject: [PATCH 12/15] Close migration gap by documentation and drop unused config flag - Fix A5 item from migration gaps review; the entry named the wrong pair, the different is between the two dataset methods, not jobs vs datasets. --- .../src/syft_datasets/dataset_storage.py | 21 +++++- packages/syft-job/src/syft_job/job_storage.py | 11 +++ syft_client/sync/peers/peer_store.py | 6 +- .../sync/sync/caches/persisted_dict.py | 2 +- syft_client/sync/version/peer_manager.py | 5 +- .../p2p/test_unknown_peer_forced_path.py | 75 +++++++++++++++++++ 6 files changed, 111 insertions(+), 9 deletions(-) create mode 100644 tests/migrations/p2p/test_unknown_peer_forced_path.py diff --git a/packages/syft-datasets/src/syft_datasets/dataset_storage.py b/packages/syft-datasets/src/syft_datasets/dataset_storage.py index d768010afaa..fef85ca65a4 100644 --- a/packages/syft-datasets/src/syft_datasets/dataset_storage.py +++ b/packages/syft-datasets/src/syft_datasets/dataset_storage.py @@ -1,3 +1,4 @@ +import logging import shutil from dataclasses import dataclass, field from datetime import datetime, timezone @@ -27,6 +28,8 @@ from .protocolcodecs import CODECS, ProtocolCodec from .url import SyftBoxURL +logger = logging.getLogger(__name__) + __all__ = [ "DatasetRef", "DatasetNotFoundError", @@ -153,6 +156,14 @@ def negotiated_protocol_version_for_peer( raise MigrationError( f"No dataset protocol schema known for peer {peer_email!r}" ) + # raise_on_unknown=False skips the refusal of a peer with an unknown + # version. A peer that speaks an earlier protocol does not read this + # layout. The dataset never arrives. + logger.warning( + f"No dataset protocol schema known for peer {peer_email!r}. This " + f"client writes dataset protocol {DATASET_PROTOCOL_VERSION}. A peer " + "that speaks an earlier protocol will not read this dataset." + ) return DATASET_PROTOCOL_VERSION def target_protocol_versions_for_peers( @@ -162,8 +173,14 @@ def target_protocol_versions_for_peers( A dataset is written once per distinct version in the audience. A known peer contributes ``min(ours, theirs)``; an unknown peer (or no audience) - contributes the widest-compatible protocol, since we cannot assume it can - read a newer layout. + contributes the widest-compatible protocol, since we cannot assume it + can read a newer layout. + + The two unknown-peer answers differ on purpose. This method serves an + audience. An unknown peer therefore takes the widest protocol, and every + reader can read a copy. ``negotiated_protocol_version_for_peer`` serves + one peer, so an unknown peer takes the current protocol. The caller of + that method accepts the risk when it passes ``raise_on_unknown=False``. """ if not peer_emails: return {self._widest_protocol_version} diff --git a/packages/syft-job/src/syft_job/job_storage.py b/packages/syft-job/src/syft_job/job_storage.py index aecc96484c6..c5a4acdcdc5 100644 --- a/packages/syft-job/src/syft_job/job_storage.py +++ b/packages/syft-job/src/syft_job/job_storage.py @@ -1,3 +1,4 @@ +import logging from pathlib import Path from typing import Iterator, Optional @@ -15,6 +16,8 @@ from .models import JobState, JobSubmissionMetadata from .protocolcodecs import CODECS, ProtocolCodec +logger = logging.getLogger(__name__) + __all__ = ["JobRef", "JobStateNotFoundError", "JobStorage"] @@ -87,6 +90,14 @@ def negotiated_protocol_version_for_peer( raise MigrationError( f"No job protocol schema known for peer {peer_email!r}" ) + # raise_on_unknown=False skips the refusal of a peer with an unknown + # version. A peer that speaks an earlier protocol does not scan this + # layout. It never sees the job. + logger.warning( + f"No job protocol schema known for peer {peer_email!r}. This client " + f"writes job protocol {JOB_PROTOCOL_VERSION}. A peer that speaks an " + "earlier protocol will not see this job." + ) return JOB_PROTOCOL_VERSION def _get_write_target_schema( diff --git a/syft_client/sync/peers/peer_store.py b/syft_client/sync/peers/peer_store.py index e349f0a745c..5c1c662795d 100644 --- a/syft_client/sync/peers/peer_store.py +++ b/syft_client/sync/peers/peer_store.py @@ -21,8 +21,8 @@ CRYPTO_KEYS_FILENAME = "crypto_keys.json" # Format of the crypto key file. Raise it when the layout of the file changes, -# and add a read path for every earlier version. A file with no version predates -# the field and is version 0. +# and add a read path for every earlier version. A file with no version was +# written before the field, and is version 0. CRYPTO_KEYS_VERSION = 1 @@ -251,7 +251,7 @@ def save_keys(self, path: Path) -> None: @classmethod def load_keys(cls, path: Path) -> "PeerStore": data = json.loads(Path(path).read_text()) - # A file with no version predates the field, and its layout is the one + # A file with no version was written before the field, and its layout is # this client reads. A later version is refused: a user cannot rebuild a # private key, so a wrong read loses the keys. version = data.get("version", 0) diff --git a/syft_client/sync/sync/caches/persisted_dict.py b/syft_client/sync/sync/caches/persisted_dict.py index c3f412d54d8..5e43ee0fe9c 100644 --- a/syft_client/sync/sync/caches/persisted_dict.py +++ b/syft_client/sync/sync/caches/persisted_dict.py @@ -33,7 +33,7 @@ # Format of the persisted file: {"version": N, "entries": {...}}. Raise it when # the layout of an entry changes. A file with no version holds the entries at the -# top level and predates the field, so it is version 0. +# top level, was written before the field, and is version 0. PERSISTED_DICT_VERSION = 1 diff --git a/syft_client/sync/version/peer_manager.py b/syft_client/sync/version/peer_manager.py index 8756d421ba9..f07ffcef50c 100644 --- a/syft_client/sync/version/peer_manager.py +++ b/syft_client/sync/version/peer_manager.py @@ -99,8 +99,9 @@ class PeerManagerConfig(BaseModel): syftbox_folder: Path email: str = "" connection_configs: List[ConnectionConfig] = [] + # Applies to a peer of unknown version only. A client version difference does + # not skip a peer, so this flag has no effect on one. force_ignore_peer_version: bool = False - force_ignore_protocol_version: bool = True suppress_version_warnings: bool = False n_threads: int = 10 has_do_role: bool = False @@ -140,7 +141,6 @@ class PeerManager(BaseModel): connection_router: ConnectionRouter peer_store: PeerStore force_ignore_peer_version: bool = False - force_ignore_protocol_version: bool = True suppress_version_warnings: bool = False n_threads: int = 10 has_do_role: bool = False @@ -211,7 +211,6 @@ def from_config(cls, config: PeerManagerConfig, email: str = "") -> "PeerManager connection_router=connection_router, peer_store=peer_store, force_ignore_peer_version=config.force_ignore_peer_version, - force_ignore_protocol_version=config.force_ignore_protocol_version, suppress_version_warnings=config.suppress_version_warnings, n_threads=config.n_threads, has_do_role=config.has_do_role, diff --git a/tests/migrations/p2p/test_unknown_peer_forced_path.py b/tests/migrations/p2p/test_unknown_peer_forced_path.py new file mode 100644 index 00000000000..f680d87ce70 --- /dev/null +++ b/tests/migrations/p2p/test_unknown_peer_forced_path.py @@ -0,0 +1,75 @@ +"""A forced submission reports the protocol version that it assumes. + +A peer of unknown version is refused before this point. A caller that passes +``raise_on_unknown=False`` skips that refusal. The storage then assumes the +current protocol. + +If the peer speaks an earlier protocol, it does not scan this layout. The job or +the dataset never arrives, so the storage writes a warning. +""" + +import logging +from pathlib import Path + +from syft_datasets.config import SyftBoxConfig +from syft_datasets.dataset_storage import DatasetStorage +from syft_datasets.migrations.registry import DATASET_PROTOCOL_VERSION +from syft_job import SyftJobConfig +from syft_job.job_storage import JobStorage +from syft_job.migrations.registry import JOB_PROTOCOL_VERSION + +DO_EMAIL = "do@test.org" +DS_EMAIL = "ds@test.org" + + +def _job_storage(tmp_path: Path) -> JobStorage: + config = SyftJobConfig( + syftbox_folder=tmp_path / "SyftBox", current_user_email=DS_EMAIL + ) + (tmp_path / "SyftBox" / DS_EMAIL).mkdir(parents=True, exist_ok=True) + return JobStorage(config=config, peer_schemas={}) + + +def _dataset_storage(tmp_path: Path) -> DatasetStorage: + config = SyftBoxConfig(syftbox_folder=tmp_path / "SyftBox", email=DO_EMAIL) + (tmp_path / "SyftBox" / DO_EMAIL).mkdir(parents=True, exist_ok=True) + return DatasetStorage(config=config, peer_schemas={}) + + +def test_a_forced_job_reports_the_assumed_protocol(tmp_path, caplog): + storage = _job_storage(tmp_path) + with caplog.at_level(logging.WARNING): + version = storage.negotiated_protocol_version_for_peer( + DO_EMAIL, raise_on_unknown=False + ) + assert version == JOB_PROTOCOL_VERSION + messages = " ".join(r.getMessage() for r in caplog.records) + assert DO_EMAIL in messages + assert "earlier protocol" in messages + + +def test_a_forced_dataset_reports_the_assumed_protocol(tmp_path, caplog): + storage = _dataset_storage(tmp_path) + with caplog.at_level(logging.WARNING): + version = storage.negotiated_protocol_version_for_peer( + DS_EMAIL, raise_on_unknown=False + ) + assert version == DATASET_PROTOCOL_VERSION + messages = " ".join(r.getMessage() for r in caplog.records) + assert DS_EMAIL in messages + assert "earlier protocol" in messages + + +def test_a_known_peer_reports_nothing(tmp_path, caplog): + # The report belongs to the forced path only. A known peer is negotiated. + from syft_migration import ProtocolSchema + + storage = _job_storage(tmp_path) + storage.peer_schemas[DO_EMAIL] = ProtocolSchema( + protocol_name="syft-job", + version=JOB_PROTOCOL_VERSION, + supported_versions={"JobState": ["1"]}, + ) + with caplog.at_level(logging.WARNING): + storage.negotiated_protocol_version_for_peer(DO_EMAIL) + assert caplog.records == [] From 71a52c1081612929eca38a8f0e824217cd31243a Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Tue, 11 Aug 2026 14:17:00 -0300 Subject: [PATCH 13/15] Deliver a dataset in every protocol layout its audience reads - Fix A1 from migration gaps review. The dataset transport dropped the protocol version: the sender flattened every file and the receiver rebuilt a flat path, so a v1 dataset arrived with metadata that pointed at a directory that was not there. - create_dataset now writes one copy for each layout in the audience, and each copy gets its own collection. Every collection is shared with the whole audience, so a peer that upgrades later moves to the newer layout with no action by the owner. - The collection folder name carries the version as a v infix before the separator. A client that predates multi-copy searches for the separator and so never lists a layout it cannot read. A protocol-0 name is unchanged, byte for byte. - Private data goes up with the copy that owns it. The copies hold separate private directories, so one upload of the newest left the others local only and a cold start did not restore them. - The watcher keeps the newest readable layout for each dataset, and warns and skips the rest. It keeps a local copy when the owner still publishes the dataset but in no layout this client reads, because that copy is the last one this client could read. - Login writes the remote version file. Only test helpers wrote it before, so the remote file kept the version that first created it. The mismatch check then prompted at every login, and a peer negotiated a job or dataset protocol version from a stale number. Closes the login item of A3. - The login mismatch prompt keeps local and remote data by default and repairs on the next sync. A full wipe is an explicit second choice. delete_unversioned_state is gone with the old first choice. A run with no terminal takes the keep-everything default instead of blocking. --- .../src/syft_datasets/dataset_manager.py | 35 +- .../src/syft_datasets/models/dataset/v1.py | 5 + .../sync/connections/base_connection.py | 39 +- .../sync/connections/connection_router.py | 65 ++- .../connections/drive/gdrive_transport.py | 384 +++++++++-------- syft_client/sync/login.py | 6 +- syft_client/sync/login_utils.py | 62 +-- syft_client/sync/syftbox_manager.py | 181 ++++---- .../sync/sync/caches/datasite_owner_cache.py | 43 +- .../sync/caches/datasite_watcher_cache.py | 172 ++++++-- .../sync/sync/datasite_owner_syncer.py | 63 ++- .../p2p/test_dataset_multicopy_delivery.py | 388 ++++++++++++++++++ tests/unit/test_create_dataset_cleanup.py | 6 +- tests/unit/test_dataset_upload_private.py | 16 +- tests/unit/test_delete_syftbox.py | 133 +++--- tests/unit/test_encryption.py | 2 +- tests/unit/test_sync_manager.py | 34 +- tests/unit/test_version_mismatch_flow.py | 290 ++++++++----- 18 files changed, 1362 insertions(+), 562 deletions(-) create mode 100644 tests/migrations/p2p/test_dataset_multicopy_delivery.py diff --git a/packages/syft-datasets/src/syft_datasets/dataset_manager.py b/packages/syft-datasets/src/syft_datasets/dataset_manager.py index 686a62c9517..c60143182d3 100644 --- a/packages/syft-datasets/src/syft_datasets/dataset_manager.py +++ b/packages/syft-datasets/src/syft_datasets/dataset_manager.py @@ -82,6 +82,38 @@ def create( Returns: Dataset: The created Dataset object (the newest protocol version written). """ + created = self.create_all( + name=name, + mock_path=mock_path, + private_path=private_path, + summary=summary, + readme_path=readme_path, + location=location, + tags=tags, + users=users, + protocol_versions=protocol_versions, + ) + # Return the newest protocol version written (richest layout). + return created[max(created, key=int)] + + def create_all( + self, + name: str, + mock_path: PathLike, + private_path: PathLike, + summary: str | None = None, + readme_path: Path | None = None, + location: str | None = None, + tags: list[str] | None = None, + users: list[str] | str | None = None, + protocol_versions: list[str] | None = None, + ) -> dict[str, "Dataset"]: + """Create a dataset and return every protocol copy it wrote. + + Same as ``create``, but returns {protocol_version: Dataset} instead of + one copy. A caller that puts the dataset on a transport needs them all, + because each copy goes to the peers that read its layout. + """ source = DatasetSourceFiles( mock=to_path(mock_path), private=to_path(private_path), @@ -98,8 +130,7 @@ def create( ) for dataset in created.values(): self._set_new_dataset_permissions(dataset=dataset, users=users) - # Return the newest protocol version written (richest layout). - return created[max(created, key=int)] + return created def migrate( self, diff --git a/packages/syft-datasets/src/syft_datasets/models/dataset/v1.py b/packages/syft-datasets/src/syft_datasets/models/dataset/v1.py index a98ca667e1e..9c1a1c50bf2 100644 --- a/packages/syft-datasets/src/syft_datasets/models/dataset/v1.py +++ b/packages/syft-datasets/src/syft_datasets/models/dataset/v1.py @@ -69,6 +69,11 @@ def disk_dict(self) -> dict: def owner(self) -> str: return self._ref.owner + @property + def protocol_version(self) -> str: + """The protocol version of the on-disk layout that holds this copy.""" + return self._ref.protocol_version + @property def syftbox_config(self) -> SyftBoxConfig: if self._syftbox_config is None: diff --git a/syft_client/sync/connections/base_connection.py b/syft_client/sync/connections/base_connection.py index a1c5afc881f..146352a2118 100644 --- a/syft_client/sync/connections/base_connection.py +++ b/syft_client/sync/connections/base_connection.py @@ -10,6 +10,9 @@ class FileCollection(BaseModel): tag: str content_hash: str has_any_permission: bool = False + # The protocol version whose layout this collection holds. A dataset has one + # collection for each version that its audience reads. + protocol_version: str = "0" class ConnectionConfig(BaseModel): @@ -33,20 +36,30 @@ def from_config(cls, config: ConnectionConfig): return config.connection_type.from_config(config) def owner_create_dataset_collection_folder( - self, tag: str, content_hash: str, owner_email: str + self, tag: str, content_hash: str, owner_email: str, protocol_version: str = "0" ) -> str: raise NotImplementedError() - def owner_tag_dataset_collection_as_any(self, tag: str, content_hash: str) -> None: + def owner_tag_dataset_collection_as_any( + self, tag: str, content_hash: str, protocol_version: str = "0" + ) -> None: raise NotImplementedError() def owner_share_dataset_collection( - self, tag: str, content_hash: str, users: list[str] + self, + tag: str, + content_hash: str, + users: list[str], + protocol_version: str = "0", ) -> None: raise NotImplementedError() def owner_upload_dataset_files( - self, tag: str, content_hash: str, files: dict[str, bytes] + self, + tag: str, + content_hash: str, + files: dict[str, bytes], + protocol_version: str = "0", ) -> None: raise NotImplementedError() @@ -60,21 +73,29 @@ def owner_list_all_dataset_collections_with_permissions( raise NotImplementedError() def watcher_list_dataset_collections(self) -> list[dict]: - """Returns list of dicts with keys: owner_email, tag, content_hash""" + """Returns dicts with: owner_email, tag, content_hash, protocol_version""" raise NotImplementedError() def watcher_download_dataset_collection( - self, tag: str, content_hash: str, owner_email: str + self, + tag: str, + content_hash: str, + owner_email: str, + protocol_version: str = "0", ) -> dict[str, bytes]: raise NotImplementedError() def owner_create_private_dataset_collection_folder( - self, tag: str, content_hash: str, owner_email: str + self, tag: str, content_hash: str, owner_email: str, protocol_version: str = "0" ) -> str: raise NotImplementedError() def owner_upload_private_dataset_files( - self, tag: str, content_hash: str, files: dict[str, bytes] + self, + tag: str, + content_hash: str, + files: dict[str, bytes], + protocol_version: str = "0", ) -> None: raise NotImplementedError() @@ -82,7 +103,7 @@ def owner_list_private_dataset_collections(self) -> list[FileCollection]: raise NotImplementedError() def owner_get_private_collection_file_metadatas( - self, tag: str, content_hash: str, owner_email: str + self, tag: str, content_hash: str, owner_email: str, protocol_version: str = "0" ) -> list[dict]: raise NotImplementedError() diff --git a/syft_client/sync/connections/connection_router.py b/syft_client/sync/connections/connection_router.py index 38c18eb5301..d833bce0d9d 100644 --- a/syft_client/sync/connections/connection_router.py +++ b/syft_client/sync/connections/connection_router.py @@ -281,22 +281,32 @@ def read_peer_encryption_bundle(self, peer_email: str) -> str | None: # ========================================================================= def owner_create_dataset_collection_folder( - self, tag: str, content_hash: str, owner_email: str + self, tag: str, content_hash: str, owner_email: str, protocol_version: str = "0" ) -> str: connection = self.connection_for_send_message() return connection.owner_create_dataset_collection_folder( - tag, content_hash, owner_email + tag, content_hash, owner_email, protocol_version ) - def owner_tag_dataset_collection_as_any(self, tag: str, content_hash: str) -> None: + def owner_tag_dataset_collection_as_any( + self, tag: str, content_hash: str, protocol_version: str = "0" + ) -> None: connection = self.connection_for_send_message() - connection.owner_tag_dataset_collection_as_any(tag, content_hash) + connection.owner_tag_dataset_collection_as_any( + tag, content_hash, protocol_version + ) def owner_share_dataset_collection( - self, tag: str, content_hash: str, users: list[str] + self, + tag: str, + content_hash: str, + users: list[str], + protocol_version: str = "0", ) -> None: connection = self.connection_for_send_message() - connection.owner_share_dataset_collection(tag, content_hash, users) + connection.owner_share_dataset_collection( + tag, content_hash, users, protocol_version + ) def owner_upload_dataset_files( self, @@ -304,6 +314,7 @@ def owner_upload_dataset_files( content_hash: str, files: dict[str, bytes], recipient_email: str | None = None, + protocol_version: str = "0", ) -> None: """Upload dataset files, encrypting each file if encryption is enabled.""" if recipient_email and self.peer_store: @@ -312,7 +323,9 @@ def owner_upload_dataset_files( for name, data in files.items() } connection = self.connection_for_send_message() - connection.owner_upload_dataset_files(tag, content_hash, files) + connection.owner_upload_dataset_files( + tag, content_hash, files, protocol_version + ) def owner_list_dataset_collections(self) -> list[str]: connection = self.connection_for_send_message() @@ -337,11 +350,15 @@ def watcher_list_dataset_collections(self) -> list[dict]: return connection.watcher_list_dataset_collections() def watcher_download_dataset_collection( - self, tag: str, content_hash: str, owner_email: str + self, + tag: str, + content_hash: str, + owner_email: str, + protocol_version: str = "0", ) -> dict[str, bytes]: connection = self.connection_for_datasite_watcher() files = connection.watcher_download_dataset_collection( - tag, content_hash, owner_email + tag, content_hash, owner_email, protocol_version ) if self.peer_store and owner_email: files = { @@ -351,29 +368,39 @@ def watcher_download_dataset_collection( return files def owner_create_private_dataset_collection_folder( - self, tag: str, content_hash: str, owner_email: str + self, tag: str, content_hash: str, owner_email: str, protocol_version: str = "0" ) -> str: connection = self.connection_for_send_message() return connection.owner_create_private_dataset_collection_folder( - tag, content_hash, owner_email + tag, content_hash, owner_email, protocol_version ) def owner_upload_private_dataset_files( - self, tag: str, content_hash: str, files: dict[str, bytes] + self, + tag: str, + content_hash: str, + files: dict[str, bytes], + protocol_version: str = "0", ) -> None: connection = self.connection_for_send_message() - connection.owner_upload_private_dataset_files(tag, content_hash, files) + connection.owner_upload_private_dataset_files( + tag, content_hash, files, protocol_version + ) def owner_list_private_dataset_collections(self) -> list[FileCollection]: connection = self.connection_for_send_message() return connection.owner_list_private_dataset_collections() def owner_get_private_collection_file_metadatas( - self, tag: str, content_hash: str, owner_email: str + self, + tag: str, + content_hash: str, + owner_email: str, + protocol_version: str = "0", ) -> List[dict]: connection = self.connection_for_datasite_watcher() return connection.owner_get_private_collection_file_metadatas( - tag, content_hash, owner_email + tag, content_hash, owner_email, protocol_version ) def connection_for_version_read( @@ -407,11 +434,15 @@ def share_version_file_with_peer(self, peer_email: str) -> None: connection.share_version_file_with_peer(peer_email) def watcher_get_dataset_collection_file_metadatas( - self, tag: str, content_hash: str, owner_email: str + self, + tag: str, + content_hash: str, + owner_email: str, + protocol_version: str = "0", ) -> List[dict]: connection = self.connection_for_datasite_watcher() return connection.watcher_get_dataset_collection_file_metadatas( - tag, content_hash, owner_email + tag, content_hash, owner_email, protocol_version ) def watcher_download_dataset_file(self, file_id: str, owner_email: str) -> bytes: diff --git a/syft_client/sync/connections/drive/gdrive_transport.py b/syft_client/sync/connections/drive/gdrive_transport.py index e5476adb025..af02d0272f2 100644 --- a/syft_client/sync/connections/drive/gdrive_transport.py +++ b/syft_client/sync/connections/drive/gdrive_transport.py @@ -4,8 +4,9 @@ import json import logging import pickle +import re from pathlib import Path -from typing import TYPE_CHECKING, Any, Optional +from typing import TYPE_CHECKING, Any, ClassVar, Optional from google.oauth2.credentials import Credentials as GoogleCredentials from google_auth_httplib2 import AuthorizedHttp @@ -17,6 +18,7 @@ PRIVATE_DATASET_COLLECTION_PREFIX, ) from syft_migration import MigrationError +from typing_extensions import Self from syft_client.sync.checkpoints.checkpoint import ( CHECKPOINT_FILENAME_PREFIX, @@ -152,54 +154,64 @@ def as_string(self) -> str: return f"{SYFT_CLIENT_VERSION}#{self.email}" -class DatasetCollectionFolder(BaseModel): - """Represents a dataset collection folder with format: {prefix}_{tag}_{hash}""" +def _collection_name_query(prefix: str) -> str: + """A Drive query that finds a collection of any protocol version. - tag: str - content_hash: str + It has no trailing '_', because a versioned name puts 'v' in that + position. A client that predates multi-copy searches with the '_' and so + never lists a layout that it cannot read. + """ + return f"name contains '{prefix}'" - def as_string(self) -> str: - return f"{DATASET_COLLECTION_PREFIX}_{self.tag}_{self.content_hash}" - @classmethod - def from_name(cls, name: str) -> "DatasetCollectionFolder": - """Parse folder name like 'syft_datasetcollection_mytag_abc123'""" - parts = name.split("_") - if len(parts) < 3: - raise ValueError(f"Invalid dataset collection folder name: {name}") - # prefix is parts[0:2] joined = "syft_datasetcollection" - # tag is parts[2:-1] joined (in case tag has underscores) - # hash is parts[-1] - tag = "_".join(parts[2:-1]) - content_hash = parts[-1] - return cls(tag=tag, content_hash=content_hash) +def _collection_name_re(prefix: str) -> "re.Pattern[str]": + """Matches '{prefix}_{tag}_{hash}' and '{prefix}v{n}_{tag}_{hash}'. - @staticmethod - def compute_hash(files: dict[str, bytes]) -> str: - """Compute a hash from file contents.""" - from syft_client.sync.file_utils import compute_file_hashes + The tag can hold an underscore, so it takes every character up to the last + one. The hash holds none. + """ + return re.compile( + rf"^{re.escape(prefix)}" + r"(?:v(?P\d+))?_(?P.+)_(?P[^_]+)$" + ) - return compute_file_hashes(files) +class _CollectionFolder(BaseModel): + """One dataset collection on Drive, in the layout of one protocol version. + + A dataset goes to a mixed audience as one collection for each protocol + version that the audience reads. The protocol version is part of the folder + name, so a peer selects the copy that it can read. -class PrivateDatasetCollectionFolder(BaseModel): - """Represents a private dataset collection folder with format: {prefix}_{tag}_{hash}""" + A subclass sets ``PREFIX``. Protocol 0 keeps the name that clients before + multi-copy write and read. + """ + + PREFIX: ClassVar[str] + NAME_RE: ClassVar["re.Pattern[str]"] tag: str content_hash: str + protocol_version: str = "0" def as_string(self) -> str: - return f"{PRIVATE_DATASET_COLLECTION_PREFIX}_{self.tag}_{self.content_hash}" + return f"{self.PREFIX}{self._version_infix}_{self.tag}_{self.content_hash}" + + @property + def _version_infix(self) -> str: + return "" if self.protocol_version == "0" else f"v{self.protocol_version}" @classmethod - def from_name(cls, name: str) -> "PrivateDatasetCollectionFolder": - """Parse folder name like 'syft_privatecollection_mytag_abc123'""" - parts = name.split("_") - if len(parts) < 3: - raise ValueError(f"Invalid private collection folder name: {name}") - tag = "_".join(parts[2:-1]) - content_hash = parts[-1] - return cls(tag=tag, content_hash=content_hash) + def from_name(cls, name: str) -> Self: + """Parse a collection folder name. A name with no version is protocol 0.""" + match = cls.NAME_RE.match(name) + if match is None: + raise ValueError(f"Invalid {cls.PREFIX} folder name: {name}") + return cls( + tag=match.group("tag"), + content_hash=match.group("content_hash"), + protocol_version=match.group("protocol_version") or "0", + ) @staticmethod def compute_hash(files: dict[str, bytes]) -> str: @@ -209,6 +221,35 @@ def compute_hash(files: dict[str, bytes]) -> str: return compute_file_hashes(files) +class DatasetCollectionFolder(_CollectionFolder): + """The collection a peer reads to get the mock files of a dataset.""" + + PREFIX: ClassVar[str] = DATASET_COLLECTION_PREFIX + NAME_RE: ClassVar["re.Pattern[str]"] = _collection_name_re( + DATASET_COLLECTION_PREFIX + ) + + +class PrivateDatasetCollectionFolder(_CollectionFolder): + """The owner-only collection that holds the private files of a dataset. + + Only the owner reads it. It still holds the protocol version, because the + private files must go back to the directory that the metadata of that copy + points to. + """ + + PREFIX: ClassVar[str] = PRIVATE_DATASET_COLLECTION_PREFIX + NAME_RE: ClassVar["re.Pattern[str]"] = _collection_name_re( + PRIVATE_DATASET_COLLECTION_PREFIX + ) + + +DATASET_COLLECTION_NAME_QUERY = _collection_name_query(DATASET_COLLECTION_PREFIX) +PRIVATE_COLLECTION_NAME_QUERY = _collection_name_query( + PRIVATE_DATASET_COLLECTION_PREFIX +) + + # Helpers for finding folders whose names embed SYFT_CLIENT_VERSION. Folder # names use '#' or '-' as field separators with the version as one field, # so we walk those fields looking for an X.Y.Z-shaped chunk -- no per-format @@ -1308,74 +1349,6 @@ def delete_file_by_id(self, file_id: str, raise_on_error: bool = False): raise e print(f"Warning: could not delete file {file_id}: {e}") - def delete_unversioned_state(self) -> None: - """Delete non-versioned remote artifacts during upgrade. - - Removes encryption bundles, dataset collections, private collections, - peers file, and version file from /SyftBox/. - """ - syftbox_folder_id = self.get_syftbox_folder_id() - ids_to_delete: list[str] = [] - - # 1. Encryption bundles folder - enc_folder_name = GdriveEncryptionBundlesFolder(email=self.email).as_string() - enc_folder_id = self._find_folder_by_name( - enc_folder_name, parent_id=syftbox_folder_id - ) - if enc_folder_id: - ids_to_delete.extend( - gather_all_file_and_folder_ids_recursive( - self.drive_service, enc_folder_id - ) - ) - ids_to_delete.append(enc_folder_id) - - # 2. Dataset collection folders (syft_datasetcollection_*) - ds_query = ( - f"name contains '{DATASET_COLLECTION_PREFIX}'" - f" and mimeType='{GOOGLE_FOLDER_MIME_TYPE}'" - f" and '{syftbox_folder_id}' in parents" - " and trashed=false" - ) - ds_results = execute_with_retries( - self.drive_service.files().list(q=ds_query, fields="files(id)") - ) - for f in ds_results.get("files", []): - ids_to_delete.extend( - gather_all_file_and_folder_ids_recursive(self.drive_service, f["id"]) - ) - ids_to_delete.append(f["id"]) - - # 3. Private collection folders (syft_privatecollection_*) - pc_query = ( - f"name contains '{PRIVATE_DATASET_COLLECTION_PREFIX}'" - f" and mimeType='{GOOGLE_FOLDER_MIME_TYPE}'" - f" and '{syftbox_folder_id}' in parents" - " and trashed=false" - ) - pc_results = execute_with_retries( - self.drive_service.files().list(q=pc_query, fields="files(id)") - ) - for f in pc_results.get("files", []): - ids_to_delete.extend( - gather_all_file_and_folder_ids_recursive(self.drive_service, f["id"]) - ) - ids_to_delete.append(f["id"]) - - # 4. SYFT_peers.json - peers_file_id = self._get_peers_file_id() - if peers_file_id: - ids_to_delete.append(peers_file_id) - - # 5. SYFT_version.json - version_file_id = self._get_version_file_id() - if version_file_id: - ids_to_delete.append(version_file_id) - - if ids_to_delete: - self.delete_multiple_files_by_ids(ids_to_delete) - self.reset_caches() - def find_orphaned_message_files(self) -> list[str]: """ Find syft files by name pattern owned by user, regardless of parent folder. @@ -1613,33 +1586,39 @@ def get_inbox_proposed_event_id_from_name( return items[0]["id"] if items else None def owner_create_dataset_collection_folder( - self, tag: str, content_hash: str, owner_email: str + self, tag: str, content_hash: str, owner_email: str, protocol_version: str = "0" ) -> str: - """Create /SyftBox/{DATASET_COLLECTION_PREFIX}_{tag}_{hash} folder.""" - folder_obj = DatasetCollectionFolder(tag=tag, content_hash=content_hash) + """Create the /SyftBox collection folder for one protocol version.""" + folder_obj = DatasetCollectionFolder( + tag=tag, content_hash=content_hash, protocol_version=protocol_version + ) folder_name = folder_obj.as_string() - cache_key = f"{tag}_{content_hash}" - # Check cache - if cache_key in self.dataset_collection_folder_id_cache: - return self.dataset_collection_folder_id_cache[cache_key] + # The name holds the version, so it keys the cache. A tag/hash key would + # give every protocol copy of a dataset the same entry. + if folder_name in self.dataset_collection_folder_id_cache: + return self.dataset_collection_folder_id_cache[folder_name] syftbox_folder_id = self.get_syftbox_folder_id() # Check if exists folder_id = self._find_folder_by_name(folder_name, parent_id=syftbox_folder_id) if folder_id: - self.dataset_collection_folder_id_cache[cache_key] = folder_id + self.dataset_collection_folder_id_cache[folder_name] = folder_id return folder_id # Create new folder folder_id = self.create_folder(folder_name, syftbox_folder_id) - self.dataset_collection_folder_id_cache[cache_key] = folder_id + self.dataset_collection_folder_id_cache[folder_name] = folder_id return folder_id - def owner_tag_dataset_collection_as_any(self, tag: str, content_hash: str) -> None: + def owner_tag_dataset_collection_as_any( + self, tag: str, content_hash: str, protocol_version: str = "0" + ) -> None: """Mark dataset collection as shared with 'any' via appProperties.""" - folder_id = self._get_dataset_collection_folder_id(tag, content_hash) + folder_id = self._get_dataset_collection_folder_id( + tag, content_hash, protocol_version + ) execute_with_retries( self.drive_service.files().update( fileId=folder_id, @@ -1648,12 +1627,18 @@ def owner_tag_dataset_collection_as_any(self, tag: str, content_hash: str) -> No ) def owner_share_dataset_collection( - self, tag: str, content_hash: str, users: list[str] + self, + tag: str, + content_hash: str, + users: list[str], + protocol_version: str = "0", ) -> None: """Share dataset collection folder with specific users via batch API.""" if not users: return - folder_id = self._get_dataset_collection_folder_id(tag, content_hash) + folder_id = self._get_dataset_collection_folder_id( + tag, content_hash, protocol_version + ) self._batch_add_permissions(folder_id, users) def _batch_add_permissions(self, file_id: str, users: list[str]) -> None: @@ -1684,10 +1669,20 @@ def callback(request_id, response, exception): batch_execute_with_retries(batch) def owner_upload_dataset_files( - self, tag: str, content_hash: str, files: dict[str, bytes] + self, + tag: str, + content_hash: str, + files: dict[str, bytes], + protocol_version: str = "0", ) -> None: - """Upload dataset files to collection folder.""" - folder_id = self._get_dataset_collection_folder_id(tag, content_hash) + """Upload dataset files to collection folder. + + The files stay flat in the folder. The collection name gives the protocol + version, and the peer builds the local directory for that version. + """ + folder_id = self._get_dataset_collection_folder_id( + tag, content_hash, protocol_version + ) for file_path, content in files.items(): file_payload, _ = self.create_file_payload(content) @@ -1701,10 +1696,14 @@ def owner_upload_dataset_files( ) def owner_list_dataset_collections(self) -> list[str]: - """List collections created by DO (owned by me).""" + """The tag of each dataset that this owner published. + + A dataset has one collection for each protocol version it was written + in, so a tag appears once here even when several collections hold it. + """ syftbox_folder_id = self.get_syftbox_folder_id() query = ( - f"name contains '{DATASET_COLLECTION_PREFIX}_' and '{syftbox_folder_id}' in parents " + f"{DATASET_COLLECTION_NAME_QUERY} and '{syftbox_folder_id}' in parents " f"and 'me' in owners and trashed=false and mimeType='{GOOGLE_FOLDER_MIME_TYPE}'" ) results = execute_with_retries( @@ -1712,13 +1711,14 @@ def owner_list_dataset_collections(self) -> list[str]: ) folders = results.get("files", []) - result = [] + result: list[str] = [] for folder in folders: try: folder_obj = DatasetCollectionFolder.from_name(folder["name"]) - result.append(folder_obj.tag) except ValueError: continue + if folder_obj.tag not in result: + result.append(folder_obj.tag) return result def owner_list_all_dataset_collections_with_permissions( @@ -1727,7 +1727,7 @@ def owner_list_all_dataset_collections_with_permissions( """List all DO's dataset collections with permissions info.""" syftbox_folder_id = self.get_syftbox_folder_id() query = ( - f"name contains '{DATASET_COLLECTION_PREFIX}_' and '{syftbox_folder_id}' in parents " + f"{DATASET_COLLECTION_NAME_QUERY} and '{syftbox_folder_id}' in parents " f"and 'me' in owners and trashed=false and mimeType='{GOOGLE_FOLDER_MIME_TYPE}'" ) results = execute_with_retries( @@ -1751,19 +1751,24 @@ def owner_list_all_dataset_collections_with_permissions( tag=folder_obj.tag, content_hash=folder_obj.content_hash, has_any_permission=has_anyone, + protocol_version=folder_obj.protocol_version, ) ) return collections def owner_delete_dataset_collection(self, tag: str) -> None: - """Delete all public dataset collection folders matching the given tag.""" + """Delete every public collection of this tag, in all protocol versions.""" collections = self.owner_list_all_dataset_collections_with_permissions() for c in collections: if c.tag == tag: self.delete_file_by_id(c.folder_id) - cache_key = f"{c.tag}_{c.content_hash}" - self.dataset_collection_folder_id_cache.pop(cache_key, None) + folder_name = DatasetCollectionFolder( + tag=c.tag, + content_hash=c.content_hash, + protocol_version=c.protocol_version, + ).as_string() + self.dataset_collection_folder_id_cache.pop(folder_name, None) def watcher_list_dataset_collections(self) -> list[dict]: """List collections shared with DS (not owned by me). @@ -1771,7 +1776,7 @@ def watcher_list_dataset_collections(self) -> list[dict]: Returns list of dicts with keys: owner_email, tag, content_hash """ query = ( - f"name contains '{DATASET_COLLECTION_PREFIX}_' and not 'me' in owners " + f"{DATASET_COLLECTION_NAME_QUERY} and not 'me' in owners " f"and trashed=false and mimeType='{GOOGLE_FOLDER_MIME_TYPE}'" ) results = execute_with_retries( @@ -1791,6 +1796,7 @@ def watcher_list_dataset_collections(self) -> list[dict]: "owner_email": owner_email, "tag": folder_obj.tag, "content_hash": folder_obj.content_hash, + "protocol_version": folder_obj.protocol_version, } ) except ValueError: @@ -1798,18 +1804,35 @@ def watcher_list_dataset_collections(self) -> list[dict]: continue return result + def _find_dataset_collection_folder_id( + self, tag: str, content_hash: str, owner_email: str, protocol_version: str + ) -> str: + """The Drive ID of a peer's collection for one protocol version.""" + folder_obj = DatasetCollectionFolder( + tag=tag, content_hash=content_hash, protocol_version=protocol_version + ) + # Find the folder by name, because the peer owns it. + folder_id = self._find_folder_by_name( + folder_obj.as_string(), owner_email=owner_email + ) + if not folder_id: + raise ValueError( + f"Collection {tag} with hash {content_hash} and protocol " + f"{protocol_version} not found" + ) + return folder_id + def watcher_download_dataset_collection( - self, tag: str, content_hash: str, owner_email: str + self, + tag: str, + content_hash: str, + owner_email: str, + protocol_version: str = "0", ) -> dict[str, bytes]: """Download all files from a dataset collection.""" - folder_obj = DatasetCollectionFolder(tag=tag, content_hash=content_hash) - folder_name = folder_obj.as_string() - # Try to find folder by name (could be owned by someone else) - folder_id = self._find_folder_by_name(folder_name, owner_email=owner_email) - - if not folder_id: - raise ValueError(f"Collection {tag} with hash {content_hash} not found") - + folder_id = self._find_dataset_collection_folder_id( + tag, content_hash, owner_email, protocol_version + ) file_metadatas = self.get_file_metadatas_from_folder(folder_id) files = {} for file_meta in file_metadatas: @@ -1820,16 +1843,16 @@ def watcher_download_dataset_collection( return files def watcher_get_dataset_collection_file_metadatas( - self, tag: str, content_hash: str, owner_email: str + self, + tag: str, + content_hash: str, + owner_email: str, + protocol_version: str = "0", ) -> list[dict]: """Get file metadata from a dataset collection without downloading.""" - folder_obj = DatasetCollectionFolder(tag=tag, content_hash=content_hash) - folder_name = folder_obj.as_string() - folder_id = self._find_folder_by_name(folder_name, owner_email=owner_email) - - if not folder_id: - raise ValueError(f"Collection {tag} with hash {content_hash} not found") - + folder_id = self._find_dataset_collection_folder_id( + tag, content_hash, owner_email, protocol_version + ) file_metadatas = self.get_file_metadatas_from_folder(folder_id) return [{"file_id": f["id"], "file_name": f["name"]} for f in file_metadatas] @@ -1837,23 +1860,27 @@ def watcher_download_dataset_file(self, file_id: str) -> bytes: """Download a single file from a dataset collection.""" return self.download_file(file_id) - def _get_dataset_collection_folder_id(self, tag: str, content_hash: str) -> str: + def _get_dataset_collection_folder_id( + self, tag: str, content_hash: str, protocol_version: str = "0" + ) -> str: """Get folder ID for dataset collection, with caching.""" - cache_key = f"{tag}_{content_hash}" - if cache_key in self.dataset_collection_folder_id_cache: - return self.dataset_collection_folder_id_cache[cache_key] - - folder_obj = DatasetCollectionFolder(tag=tag, content_hash=content_hash) + folder_obj = DatasetCollectionFolder( + tag=tag, content_hash=content_hash, protocol_version=protocol_version + ) folder_name = folder_obj.as_string() + if folder_name in self.dataset_collection_folder_id_cache: + return self.dataset_collection_folder_id_cache[folder_name] + syftbox_folder_id = self.get_syftbox_folder_id() folder_id = self._find_folder_by_name(folder_name, parent_id=syftbox_folder_id) if not folder_id: raise ValueError( - f"Collection folder {tag} with hash {content_hash} not found" + f"Collection folder {tag} with hash {content_hash} and protocol " + f"{protocol_version} not found" ) - self.dataset_collection_folder_id_cache[cache_key] = folder_id + self.dataset_collection_folder_id_cache[folder_name] = folder_id return folder_id # ========================================================================= @@ -1861,15 +1888,17 @@ def _get_dataset_collection_folder_id(self, tag: str, content_hash: str) -> str: # ========================================================================= def owner_create_private_dataset_collection_folder( - self, tag: str, content_hash: str, owner_email: str + self, tag: str, content_hash: str, owner_email: str, protocol_version: str = "0" ) -> str: - """Create /SyftBox/{PRIVATE_DATASET_COLLECTION_PREFIX}_{tag}_{hash} folder. + """Create the private collection folder for one protocol version. No sharing is applied — only the owner can access this folder. """ - folder_obj = PrivateDatasetCollectionFolder(tag=tag, content_hash=content_hash) + folder_obj = PrivateDatasetCollectionFolder( + tag=tag, content_hash=content_hash, protocol_version=protocol_version + ) folder_name = folder_obj.as_string() - cache_key = f"private_{tag}_{content_hash}" + cache_key = f"private_{folder_name}" if cache_key in self.dataset_collection_folder_id_cache: return self.dataset_collection_folder_id_cache[cache_key] @@ -1885,10 +1914,16 @@ def owner_create_private_dataset_collection_folder( return folder_id def owner_upload_private_dataset_files( - self, tag: str, content_hash: str, files: dict[str, bytes] + self, + tag: str, + content_hash: str, + files: dict[str, bytes], + protocol_version: str = "0", ) -> None: """Upload files to a private dataset collection folder.""" - folder_id = self._get_private_collection_folder_id(tag, content_hash) + folder_id = self._get_private_collection_folder_id( + tag, content_hash, protocol_version + ) for file_path, content in files.items(): file_payload, _ = self.create_file_payload(content) file_name = Path(file_path).name @@ -1903,7 +1938,7 @@ def owner_list_private_dataset_collections(self) -> list[FileCollection]: """List private collections owned by DO.""" syftbox_folder_id = self.get_syftbox_folder_id() query = ( - f"name contains '{PRIVATE_DATASET_COLLECTION_PREFIX}_' " + f"{PRIVATE_COLLECTION_NAME_QUERY} " f"and '{syftbox_folder_id}' in parents " f"and 'me' in owners and trashed=false " f"and mimeType='{GOOGLE_FOLDER_MIME_TYPE}'" @@ -1921,6 +1956,7 @@ def owner_list_private_dataset_collections(self) -> list[FileCollection]: folder_id=folder["id"], tag=folder_obj.tag, content_hash=folder_obj.content_hash, + protocol_version=folder_obj.protocol_version, ) ) except ValueError: @@ -1933,14 +1969,22 @@ def owner_delete_private_dataset_collection(self, tag: str) -> None: for c in collections: if c.tag == tag: self.delete_file_by_id(c.folder_id) - cache_key = f"private_{c.tag}_{c.content_hash}" - self.dataset_collection_folder_id_cache.pop(cache_key, None) + folder_name = PrivateDatasetCollectionFolder( + tag=c.tag, + content_hash=c.content_hash, + protocol_version=c.protocol_version, + ).as_string() + self.dataset_collection_folder_id_cache.pop( + f"private_{folder_name}", None + ) def owner_get_private_collection_file_metadatas( - self, tag: str, content_hash: str, owner_email: str + self, tag: str, content_hash: str, owner_email: str, protocol_version: str = "0" ) -> list[dict]: """Get file metadata from a private dataset collection without downloading.""" - folder_obj = PrivateDatasetCollectionFolder(tag=tag, content_hash=content_hash) + folder_obj = PrivateDatasetCollectionFolder( + tag=tag, content_hash=content_hash, protocol_version=protocol_version + ) folder_name = folder_obj.as_string() folder_id = self._find_folder_by_name(folder_name, owner_email=owner_email) @@ -1952,14 +1996,18 @@ def owner_get_private_collection_file_metadatas( file_metadatas = self.get_file_metadatas_from_folder(folder_id) return [{"file_id": f["id"], "file_name": f["name"]} for f in file_metadatas] - def _get_private_collection_folder_id(self, tag: str, content_hash: str) -> str: + def _get_private_collection_folder_id( + self, tag: str, content_hash: str, protocol_version: str = "0" + ) -> str: """Get folder ID for private dataset collection, with caching.""" - cache_key = f"private_{tag}_{content_hash}" + folder_obj = PrivateDatasetCollectionFolder( + tag=tag, content_hash=content_hash, protocol_version=protocol_version + ) + folder_name = folder_obj.as_string() + cache_key = f"private_{folder_name}" if cache_key in self.dataset_collection_folder_id_cache: return self.dataset_collection_folder_id_cache[cache_key] - folder_obj = PrivateDatasetCollectionFolder(tag=tag, content_hash=content_hash) - folder_name = folder_obj.as_string() syftbox_folder_id = self.get_syftbox_folder_id() folder_id = self._find_folder_by_name(folder_name, parent_id=syftbox_folder_id) diff --git a/syft_client/sync/login.py b/syft_client/sync/login.py index 600cab33b4b..08203ebf835 100644 --- a/syft_client/sync/login.py +++ b/syft_client/sync/login.py @@ -29,7 +29,11 @@ def _init_client_login( """Common post-creation initialization: write version, sync, load peers.""" _verify_token_matches_email(client) print_client_connecting(client.email) - client.write_local_version() + # Write the version file on both sides. A local-only write leaves the remote + # file at the version that first created it. Two things then break: the + # login mismatch check reads that stale file and prompts at every login, and + # a peer reads it to select a job or dataset protocol version for us. + client.peer_manager.write_own_version() if sync: client.sync() diff --git a/syft_client/sync/login_utils.py b/syft_client/sync/login_utils.py index d2d91516599..5221ae96094 100644 --- a/syft_client/sync/login_utils.py +++ b/syft_client/sync/login_utils.py @@ -25,17 +25,6 @@ def _read_remote_version( return conn.read_own_version_file() -def _delete_remote_unversioned_state( - email: str, - token_path: Optional[Path], -) -> None: - """Delete non-versioned remote state during upgrade.""" - from syft_client.sync.connections.drive.gdrive_transport import GDriveConnection - - conn = GDriveConnection.from_token_path(email=email, token_path=token_path) - conn.delete_unversioned_state() - - def _handle_version_incompatible( email: str, token_path: Optional[Path], @@ -43,18 +32,24 @@ def _handle_version_incompatible( local_version: Optional[VersionInfo], remote_version: Optional[VersionInfo], ) -> None: - """Handle version mismatch with unified prompt.""" + """Handle a client major/minor mismatch at login. + + The default is to keep local and remote data. Folder adopt, refuse-later + checks, and cache reset repair state on the next sync. A full wipe is an + explicit second choice only. + """ choice = _prompt_mismatch(local_version, remote_version) if choice == "1": - print(f"Upgrading to v{SYFT_CLIENT_VERSION}...") - delete_local_syftbox( - email=email, - local_syftbox_path=local_syftbox_path, - verbose=True, + print( + f"Continuing with v{SYFT_CLIENT_VERSION}. Local and remote data are " + "kept. Drive folders of an earlier client version are adopted on the " + "next sync, and caches and checkpoints rebuild themselves.\n" + "Encryption keys are the one exception. A key file from a newer " + "client is refused, because a private key cannot be rebuilt. Install " + "that client to use those keys.\n" ) - _delete_remote_unversioned_state(email, token_path) - print("Done. Continuing login.\n") - elif choice == "2": + return + if choice == "2": print(f"Deleting all state and starting fresh with v{SYFT_CLIENT_VERSION}...") delete_local_syftbox( email=email, @@ -67,22 +62,23 @@ def _handle_version_incompatible( verbose=True, ) print("Done. Continuing login.\n") - else: - print("Exiting.") - sys.exit(0) + return + print("Exiting.") + sys.exit(0) def handle_potential_version_mismatches_on_login( email: str, token_path: Optional[str | Path] = None, ) -> None: - """Check local and remote versions against installed version. + """Check local and remote versions against the installed client. Runs before client init. Creates a temporary GDrive connection to read the remote version file. - On mismatch, prompts user to upgrade (local delete only, remote preserved - via version subfolders) or hard-reset (delete everything). + On a major/minor mismatch, the default is to keep data and continue. The + user can still choose a full wipe, or quit. Patch differences are not a + mismatch. """ resolved_email = _resolve_email(email) resolved_token_path = _resolve_token_path(token_path) @@ -130,11 +126,21 @@ def _prompt_mismatch( local_version: Optional[VersionInfo], remote_version: Optional[VersionInfo], ) -> str: - """Prompt user about version mismatch. Returns choice.""" + """Prompt the user about a version mismatch. Returns the choice string.""" _print_version_status(local_version, remote_version) + if not sys.stdin.isatty(): + # No terminal, so no answer can arrive. Choice 1 keeps every file and + # changes nothing, so it is safe to take without an answer. A prompt + # here would stop a notebook or a scheduled run instead. + print( + "No terminal is attached. Continuing with all data kept.\n" + "To start fresh instead, call delete_local_syftbox and " + "delete_remote_syftbox, then log in again.\n" + ) + return "1" print( f""" -[1] Upgrade to v{SYFT_CLIENT_VERSION} and archive old data +[1] Continue with v{SYFT_CLIENT_VERSION} (keep data; repair on sync) [2] Delete all state and start fresh with v{SYFT_CLIENT_VERSION} [3] Quit diff --git a/syft_client/sync/syftbox_manager.py b/syft_client/sync/syftbox_manager.py index 739224dc441..5821303e104 100644 --- a/syft_client/sync/syftbox_manager.py +++ b/syft_client/sync/syftbox_manager.py @@ -59,9 +59,7 @@ PeerManager, PeerManagerConfig, ) -from syft_client.sync.version.version_info import VersionInfo from syft_client.utils import resolve_path -from syft_client.version import VERSION_FILE_NAME logger = logging.getLogger(__name__) @@ -460,21 +458,10 @@ class SyftboxManager(BaseModel): def __dir__(self): return list(self._PUBLIC_API) - def read_local_version(self) -> VersionInfo | None: - """Read the local SYFT_version.json from the SyftBox directory.""" - version_file = self.syftbox_folder / VERSION_FILE_NAME - if not version_file.exists(): - return None - try: - return VersionInfo.from_json(version_file.read_text()) - except Exception: - return None - - def write_local_version(self) -> None: - """Write current version info to a local SYFT_version.json.""" - self.syftbox_folder.mkdir(parents=True, exist_ok=True) - version_file = self.syftbox_folder / VERSION_FILE_NAME - version_file.write_text(VersionInfo.current().to_json()) + # Version file IO lives in syft_client.sync.version.local_version, and + # `write_own_version` writes both the local and the remote file. A + # local-only writer on the manager leaves the remote file stale, which is + # the bug that made the login mismatch prompt repeat at every login. @property def peers(self) -> PeerList: @@ -517,12 +504,13 @@ def from_config(cls, config: SyftboxManagerConfig): peer_manager = PeerManager.from_config( config.peer_manager_config, email=config.email ) - # Do not give the dataset manager a peer-schema map. Datasets go to a - # peer through the dataset-collection transport. This transport writes - # all the files of a dataset into COLLECTION_SUBPATH/. It cannot - # write a v directory. If the manager selects a newer layout, it - # writes metadata that points to a directory that the peer does not get. - dataset_manager = SyftDatasetManager.from_config(config.dataset_manager_config) + # The dataset manager gets the live peer-schema map, as the job client + # does. It selects a layout for each peer, and the transport carries one + # collection for each layout. + dataset_manager = SyftDatasetManager.from_config( + config.dataset_manager_config, + peer_schemas=peer_manager.live_peer_schemas("syft-dataset"), + ) job_client = JobClient.from_config( config.job_client_config, peer_schemas=peer_manager.live_peer_schemas("syft-job"), @@ -1069,10 +1057,14 @@ def _share_any_datasets_with_peer(self, peer_email: str): Uses cache populated during pull_initial_state() in DatasiteOwnerSyncer. """ - for tag, content_hash in self.datasite_owner_syncer._any_shared_datasets: + for ( + tag, + content_hash, + protocol_version, + ) in self.datasite_owner_syncer._any_shared_datasets: try: self._connection_router.owner_share_dataset_collection( - tag, content_hash, [peer_email] + tag, content_hash, [peer_email], protocol_version ) except Exception: # Ignore errors (e.g., already shared) @@ -1202,12 +1194,13 @@ def create_dataset( dataset_name = None created_local = False - mock_folder_id = None - private_folder_id = None + mock_folder_ids: list[str] = [] + private_folder_ids: list[str] = [] try: - # Create dataset locally - dataset = self.dataset_manager.create( + # Create the dataset locally, in one layout for each protocol + # version that the audience reads. + created = self.dataset_manager.create_all( name=name, mock_path=mock_path, private_path=private_path, @@ -1218,14 +1211,21 @@ def create_dataset( users=users, ) created_local = True + # The newest copy has the richest layout. It is what create returns. + dataset = created[max(created, key=int)] dataset_name = dataset.name - # Upload mock data to collection folder - mock_folder_id = self._upload_dataset_to_collection(dataset, users) - - # Upload private data to a separate owner-only collection - if upload_private: - private_folder_id = self._upload_private_dataset_to_collection(dataset) + # Each copy gets its own collections. The private data of a copy + # must go up with it: the copies hold separate private directories, + # so one upload of the newest would leave the others local only, and + # a cold start would not restore them. + for protocol_version in sorted(created, key=int): + copy = created[protocol_version] + mock_folder_ids.append(self._upload_dataset_to_collection(copy, users)) + if upload_private: + private_folder_id = self._upload_private_dataset_to_collection(copy) + if private_folder_id is not None: + private_folder_ids.append(private_folder_id) if sync: self.sync() @@ -1238,7 +1238,7 @@ def create_dataset( f" '{dataset_name}'" if dataset_name else "", ) self._cleanup_failed_dataset_creation( - dataset_name, created_local, mock_folder_id, private_folder_id + dataset_name, created_local, mock_folder_ids, private_folder_ids ) raise @@ -1246,11 +1246,11 @@ def _cleanup_failed_dataset_creation( self, dataset_name: str | None, created_local: bool, - mock_folder_id: str | None, - private_folder_id: str | None, + mock_folder_ids: list[str], + private_folder_ids: list[str], ) -> None: """Best-effort cleanup after a failed create_dataset, in reverse order.""" - if private_folder_id is not None: + for private_folder_id in reversed(private_folder_ids): try: self._connection_router.delete_file_by_id(private_folder_id) except Exception: @@ -1259,7 +1259,7 @@ def _cleanup_failed_dataset_creation( private_folder_id, ) - if mock_folder_id is not None: + for mock_folder_id in reversed(mock_folder_ids): try: self._connection_router.delete_file_by_id(mock_folder_id) except Exception: @@ -1278,12 +1278,19 @@ def _cleanup_failed_dataset_creation( ) def _upload_dataset_to_collection(self, dataset, users: list[str] | str) -> str: - """Upload dataset files to collection folder. Returns the folder ID.""" + """Upload one protocol copy of a dataset. Returns the folder ID. + + Each copy gets its own collection, named for its protocol version. Every + copy goes to the whole audience, and each peer selects the newest copy + that it reads. A peer that upgrades later therefore moves to the newer + layout with no action by the owner. + """ from syft_client.sync.connections.drive.gdrive_transport import ( DatasetCollectionFolder, ) collection_tag = dataset.name + protocol_version = dataset.protocol_version # Prepare files to upload files = {} @@ -1303,31 +1310,43 @@ def _upload_dataset_to_collection(self, dataset, users: list[str] | str) -> str: # Create collection folder with hash in name folder_id = self._connection_router.owner_create_dataset_collection_folder( - tag=collection_tag, content_hash=content_hash, owner_email=self.email + tag=collection_tag, + content_hash=content_hash, + owner_email=self.email, + protocol_version=protocol_version, ) # Upload files self._connection_router.owner_upload_dataset_files( - collection_tag, content_hash, files + collection_tag, + content_hash, + files, + protocol_version=protocol_version, ) # Share with users if users == "any": self._connection_router.owner_tag_dataset_collection_as_any( - collection_tag, content_hash + collection_tag, content_hash, protocol_version=protocol_version ) self.datasite_owner_syncer._any_shared_datasets.append( - (collection_tag, content_hash) + (collection_tag, content_hash, protocol_version) ) # Share with all already-approved peers peer_emails = [p.email for p in self.peer_manager.approved_peers] if peer_emails: self._connection_router.owner_share_dataset_collection( - collection_tag, content_hash, peer_emails + collection_tag, + content_hash, + peer_emails, + protocol_version=protocol_version, ) else: self._connection_router.owner_share_dataset_collection( - collection_tag, content_hash, users + collection_tag, + content_hash, + users, + protocol_version=protocol_version, ) return folder_id @@ -1340,6 +1359,7 @@ def _upload_private_dataset_to_collection(self, dataset) -> str | None: ) collection_tag = dataset.name + protocol_version = dataset.protocol_version # Collect all files in private dir (data, metadata, permissions) files = {} @@ -1355,13 +1375,16 @@ def _upload_private_dataset_to_collection(self, dataset) -> str | None: # Create private collection folder (no sharing) folder_id = ( self._connection_router.owner_create_private_dataset_collection_folder( - tag=collection_tag, content_hash=content_hash, owner_email=self.email + tag=collection_tag, + content_hash=content_hash, + owner_email=self.email, + protocol_version=protocol_version, ) ) # Upload files self._connection_router.owner_upload_private_dataset_files( - collection_tag, content_hash, files + collection_tag, content_hash, files, protocol_version ) return folder_id @@ -1405,10 +1428,6 @@ def share_dataset(self, tag: str, users: list[str] | str, sync=True): users: List of email addresses or "any" sync: Whether to sync after sharing """ - from syft_client.sync.connections.drive.gdrive_transport import ( - DatasetCollectionFolder, - ) - if self.dataset_manager is None: raise ValueError("Dataset manager is not set") @@ -1420,36 +1439,40 @@ def share_dataset(self, tag: str, users: list[str] | str, sync=True): if dataset is None: raise ValueError(f"Dataset {tag} not found") - # Compute current content hash from local files - files = {} - for mock_file in dataset.mock_files: - if mock_file.exists(): - files[mock_file.name] = mock_file.read_bytes() - metadata_path = dataset.mock_dir / "dataset.yaml" - if metadata_path.exists(): - files["dataset.yaml"] = metadata_path.read_bytes() - if dataset.readme_path and dataset.readme_path.exists(): - files[dataset.readme_path.name] = dataset.readme_path.read_bytes() + # A dataset has one collection for each protocol version it was written + # in. Share them all, so a peer of any supported version finds a copy. + # The listing gives the hash of each copy, so no hash is recomputed here. + collections = [ + c + for c in self._connection_router.owner_list_all_dataset_collections_with_permissions() + if c.tag == tag + ] + if not collections: + raise ValueError(f"No uploaded collection found for dataset {tag}") - content_hash = DatasetCollectionFolder.compute_hash(files) + if users != "any" and isinstance(users, str): + users = [users] - # Share collection - if users == "any": - self._connection_router.owner_tag_dataset_collection_as_any( - tag, content_hash - ) - self.datasite_owner_syncer._any_shared_datasets.append((tag, content_hash)) - peer_emails = [p.email for p in self.peer_manager.approved_peers] - if peer_emails: + for collection in collections: + if users == "any": + self._connection_router.owner_tag_dataset_collection_as_any( + tag, collection.content_hash, collection.protocol_version + ) + self.datasite_owner_syncer._any_shared_datasets.append( + (tag, collection.content_hash, collection.protocol_version) + ) + peer_emails = [p.email for p in self.peer_manager.approved_peers] + if peer_emails: + self._connection_router.owner_share_dataset_collection( + tag, + collection.content_hash, + peer_emails, + collection.protocol_version, + ) + else: self._connection_router.owner_share_dataset_collection( - tag, content_hash, peer_emails + tag, collection.content_hash, users, collection.protocol_version ) - else: - if isinstance(users, str): - users = [users] - self._connection_router.owner_share_dataset_collection( - tag, content_hash, users - ) if sync: self.sync() diff --git a/syft_client/sync/sync/caches/datasite_owner_cache.py b/syft_client/sync/sync/caches/datasite_owner_cache.py index eb54cd55962..5a8b59b9ded 100644 --- a/syft_client/sync/sync/caches/datasite_owner_cache.py +++ b/syft_client/sync/sync/caches/datasite_owner_cache.py @@ -58,7 +58,8 @@ class DataSiteOwnerEventCache(BaseModelCallbackMixin): email: str # Full path to collections (datasets) folder collections_folder: Path | None = None - # Cache of collection hashes: "tag" -> content_hash + # Cache of collection hashes, keyed by tag and protocol version. See + # `_collection_hash_key`: "tag" for protocol 0, "v/tag" for protocol n. collection_hashes: Dict[str, str] = {} @model_validator(mode="before") @@ -142,24 +143,50 @@ def _load_file_hashes_from_disk(self) -> float | None: def _load_collection_hashes_from_disk(self): """Scan local dataset directories and compute hashes to populate collection_hashes.""" + from syft_datasets.config import is_protocol_dir_name + from syft_client.sync.file_utils import compute_directory_hash if self.collections_folder is None or not self.collections_folder.exists(): return - for tag_dir in self.collections_folder.iterdir(): - if tag_dir.is_dir(): + for entry in self.collections_folder.iterdir(): + if not entry.is_dir(): + continue + # A v directory holds the tags of one protocol version. + if is_protocol_dir_name(entry.name): + # Names here match ^v\d+$, so drop the leading 'v'. + protocol_version = entry.name[1:] + tag_dirs = [tag for tag in entry.iterdir() if tag.is_dir()] + else: + protocol_version = "0" + tag_dirs = [entry] + for tag_dir in tag_dirs: content_hash = compute_directory_hash(tag_dir) if content_hash: - self.collection_hashes[tag_dir.name] = content_hash + self.collection_hashes[ + self._collection_hash_key(tag_dir.name, protocol_version) + ] = content_hash + + @staticmethod + def _collection_hash_key(tag: str, protocol_version: str) -> str: + # A dataset has one collection for each protocol version, and each has + # its own contents. A key of the tag alone would give them one entry. + return tag if protocol_version == "0" else f"v{protocol_version}/{tag}" - def get_collection_hash(self, tag: str) -> str | None: + def get_collection_hash(self, tag: str, protocol_version: str = "0") -> str | None: """Get the cached hash for a collection.""" - return self.collection_hashes.get(tag) + return self.collection_hashes.get( + self._collection_hash_key(tag, protocol_version) + ) - def set_collection_hash(self, tag: str, content_hash: str): + def set_collection_hash( + self, tag: str, content_hash: str, protocol_version: str = "0" + ): """Set the cached hash for a collection.""" - self.collection_hashes[tag] = content_hash + self.collection_hashes[self._collection_hash_key(tag, protocol_version)] = ( + content_hash + ) @property def latest_cached_timestamp(self) -> float | None: diff --git a/syft_client/sync/sync/caches/datasite_watcher_cache.py b/syft_client/sync/sync/caches/datasite_watcher_cache.py index 5263b0fcf36..ca2778da3d1 100644 --- a/syft_client/sync/sync/caches/datasite_watcher_cache.py +++ b/syft_client/sync/sync/caches/datasite_watcher_cache.py @@ -1,23 +1,39 @@ +import logging from concurrent.futures import ThreadPoolExecutor -from typing import Callable, Dict, List -from syft_client.sync.sync.caches.cache_file_writer_connection import FSFileConnection +from datetime import datetime, timedelta from pathlib import Path +from typing import Callable, Dict, List + from pydantic import BaseModel, Field -from datetime import datetime, timedelta + +from syft_client.sync.connections.base_connection import ConnectionConfig +from syft_client.sync.connections.connection_router import ConnectionRouter from syft_client.sync.events.file_change_event import ( FileChangeEvent, FileChangeEventsMessage, ) -from syft_client.sync.connections.connection_router import ConnectionRouter -from syft_client.sync.connections.base_connection import ConnectionConfig from syft_client.sync.sync.caches.cache_file_writer_connection import ( CacheFileConnection, + FSFileConnection, InMemoryCacheFileConnection, ) +logger = logging.getLogger(__name__) + SECONDS_BEFORE_SYNCING_DOWN = 0 +def _readable_dataset_protocol_versions() -> set[str]: + """The dataset protocol versions that this client has a layout for.""" + from syft_datasets.protocolcodecs import CODECS + + return { + protocol_version + for codec_cls in CODECS + for protocol_version in codec_cls.dataset_config_cls.protocol_versions + } + + class DataSiteWatcherCacheConfig(BaseModel): email: str = "" use_in_memory_cache: bool = True @@ -141,14 +157,34 @@ def get_collection_owner_email(self, collection_path: Path) -> str: """Extract the owner email from a collection path.""" return collection_path.relative_to(self.syftbox_folder).parts[0] - def get_collection_path(self, owner_email: str, tag: str) -> Path | None: - """Get the full path to a collection for a given owner and tag.""" + def _collection_rel_dir( + self, owner_email: str, tag: str, protocol_version: str = "0" + ) -> Path: + """The local directory of a collection, relative to the SyftBox folder. + + Protocol 0 is flat. A later protocol adds its v segment, so the files + land where the metadata of that copy points. + """ + from syft_datasets.config import protocol_dir_name + + base = Path(owner_email) / self.collection_subpath + segment = protocol_dir_name(protocol_version) + return base / segment / tag if segment else base / tag + + def get_collection_path( + self, owner_email: str, tag: str, protocol_version: str = "0" + ) -> Path | None: + """Get the full path to a collection for a given owner, tag and protocol.""" if self.syftbox_folder is None or self.collection_subpath is None: return None - return self.syftbox_folder / owner_email / self.collection_subpath / tag + return self.syftbox_folder / self._collection_rel_dir( + owner_email, tag, protocol_version + ) def _get_local_dataset_folders(self): - """Yield paths to all local dataset folders.""" + """Yield paths to all local dataset folders, in every protocol layout.""" + from syft_datasets.config import is_protocol_dir_name + if self.syftbox_folder is None or not self.syftbox_folder.exists(): return if self.collection_subpath is None: @@ -160,9 +196,14 @@ def _get_local_dataset_folders(self): datasets_dir = email_dir / self.collection_subpath if not datasets_dir.exists(): continue - for tag_dir in datasets_dir.iterdir(): - if tag_dir.is_dir(): - yield tag_dir + for entry in datasets_dir.iterdir(): + if not entry.is_dir(): + continue + # A v directory holds the tags of one protocol version. + if is_protocol_dir_name(entry.name): + yield from (tag for tag in entry.iterdir() if tag.is_dir()) + else: + yield entry def _compute_local_dataset_hash(self, collection_path: Path) -> str | None: """Compute content hash from local dataset files on disk.""" @@ -280,17 +321,69 @@ def current_hash_for_file(self, path: str) -> int | None: self.sync_down_if_needed(peer) return self.file_hashes.get(path, None) + def _select_collections_to_sync(self, collections: list[dict]) -> list[dict]: + """Keep one collection for each dataset: the newest layout we can read. + + An owner publishes a dataset once for each protocol version that its + audience reads. This client takes the newest of those that it reads, and + ignores the rest. + """ + readable = _readable_dataset_protocol_versions() + best: dict[tuple[str, str], dict] = {} + for collection in collections: + protocol_version = collection.get("protocol_version", "0") + if protocol_version not in readable: + logger.warning( + "Skipping dataset '%s' from %s: it uses dataset protocol %s, " + "which this client does not read.", + collection["tag"], + collection["owner_email"], + protocol_version, + ) + continue + key = (collection["owner_email"], collection["tag"]) + current = best.get(key) + if current is None or int(protocol_version) > int( + current.get("protocol_version", "0") + ): + best[key] = collection + return list(best.values()) + def _cleanup_stale_dataset_collections( - self, peer_email: str, remote_collections: list[dict] + self, + peer_email: str, + selected_collections: list[dict], + remote_collections: list[dict], ): - """Remove locally cached dataset collections that no longer exist remotely.""" - remote_tags = {c["tag"] for c in remote_collections} + """Remove local collections that this client no longer syncs from a peer. + + Two cases get removed: the owner deleted the dataset, and this client now + reads a newer layout of it. The second case would otherwise leave the + older copy on disk, where a dataset scan finds the same dataset twice. + + A dataset that the owner still publishes, but in no layout this client + reads, is kept. The copy on disk is then the last one this client could + read, and a delete would take it away over an upgrade by someone else. + ``_select_collections_to_sync`` already logged why it is not refreshed. + """ + selected_paths = { + self.get_collection_path( + c["owner_email"], c["tag"], c.get("protocol_version", "0") + ) + for c in selected_collections + } + published = {(c["owner_email"], c["tag"]) for c in remote_collections} + readable = {(c["owner_email"], c["tag"]) for c in selected_collections} for local_collection_path in list(self.dataset_collection_hashes.keys()): owner_email = self.get_collection_owner_email(local_collection_path) if owner_email != peer_email: continue - if local_collection_path.name in remote_tags: + if local_collection_path in selected_paths: + continue + # The last path segment is the tag, in a flat and a v layout both. + dataset = (owner_email, local_collection_path.name) + if dataset in published and dataset not in readable: continue del self.dataset_collection_hashes[local_collection_path] if self.syftbox_folder is not None: @@ -308,18 +401,22 @@ def sync_down_datasets(self, peer_email: str): # Get list of collections shared with us (now returns list of dicts) collections = self.connection_router.watcher_list_dataset_collections() - # Filter by peer - peer_collections = [c for c in collections if c["owner_email"] == peer_email] + # Filter by peer, then take one layout for each dataset + published = [c for c in collections if c["owner_email"] == peer_email] + peer_collections = self._select_collections_to_sync(published) - self._cleanup_stale_dataset_collections(peer_email, peer_collections) + self._cleanup_stale_dataset_collections(peer_email, peer_collections, published) for collection in peer_collections: owner_email = collection["owner_email"] tag = collection["tag"] content_hash = collection["content_hash"] + protocol_version = collection.get("protocol_version", "0") # Check if hash changed - skip download if unchanged - collection_path = self.get_collection_path(owner_email, tag) + collection_path = self.get_collection_path( + owner_email, tag, protocol_version + ) if collection_path is None: continue cached_hash = self.dataset_collection_hashes.get(collection_path) @@ -328,13 +425,13 @@ def sync_down_datasets(self, peer_email: str): # Download collection files files = self.connection_router.watcher_download_dataset_collection( - tag, content_hash, owner_email + tag, content_hash, owner_email, protocol_version ) # Write files to local cache (path relative to syftbox_folder) + rel_dir = self._collection_rel_dir(owner_email, tag, protocol_version) for file_name, content in files.items(): - rel_path = f"{owner_email}/{self.collection_subpath}/{tag}/{file_name}" - self.file_connection.write_file(rel_path, content) + self.file_connection.write_file(str(rel_dir / file_name), content) # Update hash cache self.dataset_collection_hashes[collection_path] = content_hash @@ -350,9 +447,10 @@ def sync_down_datasets_parallel( Downloads all files from all collections in a single parallel batch. """ collections = self.connection_router.watcher_list_dataset_collections() - peer_collections = [c for c in collections if c["owner_email"] == peer_email] + published = [c for c in collections if c["owner_email"] == peer_email] + peer_collections = self._select_collections_to_sync(published) - self._cleanup_stale_dataset_collections(peer_email, peer_collections) + self._cleanup_stale_dataset_collections(peer_email, peer_collections, published) # Gather all files to download across all collections all_downloads = [] # List of (collection_info, file_metadata) @@ -362,9 +460,12 @@ def sync_down_datasets_parallel( owner_email = collection["owner_email"] tag = collection["tag"] content_hash = collection["content_hash"] + protocol_version = collection.get("protocol_version", "0") # Check if hash changed - skip download if unchanged - collection_path = self.get_collection_path(owner_email, tag) + collection_path = self.get_collection_path( + owner_email, tag, protocol_version + ) if collection_path is None: continue cached_hash = self.dataset_collection_hashes.get(collection_path) @@ -374,7 +475,7 @@ def sync_down_datasets_parallel( # Get file metadata (no download yet) file_metadatas = ( self.connection_router.watcher_get_dataset_collection_file_metadatas( - tag, content_hash, owner_email + tag, content_hash, owner_email, protocol_version ) ) @@ -394,16 +495,21 @@ def sync_down_datasets_parallel( # Write files to local cache (path relative to syftbox_folder) for (collection, metadata), content in zip(all_downloads, downloaded_contents): - owner_email = collection["owner_email"] - tag = collection["tag"] - file_name = metadata["file_name"] - rel_path = f"{owner_email}/{self.collection_subpath}/{tag}/{file_name}" - self.file_connection.write_file(rel_path, content) + rel_dir = self._collection_rel_dir( + collection["owner_email"], + collection["tag"], + collection.get("protocol_version", "0"), + ) + self.file_connection.write_file( + str(rel_dir / metadata["file_name"]), content + ) # Update hash cache for all collections for collection in collections_to_update: collection_path = self.get_collection_path( - collection["owner_email"], collection["tag"] + collection["owner_email"], + collection["tag"], + collection.get("protocol_version", "0"), ) if collection_path is not None: self.dataset_collection_hashes[collection_path] = collection[ diff --git a/syft_client/sync/sync/datasite_owner_syncer.py b/syft_client/sync/sync/datasite_owner_syncer.py index 538af2f79c5..3af1d495329 100644 --- a/syft_client/sync/sync/datasite_owner_syncer.py +++ b/syft_client/sync/sync/datasite_owner_syncer.py @@ -87,7 +87,8 @@ class DatasiteOwnerSyncer(BaseModelCallbackMixin): _executor: ThreadPoolExecutor = PrivateAttr( default_factory=lambda: ThreadPoolExecutor(max_workers=10) ) - # Cache of datasets shared with "any" - list of (tag, content_hash) tuples + # Datasets shared with "any": (tag, content_hash, protocol_version) tuples. + # One entry for each protocol copy, because each has its own collection. _any_shared_datasets: List[tuple] = PrivateAttr(default_factory=list) # Cache of read permissions per file path → frozenset of peer emails _read_perm_cache: dict[str, frozenset[str]] = PrivateAttr(default_factory=dict) @@ -343,10 +344,22 @@ def _update_any_shared_datasets_cache(self, collections: list[FileCollection]): """Populate _any_shared_datasets cache from collections with 'any' permission.""" for collection in collections: if collection.has_any_permission: - entry = (collection.tag, collection.content_hash) + entry = ( + collection.tag, + collection.content_hash, + collection.protocol_version, + ) if entry not in self._any_shared_datasets: self._any_shared_datasets.append(entry) + def _collection_local_dir(self, collection: FileCollection) -> Path: + """The local directory that holds one protocol copy of a collection.""" + from syft_datasets.config import protocol_dir_name + + segment = protocol_dir_name(collection.protocol_version) + base = self.collections_folder + return base / segment / collection.tag if segment else base / collection.tag + def _filter_collections_needing_download( self, collections: list[FileCollection] ) -> list[FileCollection]: @@ -356,14 +369,19 @@ def _filter_collections_needing_download( result = [] for collection in collections: # Use cached hash from event_cache first - cached_hash = self.event_cache.get_collection_hash(collection.tag) + cached_hash = self.event_cache.get_collection_hash( + collection.tag, collection.protocol_version + ) if cached_hash is None and self.collections_folder is not None: # Fallback: compute hash from local filesystem (for locally created datasets) - local_dataset_dir = self.collections_folder / collection.tag - cached_hash = compute_directory_hash(local_dataset_dir) + cached_hash = compute_directory_hash( + self._collection_local_dir(collection) + ) # Update cache if we computed a hash if cached_hash is not None: - self.event_cache.set_collection_hash(collection.tag, cached_hash) + self.event_cache.set_collection_hash( + collection.tag, cached_hash, collection.protocol_version + ) if cached_hash != collection.content_hash: result.append(collection) @@ -399,14 +417,14 @@ def _download_dataset_collections_parallel(self, collections: list[FileCollectio # Write all files to disk for (collection, metadata), content in zip(all_downloads, downloaded_contents): - local_dataset_dir = self.collections_folder / collection.tag + local_dataset_dir = self._collection_local_dir(collection) local_dataset_dir.mkdir(parents=True, exist_ok=True) (local_dataset_dir / metadata["file_name"]).write_bytes(content) # Update cached hashes for downloaded collections for collection in collections: self.event_cache.set_collection_hash( - collection.tag, collection.content_hash + collection.tag, collection.content_hash, collection.protocol_version ) def _get_file_metadatas_with_new_connection( @@ -418,6 +436,7 @@ def _get_file_metadatas_with_new_connection( tag=collection.tag, content_hash=collection.content_hash, owner_email=self.email, + protocol_version=collection.protocol_version, ) def _download_file_with_new_connection(self, file_id: str) -> bytes: @@ -444,8 +463,13 @@ def _pull_private_datasets_for_initial_sync(self): ) self._download_private_collections_parallel(collections_to_download) - def _private_dataset_local_dir(self, tag: str) -> Path: - return self.syftbox_folder / self.email / "private" / "syft_datasets" / tag + def _private_dataset_local_dir(self, tag: str, protocol_version: str = "0") -> Path: + """The private directory of one protocol copy of a dataset.""" + from syft_datasets.config import protocol_dir_name + + base = self.syftbox_folder / self.email / "private" / "syft_datasets" + segment = protocol_dir_name(protocol_version) + return base / segment / tag if segment else base / tag def _filter_private_collections_needing_download( self, collections: list[FileCollection] @@ -453,7 +477,9 @@ def _filter_private_collections_needing_download( """Return private collections that don't exist locally yet.""" result = [] for collection in collections: - local_dir = self._private_dataset_local_dir(collection.tag) + local_dir = self._private_dataset_local_dir( + collection.tag, collection.protocol_version + ) if not local_dir.exists() or not any(local_dir.iterdir()): result.append(collection) return result @@ -484,13 +510,17 @@ def _download_private_collections_parallel(self, collections: list[FileCollectio ) for (collection, metadata), content in zip(all_downloads, downloaded_contents): - local_dir = self._private_dataset_local_dir(collection.tag) + local_dir = self._private_dataset_local_dir( + collection.tag, collection.protocol_version + ) local_dir.mkdir(parents=True, exist_ok=True) (local_dir / metadata["file_name"]).write_bytes(content) # Fix data_dir in private_metadata.yaml to point to current local path for collection in collections: - self._fix_private_metadata_data_dir(collection.tag) + self._fix_private_metadata_data_dir( + collection.tag, collection.protocol_version + ) def _get_private_file_metadatas_with_new_connection( self, collection: FileCollection @@ -501,11 +531,14 @@ def _get_private_file_metadatas_with_new_connection( tag=collection.tag, content_hash=collection.content_hash, owner_email=self.email, + protocol_version=collection.protocol_version, ) - def _fix_private_metadata_data_dir(self, dataset_tag: str): + def _fix_private_metadata_data_dir( + self, dataset_tag: str, protocol_version: str = "0" + ): """Update data_dir in private_metadata.yaml to match the current syftbox path.""" - local_dir = self._private_dataset_local_dir(dataset_tag) + local_dir = self._private_dataset_local_dir(dataset_tag, protocol_version) metadata_path = local_dir / "private_metadata.yaml" if not metadata_path.exists(): return diff --git a/tests/migrations/p2p/test_dataset_multicopy_delivery.py b/tests/migrations/p2p/test_dataset_multicopy_delivery.py new file mode 100644 index 00000000000..3a97951f1e5 --- /dev/null +++ b/tests/migrations/p2p/test_dataset_multicopy_delivery.py @@ -0,0 +1,388 @@ +"""A dataset reaches peers of different protocol versions, and each one reads it. + +A dataset goes to its whole audience through the dataset-collection transport. +Before multi-copy, that transport held one collection for each dataset name, and +it wrote every file flat. A dataset written in the v1 layout therefore arrived +with metadata that pointed at a directory the peer never got. + +The transport now holds one collection for each protocol version. The name of the +collection gives the version, and the peer takes the newest layout that it reads. +These tests drive that path from the name of the folder to the file on disk. +""" + +from pathlib import Path + +import pytest +from syft_client.sync.connections.drive.gdrive_transport import ( + DATASET_COLLECTION_NAME_QUERY, + DatasetCollectionFolder, +) +from syft_client.sync.syftbox_manager import COLLECTION_SUBPATH, SyftboxManager +from syft_datasets.dataset_manager import DATASET_COLLECTION_PREFIX +from syft_migration import ProtocolSchema + +from tests.unit.utils import create_tmp_dataset_files + + +# An audience member on an earlier client, so a create writes both layouts. +OLD_PEER = "old@test.org" + + +def _dataset_schema(protocol_version: str) -> ProtocolSchema: + # The slim form a peer advertises in its VersionInfo. + return ProtocolSchema( + protocol_name="syft-dataset", + version=protocol_version, + supported_versions={"Dataset": ["1"]}, + ) + + +@pytest.fixture +def pair(): + return SyftboxManager.pair_with_mock_drive_service_connection( + use_in_memory_cache=False, + ) + + +# -- folder names ---------------------------------------------------------- + + +def test_a_collection_name_carries_the_protocol_version(): + folder = DatasetCollectionFolder( + tag="mytag", content_hash="abc123", protocol_version="1" + ) + assert DatasetCollectionFolder.from_name(folder.as_string()) == folder + + +def test_a_tag_with_an_underscore_still_round_trips(): + folder = DatasetCollectionFolder( + tag="my_tag_here", content_hash="abc123", protocol_version="2" + ) + parsed = DatasetCollectionFolder.from_name(folder.as_string()) + assert parsed.tag == "my_tag_here" + assert parsed.content_hash == "abc123" + assert parsed.protocol_version == "2" + + +def test_a_protocol_0_name_is_what_earlier_clients_write(): + # Byte-identical to the name used before multi-copy, so a client that + # predates this change still finds the copy that it can read. + folder = DatasetCollectionFolder(tag="mytag", content_hash="abc123") + assert folder.as_string() == f"{DATASET_COLLECTION_PREFIX}_mytag_abc123" + + +def test_a_name_with_no_version_reads_as_protocol_0(): + parsed = DatasetCollectionFolder.from_name( + f"{DATASET_COLLECTION_PREFIX}_mytag_abc123" + ) + assert parsed.protocol_version == "0" + + +def test_an_earlier_client_does_not_see_a_versioned_collection(): + # An earlier client searches Drive for names that contain '_'. The + # version infix breaks that match, so it never lists a layout it cannot + # read. It still lists the protocol-0 copy. + versioned = DatasetCollectionFolder( + tag="mytag", content_hash="abc123", protocol_version="1" + ).as_string() + flat = DatasetCollectionFolder(tag="mytag", content_hash="abc123").as_string() + + assert f"{DATASET_COLLECTION_PREFIX}_" not in versioned + assert f"{DATASET_COLLECTION_PREFIX}_" in flat + # This client searches without the trailing '_', so it sees both. + assert DATASET_COLLECTION_PREFIX in DATASET_COLLECTION_NAME_QUERY + assert f"{DATASET_COLLECTION_PREFIX}_" not in DATASET_COLLECTION_NAME_QUERY + + +def test_a_damaged_name_raises(): + with pytest.raises(ValueError): + DatasetCollectionFolder.from_name("not_a_collection") + + +# -- local layout ---------------------------------------------------------- + + +def test_the_local_directory_of_a_collection_follows_its_protocol(pair): + # The peer writes the files where the metadata of that copy points. Protocol + # 0 is flat; a later protocol adds its v segment. + ds_manager, _ = pair + cache = ds_manager.datasite_watcher_syncer.datasite_watcher_cache + + assert ( + cache._collection_rel_dir("do@test.org", "d", "0") + == Path("do@test.org") / COLLECTION_SUBPATH / "d" + ) + assert ( + cache._collection_rel_dir("do@test.org", "d", "1") + == Path("do@test.org") / COLLECTION_SUBPATH / "v1" / "d" + ) + + +# -- delivery -------------------------------------------------------------- + + +def test_a_dataset_for_a_protocol0_peer_arrives_flat_and_reads(pair): + ds_manager, do_manager = pair + # The DS advertises dataset protocol 0, as an earlier client does. + do_manager.peer_manager.live_peer_schemas("syft-dataset")[ds_manager.email] = ( + _dataset_schema("0") + ) + + mock_path, private_path, readme_path = create_tmp_dataset_files() + do_manager.create_dataset( + name="skew dataset", + mock_path=mock_path, + private_path=private_path, + readme_path=readme_path, + users=[ds_manager.email], + ) + ds_manager.sync() + + dataset = ds_manager.datasets.get("skew dataset", datasite=do_manager.email) + # The owner wrote the layout that this peer reads, not its own newest. + assert dataset.protocol_version == "0" + assert ( + dataset.mock_dir + == ds_manager.syftbox_folder + / do_manager.email + / COLLECTION_SUBPATH + / "skew dataset" + ) + assert dataset.mock_files + for path in dataset.mock_files: + assert path.exists(), ( + f"the metadata points to a file the peer does not get: {path}" + ) + + +def test_a_mixed_audience_gets_one_collection_for_each_protocol(pair): + _, do_manager = pair + mock_path, private_path, readme_path = create_tmp_dataset_files() + + # Write both layouts, as an audience of one protocol-0 peer and one + # current peer produces. + created = do_manager.dataset_manager.create_all( + name="mixed dataset", + mock_path=mock_path, + private_path=private_path, + readme_path=readme_path, + protocol_versions=["0", "1"], + ) + assert set(created) == {"0", "1"} + for copy in created.values(): + do_manager._upload_dataset_to_collection(copy, users=[]) + + collections = [ + c + for c in do_manager._connection_router.owner_list_all_dataset_collections_with_permissions() + if c.tag == "mixed dataset" + ] + assert {c.protocol_version for c in collections} == {"0", "1"} + # Each copy has its own folder, so neither overwrites the other. + assert len({c.folder_id for c in collections}) == 2 + + +def test_the_owner_listing_names_each_dataset_once(pair): + # A dataset with two protocol copies has two collections. The listing names + # datasets, so the tag must not repeat. + _, do_manager = pair + mock_path, private_path, readme_path = create_tmp_dataset_files() + + created = do_manager.dataset_manager.create_all( + name="mixed dataset", + mock_path=mock_path, + private_path=private_path, + readme_path=readme_path, + protocol_versions=["0", "1"], + ) + for copy in created.values(): + do_manager._upload_dataset_to_collection(copy, users=[]) + + tags = do_manager._connection_router.owner_list_dataset_collections() + assert tags.count("mixed dataset") == 1 + + +def _create_for_a_mixed_audience(ds_manager, do_manager, name: str, **kwargs): + """Create a dataset for an audience of one protocol-0 peer and one current peer.""" + do_manager.peer_manager.live_peer_schemas("syft-dataset")[OLD_PEER] = ( + _dataset_schema("0") + ) + mock_path, private_path, readme_path = create_tmp_dataset_files() + return do_manager.create_dataset( + name=name, + mock_path=mock_path, + private_path=private_path, + readme_path=readme_path, + users=[ds_manager.email, OLD_PEER], + **kwargs, + ) + + +def test_a_mixed_audience_through_create_dataset_writes_both_layouts(pair): + ds_manager, do_manager = pair + _create_for_a_mixed_audience(ds_manager, do_manager, "mixed") + + public = [ + c + for c in do_manager._connection_router.owner_list_all_dataset_collections_with_permissions() + if c.tag == "mixed" + ] + assert {c.protocol_version for c in public} == {"0", "1"} + + +def test_every_copy_uploads_its_own_private_collection(pair): + # Each copy holds its own private directory. An upload of only the newest + # leaves the other copies local, and a cold start does not restore them. + ds_manager, do_manager = pair + _create_for_a_mixed_audience(ds_manager, do_manager, "mixed", upload_private=True) + + private = [ + c + for c in do_manager._connection_router.owner_list_private_dataset_collections() + if c.tag == "mixed" + ] + assert {c.protocol_version for c in private} == {"0", "1"} + + +def test_a_cold_start_restores_the_private_data_of_every_copy(pair): + import shutil + + ds_manager, do_manager = pair + _create_for_a_mixed_audience(ds_manager, do_manager, "mixed", upload_private=True) + + storage = do_manager.dataset_manager.storage + private_dirs = { + protocol_version: storage.private_dataset_dir( + storage.new_dataset_ref("mixed", protocol_version) + ) + for protocol_version in ("0", "1") + } + expected = {v: {f.name for f in d.iterdir()} for v, d in private_dirs.items()} + assert all(expected.values()), "each copy should have private files to lose" + + # Lose the local private data of every copy, then sync from cold. + for directory in private_dirs.values(): + shutil.rmtree(directory) + do_manager.datasite_owner_syncer.initial_sync_done = False + do_manager.sync() + + for protocol_version, directory in private_dirs.items(): + assert directory.exists(), ( + f"the private data of protocol {protocol_version} was not restored" + ) + assert {f.name for f in directory.iterdir()} == expected[protocol_version] + + +def test_a_collection_of_an_unreadable_protocol_is_skipped(pair, caplog): + import logging + + ds_manager, _ = pair + cache = ds_manager.datasite_watcher_syncer.datasite_watcher_cache + + remote = [ + { + "owner_email": "do@test.org", + "tag": "future dataset", + "content_hash": "abc123", + "protocol_version": "99", + } + ] + with caplog.at_level(logging.WARNING): + assert cache._select_collections_to_sync(remote) == [] + assert "future dataset" in caplog.text + assert "99" in caplog.text + + +def test_the_newest_readable_layout_wins(pair): + ds_manager, _ = pair + cache = ds_manager.datasite_watcher_syncer.datasite_watcher_cache + + remote = [ + { + "owner_email": "do@test.org", + "tag": "both", + "content_hash": "flat", + "protocol_version": "0", + }, + { + "owner_email": "do@test.org", + "tag": "both", + "content_hash": "versioned", + "protocol_version": "1", + }, + ] + selected = cache._select_collections_to_sync(remote) + assert [c["protocol_version"] for c in selected] == ["1"] + + +# -- cleanup of local copies ----------------------------------------------- + + +def _seed_local_copy(cache, peer, tag, protocol_version): + path = cache.get_collection_path(peer, tag, protocol_version) + cache.dataset_collection_hashes[path] = f"hash{protocol_version}" + return path + + +def test_an_unreadable_remote_layout_keeps_the_local_copy(pair): + """The owner upgraded past us, so keep the last copy we could read. + + A delete here would take a dataset away over an upgrade by someone else, + and we cannot replace it until this client can read the newer layout. + """ + ds_manager, do_manager = pair + cache = ds_manager.datasite_watcher_syncer.datasite_watcher_cache + peer = do_manager.email + local = _seed_local_copy(cache, peer, "shared data", "0") + + published = [ + { + "owner_email": peer, + "tag": "shared data", + "content_hash": "hash99", + "protocol_version": "99", + } + ] + selected = cache._select_collections_to_sync(published) + assert selected == [] + + cache._cleanup_stale_dataset_collections(peer, selected, published) + assert local in cache.dataset_collection_hashes + + +def test_a_deleted_dataset_removes_the_local_copy(pair): + ds_manager, do_manager = pair + cache = ds_manager.datasite_watcher_syncer.datasite_watcher_cache + peer = do_manager.email + local = _seed_local_copy(cache, peer, "gone", "0") + + cache._cleanup_stale_dataset_collections(peer, [], []) + assert local not in cache.dataset_collection_hashes + + +def test_a_newer_readable_layout_removes_the_older_local_copy(pair): + """Otherwise a dataset scan finds the same dataset twice.""" + ds_manager, do_manager = pair + cache = ds_manager.datasite_watcher_syncer.datasite_watcher_cache + peer = do_manager.email + old_local = _seed_local_copy(cache, peer, "both", "0") + + published = [ + { + "owner_email": peer, + "tag": "both", + "content_hash": "hash0", + "protocol_version": "0", + }, + { + "owner_email": peer, + "tag": "both", + "content_hash": "hash1", + "protocol_version": "1", + }, + ] + selected = cache._select_collections_to_sync(published) + assert [c["protocol_version"] for c in selected] == ["1"] + + cache._cleanup_stale_dataset_collections(peer, selected, published) + assert old_local not in cache.dataset_collection_hashes diff --git a/tests/unit/test_create_dataset_cleanup.py b/tests/unit/test_create_dataset_cleanup.py index e1f89401e8e..23d6e1b2c69 100644 --- a/tests/unit/test_create_dataset_cleanup.py +++ b/tests/unit/test_create_dataset_cleanup.py @@ -30,13 +30,13 @@ def _dataset_kwargs(self, users=None): ) def test_no_cleanup_when_local_create_fails(self): - """If dataset_manager.create raises, nothing was created so nothing to clean.""" + """If create_all raises, nothing was created so nothing to clean.""" do_manager = self._make_do_manager() with ( patch.object( do_manager.dataset_manager, - "create", + "create_all", side_effect=ValueError("bad input"), ), patch.object( @@ -47,7 +47,7 @@ def test_no_cleanup_when_local_create_fails(self): do_manager.create_dataset(**self._dataset_kwargs()) # Cleanup called with nothing to clean - mock_cleanup.assert_called_once_with(None, False, None, None) + mock_cleanup.assert_called_once_with(None, False, [], []) def test_cleanup_on_mock_upload_failure(self): """If mock upload fails, local dataset is cleaned up.""" diff --git a/tests/unit/test_dataset_upload_private.py b/tests/unit/test_dataset_upload_private.py index c877f9d824d..69b3de0dddd 100644 --- a/tests/unit/test_dataset_upload_private.py +++ b/tests/unit/test_dataset_upload_private.py @@ -1,6 +1,8 @@ -from syft_client.sync.connections.drive.gdrive_transport import GDriveConnection +from syft_client.sync.connections.drive.gdrive_transport import ( + PRIVATE_COLLECTION_NAME_QUERY, + GDriveConnection, +) from syft_client.sync.syftbox_manager import SyftboxManager -from syft_datasets.dataset_manager import PRIVATE_DATASET_COLLECTION_PREFIX from tests.unit.utils import create_tmp_dataset_files @@ -141,10 +143,7 @@ def test_ds_cannot_find_private_folders_via_gdrive_query(self): results = ( ds_connection.drive_service.files() .list( - q=( - f"name contains '{PRIVATE_DATASET_COLLECTION_PREFIX}_' " - f"and trashed=false" - ), + q=f"{PRIVATE_COLLECTION_NAME_QUERY} and trashed=false", fields="files(id,name)", ) .execute() @@ -156,10 +155,7 @@ def test_ds_cannot_find_private_folders_via_gdrive_query(self): do_results = ( do_connection.drive_service.files() .list( - q=( - f"name contains '{PRIVATE_DATASET_COLLECTION_PREFIX}_' " - f"and trashed=false" - ), + q=f"{PRIVATE_COLLECTION_NAME_QUERY} and trashed=false", fields="files(id,name)", ) .execute() diff --git a/tests/unit/test_delete_syftbox.py b/tests/unit/test_delete_syftbox.py index 5caf0ec227e..cdbf8156049 100644 --- a/tests/unit/test_delete_syftbox.py +++ b/tests/unit/test_delete_syftbox.py @@ -3,19 +3,8 @@ from pathlib import Path from unittest.mock import patch -from syft_client.sync.connections.drive.gdrive_transport import ( - GDRIVE_P2P_FOLDER_DATASITE_PREFIX, - SYFT_PEERS_FILE, - SYFT_VERSION_FILE, -) from syft_client.sync.login_utils import handle_potential_version_mismatches_on_login -from syft_client.sync.syftbox_manager import SyftboxManager from syft_client.sync.version.version_info import VersionInfo -from syft_datasets.dataset_manager import ( - DATASET_COLLECTION_PREFIX, - PRIVATE_DATASET_COLLECTION_PREFIX, -) -from tests.unit.utils import create_tmp_dataset_files EMAIL = "test@example.com" @@ -54,30 +43,56 @@ def test_delete_all( mock_delete_local.assert_called_once() mock_delete_remote.assert_called_once() - @patch("syft_client.sync.login_utils._delete_remote_unversioned_state") @patch("syft_client.sync.login_utils.delete_remote_syftbox") @patch("syft_client.sync.login_utils.delete_local_syftbox") @patch("syft_client.sync.login_utils._prompt_mismatch", return_value="1") @patch("syft_client.sync.login_utils._read_remote_version") @patch("syft_client.sync.login_utils.read_local_version") - def test_upgrade_deletes_local_only( + def test_continue_keeps_local_and_remote( self, mock_read_local, mock_read_remote, mock_prompt, mock_delete_local, mock_delete_remote, - mock_delete_unversioned, ): - """Mismatch + choice 1 (upgrade) → local deleted, unversioned state deleted, full remote preserved.""" + """Mismatch + choice 1 (continue) → no deletes; data is kept for repair.""" mock_read_local.return_value = _old_version_info() mock_read_remote.return_value = _old_version_info() handle_potential_version_mismatches_on_login(EMAIL, TOKEN_PATH) - mock_delete_local.assert_called_once() + mock_delete_local.assert_not_called() mock_delete_remote.assert_not_called() - mock_delete_unversioned.assert_called_once() + + @patch("syft_client.sync.login_utils.sys.exit") + @patch("syft_client.sync.login_utils.delete_remote_syftbox") + @patch("syft_client.sync.login_utils.delete_local_syftbox") + @patch("syft_client.sync.login_utils._prompt_mismatch", return_value="3") + @patch("syft_client.sync.login_utils._read_remote_version") + @patch("syft_client.sync.login_utils.read_local_version") + def test_quit_exits_without_delete( + self, + mock_read_local, + mock_read_remote, + mock_prompt, + mock_delete_local, + mock_delete_remote, + mock_exit, + ): + """Mismatch + choice 3 (quit) → exit, no deletes.""" + mock_read_local.return_value = _old_version_info() + mock_read_remote.return_value = _old_version_info() + mock_exit.side_effect = SystemExit(0) + + try: + handle_potential_version_mismatches_on_login(EMAIL, TOKEN_PATH) + except SystemExit: + pass + + mock_delete_local.assert_not_called() + mock_delete_remote.assert_not_called() + mock_exit.assert_called_once_with(0) @patch("syft_client.sync.login_utils._read_remote_version") @patch("syft_client.sync.login_utils.read_local_version") @@ -89,6 +104,28 @@ def test_no_mismatch_no_prompt(self, mock_read_local, mock_read_remote): handle_potential_version_mismatches_on_login(EMAIL, TOKEN_PATH) +class TestPromptWithoutATerminal: + """A notebook or a scheduled run has no terminal to answer the prompt.""" + + @patch("syft_client.sync.login_utils.sys.stdin") + def test_no_terminal_keeps_data_and_continues(self, mock_stdin): + # Choice 1 keeps every file and changes nothing, so it is safe to take + # without an answer. A prompt would stop the run instead. + from syft_client.sync.login_utils import _prompt_mismatch + + mock_stdin.isatty.return_value = False + with patch("builtins.input", side_effect=AssertionError("must not prompt")): + assert _prompt_mismatch(_old_version_info(), _old_version_info()) == "1" + + @patch("syft_client.sync.login_utils.sys.stdin") + def test_a_terminal_still_asks(self, mock_stdin): + from syft_client.sync.login_utils import _prompt_mismatch + + mock_stdin.isatty.return_value = True + with patch("builtins.input", return_value="2"): + assert _prompt_mismatch(_old_version_info(), _old_version_info()) == "2" + + def _query_files(connection, name_contains): """Query mock drive for files/folders whose name contains a substring.""" q = f"name contains '{name_contains}' and trashed=false" @@ -98,68 +135,6 @@ def _query_files(connection, name_contains): return results.get("files", []) -def test_delete_unversioned_state_removes_correct_folders(): - """delete_unversioned_state removes exactly the right artifacts from mock drive.""" - ds_manager, do_manager = SyftboxManager.pair_with_mock_drive_service_connection( - use_in_memory_cache=False, - sync_automatically=False, - encryption=True, - ) - - # Create dataset so collection folders exist - mock_path, private_path, readme_path = create_tmp_dataset_files() - do_manager.create_dataset( - name="my dataset", - mock_path=mock_path, - private_path=private_path, - summary="Test", - readme_path=readme_path, - users=[ds_manager.email], - upload_private=True, - ) - do_manager.sync() - - do_conn = do_manager.peer_manager.connection_router.connections[0] - do_email = do_manager.email - - # Assert artifacts exist before deletion - do_enc_bundles = f"syft_encryption_bundles#{do_email}" - assert len(_query_files(do_conn, do_enc_bundles)) > 0 - assert len(_query_files(do_conn, DATASET_COLLECTION_PREFIX)) > 0 - assert len(_query_files(do_conn, PRIVATE_DATASET_COLLECTION_PREFIX)) > 0 - assert len(_query_files(do_conn, SYFT_PEERS_FILE)) > 0 - assert len(_query_files(do_conn, SYFT_VERSION_FILE)) > 0 - - # Assert versioned folders exist - p2p_before = _query_files(do_conn, GDRIVE_P2P_FOLDER_DATASITE_PREFIX) - assert len(p2p_before) > 0 - - # Delete unversioned state - do_conn.delete_unversioned_state() - - # Assert unversioned artifacts are gone - assert len(_query_files(do_conn, do_enc_bundles)) == 0 - assert len(_query_files(do_conn, DATASET_COLLECTION_PREFIX)) == 0 - assert len(_query_files(do_conn, PRIVATE_DATASET_COLLECTION_PREFIX)) == 0 - # peers/version files: DO's are gone, DS's may still exist - do_peers = [ - f - for f in _query_files(do_conn, SYFT_PEERS_FILE) - if f["id"] == do_conn._get_peers_file_id() - ] - assert len(do_peers) == 0 - do_version = [ - f - for f in _query_files(do_conn, SYFT_VERSION_FILE) - if f["id"] == do_conn._get_version_file_id() - ] - assert len(do_version) == 0 - - # Assert versioned folders survive - p2p_after = _query_files(do_conn, GDRIVE_P2P_FOLDER_DATASITE_PREFIX) - assert len(p2p_after) == len(p2p_before) - - class TestDeleteSyftboxImport: def test_importable_from_top_level(self): from syft_client import ( diff --git a/tests/unit/test_encryption.py b/tests/unit/test_encryption.py index eaaa4547c94..34a4cea074a 100644 --- a/tests/unit/test_encryption.py +++ b/tests/unit/test_encryption.py @@ -555,6 +555,6 @@ def test_encrypted_dataset_collection_syncs(): c = do_collections[0] files = cr.watcher_download_dataset_collection( - c["tag"], c["content_hash"], do_manager.email + c["tag"], c["content_hash"], do_manager.email, c["protocol_version"] ) assert files, "DS could not download the dataset collection files" diff --git a/tests/unit/test_sync_manager.py b/tests/unit/test_sync_manager.py index c4a89f715a0..21e669108c9 100644 --- a/tests/unit/test_sync_manager.py +++ b/tests/unit/test_sync_manager.py @@ -1492,9 +1492,11 @@ def test_ds_dataset_cache_aware_sync(): # Get the original hash from the collection collections = ds_manager._connection_router.watcher_list_dataset_collections() remote_hash = None + remote_protocol = None for c in collections: if c["tag"] == "cached dataset": remote_hash = c["content_hash"] + remote_protocol = c["protocol_version"] break assert remote_hash is not None @@ -1531,8 +1533,11 @@ def test_ds_dataset_cache_aware_sync(): # Verify hash was loaded from disk on startup ds_cache = ds_manager2.datasite_watcher_syncer.datasite_watcher_cache - # Cache uses full path as key: syftbox_folder / owner_email / collection_subpath / tag - cache_key = ds_cache.get_collection_path(do_email, "cached dataset") + # The key is the full local path of the collection, which holds the v + # segment of the protocol version that this client selected. + cache_key = ds_cache.get_collection_path( + do_email, "cached dataset", remote_protocol + ) assert cache_key in ds_cache.dataset_collection_hashes, ( "Hash should be loaded from disk on startup" ) @@ -2083,11 +2088,14 @@ def test_dataset_delete_propagates_to_ds(): def test_dataset_delivery_layout_matches_published_metadata(): """Check that the metadata of a dataset points to the files that the peer gets. - Datasets go to a peer only through the dataset-collection transport. This - transport writes all the files of a dataset into COLLECTION_SUBPATH/. - If the owner writes a dataset in a newer v layout, the metadata points to - a directory that the peer does not get. The peer then finds no files. + A dataset goes to a peer as one collection for each protocol version that its + audience reads. The peer takes the newest layout that it reads, and writes the + files into the directory of that layout. If the layout of the files and the + layout in the metadata disagree, the peer finds no files. """ + from syft_datasets.config import protocol_dir_name + from syft_datasets.migrations.registry import DATASET_PROTOCOL_VERSION + from syft_client.sync.syftbox_manager import COLLECTION_SUBPATH ds_manager, do_manager = SyftboxManager.pair_with_mock_drive_service_connection( @@ -2103,21 +2111,23 @@ def test_dataset_delivery_layout_matches_published_metadata(): users=[ds_manager.email], ) - # The DO knows the dataset protocol version of the DS. The DO must still - # write the layout of the transport. This makes sure the test does not pass - # only because the peer is unknown. + # The DO knows the dataset protocol version of the DS. This makes sure the + # test does not pass only because the peer is unknown. assert ds_manager.email in do_manager.peer_manager.live_peer_schemas("syft-dataset") ds_manager.sync() dataset = ds_manager.datasets.get("layout dataset", datasite=do_manager.email) - assert ( - dataset.mock_dir - == ds_manager.syftbox_folder + # Both clients are current, so the peer reads the current layout. + assert dataset.protocol_version == DATASET_PROTOCOL_VERSION + expected_dir = ( + ds_manager.syftbox_folder / do_manager.email / COLLECTION_SUBPATH + / protocol_dir_name(DATASET_PROTOCOL_VERSION) / "layout dataset" ) + assert dataset.mock_dir == expected_dir assert dataset.mock_files for path in dataset.mock_files: assert path.exists(), ( diff --git a/tests/unit/test_version_mismatch_flow.py b/tests/unit/test_version_mismatch_flow.py index 75f2d9b8fa1..e56b1149d8b 100644 --- a/tests/unit/test_version_mismatch_flow.py +++ b/tests/unit/test_version_mismatch_flow.py @@ -1,4 +1,9 @@ -"""End-to-end test for version mismatch and backup flow with mock drive.""" +"""End-to-end test for a client minor upgrade that keeps local and remote data. + +Login no longer deletes SyftBox state on a major/minor mismatch. The default is +to continue; private Drive folders are adopted by rename, and P2P folders of the +earlier version are reused so a peer that has not upgraded still finds them. +""" from unittest.mock import patch @@ -11,7 +16,6 @@ MockDriveService, ) from syft_client.sync.syftbox_manager import SyftboxManager, SyftboxManagerConfig -from syft_client.sync.utils.syftbox_utils import delete_local_syftbox from syft_client.version import SYFT_CLIENT_VERSION from tests.unit.utils import create_test_project_folder, create_tmp_dataset_files @@ -45,18 +49,21 @@ def _get_backing_store(manager): return conn.drive_service._backing_store -def _reinitialize_manager(email, backing_store, has_do_role, has_ds_role): - """Create a new SyftboxManager connected to an existing mock backing store. +def _reinitialize_manager( + email, backing_store, has_do_role, has_ds_role, syftbox_folder, write_version=True +): + """Create a new SyftboxManager on the same local path and mock Drive store. - This mirrors what pair_with_mock_drive_service_connection does for a - single manager, reusing the same backing store so the new manager sees - the same GDrive state. + Reuses the local SyftBox directory so a continue-on-mismatch upgrade keeps + the data that login left in place. Reuses the backing store so GDrive state + matches the pre-upgrade client. """ config = SyftboxManagerConfig._base_config_for_testing( email=email, has_do_role=has_do_role, has_ds_role=has_ds_role, use_in_memory_cache=False, + syftbox_folder=syftbox_folder, ) manager = SyftboxManager.from_config(config) @@ -75,17 +82,13 @@ def _reinitialize_manager(email, backing_store, has_do_role, has_ds_role): manager.job_file_change_handler._handle_file_change, ) - manager.peer_manager.write_own_version() + if write_version: + manager.peer_manager.write_own_version() return manager -def _simulate_upgrade(manager, backing_store): - """Simulate handle_potential_version_mismatches_on_login with mocks. - - Patches only the I/O boundaries so that read_local_version reads from the - manager's real syftbox folder, _read_remote_version reads from the mock - drive, and delete operations target the correct local path / mock drive. - """ +def _simulate_continue_on_mismatch(manager, backing_store): + """Run the login mismatch handler with choice 1 (continue, keep data).""" email = manager.email syftbox_folder = manager.syftbox_folder @@ -95,12 +98,6 @@ def _simulate_upgrade(manager, backing_store): def read_remote(e, t): return mock_conn.read_own_version_file() - def do_delete_local(**kwargs): - delete_local_syftbox(email=email, local_syftbox_path=syftbox_folder) - - def do_delete_unversioned(e, t): - mock_conn.delete_unversioned_state() - with ( patch( "syft_client.sync.login_utils._resolve_token_path", @@ -120,22 +117,22 @@ def do_delete_unversioned(e, t): ), patch( "syft_client.sync.login_utils.delete_local_syftbox", - side_effect=do_delete_local, - ), + ) as mock_delete_local, patch( - "syft_client.sync.login_utils._delete_remote_unversioned_state", - side_effect=do_delete_unversioned, - ), + "syft_client.sync.login_utils.delete_remote_syftbox", + ) as mock_delete_remote, ): from syft_client.sync.login_utils import ( handle_potential_version_mismatches_on_login, ) handle_potential_version_mismatches_on_login(email) + mock_delete_local.assert_not_called() + mock_delete_remote.assert_not_called() -def test_version_mismatch_and_backup_flow(): - """Full flow: create state on v1 -> upgrade to v2 -> old state preserved, new version works.""" +def test_version_mismatch_continues_and_repairs(): + """Upgrade keeps peers, jobs, and data; private folders adopt; P2P reuses.""" # -- Step 1: Create DO/DS on current version -- ds_manager, do_manager = SyftboxManager.pair_with_mock_drive_service_connection( @@ -173,22 +170,26 @@ def test_version_mismatch_and_backup_flow(): assert do_manager.jobs[0].status == "done" - # -- Step 4: Assert only P2P folders with current version -- + # -- Step 4: Record P2P folders and personal folder id before upgrade -- do_conn = do_manager.peer_manager.connection_router.connections[0] do_p2p_current = _find_versioned_p2p_folders( do_conn, ds_manager.email, SYFT_CLIENT_VERSION ) assert len(do_p2p_current) > 0 - # No folders with a different version - all_do_p2p = _find_p2p_folders(do_conn, ds_manager.email) - assert len(all_do_p2p) == len(do_p2p_current) + old_personal_name = f"{SYFT_CLIENT_VERSION}#{do_manager.email}" + old_personal_id = do_conn._find_folder_by_name( + old_personal_name, + parent_id=do_conn.get_syftbox_folder_id(), + owner_email=do_manager.email, + ) + assert old_personal_id is not None # -- Step 5: Extract backing store -- backing_store = _get_backing_store(do_manager) do_email = do_manager.email ds_email = ds_manager.email - # -- Step 6+7: Upgrade DO -- + # -- Step 6+7: Upgrade DO (continue keeps data) -- with ( patch("syft_client.version.SYFT_CLIENT_VERSION", NEW_VERSION), patch( @@ -198,30 +199,53 @@ def test_version_mismatch_and_backup_flow(): patch("syft_client.sync.login_utils.SYFT_CLIENT_VERSION", NEW_VERSION), patch("syft_client.sync.version.version_info.SYFT_CLIENT_VERSION", NEW_VERSION), ): - _simulate_upgrade(do_manager, backing_store) + do_syftbox = do_manager.syftbox_folder + _simulate_continue_on_mismatch(do_manager, backing_store) do_manager = _reinitialize_manager( - do_email, backing_store, has_do_role=True, has_ds_role=False + do_email, + backing_store, + has_do_role=True, + has_ds_role=False, + syftbox_folder=do_syftbox, ) - # -- Step 8: Assert new versioned folders for DO -- + # Personal folder is adopted (same Drive id, new name), not recreated. do_conn_new = do_manager.peer_manager.connection_router.connections[0] - do_p2p_new = _find_versioned_p2p_folders(do_conn_new, ds_email, NEW_VERSION) - # New folders don't exist yet (no peers added), but personal folder does - personal_folder_name = f"{NEW_VERSION}#{do_email}" - personal_id = do_conn_new._find_folder_by_name( - personal_folder_name, + new_personal_name = f"{NEW_VERSION}#{do_email}" + new_personal_id = do_conn_new._find_folder_by_name( + new_personal_name, parent_id=do_conn_new.get_syftbox_folder_id(), owner_email=do_email, ) - assert personal_id is not None + assert new_personal_id is not None + assert new_personal_id == old_personal_id + assert ( + do_conn_new._find_folder_by_name( + old_personal_name, + parent_id=do_conn_new.get_syftbox_folder_id(), + owner_email=do_email, + ) + is None + ) + + # Peers survive: continue did not wipe SYFT_peers.json. + do_manager.load_peers() + assert any(p.email == ds_email for p in do_manager.peer_manager.approved_peers) + + # Pre-upgrade job is still present on the kept datasite. + assert any(job.name == "pre_upgrade.job" for job in do_manager.jobs) - # -- Step 9+10: Upgrade DS -- - _simulate_upgrade(ds_manager, backing_store) + # -- Step 8+9: Upgrade DS -- + ds_syftbox = ds_manager.syftbox_folder + _simulate_continue_on_mismatch(ds_manager, backing_store) ds_manager = _reinitialize_manager( - ds_email, backing_store, has_do_role=False, has_ds_role=True + ds_email, + backing_store, + has_do_role=False, + has_ds_role=True, + syftbox_folder=ds_syftbox, ) - # -- Step 11: Assert new versioned folders for DS -- ds_conn_new = ds_manager.peer_manager.connection_router.connections[0] ds_personal_name = f"{NEW_VERSION}#{ds_email}" ds_personal_id = ds_conn_new._find_folder_by_name( @@ -231,47 +255,20 @@ def test_version_mismatch_and_backup_flow(): ) assert ds_personal_id is not None - # -- Step 12: Assert peer connection is gone -- - assert len(do_manager.peer_manager.approved_peers) == 0 - assert len(ds_manager.peer_manager.approved_peers) == 0 + ds_manager.load_peers() + assert any(p.email == do_email for p in ds_manager.peer_manager.approved_peers) - # -- Step 13: Re-add peers -- - ds_manager.add_peer(do_manager.email) - do_manager.load_peers() - do_manager.approve_peer_request(ds_manager.email) - - # The P2P folders of the old version are reused, not replaced. Both - # peers compute this folder name from their own client version, so a - # peer that has not upgraded still looks for the old name. A second - # folder under NEW_VERSION would hide the first one from that peer. + # P2P folders of the old version are reused, not replaced. A peer that + # has not upgraded still looks for the old name. do_p2p_new = _find_versioned_p2p_folders(do_conn_new, ds_email, NEW_VERSION) assert len(do_p2p_new) == 0 do_p2p_old = _find_versioned_p2p_folders( do_conn_new, ds_email, SYFT_CLIENT_VERSION ) assert len(do_p2p_old) > 0 + assert len(do_p2p_old) == len(do_p2p_current) - ds_p2p_new = _find_versioned_p2p_folders(ds_conn_new, do_email, NEW_VERSION) - assert len(ds_p2p_new) == 0 - ds_p2p_old = _find_versioned_p2p_folders( - ds_conn_new, do_email, SYFT_CLIENT_VERSION - ) - assert len(ds_p2p_old) > 0 - - # -- Step 14: Re-upload dataset -- - mock_path2, private_path2, readme_path2 = create_tmp_dataset_files() - do_manager.create_dataset( - name="my dataset", - mock_path=mock_path2, - private_path=private_path2, - summary="Test dataset v2", - readme_path=readme_path2, - users=[ds_manager.email], - ) - do_manager.sync() - ds_manager.sync() - - # -- Step 15: Re-submit job -- + # -- Step 10: Submit a new job without re-peering -- project_dir2 = create_test_project_folder(with_pyproject=False) ds_manager.submit_python_job( user=do_manager.email, @@ -281,26 +278,125 @@ def test_version_mismatch_and_backup_flow(): ) do_manager.sync() - # -- Step 16: Assert only one job (new one), old folder still has old -- - assert len(do_manager.jobs) == 1 + post = [job for job in do_manager.jobs if job.name == "post_upgrade.job"] + assert len(post) == 1 + post[0].approve() + do_manager.process_approved_jobs() + do_manager.sync() + # Reload from disk; the pre-process JobState object does not update in place. + post = [job for job in do_manager.jobs if job.name == "post_upgrade.job"] + assert len(post) == 1 + assert post[0].status == "done" + + ds_manager.sync() + ds_post = [ + job for job in ds_manager.job_client.jobs if job.name == "post_upgrade.job" + ] + assert len(ds_post) == 1 + assert ds_post[0].status == "done" - # Old versioned P2P folders still have old data - old_do_p2p = _find_versioned_p2p_folders( - do_conn_new, ds_email, SYFT_CLIENT_VERSION + +def _upgraded_manager(manager): + """The same client after an upgrade: a new process on the same data. + + A real upgrade restarts the process, so the peer manager computes its own + version again. Reusing the pre-upgrade object would read a cached version. + """ + # No version write here. The test must show that login is what refreshes + # the version files, so the new manager must not do it first. + return _reinitialize_manager( + manager.email, + _get_backing_store(manager), + has_do_role=True, + has_ds_role=False, + syftbox_folder=manager.syftbox_folder, + write_version=False, + ) + + +def test_login_writes_the_remote_version_file_too(): + """Login must refresh both version files, not only the local one. + + A peer reads the remote file to select a job or dataset protocol version for + us. A local-only write leaves that file at the version that first created + it, so peers keep negotiating against a client we no longer run. + """ + from syft_client.sync.login import _init_client_login + from syft_client.sync.version.local_version import read_local_version + + _, do_manager = SyftboxManager.pair_with_mock_drive_service_connection( + use_in_memory_cache=False, + check_versions=True, + ) + conn = do_manager.peer_manager.connection_router.connections[0] + assert conn.read_own_version_file().syft_client_version == SYFT_CLIENT_VERSION + + with ( + patch("syft_client.version.SYFT_CLIENT_VERSION", NEW_VERSION), + patch("syft_client.sync.version.version_info.SYFT_CLIENT_VERSION", NEW_VERSION), + ): + upgraded = _upgraded_manager(do_manager) + new_conn = upgraded.peer_manager.connection_router.connections[0] + # Still the pre-upgrade version: nothing has refreshed it yet. + assert ( + new_conn.read_own_version_file().syft_client_version == SYFT_CLIENT_VERSION ) - assert len(old_do_p2p) > 0 - # -- Step 17: DO runs new job -- - do_manager.jobs[0].approve() - do_manager.process_approved_jobs() - do_manager.sync() + _init_client_login(upgraded, sync=False, load_peers=False) - assert do_manager.jobs[0].status == "done" + assert new_conn.read_own_version_file().syft_client_version == NEW_VERSION + local = read_local_version(upgraded.syftbox_folder) + assert local is not None + assert local.syft_client_version == NEW_VERSION - # -- Step 18: DS sees result -- - ds_manager.sync() - ds_jobs = ds_manager.job_client.jobs - assert len(ds_jobs) == 1 - # DS should have received the output file via sync - ds_job = ds_jobs[0] - assert ds_job.status == "done" + +def test_the_mismatch_prompt_does_not_return_after_a_login(): + """The prompt asks once per upgrade, not once per login. + + The check compares the installed client with the local and the remote + version file. Login refreshes both, so the next login finds no mismatch. + """ + from syft_client.sync.login import _init_client_login + + _, do_manager = SyftboxManager.pair_with_mock_drive_service_connection( + use_in_memory_cache=False, + check_versions=True, + ) + email = do_manager.email + syftbox_folder = do_manager.syftbox_folder + + with ( + patch("syft_client.version.SYFT_CLIENT_VERSION", NEW_VERSION), + patch("syft_client.sync.version.version_info.SYFT_CLIENT_VERSION", NEW_VERSION), + patch("syft_client.sync.login_utils.SYFT_CLIENT_VERSION", NEW_VERSION), + patch("syft_client.sync.login_utils._resolve_email", return_value=email), + patch("syft_client.sync.login_utils._resolve_token_path", return_value=None), + patch( + "syft_client.sync.login_utils._get_default_syftbox_path", + return_value=syftbox_folder, + ), + patch( + "syft_client.sync.login_utils._prompt_mismatch", return_value="1" + ) as mock_prompt, + ): + from syft_client.sync.login_utils import ( + handle_potential_version_mismatches_on_login, + ) + + conn = do_manager.peer_manager.connection_router.connections[0] + with patch( + "syft_client.sync.login_utils._read_remote_version", + side_effect=lambda e, t: conn.read_own_version_file(), + ): + # The check runs before the client exists, so it reads the files + # the previous client version left behind. + handle_potential_version_mismatches_on_login(email) + assert mock_prompt.call_count == 1 + + upgraded = _upgraded_manager(do_manager) + _init_client_login(upgraded, sync=False, load_peers=False) + + # Every login after that finds both files current, and asks nothing. + handle_potential_version_mismatches_on_login(email) + handle_potential_version_mismatches_on_login(email) + assert mock_prompt.call_count == 1 From e5902b46e4f852b4149351c19b8cdb9ebe13c115 Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Tue, 11 Aug 2026 14:53:49 -0300 Subject: [PATCH 14/15] Update documentation for generate_release_fixture.py to clarify release process --- scripts/generate_release_fixture.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/generate_release_fixture.py b/scripts/generate_release_fixture.py index 0632ba234c4..c6f7735f180 100644 --- a/scripts/generate_release_fixture.py +++ b/scripts/generate_release_fixture.py @@ -1,9 +1,14 @@ """Generate a p2p backward-compatibility fixture for the current syft-client release. -Run on EVERY release, after bumping the version: +Run on EVERY release, at the released commit: + git checkout syft-client/v uv run python scripts/generate_release_fixture.py +The fixture name comes from SYFT_CLIENT_VERSION in the tree. The release job +publishes the version on the branch, tags it, then bumps. A run after the bump +therefore names the fixture after the next version, which is not published yet. + Writes the serialized artifacts exactly as this release produces them, into tests/migrations/p2p/fixtures/syft_client--protocol

/ From bd12a1de573eefb65b5f4eaa4db7159b27e875c5 Mon Sep 17 00:00:00 2001 From: Pedro Werneck Date: Tue, 11 Aug 2026 16:12:05 -0300 Subject: [PATCH 15/15] Add private dataset directory fixture and update tests for versioned layouts --- packages/syft-enclave/tests/conftest.py | 38 +++++++++++++++++++ .../tests/test_enclave_datasets.py | 22 ++++------- .../syft-enclave/tests/test_immutability.py | 20 ++++++---- 3 files changed, 57 insertions(+), 23 deletions(-) create mode 100644 packages/syft-enclave/tests/conftest.py diff --git a/packages/syft-enclave/tests/conftest.py b/packages/syft-enclave/tests/conftest.py new file mode 100644 index 00000000000..d3b1c23085a --- /dev/null +++ b/packages/syft-enclave/tests/conftest.py @@ -0,0 +1,38 @@ +from pathlib import Path +from typing import Optional + +import pytest + +PRIVATE_DATASETS_REL = Path("private") / "syft_datasets" + + +def _private_dataset_dirs( + syftbox_folder: Path, owner_email: str, tag: str +) -> list[Path]: + """Every layout of one private dataset: the flat one and each v one.""" + base = syftbox_folder / owner_email / PRIVATE_DATASETS_REL + if not base.is_dir(): + return [] + candidates = [base / tag] + candidates += [d / tag for d in sorted(base.glob("v*")) if d.is_dir()] + return [p for p in candidates if p.is_dir()] + + +@pytest.fixture +def private_dataset_dir(): + """Find the private directory of a dataset, whatever protocol layout holds it. + + The layout of a private dataset is `private/syft_datasets/[v/]`, and + the segment depends on the protocol version of the copy. A test asserts that + the data arrived, so it must not name one version. + + Returns a callable `(syftbox_folder, owner_email, tag) -> Path | None`. The + callable raises if more than one layout holds the dataset. + """ + + def _find(syftbox_folder: Path, owner_email: str, tag: str) -> Optional[Path]: + dirs = _private_dataset_dirs(syftbox_folder, owner_email, tag) + assert len(dirs) <= 1, f"More than one layout holds {tag!r}: {dirs}" + return dirs[0] if dirs else None + + return _find diff --git a/packages/syft-enclave/tests/test_enclave_datasets.py b/packages/syft-enclave/tests/test_enclave_datasets.py index 77ef5c992ca..0a603ae3c02 100644 --- a/packages/syft-enclave/tests/test_enclave_datasets.py +++ b/packages/syft-enclave/tests/test_enclave_datasets.py @@ -17,7 +17,7 @@ def create_tmp_dataset_files(): return mock_path, private_path -def test_share_private_dataset_with_enclave(): +def test_share_private_dataset_with_enclave(private_dataset_dir): """Test full flow: DO creates dataset, shares private data with enclave, enclave can access it.""" enclave, do1, do2, ds = SyftEnclaveClient.quad_with_mock_drive_service_connection( use_in_memory_cache=False, @@ -50,14 +50,10 @@ def test_share_private_dataset_with_enclave(): mock_content = ds_dataset.mock_files[0].read_text() assert mock_content == "Hello, world!" - non_existing_ds_private_dir = ( - ds._manager.syftbox_folder - / do1.email - / "private" - / "syft_datasets" - / "testdataset" + assert ( + private_dataset_dir(ds._manager.syftbox_folder, do1.email, "testdataset") + is None ) - assert not non_existing_ds_private_dir.exists() # DO1 shares private dataset with enclave do1.share_private_dataset("testdataset", enclave.email) @@ -67,14 +63,10 @@ def test_share_private_dataset_with_enclave(): # Enclave can see the dataset via mock data (shared with DS and enclave shares peers) # But more importantly, enclave can access private files via shared_private_dir - enclave_private_dir = ( - enclave._manager.syftbox_folder - / do1.email - / "private" - / "syft_datasets" - / "testdataset" + enclave_private_dir = private_dataset_dir( + enclave._manager.syftbox_folder, do1.email, "testdataset" ) - assert enclave_private_dir.exists() + assert enclave_private_dir is not None private_files = list(enclave_private_dir.iterdir()) file_names = {f.name for f in private_files} assert "private.txt" in file_names diff --git a/packages/syft-enclave/tests/test_immutability.py b/packages/syft-enclave/tests/test_immutability.py index 15ea205ada1..66cb9bffb1e 100644 --- a/packages/syft-enclave/tests/test_immutability.py +++ b/packages/syft-enclave/tests/test_immutability.py @@ -20,6 +20,14 @@ def test_is_private_dataset_path_positive(): ) +def test_is_private_dataset_path_versioned_layout(): + # A protocol copy holds its files under a v segment. The filter must + # protect that layout too, and not only the flat one of protocol 0. + assert is_private_dataset_path( + "do@example.com/private/syft_datasets/v1/my_ds/data.csv" + ) + + def test_is_private_dataset_path_public(): assert not is_private_dataset_path( "do@example.com/public/syft_datasets/my_ds/data.csv" @@ -103,7 +111,7 @@ def _create_tmp_dataset_files(): return mock_path, private_path -def test_enclave_blocks_reshare_of_private_dataset(): +def test_enclave_blocks_reshare_of_private_dataset(private_dataset_dir): """After DO shares private data with enclave, a second share should not overwrite.""" enclave, do1, do2, ds = SyftEnclaveClient.quad_with_mock_drive_service_connection( use_in_memory_cache=False, @@ -124,14 +132,10 @@ def test_enclave_blocks_reshare_of_private_dataset(): do1.share_private_dataset("testdataset", enclave.email) enclave._manager.sync() - enclave_private_dir = ( - enclave._manager.syftbox_folder - / do1.email - / "private" - / "syft_datasets" - / "testdataset" + enclave_private_dir = private_dataset_dir( + enclave._manager.syftbox_folder, do1.email, "testdataset" ) - assert enclave_private_dir.exists() + assert enclave_private_dir is not None original_content = (enclave_private_dir / "private.txt").read_bytes() assert original_content == b"Hello, world private!"