From b04d1f583f9dc1d9574ae10e2204d7d69fa0468f Mon Sep 17 00:00:00 2001 From: Celeo Date: Mon, 20 Jul 2026 20:11:43 -0700 Subject: [PATCH] Fix moodle:sync stacking by cutting per-user Moodle HTTP call volume PR #111 memoized getCategories() and added a 30s per-call HTTP timeout, on the theory that a category-fetch N+1 was what pushed a single moodle:sync run past its 120-minute withoutOverlapping() lock. Deployed 2026-07-19T21:29Z; two days later the api-worker pod still had 6 concurrent moodle:sync processes stacked ~3h apart (elapsed 17h47m down to 2h47m), so that fix did not address the dominant cost. Root cause: MoodleSync::handle() walks the *entire* VATUSA user table and makes one core_user_get_users HTTP call per user just to check "is this CID in Moodle?" - the large majority return "not found", so this existence check alone is tens of thousands of sequential round-trips per run. Every user who *is* found in Moodle then triggers several more one-item-at-a-time write calls (cohorts, roles, enrolments), with staff users triggering dozens via a nested category/course enrolment loop. Per api/CLAUDE.md, the 120-minute withoutOverlapping() window on moodle:sync is deliberately short (commit a324474) so an orphaned lock self-heals in ~1-2h instead of Laravel's 24h default - widening it would undo that, so this fix instead cuts real runtime back under the window using Moodle's existing bulk Web Service endpoints. - VATUSAMoodle::getAllUserIdMap() replaces the per-user getUserId() HTTP existence check with a single query against the Moodle DB connection this class already uses elsewhere, building a full CID -> Moodle-id map up front. MoodleSync::handle() now does an in-memory lookup against that map instead of an HTTP call per VATUSA user, and passes the known id into sync() so its own redundant second getUserId() call is skipped too. The single-user `moodle:sync {cid}` CLI path is unchanged (still create-or-update via getUserId()). - VATUSAMoodle gains assignCohortsBulk()/assignRolesBulk()/ enrolUsersBulk(), thin wrappers that send the same core_cohort_add_cohort_members/core_role_assign_roles/ enrol_manual_enrol_users calls with a full array of items instead of a single-element array. The existing single-item methods are left untouched (other callers depend on them). MoodleSync::sync() now collects each user's cohort/role/enrolment items and flushes each as one bulk call at the end, instead of one HTTP round-trip per item. No changes to Kernel.php scheduling, withoutOverlapping() values, or any existing VATUSAMoodle method signature. Co-authored-by: Claude Sonnet 5 --- app/Classes/VATUSAMoodle.php | 80 ++++++++++++++++++++++ app/Console/Commands/MoodleSync.php | 102 ++++++++++++++++++++-------- tests/Unit/MoodleUserIdMapTest.php | 46 +++++++++++++ 3 files changed, 200 insertions(+), 28 deletions(-) create mode 100644 tests/Unit/MoodleUserIdMapTest.php diff --git a/app/Classes/VATUSAMoodle.php b/app/Classes/VATUSAMoodle.php index 24a7e85..60d5c71 100644 --- a/app/Classes/VATUSAMoodle.php +++ b/app/Classes/VATUSAMoodle.php @@ -326,6 +326,21 @@ public function getCidFromUserId(int $uid): ?int return $user->idnumber ?? null; } + /** + * Build a full CID -> Moodle user id map in a single query, to replace a + * whole-table per-user HTTP existence check with an in-memory lookup. + * + * @return \Illuminate\Support\Collection + */ + public function getAllUserIdMap(): \Illuminate\Support\Collection + { + return DB::connection('moodle')->table('user') + ->where('deleted', 0) + ->whereNotNull('idnumber') + ->where('idnumber', '!=', '') + ->pluck('id', 'idnumber'); + } + /** * Create user. * @@ -441,6 +456,30 @@ public function assignCohort(int $uid, string $cnumber) ]); } + /** + * Assign many users to Cohorts in a single request. + * + * @param array $items Each: ['uid' => int, 'cnumber' => string] + * + * @return mixed + * @throws \Exception + */ + public function assignCohortsBulk(array $items) + { + return $this->request("core_cohort_add_cohort_members", [ + "members" => array_values(array_map(fn ($item) => [ + "cohorttype" => [ + 'type' => 'idnumber', + 'value' => $item['cnumber'] + ], + "usertype" => [ + 'type' => 'id', + 'value' => $item['uid'] + ] + ], $items)) + ]); + } + /** * Unassign Cohort * @@ -491,6 +530,26 @@ public function assignRole(int $uid, ?int $cid, string $role, string $context) ]); } + /** + * Assign many Roles in a single request. + * + * @param array $items Each: ['uid' => int, 'cid' => int|null, 'role' => string, 'context' => string] + * + * @return mixed + * @throws \Exception + */ + public function assignRolesBulk(array $items) + { + return $this->request("core_role_assign_roles", [ + "assignments" => array_values(array_map(fn ($item) => [ + "roleid" => $this->roleIds[$item['role']], + "userid" => $item['uid'], + "contextid" => $item['cid'], + "contextlevel" => $item['context'] + ], $items)) + ]); + } + /** * * Remove Role from User in Context @@ -677,6 +736,27 @@ function enrolUser( ["enrolments" => [0 => ["roleid" => $rid, "userid" => $uid, "courseid" => $cid]]]); } + /** + * Enrol many users in courses in a single request. + * + * @param array $items Each: ['uid' => int, 'cid' => int, 'rid' => int|null] + * + * @return mixed + * @throws \Exception + */ + public + function enrolUsersBulk( + array $items + ) { + return $this->request("enrol_manual_enrol_users", [ + "enrolments" => array_values(array_map(fn ($item) => [ + "roleid" => $item['rid'] ?? $this->roleIds['STU'], + "userid" => $item['uid'], + "courseid" => $item['cid'] + ], $items)) + ]); + } + /** * Unenrol User from Course * diff --git a/app/Console/Commands/MoodleSync.php b/app/Console/Commands/MoodleSync.php index 32233dc..4eb963c 100644 --- a/app/Console/Commands/MoodleSync.php +++ b/app/Console/Commands/MoodleSync.php @@ -62,10 +62,11 @@ public function handle() } //Syncronize Users - User::with('visits', 'academyCompetencies.course', 'roles')->chunk(1000, function ($users) { + $moodleIds = $this->moodle->getAllUserIdMap(); + User::with('visits', 'academyCompetencies.course', 'roles')->chunk(1000, function ($users) use ($moodleIds) { foreach ($users as $user) { - if ($this->moodle->getUserId($user->cid)) { - $this->sync($user); + if ($moodleIds->has($user->cid)) { + $this->sync($user, (int) $moodleIds[$user->cid]); } } }); @@ -77,13 +78,19 @@ public function handle() * Synchronize Roles * * @param \App\User $user + * @param int|null $knownId Moodle user id, if already known (bulk sync path) — skips the + * redundant existence-check lookup that the single-user CLI path + * still needs to decide create-vs-update. * * @throws \Exception */ - private function sync(User $user) + private function sync(User $user, ?int $knownId = null) { //Update or Create - if ($id = $this->moodle->getUserId($user->cid)) { + if ($knownId !== null) { + $id = $knownId; + $this->moodle->updateUser($user, $id); + } elseif ($id = $this->moodle->getUserId($user->cid)) { //Update Information $this->moodle->updateUser($user, $id); } else { @@ -94,74 +101,113 @@ private function sync(User $user) //Assign Cohorts $this->moodle->clearUserCohorts($id); - $this->moodle->assignCohort($id, - Helper::ratingShortFromInt($user->rating)); //VATUSA level rating + $cohorts = []; + $cohorts[] = Helper::ratingShortFromInt($user->rating); //VATUSA level rating if ($user->flag_homecontroller) { - $this->moodle->assignCohort($id, - "$user->facility-" . Helper::ratingShortFromInt($user->rating)); //Facility level rating + $cohorts[] = "$user->facility-" . Helper::ratingShortFromInt($user->rating); //Facility level rating if (RoleHelper::userIsVATUSAStaff($user, false, true) || RoleHelper::userIsInstructor($user) || RoleHelper::userIsSeniorStaff($user, null, true) || RoleHelper::userIsMentor($user)) { - $this->moodle->assignCohort($id, "TNG"); //Training staff + $cohorts[] = "TNG"; //Training staff } } - $this->moodle->assignCohort($id, $user->facility); //Home Facility + $cohorts[] = $user->facility; //Home Facility foreach ($user->visits->pluck('facility') as $facility) { //Visiting Facilities - $this->moodle->assignCohort($id, $facility . "-V"); //Facility level visitor - $this->moodle->assignCohort($id, - "$facility-" . Helper::ratingShortFromInt($user->rating)); //Facility level rating + $cohorts[] = $facility . "-V"; //Facility level visitor + $cohorts[] = "$facility-" . Helper::ratingShortFromInt($user->rating); //Facility level rating + } + if (!empty($cohorts)) { + $this->moodle->assignCohortsBulk(array_map(fn ($cnumber) => ['uid' => $id, 'cnumber' => $cnumber], + $cohorts)); } + //Clear Roles $this->moodle->clearUserRoles($id); + $roles = []; //Assign Student Role foreach ($facilities as $facility) { - $this->moodle->assignRole($id, $this->moodle->getCategoryFromShort($facility, true), "STU", "coursecat"); + $roles[] = [ + 'uid' => $id, + 'cid' => $this->moodle->getCategoryFromShort($facility, true), + 'role' => "STU", + 'context' => "coursecat" + ]; } //Assign Category Permissions + $enrolments = []; if (RoleHelper::userIsVATUSAStaff($user, false, true) || RoleHelper::userIsSeniorStaff($user, $user->facility, true)) { - $this->moodle->assignRole($id, VATUSAMoodle::CATEGORY_CONTEXT_VATUSA, "INS", "coursecat"); - $this->moodle->assignRole($id, $this->moodle->getCategoryFromShort($user->facility, true), "TA", - "coursecat"); + $roles[] = [ + 'uid' => $id, 'cid' => VATUSAMoodle::CATEGORY_CONTEXT_VATUSA, 'role' => "INS", + 'context' => "coursecat" + ]; + $roles[] = [ + 'uid' => $id, + 'cid' => $this->moodle->getCategoryFromShort($user->facility, true), + 'role' => "TA", + 'context' => "coursecat" + ]; $artccCategories = $this->moodle->getAllSubcategories($this->moodle->getCategoryFromShort($user->facility), true); foreach ($artccCategories as $category) { $courses = $this->moodle->getCoursesInCategory($category); foreach ($courses as $course) { - $this->moodle->enrolUser($id, $course["id"]); + $enrolments[] = ['uid' => $id, 'cid' => $course["id"]]; } } } if (RoleHelper::userIsVATUSAStaff($user, false, true) || RoleHelper::userHas($user, "ZAE", "CBT")) { - $this->moodle->assignRole($id, VATUSAMoodle::CATEGORY_CONTEXT_VATUSA, "CBT", "coursecat"); + $roles[] = [ + 'uid' => $id, 'cid' => VATUSAMoodle::CATEGORY_CONTEXT_VATUSA, 'role' => "CBT", + 'context' => "coursecat" + ]; } if (RoleHelper::userIsVATUSAStaff($user, false, true) || RoleHelper::userHas($user, $user->facility, "FACCBT")) { - $this->moodle->assignRole($id, $this->moodle->getCategoryFromShort($user->facility, true), "FACCBT", - "coursecat"); + $roles[] = [ + 'uid' => $id, + 'cid' => $this->moodle->getCategoryFromShort($user->facility, true), + 'role' => "FACCBT", + 'context' => "coursecat" + ]; } if (RoleHelper::userIsVATUSAStaff($user, false, true) || RoleHelper::userIsInstructor($user, $user->facility)) { - $this->moodle->assignRole($id, VATUSAMoodle::CATEGORY_CONTEXT_VATUSA, "INS", "coursecat"); - $this->moodle->assignRole($id, $this->moodle->getCategoryFromShort($user->facility, true), "INS", - "coursecat"); + $roles[] = [ + 'uid' => $id, 'cid' => VATUSAMoodle::CATEGORY_CONTEXT_VATUSA, 'role' => "INS", + 'context' => "coursecat" + ]; + $roles[] = [ + 'uid' => $id, + 'cid' => $this->moodle->getCategoryFromShort($user->facility, true), + 'role' => "INS", + 'context' => "coursecat" + ]; } if (RoleHelper::userIsVATUSAStaff($user, false, true) || RoleHelper::userIsMentor($user)) { for ($i = Helper::ratingIntFromShort("S1"); $i <= $user->rating; $i++) { $context = "EXAM_CONTEXT_" . Helper::ratingShortFromInt($i); - $this->moodle->assignRole($id, $this->moodle->getConstant($context), "MTR", "course"); + $roles[] = [ + 'uid' => $id, 'cid' => $this->moodle->getConstant($context), 'role' => "MTR", + 'context' => "course" + ]; } } /*if (Role::where("cid", $user->cid)->where("facility", $user->facility)->where("role", "MTR")->exists()) { - $this->moodle->assignRole($id, $this->moodle->getCategoryFromShort($user->facility, true), "MTR", - "coursecat"); + $roles[] = ['uid' => $id, 'cid' => $this->moodle->getCategoryFromShort($user->facility, true), 'role' => "MTR", 'context' => "coursecat"]; }*/ + if (!empty($roles)) { + $this->moodle->assignRolesBulk($roles); + } + if (!empty($enrolments)) { + $this->moodle->enrolUsersBulk($enrolments); + } } } diff --git a/tests/Unit/MoodleUserIdMapTest.php b/tests/Unit/MoodleUserIdMapTest.php new file mode 100644 index 0000000..216c4eb --- /dev/null +++ b/tests/Unit/MoodleUserIdMapTest.php @@ -0,0 +1,46 @@ +pluck('id', 'idnumber'), where the Moodle DB's `idnumber` + * column is a VARCHAR holding the VATUSA CID as a string. MoodleSync::handle() + * then looks entries up with the integer $user->cid. This test locks in the + * assumption that swap relies on: PHP normalizes numeric string array keys to + * integers, so Collection::has()/offsetGet() with an int CID still matches a + * map keyed from string column values. + */ +class MoodleUserIdMapTest extends TestCase +{ + public function test_int_cid_lookup_matches_string_keyed_map(): void + { + // Simulates DB::connection('moodle')->table('user')->pluck('id', 'idnumber'), + // where idnumber is fetched as a string from a VARCHAR column. + $moodleIds = new Collection([ + '1234567' => 42, + '7654321' => 99, + ]); + + $this->assertTrue($moodleIds->has(1234567)); + $this->assertSame(42, $moodleIds[1234567]); + + $this->assertFalse($moodleIds->has(9999999)); + } + + public function test_users_absent_from_moodle_are_skipped(): void + { + $moodleIds = new Collection(['1234567' => 42]); + + $cidsToSync = collect([1234567, 2222222, 7654321]) + ->filter(fn ($cid) => $moodleIds->has($cid)) + ->values(); + + $this->assertSame([1234567], $cidsToSync->all()); + } +}