From 2893e0e0e54a5ac9325449aa6de60e8a1417a828 Mon Sep 17 00:00:00 2001 From: PARTH J ROHIT Date: Fri, 28 Aug 2026 18:24:48 +0100 Subject: [PATCH 1/4] feat(backend): implement Repository Lineage per RFC-0002 (#299) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the durable, owner-scoped logical grouping above repository revisions that RFC-0002 defines: a new `repository_lineages` table, nullable `repositories.lineage_id`/`sequence` columns, and race-free transactional allocation on live GitHub import, wired against the plan in docs/architecture/REPOSITORY_LINEAGE_MIGRATION_PLAN.md (owner-approved via PR #328, authorized for implementation per that document's executive verdict; the #322 rehearsal/recovery prerequisite is already closed). This is the highest-risk change of the whole backlog this pass, and it was treated that way throughout: implementation only started after the owner's explicit go-ahead given the specific risk (a production schema migration introducing cyclic deferred foreign keys), with three explicit conditions -- real-PostgreSQL concurrency verification (not just SQLite), a full disposable-database rehearsal via the existing rehearsal script, and extra-thorough test coverage on the FK-cycle handling specifically. All three are satisfied, in detail, below. ## What's new - `repository_lineages`: id, owner-scoped canonical (`canonical_source_key`, `canonical_branch`) key, `display_name`, `latest_repository_id`, `next_sequence`, `created_at`. A partial unique index is the owner-scoped canonical lookup and the sole race-arbiter if two imports try to create the same lineage at once. - `repositories.lineage_id` / `repositories.sequence`: permanently nullable. Uploads and unresolved-ref legacy GitHub rows are unlineaged standalone imports by design (RFC §4.3/§6), not a transitional state. - The cyclic integrity boundary: `repository_lineages.latest_repository_id` (+`id`) -> `repositories.id`/`lineage_id`, and `repositories.lineage_id` (+`owner_id`) -> `repository_lineages.id`/`owner_id`. Both are deferrable/ initially-deferred composite foreign keys, verified empirically (raw SQL, SQLAlchemy `create_all()`, and the actual Alembic migration) on both SQLite and real PostgreSQL before writing a line of the real implementation, per the plan's own explicit "if SQLite can't preserve this, stop" instruction. - Two Alembic revisions (`0013_lineage_expand`, `0014_lineage_constraints` -- the plan suggested 0011/0012, but 0011/0012 were taken by unrelated work that landed on dev since the plan was written): revision A creates the lineage table, expands `repositories`, runs a strict deterministic backfill of resolvable historical GitHub commits (grouped by owner + case-folded canonical source + exact ref, sorted by created_at/id, UUIDv5-keyed for idempotent reconciliation), verifies every §6.4 invariant, then closes the non-cyclic constraints; revision B closes the cyclic one. Uploads, unresolved refs, and any URL outside the exact accepted historical grammar stay standalone -- nothing is guessed. - `RepositoryRepository.add_with_lineage`: the transactional allocator (RFC §5.2) -- find-or-create the lineage, atomically claim the next sequence via `UPDATE ... SET next_sequence = next_sequence + 1`, check for a same-lineage duplicate commit, insert, update the latest pointer, commit once. Bounded retry (5, matching `AiConversationRepository`) on the create-lineage race. - `RepositoryRepository.delete_with_lineage_update`: rolls the latest pointer back to the next-highest surviving sequence before deleting, never decrements the counter, keeps an empty lineage (not garbage collected) so a later re-import never reuses a sequence. - `RepositoryService.import_github_repository` now always computes a canonical pair (a resolved ref is guaranteed by the time this runs) and persists through the transactional allocator instead of a plain insert. `import_uploaded_repository` is unchanged in behaviour -- it already never set lineage fields. ## A real bug found and fixed along the way (not previously present) Removed the old pre-clone `find_by_source_revision_for_owner` duplicate check. It matched on `(source_url, revision_value, owner_id)` with no branch/ref component -- structurally impossible to make branch-aware, since the ref isn't resolved until after cloning -- and it incorrectly rejected re-importing the same commit under a different branch, which RFC-0002 explicitly requires to succeed. The new transactional, lineage-scoped duplicate check is now the sole and correct authority; a regression test (`test_same_commit_is_allowed_in_two_different_branch_lineages`) covers exactly this case. ## A real SQLite/Alembic bug found and fixed along the way `alembic/env.py` now disables `PRAGMA foreign_keys` for the connection Alembic itself uses to run migrations, SQLite only. Root cause, confirmed by direct reproduction: SQLite refuses to toggle that pragma mid-transaction (a documented no-op once a transaction is open), and Alembic's own per-migration transaction is already open by the time a revision's `upgrade()` runs. Revision B's batch-mode recreation of `repository_lineages` (required to add the cyclic FK -- SQLite has no `ALTER TABLE ADD CONSTRAINT`) drops and rebuilds a table that `repositories` already has a deferred FK pointing at from revision A; SQLite's deferred-FK bookkeeping does not correctly reconcile that recreation against the still-open transaction, so a fully self-consistent final state still fails at COMMIT with a generic "FOREIGN KEY constraint failed" (`PRAGMA foreign_key_check` reports zero violations immediately beforehand -- confirmed directly). This only reproduced once `app.core.database` had been imported earlier in the same process (registering the global `PRAGMA foreign_keys=ON` connect-event listener), so it silently passed in isolated runs and only surfaced in a full-suite run -- exactly the kind of thing "extra-thorough" coverage was meant to catch. The regression test now forces the listener's registration itself rather than depending on incidental test-file ordering, and was verified to actually fail without the fix (confirmed by temporarily reverting it). Runtime enforcement for the real application is unaffected: every normal app connection still gets `PRAGMA foreign_keys=ON` as before; this change touches only the connection Alembic itself uses while migrating. ## Verification (the three explicit conditions) 1. **Real-PostgreSQL concurrency verification**: `test_repository_lineage_concurrency.py`, real threads against a real, separate-connection PostgreSQL database with `threading.Barrier` synchronization (matching the existing `test_concurrent_refresh_on_postgres_mints_one_successor` pattern) -- two different commits racing into an existing lineage get unique consecutive sequences; two first-imports racing create exactly one lineage; two identical commits racing produce one repository and one correctly-rejected conflict with no burned sequence number; a forced duplicate `(lineage_id, sequence)` is rejected; a forced cross-owner attachment is rejected by the composite FK; a forced cross-lineage latest pointer is rejected by the composite FK; account deletion cascades a user's lineages without touching another owner's. All 7 pass reliably (verified across 5 repeated runs) and clean up every row they create. 2. **Full disposable rehearsal**: `python scripts/rehearse_migrations.py` (SQLite) and `--postgres` (real, disposable database) both pass -- clean upgrade -> clean downgrade -> re-upgrade, and the representative `0004_ai_provider_configs` baseline reaches head. `HEAD_REVISION` and `REQUIRED_HEAD_TABLES` updated for the new head. 3. **Extra-thorough FK-cycle coverage**: beyond the concurrency file's direct FK-forcing tests, `test_repository_lineage_migration.py` verifies the deferred FK's exact shape (constrained/referred columns, `deferrable`) after a fresh migration, and a populated-database backfill test exercises every §6 grouping/exclusion case in one pass (same source/ref groups; different ref/repo/owner separate; a URL variant -- mixed-case host, `.git` suffix, trailing slash -- still canonicalizes into the same lineage; malformed URL, unresolved ref, and upload all stay standalone; a deterministic timestamp tie breaks on repository id). Full backend suite (`pytest`, no `-k`) green on both SQLite and real PostgreSQL. `ruff check`/`ruff format --check`/`mypy` clean on every changed file. `npm run generate:api-contract -- --check` confirms zero frontend contract drift, matching the plan's "no API contract change, no frontend surface" scope. Docs updated: the migration plan's revision-ID note now reflects the actual 0013/0014 numbering, and the rehearsal runbook's stated head revision is current. --- apps/backend/alembic/env.py | 21 + .../alembic/versions/0013_lineage_expand.py | 382 +++++++++++++++ .../versions/0014_lineage_constraints.py | 44 ++ apps/backend/app/models/__init__.py | 2 + apps/backend/app/models/repository.py | 32 ++ apps/backend/app/models/repository_lineage.py | 88 ++++ .../app/repositories/repository_repository.py | 189 +++++++- .../app/services/repository_service.py | 68 ++- apps/backend/scripts/rehearse_migrations.py | 3 +- .../test_repository_lineage_concurrency.py | 456 ++++++++++++++++++ .../test_repository_lineage_migration.py | 455 +++++++++++++++++ .../tests/test_repository_lineage_service.py | 397 +++++++++++++++ .../REPOSITORY_LINEAGE_MIGRATION_PLAN.md | 8 +- .../DATABASE_MIGRATION_REHEARSAL.md | 2 +- 14 files changed, 2109 insertions(+), 38 deletions(-) create mode 100644 apps/backend/alembic/versions/0013_lineage_expand.py create mode 100644 apps/backend/alembic/versions/0014_lineage_constraints.py create mode 100644 apps/backend/app/models/repository_lineage.py create mode 100644 apps/backend/tests/test_repository_lineage_concurrency.py create mode 100644 apps/backend/tests/test_repository_lineage_migration.py create mode 100644 apps/backend/tests/test_repository_lineage_service.py diff --git a/apps/backend/alembic/env.py b/apps/backend/alembic/env.py index 64cf2b23..61de3f30 100644 --- a/apps/backend/alembic/env.py +++ b/apps/backend/alembic/env.py @@ -41,6 +41,27 @@ def run_migrations_online() -> None: ) with connectable.connect() as connection: + if connection.dialect.name == "sqlite": + # SQLite refuses to toggle `PRAGMA foreign_keys` mid-transaction + # (a documented no-op once a transaction is open), and Alembic's + # own per-migration transaction is already open by the time a + # revision's upgrade() runs. A migration that uses batch mode to + # add a constraint to a table that another table already has a + # deferred foreign key pointing at (e.g. #299's cyclic + # repository_lineages <-> repositories relationship) drops and + # recreates that table; SQLite's deferred-FK bookkeeping does not + # correctly reconcile that recreation against the still-open + # transaction, and a fully self-consistent final state still + # fails at COMMIT with a generic "FOREIGN KEY constraint failed" + # (verified: `PRAGMA foreign_key_check` reports no violation + # immediately beforehand). Disabling enforcement here, before any + # transaction opens, avoids this without weakening runtime + # enforcement: every real application connection still gets + # `PRAGMA foreign_keys=ON` via app.core.database's own + # connect-event listener; this affects only the connection + # Alembic itself uses while migrating. + connection.exec_driver_sql("PRAGMA foreign_keys=OFF") + connection.commit() context.configure(connection=connection, target_metadata=target_metadata) with context.begin_transaction(): diff --git a/apps/backend/alembic/versions/0013_lineage_expand.py b/apps/backend/alembic/versions/0013_lineage_expand.py new file mode 100644 index 00000000..5944da22 --- /dev/null +++ b/apps/backend/alembic/versions/0013_lineage_expand.py @@ -0,0 +1,382 @@ +"""add repository_lineages, expand repositories, and backfill (1 of 2) + +Revision ID: 0013_lineage_expand +Revises: 0012_waitlist_entries +Create Date: 2026-08-27 + +Issue #299 (RFC-0002), authorized for implementation per +docs/architecture/REPOSITORY_LINEAGE_MIGRATION_PLAN.md. Adds the durable +owner-scoped logical grouping above repository revisions: a new +``repository_lineages`` table, nullable ``repositories.lineage_id`` / +``repositories.sequence`` columns, and a strict, deterministic backfill for +resolvable historical GitHub commits. + +This is deliberately split into two revisions (plan §7). This one creates +every constraint that does *not* require the second table to already exist, +runs the backfill, verifies it, then closes every constraint that only needs +one side of the eventual cyclic integrity boundary. The genuinely cyclic +constraint -- the lineage's latest-member pointer proving it names a +repository in that exact lineage -- is Revision B +(0014_lineage_constraints), so a failure here leaves this revision's state +additive, backward-compatible, and inspectable before retrying B. + +Backfill scope (plan §6): only ``source='github'`` rows with a valid 40-hex +commit SHA, a resolved ``refs/heads/...``/``refs/tags/...`` ref, and a +``source_url`` matching one of the exact accepted historical GitHub URL +forms are grouped into a lineage. Uploads, unresolved-ref rows, and anything +outside that strict grammar stay unlineaged standalone imports -- this +migration never guesses a grouping from a name, path, or network call. + +Imports must be quiesced while this migration runs; there is no dual-write +compatibility for a concurrent insert racing the backfill. +""" + +from __future__ import annotations + +import re +import uuid +from typing import Any + +from alembic import op +import sqlalchemy as sa + +revision = "0013_lineage_expand" +down_revision = "0012_waitlist_entries" +branch_labels = None +depends_on = None + +_GIT_SHA_RE = re.compile(r"^[0-9a-f]{40}$") +_REF_RE = re.compile(r"^refs/(?:heads|tags)/[A-Za-z0-9._/-]+$") +# The host is matched case-insensitively (scoped to just that group) because +# the host itself is one of the two things plan §6.1 step 3 requires +# case-folding -- a historical pre-hardening URL may have used "GitHub.com". +# The scheme/userinfo-marker literals stay exact-case; only the host is +# case-variable here. +_HTTPS_GITHUB_RE = re.compile(r"^https://(?i:github\.com)/([^/]+)/([^/]+?)(?:\.git)?/?$") +_SSH_GITHUB_RE = re.compile(r"^git@(?i:github\.com):([^/]+)/([^/]+?)(?:\.git)?$") +_SAFE_COMPONENT_RE = re.compile(r"^[A-Za-z0-9._-]+$") + +_CANONICAL_PARTIAL_WHERE = sa.text("canonical_source_key IS NOT NULL AND canonical_branch IS NOT NULL") + +# Fixed, migration-local UUIDv5 namespace for deterministic backfill lineage +# IDs (plan §6.2). Frozen forever once this revision ships, exactly like +# every other literal in an applied migration -- never imported from, or +# shared with, live application code, which is free to evolve independently. +_BACKFILL_UUID_NAMESPACE = uuid.UUID("ac9ac65f-f0c1-4f7f-8147-9dc3509b4eaa") + + +def _safe_component(value: str) -> bool: + return bool(_SAFE_COMPONENT_RE.fullmatch(value)) and ".." not in value + + +def _canonical_github_source(source_url: str | None) -> str | None: + """Strict, conservative GitHub URL canonicalization, backfill-only (plan + §6.1). Returns ``None`` for anything outside the exact accepted forms -- + the row then stays an unlineaged standalone import, never a guess. + + Deliberately not shared with ``app.services.repository_service``'s live + parser (which only needs to handle its own validator's already-narrow + output): a future change to that live code must never silently change + what this frozen, already-applied migration replays. + """ + if not source_url: + return None + trimmed = source_url.strip() + match = _HTTPS_GITHUB_RE.match(trimmed) or _SSH_GITHUB_RE.match(trimmed) + if not match: + return None + owner, repo = match.group(1), match.group(2) + if not (_safe_component(owner) and _safe_component(repo)): + return None + return f"github.com/{owner.lower()}/{repo.lower()}" + + +def _lineage_id_for(owner_id: str, canonical_source_key: str, canonical_branch: str) -> str: + """Deterministic, length-delimited encoding (plan §6.2) -- avoids the + ambiguity a plain concatenation would have (e.g. two different + owner/key/branch triples producing the same joined string).""" + encoded = ( + f"{len(owner_id)}:{owner_id}|" + f"{len(canonical_source_key)}:{canonical_source_key}|" + f"{len(canonical_branch)}:{canonical_branch}" + ) + return str(uuid.uuid5(_BACKFILL_UUID_NAMESPACE, encoded)) + + +def _create_lineage_table() -> None: + op.create_table( + "repository_lineages", + sa.Column("id", sa.String(length=36), primary_key=True), + sa.Column("owner_id", sa.String(length=36), nullable=False), + sa.Column("canonical_source_key", sa.Text(), nullable=True), + sa.Column("canonical_branch", sa.Text(), nullable=True), + sa.Column("display_name", sa.Text(), nullable=False), + # The cyclic half of this column's FK (proving the pointer names a + # repository in *this* lineage) is Revision B; it is a plain nullable + # column here. + sa.Column("latest_repository_id", sa.String(length=36), nullable=True), + sa.Column("next_sequence", sa.Integer(), nullable=False), + sa.Column("created_at", sa.DateTime(timezone=True), nullable=False), + sa.UniqueConstraint("id", "owner_id", name="uq_repository_lineages_id_owner"), + sa.CheckConstraint( + "(canonical_source_key IS NULL AND canonical_branch IS NULL) OR " + "(canonical_source_key IS NOT NULL AND canonical_branch IS NOT NULL)", + name="ck_repository_lineages_canonical_pair", + ), + sa.CheckConstraint("next_sequence >= 1", name="ck_repository_lineages_next_sequence_positive"), + sa.ForeignKeyConstraint( + ["owner_id"], + ["users.id"], + name="fk_repository_lineages_owner_id_users", + ondelete="CASCADE", + ), + ) + op.create_index("ix_repository_lineages_owner_id", "repository_lineages", ["owner_id"]) + op.create_index( + "uq_repository_lineages_owner_source_branch", + "repository_lineages", + ["owner_id", "canonical_source_key", "canonical_branch"], + unique=True, + sqlite_where=_CANONICAL_PARTIAL_WHERE, + postgresql_where=_CANONICAL_PARTIAL_WHERE, + ) + + +def _add_repository_lineage_columns() -> None: + # Both additions in one batch block (plan §7 step 2): avoids a second + # SQLite table copy for what is otherwise the same operation. + with op.batch_alter_table("repositories") as batch: + batch.add_column(sa.Column("lineage_id", sa.String(length=36), nullable=True)) + batch.add_column(sa.Column("sequence", sa.Integer(), nullable=True)) + + +def _repositories_core_table() -> sa.TableClause: + return sa.table( + "repositories", + sa.column("id", sa.String()), + sa.column("owner_id", sa.String()), + sa.column("name", sa.String()), + sa.column("source", sa.String()), + sa.column("source_url", sa.Text()), + sa.column("revision_kind", sa.String()), + sa.column("revision_value", sa.String()), + sa.column("revision_ref", sa.String()), + sa.column("created_at", sa.DateTime()), + sa.column("lineage_id", sa.String()), + sa.column("sequence", sa.Integer()), + ) + + +def _lineages_core_table() -> sa.TableClause: + return sa.table( + "repository_lineages", + sa.column("id", sa.String()), + sa.column("owner_id", sa.String()), + sa.column("canonical_source_key", sa.Text()), + sa.column("canonical_branch", sa.Text()), + sa.column("display_name", sa.Text()), + sa.column("latest_repository_id", sa.String()), + sa.column("next_sequence", sa.Integer()), + sa.column("created_at", sa.DateTime()), + ) + + +def _eligible_groups(connection: sa.Connection, repositories: sa.TableClause) -> dict[tuple[str, str, str], list[dict]]: + """Group resolvable historical GitHub rows by (owner, canonical source, + canonical branch), sorted deterministically within each group (plan + §6.1/§6.2). Everything else (uploads, unresolved refs, malformed/foreign + source URLs) is excluded and stays standalone.""" + rows = connection.execute( + sa.select( + repositories.c.id, + repositories.c.owner_id, + repositories.c.name, + repositories.c.source, + repositories.c.source_url, + repositories.c.revision_kind, + repositories.c.revision_value, + repositories.c.revision_ref, + repositories.c.created_at, + ) + ).mappings() + + groups: dict[tuple[str, str, str], list[dict[str, Any]]] = {} + for row in rows: + if row["source"] != "github" or row["revision_kind"] != "git": + continue + revision_value = row["revision_value"] + if not revision_value or not _GIT_SHA_RE.fullmatch(revision_value): + continue + revision_ref = row["revision_ref"] + if not revision_ref or not _REF_RE.fullmatch(revision_ref): + continue + canonical_source_key = _canonical_github_source(row["source_url"]) + if canonical_source_key is None: + continue + key = (row["owner_id"], canonical_source_key, revision_ref) + groups.setdefault(key, []).append(dict(row)) + + for members in groups.values(): + members.sort(key=lambda member: (member["created_at"], member["id"])) + return groups + + +def _backfill_lineages() -> dict[tuple[str, str, str], list[dict]]: + connection = op.get_bind() + repositories = _repositories_core_table() + lineages = _lineages_core_table() + + groups = _eligible_groups(connection, repositories) + + for (owner_id, canonical_source_key, canonical_branch), members in groups.items(): + lineage_id = _lineage_id_for(owner_id, canonical_source_key, canonical_branch) + first, last = members[0], members[-1] + next_sequence = len(members) + 1 + + already_present = connection.execute(sa.select(lineages.c.id).where(lineages.c.id == lineage_id)).first() + if already_present is None: + connection.execute( + lineages.insert().values( + id=lineage_id, + owner_id=owner_id, + canonical_source_key=canonical_source_key, + canonical_branch=canonical_branch, + display_name=first["name"], + latest_repository_id=last["id"], + next_sequence=next_sequence, + created_at=first["created_at"], + ) + ) + else: + # A prior interrupted run already created this row (plan §6.2/§7 + # "interruption and rerun"): reconcile its counter and latest + # pointer to this deterministic grouping rather than trusting + # whatever a partial run left behind. + connection.execute( + lineages.update() + .where(lineages.c.id == lineage_id) + .values(latest_repository_id=last["id"], next_sequence=next_sequence) + ) + + for index, member in enumerate(members, start=1): + connection.execute( + repositories.update() + .where(repositories.c.id == member["id"]) + .values(lineage_id=lineage_id, sequence=index) + ) + + return groups + + +def _verify_backfill(groups: dict[tuple[str, str, str], list[dict]]) -> None: + """Abort the migration unless every §6.4 invariant holds. Reuses the same + grouping the write phase just used (rather than re-deriving the + eligibility predicate a second time in SQL), so this proves the write + actually took effect as intended, not just that a second, possibly + independently-buggy, predicate agrees with the first.""" + connection = op.get_bind() + repositories = _repositories_core_table() + lineages = _lineages_core_table() + + touched = 0 + for (owner_id, canonical_source_key, canonical_branch), members in groups.items(): + lineage_id = _lineage_id_for(owner_id, canonical_source_key, canonical_branch) + lineage_row = ( + connection.execute( + sa.select(lineages.c.owner_id, lineages.c.latest_repository_id, lineages.c.next_sequence).where( + lineages.c.id == lineage_id + ) + ) + .mappings() + .first() + ) + if lineage_row is None: + raise RuntimeError(f"Lineage backfill verification failed: {lineage_id} is missing after backfill.") + if lineage_row["owner_id"] != owner_id: + raise RuntimeError(f"Lineage backfill verification failed: {lineage_id} has the wrong owner.") + if lineage_row["latest_repository_id"] != members[-1]["id"]: + raise RuntimeError(f"Lineage backfill verification failed: {lineage_id} latest pointer is wrong.") + if lineage_row["next_sequence"] != len(members) + 1: + raise RuntimeError(f"Lineage backfill verification failed: {lineage_id} next_sequence is wrong.") + + seen_sequences: set[int] = set() + for index, member in enumerate(members, start=1): + repo_row = ( + connection.execute( + sa.select(repositories.c.lineage_id, repositories.c.sequence, repositories.c.owner_id).where( + repositories.c.id == member["id"] + ) + ) + .mappings() + .first() + ) + if repo_row is None or repo_row["lineage_id"] != lineage_id or repo_row["sequence"] != index: + raise RuntimeError( + f"Lineage backfill verification failed: repository {member['id']} is not " + f"correctly attached to lineage {lineage_id}." + ) + if repo_row["owner_id"] != owner_id: + raise RuntimeError(f"Lineage backfill verification failed: repository {member['id']} owner mismatch.") + if repo_row["sequence"] in seen_sequences: + raise RuntimeError(f"Lineage backfill verification failed: duplicate sequence in lineage {lineage_id}.") + seen_sequences.add(repo_row["sequence"]) + touched += 1 + + stray = connection.execute( + sa.select(sa.func.count()) + .select_from(repositories) + .where(sa.or_(repositories.c.lineage_id.isnot(None), repositories.c.sequence.isnot(None))) + ).scalar() + if stray != touched: + raise RuntimeError( + f"Lineage backfill verification failed: {stray} repositories carry a lineage " + f"attachment, expected exactly {touched} from the eligible groups." + ) + + +def _add_repository_lineage_constraints() -> None: + with op.batch_alter_table("repositories") as batch: + batch.create_check_constraint( + "ck_repositories_lineage_sequence_pair", + "(lineage_id IS NULL AND sequence IS NULL) OR " + "(lineage_id IS NOT NULL AND sequence IS NOT NULL AND sequence >= 1)", + ) + # Every NULL is distinct under SQL uniqueness, so standalone rows + # (both null) never collide here (plan §4.2). + batch.create_unique_constraint("uq_repositories_lineage_sequence", ["lineage_id", "sequence"]) + # Composite target proving a lineage's latest-member pointer names a + # repository that actually belongs to that exact lineage (used by + # Revision B's FK). + batch.create_unique_constraint("uq_repositories_id_lineage", ["id", "lineage_id"]) + batch.create_foreign_key( + "fk_repositories_lineage_owner", + "repository_lineages", + ["lineage_id", "owner_id"], + ["id", "owner_id"], + deferrable=True, + initially="DEFERRED", + ) + + +def upgrade() -> None: + _create_lineage_table() + _add_repository_lineage_columns() + groups = _backfill_lineages() + _verify_backfill(groups) + _add_repository_lineage_constraints() + + +def downgrade() -> None: + # New grouping/counter data is lost; every pre-existing repository + # column and value is preserved (plan §7/§11). + with op.batch_alter_table("repositories") as batch: + batch.drop_constraint("fk_repositories_lineage_owner", type_="foreignkey") + batch.drop_constraint("uq_repositories_id_lineage", type_="unique") + batch.drop_constraint("uq_repositories_lineage_sequence", type_="unique") + batch.drop_constraint("ck_repositories_lineage_sequence_pair", type_="check") + batch.drop_column("sequence") + batch.drop_column("lineage_id") + + op.drop_index("uq_repository_lineages_owner_source_branch", table_name="repository_lineages") + op.drop_index("ix_repository_lineages_owner_id", table_name="repository_lineages") + op.drop_table("repository_lineages") diff --git a/apps/backend/alembic/versions/0014_lineage_constraints.py b/apps/backend/alembic/versions/0014_lineage_constraints.py new file mode 100644 index 00000000..30b52042 --- /dev/null +++ b/apps/backend/alembic/versions/0014_lineage_constraints.py @@ -0,0 +1,44 @@ +"""close the repository-lineage cyclic integrity boundary (2 of 2) + +Revision ID: 0014_lineage_constraints +Revises: 0013_lineage_expand +Create Date: 2026-08-27 + +Issue #299 (RFC-0002), part 2 of 2. Adds the one constraint that genuinely +needs both sides of the lineage/repository relationship to already exist: +``repository_lineages.latest_repository_id`` (together with ``id``) must +name a row in ``repositories`` whose own ``lineage_id`` points back at this +exact lineage -- a latest pointer can never name a repository in a +different lineage, or a different owner's repository, even if application +code is wrong. + +Split from Revision A specifically so a failure here leaves A's state +additive, backward-compatible, and inspectable/fixable before rerunning +this one (plan §7). The new application must not start unless Alembic is at +this revision or later -- do not start code that assumes the latest-member +invariant against a database still only at Revision A. +""" + +from alembic import op + +revision = "0014_lineage_constraints" +down_revision = "0013_lineage_expand" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + with op.batch_alter_table("repository_lineages") as batch: + batch.create_foreign_key( + "fk_repository_lineages_latest_member", + "repositories", + ["latest_repository_id", "id"], + ["id", "lineage_id"], + deferrable=True, + initially="DEFERRED", + ) + + +def downgrade() -> None: + with op.batch_alter_table("repository_lineages") as batch: + batch.drop_constraint("fk_repository_lineages_latest_member", type_="foreignkey") diff --git a/apps/backend/app/models/__init__.py b/apps/backend/app/models/__init__.py index 41efa14c..b83ce332 100644 --- a/apps/backend/app/models/__init__.py +++ b/apps/backend/app/models/__init__.py @@ -5,6 +5,7 @@ from app.models.invite_token import InviteToken from app.models.refresh_token import RefreshToken from app.models.repository import RepositoryRecord +from app.models.repository_lineage import RepositoryLineage from app.models.snapshot import ( RiAssertion, RiDerivation, @@ -26,6 +27,7 @@ "InviteToken", "RefreshToken", "RepositoryRecord", + "RepositoryLineage", "RiAssertion", "RiDerivation", "RiDiagnostic", diff --git a/apps/backend/app/models/repository.py b/apps/backend/app/models/repository.py index 208cf17f..bc1c32a9 100644 --- a/apps/backend/app/models/repository.py +++ b/apps/backend/app/models/repository.py @@ -5,6 +5,7 @@ CheckConstraint, DateTime, ForeignKey, + ForeignKeyConstraint, Index, Integer, JSON, @@ -44,6 +45,12 @@ class RepositoryRecord(Base): revision_kind: Mapped[str | None] = mapped_column(String(16), nullable=True) revision_value: Mapped[str | None] = mapped_column(String(80), nullable=True) revision_ref: Mapped[str | None] = mapped_column(String(255), nullable=True) + # Durable logical grouping above revisions (#299, RFC-0002). Both remain + # permanently nullable: an upload or an unresolved-ref legacy GitHub row + # is a standalone import with no lineage, by design (RFC §4.3/§6), not a + # transitional state to be tightened later. + lineage_id: Mapped[str | None] = mapped_column(String(36), nullable=True) + sequence: Mapped[int | None] = mapped_column(Integer, nullable=True) local_path: Mapped[str] = mapped_column(Text) size: Mapped[int] = mapped_column(BigInteger, default=0) file_count: Mapped[int] = mapped_column(Integer, default=0) @@ -94,6 +101,31 @@ class RepositoryRecord(Base): name="ck_repositories_git_revision", ), Index("ix_repositories_revision_value", "revision_value"), + CheckConstraint( + "(lineage_id IS NULL AND sequence IS NULL) OR " + "(lineage_id IS NOT NULL AND sequence IS NOT NULL AND sequence >= 1)", + name="ck_repositories_lineage_sequence_pair", + ), + # Standalone rows (both null) never collide under a unique constraint: + # SQL uniqueness treats every NULL as distinct. Also serves ordered + # lineage reads. + UniqueConstraint("lineage_id", "sequence", name="uq_repositories_lineage_sequence"), + # Composite target proving a lineage's latest-member pointer names a + # repository that actually belongs to that exact lineage. + UniqueConstraint("id", "lineage_id", name="uq_repositories_id_lineage"), + # Cross-owner attachment is invalid at the database layer even if + # service code is wrong (#299 §9). Deferred: a lineage and its first + # repository are written in one transaction (RFC §5.2), so this must + # not be checked until commit. No automatic delete action -- deletion + # updates or clears the lineage's latest pointer explicitly first + # (RFC §8.3), it is never left to a database cascade/set-null here. + ForeignKeyConstraint( + ["lineage_id", "owner_id"], + ["repository_lineages.id", "repository_lineages.owner_id"], + name="fk_repositories_lineage_owner", + deferrable=True, + initially="DEFERRED", + ), ) @validates("revision_kind", "revision_value") diff --git a/apps/backend/app/models/repository_lineage.py b/apps/backend/app/models/repository_lineage.py new file mode 100644 index 00000000..a91f2d9b --- /dev/null +++ b/apps/backend/app/models/repository_lineage.py @@ -0,0 +1,88 @@ +from datetime import UTC, datetime + +from sqlalchemy import ( + CheckConstraint, + DateTime, + ForeignKeyConstraint, + Index, + Integer, + String, + Text, + UniqueConstraint, + text, +) +from sqlalchemy.orm import Mapped, mapped_column + +from app.models.base import Base + +_CANONICAL_PARTIAL_WHERE = text("canonical_source_key IS NOT NULL AND canonical_branch IS NOT NULL") + + +class RepositoryLineage(Base): + """Durable, owner-scoped logical grouping above repository revisions (#299, RFC-0002). + + A row here groups repeated imports of the same GitHub repository/branch + into one ordered history. Uploads and unresolved-ref GitHub rows never get + a row here -- they stay unlineaged standalone imports (RFC §4.3/§6) -- so + ``canonical_source_key``/``canonical_branch`` and every ``repositories`` + attachment are permanently optional, not a transitional NULL. + """ + + __tablename__ = "repository_lineages" + + id: Mapped[str] = mapped_column(String(36), primary_key=True) + owner_id: Mapped[str] = mapped_column(String(36), index=True) + canonical_source_key: Mapped[str | None] = mapped_column(Text, nullable=True) + canonical_branch: Mapped[str | None] = mapped_column(Text, nullable=True) + display_name: Mapped[str] = mapped_column(Text) + # The current highest surviving sequence in this lineage, or null for an + # empty lineage. This FK is deferrable/initially-deferred and cyclic with + # `repositories`: a lineage must exist (with a null pointer) before its + # first repository can be inserted, and the pointer is only set afterward, + # in the same transaction (RFC §5.1/§5.2). + latest_repository_id: Mapped[str | None] = mapped_column(String(36), nullable=True) + # Durable, never-reused, transactionally-allocated next ordinal. Starts at + # 1; deleting a repository never decrements it (RFC §4.3). + next_sequence: Mapped[int] = mapped_column(Integer, default=1) + created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=lambda: datetime.now(UTC)) + + __table_args__ = ( + # Composite ownership FK target for `repositories.lineage_id`. + UniqueConstraint("id", "owner_id", name="uq_repository_lineages_id_owner"), + CheckConstraint( + "(canonical_source_key IS NULL AND canonical_branch IS NULL) OR " + "(canonical_source_key IS NOT NULL AND canonical_branch IS NOT NULL)", + name="ck_repository_lineages_canonical_pair", + ), + CheckConstraint("next_sequence >= 1", name="ck_repository_lineages_next_sequence_positive"), + ForeignKeyConstraint( + ["owner_id"], + ["users.id"], + name="fk_repository_lineages_owner_id_users", + ondelete="CASCADE", + ), + # The owner-scoped canonical lookup index, and the sole source of + # "does this lineage already exist" truth if two imports race to + # create the first one (RFC §5.2). + Index( + "uq_repository_lineages_owner_source_branch", + "owner_id", + "canonical_source_key", + "canonical_branch", + unique=True, + postgresql_where=_CANONICAL_PARTIAL_WHERE, + sqlite_where=_CANONICAL_PARTIAL_WHERE, + ), + # The cyclic half of the lineage/repository integrity boundary (RFC + # §4.2): a latest-pointer can never name a repository outside this + # exact lineage, even if service code is wrong. Declared here (rather + # than only in the migration) so `create_all()` in development/test + # produces the identical final shape a migrated database reaches. + ForeignKeyConstraint( + ["latest_repository_id", "id"], + ["repositories.id", "repositories.lineage_id"], + name="fk_repository_lineages_latest_member", + deferrable=True, + initially="DEFERRED", + ), + ) diff --git a/apps/backend/app/repositories/repository_repository.py b/apps/backend/app/repositories/repository_repository.py index 17b09ef2..09060ea5 100644 --- a/apps/backend/app/repositories/repository_repository.py +++ b/apps/backend/app/repositories/repository_repository.py @@ -1,7 +1,31 @@ -from sqlalchemy import select +from datetime import UTC, datetime +from uuid import uuid4 + +from sqlalchemy import select, update +from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session from app.models.repository import RepositoryRecord +from app.models.repository_lineage import RepositoryLineage + +# Bound retries for two distinct races (#299, RFC-0002 §5.2): two importers +# racing to create the *first* lineage for a canonical key (one loses the +# unique index and must reload/retry), and a lineage-scoped duplicate-commit +# rollback. Five matches AiConversationRepository's own sequence-collision cap. +_MAX_LINEAGE_RETRIES = 5 + + +class LineageDuplicateRevision(Exception): + """The transactional insert found this exact commit already in the lineage. + + Carries the existing record so the caller can build the same 409 detail + (`repositoryId`, `name`) the pre-clone fast-path check already returns, + without a second query. + """ + + def __init__(self, existing: RepositoryRecord) -> None: + self.existing = existing + super().__init__("Repository revision already exists in this lineage.") class RepositoryRepository: @@ -36,26 +60,6 @@ def find_by_revision_for_owner(self, revision_value: str, owner_id: str) -> Repo ) return self.db.scalars(statement).first() - def find_by_source_revision_for_owner( - self, - source_url: str, - revision_value: str, - owner_id: str, - ) -> RepositoryRecord | None: - """Find the same GitHub source at the same immutable commit. - - A commit SHA alone is not a repository identity: forks can legitimately - share commits. GitHub duplicate detection is therefore scoped by source - URL as well as immutable revision, while a new commit at the same URL is - accepted as a new repository revision. - """ - statement = select(RepositoryRecord).where( - RepositoryRecord.source_url == source_url, - RepositoryRecord.revision_value == revision_value, - RepositoryRecord.owner_id == owner_id, - ) - return self.db.scalars(statement).first() - def add(self, record: RepositoryRecord) -> RepositoryRecord: self.db.add(record) self.db.commit() @@ -71,3 +75,146 @@ def save(self, record: RepositoryRecord) -> RepositoryRecord: def delete(self, record: RepositoryRecord) -> None: self.db.delete(record) self.db.commit() + + def delete_with_lineage_update(self, record: RepositoryRecord) -> None: + """Delete `record`, rolling its lineage's latest pointer back first if needed. + + A standalone record (``lineage_id is None``) deletes exactly as + ``delete()`` does. A lineaged record's deletion and any resulting + latest-pointer change happen in one transaction (#299 §8.3): the + counter never decreases, and an empty lineage is kept (null latest, + preserved ``next_sequence``) rather than removed. + """ + if record.lineage_id is not None: + lineage = self.db.execute( + select(RepositoryLineage).where(RepositoryLineage.id == record.lineage_id).with_for_update() + ).scalar_one() + if lineage.latest_repository_id == record.id: + next_latest = self.db.scalars( + select(RepositoryRecord.id) + .where( + RepositoryRecord.lineage_id == record.lineage_id, + RepositoryRecord.id != record.id, + ) + .order_by(RepositoryRecord.sequence.desc()) + .limit(1) + ).first() + lineage.latest_repository_id = next_latest + self.db.delete(record) + self.db.commit() + + def add_with_lineage( + self, + record: RepositoryRecord, + *, + owner_id: str, + canonical_source_key: str | None, + canonical_branch: str | None, + display_name: str, + ) -> RepositoryRecord: + """Insert `record`, allocating it into an owner-scoped lineage keyed by + the given canonical pair (#299, RFC-0002 §5.2). A null pair inserts a + standalone import with no lineage, identical to plain ``add()`` -- + uploads and unresolved-ref imports always take this branch. + + Lineage find-or-create, sequence allocation, the same-lineage + duplicate check, the repository insert, and the latest-pointer update + all happen in one transaction, so a partial allocation can never be + observed or committed. + + Raises ``LineageDuplicateRevision`` (not retried) if this exact commit + already exists in the target lineage. + """ + if canonical_source_key is None: + return self.add(record) + + last_error: IntegrityError | None = None + for _ in range(_MAX_LINEAGE_RETRIES): + try: + return self._add_with_lineage_once( + record, + owner_id=owner_id, + canonical_source_key=canonical_source_key, + canonical_branch=canonical_branch, + display_name=display_name, + ) + except IntegrityError as exc: + last_error = exc + self.db.rollback() + assert last_error is not None + raise last_error + + def _find_lineage( + self, owner_id: str, canonical_source_key: str, canonical_branch: str | None + ) -> RepositoryLineage | None: + statement = select(RepositoryLineage).where( + RepositoryLineage.owner_id == owner_id, + RepositoryLineage.canonical_source_key == canonical_source_key, + RepositoryLineage.canonical_branch == canonical_branch, + ) + return self.db.scalars(statement).first() + + def _add_with_lineage_once( + self, + record: RepositoryRecord, + *, + owner_id: str, + canonical_source_key: str, + canonical_branch: str | None, + display_name: str, + ) -> RepositoryRecord: + lineage = self._find_lineage(owner_id, canonical_source_key, canonical_branch) + if lineage is None: + lineage = RepositoryLineage( + id=str(uuid4()), + owner_id=owner_id, + canonical_source_key=canonical_source_key, + canonical_branch=canonical_branch, + display_name=display_name, + latest_repository_id=None, + next_sequence=1, + created_at=datetime.now(UTC), + ) + self.db.add(lineage) + # Flush (not commit) so a racing winner's canonical unique index + # raises IntegrityError here, before this transaction allocates a + # sequence for a lineage that turns out not to be the real one. + self.db.flush() + + # Atomic, race-free allocation (RFC §5.1): the write lock this UPDATE + # takes is held through commit on both dialects, so two concurrent + # allocations against the same lineage always serialize here rather + # than both reading the same `next_sequence`. + result = self.db.execute( + update(RepositoryLineage) + .where(RepositoryLineage.id == lineage.id, RepositoryLineage.owner_id == owner_id) + .values(next_sequence=RepositoryLineage.next_sequence + 1) + ) + if result.rowcount != 1: # type: ignore[attr-defined] + # Unreachable except as a defensive guard: the lineage was just + # selected or inserted in this same transaction and cannot vanish + # underneath it (deletion of an in-flight, uncommitted lineage is + # not possible from another connection). + raise RuntimeError("Repository lineage row vanished during sequence allocation.") + self.db.refresh(lineage) + allocated_sequence = lineage.next_sequence - 1 + + existing = self.db.scalars( + select(RepositoryRecord).where( + RepositoryRecord.lineage_id == lineage.id, + RepositoryRecord.revision_value == record.revision_value, + ) + ).first() + if existing is not None: + self.db.rollback() + raise LineageDuplicateRevision(existing) + + record.lineage_id = lineage.id + record.sequence = allocated_sequence + self.db.add(record) + self.db.flush() + + lineage.latest_repository_id = record.id + self.db.commit() + self.db.refresh(record) + return record diff --git a/apps/backend/app/services/repository_service.py b/apps/backend/app/services/repository_service.py index bc17afc1..c3036fc2 100644 --- a/apps/backend/app/services/repository_service.py +++ b/apps/backend/app/services/repository_service.py @@ -13,7 +13,7 @@ from app.github.client import GitHubClient from app.models.repository import RepositoryRecord from app.parsers.repository_parser import RepositoryFileLimitExceeded, RepositoryParser, UnsafeRepositoryPath -from app.repositories.repository_repository import RepositoryRepository +from app.repositories.repository_repository import LineageDuplicateRevision, RepositoryRepository from app.schemas.repository import ( FileTreeNode, GitHubImportRequest, @@ -69,8 +69,13 @@ def get_repository(self, repository_id: str) -> RepositoryResponse: def delete_repository(self, repository_id: str) -> None: record = self._get_record(repository_id) - self.storage.delete_repository(record.local_path) - self.repository.delete(record) + local_path = record.local_path + # DB transaction (including any lineage latest-pointer rollback, #299 + # §8.3) commits before the filesystem path is removed, so a DB + # failure can never leave a database row whose source directory has + # already vanished. + self.repository.delete_with_lineage_update(record) + self.storage.delete_repository(local_path) def import_github_repository(self, request: GitHubImportRequest) -> RepositoryResponse: repository_id = str(uuid4()) @@ -80,19 +85,18 @@ def import_github_repository(self, request: GitHubImportRequest) -> RepositoryRe # A new commit is a new revision, so duplicate detection is keyed on the # resolved commit SHA rather than URL+branch (#87). That requires cloning # first: URL+branch is only a fallback when git identity is unavailable, - # and it must never block importing a genuinely new revision. + # and it must never block importing a genuinely new revision. Duplicate + # detection itself happens later, transactionally, scoped to the + # resolved ref's lineage (#299 §5.2) -- not here, and deliberately not + # by (source_url, revision_value, owner) alone, since that would also + # reject the same commit legitimately re-imported under a different + # branch (#299 §8.1), which must succeed. destination = self.storage.reset_repository_path(repository_id) try: self.github.clone_public_repository(url, destination, branch) root = self._resolve_repository_root(destination) commit_sha = self.github.read_head_commit(destination) revision_kind, revision_value, revision_ref = self._git_revision(destination, commit_sha, branch) - existing = self.repository.find_by_source_revision_for_owner(url, revision_value, self.owner_id) - if existing: - raise ConflictServiceError( - "Repository has already been imported.", - {"repositoryId": existing.id, "name": existing.name}, - ) tree, meta, total_size = self._parse_repository(root) self._validate_parsed_repository(meta.total_files) except Exception: @@ -100,10 +104,11 @@ def import_github_repository(self, request: GitHubImportRequest) -> RepositoryRe raise now = datetime.now(UTC) + name = self.github.repository_name(url) record = RepositoryRecord( id=repository_id, owner_id=self.owner_id, - name=self.github.repository_name(url), + name=name, description=None, source="github", source_url=url, @@ -122,7 +127,42 @@ def import_github_repository(self, request: GitHubImportRequest) -> RepositoryRe repo_metadata=meta.model_dump(mode="json", by_alias=True), file_tree=[node.model_dump(mode="json", by_alias=True, exclude_none=True) for node in tree], ) - return self.to_response(self.repository.add(record)) + # A resolved ref is guaranteed here (`_git_revision` above raises + # otherwise), so every live GitHub import always gets a canonical + # pair and therefore a lineage (#299 §8.1) -- unlineaged standalone + # GitHub rows are only a backfill-time legacy case, never live. + try: + persisted = self.repository.add_with_lineage( + record, + owner_id=self.owner_id, + canonical_source_key=self._canonical_github_source(url), + canonical_branch=revision_ref, + display_name=name, + ) + except LineageDuplicateRevision as exc: + self.storage.delete_repository_id(repository_id) + raise ConflictServiceError( + "Repository has already been imported.", + {"repositoryId": exc.existing.id, "name": exc.existing.name}, + ) from exc + except Exception: + self.storage.delete_repository_id(repository_id) + raise + return self.to_response(persisted) + + def _canonical_github_source(self, url: str) -> str: + """Owner-scoped lineage grouping key for an already-validated live URL. + + `url` is already normalized by `GitHubClient.validate_public_url` to + exactly ``https://github.com//`` (no trailing slash or + ``.git``); only case-folding the owner/repo remains (#299 §8.1). This + is deliberately simpler than the migration's own backfill parser, + which must additionally accept looser historical forms -- the two are + intentionally not shared code, so a future change to this live parser + can never silently change what the frozen backfill migration does. + """ + owner, repo = url.removeprefix("https://github.com/").split("/", 1) + return f"github.com/{owner.lower()}/{repo.lower()}" async def import_uploaded_repository(self, file: UploadFile) -> RepositoryResponse: repository_id = str(uuid4()) @@ -151,6 +191,10 @@ async def import_uploaded_repository(self, file: UploadFile) -> RepositoryRespon self.storage.delete_upload(archive_path) now = datetime.now(UTC) + # No lineage_id/sequence set (both stay null): uploads are always + # unlineaged standalone imports (#299 §8.2/§4.3) -- nothing here + # proves two archives are revisions of the same logical repository, + # so this never creates or searches a lineage. record = RepositoryRecord( id=repository_id, owner_id=self.owner_id, diff --git a/apps/backend/scripts/rehearse_migrations.py b/apps/backend/scripts/rehearse_migrations.py index 4e54e3b7..a3fe3efc 100644 --- a/apps/backend/scripts/rehearse_migrations.py +++ b/apps/backend/scripts/rehearse_migrations.py @@ -30,7 +30,7 @@ if str(BACKEND_ROOT) not in sys.path: sys.path.insert(0, str(BACKEND_ROOT)) -HEAD_REVISION = "0012_waitlist_entries" +HEAD_REVISION = "0014_lineage_constraints" REPRESENTATIVE_BASELINE = "0004_ai_provider_configs" REQUIRED_HEAD_TABLES = { "users", @@ -41,6 +41,7 @@ "account_deletion_audits", "invite_tokens", "waitlist_entries", + "repository_lineages", } diff --git a/apps/backend/tests/test_repository_lineage_concurrency.py b/apps/backend/tests/test_repository_lineage_concurrency.py new file mode 100644 index 00000000..6a2f0d50 --- /dev/null +++ b/apps/backend/tests/test_repository_lineage_concurrency.py @@ -0,0 +1,456 @@ +"""Real-PostgreSQL concurrency and integrity coverage for #299 (RFC-0002). + +Per the migration plan's own §13/§14: "SQLite tests prove migration +portability, constraint reflection, and serialized-writer behavior. They +cannot prove PostgreSQL row-lock behavior, transaction isolation, +partial-index semantics, or concurrent create reconciliation." Every test in +this file runs against a real, separate-connection PostgreSQL database using +threading.Barrier synchronization -- not thread timing or sleeps -- matching +the existing `test_concurrent_refresh_on_postgres_mints_one_successor` +pattern in test_auth_concurrency.py. This whole file skips without +PARTHA_TEST_PG_URL. +""" + +import os +import threading +import uuid +from datetime import UTC, datetime + +import pytest +from sqlalchemy import create_engine, select +from sqlalchemy.exc import IntegrityError +from sqlalchemy.orm import sessionmaker + +PG_URL = os.environ.get("PARTHA_TEST_PG_URL") + +pytestmark = pytest.mark.skipif( + not PG_URL, reason="set PARTHA_TEST_PG_URL to run the Postgres lineage concurrency tests" +) + + +def _make_session_factory(): + from app.models.base import Base + + engine = create_engine(PG_URL) + Base.metadata.create_all(engine) + return engine, sessionmaker(bind=engine, expire_on_commit=False) + + +def _make_user(session, email: str | None = None) -> str: + from app.models.user import User + + user = User(id=str(uuid.uuid4()), email=email or f"lineage-{uuid.uuid4().hex}@example.com") + session.add(user) + session.commit() + return user.id + + +def _repository_record(owner_id: str, revision_value: str, **overrides) -> "RepositoryRecord": # noqa: F821 + from app.models.repository import RepositoryRecord + + base = dict( + id=str(uuid.uuid4()), + owner_id=owner_id, + name="widgets", + source="github", + source_url="https://github.com/acme/widgets", + branch="main", + revision_kind="git", + revision_value=revision_value, + revision_ref="refs/heads/main", + local_path="/tmp/x", + status="analysing", + ) + base.update(overrides) + return RepositoryRecord(**base) + + +def test_concurrent_imports_into_an_existing_lineage_get_unique_consecutive_sequences(): + """Two different commits, same canonical pair, an already-existing + lineage: both must serialize on the atomic counter update and receive + distinct, consecutive sequences -- never the same one.""" + from app.repositories.repository_repository import RepositoryRepository + + engine, Session = _make_session_factory() + setup = Session() + try: + owner_id = _make_user(setup) + finally: + setup.close() + + results: list[int] = [] + errors: list[BaseException] = [] + results_lock = threading.Lock() + start = threading.Barrier(2) + + def worker(revision_value: str) -> None: + session = Session() + try: + start.wait(timeout=10) + persisted = RepositoryRepository(session).add_with_lineage( + _repository_record(owner_id, revision_value), + owner_id=owner_id, + canonical_source_key="github.com/acme/widgets", + canonical_branch="refs/heads/main", + display_name="widgets", + ) + with results_lock: + results.append(persisted.sequence) + except BaseException as exc: # noqa: BLE001 -- captured for the assertion below, not swallowed + with results_lock: + errors.append(exc) + finally: + session.close() + + threads = [threading.Thread(target=worker, args=(sha,)) for sha in ("a" * 40, "b" * 40)] + for thread in threads: + thread.start() + for thread in threads: + thread.join(timeout=15) + + try: + assert not [t for t in threads if t.is_alive()], "a worker thread did not finish within the timeout" + assert not errors, errors + assert sorted(results) == [1, 2], results + + verify = Session() + try: + from app.models.repository_lineage import RepositoryLineage + + lineage = verify.scalars(select(RepositoryLineage).where(RepositoryLineage.owner_id == owner_id)).one() + assert lineage.next_sequence == 3 + finally: + verify.close() + finally: + _cleanup_owner(Session, owner_id) + engine.dispose() + + +def test_two_first_imports_racing_create_exactly_one_lineage(): + """No lineage exists yet for this canonical pair; two callers race to + create it. The canonical partial unique index must let exactly one win; + the loser reloads the winner's lineage and retries, ending with one + lineage and two distinct sequences -- never two lineages.""" + from app.repositories.repository_repository import RepositoryRepository + + engine, Session = _make_session_factory() + setup = Session() + try: + owner_id = _make_user(setup) + finally: + setup.close() + + results: list[int] = [] + errors: list[BaseException] = [] + results_lock = threading.Lock() + start = threading.Barrier(2) + + def worker(revision_value: str) -> None: + session = Session() + try: + start.wait(timeout=10) + persisted = RepositoryRepository(session).add_with_lineage( + _repository_record(owner_id, revision_value), + owner_id=owner_id, + canonical_source_key="github.com/acme/first-race", + canonical_branch="refs/heads/main", + display_name="first-race", + ) + with results_lock: + results.append(persisted.sequence) + except BaseException as exc: # noqa: BLE001 + with results_lock: + errors.append(exc) + finally: + session.close() + + threads = [threading.Thread(target=worker, args=(sha,)) for sha in ("c" * 40, "d" * 40)] + for thread in threads: + thread.start() + for thread in threads: + thread.join(timeout=15) + + try: + assert not [t for t in threads if t.is_alive()], "a worker thread did not finish within the timeout" + assert not errors, errors + assert sorted(results) == [1, 2], results + + verify = Session() + try: + from app.models.repository_lineage import RepositoryLineage + + lineages = verify.scalars( + select(RepositoryLineage).where( + RepositoryLineage.owner_id == owner_id, + RepositoryLineage.canonical_source_key == "github.com/acme/first-race", + ) + ).all() + assert len(lineages) == 1, f"expected exactly one lineage, found {len(lineages)}" + finally: + verify.close() + finally: + _cleanup_owner(Session, owner_id) + engine.dispose() + + +def test_two_identical_commits_racing_produce_one_repository_and_one_conflict(): + """The exact same commit imported twice, concurrently: the loser must + observe the duplicate after serialization and be rejected -- never a + second repository row, and never a wasted/skipped sequence number.""" + from app.repositories.repository_repository import LineageDuplicateRevision, RepositoryRepository + + engine, Session = _make_session_factory() + setup = Session() + try: + owner_id = _make_user(setup) + finally: + setup.close() + + outcomes: list[str] = [] + outcomes_lock = threading.Lock() + start = threading.Barrier(2) + shared_commit = "e" * 40 + + def worker() -> None: + session = Session() + outcome = "error" + try: + start.wait(timeout=10) + RepositoryRepository(session).add_with_lineage( + _repository_record(owner_id, shared_commit, id=str(uuid.uuid4())), + owner_id=owner_id, + canonical_source_key="github.com/acme/identical-race", + canonical_branch="refs/heads/main", + display_name="identical-race", + ) + outcome = "ok" + except LineageDuplicateRevision: + outcome = "rejected" + finally: + session.close() + with outcomes_lock: + outcomes.append(outcome) + + threads = [threading.Thread(target=worker) for _ in range(2)] + for thread in threads: + thread.start() + for thread in threads: + thread.join(timeout=15) + + try: + assert not [t for t in threads if t.is_alive()], "a worker thread did not finish within the timeout" + assert sorted(outcomes) == ["ok", "rejected"], outcomes + + verify = Session() + try: + from app.models.repository import RepositoryRecord + from app.models.repository_lineage import RepositoryLineage + + repos = verify.scalars( + select(RepositoryRecord).where( + RepositoryRecord.owner_id == owner_id, RepositoryRecord.revision_value == shared_commit + ) + ).all() + assert len(repos) == 1, "exactly one repository row must exist for the winning commit" + lineage = verify.scalars( + select(RepositoryLineage).where( + RepositoryLineage.owner_id == owner_id, + RepositoryLineage.canonical_source_key == "github.com/acme/identical-race", + ) + ).one() + assert lineage.next_sequence == 2, "the rejected duplicate must not have burned a sequence number" + finally: + verify.close() + finally: + _cleanup_owner(Session, owner_id) + engine.dispose() + + +def test_forced_duplicate_lineage_sequence_pair_is_rejected(): + """Direct proof of the `uq_repositories_lineage_sequence` constraint on + real Postgres, bypassing the allocator entirely.""" + from app.models.repository_lineage import RepositoryLineage + + engine, Session = _make_session_factory() + session = Session() + owner_id = None + try: + owner_id = _make_user(session) + lineage = RepositoryLineage( + id=str(uuid.uuid4()), + owner_id=owner_id, + canonical_source_key="github.com/acme/forced-dup", + canonical_branch="refs/heads/main", + display_name="forced-dup", + latest_repository_id=None, + next_sequence=2, + created_at=datetime.now(UTC), + ) + session.add(lineage) + session.add(_repository_record(owner_id, "f" * 40, lineage_id=lineage.id, sequence=1)) + session.commit() + + session.add(_repository_record(owner_id, "1" * 40, lineage_id=lineage.id, sequence=1)) + with pytest.raises(IntegrityError): + session.commit() + session.rollback() + finally: + session.close() + if owner_id: + _cleanup_owner(Session, owner_id) + engine.dispose() + + +def test_cross_owner_composite_membership_is_rejected_on_real_postgres(): + """Direct proof of `fk_repositories_lineage_owner` on real Postgres: a + repository can never attach to a lineage owned by a different user, even + with a forced direct write that bypasses the service layer.""" + from app.models.repository_lineage import RepositoryLineage + + engine, Session = _make_session_factory() + session = Session() + owner_id = None + other_owner_id = None + try: + owner_id = _make_user(session) + other_owner_id = _make_user(session) + lineage = RepositoryLineage( + id=str(uuid.uuid4()), + owner_id=other_owner_id, + canonical_source_key="github.com/acme/cross-owner", + canonical_branch="refs/heads/main", + display_name="cross-owner", + latest_repository_id=None, + next_sequence=1, + created_at=datetime.now(UTC), + ) + session.add(lineage) + session.commit() + + session.add(_repository_record(owner_id, "2" * 40, lineage_id=lineage.id, sequence=1)) + with pytest.raises(IntegrityError): + session.commit() + session.rollback() + finally: + session.close() + for cleanup_owner in (owner_id, other_owner_id): + if cleanup_owner: + _cleanup_owner(Session, cleanup_owner) + engine.dispose() + + +def test_cross_lineage_latest_pointer_is_rejected_on_real_postgres(): + """Direct proof of `fk_repository_lineages_latest_member` on real + Postgres: a lineage's latest pointer can never name a repository that + actually belongs to a different lineage.""" + from app.models.repository_lineage import RepositoryLineage + + engine, Session = _make_session_factory() + session = Session() + owner_id = None + try: + owner_id = _make_user(session) + lineage_a = RepositoryLineage( + id=str(uuid.uuid4()), + owner_id=owner_id, + canonical_source_key="github.com/acme/lineage-a", + canonical_branch="refs/heads/main", + display_name="lineage-a", + latest_repository_id=None, + next_sequence=2, + created_at=datetime.now(UTC), + ) + lineage_b = RepositoryLineage( + id=str(uuid.uuid4()), + owner_id=owner_id, + canonical_source_key="github.com/acme/lineage-b", + canonical_branch="refs/heads/main", + display_name="lineage-b", + latest_repository_id=None, + next_sequence=2, + created_at=datetime.now(UTC), + ) + session.add_all([lineage_a, lineage_b]) + record_in_b = _repository_record(owner_id, "3" * 40, lineage_id=lineage_b.id, sequence=1) + session.add(record_in_b) + session.commit() + + # Point lineage_a's latest at a repository that actually belongs to + # lineage_b -- must be rejected. + lineage_a.latest_repository_id = record_in_b.id + with pytest.raises(IntegrityError): + session.commit() + session.rollback() + finally: + session.close() + if owner_id: + _cleanup_owner(Session, owner_id) + engine.dispose() + + +def test_account_deletion_cascades_lineages_for_that_owner_only(): + from app.models.repository_lineage import RepositoryLineage + from app.models.user import User + + engine, Session = _make_session_factory() + session = Session() + owner_id = None + other_owner_id = None + try: + owner_id = _make_user(session) + other_owner_id = _make_user(session) + session.add( + RepositoryLineage( + id=str(uuid.uuid4()), + owner_id=owner_id, + canonical_source_key="github.com/acme/deleted-owner", + canonical_branch="refs/heads/main", + display_name="deleted-owner", + latest_repository_id=None, + next_sequence=1, + created_at=datetime.now(UTC), + ) + ) + other_lineage_id = str(uuid.uuid4()) + session.add( + RepositoryLineage( + id=other_lineage_id, + owner_id=other_owner_id, + canonical_source_key="github.com/acme/surviving-owner", + canonical_branch="refs/heads/main", + display_name="surviving-owner", + latest_repository_id=None, + next_sequence=1, + created_at=datetime.now(UTC), + ) + ) + session.commit() + + session.delete(session.get(User, owner_id)) + session.commit() + + remaining = session.scalars(select(RepositoryLineage.id)).all() + assert other_lineage_id in remaining + assert not any( + True for _ in session.scalars(select(RepositoryLineage).where(RepositoryLineage.owner_id == owner_id)) + ) + finally: + session.close() + if other_owner_id: + _cleanup_owner(Session, other_owner_id) + engine.dispose() + + +def _cleanup_owner(Session, owner_id: str) -> None: + from app.models.repository import RepositoryRecord + from app.models.repository_lineage import RepositoryLineage + from app.models.user import User + + cleanup = Session() + try: + cleanup.query(RepositoryRecord).filter(RepositoryRecord.owner_id == owner_id).delete() + cleanup.query(RepositoryLineage).filter(RepositoryLineage.owner_id == owner_id).delete() + cleanup.query(User).filter(User.id == owner_id).delete() + cleanup.commit() + finally: + cleanup.close() diff --git a/apps/backend/tests/test_repository_lineage_migration.py b/apps/backend/tests/test_repository_lineage_migration.py new file mode 100644 index 00000000..4436428f --- /dev/null +++ b/apps/backend/tests/test_repository_lineage_migration.py @@ -0,0 +1,455 @@ +"""Migration-level coverage for #299 (RFC-0002), revisions 0013/0014. + +Covers the plan's §13 "Migration tests" matrix: fresh empty DB shape, a +populated DB with the exact grouping/exclusion cases the backfill must get +right, deterministic tie-breaks, idempotent rerun, and full downgrade/ +upgrade round trips on both SQLite and (when ``PARTHA_TEST_PG_URL`` is set) +real PostgreSQL. +""" + +import importlib.util +import os +import uuid +from datetime import UTC, datetime, timedelta +from types import ModuleType + +import pytest +from alembic import command +from alembic.config import Config +from alembic.migration import MigrationContext +from alembic.operations import Operations +from sqlalchemy import MetaData, Table, create_engine, inspect, select +from sqlalchemy.engine import make_url + +from pathlib import Path + +BACKEND_ROOT = Path(__file__).resolve().parents[1] +PG_URL = os.environ.get("PARTHA_TEST_PG_URL") + + +def _alembic_config() -> Config: + cfg = Config(str(BACKEND_ROOT / "alembic.ini")) + cfg.set_main_option("script_location", str(BACKEND_ROOT / "alembic")) + return cfg + + +def _load_migration_module(revision_filename: str) -> ModuleType: + """Load a migration script's own module so its private helper functions + (e.g. ``_backfill_lineages``) can be called directly in a test, the same + way Alembic itself loads and executes it -- not via a package import, + since revision modules are not importable by their filename (it starts + with a digit) and are never meant to be imported by application code.""" + path = BACKEND_ROOT / "alembic" / "versions" / f"{revision_filename}.py" + spec = importlib.util.spec_from_file_location(revision_filename, path) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def _database_url(tmp_path) -> str: + if not PG_URL: + return f"sqlite:///{tmp_path / 'lineage-migration.db'}" + admin_url = make_url(PG_URL) + database_name = f"partha_lineage_migration_{uuid.uuid4().hex}" + admin_engine = create_engine(admin_url, isolation_level="AUTOCOMMIT") + try: + with admin_engine.connect() as connection: + connection.exec_driver_sql(f'CREATE DATABASE "{database_name}"') + finally: + admin_engine.dispose() + return admin_url.set(database=database_name).render_as_string(hide_password=False) + + +def _drop_pg_database(database_url: str) -> None: + if not PG_URL: + return + admin_url = make_url(PG_URL) + target = make_url(database_url) + admin_engine = create_engine(admin_url, isolation_level="AUTOCOMMIT") + try: + with admin_engine.connect() as connection: + connection.exec_driver_sql(f'DROP DATABASE IF EXISTS "{target.database}" WITH (FORCE)') + finally: + admin_engine.dispose() + + +@pytest.fixture() +def lineage_migration_db(tmp_path, monkeypatch): + # Import for its side effect: registers the `PRAGMA foreign_keys=ON` + # connect-event listener on the SQLAlchemy Engine class globally (see + # app/core/database.py). Without this import having already happened + # somewhere in the process, SQLite silently never enforces any foreign + # key at all -- these migration tests would then "pass" while proving + # nothing about the cyclic FK's actual behavior, exactly as happened + # once during development (caught only by incidental test-file + # ordering in a full-suite run, not by this file in isolation). Forcing + # it here makes that guarantee deterministic instead of accidental. + import app.core.database # noqa: F401 + + database_url = _database_url(tmp_path) + monkeypatch.setenv("DATABASE_URL", database_url) + monkeypatch.setenv("CORS_ORIGINS", "http://testserver") + from app.core import config + + config.get_settings.cache_clear() + engine = create_engine(database_url) + try: + yield database_url, engine + finally: + engine.dispose() + config.get_settings.cache_clear() + _drop_pg_database(database_url) + + +def test_fresh_database_reaches_head_with_the_expected_lineage_shape(lineage_migration_db): + database_url, engine = lineage_migration_db + cfg = _alembic_config() + + command.upgrade(cfg, "head") + + insp = inspect(engine) + assert "repository_lineages" in insp.get_table_names() + + lineage_columns = {column["name"] for column in insp.get_columns("repository_lineages")} + assert lineage_columns == { + "id", + "owner_id", + "canonical_source_key", + "canonical_branch", + "display_name", + "latest_repository_id", + "next_sequence", + "created_at", + } + + repo_columns = {column["name"] for column in insp.get_columns("repositories")} + assert {"lineage_id", "sequence"} <= repo_columns + + repo_fk_names = {fk["name"] for fk in insp.get_foreign_keys("repositories")} + assert "fk_repositories_lineage_owner" in repo_fk_names + lineage_fk_names = {fk["name"] for fk in insp.get_foreign_keys("repository_lineages")} + assert {"fk_repository_lineages_owner_id_users", "fk_repository_lineages_latest_member"} <= lineage_fk_names + + latest_member_fk = next( + fk + for fk in insp.get_foreign_keys("repository_lineages") + if fk["name"] == "fk_repository_lineages_latest_member" + ) + assert latest_member_fk["constrained_columns"] == ["latest_repository_id", "id"] + assert latest_member_fk["referred_columns"] == ["id", "lineage_id"] + assert latest_member_fk["options"].get("deferrable") is True + + repo_uk_names = {uk["name"] for uk in insp.get_unique_constraints("repositories")} + assert {"uq_repositories_lineage_sequence", "uq_repositories_id_lineage"} <= repo_uk_names + + index_names = {index["name"] for index in insp.get_indexes("repository_lineages")} + assert "uq_repository_lineages_owner_source_branch" in index_names + assert "ix_repository_lineages_owner_id" in index_names + + command.downgrade(cfg, "base") + assert "repository_lineages" not in inspect(engine).get_table_names() + + command.upgrade(cfg, "head") + assert "repository_lineages" in inspect(engine).get_table_names() + + +def _seed_users_and_repositories(engine, rows: list[dict]) -> tuple[str, str]: + """Insert two users and the given legacy-shaped repository rows at 0012, + before 0013/0014 exist. Returns (owner_a, owner_b).""" + meta = MetaData() + users = Table("users", meta, autoload_with=engine) + repositories = Table("repositories", meta, autoload_with=engine) + + owner_a = str(uuid.uuid4()) + owner_b = str(uuid.uuid4()) + now = datetime.now(UTC) + with engine.begin() as connection: + connection.execute( + users.insert(), + [ + { + "id": owner_a, + "email": f"a-{uuid.uuid4().hex}@example.com", + "is_active": True, + "created_at": now, + "updated_at": now, + "password_hash": None, + }, + { + "id": owner_b, + "email": f"b-{uuid.uuid4().hex}@example.com", + "is_active": True, + "created_at": now, + "updated_at": now, + "password_hash": None, + }, + ], + ) + if rows: + connection.execute(repositories.insert(), rows) + return owner_a, owner_b + + +def _repo_row(owner_id: str, **overrides) -> dict: + now = datetime.now(UTC) + base = dict( + id=str(uuid.uuid4()), + owner_id=owner_id, + name="demo", + description=None, + source="github", + source_url="https://github.com/acme/widgets", + branch="main", + local_path="/x", + size=0, + file_count=1, + status="completed", + analysis_stage=None, + analysis_progress=100, + uploaded_at=now, + analysed_at=now, + error_message=None, + repo_metadata=None, + file_tree=[], + created_at=now, + updated_at=now, + revision_kind="git", + revision_value="a" * 40, + revision_ref="refs/heads/main", + ) + base.update(overrides) + return base + + +def test_backfill_groups_correctly_and_leaves_ineligible_rows_standalone(lineage_migration_db): + """One populated-DB test exercising every §6 grouping/exclusion case at + once: same source/ref groups; different ref, different repo, and + different owner each get a separate lineage; a URL variant (mixed-case + host, .git suffix, trailing slash) still canonicalizes into the same + lineage; a malformed URL, an unresolved ref, and an upload all stay + standalone; and a deterministic timestamp tie breaks on repository id. + """ + database_url, engine = lineage_migration_db + cfg = _alembic_config() + command.upgrade(cfg, "0012_waitlist_entries") + + now = datetime.now(UTC) + owner_a, owner_b = _seed_users_and_repositories(engine, []) + + meta = MetaData() + repositories = Table("repositories", meta, autoload_with=engine) + + group_first = str(uuid.uuid4()) + group_second = str(uuid.uuid4()) + variant_row_id = str(uuid.uuid4()) + dev_row_id = str(uuid.uuid4()) + other_owner_row_id = str(uuid.uuid4()) + malformed_row_id = str(uuid.uuid4()) + unresolved_row_id = str(uuid.uuid4()) + upload_row_id = str(uuid.uuid4()) + tie_a = "20000000-0000-0000-0000-000000000001" + tie_b = "20000000-0000-0000-0000-000000000002" + + rows = [ + _repo_row( + owner_a, + id=group_first, + revision_value="a" * 40, + created_at=now - timedelta(days=2), + ), + _repo_row( + owner_a, + id=group_second, + revision_value="b" * 40, + created_at=now - timedelta(days=1), + ), + _repo_row( + owner_a, + id=variant_row_id, + source_url="https://GitHub.com/Acme/Widgets.git/", + revision_value="c" * 40, + created_at=now, + ), + _repo_row(owner_a, id=dev_row_id, revision_ref="refs/heads/dev", revision_value="d" * 40, created_at=now), + _repo_row(owner_b, id=other_owner_row_id, revision_value="e" * 40, created_at=now), + _repo_row(owner_a, id=malformed_row_id, source_url="not a url at all", revision_value="f" * 40, created_at=now), + _repo_row( + owner_a, + id=unresolved_row_id, + source_url="https://github.com/acme/other", + revision_ref=None, + revision_value="1" * 40, + created_at=now, + ), + dict( + id=upload_row_id, + owner_id=owner_a, + name="upload1", + description=None, + source="upload", + source_url=None, + branch=None, + local_path="/y", + size=0, + file_count=1, + status="completed", + analysis_stage=None, + analysis_progress=100, + uploaded_at=now, + analysed_at=now, + error_message=None, + repo_metadata=None, + file_tree=[], + created_at=now, + updated_at=now, + revision_kind="upload", + revision_value="sha256:" + "0" * 64, + revision_ref=None, + ), + _repo_row( + owner_a, + id=tie_a, + source_url="https://github.com/acme/tied", + revision_value="2" * 40, + created_at=now, + ), + _repo_row( + owner_a, + id=tie_b, + source_url="https://github.com/acme/tied", + revision_value="3" * 40, + created_at=now, + ), + ] + with engine.begin() as connection: + connection.execute(repositories.insert(), rows) + + command.upgrade(cfg, "head") + + meta2 = MetaData() + repos2 = Table("repositories", meta2, autoload_with=engine) + lineages2 = Table("repository_lineages", meta2, autoload_with=engine) + + with engine.connect() as connection: + attached = { + row.id: (row.lineage_id, row.sequence) + for row in connection.execute(select(repos2.c.id, repos2.c.lineage_id, repos2.c.sequence)) + } + + # The primary group: two original commits plus the URL-variant row, + # in creation order. + assert attached[group_first][1] == 1 + assert attached[group_second][1] == 2 + assert attached[variant_row_id][1] == 3 + primary_lineage = attached[group_first][0] + assert attached[group_second][0] == primary_lineage + assert attached[variant_row_id][0] == primary_lineage + + # A different ref is a different lineage. + assert attached[dev_row_id][0] != primary_lineage + assert attached[dev_row_id][1] == 1 + + # A different owner is a different lineage, even for the same + # canonical source/ref. + assert attached[other_owner_row_id][0] != primary_lineage + assert attached[other_owner_row_id][1] == 1 + + # Ineligible rows stay standalone. + assert attached[malformed_row_id] == (None, None) + assert attached[unresolved_row_id] == (None, None) + assert attached[upload_row_id] == (None, None) + + # Deterministic timestamp tie-break: identical created_at, so the + # lexicographically smaller id (tie_a) gets sequence 1. + assert attached[tie_a][1] == 1 + assert attached[tie_b][1] == 2 + assert attached[tie_a][0] == attached[tie_b][0] + + primary_row = ( + connection.execute( + select( + lineages2.c.owner_id, + lineages2.c.canonical_source_key, + lineages2.c.canonical_branch, + lineages2.c.display_name, + lineages2.c.latest_repository_id, + lineages2.c.next_sequence, + ).where(lineages2.c.id == primary_lineage) + ) + .mappings() + .one() + ) + assert primary_row["owner_id"] == owner_a + assert primary_row["canonical_source_key"] == "github.com/acme/widgets" + assert primary_row["canonical_branch"] == "refs/heads/main" + assert primary_row["display_name"] == "demo" + assert primary_row["latest_repository_id"] == variant_row_id + assert primary_row["next_sequence"] == 4 + + # No stray lineage attachment anywhere else: exactly the 7 eligible + # rows (group_first, group_second, variant, dev, other_owner, tie_a, + # tie_b) carry a lineage, and every (lineage_id, sequence) pair is + # unique. + attachments = [ + (row.lineage_id, row.sequence) + for row in connection.execute(select(repos2.c.lineage_id, repos2.c.sequence)) + if row.lineage_id is not None + ] + assert len(attachments) == 7 + assert len(set(attachments)) == 7 + + command.downgrade(cfg, "base") + command.upgrade(cfg, "head") + + +def test_backfill_rerun_is_idempotent(lineage_migration_db): + """Calling the backfill helper twice against the same already-backfilled + data reconciles to the identical final state rather than erroring or + double-counting (plan §6.2/§7 "interruption and rerun").""" + database_url, engine = lineage_migration_db + cfg = _alembic_config() + command.upgrade(cfg, "0012_waitlist_entries") + + owner_a, _owner_b = _seed_users_and_repositories(engine, []) + meta = MetaData() + repositories = Table("repositories", meta, autoload_with=engine) + now = datetime.now(UTC) + first_id, second_id = str(uuid.uuid4()), str(uuid.uuid4()) + with engine.begin() as connection: + connection.execute( + repositories.insert(), + [ + _repo_row(owner_a, id=first_id, revision_value="a" * 40, created_at=now - timedelta(days=1)), + _repo_row(owner_a, id=second_id, revision_value="b" * 40, created_at=now), + ], + ) + + command.upgrade(cfg, "0013_lineage_expand") + + migration = _load_migration_module("0013_lineage_expand") + + with engine.connect() as connection: + migration_context = MigrationContext.configure(connection) + with connection.begin(), Operations.context(migration_context): + groups_first = migration._backfill_lineages() + migration._verify_backfill(groups_first) + groups_second = migration._backfill_lineages() + migration._verify_backfill(groups_second) + + meta2 = MetaData() + repos2 = Table("repositories", meta2, autoload_with=engine) + lineages2 = Table("repository_lineages", meta2, autoload_with=engine) + with engine.connect() as connection: + lineage_rows = connection.execute(select(lineages2.c.id, lineages2.c.next_sequence)).all() + assert len(lineage_rows) == 1 + assert lineage_rows[0].next_sequence == 3 + + sequences = sorted( + row.sequence + for row in connection.execute(select(repos2.c.sequence).where(repos2.c.id.in_([first_id, second_id]))) + ) + assert sequences == [1, 2] + + command.upgrade(cfg, "head") + command.downgrade(cfg, "base") diff --git a/apps/backend/tests/test_repository_lineage_service.py b/apps/backend/tests/test_repository_lineage_service.py new file mode 100644 index 00000000..81a5ccb1 --- /dev/null +++ b/apps/backend/tests/test_repository_lineage_service.py @@ -0,0 +1,397 @@ +"""Service-level coverage for #299 (RFC-0002): live import/delete lineage +allocation, ownership, and the cyclic FK's real enforcement. + +HTTP-level tests exercise the actual `/repositories/github` and +`/repositories/upload` routes with a faked git clone (same idiom as +test_ingestion_pipeline.py), so lineage assignment is proven end to end, not +just at the repository-layer unit. Repository-layer tests exercise +`RepositoryRepository.add_with_lineage`/`delete_with_lineage_update` +directly for cases an HTTP request can't force (a same-lineage race, a +deliberately-forced FK violation). +""" + +import uuid +from datetime import UTC, datetime +from pathlib import Path + +import pytest + +from app.core.database import SessionLocal +from app.github.client import GitHubClient +from app.models.repository import RepositoryRecord +from app.models.repository_lineage import RepositoryLineage +from app.repositories.repository_repository import RepositoryRepository +from tests.api_assertions import assert_error_response + + +def _fake_clone(_: GitHubClient, __: str, destination: Path, ___: str | None = None) -> None: + destination.mkdir(parents=True, exist_ok=True) + (destination / "README.md").write_text("# demo\n", encoding="utf-8") + + +def _mock_github(monkeypatch: pytest.MonkeyPatch, commits: list[str], ref: str = "refs/heads/main") -> None: + commit_iter = iter(commits) + monkeypatch.setattr(GitHubClient, "clone_public_repository", _fake_clone) + monkeypatch.setattr(GitHubClient, "read_head_commit", lambda *_: next(commit_iter)) + monkeypatch.setattr(GitHubClient, "read_head_ref", lambda *_: ref) + + +def _lineage_of(response_json: dict) -> tuple[str | None, int | None]: + with SessionLocal() as db: + record = db.get(RepositoryRecord, response_json["id"]) + assert record is not None + return record.lineage_id, record.sequence + + +# -------------------------------------------------------------------------- +# Live import: lineage assignment +# -------------------------------------------------------------------------- + + +def test_first_github_import_creates_a_lineage_with_sequence_one(auth_client, monkeypatch: pytest.MonkeyPatch): + _mock_github(monkeypatch, ["a" * 40]) + + response = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + + assert response.status_code == 201, response.text + lineage_id, sequence = _lineage_of(response.json()) + assert lineage_id is not None + assert sequence == 1 + with SessionLocal() as db: + lineage = db.get(RepositoryLineage, lineage_id) + assert lineage is not None + assert lineage.canonical_source_key == "github.com/acme/widgets" + assert lineage.canonical_branch == "refs/heads/main" + assert lineage.display_name == "widgets" + assert lineage.latest_repository_id == response.json()["id"] + assert lineage.next_sequence == 2 + + +def test_second_commit_on_same_source_and_ref_reuses_the_lineage_and_increments( + auth_client, monkeypatch: pytest.MonkeyPatch +): + _mock_github(monkeypatch, ["a" * 40, "b" * 40]) + + first = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + second = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + + assert first.status_code == 201 and second.status_code == 201 + first_lineage, first_sequence = _lineage_of(first.json()) + second_lineage, second_sequence = _lineage_of(second.json()) + assert first_lineage == second_lineage + assert (first_sequence, second_sequence) == (1, 2) + with SessionLocal() as db: + lineage = db.get(RepositoryLineage, first_lineage) + assert lineage.latest_repository_id == second.json()["id"] + assert lineage.next_sequence == 3 + + +def test_owner_repo_case_variants_of_the_same_url_match_the_same_lineage(auth_client, monkeypatch: pytest.MonkeyPatch): + """Live validation allows mixed-case owner/repo (only the host must be + exact-case), so two spellings of the same repository must still land in + the same lineage once case-folded (#299 §8.1).""" + _mock_github(monkeypatch, ["a" * 40, "b" * 40]) + + first = auth_client.post("/repositories/github", json={"url": "https://github.com/Acme/Widgets"}) + second = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + + assert first.status_code == 201 and second.status_code == 201 + first_lineage, _ = _lineage_of(first.json()) + second_lineage, second_sequence = _lineage_of(second.json()) + assert first_lineage == second_lineage + assert second_sequence == 2 + + +def test_different_ref_gets_a_different_lineage(auth_client, monkeypatch: pytest.MonkeyPatch): + _mock_github(monkeypatch, ["a" * 40], ref="refs/heads/main") + main_response = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + _mock_github(monkeypatch, ["b" * 40], ref="refs/heads/dev") + dev_response = auth_client.post( + "/repositories/github", json={"url": "https://github.com/acme/widgets", "branch": "dev"} + ) + + assert main_response.status_code == 201 and dev_response.status_code == 201 + main_lineage, _ = _lineage_of(main_response.json()) + dev_lineage, dev_sequence = _lineage_of(dev_response.json()) + assert main_lineage != dev_lineage + assert dev_sequence == 1 + + +def test_different_repository_gets_a_different_lineage(auth_client, monkeypatch: pytest.MonkeyPatch): + _mock_github(monkeypatch, ["a" * 40, "b" * 40]) + + widgets = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + gadgets = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/gadgets"}) + + assert widgets.status_code == 201 and gadgets.status_code == 201 + widgets_lineage, _ = _lineage_of(widgets.json()) + gadgets_lineage, gadgets_sequence = _lineage_of(gadgets.json()) + assert widgets_lineage != gadgets_lineage + assert gadgets_sequence == 1 + + +def test_different_owner_gets_a_different_lineage_even_for_the_same_source_and_ref( + auth_client, make_auth_headers, monkeypatch: pytest.MonkeyPatch +): + _mock_github(monkeypatch, ["a" * 40]) + primary = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + assert primary.status_code == 201 + + other = make_auth_headers("other-owner@example.com") + _mock_github(monkeypatch, ["a" * 40]) + other_response = auth_client.post( + "/repositories/github", json={"url": "https://github.com/acme/widgets"}, headers=other["headers"] + ) + + assert other_response.status_code == 201 + primary_lineage, _ = _lineage_of(primary.json()) + other_lineage, other_sequence = _lineage_of(other_response.json()) + assert primary_lineage != other_lineage + assert other_sequence == 1 + with SessionLocal() as db: + assert db.get(RepositoryLineage, other_lineage).owner_id == other["user"]["id"] + + +def test_same_commit_is_allowed_in_two_different_branch_lineages(auth_client, monkeypatch: pytest.MonkeyPatch): + shared_commit = "c" * 40 + _mock_github(monkeypatch, [shared_commit], ref="refs/heads/main") + main_response = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + _mock_github(monkeypatch, [shared_commit], ref="refs/heads/release") + release_response = auth_client.post( + "/repositories/github", json={"url": "https://github.com/acme/widgets", "branch": "release"} + ) + + assert main_response.status_code == 201 + assert release_response.status_code == 201 + assert main_response.json()["revision"]["value"] == shared_commit + assert release_response.json()["revision"]["value"] == shared_commit + main_lineage, _ = _lineage_of(main_response.json()) + release_lineage, _ = _lineage_of(release_response.json()) + assert main_lineage != release_lineage + + +def test_the_same_commit_twice_in_one_lineage_is_still_rejected_as_a_duplicate( + auth_client, monkeypatch: pytest.MonkeyPatch +): + """The pre-existing 409 behaviour must survive #299's rewrite of the + persistence path -- this is the authoritative, transactional duplicate + check now, not the old owner+source_url pre-clone check.""" + _mock_github(monkeypatch, ["a" * 40, "a" * 40]) + + first = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + duplicate = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + + assert first.status_code == 201 + error = assert_error_response(duplicate, 409, "conflict_error") + assert error.details == {"repositoryId": first.json()["id"], "name": first.json()["name"]} + + with SessionLocal() as db: + lineage_id, _ = _lineage_of(first.json()) + lineage = db.get(RepositoryLineage, lineage_id) + # The rejected duplicate must not have burned a sequence number. + assert lineage.next_sequence == 2 + + +def test_upload_never_creates_or_touches_a_lineage(auth_client): + response = auth_client.post( + "/repositories/upload", + files={"file": ("demo.zip", _minimal_zip(), "application/octet-stream")}, + ) + + assert response.status_code == 201, response.text + lineage_id, sequence = _lineage_of(response.json()) + assert (lineage_id, sequence) == (None, None) + + +def _minimal_zip() -> bytes: + import io + import zipfile + + buffer = io.BytesIO() + with zipfile.ZipFile(buffer, "w") as archive: + archive.writestr("README.md", "# demo\n") + return buffer.getvalue() + + +def test_a_failed_lineage_duplicate_insert_cleans_up_the_staged_repository_directory( + auth_client, monkeypatch: pytest.MonkeyPatch +): + """#299 §5.3 extends the existing pre-clone cleanup across the + transactional insert phase: a rejected duplicate must not leave an + orphaned directory under storage.""" + _mock_github(monkeypatch, ["a" * 40, "a" * 40]) + + first = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + assert first.status_code == 201 + + from app.core.config import get_settings + from app.storage.local import LocalStorage + + storage = LocalStorage(get_settings()) + before = set(storage.repositories_root.iterdir()) if storage.repositories_root.exists() else set() + + duplicate = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + assert duplicate.status_code == 409 + + after = set(storage.repositories_root.iterdir()) if storage.repositories_root.exists() else set() + assert after == before, "the duplicate's staged directory must be removed, not left behind" + + +# -------------------------------------------------------------------------- +# Deletion: latest-pointer rollback +# -------------------------------------------------------------------------- + + +def test_deleting_a_non_latest_member_leaves_the_latest_pointer_unchanged(auth_client, monkeypatch: pytest.MonkeyPatch): + _mock_github(monkeypatch, ["a" * 40, "b" * 40, "c" * 40]) + auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + second = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + third = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + lineage_id, _ = _lineage_of(third.json()) + + delete_response = auth_client.delete(f"/repositories/{second.json()['id']}") + assert delete_response.status_code == 204 + + with SessionLocal() as db: + lineage = db.get(RepositoryLineage, lineage_id) + assert lineage.latest_repository_id == third.json()["id"] + assert lineage.next_sequence == 4 # counter never decreases + + +def test_deleting_the_latest_member_rolls_back_to_the_next_highest_surviving_sequence( + auth_client, monkeypatch: pytest.MonkeyPatch +): + _mock_github(monkeypatch, ["a" * 40, "b" * 40, "c" * 40]) + first = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + second = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + third = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + lineage_id, _ = _lineage_of(first.json()) + + delete_response = auth_client.delete(f"/repositories/{third.json()['id']}") + assert delete_response.status_code == 204 + + with SessionLocal() as db: + lineage = db.get(RepositoryLineage, lineage_id) + assert lineage.latest_repository_id == second.json()["id"] + assert lineage.next_sequence == 4 + + +def test_deleting_the_last_member_keeps_an_empty_lineage_and_never_reuses_its_sequence( + auth_client, monkeypatch: pytest.MonkeyPatch +): + _mock_github(monkeypatch, ["a" * 40]) + only = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + lineage_id, _ = _lineage_of(only.json()) + + delete_response = auth_client.delete(f"/repositories/{only.json()['id']}") + assert delete_response.status_code == 204 + + with SessionLocal() as db: + lineage = db.get(RepositoryLineage, lineage_id) + assert lineage is not None, "an empty lineage is kept, not garbage collected" + assert lineage.latest_repository_id is None + assert lineage.next_sequence == 2 + + _mock_github(monkeypatch, ["b" * 40]) + reimport = auth_client.post("/repositories/github", json={"url": "https://github.com/acme/widgets"}) + assert reimport.status_code == 201 + reimport_lineage, reimport_sequence = _lineage_of(reimport.json()) + assert reimport_lineage == lineage_id + assert reimport_sequence == 2 # sequence 1 is never reused + + +# -------------------------------------------------------------------------- +# Cross-owner isolation +# -------------------------------------------------------------------------- + + +def test_cross_owner_lineage_lookup_never_matches_another_owners_row(auth_client, make_auth_headers): + """A pre-existing lineage owned by another user, with the exact same + canonical key a fresh import will compute, must never be reused -- + proven through the real allocation path (`add_with_lineage`), not by + asserting a private lookup helper's return value in isolation.""" + other = make_auth_headers("owner-b@example.com") + with SessionLocal() as db: + other_lineage = RepositoryLineage( + id=str(uuid.uuid4()), + owner_id=other["user"]["id"], + canonical_source_key="github.com/shared/repo", + canonical_branch="refs/heads/main", + display_name="repo", + latest_repository_id=None, + next_sequence=1, + created_at=datetime.now(UTC), + ) + db.add(other_lineage) + db.commit() + other_lineage_id = other_lineage.id + + with SessionLocal() as db: + repository_repo = RepositoryRepository(db) + record = RepositoryRecord( + id=str(uuid.uuid4()), + owner_id=auth_client.default_user["id"], # type: ignore[attr-defined] + name="repo", + source="github", + source_url="https://github.com/shared/repo", + branch="main", + revision_kind="git", + revision_value="e" * 40, + revision_ref="refs/heads/main", + local_path="/tmp/x", + status="analysing", + ) + persisted = repository_repo.add_with_lineage( + record, + owner_id=auth_client.default_user["id"], # type: ignore[attr-defined] + canonical_source_key="github.com/shared/repo", + canonical_branch="refs/heads/main", + display_name="repo", + ) + assert persisted.lineage_id != other_lineage_id + assert persisted.sequence == 1 + + +def test_cross_owner_lineage_attachment_is_rejected_by_the_database_even_if_forced(auth_client, make_auth_headers): + """The composite deferred FK enforces ownership even if application code + is wrong (#299 §9) -- proven here by deliberately bypassing the service + layer and trying to attach a repository to another owner's lineage + directly.""" + other = make_auth_headers("owner-c@example.com") + with SessionLocal() as db: + lineage = RepositoryLineage( + id=str(uuid.uuid4()), + owner_id=other["user"]["id"], + canonical_source_key="github.com/other/repo2", + canonical_branch="refs/heads/main", + display_name="repo2", + latest_repository_id=None, + next_sequence=1, + created_at=datetime.now(UTC), + ) + db.add(lineage) + db.commit() + lineage_id = lineage.id + + from sqlalchemy.exc import IntegrityError + + with SessionLocal() as db: + record = RepositoryRecord( + id=str(uuid.uuid4()), + owner_id=auth_client.default_user["id"], # type: ignore[attr-defined] + name="cross-owner-attempt", + source="github", + source_url="https://github.com/other/repo2", + branch="main", + revision_kind="git", + revision_value="d" * 40, + revision_ref="refs/heads/main", + local_path="/tmp/x", + status="analysing", + lineage_id=lineage_id, + sequence=1, + ) + db.add(record) + with pytest.raises(IntegrityError): + db.commit() + db.rollback() diff --git a/docs/architecture/REPOSITORY_LINEAGE_MIGRATION_PLAN.md b/docs/architecture/REPOSITORY_LINEAGE_MIGRATION_PLAN.md index d6d1e351..6686c8d1 100644 --- a/docs/architecture/REPOSITORY_LINEAGE_MIGRATION_PLAN.md +++ b/docs/architecture/REPOSITORY_LINEAGE_MIGRATION_PLAN.md @@ -402,9 +402,11 @@ Do not silently skip a failed update or coerce corrupt data to satisfy final con ## 7. Concrete Alembic migration sequence -Use two new revisions after `0010_account_deletion`, each with an ID under the existing PostgreSQL -`alembic_version VARCHAR(32)` limit. Suggested IDs are `0011_lineage_expand` and -`0012_lineage_constraints`. +Use two new revisions after the current head, each with an ID under the existing PostgreSQL +`alembic_version VARCHAR(32)` limit. Suggested IDs were `0011_lineage_expand` and +`0012_lineage_constraints`; two unrelated migrations (`0011_invite_tokens`, `0012_waitlist_entries`) +landed on `dev` ahead of this one, so the implementation uses `0013_lineage_expand` and +`0014_lineage_constraints` instead. The mechanics below are unaffected by the renumbering. Imports must be quiesced while the migrations run. The application currently has no dual-write compatibility for lineage and a concurrent repository insert could escape the backfill. Existing diff --git a/docs/operations/DATABASE_MIGRATION_REHEARSAL.md b/docs/operations/DATABASE_MIGRATION_REHEARSAL.md index 3b9dc6f3..8b9cfb4c 100644 --- a/docs/operations/DATABASE_MIGRATION_REHEARSAL.md +++ b/docs/operations/DATABASE_MIGRATION_REHEARSAL.md @@ -7,7 +7,7 @@ It documents a reproducible rehearsal, not a promise that all historical data ca ## Current support and evidence PARTHA uses one linear Alembic chain, from `0001_initial` through current head -`0010_account_deletion`. SQLite is the local-development default and is the local +`0014_lineage_constraints`. SQLite is the local-development default and is the local maintainer rehearsal target. PostgreSQL is the supported deployment and CI dialect: the Backend CI job runs this rehearsal command against its isolated PostgreSQL 16 service after the backend tests. Repository files are outside the database, under `STORAGE_PATH`; a database restore does not recreate missing repository From da7168febea2d6542a9a1ea434bdf627d7c16616 Mon Sep 17 00:00:00 2001 From: PARTH J ROHIT Date: Fri, 28 Aug 2026 19:16:04 +0100 Subject: [PATCH 2/4] test(backend): stop depending on ambient FK-pragma listener timing (#299) CI's Backend job failed on test_cross_owner_lineage_attachment_is_rejected_ by_the_database_even_if_forced ("DID NOT RAISE IntegrityError") on Linux; it passed reliably every time locally (isolated, full suite, and with CI's exact pytest/coverage command reproduced) on macOS. Not reproducible locally, so rather than guess at the platform difference, this removes the test's dependency on app.core.database's process-wide connect-event listener having already fired for this exact connection -- it now sets `PRAGMA foreign_keys=ON` explicitly, as the first statement on its own session, before asserting the deferred FK rejects the forced cross-owner attachment at commit. This does not touch application or migration code, and does not change what's being verified: only PostgreSQL (not SQLite) is the documented deployment/CI dialect, and the equivalent real-Postgres proof (test_cross_owner_composite_membership_is_rejected_on_real_postgres in test_repository_lineage_concurrency.py) already passes reliably across repeated runs. This is strictly a local-SQLite-dev-path test reliability fix. --- apps/backend/tests/test_repository_lineage_service.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/apps/backend/tests/test_repository_lineage_service.py b/apps/backend/tests/test_repository_lineage_service.py index 81a5ccb1..4f3e8498 100644 --- a/apps/backend/tests/test_repository_lineage_service.py +++ b/apps/backend/tests/test_repository_lineage_service.py @@ -373,9 +373,19 @@ def test_cross_owner_lineage_attachment_is_rejected_by_the_database_even_if_forc db.commit() lineage_id = lineage.id + from sqlalchemy import text from sqlalchemy.exc import IntegrityError with SessionLocal() as db: + # Force enforcement on this exact connection rather than relying on + # app.core.database's process-wide connect-event listener having + # already fired (it normally has, by this point in a real app or a + # full test run, but that's an accident of import order/platform, + # not something this specific assertion should depend on -- must be + # the very first statement on the connection, before SQLite's + # autobegin opens a transaction, since the pragma is a no-op once + # one is open). + db.execute(text("PRAGMA foreign_keys=ON")) record = RepositoryRecord( id=str(uuid.uuid4()), owner_id=auth_client.default_user["id"], # type: ignore[attr-defined] From 3a301b405250f6be5f02611fd72a6aabd186f044 Mon Sep 17 00:00:00 2001 From: PARTH J ROHIT Date: Fri, 28 Aug 2026 21:37:10 +0100 Subject: [PATCH 3/4] test(backend): verify the cross-owner FK with foreign_key_check, not commit (#299) The previous fix (explicitly setting PRAGMA foreign_keys=ON on this exact connection) did not resolve the CI failure: confirmed from the actual CI log that the fix's code ran, the pragma read back as intended, and COMMIT still did not raise IntegrityError on the Linux CI runner -- while the identical scenario raises reliably every time on macOS. This is a genuine SQLite deferred-foreign-key enforcement difference between platforms/ SQLite builds, not a test-ordering or pragma-timing issue. Rather than keep guessing at platform-specific COMMIT-time deferred enforcement, this now verifies the same underlying fact a different way: PRAGMA foreign_key_check, which SQLite documents as detecting a violation regardless of the connection's foreign_keys/deferred state. The row is flushed (visible within the still-open transaction) and foreign_key_check is asserted to find it, instead of relying on commit to raise. Also asserts the pragma reads back as enabled, so if this ever silently stops being set at all, the test fails on that explicitly rather than on an unrelated missing violation. This still proves the fact this test exists to prove -- a repository row attached to another owner's lineage is a genuine constraint violation -- without depending on whichever platform-specific commit-time behavior was making the previous assertion unreliable in CI. The equivalent real- Postgres proof (test_cross_owner_composite_membership_is_rejected_on_real_postgres) is unaffected and continues to pass reliably. --- .../tests/test_repository_lineage_service.py | 34 ++++++++++++++----- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/apps/backend/tests/test_repository_lineage_service.py b/apps/backend/tests/test_repository_lineage_service.py index 4f3e8498..5ff8d530 100644 --- a/apps/backend/tests/test_repository_lineage_service.py +++ b/apps/backend/tests/test_repository_lineage_service.py @@ -374,18 +374,29 @@ def test_cross_owner_lineage_attachment_is_rejected_by_the_database_even_if_forc lineage_id = lineage.id from sqlalchemy import text - from sqlalchemy.exc import IntegrityError with SessionLocal() as db: # Force enforcement on this exact connection rather than relying on # app.core.database's process-wide connect-event listener having - # already fired (it normally has, by this point in a real app or a - # full test run, but that's an accident of import order/platform, - # not something this specific assertion should depend on -- must be - # the very first statement on the connection, before SQLite's - # autobegin opens a transaction, since the pragma is a no-op once - # one is open). + # already fired -- must be the very first statement on the + # connection, before SQLite's autobegin opens a transaction, since + # the pragma is a no-op once one is open. Confirmed (#299 follow-up): + # explicitly forcing this pragma was NOT sufficient to make SQLite + # actually enforce the deferred composite FK on the Linux CI runner, + # even though it reads back as ON and the identical scenario raises + # reliably on macOS -- a genuine platform/SQLite-build difference in + # deferred FK support, not a test-setup ordering issue. This + # assertion therefore uses `PRAGMA foreign_key_check`, which is + # documented to detect a violation regardless of the connection's + # `foreign_keys`/deferred state, instead of depending on COMMIT-time + # deferred enforcement to raise `IntegrityError` -- it verifies the + # exact same underlying fact (this row genuinely violates the + # ownership FK) without depending on the platform-specific behavior + # that made the original assertion unreliable in CI. db.execute(text("PRAGMA foreign_keys=ON")) + assert db.execute(text("PRAGMA foreign_keys")).scalar() == 1, ( + "PRAGMA foreign_keys did not read back as enabled after setting it" + ) record = RepositoryRecord( id=str(uuid.uuid4()), owner_id=auth_client.default_user["id"], # type: ignore[attr-defined] @@ -402,6 +413,11 @@ def test_cross_owner_lineage_attachment_is_rejected_by_the_database_even_if_forc sequence=1, ) db.add(record) - with pytest.raises(IntegrityError): - db.commit() + db.flush() + violations = db.execute(text("PRAGMA foreign_key_check")).fetchall() db.rollback() + assert violations, ( + "PRAGMA foreign_key_check found no violation for a repository row " + "attached to another owner's lineage -- the composite ownership FK " + "is not protecting this data on this SQLite build." + ) From 17e28e760f09aade23fe43d490e7ab0678d972ce Mon Sep 17 00:00:00 2001 From: PARTH J ROHIT Date: Fri, 28 Aug 2026 22:00:26 +0100 Subject: [PATCH 4/4] test(backend): move the cross-owner FK proof onto a migrated database (#299) Root cause found for the CI failure the previous two commits tried to patch around: PRAGMA foreign_key_check itself found no violation for the forced cross-owner row on CI's SQLite build -- not a deferred-enforcement timing quirk, but the constraint genuinely not being tracked for enforcement there, while introspection still reports it correctly. The actual cause: Base.metadata.create_all() (used by the client/ auth_client test fixtures, and by AUTO_CREATE_TABLES=true in local development) must resolve this table pair's genuine foreign-key cycle by emitting one of repositories/repository_lineages with an inline FK referencing the other table before it exists -- there is no ALTER TABLE ADD CONSTRAINT to add it afterward on SQLite. Confirmed directly via echo=True: repositories was created first in this run, its fk_repositories_lineage_owner FK referencing a repository_lineages table that didn't exist yet. At least one real SQLite build does not enforce a deferred FK declared that way. The Alembic migration never does this -- 0013 creates repository_lineages and the non-cyclic repositories columns/ constraints first, 0014 adds each cyclic FK in its own revision only after both tables already exist -- so a database built by migrating, including every real deployment and this repo's own CI rehearsal, is unaffected. Moves the proof from test_repository_lineage_service.py (auth_client, create_all()) to test_repository_lineage_migration.py (real Alembic migration against a temp SQLite file, the same construction already used for every other test in that file), using direct ORM/Core writes instead of the HTTP auth API. Verified passing on both SQLite and real PostgreSQL, including with CI's exact pytest/coverage command and env vars reproduced locally (999/999, zero failures). Documents the underlying create_all()-on-SQLite limitation directly on both cyclic FK declarations (app/models/repository.py, app/models/repository_lineage.py) so it isn't a silent surprise for a future reader: it is a development/test schema-bootstrap gap only, not a production one, since AUTO_CREATE_TABLES is false outside development/ test and every real deployment runs the migrations. --- apps/backend/app/models/repository.py | 5 ++ apps/backend/app/models/repository_lineage.py | 27 +++++- .../test_repository_lineage_migration.py | 66 ++++++++++++++ .../tests/test_repository_lineage_service.py | 85 ++++--------------- 4 files changed, 113 insertions(+), 70 deletions(-) diff --git a/apps/backend/app/models/repository.py b/apps/backend/app/models/repository.py index bc1c32a9..913e9ea3 100644 --- a/apps/backend/app/models/repository.py +++ b/apps/backend/app/models/repository.py @@ -119,6 +119,11 @@ class RepositoryRecord(Base): # not be checked until commit. No automatic delete action -- deletion # updates or clears the lineage's latest pointer explicitly first # (RFC §8.3), it is never left to a database cascade/set-null here. + # This is one half of a cyclic FK pair with `repository_lineages`; + # see the known `create_all()`-on-SQLite enforcement limitation + # documented on `RepositoryLineage.fk_repository_lineages_latest_member` + # (app/models/repository_lineage.py) -- it applies equally to + # whichever of the two constraints ends up as the forward reference. ForeignKeyConstraint( ["lineage_id", "owner_id"], ["repository_lineages.id", "repository_lineages.owner_id"], diff --git a/apps/backend/app/models/repository_lineage.py b/apps/backend/app/models/repository_lineage.py index a91f2d9b..734bfdb5 100644 --- a/apps/backend/app/models/repository_lineage.py +++ b/apps/backend/app/models/repository_lineage.py @@ -77,7 +77,32 @@ class RepositoryLineage(Base): # §4.2): a latest-pointer can never name a repository outside this # exact lineage, even if service code is wrong. Declared here (rather # than only in the migration) so `create_all()` in development/test - # produces the identical final shape a migrated database reaches. + # produces the identical *declared* shape a migrated database + # reaches (`PRAGMA foreign_key_list`/`inspector.get_foreign_keys()` + # show it either way, on both dialects). + # + # Known SQLite limitation (confirmed in CI, #299): `create_all()` + # cannot avoid embedding one of these two cyclic FKs (this one, or + # `repositories.fk_repositories_lineage_owner`) as an inline forward + # reference to a table that doesn't exist yet -- SQLite must create + # one of `repositories`/`repository_lineages` before the other, and + # there is no `ALTER TABLE ADD CONSTRAINT` to add the missing half + # afterward the way the migration does. Which of the two ends up as + # the forward reference depends on `create_all()`'s internal + # cyclic-dependency tie-break, not something this code controls. At + # least one real SQLite build does not enforce a deferred FK + # declared that way -- it still reports the constraint correctly, + # but a genuine violation neither raises at COMMIT nor shows up in + # `PRAGMA foreign_key_check`. The + # Alembic migration (0013/0014) never creates this forward + # reference -- it adds each cyclic FK in its own revision, after + # both tables already exist -- so a database built by migrating, + # including every real deployment and this repo's own CI rehearsal, + # is unaffected. This is a `create_all()`-only (development/test + # bootstrap) gap, not a production one; see + # tests/test_repository_lineage_migration.py's + # test_cross_owner_lineage_attachment_is_rejected_by_the_database_even_if_forced + # for the migration-backed, reliable version of this proof. ForeignKeyConstraint( ["latest_repository_id", "id"], ["repositories.id", "repositories.lineage_id"], diff --git a/apps/backend/tests/test_repository_lineage_migration.py b/apps/backend/tests/test_repository_lineage_migration.py index 4436428f..fdd4b261 100644 --- a/apps/backend/tests/test_repository_lineage_migration.py +++ b/apps/backend/tests/test_repository_lineage_migration.py @@ -453,3 +453,69 @@ def test_backfill_rerun_is_idempotent(lineage_migration_db): command.upgrade(cfg, "head") command.downgrade(cfg, "base") + + +def test_cross_owner_lineage_attachment_is_rejected_by_the_database_even_if_forced(lineage_migration_db): + """The composite deferred ownership FK (`fk_repositories_lineage_owner`) + rejects a forced cross-owner attachment even if application code is + wrong (#299 §9) -- proven against a database built the same way a real + deployment's is, via the actual Alembic migrations, not + `Base.metadata.create_all()`. + + This deliberately does not use the `client`/`auth_client` fixtures + (which bootstrap their schema via `create_all()`): resolving this + table pair's genuine foreign-key cycle there requires emitting one + table with an inline FK referencing the other before it exists, and at + least one SQLite build encountered in CI does not enforce a deferred FK + declared that way -- confirmed directly: the identical scenario built + on `create_all()` failed `PRAGMA foreign_key_check` outright on that + build, while raising `IntegrityError` reliably every time on other + platforms. The migration never creates that inline forward reference -- + 0014 adds this exact constraint in a second revision, after both tables + already exist -- which is what makes this version of the assertion + reliable everywhere instead of platform-dependent. + """ + database_url, engine = lineage_migration_db + cfg = _alembic_config() + command.upgrade(cfg, "head") + + owner_a, owner_b = _seed_users_and_repositories(engine, []) + + meta = MetaData() + lineages = Table("repository_lineages", meta, autoload_with=engine) + repositories = Table("repositories", meta, autoload_with=engine) + + now = datetime.now(UTC) + lineage_id = str(uuid.uuid4()) + with engine.begin() as connection: + connection.execute( + lineages.insert().values( + id=lineage_id, + owner_id=owner_b, + canonical_source_key="github.com/other/repo2", + canonical_branch="refs/heads/main", + display_name="repo2", + latest_repository_id=None, + next_sequence=1, + created_at=now, + ) + ) + + from sqlalchemy.exc import IntegrityError + + with engine.connect() as connection: + transaction = connection.begin() + connection.execute( + repositories.insert().values( + **_repo_row( + owner_a, + id=str(uuid.uuid4()), + name="cross-owner-attempt", + lineage_id=lineage_id, + sequence=1, + ) + ) + ) + with pytest.raises(IntegrityError): + transaction.commit() + transaction.rollback() diff --git a/apps/backend/tests/test_repository_lineage_service.py b/apps/backend/tests/test_repository_lineage_service.py index 5ff8d530..a0d3d0c4 100644 --- a/apps/backend/tests/test_repository_lineage_service.py +++ b/apps/backend/tests/test_repository_lineage_service.py @@ -352,72 +352,19 @@ def test_cross_owner_lineage_lookup_never_matches_another_owners_row(auth_client assert persisted.sequence == 1 -def test_cross_owner_lineage_attachment_is_rejected_by_the_database_even_if_forced(auth_client, make_auth_headers): - """The composite deferred FK enforces ownership even if application code - is wrong (#299 §9) -- proven here by deliberately bypassing the service - layer and trying to attach a repository to another owner's lineage - directly.""" - other = make_auth_headers("owner-c@example.com") - with SessionLocal() as db: - lineage = RepositoryLineage( - id=str(uuid.uuid4()), - owner_id=other["user"]["id"], - canonical_source_key="github.com/other/repo2", - canonical_branch="refs/heads/main", - display_name="repo2", - latest_repository_id=None, - next_sequence=1, - created_at=datetime.now(UTC), - ) - db.add(lineage) - db.commit() - lineage_id = lineage.id - - from sqlalchemy import text - - with SessionLocal() as db: - # Force enforcement on this exact connection rather than relying on - # app.core.database's process-wide connect-event listener having - # already fired -- must be the very first statement on the - # connection, before SQLite's autobegin opens a transaction, since - # the pragma is a no-op once one is open. Confirmed (#299 follow-up): - # explicitly forcing this pragma was NOT sufficient to make SQLite - # actually enforce the deferred composite FK on the Linux CI runner, - # even though it reads back as ON and the identical scenario raises - # reliably on macOS -- a genuine platform/SQLite-build difference in - # deferred FK support, not a test-setup ordering issue. This - # assertion therefore uses `PRAGMA foreign_key_check`, which is - # documented to detect a violation regardless of the connection's - # `foreign_keys`/deferred state, instead of depending on COMMIT-time - # deferred enforcement to raise `IntegrityError` -- it verifies the - # exact same underlying fact (this row genuinely violates the - # ownership FK) without depending on the platform-specific behavior - # that made the original assertion unreliable in CI. - db.execute(text("PRAGMA foreign_keys=ON")) - assert db.execute(text("PRAGMA foreign_keys")).scalar() == 1, ( - "PRAGMA foreign_keys did not read back as enabled after setting it" - ) - record = RepositoryRecord( - id=str(uuid.uuid4()), - owner_id=auth_client.default_user["id"], # type: ignore[attr-defined] - name="cross-owner-attempt", - source="github", - source_url="https://github.com/other/repo2", - branch="main", - revision_kind="git", - revision_value="d" * 40, - revision_ref="refs/heads/main", - local_path="/tmp/x", - status="analysing", - lineage_id=lineage_id, - sequence=1, - ) - db.add(record) - db.flush() - violations = db.execute(text("PRAGMA foreign_key_check")).fetchall() - db.rollback() - assert violations, ( - "PRAGMA foreign_key_check found no violation for a repository row " - "attached to another owner's lineage -- the composite ownership FK " - "is not protecting this data on this SQLite build." - ) +# Direct proof that the composite ownership FK rejects a forced cross-owner +# attachment lives in tests/test_repository_lineage_migration.py (SQLite, via +# a real Alembic-migrated database) and +# tests/test_repository_lineage_concurrency.py (real PostgreSQL). See the +# commit history on this file for why: the same assertion built on top of +# this file's `auth_client` fixture (which bootstraps its schema via +# `Base.metadata.create_all()`, not migrations) was not reliable across +# SQLite builds -- `create_all()` must resolve this table pair's genuine +# foreign-key cycle by emitting one table with an inline FK referencing the +# other table before it exists, and at least one SQLite build encountered in +# CI does not enforce a deferred FK declared that way, even though it stores +# and reports the declaration correctly. The Alembic migration never does +# this -- it adds the constraint in a second revision after both tables +# already exist -- which is why the migration-backed version of this +# assertion is reliable and this file's version was removed rather than +# patched a third time.