Skip to content

fix(sync): reconcile sync-failures.jsonl with reality#479

Closed
vinsew wants to merge 1 commit into
garrytan:masterfrom
vinsew:vinsew/fix-sync-failures-reconcile
Closed

fix(sync): reconcile sync-failures.jsonl with reality#479
vinsew wants to merge 1 commit into
garrytan:masterfrom
vinsew:vinsew/fix-sync-failures-reconcile

Conversation

@vinsew

@vinsew vinsew commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Bug 9 follow-up. v0.31 already shipped acknowledgeSyncFailures + the --skip-failed/--retry-failed operator workflows, but the JSONL was still append-only between cycles: a file that legitimately succeeded on a later attempt stayed listed as failed forever, padding doctor noise and confusing operators.

Tight on scope after rebase to v0.32.0 — this PR now only adds the missing reconciliation primitive and its call sites. The empty-diff --retry-failed / --skip-failed fallthrough is already on master from upstream's own work, so this branch no longer touches that surface.

Real-world trigger

A brain with CJK-named pages hit a transient parse failure on 2026-04-27, recorded 6 entries to ~/.gbrain/sync-failures.jsonl, then self-healed in the DB through a subsequent put_page flow — but the JSONL kept those 6 entries forever. Every gbrain doctor run flagged them as broken even though the underlying error (Invalid slug: "" from pre-#115 CJK slugify) no longer occurs.

Changes

  • src/core/sync.ts: new resolveSyncFailure(path: string): number. Removes all JSONL entries by path; unlinks the file when the last entry clears. Same read-modify-write concurrency caveat as acknowledgeSyncFailures (documented inline).

  • src/commands/import.ts: on successful import and on hash-identical skip, calls resolveSyncFailure(relativePath).

  • src/commands/sync.ts: same primitive fires from deletes (file gone), renames (both from + to), and the per-file import success / hash-identical skip paths inside importOnePath.

  • test/sync-failures.test.ts: 4 new cases — removal-with-count, no-op on unmatched path, last-entry file deletion, absent-file return-zero.

Test plan

  • bun test test/sync-failures.test.ts — 45/45 pass
  • bun run typecheck clean (after fresh bun install to drop stale lock entries)
  • Cherry-pick onto v0.32.0 (commit 0ad309e) — clean apply, no conflicts after the scope reduction

Note on prior scope

This PR was originally larger (8 tests, ~250 lines) and included the --retry-failed fallthrough at the lastCommit === headCommit early return. Upstream landed that exact handling independently before v0.32.0, so the rebased version drops the overlap and ships only the missing reconciliation hook.

🤖 Generated with Claude Code

@vinsew vinsew force-pushed the vinsew/fix-sync-failures-reconcile branch from 492c88d to d6485fc Compare May 3, 2026 14:00
@vinsew

vinsew commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto v0.26.0 (3c032d7).

Squashed in a fixup that aligns the two new acknowledgeSyncFailures() call sites with the AcknowledgeResult { count, summary } contract introduced in v0.22.9 (#501). Without this, this PR's call sites use the pre-v0.22.9 number shape and break typecheck on master.

Verified on production brain — schema v32, all sync paths working.

@vinsew vinsew force-pushed the vinsew/fix-sync-failures-reconcile branch from d6485fc to 436a8fc Compare May 3, 2026 14:09
@vinsew vinsew force-pushed the vinsew/fix-sync-failures-reconcile branch from 436a8fc to 0ad309e Compare May 11, 2026 08:37
Bug 9 follow-up. v0.31 already shipped acknowledgeSyncFailures + the
`--skip-failed`/`--retry-failed` operator workflows, but the JSONL was
still append-only between cycles: a file that legitimately succeeded on
a later attempt stayed listed as failed forever, padding doctor noise
and confusing operators.

resolveSyncFailure(path) drops every entry for that path (or removes
the file when the last entry is cleared), and now fires from:
- gbrain import — on successful import and on hash-identical skip
- gbrain sync — on delete (file gone), on rename (both from + to),
  on per-file import success, on per-file hash-identical skip

Same read-modify-write concurrency caveat as acknowledgeSyncFailures —
sync vs autopilot races are still possible, both writers are idempotent
on retry, and failures are rare enough that this isn't worth a lock.

4 new tests in sync-failures.test.ts cover removal-with-count, no-op on
unmatched path, last-entry file deletion, and absent-file case.
@vinsew vinsew force-pushed the vinsew/fix-sync-failures-reconcile branch from 0ad309e to f06ffcb Compare June 1, 2026 19:28
@garrytan

garrytan commented Jun 8, 2026

Copy link
Copy Markdown
Owner

Thanks for this contribution — and apologies for the slow triage. We did a full pass over the entire PR backlog. gbrain has moved fast, and the maintainer's larger "cathedral" rewrites have superseded a big share of community PRs: the AI gateway + recipes + user_provided_models system replaced almost all individual provider PRs; #1805 fixed the whole Postgres module-singleton class; #1542 unified the type taxonomy; #1657 the retrieval path; #1802 the doctor; and so on.

We're closing this one in that cleanup — either the fix already landed on master, it duplicates another PR or merged change, or it's outside the current merge bar. Where a closed PR carried a genuinely valuable idea, we've recorded it in docs/designs/COMMUNITY_IDEAS.md so nothing good is lost (a few may graduate into TODOs).

Please don't read the close as a judgment of the work — thank you for contributing. If you believe the underlying issue is still live on the latest master, reopen with a quick note and we'll take another look. 🙏

@garrytan garrytan closed this Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants