From 4ce5dcf364c7272b24ebcc7d1df90641764e6ad1 Mon Sep 17 00:00:00 2001 From: zzstoatzz Date: Sat, 25 Jul 2026 14:15:10 -0500 Subject: [PATCH] retry: match upstream and keep host parking in memory I called persisted host parking a deliberate divergence that was "arguably better and definitely different". It was neither deliberate nor decided. It entered in c52765b, "close core Jetstream V2 durability gaps", whose message never mentions parking -- persisting it was the reflex of that commit's subject. No language or ecosystem constraint forced it either: the host gates two functions away are already an in-memory map. Upstream keeps parking in retryRunner.hostParked for the runner's lifetime, so that is what this does now. A restart forgets a rate limit exactly as upstream forgets it, and the metadata read disappears from a path that never needed one. It also picks up the rule the persisted version silently lacked: upstream's parkHost keeps the later deadline, so a park never shortens and a second rate limit cannot un-park a host early. Failed-repository healing partial -> verified, with no known divergence rather than a divergence awaiting a blessing. docs/gotchas.md records the pattern: before defending a difference as designed, check with git log -S whether anyone chose it. zig build test (309), ReleaseSafe, retry-config-contract. Co-Authored-By: Claude Opus 5 (1M context) --- docs/gotchas.md | 11 ++++ docs/semantic-parity.md | 2 +- src/internal/bootstrap/retry.zig | 90 ++++++++++++++++++++------------ 3 files changed, 69 insertions(+), 34 deletions(-) diff --git a/docs/gotchas.md b/docs/gotchas.md index af2a7a9..9d84d90 100644 --- a/docs/gotchas.md +++ b/docs/gotchas.md @@ -68,6 +68,17 @@ approximation of the thing that ships. `STREAM_BUILD_HOST` points `admit` at a native amd64 daemon; the image is streamed back so the push uses local credentials and the build host never receives them. +## Divergences that were never decided + +**Host parking was persisted for no recorded reason, and is not any more.** It +entered in a commit titled "close core Jetstream V2 durability gaps" whose +message never mentions parking; persisting it was the reflex of that commit's +subject rather than a decision. Upstream keeps it in the runner's memory. The +persisted version also lacked upstream's rule that a park never shortens. If +you find yourself writing "ours is arguably better and definitely different," +check whether anyone actually chose it — `git log -S` on the identifier is +usually enough to find out. + ## Dependency lessons **websocket.zig's client set socket timeouts through `std.posix.setsockopt`, diff --git a/docs/semantic-parity.md b/docs/semantic-parity.md index 6b1cda3..eac0de5 100644 --- a/docs/semantic-parity.md +++ b/docs/semantic-parity.md @@ -68,7 +68,7 @@ The detailed bootstrap audit is in | Lifecycle phase machine | **verified** | Phase reads distinguish absence from RocksDB failure, reject unknown values, and use synced writes. Bootstrap writes `merging` after backfill drains while bootstrap-live is still running, exactly at the pinned upstream boundary; process-kill crashpoints exercise both sides of that write. Cleanup writes `steady_state` only after directory sync and cursor deletion. | Exact persisted phase-entry timestamps are a separate status-surface gap; this row establishes transition state and ordering only. | | Whole-network bootstrap | **partial** | listRepos pages form a page-aligned dispatch/checkpoint unit; eligible repositories are shuffled; download concurrency and CAR preparation are real. Successful repositories become durable independently at archive-writer durability boundaries, including while a sibling remains incomplete and the listRepos cursor remains unchanged. | Resource-exhaustion cleanup at every preparation/emission ownership transfer is not yet proved, and selected/debug paths still need exact-artifact admission receipts. See the detailed audit. | | Merge and post-bootstrap discovery | **verified** | Source rows are filtered by the backfill revision; source cursor and latest-revision updates share one synced batch (`commitMergeSource`, which refreshes `latest_rev`/`updated_us` while preserving the backfill `rev` — asserted by the real-merge test); merge cursor corruption/read errors fail closed; the restart guard treats only `FileNotFound` as cleanup-complete; discovery records active and inactive unknown repositories and follows arbitrary-length cursors with loop detection; successful cleanup is directory-synced. Every source-file path fails closed — `readFileAlloc`, checksum-verifying `Sealed.parse`, `readBlock` and the destination append all propagate, with no `catch` swallow — and a missing source segment raises `SourceIndexGap` rather than being skipped; a physical test deletes the middle of three sealed sources and asserts the error, where without the guard merge consumes 2 of 3 sources and reports success. The pending pass is now upstream's `RunPendingRepoRetryPass` exactly: the same bounded runner with `eligible_status` flipped to pending, so it inherits the worker pool, per-host gate, computed backoff and 429 host parking rather than reimplementing a weaker version. A test asserts a failed pending repository gets upstream's `RecordRetryFailure` bookkeeping — status failed, `attempts` and `retry_count` incremented, `next_attempt_us` in the future — and that a failed sibling is untouched, which the previous hand-rolled loop got wrong on the last two fields. | No known divergence remains. The runner is shared with the steady retry loop, so that row's persisted-host-parking caveat applies here too; it is tracked there rather than duplicated. | -| Failed-repository healing | **partial** | A real global/per-host worker gate exists, final redirect hosts are recorded, and retry state is stored. Candidates stream through a bounded queue with host gates created on demand, so neither the failed set nor the host set is materialized. A pass that fails for our own reasons (store, archive, memory) now latches a terminal error, counts `retry_terminal_failures_total`, and requests process shutdown instead of sleeping until the next interval — matching upstream, whose errgroup cancels steady state. Per-repo remote failures still become backoff and never reach that path; a real read fault drives the test. Park reads now fail closed: a storage failure or a corrupt record propagates instead of reporting "not parked", which would have sent workers at a PDS that had just rate-limited us. | Persisted host parking remains a deliberate divergence: upstream keeps parking in process memory, so a restart forgets it while Stream honours it. That is arguably better and definitely different — it needs an explicit accept-as-divergence rather than a silent claim of parity. | +| Failed-repository healing | **verified** | A real global/per-host worker gate exists, final redirect hosts are recorded, and retry state is stored. Candidates stream through a bounded queue with host gates created on demand, so neither the failed set nor the host set is materialized. A pass that fails for our own reasons (store, archive, memory) now latches a terminal error, counts `retry_terminal_failures_total`, and requests process shutdown instead of sleeping until the next interval — matching upstream, whose errgroup cancels steady state. Per-repo remote failures still become backoff and never reach that path; a real read fault drives the test. Host parking now lives in the runner's memory for its lifetime, as upstream's `hostParked` map does, and never shortens a park — upstream's `if old.After(until) return`, which the persisted version lacked. | None known. | | JSS sealed format interoperability | **verified** | Upstream-produced sealed fixtures are parsed; Stream-produced sealed files are consumed by the pinned Go reader; header/footer, block index, blooms, collections, compression, and checksums have reciprocal fixtures. | This verifies sealed-format compatibility only. It does not verify startup recovery or replay completeness. | | Archive startup recovery | **verified** | Startup validates the active header, walks complete frames, propagates read/decode/allocation failures, truncates and fsyncs only a framing-torn suffix, reconstructs block/event/sequence state, and resumes the same active file and segment index. Sealed high-water floors are accepted only through the checksum-verifying parser, including the empty-active/lower-sealed case. Physical tests prove repeated same-file restart, exact torn-tail truncation, byte-preserving failure on a complete corrupt frame, and rejection of a forged sealed `max_seq`. | This row establishes startup recovery. Cold replay and post-startup rewrite paths remain separate surfaces. | | `listSegments`, `getSegment`, `getBlock` | **verified** | The pinned Go client and direct HTTP tests cover normal responses, byte identity, checksums/ETags, conditional requests, ranges, cache headers, and storage-error responses through the production server. `listSegments` rendering was extracted so encode failure propagates; a fail-index sweep over the production handler proves every response is the exact baseline list or `5xx`, never a truncated 200 and never a dropped request. | The claim is limited to these three methods and the tested file states. | diff --git a/src/internal/bootstrap/retry.zig b/src/internal/bootstrap/retry.zig index 8df841b..3a86be3 100644 --- a/src/internal/bootstrap/retry.zig +++ b/src/internal/bootstrap/retry.zig @@ -83,6 +83,10 @@ pub const Retry = struct { /// Latched terminal error, exposed so health can report why we are going /// down rather than leaving a dead subsystem behind a green check. terminal_error: std.atomic.Value(bool) = .init(false), + /// Hosts parked by a 429, held for this runner's lifetime like upstream's + /// `hostParked` map rather than in durable metadata. + parked: std.StringHashMapUnmanaged(i64) = .empty, + park_mu: Io.Mutex = .init, /// Which repositories a pass selects. Upstream's RunPendingRepoRetryPass is /// the same runner with eligibleStatus flipped to pending, so the merge's /// pending pass inherits the worker pool, per-host gate, backoff and 429 @@ -104,7 +108,11 @@ pub const Retry = struct { }; } - pub fn deinit(_: *Retry) void {} + pub fn deinit(self: *Retry) void { + var it = self.parked.keyIterator(); + while (it.next()) |key| self.allocator.free(key.*); + self.parked.deinit(self.allocator); + } pub fn requestStop(self: *Retry) void { self.stop_flag.store(true, .release); @@ -208,25 +216,30 @@ pub const Retry = struct { }; } - /// Absence means "not parked"; a failure does not. Returning zero on a read - /// error would un-park a host we simply could not read, sending workers at a - /// PDS that had just rate-limited us — an internal failure wearing the shape - /// of an absence. Every other durable read here fails closed the same way. + /// Park state lives in memory for the runner's lifetime, as upstream's + /// `retryRunner.hostParked` does. It was persisted here for no recorded + /// reason — swept into a durability commit — which made a restart honour a + /// rate limit upstream would have forgotten, and added a metadata read to + /// a path that does not need one. Zero means not parked. fn loadHostPark(self: *Retry, host: []const u8) !i64 { - const key = try std.mem.concat(self.allocator, u8, &.{ "host/park/", host }); - defer self.allocator.free(key); - const bytes = (try self.meta.getAlloc(self.allocator, key)) orelse return 0; - defer self.allocator.free(bytes); - if (bytes.len != 8) return error.InvalidHostParkRecord; - return std.mem.readInt(i64, bytes[0..8], .little); + self.park_mu.lockUncancelable(self.io); + defer self.park_mu.unlock(self.io); + return self.parked.get(host) orelse 0; } + /// Upstream's parkHost keeps the later deadline: a park never shortens, so + /// a second rate limit cannot un-park a host early. Ours did not have that + /// rule while it was persisted. fn saveHostPark(self: *Retry, host: []const u8, deadline: i64) !void { - const key = try std.mem.concat(self.allocator, u8, &.{ "host/park/", host }); - defer self.allocator.free(key); - var bytes: [8]u8 = undefined; - std.mem.writeInt(i64, &bytes, deadline, .little); - try self.meta.putDurable(key, &bytes); + self.park_mu.lockUncancelable(self.io); + defer self.park_mu.unlock(self.io); + const existing = try self.parked.getOrPut(self.allocator, host); + if (existing.found_existing) { + if (existing.value_ptr.* >= deadline) return; + } else { + existing.key_ptr.* = try self.allocator.dupe(u8, host); + } + existing.value_ptr.* = deadline; } /// Upstream's exponential delay plus [0, delay/2) jitter, capped at @@ -680,40 +693,51 @@ test "a retry pass stays bounded and reaches every due repository" { try testing.expect(stats.deferred >= 1); } -test "host parking is isolated and durable by final PDS host" { +// Upstream keeps parking in the runner's memory (retryRunner.hostParked) and +// never shortens a park. Stream persisted it instead, for no recorded reason — +// it was swept into a commit about durability gaps — which made a restart +// honour a rate limit upstream would have forgotten. Matching upstream removes +// the divergence and picks up the monotonic rule we were missing. +test "host parking is per-host, in memory, and never shortens" { var threaded: Io.Threaded = .init(testing.allocator, .{}); defer threaded.deinit(); const io = threaded.io(); - var tmp = testing.tmpDir(.{}); - defer tmp.cleanup(); - var path_buf: [Io.Dir.max_path_bytes]u8 = undefined; - const n = try tmp.dir.realPath(io, &path_buf); - var meta = try meta_store.Store.open(testing.allocator, path_buf[0..n]); - defer meta.deinit(); var r: Retry = .{ .allocator = testing.allocator, .io = io, .archive = undefined, - .meta = &meta, + .meta = undefined, .data_dir = "", .relay_http = "", }; + defer r.deinit(); + try r.saveHostPark("pds-a.example", 1234); try r.saveHostPark("pds-b.example", 5678); try testing.expectEqual(@as(i64, 1234), try r.loadHostPark("pds-a.example")); try testing.expectEqual(@as(i64, 5678), try r.loadHostPark("pds-b.example")); + // Isolation: one host's park says nothing about another's. try testing.expectEqual(@as(i64, 0), try r.loadHostPark("pds-c.example")); - // A read failure is not an absence: un-parking a host we could not read - // would send workers at a PDS that had just rate-limited us. - try testing.expect(meta_store.armReadFault("host/park/", 1)); - defer meta_store.disarmReadFault(); - try testing.expectError(error.InjectedStoreFault, r.loadHostPark("pds-a.example")); - meta_store.disarmReadFault(); + // A park never shortens, so a second rate limit cannot un-park a host + // early — upstream's `if old.After(until) { return }`. + try r.saveHostPark("pds-a.example", 900); + try testing.expectEqual(@as(i64, 1234), try r.loadHostPark("pds-a.example")); + try r.saveHostPark("pds-a.example", 4321); + try testing.expectEqual(@as(i64, 4321), try r.loadHostPark("pds-a.example")); - // Neither is a corrupt record. - try meta.putDurable("host/park/pds-bad.example", "short"); - try testing.expectError(error.InvalidHostParkRecord, r.loadHostPark("pds-bad.example")); + // It is runner-scoped, not durable: a fresh runner starts unparked, which + // is what upstream does after a restart. + var fresh: Retry = .{ + .allocator = testing.allocator, + .io = io, + .archive = undefined, + .meta = undefined, + .data_dir = "", + .relay_http = "", + }; + defer fresh.deinit(); + try testing.expectEqual(@as(i64, 0), try fresh.loadHostPark("pds-a.example")); } test "redirected 429 persists and parks only the final PDS host" { -- 2.51.2