Skip to content

Commit 6aa1fd8

Browse files
committed
fix(face-clusters): age recluster results from completion; harden poll loop
Addresses review feedback on the async reclustering flow. - ReclusterTask now records finished_at, and the cleanup loop ages terminal results from completion time instead of creation time. Previously a job that ran close to the TTL could be reaped almost immediately after finishing, making polling clients see a 404 instead of the result. - useGlobalRecluster now polls with a self-scheduling setTimeout (the next tick is queued only after the current request resolves, so status requests can't stack/overlap) and guards every async callback with a monotonically increasing run id. A newer trigger() — or unmount — invalidates older runs so they cannot keep polling or overwrite state, fixing the orphaned-interval leak on repeated triggers.
1 parent 2f1554f commit 6aa1fd8

2 files changed

Lines changed: 42 additions & 13 deletions

File tree

backend/app/routes/face_clusters.py

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ class ReclusterTask:
5454
faces_skipped: Optional[int] = None
5555
message: Optional[str] = None
5656
created_at: datetime = field(default_factory=lambda: datetime.now(timezone.utc))
57+
finished_at: Optional[datetime] = None
5758
task: Optional[asyncio.Task] = None
5859

5960

@@ -66,9 +67,10 @@ class ReclusterTask:
6667
# with no await between check-and-set.
6768
_active_recluster_task_id: Optional[str] = None
6869

69-
# How long a finished task's result is retained for polling before the cleanup
70-
# loop reaps it. Running tasks are never reaped (they are bounded to one by the
71-
# concurrency guard above).
70+
# How long a finished task's result is retained for polling, measured from when
71+
# it finished (not when it started) so a long-running job's result isn't reaped
72+
# almost immediately after completion. Running tasks are never reaped (they are
73+
# bounded to one by the concurrency guard above).
7274
RECLUSTER_TASK_TTL_MINUTES = 15
7375

7476

@@ -94,8 +96,10 @@ async def _run_global_recluster(task_id: str):
9496
entry.status = "error"
9597
entry.message = f"Global reclustering failed: {str(e)}"
9698
finally:
97-
# Release the concurrency guard so a new job can be started, while the
98-
# finished result stays in recluster_tasks for the client to poll.
99+
# Stamp completion time so cleanup ages the result from when it finished,
100+
# and release the concurrency guard so a new job can be started, while
101+
# the finished result stays in recluster_tasks for the client to poll.
102+
entry.finished_at = datetime.now(timezone.utc)
99103
if _active_recluster_task_id == task_id:
100104
_active_recluster_task_id = None
101105

@@ -113,7 +117,8 @@ async def _cleanup_stale_recluster_tasks():
113117
tid
114118
for tid, entry in recluster_tasks.items()
115119
if entry.status != "running"
116-
and (now - entry.created_at).total_seconds()
120+
and entry.finished_at is not None
121+
and (now - entry.finished_at).total_seconds()
117122
> RECLUSTER_TASK_TTL_MINUTES * 60
118123
]
119124
for tid in stale:

frontend/src/hooks/useGlobalRecluster.tsx

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -39,34 +39,54 @@ const idleState: ReclusterState = {
3939
*/
4040
export function useGlobalRecluster() {
4141
const queryClient = useQueryClient();
42-
const pollHandleRef = useRef<ReturnType<typeof setInterval> | null>(null);
42+
const pollTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
43+
// Identifies the latest trigger() call. Every async callback captures the id
44+
// it started with and bails if a newer trigger (or unmount) has since bumped
45+
// it, so stale runs can't update state or keep polling.
46+
const runIdRef = useRef(0);
4347
const [state, setState] = useState<ReclusterState>(idleState);
4448

4549
const stopPolling = useCallback(() => {
46-
if (pollHandleRef.current) {
47-
clearInterval(pollHandleRef.current);
48-
pollHandleRef.current = null;
50+
if (pollTimeoutRef.current) {
51+
clearTimeout(pollTimeoutRef.current);
52+
pollTimeoutRef.current = null;
4953
}
5054
}, []);
5155

52-
useEffect(() => stopPolling, [stopPolling]);
56+
// On unmount, invalidate any in-flight run and stop polling.
57+
useEffect(() => {
58+
return () => {
59+
runIdRef.current += 1;
60+
stopPolling();
61+
};
62+
}, [stopPolling]);
5363

5464
const trigger = useCallback(() => {
65+
const runId = runIdRef.current + 1;
66+
runIdRef.current = runId;
67+
const isActive = () => runId === runIdRef.current;
68+
5569
stopPolling();
5670
setState({ ...idleState, isPending: true });
5771

5872
startGlobalReclustering()
5973
.then((startRes) => {
74+
if (!isActive()) return;
75+
6076
const taskId = startRes.data?.task_id;
6177
if (!taskId) {
6278
throw new Error('Backend did not return a task_id for reclustering.');
6379
}
6480

65-
pollHandleRef.current = setInterval(async () => {
81+
// Self-scheduling poll: the next tick is only queued after the current
82+
// request resolves, so requests can't stack up or overlap.
83+
const poll = async () => {
6684
try {
6785
const statusRes = await getGlobalReclusterStatus(taskId);
86+
if (!isActive()) return;
6887

6988
if (statusRes.data?.status === 'running') {
89+
pollTimeoutRef.current = setTimeout(poll, POLL_INTERVAL_MS);
7090
return;
7191
}
7292

@@ -92,6 +112,7 @@ export function useGlobalRecluster() {
92112
successMessage: statusRes.message,
93113
});
94114
} catch (err) {
115+
if (!isActive()) return;
95116
stopPolling();
96117
setState({
97118
...idleState,
@@ -100,9 +121,12 @@ export function useGlobalRecluster() {
100121
errorMessage: getErrorMessage(err),
101122
});
102123
}
103-
}, POLL_INTERVAL_MS);
124+
};
125+
126+
poll();
104127
})
105128
.catch((err) => {
129+
if (!isActive()) return;
106130
setState({
107131
...idleState,
108132
isError: true,

0 commit comments

Comments
 (0)