Keep TLS session resumption alive across config reloads

[kriszyp]

Human-Review-Need: 4 @ 4205895

What this is

symphony already supports TLS session resumption, and it works — but only until the first config reload. This makes it survive one.

The problem

src/tls.rs sets both halves of resumption on every terminating ServerConfig (rustls defaults to neither):

• session_storage = ServerSessionMemoryCache::new(1024) — TLS 1.2 session IDs
• ticketer = ring::Ticketer::new() — TLS 1.3 tickets

Both of those live on the ServerConfig, and build_route_table created its own TlsConfigCache per call. So every route-table rebuild minted a brand-new ServerConfig for every route — including routes whose cert had not changed at all — which discarded the session cache and generated fresh random ticket keys. Every ticket already handed to a client became undecryptable.

Nothing errors when this happens. Clients just quietly go back to full handshakes.

That matters because reloads are routine: adding or removing a route, or an on-disk cert renewal picked up by the file watcher, rebuilds the whole table. In a multi-tenant deployment, resumption rarely survived long enough to do anything.

Measured against main with a Node client, identical routes and an identical cert on both sides of the reload:

=== TLSv1.3 ===
  1st connect:                       reused=false   (expected — nothing to resume)
  2nd connect (same session):        reused=true    resumption works
  3rd connect after updateConfig():  reused=false   <-- reload wiped it

=== TLSv1.2 ===
  same shape

The fix

The proxy owns the TlsConfigCache and threads it through every build_route_table, so an unchanged cert maps to the same Arc<ServerConfig> and keeps its session state across hot-swaps. Keying on the cert bytes gives the right lifetime for free: session state lives exactly as long as the cert it was issued under, and rotating a cert retires it.

Two details are load-bearing and worth a close look:

• The sweep is the caller's, and runs at the commit point. retain_used() (mark-and-sweep over the keys touched during a build) runs in proxy.rs after the new table is swapped in — never inside build_route_table. updateConfig is all-or-nothing: a route build can succeed and then be discarded when the protection half fails validation, and sweeping against a table that never went live would retire configs the still-running table is serving from. Over-retaining for one generation is the safe direction; under-retaining costs live session state.
• Ticket keys stay per-config, not process-global. A single process-wide ticketer is the obvious optimization here and it's the wrong one: it would let a ticket minted under one tenant's cert resume against another tenant's route.

Not addressed (both documented in CLAUDE.md)

• rustls' ring Ticketer rotates its keys on its own ~6h schedule, independent of reloads.
• Ticket keys are per-process, so during host-manager's overlapping-process upgrade window, clients that land on the new process fall back to a full handshake until they hold one of its tickets.

Both degrade to a full handshake, never to an error.

Tests

• router.rs — unchanged_cert_keeps_its_server_config_across_rebuilds asserts config identity across a rebuild, and carries the old per-build cache as an explicit control so the test states what the bug was. sweep_retires_configs_no_route_references covers the eviction side.
• __test__/session-resumption.spec.ts — end-to-end over both TLS 1.2 and 1.3: a session is issued and resumes, it survives an updateConfig() that doesn't change the cert, and it correctly does not resume across a cert rotation.

I confirmed the reload assertions fail on the previous behaviour before keeping them — reverting just the shared-cache change turns those two green tests red and leaves the rest passing.

Full suite: 110/110 JS, 107/107 Rust, clippy clean.

