Audit runtime execution and concurrency

2026-08-08 21:27:37 +00:00
parent e2b82746ab
commit 70c199aa31
1 changed files with 273 additions and 4 deletions
--- a/docs/roadmap/audit.md
+++ b/docs/roadmap/audit.md
@@ -19,9 +19,10 @@ change only roadmap audit documents do not change that production target.
 ## Executive Summary

 Pending final synthesis. The initial baseline is healthy. The architecture,
-configuration/CLI, pipeline composition, and reference/handoff reviews have
-found two Medium findings and seven Low findings, with no production dependency
-inversion or unsafe typed-erasure boundary.
+configuration/CLI, pipeline composition, reference/handoff, and runtime reviews
+have found two High findings, three Medium findings, and eight Low findings,
+with no production dependency inversion, unbounded framework worker pool, or
+completion-order-dependent result assembly.

 ## Finding Index

@@ -37,6 +38,10 @@ Final cross-area ordering is pending synthesis.
 | REF-001 | Medium | Efficiency | Bound reference reads before allocating the file |
 | REF-002 | Low | Efficiency | Index accepted outputs once per ordered handoff |
 | REF-003 | Low | Correctness | Include canonical size in generated-reference fingerprints |
+| RUN-001 | High | Correctness | Check cancellation at unguarded dispatch boundaries |
+| RUN-002 | High | Correctness | Isolate typed validator values from stage output |
+| RUN-003 | Medium | Correctness | Preserve warnings from the terminal rejected attempt |
+| RUN-004 | Low | Documentation/Comments | Document the lane collector's liveness invariant |

 ## Findings

@@ -292,6 +297,164 @@ Final cross-area ordering is pending synthesis.
  `go test ./internal/framework/pipeline ./internal/modules/integration/...`.
 - **Grouping:** Independent.

+### Execution, Validation, Retry, And Concurrency
+
+### RUN-001 — Check cancellation at unguarded dispatch boundaries
+
+- **Severity:** High
+- **Category:** Correctness
+- **Evidence:** `Runner.Run` invokes the input adapter at
+  `internal/framework/pipeline/runner.go:159`–`177` without checking
+  `ctx.Err()` first; the first framework-owned cancellation check on that path
+  is inside `runWithRetry` at lines 389–418, after source work and its
+  checkpoint/debug side effects. At the other end of the run, ordered lane work
+  completes at lines 278–282, then evidence/debug assembly and
+  `encoder.Encode` run at lines 297–346 without another cancellation check.
+  The worker and retry paths do check cancellation, but
+  `TestRunnerReturnsParentCancellationAndStopsQueuedExtracts` covers only
+  cancellation while two extract calls are active
+  (`runner_concurrency_test.go:559`–`585`). A context-ignoring input or output
+  module can therefore be called with an already-cancelled context; in the
+  output case it can return files and make the cancelled run report success.
+- **Impact:** Work can start after cancellation, and a cancellation arriving
+  after lane completion or during output debug assembly can still publish a
+  successful logical output. This violates the documented guarantee that
+  parent cancellation prevents queued work and output encoding, and it leaves
+  correctness dependent on every module independently honoring an already
+  cancelled context.
+- **Recommendation:** Add framework-owned cancellation gates at run entry and
+  immediately before every module dispatch not already protected by
+  `runWithRetry` or a lane worker, especially input parsing and output encoding.
+  Recheck after intervening collaborator/debug/evidence work so a cancellation
+  cannot slip between the gate and the operation, and after an unguarded module
+  returns so a module that did not observe mid-call cancellation cannot publish
+  success. Return the parent context error through the existing failed-output
+  path.
+- **Preserve:** Continue to pass the caller context into active operations,
+  wait for all started lane work, suppress output files on framework failure,
+  select an actual framework error deterministically when the parent remains
+  live, and prefer the parent cancellation when it is set.
+- **Validation:** Add a pre-cancelled run whose input adapter ignores context
+  and must not be called, plus a run whose debug recorder cancels immediately
+  before output dispatch while a context-ignoring encoder records calls, and an
+  encoder that cancels mid-call but returns files. Assert `context.Canceled`, no
+  invocation after pre-dispatch cancellation, and no output files in either
+  output case; retain the active-extract cancellation case and run the package
+  under `-race`.
+- **Grouping:** Independent.
+
+### RUN-002 — Isolate typed validator values from stage output
+
+- **Severity:** High
+- **Category:** Correctness
+- **Evidence:** Extract, merge, and normalize serialize a canonical candidate
+  before validation, but pass the operation's original typed value alongside
+  it; representative extract code is
+  `internal/framework/pipeline/runner_concurrent.go:424`–`439`, and merge and
+  normalize do the same in `runner_typed.go:264`–`277` and 359–392.
+  `validateTypedArtifact` clones source input, references, metadata, and chunks,
+  but `requestTarget := target` leaves `target.value` shallow-copied
+  (`runner_typed.go:458`–`499`). The registered typed adapter then exact-casts
+  that value and passes it directly to the validator
+  (`validator_registry.go:109`–`119`). Struct values containing slices, maps,
+  or pointers therefore retain aliases to the stage output. A validator can
+  mutate what later validators and final checkpoint serialization observe,
+  while serialized validators and attempt debug continue to describe the
+  pre-mutation canonical candidate. Candidate-encoding tests verify encode
+  counts and rejection-before-checkpoint behavior, but no focused test mutates
+  a typed validator request.
+- **Impact:** A buggy validator can corrupt an accepted output, cause validators
+  in one chain to inspect different values, or make persisted output differ
+  from the candidate that was serialized for validation and debug. This breaks
+  the documented immutable whole-output validation contract at all three typed
+  stages.
+- **Recommendation:** Treat the already serialized canonical candidate as the
+  clone boundary. Decode a fresh exact typed value through the active codec for
+  each typed validator, or provide an equivalent codec-backed deep clone, and
+  never expose the stage operation's retained value. Keep serialized validators
+  on independent schema/content copies of the same candidate.
+- **Preserve:** Retain exact Go-type checks, one canonical candidate encode per
+  attempt, validator declaration order, isolated request metadata/references,
+  serialized-validator byte ownership, contextual errors, and final encoding
+  only after the entire chain approves.
+- **Validation:** Use an artifact with both slice and map fields. Have the first
+  typed validator mutate both, then assert that the next typed validator, a
+  serialized validator, attempt debug, and accepted extract/merge/normalize
+  output all observe the original canonical value. Run the focused package
+  tests under `-race` as well as normally.
+- **Grouping:** Independent.
+
+### RUN-003 — Preserve warnings from the terminal rejected attempt
+
+- **Severity:** Medium
+- **Category:** Correctness
+- **Evidence:** The runtime contract says warnings from the final accepted or
+  rejected attempt are preserved (`docs/internal/pipeline.md:122`–`126`). Each
+  retry closure assembles per-attempt operation and validator warnings, but
+  stores them only on acceptance. Generated chunk-plan rejection returns at
+  `internal/framework/pipeline/runner_chunk_plan.go:142`–`150` before assigning
+  `result.warnings` at line 154, and `Runner.Run` promotes chunk warnings only
+  for acceptance or a cache hit (`runner.go:228`–`233`). Extract follows the
+  same pattern at `runner_concurrent.go:425`–`457`; `finalizeLaneExtract` skips
+  directly from a rejected result to the next chunk at lines 469–476. Merge
+  and normalize collect `attemptWarnings` but return terminal rejections before
+  appending them to `RunOutput` (`runner_typed.go:264`–`301` and 359–415).
+  `runWithRetry` returns only the last rejection, not its warnings
+  (`runner.go:389`–`428`). Existing retry-warning tests prove that discarded
+  attempts are not promoted, and final-rejection debug tests prove rejection
+  recording, but neither asserts terminal rejection warnings.
+- **Impact:** Module warnings and warnings from validators that approved before
+  the rejecting validator disappear exactly when the final candidate is
+  rejected. Operators receive the rejection but lose diagnostics produced by
+  that terminal attempt, contrary to the documented output contract.
+- **Recommendation:** Carry the warnings associated with the last rejection
+  through the retry result, then promote only that terminal attempt's warnings
+  at chunk, extract, merge, and normalize rejection handling. Keep warnings
+  from earlier failed or rejected attempts confined to attempt debug. Coordinate
+  any persistence-format addition with the later checkpoint/state audit rather
+  than silently changing state ownership here.
+- **Preserve:** Do not promote warnings from attempts superseded by a later
+  retry, do not turn rejection into a framework error, keep deterministic
+  lane/chunk warning order, and retain accepted and reused checkpoint warning
+  behavior.
+- **Validation:** Add table-driven chunk, extract, merge, and normalize cases
+  with distinct first-attempt and final-rejection warning scopes. Assert that
+  only the final scopes reach `RunOutput`, rejection attempt counts remain
+  correct, intermediate warnings remain in attempt debug, and shuffled/race
+  runs preserve order.
+- **Grouping:** Independent.
+
+### RUN-004 — Document the lane collector's liveness invariant
+
+- **Severity:** Low
+- **Category:** Documentation/Comments
+- **Evidence:** `laneEngine.collect` coordinates a dynamically enabled
+  continuation send, extract-result closure, cancellation, and the
+  `pending`/`launched`/`completed` counters in
+  `internal/framework/pipeline/runner_concurrent.go:256`–`281`, with state
+  changes split across `handleExtractResult` and `handleCompletion` at lines
+  283–313. The loop has no invariant-level comment. Its non-obvious purpose is
+  to discard only unlaunched continuations after cancellation while continuing
+  to drain extract results and exactly one completion from every launched
+  continuation before the continuation channel is closed and workers are
+  awaited.
+- **Impact:** Current tests and the race run support the implementation, but a
+  future change to channel buffering, cancellation cleanup, or a counter can
+  introduce a deadlock or early close without the ownership/liveness rule being
+  visible at the maintenance point.
+- **Recommendation:** Add one short comment immediately above
+  `laneEngine.collect` stating that it is the sole owner of pending/launch/
+  completion accounting and must drain closed extract results plus every
+  launched continuation completion even after cancellation. Do not narrate the
+  `select` cases.
+- **Preserve:** Keep chunk-first/lane-second extract dispatch, bounded extract
+  and continuation pools, overlap between completed lanes and remaining
+  extracts, cancellation of undispatched work, and the final worker wait.
+- **Validation:** Review the comment against the collector termination
+  predicate and retain the existing cancellation, reverse-completion,
+  continuation-overlap, worker-bound, race, and shuffled tests.
+- **Grouping:** Independent.
+
 <!--
 Finding template for later audit stages:

@@ -380,6 +543,30 @@ Finding template for later audit stages:
  keeps validators on authoritative inputs. PIPE-002 applies only to adjacent
  construction-builder copies and should not remove these operation-time
  ownership boundaries.
+- `Runner.Run` is a long linear coordinator, but its sequence is the runtime
+  contract: source state precedes chunk planning; a rejected chunk plan skips
+  lanes but remains an encodable ordinary outcome; ordered steps are barriers;
+  evidence is built only from accepted normalized artifacts; and logical output
+  encoding is last. RUN-001 requires narrow cancellation gates and should not
+  replace that readable lifecycle with a generic stage engine.
+- Extract jobs and lane continuations deliberately use separate pools, each
+  bounded by the resolved worker count. This permits a completed lane to enter
+  its serial merge/normalize continuation while other lanes are still
+  extracting, without letting either class grow with lane or chunk count. The
+  two pools should not be collapsed merely to make the configured limit a
+  process-wide semaphore; provider calls have their own shared scheduler.
+- Chunk, extract, merge, and normalize retry closures repeat candidate debug,
+  rejection, and warning mechanics around different typed operations and
+  checkpoint transitions. Their common retry counter and cancellation policy
+  appropriately live in `runWithRetry`, while stage-specific serialization,
+  normalize fallback, and state recording remain explicit. RUN-003 calls for a
+  small terminal-warning result, not a parameter-heavy generic stage runner.
+- `synchronizedCheckpointRecorder`, `synchronizedDebugRecorder`, and
+  `synchronizedLLMDebugRecorder` intentionally serialize collaborators whose
+  contracts do not promise concurrent safety. Each adapter owns one lock and
+  invokes only its wrapped collaborator, so there is no framework lock-order
+  cycle; removing them would push concurrency requirements into filesystem and
+  test implementations.

 ## Areas Reviewed Without Findings

@@ -591,6 +778,83 @@ Finding template for later audit stages:
  cover semantic generated-reference consumers. REF-001 and REF-003 identify
  the two unproved identity/resource details.

+### Execution, Validation, Retry, And Concurrency
+
+- **Runtime state diagram:** The audited framework lifecycle is:
+
+  ```text
+  input validation / owned metadata
+    -> source load or parse -> document validation
+    -> chunk load or plan -> materialize -> whole-plan validator chain
+       -> rejected: terminal ordinary outcome ----------------------+
+       -> accepted: for each ordered step                            |
+          -> generated handoff barrier                              |
+          -> initialize lane checkpoint states                      |
+          -> dispatch extract jobs (chunk first, lane second)       |
+             -> operation -> canonical candidate -> validators      |
+             -> retry | rejected chunk | accepted chunk             |
+          -> completed lane enters bounded continuation             |
+             -> merge -> validators -> retry/reject                 |
+             -> normalize -> validators -> retry/reject             |
+          -> await all started work -> stable lane merge -> barrier |
+    +---------------------------------------------------------------+
+    -> manifest/evidence -> logical output encoder
+
+  any framework error or parent cancellation
+    -> cancel derived work -> stop dispatch -> drain/await started work
+    -> stable error selection -> failed output (no logical files)
+  ```
+
+- **Ordering and bounded work:** `dispatchExtractJobs` enumerates chunks first
+  and lanes second. One fixed extract pool and one fixed continuation pool are
+  each bounded by `ExtractWorkers`; a lane continuation is serial merge then
+  normalize, while different completed lanes may overlap remaining extracts.
+  Results are indexed by lane and chunk, extracts are sorted by chunk index,
+  lane outputs merge in prepared order, and framework errors sort by stage,
+  lane, and chunk after child cancellation errors are filtered. Reverse-
+  completion, continuation-overlap, continuation-bound, provider-bound, and
+  stable-error tests confirm these contracts independently of goroutine finish
+  order.
+- **Cancellation and liveness:** The lane engine checks cancellation before
+  starting queued extract and continuation work, stops dispatch, clears
+  unlaunched continuations, drains worker results, receives every launched
+  completion, and waits for both pools. The retry loop checks before an attempt
+  and after failed/rejected attempts. Parent cancellation takes precedence over
+  collected child cancellation, while an actual framework failure is selected
+  deterministically when the parent remains live. RUN-001 records the two
+  unguarded outer dispatch boundaries; no additional worker leak or deadlock
+  path was found. RUN-004 records the missing collector invariant comment.
+- **Terminal outcomes:** Chunk, extract, merge, and normalize validators inspect
+  whole candidates, not partial streams. A rejection is collected as ordinary
+  output: a rejected chunk plan skips all lanes, a rejected extract chunk does
+  not prevent accepted chunks in that lane from merging, and merge/normalize
+  rejection terminates only that lane. A framework error cancels sibling work,
+  blocks later ordered steps, and returns through `failOutput`, which leaves no
+  logical output files. RUN-002 records the typed-value ownership violation;
+  serialized validators receive owned canonical bytes.
+- **Retries and diagnostics:** Operation and complete validator-chain execution
+  share one retry budget. Debug persistence failure is terminal rather than
+  retried; cancellation stops retries; rejections record their final attempt;
+  and normalize retry directives use the same budget before validating their
+  final fallback. Candidate serialization precedes validation and accepted
+  checkpoint serialization follows it, so invalid candidates are not recorded
+  as success. Intermediate-attempt warnings remain only in attempt debug as
+  intended; RUN-003 records loss of the final rejected attempt's warnings.
+- **Collaborator synchronization:** Runner-local wrappers serialize checkpoint,
+  debug, and LLM-debug collaborators without acquiring multiple framework
+  locks at once. Module/provider concurrency remains separately bounded by the
+  shared LLM scheduler. No duplicate recording or permit leak was found in the
+  audited execution layer; durable checkpoint transitions and filesystem
+  atomicity remain in the next audit area.
+- **Focused test review:** Concurrency tests exercise chunk-first dispatch,
+  reverse completion, extract and continuation bounds, intended overlap,
+  provider limits, stable stage/lane error priority, ordinary rejection, parent
+  cancellation, and stopped queued extracts. Retry, rejection, candidate-
+  encoding, session, manifest, typed-checkpoint, debug-attempt, accepted-
+  checkpoint, and generated-handoff tests cover the remaining execution
+  branches. The consequential missing runtime cases are included in RUN-001
+  through RUN-003 rather than duplicated as test-only findings.
+
 ## Validation Record

 | Date | Scope | Command or check | Result |
