Skip to content

fix(observability): drop cron agent-job session-expired events in before_send (TAURI-RUST-M) - #3736

Closed
CodeGhost21 wants to merge 2 commits into
tinyhumansai:mainfrom
CodeGhost21:fix/sentry-80-cron-session-expired-before-send
Closed

CodeGhost21 wants to merge 2 commits into
tinyhumansai:mainfrom
CodeGhost21:fix/sentry-80-cron-session-expired-before-send

Conversation

@CodeGhost21

Copy link
Copy Markdown
Member

Summary

  • Closes a defense-in-depth gap in the Sentry before_send filter is_session_expired_event: the cron domain was excluded, so cron-fired agent-job session-expired noise (Sentry TAURI-RUST-M / Local AI: guided model selection by device capability (tiered presets + user override) #80, 42k+ events) had only the call-site guard protecting it.
  • Adds cron to the filter's domain allowlist and accepts on the session-expired body alone (the cron report_error path carries no status tag, mirroring the existing rpc pre-flight branch).
  • Genuine non-session cron failures still page — suppression requires the body to match is_session_expired_message.

Problem

A scheduled morning_briefing cron agent fires while the user's backend JWT has lapsed. Every retry hits the same global 401, the loop exhausts, and cron::scheduler::execute_job_with_retry calls report_error with domain=cron, operation=agent_job, failure=retries_exhausted, carrying the raw SESSION_EXPIRED: backend session not active — sign in to resume LLM work sentinel. This is expected user state (only fixable by signing in from the UI), not a defect.

The primary fix already shipped — PR #3350 (is_session_expired_failure, in v0.57.18+) halts the retry loop on first occurrence and skips report_error entirely. But the before_send defense-in-depth filter is_session_expired_event was scoped to llm_provider/backend_api/rpc and excluded cron, leaving the call-site guard as the sole protection. Any future cron call site that re-emits the shape without routing through that guard would flood Sentry again.

Note: this hardening helps future builds only — it cannot retroactively silence the live #80 events, which originate from clients on pre-fix builds (98.8% from a single client stuck on 0.56.0). Those go silent once clients update to a build containing #3350; the Sentry issue should be marked resolved in v0.57.18.

Solution

  • is_session_expired_event (src/core/observability.rs): add cron to the domain matches! allowlist, and extend the body-only acceptance branch from domain == "rpc" to matches!(domain, "rpc" | "cron").
  • The body-match requirement (is_session_expired_message) keeps it narrowly scoped: a genuine cron failure with a non-session error body is not suppressed and still reaches Sentry.
  • Doc comment updated to document the cron arm and its relationship to the fix(cron): halt agent-job retry on backend session-expired (TAURI-RUST-N) #3350 primary guard.

Submission Checklist

  • Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy — added session_expired_before_send_matches_cron_agent_job (positive) and session_expired_before_send_keeps_genuine_cron_failures (negative/edge).
  • Diff coverage ≥ 80% — both changed logic lines are exercised by the two new tests; whole change is in src/core/observability.rs (cargo-llvm-cov path). Existing 39 session-expired tests still pass.
  • Coverage matrix updated — N/A: behaviour-only defense-in-depth change, no new feature row.
  • All affected feature IDs from the matrix are listed under ## Related — N/A: no matrix feature ID affected.
  • No new external network dependencies introduced (mock backend used per Testing Strategy) — pure in-process classifier, no network.
  • Manual smoke checklist updated if this touches release-cut surfaces — N/A: internal Sentry-noise filter, no user-facing surface.
  • Linked issue closed via Closes #NNN in the ## Related section — N/A: tracked in self-hosted Sentry (TAURI-RUST-M / #80), no GitHub issue.

Impact

Related


AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

  • Key: N/A
  • URL: N/A

Commit & Branch

  • Branch: fix/sentry-80-cron-session-expired-before-send
  • Commit SHA: ba21f4bbd7845a19f0b37dfc4d8fd6346ee4fefb

Validation Run

  • pnpm --filter openhuman-app format:check — N/A: Rust-only change
  • pnpm typecheck — N/A: Rust-only change
  • Focused tests: cargo test --lib session_expired — 39 passed, 0 failed (incl. 2 new cron tests)
  • Rust fmt/check (if changed): cargo fmt --check clean
  • Tauri fmt/check (if changed): N/A: Tauri shell untouched

Validation Blocked

  • command: N/A
  • error: N/A
  • impact: N/A

Behavior Changes

  • Intended behavior change: cron-domain session-expired events are dropped in before_send (defense-in-depth) on future builds.
  • User-visible effect: none — internal Sentry-noise reduction only.

Parity Contract

  • Legacy behavior preserved: llm_provider/backend_api/rpc matching unchanged; only adds the cron arm.
  • Guard/fallback/dispatch parity checks: cron suppression gated on body-match (is_session_expired_message), so genuine cron failures still page — verified by session_expired_before_send_keeps_genuine_cron_failures.

Duplicate / Superseded PR Handling

  • Duplicate PR(s): N/A
  • Canonical PR: N/A
  • Resolution (closed/superseded/updated): N/A

@coderabbitai

coderabbitai Bot commented Jun 17, 2026 •

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Warning

Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice.


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

@senamakel

Copy link
Copy Markdown
Member

Triage check-in: this draft hasn't moved in a few weeks. Are you still working on it? If it's ready for feedback, mark it ready for review; if it's been superseded or abandoned, feel free to close it. No response in ~2 weeks and we'll close it to keep the queue tidy — it can always be reopened.

@M3gA-Mind
M3gA-Mind force-pushed the fix/sentry-80-cron-session-expired-before-send branch from ba21f4b to c53fe96 Compare September 1, 2026 23:19
@M3gA-Mind

Copy link
Copy Markdown
Collaborator

Maintainer housekeeping — rebased onto current main (fa044d388) and pushed. This branch was 7,534 commits behind and conflicting; it is now conflict-free. Still a draft — I have not touched that.

Your commit is unchanged (a36d9b1d5, authorship preserved). The conflict was doc-comment-only: main had rewritten the Scope: paragraph above is_session_expired_event and added a #[cfg(feature = "crash-reporting")] line. I kept your four-domain doc text and main's attribute. The function body merged cleanly — | "cron" and matches!(domain, "rpc" | "cron") are both in, and the change is still needed: main today is still scoped to the three domains.

I added one commit of my own (c53fe9604), restoring two #[cfg(feature = "crash-reporting")] attributes. Please look at the second one, it is not obvious:

  1. session_expired_before_send_keeps_genuine_cron_failures — your new negative test went in without the attribute its positive sibling carries.
  2. max_iterations_filter_matches_message_path — a pre-existing test that main gates. Inserting your two tests directly above it handed its #[cfg] to the first of yours. The attribute binds to whatever item follows it, so a new item in between takes it and the original silently loses its gate.

That matters because crash-reporting is not in the default feature list, while is_session_expired_event, is_max_iterations_event and the event_with_* helpers are all gated on it — so the test module simply does not compile on any lane that builds the contributor default set, including cargo clippy -p openhuman -- -D warnings (ci-lite.yml:449).

Verified locally (via the fleet CI-slot runner):

check result
cargo check -p openhuman --lib --tests (default set) clean
cargo check -p openhuman --lib --tests --features crash-reporting clean
cargo fmt --all -- --check clean
revert-check: both #[cfg] lines removed fails E0425 naming exactly is_session_expired_event / event_with_tags_and_message at your test, and is_max_iterations_event / event_with_message at the pre-existing one

Not approving and not marking ready for review — a maintainer does that.

M3gA-Mind
M3gA-Mind previously approved these changes Sep 2, 2026

@M3gA-Mind M3gA-Mind left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved after a maintainer-side verification pass.

Verified on the current head: MERGEABLE against main, zero failing and zero pending required checks, and no unresolved, non-outdated review threads.

This is one of two required approvals; a second maintainer review is still needed before merge.

@M3gA-Mind
M3gA-Mind force-pushed the fix/sentry-80-cron-session-expired-before-send branch from c53fe96 to 2c3aab0 Compare September 2, 2026 01:24
CodeGhost21 and others added 2 commits September 2, 2026 06:54
…ore_send (TAURI-RUST-M)

The morning_briefing cron agent fires while the user's backend JWT has
lapsed; the retry loop exhausts and reports with domain=cron,
operation=agent_job, failure=retries_exhausted carrying the raw
SESSION_EXPIRED sentinel. The primary call-site guard
(is_session_expired_failure, tinyhumansai#3350) halts on first occurrence, but the
before_send defense-in-depth filter is_session_expired_event was scoped
to llm_provider/backend_api/rpc and excluded cron, so any future cron
call site that bypasses the guard would still flood Sentry (tinyhumansai#80, 42k+
events).

Add cron to the allowlist and accept on the session-expired body alone
(the cron report carries no status tag, mirroring the rpc pre-flight
branch). Genuine non-session cron failures don't match the body
classifier, so real defects still page.
… block

Every test in this block is `#[cfg(feature = "crash-reporting")]` on
`main`, because the symbols they use are: `is_session_expired_event`,
`is_max_iterations_event` and the `event_with_*` helpers are all gated
on that feature, and `crash-reporting` is NOT in the default feature
list. Two tests here were left ungated:

1. `session_expired_before_send_keeps_genuine_cron_failures` -- the new
   negative test, added without the attribute its positive sibling
   `session_expired_before_send_matches_cron_agent_job` carries.

2. `max_iterations_filter_matches_message_path` -- a PRE-EXISTING test
   that `main` gates. Rebasing this branch attached that `#[cfg]` to the
   new test inserted directly above it instead, silently un-gating a
   test that was fine before. Insertion-by-adjacency: the attribute
   belongs to whichever item follows it, and a new item in between
   takes it.

Without both, the test module does not compile on any lane that builds
the contributor default set -- including `cargo clippy -p openhuman --
-D warnings` (ci-lite.yml:449). Verified: with these two lines reverted,
`cargo check -p openhuman --lib --tests` fails E0425 on exactly these
functions; with them, both that lane and `--features crash-reporting`
compile clean.

No change to any assertion or to production code.
@M3gA-Mind
M3gA-Mind force-pushed the fix/sentry-80-cron-session-expired-before-send branch from 2c3aab0 to d4b29a3 Compare September 2, 2026 01:25
@M3gA-Mind

Copy link
Copy Markdown
Collaborator

Maintainer housekeeping — rebased onto current main (61d25fe21). The branch had fallen 44 commits behind; it is now 0 behind and MERGEABLE. Still a draft — I have not touched that.

Nothing about your change was modified. Both commits keep their original content and authorship (7ed34b15e, @CodeGhost21; d4b29a3c3, the #[cfg] restoration I added earlier). main did not touch src/core/observability.rs in those 44 commits, so the rebase was a clean replay with no conflicts, and the net diff is unchanged at 1 file / +70 −8.

One note on how this was done, because it took two steps:

I first used gh pr update-branch --rebase. It worked, but GitHub's server-side rebase strips commit signatures — both commits came back as unsigned (sig=N), and signed commits are required here. So I redid the same rebase locally with --gpg-sign and pushed that instead.

To be sure the second push changed nothing but the signatures, I compared the two results by tree hash:

server-side rebase tree: 91551786de8a78d6f517abf8f34341a7cf7b3333
signed local rebase tree: 91551786de8a78d6f517abf8f34341a7cf7b3333

Identical — same content, same authorship, signatures restored.

Also re-confirmed the gating this branch fixes is still intact after the replay: all six tests in the sentry block carry #[cfg(feature = "crash-reporting")], including max_iterations_filter_matches_message_path, the pre-existing test that had silently lost its gate to insertion-by-adjacency.

CI is re-running against the new base. Expect the approval to need re-confirming — that is a maintainer's call, not mine. Not approving and not merging.

@M3gA-Mind
M3gA-Mind dismissed their stale review September 2, 2026 01:54

Dismissing this approval. It was granted during a bulk maintainer verification pass that should not have included draft PRs — this PR is a draft, so it was out of scope for that review. No judgement on the change itself; the approval is simply being withdrawn so a stale one is not sitting on a draft when it is later marked ready. Re-review will happen once it comes out of draft.

@senamakel senamakel closed this Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants