fix(l1): restart kurtosis engine before daily snapsync jobs#6203
fix(l1): restart kurtosis engine before daily snapsync jobs#6203
Conversation
The self-hosted runner's kurtosis engine stays at the old version across runs, causing API version mismatches when the CLI is updated. Add a dedicated engine-restart job that both sync jobs depend on.
🤖 Kimi Code ReviewReview of PR #6203This PR adds a new Issues Found:
Suggested Changes:engine-restart:
name: Restart Kurtosis Engine
runs-on: ethrex-sync
needs: prepare # Add this dependency
steps:
- name: Restart engine to match CLI version
run: |
# Add error handling and retry logic
for i in {1..3}; do
if kurtosis engine restart; then
echo "Engine restart successful"
sleep 5 # Give engine time to fully initialize
exit 0
fi
echo "Attempt $i failed, retrying..."
sleep 10
done
echo "Engine restart failed after 3 attempts"
exit 1Alternative Approach:Consider moving the engine restart into the sync job steps themselves, ensuring it happens immediately before the sync test runs. This would eliminate the parallel execution concerns. Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Codex Code ReviewSummary Findings
Ethereum-specific review Overall Next steps (optional)
Automated review by OpenAI Codex · custom prompt |
🤖 Claude Code ReviewNow I have enough context. Let me provide the code review. PR Review: fix(l1): restart kurtosis engine before daily snapsync jobsOverall AssessmentThis is a well-targeted fix for a version mismatch issue between the Kurtosis CLI and engine. The approach is sound and the implementation is clean. Positive Points
Concerns & Suggestions1. Missing error handling (Medium priority)The - name: Restart engine to match CLI version
run: kurtosis engine restartRecommendation: Add error handling and verification to ensure the restart succeeded: - name: Restart engine to match CLI version
run: |
kurtosis engine restart
# Verify engine is running and responsive
kurtosis engine status2. Race condition potential (Low priority)Both Current mitigation: The workflow already has Note: No action needed, but worth verifying the concurrency group behaves as expected with the new job structure. 3. Idempotency consideration (Informational)The
Recommendation: Consider checking the engine version before restarting, though this might be over-engineering for the current use case: - name: Restart engine to match CLI version
run: |
echo "Current engine status:"
kurtosis engine status || true
echo "Restarting engine..."
kurtosis engine restart
echo "New engine status:"
kurtosis engine statusThis provides better logging for debugging future issues. 4. Test plan alignment (Informational)The test plan mentions:
Ensure the manual test actually validates that:
Minor Observations
VerdictLGTM with suggestions. The core fix is correct and addresses the immediate problem. The suggested improvements around error handling and logging would make the workflow more robust and easier to debug, but are not blocking issues. The change is minimal, focused, and appropriate for the problem at hand. Once the test plan is executed successfully, this should be safe to merge. Automated review by Claude (Anthropic) · custom prompt |
Greptile OverviewGreptile SummaryAdds a dedicated Key changes:
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| .github/workflows/daily_snapsync.yaml | Adds engine-restart job to synchronize Kurtosis engine version with CLI before sync jobs run |
Last reviewed commit: 0614d43
Summary
ethrex-sync) where the kurtosis engine persists across runsengine-restartjob that runskurtosis engine restartbefore both sync jobsTest plan