fix: preserve active cronjob records during cleanup - #13474
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
The cleanup flow can leave DB records pointing to already-deleted log paths if the DB deletion fails, and should delete the DB row before removing the log file to avoid inconsistent state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Fixes scheduled-task record cleanup so that records for currently executing cronjobs are preserved (addressing #13471), preventing loss of in-progress task visibility/control during a “clear records” operation.
Changes:
- Update cronjob record cleanup to skip deleting
Waiting/Runningrecords and delete completed records individually. - Update multiple i18n strings (log filter label and Enterprise→Community restore confirmation copy) across several locales.
File summaries
| File | Description |
|---|---|
| frontend/src/lang/modules/zh.ts | Updates log filter label and restore confirmation wording (Simplified Chinese). |
| frontend/src/lang/modules/zh-Hant.ts | Updates log filter label and restore confirmation wording (Traditional Chinese). |
| frontend/src/lang/modules/tr.ts | Updates log filter label and restore confirmation wording (Turkish). |
| frontend/src/lang/modules/ru.ts | Updates log filter label and restore confirmation wording (Russian). |
| frontend/src/lang/modules/pt-br.ts | Updates log filter label and restore confirmation wording (Portuguese - Brazil). |
| frontend/src/lang/modules/ms.ts | Updates log filter label and restore confirmation wording (Malay). |
| frontend/src/lang/modules/lo.ts | Updates log filter label and restore confirmation wording (Lao). |
| frontend/src/lang/modules/ko.ts | Updates log filter label and restore confirmation wording (Korean). |
| frontend/src/lang/modules/ja.ts | Updates log filter label and restore confirmation wording (Japanese). |
| frontend/src/lang/modules/fa.ts | Updates log filter label and restore confirmation wording (Persian). |
| frontend/src/lang/modules/es-es.ts | Updates log filter label and restore confirmation wording (Spanish - Spain). |
| frontend/src/lang/modules/en.ts | Updates log filter label and restore confirmation wording (English). |
| agent/app/service/cronjob.go | Preserves active cronjob record rows during cleanup by skipping Waiting/Running statuses. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| previousPage: 'Previous page', | ||
| nextPage: 'Next page', | ||
| filter: 'Filter', | ||
| filter: 'Message filter', |
| _ = os.RemoveAll(del.Records) | ||
| } | ||
| if err := cronjobRepo.DeleteRecord(cronjobRepo.WithByJobID(int(req.CronjobID))); err != nil { | ||
| return err | ||
| if err := cronjobRepo.DeleteRecord(repo.WithByID(del.ID)); err != nil { | ||
| return err | ||
| } |
Co-authored-by: ssongliu <73214554+ssongliu@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The bugfix is mixed with broad, unrelated i18n text updates, making it harder to review/backport as a focused fix.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
frontend/src/lang/modules/en.ts:2257
- This PR is titled/scoped as a cronjob-record cleanup fix, but it also changes multiple unrelated UI translation strings (e.g. log filter label and community restore confirmation). Splitting the i18n updates into a separate PR would keep the bugfix reviewable and easier to backport.
hostSystem: 'Host logs',
previousPage: 'Previous page',
nextPage: 'Next page',
filter: 'Message filter',
agent/app/service/cronjob.go:548
CleanRecordnow issues oneDeleteRecordDB call per record, which can become slow for cronjobs with many history rows. Consider collecting the IDs of non-active records, deleting them in a singleDELETE ... WHERE id IN (...), then best-effort removing the corresponding record files.
for _, del := range delRecords {
if del.Status == constant.StatusWaiting || del.Status == constant.StatusRunning {
continue
}
if err := cronjobRepo.DeleteRecord(repo.WithByID(del.ID)); err != nil {
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Refs #13471