@@ -615,6 +879,11 @@ Finding template for later audit stages:
 | 2026-08-08 | Reference and handoff graph review | Architecture, exact symbol reads, and call traces across external materialization, target resolution, operation cloning, generated codec handoff, consumer fingerprints, ordered execution, and accepted-checkpoint hydration | REF-001, REF-002, and REF-003 recorded; no content/evidence leak found |
 | 2026-08-08 | Focused pipeline and CLI tests | `go test ./internal/framework/pipeline ./internal/cli` | Pass |
 | 2026-08-08 | Assembled integration tests | `go test ./internal/modules/integration/...` | Pass |
+| 2026-08-08 | Audit target integrity before runtime review | `git diff --quiet 92e89076a268089e703978fb9d7176200e93344c..HEAD -- . ':(exclude)docs/roadmap/**'` | Pass; production target unchanged |
+| 2026-08-08 | Runtime graph and state-machine review | Exact symbol reads, complexity inspection, call traces, cancellation-search coverage, and focused concurrency/retry/rejection/session/checkpoint/debug/manifest/handoff test review | RUN-001 through RUN-004 recorded; bounded work, stable assembly, and worker draining otherwise confirmed |
+| 2026-08-08 | Pipeline race tests | `go test -race ./internal/framework/pipeline` | Pass |
+| 2026-08-08 | Fresh pipeline tests | `go test -count=1 ./internal/framework/pipeline` | Pass |
+| 2026-08-08 | Shuffled pipeline tests | `go test -shuffle=on ./internal/framework/pipeline` | Pass |

 ## Coverage Matrix

@@ -624,7 +893,7 @@ Finding template for later audit stages:
 | Configuration and CLI composition | Reviewed | `docs/config.md`, `docs/cli.md`, `docs/operations.md`, internal configuration/CLI docs; `internal/core/config/`; CLI run, catalog, session, profile, result, and terminal owners; focused config, command, run, reference, recomputation, session, production, example, result, and state tests | Target-integrity check, graph call/data-owner traces, YAML decoder contract, focused tests and vet | CFGCLI-001, CFGCLI-002 |
 | Pipeline resolution, preparation, and typed registries | Reviewed | Internal pipeline/module docs; `internal/framework/contracts/`; pipeline profile, options, module, construction, preparation, fingerprint, stage/validator/chain/codec/evidence registry implementations and focused tests | Target-integrity check, graph architecture/complexity/call traces, focused tests and vet | PIPE-001, PIPE-002 |
 | References and ordered handoffs | Reviewed | Reference and ordered-step sections of configuration, internal pipeline, and state docs; pipeline reference resolution/materialization, preparation ownership, generated handoff, consumer fingerprint, checkpoint hydration, and runner barriers; CLI selector/recomputation owners; focused profile, reference, handoff, checkpoint, recomputation, and assembled integration tests | Target-integrity check, graph architecture/complexity/call traces, focused pipeline/CLI tests, assembled integration tests | REF-001, REF-002, REF-003 |
-| Execution, validation, retry, and concurrency | Pending | — | — | — |
+| Execution, validation, retry, and concurrency | Reviewed | Internal pipeline runtime contract; runner, chunk planning/validation, concurrent lane engine, typed execution/validation, retry/normalize, synchronization, output suppression, and focused concurrency, retry, rejection, session, typed-checkpoint, debug, manifest, candidate-encoding, and handoff tests | Target-integrity check, graph state-machine/call/complexity review, race tests, fresh tests, shuffled tests | RUN-001, RUN-002, RUN-003, RUN-004 |
 | State, checkpoints, debugging, and file safety | Pending | — | — | — |
 | LLM runtime, prompt filesystems, and assets | Pending | — | — | — |
 | Generic and Seriatim modules | Pending | — | — | — |