fix(loopback): circles:check always fails due to APCu race and missing CLI wrapper#2387
Open
fix(loopback): circles:check always fails due to APCu race and missing CLI wrapper#2387
Conversation
The `circles:check` loopback test always fails with a 401 on the POST request to `asyncBroadcast` with `test-dummy-token` when the config key `test_dummy_token` does not already exist in the database. Root cause: The GET request to `core.CSRFToken.index` (which runs first) causes the web server process to load all `appconfig` values into its APCu local cache (TTL=3s). Only then does the CLI process insert `test_dummy_token` into the database and clear its own APCu cache — but CLI and web server run in separate SAPI contexts with independent APCu stores, so the web cache is unaffected. When the POST request arrives shortly after the GET (well within the 3-second TTL), the web process reads from its still-valid APCu cache, which does not contain the newly inserted key. `getValueInt()` therefore returns the default `0`, and `0 < time()` evaluates to true, causing the controller to return 401. Moving `setValueInt()` before the GET request ensures the key exists in the database before any web request loads the appconfig into APCu. Signed-Off-By: Iva Horn <iva.horn@nextcloud.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Even after the 401 is resolved, `circles:check` still fails with
"Event created too many Wrappers" (actually zero wrappers found, the
message is misleading). The LoopbackTest FederatedEvent never gets an
EventWrapper created when running from CLI.
Root cause: `initBroadcast()` has an early-return guard:
if (empty($instances) && (!$event->isAsync() || OC::$CLI))
For a LoopbackTest event (which implements IFederatedItemAsyncProcess but
has no remote instances), `$instances` is empty. Combined with
`OC::$CLI === true`, the guard returns false — skipping all wrapper
creation. This optimisation is correct for regular async events in CLI
mode, where `newEvent()` already calls `manage()` synchronously and no
async broadcast is needed. However, the LoopbackTest specifically exists
to verify that the async loopback path works end-to-end: wrapper
creation, async POST, `manageWrapper()`, and `confirmStatus()` must all
run to produce the verify/manage values that `testLoopback()` asserts.
The fix exempts events whose class implements IFederatedItemLoopbackTest
from the CLI guard, so that a wrapper is created and the async broadcast
is triggered even when running from CLI. The double call to `manage()`
(once synchronously in `newEvent()`, once asynchronously via the
wrapper) is harmless for LoopbackTest since it only sets an idempotent
result value.
Signed-Off-By: Iva Horn <iva.horn@nextcloud.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes two independent bugs that caused occ circles:check to always report an invalid loopback address, even when the address is correct.
Changes:
- APCu race condition fix (
CirclesCheck.php): MovessetValueInt()fortest_dummy_tokento before the GET request, ensuring the key is in the database before any web request can populate the APCu cache. This prevents the web server from caching a database snapshot that lacks the newly inserted key, which previously caused the subsequentasyncBroadcastPOST to return HTTP 401. - CLI guard bypass for loopback tests (
FederatedEventService.php): Exempts events whose class implementsIFederatedItemLoopbackTestfrom the early-return guard ininitBroadcast()that skips async wrapper creation when running under CLI. This allows theLoopbackTestevent to go through the full async path (wrapper creation → async POST →manageWrapper()) needed to verify end-to-end loopback functionality.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
lib/Command/CirclesCheck.php |
Reorders setValueInt before the GET request to avoid APCu cache race condition |
lib/Service/FederatedEventService.php |
Adds IFederatedItemLoopbackTest exemption to the CLI guard in initBroadcast() to allow async wrapper creation for loopback test events |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Summary
occ circles:checkalways reports an invalid loopback address even when the address is correct. Two independent bugs prevent the check from ever succeeding:asyncBroadcastwithtest-dummy-tokento return HTTP 401initBroadcast()preventsEventWrappercreation forLoopbackTestevents, causing the subsequent async verification to find zero wrappersBoth bugs are fixed in separate commits for clarity.
Bug 1: APCu cache race condition (401 on POST)
testLoopback()first makes a GET request tocore.CSRFToken.index, then insertstest_dummy_tokenintoappconfig, then makes a POST tocircles.EventWrapper.asyncBroadcast.The GET request causes the web server to load all
appconfigvalues into its APCu local cache (TTL = 3 seconds). The CLI process then insertstest_dummy_tokeninto the database and clears its own APCu cache — but CLI and web server run under different SAPI contexts with independent APCu stores, so the web server's cache is unaffected. When the POST request arrives (well within the 3-second TTL), the web process reads from its still-valid APCu cache, which does not contain the newly inserted key.IAppConfig::getValueInt()returns the default0, and0 < time()evaluates totrue, so theasyncBroadcastcontroller returns 401.Fix: Move
setValueInt()to before the GET request so the key exists in the database before any web request populates the APCu cache.Bug 2: CLI guard prevents wrapper creation (0 wrappers)
Even after the 401 is resolved,
circles:checkfails with "Event created too many Wrappers" (actually zero).initBroadcast()has an early-return guard:For a
LoopbackTestevent (implementsIFederatedItemAsyncProcess, no remote instances),$instancesis empty. Combined withOC::$CLI === true, the guard returnsfalse— skipping all wrapper creation. This optimisation is correct for regular async events in CLI mode wherenewEvent()already callsmanage()synchronously. However,LoopbackTestspecifically needs the full async path (wrapper creation → async POST →manageWrapper()→confirmStatus()) to verify that the loopback works end-to-end.Fix: Exempt events whose class implements
IFederatedItemLoopbackTestfrom the CLI guard. The double call tomanage()(once synchronously, once via the async wrapper) is harmless forLoopbackTestsince it only sets an idempotent result value.Steps to reproduce
docker run --name test-circles --detach --publish 8085:80 \ --env SQLITE_DATABASE=nextcloud.sqlite \ --env NEXTCLOUD_ADMIN_PASSWORD=admin \ --env NEXTCLOUD_ADMIN_USER=admin \ nextcloud:32.0.6 docker exec -u www-data test-circles php occ circles:checkBefore fix
After fix
Test plan
occ circles:checkon a fresh Nextcloud installation — loopback check should passocc circles:checkrepeatedly — should pass consistently (no intermittent 401)initBroadcastchangetest_dummy_tokenTTL (10 seconds) is not exceeded betweensetValueIntand the POST request under normal conditions🤖 Generated with Claude Code