less spinlock/UB

[PhilipDeegan]

Summary by CodeRabbit

• Bug Fixes

  • Enhanced thread-safety in asynchronous stream operations with atomic synchronization.
  • Improved work scheduling robustness by extending internal state management and refining selection logic to handle edge cases.

• Refactor

  • Removed unused internal definitions.

[coderabbitai]

📝 Walkthrough

Walkthrough

stream.hpp adds #include <atomic>, extends the SFS enum with a SKIP state, changes the host-group mutex retry path to set SFS::SKIP on failed lock attempts, updates get_work() to reset SKIP entries back to FIRST and continue scanning, and changes ThreadedStreamLauncher::done from bool to std::atomic<bool>. An unused struct A is removed from the async streaming test.

Changes

Stream Threading and State-Machine Fix

Layer / File(s)	Summary
SFS enum, SKIP scheduling, and atomic done inc/mkn/gpu/stream.hpp	Adds #include <atomic>, extends SFS with a SKIP state (FIRST=0, BUSY, WAIT, FIN, SKIP), changes the failed-mutex retry path to store SFS::SKIP instead of SFS::FIRST, adds a SFS::SKIP branch in get_work() that resets to FIRST and continues scanning, and changes the done member to std::atomic<bool>.
Remove unused test struct tst/any/async_streaming.cpp	Deletes the unreferenced local struct A definition immediately before the test() function.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 A mutex slipped, a flag was bare,
SKIP now signals "not my turn yet, dear!"
atomic<bool> for done — thread-safe and right,
struct A deleted, no ghost in the night.
Hop along streams, all synchronized tight! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'less spinlock/UB' directly relates to the main changes: replacing spinlock-prone busy-wait logic with a SKIP state mechanism and converting a plain bool to std::atomic to eliminate undefined behavior.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch next

---

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 and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

inc/mkn/gpu/stream.hpp (1)

321-330: ⚡ Quick win

Good fix for spinlock behavior.

Setting SFS::SKIP instead of SFS::FIRST prevents immediate retry within the same scan, reducing contention.

Note: There's a remaining data race between this write (outside work_ mutex) and reads in get_work() (inside mutex). The ownership model provides practical safety since only the "owning" thread (that set BUSY) modifies this slot, and other threads skip BUSY items. For full correctness, status elements would need to be atomic, but that's a larger change for a follow-up.

🤖 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 `@inc/mkn/gpu/stream.hpp` around lines 321 - 330, There is a data race between
the write to strat.status[i] in the run method (outside the work_ mutex) and
reads in get_work() (inside the mutex). To fix this, convert the status field or
its elements to use atomic types such as std::atomic<SFS> to ensure thread-safe
access to the status array across the work scheduling logic. This provides
proper synchronization for reads and writes without relying on external locking.

🤖 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.

Nitpick comments:
In `@inc/mkn/gpu/stream.hpp`:
- Around line 321-330: There is a data race between the write to strat.status[i]
in the run method (outside the work_ mutex) and reads in get_work() (inside the
mutex). To fix this, convert the status field or its elements to use atomic
types such as std::atomic<SFS> to ensure thread-safe access to the status array
across the work scheduling logic. This provides proper synchronization for reads
and writes without relying on external locking.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 53e94561-7bc6-4677-afd6-ff42655595f1

📥 Commits

Reviewing files that changed from the base of the PR and between f32caeb and dfcb849.

📒 Files selected for processing (2)

• inc/mkn/gpu/stream.hpp
• tst/any/async_streaming.cpp

💤 Files with no reviewable changes (1)

• tst/any/async_streaming.cpp