[codex] Defer Deepwell text block S3 cleanup until commit from PR #1#75
[codex] Defer Deepwell text block S3 cleanup until commit from PR #1#75Rokurolize wants to merge 1 commit into
Conversation
WalkthroughThe PR adds Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5df3dfde-67e7-4586-a72a-08c91565bd62
📒 Files selected for processing (5)
deepwell/src/api.rsdeepwell/src/services/context.rsdeepwell/src/services/mod.rsdeepwell/src/services/text_block/service.rsdeepwell/tests/text_block.rs
| pub async fn run_after_commit(&self, state: &ServerState) { | ||
| let actions = { | ||
| let mut actions = self | ||
| .actions | ||
| .lock() | ||
| .expect("post-commit actions mutex poisoned"); | ||
| std::mem::take(&mut *actions) | ||
| }; | ||
|
|
||
| for action in actions { | ||
| match action { | ||
| PostCommitAction::DeleteTextBlockObjects(filenames) => { | ||
| delete_text_block_objects_after_commit(state, filenames).await; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async fn delete_text_block_objects_after_commit( | ||
| state: &ServerState, | ||
| filenames: Vec<String>, | ||
| ) { | ||
| let bucket = &state.s3_tblocks_bucket; | ||
| for filename in filenames { | ||
| if let Err(error) = bucket.delete_object(&filename).await { | ||
| warn!("Failed to delete committed stale S3 text block {filename}: {error}"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial
Make post-commit cleanup durable if S3 reclamation must be guaranteed.
run_after_commit drains the in-memory queue before awaiting S3 deletes, and delete_text_block_objects_after_commit only logs failures. A process crash, cancellation, or persistent S3 error after DB commit drops the cleanup with no retry, leaving stale objects behind. Consider a durable outbox/cleanup table or background retry path for failed deletes.
| #[tokio::test] | ||
| async fn queued_text_block_cleanup_preserves_objects_when_not_drained() { | ||
| let runner = TestRunner::setup().await; | ||
| let bucket = runner.context().s3_tblocks_bucket(); | ||
| let filename = format!("test-rollback-text-block-{}", Uuid::new_v4()); | ||
|
|
||
| bucket | ||
| .put_object_with_content_type(&filename, b"rollback text block", "text/plain") | ||
| .await | ||
| .expect("test text-block object should upload"); | ||
| assert_object_status(bucket, &filename, 200).await; | ||
|
|
||
| let actions = PostCommitActions::default(); | ||
| actions.delete_text_block_objects([filename.clone()]); | ||
| assert_eq!(actions.pending_count(), 1); | ||
|
|
||
| assert_object_status(bucket, &filename, 200).await; | ||
|
|
||
| bucket | ||
| .delete_object(&filename) | ||
| .await | ||
| .expect("rollback preservation test object should clean up"); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Exercise a real rollback instead of only skipping the drain.
This proves queued actions are inert until run_after_commit, but it would still pass if the JSON-RPC/transaction wrapper accidentally drained after a failed transaction. Add coverage that queues cleanup inside a transaction or RPC call that returns Err, then verifies the object remains after rollback.
Rokurolize
force-pushed
the
fix/pr1-text-block-post-commit-cleanup-20260625
branch
from
ee892fb to
ba5088a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
deepwell/src/services/text_block/service.rs (1)
178-183: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUploads still occur mid-transaction; rollback leaves stale S3 content.
This PR defers deletions to post-commit, but
add_blocksstill uploads new objects viaput_object_with_content_typeinside the transaction. Since keys are derived frompage_id/index/block_type(not content), a PutObject overwrites the existing object; if the outer transaction later rolls back, the DB reverts but S3 retains the new content, so committed rows point at mismatched blobs. This is the inverse of the consistency issue the PR targets — confirm whether it's intentionally out of scope.deepwell/src/api.rs (1)
227-267: 🩺 Stability & Availability | 🟠 MajorDrain post-commit actions in non-RPC contexts too.
add_blocks()queues S3 deletions, butdeepwell/src/services/job/worker.rsanddeepwell/src/database/seeder/mod.rsbuild plainServiceContextvalues and can reach the page-rendering path without anyrun_after_commit()call, so those deletions never execute there.PageService::delete()is covered by the JSON-RPC wrapper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c63898a8-664a-45e3-9d75-1a7d64079f5e
📒 Files selected for processing (5)
deepwell/src/api.rsdeepwell/src/services/context.rsdeepwell/src/services/mod.rsdeepwell/src/services/text_block/service.rsdeepwell/tests/text_block.rs
| if result.is_ok() { | ||
| post_commit_actions.run_after_commit(&db_state).await; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Post-commit deletes run inline before returning the response.
run_after_commit awaits each S3 delete_object sequentially on the request task, so the client response is held until cleanup finishes (and slow/failing S3 inflates latency). Consider spawning the drain on a background task (tokio::spawn) so cleanup doesn't block the response path.
| async fn assert_object_status( | ||
| bucket: &s3::bucket::Bucket, | ||
| filename: &str, | ||
| expected: u16, | ||
| ) { | ||
| let (_head, status) = bucket | ||
| .head_object(filename) | ||
| .await | ||
| .expect("test text-block object HEAD should complete"); | ||
| assert_eq!( | ||
| status, expected, | ||
| "unexpected S3 status for text-block object {filename}", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🌐 Web query:
rust-s3 0.37 Bucket head_object return value for 404 not found object error or status code
💡 Result:
In the rust-s3 crate (version 0.37 and later), the bucket.head_object method returns a Result<(HeadObjectResult, u16), S3Error> [1]. When an object is not found, the S3 service returns a 404 status code, which the crate treats as an error, typically surfacing it as an S3Error::HttpFailWithBody(404, ...) or similar error variant depending on your specific configuration [2][3]. If you specifically need to check for the existence of an object without handling raw S3 errors, it is recommended to use the bucket.object_exists(path) method instead of head_object [3]. This method is designed to return Ok(false) specifically when the server returns a 404 status code, and Ok(true) otherwise [4][3]. Summary of approaches: 1. head_object: Returns an error when the object is missing (e.g., 404 status code) [2][1]. You must catch and inspect the error to identify the 404 status. 2. object_exists: A higher-level helper that abstracts the 404 check, returning Result<bool, S3Error> where false indicates the object was not found [4][3].
Citations:
- 1: https://docs.rs/rust-s3-async/latest/s3/bucket/struct.Bucket.html
- 2: Checking for object existence durch/rust-s3#373
- 3: https://github.com/durch/rust-s3/blob/master/s3/src/bucket.rs
- 4: Avoid retrying object_exists 404 responses durch/rust-s3#458
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test and the rust-s3 dependency version used by the repo.
git ls-files deepwell/tests/text_block.rs Cargo.toml Cargo.lock | sed -n '1,20p'
printf '\n== deepwell/tests/text_block.rs ==\n'
cat -n deepwell/tests/text_block.rs | sed -n '1,180p'
printf '\n== rust-s3 references ==\n'
rg -n 'rust-s3|s3 =' Cargo.toml Cargo.lock deepwell -g 'Cargo.toml' -g 'Cargo.lock' -g '*.toml' -g '*.lock' -g '*.rs'Repository: Rokurolize/wikijump
Length of output: 4148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test and the rust-s3 dependency version used by the repo.
git ls-files deepwell/tests/text_block.rs Cargo.toml Cargo.lock | sed -n '1,20p'
printf '\n== deepwell/tests/text_block.rs ==\n'
cat -n deepwell/tests/text_block.rs | sed -n '1,180p'
printf '\n== rust-s3 references ==\n'
rg -n 'rust-s3|s3 =' Cargo.toml Cargo.lock deepwell -g 'Cargo.toml' -g 'Cargo.lock' -g '*.toml' -g '*.lock' -g '*.rs'Repository: Rokurolize/wikijump
Length of output: 4148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test and the rust-s3 dependency version used by the repo.
git ls-files deepwell/tests/text_block.rs Cargo.toml Cargo.lock | sed -n '1,20p'
printf '\n== deepwell/tests/text_block.rs ==\n'
cat -n deepwell/tests/text_block.rs | sed -n '1,180p'
printf '\n== rust-s3 references ==\n'
rg -n 'rust-s3|s3 =' Cargo.toml Cargo.lock deepwell -g 'Cargo.toml' -g 'Cargo.lock' -g '*.toml' -g '*.lock' -g '*.rs'Repository: Rokurolize/wikijump
Length of output: 4148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files deepwell/tests/text_block.rs Cargo.toml Cargo.lock | sed -n '1,20p'
printf '\n== deepwell/tests/text_block.rs ==\n'
cat -n deepwell/tests/text_block.rs | sed -n '1,180p'
printf '\n== rust-s3 references ==\n'
rg -n 'rust-s3|s3 =' Cargo.toml Cargo.lock deepwell -g 'Cargo.toml' -g 'Cargo.lock' -g '*.toml' -g '*.lock' -g '*.rs'Repository: Rokurolize/wikijump
Length of output: 4148
Handle the 404 case explicitly. rust-s3 0.37 returns Err for a missing object on head_object, so .expect(...) will panic before the 404 assertion. Match the error or use bucket.object_exists(...) for the post-delete check.
|
Closing this draft extraction. CodeRabbit correctly surfaced that deferring deletions alone is not safe on current develop: add_blocks still overwrites fixed S3 keys inside the transaction, so rollback can leave committed DB rows pointing at new blob contents. This needs to be redesigned together with the text-block s3_filename/unique-key work or a broader durable cleanup/outbox path, not landed as an independent small PR. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Extracted from the closed PR #1 completion-closure stack as a focused Deepwell text-block consistency slice.
Scope:
Adaptation note:
Why this is separated:
Validation run locally: