From afafe324572ef4994013cc92dfd6fc2e0c85dc1d Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Fri, 31 Jul 2026 08:55:02 +0000 Subject: [PATCH 1/6] fix: retry rate-limited Leantime requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fetchFromLeantime() calls getContent() with default error handling, so a 429 throws out of updateAsJob() before the next page is queued — one rate-limited response ended the whole pagination chain. With five staggered nightly syncs and a 15-minute incremental cron against the same API, that is the likeliest halt in production. RetryableHttpClient with a 429 strategy was added for exactly this in b64773db and deleted in b27ba16e with the old Jira stack, leaving docker-compose.server.override.yml still commenting that the sync is rate limited by the Leantime API. This restores it as app.leantime.http_client, injected only into LeantimeApiService so the monitoring ping keeps the plain client, and tunable via APP_HTTP_CLIENT_RETRY_DELAY_MS and APP_HTTP_CLIENT_MAX_RETRIES. The retried status codes are a flat list rather than GenericRetryStrategy's defaults: those express transport errors and 500/504/507/510 as [code => idempotent methods], which excludes POST, and the Leantime data API uses POST even for reads. Also sets timeout and max_duration on the default HTTP client. Symfony caps neither, so a Leantime instance that accepted a connection and then stalled held the worker indefinitely — messenger's --time-limit is only checked between messages. max_duration is 300s to clear the unpaginated /deleted endpoint used by sync-deleted --interval=P1W. --- CHANGELOG.md | 17 ++ config/packages/framework.yaml | 12 ++ config/services.yaml | 36 ++++ .../Service/LeantimeApiServiceRetryTest.php | 190 ++++++++++++++++++ 4 files changed, 255 insertions(+) create mode 100644 tests/Integration/Service/LeantimeApiServiceRetryTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b0632ab6..6d6b6f16c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 plugin ignores — it reads `deleted`. Every delete-sync was pulling the entire deletion history, on an endpoint the plugin does not paginate. Deletion entries with no id are now skipped and logged rather than aborting the remaining types. + * Restored retrying of rate-limited Leantime requests. `RetryableHttpClient` with a + 429 retry strategy was added in `b64773db` ("1595: Added retryable http client to + handle rate limiting") and lost in `b27ba16e` when the Jira stack was removed, leaving + `docker-compose.server.override.yml` still commenting that the sync is rate limited by + the Leantime API. A 429 surfaces inside `updateAsJob()` before the next page is + queued, so a single one ended the whole pagination chain. `LeantimeApiService` now + gets a retrying client via `app.leantime.http_client`, tunable with + `APP_HTTP_CLIENT_RETRY_DELAY_MS` and `APP_HTTP_CLIENT_MAX_RETRIES`. The retried + status codes are a flat list, because `GenericRetryStrategy`'s defaults restrict + transport errors and 5xx to idempotent methods — which excludes the POSTs the + Leantime data API uses even for reads. + * Added `timeout: 30` and `max_duration: 300` to `framework.http_client.default_options`. + Symfony caps neither by default, so a Leantime instance that accepted a connection and + then stalled held the messenger worker indefinitely — `--time-limit` is only checked + between messages, never during one. `max_duration` has to clear the unpaginated + `/deleted` endpoint, which returns a week of history in one response for + `sync-deleted --interval=P1W`. * Added `LeantimeApiServiceTest::testUpdateWithNullValues()`, covering the nullable payload from data-api#18 plus two probes that a single unmappable row is logged and skipped rather than stopping the sync: a `TypeError` and a failure raised inside the upsert diff --git a/config/packages/framework.yaml b/config/packages/framework.yaml index 12bcef43d..635f8d635 100644 --- a/config/packages/framework.yaml +++ b/config/packages/framework.yaml @@ -17,6 +17,18 @@ framework: php_errors: log: true + http_client: + default_options: + # Symfony caps neither by default, so a Leantime instance that accepts the connection and + # then stalls holds the messenger worker indefinitely: --time-limit is only checked + # between messages, never during one. + # + # timeout is the idle gap between chunks; max_duration is the whole request. The latter + # has to clear the /deleted endpoint, which the Leantime plugin does not paginate, so + # `sync-deleted --interval=P1W` pulls a week of history in one response. + timeout: 30 + max_duration: 300 + # see https://symfony.com/doc/current/deployment/proxies.html trusted_headers: [ diff --git a/config/services.yaml b/config/services.yaml index 46311befb..c774e4b87 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -7,6 +7,10 @@ imports: # Put parameters here that don't need to change on each machine where the app is deployed # https://symfony.com/doc/current/best_practices.html#use-parameters-for-application-configuration parameters: + # Retry budget for the Leantime data API. Defaults live here rather than in .env so the sync + # works out of the box; override per environment if Leantime's rate limit changes. + env(APP_HTTP_CLIENT_RETRY_DELAY_MS): 2000 + env(APP_HTTP_CLIENT_MAX_RETRIES): 3 services: # default configuration for services in *this* file @@ -64,3 +68,35 @@ services: App\Command\SyncCommand: arguments: $monitoringUrl: "%env(string:SYNC_MONITORING_URL)%" + + # Leantime rate-limits its data API, and a full sync makes hundreds of paged requests. A 429 + # surfaces inside updateAsJob() before the next page is queued, so without retrying, one of them + # ends the whole pagination chain. Restores the RetryableHttpClient that was dropped along with + # the old Jira stack, while the deploy comment still assumes rate limiting is handled. + Symfony\Component\HttpClient\Retry\RetryStrategyInterface: + class: Symfony\Component\HttpClient\Retry\GenericRetryStrategy + # Fetched directly by LeantimeApiServiceRetryTest, which drives the configured strategy + # against mock responses rather than asserting on a copy of the numbers below. + public: true + arguments: + # A flat list retries on any HTTP method. GenericRetryStrategy's own defaults restrict + # transport errors (0) and 500/504/507/510 to idempotent methods, which excludes the + # POSTs the Leantime data API uses even for reads — every one of these calls is a read. + $statusCodes: [0, 423, 425, 429, 500, 502, 503, 504, 507, 510] + $delayMs: "%env(int:APP_HTTP_CLIENT_RETRY_DELAY_MS)%" + $multiplier: 2.0 + # RetryableHttpClient prefers the response's Retry-After header over this backoff, so the + # cap only bounds the cases where Leantime sends no hint. + $maxDelayMs: 60000 + + app.leantime.http_client: + class: Symfony\Component\HttpClient\RetryableHttpClient + arguments: + $client: "@http_client" + $strategy: '@Symfony\Component\HttpClient\Retry\RetryStrategyInterface' + $maxRetries: "%env(int:APP_HTTP_CLIENT_MAX_RETRIES)%" + $logger: "@logger" + + App\Service\LeantimeApiService: + arguments: + $httpClient: "@app.leantime.http_client" diff --git a/tests/Integration/Service/LeantimeApiServiceRetryTest.php b/tests/Integration/Service/LeantimeApiServiceRetryTest.php new file mode 100644 index 000000000..5898534ee --- /dev/null +++ b/tests/Integration/Service/LeantimeApiServiceRetryTest.php @@ -0,0 +1,190 @@ + */ + private array $dispatched = []; + + public function testLeantimeClientIsRetryable(): void + { + self::bootKernel(); + + $service = self::getContainer()->get(LeantimeApiService::class); + + $client = (new \ReflectionProperty($service, 'httpClient'))->getValue($service); + + $this->assertInstanceOf( + RetryableHttpClient::class, + $client, + 'LeantimeApiService must get a retrying client, or one 429 ends the pagination chain.', + ); + } + + /** + * The regression itself: a rate-limited page must be retried, not dropped. + */ + public function testRateLimitedPageIsRetriedAndStillPaginates(): void + { + $service = $this->createService([ + new MockResponse('', ['http_code' => 429]), + new MockResponse($this->fullPageJson(), ['http_code' => 200]), + ]); + + $service->updateAsJob(Project::class, 0, self::LIMIT, 1); + + $this->assertCount(self::LIMIT, $this->dispatchedOfType(UpsertProjectMessage::class), 'Every row of the retried page should still be upserted.'); + $this->assertCount(1, $this->dispatchedOfType(LeantimeUpdateMessage::class), 'The chain must continue to the next page.'); + } + + public function testServiceUnavailableIsRetried(): void + { + $service = $this->createService([ + new MockResponse('', ['http_code' => 503]), + new MockResponse($this->fullPageJson(), ['http_code' => 200]), + ]); + + $service->updateAsJob(Project::class, 0, self::LIMIT, 1); + + $this->assertCount(1, $this->dispatchedOfType(LeantimeUpdateMessage::class)); + } + + /** + * The Leantime data API is POST-only, including for reads, and every call this service makes is + * a read. GenericRetryStrategy's own defaults express 500/504/507/510 and transport errors as + * [code => idempotent methods], which excludes POST — so those cases would silently stop being + * retried if the configured list were ever "simplified" back to the defaults. A flat list of + * codes is what makes them apply to POST. + */ + public function testRetryStrategyCoversPostForTransportErrorsAndServerErrors(): void + { + self::bootKernel(); + + $strategy = self::getContainer()->get(RetryStrategyInterface::class); + $this->assertInstanceOf(RetryStrategyInterface::class, $strategy); + + $statusCodes = (array) (new \ReflectionProperty($strategy, 'statusCodes'))->getValue($strategy); + + // 0 is the transport-error slot: connection reset, DNS failure, timeout. + foreach ([0, 429, 500, 502, 503, 504] as $code) { + $this->assertContains( + $code, + $statusCodes, + sprintf('%d must be retried for any method; a [code => methods] entry would skip POST.', $code), + ); + } + } + + /** + * Retrying must not turn a genuinely unavailable API into a silent success: once the retries are + * spent the error still has to surface, so the caller can fail the page visibly. + */ + public function testPersistentRateLimitStillFails(): void + { + $service = $this->createService(array_fill(0, 10, new MockResponse('', ['http_code' => 429]))); + + $this->expectException(HttpExceptionInterface::class); + + $service->updateAsJob(Project::class, 0, self::LIMIT, 1); + } + + /** + * A full page of project rows, so the chain queues a next page. Only the transport behaviour is + * under test, so the payload is fixed and valid. + */ + private function fullPageJson(): string + { + $results = array_map(static fn (int $id) => [ + 'id' => $id, + 'name' => 'Project '.$id, + 'modified' => '2026-07-30 12:00:00', + ], range(1, self::LIMIT)); + + return json_encode([ + 'results' => $results, + 'resultsCount' => count($results), + ], JSON_THROW_ON_ERROR); + } + + /** + * The service under test, wrapping the given mock responses in the retry strategy the container + * actually configures — the point is to exercise the real strategy, not a copy of it. + * + * @param list $responses + */ + private function createService(array $responses): LeantimeApiService + { + self::bootKernel(); + $container = self::getContainer(); + + $strategy = $container->get(RetryStrategyInterface::class); + $this->assertInstanceOf(RetryStrategyInterface::class, $strategy); + + $httpClient = new RetryableHttpClient(new MockHttpClient($responses), $strategy, 3); + + $this->dispatched = []; + $messageBus = $this->createMock(MessageBusInterface::class); + $messageBus->method('dispatch')->willReturnCallback( + function (object $message, array $stamps = []): Envelope { + $this->dispatched[] = $message; + + return new Envelope($message, $stamps); + } + ); + + $dataProvider = new DataProvider(); + $dataProvider->setName('Retry test provider'); + $dataProvider->setEnabled(true); + $dataProvider->setClass(LeantimeApiService::class); + $dataProvider->setUrl('http://leantime.example.com'); + $dataProvider->setSecret('Not so secret'); + + $dataProviderRepository = $this->createMock(DataProviderRepository::class); + $dataProviderRepository->method('find')->willReturn($dataProvider); + + return new LeantimeApiService( + $httpClient, + $messageBus, + $dataProviderRepository, + $this->createMock(EntityManagerInterface::class), + $this->createMock(ProjectRepository::class), + $this->createMock(LoggerInterface::class), + ); + } + + /** + * @return list + */ + private function dispatchedOfType(string $class): array + { + return array_values(array_filter($this->dispatched, static fn (object $m) => $m instanceof $class)); + } +} From 7e98e4f24355a0961d89abc72fe1c3e277c34b23 Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Fri, 31 Jul 2026 10:59:59 +0200 Subject: [PATCH 2/6] fix: moved env defaults to .env --- .env | 6 ++++-- config/services.yaml | 4 ---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/.env b/.env index 7c7e415c7..6374eb2ba 100644 --- a/.env +++ b/.env @@ -25,8 +25,9 @@ APP_INVOICE_SUPPLIER_ACCOUNT=APP_INVOICE_SUPPLIER_ACCOUNT APP_INVOICE_EXTERNAL_RECEIVER_ACCOUNT= APP_INVOICE_DESCRIPTION_TEMPLATE="Spørgsmål vedrørende fakturaen rettes til %name%, %email%." APP_PROJECT_BILLING_DEFAULT_DESCRIPTION= -APP_HTTP_CLIENT_RETRY_DELAY_MS=1000 -APP_HTTP_CLIENT_MAX_RETRIES=3 +# Retry budget for the Leantime data API. +APP_HTTP_CLIENT_RETRY_DELAY_MS: 2000 +APP_HTTP_CLIENT_MAX_RETRIES: 3 EMAIL_FROM_ADDRESS= ###> itk-dev/openid-connect-bundle ### @@ -112,3 +113,4 @@ APP_API_KEY='' TIDY_FEEDBACK_WIDGET_URL= TIDY_FEEDBACK_API_KEY= ###< tidy-feedback ### + diff --git a/config/services.yaml b/config/services.yaml index c774e4b87..9d0f6bcfc 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -7,10 +7,6 @@ imports: # Put parameters here that don't need to change on each machine where the app is deployed # https://symfony.com/doc/current/best_practices.html#use-parameters-for-application-configuration parameters: - # Retry budget for the Leantime data API. Defaults live here rather than in .env so the sync - # works out of the box; override per environment if Leantime's rate limit changes. - env(APP_HTTP_CLIENT_RETRY_DELAY_MS): 2000 - env(APP_HTTP_CLIENT_MAX_RETRIES): 3 services: # default configuration for services in *this* file From 4140dc24c8ca88648e56ec8b7efc23229be1a7a2 Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Fri, 31 Jul 2026 11:04:02 +0200 Subject: [PATCH 3/6] fix: fixed env default assignments --- .env | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.env b/.env index 6374eb2ba..11c023a9d 100644 --- a/.env +++ b/.env @@ -26,8 +26,8 @@ APP_INVOICE_EXTERNAL_RECEIVER_ACCOUNT= APP_INVOICE_DESCRIPTION_TEMPLATE="Spørgsmål vedrørende fakturaen rettes til %name%, %email%." APP_PROJECT_BILLING_DEFAULT_DESCRIPTION= # Retry budget for the Leantime data API. -APP_HTTP_CLIENT_RETRY_DELAY_MS: 2000 -APP_HTTP_CLIENT_MAX_RETRIES: 3 +APP_HTTP_CLIENT_RETRY_DELAY_MS=2000 +APP_HTTP_CLIENT_MAX_RETRIES=3 EMAIL_FROM_ADDRESS= ###> itk-dev/openid-connect-bundle ### From 017acd77b220cb7c69895d3d0518f56ccbc6a0d2 Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Wed, 19 Aug 2026 11:18:54 +0000 Subject: [PATCH 4/6] fix: moved Leantime retrying onto the message queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reworked after review of PR-327. The async transport now declares its own retry_strategy instead of inheriting Symfony's 1s/2s/4s, which are shorter than any rate-limit window — a page that drew a 429 spent all three attempts inside the window that produced it. The RetryableHttpClient is gone: PR-325 and PR-326 already narrowed the handlers so a 429 reaches the transport on its own, and retrying underneath the queue only hides the failure from it. Leantime gets a client of its own at timeout 5 / max_duration 30 rather than raising framework.http_client.default_options, so nothing else inherits the numbers, and a ThrottlingHttpClient over a sliding_window limiter spends the rate budget before the request instead of reacting to a 429 afterwards. The 300s max_duration is no longer needed now that /deleted paginates. A 4xx other than 408/423/425/429 now fails the message immediately rather than burning the retry budget to arrive at the same answer. --- .env | 8 +- CHANGELOG.md | 19 ++ composer.json | 1 + composer.lock | 76 ++++++- config/packages/framework.yaml | 12 -- config/packages/messenger.yaml | 11 + config/packages/rate_limiter.yaml | 15 ++ config/services.yaml | 54 ++--- docker-compose.server.override.yml | 3 +- src/MessageHandler/LeantimeDeleteHandler.php | 5 + src/MessageHandler/LeantimeUpdateHandler.php | 5 + .../RethrowsTransientHttpFailuresTrait.php | 36 ++++ .../Service/LeantimeApiClientTest.php | 79 ++++++++ .../Service/LeantimeApiServiceRetryTest.php | 190 ------------------ .../LeantimeDeleteHandlerTest.php | 48 +++++ .../LeantimeUpdateHandlerTest.php | 52 +++++ 16 files changed, 384 insertions(+), 230 deletions(-) create mode 100644 config/packages/rate_limiter.yaml create mode 100644 src/MessageHandler/RethrowsTransientHttpFailuresTrait.php create mode 100644 tests/Integration/Service/LeantimeApiClientTest.php delete mode 100644 tests/Integration/Service/LeantimeApiServiceRetryTest.php diff --git a/.env b/.env index 11c023a9d..ad99789c8 100644 --- a/.env +++ b/.env @@ -25,9 +25,11 @@ APP_INVOICE_SUPPLIER_ACCOUNT=APP_INVOICE_SUPPLIER_ACCOUNT APP_INVOICE_EXTERNAL_RECEIVER_ACCOUNT= APP_INVOICE_DESCRIPTION_TEMPLATE="Spørgsmål vedrørende fakturaen rettes til %name%, %email%." APP_PROJECT_BILLING_DEFAULT_DESCRIPTION= -# Retry budget for the Leantime data API. -APP_HTTP_CLIENT_RETRY_DELAY_MS=2000 -APP_HTTP_CLIENT_MAX_RETRIES=3 +# Leantime data API throttling, see config/packages/rate_limiter.yaml. PROVISIONAL: nothing in +# Leantime or the data-api plugin documents a published limit, so these are a deliberately +# conservative guess until the real ceiling is measured against a running instance. +APP_LEANTIME_RATE_LIMIT=60 +APP_LEANTIME_RATE_INTERVAL="1 minute" EMAIL_FROM_ADDRESS= ###> itk-dev/openid-connect-bundle ### diff --git a/CHANGELOG.md b/CHANGELOG.md index 28731bddc..3a23ba50b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +* [PR-327](https://github.com/itk-dev/economics/pull/327) + * Gave the `async` transport its own `retry_strategy` instead of leaving it on Symfony's 1s/2s/4s defaults, + which are shorter than any rate-limit window — a page that drew a 429 spent all three attempts inside the + window that produced it. Retrying rate-limited Leantime requests is the transport's job: PR-325 and PR-326 + already narrowed the handlers so only a failure describing the message itself is unrecoverable, which is + what lets a 429 reach the queue at all. + * Throttled the Leantime data API rather than reacting to its 429s. `app.leantime.http_client` is a + `ThrottlingHttpClient` over a `sliding_window` limiter, so the sync spends the budget before the request. + `APP_LEANTIME_RATE_LIMIT` and `APP_LEANTIME_RATE_INTERVAL` are provisional — nothing in Leantime or the + data-api plugin documents a published limit, and no `Retry-After` header has been observed, so the values + are a conservative guess until the real ceiling is measured. + * Bounded Leantime requests with `timeout: 5` and `max_duration: 30` on a client of their own rather than on + `framework.http_client.default_options`, so nothing else inherits them. Symfony caps neither by default and + an uncapped request holds the worker forever, since `messenger:consume` only checks `--time-limit` between + messages. A scoped client cannot express this: scopes key on `base_uri`, and the Leantime one comes from the + `DataProvider` entity at runtime. + * A 4xx other than 408/423/425/429 now fails the message immediately instead of being retried five times to + arrive at the same answer — the endpoint returns 400 for a missing `type` or the retired `deleted` + parameter, and the retry budget cannot rewrite the request. * [PR-334](https://github.com/itk-dev/economics/pull/334) * Paginated the Leantime delete sync, following [data-api#21](https://github.com/ITK-Leantime/data-api/pull/21): `/deleted` now serves one type per request with `start`/`limit`, so `delete()` queues a message per type and diff --git a/composer.json b/composer.json index e3364ba0d..633af2430 100644 --- a/composer.json +++ b/composer.json @@ -34,6 +34,7 @@ "symfony/messenger": "~7.4.0", "symfony/mime": "~7.4.0", "symfony/monolog-bundle": "^3.8", + "symfony/rate-limiter": "~7.4.0", "symfony/runtime": "~7.4.0", "symfony/security-bundle": "~7.4.0", "symfony/stimulus-bundle": "^2.10", diff --git a/composer.lock b/composer.lock index 76cb9c7ee..b42e7a074 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "576878c6146b0f5528a3bdcdcd5b3988", + "content-hash": "0eb2aa34db4153bf8cbd76a326be9a36", "packages": [ { "name": "beberlei/doctrineextensions", @@ -7788,6 +7788,80 @@ ], "time": "2026-03-24T13:12:05+00:00" }, + { + "name": "symfony/rate-limiter", + "version": "v7.4.16", + "source": { + "type": "git", + "url": "https://github.com/symfony/rate-limiter.git", + "reference": "6703d040ab83401b27f3c7b29ff4c8c8106fedba" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/symfony/rate-limiter/zipball/6703d040ab83401b27f3c7b29ff4c8c8106fedba", + "reference": "6703d040ab83401b27f3c7b29ff4c8c8106fedba", + "shasum": "" + }, + "require": { + "php": ">=8.2", + "symfony/options-resolver": "^7.3|^8.0" + }, + "require-dev": { + "psr/cache": "^1.0|^2.0|^3.0", + "symfony/lock": "^6.4|^7.0|^8.0" + }, + "type": "library", + "autoload": { + "psr-4": { + "Symfony\\Component\\RateLimiter\\": "" + }, + "exclude-from-classmap": [ + "/Tests/" + ] + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "Wouter de Jong", + "email": "wouter@wouterj.nl" + }, + { + "name": "Symfony Community", + "homepage": "https://symfony.com/contributors" + } + ], + "description": "Provides a Token Bucket implementation to rate limit input and output in your application", + "homepage": "https://symfony.com", + "keywords": [ + "limiter", + "rate-limiter" + ], + "support": { + "source": "https://github.com/symfony/rate-limiter/tree/v7.4.16" + }, + "funding": [ + { + "url": "https://symfony.com/sponsor", + "type": "custom" + }, + { + "url": "https://github.com/fabpot", + "type": "github" + }, + { + "url": "https://github.com/nicolas-grekas", + "type": "github" + }, + { + "url": "https://tidelift.com/funding/github/packagist/symfony/symfony", + "type": "tidelift" + } + ], + "time": "2026-08-07T15:34:54+00:00" + }, { "name": "symfony/routing", "version": "v7.4.12", diff --git a/config/packages/framework.yaml b/config/packages/framework.yaml index 635f8d635..12bcef43d 100644 --- a/config/packages/framework.yaml +++ b/config/packages/framework.yaml @@ -17,18 +17,6 @@ framework: php_errors: log: true - http_client: - default_options: - # Symfony caps neither by default, so a Leantime instance that accepts the connection and - # then stalls holds the messenger worker indefinitely: --time-limit is only checked - # between messages, never during one. - # - # timeout is the idle gap between chunks; max_duration is the whole request. The latter - # has to clear the /deleted endpoint, which the Leantime plugin does not paginate, so - # `sync-deleted --interval=P1W` pulls a week of history in one response. - timeout: 30 - max_duration: 300 - # see https://symfony.com/doc/current/deployment/proxies.html trusted_headers: [ diff --git a/config/packages/messenger.yaml b/config/packages/messenger.yaml index 23b565cc6..16be38c56 100644 --- a/config/packages/messenger.yaml +++ b/config/packages/messenger.yaml @@ -8,6 +8,17 @@ framework: # @see https://symfony.com/doc/current/messenger.html#transport-configuration async: dsn: "%env(MESSENGER_TRANSPORT_DSN)%" + # Spelled out rather than left to Symfony's 1s/2s/4s defaults, which are shorter than + # any rate-limit window: a sync page that draws a 429 would spend all three attempts + # inside the window that produced it. This is where retrying belongs — the handlers + # only mark a failure unrecoverable when it describes the message itself, so anything + # transient reaches the transport. + retry_strategy: + max_retries: 5 + delay: 10000 + multiplier: 3 + # 10s, 30s, 90s, 270s, 600s — a little under 17 minutes in total. + max_delay: 600000 # @see https://symfony.com/doc/current/messenger.html#saving-retrying-failed-messages failed: dsn: "%env(MESSENGER_TRANSPORT_DSN_FAILED)%" diff --git a/config/packages/rate_limiter.yaml b/config/packages/rate_limiter.yaml new file mode 100644 index 000000000..c8736a3c3 --- /dev/null +++ b/config/packages/rate_limiter.yaml @@ -0,0 +1,15 @@ +# @see https://symfony.com/doc/current/rate_limiter.html +framework: + rate_limiter: + # Throttles outgoing calls to the Leantime data API, see app.leantime.http_client. + # + # A sliding window rather than a token bucket: the limit we are respecting is "no more than + # n requests in the last interval", and a bucket would let a full sync spend a whole refilled + # burst at once, which is exactly the shape that draws a 429. + # + # framework.lock is configured, so the limiter takes a lock and the window is shared across + # every worker and cron process rather than counted per process. + leantime_api: + policy: sliding_window + limit: "%env(int:APP_LEANTIME_RATE_LIMIT)%" + interval: "%env(string:APP_LEANTIME_RATE_INTERVAL)%" diff --git a/config/services.yaml b/config/services.yaml index 9d0f6bcfc..77cf2bb20 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -65,33 +65,41 @@ services: arguments: $monitoringUrl: "%env(string:SYNC_MONITORING_URL)%" - # Leantime rate-limits its data API, and a full sync makes hundreds of paged requests. A 429 - # surfaces inside updateAsJob() before the next page is queued, so without retrying, one of them - # ends the whole pagination chain. Restores the RetryableHttpClient that was dropped along with - # the old Jira stack, while the deploy comment still assumes rate limiting is handled. - Symfony\Component\HttpClient\Retry\RetryStrategyInterface: - class: Symfony\Component\HttpClient\Retry\GenericRetryStrategy - # Fetched directly by LeantimeApiServiceRetryTest, which drives the configured strategy - # against mock responses rather than asserting on a copy of the numbers below. - public: true + # Leantime rate-limits its data API and a full sync makes hundreds of paged requests, so the + # sync spends its time waiting on one host. Everything below configures that one caller; + # framework.http_client.default_options stays untouched so nothing else inherits these numbers. + # + # A scoped client cannot express this: scopes key on base_uri, and the Leantime base URI comes + # from the DataProvider entity at runtime, not from config. + app.leantime.rate_limiter: + class: Symfony\Component\RateLimiter\LimiterInterface + # Spending the budget before the request rather than reacting to a 429 afterwards. Shared + # by every Leantime call, since they all compete for the same limit. + factory: ["@limiter.leantime_api", create] + + app.leantime.http_client.options: + class: Symfony\Contracts\HttpClient\HttpClientInterface + factory: ["@http_client", withOptions] arguments: - # A flat list retries on any HTTP method. GenericRetryStrategy's own defaults restrict - # transport errors (0) and 500/504/507/510 to idempotent methods, which excludes the - # POSTs the Leantime data API uses even for reads — every one of these calls is a read. - $statusCodes: [0, 423, 425, 429, 500, 502, 503, 504, 507, 510] - $delayMs: "%env(int:APP_HTTP_CLIENT_RETRY_DELAY_MS)%" - $multiplier: 2.0 - # RetryableHttpClient prefers the response's Retry-After header over this backoff, so the - # cap only bounds the cases where Leantime sends no hint. - $maxDelayMs: 60000 + # timeout is the idle gap between chunks, not the whole request, so 5s is generous for an + # API that answers in milliseconds. max_duration bounds one page; if a page ever needs + # longer, lower LeantimeApiService::LIMIT rather than raising this. Symfony caps neither + # by default, and an uncapped request holds the worker forever: messenger:consume only + # checks --time-limit between messages, never during one. + - { timeout: 5, max_duration: 30 } + # Deliberately not a RetryableHttpClient. Every Leantime fetch is driven by a message the async + # transport already retries, and retrying underneath the queue only hides the failure from it. app.leantime.http_client: - class: Symfony\Component\HttpClient\RetryableHttpClient + class: Symfony\Component\HttpClient\ThrottlingHttpClient + # Not autoconfigured, which would tag it kernel.reset: ThrottlingHttpClient::reset() resets + # the limiter with it, and the worker resets services between messages — the window would be + # wiped after every single request, for every process sharing it. Symfony registers its own + # scoped-client throttling the same untagged way. + autoconfigure: false arguments: - $client: "@http_client" - $strategy: '@Symfony\Component\HttpClient\Retry\RetryStrategyInterface' - $maxRetries: "%env(int:APP_HTTP_CLIENT_MAX_RETRIES)%" - $logger: "@logger" + $client: "@app.leantime.http_client.options" + $rateLimiter: "@app.leantime.rate_limiter" App\Service\LeantimeApiService: arguments: diff --git a/docker-compose.server.override.yml b/docker-compose.server.override.yml index 7fb442eba..c3967169e 100644 --- a/docker-compose.server.override.yml +++ b/docker-compose.server.override.yml @@ -34,7 +34,8 @@ services: - APP_SUPERVISOR_COMMAND=/app/bin/console messenger:consume --env=prod --no-debug --time-limit=900 --failure-limit=1 async - APP_SUPERVISOR_WORKERS=1 - APP_SUPERVISOR_USER=deploy - # Sync job is rate limited by LeanTime API so it will take some time to complete. + # A backstop only: --time-limit above ends the process first, and the Leantime client caps a + # single request at max_duration: 30s, so nothing inside one message runs anywhere near this. - PHP_MAX_EXECUTION_TIME=1800 - PHP_MEMORY_LIMIT=512M - PHP_TIMEZONE=UTC diff --git a/src/MessageHandler/LeantimeDeleteHandler.php b/src/MessageHandler/LeantimeDeleteHandler.php index 63fd8ed33..5ebd13355 100644 --- a/src/MessageHandler/LeantimeDeleteHandler.php +++ b/src/MessageHandler/LeantimeDeleteHandler.php @@ -8,10 +8,13 @@ use Psr\Log\LoggerInterface; use Symfony\Component\Messenger\Attribute\AsMessageHandler; use Symfony\Component\Messenger\Exception\UnrecoverableMessageHandlingException; +use Symfony\Contracts\HttpClient\Exception\ClientExceptionInterface; #[AsMessageHandler] readonly class LeantimeDeleteHandler { + use RethrowsTransientHttpFailuresTrait; + public function __construct( private LoggerInterface $logger, private LeantimeApiService $leantimeApiService, @@ -36,6 +39,8 @@ public function __invoke(LeantimeDeleteMessage $message): void $message->asyncJobQueue, $message->deletedAfter, ); + } catch (ClientExceptionInterface $e) { + $this->rethrowUnlessPermanent($e, $this->logger); } catch (NotFoundException|\TypeError $e) { // Narrow on purpose: see UpsertIssueHandler. Infrastructure failures must propagate. $this->logger->error($e->getMessage()); diff --git a/src/MessageHandler/LeantimeUpdateHandler.php b/src/MessageHandler/LeantimeUpdateHandler.php index d10bb99d0..1f7cba2ff 100644 --- a/src/MessageHandler/LeantimeUpdateHandler.php +++ b/src/MessageHandler/LeantimeUpdateHandler.php @@ -8,10 +8,13 @@ use Psr\Log\LoggerInterface; use Symfony\Component\Messenger\Attribute\AsMessageHandler; use Symfony\Component\Messenger\Exception\UnrecoverableMessageHandlingException; +use Symfony\Contracts\HttpClient\Exception\ClientExceptionInterface; #[AsMessageHandler] readonly class LeantimeUpdateHandler { + use RethrowsTransientHttpFailuresTrait; + public function __construct( private LoggerInterface $logger, private LeantimeApiService $leantimeApiService, @@ -33,6 +36,8 @@ public function __invoke(LeantimeUpdateMessage $message): void $message->modifiedAfter, $message->disableModifiedAtCheck, ); + } catch (ClientExceptionInterface $e) { + $this->rethrowUnlessPermanent($e, $this->logger); } catch (NotFoundException|\TypeError $e) { // Narrow on purpose: see UpsertIssueHandler. A page that fails because Leantime or the // database is unavailable must be retried, not dropped along with the pages after it. diff --git a/src/MessageHandler/RethrowsTransientHttpFailuresTrait.php b/src/MessageHandler/RethrowsTransientHttpFailuresTrait.php new file mode 100644 index 000000000..8da6269a7 --- /dev/null +++ b/src/MessageHandler/RethrowsTransientHttpFailuresTrait.php @@ -0,0 +1,36 @@ +getResponse()->getStatusCode(), self::RETRY_LATER_STATUS_CODES, true)) { + throw $e; + } + + $logger->error($e->getMessage()); + + throw new UnrecoverableMessageHandlingException($e->getMessage()); + } +} diff --git a/tests/Integration/Service/LeantimeApiClientTest.php b/tests/Integration/Service/LeantimeApiClientTest.php new file mode 100644 index 000000000..15dbec757 --- /dev/null +++ b/tests/Integration/Service/LeantimeApiClientTest.php @@ -0,0 +1,79 @@ +assertInstanceOf( + ThrottlingHttpClient::class, + $this->leantimeHttpClient(), + 'Spending the rate limit up front is what keeps a 429 from happening at all.', + ); + } + + /** + * A retry here would be a second retry loop underneath the transport's, invisible to it: the + * queue would see one slow message rather than a rate-limited one, and the backoff the async + * transport is configured with would never get to apply. + */ + public function testLeantimeClientDoesNotRetryUnderneathTheQueue(): void + { + foreach ($this->decoratorChain($this->leantimeHttpClient()) as $layer) { + $this->assertNotInstanceOf( + RetryableHttpClient::class, + $layer, + 'Retrying belongs to the async transport, which retries the message, not the request.', + ); + } + } + + private function leantimeHttpClient(): HttpClientInterface + { + self::bootKernel(); + + $service = self::getContainer()->get(LeantimeApiService::class); + $client = (new \ReflectionProperty($service, 'httpClient'))->getValue($service); + + $this->assertInstanceOf(HttpClientInterface::class, $client); + + return $client; + } + + /** + * The client and everything it decorates. Symfony's decorators all hold the one they wrap in a + * `client` property, so following it walks the whole stack. + * + * @return list + */ + private function decoratorChain(HttpClientInterface $client): array + { + $chain = [$client]; + + while (property_exists($client, 'client')) { + $client = (new \ReflectionProperty($client, 'client'))->getValue($client); + + if (!$client instanceof HttpClientInterface) { + break; + } + + $chain[] = $client; + } + + return $chain; + } +} diff --git a/tests/Integration/Service/LeantimeApiServiceRetryTest.php b/tests/Integration/Service/LeantimeApiServiceRetryTest.php deleted file mode 100644 index 5898534ee..000000000 --- a/tests/Integration/Service/LeantimeApiServiceRetryTest.php +++ /dev/null @@ -1,190 +0,0 @@ - */ - private array $dispatched = []; - - public function testLeantimeClientIsRetryable(): void - { - self::bootKernel(); - - $service = self::getContainer()->get(LeantimeApiService::class); - - $client = (new \ReflectionProperty($service, 'httpClient'))->getValue($service); - - $this->assertInstanceOf( - RetryableHttpClient::class, - $client, - 'LeantimeApiService must get a retrying client, or one 429 ends the pagination chain.', - ); - } - - /** - * The regression itself: a rate-limited page must be retried, not dropped. - */ - public function testRateLimitedPageIsRetriedAndStillPaginates(): void - { - $service = $this->createService([ - new MockResponse('', ['http_code' => 429]), - new MockResponse($this->fullPageJson(), ['http_code' => 200]), - ]); - - $service->updateAsJob(Project::class, 0, self::LIMIT, 1); - - $this->assertCount(self::LIMIT, $this->dispatchedOfType(UpsertProjectMessage::class), 'Every row of the retried page should still be upserted.'); - $this->assertCount(1, $this->dispatchedOfType(LeantimeUpdateMessage::class), 'The chain must continue to the next page.'); - } - - public function testServiceUnavailableIsRetried(): void - { - $service = $this->createService([ - new MockResponse('', ['http_code' => 503]), - new MockResponse($this->fullPageJson(), ['http_code' => 200]), - ]); - - $service->updateAsJob(Project::class, 0, self::LIMIT, 1); - - $this->assertCount(1, $this->dispatchedOfType(LeantimeUpdateMessage::class)); - } - - /** - * The Leantime data API is POST-only, including for reads, and every call this service makes is - * a read. GenericRetryStrategy's own defaults express 500/504/507/510 and transport errors as - * [code => idempotent methods], which excludes POST — so those cases would silently stop being - * retried if the configured list were ever "simplified" back to the defaults. A flat list of - * codes is what makes them apply to POST. - */ - public function testRetryStrategyCoversPostForTransportErrorsAndServerErrors(): void - { - self::bootKernel(); - - $strategy = self::getContainer()->get(RetryStrategyInterface::class); - $this->assertInstanceOf(RetryStrategyInterface::class, $strategy); - - $statusCodes = (array) (new \ReflectionProperty($strategy, 'statusCodes'))->getValue($strategy); - - // 0 is the transport-error slot: connection reset, DNS failure, timeout. - foreach ([0, 429, 500, 502, 503, 504] as $code) { - $this->assertContains( - $code, - $statusCodes, - sprintf('%d must be retried for any method; a [code => methods] entry would skip POST.', $code), - ); - } - } - - /** - * Retrying must not turn a genuinely unavailable API into a silent success: once the retries are - * spent the error still has to surface, so the caller can fail the page visibly. - */ - public function testPersistentRateLimitStillFails(): void - { - $service = $this->createService(array_fill(0, 10, new MockResponse('', ['http_code' => 429]))); - - $this->expectException(HttpExceptionInterface::class); - - $service->updateAsJob(Project::class, 0, self::LIMIT, 1); - } - - /** - * A full page of project rows, so the chain queues a next page. Only the transport behaviour is - * under test, so the payload is fixed and valid. - */ - private function fullPageJson(): string - { - $results = array_map(static fn (int $id) => [ - 'id' => $id, - 'name' => 'Project '.$id, - 'modified' => '2026-07-30 12:00:00', - ], range(1, self::LIMIT)); - - return json_encode([ - 'results' => $results, - 'resultsCount' => count($results), - ], JSON_THROW_ON_ERROR); - } - - /** - * The service under test, wrapping the given mock responses in the retry strategy the container - * actually configures — the point is to exercise the real strategy, not a copy of it. - * - * @param list $responses - */ - private function createService(array $responses): LeantimeApiService - { - self::bootKernel(); - $container = self::getContainer(); - - $strategy = $container->get(RetryStrategyInterface::class); - $this->assertInstanceOf(RetryStrategyInterface::class, $strategy); - - $httpClient = new RetryableHttpClient(new MockHttpClient($responses), $strategy, 3); - - $this->dispatched = []; - $messageBus = $this->createMock(MessageBusInterface::class); - $messageBus->method('dispatch')->willReturnCallback( - function (object $message, array $stamps = []): Envelope { - $this->dispatched[] = $message; - - return new Envelope($message, $stamps); - } - ); - - $dataProvider = new DataProvider(); - $dataProvider->setName('Retry test provider'); - $dataProvider->setEnabled(true); - $dataProvider->setClass(LeantimeApiService::class); - $dataProvider->setUrl('http://leantime.example.com'); - $dataProvider->setSecret('Not so secret'); - - $dataProviderRepository = $this->createMock(DataProviderRepository::class); - $dataProviderRepository->method('find')->willReturn($dataProvider); - - return new LeantimeApiService( - $httpClient, - $messageBus, - $dataProviderRepository, - $this->createMock(EntityManagerInterface::class), - $this->createMock(ProjectRepository::class), - $this->createMock(LoggerInterface::class), - ); - } - - /** - * @return list - */ - private function dispatchedOfType(string $class): array - { - return array_values(array_filter($this->dispatched, static fn (object $m) => $m instanceof $class)); - } -} diff --git a/tests/Unit/MessageHandler/LeantimeDeleteHandlerTest.php b/tests/Unit/MessageHandler/LeantimeDeleteHandlerTest.php index 806f1cf27..4cc981708 100644 --- a/tests/Unit/MessageHandler/LeantimeDeleteHandlerTest.php +++ b/tests/Unit/MessageHandler/LeantimeDeleteHandlerTest.php @@ -8,10 +8,58 @@ use App\Service\LeantimeApiService; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; +use Symfony\Component\HttpClient\Exception\ClientException; +use Symfony\Component\HttpClient\MockHttpClient; +use Symfony\Component\HttpClient\Response\MockResponse; use Symfony\Component\Messenger\Exception\UnrecoverableMessageHandlingException; class LeantimeDeleteHandlerTest extends TestCase { + /** + * The endpoint answers 400 to a missing `type` or the retired `deleted` parameter, and no amount + * of retrying rewrites the request. + */ + public function testInvokeOnPermanentClientErrorThrowsUnrecoverable(): void + { + $message = new LeantimeDeleteMessage(LeantimeApiService::TIMESHEETS, 0, 100, 1, false, null); + + $service = $this->createMock(LeantimeApiService::class); + $service->method('deleteAsJob')->willThrowException($this->clientException(400)); + + $handler = new LeantimeDeleteHandler($this->createMock(LoggerInterface::class), $service); + + $this->expectException(UnrecoverableMessageHandlingException::class); + $handler($message); + } + + public function testInvokeOnRateLimitPropagates(): void + { + $message = new LeantimeDeleteMessage(LeantimeApiService::TIMESHEETS, 0, 100, 1, false, null); + + $service = $this->createMock(LeantimeApiService::class); + $service->method('deleteAsJob')->willThrowException($this->clientException(429)); + + $handler = new LeantimeDeleteHandler($this->createMock(LoggerInterface::class), $service); + + try { + $handler($message); + $this->fail('Expected the rate limit to propagate.'); + } catch (ClientException $e) { + $this->assertSame(429, $e->getResponse()->getStatusCode()); + } + } + + private function clientException(int $statusCode): ClientException + { + $client = new MockHttpClient(new MockResponse('', ['http_code' => $statusCode])); + $response = $client->request('POST', 'https://leantime.example.com/APIData/API/deleted'); + + // Reading the code settles the mock, so ClientException finds the info it formats from. + $response->getStatusCode(); + + return new ClientException($response); + } + public function testInvokeCallsDeleteAsJob(): void { $deletedAfter = new \DateTime('2024-01-01'); diff --git a/tests/Unit/MessageHandler/LeantimeUpdateHandlerTest.php b/tests/Unit/MessageHandler/LeantimeUpdateHandlerTest.php index c6490f326..59d173b29 100644 --- a/tests/Unit/MessageHandler/LeantimeUpdateHandlerTest.php +++ b/tests/Unit/MessageHandler/LeantimeUpdateHandlerTest.php @@ -8,10 +8,62 @@ use App\Service\LeantimeApiService; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; +use Symfony\Component\HttpClient\Exception\ClientException; +use Symfony\Component\HttpClient\MockHttpClient; +use Symfony\Component\HttpClient\Response\MockResponse; use Symfony\Component\Messenger\Exception\UnrecoverableMessageHandlingException; class LeantimeUpdateHandlerTest extends TestCase { + /** + * A 4xx that Leantime will answer the same way next time. Retrying spends the whole backoff to + * arrive back here, so the message is dropped instead. + */ + public function testInvokeOnPermanentClientErrorThrowsUnrecoverable(): void + { + $message = new LeantimeUpdateMessage('App\Entity\Project', 0, 100, 1, false, null); + + $service = $this->createMock(LeantimeApiService::class); + $service->method('updateAsJob')->willThrowException($this->clientException(400)); + + $handler = new LeantimeUpdateHandler($this->createMock(LoggerInterface::class), $service); + + $this->expectException(UnrecoverableMessageHandlingException::class); + $handler($message); + } + + /** + * The regression this branch exists for: a rate-limited page must reach the transport unwrapped, + * so the async retry strategy queues it again instead of the sync ending here. + */ + public function testInvokeOnRateLimitPropagates(): void + { + $message = new LeantimeUpdateMessage('App\Entity\Project', 0, 100, 1, false, null); + + $service = $this->createMock(LeantimeApiService::class); + $service->method('updateAsJob')->willThrowException($this->clientException(429)); + + $handler = new LeantimeUpdateHandler($this->createMock(LoggerInterface::class), $service); + + try { + $handler($message); + $this->fail('Expected the rate limit to propagate.'); + } catch (ClientException $e) { + $this->assertSame(429, $e->getResponse()->getStatusCode()); + } + } + + private function clientException(int $statusCode): ClientException + { + $client = new MockHttpClient(new MockResponse('', ['http_code' => $statusCode])); + $response = $client->request('POST', 'https://leantime.example.com/APIData/API/projects'); + + // Reading the code settles the mock, so ClientException finds the info it formats from. + $response->getStatusCode(); + + return new ClientException($response); + } + public function testInvokeCallsUpdateAsJob(): void { $modifiedAfter = new \DateTime('2024-01-01'); From 466c1c7ee99c900df8d493137ba0d6ab68a55648 Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:43:23 +0000 Subject: [PATCH 5/6] fix: dropped the Leantime throttle; the 429s were fixed in Leantime --- .env | 6 -- CHANGELOG.md | 38 +++++++--- composer.json | 1 - composer.lock | 76 +------------------ config/packages/messenger.yaml | 11 +-- config/packages/rate_limiter.yaml | 15 ---- config/services.yaml | 29 ++----- docker-compose.server.override.yml | 6 +- src/MessageHandler/LeantimeDeleteHandler.php | 1 + src/MessageHandler/LeantimeUpdateHandler.php | 1 + .../RethrowsTransientHttpFailuresTrait.php | 2 +- .../Service/LeantimeApiClientTest.php | 64 +++++++--------- 12 files changed, 75 insertions(+), 175 deletions(-) delete mode 100644 config/packages/rate_limiter.yaml rename src/MessageHandler/{ => Trait}/RethrowsTransientHttpFailuresTrait.php (97%) diff --git a/.env b/.env index ad99789c8..eee164cf8 100644 --- a/.env +++ b/.env @@ -25,11 +25,6 @@ APP_INVOICE_SUPPLIER_ACCOUNT=APP_INVOICE_SUPPLIER_ACCOUNT APP_INVOICE_EXTERNAL_RECEIVER_ACCOUNT= APP_INVOICE_DESCRIPTION_TEMPLATE="Spørgsmål vedrørende fakturaen rettes til %name%, %email%." APP_PROJECT_BILLING_DEFAULT_DESCRIPTION= -# Leantime data API throttling, see config/packages/rate_limiter.yaml. PROVISIONAL: nothing in -# Leantime or the data-api plugin documents a published limit, so these are a deliberately -# conservative guess until the real ceiling is measured against a running instance. -APP_LEANTIME_RATE_LIMIT=60 -APP_LEANTIME_RATE_INTERVAL="1 minute" EMAIL_FROM_ADDRESS= ###> itk-dev/openid-connect-bundle ### @@ -115,4 +110,3 @@ APP_API_KEY='' TIDY_FEEDBACK_WIDGET_URL= TIDY_FEEDBACK_API_KEY= ###< tidy-feedback ### - diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a23ba50b..86fa3c63c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,24 +9,38 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] * [PR-327](https://github.com/itk-dev/economics/pull/327) + * Recorded where the 429s came from, since nothing here did and the answer is not in the data-api plugin. + Leantime core rate-limits every request in `app/Core/Middleware/RequestRateLimiter.php` (v3.9.7): the API + bucket defaults to **100 requests per 60 seconds, keyed on client IP**, and the 429 carries `Retry-After` + plus three `X-RateLimit-*` headers. It reaches `/APIData/API/…` because `IncomingRequest::isApiRequest()` + lowercases the URI and prefix-matches `/api`. It is disabled outright when `app.debug` is true, which is + why development never saw it. **What resolved the 429s was raising `LEAN_RATELIMIT_API` to 10000 on the + Leantime side**, not anything in this repository — a full sync cannot approach that. * Gave the `async` transport its own `retry_strategy` instead of leaving it on Symfony's 1s/2s/4s defaults, - which are shorter than any rate-limit window — a page that drew a 429 spent all three attempts inside the - window that produced it. Retrying rate-limited Leantime requests is the transport's job: PR-325 and PR-326 - already narrowed the handlers so only a failure describing the message itself is unrecoverable, which is - what lets a 429 reach the queue at all. - * Throttled the Leantime data API rather than reacting to its 429s. `app.leantime.http_client` is a - `ThrottlingHttpClient` over a `sliding_window` limiter, so the sync spends the budget before the request. - `APP_LEANTIME_RATE_LIMIT` and `APP_LEANTIME_RATE_INTERVAL` are provisional — nothing in Leantime or the - data-api plugin documents a published limit, and no `Retry-After` header has been observed, so the values - are a conservative guess until the real ceiling is measured. + which are over before anything transient has had time to end: a Leantime restart, a database failover or a + rate-limit window all outlast three attempts inside seven seconds. Now 10s, 30s, 90s, 270s, 600s. This + applies to every message on the transport, not only the Leantime ones. PR-325 and PR-326 already narrowed + the handlers so only a failure describing the message itself is unrecoverable, which is what lets a + transient failure reach the queue at all. + * A 4xx other than 408/423/425/429 now fails the message immediately instead of being retried five times to + arrive at the same answer — the endpoint returns 400 for a missing `type` or the retired `deleted` + parameter, and the retry budget cannot rewrite the request. This is the delete sync's only cover: + `sync-deleted` dispatches on the `sync` transport, which has no retry strategy, and it has to stay there — + what keeps `DELETED_TYPES` in child-before-parent order is the handlers running inline. * Bounded Leantime requests with `timeout: 5` and `max_duration: 30` on a client of their own rather than on `framework.http_client.default_options`, so nothing else inherits them. Symfony caps neither by default and an uncapped request holds the worker forever, since `messenger:consume` only checks `--time-limit` between messages. A scoped client cannot express this: scopes key on `base_uri`, and the Leantime one comes from the `DataProvider` entity at runtime. - * A 4xx other than 408/423/425/429 now fails the message immediately instead of being retried five times to - arrive at the same answer — the endpoint returns 400 for a missing `type` or the retired `deleted` - parameter, and the retry budget cannot rewrite the request. + * Dropped `--failure-limit=1` from the supervisor's `messenger:consume`. `StopWorkerOnFailureLimitListener` + counts every `WorkerMessageFailedEvent`, which the worker dispatches for retryable failures too, so with one + worker configured the first transient error stopped it — now that a retry ladder exists, that is the wrong + thing to count. + * Considered and rejected throttling the client with a `ThrottlingHttpClient` over a rate limiter. Against the + configured 10000/min it would guard nothing, and it pauses inside the message handler, so the single worker + stops draining the queue while it waits. Retrying is also the layer that cannot read the `Retry-After` + Leantime sends — a transport retry strategy never sees the response — but the ladder above outlasts a 60s + window by its third attempt, so the header would not change the outcome. * [PR-334](https://github.com/itk-dev/economics/pull/334) * Paginated the Leantime delete sync, following [data-api#21](https://github.com/ITK-Leantime/data-api/pull/21): `/deleted` now serves one type per request with `start`/`limit`, so `delete()` queues a message per type and diff --git a/composer.json b/composer.json index 633af2430..e3364ba0d 100644 --- a/composer.json +++ b/composer.json @@ -34,7 +34,6 @@ "symfony/messenger": "~7.4.0", "symfony/mime": "~7.4.0", "symfony/monolog-bundle": "^3.8", - "symfony/rate-limiter": "~7.4.0", "symfony/runtime": "~7.4.0", "symfony/security-bundle": "~7.4.0", "symfony/stimulus-bundle": "^2.10", diff --git a/composer.lock b/composer.lock index b42e7a074..76cb9c7ee 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "0eb2aa34db4153bf8cbd76a326be9a36", + "content-hash": "576878c6146b0f5528a3bdcdcd5b3988", "packages": [ { "name": "beberlei/doctrineextensions", @@ -7788,80 +7788,6 @@ ], "time": "2026-03-24T13:12:05+00:00" }, - { - "name": "symfony/rate-limiter", - "version": "v7.4.16", - "source": { - "type": "git", - "url": "https://github.com/symfony/rate-limiter.git", - "reference": "6703d040ab83401b27f3c7b29ff4c8c8106fedba" - }, - "dist": { - "type": "zip", - "url": "https://api.github.com/repos/symfony/rate-limiter/zipball/6703d040ab83401b27f3c7b29ff4c8c8106fedba", - "reference": "6703d040ab83401b27f3c7b29ff4c8c8106fedba", - "shasum": "" - }, - "require": { - "php": ">=8.2", - "symfony/options-resolver": "^7.3|^8.0" - }, - "require-dev": { - "psr/cache": "^1.0|^2.0|^3.0", - "symfony/lock": "^6.4|^7.0|^8.0" - }, - "type": "library", - "autoload": { - "psr-4": { - "Symfony\\Component\\RateLimiter\\": "" - }, - "exclude-from-classmap": [ - "/Tests/" - ] - }, - "notification-url": "https://packagist.org/downloads/", - "license": [ - "MIT" - ], - "authors": [ - { - "name": "Wouter de Jong", - "email": "wouter@wouterj.nl" - }, - { - "name": "Symfony Community", - "homepage": "https://symfony.com/contributors" - } - ], - "description": "Provides a Token Bucket implementation to rate limit input and output in your application", - "homepage": "https://symfony.com", - "keywords": [ - "limiter", - "rate-limiter" - ], - "support": { - "source": "https://github.com/symfony/rate-limiter/tree/v7.4.16" - }, - "funding": [ - { - "url": "https://symfony.com/sponsor", - "type": "custom" - }, - { - "url": "https://github.com/fabpot", - "type": "github" - }, - { - "url": "https://github.com/nicolas-grekas", - "type": "github" - }, - { - "url": "https://tidelift.com/funding/github/packagist/symfony/symfony", - "type": "tidelift" - } - ], - "time": "2026-08-07T15:34:54+00:00" - }, { "name": "symfony/routing", "version": "v7.4.12", diff --git a/config/packages/messenger.yaml b/config/packages/messenger.yaml index 16be38c56..555349484 100644 --- a/config/packages/messenger.yaml +++ b/config/packages/messenger.yaml @@ -8,11 +8,12 @@ framework: # @see https://symfony.com/doc/current/messenger.html#transport-configuration async: dsn: "%env(MESSENGER_TRANSPORT_DSN)%" - # Spelled out rather than left to Symfony's 1s/2s/4s defaults, which are shorter than - # any rate-limit window: a sync page that draws a 429 would spend all three attempts - # inside the window that produced it. This is where retrying belongs — the handlers - # only mark a failure unrecoverable when it describes the message itself, so anything - # transient reaches the transport. + # Spelled out rather than left to Symfony's 1s/2s/4s defaults, which are over before + # anything transient has had time to end: a Leantime restart, a database failover or + # a rate-limit window all outlast three attempts inside seven seconds. This applies + # to every message on the transport, not only the Leantime ones — the handlers mark a + # failure unrecoverable only when it describes the message itself, so anything + # transient reaches the transport and now waits long enough to be worth waiting for. retry_strategy: max_retries: 5 delay: 10000 diff --git a/config/packages/rate_limiter.yaml b/config/packages/rate_limiter.yaml deleted file mode 100644 index c8736a3c3..000000000 --- a/config/packages/rate_limiter.yaml +++ /dev/null @@ -1,15 +0,0 @@ -# @see https://symfony.com/doc/current/rate_limiter.html -framework: - rate_limiter: - # Throttles outgoing calls to the Leantime data API, see app.leantime.http_client. - # - # A sliding window rather than a token bucket: the limit we are respecting is "no more than - # n requests in the last interval", and a bucket would let a full sync spend a whole refilled - # burst at once, which is exactly the shape that draws a 429. - # - # framework.lock is configured, so the limiter takes a lock and the window is shared across - # every worker and cron process rather than counted per process. - leantime_api: - policy: sliding_window - limit: "%env(int:APP_LEANTIME_RATE_LIMIT)%" - interval: "%env(string:APP_LEANTIME_RATE_INTERVAL)%" diff --git a/config/services.yaml b/config/services.yaml index 77cf2bb20..73370ea5d 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -65,19 +65,15 @@ services: arguments: $monitoringUrl: "%env(string:SYNC_MONITORING_URL)%" - # Leantime rate-limits its data API and a full sync makes hundreds of paged requests, so the - # sync spends its time waiting on one host. Everything below configures that one caller; - # framework.http_client.default_options stays untouched so nothing else inherits these numbers. + # A full sync makes hundreds of paged requests to one host, so the Leantime caller gets bounds of + # its own; framework.http_client.default_options stays untouched so nothing else inherits them. # # A scoped client cannot express this: scopes key on base_uri, and the Leantime base URI comes # from the DataProvider entity at runtime, not from config. - app.leantime.rate_limiter: - class: Symfony\Component\RateLimiter\LimiterInterface - # Spending the budget before the request rather than reacting to a 429 afterwards. Shared - # by every Leantime call, since they all compete for the same limit. - factory: ["@limiter.leantime_api", create] - - app.leantime.http_client.options: + # + # Deliberately not a RetryableHttpClient. Every Leantime fetch is driven by a message the async + # transport already retries, and retrying underneath the queue only hides the failure from it. + app.leantime.http_client: class: Symfony\Contracts\HttpClient\HttpClientInterface factory: ["@http_client", withOptions] arguments: @@ -88,19 +84,6 @@ services: # checks --time-limit between messages, never during one. - { timeout: 5, max_duration: 30 } - # Deliberately not a RetryableHttpClient. Every Leantime fetch is driven by a message the async - # transport already retries, and retrying underneath the queue only hides the failure from it. - app.leantime.http_client: - class: Symfony\Component\HttpClient\ThrottlingHttpClient - # Not autoconfigured, which would tag it kernel.reset: ThrottlingHttpClient::reset() resets - # the limiter with it, and the worker resets services between messages — the window would be - # wiped after every single request, for every process sharing it. Symfony registers its own - # scoped-client throttling the same untagged way. - autoconfigure: false - arguments: - $client: "@app.leantime.http_client.options" - $rateLimiter: "@app.leantime.rate_limiter" - App\Service\LeantimeApiService: arguments: $httpClient: "@app.leantime.http_client" diff --git a/docker-compose.server.override.yml b/docker-compose.server.override.yml index c3967169e..d3abfdd34 100644 --- a/docker-compose.server.override.yml +++ b/docker-compose.server.override.yml @@ -31,7 +31,11 @@ services: restart: unless-stopped stop_grace_period: 20s environment: - - APP_SUPERVISOR_COMMAND=/app/bin/console messenger:consume --env=prod --no-debug --time-limit=900 --failure-limit=1 async + # No --failure-limit: StopWorkerOnFailureLimitListener counts every WorkerMessageFailedEvent, + # which the worker dispatches for retryable failures too, so a limit of 1 stopped the only + # worker on the first transient error rather than on anything final. --time-limit still recycles + # the process, and a message that genuinely fails ends up in the failed transport either way. + - APP_SUPERVISOR_COMMAND=/app/bin/console messenger:consume --env=prod --no-debug --time-limit=900 async - APP_SUPERVISOR_WORKERS=1 - APP_SUPERVISOR_USER=deploy # A backstop only: --time-limit above ends the process first, and the Leantime client caps a diff --git a/src/MessageHandler/LeantimeDeleteHandler.php b/src/MessageHandler/LeantimeDeleteHandler.php index 5ebd13355..d494d4225 100644 --- a/src/MessageHandler/LeantimeDeleteHandler.php +++ b/src/MessageHandler/LeantimeDeleteHandler.php @@ -4,6 +4,7 @@ use App\Exception\NotFoundException; use App\Message\LeantimeDeleteMessage; +use App\MessageHandler\Trait\RethrowsTransientHttpFailuresTrait; use App\Service\LeantimeApiService; use Psr\Log\LoggerInterface; use Symfony\Component\Messenger\Attribute\AsMessageHandler; diff --git a/src/MessageHandler/LeantimeUpdateHandler.php b/src/MessageHandler/LeantimeUpdateHandler.php index 1f7cba2ff..93ee0852c 100644 --- a/src/MessageHandler/LeantimeUpdateHandler.php +++ b/src/MessageHandler/LeantimeUpdateHandler.php @@ -4,6 +4,7 @@ use App\Exception\NotFoundException; use App\Message\LeantimeUpdateMessage; +use App\MessageHandler\Trait\RethrowsTransientHttpFailuresTrait; use App\Service\LeantimeApiService; use Psr\Log\LoggerInterface; use Symfony\Component\Messenger\Attribute\AsMessageHandler; diff --git a/src/MessageHandler/RethrowsTransientHttpFailuresTrait.php b/src/MessageHandler/Trait/RethrowsTransientHttpFailuresTrait.php similarity index 97% rename from src/MessageHandler/RethrowsTransientHttpFailuresTrait.php rename to src/MessageHandler/Trait/RethrowsTransientHttpFailuresTrait.php index 8da6269a7..c69a2abda 100644 --- a/src/MessageHandler/RethrowsTransientHttpFailuresTrait.php +++ b/src/MessageHandler/Trait/RethrowsTransientHttpFailuresTrait.php @@ -1,6 +1,6 @@ assertInstanceOf( - ThrottlingHttpClient::class, + self::bootKernel(); + + $this->assertNotSame( + self::getContainer()->get('http_client'), $this->leantimeHttpClient(), - 'Spending the rate limit up front is what keeps a 429 from happening at all.', + 'Drop the explicit $httpClient argument and the service autowires the unbounded shared client.', ); } - /** - * A retry here would be a second retry loop underneath the transport's, invisible to it: the - * queue would see one slow message rather than a rate-limited one, and the backoff the async - * transport is configured with would never get to apply. - */ - public function testLeantimeClientDoesNotRetryUnderneathTheQueue(): void + public function testLeantimeClientIsBounded(): void { - foreach ($this->decoratorChain($this->leantimeHttpClient()) as $layer) { - $this->assertNotInstanceOf( - RetryableHttpClient::class, - $layer, - 'Retrying belongs to the async transport, which retries the message, not the request.', - ); - } + $options = $this->defaultOptions($this->leantimeHttpClient()); + + $this->assertSame(5.0, (float) $options['timeout'], 'Idle gap between chunks.'); + $this->assertSame(30.0, (float) $options['max_duration'], 'Whole request, one page.'); } private function leantimeHttpClient(): HttpClientInterface @@ -55,25 +49,23 @@ private function leantimeHttpClient(): HttpClientInterface } /** - * The client and everything it decorates. Symfony's decorators all hold the one they wrap in a - * `client` property, so following it walks the whole stack. + * The options the client was built with. Symfony's clients keep them in a `defaultOptions` + * property and its decorators hold the one they wrap in `client`, so walking the stack finds + * whichever layer carries them — the profiler wraps this one in test. * - * @return list + * @return array */ - private function decoratorChain(HttpClientInterface $client): array + private function defaultOptions(HttpClientInterface $client): array { - $chain = [$client]; - - while (property_exists($client, 'client')) { - $client = (new \ReflectionProperty($client, 'client'))->getValue($client); + while (!property_exists($client, 'defaultOptions')) { + $this->assertTrue(property_exists($client, 'client'), 'No layer of the client carries default options.'); - if (!$client instanceof HttpClientInterface) { - break; - } + $inner = (new \ReflectionProperty($client, 'client'))->getValue($client); + $this->assertInstanceOf(HttpClientInterface::class, $inner); - $chain[] = $client; + $client = $inner; } - return $chain; + return (new \ReflectionProperty($client, 'defaultOptions'))->getValue($client); } } From f229dbe99c138b36ed54d982d1abd2fb93c534c2 Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Thu, 20 Aug 2026 10:06:24 +0000 Subject: [PATCH 6/6] fix: shortened the retry ladder to 10s/30s/90s --- CHANGELOG.md | 15 ++++++++------- config/packages/messenger.yaml | 20 +++++++++++--------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 86fa3c63c..f0e128a54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,13 +16,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 lowercases the URI and prefix-matches `/api`. It is disabled outright when `app.debug` is true, which is why development never saw it. **What resolved the 429s was raising `LEAN_RATELIMIT_API` to 10000 on the Leantime side**, not anything in this repository — a full sync cannot approach that. - * Gave the `async` transport its own `retry_strategy` instead of leaving it on Symfony's 1s/2s/4s defaults, - which are over before anything transient has had time to end: a Leantime restart, a database failover or a - rate-limit window all outlast three attempts inside seven seconds. Now 10s, 30s, 90s, 270s, 600s. This - applies to every message on the transport, not only the Leantime ones. PR-325 and PR-326 already narrowed - the handlers so only a failure describing the message itself is unrecoverable, which is what lets a - transient failure reach the queue at all. - * A 4xx other than 408/423/425/429 now fails the message immediately instead of being retried five times to + * Respaced the `async` transport's `retry_strategy`. Three attempts was never the problem; 1s/2s/4s was, being + over before anything transient has had time to end. Now 10s, 30s, 90s, so the last attempt lands 130s after + the first failure — past a Leantime restart, a database failover, or a 60s rate-limit window. No wider than + that, because a page only queues its successor once it succeeds: a page waiting to be retried is the whole + entity type's sync waiting with it, against an hourly cron. This applies to every message on the transport, + not only the Leantime ones. PR-325 and PR-326 already narrowed the handlers so only a failure describing the + message itself is unrecoverable, which is what lets a transient failure reach the queue at all. + * A 4xx other than 408/423/425/429 now fails the message immediately instead of being retried three times to arrive at the same answer — the endpoint returns 400 for a missing `type` or the retired `deleted` parameter, and the retry budget cannot rewrite the request. This is the delete sync's only cover: `sync-deleted` dispatches on the `sync` transport, which has no retry strategy, and it has to stay there — diff --git a/config/packages/messenger.yaml b/config/packages/messenger.yaml index 555349484..0b6fce010 100644 --- a/config/packages/messenger.yaml +++ b/config/packages/messenger.yaml @@ -8,18 +8,20 @@ framework: # @see https://symfony.com/doc/current/messenger.html#transport-configuration async: dsn: "%env(MESSENGER_TRANSPORT_DSN)%" - # Spelled out rather than left to Symfony's 1s/2s/4s defaults, which are over before - # anything transient has had time to end: a Leantime restart, a database failover or - # a rate-limit window all outlast three attempts inside seven seconds. This applies - # to every message on the transport, not only the Leantime ones — the handlers mark a - # failure unrecoverable only when it describes the message itself, so anything - # transient reaches the transport and now waits long enough to be worth waiting for. + # Symfony's three attempts are the right number; 1s/2s/4s is the wrong spacing, over + # before anything transient has had time to end. Widened to 10s, 30s, 90s, so the + # last attempt lands 130s after the first failure — past a Leantime restart, a + # database failover, or a 60s rate-limit window. + # + # Not widened further: a page only queues its successor once it succeeds, so a page + # waiting to be retried is the whole entity type's sync waiting with it, against an + # hourly cron. This applies to every message on the transport, not only the Leantime + # ones — the handlers mark a failure unrecoverable only when it describes the message + # itself, so anything transient reaches the transport. retry_strategy: - max_retries: 5 + max_retries: 3 delay: 10000 multiplier: 3 - # 10s, 30s, 90s, 270s, 600s — a little under 17 minutes in total. - max_delay: 600000 # @see https://symfony.com/doc/current/messenger.html#saving-retrying-failed-messages failed: dsn: "%env(MESSENGER_TRANSPORT_DSN_FAILED)%"