From 3a8348d133b56d43c76cb7d362e38724b1292b79 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 14 Aug 2026 13:04:51 +0800 Subject: [PATCH 1/9] fix(registry): cron-repair stored membership status/is_ended stale against the clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit status and is_ended on spp.group.membership are store=True computes that depend only on ended_date and compare it against now(), so a recompute fires on a write to ended_date but never when the clock crosses it. A departure recorded ahead of time (future-dated ended_date) stayed stored as active/is_ended=False indefinitely once the date passed — rosters, metrics, API search and downstream authorization gates kept treating the member as current. Add an hourly cron that searches (archived rows included) for rows whose stored values disagree with the clock and re-triggers both computes via modified(). Its first run self-heals rows already stale in existing databases, so no migration script is needed. Fixes #417 --- spp_registry/__manifest__.py | 3 +- spp_registry/data/ir_cron.xml | 16 +++ spp_registry/models/group_membership.py | 40 +++++++ spp_registry/readme/HISTORY.md | 4 + spp_registry/tests/__init__.py | 1 + .../tests/test_membership_status_cron.py | 106 ++++++++++++++++++ 6 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 spp_registry/data/ir_cron.xml create mode 100644 spp_registry/tests/test_membership_status_cron.py diff --git a/spp_registry/__manifest__.py b/spp_registry/__manifest__.py index 81e898bf8..c8e43552f 100644 --- a/spp_registry/__manifest__.py +++ b/spp_registry/__manifest__.py @@ -3,7 +3,7 @@ { "name": "OpenSPP Registry", "category": "OpenSPP/Core", - "version": "19.0.2.2.2", + "version": "19.0.2.2.3", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", @@ -33,6 +33,7 @@ # "data/id_types.xml", "data/vocabularies.xml", "data/res_users.xml", + "data/ir_cron.xml", # Security "security/privileges.xml", "security/groups.xml", diff --git a/spp_registry/data/ir_cron.xml b/spp_registry/data/ir_cron.xml new file mode 100644 index 000000000..1c85da980 --- /dev/null +++ b/spp_registry/data/ir_cron.xml @@ -0,0 +1,16 @@ + + + + + Registry: Recompute Ended Group Memberships + + code + model.cron_recompute_ended_status() + 1 + hours + True + + diff --git a/spp_registry/models/group_membership.py b/spp_registry/models/group_membership.py index 467170fd6..d6821536e 100644 --- a/spp_registry/models/group_membership.py +++ b/spp_registry/models/group_membership.py @@ -211,6 +211,46 @@ def _compute_status(self): else: record.status = "active" + @api.model + def cron_recompute_ended_status(self): + """Repair stored ``status``/``is_ended`` on rows the clock has crossed. + + Both computes depend only on ``ended_date`` and compare it against + *now*, so a recompute fires on a write to ``ended_date`` but never + when time passes it: a future-dated departure would stay stored as + active forever. This cron finds rows whose stored values disagree + with the clock and re-triggers the computes through the normal ORM + path. Archived rows are included — the UI onchange archives + memberships ended in the past, and those must be repaired too. + """ + now = fields.Datetime.now() + memberships = self.with_context(active_test=False) + to_end = memberships.search( + [ + ("ended_date", "<=", now), + "|", + ("is_ended", "=", False), + ("status", "!=", "inactive"), + ] + ) + to_reactivate = memberships.search( + [ + "|", + ("ended_date", "=", False), + ("ended_date", ">", now), + "|", + ("is_ended", "=", True), + ("status", "!=", "active"), + ] + ) + stale = to_end | to_reactivate + if stale: + stale.modified(["ended_date"]) + _logger.info( + "[spp.registry] Recomputed ended status for %d group membership(s)", + len(stale), + ) + @api.constrains("ended_date") def _check_ended_date(self): for record in self: diff --git a/spp_registry/readme/HISTORY.md b/spp_registry/readme/HISTORY.md index aed786664..e2cec4a34 100644 --- a/spp_registry/readme/HISTORY.md +++ b/spp_registry/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.2.2.3 + +- fix(registry): add an hourly cron that repairs the stored `status`/`is_ended` computes on `spp.group.membership`. Both fields depend only on `ended_date` compared against *now*, so a future-dated departure never took effect once the clock crossed it — rosters, metrics, API search and downstream gates kept treating the member as active indefinitely. The cron finds rows whose stored values disagree with the clock (archived rows included) and re-triggers the computes; its first run self-heals any rows already stale in existing databases (#417) + ### 19.0.2.2.2 - fix(registry): let an ID type be used again after its ID was removed. Removing an ID through a change request keeps the row and marks it Invalid, and the old uniqueness rule counted those dead rows — so the registrant was left with an Invalid ID and no way to add a valid one of the same type. Uniqueness now applies to live IDs only, and is refused before the write so the message names the ID type rather than surfacing a database error (#1136) diff --git a/spp_registry/tests/__init__.py b/spp_registry/tests/__init__.py index 2177f1fd9..a5fc2ccb5 100644 --- a/spp_registry/tests/__init__.py +++ b/spp_registry/tests/__init__.py @@ -10,6 +10,7 @@ from . import test_phone_number from . import test_reg_id from . import test_membership_constraints +from . import test_membership_status_cron from . import test_registrant_misc from . import test_group_aggregation from . import test_res_config_settings diff --git a/spp_registry/tests/test_membership_status_cron.py b/spp_registry/tests/test_membership_status_cron.py new file mode 100644 index 000000000..43cda326f --- /dev/null +++ b/spp_registry/tests/test_membership_status_cron.py @@ -0,0 +1,106 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Cron repair of the stored ``status``/``is_ended`` computes (issue #417). + +Both fields depend only on ``ended_date`` and compare it against *now*, so a +recompute fires on a write to ``ended_date`` but never when the clock crosses +it: a future-dated departure stays ``active``/``is_ended = False`` forever. +The stale state cannot be produced through the ORM (writing ``ended_date`` +recomputes at write time), so these tests age rows behind the ORM's back with +raw SQL — exactly how production rows drift. +""" + +from datetime import timedelta + +from odoo import fields +from odoo.tests import tagged + +from .test_membership_constraints import MembershipCommon + + +@tagged("post_install", "-at_install") +class TestMembershipEndedStatusCron(MembershipCommon): + """``cron_recompute_ended_status`` — repair rows the clock has crossed.""" + + def _make_membership(self, individual, **vals): + vals.update({"group": self.group.id, "individual": individual.id}) + return self.Membership.create(vals) + + def _age_row(self, rec, start_date, ended_date, active=True): + """Rewrite the date window (and ``active``) behind the ORM's back so + the stored computes keep their now-wrong values.""" + self.env.flush_all() + self.env.cr.execute( + "UPDATE spp_group_membership SET start_date = %s, ended_date = %s, active = %s WHERE id = %s", + (start_date, ended_date, active, rec.id), + ) + rec.invalidate_recordset() + + def test_cron_ends_membership_the_clock_has_crossed(self): + rec = self._make_membership(self.individual_a) + now = fields.Datetime.now() + self._age_row(rec, now - timedelta(days=730), now - timedelta(days=365)) + + # Stale precondition: departed a year ago, still stored as active. + self.assertEqual(rec.status, "active") + self.assertFalse(rec.is_ended) + + self.Membership.cron_recompute_ended_status() + + self.assertEqual(rec.status, "inactive") + self.assertTrue(rec.is_ended) + + def test_cron_reactivates_membership_whose_end_moved_to_future(self): + past = fields.Datetime.now() - timedelta(days=365) + rec = self._make_membership( + self.individual_a, + start_date=past - timedelta(days=1), + ended_date=past, + ) + self.assertEqual(rec.status, "inactive") + self.assertTrue(rec.is_ended) + + # The end date is pushed to the future behind the ORM's back; the + # stored "inactive" is now wrong in the other direction. + future = fields.Datetime.now() + timedelta(days=365) + self._age_row(rec, past - timedelta(days=1), future) + self.assertEqual(rec.status, "inactive") + self.assertTrue(rec.is_ended) + + self.Membership.cron_recompute_ended_status() + + self.assertEqual(rec.status, "active") + self.assertFalse(rec.is_ended) + + def test_cron_repairs_archived_rows(self): + rec = self._make_membership(self.individual_a) + now = fields.Datetime.now() + self._age_row(rec, now - timedelta(days=730), now - timedelta(days=365), active=False) + self.assertEqual(rec.status, "active") + self.assertFalse(rec.is_ended) + + self.Membership.cron_recompute_ended_status() + + self.assertEqual(rec.status, "inactive") + self.assertTrue(rec.is_ended) + + def test_cron_leaves_correct_rows_untouched(self): + open_ended = self._make_membership(self.individual_a) + past = fields.Datetime.now() - timedelta(days=365) + already_ended = self._make_membership( + self.individual_b, + start_date=past - timedelta(days=1), + ended_date=past, + ) + + self.Membership.cron_recompute_ended_status() + + self.assertEqual(open_ended.status, "active") + self.assertFalse(open_ended.is_ended) + self.assertEqual(already_ended.status, "inactive") + self.assertTrue(already_ended.is_ended) + + def test_cron_record_registered(self): + cron = self.env.ref("spp_registry.cron_recompute_membership_ended_status") + self.assertEqual(cron.model_id.model, "spp.group.membership") + self.assertTrue(cron.active) + self.assertIn("cron_recompute_ended_status", cron.code) From f80ccf36ce093a137af1a88f398485813d6de0dc Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 14 Aug 2026 15:44:59 +0800 Subject: [PATCH 2/9] fix(registry): address expert-review findings on the ended-status cron - invalidate group metrics for repaired memberships: the recompute flushes through low-level SQL and bypasses the write() override, so the metric-invalidation funnel must be called explicitly - rename the cron entry point to _cron_recompute_ended_status so it is not RPC-callable, matching the repo's cron naming pattern - bound each run to batch_size (default 10000) rows per direction so a large first-run backlog cannot exceed the cron time limit; repaired rows drop out of the domains, so subsequent runs drain the remainder - index ended_date, which both sweep domains filter on - return the repaired recordset and strengthen the tests: raw-SQL column assertions, over-match guard on the no-op case, metric-funnel invalidation, batch-size behavior, archived rows keep active=False, cron interval asserted --- spp_registry/data/ir_cron.xml | 2 +- spp_registry/models/group_membership.py | 29 +++++-- .../tests/test_membership_status_cron.py | 75 +++++++++++++++++-- 3 files changed, 94 insertions(+), 12 deletions(-) diff --git a/spp_registry/data/ir_cron.xml b/spp_registry/data/ir_cron.xml index 1c85da980..8b878a268 100644 --- a/spp_registry/data/ir_cron.xml +++ b/spp_registry/data/ir_cron.xml @@ -8,7 +8,7 @@ Registry: Recompute Ended Group Memberships code - model.cron_recompute_ended_status() + model._cron_recompute_ended_status() 1 hours True diff --git a/spp_registry/models/group_membership.py b/spp_registry/models/group_membership.py index d6821536e..11dbe9c7a 100644 --- a/spp_registry/models/group_membership.py +++ b/spp_registry/models/group_membership.py @@ -51,7 +51,7 @@ def _compute_has_group_membership_type_codes(self): rec.has_group_membership_type_codes = has_codes start_date = fields.Datetime(default=lambda self: fields.Datetime.now()) - ended_date = fields.Datetime() + ended_date = fields.Datetime(index=True) status = fields.Selection( [("inactive", "Inactive"), ("active", " ")], compute="_compute_status", @@ -212,7 +212,7 @@ def _compute_status(self): record.status = "active" @api.model - def cron_recompute_ended_status(self): + def _cron_recompute_ended_status(self, batch_size=10000): """Repair stored ``status``/``is_ended`` on rows the clock has crossed. Both computes depend only on ``ended_date`` and compare it against @@ -222,6 +222,16 @@ def cron_recompute_ended_status(self): with the clock and re-triggers the computes through the normal ORM path. Archived rows are included — the UI onchange archives memberships ended in the past, and those must be repaired too. + ``active`` itself is deliberately left untouched: archiving changes + record visibility everywhere and is a separate decision from the + stored computes (see issue #417). + + At most ``batch_size`` rows per direction are repaired per run so a + large backlog (e.g. the first run on a database that predates this + cron) cannot exceed the cron time limit; the repaired rows drop out + of the domains, so subsequent runs drain the remainder. + + Returns the repaired memberships. """ now = fields.Datetime.now() memberships = self.with_context(active_test=False) @@ -231,7 +241,8 @@ def cron_recompute_ended_status(self): "|", ("is_ended", "=", False), ("status", "!=", "inactive"), - ] + ], + limit=batch_size, ) to_reactivate = memberships.search( [ @@ -241,15 +252,23 @@ def cron_recompute_ended_status(self): "|", ("is_ended", "=", True), ("status", "!=", "active"), - ] + ], + limit=batch_size, ) stale = to_end | to_reactivate if stale: stale.modified(["ended_date"]) + # The recompute flushes through low-level SQL and bypasses this + # model's write() override, so the metric-invalidation hook must + # be called explicitly. + self._invalidate_group_metrics(stale.mapped("group")) _logger.info( - "[spp.registry] Recomputed ended status for %d group membership(s)", + "[spp.registry] Scheduled ended-status recompute for %d group membership(s)", len(stale), ) + if len(to_end) == batch_size or len(to_reactivate) == batch_size: + _logger.info("[spp.registry] Ended-status backlog remains; the next cron run will continue") + return stale @api.constrains("ended_date") def _check_ended_date(self): diff --git a/spp_registry/tests/test_membership_status_cron.py b/spp_registry/tests/test_membership_status_cron.py index 43cda326f..e88020c09 100644 --- a/spp_registry/tests/test_membership_status_cron.py +++ b/spp_registry/tests/test_membership_status_cron.py @@ -10,6 +10,7 @@ """ from datetime import timedelta +from unittest.mock import patch from odoo import fields from odoo.tests import tagged @@ -19,7 +20,7 @@ @tagged("post_install", "-at_install") class TestMembershipEndedStatusCron(MembershipCommon): - """``cron_recompute_ended_status`` — repair rows the clock has crossed.""" + """``_cron_recompute_ended_status`` — repair rows the clock has crossed.""" def _make_membership(self, individual, **vals): vals.update({"group": self.group.id, "individual": individual.id}) @@ -35,19 +36,36 @@ def _age_row(self, rec, start_date, ended_date, active=True): ) rec.invalidate_recordset() + def _read_stored_columns(self, rec): + """Read ``status``/``is_ended`` straight from the SQL columns, the way + the raw-SQL consumers of ``is_ended`` do — bypassing the ORM cache, + which would recompute pending fields on read and mask a missing + column write.""" + self.env.flush_all() + self.env.cr.execute( + "SELECT status, is_ended FROM spp_group_membership WHERE id = %s", + (rec.id,), + ) + return self.env.cr.fetchone() + def test_cron_ends_membership_the_clock_has_crossed(self): rec = self._make_membership(self.individual_a) now = fields.Datetime.now() self._age_row(rec, now - timedelta(days=730), now - timedelta(days=365)) # Stale precondition: departed a year ago, still stored as active. + self.assertEqual(self._read_stored_columns(rec), ("active", False)) self.assertEqual(rec.status, "active") self.assertFalse(rec.is_ended) - self.Membership.cron_recompute_ended_status() + repaired = self.Membership._cron_recompute_ended_status() + self.assertEqual(repaired, rec) self.assertEqual(rec.status, "inactive") self.assertTrue(rec.is_ended) + # The SQL columns must be repaired too — four consumers read + # is_ended in raw SQL and never see the ORM cache. + self.assertEqual(self._read_stored_columns(rec), ("inactive", True)) def test_cron_reactivates_membership_whose_end_moved_to_future(self): past = fields.Datetime.now() - timedelta(days=365) @@ -66,10 +84,12 @@ def test_cron_reactivates_membership_whose_end_moved_to_future(self): self.assertEqual(rec.status, "inactive") self.assertTrue(rec.is_ended) - self.Membership.cron_recompute_ended_status() + repaired = self.Membership._cron_recompute_ended_status() + self.assertEqual(repaired, rec) self.assertEqual(rec.status, "active") self.assertFalse(rec.is_ended) + self.assertEqual(self._read_stored_columns(rec), ("active", False)) def test_cron_repairs_archived_rows(self): rec = self._make_membership(self.individual_a) @@ -78,10 +98,12 @@ def test_cron_repairs_archived_rows(self): self.assertEqual(rec.status, "active") self.assertFalse(rec.is_ended) - self.Membership.cron_recompute_ended_status() + self.Membership._cron_recompute_ended_status() self.assertEqual(rec.status, "inactive") self.assertTrue(rec.is_ended) + # The cron repairs the computes only; archiving stays as it was. + self.assertFalse(rec.active) def test_cron_leaves_correct_rows_untouched(self): open_ended = self._make_membership(self.individual_a) @@ -92,15 +114,56 @@ def test_cron_leaves_correct_rows_untouched(self): ended_date=past, ) - self.Membership.cron_recompute_ended_status() + repaired = self.Membership._cron_recompute_ended_status() + # An over-matching domain would sweep these rows in; they must not + # be selected at all, not merely end up with unchanged values. + self.assertFalse(repaired) self.assertEqual(open_ended.status, "active") self.assertFalse(open_ended.is_ended) self.assertEqual(already_ended.status, "inactive") self.assertTrue(already_ended.is_ended) + def test_cron_invalidates_group_metrics(self): + rec = self._make_membership(self.individual_a) + now = fields.Datetime.now() + self._age_row(rec, now - timedelta(days=730), now - timedelta(days=365)) + + # The recompute flushes through low-level SQL and bypasses write(), + # so the cron must call the metric-invalidation funnel itself. + with patch.object( + type(self.env["res.partner"]), + "invalidate_group_metrics", + autospec=True, + ) as funnel: + self.Membership._cron_recompute_ended_status() + + funnel.assert_called_once() + self.assertEqual(funnel.call_args.args[0], self.group) + + def test_cron_respects_batch_size(self): + carol = self.Partner.create({"name": "Carol", "is_registrant": True, "is_group": False}) + now = fields.Datetime.now() + rows = self.Membership.browse() + for individual in (self.individual_a, self.individual_b, carol): + rec = self._make_membership(individual) + self._age_row(rec, now - timedelta(days=730), now - timedelta(days=365)) + rows |= rec + + first = self.Membership._cron_recompute_ended_status(batch_size=2) + self.assertEqual(len(first), 2) + + second = self.Membership._cron_recompute_ended_status(batch_size=2) + self.assertEqual(len(second), 1) + self.assertEqual(first | second, rows) + for rec in rows: + self.assertEqual(rec.status, "inactive") + self.assertTrue(rec.is_ended) + def test_cron_record_registered(self): cron = self.env.ref("spp_registry.cron_recompute_membership_ended_status") self.assertEqual(cron.model_id.model, "spp.group.membership") self.assertTrue(cron.active) - self.assertIn("cron_recompute_ended_status", cron.code) + self.assertIn("_cron_recompute_ended_status", cron.code) + self.assertEqual(cron.interval_number, 1) + self.assertEqual(cron.interval_type, "hours") From aba3aa63c788e97b86ba8b422e7e436017da408b Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Tue, 1 Sep 2026 11:20:24 +0800 Subject: [PATCH 3/9] =?UTF-8?q?fix(registry):=20address=20PR=20#418=20revi?= =?UTF-8?q?ew=20=E2=80=94=20exact-time=20triggers,=20chunked=20commits,=20?= =?UTF-8?q?NULL=20repair?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - schedule the repair cron via ir.cron.trigger at the exact future ended_date being written; the periodic sweep drops to a daily safety net - drain backlogs in batch_size chunks committed via ir.cron._commit_progress (fault isolation, ASAP resume on time-out, no more == batch_size false log) - repair is_ended = NULL rows in SQL (the ORM cannot write NULL -> False) - split sweep domains into conjunctive per-leg searches; ended_date index becomes btree_not_null; batch default follows the 5,000 principle cap - single _is_ended_as_of home for the ended-at-T predicate - pin cron user_id to base.user_root; daily cadence; dedup doc paragraph - tests: scoped assertions, shared funnel patch, _age_row defaults, new NULL-repair/trigger/chunking coverage --- spp_registry/data/ir_cron.xml | 14 +- spp_registry/models/group_membership.py | 201 +++++++++++++----- spp_registry/readme/HISTORY.md | 2 +- .../tests/test_membership_status_cron.py | 160 ++++++++++---- 4 files changed, 277 insertions(+), 100 deletions(-) diff --git a/spp_registry/data/ir_cron.xml b/spp_registry/data/ir_cron.xml index 8b878a268..72607fc50 100644 --- a/spp_registry/data/ir_cron.xml +++ b/spp_registry/data/ir_cron.xml @@ -1,16 +1,20 @@ - + Registry: Recompute Ended Group Memberships code model._cron_recompute_ended_status() 1 - hours + days + True diff --git a/spp_registry/models/group_membership.py b/spp_registry/models/group_membership.py index 11dbe9c7a..795fd04e6 100644 --- a/spp_registry/models/group_membership.py +++ b/spp_registry/models/group_membership.py @@ -51,7 +51,9 @@ def _compute_has_group_membership_type_codes(self): rec.has_group_membership_type_codes = has_codes start_date = fields.Datetime(default=lambda self: fields.Datetime.now()) - ended_date = fields.Datetime(index=True) + # btree_not_null: a plain btree would index every NULL ended_date (the + # open-membership majority) for no query benefit, amplifying writes. + ended_date = fields.Datetime(index="btree_not_null") status = fields.Selection( [("inactive", "Inactive"), ("active", " ")], compute="_compute_status", @@ -122,14 +124,22 @@ def _name_search(self, name, domain=None, operator="ilike", limit=100, order=Non domain = [("group", operator, name)] + domain return self._search(domain, limit=limit, order=order) + @api.model + def _is_ended_as_of(self, ended_date, now): + """Single home of the "membership is ended at time T" predicate. + + Used by both stored computes, the archiving onchange and the + repair-cron domains; keep external consumers (raw-SQL readers of + ``is_ended`` in spp_registry/models/group.py, spp_api_v2_gis, + spp_cel_domain, spp_gis_report) in mind when changing it (#421). + """ + return bool(ended_date and ended_date <= now) + @api.depends("ended_date") def _compute_is_ended(self): + now = fields.Datetime.now() for rec in self: - is_ended = False - if rec.ended_date and rec.ended_date <= fields.Datetime.now(): - is_ended = True - - rec.is_ended = is_ended + rec.is_ended = self._is_ended_as_of(rec.ended_date, now) def _invalidate_group_metrics(self, groups): """Schedule metric invalidation for affected groups. @@ -159,6 +169,7 @@ def write(self, vals): affected_groups |= self.mapped("group") self._invalidate_group_metrics(affected_groups) + self._schedule_ended_status_repair([vals]) return res @api.model_create_multi @@ -167,6 +178,7 @@ def create(self, vals_list): # Invalidate metrics for all affected groups groups = res.mapped("group") self._invalidate_group_metrics(groups) + self._schedule_ended_status_repair(vals_list) return res def unlink(self): @@ -204,71 +216,147 @@ def open_group_form(self): @api.depends("ended_date") def _compute_status(self): + now = fields.Datetime.now() for record in self: - # check if memebership end date available and less than current date - if record.ended_date and record.ended_date <= fields.Datetime.now(): - record.status = "inactive" - else: - record.status = "active" + # check if membership end date available and less than current date + record.status = "inactive" if self._is_ended_as_of(record.ended_date, now) else "active" + + def _schedule_ended_status_repair(self, vals_list): + """Point the repair cron at every future ``ended_date`` being written. + + The stored computes go stale the moment the clock crosses + ``ended_date`` (see ``_cron_recompute_ended_status``); a persistent + ``ir.cron.trigger`` at exactly that time shrinks the staleness + window from the sweep cadence to about a minute. A stale or + duplicate trigger is harmless — it just runs the idempotent sweep. + """ + now = fields.Datetime.now() + at_list = { + ended + for vals in vals_list + if (ended := fields.Datetime.to_datetime(vals.get("ended_date"))) and ended > now + } + if at_list: + self.env.ref("spp_registry.cron_recompute_membership_ended_status")._trigger(at=at_list) @api.model - def _cron_recompute_ended_status(self, batch_size=10000): + def _stale_ended_status_domains(self, now): + """Domains selecting rows whose stored ``status``/``is_ended`` + disagree with the clock, one conjunctive leg per (date-window, + stale-column) pair so each search stays servable by the + ``ended_date`` index instead of forcing a full-table read. + """ + return [ + # Ended by the clock, still stored as active. + [("ended_date", "<=", now), ("is_ended", "=", False)], + [("ended_date", "<=", now), ("status", "!=", "inactive")], + # Not (or no longer) ended, still stored as ended. + [("ended_date", "=", False), ("is_ended", "=", True)], + [("ended_date", "=", False), ("status", "!=", "active")], + [("ended_date", ">", now), ("is_ended", "=", True)], + [("ended_date", ">", now), ("status", "!=", "active")], + ] + + @api.model + def _repair_null_is_ended(self, now): + """Repair rows holding ``is_ended = NULL`` that should read active. + + Rows written behind the ORM can leave ``is_ended`` NULL, and every + raw-SQL consumer of the column treats NULL as ended. The ORM + cannot repair the open-membership case — NULL reads back from the + cache as False, so a recompute writes nothing — hence this one + SQL-level leg mirroring ``_is_ended_as_of``. (NULL rows whose + ``ended_date`` has passed need no special casing: the computed + True differs from the cached False, so the ORM legs repair them.) + """ + self.flush_model(["is_ended", "ended_date"]) + self.env.cr.execute( + "UPDATE spp_group_membership SET is_ended = false " + "WHERE is_ended IS NULL AND (ended_date IS NULL OR ended_date > %s) " + "RETURNING id", + (now,), + ) + ids = [row[0] for row in self.env.cr.fetchall()] + if not ids: + return self.browse() + repaired = self.browse(ids) + repaired.invalidate_recordset() + self._invalidate_group_metrics(repaired.mapped("group")) + return repaired + + @api.model + def _cron_recompute_ended_status(self, batch_size=5000): """Repair stored ``status``/``is_ended`` on rows the clock has crossed. Both computes depend only on ``ended_date`` and compare it against *now*, so a recompute fires on a write to ``ended_date`` but never when time passes it: a future-dated departure would stay stored as - active forever. This cron finds rows whose stored values disagree - with the clock and re-triggers the computes through the normal ORM - path. Archived rows are included — the UI onchange archives - memberships ended in the past, and those must be repaired too. - ``active`` itself is deliberately left untouched: archiving changes - record visibility everywhere and is a separate decision from the - stored computes (see issue #417). - - At most ``batch_size`` rows per direction are repaired per run so a - large backlog (e.g. the first run on a database that predates this - cron) cannot exceed the cron time limit; the repaired rows drop out - of the domains, so subsequent runs drain the remainder. + active forever. Writes of a future ``ended_date`` schedule a cron + trigger at exactly that moment (``_schedule_ended_status_repair``), + so this periodic sweep is the safety net — it self-heals rows + already stale in pre-existing databases and rows written behind + the ORM. It finds rows whose stored values disagree with the clock + and re-triggers the computes through the normal ORM path. Archived + rows are included — the UI onchange archives memberships ended in + the past, and those must be repaired too. ``active`` itself is + deliberately left untouched: archiving changes record visibility + everywhere and is a separate decision from the stored computes + (see issue #417). + + Two write-path side effects intentionally differ from a real + write: the recompute flush still stamps ``write_uid``/``write_date`` + (repaired rows show the cron user as last modified — e.g. + ``changed_by`` in the API's membership history reads ``write_uid``), + and the repair is invisible to ``spp_audit`` write-rules, which + hook ``write()``. Both are accepted: the repair only restores what + a timely recompute would have stored. + + Rows are repaired in ``batch_size`` chunks, each committed via + ``ir.cron._commit_progress``: a large backlog drains within one + run, a run that exhausts the cron time budget is resumed ASAP + instead of waiting a full sweep interval, and a serialization + failure rolls back only its own chunk. The chunk size follows the + 5,000-record cap in docs/principles/performance-scalability.md. Returns the repaired memberships. """ - now = fields.Datetime.now() + if batch_size < 1: + raise ValueError("batch_size must be a positive number of rows") memberships = self.with_context(active_test=False) - to_end = memberships.search( - [ - ("ended_date", "<=", now), - "|", - ("is_ended", "=", False), - ("status", "!=", "inactive"), - ], - limit=batch_size, - ) - to_reactivate = memberships.search( - [ - "|", - ("ended_date", "=", False), - ("ended_date", ">", now), - "|", - ("is_ended", "=", True), - ("status", "!=", "active"), - ], - limit=batch_size, - ) - stale = to_end | to_reactivate - if stale: - stale.modified(["ended_date"]) + repaired = memberships._repair_null_is_ended(fields.Datetime.now()) + while True: + # Re-read the clock every pass: a row whose ended_date is + # crossed while the run is in flight recomputes to the very + # values a stale `now` would keep selecting it for. + now = fields.Datetime.now() + chunk = memberships.browse() + for leg in self._stale_ended_status_domains(now): + chunk |= memberships.search(leg, limit=batch_size) + chunk = chunk[:batch_size] + if not chunk: + break + chunk.modified(["ended_date"]) # The recompute flushes through low-level SQL and bypasses this # model's write() override, so the metric-invalidation hook must # be called explicitly. - self._invalidate_group_metrics(stale.mapped("group")) + self._invalidate_group_metrics(chunk.mapped("group")) + repaired |= chunk + self.env.flush_all() + # A short chunk means every leg came back exhausted, so the + # backlog is drained (repaired rows drop out of the domains). + drained = len(chunk) < batch_size + time_left = self.env["ir.cron"]._commit_progress(len(chunk), remaining=0 if drained else batch_size) + if drained: + break + if not time_left: + _logger.info("[spp.registry] Ended-status backlog remains; the cron will be re-triggered to continue") + break + if repaired: _logger.info( - "[spp.registry] Scheduled ended-status recompute for %d group membership(s)", - len(stale), + "[spp.registry] Repaired ended-status on %d group membership(s)", + len(repaired), ) - if len(to_end) == batch_size or len(to_reactivate) == batch_size: - _logger.info("[spp.registry] Ended-status backlog remains; the next cron run will continue") - return stale + return repaired @api.constrains("ended_date") def _check_ended_date(self): @@ -278,8 +366,7 @@ def _check_ended_date(self): @api.onchange("ended_date") def _onchange_ended_date(self): + now = fields.Datetime.now() for record in self: - record.active = True # if ended date is less than current date, set active to false - if record.ended_date and record.ended_date <= fields.Datetime.now(): - record.active = False + record.active = not self._is_ended_as_of(record.ended_date, now) diff --git a/spp_registry/readme/HISTORY.md b/spp_registry/readme/HISTORY.md index e2cec4a34..e5fca57c6 100644 --- a/spp_registry/readme/HISTORY.md +++ b/spp_registry/readme/HISTORY.md @@ -1,6 +1,6 @@ ### 19.0.2.2.3 -- fix(registry): add an hourly cron that repairs the stored `status`/`is_ended` computes on `spp.group.membership`. Both fields depend only on `ended_date` compared against *now*, so a future-dated departure never took effect once the clock crossed it — rosters, metrics, API search and downstream gates kept treating the member as active indefinitely. The cron finds rows whose stored values disagree with the clock (archived rows included) and re-triggers the computes; its first run self-heals any rows already stale in existing databases (#417) +- fix(registry): repair the stored `status`/`is_ended` computes on `spp.group.membership` once the clock crosses `ended_date`. Both fields depend only on `ended_date` compared against *now*, so a future-dated departure never took effect once the clock crossed it — rosters, metrics, API search and downstream gates kept treating the member as active indefinitely. Writing a future `ended_date` now schedules the repair cron at exactly that moment (staleness window ~1 minute), and a daily sweep self-heals everything else: rows already stale in existing databases (drained in committed batches on the first run, however large the backlog) and rows written behind the ORM, including `is_ended = NULL` rows that raw-SQL consumers treated as ended (#417) ### 19.0.2.2.2 diff --git a/spp_registry/tests/test_membership_status_cron.py b/spp_registry/tests/test_membership_status_cron.py index e88020c09..63784d82a 100644 --- a/spp_registry/tests/test_membership_status_cron.py +++ b/spp_registry/tests/test_membership_status_cron.py @@ -1,9 +1,7 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. -"""Cron repair of the stored ``status``/``is_ended`` computes (issue #417). +"""Repair of stored ``status``/``is_ended`` stale against the clock — see +``_cron_recompute_ended_status`` (#417) for the full story. -Both fields depend only on ``ended_date`` and compare it against *now*, so a -recompute fires on a write to ``ended_date`` but never when the clock crosses -it: a future-dated departure stays ``active``/``is_ended = False`` forever. The stale state cannot be produced through the ORM (writing ``ended_date`` recomputes at write time), so these tests age rows behind the ORM's back with raw SQL — exactly how production rows drift. @@ -16,6 +14,7 @@ from odoo.tests import tagged from .test_membership_constraints import MembershipCommon +from .test_metric_invalidation import _patch_invalidate_funnel @tagged("post_install", "-at_install") @@ -26,9 +25,14 @@ def _make_membership(self, individual, **vals): vals.update({"group": self.group.id, "individual": individual.id}) return self.Membership.create(vals) - def _age_row(self, rec, start_date, ended_date, active=True): + def _age_row(self, rec, ended_date=None, active=True): """Rewrite the date window (and ``active``) behind the ORM's back so - the stored computes keep their now-wrong values.""" + the stored computes keep their now-wrong values. Defaults to a + departure one year ago; ``start_date`` is derived so the row stays + consistent with the start/end constraint.""" + if ended_date is None: + ended_date = fields.Datetime.now() - timedelta(days=365) + start_date = ended_date - timedelta(days=365) self.env.flush_all() self.env.cr.execute( "UPDATE spp_group_membership SET start_date = %s, ended_date = %s, active = %s WHERE id = %s", @@ -48,19 +52,41 @@ def _read_stored_columns(self, rec): ) return self.env.cr.fetchone() + def _run_cron(self, **kwargs): + """Run the repair cron with ``ir.cron._commit_progress`` stubbed out. + + The real method commits, which on a TestCursor releases the test + savepoint and leaks this test's rows into the rest of the class. + The stub records ``(processed, remaining)`` per chunk so tests can + assert the chunking behaviour. + """ + calls = [] + + def fake_commit_progress(_cron, processed=0, remaining=None, **_kw): + calls.append((processed, remaining)) + return float("inf") + + with patch.object( + type(self.env["ir.cron"]), + "_commit_progress", + autospec=True, + side_effect=fake_commit_progress, + ): + repaired = self.Membership._cron_recompute_ended_status(**kwargs) + return repaired, calls + def test_cron_ends_membership_the_clock_has_crossed(self): rec = self._make_membership(self.individual_a) - now = fields.Datetime.now() - self._age_row(rec, now - timedelta(days=730), now - timedelta(days=365)) + self._age_row(rec) # Stale precondition: departed a year ago, still stored as active. self.assertEqual(self._read_stored_columns(rec), ("active", False)) self.assertEqual(rec.status, "active") self.assertFalse(rec.is_ended) - repaired = self.Membership._cron_recompute_ended_status() + repaired, _calls = self._run_cron() - self.assertEqual(repaired, rec) + self.assertIn(rec, repaired) self.assertEqual(rec.status, "inactive") self.assertTrue(rec.is_ended) # The SQL columns must be repaired too — four consumers read @@ -80,31 +106,49 @@ def test_cron_reactivates_membership_whose_end_moved_to_future(self): # The end date is pushed to the future behind the ORM's back; the # stored "inactive" is now wrong in the other direction. future = fields.Datetime.now() + timedelta(days=365) - self._age_row(rec, past - timedelta(days=1), future) + self._age_row(rec, future) self.assertEqual(rec.status, "inactive") self.assertTrue(rec.is_ended) - repaired = self.Membership._cron_recompute_ended_status() + repaired, _calls = self._run_cron() - self.assertEqual(repaired, rec) + self.assertIn(rec, repaired) self.assertEqual(rec.status, "active") self.assertFalse(rec.is_ended) self.assertEqual(self._read_stored_columns(rec), ("active", False)) def test_cron_repairs_archived_rows(self): rec = self._make_membership(self.individual_a) - now = fields.Datetime.now() - self._age_row(rec, now - timedelta(days=730), now - timedelta(days=365), active=False) + self._age_row(rec, active=False) self.assertEqual(rec.status, "active") self.assertFalse(rec.is_ended) - self.Membership._cron_recompute_ended_status() + self._run_cron() self.assertEqual(rec.status, "inactive") self.assertTrue(rec.is_ended) # The cron repairs the computes only; archiving stays as it was. self.assertFalse(rec.active) + def test_cron_repairs_null_is_ended_row(self): + # A raw INSERT that omits the nullable computed columns: every + # raw-SQL consumer treats NULL is_ended as ended, and the ORM alone + # cannot repair NULL -> False (the cache reads NULL as False, so a + # recompute writes nothing). + rec = self._make_membership(self.individual_a) + self.env.flush_all() + self.env.cr.execute( + "UPDATE spp_group_membership SET is_ended = NULL, status = NULL, ended_date = NULL WHERE id = %s", + (rec.id,), + ) + rec.invalidate_recordset() + self.assertEqual(self._read_stored_columns(rec), (None, None)) + + repaired, _calls = self._run_cron() + + self.assertIn(rec, repaired) + self.assertEqual(self._read_stored_columns(rec), ("active", False)) + def test_cron_leaves_correct_rows_untouched(self): open_ended = self._make_membership(self.individual_a) past = fields.Datetime.now() - timedelta(days=365) @@ -114,11 +158,11 @@ def test_cron_leaves_correct_rows_untouched(self): ended_date=past, ) - repaired = self.Membership._cron_recompute_ended_status() + repaired, _calls = self._run_cron() # An over-matching domain would sweep these rows in; they must not # be selected at all, not merely end up with unchanged values. - self.assertFalse(repaired) + self.assertFalse(repaired & (open_ended | already_ended)) self.assertEqual(open_ended.status, "active") self.assertFalse(open_ended.is_ended) self.assertEqual(already_ended.status, "inactive") @@ -126,44 +170,86 @@ def test_cron_leaves_correct_rows_untouched(self): def test_cron_invalidates_group_metrics(self): rec = self._make_membership(self.individual_a) - now = fields.Datetime.now() - self._age_row(rec, now - timedelta(days=730), now - timedelta(days=365)) + self._age_row(rec) # The recompute flushes through low-level SQL and bypasses write(), # so the cron must call the metric-invalidation funnel itself. - with patch.object( - type(self.env["res.partner"]), - "invalidate_group_metrics", - autospec=True, - ) as funnel: - self.Membership._cron_recompute_ended_status() + with _patch_invalidate_funnel(self.env) as funnel: + self._run_cron() - funnel.assert_called_once() - self.assertEqual(funnel.call_args.args[0], self.group) + self.assertTrue(funnel.called) + invalidated = self.env["res.partner"].browse() + for call in funnel.call_args_list: + invalidated |= call.args[0] + self.assertIn(self.group, invalidated) - def test_cron_respects_batch_size(self): + def test_cron_drains_backlog_in_batches(self): carol = self.Partner.create({"name": "Carol", "is_registrant": True, "is_group": False}) - now = fields.Datetime.now() rows = self.Membership.browse() for individual in (self.individual_a, self.individual_b, carol): rec = self._make_membership(individual) - self._age_row(rec, now - timedelta(days=730), now - timedelta(days=365)) + self._age_row(rec) rows |= rec - first = self.Membership._cron_recompute_ended_status(batch_size=2) - self.assertEqual(len(first), 2) + repaired, calls = self._run_cron(batch_size=2) - second = self.Membership._cron_recompute_ended_status(batch_size=2) - self.assertEqual(len(second), 1) - self.assertEqual(first | second, rows) + # One run drains the whole backlog in batch_size chunks, each + # reported (and committed) through _commit_progress. + self.assertTrue(all(rec in repaired for rec in rows)) for rec in rows: self.assertEqual(rec.status, "inactive") self.assertTrue(rec.is_ended) + self.assertEqual(sum(processed for processed, _remaining in calls), len(repaired)) + self.assertEqual(len(calls), -(-len(repaired) // 2)) # ceil(len/2) chunks + # A short final chunk reports the backlog as drained. (Guarded: if + # ambient stale rows ever pad the backlog to a multiple of the + # batch size, the last full chunk legitimately reports more work.) + if calls[-1][0] < 2: + self.assertEqual(calls[-1][1], 0) + + def test_future_ended_date_schedules_cron_trigger(self): + cron = self.env.ref("spp_registry.cron_recompute_membership_ended_status") + Trigger = self.env["ir.cron.trigger"] + future = fields.Datetime.now() + timedelta(days=30) + + before = Trigger.search([("cron_id", "=", cron.id)]) + rec = self._make_membership(self.individual_a, ended_date=future) + created = Trigger.search([("cron_id", "=", cron.id)]) - before + self.assertEqual(len(created), 1) + self.assertEqual(created.call_at, future) + + later = future + timedelta(days=5) + before = Trigger.search([("cron_id", "=", cron.id)]) + rec.write({"ended_date": later}) + created = Trigger.search([("cron_id", "=", cron.id)]) - before + self.assertEqual(len(created), 1) + self.assertEqual(created.call_at, later) + + def test_past_ended_date_schedules_no_cron_trigger(self): + # A past departure is recomputed correctly at write time; only a + # future one needs the clock-crossing repair scheduled. + cron = self.env.ref("spp_registry.cron_recompute_membership_ended_status") + Trigger = self.env["ir.cron.trigger"] + past = fields.Datetime.now() - timedelta(days=365) + + before = Trigger.search([("cron_id", "=", cron.id)]) + rec = self._make_membership( + self.individual_a, + start_date=past - timedelta(days=1), + ended_date=past, + ) + rec.write({"ended_date": past + timedelta(days=1)}) + self.assertFalse(Trigger.search([("cron_id", "=", cron.id)]) - before) def test_cron_record_registered(self): cron = self.env.ref("spp_registry.cron_recompute_membership_ended_status") self.assertEqual(cron.model_id.model, "spp.group.membership") self.assertTrue(cron.active) self.assertIn("_cron_recompute_ended_status", cron.code) + # Daily safety net only — the common path is the per-row trigger + # scheduled at the exact ended_date. self.assertEqual(cron.interval_number, 1) - self.assertEqual(cron.interval_type, "hours") + self.assertEqual(cron.interval_type, "days") + # Superuser pin: the unsudo'd searches must see memberships of + # disabled registrants despite the global ir.rule pair. + self.assertEqual(cron.user_id, self.env.ref("base.user_root")) From 43feda57e7c005cc93879579baba97888efe13be Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Tue, 1 Sep 2026 11:29:45 +0800 Subject: [PATCH 4/9] docs(registry): apply CI-generated README for 19.0.2.2.3 HISTORY fragment --- spp_registry/README.rst | 16 +++++++++++++ spp_registry/static/description/index.html | 27 ++++++++++++++++++---- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/spp_registry/README.rst b/spp_registry/README.rst index 1ead9f1ac..c38015ba6 100644 --- a/spp_registry/README.rst +++ b/spp_registry/README.rst @@ -139,6 +139,22 @@ Dependencies Changelog ========= +19.0.2.2.3 +~~~~~~~~~~ + +- fix(registry): repair the stored ``status``/``is_ended`` computes on + ``spp.group.membership`` once the clock crosses ``ended_date``. Both + fields depend only on ``ended_date`` compared against *now*, so a + future-dated departure never took effect once the clock crossed it — + rosters, metrics, API search and downstream gates kept treating the + member as active indefinitely. Writing a future ``ended_date`` now + schedules the repair cron at exactly that moment (staleness window ~1 + minute), and a daily sweep self-heals everything else: rows already + stale in existing databases (drained in committed batches on the first + run, however large the backlog) and rows written behind the ORM, + including ``is_ended = NULL`` rows that raw-SQL consumers treated as + ended (#417) + 19.0.2.2.2 ~~~~~~~~~~ diff --git a/spp_registry/static/description/index.html b/spp_registry/static/description/index.html index 063ba1ff7..a7330bf6c 100644 --- a/spp_registry/static/description/index.html +++ b/spp_registry/static/description/index.html @@ -518,6 +518,23 @@

