Skip to content

fix(activesync): treat collection lock contention as transient during PING#75

Merged
ralflang merged 1 commit into
FRAMEWORK_6_0from
fix/activesync-collection-lock-contention
Jul 2, 2026
Merged

fix(activesync): treat collection lock contention as transient during PING#75
ralflang merged 1 commit into
FRAMEWORK_6_0from
fix/activesync-collection-lock-contention

Conversation

@TDannhauer

Copy link
Copy Markdown
Contributor

Summary

  • Collection lock contention during PING no longer triggers a destructive device state reset or an HTTP 500.
  • Lock contention in the Sql and Mongo state drivers now throws Horde_ActiveSync_Exception_TemporaryFailure instead of a generic Horde_ActiveSync_Exception.
  • loadState() no longer emits PHP 8 "Undefined array key" warnings when invoked with an empty collection array during error recovery.

Motivation

Production logs showed this failure chain when a PING polled a collection whose lock was held by a parallel SYNC:

  1. initCollectionState() threw the generic Horde_ActiveSync_Exception('Collection lock held.').
  2. The generic catch in pollForChanges() treated this as corrupt state and started a recovery reset: loadState([], null, SYNC, $id).
  3. The empty collection array caused Undefined array key "class" / "serverid" warnings in State/Base.php.
  4. The reset itself tried to acquire the same held lock, threw again, and escaped uncaught — the client received HTTP 500.

Lock contention is a normal, transient condition. Reacting with a state reset risks wiping healthy sync state whenever two requests overlap.

Changes

  • State/Sql.php, State/Mongo.php: throw Horde_ActiveSync_Exception_TemporaryFailure when the collection lock cannot be acquired. As a subclass of Horde_ActiveSync_Exception, all existing generic catch sites keep working.
  • Collections.php (pollForChanges()): catch TemporaryFailure before the generic handler and skip the collection for this iteration (retried on the next poll) instead of resetting state. The two StaleState recovery resets are wrapped the same way: if the reset is blocked by a held lock, it is deferred to the next iteration instead of aborting the request.
  • State/Base.php (loadState()): read class/serverid with null coalescing so an error-recovery reset with an empty collection array creates a plain Horde_ActiveSync_Folder_Collection without PHP 8 warnings (matches pre-PHP-8 behavior).

Test plan

  • New: held lock throws TemporaryFailure and rolls back the lock transaction (CollectionLockTest::testHeldLockThrowsTemporaryFailure)
  • New: loadState([], null, SYNC, $id) emits no warnings and creates a folder object (CollectionLockTest::testLoadStateResetWithEmptyCollectionEmitsNoWarnings)
  • New: pollForChanges() skips the collection on lock contention without calling loadState() reset (PingCollectionLockContentionTest)
  • Existing state/lock/ping unit tests pass

… PING

A PING polling loop that hit a collection lock held by a parallel SYNC
received a generic Horde_ActiveSync_Exception and reacted with the
corrupt-state recovery path: a device state reset via loadState() with
an empty collection array. This caused "Undefined array key" warnings
in State/Base.php, and the reset itself failed again on the still-held
lock, escaping as an uncaught exception and returning HTTP 500 to the
client. In the worst case the recovery would wipe healthy sync state
just because another request momentarily held the lock.

Throw Horde_ActiveSync_Exception_TemporaryFailure on lock contention in
the Sql and Mongo state drivers, and catch it in pollForChanges() to
skip the affected collection until the next poll iteration instead of
resetting state. Defer the StaleState recovery resets the same way when
the lock is unavailable. Guard the folder object creation in
loadState() against an empty collection array to silence the PHP 8
warnings on legitimate resets.
@TDannhauer TDannhauer requested a review from ralflang July 2, 2026 07:21
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

🔍 CI Results

Overall: ❌ 12/12 lanes failed

TL;DR: ❌ Quality issues: PHPUnit: 36 tests failed; PHPStan: 333 unique errors in 8 lanes.

Summary by PHP Version

PHP dev stable
8.0
8.1
8.2
8.3
8.4
8.5

Quality Metrics

  • PHPUnit: 16 failures, 20 errors out of 202 tests in 8 lanes ❌
    Per-test detail in the table below. Raw JUnit XML and JSON are uploaded as workflow artifacts.

    Which tests failed
    Type Test Lanes Message
    ❌ failure Horde\ActiveSync\AppointmentTest::testSetRecurrenceEas16SetsFirstDayOfWeek php8.2-dev, php8.2-stable, php8.3-dev, php8.3-stable, php8.4-dev, php8.4-stable, php8.5-dev, php8.5-stable [PHPUnit\Framework\ExpectationFailedException] Failed asserting that '' matches expected 0.
    ⚠️ error Horde\ActiveSync\Request\FindRequestTest::testMockDriverReturnsSuccessfulEmptyFindResults php8.2-dev, php8.2-stable, php8.3-dev, php8.3-stable, php8.4-dev, php8.4-stable, php8.5-dev, php8.5-stable [InvalidArgumentException] Missing required state object
    ❌ failure Horde\ActiveSync\Request\FindRequestTest::testParseFindRangeHonorsClientWindow php8.2-dev, php8.2-stable, php8.3-dev, php8.3-stable, php8.4-dev, php8.4-stable, php8.5-dev, php8.5-stable [PHPUnit\Framework\ExpectationFailedException] Failed asserting that 100 is identical to 101.
    ⚠️ error Horde\ActiveSync\SyncTimeBudgetTest::testGetSyncConfigDefaultsToEmptyArray php8.3-dev, php8.3-stable, php8.4-dev, php8.4-stable, php8.5-dev, php8.5-stable [Error] Call to undefined method PHPUnit\Framework\MockObject\MockBuilder::getMockForAbstractClass()
    ⚠️ error Horde\ActiveSync\SyncTimeBudgetTest::testGetSyncConfigReturnsConfiguredValues php8.3-dev, php8.3-stable, php8.4-dev, php8.4-stable, php8.5-dev, php8.5-stable [Error] Call to undefined method PHPUnit\Framework\MockObject\MockBuilder::getMockForAbstractClass()
  • PHPStan (advisory): 333 unique errors in 8 lanes ⚠️

  • PHP-CS-Fixer: did not run on any lane ❌

❌ Failed Lanes

php8.0-dev

  • PHPUnit: ❌ Setup failed (No result file found)
  • PHPStan: ❌ Setup failed (No result file found)

php8.0-stable

  • PHPUnit: ❌ Setup failed (Composer install failed in /tmp/horde-ci/lanes/php8.0-stable/ActiveSync: Your requirements could not be resolved to an installable set of packages.)
  • PHPStan: ❌ Setup failed (Composer install failed in /tmp/horde-ci/lanes/php8.0-stable/ActiveSync: Your requirements could not be resolved to an installable set of packages.)

php8.1-dev

  • PHPUnit: ❌ Setup failed (No result file found)
  • PHPStan: ❌ Setup failed (No result file found)

php8.1-stable

  • PHPUnit: ❌ Setup failed (No result file found)
  • PHPStan: ❌ Setup failed (No result file found)

php8.2-dev

  • PHPUnit: 2 failures, 1 error

php8.2-stable

  • PHPUnit: 2 failures, 1 error

php8.3-dev

  • PHPUnit: 2 failures, 3 errors

php8.3-stable

  • PHPUnit: 2 failures, 3 errors

php8.4-dev

  • PHPUnit: 2 failures, 3 errors
  • PHP-CS-Fixer: ❌ Setup failed (No result file found)

php8.4-stable

  • PHPUnit: 2 failures, 3 errors

php8.5-dev

  • PHPUnit: 2 failures, 3 errors

php8.5-stable

  • PHPUnit: 2 failures, 3 errors

CI powered by horde-componentsView full results

@ralflang ralflang merged commit 1ded830 into FRAMEWORK_6_0 Jul 2, 2026
1 check failed
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.

2 participants