Skip to content

Preserve Windows task results#13

Merged
loreste merged 1 commit into
loreste:mainfrom
attahn:fix/windows-task-join-results
Jul 22, 2026
Merged

Preserve Windows task results#13
loreste merged 1 commit into
loreste:mainfrom
attahn:fix/windows-task-join-results

Conversation

@attahn

@attahn attahn commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Windows task joins were replacing completed task results with NULL because the native pthread shim cannot return a thread result.

This keeps the result on MakoTask, documents its publication contract, and makes six task and timed-join suites blocking on Windows.

Verified with 78 Rust tests, focused Windows and WSL/Clang runs, and the full Windows suite improving from 304 passed / 59 failed to 334 passed / 29 failed.

Summary by CodeRabbit

  • Bug Fixes

    • Preserved task return values when joining native Windows worker threads.
    • Improved task awaiting behavior on Windows, including timed waits.
  • Tests

    • Expanded Windows runtime test coverage for actors, worker tasks, typed joins, function calls, and wave queues.
  • Documentation

    • Updated the unreleased changelog with the Windows task return value fix.

@attahn

attahn commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai[bot] review

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The runtime adds a join wrapper that prevents native Windows thread joins from overwriting stored task results. Await paths use the wrapper, the task result contract is documented, Windows CI runs additional tests, and the changelog records the behavior.

Changes

Windows task result preservation

Layer / File(s) Summary
Join result handling
runtime/mako_rt.h
The task result is documented as read-only after completion, and mako_task_join_thread joins threads without storing the pthread return value.
Await integration and validation
runtime/mako_rt.h, .github/workflows/ci.yml, CHANGELOG.md
Non-pooled await paths use the new join wrapper; additional Windows test programs run in CI, and the runtime behavior is documented in the changelog.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: loreste

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.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
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: preserving task results on Windows during task joins.
✨ 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

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

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

@attahn: I’ll review the changes in #13.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@attahn

attahn commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

CI is green except for the strict 1.5x benchmark gate. This run measured slice100k at 1.70x and map50k at 2.31x; the preceding green run measured the same cases at 1.08x and 0.95x. This change does not touch benchmarked code, so the failed job should pass on rerun.

@attahn
attahn marked this pull request as ready for review July 21, 2026 21:37
@loreste
loreste merged commit 50763ef into loreste:main Jul 22, 2026
11 of 12 checks passed
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.

2 participants