chore(coprocessor): prevent row-lock contention between execute and upload threads#121
Merged
chore(coprocessor): prevent row-lock contention between execute and upload threads#121
Conversation
96f88c5 to
f37cf04
Compare
a7e7849 to
a6ca1b5
Compare
antoniupop
approved these changes
Jun 12, 2025
a6ca1b5 to
b0f4b33
Compare
b0f4b33 to
d0e7519
Compare
6bf1f8f to
78db39d
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR updates the coprocessor to prevent row-lock contention by moving from using the HandleItem type to a new UploadJob enum, distinguishing between normal uploads and uploads that require a database lock. Key changes include:
- Refactoring various modules (tests, library, executor, and AWS upload) to use UploadJob instead of HandleItem.
- Introducing a cleanup_interval configuration and related garbage collection logic.
- Updating dependency versions (e.g. bytesize) and adding progress logging to enhance monitoring.
Reviewed Changes
Copilot reviewed 11 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| coprocessor/fhevm-engine/sns-executor/src/tests/mod.rs | Changed channel type from HandleItem to UploadJob and added cleanup_interval to test setup. |
| coprocessor/fhevm-engine/sns-executor/src/lib.rs | Replaced reset of ciphertext128 with logging and updated channel types for upload jobs. |
| coprocessor/fhevm-engine/sns-executor/src/executor.rs | Updated task handling to use UploadJob, introduced interval tickers for polling and GC, and added garbage collection. |
| coprocessor/fhevm-engine/sns-executor/src/aws_upload.rs | Refactored S3 upload processing to use UploadJob and enhanced resubmission logic with new intervals. |
| coprocessor/fhevm-engine/sns-executor/Cargo.toml | Updated dependency version and workspace configuration for bytesize. |
| coprocessor/fhevm-engine/fhevm-engine-common/src/tenant_keys.rs | Added logging progress with bandwidth measurement during large object reading and updated dependency version. |
| coprocessor/fhevm-engine/fhevm-engine-common/Cargo.toml & coprocessor/fhevm-engine/Cargo.toml | Synchronized bytesize dependency configuration. |
Files not reviewed (5)
- coprocessor/fhevm-engine/sns-executor/.sqlx/query-455eb6d9fe4e6ffd2fe7f45c2fd668b1e9884a2eff149dd2638089417bad1e95.json: Language not supported
- coprocessor/fhevm-engine/sns-executor/.sqlx/query-5006562c943244b0e6dd278a60e76924ff0e43a08971950550f8c97a6651dfe3.json: Language not supported
- coprocessor/fhevm-engine/sns-executor/.sqlx/query-a006d9ccf85f04ed5c9d65eb759b6e83376343cfc56ad730b15d5fa476d5db37.json: Language not supported
- coprocessor/fhevm-engine/sns-executor/.sqlx/query-a8b64a29e5c7c917a75c5e758ad494cf75ab8291ba6d47366157827b3550cc0f.json: Language not supported
- coprocessor/fhevm-engine/sns-executor/.sqlx/query-fd37e7dc679caa21ba22d7724a27409c232cceb719aa41a71e7faf44d2cb8ef9.json: Language not supported
Comments suppressed due to low confidence (3)
coprocessor/fhevm-engine/sns-executor/src/lib.rs:129
- The removal of the ciphertext reset query is a significant change. Please ensure there is adequate documentation or an inline comment explaining how and when the disk space is reclaimed, so that future maintainers understand this design decision.
// Reset ciphertext128 as the ct128 has been successfully uploaded to S3
coprocessor/fhevm-engine/sns-executor/src/executor.rs:510
- The indefinite loop in try_resubmit relies on batch_size and external cancellation for exit. Consider adding a comment that clarifies its exit conditions or refactoring it into smaller, more manageable chunks for improved readability.
async fn try_resubmit(
coprocessor/fhevm-engine/fhevm-engine-common/src/tenant_keys.rs:260
- [nitpick] The variable name 'timestamp' is ambiguous in the context of tracking log intervals. Consider renaming it to 'last_log_time' to more clearly reflect its purpose.
let mut timestamp = std::time::Instant::now();
…sns-executor Each worker performs write operations on non-shared database tables. The executor do updates to pbs_computations and ciphertexts, while the uploader only updates the ciphertext_digest, thus optimizing overall performance by reducing write conflicts. In addition, the executor does a garbage collecting for any uploaded ct128.
78db39d to
d92b655
Compare
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
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.
The PR ensures the scenario from https://github.com/zama-ai/fhevm-internal/issues/76 is fully handled
fixes: zama-ai/fhevm-internal#76
fixes: zama-ai/fhevm-internal#116