Reduce CoordinatedTextWriter secret-scan contention

[thomhurst]

Summary

• cache a case-insensitive SearchValues<string> with each secret-pattern snapshot and use it for one-pass match discovery
• compute retained secret-prefix length once per processing pass and avoid re-obfuscating already-clean output
• replace the global writer lock with per-stream state locks while retaining serialized writes to the underlying console
• add regression tests for single-pass obfuscation and concurrent module-buffer processing

Validation

• SecretMaskingPatternTests: 25 passed
• OutputCoordinatorTests: 19 passed
• ModularPipelines.slnx Release build: 0 warnings, 0 errors

Closes #3755

[claude]

Code review

Reviewed CoordinatedTextWriter.cs and the new tests in SecretMaskingPatternTests.cs. Traced the concurrency model, the secret-pattern matching, and the removed obfuscation calls end-to-end — no correctness or security-regression issues found.

Verified correct (worth calling out since this touches secret redaction):

• WriteCompletedLine, FlushDirectPrefix, and FlushPartialPrefix no longer call _secretObfuscator.Obfuscate(...) before writing to the real console. This looks alarming at first glance in a secret-masking path, but it's safe: ObfuscateCompletePatterns already mutates state.Buffer in place before any of these flush methods run (every call site — ProcessPendingOutput, Flush(), FlushAvailableAsync() — calls it first), so the content those methods write out is already redacted. The removed call was genuinely redundant: it re-obfuscated against the same secret set (_secretProvider.GetSnapshot()), and since CoordinatedTextWriter's own pass is hardcoded OrdinalIgnoreCase while SecretObfuscator.Obfuscate defaults to case-sensitive (SecretMaskingOptions.CaseInsensitive defaults to false), the local pass was already a superset of what the removed call would have masked. Good catch eliminating dead work.
• Lock ordering is consistent and deadlock-free: state.SyncRoot → _lineBufferLock is the only nesting direction (established in ProcessPendingOutput → GetSecretPatterns()); Flush()/FlushAvailableAsync() never hold _lineBufferLock while acquiring a state.SyncRoot. The new _outputLock cleanly serializes the actual writes to _realConsole independent of per-buffer locks, which is exactly what's needed now that different modules' buffers process concurrently.
• FindFirstPattern's replacement of the old per-pattern IndexOf loop with SearchValues<string>.IndexOfAny preserves the original tie-break semantics (leftmost match, longest pattern wins) because patterns.Values stays sorted longest-first and the StartsWith scan after IndexOfAny walks that same order.

Architectural suggestion (not a bug, but worth considering as a follow-up):

CoordinatedTextWriter.GetSecretPatterns() (lines 209–236) now duplicates a fair amount of logic that already exists in SecretObfuscator.GetRegisteredSecretCache() (src/ModularPipelines/Engine/SecretObfuscator.cs): the same (version & 1) == 0 in-progress-update check, the same distinct/order-by-length/SearchValues.Create construction, sourced from the same ISecretProvider. The two implementations have already drifted once (this class hardcodes OrdinalIgnoreCase, while SecretObfuscator honors SecretMaskingOptions.CaseInsensitive) — harmless today only because the hardcoded version happens to be the more permissive one, but that's incidental, not designed.

Since this PR is already reworking CoordinatedTextWriter's pattern cache into a small SecretPatterns record (Values + SearchValues<string>?), it'd be a natural point to extract a shared SecretPatternCache-style type (mirroring SecretObfuscator.SecretCache) that both classes consume — one place computing "current secrets as sorted, deduped, cased SearchValues", parameterized by case-sensitivity. That would remove the duplication and make it impossible for the two obfuscation passes to silently diverge on matching semantics in the future. Not blocking this PR, but flagging it since the two are functionally coupled today only by convention.

No CLAUDE.md compliance issues — the changes stay within the core library (ModularPipelines.slnx scope) and don't touch build/test tooling.

[thomhurst]

CI diagnosis: the sole Ubuntu failure was an unrelated timing timeout in EngineCancellationTokenTests.StopOnFirstException_PendingModuleAwaiterReturnsTerminatedResult after the full core suite had run for about 15 minutes. The test expected ModuleFailedException but its outer 12-second wait expired; no CoordinatedTextWriter test failed. Triggered the one allowed failed-job rerun.

[thomhurst]

Refreshed onto current main at 1127d84. Current-head validation: SecretMaskingPatternTests 29/29; OutputCoordinatorTests 19/19; core Release build 0 warnings / 0 errors. Please re-review current head.

[claude]

Code review

Re-reviewed current head (1127d849f, after the rebase onto main) — thanks for the ping to re-review. Confirmed the concurrency rework itself (per-buffer SyncRoot, _outputLock serializing real-console writes, lock ordering SyncRoot → _lineBufferLock/_outputLock never reversed) is sound and deadlock-free, and the new SearchValues<string>-based single-pass matching preserves the original leftmost-longest-match semantics.

