Skip to content

fix(security-sync): resume owner progress on lease takeover - #7047

Merged
eshurakov merged 1 commit into
mainfrom
eshurakov/zealous-orchard
Oct 1, 2026
Merged

eshurakov merged 1 commit into
mainfrom
eshurakov/zealous-orchard

Conversation

@eshurakov

Copy link
Copy Markdown
Contributor

Summary

Some large owners have stopped completing a Security Agent sync. Each run covers the same first ~10 repositories and never reaches the rest, so last_synced_at never advances. After #7023, an owner can also be locked out for up to 7 days.

Two causes in claimOwnerSyncLease:

  • Takeover discards progress. When a different run claims the owner lease, it overwrote sync_run with an empty skeleton. Whenever another run's message slipped in between a run's chunks, the run started over at repo 0. Slow repositories in the first part used up the 8-minute budget every time.
  • 7-day lease TTL. If a chain died (its continuation was lost or delayed), every other run for that owner was denied with held_by_other until the lease expired a week later.

Fix:

  • Adopt the previous checkpoint on takeover. The run that takes over keeps the earlier sync_run (completed repos and counters) under its own run ID. It drops the old chunkIndex, which would otherwise reject its writes, and resets noProgressChunks. Taking over now continues the remaining work instead of restarting, so completion always moves forward whichever chain runs.
  • Lease TTL: 7 days → 20 minutes. This is still longer than the 15-minute queue consumer limit, so a live chunk is never taken over. A late continuation of the same run can still reclaim its own expired lease.

Verification

  • No manual test against a deployed worker yet. Production acceptance comes after deploy: the affected owners log claim acquired with resumed: true, then Security sync completed for owner, and last_synced_at advances.

Automated (local Postgres): security-sync unit tests 93/93, integration tests 11/11. The two new integration cases fail with the fix reverted:

  • an expired takeover adopts the previous progress, and the new run's chunk 0 can write and complete
  • end-to-end through real syncOwner: a run that died at chunk 3 having synced a, b; the next run fetches only c and the owner completes

Visual Changes

N/A

Reviewer Notes

  • Post-deploy step. Changing the constant does not shorten 7-day leases that are already stored. After deploy, clamp them. This keeps sync_run, so stuck owners pick up where they left off, and gives any chunk that is running a 20-minute grace period:
    UPDATE agent_configs
    SET runtime_state = jsonb_set(runtime_state, '{sync_lease,expiresAt}', to_jsonb(now() + interval '20 minutes'))
    WHERE agent_type = 'security_scan' AND platform = 'github'
      AND (runtime_state->'sync_lease'->>'expiresAt')::timestamptz > now() + interval '20 minutes';
  • Tradeoff. After a takeover, freshness can be up to one cycle optimistic: repositories completed by the earlier run count toward the run that finishes.
  • Out of scope. Manual sync COMMAND_STALLED while the queue backlog exceeds the 5-minute reconciler threshold. Repositories reporting 403 "Dependabot alerts are disabled" need the GitHub App permission on the customer side.

A run that took over an owner lease replaced the saved checkpoint with an
empty skeleton, so every takeover restarted at the first repository and
large owners rescanned the same prefix without ever completing. The 7-day
lease TTL also locked an owner for a week whenever its chain died.

A taking-over run now adopts the previous checkpoint (completed repos and
counters, without the chunk fence or no-progress count), and the lease TTL
drops to 20 minutes, which still outlives the 15-minute consumer limit.
@kilo-code-bot

kilo-code-bot Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

Reviewed the lease-takeover change in claimOwnerSyncLease (adopting the previous run's checkpoint while dropping the chunk fence) and the lease TTL reduction from 7 days to 20 minutes; the SQL fence/merge semantics and the new integration coverage are consistent with the surrounding write/clear/freshness logic, and no high-confidence defect was found.

Files Reviewed (2 files)
  • services/security-sync/src/sync.ts
  • services/security-sync/src/sync.integration.test.ts

Notes (not blocking): adoption carries the prior run's staleRepos/authInvalidRepos/counters forward, which can only make the freshness gate stricter, never loosen it, so no repositories are silently skipped. The 20-minute TTL comment assumes the 15-minute queue consumer wall-clock limit; a chunk that overruns it due to a slow single repository (the first repo has no budget check) could still be taken over, but its writes remain fenced by runId, so this is bounded redundant work rather than corruption.


Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0

Review guidance: REVIEW.md from base branch main

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