fix(registry): cron-repair stored membership status/is_ended that go stale with the clock

[gonzalesedwin1123]

Fixes #417.

Problem

spp.group.membership.status and is_ended are store=True computes that depend only on ended_date and compare it against fields.Datetime.now(). A recompute fires on a write to ended_date, never when the clock crosses it — so a departure recorded ahead of time (a future-dated ended_date) stays stored as active / is_ended = False indefinitely once the date passes. Every consumer inherits the staleness: rosters, metrics, API search, and downstream gates keep treating the departed member as current. See #417 for the full consumer inventory.

Fix

Option (1) from the issue, as ranked there: keep the fields stored and add an hourly ir.cron in spp_registry (_cron_recompute_ended_status, private so it is not RPC-callable) that finds rows whose stored values disagree with the clock and re-triggers both computes through the normal ORM path (modified(["ended_date"])).

• Keeps the fields searchable and preserves all four raw-SQL consumers of the is_ended column.
• Searches with active_test=False so memberships archived by the UI onchange are repaired too.
• Checks both directions (ended-but-stored-active and future/no-end-but-stored-inactive) and both fields, so rows drifted by direct SQL or imports are also caught.
• Batched: at most batch_size (default 10,000) rows per direction per run, so a large first-run backlog cannot exceed the cron time limit; repaired rows drop out of the domains, so subsequent hourly runs drain the remainder (with a log line when a backlog remains). ended_date, which both sweep domains filter on, is now indexed.
• Explicitly re-invalidates cached group metrics for the repaired memberships: the recompute flushes through low-level SQL and bypasses this model's write() override, so the metric-invalidation funnel would otherwise never fire (only the two target computes depend on ended_date in ORM terms, but that hook is a manual, non-ORM dependency).
• The first run self-heals any rows already stale in existing databases — no migration script needed (the ended_date index is likewise created automatically on upgrade).
• Staleness is now bounded by the cron interval (1 hour). Security-sensitive call sites that need exactness at read time should additionally evaluate the window in the query (issue option 3); the reporter already does this on their side.

Out of scope (per issue discussion)

