fix: Retry the file deletion to prevent flakies for issue #938

[lazysegtree]

Summary by CodeRabbit

• Tests
  • Improved reliability of automated tests by moving cleanup to test teardown, adding retry-based deletion and clearer failure reporting.
  • Reduces flakiness (notably on Windows) and improves stability in CI and local runs.
  • No changes to application features, behavior, performance, or compatibility; end users will not observe functional differences.

[coderabbitai]

Warning

Rate limit exceeded

@lazysegtree has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 10 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between b7e6bb8 and 8283cf7.

📒 Files selected for processing (1)

• src/internal/handle_file_operation_test.go (1 hunks)

Walkthrough

Test teardown in src/internal/handle_file_operation_test.go was changed: cleanup of extractedDir and zipFile is now registered via t.Cleanup and performed with require.Eventually retry loops (using DefaultTestTimeout/DefaultTestTick) that report explicit failure messages on removal errors.

Changes

Cohort / File(s)	Summary of changes
Test cleanup via t.Cleanup src/internal/handle_file_operation_test.go	Removed immediate os.RemoveAll calls and registered a t.Cleanup that uses require.Eventually to retry os.RemoveAll on extractedDir and zipFile until success or timeout; added explicit error messages ("Failed to remove extracted directory %q: %v", "Failed to remove zip file %q: %v"); uses DefaultTestTimeout and DefaultTestTick.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant T as Test
  participant Teardown as t.Cleanup
  participant FS as Filesystem
  note over T: Test body runs (extraction assertions unchanged)
  T->>Teardown: register cleanup (require.Eventually loops)
  Teardown->>FS: os.RemoveAll(extractedDir) (retry until success or timeout)
  alt removed
    note over Teardown,FS: success
  else error
    note over Teardown: require.Eventually reports failure with path/error
  end
  Teardown->>FS: os.RemoveAll(zipFile) (retry until success or timeout)
  alt removed
    note over Teardown,FS: success
  else error
    note over Teardown: require.Eventually reports failure with path/error
  end
  note over Teardown: cleanup runs at test end (even on failure)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I nibble crumbs from tests at dusk,
Register a cleanup, steady, brusque.
Tick by tick I try once more,
Until the warren shows no store.
Paths cleared, I twitch my nose—huzzah! 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title clearly and concisely summarizes the main change: adding retry logic for deleting files to avoid flaky tests tied to issue #938, which matches the diff that replaces immediate removals with a t.Cleanup using require.Eventually to repeatedly call os.RemoveAll. It is specific, related to the changeset, and readable for teammates scanning PR history. The conventional "fix:" prefix is appropriate for this test-flakiness fix.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/internal/handle_file_operation_test.go (2)

155-161: Prefer require.Eventually and include the last error + path for faster debug.

This fails the subtest immediately on cleanup failure and surfaces the real OS error in the failure message, while keeping the change small and simple.

Apply this diff:

-			// Cleanup with retry for Windows file handle issues
-			assert.Eventually(t, func() bool {
-				return os.RemoveAll(extractedDir) == nil
-			}, DefaultTestTimeout, DefaultTestTick, "Failed to remove extracted directory")
-			assert.Eventually(t, func() bool {
-				return os.RemoveAll(zipFile) == nil
-			}, DefaultTestTimeout, DefaultTestTick, "Failed to remove zip file")
+			// Cleanup with retry for Windows file handle issues
+			var rmExtractErr error
+			require.Eventually(t, func() bool {
+				rmExtractErr = os.RemoveAll(extractedDir)
+				return rmExtractErr == nil
+			}, DefaultTestTimeout, DefaultTestTick, "Failed to remove extracted directory %q: %v", extractedDir, rmExtractErr)
+			var rmZipErr error
+			require.Eventually(t, func() bool {
+				rmZipErr = os.RemoveAll(zipFile)
+				return rmZipErr == nil
+			}, DefaultTestTimeout, DefaultTestTick, "Failed to remove zip file %q: %v", zipFile, rmZipErr)

Note: if your testify version lacks require.Eventually, keep assert.Eventually but still capture and log lastErr.

---

155-161: Run cleanup via t.Cleanup so it executes even if the test fails earlier.

Keeps tests independent and prevents artifacts from leaking into later subtests, matching your simplicity preference without over-engineering.

Add right after extractedDir is computed (Line 143):