Note on the diff: cargo fmt and prettier --write reformat this repo wholesale (it isn't formatter-clean today), so I reverted that churn and hand-matched the surrounding style. The diff is only the change.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request implements TLS session resumption across configuration reloads by threading a long-lived TlsConfigCache through the route table build process, ensuring that unchanged certificates retain their ServerConfig allocations (and thus their session caches and ticket keys). It also introduces a mark-and-sweep mechanism to retire rotated-out certificates and adds comprehensive tests. The review feedback correctly identifies a potential issue where aborted or failed configuration builds can leave stale keys in the cache's used set, leading to temporary memory leaks on subsequent successful reloads. It is recommended to implement the suggested clear_used method and invoke it at the start of each route table build to prevent this.

[Devin-Holland]

Reviewed at head 6348b46. Built the addon, ran both suites, and probed the sweep rather than reading it — which turned up one real gap.

1. should-fix (confirmed empirically) — the sweep retires a config the committed table is still serving from. The carry-forward branch at router.rs:424 (prev.clone(), the mid-rotation cert/key mismatch path) reuses the previous table's Arc<ServerConfig> without going through get_or_build, so its cache key is never marked, and retain_used() then drops it.

   I added a temporary test at this head to confirm: after a mismatch carries the route forward, cache.len() goes to 0 while the live table still serves that exact config — so there is no immediate breakage, and clients keep resuming. The cost lands on the next rebuild: when the mismatch heals back to the same cert bytes, get_or_build misses and mints a different ServerConfig (I printed the pointers: 0x125805f90 vs 0x125804db0) — fresh ticketer, so every outstanding ticket for that tenant dies. That is exactly the failure this PR removes, reappearing through the one path the carry-forward logic exists to protect.

   Worth noting the proxy.rs comment states the intended invariant — "retire the ServerConfigs that no route in the new table asked for" — and a carried-forward route is in the new table. The mark is keyed on build participation rather than table membership. Nothing covers this: the e2e spec exercises reload and rotation, never a mid-rotation mismatch.

   I validated a one-line fix in a worktree — it makes the probe pass and keeps the existing eviction test green (full lib suite 123/123 with the probe added):

   self.cache.retain(|k, v| used.contains(k) || Arc::strong_count(v) > 1);

   The cache holds one strong reference, so > 1 means a live table (or an in-flight connection) still holds it. The trade-off is over-retention: a rotated-out cert's config lingers while the old table or a long-lived connection references it — the direction this PR already declares safe, and bounded by connection lifetime rather than unbounded. The more explicit alternative is storing the CacheKey on Route so the carry-forward branch can re-mark directly; Route currently keeps only the Arc (router.rs:167).

2. note, latent — not reachable today — the build and the commit-point sweep are not one critical section, and the mark set is shared. Two interleaved update_config calls (A builds, B builds, B commits and sweeps clearing used, A commits and sweeps against an empty mark set) would empty the entire cache. Not reachable now: update_config is a synchronous #[napi] method (proxy.rs:561-562), so JS cannot interleave two of them. Worth a clause in the comment beside the sweep, because it becomes reachable the moment that signature goes async — and the strong_count guard above happens to defuse it as well.

Verified (ran it, not read it):

• Rust suite at this head: 122 pass / 0 fail, including both new tests. unchanged_cert_keeps_its_server_config_across_rebuilds earns its keep — the explicit per-build-cache control is what makes it state the bug rather than merely assert current behavior.
• Built the debug addon and ran the new e2e spec. All three subtests green across TLS 1.3 and 1.2: a session is issued and resumes, it survives an updateConfig() that leaves the cert alone, and it correctly does not resume across a rotation. The spec is wired into npm test, so it will not rot unrun.
• The sweep-at-the-commit-point argument holds. update_config builds the route table and can still bail on protection validation before the swap, so sweeping inside build_route_table really would retire configs the still-live table is serving from. drop(cache) before validation and re-locking after the swap is the right shape.
• Per-config ticketer rather than process-global is right for tenant isolation. Routes sharing identical cert bytes do share a config and therefore ticket keys — that is the same TLS identity, and routing stays SNI-based off the ClientHello, so no cross-tenant reachability follows from it.
• Lock handling is deliberately asymmetric and correct — the build path turns a poisoned mutex into an error, the commit path uses if let Ok and skips the sweep, which over-retains (safe) rather than dropping live state.
• No CI regression from the new API surface. cargo clippy --all-targets, which is what CI runs and it does not deny warnings, reports 12 warnings on both this head and origin/main; the new len/is_empty only look dead in a lib-only build.

Verdict: comment.

— Claude & DevAIn (Claude Opus 5)

[Devin-Holland]

Re-reviewed at head 4205895d. Both findings from the last round are fixed. I probed each half by reverting it rather than by reading the new tests, and both new tests fail without their fix — they earn their keep.

Round-1 findings: both closed

1. The sweep retired a carried-forward config — fixed by the Arc::strong_count(config) > 1 clause in retain_used. Reverting just that clause turns carried_forward_config_survives_sweep red (123 pass / 1 fail), so the test pins the actual bug rather than merely asserting current behaviour.

2. Gemini's stale-marks leak — fixed by clear_used() at the top of build_route_table, in a stronger form than the suggested one. Reducing clear_used to the suggested self.used.clear() alone turns aborted_build_marks_do_not_retain_configs red, as does removing the call entirely. Worth stating why the stronger form matters: retain_used never runs on the abort path, so back-to-back failing reloads — a persistently-invalid protection config retried by the reconcile loop — would add one config per attempt with no upper bound. The extra retain inside clear_used is what bounds that, and the test's first assertion is the one that catches it.

Verified (ran it, not read it)

• Lib suite at this head: 124 pass / 0 fail. npm test: 124 pass / 0 fail across 43 suites, including the session-resumption spec.
• The unmarked-Arc surface is closed. get_or_build has exactly one caller (build_route, router.rs:540), and listener-level TLS only supplies fallback cert bytes — it never holds a ServerConfig of its own. So a live route acquires its config in exactly two ways: get_or_build (marked) and the carry-forward prev.clone() (unmarked, now covered by refcount). There is no third path to miss.
• Over-retention is bounded by connection lifetime, not unbounded. proxy_conn.rs:188 clones the route's Arc<ServerConfig> into per-connection state, so a rotated-out config lingers only as long as the longest connection that used it, then goes on the next sweep.
• The interleaving note from last round is now defused, not merely unreachable. A live table's configs always have strong_count >= 2, so even two interleaved update_config calls trampling the shared mark set could not evict live state.

Blocking, but mechanical: this doesn't merge

main is 6 commits ahead (route-scoped proxy metrics + duplicate-route isolation) and touched build_route_table on the same lines. I resolved it to check for a semantic conflict, and there isn't one — it's two hunks plus five call sites:

• router.rs, the build_route call — take main's let mut route with this PR's cache (no longer a local): let mut route = match build_route(spec, listener_tls, cache) {
• router.rs, transient_failure_retains_last_good_on_hot_swap — keep main's metric_identity assertions, add the 4th argument.
• Five build_route_table(...) calls in main's new metrics tests need &mut TlsConfigCache::new().

Merged result: 130/130 Rust, 131/131 JS, both new tests among them.

Design note — no action needed

The mark set is now redundant in production. Because swap() precedes the sweep, a committed table's configs always have strong_count >= 2, so used never decides anything. I checked by deleting used entirely and evicting purely on refcount: 124/124 still pass, the only edit needed being to bind the table in sweep_retires_configs_no_route_references (it currently discards it — the one thing production never does).

I am not suggesting you change it: used is the more explicit contract, and pure refcounting over-retains silently if anything ever stashes an Arc<ServerConfig> outside a route. But it does mean the refcount clause, not the mark set, is what makes this correct now — and nothing in the file says so. Hence the one inline comment below.

Verdict: approve. Both inline notes are comment-only; the rebase is the only thing to do before merge.

— Claude & DevAIn (Claude Opus 5)

[Devin-Holland]

strong_count is the half that fixes the carried-forward bug, and it currently reads as redundant next to the explicit mark set — a future cleanup could delete it and silently reintroduce the fresh-ticketer regression. Same for the retain in clear_used, which is what bounds repeatedly-failing reloads.

Suggested change

	/// Drop every entry not requested since the previous sweep, retiring rotated-out certs.
	/// Callers run this once they *commit* the table they just built — see `build_route_table`.
	pub fn retain_used(&mut self) {
	let used = std::mem::take(&mut self.used);
	self.cache.retain(|k, config| used.contains(k) || Arc::strong_count(config) > 1);
	}
	/// Discard marks and cache-only configs from a route table that was never committed.
	pub(crate) fn clear_used(&mut self) {
	self.used.clear();
	self.cache.retain(|_, config| Arc::strong_count(config) > 1);
	}
	/// Drop every entry not requested since the previous sweep, retiring rotated-out certs.
	/// Callers run this once they *commit* the table they just built — see `build_route_table`.
	///
	/// `strong_count` is load-bearing, not defensive: the carry-forward branch in
	/// `build_route_table` reuses the previous table's `Arc` without going through `get_or_build`,
	/// so a mid-rotation route is live but unmarked.
	pub fn retain_used(&mut self) {
	let used = std::mem::take(&mut self.used);
	self.cache.retain(|k, config| used.contains(k) || Arc::strong_count(config) > 1);
	}
	/// Discard marks and cache-only configs from a route table that was never committed.
	/// `strong_count == 1` means only the cache holds it, so no live table can lose one here;
	/// without the retain, repeatedly-failing reloads accumulate a config per attempt.
	pub(crate) fn clear_used(&mut self) {
	self.used.clear();
	self.cache.retain(|_, config| Arc::strong_count(config) > 1);
	}

[Devin-Holland]

This paragraph now understates the function: it says sweeping is the caller's job, but the build calls clear_used, which does evict. The invariant still holds — clear_used can only drop entries no live table references — but a reader stopping here would conclude the build never touches cache lifetime.

Suggested change

	/// `cache` is the proxy's long-lived `TlsConfigCache`, deliberately *not* created here: an
	/// unchanged cert must map to the same `Arc<ServerConfig>` across rebuilds so its TLS session
	/// state survives the reload. Sweeping it (`retain_used`) is the *caller's* job, once it commits
	/// the returned table — this table is only one half of an all-or-nothing `updateConfig`, and
	/// retiring configs for a table that never goes live would strand the running table's sessions.
	/// `cache` is the proxy's long-lived `TlsConfigCache`, deliberately *not* created here: an
	/// unchanged cert must map to the same `Arc<ServerConfig>` across rebuilds so its TLS session
	/// state survives the reload. Sweeping it (`retain_used`) is the *caller's* job, once it commits
	/// the returned table — this table is only one half of an all-or-nothing `updateConfig`, and
	/// retiring configs for a table that never goes live would strand the running table's sessions.
	/// The `clear_used` below is not that sweep: it drops only configs nothing references yet, i.e.
	/// the previous build's, if that update was abandoned.