Bound run history across pipeline identities

[thomhurst]

Summary

• add a configurable global cap across all built-in run-history identities
• prune only owned files using the embedded UTC run timestamp across identity hashes
• document and validate the new GlobalHistoryRetention option

Validation

• RunReportTests: 48/48 passed
• exact abandoned-identity regression: passed
• core Release build: 0 warnings, 0 errors
• scoped format completed (expected F# unsupported notice only)

Closes #3739

[thomhurst]

@codex review

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[claude]

Review: Bound run history across pipeline identities (#3833)

Verified locally: core builds clean (0 warnings/errors) and RunReportTests passes 48/48 (47 passed, 1 skipped Windows-only test), matching the PR description.

The fix directly addresses #3739 — pruning is genuinely broken today because per-identity pruning only ever looks at {filePrefix}*.json, so a derived identity that changes (module added/removed, path tweak) abandons its old files forever. Adding a second, cross-identity sweep over modularpipelines-run-*.json is the right shape, and switching the sort key from filename-lexicographic to an explicitly parsed embedded timestamp (GetHistoryTimestamp) is a necessary and correct change — lexicographic filename order across different identity hashes sorts by hash first, not by time, so the naive approach wouldn't have worked for the global pass. Good catch, and the new FileSystemHistoryStorePrunesAbandonedPipelineIdentitiesToGlobalRetention test exercises exactly the regression from the issue.

Two design points worth considering before/after merge:

1. The global cap can silently evict active identities, not just abandoned ones

PruneFiles ranks every owned file across all identities purely by recency and keeps the top GlobalHistoryRetention. That's a flat pool, not "per-identity retention, plus cleanup of stale identities." Concretely: with the defaults (HistoryRetention = 20, GlobalHistoryRetention = 200), 10+ actively-used identities in a monorepo/multi-pipeline setup will collectively exceed 200 files, and the global sweep will then delete the oldest files across the whole pool — potentially wiping out a low-frequency identity's entire history (which is still well inside its own 20-file budget) because a high-frequency identity's runs dominate the "recent" ranking.

This is a real regression relative to the current doc/comment on HistoryRetention: "The limit applies independently to each pipeline identity" (RunReportOptions.cs:32) — that guarantee no longer holds once the global cap kicks in, and the change silently narrows it without saying so. The issue itself suggested this as one option ("apply a global cap or age-based sweep"), but a pure global-recency cap doesn't actually distinguish "abandoned" from "just quieter than its neighbors."

Suggestion: give each identity a protected floor before the global sweep considers it fair game — e.g., only include an identity's files beyond its own newest N (or beyond some age threshold) in the global candidate pool, so a quiet-but-active identity always keeps at least a minimal history regardless of how busy other identities are. That's closer to "sweep the obsolete stuff" than "keep the newest 200 regardless of origin." At minimum, this trade-off deserves a callout in the docs (run-reports.md) beyond "prevents obsolete derived identities from growing... indefinitely," since as implemented it can also prune non-obsolete ones.

2. No validation that GlobalHistoryRetention >= HistoryRetention

If a user sets HistoryRetention = 50 for a single-identity pipeline but leaves GlobalHistoryRetention at a lower value (or vice versa — the defaults happen to be compatible, but a custom config easily won't be), the global sweep runs immediately after the per-identity sweep on every save and will quietly cap history below the value the user explicitly configured, with no warning. OptionsValidator already rejects negative values for both settings independently (OptionsValidator.cs:80-86) — it'd be worth also flagging GlobalHistoryRetention > 0 && GlobalHistoryRetention < HistoryRetention as at least a warning, since that combination makes HistoryRetention partially or fully unreachable in a way that isn't obvious from either value in isolation.

Minor

• Ordering now depends on the PipelineRunReport.End timestamp embedded in the filename rather than filesystem write time. That's the right call for determinism, but it does mean cross-identity ordering (the new global sweep) now implicitly assumes reasonably synced clocks across whatever machines/CI runners write into a shared history directory — a skewed clock on one runner could cause its identity to dominate the "newest" ranking and starve others faster than real recency would suggest. Probably fine in practice, just worth being aware of given the docs already call out CI agents restoring a shared cache.
• SaveAsync now does two full Directory.EnumerateFiles + sort passes per save (per-identity, then global). Not a real concern at the default retention sizes, but if GlobalHistoryRetention is set very high this scales linearly with total history-directory size on every single save — a non-issue today, just flagging for awareness if defaults ever grow.

Nothing here is a blocker — the fix is correct for the reported bug and well tested. The main thing I'd want addressed (or at least consciously accepted and documented) is point 1, since it changes the reliability guarantee HistoryRetention currently advertises.

[thomhurst]

Addressed both review design points in 6c049a1. The API docs and run-report guide now explicitly state that the flat global pool supersedes per-identity retention and can evict a quiet identity entirely, with GlobalHistoryRetention=0 and separate directories documented as preservation options. Configuration now rejects a positive global limit below HistoryRetention. Validation: focused global-retention tests 3/3; core Release build 0 warnings/errors.

[claude]

Code review

Summary: This PR adds a GlobalHistoryRetention cap (default 200) that bounds run-history files across all pipeline identities, on top of the existing per-identity HistoryRetention. It reworks pruning to sort by a timestamp embedded in the filename rather than by raw filename, since across identities the filename is no longer chronologically sortable (each identity's SHA-256 hash prefix dominates the ordinal comparison). Validation now rejects a negative GlobalHistoryRetention and a positive GlobalHistoryRetention set lower than HistoryRetention. Docs and tests are updated accordingly.

I checked this against the two review points referenced in the "Addressed both review design points in 6c049a1" comment (docs clarifying that the global limit supersedes per-identity retention, and validation rejecting GlobalHistoryRetention < HistoryRetention) — both are present in the current diff.

Overall this is solid: the core insight (filename ordinal sort breaks down once you compare across identity hashes, so pruning needs to parse the embedded timestamp) is correct, and it's well covered by FileSystemHistoryStorePrunesAbandonedPipelineIdentitiesToGlobalRetention.

Design suggestion: don't derive the timestamp offset from a duplicated magic number

ModularPipelines/src/ModularPipelines/Engine/FileSystemRunHistoryStore.cs

Lines 189 to 207 in 6c049a1

	private static DateTime GetHistoryTimestamp(string path)
	{
	var fileName = Path.GetFileName(path);
	var timestampStart = OwnedFilePrefix.Length + IdentityHashLength + 1;
	if (fileName.Length < timestampStart + FileTimestampFormat.Length
	|| !DateTime.TryParseExact(
	fileName.AsSpan(timestampStart, FileTimestampFormat.Length),
	FileTimestampFormat,
	CultureInfo.InvariantCulture,
	DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal,
	out var timestamp))
	{
	return DateTime.MinValue;
	}
	return timestamp;
	}

GetHistoryTimestamp locates the timestamp via a hardcoded offset:

private const int IdentityHashLength = 64;
...
var timestampStart = OwnedFilePrefix.Length + IdentityHashLength + 1;

IdentityHashLength = 64 is only correct because GetPipelineFilePrefix happens to hex-encode a SHA-256 hash (32 bytes → 64 hex chars). Nothing ties these two facts together — if the hashing scheme in GetPipelineFilePrefix ever changes (different algorithm, truncated hash, etc.), IdentityHashLength won't be updated automatically, and the failure mode is silent: TryParseExact just fails, GetHistoryTimestamp falls back to DateTime.MinValue, and pruning quietly degrades.

That degradation matters most for the new global-retention feature specifically: within a single identity, the per-identity prune still has a secondary sort (ThenByDescending on filename) that happens to stay correct because all filenames in that group share the same prefix. But across identities — the whole point of GlobalHistoryRetention — the secondary filename sort is dominated by the (differing) identity hashes again, which is precisely the bug this PR is fixing. So a future hash-format change would silently reintroduce unfair global pruning (some identities always "look oldest" and get evicted first) without any exception or test failure to flag it.

A more robust approach would avoid needing to know the hash length at all — e.g. strip the known OwnedFilePrefix, then split the remainder on - to get [hash, timestamp, guid] segments (hex hashes and the yyyyMMddHHmmssfffffff timestamp never contain -, so this is unambiguous). That removes IdentityHashLength entirely and makes the parsing logic self-adjusting if the hash representation ever changes, rather than depending on two places in the file staying in sync by convention.

This is a maintainability/robustness suggestion rather than a bug in the current code (today's hash length is in fact 64), so not blocking.

[thomhurst]

Required pipeline failed on unrelated core process-cancellation test ExecuteCommandLineToolAsync_ForcefulCancellation_KillsDescendantProcesses while this PR only changes run-history retention. Triggered the single allowed failed-job rerun for workflow 30938677735.

[claude]

Code review

Re-checked after the latest push (650682fd). That commit is a clean merge from main — it only pulls in unrelated upstream test additions (AtomicFileWriter tests, Status.CachedResult case) and doesn't touch any of this PR's own logic. The FileSystemRunHistoryStore, RunReportOptions, OptionsValidator, and history-retention test changes are byte-for-byte the same as what was already reviewed.

Both design points from the first review are addressed:

• Docs (run-reports.md) now explicitly state the global cap supersedes per-identity retention and can evict a quiet identity entirely, with GlobalHistoryRetention = 0 and separate directories/identities documented as workarounds.
• OptionsValidator now rejects a positive GlobalHistoryRetention below HistoryRetention.

The maintainability suggestion from the second review (deriving the timestamp offset from IdentityHashLength = 64 rather than parsing hash/timestamp/guid segments) is still unaddressed, but that was explicitly called out as a non-blocking robustness suggestion, not a bug in the current code — today's SHA-256 hash length is in fact 64, so nothing here is incorrect as written.

No new issues found in this pass.

[thomhurst]

Addressed the remaining maintainability suggestion: history timestamps are now parsed from trailing filename segments rather than assuming a 64-character identity hash. Added a variable-length identity regression, merged current main while preserving stale atomic-temp cleanup, and validated 4 focused tests plus the Release core build (0 warnings/errors).

[claude]

Code review

Re-checked after the latest push (6c049a1 → current, which addresses the remaining maintainability suggestion from the prior review).

What changed since the last review pass: GetHistoryTimestamp no longer relies on the hardcoded IdentityHashLength = 64 constant. It now locates the timestamp by finding the last two - separators in the filename (...-{timestamp}-{guid}.json) rather than assuming a fixed hash length:

var uniqueIdSeparator = fileName.LastIndexOf('-');
var timestampSeparator = uniqueIdSeparator > 0
    ? fileName.LastIndexOf('-', uniqueIdSeparator - 1)
    : -1;

I verified this is safe against the actual filename format produced by GetPipelineFilePrefix/SaveAsync (modularpipelines-run-{hexHash}-{yyyyMMddHHmmssfffffff}-{guid:N}.json, FileSystemRunHistoryStore.cs:142-143,239-243) — the hash is hex (no dashes), the timestamp format has no dashes, and Guid:N has no dashes, so splitting on the last two - unambiguously isolates the timestamp regardless of hash length. The new FileSystemHistoryStoreReadsTimestampWithoutAssumingIdentityLength test exercises exactly this by using a "legacy"-length fake hash segment instead of a 64-char one, and confirms parsing still works. Also checked the failure path (no - found, or uniqueIdSeparator <= 0): timestampStart/timestampLength stay non-negative before the length-guard runs, so there's no risk of an out-of-range AsSpan call — it just falls back to DateTime.MinValue as before. This closes out the maintainability concern cleanly without behavior changes for well-formed filenames.

Other points from earlier reviews, re-verified against the current diff:

• Docs (run-reports.md) still correctly disclose that the global cap supersedes per-identity retention and can evict a quiet identity entirely, with GlobalHistoryRetention = 0 and separate identities/directories documented as the escape hatches. This was the main design trade-off flagged in the first review pass (a flat global-recency pool can prune an active-but-quiet identity, not just abandoned ones) — it's a real, deliberate behavior change from the previous per-identity-only guarantee, and it's now called out explicitly rather than left implicit, which is the right amount of transparency for a non-blocking design choice.
• OptionsValidator rejects negative GlobalHistoryRetention and a positive GlobalHistoryRetention < HistoryRetention — read through the logic (OptionsValidator.cs:80-96) and it matches both new tests (RunReportOptionsRejectNegativeGlobalRetention, RunReportOptionsRejectGlobalRetentionBelowPerIdentityRetention).
• Traced through FileSystemHistoryStorePrunesAbandonedPipelineIdentitiesToGlobalRetention by hand (4 identities written sequentially, HistoryRetention=2, GlobalHistoryRetention=3) — the expected eviction of the oldest identity (pipeline-0) once the pool exceeds 3 matches the implementation's OrderByDescending(GetHistoryTimestamp).Skip(retention) logic.

No new issues found. All design and maintainability feedback from the review history on this PR has now been addressed, and the changes are well covered by tests. Nothing blocking from my side.

[thomhurst]

Addressed CodeFactor’s complex-method finding by extracting run-report option validation into a focused helper. RunReportTests pass 59/59; core Release build has 0 warnings/errors. @claude review

[claude]

Code review

Re-checked after the latest push (86f705bb), which addresses the CodeFactor complex-method finding from the last comment.

What changed since the last review pass: OptionsValidator.ValidateOptions had its RunReport.* validation block (four checks: negative HistoryRetention, negative GlobalHistoryRetention, GlobalHistoryRetention < HistoryRetention, empty HistoryDirectory) extracted into a new private static ValidateRunReportOptions(RunReportOptions options, ValidationResult result) method, called via ValidateRunReportOptions(options.RunReport, result). I diffed this against the previously-reviewed version and the extracted logic is byte-for-byte identical (same conditions, same error messages) — this is a pure extract-method refactor with no behavior change, and it matches the existing pattern already used in this file for ValidateRunOnlyCategories/ValidateIgnoreCategories, so it's consistent with the surrounding style rather than a one-off.

The rest of the diff between the last-reviewed commit and current HEAD is merge noise from pulling in unrelated main changes (source generator updates, secret metadata, the temp-file-pruning and successful-only duration-delta changes from #3846, etc.) — none of that touches this PR's own logic (FileSystemRunHistoryStore's global-retention pruning, GetHistoryTimestamp, and OptionsValidator's RunReport checks), which remain as previously reviewed and unaffected by the merges.

No new issues found. All design and maintainability feedback from the review history on this PR has been addressed, and the CodeFactor complexity finding is resolved with a clean, low-risk refactor.