t.Cleanup(func() {
	var rmExtractErr error
	require.Eventually(t, func() bool {
		rmExtractErr = os.RemoveAll(extractedDir)
		return rmExtractErr == nil
	}, DefaultTestTimeout, DefaultTestTick, "Failed to remove extracted directory %q: %v", extractedDir, rmExtractErr)

	var rmZipErr error
	require.Eventually(t, func() bool {
		rmZipErr = os.RemoveAll(zipFile)
		return rmZipErr == nil
	}, DefaultTestTimeout, DefaultTestTick, "Failed to remove zip file %q: %v", zipFile, rmZipErr)
})

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a07eac0 and 6bf99b3.

📒 Files selected for processing (1)

• src/internal/handle_file_operation_test.go (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.

Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the tests were actually passing despite lazysegtree reporting crashes. The real issue was that production image preview crashes occurred during actual application usage due to duplicate ImagePreviewer instances (one in defaultModelConfig and one in preview.New()), while the test environment didn't stress the image preview system enough to expose the conflicts.

Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.

Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.

Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.

Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.

Learnt from: lazysegtree
PR: yorukot/superfile#969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: lazysegtree emphasizes proper dependency direction in software architecture, preferring that low-level components (like modal handlers) should not depend on high-level components (like the main model object). He also prioritizes performance considerations, noting that creating objects on every keypress in hot code paths like key handling is inefficient and should be avoided.

Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.

Learnt from: lazysegtree
PR: yorukot/superfile#985
File: src/internal/model.go:0-0
Timestamp: 2025-08-11T01:49:30.040Z
Learning: lazysegtree prefers maintaining code correctness through proper design and invariants rather than adding defensive bounds checks at every slice access point, viewing such defensive programming as "duct taping" that can mask actual bugs instead of fixing them at their source.

Learnt from: lazysegtree
PR: yorukot/superfile#973
File: src/internal/ui/processbar/model_update.go:7-27
Timestamp: 2025-08-03T14:49:31.221Z
Learning: lazysegtree prefers to keep test-only code simple without adding production-level concerns like goroutine synchronization, cancellation contexts, or complex lifecycle management, even when such patterns might prevent potential issues, since the complexity isn't justified for test utilities.

Learnt from: lazysegtree
PR: yorukot/superfile#1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.

Learnt from: lazysegtree
PR: yorukot/superfile#950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.

Learnt from: lazysegtree
PR: yorukot/superfile#967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.

Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.

🧬 Code graph analysis (1)

src/internal/handle_file_operation_test.go (1)

src/internal/test_utils.go (2)

• DefaultTestTimeout (19-19)
• DefaultTestTick (18-18)

🔇 Additional comments (1)

src/internal/handle_file_operation_test.go (1)

155-161: Good call—retrying RemoveAll tackles Windows handle flakies.

The Eventually-based cleanup aligns with the PR goal and should deflake deletions after extract/compress.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

src/internal/handle_file_operation_test.go (3)

145-158: Eventually message will likely print a nil error.

rmExtractErr/rmZipErr are evaluated when require.Eventually is invoked, not at timeout, so the formatted %v often shows . Capture the last error and fail after the Eventually result instead.

Apply this diff:

 t.Cleanup(func() {
-  var rmExtractErr error
-  require.Eventually(t, func() bool {
-    rmExtractErr = os.RemoveAll(extractedDir)
-    return rmExtractErr == nil
-  }, DefaultTestTimeout, DefaultTestTick, "Failed to remove extracted directory %q: %v", extractedDir, rmExtractErr)
+  var rmExtractErr error
+  ok := assert.Eventually(t, func() bool {
+    rmExtractErr = os.RemoveAll(extractedDir)
+    return rmExtractErr == nil
+  }, DefaultTestTimeout, DefaultTestTick)
+  if !ok {
+    t.Fatalf("Failed to remove extracted directory %q: %v", extractedDir, rmExtractErr)
+  }
 
-  var rmZipErr error
-  require.Eventually(t, func() bool {
-    rmZipErr = os.RemoveAll(zipFile)
-    return rmZipErr == nil
-  }, DefaultTestTimeout, DefaultTestTick, "Failed to remove zip file %q: %v", zipFile, rmZipErr)
+  var rmZipErr error
+  ok = assert.Eventually(t, func() bool {
+    rmZipErr = os.RemoveAll(zipFile)
+    return rmZipErr == nil
+  }, DefaultTestTimeout, DefaultTestTick)
+  if !ok {
+    t.Fatalf("Failed to remove zip file %q: %v", zipFile, rmZipErr)
+  }
 })

---

145-158: Optional: small DRY helper for the two removals.

Keeps test code concise without adding complexity.

Apply this diff:

 t.Cleanup(func() {
-  var rmExtractErr error
-  ok := assert.Eventually(t, func() bool {
-    rmExtractErr = os.RemoveAll(extractedDir)
-    return rmExtractErr == nil
-  }, DefaultTestTimeout, DefaultTestTick)
-  if !ok {
-    t.Fatalf("Failed to remove extracted directory %q: %v", extractedDir, rmExtractErr)
-  }
-
-  var rmZipErr error
-  ok = assert.Eventually(t, func() bool {
-    rmZipErr = os.RemoveAll(zipFile)
-    return rmZipErr == nil
-  }, DefaultTestTimeout, DefaultTestTick)
-  if !ok {
-    t.Fatalf("Failed to remove zip file %q: %v", zipFile, rmZipErr)
-  }
+  cleanupWithRetry := func(path, label string) {
+    var lastErr error
+    ok := assert.Eventually(t, func() bool {
+      lastErr = os.RemoveAll(path)
+      return lastErr == nil
+    }, DefaultTestTimeout, DefaultTestTick)
+    if !ok {
+      t.Fatalf("Failed to remove %s %q: %v", label, path, lastErr)
+    }
+  }
+  cleanupWithRetry(extractedDir, "extracted directory")
+  cleanupWithRetry(zipFile, "zip file")
 })

---

145-158: Optional: register cleanup earlier.

Consider computing extractedDir before extraction and registering t.Cleanup immediately after zipFile/extractedDir are known, so it also runs if the test fails between SendKey and cleanup registration. Small robustness win, no behavioral change.

Would you like me to propose the minimal move to register t.Cleanup right after setting zipFile/extractedDir?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6bf99b3 and b7e6bb8.

📒 Files selected for processing (1)

• src/internal/handle_file_operation_test.go (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.

Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the tests were actually passing despite lazysegtree reporting crashes. The real issue was that production image preview crashes occurred during actual application usage due to duplicate ImagePreviewer instances (one in defaultModelConfig and one in preview.New()), while the test environment didn't stress the image preview system enough to expose the conflicts.

Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.

Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.

Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.

Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.

Learnt from: lazysegtree
PR: yorukot/superfile#969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: lazysegtree emphasizes proper dependency direction in software architecture, preferring that low-level components (like modal handlers) should not depend on high-level components (like the main model object). He also prioritizes performance considerations, noting that creating objects on every keypress in hot code paths like key handling is inefficient and should be avoided.

Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.

Learnt from: lazysegtree
PR: yorukot/superfile#985
File: src/internal/model.go:0-0
Timestamp: 2025-08-11T01:49:30.040Z
Learning: lazysegtree prefers maintaining code correctness through proper design and invariants rather than adding defensive bounds checks at every slice access point, viewing such defensive programming as "duct taping" that can mask actual bugs instead of fixing them at their source.

Learnt from: lazysegtree
PR: yorukot/superfile#973
File: src/internal/ui/processbar/model_update.go:7-27
Timestamp: 2025-08-03T14:49:31.221Z
Learning: lazysegtree prefers to keep test-only code simple without adding production-level concerns like goroutine synchronization, cancellation contexts, or complex lifecycle management, even when such patterns might prevent potential issues, since the complexity isn't justified for test utilities.

Learnt from: lazysegtree
PR: yorukot/superfile#1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.

Learnt from: lazysegtree
PR: yorukot/superfile#950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.

Learnt from: lazysegtree
PR: yorukot/superfile#967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.

Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.

🧬 Code graph analysis (1)

src/internal/handle_file_operation_test.go (1)

src/internal/test_utils.go (2)

• DefaultTestTimeout (19-19)
• DefaultTestTick (18-18)

⏰ 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: Build and Test (ubuntu-latest)

🔇 Additional comments (1)

src/internal/handle_file_operation_test.go (1)

145-158: Good call moving cleanup to t.Cleanup with retries.

This should deflake Windows handle-release timing and guarantees cleanup even on failures.