Claude: changes for issue #85 #86
No reviewers
Labels
No labels
breaking-change
bug
documentation
duplicate
enhancement
good first issue
help wanted
performance
question
wontfix
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Grandiras/ATProto.NET!86
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "claude/issue-85"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #85.
🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the task runs.
🛑 Verdict: request changes — blocking issues below.
This is a large, well-documented addition of Jetstream v2 archive replay/snapshot support (plan/download/decode/filter, then cutover to the live tail). The
.jsssegment decoder, retry/resume logic inJetstreamArchiveClient, and the docs/sample/tests are thorough and internally consistent. One correctness concern in the backfill loop undermines the PR's central "no gap" guarantee; everything else looks solid (byte-range resume handles the ignored-Range case, 401 vs 429 retry semantics are correct, cursor persistence on every exit path viafinally, collection-filter-only-applies-to-commits semantics match the live tail).⏱ 2m 52s · 8 turns · tokens: 90.7k in / 17.1k out (+760.5k cache-read)
📜 Full session transcript
@ -0,0 +283,4 @@// The server truncates a page at a whole work-unit boundary and always admits at least// one, so plannedThroughSeq advances every page. Stopping when it does not is what// keeps a server that disagrees from looping forever over the same page.if (plan.PlannedThroughSeq <= planned)BackfillAsyncsilently truncates the backfill when a plan page fails to advance past the previously planned sequence (plan.PlannedThroughSeq <= planned) — it just logs a warning andbreaks, even though the loop is still below the pinned ceilingtip/PinnedTipSeq. Back inReplayAsync, the cutover then connects the live socket attip = PinnedTipSeq ?? …(line ~168), which is inclusive and drops everything at or belowLastCursor. Any sequence range between the actual last delivered event and the pinned ceiling that was skipped by this early break is therefore never delivered by either phase — a permanent, silent gap, with nothing surfaced to the caller besides a log line (no exception, no flag on the consumer).This directly contradicts the feature's advertised guarantee (CHANGELOG: "the cutover is inclusive and deduplicated... no gap"; docs: "nothing is lost at the handoff") and the code's own comment two lines above claiming paging "always progresses." The scenario isn't purely hypothetical: if
plannedThroughSeqreflects what the server has actually sealed/served rather than a hard filter-derived count, a lag between whensealedTipSeqwas reported and when the corresponding segments become fetchable would produce exactly this non-advancing response — a plausible race, not just server misbehavior.Consider instead treating a non-advancing plan below the ceiling as a retryable condition (bounded retries/backoff) and only failing loudly (throwing, e.g. a
JetstreamArchiveException) if it truly cannot make progress, rather than silently accepting data loss and proceeding to cut over.@claude-bot
🤖 On it — follow along in the action run; the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the task runs.
Fixed the blocking review finding and pushed to
claude/issue-85(commit7835730).The gap
BackfillAsynctreated a plan page whoseplannedThroughSeqdidn't advance as "the archive is exhausted" — warn andbreak— even with the pinned ceiling still ahead.ReplayAsyncthen connected the live socket atPinnedTipSeq, which is inclusive, so the range between the last delivered event and that ceiling was delivered by neither phase. The reviewer's read is right, and the lag case (a sealed tip reported before its segments are servable) makes it a plausible race rather than only server misbehaviour.A non-advancing page below the ceiling is now a stall, not a finish:
MaxRetryDelay), up to the newJetstreamArchiveOptions.MaxStalledPlanAttempts(default 5);JetstreamArchiveException, naming the sequence it got through and the tip it couldn't reach, so the caller can resume from it instead of cutting over across a hole;Adjacent bug found while there
BeforeSeqwas only sent on the first plan request — from the second page on,beforeSeq = tip ?? _archive.BeforeSeqresolved to the pinnedsealedTipSeq, and the row filter used that same value as its ceiling. A bounded snapshot therefore ran past the caller's bound all the way to the sealed tip. The ceiling is nowmin(BeforeSeq, sealedTipSeq). This also makes the "one segment's worth of history, so the test spends a bounded number of bytes" integration test actually bounded.Docs & tests
CHANGELOG.md(amended in place — the feature is still unreleased) and the How it works section ofdocs/jetstream.mdnow state the retry-then-fail behaviour and theBeforeSeqcap, replacing the "paging always progresses" claim the old code leaned on. TheAPlanThatDoesNotAdvanceEndsTheBackfillInsteadOfLoopingtest asserted the buggy behaviour, so it's replaced by four: stall-then-fail (asserting bounded re-plans and that no segment is re-downloaded), stall-then-recover, resume-at-the-sealed-tip, and theBeforeSeqceiling on later pages.Verified: installed the .NET 10 SDK in the container (none was present);
dotnet buildclean across the solution with 0 warnings,dotnet test tests/ATProtoNet.Tests/→ 1291 passed, 0 failed, 5 skipped (the Aspire manifest skips are pre-existing), and the integration test project compiles. Integration tests themselves weren't run — they need a live PDS/archive key.⏱ 6m 28s · 39 turns · tokens: 71.0k in / 21.7k out (+2512.7k cache-read)
📜 Full session transcript
@claude-reviewer
🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the task runs.
✅ Verdict: approve — no blocking issues found.
Re-review: the blocking issue from my previous pass is fixed.
BackfillAsyncinsrc/ATProtoNet/Streaming/JetstreamReplayConsumer.csno longer silentlybreaks out of the backfill loop when a plan page fails to advance below the pinned ceiling. It now retries with exponential backoff (StallDelay, capped byMaxRetryDelay) up toJetstreamArchiveOptions.MaxStalledPlanAttempts(default 5), and if the plan genuinely cannot make progress it throws aJetstreamArchiveExceptioninstead of proceeding to cut over across the gap. This matches the documented guarantee (CHANGELOG, docs/jetstream.md) that a stalled plan either catches up or fails loudly rather than silently dropping a range that the inclusive cutover would never redeliver. The boundary conditions (planned < ceiling,plannedThroughSeq <= planned) look correct: a plan that starts already at or above the ceiling skips the stall check and proceeds straight to the terminal break, and a genuinely stalled plan resetsstallsback to zero as soon as it advances.I didn't spot other correctness issues in the areas I re-checked (cutover retry/re-plan flow, cursor tracking/dedup in
TrackAsync, the bounded-channel download pipeline inDownloadAsync, and the.jsssegment/block decoding math). The rest of the PR (docs, sample, archive client retry/resume semantics, segment reader) was already reviewed favorably last pass and is unchanged in substance. Good to merge.⏱ 0m 48s · 7 turns · tokens: 78.6k in / 3.9k out (+627.0k cache-read)
📜 Full session transcript