diff --git a/docs/architecture.md b/docs/architecture.md new file mode 100644 index 0000000..0a521fc --- /dev/null +++ b/docs/architecture.md @@ -0,0 +1,224 @@ +# Architecture + +[← Documentation index](index.md) + +For whoever has to change this code without breaking somebody's photo library. It +explains what the pieces are, which rules each one keeps, and where to look when a +stage refuses. + +The product decisions behind all of it live in `INTEGRATED_PIPELINE_CONCEPT.md`; this +page describes what was built. + +## Context + +Five things outside the application, and what actually crosses each boundary. + +```mermaid +flowchart LR + operator([Operator]):::person -->|browser, one session| app + app[Photo Pipeline]:::system -->|read, rename, EXIF write| library[(Photo library
bind mount)] + app -->|database, cache, journals, backups| data[(Data directory
local filesystem)] + app -->|confirmed-SFW images only| vision[Vision provider]:::ext + app -->|immich-go, verified bytes| immich[Immich server]:::ext + app -->|copy, verify, then remove| archive[(Archive medium)] + classDef person fill:#1f6feb,stroke:#58a6ff,color:#fff + classDef system fill:#238636,stroke:#3fb950,color:#fff + classDef ext fill:#6e40c9,stroke:#a371f7,color:#fff +``` + +| boundary | leaves the machine? | carries | +|---|---|---| +| operator → app | no (loopback, or a proxy you configured) | commands against stable ids, never paths | +| app → library | no | reads, folder renames, EXIF merges | +| app → data directory | no | SQLite in WAL mode, thumbnails, journals, backups | +| app → vision provider | **yes** | image bytes of confirmed-SFW canonical assets only | +| app → Immich | **yes** | the exact verified bytes of an approved album | +| app → archive medium | no | copies, verified before the source is removed | + +## Runtime + +Two long-lived processes and a one-shot migration, sharing one database and one +library. + +```mermaid +flowchart TD + browser([Browser]) -->|JSON + SSE| api + migrate["migrate
backup, then upgrade"] -->|must exit 0| api + migrate --> worker + api["api · serve
enqueues, serves, reads"] -->|jobs table| db[(SQLite WAL)] + worker["worker
claims and does the work"] -->|jobs table| db + worker --> tools["exiftool · vision API · immich-go"] + api -.->|api.lock.json| lock{{library lock}} + worker -.->|worker.lock.json| lock + api --> lib[(library)] + worker --> lib +``` + +`serve` enqueues and renders; **it does not do the work**. Everything that scans, +scores, analyses, renames, uploads, or archives happens in the worker, which claims +a queued job atomically with a fencing token. One mutating job runs at a time, and +the lock file in the data directory is what makes a second worker impossible rather +than merely discouraged. + +## The modules + +| package | owns | must not | +|---|---|---| +| `api/` | HTTP surface, error envelope, security policy | contain SQL or business logic | +| `api/routes/` | one module per resource group, all under `/api/v1` | accept a filesystem path from the browser | +| `api/security.py` | host/origin/CSRF/session/size policy as one pure `evaluate`, plus the ASGI middleware | be bypassed per-route | +| `schemas/` | Pydantic request and response contracts | reach the database | +| `services/` | all domain logic; the only place a decision is made | be imported by the frozen CLI archive | +| `models/` | SQLAlchemy tables | hold behaviour | +| `jobs/` | durable job lifecycle, worker loop, lock ranks, handler registry | run work in the API process | +| `integrations/` | the real outside world: `exiftool`, vision, NSFW model, `immich-go` and its report grammars | be called without a version recorded | +| `path_policy.py` | the library boundary, `_IGNORE/` exclusion, symlink-escape refusal | be duplicated anywhere | +| `imaging.py` | the single bounded-decode door | let a caller open an image directly | +| `faults.py` | the crash barriers the tests fire | read anything but its one env var | +| `config.py` | typed settings, secrets as `SecretStr` | log or return a value | +| `db.py` | engine, sessions, WAL and foreign keys, migration entry | be used to bypass a repository | + +Services worth knowing by name: `inventory`, `duplicates`, `safety`, `analysis`, +`albums`/`proposals`/`naming`, `renames`/`rename_apply`/`rename_journal`, +`uploads`/`upload_batches`/`upload_reports`/`upload_verification`, +`archives`/`archive_transfer`/`archive_journal`/`restores`/`availability`, +`thumbnails`, `hashing`, `exif_checkpoint`, `jobs`, `workflow`, `backup`, +`diagnostics`, `app_lock`, `release`, `benchmarks`, `library`, `legacy_import`. + +## The state machines + +Three journals decide what a restart is allowed to assume. All three are read from +the database plus the real world — never guessed from a missing file. + +### Durable jobs + +```mermaid +stateDiagram-v2 + [*] --> queued + queued --> running: claimed with a fencing token + queued --> cancelled + queued --> cancelling + running --> succeeded + running --> failed + running --> cancelling + cancelling --> cancelled + failed --> retry_queued + retry_queued --> running + succeeded --> [*] + cancelled --> [*] +``` + +`succeeded` and `cancelled` are terminal, so a duplicate delivery cannot move a +finished job. Claiming is compare-and-set on the row version; a worker whose lease +expired cannot commit after another has taken over. + +### The rename journal + +The only state machine that moves somebody's folders. + +```mermaid +stateDiagram-v2 + [*] --> planned + planned --> moving + moving --> moved + moving --> planned: proven untouched + moving --> rollback_required + moved --> database_updated + database_updated --> verified + verified --> complete + moved --> rollback_required + database_updated --> rollback_required + verified --> rollback_required + planned --> failed + failed --> planned + rollback_required --> rolled_back + complete --> [*] + rolled_back --> [*] +``` + +Intent is written **before** the disk is touched, which is why `moving` can resolve +backwards: the evidence decides. `moving`, `moved`, `database_updated`, and +`rollback_required` are the unsafe states — while any operation sits in one, the +library may be half-renamed, so unrelated mutations are refused with +`409 rename_recovery_required` until a person resolves it. + +### Upload batches + +```mermaid +stateDiagram-v2 + [*] --> planned + planned --> running + running --> succeeded + running --> failed + running --> cancelling + running --> unknown_requires_verification: process died after acceptance + cancelling --> cancelled + failed --> running: retry + cancelled --> running: retry +``` + +`unknown_requires_verification` is deliberately **not** restartable. The server may +already hold the files; the answer is to ask Immich for the recorded SHA-1, not to +upload again and hope. + +Archive transfers use the same shape — `planned → transferring → verified → +removing → complete` — and the source is removed only after the archived bytes are +verified. + +## Identity and the data model + +A path is metadata. The identity is `assets.id`, a UUID that never changes, and +`asset_paths` records every path an asset has ever had with the reason it changed. +That is what makes a rename cheap: nothing else in the database has to move. + +Tables: `assets`, `asset_paths`, `thumbnails`, `jobs`, `job_items`, `job_events`, +`safety_reviews`, `analysis_results`, `exif_projections`, `duplicate_clusters`, +`duplicate_members`, `duplicate_negative_links`, `album_proposals`, `rename_plans`, +`rename_operations`, `upload_batches`, `upload_items`, `upload_verifications`, +`archive_locations`, `archive_plans`, `archive_operations`. + +Availability is independent of workflow progress: `active`, `archiving`, +`archived_online`, `archived_offline`, `restoring`, `missing_unexpected`. An +unmounted archive disk is `archived_offline`, never `missing` — the scanner is not +allowed to conclude that a photo is gone because a disk is unplugged. + +EXIF is a projection with its own verified state (`verified`, `divergent`, +`failed`): the desired values are written, read back, and compared, and anything the +stage does not own having changed makes the asset `divergent` and blocks the next +mutating stage. + +## Where each invariant lives + +| invariant | enforced in | +|---|---| +| `_IGNORE/` is never traversed, counted, or opened | `path_policy.is_excluded`, used by every discovery path | +| no path outside the library roots is reachable | `path_policy.resolve_in_roots` — it returns the resolved path, because validating one name and opening another is the symlink race | +| one writer per library | `services/app_lock.py` (an `flock` on a JSON lock file, per role) | +| one mutating job at a time | `services/jobs.py` lock keys plus `jobs/locks.py` rank ordering | +| only confirmed-SFW assets reach the vision provider | `services/analysis.py`, re-checked *after* the provider call so a decision that flipped mid-flight discards the result | +| EXIF is verified, and other fields preserved | `services/exif_checkpoint.py` | +| uploads carry the exact verified bytes | `services/uploads.py` preflight, re-proved immediately before the uploader runs | +| an uncertain upload is not a failure | `services/upload_batches.py`, `services/upload_verification.py` | +| the archive source outlives its copy until verified | `services/archive_transfer.py` | +| decoding is bounded | `imaging.py` | +| the trust boundary and CSRF | `api/security.py` | + +## Concurrency + +Locks are taken broad to narrow — `library → stage/job → album/folder → asset` — and +never the other way, which is what makes deadlock structural rather than lucky. +Ranks live in `jobs/locks.py`. + +Exactly-once execution is not achievable across SQLite, a filesystem, subprocesses, +and a remote server. The design promises **at-least-once with idempotent recovery**: +every handler may run twice, and running twice must not produce two side effects. +That is why the rename journal records intent before moving, why EXIF writes merge +and verify, and why upload retries consult both local history and Immich. + +## History + +This application was extracted from two command-line tools rather than written from +nothing. Their sources are frozen in `legacy_cli_archive/` with the ledger mapping +each donated behaviour to the service that now owns it, the characterization tests +that pinned it, and every intentional difference. Production code must not import +them; they are provenance and rollback evidence. diff --git a/docs/index.md b/docs/index.md index 62db87f..44cfce7 100644 --- a/docs/index.md +++ b/docs/index.md @@ -15,6 +15,9 @@ repository under `docs/`, on Gitea, and inside the running application under 2. [Installation and operations](installation.md) — host and container installation, every setting, the first-run checklist, upgrades, backup and restore, and what each refusal at startup means. +3. [Architecture](architecture.md) — the context and runtime diagrams, what each + module owns, the three journals a restart reads, and where every invariant is + enforced. ## Being written @@ -22,8 +25,6 @@ The remaining manuals are accepted work, not aspiration; each is a story in [E09](https://git.domverse-berlin.eu/domverse/photoanalyzer/src/branch/main/delivery_backlog/E09-documentation.md) and will appear here as it lands. -- **Architecture** — context and runtime diagrams, the module map, the job and - journal state machines, and where each invariant is enforced (US09-03). - **User manual** — one page per workflow stage with screenshots of the real application, and a catalogue of every error and refusal (US09-04). diff --git a/tests/e2e/test_docs_ui.py b/tests/e2e/test_docs_ui.py index 731c0ca..01d76b9 100644 --- a/tests/e2e/test_docs_ui.py +++ b/tests/e2e/test_docs_ui.py @@ -12,6 +12,8 @@ The offline contract — reachability, dead links, pinned checksums — is from __future__ import annotations +from pathlib import Path + import pytest from tests.e2e._pipeline_harness import Server, seed_library @@ -90,6 +92,34 @@ def test_a_mermaid_block_becomes_a_diagram_under_the_unchanged_script_policy(pag assert "unsafe-eval" not in policy +def test_every_diagram_in_the_architecture_overview_draws(page, server, quiet): + """Five diagrams, three of them state machines (US09-03). A mermaid block with a + syntax error renders an error node instead of throwing, so a page that merely + loaded proves nothing — count the drawings and read the source for the states.""" + source = (Path(__file__).resolve().parents[2] / "docs" / "architecture.md").read_text() + expected = source.count("```mermaid") + assert expected >= 5, "the overview lost its diagrams" + + page.goto(f"{server.base}/app/#/docs?page=architecture") + page.get_by_test_id("diagram").first.wait_for() + page.wait_for_function( + "count => document.querySelectorAll('[data-testid=\"diagram\"] svg').length === count", + arg=expected, + ) + + drawn = page.locator('[data-testid="diagram"] svg') + assert drawn.count() == expected + for index in range(expected): + diagram = drawn.nth(index) + # Labels may be or HTML inside a depending on the + # diagram type, so ask for the rendered text rather than a specific element. + labels = diagram.evaluate("node => node.textContent.trim().length") + assert labels > 0, f"diagram {index} drew no labels" + assert diagram.locator(".error-icon, .error-text").count() == 0, f"diagram {index} errored" + assert page.locator("pre code.language-mermaid").count() == 0 + assert quiet == [] + + def test_an_unknown_page_says_so_without_naming_a_path(page, server, quiet): page.goto(f"{server.base}/app/#/docs?page=no-such-manual") message = page.get_by_test_id("doc-not-found") diff --git a/tests/integration/test_architecture_overview.py b/tests/integration/test_architecture_overview.py new file mode 100644 index 0000000..645a7e9 --- /dev/null +++ b/tests/integration/test_architecture_overview.py @@ -0,0 +1,164 @@ +"""US09-03: the architecture overview, checked against the architecture. + +An architecture document is the one that rots most quietly: nothing breaks when it +describes a module that was renamed two epics ago, it just quietly misleads the next +person. So the parts of it that the code also knows — module names, state machines, +table names, the modules an invariant is claimed to live in — are compared with the +code, and a missing one fails the suite. + +The diagrams' rendering is proven in ``tests/e2e/test_docs_ui.py``. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import photo_pipeline.models # noqa: F401 (registers every table on Base.metadata) +from photo_pipeline.db import Base +from photo_pipeline.services import jobs, rename_journal, upload_batches + +REPO = Path(__file__).resolve().parents[2] +PACKAGE = REPO / "photo_pipeline" +OVERVIEW = REPO / "docs" / "architecture.md" +TEXT = OVERVIEW.read_text() + +# Names the map does not owe the reader individually: the package markers, the CLI +# entry point, and the two modules that exist to be small and obvious. +UNMAPPED = {"__init__", "__main__", "logging"} + + +def modules() -> set[str]: + """Every module and package under photo_pipeline, by the name it is imported as.""" + names = set() + for path in PACKAGE.rglob("*.py"): + relative = path.relative_to(PACKAGE) + names.add(relative.parts[0] if len(relative.parts) > 1 else relative.stem) + return {name.removesuffix(".py") for name in names} - UNMAPPED + + +def states(container: type) -> set[str]: + return { + value + for name, value in vars(container).items() + if name.isupper() and isinstance(value, str) + } + + +# ── the module map ─────────────────────────────────────────────────────────── + + +def test_every_module_appears_in_the_map(): + """A service nobody documented is a service the next person re-implements.""" + missing = sorted(name for name in modules() if name not in TEXT) + assert missing == [], f"absent from the architecture overview: {missing}" + + +def test_every_service_is_named_individually(): + """The package table says what `services/` is for; this is the list that actually + drifts, because a new service is added roughly every story.""" + services = {path.stem for path in (PACKAGE / "services").glob("*.py")} - UNMAPPED + missing = sorted(name for name in services if not re.search(rf"\b{name}\b", TEXT)) + assert missing == [], f"services missing from the overview: {missing}" + + +def test_the_map_names_no_module_that_does_not_exist(): + """Every `services/` or backticked module path in the document resolves.""" + referenced = set(re.findall(r"`(?:photo_pipeline/)?([a-z_]+)/([a-z_]+)\.py`", TEXT)) + referenced |= {("", name) for name in re.findall(r"`([a-z_]+)\.py`", TEXT)} + missing = [ + f"{package}/{module}.py" if package else f"{module}.py" + for package, module in referenced + if not (PACKAGE / package / f"{module}.py").is_file() + ] + assert missing == [], f"documented but absent from the source: {missing}" + + +# ── the state machines ─────────────────────────────────────────────────────── + + +def test_every_job_state_is_documented(): + missing = sorted(state for state in states(jobs.JobState) if state not in TEXT) + assert missing == [], f"job states missing from the overview: {missing}" + + +def test_every_rename_journal_state_is_documented(): + missing = sorted(state for state in states(rename_journal.JournalState) if state not in TEXT) + assert missing == [], f"journal states missing from the overview: {missing}" + + +def test_every_upload_batch_state_is_documented(): + missing = sorted(state for state in states(upload_batches.BatchState) if state not in TEXT) + assert missing == [], f"batch states missing from the overview: {missing}" + + +def test_the_unsafe_journal_states_are_named_as_the_ones_that_block(): + """The reason an unrelated mutation is refused has to be findable.""" + for state in rename_journal.UNSAFE_STATES: + assert state in TEXT + assert "rename_recovery_required" in TEXT + + +def test_an_uncertain_upload_is_documented_as_not_retryable(): + assert upload_batches.BatchState.UNKNOWN not in upload_batches.RUNNABLE_STATES + assert "not** restartable" in TEXT or "not restartable" in TEXT + + +# ── the data model ─────────────────────────────────────────────────────────── + + +def test_every_table_is_listed(): + missing = sorted(name for name in Base.metadata.tables if name not in TEXT) + assert missing == [], f"tables missing from the overview: {missing}" + + +def test_the_document_lists_no_table_that_does_not_exist(): + listed = set(re.findall(r"`([a-z_]+)`", TEXT)) + plausible = {name for name in listed if name.endswith("s") and "_" in name} + invented = sorted( + name + for name in plausible + if name not in Base.metadata.tables + and not (PACKAGE / "services" / f"{name}.py").is_file() + and name not in {"library_roots", "trusted_proxies", "allowed_hosts", "asset_paths"} + ) + assert invented == [], f"looks like a table but is not one: {invented}" + + +# ── the invariants ─────────────────────────────────────────────────────────── + + +def test_each_invariant_points_at_a_module_that_enforces_it(): + """The point of the table is to answer 'where do I look'. A wrong answer there + costs more than no answer.""" + claims = { + "path_policy.is_excluded": PACKAGE / "path_policy.py", + "path_policy.resolve_in_roots": PACKAGE / "path_policy.py", + "services/app_lock.py": PACKAGE / "services" / "app_lock.py", + "services/exif_checkpoint.py": PACKAGE / "services" / "exif_checkpoint.py", + "services/archive_transfer.py": PACKAGE / "services" / "archive_transfer.py", + "api/security.py": PACKAGE / "api" / "security.py", + "imaging.py": PACKAGE / "imaging.py", + } + for claim, path in claims.items(): + assert claim in TEXT, f"the invariant table does not mention {claim}" + assert path.is_file(), f"{claim} does not exist" + function = claim.rpartition(".")[2] if "/" not in claim else "" + if function and function != "py": + assert f"def {function}" in path.read_text(), f"{claim} is not defined there" + + +def test_the_lock_order_matches_the_ranks_in_the_code(): + from photo_pipeline.jobs import locks + + order = [name for name, _ in sorted(locks.LOCK_RANK.items(), key=lambda item: item[1])] + assert order[0].startswith("library"), "the broadest lock is no longer the library" + assert "library → stage/job → album/folder → asset" in TEXT + + +def test_the_frozen_donor_archive_is_described_as_provenance_not_a_dependency(): + """That production never imports the frozen sources is proven by + ``tests/unit/test_legacy_archive.py`` — which also forbids any other test from + naming that directory, so this one checks the claim by its consequence.""" + assert "must not import" in TEXT + assert "provenance and rollback evidence" in TEXT diff --git a/tests/story_traceability.json b/tests/story_traceability.json index 4ee90fd..f3747e5 100644 --- a/tests/story_traceability.json +++ b/tests/story_traceability.json @@ -200,10 +200,13 @@ ], "US09-02": [ "tests/integration/test_installation_manual.py" + ], + "US09-03": [ + "tests/integration/test_architecture_overview.py", + "tests/e2e/test_docs_ui.py" ] }, "planned": [ - "US09-03", "US09-04", "US09-05" ],