fix(swarm): wait for job.Runs directly in scheduler skip test

[euxaristia]

Problem

TestSchedulerSkipsWhilePreviousRuns intermittently failed on CI (observed on macOS) with job runs=1 skipped=1, want 2/1.

fireIfIdle's Spawn call (which the launcher's recorded() count observes) and run's subsequent job.incRuns() are sequential but distinct steps in the same scheduler goroutine. The test polled only the launcher's recorded count via waitFor, then asserted job.Runs immediately afterward with no wait of its own. Under contention, the scheduler goroutine could be preempted between those two steps, so the test's very next read caught a stale Runs value.

Solution

Wait for job.Runs to actually reach 2 before asserting on it, instead of relying on the launcher's recorded count as a proxy for it.

Verified with go test ./internal/swarm/... -run TestSchedulerSkipsWhilePreviousRuns -count=50 -race, all passing.

Summary by CodeRabbit

• Tests
  • Improved scheduler test reliability by waiting for job execution counts to update before making assertions.
  • Added clarification around the sequence of scheduler events during test execution.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9143d4c6-9e49-4864-8da3-ae841d781c71

📥 Commits

Reviewing files that changed from the base of the PR and between fa3052a and 69e7e50.

📒 Files selected for processing (1)

• internal/swarm/scheduler_test.go

---

Walkthrough

The scheduler test now waits until the job’s Runs counter reaches 2 after the second tick, with comments distinguishing spawn observation from run-counter updates.

Changes

Scheduler test synchronization

Layer / File(s)	Summary
Wait for the second job run internal/swarm/scheduler_test.go	The test polls sched.List() until the job reports Runs == 2 after the second scheduler tick, and documents the sequential scheduler steps.

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

Suggested reviewers: gnanam1990

🚥 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 clearly matches the main change: the scheduler skip test now waits on job.Runs directly to avoid a race.
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

• X
• Mastodon
• Reddit
• LinkedIn

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

[jatmn]

Thanks for the contribution. I do not see any actionable issues from my review.

@Vasanthdev2004 LGTM

[Vasanthdev2004]

Verified the flake fix the diff adds a waitFor on job.Runs == 2 before asserting, instead of treating the launcher recording the spawn as a proxy for the run counter. That's the right fix for the race: the spawn and the subsequent incRuns() are sequential but distinct steps in the scheduler goroutine, so the old proxy read could catch a stale Runs value. Ran the test 50x under -race and it's green. Approving.

[gnanam1990]

Verdict: Approve

Test-only fix for a real CI flake (job runs=1 skipped=1, want 2/1 on macOS). The added waitFor on job.Runs == 2 is the right fix.

What looks good

• Root cause is correct: in run(), fireIfIdle() (launcher spawn) and job.incRuns() are sequential but distinct steps in the scheduler goroutine, so len(l.recorded()) == 2 is not a reliable proxy for Runs == 2.
• Matches the existing pattern in this test (already waits on j.Skipped == 1 before asserting).
• Comment explains the race without being noisy.
• CI green on all three OSes; @Vasanthdev2004 verified 50× under -race.

Local verification (merged latest main, no conflicts):

• make lint — pass
• go test -race -count=50 ./internal/swarm/... -run TestSchedulerSkipsWhilePreviousRuns — pass (50/50)

Nits

• No linked issue-approved parent issue per CONTRIBUTING — acceptable for a maintainer CI flake fix.

Issues

None.