fix(ci): retry transient platform errors, run firms in parallel, warn on firm drift#28
Conversation
…n on firm drift
Production revealed three problems with the parallel liquid-test run:
1. Transient platform errors counted as test failures. The platform
sometimes returns internal_error ('Internal error. Try to run the test
again...') or no response at all; the workflow reported those as FAILED
with no second chance. Each template now gets up to MAX_TEST_ATTEMPTS
(default 3) when its failure reason is transient (internal error, no
response, run never completed, timeout). Genuine test failures - the CLI
names the failing unit tests - are never retried.
2. A CLI crash failed every template in the batch. A template whose
verdict is missing (the CLI died or dropped it) is now retried instead
of immediately failing, and only reported as failed when retries are
exhausted. A crashed bucket runner is also detected per template.
3. Firms waited on each other. Each <firm>|<type> bucket now runs as its
own background job (capped at MAX_PARALLEL_FIRMS, default 3), so a slow
firm only delays its own bucket. Bucket output is flushed as one log
group per bucket when it finishes, with a heartbeat line while waiting.
Also: warn once when templates fall back to 'first firm id in config.json'
- that choice silently changes when config.json is regenerated (this moved
a whole PR's tests to a firm without test data and failed them all), and
add timeout-minutes to the job now that retries can extend it.
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe run_tests.yml workflow adds job timeout and new tuning variables, switches template execution to bucketized parallel runs with retries, tracks fallback firm-id selection, and updates summary handling for missing or transient test verdicts. ChangesTest Runner Concurrency and Retry Logic
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: ci, workflow Suggested reviewers: None identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
|
Live validation on be_market: run 28661065102 (test branch pointed at this ref) — 27 templates, 2 firms, 3 buckets, 5m20s total. Everything this PR adds fired for real in that run:
Final tally: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/run_tests.yml (1)
379-384: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication in SUMMARY append.
Both branches push the identical tuple to
SUMMARY; only theERRORSpush differs. This can be collapsed to reduce drift risk.♻️ Optional simplification
- echo "${NAME}: ${STATUS}${NOTE}" - if [[ "${STATUS}" == "PASS" ]]; then - SUMMARY+=("${STATUS}${NOTE}"$'\t'"${FIRM_ID}"$'\t'"${NAME}") - else - ERRORS+=("${NAME}") - SUMMARY+=("${STATUS}${NOTE}"$'\t'"${FIRM_ID}"$'\t'"${NAME}") - fi + echo "${NAME}: ${STATUS}${NOTE}" + SUMMARY+=("${STATUS}${NOTE}"$'\t'"${FIRM_ID}"$'\t'"${NAME}") + [[ "${STATUS}" != "PASS" ]] && ERRORS+=("${NAME}")🤖 Prompt for 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. In @.github/workflows/run_tests.yml around lines 379 - 384, The STATUS branch in the test summary block duplicates the same SUMMARY append in both outcomes, so simplify the logic in the workflow step around the STATUS/PASS check by moving the SUMMARY+= call outside the if/else and keeping only the ERRORS+=("${NAME}") addition in the non-PASS branch. Use the existing STATUS, NOTE, SUMMARY, and ERRORS handling in this block to preserve behavior while removing the repeated tuple append.
🤖 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.
Nitpick comments:
In @.github/workflows/run_tests.yml:
- Around line 379-384: The STATUS branch in the test summary block duplicates
the same SUMMARY append in both outcomes, so simplify the logic in the workflow
step around the STATUS/PASS check by moving the SUMMARY+= call outside the
if/else and keeping only the ERRORS+=("${NAME}") addition in the non-PASS
branch. Use the existing STATUS, NOTE, SUMMARY, and ERRORS handling in this
block to preserve behavior while removing the repeated tuple append.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d77143d2-83fb-443c-9d31-5074f2ea4057
📒 Files selected for processing (1)
.github/workflows/run_tests.yml
|
CLI-side follow-up is up as well: silverfin/silverfin-cli#263 caps the poll interval at 5s (was growing to 30-100s on long runs). Together: this PR removes false reds and firm serialization; that one removes the detection lag on long tests. |
| KEY="${KEYS[${NEXT_LAUNCH}]}" | ||
| FIRM="${KEY%%|*}" | ||
| TYPE="${KEY##*|}" | ||
| printf '%s' "${TEMPLATE_BUCKETS[${KEY}]}" | sort -u | grep -v '^$' > "${WORKDIR}/list.${NEXT_LAUNCH}" |
There was a problem hiding this comment.
🟡 Minor — step aborts if a bucket's identifiers are all empty. Under the Actions default bash -eo pipefail, this bare pipeline aborts the entire step (killing every bucket, not just this one) whenever grep -v '^$' selects nothing — i.e. a bucket whose only identifier is an empty string. That is reachable: a reconciliation config.json with "handle": "" yields IDENTIFIER="", because .handle // .name only falls through on null/false, not on an empty string.
The pre-refactor code ran the identical pipeline inside a <( … ) process substitution feeding mapfile, where the non-zero exit was ignored; moving it to a plain redirect dropped that implicit guard. Fails red (not silently green), but one malformed config would fail CI for all templates in the run.
| printf '%s' "${TEMPLATE_BUCKETS[${KEY}]}" | sort -u | grep -v '^$' > "${WORKDIR}/list.${NEXT_LAUNCH}" | |
| printf '%s' "${TEMPLATE_BUCKETS[${KEY}]}" | sort -u | grep -v '^$' > "${WORKDIR}/list.${NEXT_LAUNCH}" || true |
There was a problem hiding this comment.
We should assume that the config file is always correctly formatted and contains at least the handle or account name.
…ntifiers Address review on PR #28: - launch_ready_buckets: the `sort -u | grep -v '^$'` pipeline exits 1 when a bucket's identifiers are all empty (e.g. a config with "handle": "", since `.handle // .name` only falls through on null/false). Under the Actions default `bash -eo pipefail` that aborted the whole step, killing every bucket. Add `|| true`; an empty list is already handled downstream. - Collapse the duplicated SUMMARY append into a single push with a guarded ERRORS append (CodeRabbit nitpick), matching the existing `&&` idiom.
2 participants
Why
The first real PRs after #27 merged showed three problems (analysed from runs 28656835005, 28647828601, 28658481661 on be_market):
vol_2_1"failed" withInternal error. Try to run the test again or contact support if the issue persists.- the platform itself says to retry, and nothing did.test_firm_idand noSF_TEST_FIRM_ID, a template runs on the first firm id in itsconfig.json. Regenerating config.json reorders those keys: one PR's templates all silently moved to firm 400583 (no matching test data there) and failed en masse - templates that pass on firms 6056/542. This logic predates feat(ci): run liquid tests in parallel, grouped per firm and template type #27, but parallel runs made the blast radius visible.Worth noting from the same analysis: the parallelism itself works (a 10-template batch finished in 16s), and two of the "failures" investigated were genuine test regressions that CI should have caught. The real defects were the missing retry, the firm drift, and the serialization.
What this does
MAX_TEST_ATTEMPTS(default 3) when its failure reason is transient: internal error, no response, run never completed, timeout - the CLI prints the reason as an indented line since silverfin-cli#262, and that is what the classifier reads. Genuine test failures (named failing unit tests) are never retried - rerunning cannot fix an assertion.<firm>|<type>bucket is a background job, capped atMAX_PARALLEL_FIRMS(default 3). Global in-flight ceiling =MAX_PARALLEL_TESTS x MAX_PARALLEL_FIRMS. A slow firm now only delays its own bucket.PASS (attempt 2)) so retried flakes are visible.::warning::when templates fall back to "first firm id in config.json", with the fix (pintest_firm_idor setSF_TEST_FIRM_ID). The real fix for drift is pinning - the workflow can only make it visible.timeout-minutes: 120on the job, since retries can extend it (a single platform test can poll up to ~33 min).What this deliberately does not do
Validation
Offline, with a stateful mock CLI (every scenario asserted on call counts):
PASS (attempt 2)FAIL - transient platform error persistedPASS (attempt 2); batch-mates unaffected& ( )and spacesLive: validated on be_market (test branch pointed at this ref) - run linked in comments once green.