Skip to content

Commit fefb292

Browse files
fralapoclaude
andcommitted
fix(review): apply remaining MEDIUM/LOW backlog from multi-agent review
Round-3: clears the deferred backlog after the round-2 CRITICAL/HIGH pass. 7 parallel fixers over disjoint file sets; each finding verified against source. Concurrency / correctness: - smartcut._clip_lock: replace size-cap eviction with a reference-counted context manager — an entry now lives exactly as long as some caller holds or waits on it, closing the residual TOCTOU where two callers could be handed different locks for one path (concurrent smart_cut clobbering one output) - app.py /api/reframe: a metadata-save failure after a successful reframe now raises HTTP 500 instead of returning 200 with on-disk/metadata divergence - app.py /api/batch: clean up the failing item's output dir + jobs entry on the build_main_cmd ValueError and QueueFull paths (was leaking orphan dirs) Resilience / hang protection: - diarization.extract_audio_to_wav: add 1800s ffmpeg timeout (matches the ASR sibling) + broaden except to TimeoutExpired - social_publisher: 5s bound on the SSRF-guard getaddrinfo (was unbounded, could pin a thread-pool slot) - download.py: re-resolve the URL host at download time and refuse if it now points only at internal/loopback ranges (DNS-rebinding past the API SSRF check) Validation: - schemas: ZernioConfigRequest gains api_key/timezone max_length + an accounts validator (≤16 entries, known platforms, str ≤256); PublishRequest.scheduled_for gains max_length=64 Observability (surface silent failures, keep fallbacks): - main.transcribe_video: WARNING when Deepgram fails before the Whisper fallback - transcribe_cache.load_cached_transcript: distinguish corrupt entry (warn + delete so next run rewrites) from a genuine cache miss - job_results._build_clips + job_artifacts.relocate_root_job_artifacts: log the swallowed exception instead of silent pass/return-False - social_publisher: scheduling-without-collision-data is now explicit in logs (Zernio fetch failure no longer reads like an empty calendar) Performance / cleanup: - reframe.py: lazy-init MediaPipe FaceDetection/FaceMesh (was ~300ms TFLite load at import on every pipeline + every --reframe-only subprocess); all .process() call sites migrated to the getters. Removed dead SpeakerTracker stabilization_frames param/attribute (never read; real suppression is cooldown) - compose._apply_smartcut: drop the dead legacy-sidecar shortcut (the bare *_smartcut.mp4 name is never written under the current Subtitles→Smart Cut order; smart_cut's own plan-hash cache + legacy fallback already cover it). Supersedes the round-1 mtime guard on that now-removed branch. Frontend (live redesign/): - useModalA11y: callback-ref for onClose so the focus-trap effect stops re-running (and stealing focus mid-type) on every parent re-render - publish.jsx: mountedRef guards the 500ms post-publish setTimeout - RedesignApp: track + clear toast auto-dismiss timers on unmount - create.jsx: aria-pressed on the preset cards Tests: test_concurrency_fixes.py rewritten for the context-manager _clip_lock (registry identity + refcount + held-entry-not-evicted). Host 418 passed/3 skipped; Docker integration 23 passed; eslint 0; vite build OK. Deliberately NOT changed (contract/behavior risk outweighs benefit; documented): response_model on every endpoint (would risk dropping fields the UI reads), process_endpoint multipart/json double-parse refactor (no repro, risky), window.prompt/confirm replacement (cosmetic), global-smooth odd-frame trajectory (opt-in quality knob — eye-decides per CLAUDE.md). esbuild≤0.24 dev-CORS is dev-server-only and already mitigated by the backend origin checks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDkEfcWP5suCdutKJKnvCM
1 parent f939db6 commit fefb292

17 files changed

Lines changed: 253 additions & 101 deletions

File tree

