[concurrency] CRITICAL: Race Condition in create_issue Tool - Module-Level Mutable State

[github-actions]

Severity: CRITICAL
Tool: create_issue
File: actions/setup/js/create_issue.cjs:10-18,462
Analysis Date: 2026-02-12

Summary

The create_issue tool contains a critical concurrency safety issue caused by module-level mutable state (issuesToAssignCopilotGlobal). Multiple concurrent invocations of the tool share the same global array, leading to potential race conditions, lost updates, and data corruption when tracking issues for Copilot assignment.

Issue Details

Type: Global State / Mutable Data Structure / Missing Synchronization

Location: actions/setup/js/create_issue.cjs:10,462

Code Pattern:

// ❌ UNSAFE: Module-level mutable state shared across invocations
let issuesToAssignCopilotGlobal = [];  // Line 10

function getIssuesToAssignCopilot() {
  return issuesToAssignCopilotGlobal;  // Line 17 - returns shared state
}

// In the handler function (line 462):
if (hasCopilot && assignCopilot) {
  issuesToAssignCopilotGlobal.push(`\$\{qualifiedItemRepo}:\$\{issue.number}`);  // RACE CONDITION!
  core.info(`Queued issue \$\{qualifiedItemRepo}#\$\{issue.number} for copilot assignment`);
}

Race Condition Scenario:

1. T=0ms: Workflow invokes create_issue handler A (processes message 1)
2. T=1ms: Workflow invokes create_issue handler B concurrently (processes message 2)
3. T=10ms: Handler A pushes repo1:123 to issuesToAssignCopilotGlobal (array: ["repo1:123"])
4. T=11ms: Handler B pushes repo2:456 to issuesToAssignCopilotGlobal (array: ["repo1:123", "repo2:456"])
5. T=15ms: Handler A pushes repo1:124 to issuesToAssignCopilotGlobal (array: ["repo1:123", "repo2:456", "repo1:124"])
6. T=20ms: getIssuesToAssignCopilot() is called by handler manager
7. Result: Both handlers modified the same shared array - this works by accident in Node.js single-threaded event loop BUT:
   • Lost updates: If handlers are reset concurrently
   • Unexpected behavior: State from one handler leaks into another
   • Flaky behavior: Order-dependent results based on event loop scheduling
   • Testing issues: Global state persists across test cases

Detailed Analysis

Root Cause

The root cause is shared mutable state at module level. In JavaScript/Node.js:

• Modules are loaded once and cached
• Module-level variables are shared across all imports
• Multiple concurrent function calls share the same module scope
• Array .push() operations mutate the array in-place

While Node.js has a single-threaded event loop (preventing true race conditions), this pattern still causes issues:

1. State Leakage: If multiple workflow runs process create_issue concurrently, they share the same array
2. Unexpected Behavior: Handler A's state affects Handler B's results
3. Testing Complexity: Global state makes tests non-isolated (requires manual reset)
4. Future-Proofing: If Node.js ever supports true parallelism (worker threads), this becomes a data race

Concurrent Execution Example

// Timeline showing how global state is problematic:

// T=0ms: Test 1 starts
handler1.processMessage({title: "Issue 1"});  // Sets global state

// T=5ms: Test 2 starts (before Test 1 finishes)
handler2.processMessage({title: "Issue 2"});  // Modifies SAME global state

// T=10ms: Test 1 checks results
getIssuesToAssignCopilot();  // Returns ["repo:1", "repo:2"] - contaminated by Test 2!

// Result: Tests interfere with each other, flaky failures

Impact Assessment

• Data Integrity: Potential for lost updates if reset operations interleave with push operations
• Reliability: Order-dependent behavior makes debugging difficult; flaky test failures
• Security: Minimal direct security impact, but state leakage could expose issue numbers across workflow runs
• Maintainability: Global mutable state is an anti-pattern that complicates testing and reasoning about code

Recommended Fix

Approach: State Isolation - Move state into handler closure or pass via return values

// ✅ SAFE: State isolated within each handler invocation

async function main(config = {}) {
  // ... existing config setup ...
  
  // Track issues that need copilot assignment LOCALLY (not globally)
  const issuesToAssignCopilot = [];  // ✅ Local to this handler invocation
  
  // Message handler function
  const handler = async (message) => {
    // ... existing logic ...
    
    // Track issue for copilot assignment
    if (hasCopilot && assignCopilot) {
      issuesToAssignCopilot.push(`\$\{qualifiedItemRepo}:\$\{issue.number}`);  // ✅ Modifies local array
      core.info(`Queued issue \$\{qualifiedItemRepo}#\$\{issue.number} for copilot assignment`);
    }
    
    // ... rest of handler ...
  };
  
  // Return handler with accessor function
  return {
    handler,
    getIssuesToAssignCopilot: () => issuesToAssignCopilot,  // ✅ Closure-based accessor
  };
}

// Update handler manager to use the closure-based accessor
// In safe_output_handler_manager.cjs:
const createIssueHandlerResult = await createIssueModule.main(config);
const createIssueHandler = createIssueHandlerResult.handler;
// ... process messages ...
const issuesToAssignCopilot = createIssueHandlerResult.getIssuesToAssignCopilot();

Explanation: This fix eliminates shared mutable state by:

1. Moving issuesToAssignCopilot array into the handler closure (local scope)
2. Each handler invocation gets its own isolated array
3. Returning an accessor function via closure to retrieve the results
4. No module-level state - complete isolation between invocations

Implementation Steps:

1. Remove module-level issuesToAssignCopilotGlobal declaration (line 10)
2. Remove getIssuesToAssignCopilot() function (lines 16-18)
3. Remove resetIssuesToAssignCopilot() function (lines 24-26) - no longer needed
4. Add local issuesToAssignCopilot = [] inside main() function
5. Change handler to modify the local array instead of global
6. Return { handler, getIssuesToAssignCopilot: () => issuesToAssignCopilot } from main()
7. Update safe_output_handler_manager.cjs and safe_output_unified_handler_manager.cjs to use the returned accessor
8. Update tests to remove resetIssuesToAssignCopilot() calls (no longer needed with isolated state)

Alternative Solutions

Option 1: Return accumulated state with results

• Pros: No accessor needed, simpler API
• Cons: Requires handler API change (break existing interface)

Option 2: Use WeakMap for per-invocation state

• Pros: No API changes needed
• Cons: More complex, requires invocation ID tracking

Option 3: Make handlers stateless, pass state as parameter

• Pros: Pure functional approach, most testable
• Cons: Requires significant refactoring of handler architecture

Recommended: Option 1 (closure-based isolation) balances simplicity with safety.

Testing Strategy

To verify the fix handles concurrent execution safely:

describe('create_issue concurrency safety', () => {
  test('multiple handlers do not share state', async () => {
    // Create two separate handler instances
    const handler1 = await createIssueModule.main({ /* config */ });
    const handler2 = await createIssueModule.main({ /* config */ });
    
    // Process messages with both handlers
    await handler1.handler({ title: "Issue 1", body: "Test 1" });
    await handler2.handler({ title: "Issue 2", body: "Test 2" });
    await handler1.handler({ title: "Issue 3", body: "Test 3" });
    
    // Verify each handler has isolated state
    const issues1 = handler1.getIssuesToAssignCopilot();
    const issues2 = handler2.getIssuesToAssignCopilot();
    
    expect(issues1).toHaveLength(2);  // handler1 processed 2 messages
    expect(issues2).toHaveLength(1);  // handler2 processed 1 message
    expect(issues1).not.toEqual(issues2);  // No state leakage
  });
  
  test('handlers can run without reset between tests', () => {
    // With proper isolation, no manual reset needed
    const handler1 = await createIssueModule.main({ /* config */ });
    // State is fresh for each handler instance
    expect(handler1.getIssuesToAssignCopilot()).toEqual([]);
  });
});

References

• JavaScript Concurrency Model: Node.js event loop is single-threaded, but module-level state persists across invocations
• Best Practice: Node.js Best Practices - Avoid Mutable Global State
• Pattern: Closure-based state isolation for handler factories
• Related Code: Similar pattern should be checked in other handlers (assign_to_agent, create_agent_session, etc.)

---

Priority: P0-Critical
Effort: Medium (requires handler manager integration updates)
Expected Impact: Eliminates race condition risk, improves testability, removes global state anti-pattern

AI generated by Daily MCP Tool Concurrency Analysis

• expires on Feb 19, 2026, 9:22 AM UTC