mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 00:03:45 +00:00
fix(worktrees): keep the shared snapshot out of provider internals
The snapshot proxy passed itself as the Reflect.get receiver, so prototype methods invoked through it ran with `this` bound to the proxy. A provider whose own shutdown() re-read state via `this.listProcesses()` would then silently get this sweep's cached snapshot instead of the live host — batching leaking past the calls it was built for. Bind non-listProcesses members to the target so only the sweep's own calls share the snapshot. No shipped provider does this today; the point is that adding one must not quietly change teardown semantics. Raised by Greptile as an undocumented implicit constraint; closed structurally rather than by comment. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -154,6 +154,37 @@ describe('stopMissingWorktreeTerminals', () => {
|
||||
expect(provider.shutdownCalls).toHaveLength(ids.length)
|
||||
})
|
||||
|
||||
// Why: the batching must not leak past the calls it was built for. If the proxy
|
||||
// were the receiver, a provider whose own method called `this.listProcesses()`
|
||||
// would silently read this sweep's cached snapshot instead of the live host.
|
||||
it('does not serve the shared snapshot to provider-internal listProcesses', async () => {
|
||||
class SelfListingProvider {
|
||||
listCalls = 0
|
||||
constructor(private readonly sessions: { id: string; cwd: string; title: string }[]) {}
|
||||
async listProcesses(): Promise<{ id: string; cwd: string; title: string }[]> {
|
||||
this.listCalls += 1
|
||||
return this.sessions
|
||||
}
|
||||
async shutdown(): Promise<void> {
|
||||
// A provider that re-reads live state as part of stopping.
|
||||
await this.listProcesses()
|
||||
}
|
||||
}
|
||||
const ids = ['repo-1::/workspace/a', 'repo-1::/workspace/b']
|
||||
const provider = new SelfListingProvider(
|
||||
ids.map((id) => ({ id: `${id}@@session`, cwd: '/workspace', title: 'shell' }))
|
||||
)
|
||||
|
||||
await stopMissingWorktreeTerminals({ ...localRepo, connectionId: 'ssh-1' }, ids, [], {
|
||||
runtime: createRuntime(),
|
||||
getLocalProvider: () => null,
|
||||
getSshProvider: () => provider as unknown as IPtyProvider
|
||||
})
|
||||
|
||||
// One shared sweep scan, plus each shutdown's own live re-read.
|
||||
expect(provider.listCalls).toBe(1 + ids.length)
|
||||
})
|
||||
|
||||
// Why: caching a rejected scan would let one transient failure suppress
|
||||
// teardown for every remaining worktree in the sweep.
|
||||
it('does not reuse a failed process scan', async () => {
|
||||
|
||||
@@ -18,9 +18,14 @@ const MISSING_WORKTREE_TEARDOWN_CONCURRENCY = 4
|
||||
function withSharedProcessSnapshot(provider: IPtyProvider): IPtyProvider {
|
||||
let snapshot: Promise<Awaited<ReturnType<IPtyProvider['listProcesses']>>> | null = null
|
||||
return new Proxy(provider, {
|
||||
get(target, property, receiver) {
|
||||
get(target, property) {
|
||||
if (property !== 'listProcesses') {
|
||||
return Reflect.get(target, property, receiver)
|
||||
// Why: bind other members to `target`, not the proxy. With the proxy as
|
||||
// receiver, a provider whose own method called `this.listProcesses()`
|
||||
// would silently read this sweep's cached snapshot instead of the live
|
||||
// host — the batching must not leak past the calls it was built for.
|
||||
const member: unknown = Reflect.get(target, property)
|
||||
return typeof member === 'function' ? member.bind(target) : member
|
||||
}
|
||||
return async (opts?: { deadlineMs?: number }) => {
|
||||
const pending = (snapshot ??= target.listProcesses(opts))
|
||||
|
||||
Reference in New Issue
Block a user