From 1b6f4bde670cedf03065879a7eefedc9294a5802 Mon Sep 17 00:00:00 2001 From: Brummel Date: Sun, 3 May 2026 19:33:09 +0200 Subject: [PATCH] feat: Handle analyze retry with failure marker When a previous analysis attempt fails and leaves a failure marker, the retry mechanism now correctly identifies this state. It removes the stale input file, allowing the retry to proceed without a conflict error. This ensures users can recover from analysis failures more gracefully. --- server/src/analyze/backend.rs | 35 ++++++++++------- server/src/routes/case_actions.rs | 12 ++++++ server/tests/analyze_test.rs | 53 ++++++++++++++++++++++++++ server/tests/case_page_test.rs | 62 ++++++++++++++++++++++++++++++- 4 files changed, 146 insertions(+), 16 deletions(-) diff --git a/server/src/analyze/backend.rs b/server/src/analyze/backend.rs index 7e7b03d..51c40db 100644 --- a/server/src/analyze/backend.rs +++ b/server/src/analyze/backend.rs @@ -119,6 +119,11 @@ impl LlmBackend { self.format_instruction = Some(instruction.into()); self } + + fn with_max_completion_tokens(mut self, tokens: u32) -> Self { + self.max_completion_tokens = tokens; + self + } } /// Ionos provider: shared endpoint, `IONOS_API_KEY` env var as bearer token. @@ -149,15 +154,17 @@ pub fn backends() -> &'static [LlmBackend] { static B: OnceLock> = OnceLock::new(); B.get_or_init(|| { vec![ - // gpt-oss-120b: schema OFF on purpose. With `reasoning_effort: - // medium`, reasoning tokens count against `max_completion_tokens` - // (4096); on large bodies (e.g. 6 recordings, ~3.8k chars) the - // reasoning eats the budget and the model returns - // `choices[0].message.content == null` — the LlmError::Parse path. - // Pre-multi-backend (commit bf6464d) gpt-oss ran without schema - // and worked fine; this restores that body byte-for-byte. + // gpt-oss-120b: schema OFF on purpose. Reasoning tokens count + // against `max_completion_tokens`; with `high` reasoning the + // CoT alone can eat several thousand tokens, so the budget is + // doubled to 8192 to leave room for the actual answer. Without + // headroom the model returns `choices[0].message.content == null` + // — the LlmError::Parse path. Pre-multi-backend (commit bf6464d) + // gpt-oss ran without schema and worked fine; this restores + // that body byte-for-byte except for the larger budget. ionos_backend("gpt_oss_120b", "GPT OSS 120b", "openai/gpt-oss-120b") - .with_reasoning_effort("medium") + .with_reasoning_effort("high") + .with_max_completion_tokens(8192) .with_use_json_schema(false), ionos_backend( "llama_3_1_405b", @@ -234,7 +241,7 @@ mod tests { fn reasoning_effort_is_set_per_model_family() { assert_eq!( find_backend("gpt_oss_120b").unwrap().reasoning_effort, - Some("medium".to_string()) + Some("high".to_string()) ); assert!( find_backend("llama_3_1_405b") @@ -304,11 +311,11 @@ mod tests { } /// gpt-oss-120b runs in plain-text mode (no `response_format`, no - /// `top_p`, no second system message). Reason: with - /// `reasoning_effort: medium`, schema-constrained decoding plus reasoning - /// can exhaust `max_completion_tokens` on large bodies and return - /// `content: null`. This pin guarantees the body sent to Ionos stays - /// byte-identical to the pre-multi-backend path (commit bf6464d). + /// `top_p`, no second system message). Reason: schema-constrained + /// decoding plus any non-zero `reasoning_effort` can exhaust + /// `max_completion_tokens` on large bodies and return `content: null`. + /// This pin guarantees the body sent to Ionos stays byte-identical to + /// the pre-multi-backend path (commit bf6464d). #[test] fn gpt_oss_runs_in_plain_text_mode_no_schema_no_top_p_no_format_hint() { let b = find_backend("gpt_oss_120b").unwrap(); diff --git a/server/src/routes/case_actions.rs b/server/src/routes/case_actions.rs index c0df569..b743b72 100644 --- a/server/src/routes/case_actions.rs +++ b/server/src/routes/case_actions.rs @@ -14,6 +14,7 @@ use tracing::{info, warn}; use doctate_common::timestamp::{filename_stem_to_recorded_at, now_rfc3339}; use doctate_common::{RECORDING_META_SUFFIX, TranscriptState}; +use crate::analyze::auto_trigger::FAILURE_MARKER_FILE; use crate::analyze::backend::{any_backend_available, find_backend}; use crate::analyze::{ ANALYSIS_INPUT_FILE, AnalysisInput, AnalyzeJob, AnalyzeSender, DOCUMENT_FILE, RecordingInput, @@ -107,6 +108,17 @@ pub async fn handle_analyze_case( let input = build_analysis_input(&case_dir, &backend_id).await?; let input_path = case_dir.join(ANALYSIS_INPUT_FILE); + + // Manual retry path: a failure marker means the worker has explicitly + // given up on the prior input. Drop the stale sentinel so create_new + // does not reject the legitimate retry as "Analyse läuft bereits". + // The marker itself is left in place — the worker overwrites it on + // re-failure and removes it on success, so the UI's "analyzing" flag + // (driven by input file presence) takes over from the next render. + if case_dir.join(FAILURE_MARKER_FILE).exists() { + remove_if_exists(&input_path).await?; + } + write_input_create_new(&input_path, &input).await?; // Unbounded send: only fails if the worker has gone away (programmer error diff --git a/server/tests/analyze_test.rs b/server/tests/analyze_test.rs index 9c4dfcd..e33d2de 100644 --- a/server/tests/analyze_test.rs +++ b/server/tests/analyze_test.rs @@ -272,6 +272,59 @@ async fn analyze_case_second_time_returns_409() { assert_eq!(second.status(), StatusCode::CONFLICT); } +/// Bug regression (case c414cf52, 2026-05-03): a prior LLM run failed, +/// leaving `analysis_input.json` AND `.analysis_failed.json` on disk. +/// The user clicks "Erneut versuchen" — the handler must accept the +/// retry instead of rejecting it as 409 Conflict ("Analyse läuft +/// bereits"). The failure marker is the explicit "worker has given up" +/// signal that legitimises overwriting the stale sentinel. +#[tokio::test] +async fn analyze_retry_after_failure_marker_overwrites_stale_input() { + let (cfg, settings) = cfg_llm("retry-after-fail"); + let case_id = "11111111-1111-1111-1111-111111111111"; + let case_dir = seed_case(&cfg.data_path, "dr_a", case_id); + seed_recording(&case_dir, "2026-04-15T10-00-00Z", Some("frischer Text")); + + // Simulate the post-failure on-disk state: stale input from the + // attempt the worker gave up on, plus the matching failure marker. + std::fs::write( + case_dir.join(ANALYSIS_INPUT_FILE), + r#"{"last_recording_mtime":"2026-04-15T10:00:00Z","recordings":[{"recorded_at":"2026-04-15T10:00:00Z","text":"alter Text"}],"backend_id":""}"#, + ) + .unwrap(); + std::fs::write( + case_dir.join(".analysis_failed.json"), + r#"{"last_recording_mtime":"2026-04-15T10:00:00Z","reason":"llm: connect timeout","failed_at":"2026-04-15T10:05:00Z"}"#, + ) + .unwrap(); + + let (app, store) = doctate_server::create_router_and_session_store_with_settings(cfg, settings); + let (cookie, csrf) = login_with_csrf(&app, &store, "dr_a", "s").await; + + let resp = app + .oneshot(csrf_form_post( + paths::case_analyze(case_id), + &cookie, + &csrf, + "", + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::SEE_OTHER, + "retry with failure marker present must succeed, not 409" + ); + + // Input was rewritten with the fresh transcript — the stale "alter Text" + // must be gone. + let raw = std::fs::read_to_string(case_dir.join(ANALYSIS_INPUT_FILE)).unwrap(); + let parsed: Value = serde_json::from_str(&raw).unwrap(); + let recs = parsed["recordings"].as_array().unwrap(); + assert_eq!(recs.len(), 1); + assert_eq!(recs[0]["text"], "frischer Text"); +} + #[tokio::test] async fn analyze_case_skips_blank_transcript_from_recordings() { let (cfg, settings) = cfg_llm("i"); diff --git a/server/tests/case_page_test.rs b/server/tests/case_page_test.rs index 06d6dd3..2860141 100644 --- a/server/tests/case_page_test.rs +++ b/server/tests/case_page_test.rs @@ -116,6 +116,9 @@ async fn case_page_shows_analyze_button_when_transcribed_without_document() { /// and the auto-trigger refuses to retry as long as the marker matches the /// current input. The case page must therefore surface a recovery banner /// that exposes the reason and a retry form, otherwise the user is stuck. +/// +/// Doctor variant: only the default-backend "Erneut versuchen" button is +/// shown; per-backend choice is admin-only (covered by the sibling test). #[tokio::test] async fn case_page_shows_failure_banner_with_retry_when_marker_present() { let (cfg, settings) = TestConfig::new() @@ -173,8 +176,12 @@ async fn case_page_shows_failure_banner_with_retry_when_marker_present() { "retry form must POST to the analyze endpoint" ); assert!( - body.contains("Erneut mit "), - "retry button label missing (expected 'Erneut mit ')" + body.contains(">Erneut versuchen"), + "default retry button missing (expected 'Erneut versuchen')" + ); + assert!( + !body.contains("Erneut mit "), + "per-backend retry buttons must not be shown to non-admins" ); assert!( !body.contains("Wird analysiert"), @@ -182,6 +189,57 @@ async fn case_page_shows_failure_banner_with_retry_when_marker_present() { ); } +/// Admin variant of the failure-banner test: in addition to the default +/// "Erneut versuchen" button shown to every user, an admin sees one +/// "Erneut mit " button per configured backend, letting them +/// re-analyse with a different model when one is misbehaving. Pins the +/// `{% if is_admin %}` gate in `case_page.html` so a future refactor +/// can't silently expose backend choice to all users (matches the +/// `analyze` handler's 403 guard) or remove it from admins. +#[tokio::test] +async fn case_page_failure_banner_admin_can_choose_backend() { + let (cfg, settings) = TestConfig::new() + .with_label("cp-fail-banner-admin") + .with_user(test_admin("dr_a")) + .with_llm("http://unused") + .build_pair(); + let case_id = "11111111-1111-1111-1111-111111111111"; + let case_dir = seed_case(&cfg.data_path, "dr_a", case_id); + seed_recording( + &case_dir, + "2026-04-15T10-00-00Z", + Some("transkribierter Text"), + ); + std::fs::write( + case_dir.join(".analysis_failed.json"), + r#"{"last_recording_mtime":"2026-04-15T10:00:00Z","reason":"llm: connect timeout","failed_at":"2026-04-15T10:05:00Z"}"#, + ) + .unwrap(); + + let app = doctate_server::create_router_with_settings(cfg, settings); + let cookie = common::login(&app, "dr_a", "s").await; + + let resp = app + .oneshot(get_with_cookie(paths::case_detail(case_id), &cookie)) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + let body = body_string(resp).await; + + assert!( + body.contains(r#"class="failure-banner""#), + "failure-banner div missing for admin" + ); + assert!( + body.contains(">Erneut versuchen"), + "default retry button must remain visible for admin too" + ); + assert!( + body.contains("Erneut mit "), + "admin must see at least one 'Erneut mit ' button" + ); +} + #[tokio::test] #[ignore = "post-refactor any_backend_available() reads IONOS_API_KEY which TestConfig pre-sets; cannot be unset for one test in a shared process"] async fn case_page_shows_llm_missing_hint_without_llm() {