#475 fix(error-handling): harden ETL/worklog error paths against silent failure

[Anonymous]

Originally created by: adityaharishch

Summary

Follow-up audit after the recent pm_worklog stuck-hour fix (#471). Went through src/ looking for the same failure class (state that can wedge silently) plus general error-handling gaps: bare unwrap(), swallowed errors, and missing .context() on the DB/IO path.

• etl/runner.rs: added .context() to every DB/IO call in run_etl — previously the daemon's most crash-sensitive loop propagated bare errors with no indication of which step failed (cursor read, frame batch read, gap insert, block close, cursor advance, etc).
• main.rs: the two etl_tick_span mutex locks used a plain .lock().unwrap(); made them poison-tolerant (unwrap_or_else(|e| e.into_inner())), matching the pattern already used elsewhere in this codebase for the same hazard — a panic anywhere else while holding the lock would otherwise permanently kill future tick spans.
• session_categorizer: partial_cmp(...).unwrap() in the score-winner comparison could panic on a NaN score; switched to total_cmp which is total-order and panic-free.
• pm_worklog/generate.rs: the recovery writes that run after a worklog action fails (mark_error, revert_create, revert_post) were silently swallowing their own failures (let _ = ...). The primary error was already logged, but if the recovery write itself failed, that was invisible. Now logged via tracing::warn!.

Also audited for other "stuck state after crash" bugs analogous to the pm_worklog one — checked etl_runs, day_task_worklogs, and other status-column tables; all either already have startup/tick-time recovery or are dev-only/dead-code paths not reachable in normal daemon operation, so no further table needed a fix.

Test plan

• [x] cargo build
• [x] cargo fmt --check
• [x] cargo clippy --all-targets -- -D warnings
• [x] cargo test --lib (669 passed)
• [x] ETL integration test targets (etl_basic, etl_gaps, etl_session_close, etl_session_text, etl_ui_events, etl_coding_agent_skip)
• [x] pre-push hook suite (fmt, clippy, ui build/tests, security audit, cargo test) — all green

[Anonymous]

Originally posted by: coderabbitai[bot]

[!IMPORTANT]

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d59f44d7-15df-4a59-a9e0-40da22994b07

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:
- [ ] 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

- [ ] Create PR with unit tests - [ ] Commit unit tests in branch `fix/error-handling-hardening`

---

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.

❤️ Share

- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)

_{Comment @coderabbitai help to get the list of available commands.}

[Anonymous]

Originally posted by: Akarsh-Hegde

Overview

