refactor(thumbnails): keep the after-scan queue's store rule and pin the write lock (#320)
A planned target keeps the plan's own containment rule (inside the store, as the thumbnail job writes it); captures keep the serve rule. The structural test also covers PREVIEW_FOLDER joins and the repo-root modules; the write-lock test asserts the lock instead of waiting on a timer; new tests cover nested roots in the after-scan queue and a legacy upper-case preview served through the resolver.
This commit is contained in:
Vito Sansevero
@@ -16,7 +16,9 @@ under ``write_lock`` and only when the generation is unchanged and no hold
|
||||
applies (``thumbnail_encode.replace_unless_held``). Changing the location
|
||||
bumps the generation under the same lock, so no write that began before can
|
||||
land in the old store afterwards. Nothing reads or commits config while
|
||||
``write_lock`` is held.
|
||||
``write_lock`` is held. The hold check runs under it too, so hold checks take
|
||||
no lock (the job registry's ``running()`` is a lock-free snapshot) and
|
||||
``bump_generation`` is never called with a registry or config lock held.
|
||||
|
||||
All functions are blocking: call them off the event loop.
|
||||
"""
|
||||
|
||||
@@ -163,18 +163,25 @@ class DeferredPreview:
|
||||
(#320): a queued capture follows the thumbnail location at write time,
|
||||
not the one when it was queued."""
|
||||
|
||||
__slots__ = ("root", "rel")
|
||||
__slots__ = ("root", "rel", "strict")
|
||||
|
||||
def __init__(self, root, rel):
|
||||
def __init__(self, root, rel, *, strict=True):
|
||||
self.root = Path(root)
|
||||
self.rel = Path(rel)
|
||||
# A capture refuses a store linked out of its container (the serve
|
||||
# route's rule); a planned target keeps the plan's rule: inside the
|
||||
# store, as the thumbnail job writes it.
|
||||
self.strict = strict
|
||||
|
||||
def resolve(self):
|
||||
"""The preview's path now, or None when nothing may be written."""
|
||||
dst = media_preview_path(self.root, self.rel)
|
||||
if dst is None or not preview_store.owns(self.root, dst):
|
||||
if dst is None:
|
||||
return None
|
||||
return dst
|
||||
if self.strict:
|
||||
return dst if preview_store.owns(self.root, dst) else None
|
||||
store = preview_store.store_dir(self.root)
|
||||
return dst if _is_within(dst, store) else None
|
||||
|
||||
|
||||
def _resolved(dst):
|
||||
@@ -325,7 +332,7 @@ def _deferred(roots, src, dst):
|
||||
continue
|
||||
rel = src.relative_to(root)
|
||||
if media_preview_path(root, rel) == Path(dst):
|
||||
return DeferredPreview(root, rel)
|
||||
return DeferredPreview(root, rel, strict=False)
|
||||
return dst
|
||||
|
||||
|
||||
|
||||
+58
-24
@@ -191,14 +191,16 @@ class TestWritesAndTheSwitch(SourceModeCase):
|
||||
replace_unless_held(tmp, dst, preview_store.write_guard(lambda: False))
|
||||
self.assertFalse(dst.exists())
|
||||
|
||||
def test_a_switch_waits_for_a_rename_in_flight(self):
|
||||
"""The rename and the generation bump exclude each other: a bump that
|
||||
starts during a rename completes after it (event-driven)."""
|
||||
def test_the_check_and_the_rename_run_under_the_write_lock(self):
|
||||
"""The hold check and the rename hold ``write_lock``, the lock the
|
||||
generation bump takes: a switch cannot fall between them."""
|
||||
tmp, dst = self._temp_and_dst()
|
||||
in_check = threading.Event()
|
||||
release = threading.Event()
|
||||
seen = {}
|
||||
|
||||
def slow_hold():
|
||||
seen["locked"] = preview_store.write_lock.locked()
|
||||
in_check.set()
|
||||
release.wait(WAIT)
|
||||
return True
|
||||
@@ -206,18 +208,17 @@ class TestWritesAndTheSwitch(SourceModeCase):
|
||||
guard = preview_store.write_guard(slow_hold)
|
||||
writer = threading.Thread(target=replace_unless_held, args=(tmp, dst, guard))
|
||||
writer.start()
|
||||
self.assertTrue(in_check.wait(WAIT))
|
||||
bumped = threading.Event()
|
||||
bumper = threading.Thread(
|
||||
target=lambda: (preview_store.bump_generation(), bumped.set())
|
||||
)
|
||||
bumper.start()
|
||||
self.assertFalse(bumped.wait(0.05)) # blocked behind the rename
|
||||
release.set()
|
||||
writer.join(WAIT)
|
||||
bumper.join(WAIT)
|
||||
self.assertTrue(bumped.is_set())
|
||||
try:
|
||||
self.assertTrue(in_check.wait(WAIT))
|
||||
self.assertTrue(seen["locked"])
|
||||
self.assertTrue(preview_store.write_lock.locked())
|
||||
finally:
|
||||
release.set()
|
||||
writer.join(WAIT)
|
||||
self.assertFalse(preview_store.write_lock.locked())
|
||||
self.assertEqual(dst.read_bytes(), b"new")
|
||||
bumped = preview_store.bump_generation()
|
||||
self.assertEqual(preview_store.generation(), bumped)
|
||||
|
||||
|
||||
class TestOnlyTheResolverBuildsStorePaths(unittest.TestCase):
|
||||
@@ -234,21 +235,25 @@ class TestOnlyTheResolverBuildsStorePaths(unittest.TestCase):
|
||||
r"""|joinpath\([^)]*["']thumbnails["']"""
|
||||
r"""|os\.path\.join\([^)]*["']thumbnails["']"""
|
||||
r"""|["']thumbnails/"""
|
||||
r"""|/\s*PREVIEW_FOLDER|joinpath\([^)]*PREVIEW_FOLDER"""
|
||||
r"""|Path\([^)]*,\s*["']thumbnails["']"""
|
||||
)
|
||||
|
||||
def test_no_thumbnails_join_outside_the_resolver(self):
|
||||
offenders = []
|
||||
files = sorted(REPO.glob("*.py"))
|
||||
for folder in ("py", "utils", "database"):
|
||||
for path in sorted((REPO / folder).rglob("*.py")):
|
||||
rel = path.relative_to(REPO)
|
||||
if rel in self.ALLOWED:
|
||||
continue
|
||||
for number, line in enumerate(
|
||||
path.read_text(encoding="utf-8").splitlines(), 1
|
||||
):
|
||||
code = line.split("#", 1)[0]
|
||||
if self.JOIN.search(code):
|
||||
offenders.append(f"{rel}:{number}: {line.strip()}")
|
||||
files += sorted((REPO / folder).rglob("*.py"))
|
||||
for path in files:
|
||||
rel = path.relative_to(REPO)
|
||||
if rel in self.ALLOWED:
|
||||
continue
|
||||
for number, line in enumerate(
|
||||
path.read_text(encoding="utf-8").splitlines(), 1
|
||||
):
|
||||
code = line.split("#", 1)[0]
|
||||
if self.JOIN.search(code):
|
||||
offenders.append(f"{rel}:{number}: {line.strip()}")
|
||||
self.assertEqual(offenders, [])
|
||||
|
||||
|
||||
@@ -317,6 +322,16 @@ class TestEveryCallerFollowsTheStore(RedirectedStoreCase):
|
||||
self.assertEqual(resp.status, 200)
|
||||
self.assertEqual(await resp.read(), preview.read_bytes())
|
||||
|
||||
async def test_a_legacy_upper_case_preview_is_served_from_the_store(self):
|
||||
make_png(self.output_dir / "k.JPG", size=(64, 64))
|
||||
legacy = self.store / "k_thumb.JPG"
|
||||
legacy.write_bytes(b"legacy preview")
|
||||
resp = await self.client.get(
|
||||
"/prompt_manager/images/serve/thumbnails/k_thumb.JPG?root=0"
|
||||
)
|
||||
self.assertEqual(resp.status, 200)
|
||||
self.assertEqual(await resp.read(), b"legacy preview")
|
||||
|
||||
async def test_a_missing_preview_is_made_in_the_store_on_first_view(self):
|
||||
make_png(self.output_dir / "sub" / "b.png", size=(640, 480))
|
||||
url = _entry(self.api, self.output_dir, "sub/b.png")["thumbnail_url"]
|
||||
@@ -409,6 +424,25 @@ class TestCaptureResolvesAtWriteTime(RedirectedStoreCase):
|
||||
thumbnailer._write_queued(thumbnailer._queue.popleft())
|
||||
self.assertEqual(written, [moved / "i_thumb.webp"])
|
||||
|
||||
def test_the_after_scan_queue_maps_each_target_to_its_own_root(self):
|
||||
from py.api import thumbnail_auto as auto
|
||||
|
||||
nested = self.output_dir / "inner"
|
||||
nested.mkdir()
|
||||
src = nested / "j.png"
|
||||
outer = auto._deferred(
|
||||
[self.output_dir, nested], src, self._preview("inner/j.png")
|
||||
)
|
||||
inner = auto._deferred(
|
||||
[self.output_dir, nested], src, nested / "thumbnails" / "j_thumb.webp"
|
||||
)
|
||||
self.assertEqual(
|
||||
(outer.root, outer.rel), (self.output_dir, Path("inner/j.png"))
|
||||
)
|
||||
self.assertEqual((inner.root, inner.rel), (nested, Path("j.png")))
|
||||
stray = self.comfy_dir / "x_thumb.webp"
|
||||
self.assertEqual(auto._deferred([self.output_dir], src, stray), stray)
|
||||
|
||||
def test_serve_and_history_share_one_flight_key(self):
|
||||
from py.api import thumbnail_auto as auto
|
||||
|
||||
|
||||
Reference in New Issue
Block a user