diff --git a/cmd/admin/settings.go b/cmd/admin/settings.go index f902cef1..68ce6522 100644 --- a/cmd/admin/settings.go +++ b/cmd/admin/settings.go @@ -26,7 +26,7 @@ func loadingSettings(mgr *settings.Settings, cfg *config.ServiceParameters) erro } // Check if service settings for node inactive hours is ready if !mgr.IsValue(config.ServiceAdmin, settings.InactiveHours, settings.NoEnvironmentID) { - if err := mgr.NewIntegerValue(config.ServiceAdmin, settings.InactiveHours, int64(defaultInactive), settings.NoEnvironmentID); err != nil { + if err := mgr.NewIntegerValue(config.ServiceAdmin, settings.InactiveHours, settings.DefaultInactiveHours, settings.NoEnvironmentID); err != nil { return fmt.Errorf("failed to add %s to configuration: %w", settings.InactiveHours, err) } } diff --git a/cmd/api/settings.go b/cmd/api/settings.go index 333743c2..3661d33f 100644 --- a/cmd/api/settings.go +++ b/cmd/api/settings.go @@ -29,6 +29,15 @@ func loadingSettings(mgr *settings.Settings, cfg *config.ServiceParameters) erro return fmt.Errorf("failed to add %s to settings: %w", settings.RefreshSettings, err) } } + // Ensure the inactive_hours admin setting exists so the API service can + // correctly classify nodes as active/inactive even if the admin service + // has never been started. The setting is stored under ServiceAdmin (read + // by all services via InactiveHours); we seed it here defensively. + if !mgr.IsValue(config.ServiceAdmin, settings.InactiveHours, settings.NoEnvironmentID) { + if err := mgr.NewIntegerValue(config.ServiceAdmin, settings.InactiveHours, settings.DefaultInactiveHours, settings.NoEnvironmentID); err != nil { + return fmt.Errorf("failed to add %s to configuration: %w", settings.InactiveHours, err) + } + } // Write JSON config to settings if err := mgr.SetAPIJSON(cfg, settings.NoEnvironmentID); err != nil { return fmt.Errorf("failed to add JSON values to configuration: %w", err) diff --git a/frontend/src/features/dashboard/DashboardPage.test.tsx b/frontend/src/features/dashboard/DashboardPage.test.tsx index 757e80c9..ada676ce 100644 --- a/frontend/src/features/dashboard/DashboardPage.test.tsx +++ b/frontend/src/features/dashboard/DashboardPage.test.tsx @@ -224,4 +224,17 @@ describe('DashboardPage', () => { ); expect(screen.getByText('Retry')).toBeInTheDocument(); }); + + // Regression: when the backend returns inactive_hours: 0 (e.g. setting + // missing from DB), the dashboard must fall back to the default 72h + // label rather than showing "Inactive >= 0h". + it('falls back to default inactive threshold when API returns 0', async () => { + mockGetStats.mockResolvedValue(makeStatsResponse({ inactive_hours: 0 })); + renderWithProviders(makeTestRouter()); + + await waitFor(() => expect(screen.getByText('Active Nodes')).toBeInTheDocument()); + + expect(screen.getByText('Inactive \u2265 72h')).toBeInTheDocument(); + expect(screen.queryByText('Inactive \u2265 0h')).not.toBeInTheDocument(); + }); }); diff --git a/frontend/src/features/dashboard/DashboardPage.tsx b/frontend/src/features/dashboard/DashboardPage.tsx index 41f5f100..e3e26ee0 100644 --- a/frontend/src/features/dashboard/DashboardPage.tsx +++ b/frontend/src/features/dashboard/DashboardPage.tsx @@ -1092,7 +1092,10 @@ export function DashboardPage() { refetchInterval: 30_000, refetchIntervalInBackground: false, }); - const inactiveHours = data?.inactive_hours ?? DEFAULT_INACTIVE_HOURS; + const inactiveHours = + data?.inactive_hours && data.inactive_hours > 0 + ? data.inactive_hours + : DEFAULT_INACTIVE_HOURS; const [palette, setPaletteEntry, resetPalette] = useChartPalette(); diff --git a/pkg/settings/settings.go b/pkg/settings/settings.go index 2db0c9fc..a971d6ac 100644 --- a/pkg/settings/settings.go +++ b/pkg/settings/settings.go @@ -102,6 +102,13 @@ const ( NoEnvironmentID = iota ) +// DefaultInactiveHours is the fallback threshold (in hours) for classifying +// nodes as active vs inactive when the inactive_hours setting is absent or +// invalid. Keeps every service — including the API, which does not seed the +// setting itself — from treating all nodes as inactive when the DB row is +// missing. +const DefaultInactiveHours int64 = 72 + // SettingValue to hold each value for settings type SettingValue struct { gorm.Model @@ -563,11 +570,17 @@ func (conf *Settings) CleanupExpired() int64 { return value.Integer } -// InactiveHours gets the value in hours for a node to be inactive by service +// InactiveHours gets the value in hours for a node to be inactive by service. +// Returns DefaultInactiveHours when the setting is absent or invalid so that +// callers never receive a zero threshold (which would make every node appear +// inactive). func (conf *Settings) InactiveHours(envID uint) int64 { value, err := conf.RetrieveValue(config.ServiceAdmin, InactiveHours, envID) if err != nil { - return 0 + return DefaultInactiveHours + } + if value.Integer <= 0 { + return DefaultInactiveHours } return value.Integer } diff --git a/pkg/settings/settings_test.go b/pkg/settings/settings_test.go new file mode 100644 index 00000000..17750782 --- /dev/null +++ b/pkg/settings/settings_test.go @@ -0,0 +1,68 @@ +package settings + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/driver/sqlite" + "gorm.io/gorm" + + "github.com/jmpsec/osctrl/pkg/config" +) + +func setupSettingsTestDB(t *testing.T) *gorm.DB { + t.Helper() + // Use a unique DSN per test so in-memory databases don't bleed into + // each other (file::memory:?cache=shared is a single shared DB). + db, err := gorm.Open(sqlite.Open("file:"+t.Name()+"?mode=memory"), &gorm.Config{}) + require.NoError(t, err, "Failed to open in-memory database") + require.NoError(t, db.AutoMigrate(&SettingValue{}), "Failed to migrate schema") + return db +} + +// InactiveHours must return DefaultInactiveHours when the setting row is +// missing from the DB. This is the regression for the bug where the API +// service (which does not seed inactive_hours) would get 0 hours, causing +// every node to appear inactive on the dashboard. +func TestInactiveHours_DefaultsWhenMissing(t *testing.T) { + db := setupSettingsTestDB(t) + conf := &Settings{DB: db} + + got := conf.InactiveHours(NoEnvironmentID) + assert.Equal(t, DefaultInactiveHours, got, + "InactiveHours should fall back to DefaultInactiveHours when setting is absent") +} + +// InactiveHours must return DefaultInactiveHours when the stored value is 0 +// or negative, since a zero threshold makes every node inactive. +func TestInactiveHours_DefaultsOnNonPositive(t *testing.T) { + tests := []struct { + name string + value int64 + }{ + {"zero value", 0}, + {"negative value", -5}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + db := setupSettingsTestDB(t) + conf := &Settings{DB: db} + require.NoError(t, conf.NewIntegerValue(config.ServiceAdmin, InactiveHours, tt.value, NoEnvironmentID)) + got := conf.InactiveHours(NoEnvironmentID) + assert.Equal(t, DefaultInactiveHours, got, + "InactiveHours should fall back to DefaultInactiveHours for non-positive stored value") + }) + } +} + +// InactiveHours must return the configured value when it is positive. +func TestInactiveHours_ReturnsConfigured(t *testing.T) { + db := setupSettingsTestDB(t) + conf := &Settings{DB: db} + const configured = int64(168) + require.NoError(t, conf.NewIntegerValue(config.ServiceAdmin, InactiveHours, configured, NoEnvironmentID)) + got := conf.InactiveHours(NoEnvironmentID) + assert.Equal(t, configured, got, + "InactiveHours should return the stored positive value") +}