[concurrency] Concurrency Safety Issue in `create_issue` - processedCount TOCTOU and dead module-level mutable state

[github-actions]

Concurrency Safety Issue in create_issue

Severity: MEDIUM
Tool: create_issue
File: actions/setup/js/create_issue.cjs
Analysis Date: 2026-04-21

Summary

The create_issue handler has two concurrency issues: (1) a module-level mutable array issuesToAssignCopilotGlobal that is dead production code but adds mutation risk, and (2) a time-of-check vs time-of-use (TOCTOU) race condition on processedCount that could allow the configured max issue limit to be exceeded under concurrent invocations.

---

Issue 1: Module-Level Mutable State (LOW)

Type: Global/Module-Level Mutable State
Location: actions/setup/js/create_issue.cjs:13

Code Pattern:

// ❌ Module-level mutable state — never populated in production
let issuesToAssignCopilotGlobal = [];

function getIssuesToAssignCopilot() {
  return [...issuesToAssignCopilotGlobal]; // defensive copy (good)
}

function resetIssuesToAssignCopilot() {
  issuesToAssignCopilotGlobal.length = 0; // in-place mutation (risky)
}

Race Condition Scenario (in test environments):

1. Test A calls getIssuesToAssignCopilot() → receives a snapshot []
2. Test B calls resetIssuesToAssignCopilot() → mutates array in-place via .length = 0
3. Any test holding a reference to the live array (not the snapshot) sees it emptied unexpectedly

Notes:

• The comment in the file acknowledges this is dead code: "this list is never populated during normal operation"
• getIssuesToAssignCopilot() correctly returns a defensive copy
• resetIssuesToAssignCopilot() uses in-place mutation (array.length = 0) which affects any caller that retained a reference to the original array rather than a copy
• Risk in production: none (variable never written to); risk in concurrent test scenarios: low but present

Recommended Fix: Remove the dead module-level state entirely, or convert to a factory pattern that isolates state per invocation. At minimum, simplify resetIssuesToAssignCopilot() to reassign rather than mutate in-place.

// ✅ SAFE: reassignment is clear and non-destructive to existing snapshots
function resetIssuesToAssignCopilot() {
  issuesToAssignCopilotGlobal = []; // new array, does not affect existing snapshots
}

---

Issue 2: processedCount TOCTOU Race Condition (MEDIUM)

Type: Time-of-Check vs Time-of-Use
Location: actions/setup/js/create_issue.cjs:326 (check) and :573 (increment)

Code Pattern:

// In main() closure — shared across all handleCreateIssue calls:
let processedCount = 0;  // Line 301

return async function handleCreateIssue(message, resolvedTemporaryIds) {
  // CHECK — line 326
  if (processedCount >= maxCount) {
    core.warning(`Skipping create_issue: max count of \$\{maxCount} reached`);
    return { success: false, error: `Max count of \$\{maxCount} reached` };
  }

  // ... many async await operations (API calls with retries, delays up to 45s) ...

  // INCREMENT — line 573 (much later)
  processedCount++;
  // ... proceed to create issue ...
};

Race Condition Scenario (if two concurrent calls to handleCreateIssue occur):

T=0ms:  Call A reads processedCount=0, 0 < maxCount=1, passes check
T=1ms:  Call B reads processedCount=0, 0 < maxCount=1, passes check (A is awaiting API)
T=200ms: Call A increments processedCount → 1, creates issue #1
T=201ms: Call B increments processedCount → 2, creates issue #2
Result: 2 issues created despite maxCount=1 ❌

Impact Assessment:

• Data Integrity: More GitHub issues created than the operator configured with max
• Reliability: Non-deterministic behavior depending on API response times
• Severity in practice: Low-Medium — the safe-outputs system currently replays tool calls sequentially, so concurrent invocations are uncommon. However, the HTTP transport mode (mcp_http_transport.cjs) can receive concurrent requests, making this exploitable in gateway scenarios.

Detailed Analysis

Root Cause

The handleCreateIssue function is a closure over the main() function's processedCount variable. The check and the increment are separated by dozens of async operations (GitHub API calls, retry delays of 15–45 seconds). In Node.js, the single-threaded event loop means true parallelism is impossible, but concurrent async tasks CAN interleave at every await point.

The same pattern affects copilotClient (lazy initialization, line 262 in the closure):

let copilotClient = null;
// ...
if (!copilotClient) {
  copilotClient = await createCopilotAssignmentClient(config); // yields!
}
// Two concurrent calls can both reach this null check before either completes init

Other Shared Closure State

These variables in the main() closure are also shared across concurrent handleCreateIssue invocations:

• createdIssues array — mutated via .push()
• temporaryIdMap Map — mutated via .set()
• parentIssueCache Map — mutated via .set()

These are less severe because Map/array mutations are synchronous (no gaps between check and update), but cross-invocation state sharing remains architecturally unsound if future changes introduce async gaps in those code paths.

Recommended Fix

Approach: Move the max-count check and increment into an atomic synchronous block, or use a pre-allocated slot pattern.

// ✅ SAFE: Atomically reserve a slot before any async work
return async function handleCreateIssue(message, resolvedTemporaryIds) {
  // Atomically claim a slot (synchronous — no await between check and increment)
  if (processedCount >= maxCount) {
    core.warning(`Skipping create_issue: max count of \$\{maxCount} reached`);
    return { success: false, error: `Max count of \$\{maxCount} reached` };
  }
  processedCount++; // Reserve slot BEFORE any async work
  
  // Now proceed with async operations — slot is already reserved
  // ... API calls, retries, etc. ...
};

Implementation Steps:

1. Move processedCount++ to immediately after the max-count guard check (before any await)
2. If the issue creation subsequently fails, optionally decrement processedCount to free the slot (though this adds complexity and may not be needed given sequential execution in practice)
3. Remove or simplify the issuesToAssignCopilotGlobal dead code

---

Testing Strategy

describe('create_issue concurrency safety', () => {
  test('respects maxCount under concurrent calls', async () => {
    const handler = await main({ max: 1 });
    
    // Launch two concurrent calls
    const [result1, result2] = await Promise.all([
      handler({ title: 'Issue 1', body: 'body' }, {}),
      handler({ title: 'Issue 2', body: 'body' }, {}),
    ]);
    
    const successes = [result1, result2].filter(r => r.success);
    // Only ONE should succeed with maxCount=1
    expect(successes).toHaveLength(1);
  });
});

---

Priority: P2-Medium
Effort: Small (fix for Issue 2 is a one-line change; Issue 1 is a small refactor)
Expected Impact: Ensures max issue creation limits are correctly enforced under concurrent tool invocations

Generated by Daily MCP Tool Concurrency Analysis · ● 3.5M · ◷

• expires on Apr 28, 2026, 9:55 AM UTC

[github-actions]

🍪 *Issue Monster selected this for Copilotecho BEGIN___COMMAND_OUTPUT_MARKER ; PS1= ; PS2= ; unset HISTFILE ; EC=0 ; echo ___BEGIN___COMMAND_DONE_MARKER___0 ; }

I've identified this issue as a good candidate for automated resolution and requested assignment to the Copilot coding agent.

If assignment succeeds, the Copilot coding agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

Caution

Security scanning requires review for Issue Monster

Details

Potential security threats were detected in the agent output. The workflow output should be reviewed before merging.

Review the workflow run logs for details.

🍪 Om nom nom by Issue Monster · ● 15.4K · ◷