[COMMS-1050] Remove unused inline-created labels by akabiru · Pull Request #26187 · opf/openproject

akabiru · 2026-10-21T07:29:17Z

Ticket

https://community.openproject.org/wp/80855

Stacked on #25320.

What are you trying to accomplish?

Add a cleanup strategy for unused labels created inline; possibly as a result of typos. Run a cleanup job to delete labels that have never been assigned to a work package.

What approach did you choose and why?

The origin is a boolean on the label (created_inline), set only on the API's find-or-create path and cleared when an admin edits the label. Admin-created labels therefore survive while unused, which the epic asks for.

"Unused" means no labelings row and no label_journals row, since the snapshot table keeps no foreign key to labels and a deleted label would otherwise leave holes in history. A 15 minute grace period on updated_at covers the gap between creating the label through the API and saving the work package that uses it. A form left open longer than that and then saved gets a "label does not exist" error on that label; product accepts that.

Creating a label inline books a cleanup run 15 minutes later, and the job books its own next run while unused inline labels remain, so instances without inline activity stay quiet; the nightly entry stays as a safety net. Running it as a job rather than synchronously in the work package services covers every path that removes labelings (bulk edits, moves, deletions) without coupling those flows to label housekeeping. The job locks its candidates FOR UPDATE SKIP LOCKED before deleting: a work package save holds the labels it is about to use with a key share lock during validation, so a label being attached right now is skipped until the next run, and a save that starts after the job locked a label fails validation cleanly instead of losing its labeling to the cascade. A key share lock alone was not enough, because Postgres does not re-evaluate a blocked DELETE's conditions against a row that was merely locked.

AI involvement

Collaborative – AI generated a substantial part of the code; I reviewed and understand every line.

Merge checklist

Added/updated tests
Added/updated documentation in Lookbook (patterns, previews, etc)
Tested major browsers (Chrome, Firefox, Edge, ...)

github-actions · 2026-10-21T14:49:33Z

Warning

Flaky specs

rspec ./spec/features/work_packages/update_ancestors_spec.rb[1:3:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #26187, linked for reference only):

- `rspec ./spec/features/work_packages/update_ancestors_spec.rb[1:3:1]`

Treat this as a standalone task, unrelated to PR #26187. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #26187 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @akabiru to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @akabiru, and request a review from @akabiru.
On every commit, set @akabiru as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

brunopagno

I think I understand why we want a job to cleanup labels, but this affects the product substantially in my opinion. Admins will still be able to see "ghost" labels for a whole day before they get cleaned up. Maybe worth checkin with the team (if you haven't already)

The alternative would be to create labels "on save" instead of automagically inline while typing. Which would solve the "created_inline" flag. But I imagine this would create some extra frontend challenge. Is it worth the tradeoff?

brunopagno · 2026-10-23T07:22:42Z

+  scope :never_used, -> {
+    where("NOT EXISTS (SELECT 1 FROM labelings WHERE labelings.label_id = labels.id)")
+      .where("NOT EXISTS (SELECT 1 FROM label_journals WHERE label_journals.label_id = labels.id)")


~~Why do we need to check journals for the never_used?~~
Just re read the PR description.

I am thinking if this makes it more difficult to reuse labels on a non journalized model, but I guess that's not really an issue since the query will simply not find any journal entries.

I guess it's just the coupling with journals that bothers me.

I'll leave this comment in as something to give 10 seconds of thought and then forget 😅

The coupling with journals is so that a label does not show up as "(deleted)" on the AT- the rationale is if it was used at least once; it's a legit label. AFAIK from product, the whole idea is to cleanup typos that are created within the inline creation phase.

We could use some discussion on this 👍🏾

brunopagno · 2026-10-23T07:36:47Z

+  private
+
+  def candidate_ids
+    candidates.lock("FOR UPDATE SKIP LOCKED").pluck(:id)


github-actions · 2026-10-23T08:15:41Z

Warning

Flaky specs

rspec ./spec/features/work_packages/table/delete_work_packages_with_descendants_spec.rb[1:2:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #26187, linked for reference only):

- `rspec ./spec/features/work_packages/table/delete_work_packages_with_descendants_spec.rb[1:2:1]`

Treat this as a standalone task, unrelated to PR #26187. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #26187 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @akabiru to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @akabiru, and request a review from @akabiru.
On every commit, set @akabiru as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

akabiru · 2026-10-23T08:17:03Z

The alternative would be to create labels "on save" instead of automagically inline while typing. Which would solve the "created_inline" flag. But I imagine this would create some extra frontend challenge. Is it worth the tradeoff?

Good shout, I think it's worth a shot as it narrows the grace period significantly. The current approach would have a label have a lifetime for 24-48hrs. I'll take a stab at it, though I suspect this would be a new pattern on the frontend-side; but should be covered by the existing Lables::FindOrCreateService. 👍🏾

Admin-curated labels and labels created from the work package form need telling apart, since only the latter are removed again when unused. See COMMS-1050.

Only labels created through the API's find-or-create path are marked. Editing a label in the administration turns it into a curated one. See COMMS-1050.

Deletes labels created from the work package form that no work package carries and no journal ever snapshotted, so a label removed again inside the journal aggregation window vanishes while one that was part of a work package's history stays. The one day grace period covers the gap between creating the label through the API and saving the work package that uses it, which can be a form left open for a long time. See COMMS-1050.

Keeps the nightly cleanup from deleting a label between the existence check and the labeling insert of the same save. See COMMS-1050.

A work package save holds the labels it is about to use with a key share lock. Taking the rows for update and skipping locked ones means the job never deletes a label a save is attaching, and a save that starts after the job has locked a label fails its validation instead of the insert. See COMMS-1050.

Asserts the schedule itself and drops an example the model spec already covers.

The candidate scan reads usage from the SELECT snapshot; a work package save that commits during that scan leaves the label unlocked but used. Re-applying never_used in the DELETE sees that labeling.

Postgres cannot turn NOT IN (subselect) into an index-backed anti-join and label_journals grows with every work package journal version.

Keying the cleanup on created_at let a stale unused inline label be returned by find-or-create and deleted before the work package save.

A no-op re-submit of the admin form or an empty API PATCH is not curation and must not make an unused typo label immortal.

…a day Product accepts a short grace period, so an inline label creation books a cleanup run 15 minutes later and the job books its own next run while unused inline labels remain. The nightly cron entry stays as a safety net.

akabiru self-assigned this Oct 21, 2026

akabiru added this pull request to stack #26188 October 21, 2026 07:35

akabiru removed this pull request from stack #26188 October 21, 2026 08:19

akabiru force-pushed the implementation/comms-1050-remove-unused-inline-created-labels branch 3 times, most recently from 8df831e to ea8f043 Compare October 21, 2026 12:39

github-actions Bot added the ai: Collaborative 💻 AI generated a substantial part of the code; A human reviewed and understands every line. label Oct 21, 2026

akabiru force-pushed the implementation/comms-1050-remove-unused-inline-created-labels branch from ea8f043 to 57e89d8 Compare October 21, 2026 14:10

Base automatically changed from implementation/comms-1022-labels-journaling to dev October 21, 2026 15:05

akabiru marked this pull request as ready for review October 21, 2026 15:10

akabiru force-pushed the implementation/comms-1050-remove-unused-inline-created-labels branch from 57e89d8 to dcff78d Compare October 21, 2026 15:10

akabiru marked this pull request as draft October 21, 2026 15:16

akabiru force-pushed the implementation/comms-1050-remove-unused-inline-created-labels branch from dcff78d to 2a227b2 Compare October 23, 2026 06:47

akabiru marked this pull request as ready for review October 23, 2026 06:49

akabiru requested a review from a team October 23, 2026 06:49

akabiru added this to the 18.0.x milestone Oct 23, 2026

brunopagno reviewed Oct 23, 2026

View reviewed changes

akabiru mentioned this pull request Oct 23, 2026

[COMMS-1071] Create labels when the work package is saved #26270

Draft

akabiru added 9 commits October 23, 2026 15:55

Record whether a label was created inline

dd3803d

Admin-curated labels and labels created from the work package form need telling apart, since only the latter are removed again when unused. See COMMS-1050.

Flag labels created from the work package form as inline

425d457

Only labels created through the API's find-or-create path are marked. Editing a label in the administration turns it into a curated one. See COMMS-1050.

Build the label journal row through its factory in the label spec

5885f48

Hold label rows while a work package writes its labelings

044c882

Keeps the nightly cleanup from deleting a label between the existence check and the labeling insert of the same save. See COMMS-1050.

Tighten the label cleanup job spec

b83ec01

Asserts the schedule itself and drops an example the model spec already covers.

Re-check label usage under the cleanup lock before deleting

a704807

The candidate scan reads usage from the SELECT snapshot; a work package save that commits during that scan leaves the label unlocked but used. Re-applying never_used in the DELETE sees that labeling.

Anti-join unused labels with NOT EXISTS

1e4eeba

Postgres cannot turn NOT IN (subselect) into an index-backed anti-join and label_journals grows with every work package journal version.

akabiru added 4 commits October 23, 2026 15:56

Shorten the label lock comment and note the cleanup cron time

67e44f9

Restart the inline label grace period whenever it is handed out again

1d638bf

Keying the cleanup on created_at let a stale unused inline label be returned by find-or-create and deleted before the work package save.

Keep an inline label a cleanup candidate when an edit changes nothing

bc5d72a

A no-op re-submit of the admin form or an empty API PATCH is not curation and must not make an unused typo label immortal.

akabiru force-pushed the implementation/comms-1050-remove-unused-inline-created-labels branch from 73264e3 to 8972186 Compare October 23, 2026 12:56

Navigation Menu

Repository navigation

[COMMS-1050] Remove unused inline-created labels - #26187

[COMMS-1050] Remove unused inline-created labels#26187
akabiru wants to merge 13 commits into
devfrom
implementation/comms-1050-remove-unused-inline-created-labels

akabiru commented Oct 21, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 21, 2026

Uh oh!

brunopagno left a comment

Uh oh!

brunopagno Oct 23, 2026

Uh oh!

brunopagno Oct 23, 2026

Uh oh!

akabiru Oct 23, 2026

Uh oh!

brunopagno Oct 23, 2026

Uh oh!

github-actions Bot commented Oct 23, 2026

Uh oh!

akabiru commented Oct 23, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Conversation

akabiru commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

What approach did you choose and why?

AI involvement

Merge checklist

Uh oh!

github-actions Bot commented Oct 21, 2026

Uh oh!

brunopagno left a comment

Choose a reason for hiding this comment

Uh oh!

brunopagno Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

brunopagno Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

akabiru Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

brunopagno Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 23, 2026

Uh oh!

akabiru commented Oct 23, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

akabiru commented Oct 21, 2026 •

edited

Loading