Halt and keel serve hardening: drain every POST, HALT on a failed Pulse, serve keeps --external-host (#955, #958, #971)

keel/commands/pulse.py

@@ -263,6 +263,19 @@ def build_pulse(
     )
 
 
+def halt_resume_command(repo: Repository) -> str:
+    """`RESUME_COMMAND` when the kill switch reads engaged, else `""` -- the halt control's one
+    input besides its token, read ALONE (#958).
+
+    The Pulse report reads the same fact through `gather_status`, and when any part of that
+    report fails the whole report fails with it. The HALT button must not: stopping is never
+    slower than starting (`keel/web/gates.py`). So the web layer reads this on the failure path,
+    where nothing else on the ledger is trusted to answer. The same reading as
+    `gather_status`'s, `default=True` included -- an unset switch is ENGAGED, as the guards treat
+    it."""
+    return RESUME_COMMAND if bool(repo.get_state("kill_switch", default=True)) else ""
+
+
 def gather_pulse(
     repo: Repository, config: Config, feed: ActivityFeed, *, now_ts: int
 ) -> PulseReport:

keel/commands/setup.py

@@ -46,6 +46,7 @@
 import click
 
 from keel.commands._common import default_config_path, default_db_path
+from keel.data.db import connect_read_only
 
 #: The exact schema names this module reads. Named constants rather than string literals for a
 #: reason that is not style: I wrote `"subscriptions"` and `"state"` here first, and both are
@@ -446,7 +447,10 @@ def inspect(config_path: str | Path, db_path: str | Path) -> DeploymentState:
     conn: sqlite3.Connection | None = None
     if db_file.exists():
         try:
-            conn = sqlite3.connect(f"file:{db_file}?mode=ro", uri=True)
+            # `connect_read_only`, not `mode=ro` (#976): a WAL file whose sidecars are gone
+            # cannot be opened `mode=ro`, and this probe then reported a running deployment as
+            # having no usable database -- the console's stopped view, which has no HALT.
+            conn = connect_read_only(db_file)
             # `Repository` reads rows by COLUMN NAME (`row["value"]`, `row["state"]`, ...), which
             # `_scope_observation`/`_venue_interest_observation` now call straight into for the
             # same reason `_withdrawals_observation` stayed on raw SQL: one place to decode a
@@ -1297,7 +1301,7 @@ def _sleeve_sell_kind(db_path: Path, rule_id: int) -> str | None:
     from keel.data.repository import Repository
 
     try:
-        conn = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True)
+        conn = connect_read_only(db_path)  # not `mode=ro`: see `inspect` (#976)
     except sqlite3.Error as exc:
         raise _UnreadableRule from exc
     conn.row_factory = sqlite3.Row

keel/data/db.py

@@ -16,8 +16,9 @@
 import json
 import sqlite3
 import time
+import urllib.parse
 from collections.abc import Callable
-from pathlib import Path
+from pathlib import Path, PurePath
 from typing import Any
 
 SCHEMA_VERSION = 22
@@ -1213,6 +1214,61 @@ def connect(path: str | Path = "keel.db") -> sqlite3.Connection:
     return conn
 
 
