Fix #2316: skip synthetic-session checkpoints + serialize cross-source-repo pushes - #2328
Conversation
…e-repo pushes Two checkpoint storage failures were firing on every pipeline run. Failure 1: orchestrator-internal helpers (ls-remote, failsafe-fetch) register synthetic sessions on the gateway. On session deletion, the gateway tries to capture a session-end checkpoint for them — but they have no proxy buffer (no agent ran), so they only produce metadata-only checkpoints. When the source repo is read-only (e.g. Khan/actions), the push falls back to the source origin and fails with permission denied, logging at ERROR. Fix: add a `synthetic` flag to Session, plumb it through register_session and the four temp-session helpers in gateway_client.py, and short-circuit `_capture_and_cleanup_session` for synthetic sessions. No data is lost — these checkpoints only carried synthetic container_id metadata. Failure 2: when two writers from different source repos targeted the same shared `egg/checkpoints/v2` branch in jwbron/egg-checkpoints, the per-source-repo lock didn't serialize them. The second writer hit a non-FF rejection, attempted rebase, and failed with a content conflict on `index.json` (which is structurally an append but textually a diff). After 3 attempts the checkpoint was dropped — real session-end transcripts and tool calls lost. Fix: - Re-key the store lock by `checkpoint_repo or repo_path` so writers contending on the shared destination serialize. Renamed the dict and helper to reflect the broader scope (`_store_locks`, `_get_store_lock`). - Replace rebase-on-non-FF with regenerate-on-non-FF: discard the local commit, fetch the latest tip, reset the temp worktree, re-apply this checkpoint's delta against the freshly fetched index, recommit, push. This treats `index.json` as a structural append rather than a textual diff, so concurrent writers never conflict.
This comment has been minimized.
This comment has been minimized.
Combine #2316's destination-keyed in-process lock with #2311's cross-process bare_repo_lock into a single _get_store_lock context manager. Drops _get_repo_lock now that the store lock subsumes it.
Conflict Resolution SummaryWhen I went to resolve conflicts with The conflict was in
Both intents preserved:
The merged Verification (against
Please review: the lock-key separation in — Authored by egg |
This comment has been minimized.
This comment has been minimized.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed both fixes end-to-end. The changes correctly address both failure modes from #2316:
- Synthetic short-circuit is wired through
register_session→Session→_capture_and_cleanup_session. Persistence round-trips viato_dict_for_persistence/from_persistence, defaulting toFalse. The early return atgateway/session_manager.py:110correctly bypasses both the auto-commit attempt and the checkpoint capture+buffer cleanup, which is the right behavior for sessions that have no agent and no proxy buffer. - Target-keyed lock preserves the original
.git/worktreesserialization becausebare_repo_lock(repo_path)is still acquired second and carries its own in-processRLock(shared/egg_git/cross_process_lock.py:46-90). I traced the four ordering combinations for(thread_lock_key, repo_path)and there is no deadlock potential. - Regenerate-on-non-FF correctly discards the local commit, force-fetches the remote tip, and rebuilds
index.jsonviaadd_checkpoint_to_index_v2's read-modify-write (shared/egg_contracts/checkpoint_loader.py:282-322). The unique-by-id checkpoint file path means the file write never collides;commit_msgis captured before the loop so the regenerate commit is identical.
No blocking issues. A few non-blocking observations below.
Non-blocking
1. Other temp-session helpers in orchestrator/gateway_client.py weren't marked synthetic=True
The PR threads synthetic=True through fetch_worktree_branch, fetch_branch, ls_remote_branch, and get_remote_branch_sha. But there are at least five other temp-session helpers in the same file with the same shape — register, do one gateway call, delete — and they will continue to produce metadata-only checkpoints whose push fails noisily on read-only source repos (Failure 1):
auto-pratorchestrator/gateway_client.py:1149(passesrepos=[repo], so it actually creates a worktree — biggest impact)rebase_ontoatorchestrator/gateway_client.py:1374slice-branch-…atorchestrator/gateway_client.py:1497stacked-pr-listatorchestrator/gateway_client.py:1575stacked-pr-ls-remoteatorchestrator/gateway_client.py:1670
Worth either marking these synthetic in this PR or filing a follow-up — they're the same root cause and the fix is one keyword argument each.
2. Pre-existing race in the orphan-branch path is unchanged by the regenerate flow
gateway/checkpoint_handler.py:976-979 does worktree add --detach followed by checkout --orphan CHECKPOINT_BRANCH, which leaves the worktree on CHECKPOINT_BRANCH (no longer detached). If a concurrent writer creates the branch on origin between the _branch_exists check at line 895 and the push at line 1031, our orphan push gets non-FF. The new regenerate flow then runs:
self._run_git(
repo_path,
["fetch", target, f"+{CHECKPOINT_BRANCH}:{CHECKPOINT_BRANCH}"],
...
)Git refuses to fetch into a ref that's checked out in any worktree, so this fails and the whole regenerate aborts. The original rebase code had the same problem (running fetch from temp_path makes no difference — the checked-out check is across all worktrees of the bare repo). It's pre-existing, so not strictly a regression, but the PR is in the area — a git checkout --detach after the orphan commit (or at the top of the regenerate retry block) would close it.
3. Test could use a Barrier like its sibling
test_concurrent_stores_with_shared_checkpoint_repo_serialized (gateway/tests/test_checkpoint_handler.py:858-912) asserts max_in_flight == 1 but doesn't use a threading.Barrier to force overlap when serialization is broken. The sibling test_concurrent_stores_on_different_repos_not_serialized at gateway/tests/test_checkpoint_handler.py:819,829 does — without it, a regression where the lock disappears could pass this test if the OS happens to schedule the threads sequentially. The 50ms time.sleep plus multiple _run_git calls makes it unlikely to be flaky in practice, but the barrier is the more rigorous shape.
4. synthetic field accepts any truthy value at the gateway
gateway/gateway.py:8010 does synthetic = bool(data.get("synthetic", False)) with no type check. Other optional fields in this endpoint validate type explicitly (e.g., pipeline_id at line 8038, agent_role at line 8056). Since /api/v1/sessions/create is launcher-secret authenticated this isn't a security issue, but a if not isinstance(synthetic, bool): return make_error(...) would be consistent with the surrounding validation style.
5. _get_store_lock docstring doesn't call out the cross-pod limit
gateway/checkpoint_handler.py:1373-1391: the docstring says "Per-destination ... in-process serialization" but doesn't mention that the destination key only synchronizes within a single gateway process. With multiple gateway pods writing to the same checkpoint_repo, the regenerate-on-non-FF retry is the actual cross-process protection (the bare_repo_lock flock is keyed by repo_path, not by destination). One sentence in the docstring would make this explicit.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Mark 5 additional orchestrator-internal temp-session helpers as synthetic=True (auto-pr, rebase_onto, slice-branch, stacked-pr-list, stacked-pr-ls-remote) so their session-end checkpoints skip the same noisy push path the four read-only helpers already skip. - Validate ``synthetic`` is a bool at /api/v1/sessions/create (matches the surrounding pipeline_id / agent_role validation style; replaces the silent ``bool(data.get(...))`` coercion). - Document in ``_get_store_lock`` that the destination key only serializes within a single gateway process — cross-pod writers race past it and the regenerate-on-non-FF retry is the actual cross-pod protection. - Add ``threading.Barrier`` to the new shared-checkpoint_repo serialization test, mirroring its sibling, so a regression that drops the destination-keyed lock is caught even if the OS happens to schedule the threads sequentially. - Detach the temp worktree from CHECKPOINT_BRANCH at the top of the regenerate retry block. The orphan path leaves the worktree on the branch (``checkout --orphan`` switches to it), so a concurrent writer creating the branch on origin between ``_branch_exists`` and our push would otherwise cause the regenerate fetch to fail (git refuses to fetch into a ref checked out in any worktree).
|
Addressing the 5 non-blocking observations from the review.
Verification:
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
Autofix tracking{"Lint/Python": 1} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-reviewed the delta from baaeb49 → 662658d against the previous round's 5 non-blocking observations. All 5 are addressed correctly.
Verification of previous suggestions
-
Other temp-session helpers marked
synthetic=True✓ — Confirmed atorchestrator/gateway_client.py:1158, 1382, 1506, 1582, 1746(auto-pr,rebase_onto,slice-branch,stacked-pr-list,stacked-pr-ls-remote).register_sessiononly forwards the field when truthy (line 428-429), so the wire payload is unchanged for non-synthetic callers. -
git checkout --detachbefore fetch in regenerate retry ✓ — Placed atgateway/checkpoint_handler.py:1059-1062, immediately before the fetch+reset sequence. I traced both paths:- Orphan path leaves the worktree on
CHECKPOINT_BRANCHaftercheckout --orphan(line 978), so the detach is load-bearing — without itfetch +CHECKPOINT_BRANCH:CHECKPOINT_BRANCHwould be refused because the ref is checked out. - Branch-exists path uses
worktree add --detach(line 956-960), so HEAD is already detached when the loop starts; the newcheckout --detachis a no-op there. Still safe in both paths.
- Orphan path leaves the worktree on
-
threading.Barrier(2)in concurrent test ✓ —gateway/tests/test_checkpoint_handler.py:885,892-895. Barrier-with-timeout is the right shape: under serialization only one thread reaches it, the 2.0s wait times out,BrokenBarrierErroris caught, subsequent waits raise immediately (broken state), so the test runs in ~2s rather than hanging. If the destination-keyed lock regresses, both threads entertrack_run_git, the barrier opens with 2 parties, andmax_in_flightjumps to 2. -
Strict
isinstance(synthetic, bool)validation ✓ —gateway/gateway.py:8087-8089. Default branch (data.get("synthetic", False)) yields a realFalse, which passes the isinstance check. Testtest_session_create_rejects_non_bool_syntheticcovers"yes". Notefrom_persistenceatgateway/session_manager.py:404still doesbool(data.get("synthetic", False)), but that read path consumes already-validated data so loose coercion there is fine. -
_get_store_lockdocstring cross-pod note ✓ —gateway/checkpoint_handler.py:1397-1403. Explicit on the limitation: destination key only synchronizes within a single gateway process; the regenerate-on-non-FF retry is the cross-pod protection becausebare_repo_lockis keyed byrepo_pathnot destination.
Merge resolution review
The PR description flags that _get_store_lock(key, repo_path) was reworked during conflict resolution to combine #2316's destination-keyed lock with #2311's bare_repo_lock. Confirmed sound:
- Lock ordering inside the context manager is
thread_lockthenbare_repo_lock(gateway/checkpoint_handler.py:1416), matchingWorktreeManager._get_repo_lock(gateway/worktree_manager.py:614). Same ordering across both callsites = no AB/BA deadlock. - The two keys are intentionally different: destination key serializes cross-source-repo writers contending on the shared
egg/checkpoints/v2branch;repo_pathkey serializes cross-process contention with the orchestrator's state-store on.git/config.lock. They cover orthogonal hazards.
Non-blocking
1. Test for checkout --detach doesn't enforce ordering relative to fetch
gateway/tests/test_checkpoint_handler.py:1929-1936 asserts len(detach_after_push) >= 1 and len(fetch_after_push) >= 1 independently. A regression that re-orders to fetch → detach (or omits the detach in one of the two paths) would still pass this test because both calls are present in post_push. Tightening to e.g. assert post_push.index(detach_after_push[0]) < post_push.index(fetch_after_push[0]) would catch reordering. Minor — the current shape still catches the "detach missing entirely" regression that was the actual concern.
2. test_session_create_rejects_non_bool_synthetic only covers "yes"
Other non-bool truthy types (int 1, list [], dict {}, None is treated as default) all hit the same isinstance check, but the test only exercises the string case. Not a coverage gap that matters in practice — isinstance(x, bool) is total — but parametrizing across a couple of types would be more thorough.
— Authored by egg
Resolved conflicts in: - gateway/checkpoint_handler.py: combined PR's destination-keyed store lock (#2316) with main's narrowed bare_repo_lock windows (#2332). The wrapper is now _get_store_lock(key) — in-process only; bare_repo_lock is applied at specific worktree ops. - gateway/tests/test_checkpoint_handler.py: kept both new tests (additive); updated the #2332 test to use the renamed _store_locks.
This comment has been minimized.
This comment has been minimized.
Conflict Resolution SummaryResolved merge conflicts with
Verification
Please reviewThe two PRs interact: #2316 (this PR) re-keys the in-process lock by destination, and #2332 (now on — Authored by egg |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conflict Resolution SummaryMerged Files updated by the merge (all from main, none touched by this PR):
Verification
Please review: Nothing to flag — the merge was textually clean and main's changes are in unrelated areas (#2323 cursor-file race fix, #2347 docs). — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-reviewed the delta from 662658d → eff66d3. The only change is the merge commit bringing origin/main (#2332's narrowed bare_repo_lock + #2347's _module_loader.py extraction) into the PR branch. The conflict resolution in gateway/checkpoint_handler.py looks correct.
Verification of merge resolution
1. _get_store_lock(key) is now in-process only (gateway/checkpoint_handler.py:1402-1437). The combined design preserves both intents:
- #2316: destination-keyed
threading.Lockkeyed oncheckpoint_repo or repo_pathso cross-source-repo writers contending on the sharedegg/checkpoints/v2branch serialize their fetch + commit + push. - #2332:
bare_repo_lock(repo_path)is no longer held across the entire op (which would block state-store commits for up to ~135s under fetch-retry pathology). Instead it wraps the three narrow windows that touch the bare repo's.git/:worktree add --detachin the existing-branch path (checkpoint_handler.py:957-967)branch -D+worktree add --detachin the orphan path (checkpoint_handler.py:978-987)worktree remove --force+worktree prunein the cleanupfinally(checkpoint_handler.py:1138-1151)
The updated docstring at checkpoint_handler.py:1418-1428 explicitly notes that cross-process protection on .git/config.lock is now handled by the narrower windows, with the rationale for not holding the flock across fetch.
2. Lock ordering is consistent and deadlock-free. Within store_checkpoint_v2, the order is always _get_store_lock(key) (in-process thread lock, held for the whole with block) → bare_repo_lock(repo_path) (acquired and released narrowly inside). The flock is never nested with itself; multiple narrow acquire/release cycles are independent. _get_store_lock does not interact with WorktreeManager._get_repo_lock, and the orphan-container cleanup path that calls _capture_and_cleanup_session (worktree_manager.py:1518-1522) runs outside _get_repo_lock, so the worktree-manager lock never composes with _get_store_lock in the wrong order.
3. Regenerate-on-non-FF still works in both paths. The checkout --detach added in this PR (checkpoint_handler.py:1071-1074) is still load-bearing for the orphan path: after checkout --orphan CHECKPOINT_BRANCH at line 990, the temp worktree has CHECKPOINT_BRANCH checked out, so the regenerate-step fetch +CHECKPOINT_BRANCH:CHECKPOINT_BRANCH would otherwise be refused. Existing-branch path uses worktree add --detach, so the detach is a safe no-op there. Either way, +CHECKPOINT_BRANCH:CHECKPOINT_BRANCH force-updates the local ref, then reset --hard CHECKPOINT_BRANCH repositions the temp worktree's HEAD; the subsequent commit sits on a detached HEAD and pushes as a fast-forward against the just-fetched tip.
4. Test coverage is intact. The autouse _stub_bare_repo_lock no-op fixture (test_checkpoint_handler.py:28-50) is per-test-overridable; the new test_bare_repo_lock_not_held_across_fetch_retry (test_checkpoint_handler.py:930-999) installs its own recording stand-in. It asserts the three initial fetch attempts run at flock depth 0 and that worktree add/worktree remove/worktree prune run at depth ≥ 1 — exactly the invariant the merged design needs. The PR's own concurrent-stores serialization test (test_concurrent_stores_with_shared_checkpoint_repo_serialized) and the orphan-detach assertion in test_push_retries_on_non_fast_forward are both unaffected by the resolution.
Non-blocking
1. Regenerate-path fetch is not asserted to run outside the flock
test_bare_repo_lock_not_held_across_fetch_retry mocks _branch_exists=True and exercises only the initial fetch retry loop (checkpoint_handler.py:920-951). The regenerate fetch at checkpoint_handler.py:1079-1084 runs in a separate code path that's only reachable after a non-FF push rejection, and it has the same flock-must-not-be-held requirement (it shares the same fetch-timeout pathology). A future change that wrapped the regenerate fetch in bare_repo_lock would not be caught by the existing test. Adding a parametrized variant that drives a non-FF rejection and asserts the second fetch is also at depth 0 would close this gap.
2. Test only exercises worktree add in the existing-branch path
test_bare_repo_lock_not_held_across_fetch_retry mocks _branch_exists=True, so the orphan path's branch -D is never executed. A regression that moved branch -D outside the flock window (checkpoint_handler.py:978-987) would not be caught. Same observation applies to a regression that moved checkout --orphan inside the flock — the test doesn't differentiate. Minor; the existing-branch path is the common case, and the previous review's #2332 design rationale (only the bare-repo-.git/ writers need the flock) is documented in the comments.
3. PR description still references _get_store_lock(key, repo_path)
The PR's "Conflict Resolution Summary" comment (and the prior body) describes the function as _get_store_lock(key, repo_path). After the actual resolution it's _get_store_lock(key) with bare_repo_lock moved to narrower windows. The implementation matches main's #2332 shape, not the two-arg combined shape that earlier comments suggested. Worth syncing the PR body before merge so reviewers reading the description aren't looking for the wrong signature.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-reviewed the delta from eff66d31 → 4f21940. No new PR-authored code in this delta — the only commit on the PR branch since the last review is a clean merge of origin/main into egg/fix-2316-checkpoint-storage. I confirmed this two ways:
git log eff66d31..4f21940 --no-mergesreturns only commits from main (#2326 cursor-file race fix, #2347 docs structure listing). No author-authored commits.git diff <merge-base>..eff66d31(PR-only changes from before) andgit diff origin/main..4f21940(PR-only changes after the merge) are byte-identical (690 lines each, zero diff between them).
The merge is a clean Y-shape: changes vs parent 1 (eff66d31) come entirely from main's tree (docs/, orchestrator/routes/pipelines.py, sandbox/egg_lib/orch_cli.py, sandbox/tests/test_message_wait_cli.py); changes vs parent 2 (0110999ac) are entirely the PR's own gateway/checkpoint changeset. Zero overlap, no manual conflict resolution, and main's incoming files are in unrelated areas.
Verification of the merged result
_get_store_lock(key)keeps its single-arg signature atgateway/checkpoint_handler.py:1402(combined #2316 + #2332 design from the prior mergeeff66d31).bare_repo_lock(repo_path)still wraps only the three narrow bare-repo.git/writers —worktree add(existing-branch path, line 957),branch -D+worktree add(orphan path, line 978), andworktree remove --force+worktree prune(cleanupfinally, line 1138).- The single in-process acquire at the top of
store_checkpoint_v2(checkpoint_handler.py:888) and the regenerate-on-non-FF push retry (lines 1041–1106) are unchanged.
Non-blocking (carried over from the previous review — still applicable)
The three non-blocking suggestions from eff66d31 still stand because the underlying code paths are unchanged:
- Regenerate-path fetch is not asserted to run outside the flock.
test_bare_repo_lock_not_held_across_fetch_retry(gateway/tests/test_checkpoint_handler.py:930-999) only exercises the initial fetch retry loop. The regenerate fetch atcheckpoint_handler.py:1079-1084has the same flock-must-not-be-held requirement and would benefit from a parametrized variant that drives a non-FF rejection and asserts the second fetch is also at depth 0. - Test only exercises
worktree addin the existing-branch path. With_branch_exists=Truethe orphan path'sbranch -Dis never executed, so a regression that movedbranch -Doutside (orcheckout --orphaninside) the flock would not be caught. - PR description still references
_get_store_lock(key, repo_path)in the older "Conflict Resolution Summary" comment thread. The implementation is single-arg_get_store_lock(key). Worth syncing the PR comment narrative before merge.
None of these block the merge — the implementation is correct and the test suite exercises the load-bearing invariants for the common path.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Two new TestStoreCheckpointV2Concurrency tests close coverage gaps called out as non-blocking review feedback on PR #2328: - test_bare_repo_lock_not_held_across_regenerate_fetch drives a non-FF push rejection so the regenerate-path fetch runs, and asserts every fetch (initial + regenerate) executes at flock depth 0. The existing test_bare_repo_lock_not_held_across_fetch_retry only exercised the initial fetch; a regression that wrapped the regenerate fetch in bare_repo_lock would have gone undetected. - test_bare_repo_lock_wraps_branch_d_in_orphan_path mocks _branch_exists=False to drive the orphan path. Asserts `branch -D` and `worktree add` run under the flock (depth >=1), and `checkout --orphan` runs outside it (depth 0). The existing test mocked _branch_exists=True so the orphan path's `branch -D` was never executed.
|
Addressing the 3 non-blocking items carried over in the latest review.
Verification:
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-reviewed the delta 4f21940 → 3e7e8ee7. Single commit adds two regression tests (gateway/tests/test_checkpoint_handler.py, +135 lines). No production code changes.
Verification of the new tests
1. test_bare_repo_lock_not_held_across_regenerate_fetch (gateway/tests/test_checkpoint_handler.py:1001-1065) — closes non-blocking #1 from the prior review.
Traced execution against the production path at gateway/checkpoint_handler.py:880-1151:
_branch_exists=Truedrives the existing-branch path. Initial fetch at line 922 runs at depth 0 (correct — outside the flock).worktree addat line 957-967 runs underbare_repo_lockat depth 1.- Mocked
track_run_gitraisesCheckpointError("non-fast-forward")only on the first push (line 1043), sopush_attempt=1enters the regenerate branch at line 1050-1106. The regenerate-pathfetch +CHECKPOINT_BRANCH:CHECKPOINT_BRANCHat line 1079-1084 runs at depth 0 (outside any flock). push_attempt=2succeeds; cleanupfinallyrunsworktree remove+worktree pruneat depth 1.
The assertion all(d == 0 for d in fetch_observations) correctly catches a regression that would wrap line 1079-1084 in bare_repo_lock(repo_path). The len(push_calls) == 2 guard confirms the regenerate path was actually exercised — without it the test could pass even if the code skipped the regenerate branch entirely.
2. test_bare_repo_lock_wraps_branch_d_in_orphan_path (gateway/tests/test_checkpoint_handler.py:1067-1134) — closes non-blocking #2 from the prior review.
Traced execution:
_branch_exists=Falsedrives the orphan path atcheckpoint_handler.py:968-992.branch -Dat line 980-983 runs insidewith bare_repo_lock(repo_path)(line 978), so depth = 1. ✓worktree add --detachat line 984-987 runs in the samewithblock, depth = 1. ✓checkout --orphanat line 988-991 runs after thewithblock exits, depth = 0. ✓
Both regression directions called out in the prior review are caught:
- A regression that hoisted
branch -Dabove line 978 →branch -Dat depth 0 → assertion fails. - A regression that nested
checkout --orphaninside the flock →checkout --orphanat depth 1 → assertion fails.
Test design
Both tests use monkeypatch.setattr(checkpoint_handler, "bare_repo_lock", recording_flock), which overrides the autouse _stub_bare_repo_lock no-op (test_checkpoint_handler.py:28-50) — this works because monkeypatch's teardown order undoes the per-test patch first; the autouse no-op is only relevant for tests that don't override it. The recording stand-in is a thin context manager that increments/decrements flock_depth[0] — straightforward and free of accidental nesting.
The tests exercise the production code path (handler.store_checkpoint_v2(...)), not a hand-built fixture. The _run_git test seam is the same one used by the sibling tests in TestStoreCheckpointV2Concurrency. Both tests pass locally (pytest gateway/tests/test_checkpoint_handler.py — 86 passed).
Non-blocking
1. Regenerate-path fetch test could pin the count exactly
test_bare_repo_lock_not_held_across_regenerate_fetch asserts len(fetch_observations) >= 2. Given track_run_git only fails pushes (never fetches), the run produces exactly one initial fetch + one regenerate fetch. Tightening to len(fetch_observations) == 2 would catch a regression where someone added a redundant fetch inside the flock — the looser >= 2 would mask it because the extra fetch would slip past as long as every other fetch was at depth 0. Minor.
2. Orphan-path test doesn't assert ordering between branch -D and worktree add
The test confirms both run at depth ≥ 1 but doesn't check that branch -D runs before worktree add. A regression that swapped them — running worktree add against a still-existing branch — would fail at the git layer in production but pass this test. Not a real concern given _run_git is mocked, but the existing-path concurrent-stores test's pattern of checking ordering via observations.index(...) would be a stronger shape.
3. PR description signature drift (carried over)
Still applies: an older james-in-a-box[bot] "Conflict Resolution Summary" comment references _get_store_lock(key, repo_path). The implementation is single-arg _get_store_lock(key) at checkpoint_handler.py:1402. The PR body is current; only the historical bot comment is stale.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…n-path test Addresses non-blocking review feedback on PR #2328: - test_bare_repo_lock_not_held_across_regenerate_fetch: pin fetch count to exactly 2 (initial + regenerate-path) so a regression that adds a redundant fetch *inside* the flock — masked by other at-depth-0 fetches — is caught. Production code path runs exactly one initial fetch (non-failing) plus one regenerate fetch when the first push is rejected non-FF. - test_bare_repo_lock_wraps_branch_d_in_orphan_path: assert branch -D runs before worktree add in the orphan path. They share one flock window so the depth assertion alone would pass even if they were swapped, which would then fail at the git layer in production (worktree add against a still- existing branch). The new index ordering check guards against that swap regression.
|
Addressing the 3 non-blocking items from the latest review.
Verification:
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-reviewed the delta from 3e7e8ee7 → 42dc5f85. Single commit, 23+/4-, test-only — no production code changes. Both non-blocking items from the previous review are correctly addressed.
Verification
1. Fetch count pinned to exactly 2 — gateway/tests/test_checkpoint_handler.py:1060. Traced against the production path:
_branch_exists=True→ existing-branch path. Mockedtrack_run_gitonly fails pushes (never fetches), so the initial-fetch retry loop atcheckpoint_handler.py:920-951succeeds on attempt 1 → 1 fetch.- First push (
push_count==1) raisesCheckpointError("non-fast-forward")→ enters regenerate branch at line 1050-1106 → 1 regenerate fetch at line 1079-1084. - Second push (
push_count==2) succeeds → loop breaks.
Total fetches: exactly 2. The tightened == 2 assertion now catches a regression that adds a redundant fetch inside the flock — the prior >= 2 would have masked it because the at-depth-0 fetches around it would still satisfy the depth check. The companion len(push_calls) == 2 guard ensures the regenerate branch was actually reached, so a regression that skipped regeneration entirely (e.g. broke the non-FF detection at line 1051) would also fail.
2. branch -D ordering before worktree add — gateway/tests/test_checkpoint_handler.py:1131-1141. The assertion uses next(i for i, args in enumerate(observed_args) if args[:2] == ["branch", "-D"]) and the matching pattern for worktree add. Both next() calls are guarded by the prior non-empty assertions at lines 1112 and 1120 — if either disappears the test fails with StopIteration rather than passing silently.
The depth check alone (depth ≥ 1 for both) would pass under a swap because both still execute inside the same with bare_repo_lock window (checkpoint_handler.py:978-987). A swap would surface only at the git layer in production (worktree add against a still-existing branch). The new index ordering assertion closes that gap.
Non-blocking
None. The two prior non-blocking items are both addressed; nothing new to flag in this delta.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
|
The most recent review at commit There is no new actionable feedback in this round. For completeness, here is the running disposition for items raised across the review history (all closed in earlier commits on this PR): Round 1 (commit
Round 2 (commit Round 3 (commit
Round 4 (commit Round 5 (commit
Round 6 (commit No code changes pushed in this run. — Authored by egg |
|
egg feedback addressed. View run logs 22 previous review(s) hidden. |
1 participant
Summary
Closes #2316. Two checkpoint storage failures were firing on every pipeline run; only one was real data loss.
Failure 1 (push permission denied on a read-only source repo): orchestrator-internal helpers (ls-remote, failsafe-fetch) register short-lived sessions on the gateway. On deletion the gateway tried to capture a session-end checkpoint, but these helpers have no proxy buffer — the captured checkpoint was metadata-only. When the source repo is read-only, the push falls back to source origin and fails noisily at ERROR. Fix: add a
syntheticflag toSession, plumb it throughregister_sessionand the four temp-session helpers inorchestrator/gateway_client.py, and short-circuit_capture_and_cleanup_sessionfor synthetic sessions. No data lost — these checkpoints only carried synthetic container_id metadata.Failure 2 (non-FF rebase conflict on
egg/checkpoints/v2): when two writers from different source repos target the same sharedjwbron/egg-checkpointsbranch, the per-source-repo lock did not serialize them. The second writer got a non-FF rejection, hit the rebase retry, and failed with a content conflict onindex.json(structurally an append, textually a diff). After 3 attempts the checkpoint was dropped — real session-end transcripts and tool calls lost. Fix in two parts:checkpoint_repo or repo_path(renamed_store_locks/_get_store_lock) so cross-source-repo writers contending on the same destination serialize.index.jsonis regenerated from the new remote state plus this writer's summary, so concurrent appends never produce textual conflicts.Test plan
gateway/tests/test_checkpoint_handler.py— 117 passing, including:test_push_retries_on_non_fast_forwardto assert the regenerate flow (fetch + reset --hard + re-add + re-commit, no rebase).test_push_fails_when_rebase_in_retry_failswith a regenerate-commit-failure equivalent.test_concurrent_stores_with_shared_checkpoint_repo_serialized— two source repos targeting the samecheckpoint_reposerialize.gateway/tests/test_session_manager.py— 88 passing, including:test_register_synthetic_sessionandtest_synthetic_session_skips_checkpoint_capture.orchestrator/tests/test_gateway_client*— 98 passing.ruff checkclean on all touched files.