[concurrency] Concurrency safety: assign_to_agent module-level state + unserialized MCP stdio dispatch

[github-actions]

Summary

assign_to_agent.cjs stores handler results in a module-level mutable array (_allResults) instead of closure-local state, and its handleMessage message-processor has an interleavable read-modify-write section (processedCount check → await sleep(10000) → mutation) that is not protected against concurrent invocation. Combined with the MCP server's stdio dispatch loop (mcp_server_core.cjs), which does not serialize concurrent tools/call requests if the client pipelines multiple JSON-RPC messages before awaiting responses, this creates a real risk of result-array corruption, incorrect max enforcement, and cross-invocation data bleed between unrelated handler runs.

Key highlights

• Module-level let _allResults = [] shared across all assign_to_agent message invocations and read by exported getters (getAssignToAgentAssigned, getAssignToAgentErrors, getAssignToAgentErrorCount, writeAssignToAgentSummary).
• _allResults = [] reset only happens once in main() per handler-module load, not per message — safe only if messages are processed strictly sequentially.
• processedCount max-count enforcement (assign_to_agent.cjs:171-176) plus the await sleep(10000) at line 179-182 creates a window where two concurrently-dispatched messages could both pass the processedCount >= maxCount check before either increments processedCount (line 334), allowing more than maxCount assignments to succeed.
• The MCP stdio server (mcp_server_core.cjs:1046-1054) registers process.stdin.on("data", onData) where onData is async but not awaited and not queued — if stdin delivers multiple data events before a prior onData invocation finishes (e.g. client pipelines requests), processReadBuffer can execute concurrently from multiple invocations, each looping over the shared server.readBuffer and invoking handleMessage/tool handlers without any mutual exclusion.

Issue Details

Type: Global State / Missing Synchronization / TOCTOU

Location: actions/setup/js/assign_to_agent.cjs:21,136,171-182,334,494-545; contributing dispatch issue in actions/setup/js/mcp_server_core.cjs:1013-1054

Code Pattern (module-level shared state):

// assign_to_agent.cjs
let _allResults = [];   // module scope — shared by ALL invocations of this loaded module

async function main(config = {}) {
  // ...
  _allResults = [];     // reset once per handler-module load, not per message
  return async function handleMessage(message, resolvedTemporaryIds, temporaryIdMap) {
    if (processedCount >= maxCount) {              // check
      _allResults.push({ ... skipped: true });
      return { success: false, skipped: true };
    }
    if (processedCount > 0) {
      await sleep(10000);                          // <-- yields control; TOCTOU window
    }
    // ... several more awaits (repo resolution, API calls) ...
    processedCount++;                              // increment happens much later
    // ...
    _allResults.push({ ... success: true });        // mutate shared array
  };
}

function getAssignToAgentAssigned() {
  return _allResults.filter(r => r.success && !r.skipped)...;  // read shared array
}

Race Condition Scenario:

1. The MCP server receives two assign_to_agent tool calls in quick succession over stdio (client pipelines two tools/call JSON-RPC requests without waiting for the first response).
2. process.stdin emits two data events; onData (not awaited, not queued) fires twice, and both invocations of processReadBuffer/handleMessage run interleaved on the Node.js event loop.
3. Both handleMessage calls read processedCount (e.g. 0) before either increments it, both pass the processedCount >= maxCount guard for maxCount = 1.
4. Both proceed to call assignAgentToIssue and both push entries into the same shared _allResults array — resulting in 2 successful assignments even though max: 1 was configured, and both entries co-mingled in the final summary/output computed by getAssignToAgentAssigned() / writeAssignToAgentSummary().
5. Result: max-count enforcement is bypassed, and _allResults accumulates entries out of order / from concurrent runs, corrupting the reported summary output (assign_to_agent_assigned / assign_to_agent_assignment_errors GitHub Action outputs).

Detailed Analysis

Root Cause

_allResults is declared as a module-level let specifically to let the handler-manager read the finalized results after all messages are processed (per the code comment at line 16-18). This design assumes:

1. All assign_to_agent messages for a given handler-module instance are processed strictly sequentially (never concurrently), and
2. The module is not reused across genuinely independent invocation contexts without an explicit reset.

Neither assumption is enforced by the surrounding infrastructure:

• safe_output_handler_manager.cjs does process messages in a for loop with await messageHandler(...) per iteration (this part is fine — sequential by construction there).
• However, the raw MCP tool-call dispatch path (mcp_server_core.cjs) that a client talks to directly is not guaranteed to serialize concurrent tools/call invocations, since onData is an unawaited async callback registered on 'data'. If the underlying transport ever delivers overlapping chunks that each yield one or more complete JSON-RPC messages before the previous chunk's processing finishes (e.g., due to slow await operations inside a handler like the 10-second sleep, network calls, or the event loop scheduling multiple data events before microtasks resolve), two tools/call handlers can run concurrently.
• Even if today's specific client behavior always waits for a response before sending the next request, this is not enforced/documented as an invariant, and the code has no defensive locking. A future client change, retry-with-overlap, or a bug in the calling harness would silently break max enforcement and corrupt the shared results without any error being raised.

Concurrent Execution Example

// Timeline of two concurrent assign_to_agent tool calls with max: 1
// T=0ms:   Call A reads processedCount (0), passes maxCount check
// T=1ms:   Call B reads processedCount (0), passes maxCount check (should have been blocked!)
// T=2ms:   Call A calls sleep(0) [processedCount was 0, so no sleep] -> proceeds
// T=5ms:   Call B proceeds similarly
// T=50ms:  Call A increments processedCount -> 1, pushes success result to _allResults
// T=55ms:  Call B increments processedCount -> 2, pushes success result to _allResults
// Result: 2 agents assigned despite max: 1, and _allResults has both entries mixed together

Impact Assessment

• Data Integrity: _allResults can contain entries from what should be mutually-exclusive invocations, producing incorrect assign_to_agent_assigned/assign_to_agent_assignment_errors outputs and incorrect step summaries.
• Reliability: The max count safety limit (a documented safeguard against runaway agent assignment / cost control) can be silently bypassed under concurrent dispatch.
• Security: Low direct security impact, but bypassing max enforcement could allow more agent-assignment side effects (e.g., PR/issue mutations, external agent invocations) than the workflow author intended, which is a control-bypass concern for cost/quota and blast-radius limits.

Recommended Fix

Approach: State isolation + explicit serialization of the underlying tool-call dispatch loop.

// ✅ SAFE: Move _allResults into closure scope, created fresh per main() call,
// and expose accessor functions that close over it via a returned handler bundle
// instead of relying on module-level state read by unrelated top-level exports.

async function main(config = {}) {
  const state = { allResults: [], processedCount: 0 };
  // ...
  const handleMessage = async (message, resolvedTemporaryIds, temporaryIdMap) => {
    // Use an atomic check-and-increment instead of check-then-later-increment:
    if (state.processedCount >= maxCount) {
      state.allResults.push({ ...skippedEntry });
      return { success: false, skipped: true };
    }
    state.processedCount++;   // reserve the slot BEFORE any await, closing the TOCTOU window
    if (state.processedCount > 1) {
      await sleep(10000);
    }
    // ... rest of logic uses state.allResults.push(...) ...
  };
  handleMessage.getAssigned = () => computeAssigned(state.allResults);
  handleMessage.getErrors = () => computeErrors(state.allResults);
  handleMessage.getErrorCount = () => computeErrorCount(state.allResults);
  handleMessage.writeSummary = () => writeSummary(state.allResults);
  return handleMessage;
}

And separately, harden the MCP stdio dispatch loop so tools/call requests are processed strictly one-at-a-time even if the transport delivers overlapping data events:

// ✅ SAFE: Serialize onData invocations with a promise chain / mutex
let processingChain = Promise.resolve();
const onData = chunk => {
  server.readBuffer.append(chunk);
  processingChain = processingChain.then(() => processReadBuffer(server, defaultHandler)).catch(err => server.debug(`processReadBuffer error: ${err}`));
};

Explanation: Moving _allResults/processedCount into a closure created per main() invocation eliminates any possibility of unrelated invocations sharing mutable state, and reserving the processedCount slot before the first await closes the TOCTOU race window entirely (classic "increment before yield" pattern). Serializing onData with a promise chain guarantees handleMessage/tool-call dispatch never overlaps regardless of how stdin delivers chunks, which is the correct invariant for the current design where handlers assume sequential processing.

Implementation Steps:

1. Refactor assign_to_agent.cjs to remove the module-level _allResults and attach a per-call results container to the returned handleMessage function (or return an object bundling the handler with its own getters), updating safe_output_handler_manager.cjs's require(...) destructuring accordingly.
2. Move the processedCount++ guard to occur atomically with the processedCount >= maxCount check (no await in between).
3. Add a lightweight promise-chain-based serialization to onData in mcp_server_core.cjs so processReadBuffer invocations never run concurrently, independent of how assign_to_agent.cjs (or other handlers) are refactored.
4. Add regression tests exercising concurrent handleMessage invocations (see Testing Strategy) to lock in the fix.

Alternative Solutions

Option 1: Keep module-level state but add a mutex

• Pros: Smaller code diff.
• Cons: Adds unnecessary lock complexity to a case (assign_to_agent processing) that can be trivially made stateless per invocation via closures — locks are a code smell here rather than the right tool.

Option 2: Only fix the stdio dispatch serialization, leave assign_to_agent.cjs module-level state as-is

• Pros: Fixes the root cause of concurrent dispatch broadly for all tools, not just this one.
• Cons: Does not remove the fragile module-level-state pattern in assign_to_agent.cjs, which remains a landmine if any future code path (tests, alternate entry points, batch processing) invokes the module concurrently outside the MCP server's dispatch loop.

Recommendation: apply both fixes — they address different layers of the same risk.

Testing Strategy

To verify the fix:

// Test concurrent execution of assign_to_agent's message handler
describe('assign_to_agent concurrency safety', () => {
  test('max count enforcement holds under concurrent invocation', async () => {
    const handleMessage = await main({ max: "1", name: "copilot" });
    // Launch 5 concurrent calls with distinct issue numbers
    const promises = [1, 2, 3, 4, 5].map(n =>
      handleMessage({ issue_number: n, agent: "copilot" }, {}, new Map())
    );
    const results = await Promise.all(promises);
    const successes = results.filter(r => r.success && !r.skipped);
    // Only ONE assignment should succeed regardless of concurrency
    expect(successes.length).toBeLessThanOrEqual(1);
  });

  test('two independent main() invocations do not share _allResults', async () => {
    const handlerA = await main({ max: "5" });
    const handlerB = await main({ max: "5" });
    await handlerA({ issue_number: 1, agent: "copilot" }, {}, new Map());
    // handlerB's results must not include handlerA's entry
    expect(getAssignToAgentAssigned()).not.toContain("issue:1:copilot"); // fails today due to shared module state
  });
});

References

• JavaScript Concurrency Model: Node.js event loop — async functions registered as event-emitter callbacks (e.g. stream.on('data', asyncFn)) are NOT automatically serialized; each invocation starts independently and can interleave via microtask/macrotask scheduling.
• Classic TOCTOU pattern: check-then-act across an await boundary without reserving state first.
• Related file: actions/setup/js/mcp_server_core.cjs (start() / onData / processReadBuffer) — underlying dispatch loop referenced above.

---

Priority: P1-High
Effort: Medium
Expected Impact: Prevents max-count bypass and result-array corruption for assign_to_agent, and closes a systemic concurrent-dispatch gap in the safe-outputs MCP server's stdio transport that could affect other stateful handlers in the future.

Generated by 📊 Daily MCP Tool Concurrency Analysis · auto · 98.9 AIC · ⌖ 5.29 AIC · ⊞ 11.6K · ◷

• expires on Aug 18, 2026, 1:38 AM UTC-08:00

[github-actions]

🍪 Issue Monster selected this for Copilot

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! 🍪

🍪 Om nom nom by Issue Monster · gpt54 · 2.76 AIC · ⌖ 8.72 AIC · ⊞ 11.4K · ◷