dashboard/src/redesign/RedesignApp.jsx

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// Visual layer is the Claude Design handoff; data layer reuses the same
33
// useJobSubmission / useJobPolling / useHistory / useClipStates the production
44
// app uses, so the pipeline behaves identically.
5-
import { useState, useEffect, useMemo, useCallback } from 'react';
5+
import { useState, useEffect, useMemo, useCallback, useRef } from 'react';
66
import './tokens.css';
77
import './app.css';
88
import { Icon, Btn } from './primitives';
@@ -91,6 +91,10 @@ export default function RedesignApp() {
9191
const [preselections, setPreselectionsRaw] = useState(null);
9292
const [confetti, setConfetti] = useState(false);
9393
const [toasts, setToasts] = useState([]);
94+
// Track all auto-dismiss timer ids so they can be cleared if the component
95+
// unmounts before any timer fires (prevents setState on an unmounted tree).
96+
const toastTimerIds = useRef([]);
97+
useEffect(() => () => { toastTimerIds.current.forEach(clearTimeout); }, []);
9498
const [publishClips, setPublishClips] = useState(null);
9599
const [editClip, setEditClip] = useState(null);
96100
const [viewingHistory, setViewingHistory] = useState(false);
@@ -115,7 +119,8 @@ export default function RedesignApp() {
115119
const pushToast = useCallback((type, msg) => {
116120
const id = Date.now() + Math.random();
117121
setToasts((t) => [...t, { id, type, msg }]);
118-
setTimeout(() => setToasts((t) => t.filter((x) => x.id !== id)), 3600);
122+
const tid = setTimeout(() => setToasts((t) => t.filter((x) => x.id !== id)), 3600);
123+
toastTimerIds.current.push(tid);
119124
}, []);
120125

121126
// Background clip reprocess: the Edit modal stages the changes and hands them

dashboard/src/redesign/create.jsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ function PresetCards({ presets, active, defaultId, onPick, onSetDefault, onDelet
1414
// role=button (not a real <button>) so the star/trash actions inside
1515
// can be real, keyboard-operable <button>s — interactive-in-interactive
1616
// is invalid HTML.
17-
<div key={p.id} role="button" tabIndex={0} className={'preset' + (active === p.id ? ' on' : '')}
17+
<div key={p.id} role="button" tabIndex={0} aria-pressed={active === p.id} className={'preset' + (active === p.id ? ' on' : '')}
1818
onClick={() => onPick(p)}
1919
onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); onPick(p); } }}>
2020
<span className="pcheck"><Icon n="check" /></span>

dashboard/src/redesign/publish.jsx

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// Every selected clip is published in parallel (Promise.allSettled) — the fix
33
// for the old sequential stall — each row showing live queued→uploading→
44
// live/error status. Per-clip compose_first honours the clip's toggles.
5-
import { useState, useEffect } from 'react';
5+
import { useState, useEffect, useRef } from 'react';
66
import { Icon, Social, Btn, Switch, PlatPill, PLATFORMS } from './primitives';
77
import { clipVideoSrc } from './realApi';
88
import { publishClip, getZernio } from './realApi';
@@ -76,6 +76,11 @@ export function PublishModal({ clips, jobId, clipStates = {}, preselections, onC
7676
// Accessibility: focus trap + Escape-to-close + focus restore.
7777
const panelRef = useModalA11y(onClose);
7878

79+
// Guard the post-publish setTimeout so it never calls setState after the
80+
// modal has been unmounted (e.g. parent closes it while the delay is in flight).
81+
const mountedRef = useRef(true);
82+
useEffect(() => () => { mountedRef.current = false; }, []);
83+
7984
const accounts = zernio?.accounts || {};
8085
const toggle = (k) => setPlats((p) => ({ ...p, [k]: !p[k] }));
8186
const platTargets = () => Object.keys(plats)
@@ -133,6 +138,7 @@ export function PublishModal({ clips, jobId, clipStates = {}, preselections, onC
133138
const ok = results.filter((r) => r.status === 'fulfilled' && r.value).length;
134139
const fail = clips.length - ok;
135140
setTimeout(() => {
141+
if (!mountedRef.current) return;
136142
setStage('done');
137143
pushToast?.(fail === 0 ? 'success' : 'warn', `Published ${ok}/${clips.length}${fail ? `, ${fail} failed` : ''}`);
138144
}, 500);

dashboard/src/redesign/useModalA11y.js

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,15 @@ import { useEffect, useRef } from 'react';
77
export function useModalA11y(onClose) {
88
const ref = useRef(null);
99

10+
// Keep a ref to the latest onClose so the keydown handler always calls the
11+
// current callback without being listed as an effect dependency. Without this
12+
// pattern, callers that pass a fresh inline lambda on every render (e.g.
13+
// `() => setEditClip(null)`) would cause the effect to re-run — and reset
14+
// focus to the first focusable element — on every parent re-render while the
15+
// modal is open (e.g. every toast that appears).
16+
const onCloseRef = useRef(onClose);
17+
useEffect(() => { onCloseRef.current = onClose; }, [onClose]);
18+
1019
useEffect(() => {
1120
const panel = ref.current;
1221
const prevActive = document.activeElement;
@@ -26,7 +35,7 @@ export function useModalA11y(onClose) {
2635
else if (panel) { panel.setAttribute('tabindex', '-1'); panel.focus(); }
2736

2837
const onKey = (e) => {
29-
if (e.key === 'Escape') { onClose?.(); return; }
38+
if (e.key === 'Escape') { onCloseRef.current?.(); return; }
3039
if (e.key !== 'Tab') return;
3140
const items = focusables();
3241
if (items.length === 0) { e.preventDefault(); return; }
@@ -45,7 +54,8 @@ export function useModalA11y(onClose) {
4554
// Restore focus to where it was before the modal opened.
4655
if (prevActive && typeof prevActive.focus === 'function') prevActive.focus();
4756
};
48-
}, [onClose]);
57+
// eslint-disable-next-line react-hooks/exhaustive-deps
58+
}, []); // run once on mount — onClose changes are handled via onCloseRef above
4959

5060
return ref;
5161
}

src/clippyme/api/app.py

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -519,6 +519,9 @@ async def batch_process(req: BatchRequest, request: Request):
519519
model=getattr(req, "model", None),
520520
)
521521
except ValueError as exc:
522+
# This item's output dir was already created above but it never
523+
# made it into `jobs` — clean it up so a bad URL can't orphan a dir.
524+
await asyncio.to_thread(shutil.rmtree, job_output_dir, True)
522525
raise HTTPException(status_code=400, detail=str(exc))
523526

524527
env = os.environ.copy()
@@ -536,8 +539,10 @@ async def batch_process(req: BatchRequest, request: Request):
536539
job_queue.put_nowait(job_id)
537540
batch_jobs.append({"url": url, "job_id": job_id})
538541
except asyncio.QueueFull:
539-
del jobs[job_id]
540-
shutil.rmtree(job_output_dir, ignore_errors=True)
542+
# Only the item that failed to enqueue is cleaned up — already
543+
# enqueued jobs stay running. Mirrors the single /api/process path.
544+
jobs.pop(job_id, None)
545+
await asyncio.to_thread(shutil.rmtree, job_output_dir, True)
541546
# Stop adding more — queue is full
542547
break
543548

@@ -1080,14 +1085,22 @@ async def reframe_clip(job_id: str, clip_index: int, req: ReframeRequest, reques
10801085
clean_video_url = f"/videos/{job_id}/{original_clip_filename}"
10811086
new_video_url = f"{clean_video_url}?v={cache_bust}"
10821087

1083-
# Update metadata.json and in-memory job state with the CLEAN url
1088+
# Update in-memory metadata structures with the CLEAN url, then persist.
1089+
clips[clip_index]["video_url"] = clean_video_url
1090+
clips[clip_index]["reframe_mode"] = mode
1091+
data["shorts"] = clips
1092+
1093+
# A persistence failure must NOT silently succeed: the clip on disk has
1094+
# already been re-rendered with the new mode, so if metadata.json still
1095+
# carries the OLD reframe_mode/video_url the divergence is invisible until
1096+
# a restart reloads stale state. Update in-memory job state regardless
1097+
# (so the live session is correct), but surface the save failure as a 500.
1098+
save_failed = None
10841099
try:
1085-
clips[clip_index]["video_url"] = clean_video_url
1086-
clips[clip_index]["reframe_mode"] = mode
1087-
data["shorts"] = clips
10881100
save_job_metadata(metadata_path, data)
10891101
except Exception as e:
1090-
logger.warning("Failed to update metadata.json after reframe: %s", e)
1102+
logger.error("Failed to persist metadata.json after reframe: %s", e)
1103+
save_failed = e
10911104

10921105
if (
10931106
job_id in jobs
@@ -1100,6 +1113,12 @@ async def reframe_clip(job_id: str, clip_index: int, req: ReframeRequest, reques
11001113
jobs[job_id]["result"]["clips"][clip_index]["video_url"] = clean_video_url
11011114
jobs[job_id]["result"]["clips"][clip_index]["reframe_mode"] = mode
11021115

1116+
if save_failed is not None:
1117+
raise HTTPException(
1118+
status_code=500,
1119+
detail="Reframe succeeded but metadata persistence failed; reload may show stale state",
1120+
)
1121+
11031122
return {
11041123
"success": True,
11051124
"new_video_url": new_video_url,

src/clippyme/api/schemas.py

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ class PublishRequest(BaseModel):
224224
caption: str = Field("", max_length=2200)
225225
platforms: List[dict] = Field(..., min_length=1, max_length=14)
226226
schedule_mode: str = Field("now", pattern=r"^(now|auto|manual)$")
227-
scheduled_for: Optional[str] = None
227+
scheduled_for: Optional[str] = Field(None, max_length=64)
228228
# Optional YYYY-MM-DD that defines the day the SmartScheduler should
229229
# start picking slots from when schedule_mode="auto". Ignored by
230230
# "now" / "manual". Defaults to today if omitted.
@@ -312,6 +312,21 @@ def _bound_tiktok_settings(cls, v):
312312

313313
class ZernioConfigRequest(BaseModel):
314314
"""Persisted Zernio settings (saved to data/config.json)."""
315-
api_key: Optional[str] = None
315+
api_key: Optional[str] = Field(None, max_length=512)
316316
accounts: Optional[dict] = None # {"tiktok": "...", "instagram": "...", "youtube": "..."}
317-
timezone: Optional[str] = None
317+
timezone: Optional[str] = Field(None, max_length=64)
318+
319+
@field_validator("accounts")
320+
@classmethod
321+
def _validate_accounts(cls, v):
322+
if v is None:
323+
return v
324+
if len(v) > 16:
325+
raise ValueError("too many account entries")
326+
allowed = {"tiktok", "instagram", "youtube"}
327+
for k, val in v.items():
328+
if k not in allowed:
329+
raise ValueError(f"unknown account platform: {k!r}")
330+
if val is not None and (not isinstance(val, str) or len(val) > 256):
331+
raise ValueError(f"account id for {k!r} must be a string <= 256 chars")
332+
return v

src/clippyme/domain/compose.py

Lines changed: 8 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -63,29 +63,14 @@ async def _apply_smartcut(
6363
intermediate_files: list,
6464
drop_ranges=None,
6565
) -> str:
66-
# Cache key must follow the ACTUAL input. The previous implementation
67-
# keyed off `base_clip` unconditionally, which meant that if we'd
68-
# smart-cut the raw clip before, we'd return the stale raw smart-cut
69-
# even when the current call is operating on a subtitled/hooked
70-
# variant. Use current_input instead so each input gets its own
71-
# sidecar _smartcut.mp4.
72-
# Legacy fixed-name sidecar shortcut — only safe for the plain auto cut.
73-
# A manual trim (drop_ranges) must NOT reuse it: the same input with
74-
# different hand-picked drops would otherwise serve a stale cut. smart_cut
75-
# itself plan-hashes the output, so manual trims still cache correctly.
76-
smartcut_path = current_input.replace(".mp4", "_smartcut.mp4")
77-
# Only reuse the sidecar if it's newer than its source. A post-run reframe
78-
# overwrites the clip .mp4 in place, so a stale _smartcut.mp4 from before the
79-
# reframe must NOT be served — re-cut instead. (smart_cut itself plan-hashes,
80-
# but this fixed-name shortcut bypasses that check, hence the explicit mtime.)
81-
if not drop_ranges and os.path.exists(smartcut_path):
82-
try:
83-
fresh = os.path.getmtime(smartcut_path) >= os.path.getmtime(current_input)
84-
except OSError:
85-
fresh = False
86-
if fresh:
87-
intermediate_files.append(smartcut_path)
88-
return smartcut_path
66+
# Smart Cut caching is delegated entirely to smart_cut() itself, which
67+
# writes a plan-hashed output (`{base}_smartcut_{hash}.mp4`) and validates
68+
# cache hits against the source mtime (plus a legacy bare-`_smartcut.mp4`
69+
# back-compat candidate). The previous fixed-name sidecar shortcut here
70+
# built `{base}_smartcut.mp4` — a name smart_cut NEVER produces under the
71+
# current Subtitles→Smart Cut ordering (current_input is `composed_sub_N.mp4`)
72+
# — so it could only ever match stale artifacts that smart_cut already
73+
# handles itself. Removed: always delegate to smart_cut's own correct cache.
8974
transcript = metadata.get("transcript", {})
9075
sc_output, _ = await asyncio.to_thread(
9176
smart_cut,

src/clippyme/domain/job_artifacts.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
"""Filesystem helpers for locating and relocating job artifacts written by main.py."""
22
import glob
33
import json
4+
import logging
45
import os
56
import shutil
67
from typing import Tuple
78

9+
logger = logging.getLogger("clippyme")
10+
811

912
def find_job_metadata_path(job_id: str, output_dir: str) -> str:
1013
"""Return the path to a job's ``*_metadata.json`` file.
@@ -94,5 +97,6 @@ def relocate_root_job_artifacts(job_id: str, job_output_dir: str, output_dir: st
9497
shutil.move(clip_path, dest_clip)
9598

9699
return True
97-
except Exception:
100+
except Exception as exc:
101+
logger.warning("relocate_root_job_artifacts failed for %s: %s", job_id, exc)
98102
return False

src/clippyme/domain/job_results.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,12 @@
33

44
import glob
55
import json
6+
import logging
67
import os
78
import re
89

10+
logger = logging.getLogger("clippyme")
11+
912
ALLOWED_REFRAME_MODES = frozenset({"auto", "disabled", "object"})
1013
MAX_INSTRUCTIONS_LEN = 2000
1114

@@ -112,10 +115,10 @@ def _build_clips(data: dict, base_name: str, job_id: str, output_dir: str, only_
112115
'e': w.get('end', 0.0),
113116
})
114117
backfill_hook_text(clips, words, fallback_title=base_name)
115-
except Exception:
118+
except Exception as exc:
116119
# Never break result loading because of backfill logic —
117120
# stale metadata should still render even if hooks are empty.
118-
pass
121+
logger.debug("backfill_hook_text failed: %s", exc)
119122

120123
result = []
121124
for i, clip in enumerate(clips):

src/clippyme/domain/smartcut.py

Lines changed: 30 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -89,32 +89,41 @@
8989
# same source clip from clobbering each other's _smartcut.mp4 output.
9090
# Lazily populated by _clip_lock(). Workers from FastAPI's executor pool
9191
# may hit the same clip when the user spam-clicks Download.
92-
_CLIP_LOCKS: dict[str, threading.Lock] = {}
92+
#
93+
# path -> [lock, refcount]. The refcount keeps an entry alive for exactly as
94+
# long as some caller holds or waits on it, so eviction can never hand two
95+
# callers different locks for one path (the bug a plain size-cap eviction had).
96+
_CLIP_LOCKS: dict[str, list] = {}
9397
_CLIP_LOCKS_GUARD = threading.Lock()
9498

9599

96-
def _clip_lock(clip_path: str) -> threading.Lock:
97-
"""Return a process-wide lock unique to `clip_path`. Lazy + thread-safe."""
100+
@contextlib.contextmanager
101+
def _clip_lock(clip_path: str):
102+
"""Process-wide per-path mutex, reference-counted and self-bounding.
103+
104+
Usage: ``with _clip_lock(path): ...`` — serialises smart_cut on one clip.
105+
"""
98106
abs_path = os.path.abspath(clip_path)
99107
with _CLIP_LOCKS_GUARD:
100-
lock = _CLIP_LOCKS.get(abs_path)
101-
if lock is None:
102-
lock = threading.Lock()
103-
_CLIP_LOCKS[abs_path] = lock
104-
# Bound the registry WITHOUT ever evicting a lock another thread
105-
# may still hold — handing two callers different locks for one
106-
# path would defeat the mutex. Only drop currently-free locks.
107-
if len(_CLIP_LOCKS) > 256:
108-
for k in list(_CLIP_LOCKS.keys()):
109-
if len(_CLIP_LOCKS) <= 256:
110-
break
111-
other = _CLIP_LOCKS[k]
112-
if other is not lock and other.acquire(blocking=False):
113-
try:
114-
del _CLIP_LOCKS[k]
115-
finally:
116-
other.release()
117-
return lock
108+
entry = _CLIP_LOCKS.get(abs_path)
109+
if entry is None:
110+
entry = [threading.Lock(), 0]
111+
_CLIP_LOCKS[abs_path] = entry
112+
entry[1] += 1
113+
lock = entry[0]
114+
try:
115+
with lock:
116+
yield lock
117+
finally:
118+
with _CLIP_LOCKS_GUARD:
119+
entry[1] -= 1
120+
# Drop only when no one else references this exact entry AND the
121+
# registry has grown past the soft cap. Safe: refcount 0 means no
122+
# holder/waiter could be handed a stale lock.
123+
if entry[1] <= 0 and len(_CLIP_LOCKS) > 256:
124+
# Only delete if it's still the same entry object.
125+
if _CLIP_LOCKS.get(abs_path) is entry:
126+
del _CLIP_LOCKS[abs_path]
118127

119128

120129
# Regex that strips ALL non-alphanumeric chars (unicode-aware) for the

0 commit comments

Comments
 (0)