-
Notifications
You must be signed in to change notification settings - Fork 10
fix(server): make feature-request reads idempotent (ARN-240) #396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
92be2ad
3b6cb93
e7d58ea
9c0c2e1
dff41d3
e7cf9eb
fb55f26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ use axum::extract::{Path, Query, State}; | |
| use axum::http::{HeaderMap, StatusCode}; | ||
| use axum::response::Json; | ||
| use axum::response::sse::{Event, KeepAlive, Sse}; | ||
| use sha2::{Digest, Sha256}; | ||
| use temper_evolution::FeatureRequestDisposition; | ||
| use temper_runtime::tenant::TenantId; | ||
| use tokio_stream::StreamExt; | ||
|
|
@@ -19,15 +20,19 @@ use crate::sentinel; | |
| use crate::state::{ObserveRefreshHint, ServerState}; | ||
|
|
||
| mod materialize; | ||
| mod reconcile; | ||
| mod support; | ||
|
|
||
| pub(crate) use materialize::{handle_evolution_analyze, handle_evolution_materialize}; | ||
|
|
||
| use reconcile::reconcile_legacy_feature_requests; | ||
| use support::{ | ||
| create_system_entity_logged, emit_refresh_hints, next_system_entity_id, persist_alerts, | ||
| persist_insights, spawn_intent_discovery, | ||
| dispatch_system_action_idempotent, emit_refresh_hints, persist_alerts, persist_insights, | ||
| spawn_intent_discovery, | ||
| }; | ||
|
|
||
| const FEATURE_REQUEST_GENERATOR_VERSION: &str = "v1"; | ||
|
|
||
| /// POST /api/evolution/sentinel/check -- trigger sentinel rule evaluation. | ||
| /// | ||
| /// Evaluates all default sentinel rules against current server state. | ||
|
|
@@ -106,6 +111,8 @@ pub(crate) async fn handle_sentinel_check( | |
| tracing::Span::current().record("insights_count", insights.len()); | ||
| tracing::info!(insights_count = insights.len(), "evolution.insight"); | ||
| let insight_results = persist_insights(&state, &insights).await; | ||
| let feature_request_ids = | ||
| materialize_feature_requests(&state, &analysis_tenant, &trajectory_entries).await?; | ||
|
|
||
| emit_refresh_hints( | ||
| &state, | ||
|
|
@@ -123,9 +130,133 @@ pub(crate) async fn handle_sentinel_check( | |
| "intent_discoveries": discovery_results, | ||
| "insights_count": insights.len(), | ||
| "insights": insight_results, | ||
| "feature_requests_count": feature_request_ids.len(), | ||
| "feature_request_ids": feature_request_ids, | ||
| }))) | ||
| } | ||
|
|
||
| pub(crate) fn stable_feature_request_id( | ||
| tenant: &TenantId, | ||
| generator_version: &str, | ||
| feature_request: &temper_evolution::FeatureRequestRecord, | ||
| ) -> String { | ||
| let mut trajectory_refs = feature_request.trajectory_refs.clone(); | ||
| trajectory_refs.sort(); | ||
| let category = match feature_request.category { | ||
| temper_evolution::PlatformGapCategory::MissingMethod => "missing_method", | ||
| temper_evolution::PlatformGapCategory::GovernanceBlocked => "governance_blocked", | ||
| temper_evolution::PlatformGapCategory::UnsupportedIntegration => "unsupported_integration", | ||
| temper_evolution::PlatformGapCategory::MissingCapability => "missing_capability", | ||
| }; | ||
| let identity = serde_json::json!({ | ||
| "tenant": tenant.as_str(), | ||
| "generator_version": generator_version, | ||
| "category": category, | ||
| "description": feature_request.description, | ||
| "trajectory_refs": trajectory_refs, | ||
| }); | ||
| format!("FR-{:x}", Sha256::digest(identity.to_string())) | ||
| } | ||
|
|
||
| async fn materialize_feature_requests( | ||
| state: &ServerState, | ||
| tenant: &TenantId, | ||
| trajectory_entries: &[crate::state::TrajectoryEntry], | ||
| ) -> Result<Vec<String>, StatusCode> { | ||
| let tenant_entries = trajectory_entries | ||
| .iter() | ||
| .filter(|entry| entry.tenant == tenant.as_str()) | ||
| .cloned() | ||
| .collect::<Vec<_>>(); | ||
| let generated = insight_generator::generate_feature_requests(&tenant_entries); | ||
| if generated.is_empty() { | ||
| return Ok(Vec::new()); | ||
| } | ||
| let store = state | ||
| .platform_metadata_store() | ||
| .ok_or(StatusCode::SERVICE_UNAVAILABLE)?; | ||
| let existing_rows = store | ||
| .list_feature_requests(None) | ||
| .await | ||
| .map_err(|error| { | ||
| tracing::error!(error = %error, backend = store.backend_name(), "failed to load feature requests for reconciliation"); | ||
| StatusCode::INTERNAL_SERVER_ERROR | ||
| })?; | ||
| let mut materialized_ids = Vec::with_capacity(generated.len()); | ||
|
|
||
| for feature_request in &generated { | ||
| let stable_id = | ||
| stable_feature_request_id(tenant, FEATURE_REQUEST_GENERATOR_VERSION, feature_request); | ||
| let category = format!("{:?}", feature_request.category); | ||
| let refs_json = serde_json::to_string(&feature_request.trajectory_refs) | ||
| .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; | ||
| let disposition = match feature_request.disposition { | ||
| FeatureRequestDisposition::Open => "Open", | ||
| FeatureRequestDisposition::Acknowledged => "Acknowledged", | ||
| FeatureRequestDisposition::Planned => "Planned", | ||
| FeatureRequestDisposition::WontFix => "WontFix", | ||
| FeatureRequestDisposition::Resolved => "Resolved", | ||
| }; | ||
| let params = serde_json::json!({ | ||
| "category": category, | ||
|
Comment on lines
+178
to
+201
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/temper-server/src/observe/evolution/operations.rs
Line: 173-196
Comment:
**`existing_rows` snapshot is stale during the multi-feature-request loop**
`existing_rows` is loaded once before the loop, then passed by reference to every `reconcile_legacy_feature_requests` call inside it. For a first-time stable row insert the `canonical_notes` path in `merged_review_state` is always `None` (the stable row wasn't in the pre-loop snapshot), which is correct but non-obvious. A comment explaining the invariant — that `is_same_evidence_revision` ensures disjoint legacy-row sets per generated feature request, making the stale snapshot safe — would make the intent explicit.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| "description": feature_request.description, | ||
| "frequency": feature_request.frequency.to_string(), | ||
| "developer_notes": feature_request.developer_notes.clone().unwrap_or_default(), | ||
| }); | ||
|
|
||
| dispatch_system_action_idempotent( | ||
| state, | ||
| "FeatureRequest", | ||
| &stable_id, | ||
| "CreateFeatureRequest", | ||
| params, | ||
| &format!("feature-request:{stable_id}"), | ||
| ) | ||
| .await | ||
| .map_err(|error| { | ||
| tracing::error!(error = %error, feature_request_id = %stable_id, "failed to materialize feature request entity"); | ||
| StatusCode::INTERNAL_SERVER_ERROR | ||
| })?; | ||
|
|
||
| store | ||
| .upsert_feature_request( | ||
| &stable_id, | ||
| &category, | ||
| &feature_request.description, | ||
| feature_request.frequency as i64, | ||
| &refs_json, | ||
| disposition, | ||
| feature_request.developer_notes.as_deref(), | ||
| ) | ||
| .await | ||
| .map_err(|error| { | ||
| tracing::error!(error = %error, backend = store.backend_name(), feature_request_id = %stable_id, "failed to project feature request"); | ||
| StatusCode::INTERNAL_SERVER_ERROR | ||
| })?; | ||
| reconcile_legacy_feature_requests( | ||
| store.as_ref(), | ||
| &existing_rows, | ||
| tenant, | ||
| trajectory_entries, | ||
| &stable_id, | ||
| feature_request, | ||
| ) | ||
| .await?; | ||
| materialized_ids.push(stable_id); | ||
| } | ||
|
|
||
| Ok(materialized_ids) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| pub(crate) async fn materialize_feature_requests_for_test( | ||
| state: &ServerState, | ||
| tenant: &TenantId, | ||
| trajectory_entries: &[crate::state::TrajectoryEntry], | ||
| ) -> Result<Vec<String>, StatusCode> { | ||
| materialize_feature_requests(state, tenant, trajectory_entries).await | ||
| } | ||
|
|
||
| /// GET /observe/evolution/unmet-intents -- grouped unmet intents from trajectories. | ||
| /// | ||
| /// Uses a SQL GROUP BY aggregation instead of loading raw trajectory rows to | ||
|
|
@@ -228,51 +359,7 @@ pub(crate) async fn handle_feature_requests( | |
| require_observe_auth(&state, &headers, "read_evolution", "Evolution")?; | ||
| let disposition_filter = params.get("disposition").map(String::as_str); | ||
|
|
||
| let trajectory_entries = state.load_trajectory_entries(1_000).await; | ||
|
|
||
| if let Some(store) = state.platform_metadata_store() { | ||
| let generated = insight_generator::generate_feature_requests(&trajectory_entries); | ||
| for feature_request in &generated { | ||
| let refs_json = serde_json::to_string(&feature_request.trajectory_refs) | ||
| .unwrap_or_else(|_| "[]".to_string()); | ||
| let disposition = match feature_request.disposition { | ||
| FeatureRequestDisposition::Open => "Open", | ||
| FeatureRequestDisposition::Acknowledged => "Acknowledged", | ||
| FeatureRequestDisposition::Planned => "Planned", | ||
| FeatureRequestDisposition::WontFix => "WontFix", | ||
| FeatureRequestDisposition::Resolved => "Resolved", | ||
| }; | ||
| if let Err(error) = store | ||
| .upsert_feature_request( | ||
| &feature_request.header.id, | ||
| &format!("{:?}", feature_request.category), | ||
| &feature_request.description, | ||
| feature_request.frequency as i64, | ||
| &refs_json, | ||
| disposition, | ||
| feature_request.developer_notes.as_deref(), | ||
| ) | ||
| .await | ||
| { | ||
| tracing::warn!(error = %error, backend = store.backend_name(), "failed to upsert feature request"); | ||
| } | ||
|
|
||
| create_system_entity_logged( | ||
| &state, | ||
| "FeatureRequest", | ||
| &next_system_entity_id("FR"), | ||
| "CreateFeatureRequest", | ||
| serde_json::json!({ | ||
| "category": format!("{:?}", feature_request.category), | ||
| "description": feature_request.description, | ||
| "frequency": feature_request.frequency.to_string(), | ||
| "developer_notes": feature_request.developer_notes.clone().unwrap_or_default(), | ||
| "legacy_record_id": feature_request.header.id, | ||
| }), | ||
| ) | ||
| .await; | ||
| } | ||
|
|
||
| return match store.list_feature_requests(disposition_filter).await { | ||
| Ok(rows) => { | ||
| let feature_requests = rows | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.