Skip to content

fix(server): lazily open stores on cache miss (multi-replica 404) - #135

Merged
beinan merged 1 commit into
lance-format:mainfrom
beinan:fix-multi-replica-lazy-open
Jul 14, 2026
Merged

fix(server): lazily open stores on cache miss (multi-replica 404)#135
beinan merged 1 commit into
lance-format:mainfrom
beinan:fix-multi-replica-lazy-open

Conversation

@beinan

@beinan beinan commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Problem

The server caches each opened store in one process's memory and treats present in the local HashMap as the store exists.

Under a multi-replica deployment (N pods behind a Service):

  • create lands on pod A → store enters only A's in-memory map (dataset is really created on shared object storage).
  • A subsequent add/list/get is load-balanced to pod B → B's map has no entry → 404 "store does not exist", even though the dataset exists on blob.

Single-pod localhost tests never exercised the cross-pod path, so this was missed. Same bug affects both rollout stores and context stores (same RwLock<HashMap> pattern).

Fix

Read/write handlers now fill the cache lazily on a miss by loading the store from object storage — never creating it.

  • RolloutStore::open_existing_with_options / ContextStore::open_existing_with_options: load an existing dataset, return DatasetNotFound (→ 404) when absent. A genuinely-missing store must still 404, not silently materialize an empty table for a mistyped name. (Existing open_with_options refactored to share via a create_if_missing flag.)
  • AppState::get_or_open_rollout_store / get_or_open_context_store: fast-path cache read → slow-path load without holding the map write lock → concurrent re-check before insert → (rollout) spawn_periodic_cleanup on the lazily-opened store so it still gets the background WAL-cleanup timer.
  • All rollout + context read/write handlers switch to these helpers. create/list/delete keep their existing semantics.

Key correctness points

  • load, not open: read path must not create-on-absence (would mask 404s / build empty tables for bad names).
  • concurrent re-check: two requests racing to open the same store → the loser drops its handle and reuses the winner's.
  • don't drop the cleanup timer: lazy-opened rollout stores get spawn_periodic_cleanup too.
  • credentials: lazy open passes storage_options: None, relying on the pod's workload-identity env (same as production create).

Test plan

  • New regression tests (rollout + context): a second AppState (empty cache, shared data dir) serves reads/writes for a store created by the first instance; a never-created store still 404s.
  • cargo test -p lance-context-core -p lance-context-server → 114 + 31 pass, 0 fail
  • cargo fmt --all --check, cargo clippy -p lance-context-server -p lance-context-core --all-targets clean

Note: cross-pod behavior should also be verified in a real multi-replica env (port-forward to the Service so requests hit different pods) — the unit tests simulate it with two AppStates over one data dir.

🤖 Generated with Claude Code

@beinan @claude
fix(server): lazily open stores on cache miss for multi-replica corre…
5f87199 
…ctness

The server cached each opened store in one process's memory and treated
"present in the local HashMap" as "the store exists". Under a multi-replica
deployment (N pods behind a Service) a store created on pod A only entered
A's map, so an add/list/get that the load balancer routed to pod B returned
404 — even though the dataset existed on shared object storage. Single-pod
localhost tests never exercised the cross-pod path, so this was missed.

Fix: read/write handlers now fill the cache lazily on a miss by LOADING the
store from object storage (never creating it), via new load-only core
entry points:

- RolloutStore::open_existing_with_options / ContextStore::open_existing_with_options
  load an existing dataset and return DatasetNotFound (-> 404) when absent,
  instead of the create-on-absence fallback used by the create route. A
  genuinely missing store must still 404, not silently materialize an empty
  table for a mistyped name.
- AppState::get_or_open_rollout_store / get_or_open_context_store: fast-path
  cache read, slow-path load without holding the map write lock, concurrent
  re-check before insert, and (for rollout) spawn_periodic_cleanup on the
  lazily-opened store so it still gets the background WAL-cleanup timer.

All rollout and context read/write handlers switch from the cache-miss-404
lookup to these helpers. create/list/delete keep their existing semantics.

Regression tests: a second AppState (empty cache, shared data dir) serves
reads/writes for a store created by the first instance, and a never-created
store still 404s — for both rollout and context stores.

Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
@beinan
beinan merged commit f948266 into lance-format:main Jul 14, 2026
9 checks passed
@beinan beinan mentioned this pull request Jul 14, 2026
Merged
beinan added a commit that referenced this pull request Jul 14, 2026
@beinan @claude
feat(server): scale rollout stores via registry + LRU + global sweeper (
5784d3b 
#136)

## Why

We want to partition rollouts by `experiment_name` — one physical Lance
dataset per experiment. At the target scale (tens to hundreds of
thousands of experiments) the current server runtime cannot cope:

1. `rollout_stores` is an **unbounded `HashMap`** of open handles →
never evicted → OOM.
2. Each store spawns its **own tokio cleanup timer**
(`spawn_periodic_cleanup`) → 100k+ background timers wedge the process.
3. `list_rollout_stores` only enumerates the **in-memory cache**, so
with eviction it would be incomplete, and there is no durable directory
to enumerate.

This PR decouples resident memory / background tasks from the number of
stores.

## What

- **Durable registry** (`RolloutRegistry`, new
`crates/lance-context-core/src/registry.rs`): a single small Lance table
(`name`, `uri`, `created_at`) that is the source of truth for which
stores exist. `create` / `list` / `delete` / existence checks now go
through it. Existence is no longer inferred from cache membership, so
eviction cannot cause a false 404, and `list` returns every store.
- **Bounded LRU** replaces the unbounded `HashMap`; evicted stores are
transparently reopened from storage on the next request (reusing
`open_existing_with_options` from #135).
- **Single global sweeper** (`AppState::spawn_global_sweeper`) replaces
one-timer-per-store. It sweeps only the resident set each tick, with the
same per-pass `tokio::time::timeout` hardening as #133. Cold (evicted)
stores accrue no new generations and are swept again once re-opened.
- `RolloutStoreInfo.version` → `Option<u64>` (omitted in `list` to avoid
opening every dataset; populated for single get/create). Client updated
(`unwrap_or(0)`).
- New `--rollout-cache-capacity` / `ROLLOUT_CACHE_CAPACITY` (default
2000).

## Tests

- `registry.rs`: upsert idempotency, contains/remove, list,
survive-reopen.
- Server: evicted-store-transparently-reopened (capacity-1 LRU),
list-enumerates-all-registered, delete-unregisters, sweeper gating.
Existing multi-replica regression tests (#135) still pass.
- `cargo test --workspace`, `cargo clippy --workspace --all-targets`,
`cargo fmt --check` all clean.

## Follow-ups (out of scope)

- Startup reconciliation (scan `data_dir` to backfill registry rows if a
crash left a dataset without its row).
- Optional btree index on `registry.name` if the narrow scan becomes
hot.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@beinan