diff --git a/docs/superpowers/contracts/endurain-db-dedupe-2026-08-23.json b/docs/superpowers/contracts/endurain-db-dedupe-2026-08-23.json new file mode 100644 index 00000000..d4a148dd --- /dev/null +++ b/docs/superpowers/contracts/endurain-db-dedupe-2026-08-23.json @@ -0,0 +1,19 @@ +{ + "title": "endurain importer dedupe via postgres", + "objective": "Give the importer a working answer to 'does Endurain already hold this run?', now that the HTTP route cannot provide one: API keys are upload-only and the activities read endpoint is JWT-only, so the existing check always returned None and every pass relied on the local ledger alone.", + "acceptance_criteria": [ + "A run Endurain already holds is skipped even when the local ledger is empty, and the activity count does not grow.", + "The same run IS imported when the fallback is disabled, proving the check is what blocks it.", + "A dead or renamed container yields None (ledger fallback), while a user with no activities yields an empty list; the two are never conflated.", + "The journal distinguishes which backend answered, so a DB-sourced pass is not mistaken for a ledger-only one.", + "Verified in the real systemd unit, not only from an interactive shell.", + "100% branch coverage on the new and changed modules, with no lint suppressions added.", + "No test touches the real postgres container." + ], + "out_of_scope": [ + "Removing or replacing the HTTP lookup - it stays first in the chain so the supported path resumes working by itself if Endurain widens the API-key scope allow-list.", + "Exposing a postgres host port or adding a database driver dependency.", + "Patching Endurain's own source or forging scopes in users_api_keys to work around the upload-only allow-list." + ], + "verifier": "pre-commit + python_pkg/endurain_import/tests + a real systemctl --user run with journal inspection" +} diff --git a/docs/superpowers/evidence/endurain-db-dedupe-2026-08-23.json b/docs/superpowers/evidence/endurain-db-dedupe-2026-08-23.json new file mode 100644 index 00000000..ecaa2087 --- /dev/null +++ b/docs/superpowers/evidence/endurain-db-dedupe-2026-08-23.json @@ -0,0 +1,61 @@ +{ + "intent": "Stop the importer re-uploading runs RunnerUp already sent to Endurain, by reading start times from postgres when the HTTP API cannot answer. Endurain API keys are upload-only (scope allow-list is frozenset({'activities:upload'}) and the activities read route is mounted JWT-only), so the existing HTTP dedupe check could never succeed and every pass fell back to the local ledger alone.", + "scope": [ + "python_pkg/endurain_import/db_lookup.py (new): postgres reader + known_start_times orchestration", + "python_pkg/endurain_import/__main__.py: delegates dedupe to known_start_times; new env vars documented", + "python_pkg/endurain_import/upload.py: 401 warning reworded, it is no longer the end of the story", + "python_pkg/endurain_import/tests/: new test_db_lookup.py, two DB-fallback cases, conftest isolation", + "Non-goal: removing the HTTP path. It is tried first so the supported route starts working by itself if Endurain ever widens the scope allow-list.", + "Non-goal: exposing a postgres host port or adding a psycopg dependency. Reads go through docker exec, which is how the container is already reachable." + ], + "changes": [ + "EndurainDatabase.recent_start_times shells out to `docker exec -i psql -At` and parses one timestamp per line, reusing upload._parse_start so both backends normalise to UTC identically.", + "The query is fed on stdin with -v bindings (uid/lim) rather than -c, because psql only substitutes -v variables in scripts it reads; user_id never reaches the SQL string. ON_ERROR_STOP=1 makes a SQL error a non-zero exit rather than silently empty output.", + "None vs [] is the load-bearing distinction: a dead container, OSError, timeout or non-zero exit yields None ('could not ask', fall back to the ledger), while an empty result set yields [] ('this user has no activities'). Conflating them would re-upload everything on any docker hiccup.", + "known_start_times chains HTTP -> postgres -> None and logs which backend answered, so a DB-sourced pass is distinguishable from a ledger-only one in the journal.", + "The subprocess runner is injected (default _run) so every failure branch is reachable in unit tests without monkeypatching subprocess globally.", + "conftest._env sets ENDURAIN_NO_DB=1 by default so no test can shell out to the real postgres container; the two tests that exercise the fallback opt in and stub EndurainDatabase." + ], + "verification": [ + { + "command": "systemctl --user start endurain-import.service && journalctl --user -u endurain-import.service", + "result": "pass", + "evidence": "pulled 5 file(s) from the phone / could not list recent activities over HTTP: 401 / duplicate detection: 4 activity start time(s) via the database / done: 0 imported, 5 already known, 0 rejected, 0 ambiguous. Confirms the fallback answers in the real systemd deployment path, not just an interactive shell." + }, + { + "command": "ENDURAIN_INBOX= ENDURAIN_STATE= python -m python_pkg.endurain_import (known-duplicate TCX, empty ledger)", + "result": "pass", + "evidence": "'RunnerUp_2026-08-22-23-51-04_Running.tcx is already in Endurain (uploaded outside this importer); moved to processed/', activities count stayed at 4. With an empty ledger only the DB check could have caught this, so it proves the feature rather than the ledger." + }, + { + "command": "same run with ENDURAIN_NO_DB=1 (negative control)", + "result": "pass", + "evidence": "'imported RunnerUp_2026-08-22-23-51-04_Running.tcx' - the same file WAS uploaded with the fallback disabled, proving the DB check is what blocked it. The duplicate this created (activity id 16) was deleted afterwards; activities is back to 4 rows with 0 duplicate start_times." + }, + { + "command": "python -m pytest python_pkg/endurain_import/tests/ --cov=python_pkg.endurain_import", + "result": "pass", + "evidence": "98 passed. db_lookup.py 52 stmts 0 miss 8 branch 0 partial 100%; __main__.py 91 stmts 0 miss 18 branch 0 partial 100%." + }, + { + "command": "pre-commit run --files ", + "result": "pass", + "evidence": "ruff, ruff format, mypy, pylint, bandit, codespell, 250-line cap and pytest-coverage all pass. No noqa/type-ignore/nosec suppressions were added; the three the first draft carried were removed by fixing the underlying typing instead." + }, + { + "command": "systemd-run --user --wait docker exec endurain-postgres psql ...", + "result": "pass", + "evidence": "exit 0 with rows returned, checked BEFORE writing the module: docker access works from the systemd user manager, not only from an interactive shell." + } + ], + "risks": [ + "Couples the importer to the postgres container name and to docker exec being available to the systemd user manager. Both are verified above; a change to either degrades to None and the ledger, which is the pre-existing behaviour, not a new failure.", + "Reads Endurain's schema directly, so an upstream rename of activities.start_time or activities.user_id would break the fallback. It fails closed (non-zero exit -> None -> ledger), so the failure mode is 'no extra dedupe', never a wrong answer.", + "The importer's postgres access is read-only by construction (a single SELECT), but it does run as a superuser inside the container; nothing constrains it beyond the statement itself." + ], + "rollback": [ + "git revert the commit. __main__.py returns to calling client.recent_start_times directly and db_lookup.py disappears.", + "Or set ENDURAIN_NO_DB=1 in the unit for a no-deploy disable, which restores ledger-only dedupe without touching code.", + "After either, run the service and confirm 'duplicate detection ... falling back to the local ledger only' plus a clean 'done:' line, and check `SELECT start_time, count(*) FROM activities GROUP BY start_time HAVING count(*)>1` still returns no rows." + ] +} diff --git a/python_pkg/endurain_import/__main__.py b/python_pkg/endurain_import/__main__.py index df1a8ce6..aa260ef6 100644 --- a/python_pkg/endurain_import/__main__.py +++ b/python_pkg/endurain_import/__main__.py @@ -3,15 +3,22 @@ Run from a systemd user timer. Configuration comes from the environment: ENDURAIN_URL base URL (default http://127.0.0.1:8085) - ENDURAIN_API_KEY API key with activities:upload (required) and - activities:read (optional; enables detection of - runs RunnerUp uploaded to Endurain directly) + ENDURAIN_API_KEY API key with activities:upload (required) ENDURAIN_INBOX WebDAV inbox (default ~/cloud/RunnerUp) ENDURAIN_STATE ledger directory (default ~/.local/state/endurain-import) ENDURAIN_BULK_DIR bulk_import dir for files the server rejected ENDURAIN_NO_ADB set to 1 to disable the phone fallback ENDURAIN_ADB_SERIAL which phone to pull from when several are attached ENDURAIN_USER_ID Endurain user whose activities are checked (default 1) + ENDURAIN_NO_DB set to 1 to disable the postgres dedupe fallback + ENDURAIN_PG_CONTAINER postgres container name (default endurain-postgres) + +Duplicate detection asks Endurain what it already holds, so a run RunnerUp +uploaded directly is not imported a second time. The HTTP API is tried first, +but API keys cannot read activities (the scope allow-list is upload-only and +the read route is JWT-only), so in practice the postgres fallback is what +answers. If both are unavailable the local ledger alone decides, which is the +behaviour this importer had before either check existed. Exit status is 0 only when every file was accounted for; anything left in an ambiguous state exits non-zero so the timer surfaces it rather than logging @@ -29,6 +36,7 @@ from typing import TYPE_CHECKING import requests +from python_pkg.endurain_import.db_lookup import known_start_times from python_pkg.endurain_import.ledger import ( Entry, Ledger, @@ -190,15 +198,10 @@ def main() -> int: _logger.info("nothing to import (ledger holds %d file(s))", len(ledger)) return 0 - # One query per run, not per file: the page is a snapshot of what Endurain - # already holds, and every file in this pass is compared against it. + # One query per run, not per file: the result is a snapshot of what + # Endurain already holds, and every file in this pass is compared against it. user_id = int(os.environ.get("ENDURAIN_USER_ID", _DEFAULT_USER_ID)) - remote_starts = client.recent_start_times(user_id) - if remote_starts is None: - _logger.warning( - "duplicate detection against Endurain is unavailable; falling " - "back to the local ledger only" - ) + remote_starts = known_start_times(client, user_id) counts = {"skipped": 0, "imported": 0, "rejected": 0, "ambiguous": 0} for path in pending: diff --git a/python_pkg/endurain_import/db_lookup.py b/python_pkg/endurain_import/db_lookup.py new file mode 100644 index 00000000..02f65977 --- /dev/null +++ b/python_pkg/endurain_import/db_lookup.py @@ -0,0 +1,183 @@ +"""Duplicate detection that reads Endurain's database directly. + +Endurain's API keys cannot answer "what do you already hold": the key scope +allow-list is ``{"activities:upload"}`` (enforced twice, deliberately), and the +activities read route is mounted JWT-only, so a key fails at *authentication* +before any scope check. Verified against v0.19.0; upstream documents no plan to +widen it. + +That leaves the database. The postgres container publishes no host port, so +this goes through ``docker exec`` rather than a TCP client -- no new dependency, +no compose change, and no port exposed to the host just to read four rows. + +This is a *fallback*, not a replacement: :mod:`__main__` asks the HTTP API first +so that the supported path starts working by itself if Endurain ever grants the +scope. +""" + +from __future__ import annotations + +from collections.abc import Callable, Sequence +import logging +import os +import subprocess +from typing import TYPE_CHECKING, Protocol + +from python_pkg.endurain_import.upload import _parse_start + +if TYPE_CHECKING: + from datetime import datetime + + +class SupportsRecentStartTimes(Protocol): + """The lookup surface :func:`known_start_times` needs from a client.""" + + def recent_start_times(self, user_id: int) -> list[datetime] | None: + """Return recent activity start times, or None if unknowable.""" + ... # pragma: no cover - structural typing only, never executed + + +_logger = logging.getLogger(__name__) + +_CONTAINER_ENV = "ENDURAIN_PG_CONTAINER" +_NO_DB_ENV = "ENDURAIN_NO_DB" +_DEFAULT_CONTAINER = "endurain-postgres" +_DB_USER = "endurain" +_DB_NAME = "endurain" +# Mirrors the HTTP path's page size so both backends answer the same question. +_RECENT_RECORDS = 50 +_TIMEOUT = 30 + +# -At: tuples only, unaligned -- one bare timestamp per line, no header or +# padding to strip. The query goes in on stdin rather than via -c because psql +# only substitutes -v variables in scripts it reads, never inside -c; that +# keeps user_id out of the SQL string instead of formatting it in. +_QUERY = ( + "SELECT start_time FROM activities " + "WHERE user_id = :uid ORDER BY start_time DESC LIMIT :lim;" +) + +Runner = Callable[[Sequence[str], str], "subprocess.CompletedProcess[str]"] + + +def _run(argv: Sequence[str], stdin: str) -> subprocess.CompletedProcess[str]: + """Execute ``argv``, feeding ``stdin``, and capture its output. + + Split out so tests can inject a stub: the real call is unreachable in a + unit test, and the branches around its result are exactly what needs + covering. + """ + return subprocess.run( + list(argv), + input=stdin, + capture_output=True, + text=True, + timeout=_TIMEOUT, + check=False, + ) + + +class EndurainDatabase: + """Reads recent activity start times straight out of postgres.""" + + def __init__(self, container: str | None = None, runner: Runner = _run) -> None: + """Build a reader for ``container``, executing through ``runner``.""" + self._container = container or os.environ.get( + _CONTAINER_ENV, _DEFAULT_CONTAINER + ) + self._run = runner + + def recent_start_times(self, user_id: int) -> list[datetime] | None: + """Return recent activity start times, or None if unknowable. + + None means the question could not be answered -- the container is gone, + psql failed, or the call timed out. An empty list is a real answer + ("this user has no activities") and callers may act on it; None must + only ever mean "fall back". Conflating the two would let a docker + failure read as "nothing is stored" and re-upload everything. + """ + argv = [ + "docker", + "exec", + "-i", + self._container, + "psql", + "-U", + _DB_USER, + "-d", + _DB_NAME, + "-At", + "-v", + "ON_ERROR_STOP=1", + "-v", + f"uid={int(user_id)}", + "-v", + f"lim={_RECENT_RECORDS}", + ] + try: + proc = self._run(argv, _QUERY) + except (OSError, subprocess.SubprocessError) as exc: + _logger.warning("could not read activities from the database: %s", exc) + return None + if proc.returncode != 0: + _logger.warning( + "could not read activities from the database: psql exited %s (%s)", + proc.returncode, + (proc.stderr or "").strip()[:200] or "no stderr", + ) + return None + return _parse_rows(proc.stdout) + + +def _parse_rows(stdout: str) -> list[datetime]: + """Turn psql's one-timestamp-per-line output into datetimes. + + Unparsable lines are dropped rather than failing the lookup: a row this + function cannot read is one fewer duplicate it can catch, which is the + safe direction -- the ledger still guards the upload. + """ + return [ + parsed + for line in stdout.splitlines() + if (stripped := line.strip()) and (parsed := _parse_start(stripped)) is not None + ] + + +def known_start_times( + client: SupportsRecentStartTimes, user_id: int +) -> list[datetime] | None: + """Ask Endurain what it already holds, HTTP first then postgres. + + Returns None only when *neither* backend could answer, which the caller + must read as "unknown" rather than "nothing stored" -- an empty list is a + real answer and means this user genuinely has no activities. + + The HTTP path is tried first even though it cannot currently succeed, so + that the supported route starts working on its own if Endurain ever widens + the API-key scope allow-list. + """ + starts = client.recent_start_times(user_id) + if starts is not None: + _logger.info( + "duplicate detection: %d activity start time(s) via the API", len(starts) + ) + return starts + + if os.environ.get(_NO_DB_ENV) == "1": + _logger.warning( + "duplicate detection against Endurain is unavailable and the " + "database fallback is disabled; falling back to the local ledger only" + ) + return None + + starts = EndurainDatabase().recent_start_times(user_id) + if starts is None: + _logger.warning( + "duplicate detection against Endurain is unavailable; falling " + "back to the local ledger only" + ) + return None + _logger.info( + "duplicate detection: %d activity start time(s) via the database", len(starts) + ) + return starts diff --git a/python_pkg/endurain_import/tests/conftest.py b/python_pkg/endurain_import/tests/conftest.py index 990bae10..660188ae 100644 --- a/python_pkg/endurain_import/tests/conftest.py +++ b/python_pkg/endurain_import/tests/conftest.py @@ -61,6 +61,12 @@ def _env( monkeypatch.setenv("ENDURAIN_INBOX", str(inbox)) monkeypatch.setenv("ENDURAIN_STATE", str(tmp_path / "state")) monkeypatch.setenv("ENDURAIN_NO_ADB", "1") + # Off by default so no test ever shells out to the real postgres container: + # the DB fallback runs whenever the stubbed HTTP lookup returns None, and a + # test that reached the live database would pass or fail on this machine's + # actual activity rows. Tests that want it set ENDURAIN_NO_DB="0" and stub + # EndurainDatabase explicitly. + monkeypatch.setenv("ENDURAIN_NO_DB", "1") for key, value in extra.items(): monkeypatch.setenv(key, value) diff --git a/python_pkg/endurain_import/tests/test_db_lookup.py b/python_pkg/endurain_import/tests/test_db_lookup.py new file mode 100644 index 00000000..cde08325 --- /dev/null +++ b/python_pkg/endurain_import/tests/test_db_lookup.py @@ -0,0 +1,119 @@ +"""Reading Endurain's activity start times straight out of postgres. + +The load-bearing distinction is None vs []: None means "could not ask" and +must send the caller back to the ledger, while [] is a real answer meaning +this user has no activities. Swapping them would upload a duplicate every +time docker hiccuped, which is the bug the dedupe check exists to prevent. +""" + +from __future__ import annotations + +from collections.abc import Sequence +from datetime import datetime, timezone +import subprocess + +import pytest + +from python_pkg.endurain_import import db_lookup + + +def _proc( + returncode: int = 0, stdout: str = "", stderr: str = "" +) -> subprocess.CompletedProcess[str]: + return subprocess.CompletedProcess( + args=["docker"], returncode=returncode, stdout=stdout, stderr=stderr + ) + + +def _db_returning( + result: subprocess.CompletedProcess[str] | Exception, + captured: dict[str, object] | None = None, +) -> db_lookup.EndurainDatabase: + def _runner(argv: Sequence[str], stdin: str) -> subprocess.CompletedProcess[str]: + if captured is not None: + captured["argv"] = list(argv) + captured["stdin"] = stdin + if isinstance(result, Exception): + raise result + return result + + return db_lookup.EndurainDatabase(runner=_runner) + + +def test_parses_rows_into_utc() -> None: + db = _db_returning(_proc(stdout="2026-08-22 23:51:05+02\n2026-08-19 19:10:06+02\n")) + assert db.recent_start_times(1) == [ + datetime(2026, 8, 22, 21, 51, 5, tzinfo=timezone.utc), + datetime(2026, 8, 19, 17, 10, 6, tzinfo=timezone.utc), + ] + + +def test_empty_table_is_an_answer_not_a_failure() -> None: + """No rows means "nothing stored" -- callers may act on it.""" + assert _db_returning(_proc(stdout="")).recent_start_times(1) == [] + + +def test_nonzero_exit_is_unknown() -> None: + """A dead container must not read as "no activities exist".""" + db = _db_returning(_proc(returncode=1, stderr="No such container")) + assert db.recent_start_times(1) is None + + +def test_nonzero_exit_without_stderr_is_unknown() -> None: + """Covers the empty-stderr branch of the failure message.""" + assert _db_returning(_proc(returncode=2)).recent_start_times(1) is None + + +def test_subprocess_error_is_unknown() -> None: + db = _db_returning(subprocess.TimeoutExpired(cmd="docker", timeout=30)) + assert db.recent_start_times(1) is None + + +def test_os_error_is_unknown() -> None: + """docker missing from PATH raises OSError, not a SubprocessError.""" + assert _db_returning(OSError("no docker")).recent_start_times(1) is None + + +def test_unparseable_rows_are_dropped_not_fatal() -> None: + db = _db_returning(_proc(stdout="not a timestamp\n2026-08-22 23:51:05+02\n\n")) + assert db.recent_start_times(1) == [ + datetime(2026, 8, 22, 21, 51, 5, tzinfo=timezone.utc) + ] + + +def test_query_is_parameterised_and_user_id_coerced() -> None: + """user_id reaches psql as a -v binding, never spliced into the SQL.""" + captured: dict[str, object] = {} + _db_returning(_proc(stdout=""), captured).recent_start_times(7) + argv = captured["argv"] + assert isinstance(argv, list) + assert "uid=7" in argv + assert "ON_ERROR_STOP=1" in argv + # The statement itself carries no interpolated id. + assert "7" not in str(captured["stdin"]) + assert ":uid" in str(captured["stdin"]) + + +def test_container_name_defaults_and_overrides() -> None: + assert db_lookup.EndurainDatabase()._container == "endurain-postgres" + assert db_lookup.EndurainDatabase(container="other")._container == "other" + + +def test_container_name_from_environment(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv(db_lookup._CONTAINER_ENV, "from-env") + assert db_lookup.EndurainDatabase()._container == "from-env" + + +def test_default_runner_executes_subprocess(monkeypatch: pytest.MonkeyPatch) -> None: + """The real _run is otherwise unreachable in a unit test.""" + seen: dict[str, object] = {} + + def _fake_run(argv: object, **kwargs: object) -> subprocess.CompletedProcess[str]: + seen["argv"] = argv + seen["input"] = kwargs.get("input") + return _proc(stdout="") + + monkeypatch.setattr(subprocess, "run", _fake_run) + assert db_lookup._run(["docker", "true"], "SELECT 1;").returncode == 0 + assert seen["argv"] == ["docker", "true"] + assert seen["input"] == "SELECT 1;" diff --git a/python_pkg/endurain_import/tests/test_remote_dedupe.py b/python_pkg/endurain_import/tests/test_remote_dedupe.py index c92a0d48..357a6a77 100644 --- a/python_pkg/endurain_import/tests/test_remote_dedupe.py +++ b/python_pkg/endurain_import/tests/test_remote_dedupe.py @@ -109,3 +109,44 @@ def test_user_id_is_configurable( _patch_client(monkeypatch, _Recording(Result(Outcome.OK, 1, "ok"))) assert main() == 0 assert seen == [7] + + +def _patch_database( + monkeypatch: pytest.MonkeyPatch, result: list[datetime] | None +) -> None: + """Stand in for the postgres reader; never touches a real container.""" + from python_pkg.endurain_import import db_lookup + + class _StubDatabase: + def recent_start_times(self, _user_id: int) -> list[datetime] | None: + return result + + monkeypatch.setattr(db_lookup, "EndurainDatabase", _StubDatabase) + + +def test_database_fallback_catches_what_the_api_cannot( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, inbox: Path +) -> None: + """API keys cannot read activities, so postgres answers instead.""" + _file(inbox, _RUN_NAME) + client = _StubMainClient(Result(Outcome.OK, 1, "uploaded"), remote=None) + _env(monkeypatch, tmp_path, inbox, ENDURAIN_NO_DB="0") + _patch_client(monkeypatch, client) + _patch_database(monkeypatch, [_run_start()]) + + assert main() == 0 + assert client.uploads == 0 + + +def test_database_failure_falls_back_to_the_ledger( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, inbox: Path +) -> None: + """Both backends unknown must not block the import.""" + _file(inbox, _RUN_NAME) + client = _StubMainClient(Result(Outcome.OK, 1, "uploaded"), remote=None) + _env(monkeypatch, tmp_path, inbox, ENDURAIN_NO_DB="0") + _patch_client(monkeypatch, client) + _patch_database(monkeypatch, None) + + assert main() == 0 + assert client.uploads == 1 diff --git a/python_pkg/endurain_import/upload.py b/python_pkg/endurain_import/upload.py index 21c781c2..6e356214 100644 --- a/python_pkg/endurain_import/upload.py +++ b/python_pkg/endurain_import/upload.py @@ -112,8 +112,8 @@ class EndurainClient: return None if resp.status_code >= _HTTP_BAD_REQUEST: _logger.warning( - "could not list recent activities: %s (the API key needs the " - "activities:read scope for duplicate detection)", + "could not list recent activities over HTTP: %s (API keys " + "cannot read activities; the database fallback answers instead)", resp.status_code, ) return None