mirror of
https://github.com/stablyai/orca.git
synced 2026-10-17 08:03:14 +00:00
fix: address pr-bug-scan validated finding from #7769
Key relay listFiles single-flight per (clientId,key) instead of clientId only; different-key concurrent callers no longer supersede each other, blocking the superseded-error abort at scan-coordinator:
This commit is contained in:
@@ -66,7 +66,6 @@ import {
|
||||
import { RelayDispatcher } from './dispatcher'
|
||||
import { RelayContext } from './context'
|
||||
import { FsHandler } from './fs-handler'
|
||||
import { LIST_FILES_SUPERSEDED_MESSAGE } from './fs-list-files-scan-coordinator'
|
||||
|
||||
async function flushPipe(): Promise<void> {
|
||||
// The in-memory pipe defers each hop with setImmediate; a few macrotask
|
||||
@@ -151,29 +150,31 @@ describe('Integration: cancellable fs.listFiles (#7721)', () => {
|
||||
expect(fakeListFiles.scans[0].signal?.aborted).toBe(true)
|
||||
})
|
||||
|
||||
it('a scan for a different workspace supersedes the previous scan', async () => {
|
||||
const first = mux.request('fs.listFiles', { rootPath: '/workspace/a' })
|
||||
// Attach the rejection handler up front: the error response arrives on a
|
||||
// macrotask inside flushPipe, before any later `await expect` could.
|
||||
const firstOutcome = first.then(
|
||||
() => null,
|
||||
(err: Error) => err
|
||||
)
|
||||
// Why #7769: two independent callers on one SSH connection (one relay
|
||||
// clientId) — e.g. the editor's markdown-document scan (no excludePaths) and
|
||||
// Quick Open (nested-worktree excludePaths) — carry different scan keys.
|
||||
// Neither may supersede the other: the earlier one must complete and return
|
||||
// its own result, not fail with a "superseded" error nobody triggered.
|
||||
it('runs two concurrent unrelated listings without superseding either', async () => {
|
||||
const markdown = mux.request('fs.listFiles', { rootPath: '/workspace/a' })
|
||||
await flushPipe()
|
||||
expect(fakeListFiles.scans).toHaveLength(1)
|
||||
|
||||
const second = mux.request('fs.listFiles', { rootPath: '/workspace/b' })
|
||||
const quickOpen = mux.request('fs.listFiles', {
|
||||
rootPath: '/workspace/a',
|
||||
excludePaths: ['/workspace/a/worktrees/feature']
|
||||
})
|
||||
await flushPipe()
|
||||
expect(fakeListFiles.scans).toHaveLength(2)
|
||||
|
||||
// The stale scan is aborted and its request fails fast — it does not run
|
||||
// to completion behind the new one and does not hit the 30s timeout.
|
||||
expect(fakeListFiles.scans[0].signal?.aborted).toBe(true)
|
||||
const firstError = await firstOutcome
|
||||
expect(firstError?.message).toContain(LIST_FILES_SUPERSEDED_MESSAGE)
|
||||
// The first scan is not aborted by the second — both stay live.
|
||||
expect(fakeListFiles.scans[0].signal?.aborted).toBe(false)
|
||||
expect(fakeListFiles.scans[1].signal?.aborted).toBe(false)
|
||||
|
||||
fakeListFiles.scans[1].resolve(['b.ts'])
|
||||
await expect(second).resolves.toEqual(['b.ts'])
|
||||
fakeListFiles.scans[0].resolve(['README.md'])
|
||||
fakeListFiles.scans[1].resolve(['src/index.ts'])
|
||||
await expect(markdown).resolves.toEqual(['README.md'])
|
||||
await expect(quickOpen).resolves.toEqual(['src/index.ts'])
|
||||
})
|
||||
|
||||
it('identical concurrent requests coalesce into a single scan', async () => {
|
||||
|
||||
@@ -1,8 +1,5 @@
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import {
|
||||
LIST_FILES_SUPERSEDED_MESSAGE,
|
||||
ListFilesScanCoordinator
|
||||
} from './fs-list-files-scan-coordinator'
|
||||
import { ListFilesScanCoordinator } from './fs-list-files-scan-coordinator'
|
||||
import {
|
||||
FileListingCancelledError,
|
||||
isFileListingCancellation
|
||||
@@ -67,20 +64,27 @@ describe('ListFilesScanCoordinator', () => {
|
||||
await expect(second).resolves.toEqual(['x.ts'])
|
||||
})
|
||||
|
||||
it('supersedes a different-key scan: aborts the old one and rejects it fast', async () => {
|
||||
// Why #7769: two unrelated callers on one relay client (one SSH connection)
|
||||
// — e.g. the editor's markdown-document scan and Quick Open — carry
|
||||
// different keys. Neither may supersede the other: the loser used to reject
|
||||
// with the "superseded" error even though nobody cancelled it.
|
||||
it('runs different-key scans concurrently instead of superseding (#7769)', async () => {
|
||||
const coordinator = new ListFilesScanCoordinator()
|
||||
const scan = controllableScan()
|
||||
const scanA = controllableScan()
|
||||
const scanB = controllableScan()
|
||||
|
||||
const first = coordinator.run({ clientId: 1, key: 'workspace-a', start: scan.start })
|
||||
const second = coordinator.run({ clientId: 1, key: 'workspace-b', start: scan.start })
|
||||
const first = coordinator.run({ clientId: 1, key: 'workspace-a', start: scanA.start })
|
||||
const second = coordinator.run({ clientId: 1, key: 'workspace-b', start: scanB.start })
|
||||
|
||||
expect(scan.starts).toHaveLength(2)
|
||||
expect(scan.starts[0].aborted).toBe(true)
|
||||
await expect(first).rejects.toThrow(LIST_FILES_SUPERSEDED_MESSAGE)
|
||||
await first.catch((err) => expect(isFileListingCancellation(err)).toBe(true))
|
||||
// Each caller gets its own live scan; neither aborts the other.
|
||||
expect(scanA.starts).toHaveLength(1)
|
||||
expect(scanB.starts).toHaveLength(1)
|
||||
expect(scanA.starts[0].aborted).toBe(false)
|
||||
expect(scanB.starts[0].aborted).toBe(false)
|
||||
|
||||
expect(scan.starts[1].aborted).toBe(false)
|
||||
scan.finish(['b.ts'])
|
||||
scanA.finish(['a.ts'])
|
||||
scanB.finish(['b.ts'])
|
||||
await expect(first).resolves.toEqual(['a.ts'])
|
||||
await expect(second).resolves.toEqual(['b.ts'])
|
||||
})
|
||||
|
||||
|
||||
@@ -4,21 +4,21 @@
|
||||
* Why: rapid workspace switching used to stack N concurrent full-tree scans
|
||||
* on the single-threaded relay and its one SSH channel, starving small
|
||||
* interactive fs.readDir/fs.stat requests past their 30s timeout. This
|
||||
* coordinator guarantees at most one scan in flight per client:
|
||||
* coordinator guarantees at most one scan in flight per (client, scan key):
|
||||
* - a request for the same root/excludes joins the in-flight scan
|
||||
* (Quick Open + file-explorer filter share one scan),
|
||||
* - a request for a different root supersedes it — the old scan is aborted
|
||||
* and its request fails fast instead of running to completion (this also
|
||||
* protects against older Orca clients that never send rpc.cancel),
|
||||
* - a request for a DIFFERENT root/excludes runs as its own concurrent
|
||||
* single-flight scan. Why #7769: two independent callers on one SSH
|
||||
* connection — e.g. the editor's markdown-document scan (no excludes) and
|
||||
* Quick Open (nested-worktree excludes) — share one relay clientId. Keying
|
||||
* only by clientId let a newer request supersede an older unrelated one,
|
||||
* surfacing a spurious "superseded"/empty-list failure to a caller that
|
||||
* never cancelled. Abandoned scans are still stopped via their requester's
|
||||
* rpc.cancel (see attach below), not by evicting a sibling caller,
|
||||
* - when every joined requester cancels (rpc.cancel / client detach), the
|
||||
* scan is aborted so abandoned work stops immediately.
|
||||
*/
|
||||
import {
|
||||
FileListingCancelledError,
|
||||
fileListingCancellationError
|
||||
} from '../shared/file-listing-cancellation'
|
||||
|
||||
export const LIST_FILES_SUPERSEDED_MESSAGE = 'File listing superseded by a newer request'
|
||||
import { fileListingCancellationError } from '../shared/file-listing-cancellation'
|
||||
|
||||
type ScanEntry = {
|
||||
key: string
|
||||
@@ -28,7 +28,7 @@ type ScanEntry = {
|
||||
}
|
||||
|
||||
export class ListFilesScanCoordinator {
|
||||
private readonly scansByClient = new Map<number, ScanEntry>()
|
||||
private readonly scansByClient = new Map<number, Map<string, ScanEntry>>()
|
||||
|
||||
run(opts: {
|
||||
clientId: number
|
||||
@@ -41,12 +41,14 @@ export class ListFilesScanCoordinator {
|
||||
return Promise.reject(fileListingCancellationError(signal))
|
||||
}
|
||||
|
||||
const existing = this.scansByClient.get(clientId)
|
||||
if (existing && existing.key === key && !existing.controller.signal.aborted) {
|
||||
let byKey = this.scansByClient.get(clientId)
|
||||
const existing = byKey?.get(key)
|
||||
if (existing && !existing.controller.signal.aborted) {
|
||||
return this.attach(existing, signal)
|
||||
}
|
||||
if (existing) {
|
||||
existing.controller.abort(new FileListingCancelledError(LIST_FILES_SUPERSEDED_MESSAGE))
|
||||
if (!byKey) {
|
||||
byKey = new Map<string, ScanEntry>()
|
||||
this.scansByClient.set(clientId, byKey)
|
||||
}
|
||||
|
||||
const controller = new AbortController()
|
||||
@@ -56,10 +58,14 @@ export class ListFilesScanCoordinator {
|
||||
promise: Promise.resolve([]),
|
||||
attachedCount: 0
|
||||
}
|
||||
this.scansByClient.set(clientId, entry)
|
||||
byKey.set(key, entry)
|
||||
entry.promise = start(controller.signal).finally(() => {
|
||||
if (this.scansByClient.get(clientId) === entry) {
|
||||
this.scansByClient.delete(clientId)
|
||||
const scans = this.scansByClient.get(clientId)
|
||||
if (scans?.get(key) === entry) {
|
||||
scans.delete(key)
|
||||
if (scans.size === 0) {
|
||||
this.scansByClient.delete(clientId)
|
||||
}
|
||||
}
|
||||
})
|
||||
return this.attach(entry, signal)
|
||||
|
||||
Reference in New Issue
Block a user