Show determinate download progress without changing WinGet execution - #5402
Cyranth (Cynrath) wants to merge 5 commits into
Conversation
Introduce OperationProgress (stage, nullable percentage, byte counters, measured throughput) and side-car ReportProgress/ResetProgress plumbing on AbstractOperation with a single injected clock. Reporting never logs and never touches execution, retries, or history.
Surface real byte counters from the existing HTTP download path as determinate card progress (percent, downloaded/total, measured MB/s) and report role stage markers otherwise. WinGet CLI execution, retries, elevation, proxy, return codes, and history behavior are unchanged; piped winget.exe emits no progress frames, so those cards stay indeterminate.
…egression Model and single-clock throughput rules, card mapping, loopback-server measured-speed test, and real captured winget download output proving history preservation with no invented progress.
… card The ProgressIndicator gate used card indeterminacy, which is also false while queued or before any report arrives. That hid status/queue lines and left verbose OperationInformation text stuck on the card. Gate only while a determinate report is active; clear ownership when leaving Running.
Gabriel Dufresne (GabrielDuf)
left a comment
Gabriel Dufresne (GabrielDuf)
left a comment
There was a problem hiding this comment.
Hi, I left nine comments on the pr. if you could take a look at them, it would be appreciated.
| if (canReportProgress) | ||
| { | ||
| var progress = (int)((totalRead * 100L) / totalBytes); | ||
| ReportProgress( |
There was a problem hiding this comment.
Blocking: this fires on every socket read, unthrottled.
The Line(...) call a few lines below is deliberately guarded by if (progress != oldProgress), which caps the log path at ~100 emissions per download no matter how large the file is. ReportProgress is called unconditionally, once per ReadAsync return. Each one allocates a record, takes ProgressLock, and costs a Dispatcher.UIThread.Post in OperationViewModel that writes three bound properties.
I measured it against a paced loopback server (16 MiB payload):
| server chunk | progress reports | log lines |
|---|---|---|
| 32 KiB | 512 | 104 |
| 64 KiB | 256 | 104 |
| 256 KiB | 68 | 67 |
Reports track socket reads 1:1, so the count scales with both file size and bandwidth while the log path stays flat. A 2 GB installer arriving in 64 KiB reads is roughly 32,000 dispatcher posts.
Worth noting that #5390 had coalescing for exactly this (sub-1% deltas within 200 ms) and this version dropped it. The fix is to reuse the throttle that is already here:
if (progress != oldProgress)
{
oldProgress = progress;
ReportProgress(OperationProgress.FromDownload((ulong)totalRead, (ulong)totalBytes));
Line(...);
}That still gives a smooth 0-100 bar and a usable speed readout.
There was a problem hiding this comment.
Addressed in ac9284f3dce04947d47877e6f9099451af1288f0. Progress reporting now uses the existing integer-percent gate, so UI events no longer track socket reads 1:1 (max ~101 determinate reports, 0→100 preserved). Added bounded-report coverage in DownloadOperationProgressTests.
| /// negative values mean the installer supplied no usable progress and map to | ||
| /// indeterminate rather than a fake number. | ||
| /// </summary> | ||
| public static OperationProgress FromInstall(double? percent) => |
There was a problem hiding this comment.
Blocking: FromInstall, FromUpdate and FromUninstall have zero production callers.
I grepped the whole tree excluding tests - these three are only ever constructed from OperationProgressTests. No code path supplies a percentage for the Installing, Updating or Uninstalling stages, so those branches of the model are unreachable in the shipping app.
The only live producers are FromDownload (in DownloadOperation) and ForStage (download, plus the role marker in PackageOperations).
This is the mapper contract from #5390's WinGetProgressMapper, kept in place so the COM path drops in later. I would rather not carry an unreachable public API on that basis - if the native path is proposed again it should bring its own model surface and be reviewed on its own merits. Please cut these three, plus FromStagePercent, down to what is actually wired.
Same applies to ResetProgress() in AbstractOperation_Progress.cs - no operation calls it.
There was a problem hiding this comment.
Addressed in ac9284f3dce04947d47877e6f9099451af1288f0. Removed the unused FromInstall, FromUpdate, FromUninstall, FromStagePercent, and dead ResetProgress(), plus tests that only exercised those APIs. Verified with repo-wide search.
| } | ||
|
|
||
| [Fact] | ||
| public void RealWingetDownloadOutput_ContainsNoParsableProgressSignals() |
There was a problem hiding this comment.
This test asserts that six string literals declared ~25 lines above it do not contain "%", "MB", "KB", "GB" or "B/s". It does not execute any product code, so it cannot fail for any reason other than someone editing the fixture array in this same file.
The sibling test above has the same problem from the other direction: ProgressProbeOperation never calls ReportProgress, so Assert.Equal(0, progressEvents) holds regardless of what the lines contain or what the production code does.
The regression table in the PR description cites this file as the evidence for "Detailed CLI output/history - untouched + regression test", which makes it the most load-bearing-looking test here and the one that proves the least.
If the intent is to pin the observation that piped winget emits no parsable frames, that belongs in the PR description or a comment - not in a test that can never go red. I would drop both and keep the finding in prose.
There was a problem hiding this comment.
Addressed in ac9284f3dce04947d47877e6f9099451af1288f0. Removed WingetCliOutputProgressRegressionTests; the redirected-WinGet observation is now documentation only and is no longer cited as product regression-test evidence.
| // per-frame progress text is gated out solely in that case; in every other | ||
| // state (queue, indeterminate, terminal, or no report ever received) log lines | ||
| // flow to the card exactly as before, so status/queue lines are never hidden. | ||
| private bool _determinateProgressActive; |
There was a problem hiding this comment.
OperationCardProgressState is described as "the exact card mapping ... unit-tested without Avalonia", and it gets 242 lines of tests - but the behavior that actually decides what the card shows lives here, in _determinateProgressActive and _lastLogLine, and nothing covers it.
OperationCardProgressStateTests.RetryReset_ReturnsToIndeterminate_KeepingLineForVmLogRestore documents the gap explicitly: it asserts the pure mapping keeps the stale determinate line, and its comment says the ViewModel swaps it out. So the tested type is not the source of truth for the tested behavior.
I did trace the part I was most worried about by hand, and it holds: the Status setter raises StatusChanged synchronously, and both handlers go through Dispatcher.UIThread.Post, which is FIFO at the same priority - so on the failure path the flag is cleared before Metadata.FailureMessage + "Click here for more details" is emitted as a ProgressIndicator line, and that line is not swallowed. Good, but that is subtle ordering with nothing pinning it.
One gap I did find: the constructor sync block near the bottom sets _card and _lastLogLine from current state, but never initialises _determinateProgressActive from operation.CurrentProgress. A card constructed while a download is already reporting determinate progress will let raw progress lines clobber the formatted line until the next report arrives. Cosmetic, but it is the same state machine.
There was a problem hiding this comment.
Addressed in ac9284f3dce04947d47877e6f9099451af1288f0. Fixed constructor initialization from CurrentProgress via OperationCardController.SyncInitial and added direct state-machine tests for active construction, raw-line suppression, failure ordering, retry/reset, and stage transition.
| public OperationProgress CurrentProgress { get; private set; } = OperationProgress.Unknown; | ||
|
|
||
| private readonly object ProgressLock = new(); | ||
| private Func<DateTime> UtcNowProvider = static () => DateTime.UtcNow; |
There was a problem hiding this comment.
DateTime.UtcNow is the wrong clock for measuring elapsed intervals. On Windows it advances in ~15.6 ms steps by default, and download chunks frequently arrive faster than that - so many samples land in the elapsed <= TimeSpan.Zero branch below, and the ones that do not are dividing by a coarsely quantised interval.
Stopwatch.GetTimestamp() / Stopwatch.GetElapsedTime() is monotonic and high-resolution, and would stay injectable the same way for the ManualClock tests. The EMA hides most of the jitter today, but the smoothing is compensating for a measurement artifact rather than for real network variance.
This gets less pressing if the per-chunk reporting is throttled as suggested in DownloadOperation - at roughly one report per percent, the intervals sit comfortably above the timer granularity.
There was a problem hiding this comment.
Addressed in ac9284f3dce04947d47877e6f9099451af1288f0. Replaced wall-clock elapsed measurement with injectable monotonic Stopwatch.GetTimestamp/GetElapsedTime and updated deterministic throughput tests to the new clock.
| return progress with { BytesPerSecond = null }; | ||
| } | ||
|
|
||
| if (bytes == LastThroughputBytes) |
There was a problem hiding this comment.
Minor, but worth a thought: this branch keeps reporting the last smoothed speed when the byte counter has not moved, and in practice a genuinely stalled connection produces no ReadAsync return at all, so no report is made either. Both paths leave the card displaying something like 1.2 MB/s indefinitely on a connection that is doing nothing.
That is the one place the UI states something untrue, which sits awkwardly next to the "no fake percentages or speeds" line in the description. Decaying the smoothed value toward zero once the sample age exceeds a couple of seconds - or dropping the speed suffix past that age - would keep the guarantee honest.
There was a problem hiding this comment.
Addressed in ac9284f3dce04947d47877e6f9099451af1288f0. Added throughput freshness expiry (2 s): stale speed is omitted and returns only after fresh byte movement. Single one-shot timer exists only while a fresh speed is active, stopped on stage/reset/completion/disposal, covered by deterministic tests.
| }; | ||
|
|
||
| private static string FormatBytes(ulong value) => | ||
| value > (ulong)long.MaxValue |
There was a problem hiding this comment.
Two small things in this file.
value > (ulong)long.MaxValue is unreachable - that is an 8 exabyte download. BytesDownloaded and BytesTotal both originate from long values in DownloadOperation, so the cast is always safe. I would drop the branch rather than keep a TB formatter that can never run.
Separately, the composed string above ({label} - {percent}% - {a} / {b} - {speed}) is hardcoded. Only the stage word goes through Translate, so the separator, ordering and unit placement are fixed LTR, and RTL locales get a mechanically reversed-looking line. A single translatable template with positional placeholders - the codebase already does this, e.g. "Operation on queue (position {0})..." - would let translators control the whole thing.
Also, the eight new keys are near-duplicates in pairs ("Downloading" / "Downloading..."). Since the ellipsis variants differ only by punctuation, one key plus the existing formatting conventions would be kinder to the 60 translation files.
There was a problem hiding this comment.
Addressed in ac9284f3dce04947d47877e6f9099451af1288f0. Removed the unreachable large-value branch, reduced stage translation duplication (no ... duplicates), and moved full progress composition to translatable positional templates.
| /// single-clock speed measurement, EMA smoothing, reset semantics, thread safety, and | ||
| /// the guarantee that structured progress never touches the log/history path. | ||
| /// </summary> | ||
| public sealed class OperationProgressTests |
There was a problem hiding this comment.
The test suite is about 5x this repo's own density, and it is aimed away from the risk.
| test lines | production lines | ratio | |
|---|---|---|---|
| this PR | 1,177 | 517 | 2.28 : 1 |
PackageEngine + Core on main |
21,385 | 49,124 | 0.44 : 1 |
That would be fine on its own - nobody gets penalised for testing. The problem is the distribution of the 70 cases:
- ~10 cases exercise
FromInstall/FromUpdate/FromUninstall, which have no production caller (see the comment onOperationProgress.cs). They test code that cannot execute in the shipped app. - 2 cases in
WingetCliOutputProgressRegressionTestsassert properties of string literals declared in that same file, and touch no product code. - 15 cases / 242 lines cover
OperationCardProgressState, a ~40-line switch expression - andRetryReset_ReturnsToIndeterminate_KeepingLineForVmLogRestoreconcedes in its own comment that the ViewModel overrides the result, so the tested type is not the source of truth. - 170 lines stand up a TCP listener and a hand-rolled HTTP/1.1 server to produce a single assertion. That is an integration test in a unit suite; it is the slowest thing in the run and the only part that can fail for environmental reasons (its
Disposeblocks up to 30 s on a hung transfer). - 1 case spawns 8 threads x 50 reports to confirm that a
lockworks.
Meanwhile the one genuinely subtle thing in this PR has no coverage at all: the _determinateProgressActive / _lastLogLine state machine in OperationViewModel, which decides whether Metadata.FailureMessage + "Click here for more details" reaches the card. I had to hand-trace the Dispatcher.UIThread.Post ordering to satisfy myself it was correct, and found an uninitialised-flag gap while doing it. If this feature ships a bug, that is where it will be.
The part I would keep as-is is the throughput tracker: first sample, rewound counter, stalled counter, zero elapsed time, EMA determinism, stage reset. That is real stateful logic with non-obvious corners and those tests earn their place.
What I am asking for: drop the dead-factory cases (they go away for free once FromInstall/FromUpdate/FromUninstall are cut), drop WingetCliOutputProgressRegressionTests entirely, and collapse the card-state file into a handful of cases. Then spend a fraction of that on the ViewModel state machine - the ordering guarantee and the reset-on-retry path are worth pinning, and right now nothing does.
Net that is a smaller diff with strictly better coverage of the code that can actually break.
There was a problem hiding this comment.
Addressed in ac9284f3dce04947d47877e6f9099451af1288f0. Reduced dead/redundant tests and moved coverage to the actual ViewModel state machine and throughput invariants. Focused suite is now 37/37 passing on both TFMs (was 70).
| enriched = EnrichWithThroughputUnlocked(progress); | ||
| CurrentProgress = enriched; | ||
| } | ||
| ProgressChanged?.Invoke(this, enriched); |
There was a problem hiding this comment.
The XML doc four lines up claims "never fails the operation", and this line is why that is not true.
There is no try/catch around the invocation, so any exception thrown by a ProgressChanged subscriber propagates straight back out of ReportProgress. Both call sites are inside PerformOperation, so it unwinds into AbstractOperation.MainThread's catch (Exception e), which sets result = OperationVeredict.Failure and marks the operation Failed.
The DownloadOperation case is the unpleasant one: the call sits inside the read loop, so a throwing subscriber would abort a download that was otherwise succeeding and report it to the user as a genuine operation failure, with the subscriber's stack trace pasted into the operation log as LineType.Error.
This is latent today - the only subscriber is OperationViewModel, which does nothing but Dispatcher.UIThread.Post, and that does not throw synchronously. But ProgressChanged is a public event on a public abstract class, so nothing stops the IPC layer, the toast/notification routing, or a future card implementation from subscribing with synchronous work in the handler. At that point a display-layer bug becomes an operation failure, which is exactly the coupling the rest of this PR is careful to avoid.
Either make the guarantee real:
try
{
ProgressChanged?.Invoke(this, enriched);
}
catch (Exception ex)
{
Logger.Warn($"A progress subscriber threw; progress reporting is observational and will not fail the operation: {ex}");
}...or drop "never fails the operation" from the doc comment. I would prefer the former, since the claim is load-bearing for the "observational only, never touches execution" framing in the PR description.
Secondary, same line: the invoke is deliberately outside ProgressLock (correct - holding a lock across a subscriber callback invites deadlock), but that means two concurrent reporters can deliver events in a different order than they updated CurrentProgress. Single-threaded in practice for both current call sites, so this is a note rather than a request - worth a comment saying the ordering is only guaranteed for single-threaded reporters, so nobody later assumes otherwise.
There was a problem hiding this comment.
Addressed in ac9284f3dce04947d47877e6f9099451af1288f0. Progress subscribers are now isolated per-subscriber so a display-layer exception cannot fail the package operation nor block later subscribers; callbacks run outside ProgressLock with ordering documented for single-threaded reporters. Added regression tests.
- Throttle DownloadOperation progress to integer-percent gate (bounded UI events, 0/100 preserved) - Remove dead FromInstall/FromUpdate/FromUninstall/FromStagePercent and ResetProgress - Delete WingetCliOutputProgressRegressionTests (fixture-only) - Fix OperationViewModel constructor determinate init via OperationCardController - Add direct controller state-machine tests (construct, failure order, retry, stage) - Switch throughput to monotonic Stopwatch timestamps (injectable) - Add stale-speed expiry (2s, timer only while fresh, disposed cleanly) - Isolate ProgressChanged subscribers (observational, per-subscriber try/catch) - Localize full progress templates, drop duplicate ellipsis keys and TB branch - Shrink suite to focused coverage (37 tests, both TFMs)
|
Hi Gabriel Dufresne (@GabrielDuf), I’ve addressed all nine review comments in ac9284f. I also replied to each thread with the corresponding change and verification. The PR is ready for another review when you have time. Thanks for the detailed feedback. |
2 participants
This is a clean reimplementation following the feedback on #5390. It keeps the existing WinGet CLI execution path intact and limits the change to observational progress reporting.
winget.exeremains the execution path; no WinGet COM install/update/uninstall execution was added.What it does
Operations with real byte counters (the existing HTTP installer-download path, which already reads
Content-Lengthand cumulative bytes) now surface determinate cards, e.g.Downloading · 21% · 10.0 MB / 46.7 MB · 1.2 MB/s, with speed measured generically as Δbytes/Δtime on one injectable monotonic clock (Stopwatch.GetTimestamp/GetElapsedTime). Reports are gated by the existing integer-percent change (max ~101 determinate reports per download, 0→100 preserved), so UI events no longer track socket reads 1:1. Role stages (Installing,Updating,Uninstallingvia translatable{0}...template) render indeterminate while running.Raw capture (winget v1.29.290,
winget downloadwith stdout redirected) showed zero progress frames: six plain CR LF lines over an ~8.5 s download, empty stderr, no ANSI, no counters. Pipedwinget.exetherefore stays indeterminate by design — there is nothing reliable to parse, and no parser was added.Regression evidence
All rows: existing path structurally unchanged (
git difftouches none of these files)._runOperationloop unchangedPrepareProcessStartInfo,ApplyElevationRequirementsunchangedWinGetPkgOperationHelper._getOperationResultunchanged--versionconstruction unchangedWinGet_DropArchAndScopepath unchangedWinGet.GetProxyArgumentunchanged0x8A150109branch unchanged0x8A150011branch unchangedLastReturnCodepath unchangedLine/GetOutput/history store unchanged (no invented progress; piped-WinGet observation kept as prose only)Checks
dotnet build src/UniGetUI.Windows.slnx /p:Platform=x64: 0 errorsnet10.0andnet10.0-windows10.0.26100.0)PackageEngine.Tests: only pre-existing environment failures, each reproduced on cleanorigin/main(OperationHistory tail-marker, WinGet explainers, PowerShell launcher, Scoop/PS call-args wiring)dotnet format whitespace/style --verify-no-changes: cleanVerify-Translations.ps1: clean (no placeholder issues); English sources use positional templates ({0} · {1}% · {2} / {3}[ · {4}],{0}...) so translators control ordering/separators; stage keys reduced toDownloading/Installing/Updating/Uninstalling(no punctuation-duplicate...keys)DownloadOperationcoverage plus real-clock manual verification of 0→100 determinate flow; history record intact