Close workflow audit log writers - #6110
Conversation
jerm-dro
left a comment
jerm-dro
left a comment
There was a problem hiding this comment.
Thanks for taking this on — the diagnosis and the design are right. Retaining the writer, factoring out the shared close helper, and catching the stdout trap rather than inheriting it are all exactly what #6094 asked for, and putting the auditor's lifecycle on coreVMCP (next to stopStore and healthMonitor) is the correct owner.
Two blockers, both mechanical, both about the tests rather than the fix:
- The two stdout tests pass against the unfixed code —
os.Stdout.Close()returnsnil, sorequire.NoErrorcan't distinguish fixed from broken. As written they'd stay green if the guard were removed. - The
coreVMCPchange is the code that actually plugs the leak, and it has no coverage — including the threeNewerror paths.
Details and suggested code in the line comments.
| func TestAuditor_CloseDoesNotCloseStdout(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| auditor := &Auditor{logWriter: os.Stdout} | ||
|
|
||
| require.NoError(t, auditor.Close()) | ||
| } |
There was a problem hiding this comment.
blocker: This test passes against the unfixed code, so it can't protect the guard it's guarding.
os.Stdout.Close() succeeds — it closes fd 1 and returns nil. So require.NoError(t, auditor.Close()) is satisfied whether or not closeLogWriter skips stdout. Delete the guard in auditor.go and this test still goes green.
There's a second-order hazard: the test is t.Parallel(), so if the guard ever regresses, this test closes fd 1 for the whole test binary while other tests in the package are running — they'd fail confusingly and this one would report a pass.
The assertion has to observe the descriptor, not the error:
func TestAuditor_CloseDoesNotCloseStdout(t *testing.T) {
t.Parallel()
auditor := &Auditor{logWriter: os.Stdout}
require.NoError(t, auditor.Close())
// The point of the guard: fd 1 must still be usable afterwards.
// Write(nil) touches the descriptor without emitting output.
_, err := os.Stdout.Write(nil)
require.NoError(t, err, "Close() must not close os.Stdout")
}On the fixed code Write(nil) returns nil; on the unfixed code it returns a non-nil error (file already closed). I verified both directions.
The same change is needed in the does not close stdout subtest of TestWorkflowAuditor_Close above — assert.Same(t, os.Stdout, auditor.logWriter) only proves the field was retained, not that the fd survived.
(Minor, while you're here: this test covers Auditor, not WorkflowAuditor — it belongs in auditor_test.go.)
| if c.workflowAuditor != nil { | ||
| if err := c.workflowAuditor.Close(); err != nil { | ||
| slog.Warn("failed to close workflow auditor", "error", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
46bd4b0 to
812b3ed
Compare
|
Addressed the two mechanical blockers in Changes:
Local verification:
I also started |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6110 +/- ##
==========================================
+ Coverage 72.37% 72.99% +0.62%
==========================================
Files 733 742 +9
Lines 75804 78508 +2704
==========================================
+ Hits 54860 57305 +2445
- Misses 17046 17211 +165
- Partials 3898 3992 +94 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Follow-up on the local
I re-ran both failures in isolation and at package scope with the same race/ldflags shape, and they passed:
So I don't see evidence that those two full-suite failures are caused by the workflow-auditor close changes. |
Signed-off-by: Emre K <110906681+kocaemre@users.noreply.github.com>
812b3ed to
17d4e5b
Compare
|
Added one more focused core close-path regression in The new subtest pre-closes the retained workflow auditor and then calls Verification:
|
2 participants
Summary
Config.LogFileis set becauseNewWorkflowAuditoropens a log writer but the returned auditor does not retain or expose a way to close it.WorkflowAuditor.Close(), and close the optional workflow auditor fromcoreVMCP.Close()and core construction error paths.os.Stdout/os.Stderrwhen audit logging uses default output.Fixes #6094
Type of change
Test plan
task test)task test-e2e)task lint-fix)Manual testing:
PATH=/usr/local/go/bin:/root/go/bin:$PATH task lintPATH=/usr/local/go/bin:/root/go/bin:$PATH go test -race ./pkg/audit ./pkg/vmcp/corePATH=/usr/local/go/bin:/root/go/bin:$PATH go test ./pkg/vmcp/composerAPI Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Changes
pkg/audit/auditor.gopkg/audit/workflow_auditor.goClose().pkg/audit/workflow_auditor_test.gopkg/vmcp/core/core_vmcp.goDoes this introduce a user-facing change?
No. This releases an internal audit log file descriptor when the workflow auditor owner is closed.