Manifest discovery aborts when a nested directory becomes unavailable

[bobtista]

Description

Recursive manifest discovery can abort when a nested directory is inaccessible or removed during enumeration. The exception occurs outside the per-file load handler, preventing manifests in accessible sibling directories from being returned.

Expected behavior

Discovery should skip unavailable directories, log the failure, and continue scanning accessible locations without suppressing cancellation.

Acceptance criteria

• Continue after UnauthorizedAccessException and filesystem IOException failures for individual directories.
• Discover manifests in accessible sibling directories.
• Preserve cancellation behavior.
• Apply consistent traversal behavior to both filesystem discovery paths.
• Add deterministic filesystem tests for inaccessible and concurrently removed directories.
• Avoid unrelated manifest parsing or cache refactoring.

[coderabbitai]

🔗 Related PRs

#305 - fix(manifest): use JSON file pattern during discovery [open]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[coderabbitai]

Coding Plan

Summary

• Create one reusable resilient directory-walk helper that manually recurses and isolates failures to individual directories, replacing the lazy SearchOption.AllDirectories enumerator whose exceptions escaped the per-file handler.
• Route both ManifestDiscoveryService discovery methods through this helper for consistent traversal, fixing the incidental *.json pattern literal-string bug while preserving all signatures, return contracts, and cancellation.
• Guarantee cancellation is honored by checking the token per directory and catching OperationCanceledException separately from recoverable IO errors.
• Add deterministic, cross-platform tests using an injectable enumeration seam on the helper to simulate inaccessible and concurrently-removed directories, plus discovery-level tests proving sibling directories are still returned.

File-Level Change Summary & Follow-up Notes

• New: GenHub/GenHub/Infrastructure/FileSystem/ResilientDirectoryEnumerator.cs (or similar shared location) — manual-recursion safe walker with per-directory try/catch, cancellation support, and optional enumeration delegates for testing.
• Modified: GenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs — both DiscoverManifestsAsync and DiscoverFileSystemManifestsAsync route through the new walker; fix the FileTypes.JsonFilePattern literal-string bug; preserve signatures, return types, per-file handling, and token flow.
• Unchanged (verified, no edits): FileSystemDiscoverer.cs and ManifestInitializationService.cs — their existing outer try/catch behavior is intentionally kept as safety nets.
• New/Modified tests: new helper test class under GenHub.Tests.Core/Infrastructure/FileSystem/; extended ManifestDiscoveryServiceTests.cs.

Follow-up agent guidance:

• Do not introduce a broad IFileSystem abstraction or refactor manifest parsing/caching — scope is strictly directory-traversal resilience.
• Ensure cancellation is checked/caught before the generic IO catch blocks so OperationCanceledException is never suppressed.
• Match existing logging conventions: ILogger.LogWarning(ex, "…{Directory}…", directory) for skipped directories; reserve LogError/re-throw for truly unexpected failures.
• Consider (optional, only if trivial) reusing the new walker at other unguarded SearchOption.AllDirectories call sites noted during exploration, but this is out of scope for this ticket and should not be bundled in.

Design Choices

Design Choice 1: Which code constitutes the "both filesystem discovery paths" to be made consistent?

Options Considered:

1. The two methods within ManifestDiscoveryService — DiscoverManifestsAsync and DiscoverFileSystemManifestsAsync — which share the exact same SearchOption.AllDirectories fragility described by the ticket ("exception occurs outside the per-file load handler").
2. ManifestDiscoveryService plus the separate manual-recursion scanner in ManifestGenerationService/ContentManifestBuilder.

Chosen Option: 1

Rationale: The ticket's failure description (lazy enumeration throwing outside the per-file handler) precisely matches the two ManifestDiscoveryService methods; ManifestGenerationService already uses guarded manual recursion and is a different concern (manifest generation, not discovery), so pulling it in would be scope creep.

Design Choice 2: How should deterministic tests simulate inaccessible / concurrently-removed directories?

Options Considered:

