docs: address the six open review threads on PR #4

docs/performance-review-v1.2.md

@@ -57,7 +57,11 @@ The app's heavy operations are all in `/process` (parse + flatten + full-dataset
 **Description:** `Workbook()` (normal mode) holds all rows in memory, and `wb.save(output)` + `output.getvalue()` create a second full copy (plus the JSON request body already holds `csv_data`). With a ~10 MB `csv_data` payload the request can transiently consume several hundred MB — enough to OOM a free-tier dyno and a 413-adjacent DoS amplifier (60/min limit only partially mitigates). CSV export (`routes.py:217-238`) has the same pattern with `io.StringIO` (smaller but still full-buffer).
 
 **Remediation:**
-- **Diskless by default.** openpyxl's `write_only=True` mode and `tempfile.SpooledTemporaryFile` both rely on OS temporary files (transient payload-derived data on disk), which conflicts with the absolute "no disk writes of payloads" rule. The diskless path is a normal-mode `Workbook` plus a hard `MAX_EXPORT_ROWS` cap (default e.g. 100k → 400 instead of OOM), which bounds peak memory without touching disk. The memory-light temp-file route is only acceptable under an explicit documented exception (roadmap D6). Note that even `write_only=True` does not eliminate the final `save()` zip assembly.
+- **Diskless by default.** openpyxl's `write_only=True` mode writes worksheet parts to OS temporary files, which conflicts with the absolute "no disk writes of payloads" rule. `tempfile.SpooledTemporaryFile` is not the diskless escape hatch it looks like, but not for the usual reason: with its **default `max_size=0` it never rolls over on write at all** (`_check()` tests `if max_size and tell() > max_size`, so a zero threshold is falsy) — it is then a `BytesIO` with extra indirection and **zero** memory benefit, which is the actual reason it fails to solve P3. Set a non-zero `max_size` and it *does* write payload bytes to an OS temp file once that threshold is crossed; and any call to `fileno()` (or `rollover()`) forces the disk file regardless of threshold. So it is either pointless or disk-backed — never both diskless and memory-light. The diskless path is a normal-mode `Workbook` plus the `MAX_EXPORT_CELLS` budget below, which bounds peak memory without touching disk. The memory-light temp-file route is only acceptable under an explicit documented exception (roadmap D6). Note that even `write_only=True` does not eliminate the final `save()` zip assembly.
+- **The XLSX guard is enabled by default, budgeted in cells, and never a silent rejection.** An unlimited default would leave P3 — a High finding — unmitigated, which is the whole point of this item. What was wrong in the first pass was not *having* a guard but expressing it as a fixed row count that could reject data `/process` had already accepted, with no warning and no route forward. Both properties are achievable at once:
+  - **Budget in cells, not rows, and keep it on.** openpyxl memory tracks `rows × columns`: a 10 MiB body with 3 columns and one with 500 columns have wildly different footprints at the same row count. The guard is `MAX_EXPORT_CELLS`, **enabled by default**, with the default derived from the §4 measurement as the largest cell count that holds peak RSS inside the 150 MiB export target on the worst aspect ratio tested. Re-derive it whenever that measurement is re-run; do not pick a round number by feel. `0` disables it, for operators who knowingly opt out.
+  - **A bounded input is not a bounded footprint.** `/export-xlsx` bodies are already capped by `MAX_CONTENT_LENGTH` (10 MiB), so peak memory is *finite* — but the JSON → Python → openpyxl → zip expansion multiplier is large and data-dependent, so "finite" is not a useful operating bound. The measured cell budget is the bound; the request cap is not a substitute for it. The guard is **XLSX-only**: CSV/TSV are generator-streamed and stay uncapped, so every dataset `/process` accepts remains exportable by some route. That, rather than an unlimited XLSX path, is what keeps the export contract at least as wide as the input contract.
+  - **Advertised, never silent.** `/process` already returns `total_rows` (`routes.py:192`) — reuse it, do not add a second row-count field. The new keys are `total_cells` (`total_rows × len(csv_columns)`) and `max_export_cells`, echoing the effective limit. Client rule: disable the Excel entry iff `max_export_cells > 0 && total_cells > max_export_cells`, labelling it "too large for Excel — use CSV/TSV" *before* the user clicks. `/export-xlsx` independently returns `400 {"error": "Dataset is N cells, above the Excel export limit of M; export CSV or TSV instead."}` (defence in depth for direct API callers). No silent truncation: a partial spreadsheet is worse than a refusal.
 - Deliver via `send_file`/`Response(iter)`; do **not** `output.getvalue()` the whole buffer into memory.
 - For CSV: use a generator-based `Response` that yields rows (`csv` writer needs a text wrapper — yield `''.join`-chunked or use `io.StringIO` flush per N rows). CSV is natively streamable with no temp files. Keep `extrasaction='ignore'` and the F1 sanitization.
 
