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

❌ Patch coverage is 99.09091% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 76.31%. Comparing base (87fa05f) to head (bf34983).
⚠️ Report is 7 commits behind head on 19.0.

Files with missing lines	Patch %	Lines
spp_registry/models/group_membership.py	99.09%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             19.0     #418      +/-   ##
==========================================
+ Coverage   76.26%   76.31%   +0.05%     
==========================================
  Files         662      698      +36     
  Lines       44223    46572    +2349     
==========================================
+ Hits        33726    35542    +1816     
- Misses      10497    11030     +533     

Flag	Coverage Δ
spp_analytics	93.25% <ø> (ø)
spp_api_v2	79.99% <ø> (ø)
spp_api_v2_change_request	73.37% <ø> (ø)
spp_api_v2_cycles	71.03% <ø> (ø)
spp_api_v2_data	77.77% <ø> (ø)
spp_api_v2_entitlements	70.23% <ø> (ø)
spp_api_v2_gis	74.60% <ø> (ø)
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.85% <ø> (ø)
spp_area	80.16% <ø> (?)
spp_area_hdx	81.60% <ø> (?)
spp_audit	72.13% <ø> (?)
spp_base_common	91.07% <ø> (ø)
spp_programs	67.58% <ø> (ø)
spp_registry	88.94% <99.09%> (+1.15%)	⬆️
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	89.71% <99.09%> (+7.98%)	⬆️

... and 36 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.

[gonzalesedwin1123]

Adopted. The sweep now loops in batch_size chunks, each committed via ir.cron._commit_progress (with remaining set so a run that exhausts the cron time budget is rescheduled ASAP), so a backlog drains across immediately-rescheduled runs instead of one hourly batch per direction, and a serialization failure rolls back only its own chunk. The == batch_size heuristic and its false positive are gone, and batch_size < 1 is now rejected instead of silently dropping the LIMIT. The resume path is pinned by test_cron_resumes_after_time_budget_exhausted.

