Fix threadpool futexes_linux wait/wake syscall number

[nitely]

The NR_Futex value differs per arch, for example i386 is 240, not 202 (where in this case makes it return immediately instead of wait).

Added regression tests for futexes and backoff.

Summary by CodeRabbit

• Bug Fixes

  • Improved Linux futex behavior by using the system’s SYS_futex syscall definition instead of a hardcoded value.

• Tests

  • Added new threadpool tests for backoff/event counting: cancellation semantics, stale-ticket handling, wake()/wakeAll(), and supporting more than 256 committed waiters.
  • Added futex tests for wait() (including immediate-return) and wakeAll() (broadcast release verification).
  • Included both new threadpool test scripts in the test suite.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds Linux futex syscall portability and introduces threadpool tests for futex waiting and EventCount parking, wakeup, broadcast, cancellation, and high waiter counts.

Changes

Threadpool synchronization

Layer / File(s)	Summary
Portable Linux futex syscall binding constantine/threadpool/primitives/futexes_linux.nim	The futex syscall number is imported from the platform’s SYS_futex definition instead of using a hardcoded value.
Futex wait and broadcast validation tests/threadpool/t_futexes.nim	Tests cover parked waits, immediate returns for changed futex values, explicit wakeups, and wakeAll() across multiple waiters.
EventCount parking and waiter coverage tests/threadpool/t_backoff.nim, constantine.nimble	Tests cover cancellation, stale tickets, single and broadcast wakeups, and 257 committed waiters; both test scripts are added to the threadpool test task.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• mratsim/constantine#623: Both changes cover EventCount behavior around the 256+ waiter boundary.

Poem

I’m a rabbit with threads in a row,
Watching waiters park, wake, and go.
Futexes now know where to call,
EventCounts can rouse them all.
Two new tests hop into the run—
257? We’re not done!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: fixing the Linux futex syscall number used by the threadpool primitives.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR makes Linux futex calls use the target architecture's syscall number and adds tests for waiting behavior. The main changes are:

• Replace the hard-coded futex syscall number with SYS_futex.
• Add futex wait, wake, and wake-all tests.
• Add EventCount parking and waiter-count tests.
• Register the new tests in the threadpool suite.

Confidence Score: 4/5

The SYS_futex binding should be immutable before merging.

• Importing a preprocessor constant as var can generate an invalid external-variable binding.
• The same repository pattern already binds SYS_getrandom with let.
• The architecture-specific syscall selection and test coverage otherwise match the intended fix.

constantine/threadpool/primitives/futexes_linux.nim

Important Files Changed

Filename	Overview
constantine/threadpool/primitives/futexes_linux.nim	Uses the architecture-specific futex syscall constant, but imports the macro as mutable storage.
tests/threadpool/t_futexes.nim	Adds tests for futex parking, mismatched values, wake, and wake-all behavior.
tests/threadpool/t_backoff.nim	Adds EventCount tests for cancellation, stale tickets, parking, wake-all, and large waiter counts.
constantine.nimble	Registers the two new threadpool test files.

_{Reviews (1): Last reviewed commit: "Fix futexes_linux wait/wake syscall numb..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

tests/threadpool/t_backoff.nim (1)

119-157: 📐 Maintainability & Code Quality | 🔵 Trivial

Duplicate wakeAll test bodies for 4 vs. 257 waiters.

The two tests are structurally identical (busy-wait for committedSleep == numWaiters, set condition, wakeAll(), join, assert), differing only in thread count and container type (array vs seq). Consider extracting a shared helper parameterized by waiter count to avoid the duplication.
[recommended]

♻️ Proposed refactor

+proc runWakeAllTest(numWaiters: int) =
+  var s: WakeAllState
+  s.ec.initialize()
+
+  var threads = newSeq[Thread[ptr WakeAllState]](numWaiters)
+  for t in mitems(threads):
+    createThread(t, multiParker, addr s)
+
+  while s.ec.getNumWaiters().committedSleep != numWaiters:
+    discard
+
+  s.condition.store(true, moRelease)
+  s.ec.wakeAll()
+  joinThreads(threads)
+
+  check s.woke.load(moAcquire) == numWaiters
+  check s.ec.getNumWaiters().preSleep == 0
+  check s.ec.getNumWaiters().committedSleep == 0
+
 suite "EventCount":
   ...
   test "wakeAll() releases every parked waiter":
-    const numWaiters = 4
-    var s: WakeAllState
-    s.ec.initialize()
-
-    var threads: array[numWaiters, Thread[ptr WakeAllState]]
-    for t in mitems(threads):
-      createThread(t, multiParker, addr s)
-
-    while s.ec.getNumWaiters().committedSleep != numWaiters:
-      discard
-
-    s.condition.store(true, moRelease)
-    s.ec.wakeAll()
-    joinThreads(threads)
-
-    check s.woke.load(moAcquire) == numWaiters
-    check s.ec.getNumWaiters().preSleep == 0
-    check s.ec.getNumWaiters().committedSleep == 0
+    runWakeAllTest(4)

   test "supports more than 256 committed waiters":
-    const numWaiters = 257
-    var s: WakeAllState
-    s.ec.initialize()
-
-    var threads = newSeq[Thread[ptr WakeAllState]](numWaiters)
-    for t in mitems(threads):
-      createThread(t, multiParker, addr s)
-
-    while s.ec.getNumWaiters().committedSleep != numWaiters:
-      discard
-
-    s.condition.store(true, moRelease)
-    s.ec.wakeAll()
-    joinThreads(threads)
-
-    check s.woke.load(moAcquire) == numWaiters
-    check s.ec.getNumWaiters().preSleep == 0
-    check s.ec.getNumWaiters().committedSleep == 0
+    runWakeAllTest(257)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/threadpool/t_backoff.nim` around lines 119 - 157, Extract the
duplicated wakeAll scenario from the tests into a shared helper parameterized by
waiter count, including thread creation, waiter synchronization, condition
update, wakeAll invocation, joining, and final assertions. Update both tests to
call the helper with 4 and 257, while preserving support for each count and
avoiding separate array-versus-seq implementations.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/threadpool/t_futexes.nim`:
- Around line 35-38: Move the s.reachedWait.store readiness update from before
the futex-checking loop into the loop body immediately before s.futex.wait(0),
for both affected wait paths. Keep the existing acquire load, wait call, and
spin counter update unchanged so readiness is only signaled after the worker
reaches the blocking operation.
- Around line 70-79: Replace the mismatched-value test’s use of waiter with a
dedicated thread procedure that unconditionally calls s.futex.wait(0) and then
records completion in the shared WaitState. Start and join that procedure after
storing 1, and retain the completion check to verify wait returns immediately
when the value differs from the expected value.

---

Nitpick comments:
In `@tests/threadpool/t_backoff.nim`:
- Around line 119-157: Extract the duplicated wakeAll scenario from the tests
into a shared helper parameterized by waiter count, including thread creation,
waiter synchronization, condition update, wakeAll invocation, joining, and final
assertions. Update both tests to call the helper with 4 and 257, while
preserving support for each count and avoiding separate array-versus-seq
implementations.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e3443c83-9847-4d9d-ade2-207c3838865c

📥 Commits

Reviewing files that changed from the base of the PR and between ca0006c and 9adfbb2.

📒 Files selected for processing (4)

• constantine.nimble
• constantine/threadpool/primitives/futexes_linux.nim
• tests/threadpool/t_backoff.nim
• tests/threadpool/t_futexes.nim