fix(synccompactor): use ResumeSync when reopening the merged sync

[manojacs]

Follow-up to #1013, blocker #4 from the approval review.

The problem

After the merge, incremental grant expansion reopens the compacted sync so it can write derived grants. It did that with StartOrResumeSync:

// pkg/dotc1z/engine/pebble/adapter.go:175
if syncID != "" {
    if _, err := e.GetSyncRunRecord(ctx, syncID); err == nil {
        return e.ResumeSync(...)      // happy path
    }
}
// any lookup error at all falls through to:
e.StartNewSync(ctx, syncType, "")

StartNewSync calls ResetForNewSync, which excises the key range from typeResourceType to typeEngineMeta — everything the merge just wrote.

So a transient failure looking up a sync id that demonstrably exists (the merge produced it moments earlier) discards the compacted output and returns an empty artifact, with the compaction reporting success.

The fix

Call ResumeSync directly. It is already on connectorstore.Writer, and it has no "start a new one" branch: a lookup error returns an error and leaves the destination untouched.

That keeps the existing error classification correct. The failure stays a plain error rather than errIncrementalFatal, and ResumeSync returns before binding anything, so the store is still in the ended state the full-expansion fallback expects — no restoreEndedSync needed.

Tests

New pkg/dotc1z/engine/pebble/resume_sync_test.go:

• TestResumeSyncUnknownIDFailsClosed — unresolvable id errors, records survive.
• TestResumeSyncOnEndedSyncAllowsWrites — resuming an ended sync succeeds and accepts writes. This is the contract the compactor actually depends on (the merge leaves the sync ended), and nothing pinned it before.
• TestStartOrResumeSyncUnknownIDWipesRecords — pins the destructive fallback so the reason for this change stays visible.

Note for a separate issue

That last test documents current behavior, not desired behavior. SQLite's C1File.StartOrResumeSync (pkg/dotc1z/sync_runs.go:704) returns NotFound rather than starting a new sync when an explicit id fails to resolve, so the engines diverge — despite adapter.go's comment claiming the Pebble path mirrors the SQLite cascade. Out of scope here, since incremental expansion is Pebble-only, but worth aligning. After this change no production caller passes a non-empty id to StartOrResumeSync at all.

🤖 Generated with Claude Code

[github-actions]

🟡 Suggestion (medium confidence): This test pins a real data-loss hazard that the PR fixes at one call site rather than at the source. Engine.StartOrResumeSync (pkg/dotc1z/engine/pebble/adapter.go:176-180) still treats any GetSyncRunRecord error on an explicit non-empty id — including a transient read failure — as "nothing to resume" and falls through to StartNewSync, whose ResetForNewSync wipes the sync-scoped keyspace. SQLite's C1File.StartOrResumeSync (pkg/dotc1z/sync_runs.go:704) fails closed with NotFound in the same situation. Since no production caller passes a non-empty id to StartOrResumeSync today (only pkg/sync/syncer.go:839, with ""), aligning Pebble to fail closed would be behavior-preserving and would remove the class of bug instead of one instance. The PR body already flags this as out of scope — noting it so the follow-up is tracked.

[github-actions]

General PR Review: fix(synccompactor): use ResumeSync when reopening the merged sync

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 8bd119175bf3.
Review mode: full
View review run

Review Summary

The full PR diff (one call-site change in pkg/synccompactor/compactor.go plus a new engine-level test file) was scanned for security and correctness. Risk triage per docs/BUG_CATCHING.md §2 puts the pre-fix failure mode at HIGH — silent (compaction reported success while emitting an empty artifact) and durable (c1z contents consumed by the platform), remediation rung 2 (re-sync) — and this PR narrows exactly that class: ResumeSync has no StartNewSync/ResetForNewSync fallback, so a lookup failure now errors and leaves the merged records intact. The change is verified correct against the engine: ResumeSync returns before bindCurrentSync on every error path (so the store stays in the ended state the full-expansion fallback expects, and restoreEndedSync is genuinely unneeded), bindCurrentSync unseals (pkg/dotc1z/engine/pebble/engine.go:382) so the resumed ended sync accepts grant writes, incremental expansion is Pebble-gated (compactor.go:1241) so SQLite's stricter "cannot resume an ended sync" rule is not reachable, and the happy path is byte-for-byte the same as before since the old StartOrResumeSync delegated to ResumeSync on a successful lookup. No exported API, proto, serialized-state, default-behavior, or dependency changes; no go.mod/go.sum diff. No blocking issues found.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• pkg/dotc1z/engine/pebble/resume_sync_test.go:86 — the destructive StartOrResumeSync fallback the new test documents is still live in adapter.go:176-180; consider failing closed on an explicit non-empty id to match SQLite (pkg/dotc1z/sync_runs.go:704) instead of fixing one call site. Acknowledged as out of scope in the PR body.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/dotc1z/engine/pebble/adapter.go`:
- Around line 175-189: `Engine.StartOrResumeSync` treats any `GetSyncRunRecord` error on a
  caller-supplied non-empty syncID as "nothing to resume" and falls through to
  `StartNewSync`, whose `ResetForNewSync` wipes the sync-scoped keyspace. A transient read
  failure on an id that exists therefore silently destroys the store's records. SQLite's
  `C1File.StartOrResumeSync` (pkg/dotc1z/sync_runs.go:704) instead returns NotFound in this
  case, so the two engines diverge despite adapter.go's comment claiming the Pebble path
  mirrors the SQLite cascade. Change the non-empty-syncID branch to fail closed: propagate
  the lookup error (adapted to NotFound where appropriate) rather than falling through to
  `StartNewSync`. No production caller passes a non-empty id today (only
  pkg/sync/syncer.go:839, which passes ""), so this is behavior-preserving for current
  callers. If you make this change, invert the assertions in
  `TestStartOrResumeSyncUnknownIDWipesRecords`
  (pkg/dotc1z/engine/pebble/resume_sync_test.go:86) as that test's own comment instructs,
  and update the adapter.go doc comment.

[github-actions]

No blocking issues found.