diff --git a/.github/workflows/engineer-bot.yml b/.github/workflows/engineer-bot.yml index 5bed4b8e6..12a8e041c 100644 --- a/.github/workflows/engineer-bot.yml +++ b/.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 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);