Skip to content

FEAT: Report the documents a storage listing cannot use (part 1) - #3891

Open
varunj-msft wants to merge 21 commits into
microsoft:mainfrom
varunj-msft:varunj-msft/13178-Unreadable-Document-Listing
Open

varunj-msft wants to merge 21 commits into
microsoft:mainfrom
varunj-msft:varunj-msft/13178-Unreadable-Document-Listing

Conversation

@varunj-msft

@varunj-msft varunj-msft commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Description

Part 1 of saving Registry entries across restarts. It's the bottom of the stack and sits on main; part 2 (#3892) builds on it.

Design: 4.2 Where recipes live. Where every part fits: PR map.

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_documents skips 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, and Path.glob returns 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:

  • unreadable documents, each with a short reason and no path or SAS token in it. That covers links that point nowhere or loop, and folders or pipes named like a document (never read, since reading a pipe blocks). It also covers Azure download errors, which carry the status and error code.
  • ignored files, whose names break the naming rule, are never read.

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 uses os.scandir, which raises on an unreadable folder, with fnmatch for the same case rule as glob.

_list_documents now 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).

  • With custom initializers enabled, a local initializer folder that can't be read now stops startup (the runtime asks for a restart), as a Blob container that can't be listed already did. Before, the backend started without custom initializers.
  • The preset, suite and custom initializer routes now return 503, without the server path, when their storage can't be read or written. Before, a local folder that couldn't be read gave 200 with no stored entries on the list routes (and 404 on initializer delete), and other storage errors gave 403, 404 or 500 with the path, or 400 when registering an initializer whose file couldn't be written.

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 main now, 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

  • New 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_documents logs 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.py and test_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.py and test_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 skipped
  • pre-commit run --from-ref origin/main --to-ref HEAD: passed
  • No doc change. JupyText: not run (no notebooks changed).

Copilot AI balanced review requested due to automatic review settings November 3, 2026 17:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread pyrit/registry/file_document_storage.py Resolved
Comment thread pyrit/registry/file_document_storage.py Resolved
Comment thread pyrit/registry/file_document_storage.py Resolved
@varunj-msft varunj-msft changed the title FEAT: Report the documents a storage listing cannot use FEAT: Report the documents a storage listing cannot use (part 1) Nov 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread pyrit/registry/file_document_storage.py Resolved
Comment thread pyrit/registry/file_document_storage.py Resolved
Comment thread pyrit/registry/file_document_storage.py Resolved
Comment thread pyrit/registry/file_document_storage.py Resolved
Comment thread pyrit/registry/file_document_storage.py Resolved
Comment thread pyrit/registry/file_document_storage.py Resolved
Comment thread pyrit/registry/file_document_storage.py Resolved
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
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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants