Skip to content

fix(stream): startup adopts or refuses a published stream, never replaces it - #177

Open
nhobin219 wants to merge 2 commits into
mainfrom
fix/startup-keeps-data
Open

nhobin219 wants to merge 2 commits into
mainfrom
fix/startup-keeps-data

Conversation

@nhobin219

Copy link
Copy Markdown
Owner

Fixes #144. Fixes the startup half of #146: the refusal at creation, but not the new/open API split (see below).

What was wrong

#144: a home that lost its metadata while its logs survived. serve (_metadata.ensure), migrate, retire and retain each described the log afresh with _metadata.single, which mints a new stream_id. ensure then uploaded that over the published copy.

  • Every reader that records the id was refused (StreamMismatch, the snapshot and catch-up id checks).
  • A migrated stream's copy also lost its sealed logs and its manifest.

#146: a home that is gone entirely. On a fresh box, Stream.new with the usual published= created an empty stream at the prefix.

  • Its first serve replaced the published metadata with the empty stream's.
  • With WAL replication, litestream 0.5.16 shipped the empty buffer as the replica's next transaction, so every later restore returned nothing.
  • litelink already refused a prefix whose table held data. A prefix holding only the metadata, only the replica, or both (a stream served before its first publish) was not refused.

What changes

  • _versions.adopt writes the remote copy's current version into a home that has none, manifest included. It is a compare-and-swap against no version. _versions._published is factored out of fetch so the manifest can be read by the name it has under the prefix.
  • _metadata.ensure adopts the copy when the home has no metadata, then takes its existing live-log check and complete path. When nothing changed, it does not upload again.
  • _metadata.recovered is adopt-or-single. migrate, retire and retain use it where they called single.
    • When the copy's live log is not the one opened, it refuses with a ValueError, after adopting. A home without metadata can only open the first log. Since the copy is now in the home, the next run opens the live log.
    • retain re-reads its compare-and-swap version after adopting, and retries on a Conflict.
  • _open_or_create (used by Stream.new):
    • On RetiredError with no local metadata (a migrated or retired stream that lost its metadata), it adopts the copy and asks the caller to run again. Before, it raised RetiredError with no way forward.
    • In the create branch, with a remote prefix, _versions.occupied looks for the stream's metadata, or anything one level under <prefix>/<stream>/ (the first log's table or its _wal replica). It checks both the stream's own prefix and the bare prefix, where a stream from before its own directory keeps them. If anything is there, it raises FileExistsError naming Stream.restore.
    • The check costs one metadata read and one one-level listing per prefix, and runs only when the stream is not on this disk. An existing home never lists, and a test asserts that.
  • SPEC §metadata and §8b, the Stream.new docstring and the CHANGELOG are updated.

Breaking?

It is a behaviour change on a path that destroyed data, so I'm calling it a fix: ship it as a patch, 0.23.1. Consumers need no pin move.

  • A deployment that wipes its disk and calls Stream.new on the same prefix to "start fresh" is now refused (FileExistsError). The CHANGELOG says so.
  • A prefix whose table held data was already refused, by litelink's ValueError. It is now refused with FileExistsError.

