fix(health): skip immediate health check on import when health is disabled#689
Merged
Conversation
…abled ScheduleHealthCheck only guarded on a nil healthRepo, never on the health.enabled config flag. Since health checking is disabled by default, every successful import queued file_health rows as pending (and resolved pending repairs) for a feature the user had turned off; the worker then refused to process them, leaving orphaned rows forever. Add an early-return guard on GetHealthEnabled() before any path expansion, queuing, or repair resolution. Enable health in the shared scheduler test helper (its intent is to exercise scheduling) and add a test asserting the disabled path queues nothing and leaves stale repairs untouched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When health checking is disabled in config (
health.enabled: false), successfully imported items were still queued for an immediate health check. Records were written to thefile_healthtable aspendingand stayed there forever — the health worker correctly refuses to process them (internal/health/worker.go:128), but they should never have been queued in the first place.This affected the default install, since health checking is disabled by default (
internal/config/manager.go:1439).Root cause
Coordinator.ScheduleHealthCheckis called unconditionally after every successful import (coordinator.go:133). The scheduler only guarded onc.healthRepo == nil(a dependency check) and never checkedcfg.GetHealthEnabled(), so it queued records and resolved pending repairs regardless of the config flag.Change
health_scheduler.go: Added an early-return guard onc.configGetter().GetHealthEnabled()right after thehealthRepo == nilcheck — before any path expansion, record building, batch upsert, or pending-repair resolution. Skipping repair resolution when disabled is intentional and consistent: both are health-subsystem concerns.health_scheduler_test.go: AddedTestScheduleHealthCheck_DisabledQueuesNothing(asserts no records queued and stale repairs untouched when disabled). SetHealth.Enabled = truein the sharedsetupSchedulerTesthelper so the existing scheduling tests run with health on — their actual intent. They previously passed only because of the bug, sinceDefaultConfig()disables health.Verification
go test ./internal/importer/postprocessor/... -run HealthCheck -race— passgo build ./...— cleango test ./internal/importer/...— all greenReviewer notes
This fixes the import-time trigger specifically. A separate library-sync path can also register health records; that was out of scope for this report ("imported items sent to immediate health check") but may be worth a follow-up check on whether it's also gated on
health.enabled.