Skip to content

demo(06-concurrency-race): 06 — Concurrency: lock removed from shared counter increment#155

Open
EricCogen wants to merge 1 commit intomainfrom
demo/06-concurrency-race
Open

demo(06-concurrency-race): 06 — Concurrency: lock removed from shared counter increment#155
EricCogen wants to merge 1 commit intomainfrom
demo/06-concurrency-race

Conversation

@EricCogen
Copy link
Copy Markdown
Owner

06 — Concurrency: lock removed from shared counter increment

Expected verdict: ❌ Fails — GauntletCI should fire GCI0016 (concurrency / non-atomic shared state).

What changed

OrderProcessor gained a static long _processedCount for a metrics
rollout, exposed via ProcessedCount. Inside ProcessAsync the counter
is incremented with the bare ++ operator:

private static long _processedCount;
public static long ProcessedCount => _processedCount;

// inside ProcessAsync:
_processedCount++;

Why this is risky

  • OrderProcessor is registered as scoped in DI but _processedCount
    is static — every concurrent HTTP request races on it.
  • long++ on a 32-bit ABI is two non-atomic operations. Reads from
    another thread can observe torn values.
  • Updates can be lost: under load the metric will under-count, sometimes
    by a lot. The bug looks like "instrumentation drift" instead of a race.

The fix is one of Interlocked.Increment(ref _processedCount), a
lock, or a proper metrics primitive (Counter<long>).

What GauntletCI catches

GCI0016 Concurrency / non-atomic shared state — a static mutable
field is being mutated without synchronisation in a code path reachable
from concurrent request handling.

… counter increment

See scenarios/06-concurrency-race/README.md for the expected verdict.
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GauntletCI found the following issues:

These findings reference lines outside the PR diff, so they appear here instead of inline. Expand each entry for full evidence, rationale, and suggested action.

GCI0016 — Concurrency and State Risk: Static mutable field detected — potential shared state without synchronization.

GCI0016 — Concurrency and State Risk

Static mutable field detected — potential shared state without synchronization.

Evidence:

Line 12: private static long _processedCount;
Line 34: public static long ProcessedCount => _processedCount;

⚠️ Why it matters: Mutable static fields are shared across all threads and requests, requiring explicit synchronization to avoid race conditions.

💡 Suggested action: Make the field readonly, use Interlocked for simple counters, or prefer instance fields with DI.

Confidence: Medium | Severity: Block | 2 occurrences

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 48b91d4c4f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

}

await _repo.UpdateAsync(order, ct).ConfigureAwait(false);
_processedCount++;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use atomic increment for shared processed counter

ProcessAsync is executed for concurrent HTTP requests, but this line updates a static field with _processedCount++ and no synchronization. That operation is a non-atomic read/modify/write, so simultaneous requests can lose increments and produce an incorrect processed-order metric (and long access can tear on 32-bit runtimes). Use Interlocked.Increment(ref _processedCount) (and an atomic read strategy) or protect updates with a lock.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant