Skip to content

docs(rollout): RolloutStore::add doc comment still promises synchronous visibility #183

Description

@beinan

Summary

After #181 (feat(rollout): concurrent writes with periodic flush, 8b2c49c), RolloutStore::add no longer seals and drains on the append path — it only does a durable put. The doc comment on add was not fully updated and still describes the old contract.

Evidence

crates/lance-context-core/src/rollout_store.rs:520-542 (doc comment):

Each append is sealed and its flushed generation committed to the shard manifest before returning, so the rows are immediately visible to reads on any instance

The per-append work is put (WAL-durable) → force_seal_active (freeze this append's memtable) → wait_for_flush_drain (await the generation's manifest commit)

The actual implementation at rollout_store.rs:543-576 performs only put. force_seal_active + wait_for_flush_drain now live in flush() (rollout_store.rs:621-640), driven periodically by the server's flush sweeper (crates/lance-context-server/src/state.rs:474-518), default interval 30s (crates/lance-context-server/src/config.rs:58).

The inline comment at rollout_store.rs:549-555 already states the correct new semantics ("Read-after-write is therefore asynchronous, up to the flush interval; durability is not"), so the doc comment directly contradicts the code a few lines below it.

Why this matters

This is a public API doc comment that ships in rustdoc. It tells every downstream caller that a write is globally visible on return, when in the default configuration it may take up to 30 seconds. Durability is genuinely unchanged — only visibility regressed — but the contract as written is wrong.

Proposed fix

Rewrite the doc comment to state:

  • add is durable on return (put waits for the WAL entry to reach object storage)
  • add is not read-your-write visible; visibility is bounded by the flush interval
  • point at flush() for callers that need synchronous visibility

Remove the stale three-step put → force_seal_active → wait_for_flush_drain description.

Notes

Not a correctness bug. Rollout rows are immutable and the LSM read path dedups by id (rollout_store.rs:1690-1732), so the decoupling does not introduce duplicates.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions