fix(channels): make compact numbering repack idempotent

The compact repack read its channels with no ORDER BY, so the pack followed PostgreSQL's physical row order. That order drifts after the
UPDATEs each repack issues, so successive syncs
packed the same channels into different numbers within the configured range. Auto-synced channel numbers reshuffled on every sync even when the provider had not changed.

- Add .order_by("id") to the _repack_inner channel query so the pack is deterministic. id order is creation order, which tracks the provider stream order used by the default "provider" sort.
- Add c.id as a secondary key to the name / tvg_id / updated_at sorts so equal values (e.g. blank tvg_id) break ties deterministically instead of churning.
- Add a deterministic regression test that forces a physical heap reorder (CLUSTER) and asserts two consecutive repacks produce identical channel numbers.
This commit is contained in:
None 2026-06-05 00:01:22 -05:00
parent bdf59b8c19
commit cae040d45d
2 changed files with 118 additions and 7 deletions

View file

@ -324,12 +324,17 @@ def _repack_inner(group_relation):
# into the SAME target group, their channels are indistinguishable
# here (channels carry no source-group back-reference), so each repack
# renumbers the shared target's channels into its own range.
# order_by("id") makes the pack deterministic. Without it the query
# returns rows in unspecified physical order, which shifts after the
# renumber's own UPDATEs and autovacuum, so the default "provider" sort
# below would repack channels into different numbers on every sync.
# id order is creation order, which tracks the provider stream order.
channels = list(
Channel.objects.filter(
auto_created=True,
auto_created_by_id=account_id,
channel_group_id__in=group_ids,
).select_related("override")
).select_related("override").order_by("id")
)
visible = []
@ -344,21 +349,22 @@ def _repack_inner(group_relation):
visible.append(ch)
# Sort the visible set by the group's configured channel_sort_order.
# Provider order (the default) preserves DB-iteration order which is
# roughly creation order; treat unrecognized values the same way.
# Provider order (the default) keeps the id order from the query above.
# Each explicit sort carries c.id as a secondary key so equal values
# (e.g. blank tvg_id) break ties deterministically instead of churning.
if sort_order == "name":
visible.sort(
key=lambda c: natural_sort_key(c.name or ""),
key=lambda c: (natural_sort_key(c.name or ""), c.id),
reverse=sort_reverse,
)
elif sort_order == "tvg_id":
visible.sort(
key=lambda c: c.tvg_id or "",
key=lambda c: (c.tvg_id or "", c.id),
reverse=sort_reverse,
)
elif sort_order == "updated_at":
visible.sort(
key=lambda c: c.updated_at,
key=lambda c: (c.updated_at, c.id),
reverse=sort_reverse,
)

View file

@ -6,7 +6,10 @@ first (fails on HEAD prior to the Tier 2 patch), then is flipped to assert
the correct post-fix behavior. Comments call out the failure mode and the
fix location.
"""
from django.test import TestCase
from unittest import skipUnless
from django.db import connection
from django.test import TestCase, TransactionTestCase
from django.utils import timezone
from apps.channels.models import (
@ -2195,3 +2198,105 @@ class CompactNumberingWithGroupOverrideTests(TestCase):
large,
f"repack query count scaled with channel count: {small} -> {large}",
)
@skipUnless(
connection.vendor == "postgresql",
"Idempotency repro forces a physical heap reorder via CLUSTER, which is "
"PostgreSQL-specific (the suite's target DB).",
)
class CompactNumberingIdempotencyTests(TransactionTestCase):
"""
A compact repack must be idempotent: with no change to hide state or
overrides, repacking again must leave every channel on the same number.
The unpatched _repack_inner read its channels with no ORDER BY, so the
pack followed PostgreSQL's physical row order. That order drifts after
the UPDATEs each repack issues (and after autovacuum), so successive
syncs packed the same channels into different numbers. That is the daily
channel-number churn users reported.
This test forces the divergence deterministically. After the first pack
it rewrites every channel_number to the reverse of id order, then
physically clusters the table on that column so the heap order becomes
the reverse of id order. An unordered SELECT then returns the rows in the
opposite order from the first pass. Unpatched, the second pack assigns
numbers in that reversed order and the channel->number mapping flips;
patched, .order_by("id") keeps both packs identical.
Fail signature: channel->number mapping differs between the two repacks
= _repack_inner is following physical row order instead of id order.
Fix location: apps/channels/compact_numbering.py (_repack_inner channel
query .order_by("id")).
"""
# TransactionTestCase commits its rows (TestCase's savepoint rollback
# would hide them from CLUSTER, which also cannot run inside the
# transaction block TestCase wraps each test in).
def _mapping(self, account):
return {
c.id: c.channel_number
for c in Channel.objects.filter(
auto_created=True, auto_created_by=account
)
}
def test_repack_is_idempotent_under_physical_reorder(self):
from apps.channels.compact_numbering import repack_group
account = _make_account()
group = _make_group(name="Sports")
rel = _attach_group_to_account(
account, group, custom_properties={"compact_numbering": True}
)
rel.auto_sync_channel_start = 8000
rel.auto_sync_channel_end = 8099
rel.save()
# Eight visible auto channels; ascending id is creation order.
channels = [
Channel.objects.create(
name=f"C{i}",
channel_group=group,
auto_created=True,
auto_created_by=account,
)
for i in range(8)
]
repack_group(rel)
first = self._mapping(account)
# Provider-order pack (the default) assigns by id, so the lowest id
# takes the range start.
lowest_id = min(c.id for c in channels)
self.assertEqual(first[lowest_id], 8000)
# Set channel_number to the reverse of id order, then cluster the
# heap on that column so physical order becomes reverse-id order.
# Values sit above the range so they cannot collide with the pack.
table = Channel._meta.db_table
with connection.cursor() as cur:
for pos, ch in enumerate(channels):
cur.execute(
f"UPDATE {table} SET channel_number = %s WHERE id = %s",
[9000 - pos, ch.id],
)
cur.execute(
f"CREATE INDEX IF NOT EXISTS churn_cn_idx "
f"ON {table} (channel_number)"
)
cur.execute(f"CLUSTER {table} USING churn_cn_idx")
cur.execute("DROP INDEX IF EXISTS churn_cn_idx")
repack_group(rel)
second = self._mapping(account)
self.assertEqual(
first,
second,
"Repack is not idempotent: channel numbers changed on a second "
"pass with no hide or override change. _repack_inner is following "
"physical row order instead of id order.",
)