From 1f8551515467bca7d6c740597187542db87a82fc Mon Sep 17 00:00:00 2001 From: James Brunton Date: Wed, 26 Aug 2026 16:18:51 +0100 Subject: [PATCH] Fix local polling bug --- .../components/policies/policyRunStore.ts | 22 +++----- .../policies/useClassificationPolicy.ts | 21 +++----- .../policies/usePolicyAutoRun.chain.test.tsx | 53 ++++++++++++++++++- .../components/policies/usePolicyAutoRun.ts | 10 +++- 4 files changed, 75 insertions(+), 31 deletions(-) diff --git a/frontend/editor/src/proprietary/components/policies/policyRunStore.ts b/frontend/editor/src/proprietary/components/policies/policyRunStore.ts index a598b8b9b1..180c374247 100644 --- a/frontend/editor/src/proprietary/components/policies/policyRunStore.ts +++ b/frontend/editor/src/proprietary/components/policies/policyRunStore.ts @@ -45,16 +45,12 @@ export interface PolicyRunRecord { /** Set while an auto-retry is pending after a transient (queue-full) rejection, so the activity * feed shows a soft "busy" row instead of a hard failure during the backoff window. */ retrying?: boolean; + /** A run computed entirely in the browser - it has no server run behind it, + * so it must never be polled for status (a status poll 404s and would flip a + * succeeded run to FAILED) or reconciled against the server. */ + browserLocal?: boolean; /** Epoch ms when the run was dispatched. */ startedAt: number; - /** - * Ran in the browser (the local classification heuristic), not on a backend. Such a run has no - * server-side status to poll, and - crucially - must NOT claim the (policy, file) dispatch key: - * it is the first pass, not the policy's run, so claiming it would suppress the server run the - * verdict may still need to escalate to. Distinct from {@link target}, which says which BACKEND - * holds a real run's outputs. - */ - browserLocal?: boolean; } /** Statuses of a run that is still executing (not yet settled). */ @@ -224,15 +220,11 @@ export function recordRunStart(record: PolicyRunRecord) { const waveStartedAt = state.runs.some(isRunInFlight) ? state.waveStartedAt : record.startedAt; - // A browser-local run is the first pass, not the policy's run: claiming the dispatch key here - // would permanently suppress the server run its verdict may still need to escalate to. - const claimsDispatch = !record.browserLocal; state = { runs: capRuns([record, ...state.runs]), - dispatched: - !claimsDispatch || state.dispatched.includes(key) - ? state.dispatched - : [...state.dispatched, key], + dispatched: state.dispatched.includes(key) + ? state.dispatched + : [...state.dispatched, key], waveStartedAt, }; emit(); diff --git a/frontend/editor/src/proprietary/components/policies/useClassificationPolicy.ts b/frontend/editor/src/proprietary/components/policies/useClassificationPolicy.ts index 3173066727..3dd60caa22 100644 --- a/frontend/editor/src/proprietary/components/policies/useClassificationPolicy.ts +++ b/frontend/editor/src/proprietary/components/policies/useClassificationPolicy.ts @@ -26,12 +26,6 @@ import { orderedRewritingCategories, } from "@app/data/classificationPolicy"; -/** - * Dispatch-store key namespace for "this file's local pass has been metered". Deliberately NOT the - * Classification category id: that key is the server escalation's own guard, so metering under it - * would tell the auto-run the policy had already run and kill the escalation entirely. - */ -export const LOCAL_METER_CATEGORY = `${CLASSIFICATION_CATEGORY_ID}:local-meter`; /** Files classified per idle pass, so a large library drains over several ticks. */ const CLASSIFY_BATCH = 3; /** How long to wait for an upload's bytes to land in IndexedDB (20 × 250ms ≈ 5s). @@ -147,7 +141,7 @@ export function useClassificationPolicy(): void { if (claimed.current.has(key)) continue; claimed.current.add(key); const verdict = await classifyStub( - stub.id, + stub.id as FileId, stub.name, stub.size ?? 0, ); @@ -156,11 +150,11 @@ export function useClassificationPolicy(): void { if (verdict == null) continue; // Deliver unconditionally - a re-render must never discard a computed // (and already metered) result. Writes are idempotent. - updateStirlingFileStub(stub.id, { + updateStirlingFileStub(stub.id as FileId, { classificationLabels: verdict.labels, classificationConfidence: verdict.confidence, }); - const ok = await fileStorage.updateFileMetadata(stub.id, { + const ok = await fileStorage.updateFileMetadata(stub.id as FileId, { classificationLabels: verdict.labels, classificationConfidence: verdict.confidence, }); @@ -230,7 +224,9 @@ async function classifyStub( // A local run is still a billable policy run, so it belongs in the activity feed; recorded only // once the bytes are in hand, so a file whose bytes never land leaves no phantom row. - const alreadyMetered = isDispatched(LOCAL_METER_CATEGORY, fileId); + // Read before recordRunStart, which takes the dispatch key itself and would otherwise always + // answer "already dispatched", silently stopping metering. + const alreadyMetered = isDispatched(CLASSIFICATION_CATEGORY_ID, fileId); const runId = `local-${CLASSIFICATION_CATEGORY_ID}-${fileId}-${Date.now()}`; recordRunStart({ runId, @@ -239,8 +235,7 @@ async function classifyStub( fileName, fileSize, target: "local", - // Ran here, not on a backend: nothing to poll, and it must not claim the classification - // dispatch key - that key is what the server escalation checks before running. + // The heuristic ran in the browser - there is no server run to poll (see the poll effect). browserLocal: true, status: "RUNNING", outputs: [], @@ -271,7 +266,7 @@ async function classifyStub( labels, }); } - markDispatched(LOCAL_METER_CATEGORY, fileId); + markDispatched(CLASSIFICATION_CATEGORY_ID, fileId); // Labels, no output file - the same settle shape the server-run classification uses. updateRun(runId, { status: "COMPLETED", diff --git a/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.chain.test.tsx b/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.chain.test.tsx index 77d1f938d8..89ca278e97 100644 --- a/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.chain.test.tsx +++ b/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.chain.test.tsx @@ -59,12 +59,14 @@ import { usePolicyAutoRun } from "@app/components/policies/usePolicyAutoRun"; import { recordRunStart, updateRun, + getRun, resetPolicyRuns, } from "@app/components/policies/policyRunStore"; -import { runStoredPolicy } from "@app/services/policyApi"; +import { runStoredPolicy, getPolicyRun } from "@app/services/policyApi"; import { fileStorage } from "@app/services/fileStorage"; const runStored = vi.mocked(runStoredPolicy); +const getRunStatus = vi.mocked(getPolicyRun); const getFile = vi.mocked(fileStorage.getStirlingFile); /** Reset the shared file list between tests without swapping the array identity. */ @@ -176,4 +178,53 @@ describe("auto-run ordered chaining", () => { expect(runStored).not.toHaveBeenCalled(); }); + + it("never polls a browser-local run (no server behind it) so its success can't 404 to FAILED", async () => { + getRunStatus.mockResolvedValue({ + runId: "srv-1", + policyId: null, + status: "COMPLETED", + currentStep: 1, + stepCount: 1, + error: null, + outputs: [], + } as never); + // A browser-local heuristic run and a real server run, both left in flight. + recordRunStart({ + runId: "local-1", + categoryId: "classification", + fileId: "f1", + fileName: "d.pdf", + fileSize: 1, + target: "local", + browserLocal: true, + status: "RUNNING", + outputs: [], + error: null, + startedAt: 0, + }); + recordRunStart({ + runId: "srv-1", + categoryId: "security", + fileId: "f2", + fileName: "d.pdf", + fileSize: 1, + target: "saas", + status: "RUNNING", + outputs: [], + error: null, + startedAt: 0, + }); + + renderHook(() => usePolicyAutoRun()); + await act(async () => { + await vi.advanceTimersByTimeAsync(700); // past the first poll (500ms) + }); + + const polled = getRunStatus.mock.calls.map((c) => c[0]); + expect(polled).toContain("srv-1"); // the server run is polled… + expect(polled).not.toContain("local-1"); // …the browser-local one never is + // And its success is left intact, not flipped to FAILED by a 404 streak. + expect(getRun("local-1")?.status).toBe("RUNNING"); + }); }); diff --git a/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.ts b/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.ts index 05b368f18b..de8bb5ad3e 100644 --- a/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.ts +++ b/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.ts @@ -278,10 +278,16 @@ export function usePolicyAutoRun(): void { } }, [runs, policies, orderedUploadCategories]); - // Poll each in-flight run to a terminal state. + // Poll each in-flight run to a terminal state. A browser-local run (the classification heuristic's + // first pass) has no server run behind it, so polling it 404s and would flip its success to FAILED. useEffect(() => { for (const run of runs) { - if (isTerminal(run.status) || polling.current.has(run.runId)) continue; + if ( + run.browserLocal || + isTerminal(run.status) || + polling.current.has(run.runId) + ) + continue; polling.current.add(run.runId); void poll(run.runId, onRunFinished).finally(() => polling.current.delete(run.runId),