fix(FN-6035): align workflow dispatch and broad suite#1539
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryThis PR addresses a race condition in the scheduler's global semaphore accounting and modernizes the broad test suite for Vitest 4 compatibility. The concurrency fix ensures tasks already in
Confidence Score: 4/5The scheduler change is a targeted, well-reasoned fix with a direct regression test; the remaining 29 files are test-only Vitest 4 compatibility updates. The concurrency fix in scheduler.ts is logically sound: taking the minimum of the semaphore's own available-slot view and the remaining capacity computed from in-progress row count correctly prevents over-admission. The new test covers the precise race scenario described in the PR. The broad test migration is mechanical and low-risk. One assertion in heartbeat-monitor-per-agent-config.test.ts was weakened without explanation, which slightly reduces test fidelity on the warning-emit path. packages/engine/src/tests/heartbeat-monitor-per-agent-config.test.ts (weakened warn-call assertion) and packages/engine/src/tests/executor-task-done-invariant.test.ts (reason string change to workflow-graph-review — the corresponding production-side change is not visible in this diff). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["schedule() called"] --> B["agentSlots = inProgress.length"]
B --> C{"Semaphore provided?"}
C -- No --> D["semaphoreAvailable = Infinity"]
C -- Yes --> E["availableCount = limit minus active"]
E --> F["capacityByRows = limit minus agentSlots"]
F --> G["semaphoreAvailable = min of both"]
G --> H["available = min of maxConcurrent gap, worktree gap, semaphoreAvailable"]
D --> H
H --> I{"available <= 0?"}
I -- Yes --> J["Return early - no dispatch"]
I -- No --> K["Admit tasks from todo to in-progress"]
Reviews (1): Last reviewed commit: "fix(FN-6035): cap scheduler dispatch by ..." | Re-trigger Greptile |
| expect(config.pollIntervalMs).toBe(60_000); | ||
| expect(config.heartbeatTimeoutMs).toBe(30_000); | ||
| expect(warnSpy).toHaveBeenCalledTimes(1); | ||
| expect(warnSpy).toHaveBeenCalled(); |
There was a problem hiding this comment.
The assertion was relaxed from
toHaveBeenCalledTimes(1) to toHaveBeenCalled(), which no longer guards against the warning being emitted multiple times. If a refactor introduces duplicate warn calls on the fallback path this test would still pass silently. The exact-count assertion was the tighter, more useful signal here unless there's a known reason it fires more than once in this scenario.
| expect(warnSpy).toHaveBeenCalled(); | |
| expect(warnSpy).toHaveBeenCalledTimes(1); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
in-progressdispatch respects the global semaphore limit as well as project WIP capacity.Validation
pnpm testpnpm --filter @fusion/engine testpnpm --filter @fusion/engine test:corepnpm typecheckpnpm lintpnpm buildNotes
The concurrency fix covers the case where rows are already in
in-progressbut executors have not acquired semaphore slots yet; scheduler now treats those rows as consuming global dispatch capacity before admitting more work.Summary by CodeRabbit
Bug Fixes
Tests