fix(scheduler): contain panics so a bad cycle skips instead of wedging background refresh - #428
Merged
Merged
Conversation
…g refresh A panic anywhere in the scheduled-analysis path permanently disabled background refresh for the rest of the process lifetime. Mechanism: monitoring.rs claims the gate with `is_checking.swap(true, SeqCst)` BEFORE emitting `scheduled-analysis`; app_setup then ran `run_scheduled_analysis` inside a bare `tauri::async_runtime::spawn` whose JoinHandle is dropped, awaiting `fill_cache_background` unguarded. Every site that clears the gate — `complete_scheduled_check`, the scoring-error arm, both foreground-collision guards — sits AFTER the work. So one unwind skipped all of them and latched the gate at `true`: every later tick saw `is_checking == true` and silently skipped itself. No error, no UI signal — the feed just stopped updating. `void_engine::heartbeat` already carried a "check if monitoring is_checking stuck" probe, i.e. the wedge was a known shape with no recovery path. `headless.rs` had the twin failure: an unwind out of a cycle propagated through the daemon loop and killed `fourda-engine`. - new `task_guard::contain` — catch_unwind wrapper mirroring the existing pattern in analysis_status.rs, logging the panic payload - `run_scheduled_cycle_contained` releases the gate in the recovery arm only. NOT unconditional: clearing after a normal completion could race a tick that already claimed the gate for the next cycle, trading a wedge for a double-run - headless daemon loop survives a panicking cycle and retries next tick; `--once` exits with its documented code 1 instead of an unwind Regression cover proven by falsification: with the containment replaced by a bare `.await`, both panic tests fail (the unwind escapes) while the two success-path tests still pass. Also adopts `clippy::string_slice` for `source_fetching`, the module where two of the 23 byte-slice panics #422 fixed lived. Measured against this tree the lint fires 280 times crate-wide (sources/ 55, ace/ 38, scoring/ 36, content_personalization/ 25), so it is NOT enabled globally — bulk-allowing that backlog would cement any live panic hiding in it. Adopting per-module keeps the guarantee real where ingested text is cut. The two flagged sites collapse into one proven `cap_on_char_boundary` helper; fetcher_tests previously re-implemented the capping inline and so could not have caught the original bug — they now call the real function, plus new multi-byte/emoji boundary cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUeKTKwNmdow8yUk3q8RB2
runyourempire
enabled auto-merge (squash)
August 14, 2026 13:49
runyourempire
added a commit
that referenced
this pull request
Aug 14, 2026
…mmit gate (fleet cannot commit) (#430) ## Why this is urgent `src-tauri/src/analysis_rerank.rs` landed at **1032 lines** in #423, over the **1000-line hard error threshold** in `scripts/check-file-sizes.cjs`. That script exits `1`, and `.husky/pre-commit:38-41` treats a non-zero exit as blocking: ```sh node scripts/check-file-sizes.cjs || { echo "File size check failed. Split large files or add justified exceptions." exit 1 } ``` **Every developer on this repo currently cannot commit anything locally.** This PR restores that. ### How it got in #423 was Rust-only, so CI's `Frontend` job — the only place the file-size check runs — was skipped by the path filter and the gate never fired. The CI path-filter fix is a **separate change owned by another agent**; this PR deliberately does not touch `.github/workflows/`. ## What this does Option A (split), not an exception-list entry. The file had a self-contained 169-line inline `#[cfg(test)] mod rerank_breaker_tests` block at the bottom, which is the cleanest possible seam — it touches **zero production logic**. Moved it into `src-tauri/src/analysis_rerank_tests.rs`, declared with: ```rust #[cfg(test)] #[path = "analysis_rerank_tests.rs"] mod rerank_breaker_tests; ``` This is the established pattern in this crate — **45 other modules** already use `#[cfg(test)] #[path = "*_tests.rs"] mod ...;` with the test file holding the module body directly (e.g. `adversarial.rs:495-497`). Test files get 2x the size limit, so the extracted file sits well inside it. | File | Before | After | Status | |---|---|---|---| | `analysis_rerank.rs` | 1032 | **866** | warn only (non-blocking) | | `analysis_rerank_tests.rs` | — | 166 | test file, exempt from warnings | The module name is unchanged, so test paths stay `analysis_rerank::rerank_breaker_tests::*` — no test-name churn. ## Verification Gate, before and after: ``` # before 1 file(s) exceed error threshold. Split large files or add justified exceptions. EXIT=1 # after 43 file(s) approaching size limits (warnings only). EXIT=0 ``` | Check | Result | |---|---| | `node scripts/check-file-sizes.cjs` | **exit 0** (was exit 1) | | `cargo fmt --check` | clean | | `cargo clippy -- -D warnings` (default) | clean | | `cargo clippy --features experimental -- -D warnings` | clean | | `cargo test --lib` | **4290 passed, 0 failed, 10 ignored** | The test count is **identical** to the pre-change baseline measured on the same tree. Beyond the count, a full test-**name** set diff between the two runs shows **4300 unique names on both sides, 0 added, 0 removed** — proving nothing was orphaned by the module move (a missed `#[path]` declaration would silently drop tests, which a passing run alone would not catch). The production diff is a single hunk starting at line 862; everything above it is byte-for-byte untouched. The extracted body was verified as a byte-exact dedent of the original, with the only difference being rustfmt's re-wrap of the `use super::{...}` block at the reduced indent level. ## Notes for the reviewer - Rebased onto latest `main` (`4e8e197b`, #428). No overlap — #428 does not touch `analysis_rerank.rs`. - Local pre-commit and pre-push hooks both ran in full and passed. No `--no-verify` was used anywhere. (Hooks were invoked via `git -c core.hooksPath=.husky ...` so the worktree ran **its own tracked hooks** rather than the shared tree's `core.hooksPath` absolute path — that is the correct branch's gate, not a bypass.) - `cargo audit` reports a pre-existing `RUSTSEC-2026-0221` (`event-listener` unsound) on main; unrelated to this change and warn-only in the hook. - The exception list in `scripts/check-file-sizes.cjs` was **not** touched. Worth a separate cleanup pass: it reportedly carries 5 stale entries for deleted files and 3 with false justifications. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01AUeKTKwNmdow8yUk3q8RB2 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
The bug
A panic anywhere in the scheduled-analysis path permanently disabled background refresh for the rest of the process lifetime. No error, no UI signal — the feed just quietly stopped updating.
Verified mechanism:
monitoring.rs:1183/:1335claims the gate:is_checking.swap(true, SeqCst)— beforescheduled-analysisis emitted.app_setup.rs:785ranrun_scheduled_analysis(handle)inside a baretauri::async_runtime::spawn. TheJoinHandleis dropped, so nothing ever observes aJoinError.fill_cache_background(&handle).awaitunguarded (app_setup.rs:1985).app_setup.rs:2011,:2109,monitoring.rs:1348,monitoring_notifications.rs:69— sits after the work.So one unwind skipped all four clear sites and latched
is_checkingattrueforever. Every subsequent tick sawis_checking == trueand silently skipped itself.This was already a known shape with no recovery path:
void_engine/heartbeat.rs:158carries the comment "check if monitoring is_checking stuck (simple heuristic)".headless.rs:258had the twin failure — an unwind out of a cycle propagated through the daemon loop and killedfourda-engineoutright.The fix
Contain the panic at the boundary so a bad cycle becomes a skipped cycle, not a permanent wedge.
src-tauri/src/task_guard.rs(new) —contain(), anAssertUnwindSafe(..).catch_unwind()wrapper that logs the panic payload and returnsNone. Mirrors the pattern already proven inanalysis_status.rs:59-68.app_setup.rs:1983-2025—run_scheduled_cycle_contained()wraps the cycle and releases the gate in the recovery arm only.Deliberately not an unconditional clear: clearing after a normal completion could race a tick that has already claimed the gate for the next cycle, trading a wedge for two concurrent scheduled analyses against shared state. The normal paths already release it; only the unwind path was unreachable.
headless.rs:583-599— the daemon loop survives a panicking cycle and retries on the next tick instead of dying.headless.rs:133-144—--onceexits with its documented code1instead of an unwind/abort, so an OS scheduler records a clean failed run.Proving the regression test is real
A test that passes both before and after proves nothing, so this was falsified explicitly: the containment in
run_scheduled_cycle_containedwas temporarily replaced with a barecycle.await(the exact unfixed shape) and the suite re-run.Both panic tests fail against unfixed code (the unwind escapes and kills the test); both success-path tests still pass, confirming they are not just passing by accident. The containment was then restored and all four pass.
The tests use a local
MonitoringStaterather than the process-wide global, so they are hermetic under parallel execution.clippy::string_slice— measured, then scopedThe intent was a crate-wide
string_slice = "deny"on the theory that residual hits would be few after #422. Measured against this tree it fires 280 times, so that premise did not hold:sources/monitoring_briefing.rsace/preemption.rsscoring/diagnostics.rscontent_personalization/The lint is the right one: the only two hits in
source_fetching/are #422's ownfloor_char_boundaryslices, which confirms it would have flagged all 23 original panic sites. But bulk-#[allow]ing 278 pre-existing sites — 55 of them insources/adapters that parse ingested text — would risk cementing a live panic behind an allow that claims safety without an audit. That is the same "large churn" bar this PR was told to apply toindexing_slicing.So it is adopted per-module, starting where the panic class actually lives:
#![deny(clippy::string_slice)]onsrc/source_fetching/mod.rs— the module that caps scraped bodies, and where two of fix(utf8): eliminate 23 string byte-slicing panic vectors on ingested text #422's 23 panics were.cap_on_char_boundary()helper carrying the single char-boundary proof, removing the duplication.fetcher_tests.rspreviously re-implemented the capping logic inline (content[..CONTENT_CAP]), so it asserted against a copy of the logic and could never have caught the original bug. It now calls the real function, plus new multi-byte (3-byteあacross every cap 1..=8) and emoji (4-byte) boundary cases.Cargo.tomlcarries the measurement and the promotion path for whoever audits the backlog.Verification
cargo clippy -- -D warnings(default)cargo clippy --features experimental -- -D warningscargo fmt --checkcargo test --libThe one failure is
db::migrations::recovery_tests::locked_db_returns_recovery_failed_without_quarantine, a timing assertion ("locked DB recovery must return quickly, elapsed 32.7s"). It passes in 2.03s in isolation — starved by parallel load on a busy machine, unrelated to this diff, which touches no DB-migration code.cargo clippy --all-featuresis pre-existing broken on main (~56 errors inteam_sync/webhooks,chacha20poly1305::generate_noncegone from the lockedaead 0.6.1) and was not touched. This diff cannot affect it:source_fetchingcontains zerocfg(feature)code, so the scoped deny behaves identically under every feature combination.Follow-ups deliberately left out
clippy::indexing_slicing— not added, as instructed. Not separately measured; expected to exceed the 280string_slicecount since it covers slices and index ops on all sequence types.string_slicebacklog — needs a real per-site audit, best done module-by-module (sources/55 first — highest ingested-text exposure). Some of those may be live panics of exactly the fix(utf8): eliminate 23 string byte-slicing panic vectors on ingested text #422 kind.spawnsites — scoped out per instructions.contain()is now available for them. Nospawn_guardedwrapper was added: both call sites here need custom recovery (a gate release / an exit code), so a generic spawner would have had zero users and tripped the dead-code doctrine.crash_guardinteraction (pre-existing, worth knowing) —crash_guard.rsinstalls a panic hook that zeroizes API keys on any unwind, including ones caught bycatch_unwind. That already applied to the existing guards inanalysis_status.rsandextractors/pdf.rs; this PR does not change it. But it means a survived scheduled panic leaves the process running with zeroized LLM keys. std's hook API cannot distinguish caught from fatal panics, so fixing it is a separate design question.🤖 Generated with Claude Code
https://claude.ai/code/session_01AUeKTKwNmdow8yUk3q8RB2