Skip to content

Cleanup CfT and Chromium profiles#3123

Merged
VietND96 merged 3 commits intoSeleniumHQ:trunkfrom
cgoldberg:chromium-cleanup
Apr 23, 2026
Merged

Cleanup CfT and Chromium profiles#3123
VietND96 merged 3 commits intoSeleniumHQ:trunkfrom
cgoldberg:chromium-cleanup

Conversation

@cgoldberg
Copy link
Copy Markdown
Member

@cgoldberg cgoldberg commented Apr 23, 2026

Description

Fixes #3122

  • Fix NodeChrome/chrome-cleanup.sh so it also deletes Chrome for Testing temp files
  • Fix NodeChromium/chrome-cleanup.sh so it deletes Chromium temp files

Note: this uses a glob when finding temp files because sometimes the directories start with a dot (org.chromium.Chromium.*) and sometimes they don't (.org.chromium.Chromium.scoped_dir.*)

Motivation and Context

chrome-cleanup.sh scripts were not properly cleaning temp profiles

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Fix Chrome and Chromium temp file cleanup patterns

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Fix Chrome cleanup script regex pattern to match temp files
• Add Chromium temp file cleanup to Chrome node script
• Update Chromium cleanup script to target correct profiles
• Widen regex search patterns for better file matching
Diagram
flowchart LR
  A["NodeChrome/chrome-cleanup.sh"] -->|"Widen regex pattern"| B["Match Chrome temp files"]
  A -->|"Add new pattern"| C["Match Chromium temp files"]
  D["NodeChromium/chrome-cleanup.sh"] -->|"Fix pattern"| E["Match Chromium profiles"]
  D -->|"Update message"| F["Clarify Chromium cleanup"]
Loading

Grey Divider

File Changes

1. NodeChrome/chrome-cleanup.sh 🐞 Bug fix +2/-1

Expand Chrome cleanup to include Chromium profiles

• Changed regex pattern from .com.google.Chrome.* to *com.google.Chrome.* to match Chrome temp
 files
• Added new find command to delete Chromium temp files matching *org.chromium.Chromium.* pattern
• Both patterns target directories older than configured threshold in /tmp

NodeChrome/chrome-cleanup.sh


2. NodeChromium/chrome-cleanup.sh 🐞 Bug fix +2/-2

Fix Chromium cleanup to target correct profiles

• Changed regex pattern from .com.google.Chrome.* to *org.chromium.Chromium.* to match actual
 Chromium profiles
• Updated echo message from "Chrome files" to "Chromium files" for accuracy
• Ensures cleanup targets correct Chromium temporary directories

NodeChromium/chrome-cleanup.sh


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Apr 23, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)

Grey Divider


Action required

1. SE_BROWSER_LEFTOVERS_TEMPFILES_DAYS unquoted 📘 Rule violation ☼ Reliability
Description
The new find command passes SE_BROWSER_LEFTOVERS_TEMPFILES_DAYS to -mtime without
validation/quoting, which can break cleanup (or behave unexpectedly) if the variable is empty or
non-numeric. This reduces script robustness and can prevent the intended retention-based cleanup.
Code

NodeChrome/chrome-cleanup.sh[R14-15]

+  find /tmp -name "*com.google.Chrome.*" -type d -mtime +${SE_BROWSER_LEFTOVERS_TEMPFILES_DAYS} -exec rm -rf "{}" +
+  find /tmp -name "*org.chromium.Chromium.*" -type d -mtime +${SE_BROWSER_LEFTOVERS_TEMPFILES_DAYS} -exec rm -rf "{}" +
Evidence
PR Compliance ID 5 requires robust shell parsing/handling (quoting and correctness). The added lines
use -mtime +${SE_BROWSER_LEFTOVERS_TEMPFILES_DAYS} directly, which is brittle if unset/malformed.

NodeChrome/chrome-cleanup.sh[14-15]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`SE_BROWSER_LEFTOVERS_TEMPFILES_DAYS` is used unquoted/unvalidated in `find -mtime`, which is brittle if the env var is empty or non-numeric.

## Issue Context
The cleanup behavior depends on `-mtime` parsing correctly; a malformed value can cause the cleanup to fail and leave temp directories behind.

## Fix Focus Areas
- NodeChrome/chrome-cleanup.sh[14-15]

## Suggested approach
- Set a local variable with a safe default, e.g. `days="${SE_BROWSER_LEFTOVERS_TEMPFILES_DAYS:-1}"`.
- Validate it is numeric (e.g., `case "$days" in (""|*[!0-9]*) days=1;; esac`).
- Pass to find as `-mtime "+$days"` (quoted) to avoid word-splitting and ensure correct parsing.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

2. Duplicate /tmp traversal 🐞 Bug ➹ Performance
Description
NodeChrome/chrome-cleanup.sh now runs two separate find /tmp ... traversals each cleanup cycle,
which increases I/O cost in containers with large /tmp. The two name patterns can be combined into
a single find invocation with grouped -name predicates.
Code

NodeChrome/chrome-cleanup.sh[R14-15]

+  find /tmp -name "*com.google.Chrome.*" -type d -mtime +${SE_BROWSER_LEFTOVERS_TEMPFILES_DAYS} -exec rm -rf "{}" +
+  find /tmp -name "*org.chromium.Chromium.*" -type d -mtime +${SE_BROWSER_LEFTOVERS_TEMPFILES_DAYS} -exec rm -rf "{}" +
Evidence
The script runs in an infinite loop and executes two find /tmp ... commands back-to-back every
interval; each command traverses the /tmp tree independently.

NodeChrome/chrome-cleanup.sh[12-37]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
NodeChrome cleanup traverses `/tmp` twice per cycle (once for Chrome and once for Chromium/CfT), which is unnecessary overhead.

### Issue Context
This runs repeatedly (default every hour). In long-running containers `/tmp` can get large.

### Fix Focus Areas
- NodeChrome/chrome-cleanup.sh[12-16]

### Suggested fix
Use a single `find` traversal with grouped predicates, e.g.:
```bash
find /tmp -type d \
 \( -name "*com.google.Chrome.*" -o -name "*org.chromium.Chromium.*" \) \
 -mtime +"${SE_BROWSER_LEFTOVERS_TEMPFILES_DAYS}" \
 -exec rm -rf -- "{}" +
```
This can be done together with the quoting/validation fix from the other finding.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread NodeChrome/chrome-cleanup.sh
@VietND96 VietND96 merged commit d107dd9 into SeleniumHQ:trunk Apr 23, 2026
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Chrome cleanup is missing .org.chromium.Chromium.* directories

2 participants