refactor: replace CLI and migration logError callbacks with spans - #4
Conversation
Source PR: appwrite#13491 Source head: 44fc635
⛔ Shipwright · BlockedRecommendation: do not merge PR #4 · Tier
Findings (18)
Fireworks usage: 17,469 input · 1,862 output · 19,331 total tokens · $0.0051 · 24s · 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}
| array_unshift($newQueries, Query::cursorAfter($latestDocument)); | ||
| } | ||
| $newQueries[] = Query::limit($limit); | ||
| $results = $database->skipValidation(fn () => $database->find($collection, $newQueries)); |
There was a problem hiding this comment.
Shipwright · CRITICAL
Removing the try/catch in Action::foreachDocument changes error semantics: database find failures now propagate instead of being logged and skipped.
Impact: Removing the try/catch in Action::foreachDocument changes error semantics: database find failures now propagate instead of being logged and skipped. Any transient DB error aborts the entire document iteration, potentially leaving partial processing and breaking callers that relied on continuation.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
|
|
||
| $sampler = function (Span $span) use ($traceEnabled, $traceProjectId, $traceFunctionId): bool { | ||
| // Terminal diagnostics must survive tracing filters, including failures | ||
| // before project or function resolution. |
There was a problem hiding this comment.
Shipwright · CRITICAL
The sampler returns true for any span with an error before applying selective tracing filters.
Impact: The sampler returns true for any span with an error before applying selective tracing filters. This bypasses _APP_TRACE_PROJECT_ID/_APP_TRACE_FUNCTION_ID and exports error spans for unrelated projects/functions, leaking error details across tenants.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| array_unshift($newQueries, Query::cursorAfter($latestDocument)); | ||
| } | ||
| $newQueries[] = Query::limit($limit); | ||
| $results = $database->skipValidation(fn () => $database->find($collection, $newQueries)); |
There was a problem hiding this comment.
Shipwright · HIGH
In Action.php, the previous try/catch around '$database->find' swallowed exceptions and invoked the logError callback.
Impact: In Action.php, the previous try/catch around '$database->find' swallowed exceptions and invoked the logError callback. The new code removes the catch entirely and uses 'skipValidation'. Any exception from 'find' now propagates out of 'foreachDocument', changing the observable behavior for callers that previously relied on the error being logged and the loop continuing.
Suggested fix: Fix the review finding before release.
| $this->assertSame(1, $reported); | ||
| $this->assertSame('Mail unavailable', $span->get('warning.message')); | ||
| $this->assertSame('migration-id', $span->get('migration.id')); | ||
| $this->assertNotInstanceOf(\Throwable::class, $span->getError()); |
There was a problem hiding this comment.
Shipwright · HIGH
The test asserts assertNotInstanceOf(Throwable::class, $span->getError()) on a span that was never finished with an error, so the assertion is trivially true and does not verify th
Impact: The test asserts assertNotInstanceOf(Throwable::class, $span->getError()) on a span that was never finished with an error, so the assertion is trivially true and does not verify the warning-reporting behavior it claims to test.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| }); | ||
|
|
||
| $cli->shutdown()->action(function () { | ||
| Span::current()?->finish(); |
There was a problem hiding this comment.
Shipwright · HIGH
CLI init unconditionally calls Span::init("task.$taskName").
Impact: CLI init unconditionally calls Span::init("task.$taskName"). If a span already exists, this creates a duplicate/nested span, and shutdown only finishes the current span, leaving the original span unfinished or misattributing task duration.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| Console::error('[Error] Line: ' . $error->getLine()); | ||
| ->action(function (Throwable $error) use ($workerName) { | ||
| // Initialization can fail before the message span or project is available. | ||
| $span = Span::current() ?? Span::init("worker.{$workerName}"); |
There was a problem hiding this comment.
Shipwright · HIGH
Worker error handler finishes the current span with an error.
Impact: Worker error handler finishes the current span with an error. If the framework expects to finish that span later, this can cause double-finish or loss of subsequent span attributes.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| } catch (\Exception $e) { | ||
| $totalErrors += \count($batch); | ||
| $this->logError($e, $model); | ||
| Span::add('embedding.model', $model); |
There was a problem hiding this comment.
Shipwright · HIGH
Embedding batch error handling overwrites warning.message and warning.code on each failed batch, so only the last error is retained and earlier diagnostic information is lost.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| 'source' => $migration->getAttribute('source') ?? '', | ||
| 'destination' => $migration->getAttribute('destination') ?? '', | ||
| ]; | ||
| Span::add('warning.message', $th->getMessage()); |
There was a problem hiding this comment.
Shipwright · HIGH
Migration error handling now calls Span::add directly without the previous try/catch around reportError.
Impact: Migration error handling now calls Span::add directly without the previous try/catch around reportError. If span storage or add throws, the migration error path itself can throw and escape the catch block.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
|
|
||
| $sampler = function (Span $span) use ($traceEnabled, $traceProjectId, $traceFunctionId): bool { | ||
| // Terminal diagnostics must survive tracing filters, including failures | ||
| // before project or function resolution. |
There was a problem hiding this comment.
Shipwright · MEDIUM
The sampler now returns true for any span with an error before applying the listener or selective tracing filters.
Impact: The sampler now returns true for any span with an error before applying the listener or selective tracing filters. This means terminal error spans bypass the _APP_TRACE_PROJECT_ID/_APP_TRACE_FUNCTION_ID filters and are always exported, which may leak error details for unrelated projects/functions and contradicts the comment that only non-terminal spans are filtered.
Suggested fix: Fix the review finding before release.
| }); | ||
|
|
||
| $cli->shutdown()->action(function () { | ||
| Span::current()?->finish(); |
There was a problem hiding this comment.
Shipwright · MEDIUM
In cli.php, the init action calls 'Span::init("task.$taskName")' unconditionally.
Impact: In cli.php, the init action calls 'Span::init("task.$taskName")' unconditionally. If the CLI task has already started a span elsewhere, this creates a nested/duplicate span and the shutdown action finishes only the current span, potentially leaving the original span unfinished or misattributing the task duration.
Suggested fix: Fix the review finding before release.
| Console::error('[Error] Line: ' . $error->getLine()); | ||
| ->action(function (Throwable $error) use ($workerName) { | ||
| // Initialization can fail before the message span or project is available. | ||
| $span = Span::current() ?? Span::init("worker.{$workerName}"); |
There was a problem hiding this comment.
Shipwright · MEDIUM
The worker error handler now calls '$span->finish(error: $error)' on the current span.
Impact: The worker error handler now calls '$span->finish(error: $error)' on the current span. If the current span is the message span that the worker framework expects to remain open for subsequent processing or for the framework to finish, finishing it here can cause the framework to attempt to finish an already-finished span or lose subsequent span attributes.
Suggested fix: Fix the review finding before release.
| } catch (\Exception $e) { | ||
| $totalErrors += \count($batch); | ||
| $this->logError($e, $model); | ||
| Span::add('embedding.model', $model); |
There was a problem hiding this comment.
Shipwright · MEDIUM
The embedding error handling now calls 'Span::add('warning.message', $e->getMessage())' and 'Span::add('warning.code', $e->getCode())' inside a loop over batches.
Impact: The embedding error handling now calls 'Span::add('warning.message', $e->getMessage())' and 'Span::add('warning.code', $e->getCode())' inside a loop over batches. If multiple batches fail, each iteration overwrites the previous warning message and code, so only the last error is retained in the span, losing diagnostic information for earlier failures.
Suggested fix: Fix the review finding before release.
| 'source' => $migration->getAttribute('source') ?? '', | ||
| 'destination' => $migration->getAttribute('destination') ?? '', | ||
| ]; | ||
| Span::add('warning.message', $th->getMessage()); |
There was a problem hiding this comment.
Shipwright · MEDIUM
The migration error handling now records warnings via 'Span::add' but no longer calls '$this->reportError'.
Impact: The migration error handling now records warnings via 'Span::add' but no longer calls '$this->reportError'. The previous 'reportError' had a try/catch around the logging callback to prevent logging failures from escaping. The new direct 'Span::add' calls are not wrapped, so if the span storage or add operation throws, the migration error path itself can throw and escape the catch block.
Suggested fix: Fix the review finding before release.
| $this->assertSame(1, $reported); | ||
| $this->assertSame('Mail unavailable', $span->get('warning.message')); | ||
| $this->assertSame('migration-id', $span->get('migration.id')); | ||
| $this->assertNotInstanceOf(\Throwable::class, $span->getError()); |
There was a problem hiding this comment.
Shipwright · MEDIUM
The test 'testNotificationFailureIsReportedAsWarning' sets a span storage and initializes a span, but the assertion '$this->assertNotInstanceOf(\Throwable::class, $span->getError()
Impact: The test 'testNotificationFailureIsReportedAsWarning' sets a span storage and initializes a span, but the assertion '$this->assertNotInstanceOf(\Throwable::class, $span->getError())' is always true because the span was never finished with an error. The test does not actually verify that the notification failure is reported as a warning in the way the production code would, making the test misleading.
Suggested fix: Fix the review finding before release.
| return true; | ||
| } | ||
|
|
||
| $warning = $span->get('warning.message') !== null || (int) $span->get('embedding.errors') > 0; |
There was a problem hiding this comment.
Shipwright · LOW
The sampler's warning detection is coupled to an embedding-specific attribute, embedding.errors, inside a generic span initializer.
Impact: The sampler's warning detection is coupled to an embedding-specific attribute, embedding.errors, inside a generic span initializer. This makes the generic tracing path depend on a domain-specific key and is confusing for future maintainers.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| Console::error($error->getMessage() . ' Migration: ' . $migration->getId()); | ||
| $this->reportError($error, $migration); | ||
| Console::error('Invalid initiating user sequence for export migration. Migration: ' . $migration->getId()); | ||
| Span::add('warning.message', 'Invalid initiating user sequence for export migration.'); |
There was a problem hiding this comment.
Shipwright · LOW
resolveExportUser records warning.code as integer 0, but the sampler does not use warning.code to decide warning export.
Impact: resolveExportUser records warning.code as integer 0, but the sampler does not use warning.code to decide warning export. This value appears unused and may mislead readers into thinking it drives warning behavior.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| return true; | ||
| } | ||
|
|
||
| $warning = $span->get('warning.message') !== null || (int) $span->get('embedding.errors') > 0; |
There was a problem hiding this comment.
Shipwright · LOW
The warning detection uses '(int) $span->get('embedding.errors') > 0'.
Impact: The warning detection uses '(int) $span->get('embedding.errors') > 0'. If 'embedding.errors' is not set, 'get' returns null and '(int) null' is 0, which is safe. However, if the value is a non-numeric string, the cast silently becomes 0 and the warning is not emitted. More importantly, the same key is later set as an integer in Create.php, so this is consistent, but the sampler's warning logic is coupled to an…
Suggested fix: Fix the review finding before release.
| Console::error($error->getMessage() . ' Migration: ' . $migration->getId()); | ||
| $this->reportError($error, $migration); | ||
| Console::error('Invalid initiating user sequence for export migration. Migration: ' . $migration->getId()); | ||
| Span::add('warning.message', 'Invalid initiating user sequence for export migration.'); |
There was a problem hiding this comment.
Shipwright · LOW
In 'resolveExportUser', the new code calls 'Span::add('warning.code', 0)' with an integer literal 0.
Impact: In 'resolveExportUser', the new code calls 'Span::add('warning.code', 0)' with an integer literal 0. The sampler's warning detection checks '(int) $span->get('embedding.errors') > 0', but this is a different key. The warning.code value of 0 is not used by the sampler, so these warnings may not be exported as warnings unless another mechanism reads warning.code.
Suggested fix: Fix the review finding before release.
1 participant
What does this PR do?
Remove
logErrorcallback injection from CLI tasks and migration workers. Global CLI/worker error handlers export terminal failures once; recovered migration and embedding failures attach warning attributes usingSpan::add(). The sampler exports warning listener spans and marks recovered failures as warnings without attaching a Throwable or changing successful HTTP responses into Sentry errors. Partial embedding responses also expose the existing error count on the span.CLI success now finishes its operation span, and worker errors before project/span initialization still get an error span. Pagination read failures propagate instead of potentially reprocessing the previous page;
skipValidation()restores validation on both success and failure.Realtime and presence reporting keep their existing handling for this first pass.
Test Plan
versionproduces one success span; an unknown task produces one terminal error span and exits 1.No new HTTP/queue end-to-end drill or staging deployment has been performed. Existing migration HTTP E2E remains a CI gate; partial embedding warning behavior has not been exercised against a live embedding backend.
Related PRs and Issues
Cloud follow-up: https://github.com/appwrite-labs/cloud/pull/5670 (held while this CE change goes first). Cloud overrides
Migrations::action(): remove its logger parameter and forwarded argument when updating its server-ce dependency. Do not bump Cloud to this CE commit without that coordinated signature update.Source merge-base:
231e47ac41b57498f6c92e80ebaaeed3c3c504fbSource head:
44fc6355bfbd90f09631c7997415fad734836c65