Repository navigation
fix(explorer): cancel superseded query admissions - #4823
Conversation
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| useEffect(() => { | ||
| return () => { | ||
| requestGeneration.current += 1; | ||
| const previous = acceptedRun.current; | ||
| acceptedRun.current = null; | ||
| if (previous !== null) void cancelPlannerQuery(previous.scope, previous.id); | ||
| }; |
There was a problem hiding this comment.
🔴 Deep-linked searches cancel during mount
With ?q=graph, requestGeneration advances during StrictMode's simulated cleanup, invalidating the restored search. useExplorerUrlState restores only once, so the admitted run gets cancelled and the search remains pending.
Learn more
The production root wraps the router in React.StrictMode. On initial mount, useExplorerUrlState submits a query from the URL in an effect guarded by restored. StrictMode runs effect setup, cleanup, and setup again without discarding refs. This cleanup invalidates that admission, but restored.current prevents the second setup from dispatching a replacement. The old admission is then cancelled and no status polling starts, leaving the restored query pending.
Example: Open /explorer?q=graph in the dashboard. The first effect dispatches graph; the simulated cleanup increments its generation. When its POST returns pending, the controller sends DELETE instead of installing the run; the page still shows graph as submitted.
Recommended fix: Distinguish an actual unmount from StrictMode's effect replay, or make URL restoration re-dispatch when the replay invalidates its admission. Add an ExplorerPage test under StrictMode that opens a ?q= URL, waits for admission, and checks that its run stays active.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const retireAcceptedRun = () => { | ||
| const previous = acceptedRun.current; | ||
| acceptedRun.current = null; | ||
| if (previous !== null) void cancelPlannerQuery(previous.scope, previous.id); | ||
| }; |
There was a problem hiding this comment.
🟡 Failed cancellations leave orphaned runs
When a superseding search's DELETE fails, retireAcceptedRun discards the run without checking the typed result. The old run keeps consuming server work until its deadline, with no cancellation retry or visible failure.
Learn more
fetchEnvelope returns { outcome: 'transport' } on offline and HTTP errors. The new fire-and-forget cancellation discards that result and clears acceptedRun first; the same pattern occurs for stale POST responses and unmount cleanup. No subsequent action retains the run identity to complete the cancellation. The daemon keeps pending runs until cancelled or timed out, and its run registry has finite capacity.
Example: Submit alpha, then beta while the DELETE for alpha receives HTTP 503. The controller forgets alpha and shows beta; alpha continues doing work, and nothing reports the failed cancellation.
Recommended fix: Centralize automatic cancellation and handle its EnvelopeResult explicitly. Retain failed cancellation identities for an appropriate bounded retry or expose a typed failure when retries are impossible; handle both non-2xx transport outcomes and rejected requests.
Was this helpful? React with 👍 or 👎 to provide feedback.
Scope — Refs #4432 (behavioral correctness slice; NOT full acceptance)
Problem: Explorer's controller used to accept every asynchronous POST response as the currently active query run. A slow older creation response arriving after a newer search could replace the newer run ID. Reset, supersession, scope changes and unmount stopped status polling but left accepted server-side work active until its deadline.
Change: Bind each create attempt to its exact scope and a monotonic client request generation. Only the current attempt can install an active run ID. Older attempts that finish after supersession/unmount explicitly cancel their own accepted run, even if React Query no longer has a mounted observer. Retiring an active search (including a scope switch or unmount) sends DELETE to the original project's route, not the new scope; completed runs are not cancelled. Source ordering, partial results, current status polling, server authorization and existing contracts remain unchanged.
Regressions committed: Two real React hook/fake-daemon DOM tests in
controllerPolling.dom.test.tsxuse a withheld earlier POST to assert the newer query remains active while the stale accepted run is cancelled, and assert supersession/unmount cancel the exact active run identities. Existing polling tests and assertions remain.Review: Focused commits
856fad24and87a5dff, based onmaster91c72aba7750; exactly two Explorer files. Independently reviewed GitHub commit diff andmaster...headcompare; fast-forward branch push usedforce=falseand exact expected previous head. No existing PR branch or peer changes overwritten.Verification: Four nonzero standalone Node state-machine assertions passed (including baseline old-behavior failure and new-generation cancellation). This is not execution of the committed React tests. This runner has Node/TypeScript but no repository checkout,
pnpm, React/Vitest dependencies or Bazel; the in-repo DOM tests have not been executed here. Exact-head hosted CI #42530450102 is queued and must run the Dashboard tests before this is considered fully verified. Do not claim a Rust/Bazel/native macOS pass if those jobs are skipped.Outstanding issue acceptance: This is not the #4432 lifecycle consolidation. The dashboard's
QUERY_RUNSjob registry, independent create/status/cancel endpoints, polling and duplicated state/finality still exist. Complete #4432 needs a request-owned progressively streamed response or the canonical daemon operation stream with an explicit compatibility/migration plan, generated client contract/UI cutover, cross-project and real cancellation tests. Keep this draft unmerged and #4432 open on the evidence above. No placeholder replacement query manager was introduced.Coordinating CI/eligible merges remains with the separate PR-maintenance routine. #4776 HOLD and #4763 READ ONLY.
Follow-up verification
The controller takes ownership when the admission response arrives, including when the surface unmounts before React Query finishes its callbacks. Pending late admissions are cancelled at their original scope. Already completed admissions require no cancellation. The polling fixture distinguishes GET status reads from DELETE cancellation. The callback-interleaving test failed before the change and passed afterward. The full Bazel dashboard suite passed all 2,245 tests, and dashboard typechecking passed. The existing draft hold and the remaining #4432 scope still apply.