From da56f2acb86b8ec38d96b09dda0e2627ac1ca996 Mon Sep 17 00:00:00 2001 From: Christopher Graham Date: Thu, 16 Jul 2026 19:02:10 -0400 Subject: [PATCH 1/3] feat: add --reset to reset a start-only Connect to a clean state Returns a running start-only Connect to its clean, just-bootstrapped state with the same container, port, and API key, without restarting the container. Useful for test isolation, and for anyone who wants a clean Connect without a full restart. --- .github/workflows/ci.yml | 71 ++++++- README.md | 44 ++++- action.yml | 13 ++ main.py | 385 +++++++++++++++++++++++++++++++++---- pyproject.toml | 4 +- test_integration.py | 94 +++++++++ test_main.py | 398 ++++++++++++++++++++++++++++++++++----- 7 files changed, 922 insertions(+), 87 deletions(-) create mode 100644 test_integration.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index eb09100..a0c6f6d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,6 +24,8 @@ jobs: - name: Run unit tests run: uv run test_main.py + # Command mode on the modern ghcr.io/posit-dev/connect image (default version), + # across x86 and arm runners. test-action: strategy: fail-fast: false @@ -58,6 +60,7 @@ jobs: [ "$TEST_STRING" = "This contains single quotes" ] || exit 1 echo "✓ Multiline test passed - variables, single quotes, and double quotes all work" + # Legacy image (rstudio/rstudio-connect) start-only + graceful stop. test-action-start-only: runs-on: ubuntu-latest steps: @@ -67,7 +70,7 @@ jobs: id: start-connect uses: ./ with: - version: 2024.08.0 + version: 2024.08.0 # legacy rstudio/rstudio-connect image license: ${{ secrets.CONNECT_LICENSE }} - name: Verify outputs are set @@ -112,6 +115,72 @@ jobs: env: CONTAINER_ID: ${{ steps.start-connect.outputs.CONTAINER_ID }} + # Full --reset cycle (dirty -> reset -> pristine + same key) on BOTH the modern + # ghcr.io/posit-dev/connect image and the legacy rstudio/rstudio-connect image. + test-action-reset: + name: test-action-reset (${{ matrix.label }}) + strategy: + fail-fast: false + matrix: + include: + - version: "release" # modern ghcr.io/posit-dev/connect image + label: modern + - version: "2024.08.0" # legacy rstudio/rstudio-connect image + label: legacy + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + + - name: Start Connect (start-only) + id: start + uses: ./ + with: + version: ${{ matrix.version }} + license: ${{ secrets.CONNECT_LICENSE }} + + - name: Deploy content to dirty the instance + run: | + set -euo pipefail + curl -f -s -X POST "$CONNECT_SERVER/__api__/v1/content" \ + -H "Authorization: Key $CONNECT_API_KEY" \ + -H 'Content-Type: application/json' \ + -d '{"name":"ci-reset-test","title":"CI Reset Test"}' > /dev/null + COUNT=$(curl -f -s -H "Authorization: Key $CONNECT_API_KEY" "$CONNECT_SERVER/__api__/v1/content" \ + | python3 -c 'import sys, json; print(len(json.load(sys.stdin)))') + echo "Content count after deploy: $COUNT" + [ "$COUNT" = "1" ] || { echo "ERROR: expected 1 content item"; exit 1; } + echo "✓ Instance dirtied with one content item" + env: + CONNECT_API_KEY: ${{ steps.start.outputs.CONNECT_API_KEY }} + CONNECT_SERVER: ${{ steps.start.outputs.CONNECT_SERVER }} + + - name: Reset Connect + uses: ./ + with: + reset: ${{ steps.start.outputs.CONTAINER_ID }} + + - name: Verify pristine state with the same API key + run: | + set -euo pipefail + RESPONSE=$(curl -f -H "Authorization: Key $CONNECT_API_KEY" "$CONNECT_SERVER/__api__/v1/content") + echo "Content after reset: $RESPONSE" + [ "$RESPONSE" = "[]" ] || { echo "ERROR: content should be wiped after reset"; exit 1; } + if ! docker ps -q --filter "id=$CONTAINER_ID" | grep -q .; then + echo "ERROR: container should still be running after reset" + exit 1 + fi + echo "✓ Reset produced a clean Connect, kept the same API key, and left the container running" + env: + CONNECT_API_KEY: ${{ steps.start.outputs.CONNECT_API_KEY }} + CONNECT_SERVER: ${{ steps.start.outputs.CONNECT_SERVER }} + CONTAINER_ID: ${{ steps.start.outputs.CONTAINER_ID }} + + - name: Stop Connect + uses: ./ + with: + stop: ${{ steps.start.outputs.CONTAINER_ID }} + + # CLI command mode on the legacy rstudio/rstudio-connect image. test-cli: runs-on: ubuntu-latest steps: diff --git a/README.md b/README.md index 9243311..2dc622f 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,7 @@ Without `bash -c`, the environment variables would be evaluated before `with-con | `--port` | `3939` | Port to map the Connect container to. Allows running multiple Connect instances simultaneously. | | `-e`, `--env` | | Environment variables to pass to the Docker container (format: KEY=VALUE). Can be specified multiple times. | | `--stop` | | Stop a running Connect container by ID, or use `CONTAINER_ID` env var if not specified. | +| `--reset` | | Reset a running start-only container to its clean baseline (same container, port, and API key), or use `CONTAINER_ID` env var if not specified. | Example: @@ -105,12 +106,50 @@ You can eval the output to set the variables in your shell: eval $(with-connect --license ./rstudio-connect.lic) curl -H "Authorization: Key $CONNECT_API_KEY" $CONNECT_SERVER/__api__/v1/content -# Stop Connect when done (--stop without argument uses $CONTAINER_ID) -with-connect --stop +# Stop Connect when done +with-connect --stop "$CONTAINER_ID" ``` +`eval` sets these as ordinary shell variables, so pass `"$CONTAINER_ID"` explicitly. The no-argument forms of `--stop`/`--reset` instead read a `CONTAINER_ID` environment variable, which is how the GitHub Action wires them up. + This is useful when you need to run multiple commands or use other tools against the running Connect instance. +### Resetting Connect + +Every start-only container can be reset. `with-connect --reset` returns Connect to its clean, just-bootstrapped state — no deployed content, no extra users — **without stopping the container**. The same `CONNECT_API_KEY`, `CONNECT_SERVER`, and `CONTAINER_ID` stay valid, so a test framework can reset between runs and keep the credentials it already holds: + +```bash +eval $(with-connect --license ./rstudio-connect.lic) + +# ... run a test that deploys content ... + +# Reset to a clean Connect between tests +with-connect --reset "$CONTAINER_ID" +# The same $CONNECT_API_KEY and $CONNECT_SERVER still work; Connect is now clean. + +# ... run the next test ... + +with-connect --stop "$CONTAINER_ID" +``` + +Reset is fast (usually a few seconds) because it never restarts the container, re-pulls the image, or re-bootstraps. Under the hood, start-only containers run Connect under a keep-alive process so Connect can be cycled in place; the reset restores a snapshot of the data directory captured right after bootstrap. Reset only applies to start-only containers (command mode containers are ephemeral). + +`--reset` supports only Connect's default SQLite data directory (`/var/lib/rstudio-connect`). If you override `Server.DataDir` (via `--config` or `CONNECT_SERVER_DATADIR`) or point Connect at an external database, `--reset` refuses to run and reports an error rather than silently leaving that state in place. (Start-only mode itself works fine with a custom data directory; only reset is unsupported there.) + +#### Detecting a crashed Connect + +Because a start-only container stays running under the keep-alive process, a crashed Connect does **not** stop the container. To make crashes visible, start-only containers are given a healthcheck that probes Connect's `/__ping__` endpoint. Check it during a test run: + +```bash +docker inspect --format '{{.State.Health.Status}}' "$CONTAINER_ID" +# healthy -> Connect is serving +# unhealthy -> Connect stopped responding (e.g. crashed) and did not recover +``` + +Nothing auto-restarts Connect, so a crash stays `unhealthy` until the next `--reset`. Assert on `healthy` (rather than container liveness) if a test needs to confirm Connect stayed up. + +> Note: the health status is refreshed on the container's healthcheck interval, so immediately after a `--reset` it may briefly lag (reset polls `/__ping__` directly and returns as soon as Connect is serving). For an immediate readiness signal use `/__ping__`; for "did Connect stay up over time" use the health status. + ## GitHub Actions This project contains a GitHub Action for use in CI/CD workflows. Use the `@main` tag to reference the action. @@ -132,6 +171,7 @@ The GitHub Action supports the following inputs: | `env` | No | | Environment variables to pass to Docker container (one per line, format: KEY=VALUE) | | `command` | No | | Command to run against Connect (omit for start-only mode) | | `stop` | No | | Container ID to stop (use instead of starting a new container) | +| `reset` | No | | Container ID of a start-only container to reset to its clean baseline (use instead of starting a new container) | ### GitHub Action Outputs diff --git a/action.yml b/action.yml index d23f0e1..9dc5e76 100644 --- a/action.yml +++ b/action.yml @@ -31,6 +31,9 @@ inputs: stop: description: 'Container ID to stop (use instead of starting a new container)' required: false + reset: + description: 'Container ID of a start-only container to reset to its clean baseline (use instead of starting a new container)' + required: false outputs: CONNECT_API_KEY: @@ -53,7 +56,11 @@ runs: shell: bash run: uv tool install ${{ github.action_path }} + # Only write the license when starting a new container. On stop/reset there is + # no license input, and the running container bind-mounts this file as its + # license -- rewriting it here would blank that out and break --reset. - name: Create license file + if: ${{ inputs.stop == '' && inputs.reset == '' }} shell: bash run: echo "${{ inputs.license }}" > rstudio-connect.lic @@ -67,6 +74,12 @@ runs: exit 0 fi + # Handle reset mode + if [ -n "${{ inputs.reset }}" ]; then + with-connect --reset "${{ inputs.reset }}" + exit 0 + fi + # Build arguments ARGS="--version ${{ inputs.version }} --port ${{ inputs.port }}" if [ -n "${{ inputs.config-file }}" ]; then diff --git a/main.py b/main.py index 8be4ad5..fa04424 100644 --- a/main.py +++ b/main.py @@ -2,10 +2,12 @@ import base64 import os import re +import shlex import socket import subprocess import sys import time +import urllib.request import docker from rsconnect.api import RSConnectClient, RSConnectServer @@ -22,6 +24,40 @@ REGISTRY_CUTOVER_YEAR = 2026 REGISTRY_CUTOVER_MONTH = 4 +# Connect's mutable state directory inside the container (SQLite DB, deployed +# content, etc.). The license is bind-mounted read-only into it and must be +# preserved across reset. The clean baseline snapshot is stored outside it so +# reset's wipe never touches it. +DATA_DIR = "/var/lib/rstudio-connect" +LICENSE_FILENAME = "rstudio-connect.lic" +BASELINE_DIR = "/var/lib/with-connect" +BASELINE_PATH = f"{BASELINE_DIR}/baseline.tgz" + +# In start-only mode, PID 1 is docker-init (added via init=True in main()), which +# reaps orphaned Connect child processes and forwards signals. It runs a keep-alive +# `sleep infinity` as its child so Connect can be cycled (for --reset) without +# stopping the container; Connect itself is launched via `docker exec` using the +# image's own launch command (see get_connect_launch_command). +KEEPALIVE_CMD = ["sleep", "infinity"] +CONNECT_BINARY = "/opt/rstudio-connect/bin/connect" + +# Health probe so a crashed Connect is visible via the container's health status +# even though PID 1 (the keep-alive) keeps the container running. It mirrors the +# probe the modern image already ships (curl /__ping__ on an interval); we set it +# ourselves so legacy and other images that lack a healthcheck get it too. Since +# nothing auto-restarts Connect, a crash stays "unhealthy" until the next reset. +# Durations are nanoseconds (docker SDK). +HEALTHCHECK = { + "test": [ + "CMD-SHELL", + "curl --fail --silent --output /dev/null http://localhost:3939/__ping__", + ], + "interval": 10_000_000_000, # probe every 10s + "timeout": 5_000_000_000, # 5s per probe + "retries": 3, # 3 consecutive failures -> unhealthy + "start_period": 60_000_000_000, # grace while Connect first boots +} + def parse_args(): """ @@ -76,6 +112,14 @@ def parse_args(): metavar="CONTAINER_ID", help="Stop a running Connect container by ID (uses CONTAINER_ID env var if not specified)", ) + parser.add_argument( + "--reset", + nargs="?", + default=None, + const="", # sentinel for --reset without argument + metavar="CONTAINER_ID", + help="Reset a running start-only Connect container to its clean baseline (uses CONTAINER_ID env var if not specified)", + ) # Handle -- separator and capture remaining args if "--" in sys.argv: @@ -241,18 +285,56 @@ def get_docker_tag(version: str) -> tuple[str, str]: return (LEGACY_IMAGE, version) +def build_run_kwargs(image_name, port, mounts, container_env, base_image, is_start_only): + """Assemble the docker containers.run kwargs. + + Start-only mode adds a keep-alive PID 1 command, a crash-surfacing healthcheck, + and init=True; command mode uses the image's default entrypoint. See main(). + """ + run_kwargs = { + "image": image_name, + "detach": True, + "tty": True, + "stdin_open": True, + "privileged": True, + "ports": {"3939/tcp": port}, + "mounts": mounts, + "environment": container_env, + } + if _force_amd64(base_image): + run_kwargs["platform"] = "linux/amd64" + if is_start_only: + # PID 1 is a keep-alive so Connect can be cycled (for --reset) without + # stopping the container; Connect itself is launched via exec (see + # start_connect). init=True injects docker-init (tini) as PID 1, which + # reaps the Connect child processes orphaned on each stop/reset (a bare + # sleep PID 1 never reaps) and makes sleep a child that responds to + # SIGTERM for a prompt stop. The healthcheck surfaces a crashed Connect + # via the container's health status. + run_kwargs["command"] = KEEPALIVE_CMD + run_kwargs["healthcheck"] = HEALTHCHECK + run_kwargs["init"] = True + return run_kwargs + + def main() -> int: """ Main entry point for the with-connect CLI tool. - Orchestrates the full workflow: + --stop and --reset short-circuit to stop or reset an existing container and + return. Otherwise the workflow is: 1. Parse arguments and validate file paths - 2. Ensure Docker image is available - 3. Start Connect container with license and optional config - 4. Wait for Connect to start and validate license - 5. Bootstrap and retrieve API key - 6. Execute user command with CONNECT_API_KEY and CONNECT_SERVER set - 7. Stop container and exit with command's exit code + 2. Ensure the Docker image is available + 3. Start the Connect container with the license and optional config + (start-only mode runs a keep-alive PID 1 so Connect can be reset in place; + command mode uses the image's default entrypoint) + 4. Wait for Connect to start and validate the license + 5. Bootstrap and retrieve an API key + 6. Command mode: run the user command with CONNECT_API_KEY and CONNECT_SERVER + set, then stop the container and exit with the command's code + 7. Start-only mode: capture a clean baseline snapshot for --reset, print the + credentials (API key, server URL, container id), and leave the container + running """ args = parse_args() @@ -261,14 +343,16 @@ def main() -> int: container_id = args.stop or os.environ.get("CONTAINER_ID") if not container_id: raise RuntimeError("No container ID provided and CONTAINER_ID environment variable not set") - client = docker.from_env() - try: - container = client.containers.get(container_id) - container.stop() - print(f"Stopped container {container_id}", file=sys.stderr) - return 0 - except docker.errors.NotFound: - raise RuntimeError(f"Container not found: {container_id}") + stop_container(container_id) + return 0 + + # Handle --reset mode: reset a start-only container to its clean baseline + if args.reset is not None: + container_id = args.reset or os.environ.get("CONTAINER_ID") + if not container_id: + raise RuntimeError("No container ID provided and CONTAINER_ID environment variable not set") + reset_container(container_id) + return 0 license_path = os.path.abspath(os.path.expanduser(args.license)) if not os.path.exists(license_path): @@ -327,24 +411,25 @@ def main() -> int: key, value = env_var.split("=", 1) container_env[key] = value - run_kwargs = { - "image": image_name, - "detach": True, - "tty": True, - "stdin_open": True, - "privileged": True, - "ports": {"3939/tcp": args.port}, - "mounts": mounts, - "environment": container_env, - } - if _force_amd64(base_image): - run_kwargs["platform"] = "linux/amd64" + # Start-only mode (no command after --) launches Connect under a keep-alive + # PID 1 so it can be reset in place; command mode keeps the default entrypoint. + is_start_only = not args.command + + run_kwargs = build_run_kwargs( + image_name, args.port, mounts, container_env, base_image, is_start_only + ) container = client.containers.run(**run_kwargs) server_url = f"http://localhost:{args.port}" - stop_container = True + stop_on_exit = True try: + # Inside the try so a start_connect failure (e.g. a custom image with no + # launch command) still hits the finally and stops the just-created + # container instead of leaking it and holding the port. + if is_start_only: + start_connect(container) + print(f"Waiting for port {args.port} to open...", file=sys.stderr) if not is_port_open("localhost", args.port, timeout=60.0): print("\nContainer logs:", file=sys.stderr) @@ -375,15 +460,22 @@ def main() -> int: except subprocess.CalledProcessError as e: exit_code = e.returncode else: - # Start-only mode: output credentials and keep container running + # Start-only mode: capture a clean baseline for --reset, then output + # credentials and keep the container running. + print("Capturing clean baseline snapshot for --reset...", file=sys.stderr) + capture_baseline(container) + if not wait_until_healthy("localhost", args.port): + print("\nContainer logs:", file=sys.stderr) + print(container.logs().decode("utf-8", errors="replace"), file=sys.stderr) + raise RuntimeError("Connect did not become healthy after baseline capture") print(f"CONNECT_API_KEY={api_key}") print(f"CONNECT_SERVER={server_url}") print(f"CONTAINER_ID={container.id}") - stop_container = False + stop_on_exit = False return exit_code finally: - if stop_container: + if stop_on_exit: container.stop() @@ -494,9 +586,238 @@ def get_api_key(bootstrap_secret: str, container, server_url: str) -> str: raise RuntimeError(f"Failed to bootstrap Connect and retrieve API key: {e}") -if __name__ == "__main__": +def get_connect_launch_command(container) -> list[str]: + """Return the command the image uses to launch Connect (Entrypoint + Cmd). + + Derived from the image config so Connect is launched the same way the image + would on its own -- covering the modern (startup.sh) and legacy + (tini -- startup.sh) Connect images. + """ + config = container.image.attrs.get("Config", {}) or {} + entrypoint = config.get("Entrypoint") or [] + cmd = config.get("Cmd") or [] + launch = list(entrypoint) + list(cmd) + if not launch: + raise RuntimeError("Could not determine Connect launch command from image config") + return launch + + +def start_connect(container) -> None: + """Launch Connect as a managed background child using the image's own launch + command, routing output to PID 1's stdout so container.logs() still captures + Connect's log stream.""" + launch = get_connect_launch_command(container) + joined = " ".join(shlex.quote(part) for part in launch) + container.exec_run( + ["bash", "-lc", f"{joined} > /proc/1/fd/1 2>&1"], + detach=True, + ) + + +def get_connect_pid(container): + """Return the PID of the running Connect process, or None if not running. + + Raises if ps itself fails, so a broken/absent ps (e.g. a custom image) is not + mistaken for 'Connect not running' (which would make stop_connect silently + no-op and let reset tar a live database). + """ + code, output = container.exec_run(["ps", "-eo", "pid,args"]) + text = output.decode("utf-8", errors="replace") if output else "" + if code != 0: + raise RuntimeError(f"Could not list processes to find Connect (ps exited {code}): {text}") + for line in text.splitlines(): + if CONNECT_BINARY in line: + parts = line.split(None, 1) + if parts and parts[0].isdigit(): + return int(parts[0]) + return None + + +def stop_connect(container, timeout: float = 30.0, poll_interval: float = 0.5) -> None: + """Stop the Connect process inside the container: SIGTERM, wait, then SIGKILL.""" + pid = get_connect_pid(container) + if pid is None or pid == 1: + # Never signal PID 1: in start-only mode it is the keep-alive that holds + # the container open. Killing it would stop the container and defeat the + # whole point of resetting in place. + return + container.exec_run(["kill", "-TERM", str(pid)]) + deadline = time.time() + timeout + while time.time() < deadline: + code, _ = container.exec_run(["kill", "-0", str(pid)]) + if code != 0: + return + time.sleep(poll_interval) + container.exec_run(["kill", "-KILL", str(pid)]) + + +def has_baseline(container) -> bool: + """Whether the container has a captured clean baseline (i.e. was started by + with-connect in start-only mode).""" + code, _ = container.exec_run(["test", "-f", BASELINE_PATH]) + return code == 0 + + +def connect_data_is_default(container) -> bool: + """Whether Connect's SQLite database lives under the default data dir. + + Reset snapshots/restores DATA_DIR, so if Connect was configured with a custom + Server.DataDir (or an external database) its data isn't there and a reset + would silently do nothing. A SQLite DB under DATA_DIR/db confirms reset can + actually work. + """ + code, _ = container.exec_run(["sh", "-lc", f"ls {DATA_DIR}/db/*.db"]) + return code == 0 + + +def discover_host_port(container) -> int: + """Return the host port mapped to Connect's container port 3939/tcp.""" + container.reload() + ports = container.attrs.get("NetworkSettings", {}).get("Ports", {}) or {} + binding = ports.get("3939/tcp") + if not binding: + raise RuntimeError("Container has no 3939/tcp port mapping") + return int(binding[0]["HostPort"]) + + +def wait_until_healthy( + host: str, port: int, timeout: float = 60.0, poll_interval: float = 1.0 +) -> bool: + """Poll Connect's unauthenticated /__ping__ endpoint until it returns 200.""" + url = f"http://{host}:{port}/__ping__" + deadline = time.time() + timeout + while time.time() < deadline: + try: + with urllib.request.urlopen(url, timeout=5) as resp: + if resp.status == 200: + return True + except Exception: + pass + time.sleep(poll_interval) + return False + + +def exec_or_raise(container, cmd, message: str): + """Run cmd in the container as root; raise RuntimeError with output on failure.""" + code, output = container.exec_run(cmd, user="root") + if code != 0: + detail = output.decode("utf-8", errors="replace") if output else "" + raise RuntimeError(f"{message}: {detail}") + return output + + +def capture_baseline(container) -> None: + """Snapshot the clean data dir (excluding the license mount) for later reset. + + Stops Connect first so the SQLite copy is consistent, then restarts it. The + caller is responsible for waiting until Connect is healthy again. + """ + stop_connect(container) + exec_or_raise(container, ["mkdir", "-p", BASELINE_DIR], "Failed to create baseline dir") + exec_or_raise( + container, + ["tar", "czf", BASELINE_PATH, "-C", DATA_DIR, f"--exclude=./{LICENSE_FILENAME}", "."], + "Failed to capture baseline snapshot", + ) + start_connect(container) + + +def restore_baseline(container) -> None: + """Wipe the data dir (preserving the license mount) and restore the baseline.""" + wipe = ( + f"cd {DATA_DIR} && find . -mindepth 1 " + f"-not -path './{LICENSE_FILENAME}' -delete" + ) + exec_or_raise(container, ["bash", "-lc", wipe], "Failed to wipe data dir") + exec_or_raise( + container, ["tar", "xzf", BASELINE_PATH, "-C", DATA_DIR], + "Failed to restore baseline snapshot", + ) + + +def stop_container(container_id: str) -> None: + """Stop a Connect container by id. + + Start-only containers run under a keep-alive PID 1 that ignores SIGTERM, so a + plain container.stop() would idle the full ~10s grace period. For those, stop + Connect gracefully first, then stop the container promptly. + """ + client = docker.from_env() + try: + container = client.containers.get(container_id) + except docker.errors.NotFound: + raise RuntimeError(f"Container not found: {container_id}") + container.reload() + if container.status == "running" and has_baseline(container): + # Best-effort graceful Connect shutdown (flush its SQLite state before the + # container is torn down). If listing/signalling Connect fails -- e.g. a + # custom image without ps -- still stop the container rather than erroring + # out and leaving it running. + try: + stop_connect(container, timeout=8) + except RuntimeError as e: + print(f"Warning: could not stop Connect gracefully: {e}", file=sys.stderr) + container.stop(timeout=2) + else: + container.stop() + print(f"Stopped container {container_id}", file=sys.stderr) + + +def reset_container(container_id: str) -> None: + """Reset a running start-only Connect container to its clean baseline. + + Stops Connect inside the container, restores the pristine data dir, and + restarts Connect — without stopping the container. The restored DB holds the + same admin API key, so existing credentials keep working. + """ + client = docker.from_env() + try: + container = client.containers.get(container_id) + except docker.errors.NotFound: + raise RuntimeError(f"Container not found: {container_id}") + + container.reload() + if container.status != "running": + raise RuntimeError(f"Container is not running: {container_id}") + + if not has_baseline(container): + raise RuntimeError( + f"Container {container_id} was not started by with-connect in " + "start-only mode, so it has no clean baseline to reset to." + ) + + if not connect_data_is_default(container): + raise RuntimeError( + f"Cannot reset container {container_id}: Connect's data is not under the " + f"default data directory {DATA_DIR} (a custom Server.DataDir or an external " + "database is configured). --reset only supports the default SQLite data " + "directory; resetting would silently leave that state in place." + ) + + port = discover_host_port(container) + print(f"Resetting Connect in container {container_id}...", file=sys.stderr) + stop_connect(container) + restore_baseline(container) + start_connect(container) + + if not wait_until_healthy("localhost", port): + print("\nContainer logs:", file=sys.stderr) + print(container.logs().decode("utf-8", errors="replace"), file=sys.stderr) + raise RuntimeError("Connect did not become healthy after reset") + + print(f"Connect reset and ready at http://localhost:{port}", file=sys.stderr) + + +def cli() -> None: + """Console-script entry point. Wraps main so a RuntimeError prints a clean + 'Error: ...' line with exit code 1 instead of a traceback. The installed + command skips the __main__ block, so it must target cli, not main.""" try: sys.exit(main()) except RuntimeError as e: print(f"Error: {e}", file=sys.stderr) sys.exit(1) + + +if __name__ == "__main__": + cli() diff --git a/pyproject.toml b/pyproject.toml index cb9c868..ddf14a4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -10,7 +10,9 @@ dependencies = [ ] [project.scripts] -with-connect = "main:main" +# Target cli, not main: the installed command skips the __main__ block, so cli +# gives it the same clean error handling as python main.py. +with-connect = "main:cli" [tool.setuptools] py-modules = ["main"] diff --git a/test_integration.py b/test_integration.py new file mode 100644 index 0000000..8757360 --- /dev/null +++ b/test_integration.py @@ -0,0 +1,94 @@ +"""End-to-end test for --reset against a real Connect container. + +Skipped unless CONNECT_LICENSE_FILE points to a valid license file and Docker is +available. CONNECT_LICENSE_FILE is the same env var the Connect repo uses for a +license path, so this "just works" for Connect developers. Run: + + CONNECT_LICENSE_FILE=/path/to/license.lic \ + env -u PYENV_VERSION uv run --with pytest pytest test_integration.py -v +""" +import json +import os +import subprocess +import sys +import urllib.request + +import pytest + +LICENSE = os.environ.get("CONNECT_LICENSE_FILE") +PORT = 3951 + +pytestmark = pytest.mark.skipif( + not (LICENSE and os.path.exists(LICENSE)), + reason="set CONNECT_LICENSE_FILE to a valid license path to run", +) + + +def _api(server, path, key, method="GET", body=None): + req = urllib.request.Request( + server + path, + method=method, + headers={"Authorization": f"Key {key}"}, + ) + if body is not None: + req.data = json.dumps(body).encode() + req.add_header("Content-Type", "application/json") + with urllib.request.urlopen(req, timeout=15) as resp: + raw = resp.read().decode() + return resp.status, (json.loads(raw) if raw else None) + + +def test_reset_end_to_end(): + import docker + + client = docker.from_env() + + start = subprocess.run( + [sys.executable, "main.py", "--license", LICENSE, "--port", str(PORT)], + capture_output=True, + text=True, + timeout=300, + ) + assert start.returncode == 0, start.stderr + kv = dict( + line.split("=", 1) + for line in start.stdout.strip().splitlines() + if "=" in line + ) + key, server, cid = kv["CONNECT_API_KEY"], kv["CONNECT_SERVER"], kv["CONTAINER_ID"] + + try: + # Clean to start. + status, content = _api(server, "/__api__/v1/content", key) + assert status == 200 and content == [] + + # Dirty it. + status, _ = _api( + server, "/__api__/v1/content", key, "POST", + {"name": "itest-content", "title": "itest"}, + ) + assert status == 200 + _, content = _api(server, "/__api__/v1/content", key) + assert len(content) == 1 + + # Reset. + reset = subprocess.run( + [sys.executable, "main.py", "--reset", cid], + capture_output=True, + text=True, + timeout=180, + ) + assert reset.returncode == 0, reset.stderr + + # Same key still authenticates; content is gone; container still running. + status, content = _api(server, "/__api__/v1/content", key) + assert status == 200, "same API key should still work after reset" + assert content == [], "content should be wiped after reset" + client.containers.get(cid).reload() + assert client.containers.get(cid).status == "running" + finally: + subprocess.run( + [sys.executable, "main.py", "--stop", cid], + capture_output=True, + text=True, + ) diff --git a/test_main.py b/test_main.py index 37b2c26..7020b60 100644 --- a/test_main.py +++ b/test_main.py @@ -277,80 +277,376 @@ def test_stop_nonexistent_container(): assert "Container not found" in result.stderr -if __name__ == "__main__": - test_license_file_not_exists() - print("✓ test_license_file_not_exists passed") +def test_reset_argument_in_help(): + """Test that --reset argument is available.""" + result = subprocess.run( + [sys.executable, "main.py", "--help"], + capture_output=True, + text=True, + ) - test_config_file_not_exists() - print("✓ test_config_file_not_exists passed") + assert "--reset" in result.stdout + assert "CONTAINER_ID" in result.stdout - test_license_file_with_tilde_expansion() - print("✓ test_license_file_with_tilde_expansion passed") - test_invalid_license_detection() - print("✓ test_invalid_license_detection passed") +def test_reset_nonexistent_container(): + """Test that --reset with nonexistent container returns error.""" + result = subprocess.run( + [sys.executable, "main.py", "--reset", "nonexistent_container_id"], + capture_output=True, + text=True, + ) - test_valid_license_http_server_starts() - print("✓ test_valid_license_http_server_starts passed") + assert result.returncode == 1 + assert "Container not found" in result.stderr - test_image_and_version_exclusive() - print("✓ test_image_and_version_exclusive passed") - test_image_without_tag() - print("✓ test_image_without_tag passed") +def test_get_connect_pid_found(): + """Returns the Connect worker PID, not the PID 1 keep-alive that shares the + process table with it. stop_connect kills whatever this returns, so picking + the wrong line (e.g. PID 1) would stop the container.""" + c = MagicMock() + c.exec_run.return_value = ( + 0, + b" PID COMMAND\n 1 sleep infinity\n 20 /opt/rstudio-connect/bin/connect --config /etc/rstudio-connect/rstudio-connect.gcfg\n", + ) + assert main.get_connect_pid(c) == 20 - test_image_with_tag() - print("✓ test_image_with_tag passed") - test_get_docker_tag_latest() - print("✓ test_get_docker_tag_latest passed") +def test_get_connect_pid_not_found(): + """When only the keep-alive (sleep infinity, PID 1) is running and Connect is + not, returns None so stop_connect safely no-ops instead of targeting PID 1.""" + c = MagicMock() + c.exec_run.return_value = (0, b" PID COMMAND\n 1 sleep infinity\n") + assert main.get_connect_pid(c) is None - test_get_docker_tag_release() - print("✓ test_get_docker_tag_release passed") - test_get_docker_tag_preview() - print("✓ test_get_docker_tag_preview passed") +def test_get_connect_pid_raises_on_ps_failure(): + c = MagicMock() + c.exec_run.return_value = (127, b"sh: ps: not found") + try: + main.get_connect_pid(c) + assert False, "expected RuntimeError" + except RuntimeError as e: + assert "ps exited" in str(e).lower() or "could not list processes" in str(e).lower() + + +def test_stop_connect_graceful(): + c = MagicMock() + c.exec_run.side_effect = [ + (0, b" 20 /opt/rstudio-connect/bin/connect --config x"), # ps + (0, b""), # kill -TERM + (1, b""), # kill -0 -> gone + ] + main.stop_connect(c, timeout=5, poll_interval=0.01) + cmds = [call.args[0] for call in c.exec_run.call_args_list] + assert ["kill", "-TERM", "20"] in cmds + assert ["kill", "-KILL", "20"] not in cmds + + +def test_stop_connect_force_kill_on_timeout(): + c = MagicMock() + seq = [ + (0, b" 20 /opt/rstudio-connect/bin/connect --config x"), # ps + (0, b""), # kill -TERM + ] + + def fake_exec(cmd, **kwargs): + if seq: + return seq.pop(0) + return (0, b"") # kill -0 always alive; then kill -KILL + + c.exec_run.side_effect = fake_exec + main.stop_connect(c, timeout=0.05, poll_interval=0.01) + cmds = [call.args[0] for call in c.exec_run.call_args_list] + assert ["kill", "-KILL", "20"] in cmds + + +def test_stop_connect_never_kills_pid_1(): + """Safety: stop_connect must never signal PID 1 (the keep-alive), even if the + process table reports Connect as PID 1 -- killing it stops the container and + defeats reset-in-place.""" + c = MagicMock() + c.exec_run.return_value = ( + 0, + b" PID COMMAND\n 1 /opt/rstudio-connect/bin/connect --config x\n", + ) + main.stop_connect(c, timeout=0.05, poll_interval=0.01) + cmds = [call.args[0] for call in c.exec_run.call_args_list] + assert not any(cmd[:2] == ["kill", "-TERM"] for cmd in cmds), "must not SIGTERM PID 1" + assert not any(cmd[:2] == ["kill", "-KILL"] for cmd in cmds), "must not SIGKILL PID 1" - test_get_docker_tag_jammy_version() - print("✓ test_get_docker_tag_jammy_version passed") - test_get_docker_tag_bionic_version() - print("✓ test_get_docker_tag_bionic_version passed") +def test_discover_host_port(): + c = MagicMock() + c.attrs = { + "NetworkSettings": {"Ports": {"3939/tcp": [{"HostIp": "0.0.0.0", "HostPort": "3941"}]}} + } + assert main.discover_host_port(c) == 3941 + c.reload.assert_called_once() - test_get_docker_tag_old_version() - print("✓ test_get_docker_tag_old_version passed") - test_get_docker_tag_invalid_format() - print("✓ test_get_docker_tag_invalid_format passed") +def test_discover_host_port_missing(): + c = MagicMock() + c.attrs = {"NetworkSettings": {"Ports": {}}} + try: + main.discover_host_port(c) + assert False, "expected RuntimeError" + except RuntimeError as e: + assert "3939/tcp" in str(e) + + +def test_restore_baseline_wipes_and_extracts(): + c = MagicMock() + c.exec_run.return_value = (0, b"") + main.restore_baseline(c) + cmds = [call.args[0] for call in c.exec_run.call_args_list] + # First: bash wipe that deletes everything except the license bind-mount + assert cmds[0][0] == "bash" + assert f"-not -path './{main.LICENSE_FILENAME}'" in cmds[0][2] + assert "-delete" in cmds[0][2] + # Second: extract the baseline archive into the data dir + assert cmds[1][:3] == ["tar", "xzf", main.BASELINE_PATH] + assert cmds[1][-1] == main.DATA_DIR + + +def test_restore_baseline_raises_on_tar_failure(): + c = MagicMock() + c.exec_run.side_effect = [(0, b""), (2, b"tar: broken")] + try: + main.restore_baseline(c) + assert False, "expected RuntimeError" + except RuntimeError as e: + assert "restore baseline" in str(e).lower() - test_extract_server_version() - print("✓ test_extract_server_version passed") - test_extract_server_version_multiple_lines() - print("✓ test_extract_server_version_multiple_lines passed") +def test_restore_baseline_raises_on_wipe_failure(): + c = MagicMock() + c.exec_run.side_effect = [(1, b"find: permission denied")] # wipe fails first + try: + main.restore_baseline(c) + assert False, "expected RuntimeError" + except RuntimeError as e: + assert "wipe data dir" in str(e).lower() - test_extract_server_version_not_found() - print("✓ test_extract_server_version_not_found passed") - test_extract_server_version_dev() - print("✓ test_extract_server_version_dev passed") +def test_get_connect_launch_command_cmd_only(): + c = MagicMock() + c.image.attrs = {"Config": {"Entrypoint": None, "Cmd": ["/usr/local/bin/startup.sh"]}} + assert main.get_connect_launch_command(c) == ["/usr/local/bin/startup.sh"] - test_local_image_usage() - print("✓ test_local_image_usage passed") - test_release_always_pulls() - print("✓ test_release_always_pulls passed") +def test_get_connect_launch_command_entrypoint_and_cmd(): + c = MagicMock() + c.image.attrs = {"Config": {"Entrypoint": ["/tini", "--"], "Cmd": ["connect", "serve"]}} + assert main.get_connect_launch_command(c) == ["/tini", "--", "connect", "serve"] - test_preview_always_pulls() - print("✓ test_preview_always_pulls passed") - test_custom_port() - print("✓ test_custom_port passed") +def test_get_connect_launch_command_empty_raises(): + c = MagicMock() + c.image.attrs = {"Config": {"Entrypoint": None, "Cmd": None}} + try: + main.get_connect_launch_command(c) + assert False, "expected RuntimeError" + except RuntimeError as e: + assert "launch command" in str(e).lower() + + +def test_start_connect_uses_image_command_and_routes_logs(): + """start_connect runs the image's own launch command (Entrypoint + Cmd) as a + detached child, routing output to PID 1 so container.logs() keeps working.""" + c = MagicMock() + c.image.attrs = {"Config": {"Entrypoint": ["tini", "--"], "Cmd": ["/usr/local/bin/startup.sh"]}} + main.start_connect(c) + cmd = c.exec_run.call_args.args[0] + assert cmd[0] == "bash" + assert "tini -- /usr/local/bin/startup.sh" in cmd[2] + assert "/proc/1/fd/1" in cmd[2] + assert c.exec_run.call_args.kwargs.get("detach") is True + + +def test_capture_baseline_snapshots_excluding_license(): + c = MagicMock() + c.exec_run.return_value = (0, b"") + c.image.attrs = {"Config": {"Entrypoint": None, "Cmd": ["/usr/local/bin/startup.sh"]}} + main.capture_baseline(c) + cmds = [call.args[0] for call in c.exec_run.call_args_list] + tar_cmds = [cmd for cmd in cmds if cmd and cmd[0] == "tar" and "czf" in cmd] + assert tar_cmds, "expected a tar czf command" + tar = tar_cmds[0] + assert main.BASELINE_PATH in tar + # The license bind-mount must be excluded from the snapshot, or tar would + # read the license through the mount into the archive. + assert f"--exclude=./{main.LICENSE_FILENAME}" in tar + + +def test_capture_baseline_raises_on_tar_failure(): + c = MagicMock() + c.image.attrs = {"Config": {"Entrypoint": None, "Cmd": ["/usr/local/bin/startup.sh"]}} + # stop_connect ps (no connect), mkdir, then tar czf fails + c.exec_run.side_effect = [(0, b""), (0, b""), (2, b"tar: broken")] + try: + main.capture_baseline(c) + assert False, "expected RuntimeError" + except RuntimeError as e: + assert "capture baseline" in str(e).lower() + + +def test_stop_container_graceful_for_start_only(): + """A start-only container (has a baseline) is stopped gracefully: Connect + first, then a short container-stop timeout instead of the full ~10s grace.""" + from unittest.mock import patch + + c = MagicMock() + c.status = "running" + c.exec_run.return_value = (0, b"") # has_baseline True; ps finds no connect + client = MagicMock() + client.containers.get.return_value = c + with patch.object(main.docker, "from_env", return_value=client): + main.stop_container("abc123") + c.stop.assert_called_once_with(timeout=2) + + +def test_stop_container_stops_even_if_connect_stop_fails(): + """The graceful Connect stop is best-effort. If it fails (e.g. a custom image + without ps, so get_connect_pid raises), --stop must still stop the container + instead of erroring out and leaving it running.""" + from unittest.mock import patch + + c = MagicMock() + c.status = "running" + # has_baseline -> (0) True; then stop_connect's ps -> non-zero (raises) + c.exec_run.side_effect = [(0, b""), (127, b"sh: ps: not found")] + client = MagicMock() + client.containers.get.return_value = c + with patch.object(main.docker, "from_env", return_value=client): + main.stop_container("abc123") + c.stop.assert_called_once_with(timeout=2) + + +def test_stop_container_plain_for_unmanaged(): + """A container without a baseline uses a plain container.stop().""" + from unittest.mock import patch + + c = MagicMock() + c.status = "running" + c.exec_run.return_value = (1, b"") # has_baseline False + client = MagicMock() + client.containers.get.return_value = c + with patch.object(main.docker, "from_env", return_value=client): + main.stop_container("abc123") + c.stop.assert_called_once_with() + + +def test_reset_container_no_baseline_raises(): + from unittest.mock import patch + + mock_container = MagicMock() + mock_container.status = "running" + mock_container.exec_run.return_value = (1, b"") # has_baseline -> False + mock_client = MagicMock() + mock_client.containers.get.return_value = mock_container + with patch.object(main.docker, "from_env", return_value=mock_client): + try: + main.reset_container("abc123") + assert False, "expected RuntimeError" + except RuntimeError as e: + assert "start-only mode" in str(e) + + +def test_reset_container_custom_datadir_raises(): + """Reset must fail loudly (not silently no-op) when Connect's data is not under + the default data dir, e.g. a custom Server.DataDir.""" + from unittest.mock import patch + + mock_container = MagicMock() + mock_container.status = "running" + + def fake_exec(cmd, **kwargs): + joined = " ".join(cmd) if isinstance(cmd, list) else cmd + if "baseline.tgz" in joined: # has_baseline -> True + return (0, b"") + if "/db/" in joined: # connect_data_is_default -> False (no SQLite db) + return (2, b"ls: no such file or directory") + return (0, b"") + + mock_container.exec_run.side_effect = fake_exec + mock_client = MagicMock() + mock_client.containers.get.return_value = mock_container + with patch.object(main.docker, "from_env", return_value=mock_client): + try: + main.reset_container("abc123") + assert False, "expected RuntimeError" + except RuntimeError as e: + assert "data directory" in str(e).lower() + # It must bail before mutating: the data-dir wipe (restore_baseline) never ran. + cmds = [ + " ".join(c.args[0]) if isinstance(c.args[0], list) else str(c.args[0]) + for c in mock_container.exec_run.call_args_list + ] + assert not any("-delete" in c for c in cmds), "must not wipe the data dir when the guard fires" + + +def test_reset_container_not_running_raises(): + from unittest.mock import patch - test_stop_argument_in_help() - print("✓ test_stop_argument_in_help passed") + mock_container = MagicMock() + mock_container.status = "exited" + mock_client = MagicMock() + mock_client.containers.get.return_value = mock_container + with patch.object(main.docker, "from_env", return_value=mock_client): + try: + main.reset_container("abc123") + assert False, "expected RuntimeError" + except RuntimeError as e: + assert "not running" in str(e) + + +def test_start_only_run_kwargs_have_healthcheck_and_init(): + """Start-only containers must get the keep-alive command, the crash-surfacing + healthcheck, and init=True so docker-init reaps orphaned Connect processes.""" + kwargs = main.build_run_kwargs( + "img:tag", 3939, [], {}, "ghcr.io/posit-dev/connect", is_start_only=True + ) + assert kwargs["command"] == main.KEEPALIVE_CMD + assert kwargs["healthcheck"] == main.HEALTHCHECK + assert kwargs["init"] is True + + +def test_command_mode_run_kwargs_omit_healthcheck_and_init(): + """Command mode uses the image's default entrypoint: no keep-alive command, no + injected healthcheck, no init override.""" + kwargs = main.build_run_kwargs( + "img:tag", 3939, [], {}, "ghcr.io/posit-dev/connect", is_start_only=False + ) + assert "command" not in kwargs + assert "healthcheck" not in kwargs + assert "init" not in kwargs - test_stop_nonexistent_container() - print("✓ test_stop_nonexistent_container passed") +def test_cli_reports_runtime_error_without_traceback(): + result = subprocess.run( + [sys.executable, "-c", + "import sys; sys.argv=['with-connect','--reset','nonexistent_cli_test']; " + "import main; main.cli()"], + capture_output=True, text=True, + ) + assert result.returncode == 1 + assert "Error: Container not found" in result.stderr + assert "Traceback" not in result.stderr + + +if __name__ == "__main__": + _failures = 0 + for _name, _fn in list(globals().items()): + if _name.startswith("test_") and callable(_fn): + try: + _fn() + print(f"✓ {_name} passed") + except Exception as _e: # noqa: BLE001 + _failures += 1 + print(f"✗ {_name} FAILED: {_e!r}") + if _failures: + print(f"\n{_failures} test(s) failed!") + sys.exit(1) print("\nAll tests passed!") From 2333eb8c83ae96b6127f67e982089f3c6f30f0a5 Mon Sep 17 00:00:00 2001 From: Christopher Graham Date: Mon, 20 Jul 2026 17:48:02 -0400 Subject: [PATCH 2/3] fix: resolve --reset's data directory dynamically and guard against unsafe overlaps --- .github/workflows/ci.yml | 45 +++++- README.md | 2 +- main.py | 129 +++++++++++++--- test_integration.py | 152 ++++++++++++++++++- test_main.py | 314 +++++++++++++++++++++++++++++++++++++-- 5 files changed, 605 insertions(+), 37 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a0c6f6d..4b1158b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -115,8 +115,7 @@ jobs: env: CONTAINER_ID: ${{ steps.start-connect.outputs.CONTAINER_ID }} - # Full --reset cycle (dirty -> reset -> pristine + same key) on BOTH the modern - # ghcr.io/posit-dev/connect image and the legacy rstudio/rstudio-connect image. + # Full --reset cycle (dirty -> reset -> pristine + same key). test-action-reset: name: test-action-reset (${{ matrix.label }}) strategy: @@ -127,6 +126,8 @@ jobs: label: modern - version: "2024.08.0" # legacy rstudio/rstudio-connect image label: legacy + - version: "2022.10.0" # README's documented minimum (bionic image) + label: legacy-minimum runs-on: ubuntu-latest steps: - uses: actions/checkout@v6 @@ -180,6 +181,46 @@ jobs: with: stop: ${{ steps.start.outputs.CONTAINER_ID }} + # --reset must refuse (not silently no-op) when Connect points at an external + # database, against a live Postgres-backed Connect rather than a mock. + test-reset-external-db: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + + - name: Set up uv + uses: astral-sh/setup-uv@v7 + with: + enable-cache: true + + - name: Create license file + run: echo "${{ secrets.CONNECT_LICENSE }}" > rstudio-connect.lic + + - name: Run external-database reset test + run: | + CONNECT_LICENSE_FILE=rstudio-connect.lic \ + uv run --with pytest pytest test_integration.py -k test_reset_refuses_external_database -v + + # --reset must work against a custom Server.DataDir path that doesn't already + # exist in the image (Connect logs "Creating", not "Using", the first time). + test-reset-custom-datadir: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + + - name: Set up uv + uses: astral-sh/setup-uv@v7 + with: + enable-cache: true + + - name: Create license file + run: echo "${{ secrets.CONNECT_LICENSE }}" > rstudio-connect.lic + + - name: Run custom-DataDir reset test + run: | + CONNECT_LICENSE_FILE=rstudio-connect.lic \ + uv run --with pytest pytest test_integration.py -k test_reset_works_with_custom_datadir -v + # CLI command mode on the legacy rstudio/rstudio-connect image. test-cli: runs-on: ubuntu-latest diff --git a/README.md b/README.md index 2dc622f..a74f370 100644 --- a/README.md +++ b/README.md @@ -134,7 +134,7 @@ with-connect --stop "$CONTAINER_ID" Reset is fast (usually a few seconds) because it never restarts the container, re-pulls the image, or re-bootstraps. Under the hood, start-only containers run Connect under a keep-alive process so Connect can be cycled in place; the reset restores a snapshot of the data directory captured right after bootstrap. Reset only applies to start-only containers (command mode containers are ephemeral). -`--reset` supports only Connect's default SQLite data directory (`/var/lib/rstudio-connect`). If you override `Server.DataDir` (via `--config` or `CONNECT_SERVER_DATADIR`) or point Connect at an external database, `--reset` refuses to run and reports an error rather than silently leaving that state in place. (Start-only mode itself works fine with a custom data directory; only reset is unsupported there.) +`--reset` resolves Connect's effective data directory at runtime (from its startup log), so it works with the default location (`/var/lib/rstudio-connect`), a custom `Server.DataDir` (via `--config` or `CONNECT_SERVER_DATADIR`), and older images that default elsewhere (e.g. `/data`). It restores Connect's built-in SQLite database, so if Connect is pointed at an external database `--reset` refuses to run and reports an error rather than silently leaving that state in place. #### Detecting a crashed Connect diff --git a/main.py b/main.py index fa04424..faf9bd0 100644 --- a/main.py +++ b/main.py @@ -24,10 +24,13 @@ REGISTRY_CUTOVER_YEAR = 2026 REGISTRY_CUTOVER_MONTH = 4 -# Connect's mutable state directory inside the container (SQLite DB, deployed -# content, etc.). The license is bind-mounted read-only into it and must be -# preserved across reset. The clean baseline snapshot is stored outside it so -# reset's wipe never touches it. +# Connect's default mutable state directory (SQLite DB, deployed content, etc.). +# An image or config can move it (the legacy image defaults to /data), so reset +# resolves the effective directory at runtime via get_data_dir and only falls +# back to this default. The license is bind-mounted read-only into the default +# location and is preserved across reset when it lives inside the data dir. The +# clean baseline snapshot is stored outside any data dir so reset's wipe never +# touches it. DATA_DIR = "/var/lib/rstudio-connect" LICENSE_FILENAME = "rstudio-connect.lic" BASELINE_DIR = "/var/lib/with-connect" @@ -463,7 +466,14 @@ def main() -> int: # Start-only mode: capture a clean baseline for --reset, then output # credentials and keep the container running. print("Capturing clean baseline snapshot for --reset...", file=sys.stderr) - capture_baseline(container) + data_dir = resolve_real_path(container, get_data_dir(container)) + if data_dir_overlaps_baseline(container, data_dir): + raise RuntimeError( + f"Connect's data directory {data_dir} is, or contains, " + f"{BASELINE_DIR}, the internal baseline storage location. " + "Choose a Server.DataDir that doesn't overlap it." + ) + capture_baseline(container, data_dir) if not wait_until_healthy("localhost", args.port): print("\nContainer logs:", file=sys.stderr) print(container.logs().decode("utf-8", errors="replace"), file=sys.stderr) @@ -658,18 +668,49 @@ def has_baseline(container) -> bool: return code == 0 -def connect_data_is_default(container) -> bool: - """Whether Connect's SQLite database lives under the default data dir. +def data_dir_has_sqlite_db(container, data_dir: str) -> bool: + """Whether Connect's built-in SQLite database lives under data_dir. - Reset snapshots/restores DATA_DIR, so if Connect was configured with a custom - Server.DataDir (or an external database) its data isn't there and a reset - would silently do nothing. A SQLite DB under DATA_DIR/db confirms reset can - actually work. + Reset snapshots and restores the data dir as files, so it can only reset the + built-in SQLite database. If Connect points at an external database there is + no SQLite db under data_dir/db, and a file-level restore would leave that + state untouched -- so reset refuses instead of silently no-opping. """ - code, _ = container.exec_run(["sh", "-lc", f"ls {DATA_DIR}/db/*.db"]) + code, _ = container.exec_run(["sh", "-lc", f"ls {shlex.quote(data_dir)}/db/*.db"]) return code == 0 +def data_dir_contains_baseline_dir(data_dir: str, baseline_dir: str = BASELINE_DIR) -> bool: + """Whether wiping data_dir would touch baseline_dir: a pure path comparison. + + restore_baseline's wipe deletes everything under data_dir. If data_dir is + baseline_dir itself or an ancestor of it (e.g. a custom Server.DataDir set + to /var/lib), the wipe would destroy the baseline archive the restore step + is about to extract, along with anything else under that ancestor. + + See data_dir_overlaps_baseline for the container-aware caller, which + checks this against both BASELINE_DIR's literal path and its real target. + """ + baseline = os.path.normpath(baseline_dir) + data = os.path.normpath(data_dir) + return os.path.commonpath([baseline, data]) == data + + +def data_dir_overlaps_baseline(container, data_dir: str) -> bool: + """Whether wiping data_dir (already resolved -- see resolve_real_path) + would touch BASELINE_DIR in either sense that matters. + + Checks BASELINE_DIR's literal path -- deleting just its directory entry, + e.g. if it's itself a symlink pointing elsewhere, still breaks every + subsequent reference built from the literal BASELINE_PATH constant -- and + its resolved real target, in case BASELINE_DIR is a symlink into a + location data_dir would otherwise destroy for real. + """ + return data_dir_contains_baseline_dir(data_dir, BASELINE_DIR) or data_dir_contains_baseline_dir( + data_dir, resolve_real_path(container, BASELINE_DIR) + ) + + def discover_host_port(container) -> int: """Return the host port mapped to Connect's container port 3939/tcp.""" container.reload() @@ -706,7 +747,43 @@ def exec_or_raise(container, cmd, message: str): return output -def capture_baseline(container) -> None: +def get_data_dir(container) -> str: + """Resolve Connect's effective data directory from its startup log. + + Connect logs `Using data directory: ` on boot, reflecting the effective + value whatever its source -- the image's gcfg default, a --config override, or + a CONNECT_SERVER_DATADIR env var. That lets reset work on the modern image + (/var/lib/rstudio-connect) and older images that default elsewhere (/data) + alike, without parsing config. But that directory only logs as "Using" if it + already exists on disk (true for both images' built-in defaults, which are + created at image-build time); a directory that doesn't yet exist -- e.g. a + custom Server.DataDir/CONNECT_SERVER_DATADIR pointed at a fresh path -- logs + "Creating data directory: " on its first boot instead, which the regex + must also match or it silently falls back to the wrong default and captures + an empty baseline. A container accumulates one line per boot (one from + bootstrap, one per reset), so use the most recent; fall back to the default + if no line is present at all. + """ + # "Creating" is logged on the directory's first-ever boot; every boot + # after that (bootstrap restarts, resets) logs "Using" for the same path. + logs = container.logs().decode("utf-8", errors="replace") + matches = re.findall(r"(?:Using|Creating) data directory: ([^\"\n]+)", logs) + return matches[-1].strip() if matches else DATA_DIR + + +def resolve_real_path(container, path: str) -> str: + """Resolve path to its real, symlink-free form inside the container. + + data_dir_contains_baseline_dir compares paths lexically; if Server.DataDir + were a symlink into BASELINE_DIR (or an ancestor of it), that comparison + would miss the overlap even though the shell commands in capture_baseline + and restore_baseline would still follow the symlink and touch it for real. + """ + output = exec_or_raise(container, ["readlink", "-f", path], f"Failed to resolve real path of {path}") + return output.decode("utf-8", errors="replace").strip() + + +def capture_baseline(container, data_dir: str) -> None: """Snapshot the clean data dir (excluding the license mount) for later reset. Stops Connect first so the SQLite copy is consistent, then restarts it. The @@ -716,21 +793,21 @@ def capture_baseline(container) -> None: exec_or_raise(container, ["mkdir", "-p", BASELINE_DIR], "Failed to create baseline dir") exec_or_raise( container, - ["tar", "czf", BASELINE_PATH, "-C", DATA_DIR, f"--exclude=./{LICENSE_FILENAME}", "."], + ["tar", "czf", BASELINE_PATH, "-C", data_dir, f"--exclude=./{LICENSE_FILENAME}", "."], "Failed to capture baseline snapshot", ) start_connect(container) -def restore_baseline(container) -> None: +def restore_baseline(container, data_dir: str) -> None: """Wipe the data dir (preserving the license mount) and restore the baseline.""" wipe = ( - f"cd {DATA_DIR} && find . -mindepth 1 " + f"cd {shlex.quote(data_dir)} && find . -mindepth 1 " f"-not -path './{LICENSE_FILENAME}' -delete" ) exec_or_raise(container, ["bash", "-lc", wipe], "Failed to wipe data dir") exec_or_raise( - container, ["tar", "xzf", BASELINE_PATH, "-C", DATA_DIR], + container, ["tar", "xzf", BASELINE_PATH, "-C", data_dir], "Failed to restore baseline snapshot", ) @@ -786,18 +863,24 @@ def reset_container(container_id: str) -> None: "start-only mode, so it has no clean baseline to reset to." ) - if not connect_data_is_default(container): + data_dir = resolve_real_path(container, get_data_dir(container)) + if not data_dir_has_sqlite_db(container, data_dir): + raise RuntimeError( + f"Cannot reset container {container_id}: no SQLite database under Connect's " + f"data directory {data_dir} (an external database is configured). --reset only " + "restores the built-in SQLite database; resetting would leave that state in place." + ) + if data_dir_overlaps_baseline(container, data_dir): raise RuntimeError( - f"Cannot reset container {container_id}: Connect's data is not under the " - f"default data directory {DATA_DIR} (a custom Server.DataDir or an external " - "database is configured). --reset only supports the default SQLite data " - "directory; resetting would silently leave that state in place." + f"Cannot reset container {container_id}: Connect's data directory {data_dir} " + f"is, or contains, {BASELINE_DIR}, the internal baseline storage location. " + "Wiping data_dir would delete the baseline archive needed to restore it." ) port = discover_host_port(container) print(f"Resetting Connect in container {container_id}...", file=sys.stderr) stop_connect(container) - restore_baseline(container) + restore_baseline(container, data_dir) start_connect(container) if not wait_until_healthy("localhost", port): diff --git a/test_integration.py b/test_integration.py index 8757360..cad99b8 100644 --- a/test_integration.py +++ b/test_integration.py @@ -1,4 +1,4 @@ -"""End-to-end test for --reset against a real Connect container. +"""End-to-end tests for --reset against a real Connect container. Skipped unless CONNECT_LICENSE_FILE points to a valid license file and Docker is available. CONNECT_LICENSE_FILE is the same env var the Connect repo uses for a @@ -11,12 +11,15 @@ import os import subprocess import sys +import time import urllib.request import pytest LICENSE = os.environ.get("CONNECT_LICENSE_FILE") PORT = 3951 +EXTERNAL_DB_PORT = 3952 +CUSTOM_DATADIR_PORT = 3953 pytestmark = pytest.mark.skipif( not (LICENSE and os.path.exists(LICENSE)), @@ -92,3 +95,150 @@ def test_reset_end_to_end(): capture_output=True, text=True, ) + + +def test_reset_refuses_external_database(): + """--reset must refuse (not silently no-op) when Connect points at an + external Postgres database instead of its built-in SQLite database.""" + import docker + + client = docker.from_env() + pg = client.containers.run( + "postgres:17.0", + detach=True, + environment={ + "POSTGRES_USER": "admin", + "POSTGRES_PASSWORD": "password", + "POSTGRES_DB": "connect", + }, + ) + try: + # Run via `exec` (inside the container), not a host-side TCP check: + # Docker Desktop's containers live inside its VM, so a container's + # bridge IP is reachable from OTHER containers but not from the host + # process itself -- a host-side check would never succeed there. + # + # `-h 127.0.0.1 -d connect` (rather than bare `pg_isready -U admin`) + # forces checking the real TCP listener against the actual target + # database: postgres's own init runs a temporary, unix-socket-only + # server (explicitly configured with no TCP listener) to execute init + # scripts before the real server binds 0.0.0.0:5432, so an unqualified + # `pg_isready` risks reporting ready against that temp server; `-d + # connect` also avoids spurious "database admin does not exist" + # errors from pg_isready's default-to-username database. + for _ in range(30): + code, _ = pg.exec_run(["pg_isready", "-h", "127.0.0.1", "-U", "admin", "-d", "connect"]) + if code == 0: + break + time.sleep(1) + else: + pytest.fail("postgres did not become ready in time") + + # IP-based, not hostname-based: works whether the runner is Docker + # Desktop (macOS) or a plain Linux Docker Engine (CI), since both put + # unrelated containers on the same default bridge network by default. + pg.reload() + pg_ip = pg.attrs["NetworkSettings"]["Networks"]["bridge"]["IPAddress"] + pg_url = f"postgres://admin:password@{pg_ip}:5432/connect?sslmode=disable" + + start = subprocess.run( + [ + sys.executable, "main.py", + "--license", LICENSE, + "--port", str(EXTERNAL_DB_PORT), + "--env", "CONNECT_DATABASE_PROVIDER=postgres", + "--env", f"CONNECT_POSTGRES_URL={pg_url}", + ], + capture_output=True, + text=True, + timeout=300, + ) + assert start.returncode == 0, start.stderr + kv = dict( + line.split("=", 1) + for line in start.stdout.strip().splitlines() + if "=" in line + ) + cid = kv["CONTAINER_ID"] + + try: + reset = subprocess.run( + [sys.executable, "main.py", "--reset", cid], + capture_output=True, + text=True, + timeout=60, + ) + assert reset.returncode != 0, "reset should refuse against an external database" + assert "external database" in reset.stderr.lower(), reset.stderr + + client.containers.get(cid).reload() + assert client.containers.get(cid).status == "running", ( + "container must be left running, untouched, after refusing to reset" + ) + finally: + subprocess.run( + [sys.executable, "main.py", "--stop", cid], + capture_output=True, + text=True, + ) + finally: + pg.stop() + pg.remove() + + +def test_reset_works_with_custom_datadir(): + """--reset must work against a custom Server.DataDir path that doesn't + already exist in the image. Connect logs "Creating data directory" (not + "Using") the first time such a path is created, unlike the two built-in + default dirs, which are pre-created at image-build time and always log + "Using" even on a fresh container -- get_data_dir must resolve this + correctly or capture_baseline silently snapshots the wrong, empty + directory, and a later --reset destroys real data instead of restoring it.""" + start = subprocess.run( + [ + sys.executable, "main.py", + "--license", LICENSE, + "--port", str(CUSTOM_DATADIR_PORT), + "--env", "CONNECT_SERVER_DATADIR=/custom-data-dir", + ], + capture_output=True, + text=True, + timeout=300, + ) + assert start.returncode == 0, start.stderr + kv = dict( + line.split("=", 1) + for line in start.stdout.strip().splitlines() + if "=" in line + ) + key, server, cid = kv["CONNECT_API_KEY"], kv["CONNECT_SERVER"], kv["CONTAINER_ID"] + + try: + status, content = _api(server, "/__api__/v1/content", key) + assert status == 200 and content == [] + + status, _ = _api( + server, "/__api__/v1/content", key, "POST", + {"name": "itest-custom-datadir", "title": "itest"}, + ) + assert status == 200 + _, content = _api(server, "/__api__/v1/content", key) + assert len(content) == 1 + + reset = subprocess.run( + [sys.executable, "main.py", "--reset", cid], + capture_output=True, + text=True, + timeout=180, + ) + assert reset.returncode == 0, reset.stderr + + status, content = _api(server, "/__api__/v1/content", key) + assert status == 200, "same API key should still work after reset" + assert content == [], "content should be wiped after reset" + finally: + subprocess.run( + [sys.executable, "main.py", "--stop", cid], + capture_output=True, + text=True, + ) diff --git a/test_main.py b/test_main.py index 7020b60..e5efd26 100644 --- a/test_main.py +++ b/test_main.py @@ -399,10 +399,11 @@ def test_discover_host_port_missing(): def test_restore_baseline_wipes_and_extracts(): c = MagicMock() c.exec_run.return_value = (0, b"") - main.restore_baseline(c) + main.restore_baseline(c, main.DATA_DIR) cmds = [call.args[0] for call in c.exec_run.call_args_list] # First: bash wipe that deletes everything except the license bind-mount assert cmds[0][0] == "bash" + assert main.DATA_DIR in cmds[0][2] assert f"-not -path './{main.LICENSE_FILENAME}'" in cmds[0][2] assert "-delete" in cmds[0][2] # Second: extract the baseline archive into the data dir @@ -410,11 +411,22 @@ def test_restore_baseline_wipes_and_extracts(): assert cmds[1][-1] == main.DATA_DIR +def test_restore_baseline_targets_resolved_data_dir(): + """The wipe and extract operate on the data dir passed in, not a hard-coded + default -- so a legacy image's /data is reset, not the empty default dir.""" + c = MagicMock() + c.exec_run.return_value = (0, b"") + main.restore_baseline(c, "/data") + cmds = [call.args[0] for call in c.exec_run.call_args_list] + assert "/data" in cmds[0][2] and "-delete" in cmds[0][2] + assert cmds[1][-1] == "/data" + + def test_restore_baseline_raises_on_tar_failure(): c = MagicMock() c.exec_run.side_effect = [(0, b""), (2, b"tar: broken")] try: - main.restore_baseline(c) + main.restore_baseline(c, main.DATA_DIR) assert False, "expected RuntimeError" except RuntimeError as e: assert "restore baseline" in str(e).lower() @@ -424,7 +436,7 @@ def test_restore_baseline_raises_on_wipe_failure(): c = MagicMock() c.exec_run.side_effect = [(1, b"find: permission denied")] # wipe fails first try: - main.restore_baseline(c) + main.restore_baseline(c, main.DATA_DIR) assert False, "expected RuntimeError" except RuntimeError as e: assert "wipe data dir" in str(e).lower() @@ -469,12 +481,14 @@ def test_capture_baseline_snapshots_excluding_license(): c = MagicMock() c.exec_run.return_value = (0, b"") c.image.attrs = {"Config": {"Entrypoint": None, "Cmd": ["/usr/local/bin/startup.sh"]}} - main.capture_baseline(c) + main.capture_baseline(c, main.DATA_DIR) cmds = [call.args[0] for call in c.exec_run.call_args_list] tar_cmds = [cmd for cmd in cmds if cmd and cmd[0] == "tar" and "czf" in cmd] assert tar_cmds, "expected a tar czf command" tar = tar_cmds[0] assert main.BASELINE_PATH in tar + # The snapshot is taken from the resolved data dir (-C ). + assert tar[tar.index("-C") + 1] == main.DATA_DIR # The license bind-mount must be excluded from the snapshot, or tar would # read the license through the mount into the archive. assert f"--exclude=./{main.LICENSE_FILENAME}" in tar @@ -486,12 +500,60 @@ def test_capture_baseline_raises_on_tar_failure(): # stop_connect ps (no connect), mkdir, then tar czf fails c.exec_run.side_effect = [(0, b""), (0, b""), (2, b"tar: broken")] try: - main.capture_baseline(c) + main.capture_baseline(c, main.DATA_DIR) assert False, "expected RuntimeError" except RuntimeError as e: assert "capture baseline" in str(e).lower() +def test_get_data_dir_uses_most_recent_log_line(): + """Resolves Connect's effective data dir from its startup log, taking the + latest line (a container accumulates one per boot / reset).""" + c = MagicMock() + c.logs.return_value = ( + b'time="..." level=info msg="Using data directory: /var/lib/rstudio-connect"\n' + b'time="..." level=info msg="Using data directory: /data"\n' + ) + assert main.get_data_dir(c) == "/data" + + +def test_get_data_dir_falls_back_to_default(): + c = MagicMock() + c.logs.return_value = b"no data directory line logged here" + assert main.get_data_dir(c) == main.DATA_DIR + + +def test_get_data_dir_matches_creating_on_first_boot(): + """A directory that doesn't already exist on disk (e.g. a custom + Server.DataDir/CONNECT_SERVER_DATADIR pointed at a fresh path) logs + "Creating data directory: " on its first boot, not "Using" -- the + built-in default dirs only log "Using" because they're pre-created in the + image. Missing this would silently fall back to DATA_DIR and capture an + empty baseline from the wrong directory.""" + c = MagicMock() + c.logs.return_value = ( + b'time="..." level=info msg="Creating data directory: /custom-data-dir"\n' + ) + assert main.get_data_dir(c) == "/custom-data-dir" + + +def test_resolve_real_path_follows_symlink(): + c = MagicMock() + c.exec_run.return_value = (0, b"/var/lib/with-connect\n") + assert main.resolve_real_path(c, "/mydata") == "/var/lib/with-connect" + c.exec_run.assert_called_once_with(["readlink", "-f", "/mydata"], user="root") + + +def test_resolve_real_path_raises_on_failure(): + c = MagicMock() + c.exec_run.return_value = (1, b"readlink: /mydata: No such file or directory") + try: + main.resolve_real_path(c, "/mydata") + assert False, "expected RuntimeError" + except RuntimeError as e: + assert "resolve real path" in str(e).lower() + + def test_stop_container_graceful_for_start_only(): """A start-only container (has a baseline) is stopped gracefully: Connect first, then a short container-stop timeout instead of the full ~10s grace.""" @@ -554,19 +616,22 @@ def test_reset_container_no_baseline_raises(): assert "start-only mode" in str(e) -def test_reset_container_custom_datadir_raises(): - """Reset must fail loudly (not silently no-op) when Connect's data is not under - the default data dir, e.g. a custom Server.DataDir.""" +def test_reset_container_external_db_raises(): + """Reset must fail loudly (not silently no-op) when the resolved data dir has + no built-in SQLite database -- i.e. Connect points at an external database.""" from unittest.mock import patch mock_container = MagicMock() mock_container.status = "running" + mock_container.logs.return_value = b'msg="Using data directory: /var/lib/rstudio-connect"' def fake_exec(cmd, **kwargs): joined = " ".join(cmd) if isinstance(cmd, list) else cmd + if cmd[:2] == ["readlink", "-f"]: # resolve_real_path -> echo input unchanged + return (0, (cmd[2] + "\n").encode()) if "baseline.tgz" in joined: # has_baseline -> True return (0, b"") - if "/db/" in joined: # connect_data_is_default -> False (no SQLite db) + if "/db/" in joined: # data_dir_has_sqlite_db -> False (external DB) return (2, b"ls: no such file or directory") return (0, b"") @@ -578,7 +643,7 @@ def fake_exec(cmd, **kwargs): main.reset_container("abc123") assert False, "expected RuntimeError" except RuntimeError as e: - assert "data directory" in str(e).lower() + assert "external database" in str(e).lower() # It must bail before mutating: the data-dir wipe (restore_baseline) never ran. cmds = [ " ".join(c.args[0]) if isinstance(c.args[0], list) else str(c.args[0]) @@ -587,6 +652,235 @@ def fake_exec(cmd, **kwargs): assert not any("-delete" in c for c in cmds), "must not wipe the data dir when the guard fires" +def test_data_dir_contains_baseline_dir(): + assert main.data_dir_contains_baseline_dir(main.BASELINE_DIR) is True + assert main.data_dir_contains_baseline_dir("/var/lib") is True + # The root dir is an ancestor of everything, including BASELINE_DIR -- a + # naive `baseline.startswith(data + "/")` check turns this into + # `startswith("//")`, which never matches, so this case regressed once. + assert main.data_dir_contains_baseline_dir("/") is True + assert main.data_dir_contains_baseline_dir(main.DATA_DIR) is False + assert main.data_dir_contains_baseline_dir("/data") is False + # Must not falsely match on a raw string prefix without a path separator: + # "/var/lib/with-connect-other" is a sibling, not an ancestor of BASELINE_DIR. + assert main.data_dir_contains_baseline_dir(main.BASELINE_DIR + "-other") is False + + +def test_data_dir_overlaps_baseline_checks_literal_and_resolved(): + """data_dir_overlaps_baseline must catch an overlap via EITHER BASELINE_DIR's + literal path or its resolved real target, since either one being deleted + is unsafe (the literal path breaks our own subsequent references; the + resolved target is where the archive's actual bytes live).""" + def readlink_returns(target): + c = MagicMock() + c.exec_run.return_value = (0, (target + "\n").encode()) + return c + + # Resolved target overlaps data_dir, even though the literal path doesn't. + assert main.data_dir_overlaps_baseline(readlink_returns("/data/with-connect"), "/data") is True + # Literal path overlaps data_dir, even though the resolved target doesn't. + assert main.data_dir_overlaps_baseline(readlink_returns("/opt/with-connect"), "/var/lib") is True + # Neither overlaps. + assert main.data_dir_overlaps_baseline(readlink_returns(main.BASELINE_DIR), "/data") is False + + +def test_reset_container_baseline_dir_overlap_raises(): + """Reset must fail loudly (not silently no-op, and not wipe anything) when + the resolved data dir is or contains BASELINE_DIR -- e.g. a custom + Server.DataDir set to /var/lib -- since the wipe would delete the baseline + archive the restore step needs, along with anything else under it.""" + from unittest.mock import patch + + mock_container = MagicMock() + mock_container.status = "running" + mock_container.logs.return_value = b'msg="Using data directory: /var/lib"' + + def fake_exec(cmd, **kwargs): + joined = " ".join(cmd) if isinstance(cmd, list) else cmd + if cmd[:2] == ["readlink", "-f"]: # resolve_real_path -> echo input unchanged + return (0, (cmd[2] + "\n").encode()) + if "baseline.tgz" in joined: # has_baseline -> True + return (0, b"") + if "/db/" in joined: # data_dir_has_sqlite_db -> True + return (0, b"/var/lib/db/connect.db\n") + return (0, b"") + + mock_container.exec_run.side_effect = fake_exec + mock_client = MagicMock() + mock_client.containers.get.return_value = mock_container + with patch.object(main.docker, "from_env", return_value=mock_client): + try: + main.reset_container("abc123") + assert False, "expected RuntimeError" + except RuntimeError as e: + assert main.BASELINE_DIR in str(e) + cmds = [ + " ".join(c.args[0]) if isinstance(c.args[0], list) else str(c.args[0]) + for c in mock_container.exec_run.call_args_list + ] + assert not any("-delete" in c for c in cmds), "must not wipe the data dir when the guard fires" + + +def test_reset_container_symlinked_datadir_overlap_raises(): + """The overlap guard must catch a Server.DataDir that's a symlink into + BASELINE_DIR, not just a lexical match -- resolve_real_path follows the + symlink before data_dir_contains_baseline_dir compares paths.""" + from unittest.mock import patch + + mock_container = MagicMock() + mock_container.status = "running" + mock_container.logs.return_value = b'msg="Using data directory: /mydata"' + + def fake_exec(cmd, **kwargs): + joined = " ".join(cmd) if isinstance(cmd, list) else cmd + if cmd[:2] == ["readlink", "-f"]: # /mydata is a symlink into BASELINE_DIR + return (0, b"/var/lib/with-connect\n") + if "baseline.tgz" in joined: # has_baseline -> True + return (0, b"") + if "/db/" in joined: # data_dir_has_sqlite_db -> True + return (0, b"/var/lib/with-connect/db/connect.db\n") + return (0, b"") + + mock_container.exec_run.side_effect = fake_exec + mock_client = MagicMock() + mock_client.containers.get.return_value = mock_container + with patch.object(main.docker, "from_env", return_value=mock_client): + try: + main.reset_container("abc123") + assert False, "expected RuntimeError" + except RuntimeError as e: + assert main.BASELINE_DIR in str(e) + cmds = [ + " ".join(c.args[0]) if isinstance(c.args[0], list) else str(c.args[0]) + for c in mock_container.exec_run.call_args_list + ] + assert not any("-delete" in c for c in cmds), "must not wipe the data dir when the guard fires" + + +def test_reset_container_symlinked_baseline_dir_overlap_raises(): + """The overlap guard must also resolve BASELINE_DIR itself, not just + data_dir -- if BASELINE_DIR is a symlink (e.g. because /var/lib is + symlinked on a read-only-root image), comparing data_dir against the + unresolved BASELINE_DIR constant would miss an overlap that only exists + through the symlink's real target.""" + from unittest.mock import patch + + mock_container = MagicMock() + mock_container.status = "running" + mock_container.logs.return_value = b'msg="Using data directory: /data"' + + def fake_exec(cmd, **kwargs): + joined = " ".join(cmd) if isinstance(cmd, list) else cmd + if cmd[:2] == ["readlink", "-f"]: + if cmd[2] == main.BASELINE_DIR: + # BASELINE_DIR is a symlink whose real target lives under /data. + return (0, b"/data/with-connect\n") + return (0, (cmd[2] + "\n").encode()) # data_dir itself isn't a symlink + if "baseline.tgz" in joined: # has_baseline -> True + return (0, b"") + if "/db/" in joined: # data_dir_has_sqlite_db -> True + return (0, b"/data/db/connect.db\n") + return (0, b"") + + mock_container.exec_run.side_effect = fake_exec + mock_client = MagicMock() + mock_client.containers.get.return_value = mock_container + with patch.object(main.docker, "from_env", return_value=mock_client): + try: + main.reset_container("abc123") + assert False, "expected RuntimeError" + except RuntimeError as e: + assert main.BASELINE_DIR in str(e) + cmds = [ + " ".join(c.args[0]) if isinstance(c.args[0], list) else str(c.args[0]) + for c in mock_container.exec_run.call_args_list + ] + assert not any("-delete" in c for c in cmds), "must not wipe the data dir when the guard fires" + + +def test_reset_container_literal_baseline_dir_overlap_raises(): + """The overlap guard must also catch data_dir overlapping BASELINE_DIR's + LITERAL path even when BASELINE_DIR resolves elsewhere -- e.g. BASELINE_DIR + is itself a symlink pointing outside data_dir. Wiping data_dir would still + delete that symlink's directory entry (not its target's contents), which + breaks every subsequent reference built from the literal BASELINE_PATH + constant, even though the archive's real bytes survive. A resolved-only + comparison would miss this, since the resolved target doesn't overlap.""" + from unittest.mock import patch + + mock_container = MagicMock() + mock_container.status = "running" + mock_container.logs.return_value = b'msg="Using data directory: /var/lib"' + + def fake_exec(cmd, **kwargs): + joined = " ".join(cmd) if isinstance(cmd, list) else cmd + if cmd[:2] == ["readlink", "-f"]: + if cmd[2] == main.BASELINE_DIR: + # BASELINE_DIR is a symlink resolving OUTSIDE data_dir. + return (0, b"/opt/with-connect\n") + return (0, (cmd[2] + "\n").encode()) # data_dir itself isn't a symlink + if "baseline.tgz" in joined: # has_baseline -> True + return (0, b"") + if "/db/" in joined: # data_dir_has_sqlite_db -> True + return (0, b"/var/lib/db/connect.db\n") + return (0, b"") + + mock_container.exec_run.side_effect = fake_exec + mock_client = MagicMock() + mock_client.containers.get.return_value = mock_container + with patch.object(main.docker, "from_env", return_value=mock_client): + try: + main.reset_container("abc123") + assert False, "expected RuntimeError" + except RuntimeError as e: + assert main.BASELINE_DIR in str(e) + cmds = [ + " ".join(c.args[0]) if isinstance(c.args[0], list) else str(c.args[0]) + for c in mock_container.exec_run.call_args_list + ] + assert not any("-delete" in c for c in cmds), "must not wipe the data dir when the guard fires" + + +def test_reset_container_resets_resolved_data_dir(): + """Reset resolves the effective data dir (e.g. the legacy image's /data) and + wipes/restores THAT dir, not the hard-coded default.""" + from unittest.mock import patch + + mock_container = MagicMock() + mock_container.status = "running" + mock_container.logs.return_value = b'msg="Using data directory: /data"' + mock_container.image.attrs = { + "Config": {"Entrypoint": None, "Cmd": ["/usr/local/bin/startup.sh"]} + } + + def fake_exec(cmd, **kwargs): + joined = " ".join(cmd) if isinstance(cmd, list) else cmd + if cmd[:2] == ["readlink", "-f"]: # resolve_real_path -> echo input unchanged + return (0, (cmd[2] + "\n").encode()) + if "test -f" in joined and "baseline.tgz" in joined: # has_baseline -> True + return (0, b"") + if "/db/*.db" in joined: # has SQLite db -> True + return (0, b"/data/db/connect.db\n") + if "ps -eo" in joined: # get_connect_pid -> none running + return (0, b" PID ARGS\n1 sleep infinity\n") + return (0, b"") # wipe, tar restore, start_connect all succeed + + mock_container.exec_run.side_effect = fake_exec + mock_client = MagicMock() + mock_client.containers.get.return_value = mock_container + with patch.object(main.docker, "from_env", return_value=mock_client), \ + patch.object(main, "discover_host_port", return_value=3939), \ + patch.object(main, "wait_until_healthy", return_value=True): + main.reset_container("abc123") + cmds = [ + " ".join(c.args[0]) if isinstance(c.args[0], list) else str(c.args[0]) + for c in mock_container.exec_run.call_args_list + ] + # The wipe targets /data, and the baseline is extracted back into /data. + assert any("/data" in c and "-delete" in c for c in cmds), cmds + assert any("-C /data" in c for c in cmds), cmds + + def test_reset_container_not_running_raises(): from unittest.mock import patch From c7b88df1ce3f43ddf7035cf9aa25b5c061776d5c Mon Sep 17 00:00:00 2001 From: Christopher Graham Date: Tue, 21 Jul 2026 11:23:47 -0400 Subject: [PATCH 3/3] address PR review: drop stray ci.yml comments, clarify main()/run() naming Removed two comments added to test-action/test-action-start-only that described jobs this PR didn't otherwise touch, which the reviewer found confusing. Renamed the old main()/cli() pair so the console-script entry point and the __main__ guard both call a function actually named main(), matching the Google Python Style Guide and Python Packaging User Guide's own convention. The former main() (core CLI logic) is now run(). --- .github/workflows/ci.yml | 3 --- main.py | 22 ++++++++++++---------- pyproject.toml | 4 +--- test_main.py | 2 +- 4 files changed, 14 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4b1158b..d4a9c44 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,8 +24,6 @@ jobs: - name: Run unit tests run: uv run test_main.py - # Command mode on the modern ghcr.io/posit-dev/connect image (default version), - # across x86 and arm runners. test-action: strategy: fail-fast: false @@ -60,7 +58,6 @@ jobs: [ "$TEST_STRING" = "This contains single quotes" ] || exit 1 echo "✓ Multiline test passed - variables, single quotes, and double quotes all work" - # Legacy image (rstudio/rstudio-connect) start-only + graceful stop. test-action-start-only: runs-on: ubuntu-latest steps: diff --git a/main.py b/main.py index faf9bd0..f889309 100644 --- a/main.py +++ b/main.py @@ -36,7 +36,7 @@ BASELINE_DIR = "/var/lib/with-connect" BASELINE_PATH = f"{BASELINE_DIR}/baseline.tgz" -# In start-only mode, PID 1 is docker-init (added via init=True in main()), which +# In start-only mode, PID 1 is docker-init (added via init=True in run()), which # reaps orphaned Connect child processes and forwards signals. It runs a keep-alive # `sleep infinity` as its child so Connect can be cycled (for --reset) without # stopping the container; Connect itself is launched via `docker exec` using the @@ -292,7 +292,7 @@ def build_run_kwargs(image_name, port, mounts, container_env, base_image, is_sta """Assemble the docker containers.run kwargs. Start-only mode adds a keep-alive PID 1 command, a crash-surfacing healthcheck, - and init=True; command mode uses the image's default entrypoint. See main(). + and init=True; command mode uses the image's default entrypoint. See run(). """ run_kwargs = { "image": image_name, @@ -320,9 +320,9 @@ def build_run_kwargs(image_name, port, mounts, container_env, base_image, is_sta return run_kwargs -def main() -> int: +def run() -> int: """ - Main entry point for the with-connect CLI tool. + Core logic for the with-connect CLI tool. --stop and --reset short-circuit to stop or reset an existing container and return. Otherwise the workflow is: @@ -891,16 +891,18 @@ def reset_container(container_id: str) -> None: print(f"Connect reset and ready at http://localhost:{port}", file=sys.stderr) -def cli() -> None: - """Console-script entry point. Wraps main so a RuntimeError prints a clean - 'Error: ...' line with exit code 1 instead of a traceback. The installed - command skips the __main__ block, so it must target cli, not main.""" +def main() -> None: + """Console-script entry point (see pyproject.toml's [project.scripts]) and + __main__ target. Wraps run() so a RuntimeError prints a clean 'Error: ...' + line with exit code 1 instead of a traceback. Installed console scripts + call this function directly and never go through the __main__ block below, + so the error handling has to live here rather than in that block.""" try: - sys.exit(main()) + sys.exit(run()) except RuntimeError as e: print(f"Error: {e}", file=sys.stderr) sys.exit(1) if __name__ == "__main__": - cli() + main() diff --git a/pyproject.toml b/pyproject.toml index ddf14a4..cb9c868 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -10,9 +10,7 @@ dependencies = [ ] [project.scripts] -# Target cli, not main: the installed command skips the __main__ block, so cli -# gives it the same clean error handling as python main.py. -with-connect = "main:cli" +with-connect = "main:main" [tool.setuptools] py-modules = ["main"] diff --git a/test_main.py b/test_main.py index e5efd26..0a17e0d 100644 --- a/test_main.py +++ b/test_main.py @@ -922,7 +922,7 @@ def test_cli_reports_runtime_error_without_traceback(): result = subprocess.run( [sys.executable, "-c", "import sys; sys.argv=['with-connect','--reset','nonexistent_cli_test']; " - "import main; main.cli()"], + "import main; main.main()"], capture_output=True, text=True, ) assert result.returncode == 1