Skip to content

Commit b378e6d

Browse files
committed
fix(app): wait for the server before running the one-time imports
Found testing an upgrade against a real profile. The database migration worked and the extension storage was intact, yet no providers or scheduled tasks appeared, and the run looked like data loss. The imports run when the background starts, which is the same moment the server starts. They fired into a socket nothing was listening on yet and threw. Because a failed run leaves its marker unset, the next launch lost the same race, so the data never arrived at all while the database migration looked like it had succeeded. Roughly six seconds passed between the browser launching and the server answering. The existing retry did not help because it retried the wrong thing. resolveAgentServerUrlWithRetry retries getAgentServerUrl, which only reads a preference and effectively never fails, so the budget was spent before the request that needed it. The request itself had one attempt. The imports now wait on the health endpoint first, so they wait for their dependency rather than race it. Giving up returns false rather than throwing, which leaves the markers unset deliberately for the next start, and failures are reported through Sentry instead of swallowed: silence is what made this look like lost data rather than a slow start.
1 parent f592d57 commit b378e6d

3 files changed

Lines changed: 228 additions & 24 deletions

File tree

packages/browseros-agent/apps/app/modules/local-first-migration/start-local-first-migration.ts

Lines changed: 68 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
scheduledJobRunStorage,
1313
scheduledJobStorage,
1414
} from '@/lib/schedules/scheduleStorage'
15+
import { sentry } from '@/lib/sentry/sentry'
1516
import { resolveAgentServerUrlWithRetry } from '@/modules/browseros/agent-server-url.helpers'
1617
import { putDefaultProvider } from '@/modules/llm-providers/llm-providers.api'
1718
import { importScheduledJobRuns } from '@/modules/schedules/schedules.api'
@@ -25,6 +26,7 @@ import {
2526
parseProviderBackup,
2627
type ScheduledJobImport,
2728
} from './local-first-migration.helpers'
29+
import { waitForAgentServer } from './wait-for-agent-server'
2830

