Skip to content

fix: preserve full sync requests during active runs - #908

Merged
cpcloud merged 8 commits into
mainfrom
codex/forge-full-sync-stale
Aug 19, 2026
Merged

fix: preserve full sync requests during active runs#908
cpcloud merged 8 commits into
mainfrom
codex/forge-full-sync-stale

Conversation

@cpcloud

@cpcloud cpcloud commented Aug 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

What changed

  • Retain full, priority, and repository-scoped sync requests made while another provider sync is running.
  • Claim or update the single-flight slot before returning HTTP 202 while keeping provider execution asynchronous.
  • Merge repeated requests into one bounded follow-up and keep status running until all retained work finishes.
  • Preserve stable provider identity and limit scoped cadence and feature-cooldown bypasses to the requested repositories across GitHub, GitLab, Forgejo, and Gitea.

Failure scenario

  1. A sync starts and reads the provider's current data.
  2. The provider receives new data before that sync finishes.
  3. The user starts a full sync while the first sync still owns the work slot.
  4. The old path either discarded that request or returned 202 before recording it as pending work.
  5. The first sync could finish with its earlier snapshot and leave SQLite and the UI stale without a follow-up provider read.
  6. After the request was retained, each pass still briefly published running: false before starting its queued follow-up, so status and SSE clients could stop waiting before fresh data reached SQLite.

Admission now happens before the response, handoffs retain the slot continuously, and terminal status is published only when no pending pass remains. Terminal publication is ordered with slot release so a later run cannot be overwritten by a stale idle snapshot. A stopped syncer rejects new requests instead of returning 202.

cpcloud added 3 commits August 13, 2026 19:51
@cpcloud
fix: preserve sync requests during active runs
c5d31e6 
A user refresh accepted while another sync owned the single-flight slot returned immediately without scheduling follow-up work. If provider data changed after the active pass took its snapshot, the database stayed stale despite the full-sync response being accepted.

Coalesce accepted full, priority, and scoped intents into one lifecycle-owned follow-up pass while preserving the existing no-overlap and cadence rules. Full work subsumes scoped work, and repository selection follows stable provider identity so a rename or route reuse cannot redirect queued work.
@cpcloud
test: guard queued full sync through HTTP
b755eda 
The syncer regression covered provider and SQLite behavior directly, but it did not protect the user-facing 202 response path. A route regression could therefore accept a full sync during an active fetch while leaving the database stale.

Drive both overlapping requests through the generated HTTP client and real server, hold the first provider snapshot in flight, then require the queued pass to persist newer repository metadata. This keeps the cross-boundary concurrency contract executable at the boundary where users observe it.
@cpcloud
fix: make queued sync handoffs lossless
ea5cb71 
A full sync accepted behind a scoped run could still be lost when another run claimed the single-flight slot between completion and asynchronous replay. An explicitly empty scoped intent was also collapsed to nil while queued, widening it into a full sync.

Transfer slot ownership atomically until the accepted follow-up is registered, and preserve scope nilness while deduplicating repository intents. Later triggers still coalesce into one bounded pending intent, so request bursts do not turn into a click-count-sized queue.
@roborev-ci

roborev-ci Bot commented Aug 14, 2026

Copy link
Copy Markdown

roborev: Combined Review (ea5cb71)

Medium-severity issue found in sync-request coalescing; no security regressions identified.

Medium

  • internal/github/sync.go:6135 — Coalescing globally ORs bypassNextSyncAfter. If a cadence-respecting full run is pending behind a scoped run and a user-triggered scoped refresh arrives, the follow-up remains full but bypasses cadence for every configured repository—not just the requested scope.
    • Suggested fix: Track forced repositories separately from the full run’s cadence policy and apply bypass only to those repositories, or prevent a scoped refresh from promoting a cadence-gated full pass to a globally bypassing pass.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 6m29s

@cpcloud
fix: keep scoped sync bypasses repository-bound
3b11f8f 
A scoped user refresh coalesced into a cadence-respecting full pass by ORing its bypass flag into the whole pending run. The follow-up could then ignore cadence and feature cooldowns for unrelated repositories, causing provider work the user did not request.

Carry scoped bypasses by stable provider repository identity and evaluate them per repository while the full pass retains its own cadence policy. This preserves one bounded follow-up and immediate user refreshes without widening either bypass to unrelated repositories.
@roborev-ci

roborev-ci Bot commented Aug 14, 2026

Copy link
Copy Markdown

roborev: Combined Review (3b11f8f)

One medium-severity issue: sync status briefly reports idle before an accepted follow-up run begins.

