diff --git a/docs/roadmap/error-handling.md b/docs/roadmap/error-handling.md new file mode 100644 index 0000000..c095d1a --- /dev/null +++ b/docs/roadmap/error-handling.md @@ -0,0 +1,158 @@ +# Error-Handling Audit + +Reviewed revision: `5925066d35795b2dc1d31d069a1fb23024a62ab8`. +Audit date: 2026-10-25. + +## Scope And Architecture + +The audit traced source construction and discovery, host/native filesystem +acquisition, import selection, grouped composition, inheritance, typed decoding +and codecs, application callbacks, secrets bootstrap, diagnostics, inspection, +and rendering/diff failure paths. It reviewed error classification, propagation, +partial state, exceptional cleanup, and the root facade's conversions against +the [configuration contract](../config.md) and existing tests. + +The [implemented architecture](../internal/overview.md) separates public wrappers +from one synchronous private engine lifecycle. The document package provides the +owned tree; the YAML adapter isolates parser/dumper failures. Engine loading +collects safe structured diagnostics and returns a nil result on failure; the +facade translates engine load errors into root-owned `LoadError` values. Secrets +bootstrap is a separate operation with its own partial report and error type. +Published inspections read frozen state rather than reloading input. + +One new finding warrants remediation. Previously recorded exceptional-path +defects are cross-referenced below rather than duplicated as new work. + +## 1. Directory Read And Close Failures Can Be Misclassified As Absence + +Status: open. Priority: medium. Category: filesystem/selection error propagation. + +**Relevant code.** `filesystemAccess.readDirectory` in +[filesystem_access.go](../../internal/engine/filesystem_access.go), lines 144–184, +returns errors from opening a directory, reading its entries, and closing it +through the same unqualified error result. `visitBoundedImportDirectory` in +[directory_walk.go](../../internal/engine/directory_walk.go) forwards that error. +Both `importDirectoryEntries` in +[import_selection.go](../../internal/engine/import_selection.go), lines 295–306, +and `readGlobEntries` in [glob.go](../../internal/engine/glob.go), lines 187–198, +interpret any resulting `fs.ErrNotExist` as an absent directory and discard its +entries without a diagnostic. + +**Failure condition.** A directory has been successfully inspected and opened, +but its `ReadDir` fails with an error wrapping `fs.ErrNotExist`, possibly together +with entries already read. The same issue occurs if enumeration succeeds but +`Close` returns that classification. A caller-supplied `fs.FS` can legitimately +encounter such failures during its backing operations. The helper loses the +distinction between an absent target at lookup/open and a failure after opening. + +**Expected behavior.** Preserve the existing +[absence and requiredness rules](../config.md#explicit-imports) for genuinely +missing targets, while reporting a safe load failure when enumeration fails +after opening. A close failure that the directory helper elects to report must +not become an empty successful selection just because of its error identity. +This follows the existing distinction between absence and failed acquisition; +the regular-file helper in +[source_file.go](../../internal/engine/source_file.go) already restricts its +`missingTarget` flag to preflight/open failures rather than subsequent reads. + +**Actual behavior and consequence.** The failed directory is treated as though +it contributed no files. Optional loading can succeed with an incomplete +configuration. Required directory/glob selection can also succeed if another +directory supplies a match, since requiredness is checked against the surviving +combined selection. If it was the only directory, the outcome is either a +successful optional empty selection or a misleading `import.missing` error, +rather than the acquisition failure. + +**Evidence.** A standalone public-API diagnostic used a borrowed filesystem +backed by `fstest.MapFS` with these files: + +```text +main.yml +bad/b.yml # b: 2 +good/a.yml # a: 1 +``` + +The main input selected either `dir: [bad, good]` or `glob: '*/*.yml'`, with +`required: true`. The filesystem kept metadata and file opens working and +wrapped only the opened `bad` directory. Its wrapper returned an injected error +either alongside its `ReadDir` entries or from `Close` after enumeration. + +For both selectors and both failure phases, `fs.ErrNotExist` produced a +successful load with `a=1, b=0`. Replacing only the injected error with +`fs.ErrPermission` failed loading (`import.invalid` for directory search and +`import.permission` for glob search). All eight small cases were executed; no +repository test or production code was changed. The filesystem still contained +the omitted file, so this was not a missing-target fixture. + +**Potential remediation.** Preserve failure phase across the shared directory +boundary, for example with a small private result/failure record distinguishing +missing-on-open from read/close failure, analogous to `sourceReadFailure`. +Keep the existing preflight and open-time missing-directory behavior, entry +budgets, first-error precedence, and caller-owned filesystem lifetime. Map later +failures to the existing safe traversal diagnostics without exposing raw backing +error text. Review the standard `fs.ReadDir` fallback as well as the batched +`fs.ReadDirFile` path. No retry policy, filesystem transaction, rollback service, +or new restriction on supported filesystems is needed. + +## Existing Findings Relevant To Error Handling + +These remain open in their original owners; this audit creates no duplicate +remediation requirement: + +- [Correctness finding 1](correctness.md#1-pointer-only-recursive-types-can-overflow-the-stack-or-hang): + pointer-only recursive descriptors can turn incompatible input or a helper + query into stack exhaustion/nontermination instead of a safe failure. +- [Correctness finding 2](correctness.md#2-failed-dynamic-schema-construction-leaves-a-reusable-partial-descriptor): + failed dynamic descriptor construction leaves partial cache state; a later + helper in the same callback can panic instead of returning the recorded load + diagnostics. Error-state rollback belongs to that finding. +- [Correctness finding 4](correctness.md#4-callback-contexts-remain-active-after-panic-unwinding) + and [resource finding 3](resources.md#3-an-imported-version-check-panic-leaves-the-host-root-open): + panic unwinding bypasses callback deactivation and imported host-root cleanup. + Application panics should still propagate; neither finding requests recovery + or publication of partial results. +- [Resource findings 1 and 2](resources.md): oversized typed-container allocation + can panic before a quota diagnostic, and incomplete history accounting can + omit an established limit failure. + +## Deliberate Behaviors Preserved + +- Raw parser, reader, codec, filesystem, and returned callback error text is + intentionally suppressed at the loading boundary. Stable codes, safe messages, + semantic paths, and available locations provide context; adding raw error + chains would conflict with the [safe-failure contract](../config.md#safe-failures). +- Version/resolution callbacks that return an error without reporting one receive + a generic hook-failed diagnostic. Warnings alone do not suppress that failure. + Application callback panics remain programming failures. +- Read-only regular-file close errors are explicitly ignored by the shared + helper. Cleanup failures during an existing primary failure do not need to + replace that primary failure. These choices are not findings by themselves. +- Secrets bootstrap stages reads before writes and documents that an apply + failure leaves earlier writes in place, reports the failed entry, and leaves + later entries unapplied. That partial-state reporting is intentional, not a + missing rollback guarantee. +- Effective rendering, diagnostic rendering, explanation output, and bootstrap + reports propagate writer failures, including short writes; partial output is + permitted. Internal snapshot/diff serialization uses safe errors or redacted + fallback snapshots rather than exposing internal serialization details. +- ConfigKit owns no background cancellation, timeout, retry, or shutdown service. + Synchronous application readers/callbacks can block; this audit does not invent + a cancellation guarantee or move their resource ownership into ConfigKit. + +## Validation + +The following passed with Go 1.26.10 on Linux amd64, using cached dependencies +with network/toolchain acquisition disabled: + +```sh +go test ./... -count=1 +go vet ./... +go test ./internal/engine -run 'Failure|Failures|Error|Errors|Preflight|Propagates|HookOrder' -count=1 +go test ./internal/yaml -run 'Safe|Writer|Malformed' -count=1 +``` + +The standalone fault-injection diagnostic ran outside the repository. No +benchmarks, new repository tests, production changes, or native macOS/Windows +validation were performed. Passing existing tests does not cover the newly +identified phase-classification case. This report adds no public contract, +security hardening requirement, or release gate.