Repository navigation
Conversation
Only a lease refusal left start()'s reconcile owed. A listing or HEAD the object store failed, or a catalog error, was recorded and dropped, and a pass whose HEAD or index failed for a pack still returned true and cleared what was owed. With reconcile_interval_ns 0, the default, a pack a crashed process uploaded and never indexed then stayed out of the catalog until the next process start. Every pass that does not finish -- false, or thrown -- is now owed (reconcile_or_owe), start()'s and the loop's alike, and reconcile() returns false when a listed pack could not be read or indexed. The loop runs an owed pass again on a backoff of its own, poll_interval_ns doubled per unfinished pass and capped at max_backoff_ns, so an object that never answers a HEAD costs one bucket listing per max_backoff_ns at most and does not slow the uploads. flush() still never reconciles.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The newly introduced retry path for indexing failures lacks direct regression coverage.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds reliable retry bookkeeping and backoff for incomplete reconciliation passes.
Changes:
- Retries failed startup and loop reconciliation passes.
- Treats HEAD and indexing failures as incomplete passes.
- Adds live regression coverage for listing and HEAD failures.
| File | Description |
|---|---|
native/csrc/catalog/storage_service.cpp |
Implements reconciliation retry state and backoff. |
native/csrc/catalog/storage_service.h |
Declares retry helpers and state. |
tests/test_native_capture_storage_live.py |
Tests startup recovery and HEAD retry backoff. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // (reject) is not left unindexed: no pass could index it. | ||
| std::vector<PackRefData> unindexed; | ||
| if (!missing.empty()) index_bounded(std::move(missing), &unindexed); | ||
| if (!unindexed.empty()) all_indexed = false; |
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.
Fixes the reconcile part of a defect the formal re-check of main @ 5b3b632 found: finding E2 in the
PackPipelineLoopmodel onspecs/formal-models-archive. There,live_startonlyis violated on main andlive_failuresowedholds with this change. Nospecs/files are added here.The defect
Only a lease refusal made a failed start reconcile owed (
storage_service.cpp,sweep_and_reconcile_at_start). A listing or HEAD the object store failed, or a catalog error, was recorded and then dropped.A pass that missed a pack still reported it had finished.
reconcile()returnedtrueeven when a HEAD failed or a pack it found could not be indexed, so the loop clearedreconcile_owed_. Its own comment, "the next pass retries it", held only when a periodic pass was configured.With
reconcile_interval_ns = 0, the default, no later pass ever runs in that process.Scenario: a process crashes after uploading a pack and before indexing it. The next process starts with
reconcile_on_start=True, but the object store blips during its listing. The orphaned pack stays out of the catalog until the next process start, which for a long capture can be hours.The fix
reconcile_or_owe()runs a pass and books it, at start and in the loop alike. A pass that finished clears what was owed. One that returnedfalseor threw is owed, and an exception is rethrown as before.reconcile()returnsfalsewhen the pass did not finish. That now includes a listed pack that could not be HEADed or indexed. A packindex_boundedsets aside (reject) doesn't count, since no pass could index it.poll_interval_nsdoubled per unfinished pass, capped atmax_backoff_ns. That is the same formula as the loop's backoff, but separate from it. So an object that never answers a HEAD costs one bucket listing permax_backoff_nsat most, and the cycle's uploads keep their own pace. The same gate also stops a failing periodic pass from re-listing the bucket every cycle.flush()still never reconciles, sinceallow_reconcileis unchanged.Tests
Two new tests in
tests/test_native_capture_storage_live.py, which needs a live ClickHouse plus the fake S3. Both fail on main.test_a_reconcile_that_failed_at_start_is_run_again_by_the_loop: a crash-window pack is in the bucket and the S3 endpoint is cut whilestart()reconciles. Once it is restored, the loop reconciles the pack in the same process, and the pack reads back. On main this times out.test_a_reconcile_that_missed_a_pack_is_run_again_backing_off: an object answers every HEAD with 403. The pass is re-run 3 to 15 times in 3 s rather than once (main: 1) or every 20 ms poll. The loop keeps its own poll (≥ 50 cycles).Ran locally (ClickHouse 127.0.0.1):
test_native_capture_storage_live.py,test_native_capture_storage_wiring.py,test_native_catalog_lease_live.py,test_native_spool_adoption_live.py,test_native_lease_request_bound.py,test_native_catalog_connection.py,test_native_capture_chain_live.pyandtest_native_live_spool.py(-m 'manual or not manual'): 383 passed, 0 failed.