Changelog

+

19.0.2.2.3

+
    +
  • fix(registry): repair the stored status/is_ended computes on +spp.group.membership once the clock crosses ended_date. Both +fields depend only on ended_date compared against now, so a +future-dated departure never took effect once the clock crossed it — +rosters, metrics, API search and downstream gates kept treating the +member as active indefinitely. Writing a future ended_date now +schedules the repair cron at exactly that moment (staleness window ~1 +minute), and a daily sweep self-heals everything else: rows already +stale in existing databases (drained in committed batches on the first +run, however large the backlog) and rows written behind the ORM, +including is_ended = NULL rows that raw-SQL consumers treated as +ended (#417)
    • +
+
+

19.0.2.2.2

  • fix(registry): let an ID type be used again after its ID was removed. @@ -529,7 +546,7 @@

    19.0.2.2.2

    surfacing a database error (#1136)
-
+

19.0.2.2.1

  • feat(registry): registry configuration is consolidated into one @@ -540,7 +557,7 @@

    19.0.2.2.1

    framework refuses a settings save from anyone else (#1009)
-
+

19.0.2.1.4

  • fix(registry): remove the dead @api.constrains("age") @@ -552,7 +569,7 @@

    19.0.2.1.4

    dropped
-
+

19.0.2.1.3

  • fix(registry): show an ID Status column on the group form @@ -563,7 +580,7 @@

    19.0.2.1.3

    (#1110)
-
+

19.0.2.1.1

  • fix(views): add reusable x2many_no_padding JS widget that @@ -573,7 +590,7 @@

    19.0.2.1.1

    don’t bloat the layout (#943).
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • From 35758197c5bdcf6a7986c7a60934298778a5e9a9 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Tue, 1 Sep 2026 12:00:38 +0800 Subject: [PATCH 5/9] fix(registry): harden the ended-status sweep per adversarial review - stop searching legs once a chunk is full; close the progress report on the empty-pass exit so even backlogs divide evenly into chunks - batch the NULL is_ended repair (LIMIT subquery + progress commits) - create() reads ended_date back from records (default_ended_date context fills are scheduled too); write() skips empty recordsets/unrelated vals - round trigger times up to the next minute (cron precision, dedupes bursts) - accumulate repaired ids as a set; report remaining as a 0/1 signal - honest docs: backlog drains across ASAP-rescheduled runs, not one run; the IS NULL legs are not index-served (expected empty); outside-cron calls commit - ir_cron.xml: eval booleans per house style; first nextcall deferred 1h - tests: time-budget resume path, NULL+past ORM-leg repair, default_ended_date trigger, minute rounding, officer-vs-root disabled-registrant pin; batch test ambient-row-proof; _run_cron model arg uses 'is None' (an empty model handle is falsy) --- spp_registry/data/ir_cron.xml | 8 +- spp_registry/models/group_membership.py | 114 +++++++++++------ spp_registry/readme/HISTORY.md | 2 +- .../tests/test_membership_status_cron.py | 116 ++++++++++++++++-- 4 files changed, 191 insertions(+), 49 deletions(-) diff --git a/spp_registry/data/ir_cron.xml b/spp_registry/data/ir_cron.xml index 72607fc50..1c8dde041 100644 --- a/spp_registry/data/ir_cron.xml +++ b/spp_registry/data/ir_cron.xml @@ -15,6 +15,12 @@ 1 days - True + + + diff --git a/spp_registry/models/group_membership.py b/spp_registry/models/group_membership.py index 795fd04e6..f4bf20baf 100644 --- a/spp_registry/models/group_membership.py +++ b/spp_registry/models/group_membership.py @@ -1,6 +1,7 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. import logging +from datetime import timedelta from odoo import _, api, fields, models from odoo.exceptions import ValidationError @@ -169,7 +170,8 @@ def write(self, vals): affected_groups |= self.mapped("group") self._invalidate_group_metrics(affected_groups) - self._schedule_ended_status_repair([vals]) + if self and "ended_date" in vals: + self._schedule_ended_status_repair([vals["ended_date"]]) return res @api.model_create_multi @@ -178,7 +180,9 @@ def create(self, vals_list): # Invalidate metrics for all affected groups groups = res.mapped("group") self._invalidate_group_metrics(groups) - self._schedule_ended_status_repair(vals_list) + # Read the dates back from the records, not vals_list: a missing + # key can still be filled from a default_ended_date context key. + self._schedule_ended_status_repair(res.mapped("ended_date")) return res def unlink(self): @@ -221,21 +225,26 @@ def _compute_status(self): # check if membership end date available and less than current date record.status = "inactive" if self._is_ended_as_of(record.ended_date, now) else "active" - def _schedule_ended_status_repair(self, vals_list): + def _schedule_ended_status_repair(self, ended_dates): """Point the repair cron at every future ``ended_date`` being written. The stored computes go stale the moment the clock crosses ``ended_date`` (see ``_cron_recompute_ended_status``); a persistent - ``ir.cron.trigger`` at exactly that time shrinks the staleness - window from the sweep cadence to about a minute. A stale or - duplicate trigger is harmless — it just runs the idempotent sweep. + ``ir.cron.trigger`` at that time shrinks the staleness window from + the sweep cadence to about a minute. Each moment is rounded up to + the next full minute — the cron worker's own precision — so bursts + of departures sharing a minute collapse into one trigger. A stale + or duplicate trigger is harmless: it just runs the idempotent sweep. """ now = fields.Datetime.now() - at_list = { - ended - for vals in vals_list - if (ended := fields.Datetime.to_datetime(vals.get("ended_date"))) and ended > now - } + at_list = set() + for ended in ended_dates: + ended = fields.Datetime.to_datetime(ended) + if not ended or ended <= now: + continue + if ended.second or ended.microsecond: + ended = ended.replace(second=0, microsecond=0) + timedelta(minutes=1) + at_list.add(ended) if at_list: self.env.ref("spp_registry.cron_recompute_membership_ended_status")._trigger(at=at_list) @@ -243,8 +252,13 @@ def _schedule_ended_status_repair(self, vals_list): def _stale_ended_status_domains(self, now): """Domains selecting rows whose stored ``status``/``is_ended`` disagree with the clock, one conjunctive leg per (date-window, - stale-column) pair so each search stays servable by the - ``ended_date`` index instead of forcing a full-table read. + stale-column) pair — no ORs, so the date-bound legs can be served + by the ``ended_date`` index. The two ``ended_date IS NULL`` legs + (which that partial index cannot serve) only guard rows written + behind the ORM and are expected to match nothing; if a registry + ever accumulates enough non-ORM drift for their scans to matter, + give them partial indexes or a last-swept watermark (see #421 for + the planned collapse of the status legs). """ return [ # Ended by the clock, still stored as active. @@ -258,7 +272,7 @@ def _stale_ended_status_domains(self, now): ] @api.model - def _repair_null_is_ended(self, now): + def _repair_null_is_ended(self, now, batch_size): """Repair rows holding ``is_ended = NULL`` that should read active. Rows written behind the ORM can leave ``is_ended`` NULL, and every @@ -268,20 +282,34 @@ def _repair_null_is_ended(self, now): SQL-level leg mirroring ``_is_ended_as_of``. (NULL rows whose ``ended_date`` has passed need no special casing: the computed True differs from the cached False, so the ORM legs repair them.) + + Expected to match nothing on a healthy database (the column has a + Python default, so it was backfilled at creation); still bounded + to ``batch_size`` rows per statement, with progress committed + between batches, so a pathological NULL population cannot pin one + unbounded UPDATE against the cron time limit. """ self.flush_model(["is_ended", "ended_date"]) - self.env.cr.execute( - "UPDATE spp_group_membership SET is_ended = false " - "WHERE is_ended IS NULL AND (ended_date IS NULL OR ended_date > %s) " - "RETURNING id", - (now,), - ) - ids = [row[0] for row in self.env.cr.fetchall()] - if not ids: - return self.browse() - repaired = self.browse(ids) - repaired.invalidate_recordset() - self._invalidate_group_metrics(repaired.mapped("group")) + repaired = self.browse() + while True: + self.env.cr.execute( + "UPDATE spp_group_membership SET is_ended = false " + "WHERE id IN (SELECT id FROM spp_group_membership " + "WHERE is_ended IS NULL AND (ended_date IS NULL OR ended_date > %s) LIMIT %s) " + "RETURNING id", + (now, batch_size), + ) + ids = [row[0] for row in self.env.cr.fetchall()] + if not ids: + break + batch = self.browse(ids) + batch.invalidate_recordset() + self._invalidate_group_metrics(batch.mapped("group")) + repaired |= batch + if len(ids) < batch_size: + break + if not self.env["ir.cron"]._commit_progress(len(ids)): + break return repaired @api.model @@ -312,18 +340,21 @@ def _cron_recompute_ended_status(self, batch_size=5000): a timely recompute would have stored. Rows are repaired in ``batch_size`` chunks, each committed via - ``ir.cron._commit_progress``: a large backlog drains within one - run, a run that exhausts the cron time budget is resumed ASAP - instead of waiting a full sweep interval, and a serialization - failure rolls back only its own chunk. The chunk size follows the - 5,000-record cap in docs/principles/performance-scalability.md. + ``ir.cron._commit_progress``: a serialization failure rolls back + only its own chunk, and a backlog larger than the cron time budget + drains across runs — a partially-done run is rescheduled ASAP + instead of waiting a full sweep interval. Because of those + commits, calling this outside a cron (e.g. from a shell) commits + the current transaction. The chunk size follows the 5,000-record + cap in docs/principles/performance-scalability.md. Returns the repaired memberships. """ if batch_size < 1: raise ValueError("batch_size must be a positive number of rows") memberships = self.with_context(active_test=False) - repaired = memberships._repair_null_is_ended(fields.Datetime.now()) + repaired = memberships._repair_null_is_ended(fields.Datetime.now(), batch_size) + repaired_ids = set(repaired.ids) while True: # Re-read the clock every pass: a row whose ended_date is # crossed while the run is in flight recomputes to the very @@ -331,26 +362,35 @@ def _cron_recompute_ended_status(self, batch_size=5000): now = fields.Datetime.now() chunk = memberships.browse() for leg in self._stale_ended_status_domains(now): - chunk |= memberships.search(leg, limit=batch_size) - chunk = chunk[:batch_size] + quota = batch_size - len(chunk) + if quota <= 0: + break + chunk |= memberships.search(leg, limit=quota) if not chunk: + if repaired_ids: + # Close the progress report so a backlog that divided + # evenly into chunks doesn't leave the job marked + # partially done (and pointlessly rescheduled ASAP). + self.env["ir.cron"]._commit_progress(0, remaining=0) break chunk.modified(["ended_date"]) # The recompute flushes through low-level SQL and bypasses this # model's write() override, so the metric-invalidation hook must # be called explicitly. self._invalidate_group_metrics(chunk.mapped("group")) - repaired |= chunk + repaired_ids.update(chunk.ids) self.env.flush_all() # A short chunk means every leg came back exhausted, so the - # backlog is drained (repaired rows drop out of the domains). + # backlog is drained (repaired rows drop out of the domains); + # `remaining` is a drained/not-drained signal, not a count. drained = len(chunk) < batch_size - time_left = self.env["ir.cron"]._commit_progress(len(chunk), remaining=0 if drained else batch_size) + time_left = self.env["ir.cron"]._commit_progress(len(chunk), remaining=0 if drained else 1) if drained: break if not time_left: _logger.info("[spp.registry] Ended-status backlog remains; the cron will be re-triggered to continue") break + repaired = memberships.browse(repaired_ids) if repaired: _logger.info( "[spp.registry] Repaired ended-status on %d group membership(s)", diff --git a/spp_registry/readme/HISTORY.md b/spp_registry/readme/HISTORY.md index e5fca57c6..0e6ddec50 100644 --- a/spp_registry/readme/HISTORY.md +++ b/spp_registry/readme/HISTORY.md @@ -1,6 +1,6 @@ ### 19.0.2.2.3 -- fix(registry): repair the stored `status`/`is_ended` computes on `spp.group.membership` once the clock crosses `ended_date`. Both fields depend only on `ended_date` compared against *now*, so a future-dated departure never took effect once the clock crossed it — rosters, metrics, API search and downstream gates kept treating the member as active indefinitely. Writing a future `ended_date` now schedules the repair cron at exactly that moment (staleness window ~1 minute), and a daily sweep self-heals everything else: rows already stale in existing databases (drained in committed batches on the first run, however large the backlog) and rows written behind the ORM, including `is_ended = NULL` rows that raw-SQL consumers treated as ended (#417) +- fix(registry): repair the stored `status`/`is_ended` computes on `spp.group.membership` once the clock crosses `ended_date`. Both fields depend only on `ended_date` compared against *now*, so a future-dated departure never took effect once the clock crossed it — rosters, metrics, API search and downstream gates kept treating the member as active indefinitely. Writing a future `ended_date` now schedules the repair cron at exactly that moment (staleness window ~1 minute), and a daily sweep self-heals everything else: rows already stale in existing databases (drained in committed batches, resuming across runs until the backlog is gone) and rows written behind the ORM, including `is_ended = NULL` rows that raw-SQL consumers treated as ended (#417) ### 19.0.2.2.2 diff --git a/spp_registry/tests/test_membership_status_cron.py b/spp_registry/tests/test_membership_status_cron.py index 63784d82a..c6ae086c7 100644 --- a/spp_registry/tests/test_membership_status_cron.py +++ b/spp_registry/tests/test_membership_status_cron.py @@ -52,27 +52,32 @@ def _read_stored_columns(self, rec): ) return self.env.cr.fetchone() - def _run_cron(self, **kwargs): + def _run_cron(self, model=None, time_left=float("inf"), **kwargs): """Run the repair cron with ``ir.cron._commit_progress`` stubbed out. The real method commits, which on a TestCursor releases the test savepoint and leaks this test's rows into the rest of the class. The stub records ``(processed, remaining)`` per chunk so tests can - assert the chunking behaviour. + assert the chunking behaviour; ``time_left`` is what the stub + reports back as the remaining cron time budget. """ calls = [] def fake_commit_progress(_cron, processed=0, remaining=None, **_kw): calls.append((processed, remaining)) - return float("inf") + return time_left + # `is None`, not `or`: a model handle is an empty recordset and + # therefore falsy — `model or ...` would silently fall back to the + # superuser-bound self.Membership. + target = self.Membership if model is None else model with patch.object( type(self.env["ir.cron"]), "_commit_progress", autospec=True, side_effect=fake_commit_progress, ): - repaired = self.Membership._cron_recompute_ended_status(**kwargs) + repaired = target._cron_recompute_ended_status(**kwargs) return repaired, calls def test_cron_ends_membership_the_clock_has_crossed(self): @@ -149,6 +154,49 @@ def test_cron_repairs_null_is_ended_row(self): self.assertIn(rec, repaired) self.assertEqual(self._read_stored_columns(rec), ("active", False)) + def test_cron_repairs_null_columns_with_past_ended_date(self): + # NULL is_ended/status with a departure already in the past must be + # repaired by the ORM legs: the computed True/"inactive" differs + # from the cached False, so — unlike the NULL-and-active case — a + # normal recompute does write. Pins the ORM-internals assumption + # documented on _repair_null_is_ended. + rec = self._make_membership(self.individual_a) + now = fields.Datetime.now() + self.env.flush_all() + self.env.cr.execute( + "UPDATE spp_group_membership SET is_ended = NULL, status = NULL, " + "start_date = %s, ended_date = %s WHERE id = %s", + (now - timedelta(days=730), now - timedelta(days=365), rec.id), + ) + rec.invalidate_recordset() + self.assertEqual(self._read_stored_columns(rec), (None, None)) + + repaired, _calls = self._run_cron() + + self.assertIn(rec, repaired) + self.assertEqual(self._read_stored_columns(rec), ("inactive", True)) + + def test_cron_repairs_memberships_of_disabled_registrants(self): + # Two global ir.rule records hide memberships of disabled + # registrants from ordinary users; the cron record pins + # user_id=base.user_root precisely so the sweep sees them. An + # officer-run sweep misses the row, the root-run sweep repairs it. + rec = self._make_membership(self.individual_a) + self._age_row(rec) + self.individual_a.disabled = fields.Datetime.now() + # Flush: the rule domain is evaluated in SQL, which must see the + # disabled stamp, not the pending cache value. + self.env.flush_all() + + officer = self._make_user("status_cron_officer", ["spp_registry.group_registry_officer"]) + missed, _calls = self._run_cron(model=self.Membership.with_user(officer)) + self.assertNotIn(rec, missed) + self.assertEqual(self._read_stored_columns(rec), ("active", False)) + + repaired, _calls = self._run_cron() + self.assertIn(rec, repaired) + self.assertEqual(self._read_stored_columns(rec), ("inactive", True)) + def test_cron_leaves_correct_rows_untouched(self): open_ended = self._make_membership(self.individual_a) past = fields.Datetime.now() - timedelta(days=365) @@ -194,23 +242,52 @@ def test_cron_drains_backlog_in_batches(self): repaired, calls = self._run_cron(batch_size=2) # One run drains the whole backlog in batch_size chunks, each - # reported (and committed) through _commit_progress. + # reported (and committed) through _commit_progress. Assertions + # stay scoped to the three rows this test created — ambient stale + # rows on a demo-seeded DB may only add to the totals. self.assertTrue(all(rec in repaired for rec in rows)) for rec in rows: self.assertEqual(rec.status, "inactive") self.assertTrue(rec.is_ended) - self.assertEqual(sum(processed for processed, _remaining in calls), len(repaired)) - self.assertEqual(len(calls), -(-len(repaired) // 2)) # ceil(len/2) chunks + self.assertGreaterEqual(len(calls), 2) + self.assertGreaterEqual(sum(processed for processed, _remaining in calls), len(rows)) + self.assertTrue(all(processed <= 2 for processed, _remaining in calls)) # A short final chunk reports the backlog as drained. (Guarded: if # ambient stale rows ever pad the backlog to a multiple of the # batch size, the last full chunk legitimately reports more work.) if calls[-1][0] < 2: self.assertEqual(calls[-1][1], 0) + def test_cron_resumes_after_time_budget_exhausted(self): + # The production path for any real backlog: _commit_progress + # reports no time left, the run stops after its current chunk with + # the backlog flagged, and the rescheduled run finishes the job. + carol = self.Partner.create({"name": "Carol", "is_registrant": True, "is_group": False}) + rows = self.Membership.browse() + for individual in (self.individual_a, self.individual_b, carol): + rec = self._make_membership(individual) + self._age_row(rec) + rows |= rec + + first, calls = self._run_cron(batch_size=1, time_left=0.0) + + self.assertEqual(len(calls), 1) + self.assertEqual(calls[0][0], 1) + self.assertNotEqual(calls[0][1], 0) # backlog reported as remaining + self.assertEqual(len(first & rows), 1) + + second, _calls = self._run_cron(batch_size=1) + + self.assertEqual((first | second) & rows, rows) + for rec in rows: + self.assertEqual(rec.status, "inactive") + self.assertTrue(rec.is_ended) + def test_future_ended_date_schedules_cron_trigger(self): cron = self.env.ref("spp_registry.cron_recompute_membership_ended_status") Trigger = self.env["ir.cron.trigger"] - future = fields.Datetime.now() + timedelta(days=30) + # A minute-aligned departure gets its trigger at exactly that time. + future = fields.Datetime.now().replace(second=0) + timedelta(days=30) before = Trigger.search([("cron_id", "=", cron.id)]) rec = self._make_membership(self.individual_a, ended_date=future) @@ -218,12 +295,31 @@ def test_future_ended_date_schedules_cron_trigger(self): self.assertEqual(len(created), 1) self.assertEqual(created.call_at, future) - later = future + timedelta(days=5) + # A mid-minute departure is rounded up to the next full minute + # (cron precision), never scheduled before the date itself. + later = future + timedelta(days=5, seconds=30) before = Trigger.search([("cron_id", "=", cron.id)]) rec.write({"ended_date": later}) created = Trigger.search([("cron_id", "=", cron.id)]) - before self.assertEqual(len(created), 1) - self.assertEqual(created.call_at, later) + self.assertEqual(created.call_at, later.replace(second=0) + timedelta(minutes=1)) + + def test_default_ended_date_context_schedules_cron_trigger(self): + # default_get fills a missing ended_date from the context; the + # trigger must still be scheduled (create reads the records back, + # not the raw vals). + cron = self.env.ref("spp_registry.cron_recompute_membership_ended_status") + Trigger = self.env["ir.cron.trigger"] + future = fields.Datetime.now().replace(second=0) + timedelta(days=30) + + before = Trigger.search([("cron_id", "=", cron.id)]) + rec = self.Membership.with_context(default_ended_date=future).create( + {"group": self.group.id, "individual": self.individual_a.id} + ) + self.assertEqual(rec.ended_date, future) + created = Trigger.search([("cron_id", "=", cron.id)]) - before + self.assertEqual(len(created), 1) + self.assertEqual(created.call_at, future) def test_past_ended_date_schedules_no_cron_trigger(self): # A past departure is recomputed correctly at write time; only a From c905f8a4105aadfdff536edafd3dbbacb32850ef Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Tue, 1 Sep 2026 12:06:30 +0800 Subject: [PATCH 6/9] docs(registry): apply CI-generated README for reworded 19.0.2.2.3 fragment --- spp_registry/README.rst | 8 ++++---- spp_registry/static/description/index.html | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/spp_registry/README.rst b/spp_registry/README.rst index c38015ba6..0a3c888d0 100644 --- a/spp_registry/README.rst +++ b/spp_registry/README.rst @@ -150,10 +150,10 @@ Changelog member as active indefinitely. Writing a future ``ended_date`` now schedules the repair cron at exactly that moment (staleness window ~1 minute), and a daily sweep self-heals everything else: rows already - stale in existing databases (drained in committed batches on the first - run, however large the backlog) and rows written behind the ORM, - including ``is_ended = NULL`` rows that raw-SQL consumers treated as - ended (#417) + stale in existing databases (drained in committed batches, resuming + across runs until the backlog is gone) and rows written behind the + ORM, including ``is_ended = NULL`` rows that raw-SQL consumers treated + as ended (#417) 19.0.2.2.2 ~~~~~~~~~~ diff --git a/spp_registry/static/description/index.html b/spp_registry/static/description/index.html index a7330bf6c..0849e997d 100644 --- a/spp_registry/static/description/index.html +++ b/spp_registry/static/description/index.html @@ -528,10 +528,10 @@

    19.0.2.2.3

    member as active indefinitely. Writing a future ended_date now schedules the repair cron at exactly that moment (staleness window ~1 minute), and a daily sweep self-heals everything else: rows already -stale in existing databases (drained in committed batches on the first -run, however large the backlog) and rows written behind the ORM, -including is_ended = NULL rows that raw-SQL consumers treated as -ended (#417) +stale in existing databases (drained in committed batches, resuming +across runs until the backlog is gone) and rows written behind the +ORM, including is_ended = NULL rows that raw-SQL consumers treated +as ended (#417)
From f31fd28ba9b1ba793844f4541ca09b21e7dbbb2c Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Wed, 2 Sep 2026 10:40:37 +0800 Subject: [PATCH 7/9] fix(registry): harden ended-status repair per round-2 review - split the sweep: writes of a future ended_date now trigger a lightweight cron running only the two index-served crossed legs, while the daily safety net keeps the full-scan NULL-drift probes - a per-departure run no longer seq-scans the membership table - track backlog exhaustion per leg: the overlapping legs de-duplicate in the chunk union, so a short chunk alone falsely reported the backlog drained (remaining=0 -> next sweep a day out) - report progress explicitly in the NULL-repair loop: the implicit remaining computed to 0 on its budget break, downgrading the ASAP continuation to a daily one; the loop also hands its budget back so the ORM passes are skipped once time is gone - rotate the starting leg per pass so one direction's backlog cannot starve the others within a run - stamp write_date/write_uid in the raw NULL repair so write_date-keyed consumers (incremental syncs, API changed_by) see it - degrade gracefully when the trigger cron record was deleted (raise_if_not_found=False) instead of breaking membership writes - round every trigger up past ended_date unconditionally: the cron machinery consumes triggers on the DB clock while the predicate uses the app clock, so an exact-time trigger could be consumed early - reuse pending triggers for the same minute so cohort exits written row-per-call file one trigger, not one per membership - HISTORY upgrade note: pre-create the ended_date partial index CONCURRENTLY on very large registries --- spp_registry/data/ir_cron.xml | 34 ++- spp_registry/models/group_membership.py | 209 +++++++++++++----- spp_registry/readme/HISTORY.md | 3 +- .../tests/test_membership_status_cron.py | 199 +++++++++++++++-- 4 files changed, 358 insertions(+), 87 deletions(-) diff --git a/spp_registry/data/ir_cron.xml b/spp_registry/data/ir_cron.xml index 1c8dde041..c87a2b195 100644 --- a/spp_registry/data/ir_cron.xml +++ b/spp_registry/data/ir_cron.xml @@ -1,12 +1,12 @@ - + Registry: Recompute Ended Group Memberships @@ -23,4 +23,24 @@ /> + + + + Registry: Repair Clock-Crossed Group Memberships + + code + model._cron_repair_crossed_ended_status() + 1 + days + + + + diff --git a/spp_registry/models/group_membership.py b/spp_registry/models/group_membership.py index f4bf20baf..0423a0557 100644 --- a/spp_registry/models/group_membership.py +++ b/spp_registry/models/group_membership.py @@ -231,10 +231,17 @@ def _schedule_ended_status_repair(self, ended_dates): The stored computes go stale the moment the clock crosses ``ended_date`` (see ``_cron_recompute_ended_status``); a persistent ``ir.cron.trigger`` at that time shrinks the staleness window from - the sweep cadence to about a minute. Each moment is rounded up to - the next full minute — the cron worker's own precision — so bursts - of departures sharing a minute collapse into one trigger. A stale - or duplicate trigger is harmless: it just runs the idempotent sweep. + the sweep cadence to a couple of minutes. Each moment is rounded up + to the next full minute *past* ``ended_date`` — unconditionally, + because the cron machinery consumes triggers against the database + clock while the repair predicate compares against the application + clock, so a trigger firing exactly at ``ended_date`` could be + consumed while the row still reads as not-yet-ended. Triggers + already pending for the same minute are reused, so a cohort + departure written one row per call files a single trigger; two + concurrent transactions can still race to a duplicate, and a + trigger orphaned by a later date change survives to its moment — + both harmless, each just runs one idempotent index-served sweep. """ now = fields.Datetime.now() at_list = set() @@ -242,28 +249,55 @@ def _schedule_ended_status_repair(self, ended_dates): ended = fields.Datetime.to_datetime(ended) if not ended or ended <= now: continue - if ended.second or ended.microsecond: - ended = ended.replace(second=0, microsecond=0) + timedelta(minutes=1) - at_list.add(ended) + at_list.add(ended.replace(second=0, microsecond=0) + timedelta(minutes=1)) + if not at_list: + return + # raise_if_not_found=False: if the noupdate cron record has been + # deleted, membership writes must degrade to the daily sweep, not + # break the core registry write path over a latency optimisation. + cron = self.env.ref( + "spp_registry.cron_repair_crossed_membership_ended_status", + raise_if_not_found=False, + ) + if not cron: + return + pending = ( + self.env["ir.cron.trigger"] # nosemgrep: odoo-sudo-without-context + .sudo() # system table ordinary users cannot read; _trigger creates as sudo anyway + .search([("cron_id", "=", cron.id), ("call_at", "in", list(at_list))]) + ) + at_list -= set(pending.mapped("call_at")) if at_list: - self.env.ref("spp_registry.cron_recompute_membership_ended_status")._trigger(at=at_list) + cron._trigger(at=at_list) @api.model - def _stale_ended_status_domains(self, now): - """Domains selecting rows whose stored ``status``/``is_ended`` - disagree with the clock, one conjunctive leg per (date-window, - stale-column) pair — no ORs, so the date-bound legs can be served - by the ``ended_date`` index. The two ``ended_date IS NULL`` legs - (which that partial index cannot serve) only guard rows written - behind the ORM and are expected to match nothing; if a registry - ever accumulates enough non-ORM drift for their scans to matter, - give them partial indexes or a last-swept watermark (see #421 for - the planned collapse of the status legs). + def _crossed_ended_status_domains(self, now): + """Domains selecting rows the clock has crossed: ``ended_date`` is + past but a stored column still reads active. One conjunctive leg + per stale column — no ORs, so both legs are served by the partial + ``ended_date`` index. These are the only stale states the mere + passage of time can produce, so the trigger-driven cron sweeps + just these. """ return [ - # Ended by the clock, still stored as active. [("ended_date", "<=", now), ("is_ended", "=", False)], [("ended_date", "<=", now), ("status", "!=", "inactive")], + ] + + @api.model + def _stale_ended_status_domains(self, now): + """All domains selecting rows whose stored ``status``/``is_ended`` + disagree with the clock. Beyond the crossed legs this adds the + reactivation legs, which only guard rows whose ``ended_date`` was + cleared or future-moved behind the ORM and are expected to match + nothing. The two ``ended_date IS NULL`` legs cannot be served by + the partial index (each probe is a full table scan), which is why + only the daily safety net runs them — never the trigger-driven + cron. If a registry ever accumulates enough non-ORM drift for + those scans to matter, give them partial indexes or a last-swept + watermark (see #421 for the planned collapse of the status legs). + """ + return self._crossed_ended_status_domains(now) + [ # Not (or no longer) ended, still stored as ended. [("ended_date", "=", False), ("is_ended", "=", True)], [("ended_date", "=", False), ("status", "!=", "active")], @@ -284,20 +318,30 @@ def _repair_null_is_ended(self, now, batch_size): True differs from the cached False, so the ORM legs repair them.) Expected to match nothing on a healthy database (the column has a - Python default, so it was backfilled at creation); still bounded - to ``batch_size`` rows per statement, with progress committed - between batches, so a pathological NULL population cannot pin one - unbounded UPDATE against the cron time limit. + Python default, so it was backfilled at creation) — but the + ``is_ended IS NULL`` probe has no index behind it, so even proving + that costs a full table scan, which is why only the daily safety + net calls this, never the trigger-driven cron. The UPDATE stamps + ``write_date``/``write_uid`` the way the ORM legs' flush does, so + ``write_date``-keyed consumers (incremental syncs, the API's + ``changed_by``) see the repair. Work is bounded to ``batch_size`` + rows per statement, with progress committed between batches. + + Returns ``(repaired, time_left)``: the repaired memberships and + the cron time budget reported by the last progress commit — falsy + when the budget ran out with NULL rows possibly left. """ self.flush_model(["is_ended", "ended_date"]) repaired = self.browse() + time_left = float("inf") while True: self.env.cr.execute( - "UPDATE spp_group_membership SET is_ended = false " + "UPDATE spp_group_membership " + "SET is_ended = false, write_date = %s, write_uid = %s " "WHERE id IN (SELECT id FROM spp_group_membership " "WHERE is_ended IS NULL AND (ended_date IS NULL OR ended_date > %s) LIMIT %s) " "RETURNING id", - (now, batch_size), + (now, self.env.uid, now, batch_size), ) ids = [row[0] for row in self.env.cr.fetchall()] if not ids: @@ -307,70 +351,118 @@ def _repair_null_is_ended(self, now, batch_size): self._invalidate_group_metrics(batch.mapped("group")) repaired |= batch if len(ids) < batch_size: + self.env["ir.cron"]._commit_progress(len(ids), remaining=0) break - if not self.env["ir.cron"]._commit_progress(len(ids)): + # A full batch may not be the last: report the backlog so a + # budget-exhausted run is continued ASAP instead of waiting a + # full sweep interval on a "no work remaining" report. + time_left = self.env["ir.cron"]._commit_progress(len(ids), remaining=1) + if not time_left: break - return repaired + return repaired, time_left + + @api.model + def _cron_repair_crossed_ended_status(self, batch_size=5000): + """Trigger-driven repair: only the index-served crossed legs. + + ``_schedule_ended_status_repair`` points this cron at every future + ``ended_date`` written, so it can run many times a day and must + stay cheap — no NULL-drift legs (each of those probes is a full + table scan; the daily ``_cron_recompute_ended_status`` sweeps + them). See that method for the full story and the shared + semantics. + """ + return self._repair_stale_ended_status(batch_size, full=False) @api.model def _cron_recompute_ended_status(self, batch_size=5000): - """Repair stored ``status``/``is_ended`` on rows the clock has crossed. + """Daily safety net: repair stored ``status``/``is_ended`` on every + row whose stored values disagree with the clock. Both computes depend only on ``ended_date`` and compare it against *now*, so a recompute fires on a write to ``ended_date`` but never when time passes it: a future-dated departure would stay stored as - active forever. Writes of a future ``ended_date`` schedule a cron - trigger at exactly that moment (``_schedule_ended_status_repair``), - so this periodic sweep is the safety net — it self-heals rows - already stale in pre-existing databases and rows written behind - the ORM. It finds rows whose stored values disagree with the clock - and re-triggers the computes through the normal ORM path. Archived - rows are included — the UI onchange archives memberships ended in - the past, and those must be repaired too. ``active`` itself is - deliberately left untouched: archiving changes record visibility - everywhere and is a separate decision from the stored computes - (see issue #417). + active forever. Writes of a future ``ended_date`` schedule the + lightweight ``_cron_repair_crossed_ended_status`` in the minute + after that moment (``_schedule_ended_status_repair``), so this + daily sweep self-heals what triggers cannot cover: rows already + stale in pre-existing databases and rows written behind the ORM, + including the full-scan NULL-drift legs and + ``_repair_null_is_ended``. It finds stale rows and re-triggers the + computes through the normal ORM path. Archived rows are included — + the UI onchange archives memberships ended in the past, and those + must be repaired too. ``active`` itself is deliberately left + untouched: archiving changes record visibility everywhere and is a + separate decision from the stored computes (see issue #417). Two write-path side effects intentionally differ from a real write: the recompute flush still stamps ``write_uid``/``write_date`` (repaired rows show the cron user as last modified — e.g. - ``changed_by`` in the API's membership history reads ``write_uid``), - and the repair is invisible to ``spp_audit`` write-rules, which - hook ``write()``. Both are accepted: the repair only restores what - a timely recompute would have stored. + ``changed_by`` in the API's membership history reads ``write_uid``; + the raw NULL-repair UPDATE stamps the same columns itself), and + the repair is invisible to ``spp_audit`` write-rules, which hook + ``write()``. Both are accepted: the repair only restores what a + timely recompute would have stored. Rows are repaired in ``batch_size`` chunks, each committed via ``ir.cron._commit_progress``: a serialization failure rolls back only its own chunk, and a backlog larger than the cron time budget drains across runs — a partially-done run is rescheduled ASAP - instead of waiting a full sweep interval. Because of those + instead of waiting a full sweep interval. ``_commit_progress`` is + Odoo 19's sanctioned batching API for cron work and the deliberate + exception to the AGENTS.md checklist's "no ``cr.commit()`` in + loops" rule, which targets ad-hoc commits. Because of those commits, calling this outside a cron (e.g. from a shell) commits the current transaction. The chunk size follows the 5,000-record cap in docs/principles/performance-scalability.md. Returns the repaired memberships. """ + return self._repair_stale_ended_status(batch_size, full=True) + + @api.model + def _repair_stale_ended_status(self, batch_size, full): + """Shared repair loop; see ``_cron_recompute_ended_status``.""" if batch_size < 1: raise ValueError("batch_size must be a positive number of rows") memberships = self.with_context(active_test=False) - repaired = memberships._repair_null_is_ended(fields.Datetime.now(), batch_size) - repaired_ids = set(repaired.ids) - while True: + repaired_ids = set() + time_left = float("inf") + if full: + null_repaired, time_left = memberships._repair_null_is_ended(fields.Datetime.now(), batch_size) + repaired_ids.update(null_repaired.ids) + domains = self._stale_ended_status_domains if full else self._crossed_ended_status_domains + pass_no = 0 + while time_left: # Re-read the clock every pass: a row whose ended_date is # crossed while the run is in flight recomputes to the very # values a stale `now` would keep selecting it for. now = fields.Datetime.now() + legs = domains(now) + # Rotate the starting leg each pass so one direction's large + # backlog cannot starve the other directions within a run. + offset = pass_no % len(legs) + pass_no += 1 chunk = memberships.browse() - for leg in self._stale_ended_status_domains(now): + exhausted = True + for leg in legs[offset:] + legs[:offset]: quota = batch_size - len(chunk) if quota <= 0: + exhausted = False break - chunk |= memberships.search(leg, limit=quota) + found = memberships.search(leg, limit=quota) + if len(found) == quota: + # The leg may hold more. Exhaustion must be tracked + # per leg: the legs overlap and the union + # de-duplicates, so a short chunk alone is no proof + # the backlog is drained. + exhausted = False + chunk |= found if not chunk: if repaired_ids: - # Close the progress report so a backlog that divided - # evenly into chunks doesn't leave the job marked - # partially done (and pointlessly rescheduled ASAP). + # Close the progress report so a drained backlog isn't + # left marked partially done (and pointlessly + # rescheduled ASAP). self.env["ir.cron"]._commit_progress(0, remaining=0) break chunk.modified(["ended_date"]) @@ -380,16 +472,13 @@ def _cron_recompute_ended_status(self, batch_size=5000): self._invalidate_group_metrics(chunk.mapped("group")) repaired_ids.update(chunk.ids) self.env.flush_all() - # A short chunk means every leg came back exhausted, so the - # backlog is drained (repaired rows drop out of the domains); # `remaining` is a drained/not-drained signal, not a count. - drained = len(chunk) < batch_size - time_left = self.env["ir.cron"]._commit_progress(len(chunk), remaining=0 if drained else 1) - if drained: - break - if not time_left: - _logger.info("[spp.registry] Ended-status backlog remains; the cron will be re-triggered to continue") + if exhausted: + self.env["ir.cron"]._commit_progress(len(chunk), remaining=0) break + time_left = self.env["ir.cron"]._commit_progress(len(chunk), remaining=1) + if not time_left: + _logger.info("[spp.registry] Ended-status backlog remains; the cron will be re-triggered to continue") repaired = memberships.browse(repaired_ids) if repaired: _logger.info( diff --git a/spp_registry/readme/HISTORY.md b/spp_registry/readme/HISTORY.md index 0e6ddec50..f75e01fb1 100644 --- a/spp_registry/readme/HISTORY.md +++ b/spp_registry/readme/HISTORY.md @@ -1,6 +1,7 @@ ### 19.0.2.2.3 -- fix(registry): repair the stored `status`/`is_ended` computes on `spp.group.membership` once the clock crosses `ended_date`. Both fields depend only on `ended_date` compared against *now*, so a future-dated departure never took effect once the clock crossed it — rosters, metrics, API search and downstream gates kept treating the member as active indefinitely. Writing a future `ended_date` now schedules the repair cron at exactly that moment (staleness window ~1 minute), and a daily sweep self-heals everything else: rows already stale in existing databases (drained in committed batches, resuming across runs until the backlog is gone) and rows written behind the ORM, including `is_ended = NULL` rows that raw-SQL consumers treated as ended (#417) +- fix(registry): repair the stored `status`/`is_ended` computes on `spp.group.membership` once the clock crosses `ended_date`. Both fields depend only on `ended_date` compared against *now*, so a future-dated departure never took effect once the clock crossed it — rosters, metrics, API search and downstream gates kept treating the member as active indefinitely. Writing a future `ended_date` now schedules a lightweight, index-served repair cron in the minute after that moment (staleness window ≈1–2 minutes), and a daily sweep self-heals everything else: rows already stale in existing databases (drained in committed batches, resuming across runs until the backlog is gone) and rows written behind the ORM, including `is_ended = NULL` rows that raw-SQL consumers treated as ended (#417) +- upgrade note: this version adds a partial index on `spp_group_membership.ended_date`, built with a write-blocking `CREATE INDEX` during the module upgrade. On a very large registry, pre-create it concurrently before upgrading and the upgrade will skip the build: `CREATE INDEX CONCURRENTLY IF NOT EXISTS spp_group_membership__ended_date_index ON spp_group_membership (ended_date) WHERE ended_date IS NOT NULL;` ### 19.0.2.2.2 diff --git a/spp_registry/tests/test_membership_status_cron.py b/spp_registry/tests/test_membership_status_cron.py index c6ae086c7..de7dd034e 100644 --- a/spp_registry/tests/test_membership_status_cron.py +++ b/spp_registry/tests/test_membership_status_cron.py @@ -52,14 +52,15 @@ def _read_stored_columns(self, rec): ) return self.env.cr.fetchone() - def _run_cron(self, model=None, time_left=float("inf"), **kwargs): - """Run the repair cron with ``ir.cron._commit_progress`` stubbed out. + def _run_cron(self, model=None, time_left=float("inf"), method="_cron_recompute_ended_status", **kwargs): + """Run a repair cron with ``ir.cron._commit_progress`` stubbed out. The real method commits, which on a TestCursor releases the test savepoint and leaks this test's rows into the rest of the class. The stub records ``(processed, remaining)`` per chunk so tests can assert the chunking behaviour; ``time_left`` is what the stub - reports back as the remaining cron time budget. + reports back as the remaining cron time budget. ``method`` picks + the cron entrypoint (daily full sweep by default). """ calls = [] @@ -77,7 +78,7 @@ def fake_commit_progress(_cron, processed=0, remaining=None, **_kw): autospec=True, side_effect=fake_commit_progress, ): - repaired = target._cron_recompute_ended_status(**kwargs) + repaired = getattr(target, method)(**kwargs) return repaired, calls def test_cron_ends_membership_the_clock_has_crossed(self): @@ -141,10 +142,12 @@ def test_cron_repairs_null_is_ended_row(self): # cannot repair NULL -> False (the cache reads NULL as False, so a # recompute writes nothing). rec = self._make_membership(self.individual_a) + old_stamp = fields.Datetime.now() - timedelta(days=365) self.env.flush_all() self.env.cr.execute( - "UPDATE spp_group_membership SET is_ended = NULL, status = NULL, ended_date = NULL WHERE id = %s", - (rec.id,), + "UPDATE spp_group_membership SET is_ended = NULL, status = NULL, ended_date = NULL, " + "write_date = %s WHERE id = %s", + (old_stamp, rec.id), ) rec.invalidate_recordset() self.assertEqual(self._read_stored_columns(rec), (None, None)) @@ -153,6 +156,16 @@ def test_cron_repairs_null_is_ended_row(self): self.assertIn(rec, repaired) self.assertEqual(self._read_stored_columns(rec), ("active", False)) + # The raw repair stamps write metadata the way the ORM legs' flush + # does, so write_date-keyed consumers (incremental syncs, the + # API's changed_by) see the repair. + self.env.cr.execute( + "SELECT write_date, write_uid FROM spp_group_membership WHERE id = %s", + (rec.id,), + ) + write_date, write_uid = self.env.cr.fetchone() + self.assertGreater(write_date, old_stamp) + self.assertEqual(write_uid, self.env.uid) def test_cron_repairs_null_columns_with_past_ended_date(self): # NULL is_ended/status with a departure already in the past must be @@ -283,20 +296,131 @@ def test_cron_resumes_after_time_budget_exhausted(self): self.assertEqual(rec.status, "inactive") self.assertTrue(rec.is_ended) + def test_overlapping_legs_do_not_end_run_early(self): + # Rows stale on both columns match the first two legs at once and + # the chunk union de-duplicates, so a chunk shorter than + # batch_size is no proof the backlog is drained — exhaustion must + # be tracked per leg, or rows stale on one column only are left + # behind with the run reported fully done. + status_only = self.Membership.browse() + for name in ("Dave", "Erin"): + individual = self.Partner.create({"name": name, "is_registrant": True, "is_group": False}) + rec = self._make_membership(individual) + self._age_row(rec) + # is_ended already correct; only status is stale (leg 2 only). + self.env.cr.execute( + "UPDATE spp_group_membership SET is_ended = true WHERE id = %s", + (rec.id,), + ) + rec.invalidate_recordset() + status_only |= rec + # Newer ids — searched first under `id desc` — stale on both. + carol = self.Partner.create({"name": "Carol", "is_registrant": True, "is_group": False}) + both_stale = self.Membership.browse() + for individual in (self.individual_a, self.individual_b, carol): + rec = self._make_membership(individual) + self._age_row(rec) + both_stale |= rec + + repaired, calls = self._run_cron(batch_size=4) + + # One run repairs every row: pass 1 fills mostly from leg 1 and + # its leg-2 quota only re-finds chunk rows, so a union-size + # drained check would have stopped here with the status-only rows + # unrepaired until the next daily sweep. + rows = both_stale | status_only + self.assertEqual(repaired & rows, rows) + for rec in rows: + self.assertEqual(self._read_stored_columns(rec), ("inactive", True)) + self.assertEqual(calls[-1][1], 0) + + def test_crossed_cron_repairs_only_index_served_legs(self): + crossed = self._make_membership(self.individual_a) + self._age_row(crossed) + null_row = self._make_membership(self.individual_b) + self.env.flush_all() + self.env.cr.execute( + "UPDATE spp_group_membership SET is_ended = NULL, status = NULL, ended_date = NULL WHERE id = %s", + (null_row.id,), + ) + null_row.invalidate_recordset() + + repaired, _calls = self._run_cron(method="_cron_repair_crossed_ended_status") + + self.assertIn(crossed, repaired) + self.assertEqual(self._read_stored_columns(crossed), ("inactive", True)) + # NULL drift needs full-scan probes; the trigger-driven cron runs + # per departure and must leave those to the daily safety net. + self.assertNotIn(null_row, repaired) + self.assertEqual(self._read_stored_columns(null_row), (None, None)) + + repaired, _calls = self._run_cron() + + self.assertIn(null_row, repaired) + self.assertEqual(self._read_stored_columns(null_row), ("active", False)) + + def _make_null_rows(self): + rows = self.Membership.browse() + for individual in (self.individual_a, self.individual_b): + rows |= self._make_membership(individual) + self.env.flush_all() + self.env.cr.execute( + "UPDATE spp_group_membership SET is_ended = NULL, ended_date = NULL WHERE id IN %s", + (tuple(rows.ids),), + ) + rows.invalidate_recordset() + return rows + + def test_null_repair_drains_in_batches(self): + rows = self._make_null_rows() + + repaired, calls = self._run_cron(batch_size=1) + + self.assertEqual(repaired & rows, rows) + for rec in rows: + self.assertEqual(self._read_stored_columns(rec), ("active", False)) + # Every batch is bounded and reported; the run closes with the + # backlog reported drained. + self.assertTrue(all(processed <= 1 for processed, _remaining in calls)) + self.assertGreaterEqual(sum(processed for processed, _remaining in calls), len(rows)) + self.assertEqual(calls[-1][1], 0) + + def test_null_repair_stops_when_time_budget_exhausted(self): + # A NULL row with no ended_date matches none of the ORM legs, so + # the NULL loop itself must report the backlog: with the budget + # gone after one batch, the run stops flagged partially done and + # is continued ASAP — not a day later on a "fully done" report. + rows = self._make_null_rows() + + first, calls = self._run_cron(batch_size=1, time_left=0.0) + + self.assertEqual(calls, [(1, 1)]) + self.assertEqual(len(first & rows), 1) + + second, _calls = self._run_cron(batch_size=1) + + self.assertEqual((first | second) & rows, rows) + for rec in rows: + self.assertEqual(self._read_stored_columns(rec), ("active", False)) + def test_future_ended_date_schedules_cron_trigger(self): - cron = self.env.ref("spp_registry.cron_recompute_membership_ended_status") + cron = self.env.ref("spp_registry.cron_repair_crossed_membership_ended_status") Trigger = self.env["ir.cron.trigger"] - # A minute-aligned departure gets its trigger at exactly that time. + # Every departure is scheduled in the minute *after* ended_date — + # even a minute-aligned one: the cron machinery consumes triggers + # against the database clock while the repair predicate compares + # the application clock, so an exact-time trigger could be + # consumed a hair before the row reads as ended. future = fields.Datetime.now().replace(second=0) + timedelta(days=30) before = Trigger.search([("cron_id", "=", cron.id)]) rec = self._make_membership(self.individual_a, ended_date=future) created = Trigger.search([("cron_id", "=", cron.id)]) - before self.assertEqual(len(created), 1) - self.assertEqual(created.call_at, future) + self.assertEqual(created.call_at, future + timedelta(minutes=1)) - # A mid-minute departure is rounded up to the next full minute - # (cron precision), never scheduled before the date itself. + # A mid-minute departure lands in the next full minute as well, + # never scheduled at or before the date itself. later = future + timedelta(days=5, seconds=30) before = Trigger.search([("cron_id", "=", cron.id)]) rec.write({"ended_date": later}) @@ -308,7 +432,7 @@ def test_default_ended_date_context_schedules_cron_trigger(self): # default_get fills a missing ended_date from the context; the # trigger must still be scheduled (create reads the records back, # not the raw vals). - cron = self.env.ref("spp_registry.cron_recompute_membership_ended_status") + cron = self.env.ref("spp_registry.cron_repair_crossed_membership_ended_status") Trigger = self.env["ir.cron.trigger"] future = fields.Datetime.now().replace(second=0) + timedelta(days=30) @@ -319,12 +443,12 @@ def test_default_ended_date_context_schedules_cron_trigger(self): self.assertEqual(rec.ended_date, future) created = Trigger.search([("cron_id", "=", cron.id)]) - before self.assertEqual(len(created), 1) - self.assertEqual(created.call_at, future) + self.assertEqual(created.call_at, future + timedelta(minutes=1)) def test_past_ended_date_schedules_no_cron_trigger(self): # A past departure is recomputed correctly at write time; only a # future one needs the clock-crossing repair scheduled. - cron = self.env.ref("spp_registry.cron_recompute_membership_ended_status") + cron = self.env.ref("spp_registry.cron_repair_crossed_membership_ended_status") Trigger = self.env["ir.cron.trigger"] past = fields.Datetime.now() - timedelta(days=365) @@ -337,15 +461,52 @@ def test_past_ended_date_schedules_no_cron_trigger(self): rec.write({"ended_date": past + timedelta(days=1)}) self.assertFalse(Trigger.search([("cron_id", "=", cron.id)]) - before) + def test_same_minute_departures_share_one_trigger(self): + # A cohort exit written one membership per call must not file one + # trigger row per membership — pending triggers for the same + # minute are reused (ir.cron._trigger itself never de-duplicates + # across calls). + cron = self.env.ref("spp_registry.cron_repair_crossed_membership_ended_status") + Trigger = self.env["ir.cron.trigger"] + future = fields.Datetime.now().replace(second=0) + timedelta(days=30) + + before = Trigger.search([("cron_id", "=", cron.id)]) + self._make_membership(self.individual_a, ended_date=future) + self._make_membership(self.individual_b, ended_date=future + timedelta(seconds=30)) + created = Trigger.search([("cron_id", "=", cron.id)]) - before + self.assertEqual(len(created), 1) + self.assertEqual(created.call_at, future + timedelta(minutes=1)) + + def test_missing_trigger_cron_degrades_to_daily_sweep(self): + # The trigger cron is a latency optimisation. If the noupdate + # record was deleted by an admin, writing a membership must still + # succeed — the daily sweep repairs the row eventually — rather + # than raising from env.ref on the core registry write path. + self.env.ref("spp_registry.cron_repair_crossed_membership_ended_status").unlink() + future = fields.Datetime.now() + timedelta(days=30) + + rec = self._make_membership(self.individual_a, ended_date=future) + + self.assertEqual(rec.ended_date, future) + def test_cron_record_registered(self): + # Daily full safety net — the common path is the per-departure + # trigger on the crossed cron below. cron = self.env.ref("spp_registry.cron_recompute_membership_ended_status") self.assertEqual(cron.model_id.model, "spp.group.membership") self.assertTrue(cron.active) self.assertIn("_cron_recompute_ended_status", cron.code) - # Daily safety net only — the common path is the per-row trigger - # scheduled at the exact ended_date. self.assertEqual(cron.interval_number, 1) self.assertEqual(cron.interval_type, "days") - # Superuser pin: the unsudo'd searches must see memberships of - # disabled registrants despite the global ir.rule pair. - self.assertEqual(cron.user_id, self.env.ref("base.user_root")) + # Trigger target for exact-time repairs: crossed legs only, so a + # per-departure run stays index-served. + crossed = self.env.ref("spp_registry.cron_repair_crossed_membership_ended_status") + self.assertEqual(crossed.model_id.model, "spp.group.membership") + self.assertTrue(crossed.active) + self.assertIn("_cron_repair_crossed_ended_status", crossed.code) + # Superuser pin on both: the unsudo'd searches must see + # memberships of disabled registrants despite the global ir.rule + # pair. + root = self.env.ref("base.user_root") + self.assertEqual(cron.user_id, root) + self.assertEqual(crossed.user_id, root) From bf34983e16bcfa029b6c8dea4f568c0a6b458a56 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Wed, 2 Sep 2026 10:48:31 +0800 Subject: [PATCH 8/9] docs(registry): apply CI-generated README for round-2 HISTORY changes --- spp_registry/README.rst | 19 +++++++++++++------ spp_registry/static/description/index.html | 19 +++++++++++++------ 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/spp_registry/README.rst b/spp_registry/README.rst index 0a3c888d0..da0eeb46a 100644 --- a/spp_registry/README.rst +++ b/spp_registry/README.rst @@ -148,12 +148,19 @@ Changelog future-dated departure never took effect once the clock crossed it — rosters, metrics, API search and downstream gates kept treating the member as active indefinitely. Writing a future ``ended_date`` now - schedules the repair cron at exactly that moment (staleness window ~1 - minute), and a daily sweep self-heals everything else: rows already - stale in existing databases (drained in committed batches, resuming - across runs until the backlog is gone) and rows written behind the - ORM, including ``is_ended = NULL`` rows that raw-SQL consumers treated - as ended (#417) + schedules a lightweight, index-served repair cron in the minute after + that moment (staleness window ≈1–2 minutes), and a daily sweep + self-heals everything else: rows already stale in existing databases + (drained in committed batches, resuming across runs until the backlog + is gone) and rows written behind the ORM, including + ``is_ended = NULL`` rows that raw-SQL consumers treated as ended + (#417) +- upgrade note: this version adds a partial index on + ``spp_group_membership.ended_date``, built with a write-blocking + ``CREATE INDEX`` during the module upgrade. On a very large registry, + pre-create it concurrently before upgrading and the upgrade will skip + the build: + ``CREATE INDEX CONCURRENTLY IF NOT EXISTS spp_group_membership__ended_date_index ON spp_group_membership (ended_date) WHERE ended_date IS NOT NULL;`` 19.0.2.2.2 ~~~~~~~~~~ diff --git a/spp_registry/static/description/index.html b/spp_registry/static/description/index.html index 0849e997d..f65bf0c23 100644 --- a/spp_registry/static/description/index.html +++ b/spp_registry/static/description/index.html @@ -526,12 +526,19 @@

19.0.2.2.3

future-dated departure never took effect once the clock crossed it — rosters, metrics, API search and downstream gates kept treating the member as active indefinitely. Writing a future ended_date now -schedules the repair cron at exactly that moment (staleness window ~1 -minute), and a daily sweep self-heals everything else: rows already -stale in existing databases (drained in committed batches, resuming -across runs until the backlog is gone) and rows written behind the -ORM, including is_ended = NULL rows that raw-SQL consumers treated -as ended (#417) +schedules a lightweight, index-served repair cron in the minute after +that moment (staleness window ≈1–2 minutes), and a daily sweep +self-heals everything else: rows already stale in existing databases +(drained in committed batches, resuming across runs until the backlog +is gone) and rows written behind the ORM, including +is_ended = NULL rows that raw-SQL consumers treated as ended +(#417) +
  • upgrade note: this version adds a partial index on +spp_group_membership.ended_date, built with a write-blocking +CREATE INDEX during the module upgrade. On a very large registry, +pre-create it concurrently before upgrading and the upgrade will skip +the build: +CREATE INDEX CONCURRENTLY IF NOT EXISTS spp_group_membership__ended_date_index ON spp_group_membership (ended_date) WHERE ended_date IS NOT NULL;
    • From a53e6af1a98394b1daf59b0328199949eeb0a953 Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Wed, 2 Sep 2026 16:05:14 +0700 Subject: [PATCH 9/9] fix(registry): offset the two ended-status crons so they cannot collide Both ir.cron records were given the same nextcall (load time + 1 hour), and both sweep the same two crossed legs with the same `id desc` order. Each ir_cron row is acquired with FOR NO KEY UPDATE SKIP LOCKED, so two cron workers take the two jobs at once and UPDATE the same rows; under Odoo's REPEATABLE READ the run that commits second dies with "could not serialize access due to concurrent update" and is logged as a failure. The window is widest on the first sweep after upgrading a registry with a stale backlog - the case this fix exists to self-heal. The daily sweep now starts two hours after install/upgrade, an hour behind the crossed cron. Both are daily, so the offset holds on every later run. --- spp_registry/data/ir_cron.xml | 12 ++++++++--- .../tests/test_membership_status_cron.py | 20 +++++++++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/spp_registry/data/ir_cron.xml b/spp_registry/data/ir_cron.xml index c87a2b195..5490e21f9 100644 --- a/spp_registry/data/ir_cron.xml +++ b/spp_registry/data/ir_cron.xml @@ -15,11 +15,17 @@ 1 days - + diff --git a/spp_registry/tests/test_membership_status_cron.py b/spp_registry/tests/test_membership_status_cron.py index de7dd034e..0afec4827 100644 --- a/spp_registry/tests/test_membership_status_cron.py +++ b/spp_registry/tests/test_membership_status_cron.py @@ -510,3 +510,23 @@ def test_cron_record_registered(self): root = self.env.ref("base.user_root") self.assertEqual(cron.user_id, root) self.assertEqual(crossed.user_id, root) + + def test_scheduled_sweeps_are_offset(self): + # Both crons sweep the same two crossed legs, and + # `_order = "id desc"` makes their `search(leg, limit=quota)` + # calls return the same rows in the same order. With a shared + # nextcall two cron workers pick the two jobs at once (each + # ir_cron row is taken with FOR NO KEY UPDATE SKIP LOCKED, so + # neither blocks the other) and UPDATE the same ids; under + # Odoo's REPEATABLE READ the run that commits second dies with + # "could not serialize access due to concurrent update" and is + # logged as a failure. The window is widest on the first sweep + # after upgrading a registry with a stale backlog. Their + # scheduled runs must therefore start apart — both being daily, + # the initial offset is preserved on every later run. + daily = self.env.ref("spp_registry.cron_recompute_membership_ended_status") + crossed = self.env.ref("spp_registry.cron_repair_crossed_membership_ended_status") + self.assertGreaterEqual( + abs(daily.nextcall - crossed.nextcall), + timedelta(minutes=30), + )