Repository navigation
fix: skip the prune when the LDAP walk does not finish - #20
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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$keptempty and the next line ran: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
warningin 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()returnsnullon failure, which callers must not confuse with an empty page. The log line is now anerrorwith the exception attached, and says the prune was skipped.null, withcomplete => falseandpruned => 0. The table keeps whatever the last good run wrote.complete: bool.emptyStats()(user_ldap unavailable) reportsfalsetoo — nothing was read, so nothing may be pruned.occ groupsharemachine:syncmarks the affected lineINCOMPLETE (prune skipped)and exits non-zero, so a cron wrapper or deploy script notices.warninginstead ofinfowhen 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:syncnow 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
testFailedSearchDoesNotPruneTheTables: a fake Access whosesearchGroups/searchUsersthrow; assertsdeleteNotInis never called on either mapper and that both halves reportcomplete => false.phpunit48/48, Psalm clean,php-cs-fixerclean.Groups: seen=4 kept=4 pruned=0and exits 0.