Medium

  • internal/github/sync.go:6096, internal/github/sync.go:6133 — Each pass publishes Running: false before the deferred handoff checks pendingRun, even though the single-flight slot remains owned. Consequently, /sync/status and SSE can report completion between an active pass and its queued follow-up, causing clients to refresh stale data or associate the wrong last_run_at with their trigger.
    • Fix: Determine the handoff before publishing terminal status, keep the status running while follow-up work remains, register accepted triggers with the slot before returning 202, and add coverage ensuring status never becomes idle while work is queued.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 10m0s

@cpcloud
test: cover repository-bound scoped sync coalescing
1a908d6 
A scoped only_repo request can merge into queued cadence-respecting background work while another scoped refresh owns the sync slot. Without one full-stack guard, HTTP filter parsing, coalescing, and SQLite persistence could drift independently and widen the scoped bypass to unrelated repositories.\n\nExercise that mixed path through the generated client and real SQLite while keeping the provider boundary synthetic. The regression verifies that the selected repository advances to the queued snapshot and an unrelated repository remains untouched inside its cadence window.
@roborev-ci

roborev-ci Bot commented Aug 14, 2026

Copy link
Copy Markdown

roborev: Combined Review (1a908d6)

High-severity issues remain in sync handoff status and end-to-end identity coverage.

High

  • Premature completion during deferred handoff — internal/github/sync.go:5876, internal/github/sync.go:6133
    The current pass publishes Running: false and advances LastRunAt before launching an accepted pending pass. Pollers and SSE subscribers can therefore treat the queued request as complete and proceed using stale SQLite data. Keep public sync status running across the handoff, emit terminal status only after atomically confirming no pending run remains, and add an end-to-end assertion that completion is never reported between a blocked pass and its queued follow-up.

  • Stable identity across rename and route reuse lacks full-stack coverage — internal/github/sync_test.go:14640
    Current coverage calls helpers directly, while HTTP/SQLite tests use static routes. This could miss a regression that redirects a queued scoped refresh to a replacement repository. Add a server end-to-end test that queues an HTTP-scoped refresh, changes the tracked route while the active pass is blocked, and verifies that the renamed original identity is refreshed while a successor reusing the old route is not.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 9m17s

@cpcloud
fix: retain sync requests before accepting them
295b17b 
The HTTP sync route could return 202 while the trigger goroutine had not yet acquired the single-flight lock. A request accepted in that window was not yet part of active or pending work, making the API contract scheduler-dependent and leaving the strict boundary regression racy.

Admit or coalesce triggered work synchronously under the lifecycle and run locks, then start only newly claimed provider work asynchronously. Coalesced requests create no extra goroutine, stopped syncers are rejected, and cadence, provider dispatch, and SQLite behavior remain unchanged.
@cpcloud
cpcloud marked this pull request as draft August 14, 2026 14:39
@roborev-ci

This comment has been minimized.

Sign in to view
@cpcloud
fix: keep sync status running through queued work
93cdf42 
An accepted follow-up retained the single-flight slot, but each completed pass still published Running:false before its deferred handoff started the queued pass. Status pollers and SSE consumers could therefore treat the earlier provider snapshot as terminal and stop waiting before fresh data reached SQLite.

Hold each pass's terminal status snapshot until the handoff confirms that no pending work remains. Per-pass completion hooks and provider execution remain unchanged, while the HTTP/SQLite regression now rejects any idle-to-running transition inside one accepted sync chain.

Addresses roborev-ci finding: #908 (comment)
@roborev-ci

This comment has been minimized.

Sign in to view
@cpcloud
fix: prevent stale idle status from masking new syncs
e7b860d 
Keeping sync status running across queued passes moved terminal publication after the final run-slot release. A new trigger could claim that slot and publish Running:true before the completed pass published Running:false, leaving status pollers idle while provider work was active.

Order terminal publication with slot release under the existing status mutex, with statusMu preceding runMu whenever both are needed. The concurrency regression holds the real publication boundary, then verifies a real trigger remains coalesced until the terminal snapshot is ordered.
@roborev-ci

roborev-ci Bot commented Aug 14, 2026

Copy link
Copy Markdown

roborev: Combined Review (e7b860d)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 9m8s

@cpcloud
cpcloud marked this pull request as ready for review August 17, 2026 12:25
@cpcloud
cpcloud merged commit 336553e into main Aug 19, 2026
44 of 46 checks passed
@cpcloud
cpcloud deleted the codex/forge-full-sync-stale branch August 19, 2026 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mariusvniekerk mariusvniekerk mariusvniekerk approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cpcloud @mariusvniekerk