From: Javier Sagredo Date: Mon, 1 Jun 2026 00:03:03 +0000 (+0200) Subject: Add Claude's help X-Git-Url: https://git.sagredo.dev/?a=commitdiff_plain;h=367a0388489a5f9405778a477f897f1df4b95bb7;p=sula.git Add Claude's help --- diff --git a/scryer-findings.md b/scryer-findings.md new file mode 100644 index 0000000..fc4f564 --- /dev/null +++ b/scryer-findings.md @@ -0,0 +1,124 @@ +# Memory growth in sula: findings + +Investigation into why the sula process's resident memory (RSS) kept +increasing on every served request. Summary: there were **two distinct +causes** — a large, fixable heap leak, and a small, residual arena leak that +is an architectural limitation of Scryer Prolog. + +## Symptom + +Each served file increased RSS and it never came back down. Heavy use drove +the process past 15 GB. Re-requesting the same page repeatedly kept growing +RSS monotonically. + +## Cause 1 (major): Scryer never GCs the heap; it reclaims it only on backtracking + +Scryer Prolog has **no automatic global-stack (heap) garbage collector**. +There is no `gc` Prolog flag, and the marking code in +`scryer-prolog/src/machine/gc.rs` is term-traversal machinery that is *not* +wired into the run loop. The only mechanism that reclaims heap is +**backtracking**: returning to a choicepoint restores the heap top `H` to the +value saved in that choicepoint. + +The original accept loop `with_connection_loop/3` was deterministic and +forward-recursive. Every request, `serve_text/3` does +`phrase_from_file(seq(Body), File)` (`response.pl`), which reads the **entire +file onto the heap**, plus request-parsing terms. Because the loop never +backtracked, none of that heap was ever reclaimed — it grew by ~filesize per +request. + +Note: **TCO / last-call optimization does not help.** TCO reclaims the +*environment (local) stack*, never the heap. Adding a `!` to make the loop +tail-recursive did nothing for this. + +### Reproduction + +A properly tail-recursive loop reading a 270 KB file 2000× climbed to ~20 GB +RSS. The identical work in a failure-driven loop stayed flat: + +```prolog +% Grows without bound (deterministic, never backtracks): +loop(N) :- N =< 0 -> true ; phrase_from_file(seq(_), "f.dat"), N1 is N-1, loop(N1). + +% Flat (backtracks to between/3 every iteration -> heap truncated): +loop(N) :- between(1, N, _), ( phrase_from_file(seq(_), "f.dat") -> true ; true ), fail. +loop(_). +``` + +### Fix (in this repo) + +`with_connection_loop/3` was rewritten as a **failure-driven loop**: + +```prolog +with_connection_loop(Context, Socket, Kont) :- + repeat, + ( catch(setup_call_cleanup(, , ), E, handle_conn_error(E)) + -> true ; true ), + fail. +``` + +Each handled connection backtracks to `repeat`, so Scryer truncates the heap +back to a fixed point and the whole per-request allocation (file contents, +parse terms) is reclaimed. This eliminated the large per-request growth. + +## Cause 2 (residual): Scryer never reclaims arena slabs during execution + +After the heap fix, RSS still grew ~4–8 KB per request and never recovered. + +Scryer allocates sockets, TLS streams, bignums, etc. in an **arena**: an +append-only singly-linked list of heap-`Box`ed slabs (`arena.rs`, `fn alloc`). +Slabs are freed **only** in `Drop for Arena` (`arena.rs:585`), i.e. at process +exit. There is no incremental sweep, and **backtracking does not touch the +arena** (it is separate from the global stack). + +`close/1` calls `remove_stream` and `drop_payload` (frees the inner payload, +e.g. the rustls buffers), but the **slab husk** (header + struct shell) stays +linked forever. Every Gemini connection allocates a TCP stream slab + a TLS +stream slab, so ~4–8 KB/request is never reclaimed while the process runs. + +### Reproduction + +A failure-driven `open/close` loop (so the heap *is* reclaimed) still grows +~30 MB/s, monotonically: + +``` +t=1 RSS 53184 kB +t=2 RSS 82604 kB +t=3 RSS 112076 kB +... # linear, never decreases +``` + +### Why it isn't cheaply fixable + +A stream is represented on the heap as a `Cons` cell pointing at the arena +slab (`streams.rs`). Eagerly freeing the slab in `close/1` would leave that +heap cell dangling until backtracking truncates it — safe for sula's +failure-driven loop, but a use-after-free risk for arbitrary programs. The +correct general fix is a real arena mark-and-sweep GC (mark from heap, stacks, +and `indices.streams`; free unmarked slabs), which is a large, higher-risk +change. + +### Decision + +**Accept the residual leak** as a known Scryer limitation and mitigate +operationally: + +- periodic restart, or +- systemd: `MemoryMax=512M` + `Restart=always`. + +This is documented inline in `with_connection_loop/3` in `sula.pl`. + +## Companion changes in the Scryer fork + +These reduce the *peak* per-connection footprint but do not fix the slab-husk +growth (Cause 2). On `../scryer-prolog`, branch `js/fixes`, commit +`52d70bcb`: + +1. `Stream::close` (`src/machine/streams.rs`) — the `NamedTcp` and `NamedTls` + arms now call `drop_payload()` after shutting down the socket / sending + `close_notify`, releasing the rustls `ServerConnection` buffers immediately + instead of waiting for the arena `Drop` at exit (matches file/byte/pipe + stream variants). +2. `tls_accept_client` (`src/machine/system_calls.rs`) — caches the + `Arc` keyed by the raw cert/key bytes, so the certificate + chain and (4096-bit) private key are not re-parsed on every connection. diff --git a/scryer-patches.md b/scryer-patches.md new file mode 100644 index 0000000..1a8bc66 --- /dev/null +++ b/scryer-patches.md @@ -0,0 +1,110 @@ +# Scryer Prolog patches for sula + +sula runs on a patched build of Scryer Prolog. The fork lives at +`../scryer-prolog` on branch **`js/fixes`**. These are the patches applied on +top of upstream, in order, starting at `0c9d76c8`: + +| Commit | Summary | +|------------|------------------------------------------------------| +| `0c9d76c8` | `copy_stream/2` | +| `8aaa6f57` | Fix `phrase_from_stream` infinite loop | +| `20df3c69` | Make `tcp_accept` poll for INTERRUPT (catchable C-c) | +| `840a56ff` | Switch TLS to rustls + expose client certificate | +| `52d70bcb` | Release TLS/TCP connection state on close + cache config | + +For the memory-related context behind `52d70bcb`, see +[`scryer-findings.md`](scryer-findings.md). + +--- + +## `0c9d76c8` — `copy_stream/2` + +Adds a new builtin `copy_stream(+In, +Out)` (exported from `library(sockets)`, +backed by `'$copy_stream'/2`). + +- **What:** copies all remaining bytes from `In` to `Out` in a single native + call using a 64 KiB buffer loop (`src/machine/system_calls.rs`, + `copy_stream`), without materialising the content on the Prolog heap. +- **Why:** serving a file by reading it into a list (`phrase_from_file`/`seq`) + and writing it back puts the whole file on the heap. For binary content + (file → TLS socket) this is wasteful; `copy_stream/2` streams it directly. +- **Used by:** `serve_binary/3` in `response.pl`. +- **Touches:** `build/instructions_template.rs`, `src/instructions.rs`, + `src/machine/dispatch.rs`, `src/machine/system_calls.rs`, + `src/lib/sockets.pl` (the usual 5 places needed to register a new builtin). + +## `8aaa6f57` — Fix `phrase_from_stream` infinite loop + +- **What:** in `src/lib/pio.pl`, `buffer_prepare_for_n/5` now treats + `get_n_chars/3` returning `[]` as **EOF** (sets the buffer tail to `[]` and + stops), instead of looping. +- **Why:** on streams whose `at_end_of_stream/1` never reports `true` — notably + **pipes** (e.g. the `openssl` subprocess used in `cert.pl` to read the + certificate subject) — the old code looped forever once the stream drained. + This made `phrase_from_stream/2` hang. +- **Used by:** `cert_is_for_hostname/2` in `cert.pl` (parses `openssl x509` + output over a pipe). + +## `20df3c69` — Make `tcp_accept` poll for INTERRUPT + +- **What:** `socket_server_accept` (`src/machine/system_calls.rs`) sets the + listener non-blocking and polls `accept()` in a loop, sleeping 100 ms on + `WouldBlock`. On each poll it checks the global `machine::INTERRUPT` flag; if + a SIGINT arrived it restores blocking mode and returns an `Err` carrying an + interrupt error so the dispatch site's `try_or_throw!` performs the + throw/backtrack correctly. +- **Why:** a blocking `accept()` syscall swallows Ctrl-C — the signal can't be + turned into a catchable `'$interrupt_thrown'` exception while the thread is + parked in the kernel. sula's top-level handler + (`handle_top_level_error(error('$interrupt_thrown', _))`) relies on that + exception to shut down cleanly. Polling lets the interrupt land in Prolog. +- **Trade-off:** up to ~100 ms latency between SIGINT and shutdown; negligible + accept latency since a ready connection returns immediately. + +## `840a56ff` — Switch TLS to rustls and expose the client certificate + +The largest patch. Replaces the `native-tls` backend with **rustls** +(`aws-lc-rs` provider) and reworks the server-side TLS API for Gemini's +certificate model. + +- **Dependencies (`Cargo.toml`):** `tls` feature now pulls in `rustls` (0.22, + `aws_lc_rs`), `rustls-native-certs`, `rustls-pemfile`, `aws-lc-rs`, `hex`; + drops `native-tls`. +- **Context API (`src/lib/tls.pl`):** `tls_server_context/2` now takes + `[certificate(Cert), key(Key)]` — **PEM** cert chain + private key (PKCS#8 / + PKCS#1 / SEC1) — instead of a PKCS#12 archive + password. +- **Negotiation API:** `tls_server_negotiate/3` → **`tls_server_negotiate/4`**, + gaining a `-ClientCert` argument. Backed by `'$tls_accept_client'/5` + (`src/machine/system_calls.rs`, `tls_accept_client`). +- **Client certificate (`ClientCert`):** the server *requests* a client cert + but does not require one (Gemini's TOFU model). After the handshake, + `ClientCert` is bound to either the atom `none` (anonymous client) or the + **SHA-256 digest of the client's leaf certificate**, returned as a list of + byte values. +- **`GeminiClientVerifier`** (custom `rustls` `ClientCertVerifier`): offers + client auth, does not make it mandatory, sends no CA hints, and accepts any + presented certificate **as-is** (no signature/expiry validation) — i.e. + trust-on-first-use, matching Gemini semantics. Signature-scheme support is + delegated to the aws-lc-rs provider. +- **Streams (`src/machine/streams.rs`):** the `NamedTls` stream variant is + reworked to wrap a rustls `StreamOwned`. +- **Used by:** `cert.pl` (`with_tls_connection/3` calls + `tls_server_negotiate/4`, threads `ClientCert` to the request handler) and + `sula.pl` (`req_serve/2` receives `_ClientCert`). + +## `52d70bcb` — Release connection state on close + cache `ServerConfig` + +Memory-footprint fixes for the per-connection TLS path (full context in +[`scryer-findings.md`](scryer-findings.md)). + +- **`Stream::close` (`src/machine/streams.rs`):** the `NamedTcp` and `NamedTls` + arms now call `drop_payload()` after the socket shutdown / `close_notify`, so + the rustls `ServerConnection` and its buffers are released immediately rather + than lingering until the arena is dropped at process exit (matches the + file/byte/pipe stream variants). +- **`tls_accept_client` (`src/machine/system_calls.rs`):** caches the + `Arc` keyed by the raw cert/key bytes, avoiding a re-parse of + the certificate chain and (4096-bit) private key on every connection. +- **Note:** this reduces *peak* footprint but does not stop the residual + ~4–8 KB/request growth from arena stream-slab husks — that is a Scryer + limitation (no incremental arena reclamation); see `scryer-findings.md`.