Repository navigation
FEAT: Report the documents a storage listing cannot use (part 1) - #3891
varunj-msft wants to merge 21 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
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.
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.
# Conflicts: # pyrit/backend/middleware/error_handlers.py # pyrit/backend/routes/scenario_presets.py # pyrit/backend/routes/scenario_suites.py # pyrit/registry/file_document_storage.py
…y the other storage tests do
Its text can carry a SAS URL. The 503 handler no longer logs it as a debug traceback, and the scenario suite service no longer attaches it to its warning; both log the storage message, which has no credentials.


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. For the same reason, the 503 handler and the scenario suite service log only the storage message, never the error behind it as a traceback.Behavior changes (for the release notes).
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 is on
mainnow, and its version-checked delete follows the same rules: a lock that can't be taken or a container that can't be used is a 503 through the shared handler, and a folder or pipe named like a document is never read.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.tests/unit/backend/test_error_handlers.pyandtest_scenario_suite_service.py: a SAS URL in the error behind a storage failure stays out of the log, even at debug level.test_file_document_storage.py: a conditional blob write whose upload fails with a transport or authentication error reports unavailable storage, with the original cause and no SAS in the message.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