Skip to content

fix: skip the prune when the LDAP walk does not finish - #20

Merged
eluttine merged 1 commit into
mainfrom
fix/sync-prune-on-failure
Oct 8, 2026
Merged

eluttine merged 1 commit into
mainfrom
fix/sync-prune-on-failure

Conversation

@eluttine

@eluttine eluttine commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator

Problem

LdapSync::safeSearch() returned [] both when a page was genuinely empty and when the LDAP search threw. An empty page is what ends the paging loop, so a failed search left $kept empty and the next line ran:

$pruned = $this->groupMapper->deleteNotIn($kept);   // deletes everything

One transient LDAP failure in the 15-minute background job therefore emptied both tables, blanking the share picker for every teacher until a later run happened to succeed. The only trace was a warning in the log.

A partial failure is worse than a total one: if LDAP drops after page three of ten, the sync prunes against a third of the directory and deletes the rest.

Changes

  • safeSearch() returns null on failure, which callers must not confuse with an empty page. The log line is now an error with the exception attached, and says the prune was skipped.
  • Both sync halves return early on null, with complete => false and pruned => 0. The table keeps whatever the last good run wrote.
  • The stats shape gains complete: bool. emptyStats() (user_ldap unavailable) reports false too — nothing was read, so nothing may be pruned.
  • occ groupsharemachine:sync marks the affected line INCOMPLETE (prune skipped) and exits non-zero, so a cron wrapper or deploy script notices.
  • The background job logs at warning instead of info when a run was incomplete.

Deliberately not retried or backed off here: the job runs again in 15 minutes, and stale-but-intact tables are a much better failure mode than empty ones.

Note on the exit code

occ groupsharemachine:sync now exits non-zero when user_ldap is unavailable, where it previously printed zeros and exited 0. That is the intended reading — a sync that could not reach the directory did not do its job — but it is a behaviour change for anything scripting the command, including the install steps in the README.

Testing

  • New testFailedSearchDoesNotPruneTheTables: a fake Access whose searchGroups/searchUsers throw; asserts deleteNotIn is never called on either mapper and that both halves report complete => false.
  • Existing stats assertions updated for the new key.
  • phpunit 48/48, Psalm clean, php-cs-fixer clean.
  • Verified on the Nextcloud 33 dev instance: a normal run still reports Groups: seen=4 kept=4 pruned=0 and exits 0.

@eluttine
eluttine merged commit a2ef81d into main Oct 8, 2026
31 checks passed
@eluttine
eluttine deleted the fix/sync-prune-on-failure branch October 8, 2026 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant