Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion pkg/audit/auditor.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,15 @@ func NewAuditorWithTransport(config *Config, transportType string) (*Auditor, er
// Close closes the underlying log writer if it implements io.Closer.
// This should be called when the auditor is no longer needed to properly release resources.
func (a *Auditor) Close() error {
if closer, ok := a.logWriter.(io.Closer); ok {
return closeLogWriter(a.logWriter)
}

func closeLogWriter(logWriter io.Writer) error {
if logWriter == os.Stdout || logWriter == os.Stderr {
return nil
}

if closer, ok := logWriter.(io.Closer); ok {
return closer.Close()
}
return nil
Expand Down
11 changes: 11 additions & 0 deletions pkg/audit/auditor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"io"
"net/http"
"net/http/httptest"
"os"
"strings"
"testing"
"time"
Expand All @@ -34,6 +35,16 @@ func TestNewAuditor(t *testing.T) {
assert.Equal(t, config, auditor.config)
}

func TestAuditor_CloseDoesNotCloseStdout(t *testing.T) {
t.Parallel()

auditor := &Auditor{logWriter: os.Stdout}

require.NoError(t, auditor.Close())
_, err := os.Stdout.Write(nil)
require.NoError(t, err, "Close() must not close os.Stdout")
}

func TestAuditorMiddlewareDisabled(t *testing.T) {
t.Parallel()
config := &Config{}
Expand Down
9 changes: 9 additions & 0 deletions pkg/audit/workflow_auditor.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"context"
"encoding/json"
"fmt"
"io"
"log/slog"
"time"

Expand All @@ -21,6 +22,7 @@ type WorkflowAuditor struct {
auditLogger *slog.Logger
config *Config
component string
logWriter io.Writer
}

// NewWorkflowAuditor creates a new workflow auditor.
Expand All @@ -45,9 +47,16 @@ func NewWorkflowAuditor(config *Config) (*WorkflowAuditor, error) {
auditLogger: NewAuditLogger(logWriter),
config: config,
component: component,
logWriter: logWriter,
}, nil
}

// Close closes the underlying log writer if it owns a closeable resource.
// This should be called when the workflow auditor is no longer needed.
func (w *WorkflowAuditor) Close() error {
return closeLogWriter(w.logWriter)
}

// LogWorkflowStarted logs the start of workflow execution.
func (w *WorkflowAuditor) LogWorkflowStarted(
ctx context.Context,
Expand Down
57 changes: 57 additions & 0 deletions pkg/audit/workflow_auditor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,25 @@ type testLogWriter struct {
logs []string
}

type closeTrackingWriter struct {
closed bool
closeErr error
}

func (w *testLogWriter) Write(p []byte) (n int, err error) {
w.logs = append(w.logs, string(p))
return len(p), nil
}

func (*closeTrackingWriter) Write(p []byte) (n int, err error) {
return len(p), nil
}

func (w *closeTrackingWriter) Close() error {
w.closed = true
return w.closeErr
}

func (w *testLogWriter) getLastLog() string {
if len(w.logs) == 0 {
return ""
Expand All @@ -53,6 +67,7 @@ func createTestAuditor(t *testing.T, config *Config) (*WorkflowAuditor, *testLog
auditLogger: NewAuditLogger(writer),
config: config,
component: "vmcp-composer",
logWriter: writer,
}

return auditor, writer
Expand Down Expand Up @@ -123,6 +138,48 @@ func TestNewWorkflowAuditor(t *testing.T) {
}
}

func TestWorkflowAuditor_Close(t *testing.T) {
t.Parallel()

t.Run("closes retained file writer", func(t *testing.T) {
t.Parallel()

logFilePath := t.TempDir() + "/workflow-audit.log"
auditor, err := NewWorkflowAuditor(&Config{LogFile: logFilePath})
require.NoError(t, err)

_, ok := auditor.logWriter.(interface{ Close() error })
require.True(t, ok, "file-backed workflow auditor should retain a closeable writer")

require.NoError(t, auditor.Close())
})

t.Run("does not close stdout", func(t *testing.T) {
t.Parallel()

auditor, err := NewWorkflowAuditor(&Config{})
require.NoError(t, err)

require.NoError(t, auditor.Close())
_, err = os.Stdout.Write(nil)
require.NoError(t, err, "Close() must not close os.Stdout")
assert.Same(t, os.Stdout, auditor.logWriter)
})

t.Run("propagates close errors", func(t *testing.T) {
t.Parallel()

closeErr := errors.New("close failed")
writer := &closeTrackingWriter{closeErr: closeErr}
auditor := &WorkflowAuditor{logWriter: writer}

err := auditor.Close()

require.ErrorIs(t, err, closeErr)
assert.True(t, writer.closed)
})
}

func TestWorkflowAuditor_LogWorkflowStarted(t *testing.T) {
t.Parallel()

Expand Down
50 changes: 43 additions & 7 deletions pkg/vmcp/core/core_vmcp.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package core

import (
"context"
"errors"
"fmt"
"log/slog"
"sync"
Expand Down Expand Up @@ -71,6 +72,10 @@ type coreVMCP struct {
// by advertised tool name.
workflowDefs map[string]*composer.WorkflowDefinition

// workflowAuditor owns the optional workflow audit log writer and is closed
// with the core. Nil when workflow audit logging is disabled.
workflowAuditor *audit.WorkflowAuditor

// composerFactory builds a per-call composite-tool engine bound to a routing
// table, generalizing server.New's sessionComposerFactory (server.go:393).
composerFactory func(sessionRT *vmcp.RoutingTable, sessionTools []vmcp.Tool) composer.Composer
Expand All @@ -81,6 +86,7 @@ type coreVMCP struct {
stopStore func()

closeOnce sync.Once
closeErr error
}

var _ VMCP = (*coreVMCP)(nil)
Expand Down Expand Up @@ -138,6 +144,16 @@ func New(cfg *Config) (VMCP, error) {
}
slog.Info("workflow audit logging enabled")
}
closeWorkflowAuditor := func() error {
if workflowAuditor == nil {
return nil
}
if err := workflowAuditor.Close(); err != nil {
slog.Warn("failed to close workflow auditor", "error", err)
return err
}
return nil
}

// The elicitation handler depends only on the domain-typed ElicitationRequester
// (#5436); no mcp-go types cross this boundary (vmcp anti-pattern #5).
Expand Down Expand Up @@ -168,7 +184,11 @@ func New(cfg *Config) (VMCP, error) {
instruments, err := newWorkflowInstruments(cfg.TelemetryProvider)
if err != nil {
stopStore()
return nil, fmt.Errorf("failed to create workflow telemetry instruments: %w", err)
cleanupErr := closeWorkflowAuditor()
return nil, errors.Join(
fmt.Errorf("failed to create workflow telemetry instruments: %w", err),
cleanupErr,
)
}

// composerFactory builds a composite-tool engine bound to a specific routing
Expand All @@ -195,7 +215,8 @@ func New(cfg *Config) (VMCP, error) {
workflowDefs, err := validateWorkflowDefs(validationEngine, cfg.WorkflowDefs)
if err != nil {
stopStore()
return nil, fmt.Errorf("workflow validation failed: %w", err)
cleanupErr := closeWorkflowAuditor()
return nil, errors.Join(fmt.Errorf("workflow validation failed: %w", err), cleanupErr)
}

// Build and start the backend health monitor (#5443 reversal: the core owns its
Expand All @@ -206,7 +227,8 @@ func New(cfg *Config) (VMCP, error) {
healthMonitor, healthProvider, err := buildHealthMonitor(cfg)
if err != nil {
stopStore()
return nil, err
cleanupErr := closeWorkflowAuditor()
return nil, errors.Join(err, cleanupErr)
}

return &coreVMCP{
Expand All @@ -217,6 +239,7 @@ func New(cfg *Config) (VMCP, error) {
healthMonitor: healthMonitor,
admission: admission,
workflowDefs: workflowDefs,
workflowAuditor: workflowAuditor,
composerFactory: composerFactory,
stopStore: stopStore,
}, nil
Expand Down Expand Up @@ -537,19 +560,32 @@ func (c *coreVMCP) InvalidateCapabilityCache() {
invalidator.InvalidateAll()
}

// Close stops the workflow state store's cleanup goroutine. It is idempotent:
// the underlying Stop closes a channel that cannot be closed twice, so the work
// is guarded by sync.Once and subsequent calls return nil.
// Close stops the workflow state store's cleanup goroutine, closes the optional
// workflow auditor, and stops the health monitor. It returns cleanup errors from
// the first call, and is idempotent: the underlying cleanup work is guarded by
// sync.Once and subsequent calls return nil.
func (c *coreVMCP) Close() error {
ran := false
c.closeOnce.Do(func() {
ran = true
c.stopStore()
if c.workflowAuditor != nil {
if err := c.workflowAuditor.Close(); err != nil {
slog.Warn("failed to close workflow auditor", "error", err)
c.closeErr = errors.Join(c.closeErr, err)
}
}
Comment on lines +572 to +577

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocker: This is the file that actually fixes #6094, and it's the only one in the diff with no test coverage. All three new tests exercise WorkflowAuditor.Close() directly in pkg/audit; nothing asserts that a core built with a file-backed AuditConfig releases the descriptor, and nothing covers the three New error paths where closeWorkflowAuditor() was added. Those manual, repeated cleanup calls are exactly what a future refactor drops.

pkg/vmcp/core already has the harness — baseConfig(t) in core_vmcp_test.go:46 plus the t.Cleanup(func() { _ = c.Close() }) pattern used throughout core_backends_test.go:

func TestNew_CloseReleasesWorkflowAuditLogFile(t *testing.T) {
	t.Parallel()

	cfg, _ := baseConfig(t)
	cfg.AuditConfig = &audit.Config{LogFile: filepath.Join(t.TempDir(), "audit.log")}

	c, err := New(cfg)
	require.NoError(t, err)
	require.NoError(t, c.Close())

	// A second close on the same *os.File reports ErrClosed, proving the
	// first one reached the descriptor rather than silently no-opping.
	require.ErrorIs(t, c.(*coreVMCP).workflowAuditor.Close(), os.ErrClosed)
}

If you'd rather keep it to one test, please at least cover a single New error path (e.g. forcing validateWorkflowDefs to fail) and assert the fd was released — that's the regression the cleanup ladder exists to prevent.

if c.healthMonitor != nil {
if err := c.healthMonitor.Stop(); err != nil {
slog.Warn("failed to stop health monitor", "error", err)
c.closeErr = errors.Join(c.closeErr, err)
}
}
})
return nil
if !ran {
return nil
}
return c.closeErr
}

// aggregatedView health-filters the backend registry and aggregates capabilities
Expand Down
Loading
Loading