[venkata-stepsecurity]

Problem

The agent never changes the kernel audit queue settings, so the kernel defaults apply: backlog limit 64 and backlog wait time 60s. When a job makes audited syscalls faster than the agent reads records, the queue fills, and the kernel puts each audited syscall to sleep until there is room. The job slows down rather than the kernel dropping records.

In a workload that streams many small sends over localhost TCP (480 s run, ubuntu-latest), the kernel reported backlog_wait_time_actual of about 1,150,000 ms. That is roughly 2.4 s of syscalls sleeping per second of the run.

Change

This uses the default "auto" backpressure strategy from Elastic Auditbeat (audit_linux.go, initClient). osquery also ships with a zero backlog wait time.

• Failure mode set to silent (Auditbeat's default), so a dropped record cannot panic the kernel.
• Backlog limit 8192 (Auditbeat's default).
• Backlog wait time 0 when the kernel supports it, so the kernel drops a record when the queue is full instead of blocking the syscall.
• On kernels that cannot set the wait time, a 5000/s rate limit instead, as Auditbeat does.
• The kernel's lost record counter is polled every 15 s on a separate audit connection, as Auditbeat does. New losses are logged.

The settings are applied before audit is enabled and before rules are added. They are skipped if audit config is locked. A failure is logged and does not stop monitoring.

Audit rules are unchanged.

Trade-off

Under sustained overload, the kernel now drops records instead of slowing the job. Auditbeat and osquery make the same choice. Losses are now visible in the agent log.

Testing

• Unit tests for the settings logic (kernel defaults, no wait time support, existing rate limit, panic failure mode, locked config, setter error), the lost counter (delta, reset), and poller shutdown.
• go mod vendor, go mod verify, go vet and go test -race on Linux with Go 1.27.3. TestRun and Test_addAuditRules fail in a container on both this branch and int, because they need a real runner; CI covers them.
• End to end on ubuntu-latest, with the same 480 s workload, the int build at 2235fe6 against this branch:

	int 2235fe6	this branch	no agent
workload tests/min	246	1577 (873 in an earlier run)	2627
kernel wait time actual	1,149,790 ms	0	-
kernel lost records	0	33,222,871	-
agent CPU, median	102%	83%	-
agent peak RSS	49 MiB	71 MiB	-
failure mode	1	0	-

On both builds, at start, middle and end of the run, all detection probes were reported: HTTPS connections, a UDP sendto, and the source file overwrite.

[step-security-bot-int]

Please find StepSecurity AI-CodeWise code comments below.

Code Comments

procmon_linux_test.go

• [High]Check error return values from each call to Set methods in configureBackpressure*
  In configureBackpressure, each call to methods like SetFailure, SetBacklogLimit, etc., may return an error. Ignoring these errors violates defensive programming practices recommended by authoritative sources such as the Go Blog and Effective Go. Handling the errors immediately prevents hidden failures that can lead to incorrect system behavior or silent misconfiguration. Modify configureBackpressure to check and propagate errors from each Set* method. For example:

err := qc.SetFailure(...)
if err != nil {
return err
}
// Repeat for all subsequent Set* calls

• [High]Use deep equality checks when comparing slices in tests
  In the test configureBackpressure(), slice comparisons use fmt.Sprint which is string conversion rather than semantic equivalence. This can produce false positives or negatives. The Go standard library's reflect.DeepEqual provides semantic slice comparison and is recommended as per Go testing best practices. Replace comparisons like 'fmt.Sprint(f.calls) != fmt.Sprint(c.want)' with 'reflect.DeepEqual(f.calls, c.want)'.
  Import "reflect" and update assertions accordingly.
• [Medium]Improve test coverage by adding negative and boundary test cases
  While the test cases cover many configurations, they omit tests for situations such as feature bitmap lacking all relevant flags, zero or negative backlog limits, and large values. Comprehensive testing is critical to catch edge cases and ensure robustness (source: ISTQB and Go testing guidelines). Add test cases with zero or negative BacklogLimit, status with no FeatureBitmap flags set, and very large integer values. This will help verify correct handling of edge conditions.
• [Medium]Avoid using fmt.Sprint for logging or comparing values in tests
  Using fmt.Sprint converts slices and other complex data types to strings, which is not reliable for structured data comparison. This can obscure problems and reduce maintainability, violating testing principles emphasized in Go's official blog and style guidelines. Use appropriate data comparison or printing alternatives such as reflect.DeepEqual for comparison and testify or cmp packages for richer error messages.
• [Medium]Make use of table-driven testing idioms by including test sub-names and proper assertion libraries
  While the tests use t.Run and basic assertions, more descriptive assertion failures will improve debugging. Go best practices encourage descriptive errors and assertion helpers (as found in testify or gotest.tools). Use assert or require packages for better assertion messages, or wrap failures with contextual information. Example:
  assert.Equal(t, c.want, f.calls, "Calls mismatch in test %q", c.name)
• [Low]Add comments describing fakeQueueControl methods' purpose and parameters
  Current code methods like SetFailure or SetBacklogLimit do not have doc comments, which hinders maintainability and readability as prescribed by Effective Go and golangci-lint recommendations. Add comments preceding each method describing its function, parameters, and expected behavior.
• [Low]Consistently handle error paths in tests
  In Test_configureBackpressure_error, the check for f.calls length is less descriptive. Providing explicit error messages aids test failure diagnosis. Update error messages with descriptive failure reasons, e.g.,
  if len(f.calls) != 1 {
  t.Errorf("Expected 1 call before error, got %d calls: %v", len(f.calls), f.calls)
  }
• [Low]Use defer to close channels and cleanup resources in test goroutines
  In Test_monitorKernelLost_stops, the test could better manage lifecycle with defer statements to avoid subtle leaks. This is aligned with Go concurrency best practices. Add defer statements as appropriate when spawning goroutines; ensure that channel closure and resource cleanup happen reliably.

procmon.go

• [High]Add comments explaining the purpose and usage of new constants for maintainability
  While some comments are added for the new constants, they should be more detailed and consistent to improve code readability and maintainability. According to 'Clean Code' by Robert C. Martin, meaningful comments help future developers understand the intent behind code. Expand existing comments to clarify the rationale behind each constant, their relationship to kernel audit settings, and implications of changing these values.
• [High]Ensure constants related to kernel audit settings are configurable via configuration files or environment variables
  Hardcoding kernel audit parameters reduces flexibility and can make tuning or testing difficult. The Twelve-Factor App principles and best practices suggest configuration should be externalized to allow adjustments without code changes. Refactor the constants auditBacklogLimit, auditFallbackRateLimit, and kernelLostCheckInterval to be initialized from application configuration or environment variables, with fallback defaults.
• [Medium]Use a consistent naming convention for atomic counters to clearly indicate their thread-safe nature
  Naming atomic variables appropriately improves code clarity about concurrent access. The Go official concurrency patterns recommend naming possibly with suffix 'Atomic' or prefix to denote atomicity. Rename 'kernelLost' field to 'kernelLostAtomic' or similar to reflect its atomic type explicitly.
• [Medium]Group related constant declarations in a single block with consistent style
  Defining related constants in one block improves code organization and readability as recommended by effective Go style guidelines. Combine the new kernel audit queue constants with existing 'const' declaration block, e.g., merge them below the existing constants at the top of the file.
• [Low]Add unit tests or integration tests covering new atomic counter usage to prevent regressions
  Testing changes to the ProcessMonitor struct fields can help ensure correctness under concurrency as per best practices in Go testing from the standard library documentation. Add tests that simulate concurrent increments on 'kernelLost' atomic and verify accurate counting.
• [Low]Fix minor formatting inconsistencies in comment style and spacing
  Consistent comment formatting improves readability; gofmt enforces this standard. Ensure comments start with a capital letter and end with a period; align with existing style.

procmon_linux.go

• [High]Avoid uncontrolled goroutine leaks by ensuring monitorKernelLost goroutine exits appropriately
  The MonitorProcesses function launches a goroutine monitorKernelLost which relies on a channel stopLostCheck to terminate. The stopLostCheck is closed via defer when MonitorProcesses returns. If MonitorProcesses never returns (e.g., it loops for the lifetime of the process), the goroutine may leak, or conversely, if MonitorProcesses ends early, the goroutine is stopped promptly. Explicit documentation and testing should confirm proper lifecycle management. This follows best practices for goroutine lifecycle management recommended in "Go Concurrency Patterns" (https://go.dev/blog/concurrency_patterns). Add explicit documentation specifying the lifecycle and ownership of stopLostCheck channel and ensure MonitorProcesses terminates or otherwise coordinates with goroutine termination to prevent leaks. Alternatively, consider context.Context for cancellation.
• [High]Check errors returned from WriteLog calls
  The calls to WriteLog in configureBackpressure and MonitorProcesses ignore any error returns (if any). While WriteLog may be void, if it could fail (e.g., IO-related), those errors should be handled or at least logged to avoid silent failures. Ignoring errors can hide critical issues. This aligns with error handling best practices from "Effective Go" (https://golang.org/doc/effective_go#errors). If WriteLog returns an error, propagate or handle the error. If it does not return an error, ensure the logging implementation is reliable or consider improving error visibility.
• [Medium]Avoid race conditions by protecting shared variables with synchronization primitives
  The method noteKernelLost updates p.kernelLost presumably a shared counter without synchronization primitives (mutex or atomic operations). Concurrent goroutine calls can cause race conditions leading to incorrect counts or data corruption. According to "Go Memory Model" (https://golang.org/ref/mem), shared variables accessed by multiple goroutines must be synchronized. Protect access and updates to p.kernelLost using sync.Mutex or atomic package to ensure thread-safe operations.
• [Medium]Avoid recreating AuditClient on every unsuccessful status retrieval
  In monitorKernelLost, if client.GetStatus() returns an error, client is closed and nil is set. On the next tick, a new client is opened. Frequent errors may cause repeated open/close cycles which are resource-intensive. To improve robustness and performance, implement exponential backoff or limit retries as recommended in "Robustness Principle" (https://en.wikipedia.org/wiki/Robustness_principle). Add retry limits or exponential backoff when re-creating AuditClient to avoid tight loops on failure.
• [Medium]Check for possible integer underflow in noteKernelLost when current < last
  The logic in noteKernelLost assumes that if current < last, the counter was reset and sets delta = current (effectively missing lost events during the reset). This might undercount lost records. Defensive coding would handle this case explicitly or log a warning. This is consistent with defensive programming principles as discussed in "Secure Programming Cookbook" (https://www.informit.com/articles/article.aspx?p=2053172). Add explicit handling/logging if current < last to indicate possible resets and avoid missing delta calculations.
• [Low]Use context.Context for cancellation instead of custom channels where applicable
  Using context.Context is the idiomatic way in Go to handle cancellation and timeouts, instead of custom done channels. This improves integration, testability, and adheres to modern Go best practices (https://blog.golang.org/context). Replace stopLostCheck channel with a context.Context passed down to monitorKernelLost and use context cancellation for stopping the goroutine.
• [Low]Declare constants explicitly for magic numbers
  Values such as 0, 2, and specific numeric limits appear in code (e.g., status.Enabled == 2) without named constants. Using named constants improves readability and maintainability as per "Clean Code" principles (https://www.investigatii.md/uploads/resurse/Clean_Code_Robert_C._Martin.pdf). Define named constants for magic numbers like auditStatusLocked=2 to clarify code intent.
• [Low]Add comments describing the meaning of audit status codes
  The code compares status.Enabled and status.Failure to numeric values but does not explain what these mean, reducing readability and maintainability, violating best practices from "Go Code Review Comments" (https://github.com/golang/go/wiki/CodeReviewComments). Add inline comments or documentation describing the meaning of these status codes or refer to constants/enums.

Feedback

We appreciate your feedback in helping us improve the service! To provide feedback, please use emojis on this comment. If you find the comments helpful, give them a 👍. If they aren't useful, kindly express that with a 👎. If you have questions or detailed feedback, please create n GitHub issue in StepSecurity/AI-CodeWise.