test(persistence): make the bulk-submit directives test claim its own manifest - #911
Open
aacruzgon wants to merge 1 commit into
Open
test(persistence): make the bulk-submit directives test claim its own manifest#911aacruzgon wants to merge 1 commit into
aacruzgon wants to merge 1 commit into
Conversation
… manifest `postgres_bulk_submit_import_directives_round_trip` called `claim_next_manifest` and asserted the claimed view carried the directives it had just set. The claim queue is cross-tenant and ordered by `added_at`, the test binary shares one container database, and the batch test that #880 added leaves its manifest as `processing` with no lease — which the claim query treats as an orphan to reclaim. Whenever that manifest was added first, the directives test claimed it instead of its own and failed with `left: []`; main's coverage job has been red on most runs since. The test now claims through `claim_specific_manifest`, the submit-side twin of the export tests' `claim_specific`: loop until the target manifest comes back, hold any foreign lease picked up along the way so it cannot be re-claimed, then release those back to the queue. The batch test's comment no longer presents its `process_entries` call as a defence against concurrent claims. No product code changes. Tests: the two submit tests pass five consecutive runs together; the full postgres_tests binary passes (156).
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Summary
postgres_integration::postgres_bulk_submit_import_directives_round_triphas been failingmain'sCode Coveragejob on 5 of the last 7 runs (andTest Rusttwice) since #880 merged, with:It also fails on PRs that don't touch it (#905, and the docs-only #909). This makes the test claim its own manifest.
Why
The test calls
claim_next_manifestand asserts the claimed view carries the directives it just set. That claim is cross-tenant by design and ordered byadded_at, and the test binary shares one container database. #880 addedpostgres_bulk_submit_batch_commits_bookkeeping_and_contains_errors, which registers a manifest and callsprocess_entrieson it "to move it out ofpending" — butprocess_entriesleaves the manifest asprocessingwith no lease, and the claim query treats an unleasedprocessingmanifest as an orphan to reclaim. So whenever the batch test's manifest was added first, the directives test claims that one (no directives, henceleft: []). Whether it flakes is a race between two tests'add_manifesttimestamps, which is why it fails roughly half the time.Changes
crates/persistence/tests/postgres_tests.rs: newclaim_specific_manifesthelper — the submit-side twin of the existing exportclaim_specific— loopsclaim_next_manifestuntil the target(submission_id, manifest_id)comes back, holding any other lease it picks up (so the loop cannot re-claim the same foreign manifest) and returning those to the queue withreleaseonce the target is held. The directives test uses it with a unique worker id.process_entriescall is a defence against concurrent claims (it isn't); the code is unchanged.No product code changes. Whether the worker's claim query should treat a synchronously-processed, unleased manifest as reclaimable is a separate question for the bulk-submit owners; this PR only stops two tests from racing over it.
Testing
cargo test -p helios-persistence --features postgres --test postgres_tests -- postgres_bulk_submit— 2 passed, five consecutive runs (the two tests run concurrently in the same binary, which is the racing pair).postgres_testsbinary: 156 passed.cargo fmt --all; CI-exact clippy clean.