[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.

[gonzalesedwin1123]

Done — rebased onto 19.0, renumbered to 19.0.2.2.3, HISTORY fragment repositioned above 2.2.2, and README.rst/index.html regenerated on the rebased base by CI's pinned generator.

[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.

[gonzalesedwin1123]

Fixed in the cron — and it turned out worse than a missed domain leg: the ORM cannot repair NULL→False, because the cache reads NULL back as False, so a recompute writes nothing. The sweep now opens with a bounded, parameterized SQL leg (_repair_null_is_ended, LIMIT-batched with progress commits) that sets is_ended = false where it is NULL and the row should read active, with invalidation + the metric funnel; NULL rows whose ended_date has passed are already repaired by the ORM legs (computed True ≠ cached False — pinned by test_cron_repairs_null_columns_with_past_ended_date). We considered NOT NULL DEFAULT false, but keeping the constraint requires required=True (Odoo's update_db_notnull drops NOT NULL for non-required fields on every update), which felt heavier than the drift this exists to absorb.

[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.

[gonzalesedwin1123]

Both suggestions taken: the directions are now six conjunctive per-leg searches (no ORs, so the date-bound legs are servable by the ended_date index; the two ended_date IS NULL legs can't use that partial index, but they only guard non-ORM drift and are expected to match nothing — noted in the code, with partial indexes / a last-swept watermark as the escalation path if that ever matters), and the periodic sweep runs daily instead of hourly, with the common path moved to exact-time triggers (see the ir_cron.xml thread). Leg searches also stop once a chunk is full, so a large backlog in one direction no longer re-scans the other legs on every pass.

[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")

[gonzalesedwin1123]

Applied.

[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)).

[gonzalesedwin1123]

Applied the scoped shapes — assertIn at the single-row sites, the set-difference assert in test_cron_leaves_correct_rows_untouched, the funnel assertion now checks the group across all calls, and the batch test asserts internally-consistent chunk math against the rows it created rather than absolute table-wide counts.

[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).

[gonzalesedwin1123]

Agreed, and deferred as you suggest — folded into #421's scope (comment added there), since it changes the same compute surface that issue is queued to rework. The single home of the predicate is now _is_ended_as_of, which gives that change one place to land.

[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.

[gonzalesedwin1123]

Added _is_ended_as_of(ended_date, now) as the single home of the predicate (both computes + the archiving onchange) and _stale_ended_status_domains(now) for the sweep's domains, with a pointer to the external raw-SQL consumers for #421.

[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.

[gonzalesedwin1123]

Adopted — create/write now _trigger this cron at every future ended_date written (_schedule_ended_status_repair; times rounded up to the next full minute — the cron's own precision — so bursts sharing a minute collapse into one trigger, and create reads the dates back from the records so default_ended_date context fills are covered too). The periodic sweep dropped to a daily safety net, with its first nextcall deferred an hour past install/upgrade. Staleness for ORM-written departures shrinks from ≤1 h to ~1 minute.

[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.

[gonzalesedwin1123]

Applied.

[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).

[gonzalesedwin1123]

Applied the suggested shape — _age_row(rec, ended_date=None, active=True) defaults to a departure a year ago and derives start_date internally.

[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.

[gonzalesedwin1123]

Documented both in the cron docstring — the write_uid/write_date stamping (including the API changed_by example) and the invisibility to spp_audit write-rules — along with why both are accepted.

[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.

[gonzalesedwin1123]

Applied — the default is now 5000 (it's a chunk size under the _commit_progress loop), with a pointer to the principle doc. Side note for a separate docs change: that doc's "no commit in loops" rule deserves an explicit carve-out for ir.cron._commit_progress.

[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).

[gonzalesedwin1123]

Done — the method docstring is the canonical home; this XML comment and the test-module docstring are pointers now. HISTORY keeps its own prose since a changelog entry is a record rather than a reference.

[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.

[gonzalesedwin1123]

Applied — user_id pinned to base.user_root with the reasoning in a comment beside it, and test_cron_repairs_memberships_of_disabled_registrants now pins the behaviour itself: an officer-run sweep misses a disabled registrant's stale membership, the root-run sweep repairs it.

[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. 🙏

[gonzalesedwin1123]

Thanks for the thorough review @kneckinator — every comment checked out and all fifteen are addressed (per-thread replies inline). Highlights of the resulting shape:

• Rebased on 19.0, spp_registry renumbered to 19.0.2.2.3, README regenerated by CI's pinned generator.
• Exact-time repair: writes of a future ended_date now _trigger the cron at that moment (minute-rounded), so ORM-written departures go stale for ~1 minute instead of up to an hour. The periodic sweep drops to a daily safety net.
• Chunked, committed sweep via Odoo 19's ir.cron._commit_progress: fault isolation per chunk, ASAP rescheduling when the time budget runs out, and conjunctive per-leg domains with an early-exit fill.
• is_ended = NULL turned out unrepairable through the ORM for the open-membership case (the cache reads NULL as False, so a recompute writes nothing) — a bounded SQL leg in the sweep handles it.
• The one deferral, as suggested: deriving status from is_ended is folded into spp.group.membership: status/is_ended ignore start_date — a membership starting in the future counts as active today #421 (commented there).

Beyond the review comments, an adversarial pass we ran on the result also hardened: batched NULL repair, a final remaining=0 report so evenly-divided backlogs don't reschedule a wasted run, default_ended_date context coverage on create, and behavioural tests for the time-budget resume path and the base.user_root pin (officer-run sweep misses a disabled registrant's row, root-run sweep repairs it).

CI is green including the regenerated README. Ready for another look.

[kneckinator]

drained = len(chunk) < batch_size is not a reliable "backlog is empty" signal, because the six legs overlap and chunk |= ... de-duplicates.

Concrete: batch_size=5000. Leg 1 (ended_date <= now AND is_ended = false) matches 3000 rows -> chunk = 3000. Leg 2 (ended_date <= now AND status != 'inactive') actually matches 8000 rows, but gets limit=2000, and with _order = "id desc" those 2000 are mostly rows already in chunk. The union lands at ~3500 < 5000, so drained=True, _commit_progress(..., remaining=0) runs, _run_job records FULLY_DONE, and _reschedule_later puts the next sweep a full day out with ~4500 rows still stale.

It self-heals on the following daily run (leg 1 is empty by then, so leg 2 gets the whole quota), but the run reports "drained" when it isn't - which is exactly the property the PR description claims ("resuming across runs until the backlog is gone"). Track exhaustion per leg instead of inferring it from the union size:

chunk = memberships.browse()
exhausted = True
for leg in self._stale_ended_status_domains(now):
    quota = batch_size - len(chunk)
    if quota <= 0:
        exhausted = False
        break
    found = memberships.search(leg, limit=quota)
    if len(found) == quota:
        exhausted = False   # the leg may have more
    chunk |= found
...
drained = exhausted

[gonzalesedwin1123]

Adopted your per-leg exhaustion tracking verbatim — drained is now exhausted, cleared whenever a leg returns exactly its quota (or the quota runs out before every leg was consulted), so an overlap-shrunk union can no longer end the run with remaining=0. Pinned by test_overlapping_legs_do_not_end_run_early, which builds your exact shape — a leg-1 backlog whose leg-2 quota only re-finds chunk rows, with status-only-stale rows behind it — and asserts one run drains all of it with the final report drained.

[kneckinator]

remaining is left at its default None here, so _commit_progress computes remaining = max(progress.remaining - processed, 0), which is 0 on the first call (ir.cron.progress.remaining defaults to 0). That means this break - the "time budget exhausted, NULL rows still left" path - reports no work remaining.

That matters because a NULL-is_ended row with ended_date IS NULL and status = 'active' matches none of the six ORM legs. So after this break, the main loop finds chunk empty, takes the if repaired_ids: branch, calls _commit_progress(0, remaining=0), and _run_job sees (True, done>0, 0) -> FULLY_DONE -> _reschedule_later. A large NULL population then drains one batch_size chunk per day instead of getting the ASAP reschedule the ORM loop correctly asks for with remaining=1.

Mirror the ORM loop: pass remaining=1 on this break and remaining=0 on the len(ids) < batch_size break above.

[gonzalesedwin1123]

Fixed — the NULL loop now reports explicitly on every commit: remaining=1 after each full batch (so a budget-exhausted break is continued ASAP rather than parked a day on a phantom remaining=0) and remaining=0 on the short final batch. It also returns the reported time budget so the caller skips the ORM passes entirely when time is already gone, instead of burning over-budget time on six searches first. Pinned by test_null_repair_stops_when_time_budget_exhausted, which asserts the exact (processed, remaining) pairs.

[kneckinator]

WHERE is_ended IS NULL has no index behind it, so Postgres must sequentially scan the whole spp_group_membership table to prove there is nothing to repair. The docstring's reasoning ("Expected to match nothing on a healthy database") inverts the cost model: the no-match case is precisely the one that pays the full scan, on every single run. The two ("ended_date", "=", False) legs in _stale_ended_status_domains have the same problem - the new btree_not_null partial index deliberately excludes NULLs - so a healthy run costs three full table scans.

And this is not once a day. _schedule_ended_status_repair files a trigger for every future ended_date written, so on a registry that records departures ahead of time the sweep can run many times a day, each time re-scanning the entire membership table. On a multi-million-row table this becomes the dominant cost of the whole feature.

Worth either a partial index (CREATE INDEX ... ON spp_group_membership (id) WHERE is_ended IS NULL, plus equivalents for the ended_date IS NULL legs), or moving the NULL-repair leg out of the trigger-driven path entirely - a clock-crossing trigger can never produce a NULL, so only the periodic sweep needs it.

[gonzalesedwin1123]

Took your second option, extended to all three unindexed probes: the cron is split in two. Writes of a future ended_date now trigger a new _cron_repair_crossed_ended_status, which sweeps only the two index-served crossed legs — the only stale states the mere passage of time can produce — so per-departure runs never touch a full-scan probe. The NULL repair and the two ended_date IS NULL legs moved to the daily _cron_recompute_ended_status safety net, restoring the originally-costed three-scans-per-day. test_crossed_cron_repairs_only_index_served_legs pins the split.

[kneckinator]

env.ref defaults to raise_if_not_found=True, and the cron record is declared noupdate="1" - so if an admin deletes it from Settings -> Technical -> Scheduled Actions, or a partial data load leaves it missing, it is never recreated. From that point on every create/write of a membership with a future ended_date raises ValueError: External ID not found, i.e. a hard failure on the core registry write path (spp_change_request_v2 strategies, spp_api_v2 update_member, the UI form) in exchange for what is only a latency optimisation.

Suggested change

	if at_list:
	self.env.ref("spp_registry.cron_recompute_membership_ended_status")._trigger(at=at_list)
	cron = self.env.ref("spp_registry.cron_recompute_membership_ended_status", raise_if_not_found=False)
	if at_list and cron:
	cron._trigger(at=at_list)

[gonzalesedwin1123]

Applied as suggested (raise_if_not_found=False plus guard), pointed at the new trigger cron — a deleted cron record now degrades to the daily sweep instead of breaking membership writes. Pinned by test_missing_trigger_cron_degrades_to_daily_sweep.

[kneckinator]

A minute-aligned ended_date gets its trigger at exactly ended_date, with zero margin - and that is the common shape, not the edge case: spp_api_v2/services/group_service.py builds datetime.combine(ended_date, time.min), and the datetime widget writes second=0.

Zero margin matters because the two sides use different clocks. _get_ready_sql_condition and _clear_schedule both use cr.now() (the PostgreSQL clock), and _clear_schedule deletes the trigger before _run_job executes the body - which then evaluates ended_date <= fields.Datetime.now() against the application server clock. If the DB clock leads the app clock, the trigger is consumed while the predicate is still false, nothing is repaired, and the trigger is gone; the row then waits for the daily sweep, i.e. up to 24h of exactly the staleness this PR is fixing.

Rounding up unconditionally costs at most 60s of extra latency and removes the race:

Suggested change

	if ended.second or ended.microsecond:
	ended = ended.replace(second=0, microsecond=0) + timedelta(minutes=1)

[gonzalesedwin1123]

Applied — the round-up is now unconditional, so a minute-aligned ended_date (the group_service/widget common shape, as you say) gets its trigger one minute after the moment rather than at it. Test expectations updated accordingly.

[kneckinator]

_trigger unconditionally INSERTs an ir.cron.trigger row - the set de-duplicates only within a single call. Across calls nothing dedups, and nothing removes a stale trigger when ended_date is later moved or cleared.

Two consequences worth weighing:

• A cohort exit written one membership at a time (the spp_change_request_v2 strategies, spp_api_v2 remove_member/update_member) with a shared future date produces one trigger row per membership, all with the same call_at. They all fire together and each one drives a full-table sweep (see the seq-scan note on _repair_null_is_ended).
• _gc_cron_triggers only collects triggers whose cron is inactive, so an orphaned future trigger (departure entered, then cancelled) survives until its call_at and then costs one more pointless sweep.

Bounded, and the docstring is right that each one is individually harmless - but the aggregate is a sweep-per-departure rather than a sweep-per-minute. A search_count guard on (cron_id, call_at) before triggering, or reusing the existing trigger, would collapse them.

[gonzalesedwin1123]

Adopted the reuse option: _schedule_ended_status_repair now searches pending triggers for the same (cron, call_at) and files only the missing minutes, so a row-per-call cohort exit produces one trigger (test_same_minute_departures_share_one_trigger). Two concurrent transactions can still race to a duplicate, and an orphaned trigger still survives to its moment — both documented as harmless, since with the cron split each spurious firing is now just two index probes.

[kneckinator]

The six legs share one batch_size quota and are consumed strictly in order, so as long as leg 1 has at least batch_size matches, legs 2-6 never execute at all.

That starves the other directions: a future-dated membership wrongly stored as status = 'inactive' (leg 6) stays wrong until the entire leg-1 backlog is drained. On a first sweep of a large registry that is many runs, and once _reschedule_later takes over those runs are a day apart. Dividing the quota across legs (or rotating the leg order per pass) would keep every direction making progress.

[gonzalesedwin1123]

Adopted rotation — each pass starts from a different leg (offset = pass_no % len(legs)), so within any multi-pass run every direction gets the full quota. With the per-leg exhaustion and explicit-remaining fixes on the other threads, the cross-run gap is also an ASAP continuation rather than a day, so a first-sweep backlog delays the other directions by passes, not sweeps.

[kneckinator]

_cron_recompute_ended_status's docstring says the repair "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)". That holds for the ORM modified()/flush path, but not for this raw UPDATE, which sets only is_ended.

So rows repaired by the NULL leg keep their old write_date/write_uid, and any write_date-based incremental sync or export - including the changed_by the docstring names - will never see the change. Either add write_date = now() AT TIME ZONE 'UTC', write_uid = %s to the SET, or scope the docstring claim to the ORM legs so the next reader isn't misled.

[gonzalesedwin1123]

Went with the first option — the raw UPDATE now stamps write_date/write_uid alongside is_ended, so write_date-keyed syncs and the API's changed_by see the NULL repair too, and the docstring claim is now true for both paths. Pinned in test_cron_repairs_null_is_ended_row (an aged write_date must move forward and write_uid must be the running user).

[kneckinator]

Convention note: AGENTS.md's Quick Checklist says "No cr.commit() in loops - use queue_job". ir.cron._commit_progress ends in self.env.cr.commit(), and it is called from inside while True: here and again at line 311.

_commit_progress is Odoo 19's sanctioned batching API and is very likely the right call for a cron sweep, so I'm not asking you to change it - but it is a deliberate deviation from the repo's own checklist and from the queue_job pattern docs/principles/performance-scalability.md prescribes, and nothing in the PR calls it out. A sentence in the docstring (or an AGENTS.md amendment) would save the next reviewer from re-deriving it.

[gonzalesedwin1123]

Documented in the cron docstring: _commit_progress is Odoo 19's sanctioned batching API for cron work and the deliberate exception to the checklist's "no cr.commit() in loops" rule, which targets ad-hoc commits. The AGENTS.md / performance-principles carve-out itself goes in the separate docs change already noted on the batch-size thread, to keep this PR's scope clean.

[kneckinator]

Adding index= makes Odoo issue a plain CREATE INDEX (not CONCURRENTLY) during _auto_init, which takes a SHARE lock on spp_group_membership and blocks all writes for the duration of the build.

spp_registry is a Layer-1 foundation module, so this runs on every production -u spp_registry. On a registry with millions of memberships that turns a routine module upgrade into a multi-minute write outage. Worth a line in the upgrade notes (or pre-creating the index CONCURRENTLY ahead of the upgrade) so operators aren't surprised.

[gonzalesedwin1123]

Added an upgrade note to the 19.0.2.2.3 HISTORY entry with the exact statement — CREATE INDEX CONCURRENTLY IF NOT EXISTS spp_group_membership__ended_date_index ON spp_group_membership (ended_date) WHERE ended_date IS NOT NULL; — name and definition matched to what _auto_init would build, so the upgrade's create_index sees the existing name and skips the blocking build.

[kneckinator]

Coverage gap: _repair_null_is_ended's batching machinery is never exercised. Both NULL tests set up exactly one row, so with the default batch_size=5000 the loop takes the len(ids) < batch_size break on its first pass - the while True continuation, the _commit_progress break, and the accumulate-across-batches path are all uncovered. That is also why the missing remaining signal I flagged on group_membership.py:311 isn't caught by the suite.

A batch_size=1 test over two NULL rows, asserting both rows are repaired and the (processed, remaining) pairs _run_cron records, would pin the loop - it's the same shape as test_cron_drains_backlog_in_batches, just for the SQL leg.

[gonzalesedwin1123]

Added both: test_null_repair_drains_in_batches (two NULL rows, batch_size=1) pins the loop continuation, the per-batch (processed, remaining) reports and the drained close, and test_null_repair_stops_when_time_budget_exhausted pins the budget break — the exact path that hid the remaining bug you flagged on the model.

[gonzalesedwin1123]

@kneckinator round 2 addressed in f31fd28 — all 11 comments adopted, no pushback on any of them (each claim checked against the Odoo 19 source first). The headline change is structural, per your seq-scan comment: the sweep is now two crons — writes of a future ended_date trigger a lightweight _cron_repair_crossed_ended_status that runs only the two index-served crossed legs, while the daily _cron_recompute_ended_status keeps the full-scan NULL-drift probes as the safety net. On top of that: per-leg exhaustion tracking (no more false "drained" from the de-duplicated union), explicit remaining reporting in the NULL loop (plus it hands its time budget back so the ORM passes are skipped when it's spent), per-pass leg rotation, write_date/write_uid stamping in the raw repair, a guarded env.ref on the write path, unconditional minute round-up for triggers, trigger reuse per (cron, minute), the AGENTS.md deviation documented in the docstring, an upgrade note with the exact CREATE INDEX CONCURRENTLY statement, and the two NULL-batching tests (6 new tests, 3 updated). Suite green locally: 0 failed / 0 errors of 278.