Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-scene-drops.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@viamrobotics/motion-tools': patch
---

fix: stream auto-reconnect for draw service and preserve scene entities during disconnect
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@
"@typescript-eslint/parser": "8.56.1",
"@viamrobotics/prime-core": "0.1.5",
"@viamrobotics/sdk": "0.69.0",
"@viamrobotics/svelte-sdk": "1.2.2",
"@viamrobotics/svelte-sdk": "1.2.3",
"@viamrobotics/tweakpane-config": "0.1.1",
"@vitest/browser-playwright": "4.1.9",
"@vitest/coverage-v8": "4.1.9",
Expand Down
39 changes: 27 additions & 12 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

196 changes: 196 additions & 0 deletions src/lib/__tests__/retry-stream.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
import { describe, expect, it, vi } from 'vitest'

import { retryStream } from '../retry-stream'

describe('retryStream', () => {
it('calls run and resolves when run succeeds', async () => {
const run = vi.fn().mockResolvedValue(undefined)
const controller = new AbortController()

// run resolves once, retryStream will call it again — abort after first call
run.mockImplementation(async () => {
controller.abort()
})

await retryStream(run, controller.signal)

expect(run).toHaveBeenCalledTimes(1)
})

it('retries when run throws', async () => {
vi.useFakeTimers()

const controller = new AbortController()
let callCount = 0

const run = vi.fn().mockImplementation(async () => {
callCount++
if (callCount < 3) {
throw new Error('stream error')
}
controller.abort()
})

const promise = retryStream(run, controller.signal)
// Advance through the backoff delays
await vi.advanceTimersByTimeAsync(1_000)
await vi.advanceTimersByTimeAsync(2_000)

await promise

expect(run).toHaveBeenCalledTimes(3)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test: delay cap at MAX_DELAY_MS (30 s) is untested.

The exponential doubling is exercised up to 2 s but never hits the 30 s ceiling. Worth one parameterized test that runs enough failures to saturate the cap and verifies onRetry stops receiving larger values.

Also missing: a test for what happens when signal is already aborted before retryStream is called (the while (!signal.aborted) check on the first iteration). The current suite only aborts during an in-flight operation or backoff.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude (@claude) add these tests

@claude claude Bot Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude finished Devin T. Currie (@DTCurrie)'s task in 4m 12s —— View job


  • Read CLAUDE.md and test file context
  • Read retry-stream implementation
  • Add test for MAX_DELAY_MS cap (30s ceiling)
  • Add test for pre-aborted signal
  • Commit and push

Both missing tests have been added in commit 4e925fb:

caps delay at MAX_DELAY_MS (30s) and does not double beyond it — runs 6 consecutive failures to drive the backoff through 1s → 2s → 4s → 8s → 16s → 30s (capped), then asserts the 6th onRetry call received 30_000 rather than 32_000.

does not call run when signal is already aborted before retryStream is called — pre-aborts the controller before passing the signal, asserts run is never invoked (tests the while (!signal.aborted) guard on first entry).
· Branch


vi.useRealTimers()
})

it('calls onRetry but does not retry run when signal is aborted during backoff', async () => {
vi.useFakeTimers()

const controller = new AbortController()
const run = vi.fn().mockRejectedValue(new Error('stream error'))
const onRetry = vi.fn()

const promise = retryStream(run, controller.signal, onRetry)

// First call fails immediately, then waits for backoff
await vi.advanceTimersByTimeAsync(0)
expect(run).toHaveBeenCalledTimes(1)

// Abort during backoff wait
controller.abort()
await vi.advanceTimersByTimeAsync(1_000)

await promise

// Should have called onRetry once, but not retried run
expect(onRetry).toHaveBeenCalledTimes(1)
Comment thread
DTCurrie marked this conversation as resolved.
expect(run).toHaveBeenCalledTimes(1)

vi.useRealTimers()
})

it('calls onRetry with the current delay', async () => {
vi.useFakeTimers()

const controller = new AbortController()
let callCount = 0

const run = vi.fn().mockImplementation(async () => {
callCount++
if (callCount < 3) {
throw new Error('stream error')
}
controller.abort()
})

const onRetry = vi.fn()
const promise = retryStream(run, controller.signal, onRetry)

await vi.advanceTimersByTimeAsync(1_000)
await vi.advanceTimersByTimeAsync(2_000)

await promise

expect(onRetry).toHaveBeenCalledTimes(2)
expect(onRetry).toHaveBeenNthCalledWith(1, 1_000)
expect(onRetry).toHaveBeenNthCalledWith(2, 2_000)

vi.useRealTimers()
})

it('does not call onRetry and restarts immediately on clean stream end', async () => {
const controller = new AbortController()
let callCount = 0

const run = vi.fn().mockImplementation(async () => {
callCount++
if (callCount === 1) return // clean end — server closed the stream
controller.abort()
})

const onRetry = vi.fn()
await retryStream(run, controller.signal, onRetry)

expect(run).toHaveBeenCalledTimes(2)
expect(onRetry).not.toHaveBeenCalled()
})

it('caps delay at MAX_DELAY_MS (30s) and does not double beyond it', async () => {
vi.useFakeTimers()

const controller = new AbortController()
let callCount = 0

const run = vi.fn().mockImplementation(async () => {
callCount++
if (callCount <= 6) throw new Error('stream error')
controller.abort()
})

const onRetry = vi.fn()
const promise = retryStream(run, controller.signal, onRetry)

// Advance through each exponential backoff step
await vi.advanceTimersByTimeAsync(1_000) // delay: 1000
await vi.advanceTimersByTimeAsync(2_000) // delay: 2000
await vi.advanceTimersByTimeAsync(4_000) // delay: 4000
await vi.advanceTimersByTimeAsync(8_000) // delay: 8000
await vi.advanceTimersByTimeAsync(16_000) // delay: 16000
await vi.advanceTimersByTimeAsync(30_000) // delay: capped at 30000 (not 32000)

await promise

expect(onRetry).toHaveBeenCalledTimes(6)
expect(onRetry).toHaveBeenNthCalledWith(5, 16_000)
expect(onRetry).toHaveBeenNthCalledWith(6, 30_000)

vi.useRealTimers()
})

it('does not call run when signal is already aborted before retryStream is called', async () => {
const controller = new AbortController()
controller.abort()

const run = vi.fn()
await retryStream(run, controller.signal)

expect(run).not.toHaveBeenCalled()
})

it('resets delay after a successful run', async () => {
vi.useFakeTimers()

const controller = new AbortController()
let callCount = 0

const run = vi.fn().mockImplementation(async () => {
callCount++
// First call: fail
if (callCount === 1) throw new Error('fail')
// Second call: succeed (stream ended cleanly)
if (callCount === 2) return
// Third call: fail
if (callCount === 3) throw new Error('fail')
// Fourth call: abort
controller.abort()
})

const onRetry = vi.fn()
const promise = retryStream(run, controller.signal, onRetry)

// First failure + 1s backoff
await vi.advanceTimersByTimeAsync(1_000)
// Second call succeeds, delay resets. Third call fails, should use 1s again
await vi.advanceTimersByTimeAsync(1_000)
// Fourth call - abort
await vi.advanceTimersByTimeAsync(2_000)

await promise

// Both retries should have used 1000ms (reset after success)
expect(onRetry).toHaveBeenNthCalledWith(1, 1_000)
expect(onRetry).toHaveBeenNthCalledWith(2, 1_000)

vi.useRealTimers()
})
})
24 changes: 16 additions & 8 deletions src/lib/hooks/useGeometries.svelte.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,19 +222,27 @@ export const provideGeometries = (partID: () => string) => {
})
}

// Clean up owners whose queries disappeared entirely
// Clean up owners whose queries disappeared entirely.
// Guard: if ALL queries are gone (activeQueryKeys empty), the machine is likely
// temporarily disconnected — preserve entities so they reappear on reconnect.
// Only destroy when the partID changed (old-partID entities) or other queries
// are still active (connected machine, resource legitimately removed).
const anyQueriesActive = activeQueryKeys.size > 0
for (const [queryKey, keys] of queryEntityKeys) {
if (!activeQueryKeys.has(queryKey)) {
for (const key of keys) {
const entity = entities.get(key)
if (entity && world.has(entity)) {
entity.destroy()
const queryPartID = queryKey.split(':')[0]!
if (queryPartID !== currentPartID || anyQueriesActive) {
Comment thread
claude[bot] marked this conversation as resolved.
for (const key of keys) {
const entity = entities.get(key)
if (entity && world.has(entity)) {
entity.destroy()
}

entities.delete(key)
}

entities.delete(key)
queryEntityKeys.delete(queryKey)
}

queryEntityKeys.delete(queryKey)
}
}
})
Expand Down
16 changes: 12 additions & 4 deletions src/lib/hooks/usePointclouds.svelte.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,13 +188,21 @@ export const providePointclouds = (partID: () => string) => {
})
}

// clean up queries that disappeared entirely
// clean up queries that disappeared entirely.
// Guard: if ALL queries are gone (activeQueryKeys empty), the machine is likely
// temporarily disconnected — preserve entities so they reappear on reconnect.
// Only destroy when the partID changed (old-partID entities) or other queries
// are still active (connected machine, camera legitimately removed).
const anyQueriesActive = activeQueryKeys.size > 0
for (const [queryKey, entity] of entities) {
if (!activeQueryKeys.has(queryKey)) {
if (world.has(entity)) {
entity.destroy()
const queryPartID = queryKey.split(':')[0]!
Comment thread
claude[bot] marked this conversation as resolved.
if (queryPartID !== currentPartID || anyQueriesActive) {
if (world.has(entity)) {
entity.destroy()
}
entities.delete(queryKey)
}
entities.delete(queryKey)
}
}
})
Expand Down
Loading
Loading