Fix eta calculation - #166
Conversation
- Exclude 'ok', 'fail', 'aborted', and 'stopped' files from remaining ETA byte count - Remove unused t_start variable and clarify ETA smoothing window comments - Add test_snapshot_eta_ignores_terminal_states test case
Remove unused variable t_start from eta calculation.
LeyckerS
left a comment
There was a problem hiding this comment.
@8nt0n — a lot of this is right, and the parts that are right are the parts that were hard. Decoupling the ETA rate from the live-speed window, returning None instead of the 7200 sentinel, teaching fmtEta to render --, and excluding terminal states so a failed partial download stops contributing ghost bytes — all correct, and the terminal-state filter is something I had not thought of.
Two things block, and I verified both by running them rather than by reading.
1. self._tracked is iterated outside the lock
Both new loops run after the with self._lock: block has closed:
for record in self._tracked.values():
if record.file_bytes > 0:_track() writes to that dict from the asyncio worker thread (moon_engine.py:519), and snapshot() is called from the caller's thread about twelve times a second. Iterating a dict while another thread inserts into it raises. Reproduced on your branch — one thread calling _track() in a loop, one calling snapshot(0):
RESULT: RuntimeError: dictionary changed size during iteration
The same probe on main ran to completion with no error, because main never iterates the live dict.
The fix is already in this file, sixty lines above yours. _files_payload() does exactly the right thing:
def _files_payload(self) -> list[dict]:
with self._lock:
tracked = list(self._tracked.items())
# ...everything else works on the copyTake a copy inside the existing lock block — snapshot() already holds it a few lines earlier to read the counters — and iterate the copy.
2. Files that have not been picked up yet contribute nothing
dl_size_left sums only records in _tracked, and a link enters _tracked when a worker picks it up (moon_engine.py:156 and :235), not when it is queued. So everything still waiting counts as zero work remaining.
Measured on your branch — 100 files of 100 MB, ten picked up, none finished:
speed reported : 6.4 MB/s
eta reported : 111.1 s
files : 0/100
The real figure is 10,000 MB at 6.4 MB/s, roughly 1,560 seconds. The ETA is short by about fourteen times, and it will climb steadily through the run rather than falling — the estimate going backwards is the failure mode users notice most.
main avoided this by multiplying files_remaining = dl_tot - dl_done, which counts queued files even though its per-file average was wrong. Your avg_file is computed correctly but is only applied to tracked records whose size is unknown; the untracked ones are missing entirely. The formula needs its second term:
dl_size_left = Σ (file_bytes − done_bytes) over tracked, non-terminal records
+ (dl_total − dl_done − tracked_non_terminal_count) × avg_file
That is the shape I described on #85 and I should have been clearer that the second line was load-bearing rather than a footnote.
Two smaller things, neither blocking:
The new test uses now = time.time() while the engine filters on time.monotonic() — the test in the same file directly above it uses monotonic. It passes today only because wall-clock values are enormous compared to monotonic ones, so every sample lands inside the window by accident. Match the existing test and it will keep testing what it says.
Two unrelated blank lines are removed, after self._proxy_status = "empty_file" and after self._last_proxy_check = now. Harmless, but they are not part of #85.
None of this is a rewrite: the first is a two-line change, the second is one added term. The structure you built is the right one, and the terminal-state filter is a genuine improvement I want to keep. Push when ready and I will re-run both probes against it.
Fixes #85
Description
This PR resolves multiple accuracy, stability, and edge-case issues in the ETA calculation logic (
moon_engine.pyandapp.js):raw_eta >= 7200to returnNone(instead of the7200sentinel), allowingapp.jsto properly render--.file_bytesfor in-flight files and applying average estimates only to pending files without known sizes.ok,fail,aborted,stopped) from_trackedso failed or aborted partial downloads no longer contribute ghost remaining bytes to the ETA.t_start), updated window comments, and added unit tests intest_snapshot_speed.pycovering clamp behavior and terminal-state handling.Type of change
Checklist
moon_cli.py