1. Real OS permission manipulation (Linux UnixFileMode.None, Windows ACLs) — platform-specific and not deterministic cross-platform.
2. Give the new resilient-walk helper optional injectable enumeration delegates (defaulting to Directory.GetFiles/Directory.GetDirectories), so tests can inject delegates that throw UnauthorizedAccessException/IOException for a chosen path or delete a directory mid-walk, while production uses the real defaults.

Chosen Option: 2

Rationale: A thin delegate seam on the new helper yields fully deterministic, cross-platform tests without a broad filesystem abstraction, aligns with the codebase's preference for injectable seams over ACL/chmod manipulation, and keeps the change scoped to the new utility.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Introduce a resilient recursive directory-walk utility

Create a single reusable helper that performs manual top-down recursion and treats each directory independently, so that a failure in one subtree cannot abort the traversal of its siblings or of subsequent root directories. This helper becomes the shared foundation both discovery paths build on, replacing the all-or-nothing SearchOption.AllDirectories enumerator.

Create the resilient directory enumerator helper

Add a new utility that yields matching file paths across a directory tree while gracefully skipping unavailable directories.

• Create a new static helper (e.g. ResilientDirectoryEnumerator) in a shared infrastructure/utility location under GenHub/GenHub/ (e.g. Infrastructure/FileSystem/), not inside the Manifest feature, since the intent is a reusable safe walker.
• Implement manual recursion: for the current directory, list its files (top-directory-only) and then its subdirectories (top-directory-only), recursing into each subdirectory. Do not use SearchOption.AllDirectories.
• Wrap each GetFiles and GetDirectories call (and each recursive descent) in its own try/catch that handles UnauthorizedAccessException, IOException (covering DirectoryNotFoundException), and PathTooLongException; on catch, log a warning through the caller-supplied ILogger identifying the skipped directory, then continue with siblings rather than propagating.
• Accept a CancellationToken and call ThrowIfCancellationRequested() at each directory level so OperationCanceledException propagates unhindered and is never swallowed by the general catch blocks (order the cancellation check/catch so cancellation is not treated as a recoverable IO failure).
• Expose optional enumeration delegates (defaulting to Directory.GetFiles / Directory.GetDirectories) purely to provide a deterministic test seam; production callers rely on the defaults.
• Return the discovered file paths as an aggregated collection (or yield them) so callers retain their existing per-file processing loops unchanged.

🤖 Prompt for AI agents

Implement a new reusable resilient directory-walk utility that replaces the
fragile `SearchOption.AllDirectories` enumeration pattern. Goal: isolate
failures to individual directories so sibling directories continue to be
traversed.

- Create a new static helper class (e.g. `ResilientDirectoryEnumerator`) in a
shared infrastructure/utility location under `GenHub/GenHub/` (e.g.
`Infrastructure/FileSystem/`), not inside the Manifest feature.
- Implement manual top-down recursion: for the current directory, list its files
(top-directory-only) then its subdirectories (top-directory-only), recursing
into each subdirectory. Do NOT use `SearchOption.AllDirectories`.
- Wrap each `GetFiles` and `GetDirectories` call (and each recursive descent) in
its own try/catch handling `UnauthorizedAccessException`, `IOException`
(covering `DirectoryNotFoundException`), and `PathTooLongException`. On catch,
log a warning via the caller-supplied `ILogger` identifying the skipped
directory, then continue with siblings instead of propagating.
- Accept a `CancellationToken` and call `ThrowIfCancellationRequested()` at each
directory level so `OperationCanceledException` propagates unhindered; ensure
the cancellation check/catch ordering means cancellation is never swallowed as a
recoverable IO failure.
- Expose optional enumeration delegates (defaulting to `Directory.GetFiles` /
`Directory.GetDirectories`) solely to provide a deterministic test seam;
production callers should rely on the defaults.
- Return the discovered file paths as an aggregated collection (or yield them)
so existing callers can keep their per-file processing loops unchanged.

Phase 2: Route both discovery paths through the resilient walker

Update ManifestDiscoveryService so both discovery methods use the new helper, giving them identical, resilient traversal semantics while keeping their public signatures, return types, and per-file error handling intact.

Task 1: Make `DiscoverManifestsAsync` resilient

Replace the fragile enumeration in the public discovery path.

• In GenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs, replace the Directory.EnumerateFiles(directory, ..., SearchOption.AllDirectories) call with a call to the new resilient enumerator, passing the injected _logger and the cancellationToken.
• Fix the existing literal-string defect: use the FileTypes.JsonFilePattern constant instead of the literal string "FileTypes.JsonFilePattern" currently passed as the search pattern.
• Keep the existing per-file try/catch (log-warning-and-continue) that wraps LoadManifestAsync, so individual bad manifest files are still skipped independently.
• Preserve the method signature and Task<Dictionary<string, ContentManifest>> return type so FileSystemDiscoverer is unaffected; partial results now flow back as a populated dictionary instead of a bubbled exception.

Task 2: Make `DiscoverFileSystemManifestsAsync` resilient

Apply the same treatment to the cache-initialization discovery path.

• In the same file, replace both Directory.EnumerateFiles(..., SearchOption.AllDirectories) calls used to build the manifest/JSON file set with the resilient enumerator, preserving the existing filter that excludes .manifest.json files from the plain *.json pass.
• Retain the per-file try/catch around File.OpenRead/deserialization so a single unreadable file is skipped, not fatal.
• Keep threading the existing CancellationToken through so cancellation during cache initialization still propagates out to ManifestInitializationService.
• Leave ManifestInitializationService.StartAsync's try/catch { ...; throw; } unchanged; it remains a safety net for genuinely unexpected/fatal errors but will no longer be triggered by a single inaccessible directory.

🤖 Prompt for AI agents

Update `GenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs` to route
both filesystem discovery methods through the new
`ResilientDirectoryEnumerator`, giving them identical resilient traversal
semantics while preserving signatures, return types, and per-file error
handling.

For `DiscoverManifestsAsync`:
- Replace the `Directory.EnumerateFiles(directory, ...,
SearchOption.AllDirectories)` call with a call to the new resilient enumerator,
passing the injected `_logger` and the `cancellationToken`.
- Fix the literal-string defect: use the `FileTypes.JsonFilePattern` constant
instead of the literal string `"FileTypes.JsonFilePattern"` currently passed as
the search pattern.
- Keep the existing per-file try/catch (log-warning-and-continue) around
`LoadManifestAsync`, so bad manifest files are still skipped individually.
- Preserve the method signature and `Task<Dictionary<string, ContentManifest>>`
return type so `FileSystemDiscoverer` is unaffected; partial results should now
flow back as a populated dictionary instead of a bubbled exception.

