Repository navigation
FEAT: Report the documents a storage listing cannot use (part 1) - #3891
varunj-msft wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A file-replacement race can cause reads to block indefinitely on a FIFO, and deleted symlinks are incorrectly reported as unreadable rather than skipped.
2 open issues
What changed in this PR
Adds issue-aware document listings to support future restoration of saved registry entries, while leaving existing listing APIs and their callers unchanged.
Changes:
- Returns readable documents alongside unreadable and ignored entries for local and Azure Blob storage.
- Surfaces listing failures and provides concise read-error descriptions.
- Adds tests for naming rules, storage failures, non-regular files, deletion races, and error-message privacy.
| File | Description |
|---|---|
tests/unit/registry/test_file_document_storage.py |
Tests local and Blob listings, failure handling, and compatibility. |
pyrit/registry/file_document_storage.py |
Adds structured listing results and issue-aware storage methods. |
Review effort: Balanced
…s that can't be checked
varunj-msft
changed the title
Richard Lundeen (richlundeen)
left a comment
Richard Lundeen (richlundeen)
left a comment
There was a problem hiding this comment.
Requesting changes. The direction is right, and the fixes for the pipe hang and the unreadable folder are good. Inline comments marked Blocking need to be addressed before merge; the ones marked Non-blocking are judgment calls. Several of the blocking ones matter most for the restore stack built on this (#3892 through #4071), which inherits this listing's classifications and messages.
One more blocking item has no code line to attach to: please describe the two behavior changes in the PR description and release notes.
- With custom initializers enabled, an unreadable local folder now stops startup (the runtime goes to "restart required"). Before, the backend started without custom initializers.
- The preset, suite and custom initializer list routes now return 503 for an unreadable local folder. Before, they returned 200 with no stored entries.
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class UnreadableDocument: |
There was a problem hiding this comment.
Blocking: keep the original error on this report.
UnreadableDocument keeps only a name and a short message. The restore stack turns each one into a restore failure, and with only a string, nothing downstream can log the cause or tell a temporary failure (where Retry is the right action) from a broken file (where Delete is).
Suggested fix: add error: BaseException | None = field(default=None, compare=False, repr=False) and set it wherever a report is created (_read_local_entry, _describe_non_file and the Blob loop). Keep message as the safe text for display.
| continue | ||
| try: | ||
| documents[name] = client.download_blob(blob_name).readall() | ||
| except AzureError as error: |
There was a problem hiding this comment.
Blocking: raise on Blob failures that affect the whole store.
Every download error except a deleted blob becomes a per-file report here. So a list-only SAS, an auth failure, a network error after retries, or a deleted container marks every file unreadable: _list_documents returns {}, the preset, suite and initializer lists answer 200 without any stored entries, and restore reports one failure per file. A local folder that can't be read raises instead.
Suggested fix: re-raise ClientAuthenticationError, any 401 or 403, ContainerNotFound, and transport errors (ServiceRequestError, ServiceResponseError). Keep per-file reports for errors specific to one blob. test_skips_only_a_blob_deleted_after_listing (test_file_document_storage.py:307-330) should then expect a raise for the missing container.
| return str(error) | ||
| return None | ||
|
|
||
| def _read_document_bytes(self, name: str) -> bytes | None: |
There was a problem hiding this comment.
Blocking: make the single-file read agree with the listing.
This read is unchanged: path.read_bytes() if path.is_file() else None. It treats a broken link, a link loop, a folder or a pipe named like a document as "doesn't exist", while the new listing reports the same entries as unreadable. It can also still hang if a file is replaced by a pipe between is_file() and read_bytes(). This matters for the stack: #4071's delete reads through here, so it returns 404 for those entries even though the listing reports them as unreadable and they keep the backend from being ready. On Blob, any 404 here, including a missing container, counts as "doesn't exist".
Suggested fix: read local files with _read_regular_file, return None only when os.lstat raises FileNotFoundError, and raise a dedicated error carrying the safe message for anything else (a non-regular file or another OSError). On Blob, treat only BlobNotFound as missing.
|
|
||
|
|
||
| def _storage_unavailable() -> HTTPException: | ||
| def _storage_unavailable(error: Exception) -> HTTPException: |
There was a problem hiding this comment.
Blocking: one storage error and one handler instead of per-route mapping.
This helper is copied three times (here, scenario_suites.py and initializers.py), and (AzureError, OSError) is caught only on the list routes and initializer delete. Every other route that touches the same storage still falls through to the global handlers:
PermissionErrorreturns a 403 whose body includes the server path (error_handlers.py:148-163).FileNotFoundErrorreturns a 404 with the path (error_handlers.py:124-139).- A lock
TimeoutErrorreturns a 500. - Register wraps a write error in a
ValueError(initializer_registry.py:436), which initializers.py:222 returns as a 400 with the path.
Catching OSError in a route also catches lock timeouts, and any authorization PermissionError a service might raise.
Suggested fix: have FileDocumentStorage raise one DocumentStorageUnavailableError carrying display_source and the describe_storage_error reason, chained from the original error. Map it to 503 in a single exception handler that also logs. Then delete the three helpers and the (AzureError, OSError) clauses, and stop wrapping write errors in ValueError in register_from_content.
| Returns: | ||
| HTTPException: A service-unavailable response without SDK details. | ||
| """ | ||
| logger.warning(f"Scenario preset storage is unavailable: {describe_storage_error(error)}") |
There was a problem hiding this comment.
Blocking: log which storage failed.
This logs only the short reason, such as Scenario preset storage is unavailable: Permission denied or ...: ServiceRequestError, with no location and no traceback, and the route tests assert the folder path is not in the log. I know this came out of an earlier review thread, but _list_documents in this same PR logs the path through display_source (file_document_storage.py:277-284), and scenario_suite_service.py:212-216 on main logs the same errors with a full traceback. Only the SAS token needs hiding, and display_source already redacts it.
Suggested fix: sanitize only the HTTP response. Log the redacted source and the reason, with the traceback at debug level. Drop the "not in caplog.text" path assertions in the three route tests, and write the rule into FileDocumentStorage's docstring so the rest of the stack follows it.
| dict[str, bytes]: Document content keyed by name. | ||
| """ | ||
| return self._list_blob_documents() if self._is_blob else self._list_local_documents() | ||
| listing = self._list_documents_with_issues() |
There was a problem hiding this comment.
Non-blocking: strict initializer startup.
register_stored_initializers(strict=True) reads through list_scripts() and this method, so in strict mode a syntax error in a script stops startup, but an unreadable or badly named script only logs a warning. Consider failing strict startup on listing.unreadable as well. It's consistent with how invalid scripts are treated, but it adds a new way for startup to fail, so I'm fine either way.
| unreadable.append(content) | ||
| return DocumentListing(documents=documents, unreadable=tuple(unreadable), ignored=tuple(ignored)) | ||
|
|
||
| def _read_local_entry(self, *, entry: os.DirEntry[str], name: str) -> bytes | UnreadableDocument | None: |
There was a problem hiding this comment.
Non-blocking: the three-way return.
Returning bytes | UnreadableDocument | None and sorting it out with isinstance in the caller works, but it's easy to break in a later edit (an empty file has to stay b"", not become None). A small result type, or raising for the unreadable case, would read more clearly. Style call.
| # ``fnmatch`` applies the platform's case rule, as ``glob`` does. | ||
| with os.scandir(directory) as entries: | ||
| candidates = sorted( | ||
| (entry for entry in entries if fnmatch.fnmatch(entry.name, f"*{self._extension}")), |
There was a problem hiding this comment.
Non-blocking: extension matching differs between backends.
Locally, fnmatch ignores case on Windows, so notes.JSON is listed; on Blob, _is_direct_document_blob compares the suffix exactly, so the same file is invisible. A file named just .json also shows up locally (as ignored) but not on Blob. This predates the PR, but matching the same way on both backends would keep the "report every file" promise. Which rule to pick is a judgment call.
| logger.warning(f"Scenario preset storage is unavailable: {describe_storage_error(error)}") | ||
| return HTTPException( | ||
| status_code=status.HTTP_503_SERVICE_UNAVAILABLE, | ||
| detail="Scenario preset storage is temporarily unavailable", |
There was a problem hiding this comment.
Non-blocking: wording. A permanent permission problem is answered with "temporarily unavailable". It matches the existing Blob behavior, so fine to leave, but "Scenario preset storage is unavailable" (and the same for suites and initializers) would be accurate in both cases.
| Returns: | ||
| bytes | None: The content, or None if the path names something other than a regular file. | ||
| """ | ||
| descriptor = os.open(path, _READ_FLAGS) |
There was a problem hiding this comment.
Non-blocking: Windows. os.open on a folder raises PermissionError on Windows, so a folder swapped in after listing is reported as "Permission denied" rather than "Not a regular file". Not worth extra code; a short comment here noting it would do.
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.
Description
This is one of the changes for saving Registry entries across restarts. It's the first of the stacked PRs and doesn't depend on the others.
Problem.
FileDocumentStorage._list_documentsskips a document it can't read, and a file whose name breaks the naming rule, with only a log line. Restoring saved registry entries has to know about every file: an entry that can't be read must be reported, not quietly dropped. The old listing also has two bugs that presets, suites and custom initializers hit today: a pipe named like a document hangs it, andPath.globreturns nothing for a folder it can't read, so an unreadable folder looks empty.Change. Add
_list_documents_with_issues(), which returns the readable documents plus:A folder or container that can't be listed raises. A document deleted between listing and reading is skipped, link or not, but on Blob only for a real blob delete (
BlobNotFound), not a missing container. Locally, each document is opened without blocking and the opened file is checked before it is read, so a pipe, folder or broken link put in place of a listed document is reported, not read. An entry that can't be checked at all is reported too. The local listing usesos.scandir, which raises on an unreadable folder, withfnmatchfor the same case rule asglob._list_documentsnow uses the same listing and logs what it leaves out, as hannahwestra25 suggested, so the old local and Blob helpers are gone. For presets, suites and custom initializers, a pipe is now skipped with a warning instead of hanging, and an unreadable local folder raises, as an unreadable Blob container already did. Their list routes, and custom initializer delete, report that as unavailable storage (503), as they already did for Blob, where main returned an empty list (and 404 for delete). With custom initializers enabled, startup stops on it, as it does for a container that can't be listed. The warnings carry the short reason rather than the raw error, which on Blob can include the request URL.What stays the same. Presets, suites and custom initializers still get every document they can read, and still log a warning for each one they skip. Reading or saving a single preset, suite or initializer doesn't list the folder, so how those report a local error is unchanged. hannahwestra25's #3656 applies cleanly on top.
Tests and Documentation
tests/unit/registry/test_file_document_storage.py(local and Blob): an unreadable document is reported and the others are kept; badly named files are listed as ignored without being read; links that point nowhere or loop, and a folder named like a document, are reported without being read; a document or link deleted between listing and reading is skipped, as is a deleted blob (only for a real blob delete); a pipe, folder or broken link swapped in for a listed document is reported without blocking or leaking a file handle; the opened file is checked rather than the path, whether it's replaced just before or just after the open; an entry that can't be checked is reported; a folder or container that can't be listed raises; descriptions carry no URL, only the Azure status and error code._list_documentslogs what it leaves out, skips a pipe without blocking, raises for a folder it can't list, and logs an unreadable blob without its SAS token. Each of those four fails against the old listing (the pipe test by its timeout).tests/unit/backend/test_scenario_preset_routes.py,test_scenario_suite_routes.pyandtest_initializer_service.py: a folder that can't be read is reported as unavailable storage (503), without its path, by the preset, suite and custom initializer list routes and by initializer delete. These fail against the old routes.uv run --frozen --extra all python -m pytest tests/unit/registry tests/unit/backend tests/unit/setup -n 8 --dist=loadfile: 4909 passed, 4 skippedpre-commit run --from-ref origin/main --to-ref HEAD: passed