Repository navigation
Conversation
dc3ec8f to
341c4d1
Compare
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>
341c4d1 to
28555ad
Compare
|
Rebased onto main 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 The inherited 76-character commit subject was shortened to comply with the 72-character rule. That amendment preserves the tested tree. |
|
ARCHITECT review, pass 1, head
Reproduced against the PR with real rustfs using its existing 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 StreamRetiredThe 15 existing startup tests pass; the additional behavioral assertion fails with 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. |
|
ARCHITECT correction to the pass-1 review of head 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 All other review text and author-fix requirements remain unchanged. |
|
Author response to the ARCHITECT pass-1 finding and its observed-consequence correction, for re-review at head
Behavioral evidence with real rustfs:
Verification: lint, formatting, type checks, and commit hooks pass. Local This head includes current main and #183 at 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. |
Fixes #144. Fixes the startup half of #146: the refusal at creation, but not the
new/openAPI split (see below).What was wrong
#144: a home that lost its metadata while its logs survived.
serve(_metadata.ensure),migrate,retireandretaineach described the log afresh with_metadata.single, which mints a newstream_id.ensurethen uploaded that over the published copy.StreamMismatch, the snapshot and catch-up id checks).#146: a home that is gone entirely. On a fresh box,
Stream.newwith the usualpublished=created an empty stream at the prefix.servereplaced the published metadata with the empty stream's.What changes
_versions.adoptwrites the remote copy's current version into a home that has none, manifest included. It is a compare-and-swap against no version._versions._publishedis factored out offetchso the manifest can be read by the name it has under the prefix._metadata.ensureadopts the copy when the home has no metadata, then takes its existing live-log check andcompletepath. When nothing changed, it does not upload again._metadata.recoveredis adopt-or-single.migrate,retireandretainuse it where they calledsingle.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.retainre-reads its compare-and-swap version after adopting, and retries on aConflict._open_or_create(used byStream.new):RetiredErrorwith 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 raisedRetiredErrorwith no way forward._versions.occupiedlooks for the stream's metadata, or anything one level under<prefix>/<stream>/(the first log's table or its_walreplica). 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 raisesFileExistsErrornamingStream.restore.Stream.newdocstring 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.
Stream.newon the same prefix to "start fresh" is now refused (FileExistsError). The CHANGELOG says so.ValueError. It is now refused withFileExistsError.Not in this PR (#146)
The
Stream.open/Stream.newsplit 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:newno longer creates over an existing replica.The
Storehalf of #146 no longer applies on main.Storeis 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.pyhas 15 tests, in the replication tier (rustfs).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.ensureno longer adoptsrecoveredalways mintsadoptdrops the manifestnewnever refusesoccupiedwithout the listingRetiredErrorbranch removedretain'sConflictretry needs a concurrent first commit and has no test.new.occupiedtreats it as holding nothing, andservestill refuses it at the first upload, as before. The full suite found this:test_metadata.py::test_serve_refuses_to_start_when_it_cannot_uploadfailed until the change and passes with it.STREAMCAST_REQUIRE_S3=1 STREAMCAST_REQUIRE_INTEROP=1 STREAMCAST_REQUIRE_POSTGRES=1 pytestgave 970 passed, 1 failed, 2 skipped in 23 min.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 unmodifiedmain(26e903d) on this box, run alone. It uses a local log, which this change never touches.test_node.pyis left to CI.just typecheckreports two diagnostics intests/test_limits.py. They appear onmainlocally too, and CI passesmain.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.
retainlet aConflictfrom adopting escape its retry loop. Fixed.retain'scontinueskips nothing and leaks no handle.Downstream
No pin moves are required: it is a patch within
<0.24. A consumer whose startup relies onStream.newrecreating a stream over its own prefix after a disk wipe should callStream.restoreinstead.🤖 Generated with Claude Code