From 9bf4e68ac8d77665bce1cc195746f7101c5ddcf4 Mon Sep 17 00:00:00 2001 From: smb Date: Sat, 12 Sep 2026 23:00:30 +0200 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=90=9B=20fix(models):=20track=20model?= =?UTF-8?q?=20availability?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 🐛 Defects: - `model_available` was never derived from the weight being on disk. It was set only by a `ModelAvailabilityChanged` event, which the Model Manager dispatches, or as a side effect of a preview advancing from `PREPARING_MODEL` to `PREVIEWING`. A preview that failed, timed out or was cancelled after the download finished therefore left the flag `False` although the weight was cached — and all three surfaces that read it agreed on the wrong answer: the action button kept offering `Download & Preview`, the status row kept reading `Not cached`, and the download notice kept demanding 927.6 MiB. Only opening the Model Manager corrected it. - `PreviewController._notification` now dispatches `ModelAvailabilityChanged(True)` when the download stage reports completion, so the fact is recorded independently of what the job does next. - `ModelDownloader._download_locked` emits its terminal `(size, size)` progress **after** the checksum is verified and the file is atomically promoted, never before. A weight that fails verification is deleted, so reporting it as available on bytes received would have been worse than the stale label. This also makes completion observable when the server sends no Content-Length. - 🧪 Tests: - `test_completed_model_stays_available_when_preview_setup_fails` and `test_completed_model_stays_available_when_preview_is_cancelled` pin the flag together with the button, the status row and the download notice, so the three cannot drift apart again. - `test_download_reports_completion_after_verified_weight_is_promoted` pins that the callback does not fire until the target file exists, and `test_unknown_content_length_reports_post_promotion_completion` pins the case the old progress path could not report at all. - 📝 Copy: - `Herunterladen & Vorschau` was a word-for-word rendering of `Download & Preview` — a bare infinitive beside a noun, which is not how a German action label reads. It is now `Modell laden & Vorschau`, naming what the button fetches first. `%s — %s Download erforderlich` becomes the sentence `Für %s ist ein Download von %s erforderlich`. Trigger: repro — select a model that is not cached, start a preview, let the download finish and the preview then fail with `segmentation process timed out`; the inspector still reports the model as not cached and the button still offers to download it. Closes #167 --- resources/matteloop_de.qm | Bin 69633 -> 69659 bytes resources/matteloop_de.ts | 10 +- resources/matteloop_en.ts | 6 +- src/matteloop/jobs/models/download.py | 3 +- .../ui/preview_controller/controller.py | 7 ++ tests/jobs/models/test_download.py | 27 ++++- tests/ui/test_preview_controller.py | 101 ++++++++++++++++++ 7 files changed, 143 insertions(+), 11 deletions(-) diff --git a/resources/matteloop_de.qm b/resources/matteloop_de.qm index 9ea45c2a78cbcd445881a0ed63c855a3be88d98f..d309ea027c25ea2291bb5c3c1d2091d0982278d3 100644 GIT binary patch delta 4335 zcmX9>30zI-8-DJ&=bXErI}}ERq^x6!Xc%I&m`IkgWN9pkjL8y0lbdO=mo1Yml8mA< zw(Mg#sWZa0nQyEM(}?;;C2PL&p>O9QeUiMhbVZV)ra0MbJ%hIFvv#cGJV_5x8& zAeQb2E+2+8V==IEGNg!2z~&%G2Wx}Ds9oXJwiEE?34S;k05rXTABy6LOVGE?uYlMJ zeY+$BKEGLU?j0+>$w2=gSHR&61~176Zf7EJ#8E&Qg`vxmVL{dj49`0R)IE=&jTYdL zj}=d)VZ;umt9}QJy88@>{Q;vt?O-|Vt$3pa#x`&u;jIXLbeS1&!VDK@e%4v>=VHwL zvR>W?EY02pY-oto>9r(!0h_TF81yI7lS+BRmFGAZNxU=_N1S-Wk^~eKg#npoaQ&P4 zK;t90;p+i(Pr>UDVCDnvT)^4qbYG$Pf zjE}bBwCAe0v5$ZOCRK{Z7a>kltqb%9;zL#2#svdE9#dsM+zyyVs`fVN4BV4cN4{MM zoE)pVyqf`HjjF_X8_@Bfs!U@8{Q6q;$l_W8OsFFWX;X9}}2So5*Ng~%!ih@K=^ykdb>5klOY#z5{g zAugZV7~fxryGs1*E+kbkps*gohGn+_-!p<`>y)=Zo32968wGgpC+yCs1@BxZ1v59Q8!8m7B9ZA8R?MFx-0avMINe&fm)r!fcM_f! zp5Vq2!n4lpD85@lWyjML<5VmD9Br{)BqR&39Gg*<*VO9o$|<(9YH=u2UH_%pZe9p$ zT&s4-ZvbfG)Qwh<=pePaSuF`I-l}eCQw-Q8tG{swXN?W&_B|=);&*C)ix-gkN*%hx z19>f7H11I-($?+1A@@{d-W@tCZm zMT-vI`29XN(KDEVoZT)4oA~?D6ftb%HunF5bz-=}6kly6CK!k#JBmww;txfkVxku# zo$+2w@jVLkF1BLibSrMPwc^cb;?9f=8qsiZXF+{nN`@6rd5ODrj%HWg5O>{lW;b;c zbN8TI66zb zy?Q>dJ6^1;sRj(Uta$mE_@ab0zhWnT4&e6_T_rp=1O0nS+7bT1sH2iYEZZ^eYsvZd z7@*TJsZ~`XyDL&^-7}SK;e17Ed+{PubWv*G(*xLaNph82(31a_I{)tmH85K0p6CJ; z_KE&wPKi^(PQ9TAyA&VA)PSj+PtS>GBrdede(a*HX!Lm}18EL&zwvA!! zofNrYKSm~G%68rE(V}n3j!DV1+}U!AjCMfaboo0U1z0{;?(R(^n_Vb(pXdvyms&9| zN%l$MdR0s1J}nu@yoR!$@H>6ylx!JThs-?R%9D0;!7CZ^n0m?=%v4{Y>Ni zh8h_+Qsa|K0ww91etC&3)pm{FnkMv*$C|;}RV-zZW?JBv02XRy=7s|Yo@nO9=z#Al zG@G}0vb*|fGM7iQdlEF+U4tp!fF7EhCR}iJeNBE&1P_H>nv*TcNhI5fF(Wkv>-qbz z^_rp~1AvNYn){o%@YFt5{3}KCDmore-L&HL<(hXIYGn96t*J<%aph^h+QGA>=WMO5 z%X`3>tF<5K1$+w9I`3)6{$CiTZJ#}X>fNdBIN~l(t&dvw-)-40Uu(NPrAnKrv_8H} z^|?pdK5f{J;jOg3y5qo;Vy$mkEwH`2HmGS1a4bw4lx8)%7p~02k@nh}=n!TwN?WUP;9(SC#T9y;CYJ;~2I{o7 zRh$=E>aqh1=lK4-$DJ|MjTsFE{Sx&%@ zK5AY(wUDEa%Xa2W_ffwhg)>~#UHw|8SPmfdt$3-eetp}M9J7DcZF+cl@jvS6?<|a9i8t$?9Omcn41--amY`Rj#b96I#xXn6(E3CtV8u&A zTL-2#^uD28S4Oz5$k5$oKXAXo;8BwUcvTn%B~!iCv4)_t*MP~Z4Wm{*;;A>-FmV|- zzQ4&3wwlF!ymHjpvmiDsVn9WP`ynhD3(fF)S#!PUEv|wq8W$7!u~N zHcq1qsV5nsTcIHn3Xo=R$Xa-xPPNmJSG^j@Ibt|!L&6(N4f#ul0RP$=3cT6@|4cOe zJs<}NPBOf!zREzh7ji%txb}Egw8_ z&FHRa2GoQY-5ZYQIbFxt<1dku%n+l0hf*H9{fq&PQaSmA7)KA~?@{ZFV_p6RY~~xs z|A$wNlb4WS-Z105gt43#T#d1HH_;)d8sk(Rd?RXWT(ql--Bn^-IfmBzVuCTf1kw{i6(`ExT(rkb!ZB;BsJiRG>D+iPj zXH45NEfmGKi%gfDdNSp|m>xb{%l_PMdK$~V-4tl5{DmU3O;vC$k}nSViaMP45A90a z1}xmGsJAl{*F{Af?Zw%yi6Tak*yD4Gm@t=C9;g`X89>-YOU9Qq< zLK5)ww9?x0l+4uMDs67ko6q|yZY`ZDl1)mt?@t2T#wz|!WLj2V3EEi2m&kF-$Q|6^ z$3{xHr#11)DL2amfBw-wamHTe19i5w<|dPowqj*ri;W?#;2dzH+%o2%XMdxsub2FBs)Y*(Zg@(@D9z zgPG{EM0rr)MuH{Ev&nBLvP`A&Ykv+f&Sv>SILGV3X3eO*e8I3c>jX9ZqQ+dWF#}s4 zVz&ECB8OwlEz)MQoBCPI9jDBqx{sRsY}n6~!94ICJ^a8X^Tb*bv_zUG`OT!~-!{*9 zaFMfOuzBtutvHB;n&T%{Go>HQi+rj$oZ6e0o#XQl7nxI3Bp&KyUcdHB8+w{kYgTdi zoM+CS#L^{Jne(oa(7iF{{2M<4&zgst&!uzWIfD7&d19MO=5nWW?tIhy=)1do*V`J# zCZDu-*H_s>`=Ri`XUu>jToA#(Vf;IjIDuE;L@!K5B(J6r$0B5P)5$h_EjDH2%d0b8T8-DJ&=bXErYY8JEm7OS&wJeooOpI-0$ri~pSrW2clx!(W`B@`jQpyst zFGbeuDTeH0iOh^JmhyYd`Qv_`bI{S8~pQmEs z@|E#5YzffzyaQbN2yw}LF5U<+mIq|W#NEIZM~FK|0#creL3S!$xeIYu7BEYKSablm zwgu9ZB|v&#Na34+&EAj>l>-F>P}46CSa}mQ=M4kabwa(bp}_wN;L`jFU{gM!na?w_ z`;1mUB>~mCsu}W zEEOYjReZM$KLxk}4(aH(GzYjBj{!rD0ZL~KicN$KX*)1D`!G;tHv%@=fWysHJhKQx zQt7Vhb{JOt5{Poe@Gq&r58qTQv|v;X2NI6KxTn|X0X?SFcLD6)sQB9vM1J2d`vI0^ z?4oDCA!%|2iSEH>tO0&bK}viP7hJ!FL(_>@2jZv`7c31$UVaF$GXuAqF_v{Uqp+tJ z&~Y~YD9I&JGhW{H2WlM0hjQYh8XDX=0nGH%H0a+A=%1!(@@h6U(Lm#2?*zOF(s=CU zK5myZJ-ID{dJw4z*;CV7vNtu=$cP60l9X##zN0NqQ?*y)Vj8W+vf5)l|PO2tXn zG|{7;0{(w%)_Q#x;vmhs0lvV(-kNQr1A!6AnvBOg0E@RKt8QzcxI}Zb?IIwzyXM*+ z8i)i7a(37F$A*nFaZRRKcuj5@&XK|*v6wK2v` zh`vGmT3d)Or9mOi!iLy;K+kl+wl(xU(6qLY`Az};br<$*uK=r~ky)RKi4=TGtA0O4h8 zSBmenP~v`;V(hQtZ)0rgMa(SWt)nwlnXT1!ctWx5){2AZ>gw0E_A`SRaXXsl*(Pe&r!bv@KWURLECYVb&}Q%E3&Vr7`6~oqbh-An?=xVl zr}kclK%jv}`!JvzEiYCv^nvzKc`0x+N&6x?2{`&l`>&P<+|Lv3s^4UR8YMck=l%Wm zVz)pVav@d>wD9?{xnjuBZOs4qHZe@0i*H!P7!z@5b#dvhe30K;jPs$TQ;Nm4J&yrh z52-l)7ZsB~t9W;in7(~G3z3(Yo>v_RU8drhCgQI2;moSz;;y?c%qB;1_kq>G;uB)d zLhf65Ks>|uY?wVqJiDNR;!F_F3;TiDN5m`B76QK}s~DXr7DX|}b5@A=R?h|Y{3@1| zmjR}eDqg!PzAj+QuS5LWpZ9}nNO)!iesY%dL;QhZTO@}lrejovpAb^S-UR)LWnj-d~pbOu7ZEE|dn(VER>FC5?HN3Hp@8}IYYa?w8Dkia5DP#PXV(W?W`HduY zW2Rg*F_+mPs92sTm&DAaYmdrhehDlz&T`q23T9KX&h8F9)S^h&bU8h6VzbWeYaO6p zq^|uR47G#JqF&hE>pHEag_W-AymDD82WWKORoAgw_0ahylfd6sb-wSYk)%8xO3yi;{>z7f=Q0~=D8t~nK-*i)VhXDsK>t@a~09`KWHgD<1>}sjo zxqJ?@XM!%HT_DBVzn(6$E;n5DRhN?)&Ze+Rm)r0OiL6v{UKd^7dOrVUt}cIIAK>{Q z-NVh?cw%!EPtVo8owE?o+*NUMxbB0F8X3G+Z^>6!xHjo4rLwnp57FDz{}(W?*4OIm z1AOt;yX&U>RqXKA5siFc&PXM-HtW9Lf`%cRqFIi@7I&AzIajJy(!Z% z%&PBcI05{5MBnpH1+c?OA5bq7INnDekYQoE#;JHKK_8M_6_^*JpHxC|r%cpGP2>Nw zXZ1^Ou@0o&)5pELV*_ID=#Si|t4E*KA1Np5TI=)9Pvyaz^yeC$W%@kTpX*3s&mZZp zy`?AYE9(nnTGTs5|Jse7I9g3#J|~EIKT2PrabPoQr(*nH2HkEF^ztz1?MgW>m<{^0 zIN)sygC&jkeKQO-Cfdlnem6s-0b4mb9X2#+_nu;NHF!0>!{+kL(51FBCmN%nUnKL` z-(VQnh9TGhf$`G z)x?HXOd&hFu~dtu$Y*gZJO!p%1Lgt)Z6@1D68Yu5X@1^q7Cu{)dNDo06f=Xdaq4VJ z%B6)KyG%Qw0LdCt+Mil(TFgL-5&@=i>@|{K@oJpG+Vy z-1MRB1`UZZ8*Wtt?#7!fow)97U$gRtn0LioFGb5bc+fmy zDG6qGGS7?|#d)EcIjYJgR>*G?9X78V$^bxO99qJX@*nQI zV19m*9~wtY=Fc}0=<4Pc;}?I{g))o9A&#G10T%ll?%S-u;%Q$D=v!Ncj+)E-cNHzc zE*kdzaLf4b!)DiymMLqfR`VN6xZ?mem*bX*o#Qx?6Yz<-=S#3xPDM=;x>k8oub`A zPZYdT#Nj@i?IcB{os$>_)LFihYSEN4rF&@z{9aMY__& z_JYi`4ocIztj(8NC?1VmD3U0peV1HdTX)6ZiA;;WDghfy`4QPe8JfxiMrf44vsGz{ zt1>d%j-%FIWx@f8Su;w-LnoD;qn~hGPgF9t)CHd2R}Q$J02&Qa4jqqXy(v*n_uS6N z`6{;mE<%EpoMe{X)K$vW$-eB@pOtH!f>`NlE7vof`GIjxx${L~;i;h%r_vK`!jwmO z9wc~Fc{$-7MHa7={NT?4#%Prt3$)q_p;i5C6OcJ ztPPW=F`L|MR`<{ds(Y)o`-TG?nf|o){lFT2Fv=QSL4vm7*73ckvgV(%PI+{Nvtn0k z{M0!~l2A9#L53Gy*N;#Zrtg#pQ{^Ky~S`CSh(_7cC`JRT2tx4spfasCdjPVRz z+%0SN4HA0bXU!=b0laL`+j=pD8&7{>eSC@7G|T$LDTOEBwLa}o4D^W!0lxXKj?ZnI s^|VriP`N$oBOH#7c5qa$R^Ob~D(i67qocJg(PQ<*@jqlOpBA_7{}5a^;{X5v diff --git a/resources/matteloop_de.ts b/resources/matteloop_de.ts index 8e9e951..ba2f33e 100644 --- a/resources/matteloop_de.ts +++ b/resources/matteloop_de.ts @@ -30,7 +30,7 @@ Original-Videobild - + Crop bounds: x %s, y %s, width %s, height %s source pixels; source %s × %s Zuschnittgrenzen: x %s, y %s, Breite %s, Höhe %s Quellpixel; Quelle %s × %s @@ -354,7 +354,7 @@ %s — %s download required - %s — %s Download erforderlich + Für %s ist ein Download von %s erforderlich @@ -1124,7 +1124,7 @@ Freed %s. Download & Preview - Herunterladen & Vorschau + Modell laden & Vorschau @@ -1273,13 +1273,13 @@ Freed %s. Video wird gerendert - + Previewing selected frame Ausgewähltes Bild wird in der Vorschau angezeigt - + Preparing model diff --git a/resources/matteloop_en.ts b/resources/matteloop_en.ts index 53126ae..7270123 100644 --- a/resources/matteloop_en.ts +++ b/resources/matteloop_en.ts @@ -30,7 +30,7 @@ Original video frame - + Crop bounds: x %s, y %s, width %s, height %s source pixels; source %s × %s Crop bounds: x %s, y %s, width %s, height %s source pixels; source %s × %s @@ -1273,13 +1273,13 @@ Freed %s. Rendering video - + Previewing selected frame Previewing selected frame - + Preparing model diff --git a/src/matteloop/jobs/models/download.py b/src/matteloop/jobs/models/download.py index ca04dd3..97eac56 100644 --- a/src/matteloop/jobs/models/download.py +++ b/src/matteloop/jobs/models/download.py @@ -234,7 +234,7 @@ def _download_locked( except OSError as error: raise _disk_error(spec.id, error) from error digest.update(chunk) - if known_total is not None: + if known_total is not None and completed < known_total: progress(completed, known_total) _raise_if_cancelled(cancelled, spec.id) if completed != artifact.size_bytes: @@ -292,6 +292,7 @@ def _download_locked( raise _permission_error(spec.id, error) from error except OSError as error: raise _disk_error(spec.id, error) from error + progress(artifact.size_bytes, artifact.size_bytes) return target except BaseException as error: try: diff --git a/src/matteloop/ui/preview_controller/controller.py b/src/matteloop/ui/preview_controller/controller.py index 92584d4..bd99d85 100644 --- a/src/matteloop/ui/preview_controller/controller.py +++ b/src/matteloop/ui/preview_controller/controller.py @@ -17,6 +17,7 @@ JobKind, JobStageChanged, JobState, + ModelAvailabilityChanged, ModelPrepared, PreviewFailed, PreviewRequested, @@ -260,6 +261,12 @@ def _notification(self, notification: object) -> None: if self._store.state.job.job_id != job_id: return if isinstance(notification, ProgressEvent): + if ( + notification.stage is ProgressStage.DOWNLOADING_MODEL + and notification.total is not None + and notification.completed == notification.total + ): + self._store.dispatch(ModelAvailabilityChanged(True)) if self._store.state.job.phase is not JobState.CANCELLING: assert self._dialog is not None self._dialog.set_progress(notification) diff --git a/tests/jobs/models/test_download.py b/tests/jobs/models/test_download.py index 0f16655..fce2a2d 100644 --- a/tests/jobs/models/test_download.py +++ b/tests/jobs/models/test_download.py @@ -131,14 +131,37 @@ def test_download_streams_to_versioned_part_verifies_and_atomically_promotes( assert response.closed is True -def test_unknown_content_length_emits_no_byte_progress(tmp_path: Path) -> None: +def test_download_reports_completion_after_verified_weight_is_promoted( + tmp_path: Path, +) -> None: + data = b"verified-model-bytes" + target = tmp_path / "2.0.75" / "u2net" / "u2net.onnx" + completion_target_states: list[bool] = [] + + _download( + tmp_path, + data, + response=FakeResponse(data, headers={"Content-Length": str(len(data))}), + progress=lambda done, total: ( + completion_target_states.append(target.is_file()) + if done == total + else None + ), + ) + + assert completion_target_states == [True] + + +def test_unknown_content_length_reports_post_promotion_completion( + tmp_path: Path, +) -> None: events: list[tuple[int, int]] = [] _download( tmp_path, b"abcdef", progress=lambda done, total: events.append((done, total)) ) - assert events == [] + assert events == [(6, 6)] @pytest.mark.parametrize("cancel_after_call", [1, 3, 6]) diff --git a/tests/ui/test_preview_controller.py b/tests/ui/test_preview_controller.py index f6567b0..8ffad95 100644 --- a/tests/ui/test_preview_controller.py +++ b/tests/ui/test_preview_controller.py @@ -15,8 +15,10 @@ from matteloop.core.errors import AppError, ErrorCode from matteloop.core.specs import CropSpec from matteloop.core.state import ( + CancelAcknowledged, CancelRequested, JobStageChanged, + ModelAvailabilityChanged, ModelPrepared, PreviewRequested, PreviewState, @@ -111,6 +113,42 @@ def close(self) -> None: return +class DownloadedModelFailureRuntime(FakePreviewRuntime): + def prepare( + self, model_id: str, extras: dict[str, object], context: JobContext + ) -> PreparedSegmentation: + super().prepare(model_id, extras, context) + context.progress( + ProgressStage.DOWNLOADING_MODEL, + 128, + total=128, + detail="128 / 128 bytes", + ) + raise RuntimeError("segmentation process timed out") + + +class DownloadedModelCancellationRuntime(FakePreviewRuntime): + def __init__(self) -> None: + super().__init__() + self.downloaded = Event() + + def prepare( + self, model_id: str, extras: dict[str, object], context: JobContext + ) -> PreparedSegmentation: + prepared = super().prepare(model_id, extras, context) + context.progress( + ProgressStage.DOWNLOADING_MODEL, + 128, + total=128, + detail="128 / 128 bytes", + ) + self.downloaded.set() + while not context.cancellation.requested: + Event().wait(0.01) + context.checkpoint("model-preparation") + return prepared + + class BlockingPreviewRuntime(FakePreviewRuntime): def __init__(self) -> None: super().__init__() @@ -295,6 +333,69 @@ def test_preview_request_uses_the_selected_oriented_crop(tmp_path: Path, qtbot) controller.shutdown() +def test_completed_model_stays_available_when_preview_setup_fails( + tmp_path: Path, qtbot, request +) -> None: + path = tmp_path / "source.mp4" + path.write_bytes(b"fixture") + runtime = DownloadedModelFailureRuntime() + store = RecordingStore() + store.dispatch(ModelAvailabilityChanged(False)) + controller = SourceController( + store, + source_adapter=FakeSourceAdapter(path), + preview_runtime=runtime, + ) + request.addfinalizer(controller.shutdown) + window = MainWindow(store, controller, _settings()) + qtbot.addWidget(window) + window.show() + + controller.dispatch(VideoDropped(path)) + qtbot.waitUntil(lambda: store.state.source.value == "ready", timeout=5000) + controller.dispatch(PreviewFrameRequested()) + + qtbot.waitUntil(lambda: store.state.preview is PreviewState.ERROR, timeout=5000) + + assert store.state.model_available is True + assert window.preview_button.text() == "Preview Frame" + assert window.inspector.model_status.text() == "● Ready" + assert not window.inspector.model_download_notice.isVisible() + + +def test_completed_model_stays_available_when_preview_is_cancelled( + tmp_path: Path, qtbot, request +) -> None: + path = tmp_path / "source.mp4" + path.write_bytes(b"fixture") + runtime = DownloadedModelCancellationRuntime() + store = RecordingStore() + store.dispatch(ModelAvailabilityChanged(False)) + preview_controller = PreviewController(store, runtime=runtime) + controller = SourceController( + store, + source_adapter=FakeSourceAdapter(path), + preview_controller=preview_controller, + ) + request.addfinalizer(controller.shutdown) + window = MainWindow(store, controller, _settings()) + qtbot.addWidget(window) + window.show() + + controller.dispatch(VideoDropped(path)) + qtbot.waitUntil(lambda: store.state.source.value == "ready", timeout=5000) + controller.dispatch(PreviewFrameRequested()) + qtbot.waitUntil(runtime.downloaded.is_set, timeout=5000) + dialog = preview_controller.dialog + assert dialog is not None + qtbot.mouseClick(dialog.cancel_button, Qt.MouseButton.LeftButton) + + qtbot.waitUntil(lambda: store.state.job.phase.value == "idle", timeout=5000) + + assert store.state.model_available is True + assert any(isinstance(event, CancelAcknowledged) for event in store.events) + + @pytest.mark.parametrize("use_escape", [False, True]) def test_cancel_keeps_modal_dialog_open_until_the_safe_checkpoint( tmp_path: Path, qtbot, use_escape: bool From a46fd9ce225c0ddb9a7b9a43dfd360466ff6911e Mon Sep 17 00:00:00 2001 From: smb Date: Sat, 12 Sep 2026 23:12:40 +0200 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=90=9B=20fix(models):=20report=20cach?= =?UTF-8?q?ed=20weights=20as=20complete?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 🐛 Defects: - Two early returns leave a verified weight on disk without reporting the terminal progress: the pre-flight check before any request is made, and the post-download check after a concurrent writer won the race. A caller that reads that progress as "the weight is usable now" never hears it on either path. - That is the same defect this branch exists to fix, on the cached path: `ModelChanged` sets `model_available` to `False`, and selecting a model that is already downloaded then takes the pre-flight return, so nothing dispatches `ModelAvailabilityChanged(True)`. If the preview that follows fails, times out or is cancelled, the inspector again reports a cached model as missing and offers to download it. - Every path that leaves a usable weight now reports completion. - 🧪 `test_an_already_cached_weight_still_reports_completion` downloads twice and asserts the second, fully cached call reports completion as the first did. It fails on the pre-flight path without this change; the post-download path was found by that test rather than by reading, because fixing only the reported line left the test red. Trigger: reported by the review bot on PR #168 — the cache-hit early return at `download.py:266` skips the terminal completion progress. Refs #167 --- src/matteloop/jobs/models/download.py | 10 ++++++++++ tests/jobs/models/test_download.py | 17 +++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/matteloop/jobs/models/download.py b/src/matteloop/jobs/models/download.py index 97eac56..e65afb5 100644 --- a/src/matteloop/jobs/models/download.py +++ b/src/matteloop/jobs/models/download.py @@ -176,6 +176,11 @@ def _download_locked( if _entry_exists(bound, target_name, spec.id): if _verified_file(bound, target_name, artifact, cancelled, spec.id): _assert_bound_named(bound) + # Every path that leaves a usable weight on disk reports the + # terminal progress, because a caller reads it as "the weight + # is now usable". Skipping it here would make selecting an + # already-cached model report the model as missing. + progress(artifact.size_bytes, artifact.size_bytes) return target _safe_unlink(bound, target_name, spec.id) _remove_stale_part(bound, part_name, spec.id) @@ -267,6 +272,11 @@ def _download_locked( if _verified_file(bound, target_name, artifact, cancelled, spec.id): _safe_unlink(bound, part_name, spec.id) _assert_bound_named(bound) + # Report completion on this path too: a caller that + # treats the terminal progress as "the weight is now + # usable" would otherwise never hear it for a weight + # that was already cached and verified. + progress(artifact.size_bytes, artifact.size_bytes) return target _safe_unlink(bound, target_name, spec.id) _before_model_promotion(bound) diff --git a/tests/jobs/models/test_download.py b/tests/jobs/models/test_download.py index fce2a2d..a746f6b 100644 --- a/tests/jobs/models/test_download.py +++ b/tests/jobs/models/test_download.py @@ -164,6 +164,23 @@ def test_unknown_content_length_reports_post_promotion_completion( assert events == [(6, 6)] +def test_an_already_cached_weight_still_reports_completion(tmp_path: Path) -> None: + """A cached weight takes the early return, and a caller that reads the + terminal progress as "usable now" must still hear it there -- otherwise + selecting an already-downloaded model reports it as missing. + """ + data = b"abcdef" + first: list[tuple[int, int]] = [] + _download(tmp_path, data, progress=lambda done, total: first.append((done, total))) + assert first == [(6, 6)] + + second: list[tuple[int, int]] = [] + + _download(tmp_path, data, progress=lambda done, total: second.append((done, total))) + + assert second == [(6, 6)] + + @pytest.mark.parametrize("cancel_after_call", [1, 3, 6]) def test_cancellation_before_between_and_after_native_reads_cleans_part( tmp_path: Path, cancel_after_call: int