From de723fd0b42bc73baff7fa80848e4495d74886c5 Mon Sep 17 00:00:00 2001 From: tkrmagid Date: Sat, 16 May 2026 22:48:16 +0900 Subject: [PATCH] v0.4.18: fix /videoCache clear vs in-flight download race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit clearAll() previously only wiped finalized files + the IN_FLIGHT set; any download that was already running on the single-thread executor kept writing past the clear, then atomically moved its .part to final and re-published into READY — resurrecting one cache entry post-clear. Fix: add an AtomicLong CACHE_EPOCH. preload() captures the epoch at submit time; clearAll() bumps the epoch BEFORE wiping disk; download(url, epoch) checks at three points (pre-start, in read loop, pre-publish) and bails if the epoch moved, deleting its own .part only AFTER closing the output stream (Windows can't delete an open file). Phase ordering in clearAll() also matters: bump epoch first → drop indexes → delete files. That way the in-flight download sees the cancellation flag before the index wipe / file delete races can interact with it. --- gradle.properties | 2 +- .../client/playback/VideoCache.java | 97 ++++++++++++++++--- 2 files changed, 86 insertions(+), 13 deletions(-) diff --git a/gradle.properties b/gradle.properties index 2f7957e..cf86871 100644 --- a/gradle.properties +++ b/gradle.properties @@ -5,7 +5,7 @@ org.gradle.configuration-cache=false # Mod mod_id=video_player -mod_version=0.4.17 +mod_version=0.4.18 maven_group=com.ejclaw.videoplayer archives_base_name=video_player diff --git a/src/main/java/com/ejclaw/videoplayer/client/playback/VideoCache.java b/src/main/java/com/ejclaw/videoplayer/client/playback/VideoCache.java index 355f9d3..8df22b0 100644 --- a/src/main/java/com/ejclaw/videoplayer/client/playback/VideoCache.java +++ b/src/main/java/com/ejclaw/videoplayer/client/playback/VideoCache.java @@ -22,6 +22,7 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicLong; /** * Client-side disk cache for preloaded HTTP video URLs. Driven by the {@code /videopreload} @@ -42,6 +43,14 @@ public final class VideoCache { private static final Map READY = new ConcurrentHashMap<>(); /** urls whose download is currently queued or in flight. */ private static final Set IN_FLIGHT = ConcurrentHashMap.newKeySet(); + /** + * Monotonic cache-generation counter. {@link #clearAll()} bumps this; every download + * captures the value at submit time and bails (deletes its {@code .part} without + * publishing) if the epoch has moved since. This is the cancellation channel for the + * in-flight download — without it, {@code clearAll} only wipes already-finalized files + * while the running download keeps writing and re-publishes one entry post-clear. + */ + private static final AtomicLong CACHE_EPOCH = new AtomicLong(0); /** * Single-thread executor that serializes all downloads. Cap enforcement (the total @@ -121,11 +130,36 @@ public final class VideoCache { /** * Wipe the entire cache directory and drop both indexes. Sent in response to - * {@code /videoCache clear}. Best-effort per-file deletion; logs failures but doesn't - * abort on the first one. In-flight downloads aren't cancelled — they'll fail at the - * atomic-move step (target dir gone) and log a warning, which is fine. + * {@code /videoCache clear}. + * + *

Three phases that need to stay in this order: + *

    + *
  1. Bump the epoch first — that flips the cancellation flag for any in-flight + * download. The download thread checks the epoch in the read loop and at the + * move step, so after this point it will refuse to publish a new entry and + * will delete its own {@code .part}.
    2. + *
  2. Clear the in-memory indexes. (READY clears whatever was already finalized + * pre-clear; IN_FLIGHT is wiped so a subsequent same-URL preload re-enters + * the queue instead of getting deduped by stale state.)
    3. + *
  3. Best-effort delete every regular file in the cache directory. We do this + * last so that if the in-flight download was momentarily holding a {@code .part} + * open, the listing here also catches its post-close residue (though normally + * phase 1 → the download deletes its own part).
    4. + *
+ * + *

Best-effort per-file deletion; logs failures but doesn't abort on the first one. + * On Windows the open {@code .part} may not be deletable here — phase 1's cancellation + * + the download's own cleanup-on-bail handles that case. */ public static void clearAll() { + // Phase 1: signal cancellation to any in-flight download BEFORE wiping disk. + CACHE_EPOCH.incrementAndGet(); + + // Phase 2: drop indexes. + READY.clear(); + IN_FLIGHT.clear(); + + // Phase 3: delete files on disk. int deleted = 0; int failed = 0; try { @@ -147,8 +181,6 @@ public final class VideoCache { VideoPlayerMod.LOG.warn("[{}] clearAll failed: {}", VideoPlayerMod.MOD_ID, t.toString()); } - READY.clear(); - IN_FLIGHT.clear(); VideoPlayerMod.LOG.info("[{}] clearAll: deleted={} failed={}", VideoPlayerMod.MOD_ID, deleted, failed); notifyChat("[videocache] 전체 캐시 삭제: " + deleted + "개 파일", ChatFormatting.YELLOW); @@ -169,7 +201,11 @@ public final class VideoCache { return; } notifyChat("[videopreload] 다운로드 대기열 추가: " + url, ChatFormatting.YELLOW); - DOWNLOAD_POOL.submit(() -> download(url)); + // Capture the current epoch at submit time. The download thread checks against + // CACHE_EPOCH later — any mismatch means clearAll() ran in between and this + // download must abort without publishing. + long epoch = CACHE_EPOCH.get(); + DOWNLOAD_POOL.submit(() -> download(url, epoch)); } /** Return the local cached file path for this URL, or {@code null} if not yet ready. */ @@ -193,24 +229,33 @@ public final class VideoCache { // -- internals ----------------------------------------------------------------------- - private static void download(String url) { + private static void download(String url, long startEpoch) { Path cacheDir = cacheDir(); + Path partPath = null; try { if (cacheDir == null) { VideoPlayerMod.LOG.warn("[{}] preload: no game dir, skipping {}", VideoPlayerMod.MOD_ID, url); return; } + // Pre-flight cancellation check — if clearAll already ran between submit and now, + // skip the whole thing. Avoids creating directories / .part files post-clear. + if (CACHE_EPOCH.get() != startEpoch) { + VideoPlayerMod.LOG.info("[{}] preload: cancelled before start (clearAll ran) — {}", + VideoPlayerMod.MOD_ID, url); + return; + } Files.createDirectories(cacheDir); String hash = sha256(url); String ext = extensionFromUrl(url); Path finalPath = cacheDir.resolve(hash + ext); - Path partPath = cacheDir.resolve(hash + ext + ".part"); + partPath = cacheDir.resolve(hash + ext + ".part"); // Resume-friendly: if the file's already on disk from an earlier session, just // index it without re-downloading. if (Files.exists(finalPath) && Files.size(finalPath) > 0) { + if (CACHE_EPOCH.get() != startEpoch) return; READY.put(url, finalPath); VideoPlayerMod.LOG.info("[{}] preload: indexed existing cache {} -> {}", VideoPlayerMod.MOD_ID, url, finalPath.getFileName()); @@ -250,21 +295,30 @@ public final class VideoCache { } long total = 0; + boolean cancelled = false; try (InputStream in = raw.getInputStream(); OutputStream out = Files.newOutputStream(partPath)) { byte[] buf = new byte[64 * 1024]; int n; while ((n = in.read(buf)) >= 0) { + // Cancellation check inside the loop. Break (not return) so the try-with + // closes the output stream first — on Windows, deleting an open .part can + // fail with AccessDeniedException, so we always close before deleting. + if (CACHE_EPOCH.get() != startEpoch) { + cancelled = true; + break; + } total += n; if (total > MAX_BYTES) { long capMb = MAX_BYTES / (1024 * 1024); VideoPlayerMod.LOG.warn( "[{}] preload: {} exceeded per-video {} MB cap; aborting", VideoPlayerMod.MOD_ID, url, capMb); - try { Files.deleteIfExists(partPath); } catch (Throwable ignored) {} + // Same close-before-delete dance for Windows. + cancelled = true; notifyChat("[videopreload] 실패 (단일 영상 " + capMb + "MB 초과): " + url, ChatFormatting.RED); - return; + break; } if (existingCacheBytes + total > MAX_CACHE_BYTES) { long capMb = MAX_CACHE_BYTES / (1024 * 1024); @@ -272,14 +326,29 @@ public final class VideoCache { VideoPlayerMod.LOG.warn( "[{}] preload: total-cache cap exceeded ({}>{} MB); aborting {}", VideoPlayerMod.MOD_ID, usedMb, capMb, url); - try { Files.deleteIfExists(partPath); } catch (Throwable ignored) {} + cancelled = true; notifyChat("[videopreload] 실패 (전체 캐시 " + capMb + "MB 초과): " + url, ChatFormatting.RED); - return; + break; } out.write(buf, 0, n); } } + // Now the .part file is closed — safe to delete on Windows. + if (cancelled) { + try { Files.deleteIfExists(partPath); } catch (Throwable ignored) {} + return; + } + + // Final cancellation check before publishing. If clearAll ran during the read + // loop's last iteration (after the loop check, before this point), we still + // refuse to publish — otherwise we'd resurrect a cache entry post-clear. + if (CACHE_EPOCH.get() != startEpoch) { + VideoPlayerMod.LOG.info("[{}] preload: cancelled at publish (clearAll ran) — {}", + VideoPlayerMod.MOD_ID, url); + try { Files.deleteIfExists(partPath); } catch (Throwable ignored) {} + return; + } Files.move(partPath, finalPath, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); @@ -293,6 +362,10 @@ public final class VideoCache { VideoPlayerMod.MOD_ID, url, t.toString()); notifyChat("[videopreload] 실패 (" + t.getClass().getSimpleName() + "): " + url, ChatFormatting.RED); + // Best-effort cleanup of any leftover .part on the error path. + if (partPath != null) { + try { Files.deleteIfExists(partPath); } catch (Throwable ignored) {} + } } finally { IN_FLIGHT.remove(url); }