Skip to content

fix: close log files in CommonRunGraph to prevent resource leak#102

Open
AR21SM wants to merge 1 commit intokrkn-chaos:mainfrom
AR21SM:fix/close-log-files
Open

fix: close log files in CommonRunGraph to prevent resource leak#102
AR21SM wants to merge 1 commit intokrkn-chaos:mainfrom
AR21SM:fix/close-log-files

Conversation

@AR21SM
Copy link
Contributor

@AR21SM AR21SM commented Jan 27, 2026

User description

Description

Fixes #101

Log files created in CommonRunGraph() were never closed, causing a resource leak. Each scenario run leaked a file descriptor, which could lead to hitting OS limits during long-running graph executions.

Added defer file.Close() in the goroutine to ensure proper cleanup.

Before:

go func() {
    defer wg.Done()
    _, err = orchestrator.RunAttached(..., file, file, ...)
}()

After:

go func() {
    defer file.Close()
    defer wg.Done()
    _, err = orchestrator.RunAttached(..., file, file, ...)
}()

Documentation

  • Is documentation needed for this update?

No - this is a bug fix with no user-facing changes.

Related Documentation PR (if applicable)

N/A


PR Type

Bug fix


Description

  • Close log files in CommonRunGraph goroutine to prevent resource leak

  • Added defer statement to ensure file cleanup after orchestrator execution

  • Prevents file descriptor exhaustion during long-running graph executions


Diagram Walkthrough

flowchart LR
  A["Log file created"] --> B["Goroutine spawned"]
  B --> C["defer file.Close added"]
  C --> D["File properly cleaned up"]
  D --> E["Resource leak prevented"]
Loading

File Walkthrough

Relevant files
Bug fix
common_functions.go
Add file close defer in goroutine                                               

pkg/scenarioorchestrator/common_functions.go

  • Added defer file.Close() statement in goroutine to ensure log file
    cleanup
  • Placed before defer wg.Done() to guarantee file closure before
    goroutine exit
  • Fixes resource leak where file descriptors were never released
+1/-0     

@qodo-code-review
Copy link

qodo-code-review bot commented Jan 27, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟢
🎫 #101
🟢 Ensure log files created with os.Create() in pkg/scenarioorchestrator/common_functions.go
are properly closed to prevent file descriptor leaks during scenario runs.
Add defer file.Close() inside the goroutine that calls orchestrator.RunAttached(...)
(alongside existing defer wg.Done()), so the file is closed when the goroutine exits.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Close error ignored: The newly added defer file.Close() does not check/record the returned close error, which
could hide failures flushing the scenario log to disk.

Referred Code
defer file.Close()
defer wg.Done()

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

qodo-code-review bot commented Jan 27, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix race condition in goroutine

To fix a race condition, pass loop variables (scID, scenario, containerName,
filename, file, env, volumes) as arguments to the goroutine to ensure each
goroutine operates on the correct data.

pkg/scenarioorchestrator/common_functions.go [76-86]

 			wg.Add(1)
 
-			go func() {
-				defer file.Close()
+			go func(scen models.Scenario, cName, fName, scenarioID string, f *os.File, e []string, v map[string]string) {
+				defer f.Close()
 				defer wg.Done()
-				_, err = orchestrator.RunAttached(scenario.Image, containerName, env, cache, volumes, file, file, nil, ctx, registry)
+				_, err = orchestrator.RunAttached(scen.Image, cName, e, cache, v, f, f, nil, ctx, registry)
 				if err != nil {
-					commChannel <- &models.GraphCommChannel{Layer: &step, ScenarioID: &scID, ScenarioLogFile: &filename, Err: err}
+					commChannel <- &models.GraphCommChannel{Layer: &step, ScenarioID: &scenarioID, ScenarioLogFile: &fName, Err: err}
 					return
 				}
-			}()
+			}(scenario, containerName, filename, scID, file, env, volumes)
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical race condition caused by capturing loop variables in a closure and provides a complete and accurate fix, preventing potential data corruption and resource leaks.

High
  • Update

@AR21SM
Copy link
Contributor Author

AR21SM commented Jan 27, 2026

@paigerube14 @tsebastiani

@tsebastiani
Copy link
Contributor

please update the fork and rebase on main

Signed-off-by: AR21SM <mahajanashishar21sm@gmail.com>
@AR21SM AR21SM force-pushed the fix/close-log-files branch from 3e6dce3 to c16d33b Compare January 30, 2026 02:46
@AR21SM
Copy link
Contributor Author

AR21SM commented Jan 30, 2026

please update the fork and rebase on main

done

@AR21SM
Copy link
Contributor Author

AR21SM commented Feb 9, 2026

@tsebastiani

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Resource leak - log files never closed in CommonRunGraph

2 participants