Skip to content

feat(cron): enforce unique active cron job names per agent+user#712

Open
nguyennguyenit wants to merge 2 commits intomainfrom
feat/cron-unique-active-name
Open

feat(cron): enforce unique active cron job names per agent+user#712
nguyennguyenit wants to merge 2 commits intomainfrom
feat/cron-unique-active-name

Conversation

@nguyennguyenit
Copy link
Copy Markdown
Contributor

Summary

  • Add migration 000037: partial unique index idx_cron_jobs_unique_active_name on cron_jobs (scoped to tenant_id + agent_id + user_id + name where enabled = true). Deduplicates existing rows before adding constraint.
  • PG store (cron_crud.go, cron_update.go): detect 23505 unique constraint violation on insert (return existing job) and on update (return ErrCronJobDuplicateName).
  • SQLite store: same logic using string-match on error message.
  • CronTool: pre-check for duplicate name before attempting insert, returning clear feedback to the agent.
  • Bump RequiredSchemaVersion → 37.

Test Plan

  • go build ./... passes
  • go build -tags sqliteonly ./... passes
  • go vet ./... passes
  • Apply migration on existing DB — dedup runs without error, index created
  • Creating a cron job with duplicate name returns the existing job (no error, no duplicate row)
  • Renaming a cron job to an existing active name returns ErrCronJobDuplicateName

Add partial unique index on cron_jobs to prevent duplicate active jobs
with the same name for a given tenant+agent+user scope.

- Migration 000037: dedup existing rows, add idx_cron_jobs_unique_active_name
- PG/SQLite stores: handle unique constraint on insert (return existing job)
  and on update (return ErrCronJobDuplicateName)
- CronTool: pre-check for duplicate name before insert to give clear feedback
- Bump RequiredSchemaVersion to 37
@nguyennguyenit nguyennguyenit requested a review from viettranx April 5, 2026 16:44
@mrgoonie
Copy link
Copy Markdown
Contributor

mrgoonie commented Apr 5, 2026

💡 Suggestion: Add suffix instead of DELETE in migration

Anh @NguyenNLT ơi, theo góp ý từ anh @viettx, em nghĩ mình nên thêm suffix vào tên job thay vì DELETE duplicate jobs trong migration.

⚠️ Vấn đề với migration hiện tại:

DELETE FROM cron_jobs WHERE id IN (SELECT id FROM dupes WHERE rn > 1);
  • Mất data: Xóa các cron jobs duplicate có thể làm mất jobs quan trọng của user
  • Không thể rollback: Sau khi delete thì không khôi phục được

✅ Hướng đề xuất:

Option 1: Rename với suffix

UPDATE cron_jobs cj1
JOIN (
    SELECT id, name, ROW_NUMBER() OVER (
        PARTITION BY tenant_id, agent_id, user_id, name
        ORDER BY created_at ASC
    ) AS rn
    FROM cron_jobs WHERE enabled = true
) dupes ON cj1.id = dupes.id
SET cj1.name = CONCAT(cj1.name, '-dup-', SUBSTRING(cj1.id, 1, 8))
WHERE dupes.rn > 1;

Option 2: Bỏ migration DELETE, chỉ tạo index

  • Để logic application (cron.go) tự handle duplicate bằng cách thêm suffix khi insert
  • Migration chỉ tạo index, không xóa data

🎯 Lợi ích:

  • An toàn: Không mất data của user
  • Reversible: Có thể rollback nếu cần
  • User-friendly: User vẫn thấy jobs cũ, chỉ cần rename lại nếu muốn

Anh cân nhắc điều chỉnh PR theo hướng này nhé! 🙏

Rename newer duplicates with '-dup-{id[:8]}' suffix before applying
unique index. Prevents data loss and keeps the migration reversible.
Addresses review feedback from PR #712.
@nguyennguyenit
Copy link
Copy Markdown
Contributor Author

Đã cập nhật theo đề xuất. Migration 000037 giờ rename các job duplicate thay vì DELETE:

UPDATE cron_jobs
SET name = name || '-dup-' || substr(id::text, 1, 8)
WHERE id IN (SELECT id FROM dupes WHERE rn > 1);
  • Không mất data
  • Reversible: down migration drop index là đủ, các job được rename vẫn còn nguyên
  • Job cũ nhất (created_at nhỏ nhất) giữ nguyên tên, các job mới hơn bị thêm suffix -dup-{8 ký tự id}

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.

2 participants