diff --git a/mobile/src/tasks/worktree-create-retry.test.ts b/mobile/src/tasks/worktree-create-retry.test.ts index a8b0d283835..c7868ff2e91 100644 --- a/mobile/src/tasks/worktree-create-retry.test.ts +++ b/mobile/src/tasks/worktree-create-retry.test.ts @@ -3,7 +3,18 @@ import type { RpcClient } from '../transport/rpc-client' import { markRpcDeliveryUnknown } from '../transport/rpc-delivery-ambiguity' import { LogicalClientCutoverError } from '../transport/stable-logical-rpc-client' import type { ConnectionState } from '../transport/types' -import { createWorktreeWithNameRetry } from './worktree-create-retry' +import { WORKTREE_CREATE_DEDUPE_TTL_MS } from '../../../src/shared/new-workspace/worktree-create-retry-policy' +import { + LIVENESS_IDLE_MS, + LIVENESS_PROBE_TIMEOUT_MS, + MISSED_PROBE_LIMIT +} from '../transport/rpc-session-liveness-watchdog' +import { + createWorktreeWithNameRetry, + WORKTREE_CREATE_AMBIGUOUS_RECONNECT_WAIT_MS, + WORKTREE_CREATE_AMBIGUOUS_REPLAY_WINDOW_MS +} from './worktree-create-retry' +import { WORKTREE_CREATE_TIMEOUT_MS } from './workspace-create-timeout' type Attempt = { method: string; params: Record } @@ -41,7 +52,12 @@ async function flush(): Promise { // cutover). Records every call so tests can assert on the clientMutationId. function scriptedClient( outcomes: Array< - { id: string } | { errorMessage: string } | { throws: unknown; dropsConnection?: boolean } + | { id: string } + | { errorMessage: string } + // takesMs models how long the ambiguity took to SURFACE — a clean close is + // instant, a half-open socket waits out the liveness watchdog or the timeout. + // reconnectsAfterMs brings the transport back so the next replay can run. + | { throws: unknown; dropsConnection?: boolean; takesMs?: number; reconnectsAfterMs?: number } >, attempts: Attempt[], connection?: ReturnType @@ -56,8 +72,14 @@ function scriptedClient( const outcome = outcomes[Math.min(call, outcomes.length - 1)]! call += 1 if ('throws' in outcome) { + if (outcome.takesMs !== undefined) { + await new Promise((resolve) => setTimeout(resolve, outcome.takesMs)) + } if (outcome.dropsConnection) { connection?.set('reconnecting') + if (outcome.reconnectsAfterMs !== undefined) { + setTimeout(() => connection?.set('connected'), outcome.reconnectsAfterMs) + } } throw outcome.throws } @@ -157,6 +179,8 @@ describe('createWorktreeWithNameRetry', () => { expect(attempts).toHaveLength(6) }) + // Also the delivery-ambiguity guard: an unmarked error is a DEFINITE failure, so + // neither the cutover nor the replay path may re-issue it. it('does not treat an ordinary transport error as a cutover', async () => { const attempts: Attempt[] = [] const client = scriptedClient([{ throws: new Error('Request timed out') }], attempts) @@ -284,24 +308,46 @@ describe('createWorktreeWithNameRetry', () => { expect(attempts[0]!.params.clientMutationId).toBeUndefined() }) - it('gives up after the delivery-ambiguity replay budget and rethrows', async () => { - const attempts: Attempt[] = [] - const client = scriptedClient( - [{ throws: markRpcDeliveryUnknown(new Error('Connection interrupted')) }], - attempts - ) - await expect( - createWorktreeWithNameRetry({ + it('gives up after the delivery-ambiguity replay budget, still inside the dedupe TTL', async () => { + vi.useFakeTimers() + try { + const attempts: Attempt[] = [] + const connection = connectionController() + const client = scriptedClient( + [ + { + throws: markRpcDeliveryUnknown(new Error('Connection interrupted')), + dropsConnection: true, + reconnectsAfterMs: 1_000 + } + ], + attempts, + connection + ) + const startedAt = Date.now() + let settledAt = -1 + const pending = createWorktreeWithNameRetry({ client, baseName: 'barnacle', buildParams: (name) => ({ repo: 'id:r', name }), supportsIdempotentCutoverRetry: true, mintMutationId: () => 'key-budget' + }).catch((error: unknown) => { + settledAt = Date.now() + throw error }) - ).rejects.toThrow('Connection interrupted') - // Initial attempt + 2 replays; the window stays inside the host's post-success - // dedupe TTL so a replay can't outlive the record it is meant to match. - expect(attempts).toHaveLength(3) + const settled = expect(pending).rejects.toThrow('Connection interrupted') + await vi.advanceTimersByTimeAsync(WORKTREE_CREATE_DEDUPE_TTL_MS) + await settled + // Initial attempt + 2 replays. + expect(attempts).toHaveLength(3) + // The budget is a count, but what keeps a replay reconciling instead of building + // a second worktree is wall clock: every attempt has to land while the host still + // holds the record, with the watchdog's detection latency already deducted. + expect(settledAt - startedAt).toBeLessThanOrEqual(WORKTREE_CREATE_AMBIGUOUS_REPLAY_WINDOW_MS) + } finally { + vi.useRealTimers() + } }) it('surfaces the original ambiguity when the transport never comes back', async () => { @@ -336,4 +382,103 @@ describe('createWorktreeWithNameRetry', () => { vi.useRealTimers() } }) + + it('does not replay an ambiguity that surfaced while the transport stayed connected', async () => { + vi.useFakeTimers() + try { + const attempts: Attempt[] = [] + const connection = connectionController() + const client = scriptedClient( + [ + { + // A silently dropped response frame leaves the socket alive, so nothing + // rejects until the request timeout — by then the host resolved at an + // unknowable point and its dedupe record is very likely gone. + throws: markRpcDeliveryUnknown(new Error('Request timed out: worktree.create')), + takesMs: WORKTREE_CREATE_TIMEOUT_MS + }, + { id: 'wt-duplicate' } + ], + attempts, + connection + ) + + const pending = createWorktreeWithNameRetry({ + client, + baseName: 'cuttlefish', + buildParams: (name) => ({ repo: 'id:r', name }), + supportsIdempotentCutoverRetry: true, + mintMutationId: () => 'key-expired' + }) + const settled = expect(pending).rejects.toThrow('Request timed out') + await vi.advanceTimersByTimeAsync(WORKTREE_CREATE_TIMEOUT_MS) + await settled + // Never left 'connected', so no drop was ever detected and the staleness of the + // ambiguity is unbounded. Re-sending would miss the record and build a SECOND + // worktree — for a folder workspace, one with the very same name. + expect(connection.getState()).toBe('connected') + expect(attempts).toHaveLength(1) + } finally { + vi.useRealTimers() + } + }) + + it('clamps a later reconnect wait to what is left of the replay window', async () => { + vi.useFakeTimers() + try { + const attempts: Attempt[] = [] + const connection = connectionController() + const spent = 5_000 + const client = scriptedClient( + [ + { + throws: markRpcDeliveryUnknown(new Error('Connection interrupted')), + dropsConnection: true, + reconnectsAfterMs: spent + }, + { + // Second drop, and this time the phone never comes back. + throws: markRpcDeliveryUnknown(new Error('Connection interrupted')), + dropsConnection: true + } + ], + attempts, + connection + ) + + const startedAt = Date.now() + let settledAt = -1 + const pending = createWorktreeWithNameRetry({ + client, + baseName: 'nautilus', + buildParams: (name) => ({ repo: 'id:r', name }), + supportsIdempotentCutoverRetry: true, + mintMutationId: () => 'key-clamped' + }).catch((error: unknown) => { + settledAt = Date.now() + throw error + }) + const settled = expect(pending).rejects.toThrow('Connection interrupted') + await vi.advanceTimersByTimeAsync(WORKTREE_CREATE_AMBIGUOUS_REPLAY_WINDOW_MS) + await settled + expect(attempts).toHaveLength(2) + // The window is anchored at the FIRST ambiguity, so the second wait gets only the + // remainder — it must not restart a full wait and carry the replay past the record. + expect(settledAt - startedAt).toBe(WORKTREE_CREATE_AMBIGUOUS_REPLAY_WINDOW_MS) + } finally { + vi.useRealTimers() + } + }) + + it('leaves room for watchdog detection inside the host dedupe TTL', () => { + // Restates the budget from the watchdog's side: a half-open socket is only reported + // after a full idle period plus the missed-probe budget, and the wait runs *after* + // that. Raising the wait without re-deriving it against the TTL would let a replay + // land on a dropped record. + const worstCaseDetectionMs = LIVENESS_IDLE_MS + MISSED_PROBE_LIMIT * LIVENESS_PROBE_TIMEOUT_MS + expect(worstCaseDetectionMs + WORKTREE_CREATE_AMBIGUOUS_RECONNECT_WAIT_MS).toBeLessThanOrEqual( + WORKTREE_CREATE_DEDUPE_TTL_MS + ) + expect(WORKTREE_CREATE_AMBIGUOUS_REPLAY_WINDOW_MS).toBeGreaterThan(0) + }) }) diff --git a/mobile/src/tasks/worktree-create-retry.ts b/mobile/src/tasks/worktree-create-retry.ts index b4d0693fc0a..f612032bc1e 100644 --- a/mobile/src/tasks/worktree-create-retry.ts +++ b/mobile/src/tasks/worktree-create-retry.ts @@ -7,8 +7,14 @@ import { CLIENT_WORKTREE_CREATE_MAX_ATTEMPTS, getClientWorktreeCreateCandidate, getGeneratedWorktreeCreateRetryCandidate, - isRetryableWorktreeCreateConflict + isRetryableWorktreeCreateConflict, + WORKTREE_CREATE_DEDUPE_TTL_MS } from '../../../src/shared/new-workspace/worktree-create-retry-policy' +import { + LIVENESS_IDLE_MS, + LIVENESS_PROBE_TIMEOUT_MS, + MISSED_PROBE_LIMIT +} from '../transport/rpc-session-liveness-watchdog' import { WORKTREE_CREATE_TIMEOUT_MS } from './workspace-create-timeout' // Why: server-side collision checks (branch already exists locally / on a remote @@ -34,10 +40,23 @@ const WORKTREE_CREATE_CUTOVER_MAX_RETRIES = 5 // to be caught by one. Surfacing that as a failure is wrong: the host may well have // finished the worktree. Replay on the same clientMutationId instead. const WORKTREE_CREATE_AMBIGUOUS_MAX_RETRIES = 2 -// Why: the host drops a completed create's dedupe record 60s after it resolves, so -// keep the whole replay window comfortably inside that or a replay lands as a fresh -// create and suffixes a duplicate. An in-flight create is deduped without any TTL. -const WORKTREE_CREATE_AMBIGUOUS_RECONNECT_WAIT_MS = 20_000 + +// Why: the host keeps a settled create's dedupe record for WORKTREE_CREATE_DEDUPE_TTL_MS +// after it resolves; a replay that lands later misses it and the host's suffix loop +// builds a SECOND worktree — and for folder workspaces a second one with the SAME name +// and no collision check at all. So what bounds a safe replay is not how long the create +// ran, but how STALE the client's knowledge is: the worst case between the host losing a +// response and the client noticing. A dead socket is detected by the liveness watchdog +// within one idle period plus its missed-probe budget, so that is the staleness ceiling +// for any drop the transport actually reports. +const WORKTREE_CREATE_AMBIGUITY_DETECTION_CEILING_MS = + LIVENESS_IDLE_MS + MISSED_PROBE_LIMIT * LIVENESS_PROBE_TIMEOUT_MS +// What is left of the host's record once that detection latency is spent. The reconnect +// wait plus any further replay has to fit inside it. +export const WORKTREE_CREATE_AMBIGUOUS_REPLAY_WINDOW_MS = + WORKTREE_CREATE_DEDUPE_TTL_MS - WORKTREE_CREATE_AMBIGUITY_DETECTION_CEILING_MS +export const WORKTREE_CREATE_AMBIGUOUS_RECONNECT_WAIT_MS = + WORKTREE_CREATE_AMBIGUOUS_REPLAY_WINDOW_MS export type CreateWorktreeWithNameRetryArgs = { client: RpcClient @@ -105,6 +124,9 @@ async function sendWorktreeCreateResilient( ): Promise { let migrationRetry = 0 let ambiguousRetry = 0 + // Anchored at the FIRST ambiguity, not at the send: a create legitimately runs for + // minutes, and the host's record only starts ticking once it resolves. + let replayDeadlineAt: number | null = null for (;;) { try { return await client.sendRequest('worktree.create', params, { @@ -126,12 +148,32 @@ async function sendWorktreeCreateResilient( if (!isRpcDeliveryUnknown(error) || ambiguousRetry >= WORKTREE_CREATE_AMBIGUOUS_MAX_RETRIES) { throw error } + // Why: every transport path that reports a *drop* leaves 'connected' before the + // rejection reaches us (rpc-client.ts:675/695/1213 set state first or reject via + // queueMicrotask; the relay's fail() publishes synchronously). So still being + // 'connected' here means the socket was healthy the whole time and only the + // response went missing — the request-timeout path, which surfaces after + // WORKTREE_CREATE_TIMEOUT_MS. That says nothing about when the host actually + // resolved, so the dedupe record may be long gone and a replay would build a + // second worktree instead of reconciling. Fail the create instead. + if (client.getState() === 'connected') { + throw error + } + replayDeadlineAt ??= Date.now() + WORKTREE_CREATE_AMBIGUOUS_REPLAY_WINDOW_MS + const remainingWindowMs = replayDeadlineAt - Date.now() + if (remainingWindowMs <= 0) { + throw error + } ambiguousRetry += 1 // Why: unlike a cutover, no replacement session exists yet — resending now // would just hit the dead one, so wait for the transport to come back and - // surface the original ambiguity if it does not. + // surface the original ambiguity if it does not. Clamped to the window so the + // wait itself cannot carry the replay past the host's record. if ( - !(await waitForRpcClientReconnected(client, WORKTREE_CREATE_AMBIGUOUS_RECONNECT_WAIT_MS)) + !(await waitForRpcClientReconnected( + client, + Math.min(WORKTREE_CREATE_AMBIGUOUS_RECONNECT_WAIT_MS, remainingWindowMs) + )) ) { throw error } diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 83d3c138b97..8e2dad90671 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -1077,6 +1077,7 @@ import { isGeneratedWorktreeCreateName, WORKTREE_CREATE_MAX_SUFFIX_ATTEMPTS } from '../worktree-create-candidates' +import { WORKTREE_CREATE_DEDUPE_TTL_MS } from '../../shared/new-workspace/worktree-create-retry-policy' import { failedWorktreeCreationNeedsRetirement, getRetiredNameRegistryForRepo, @@ -2015,8 +2016,9 @@ function getAgentLaunchPlatformForRepo( const MOBILE_TERMINAL_CREATE_RESULT_TTL_MS = 60_000 // Why: same idempotency window for worktree.create — a phone whose create was // interrupted by a connection migration retries with the same clientMutationId -// and reuses the just-created worktree instead of spawning a duplicate. -const WORKTREE_CREATE_RESULT_TTL_MS = 60_000 +// and reuses the just-created worktree instead of spawning a duplicate. Shared with +// the mobile client so its replay budget is sized against the real window. +const WORKTREE_CREATE_RESULT_TTL_MS = WORKTREE_CREATE_DEDUPE_TTL_MS const FOREGROUND_AGENT_WRAPPER_RETRY_INTERVAL_MS = 150 const FOREGROUND_AGENT_WRAPPER_RETRY_TIMEOUT_MS = 6_500 const BRACKETED_PASTE_BEGIN = '\x1b[200~' diff --git a/src/shared/new-workspace/worktree-create-retry-policy.ts b/src/shared/new-workspace/worktree-create-retry-policy.ts index e45da0d5530..2d3438b0218 100644 --- a/src/shared/new-workspace/worktree-create-retry-policy.ts +++ b/src/shared/new-workspace/worktree-create-retry-policy.ts @@ -65,3 +65,10 @@ export function getGeneratedWorktreeCreateRetryCandidate( export function isRetryableWorktreeCreateConflict(message: string): boolean { return RETRYABLE_WORKTREE_CREATE_CONFLICT_PATTERNS.some((pattern) => pattern.test(message)) } + +// Why: the host drops a *settled* worktree.create dedupe record this long after it +// resolves, so a client replaying an idempotent create must land inside the window +// or the replay misses the record and the host's suffix loop builds a second +// worktree. Shared so the client's replay budget can be asserted against it instead +// of assuming a value that lives in another process. +export const WORKTREE_CREATE_DEDUPE_TTL_MS = 60_000