feat(embeddings): add embeddings lambda function implementation CORE-557#517
Merged
irumvanselme merged 1 commit intoJul 17, 2026
Conversation
irumvanselme
changed the title
irumvanselme
marked this pull request as draft
July 16, 2026 09:58
irumvanselme
force-pushed
the
sub-task/embedding-lambda-function-implementation-CORE-557
branch
4 times, most recently
from
July 16, 2026 13:11
9d1e2c7 to
b260276
Compare
irumvanselme
force-pushed
the
sub-task/embedding-lambda-function-implementation-CORE-557
branch
from
July 16, 2026 13:46
b260276 to
451685a
Compare
rav3n11
marked this pull request as ready for review
July 17, 2026 06:37
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+198
to
+201
| // Clean up the process state so that a failed attempt does not leave an orphaned | ||
| // unfinished record behind, which would block any future embedding process for the model. | ||
| try { | ||
| await this.embeddingProcessStateRepository.deleteById(embeddingProcessState.id); | ||
| } catch (cleanupError) { | ||
| console.error( | ||
| new Error( | ||
| `Failed to clean up the embedding process state ${embeddingProcessState.id} after a failed trigger for model ${modelId}.`, | ||
| { cause: cleanupError } | ||
| ) | ||
| ); | ||
| } | ||
| throw new Error(`Failed to trigger embedding process for model ${modelId}.`, { cause: e }); | ||
| await this.cleanUpProcessState(processId, modelId); | ||
| throw new Error(`Failed to publish embedding tasks for process ${processId} of model ${modelId}.`, { cause: e }); |
There was a problem hiding this comment.
Bug: If publishEmbeddingTasks fails mid-execution, the process state is deleted, leaving entities permanently stuck in a PENDING status with no way to recover.
Severity: HIGH
Suggested Fix
Instead of deleting the process state on failure, update its status to FAILED. This preserves the record for debugging and potential recovery. Additionally, consider adding logic to handle or reset entities stuck in PENDING status when a new embedding process is initiated for the same model.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: backend/src/embeddings/embeddingProcess/embeddingProcess.service.ts#L198-L201
Potential issue: In `publishEmbeddingTasks`, if a failure occurs after entities are
marked as `PENDING` but before the process state is updated, the cleanup logic deletes
the process state record entirely. This leaves the entities permanently stuck in a
`PENDING` status. Subsequent SQS messages for these entities are silently skipped
because their corresponding process state is missing, leaving no clear path for recovery
or reprocessing.
Did we get this right? 👍 / 👎 to inform future reviews.
Comment on lines
+287
to
+294
| entityId: task.entityId, | ||
| embeddingServiceId, | ||
| status: EntityEmbeddingStatus.COMPLETED, | ||
| }); | ||
| await this.recordTaskOutcome(processState, { completedDocuments: 1 }); | ||
| } catch (e: unknown) { | ||
| await this.recordTaskFailure(preparedTask, e); | ||
| } |
There was a problem hiding this comment.
Bug: A race condition in storeAndCompleteTask can incorrectly mark a successfully completed entity as FAILED if a subsequent database operation in recordTaskOutcome fails.
Severity: MEDIUM
Suggested Fix
In the recordTaskFailure function, check the entity's current status before overwriting it. If the status is already COMPLETED, avoid changing it to FAILED. This ensures the entity's final state accurately reflects that the embedding was successful, even if process state accounting fails.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: backend/src/embeddings/service/service.ts#L287-L294
Potential issue: In `storeAndCompleteTask`, an entity's status is set to `COMPLETED`. If
the subsequent call to `recordTaskOutcome` fails due to a database error, the main
`catch` block executes `recordTaskFailure`. This function then overwrites the entity's
status to `FAILED`, creating an inconsistency where a successfully processed entity is
marked as failed. This also causes the process state's `errorCounts` to be incremented
instead of `completedDocuments`, leading to incorrect process tracking.
Did we get this right? 👍 / 👎 to inform future reviews.
irumvanselme
deleted the
sub-task/embedding-lambda-function-implementation-CORE-557
branch
July 17, 2026 07:03
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes:-