Repository navigation
FEAT: Restore saved registry entries and stay not ready if any fail (part 4) - #3909
varunj-msft wants to merge 96 commits into
Conversation
…API input External construction now rejects a parameter whose type includes Path unless the calling service passes the name in allowed_path_params after making the value safe. The converter service allows its upload parameters once they are stored, and the target service allows the server-set upload folder and the GitHubCopilotTarget working directory. The scorer-only check is removed, and the "Valid parameters" list leaves out path parameters callers cannot set.
The target catalog now applies the path rule to required parameters too, so a target that needs a disallowed path is left out instead of listed without the field, matching the scorer catalog. allowed_path_params is typed frozenset[str] so a bare string cannot turn the allow-list into a substring match.
The target catalog still lists a target whose required path parameter is its server-set upload folder when the server has one, and leaves it out otherwise.
# Conflicts: # tests/unit/backend/test_converter_service.py
…benchmark's recipes folder
# Conflicts: # pyrit/backend/services/runtime_lifecycle.py # tests/unit/backend/test_runtime_lifecycle.py
Withdrawing my approval for now so I can review this PR again.
| self.restore_failures, self.restore_ignored = report.failures, report.ignored | ||
| if not report.failures: | ||
| return True | ||
| self.state, self.outcome = "failed", "restore-failed" |
There was a problem hiding this comment.
Design: one failed entry makes the API unavailable for everyone
Any restore failure leaves the runtime failed, and the runtime gate then returns 503 for every /api/ route except config, initializers, auth and health. Targets, attacks, scenarios, presets, converters and scorers are down for every user until the file is fixed or deleted. I agree with the goal: a scan must never quietly run without a saved entry it expects. But this ties the whole app's availability to the health of every saved file, including ones nothing uses.
Realistic ways to get here:
- Upgrades. Moving a class to another module fails every saved entry of that class as
module_mismatch(cls.__module__ != recipe.class_module). Removing or renaming a class fails them asunknown_class. PyRIT does move classes between releases, so a routine upgrade could come up not ready. - Rollbacks. After a rollback across a format change, entries saved by the newer version fail as
newer_schema_version, so rolling back doesn't restore service either. - Shared deployments. One user's broken target takes the API down for everyone.
- Storage hiccups. A Blob error on a single download becomes an
unreadable_recipefailure, which blocks too.
Suggestion: stay ready, and fail closed only where a failed entry could change what runs.
- Reserve the names of failed entries, so a lookup gets the restore failure rather than "not found" or a fallback.
- Refuse a launch that would use a failed entry, by name or by tag. If tag matching is too hard to get right, refuse scenario launches while any restore failure exists, and keep everything else up, including the Retry and Delete that come in later PRs.
- If some deployments want today's all-or-nothing behavior, make it an opt-in setting.
Related: diagnostics. When this does block, the operator needs the cause quickly. The restore code doesn't log anything, and the log line below records only kind/name: reason. The API message is the only detail, and _safe_message hides it entirely when it contains @ or %. Before this merges, please log each failure once per restore with its message and, when there is one, the exception with its traceback, with any SAS token redacted. The API text can stay sanitized as it is.
There was a problem hiding this comment.
Put the launch check in ScenarioRunService; a coarser rule is simpler
The code supports staying ready, as the #3908 review now plans. A missing saved entry can change what runs only in a scenario run: all, default, and tag selections skip a missing technique, and a missing adversarial_chat or objective_scorer_chat falls back to a plain OpenAIChatTarget (scenario_target_defaults.py:133-153). Only scenario code and scenario_run_service.py (lines 408, 456, 523) reach these paths. Other lookups by name already fail with "not found", except after a name_clash: there the startup entry keeps the name, and the failure list names the clash.
- Put the check in
ScenarioRunService.start_run_asyncandresume_run_async, not only in the routes. Suite launches go throughstart_run_async, and resume has no route check today (routes/scenarios.py:269). - Points 2 and 4 of that review keep launches that name their techniques running. They need a failed-names check in each resolver and point 3 for a folder that cannot be listed (that failure has no name), and a
name_clashstill leaves the startup entry under the name. A simpler rule is to refuse every scenario start and resume whilerestore_failuresis not empty. It needs none of these changes. Its cost is that no scenario launch starts while any failure exists; the rest of the API stays up either way. - If a deployment must still stop on a failure, add a failure count to
/api/healthfor the deploy check.deployment_common.sh:263stops only on the statesfailed,restart-required, andstopping. - Where live apply is on (
enable_live_reinitialization: true, off by default), apply then works as a retry after a fix, because it resets the registries and restores again (line 265). Todaybegin_applyrefusesfailed(line 204), but the GUI still offers Reinitialize in that state (Reinitialize.tsx:101). - Logging: log each failure's sanitized
messagenow (line 158 logs onlykind/name: reason), with acaplogtest for one record per failure and nosig=value. For a failure with no name, the message, log, and docs must say that the source could not be read, not "fix or delete the files". The traceback needs FEAT: Rebuild saved registry entries in dependency order (part 3) #3908 to keep the exception (r4517083964); keep it out ofRestoreFailure, becausestatus()returns that model to the API.
Make this change here, because #3965 builds Retry and its banner on the failed state.
varunj-msft
changed the title
FileDocumentStorage now raises DocumentStorageUnavailableError when the folder or container can't be used, and DocumentUnreadableError when one document exists but can't be read. The message names the location, with any SAS token hidden, and a short reason. One backend handler maps both to 503 and logs the location and reason, with the traceback at debug level, so the per-route storage helpers are gone. A single read, a conditional save and a delete now follow the listing's rules: a folder, pipe or broken link named like a document is unreadable rather than missing, and on Blob only a deleted blob counts as missing. Blob sign-in, permission, network and missing-container errors stop the listing instead of marking every file unreadable. Each unreadable report keeps the real file name and the original error, and the listing's documents are read-only. Initializer delete removes a script that can't be read or decoded, and registering one no longer turns a storage write failure into a 400. Tests patch module-level aliases instead of os.
List recipes escapes the OS text it reports, such as a file name that isn't UTF-8, and a recipe whose JSON holds an unpaired surrogate escape is reported as invalid, so the models no longer need a JSON-only serializer. Recipes may start with a byte-order mark. A document without schema_version reads as version 1, and each file is checked against the format version of the model that reads it. Validation summaries shorten each location part and the whole message, since a location can repeat a stored key. Recipes and StoredRecipe are no longer frozen, since their params and tags never were, and StoredRecipe checks that its recipe type matches its registry. The listing's recipes are read-only, and it keeps the error behind each recipe that couldn't be read, for the log.
Listing catches only storage errors, so a bug is raised instead of being reported as unreadable storage, and a bug while planning is a build error rather than an invalid recipe. Each failure is logged once, with the error behind it and URL credentials removed. The report shows the error's text with each URL's user name, query and fragment removed, any SAS signature hidden, and the length capped. A class that moved within PyRIT is restored; one now registered from outside PyRIT still fails. A technique whose name or tag clashes is a name clash, and anything else registration refuses is an invalid recipe. A recipe that sets a server file or folder is refused before it is built. Class metadata comes from the registry's cache. The class docstring lists the steps the API's create runs that restore skips.
The runtime becomes ready even when some saved entries fail; the failures stay in the runtime status. Each failed entry keeps its name in its registry: looking it up, or taking the name, raises SavedEntryUnavailableError, which the backend answers with 409, so nothing else stands in for it. Administrators see the full message and everyone else a fixed explanation of the reason. A folder that can't be listed holds every unregistered name of its kind, and each entry it left unbuilt is reported. While a saved technique is held, a launch or estimate that picks techniques by all, default or a tag is refused with 409 for scenarios whose techniques come from the registry, since the broken technique's tags may be unknown. Naming techniques still works; naming the held one fails with its reason. Route catch-alls let the error through instead of turning it into a 500.
| registry_recipes_source: https://account.blob.core.windows.net/pyrit-storage/registry-recipes | ||
| ``` | ||
|
|
||
| Each kind has its own folder under the source (`targets/`, `converters/`, `scorers/`, `techniques/`), with one JSON file per saved entry named after the entry. A file holds what's needed to rebuild the entry; restore refuses a file that stores a value for a parameter its class marks as a credential, such as `api_key`. A SAS query string may be included; otherwise, PyRIT uses `DefaultAzureCredential`. The default is `~/.pyrit/registry_recipes`. |
There was a problem hiding this comment.
[P2] Warn that write access to this source gives administrator authority
Restore builds every file in this source at startup and after a live apply, but it skips the create API's administrator checks (target_service.py:464-469). A principal that can write a file here, but is not a backend administrator, can save a HuggingFaceChatTarget with trust_remote_code: true. When the server has the Hugging Face dependencies (torch and transformers) and a HUGGINGFACE_TOKEN, restore then loads the model with remote code under the backend identity. The same writer can save an AzureBlobStorageTarget without sas_token, so the target uses the server's credentials on a container that the writer chose. This paragraph mentions only the credential check, so an operator can think that these files are screened. Add a warning like the one for custom_initializers_source above, here and in .pyrit_conf_example: give write access only to principals that you trust as backend administrators, and use a dedicated folder or blob prefix. If building moves to the service create methods (#3908 comment), restore still needs an administrator context to rebuild entries that an administrator saved, because a recipe does not record who saved it. That context passes these checks, so the warning is still necessary.
Offline repro (model loader replaced, no network): a saved HuggingFaceChatTarget file with trust_remote_code: true restored with no failure, and the loader got trust_remote_code=True. create_target_async(..., is_admin=False) with the same params raised PermissionError.
| targets = TargetRegistry.get_registry_singleton() | ||
| config = await runtime._load_async() | ||
| config.registry_recipes_source = str(recipes) | ||
| config.apply_prepared_reinitialization_async.side_effect = lambda **_: targets.instances.register( |
There was a problem hiding this comment.
[P3] Test an apply that follows a startup restore
In production, a live apply runs only after startup restored every saved entry, so the registries already hold those entries. Only reset_setup_registries() in the real apply prevents a name_clash for each one. This stub skips that reset, and startup_chat starts with empty registries. If the reset stops clearing converters or scorers, no test fails, and every apply on a server with saved entries of that kind fails its restore. Other tests pin only the target and technique resets, and no test runs an apply after a startup restore. Restore the same recipes at startup first, and make this stub call reset_setup_registries() before it registers startup_chat. Repro: restoring a ROT13Converter recipe twice without a reset gives name_clash ("Instance 'rot' already exists"); after reset_setup_registries() it restores.
Description
This is one of the changes for saving Registry entries across restarts. It builds on #3908. Until the earlier PRs merge, the diff here also shows their changes.
Problem. Saved entries need to come back at startup and after a live apply. A broken one must stop runs instead of vanishing; otherwise a scan that expects a saved converter or
adversarial_chatwould run with something else, or fail later.Change.
registry_recipes_source: a local folder or Azure Blob location, default~/.pyrit/registry_recipes. It's validated likescenario_presets_sourceand documented inpyrit_conf.mdand.pyrit_conf_example.RuntimeLifecyclerestores saved entries in two places. At startup, after the initializers run and the configuration version is read. On apply, after the new version is recorded; apply already resets the registries, so restore starts clean. The recipes are read in a worker thread, and the entries are built on the runtime loop, as the target API builds them, since a constructor such asHuggingFaceChatTarget's starts a task on that loop. As with the target API, a slow constructor holds the loop while it runs.failedwith outcomerestore-failedand doesn't publish. The middleware already returns 503 for runtime routes, and/api/healthshowsready: false.GET /api/config/runtimelistsrestore_failuresandrestore_ignored, and the failures are logged. For now the fix is to correct or delete the file and restart.What stays the same. With no saved entries, startup and apply behave as before. A file whose name isn't a valid entry name is listed as ignored and doesn't block readiness.
Next. Fixing or deleting a broken entry without a restart, and showing the failure list to every signed-in user rather than only in the admin runtime status, come with the recovery routes in #3965 (part 5).
Tests and Documentation
tests/unit/backend/test_runtime_lifecycle.py: startup and apply restore an entry that uses a target the startup scripts register, the recipes are read in a worker thread and restored entries are built on the runtime loop, startup and apply with a broken entry stay not ready and list it in the runtime status, ignored files don't block readiness, fixing the file and restarting reaches ready, cancellation still propagates, and a reference name or file name that isn't valid text still returns 200.tests/unit/setup/test_configuration_loader.pycovers the new setting;tests/unit/backend/conftest.py, the analytics benchmark fixture and the offline browser fixture (frontend/e2e/fixtures/manual_send_backend.py) point the recipes folder at a temp folder, so they never read a developer's saved entries.pytest tests/unit/backend tests/unit/setup tests/unit/registry tests/unit/models -n 4: 8,053 passed, 9 skipped, 4 xfailedpre-commit run --from-ref origin/main --to-ref HEAD: passed/api/healthshowedready: false,GET /api/config/runtimereturned 200 with all three failures and the escaped file name, and neither the status nor the log held the test secret. After deleting the three broken files and restarting, it reached ready, with the oddly named file still listed as ignored.HuggingFaceChatTarget(offline, so only its background model load fails): it reached ready at startup and after a live apply. With the restore in a worker thread instead, startup failed withno running event loop.doc/getting_started/pyrit_conf.mdand.pyrit_conf_exampledescriberegistry_recipes_source. JupyText: not run (no notebooks changed).