fix(test): attempt to fix HTTP audit functional test flake and artifact log writer bug

[Clee2691]

Description

• Fix intermittent context deadline exceeded failure in the HTTP audit log timestamp functional test by giving it a fresh framework instead of reusing one pre-configured for application logs
• Fix a Go defer argument evaluation bug in BufferedLogWriter.FlushToArtifactsDir that silently prevented test failure artifact logs from being written

NO-JIRA

/cc @vparfonov
/assign @jcantrill

Summary by CodeRabbit

• Bug Fixes
  • Improved buffered log flushing so logs are written to artifact files more reliably and closed/flushed promptly when artifact output is successfully created.
  • Refined flush behavior to consistently restore the active output stream when artifact flushing is not used.
• Tests
  • Updated the HTTP audit-log timestamp functional test to initialize a fresh functional framework instance within each table-driven test case.

[coderabbitai]

📝 Walkthrough

Walkthrough

Modified buffered log artifact flushing to close files immediately after writing and added framework initialization to one HTTP forwarder functional test entry.

Changes

Test infrastructure fixes

Layer / File(s)	Summary
Buffered artifact flush path test/framework/common/log/buffered_log_writer.go	FlushToArtifactsDir now locks internally, writes buffered logs to a created artifact file immediately, closes it immediately, restores w.out to stdout, and uses a shared locked flush helper for the non-artifact path and Flush().
HTTP table entry setup test/functional/outputs/http/forward_to_http_test.go	The audit-log timestamp table entry now initializes framework with functional.NewCollectorFunctionalFramework() before configuring the forwarder and running deploy/write/read assertions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Related Issues: None specified.

Related PRs: None specified.

Suggested labels: test, bugfix

Suggested reviewers: None specified.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately captures both fixes in the PR and is specific enough for history scanning.
Description check	✅ Passed	The description covers the two fixes and includes /cc and /assign, but it omits the Links section from the template.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

test/framework/common/log/buffered_log_writer.go (1)

44-52: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Unsynchronized mutation of w.out alongside a mutex-protected Flush().

w.out is reassigned directly at Lines 47 and 50, outside w.mtx, while Flush() (Lines 59-66) reads w.out under lock. If any concurrent writer (e.g., another goroutine calling Write/Flush on the same instance) races with this reassignment, it's an unsynchronized access to shared state that go test -race would flag. Low practical risk given typical single-goroutine test-cleanup usage, but worth guarding for consistency with the rest of the type's locking discipline.

🔒 Suggested fix: guard `w.out` mutation with the mutex

			if file, err := os.Create(fullPath); err != nil {
				w.log.Error(err, fmt.Sprintf("Unable to flush logs to file: %s", fullPath))
			} else {
+				w.mtx.Lock()
				w.out = file
-				w.Flush()
+				w.mtx.Unlock()
+				w.Flush()
				errors.LogIfError(file.Close())
+				w.mtx.Lock()
				w.out = os.Stdout
+				w.mtx.Unlock()
				return
			}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/framework/common/log/buffered_log_writer.go` around lines 44 - 52, The
`bufferedLogWriter` mutates shared state unsafely: `w.out` is reassigned in the
`os.Create` success path while `Flush()` uses `w.mtx` to protect access, so
update the `Write`/flush path to guard every `w.out` read and write with the
same mutex. Use the `bufferedLogWriter` methods (`Write` and `Flush`) as the
coordination point so the temporary switch to the file and the restoration back
to `os.Stdout` happen under lock, matching the type’s existing locking
discipline.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@test/framework/common/log/buffered_log_writer.go`:
- Around line 44-52: The `bufferedLogWriter` mutates shared state unsafely:
`w.out` is reassigned in the `os.Create` success path while `Flush()` uses
`w.mtx` to protect access, so update the `Write`/flush path to guard every
`w.out` read and write with the same mutex. Use the `bufferedLogWriter` methods
(`Write` and `Flush`) as the coordination point so the temporary switch to the
file and the restoration back to `os.Stdout` happen under lock, matching the
type’s existing locking discipline.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8def264e-926a-4059-9666-016d04fea379

📥 Commits

Reviewing files that changed from the base of the PR and between 79ace36 and e5997d8.

📒 Files selected for processing (2)

• test/framework/common/log/buffered_log_writer.go
• test/functional/outputs/http/forward_to_http_test.go

[coderabbitai]

🧹 Nitpick comments (1)

test/framework/common/log/buffered_log_writer.go (1)

60-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider documenting the locking precondition on flushLocked.

The name implies the caller must hold w.mtx, but there's no comment enforcing that contract for future maintainers who might add new call sites.

📝 Suggested doc comment

+// flushLocked writes the buffer to w.out and resets it.
+// Callers must hold w.mtx before invoking this method.
 func (w *BufferedLogWriter) flushLocked() {
 	if _, err := w.out.Write(w.buffer); err != nil {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/framework/common/log/buffered_log_writer.go` around lines 60 - 65, Add a
doc comment to BufferedLogWriter.flushLocked that clearly states it must only be
called while w.mtx is already held. Keep the contract explicit for future call
sites, and make sure the comment sits on the flushLocked method so the locking
precondition is easy to find alongside write and flush behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@test/framework/common/log/buffered_log_writer.go`:
- Around line 60-65: Add a doc comment to BufferedLogWriter.flushLocked that
clearly states it must only be called while w.mtx is already held. Keep the
contract explicit for future call sites, and make sure the comment sits on the
flushLocked method so the locking precondition is easy to find alongside write
and flush behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3c78c5a2-43f3-4e48-bc09-c9e098cf6d96

📥 Commits

Reviewing files that changed from the base of the PR and between e5997d8 and dc1a01f.

📒 Files selected for processing (2)

• test/framework/common/log/buffered_log_writer.go
• test/functional/outputs/http/forward_to_http_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• test/functional/outputs/http/forward_to_http_test.go

[vparfonov]

/lgtm

[vparfonov]

/retest-required

[jcantrill]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Clee2691, jcantrill

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [jcantrill]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Clee2691]

/retest

[openshift-ci]

@Clee2691: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-using-bundle	dc1a01f	link	false	/test e2e-using-bundle

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.