Repository navigation
🐛 fix: alert on runtime classifier failures - #14528
hivecommons-hive[bot] merged 2 commits into
Conversation
Hive-Run: hivecommons#14509 Hive-Plan: runtime classifier alert Hive-Spec: ioscan-classifier#fail-closed Signed-off-by: Karibhou <157208370+Karibhou@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Correctness review (head 452daac)
Runtime alert is sticky across transient errors — raised, never cleared until restart. src/pkg/scheduler/ioscan_enforce.go:255 calls s.classifierUnavailable(err) on every fail-closed Score error, and the boot callback (src/cmd/hive/ioscan_classifier_boot.go:49-51) only ever calls raiseIoscanClassifierAlert. The only clearIoscanClassifierAlert calls remain in installIoscanClassifier (boot-only; sole production call site src/cmd/hive/main.go:2782). So one timed-out HTTP call to the LLM endpoint at L5/L6 permanently raises an error-severity banner whose text (ioscan_classifier_boot.go:155-159) tells the operator the classifier "failed or is unavailable" and to reconfigure/restart — even though the very next Score succeeded and kicks are flowing again. Construction-time failures (UnavailableClassifier) keep failing every call so they stay raised either way; this only affects genuinely transient runtime errors, but those are the common case for a network endpoint.
Suggested fix — signal recovery with a nil error and clear on it:
--- a/src/pkg/scheduler/ioscan_enforce.go
+++ b/src/pkg/scheduler/ioscan_enforce.go
@@ enforceTextFrom
return sanitized, v
}
+ // A successful score after a fail-closed error means the classifier is
+ // back; let the dashboard drop the unavailable banner.
+ s.classifierUnavailable(nil)
action := ioscan.ActionForScore(score, thresholds, s.classifierFailClosed())
--- a/src/cmd/hive/ioscan_classifier_boot.go
+++ b/src/cmd/hive/ioscan_classifier_boot.go
@@ installIoscanClassifier
sched.SetClassifierUnavailableFunc(func(err error) {
+ if err == nil {
+ clearIoscanClassifierAlert(alerts)
+ return
+ }
raiseIoscanClassifierAlert(alerts, acmmLevel, err)
})(Optionally gate the clear behind a sync.Bool/flag set on raise so the dashboard mutex isn't taken on every successful score; AddSystemAlert/ClearSystemAlert at src/pkg/dashboard/server.go:2655-2678 are cheap slice scans, so it's not strictly required.) Worth adding a test that a success after a failure clears alerts.blocked().
Everything else checks out: callback runs after s.mu.RUnlock() (ioscan_enforce.go:425-431), no lock held in enforceTextFrom when it fires; AddSystemAlert replaces by id so per-call errors don't accumulate; quota path returns before the error branch (ioscan_enforce.go:238-249) as the new test asserts; fakeClassifierSink is the only other ioscanClassifierSink implementor.
— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the diff against v5 plus the enforce path and alert sink. This does what #14509 asked — raises the existing ioscan-classifier-blocked banner from the runtime fail-closed path — and looks correct to me.
What I verified:
src/pkg/scheduler/ioscan_enforce.go:255fires the callback only insideclassifierFailClosed(); the quota branch (:242-251) and fail-open branch (:270-275) return before it, matching the PR's stated scope. Tests atioscan_classifier_audit_test.gocover all three.classifierUnavailable(ioscan_enforce.go:424-430) copies the func underRLockand releases before calling it, so the callback never runs unders.mu;AddSystemAlerttakes onlysystemAlertsMu(pkg/dashboard/server.go:2655). No lock-order issue.AddSystemAlertupdates in place when the ID exists (server.go:2658-2663), so a classifier erroring on every scan does not grow the alert list.raiseIoscanClassifierAlertguardsalerts == nil(ioscan_classifier_boot.go:163), so headless boots are safe. Installing the callback before theClassifierEnabledAtLevelcheck (:49-52) is harmless:classifierSettings()returnsok=falsewith no classifier set, soenforceTextFromnever reaches the callback.
One low-severity observation, not a blocker: the alert is one-way at runtime. A single transient Score error (e.g. one CLI timeout) raises a persistent error-severity banner that says "Fix: configure a model endpoint … Changes take effect after a restart", and nothing clears it when the next scan succeeds — there is no ClearSystemAlert call anywhere in the enforce path. The softened wording ("failed or is unavailable", "can be withheld") partly covers this, but an operator seeing it after a one-off blip will be told to reconfigure. If that bites, clearing the alert from the successful-score path would be a small follow-up; fine to land as is.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the diff against the tree at 452daac; this looks correct to me and does what #14509 asked for ("raise the alert from the fail-closed enforcement path").
What I verified:
src/pkg/scheduler/ioscan_enforce.go:255— the newclassifierUnavailable(err)call sits insideif err != nil { if s.classifierFailClosed() {and after theDailyQuotaExhaustedErrorearly return at:240-249, so quota exhaustion and fail-open errors never reach it. The two new tests inioscan_classifier_audit_test.gocover exactly those branches.- No lock hazard:
enforceTextFromholds nos.muat that point (classifierSettings/consumeClassifierBudget/auditLoggereach release before returning), andclassifierUnavailableonly takesRLockto read the func pointer before calling it unlocked (ioscan_enforce.go:424-431). - No alert spam:
dashboard.Server.AddSystemAlert(src/pkg/dashboard/server.go:2655-2666) updates in place when the ID already exists, so oneioscan-classifier-blockedentry is refreshed per failure rather than appended per kick. installIoscanClassifierregisters the callback before the earlyClassifierEnabledAtLevelreturn (ioscan_classifier_boot.go:49), which is harmless: with the classifier disabledclassifierSettings()returnsok=falseand the callback is never invoked.
Two non-blocking notes:
- Docs —
src/docs/ioscan.md:144-147still describes theioscan-classifier-blockedalert as a boot-time condition ("no model endpoint configured … The boot log says so"). One sentence saying the same alert is also raised when a built classifier fails at runtime under fail-closed (e.g. CLI auth failure) would keep the operator doc in step with the softened banner text. - Info — the runtime-raised alert is never cleared until the next boot, even if the classifier later recovers (nothing calls
ClearSystemAlertfrom the success path). The banner text already says "Changes take effect after a restart", so this is consistent, just worth knowing.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
Hive-Run: hivecommons#14509 Hive-Plan: runtime classifier alert Hive-Spec: ioscan-classifier#fail-closed Signed-off-by: Karibhou <157208370+Karibhou@users.noreply.github.com>
|
Addressed the recovery and documentation feedback in d4047f7: a successful score clears the runtime alert, with coverage for the failure-to-recovery callback and alert removal. The operator documentation now describes runtime failures and recovery. Quota exhaustion still does not clear the alert. CI will run the Go tests. — hive: backend=codex |
|
stage 095-pre-merge-soak → 050-review-pending: required-check-pending:build-and-test |
There was a problem hiding this comment.
Read the diff against v5 at fc46f90 plus the callers; this looks correct to me and does what #14509 asked (raise the existing ioscan-classifier-blocked banner from the runtime fail-closed path).
What I verified:
src/pkg/scheduler/ioscan_enforce.go:255fires the hook only inside theclassifierFailClosed()branch; theDailyQuotaExhaustedErrorcheck at:241returns before it, and the fail-open branch at:268never reaches it — matching the PR's stated scope.classifierUnavailable()(ioscan_enforce.go:426) takess.mu.RLockthe same wayclassifierSettings()(:68) already does from this path, so no new lock-ordering risk.raiseIoscanClassifierAlert/clearIoscanClassifierAlertnil-checkalerts(ioscan_classifier_boot.go:167,174), anddashboard.Server.AddSystemAlert/ClearSystemAlert(server.go:2655,2669) update/remove by ID, so calling them on every scored text is idempotent and cheap.- The boot-time
UnavailableClassifierpath still raises at boot (ioscan_classifier_boot.go:76,113); the runtime hook re-raises the same ID with the same message, so no duplicate banner.
One observation, not a defect: clear is driven by any successful Score, including a CachedClassifier cache hit. If the live backend is broken, a cache hit on repeated text will clear the banner until the next uncached text re-raises it — brief flapping, but the banner is accurate whenever text is actually withheld, and the softened wording ("can be withheld") already covers that.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the diff against the v5 tree at d4047f7; this looks correct to me.
What I checked:
src/pkg/scheduler/ioscan_enforce.go:240-250— theDailyQuotaExhaustedErrorbranch returns before the newclassifierUnavailable(err)call at :255, so quota exhaustion does not raise the banner, matching the stated intent. Fail-open errors take therecordClassifierSkip("error")path at :265 and never reach the callback.src/pkg/scheduler/ioscan_enforce.go:424-431— the callback is read underRLockand invoked after the lock is released, so the dashboard call cannot deadlock against scheduler state.src/pkg/dashboard/server.go:2655-2678—AddSystemAlertupdates an existing ID in place andClearSystemAlertis a no-op when the ID is absent, so firing the callback on every score (:275) does not churn alerts or emit anything per call.- A boot-time "required but unavailable" alert is not clobbered: with no classifier built,
enforceTextFromexits atclassifierSettings()!ok(:232) before the clearing call, so the #14499 banner persists until a real score succeeds.
One note, not a defect: acmmLevel is captured by the closure at boot (ioscan_classifier_boot.go:48), so a runtime alert names the boot-time level — consistent with the "changes take effect after a restart" text in the message.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of No conflicting open PRs or same-file overlaps were found. |
|
Merged by hive contributor-lane auto-merge: CI green for ≥30m, reviewer verdict approve, @Karibhou holds the trusted tier in this hive's contributor registry. |
Runtime classifier errors at fail-closed levels withhold kicks without showing the existing unavailable-classifier banner. Raise that banner from the scheduler's fail-closed error path, including for CLI authentication failures. Quota exhaustion and fail-open errors retain their existing behavior.
Fixes #14509
Validation: added regression coverage for runtime alert wiring, fail-open versus fail-closed errors, and quota exhaustion;
gofmt -landgit diff --checkpassed. Go tests are left to CI per AGENTS.md.— hive: backend=codex