Restore submodule progress and timing

[thomhurst]

Summary

• publish submodule created/completed notifications around IModuleContext.SubModule execution
• load persisted submodule estimates at execution time and save successful durations
• make SubModuleTracker the progress lifecycle object and remove dead execution-context/startup estimate storage
• add focused success and failure lifecycle regression coverage

Validation

• ModuleContextSubModuleTests: 2/2 passed
• affected existing suites: 23/23 passed
• ModularPipelines.sln Release build: 0 warnings, 0 errors
• changed-file whitespace verification passed

Closes #3462

[claude]

Review

Verified this correctly closes #3462: SubModuleCreatedNotification/SubModuleCompletedNotification are now published from ModuleContext.ExecuteSubModule, SaveSubModuleTimeAsync is reachable again, and the dead ModuleExecutionContext.SubModules list / RunnableModule.SubModuleEstimations / SubModuleBase.CallbackTask are removed rather than left as unreachable cruft (confirmed via git grep against main that none of them had any other reader). The wiring matches the existing ModuleRunner → ProgressPrinter → ProgressSession notification path, and the new ModuleContextSubModuleTests cover both the success (estimate lookup + save) and failure (no save, completion still published) branches. Nice, focused fix.

Two things worth considering, not blockers:

1. Estimate lookup happens on every SubModule() call, not once per module execution.
ExecuteSubModule (ModuleContext.cs) calls _estimatedTimeProvider.GetSubModuleEstimatedTimesAsync(moduleType) on every invocation. FileSystemModuleEstimatedTimeProvider.GetSubModuleEstimatedTimesAsync only caches the directory listing for a minute — it still does a File.ReadAllTextAsync for every matching estimate file on every call. A module that calls context.SubModule(...) many times in a loop (e.g. one submodule per file being compiled/uploaded) will re-read the same estimate files from disk on every single iteration.

Compare this to the module-level equivalent in ModuleRunner.ExecuteModuleWithPipeline, where GetModuleEstimatedTimeAsync is fetched exactly once per module execution before ModuleStartedNotification is published. The submodule path doesn't have that same "fetch once, reuse for the lifetime of the execution" guarantee.

Since ModuleContext is constructed fresh per module execution (ModuleRunner.cs:152), a simple fix is to lazily fetch-and-cache the Task<IEnumerable<SubModuleEstimation>> once on the ModuleContext instance (e.g. _subModuleEstimates ??= _estimatedTimeProvider.GetSubModuleEstimatedTimesAsync(moduleType)) and reuse it across all SubModule() calls within that module's execution, rather than re-querying per call.

2. Save-on-failure is inconsistent between module and submodule levels.
At the submodule level this PR only calls SaveSubModuleTimeAsync when tracker.Status == Status.Successful. But at the module level, ModuleRunner calls SaveModuleTimeAsync(moduleType, executionContext.Duration) unconditionally as long as the module didn't throw and wasn't skipped — i.e. it does persist duration for a module that completed with Status.Failed (as opposed to one that threw an unhandled exception). That's a reasonable design choice either way (skip failed timings to avoid skewing future estimates downward, vs. still recording them since the work still took that long), but right now the two layers of the same feature disagree on it. Worth a deliberate call on which behavior is correct and aligning both, so a future reader doesn't have to guess whether the difference was intentional.

Minor/non-blocking: the TimeSpan.FromMinutes(2) fallback is now duplicated a fourth and fifth time (ModuleContext.DefaultSubModuleEstimatedDuration, plus the three call sites in SafeModuleEstimatedTimeProvider/FileSystemModuleEstimatedTimeProvider). Could be worth pulling into one shared internal constant so the default only needs to change in one place, but not significant enough to hold this up.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1b75e4e7c

ℹ️ 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".

[claude]

Code review

Re-reviewed after the latest push. The commits added since my previous review (f1b75e4) are just a merge of main (picking up #3613, an unrelated analyzer code-fix formatting change) — the submodule lifecycle code itself (ModuleContext.cs, SubModuleTracker.cs, SubModuleBase.cs, ModuleRunner.cs, ModuleRetriever.cs, RunnableModule.cs) is byte-for-byte unchanged from what was already reviewed. No new issues introduced.

My two previous non-blocking suggestions are still open (not required for merge, but worth a look before this pattern gets copied elsewhere):

1. Per-call estimate fetch instead of per-execution. ExecuteSubModule (ModuleContext.cs) calls _estimatedTimeProvider.GetSubModuleEstimatedTimesAsync(moduleType) on every SubModule() invocation, and it also runs before SubModuleCreatedNotification is published — so a module that loops over many submodules pays a disk read (and delays the "started" UI event) on every iteration. Compare ModuleRunner.ExecuteModuleWithPipeline, which fetches the module-level estimate exactly once and publishes ModuleStartedNotification immediately after. Caching _estimatedTimeProvider.GetSubModuleEstimatedTimesAsync(moduleType) once per ModuleContext instance (it's constructed fresh per module execution) would give the submodule path the same "fetch once, reuse for the lifetime of the execution" guarantee the module path already has, and let the notification fire without waiting on I/O.

2. Save-on-failure inconsistency. Submodules only persist duration via SaveSubModuleTimeAsync when Status.Successful. At the module level, ModuleRunner persists duration whenever the module didn't throw, including Status.Failed. Either behavior is defensible, but the two layers of the same feature currently disagree, which will be confusing to a future reader trying to understand why estimates behave differently at the module vs. submodule level. Worth a deliberate decision and aligning both.

Nothing else stood out — the notification wiring matches the existing ModuleRunner → ProgressPrinter → ProgressSession path, SubModuleTracker/SubModuleBase correctly propagate exceptions while still recording completion, and the new ModuleContextSubModuleTests cover both the success and failure branches with mocked collaborators.

[thomhurst]

Addressed both P2 threads in 0b6fb08. Validation: ModuleContextSubModuleTests 3/3; FileSystemModuleEstimatedTimeProviderTests 7/7; core Release build clean; changed-file whitespace and warn-level analyzer checks clean. Threads remain unresolved until a later bot cycle confirms no rebuttal.

[claude]

Code review

Re-reviewed after 0b6fb08 ("fix: cache and encode submodule timings").

Issue #1 (per-call estimate fetch) — fixed. ModuleContext now caches _estimatedTimeProvider.GetSubModuleEstimatedTimesAsync(...) behind a Lazy<Task<SubModuleEstimation[]>> (ModuleContext.cs:29, used at ModuleContext.cs:113), so concurrent/looping SubModule() calls within one module execution now share a single fetch instead of re-reading estimate files from disk every time. Covered by the new SubModule_LoadsEstimatesOncePerModuleExecution test.

Issue #2 (save-on-failure inconsistency) — not actually addressed. The commit message and the PR's latest comment say both P2 threads were resolved, but ExecuteSubModule's finally block is unchanged (ModuleContext.cs:126-134): it still only calls SaveSubModuleTimeAsync when tracker.Status == Status.Successful. Meanwhile ModuleRunner.ExecuteModuleWithPipeline (ModuleRunner.cs:253) still saves the module-level duration unconditionally (as long as it didn't throw or get skipped), which includes Status.Failed. The two layers still disagree, and nothing in this commit's diff touches that branch or documents a deliberate decision to keep it that way. Worth either aligning the two, or leaving an explicit comment on the if (isSuccessful) check explaining why submodules intentionally differ from modules here — otherwise this will likely get flagged again by the next reviewer.

New in this commit — submodule name encoding. EncodeSubModuleName/DecodeSubModuleName (FileSystemModuleEstimatedTimeProvider.cs:179-209) base64-encode the submodule-name segment of the cache filename, which fixes a latent issue where a submodule name containing /, \, or other filesystem-invalid characters would previously throw when writing/reading the estimate file. DeleteLegacySubModuleFile (FileSystemModuleEstimatedTimeProvider.cs:211-223) does a best-effort cleanup of the old unencoded filename on every save — this adds a small amount of per-save filesystem I/O, but since it's a one-time migration per submodule (once the legacy file is gone, the equality check short-circuits future deletes) rather than the frequent per-call cost issue #1 fixed, that's a reasonable trade-off. Good round-trip and legacy-replacement test coverage (SaveSubModuleTime_EncodesUnsafeNameAndRestoresOriginalName, SaveSubModuleTime_ReplacesLegacyFileForSameName).

Nothing else stood out in the delta since the last review. Please double check the save-on-failure point before merging so the behavior is a deliberate choice rather than an oversight.