Handle failing push better

[Soph]

https://entire.io/gh/entireio/cli/trails/365

Make a stuck entire/checkpoints/v1 push diagnosable and bounded

A colleague's git push hung with an unbounded stream of dots on the pre-push hook's [entire] Pushing entire/checkpoints/v1 to origin... line. Only Ctrl-C broke it, and the resulting Warning: couldn't sync ...: fetch failed: had nothing after the colon — the doctor bundle showed no push events at all.

Root cause

exec.CommandContext's default cancellation only SIGKILLs the direct child. git push over HTTPS spawns git-remote-https + credential helpers that inherit our stdio pipes; killing git leaves them alive holding stdout, so cmd.CombinedOutput() blocks past tryPushSessionsCommon's 2-minute deadline. The dots are 1-Hz progress emitted in-process; with no cancellation, they tick forever.

Fixes

• execx.KillOnCancel(cmd, waitDelay) — Unix uses Setpgid + Cancel killing the process group; Windows uses CREATE_NEW_PROCESS_GROUP + taskkill.exe /F /T /PID for descendant-tree termination. WaitDelay backstops pipe drain. newCommand in checkpoint/remote applies it to every git subprocess.
• doPushBranch UX: returns distinct errPushTimedOut/errFetchTimedOut; ends the dot line with " timed out" instead of trailing silently; skips the sync→retry cascade on timeout (same network condition would just trip the fetch deadline); falls back to the wrapped fetch error when git's output is empty so no more bare fetch failed:.
• INFO logging of every push attempt (branch, target alias, elapsed, classification — never raw URLs/tokens) so doctor bundles capture push history at the default log level.
• Outer-ctx cancellation (Ctrl-C / hook deadline) bails silently at every stage of the cascade — first attempt, sync stage, and retry — instead of cascading into misleading warnings.

Tests

• execx: pins the stdlib hang (TestExecCommandContext_HangsOnGrandchildHoldingPipe) and verifies KillOnCancel returns within WaitDelay (~200 ms vs 5 s).
• strategy: covers timeout suffix, sync-cascade skip, outer-cancel bail, INFO log writes, sentinel propagation, classifyForLog, and the reportPushFailure chokepoint under -race.

Net effect for the colleague's symptom

Next time GitHub stalls a push, the 2-minute timeout actually fires, the dot line ends with timed out, a hint points at log_level=DEBUG, and the bundle records the attempt. No infinite dots, no Ctrl-C needed.

---

Note

Medium Risk
Touches git push/fetch execution and hook messaging, including new process-group kill behavior and timeout handling; while scoped to checkpoint syncing, it could change cancellation semantics across platforms and affect reliability if misconfigured.

Overview
Prevents checkpoint git push/fetch from hanging indefinitely on cancellation. All git subprocesses created in checkpoint/remote/newCommand are now configured with a new cross-platform execx.KillOnCancel helper that kills the entire process tree/process group and applies a bounded WaitDelay to unblock CombinedOutput when helpers keep stdio pipes open.

Improves checkpoint push UX and diagnosability. doPushBranch now logs push attempts at INFO with a sanitized class field, introduces explicit push/fetch timeout sentinels, prints clearer timeout messaging (including a DEBUG logging hint), skips the sync→retry cascade on push timeouts, bails quietly when the outer context is cancelled, and ensures fetch failures include a non-empty underlying error when git produced no output.

Adds focused regression tests. New unix-only tests pin the stdlib hang behavior and verify KillOnCancel wiring, plus strategy tests covering timeout messaging, cancellation behavior, logging, and error classification.

^{Reviewed by Cursor Bugbot for commit 8246e78. Configure here.}

[Copilot]

Pull request overview

This PR makes checkpoint branch pushing (entire/checkpoints/v1) bounded and diagnosable by ensuring git subprocess cancellation reliably terminates descendant helper processes (preventing indefinite hangs), and by improving timeout/error messaging plus INFO-level logging so doctor bundles capture push attempts.

Changes:

