feat: add ClickHouse execution storage - #1
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: appwrite#13345 Source head: 4e192b0
⛔ Shipwright · BlockedRecommendation: do not merge PR #1 · Tier
Findings (29)
Fireworks usage: 53,909 input · 3,627 output · 57,536 total tokens · $0.0143 · 26s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| Span::add('execution.id', $execution->getId()); | ||
| Span::add('execution.cancelled', true); | ||
|
|
||
| $executionStore->delete($executionMessage->project->getId(), $execution); |
There was a problem hiding this comment.
Shipwright · CRITICAL
The cancelled execution path deletes the ClickHouse mirror before the database deletion.
Impact: The cancelled execution path deletes the ClickHouse mirror before the database deletion. If deleteDocument returns false or throws, the mirror is already gone while the source of truth still has the execution. A retry cannot heal the mirror because the delete is idempotent only in the database, not in the mirror. This is a concrete data-consistency defect.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| // An unchanged redelivery does not invoke upsertDocuments' callback. | ||
| // Read those rows back so a retry after a ClickHouse failure still heals | ||
| // the mirror and preserves the database-assigned sequence. | ||
| foreach ($final as $execution) { |
There was a problem hiding this comment.
Shipwright · CRITICAL
The fallback read-back loop calls getDocument without verifying the document exists.
Impact: The fallback read-back loop calls getDocument without verifying the document exists. If upsertDocuments returns 0 because the document is missing rather than unchanged, getDocument can throw or return null, causing an unhandled worker failure. The comment assumes a no-op redelivery but the code does not guard against a missing document.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| function (Document $execution) use (&$stored): void { | ||
| $stored = $execution; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Shipwright · CRITICAL
The single final path uses $stored ??= getDocument(...).
Impact: The single final path uses $stored ??= getDocument(...). If upsertDocuments returns 0 because the document was not found, getDocument may throw or return null, and the subsequent executionStore->upsert() receives a null document, causing a type error or mirroring a null record.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| $projectId = $document->getId(); | ||
|
|
||
| $executionStore?->deleteProject($projectId); | ||
|
|
There was a problem hiding this comment.
Shipwright · CRITICAL
deleteProject() calls executionStore->deleteProject() before the try/catch that validates the DSN.
Impact: deleteProject() calls executionStore->deleteProject() before the try/catch that validates the DSN. If the DSN is invalid, the method throws InvalidArgumentException after the ClickHouse data has already been deleted, leaving the project database intact but its execution mirror removed.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| $dbForProject = $getProjectDB($project); | ||
|
|
||
| // Delete Executions | ||
| $executionStore?->deleteBefore($project->getId(), $datetime); |
There was a problem hiding this comment.
Shipwright · CRITICAL
deleteExecutionLogs() deletes from the execution store before the database deletion.
Impact: deleteExecutionLogs() deletes from the execution store before the database deletion. If the database deletion fails, the ClickHouse data is already gone, leaving the two stores inconsistent.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| // delete everything older | ||
| $cutoffTime = $execution->getAttribute('$createdAt'); | ||
|
|
||
| $executionStore?->deleteByResource($project->getId(), $resourceInternalId, $resourceType, $cutoffTime); |
There was a problem hiding this comment.
Shipwright · CRITICAL
deleteExecutionsByLimit() deletes from the execution store before the database deletion.
Impact: deleteExecutionsByLimit() deletes from the execution store before the database deletion. If the database deletion fails, the ClickHouse data is already gone, leaving the two stores inconsistent.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| * Delete Logs | ||
| */ | ||
| Console::info("Deleting logs for site " . $siteId); | ||
| $executionStore?->deleteByResource($project->getId(), (string) $siteInternalId, RESOURCE_TYPE_SITES); |
There was a problem hiding this comment.
Shipwright · CRITICAL
deleteSite() deletes from the execution store before the database deletion.
Impact: deleteSite() deletes from the execution store before the database deletion. If the database deletion fails, the ClickHouse data is already gone, leaving the two stores inconsistent.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| * Delete Executions | ||
| */ | ||
| Console::info("Deleting executions for function " . $functionId); | ||
| $executionStore?->deleteByResource($project->getId(), (string) $functionInternalId, RESOURCE_TYPE_FUNCTIONS); |
There was a problem hiding this comment.
Shipwright · CRITICAL
deleteFunction() deletes from the execution store before the database deletion.
Impact: deleteFunction() deletes from the execution store before the database deletion. If the database deletion fails, the ClickHouse data is already gone, leaving the two stores inconsistent.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| Span::add('execution.id', $execution->getId()); | ||
| Span::add('execution.cancelled', true); | ||
|
|
||
| $executionStore->delete($executionMessage->project->getId(), $execution); |
There was a problem hiding this comment.
Shipwright · HIGH
In the cancelled execution path, '$executionStore->delete()' is called before '$dbForProject->deleteDocument()'.
Impact: In the cancelled execution path, '$executionStore->delete()' is called before '$dbForProject->deleteDocument()'. If the database deletion fails (returns false or throws), the ClickHouse mirror has already been deleted while the source of truth still contains the execution. A retry will then attempt to delete again, but the mirror is already gone, leaving the systems inconsistent. The delete should occur after…
Suggested fix: Fix the review finding before release.
| $executionStore->setup(); | ||
| $health = $executionStore->healthCheck(); | ||
| if (($health['schemaReady'] ?? false) !== true) { | ||
| throw new \RuntimeException('Execution schema health check failed'); |
There was a problem hiding this comment.
Shipwright · HIGH
In UsageSetup.php, the health check for the execution store uses $health['schemaReady'] while the usage connection health check uses $health['healthy'].
Impact: In UsageSetup.php, the health check for the execution store uses $health['schemaReady'] while the usage connection health check uses $health['healthy']. If the execution store's healthCheck() returns a different key (e.g., 'healthy' like the usage connection), the check will always fail and throw a RuntimeException, causing the setup task to fail even when the schema is ready. The diff does not show the Store::healt…
Suggested fix: Fix the review finding before release.
| $projectId = $document->getId(); | ||
|
|
||
| $executionStore?->deleteProject($projectId); | ||
|
|
There was a problem hiding this comment.
Shipwright · HIGH
In Deletes.php, deleteProject() calls $executionStore?->deleteProject($projectId) before the try/catch that validates the DSN.
Impact: In Deletes.php, deleteProject() calls $executionStore?->deleteProject($projectId) before the try/catch that validates the DSN. If the DSN is invalid, the method throws an InvalidArgumentException and the execution store deletion has already occurred, leaving the ClickHouse data deleted while the project database deletion is aborted. This creates an inconsistent state where execution data is removed but the projec…
Suggested fix: Fix the review finding before release.
| // An unchanged redelivery does not invoke upsertDocuments' callback. | ||
| // Read those rows back so a retry after a ClickHouse failure still heals | ||
| // the mirror and preserves the database-assigned sequence. | ||
| foreach ($final as $execution) { |
There was a problem hiding this comment.
Shipwright · HIGH
The fallback read-back loop calls '$dbForProject->getDocument('executions', $execution->getId())' without checking whether the document exists.
Impact: The fallback read-back loop calls '$dbForProject->getDocument('executions', $execution->getId())' without checking whether the document exists. If 'upsertDocuments' returns 0 because the document was not found (rather than unchanged), 'getDocument' may throw or return null, causing an unhandled exception and worker failure. The comment assumes a no-op redelivery, but the code does not verify the document exist…
Suggested fix: Fix the review finding before release.
| $dbForProject = $getProjectDB($project); | ||
|
|
||
| // Delete Executions | ||
| $executionStore?->deleteBefore($project->getId(), $datetime); |
There was a problem hiding this comment.
Shipwright · HIGH
In Deletes.php, deleteExecutionLogs() calls $executionStore?->deleteBefore($project->getId(), $datetime) before the database deletion.
Impact: In Deletes.php, deleteExecutionLogs() calls $executionStore?->deleteBefore($project->getId(), $datetime) before the database deletion. If the database deletion fails (e.g., due to a database exception), the ClickHouse data has already been deleted, leaving the two stores inconsistent. The execution store deletion should occur after the database deletion succeeds or be wrapped in a transaction.
Suggested fix: Fix the review finding before release.
| function (Document $execution) use (&$stored): void { | ||
| $stored = $execution; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Shipwright · HIGH
The single final path uses '$stored ??= $dbForProject->getDocument(...)'.
Impact: The single final path uses '$stored ??= $dbForProject->getDocument(...)'. If 'upsertDocuments' returns 0 because the document was not found, 'getDocument' may throw or return null, and the subsequent '$executionStore->upsert()' will receive a null document, causing a type error or mirroring a null record. The code does not guard against a missing document.
Suggested fix: Fix the review finding before release.
| // delete everything older | ||
| $cutoffTime = $execution->getAttribute('$createdAt'); | ||
|
|
||
| $executionStore?->deleteByResource($project->getId(), $resourceInternalId, $resourceType, $cutoffTime); |
There was a problem hiding this comment.
Shipwright · HIGH
In Deletes.php, deleteExecutionsByLimit() calls $executionStore?->deleteByResource() before the database deletion.
Impact: In Deletes.php, deleteExecutionsByLimit() calls $executionStore?->deleteByResource() before the database deletion. If the database deletion fails, the ClickHouse data has already been deleted, leaving the two stores inconsistent. The execution store deletion should occur after the database deletion succeeds or be wrapped in a transaction.
Suggested fix: Fix the review finding before release.
| * Delete Logs | ||
| */ | ||
| Console::info("Deleting logs for site " . $siteId); | ||
| $executionStore?->deleteByResource($project->getId(), (string) $siteInternalId, RESOURCE_TYPE_SITES); |
There was a problem hiding this comment.
Shipwright · HIGH
In Deletes.php, deleteSite() calls $executionStore?->deleteByResource() before the database deletion.
Impact: In Deletes.php, deleteSite() calls $executionStore?->deleteByResource() before the database deletion. If the database deletion fails, the ClickHouse data has already been deleted, leaving the two stores inconsistent. The execution store deletion should occur after the database deletion succeeds or be wrapped in a transaction.
Suggested fix: Fix the review finding before release.
| * Delete Executions | ||
| */ | ||
| Console::info("Deleting executions for function " . $functionId); | ||
| $executionStore?->deleteByResource($project->getId(), (string) $functionInternalId, RESOURCE_TYPE_FUNCTIONS); |
There was a problem hiding this comment.
Shipwright · HIGH
In Deletes.php, deleteFunction() calls $executionStore?->deleteByResource() before the database deletion.
Impact: In Deletes.php, deleteFunction() calls $executionStore?->deleteByResource() before the database deletion. If the database deletion fails, the ClickHouse data has already been deleted, leaving the two stores inconsistent. The execution store deletion should occur after the database deletion succeeds or be wrapped in a transaction.
Suggested fix: Fix the review finding before release.
| Console::success('[Setup] - Execution schema is ready'); | ||
| break; | ||
| } catch (\Throwable $e) { | ||
| if ($attempts >= $max) { |
There was a problem hiding this comment.
Shipwright · HIGH
The executionStore setup retry loop in app/http.php never finishes the Span when the max attempts are reached.
Impact: The executionStore setup retry loop in app/http.php never finishes the Span when the max attempts are reached. In the catch block, when $attempts >= $max, the code adds a span attribute and breaks, but does not call Span::current()?->finish(). This leaves the 'executions.setup' span open, causing incorrect tracing data and potential resource leaks in the tracing system.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| } | ||
| ); | ||
|
|
||
| // An unchanged redelivery does not invoke upsertDocuments' callback. |
There was a problem hiding this comment.
Shipwright · HIGH
In the batch final path, $stored is keyed by $execution->getId() but then passed as array_values($stored).
Impact: In the batch final path, $stored is keyed by $execution->getId() but then passed as array_values($stored). If upsertDocuments invokes the callback for a document whose ID is not in $final, the array will contain an extra document not originally requested. If two final executions share the same ID, the second overwrites the first, silently dropping one mirror write.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| fn () => new Client((new SwooleClientAdapter())->withConnectionReuse()), | ||
| timeout: 3.0, | ||
| )); | ||
|
|
There was a problem hiding this comment.
Shipwright · HIGH
The executionStore container definition uses _APP_USAGE_PASS for the ClickHouse password instead of a dedicated executions password.
Impact: The executionStore container definition uses _APP_USAGE_PASS for the ClickHouse password instead of a dedicated executions password. The new _APP_CONNECTIONS_DB_EXECUTIONS env var is intended to provide the full DSN, but the fallback default hardcodes the usage password, making the executions connection dependent on an unrelated credential.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| fn () => new Client((new SwooleClientAdapter())->withConnectionReuse()), | ||
| timeout: 3.0, | ||
| )); | ||
|
|
There was a problem hiding this comment.
Shipwright · MEDIUM
The executionStore container definition uses '_APP_USAGE_PASS' for the ClickHouse password instead of a dedicated executions password.
Impact: The executionStore container definition uses '_APP_USAGE_PASS' for the ClickHouse password instead of a dedicated executions password. If the usage password is changed or differs from the executions ClickHouse credentials, the executions store will fail to authenticate. The new '_APP_CONNECTIONS_DB_EXECUTIONS' env var is intended to provide the full DSN, but the fallback default hardcodes the usage password, making…
Suggested fix: Fix the review finding before release.
| Console::success('[Setup] - Execution schema is ready'); | ||
| break; | ||
| } catch (\Throwable $e) { | ||
| if ($attempts >= $max) { |
There was a problem hiding this comment.
Shipwright · MEDIUM
The executionStore setup retry loop in app/http.php never finishes the Span when the max attempts are reached.
Impact: The executionStore setup retry loop in app/http.php never finishes the Span when the max attempts are reached. In the catch block, when $attempts >= $max, the code adds a span attribute and breaks, but does not call Span::current()?->finish(). This leaves the 'executions.setup' span open, causing incorrect tracing data and potential resource leaks in the tracing system.
Suggested fix: Fix the review finding before release.
| } | ||
| ); | ||
|
|
||
| // An unchanged redelivery does not invoke upsertDocuments' callback. |
There was a problem hiding this comment.
Shipwright · MEDIUM
In the batch final path, '$stored' is keyed by '$execution->getId()' but then passed as '\array_values($stored)'.
Impact: In the batch final path, '$stored' is keyed by '$execution->getId()' but then passed as '\array_values($stored)'. If 'upsertDocuments' invokes the callback for a document whose ID is not in '$final' (e.g., a database-assigned or normalized ID), the array will contain an extra document not originally requested. More importantly, if two final executions share the same ID (unlikely but possible with duplicate messag…
Suggested fix: Fix the review finding before release.
| $this->assertSame('execution', $row['id']); | ||
| $this->assertSame(['user:abc'], $row['readRoles']); | ||
| $this->assertSame('waiting', $row['status']); | ||
| $this->assertSame(0, $row['deleted']); |
There was a problem hiding this comment.
Shipwright · LOW
The test asserts that the expiresAt value is '2026-09-08 10:00:00.000', but the input createdAt is '2026-08-25T10:00:00.000+00:00'.
Impact: The test asserts that the expiresAt value is '2026-09-08 10:00:00.000', but the input createdAt is '2026-08-25T10:00:00.000+00:00'. The expected expiration is 14 days later, which would be 2026-09-08, so this assertion is correct. However, the test does not verify the actual TTL calculation logic in the Store implementation, only the output. This is not a defect in the diff.
Suggested fix: Fix the review finding before release.
| $store->expects($this->once()) | ||
| ->method('upsertMany') | ||
| ->willReturnCallback(function (string $projectId, array $executions): void { | ||
| $this->assertSame('project', $projectId); |
There was a problem hiding this comment.
Shipwright · LOW
The test 'testBatchMirrorsDatabaseAssignedSequences' asserts '$executions[0]->getSequence()' equals ''42'' (string), but the stored document sets ''$sequence' => 42' (integer).
Impact: The test 'testBatchMirrorsDatabaseAssignedSequences' asserts '$executions[0]->getSequence()' equals ''42'' (string), but the stored document sets ''$sequence' => 42' (integer). If 'getSequence()' returns the raw integer, the assertion will fail; if it returns a string, the test is misleading about the actual type. Either way, the test does not accurately reflect the expected sequence type.
Suggested fix: Fix the review finding before release.
| '$sequence' => 42, | ||
| 'status' => 'completed', | ||
| ]); | ||
| $dbForProject = $this->createMock(Database::class); |
There was a problem hiding this comment.
Shipwright · LOW
The test 'testBatchRedeliveryHealsClickHouseAfterDatabaseNoOp' mocks 'upsertDocuments' to return 0 and then expects 'getDocument' to be called.
Impact: The test 'testBatchRedeliveryHealsClickHouseAfterDatabaseNoOp' mocks 'upsertDocuments' to return 0 and then expects 'getDocument' to be called. However, the production code only calls 'getDocument' for documents not present in '$stored'. Since the mock returns 0 without invoking the callback, '$stored' remains empty, so the fallback is triggered. This test passes, but it does not verify the actual no-op redelivery s…
Suggested fix: Fix the review finding before release.
1 participant
What does this PR do?
Moves function and site execution persistence toward ClickHouse while keeping the project database as the sole read authority throughout the dual-write phase.
ReplacingMergeTree.There is no ClickHouse read switch, parity sampling, or backfill during dual write.
Test Plan
composer refactor:checkvendor/bin/pint --test <changed PHP files>docker compose config --quietwaiting -> completed -> tombstonesnapshots and sitecompleted -> tombstonesnapshots with request metadata and logs preservedRelated PRs and Issues
Checklist
Source merge-base:
e6a468a11fe70378596d168b0b4e5329b5f18ba2Source head:
4e192b0a5e0e6bc0965d5c15ce5c4d2c1d6d8be6