fix(risk): cap sync gating enforcement scans at 1s (DNO-745) - #4849
fix(risk): cap sync gating enforcement scans at 1s (DNO-745)#4849speakeasyforgebot wants to merge 1 commit into
Conversation
Gating p95/p99 spikes on tool.requested and prompt.submitted were driven by OpenRouter prompt-injection classify calls holding ScanForEnforcement for multi-seconds. Bound the sync hooks path to the documented ~1s evaluation deadline and fail open when it expires. Co-authored-by: Forge Bot <speakeasyforgebot@users.noreply.github.com>
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/internal/risk/scanner.go">
<violation number="1" location="server/internal/risk/scanner.go:268">
P1: According to linked Linear issue DNO-745, sync gating must cap at 1s, but this cooperative timeout cannot stop an in-progress Gitleaks scan and `g.Wait()` still waits for it. Bound hook scan input or make CPU scanners deadline-aware so oversized tool input cannot retain the multi-second gating latency.</violation>
</file>
<file name="server/internal/risk/scanner_test.go">
<violation number="1" location="server/internal/risk/scanner_test.go:297">
P2: The 50ms deadline is applied to the entire ScanForEnforcement call, including the pre-scan DB reads (ListEnabledEnforcingPoliciesByProject, riskPolicyGrants, exclusion lookups). If those queries take longer than 50ms on a slow/loaded CI, the deadline fires before the Presidio AnalyzeBatch is ever reached, so the returned error is "list enforcing policies: ..."/"resolve risk policy audience principals: ..." rather than "enforcement scan deadline exceeded", and pii.cancellations stays 0 — failing the `require.Contains(err.Error(), "enforcement scan deadline exceeded")` and `require.Positive(pii.cancellations.Load())` assertions spuriously. The test is only reliable when all DB work completes within 50ms. Consider widening the deadline to something comfortably above DB time but still far below the 2s Presidio delay (e.g. 200ms) while keeping the 500ms elapsed bound and the 2s delay.</violation>
</file>
Linked issue analysis
Linked issue: DNO-745: investigation: Gating p95 latency has trended up
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Cap sync-path enforcement scans with a 1s timeout in ScanForEnforcement | PR adds an enforcementScanBudget = 1s and applies context.WithTimeout(ctx, enforcementScanBudget) in ScanForEnforcement. |
| ✅ | On timeout, return an error so hooks fail-open and metrics record failure (record failure instead of a false clean allow) | PR checks ctx.Err() after scanning, records failure metric, and returns an error so the ingest path can fail open and metrics reflect the timeout. |
| ✅ | Add tests to assert the budget constant == 1s and that a slow scanner aborts / sees cancellation | PR adds a unit test that asserts enforcementScanBudget == 1s and a test that exercises a shortened parent deadline to verify cancellation of a slow scanner. |
| ✅ | Shorter parent deadlines still win (ScanForEnforcement respects parent context deadlines) | The tests create a 50ms parent deadline and verify the scan aborts near that deadline and that the slow scanner observed cancellation, demonstrating the parent deadline takes precedence. |
| ❌ | After deploy: verify hooks.event.duration p95/p99 for scanned canonical events are near ≤1s (fail-open on timeout) rather than multi-second successful judge calls | This is a post-deploy monitoring step referenced in the PR’s test plan and not something the diff implements or can verify; requires runtime observation after deployment. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| // Cap sync-path scanners (Presidio, OpenRouter PI, prompt-policy judges) | ||
| // so a slow model call cannot hold the gating RPC for multi-seconds. | ||
| // Parent contexts with a tighter deadline still win. | ||
| ctx, cancel := context.WithTimeout(ctx, enforcementScanBudget) |
There was a problem hiding this comment.
P1: According to linked Linear issue DNO-745, sync gating must cap at 1s, but this cooperative timeout cannot stop an in-progress Gitleaks scan and g.Wait() still waits for it. Bound hook scan input or make CPU scanners deadline-aware so oversized tool input cannot retain the multi-second gating latency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/internal/risk/scanner.go, line 268:
<comment>According to linked Linear issue DNO-745, sync gating must cap at 1s, but this cooperative timeout cannot stop an in-progress Gitleaks scan and `g.Wait()` still waits for it. Bound hook scan input or make CPU scanners deadline-aware so oversized tool input cannot retain the multi-second gating latency.</comment>
<file context>
@@ -254,6 +262,12 @@ func (s *Scanner) ScanForEnforcement(
+ // Cap sync-path scanners (Presidio, OpenRouter PI, prompt-policy judges)
+ // so a slow model call cannot hold the gating RPC for multi-seconds.
+ // Parent contexts with a tighter deadline still win.
+ ctx, cancel := context.WithTimeout(ctx, enforcementScanBudget)
+ defer cancel()
+
</file context>
| require.NoError(t, err) | ||
|
|
||
| authCtx, _ := contextvalues.GetAuthContext(ctx) | ||
| deadlineCtx, cancel := context.WithTimeout(ctx, 50*time.Millisecond) |
There was a problem hiding this comment.
P2: The 50ms deadline is applied to the entire ScanForEnforcement call, including the pre-scan DB reads (ListEnabledEnforcingPoliciesByProject, riskPolicyGrants, exclusion lookups). If those queries take longer than 50ms on a slow/loaded CI, the deadline fires before the Presidio AnalyzeBatch is ever reached, so the returned error is "list enforcing policies: ..."/"resolve risk policy audience principals: ..." rather than "enforcement scan deadline exceeded", and pii.cancellations stays 0 — failing the require.Contains(err.Error(), "enforcement scan deadline exceeded") and require.Positive(pii.cancellations.Load()) assertions spuriously. The test is only reliable when all DB work completes within 50ms. Consider widening the deadline to something comfortably above DB time but still far below the 2s Presidio delay (e.g. 200ms) while keeping the 500ms elapsed bound and the 2s delay.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/internal/risk/scanner_test.go, line 297:
<comment>The 50ms deadline is applied to the entire ScanForEnforcement call, including the pre-scan DB reads (ListEnabledEnforcingPoliciesByProject, riskPolicyGrants, exclusion lookups). If those queries take longer than 50ms on a slow/loaded CI, the deadline fires before the Presidio AnalyzeBatch is ever reached, so the returned error is "list enforcing policies: ..."/"resolve risk policy audience principals: ..." rather than "enforcement scan deadline exceeded", and pii.cancellations stays 0 — failing the `require.Contains(err.Error(), "enforcement scan deadline exceeded")` and `require.Positive(pii.cancellations.Load())` assertions spuriously. The test is only reliable when all DB work completes within 50ms. Consider widening the deadline to something comfortably above DB time but still far below the 2s Presidio delay (e.g. 200ms) while keeping the 500ms elapsed bound and the 2s delay.</comment>
<file context>
@@ -268,6 +268,49 @@ func TestScanner_FanOutAcrossPoliciesIsConcurrent(t *testing.T) {
+ require.NoError(t, err)
+
+ authCtx, _ := contextvalues.GetAuthContext(ctx)
+ deadlineCtx, cancel := context.WithTimeout(ctx, 50*time.Millisecond)
+ t.Cleanup(cancel)
+
</file context>
| deadlineCtx, cancel := context.WithTimeout(ctx, 50*time.Millisecond) | |
| deadlineCtx, cancel := context.WithTimeout(ctx, 200*time.Millisecond) |
3 participants
Summary
Investigates and mitigates DNO-745 (gating p95 latency trending up).
Sync gating on canonical hook events (
tool.requested,prompt.submitted) was spending multi-seconds insiderisk.ScanForEnforcement→ OpenRouter prompt-injection classify. Classic Claude hook events stayed ~15–20ms; scanned canonical events drove the dashboard spikes (p99 multi-second).Fix
context.WithTimeoutinScanForEnforcement(documented gating evaluation deadline). Nested parent deadlines still win.Investigation notes (Datadog)
hooks.event.duration(dashboard Hooks & Otel Ingestion)tool.requested/prompt.submittedwithrisk_scanned:true; onset ~ mid-July when canonical ingest + PI enforcement volume roserisk.prompt_injection.classify→ OpenRouter chat completions (often ~2.5–3.6s when successful)Linear
Could not comment on DNO-745 from this environment (Linear MCP unauthenticated). Please paste findings onto the issue when reviewing.
Test plan
go test ./server/internal/risk/ -run 'TestScanner_ScanForEnforcement|TestScanner_FanOut|TestEnforcementScanBudget'mise run lint:serverhooks.event.durationp95/p99 for scanned canonical events stay near ≤1s (fail-open on timeout) rather than multi-second successful judge callsLinear Issue: DNO-745
Summary by cubic
Cap sync gating enforcement scans to a 1s budget in
risk.ScanForEnforcementto stop multi-second delays from OpenRouter PI classify and bring p95/p99 back in line (DNO-745). On timeout, we fail open and record a failure; shorter parent deadlines still win.context.WithTimeoutinScanForEnforcement; return an error on deadline to fail open and record failure.Written for commit 54789ce. Summary will update on new commits.