From ad6acd89cd531c31e57624dfce6b4769adcb44e0 Mon Sep 17 00:00:00 2001 From: eric-wang-1990 Date: Wed, 29 Jul 2026 15:58:08 -0700 Subject: [PATCH 1/4] ci: warm test deps for Engineer Bot's offline agent + bump timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/engineer-bot.yml | 54 +++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/.github/workflows/engineer-bot.yml b/.github/workflows/engineer-bot.yml index 5bed4b8e6..9ec160477 100644 --- a/.github/workflows/engineer-bot.yml +++ b/.github/workflows/engineer-bot.yml @@ -50,7 +50,12 @@ jobs: runs-on: group: databricks-protected-runner-group labels: linux-ubuntu-latest - timeout-minutes: 45 + # Setup (~4 min) + the model-driven author step. With the test repo warmed + # offline (see warmup steps below) the real work converges in ~15-20 min; + # the headroom absorbs a hard bug without letting a stuck run idle for an + # hour. The agent itself is bounded tighter by its own step timeout so a + # runaway fails cleanly and still leaves time for the outcome comment. + timeout-minutes: 60 concurrency: # One author run per issue; a re-label while a run is in flight queues. group: engineer-bot-issue-${{ github.event.issue.number || inputs.issue_number }} @@ -88,6 +93,46 @@ jobs: # is slow and not needed for the agent's build. run: mvn -pl jdbc-core -am -B clean install -DskipTests -Ddependency-check.skip=true + # Warm the TEST-scoped repo while creds are still live. The agent's bug-fix + # flow runs `mvn test`, but the build above skips the test phase — so the + # test-compile classpath and the Surefire JUnit-Platform provider never land + # in ~/.m2. Once creds are scrubbed and Maven goes offline, the agent cannot + # fetch them and burns its whole budget fighting "surefire-junit-platform + # (absent)" / cache-miss errors (observed: ~25 min wasted, job timed out). + # Resolve everything the offline `mvn test` will need, HERE. Mirrors the + # dependency-warming in .github/workflows/warmMavenCache.yml. + - name: Warm test dependencies for the offline agent + run: | + set -euo pipefail + # Compile the test sources so all test-scoped deps are downloaded. + mvn -pl jdbc-core -am -B test-compile -Ddependency-check.skip=true + # Pull all plugins referenced by the build into ~/.m2. + mvn -pl jdbc-core -B dependency:resolve-plugins -Ddependency-check.skip=true || true + # The Surefire JUnit-Platform provider is resolved LAZILY by Surefire + # only when it actually executes JUnit-Platform tests — test-compile + # never triggers it, so fetch it explicitly at the repo's pinned version. + SUREFIRE_VERSION=$(mvn -pl jdbc-core -B help:evaluate \ + -Dexpression=maven-surefire-plugin.version -q -DforceStdout 2>/dev/null \ + | tail -n1) + mvn -B dependency:get \ + -Dartifact="org.apache.maven.surefire:surefire-junit-platform:${SUREFIRE_VERSION}" || true + + # Normalize the repo-id tracking markers BEFORE going offline. Artifacts + # downloaded through the JFrog mirror record their source repo id + # (`jfrog-central`) in each `_remote.repositories`. Under the agent's empty + # offline settings that id is absent from the resolution context, so Maven + # rejects the cached artifact as "present, but unavailable" and the agent + # thrashes on the `trackingFilename` workaround. Rewriting the id to + # `central` (the implicit default repo) makes the warmed repo resolve + # cleanly offline. Same technique warmMavenCache.yml uses before caching. + - name: Normalize _remote.repositories for offline resolution + run: | + set -euo pipefail + COUNT=$(find ~/.m2/repository -name '_remote.repositories' -print | wc -l) + find ~/.m2/repository -name '_remote.repositories' \ + -exec sed -i 's/jfrog-central/central/g' {} \; + echo "Normalized ${COUNT} _remote.repositories markers (jfrog-central -> central)" + # `mvn install` above ran spotless:apply (bound to the compile phase), which # may have reformatted tracked .java files that were already in the tree. # Revert that churn so the agent starts from a clean working tree — publish @@ -192,6 +237,13 @@ jobs: - name: Run author id: author + # Bound the model-driven step BELOW the job timeout so a stuck agent is + # killed here (this step fails) rather than at the job wall (the whole job + # is force-cancelled). Failing the step lets the always()-guarded outcome + # comment still run and report a real failure on the issue instead of a + # bare "job cancelled". 45 min leaves ~10 min of the 60-min job for setup + # + teardown. + timeout-minutes: 45 # Run from RUNNER_TEMP so the engine-rendered prompt's context file # (issue_body.txt, resolved against cwd) is read from there and the checkout # stays clean — publish's leftover check fails on any untracked path in the From 6458d16fc6c30d3788748884ad0fd2027f356f43 Mon Sep 17 00:00:00 2001 From: eric-wang-1990 Date: Wed, 29 Jul 2026 17:27:03 -0700 Subject: [PATCH 2/4] =?UTF-8?q?ci:=20address=20review=20=E2=80=94=20surefi?= =?UTF-8?q?re=20version=20fallback=20+=20accurate=20timeout=20comment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/engineer-bot.yml | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/.github/workflows/engineer-bot.yml b/.github/workflows/engineer-bot.yml index 9ec160477..196117a65 100644 --- a/.github/workflows/engineer-bot.yml +++ b/.github/workflows/engineer-bot.yml @@ -50,11 +50,16 @@ jobs: runs-on: group: databricks-protected-runner-group labels: linux-ubuntu-latest - # Setup (~4 min) + the model-driven author step. With the test repo warmed - # offline (see warmup steps below) the real work converges in ~15-20 min; - # the headroom absorbs a hard bug without letting a stuck run idle for an - # hour. The agent itself is bounded tighter by its own step timeout so a - # runaway fails cleanly and still leaves time for the outcome comment. + # Budget = setup + warmup + author + teardown. Setup+warmup (checkout, JDK, + # build, test-compile of jdbc-core -am, plugin/surefire resolution, repo-id + # normalization) is the heaviest non-author cost — call it ~10 min. The + # author step is capped at 45 min (see its own timeout-minutes), leaving + # 60-45=15 min for everything else, which comfortably covers setup+warmup + # plus teardown (publish + outcome comment). With the test repo warmed + # offline the real work converges in ~15-20 min; the headroom absorbs a hard + # bug without letting a stuck run idle for an hour. Bounding the author step + # below the job wall means a runaway fails that step cleanly (the outcome + # comment still posts) instead of the whole job being force-cancelled. timeout-minutes: 60 concurrency: # One author run per issue; a re-label while a run is in flight queues. @@ -111,9 +116,17 @@ jobs: # The Surefire JUnit-Platform provider is resolved LAZILY by Surefire # only when it actually executes JUnit-Platform tests — test-compile # never triggers it, so fetch it explicitly at the repo's pinned version. + # Fall back to 3.1.2 (matching warmMavenCache.yml) if help:evaluate + # can't resolve the expression: under `set -euo pipefail` a failed/empty + # command substitution does NOT abort the assignment, so without an + # explicit default SUREFIRE_VERSION could go empty, producing a + # 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 \ -Dexpression=maven-surefire-plugin.version -q -DforceStdout 2>/dev/null \ | tail -n1) + [ -n "$SUREFIRE_VERSION" ] || SUREFIRE_VERSION="3.1.2" mvn -B dependency:get \ -Dartifact="org.apache.maven.surefire:surefire-junit-platform:${SUREFIRE_VERSION}" || true From 3ed141aa32b247f7f470377253f66daee05950cd Mon Sep 17 00:00:00 2001 From: eric-wang-1990 Date: Wed, 29 Jul 2026 17:47:53 -0700 Subject: [PATCH 3/4] ci: execute one fast unit test during warmup to fully warm offline repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/engineer-bot.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.github/workflows/engineer-bot.yml b/.github/workflows/engineer-bot.yml index 196117a65..12a8e041c 100644 --- a/.github/workflows/engineer-bot.yml +++ b/.github/workflows/engineer-bot.yml @@ -129,6 +129,18 @@ jobs: [ -n "$SUREFIRE_VERSION" ] || SUREFIRE_VERSION="3.1.2" mvn -B dependency:get \ -Dartifact="org.apache.maven.surefire:surefire-junit-platform:${SUREFIRE_VERSION}" || true + # Finally, actually EXECUTE one fast unit test while creds are live. + # test-compile + the explicit get above resolve the test classpath and + # the provider jar, but anything Surefire resolves LAZILY at execution + # time (JUnit-Platform engine internals, provider transitives) only + # lands in ~/.m2 once a JUnit-Platform run really happens. This is what + # warmMavenCache.yml relies on — run the same fast test it uses so the + # offline `mvn test` inside Run author can't hit an execution-time + # resolution miss. `|| true`: warming the repo is the goal, not the + # test verdict (the agent runs its own tests later). + mvn -pl jdbc-core -B test \ + -Dtest="DatabricksParameterMetaDataTest#testInitialization" \ + -Ddependency-check.skip=true || true # Normalize the repo-id tracking markers BEFORE going offline. Artifacts # downloaded through the JFrog mirror record their source repo id From 661d3525f7faf8fb9e82aa193ca7097a0193d401 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" <3815206+peco-engineer-bot[bot]@users.noreply.github.com> Date: Thu, 30 Jul 2026 01:11:43 +0000 Subject: [PATCH 4/4] Thrift: NPE on metadata-less empty FetchResults batch with hasMoreRows=true (should skip + refetch) (#1567) Signed-off-by: peco-engineer-bot[bot] <3815206+peco-engineer-bot[bot]@users.noreply.github.com> --- .../impl/thrift/DatabricksThriftAccessor.java | 55 ++++++++----- .../thrift/DatabricksThriftAccessorTest.java | 80 +++++++++++++++++++ 2 files changed, 115 insertions(+), 20 deletions(-) diff --git a/src/main/java/com/databricks/jdbc/dbclient/impl/thrift/DatabricksThriftAccessor.java b/src/main/java/com/databricks/jdbc/dbclient/impl/thrift/DatabricksThriftAccessor.java index 18c126b2b..561e9222e 100644 --- a/src/main/java/com/databricks/jdbc/dbclient/impl/thrift/DatabricksThriftAccessor.java +++ b/src/main/java/com/databricks/jdbc/dbclient/impl/thrift/DatabricksThriftAccessor.java @@ -550,28 +550,43 @@ void updateConfig(DatabricksConfig newConfig) { } private TFetchResultsResp executeFetchRequest(TFetchResultsReq request) throws SQLException { - TFetchResultsResp response; - try { - response = getThriftClient().FetchResults(request); - } catch (TException e) { - String errorMessage = - String.format( - "Error while fetching results from Thrift server. Request maxRows=%d, " - + "maxBytes=%d, Error {%s}", - request.getMaxRows(), request.getMaxBytes(), e.getMessage()); - throw new DatabricksHttpException(errorMessage, e, DatabricksDriverErrorCode.INVALID_STATE); - } - String statementId = StatementId.loggableStatementId(request.getOperationHandle()); - verifySuccessStatus( - response.getStatus(), - String.format( - "Error while fetching results Request maxRows=%d, maxBytes=%d. " - + "Response hasMoreRows=%s", - request.getMaxRows(), request.getMaxBytes(), response.hasMoreRows), - statementId); + while (true) { + TFetchResultsResp response; + try { + response = getThriftClient().FetchResults(request); + } catch (TException e) { + String errorMessage = + String.format( + "Error while fetching results from Thrift server. Request maxRows=%d, " + + "maxBytes=%d, Error {%s}", + request.getMaxRows(), request.getMaxBytes(), e.getMessage()); + throw new DatabricksHttpException(errorMessage, e, DatabricksDriverErrorCode.INVALID_STATE); + } - return response; + verifySuccessStatus( + response.getStatus(), + String.format( + "Error while fetching results Request maxRows=%d, maxBytes=%d. " + + "Response hasMoreRows=%s", + request.getMaxRows(), request.getMaxBytes(), response.hasMoreRows), + statementId); + + // A legitimate intermediate empty batch: the server returned no result-set metadata but + // signalled hasMoreRows=true, meaning the rows (and metadata) arrive on a subsequent + // FetchResults. The initial-fetch consumers dereference the metadata, so skip this batch and + // re-issue FetchResults until metadata arrives (or hasMoreRows becomes false). This mirrors + // the behaviour of the reference C#/ADBC drivers. + if (response.getResultSetMetadata() == null && response.isHasMoreRows()) { + LOGGER.debug( + "Skipping metadata-less empty FetchResults batch (hasMoreRows=true) for statement {}" + + " and re-fetching", + statementId); + continue; + } + + return response; + } } private TFetchResultsReq createFetchResultsReqWithDefaults(TOperationHandle operationHandle) { diff --git a/src/test/java/com/databricks/jdbc/dbclient/impl/thrift/DatabricksThriftAccessorTest.java b/src/test/java/com/databricks/jdbc/dbclient/impl/thrift/DatabricksThriftAccessorTest.java index 58b65b5ba..cdd31ff35 100644 --- a/src/test/java/com/databricks/jdbc/dbclient/impl/thrift/DatabricksThriftAccessorTest.java +++ b/src/test/java/com/databricks/jdbc/dbclient/impl/thrift/DatabricksThriftAccessorTest.java @@ -158,6 +158,86 @@ void testExecute() throws TException, SQLException, DatabricksValidationExceptio assertEquals(resultSet.getStatementStatus().getState(), StatementState.SUCCEEDED); } + // RESULTFETCH-009: a single metadata-less empty FetchResults batch with hasMoreRows=true must be + // skipped and a follow-up FetchResults issued, instead of throwing an NPE on null metadata. + @Test + void testExecute_skipsMetadataLessEmptyBatch() + throws TException, SQLException, DatabricksValidationException { + setup(false); + TExecuteStatementReq request = new TExecuteStatementReq(); + TExecuteStatementResp tExecuteStatementResp = + new TExecuteStatementResp() + .setOperationHandle(tOperationHandle) + .setStatus(new TStatus().setStatusCode(TStatusCode.SUCCESS_STATUS)); + + // Intermediate empty batch: zero rows, hasMoreRows=true, and NO result-set metadata. + TFetchResultsResp emptyBatch = + new TFetchResultsResp() + .setStatus(new TStatus().setStatusCode(TStatusCode.SUCCESS_STATUS)) + .setResults(new TRowSet().setStartRowOffset(0).setRows(new ArrayList<>())) + .setHasMoreRows(true); + + when(thriftClient.FetchResults(getFetchResultsRequest(true))) + .thenReturn(emptyBatch) + .thenReturn(fetchResultsResponse); + when(thriftClient.ExecuteStatement(request)).thenReturn(tExecuteStatementResp); + Statement statement = mock(Statement.class); + when(parentStatement.getStatement()).thenReturn(statement); + when(statement.getQueryTimeout()).thenReturn(0); + when(thriftClient.GetOperationStatus(operationStatusReq)) + .thenReturn(operationStatusFinishedResp); + when(session.getConnectionContext()).thenReturn(connectionContext); + when(connectionContext.isComplexDatatypeSupportEnabled()).thenReturn(false); + + DatabricksResultSet resultSet = + accessor.execute(request, parentStatement, session, StatementType.SQL); + assertEquals(StatementState.SUCCEEDED, resultSet.getStatementStatus().getState()); + // The empty batch must have triggered a follow-up FetchResults. + verify(thriftClient, times(2)).FetchResults(getFetchResultsRequest(true)); + } + + // RESULTFETCH-010: multiple consecutive metadata-less empty batches must each be skipped, with + // FetchResults re-issued until metadata/rows arrive. + @Test + void testExecute_skipsMultipleConsecutiveMetadataLessEmptyBatches() + throws TException, SQLException, DatabricksValidationException { + setup(false); + TExecuteStatementReq request = new TExecuteStatementReq(); + TExecuteStatementResp tExecuteStatementResp = + new TExecuteStatementResp() + .setOperationHandle(tOperationHandle) + .setStatus(new TStatus().setStatusCode(TStatusCode.SUCCESS_STATUS)); + + TFetchResultsResp emptyBatch1 = + new TFetchResultsResp() + .setStatus(new TStatus().setStatusCode(TStatusCode.SUCCESS_STATUS)) + .setResults(new TRowSet().setStartRowOffset(0).setRows(new ArrayList<>())) + .setHasMoreRows(true); + TFetchResultsResp emptyBatch2 = + new TFetchResultsResp() + .setStatus(new TStatus().setStatusCode(TStatusCode.SUCCESS_STATUS)) + .setResults(new TRowSet().setStartRowOffset(0).setRows(new ArrayList<>())) + .setHasMoreRows(true); + + when(thriftClient.FetchResults(getFetchResultsRequest(true))) + .thenReturn(emptyBatch1) + .thenReturn(emptyBatch2) + .thenReturn(fetchResultsResponse); + when(thriftClient.ExecuteStatement(request)).thenReturn(tExecuteStatementResp); + Statement statement = mock(Statement.class); + when(parentStatement.getStatement()).thenReturn(statement); + when(statement.getQueryTimeout()).thenReturn(0); + when(thriftClient.GetOperationStatus(operationStatusReq)) + .thenReturn(operationStatusFinishedResp); + when(session.getConnectionContext()).thenReturn(connectionContext); + when(connectionContext.isComplexDatatypeSupportEnabled()).thenReturn(false); + + DatabricksResultSet resultSet = + accessor.execute(request, parentStatement, session, StatementType.SQL); + assertEquals(StatementState.SUCCEEDED, resultSet.getStatementStatus().getState()); + verify(thriftClient, times(3)).FetchResults(getFetchResultsRequest(true)); + } + @Test void testExecuteAsync() throws TException, SQLException, DatabricksValidationException { setup(true);