+def _read_only_uri(path: PurePath) -> str:
+    """The `file:` URI `connect_read_only` opens, built from an ABSOLUTE path.
+
+    **Not `Path.as_uri()`.** On Windows it yields `file:///C:/...` for a drive path but, for a UNC
+    path (`\\\\server\\share`), a form SQLite does not read as the same file; and anything built
+    by hand must percent-encode, because in a URI `#`, `?` and `%` stop being path characters. So:
+    `as_posix()`, a `file://` prefix when the path already starts with `/` (POSIX, and UNC after
+    `as_posix` -- which gives `//server/share`, hence the four slashes) and `file:///` when it
+    starts with a drive letter, and `quote` keeping only `/` and `:`.
+    """
+    posix = path.as_posix()
+    prefix = "file://" if posix.startswith("/") else "file:///"
+    return prefix + urllib.parse.quote(posix, safe="/:") + "?mode=rw"
+
+
+def connect_read_only(path: str | Path) -> sqlite3.Connection:
+    """Open an EXISTING database to read it, never to write it (#976).
+
+    **Why not `mode=ro`.** A WAL database's `-wal`/`-shm` sidecars can be gone while the file is
+    perfectly healthy -- a checkpointing last close removes them, and every copied backup lacks
+    them. Opening such a file needs the `-shm` re-created, which `mode=ro` forbids, so the open
+    fails with "unable to open database file". The console read that as "keel isn't running
+    here" on a running deployment, and its stopped view has no HALT.
+
+    So the file is opened `mode=rw` -- which may create the sidecars, and which, unlike a plain
+    `sqlite3.connect`, refuses to CREATE a missing database -- and `PRAGMA query_only = ON` is
+    set before anything is read, so SQLite refuses every ordinary write on this connection: rows,
+    schema, and header pragmas alike.
+
+    **What that does and does not rest on.** `query_only` is per-connection and the connection can
+    switch it back off, and neither it nor `mode=rw` blocks `ATTACH` or the checkpoint pragmas.
+    Read-only here is SQLite refusing ordinary writes PLUS no console reader issuing
+    `PRAGMA query_only`, `ATTACH`, a checkpoint or `VACUUM`; a structural test walks the console's
+    read path for exactly those (`test_the_console_read_path_never_asks_sqlite_to_lift_read_only`).
+
+    **Checkpoint-on-close is disabled** (`SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE`). The last connection
+    to close a WAL database otherwise checkpoints it into the main file, and a console that closes
+    after the agent has gone would be writing the trading database. The `-wal` is left for the
+    writer, which checkpoints on its own schedule.
+
+    Unlike `connect`, this sets no `journal_mode`: changing the file's journal is a write, and a
+    reader has no business deciding it.
+    """
+    conn = sqlite3.connect(_read_only_uri(Path(path).absolute()), uri=True)
+    try:
+        conn.setconfig(sqlite3.SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE, True)
+        conn.execute("PRAGMA query_only = ON")
+        conn.execute(f"PRAGMA busy_timeout = {BUSY_TIMEOUT_MS}")
+    except BaseException:
+        conn.close()
+        raise
+    conn.row_factory = sqlite3.Row
+    return conn
+
+
 def table_present(conn: sqlite3.Connection, name: str) -> bool:
     """Does `name` exist on this database?
 

keel/venue_readiness.py

@@ -65,7 +65,6 @@
 
 from __future__ import annotations
 
-import sqlite3
 from collections.abc import Mapping
 from dataclasses import dataclass
 from enum import Enum
@@ -74,6 +73,8 @@
 
 from keel_core.trade_scope import READ_ONLY, TradeScopeState, VenueTradeScope
 
+from keel.data.db import connect_read_only
+
 
 class VenueReadiness(str, Enum):
     """The five states, in the order `venue_readiness` checks them."""
@@ -322,9 +323,11 @@ def _read_only_trade_scope(db_path: str | None, venue: str) -> tuple[VenueTradeS
     an empty database that later commands would then find and believe in. `keel brokers list` is
     a read-only informational command and must litter nothing.
 
-    So this uses the `mode=ro` URI shape #610 promoted to a seam for exactly this hazard
-    (`keel/commands/_common.py::_open_repo_ro`), with the same explicit existence check in front
-    of it, because `mode=ro`'s own refusal is an `OperationalError` and this path wants an
+    So this uses `keel.data.db.connect_read_only` (#976): `mode=rw`, which creates no database
+    file, plus `PRAGMA query_only = ON`, which refuses every write. It replaced the `mode=ro`
+    URI shape (#610's seam, `keel/commands/_common.py::_open_repo_ro`), which cannot open a WAL
+    database whose `-shm` sidecar is absent and so reported a healthy `/api/venues` database as
+    unreadable. The explicit existence check stays in front of it, because this path wants an
     ANSWER, not an exception.
 
     Where it deliberately differs from that seam: `_open_repo_ro` REFUSES a database stamped
@@ -343,15 +346,14 @@ def _read_only_trade_scope(db_path: str | None, venue: str) -> tuple[VenueTradeS
     try:
         from keel.data.repository import Repository
 
-        conn = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True)
-        conn.row_factory = sqlite3.Row
+        conn = connect_read_only(db_path)
         return Repository(conn).get_venue_trade_scope(venue), False
     except Exception:
         # A file IS there and this process could not read it: a schema older than #233's
-        # migration, a permissions problem, or -- the case that motivated separating these two
-        # answers -- a WAL database whose `-shm` sidecar is absent, which `mode=ro` cannot open
-        # because SQLite would have to CREATE that file. Every copied or restored backup has
-        # that shape. Reported as unknown, never as "nothing is attested".
+        # migration, a permissions problem, a locked or damaged file. (A WAL database whose
+        # `-shm` sidecar is absent was the case that motivated separating these two answers;
+        # since #976 it is read, not reported.) Reported as unknown, never as "nothing is
+        # attested".
         return None, True
     finally:
         if conn is not None:

keel/web/api.py

@@ -65,14 +65,28 @@
 
 
 def open_repo(db_path: str) -> Any:
-    """A plain connection -- deliberately WITHOUT `migrate`.
+    """A READ-ONLY connection (`db.connect_read_only`) -- deliberately WITHOUT `migrate`.
 
     Every CLI command migrates on the way in, which is right for a command: it runs once, and a
     schema behind the code is a thing to fix rather than to fail on. It is wrong here. These
     pages auto-reload every 15 seconds, so migrating per request would have a view that calls
     itself read-only take a write lock on the deployment database four times a minute -- against
     a database the agent may be mid-cycle on. `server.ensure_schema` does it ONCE, at bind time,
-    before anything is served."""
+    before anything is served.
+
+    `query_only` since #976: every reader in this package goes through here, so SQLite itself
+    refuses a write through any of them. The halt is the one write, and it opens its own
+    connection (`open_repo_to_write`)."""
+    from keel.data.db import connect_read_only
+    from keel.data.repository import Repository
+
+    return Repository(connect_read_only(db_path))
+
+
+def open_repo_to_write(db_path: str) -> Any:
+    """The read-write connection for `gates._perform` -- the halt and the gated releases -- and
+    nothing else. Not `query_only`, so not for a reader: a page that read through this could write
+    the trading database. Like `open_repo`, it does not migrate."""
     from keel.data.db import connect
     from keel.data.repository import Repository
 
@@ -330,6 +344,30 @@ def read_pulse(cfg: ServeConfig, _query: Query, _state: Any, now_ts: int) -> dic
     return payload.pulse_payload(report, halt_token=halt_token(cfg.token))
 
 
+def pulse_halt(cfg: ServeConfig) -> dict[str, str]:
+    """The HALT control's inputs when the Pulse report could not be built (#958).
+
+    `halt_token` is minted from the session, so it cannot fail. The kill-switch reading is the one
+    ledger read here, through its own repo open and `pulse.halt_resume_command` -- never through
+    `gather_pulse`, whose failure is why this exists. **A switch that cannot be read reads as not
+    halted**, which shows the button: halting a halted engine is harmless and idempotent, while a
+    HALTED line over a running engine would hide the only stop the page has. Never `gates_token`
+    (#968), on this path as on the success one."""
+    from keel.commands.pulse import halt_resume_command
+
+    resume = ""
+    repo = None
+    try:
+        repo = open_repo(cfg.db_path)
+        resume = halt_resume_command(repo)
+    except Exception:  # noqa: BLE001 - an unreadable switch must still answer with the button
+        resume = ""
+    finally:
+        if repo is not None:
+            close_repo(repo)
+    return payload.halt_fallback(halt_token=halt_token(cfg.token), resume_command=resume)
+
+
 def read_orders(cfg: ServeConfig, query: Query, _state: Any, now_ts: int) -> dict[str, Any]:
     """The audit trail: what keel actually bought and sold, and at what price (#659).
 
@@ -882,6 +920,10 @@ class ApiRoute:
     needs_database: bool = True
     collection: str = ""
     sortable: tuple[str, ...] = field(default=())
+    #: Whether a failed read still answers with the HALT control's inputs (#958). `/api/pulse`
+    #: only: it is the view that halts, and `pulse_halt` reads what the button needs apart from
+    #: the report, so a report that raises does not take the stop with it.
+    halt_on_failure: bool = False
 
 
 #: THE READ SURFACE, in full. Every one of the rendered routes in `server.ROUTES` appears here as
@@ -895,6 +937,7 @@ class ApiRoute:
     "/api/pulse": ApiRoute(
         html_route="/pulse",
         read=read_pulse,
+        halt_on_failure=True,
         # No sortable collection: every table on the glance page is an intrinsic order --
         # `rule_counts` by status, `data_freshness` in the config's product order, `live_rules`
         # in the ledger's, cycles newest first -- and a display sort would destroy each rather
@@ -1197,18 +1240,27 @@ def respond(cfg: ServeConfig, path: str, query: Query) -> tuple[int, dict[str, A
         data = route.read(cfg, query, state, now_ts)
     except ApiRefusal as refusal:
         return refusal.status, payload.error_envelope(
-            now_ts, status=refusal.status, title=refusal.title, detail=refusal.detail
+            now_ts,
+            status=refusal.status,
+            title=refusal.title,
+            detail=refusal.detail,
+            halt=pulse_halt(cfg) if route.halt_on_failure else None,
         )
     except Exception as exc:
         # A broken report is a stated failure, never a 200 with an empty payload -- that shape is
         # precisely the "blank view, or worse, stale figures" this envelope exists to prevent. The
         # detail carries the exception's TYPE and message and no traceback: a traceback in a
         # browser is a stack of file paths from someone else's machine.
+        #
+        # Pulse's failure still carries the HALT control's inputs (#958): the report failing is
+        # not a reason for the stop to disappear. Reached only after `_admitted`, like the
+        # success document that carries the same token.
         return 500, payload.error_envelope(
             now_ts,
             status=500,
             title="That report could not be built",
             detail=f"{type(exc).__name__}: {exc}",
+            halt=pulse_halt(cfg) if route.halt_on_failure else None,
         )
 
     if column:

keel/web/gates.py

@@ -235,9 +235,9 @@ def _perform(cfg: Any, operation: Callable[[Repository], None], done: str) -> st
     A `finally`, because every reader in `keel/web/` closes in one and a writer that leaked a
     connection would hold a write lock on the deployment database until refcounting noticed.
     """
-    from keel.web.api import close_repo, open_repo
+    from keel.web.api import close_repo, open_repo_to_write
 
-    repo = open_repo(cfg.db_path)
+    repo = open_repo_to_write(cfg.db_path)
     try:
         operation(repo)
     finally:

keel/web/payload.py

@@ -2490,7 +2490,12 @@ def envelope(
 
 
 def error_envelope(
-    now_ts: float | int | None, *, status: int, title: str, detail: str
+    now_ts: float | int | None,
+    *,
+    status: int,
+    title: str,
+    detail: str,
+    halt: dict[str, str] | None = None,
 ) -> dict[str, Any]:
     """One `GET /api/*` refusal or failure.
 
@@ -2510,15 +2515,30 @@ def error_envelope(
     is always `null`: most refusals happen before the session cookie is checked, so filling it in
     would mean an unauthenticated request reading the deployment database -- the same argument
     that keeps `engine` out of this document.
+
+    `halt` (#958) is `null` on every failure but one: Pulse's own, after admission, where it is
+    `halt_fallback`'s `{halt_token, resume_command}`. The HALT button lives on Pulse, and a stop
+    that vanished whenever the report beside it failed to build would make stopping slower than
+    starting. Present and `null` everywhere else for `attestations`' reason -- a constant key set.
     """
     return {
         "as_of": iso(now_ts),
         "data": None,
         "attestations": None,
         "error": {"status": str(status), "title": title, "detail": detail},
+        "halt": halt,
     }
 
 
+def halt_fallback(*, halt_token: str, resume_command: str) -> dict[str, str]:
+    """What the HALT control needs when the Pulse report could not be built (#958): the halt
+    token and the resume command, the same two values `pulse_payload` carries as `halt_token`
+    and `halt.resume_command`, and nothing else. Bare strings, under the same names, so the
+    client hands them to the same two readers (`actions.rememberHaltToken`, `render.haltControl`)
+    rather than a second copy of either."""
+    return {"halt_token": str(halt_token), "resume_command": str(resume_command)}
+
+
 # -- ordering (#534) -----------------------------------------------------------------------------
 #
 # Server-side sort, ordered with `Decimal`. The spec's own words: "On loopback the round trip is