From 4c84bc01dfafc57051e4819b6fe4e9302420a67d Mon Sep 17 00:00:00 2001 From: Quang <20378quang@gmail.com> Date: Sat, 17 Oct 2026 11:32:31 -0400 Subject: [PATCH 1/5] fix(worker): bound delivery attempts and harden the batch loop - Verify the schema revision on worker startup, as the API and CLI already do and as docs/operations/worker.md claimed. The worker now exits 1 instead of polling against a database it was not built for. - Hold every outbound attempt to an overall deadline of the connect and read timeouts combined. The client timeouts each bound a single wait, so a destination trickling bytes could keep a request open indefinitely, outliving the lease and stalling the batch and worker shutdown. - Stream the response and never read its body, so a destination cannot dictate how much memory an attempt uses. - End the read transaction opened by load_submissions before sending, so no transaction is held across outbound HTTP calls. - Record the rest of a batch when one completion fails to record, instead of abandoning every remaining result and forcing duplicate sends. - Warn at startup when FORMS_WORKER_LEASE_SECONDS does not outlast one attempt. --- docs/architecture/delivery-semantics.md | 5 +- docs/guides/webhooks.md | 11 +- docs/operations/configuration-reference.md | 2 +- docs/operations/worker.md | 30 +++- src/hymical_forms/delivery.py | 47 ++++- src/hymical_forms/worker.py | 67 +++++++- tests/test_worker.py | 190 ++++++++++++++++++++- tests/webhook_server.py | 31 ++++ 8 files changed, 355 insertions(+), 28 deletions(-) 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/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/worker.md b/docs/operations/worker.md index 35c6093..b70f7b8 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,8 +77,22 @@ 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 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/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_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 From c85801c6c8cab8a679f50e02305820f9c642e35c Mon Sep 17 00:00:00 2001 From: Quang <20378quang@gmail.com> Date: Sat, 17 Oct 2026 11:32:31 -0400 Subject: [PATCH 2/5] fix(ingestion): report a replay's delivery from the stored submission An idempotent replay reported delivery.queued from the endpoint's current webhook configuration. If the webhook was added or removed after the original request, the replay claimed a delivery that did not exist, or denied one that did. It now reports whether the stored submission owes a delivery. --- docs/guides/idempotency.md | 4 ++- src/hymical_forms/api/submissions.py | 2 +- src/hymical_forms/storage.py | 30 +++++++++++++++++++--- tests/test_idempotency.py | 38 ++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+), 6 deletions(-) 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/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/storage.py b/src/hymical_forms/storage.py index 363b093..4b47a7f 100644 --- a/src/hymical_forms/storage.py +++ b/src/hymical_forms/storage.py @@ -242,8 +242,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 +296,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 +329,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) + return StoredSubmission(submission, replayed=False, delivery_queued=webhook is not None) + + +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/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 ----------------------------------------- From b63f3d2e5ac804c449ccdedcd5224a2fbc737ed9 Mon Sep 17 00:00:00 2001 From: Quang <20378quang@gmail.com> Date: Sat, 17 Oct 2026 11:51:19 -0400 Subject: [PATCH 3/5] fix(types): type check the submission filter helper on SQLAlchemy 2.1 CI now installs SQLAlchemy 2.1, where Select is generic over a variadic row type, so the existing bound Select[Any] no longer accepts a two-entity select and mypy fails in list_submissions. Bound the type variable by a small protocol for "has where()" instead, which type checks on both 2.0 and 2.1 (pyproject allows sqlalchemy>=2.0). No runtime change. --- src/hymical_forms/storage.py | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/src/hymical_forms/storage.py b/src/hymical_forms/storage.py index 4b47a7f..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 From 167c6f45026d5a8394d52485aa45a1f4c54a7042 Mon Sep 17 00:00:00 2001 From: Quang <20378quang@gmail.com> Date: Sat, 17 Oct 2026 11:38:39 -0400 Subject: [PATCH 4/5] chore(release): release checklist, container image and deployment docs - docs/development/releasing.md: the release checklist (version bump, release notes, PG suite, mkdocs build, annotated tag on the merge commit) and a record of the v0.2.1 discrepancy: the tag points at 235f8af, where __version__ is still 0.2.0, and has no release notes. The tag is not moved; the next release resolves it. - Dockerfile: one non-root image for the API (default), the worker, migrations and the operator CLI. uvicorn runs with --no-proxy-headers so FORMS_TRUSTED_PROXY_HOPS stays the only trust knob for XFF. - compose.yaml: example stack of postgres, one-shot migrate, api and worker, with a worker stop grace above the attempt deadline. - docs/operations/deployment.md: the image, the example, the explicit stop-migrate-start sequence, and what health checking does and does not cover. - api/health.py: replace the stale "readiness arrives with persistence" comment. No readiness route is added; that is a public-boundary decision left open and documented in Limitations. --- .dockerignore | 23 +++++ Dockerfile | 50 +++++++++ compose.yaml | 90 ++++++++++++++++ docs/api/submissions.md | 4 +- docs/development/contributing.md | 1 + docs/development/releasing.md | 171 +++++++++++++++++++++++++++++++ docs/operations/deployment.md | 153 +++++++++++++++++++++++++++ docs/operations/migrations.md | 1 + docs/operations/worker.md | 4 +- docs/reference/limitations.md | 9 ++ mkdocs.yml | 2 + src/hymical_forms/api/health.py | 9 +- 12 files changed, 512 insertions(+), 5 deletions(-) create mode 100644 .dockerignore create mode 100644 Dockerfile create mode 100644 compose.yaml create mode 100644 docs/development/releasing.md create mode 100644 docs/operations/deployment.md 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/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/operations/deployment.md b/docs/operations/deployment.md new file mode 100644 index 0000000..c1106b9 --- /dev/null +++ b/docs/operations/deployment.md @@ -0,0 +1,153 @@ +# 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. + +Whether to add a readiness check, and if so on which side of the public and +management boundary it sits, is an open decision rather than 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 one. 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 b70f7b8..73e933a 100644 --- a/docs/operations/worker.md +++ b/docs/operations/worker.md @@ -98,7 +98,9 @@ destination may receive it twice. 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..c3388ff 100644 --- a/docs/reference/limitations.md +++ b/docs/reference/limitations.md @@ -131,6 +131,15 @@ 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`. + 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..18dcb02 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 no + # readiness route: one would be a new public route, which is a boundary + # decision rather than something to add here. return HealthResponse(status="ok", service="hymical-forms", version=__version__) From 092dee94f5ce17238c5311d2f8ad7e3a119b5f90 Mon Sep 17 00:00:00 2001 From: Quang <20378quang@gmail.com> Date: Sat, 17 Oct 2026 11:44:28 -0400 Subject: [PATCH 5/5] docs(health): record that v1.0 ships without a readiness route /health stays liveness only and no database-checking route is added, so the public surface remains /health and POST /f/{endpoint_id}. The deployment page, Limitations and the health handler comment now state this as a decision rather than an open question. --- docs/operations/deployment.md | 11 ++++++----- docs/reference/limitations.md | 1 + src/hymical_forms/api/health.py | 6 +++--- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/docs/operations/deployment.md b/docs/operations/deployment.md index c1106b9..0156ee3 100644 --- a/docs/operations/deployment.md +++ b/docs/operations/deployment.md @@ -125,11 +125,12 @@ There is **no readiness endpoint**. What exists instead: - **The worker has no HTTP surface.** Its health is visible only in its logs and in whether due deliveries are being drained. -Whether to add a readiness check, and if so on which side of the public and -management boundary it sits, is an open decision rather than 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 one. See -[Limitations](../reference/limitations.md#operations). +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 diff --git a/docs/reference/limitations.md b/docs/reference/limitations.md index c3388ff..d1ef5ef 100644 --- a/docs/reference/limitations.md +++ b/docs/reference/limitations.md @@ -135,6 +135,7 @@ deploy it, and it is kept complete rather than flattering. 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 diff --git a/src/hymical_forms/api/health.py b/src/hymical_forms/api/health.py index 18dcb02..0198d13 100644 --- a/src/hymical_forms/api/health.py +++ b/src/hymical_forms/api/health.py @@ -33,7 +33,7 @@ async def health() -> HealthResponse: # 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 no - # readiness route: one would be a new public route, which is a boundary - # decision rather than something to add here. + # 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__)