Skip to content

Commit d1d4d93

Browse files
committed
fix: handle FileLock timeout explicitly, rename misleading var, align lint ignores
- Catch filelock.Timeout separately from generic errors so operators see an actionable message when a pod is stuck holding the download lock - Rename each_history → redirect_resp (response.history entries are redirect responses, not "histories") - Add E402 and UP to tests_unit/ ruff ignores to match tests/ config - Add test for FileLockTimeout → ModelFetchError propagation
1 parent 3a81b55 commit d1d4d93

3 files changed

Lines changed: 67 additions & 42 deletions

File tree

nc_py_api/ex_app/integration_fastapi.py

Lines changed: 50 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
)
2222
from fastapi.responses import JSONResponse, PlainTextResponse
2323
from filelock import FileLock
24+
from filelock import Timeout as FileLockTimeout
2425
from starlette.requests import HTTPConnection, Request
2526
from starlette.types import ASGIApp, Receive, Scope, Send
2627

@@ -207,48 +208,55 @@ def __fetch_model_as_file(
207208
) -> str:
208209
result_path = download_options.pop("save_path", urlparse(model_path).path.split("/")[-1])
209210
tmp_path = result_path + ".tmp"
210-
with FileLock(result_path + ".lock", timeout=7200), niquests.get(model_path, stream=True) as response:
211-
if not response.ok:
212-
raise ModelFetchError(
213-
f"Downloading of '{model_path}' failed, returned ({response.status_code}) {response.text}"
214-
)
215-
downloaded_size = 0
216-
linked_etag = ""
217-
for each_history in response.history:
218-
linked_etag = each_history.headers.get("X-Linked-ETag", "")
219-
if linked_etag:
220-
break
221-
if not linked_etag:
222-
linked_etag = response.headers.get("X-Linked-ETag", response.headers.get("ETag", ""))
223-
total_size = int(response.headers.get("Content-Length"))
224-
try:
225-
existing_size = os.path.getsize(result_path)
226-
except OSError:
227-
existing_size = 0
228-
if linked_etag and total_size == existing_size:
229-
with builtins.open(result_path, "rb") as file:
230-
sha256_hash = hashlib.sha256()
231-
for byte_block in iter(lambda: file.read(4096), b""):
232-
sha256_hash.update(byte_block)
233-
if f'"{sha256_hash.hexdigest()}"' == linked_etag:
234-
nc.set_init_status(min(current_progress + progress_for_task, 99))
235-
return result_path
236-
237-
try:
238-
with builtins.open(tmp_path, "wb") as file:
239-
last_progress = current_progress
240-
for chunk in response.iter_raw(-1):
241-
downloaded_size += file.write(chunk)
242-
if total_size:
243-
new_progress = min(current_progress + int(progress_for_task * downloaded_size / total_size), 99)
244-
if new_progress != last_progress:
245-
nc.set_init_status(new_progress)
246-
last_progress = new_progress
247-
os.replace(tmp_path, result_path)
248-
except BaseException:
249-
if os.path.exists(tmp_path):
250-
os.remove(tmp_path)
251-
raise
211+
try:
212+
with FileLock(result_path + ".lock", timeout=7200), niquests.get(model_path, stream=True) as response:
213+
if not response.ok:
214+
raise ModelFetchError(
215+
f"Downloading of '{model_path}' failed, returned ({response.status_code}) {response.text}"
216+
)
217+
downloaded_size = 0
218+
linked_etag = ""
219+
for redirect_resp in response.history:
220+
linked_etag = redirect_resp.headers.get("X-Linked-ETag", "")
221+
if linked_etag:
222+
break
223+
if not linked_etag:
224+
linked_etag = response.headers.get("X-Linked-ETag", response.headers.get("ETag", ""))
225+
total_size = int(response.headers.get("Content-Length"))
226+
try:
227+
existing_size = os.path.getsize(result_path)
228+
except OSError:
229+
existing_size = 0
230+
if linked_etag and total_size == existing_size:
231+
with builtins.open(result_path, "rb") as file:
232+
sha256_hash = hashlib.sha256()
233+
for byte_block in iter(lambda: file.read(4096), b""):
234+
sha256_hash.update(byte_block)
235+
if f'"{sha256_hash.hexdigest()}"' == linked_etag:
236+
nc.set_init_status(min(current_progress + progress_for_task, 99))
237+
return result_path
238+
239+
try:
240+
with builtins.open(tmp_path, "wb") as file:
241+
last_progress = current_progress
242+
for chunk in response.iter_raw(-1):
243+
downloaded_size += file.write(chunk)
244+
if total_size:
245+
new_progress = min(
246+
current_progress + int(progress_for_task * downloaded_size / total_size), 99
247+
)
248+
if new_progress != last_progress:
249+
nc.set_init_status(new_progress)
250+
last_progress = new_progress
251+
os.replace(tmp_path, result_path)
252+
except BaseException:
253+
if os.path.exists(tmp_path):
254+
os.remove(tmp_path)
255+
raise
256+
except FileLockTimeout as exc:
257+
raise ModelFetchError(
258+
f"Timed out waiting for lock on '{result_path}' after 7200s — another process may be stuck downloading"
259+
) from exc
252260

253261
return result_path
254262

pyproject.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,9 @@ lint.extend-per-file-ignores."tests/**/*.py" = [
151151
]
152152
lint.extend-per-file-ignores."tests_unit/**/*.py" = [
153153
"D",
154+
"E402",
154155
"S",
156+
"UP",
155157
]
156158
lint.mccabe.max-complexity = 16
157159

tests_unit/test_fetch_model_file.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
import pytest
99
from filelock import FileLock
10+
from filelock import Timeout as FileLockTimeout
1011

1112
from nc_py_api._exceptions import ModelFetchError
1213
from nc_py_api.ex_app.integration_fastapi import fetch_models_task
@@ -186,6 +187,20 @@ def mock_get(_url, **_kwargs):
186187
# File must be entirely one content or the other — never mixed
187188
assert data in (b"A" * 10000, b"B" * 10000)
188189

190+
def test_filelock_timeout_raises_model_fetch_error(self, tmp_path):
191+
save_path = str(tmp_path / "model.bin")
192+
lock = FileLock(save_path + ".lock")
193+
nc = _mock_nc()
194+
195+
with (
196+
mock.patch("nc_py_api.ex_app.integration_fastapi.FileLock", side_effect=FileLockTimeout(lock)),
197+
pytest.raises(ModelFetchError),
198+
):
199+
fetch_models_task(nc, {"https://example.com/m.bin": {"save_path": save_path}}, 0)
200+
201+
status_msg = nc.set_init_status.call_args_list[-1][0][1]
202+
assert "Timed out waiting for lock" in status_msg
203+
189204
def test_progress_updates_sent(self, tmp_path):
190205
save_path = str(tmp_path / "model.bin")
191206
nc = _mock_nc()

0 commit comments

Comments
 (0)