Conversation
A ClickHouse cluster behind a load balancer answers 503 "no available server" while it has no healthy backend, and refuses connections while a node restarts. Both clear within milliseconds during a failover or a rolling restart, but every blip failed the executions worker job and the API reads that hit it. Send every ClickHouse request through a bounded retry: three attempts, 100ms linear backoff, on connection failures and the availability status codes (429, 502, 503, 504). ClickHouse answers its own query errors with 500, so those still fail on the first attempt. Replaying an insert is safe -- every row carries its own version and ReplacingMergeTree collapses a duplicate snapshot -- and the request body is rewound before each attempt. Co-Authored-By: Claude Opus 5 <[email protected]>
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| try { | ||
| $response = $client->sendRequest($request); | ||
| } catch (Throwable $th) { |
There was a problem hiding this comment.
This catches every Throwable, so deterministic PSR-18 request failures are retried along with temporary network failures. A malformed request or adapter precondition failure is sent three times and delayed by 300 ms before being reported. Limit connection retries to network exceptions and propagate permanent client failures immediately.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/Execution/Store.php
Line: 477
Comment:
**Retries permanent client failures**
This catches every `Throwable`, so deterministic PSR-18 request failures are retried along with temporary network failures. A malformed request or adapter precondition failure is sent three times and delayed by 300 ms before being reported. Limit connection retries to network exceptions and propagate permanent client failures immediately.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| $client = new RecordingClient([ | ||
| new Response(503, body: new Stream('no available server')), | ||
| new Response(503, body: new Stream('no available server')), | ||
| new Response(503, body: new Stream('no available server')), | ||
| ]); |
There was a problem hiding this comment.
This fixture supplies exactly three responses and later asserts exactly three sends, duplicating the production retry limit in the test. The connection-failure and rejected-request tests use the same implementation-coupled pattern. This violates the repository directive to test observable behavior instead of mirroring source configuration. Replace exact send-count assertions with outcomes such as eventual success, preserved payload, and final failure classification. This explicit repository requirement must be satisfied before merging.
Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/Execution/StoreTest.php
Line: 414-418
Comment:
**Tests mirror retry configuration**
This fixture supplies exactly three responses and later asserts exactly three sends, duplicating the production retry limit in the test. The connection-failure and rejected-request tests use the same implementation-coupled pattern. This violates the repository directive to test observable behavior instead of mirroring source configuration. Replace exact send-count assertions with outcomes such as eventual success, preserved payload, and final failure classification. This explicit repository requirement must be satisfied before merging.
**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
This branch has not been deployed
1 participant
What
The executions worker fails ~946 times with
RuntimeException: ClickHouse execution insert failed with HTTP 503: no available server(CLOUD-3R3R).A ClickHouse cluster behind a load balancer answers
503 no available serverwhile it has no healthy backend, and refuses connections outright while a node restarts. Both clear within milliseconds during a failover or a rolling restart, butStore::insertRows()andStore::query()each made a single attempt, so every blip failed the job (and any API read that landed on it).Every ClickHouse request now goes through a bounded retry in
Store::send(): 3 attempts, 100ms linear backoff, on connection failures and the availability status codes (429, 502, 503, 504). ClickHouse answers its own query errors with 500, so a rejected statement still fails on the first attempt instead of being replayed twice for nothing.Replaying an insert is safe: every row carries its own
version, andReplacingMergeTreecollapses a duplicate snapshot (reads resolve withargMaxanyway). The request body is rewound before each attempt, the same way the Swoole client adapter rewinds on redirect.Tests
TDD — the retry tests were written first and failed against the old single-attempt code.
tests/unit/Execution/StoreTest.php:503 no available serverand re-sends the same body503The new
RecordingClientreads each request body at send time, the way a real HTTP client does, so a retry that forgets to rewind the stream shows up as an empty second body.vendor/bin/phpunit tests/unit/Execution/StoreTest.php→ 24 tests, 90 assertions, green.composer lint,composer analyzeand Rector clean on both files. The rest oftests/unit/is unchanged (the failures on this machine are missing extensions — imagick/yaml/maxminddb — and docker/DNS, all pre-existing).🤖 Generated with Claude Code