fix: address PR review feedback (6 issues)
Critical: - findLogsForTask: call worker directly instead of wrapping in wrapTeamHandler, so failures propagate to catch and trigger the main-thread fallback correctly. - TeamDataWorkerClient: scope error/exit handlers to the specific worker instance to prevent a stale worker's exit event from rejecting pending requests on a replacement worker (race condition). Major: - TeamDataWorkerClient: validate teamName and taskId before forwarding to worker thread (input sanitization). - team-data-worker: include status, since, and intervals length in cache key to prevent stale results after filter changes. - team-data-worker: move logsInFlight.delete() into .finally() so rejected lookups don't poison the in-flight map permanently. - MemberLogsTab: reset refreshCountRef and refreshing state in effect cleanup to prevent the refresh indicator from latching on permanently when the effect tears down mid-refresh (e.g. on tab switch).
This commit is contained in:
parent
da58917032
commit
0e83d99e0a
4 changed files with 44 additions and 20 deletions
|
|
@ -2153,13 +2153,14 @@ async function handleGetLogsForTask(
|
||||||
: undefined,
|
: undefined,
|
||||||
}
|
}
|
||||||
: undefined;
|
: undefined;
|
||||||
// Prefer worker thread to keep main event loop responsive
|
// Prefer worker thread to keep main event loop responsive.
|
||||||
|
// Call worker directly (not via wrapTeamHandler) so that failures
|
||||||
|
// propagate to the catch block and trigger the main-thread fallback.
|
||||||
const worker = getTeamDataWorkerClient();
|
const worker = getTeamDataWorkerClient();
|
||||||
if (worker.isAvailable()) {
|
if (worker.isAvailable()) {
|
||||||
try {
|
try {
|
||||||
return await wrapTeamHandler('getLogsForTask', () =>
|
const result = await worker.findLogsForTask(vTeam.value!, vTask.value!, opts);
|
||||||
worker.findLogsForTask(vTeam.value!, vTask.value!, opts)
|
return { success: true, data: result };
|
||||||
);
|
|
||||||
} catch (workerErr) {
|
} catch (workerErr) {
|
||||||
logger.warn(
|
logger.warn(
|
||||||
`[teams:getLogsForTask] worker failed, falling back: ${workerErr instanceof Error ? workerErr.message : workerErr}`
|
`[teams:getLogsForTask] worker failed, falling back: ${workerErr instanceof Error ? workerErr.message : workerErr}`
|
||||||
|
|
|
||||||
|
|
@ -18,6 +18,8 @@ import type { TeamDataWorkerRequest, TeamDataWorkerResponse } from './teamDataWo
|
||||||
|
|
||||||
const logger = createLogger('Service:TeamDataWorkerClient');
|
const logger = createLogger('Service:TeamDataWorkerClient');
|
||||||
const WORKER_CALL_TIMEOUT_MS = 30_000;
|
const WORKER_CALL_TIMEOUT_MS = 30_000;
|
||||||
|
const SAFE_NAME_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]{0,127}$/;
|
||||||
|
const SAFE_ID_RE = /^[a-zA-Z0-9][a-zA-Z0-9-]{0,63}$/;
|
||||||
|
|
||||||
function makeId(): string {
|
function makeId(): string {
|
||||||
return `${Date.now()}-${crypto.randomUUID().slice(0, 12)}`;
|
return `${Date.now()}-${crypto.randomUUID().slice(0, 12)}`;
|
||||||
|
|
@ -68,9 +70,10 @@ export class TeamDataWorkerClient {
|
||||||
if (!this.workerPath) throw new Error('Worker not available');
|
if (!this.workerPath) throw new Error('Worker not available');
|
||||||
if (this.worker) return this.worker;
|
if (this.worker) return this.worker;
|
||||||
|
|
||||||
this.worker = new Worker(this.workerPath);
|
const w = new Worker(this.workerPath);
|
||||||
|
this.worker = w;
|
||||||
|
|
||||||
this.worker.on('message', (msg: TeamDataWorkerResponse) => {
|
w.on('message', (msg: TeamDataWorkerResponse) => {
|
||||||
const entry = this.pending.get(msg.id);
|
const entry = this.pending.get(msg.id);
|
||||||
if (!entry) return;
|
if (!entry) return;
|
||||||
this.pending.delete(msg.id);
|
this.pending.delete(msg.id);
|
||||||
|
|
@ -81,7 +84,11 @@ export class TeamDataWorkerClient {
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
this.worker.on('error', (err) => {
|
// Scope error/exit handlers to this specific worker instance.
|
||||||
|
// Without this guard, a stale worker's exit event can reject
|
||||||
|
// pending requests that belong to a newer replacement worker.
|
||||||
|
w.on('error', (err) => {
|
||||||
|
if (this.worker !== w) return;
|
||||||
logger.error('Worker error', err);
|
logger.error('Worker error', err);
|
||||||
for (const [, entry] of this.pending) {
|
for (const [, entry] of this.pending) {
|
||||||
entry.reject(err instanceof Error ? err : new Error(String(err)));
|
entry.reject(err instanceof Error ? err : new Error(String(err)));
|
||||||
|
|
@ -90,7 +97,8 @@ export class TeamDataWorkerClient {
|
||||||
this.worker = null;
|
this.worker = null;
|
||||||
});
|
});
|
||||||
|
|
||||||
this.worker.on('exit', (code) => {
|
w.on('exit', (code) => {
|
||||||
|
if (this.worker !== w) return;
|
||||||
if (code !== 0) logger.warn(`Worker exited with code ${code}`);
|
if (code !== 0) logger.warn(`Worker exited with code ${code}`);
|
||||||
for (const [, entry] of this.pending) {
|
for (const [, entry] of this.pending) {
|
||||||
entry.reject(new Error(`Worker exited with code ${code}`));
|
entry.reject(new Error(`Worker exited with code ${code}`));
|
||||||
|
|
@ -99,7 +107,7 @@ export class TeamDataWorkerClient {
|
||||||
this.worker = null;
|
this.worker = null;
|
||||||
});
|
});
|
||||||
|
|
||||||
return this.worker;
|
return w;
|
||||||
}
|
}
|
||||||
|
|
||||||
private call(
|
private call(
|
||||||
|
|
@ -133,6 +141,7 @@ export class TeamDataWorkerClient {
|
||||||
}
|
}
|
||||||
|
|
||||||
async getTeamData(teamName: string): Promise<TeamData> {
|
async getTeamData(teamName: string): Promise<TeamData> {
|
||||||
|
if (!SAFE_NAME_RE.test(teamName)) throw new Error('Invalid teamName');
|
||||||
return this.call('getTeamData', { teamName }) as Promise<TeamData>;
|
return this.call('getTeamData', { teamName }) as Promise<TeamData>;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -146,6 +155,8 @@ export class TeamDataWorkerClient {
|
||||||
since?: string;
|
since?: string;
|
||||||
}
|
}
|
||||||
): Promise<MemberLogSummary[]> {
|
): Promise<MemberLogSummary[]> {
|
||||||
|
if (!SAFE_NAME_RE.test(teamName)) throw new Error('Invalid teamName');
|
||||||
|
if (!SAFE_ID_RE.test(taskId)) throw new Error('Invalid taskId');
|
||||||
return this.call('findLogsForTask', { teamName, taskId, options }) as Promise<
|
return this.call('findLogsForTask', { teamName, taskId, options }) as Promise<
|
||||||
MemberLogSummary[]
|
MemberLogSummary[]
|
||||||
>;
|
>;
|
||||||
|
|
|
||||||
|
|
@ -44,7 +44,7 @@ parentPort?.on('message', async (msg: TeamDataWorkerRequest) => {
|
||||||
}
|
}
|
||||||
case 'findLogsForTask': {
|
case 'findLogsForTask': {
|
||||||
const { teamName, taskId, options } = msg.payload;
|
const { teamName, taskId, options } = msg.payload;
|
||||||
const cacheKey = `${teamName}:${taskId}:${options?.owner ?? ''}`;
|
const cacheKey = `${teamName}:${taskId}:${options?.owner ?? ''}:${options?.status ?? ''}:${options?.since ?? ''}:${options?.intervals?.length ?? 0}`;
|
||||||
|
|
||||||
// Check result cache
|
// Check result cache
|
||||||
const cached = logsResultCache.get(cacheKey);
|
const cached = logsResultCache.get(cacheKey);
|
||||||
|
|
@ -56,16 +56,20 @@ parentPort?.on('message', async (msg: TeamDataWorkerRequest) => {
|
||||||
// Dedup concurrent calls
|
// Dedup concurrent calls
|
||||||
let promise = logsInFlight.get(cacheKey) as Promise<MemberLogSummary[]> | undefined;
|
let promise = logsInFlight.get(cacheKey) as Promise<MemberLogSummary[]> | undefined;
|
||||||
if (!promise) {
|
if (!promise) {
|
||||||
promise = logsFinder.findLogsForTask(teamName, taskId, options).then((result) => {
|
promise = logsFinder
|
||||||
logsInFlight.delete(cacheKey);
|
.findLogsForTask(teamName, taskId, options)
|
||||||
logsResultCache.set(cacheKey, { result, cachedAt: Date.now() });
|
.then((result) => {
|
||||||
// Cap cache
|
logsResultCache.set(cacheKey, { result, cachedAt: Date.now() });
|
||||||
if (logsResultCache.size > 100) {
|
// Cap cache
|
||||||
const firstKey = logsResultCache.keys().next().value;
|
if (logsResultCache.size > 100) {
|
||||||
if (firstKey !== undefined) logsResultCache.delete(firstKey);
|
const firstKey = logsResultCache.keys().next().value;
|
||||||
}
|
if (firstKey !== undefined) logsResultCache.delete(firstKey);
|
||||||
return result;
|
}
|
||||||
});
|
return result;
|
||||||
|
})
|
||||||
|
.finally(() => {
|
||||||
|
logsInFlight.delete(cacheKey);
|
||||||
|
});
|
||||||
logsInFlight.set(cacheKey, promise);
|
logsInFlight.set(cacheKey, promise);
|
||||||
}
|
}
|
||||||
const result = await promise;
|
const result = await promise;
|
||||||
|
|
|
||||||
|
|
@ -488,6 +488,14 @@ export const MemberLogsTab = ({
|
||||||
return () => {
|
return () => {
|
||||||
cancelled = true;
|
cancelled = true;
|
||||||
if (interval) clearInterval(interval);
|
if (interval) clearInterval(interval);
|
||||||
|
// Reset refresh state so the indicator doesn't stay latched
|
||||||
|
// when the effect tears down mid-refresh (e.g. tab switch).
|
||||||
|
refreshCountRef.current = 0;
|
||||||
|
if (refreshHideTimeoutRef.current) {
|
||||||
|
clearTimeout(refreshHideTimeoutRef.current);
|
||||||
|
refreshHideTimeoutRef.current = null;
|
||||||
|
}
|
||||||
|
setRefreshing(false);
|
||||||
};
|
};
|
||||||
// eslint-disable-next-line react-hooks/exhaustive-deps -- intervalsKey + taskSince drive refresh; deps intentionally minimal to avoid refetch loops
|
// eslint-disable-next-line react-hooks/exhaustive-deps -- intervalsKey + taskSince drive refresh; deps intentionally minimal to avoid refetch loops
|
||||||
}, [teamName, memberName, taskId, taskOwner, taskStatus, intervalsKey, taskSince, isTabActive]);
|
}, [teamName, memberName, taskId, taskOwner, taskStatus, intervalsKey, taskSince, isTabActive]);
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue