From a06539dce1049b1d2e4663a89fddc6ce174f19e5 Mon Sep 17 00:00:00 2001 From: Celeo Date: Tue, 21 Jul 2026 20:25:32 -0700 Subject: [PATCH] Diff moodle:sync cohort membership instead of clearing and re-inserting every run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit moodle:sync was clearing and re-inserting every matched user's cohort memberships on every run (~28k writes/pass), regardless of whether anything had changed since the last run three hours earlier. Each cohort write triggers Moodle's enrol_cohort sync, which bumps mdl_course.cacherev. The rating cohorts (OBS, S1-C1, ZZN, ...) all enrol into one course ("Trainee Orientation", id 8), so essentially every cohort insert funnels a cacherev UPDATE onto that single row. Those UPDATEs serialize on the row lock: observed live with 16 concurrent `UPDATE mdl_course ... WHERE id=8` stuck 20-110s and 15 transactions waiting on row locks, with sync throughput collapsing ~28x over a run (20,675 cohort writes in the first hour down to 728 in the fourth). Batching (#112/#113) and a pacing sleep cut the HTTP call count but not the per-write cacherev storm, so runs still degraded and overflowed the withoutOverlapping(120) window. Organic (non-sync) cohort changes run at single digits per hour, so ~99.9% of that churn was rewriting identical data. This diffs each user's desired cohorts against their current membership (read in bulk per chunk via one query against the moodle DB) and only issues the actual adds/removes. The end state is identical to clear-then-reassign — the user ends up in exactly the desired cohorts and no others — but a steady-state run writes almost nothing, so the cacherev row-lock pileup goes away at the source. Indexing on the affected tables is stock-Moodle-correct; the cacherev UPDATE is a primary-key point update, so there was nothing to fix at the index layer. Also stop retrying a failed sub-batch. A write that timed out on our side usually keeps running server-side holding locks; a retry just adds a second writer to the same contended rows and makes the pileup worse. moodle:sync recomputes full desired state every run, so a skipped sub-batch self-heals on the next pass. Roles are still cleared-and-reassigned per user for now (a follow-up can apply the same diff there); this change targets the cohort path, which is the dominant source of the cacherev contention. Co-Authored-By: Claude Opus 4.8 --- app/Classes/VATUSAMoodle.php | 69 +++++++++++ app/Console/Commands/MoodleSync.php | 172 +++++++++++++++++++--------- tests/Unit/MoodleCohortDiffTest.php | 86 ++++++++++++++ 3 files changed, 271 insertions(+), 56 deletions(-) create mode 100644 tests/Unit/MoodleCohortDiffTest.php diff --git a/app/Classes/VATUSAMoodle.php b/app/Classes/VATUSAMoodle.php index 70a56a6..82e5713 100644 --- a/app/Classes/VATUSAMoodle.php +++ b/app/Classes/VATUSAMoodle.php @@ -59,6 +59,9 @@ class VATUSAMoodle extends MoodleRest /** @var array Memoized results of getCoursesInCategory(), keyed by category id */ private $coursesInCategoryCache = []; + /** @var array|null Memoized cohort idnumber => id map for the lifetime of this instance */ + private $cohortIdMapCache = null; + /** @var int Seconds to bound each Moodle HTTP request to */ private const HTTP_TIMEOUT_SECONDS = 30; @@ -479,6 +482,72 @@ public function assignCohort(int $uid, string $cnumber) ]); } + /** + * Build a cohort idnumber => id map in a single query, memoized for this instance. + * Lets the sync diff a user's desired cohorts (expressed as idnumbers) against their + * current memberships (stored as cohort ids) without an HTTP round-trip per cohort. + * + * @return array + */ + public function getCohortIdMap(): array + { + if ($this->cohortIdMapCache === null) { + $this->cohortIdMapCache = DB::connection('moodle')->table('cohort') + ->whereNotNull('idnumber') + ->where('idnumber', '!=', '') + ->pluck('id', 'idnumber') + ->map(fn ($id) => (int) $id) + ->toArray(); + } + + return $this->cohortIdMapCache; + } + + /** + * Read current cohort memberships for a set of Moodle user ids in a single query, + * so a bulk sync can diff desired vs. current state without a per-user round-trip. + * + * @param int[] $uids Moodle user ids + * + * @return array userid => list of cohort ids they currently belong to + */ + public function getCohortMembershipsForUsers(array $uids): array + { + if (empty($uids)) { + return []; + } + + $map = []; + DB::connection('moodle')->table('cohort_members') + ->whereIn('userid', $uids) + ->get(['userid', 'cohortid']) + ->each(function ($row) use (&$map) { + $map[(int) $row->userid][] = (int) $row->cohortid; + }); + + return $map; + } + + /** + * Remove many cohort memberships in a single request. Routed through the Web Service + * (not a direct DB delete) so Moodle's enrol_cohort sync unenrols the user from the + * cohort's linked courses properly. + * + * @param array $items Each: ['uid' => int, 'cohortid' => int] + * + * @return mixed + * @throws \Exception + */ + public function removeCohortsBulk(array $items) + { + return $this->request("core_cohort_delete_cohort_members", [ + "members" => array_values(array_map(fn ($item) => [ + "cohortid" => $item['cohortid'], + "userid" => $item['uid'] + ], $items)) + ], self::METHOD_POST); + } + /** * Assign many users to Cohorts in a single request. * diff --git a/app/Console/Commands/MoodleSync.php b/app/Console/Commands/MoodleSync.php index 9c97a0a..44b3223 100644 --- a/app/Console/Commands/MoodleSync.php +++ b/app/Console/Commands/MoodleSync.php @@ -35,9 +35,6 @@ class MoodleSync extends Command * HTTP_TIMEOUT_SECONDS bound on dense chunks. Flush in smaller sub-batches. */ private const FLUSH_BATCH_SIZE = 200; - /** @var int Attempts per sub-batch before giving up on it and moving on */ - private const MAX_ATTEMPTS = 2; - /** @var int Microseconds to sleep between successive bulk sub-batch calls. Moodle's * cohort/role writes trigger its own course cache-invalidation (mdl_course.cacherev), * which was observed serializing against itself under back-to-back calls and @@ -71,31 +68,47 @@ public function handle() return 0; } - $items = $this->computeSyncItems($user, $this->resolveMoodleId($user)); - $this->flushItems($items); + $id = $this->resolveMoodleId($user); + $items = $this->computeSyncItems($user, $id); + $this->flushItems($id, $items); return 0; } //Syncronize Users $moodleIds = $this->moodle->getAllUserIdMap(); + $cohortIdMap = $this->moodle->getCohortIdMap(); logger()->info("moodle:sync starting bulk pass: {$moodleIds->count()} Moodle-linked users"); $chunkNum = 0; - $totals = ['users' => 0, 'updates' => 0, 'cohorts' => 0, 'roles' => 0, 'enrolments' => 0, 'failedBatches' => 0]; + $totals = ['users' => 0, 'cohortAdds' => 0, 'cohortRemoves' => 0, 'roles' => 0, + 'enrolments' => 0, 'failedBatches' => 0]; User::with('visits', 'academyCompetencies.course', 'roles')->chunk(1000, - function ($users) use ($moodleIds, &$chunkNum, &$totals) { + function ($users) use ($moodleIds, $cohortIdMap, &$chunkNum, &$totals) { $chunkNum++; - $updates = $cohorts = $roles = $enrolments = []; + // Resolve this chunk's users to their Moodle ids up front, then read all + // their current cohort memberships in one query so we can diff desired vs. + // current and only write actual changes (see diffCohorts()). + $chunkUsers = []; foreach ($users as $user) { - if (!$moodleIds->has($user->cid)) { - continue; + if ($moodleIds->has($user->cid)) { + $chunkUsers[(int) $moodleIds[$user->cid]] = $user; } - $items = $this->computeSyncItems($user, (int) $moodleIds[$user->cid]); + } + $currentCohorts = $this->moodle->getCohortMembershipsForUsers(array_keys($chunkUsers)); + + $updates = $cohortAdds = $cohortRemoves = $roles = $enrolments = []; + foreach ($chunkUsers as $id => $user) { + $items = $this->computeSyncItems($user, $id); $updates[] = $items['update']; - $cohorts = array_merge($cohorts, $items['cohorts']); + + $diff = $this->diffCohorts($id, $items['cohortIdnumbers'], + $currentCohorts[$id] ?? [], $cohortIdMap); + $cohortAdds = array_merge($cohortAdds, $diff['adds']); + $cohortRemoves = array_merge($cohortRemoves, $diff['removes']); + $roles = array_merge($roles, $items['roles']); $enrolments = array_merge($enrolments, $items['enrolments']); } @@ -104,7 +117,10 @@ function ($users) use ($moodleIds, &$chunkNum, &$totals) { $failed += $this->flushBulk($chunkNum, 'update', $updates, fn ($items) => $this->moodle->updateUsersBulk($items)); usleep(self::FLUSH_PACING_USEC); - $failed += $this->flushBulk($chunkNum, 'cohorts', $cohorts, + $failed += $this->flushBulk($chunkNum, 'cohort-removes', $cohortRemoves, + fn ($items) => $this->moodle->removeCohortsBulk($items)); + usleep(self::FLUSH_PACING_USEC); + $failed += $this->flushBulk($chunkNum, 'cohort-adds', $cohortAdds, fn ($items) => $this->moodle->assignCohortsBulk($items)); usleep(self::FLUSH_PACING_USEC); $failed += $this->flushBulk($chunkNum, 'roles', $roles, @@ -113,21 +129,21 @@ function ($users) use ($moodleIds, &$chunkNum, &$totals) { $failed += $this->flushBulk($chunkNum, 'enrolments', $enrolments, fn ($items) => $this->moodle->enrolUsersBulk($items)); - $totals['users'] += count($users); - $totals['updates'] += count($updates); - $totals['cohorts'] += count($cohorts); + $totals['users'] += count($chunkUsers); + $totals['cohortAdds'] += count($cohortAdds); + $totals['cohortRemoves'] += count($cohortRemoves); $totals['roles'] += count($roles); $totals['enrolments'] += count($enrolments); $totals['failedBatches'] += $failed; - logger()->info("moodle:sync chunk {$chunkNum}: " . count($users) . " users, " - . count($updates) . " updates, " . count($cohorts) . " cohorts, " + logger()->info("moodle:sync chunk {$chunkNum}: " . count($chunkUsers) . " users, " + . count($cohortAdds) . " cohort adds, " . count($cohortRemoves) . " cohort removes, " . count($roles) . " roles, " . count($enrolments) . " enrolments" . ($failed > 0 ? ", {$failed} sub-batches failed" : "")); }); logger()->info("moodle:sync finished bulk pass: {$totals['users']} users seen, " - . "{$totals['updates']} updates, {$totals['cohorts']} cohorts, " + . "{$totals['cohortAdds']} cohort adds, {$totals['cohortRemoves']} cohort removes, " . "{$totals['roles']} roles, {$totals['enrolments']} enrolments across {$chunkNum} chunks, " . "{$totals['failedBatches']} sub-batches permanently failed"); @@ -154,17 +170,20 @@ private function resolveMoodleId(User $user): int /** * Flush a bulk call for a chunk, split into FLUSH_BATCH_SIZE-sized sub-batches (a * 1000-user chunk can accumulate several thousand cohort/role entries, too large for - * Moodle to process within the 30s HTTP timeout in one call). Each sub-batch gets - * MAX_ATTEMPTS tries; a sub-batch that still fails is logged and skipped rather than - * aborting the rest of the chunk or the run — the next scheduled run will pick up - * anything missed, since moodle:sync recomputes full state every time. + * Moodle to process within the 30s HTTP timeout in one call). + * + * A failed sub-batch is logged and skipped — deliberately NOT retried. A write that + * timed out on our side usually keeps running server-side, holding row locks (e.g. the + * mdl_course.cacherev bump every cohort/role write triggers); firing a retry just piles + * a second writer onto the same contended rows and makes the pileup worse. moodle:sync + * recomputes full desired state every run, so anything skipped self-heals next run. * * @param int $chunkNum * @param string $kind * @param array $items * @param callable $call * - * @return int Number of sub-batches that failed after all attempts + * @return int Number of sub-batches that failed */ private function flushBulk(int $chunkNum, string $kind, array $items, callable $call): int { @@ -175,21 +194,12 @@ private function flushBulk(int $chunkNum, string $kind, array $items, callable $ usleep(self::FLUSH_PACING_USEC); } - for ($attempt = 1; $attempt <= self::MAX_ATTEMPTS; $attempt++) { - try { - $call($batch); - continue 2; - } catch (\Exception $e) { - if ($attempt < self::MAX_ATTEMPTS) { - logger()->warning("moodle:sync chunk {$chunkNum} {$kind} batch {$batchNum}: " - . "attempt {$attempt} failed ({$e->getMessage()}), retrying"); - usleep(self::FLUSH_PACING_USEC); - continue; - } - logger()->error("moodle:sync chunk {$chunkNum} {$kind} batch {$batchNum}: " - . "failed after {$attempt} attempts, skipping: {$e->getMessage()}"); - $failures++; - } + try { + $call($batch); + } catch (\Exception $e) { + logger()->error("moodle:sync chunk {$chunkNum} {$kind} batch {$batchNum}: " + . "failed, skipping (will retry next run): {$e->getMessage()}"); + $failures++; } } @@ -197,40 +207,90 @@ private function flushBulk(int $chunkNum, string $kind, array $items, callable $ } /** - * Flush a single user's computed items immediately (single-user CLI path). Still routed - * through flushBulk() for consistent retry behavior, though a single user's item counts - * never approach FLUSH_BATCH_SIZE. + * Diff a user's desired cohort set (idnumbers) against their current memberships + * (cohort ids), so we only add memberships that are missing and remove ones that no + * longer apply — instead of clearing and re-inserting every cohort every run. The old + * clear-then-reassign produced the same end state but rewrote ~all memberships every + * pass, and each write triggers a mdl_course.cacherev bump that serializes on a tiny + * hot table; diffing collapses steady-state writes to near zero. + * + * End state is identical to the previous clear-then-reassign: the user ends up in + * exactly the desired cohorts and no others. + * + * @param int $id Moodle user id + * @param string[] $desiredIdnumbers Desired cohort idnumbers (may repeat) + * @param int[] $currentCohortIds Cohort ids the user currently belongs to + * @param array $cohortIdMap idnumber => cohort id + * + * @return array{adds: array, removes: array} + */ + private function diffCohorts(int $id, array $desiredIdnumbers, array $currentCohortIds, array $cohortIdMap): array + { + // Resolve desired idnumbers to cohort ids, skipping any that don't exist in Moodle + // (the sync never creates cohorts, so an unknown idnumber was a no-op add before too). + $desiredIds = []; + foreach (array_unique($desiredIdnumbers) as $idnumber) { + if (isset($cohortIdMap[$idnumber])) { + $desiredIds[$cohortIdMap[$idnumber]] = $idnumber; + } + } + + $adds = $removes = []; + foreach ($desiredIds as $cohortId => $idnumber) { + if (!in_array($cohortId, $currentCohortIds, true)) { + $adds[] = ['uid' => $id, 'cnumber' => $idnumber]; + } + } + foreach ($currentCohortIds as $cohortId) { + if (!isset($desiredIds[$cohortId])) { + $removes[] = ['uid' => $id, 'cohortid' => $cohortId]; + } + } + + return ['adds' => $adds, 'removes' => $removes]; + } + + /** + * Flush a single user's computed items immediately (single-user CLI path). Reads the + * one user's current cohort memberships to run the same diff the bulk path uses; item + * counts never approach FLUSH_BATCH_SIZE here. * + * @param int $id Moodle user id * @param array $items */ - private function flushItems(array $items) + private function flushItems(int $id, array $items) { + $current = $this->moodle->getCohortMembershipsForUsers([$id])[$id] ?? []; + $diff = $this->diffCohorts($id, $items['cohortIdnumbers'], $current, $this->moodle->getCohortIdMap()); + $this->flushBulk(0, 'update', [$items['update']], fn ($items) => $this->moodle->updateUsersBulk($items)); - $this->flushBulk(0, 'cohorts', $items['cohorts'], fn ($items) => $this->moodle->assignCohortsBulk($items)); + $this->flushBulk(0, 'cohort-removes', $diff['removes'], + fn ($items) => $this->moodle->removeCohortsBulk($items)); + $this->flushBulk(0, 'cohort-adds', $diff['adds'], fn ($items) => $this->moodle->assignCohortsBulk($items)); $this->flushBulk(0, 'roles', $items['roles'], fn ($items) => $this->moodle->assignRolesBulk($items)); $this->flushBulk(0, 'enrolments', $items['enrolments'], fn ($items) => $this->moodle->enrolUsersBulk($items)); } /** - * Compute what needs to happen in Moodle for a user — cohorts, roles, and enrolments — - * without making any HTTP calls itself. Callers accumulate these across many users and - * flush them as bulk calls (whole-table sync), or flush a single user's items - * immediately (single-user CLI path). Cohort/role clearing stays per-user since it's a - * direct DB delete (cheap, not the HTTP bottleneck this split targets). + * Compute what needs to happen in Moodle for a user — desired cohorts, roles, and + * enrolments — without making any HTTP calls itself. Callers accumulate these across + * many users and flush them as bulk calls (whole-table sync), or flush a single user's + * items immediately (single-user CLI path). Cohorts are returned as desired idnumbers + * and diffed against current membership by the caller (see diffCohorts()); roles are + * still cleared per-user here via a direct DB delete and fully reassigned. * * @param \App\User $user * @param int $id Moodle user id * - * @return array{update: array, cohorts: array, roles: array, enrolments: array} + * @return array{update: array, cohortIdnumbers: string[], roles: array, enrolments: array} * @throws \Exception */ private function computeSyncItems(User $user, int $id): array { $facilities = $user->visits->pluck('facility')->merge(collect($user->facility))->unique(); - //Assign Cohorts - $this->moodle->clearUserCohorts($id); + //Desired Cohorts (diffed against current membership by the caller — no unconditional clear) $cohorts = []; $cohorts[] = Helper::ratingShortFromInt($user->rating); //VATUSA level rating if ($user->flag_homecontroller) { @@ -330,10 +390,10 @@ private function computeSyncItems(User $user, int $id): array }*/ return [ - 'update' => ['id' => $id, 'fname' => $user->fname, 'lname' => $user->lname, 'email' => $user->email], - 'cohorts' => array_map(fn ($cnumber) => ['uid' => $id, 'cnumber' => $cnumber], $cohorts), - 'roles' => $roles, - 'enrolments' => $enrolments, + 'update' => ['id' => $id, 'fname' => $user->fname, 'lname' => $user->lname, 'email' => $user->email], + 'cohortIdnumbers' => $cohorts, + 'roles' => $roles, + 'enrolments' => $enrolments, ]; } } diff --git a/tests/Unit/MoodleCohortDiffTest.php b/tests/Unit/MoodleCohortDiffTest.php new file mode 100644 index 0000000..b7c145c --- /dev/null +++ b/tests/Unit/MoodleCohortDiffTest.php @@ -0,0 +1,86 @@ +moodle. + $moodle = $this->createMock(VATUSAMoodle::class); + $command = new MoodleSync($moodle); + + $method = new ReflectionMethod($command, 'diffCohorts'); + $method->setAccessible(true); + + return $method->invoke($command, $id, $desired, $current, $map); + } + + public function test_steady_state_emits_no_writes(): void + { + // User already in exactly the cohorts they should be in -> nothing to do. + $diff = $this->diff(42, ['S1', 'ZDV'], [5, 10], ['S1' => 5, 'ZDV' => 10]); + + $this->assertSame([], $diff['adds']); + $this->assertSame([], $diff['removes']); + } + + public function test_missing_membership_is_added(): void + { + $diff = $this->diff(42, ['S1', 'ZDV'], [5], ['S1' => 5, 'ZDV' => 10]); + + $this->assertSame([['uid' => 42, 'cnumber' => 'ZDV']], $diff['adds']); + $this->assertSame([], $diff['removes']); + } + + public function test_stale_membership_is_removed(): void + { + // User is in cohort 99, which is no longer desired -> remove it. + $diff = $this->diff(42, ['S1'], [5, 99], ['S1' => 5, 'ZDV' => 10]); + + $this->assertSame([], $diff['adds']); + $this->assertSame([['uid' => 42, 'cohortid' => 99]], $diff['removes']); + } + + public function test_add_and_remove_together(): void + { + $diff = $this->diff(42, ['S1', 'ZDV'], [10, 99], ['S1' => 5, 'ZDV' => 10]); + + $this->assertSame([['uid' => 42, 'cnumber' => 'S1']], $diff['adds']); + $this->assertSame([['uid' => 42, 'cohortid' => 99]], $diff['removes']); + } + + public function test_unknown_desired_idnumber_is_skipped_not_added(): void + { + // A desired cohort that does not exist in Moodle can't be added (sync never creates + // cohorts); it must not appear as an add, and must not spuriously remove anything. + $diff = $this->diff(42, ['S1', 'NOPE'], [5], ['S1' => 5]); + + $this->assertSame([], $diff['adds']); + $this->assertSame([], $diff['removes']); + } + + public function test_duplicate_desired_idnumbers_dedupe(): void + { + // computeSyncItems can list the same cohort twice (e.g. facility rating); a user + // not yet in it should still only be added once. + $diff = $this->diff(42, ['ZDV', 'ZDV'], [], ['ZDV' => 10]); + + $this->assertSame([['uid' => 42, 'cnumber' => 'ZDV']], $diff['adds']); + $this->assertSame([], $diff['removes']); + } +}