Merge pull request #15 from onetimesecret/delano/next

Enhance restart reliability, add shell options, improve secrets validation

Author: delano

.pre-commit-config.yaml

@@ -82,11 +82,12 @@ repos:
           - "--template=[#{}]"
 
   # Pyright - static type checking (matches IDE)
+  # Only checks changed files rather than all of src/ for speed
   - repo: https://github.com/RobertCraigie/pyright-python
     rev: v1.1.390
     hooks:
       - id: pyright
-        args: [src/]
+        files: ^src/
         additional_dependencies:
           - cyclopts>=3.9
           - pytest>=8.0
@@ -99,23 +100,13 @@ repos:
         args: [--fix]
       - id: ruff-format
 
-  # pytest - run tests before commit (fast, no coverage)
-  # Only runs when Python files in src/ or tests/ are modified
+  # pytest - run before push with coverage and last-failed optimization
+  # Skipped on commit for speed; full suite gates push instead
   - repo: local
     hooks:
-      - id: pytest
-        name: pytest
-        entry: .venv/bin/pytest tests/ -x -q
-        language: system
-        pass_filenames: false
-        files: ^(src/|tests/).*\.py$
-        stages: [pre-commit]
-
-      # pytest with coverage - run before push
-      # Only runs when Python files in src/ or tests/ are modified
       - id: pytest-cov
         name: pytest-cov
-        entry: .venv/bin/pytest tests/ -x -q --cov=ots_containers --cov-report=term-missing --cov-fail-under=70
+        entry: .venv/bin/pytest tests/ -x -q --ff --cov=ots_containers --cov-report=term-missing --cov-fail-under=70
         language: system
         pass_filenames: false
         files: ^(src/|tests/).*\.py$

pyproject.toml

@@ -13,7 +13,7 @@ build-backend = "hatchling.build"
 
 [project]
 name = "ots-containers"
-version = "0.3.2"
+version = "0.3.3"
 description = "Service orchestration for OneTimeSecret: Podman Quadlets and systemd service management"
 readme = "README.md"
 requires-python = ">=3.11"

src/ots_containers/commands/cloudinit/app.py