• active has a related inconsistency: the UI onchange archives a membership when a past ended_date is entered, but nothing archives it when the clock crosses a future one. Left deliberately untouched (and documented in the cron's docstring) — archiving changes record visibility everywhere. Tracked in spp.group.membership: archiving via active is inconsistent — set only by a UI onchange, never when the clock crosses ended_date #420.
• start_date is ignored by both computes (a membership starting in 2099 is "active" today) — a semantics change affecting ~30 consumers, better handled separately. Tracked in spp.group.membership: status/is_ended ignore start_date — a membership starting in the future counts as active today #421.

Testing

TDD: the new TestMembershipEndedStatusCron (7 tests) reproduces the production state per the issue's recipe — aging rows behind the ORM's back with raw SQL — and each round was confirmed red before its implementation. Coverage includes raw-SQL column assertions (the raw-SQL is_ended consumers never see the ORM cache), an over-match guard asserting the no-op case selects zero rows, metric-funnel invalidation, batch-size behavior, archived rows keeping active = False, and the registered cron's interval. Full spp_registry suite: 252 passed, 0 failed, 0 errors. Pre-commit hooks pass on the changed files.

Note: README.rst/index.html regeneration is taken verbatim from CI's pinned generator (already applied), not generated locally.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.68%. Comparing base (0820667) to head (c327b32).

Additional details and impacted files

@@            Coverage Diff             @@
##             19.0     #418      +/-   ##
==========================================
+ Coverage   72.24%   72.68%   +0.43%     
==========================================
  Files         419      569     +150     
  Lines       29813    39217    +9404     
==========================================
+ Hits        21539    28503    +6964     
- Misses       8274    10714    +2440     

Flag	Coverage Δ
spp_analytics	93.25% <ø> (ø)
spp_api_v2	79.99% <ø> (?)
spp_api_v2_change_request	66.53% <ø> (ø)
spp_api_v2_cycles	71.03% <ø> (ø)
spp_api_v2_data	77.77% <ø> (ø)
spp_api_v2_entitlements	70.23% <ø> (ø)
spp_api_v2_gis	71.57% <ø> (ø)
spp_api_v2_products	65.86% <ø> (?)
spp_api_v2_programs	92.22% <ø> (ø)
spp_api_v2_service_points	71.03% <ø> (ø)
spp_api_v2_simulation	71.19% <ø> (?)
spp_api_v2_vocabulary	57.75% <ø> (?)
spp_approval	50.34% <ø> (?)
spp_area	80.16% <ø> (?)
spp_area_hdx	81.60% <ø> (?)
spp_audit	72.13% <ø> (?)
spp_base_common	91.07% <ø> (ø)
spp_programs	65.27% <ø> (ø)
spp_registry	87.44% <100.00%> (+0.29%)	⬆️
spp_security	69.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
spp_registry/models/group_membership.py	83.72% <100.00%> (+1.98%)	⬆️

... and 151 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[gonzalesedwin1123]

Applied findings from an internal expert review (commit c327b32):

• Metric invalidation: the compute flush goes through low-level SQL and bypasses this model's write() override, so the cron now calls _invalidate_group_metrics() explicitly for the repaired memberships' groups — otherwise cached household metrics would have kept counting departed members, which is part of what this PR sets out to fix.
• Private entry point: renamed to _cron_recompute_ended_status so the method is not RPC-callable, matching the repo's cron naming pattern. (Done now rather than later because the noupdate="1" cron code string freezes at install.)
• Batching: each run repairs at most batch_size (default 10,000) rows per direction, so a large first-run backlog can't blow the cron time limit; repaired rows drop out of the domains and subsequent hourly runs drain the remainder. Also added index=True on ended_date, which both sweep domains filter on.
• Stronger tests (5 → 7): raw-SQL column assertions (the four raw-SQL is_ended consumers never see the ORM cache), an over-match guard asserting the no-op case selects zero rows, metric-funnel invalidation, batch-size behavior, archived rows keep active = False, and the cron interval.

Full spp_registry suite: 252 passed, 0 failed, 0 errors.

[kneckinator]

Backlog draining / fault isolation. When more than batch_size rows are stale (first run on an existing DB, or a cohort-wide ended_date crossing the clock in the same hour), each hourly run repairs at most 10k per direction in one transaction: a 200k backlog takes ~20 hours to drain, and one contended row (REPEATABLE READ serialization failure on any of the up-to-20k rows) rolls back the whole run — repeated failures will auto-deactivate the cron.

Odoo 19's cron progress API addresses both: loop in chunks and call self.env["ir.cron"]._commit_progress(len(chunk), remaining=...) — a partially-done job is rescheduled ASAP instead of waiting an hour, and each chunk commits independently. That would also make HISTORY's "first run self-heals" claim hold for large backlogs, and it replaces this backlog heuristic — which as written logs a false positive when a run drains exactly batch_size rows, and silently disables the LIMIT if a caller ever passes batch_size=0.

[kneckinator]

This bump is stale against 19.0: HEAD already ships spp_registry 19.0.2.2.2 (HISTORY has 2.2.1 and 2.2.2 entries newer than this branch's merge-base), so 19.0.2.1.5 is a version regression, and the regenerated README.rst/index.html on this branch drop main's 2.2.x changelog entries. Needs a rebase, renumber to 19.0.2.2.3, the HISTORY fragment repositioned above 2.2.2, and README regeneration on the rebased base.

[kneckinator]

("is_ended", "=", True) compiles to is_ended IS TRUE, which excludes NULL — so a row whose is_ended is NULL (raw INSERT/ETL that omitted the column; it is nullable with no SQL default) is never repaired by this direction, while the to_end direction does catch NULL (= False compiles to IS NOT TRUE). NULL misbehaves exactly like the drift this sweep exists for: the raw-SQL consumers (NOT is_ended / is_ended = false) treat NULL rows as ended. Worth either a NULL-repair leg here, or making the column NOT NULL DEFAULT false so the state cannot exist.

[kneckinator]

Steady-state cost: this leg's date predicate (ended_date IS NULL OR > now) matches nearly every live row and the discriminating columns are unindexed, so each hourly run does a full-table read (EXPLAIN on a 1M-row mirror: backward pkey scan, ~80MB — and the same holds for the to_end direction once a registry has many historical departures; the new ended_date index cannot serve either domain as written because of the OR structure). Two cheap options: split each direction into per-leg conjunctive searches so (partial) indexes can serve them, and/or run this reactivate leg — which only guards non-ORM drift — daily rather than hourly.

[kneckinator]

index=True btree-indexes every NULL ended_date (the open-membership majority). btree_not_null skips the NULLs, serves every real query on this column at least as well (<= now here; != False AND >= since in group_service), and avoids the NULL-entry write amplification on every membership insert.

Suggested change

	ended_date = fields.Datetime(index=True)
	ended_date = fields.Datetime(index="btree_not_null")

[kneckinator]

These assertions run against the whole spp_group_membership table (the cron searches unscoped with active_test=False): assertFalse(repaired) here, assertEqual(repaired, rec) at L63/L89, the exact 2/1 counts at L154-157, and funnel.assert_called_once() at L292. That is latent flakiness: any committed install-time/demo row whose ended_date crosses now between install and the post_install run breaks them (none exists today, but ci-full runs these tests on demo-seeded DBs). Scoped shapes preserve the over-match intent: assertIn(rec, repaired) at 63/89, and here self.assertFalse(repaired & (open_ended | already_ended)).

[kneckinator]

status and is_ended are two stored encodings of the same predicate, which is why every domain here needs a second OR-leg — and the pair genuinely can disagree: the two computes each call fields.Datetime.now() at different instants, so a write in that sub-second window can store an inconsistent pair (which these two-leg domains then self-heal, so the leg should not simply be dropped as-is). The deeper fix, fine as a follow-up: make _compute_status @api.depends("is_ended") and derive "inactive" if rec.is_ended else "active" — the pair can then never disagree, the race disappears, and these domains shrink to their is_ended leg (which a partial index can serve).

[kneckinator]

These two domains are the 4th and 5th in-model spelling of the "ended at time T" predicate (_compute_is_ended, _compute_status, _onchange_ended_date), and external consumers already re-roll it in three inconsistent variants. A small model-level helper (_is_ended_at(now) plus _ended_domain(now)/_not_ended_domain(now)) used by the computes, the onchange and these searches would give #421 (start_date semantics) one place to change instead of five.

[kneckinator]

Non-blocking refinement: the exact transition time is known when ended_date is written, and Odoo 19 crons can be pointed at it — self.env.ref("spp_registry.cron_recompute_membership_ended_status")._trigger(at=ended_date) from create/write when a future end is set (~6 lines; triggers persist across restarts, and a stale trigger just runs the idempotent sweep). That shrinks the staleness window from up to an hour to about a minute and would let this periodic sweep drop to a daily safety net.

[kneckinator]

test_metric_invalidation.py already ships this exact patch as _patch_invalidate_funnel(env) (L37-48) — importing it (this file already imports from a sibling test module) drops the hand-rolled patch.object and the unittest.mock import.

[kneckinator]

Nothing these tests exercise reads start_date (both computes, both cron domains and the raw-SQL readers key off ended_date; the start/end constraint is ORM-only and bypassed by the raw UPDATE), yet it is a required positional — so four call sites repeat now - timedelta(days=730), now - timedelta(days=365) plus a throwaway now local, and L83 re-passes the value the row already has. _age_row(rec, ended_date=None, active=True) defaulting to a year ago (deriving start_date = ended_date - timedelta(days=365) inside, to keep the row consistent) collapses the call sites to _age_row(rec) / _age_row(rec, future) / _age_row(rec, active=False).

[kneckinator]

Worth extending this note with the two write-path side effects that do differ: the 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 membership.write_uid), and the repair is invisible to spp_audit write-rules (they hook write(), so a UI write of ended_date is logged but the cron's status flip is not). Both are acceptable — just worth documenting here.

[kneckinator]

The performance principles doc referenced from AGENTS.md caps batch processing at 5,000 records per chunk; this defaults to 10,000 per direction (up to 20k rows marked in one transaction). If that principle is meant to govern crons, batch_size=5000 is the one-token fix (the cron XML passes no argument and the tests pin their own values); if not, feel free to ignore.

[kneckinator]

This paragraph now exists in four places (here, the method docstring, HISTORY.md, and the test module docstring) — and #421 is queued to change exactly these semantics. Suggest keeping the method docstring as the one home and shrinking this comment (and the test docstring's overlap) to a pointer, e.g. Repairs stored status/is_ended once the clock passes ended_date; see _cron_recompute_ended_status (#417).

[kneckinator]

Hardening nit: with no user_id, the cron runs as the data-load default (OdooBot, superuser) — which is what makes the unsudo'd searches immune to the two global disabled-registrant ir.rules on this model. If an operator ever reassigns the Scheduler User to a non-superuser, memberships of disabled registrants silently stop being repaired. Pinning <field name="user_id" ref="base.user_root"/> (as the spp_dci crons do) makes the assumption explicit.

[kneckinator]

Thanks for the PR @gonzalesedwin1123
It is currently in conflict with 19.0 - please merge/rebase.

I left a couple of comments - please take a look. 🙏