Skip to content

fix(build): Prevent concurrent executions of core-build (fixes #2376).#2377

Open
Nathan903 wants to merge 9 commits into
y-scope:mainfrom
Nathan903:fix-task-file
Open

fix(build): Prevent concurrent executions of core-build (fixes #2376).#2377
Nathan903 wants to merge 9 commits into
y-scope:mainfrom
Nathan903:fix-task-file

Conversation

@Nathan903

@Nathan903 Nathan903 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes race condition of task tests:integration sporadically fails with linker errors (like undefined reference to 'main').

The core task previously invoked core-build using cmds instead of deps. Because of this, go-task could not deduplicate the build when multiple parallel test targets triggered the core task at the same time. This caused two parallel cmake --build processes to run simultaneously and corrupt each other's object files.

This PR

  • Converts the core task to use deps instead of cmds so the task runner can track it natively.
  • Adds core-generate as a dependency of core-build to maintain strict ordering.
  • Adds run: once to core-generate and core-build in taskfile.yaml, and to the core dependency task in taskfiles/deps/main.yaml, to guarantee they evaluate only once globally, regardless of differing environment configurations passed down by the parallel test wrappers.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Task now deduplicates the build step.

Previously:

task clean
task --dry tests:integration > /tmp/task-dry-run-check.log 2>&1
grep -c "utils:cmake:build.*/home/nathan/clp/build/core" /tmp/task-dry-run-check.log

gives

2

Now

task clean
task --dry tests:integration > /tmp/task-dry-run-check.log 2>&1
grep -c "utils:cmake:build.*/home/nathan/clp/build/core" /tmp/task-dry-run-check.log

gives

1

End to end test:

task tests:integration > /tmp/clp-build-verify.log 2>&1
grep -c "cmake --build .*/home/nathan/clp/build/core" /tmp/clp-build-verify.log

gives 1 (cmake --build only invoked once)

Summary by CodeRabbit

Summary by CodeRabbit

  • Refactor
    • Updated task execution to use explicit dependencies, ensuring generation runs before build.
    • Added single-run safeguards so related tasks execute only once per invocation.
  • Chores
    • Updated task configuration to run the core workflow in a more controlled, deterministic way.

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The core task flow now uses explicit task dependencies instead of inline commands, with once-only execution applied to the core task declarations.

Changes

Task dependency graph

Layer / File(s) Summary
Explicit core ordering
taskfile.yaml, taskfiles/deps/main.yaml
core now depends on core-build; core-build depends on core-generate; run: once is added to core-generate, core-build, and core.

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

Possibly related issues

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: preventing concurrent core-build executions.
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.
✨ 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.

@Nathan903 Nathan903 marked this pull request as ready for review July 9, 2026 20:53
@Nathan903 Nathan903 requested a review from a team as a code owner July 9, 2026 20:53
Comment thread taskfile.yaml Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

.

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@Nathan903 Sure, I'll re-review the entire PR from scratch.

✅ Action performed

Full review finished.


Your plan includes PR reviews subject to rate limits. More reviews will be available in 22 minutes.

@quinntaylormitchell quinntaylormitchell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think that a simpler way to fix this is to change might be to change line 8 of taskfiles/tests/integration.yaml from deps: to cmds:. iirc, dependencies run concurrently, and commands run sequentially.

@junhaoliao you will probably know which approach is best!

This reverts commit b2c5a71.

Revert "please work"

This reverts commit 507c595.

Revert "now works"

This reverts commit 97e3fcf.
@Nathan903

Nathan903 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

The core task previously invoked core-build using cmds instead of deps. Because of this, go-task could not deduplicate the build when multiple parallel test targets triggered the core task at the same time. This caused two parallel cmake --build processes to run simultaneously and corrupt each other's object files.

thx for the review. Yes, I just tested this much simpler fix (just 1 line change)

 tasks:
   default:
-    deps:
+    cmds:
       - task: "clp-py-project-imports"
       - task: "core"
       - task: "package"

and this fix works. However, removing parallelism makes building slower. I tested both version of the fixes and this simpler fix adds 35 seconds to the build time of task tests:integration.

What do you guys think about this tradeoff of speed and simplicity

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.

3 participants