fix(server): make feature-request reads idempotent (ARN-240) - #396
fix(server): make feature-request reads idempotent (ARN-240)#396rita-aga wants to merge 7 commits into
Conversation
Record the decision that feature-request GET is a pure read, materialization uses stable tenant+evidence+generator-version identity, and legacy projection reconciliation preserves human review state without claiming ambiguous tenants.
Grok independent review — PR #396 (ARN-240 / ADR-0175)Scope: Make feature-request GET a pure read; materialize on the evolution write path with stable content-addressed ids; preserve human review on upsert; tenant-safe legacy projection reconciliation. What was done well
FindingsImportant (should fix)
Suggestions (nice to have)
Plan alignmentMatches ADR-0175 sub-decisions 1–5 for the GET purity bug and legacy upgrade path. The largest open product risk is stable-id churn under growing evidence, which the ADR frames as intentional “evidence revision” but does not solve for orphan prior revisions. VerdictGET purity, review-preserving upsert, tenant-safe legacy reconcile, and retry/idempotency tests are solid and shippable for the stated ARN-240 bug. Address or explicitly track identity churn / supersession so materialization does not become a slow leak of FR rows. Verdict: PASS |
|
@greptile review |
ARENA SHIPPABLE · Grok · 2026-07-14 11:57 PDTPR: #396 Checklist
Summaryfeature-request GET idempotency |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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, |
There was a problem hiding this 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.
Prompt To Fix With AI
This 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!
|
@greptile review |
1 participant
Grok arena ARN-240. ADR-0175. No merge.
Greptile Summary
This PR fixes a long-standing violation of HTTP idempotency by removing all write-on-read side-effects from
GET /observe/evolution/feature-requestsand moving materialization exclusively to the sentinel write path. It introduces a SHA-256 content-addressed stable identity for feature requests (tenant + generator version + evidence), an idempotency key on theCreateFeatureRequestdispatch, and a legacy reconciliation pass that migrates oldFR-YYYY-*rows to canonical stable IDs while preserving human review state.handle_feature_requestsis now a bounded query; trajectory loading, upsert, and entity creation are fully removed from that handler.materialize_feature_requestscomputes a deterministicstable_feature_request_id, dispatchesCreateFeatureRequestwith an idempotency key, and callsreconcile_legacy_feature_requeststo merge legacy projections into the canonical row.upsert_feature_request(Turso + Postgres) now preservesdispositionanddeveloper_noteson conflict;delete_feature_requestis added to both stores and theEvolutionStoretrait.Confidence Score: 5/5
Safe to merge; the GET handler is now a pure read, the sentinel write path converges on a single canonical row under concurrent and retry conditions, and upserts preserve human review state on conflict.
The core idempotency invariants are well-tested across concurrent requests, restarts, legacy reconciliation, note deduplication, and multi-tenant disambiguation. Store-layer changes are straightforward and mirrored across both Turso and Postgres. The only discrepancy found is a documentation error in the ADR hash formula with no runtime impact.
docs/adrs/0175-feature-request-get-idempotency.md — hash formula lists
frequencybut the implementation omits it.Important Files Changed
frequencyeven though the ADR formula lists it.frequencyas an explicit field while the code omits it.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Client participant GET as GET /feature-requests participant POST as POST /sentinel/check participant Gen as generate_feature_requests participant Hash as stable_feature_request_id participant IOA as dispatch_system_action_idempotent participant Store as MetadataStore participant Rec as reconcile_legacy_feature_requests Client->>GET: GET /observe/evolution/feature-requests GET->>Store: list_feature_requests(filter) Store-->>GET: rows[] GET-->>Client: 200 OK (pure read, no writes) Client->>POST: POST /api/evolution/sentinel/check POST->>Gen: generate_feature_requests(trajectory_entries) Gen-->>POST: [FeatureRequestRecord, ...] POST->>Store: list_feature_requests(None) — snapshot for reconcile Store-->>POST: existing_rows[] loop per feature_request POST->>Hash: stable_feature_request_id(tenant, "v1", request) Hash-->>POST: "FR-{sha256}" POST->>IOA: "dispatch CreateFeatureRequest (idempotency_key=feature-request:{id})" IOA-->>POST: EntityResponse (no-op if already exists) POST->>Store: upsert_feature_request(stable_id, …) — preserves disposition/notes POST->>Rec: reconcile_legacy_feature_requests(existing_rows, stable_id) Rec->>Store: update_feature_request(stable_id, merged_state) Rec->>Store: delete_feature_request(legacy_id) x N end POST-->>Client: "200 OK {feature_request_ids: [...]}"%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Client participant GET as GET /feature-requests participant POST as POST /sentinel/check participant Gen as generate_feature_requests participant Hash as stable_feature_request_id participant IOA as dispatch_system_action_idempotent participant Store as MetadataStore participant Rec as reconcile_legacy_feature_requests Client->>GET: GET /observe/evolution/feature-requests GET->>Store: list_feature_requests(filter) Store-->>GET: rows[] GET-->>Client: 200 OK (pure read, no writes) Client->>POST: POST /api/evolution/sentinel/check POST->>Gen: generate_feature_requests(trajectory_entries) Gen-->>POST: [FeatureRequestRecord, ...] POST->>Store: list_feature_requests(None) — snapshot for reconcile Store-->>POST: existing_rows[] loop per feature_request POST->>Hash: stable_feature_request_id(tenant, "v1", request) Hash-->>POST: "FR-{sha256}" POST->>IOA: "dispatch CreateFeatureRequest (idempotency_key=feature-request:{id})" IOA-->>POST: EntityResponse (no-op if already exists) POST->>Store: upsert_feature_request(stable_id, …) — preserves disposition/notes POST->>Rec: reconcile_legacy_feature_requests(existing_rows, stable_id) Rec->>Store: update_feature_request(stable_id, merged_state) Rec->>Store: delete_feature_request(legacy_id) x N end POST-->>Client: "200 OK {feature_request_ids: [...]}"Reviews (2): Last reviewed commit: "fix(server): stable feature-request cate..." | Re-trigger Greptile