SpaceSyncer reports `Partial` forever for a repo the account never wrote to #99

New issue

Closed

opened 2026-08-20 21:35:51 +00:00 by claude-bot · 2 comments

claude-bot commented

2026-08-20 21:35:51 +00:00

Collaborator

SpaceSyncer.SyncRepoAsync (src/ATProtoNet/Spaces/SpaceSyncer.cs) returns SpaceSyncOutcome.Partial whenever the oplog page carries no commit:

// A commit arrives only at the head of the log; short of that there is nothing to
// compare against and the caller simply syncs again.
if (page.Commit is null)
    return new SpaceSyncResult(SpaceSyncOutcome.Partial, cursor.Rev, null, applied, null);

That reasoning holds for a truncated page, but there is a second way to get no commit. A member who has never written to the space has no repo state, and the host builds the commit from that state — buildSignedCommit in packages/pds/src/api/com/atproto/space/util.ts returns undefined when there is none, so listRepoOps answers 200 {ops: [], commit: undefined} rather than RepoNotFound. Verified against a live host on the permissioned-data branch.

So the pass applies zero operations, makes zero progress, and reports Partial — which SpaceSyncOutcome.Partial documents as "the caller should sync again to continue". A caller that loops on Partial (the documented contract) spins forever on that repo. RecoverAsync gets it right: getRepo does answer RepoNotFound, and the syncer reports NoRepo and drops what it held.

In practice a syncer driven from ListReposAsync never sees this, since the writer set only contains accounts that have written — but a syncer walking a member list, or one whose writer-set entry is stale, does.

A fix has a design choice worth making deliberately: report NoRepo when a page comes back with no ops, no commit, and a cursor that has never advanced; or add an outcome that distinguishes "nothing to do" from "call me again". tests/ATProtoNet.IntegrationTests/SpaceRepoSyncTests.cs currently asserts what such a pass does (no ops applied, no commit, store untouched) rather than the outcome enum, so it will not have to change either way — but it should assert the outcome once one is settled on.

Filed by Claude while working on #93 (run).

`SpaceSyncer.SyncRepoAsync` (`src/ATProtoNet/Spaces/SpaceSyncer.cs`) returns `SpaceSyncOutcome.Partial` whenever the oplog page carries no commit: ```csharp // A commit arrives only at the head of the log; short of that there is nothing to // compare against and the caller simply syncs again. if (page.Commit is null) return new SpaceSyncResult(SpaceSyncOutcome.Partial, cursor.Rev, null, applied, null); ``` That reasoning holds for a truncated page, but there is a second way to get no commit. A member who has never written to the space has no repo state, and the host builds the commit from that state — `buildSignedCommit` in `packages/pds/src/api/com/atproto/space/util.ts` returns `undefined` when there is none, so `listRepoOps` answers `200 {ops: [], commit: undefined}` rather than `RepoNotFound`. Verified against a live host on the `permissioned-data` branch. So the pass applies zero operations, makes zero progress, and reports `Partial` — which `SpaceSyncOutcome.Partial` documents as "the caller should sync again to continue". A caller that loops on `Partial` (the documented contract) spins forever on that repo. `RecoverAsync` gets it right: `getRepo` does answer `RepoNotFound`, and the syncer reports `NoRepo` and drops what it held. In practice a syncer driven from `ListReposAsync` never sees this, since the writer set only contains accounts that have written — but a syncer walking a member list, or one whose writer-set entry is stale, does. A fix has a design choice worth making deliberately: report `NoRepo` when a page comes back with no ops, no commit, and a cursor that has never advanced; or add an outcome that distinguishes "nothing to do" from "call me again". `tests/ATProtoNet.IntegrationTests/SpaceRepoSyncTests.cs` currently asserts what such a pass does (no ops applied, no commit, store untouched) rather than the outcome enum, so it will not have to change either way — but it should assert the outcome once one is settled on. --- _Filed by Claude while working on #93 ([run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/319))._

claude-bot referenced this issue

2026-08-20 21:35:51 +00:00

Spaces: integration tests against a permissioned-data PDS #93

claude-bot was assigned by Grandiras

2026-08-20 21:45:12 +00:00

claude-bot commented

2026-08-20 21:45:16 +00:00

Author

Collaborator

🤖 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.

🤖 On it — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/329); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-99/run-329.md) — updating every ~5 minutes while the task runs.

claude-bot referenced this issue from a commit

2026-08-20 21:50:40 +00:00

