docs: record error-contract unification (ADR-052) and panic model (ADR-053)
swadhinbiswas committed 3 months ago
6e3634a
1 parent 5b9fc82
commit 6e3634a
2 files changed
+67 -0
Showing 2 changed files+67-0
▼DECISIONS.md
+58-0
| @@ -1806,3 +1806,61 @@ incrementing on real traffic; `/ready` returns 200 healthy with a component | ||
1806 | 1806 | report, and 503 (with report) when a registered check returns falsy. |
1807 | 1807 | `cargo test --workspace` and `pytest` (minus two pre-existing, unrelated |
1808 | 1808 | failures in `test_websocket`/`test_openapi_parity`) pass. |
1809 | + | |
1810 | +## ADR-052 — 2026-07-15 — Single error-response contract (`{"detail": ...}`) | |
1811 | + | |
1812 | +**Context:** The production-readiness audit (MED#9) flagged three inconsistent | |
1813 | +error shapes across the codebase: `{"error": "..."}` (native + core handlers, | |
1814 | +resilience, panic, static files), `{"detail": ...}` (Python `HTTPException` / | |
1815 | +`RequestValidationError`), and an RFC-7807 envelope | |
1816 | +(`{"type","title","status","detail","errors"}` with `application/problem+json`) | |
1817 | +emitted by the Rust JSON-Schema validation paths. Clients had no single shape | |
1818 | +to parse. | |
1819 | + | |
1820 | +**Decision:** Adopt **`{"detail": ...}`** (FastAPI-compatible) as the single | |
1821 | +error envelope for every non-2xx response. The numeric status lives in the HTTP | |
1822 | +status line; `detail` carries the human-readable message. `content-type` is | |
1823 | +always `application/json` (the `application/problem+json` RFC-7807 variant is | |
1824 | +dropped). Applied across: | |
1825 | +- `justapi_core::error_response(status, detail)` / `validation_response(detail)` | |
1826 | + helpers added in `lib.rs`; all core error sites (server, resilience, panic, | |
1827 | + static files) routed through them. | |
1828 | +- `justapi-py` native handlers: generic 500 → `{"detail":"Internal Server Error"}`; | |
1829 | + all `{"error":...}` string bodies → `{"detail":...}`; the RFC-7807 validation | |
1830 | + blocks collapsed to `{"detail": <message>}`. | |
1831 | +- Python `system.py` error routes → `{"detail":...}`. | |
1832 | +- Sensitive internals are never placed in `detail` for 5xx (matches the secure | |
1833 | + generic-500 policy from the prior audit). | |
1834 | + | |
1835 | +**Rationale:** one parseable contract; `detail` keeps FastAPI client | |
1836 | +compatibility; RFC-7807's extra keys added no consumer value here and broke | |
1837 | +uniformity. **Evidence:** `cargo test --workspace` (core integration test | |
1838 | +updated) and `pytest` (validation / test-client / native-fastpath tests updated | |
1839 | +for `detail` + `application/json`) pass. The two remaining `pytest` failures | |
1840 | +(`test_websocket`, `test_openapi_parity`) are pre-existing and unrelated | |
1841 | +(fail on the prior save-point too). | |
1842 | + | |
1843 | +## ADR-053 — 2026-07-15 — Panic model: abort + supervisor restart, not catch_unwind | |
1844 | + | |
1845 | +**Context:** MED#9 noted that `panic = "abort"` means one bad request can kill | |
1846 | +the process. Two mitigations were considered: (a) switch to `panic = "unwind"` | |
1847 | ++ `catch_unwind` around the handler, or (b) audit/remove hot-path `.unwrap()`. | |
1848 | + | |
1849 | +**Decision:** Keep **`panic = "abort"`** (ADR-048 / `gil_pool.rs` SAFETY note). | |
1850 | +A `catch_unwind` boundary at the GIL FFI boundary is **undefined behaviour** | |
1851 | +(pyo3 C code holding the GIL cannot be unwound safely) and was measured as a | |
1852 | +**~3x throughput regression** on the GIL path — both documented in | |
1853 | +`gil_pool.rs`. The production model is therefore **crash-fast + supervisor | |
1854 | +restart** (Docker/k8s/systemd), which is the correct availability trade for a | |
1855 | +Rust HTTP server. The actionable mitigation is the **hot-path unwrap audit**: | |
1856 | +- Fixed a per-request abort: `router.fallback().unwrap()` in `server/mod.rs` | |
1857 | + now returns 404 when no fallback is registered (a 404 would otherwise abort | |
1858 | + the whole process). | |
1859 | +- Reviewed request-path `.unwrap()`/`.expect()` in `server/` and `native/`; the | |
1860 | + remainder are init-time (route registration) or logically-guaranteed (e.g. | |
1861 | + `content_type.unwrap()` only reached when `is_multipart` is true), so they | |
1862 | + cannot be triggered by attacker input. A genuine logic bug still aborts and is | |
1863 | + restarted by the supervisor — by design. | |
1864 | + | |
1865 | +**Evidence:** `cargo build --workspace` and all gates green; the core 404 path | |
1866 | +returns 200/404 normally and no longer panics on unmatched routes. | |
1809 | 1867 | |
▼PLAN.md
+9-0
| @@ -1104,3 +1104,12 @@ Status of the P0 sprint driven by the MED#9 audit verdict (see BENCHMARKS.md | ||
1104 | 1104 | - [ ] **Panic isolation** (P1/P2): `panic = "abort"` means one bad request can |
1105 | 1105 | kill the process; a GIL-path `catch_unwind` around handlers + an unwrap |
1106 | 1106 | audit on the hot path (server/ + native/) is still TODO. |
1107 | + | |
1108 | +- [x] **Error contract** (ADR-052): single `{"detail": ...}` envelope across | |
1109 | + core + Python (Rust validation RFC-7807 + `{"error":...}` collapsed; | |
1110 | + `application/json` everywhere). `error_response`/`validation_response` | |
1111 | + helpers added in `justapi_core::lib.rs`. | |
1112 | +- [x] **Panic model** (ADR-053): kept `panic = "abort"` + supervisor restart | |
1113 | + (catch_unwind at the GIL FFI boundary is UB + ~3x regression per | |
1114 | + `gil_pool.rs`); fixed the per-request `router.fallback().unwrap()` abort | |
1115 | + and audited remaining hot-path unwraps. | |
1107 | 1116 | |