@@ -140,7 +144,7 @@ The app's heavy operations are all in `/process` (parse + flatten + full-dataset
 
 **Description:** `API_FETCH_TIMEOUT` defaults to 30s and gunicorn's default `--timeout` is also 30s. An API fetch that takes ~30s races the worker kill: gunicorn SIGKILLs the worker mid-response → client sees 502, worker respawn cost, request fails with a confusing error. None of the documented gunicorn invocations set `--timeout` or `--workers`.
 
-**Remediation:** Set `--timeout 60` (≥ API_FETCH_TIMEOUT × 2) in `render.yaml` and README/Docker/systemd examples; make the relationship explicit in config docs. Optionally lower `API_FETCH_TIMEOUT` to 15s for snappier failures. Add `--workers 2` note for the free tier (memory permitting) or keep 1 and document it.
+**Remediation:** Set `--timeout 60` (≥ API_FETCH_TIMEOUT × 2) in `render.yaml` and README/Docker/systemd examples; make the relationship explicit in config docs. Optionally lower `API_FETCH_TIMEOUT` to 15s for snappier failures. Keep **one worker and one instance** as the documented default: topology is coupled to rate-limit correctness (`memory://` counters are process-local, so the limit is multiplied by `workers × replicas`), so `--workers 2`, the README's `--workers 4` examples, and any `numInstances > 1` require a shared `RATELIMIT_STORAGE_URI` first (roadmap 2.10).
 
 **Effort:** 15m. **Verification:** a 35s mock API fetch no longer returns 502.
 
@@ -220,21 +224,39 @@ Measured on a reference payload (10 MB, ~200k rows of mixed nesting), single fre
 |---|---|---|
 | `/process` transfer size | ~10-20 MB | ≤ 2-4 MB (gzip) |
 | `/process` p95 latency (paste/upload only) | seconds (unbounded) | ≤ 3s |
-| Peak RSS, `/process` (10 MB input) | ~40-60 MB | ≤ ~50 MB; no OOM under the default 512 MB |
-| Peak RSS, `/export-xlsx` 100k rows | hundreds of MB (OOM risk) | ≤ 150 MB, streams |
+| Peak RSS **delta**, `/process` (10 MB input) | ~40-60 MiB | **≤ 50 MiB** delta (pass/fail), absolute high-water < 256 MiB |
+| Peak RSS **delta**, `/export-xlsx` 100k rows | hundreds of MiB (OOM risk) | **≤ 150 MiB** delta (pass/fail), absolute high-water < 256 MiB, streams |
 | Tree-picker open, 10 MB payload | multi-second freeze | ≤ 500 ms initial; lazy children |
 | Cell with 10k-key object | freeze | renders ≤ 20 keys + "more" |
 | Static assets | revalidated every load | cached ≥ 1 day |
 
 The latency target explicitly **excludes API-fetch requests** — those are bounded separately by `API_FETCH_TIMEOUT` (default 30s) plus DNS time (P7/F6) and would otherwise make a single aggregate p95 meaningless. The peak-RSS target reflects the full parse → flatten → `jsonify` pipeline (P12); it is not a claim that gzip or dropping one copy makes the process fit under 30 MB.
 
