Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughRefactors worker unavailability tracking from a single reason to a per-worker Map, updates logic to collect/aggregate those reasons across state transitions, resets related tracked fields on restart, extends session types for diagnostics, and normalizes promise awaiting in FFmpeg executable checks. Changes
Sequence Diagram(s)sequenceDiagram
participant EvalRunner as EvaluationRunner
participant Tracker as TrackedExpectation
participant WorkerAgent as WorkerAgent
participant State as NEW_State
EvalRunner->>WorkerAgent: ask doYouSupportExpectation()
alt Worker supports
WorkerAgent-->>Tracker: support (remove entry from Map)
else Worker does not support / error / timeout
WorkerAgent-->>Tracker: set noAvailableWorkersReasons[workerId] = {user, tech}
end
Tracker->>EvalRunner: provide aggregated reasons (user msg, tech list)
EvalRunner->>State: set status reason fields from aggregated data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.ts (1)
38-38: Track the restart-failure TODO as an explicit follow-up.Line 38 sits on an error path; please convert this TODO into a concrete issue/task (retry threshold + fallback transition) to avoid indefinite RESTARTED loops.
I can draft the retry-threshold logic and open an issue template if you want.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.ts` at line 38, Convert the inline TODO about repeated restarts into a tracked follow-up and implement a retry-threshold fallback: create a task/issue describing “retry threshold + fallback transition for RESTARTED” and then in restarted.ts introduce a retry counter and constant MAX_RESTART_ATTEMPTS referenced where RESTARTED handling occurs (e.g., in the function that processes expectation states in this module), increment the counter each failure, reset it on successful transition, and when the counter exceeds MAX_RESTART_ATTEMPTS force a transition to NEW (or another safe fallback) and clear the counter; ensure the retry counter is persisted or attached to the expectation state so it survives repeated evaluations and add unit tests exercising the threshold behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.ts`:
- Around line 16-21: The code clears trackedExp.queriedWorkers,
trackedExp.availableWorkers, and trackedExp.noAvailableWorkersReasons before
calling manager.workerAgents.assignWorkerToSession which prevents immediate
worker selection because assignWorkerToSession only re-populates
availableWorkers when trackedExp.waitingForWorkerTime !== null; fix by either
(A) resetting trackedExp.waitingForWorkerTime to a non-null value immediately
after clearing those sets so
updateAvailableWorkersForExpectation/determineBestWorkerForExpectation can run
in the same cycle and avoid transitioning to noWorkerAssigned/NEW, or (B) if the
extra iteration is intended, update the inline comment above the clears to
explicitly state that clearing these sets forces a new worker query in the next
cycle and is an intentional behavior to avoid in-cycle reassignments. Ensure
changes reference trackedExp and manager.workerAgents.assignWorkerToSession and
preserve existing state transition logic (determineBestWorkerForExpectation,
noWorkerAssigned, NEW).
In
`@shared/packages/expectationManager/src/internalManager/lib/trackedWorkerAgents.ts`:
- Around line 74-84: The per-worker reason entries set on
trackedExp.noAvailableWorkersReasons are losing error detail and contain a stray
quote in the timeout message; update the two places where you set entries for
workerId (inside the doYouSupportExpectation() error handling) to: fix the typo
in the timeout tech string (remove the extra quote) and preserve full error
diagnostics by converting err to a safe string (e.g., use JSON.stringify(err) or
util.inspect(err) rather than concatenating err to ''), so the tech field
contains actionable error details instead of "[object Object]".
---
Nitpick comments:
In
`@shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.ts`:
- Line 38: Convert the inline TODO about repeated restarts into a tracked
follow-up and implement a retry-threshold fallback: create a task/issue
describing “retry threshold + fallback transition for RESTARTED” and then in
restarted.ts introduce a retry counter and constant MAX_RESTART_ATTEMPTS
referenced where RESTARTED handling occurs (e.g., in the function that processes
expectation states in this module), increment the counter each failure, reset it
on successful transition, and when the counter exceeds MAX_RESTART_ATTEMPTS
force a transition to NEW (or another safe fallback) and clear the counter;
ensure the retry counter is persisted or attached to the expectation state so it
survives repeated evaluations and add unit tests exercising the threshold
behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
shared/packages/api/src/ffmpeg.tsshared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/new.tsshared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.tsshared/packages/expectationManager/src/internalManager/lib/trackedWorkerAgents.tsshared/packages/expectationManager/src/lib/trackedExpectation.tsshared/packages/expectationManager/src/lib/types.tsshared/packages/worker/src/worker/workers/genericWorker/lib/executableDependencyHandler.ts
Comment on lines
+16
to
21
| // Clear this state, so that they will be repopulated in NEW: | ||
| trackedExp.queriedWorkers.clear() | ||
| trackedExp.availableWorkers.clear() | ||
| trackedExp.noAvailableWorkersReasons.clear() | ||
|
|
||
| await manager.workerAgents.assignWorkerToSession(trackedExp) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for evaluateExpectationStateRestarted and related restart/restarted patterns
echo "=== Searching for restart-related code ==="
rg -n -C4 --type=ts 'evaluateExpectationStateRestarted|WorkStatusState\.RESTARTED' | head -100
echo -e "\n=== Searching for assignWorkerToSession ==="
rg -n -C6 --type=ts 'assignWorkerToSession\s*\('
echo -e "\n=== Searching for noWorkerAssigned ==="
rg -n -C4 --type=ts 'noWorkerAssigned\s*\('
echo -e "\n=== Searching for waitingForWorkerTime ==="
rg -n -C4 --type=ts 'waitingForWorkerTime\b'Repository: Sofie-Automation/sofie-package-manager
Length of output: 39882
🏁 Script executed:
#!/bin/bash
# Get the structure of the restarted.ts file to understand the full restart flow
echo "=== Reading restarted.ts file ==="
wc -l shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.ts
head -50 shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.tsRepository: Sofie-Automation/sofie-package-manager
Length of output: 2038
🏁 Script executed:
#!/bin/bash
# Look for assignWorkerToSession implementation to understand what it does
echo "=== Finding assignWorkerToSession implementation ==="
fd -t f -e ts | xargs rg -l 'assignWorkerToSession.*=' | head -5Repository: Sofie-Automation/sofie-package-manager
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Get the full assignWorkerToSession implementation
sed -n '191,250p' shared/packages/expectationManager/src/internalManager/lib/trackedWorkerAgents.tsRepository: Sofie-Automation/sofie-package-manager
Length of output: 1833
🏁 Script executed:
#!/bin/bash
# Get the noWorkerAssigned implementation to understand what happens when no worker is available
sed -n '158,175p' shared/packages/expectationManager/src/expectationTracker/lib/trackedExpectationAPI.tsRepository: Sofie-Automation/sofie-package-manager
Length of output: 795
🏁 Script executed:
#!/bin/bash
# Check if there's any logic that delays assignment based on noWorkerAssignedTime or cleared state
rg -n -C8 'noWorkerAssignedTime' shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.ts
rg -n -C5 'noWorkerAssignedTime' shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationState.tsRepository: Sofie-Automation/sofie-package-manager
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Search for any logic that would prevent assignment when state is just cleared
rg -n -B3 -A3 'queriedWorkers\|availableWorkers' shared/packages/expectationManager/src/internalManager/lib/trackedWorkerAgents.ts | head -60Repository: Sofie-Automation/sofie-package-manager
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Find and examine determineBestWorkerForExpectation
rg -n -A30 'async determineBestWorkerForExpectation' shared/packages/expectationManager/src/internalManager/lib/trackedWorkerAgents.tsRepository: Sofie-Automation/sofie-package-manager
Length of output: 1333
🏁 Script executed:
#!/bin/bash
# Check if there are any guard conditions that would skip finding a worker after clearing
rg -n -B5 -A15 'updateAvailableWorkersForExpectation' shared/packages/expectationManager/src/internalManager/lib/trackedWorkerAgents.ts | head -80Repository: Sofie-Automation/sofie-package-manager
Length of output: 1763
🏁 Script executed:
#!/bin/bash
# Look at the flow to see if clearing state would affect the assignment in the same cycle
echo "=== Check if clearing affects waitingForWorkerTime ==="
rg -n 'waitingForWorkerTime' shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.ts
echo -e "\n=== Full restarted.ts context around assignment ==="
sed -n '14,50p' shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.tsRepository: Sofie-Automation/sofie-package-manager
Length of output: 1519
🏁 Script executed:
#!/bin/bash
# Check if clearing state should include resetting waitingForWorkerTime to trigger updateAvailableWorkersForExpectation
echo "=== Check how waitingForWorkerTime is managed in restart ==="
rg -n 'waitingForWorkerTime.*=' shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/ -A2 -B2
echo -e "\n=== Check if noWorkerAssigned sets waitingForWorkerTime ==="
sed -n '158,185p' shared/packages/expectationManager/src/expectationTracker/lib/trackedExpectationAPI.tsRepository: Sofie-Automation/sofie-package-manager
Length of output: 1244
🏁 Script executed:
#!/bin/bash
# Compare with how other state handlers handle worker assignment
echo "=== Check aborted.ts for comparison ==="
cat shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/aborted.ts
echo -e "\n=== Check new.ts to see if clearing is redundant ==="
cat shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/new.tsRepository: Sofie-Automation/sofie-package-manager
Length of output: 5704
🏁 Script executed:
#!/bin/bash
# Check the complete noWorkerAssigned flow to understand state transitions
sed -n '158,195p' shared/packages/expectationManager/src/expectationTracker/lib/trackedExpectationAPI.tsRepository: Sofie-Automation/sofie-package-manager
Length of output: 1427
🏁 Script executed:
#!/bin/bash
# Verify: does clearing state prevent immediate removal in the same cycle?
# Check if the removal would be skipped when no worker is assigned
echo "=== Trace the RESTARTED flow path ==="
echo "1. Clear state in restarted.ts"
echo "2. Call assignWorkerToSession"
echo "3. If availableWorkers is empty and waitingForWorkerTime is null, what happens?"
echo ""
echo "Check if waitingForWorkerTime affects state update:"
rg -n 'waitingForWorkerTime' shared/packages/expectationManager/src/expectationTracker/lib/trackedExpectationAPI.ts -B3 -A3Repository: Sofie-Automation/sofie-package-manager
Length of output: 313
🏁 Script executed:
#!/bin/bash
# Compare RESTARTED vs ABORTED - why does RESTARTED clear but ABORTED doesn't?
echo "=== ABORTED (without clearing) ==="
sed -n '15,25p' shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/aborted.ts
echo -e "\n=== RESTARTED (with clearing) ==="
sed -n '14,22p' shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.ts
echo -e "\n=== Check if determineBestWorkerForExpectation needs populated availableWorkers ==="
sed -n '100,120p' shared/packages/expectationManager/src/internalManager/lib/trackedWorkerAgents.tsRepository: Sofie-Automation/sofie-package-manager
Length of output: 1780
🏁 Script executed:
#!/bin/bash
# Critical: Check if clearing availableWorkers prevents finding workers in determineBestWorkerForExpectation
echo "=== Check the exact flow when availableWorkers is empty ==="
echo "In determineBestWorkerForExpectation, line 112:"
echo "const workerIds = Array.from(trackedExp.availableWorkers.keys())"
echo ""
echo "After clearing, availableWorkers.size = 0"
echo "So workerIds will be empty array"
echo "PromisePool.for([]).process() will not execute any iterations"
echo "bestWorker will be undefined"
echo ""
echo "=== Check updateAvailableWorkersForExpectation trigger ==="
sed -n '215,220p' shared/packages/expectationManager/src/internalManager/lib/trackedWorkerAgents.ts
echo -e "\n=== Does clearing reset waitingForWorkerTime? ==="
grep -n 'waitingForWorkerTime' shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.tsRepository: Sofie-Automation/sofie-package-manager
Length of output: 845
🏁 Script executed:
#!/bin/bash
# Check if noWorkerAssigned clears or sets waitingForWorkerTime
sed -n '158,210p' shared/packages/expectationManager/src/expectationTracker/lib/trackedExpectationAPI.tsRepository: Sofie-Automation/sofie-package-manager
Length of output: 2010
🏁 Script executed:
#!/bin/bash
# Verify: When state transitions to NEW via noWorkerAssigned, is the removal message already sent?
# Check the session state during the transition
rg -n -B5 -A10 'state: ExpectedPackageStatusAPI.WorkStatusState.NEW' shared/packages/expectationManager/src/expectationTracker/lib/trackedExpectationAPI.ts | head -50Repository: Sofie-Automation/sofie-package-manager
Length of output: 1477
🏁 Script executed:
#!/bin/bash
# Final verification: Does clearing state actually matter, or is it redundant with NEW state?
echo "=== Check if restarted.ts actually needs to clear state ==="
echo "After noWorkerAssigned, state transitions to NEW"
echo "NEW state calls updateAvailableWorkersForExpectation (which repopulates workers)"
echo ""
echo "So the question: Is the clearing necessary at all?"
echo ""
echo "=== Check what state is AFTER clearing and BEFORE calling assignWorkerToSession ==="
echo "State is still RESTARTED when clearing happens"
echo "So if assignWorkerToSession finds a worker, it will:"
echo "1. Assign the worker"
echo "2. Call removeExpectation in RESTARTED state"
echo "3. Transition to NEW state"
echo ""
echo "If no worker found (empty availableWorkers after clear):"
echo "1. noWorkerAssigned() called in RESTARTED state"
echo "2. Transitions to NEW (because availableWorkers.size === 0)"
echo "3. Next cycle: NEW state re-queries workers"
echo ""
echo "=== The concern: Does clearing + empty availableWorkers skip the removal? ==="
sed -n '21,49p' shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.tsRepository: Sofie-Automation/sofie-package-manager
Length of output: 1958
🏁 Script executed:
#!/bin/bash
# Check if there's any scenario where workers wouldn't be re-queried
echo "=== If state goes to NEW, will updateAvailableWorkersForExpectation always run? ==="
sed -n '8,35p' shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/new.ts
echo -e "\n=== Conclusion check: Can assignment and removal happen in same cycle after restart? ==="
echo "YES, IF availableWorkers is not empty after clearing"
echo "NO, IF availableWorkers is empty after clearing (because updateAvailableWorkersForExpectation is skipped)"Repository: Sofie-Automation/sofie-package-manager
Length of output: 1680
Clear worker state before assignment forces an extra iteration in restart flows.
Clearing queriedWorkers, availableWorkers, and noAvailableWorkersReasons (lines 17-19) before calling assignWorkerToSession (line 21) prevents worker assignment in the same cycle. Since assignWorkerToSession only calls updateAvailableWorkersForExpectation when waitingForWorkerTime !== null, the cleared availableWorkers set remains empty, causing determineBestWorkerForExpectation to find no workers. This triggers noWorkerAssigned(), which transitions to NEW state. The removal operation then cannot execute until the next cycle when NEW state re-queries workers.
Consider either:
- Resetting
waitingForWorkerTimewhen clearing to trigger immediate worker re-query in the same cycle, or - Accepting the extra iteration as intentional by updating the comment to explain this behavior
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.ts`
around lines 16 - 21, The code clears trackedExp.queriedWorkers,
trackedExp.availableWorkers, and trackedExp.noAvailableWorkersReasons before
calling manager.workerAgents.assignWorkerToSession which prevents immediate
worker selection because assignWorkerToSession only re-populates
availableWorkers when trackedExp.waitingForWorkerTime !== null; fix by either
(A) resetting trackedExp.waitingForWorkerTime to a non-null value immediately
after clearing those sets so
updateAvailableWorkersForExpectation/determineBestWorkerForExpectation can run
in the same cycle and avoid transitioning to noWorkerAssigned/NEW, or (B) if the
extra iteration is intended, update the inline comment above the clears to
explicitly state that clearing these sets forces a new worker query in the next
cycle and is an intentional behavior to avoid in-cycle reassignments. Ensure
changes reference trackedExp and manager.workerAgents.assignWorkerToSession and
preserve existing state transition logic (determineBestWorkerForExpectation,
noWorkerAssigned, NEW).
shared/packages/expectationManager/src/internalManager/lib/trackedWorkerAgents.ts
Show resolved
Hide resolved
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
About Me
This pull request is posted on behalf of the NRK.
Type of Contribution
This is a: Bug fix/ Feature / Code improvement
New Behavior
This PR improves the way Package Manager handles expectations in a few ways:
testFFExecutable(), the original error message got lost in the thrown Error.Status
Quality of Life Improvements
Overview
This PR improves the Package Manager's handling of Expectations, particularly when no workers are available to support them. It changes error reporting to aggregate explanations from all workers instead of only one, preserves original error messages in ffmpeg/executable checks, and ensures Expectation state is cleared on restart so worker support is re-evaluated.
Key Changes
Multi-Worker Explanations
TrackedExpectation.noAvailableWorkersReason: ReasonwithTrackedExpectation.noAvailableWorkersReasons: Map<WorkerAgentId, Reason>to record per-worker reasons.trackedWorkerAgents.ts) to:noAvailableWorkersReasonswith{ user, tech }per worker for non-support, timeout, and other errors;evaluateExpectationStates/new.tsto aggregate per-worker reasons into a user-facing message and a technical message listing all technical reasons plus queried workers, so status explains why no workers support the Expectation.Improved Error Handling
testFFExecutable()(files:shared/packages/api/src/ffmpeg.tsandshared/packages/worker/src/worker/workers/genericWorker/lib/executableDependencyHandler.ts) to await the inner Promise result into a local variable and return it, ensuring the original error message is preserved by existing catch blocks and not lost when rethrowing.State Clearing on Restart
evaluateExpectationStates/restarted.tsnow clearstrackedExp.queriedWorkers,trackedExp.availableWorkers, andtrackedExp.noAvailableWorkersReasonsbefore transitioning the Expectation back to NEW, forcing a fresh re-check of worker support.Types / API
noAssignedWorkerReason?: ReasontoExpectationStateHandlerSessioninshared/packages/expectationManager/src/lib/types.tsto carry diagnostics when no worker is assigned.getDefaultTrackedExpectation(...)to use an empty Map fornoAvailableWorkersReasons.Impact
Files Touched (high level)