For `DiscoverFileSystemManifestsAsync`:
- Replace both `Directory.EnumerateFiles(..., SearchOption.AllDirectories)`
calls used to build the manifest/JSON file set with the resilient enumerator,
preserving the existing filter that excludes `.manifest.json` files from the
plain `*.json` pass.
- Retain the per-file try/catch around `File.OpenRead`/deserialization so a
single unreadable file is skipped, not fatal.
- Keep threading the existing `CancellationToken` through so cancellation during
cache initialization still propagates to `ManifestInitializationService`.
- Do not modify `ManifestInitializationService.StartAsync`'s existing `try/catch
{ ...; throw; }`; it remains a safety net for genuinely unexpected/fatal errors.

Phase 3: Deterministic filesystem tests

Add tests proving that inaccessible and concurrently-removed directories are skipped while sibling directories are still discovered, and that cancellation is preserved — using the helper's injectable enumeration seam for determinism.

Task 1: Test the resilient walker in isolation

Add focused unit tests for the new helper covering the failure modes.

• Add a new test class (e.g. under GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/FileSystem/) using the established GUID temp-directory + IDisposable teardown idiom.
• Inaccessible-directory test: build a real tree with multiple sibling subdirectories, inject an enumeration delegate that throws UnauthorizedAccessException for one specific subdirectory, and assert that files under the healthy siblings are still returned and that a warning was logged (verify via Mock<ILogger>).
• Concurrently-removed-directory test: inject a delegate that deletes (or throws IOException/DirectoryNotFoundException for) a subdirectory at the moment the walker descends into it, and assert traversal continues over the remaining directories.
• Cancellation test: pass an already-cancelled (or mid-walk-cancelled) token and assert OperationCanceledException propagates and is not swallowed or logged as a recoverable IO error.

Task 2: Test resilient behavior through `ManifestDiscoveryService`

Add discovery-level tests confirming end-to-end resilience across sibling search directories.

• Extend GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Manifest/ManifestDiscoveryServiceTests.cs (currently only pure-logic tests) with tests that exercise DiscoverManifestsAsync against multiple real temp search directories where one directory's enumeration fails and another contains valid manifest files.
• Assert the returned dictionary contains manifests from the accessible directory and that discovery did not throw, confirming sibling-directory resilience and the corrected *.json search pattern.
• Reuse the existing Mock<ILogger> / Mock<IManifestCache> setup already present in the test class and the temp-directory fixture pattern from ContentManifestPoolTests.

🤖 Prompt for AI agents

Add deterministic, cross-platform tests validating the new resilient traversal
behavior, using the helper's injectable enumeration seam.

For the resilient walker itself:
- Add a new test class under
`GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/FileSystem/` using the
established GUID temp-directory + `IDisposable` teardown idiom.
- Inaccessible-directory test: build a real tree with multiple sibling
subdirectories, inject an enumeration delegate that throws
`UnauthorizedAccessException` for one specific subdirectory, and assert files
under the healthy siblings are still returned and a warning was logged (verify
via `Mock<ILogger>`).
- Concurrently-removed-directory test: inject a delegate that deletes (or throws
`IOException`/`DirectoryNotFoundException` for) a subdirectory at the moment the
walker descends into it, and assert traversal continues over remaining
directories.
- Cancellation test: pass an already-cancelled (or mid-walk-cancelled) token and
assert `OperationCanceledException` propagates and is not swallowed or logged as
a recoverable IO error.

For `ManifestDiscoveryService`:
- Extend
`GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Manifest/ManifestDiscoveryServiceTests.cs`
with tests exercising `DiscoverManifestsAsync` against multiple real temp search
directories, where one directory's enumeration fails and another contains valid
manifest files.
- Assert the returned dictionary contains manifests from the accessible
directory and that discovery did not throw, confirming sibling-directory
resilience and the corrected `*.json` search pattern.
- Reuse the existing `Mock<ILogger>` / `Mock<IManifestCache>` setup already in
the test class and the temp-directory fixture pattern from
`ContentManifestPoolTests`.

Research

ManifestDiscoveryService (GenHub/GenHub/Features/Manifest/) is a concrete, DI-scoped class (no interface) with two filesystem discovery paths: the public DiscoverManifestsAsync (returns Dictionary<string, ContentManifest>, called by FileSystemDiscoverer) and the private DiscoverFileSystemManifestsAsync (populates IManifestCache, reached via InitializeCacheAsync from the ManifestInitializationService hosted service). Both use the identical fragile pattern: an outer loop over search directories and an inner Directory.EnumerateFiles(dir, pattern, SearchOption.AllDirectories) loop, where only the per-file load is wrapped in try/catch. There is no reusable resilient directory-walk helper anywhere in the codebase. Tests use xUnit + Moq + FluentAssertions against the real filesystem via GUID-named temp directories with IDisposable teardown; there is no filesystem abstraction and no established deterministic pattern for simulating permission/IO failures.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Implement a new reusable resilient directory-walk utility that replaces the
fragile `SearchOption.AllDirectories` enumeration pattern. Goal: isolate
failures to individual directories so sibling directories continue to be
traversed.

- Create a new static helper class (e.g. `ResilientDirectoryEnumerator`) in a
shared infrastructure/utility location under `GenHub/GenHub/` (e.g.
`Infrastructure/FileSystem/`), not inside the Manifest feature.
- Implement manual top-down recursion: for the current directory, list its files
(top-directory-only) then its subdirectories (top-directory-only), recursing
into each subdirectory. Do NOT use `SearchOption.AllDirectories`.
- Wrap each `GetFiles` and `GetDirectories` call (and each recursive descent) in
its own try/catch handling `UnauthorizedAccessException`, `IOException`
(covering `DirectoryNotFoundException`), and `PathTooLongException`. On catch,
log a warning via the caller-supplied `ILogger` identifying the skipped
directory, then continue with siblings instead of propagating.
- Accept a `CancellationToken` and call `ThrowIfCancellationRequested()` at each
directory level so `OperationCanceledException` propagates unhindered; ensure
the cancellation check/catch ordering means cancellation is never swallowed as a
recoverable IO failure.
- Expose optional enumeration delegates (defaulting to `Directory.GetFiles` /
`Directory.GetDirectories`) solely to provide a deterministic test seam;
production callers should rely on the defaults.
- Return the discovered file paths as an aggregated collection (or yield them)
so existing callers can keep their per-file processing loops unchanged.
===============================================================================

Task: 2

Update `GenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs` to route
both filesystem discovery methods through the new
`ResilientDirectoryEnumerator`, giving them identical resilient traversal
semantics while preserving signatures, return types, and per-file error
handling.

For `DiscoverManifestsAsync`:
- Replace the `Directory.EnumerateFiles(directory, ...,
SearchOption.AllDirectories)` call with a call to the new resilient enumerator,
passing the injected `_logger` and the `cancellationToken`.
- Fix the literal-string defect: use the `FileTypes.JsonFilePattern` constant
instead of the literal string `"FileTypes.JsonFilePattern"` currently passed as
the search pattern.
- Keep the existing per-file try/catch (log-warning-and-continue) around
`LoadManifestAsync`, so bad manifest files are still skipped individually.
- Preserve the method signature and `Task<Dictionary<string, ContentManifest>>`
return type so `FileSystemDiscoverer` is unaffected; partial results should now
flow back as a populated dictionary instead of a bubbled exception.

For `DiscoverFileSystemManifestsAsync`:
- Replace both `Directory.EnumerateFiles(..., SearchOption.AllDirectories)`
calls used to build the manifest/JSON file set with the resilient enumerator,
preserving the existing filter that excludes `.manifest.json` files from the
plain `*.json` pass.
- Retain the per-file try/catch around `File.OpenRead`/deserialization so a
single unreadable file is skipped, not fatal.
- Keep threading the existing `CancellationToken` through so cancellation during
cache initialization still propagates to `ManifestInitializationService`.
- Do not modify `ManifestInitializationService.StartAsync`'s existing `try/catch
{ ...; throw; }`; it remains a safety net for genuinely unexpected/fatal errors.
===============================================================================

Task: 3

Add deterministic, cross-platform tests validating the new resilient traversal
behavior, using the helper's injectable enumeration seam.

For the resilient walker itself:
- Add a new test class under
`GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/FileSystem/` using the
established GUID temp-directory + `IDisposable` teardown idiom.
- Inaccessible-directory test: build a real tree with multiple sibling
subdirectories, inject an enumeration delegate that throws
`UnauthorizedAccessException` for one specific subdirectory, and assert files
under the healthy siblings are still returned and a warning was logged (verify
via `Mock<ILogger>`).
- Concurrently-removed-directory test: inject a delegate that deletes (or throws
`IOException`/`DirectoryNotFoundException` for) a subdirectory at the moment the
walker descends into it, and assert traversal continues over remaining
directories.
- Cancellation test: pass an already-cancelled (or mid-walk-cancelled) token and
assert `OperationCanceledException` propagates and is not swallowed or logged as
a recoverable IO error.

For `ManifestDiscoveryService`:
- Extend
`GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Manifest/ManifestDiscoveryServiceTests.cs`
with tests exercising `DiscoverManifestsAsync` against multiple real temp search
directories, where one directory's enumeration fails and another contains valid
manifest files.
- Assert the returned dictionary contains manifests from the accessible
directory and that discovery did not throw, confirming sibling-directory
resilience and the corrected `*.json` search pattern.
- Reuse the existing `Mock<ILogger>` / `Mock<IManifestCache>` setup already in
the test class and the temp-directory fixture pattern from
`ContentManifestPoolTests`.

💡 Iterate on the plan with: `@coderabbitai` <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!