fix: report NoRepo instead of Partial for a repo with no state

claude-bot referenced this issue from a pull request that will close it,

2026-08-20 21:51:18 +00:00

Claude: changes for issue #99 #100

claude-bot commented

2026-08-20 21:51:19 +00:00

Author

Collaborator

Fixed on branch claude/issue-99 (commit c2a07cf).

The design choice

I took the first of the two options in the issue — report NoRepo — rather than adding an outcome. SpaceSyncOutcome.NoRepo already documents exactly this situation ("the account holds no repo in this space, so there is nothing to sync"), it is the answer RecoverAsync already gives for the same account, and a new enum member would change the meaning of exhaustive switches that consumers have already written. Nothing about "nothing to do" needed a name that NoRepo did not already have.

The framing I settled on is that Partial is only correct when the pass has somewhere left to go. That is true in exactly two cases: it applied operations (so the next call starts from a later since), or the host offered a continuation cursor (so there is a next page). A page carrying neither has reached the end of what the host will serve, and no amount of calling again changes that.

if (page.Commit is null)
{
    return applied.Count == 0 && page.Cursor is null
        ? await NothingToCommitAsync(client, cursor, cancellationToken)
        : new SpaceSyncResult(SpaceSyncOutcome.Partial, cursor.Rev, null, applied, null);
}

Gating on the continuation cursor rather than on page.Ops.Count alone matters: the cursor is the protocol's own "more to come" signal (ListSpaceRepoOpsResponse.Cursor is documented as "absent once the response reaches the head of the oplog"), so an empty page that still carries one keeps reporting Partial, which is what it means.

The second case, which the issue does not mention

NothingToCommitAsync splits on whether the cursor has ever advanced:

cursor.Rev is null — the member has written nothing and we hold nothing. Report NoRepo directly, no full download, store untouched. This is the case the issue describes.
cursor.Rev is set — the host is reporting no state for a repo we hold a copy of. That is not "nothing to sync", it is a copy that has outlived its source, and reporting Partial spins here too. It takes the existing repair path: RecoverAsync is the one call that answers definitively, and it repairs either way — RepoNotFound drops the stale copy and reports NoRepo, and a repo that does exist gets rebuilt. Both terminate, and both leave the local copy correct.

I kept the store untouched on the first branch deliberately, matching what the existing IsMissingRepo catch in SyncRepoAsync does — a cursor at rev null holds nothing to drop.

Verification

dotnet build clean (0 warnings, 0 errors), dotnet test tests/ATProtoNet.Tests/ 1534 passed, 0 failed, 5 skipped (the usual Aspire-manifest gate). Three new unit tests, all against the existing StubHost:

SyncRepoAsync_ForAnAccountThatHasWrittenNothing_ReportsNoRepoRatherThanPartial — {"ops":[]}, fresh cursor → NoRepo, no ops, no commit, cursor.Rev still null, store not applied to / replaced / dropped.
SyncRepoAsync_WhenAHeldRepoHasNothingToCommit_RecoversRatherThanAssumingItIsGone — same empty page against a cursor at rev 3l6ov1 with a non-empty set hash, getRepo answering RepoNotFound → NoRepo, copy dropped, cursor reset.
SyncRepoAsync_WithoutACommitButWithAContinuation_ReportsPartialEvenWithNoOps — the guard against overreach: zero ops but a cursor is still Partial.

The existing SyncRepoAsync_WithoutACommit_ReportsPartialSoTheCallerContinues and SyncRepoAsync_SendsTheCursorsRevAsSince both still pass unchanged; the latter was already the "empty page with a cursor" shape.

I could not run the integration test — it needs a host serving com.atproto.space.*, which no PDS release does. As the issue asked, SpaceRepoSyncTests now asserts the outcome: SyncRepoAsync_ForAnAccountThatHasWrittenNothing_AppliesNothing is renamed to …_ReportsNoRepo and gained Assert.Equal(SpaceSyncOutcome.NoRepo, result.Outcome) alongside the assertions it already made about ops, commit, rev, and the store.

Also updated: the XML docs on Partial and NoRepo (both now state which shapes produce them), the sync section of docs/spaces.md, and CHANGELOG.md under [Unreleased] → Fixed.

No new issues filed — nothing turned up outside the scope of this one.

⏱ 5m 38s · 34 turns · tokens: 85.3k in / 18.0k out (+2257.4k cache-read)
📜 Full session transcript
→ Opened PR: #100

