stacklok / stacklok/toolhive

WorkflowAuditor leaks the audit log file descriptor

Open
#6,094 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
2.2k
Forks
300
Avg merge
1d 15h
Merged PRs (30d)
184

Description

Found while reviewing #6046.

Problem

NewWorkflowAuditor opens the audit log through config.GetLogWriter():

// pkg/audit/workflow_auditor.go:33
logWriter, err := config.GetLogWriter()

When Config.LogFile is set, GetLogWriter returns an os.OpenFile handle (pkg/audit/config.go:69-81). But WorkflowAuditor has no Close method — the HTTP Auditor does:

// pkg/audit/auditor.go:109-114
func (a *Auditor) Close() error {
    if closer, ok := a.logWriter.(io.Closer); ok {
        return closer.Close()
    }
    return nil
}

So a file-backed WorkflowAuditor holds the descriptor for the process lifetime with no way for the owner to release it. WorkflowAuditor does not even retain the writer, so the fd is unreachable once the constructor returns.

Impact

One leaked fd per WorkflowAuditor instance. Bounded and low severity if the auditor is a long-lived singleton; it matters if one is ever constructed per workflow, per reconcile, or in tests that build many.

Suggested fix

Add Close() error mirroring Auditor.Close() (which means retaining the writer on the struct), and call it wherever the workflow auditor's lifetime ends.

While there, note a pre-existing hazard worth guarding in both types: with LogFile empty, GetLogWriter returns os.Stdout, which satisfies io.Closer — so Close() on a stdout-backed auditor closes fd 1. Auditor.Close() has this today; a new WorkflowAuditor.Close() should not inherit it. Skipping the close when the writer is os.Stdout fixes both.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with pkg/audit/workflow_auditor.go and compare its construction and fields with pkg/audit/auditor.go, then inspect pkg/audit/config.go and the workflow auditor's callers for lifetime boundaries. Done means the file-backed writer can be released, stdout is not closed, and the relevant owner paths call the new cleanup method.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, observability
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.