diff --git a/.changeset/prevent-stale-stop-state.md b/.changeset/prevent-stale-stop-state.md new file mode 100644 index 0000000..76fe621 --- /dev/null +++ b/.changeset/prevent-stale-stop-state.md @@ -0,0 +1,5 @@ +--- +"@cloudflare/containers": patch +--- + +Prevent startup and stop handling races from persisting a stopped state for a running container. diff --git a/src/lib/container.ts b/src/lib/container.ts index beaf07f..b10439d 100644 --- a/src/lib/container.ts +++ b/src/lib/container.ts @@ -264,6 +264,14 @@ class ContainerState { await this.setStatusAndupdate('stopped'); } + async setStoppedIfUnchanged(previousState: State) { + if (this.status !== previousState) { + return; + } + + await this.setStopped(); + } + async setStoppedWithCode(exitCode: number) { this.status = { status: 'stopped_with_code', lastChange: Date.now(), exitCode }; await this.update(); @@ -774,10 +782,7 @@ export class Container extends DurableObject { * @returns A promise that resolves when the container start command has been issued * @throws Error if no container context is available or if all start attempts fail */ - public async start( - startOptions?: ContainerStartConfigOptions, - waitOptions?: WaitOptions - ): Promise { + public async start(startOptions?: ContainerStartConfigOptions, waitOptions?: WaitOptions) { const portToCheck = waitOptions?.portToCheck ?? this.defaultPort ?? @@ -1745,7 +1750,6 @@ export class Container extends DurableObject { }); const pollInterval = waitOptions.waitInterval ?? INSTANCE_POLL_INTERVAL_MS; const totalTries = waitOptions.retries ?? Math.ceil(TIMEOUT_TO_GET_CONTAINER_MS / pollInterval); - await this.state.setRunning(); for (let tries = 0; tries < totalTries; tries++) { // Use provided options or fall back to instance properties const envVars = options?.envVars ?? this.envVars; @@ -1809,6 +1813,7 @@ export class Container extends DurableObject { await this.refreshOutboundInterception(); this.container.start(startConfig); this.monitor = this.container.monitor(); + await this.state.setRunning(); } else { await this.scheduleNextAlarm(); } @@ -2079,17 +2084,17 @@ export class Container extends DurableObject { private async syncPendingStoppedEvents() { const state = await this.state.getState(); if (!this.container.running && (state.status === 'healthy' || state.status === 'running')) { - await this.callOnStop({ exitCode: 0, reason: 'exit' }); + await this.callOnStop({ exitCode: 0, reason: 'exit' }, state); return; } if (!this.container.running && state.status === 'stopped_with_code') { - await this.callOnStop({ exitCode: state.exitCode ?? 0, reason: 'exit' }); + await this.callOnStop({ exitCode: state.exitCode ?? 0, reason: 'exit' }, state); return; } } - private async callOnStop(onStopParams: StopParams) { + private async callOnStop(onStopParams: StopParams, stateBeforeOnStop: State) { if (this.onStopCalled) { return; } @@ -2104,7 +2109,7 @@ export class Container extends DurableObject { this.onStopCalled = false; } - await this.state.setStopped(); + await this.state.setStoppedIfUnchanged(stateBeforeOnStop); } /** diff --git a/src/tests/container.test.ts b/src/tests/container.test.ts index 7a129e1..c2e603c 100644 --- a/src/tests/container.test.ts +++ b/src/tests/container.test.ts @@ -221,6 +221,39 @@ describe('Container', () => { expect(mockCtx.container.getTcpPort).toHaveBeenCalledWith(33); }); + test('alarm should not stop a container while its start loop is in flight', async ({ + mockCtx, + container, + }) => { + let resumeStart: () => void = () => undefined; + const startBlockedBeforePhysicalStart = new Promise(resolve => { + resumeStart = resolve; + }); + vi.spyOn(container, 'scheduleNextAlarm').mockImplementationOnce( + () => startBlockedBeforePhysicalStart + ); + using onStopSpy = vi.spyOn(container, 'onStop'); + + const startPromise = container.start(undefined, { + portToCheck: 8080, + retries: 1, + waitInterval: 1, + }); + await vi.waitFor(() => { + expect(container.scheduleNextAlarm).toHaveBeenCalled(); + }); + expect(mockCtx.container.start).not.toHaveBeenCalled(); + + await container.alarm(); + resumeStart(); + await startPromise; + + expect(onStopSpy).not.toHaveBeenCalled(); + await expect(container.getState()).resolves.toEqual( + expect.objectContaining({ status: 'running' }) + ); + }); + test('syncPendingStoppedEvents should call onStop for stopped container with running state', async ({ mockCtx, container, @@ -239,6 +272,26 @@ describe('Container', () => { ); }); + test('onStop starting a new container should not overwrite its running state', async ({ + mockCtx, + container, + }) => { + mockCtx.storage.get.mockResolvedValue({ status: 'running', lastChange: Date.now() }); + mockCtx.container.running = false; + vi.spyOn(container, 'onStop').mockImplementation(async () => { + await container.start(undefined, { portToCheck: 8080, retries: 1, waitInterval: 1 }); + }); + + await ( + container as unknown as { syncPendingStoppedEvents(): Promise } + ).syncPendingStoppedEvents(); + + expect(mockCtx.container.running).toBe(true); + await expect(container.getState()).resolves.toEqual( + expect.objectContaining({ status: 'running' }) + ); + }); + test('containerFetch should forward requests to container', async ({ mockCtx, container }) => { const mockRequest = new Request('https://example.com/test?query=value', { method: 'GET', diff --git a/src/tests/fixtures.ts b/src/tests/fixtures.ts index 2f57ba6..c84d6d8 100644 --- a/src/tests/fixtures.ts +++ b/src/tests/fixtures.ts @@ -36,6 +36,7 @@ export function makeMockCtx() { put: vi.fn<(key: string, value: unknown) => Promise>().mockResolvedValue(undefined), delete: vi.fn<(key: string) => Promise>().mockResolvedValue(true), setAlarm: vi.fn<(scheduledTime: number) => Promise>().mockResolvedValue(undefined), + deleteAlarm: vi.fn<() => Promise>().mockResolvedValue(undefined), sync: vi.fn<() => Promise>().mockResolvedValue(undefined), kv: { get: vi.fn<(key: string) => Promise>(),