diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..cc183ba --- /dev/null +++ b/.dockerignore @@ -0,0 +1,23 @@ +# The image is built from pyproject.toml, README.md, LICENSE, src/ and +# alembic.ini. Everything else stays out of the build context, and .env above +# all: configuration reaches a container through its environment, never a file +# copied into a layer. +.git +.github +.claude +.context +.env +.env.* +.venv +venv +.mypy_cache +.pytest_cache +.ruff_cache +**/__pycache__ +*.pyc +build +dist +site +docs +tests +*.db diff --git a/Dockerfile b/Dockerfile new file mode 100644 index 0000000..350a0d5 --- /dev/null +++ b/Dockerfile @@ -0,0 +1,50 @@ +# One image for every Hymical Forms process. The command decides which one runs: +# +# (default) the API, on port 8000 +# python -m hymical_forms.worker the delivery worker +# alembic upgrade head apply migrations, then exit +# python -m hymical_forms.cli ... operator commands (keys, retention cleanup) +# +# Configuration comes from FORMS_* environment variables only. Nothing secret is +# baked into the image. See docs/operations/deployment.md. + +FROM python:3.13-slim AS build + +WORKDIR /src +# Only what the wheel is built from, so a change to the docs or tests does not +# invalidate this layer. +COPY pyproject.toml README.md LICENSE ./ +COPY src ./src +RUN pip wheel --no-cache-dir --no-deps --wheel-dir /wheels . + + +FROM python:3.13-slim + +ENV PYTHONDONTWRITEBYTECODE=1 \ + PYTHONUNBUFFERED=1 \ + PIP_NO_CACHE_DIR=1 \ + PIP_DISABLE_PIP_VERSION_CHECK=1 + +# A fixed numeric UID, so a Kubernetes runAsNonRoot check can verify it without +# resolving a name. +RUN groupadd --system --gid 10001 forms \ + && useradd --system --uid 10001 --gid forms --no-create-home \ + --home-dir /app --shell /usr/sbin/nologin forms + +WORKDIR /app + +COPY --from=build /wheels /wheels +RUN pip install /wheels/*.whl && rm -rf /wheels + +# Alembic looks for its ini file in the working directory. It names the +# migrations by package, so the ones shipped inside the installed wheel are used. +COPY alembic.ini ./ + +USER 10001:10001 + +EXPOSE 8000 + +# --no-proxy-headers: uvicorn would otherwise rewrite the client address from +# X-Forwarded-For when the peer is 127.0.0.1, and FORMS_TRUSTED_PROXY_HOPS is +# meant to be the only thing that decides whether that header is believed. +CMD ["uvicorn", "hymical_forms.main:app", "--host", "0.0.0.0", "--port", "8000", "--no-proxy-headers"] diff --git a/compose.yaml b/compose.yaml new file mode 100644 index 0000000..3a0c999 --- /dev/null +++ b/compose.yaml @@ -0,0 +1,90 @@ +# An example deployment: PostgreSQL, a one-shot migration, the API and one +# worker. A starting point to adapt, not a hardened production stack. See +# docs/operations/deployment.md. +# +# POSTGRES_PASSWORD=... docker compose up -d --build +# +# The password ends up inside FORMS_DATABASE_URL, so if it contains characters +# that are special in a URL (@ : / % ? #), percent-encode them or pick one that +# does not. + +x-forms-environment: &forms-environment + FORMS_DATABASE_URL: postgresql+psycopg://forms:${POSTGRES_PASSWORD:?set POSTGRES_PASSWORD}@postgres:5432/forms + +services: + postgres: + image: postgres:17 + environment: + POSTGRES_USER: forms + POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:?set POSTGRES_PASSWORD} + POSTGRES_DB: forms + volumes: + - postgres-data:/var/lib/postgresql/data + healthcheck: + test: ["CMD", "pg_isready", "-U", "forms", "-d", "forms"] + interval: 5s + timeout: 5s + retries: 10 + restart: unless-stopped + # Deliberately no published port: only the services below need to reach it. + + # All three application services share one image, built from this checkout. + # Each declares the build so that none of them tries to pull the tag instead. + + # Applies migrations and exits. The API and the worker wait for it to finish + # successfully, and would refuse to start against an unmigrated schema anyway. + migrate: + build: . + image: hymical-forms:local + command: ["alembic", "upgrade", "head"] + environment: + <<: *forms-environment + depends_on: + postgres: + condition: service_healthy + restart: "no" + + api: + build: . + image: hymical-forms:local + environment: + <<: *forms-environment + # Uncomment and set to the real number of proxies once one stands in front + # of the API. Never higher. See docs/operations/reverse-proxy.md. + # FORMS_TRUSTED_PROXY_HOPS: "1" + ports: + # Bound to loopback, so the API is reached through a reverse proxy on this + # host rather than exposed directly. + - "127.0.0.1:8000:8000" + depends_on: + migrate: + condition: service_completed_successfully + healthcheck: + # Liveness only: /health does not touch the database. + test: + - CMD + - python + - -c + - "import urllib.request; urllib.request.urlopen('http://127.0.0.1:8000/health', timeout=3)" + interval: 10s + timeout: 5s + retries: 3 + restart: unless-stopped + + worker: + build: . + image: hymical-forms:local + command: ["python", "-m", "hymical_forms.worker"] + environment: + <<: *forms-environment + depends_on: + migrate: + condition: service_completed_successfully + # SIGTERM lets the worker finish the batch it holds, which takes up to one + # attempt deadline (connect plus read timeout, 15 seconds by default). Docker's + # own default of 10 seconds would kill it part way through. + stop_grace_period: 30s + restart: unless-stopped + +volumes: + postgres-data: diff --git a/docs/api/submissions.md b/docs/api/submissions.md index 816dfba..314c418 100644 --- a/docs/api/submissions.md +++ b/docs/api/submissions.md @@ -87,7 +87,9 @@ Reports that the API process is running. This is a liveness signal only. It does not check the database, so it stays answerable while the database is down, which is what makes it useful for deciding -whether to restart the process. It is not a readiness check. +whether to restart the process. It is not a readiness check, and there is no +separate readiness route. See +[Deployment](../operations/deployment.md#health-checks). | Status | Cause | | --- | --- | diff --git a/docs/architecture/delivery-semantics.md b/docs/architecture/delivery-semantics.md index b4d6eba..82d3f92 100644 --- a/docs/architecture/delivery-semantics.md +++ b/docs/architecture/delivery-semantics.md @@ -117,8 +117,9 @@ It does not remove the window, and set too low it **creates** duplicates: another worker can claim a delivery that is still in flight. The lease must comfortably outlast `FORMS_WEBHOOK_CONNECT_TIMEOUT_SECONDS` and -`FORMS_WEBHOOK_READ_TIMEOUT_SECONDS` combined. The defaults leave a wide margin, -60 seconds against 15. See [Worker](../operations/worker.md). +`FORMS_WEBHOOK_READ_TIMEOUT_SECONDS` combined, which is the overall deadline every +attempt is held to. The defaults leave a wide margin, 60 seconds against 15. See +[Worker](../operations/worker.md). ## Two attempt counters diff --git a/docs/development/contributing.md b/docs/development/contributing.md index 08828ed..fe322ad 100644 --- a/docs/development/contributing.md +++ b/docs/development/contributing.md @@ -135,4 +135,5 @@ Guidelines that keep the site coherent: ## Related - [Testing](testing.md) +- [Releasing](releasing.md) - [Architecture overview](../architecture/overview.md) diff --git a/docs/development/releasing.md b/docs/development/releasing.md new file mode 100644 index 0000000..1046d99 --- /dev/null +++ b/docs/development/releasing.md @@ -0,0 +1,171 @@ +# Releasing + +A release is a deliberate act by a maintainer, not something CI or an agent does +on its own. This page is the checklist that act follows, so that the version a +build reports, the tag it was cut from, and the release notes that describe it +all agree. + +## Where the version lives + +There is exactly one place the version is written down: + +```python +# src/hymical_forms/__init__.py +__version__ = "0.2.0" +``` + +Everything else reads it. `pyproject.toml` declares the version as dynamic and +Hatch takes it from that file, and the running service reports it in three +places: + +- the `GET /health` response (`version`) +- the OpenAPI document (`info.version`) +- the `User-Agent` of every outbound webhook (`Hymical-Forms/`) + +A tag that points at a commit whose `__version__` disagrees with it produces a +build that misreports itself in all three. That is what the checklist below is +for. + +## Checklist + +Work through it in order, on a release branch off an up-to-date `main`. + +### 1. Decide the version + +Pick the next version from what changed since the last tag: + +```bash +git log --oneline "$(git describe --tags --abbrev=0)"..main +``` + +A fix-only release bumps the patch number. Anything that changes the webhook +payload or the `Hymical-Signature` header is a breaking change to every receiver +and must not be slipped into a patch release. + +### 2. Bump the version + +Edit `src/hymical_forms/__init__.py`, then update every place the documentation +quotes the version literally: + +```bash +grep -rn "" docs README.md src +``` + +`docs/api/submissions.md` shows a sample `/health` body with the version in it. +Old release notes keep the version they describe; do not rewrite history there. + +### 3. Write the release notes + +Add `docs/releases/v.md`, following the shape of the previous one: +what the release is, what changed, what was verified, and the limitations worth +knowing before deploying it. Then add it to the `Release Notes` section of the +`nav:` in `mkdocs.yml`, newest first, or a strict docs build will fail on an +omitted page. + +If the release contains a migration, say so, name the revision, and say whether +it is safe to apply while the previous build is still running. The default answer +is no: [deploy order](../operations/deployment.md#deploy-order) is stop, migrate, +start. + +### 4. Verify + +Every one of these must pass on the release branch: + +```bash +pytest +ruff check . +ruff format --check . +mypy +``` + +The PostgreSQL integration suite, against a real disposable database. A release +is never cut on the fast suite alone: it is the only suite that exercises row +locking, `SKIP LOCKED` claiming, constraint races and the migrations themselves. + +```bash +export HYMICAL_TEST_POSTGRES_URL=postgresql+psycopg://forms:forms@localhost:5432/forms_test +alembic -x database_url="$HYMICAL_TEST_POSTGRES_URL" upgrade head +pytest tests/integration -m postgres +``` + +The documentation: + +```bash +pip install -e ".[docs]" +mkdocs build --strict +``` + +The packages, from a clean tree, and the container image: + +```bash +pip install build +python -m build +docker build -t hymical-forms: . +``` + +Install the wheel into a fresh virtual environment and confirm it reports the +version you are about to tag: + +```bash +python -c "import hymical_forms; print(hymical_forms.__version__)" +``` + +### 5. Merge + +Open a pull request for the release branch and merge it once CI is green. CI runs +the fast suite, the PostgreSQL suite, the linters and the strict docs build, so a +green pull request repeats step 4 on a machine other than yours. + +### 6. Tag the merge commit + +Tag the commit that landed on `main`, not a commit on the release branch, and +check the version at that exact commit before pushing the tag: + +```bash +git switch main +git pull +git show HEAD:src/hymical_forms/__init__.py | grep __version__ +git tag -a v -m "v" +git push origin v +``` + +The tag name is `v` followed by exactly the string in `__version__`. Use an +annotated tag (`-a`), so the tag records who cut it and when. + +A pushed tag is never moved or deleted. If a tag turns out to be wrong, the fix is +a new release with a new version, not a retag: someone may already have built +from the old one. + +### 7. Publish + +Publishing a GitHub release, a package or an image is a separate decision from +tagging, and is made by a maintainer. Nothing in CI publishes on a tag. + +## Known discrepancy: v0.2.1 + +The checklist above exists because the `v0.2.1` tag did not follow it. + +| | `v0.2.0` | `v0.2.1` | +| --- | --- | --- | +| Commit | `8e99004` (release prep merge) | `235f8af` (delivery claim fencing merge) | +| `__version__` at that commit | `0.2.0` | `0.2.0` | +| Release notes | [v0.2.0](../releases/v0.2.0.md) | none | +| Tag type | lightweight | lightweight | + +So a build from the `v0.2.1` tag reports itself as `0.2.0` in `/health`, in +OpenAPI and in its webhook `User-Agent`, and there is no page describing what +`v0.2.1` changed. What it does contain over `v0.2.0` is one change: delivery claim +fencing, which stops a worker whose lease has expired from overwriting the state +recorded by the worker that reclaimed the delivery (see +[Delivery semantics](../architecture/delivery-semantics.md#who-owns-a-claim)). + +The tag has already been pushed, so it is not moved or rewritten. The next +release resolves the discrepancy by taking a version above `0.2.1`, bumping +`__version__` to it, and having its release notes cover everything since +`v0.2.0`, the fencing change included. + +## Related + +- [Contributing](contributing.md) +- [Testing](testing.md) +- [Deployment](../operations/deployment.md) diff --git a/docs/guides/idempotency.md b/docs/guides/idempotency.md index 48e8a47..1fddf42 100644 --- a/docs/guides/idempotency.md +++ b/docs/guides/idempotency.md @@ -41,7 +41,9 @@ winner, not a check in application code. See [Concurrency](../architecture/concurrency.md). A replay reports the same `delivery.queued` as the original and does **not** -queue a second delivery. One submission owes at most one delivery, enforced by a +queue a second delivery. It describes the stored submission, so adding or +removing the endpoint's webhook after the original request does not change what a +replay reports. One submission owes at most one delivery, enforced by a unique constraint on the submission. ## Conflict semantics diff --git a/docs/guides/webhooks.md b/docs/guides/webhooks.md index b19efc4..5785227 100644 --- a/docs/guides/webhooks.md +++ b/docs/guides/webhooks.md @@ -181,8 +181,15 @@ Do not enable it in production. | `FORMS_WEBHOOK_CONNECT_TIMEOUT_SECONDS` | `5` | Wait for the destination to accept a connection | | `FORMS_WEBHOOK_READ_TIMEOUT_SECONDS` | `10` | Wait for the destination to respond | -Both must stay comfortably below `FORMS_WORKER_LEASE_SECONDS`, or another worker -could claim a delivery that is still in flight. See [Worker](../operations/worker.md). +Each of these bounds a single wait, so on top of them every attempt has an overall +deadline of the two combined (15 seconds by default). A destination that answers +one byte at a time cannot hold a request open past it; such an attempt is +recorded as a `timeout` and retried. Only the response status is used: the +response body is never read. + +The deadline must stay comfortably below `FORMS_WORKER_LEASE_SECONDS`, or another +worker could claim a delivery that is still in flight. The worker logs a warning +at startup if it does not. See [Worker](../operations/worker.md). ## Related diff --git a/docs/operations/configuration-reference.md b/docs/operations/configuration-reference.md index 367a98a..29cc5a0 100644 --- a/docs/operations/configuration-reference.md +++ b/docs/operations/configuration-reference.md @@ -56,7 +56,7 @@ value. It must be at least 16 characters. | Variable | Default | Meaning | | --- | --- | --- | | `FORMS_WEBHOOK_CONNECT_TIMEOUT_SECONDS` | `5` | Wait for a webhook to accept a connection | -| `FORMS_WEBHOOK_READ_TIMEOUT_SECONDS` | `10` | Wait for a webhook to respond | +| `FORMS_WEBHOOK_READ_TIMEOUT_SECONDS` | `10` | Wait for a webhook to respond. With the connect timeout, also the overall deadline for one attempt | | `FORMS_WEBHOOK_MAX_ATTEMPTS` | `5` | Attempts before a delivery is given up on | | `FORMS_WEBHOOK_RETRY_INITIAL_SECONDS` | `10` | Wait before the second attempt; later waits double | | `FORMS_WEBHOOK_RETRY_MAX_SECONDS` | `3600` | Cap on the wait between attempts | diff --git a/docs/operations/deployment.md b/docs/operations/deployment.md new file mode 100644 index 0000000..0156ee3 --- /dev/null +++ b/docs/operations/deployment.md @@ -0,0 +1,154 @@ +# Deployment + +Hymical Forms is two long-running processes, the API and the worker, plus a +one-shot migration step, all against one PostgreSQL database. The repository +ships a `Dockerfile` that builds one image for all three, and a `compose.yaml` +that wires them together as a working example. + +!!! warning "An example, not a hardened stack" + + `compose.yaml` is a starting point to read and adapt. It does not terminate + TLS, does not back up the database, and runs PostgreSQL in a container on the + same host. Decide each of those for your own deployment. + +## The image + +```bash +docker build -t hymical-forms . +``` + +The image installs the package as a wheel, runs as an unprivileged user (UID +`10001`), and holds no configuration: every setting comes from `FORMS_*` +environment variables, and `.env` files are kept out of the build context. The +command decides which process a container is: + +| Command | Process | +| --- | --- | +| *(default)* | The API, listening on port `8000` | +| `python -m hymical_forms.worker` | The delivery worker | +| `alembic upgrade head` | Apply migrations, then exit | +| `python -m hymical_forms.cli ...` | Operator commands: management keys, retention cleanup | + +The default command starts uvicorn with `--no-proxy-headers`, so uvicorn never +rewrites the client address from `X-Forwarded-For` on its own. +`FORMS_TRUSTED_PROXY_HOPS` stays the only setting that decides whether that header +is believed. See [Reverse proxy](reverse-proxy.md). + +## Running the example + +```bash +export POSTGRES_PASSWORD='choose-a-real-password' +docker compose up -d --build +``` + +That starts, in order: + +1. `postgres`, and waits until it accepts connections. +2. `migrate`, which runs `alembic upgrade head` and exits. +3. `api` and `worker`, once `migrate` has exited successfully. + +The password is placed inside `FORMS_DATABASE_URL`, so if it contains a character +that is special in a URL (`@ : / % ? #`), percent-encode it or choose another. + +The API is published on `127.0.0.1:8000` only, on the assumption that a reverse +proxy on the same host fronts it. PostgreSQL is not published at all. + +### The first management key + +Keys are minted by the operator CLI and never over HTTP. Run it as a one-off +container against the same database: + +```bash +docker compose run --rm api python -m hymical_forms.cli create-key --name admin +``` + +The key is printed once. Store it; it cannot be shown again. See +[Authentication](../api/authentication.md). + +### Retention cleanup + +Nothing is deleted automatically. Run the sweep the same way, from whatever +scheduler you already use: + +```bash +docker compose run --rm api python -m hymical_forms.cli cleanup-submissions --dry-run +docker compose run --rm api python -m hymical_forms.cli cleanup-submissions +``` + +See [Submission retention](retention.md). + +## Deploy order + +**Stop the old processes, migrate, start the new ones.** Migrating while an old +build is still running is only safe if the change happens to be backwards +compatible, and this project does not promise that for any particular migration. +See [Database migrations](migrations.md). + +A plain `docker compose up -d --build` does **not** give you that order on an +upgrade: Compose would run the new `migrate` while the old `api` and `worker` were +still serving. Do it in explicit steps instead: + +```bash +docker compose build +docker compose stop api worker +docker compose run --rm migrate +docker compose up -d api worker +``` + +`docker compose stop` sends `SIGTERM`. The worker finishes the batch it holds, +which takes at most one attempt deadline (connect plus read timeout, 15 seconds by +default), so the example gives it a 30 second `stop_grace_period`. Keep that above +the deadline if you raise the timeouts; a worker killed mid-batch is not a +correctness problem, but its claimed deliveries are sent again once their lease +expires. See [Worker](worker.md#stopping). + +Submissions sent while the API is stopped fail outright (a refused connection, +or an error from your proxy); none is accepted and then lost. There is no +zero-downtime deploy, and none is claimed. + +## Health checks + +`GET /health` is a **liveness** check. It reports that the API process is running +and does not touch the database, so it keeps answering while PostgreSQL is down. +That is what the example's `api` healthcheck uses, and it is the right signal for +deciding whether to restart the process. + +There is **no readiness endpoint**. What exists instead: + +- **At startup**, the API and the worker both verify that the database is + reachable and at the revision the build expects, and exit if it is not. An API + that is up and answering `/health` passed that check when it started. +- **After startup**, if the database becomes unreachable, the API keeps answering + `/health` while every route that needs the database returns an opaque `503`. A + load balancer that routes on `/health` will keep sending it traffic in that + state. +- **The worker has no HTTP surface.** Its health is visible only in its logs and in + whether due deliveries are being drained. + +Leaving readiness out is a decision for v1.0, not an oversight. Every route is +deliberately either public (`POST /f/{endpoint_id}`, `GET /health`) or behind a +management key, and a database-checking route would be a new public one, so the +public surface stays at those two routes. If you need traffic withdrawn during a +database outage, have your load balancer or monitoring watch for `503` responses +from the API. See [Limitations](../reference/limitations.md#operations). + +## Running without containers + +The image is a convenience, not a requirement. Any process supervisor works, as +long as it: + +- runs the API (`uvicorn hymical_forms.main:app`) and the worker + (`python -m hymical_forms.worker`) as separate, independently restarted + processes; +- stops the worker with `SIGTERM` and waits longer than one attempt deadline + before killing it; +- runs `alembic upgrade head` as its own step, between stopping the old processes + and starting the new ones, never from inside either process. + +## Related + +- [Database migrations](migrations.md) +- [Worker](worker.md) +- [Reverse proxy](reverse-proxy.md) +- [Configuration reference](configuration-reference.md) +- [Releasing](../development/releasing.md) diff --git a/docs/operations/migrations.md b/docs/operations/migrations.md index e837da0..670d137 100644 --- a/docs/operations/migrations.md +++ b/docs/operations/migrations.md @@ -152,5 +152,6 @@ as migrate-then-restart. ## Related - [Installation](../getting-started/installation.md) for first-time setup +- [Deployment](deployment.md) for the deploy order with the container image - [Testing](../development/testing.md) for the migration test suite - [Limitations](../reference/limitations.md) diff --git a/docs/operations/worker.md b/docs/operations/worker.md index 35c6093..73e933a 100644 --- a/docs/operations/worker.md +++ b/docs/operations/worker.md @@ -49,11 +49,13 @@ There is no coordination service and no broker. PostgreSQL is the queue. See !!! danger "The lease must outlast a delivery attempt" - A batch is delivered concurrently, so it takes about as long as its slowest - single delivery rather than the sum. But if `FORMS_WORKER_LEASE_SECONDS` were - set below `FORMS_WEBHOOK_CONNECT_TIMEOUT_SECONDS` and - `FORMS_WEBHOOK_READ_TIMEOUT_SECONDS` combined, another worker could claim a - delivery that is still in flight and send it twice. + Every attempt has an overall deadline of + `FORMS_WEBHOOK_CONNECT_TIMEOUT_SECONDS` plus + `FORMS_WEBHOOK_READ_TIMEOUT_SECONDS`, however slowly the destination answers. + A batch is delivered concurrently, so it takes about as long as that deadline + at most rather than the sum. But if `FORMS_WORKER_LEASE_SECONDS` were set at + or below the deadline, another worker could claim a delivery that is still in + flight and send it twice. The worker logs a warning at startup when it is. The defaults leave a wide margin (60 seconds against 15). Keep it that way if you change them. @@ -75,14 +77,30 @@ the database is at migration '0003' but this build expects '0004'. Run 'alembic upgrade head' before starting. ``` -Migrating is an operator action, run when the operator chooses. See -[Database migrations](migrations.md). +The worker exits with status 1 rather than polling. Migrating is an operator +action, run when the operator chooses. See [Database migrations](migrations.md). + +## Stopping + +`SIGTERM` or `SIGINT` asks the worker to finish the batch it is on and exit. A +batch in flight ends within one attempt deadline, after which its outcomes are +recorded. A worker killed outright (`SIGKILL`, a crash, a lost host) leaves its +claimed deliveries in `processing`; they become claimable again once their lease +expires and are sent again, which is the at-least-once case described in +[Delivery semantics](../architecture/delivery-semantics.md). + +If recording one attempt fails, for example because the database connection +dropped, the worker logs it and goes on to record the rest of the batch. The +delivery it could not record stays claimed and is retried after its lease, so the +destination may receive it twice. ## Deploy order Stop the old processes, migrate, start the new ones. Migrating while an old build is still running is only safe if the change happens to be backwards compatible, -and this project does not promise that for any particular migration. +and this project does not promise that for any particular migration. See +[Deployment](deployment.md#deploy-order) for the same order with the container +image. ## What the worker does not do diff --git a/docs/reference/limitations.md b/docs/reference/limitations.md index 6d7f5df..d1ef5ef 100644 --- a/docs/reference/limitations.md +++ b/docs/reference/limitations.md @@ -131,6 +131,16 @@ deploy it, and it is kept complete rather than flattering. lock it, and a build whose expected revision does not match the database refuses to start rather than serving against a schema it does not understand. Plan a deploy as migrate-then-restart. +- **There is no readiness check.** `GET /health` is liveness only and never + touches the database. The API and the worker verify the database and its schema + revision once, at startup, and exit if either is wrong; a database lost after + that leaves `/health` answering `200` while database-backed routes return `503`. + This is deliberate for v1.0, to keep the public surface at two routes. + See [Deployment](../operations/deployment.md#health-checks). +- **The container example is an example.** The `Dockerfile` and `compose.yaml` + run every process as a non-root user, but the compose stack does not terminate + TLS, back up PostgreSQL, or ship monitoring. See + [Deployment](../operations/deployment.md). - **A concurrent `PATCH` on one endpoint is last-write-wins.** The row is locked for the transaction on PostgreSQL, so two operators rotating a signing secret at the same moment are serialised and both get an answer that matches what was diff --git a/mkdocs.yml b/mkdocs.yml index 64130a2..15c194c 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -104,6 +104,7 @@ nav: - Deliveries: api/deliveries.md - Errors: api/errors.md - Operations: + - Deployment: operations/deployment.md - Worker: operations/worker.md - Database Migrations: operations/migrations.md - Submission Retention: operations/retention.md @@ -118,6 +119,7 @@ nav: - Development: - Testing: development/testing.md - Contributing: development/contributing.md + - Releasing: development/releasing.md - Reference: - Data Handling: reference/data-handling.md - Limitations: reference/limitations.md diff --git a/src/hymical_forms/api/health.py b/src/hymical_forms/api/health.py index d3836ed..0198d13 100644 --- a/src/hymical_forms/api/health.py +++ b/src/hymical_forms/api/health.py @@ -30,7 +30,10 @@ async def health() -> HealthResponse: report that the api process is running and able to serve requests :returns: a liveness payload naming the service and its version """ - # This is a liveness signal only. Hymical Forms has no external dependencies - # yet, so there is nothing to distinguish readiness from liveness; a separate - # readiness endpoint will arrive with persistence. + # Liveness only, and deliberately free of database access, so it keeps + # answering while PostgreSQL is down and a supervisor does not restart a + # healthy process for an outage it cannot fix. The schema and reachability + # check happens once, at startup (see schema.verify_schema). There is + # deliberately no readiness route for v1.0: it would widen the public surface + # beyond this route and POST /f/{endpoint_id}. return HealthResponse(status="ok", service="hymical-forms", version=__version__) diff --git a/src/hymical_forms/api/submissions.py b/src/hymical_forms/api/submissions.py index da64f43..d104dac 100644 --- a/src/hymical_forms/api/submissions.py +++ b/src/hymical_forms/api/submissions.py @@ -467,7 +467,7 @@ async def submit(endpoint_id: str, request: Request, session: SessionDep) -> Sub received_at=stored.submission.received_at, field_count=stored.submission.field_count, idempotent_replay=stored.replayed, - delivery=DeliveryStatus(queued=webhook is not None), + delivery=DeliveryStatus(queued=stored.delivery_queued), ) diff --git a/src/hymical_forms/delivery.py b/src/hymical_forms/delivery.py index fec5ad0..ca6f237 100644 --- a/src/hymical_forms/delivery.py +++ b/src/hymical_forms/delivery.py @@ -9,6 +9,8 @@ from __future__ import annotations +import asyncio + import httpx2 from hymical_forms import __version__ @@ -53,8 +55,28 @@ def create_webhook_client(settings: Settings) -> httpx2.AsyncClient: ) +def attempt_deadline_seconds(settings: Settings) -> float: + """ + work out the longest one delivery attempt is allowed to take, start to finish + :param settings: active configuration, read for its timeouts + :returns: the overall deadline for one attempt, in seconds + """ + # The client's timeouts each bound a single wait: one connect, one read of + # whatever bytes arrive next. None of them bounds the attempt, so a + # destination that answers one byte at a time could hold a request open for + # as long as it liked, past any lease. Connecting plus one full read timeout + # is the most an attempt is worth waiting for, and it is the figure the + # operations guide tells operators to size the lease against. + return settings.webhook_connect_timeout_seconds + settings.webhook_read_timeout_seconds + + async def deliver( - client: httpx2.AsyncClient, *, url: str, secret: str, body: bytes + client: httpx2.AsyncClient, + *, + url: str, + secret: str, + body: bytes, + deadline_seconds: float, ) -> DeliveryResult: """ make one attempt to deliver a signed payload @@ -62,6 +84,7 @@ async def deliver( :param url: the destination to post to :param secret: the destination's signing secret :param body: the exact bytes to sign and transmit + :param deadline_seconds: the longest the whole attempt may take, see attempt_deadline_seconds :returns: the outcome, never raising for a destination that misbehaves """ # ``content=body`` transmits these bytes verbatim. Passing the payload object @@ -74,22 +97,34 @@ async def deliver( "User-Agent": USER_AGENT, } + # The response is streamed and its body never read. The status is the whole + # answer, and reading a body nobody looks at would let the destination decide + # how much memory an attempt takes and how long it lasts. try: - response = await client.post(url, content=body, headers=headers) + async with ( + asyncio.timeout(deadline_seconds), + client.stream("POST", url, content=body, headers=headers) as response, + ): + status = response.status_code except httpx2.TimeoutException as exc: return DeliveryResult(DeliveryOutcome.TIMEOUT, error=_describe("timed out", exc)) except httpx2.RequestError as exc: return DeliveryResult( DeliveryOutcome.NETWORK_ERROR, error=_describe("could not connect", exc) ) + except TimeoutError: + return DeliveryResult( + DeliveryOutcome.TIMEOUT, + error=f"timed out: no response within the {deadline_seconds:g}s attempt deadline", + ) - if 200 <= response.status_code < 300: - return DeliveryResult(DeliveryOutcome.SUCCEEDED, response_status=response.status_code) + if 200 <= status < 300: + return DeliveryResult(DeliveryOutcome.SUCCEEDED, response_status=status) return DeliveryResult( DeliveryOutcome.HTTP_ERROR, - response_status=response.status_code, - error=f"destination responded with HTTP {response.status_code}", + response_status=status, + error=f"destination responded with HTTP {status}", ) diff --git a/src/hymical_forms/storage.py b/src/hymical_forms/storage.py index 363b093..87c7a35 100644 --- a/src/hymical_forms/storage.py +++ b/src/hymical_forms/storage.py @@ -24,11 +24,10 @@ from collections.abc import Iterator from dataclasses import dataclass from datetime import datetime, timedelta -from typing import Any, TypeVar, cast +from typing import Any, Protocol, Self, TypeVar, cast from sqlalchemy import ( ColumnElement, - Select, and_, delete, func, @@ -67,7 +66,25 @@ # Any submission select, whatever it happens to be selecting. The submission # filters are applied to a listing, an export and a bounded count alike, and # narrowing a select never changes what it returns. -_Statement = TypeVar("_Statement", bound=Select[Any]) +_Statement = TypeVar("_Statement", bound="_Narrowable") + + +class _Narrowable(Protocol): + """ + a select of any shape, which is all the submission filters need from one + """ + + # A protocol rather than a bound on ``Select`` itself, because how a select's + # row type is spelled changed between SQLAlchemy 2.0 and 2.1 and no single + # ``Select[...]`` bound type checks against both. + def where(self, *whereclause: ColumnElement[bool]) -> Self: + """ + narrow the select by further conditions + :param whereclause: the conditions to add + :returns: the narrowed select + """ + ... + # How many rows a streamed export pulls from the server at a time. Bounded so # that an export builds its response as it goes rather than materialising every @@ -242,8 +259,12 @@ class StoredSubmission: the outcome of storing a submission """ + # ``delivery_queued`` describes the stored submission, not the endpoint as it + # is configured now. A replay answers for the submission the key was spent + # on, and the endpoint's webhook may have been added or removed since. submission: Submission replayed: bool + delivery_queued: bool def find_by_idempotency_key( @@ -292,12 +313,12 @@ def store_submission( if idempotency_key is None: _add_submission(session, submission, now=now, webhook=webhook) session.commit() - return StoredSubmission(submission, replayed=False) + return StoredSubmission(submission, replayed=False, delivery_queued=webhook is not None) # Fast path for the ordinary retry, where the first attempt already landed. existing = find_by_idempotency_key(session, submission.endpoint_id, idempotency_key) if existing is not None: - return StoredSubmission(_settle(existing, payload_fingerprint), replayed=True) + return _replayed(session, existing, payload_fingerprint) _add_submission( session, @@ -325,9 +346,27 @@ def store_submission( # The violation was something other than the idempotency constraint, # so it is not ours to interpret and must not be reported as success. raise - return StoredSubmission(_settle(existing, payload_fingerprint), replayed=True) + return _replayed(session, existing, payload_fingerprint) + + return StoredSubmission(submission, replayed=False, delivery_queued=webhook is not None) - return StoredSubmission(submission, replayed=False) + +def _replayed( + session: Session, existing: models.Submission, payload_fingerprint: str | None +) -> StoredSubmission: + """ + describe the earlier submission a retried request resolved to + :param session: the session to query through + :param existing: the submission the key was already spent on + :param payload_fingerprint: digest of the content submitted this time + :returns: the earlier submission, flagged as a replay, with the delivery it actually owes + :raises IdempotencyKeyReused: if the content differs from the earlier attempt + """ + settled = _settle(existing, payload_fingerprint) + owed = session.scalar( + select(models.WebhookDelivery.id).where(models.WebhookDelivery.submission_id == existing.id) + ) + return StoredSubmission(settled, replayed=True, delivery_queued=owed is not None) def _add_submission( diff --git a/src/hymical_forms/worker.py b/src/hymical_forms/worker.py index bea243d..dbb675f 100644 --- a/src/hymical_forms/worker.py +++ b/src/hymical_forms/worker.py @@ -15,6 +15,7 @@ import asyncio import logging import signal +import sys import threading from datetime import datetime from types import FrameType @@ -25,8 +26,9 @@ from hymical_forms import storage, webhooks from hymical_forms.config import Settings from hymical_forms.db import create_engine_from_url, create_session_factory -from hymical_forms.delivery import create_webhook_client, deliver +from hymical_forms.delivery import attempt_deadline_seconds, create_webhook_client, deliver from hymical_forms.models import utcnow +from hymical_forms.schema import SchemaNotReady, verify_schema logger = logging.getLogger(__name__) @@ -57,6 +59,14 @@ async def process_batch( submissions = storage.load_submissions(session, claimed) + # Loading the submissions opened a read transaction, and nothing else will + # end it before the requests below go out. Ending it here is what keeps the + # promise that no transaction is held across somebody else's server. + # Committing rather than rolling back, because a rollback would expire the + # claimed rows and send the recording step back to the database to reload + # them. + session.commit() + # The claim each request is about to be made under, captured before any of # them go out. Completing is fenced on this rather than on whatever the row # says once a response comes back, so a lease that expires mid-request cannot @@ -71,9 +81,16 @@ async def process_batch( job.id: webhooks.serialize_payload(webhooks.build_payload(submissions[job.id])) for job in claimed } + deadline = attempt_deadline_seconds(settings) results = await asyncio.gather( *( - deliver(client, url=job.destination_url, secret=job.signing_secret, body=bodies[job.id]) + deliver( + client, + url=job.destination_url, + secret=job.signing_secret, + body=bodies[job.id], + deadline_seconds=deadline, + ) for job in claimed ) ) @@ -83,9 +100,24 @@ async def process_batch( # would gain nothing real and would make every retry schedule approximate. policy = settings.retry_policy() for job, result in zip(claimed, results, strict=True): - recorded = storage.complete_attempt( - session, job, result, now=now, policy=policy, claim_token=claims[job.id] - ) + try: + recorded = storage.complete_attempt( + session, job, result, now=now, policy=policy, claim_token=claims[job.id] + ) + except Exception: + # Every request in the batch has already gone out. Giving up on the + # rest because one could not be recorded would leave them all to be + # sent again once their leases ran out. This one is left claimed, so + # it is retried after its lease, which is the at-least-once outcome a + # worker crashing here would have produced anyway. + session.rollback() + logger.exception( + "delivery %s attempt was made (%s) but could not be recorded, " + "it will be retried once its lease expires", + job.id, + result.outcome, + ) + continue if recorded.owned: logger.info( "delivery %s attempt %d %s (%s)", @@ -119,6 +151,25 @@ async def run_worker(settings: Settings, *, stop: threading.Event) -> None: :param stop: event that ends the loop once set """ engine = create_engine_from_url(settings.database_url) + try: + # The same check the API and the CLI make. A worker built for another + # revision would claim and complete deliveries under rules the schema no + # longer matches, which is worse than not delivering at all. + verify_schema(engine) + except BaseException: + engine.dispose() + raise + + deadline = attempt_deadline_seconds(settings) + if settings.worker_lease_seconds <= deadline: + logger.warning( + "FORMS_WORKER_LEASE_SECONDS (%.1fs) does not outlast one delivery attempt " + "(%.1fs, the connect and read timeouts combined). Another worker can " + "reclaim a delivery that is still in flight and send it twice.", + settings.worker_lease_seconds, + deadline, + ) + session_factory: sessionmaker[Session] = create_session_factory(engine) client = create_webhook_client(settings) @@ -168,7 +219,11 @@ def request_stop(signum: int, frame: FrameType | None) -> None: signal.signal(signal.SIGINT, request_stop) signal.signal(signal.SIGTERM, request_stop) - asyncio.run(run_worker(settings, stop=stop)) + try: + asyncio.run(run_worker(settings, stop=stop)) + except SchemaNotReady as exc: + logger.error("the database schema is not ready: %s", exc) + sys.exit(1) if __name__ == "__main__": diff --git a/tests/test_idempotency.py b/tests/test_idempotency.py index 5532807..bf2e7b0 100644 --- a/tests/test_idempotency.py +++ b/tests/test_idempotency.py @@ -175,6 +175,44 @@ def test_a_key_is_scoped_to_one_endpoint(empty_client: TestClient) -> None: assert {row.endpoint_id for row in stored(empty_client)} == {"contact-form", "waitlist"} +# --- what a replay reports about delivery ------------------------------------ + + +def test_a_replay_reports_no_delivery_when_the_webhook_was_added_since( + make_client: ClientFactory, +) -> None: + # The replay describes the submission the key was spent on. That one owes no + # delivery, so a webhook configured afterwards must not make it claim one. + client = make_client(allow_private_webhook_targets=True) + first = submit(client) + changed = client.patch( + "/endpoints/contact-form", json={"webhook_url": "https://hooks.example.com/forms"} + ) + assert changed.status_code == 200, changed.text + + replay = submit(client) + + assert first.json()["delivery"]["queued"] is False + assert replay.json()["idempotent_replay"] is True + assert replay.json()["delivery"]["queued"] is False + + +def test_a_replay_still_reports_its_delivery_after_the_webhook_is_removed( + make_client: ClientFactory, +) -> None: + client = make_client(seed_endpoint=False) + create_endpoint(client, webhook_url="https://hooks.example.com/forms") + first = submit(client) + changed = client.patch("/endpoints/contact-form", json={"webhook_url": None}) + assert changed.status_code == 200, changed.text + + replay = submit(client) + + assert first.json()["delivery"]["queued"] is True + assert replay.json()["idempotent_replay"] is True + assert replay.json()["delivery"]["queued"] is True + + # --- what counts as the same payload ----------------------------------------- diff --git a/tests/test_worker.py b/tests/test_worker.py index 5976d4c..7e8717c 100644 --- a/tests/test_worker.py +++ b/tests/test_worker.py @@ -8,20 +8,36 @@ from __future__ import annotations +import asyncio import hashlib import hmac import json +import logging +import threading +import time from datetime import UTC, datetime, timedelta +from pathlib import Path from typing import Any import pytest from fastapi.testclient import TestClient from sqlalchemy import select from sqlalchemy.dialects import postgresql - -from conftest import ClientFactory, app_settings, create_endpoint, open_session, work_once -from hymical_forms import models, storage -from hymical_forms.webhooks import DeliveryState, RetryPolicy +from sqlalchemy.exc import OperationalError + +from conftest import ( + ClientFactory, + app_settings, + build_settings, + create_endpoint, + open_session, + work_once, +) +from hymical_forms import models, storage, worker +from hymical_forms.db import create_engine_from_url +from hymical_forms.delivery import create_webhook_client, deliver +from hymical_forms.schema import SchemaNotReady, create_all +from hymical_forms.webhooks import DeliveryResult, DeliveryState, RetryPolicy from webhook_server import WebhookRecorder, unused_local_url ENDPOINT = "/f/contact-form" @@ -627,3 +643,169 @@ def test_the_configured_policy_reaches_the_worker(make_client: ClientFactory) -> assert policy.max_attempts == 7 assert policy.initial_seconds == 3 + + +# --- bounding one attempt ---------------------------------------------------- + + +def test_a_trickling_response_cannot_outlast_the_attempt_deadline( + make_client: ClientFactory, webhook: WebhookRecorder +) -> None: + # Each byte arrives well inside the read timeout, so only an overall deadline + # can stop this. Without one the attempt runs for as long as the destination + # cares to keep writing, which outlives any lease and stalls the whole batch. + webhook.trickle_seconds = 0.05 + client, _ = queued_client( + make_client, + webhook.url, + webhook_connect_timeout_seconds=0.25, + webhook_read_timeout_seconds=0.25, + ) + client.post(ENDPOINT, data={"email": "dev@example.com"}) + + started = time.monotonic() + work_once(client, now=due_at(client)) + elapsed = time.monotonic() - started + + assert elapsed < 3 + attempt = attempts_of(client)[0] + assert attempt.outcome == "timeout" + assert delivery_of(client).state == DeliveryState.PENDING + + +def test_a_trickling_response_body_does_not_hold_up_a_success( + make_client: ClientFactory, webhook: WebhookRecorder +) -> None: + # The status is the whole answer. Reading a body nobody looks at would let a + # destination decide how long, and how much memory, an attempt takes. + webhook.trickle_seconds = 0.05 + webhook.trickle_body = True + client, _ = queued_client( + make_client, + webhook.url, + webhook_connect_timeout_seconds=0.25, + webhook_read_timeout_seconds=0.25, + ) + client.post(ENDPOINT, data={"email": "dev@example.com"}) + + started = time.monotonic() + work_once(client, now=due_at(client)) + elapsed = time.monotonic() - started + + assert elapsed < 3 + assert attempts_of(client)[0].outcome == "succeeded" + assert delivery_of(client).state == DeliveryState.DELIVERED + + +# --- the batch and its transaction ------------------------------------------- + + +def test_no_transaction_is_open_while_requests_are_in_flight( + make_client: ClientFactory, webhook: WebhookRecorder, monkeypatch: pytest.MonkeyPatch +) -> None: + client, _ = queued_client(make_client, webhook.url) + client.post(ENDPOINT, data={"email": "dev@example.com"}) + settings = app_settings(client) + observed: list[bool] = [] + + async def run() -> None: + webhook_client = create_webhook_client(settings) + try: + with open_session(client) as session: + + async def watching(*args: Any, **kwargs: Any) -> DeliveryResult: + observed.append(session.in_transaction()) + return await deliver(*args, **kwargs) + + monkeypatch.setattr(worker, "deliver", watching) + await worker.process_batch(session, webhook_client, settings, now=due_at(client)) + finally: + await webhook_client.aclose() + + asyncio.run(run()) + + assert observed == [False] + assert delivery_of(client).state == DeliveryState.DELIVERED + + +def test_one_failed_recording_does_not_lose_the_rest_of_the_batch( + make_client: ClientFactory, webhook: WebhookRecorder, monkeypatch: pytest.MonkeyPatch +) -> None: + # Every request in the batch has already been sent by the time recording + # starts. Abandoning the rest on the first database error would leave them + # claimed with nothing recorded, to be sent a second time once the lease runs + # out, for no reason other than a neighbour's failure. + client, _ = queued_client(make_client, webhook.url) + client.post(ENDPOINT, data={"email": "first@example.com"}) + client.post(ENDPOINT, data={"email": "second@example.com"}) + real = storage.complete_attempt + calls = 0 + + def flaky(*args: Any, **kwargs: Any) -> storage.CompletedAttempt: + nonlocal calls + calls += 1 + if calls == 1: + raise OperationalError("UPDATE", {}, Exception("connection reset")) + return real(*args, **kwargs) + + monkeypatch.setattr(storage, "complete_attempt", flaky) + with open_session(client) as session: + latest = max(row.next_attempt_at for row in session.scalars(select(models.WebhookDelivery))) + + handled = work_once(client, now=latest) + + assert handled == 2 + assert len(webhook.received) == 2 + with open_session(client) as session: + states = sorted(row.state for row in session.scalars(select(models.WebhookDelivery))) + assert states == [DeliveryState.DELIVERED, DeliveryState.PROCESSING] + + +# --- starting up ------------------------------------------------------------- + + +def test_the_worker_refuses_to_start_against_an_unmigrated_database(tmp_path: Path) -> None: + settings = build_settings(database_url=f"sqlite:///{tmp_path / 'empty.db'}") + stop = threading.Event() + stop.set() + + with pytest.raises(SchemaNotReady): + asyncio.run(worker.run_worker(settings, stop=stop)) + + +def test_the_worker_warns_when_the_lease_cannot_outlast_an_attempt( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + url = f"sqlite:///{tmp_path / 'forms.db'}" + engine = create_engine_from_url(url) + create_all(engine) + engine.dispose() + settings = build_settings( + database_url=url, + worker_lease_seconds=10, + webhook_connect_timeout_seconds=5, + webhook_read_timeout_seconds=10, + ) + stop = threading.Event() + stop.set() + + with caplog.at_level(logging.WARNING, logger="hymical_forms.worker"): + asyncio.run(worker.run_worker(settings, stop=stop)) + + assert "FORMS_WORKER_LEASE_SECONDS" in caplog.text + + +def test_the_default_lease_draws_no_warning( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + url = f"sqlite:///{tmp_path / 'forms.db'}" + engine = create_engine_from_url(url) + create_all(engine) + engine.dispose() + stop = threading.Event() + stop.set() + + with caplog.at_level(logging.WARNING, logger="hymical_forms.worker"): + asyncio.run(worker.run_worker(build_settings(database_url=url), stop=stop)) + + assert "FORMS_WORKER_LEASE_SECONDS" not in caplog.text diff --git a/tests/webhook_server.py b/tests/webhook_server.py index 035fec4..06bdae1 100644 --- a/tests/webhook_server.py +++ b/tests/webhook_server.py @@ -35,6 +35,12 @@ class WebhookRecorder: status: int = 200 delay_seconds: float = 0.0 + # When set, the response is written one byte at a time with this gap between + # bytes, which no single read ever waits long enough on to time out. + # ``trickle_body`` chooses whether the status line and headers arrive + # promptly and only the body trickles, or whether the whole response does. + trickle_seconds: float = 0.0 + trickle_body: bool = False received: list[ReceivedWebhook] = field(default_factory=list) _server: ThreadingHTTPServer | None = None _thread: threading.Thread | None = None @@ -74,10 +80,35 @@ def do_POST(self) -> None: # timeout is meant to catch. if recorder.delay_seconds: time.sleep(recorder.delay_seconds) + if recorder.trickle_seconds: + self._trickle() + return self.send_response(recorder.status) self.send_header("Content-Length", "0") self.end_headers() + def _trickle(self) -> None: + """ + write a response slowly enough to defeat any per-read timeout + """ + body = b"x" * 400 + head = ( + f"HTTP/1.1 {recorder.status} OK\r\nContent-Length: {len(body)}\r\n\r\n" + ).encode("ascii") + if recorder.trickle_body: + self.wfile.write(head) + self.wfile.flush() + head = b"" + try: + for byte in head + body: + self.wfile.write(bytes([byte])) + self.wfile.flush() + time.sleep(recorder.trickle_seconds) + except OSError: + # The client gave up and closed the connection, which is the + # behaviour a trickling response is used to provoke. + pass + def log_message(self, format: str, *args: object) -> None: """ silence the per-request logging that would otherwise flood the test output