Repository navigation
fix(indexer): report cancelled join tasks as cancelled, not panicked - #1844
Conversation
Thirteen call sites mapped every tokio JoinError to "... task panicked: {e}".
A JoinError is a panic or a cancellation, and the common cancellation is a
spawn_blocking refused by the blocking pool during runtime shutdown. It
logged as "index build task panicked: task N was cancelled", tripping log
checks keyed on "panicked", and the orchestrator warned and scheduled a
retry for a process that was exiting.
- fluree-db-core: task::TaskFailure classifies a JoinError and keeps the
panic payload text; panic_message moves here from the consensus crate.
- fluree-db-indexer: IndexerError::Cancelled via IndexerError::from_join at
the three build sites. The orchestrator resolves waiters as Cancelled and
goes idle instead of backing off; startup re-derives pending work.
- fluree-db-api: import's ten join sites describe the failure faithfully.
bplatz
added
bug
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
✅ Approve with nits.
This looks great, @bplatz — putting the classification in fluree_db_core::task::TaskFailure and letting the indexer, import and consensus each decide what a cancellation means for them is the right altitude, and the orchestrator half is careful in exactly the way it needs to be: a real panic still takes the StorageWrite path it always did, and only a genuine cancellation skips the backoff. I went looking for a way Cancelled could reach the orchestrator while the process is still alive (an abort() on a build handle, a per-ledger cancel, the rebuild's dedicated runtime being torn down) and couldn't construct one: the three handles are spawn_blocking temporaries awaited in place, TriggerHandle::cancel explicitly doesn't abort in-progress builds, the dedicated rebuild runtime is a never-dropped static used only for the inner block_on, and the tokio source at the pinned 1.49.0 confirms the shutdown-spawn shape the description names. The three new cancellation strings can't trip solo's grep "panicked" smoke gates, and solo's origin/main already pins a descendant of this commit.
One thing to sort before merging, which is about the PR's base rather than its code: because it targets #1839's branch, no ci.yml job ran on it (only the release plan job) — and the diff has no code dependency on #1839 at all (the file sets are disjoint and merge-tree onto main is clean). Retargeting to main and pushing (a base edit alone triggers nothing) gets the real clippy --all-features --all-targets, nextest and wasm jobs on it; I ran the equivalents on every touched crate locally and they're clean, but the workspace-wide gates are still unverified until CI sees it. If you'd rather keep the stack order, landing #1839 first and then pushing this one rebased achieves the same thing. The only other note is the optional dedupe of the "settle as cancelled" block at orchestrator.rs:1897, which is pre-existing duplication this PR extends by one.
Adherence to repo commitments:
- Patterns/abstractions: ✔ One shared
TaskFailurein core consumed by three crates;panic_messageconsolidated rather than duplicated;IndexerError::from_joinis the single seam between classification and orchestrator policy. - Performance (speed first, memory second): ✔
JoinErrorpaths only (build failure, import failure, shutdown); no hot path touched. No performance-degradation risk. - Testing: ✔ Eight core tests including the production-shape shutdown spawn, plus the paired orchestrator tests; all run by name under
cargo test -p fluree-db-core --features native/-p fluree-db-indexer --lib, and both go red under a one-line mutation.⚠️ No CI ran because of the base. - Conventions: ✔ Single commit with a full body; fmt clean; clippy
-D warningsclean on core (native,credential), indexer, consensus (raft), api (--lib); consensus also builds withoutraft; wasm32fluree-db-api --no-default-featuresbuilds; noCargo.tomlchange, so core gains no dependency.
Verified locally at branch HEAD (ed59542b3): the tests above by name, two mutations (both red, restored), cargo fmt --all -- --check, package-scoped clippy on every touched crate with CI's feature set, cargo check -p fluree-db-consensus without raft, and cargo check --target wasm32-unknown-unknown -p fluree-db-api --no-default-features.
Approving so you can merge when ready — just be sure to get CI on it first (retarget to main and push, or land #1839 and rebase), since green here right now is absence rather than passage.
| /// shutting down is refused by the blocking pool and its handle resolves | ||
| /// `Cancelled` immediately — nothing panicked. | ||
| #[test] | ||
| fn spawn_blocking_during_shutdown_is_cancelled() { |
There was a problem hiding this comment.
fluree-db-core/src/task.rs:99 — praise. This is the right test for the production shape rather than a generic abort(): it builds a runtime, shutdown_background()s it, and spawns through the cloned Handle, which is exactly the path where tokio's BlockingPool::spawn_task sees shared.shutdown, calls task.shutdown() and hands back an already-cancelled handle (runtime/blocking/pool.rs:393-404 at the pinned 1.49.0). Flipping the Err(_) arm to Panicked fails this test and aborted_task_is_cancelled_not_panicked together, so the classification is pinned from both sides.
| ); | ||
| let mut states = self.states.lock().await; | ||
| if let Some(state) = states.get_mut(ledger_id) { | ||
| state.resolve_waiters_below(i64::MAX, IndexOutcome::Cancelled); |
There was a problem hiding this comment.
fluree-db-indexer/src/orchestrator.rs:1897 — optional. Your code didn't introduce this, but this block — resolve waiters Cancelled, clear pending_min_t and next_retry_at, set Idle — is now the sixth copy of the same shape in this file (:775-778, :800-803, :1266-1269, :1318-1321, here, and :1919-1922 in schedule_retry's cancelled branch), and they've already drifted a little (the panic-isolation copy also clears cancelled; this one leaves a stale last_error from an earlier real failure in place, which the doc comment's "no last_error" slightly overstates since the test starts from a fresh state). I wonder if a tiny LedgerIndexState::settle_cancelled() (or similar) would let all six call one thing, e.g.
impl LedgerIndexState {
/// Resolve every waiter as cancelled and return the ledger to idle.
fn settle_cancelled(&mut self) {
self.resolve_waiters_below(i64::MAX, IndexOutcome::Cancelled);
self.pending_min_t = None;
self.next_retry_at = None;
self.phase = IndexPhase::Idle;
}
}Not hot-path, not a correctness issue, and I recognize it's pre-existing debt this PR only extends — but if you agree it's right, I'd rather see it folded in now than lost in the backlog.
There was a problem hiding this comment.
Addressed in 99a4f43, though only the part of this note I could justify.
The last_error drift you spotted is now a real fix: the cancelled branch clears it, so the doc comment and the admin status API agree. The existing assertion could not have caught that — the test built a fresh state whose last_error was already None — so the test now seeds the state a real cancellation follows (an earlier error plus an armed backoff). With the clearing removed it fails on exactly that assertion.
I left the settle_cancelled() dedupe alone. Reading all six sites, they are not quite one shape: cancel (:775) and cancel_all (:800) set phase = Idle only when the phase is Pending, deliberately leaving an in-progress build running, and :1318 is guarded by has_pending_work(). A helper that sets Idle unconditionally would mark a ledger idle while its build is still in flight. Only :1897 and :1919 are identical. The dedupe that would work is narrower — resolve waiters, clear pending_min_t, clear next_retry_at, with each caller keeping its own phase decision — and I would rather do that deliberately than fold it in here. Happy to file it.
The cancelled branch documented "no last_error" but never cleared one. A ledger that failed a build, entered backoff, and was then cancelled by runtime shutdown settled Idle while still carrying the earlier failure — which the admin status API reports and the trigger-wait warnings log, so the stale string outlived the failure it described. The existing assertion could not catch this: the test built a fresh state whose last_error was already None. It now seeds the failure state a real cancellation follows (an earlier error plus an armed backoff), so the assertion fails if the clearing is removed.
|
Addressed in 51a2010 — CI now runs on this PR, and it is green. #1839 landed, so GitHub retargeted this PR to One thing worth explaining, since the run history shows a red CI on 51a2010. That failure was #1852 raises that deadline along with the other short ones in the suite, and #1851 tracks the 360 s hang, which is deliberately left untuned. |
Two CI failures in the same ten-minute window were timing, not defects: cancelled_commit_never_exposes_the_empty_cache_slot hit its 10 s deadline on PR fluree#1844 (it normally runs in 0.05 s and was green on re-run), and a time-travel test reached nextest's hard kill on main. An in-test deadline should bound a broken path, while the harness bounds hangs: nextest runs slow-timeout = 120s with terminate-after = 3, killing a hung test at 360 s. A 1-10 s in-test deadline is 36-360x less patient than the supervisor behind it, so a stalled runner reads as a failed assertion rather than a slow pass. These deadlines only elapse on failure, so raising them costs nothing on a green run. Covers both idioms present in the suite — an Instant deadline polled in a loop, and tokio::time::timeout around a future — plus the two reqwest connect_timeouts in the raft cluster harness. The cluster client's 10 s request timeout stays as it is, because the SSE test documents a dependency on that read timeout being short. Also widens the elapsed bound in propose_connect_timeout_is_retryable, which measured 5.212 s against a 7 s bound on CI. Ten seconds still proves the 5 s connect budget rather than the 15 s request timeout ended the wait, with room for a loaded runner.
2 participants
Problem
Thirteen call sites across the indexer and import turned any
tokio::task::JoinErrorinto an error reading "… task panicked: {e}". AJoinErroris either a panic or a cancellation, so a cancellation came out as:Nothing panicked. The orchestrator then logged
Indexing failed, will retryand armed a backoff for a process that was exiting. Solo's nightlysmoke-data-adoptfails on any log line containingpanicked, and it hit this intermittently when stopping a file-mode server mid-build.Root cause
The cancellation comes from runtime shutdown. Once shutdown begins, the blocking pool refuses a new
spawn_blockingand completes its handle asCancelledimmediately. The full rebuild runs its pipeline underspawn_blocking, so the awaiting orchestrator sees the error without yielding. That is why the warning and the retry line land back to back as the process exits.Fix
fluree_db_core::task::TaskFailureclassifies aJoinErrorasCancelledorPanicked(message), and keeps the panic payload text for&strandStringpayloads. Only the panic arm ofdescribe()contains "panicked".panic_messagemoves here fromfluree-db-consensus, which now imports it.IndexerError::Cancelled(String)is built throughIndexerError::from_joinat the three indexer build sites. A real panic stays aStorageWriteerror, so its handling is unchanged apart from the label and payload.Cancelled, and sets the ledger idle with no backoff and nolast_error. The next process start re-derives pending work fromcommit_t > index_t. Every other build error keeps the existing warn-and-retry path.TaskFailure::describe, so the wording is correct there too. They remain the sameImportErrorvariants.A panicking build still becomes an error rather than being re-raised with
resume_unwind. That keeps the orchestrator's current isolation behavior.Tests
fluree-db-coretask::tests: an aborted task, aspawn_blockingissued after runtime shutdown (the production shape), and&str,String, and non-string panic payloads.fluree-db-indexerorchestrator:cancelled_build_does_not_schedule_retryand its counterpartfailed_build_schedules_retry. With the cancelled branch disabled, the first test fails.