More tempdir cleanup #617
More tempdir cleanup #617
Conversation
WalkthroughReplaces internal c1z loader with a decompressor that returns both a decompressed DB file path and the temporary working directory. Adds centralized cleanup to close opened resources and remove temporary dirs on error/success; call sites and tests updated to the new signature. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-01-02T17:21:01.723ZApplied to files:
🧬 Code graph analysis (1)pkg/dotc1z/file.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/dotc1z/dotc1z.go (1)
52-58: Potential resource leak ifNewC1Filefails.If
decompressC1zsucceeds butNewC1Filefails (line 58), the temporary directory is not cleaned up. Consider cleaning up the working directory on error:🔎 Proposed fix
func NewExternalC1FileReader(ctx context.Context, tmpDir string, externalResourceC1ZPath string) (connectorstore.Reader, error) { - dbFilePath, _, err := decompressC1z(externalResourceC1ZPath, tmpDir) + dbFilePath, workingDir, err := decompressC1z(externalResourceC1ZPath, tmpDir) if err != nil { return nil, fmt.Errorf("error loading external resource c1z file: %w", err) } - return NewC1File(ctx, dbFilePath) + c1File, err := NewC1File(ctx, dbFilePath) + if err != nil { + _ = os.RemoveAll(workingDir) + return nil, err + } + return c1File, nil }
🧹 Nitpick comments (2)
pkg/dotc1z/file_test.go (1)
31-47: Missing cleanup of workingDir in successful test case.The "custom tmpDir" test creates a working directory that persists after the test. While
t.TempDir()handlestmpDircleanup, the createdworkingDircontains the db file and should be explicitly cleaned up to follow the pattern from the error case test:🔎 Proposed fix
t.Run("custom tmpDir", func(t *testing.T) { tmpDir := t.TempDir() customTmpDir := filepath.Join(tmpDir, "custom") err := os.MkdirAll(customTmpDir, 0755) require.NoError(t, err) defer os.RemoveAll(customTmpDir) nonExistentPath := filepath.Join(tmpDir, "nonexistent2.c1z") dbPath, workingDir, err := decompressC1z(nonExistentPath, customTmpDir) require.NoError(t, err) require.NotEmpty(t, dbPath) require.FileExists(t, dbPath) + defer os.RemoveAll(workingDir) // Verify it was created in the custom tmpDir require.Contains(t, dbPath, customTmpDir) require.Contains(t, workingDir, customTmpDir) })pkg/dotc1z/file.go (1)
64-68: Consider distinguishing "not found" from other errors.The current logic treats all
os.Staterrors the same as a zero-size file, but some errors (e.g., permission denied) may warrant different handling. This could mask real issues:🔎 Proposed improvement
stat, err := os.Stat(c1zPath) - if err != nil || stat.Size() == 0 { - // TODO(kans): it would be nice to know more about the error.... + if err != nil { + if !os.IsNotExist(err) { + // Non-trivial error (permission denied, etc.) - propagate it + return "", tmpDir, cleanupDir(err) + } + // File doesn't exist - treat as "create new" + return dbFilePath, tmpDir, cleanupDir(nil) + } + if stat.Size() == 0 { + // Empty file - treat as "create new" return dbFilePath, tmpDir, cleanupDir(nil) }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/dotc1z/c1file.gopkg/dotc1z/dotc1z.gopkg/dotc1z/file.gopkg/dotc1z/file_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-02T17:21:01.723Z
Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 616
File: pkg/synccompactor/compactor_test.go:44-52
Timestamp: 2026-01-02T17:21:01.723Z
Learning: In tests that verify cleanup behavior (e.g., ensuring temporary artifacts are removed or directories are empty after an operation), treat cleanup failures as test failures by asserting on the cleanup call (e.g., require.NoError(t, err) or assert.NoError(t, err)). This ensures that the cleanup path is explicitly tested and any cleanup error fails the test, confirming correct behavior of the cleanup code.
Applied to files:
pkg/dotc1z/file_test.go
🧬 Code graph analysis (1)
pkg/dotc1z/file.go (1)
pkg/dotc1z/decoder.go (2)
DecoderOption(57-57)NewDecoder(209-242)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (2)
pkg/dotc1z/c1file.go (1)
190-193: LGTM!The second return value (tmpDir) is correctly ignored here because cleanup is already handled by
C1File.Close()viacleanupDbDir(c.dbFilePath, ...)which removesfilepath.Dir(dbFilePath)— effectively the same directory.pkg/dotc1z/file.go (1)
30-56: LGTM!The cleanup closure correctly handles resource cleanup in reverse order of creation and properly joins errors. The conditional removal of
tmpDironly on error is correct — on success, the caller takes ownership of the directory.
kans
force-pushed
the
kans/cleanup-temp-dir-on-decode-error
branch
from
a7188ca to
4187470
Compare
kans
force-pushed
the
kans/cleanup-temp-dir-on-decode-error
branch
from
4187470 to
2182588
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/dotc1z/c1file.go (1)
191-211: Resource leak ifNewC1Filefails.If
decompressC1zsucceeds butNewC1Filefails (line 208), the temporary directory created bydecompressC1zis not cleaned up, leading to a resource leak.🔎 Proposed fix
c1File, err := NewC1File(ctx, dbFilePath, c1fopts...) if err != nil { - return nil, err + return nil, cleanupDbDir(dbFilePath, err) }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/dotc1z/c1file.gopkg/dotc1z/dotc1z.gopkg/dotc1z/file.gopkg/dotc1z/file_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/dotc1z/dotc1z.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-02T17:21:01.723Z
Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 616
File: pkg/synccompactor/compactor_test.go:44-52
Timestamp: 2026-01-02T17:21:01.723Z
Learning: In tests that verify cleanup behavior (e.g., ensuring temporary artifacts are removed or directories are empty after an operation), treat cleanup failures as test failures by asserting on the cleanup call (e.g., require.NoError(t, err) or assert.NoError(t, err)). This ensures that the cleanup path is explicitly tested and any cleanup error fails the test, confirming correct behavior of the cleanup code.
Applied to files:
pkg/dotc1z/file_test.go
🧬 Code graph analysis (1)
pkg/dotc1z/file.go (1)
pkg/dotc1z/decoder.go (2)
DecoderOption(57-57)NewDecoder(209-242)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: go-test (1.25.2, ubuntu-latest)
- GitHub Check: go-test (1.25.2, windows-latest)
- GitHub Check: go-lint
🔇 Additional comments (3)
pkg/dotc1z/file_test.go (1)
15-48: LGTM! Cleanup verification tests are well structured.The tests properly verify that:
- Temporary directories are cleaned up on decode errors (line 28)
- Working directories are created in the specified location (line 46)
The use of
require.NoDirExistsandrequire.Containsensures cleanup behavior is explicitly tested, aligning with best practices for cleanup verification.pkg/dotc1z/file.go (2)
18-21: Comment fix applied correctly.The typo identified in the previous review ("taht" → "that") has been fixed. The comment now accurately describes the function's behavior and return values.
21-94: Well-structured cleanup implementation.The refactored
decompressC1zproperly handles resource cleanup:
- Creates its own temporary directory for better isolation
- Defines a centralized
cleanupDirfunction that closes all resources in the correct order- Calls cleanup on all error paths and the success path
- Only removes the temporary directory on error (leaves it intact on success for the caller to use)
- Uses
errors.Jointo accumulate multiple errorsThe approach of returning the working directory as a second value enables test verification of cleanup behavior, which aligns with the PR objectives.
One test was incorrectly passing in the case that we couldn't decode a c1z (ie, we didn't cleanup the directory).
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.