Not in this PR (#146)

The Stream.open / Stream.new split is new public API, and a breaking one. It needs its own decision. The startup hazard it was motivated by is closed here without it: new no longer creates over an existing replica.

The Store half of #146 no longer applies on main. Store is now the caller's own SQLite or Postgres database for the outbox and inbox. streamcast neither creates nor replicates it.

Tests

tests/test_startup_keeps_data.py has 15 tests, in the replication tier (rustfs).

  • On unfixed main (26e903d), 11 of the first 13 fail, for example:
    • test_serve_keeps_the_id: AssertionError: assert 'bc6cfe2f-…' == 'd5d4b9c2-…', a new id;
    • test_a_stream_served_before_its_first_publish: DID NOT RAISE FileExistsError, so the empty stream was created.
    • The 2 that pass there are the controls: no metadata anywhere still mints, and an empty prefix still creates.
  • The old-layout test and the read-only-retired test were added after that run. They exercise code that is new here.
  • Falsified. Each break below made tests fail; each was restored and passes:
what I broke tests that fail
ensure no longer adopts 1
recovered always mints 5
adopt drops the manifest 1
new never refuses 4
occupied without the listing 2
the RetiredError branch removed 2
only the stream's own prefix checked 1
  • retain's Conflict retry needs a concurrent first commit and has no test.
  • A missing bucket is not refused at new. occupied treats it as holding nothing, and serve still refuses it at the first upload, as before. The full suite found this: test_metadata.py::test_serve_refuses_to_start_when_it_cannot_upload failed until the change and passes with it.
  • Full suite, locally: STREAMCAST_REQUIRE_S3=1 STREAMCAST_REQUIRE_INTEROP=1 STREAMCAST_REQUIRE_POSTGRES=1 pytest gave 970 passed, 1 failed, 2 skipped in 23 min.
    • The failure is test_maintain.py::TestFlushing::test_a_served_row_reaches_the_published_table ([publish] skipped: another owner holds a claim). It fails the same way on unmodified main (26e903d) on this box, run alone. It uses a local log, which this change never touches.
    • Node is not installed here, so test_node.py is left to CI.
    • just typecheck reports two diagnostics in tests/test_limits.py. They appear on main locally too, and CI passes main.

Critical review

Two rounds with a general-purpose agent running the repo's critical-reviewer rubric verbatim. The bar was widened to: a published copy overwritten or lost, a stream that opened before and is refused now, and a stale view of the stream that is then published.

  • Round 1: no critical findings. It raised four non-critical points:
    1. A latent stale-copy case. A migrate's upload fails, then the home loses its metadata. Adoption refuses, but the guidance it gives does not lead out. This is not a regression: the old code minted over the copy here.
    2. retain let a Conflict from adopting escape its retry loop. Fixed.
    3. Old-layout prefixes were not checked. Fixed, with a test.
    4. Flag the wipe-and-new refusal loudly. Done, in the CHANGELOG.
  • Round 2: no critical findings. It checked that the extra prefix check adds no false refusal, and that retain's continue skips nothing and leaks no handle.

Downstream

No pin moves are required: it is a patch within <0.24. A consumer whose startup relies on Stream.new recreating a stream over its own prefix after a disk wipe should call Stream.restore instead.

🤖 Generated with Claude Code

A home that lost its metadata while its logs survived adopts the published
copy, manifest included. serve, migrate, retire and retain used to mint a
new stream_id instead and upload it over the copy, dropping its sealed logs
(#144).

Stream.new refuses to create a stream where its prefix already holds the
stream's metadata, table or WAL replica, and names Stream.restore. On a box
that lost its disk it used to create an empty stream, whose first serve
replaced the published metadata and whose buffer litestream shipped over the
replica (#146).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@nhobin219
nhobin219 force-pushed the fix/startup-keeps-data branch from 341c4d1 to 28555ad Compare October 21, 2026 23:56
@nhobin219

Copy link
Copy Markdown
Owner Author

Rebased onto main 6ffde4f0db245bb81c928f503f8da71bdedf6041 (includes #183). Current head: 28555ad08eeca9dc3c0557d154d6fc5d0ae0d6f7.

Source AST comparison confirms the rebase preserves executable source from the starting head. Documentation conflicts retain the main audit.

Local lint, formatting, and type checks pass; 15 focused regression tests pass. CI for this exact head succeeds: full Python 3.11 and 3.14 suites with S3, Node, interoperability and Postgres required, wheel packaging, lint, type checking, and CI success.

The inherited 76-character commit subject was shortened to comply with the 72-character rule. That amendment preserves the tested tree.

@nhobin219

Copy link
Copy Markdown
Owner Author

ARCHITECT review, pass 1, head dc3ec8f276bd855c78cce1232324348ec4dc6c73: CRITICAL — recovered retirement is bypassed by Stream.migrate.

src/streamcast/_stream.py:668: a stream is explicitly retired, its local metadata is lost while the log and published copy survive, and startup calls the existing idempotent Stream.migrate API with the unchanged schema. _open_retired ran before adoption, so it saw no metadata. recovered then loads a non-null retirement, but execution continues through _seal_and_succeed and returns a writable successor. The returned stream accepts/acknowledges rows even though its published metadata still says it is retired at the old end. This is an actual live/published-state divergence, not a hypothetical future caller: migrate is explicitly supported as startup and the existing retirement path requires it to return read-only. A subsequent revive trusts that old retirement frontier when sealing this successor, which can exclude those acknowledged rows.

Reproduced against the PR with real rustfs using its existing _served / _lose_metadata fixtures:

await _served(tmp_path, bucket, s3)  # 20 rows
Stream.retire('t', root=tmp_path, s3_options=s3)
_lose_metadata(tmp_path)
s = Stream.migrate('t', root=tmp_path, schema=SCHEMA, s3_options=s3)
# local metadata still records Retirement(end_offset=21)
# s.retirement is None; s.log.name == 't-v2'
await s.send({'i': 100})  # succeeds; must raise StreamRetired

The 15 existing startup tests pass; the additional behavioral assertion fails with DID NOT RAISE StreamRetired. Preserve the recovered retirement on this entrypoint (including retry/crash behavior), add/falsify the regression test, and return the author fix for re-review. Also rebase onto main containing merged #183 as already requested. Not clean to merge; no merge authorized.

Other pass-1 attacks found clear so far: new/legacy prefix refusal; existing-home no-listing path; ID/history/manifest preservation on the covered serve/retire/retain/new recovery paths. #178/#179/#181 are being reviewed separately. No nits or LATENT issues are being escalated.

@nhobin219

Copy link
Copy Markdown
Owner Author

ARCHITECT correction to the pass-1 review of head dc3ec8f276bd855c78cce1232324348ec4dc6c73.

Replace the sentence “A subsequent revive trusts that old retirement frontier when sealing this successor, which can exclude those acknowledged rows.” with this observed consequence:

A subsequent explicit revive trusts the old retirement frontier and reuses an acknowledged offset. The extended real-rustfs reproduction acknowledged recovery row i=100 at offset 21, then Stream.restore(..., revive=True) acknowledged i=101 at offset 21 again. A consumer that processed the first row and saved its next cursor at 22 silently skips the second row. A full snapshot scan includes both distinct rows at the same offset. This is a reproduced loss of the resume guarantee.

All other review text and author-fix requirements remain unchanged.

@nhobin219

nhobin219 commented Oct 22, 2026 •

Copy link
Copy Markdown
Owner Author

Author response to the ARCHITECT pass-1 finding and its observed-consequence correction, for re-review at head b83baff9feae66ba2a8d7f9ba182d63af1b64915.

migrate now checks the recovered metadata's retirement before any migration or successor creation. It closes the original handle and uses the canonical retired-open path, preserving schema validation, published replay, and refused sends. Only explicit restore(..., revive=True) creates the writable successor.

Behavioral evidence with real rustfs:

  • Before the fix, three new cases fail: the ordinary recovery and retry interrupted before the hint accept a forbidden send; changed-schema recovery is also incorrectly accepted.
  • All five new cases now pass. The recovered stream refuses i=100 with StreamRetired, creates no t-v2, and leaves published metadata unchanged. Explicit revival acknowledges i=101 at 21 and i=102 at 22; a WebSocket consumer resuming at 22 receives i=102. A full snapshot contains unique offsets 1–22 and excludes the refused row.
  • Injected interruptions before the adoption hint, immediately after the hint, and after adoption each survive two subsequent startup retries without accepting a send or changing the retirement frontier.
  • Falsified: dropping retirement from _read_only makes all four send/refusal cases fail; shifting revival's start offset by one makes the offset assertion fail. The schema case also fails on the unfixed code. Restoring the source gives 93 passing startup, retirement, migration, and invariant tests. No existing tests were removed.
  • The required critical review ran using the repository rubric via the general-purpose fallback. It found no additional critical findings in recovery, interruption handling, ownership/schema validation, or revival/resume. This does not replace ARCHITECT re-review.

Verification: lint, formatting, type checks, and commit hooks pass. Local just check-all, with S3, Node, interoperability, and Postgres required, finished 975 passed, 2 failed, 1 deliberate SQLite-only skip. The disk-cache file assertion fails identically on current main; the replication waiter returned after only catalog.db appeared, although the captured log shows buffer.db uploaded shortly afterward. That replication case passes in isolation on main and this head. These local failures are retained as limitations, not reported as a passing local gate.

This head includes current main and #183 at 6ffde4f0db245bb81c928f503f8da71bdedf6041. Exact-head CI: all six jobs succeeded, including 977 passed and 1 deliberate SQLite-only skip on both Python 3.11 and 3.14, with S3, Node, interoperability, and Postgres required.

This remains a patch fix with no new public API or consumer pin change. Re-review is requested; no merge is performed or authorized by this response.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

serve mints a new stream_id for an existing stream whose local metadata is gone, and overwrites the published copy's

1 participant