Skip to content

Commit 86ef368

Browse files
koki-developclaude
andauthored
fix: resolve file handle resource leak in CLI processing (#174)
Co-authored-by: Claude <[email protected]>
1 parent 002f23e commit 86ef368

File tree

2 files changed

+23
-45
lines changed

2 files changed

+23
-45
lines changed

CLAUDE.md

Lines changed: 11 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -102,35 +102,7 @@ The project uses:
102102

103103
### 🚨 Critical Issues (Fix Immediately)
104104

105-
#### 1. Resource Leak in File Handling
106-
**File**: `cmd/root.go:54-63`
107-
**Issue**: Files are deferred to close at function end, not loop iteration end
108-
```go
109-
for _, filename := range args {
110-
f, err := os.Open(filename)
111-
if err != nil {
112-
return err
113-
}
114-
defer func() { _ = f.Close() }() // BUG: All files close at function end
115-
// ...
116-
}
117-
```
118-
**Fix**: Close files immediately after processing each one:
119-
```go
120-
for _, filename := range args {
121-
f, err := os.Open(filename)
122-
if err != nil {
123-
return err
124-
}
125-
err = g.Print(os.Stdout, f, gat.WithPretty(flagPretty), gat.WithFilename(filename))
126-
f.Close() // Close immediately
127-
if err != nil {
128-
return err
129-
}
130-
}
131-
```
132-
133-
#### 2. Extremely Low Test Coverage (4.7%)
105+
#### Extremely Low Test Coverage (4.7%)
134106
**Missing Coverage**:
135107
- Core functionality (`internal/gat/gat.go`)
136108
- CLI layer (`cmd/`)
@@ -142,7 +114,7 @@ for _, filename := range args {
142114
- Error injection tests for file I/O operations
143115
- Tests for different image formats and sizes
144116

145-
#### 3. Memory Safety Issues
117+
#### Memory Safety Issues
146118
**File**: `internal/gat/gat.go:142-146, 202-236`
147119
**Issues**:
148120
- No size limits when loading files/images into memory
@@ -156,7 +128,7 @@ for _, filename := range args {
156128

157129
### ⚠️ High Priority Issues
158130

159-
#### 4. Silent Error Suppression
131+
#### Silent Error Suppression
160132
**Files**: `cmd/root.go:59`, `internal/gat/gat.go:166, 244`
161133
```go
162134
defer func() { _ = f.Close() }() // Ignoring close errors
@@ -170,7 +142,7 @@ defer func() {
170142
}()
171143
```
172144

173-
#### 5. Performance Bottlenecks
145+
#### Performance Bottlenecks
174146
**String Concatenation**: `internal/gat/gat.go:142-146`
175147
```go
176148
buf := new(bytes.Buffer)
@@ -186,7 +158,7 @@ src = buf.String() // Unnecessary copy
186158
- No memory pool for frequent image operations
187159
- Loads entire image into memory before size checking
188160

189-
#### 6. Hard-coded Magic Values
161+
#### Hard-coded Magic Values
190162
**File**: `internal/gat/gat.go:103-135`
191163
- Binary message string is hard-coded and very long (line 135)
192164
- Magic number 1024 for peek size and binary detection (lines 104, 254-257)
@@ -203,7 +175,7 @@ const (
203175

204176
### 🔧 Medium Priority Issues
205177

206-
#### 7. Global State in Prettier Registry
178+
#### Global State in Prettier Registry
207179
**File**: `internal/prettier/registry.go:3`
208180
```go
209181
var Registry = map[string]Prettier{} // Global mutable state
@@ -219,11 +191,11 @@ func NewRegistry() *Registry {
219191
}
220192
```
221193

222-
#### 8. Tight Coupling
194+
#### Tight Coupling
223195
**File**: `internal/gat/gat.go:44-75`
224196
The `New()` function directly couples to multiple internal packages, making it hard to test in isolation.
225197

226-
#### 9. Security Concerns
198+
#### Security Concerns
227199
**Dependency Vulnerabilities**:
228200
- Several dependencies at v0.x versions
229201
- `github.com/yosssi/gohtml` (last updated 2020)
@@ -234,7 +206,7 @@ The `New()` function directly couples to multiple internal packages, making it h
234206
- Could cause OOM with very large files
235207
- No timeout for file operations
236208

237-
#### 10. Documentation Issues
209+
#### Documentation Issues
238210
**Missing Godoc Comments**:
239211
- `type Config struct` - no documentation for fields
240212
- `func New()` - no documentation for error conditions
@@ -245,7 +217,7 @@ The `New()` function directly couples to multiple internal packages, making it h
245217
- Image processing pipeline
246218
- Error handling strategy
247219

248-
#### 11. Dependency Management
220+
#### Dependency Management
249221
**Outdated Dependencies**:
250222
- `github.com/alecthomas/chroma/v2` v2.17.2 → v2.18.0
251223
- `github.com/bmatcuk/doublestar/v4` v4.7.1 → v4.8.1
@@ -256,7 +228,7 @@ The `New()` function directly couples to multiple internal packages, making it h
256228
### 📈 Action Plan
257229

258230
#### Phase 1 (Week 1) - Critical Fixes
259-
- [ ] Fix file handle leak in `cmd/root.go:54-63`
231+
- [x] ~~Fix file handle leak in `cmd/root.go:54-63`~~ **COMPLETED**
260232
- [ ] Add basic integration tests to achieve >50% coverage
261233
- [ ] Add size limits for file and image processing
262234
- [ ] Fix error handling - don't silently ignore errors

cmd/root.go

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,17 @@ import (
99
"golang.org/x/term"
1010
)
1111

12+
// processFile handles opening, processing, and closing a single file with proper defer scope
13+
func processFile(g *gat.Gat, filename string, opts ...gat.PrintOption) error {
14+
f, err := os.Open(filename)
15+
if err != nil {
16+
return err
17+
}
18+
defer func() { _ = f.Close() }()
19+
20+
return g.Print(os.Stdout, f, opts...)
21+
}
22+
1223
var rootCmd = &cobra.Command{
1324
Use: "gat [file]...",
1425
Short: "cat alternative written in Go",
@@ -52,12 +63,7 @@ var rootCmd = &cobra.Command{
5263
}
5364

5465
for _, filename := range args {
55-
f, err := os.Open(filename)
56-
if err != nil {
57-
return err
58-
}
59-
defer func() { _ = f.Close() }()
60-
if err := g.Print(os.Stdout, f, gat.WithPretty(flagPretty), gat.WithFilename(filename)); err != nil {
66+
if err := processFile(g, filename, gat.WithPretty(flagPretty), gat.WithFilename(filename)); err != nil {
6167
return err
6268
}
6369
}

0 commit comments

Comments
 (0)