fix(validation): honor segment sample percentage on large releases (#812) - #817
Merged
Conversation
) Segment sampling applied a hard 55-sample ceiling after the percentage math, so on any release with more than ~1,100 segments every setting from 1% to 99% collapsed onto the same 55 STATs. Only 100% behaved as configured, because it takes an early-return path that skips sampling entirely. Remove the ceiling in both sampling paths: - selectSegmentsForValidation (health checks, the settings slider) now honors the configured percentage exactly, keeping the min-5 floor for statistical validity. - selectFastFailSegments (import fast-fail, per file) had the same maxSamples = 55 ceiling, which capped even 100% sampling. It now takes first + last + exactly pct% of the middle; the redundant clamp is gone and slice capacity is sized from the real sample count. The two tests that asserted the cap are rewritten as regressions. Note: this raises default sampling cost. At the 5% health default a ~7,000-segment release moves from 55 STATs to ~350 per check; the 1% import default is milder.
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
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.
Fixes #812.
Problem
The segment scan percentage setting had no effect below 100%. Sampling applied a hard 55-sample ceiling after the percentage math, so on any release with more than ~1,100 segments every setting from 1% to 99% collapsed onto the same 55 STATs. Only 100% behaved as configured, because it takes an early-return path that skips sampling entirely — which is exactly the asymmetry the reporter observed.
The UI presents this as a plain percentage slider ("1% (FAST)" → "100% (SLOW)") with no mention of a cap, so the ceiling was silently overriding an explicit user choice.
Changes
internal/usenet/validation.go—selectSegmentsForValidation(health checks; the path the settings slider drives) dropsmin(..., 55)and honors the configured percentage exactly. The min-5 floor for statistical validity is kept.internal/importer/validation/fast_fail.go—selectFastFailSegments(import fast-fail, per file) had the samemaxSamples = 55ceiling, which capped even 100% sampling. It now takes first + last + exactlypct%of the middle; the redundant clamp is removed and slice capacity is sized from the real sample count.≤55comments inprocessor.goand theFastFailReleaseProbedoc updated.Reviewer notes
This raises default sampling cost. The 55 cap was doing real work to keep out-of-box checks cheap. At the 5% health default, a ~7,000-segment release moves from 55 STATs to ~350 per check; the 1% import default is milder. That is the intended meaning of the setting, but if it proves too aggressive in practice, lowering the shipped default in
manager.go:1663is a one-line follow-up. The alternative considered was making the cap a new config knob — rejected as extra surface for a setting that should already mean what it says.Testing
The two tests that asserted the 55 cap are rewritten as
#812regressions, and were confirmed red against the old code (got 55, want 2000) before the fix:go build ./...,go vet ./internal/...and the fullgo test ./...pass;internal/usenet,internal/importer,internal/importer/validationandinternal/healthre-verified with-race.