[concurrency] Concurrency Safety Issue in create_agent_session tool: module-level mutable state

[github-actions]

Concurrency Safety Issue in create_agent_session

Severity: MEDIUM
Tool: create_agent_session
File: actions/setup/js/create_agent_session.cjs
Analysis Date: 2026-08-14

Summary

create_agent_session.cjs keeps its per-run results in a module-level mutable variable (let _allResults = [], line 16) instead of closure-scoped state returned/held by main(). Because Node.js require() caches modules as singletons, this array is shared across every invocation of main() within the same process for the lifetime of the safe-outputs handler process. Every other MCP-server-related handler inspected (add_comment.cjs, push_repo_memory.cjs, mcp_server_core.cjs, safe_outputs_mcp_server.cjs, safe_outputs_append.cjs) uses purely closure-local or parameter-local state, with no comparable module-level mutable array.

Issue Details

Type: Global/Module-Level State

Location: actions/setup/js/create_agent_session.cjs:16 (declaration), :52 (reset in main()), :76,85,129,143 (mutation in handleMessage), :154,163,173-176 (read by exported getters)

Code Pattern:

// Module scope
let _allResults = [];

async function main(config = {}) {
  // Reset module-level state for this run
  _allResults = [];
  ...
  return async function handleMessage(message) {
    ...
    _allResults.push({ id: taskId, url: taskUrl, success: true });
    ...
  };
}

function getCreateAgentSessionNumber() {
  const first = _allResults.find(r => r.success && r.id);
  return first ? first.id : "";
}

Race Condition Scenario (currently latent, not yet triggered by production code):

1. safe_output_handler_manager.cjs currently calls handlerModule.main(config) once per handler type and then processes messages sequentially in a for loop (processMessages, ~line 786), awaiting each messageHandler(...) call before moving to the next message. Under today's sequential-await architecture this specific handler is safe in practice.
2. However, the state lives at module scope rather than being returned/closed-over per invocation. If main() is ever invoked twice in the same process — e.g. a future feature that processes multiple safe-output batches/workflow runs in one long-lived process, parallel/Promise.all-based message dispatch, or unit tests that call main() twice without module cache reset between assertions — the second main() call's _allResults = [] reset will wipe out results still being read by the first run's getCreateAgentSessionNumber() / getCreateAgentSessionUrl() / writeCreateAgentSessionSummary(), or interleaved .push() calls will merge unrelated session results into one array.
3. Result: session_number/session_url outputs and the step summary could reflect the wrong run's agent-session results (data corruption / cross-run contamination), or silently drop results from an earlier concurrent run.

Detailed Analysis

Root Cause

JavaScript module caching means let _allResults = [] is a singleton mutable object for the process lifetime, not per-call state. The code comment at lines 11-13 explicitly justifies this as "module-level variables (rather than closure-only state) allows the handler manager to read final output values after all messages have been processed" — but the same effect can be achieved without global mutable state by having main() return an object exposing handleMessage, getNumber, getUrl, and writeSummary bound to a closure-local array, and having the handler manager call those returned accessors instead of importing module-level functions.

Concurrent Execution Example

// Timeline if main() were ever invoked twice in the same process:
// T=0ms:  Run A calls main() -> _allResults = []
// T=1ms:  Run A's handleMessage() pushes session A1 -> _allResults = [A1]
// T=2ms:  Run B calls main() -> _allResults = []  // Run A's A1 result is WIPED
// T=3ms:  Run B's handleMessage() pushes session B1 -> _allResults = [B1]
// T=4ms:  Run A calls getCreateAgentSessionNumber() -> returns B1's id, not A1's!

Impact Assessment

• Data Integrity: Wrong session_number / session_url step outputs could be reported for a given run, and the step summary could mix or drop agent-session results from different runs.
• Reliability: Any future refactor toward parallel handler execution (a natural optimization for I/O-bound safe-output processing) would silently reintroduce this bug without an obvious test failure, since single-threaded sequential tests would still pass.
• Security: No direct security impact identified (no cross-tenant/cross-repo secret exposure), but cross-run data corruption in agent session identifiers could misdirect a session_number/session_url consumer (e.g., a step that comments a link) to the wrong created task.

Recommended Fix

Approach: State isolation — move _allResults into a per-invocation closure and return the accessor functions from main() instead of exporting module-level getters.

// ✅ SAFE: per-invocation closure state
async function main(config = {}) {
  /** `@type` {Array<{id: string, url: string, success: boolean, error?: string}>} */
  const allResults = [];

  // ... existing setup ...

  const handleMessage = async message => {
    // ... use allResults.push(...) instead of _allResults.push(...) ...
  };

  handleMessage.getSessionNumber = () => {
    const first = allResults.find(r => r.success && r.id);
    return first ? first.id : "";
  };
  handleMessage.getSessionUrl = () => {
    const first = allResults.find(r => r.success && r.url);
    return first ? first.url : "";
  };
  handleMessage.writeSummary = async () => { /* build from allResults */ };

  return handleMessage;
}

module.exports = { main };

The handler manager would then call messageHandler.getSessionNumber(), messageHandler.getSessionUrl(), and await messageHandler.writeSummary() on the specific handleMessage instance returned for that run, instead of importing separate module-level functions that read shared global state.

Explanation: Attaching the accessor functions to the returned handleMessage function closes over a const allResults array created fresh per main() call, eliminating the shared mutable module-level variable entirely while preserving the existing "read final output after all messages processed" capability described in the original code comment.

Implementation Steps:

1. Convert let _allResults = [] (module scope) into const allResults = [] declared inside main().
2. Replace all _allResults.* references inside handleMessage with allResults.*.
3. Attach getSessionNumber, getSessionUrl, and writeSummary as properties on the returned handleMessage function (or return { handleMessage, getSessionNumber, getSessionUrl, writeSummary } and update safe_output_handler_manager.cjs's call sites accordingly).
4. Update module.exports and the two import/call sites in safe_output_handler_manager.cjs (where it require("./create_agent_session.cjs") and later invokes getCreateAgentSessionNumber() / getCreateAgentSessionUrl() / writeCreateAgentSessionSummary()).
5. Update/extend create_agent_session.test.cjs to add a concurrency regression test (two independent main() invocations processed "concurrently" must not cross-contaminate results).

Alternative Solutions

Option 1: Keep module-level state but key it by a run ID

• Pros: Smaller code diff.
• Cons: Still shared mutable state; requires plumbing a run ID through every call; more complex than eliminating the shared state.

Option 2: Leave as-is, add explicit non-reentrancy documentation/guard

• Pros: No functional change; add an assertion that main() is never called twice per process.
• Cons: Doesn't fix the root cause; brittle to future refactors; violates the "safe by construction" goal of the analysis.

Testing Strategy

To verify the fix, add a test where two independent main() invocations are created and one is exercised while checking that the other's accessor state is unaffected (no shared _allResults cross-contamination). Use import() to get fresh handler instances and assert independent result arrays / getter outputs.

References

• JavaScript Concurrency Model: Node.js module caching (require() singleton semantics) and the single-threaded event loop with interleaved async execution.
• Node.js Best Practices: Prefer closures over module-level mutable state for per-invocation data.
• Related Files Verified Clean: actions/setup/js/add_comment.cjs, actions/setup/js/push_repo_memory.cjs, actions/setup/js/mcp_server_core.cjs, actions/setup/js/safe_outputs_mcp_server.cjs, actions/setup/js/safe_outputs_append.cjs (no module-level mutable state found in any of these).

---

Priority: P2-Medium
Effort: Small
Expected Impact: Prevents cross-run data corruption of agent-session results/outputs/summary if handler invocation is ever parallelized or main() is called more than once per process; improves code correctness and testability today.

Generated by 📊 Daily MCP Tool Concurrency Analysis · auto · 71.9 AIC · ⌖ 5.25 AIC · ⊞ 11.4K · ◷

• expires on Aug 21, 2026, 1:37 AM UTC-08:00

[github-actions]

🍪 Issue Monster selected this for Copilot

I identified this as a strong candidate for automated resolution and requested assignment to the Copilot coding agent.

This topic appears independent from the other issues selected in this run, so it should be safe to work in parallel.

Om nom nom! 🍪

🍪 Om nom nom by Issue Monster · gpt54 · 2.85 AIC · ⌖ 8.44 AIC · ⊞ 12.7K · ◷