fix: skip the prune when the LDAP walk does not finish

AGENTS.md

@@ -120,6 +120,7 @@ Nextcloud uses two **distinct** code paths for a group share — the picker (aut
 - Filter for users: `(&<configured>(puavoEduPersonAffiliation=teacher))`
 - Per-record school DN resolution: cached within a single sync run (see `$schoolNameCache`).
 - Background job runs every 15 minutes via `OCP\BackgroundJob\TimedJob`. Operators trigger immediate sync with `occ groupsharemachine:sync`.
+- **A failed search must never prune.** `safeSearch()` returns `null` when the LDAP search throws, as distinct from `[]` for a genuinely empty page — an empty page ends the paging loop and lets `deleteNotIn($kept)` run, so returning `[]` on failure would delete every row and blank the picker for all teachers. On `null` the sync returns early with `complete => false` and skips the prune, leaving the table as the last good run wrote it. Keep that distinction if you touch the paging loop.
 
 ## Testing
 

README.md

@@ -107,6 +107,14 @@ Reading the counters:
 
 `kept=0` on either line means nothing matched — see the [Requirements](#requirements) above and the troubleshooting notes in [`DEVELOPMENT.md`](DEVELOPMENT.md).
 
+A line can also end with `INCOMPLETE (prune skipped)`:
+
+```
+Groups:   seen=120 kept=120 pruned=0  INCOMPLETE (prune skipped)
+```
+
+That means the LDAP walk did not finish — the directory became unreachable part-way, or user_ldap is not available at all. Rows are then left exactly as the last good run wrote them instead of being pruned against a partial read, because pruning against half a directory would delete every group the sync never got to see. The command exits non-zero so a cron wrapper or deploy script notices; the reason is in the Nextcloud log. Rerun once LDAP is reachable and the counters return to normal.
+
 The 15-minute refresh runs as a Nextcloud background job, so it only happens if `cron.php` (or AJAX/webcron) is actually running on the instance — check with `occ background-job:list`.
 
 ### 3. Verify

lib/BackgroundJob/SyncGroupTypesJob.php

@@ -26,15 +26,24 @@ public function __construct(
 	protected function run(mixed $argument): void {
 		try {
 			$stats = $this->sync->run();
-			$this->logger->info(sprintf(
-				'groupsharemachine sync: groups(seen=%d kept=%d pruned=%d) teachers(seen=%d kept=%d pruned=%d)',
+			$message = sprintf(
+				'groupsharemachine sync: groups(seen=%d kept=%d pruned=%d complete=%s) teachers(seen=%d kept=%d pruned=%d complete=%s)',
 				$stats['groups']['seen'],
 				$stats['groups']['kept'],
 				$stats['groups']['pruned'],
+				$stats['groups']['complete'] ? 'yes' : 'no',
 				$stats['teachers']['seen'],
 				$stats['teachers']['kept'],
 				$stats['teachers']['pruned'],
-			));
+				$stats['teachers']['complete'] ? 'yes' : 'no',
+			);
+			if ($stats['groups']['complete'] && $stats['teachers']['complete']) {
+				$this->logger->info($message);
+			} else {
+				// Worth a warning: the tables are stale but intact, and will
+				// stay that way until a run completes.
+				$this->logger->warning($message . ' — prune skipped, tables left as they were');
+			}
 		} catch (Throwable $e) {
 			$this->logger->error('groupsharemachine sync failed: ' . $e->getMessage(), ['exception' => $e]);
 		}

lib/Command/SyncGroupTypesCommand.php

@@ -27,17 +27,29 @@ protected function configure(): void {
 	protected function execute(InputInterface $input, OutputInterface $output): int {
 		$stats = $this->sync->run();
 		$output->writeln(sprintf(
-			'Groups:   seen=%d kept=%d pruned=%d',
+			'Groups:   seen=%d kept=%d pruned=%d%s',
 			$stats['groups']['seen'],
 			$stats['groups']['kept'],
 			$stats['groups']['pruned'],
+			$stats['groups']['complete'] ? '' : '  INCOMPLETE (prune skipped)',
 		));
 		$output->writeln(sprintf(
-			'Teachers: seen=%d kept=%d pruned=%d',
+			'Teachers: seen=%d kept=%d pruned=%d%s',
 			$stats['teachers']['seen'],
 			$stats['teachers']['kept'],
 			$stats['teachers']['pruned'],
+			$stats['teachers']['complete'] ? '' : '  INCOMPLETE (prune skipped)',
 		));
-		return self::SUCCESS;
+
+		if ($stats['groups']['complete'] && $stats['teachers']['complete']) {
+			return self::SUCCESS;
+		}
+
+		// Non-zero so a cron wrapper or deploy script notices. The tables were
+		// left alone rather than pruned against a partial read, so the previous
+		// contents are still in place — rerun once LDAP is reachable again.
+		$output->writeln('');
+		$output->writeln('<error>The LDAP walk did not finish. Nothing was pruned; see the Nextcloud log.</error>');
+		return self::FAILURE;
 	}
 }

lib/Service/LdapSync.php

@@ -52,9 +52,14 @@ public function __construct(
 	}
 
 	/**
+	 * `complete` is false when the LDAP walk did not finish — either user_ldap
+	 * was unavailable or a search threw. Pruning is skipped in that case, so
+	 * the table keeps whatever the last good run wrote rather than being
+	 * emptied by a transient failure.
+	 *
 	 * @return array{
-	 *   groups: array{seen: int, kept: int, pruned: int},
-	 *   teachers: array{seen: int, kept: int, pruned: int},
+	 *   groups: array{seen: int, kept: int, pruned: int, complete: bool},
+	 *   teachers: array{seen: int, kept: int, pruned: int, complete: bool},
 	 * }
 	 */
 	public function run(): array {
@@ -65,7 +70,7 @@ public function run(): array {
 	}
 
 	/**
-	 * @return array{seen: int, kept: int, pruned: int}
+	 * @return array{seen: int, kept: int, pruned: int, complete: bool}
 	 */
 	private function syncGroups(): array {
 		$access = $this->resolveAccess('OCA\\User_LDAP\\Group_Proxy', isUser: false);
@@ -102,6 +107,12 @@ private function syncGroups(): array {
 				offset: $offset,
 				extraAttrs: [$displayAttr, self::GROUP_ABBREVIATION_ATTR],
 			);
+			if ($records === null) {
+				// The walk is incomplete, so $kept is not the full set of
+				// groups in LDAP. Pruning against it would delete every group
+				// we never got to read.
+				return ['seen' => $seen, 'kept' => count($kept), 'pruned' => 0, 'complete' => false];
+			}
 			foreach ($records as $record) {
 				$seen++;
 				$dn = $record['dn'][0] ?? null;
@@ -134,7 +145,7 @@ private function syncGroups(): array {
 		} while (count($records) === self::PAGE_SIZE);
 
 		$pruned = $this->groupMapper->deleteNotIn($kept);
-		return ['seen' => $seen, 'kept' => count($kept), 'pruned' => $pruned];
+		return ['seen' => $seen, 'kept' => count($kept), 'pruned' => $pruned, 'complete' => true];
 	}
 
 	private function resolveSchoolName(object $access, string $schoolDn): ?string {
@@ -152,7 +163,7 @@ private function resolveSchoolName(object $access, string $schoolDn): ?string {
 	}
 
 	/**
-	 * @return array{seen: int, kept: int, pruned: int}
+	 * @return array{seen: int, kept: int, pruned: int, complete: bool}
 	 */
 	private function syncTeachers(): array {
 		$access = $this->resolveAccess('OCA\\User_LDAP\\User_Proxy', isUser: true);
@@ -172,6 +183,9 @@ private function syncTeachers(): array {
 		$offset = 0;
 		do {
 			$records = $this->safeSearch($access, isUser: true, filter: $filter, offset: $offset);
+			if ($records === null) {
+				return ['seen' => $seen, 'kept' => count($kept), 'pruned' => 0, 'complete' => false];
+			}
 			foreach ($records as $record) {
 				$seen++;
 				$dn = $record['dn'][0] ?? null;
@@ -200,14 +214,18 @@ private function syncTeachers(): array {
 		} while (count($records) === self::PAGE_SIZE);
 
 		$pruned = $this->teacherMapper->deleteNotIn($kept);
-		return ['seen' => $seen, 'kept' => count($kept), 'pruned' => $pruned];
+		return ['seen' => $seen, 'kept' => count($kept), 'pruned' => $pruned, 'complete' => true];
 	}
 
 	/**
+	 * Returns null when the search failed, which the callers must not confuse
+	 * with an empty page: an empty array ends the paging loop and lets the
+	 * prune run, so returning one here would delete every row in the table.
+	 *
 	 * @param list<string> $extraAttrs additional (lowercased) attributes to request
-	 * @return array
+	 * @return ?array
 	 */
-	private function safeSearch(object $access, bool $isUser, string $filter, int $offset, array $extraAttrs = []): array {
+	private function safeSearch(object $access, bool $isUser, string $filter, int $offset, array $extraAttrs = []): ?array {
 		try {
 			$attrs = $isUser
 				? ['dn', strtolower(self::SCHOOL_ATTR)]
@@ -218,10 +236,12 @@ private function safeSearch(object $access, bool $isUser, string $filter, int $o
 				: $access->searchGroups($filter, $attrs, self::PAGE_SIZE, $offset);
 			return is_array($records) ? $records : [];
 		} catch (Throwable $e) {
-			$this->logger->warning(
-				($isUser ? 'searchUsers' : 'searchGroups') . " failed at offset {$offset}: " . $e->getMessage(),
+			$this->logger->error(
+				($isUser ? 'searchUsers' : 'searchGroups') . " failed at offset {$offset}, "
+				. 'skipping the prune for this run: ' . $e->getMessage(),
+				['exception' => $e],
 			);
-			return [];
+			return null;
 		}
 	}
 
@@ -292,9 +312,10 @@ private static function escapeLdapValue(string $value): string {
 	}
 
 	/**
-	 * @return array{seen: int, kept: int, pruned: int}
+	 * @return array{seen: int, kept: int, pruned: int, complete: bool}
 	 */
 	private function emptyStats(): array {
-		return ['seen' => 0, 'kept' => 0, 'pruned' => 0];
+		// complete=false: nothing was read, so nothing may be pruned either.
+		return ['seen' => 0, 'kept' => 0, 'pruned' => 0, 'complete' => false];
 	}
 }

tests/Unit/Service/LdapSyncTest.php

@@ -51,8 +51,8 @@ public function testGroupSearchUpsertsAllowedTypes(): void {
 		// Empty users list → no User_Proxy sample → teacher sync skipped entirely.
 
 		$stats = $sync->run();
-		$this->assertSame(['seen' => 2, 'kept' => 2, 'pruned' => 0], $stats['groups']);
-		$this->assertSame(['seen' => 0, 'kept' => 0, 'pruned' => 0], $stats['teachers']);
+		$this->assertSame(['seen' => 2, 'kept' => 2, 'pruned' => 0, 'complete' => true], $stats['groups']);
+		$this->assertSame(['seen' => 0, 'kept' => 0, 'pruned' => 0, 'complete' => false], $stats['teachers']);
 	}
 
 	public function testGroupSearchSkipsRecordsWithoutDnOrType(): void {
@@ -155,7 +155,7 @@ public function testTeacherSearchWritesOneRowPerSchool(): void {
 			->willReturn(0);
 
 		$stats = $sync->run();
-		$this->assertSame(['seen' => 2, 'kept' => 3, 'pruned' => 0], $stats['teachers']);
+		$this->assertSame(['seen' => 2, 'kept' => 3, 'pruned' => 0, 'complete' => true], $stats['teachers']);
 	}
 
 	public function testTeacherWithoutAnySchoolIsSkipped(): void {
@@ -227,8 +227,8 @@ protected function resolveProxy(string $class): ?object {
 		$stats = $sync->run();
 		$this->assertSame(
 			[
-				'groups' => ['seen' => 0, 'kept' => 0, 'pruned' => 0],
-				'teachers' => ['seen' => 0, 'kept' => 0, 'pruned' => 0],
+				'groups' => ['seen' => 0, 'kept' => 0, 'pruned' => 0, 'complete' => false],
+				'teachers' => ['seen' => 0, 'kept' => 0, 'pruned' => 0, 'complete' => false],
 			],
 			$stats,
 		);
@@ -245,15 +245,40 @@ public function testEmptyLdapBecomesNoOp(): void {
 		$this->assertSame(0, $stats['teachers']['seen']);
 	}
 
+	public function testFailedSearchDoesNotPruneTheTables(): void {
+		// A search that throws used to be indistinguishable from the last page
+		// of results: the loop ended with an empty $kept and deleteNotIn([])
+		// wiped every row, blanking the share picker until the next good sync.
+		$sync = $this->makeSync(
+			groups: [
+				'puavoid=10,ou=groups' => ['gid' => 'class_1a', 'puavoedugrouptype' => ['year class']],
+			],
+			users: [
+				'puavoid=20,ou=people' => ['uid' => 'teacher1', 'puavoschool' => ['puavoId=1,ou=Schools']],
+			],
+			searchThrows: true,
+		);
+
+		$this->groupMapper->expects($this->never())->method('deleteNotIn');
+		$this->teacherMapper->expects($this->never())->method('deleteNotIn');
+
+		$stats = $sync->run();
+
+		$this->assertFalse($stats['groups']['complete']);
+		$this->assertFalse($stats['teachers']['complete']);
+		$this->assertSame(0, $stats['groups']['pruned']);
+		$this->assertSame(0, $stats['teachers']['pruned']);
+	}
+
 	/**
 	 * Build an LdapSync subclass with fake Group_Proxy / User_Proxy.
 	 *
 	 * @param array<string, array{gid: ?string, puavoedugrouptype: list<string>}> $groups DN-keyed
 	 * @param array<string, array{uid: ?string}> $users DN-keyed
 	 */
-	private function makeSync(array $groups, array $users, array $schools = []): LdapSyncTestDouble {
-		$groupAccess = $this->buildAccess($groups, $schools);
-		$userAccess = $this->buildAccess($users, []);
+	private function makeSync(array $groups, array $users, array $schools = [], bool $searchThrows = false): LdapSyncTestDouble {
+		$groupAccess = $this->buildAccess($groups, $schools, $searchThrows);
+		$userAccess = $this->buildAccess($users, [], $searchThrows);
 
 		$groupSample = array_values($groups)[0]['gid'] ?? null;
 		$userSample = array_values($users)[0]['uid'] ?? null;
@@ -272,8 +297,8 @@ private function makeSync(array $groups, array $users, array $schools = []): Lda
 	/**
 	 * @param array<string, array<string, mixed>> $records DN-keyed test data
 	 */
-	private function buildAccess(array $records, array $schoolDirectory): object {
-		return new class($records, $schoolDirectory) {
+	private function buildAccess(array $records, array $schoolDirectory, bool $searchThrows = false): object {
+		return new class($records, $schoolDirectory, $searchThrows) {
 			/** @var list<array<int, string>> */
 			public array $capturedFilterCalls = [];
 
@@ -283,6 +308,7 @@ private function buildAccess(array $records, array $schoolDirectory): object {
 			public function __construct(
 				private array $records,
 				private array $schoolDirectory,
+				private bool $searchThrows,
 			) {
 			}
 
@@ -300,13 +326,22 @@ public function combineFilterWithAnd(array $filters): string {
 			}
 
 			public function searchGroups(string $filter, array $attr, int $limit, int $offset): array {
+				$this->maybeThrow();
 				return $this->materialize($offset, $limit);
 			}
 
 			public function searchUsers(string $filter, array $attr, int $limit, int $offset): array {
+				$this->maybeThrow();
 				return $this->materialize($offset, $limit);
 			}
 
+			private function maybeThrow(): void {
+				if ($this->searchThrows) {
+					// What a dropped connection looks like from in here.
+					throw new \RuntimeException('Can\'t contact LDAP server');
+				}
+			}
+
 			private function materialize(int $offset, int $limit): array {
 				$out = [];
 				$dns = array_keys($this->records);