-
Notifications
You must be signed in to change notification settings - Fork 156
Fix trigger.dev preview environment archiving reliability #2186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Prevent archiving operations from being cancelled by concurrent workflows - Add retry logic with 3 attempts and 10-second delays - Add branch name validation to prevent empty branch errors - Add verification step to confirm archiving succeeded - Add cleanup fallback step to handle stale environments - Improve logging for better debugging Addresses issues identified in #2181 Co-Authored-By: [email protected] <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Updates to Preview Branch (devin/1750759330-fix-trigger-dev-archiving) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
PR Reviewer Guide 🔍(Review updated until commit 1d288dd)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 1d288dd
Previous suggestionsSuggestions up to commit 6ee5efd
|
- Remove branch name validation as requested by [email protected] - Keep all other enhancements: retry logic, verification, cleanup step - Use direct github.head_ref instead of BRANCH_NAME variable Co-Authored-By: [email protected] <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves the reliability of archiving preview environments on trigger.dev by preventing cancellation of concurrent workflows, adding robust retry logic with verification, and ensuring cleanup of stale environments.
- Updated concurrency to conditionally prevent cancellation based on event action
- Implemented retry logic with logging and verification for the archive command
- Added a cleanup fallback step for stale preview environments
if pnpm --filter @liam-hq/jobs exec trigger preview list --branch "${{ github.head_ref }}" 2>/dev/null | grep -q "${{ github.head_ref }}"; then | ||
echo "Warning: Environment may still be active after archiving" | ||
else | ||
echo "Archive verification successful - environment no longer listed" | ||
fi | ||
|
||
exit 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The verification step logs a warning when the preview environment is still detected but then exits with success. This could lead to false positives where failures are not caught; consider exiting with a non-zero status or adding additional retry logic when the environment is still active.
if pnpm --filter @liam-hq/jobs exec trigger preview list --branch "${{ github.head_ref }}" 2>/dev/null | grep -q "${{ github.head_ref }}"; then | |
echo "Warning: Environment may still be active after archiving" | |
else | |
echo "Archive verification successful - environment no longer listed" | |
fi | |
exit 0 | |
MAX_VERIFY_RETRIES=3 | |
VERIFY_RETRY_COUNT=0 | |
while [ $VERIFY_RETRY_COUNT -lt $MAX_VERIFY_RETRIES ]; do | |
if pnpm --filter @liam-hq/jobs exec trigger preview list --branch "${{ github.head_ref }}" 2>/dev/null | grep -q "${{ github.head_ref }}"; then | |
echo "Environment still active after archiving. Retrying verification in 10 seconds..." | |
VERIFY_RETRY_COUNT=$((VERIFY_RETRY_COUNT + 1)) | |
sleep 10 | |
else | |
echo "Archive verification successful - environment no longer listed" | |
exit 0 | |
fi | |
done | |
echo "Verification failed: Environment still active after maximum retries" | |
exit 1 |
Copilot uses AI. Check for mistakes.
fi | ||
done | ||
|
||
- name: Cleanup stale environments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The condition combining 'github.event.action == "closed"' with 'always()' in the cleanup step can be confusing. Adding a comment to clarify the intended behavior of running the cleanup step regardless of previous failures while ensuring it only applies when the event is 'closed' would improve maintainability.
- name: Cleanup stale environments | |
- name: Cleanup stale environments | |
# This step ensures cleanup of stale environments only when the event action is 'closed'. | |
# The 'always()' function ensures this step runs regardless of the success or failure of previous steps. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
# Attempt cleanup even if the previous step failed | ||
pnpm --filter @liam-hq/jobs exec trigger preview archive --branch "${{ github.head_ref }}" 2>/dev/null || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempt cleanup even if the previous step failed
I understand this runs even if the previous step succeeded ? — which seems fine either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the motivation is to remove it completely.
@@ -36,4 +36,56 @@ jobs: | |||
TRIGGER_ACCESS_TOKEN: ${{ secrets.TRIGGER_ACCESS_TOKEN }} | |||
TRIGGER_PROJECT_ID: ${{ vars.TRIGGER_PROJECT_ID }} | |||
run: | | |||
pnpm --filter @liam-hq/jobs exec trigger preview archive --branch ${{ github.head_ref }} | |||
set -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nits]
Looks good! Just a note — shell: bash
already implies bash --noprofile --norc -eo pipefail {0}
, so set -e
might not be needed.
https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!! @claude Please implement this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing a problem with claude not working on #2216
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix it: cb9e1c8
GitHub Actions' shell: bash already includes error handling with -eo pipefail, making the explicit set -e unnecessary. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Fix trigger.dev preview environment archiving reliability
Summary
This PR implements the fixes proposed by Claude in issue #2181 to improve the reliability of trigger.dev preview environment archiving in GitHub Actions workflows.
Changes Made
1. Prevent Archive Cancellation
cancel-in-progress
setting to${{ github.event.action != 'closed' }}
instead oftrue
2. Enhanced Archive Step with Robust Error Handling
3. Added Cleanup Fallback Step
always()
condition to run regardless of previous step statusProblem Addressed
The original workflow had several potential failure scenarios:
Testing Transparency
What I Actually Checked
What I Did Not Check
Review Checklist
Please verify:
trigger preview list --branch
works as expected in your environmentRelated Issues
Fixes #2181
Link to Devin run: https://app.devin.ai/sessions/daa3c06b19b54017bfc27c17ae8cfb6a
Requested by: [email protected]