Skip to content

isGitCheckpointable collapses a git failure into not-a-repo, and the fix has a policy question #16864

Closed

Description

@Adamulek123

Problem

isGitCheckpointable in CheckpointService.ts ends in Effect.orElseSucceed(() => false). That makes two very different situations indistinguishable:

  • this directory is not a git repository, which is a legitimate "not checkpointable"; and
  • git was asked and failed (a timeout, a lock, a corrupt repo), which is an error.

It is called from captureBaseline, capture availability, and capture. So a transient git rev-parse failure at capture time makes the turn commit with status: "missing" instead of a checkpoint. No checkpoint exists, rollback is impossible, and nothing signals that anything went wrong.

Why this is not simply fixed

Removing the orElseSucceed makes git failures propagate, which is what we want in isolation. The problem is the one caller that has no handler:

  • CheckpointCaptureService calls materializeBaselineCheckpoint / capture with no recovery.
  • RunFinalizationService.finalize (RunFinalizationService.ts around the checkpointCapture.execute(...) call) maps that error to RunFinalizationError and does not catch it.
  • EffectWorker maps that to OrchestrationEffectExecutionError and retries to maxAttempts (5) before outbox.fail.

So one flaky git rev-parse at the end of a turn would burn five attempts and leave the effect terminally failed. Compare the neighbouring provider-thread.rollback effect case, which attaches Effect.tapCause and dispatches a user-visible failure event instead.

The other call site, RunExecutionService, catches and logs, so it is unaffected.

The question

Should a single transient git failure be able to fail a whole run's finalization?

Three defensible answers:

  1. No -- capture should degrade to status: "missing" and continue, but record that it was a failure rather than a non-repo, so the run still settles and the cause is visible.
  2. Yes -- a turn whose checkpoint could not be captured should not be treated as fully settled, because the user may later find they cannot rewind it.
  3. Retry harder first -- distinguish a timeout from a non-repo, retry the git call, and only propagate after retries are exhausted.

Option 1 preserves current run-settlement behaviour while still fixing the silent-data-loss aspect, and is the smallest change. Option 3 is the most correct but adds retry policy to a path that currently has none.

Suggested shape if (1) is chosen

Keep the distinction in isGitCheckpointable (a repo-detection failure is not a non-repo), and add a recovery at the RunFinalizationService call site that records a missing checkpoint carrying the real cause, then continues. That keeps the honest signal without making finalization fail.

A proof would be a test that drives a failing isGitRepository through RunFinalizationService.finalize and asserts the run still reaches a terminal state.

Environment

Found by a static audit of the merged orchestrator-v2 work (#2829). Re-verified present at main. Deliberately kept out of #16863 so that PR stays a clean two-fix ask.

Activity

  1. locked and limited conversation to collaborators on Oct 22, 2026
  2. converted this issue into a discussion #23295 on Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions