From f689a5fed624944f1745d9c7a7399697c194fecd Mon Sep 17 00:00:00 2001 From: yuriyryabikov <22548029+kurok@users.noreply.github.com> Date: Fri, 10 Jul 2026 11:29:38 +0100 Subject: [PATCH] fix: wait for the stopped state in mode:stop before returning StopInstances only moves the instance to 'stopping'; mode:stop with reuse:stop returned immediately, so a workflow run queued right behind the stopping one saw an empty warm pool (findStoppedPoolInstance and consumers filter on instance-state-name=stopped) and cold-launched a duplicate instance that the reaper does not drain until its stopped-max-age passes. Observed in terraform-provider-namecheap CI on 2026-07-10: run A's stop-runner exited at 08:45:51 (0.2ms after StopInstances), run B's queue released at 08:45:55, and at 08:46:04 run B logged warm_start outcome:pool_empty and launched a second instance; every subsequent run that day warned 'Found 2 stopped warm-pool instances'. Block in stopInstanceById on waitUntilInstanceStopped (maxWaitTime 300s, mirroring waitForInstanceRunning) so 'mode:stop finished' implies 'instance is reusable'. A stop that cannot complete within the window now fails the step loudly instead of leaving a silent stopping-state race. Signed-off-by: yuriyryabikov <22548029+kurok@users.noreply.github.com> --- dist/index.js | 24 +++++++++++++++++++++++- src/aws.js | 24 +++++++++++++++++++++++- tests/warmpool.test.js | 19 ++++++++++++++++++- 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/dist/index.js b/dist/index.js index 54250cdb..674df534 100644 --- a/dist/index.js +++ b/dist/index.js @@ -104662,6 +104662,7 @@ const { ModifyInstanceAttributeCommand, AssociateAddressCommand, waitUntilInstanceRunning, + waitUntilInstanceStopped, } = __nccwpck_require__(5193); const fs = __nccwpck_require__(9896); const core = __nccwpck_require__(7484); @@ -105835,11 +105836,32 @@ async function warmStartInstance(instanceId, { userData, label }) { } // Stop (not terminate) an instance so it can be reused from the pool. +// +// Blocks until the instance actually reaches the `stopped` state before +// returning. StopInstances alone only moves it to `stopping`, and a warm-pool +// lookup that runs during that window (findStoppedPoolInstance filters on +// instance-state-name=stopped) sees an empty pool and cold-launches a +// duplicate instance the reaper won't drain until its stopped-max-age passes. +// Waiting here makes "mode: stop finished" imply "instance is reusable", which +// is the serialization boundary queued follow-up workflow runs rely on. async function stopInstanceById(instanceId) { const client = ec2Client(); await withRetry('stop_instance', () => client.send(new StopInstancesCommand({ InstanceIds: [instanceId] }))); log.info('stop_instance', { instance_id: instanceId }); - core.info(`AWS EC2 instance ${instanceId} is stopped (warm pool)`); + const start = Date.now(); + log.info('wait_for_instance_stopped', { instance_id: instanceId }); + try { + await waitUntilInstanceStopped( + { client, maxWaitTime: 300 }, + { InstanceIds: [instanceId] }, + ); + log.info('wait_for_instance_stopped', { instance_id: instanceId, elapsed_ms: Date.now() - start }); + core.info(`AWS EC2 instance ${instanceId} is stopped (warm pool)`); + } catch (error) { + log.error('wait_for_instance_stopped', { instance_id: instanceId, error: error.name, message: error.message }); + core.error(`AWS EC2 instance ${instanceId} did not reach the stopped state within the wait window`); + throw error; + } } // Read the reuse-cycle counter tag for an instance (0 when absent). diff --git a/src/aws.js b/src/aws.js index 854f6164..1d319877 100644 --- a/src/aws.js +++ b/src/aws.js @@ -12,6 +12,7 @@ const { ModifyInstanceAttributeCommand, AssociateAddressCommand, waitUntilInstanceRunning, + waitUntilInstanceStopped, } = require('@aws-sdk/client-ec2'); const fs = require('fs'); const core = require('@actions/core'); @@ -1185,11 +1186,32 @@ async function warmStartInstance(instanceId, { userData, label }) { } // Stop (not terminate) an instance so it can be reused from the pool. +// +// Blocks until the instance actually reaches the `stopped` state before +// returning. StopInstances alone only moves it to `stopping`, and a warm-pool +// lookup that runs during that window (findStoppedPoolInstance filters on +// instance-state-name=stopped) sees an empty pool and cold-launches a +// duplicate instance the reaper won't drain until its stopped-max-age passes. +// Waiting here makes "mode: stop finished" imply "instance is reusable", which +// is the serialization boundary queued follow-up workflow runs rely on. async function stopInstanceById(instanceId) { const client = ec2Client(); await withRetry('stop_instance', () => client.send(new StopInstancesCommand({ InstanceIds: [instanceId] }))); log.info('stop_instance', { instance_id: instanceId }); - core.info(`AWS EC2 instance ${instanceId} is stopped (warm pool)`); + const start = Date.now(); + log.info('wait_for_instance_stopped', { instance_id: instanceId }); + try { + await waitUntilInstanceStopped( + { client, maxWaitTime: 300 }, + { InstanceIds: [instanceId] }, + ); + log.info('wait_for_instance_stopped', { instance_id: instanceId, elapsed_ms: Date.now() - start }); + core.info(`AWS EC2 instance ${instanceId} is stopped (warm pool)`); + } catch (error) { + log.error('wait_for_instance_stopped', { instance_id: instanceId, error: error.name, message: error.message }); + core.error(`AWS EC2 instance ${instanceId} did not reach the stopped state within the wait window`); + throw error; + } } // Read the reuse-cycle counter tag for an instance (0 when absent). diff --git a/tests/warmpool.test.js b/tests/warmpool.test.js index 787e5b02..11fe2477 100644 --- a/tests/warmpool.test.js +++ b/tests/warmpool.test.js @@ -18,6 +18,7 @@ jest.mock('@aws-sdk/client-ec2', () => ({ ModifyInstanceAttributeCommand: jest.fn((p) => ({ __command: 'ModifyInstanceAttribute', ...p })), AssociateAddressCommand: jest.fn((p) => ({ __command: 'AssociateAddress', ...p })), waitUntilInstanceRunning: jest.fn(), + waitUntilInstanceStopped: jest.fn(), })); jest.mock('../src/retry', () => ({ withRetry: (_step, fn) => fn() })); jest.mock('../src/config', () => ({ @@ -101,11 +102,27 @@ describe('warmStartInstance', () => { }); describe('stopInstanceById', () => { - test('sends StopInstances', async () => { + const { waitUntilInstanceStopped } = require('@aws-sdk/client-ec2'); + + beforeEach(() => waitUntilInstanceStopped.mockReset()); + + test('sends StopInstances, then waits for the stopped state', async () => { mockSend.mockResolvedValue({}); + waitUntilInstanceStopped.mockResolvedValueOnce({ state: 'SUCCESS' }); await aws.stopInstanceById('i-9'); expect(commandsSent()).toEqual(['StopInstances']); expect(mockSend.mock.calls[0][0].InstanceIds).toEqual(['i-9']); + // The waiter is the whole point: returning at `stopping` lets a queued + // follow-up run see an empty pool and cold-launch a duplicate instance. + expect(waitUntilInstanceStopped).toHaveBeenCalledTimes(1); + expect(waitUntilInstanceStopped.mock.calls[0][1]).toEqual({ InstanceIds: ['i-9'] }); + }); + + test('throws when the instance never reaches stopped', async () => { + mockSend.mockResolvedValue({}); + waitUntilInstanceStopped.mockRejectedValueOnce(new Error('TimeoutError')); + await expect(aws.stopInstanceById('i-9')).rejects.toThrow('TimeoutError'); + expect(commandsSent()).toEqual(['StopInstances']); }); });