fix: Make sure an alarm can't call onStop in the middle of a start() call

.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.

src/lib/container.ts

@@ -264,6 +264,14 @@
     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 @@
    * @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<void> {
+  public async start(startOptions?: ContainerStartConfigOptions, waitOptions?: WaitOptions) {
     const portToCheck =
       waitOptions?.portToCheck ??
       this.defaultPort ??
@@ -1745,7 +1750,6 @@
     });
     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 @@
         await this.refreshOutboundInterception();
         this.container.start(startConfig);
         this.monitor = this.container.monitor();
+        await this.state.setRunning();
       } else {
         await this.scheduleNextAlarm();
       }
@@ -2079,17 +2084,17 @@
   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 @@
       this.onStopCalled = false;
     }
 
-    await this.state.setStopped();
+    await this.state.setStoppedIfUnchanged(stateBeforeOnStop);
   }
 
   /**

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<void>(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<void> }
+    ).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',

src/tests/fixtures.ts

@@ -36,6 +36,7 @@ export function makeMockCtx() {
       put: vi.fn<(key: string, value: unknown) => Promise<void>>().mockResolvedValue(undefined),
       delete: vi.fn<(key: string) => Promise<boolean>>().mockResolvedValue(true),
       setAlarm: vi.fn<(scheduledTime: number) => Promise<void>>().mockResolvedValue(undefined),
+      deleteAlarm: vi.fn<() => Promise<void>>().mockResolvedValue(undefined),
       sync: vi.fn<() => Promise<void>>().mockResolvedValue(undefined),
       kv: {
         get: vi.fn<(key: string) => Promise<unknown>>(),