fix: address Copilot review comments on runner count cache PR

- Fix double-counting issue: only count 'running' state, ignore 'pending'
- Add warning log when DynamoDB returns negative counts
- Add EC2 permission comment explaining why resource='*' is required
- Fix 'Time out' typo to 'Timeout' in variable descriptions
- Fix README AWS provider version mismatch (~>5.27 -> >=6.21)
- Add race condition documentation between cache layers
- Add debug log when stale DynamoDB cache is used
- Fix Terraform formatting

Signed-off-by: s1v4-d <161426787+s1v4-d@users.noreply.github.com>

Author: s1v4-d

lambdas/functions/control-plane/src/scale-runners/cache.ts

@@ -228,6 +228,16 @@ export class dynamoDbRunnerCountCache {
 
       logger.debug('DynamoDB cache hit', { pk, count, isStale, ageMs: Date.now() - updated });
 
+      // Normalize negative counts to zero. This can happen due to race conditions with
+      // EventBridge events (e.g., termination event arrives before running event).
+      if (count < 0) {
+        logger.warn('DynamoDB cache returned negative count, normalizing to 0', {
+          pk,
+          rawCount: count,
+          updated,
+        });
+      }
+
       return { count: Math.max(0, count), isStale };
     } catch (error) {
       logger.warn('Failed to read from DynamoDB cache', { pk, error });

lambdas/functions/control-plane/src/scale-runners/scale-up.ts

@@ -394,6 +394,11 @@ export async function scaleUp(payloads: ActionRequestMessageSQS[]): Promise<stri
             group,
             cacheSource,
           });
+        } else if (dynamoResult !== null && dynamoResult.isStale) {
+          logger.debug('DynamoDB cache entry is stale, falling back to other caches', {
+            staleCount: dynamoResult.count,
+            group,
+          });
         }
       }
 
@@ -484,7 +489,11 @@ export async function scaleUp(payloads: ActionRequestMessageSQS[]): Promise<stri
       githubInstallationClient,
     );
 
-    // Update the cache with the new runner count to avoid stale data in subsequent iterations
+    // Update the cache with the new runner count to avoid stale data in subsequent iterations.
+    // NOTE: There's an inherent race condition between this in-memory cache increment and
+    // the DynamoDB cache updates from EventBridge. The in-memory cache provides immediate
+    // visibility within this Lambda invocation, while DynamoDB provides cross-invocation
+    // consistency. The stale threshold ensures we eventually fall back to EC2 API for accuracy.
     if (instances.length > 0) {
       ec2RunnerCountCache.increment(environment, runnerType, group, instances.length);
     }

lambdas/functions/runner-count-cache/src/lambda.ts

@@ -153,16 +153,19 @@ export async function handler(
   // Generate partition key
   const pk = `${tags.environment}#${tags.type}#${tags.owner}`;
 
-  // Determine increment based on state
+  // Determine increment based on state.
+  // IMPORTANT: We only count 'running' state as +1 to avoid double-counting when instances
+  // transition from pending -> running. The 'pending' state is ignored because all instances
+  // that reach 'running' must first pass through 'pending', which would cause double-counting.
   let increment = 0;
-  if (state === 'running' || state === 'pending') {
+  if (state === 'running') {
     increment = 1;
   } else if (state === 'terminated' || state === 'stopped' || state === 'shutting-down') {
     increment = -1;
   }
 
   if (increment === 0) {
-    logger.debug('State does not affect counter', { state });
+    logger.debug('State does not affect counter (pending or other transitional states are ignored)', { state });
     return;
   }
 

modules/runner-count-cache/README.md

@@ -75,7 +75,7 @@ The scale-up Lambda can be configured to use this cache by setting these environ
 | Name | Version |
 |------|---------|
 | terraform | >= 1.3.0 |
-| aws | ~> 5.27 |
+| aws | >= 6.21 |
 
 ## Inputs
 

modules/runner-count-cache/lambda.tf

@@ -26,10 +26,10 @@ resource "aws_lambda_function" "counter" {
 
   environment {
     variables = {
-      DYNAMODB_TABLE_NAME   = aws_dynamodb_table.runner_counts.name
-      ENVIRONMENT_FILTER    = var.environment_filter
-      TTL_SECONDS           = var.ttl_seconds
-      LOG_LEVEL             = "info"
+      DYNAMODB_TABLE_NAME     = aws_dynamodb_table.runner_counts.name
+      ENVIRONMENT_FILTER      = var.environment_filter
+      TTL_SECONDS             = var.ttl_seconds
+      LOG_LEVEL               = "info"
       POWERTOOLS_SERVICE_NAME = "runner-count-cache"
     }
   }
@@ -110,6 +110,9 @@ data "aws_iam_policy_document" "counter_ec2" {
       "ec2:DescribeInstances",
       "ec2:DescribeTags",
     ]
+    # EC2 Describe* actions require resource = "*" - they cannot be scoped to specific
+    # instance ARNs. See: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-policy-structure.html
+    # The Lambda filters instances by tags after fetching to ensure only runner instances are counted.
     resources = ["*"]
   }
 }

modules/runner-count-cache/outputs.tf

@@ -33,7 +33,7 @@ output "lambda_role" {
 output "cache_config" {
   description = "Configuration for scale-up Lambda to use the cache"
   value = {
-    table_name              = aws_dynamodb_table.runner_counts.name
-    stale_threshold_ms      = var.cache_stale_threshold_ms
+    table_name         = aws_dynamodb_table.runner_counts.name
+    stale_threshold_ms = var.cache_stale_threshold_ms
   }
 }

modules/runner-count-cache/variables.tf

@@ -21,7 +21,7 @@ variable "environment_filter" {
 }
 
 variable "counter_lambda_timeout" {
-  description = "Time out for the counter update lambda in seconds."
+  description = "Timeout for the counter update lambda in seconds."
   type        = number
   default     = 30
 }

modules/runners/scale-up.tf

@@ -182,9 +182,9 @@ resource "aws_iam_role_policy" "scale_up_runner_count_cache" {
     Version = "2012-10-17"
     Statement = [
       {
-        Sid      = "AllowDynamoDBRead"
-        Effect   = "Allow"
-        Action   = [
+        Sid    = "AllowDynamoDBRead"
+        Effect = "Allow"
+        Action = [
           "dynamodb:GetItem"
         ]
         Resource = "arn:${var.aws_partition}:dynamodb:${var.aws_region}:${data.aws_caller_identity.current.account_id}:table/${var.runner_count_cache.table_name}"

variables.runner-count-cache.tf

@@ -10,7 +10,7 @@ variable "runner_count_cache" {
     `stale_threshold_ms`: How long (in milliseconds) before a cached count is considered stale and falls back to EC2 API. Default 60000 (1 minute).
     `ttl_seconds`: TTL for DynamoDB items in seconds. Default 86400 (24 hours).
     `lambda_memory_size`: Memory size limit in MB of the counter lambda.
-    `lambda_timeout`: Time out of the counter lambda in seconds.
+    `lambda_timeout`: Timeout of the counter lambda in seconds.
     `lambda_s3_key`: S3 key for lambda function. Required if using S3 bucket to specify lambdas.
     `lambda_s3_object_version`: S3 object version for lambda function.
   EOF