Run the state setter on every path reaching a committed manifest - #36
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @LucaCappelletti94, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 2 days and 23 hours by commenting @sourcery-ai review. Upgrade to get a review now.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: LucaCappelletti94/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer's GuideThe upload commit path now re-runs the idempotent content state setter on already committed manifests, preventing metadata rows created by later deduplicated sessions from hanging indefinitely; an integration test verifies the behavior with a fresh ticket and recorded setter invocation. Sequence diagram for re-committing a deduplicated manifestsequenceDiagram
participant Session1
participant Session2
participant UploadAPI
participant Database
participant StateSetter
participant RelayHub
Session1->>UploadAPI: post_commit
UploadAPI->>Database: load_manifest_locked
Database-->>UploadAPI: Uncommitted manifest
UploadAPI->>StateSetter: connetto_set_content_state(content_id, available, caller)
StateSetter->>Database: UPDATE metadata by content_id
StateSetter-->>UploadAPI: success
UploadAPI-->>Session1: 200 OK
Session2->>UploadAPI: post_commit with fresh ticket
UploadAPI->>Database: load_manifest_locked
Database-->>UploadAPI: Committed manifest
UploadAPI->>StateSetter: connetto_set_content_state(content_id, available, caller)
StateSetter->>Database: UPDATE later-session metadata
StateSetter-->>UploadAPI: success
UploadAPI-->>Session2: 200 OK
Database-->>RelayHub: CDC state update
RelayHub-->>Session2: content_state available
Flow diagram for committed manifest state handlingflowchart TD
A[post_commit] --> B[load_manifest_locked]
B --> C{Manifest state}
C -->|Uncommitted| D[verify_file_identity]
D --> E[connetto_set_content_state]
E --> F[200 OK]
C -->|Committed| G[connetto_set_content_state]
G --> H[200 OK]
H --> I[Update metadata by content_id]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #36 +/- ##
=======================================
Coverage 83.24% 83.24%
=======================================
Files 114 114
Lines 25693 25700 +7
Branches 25693 25700 +7
=======================================
+ Hits 21388 21395 +7
+ Misses 3079 3078 -1
- Partials 1226 1227 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8e38ead to
2a5ad5e
Compare
|
1 participant
Running the browser suites two-at-a-time on one stack, as the demo registration in pull request #34 does, exposed a deterministic stall: the first photo suite passed and every content suite after it hung forever waiting for content_state to flip. The cause is in the upload commit path. File identity is a hash of content bytes, so a second session staging identical bytes dedupes against the manifest a previous session committed; post_commit saw ManifestState::Committed, returned 200 and skipped connetto_set_content_state, but the caller's own metadata row for the same content, inserted in this session, was waiting on exactly that call. The relay hub took the 200 as done, no CDC event ever fired for the row, and the waiting tab hung until the test runner killed it. The fix re-runs the setter on the already-committed path; the setter updates by content_id and is idempotent for rows already at the target state.
The regression test in tests/it/upload.rs records every setter call against a replacement function, uploads once through the full intent, chunk and commit flow, then commits again on the committed manifest with a fresh ticket and asserts the setter ran for available with the right caller. It fails on main, passing run shows 1 passed in 6.2s, failing run shows the recorded count at zero, and passes with the fix. The browser-level proof is the exact CI adjacency that deterministically hung before this change, photo_offline then the yew demo photo suite on one shared stack, now green: SMOKE-PASS then YEW-PASS, 2 passed. fmt and clippy -D warnings --all-features --tests are clean for connetto-file-server.
Summary by Sourcery
Re-run the content state setter whenever a commit targets an already-committed manifest so deduplicated sessions complete reliably.
Bug Fixes:
Tests: