Skip to content

Audit stream opens on flush and pre-write panics - #6005

Merged
JAORMX merged 1 commit into
mainfrom
audit-stream-open-followups
Jul 27, 2026
Merged

Audit stream opens on flush and pre-write panics#6005
JAORMX merged 1 commit into
mainfrom
audit-stream-open-followups

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

Stacked on #5874 — addresses the two minor, non-blocking follow-ups from @rdimitrov's review, both in the deferred stream-open logging in pkg/audit/auditor.go:

  1. streamOpenWriter.Flush() bypassed logOnce — a handler that establishes the stream by flushing headers and then blocks waiting for events sends the implicit 200 through Flush() without ever hitting WriteHeader/Write, so the connection event was delayed until a later data write or connection close. Flush() now calls logOnce(http.StatusOK); a flush commits the response headers with an implicit 200, so it counts as the stream being established.

  2. Stream-open event lost on a panic before the first write — on the vMCP Serve path, recovery is the outermost wrapper (outside audit), so a panic in an inner layer before any byte is written unwound straight past audit: recovery returns 500 and no stream-open event was recorded. The sw.logOnce(http.StatusOK) call moved into a defer registered before next.ServeHTTP, so the event is produced during unwinding. If the stream already logged on first write, the deferred call is a no-op. This restores parity with the pre-Audit authentication failures and webhook denials #5874 arrival-time logging for that case (the connection-open is recorded; the 500 written by the outer recovery is not visible to audit, same as before).

The runner/proxy path was already unaffected by (2) — recovery is innermost there, so the 500 flows back out through streamOpenWriter and logOnce(500) fires.

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)

Two new subtests in TestStreamOpenAuditEvents: flush before first write logs the connection event and panic before first write still logs the connection event.

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

No CRD or exported-API changes.

Does this introduce a user-facing change?

Yes. Audit logs now always contain the stream-connection event for SSE / streamable GET opens: previously it could be delayed or lost for handlers that flush before writing, and lost entirely on a pre-write panic on the vMCP Serve path.

@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Jul 27, 2026
rdimitrov
rdimitrov previously approved these changes Jul 27, 2026
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.14%. Comparing base (a6a0d4c) to head (9c98e94).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6005      +/-   ##
==========================================
- Coverage   72.15%   72.14%   -0.01%     
==========================================
  Files         720      720              
  Lines       74744    74745       +1     
==========================================
- Hits        53929    53926       -3     
- Misses      16942    16949       +7     
+ Partials     3873     3870       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Base automatically changed from audit-full-chain-coverage to main July 27, 2026 11:13
Stacked on #5874. Two fast-follows from rdimitrov's review:

- Flush() now logs the stream connection event: a handler that
  establishes the stream by flushing headers and then blocks never hits
  WriteHeader/Write, so the event was delayed until the first data
  write or lost entirely.
- The stream-open log moved into a defer registered before ServeHTTP,
  so a panic in an inner handler still produces the event during
  unwinding. On chains whose recovery middleware runs outside audit
  (the vMCP Serve path) the event previously vanished.
@JAORMX
JAORMX force-pushed the audit-stream-open-followups branch from ff7ece7 to 9c98e94 Compare July 27, 2026 11:27
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Jul 27, 2026
@JAORMX
JAORMX merged commit 96f25d0 into main Jul 27, 2026
81 of 82 checks passed
@JAORMX
JAORMX deleted the audit-stream-open-followups branch July 27, 2026 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants