mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 00:03:20 +00:00
fix(review): consolidate SSH ownership fence and add fail-closed observability
- move the SSH status==='connected' liveness gate and canonical host-id derivation into the shared buildFileMutationOwnership so web fails closed on a reconnecting connection the same way mobile does; reduce buildMobileFileMutationOwnership to a thin delegate (removes the undocumented override/divergence) [#1] - log a cause tag at every mobile fence rejection via runtimeChangedError() so a repeated 'Orca server changed' toast is diagnosable, mirroring the transport '[net]' convention [#2] - add tests for the post-create files.open failure, replacement-runtime reply, and transport-cutover-before-open branches [#3] - log ownership route mismatches in the server gate (assertRuntimeFileMutationExpectation), matching the file's other logged failure paths [#4] - add a web regression test for the non-connected SSH fail-closed path Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -3,8 +3,6 @@ import {
|
||||
assertFileMutationOwnershipCapability,
|
||||
buildFileMutationOwnership,
|
||||
FILE_MUTATION_OWNER_UNVERIFIED_MESSAGE,
|
||||
FILE_MUTATION_RUNTIME_UNVERIFIED_MESSAGE,
|
||||
FILE_MUTATION_SSH_UNVERIFIED_MESSAGE,
|
||||
type FileMutationOwnership
|
||||
} from '../../../src/shared/file-mutation-ownership'
|
||||
import { inferFolderWorkspacePathConnection } from '../../../src/shared/folder-workspace-path-connection'
|
||||
@@ -40,6 +38,13 @@ type MutationTargetHost = {
|
||||
runtimeIds: string[]
|
||||
}
|
||||
|
||||
// Why: all fence rejections share one generic user message, so log the distinguishing cause
|
||||
// (mirroring the transport '[net]' convention) to keep a repeated failure diagnosable.
|
||||
function runtimeChangedError(cause: string, detail?: Record<string, unknown>): Error {
|
||||
console.warn('[file-mutation-fence] runtime changed', { cause, ...detail })
|
||||
return new Error(RUNTIME_CHANGED_MESSAGE)
|
||||
}
|
||||
|
||||
export function getMobileFileMutationFailureMessage(failure: RpcFailure): string {
|
||||
if (
|
||||
failure.error.code === 'selector_not_found' ||
|
||||
@@ -54,17 +59,9 @@ export function buildMobileFileMutationOwnership(
|
||||
worktreeHostId: string | null | undefined,
|
||||
sshState: SshConnectionState | null = null
|
||||
): MobileFileMutationOwnership {
|
||||
const host = parseExecutionHostId(worktreeHostId)
|
||||
if (host?.kind === 'runtime') {
|
||||
throw new Error(FILE_MUTATION_RUNTIME_UNVERIFIED_MESSAGE)
|
||||
}
|
||||
if (host?.kind === 'ssh' && sshState?.status !== 'connected') {
|
||||
throw new Error(FILE_MUTATION_SSH_UNVERIFIED_MESSAGE)
|
||||
}
|
||||
const ownership = buildFileMutationOwnership(worktreeHostId, sshState)
|
||||
return host?.kind === 'ssh'
|
||||
? { ...ownership, expectedExecutionHostId: toSshExecutionHostId(host.targetId) }
|
||||
: ownership
|
||||
// Ownership validation (runtime rejection, SSH liveness fence, canonical host id) lives in the
|
||||
// shared builder so mobile and web stay identical.
|
||||
return buildFileMutationOwnership(worktreeHostId, sshState)
|
||||
}
|
||||
|
||||
export async function captureMobileFileMutationOwnership(
|
||||
@@ -82,7 +79,7 @@ export async function captureMobileFileMutationOwnership(
|
||||
throw normalizePreflightError(error)
|
||||
}
|
||||
}
|
||||
throw new Error(RUNTIME_CHANGED_MESSAGE)
|
||||
throw runtimeChangedError('preflight-retry-exhausted')
|
||||
}
|
||||
|
||||
export function assertMobileFileMutationFenceCurrent(
|
||||
@@ -95,7 +92,10 @@ export function assertMobileFileMutationFenceCurrent(
|
||||
currentGeneration !== null &&
|
||||
currentGeneration !== fence.transportGeneration
|
||||
) {
|
||||
throw new Error(RUNTIME_CHANGED_MESSAGE)
|
||||
throw runtimeChangedError('transport-generation-changed', {
|
||||
expected: fence.transportGeneration,
|
||||
current: currentGeneration
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -103,8 +103,12 @@ export function assertMobileFileMutationResponseRuntime(
|
||||
fence: MobileFileMutationFence,
|
||||
response: RpcSuccess | RpcFailure
|
||||
): void {
|
||||
if (getResponseRuntimeId(response) !== fence.runtimeId) {
|
||||
throw new Error(RUNTIME_CHANGED_MESSAGE)
|
||||
const runtimeId = getResponseRuntimeId(response)
|
||||
if (runtimeId !== fence.runtimeId) {
|
||||
throw runtimeChangedError('mutation-runtime-mismatch', {
|
||||
expected: fence.runtimeId,
|
||||
actual: runtimeId
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -242,7 +246,7 @@ async function requestResult<TResult>(
|
||||
function getResponseRuntimeId(response: RpcSuccess | RpcFailure): string {
|
||||
const runtimeId = response._meta?.runtimeId
|
||||
if (typeof runtimeId !== 'string' || !runtimeId.trim()) {
|
||||
throw new Error(RUNTIME_CHANGED_MESSAGE)
|
||||
throw runtimeChangedError('missing-response-runtime-id')
|
||||
}
|
||||
return runtimeId.trim()
|
||||
}
|
||||
@@ -268,7 +272,10 @@ function assertCaptureTransportCurrent(
|
||||
|
||||
function assertSameRuntime(expectedRuntimeId: string, actualRuntimeId: string): void {
|
||||
if (actualRuntimeId !== expectedRuntimeId) {
|
||||
throw new Error(RUNTIME_CHANGED_MESSAGE)
|
||||
throw runtimeChangedError('capture-runtime-mismatch', {
|
||||
expected: expectedRuntimeId,
|
||||
actual: actualRuntimeId
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -292,7 +299,7 @@ function normalizePreflightError(error: unknown): Error {
|
||||
return new Error(PREFLIGHT_TIMEOUT_MESSAGE)
|
||||
}
|
||||
if (isLogicalClientCutoverError(error)) {
|
||||
return new Error(RUNTIME_CHANGED_MESSAGE)
|
||||
return runtimeChangedError('logical-cutover')
|
||||
}
|
||||
return error instanceof Error ? error : new Error(String(error))
|
||||
}
|
||||
|
||||
@@ -66,6 +66,73 @@ describe('mobile markdown note creation fence', () => {
|
||||
)
|
||||
})
|
||||
|
||||
it('surfaces a files.open failure after a successful create', async () => {
|
||||
const sendRequest = vi.fn(async (method: string) => {
|
||||
if (method === 'status.get') {
|
||||
return statusResponse()
|
||||
}
|
||||
if (method === 'worktree.show') {
|
||||
return success({ worktree: { hostId: 'local' } })
|
||||
}
|
||||
if (method === 'files.createFile') {
|
||||
return success({ ok: true })
|
||||
}
|
||||
if (method === 'files.open') {
|
||||
return failure('Cannot open file')
|
||||
}
|
||||
throw new Error(`Unexpected RPC request: ${method}`)
|
||||
})
|
||||
|
||||
await expect(createAndOpenMobileMarkdownNote({ sendRequest }, 'id:worktree-1')).rejects.toThrow(
|
||||
'Cannot open file'
|
||||
)
|
||||
})
|
||||
|
||||
it('rejects a files.open reply from a replacement runtime', async () => {
|
||||
const sendRequest = vi.fn(async (method: string) => {
|
||||
if (method === 'status.get') {
|
||||
return statusResponse()
|
||||
}
|
||||
if (method === 'worktree.show') {
|
||||
return success({ worktree: { hostId: 'local' } })
|
||||
}
|
||||
if (method === 'files.createFile') {
|
||||
return success({ ok: true })
|
||||
}
|
||||
if (method === 'files.open') {
|
||||
return success({ opened: true }, 'runtime-new')
|
||||
}
|
||||
throw new Error(`Unexpected RPC request: ${method}`)
|
||||
})
|
||||
|
||||
await expect(createAndOpenMobileMarkdownNote({ sendRequest }, 'id:worktree-1')).rejects.toThrow(
|
||||
'Orca server changed'
|
||||
)
|
||||
})
|
||||
|
||||
it('stops before opening when the transport cuts over after a successful create', async () => {
|
||||
let transportGeneration = 1
|
||||
const sendRequest = vi.fn(async (method: string) => {
|
||||
if (method === 'status.get') {
|
||||
return statusResponse()
|
||||
}
|
||||
if (method === 'worktree.show') {
|
||||
return success({ worktree: { hostId: 'local' } })
|
||||
}
|
||||
if (method === 'files.createFile') {
|
||||
transportGeneration = 2
|
||||
return success({ ok: true })
|
||||
}
|
||||
throw new Error(`Unexpected RPC request: ${method}`)
|
||||
})
|
||||
const client = { sendRequest, getGeneration: () => transportGeneration }
|
||||
|
||||
await expect(createAndOpenMobileMarkdownNote(client, 'id:worktree-1')).rejects.toThrow(
|
||||
'Orca server changed'
|
||||
)
|
||||
expect(sendRequest.mock.calls.some(([method]) => method === 'files.open')).toBe(false)
|
||||
})
|
||||
|
||||
it('keeps one SSH ownership lease across name-collision retries', async () => {
|
||||
let createAttempts = 0
|
||||
const sendRequest = vi.fn(async (method: string) => {
|
||||
|
||||
@@ -129,6 +129,14 @@ function assertRuntimeFileMutationExpectation(
|
||||
!declaredMatchesRoute ||
|
||||
!executionHostMatchesFileRoute(expectedExecutionHostId, connectionId)
|
||||
) {
|
||||
// Why: this is the strict ownership gate; leave an audit trail for stale clients, reconnect
|
||||
// races, and any spoofed-ownership attempt, matching this file's other logged failure paths.
|
||||
console.warn('[runtime-files.mutation] ownership route mismatch', {
|
||||
worktreeHostId,
|
||||
connectionId,
|
||||
expectedExecutionHostId,
|
||||
declaredMatchesRoute
|
||||
})
|
||||
throw new Error('Workspace host changed; refresh and try again')
|
||||
}
|
||||
assertSshMutationExpectation(connectionId, expectedSshTargetId, expectedSshConnectionGeneration)
|
||||
|
||||
@@ -231,6 +231,21 @@ describe('paired web file mutation methods', () => {
|
||||
expect(callRuntimeResult).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('fails closed before mutation when the SSH connection is not connected', async () => {
|
||||
// Why: a reconnecting connection can still report a stale-but-defined generation; the shared
|
||||
// builder must fence it the same way for web as it does for mobile.
|
||||
getSshState.mockResolvedValueOnce({
|
||||
...connectedSshState('hub-private-target', 17),
|
||||
status: 'reconnecting'
|
||||
})
|
||||
const methods = createWebFileMutationMethods({ captureSession })
|
||||
|
||||
await expect(
|
||||
methods.writeFile({ filePath: '/ssh/repo/source.md', content: 'unsafe' })
|
||||
).rejects.toThrow(FILE_MUTATION_SSH_UNVERIFIED_MESSAGE)
|
||||
expect(callRuntimeResult).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it.each([
|
||||
['missing', undefined],
|
||||
['invalid', 'runtime:' as Worktree['hostId']]
|
||||
|
||||
@@ -2,7 +2,7 @@ import {
|
||||
FILE_MUTATION_OWNERSHIP_RUNTIME_CAPABILITY,
|
||||
FILE_MUTATION_OWNERSHIP_UPDATE_REQUIRED_MESSAGE
|
||||
} from './protocol-version'
|
||||
import { parseExecutionHostId } from './execution-host'
|
||||
import { parseExecutionHostId, toSshExecutionHostId } from './execution-host'
|
||||
import type { RuntimeStatus } from './runtime-types'
|
||||
import type { SshConnectionState, SshMutationExpectation } from './ssh-types'
|
||||
|
||||
@@ -39,11 +39,17 @@ export function buildFileMutationOwnership(
|
||||
if (host.kind === 'runtime') {
|
||||
throw new Error(FILE_MUTATION_RUNTIME_UNVERIFIED_MESSAGE)
|
||||
}
|
||||
if (sshState?.targetId !== host.targetId || sshState.connectionGeneration === undefined) {
|
||||
// Why: every client (mobile + web) must fence SSH mutations to a live, matching connection and
|
||||
// a canonical host id — a reconnecting state with a stale-but-defined generation fails closed.
|
||||
if (
|
||||
sshState?.status !== 'connected' ||
|
||||
sshState.targetId !== host.targetId ||
|
||||
sshState.connectionGeneration === undefined
|
||||
) {
|
||||
throw new Error(FILE_MUTATION_SSH_UNVERIFIED_MESSAGE)
|
||||
}
|
||||
return {
|
||||
expectedExecutionHostId: host.id,
|
||||
expectedExecutionHostId: toSshExecutionHostId(host.targetId),
|
||||
expectedSshTargetId: host.targetId,
|
||||
expectedSshConnectionGeneration: sshState.connectionGeneration
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user