Fix/syncer minor fixes

CHANGELOG.md

@@ -27,6 +27,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Bugfixes
 
 - Fixed package blacklist changes not appearing in UI immediately after save
+- Fixed the syncer crashing the Nebraska process when the upstream server returns a malformed or empty Omaha response
+- Fixed mandatory floor packages being skipped when an instance reports a pre-release of the floor's version
 
 ## [3.0.0] - 28/11/2025
 

backend/pkg/api/packages_floors.go

@@ -5,7 +5,9 @@ import (
 	"errors"
 	"fmt"
 	"regexp"
+	"sort"
 
+	"github.com/blang/semver/v4"
 	"github.com/doug-martin/goqu/v9"
 	"gopkg.in/guregu/null.v4"
 )
@@ -26,29 +28,6 @@ func semverToIntArray(column string) (string, error) {
 	return fmt.Sprintf("string_to_array((regexp_split_to_array(%s, '[+-]'))[1], '.')::int[]", column), nil
 }
 
-// versionCompareExpr creates a version comparison expression
-func versionCompareExpr(column, operator, value string) (goqu.Expression, error) {
-	// Validate operator to prevent SQL injection
-	validOperators := map[string]bool{
-		">": true, ">=": true, "<": true, "<=": true, "=": true, "!=": true,
-	}
-	if !validOperators[operator] {
-		return nil, fmt.Errorf("versionCompareExpr: invalid operator %q - potential SQL injection", operator)
-	}
-
-	colArray, err := semverToIntArray(column)
-	if err != nil {
-		return nil, err
-	}
-
-	valArray, err := semverToIntArray("?")
-	if err != nil {
-		return nil, err
-	}
-
-	return goqu.L(fmt.Sprintf("%s %s %s", colArray, operator, valArray), value), nil
-}
-
 // AddChannelPackageFloor marks a package as a floor for a specific channel
 func (api *API) AddChannelPackageFloor(channelID, packageID string, floorReason null.String) error {
 	// Verify channel and package exist and are compatible in a single query
@@ -226,7 +205,9 @@ const (
 	DefaultMaxFloorsPerResponse = 5
 )
 
-// GetRequiredChannelFloors returns floor packages between instance and target versions for a channel
+// GetRequiredChannelFloors returns the floor packages between the instance version and
+// the channel target (instance < floor <= target), sorted ascending by semantic version
+// and capped at the configured limit.
 func (api *API) GetRequiredChannelFloors(channel *Channel, instanceVersion string) ([]*Package, error) {
 	if channel == nil || channel.Package == nil {
 		return nil, ErrNoPackageFound
@@ -235,52 +216,55 @@ func (api *API) GetRequiredChannelFloors(channel *Channel, instanceVersion strin
 		return nil, fmt.Errorf("instance version cannot be empty")
 	}
 
-	targetVersion := channel.Package.Version
-
-	maxFloorsPerResponse := DefaultMaxFloorsPerResponse
+	limit := DefaultMaxFloorsPerResponse
 	if api.maxFloorsPerResponse > 0 {
-		maxFloorsPerResponse = api.maxFloorsPerResponse
+		limit = api.maxFloorsPerResponse
 	}
 
-	// No blacklist check needed for floors
-	gtExpr, err := versionCompareExpr("p.version", ">", instanceVersion)
+	allFloors, err := api.GetChannelFloorPackages(channel.ID)
 	if err != nil {
 		return nil, err
 	}
 
-	lteExpr, err := versionCompareExpr("p.version", "<=", targetVersion)
+	floors, _, err := selectFloorsInRange(allFloors, instanceVersion, channel.Package.Version, limit)
+	return floors, err
+}
+
+// selectFloorsInRange returns the floor packages with instanceVersion < v <= targetVersion,
+// sorted ascending by semantic version and capped at limit. hasMore is true when more
+// floors fall in range than the limit.
+func selectFloorsInRange(floors []*Package, instanceVersion, targetVersion string, limit int) ([]*Package, bool, error) {
+	inst, err := semver.Make(instanceVersion)
 	if err != nil {
-		return nil, err
+		return nil, false, fmt.Errorf("invalid instance version %q: %w", instanceVersion, err)
 	}
-
-	semverExpr, err := semverToIntArray("p.version")
+	target, err := semver.Make(targetVersion)
 	if err != nil {
-		return nil, err
+		return nil, false, fmt.Errorf("invalid target version %q: %w", targetVersion, err)
 	}
 
-	query, _, err := goqu.From(goqu.L(`
-		package p
-		JOIN channel_package_floors cpf ON p.id = cpf.package_id
-	`)).
-		Select(goqu.L(`
-			p.*,
-			true as is_floor,
-			cpf.floor_reason
-		`)).
-		Where(goqu.And(
-			goqu.C("channel_id").Table("cpf").Eq(channel.ID),
-			gtExpr,
-			lteExpr,
-		)).
-		Order(goqu.L(semverExpr).Asc()).
-		Limit(uint(maxFloorsPerResponse)).
-		ToSQL()
-
-	if err != nil {
-		return nil, err
+	inRange := make([]*Package, 0, len(floors))
+	for _, p := range floors {
+		v, err := semver.Make(p.Version)
+		if err != nil {
+			continue // versions are validated on insert; skip defensively
+		}
+		if v.GT(inst) && v.LTE(target) {
+			inRange = append(inRange, p)
+		}
 	}
 
-	return api.getPackagesFromQuery(query)
+	sort.SliceStable(inRange, func(i, j int) bool {
+		vi, _ := semver.Make(inRange[i].Version)
+		vj, _ := semver.Make(inRange[j].Version)
+		return vi.LT(vj)
+	})
+
+	hasMore := len(inRange) > limit
+	if hasMore {
+		inRange = inRange[:limit]
+	}
+	return inRange, hasMore, nil
 }
 
 // GetChannelFloorPackagesCount returns the count of floor packages for a channel

backend/pkg/api/packages_floors_test.go

@@ -291,3 +291,63 @@ func TestFloorBlacklistConflict(t *testing.T) {
 		assert.Error(t, err)
 	})
 }
+
+// TestSelectFloorsInRange covers the floor version range, ordering, and limit logic,
+// including pre-release and build metadata precedence.
+func TestSelectFloorsInRange(t *testing.T) {
+	mk := func(vs ...string) []*Package {
+		ps := make([]*Package, len(vs))
+		for i, v := range vs {
+			ps[i] = &Package{Version: v}
+		}
+		return ps
+	}
+	vers := func(ps []*Package) []string {
+		out := make([]string, 0, len(ps))
+		for _, p := range ps {
+			out = append(out, p.Version)
+		}
+		return out
+	}
+
+	tests := []struct {
+		name             string
+		floors           []*Package
+		instance, target string
+		limit            int
+		want             []string
+		wantMore         bool
+	}{
+		{"release floor not skipped over pre-release instance", mk("2000.0.0"), "2000.0.0-rc1", "3000.0.0", 5, []string{"2000.0.0"}, false},
+		{"same-core floors ordered, none dropped", mk("2000.0.0", "2000.0.0-beta", "2000.0.0-alpha"), "1000.0.0", "3000.0.0", 5, []string{"2000.0.0-alpha", "2000.0.0-beta", "2000.0.0"}, false},
+		{"lower pre-release below release instance excluded", mk("2000.0.0-rc1"), "2000.0.0", "3000.0.0", 5, []string{}, false},
+		{"build metadata treated as the release", mk("2000.0.0+build"), "1999.0.0", "3000.0.0", 5, []string{"2000.0.0+build"}, false},
+		{"shuffled input sorted, limited, hasMore", mk("3000.0.0", "1000.0.0", "2000.0.0"), "500.0.0", "4000.0.0", 2, []string{"1000.0.0", "2000.0.0"}, true},
+		{"target inclusive, above-target excluded", mk("2000.0.0", "4000.0.0"), "1000.0.0", "3000.0.0", 5, []string{"2000.0.0"}, false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			res, more, err := selectFloorsInRange(tt.floors, tt.instance, tt.target, tt.limit)
+			require.NoError(t, err)
+			assert.Equal(t, tt.want, vers(res))
+			assert.Equal(t, tt.wantMore, more)
+		})
+	}
+}
+
+// TestFloorPreReleaseNotSkipped checks that a release floor is still returned when the
+// instance reports a pre-release of the same version.
+func TestFloorPreReleaseNotSkipped(t *testing.T) {
+	a := newForTest(t)
+	defer a.Close()
+
+	setup := setupFloors(t, a, "prerelease", []string{"2000.0.0"}, "3000.0.0")
+
+	ch, err := a.GetChannel(setup.Channel.ID)
+	require.NoError(t, err)
+	floors, err := a.GetRequiredChannelFloors(ch, "2000.0.0-rc1")
+	require.NoError(t, err)
+	require.Len(t, floors, 1)
+	assert.Equal(t, "2000.0.0", floors[0].Version)
+}

backend/pkg/syncer/syncer.go

@@ -220,7 +220,11 @@ func (s *Syncer) checkForUpdates() error {
 				return err
 			}
 		} else {
-			l.Debug().Str("channel", descriptor.name).Str("arch", descriptor.arch.String()).Str("currentVersion", currentVersion).Msgf("checkForUpdates, no update available updateStatus %v", update.Status)
+			status := "no updatecheck in response"
+			if update != nil {
+				status = string(update.Status)
+			}
+			l.Debug().Str("channel", descriptor.name).Str("arch", descriptor.arch.String()).Str("currentVersion", currentVersion).Msgf("checkForUpdates, no update available updateStatus %v", status)
 		}
 
 		select {
@@ -273,13 +277,27 @@ func (s *Syncer) doOmahaRequest(descriptor channelDescriptor, currentVersion str
 	}
 	l.Debug().Str("response", string(body)).Msg("doOmahaRequest")
 
-	oresp := &omaha.Response{}
-	err = xml.Unmarshal(body, oresp)
+	update, err := parseOmahaUpdateResponse(body)
 	if err != nil {
-		l.Error().Err(err).Msg("checkForUpdates, unmarshalling omaha response")
+		l.Error().Err(err).Msg("checkForUpdates, parsing omaha response")
 		return nil, err
 	}
 
+	return update, nil
+}
+
+// parseOmahaUpdateResponse unmarshals an Omaha response body and returns the
+// update check of its first app. It returns an error (rather than panicking)
+// when the body is well-formed XML but carries no <app> element. The returned
+// update check may be nil when the app has no <updatecheck>.
+func parseOmahaUpdateResponse(body []byte) (*omaha.UpdateResponse, error) {
+	oresp := &omaha.Response{}
+	if err := xml.Unmarshal(body, oresp); err != nil {
+		return nil, err
+	}
+	if len(oresp.Apps) == 0 {
+		return nil, fmt.Errorf("omaha response contained no apps")
+	}
 	return oresp.Apps[0].UpdateCheck, nil
 }
 
@@ -313,6 +331,14 @@ func (s *Syncer) processSingleManifestUpdate(descriptor channelDescriptor, updat
 		return err
 	}
 
+	// A single manifest can still be a floor (e.g. the channel target is itself a
+	// floor). Record it as a floor so downstream clients cannot skip it.
+	if manifest.IsFloor {
+		if err := s.markPackageAsFloor(descriptor, pkg, manifest); err != nil {
+			return err
+		}
+	}
+
 	// Update channel to point to the package
 	if err := s.updateChannelToPackage(descriptor, pkg); err != nil {
 		l.Error().Err(err).

backend/pkg/syncer/syncer_parse_test.go

@@ -0,0 +1,56 @@
+package syncer
+
+import (
+	"encoding/xml"
+	"testing"
+
+	"github.com/flatcar/go-omaha/omaha"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestParseOmahaUpdateResponse covers the guard that previously let a malformed
+// upstream response panic the whole syncer process: a response with no <app>
+// must yield an error, and an <app> without <updatecheck> must yield a nil
+// update check (not a panic).
+func TestParseOmahaUpdateResponse(t *testing.T) {
+	const appID = "e96281a6-d1af-4bde-9a0a-97b76e56dc57"
+
+	t.Run("app with updatecheck returns it", func(t *testing.T) {
+		resp := omaha.NewResponse()
+		resp.AddApp(appID, omaha.AppOK).AddUpdateCheck(omaha.UpdateOK)
+		body, err := xml.Marshal(resp)
+		require.NoError(t, err)
+
+		uc, err := parseOmahaUpdateResponse(body)
+		require.NoError(t, err)
+		require.NotNil(t, uc)
+		assert.Equal(t, omaha.UpdateOK, uc.Status)
+	})
+
+	t.Run("no app element returns an error, not a panic", func(t *testing.T) {
+		body, err := xml.Marshal(omaha.NewResponse())
+		require.NoError(t, err)
+
+		uc, err := parseOmahaUpdateResponse(body)
+		require.Error(t, err)
+		assert.Nil(t, uc)
+		assert.Contains(t, err.Error(), "no apps")
+	})
+
+	t.Run("app without updatecheck returns nil and no error", func(t *testing.T) {
+		resp := omaha.NewResponse()
+		resp.AddApp(appID, omaha.AppOK)
+		body, err := xml.Marshal(resp)
+		require.NoError(t, err)
+
+		uc, err := parseOmahaUpdateResponse(body)
+		require.NoError(t, err)
+		assert.Nil(t, uc)
+	})
+
+	t.Run("malformed body returns an error", func(t *testing.T) {
+		_, err := parseOmahaUpdateResponse([]byte("not xml at all"))
+		require.Error(t, err)
+	})
+}