Repository navigation
isGitCheckpointable collapses a git failure into not-a-repo, and the fix has a policy question #23295
Adamulek123
started this conversation in
Ideas
Replies: 0 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Problem
isGitCheckpointableinCheckpointService.tsends inEffect.orElseSucceed(() => false). That makes two very different situations indistinguishable:It is called from
captureBaseline, capture availability, andcapture. So a transientgit rev-parsefailure at capture time makes the turn commit withstatus: "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
orElseSucceedmakes git failures propagate, which is what we want in isolation. The problem is the one caller that has no handler:CheckpointCaptureServicecallsmaterializeBaselineCheckpoint/capturewith no recovery.RunFinalizationService.finalize(RunFinalizationService.tsaround thecheckpointCapture.execute(...)call) maps that error toRunFinalizationErrorand does not catch it.EffectWorkermaps that toOrchestrationEffectExecutionErrorand retries tomaxAttempts(5) beforeoutbox.fail.So one flaky
git rev-parseat the end of a turn would burn five attempts and leave the effect terminally failed. Compare the neighbouringprovider-thread.rollbackeffect case, which attachesEffect.tapCauseand 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:
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.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 theRunFinalizationServicecall site that records amissingcheckpoint 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
isGitRepositorythroughRunFinalizationService.finalizeand 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.All reactions