Thrift: NPE on metadata-less empty FetchResults batch with hasMoreRows=true (should skip + refetch) (#1567)

.github/workflows/engineer-bot.yml

@@ -50,7 +50,17 @@ jobs:
     runs-on:
       group: databricks-protected-runner-group
       labels: linux-ubuntu-latest
-    timeout-minutes: 45
+    # 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.
       group: engineer-bot-issue-${{ github.event.issue.number || inputs.issue_number }}
@@ -88,6 +98,66 @@ 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.
+          # 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
+          # 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
+      # (`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 +262,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

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) {

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);