v1.4.1: reliability, correctness fixes, and multi-provider AI - #160
Closed
nghiadaulau wants to merge 7 commits into
Closed
v1.4.1: reliability, correctness fixes, and multi-provider AI#160nghiadaulau wants to merge 7 commits into
nghiadaulau wants to merge 7 commits into
Conversation
Closes the v1.4.1 reliability item: keep the agent up and observable
when log backends or the AI provider misbehave.
Source resilience (pkg/agent/health.go, worker.go):
- Per-source HealthTracker with exponential backoff cooldown
(source_backoff_initial doubling to source_backoff_max). A failing
source is skipped during its cooldown instead of being hammered.
- Per-pull context deadline (reliability.pull_timeout, default 20s)
so a hung backend cannot stall the tick.
- batch_max truncation now records total_signals_dropped instead of
dropping data silently.
AI resilience (pkg/agent/ai/openai.go, breaker.go, factory_ai.go):
- Retry on HTTP 429, 5xx, and network errors with exponential backoff
plus jitter (ai_retry.max_attempts, initial_backoff, max_backoff).
4xx other than 429 surface immediately.
- Three-state circuit breaker (closed → open → half-open) wrapping
AI calls. Short-circuits during cooldown so OpenAI outages do not
burn through the per-hour rate limit. One probe is granted after
the cooldown; success closes the breaker, failure re-opens it.
- Rolling-window latency tracking (last 100 successful calls feed
p50 / p95).
Observability (pkg/controllers/agent.go):
- GET /api/agent/status now returns:
- sources: [{name, ok, consecutive_failures, last_error,
last_error_at, last_success_at, in_cooldown_until, total_pulls_ok,
total_pulls_failed, total_signals_pulled, total_signals_dropped,
last_pull_duration_ms, last_signals_pulled}]
- ai: {state, consecutive_failures, total_success, total_failure,
total_opens, total_probes, last_error, last_error_at,
last_success_at, opened_at, latency_p50_ms, latency_p95_ms}
Configuration:
- New agent.reliability block in config/config.yaml with documented
defaults. The block is optional; existing deployments behave
unchanged when it is omitted.
Tests:
- pkg/agent/health_test.go: 6 cases (backoff math, cooldown skip,
success reset, register idempotency, disabled backoff, deep-copy
snapshot).
- pkg/agent/ai/breaker_test.go: 7 cases (disabled, threshold trip,
half-open success closes, half-open failure reopens, success
resets consecutive count, latency p50/p95, stats counters).
- pkg/agent/ai/openai_test.go: 6 cases hitting httptest.Server
(429 retry, 5xx retry, 4xx no-retry, retry exhausted, attempts=1
disabled, context cancel stops retry).
Coverage: pkg/agent 53.9%, pkg/agent/ai 77.8%.
ROADMAP.md and CHANGELOG.md updated; v1.4.1 reliability item ticked.
…tatus Three related correctness bugs that have been present since the v1.4.0 incident-management work landed. 1. Data race in Catalog.Get (pkg/agent/catalog.go) Get returned a live *Pattern pointer that callers (worker.classify via prev.BaselineFrequency / prev.Count) read without holding the catalog mutex while a concurrent Upsert from another source's tick wrote to the same struct. Existing tests passed under -race only because no test exercised concurrent ticks for the same pattern. Get now returns a deep copy (including a copied Tags slice). A new TestCatalog_ConcurrentGetUpsertNoRace runs both operations in parallel under -race. 2. Alert fan-out short-circuited on first error (pkg/core/alert.go) The legacy SendAlert returned on the first provider error and never invoked subsequent providers — a flaky Slack silently muted Telegram and Email. The whole point of the multi-channel design was defeated. Added Alert.SendAllAlerts which tries every configured provider regardless of earlier failures and returns an AlertResult with per-channel success / failure plus a joined error. The legacy SendAlert is kept as a thin wrapper for any external callers. Each AlertProvider now also exposes Name() so failures are attributable in logs and the audit record. 3. OnCallTriggered reported true even when escalation never started (pkg/services/incident.go) The flag was set at record-build time, before SendAlert and workflow.Start ran. If workflow.Start later failed the function returned early and the persisted record claimed on-call was triggered while no one was actually paged. CreateIncident now uses SendAllAlerts; it stores ChannelsNotified as the channels that ACTUALLY succeeded (vs. the new ChannelsEnabled field for what was configured), sets NotifyStatus to "sent" / "partial" / "failed" based on the fan-out result, and walks OnCallTriggered back to false (writing OnCallError) if workflow.Start fails. Storage schema gained ChannelsEnabled and OnCallError to support this. Test coverage: - TestSendAllAlerts_TriesEveryProvider (3 providers, middle one OK, two fail; verifies every provider is called exactly once and Succeeded reflects truth). - TestSendAllAlerts_AllSucceed / AllFail. - TestSendAlertLegacy_ReturnsJoinedErr. - TestCatalog_ConcurrentGetUpsertNoRace.
Add agent.catalog.max_patterns (default 10000). When Upsert would exceed the cap, the least-frequent NON-"known" pattern is evicted. Operator-curated patterns (verdict: "known") survive eviction regardless of their count, on the assumption that someone marked them deliberately as baseline. Before this change, long-running agents in noisy environments grew patterns.json without bound. Each persist tick marshalled the entire map, so disk write latency scaled linearly with the worst case. max_patterns: 0 disables the cap (legacy behaviour). Wired through NewWorker. Tests: - TestCatalog_MaxPatternsEvictsLeastFrequent (3-pattern cap, 4th upsert evicts the one with lowest Count). - TestCatalog_MaxPatternsPreservesKnown (known pattern with the lowest count is NOT evicted; a higher-count non-known one is). - TestCatalog_MaxPatternsZeroDisabled (legacy behaviour preserved).
All three admin controllers (agent, config, incidents) used a naive `got != expected` string compare for the X-Gateway-Secret header. Switched to crypto/subtle.ConstantTimeCompare via a small shared helper (pkg/controllers/secret.go) so prefix-match timing oracles are eliminated. The risk surface is small (local network, ~64-byte secret) but the fix is one line per call site. Empty configured secrets continue to reject every request, which is the existing contract documented in the controller and in CLAUDE.md.
os.WriteFile + os.Rename did not call fsync on the tmp file before
the rename. The rename is journaled by ext4 / xfs; the tmp file's
data is not, so a power loss in the brief window between write and
rename could replace a previous good patterns.json or incidents.json
with a zero-length file.
Extracted a writeFileAtomicSync helper that opens the .tmp file,
writes the payload, fsyncs, closes, renames over the target, and
cleans up the .tmp on any error path. WriteBlob (agent catalog,
shadow log, detect log) and persistIncidentsLocked (every alert)
now go through this helper.
Note: this commit was originally landed via commit-fixes.sh with the
wrong commit message ("correctness regressions ...") due to a heredoc
syntax bug in the script. The message has been amended; the diff is
unchanged.
Wraps up the post-v1.4.1 code-review fixes that did not land via the earlier commits (the commit script's heredocs broke on some hosts). This is one combined commit because the worker / openai changes touch the same files for two related concerns. Worker hardening (pkg/agent/worker.go, worker_panic_test.go): - Reorder: circuit breaker check now runs BEFORE the rate limiter so an open breaker does not consume per-hour quota slots. - Panic recovery: defer recover() inside each per-source goroutine spawned by tick(). A panic in tickSource logs the source name + full stack and records the failure on the HealthTracker instead of silently killing the worker goroutine. - Wire SetMaxPatterns from the configured catalog cap. AI client hardening (pkg/agent/ai/openai.go, openai_test.go): - Retry sleep is now ctx-aware (select on ctx.Done() / time.After) so SIGTERM is not blocked by an in-flight backoff (was up to ~15s at default settings). - Removed the per-instance *rand.Rand — math/rand.Rand is not goroutine-safe and Analyze runs concurrently from multiple source ticks. Use the package-level rand.Int63n instead (Go 1.20+ guarantees concurrent safety on the global). - Guard against rand.Int63n(0) when an operator configures an extremely small initial_backoff. - Surface the last underlying HTTP error on ctx-cancel mid-retry instead of replacing it with a bare "context canceled". - Include the first 300 chars of the raw model reply in ParseFinding errors so the detect audit log shows operators why parsing failed instead of just "no JSON object found". Multi-provider AI (pkg/agent/ai/openai.go, openai_test.go, examples/gemini-test/): - Removed the hard-coded OpenAI URL constant; AgentAIConfig.BaseURL (config field landed in the earlier catalog-cap commit) now routes the analyzer at any OpenAI-compatible endpoint — Gemini's /v1beta/openai/chat/completions, LiteLLM, OpenRouter, ... - finish_reason is now read from each choice. On "length" (model hit max_tokens mid-JSON) the analyzer auto-retries once with max_tokens doubled, capped at 4096. This recovers Gemini-2.5- flash's verbose output without operator intervention. On "content_filter" the analyzer surfaces a clear error and does NOT retry. - examples/gemini-test/ contains a runnable end-to-end demo (GEMINI_API_KEY env var required) used to validate the fix against the real Gemini API. Tests (all green under -race): - TestWorker_TickRecoversFromSourcePanic — panicking source does not kill the worker; sibling source still runs. - TestOpenAI_CancelDuringSleepReturnsFast — ctx cancel mid-backoff returns within 500ms even with a 2-second backoff configured. - TestOpenAI_BaseURLDefault / BaseURLConfigured. - TestOpenAI_TruncationAutoRetry — first call returns finish_reason=length, second call succeeds; exactly 2 HTTP calls. - TestOpenAI_TruncationRetryFailsWithClearError — both calls truncate; error message must mention "truncated" and "max_tokens". - TestOpenAI_ContentFilterErrorIsClear — single call, "safety filter" in error string. - The existing retry / breaker tests were refactored to use the new BaseURL config pointing at the httptest server, removing the previous `rewritingTransport` hack. Misc: - .gitignore now allows examples/**/*.sh so this PR's runnable demo script ships with the example. - CHANGELOG.md updated with the Added (multi-provider AI, finish_ reason handling) and Fixed (correctness regressions + hardening) sections covering this entire round of v1.4.1 follow-up work.
nghiadaulau
changed the title
Addresses review feedback on PR VersusControl#160: drop the verbose base_url and let operators pick a provider by name instead. Before: agent: ai: base_url: "https://generativelanguage.googleapis.com/v1beta/openai/chat/completions" api_key: ... After: agent: ai: provider: gemini # openai | gemini | claude api_key: ... The chat/completions URL is resolved from the provider name at startup via ai.ProviderURL: openai -> https://api.openai.com/v1/chat/completions gemini -> https://generativelanguage.googleapis.com/v1beta/openai/chat/completions claude -> https://api.anthropic.com/v1/chat/completions All three speak the OpenAI chat/completions wire format (Gemini and Claude via their respective OpenAI compatibility shims), so the client code is unchanged. Empty / missing provider defaults to "openai" for backwards compatibility with configs that omit the field. Unknown values fail fast at startup with a clear error ("ai: unknown provider %q (want openai | gemini | claude)") rather than 404'ing on every call. OpenAI.Name() now returns the canonical lowercase provider name ("openai" / "gemini" / "claude") instead of always "openai", so the startup banner and the detect audit log identify the backend each call actually hit. Also drops the no-op sources_path field from the Gemini example config — the codebase has hardcoded "agent_sources.yaml next to the main config" for a while now, so sources_path was silently ignored. Tests: - TestProviderURL covers every supported value plus three rejection cases (anthropic / bedrock / openrouter) and the case-insensitive + whitespace-tolerant input handling. - TestNewOpenAI_PropagatesProviderError asserts that an unknown provider name surfaces at construction time, not on first call. - TestOpenAI_NameReflectsProvider locks down the Name() contract for every supported value. - The withHandler helper and the two large-backoff tests that previously set BaseURL=srv.URL now construct with Provider="openai" and override the unexported chatURL field directly (same-package test access; no new public seam).
Member
There was a problem hiding this comment.
Please use YAML format and write only in English.
Member
There was a problem hiding this comment.
The ProviderURL function should be included in the factory file.
Member
There was a problem hiding this comment.
The function should rename the name from NewOpenAIWithRetry to NewAIWithRetry, and, based on the provider, return the AI instance for that provider using the factory pattern.
Member
There was a problem hiding this comment.
Can we move the sourcePayload into the agent folder?
Member
There was a problem hiding this comment.
Please update the document with the new config options: agent.reliability and agent.catalog.max_patterns in the README and src folder. Additionally, create a new page explaining Reliability and graceful degradation in src/agent/. Use simple language. If you understand, please commit these documents.
Member
|
Please update the comment with the latest code. Run the test using the mode file logs to ensure the agent's max_patterns function works correctly. You can use the script to generate fake logs located in the scripts folder. |
Member
|
Could you please move the section "Code review caught 3 correctness bugs that have been latent since v1.4.0 (catalog data race, alert fan-out short-circuit, and on-call status discrepancy)" into a separate pull request? This would add more value and allow me to merge it first. The remaining features, such as graceful degradation and multi-provider support, will likely change since I plan to use the Eino agent framework. I can't merge those changes after I apply them. |
5 tasks
Member
Author
|
Note: this PR appears to have been auto-closed by GitHub when I Since this PR was opened, the maintainer landed the Eino agent
The reviewer flagged this risk earlier ("the remaining features Most of this PR's work is now obsolete on the new architecture:
The correctness bug fixes are already split out to #162 Small focused follow-up PRs to consider, separate from this one:
Thanks for the patience and for keeping #162 alive. |
14 tasks
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
2 participants
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.
What does this PR do?
Closes the v1.4.1 reliability roadmap item (graceful degradation on log-source
outages and AI failures), folds in correctness regressions caught during a
post-review of the v1.4.0 incident-management work, and adds OpenAI-compatible
base_urlso the AI SRE analyzer can target Gemini, LiteLLM, OpenRouter, etc.35 files, +2391/-117 LOC, 6 commits, 30+ new tests. Backwards-compatible:
every new config field has a safe default; deployments running today work
unchanged.
Why?
ROADMAP.md— graceful degradation.race, alert fan-out short-circuit, on-call status lie). Better to ship
them with v1.4.1 than carry a known-broken
main.endpoint was hard-coded; a single
base_urlfield unlocks everyOpenAI-compatible provider.
How to test
Unit tests —
go test -race ./...(30+ new tests, full list in commitsf1f43afand8bdeb5f):Live: source resilience — point an Elasticsearch / Loki source at a
dead port, send some alerts, watch
/api/agent/status.sources[].in_cooldown_untilback off exponentially; bring the backend up and watch it recover.
Live: multi-channel fan-out — enable Slack (real) + Lark (point at a
404 URL). POST an incident. Slack receives the message; Lark fails. Check
/api/admin/incidents/:id→channels_notified: ["slack"],notify_status: "partial",notify_error: "lark: ...".Live: catalog cap — set
agent.catalog.max_patterns: 3, inject 5distinct error patterns.
/api/agent/patternsreturns exactly 3.Live: Gemini end-to-end —
examples/gemini-test/run.shboots adetect-mode config pointed at Gemini's OpenAI-compatible endpoint, injects
sample errors, and prints the parsed AI finding. Requires
GEMINI_API_KEYenv var. Verified against real Gemini API: 5.3s p50 latency, finding emitted
as incident.
Type of change
constant-time secret compare, fsync atomicity
OpenAI-compatible
base_url,finish_reason+ truncation auto-retry,panic recovery in worker
Checklist
go test ./...passes locally (under-race)go vet ./...is cleangofmt'dpkg/agent,pkg/agent/ai,pkg/core)config/config.yamlcomments,CHANGELOG.md)ROADMAP.md(ticked the v1.4.1 reliability item)Changes by theme
Reliability (closes the roadmap item)
pkg/agent/health.go—HealthTracker, per-source exponential backoffpkg/agent/ai/breaker.go— three-state circuit breaker(closed → open → half-open) for AI calls
pkg/agent/ai/openai.go— retry on 429 / 5xx / network errors withjitter; ctx-aware sleep;
finish_reason: lengthauto-retry once withdoubled
max_tokenspkg/agent/worker.go— per-pull context deadline (pull_timeout);truncation counter when
batch_maxdrops signals; panic recovery inper-source goroutines
pkg/controllers/agent.go—/api/agent/statusnow returnssources: [...]andai: {...}blocksCorrectness regressions (v1.4.0 follow-up)
pkg/agent/catalog.go—Getreturns a deep copy of*Patterninstead of the live pointer (fixes a data race with
Upsertundermulti-source workloads)
pkg/core/alert.go— newAlert.SendAllAlertstries every providerregardless of earlier failures; per-channel success / failure
tracked in
AlertResult. Six provider files gainName().pkg/services/incident.go—ChannelsNotifiednow reflects channelsthat actually succeeded (vs.
ChannelsEnabledfor what wasconfigured);
OnCallTriggeredwalked back tofalsewhenworkflow.Startfails.Multi-provider AI
pkg/config/agent.go,pkg/agent/ai/openai.go—agent.ai.base_urlconfig; default unchanged (OpenAI). Verifiedagainst Gemini's
/v1beta/openai/chat/completions.examples/gemini-test/— runnable end-to-end demo.Hardening (security / durability / scaling)
pkg/controllers/secret.go—crypto/subtle.ConstantTimeComparefor
X-Gateway-Secret(was a naive==).pkg/agent/catalog.go—agent.catalog.max_patternscap(default 10000); LFU eviction;
verdict: "known"patterns arepreserved.
pkg/storage/file.go—writeFileAtomicSyncaddsf.Sync()between tmp-write and rename so power loss can't replace good files
with zero-length ones.
New configuration (all optional, sensible defaults)