Improve CMT reliability with partial provisioning fallback and smarter mobile version selection - #94
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesCMT provisioning and version selection
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/config-matterwick.default.json`:
- Line 85: Update e2ePRInstanceMaxAge() to use an 8-hour fallback when
E2EPRInstanceMaxAge is unset, and revise its test to expect 8 hours instead of
24. Keep the configured-value behavior unchanged.
In `@server/push_events.go`:
- Around line 198-205: Update cleanupCreatedInstallation in the
createCloudInstallationWithRetry flow to handle DeleteInstallation failures
before another attempt: retry deletion or preserve the created installation ID
for later cleanup instead of discarding it. Add coverage for poll failure,
cancellation, and deletion failure after creation, while preserving fresh DNS
names per retry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ed9e773-98f2-4b2f-b65f-3aad6d7aebea
📒 Files selected for processing (7)
config/config-matterwick.default.jsonserver/e2e_dryrun_test.goserver/e2e_tests.goserver/e2e_tests_test.goserver/push_events.goserver/workflow_run.goserver/workflow_run_test.go
…letes Addresses CodeRabbit review on #94. e2ePRInstanceMaxAge() fell back to 24h when E2EPRInstanceMaxAge was unset while config-matterwick.default.json now ships 8h. Align the fallback (and its test) with the default so an unset key behaves like the shipped config. createCloudInstallation's cleanup discarded DeleteInstallation errors, so a single failed delete orphaned a paid server: the caller drops the ID on return and a retry provisions under a fresh name, leaving nothing to reference it until the periodic stale scan reaps it hours later. Cleanup now retries the delete (3 attempts) and reports an unrecoverable orphan to Mattermost with the installation ID. Adds coverage for the post-create failure paths: status-poll failure, delete failure after creation, and per-attempt cleanup during retry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NARSimoes
left a comment
There was a problem hiding this comment.
Thanks @yasserfaraazkhan looks good. @saturninoabril could you please also take look since you are familiar?
Agree with this tradeoff so that failed installation won't stop CMT from dispatching at all. However, what's the retry path? Will there be a workflow that could re-dispatch the dropped versions so it can complete the CMT requirements? |
saturninoabril
left a comment
There was a problem hiding this comment.
Thanks @yasserfaraazkhan!
Non-blocking, but worth checking the boundary between matterwick and the CI workflows, ex:
- matterwick: Actions trigger/listener, cloud test server management
- CI workflow - server versions
Mainly a separation of concerns: matterwirck owns infra lifecycle, the workflow owns the test policy. The workflow determines the versions and hand them over, while matterwick provisions what it's given and fails if it can't.
That keeps CMT versions declarative and maintainable in CI, where they can evolve with requirements.
@saturninoabril On the CI - matterwick : CI owning test policy and matterwick owning infra is the cleaner split. We actually tried that. The workflow declaring server_versions and handing them to matterwick. In practice workflow_run doesn’t expose those inputs to the listener, so we ended up with side channels (HTTP dispatch endpoints / artifact upload+download) that was not working and hard code all the logical sceanrios to handle. That’s why #90 moved version resolution into matterwick and reduced the trigger workflow to a signal. Until GitHub gives us a reliable input path on workflow_run. |
Dropped versions now get a targeted follow-up in matterwick. After the primary matrix runs, we wait for provisioner capacity, re-provision only the dropped versions, and if any recover we dispatch a second compatibility-matrix-testing.yml. This should happen automatically. |
Summary
This change makes CMT more resilient to provisioning failures while reducing the provisioning load for mobile runs.
Fixed CMT on release branches: the mobile CMT matrix again populates matrix.server.url, which every live build-release-* branch reads — without it those branches test against an empty server URL.
Mobile CMT now selects by category — latest ESR, latest production, latest RC — cutting provisioning from 25 to 15 installations per run. Empty categories (no RC in flight, no ESR flagged) backfill with the next stable line.
Desktop CMT is unchanged at up to five server versions and its existing selection logic.
A server version that fails to provision is dropped instead of aborting the whole matrix, so CMT runs with the versions that came up. Reduced coverage is announced via the Mattermost webhook, naming kept and dropped versions. If every version fails, CMT still can't run — but it now says so instead of failing silently.
Each installation gets one retry with a fresh DNS name, so a single transient provisioner failure no longer costs an entire version.
Push-triggered E2E failures (provisioning, dispatch, zero instances) now post to the Mattermost webhook instead of only logging.
E2EPRInstanceMaxAge reduced 24h → 8h to return capacity sooner, now that each mobile PR holds five servers instead of three