From 1166e13e441c837d13434602396104775588a9c0 Mon Sep 17 00:00:00 2001 From: mwiegand Date: Fri, 8 May 2026 19:22:09 +0200 Subject: [PATCH] feat(l4d2-web): server identity by id, name as display label Host-side identifier (systemd unit name and /var/lib/left4me dirs) is now str(server.id), centralized in services/server_identity.server_unit_name. Server.name becomes a free-form display label, required and unique per user (was [a-z0-9_-]{1,64} and globally unique). Migration 0006 swaps the old global UNIQUE(name) for UNIQUE(user_id, name). Web routes already keyed on id; templates only used name for display. Co-Authored-By: Claude Opus 4.7 (1M context) --- ...-08-server-id-as-host-identifier-design.md | 55 ++++++++++ .../versions/0006_server_name_per_user.py | 44 ++++++++ l4d2web/models.py | 5 +- l4d2web/routes/log_routes.py | 3 +- l4d2web/routes/server_routes.py | 22 +++- l4d2web/services/job_worker.py | 3 +- l4d2web/services/l4d2_facade.py | 19 ++-- l4d2web/services/security.py | 14 --- l4d2web/services/server_identity.py | 5 + l4d2web/templates/servers.html | 2 +- l4d2web/tests/test_l4d2_facade.py | 11 +- l4d2web/tests/test_servers.py | 100 +++++++++++++++++- 12 files changed, 246 insertions(+), 37 deletions(-) create mode 100644 docs/superpowers/specs/2026-05-08-server-id-as-host-identifier-design.md create mode 100644 l4d2web/alembic/versions/0006_server_name_per_user.py create mode 100644 l4d2web/services/server_identity.py diff --git a/docs/superpowers/specs/2026-05-08-server-id-as-host-identifier-design.md b/docs/superpowers/specs/2026-05-08-server-id-as-host-identifier-design.md new file mode 100644 index 0000000..8526db6 --- /dev/null +++ b/docs/superpowers/specs/2026-05-08-server-id-as-host-identifier-design.md @@ -0,0 +1,55 @@ +# Server ID as Host Identifier Design + +**Goal:** Decouple the user-facing server label from the host-side identifier. The systemd unit name and on-disk paths become functions of `Server.id`; `Server.name` becomes a free-form display label. + +**Approval status:** User-approved 2026-05-08. + +## Context + +`Server.name` was doing two unrelated jobs. It was the human label rendered in the UI *and* the literal string fed to `l4d2ctl`, which became the systemd unit instance (`left4me-server@.service`) and the directories under `/var/lib/left4me/{instances,runtime}//`. To stay safe as a unit-template parameter and a path component, the name was forced through `[a-z0-9][a-z0-9_-]{0,63}` and held globally unique. The cost was a UI that demanded machine-friendly slugs, no rename support, and an awkward divergence from overlays — which already separate identity (`id`) from label (`name`). + +This change moves servers onto the same model as overlays. Web URLs already key on `id` (`/servers/`), so the change is mostly local: pick an id-derived host identifier, pass that everywhere `server.name` was passed, and relax the `name` constraints. + +## Locked Decisions + +1. **Host-side identifier = plain numeric id.** `left4me-server@42.service`, `/var/lib/left4me/instances/42/`, `/var/lib/left4me/runtime/42/`. The host CLI's `validate_instance_name` regex (`[a-z0-9][a-z0-9_-]{0,63}`), the systemctl helper's argument check (`[A-Za-z0-9_.-]`), and the unit template (`%i`) all already accept digit-only strings — no host-side change. +2. **Name = free-form display label, unique per user, required (≤128 chars).** Whitespace is stripped on save. Two users can both have a server named "Practice"; one user cannot. +3. **No data preservation.** Dev-only deploy. Existing servers on the test host are not migrated; their old `left4me-server@.service` units and `/` directories become orphans and are cleaned up manually. +4. **Single source of truth for the id-to-host-name rule.** A one-line helper (`server_unit_name(server_id) -> str(server_id)`) lives in `l4d2web/services/server_identity.py`. Every callsite that used to pass `server.name` to `l4d2ctl` or `journalctl` calls this. Future format tweaks (e.g. `srv-{id}`) are a one-line edit. + +## Schema + +`servers` (Alembic 0006): +- Drop the (unnamed) global `UNIQUE (name)` from the original 0001 schema. +- Add `UNIQUE (user_id, name)` as `uq_servers_user_name`. +- Column stays `name VARCHAR(128) NOT NULL`. + +The migration uses `batch_alter_table(recreate="always")` with a `naming_convention` so the originally-anonymous unique can be referenced as `uq_servers_name` for `drop_constraint`. + +## Code touchpoints + +- `l4d2web/services/server_identity.py` (new) +- `l4d2web/models.py` — drop `unique=True` on `Server.name`; add `__table_args__` with the per-user unique. +- `l4d2web/alembic/versions/0006_server_name_per_user.py` (new) +- `l4d2web/services/l4d2_facade.py` — five `l4d2ctl` invocations switched to `server_unit_name(server.id)`. Parameter renamed to `unit_name` on `server_status` / `stream_server_logs`. +- `l4d2web/services/job_worker.py` — status refresh uses `server_unit_name(server.id)`. The `server_name` log-label variable still holds `server.name` (the display label); that's correct now and shows up in job logs as e.g. "starting initialize for My Practice". +- `l4d2web/routes/log_routes.py` — SSE log stream feeds `server_unit_name(server.id)` to `journalctl`. +- `l4d2web/routes/server_routes.py` — replace `validate_instance_name` with `_validate_display_name` (strip + non-empty + length ≤128). Broaden the `IntegrityError` handler to disambiguate `servers.name` (409 "name already in use") from `servers.port` (409 "port already in use") via the underlying SQLite error string. +- `l4d2web/services/security.py` — `validate_instance_name` deleted (no remaining callers). +- `l4d2web/templates/servers.html` — name input gains `maxlength="128"`. + +## Failure modes + +- **Name with shell metacharacters reaches a host command.** Cannot happen — the host call now receives only `str(server.id)` (digits). The display name is never passed through `l4d2ctl`. +- **Two servers under the same user with the same name.** Blocked at the DB layer (`uq_servers_user_name`); surfaced as a 409 "name already in use" with no row written. +- **Migration on a DB with existing servers.** `batch_alter_table(recreate="always")` rebuilds the table preserving rows; the new per-user constraint is satisfied trivially since the old global constraint already enforced strict uniqueness. + +## Verification + +1. `python -m pytest l4d2web l4d2host deploy` from the repo root — green. +2. Stepwise migration on a fresh sqlite (upgrade to 0005, insert two users + a server, upgrade to 0006): row preserved, second user can take the same name, same user cannot (UNIQUE constraint failed: servers.user_id, servers.name). +3. Post-deploy on the test host: create a server named `"My Practice"` (with the space), confirm the systemd unit is `left4me-server@.service`, confirm `/var/lib/left4me/runtime//merged` is mounted on start, confirm log streaming still works. + +## Operator note + +After deploy, on the test host: stop and remove any pre-existing `left4me-server@.service` units and their `/var/lib/left4me/{instances,runtime}//` directories. The web app no longer references them. diff --git a/l4d2web/alembic/versions/0006_server_name_per_user.py b/l4d2web/alembic/versions/0006_server_name_per_user.py new file mode 100644 index 0000000..47eb1e1 --- /dev/null +++ b/l4d2web/alembic/versions/0006_server_name_per_user.py @@ -0,0 +1,44 @@ +"""server name unique per user + +Revision ID: 0006_server_name_per_user +Revises: 0005_script_overlays +Create Date: 2026-05-08 +""" +from typing import Sequence, Union + +from alembic import op + + +revision: str = "0006_server_name_per_user" +down_revision: Union[str, Sequence[str], None] = "0005_script_overlays" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +# 0001_initial defined `name` with column-level `unique=True`, which SQLite +# stored as an unnamed UNIQUE constraint. The naming_convention here lets +# batch_alter_table refer to it as "uq_servers_name" so we can drop it before +# recreating the table with the new (user_id, name) composite. +_NAMING_CONVENTION = {"uq": "uq_%(table_name)s_%(column_0_name)s"} + + +def upgrade() -> None: + with op.batch_alter_table( + "servers", + naming_convention=_NAMING_CONVENTION, + recreate="always", + ) as batch_op: + batch_op.drop_constraint("uq_servers_name", type_="unique") + batch_op.create_unique_constraint( + "uq_servers_user_name", ["user_id", "name"] + ) + + +def downgrade() -> None: + with op.batch_alter_table( + "servers", + naming_convention=_NAMING_CONVENTION, + recreate="always", + ) as batch_op: + batch_op.drop_constraint("uq_servers_user_name", type_="unique") + batch_op.create_unique_constraint("uq_servers_name", ["name"]) diff --git a/l4d2web/models.py b/l4d2web/models.py index aa3a7e9..b3a5478 100644 --- a/l4d2web/models.py +++ b/l4d2web/models.py @@ -123,11 +123,14 @@ class BlueprintOverlay(Base): class Server(Base): __tablename__ = "servers" + __table_args__ = ( + UniqueConstraint("user_id", "name", name="uq_servers_user_name"), + ) id: Mapped[int] = mapped_column(Integer, primary_key=True) user_id: Mapped[int] = mapped_column(ForeignKey("users.id"), nullable=False) blueprint_id: Mapped[int] = mapped_column(ForeignKey("blueprints.id"), nullable=False) - name: Mapped[str] = mapped_column(String(128), unique=True, nullable=False) + name: Mapped[str] = mapped_column(String(128), nullable=False) port: Mapped[int] = mapped_column(Integer, unique=True, nullable=False) desired_state: Mapped[str] = mapped_column(String(16), default="stopped", nullable=False) actual_state: Mapped[str] = mapped_column(String(16), default="unknown", nullable=False) diff --git a/l4d2web/routes/log_routes.py b/l4d2web/routes/log_routes.py index 5f80eb7..7debc48 100644 --- a/l4d2web/routes/log_routes.py +++ b/l4d2web/routes/log_routes.py @@ -5,6 +5,7 @@ from l4d2web.auth import current_user, require_login from l4d2web.db import session_scope from l4d2web.models import Server from l4d2web.services import l4d2_facade as facade +from l4d2web.services.server_identity import server_unit_name bp = Blueprint("logs", __name__) @@ -27,7 +28,7 @@ def stream_server_logs(server_id: int) -> Response: return Response(status=404) def generate(): - for line in facade.stream_server_logs(server.name, lines=200, follow=True): + for line in facade.stream_server_logs(server_unit_name(server.id), lines=200, follow=True): if line == "": yield ": keepalive\n\n" else: diff --git a/l4d2web/routes/server_routes.py b/l4d2web/routes/server_routes.py index 805a943..bb77c0a 100644 --- a/l4d2web/routes/server_routes.py +++ b/l4d2web/routes/server_routes.py @@ -6,12 +6,25 @@ from l4d2web.auth import current_user, require_login from l4d2web.db import session_scope from l4d2web.models import Blueprint as BlueprintModel from l4d2web.models import Job, Server -from l4d2web.services.security import validate_instance_name bp = Blueprint("server", __name__) +_NAME_MAX_LENGTH = 128 + + +def _validate_display_name(raw: object) -> str: + if not isinstance(raw, str): + raise ValueError("name must be a string") + cleaned = raw.strip() + if not cleaned: + raise ValueError("name must not be empty") + if len(cleaned) > _NAME_MAX_LENGTH: + raise ValueError("name too long") + return cleaned + + def _allocate_next_port(db) -> int | None: start = int(current_app.config["PORT_RANGE_START"]) end = int(current_app.config["PORT_RANGE_END"]) @@ -51,7 +64,7 @@ def create_server() -> Response: payload = request.get_json(silent=True) if json_response else request.form try: - name = validate_instance_name(str(payload["name"])) + name = _validate_display_name(payload["name"]) except (KeyError, TypeError, ValueError): return Response("invalid server name", status=400) @@ -82,8 +95,11 @@ def create_server() -> Response: try: db.flush() - except IntegrityError: + except IntegrityError as exc: db.rollback() + detail = str(exc.orig) if exc.orig is not None else str(exc) + if "servers.name" in detail: + return Response("name already in use", status=409) return Response("port already in use", status=409) server_id = server.id diff --git a/l4d2web/services/job_worker.py b/l4d2web/services/job_worker.py index 37cb959..5dad4ed 100644 --- a/l4d2web/services/job_worker.py +++ b/l4d2web/services/job_worker.py @@ -21,6 +21,7 @@ from l4d2web.models import ( WorkshopItem, ) from l4d2web.services.host_commands import CommandCancelledError +from l4d2web.services.server_identity import server_unit_name TERMINAL_JOB_STATES = {"succeeded", "failed", "cancelled"} @@ -570,7 +571,7 @@ def refresh_server_actual_state(server_id: int) -> str: server = db.scalar(select(Server).where(Server.id == server_id)) if server is None: return "unknown" - status = l4d2_facade.server_status(server.name) + status = l4d2_facade.server_status(server_unit_name(server.id)) server.actual_state = status.state server.actual_state_updated_at = now server.updated_at = now diff --git a/l4d2web/services/l4d2_facade.py b/l4d2web/services/l4d2_facade.py index fc741da..929b375 100644 --- a/l4d2web/services/l4d2_facade.py +++ b/l4d2web/services/l4d2_facade.py @@ -14,6 +14,7 @@ from l4d2web.models import ( WorkshopItem, ) from l4d2web.services import host_commands +from l4d2web.services.server_identity import server_unit_name from l4d2web.services.spec_yaml import write_temp_spec from l4d2web.services.workshop_paths import cache_path @@ -83,7 +84,7 @@ def initialize_server(server_id: int, on_stdout=None, on_stderr=None, should_can spec_path = write_temp_spec(build_server_spec_payload(server, blueprint, overlay_refs)) try: host_commands.run_command( - ["l4d2ctl", "initialize", server.name, "-f", str(spec_path)], + ["l4d2ctl", "initialize", server_unit_name(server.id), "-f", str(spec_path)], on_stdout=on_stdout, on_stderr=on_stderr, should_cancel=should_cancel, @@ -176,7 +177,7 @@ def _check_workshop_overlay_caches(*, blueprint_id: int) -> None: def start_server(server_id: int, on_stdout=None, on_stderr=None, should_cancel=None) -> None: server, _, _ = load_server_blueprint_bundle(server_id) host_commands.run_command( - ["l4d2ctl", "start", server.name], + ["l4d2ctl", "start", server_unit_name(server.id)], on_stdout=on_stdout, on_stderr=on_stderr, should_cancel=should_cancel, @@ -186,7 +187,7 @@ def start_server(server_id: int, on_stdout=None, on_stderr=None, should_cancel=N def stop_server(server_id: int, on_stdout=None, on_stderr=None, should_cancel=None) -> None: server, _, _ = load_server_blueprint_bundle(server_id) host_commands.run_command( - ["l4d2ctl", "stop", server.name], + ["l4d2ctl", "stop", server_unit_name(server.id)], on_stdout=on_stdout, on_stderr=on_stderr, should_cancel=should_cancel, @@ -196,7 +197,7 @@ def stop_server(server_id: int, on_stdout=None, on_stderr=None, should_cancel=No def delete_server(server_id: int, on_stdout=None, on_stderr=None, should_cancel=None) -> None: server, _, _ = load_server_blueprint_bundle(server_id) host_commands.run_command( - ["l4d2ctl", "delete", server.name], + ["l4d2ctl", "delete", server_unit_name(server.id)], on_stdout=on_stdout, on_stderr=on_stderr, should_cancel=should_cancel, @@ -206,15 +207,15 @@ def delete_server(server_id: int, on_stdout=None, on_stderr=None, should_cancel= def reset_server(server_id: int, on_stdout=None, on_stderr=None, should_cancel=None) -> None: server, _, _ = load_server_blueprint_bundle(server_id) host_commands.run_command( - ["l4d2ctl", "reset", server.name], + ["l4d2ctl", "reset", server_unit_name(server.id)], on_stdout=on_stdout, on_stderr=on_stderr, should_cancel=should_cancel, ) -def server_status(server_name: str) -> ServerStatus: - result = host_commands.run_command(["l4d2ctl", "status", server_name, "--json"]) +def server_status(unit_name: str) -> ServerStatus: + result = host_commands.run_command(["l4d2ctl", "status", unit_name, "--json"]) payload = json.loads(result.stdout or "{}") return ServerStatus( state=str(payload.get("state", "unknown")), @@ -223,7 +224,7 @@ def server_status(server_name: str) -> ServerStatus: ) -def stream_server_logs(server_name: str, *, lines: int = 200, follow: bool = True): - command = ["l4d2ctl", "logs", server_name, "--lines", str(lines)] +def stream_server_logs(unit_name: str, *, lines: int = 200, follow: bool = True): + command = ["l4d2ctl", "logs", unit_name, "--lines", str(lines)] command.append("--follow" if follow else "--no-follow") return host_commands.stream_command(command) diff --git a/l4d2web/services/security.py b/l4d2web/services/security.py index 4ac7dbf..6ff0b52 100644 --- a/l4d2web/services/security.py +++ b/l4d2web/services/security.py @@ -1,17 +1,3 @@ -import re - - -_INSTANCE_NAME_RE = re.compile(r"^[a-z0-9][a-z0-9_-]{0,63}$") - - -def validate_instance_name(raw: str) -> str: - if not _INSTANCE_NAME_RE.fullmatch(raw): - raise ValueError( - "instance name must match [a-z0-9][a-z0-9_-]{0,63}" - ) - return raw - - def validate_overlay_ref(raw: str) -> str: if raw != raw.strip(): raise ValueError("overlay ref must not have leading or trailing whitespace") diff --git a/l4d2web/services/server_identity.py b/l4d2web/services/server_identity.py new file mode 100644 index 0000000..fb0d3b9 --- /dev/null +++ b/l4d2web/services/server_identity.py @@ -0,0 +1,5 @@ +def server_unit_name(server_id: int) -> str: + """Host-side identifier for a server (systemd unit suffix and on-disk + directory name). Lives in one place so a future format change is a + single-line edit.""" + return str(server_id) diff --git a/l4d2web/templates/servers.html b/l4d2web/templates/servers.html index e106bba..fdc1a8f 100644 --- a/l4d2web/templates/servers.html +++ b/l4d2web/templates/servers.html @@ -39,7 +39,7 @@
- +