diff --git a/DOCS-reload.md b/DOCS-reload.md new file mode 100644 index 0000000..811f1a7 --- /dev/null +++ b/DOCS-reload.md @@ -0,0 +1,59 @@ +# Picking up a change without a restart + +Two kinds of change reach the running bot, and they are handled differently. + +## Data: `POST /_reload` + +Aliases, an album's `people.json`/`tags.json`, and every setting a page in the +management UI writes are *data*. The command table is built from that data by +one function (`signal_bot/wiring.py`), and `RegistryHolder` can run it again: +the new table is built whole, then swapped in with a single assignment. Nothing +restarts, the websocket stays up, and no frame is lost or replayed. + +```sh +scripts/reload.sh # rebuild the table, report how many commands are live +scripts/reload.sh '!nowaosoba' # ...and then type that at the bot in testbot +``` + +The answer is what the bot is serving **now**: + +| status | code | meaning | +|------------|------|--------------------------------------------------------------------| +| `reloaded` | 200 | the new table is live; `commands` is its size | +| `kept` | 409 | the data no longer builds; the previous table is still answering, and `reason` says what was wrong | +| `connecting` | 503 | the bot has not reached Signal yet; there is no table to rebuild | + +A rejected reload is deliberate: the builder reads files people edit by hand +and pages write mid-session, and the only safe answer to a file that does not +load is the table that was already answering. Only bad *data* is survived +(`ConfigError`, `DuplicateTriggerError`); a programming error still surfaces +as a 500 and a traceback in the journal, because a bug is not a file. + +Startup is the one time a bad table is fatal: there is nothing to fall back +on, and `Restart=always` looping on a `ConfigError` is a clearer signal than a +bot that answers nothing. + +## Code: `scripts/reload.sh --code` + +A Python change needs the process replaced. The restart is about a second and +lossless (signal-cli replays what arrived meanwhile), but it is a deploy step, +so it is the flag rather than the default: + +```sh +scripts/reload.sh --code '!pomoc' +``` + +restarts the unit, waits for `/healthz` to report `ok` again, prints the +version it came back as, and -- with a command given -- types it in the test +group so "restarted" means "answered". There is no hot-reload of Python on +purpose: `importlib.reload` leaves every object the old module already +created bound to the old classes, which is a bug that looks like a feature +until the first `isinstance` check. + +## Proven where + +`scripts/e2e.sh` brackets a `scripts/reload.sh` with the two scenarios in +`fake_signal/proofs/`: `!delta` is unknown, a fifth person is tagged into the +synthetic pack, the script runs, and the same process posts delta's photo. +The route itself is covered in `tests/test_app.py`, the holder in +`tests/test_registry_holder.py`, and the album case in `tests/test_bot.py`. diff --git a/README.md b/README.md index f45dcbb..1e2f9dd 100644 --- a/README.md +++ b/README.md @@ -227,9 +227,9 @@ things: `!staty` `!wczoraj` `!tydzień` `!miesiąc` `!rok` `!rekordy` `!korona` ## Managing it -`./web.sh` serves one page on `127.0.0.1:8140`: health, the configuration -(editable) and the album's tagger. Started by hand and stopped again, because -it can rewrite `.env`: [DOCS-web-ui.md](DOCS-web-ui.md). +`./web.sh` serves one page on `127.0.0.1:8140`: health, the configuration (editable) and the +album's tagger. Started by hand and stopped again, because it can rewrite `.env`: [DOCS-web-ui.md](DOCS-web-ui.md). +A tagged person goes live with `scripts/reload.sh`, no restart; code with `--code`: [DOCS-reload.md](DOCS-reload.md). ## Develop diff --git a/fake_signal/proofs/reload_after.yaml b/fake_signal/proofs/reload_after.yaml new file mode 100644 index 0000000..4bb858a --- /dev/null +++ b/fake_signal/proofs/reload_after.yaml @@ -0,0 +1,7 @@ +# The other half of the bracket: the same process, one POST /_reload later, +# and the person tagged into the album meanwhile answers with a photo. +name: "!delta posts a photo after the reload" +inject: + - { text: "!delta", uuid: uuid-reload-after, name: ReloadAfter } +expect: + - { kind: image, to: reply } diff --git a/fake_signal/proofs/reload_before.yaml b/fake_signal/proofs/reload_before.yaml new file mode 100644 index 0000000..263dc2e --- /dev/null +++ b/fake_signal/proofs/reload_before.yaml @@ -0,0 +1,8 @@ +# Not in scenarios/: that glob replays against the pack as first written, +# and this pair brackets the POST /_reload that scripts/e2e_proofs.sh makes +# in between. Before it, !delta is a word the bot has never heard. +name: "!delta is unknown before the reload" +inject: + - { text: "!delta", uuid: uuid-reload-before, name: ReloadBefore } +expect: + - { kind: text, to: reply, contains: "Nie znam !delta" } diff --git a/scripts/e2e.sh b/scripts/e2e.sh index c5d5897..2b02a15 100755 --- a/scripts/e2e.sh +++ b/scripts/e2e.sh @@ -95,6 +95,24 @@ wait_for_subscription() { done } +# Drive the fake through these scenario files, against the bot booted above. +# One place for the group map, so a proof that injects after the main replay +# resolves the same symbolic names the scenarios do. +replay() { + "$PYTHON" -m fake_signal.cli "$@" \ + --settle "${E2E_SETTLE:-0.15}" \ + --base-url "$FAKE_URL" \ + --group "watched=${WATCHED_GROUP_INTERNAL}" \ + --group "reply=${WATCHED_GROUP}" \ + --group "second=${SECOND_GROUP_INTERNAL}" \ + --group "second_reply=${SECOND_GROUP}" \ + --group "third=${THIRD_GROUP_INTERNAL}" \ + --group "third_reply=${THIRD_GROUP}" \ + --group "fourth=${FOURTH_GROUP_INTERNAL}" \ + --group "fourth_reply=${FOURTH_GROUP}" \ + --group "other=${OTHER_GROUP}" +} + validate_requirements() { if [[ ! -x "$PYTHON" ]]; then echo "Error: no interpreter at $PYTHON. Run: uv venv .venv && uv pip install -e '.[dev]'" >&2 @@ -159,23 +177,12 @@ main() { wait_for_subscription echo "== replaying scenarios" - "$PYTHON" -m fake_signal.cli \ - "$REPO_ROOT"/fake_signal/scenarios/*.yaml \ - --settle "${E2E_SETTLE:-0.15}" \ - --base-url "$FAKE_URL" \ - --group "watched=${WATCHED_GROUP_INTERNAL}" \ - --group "reply=${WATCHED_GROUP}" \ - --group "second=${SECOND_GROUP_INTERNAL}" \ - --group "second_reply=${SECOND_GROUP}" \ - --group "third=${THIRD_GROUP_INTERNAL}" \ - --group "third_reply=${THIRD_GROUP}" \ - --group "fourth=${FOURTH_GROUP_INTERNAL}" \ - --group "fourth_reply=${FOURTH_GROUP}" \ - --group "other=${OTHER_GROUP}" + replay "$REPO_ROOT"/fake_signal/scenarios/*.yaml # !dodaj is unwired until photos land in an approval queue instead of # the album (2026-09-04). Its scenarios, the media generation step and # prove_the_add_photo_invariants left with it; git has them at 64852ae. + prove_the_table_reloads_without_a_restart prove_the_deafness_gate enforce_the_time_budget "$STARTED_AT" } diff --git a/scripts/e2e_proofs.sh b/scripts/e2e_proofs.sh index 86e40c4..4f12661 100755 --- a/scripts/e2e_proofs.sh +++ b/scripts/e2e_proofs.sh @@ -43,6 +43,28 @@ prove_the_deafness_gate() { echo "== /healthz recovered on its own after ${waited}s" } +# A person tagged while the bot is up becomes a command after POST /_reload, +# with no restart: the same process that just answered "Nie znam !delta" +# posts delta's photo a moment later. The scenarios live next to the main +# set but outside its glob, because the second one is only true after the +# reload that this proof performs between them. +prove_the_table_reloads_without_a_restart() { + local proofs="${REPO_ROOT}/fake_signal/proofs" + echo "== before the reload, !delta is nobody" + replay "${proofs}/reload_before.yaml" + + echo "== tagging a fifth person into the pack, then scripts/reload.sh" + "$PYTHON" "$REPO_ROOT/tool/e2e_pack.py" \ + --dir "${TEMP_DIR}/pack" --group "$WATCHED_GROUP" --extra delta >/dev/null + # The operator's script, not a bare curl: this is the one place it runs + # against a real process in CI, so its exit code is the proof. + BIND_PORT="$BOT_PORT" "$REPO_ROOT/scripts/reload.sh" || + die "scripts/reload.sh did not get the new table live" + + echo "== after the reload, !delta posts a photo" + replay "${proofs}/reload_after.yaml" +} + # A budget, not a benchmark. This run was four minutes until the liveness # tick became configurable, and nothing announced the cost -- slower is the # one regression a green suite never reports. Exceeding it fails the run; diff --git a/scripts/reload.sh b/scripts/reload.sh new file mode 100755 index 0000000..8ebeed4 --- /dev/null +++ b/scripts/reload.sh @@ -0,0 +1,121 @@ +#!/bin/bash + +# ============================================================================ +# Make the running bot pick up a change, and prove that it did. +# +# Two kinds of change, two mechanisms: +# +# scripts/reload.sh [cmd] data changed (aliases, an album, a +# command switched off): POST /_reload, +# the table is rebuilt in place, nothing +# restarts and no frame is lost. +# scripts/reload.sh --code [cmd] code changed: restart the unit, wait for +# /healthz to go green again. Lossless too +# (signal-cli replays what arrived), but +# a deploy step -- so it is the flag, not +# the default. +# +# With [cmd], the bot then types that command at itself in the test group, +# so "reloaded" means "answered", not "returned 200". E.g. +# +# scripts/reload.sh '!nowaosoba' +# scripts/reload.sh --code '!pomoc' +# ============================================================================ + +set -euo pipefail + +SCRIPT_NAME="$(basename "$0")" +readonly SCRIPT_NAME +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +readonly SCRIPT_DIR +readonly BOT="http://127.0.0.1:${BIND_PORT:-8137}" +readonly UNIT="signal-bot" +readonly TEST_GROUP="${RELOAD_GROUP:-testbot}" + +RESTART=0 +PROBE="" + +usage() { + echo "Usage: $SCRIPT_NAME [--code] [command-to-selftest]" + echo " --code restart the unit (code changed) instead of rebuilding the table" + exit 0 +} + +validate_requirements() { + if ! command -v jq >/dev/null; then + echo "Error: jq is required" >&2 + exit 1 + fi +} + +wait_healthy() { + local attempt=0 + # 150 x 0.4 s: a restart is ~1 s, but signal-cli can take a while to + # accept the websocket again after its own reconnect. + until [[ "$(curl -s "${BOT}/healthz" | jq -r '.status // "down"')" == "ok" ]]; do + attempt=$((attempt + 1)) + if [[ $attempt -ge 150 ]]; then + echo "Error: ${BOT}/healthz never reported ok" >&2 + curl -s "${BOT}/healthz" >&2 || true + exit 1 + fi + sleep 0.4 + done +} + +reload_data() { + local reply status + reply="$(curl -s -X POST "${BOT}/_reload")" + status="$(jq -r '.status // "?"' <<<"$reply")" + case "$status" in + reloaded) + echo "reloaded: $(jq -r '.commands' <<<"$reply") commands live" + ;; + kept) + # The old table is still answering; the new data is what is broken. + echo "Error: reload refused, previous table kept: $(jq -r '.reason' <<<"$reply")" >&2 + exit 1 + ;; + *) + echo "Error: unexpected answer from ${BOT}/_reload: ${reply}" >&2 + exit 1 + ;; + esac +} + +restart_code() { + systemctl --user restart "$UNIT" + wait_healthy + echo "restarted: $(curl -s "${BOT}/healthz" | jq -r '"\(.version) \(.pope)"')" +} + +main() { + validate_requirements + if [[ $RESTART -eq 1 ]]; then + restart_code + else + wait_healthy + reload_data + fi + if [[ -n "$PROBE" ]]; then + "${SCRIPT_DIR}/selftest.sh" "$TEST_GROUP" "$PROBE" + fi +} + +while [[ $# -gt 0 ]]; do + case $1 in + --code) + RESTART=1 + shift + ;; + -h | --help) + usage + ;; + *) + PROBE="$1" + shift + ;; + esac +done + +main diff --git a/signal_bot/app.py b/signal_bot/app.py index 4424c88..79cb2d6 100644 --- a/signal_bot/app.py +++ b/signal_bot/app.py @@ -143,3 +143,25 @@ async def selftest(request: SelfTestRequest, response: Response) -> dict[str, st response.status_code = status.HTTP_403_FORBIDDEN return {"status": "refused", "reason": str(refusal)} return {"status": "delivered", "group": group_id, "text": request.text} + + +@app.post("/_reload") +async def reload_commands(response: Response) -> dict[str, str | int]: + """Rebuild the command table from the data on disk, without a restart. + + The answer is what the bot is serving *now*: after a rejected reload + that is still the previous table, and ``reason`` says what was wrong + with the new one. 409 rather than 500, because the data was bad and the + bot is fine. Loopback only, like everything else on this port. + """ + starter: Starter = app.state.starter + runtime = starter.runtime + if runtime is None: + response.status_code = status.HTTP_503_SERVICE_UNAVAILABLE + return {"status": "connecting", "reason": starter.reason} + bot = runtime.bot + if bot.reload_commands(): + return {"status": "reloaded", "commands": bot.command_count} + response.status_code = status.HTTP_409_CONFLICT + holder = bot.dispatcher.holder + return {"status": "kept", "commands": bot.command_count, "reason": holder.last_error} diff --git a/signal_bot/bot.py b/signal_bot/bot.py index 73d9249..d2bac13 100644 --- a/signal_bot/bot.py +++ b/signal_bot/bot.py @@ -14,38 +14,22 @@ from collections.abc import Callable from dataclasses import dataclass from typing import TYPE_CHECKING -from signal_bot.analytics_command import register_analytics_command -from signal_bot.animals import load_aliases, register_animal_commands from signal_bot.announce import announce_all -from signal_bot.art_command import register_art_command from signal_bot.changelog import load as load_changelog -from signal_bot.decorators import cooldown from signal_bot.dispatcher import Dispatcher -from signal_bot.help_command import register_help_command from signal_bot.http_client import ImageFetcher -from signal_bot.pack_command import register_pack_commands -from signal_bot.pack_holder import PackHolder -from signal_bot.pack_load import load as load_pack -from signal_bot.pack_random import register_random_command -from signal_bot.poem_command import register_poem_command -from signal_bot.popes_command import register_pope_command from signal_bot.portraits import PortraitCache -from signal_bot.registry import CommandRegistry +from signal_bot.registry_holder import RegistryHolder from signal_bot.reporting import compose from signal_bot.stats import periods -from signal_bot.stats.commands import register_stats_commands from signal_bot.stats.nightly import days_to_report, mark_reported -from signal_bot.suggest_command import register_suggest_command -from signal_bot.traps_command import register_trap_command from signal_bot.versioning import POPE +from signal_bot.wiring import COOLDOWN_MESSAGE, build_registry if TYPE_CHECKING: - from pathlib import Path - import httpx from signal_bot.config import Config - from signal_bot.decorators import Decorator from signal_bot.discovery import GroupDirectory from signal_bot.liveness import Liveness from signal_bot.store import Store @@ -55,7 +39,9 @@ logger = logging.getLogger(__name__) Clock = Callable[[], dt.datetime] -COOLDOWN_MESSAGE = "BEEP BOOP POCZEKAJ {seconds} {unit}." +# Re-exported: the message lives with the wiring now, and this is where +# every test and the docs have always imported it from. +__all__ = ["COOLDOWN_MESSAGE", "Bot", "Clock", "build_bot", "now_local"] def now_local() -> dt.datetime: @@ -164,18 +150,14 @@ class Bot: self.liveness.note_envelope() await self.dispatcher.handle(frame) + def reload_commands(self) -> bool: + """Rebuild the command table from the data on disk, keeping the old one on failure.""" + return self.dispatcher.holder.reload() -def _register_album(registry: CommandRegistry, directory: Path, rate_limit: Decorator) -> None: - """Wire the album's commands, all of them reading one mutable holder. - - !random takes the same cooldown as every other photo command; !add_photo - takes no decorator at all, per "NO CAP, NO LIMITS" -- no cooldown, no - size cap, no daily limit. - """ - aliases = load_aliases() - holder = PackHolder(load_pack(directory)) - register_pack_commands(registry, holder, decorate=rate_limit) - register_random_command(registry, holder, tuple(aliases["random"]), decorate=rate_limit) + @property + def command_count(self) -> int: + """How many commands the live table holds -- what /_reload reports back.""" + return len(self.dispatcher.registry.commands) def build_bot( @@ -187,46 +169,20 @@ def build_bot( liveness: Liveness, clock: Clock = now_local, ) -> Bot: - """Assemble the object graph from its injected dependencies.""" - registry = CommandRegistry() + """Assemble the object graph from its injected dependencies. + + The registry is built through a holder so that the data behind it -- + aliases, albums -- can be re-read without a restart: see registry_holder. + The fetcher and the portrait cache outlive any one table and are shared + with every rebuild. + """ fetcher = ImageFetcher(client) portraits = PortraitCache(fetcher, config.commons_api_url) - rate_limit = cooldown(config.cooldown_seconds, COOLDOWN_MESSAGE) - register_animal_commands( - registry, - fetcher, - decorate=rate_limit, - endpoints=config.image_api_urls, - ) - register_pope_command(registry, portraits, decorate=rate_limit) - register_trap_command(registry, fetcher, config.commons_api_url, decorate=rate_limit) - register_art_command(registry, fetcher, config.commons_api_url, decorate=rate_limit) - register_poem_command(registry, fetcher, config.poem_text_url, decorate=rate_limit) - register_stats_commands(registry, decorate=rate_limit) - # Reads the *command* history rather than the message history, and - # reads the registry when it runs -- so the album's commands, which - # are registered below this line, are still in the near-miss set. - register_analytics_command(registry, decorate=rate_limit) - # Rate-limited like every other command, and unlike !add_photo, whose - # "NO CAP, NO LIMITS" is a deliberate exception rather than a default. - # The cost is real and accepted: somebody told "napisz !zaproponuj ..." - # who then files two ideas in a row is asked to wait for the second. The - # alternative is one person being able to fill a table nothing prunes. - register_suggest_command(registry, decorate=rate_limit) - # Absent unless this installation has an album, and scoped to that - # album's own groups when it does -- so on every other machine, and in - # every other group, these commands simply do not exist. - # "album", not "directory": build_bot's own `directory` is the group - # directory, and shadowing it here silently passed the wrong object. - for album in config.photo_pack_dirs: - _register_album(registry, album, rate_limit) - # Last, so !pomoc trails the list it prints; it reads the registry - # when it runs, so the order is presentation only. - register_help_command(registry, decorate=rate_limit) + holder = RegistryHolder(lambda: build_registry(config, fetcher, portraits)) dispatcher = Dispatcher( config=config, directory=directory, - registry=registry, + holder=holder, store=store, transport=transport, clock=clock, diff --git a/signal_bot/dispatcher.py b/signal_bot/dispatcher.py index 8e172db..eb0eb93 100644 --- a/signal_bot/dispatcher.py +++ b/signal_bot/dispatcher.py @@ -22,6 +22,7 @@ if TYPE_CHECKING: from signal_bot.discovery import GroupDirectory from signal_bot.models import IncomingMessage from signal_bot.registry import CommandRegistry + from signal_bot.registry_holder import RegistryHolder from signal_bot.store import Store from signal_bot.transport_base import Transport @@ -36,11 +37,19 @@ class Dispatcher: config: Config directory: GroupDirectory - registry: CommandRegistry + # The box, not the table: the table is rebuilt and swapped when the + # data behind it changes, and a dispatcher holding the table itself + # would keep answering from the one it was built with. + holder: RegistryHolder store: Store transport: Transport clock: Clock + @property + def registry(self) -> CommandRegistry: + """The command table as it stands right now.""" + return self.holder.registry + async def _answer_unknown(self, message: IncomingMessage, group: str) -> None: """Answer a "!" word nothing claims, and remember it. diff --git a/signal_bot/pack_load.py b/signal_bot/pack_load.py index b3f1c42..fdea3df 100644 --- a/signal_bot/pack_load.py +++ b/signal_bot/pack_load.py @@ -4,34 +4,25 @@ Every check here fails startup rather than a command, for the reason config_env.py gives: a manifest that names a person who does not exist, a trigger the folder can never match, or a file that has since been renamed is -an answer, not an outage, and no amount of waiting fixes it. The one thing -that is *not* an error is a pack that has not been tagged yet -- that is an -album waiting for the tagger, and it says so in the log. +an answer, not an outage, and no amount of waiting fixes it. What is *not* +an error lives in pack_tags.py: a pack that has not been tagged yet, and a +tag whose file has since gone. """ from __future__ import annotations -import json -import logging from typing import TYPE_CHECKING, Any, Final from signal_bot.folding import fold from signal_bot.groups import ConfigError -from signal_bot.pack import ( - MEDIA_DIR, - MEDIA_TYPES, - PEOPLE_FILE, - TAGS_FILE, - Entry, - Pack, -) +from signal_bot.pack import PEOPLE_FILE, Entry, Pack from signal_bot.pack_custom import load as load_custom +from signal_bot.pack_tags import document as _document +from signal_bot.pack_tags import tags as _tags if TYPE_CHECKING: from pathlib import Path -logger = logging.getLogger(__name__) - # "!" plus at least one character to fold. MIN_TRIGGER_LENGTH: Final = 2 @@ -66,24 +57,6 @@ def load(directory: Path) -> Pack: ) -def _document(path: Path) -> dict[str, Any]: - """One JSON manifest, read strictly.""" - try: - text = path.read_text(encoding="utf-8") - except OSError as error: - msg = f"photo pack manifest {path} cannot be read: {error}" - raise ConfigError(msg) from error - try: - document = json.loads(text) - except json.JSONDecodeError as error: - msg = f"photo pack manifest {path} is not valid JSON: {error}" - raise ConfigError(msg) from error - if not isinstance(document, dict): - msg = f"photo pack manifest {path} must be an object" - raise ConfigError(msg) - return document - - def _groups(document: dict[str, Any]) -> frozenset[str]: """The groups the album may be posted into, which may not be empty. @@ -213,38 +186,3 @@ def _unique_triggers(entries: list[Entry]) -> None: if owner != entry.key: msg = f"{PEOPLE_FILE} trigger {trigger!r} is claimed by {owner!r} and {entry.key!r}" raise ConfigError(msg) - - -def _tags(directory: Path, people: set[str]) -> dict[str, frozenset[str]]: - """Which file shows whom, checked against the album and the manifest. - - A missing tags.json is not an error: that is a pack the tagger has not - been run on yet, and the commands answer "no such photo" until it has. - A tag naming a file that is not there *is* an error -- a renamed file - would otherwise drop out of the album silently. - """ - path = directory / TAGS_FILE - if not path.exists(): - logger.warning("photo pack %s has no %s yet; run ./web.sh", directory, TAGS_FILE) - return {} - document = _document(path) - tags: dict[str, frozenset[str]] = {} - for filename, keys in document.items(): - _media_file(directory, str(filename)) - if not isinstance(keys, list) or any(str(key) not in people for key in keys): - msg = f"{TAGS_FILE} entry {filename!r} names somebody who is not in {PEOPLE_FILE}" - raise ConfigError(msg) - if keys: - tags[str(filename)] = frozenset(str(key) for key in keys) - return tags - - -def _media_file(directory: Path, filename: str) -> None: - """The tagged file has to exist, and be something Signal can render.""" - path = directory / MEDIA_DIR / filename - if path.name != filename or not path.is_file(): - msg = f"{TAGS_FILE} names {filename!r}, which is not a file in {MEDIA_DIR}/" - raise ConfigError(msg) - if path.suffix.lower() not in MEDIA_TYPES: - msg = f"{TAGS_FILE} names {filename!r}, whose type is not one Signal is told about" - raise ConfigError(msg) diff --git a/signal_bot/pack_tags.py b/signal_bot/pack_tags.py new file mode 100644 index 0000000..cede643 --- /dev/null +++ b/signal_bot/pack_tags.py @@ -0,0 +1,96 @@ +# Copyright (c) 2026 Krzysztof Rudnicki. MIT licence. +"""Reading tags.json: which file shows whom, checked against the album. + +Split out of pack_load.py at the 250-line cap, and the half whose checks are +*not* all fatal. The manifest half refuses anything it cannot honour, for +the reason given there; this half has two conditions it survives, because +both are states a healthy album passes through: no tags.json yet, and a tag +whose file is no longer there. +""" + +from __future__ import annotations + +import json +import logging +from typing import TYPE_CHECKING, Any + +from signal_bot.groups import ConfigError +from signal_bot.pack import MEDIA_DIR, MEDIA_TYPES, PEOPLE_FILE, TAGS_FILE + +if TYPE_CHECKING: + from pathlib import Path + +logger = logging.getLogger(__name__) + + +def document(path: Path) -> dict[str, Any]: + """One JSON manifest, read strictly.""" + try: + text = path.read_text(encoding="utf-8") + except OSError as error: + msg = f"photo pack manifest {path} cannot be read: {error}" + raise ConfigError(msg) from error + try: + document = json.loads(text) + except json.JSONDecodeError as error: + msg = f"photo pack manifest {path} is not valid JSON: {error}" + raise ConfigError(msg) from error + if not isinstance(document, dict): + msg = f"photo pack manifest {path} must be an object" + raise ConfigError(msg) + return document + + +def tags(directory: Path, people: set[str]) -> dict[str, frozenset[str]]: + """Which file shows whom, checked against the album and the manifest. + + A missing tags.json is not an error: that is a pack the tagger has not + been run on yet, and the commands answer "no such photo" until it has. + Nor is a tag naming a file that is no longer there: that entry is + skipped, with a warning naming it. It used to be fatal, so a renamed + file could not drop out of the album silently -- and on 2026-09-01 two + such entries appeared in the live pack, which meant the next restart + would have looped on ConfigError. One stale row is drift in a file a + page edits mid-session, not a broken album, and now that the table is + rebuilt from that file while the bot runs, the only safe reading of + drift is "everything else still works". + """ + path = directory / TAGS_FILE + if not path.exists(): + logger.warning("photo pack %s has no %s yet; run ./web.sh", directory, TAGS_FILE) + return {} + tags: dict[str, frozenset[str]] = {} + for filename, keys in document(path).items(): + if not _media_file(directory, str(filename)): + logger.warning( + "%s names %r, which is not a file in %s/ -- skipped", TAGS_FILE, filename, MEDIA_DIR + ) + continue + if not isinstance(keys, list) or any(str(key) not in people for key in keys): + msg = f"{TAGS_FILE} entry {filename!r} names somebody who is not in {PEOPLE_FILE}" + raise ConfigError(msg) + if keys: + tags[str(filename)] = frozenset(str(key) for key in keys) + return tags + + +def _media_file(directory: Path, filename: str) -> bool: + """Whether the tagged file is there; if it is, it must be something Signal can render. + + A name that reaches outside the album is refused outright rather than + skipped: that is not a file that went missing, it is a tag that should + never have been written. + + Raises: + ConfigError: The name is a path, or the type is one Signal cannot show. + """ + path = directory / MEDIA_DIR / filename + if path.name != filename: + msg = f"{TAGS_FILE} names {filename!r}, which is not a file in {MEDIA_DIR}/" + raise ConfigError(msg) + if not path.is_file(): + return False + if path.suffix.lower() not in MEDIA_TYPES: + msg = f"{TAGS_FILE} names {filename!r}, whose type is not one Signal is told about" + raise ConfigError(msg) + return True diff --git a/signal_bot/registry_holder.py b/signal_bot/registry_holder.py new file mode 100644 index 0000000..c82af17 --- /dev/null +++ b/signal_bot/registry_holder.py @@ -0,0 +1,77 @@ +# Copyright (c) 2026 Krzysztof Rudnicki. MIT licence. +"""The one mutable box the command table lives in, so data changes need no restart. + +``PackHolder`` made a *photo* visible the moment it was stored, and said out +loud what it would never do: add a command. A new person in the tagger, a +new alias, a command switched off from the management page -- every one of +those changes the registry itself, and until now every one of them meant +restarting the unit. Restarting is cheap and lossless (signal-cli replays +what arrived meanwhile), but it is a deploy step somebody has to remember, +and a 21:37 report can land in the middle of it. + +So the registry is rebuilt, whole, from the same builder that made it at +startup, and swapped in one assignment. Never patched in place: a table +that is edited command by command can be observed half-edited, and a +duplicate trigger found on the third command of five leaves the first two +already live. Building a fresh table first means ``DuplicateTriggerError`` +and ``ConfigError`` are raised before anything the dispatcher reads has +changed. + +``reload`` never raises on bad *data*. The builder reads files people edit +by hand and pages write mid-session, and the only safe answer to a file +that does not load is the table that was already answering. A programming +error is not data and is not swallowed: it reaches the caller, which for +``POST /_reload`` means a 500 and a traceback in the journal. +""" + +from __future__ import annotations + +import logging +from typing import TYPE_CHECKING + +from signal_bot.groups import ConfigError +from signal_bot.registry import DuplicateTriggerError + +if TYPE_CHECKING: + from collections.abc import Callable + + from signal_bot.registry import CommandRegistry + +logger = logging.getLogger(__name__) + +# A `type` alias is evaluated lazily, so the TYPE_CHECKING-only names are fine. +type Builder = Callable[[], CommandRegistry] + + +class RegistryHolder: + """A mutable reference to the command table the dispatcher reads through.""" + + def __init__(self, build: Builder) -> None: + """Build the first table now, loudly. + + Startup is the one time a bad table must *not* be survived: there is + no previous one to fall back on, and ``Restart=always`` looping on a + ConfigError is a clearer signal than a bot that answers nothing. + """ + self._build = build + self.registry = build() + self.last_error = "" + + def reload(self) -> bool: + """Rebuild the table from whatever is on disk now and publish it. + + Returns: + True when the new table is live, False when the old one is still + being served because the data no longer builds -- and then + ``last_error`` says why, for whoever asked. + """ + try: + fresh = self._build() + except (ConfigError, DuplicateTriggerError) as error: + self.last_error = str(error) + logger.exception("command registry no longer builds; keeping the previous one") + return False + self.registry = fresh + self.last_error = "" + logger.info("command registry rebuilt: %d commands", len(fresh.commands)) + return True diff --git a/signal_bot/wiring.py b/signal_bot/wiring.py new file mode 100644 index 0000000..03bb109 --- /dev/null +++ b/signal_bot/wiring.py @@ -0,0 +1,95 @@ +# Copyright (c) 2026 Krzysztof Rudnicki. MIT licence. +"""Which commands exist: the one function that fills a CommandRegistry. + +Split out of bot.py when the registry stopped being built once. ``build_bot`` +still assembles the object graph; this is the part of it that reads data -- +aliases.json, every album's manifests -- and so the part ``RegistryHolder`` +runs again when that data changes. Everything with a lifetime longer than +one table (the HTTP client, the portrait cache, the store) is passed in, so +a rebuild reuses them rather than opening a second of each. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from signal_bot.analytics_command import register_analytics_command +from signal_bot.animals import load_aliases, register_animal_commands +from signal_bot.art_command import register_art_command +from signal_bot.decorators import cooldown +from signal_bot.help_command import register_help_command +from signal_bot.pack_command import register_pack_commands +from signal_bot.pack_holder import PackHolder +from signal_bot.pack_load import load as load_pack +from signal_bot.pack_random import register_random_command +from signal_bot.poem_command import register_poem_command +from signal_bot.popes_command import register_pope_command +from signal_bot.registry import CommandRegistry +from signal_bot.stats.commands import register_stats_commands +from signal_bot.suggest_command import register_suggest_command +from signal_bot.traps_command import register_trap_command + +if TYPE_CHECKING: + from pathlib import Path + + from signal_bot.config import Config + from signal_bot.decorators import Decorator + from signal_bot.http_client import ImageFetcher + from signal_bot.portraits import PortraitCache + +COOLDOWN_MESSAGE = "BEEP BOOP POCZEKAJ {seconds} {unit}." + + +def _register_album(registry: CommandRegistry, directory: Path, rate_limit: Decorator) -> None: + """Wire the album's commands, all of them reading one mutable holder. + + !random takes the same cooldown as every other photo command. !add_photo + is not wired: a photo posted to a group must not reach the album until + somebody has approved it, and no approval queue exists yet. + """ + aliases = load_aliases() + holder = PackHolder(load_pack(directory)) + register_pack_commands(registry, holder, decorate=rate_limit) + register_random_command(registry, holder, tuple(aliases["random"]), decorate=rate_limit) + + +def build_registry( + config: Config, fetcher: ImageFetcher, portraits: PortraitCache +) -> CommandRegistry: + """Read the data and register every command on a fresh table. + + Raises: + ConfigError: An album no longer loads. + DuplicateTriggerError: Two commands live in one group claim a word. + """ + registry = CommandRegistry() + rate_limit = cooldown(config.cooldown_seconds, COOLDOWN_MESSAGE) + register_animal_commands( + registry, + fetcher, + decorate=rate_limit, + endpoints=config.image_api_urls, + ) + register_pope_command(registry, portraits, decorate=rate_limit) + register_trap_command(registry, fetcher, config.commons_api_url, decorate=rate_limit) + register_art_command(registry, fetcher, config.commons_api_url, decorate=rate_limit) + register_poem_command(registry, fetcher, config.poem_text_url, decorate=rate_limit) + register_stats_commands(registry, decorate=rate_limit) + # Reads the *command* history rather than the message history, and + # reads the registry when it runs -- so the album's commands, which + # are registered below this line, are still in the near-miss set. + register_analytics_command(registry, decorate=rate_limit) + # Rate-limited like every other command. The cost is real and accepted: + # somebody told "napisz !zaproponuj ..." who then files two ideas in a + # row is asked to wait for the second. The alternative is one person + # being able to fill a table nothing prunes. + register_suggest_command(registry, decorate=rate_limit) + # Absent unless this installation has an album, and scoped to that + # album's own groups when it does -- so on every other machine, and in + # every other group, these commands simply do not exist. + for album in config.photo_pack_dirs: + _register_album(registry, album, rate_limit) + # Last, so !pomoc trails the list it prints; it reads the registry + # when it runs, so the order is presentation only. + register_help_command(registry, decorate=rate_limit) + return registry diff --git a/tests/test_analytics_command.py b/tests/test_analytics_command.py index 67ebb35..17f5630 100644 --- a/tests/test_analytics_command.py +++ b/tests/test_analytics_command.py @@ -15,6 +15,7 @@ from signal_bot.analytics.usage import CommandUse, PersonUse from signal_bot.analytics_command import register_analytics_command, triggers_for from signal_bot.dispatcher import Dispatcher from signal_bot.registry import CommandRegistry +from signal_bot.registry_holder import RegistryHolder from signal_bot.stats.events import MessageEvent from tests.analytics_seed import ALA, BASIA, Shape, ran from tests.conftest import WATCHED_GROUP, envelope @@ -220,7 +221,7 @@ async def test_the_command_answers_in_the_group_that_asked( dispatcher = Dispatcher( config=config, directory=directory, - registry=a_registry(), + holder=RegistryHolder(a_registry), store=store, transport=transport, clock=lambda: NOW, diff --git a/tests/test_app.py b/tests/test_app.py index 01708ca..a17feae 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -10,6 +10,7 @@ from fastapi.testclient import TestClient from signal_bot import app as app_module from signal_bot.app import lifespan +from signal_bot.groups import ConfigError from signal_bot.versioning import __version__ from tests.conftest import WATCHED_GROUP, mock_client, signal_client @@ -79,3 +80,53 @@ async def test_lifespan_is_an_async_context_manager(monkeypatch: pytest.MonkeyPa # Present immediately, before anything has reached Signal: /healthz # reads it, and an absent attribute would be a 500 rather than a 503. assert app_module.app.state.starter is not None + + +def test_reload_is_refused_while_still_connecting(monkeypatch: pytest.MonkeyPatch) -> None: + with _serving(monkeypatch, mock_client(_offline)) as client: + response = client.post("/_reload") + assert response.status_code == httpx.codes.SERVICE_UNAVAILABLE + assert response.json()["status"] == "connecting" + + +def _connected(client: TestClient) -> None: + """Spin the loop until the connecting task has produced a runtime.""" + for _ in range(50): + if client.get("/healthz").json()["status"] == "ok": + return + msg = "the bot never connected" + raise AssertionError(msg) + + +def test_reload_rebuilds_the_command_table(monkeypatch: pytest.MonkeyPatch) -> None: + with _serving(monkeypatch, signal_client(WATCHED_GROUP)) as client: + _connected(client) + response = client.post("/_reload") + assert response.status_code == httpx.codes.OK + body = response.json() + assert body["status"] == "reloaded" + assert body["commands"] > 0 + + +def test_reload_keeps_the_old_table_when_the_data_is_bad( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """409, not 500: the bot is fine, the file somebody edited is not.""" + with _serving(monkeypatch, signal_client(WATCHED_GROUP)) as client: + _connected(client) + before = client.post("/_reload").json()["commands"] + + complaint = ConfigError("people.json: !alfa claimed twice") + + def broken(*args: object) -> object: + raise complaint + + monkeypatch.setattr("signal_bot.bot.build_registry", broken) + response = client.post("/_reload") + assert response.status_code == httpx.codes.CONFLICT + body = response.json() + assert body["status"] == "kept" + assert body["commands"] == before + assert "claimed twice" in body["reason"] + # And the old table still answers. + assert client.get("/healthz").json()["status"] == "ok" diff --git a/tests/test_bot.py b/tests/test_bot.py index cea67e8..fd69f1d 100644 --- a/tests/test_bot.py +++ b/tests/test_bot.py @@ -8,8 +8,10 @@ from dataclasses import replace from typing import TYPE_CHECKING import httpx +import pytest from signal_bot.bot import Bot, build_bot, now_local +from signal_bot.groups import ConfigError from signal_bot.liveness import Liveness from signal_bot.stats.events import MessageEvent from signal_bot.stats.periods import WARSAW @@ -125,3 +127,44 @@ def test_an_album_is_wired_up_only_where_there_is_one( assert command is not None assert command.name == "pack:p1" assert command.groups == frozenset({WATCHED_GROUP}) + + +def test_reload_commands_picks_up_a_person_added_to_the_album( + tmp_path: Path, + config: Config, + store: Store, + transport: FakeTransport, + directory: GroupDirectory, +) -> None: + """A person tagged after startup is a command after a reload, not a restart.""" + root = pack_fixture.build(tmp_path / "pack") + bot = _bot(replace(config, photo_pack_dirs=(root,)), store, transport, directory) + before = bot.command_count + assert bot.dispatcher.registry.resolve("!delta") is None + + people = [*pack_fixture.PEOPLE, {"key": "p4", "triggers": ["!delta"]}] + pack_fixture.build(root, people=people, tags={**pack_fixture.TAGS, "delta.png": ["p4"]}) + + assert bot.reload_commands() is True + assert bot.dispatcher.registry.resolve("!delta") is not None + assert bot.command_count == before + 1 + + +def test_reload_commands_keeps_serving_when_the_album_breaks( + tmp_path: Path, + config: Config, + store: Store, + transport: FakeTransport, + directory: GroupDirectory, +) -> None: + """A half-edited manifest must not take the working commands down with it.""" + root = pack_fixture.build(tmp_path / "pack") + bot = _bot(replace(config, photo_pack_dirs=(root,)), store, transport, directory) + (root / "people.json").write_text("{not json", encoding="utf-8") + + assert bot.reload_commands() is False + assert bot.dispatcher.registry.resolve("!alfa") is not None + assert bot.dispatcher.holder.last_error + # And the real loader is what said no -- the same error a restart would die on. + with pytest.raises(ConfigError): + _bot(replace(config, photo_pack_dirs=(root,)), store, transport, directory) diff --git a/tests/test_dispatcher.py b/tests/test_dispatcher.py index 89b90fe..6f43a7c 100644 --- a/tests/test_dispatcher.py +++ b/tests/test_dispatcher.py @@ -10,6 +10,7 @@ import pytest from signal_bot.dispatcher import Dispatcher from signal_bot.registry import CommandRegistry +from signal_bot.registry_holder import RegistryHolder from tests.conftest import OTHER_GROUP, SECOND_GROUP, WATCHED_GROUP, envelope if TYPE_CHECKING: @@ -56,7 +57,7 @@ def dispatcher( return Dispatcher( config=config, directory=directory, - registry=registry, + holder=RegistryHolder(lambda: registry), store=store, transport=transport, clock=lambda: NOW, @@ -156,7 +157,7 @@ async def test_a_failing_command_does_not_kill_the_loop( dispatcher = Dispatcher( config=config, directory=directory, - registry=registry, + holder=RegistryHolder(lambda: registry), store=store, transport=transport, clock=lambda: NOW, @@ -184,7 +185,7 @@ async def test_a_reply_goes_back_to_the_group_that_asked( dispatcher = Dispatcher( config=config, directory=directory, - registry=registry, + holder=RegistryHolder(lambda: registry), store=store, transport=transport, clock=lambda: NOW, diff --git a/tests/test_dispatcher_gates.py b/tests/test_dispatcher_gates.py index a3063b0..bb2187d 100644 --- a/tests/test_dispatcher_gates.py +++ b/tests/test_dispatcher_gates.py @@ -9,6 +9,7 @@ from typing import TYPE_CHECKING from signal_bot.decorators import cooldown from signal_bot.dispatcher import Dispatcher from signal_bot.registry import CommandRegistry +from signal_bot.registry_holder import RegistryHolder from signal_bot.stats.queries_day import day_rows from tests.conftest import SECOND_GROUP, WATCHED_GROUP, envelope @@ -40,7 +41,7 @@ async def test_a_refused_command_records_nothing( dispatcher = Dispatcher( config=config, directory=directory, - registry=registry, + holder=RegistryHolder(lambda: registry), store=store, transport=transport, clock=lambda: NOW, @@ -72,7 +73,7 @@ async def test_waiting_the_stated_time_lets_the_command_run( dispatcher = Dispatcher( config=config, directory=directory, - registry=registry, + holder=RegistryHolder(lambda: registry), store=store, transport=transport, clock=lambda: clock[0], @@ -110,7 +111,7 @@ async def test_a_scoped_command_in_another_group_is_not_a_command_at_all( dispatcher = Dispatcher( config=config, directory=directory, - registry=registry, + holder=RegistryHolder(lambda: registry), store=store, transport=transport, clock=lambda: NOW, @@ -141,7 +142,7 @@ async def test_the_words_after_a_trigger_reach_the_handler( dispatcher = Dispatcher( config=config, directory=directory, - registry=registry, + holder=RegistryHolder(lambda: registry), store=store, transport=transport, clock=lambda: NOW, diff --git a/tests/test_dispatcher_recording.py b/tests/test_dispatcher_recording.py index b3d0946..89bf718 100644 --- a/tests/test_dispatcher_recording.py +++ b/tests/test_dispatcher_recording.py @@ -19,6 +19,7 @@ import pytest from signal_bot.analytics import probe from signal_bot.dispatcher import Dispatcher from signal_bot.registry import CommandRegistry +from signal_bot.registry_holder import RegistryHolder from tests.conftest import WATCHED_GROUP, envelope if TYPE_CHECKING: @@ -61,7 +62,7 @@ def build( return Dispatcher( config=config, directory=directory, - registry=registry, + holder=RegistryHolder(lambda: registry), store=store, transport=transport, clock=lambda: NOW, diff --git a/tests/test_dispatcher_unknown.py b/tests/test_dispatcher_unknown.py index 273bdb0..457a103 100644 --- a/tests/test_dispatcher_unknown.py +++ b/tests/test_dispatcher_unknown.py @@ -17,6 +17,7 @@ from signal_bot.context import CommandContext from signal_bot.discovery import GroupDirectory from signal_bot.dispatcher import Dispatcher from signal_bot.registry import CommandRegistry +from signal_bot.registry_holder import RegistryHolder from tests.conftest import WATCHED_GROUP, envelope if TYPE_CHECKING: @@ -53,7 +54,7 @@ def dispatcher( return Dispatcher( config=config, directory=directory, - registry=registry, + holder=RegistryHolder(lambda: registry), store=store, transport=transport, clock=lambda: NOW, diff --git a/tests/test_pack_add_invariants.py b/tests/test_pack_add_invariants.py index 95ee5a7..178c952 100644 --- a/tests/test_pack_add_invariants.py +++ b/tests/test_pack_add_invariants.py @@ -27,6 +27,7 @@ from signal_bot.pack_load import load from signal_bot.pack_replies import FAILED from signal_bot.pack_store import ADDS_FILE from signal_bot.registry import CommandRegistry +from signal_bot.registry_holder import RegistryHolder from tests import pack_fixture from tests.conftest import WATCHED_GROUP, envelope from tests.pack_add_fixture import ( @@ -71,7 +72,7 @@ async def test_a_typed_label_reaches_no_log_line_no_row_and_no_reply( dispatcher = Dispatcher( config=config, directory=directory, - registry=registry, + holder=RegistryHolder(lambda: registry), store=store, transport=transport, clock=lambda: dt.datetime(2026, 8, 31, 12, 0, tzinfo=dt.UTC), diff --git a/tests/test_pack_holder.py b/tests/test_pack_holder.py index 9ac945a..7b49dcf 100644 --- a/tests/test_pack_holder.py +++ b/tests/test_pack_holder.py @@ -38,9 +38,10 @@ def test_a_pack_that_no_longer_loads_keeps_the_old_one_and_does_not_raise( holder = PackHolder(load(root)) before = holder.pack - # A tag naming a file that is not there: exactly what a half-rolled-back - # write leaves behind. - (root / "tags.json").write_text(json.dumps({"vanished.png": ["p1"]}), encoding="utf-8") + # Half a write: exactly what a page interrupted mid-save leaves behind. + # (A tag naming a vanished file used to be the example here; since + # 2026-09-04 that is skipped with a warning rather than refused.) + (root / "tags.json").write_text('{"alfa.png": ["p1"', encoding="utf-8") assert holder.reload() is False assert holder.pack is before diff --git a/tests/test_pack_load.py b/tests/test_pack_load.py index 1513286..6903d32 100644 --- a/tests/test_pack_load.py +++ b/tests/test_pack_load.py @@ -158,10 +158,21 @@ def test_an_untagged_pack_loads_and_warns(tmp_path: Path, caplog: pytest.LogCapt assert "./web.sh" in caplog.text -def test_a_tag_naming_a_file_that_is_gone_is_refused(tmp_path: Path) -> None: - """A renamed file would otherwise drop out of the album in silence.""" - with pytest.raises(ConfigError, match="not a file"): - _load(tmp_path, tags={"alfa.png": ["p1"]}, media={}) +def test_a_tag_naming_a_file_that_is_gone_is_skipped_out_loud( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """Drift in one row must not take the album down; it is named, then ignored. + + Fatal until 2026-09-04, when two such rows in the live pack meant the + next restart would have looped on ConfigError. + """ + pack = _load( + tmp_path, + tags={"alfa.png": ["p1"], "gone.jpg": ["p2"]}, + media={"alfa.png": pack_fixture.PIXEL}, + ) + assert set(pack.tags) == {"alfa.png"} + assert "'gone.jpg', which is not a file in media/ -- skipped" in caplog.text def test_a_tag_may_not_reach_outside_the_album(tmp_path: Path) -> None: diff --git a/tests/test_registry_holder.py b/tests/test_registry_holder.py new file mode 100644 index 0000000..b924887 --- /dev/null +++ b/tests/test_registry_holder.py @@ -0,0 +1,109 @@ +# Copyright (c) 2026 Krzysztof Rudnicki. MIT licence. +"""The registry holder: rebuilt whole, swapped once, never left half-built.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pytest + +from signal_bot.groups import ConfigError +from signal_bot.registry import CommandRegistry, DuplicateTriggerError +from signal_bot.registry_holder import RegistryHolder + +if TYPE_CHECKING: + from signal_bot.context import CommandContext + + +async def _noop(ctx: CommandContext) -> None: + """A handler that does nothing; only the table's shape matters here.""" + + +def _table(*names: str) -> CommandRegistry: + registry = CommandRegistry() + for name in names: + registry.register(name, [f"!{name}"], _noop, f"{name} description") + return registry + + +class _Builder: + """A builder whose answer can be changed between calls, like data on disk.""" + + def __init__(self, *names: str) -> None: + self.names = names + self.error: Exception | None = None + self.calls = 0 + + def __call__(self) -> CommandRegistry: + self.calls += 1 + if self.error is not None: + raise self.error + return _table(*self.names) + + +def test_the_first_table_is_built_at_construction() -> None: + builder = _Builder("cat") + holder = RegistryHolder(builder) + assert builder.calls == 1 + assert holder.registry.resolve("!cat") is not None + assert holder.last_error == "" + + +def test_a_bad_table_at_startup_is_fatal() -> None: + """No previous table to keep, so the unit must fail rather than answer nothing.""" + builder = _Builder("cat") + builder.error = ConfigError("tags.json names a file that is not there") + with pytest.raises(ConfigError): + RegistryHolder(builder) + + +def test_reload_publishes_the_new_table() -> None: + builder = _Builder("cat") + holder = RegistryHolder(builder) + builder.names = ("cat", "dog") + + assert holder.reload() is True + assert holder.registry.resolve("!dog") is not None + assert {command.name for command in holder.registry.commands} == {"cat", "dog"} + + +@pytest.mark.parametrize( + "error", + [ + ConfigError("people.json is not valid JSON"), + DuplicateTriggerError("!cat is claimed twice"), + ], +) +def test_bad_data_keeps_the_previous_table( + error: Exception, caplog: pytest.LogCaptureFixture +) -> None: + """The table that was answering keeps answering; the caller learns why.""" + builder = _Builder("cat") + holder = RegistryHolder(builder) + before = holder.registry + builder.error = error + + assert holder.reload() is False + assert holder.registry is before + assert holder.last_error == str(error) + assert "keeping the previous one" in caplog.text + + +def test_a_later_good_reload_clears_the_error() -> None: + builder = _Builder("cat") + holder = RegistryHolder(builder) + builder.error = ConfigError("broken") + holder.reload() + builder.error = None + + assert holder.reload() is True + assert holder.last_error == "" + + +def test_a_programming_error_is_not_swallowed() -> None: + """Only bad *data* is survivable; a bug must surface, not become a 409.""" + builder = _Builder("cat") + holder = RegistryHolder(builder) + builder.error = TypeError("register() got an unexpected keyword") + with pytest.raises(TypeError): + holder.reload() diff --git a/tool/e2e_pack.py b/tool/e2e_pack.py index a886e0e..e9c8cee 100755 --- a/tool/e2e_pack.py +++ b/tool/e2e_pack.py @@ -42,15 +42,25 @@ TAGS: Final = { } -def write(directory: Path, group: str) -> None: - """Write the manifest, the tags and the files themselves.""" +def write(directory: Path, group: str, extra: str | None = None) -> None: + """Write the manifest, the tags and the files themselves. + + ``extra`` adds one more person, ``!`` with one photo, on top of + the fixed four -- run a second time over the same directory, that is + what "somebody tagged a new person while the bot was up" looks like, + and what POST /_reload has to pick up without a restart. + """ + people, tags = list(PEOPLE), dict(TAGS) + if extra is not None: + people.append({"key": "p5", "triggers": [f"!{extra}"]}) + tags[f"{extra}.png"] = ["p5"] album = directory / "media" album.mkdir(parents=True, exist_ok=True) - for name in TAGS: + for name in tags: (album / name).write_bytes(CLIP if name.endswith(".mp4") else PIXEL) - manifest = {"groups": [group], "people": PEOPLE, "combos": COMBOS} + manifest = {"groups": [group], "people": people, "combos": COMBOS} (directory / "people.json").write_text(json.dumps(manifest, indent=2), encoding="utf-8") - (directory / "tags.json").write_text(json.dumps(TAGS, indent=2), encoding="utf-8") + (directory / "tags.json").write_text(json.dumps(tags, indent=2), encoding="utf-8") def main() -> int: @@ -58,8 +68,9 @@ def main() -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--dir", required=True, type=Path, help="where to write the pack") parser.add_argument("--group", required=True, help="the group id the pack belongs to") + parser.add_argument("--extra", help="one more person to add, by trigger word") arguments = parser.parse_args() - write(arguments.dir, arguments.group) + write(arguments.dir, arguments.group, arguments.extra) sys.stdout.write(f"== wrote a synthetic pack to {arguments.dir}\n") return 0