Record resource management audit findings
This commit is contained in:
189
docs/roadmap/resources.md
Normal file
189
docs/roadmap/resources.md
Normal file
@@ -0,0 +1,189 @@
|
||||
# Resource Management Audit
|
||||
|
||||
Reviewed revision: `0cca633a67abd3d0fdcb7db6e83260dcaf1eaac8`.
|
||||
Audit date: 2026-10-25.
|
||||
|
||||
## Scope And Architecture
|
||||
|
||||
The audit covered input snapshots and readers, discovery, filesystem acquisition,
|
||||
imports and selected overlays, typed projection and cloning, composition and
|
||||
inheritance, callback teardown, retained inspection history, rendering/diff, and
|
||||
secrets bootstrap. Knowledge-graph discovery and caller tracing were supplemented
|
||||
by source inspection, existing tests, and small standalone diagnostics outside
|
||||
the repository. No benchmarks or large-memory stress tests were conducted.
|
||||
|
||||
The [implemented architecture](../internal/overview.md) has a mostly clear
|
||||
resource-ownership model:
|
||||
|
||||
- The public facade delegates to a synchronous engine lifecycle, with private
|
||||
document and YAML packages. Loading creates no library-owned background
|
||||
goroutines, timers, network connections, or shutdown service.
|
||||
- Ordinary file reads close owned handles, including read/stat failures. Host
|
||||
import and overlay acquisition own their root handles; caller-supplied readers
|
||||
and filesystems remain borrowed.
|
||||
- Published inspection retains immutable documents, history, indexes, and passive
|
||||
resource contexts. A later explicit resource open transfers ownership of that
|
||||
returned file to the caller; publication does not normally retain acquisition
|
||||
handles.
|
||||
- Retained history and detached query results are intentional ownership choices,
|
||||
not leaks by themselves. The findings below concern enforcing existing limits
|
||||
and closing owned resources, rather than removing that functionality.
|
||||
|
||||
Three new findings warrant remediation. All remain open.
|
||||
|
||||
## 1. Typed Containers Allocate Before Their Node Budget Is Enforced
|
||||
|
||||
Priority: medium. Category: peak memory and failure handling.
|
||||
|
||||
**Relevant code.** `projectSequence` and `projectMap` in
|
||||
[projection.go](../../internal/engine/projection.go), `valueOperation.charge` in
|
||||
[value_operation.go](../../internal/engine/value_operation.go), and the defaults
|
||||
projection in [decode.go](../../internal/engine/decode.go).
|
||||
|
||||
**Circumstances.** Sequence projection reserves `value.Len()` document pointers
|
||||
before visiting any children. Map projection similarly reserves the complete
|
||||
entry array and obtains all map keys before charging individual entries. The
|
||||
existing node limit therefore rejects an oversized typed value only after a
|
||||
potentially much larger allocation has already occurred. Defaults and values
|
||||
returned by application resolution can reach these paths.
|
||||
|
||||
**Evidence.** A standalone public-API diagnostic loaded `NoSource()` with defaults
|
||||
containing an `Items []struct{}` field initialized with
|
||||
`make([]struct{}, int(^uint(0)>>1))`. Zero-sized elements make the input slice
|
||||
inexpensive, but projection immediately panicked with
|
||||
`runtime error: makeslice: cap out of range`. It did not return the existing
|
||||
`schema.projection_limit` diagnostic. No enormous backing array was successfully
|
||||
allocated by this diagnostic.
|
||||
|
||||
**Consequence.** Less extreme oversized containers can cause excessive temporary
|
||||
memory allocation before rejection; sufficiently large lengths panic or exhaust
|
||||
memory instead of reaching the intended limit failure. This is a library-created
|
||||
allocation, distinct from memory an application has already allocated itself.
|
||||
|
||||
**Potential remediation.** Before bulk reservations, compare the known minimum
|
||||
node cost of the container against the operation's remaining existing allowance,
|
||||
using overflow-safe arithmetic. Alternatively, bound initial capacity and grow
|
||||
only as successfully charged elements are accepted. Review the analogous map
|
||||
reservation/key collection. Preserve current node/depth semantics, inline-field
|
||||
behavior, and useful diagnostic paths; do not introduce an unrelated container
|
||||
length cap or promise a universal heap-size bound.
|
||||
|
||||
## 2. Retained-History Accounting Omits Published Replay Trees
|
||||
|
||||
Priority: medium. Category: retained memory and lifecycle integration.
|
||||
|
||||
**Relevant code.** The history check in `executeLoadRequest` in
|
||||
[load.go](../../internal/engine/load.go), `historyNodesWithin` in
|
||||
[history.go](../../internal/engine/history.go), `preparePostDecodeOverrideContext`
|
||||
in [override_context.go](../../internal/engine/override_context.go), and
|
||||
`resolveLoadInput` / `publishLoadResult` in
|
||||
[load_result.go](../../internal/engine/load_result.go).
|
||||
|
||||
**Expected behavior.** The
|
||||
[retained-history contract](../config.md#historical-value-explanation) limits retained
|
||||
history to 4,000,000 distinct document nodes, counting shared nodes once and
|
||||
reporting `explain.limit_exceeded` when exceeded.
|
||||
|
||||
**Actual behavior.** The load check visits the original decoded baseline and
|
||||
pre-CLI final tree, the resolved final tree, application trees, resolution deltas,
|
||||
and the acquisition inheritance scope. Publication retains a different root set:
|
||||
it includes a separately cloned post-CLI decoded tree as inspection input, plus
|
||||
independent `structuralInput` and `preCLI` replay snapshots. The replay snapshots
|
||||
are cloned again during publication and included in its immutable lookup. They
|
||||
are not passed to the budget check. Inheritance operation copies are also made
|
||||
after that check. Counting the earlier root set does not bound the distinct
|
||||
nodes actually kept alive by the result.
|
||||
|
||||
**Evidence.** A successful public load of `items: [1, 2]`, decoded into
|
||||
`struct { Items []int }` with a no-op resolver, was inspected by a read-only
|
||||
standalone reflection diagnostic. Its baseline/input/final/application roots
|
||||
contained 14 distinct nodes; including the two published replay roots yielded
|
||||
22. The checkpoint uses a different decoded-final instance of the same size in
|
||||
this example. Source inspection establishes that its count is also 14.
|
||||
|
||||
For this fixture generalized to N scalar items, the checkpoint counts `3N + 8`
|
||||
nodes, while the published roots retain `5N + 12`. Thus 800,000 items would pass
|
||||
the history checkpoint with 2,400,008 nodes yet retain 4,000,012. Such a source
|
||||
also fits the existing per-source byte and node allowances. This larger case is
|
||||
a structural derivation, not an executed allocation test.
|
||||
|
||||
**Consequence.** Successful loads can exceed the established retained-history
|
||||
limit, with additional retained lookup/index overhead. The quota currently gives
|
||||
an incomplete account of the memory-owning snapshot graph.
|
||||
|
||||
**Potential remediation.** Apply one pointer-aware accounting operation to the
|
||||
complete set of document roots actually retained at publication, including replay
|
||||
snapshots and copied inheritance history. Preserve count-once semantics for
|
||||
shared nodes. Where practical, charge or reserve these copies before creating
|
||||
them so failure does not first incur avoidable peak allocation. Reusing an
|
||||
already-owned immutable snapshot can be considered where equivalent, but this
|
||||
finding does not require the broader metadata-copy redesign deferred in
|
||||
[future experiments](future.md#3-performance-experiments), weaker detachment, a
|
||||
smaller limit, or removal of historical inspection.
|
||||
|
||||
## 3. An Imported Version-Check Panic Leaves The Host Root Open
|
||||
|
||||
Priority: medium. Category: file-descriptor cleanup during unwinding.
|
||||
|
||||
**Relevant code.** `acquireImportedInput` / `acquireImportedWithAccess` and imported
|
||||
version processing in
|
||||
[import_acquisition.go](../../internal/engine/import_acquisition.go), together with
|
||||
`openHostAccess` / `filesystemAccess.close` in
|
||||
[filesystem_access.go](../../internal/engine/filesystem_access.go). Compare the
|
||||
deferred fallback cleanup in
|
||||
[selected_overlay.go](../../internal/engine/selected_overlay.go) and deferred file
|
||||
cleanup in [source_file.go](../../internal/engine/source_file.go).
|
||||
|
||||
**Circumstances.** Host import acquisition opens an `os.Root`, then closes it
|
||||
explicitly on ordinary failure paths and after import traversal. It has no
|
||||
deferred fallback closure. A user version-check callback runs for imported
|
||||
sources while that root is open. If it panics and the application recovers outside
|
||||
`Load`, unwinding bypasses all those explicit root closes. The imported file's
|
||||
own deferred close still runs; the outstanding resource is the directory/root
|
||||
handle.
|
||||
|
||||
**Evidence.** A standalone Linux diagnostic created a temporary main file and one
|
||||
import, both with a version, enabled source-version checks, and panicked only on
|
||||
the imported source's check. With automatic GC disabled only in that disposable
|
||||
diagnostic process, the number of `/proc/self/fd` entries targeting the temporary
|
||||
root went from zero to one after external recovery. The non-panicking control
|
||||
ran both checks and returned to zero. One outstanding root was sufficient to
|
||||
demonstrate the issue; no descriptor-exhaustion workload was run.
|
||||
|
||||
**Consequence.** Library-owned root cleanup depends on eventual runtime cleanup
|
||||
instead of load-scope unwinding. Repeated recovered callback failures can retain
|
||||
descriptors long enough to exhaust them. This does not imply that ConfigKit must
|
||||
recover application panics or successfully return a configuration after one.
|
||||
|
||||
**Potential remediation.** Install a deferred fallback close at the owning
|
||||
acquisition boundary, while preserving normal explicit close timing and close
|
||||
error diagnostics, as the selected-overlay path already does. Avoid double-close
|
||||
confusion and leave borrowed native filesystems untouched. Preserve panic
|
||||
propagation. This is separate from
|
||||
[correctness finding 4](correctness.md#4-callback-contexts-remain-active-after-panic-unwinding):
|
||||
deactivating a callback context alone does not close the root, and closing the
|
||||
root alone does not deactivate that context.
|
||||
|
||||
## Validation And Boundaries
|
||||
|
||||
The following passed with Go 1.26.10 on Linux amd64, using cached dependencies
|
||||
with automatic toolchain/network acquisition disabled:
|
||||
|
||||
```sh
|
||||
go test ./internal/engine -run 'Closes|Close|Budget|Limits|Limit|SecretsDir' -count=1
|
||||
go test ./... -count=1
|
||||
```
|
||||
|
||||
The three standalone diagnostics described above were run outside the repository;
|
||||
they added no production code or repository tests. The history diagnostic only
|
||||
read private fields to count pointers; it did not mutate inspection state. No
|
||||
native macOS or Windows checks were performed.
|
||||
|
||||
No additional network, timer, goroutine, buffer-pool, or ordinary-return handle
|
||||
leak was established. Borrowed blocking readers/callbacks do not create a new
|
||||
cancellation guarantee. Previously reported recursive-schema stack exhaustion
|
||||
and callback-state retention remain owned by the
|
||||
[correctness audit](correctness.md), rather than duplicated here. Existing
|
||||
algorithmic and maintainability findings remain owned by their respective
|
||||
audits. This report adds no API, release gate, new resource quota, or background
|
||||
management requirement.
|
||||
Reference in New Issue
Block a user