Small, well-scoped follow-up to [#471] auditing src/ for the same failure class ("state that can silently wedge") plus general error-handling gaps. Four files touched, +62/-26:

• src/etl/runner.rs: adds .context("…") to every ? on a DB/IO call in run_etl (cursor read, frame batch reads, gap inserts, block closes, cursor advance, run completion).
• src/main.rs: makes the two etl_tick_span.lock() sites poison-tolerant (unwrap_or_else(|e| e.into_inner())).
• src/intelligence/session_categorizer/mod.rs: replaces partial_cmp(...).unwrap() with total_cmp in Scores::winner() to remove a NaN-panic path.
• src/pm_worklog/generate.rs: turns three let _ = ... swallowed recovery writes (mark_error, revert_create, revert_post) into logged failures via tracing::warn!.

No ETL logic changes — only added .context() around existing ? calls, so the documented ETL invariants (gap classification, duration_s excluding gap time, Option C/D, cross-run vs intra-batch gap handling) are untouched. Confirmed no test in tests/etl_*.rs asserts on exact error-message text, so wrapping errors in context strings can't break the suite.

Strengths

• Precedent-consistent fix for the mutex. Checked: unwrap_or_else(|e| e.into_inner()) is the established poison-tolerant pattern already used in src/embedder/mod.rs and three call sites each in src/llm/detect.rs / rate_limit.rs / resolver.rs. This PR correctly extends it to the two etl_tick_span sites in main.rs, and both writer sites were caught (no .lock() on that mutex was missed).
• total_cmp is a real behavior improvement, not just cosmetic. Scores is a fixed [f32; 10] array populated only via add(), so the .expect("scores array is fixed-size and non-empty") replacing .unwrap() is accurate and safe. For all-finite scores, total_cmp and partial_cmp order identically, so there's no regression for the normal path — this is the one actual behavioral change in the diff (everything else is additive: context strings and logging are no-ops for the happy path).
• The three pm_worklog/generate.rs fixes are consistent with existing style in the same function — the file already has an equivalent if let Err(e) = ... pattern a few lines below the last change (linking the day-task card, "best-effort" comment), so this isn't introducing a new idiom, just applying the existing one to previously-silent spots.
• New warn! calls use structured fields (error = %e, task_id, task_key), matching the project's tracing conventions.
• .context() messages name the specific failing step ("failed to read etl_cursor", "failed to insert cross-run gap row", etc.), which is exactly the debuggability this PR is going for.

Issues / Suggestions

1. Minor, non-blocking: the ETL failure-recovery path can still mask the original error. In the if let Err(e) = result branch (runner.rs), both recovery writes now use .context(...)?:
   rust complete_etl_run(meridian, run_id, sessions_closed, Some(&e.to_string())) .await .context("failed to mark etl_runs row failed")?; update_cursor(meridian, last_processed_id, run_id) .await .context("failed to advance etl_cursor after failed run")?; return Err(e);
   If either of these recovery writes itself fails, the function returns that error instead of the original ETL failure e — the same "recovery write masks/loses the real error" hazard this PR fixes on the pm_worklog side by converting let _ = into log-and-continue. Since e is already captured by the warn! immediately above, and a stuck running etl_runs row is reclaimed by cleanup_incomplete_runs on startup, nothing is actually lost today — but for consistency with the PR's own theme, consider logging-and-continuing here too (so return Err(e) always returns the real cause), rather than propagating whichever failure happened last.
2. Test coverage gap (small). total_cmp is the one behavioral change in the diff; a one-line unit test in session_categorizer asserting winner() doesn't panic on a NaN-containing Scores would pin down the fix (currently unverified beyond "doesn't panic in practice").
3. Scope is intentionally partial (not a flaw, just worth flagging for a follow-up). git grep on the PR branch still shows ~20 remaining let _ = ...await swallowed-error sites elsewhere in src/ (intelligence/providers/{jira,github,linear,trello}/mod.rs, main.rs, notices.rs) and roughly 459 bare .unwrap() calls overall. A couple of the swallowed ones — stamp_sync_error/clear_sync_error in the PM-provider sync paths — are the same failure class as the worklog fix here (a failed "record the sync error" write is itself invisible) and look like a natural next target for a follow-up PR.

Risks

• Low risk overall: this is additive error-context/logging, not control-flow change, for 3 of the 4 files. The only semantic change (total_cmp) is a strict improvement over a panic and is provably equivalent for finite inputs.
• The one soft risk is the recovery-path masking noted above (#1) — it's pre-existing behavior (not introduced by this PR) and low-probability (both calls are simple single-row writes to the daemon's own DB), so not a blocker.

Test coverage

• PR reports cargo build, cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo test --lib (669 passed), and the named ETL integration targets (etl_basic, etl_gaps, etl_session_close, etl_session_text, etl_ui_events, etl_coding_agent_skip) plus the full pre-push suite — consistent with the diff being non-invasive to ETL semantics.
• No new tests were added for the total_cmp NaN-panic fix or the new warn! paths in generate.rs; given the small blast radius I'd call this acceptable for a hardening PR of this size, but see suggestion [#2] above.

Overall: a clean, low-risk, well-motivated hardening pass. Non-blocking suggestions above; happy to see this merged as-is if the team doesn't want to scope-creep it further.

[Anonymous]

Originally posted by: adityaharishch

Pushed a follow-up commit addressing both non-blocking suggestions:

1. etl/runner.rs: the failure-recovery writes (complete_etl_run, update_cursor) now log-and-continue on error instead of propagating via ?, so a recovery-write failure can no longer mask/replace the original ETL error e that's returned.
2. Added winner_does_not_panic_on_nan_score in session_categorizer, pinning the total_cmp NaN-safety fix (a NaN score wins outright under total order rather than panicking like partial_cmp would).

[Anonymous]

Ticket changed by: adityaharishch

• status: open --> closed