diff --git a/Dockerfile b/Dockerfile index 5d896e7..8f1bea2 100644 --- a/Dockerfile +++ b/Dockerfile @@ -67,4 +67,9 @@ HEALTHCHECK --interval=30s --timeout=5s --start-period=20s --retries=3 \ # Override with `python3.13 engine/build.py` for the CLI renderer or # `python3.13 tools/editor/account_purge_worker.py` for the erasure worker. -CMD ["python3.13", "-m", "gunicorn", "--bind", "0.0.0.0:8000", "--workers", "1", "--timeout", "120", "tools.editor.server:app"] +# --no-control-socket: gunicorn 26 added a management socket that defaults to +# $HOME/.gunicorn/gunicorn.ctl. The read-only rootfs already makes that fail, +# but it fails LOUDLY (an ERROR on every boot) and only incidentally — make +# /app writable one day and the container would quietly start exposing a +# control interface. Refuse it explicitly instead. +CMD ["python3.13", "-m", "gunicorn", "--bind", "0.0.0.0:8000", "--workers", "1", "--timeout", "120", "--no-control-socket", "tools.editor.server:app"] diff --git a/requirements.lock b/requirements.lock index 05d87a2..1581943 100644 --- a/requirements.lock +++ b/requirements.lock @@ -23,13 +23,13 @@ blinker==1.9.0 \ --hash=sha256:b4ce2265a7abece45e7cc896e98dbebe6cead56bcf805a3d23136d145f5445bf \ --hash=sha256:ba0efaa9080b619ff2f3459d1d500c57bddea4a6b424b60a91141db6fd2f08bc # via flask -boto3==1.43.40 \ - --hash=sha256:5fa80de4b4b7bab383dd8c563e235d51fcc7df4502662af56f0a2472c3c15651 \ - --hash=sha256:a7108b9ce25b8f92d1ce96b9e35090794d5c58b219ff2f6a7a65c8986a4ed6f4 +boto3==1.43.65 \ + --hash=sha256:a8217fb68cae3f8a7575eef395383b68b846d92d29b0e8f6e948e6e9e08dcc3f \ + --hash=sha256:f2331154aee1ae97ece48077d77f41d3bd5ea39eb4e3037030448b58695a3a79 # via -r requirements.txt -botocore==1.43.40 \ - --hash=sha256:0bc9d352267c9e48415c5d7bb61ff05c3f193eac2fc7e69cfd229a05fbab67d6 \ - --hash=sha256:2085a4314cfd2c8bc1d08ab8039f76c92e99278db0d2a0e2437010526d5d5d70 +botocore==1.43.65 \ + --hash=sha256:41a13d9a1954d786f90cdbba0be0953fe9b9389f240b5ff71340c4706ea40135 \ + --hash=sha256:eea5440cf4b850d0f4de4f7eda418c325c87de6d8980873f8b636bb482b31d0f # via # boto3 # s3transfer @@ -517,9 +517,9 @@ google-genai==2.10.0 \ --hash=sha256:77912cd558cd7dfd5b75c25fd1c609e78d7954dde583331104022a46ea90f9ee \ --hash=sha256:d5350311567ae660c24cbc1752aee4b3d660f89c0106d2dcd2a69978c35afe1e # via -r requirements.txt -gunicorn==23.0.0 \ - --hash=sha256:ec400d38950de4dfd418cff8328b2c8faed0edb0d517d3394e457c317908ca4d \ - --hash=sha256:f014447a0101dc57e294f6c18ca6b40227a4c90e9bdb586042628030cba004ec +gunicorn==26.0.0 \ + --hash=sha256:40233d26a5f0d1872916188c276e21641155111c2853f0c2cd55260aec0d24fc \ + --hash=sha256:ca9346f85e3a4aeeb64d491045c16b9a35647abd37ea15efe53080eb8b090baf # via -r requirements.txt h11==0.16.0 \ --hash=sha256:4e35b956cf45792e4caa5885e69fba00bdbc6ffafbfa020300e549b208ee5ff1 \ diff --git a/requirements.txt b/requirements.txt index 4b6f62a..139c69e 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,11 +23,11 @@ anthropic>=0.117.0,<1.0 google-genai>=1.0,<3.0 # Serving + config -gunicorn>=21.0,<24.0 # WSGI server for the containerized editor -python-dotenv>=1.0,<2.0 # load .env (ANTHROPIC_API_KEY, future Supabase/R2/KEK) +gunicorn>=26.0.0,<27.0 # WSGI server for the containerized editor +python-dotenv>=1.2.2,<2.0 # load .env (ANTHROPIC_API_KEY, future Supabase/R2/KEK) # Cloud: Supabase auth + per-user storage (Stage 2) -supabase>=2.0,<3.0 # supabase-py client; PostgREST runs under the user JWT (RLS) +supabase>=2.31.0,<3.0 # supabase-py client; PostgREST runs under the user JWT (RLS) pyjwt[crypto]>=2.8,<3.0 # verify Supabase JWT via JWKS (ES256/RS256); pulls in cryptography cryptography>=48.0.1,<49.0 # AES-256-GCM envelope encryption for BYO LLM keys (Stage 4) pyasn1>=0.6.4,<0.7.0 # security floor for the ASN.1 dependency used by auth stacks @@ -38,7 +38,7 @@ flask-limiter>=3.5,<4.0 # per-user rate limits on AI + key routes (in-memory # Cloud: PDF storage + delivery via Cloudflare R2 (Stage 8). OPTIONAL at # runtime — when the R2_* env vars are unset the editor streams PDFs directly # (graceful fallback). boto3 talks to R2's S3-compatible API for upload + signed URLs. -boto3>=1.34,<2.0 +boto3>=1.43.51,<2.0 # Dev / tests pytest>=9.1.1,<10.0 diff --git a/supabase/migrations/20260806090000_pdf_cleanup_retry_backoff.sql b/supabase/migrations/20260806090000_pdf_cleanup_retry_backoff.sql new file mode 100644 index 0000000..119a671 --- /dev/null +++ b/supabase/migrations/20260806090000_pdf_cleanup_retry_backoff.sql @@ -0,0 +1,65 @@ +-- OnePager — bound the derived-PDF cleanup outbox retry rate. +-- +-- `record_pdf_artifact_cleanup_failure` incremented `attempt_count` and stamped +-- `last_attempt_at`, but never advanced `not_before`. The worker's batch query +-- selects on `not_before <= now()`, so a row whose R2 DELETE fails permanently +-- (revoked key, deleted bucket, object owned by another account) was re-claimed +-- on every single cycle: a hot loop of failing network calls, matching log +-- spam, and no signal that the row needs attention. +-- +-- Failures now back off exponentially — 1, 2, 4, 8, 16, 32 minutes — capped at +-- one hour so a transient outage still drains promptly once it clears. +-- +-- The row is deliberately NEVER dropped. These entries exist because a user +-- deleted a document or erased an account, so the object must eventually leave +-- R2; discarding the queue row would orphan it permanently and silently. A +-- chronically failing row therefore parks at the hourly cap and stays visible +-- through `attempt_count`, which the worker logs. +-- +-- Signature and return type are unchanged, so this is a plain replace: existing +-- grants and the release schema contract continue to hold. + +create or replace function public.record_pdf_artifact_cleanup_failure( + p_owner_id uuid, + p_cleanup_id uuid, + p_claim_token uuid +) +returns boolean +language plpgsql +security definer +set search_path = '' +set lock_timeout = '2s' +as $$ +declare + updated_id uuid; +begin + if p_owner_id is null or p_cleanup_id is null or p_claim_token is null then + raise exception 'owner, cleanup id, and claim token are required' + using errcode = '22023'; + end if; + + update public.pdf_artifact_cleanup_queue as q + set attempt_count = q.attempt_count + 1, + last_attempt_at = now(), + -- least() caps the doubling; the inner least() keeps the exponent + -- small enough that power() cannot overflow the interval on a row + -- that has been failing for a very long time. + not_before = now() + least( + interval '1 minute' * power(2, least(q.attempt_count, 6)), + interval '1 hour' + ), + claim_token = null, + claimed_at = null + where q.id = p_cleanup_id + and q.owner_id = p_owner_id + and q.claim_token = p_claim_token + returning q.id into updated_id; + + return updated_id is not null; +end; +$$; + +revoke all on function public.record_pdf_artifact_cleanup_failure(uuid, uuid, uuid) + from public, anon, authenticated; +grant execute on function public.record_pdf_artifact_cleanup_failure(uuid, uuid, uuid) + to service_role; diff --git a/tests/conftest.py b/tests/conftest.py index 27831da..6fb49ae 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2149,6 +2149,22 @@ def _review_token_secret_for_tests(monkeypatch): monkeypatch.setenv("ONEPAGER_REVIEW_TOKEN_SECRET", "onepager-test-review-secret") +@pytest.fixture(autouse=True) +def _reset_pending_cleanup_throttle(): + """Keep the read-path cleanup drain deterministic across the session. + + `server` is imported once per session, so its rate-limit timestamp would + otherwise carry from whichever test drained the outbox first and silently + suppress the drain in every test that ran within the next minute. + """ + import importlib + + try: + importlib.import_module("server")._reset_pending_cleanup_throttle() + except Exception: # noqa: BLE001 — suites that never import server + pass + + @pytest.fixture def server_module(): import importlib diff --git a/tests/test_container_hardening.py b/tests/test_container_hardening.py index 7d72c9c..e9d5196 100644 --- a/tests/test_container_hardening.py +++ b/tests/test_container_hardening.py @@ -113,6 +113,26 @@ def test_release_compose_removes_builds_and_requires_immutable_images(): assert "onepager-caddy:2.11.4" not in release +def test_app_refuses_the_gunicorn_control_socket(): + """No management socket in the production image. + + gunicorn 26 added a control socket defaulting to $HOME/.gunicorn. The + read-only rootfs makes creating it fail, so the container is safe today — + but only incidentally, and it logs an ERROR on every boot. Make /app + writable and the container would quietly start exposing a control + interface, so the refusal is explicit. + """ + dockerfile = _read("Dockerfile") + + assert "--no-control-socket" in dockerfile + cmd = next( + line for line in dockerfile.splitlines() + if line.startswith("CMD [") and "gunicorn" in line + ) + assert '"--no-control-socket"' in cmd, cmd + assert "--control-socket" not in cmd.replace("--no-control-socket", "") + + def test_workflow_pull_policy_assertions_match_the_compose_service_count(): """Keep the CI/deploy pull_policy gates tied to the real service list. diff --git a/tests/test_pdf_cleanup_worker.py b/tests/test_pdf_cleanup_worker.py index 8daca67..24f29fe 100644 --- a/tests/test_pdf_cleanup_worker.py +++ b/tests/test_pdf_cleanup_worker.py @@ -817,3 +817,75 @@ def test_startup_error_log_never_contains_exception_message( assert worker_mod.main([], {}) == 2 assert "RuntimeError" in caplog.text assert secret not in caplog.text + + +class TestReadPathDrainIsThrottled: + """The in-request outbox drain must not scale with library read traffic. + + The dedicated worker owns the durable queue; the drain inside + ``GET /api/cvs`` and ``GET /api/cover-letters`` is only the fallback for + deployments that do not run it. Unthrottled, every list request could block + on up to ten sequential R2 deletes, putting object-store latency directly on + the user's library view. + """ + + def _server(self, monkeypatch): + import server as server_mod + + monkeypatch.setattr(server_mod, "_last_pending_cleanup_at", 0.0) + monkeypatch.setattr(server_mod, "_artifact_cleanup_applies", lambda: True) + return server_mod + + def test_batch_is_small_and_drain_is_rate_limited(self, monkeypatch): + server_mod = self._server(monkeypatch) + calls: list[int] = [] + + def _drain(*, limit: int) -> int: + calls.append(limit) + return 0 + + monkeypatch.setattr( + server_mod.pdf_store, "retry_pending_artifact_cleanup", _drain + ) + clock = {"now": 1_000.0} + monkeypatch.setattr(server_mod.time, "monotonic", lambda: clock["now"]) + + server_mod._retry_pending_pdf_artifacts_best_effort() + server_mod._retry_pending_pdf_artifacts_best_effort() + server_mod._retry_pending_pdf_artifacts_best_effort() + assert calls == [server_mod._PENDING_CLEANUP_BATCH], calls + assert server_mod._PENDING_CLEANUP_BATCH <= 3 + + clock["now"] += server_mod._PENDING_CLEANUP_MIN_INTERVAL_S + 1 + server_mod._retry_pending_pdf_artifacts_best_effort() + assert len(calls) == 2, calls + + def test_a_failing_backend_does_not_retry_on_every_read(self, monkeypatch): + """The throttle stamps before the attempt, not after a success.""" + server_mod = self._server(monkeypatch) + calls: list[int] = [] + + def _boom(*, limit: int) -> int: + calls.append(limit) + raise RuntimeError("object store unavailable") + + monkeypatch.setattr( + server_mod.pdf_store, "retry_pending_artifact_cleanup", _boom + ) + monkeypatch.setattr(server_mod.time, "monotonic", lambda: 2_000.0) + + for _ in range(5): + assert server_mod._retry_pending_pdf_artifacts_best_effort() == 0 + assert len(calls) == 1, calls + + def test_demo_shell_never_drains(self, monkeypatch): + server_mod = self._server(monkeypatch) + monkeypatch.setattr(server_mod, "_artifact_cleanup_applies", lambda: False) + + def _never(**_kwargs): + raise AssertionError("demo shell reached the durable cleanup queue") + + monkeypatch.setattr( + server_mod.pdf_store, "retry_pending_artifact_cleanup", _never + ) + assert server_mod._retry_pending_pdf_artifacts_best_effort() == 0 diff --git a/tools/db/migration_probes.sql b/tools/db/migration_probes.sql index 8b08b67..c158034 100644 --- a/tools/db/migration_probes.sql +++ b/tools/db/migration_probes.sql @@ -216,6 +216,78 @@ begin failures := array_append(failures, 'a plain source_db label was incorrectly dropped'); end if; + --------------------------------------------------------------------------- + -- A failing cleanup row must back off, not spin. Before the backoff landed, + -- record_pdf_artifact_cleanup_failure bumped attempt_count but left + -- not_before alone, so the worker re-claimed the same doomed row every cycle. + --------------------------------------------------------------------------- + declare + cleanup_id uuid; + token constant uuid := '00000000-0000-4000-8000-0000000000ff'; + delay_first interval; + delay_later interval; + begin + insert into public.pdf_artifact_cleanup_queue ( + owner_id, document_id, r2_key, content_hash, claim_token, claimed_at) + values ( + owner, + '00000000-0000-4000-8000-0000000000aa', + 'probe/owner/doc.pdf', + repeat('a', 64), + token, + now()) + returning id into cleanup_id; + + checks := checks + 1; + if not public.record_pdf_artifact_cleanup_failure(owner, cleanup_id, token) then + failures := array_append(failures, 'failure accounting did not update the queue row'); + end if; + + select q.not_before - now() into delay_first + from public.pdf_artifact_cleanup_queue q where q.id = cleanup_id; + + checks := checks + 1; + if delay_first is null or delay_first <= interval '0' then + failures := array_append(failures, + format('first failure did not defer the row: %s', delay_first)); + end if; + + -- Simulate a row that has already failed repeatedly; the delay must grow. + update public.pdf_artifact_cleanup_queue + set attempt_count = 4, claim_token = token, claimed_at = now() + where id = cleanup_id; + perform public.record_pdf_artifact_cleanup_failure(owner, cleanup_id, token); + select q.not_before - now() into delay_later + from public.pdf_artifact_cleanup_queue q where q.id = cleanup_id; + + checks := checks + 1; + if delay_later is null or delay_later <= delay_first then + failures := array_append(failures, + format('backoff did not grow with attempts: first=%s later=%s', + delay_first, delay_later)); + end if; + + -- Capped, so a long-failing row still retries hourly rather than never. + update public.pdf_artifact_cleanup_queue + set attempt_count = 40, claim_token = token, claimed_at = now() + where id = cleanup_id; + perform public.record_pdf_artifact_cleanup_failure(owner, cleanup_id, token); + + checks := checks + 1; + select q.not_before - now() into delay_later + from public.pdf_artifact_cleanup_queue q where q.id = cleanup_id; + if delay_later > interval '1 hour' + interval '1 minute' then + failures := array_append(failures, + format('backoff is unbounded: %s', delay_later)); + end if; + + -- The row must survive: it exists because an object still needs deleting. + checks := checks + 1; + if not exists (select 1 from public.pdf_artifact_cleanup_queue where id = cleanup_id) then + failures := array_append(failures, 'a failing cleanup row was discarded'); + end if; + end; + --------------------------------------------------------------------------- -- Every public table carries RLS. --------------------------------------------------------------------------- diff --git a/tools/editor/server.py b/tools/editor/server.py index 39a97ff..b90a9e5 100644 --- a/tools/editor/server.py +++ b/tools/editor/server.py @@ -2002,12 +2002,49 @@ def _delete_detached_document_pdf_artifacts(rows: list[dict[str, Any]]) -> int: raise _ArtifactCleanupUnavailable(document_deleted=True) from exc +# Library reads are a hot path, and draining the outbox costs one R2 round-trip +# per row. The dedicated pdf-cleanup-worker owns the durable queue; this in- +# request drain is only the fallback for deployments that do not run it, so it +# is throttled per process rather than firing on every list request. +_PENDING_CLEANUP_MIN_INTERVAL_S = 60.0 +_PENDING_CLEANUP_BATCH = 3 +_last_pending_cleanup_at = 0.0 + + +def _reset_pending_cleanup_throttle() -> None: + """Allow the next library read to drain the outbox immediately. + + The throttle is process-global by design, which makes it order-dependent + under a test session sharing one imported module. Tests reset it so the + drain contract is exercised deterministically rather than depending on + which test happened to consume the window first. + """ + global _last_pending_cleanup_at + _last_pending_cleanup_at = 0.0 + + def _retry_pending_pdf_artifacts_best_effort() -> int: - """Opportunistically consume a small durable cleanup batch on library reads.""" + """Opportunistically consume a small durable cleanup batch on library reads. + + Bounded twice over: at most ``_PENDING_CLEANUP_BATCH`` objects, and at most + once every ``_PENDING_CLEANUP_MIN_INTERVAL_S`` seconds per process. Before + the throttle, every ``GET /api/cvs`` and ``GET /api/cover-letters`` could + block on up to ten sequential object deletes, so a slow or failing R2 put + that latency straight onto the user's library view. + """ + global _last_pending_cleanup_at if not _artifact_cleanup_applies(): return 0 + now = time.monotonic() + if now - _last_pending_cleanup_at < _PENDING_CLEANUP_MIN_INTERVAL_S: + return 0 + # Stamp before the attempt: a failing backend must not be retried on every + # subsequent read just because it never reached the success path. + _last_pending_cleanup_at = now try: - return pdf_store.retry_pending_artifact_cleanup(limit=10) + return pdf_store.retry_pending_artifact_cleanup( + limit=_PENDING_CLEANUP_BATCH + ) except Exception as exc: # noqa: BLE001 - queue remains durable for retry app.logger.warning( "Pending PDF object cleanup deferred: %s",