2931
/**
3032
* Per profile, because extension storage is per profile. Losing it costs a
@@ -83,35 +85,77 @@ async function importScheduledJobs(jobs: ScheduledJobImport[]): Promise<void> {
8385
}
8486
}
8587

86-
/** Fire and forget from the background; a failure retries on next startup. */
88+
/**
89+
* Runs the one-time imports once the server is up.
90+
*
91+
* Everything here happens when the background starts, which is also when the
92+
* server starts, so firing straight away meant importing into a socket nothing
93+
* was listening on. Each import then failed, and because a failed run leaves
94+
* its marker unset it lost the same race on the next launch too, so a user
95+
* upgrading saw their providers and scheduled tasks simply never arrive while
96+
* the database migration looked like it had worked.
97+
*
98+
* Waiting on health first removes the race. A failure past that point is worth
99+
* seeing rather than swallowing: it is the difference between a slow start and
100+
* data that never came across.
101+
*/
87102
export function startLocalFirstMigration(): void {
88-
// The default is chained onto the provider import rather than fired
89-
// alongside it: the id it names has to exist server side before it can be
90-
// made default. The run history is independent and does not wait.
91-
void runLocalFirstMigration({
92-
isDone: () => migrationDoneStorage.getValue(),
93-
markDone: () => migrationDoneStorage.setValue(true),
94-
loadStoredProviders: async () => (await providersStorage.getValue()) ?? [],
95-
loadBackupProviders,
96-
loadScheduledJobs: async () => (await scheduledJobStorage.getValue()) ?? [],
97-
importProviders,
98-
importScheduledJobs,
99-
})
100-
.then(() =>
101-
runDefaultProviderMigration({
103+
void (async () => {
104+
if (!(await waitForAgentServer())) {
105+
sentry.captureException(
106+
new Error('Agent server unreachable before the local-first import'),
107+
{
108+
extra: {
109+
message:
110+
'Imports deferred to the next start; markers remain unset so they will run again',
111+
},
112+
},
113+
)
114+
return
115+
}
116+
117+
// The default is chained onto the provider import rather than run
118+
// alongside it: the id it names has to exist server side before it can be
119+
// made default. The run history is independent and does not wait.
120+
try {
121+
await runLocalFirstMigration({
122+
isDone: () => migrationDoneStorage.getValue(),
123+
markDone: () => migrationDoneStorage.setValue(true),
124+
loadStoredProviders: async () =>
125+
(await providersStorage.getValue()) ?? [],
126+
loadBackupProviders,
127+
loadScheduledJobs: async () =>
128+
(await scheduledJobStorage.getValue()) ?? [],
129+
importProviders,
130+
importScheduledJobs,
131+
})
132+
await runDefaultProviderMigration({
102133
isDone: () => defaultMigrationDoneStorage.getValue(),
103134
markDone: () => defaultMigrationDoneStorage.setValue(true),
104135
loadStoredDefaultId: async () =>
105136
(await defaultProviderIdStorage.getValue()) || null,
106137
setDefault: putDefaultProvider,
107-
}),
108-
)
109-
.catch(() => null)
138+
})
139+
} catch (error) {
140+
// Reported rather than swallowed: a silent failure here is
141+
// indistinguishable from the user's data having vanished, which is
142+
// exactly how this went unnoticed.
143+
sentry.captureException(error, {
144+
extra: { message: 'Provider and scheduled job import failed' },
145+
})
146+
}
110147

111-
void runScheduledRunsMigration({
112-
isDone: () => runsMigrationDoneStorage.getValue(),
113-
markDone: () => runsMigrationDoneStorage.setValue(true),
114-
loadRuns: async () => (await scheduledJobRunStorage.getValue()) ?? [],
115-
importRuns: importScheduledJobRuns,
116-
}).catch(() => null)
148+
try {
149+
await runScheduledRunsMigration({
150+
isDone: () => runsMigrationDoneStorage.getValue(),
151+
markDone: () => runsMigrationDoneStorage.setValue(true),
152+
loadRuns: async () => (await scheduledJobRunStorage.getValue()) ?? [],
153+
importRuns: importScheduledJobRuns,
154+
})
155+
} catch (error) {
156+
sentry.captureException(error, {
157+
extra: { message: 'Scheduled run history import failed' },
158+
})
159+
}
160+
})()
117161
}
Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
import { describe, expect, it } from 'bun:test'
2+
import { waitForAgentServer } from './wait-for-agent-server'
3+
4+
function harness(healthyAfter: number) {
5+
let calls = 0
6+
let clock = 0
7+
return {
8+
calls: () => calls,
9+
elapsed: () => clock,
10+
opts: {
11+
isHealthy: async () => {
12+
calls += 1
13+
return calls > healthyAfter
14+
},
15+
now: () => clock,
16+
sleep: async (ms: number) => {
17+
clock += ms
18+
},
19+
timeoutMs: 60_000,
20+
intervalMs: 1_000,
21+
},
22+
}
23+
}
24+
25+
describe('waitForAgentServer', () => {
26+
it('returns immediately when the server is already up', async () => {
27+
const h = harness(0)
28+
29+
expect(await waitForAgentServer(h.opts)).toBe(true)
30+
expect(h.calls()).toBe(1)
31+
expect(h.elapsed()).toBe(0)
32+
})
33+
34+
// The case this exists for: the background starts at the same moment as the
35+
// server, so the first few probes find nothing listening.
36+
it('waits out a server that is still starting', async () => {
37+
const h = harness(6)
38+
39+
expect(await waitForAgentServer(h.opts)).toBe(true)
40+
expect(h.calls()).toBe(7)
41+
expect(h.elapsed()).toBe(6_000)
42+
})
43+
44+
// Six seconds is roughly what was observed between the browser launching and
45+
// the server answering, and the previous behaviour gave up inside two.
46+
it('outlasts the gap that made the import fail', async () => {
47+
const h = harness(6)
48+
await waitForAgentServer(h.opts)
49+
50+
expect(h.elapsed()).toBeGreaterThan(1_500)
51+
})
52+
53+
// Giving up rather than throwing is what lets the caller leave the markers
54+
// unset, so the next start tries again.
55+
it('reports failure rather than throwing when the server never answers', async () => {
56+
const h = harness(Number.POSITIVE_INFINITY)
57+
58+
expect(await waitForAgentServer(h.opts)).toBe(false)
59+
})
60+
61+
it('stops probing once the deadline passes', async () => {
62+
const h = harness(Number.POSITIVE_INFINITY)
63+
await waitForAgentServer(h.opts)
64+
65+
expect(h.elapsed()).toBeLessThanOrEqual(60_000)
66+
expect(h.calls()).toBeLessThanOrEqual(62)
67+
})
68+
69+
// Connection refused is the expected state early on, so a probe that throws
70+
// has to read as not reachable rather than end the wait.
71+
it('treats a throwing probe as not reachable and keeps waiting', async () => {
72+
let calls = 0
73+
let clock = 0
74+
75+
const result = await waitForAgentServer({
76+
isHealthy: async () => {
77+
calls += 1
78+
if (calls < 3) throw new Error('connection refused')
79+
return true
80+
},
81+
now: () => clock,
82+
sleep: async (ms) => {
83+
clock += ms
84+
},
85+
timeoutMs: 60_000,
86+
intervalMs: 1_000,
87+
})
88+
89+
expect(result).toBe(true)
90+
expect(calls).toBe(3)
91+
expect(clock).toBe(2_000)
92+
})
93+
})
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
import { getHealthCheckUrl } from '@/lib/browseros/helpers'
2+
3+
/**
4+
* Long enough to cover a cold start where the server is booting alongside the
5+
* browser and has migrations of its own to apply, short enough that a genuinely
6+
* absent server does not keep a background task alive all session.
7+
*/
8+
export const SERVER_WAIT_TIMEOUT_MS = 60_000
9+
export const SERVER_WAIT_INTERVAL_MS = 1_000
10+
11+
export interface WaitForAgentServerOptions {
12+
isHealthy?: () => Promise<boolean>
13+
timeoutMs?: number
14+
intervalMs?: number
15+
now?: () => number
16+
sleep?: (ms: number) => Promise<void>
17+
}
18+
19+
async function defaultIsHealthy(): Promise<boolean> {
20+
try {
21+
const response = await fetch(await getHealthCheckUrl())
22+
return response.ok
23+
} catch {
24+
return false
25+
}
26+
}
27+
28+
const defaultSleep = (ms: number) =>
29+
new Promise<void>((resolve) => setTimeout(resolve, ms))
30+
31+
/**
32+
* Waits until the local server answers, or gives up.
33+
*
34+
* The one-time import runs when the background starts, which is the same moment
35+
* the server starts. It used to fire straight into a socket nothing was
36+
* listening on yet and fail, and because a failed run leaves its marker unset
37+
* it simply lost the same race on the next launch, so the imported data never
38+
* appeared at all.
39+
*
40+
* Polling health first is what makes the import wait for its dependency rather
41+
* than race it. Returning false rather than throwing keeps the caller's
42+
* decision explicit: leave the markers unset and try again next start.
43+
*/
44+
export async function waitForAgentServer({
45+
isHealthy = defaultIsHealthy,
46+
timeoutMs = SERVER_WAIT_TIMEOUT_MS,
47+
intervalMs = SERVER_WAIT_INTERVAL_MS,
48+
now = Date.now,
49+
sleep = defaultSleep,
50+
}: WaitForAgentServerOptions = {}): Promise<boolean> {
51+
const deadline = now() + timeoutMs
52+
53+
while (true) {
54+
// A probe that throws means not reachable, not a reason to abandon the
55+
// wait. The default one already swallows fetch errors; catching here means
56+
// any probe behaves the same way.
57+
let healthy = false
58+
try {
59+
healthy = await isHealthy()
60+
} catch {
61+
healthy = false
62+
}
63+
if (healthy) return true
64+
if (now() >= deadline) return false
65+
await sleep(intervalMs)
66+
}
67+
}

0 commit comments

Comments
 (0)