(incubator-devlake) branch main updated: fix(azuredevops_go): handle empty/invalid timeline responses for YAML… (#8839)

[kl...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

klesh pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/incubator-devlake.git


The following commit(s) were added to refs/heads/main by this push:
     new 3472cc364 fix(azuredevops_go): handle empty/invalid timeline responses for YAML… (#8839)
3472cc364 is described below

commit 3472cc3644f3684b3ae3e6599e359dcc58d2bf5d
Author: Cornelius Schuchardt <co...@gmail.com>
AuthorDate: Wed Apr 29 06:05:08 2026 +0200

    fix(azuredevops_go): handle empty/invalid timeline responses for YAML… (#8839)
    
    * fix(azuredevops_go): handle empty/invalid timeline responses for YAML-broken builds
    
    Builds that fail due to YAML syntax errors in pipeline definitions return
    an empty or non-JSON body from the Timeline API (200 OK with no usable
    content). Previously this caused ParseRawMessageFromRecords to propagate
    a hard error that aborted the entire collectApiTimelineRecords subtask.
    
    Two complementary fixes:
    
    1. Add ignoreInvalidTimelineResponse AfterResponse handler that supersedes
       ignoreDeletedBuilds: it still skips 404s (deleted builds) and now also
       skips responses with empty or non-JSON bodies by returning
       api.ErrIgnoreAndContinue, leaving the rest of the pipeline intact.
       The body is peeked and then restored via io.NopCloser so the downstream
       ResponseParser can still read it when the body is valid JSON.
    
    2. Extend the DB query in CollectRecords to exclude builds with
       result = 'none' in addition to result = 'failed'. Builds broken by
       YAML syntax errors are categorised as 'none' by Azure DevOps (already
       mapped to RESULT_FAILURE in cicdBuildResultRule), so fetching their
       timeline is both unnecessary and the primary source of the bad
       responses. Using NOT IN ? with a []string slice follows the same
       pattern used in blueprint_helper.go.
    
    Unit tests added for ignoreInvalidTimelineResponse covering: 404,
    empty body, non-JSON body, valid JSON (nil error + body restored),
    and valid empty JSON object.
    
    Fixes: #8838
    Signed-off-by: Cornelius Schuchardt <co...@beck.de>
    
    * fix(azuredevops_go): remove unused ignoreDeletedBuilds function
    
    The ignoreDeletedBuilds function was replaced by
    ignoreInvalidTimelineResponse
    in ci_cd_timeline_records_collector.go but was left behind in shared.go.
    This caused the 'unused' linter to fail with exit code 2.
    
    * fix: resolve govet non-constant format string warnings
    
    Use constant format verbs instead of dynamic strings passed directly
    as format arguments to avoid govet printf-family warnings:
    
    - core/migration/linter: fmt.Errorf with string concat replaced by %s verb
    - impls/logruslog: Warn/Error pass pre-formatted message via "%s" arg
      instead of using it directly as the format string
    
    ---------
    
    Signed-off-by: Cornelius Schuchardt <co...@beck.de>
---
 backend/core/migration/linter/main.go              |  2 +-
 backend/impls/logruslog/logger.go                  |  4 +-
 .../tasks/ci_cd_timeline_records_collector.go      |  4 +-
 backend/plugins/azuredevops_go/tasks/shared.go     | 29 ++++++++-
 .../plugins/azuredevops_go/tasks/shared_test.go    | 72 ++++++++++++++++++++++
 5 files changed, 103 insertions(+), 8 deletions(-)

diff --git a/backend/core/migration/linter/main.go b/backend/core/migration/linter/main.go
index bf9190b72..11aae870c 100644
--- a/backend/core/migration/linter/main.go
+++ b/backend/core/migration/linter/main.go
@@ -51,7 +51,7 @@ func firstLineFromFile(path string) string {
 	for scanner.Scan() {
 		return scanner.Text()
 	}
-	panic(fmt.Errorf("empty file: " + path))
+	panic(fmt.Errorf("empty file: %s", path))
 }
 
 const (
diff --git a/backend/impls/logruslog/logger.go b/backend/impls/logruslog/logger.go
index 857d75ac2..88287c9f5 100644
--- a/backend/impls/logruslog/logger.go
+++ b/backend/impls/logruslog/logger.go
@@ -72,11 +72,11 @@ func (l *DefaultLogger) Info(format string, a ...interface{}) {
 }
 
 func (l *DefaultLogger) Warn(err error, format string, a ...interface{}) {
-	l.Log(log.LOG_WARN, formatMessage(err, format, a...))
+	l.Log(log.LOG_WARN, "%s", formatMessage(err, format, a...))
 }
 
 func (l *DefaultLogger) Error(err error, format string, a ...interface{}) {
-	l.Log(log.LOG_ERROR, formatMessage(err, format, a...))
+	l.Log(log.LOG_ERROR, "%s", formatMessage(err, format, a...))
 }
 
 func (l *DefaultLogger) SetStream(config *log.LoggerStreamConfig) {
diff --git a/backend/plugins/azuredevops_go/tasks/ci_cd_timeline_records_collector.go b/backend/plugins/azuredevops_go/tasks/ci_cd_timeline_records_collector.go
index 07d4b7627..9ac766917 100644
--- a/backend/plugins/azuredevops_go/tasks/ci_cd_timeline_records_collector.go
+++ b/backend/plugins/azuredevops_go/tasks/ci_cd_timeline_records_collector.go
@@ -49,7 +49,7 @@ func CollectRecords(taskCtx plugin.SubTaskContext) errors.Error {
 	cursor, err := db.Cursor(
 		dal.Select("azuredevops_id"),
 		dal.From(models.AzuredevopsBuild{}.TableName()),
-		dal.Where("repository_id = ? and connection_id=? and result != ?", data.Options.RepositoryId, data.Options.ConnectionId, "failed"),
+		dal.Where("repository_id = ? and connection_id=? and result NOT IN ?", data.Options.RepositoryId, data.Options.ConnectionId, []string{"failed", "none"}),
 	)
 	if err != nil {
 		return err
@@ -68,7 +68,7 @@ func CollectRecords(taskCtx plugin.SubTaskContext) errors.Error {
 		UrlTemplate:        "{{ .Params.OrganizationId }}/{{ .Params.ProjectId }}/_apis/build/builds/{{ .Input.AzuredevopsId }}/Timeline?api-version=7.1",
 		Query:              BuildPaginator(true),
 		ResponseParser:     ParseRawMessageFromRecords,
-		AfterResponse:      ignoreDeletedBuilds, // Ignore the 404 response if builds are deleted during the collection
+		AfterResponse:      ignoreInvalidTimelineResponse, // Skip builds with missing/malformed timelines (e.g. YAML syntax errors)
 	})
 	if err != nil {
 		return err
diff --git a/backend/plugins/azuredevops_go/tasks/shared.go b/backend/plugins/azuredevops_go/tasks/shared.go
index 3ceae7c98..e6fe0eca1 100644
--- a/backend/plugins/azuredevops_go/tasks/shared.go
+++ b/backend/plugins/azuredevops_go/tasks/shared.go
@@ -18,14 +18,17 @@ limitations under the License.
 package tasks
 
 import (
+	"bytes"
 	"encoding/json"
 	"fmt"
+	"io"
+	"net/http"
+	"net/url"
+
 	"github.com/apache/incubator-devlake/core/errors"
 	"github.com/apache/incubator-devlake/core/models/domainlayer/devops"
 	"github.com/apache/incubator-devlake/core/plugin"
 	"github.com/apache/incubator-devlake/helpers/pluginhelper/api"
-	"net/http"
-	"net/url"
 )
 
 // Build and TimeLine Record State and Result types can be found here:
@@ -145,9 +148,29 @@ func change203To401(res *http.Response) errors.Error {
 	return nil
 }
 
-func ignoreDeletedBuilds(res *http.Response) errors.Error {
+// ignoreInvalidTimelineResponse is an AfterResponse handler for the Timeline API.
+// It skips builds whose timeline response is missing or unparseable (e.g. builds
+// that failed due to a YAML syntax error never produce a usable timeline), instead
+// of aborting the entire subtask.
+func ignoreInvalidTimelineResponse(res *http.Response) errors.Error {
+	// Keep existing behaviour: treat 404 as a graceful skip (build was deleted).
 	if res.StatusCode == http.StatusNotFound {
 		return api.ErrIgnoreAndContinue
 	}
+
+	// Read the body so we can inspect it, then restore it for the ResponseParser.
+	body, err := io.ReadAll(res.Body)
+	_ = res.Body.Close()
+	if err != nil {
+		return api.ErrIgnoreAndContinue
+	}
+	res.Body = io.NopCloser(bytes.NewReader(body))
+
+	// An empty body or non-JSON body means the timeline is not available.
+	// Return ErrIgnoreAndContinue so the build is skipped without failing the subtask.
+	if len(body) == 0 || !json.Valid(body) {
+		return api.ErrIgnoreAndContinue
+	}
+
 	return nil
 }
diff --git a/backend/plugins/azuredevops_go/tasks/shared_test.go b/backend/plugins/azuredevops_go/tasks/shared_test.go
new file mode 100644
index 000000000..353ae3f4c
--- /dev/null
+++ b/backend/plugins/azuredevops_go/tasks/shared_test.go
@@ -0,0 +1,72 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements.  See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License.  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package tasks
+
+import (
+	"bytes"
+	"io"
+	"net/http"
+	"testing"
+
+	"github.com/apache/incubator-devlake/helpers/pluginhelper/api"
+	"github.com/stretchr/testify/assert"
+)
+
+func makeResponse(statusCode int, body string) *http.Response {
+	return &http.Response{
+		StatusCode: statusCode,
+		Body:       io.NopCloser(bytes.NewBufferString(body)),
+		Request:    &http.Request{},
+	}
+}
+
+func TestIgnoreInvalidTimelineResponse_404(t *testing.T) {
+	res := makeResponse(http.StatusNotFound, "")
+	err := ignoreInvalidTimelineResponse(res)
+	assert.Equal(t, api.ErrIgnoreAndContinue, err, "404 should return ErrIgnoreAndContinue")
+}
+
+func TestIgnoreInvalidTimelineResponse_EmptyBody(t *testing.T) {
+	res := makeResponse(http.StatusOK, "")
+	err := ignoreInvalidTimelineResponse(res)
+	assert.Equal(t, api.ErrIgnoreAndContinue, err, "empty body should return ErrIgnoreAndContinue")
+}
+
+func TestIgnoreInvalidTimelineResponse_NonJSONBody(t *testing.T) {
+	res := makeResponse(http.StatusOK, "not json at all")
+	err := ignoreInvalidTimelineResponse(res)
+	assert.Equal(t, api.ErrIgnoreAndContinue, err, "non-JSON body should return ErrIgnoreAndContinue")
+}
+
+func TestIgnoreInvalidTimelineResponse_ValidJSON_ReturnsNil(t *testing.T) {
+	validJSON := `{"records":[{"id":"abc","type":"Job","name":"Build"}]}`
+	res := makeResponse(http.StatusOK, validJSON)
+	err := ignoreInvalidTimelineResponse(res)
+	assert.Nil(t, err, "valid JSON body should return nil")
+
+	// Verify body is still readable after the handler restored it.
+	remaining, readErr := io.ReadAll(res.Body)
+	assert.NoError(t, readErr)
+	assert.Equal(t, validJSON, string(remaining), "body should be restored for downstream parser")
+}
+
+func TestIgnoreInvalidTimelineResponse_ValidEmptyJSONObject(t *testing.T) {
+	res := makeResponse(http.StatusOK, "{}")
+	err := ignoreInvalidTimelineResponse(res)
+	assert.Nil(t, err, "valid empty JSON object should return nil")
+}