Repository navigation
fix(checkpoint): bound git-refs checkpoint push and finish backlogs in the background - #2715
Open
peyton-alt wants to merge 17 commits into
Open
peyton-alt wants to merge 17 commits into
peyton-alt wants to merge 17 commits into
Conversation
The pre-push hook pushed every queued checkpoint ref in one `git push` with no deadline; only the per-ref retry fallback was bounded. A large backlog, a slow uplink, or an unreachable SSH host could hold the user's `git push` for minutes or indefinitely. - Push queued refs in chunks (checkpointRefPushChunkSize = 25), dequeuing each chunk as it lands so a backlog drains across pushes. - Share one checkpointFlushBudget across the batch and the per-ref fallback in the pre-push path; the explicit migration push stays unbounded. - A failed chunk no longer stops later chunks; only its refs are retried individually. Batching stops after 2 consecutive failed chunks, an SSH auth failure, the budget, or an interrupt. - Stop after one attempt when git cannot connect at all (connect-phase ssh/curl failures, before any chunk landed), and print that line with credentials redacted, instead of retrying ref by ref. - Add `-o ConnectTimeout=30` for OpenSSH under the pre-push hook. - Move the free-text credential redaction helper into gitremote. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4EQ5BPJG653PSJVN6EWAXYK
Under the pre-push hook, withBatchModeSSH appended `-o BatchMode=yes` to any ssh command. plink rejects `-o`, so every hook checkpoint push failed for PuTTY users. A GIT_SSH program path was also copied unquoted into GIT_SSH_COMMAND, which git runs through sh, splitting paths with spaces. Resolve the ssh variant the way git does (GIT_SSH_VARIANT, ssh.variant, then the program name parsed like split_cmdline): - OpenSSH: BatchMode + ConnectTimeout, as before. - plink/putty: -batch, plink's BatchMode; never -o. - TortoisePlink (git passes -batch itself) and simple: env left unchanged. - Unrecognized wrappers: BatchMode only, as before. Shell-quote a GIT_SSH path when rewriting it into GIT_SSH_COMMAND. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4EQFC6B4P2AY0JB87KCYFCS
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Explicit auto SSH variants bypass program detection, causing incorrect options for plink and OpenSSH.
1 open finding
What changed in this PR
Bounds pre-push checkpoint synchronization while improving SSH client compatibility and diagnostics.
Changes:
- Pushes queued checkpoint refs in bounded chunks with shared retry budgets.
- Detects unreachable remotes and configures SSH options by client variant.
- Adds regression tests and updates checkpoint architecture documentation.
| File | Description |
|---|---|
docs/development/checkpoint-implementation.md |
Documents bounded pre-push flushing. |
docs/architecture/ref-checkpoint-backend.md |
Describes chunking and failure handling. |
cmd/entire/cli/strategy/refs_push_test.go |
Updates the flush API call. |
cmd/entire/cli/strategy/refs_push_bounds_test.go |
Tests chunking, budgets, and unreachable remotes. |
cmd/entire/cli/strategy/push_common.go |
Implements chunked batch pushes. |
cmd/entire/cli/strategy/manual_commit_push.go |
Integrates bounded flushing and fallback retries. |
cmd/entire/cli/plugin_gitremote.go |
Reuses shared credential redaction. |
cmd/entire/cli/gitremote/gitremote.go |
Exposes free-text credential redaction. |
cmd/entire/cli/checkpoint/remote/git.go |
Adds reachability detection and SSH variant handling. |
cmd/entire/cli/checkpoint/remote/git_test.go |
Tests SSH variants and reachability detection. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+339
to
+346
| if v, ok := envLookup(env, "GIT_SSH_VARIANT"); ok { | ||
| res.variant = overrideSSHVariant(v) | ||
| return res | ||
| } | ||
| if hasConfigVariant { | ||
| res.variant = overrideSSHVariant(configVariant) | ||
| return res | ||
| } |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit bd9cca4. Configure here.
| if hasConfigVariant { | ||
| res.variant = overrideSSHVariant(configVariant) | ||
| return res | ||
| } |
There was a problem hiding this comment.
Auto SSH variant skips detection
Medium Severity
resolveSSH returns immediately after overrideSSHVariant, so GIT_SSH_VARIANT=auto and ssh.variant=auto never reach program-name detection. Git treats auto as fall-through; here it stays sshVariantAuto, so plink gets -o options it rejects and OpenSSH misses ConnectTimeout.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit bd9cca4. Configure here.
| batchCtx := flushCtx | ||
| if !boundBatch { | ||
| batchCtx = pushCtx | ||
| } |
There was a problem hiding this comment.
Migration fallback budget starts early
Medium Severity
flushCtx is created before the batch and still used for the per-ref fallback when boundBatch is false. A long unbounded migration upload can expire that 2m deadline before recovery runs, so diverged refs fail immediately instead of getting a fresh fallback budget.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit bd9cca4. Configure here.
Review fixes for the bounded git-refs pre-push flush:
- A link too slow to land one chunk within the flush budget cut the same
head-of-queue chunk on every push and never made progress. The chunk size
now adapts (PushQueue.ChunkSizeHint): halved when the budget cuts a chunk,
doubled back after a clean batch. A budget cut ends the flush ("Stopped
pushing") instead of running the per-ref fallback on a spent budget and
reporting it as a failed retry, and rotates the cut chunk to the back.
- The migration push's unbounded batch no longer leaves its per-ref fallback
an already-spent budget.
- ssh.variant / GIT_SSH_VARIANT "auto" defers to program-name detection, as
in git, so plink behind it gets -batch rather than -o.
- An unreadable git config leaves the environment unchanged instead of
replacing the user's core.sshCommand with plain ssh.
- ConnectTimeout raised to 60s for slow-starting ssh proxies.
- Credential redaction covers an unencoded "@" in a password; the printed
cause line drops bidi controls; curl's "failed to connect to" matches only
in its "<host> port" form.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Entire-Checkpoint: 01M4EYPFCATE1W2XQ7GKS7HBH2
…k fails A chunked flush can land chunks and then fail a later one (an SSH auth failure, say). The pre-push read any error as "nothing synced" and skipped committing the captured sync remote and the ignored-checkpoint_remote warning, though the landed checkpoints reached the remote. Act on the landed count instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4F5G5ZMZ7TWB7KMA9A33T6D
The unreachable-remote stop fired whenever nothing had landed yet, even after an earlier chunk was rejected — which equally proves the remote answered. A later transient connect error then abandoned the flush and skipped the per-ref fallback that could still land or recover those refs. Only a connect failure on the first chunk the remote saw now counts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4F6F0VGSQ9J2E6Q47W4G180
Measured against GitHub (ssh) and Entire, 200 checkpoints (~20MB) went out in 7s / 4s as one push but took 20s / 11s in chunks of 25: every push pays a fixed cost (the connection, and the remote advertising every checkpoint ref it holds), so small chunks made every healthy push about three times slower. Raise the ceiling to 200, which restores the single-push speed on healthy links, and quarter the adaptive size when the budget cuts a chunk so a slow link still converges in a few pushes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4F6XJB2HCN41C4BPA9ZFF03
Adversarial review fixes for the adaptive chunked flush: - The budget almost always cuts a long drain mid-chunk, so shrinking on any budget cut quartered the chunk size on every push of a backlog that was draining fine, down to one ref per push. Shrink only when nothing landed. - A forge declines a whole push for one bad ref (push protection, a ruleset), so with 200-ref chunks one secret sent 200 refs to the per-ref fallback and took several budgets to reach. Failed chunks are now re-pushed in halves, recursing only while the sibling half landed, which isolates a single bad ref in about 2*log2(chunk) pushes; the rest goes per-ref. - An end-to-end test now pins that a partial delivery captures the election (the earlier unit test passed without that fix). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4FAB92AEF0JSB43RDPS5BZJ
When the budget cut the batch, every failed chunk went straight to the "stay queued" stop, including one the remote had refused outright earlier in the flush. The per-ref fallback that would show the remote's reason does not run on a spent budget, so a ref blocked for its content could ride along with every budget cut without ever saying why. Print the rejection reason from the stop itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4FB52FBYGR1B33N8QYAKBFS
The pre-push hook now uploads queued checkpoint refs inline for a short budget (5s). An ordinary push finishes inside it and behaves as before. A queue that does not fit is handed to one detached `entire __checkpoint_upload` worker per repository, so the user's git push returns and the remaining checkpoints follow. Only a budget stop is handed off; unreachable, refusing or unauthenticated remotes stay queued for the next push as before. - All flushes serialise on an upload lock in the git common dir. A push that finds it held leaves a request; whoever releases the lock starts a worker for a waiting request. The migration push waits (bounded) and says so. - The hook resolves the OPF decision before locking and carries it, with the pending sync-remote capture, to the worker, which never re-resolves them, commits the capture only after delivery, and uploads nothing if settings or redaction cannot be loaded or a required OPF run cannot happen. - The worker records its last run; the next push prints a failure or announcement once, and `entire status` shows a running or stopped upload. - Uploads stay inline under ENTIRE_CHECKPOINT_UPLOAD_FOREGROUND=1 (trail create, test harnesses), in CI, and with strategy_options.background_checkpoint_upload: false. - SpawnDetached gives children the null device instead of a pipe (ported from the OPF scan worker branch) so a worker survives its first stderr write after the parent exits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4EWAPN9JDYSTB9X7NZK3P20
Inside a worker pass each chunk push is now bounded on its own (checkpointUploadChunkTimeout, 75s). A chunk that outlasts it is recorded as stalled, rotated to the back of the queue, halves the next chunk size, and the pass carries on, so one stalled upload no longer holds every chunk behind it for the whole delivery budget. Stalls do not count toward the consecutive-failure stop. Chunks still go out one at a time: parallel pushes would split a slow uplink and multiply auth failures, for a speedup not yet measured against a real forge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4F4CQ950VWSXJDJDERB1ESD
os/exec passes a child only its stdio and ExtraFiles, but it does not close descriptors the process inherited without close-on-exec. A git pre-push hook inherits git's pipes that way, including the write end of a remote helper's stdin, so the detached checkpoint upload worker kept git-remote-entire from seeing EOF and the user's `git push` to an entire:// remote could not exit until the worker finished (measured 19-82s against Entire; 7s after). SpawnDetached now marks every inherited descriptor above stderr close-on-exec before starting the child. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4F8YP3304B1N3ZT9BHT14DT
- A chunk down to one ref is no longer cut by the worker's per-chunk timeout: a ref slower than the timeout was re-sent on every pass and never landed. It gets the rest of the pass budget. - Inline-only uploads (CI, trail create, the setting) never start a background process: a push that finds the lock held leaves its refs queued, and releasing the lock spawns a worker only where background upload is allowed. - entire status no longer reports a killed worker as running: a run record without a finish time counts as running only while the upload lock is held. - Inherited-descriptor marking falls back to scanning up to RLIMIT_NOFILE where /dev/fd or /proc/self/fd is unavailable. - A stall shrinks the chunk size only when nothing landed, matching the budget-cut rule. - Wording no longer assumes the lock holder is a background worker, and a failed spawn is not reported as a hand-off. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4FASWG4K5H5EQS6XF365500
…nt-ref-push # Conflicts: # cmd/entire/cli/checkpoint/pushqueue.go # cmd/entire/cli/checkpoint/remote/git.go # docs/development/checkpoint-implementation.md
Timing on GitHub and Entire remotes showed 5s handed off 200 x 100KB backlogs that main delivers inline in ~11s. At 10s those land before git push returns, and larger backlogs still hand off after ~13s. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4HATE36T9TM4DKH6XTXD9WR
peyton-alt
changed the title
A batch where some chunks landed and one stalled counted as clean and doubled the size back toward the cap. Clarify why an earlier push's OPF "run" outranks a later skip. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4HBNNCT0XDT20M7AJNVRFE6
The chunked flush is fail-soft about an early stop, but PushQueuedCheckpointRefs callers treat a nil error as success, so a stop between chunks could report a partial push as complete. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4HCS7V4M0VQBTMBC891HPXG
The earlier test passed without the fix: a cancelled context already failed the first chunk. Drive the real case instead — two failed chunks stop the batch, the fallback lands them, the rest stay untried. Mark that stop with a sentinel so doctor migrate does not repeat the reason the flush already printed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4HQT6YQBPF5N2ZQMCJ1KK6F
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://entire.io/gh/entireio/cli/trails/1530
Problem
The pre-push hook pushed every queued checkpoint ref in one
git pushwith no deadline. #2522 bounded only the per-ref retry fallback. A large backlog, a slow uplink or an unreachable SSH host could hold the user's owngit pushfor minutes or indefinitely, which pushes users toward--no-verifyand leaves checkpoints untracked.Changes
1. Bounded, chunked batch (
fix(checkpoint): bound the git-refs pre-push batch push)checkpointRefPushChunkSize(200), and each chunk leaves the queue as soon as it lands. A backlog drains across pushes instead of being cut at the same point every time.PushQueue.ChunkSizeHint). A link too slow to land a full chunk within the budget would otherwise cut the same chunk on every push and never make progress. A budget cut ends the flush ("Stopped pushing") rather than running the per-ref fallback on a spent budget, and moves the cut chunk to the back of the queue.checkpointFlushBudget(2m). Worst case was unbounded + 2m; it is now 2m.PushQueuedCheckpointRefs) keeps the batch unbounded, and its per-ref fallback gets a fresh budget after the batch.remote:lines; a miss falls back to the bounded fallback.-o ConnectTimeout=60under the hook unless the command sets one. A value in~/.ssh/configis overridden; 60s leaves room for slow-starting proxies.gitremoteso the push path and the plugin path share it.2. Background upload (hybrid)
The bounded flush caps a slow link's wait at the 2m budget. The background upload removes most of that wait.
checkpointInlineUploadBudget). If the budget stops it with refs still queued, it hands them to one detachedentire __checkpoint_uploadworker per repository, prints "Uploading the remaining N checkpoint ref(s) in the background", and returns.git pushreturns.entire statusshows "uploading in the background" or the last stop reason.ENTIRE_CHECKPOINT_UPLOAD_FOREGROUND=1(set bytrail createand by the test harnesses), in CI, and withstrategy_options.background_checkpoint_upload: false.execx.SpawnDetached:git-remote-entire's stdin, andgit pushto anentire://remote couldn't exit until the worker did.3. plink fix (
fix(checkpoint): give plink -batch instead of OpenSSH -o options)This bug is separate from the hang, but it lives in the same function.
The hook appended
-o BatchMode=yesto any ssh command, so plink rejected it and every hook checkpoint push failed for PuTTY users. The variant is now resolved the way git does it (GIT_SSH_VARIANT,ssh.variant, then the program name parsed likesplit_cmdline):BatchModeandConnectTimeout, unchanged-batchsimpleBatchModeonly, as before (#1523)A
GIT_SSHpath is shell-quoted when rewritten intoGIT_SSH_COMMAND, so a path with spaces stays one word.autodefers to name detection, as in git. If the git config naming the ssh command can't be read, the environment is left unchanged.Measured
Both remotes were real throwaway repos. Each run uploaded 200 checkpoints of about 100KB each (roughly 20MB) through the pre-push hook.
On a simulated slow link (1s per push plus 0.75s per ref), v0.10.6 blocked
git pushfor 152s. This PR caps the wait at the 2m budget and leaves the rest queued for the next push.With background upload (default outside CI), 200 queued checkpoints.
git pushblocked / when all checkpoints were on the remote:Known limitations
trail create), a very slow uplink still waits up to the 2m budget, and a backlog drains over several pushes.git worktree removeuntil the run ends.-ostill getBatchMode; that behavior is unchanged.Testing
GIT_SSH_VARIANT,ssh.variant);split_cmdline-style parsing.mise run checkpasses: lint, unit, integration and canary.🤖 Generated with Claude Code