Repository navigation
FEAT: Recover from a failed restore of saved registry entries without a restart (part 5) - #3965
varunj-msft wants to merge 110 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
# Conflicts: # pyrit/backend/services/runtime_lifecycle.py # tests/unit/backend/test_runtime_lifecycle.py
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.
The runtime now stays ready when saved entries fail, so the banner lists them whenever readiness reports a failure count, and reads the list only when that count or the runtime generation changes. A fixed or deleted file comes back on the next apply or restart. Retry, its route, the partial-restore bookkeeping it needed and its admission while not ready are gone. Users who aren't administrators still get a fixed explanation of each reason, now taken from the reason itself.
Richard Lundeen (richlundeen)
left a comment
Richard Lundeen (richlundeen)
left a comment
There was a problem hiding this comment.
Both earlier banner issues are fixed: Retry stays in view with 40 failures, and the banner says when the list can't be loaded. New comments: Retry and the banner depend on the failed state, which the open #3909 decision may remove; Retry does not check files of entries that already came back; and in the real app the banner is outside FluentProvider, so Retry has no button style and the banner hides the bottom of the app. The other comments are smaller (P3).
| or ``restart-required`` if the retry stopped partway through. | ||
| """ | ||
| config = self._restore_config | ||
| if self.state != "failed" or config is None: |
There was a problem hiding this comment.
[P2] Make Retry and the banner depend on the failure list, not on the failed state
Retry runs only when the state is failed (line 185), and the banner shows the list and Retry only in that state (useRuntime.tsx:145-148). The open #3909 decision (r4520174090) keeps the runtime ready when a saved entry fails. If #3909 makes that change, Retry returns 409 nothing-to-retry while failures exist, and the banner returns null because ready is true. /api/runtime has no failure count, so the GUI has no signal to show the banner. If Retry is then allowed from ready, a successful retry calls _publish, which makes a new generation on a runtime that is already ready, so open scenario selections become stale. Also, an unexpected retry error sets restart-required, which closes the API that the change keeps open. Decide #3909 CR-1 before this PR merges. To support both outcomes, allow Retry while restore_failures is not empty, and call _publish only when the state is failed. If a retry from ready stops with an unexpected error, keep the runtime ready and refuse later retries until a restart. Show the banner from a failure count in /api/runtime. Repro (offline, real RuntimeLifecycle): with state ready and one failure, retry_restore_async() returns nothing-to-retry.
| failure | ||
| for listing in listings | ||
| for failure in listing.failures | ||
| if failure.name is None or SavedEntryRef(kind=failure.kind, name=failure.name) not in restored |
There was a problem hiding this comment.
[P2] Check the files of restored entries on Retry, and keep that record on the registry entry
Retry skips each entry in restored and drops the listing failures of its file (lines 128-141). It does not compare the file with the object that it keeps. If an administrator changes, breaks, or deletes the file of an entry that came back, and then selects Retry, the retry reports no failure and the runtime becomes ready. The next restart reads the file again and then applies the change, fails, or drops the entry, and no one was told. The record is also a private set (_restored_entries, runtime_lifecycle.py:55), so each path that adds or removes a saved entry must update it. The stacked #4071 deletes a restored entry while the runtime is not ready and does not update the set. If the file is then saved again, Retry builds nothing for it and makes the runtime ready without that entry. Record each restored entry and its file version on its registry entry (create_named_instance accepts registry_metadata; techniques use register_runtime, so they need an equivalent, such as the SAVED origin that #4071 adds). A startup entry has no such mark, so a name_clash stays a failure. On Retry, skip a file whose saved entry is registered. If its version changed or the file is gone, report that in the response and the log, for example "changed after it was restored; restart to apply". Then remove _restored_entries and the restored parameter. Repro: restore a (valid) and b (unknown class); write not json to a and fix b; restore(restored=first.restored) returns no failures, and a fresh restore then fails a with invalid_recipe.
| {retryError && <div role="alert">{retryError}</div>} | ||
| </MessageBarBody> | ||
| <MessageBarActions> | ||
| <Button onClick={() => { void retry() }} disabled={retrying}>{retrying ? 'Retrying…' : 'Retry'}</Button> |
There was a problem hiding this comment.
[P2] Render the restore banner inside the app's FluentProvider, and test it there
App.tsx renders <RuntimeBanner /> before <ThemeProvider> (lines 1094-1097), so the banner is outside the app's FluentProvider and the Fluent tokens are not defined for it. Before this PR, the banner was one line of text with no control, so the missing styles mattered less. This PR adds this Button, a list, and an alert there. In the real app, the warning bar has no fill, border, or padding. Retry is the plain word "Retry" (64x21 px, with no border or fill). Operators can miss the only recovery action. The tests do not show this, because TestWrapper adds a FluentProvider (useRuntime.test.tsx:18-19) and no App test renders the failed state. Render the banner inside ThemeProvider, for example in a banner slot of MainLayout; RuntimeProvider can stay outside. Then add one App test with a failed runtime that finds Retry inside the app's provider, for example with closest('.' + fluentProviderClassNames.root), because jsdom does not resolve theme tokens.
| str: The outcome: ``success``, ``restore-failed``, ``nothing-to-retry``, ``unsupported``, | ||
| or ``restart-required`` if the retry stopped partway through. | ||
| """ | ||
| config = self._restore_config |
There was a problem hiding this comment.
[P3] Say when Retry cannot use a changed saved configuration
Retry restores from _restore_config, the configuration that startup or the last apply loaded (line 184). It does not read the saved configuration. Some failures have their cause in the configuration, for example an expired SAS token or a wrong container in a Blob registry_recipes_source. An administrator saves the fix on the Configuration page and selects Retry. Retry reads the old source again and shows the same failures with no reason, while the banner tells the user to fix the files and select Retry. Only a restart recovers. In retry_restore_async, read the version of the saved configuration and compare it with self.version. If they differ, say in the response and in the banner that Retry uses the running configuration and that a change to the saved configuration needs a restart. #3909 CR-1 already covers apply as a retry where live apply is on. Repro (offline, real RuntimeLifecycle): after a restore from folder A failed, a stub configuration source got no call during retry_restore_async(), and the retry returned restore-failed with the same failure.
| HTTPException: 409 if no failed restore is waiting, or the backend runs several workers or replicas. | ||
| """ | ||
| runtime = request.app.state.runtime_lifecycle | ||
| outcome = await runtime.retry_restore_async() |
There was a problem hiding this comment.
[P3] Write an audit record for each retry, as apply does
Any signed-in user can start a retry. A retry builds each saved file that is not restored, also a file that was added after startup, and it can make the runtime ready. Live apply has a similar effect, and it writes an audit record with the caller's oid and name (configuration.py:56-62, 76-96). A retry logs only PyRIT restore retry outcome=... generation=... (runtime_lifecycle.py:197) and any failures that remain, not the caller. Thus an operator cannot find which user started a retry, for example after a retry built a changed file that runs model repository code (#3909 CR-2). Write a secret-free audit record here with the caller's identity and the outcome, also for a refused retry. For example, move _audit_configuration_access to a shared module and call it with action="retry-restore".
| return ( | ||
| <MessageBar intent="warning"> | ||
| <MessageBarBody> | ||
| Some saved entries couldn't be restored, so new runs and sends are unavailable. Drafts are preserved. |
There was a problem hiding this comment.
[P3] Give the banner a title and remove the scope notes
The failed-state banner shows the status, the failure rows, the instruction, and any retry error in one text style with no title. It has 74 words with 4 failures, and 86 after a refused retry. The default-labels error bar in App.tsx (lines 888-901) does the same job in 19 words. Two sentences do not help a user act now: "Drafts are preserved." and "Retry rebuilds only these entries." The second sentence is also not correct, because Retry also builds saved files that were added after startup. pyrit_conf.md:207 has the same claim ("it rebuilds only the entries that didn't come back"). For administrators, describeFailure repeats a dependency that the message already names, for example "Uses target 'contoso_gpt4o_eastus', which doesn't exist. (target contoso_gpt4o_eastus)". The extra text makes the banner taller, so it hides more of the app. Use MessageBarTitle for "Some saved entries couldn't be restored". Keep one impact sentence and one instruction, and move the scope notes to the docs. Add the dependency suffix only when the message does not name it.
| try { | ||
| const result = await runtimeApi.retryRestore() | ||
| if (revision === listRevision.current) { | ||
| setFailures(result.items) |
There was a problem hiding this comment.
[P3] Show the result of each Retry
retry() stores result.items and ignores result.outcome. When no file changed, the response has the same items, so the banner does not change: after a real Retry against unchanged files, the banner pixels are the same as before the click. The button shows "Retrying…" only while the request runs, and the live region announces nothing because its content does not change. A refusal or a network error shows as one more plain line after "then select Retry", in the same style as the instructions. An administrator cannot tell whether Retry ran, and can click it again or think that it is broken. After each Retry, show a short status near the action, for example "Retry finished. 4 entries still couldn't be restored." Show retryError with error styling, for example a nested MessageBar intent="error".
| assert (item["reason"], item["message"]) == ("build_error", "It couldn't be built.") | ||
|
|
||
|
|
||
| async def test_retry_is_refused_when_no_restore_failed_async(runtime: RuntimeLifecycle) -> None: |
There was a problem hiding this comment.
[P3] Make the refusal test fail when the retry guard is removed
This test uses the runtime fixture, which never runs a restore, so _restore_config is None. Line 185 then refuses the retry because config is None, and the test passes also without the state check. Every real startup that gets to the restore sets _restore_config. Without the state check, a signed-in user can retry from ready and publish a new generation. A user can also retry from restart-required after an apply that failed before its restore. The retry then treats the saved entries that startup restored as still registered, builds nothing, and publishes ready without them. No test fails. Start this test with _start_with_recipes_async and a restorable recipe, and assert 409 and an unchanged generation. Also, at the end of test_a_retry_that_stops_partway_requires_a_restart_async, retry again and assert 409 and restart-required. This also applies if Retry later checks restore_failures instead of the state.
| } | ||
|
|
||
| describe('RuntimeBanner', () => { | ||
| beforeEach(() => { jest.clearAllMocks() }) |
There was a problem hiding this comment.
[P3] Restore real timers after each banner test
Eleven RuntimeBanner tests call jest.useFakeTimers() and call jest.useRealTimers() only on their last line. If an assertion fails first, fake timers stay on for the next tests. Then "retries and shows what still could not be restored" and "shows why a retry was refused" call userEvent.setup() without advanceTimers. user-event 14 waits on setTimeout(0) for each action, so each of these tests waits for the 15 s timeout and fails, which adds two failures that look unrelated to the first one. Add afterEach(() => { jest.useRealTimers() }) next to this beforeEach, as the RuntimeProvider block does at line 36, and remove the last-line calls.
|
|
||
| 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`. | ||
|
|
||
| The backend restores saved entries at startup and after a live apply, once the configured initializers have registered theirs. If any saved entry can't be restored, the backend stays not ready (state `failed`, outcome `restore-failed`) and lists each failure with its reason in `GET /api/config/runtime`, so a scan can't quietly run without an entry it expects. Every signed-in user can see the same list in `GET /api/restore-failures` and in the GUI's banner; users who aren't administrators get a short explanation of each reason instead of the full message. Fix or delete the listed files, then select Retry in the banner (`POST /api/restore-failures/retry`): it rebuilds only the entries that didn't come back, without a restart. Restarting also works. Files whose names aren't valid entry names (up to 64 lowercase letters, digits, and underscores, starting with a letter) are listed as ignored and don't block startup. |
There was a problem hiding this comment.
[P3] Say which list shows ignored files
This paragraph now names the GUI banner and GET /api/restore-failures just before it says that files with names that are not valid "are listed as ignored". Those two lists show only failures. Restore ignores a file such as targets/My-Target.json without a failure, so the backend becomes ready, no banner shows, and GET /api/restore-failures returns no items. Only GET /api/config/runtime lists the file (restore_ignored), only administrators can read it, and the log does not name the file. A reader whose saved entry did not come back looks in the new lists and finds nothing. Name the list in that sentence, for example "...don't block startup; administrators can see them under restore_ignored in GET /api/config/runtime." Because GET /api/restore-failures already gives administrators the full message, you can also remove the first mention of GET /api/config/runtime to make the paragraph shorter.
Description
This is one of the changes for saving Registry entries across restarts. It builds on #3909. Until the earlier PRs merge, the diff here also shows their changes.
Problem. When saved entries can't be restored, the runtime stays not ready, and getting it back takes an administrator and a restart. Only administrators can see why (
GET /api/config/runtime); everyone else sees a banner that sends them to Configuration, which they can't open. Live apply can't help either: it's refused in this state, it's off in our deployments, and it would rebuild everything.Change.
GET /api/restore-failureslists the saved entries that couldn't be restored: kind, name, reason, document version, and the entry it uses that is missing or broken. It needs sign-in like other routes, but not admin rights, and the runtime gate lets it through while the runtime isn't ready, as it does/api/runtime. Administrators get each failure's full message. Everyone else gets a short, fixed explanation of the reason instead, since a constructor's error can quote a server setting.POST /api/restore-failures/retryrebuilds the saved entries that weren't restored, in the same order and with the same checks as at startup. Entries from the startup scripts and saved entries that already came back stay as they are (same objects), so nothing that uses them changes. When nothing fails, the runtime publishes the configuration it had loaded and becomes ready, as startup would have. When something still fails, it stays not ready with the new list and keeps what came back.RegistryRecipeRestorer.restoretakes the entries that already came back and leaves them alone.What stays the same.
GET /api/config/runtime(admin only) still lists the failures with full messages, plus the ignored files./api/runtimeand/api/healthkeep their shape. Startup and live apply restore as before (registries start from the startup entries), and apply is still refused while a restore is failing. A restart still recovers.Next. Letting an entry's owner or an admin delete a broken entry from the app comes in a separate PR.
Tests and Documentation
tests/unit/backend/test_runtime_lifecycle.py:/api/runtimeand/api/healthkeep their shape.tests/unit/registry/test_registry_recipe_restore.py: a retry builds only the entries that didn't come back, including a pool that uses an entry that did, which stays the same object; a restored entry whose file changed afterward is left alone.frontend/src/hooks/useRuntime.test.tsx: the banner lists the failures in this state and doesn't ask for them in others, says when the list couldn't be loaded and tries again (keeping the last list, and ignoring a request that fails after a retry started), lets a long list scroll on its own, and keeps it current while the runtime stays failed. Retry shows what still couldn't be restored and checks readiness right away; list refreshes pause while a retry runs, and a list requested before it doesn't overwrite its result; a refused retry shows why, that message goes away once the runtime recovers, and a retry answer that arrives after the runtime recovered is dropped, also when the polls missed the recovery and only saw a new generation.pytest tests/unit/backend tests/unit/setup tests/unit/registry tests/unit/models -n 4: 8,072 passed, 9 skipped, 4 xfailednpx jest src/hooks/useRuntime.test.tsx: 17 passed.npx tsc --noEmit: passed. ESLint on the changed files: passed.pre-commit run --from-ref origin/main --to-ref HEAD: passedGET /api/restore-failureslisted both with explanations, an admin saw the full messages, and/api/targetsreturned 503. A retry before fixing the files kept the runtime not ready; after fixing them, a retry reached ready,GET /api/targetsreturned the three restored targets, and another retry returned 409.doc/getting_started/pyrit_conf.md. JupyText: not run (no notebooks changed).