Fixed on branch `claude/issue-99` (commit `c2a07cf`). ## The design choice I took the first of the two options in the issue — report `NoRepo` — rather than adding an outcome. `SpaceSyncOutcome.NoRepo` already documents exactly this situation ("the account holds no repo in this space, so there is nothing to sync"), it is the answer `RecoverAsync` already gives for the same account, and a new enum member would change the meaning of exhaustive switches that consumers have already written. Nothing about "nothing to do" needed a name that `NoRepo` did not already have. The framing I settled on is that **`Partial` is only correct when the pass has somewhere left to go**. That is true in exactly two cases: it applied operations (so the next call starts from a later `since`), or the host offered a continuation cursor (so there is a next page). A page carrying neither has reached the end of what the host will serve, and no amount of calling again changes that. ```csharp if (page.Commit is null) { return applied.Count == 0 && page.Cursor is null ? await NothingToCommitAsync(client, cursor, cancellationToken) : new SpaceSyncResult(SpaceSyncOutcome.Partial, cursor.Rev, null, applied, null); } ``` Gating on the continuation cursor rather than on `page.Ops.Count` alone matters: the cursor is the protocol's own "more to come" signal (`ListSpaceRepoOpsResponse.Cursor` is documented as "absent once the response reaches the head of the oplog"), so an empty page that still carries one keeps reporting `Partial`, which is what it means. ## The second case, which the issue does not mention `NothingToCommitAsync` splits on whether the cursor has ever advanced: - **`cursor.Rev is null`** — the member has written nothing and we hold nothing. Report `NoRepo` directly, no full download, store untouched. This is the case the issue describes. - **`cursor.Rev` is set** — the host is reporting no state for a repo we *hold a copy of*. That is not "nothing to sync", it is a copy that has outlived its source, and reporting `Partial` spins here too. It takes the existing repair path: `RecoverAsync` is the one call that answers definitively, and it repairs either way — `RepoNotFound` drops the stale copy and reports `NoRepo`, and a repo that does exist gets rebuilt. Both terminate, and both leave the local copy correct. I kept the store untouched on the first branch deliberately, matching what the existing `IsMissingRepo` catch in `SyncRepoAsync` does — a cursor at rev `null` holds nothing to drop. ## Verification `dotnet build` clean (0 warnings, 0 errors), `dotnet test tests/ATProtoNet.Tests/` **1534 passed, 0 failed, 5 skipped** (the usual Aspire-manifest gate). Three new unit tests, all against the existing `StubHost`: - `SyncRepoAsync_ForAnAccountThatHasWrittenNothing_ReportsNoRepoRatherThanPartial` — `{"ops":[]}`, fresh cursor → `NoRepo`, no ops, no commit, `cursor.Rev` still null, store not applied to / replaced / dropped. - `SyncRepoAsync_WhenAHeldRepoHasNothingToCommit_RecoversRatherThanAssumingItIsGone` — same empty page against a cursor at rev `3l6ov1` with a non-empty set hash, `getRepo` answering `RepoNotFound` → `NoRepo`, copy dropped, cursor reset. - `SyncRepoAsync_WithoutACommitButWithAContinuation_ReportsPartialEvenWithNoOps` — the guard against overreach: zero ops but a cursor is still `Partial`. The existing `SyncRepoAsync_WithoutACommit_ReportsPartialSoTheCallerContinues` and `SyncRepoAsync_SendsTheCursorsRevAsSince` both still pass unchanged; the latter was already the "empty page *with* a cursor" shape. I could not run the integration test — it needs a host serving `com.atproto.space.*`, which no PDS release does. As the issue asked, `SpaceRepoSyncTests` now asserts the outcome: `SyncRepoAsync_ForAnAccountThatHasWrittenNothing_AppliesNothing` is renamed to `…_ReportsNoRepo` and gained `Assert.Equal(SpaceSyncOutcome.NoRepo, result.Outcome)` alongside the assertions it already made about ops, commit, rev, and the store. Also updated: the XML docs on `Partial` and `NoRepo` (both now state which shapes produce them), the sync section of `docs/spaces.md`, and `CHANGELOG.md` under `[Unreleased] → Fixed`. No new issues filed — nothing turned up outside the scope of this one. --- ⏱ 5m 38s · 34 turns · tokens: 85.3k in / 18.0k out (+2257.4k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-99/run-329.md) → Opened PR: https://git.grandiras.net/Grandiras/ATProto.NET/pulls/100