ci: warm test deps for Engineer Bot's offline agent + bump timeout - #1588
Conversation
The Engineer Bot's `Run author` step consumed its entire budget fighting the offline Maven setup rather than solving the issue. Two root causes, both reproduced from run 30479109185 (timed out at 45m after the agent had a correct fix at ~14m): 1. `surefire-junit-platform` was never cached — the warmup build runs `-DskipTests`, so the test phase (which lazily resolves the JUnit Platform provider) never executes. After creds are scrubbed and Maven goes offline, the agent's `mvn test` can't fetch it → ~25 min of dead retries. 2. Cached artifacts came back "present, but unavailable" — their `_remote.repositories` markers record `jfrog-central`, a repo id absent from the agent's empty offline settings. Fix (mirrors warmMavenCache.yml, which already solves both for forks): - Add a "Warm test dependencies" step that test-compiles, resolves plugins, and explicitly fetches surefire-junit-platform while JFrog creds are still live. - Normalize `_remote.repositories` (jfrog-central -> central) before the scrub so the warmed repo resolves cleanly offline. - Bump the job timeout 45 -> 60 min and add a 45-min step timeout on `Run author` so a stuck agent fails the step (letting the outcome comment report a real failure) instead of the job being force-cancelled. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Workflow-only change; the warmup + normalize approach correctly mirrors warmMavenCache.yml and the split-timeout reasoning is sound. One medium concern: the Surefire version resolution drops the hardcoded fallback that warmMavenCache.yml relies on, so a resolution miss can silently fail to warm the provider (F1). Plus a low note that the added warmup steps make the timeout comment's "~4 min setup" stale (F2).
eric-wang-1990
added
the
engineer-bot
…ment Two review-bot findings on PR #1588, both fixed: - F1 (medium): the surefire-junit-platform resolution had no fallback, unlike the mirrored warmMavenCache.yml. Under `set -euo pipefail` a failed/empty `help:evaluate` substitution leaves SUREFIRE_VERSION empty, producing a malformed dependency:get coordinate silently swallowed by `|| true` — re-introducing the (absent) dead-end. Default to 3.1.2 when the expression can't be resolved. - F2 (low): the timeout comment's stale "~4 min" setup figure ignored the two new pre-author steps. Rewrote it to reflect the real setup+warmup cost (~10 min) and the 60-45=15 min non-author budget. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a well-reasoned CI-only change that warms test-scoped deps while JFrog creds are live and normalizes _remote.repositories before the agent goes offline, faithfully mirroring warmMavenCache.yml. Step ordering is correct (warmup → normalize → revert spotless churn → scrub/offline → author), the surefire-version fallback is more robust than the reference, and the property it evaluates exists at pom.xml:57. One low-confidence note inline about whether test-compile warms everything the offline mvn test execution path needs (the reference workflow runs real tests). NO_CHANGELOG=true is appropriate for a CI-only change per CLAUDE.md.
Addresses review F1 (low, r3679000177): test-compile + the explicit surefire-junit-platform get resolve the test classpath and provider jar, but artifacts Surefire resolves LAZILY at execution time (JUnit-Platform engine internals, provider transitives) only land in ~/.m2 once a real JUnit-Platform run happens. Run the same fast test warmMavenCache.yml uses (DatabricksParameterMetaDataTest#testInitialization) while creds are live so the offline `mvn test` in Run author can't hit an execution-time resolution miss. `|| true` — warming the repo is the goal, not the verdict. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
GitHub Actions
Inactive
There was a problem hiding this comment.
Verdict: 1 Low
Solid CI-only change — it applies the proven warmMavenCache.yml warmup + _remote.repositories normalization inline to Engineer Bot while creds are live, with correct step ordering (warmup/normalize before the spotless revert and cred scrub). Verified the pinned surefire version property and the trigger test both exist. One low concern: heavier warmup can squeeze the teardown budget so the always()-guarded outcome comment may still be lost to a job-wall cancel.
Addresses review F1 (low, r3679085155): the author step's timeout is step-relative, not wall-clock — it starts after setup+warmup. With a 45-min cap, a long warmup could push author_start+45 to the 60-min job wall, leaving ~0 min for publish + Comment outcome, re-introducing the bare "job cancelled, no issue comment" this PR prevents. Size for the invariant instead of assuming warmup stays short: max(setup+warmup) + author_cap + teardown <= job_cap. With author_cap=40 and teardown~2, allowed setup+warmup = 60-40-2 = 18 min — ~4x the observed ~4.5 min. Updated both the job-level and step-level comments with the arithmetic. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Workflow-only change; ordering is sound (warm steps run online before the scrub forces MAVEN_ARGS=-o, and the spotless-churn revert runs after them), the timeout arithmetic checks out, and the techniques faithfully mirror warmMavenCache.yml. One Low: the SUREFIRE_VERSION fallback is unreachable under set -e+pipefail if help:evaluate ever fails, contradicting its own comment.
| # malformed coordinate that the trailing `|| true` would silently | ||
| # swallow — re-introducing the exact "surefire-junit-platform (absent)" | ||
| # dead-end this step exists to prevent. | ||
| SUREFIRE_VERSION=$(mvn -pl jdbc-core -B help:evaluate \ |
There was a problem hiding this comment.
🔵 Low — The SUREFIRE_VERSION fallback is effectively unreachable, contradicting the long comment above it.
Under set -euo pipefail, the assignment is SUREFIRE_VERSION=$(mvn ... | tail -n1). With pipefail, if mvn help:evaluate exits non-zero the whole pipe exits non-zero (tail's success doesn't mask it). A standalone assignment whose command substitution fails does trip set -e in bash, so the step aborts at this line — the [ -n "$SUREFIRE_VERSION" ] || SUREFIRE_VERSION="3.1.2" fallback on the next line never runs. The comment's claim that "a failed/empty command substitution does NOT abort the assignment ... SUREFIRE_VERSION could go empty" is only true for the empty-output case, not the mvn-failure case it's framed around.
In practice maven-surefire-plugin.version is pinned (pom.xml:57), so help:evaluate succeeds and this never triggers — hence Low. But the defensive path is broken exactly when it's meant to fire. warmMavenCache.yml gets this right by putting the fallback inside the substitution ($(mvn ... || echo "3.1.2")), which keeps the substitution exit status 0. Mirroring that pattern here would make the fallback actually reachable.
There was a problem hiding this comment.
⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.
The reviewer is correct: under set -euo pipefail, SUREFIRE_VERSION=$(mvn help:evaluate ... | tail -n1) aborts the step via set -e when mvn exits non-zero, so the next-line || SUREFIRE_VERSION="3.1.2" fallback is unreachable on the failure path (the comment only holds for the empty-output case). The correct fix is to fold the default into the substitution — $(mvn ... || echo "3.1.2") — mirroring warmMavenCache.yml. But this file lives under .github/, which is a denied/non-writable path for this follow-up job, so I cannot make the edit here. Flagging for a human to apply the one-line fix in a change that is permitted to touch the workflow file.
Problem
The Engineer Bot author runs are timing out. Of the first 3 runs, 2 hit the 45-minute wall (
cancelled) and 1 failed at 21.8m. In every run theRun authorstep consumes essentially the entire budget — setup is only ~4 min.Investigating run 30479109185 (the "Thrift NPE" issue): the agent produced a correct root-cause and fix at ~minute 14 (
CompressionCodec.getCompressionMappingdereferences null metadata → returnNONE). It then spent turns 24→150 (~25 min) stuck trying to run its test, never escaping, until the job was force-cancelled.The blocker was the offline Maven setup, in two flavors visible in the log:
surefire-junit-platform:<version>is genuinely absent from~/.m2. The warmup build ismvn ... install -DskipTests, so the test phase — which is what lazily resolves the JUnit-Platform Surefire provider — never runs. After creds are scrubbed and Maven is forced offline (MAVEN_ARGS=-o), the agent'smvn testcannot fetch the provider. Dead end._remote.repositoriestracking-id mismatch — cached plugins come back "present, but unavailable" because their tracking file recordsjfrog-central, a repo id absent from the agent's empty offline settings. The agent eventually found the-Daether.enhancedLocalRepository.trackingFilenameworkaround, but only after many wasted turns.Fix
Both problems are already solved for forked PRs in
warmMavenCache.yml; this PR applies the same techniques inline to the Engineer Bot, while JFrog credentials are still live (before the scrub step):Warm test dependencies for the offline agent—test-compile(downloads test-scoped deps),dependency:resolve-plugins, and an explicitdependency:getforsurefire-junit-platformat the pom's pinned version.Normalize _remote.repositories for offline resolution— rewritesjfrog-central→centralacross~/.m2so the warmed repo resolves cleanly under the agent's offline settings.timeout-minutes45 → 60, plus a 45-min step timeout onRun authorso a genuinely stuck agent fails the step (thealways()-guarded outcome comment then reports a real failure on the issue) instead of the whole job being force-cancelled with a bare "operation was canceled".The test-compile churn is reverted by the existing "Revert spotless churn" step, which runs after these steps, keeping the tree clean for publish.
Expected impact
Reclaims the ~25 min the agent wasted on dependency resolution — the real work in the sampled run converged in ~14-18 min. The timeout headroom absorbs a genuinely hard bug without letting a stuck run idle for an hour.
Testing
Workflow-only change; validated YAML locally. Real validation is the next label-triggered (or
workflow_dispatch) Engineer Bot run — the warmup steps should succeed with creds live, and the offlinemvn testinsideRun authorshould resolve without the surefire/tracking errors.NO_CHANGELOG=true
This pull request and its description were written by Isaac.