# D-8 — Options comparison: cleanup (B+E) vs. accept-time mint (D) **Date:** August 5, 2026 **For:** Andrew and Caleb **Status:** decision memo. No code written for either option. **Companion:** `GROUP_ID_AT_ACCEPT_BRIEF.md` is the full implementation spec for **D**. This memo argues about whether to build it. --- ## Bottom line **Andrew's instinct is right, and the numbers back it.** The cleanup approach is roughly **a quarter of the work**, touches **none** of the 29 `Invitation.group_id` sites, introduces **no** concurrency changes, and fails in a way the admins already know how to handle. D is the more elegant model and the more dangerous change. **But B + E as stated does not close all four routes.** It closes two. The package that actually solves the problem is **CF10 + B + E**, where CF10 is a six-line move that Caleb's proposal was going to make unnecessary. That is still ~7-9 hours against D's ~25-30. **One correction to the framing:** **E is not a peer of B — it is one of B's call sites.** B is "clear the group when nobody is left in it"; E is that same check, run from the Delete Invitation button. Treating them as two options makes the work look bigger than it is. --- ## 1. What each option actually is | | | |---|---| | **B** | When a season group has one live member and no outstanding invitations, clear it. Runs *after* whatever event reduced the count. | | **E** | The Delete Invitation button runs that check. **This is B, called from one place.** | | **CF10** | `invite()` mints the `group_id` *before* validating the invitee. Move the mint below the checks. Six lines. | | **D** | Never mint at invite time at all; mint on the first accept. Changes what the field means. | | **CF11** | A "Disband Group" button for a solo manager. Manual escape hatch. | --- ## 2. Does B + E close the four routes? Honestly: no, two of four Route numbering is `SESSION_HANDOFF.md`'s. | # | How a player gets stranded | B fires? | E fires? | Closed by | |---|---|---|---|---| | 1 | Invite fails validation; `group_id` already saved | **no** — the group never *dropped* to one, it was born there | **no** — there is no invitation to delete | **CF10** | | 2 | Invitee declines, or accepts a rival group | **yes, if the decline paths call the check** | no | **B** | | 3 | Invitee never answers; inviter deletes the invitation | — | **yes** | **E** (= B) | | 4 | Group had members; all left or were removed | **yes** | — | **B** | **Route 1 is the gap.** A player who mistypes an email address is stranded, and nothing in B or E ever fires for them. CF10 closes it and is genuinely trivial — the block already exists, it just sits above the validation instead of below it. **It was only "superseded" because D removed the mint entirely.** **So the package is CF10 + B + E.** --- ## 3. Where B has to be called — the real number The plan's original objection to B said "six call sites". **The true count for the season flow is eleven**, verified by grepping every place a season invitation is declined or deleted, plus every place group membership shrinks: | # | Site | Route | |---|---|---| | 1 | `GroupController::deleteInvitation()` :1352 | 3 — *this is E* | | 2 | `GroupController::declineInvitation()` :1639 | 2a — invitee declines | | 3 | `GroupController::acceptSeasonInvitation()` :1453 — "you are already in a group", declines this one | 2 | | 4 | `GroupController::acceptSeasonInvitation()` :1496 — declines rivals **(loop)** | 2b | | 5 | `RegistrationController::acceptSeasonInvitationDirectly()` :519 — declines rivals **(loop)** | 2b | | 6 | `OrderCompleteSubscriber` :1040 — declines rivals, season arm **(loop)** | 2b | | 7 | `GroupInvitationsForm::declineOtherInvitations()` :1496 — season branch **(loop)** | 2b | | 8 | `GroupInvitationsForm::submitForm()` ~:1037 — admin per-row decline | 2c | | 9 | `GroupController::removeMember()` season arm ~:1830 | 4 | | 10 | `GroupController::leaveGroup()` ~:1983 | 4 | | 11 | `CancelRegistrationForm::submitForm()` — a *member* cancels, group drops to one | 4 | **The four marked (loop) are the awkward ones.** They decline *other people's* invitations, so the cleanup has to run for **the inviter of each declined invitation**, not for the current user. Roughly five lines each rather than one. **But the logic itself is one function**, ~15 lines, reusing CF8's existing `GroupDissolveService`: ```php // GroupMembershipService (or on GroupDissolveService directly). public function clearGroupIfOrphaned(int $season_id, ?string $group_id): bool { if (empty($group_id)) { return FALSE; } // Live members, D-4 definition (exclude cancelled + expired). if ($this->getLiveMemberCount($season_id, $group_id) > 1) { return FALSE; } // The carve-out that stops this dissolving a group out from under a manager // who is waiting on a slow invitee. Trivial here because pending invitations // still carry group_id — which is the whole point of not doing D. $pending = $this->entityTypeManager->getStorage('ccsoccer_invitation') ->loadByProperties(['group_id' => $group_id, 'status' => 'pending']); if (!empty($pending)) { return FALSE; } $this->groupDissolve->dissolveGroup($group_id, [ 'notify' => FALSE, // one member, and they caused this 'reason' => 'group emptied (D-8/B)', ]); return TRUE; } ``` **Note how short the pending-invitation carve-out is.** Under D that same predicate becomes *"invitations whose inviter is a live member of this group"* — a new service method, a new query shape, and an `IN ()` empty-array trap. Under B it is one `loadByProperties()` call. That contrast is most of the cost difference between the two options. --- ## 4. Side by side | | **CF10 + B + E** | **D (+ E + CF11)** | |---|---|---| | **Effort** | **~7-9 hrs** incl. testing | **~25-30 hrs** | | **`Invitation.group_id` sites touched** | **0 of 29** | **~20 of 29 re-keyed** | | **New service methods** | 1 (`clearGroupIfOrphaned`) | 6, incl. a locked mint | | **Concurrency** | **unchanged** — no new races | **3 new races**, incl. two found only in review | | **Row locking on the payment path** | **none** | **required** (`SELECT … FOR UPDATE`) | | **Template changes** | **none** | 2 | | **`getGroupSize()`** | untouched | rewritten; cap arithmetic changes | | **Admin Group Invitations page** | **unchanged** | **lost** for pre-acceptance invitations | | **Roster Builder display** | **unchanged** | phantom groups disappear | | **Rollback** | **code-only** | **needs a data step** — in-flight invitations break | | **Open questions before coding** | ~2 | **12** | | **Blockers found in adversarial review** | n/a | **6**, two of them inside the first round of fixes | | **Closes route 1** | via CF10 | at the source | | **Closes routes 2, 3** | via B | at the source | | **Closes route 4** | via B | needs CF11 | | **Field means "2+ agreed to be a group"** | **no** | **yes** | | **Failure mode if it's wrong** | a stranded solo group — *the status quo* | manager-less groups; ungrouped players **after payment** | --- ## 5. Pros and cons ### CF10 + B + E **Pros** - **Roughly a quarter of the work**, and most of it is in one file. - **`Invitation.group_id` is not touched.** All 29 read/write/query sites keep working exactly as they do now. No re-key, no `inviter IN members`, no `IN ()` empty-array traps, no NULL-handling audit. - **No concurrency change at all.** The mint stays where it is — one user, one request. B runs *after* a state change and is idempotent; two members leaving simultaneously produces at worst a *missed* cleanup, never a corrupt one. - **Builds on code that already exists.** `GroupDissolveService` (CF8) does the clearing; `LiveRegistrationTrait` (CF4) defines "live". Nothing new to design. - **Nothing the admins see changes.** Same Roster Builder, same Group Invitations page, same displays. Given the goal is *"fewer admin headaches"*, changing nothing they look at is a feature. - **Rollback is `git revert`.** No data step, no in-flight invitations to repair. - **It solves the actual reported scenario.** P1 invites P2; P3 invites P2; P2 accepts P1. The accept auto-declines P3's invitation (already true today), and B then clears P3's group. **P3 is freed with no button and no admin.** That is the case the whole cluster started from. - **Failure is visible and already-handled.** If a call site is missed, someone ends up stranded — which is today's behaviour, and admins have Dissolve. **Cons** - **Eleven call sites.** This is the real objection and it should not be waved away. The check is one function, so this is a risk of *omission* (a future code path forgets to call it), not of *drift* — but omission is still how bugs get in. - **Four of the eleven are loops** over rival declines, and have to resolve *someone else's* group. They are the fiddliest part and the easiest to get subtly wrong. - **The invariant is maintained, not guaranteed.** A solo group can exist between the event and the cleanup, and permanently if a site is missed. Anyone writing a future group query still has to think *"this might be one person"*. - **It does not make the field mean anything new.** `group_id` still means *"somebody clicked Invite"*. Caleb's stated reason for D — making the field self-describing — is not achieved. - **Needs a periodic audit to stay honest.** §12.4 query A in the brief already detects stranded solo groups; under B it should be run occasionally rather than never. ### D **Pros** - **Closes routes 1, 2 and 3 at the source.** No cleanup logic to maintain, no call sites to miss. - **The field becomes self-describing** — *two or more people agreed to be a group* — so the next person writing a group query cannot get it wrong. - **The invariant is checkable** with a one-line SQL assertion, not inferred from having called a helper everywhere. - **It removes state rather than managing it.** Genuinely the better model. **Cons** - **~20 of 29 sites re-key**, including two payment-adjacent paths. - **Three new concurrency races**, two of which were found only on the second adversarial review pass. The mitigation needs `SELECT … FOR UPDATE` inside Commerce's order transaction, where Drupal's own `lock` service does not work. **`forUpdate()` is a no-op on SQLite, so tests can pass and prove nothing.** - **Six blockers in review, two of them inside the fixes for the first four.** The concurrency design was wrong three times running. That is a signal about how hard this is to get right, not about the review. - **New failure modes nobody has a playbook for** — a manager-less group, or a player ungrouped after paying. Compare to B's failure mode, which is the status quo. - **The admin loses the Group Invitations page** for any invitation that has not been accepted yet. - **Rollback needs a data step.** Every in-flight invitation has a NULL `group_id` that the reverted accept paths will happily write onto acceptors. - **12 open questions**, four of which block writing code, one of which (Q10) is genuinely unresolved and sits on the money path. - **Route 4 still needs CF11 anyway**, so D does not remove the button, it just makes it rarer. --- ## 6. The two arguments that actually decide it ### 6.1 The failure modes are not comparable If **B** misses a call site: a player ends up in a group of one. That is **exactly what happens today**, it is visible, admins recognise it, and Dissolve fixes it. The downside of getting B wrong is *not having fixed the bug in that one path*. If **D**'s locking is wrong: two players end up in groups neither of them manages and neither can leave, or a player who has paid is silently left ungrouped. Those are **new** states. Nobody has seen them, nothing detects them today, and one of them happens after money has changed hands. **D replaces a known, tolerable, visible failure with an unknown one on the payment path.** For a ~200-player casual league where the stated goal is fewer admin tickets, that trade is hard to justify. ### 6.2 "Eleven call sites" is not the failure shape this codebase has been bitten by The plan's objection to B — *"six chances to miss one, which is the duplication shape this codebase keeps getting bitten by"* — is worth re-examining, because it conflates two different things. What has actually bitten this project is **duplicated logic that drifted**: `reset()` written out 21 times (CF4), status filters written out ten times (CF3), four hand-rolled copies of "clear three fields". Each copy was written independently and they diverged. **Eleven calls to one shared function is a different risk.** They cannot drift — there is one implementation. The risk is that a *new* code path forgets to call it. That is real, but it is: - **cheaper to detect** — one SQL query counts stranded groups; - **cheaper to fix** — add one line; - **not silent** — the player notices and emails an admin, which is the current process. **D's risk is the opposite: cheap to write, expensive to detect.** A race that fires once a season produces a corrupt group that nobody attributes to the deploy. --- ## 7. What you give up by not doing D Being fair to Caleb's proposal, because it is the better *model*: 1. **`group_id` keeps its vague meaning.** A future developer must know that a group can be one person with an outstanding invitation. 2. **The cleanup is forever.** Every new path that can reduce group membership has to remember the call. That is a small permanent tax. 3. **A solo group is still a real state**, so the Roster Builder still draws a group container for someone mid-invitation. *(Arguably useful — the admin can see who is trying to form a group.)* **None of these cost an admin ticket.** They are conceptual debt, not operational load. Given the stated goal — *let players manage their own groups without admin action* — CF10 + B + E achieves it and D achieves it slightly more elegantly for four times the work and materially more risk. --- ## 8. A variant worth ten minutes of Caleb's time: B via entity hook B's only real weakness is the eleven call sites. There is a way to collapse them to **one**: `hook_ccsoccer_invitation_update()` and `hook_ccsoccer_registration_update()` in `ccsoccer.module` — whenever an invitation leaves `pending`, or a registration's `group_id`/`status` changes, run `clearGroupIfOrphaned()` on the affected group. **For:** impossible to miss a call site, including in code written next year. One place to reason about. **Against:** - **Recursion** — the dissolve saves registrations, which fires the hook again. Needs a static guard. - **Ordering inside transactions** — the hook fires inside the entity save, so a checkout-time decline triggers cleanup mid-order-transaction. - **This codebase has already got a hook wrong**: `ccsoccer_registration_delete()` is misnamed, has **never fired**, and would throw if it did (recorded in the plan's Aug 4 audit). - Harder to trace when debugging than an explicit call. **Recommendation: explicit calls first.** Ship the eleven, get the behaviour right, and consider the hook later as a safety net if a missed site actually shows up. Explicit is easier to review, and this change wants to be easy to review. --- ## 9. Cost | | | |---|---| | `clearGroupIfOrphaned()` + reuse of `GroupDissolveService` | ~1 hr | | Seven simple call sites | ~1 hr | | Four rival-decline loops (resolve the *other* inviter) | ~2 hrs | | CF10 (move the mint below the checks; delete the stray comment) | ~0.5 hr | | LOCAL testing — 4 routes × the paths that reach them, plus the carve-out | ~2-3 hrs | | **Total** | **~7-9 hrs** | **CF11 (Disband) — optional, ~2-3 hrs.** Under this package route 4 clears automatically, so the button stops being the fix. It is still decent insurance for exactly B's weakness: if a call site *is* missed, the player unsticks themselves instead of emailing an admin. **Recommend: build it last, or not at all, and decide after the audit query has been run once.** --- ## 10. What to take to Caleb Not *"we're not doing your idea"* — the honest framing is narrower than that: 1. **His diagnosis was right.** The mint at invite time is the root cause, and D is the correct model. 2. **The implementation cost is four times what it looked like** — 29 sites, not seven, and the `getGroupSize()` rework he flagged was the *smallest* of the problems. The concurrency work was not visible from the original write-up and is the real cost. 3. **Two adversarial review passes found six blockers**, including two inside the fixes for the first four. `GROUP_ID_AT_ACCEPT_BRIEF.md` Appendix C has the log — it is worth him reading even if D is shelved, because it documents where this part of the codebase is sharp. 4. **CF10 recovers most of D's value for six lines.** It closes route 1 at the source, which is the one route cleanup genuinely cannot reach. 5. **D stays on the table.** The brief is written, verified and reviewed. If group churn grows or the cleanup calls start getting missed, it is ready to build. --- ## 11. Open questions for this option **Q-B1 — should the pending-invitation carve-out count *all* pending invitations for the group, or only the manager's?** Members can send invitations (`invite()` checks registration ownership, not manager-ness — the manager-only rule lives in Twig). Counting all of them is safer and matches `getGroupSize()`'s existing behaviour. **Recommend: all.** **Q-B2 — when a group drops to one, should the survivor be notified?** The brief's B call sites pass `notify => FALSE` because the survivor usually caused the event. But route 2b is different: P3's group is cleared because *P2 accepted someone else*, which P3 did not do and may not notice. **Recommend: no notification for routes 3 and 4, and reuse the existing "invitation declined" notification for route 2 — P3 already gets told that P2 declined.** Worth confirming that notification fires on an *auto*-decline, not just a manual one. **Q-B3 — build CF11 or not?** See §9. Recommend deciding after the audit query has run once. **Q-B4 — should `groups_locked` block the accept paths that ignore it?** Three of four accept paths do not check it (`GROUP_ID_AT_ACCEPT_BRIEF.md` §5.7). **This is a pre-existing bug and B does not make it worse** — unlike D, which turns it into "a new group can appear after the roster is locked". So it is no longer urgent, but it is still wrong. **Recommend: fix separately, not in this change.** --- ## 12. Recommendation **Build CF10 + B + E. Hold D.** Ship order: 1. ~~**The plan §10.8 testing backlog first.**~~ **✅ DONE — §10.8 passed on LOCAL, Aug 5.** It was a hard prerequisite either way, because B calls `GroupDissolveService` from **thirteen** places (not the eleven estimated here — review later found two more, both in the admin surfaces). 2. **Run `GROUP_ID_AT_ACCEPT_BRIEF.md` §12.4 query A** — read-only, two minutes. It tells you how many stranded groups exist today, which sizes the problem and settles Q-B3. 3. **CF10** — six lines, closes route 1, independently revertable. 4. **B + E** — one helper, eleven calls. 5. **CF11 only if the audit says route 4 is common.** `GROUP_ID_AT_ACCEPT_BRIEF.md` stays in the repo as the record of what D would take. Mark it **HELD — see D8_OPTIONS_COMPARISON.md** at the top rather than deleting it; the research in §4 and Appendix A is accurate and useful regardless of which option ships.