Found one high-severity issue (posted inline): removing the _secretObfuscator.Obfuscate(...) calls from FlushDirectPrefix/WriteCompletedLine/FlushPartialPrefix drops a safety net that mattered. ObfuscateCompletePatterns's early break (when the longest match at the leftmost position overlaps the retained-prefix boundary) can flush a buffer region verbatim even though it contains a different, shorter, fully-contained registered secret earlier in that same region. I verified this with a concrete repro and a standalone reimplementation of the exact algorithm — confirmed the leak occurs post-PR and that the removed second Obfuscate() pass would have caught it pre-PR. Details and a suggested fix direction are in the inline comment.

My previous review on this PR (before the rebase) concluded the removed calls were "genuinely redundant" — that conclusion was wrong; this scenario slipped through because it depends on interaction between two separately-registered secrets near the retained-prefix boundary, which the earlier pass didn't construct a counterexample for.

Everything else from the previous review still stands: the architectural suggestion to extract a shared SecretPatternCache-style type (mirroring SecretObfuscator.GetRegisteredSecretCache) remains a reasonable non-blocking follow-up, not required for this PR.

[thomhurst]

@claude review

[thomhurst]

@codex review

[claude]

Code review

Re-reviewed at current head (ef575ed), which adds the "mask nested retained-prefix secrets" fix on top of what I reviewed previously. Traced the new logic end-to-end — no correctness or security-regression issues found.

Verified correct — the nested-secret fix (ObfuscateCompletePatterns):

The old behavior: once a match overlapping the retained-prefix boundary was found, the loop appended everything from searchIndex to the end of the buffer verbatim and broke — including any earlier, fully-resolved secret between searchIndex and the overlapping match, which could then leak unobfuscated once flushed. The new code instead only skips the individual overlapping match (searchIndex = match.Index + 1; continue) and keeps scanning, so earlier matches still get obfuscated. I confirmed the new DirectConsoleWrite_MasksContainedSecretBeforeRetainedPrefix test exercises exactly this case ("helloabcXYZdef" with overlapping patterns abcXYZdef/XYZ/efGHI") and traced it by hand — it correctly obfuscates XYZ while still retaining ef as a possible prefix of efGHI.

I also verified the invariant the fix depends on: when preservePotentialLongerMatch is true, no match with Index + Length > retainedPrefixStart is ever obfuscated (it's always skipped via continue), so the trailing retainedPrefixLength characters of pending are always copied verbatim into output. That's what makes it safe to keep reusing the caller-supplied retainedPrefixLength in FlushSafeOutput/FlushSafeOutput without invalidation in the common case, and retainedPrefixInvalidated correctly triggers a recompute for the preservePotentialLongerMatch: false paths (Flush/FlushAvailableAsync) where boundary-overlapping matches are obfuscated.

Also checked (unchanged from previous review, still holds at this head):

• Lock ordering stays consistent and deadlock-free with the added _outputLock: it's always the innermost lock, never held while acquiring state.SyncRoot or _lineBufferLock, and state.SyncRoot → _lineBufferLock remains the only nesting direction (via ProcessPendingOutput → GetSecretPatterns()).
• FindFirstPattern's SearchValues<string>.IndexOfAny + ordered StartsWith fallback still preserves leftmost/longest-match tie-break semantics, and the defensive InvalidOperationException for "no match found after IndexOfAny" is unreachable given patterns.Values/patterns.SearchValues are built from the same source with matching case-sensitivity.
• New DifferentModuleBuffers_ProcessConcurrently test meaningfully validates the per-buffer locking goal of this PR (gates one module's obfuscation call and confirms a second module's write completes independently), not just a smoke test.

Still-open, non-blocking suggestion from the previous review: CoordinatedTextWriter.GetSecretPatterns() still duplicates the pattern-cache construction logic in SecretObfuscator.GetRegisteredSecretCache() (src/ModularPipelines/Engine/SecretObfuscator.cs) — same version-check, same distinct/sort/SearchValues.Create construction, same ISecretProvider source, differing only in hardcoded OrdinalIgnoreCase vs. honoring SecretMaskingOptions.CaseInsensitive. Worth extracting into a shared SecretPatternCache-style type parameterized by case-sensitivity as a follow-up, so the two obfuscation passes can't silently diverge on matching semantics again. Not blocking.

No CLAUDE.md compliance issues — both changed files stay within the core library (ModularPipelines.slnx/ModularPipelines.Tests.slnf scope) and don't touch build/test tooling or generated options classes.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ef575ed76f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Mask shorter same-start secrets before retaining longer ones

When direct output ends with a retained prefix for another secret and two registered secrets start at the same index, this branch skips all matches at that index. For example, with secrets abc, abcdef, and efxyZ, writing abcdefxy retains efxy; FindFirstPattern returns the longer abcdef, this code advances to index 1, and the shorter abc match is never obfuscated. Since the safe prefix is now written without a final _secretObfuscator.Obfuscate pass, abcd reaches the real console and leaks the registered abc secret; consider checking shorter same-start matches before skipping the overlapping longer one.

AGENTS.md reference: AGENTS.md:L158-L162

Useful? React with 👍 / 👎.