@@ -43,7 +43,8 @@ def generate(
         bool, cyclopts.Parameter(help="Include Valkey apt repository")
     ] = False,
     include_xcaddy: Annotated[
-        bool, cyclopts.Parameter(help="Include xcaddy repo and build custom Caddy (web profile)")
+        bool,
+        cyclopts.Parameter(help="Include xcaddy repo and build custom Caddy (web profile)"),
     ] = False,
     caddy_version: Annotated[
         str,

src/ots_containers/commands/instance/_helpers.py

@@ -3,12 +3,14 @@
 """Internal helper functions for instance commands."""
 
 import shlex
+import sys
 import time
 from collections.abc import Callable, Sequence
 from pathlib import Path
 
 from ots_containers import systemd
 from ots_containers.environment_file import get_secrets_from_env_file
+from ots_containers.systemd import SystemctlError
 
 from .annotations import InstanceType
 
@@ -61,7 +63,12 @@ def build_secret_args(env_file: Path) -> list[str]:
     secret_specs = get_secrets_from_env_file(env_file)
     args: list[str] = []
     for spec in secret_specs:
-        args.extend(["--secret", f"{spec.secret_name},type=env,target={spec.env_var_name}"])
+        args.extend(
+            [
+                "--secret",
+                f"{spec.secret_name},type=env,target={spec.env_var_name}",
+            ]
+        )
     return args
 
 
@@ -171,7 +178,15 @@ def for_each_instance(
     for i, (itype, id_) in enumerate(items, 1):
         unit = systemd.unit_name(itype.value, id_)
         print(f"[{i}/{total}] {verb} {unit}...")
-        action(itype, id_)
+        try:
+            action(itype, id_)
+        except SystemctlError as exc:
+            print(f"Error: {exc}", file=sys.stderr)
+            if exc.journal:
+                print(f"\nRecent journal output for {exc.unit}:", file=sys.stderr)
+                for line in exc.journal.splitlines():
+                    print(f"  {line}", file=sys.stderr)
+            raise SystemExit(1) from None
         if i < total and delay > 0:
             print(f"Waiting {delay}s...")
             time.sleep(delay)

src/ots_containers/commands/instance/app.py

@@ -71,7 +71,9 @@ def _list_instances_impl(
                 else:
                     # Worker/scheduler: query by notes containing instance ID
                     deployments = db.get_deployments(
-                        cfg.db_path, limit=1, notes_like=f"%{inst_type.value}_id={id_}%"
+                        cfg.db_path,
+                        limit=1,
+                        notes_like=f"%{inst_type.value}_id={id_}%",
                     )
                 if deployments:
                     dep = deployments[0]
@@ -132,7 +134,9 @@ def _list_instances_impl(
             else:
                 # Worker/scheduler: query by notes containing instance ID
                 deployments = db.get_deployments(
-                    cfg.db_path, limit=1, notes_like=f"%{inst_type.value}_id={id_}%"
+                    cfg.db_path,
+                    limit=1,
+                    notes_like=f"%{inst_type.value}_id={id_}%",
                 )
             if deployments:
                 dep = deployments[0]
@@ -737,17 +741,20 @@ def restart(
     web: WebFlag = False,
     worker: WorkerFlag = False,
     scheduler: SchedulerFlag = False,
+    delay: Delay = 30,
 ):
     """Restart systemd unit(s) for instance(s).
 
     Does NOT regenerate quadlet - use 'redeploy' for that.
     Only restarts running instances; stopped instances are skipped.
+    Waits between instances to allow startup before Caddy health checks.
 
     Examples:
         ots instances restart                       # Restart all running
         ots instances restart --web                 # Restart web instances
         ots instances restart --web 7043 7044       # Restart specific web
         ots instances restart --scheduler main      # Restart specific scheduler
+        ots instances restart --delay 10            # Longer wait between restarts
     """
     itype = resolve_instance_type(instance_type, web, worker, scheduler)
     instances = resolve_identifiers(identifiers, itype, running_only=True)
@@ -756,15 +763,11 @@ def restart(
         print("No running instances found")
         return
 
-    for inst_type, ids in instances.items():
-        for id_ in ids:
-            unit = systemd.unit_name(inst_type.value, id_)
-            systemd.restart(unit)
-            print(f"Restarted {unit}")
+    def do_restart(inst_type: InstanceType, id_: str) -> None:
+        unit = systemd.unit_name(inst_type.value, id_)
+        systemd.restart(unit)
 
-    hint = format_journalctl_hint(instances)
-    if hint:
-        print(f"\nView logs: {hint}")
+    for_each_instance(instances, delay, do_restart, "Restarting", show_logs_hint=True)
 
 
 @app.command
@@ -1019,6 +1022,20 @@ def shell(
             help="Named volume for persistent data (survives exit)",
         ),
     ] = None,
+    volume: Annotated[
+        str | None,
+        cyclopts.Parameter(
+            name=["--volume", "-v"],
+            help="Host path to bind-mount at /app/data (rw, user-mapped)",
+        ),
+    ] = None,
+    env: Annotated[
+        tuple[str, ...],
+        cyclopts.Parameter(
+            name=["--env", "-e"],
+            help="Set container env var (KEY=VALUE, repeatable)",
+        ),
+    ] = (),
     command: Annotated[
         str | None,
         cyclopts.Parameter(
@@ -1046,14 +1063,24 @@ def shell(
 
     By default uses tmpfs at /app/data (data destroyed on exit).
     Use --persistent to create a named volume that survives exit.
+    Use --volume to bind-mount a host directory at /app/data.
     Config is mounted read-only at /app/etc.
 
     Examples:
         ots instance shell                              # tmpfs, interactive bash
         ots instance shell --persistent upgrade-v024    # named volume survives exit
         ots instance shell -c "bin/ots migrate"         # run command and exit
         ots instance shell --tag v0.24.0                # specific image tag
+        ots instance shell -v ./data                    # bind-mount ./data at /app/data
+        ots instance shell -v ./data -e REDIS_URL=redis://10.0.0.5:6379/0  # with env
+        ots instance shell -e FOO=bar -e BAZ=qux        # multiple env vars, tmpfs default
     """
+    from pathlib import Path
+
+    if persistent and volume:
+        print("Error: --persistent and --volume are mutually exclusive")
+        raise SystemExit(1)
+
     cfg = Config()
 
     # Resolve image/tag (same pattern as run command)
@@ -1084,13 +1111,21 @@ def shell(
         cmd.extend(["--env-file", str(env_file)])
         cmd.extend(build_secret_args(env_file))
 
-    # Data volume: tmpfs (default) or persistent named volume
-    if persistent:
+    # Data volume: bind-mount, persistent named volume, or tmpfs (default)
+    if volume:
+        host_path = Path(volume).resolve()
+        host_path.mkdir(parents=True, exist_ok=True)
+        cmd.extend(["-v", f"{host_path}:/app/data:rw,U"])
+    elif persistent:
         volume_name = f"ots-migration-{persistent}"
         cmd.extend(["-v", f"{volume_name}:/app/data"])
     else:
         cmd.extend(["--tmpfs", "/app/data"])
 
+    # Ad-hoc environment variables
+    for entry in env:
+        cmd.extend(["-e", entry])
+
     # Config overrides (per-file, if any exist on host)
     for f in cfg.existing_config_files:
         resolved = f.resolve()  # symlink resolution for macOS podman VM

src/ots_containers/commands/proxy/_helpers.py

@@ -69,7 +69,14 @@ def validate_caddy_config(content: str) -> None:
 
     try:
         result = subprocess.run(
-            ["caddy", "validate", "--config", temp_path, "--adapter", "caddyfile"],
+            [
+                "caddy",
+                "validate",
+                "--config",
+                temp_path,
+                "--adapter",
+                "caddyfile",
+            ],
             capture_output=True,
             text=True,
         )

src/ots_containers/commands/proxy/app.py

@@ -32,7 +32,7 @@
     Path | None,
     cyclopts.Parameter(
         name=["--template", "-t"],
-        help="Template file path (default: /etc/onetimesecret/Caddyfile.template)",
+        help="Template file path (e.g. /etc/onetimesecret/Caddyfile.template)",
     ),
 ]
 

src/ots_containers/config.py

@@ -10,7 +10,12 @@
 
 # Config files that ship as defaults in the container image (etc/defaults/*.defaults.yaml).
 # Only files present on the host override the container's built-in defaults.
-CONFIG_FILES: tuple[str, ...] = ("config.yaml", "auth.yaml", "logging.yaml")
+CONFIG_FILES: tuple[str, ...] = (
+    "config.yaml",
+    "auth.yaml",
+    "logging.yaml",
+    "billing.yaml",
+)
 
 
 @dataclass

src/ots_containers/environment_file.py

@@ -320,9 +320,8 @@ def extract_secrets(env_file: EnvFile) -> tuple[list[SecretSpec], list[str]]:
             continue
 
         # No entry found - neither VARNAME nor _VARNAME exists
+        # Skip adding to secrets list so quadlet won't reference a non-existent secret
         messages.append(f"Warning: {var_name} listed in SECRET_VARIABLE_NAMES but not found")
-        # Still include in secrets list so quadlet line is generated
-        secrets.append(SecretSpec.from_env_var(var_name, value=None))
 
     return secrets, messages
 
@@ -406,12 +405,16 @@ def ensure_podman_secret(secret_name: str, value: str) -> str:
 
 
 def secret_exists(secret_name: str) -> bool:
-    """Check if a podman secret exists."""
-    result = subprocess.run(
-        ["podman", "secret", "exists", secret_name],
-        capture_output=True,
-    )
-    return result.returncode == 0
+    """Check if a podman secret exists. Returns False if podman is unavailable."""
+    try:
+        result = subprocess.run(
+            ["podman", "secret", "exists", secret_name],
+            capture_output=True,
+            timeout=10,
+        )
+        return result.returncode == 0
+    except (subprocess.SubprocessError, OSError):
+        return False
 
 
 def generate_quadlet_secret_lines(secrets: list[SecretSpec]) -> str:

src/ots_containers/quadlet.py

@@ -14,6 +14,7 @@
 from .environment_file import (
     generate_quadlet_secret_lines,
     get_secrets_from_env_file,
+    secret_exists,
 )
 
 # Default environment file path
@@ -119,7 +120,23 @@ def get_secrets_section(env_file_path: Path | None = None) -> str:
     if not secrets:
         return "# No secrets configured (SECRET_VARIABLE_NAMES not set in env file)"
 
-    return generate_quadlet_secret_lines(secrets)
+    # Defense-in-depth: only include secrets that actually exist as podman secrets.
+    # A secret may have been processed in the env file but later deleted from the
+    # podman secret store. Warn rather than letting podman fail at container start.
+    verified = []
+    for spec in secrets:
+        if secret_exists(spec.secret_name):
+            verified.append(spec)
+        else:
+            print(
+                f"Warning: podman secret '{spec.secret_name}' not found, "
+                f"skipping Secret= line for {spec.env_var_name}"
+            )
+
+    if not verified:
+        return "# No secrets configured (none found in podman secret store)"
+
+    return generate_quadlet_secret_lines(verified)
 
 
 def get_config_volumes_section(cfg: Config) -> str: