diff --git a/docs/review/remote_pull_and_mirror_operability_2026-08-04.md b/docs/review/remote_pull_and_mirror_operability_2026-08-04.md index b830fab..2188f45 100644 --- a/docs/review/remote_pull_and_mirror_operability_2026-08-04.md +++ b/docs/review/remote_pull_and_mirror_operability_2026-08-04.md @@ -127,6 +127,28 @@ synchronous consumer must therefore either cache or expect minutes; ours set a *Suggested:* document the cost on `/remote/ls`, and consider a cheap existence/count path that does not enumerate. +**Answered in v5.8.0. Documented, existence check pointed at, count declined — +with a reason.** + +- The cost and both measurements are now on `GET /remote/ls` in + `docs/source/http.md` (§ Listing Cost) and in `owi remote ls --help`, + including the advice not to use `ls` as a liveness probe and not to set a + short timeout against an object-store remote. +- **The cheap existence check already existed and now works.** + `FileBasedRepository.status` does one `fs.exists` per access level and never + enumerates, and F7 is what stopped it raising for exactly these backends. So + the non-enumerating probe consumers want is `POST /remote/doctor/status`, + which is where the docs now send them. +- **A cheap *count* is not offered, deliberately.** Counting datasets in an + object store means listing the keys under the prefix; there is no cheaper + primitive to wrap. The measurement above is the evidence rather than an + assumption: the 84 s call returned *zero* datasets, so `load_metadata` was + never reached and the whole 84 s went into `listdir`/`ls` traversal. A + "shallow count" that skipped per-dataset metadata would therefore have cost + the same 84 s while advertising itself as cheap. Serving a count without + enumerating requires a maintained index or manifest — a different feature, + and not one `ls` can be adapted into. + --- ## F5 — `remote doctor` gives HTTP callers nothing to read @@ -193,12 +215,22 @@ discovered backend, including ones added later. ## F6 — Smaller things +**Both fixed and released in v5.8.0.** + - **Two bare `except:`** clauses in `core/tasks/remote.py` swallow - `KeyboardInterrupt` and `SystemExit` along with everything else. + `KeyboardInterrupt` and `SystemExit` along with everything else. *Fixed:* + `except Exception` with a debug log; the expected case is a missing README or + stats file, which is not worth a warning but is worth being able to see. - **`ask_yes_no` assumes a TTY**: `termios.tcgetattr(sys.stdin)` raises `Inappropriate ioctl for device` when stdin is not a terminal. Reached whenever a non-interactive caller omits `--yes`; detecting `sys.stdin.isatty()` - and declining (or erroring clearly) would be kinder than a traceback. + and declining (or erroring clearly) would be kinder than a traceback. *Fixed:* + declines and prints what to pass instead. Declining rather than accepting is + deliberate — `default='y'` describes what Enter means for a human at a prompt, + not consent from a caller that was never asked. `--yes` still short-circuits + ahead of the check. Only `core/manager/ui.py` had this; the second + `ask_yes_no` in `compat/core_base.py` uses `Prompt.ask` and never touches + termios. - **Not a finding, recorded to prevent re-reporting:** `files` is parsed with `ast.literal_eval` in `query_utils.py`, not bare `eval`. Older versions do evaluate `files` directly. Checked against the tags while releasing v5.6.0: @@ -211,6 +243,30 @@ discovered backend, including ones added later. --- +## F8 — `owi remote ls --help` crashed + +**Severity: low, but it had been broken for a long time. Fixed and released in +v5.8.0.** + +Found while documenting F4, by running the command whose help was being edited: + +``` +MarkupError: closing tag '[/]' at position 74 doesn't match any open tag +``` + +Typer renders docstrings as Rich markup, and the specifier syntax in that +docstring — `[:|...][/]` — is *also* valid Rich markup, so +`[/]` parsed as a closing tag with nothing open. Help for the most-used +remote command therefore produced a traceback instead of help. Reproduced +against the v5.7.0 tag, so this predates the current work by some distance. + +*Fixed:* the docstring is raw and the literal brackets are escaped. Nothing had +ever invoked `--help` in a test, which is why it went unnoticed; +`tests/owilix/cli/test_help_renders.py` now sweeps **all 57 commands**, and +fails at v5.7.0 on exactly `remote ls`. + +--- + ## What we changed on our side For the record, so this review is not read as "all of it is yours": diff --git a/docs/source/http.md b/docs/source/http.md index 782751f..5fee40e 100644 --- a/docs/source/http.md +++ b/docs/source/http.md @@ -89,7 +89,8 @@ Local endpoints: Remote endpoints: -- `GET /remote/ls` +- `GET /remote/ls` (**can take minutes on object-store remotes** — see + [Listing cost](#listing-cost)) - `POST /remote/search` - `POST /remote/pull` - `POST /remote/push` @@ -177,12 +178,46 @@ owilix http serve For local-only testing, `OWILIX_HTTP_S3_NO_AUTH=true` disables S3 facade credential checks. +## Listing Cost + +`GET /remote/ls` enumerates. For a LEXIS repository the catalogue is queried and +the answer comes back quickly; for an object-store remote (`s3a`/`s3`) the store +itself is walked, so **the cost tracks the size of the store rather than the size +of the answer** — and it is slowest when it has least to say. Measured against a +full OWI cluster: + +| specifier | time | result | +| --- | --- | --- | +| `owi-up/collectionName=main` (s3a) | **84.1 s** | 0 datasets | +| `lexis/collectionName=main` | 7.1 s | 584 datasets | + +Consequences for a synchronous caller: + +- **Budget minutes, not seconds.** A 30 s client timeout against an object-store + remote will fail every time, and it fails in the way least likely to be + diagnosed correctly — as a connection error rather than as a slow answer. +- **Cache the result** if you are rendering it repeatedly (a dashboard panel, a + health view). The answer changes far more slowly than the cost of asking. +- **Do not use `ls` as a liveness probe.** Use `POST /remote/doctor/status`, + which reports per-access-level existence with a handful of prefix checks and + does not enumerate. It answers "is this remote reachable, and does it hold + anything at each access level" — which is what a health light needs. + +There is deliberately **no cheap dataset *count*** endpoint. A count over an +object store requires listing the keys under the prefix; there is no cheaper +primitive to expose. The measurement above is the evidence: that 84 s call +returned *zero* datasets, so no per-dataset metadata was ever fetched and the +time went entirely into walking the store. A "count without enumerating" would +have to be served from a maintained index or manifest rather than from the store, +which is a different feature and not something `ls` can be made to do. + ## Known Limits - HTTP endpoint authorization is preview-level; use only on localhost or behind external access control. - Jobs are in-process only and disappear after restart. - Job cancellation and durable progress records are not implemented. -- `remote/ls` pagination currently slices after fetching backend results. +- `remote/ls` pagination currently slices after fetching backend results — so a + page request costs the same as the full listing (see [Listing cost](#listing-cost)). - The S3 facade has been smoke-tested; broader compatibility with `aws s3`, `boto3`, and `rclone` remains follow-up work. diff --git a/owilix/cli/remote.py b/owilix/cli/remote.py index 8862308..a4f3aed 100644 --- a/owilix/cli/remote.py +++ b/owilix/cli/remote.py @@ -76,11 +76,11 @@ def ls( files_glob: Optional[str] = typer.Option(None, "--files", "-f", help="List files matching glob pattern (e.g., '**/*.parquet')"), file_details: bool = typer.Option(False, "--file-details", help="Show file details (size) - slower"), ): - """ + r""" List remote datasets matching SPECIFIER. - + Specifier format: - [:|latest|#|..][/] + \[:|latest|#|..]\[/] Scope aliases: all - all configured remote sources @@ -107,6 +107,19 @@ def ls( owi remote ls all/collectionName=main;zone=IT4ILexisV2,OWILRZZONE owi remote ls all/title*=.*Web.*; owi remote ls all/id=abc123 --files "**/*.parquet" + + Cost: + Listing an object-store remote (s3a/s3) scans it, so the time tracks the + size of the store rather than the size of the answer -- and it is slowest + when it has least to say. Measured on a full OWI cluster: + + owi-up/collectionName=main (s3a) 84.1 s -> 0 datasets + lexis/collectionName=main 7.1 s -> 584 datasets + + Budget minutes, or cache, for any synchronous caller. To ask only whether + a remote is reachable and holds anything, use 'owi remote doctor' + (POST /remote/doctor/status), which checks for existence per access level + instead of enumerating. """ cli_ctx: CLIContext = ctx.obj cli_ctx.fields = fields diff --git a/tests/owilix/cli/test_help_renders.py b/tests/owilix/cli/test_help_renders.py new file mode 100644 index 0000000..922be4f --- /dev/null +++ b/tests/owilix/cli/test_help_renders.py @@ -0,0 +1,75 @@ +"""`--help` must render for every command. + +`owi remote ls --help` crashed with + + MarkupError: closing tag '[/]' at position 74 doesn't match any open tag + +because the specifier syntax in its docstring -- ``[:...][/]`` +-- is also valid Rich markup, and Typer renders docstrings as markup. The help +for the most-used remote command was unreadable, and nothing failed to say so: +no test invoked ``--help``. + +Sweeping every command rather than pinning the one that broke, since the trap is +in the docstrings themselves and applies to any of them. +""" +import pytest +from typer.testing import CliRunner + +from owilix.cli import app + +runner = CliRunner() + + +def _command_paths(typer_app, prefix=()): + """Every (sub)command path in the CLI, as argv fragments.""" + import typer.main + + click_command = typer.main.get_command(typer_app) + yield from _walk(click_command, prefix) + + +def _walk(command, prefix): + subcommands = getattr(command, "commands", None) + if not subcommands: + return + for name, sub in subcommands.items(): + path = (*prefix, name) + yield path + yield from _walk(sub, path) + + +ALL_COMMANDS = sorted(_command_paths(app)) + + +def test_command_discovery_found_something(): + """Guard the guard: an empty parametrisation would pass silently.""" + assert len(ALL_COMMANDS) > 20, f"only discovered {len(ALL_COMMANDS)} commands" + assert ("remote", "ls") in ALL_COMMANDS + + +@pytest.mark.parametrize("path", ALL_COMMANDS, ids=lambda p: " ".join(p)) +def test_help_renders(path): + result = runner.invoke(app, [*path, "--help"]) + + assert result.exit_code == 0, ( + f"`owi {' '.join(path)} --help` failed:\n{result.output}" + ) + # A MarkupError is *raised*, so it surfaces as a non-zero exit above; check + # the text too in case it is ever caught and printed instead. + assert "MarkupError" not in result.output + + +def test_remote_ls_help_shows_the_specifier_syntax_literally(): + """The brackets are syntax to copy, not markup to interpret.""" + result = runner.invoke(app, ["remote", "ls", "--help"]) + + assert result.exit_code == 0 + assert "[/]" in result.output + + +def test_remote_ls_help_documents_the_listing_cost(): + """F4: the cost of listing an object-store remote must be discoverable.""" + result = runner.invoke(app, ["remote", "ls", "--help"]) + + assert "84.1 s" in result.output + assert "doctor" in result.output, "point the reader at the cheap probe"