• Add execx.KillOnCancel to kill the full descendant process tree on context cancellation (Unix via process groups; Windows via taskkill /T), with WaitDelay as a pipe-drain backstop.
• Improve doPushBranch push UX and control flow: distinct push/fetch timeout sentinels, clearer dot-line suffixes, skip sync→retry cascade on push timeout, and avoid empty fetch failed: messaging.
• Add INFO logging for push attempts and add regression/unit tests covering the cancellation and messaging behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
cmd/entire/cli/strategy/push_messaging_test.go	New tests covering timeout UX, cascade suppression, cancellation bail-out, INFO logging, and error classification.
cmd/entire/cli/strategy/push_common.go	Adds timeout sentinels, improved messaging/control flow, INFO logs, and shared failure-reporting helper.
cmd/entire/cli/strategy/push_common_test.go	Updates stderr capture helper to also redirect pushStderr for new output plumbing.
cmd/entire/cli/execx/killoncancel_windows.go	Windows implementation of KillOnCancel (process-group isolation + taskkill /T).
cmd/entire/cli/execx/killoncancel_unix.go	Unix implementation of KillOnCancel (Setpgid + kill process group).
cmd/entire/cli/execx/killoncancel_unix_test.go	Unix tests pinning the stdlib hang and verifying KillOnCancel unblocks promptly.
cmd/entire/cli/checkpoint/remote/git.go	Wires KillOnCancel into all git subprocesses created by newCommand.
cmd/entire/cli/checkpoint/remote/git_killoncancel_unix_test.go	Verifies newCommand applies KillOnCancel wiring on Unix.

Comments suppressed due to low confidence (5)

cmd/entire/cli/execx/killoncancel_unix_test.go:59

• Same portability issue as above: the test guards with LookPath("sh") but still invokes /bin/sh. Prefer using the LookPath result (or sh by name) so the test doesn’t depend on /bin/sh existing.

	if _, err := exec.LookPath("sh"); err != nil {
		t.Skip("sh not available")
	}

	ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond)
	defer cancel()

	cmd := exec.CommandContext(ctx, "/bin/sh", "-c", grandchildHoldingStdoutCmd)
	KillOnCancel(cmd, 500*time.Millisecond)

cmd/entire/cli/strategy/push_messaging_test.go:167

• Same flakiness risk here: pushAttemptTimeout = 1*time.Millisecond may not reliably expire before remote.Push returns when pushing to a local bare repo. Use an immediate timeout (0 duration) or another deterministic approach so the test consistently exercises the errPushTimedOut path.

	withShortPushTimeout(t, 1*time.Millisecond)

	_, err := tryPushSessionsCommon(context.Background(), bareDir, paths.MetadataBranchName)
	require.Error(t, err)
	assert.ErrorIs(t, err, errPushTimedOut,
		"err must wrap errPushTimedOut; got %v (class=%q)", err, classifyForLog(err))
}

cmd/entire/cli/strategy/push_common.go:137

• push attempt failed logs omit the target alias. Adding the same redacted/aliased target field used in push attempt start would make failure events diagnosable without relying on surrounding context.

	logging.Info(ctx, "push attempt failed",
		slog.String("branch", branchName),
		slog.Duration("elapsed", time.Since(pushStart)),
		slog.String("class", classifyForLog(err)),
	)

cmd/entire/cli/strategy/push_common.go:209

• push retry completed logs omit the target alias, which makes it harder to interpret retries in aggregated logs. Consider logging the same redacted/aliased target field as the initial attempt.

	logging.Info(ctx, "push retry completed",
		slog.String("branch", branchName),
		slog.Duration("elapsed", time.Since(retryStart)),
		slog.Bool("up_to_date", retryResult.upToDate),
	)

cmd/entire/cli/strategy/push_common.go:252

• reportPushFailure INFO logs include branch/elapsed/class but not the push target. Since reportPushFailureArgs already carries target, consider logging a redacted/aliased target value here as well to make sync/retry failures attributable in doctor bundles.

	logging.Info(ctx, a.logMsg,
		slog.String("branch", a.branch),
		slog.Duration("elapsed", time.Since(a.start)),
		slog.String("class", classifyForLog(a.err)),
	)

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ 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 8246e78. Configure here.}

[cursor]

Fetch timeout check can't distinguish inner from outer deadline

Medium Severity

In fetchAndRebaseSessionsCommon, ctx is shadowed at line 538 by context.WithTimeout, so the new timeout check at line 572 (errors.Is(ctx.Err(), context.DeadlineExceeded)) cannot distinguish whether the inner fetch deadline or the outer context deadline fired. In contrast, tryPushSessionsCommon correctly avoids this by using localCtx and guarding with ctx.Err() == nil. If the outer context's deadline fires during the fetch (e.g., hook-level timeout shorter than fetchAttemptTimeout), this incorrectly returns errFetchTimedOut instead of letting the outer cancellation propagate naturally.

Additional Locations (1)

• cmd/entire/cli/strategy/push_common.go#L433-L442

 

^{Reviewed by Cursor Bugbot for commit 8246e78. Configure here.}

[pfleidi]

This sounds like we should fix it 😅