+**RSS measurement method (identical for `/process` and for the export measurement — both rows are pass/fail against these exact rules). Note `ru_maxrss` is a per-process high-water mark that cannot be reset, which drives the fresh-worker protocol below:**
+
+| Parameter | Definition |
+|---|---|
+| Units | **`ru_maxrss` is not portable.** On Linux it is **KiB**; on macOS/Darwin the same field is **bytes**. Read it, multiply by 1024 on Linux, and report everything in **MiB** (1 MiB = 1048576 bytes). Every memory threshold in this budget is written in **MiB**, not decimal MB. A harness that skips the platform conversion silently reports numbers 1024× off. |
+| Environment parity | Both runs of a pair must execute on the **same OS and kernel, the same container image, the same Python build, and the same cgroup/container resource limits**, on an otherwise idle host. A baseline from one image and a measurement from another is not a delta. Record all of these alongside the result. |
+| Peak, and why not sampling | `resource.getrusage(RUSAGE_SELF).ru_maxrss`, read **in the worker** after the response is fully delivered. The kernel maintains this high-water mark continuously, so no transient spike during JSON serialization or XLSX zip assembly can slip between samples. 50 ms sampling from a monitor thread is kept only as a **supplementary trace** for locating *where* the peak occurs; a sampled maximum is never the number reported. |
+| **No warm-up inside a measured process** | `ru_maxrss` is **monotonic for the lifetime of the process** — it cannot be reset, and any earlier peak stays in it. A warm-up request served by the same worker therefore leaves its own peak behind, and subtracting a post-warm-up baseline would not isolate the measured request (a large warm-up could even produce a false failure). So each measured run uses a **fresh worker that serves exactly one request** and is then discarded. First-request costs (lazy imports, the openpyxl module tree, cold allocator arenas) are deliberately *inside* the measurement — they are real memory the first request after a restart pays. |
+| Two runs, not two samples | Because the counter cannot be reset mid-process, the delta is taken **across two fresh-worker runs of the same build**, never within one: `baseline` = `ru_maxrss` of a freshly booted worker that has served **zero** requests; `measured` = `ru_maxrss` of a freshly booted worker that has served **exactly one** request. `delta = measured − baseline`. Both operands are absolute high-water marks of comparable processes, so the subtraction is meaningful. |
+| Optional refinement | On cgroup v2, `memory.peak` for a dedicated per-run cgroup **is** resettable (write to it), which allows a true within-process warm-up-then-measure delta. Use it if the harness has cgroup control; report which mechanism was used, since the two are not interchangeable. |
+| Absolute vs delta | Both are recorded and reported. The **delta** is the portable pass/fail target (it cancels interpreter/build differences). The **absolute** high-water mark from the measured run is what the no-OOM criterion is checked against, since a delta says nothing about total footprint. |
+| Concurrency | **1** — exactly one in-flight request, single gunicorn worker (`--workers 1 --threads 1`), no other traffic. |
+| Payload | The fixed reference payload (10 MB, ~200k rows of mixed nesting) for `/process`; a 100k-row `csv_data` body for `/export-xlsx`. Both committed as fixtures/generators so runs are reproducible. |
+| Verdict | **Pass** iff the cross-run delta `≤ 50 MiB` (`/process`) or `≤ 150 MiB` (`/export-xlsx`) **and** the measured run's absolute high-water mark stays under half the container limit (256 MiB of the default 512 MiB), as the **median of 3 run-pairs**; a single pair above a limit is retried, two of three above it is a fail. |
+| Blocked pairs are failures | A pair that cannot produce a number — OOM kill, worker crash, request timeout, truncated or incomplete response, or a missing/unreadable `ru_maxrss` — **counts as a failed pair and stays in the set of three**. It is never dropped, re-rolled as though it had not happened, or treated as a clean sample. An OOM is the single most important signal this budget exists to catch; discarding it as "no data" would invert the result. Record the failure mode with the run. |
+
+These are manual/CI-optional measurements (no perf tests gate CI in v1.2), but the numbers reported against this budget must be produced by exactly this method or they are not comparable.
+
 ---
 
 ## 5. Design Constraints That Bound the Fixes
 
 - **No server-side payload persistence** (`MEMORY.md` 2026-05-12): the full-dataset-in-`/process`-response design must not be replaced with server-side caching/sessions for export. All fixes must respect this (compression, streaming, client-side lazy rendering are compatible; a server-side export token is not).
-- **No disk writes of payloads** (`AGENTS.md`): export buffering must not rely on OS temp files. openpyxl `write_only` mode and `SpooledTemporaryFile` are therefore off the table unless a documented exception (roadmap D6) is approved.
+- **No disk writes of payloads** (`AGENTS.md`): export buffering must not rely on OS temp files. openpyxl `write_only` mode is therefore off the table unless a documented exception (roadmap D6) is approved, and `SpooledTemporaryFile` is off the table for a different reason — at its default `max_size=0` it never rolls over and so saves no memory, while any non-zero threshold (or a `fileno()` call) puts payload bytes on disk.
 - **No frontend framework / build step**: all client fixes are vanilla JS.
 - **Strict CSP, no inline JS**: any new client code stays in `static/js/app.js`.
 - **Exact-pinned dependencies**: any new dependency (e.g. `Flask-Compress`) must be pinned exactly and added deliberately; the zero-dependency middleware is preferred.
-- **Per-worker in-memory rate limiter**: benchmark results should assume no shared counters across workers.
+- **Process-local in-memory rate limiter**: `RATELIMIT_STORAGE_URI` defaults to `memory://`, whose counters are **process-local** — the effective limit is multiplied by `workers × replicas`, not by workers alone. The default deployment therefore stays at one worker and one instance; any configuration above that (roadmap 2.8/2.10) must set a shared `RATELIMIT_STORAGE_URI` (e.g. Redis). Benchmarks must record both counts and the storage backend, and assume no shared counters on `memory://`.