@cryptotaxi247 / netdata / commits / 4f5d490b8

Fix label migration logic to handle key-value changes and duplicate removal (#22586)

* Fix label migration logic to handle key-value changes and duplicate removal - Ensure proper handling of updated values during `rrdlabels_migrate_to_these`. - Remove stale (key, old-value) entries for duplicate keys, even with `DONT_DELETE` set. - Refactor memory handling and pointer updates for consistency. - Add unit tests to validate migration edge cases and duplicate prevention logic. * Fix duplicate key handling in label migration logic - Add loops to ensure all stale same-key entries are removed. - Address edge cases caused by prior buggy paths introducing duplicates. * Add unit tests for `rrdlabels_copy` to validate value changes and duplicate removal - Test handling of same-key cleanup for value changes. - Ensure `DONT_DELETE` flag behavior retains updated values without duplicates. - Validate edge cases with single key-value updates. * Improve duplicate key cleanup logic for label migration - Refactor loops to ensure consistent stale (key, old-value) removal. - Handle pre-existing duplicates left by previous buggy paths. - Enhance comments to clarify key-value update behavior in both insertion and presence scenarios. * Enhance duplicate key cleanup and state mutation tracking in label migration - Track and return cleanup count (`cleaned`) in migration logic. - Ensure state mutations (e.g., version bumps, stats updates) occur during duplicate removal. - Improve comments to clarify behavior and implications of key cleanup. * Add tests for multi-duplicate cleanup in label migration and copy functions * Refine tests for cleanup-only paths in label migration and copy functions * Add tests to ensure `DONT_DELETE` flag preserves unrelated keys in migration and copy operations * Refactor and enhance unit tests for duplicate key handling in label migration and copy operations - Introduce helper functions to simplify value comparisons and duplicate planting. - Replace inline test logic with `rrdlabels_unittest_value_is` for clarity. - Add `rrdlabels_unittest_plant_dups` to streamline duplicate entry creation for tests. - Improve readability and maintainability.

Stelios Fragkakis committed May 29, 2026 at 23:06 UTC 4f5d490b83aebe1b122b2005c92d80516f952479

2 files changed +238 -16

src/database/rrdhost-labels.c

@@ -5,6 +5,9 @@

5 #include "streaming/stream.h"

7 void rrdhost_set_is_parent_label(void) {

8 + if (!localhost || !localhost->rrdlabels)

9 + return;

10 +

11 uint32_t count = stream_receivers_currently_connected();

13 if (count == 0 || count == 1) {

src/database/rrdlabels.c

+235 -16

@@ -711,28 +711,53 @@ bool rrdlabels_migrate_to_these(RRDLABELS *dst, RRDLABELS *src) {

711 RRDLABEL *label;

712 Pvoid_t *PValue;

713 size_t added = 0;

714 + size_t cleaned = 0;

715

716 RRDLABEL_SRC ls;

717 lfe_start_nolock(src, label, ls)

718 {

719 JudyAllocThreadPulseGetAndReset();

720

721 + // labels->JudyL is keyed by the deduplicated RRDLABEL pointer produced by

722 + // add_label_name_value(), which encodes BOTH key and value. A same-key

723 + // value change in src therefore yields a different pointer than the one

724 + // dst already has, so JudyLIns lands on an empty slot.

725 PValue = JudyLIns(&dst->JudyL, (Word_t)label, PJE0);

726 if(unlikely(!PValue || PValue == PJERR))

727 fatal("RRDLABELS migrate: corrupted labels array");

728

724 - RRDLABEL_SRC flag;

729 if (!*PValue) {

726 - flag = (ls & ~(RRDLABEL_FLAG_OLD | RRDLABEL_FLAG_NEW)) | RRDLABEL_FLAG_NEW;

730 + // Write through PValue BEFORE any subsequent JudyLDel: Judy

731 + // invalidates previously-returned PValue pointers when the array

732 + // is modified. Same ordering as labels_add_already_sanitized().

733 + *((RRDLABEL_SRC *)PValue) = (ls & ~(RRDLABEL_FLAG_OLD | RRDLABEL_FLAG_NEW)) | RRDLABEL_FLAG_NEW;

734 dup_label(label);

735 int64_t judy_mem = JudyAllocThreadPulseGetAndReset();

736 RRDLABELS_MEMORY_DELTA(&dictionary_stats_category_rrdlabels, judy_mem, 0);

737 added++;

738 }

739 else

733 - flag = RRDLABEL_FLAG_OLD;

740 + *((RRDLABEL_SRC *)PValue) |= RRDLABEL_FLAG_OLD;

741

735 - *((RRDLABEL_SRC *)PValue) |= flag;

742 + // Ensure at most one entry per key. The remove-unmarked sweep below

743 + // preserves RRDLABEL_FLAG_DONT_DELETE, so a stale (key, *) entry

744 + // would otherwise survive next to the desired (key, value) entry.

745 + // Runs in BOTH branches because the stale entry may pre-date this

746 + // iteration (e.g. a duplicate left by a prior buggy path) and is

747 + // independent of whether the current src label is a fresh insert

748 + // or an already-present (key, value). The find helper skips the

749 + // just-inserted/just-found entry via same_value=false.

750 + for (;;) {

751 + RRDLABEL *old_label_with_same_key = rrdlabels_find_label_with_key_unsafe(dst, label, false);

752 + if (!old_label_with_same_key)

753 + break;

754 + int del_result = JudyLDel(&dst->JudyL, (Word_t)old_label_with_same_key, PJE0);

755 + (void)del_result;

756 + int64_t old_judy_mem = JudyAllocThreadPulseGetAndReset();

757 + RRDLABELS_MEMORY_DELTA(&dictionary_stats_category_rrdlabels, old_judy_mem, 0);

758 + delete_label(old_label_with_same_key);

759 + cleaned++;

760 + }

761 }

762 lfe_done_nolock();

763

@@ -742,7 +767,12 @@ bool rrdlabels_migrate_to_these(RRDLABELS *dst, RRDLABELS *src) {

767 spinlock_unlock(&src->spinlock);

768 spinlock_unlock(&dst->spinlock);

769

745 - return (added > 0) || (removed > 0);

770 + // cleaned counts duplicates dropped by the same-key cleanup loop above.

771 + // Without it, a stale (key,*) duplicate removed while the desired

772 + // (key,value) was already present would mutate dst silently -- callers

773 + // gating on this return (e.g. rrdset_update_rrdlabels setting

774 + // RRDSET_FLAG_PENDING_LABEL_RECHECK) would miss the change.

775 + return (added > 0) || (removed > 0) || (cleaned > 0);

776 }

777

778 //

@@ -794,27 +824,42 @@ void rrdlabels_copy(RRDLABELS *dst, RRDLABELS *src)

824 bool update_statistics = false;

825 lfe_start_nolock(src, label, ls)

826 {

797 - RRDLABEL *old_label_with_key = rrdlabels_find_label_with_key_unsafe(dst, label, false);

827 Pvoid_t *PValue = JudyLIns(&dst->JudyL, (Word_t)label, PJE0);

828 if(unlikely(!PValue || PValue == PJERR))

829 fatal("RRDLABELS: corrupted labels array");

830

831 if (!*PValue) {

832 + // Write through PValue BEFORE any subsequent JudyLDel: Judy

833 + // invalidates previously-returned PValue pointers when the array

834 + // is modified. Same ordering as labels_add_already_sanitized().

835 + *((RRDLABEL_SRC *)PValue) = (ls & ~(RRDLABEL_FLAG_OLD)) | RRDLABEL_FLAG_NEW;

836 dup_label(label);

804 - ls = (ls & ~(RRDLABEL_FLAG_OLD)) | RRDLABEL_FLAG_NEW;

837 dst->version++;

838 update_statistics = true;

807 - if (old_label_with_key) {

808 - int64_t judy_mem = JudyAllocThreadPulseGetAndReset();

809 - (void)JudyLDel(&dst->JudyL, (Word_t)old_label_with_key, PJE0);

810 - RRDLABELS_MEMORY_DELTA(&dictionary_stats_category_rrdlabels, judy_mem, 0);

811 - delete_label((RRDLABEL *)old_label_with_key);

812 - }

839 }

840 else

815 - ls = (ls & ~(RRDLABEL_FLAG_NEW)) | RRDLABEL_FLAG_OLD;

816 -

817 - *((RRDLABEL_SRC *)PValue) = ls;

841 + *((RRDLABEL_SRC *)PValue) = (ls & ~(RRDLABEL_FLAG_NEW)) | RRDLABEL_FLAG_OLD;

842 +

843 + // Drop any other entry sharing this key. Runs in BOTH branches so

844 + // pre-existing same-key duplicates (e.g. left behind by a prior

845 + // buggy path) get cleaned up even when the current src label is

846 + // already present in dst. Loop because the find helper returns

847 + // the first match only. The find skips the just-inserted /

848 + // just-updated entry via same_value=false.

849 + for (;;) {

850 + RRDLABEL *old_label_with_key = rrdlabels_find_label_with_key_unsafe(dst, label, false);

851 + if (!old_label_with_key)

852 + break;

853 + (void)JudyLDel(&dst->JudyL, (Word_t)old_label_with_key, PJE0);

854 + int64_t judy_mem = JudyAllocThreadPulseGetAndReset();

855 + RRDLABELS_MEMORY_DELTA(&dictionary_stats_category_rrdlabels, judy_mem, 0);

856 + delete_label((RRDLABEL *)old_label_with_key);

857 + // Cleanup is itself a state mutation: bump version and request

858 + // tail-stats accounting so version-based consumers and the

859 + // memory pulse stay correct even when no new insert happened.

860 + dst->version++;

861 + update_statistics = true;

862 + }

863 }

864 lfe_done_nolock();

865 if (update_statistics) {

@@ -1587,6 +1632,27 @@ static int rrdlabels_unittest_mark_source_as_old(void) {

1632 } \

1633 } while (0)

1634

1635 +// Returns true when labels[key] resolves to exactly `expected`.

1636 +static bool rrdlabels_unittest_value_is(RRDLABELS *labels, const char *key, const char *expected) {

1637 + char *v = NULL;

1638 + rrdlabels_get_value_strdup_or_null(labels, &v, key);

1639 + bool ok = (v != NULL && strcmp(v, expected) == 0);

1640 + freez(v);

1641 + return ok;

1642 +}

1643 +

1644 +// Plant `n` pinned DONT_DELETE entries that share `key` but carry distinct

1645 +// `values`, writing them straight into labels->JudyL. This bypasses the

1646 +// public add/migrate/copy paths so a multi-duplicate starting state -- which

1647 +// the fixed code can no longer produce -- can be constructed for tests.

1648 +static void rrdlabels_unittest_plant_dups(RRDLABELS *labels, const char *key, const char *const *values, size_t n) {

1649 + for (size_t i = 0; i < n; i++) {

1650 + RRDLABEL *stale = add_label_name_value(key, values[i]);

1651 + Pvoid_t *p = JudyLIns(&labels->JudyL, (Word_t)stale, PJE0);

1652 + *((RRDLABEL_SRC *)p) = RRDLABEL_SRC_CONFIG | RRDLABEL_FLAG_DONT_DELETE;

1653 + }

1654 +}

1655 +

1656 static int rrdlabels_unittest_change_detection(void) {

1657 fprintf(stderr, "\n%s() tests\n", __FUNCTION__);

1658 int errors = 0;

@@ -1650,6 +1716,159 @@ static int rrdlabels_unittest_change_detection(void) {

1716 rrdlabels_destroy(dst);

1717 rrdlabels_destroy(src);

1718

1719 + // Value change via migrate (no DONT_DELETE): dst ends with a single entry

1720 + // for the key, carrying the new value.

1721 + dst = rrdlabels_create();

1722 + src = rrdlabels_create();

1723 + rrdlabels_add(dst, "k1", "v1", RRDLABEL_SRC_CONFIG);

1724 + rrdlabels_add(src, "k1", "v2", RRDLABEL_SRC_CONFIG);

1725 + UT_EXPECT(rrdlabels_migrate_to_these(dst, src) == true,

1726 + "migrate with a value change should return true");

1727 + UT_EXPECT(rrdlabels_entries(dst) == 1,

1728 + "migrate with a value change should leave one entry per key");

1729 + UT_EXPECT(rrdlabels_unittest_value_is(dst, "k1", "v2"),

1730 + "migrate with a value change should leave the new value in dst");

1731 + rrdlabels_destroy(dst);

1732 + rrdlabels_destroy(src);

1733 +

1734 + // Value change via migrate where dst pinned the old value with DONT_DELETE:

1735 + // the stale (key, old-value) must not survive, even though DONT_DELETE

1736 + // would otherwise protect it from the remove-unmarked sweep.

1737 + dst = rrdlabels_create();

1738 + src = rrdlabels_create();

1739 + rrdlabels_add(dst, "k1", "v1", RRDLABEL_SRC_CONFIG | RRDLABEL_FLAG_DONT_DELETE);

1740 + rrdlabels_add(src, "k1", "v2", RRDLABEL_SRC_CONFIG);

1741 + UT_EXPECT(rrdlabels_migrate_to_these(dst, src) == true,

1742 + "migrate with a value change for a DONT_DELETE key should return true");

1743 + UT_EXPECT(rrdlabels_entries(dst) == 1,

1744 + "migrate must not leave a duplicate (key,old-value) when DONT_DELETE was set");

1745 + UT_EXPECT(rrdlabels_unittest_value_is(dst, "k1", "v2"),

1746 + "migrate with a DONT_DELETE value change should leave the new value in dst");

1747 + rrdlabels_destroy(dst);

1748 + rrdlabels_destroy(src);

1749 +

1750 + // ---- rrdlabels_copy: same-key cleanup path ----

1751 + // Value change via copy: dst ends with a single entry for the key,

1752 + // carrying the new value from src. Exercises the same-key cleanup loop

1753 + // that mirrors the migrate path.

1754 + dst = rrdlabels_create();

1755 + src = rrdlabels_create();

1756 + rrdlabels_add(dst, "k1", "v1", RRDLABEL_SRC_CONFIG);

1757 + rrdlabels_add(src, "k1", "v2", RRDLABEL_SRC_CONFIG);

1758 + rrdlabels_copy(dst, src);

1759 + UT_EXPECT(rrdlabels_entries(dst) == 1,

1760 + "copy with a value change should leave one entry per key");

1761 + UT_EXPECT(rrdlabels_unittest_value_is(dst, "k1", "v2"),

1762 + "copy with a value change should leave the new value in dst");

1763 + rrdlabels_destroy(dst);

1764 + rrdlabels_destroy(src);

1765 +

1766 + // Value change via copy where dst pinned the old value with DONT_DELETE.

1767 + dst = rrdlabels_create();

1768 + src = rrdlabels_create();

1769 + rrdlabels_add(dst, "k1", "v1", RRDLABEL_SRC_CONFIG | RRDLABEL_FLAG_DONT_DELETE);

1770 + rrdlabels_add(src, "k1", "v2", RRDLABEL_SRC_CONFIG);

1771 + rrdlabels_copy(dst, src);

1772 + UT_EXPECT(rrdlabels_entries(dst) == 1,

1773 + "copy must not leave a duplicate (key,old-value) when DONT_DELETE was set");

1774 + UT_EXPECT(rrdlabels_unittest_value_is(dst, "k1", "v2"),

1775 + "copy with a DONT_DELETE value change should leave the new value in dst");

1776 + rrdlabels_destroy(dst);

1777 + rrdlabels_destroy(src);

1778 +

1779 + // ---- Multi-duplicate drain ----

1780 + // The fixed code path can no longer produce a (key, *) duplicate state.

1781 + // To verify the same-key cleanup LOOP drains arbitrarily many duplicates

1782 + // (not just the first), plant multiple stale entries directly into

1783 + // dst->JudyL via add_label_name_value() + JudyLIns(). This bypasses the

1784 + // public add/migrate/copy paths so the pathological starting state can

1785 + // be constructed for the test.

1786 +

1787 + // Migrate (cleanup-only path): dst seeded with 3 pinned DONT_DELETE

1788 + // entries sharing "k1"; src carries (k1, vc) whose dedup pointer is

1789 + // ALREADY one of the planted entries, so JudyLIns lands on an existing

1790 + // slot, takes the else-branch (added stays 0), and the cleanup loop is

1791 + // the only mutation. Asserts that:

1792 + // - the loop drains the OTHER two stale entries (not just the first);

1793 + // - the function returns true even though added==0 and removed==0

1794 + // (regression on the `cleaned` term in the return would fail here);

1795 + // - dst ends with the single source entry.

1796 + static const char *const planted_dups[] = { "va", "vb", "vc" };

1797 + dst = rrdlabels_create();

1798 + src = rrdlabels_create();

1799 + rrdlabels_unittest_plant_dups(dst, "k1", planted_dups, 3);

1800 + UT_EXPECT(rrdlabels_entries(dst) == 3,

1801 + "test setup: dst should start with 3 manually-planted same-key entries");

1802 + rrdlabels_add(src, "k1", "vc", RRDLABEL_SRC_CONFIG);

1803 + UT_EXPECT(rrdlabels_migrate_to_these(dst, src) == true,

1804 + "migrate must return true when cleanup is the only mutation (added=0, removed=0, cleaned>0)");

1805 + UT_EXPECT(rrdlabels_entries(dst) == 1,

1806 + "migrate must drain all stale same-key duplicates, not just the first");

1807 + UT_EXPECT(rrdlabels_unittest_value_is(dst, "k1", "vc"),

1808 + "migrate multi-duplicate drain should leave only the src value in dst");

1809 + rrdlabels_destroy(dst);

1810 + rrdlabels_destroy(src);

1811 +

1812 + // Copy (cleanup-only path): same setup; src's (k1, vc) matches one of

1813 + // the planted entries so the copy takes the else-branch and the cleanup

1814 + // is the ONLY mutation. Asserts dst entries+value AND that

1815 + // dst->version advances despite no insert -- regression on the

1816 + // version bump inside the cleanup loop would fail this check.

1817 + dst = rrdlabels_create();

1818 + src = rrdlabels_create();

1819 + rrdlabels_unittest_plant_dups(dst, "k1", planted_dups, 3);

1820 + UT_EXPECT(rrdlabels_entries(dst) == 3,

1821 + "test setup: dst should start with 3 manually-planted same-key entries (copy)");

1822 + rrdlabels_add(src, "k1", "vc", RRDLABEL_SRC_CONFIG);

1823 + uint32_t copy_version_before = rrdlabels_version(dst);

1824 + rrdlabels_copy(dst, src);

1825 + UT_EXPECT(rrdlabels_entries(dst) == 1,

1826 + "copy must drain all stale same-key duplicates, not just the first");

1827 + UT_EXPECT(rrdlabels_version(dst) > copy_version_before,

1828 + "copy must advance dst->version when cleanup is the only mutation");

1829 + UT_EXPECT(rrdlabels_unittest_value_is(dst, "k1", "vc"),

1830 + "copy multi-duplicate drain should leave only the src value in dst");

1831 + rrdlabels_destroy(dst);

1832 + rrdlabels_destroy(src);

1833 +

1834 + // ---- Cleanup respects key boundaries ----

1835 + // The cleanup loop calls find_label_with_key_unsafe() which filters by

1836 + // STRING key pointer. Regressing that key check would make the loop

1837 + // delete entries with DIFFERENT keys. Plant pinned DONT_DELETE entries

1838 + // under TWO keys; mutate only the first key's value; the second key's

1839 + // entry must survive untouched.

1840 + dst = rrdlabels_create();

1841 + src = rrdlabels_create();

1842 + rrdlabels_add(dst, "k1", "v1", RRDLABEL_SRC_CONFIG | RRDLABEL_FLAG_DONT_DELETE);

1843 + rrdlabels_add(dst, "k2", "v2", RRDLABEL_SRC_CONFIG | RRDLABEL_FLAG_DONT_DELETE);

1844 + rrdlabels_add(src, "k1", "v3", RRDLABEL_SRC_CONFIG);

1845 + UT_EXPECT(rrdlabels_migrate_to_these(dst, src) == true,

1846 + "migrate with mixed-key DONT_DELETE dst must return true on k1 change");

1847 + UT_EXPECT(rrdlabels_entries(dst) == 2,

1848 + "migrate cleanup must leave the unrelated DONT_DELETE key intact");

1849 + UT_EXPECT(rrdlabels_unittest_value_is(dst, "k1", "v3"),

1850 + "migrate must update k1 to the src value");

1851 + UT_EXPECT(rrdlabels_unittest_value_is(dst, "k2", "v2"),

1852 + "migrate must preserve the unrelated DONT_DELETE key's value");

1853 + rrdlabels_destroy(dst);

1854 + rrdlabels_destroy(src);

1855 +

1856 + // Same scenario for rrdlabels_copy().

1857 + dst = rrdlabels_create();

1858 + src = rrdlabels_create();

1859 + rrdlabels_add(dst, "k1", "v1", RRDLABEL_SRC_CONFIG | RRDLABEL_FLAG_DONT_DELETE);

1860 + rrdlabels_add(dst, "k2", "v2", RRDLABEL_SRC_CONFIG | RRDLABEL_FLAG_DONT_DELETE);

1861 + rrdlabels_add(src, "k1", "v3", RRDLABEL_SRC_CONFIG);

1862 + rrdlabels_copy(dst, src);

1863 + UT_EXPECT(rrdlabels_entries(dst) == 2,

1864 + "copy cleanup must leave the unrelated DONT_DELETE key intact");

1865 + UT_EXPECT(rrdlabels_unittest_value_is(dst, "k1", "v3"),

1866 + "copy must update k1 to the src value");

1867 + UT_EXPECT(rrdlabels_unittest_value_is(dst, "k2", "v2"),

1868 + "copy must preserve the unrelated DONT_DELETE key's value");

1869 + rrdlabels_destroy(dst);

1870 + rrdlabels_destroy(src);

1871 +

1872 // ---- rrdlabels_remove_all_unmarked_and_changed: CLABEL-commit semantics ----

1873 // (1) unmark + re-add identical set => no change => false

1874 l = rrdlabels_create();