Skip to content

fix(core): fetch every branch when reusing a git clone - #3702

Merged
trunk-io[bot] merged 1 commit into
mainfrom
claude/laughing-brown-pb0j33-git-fetch-branches
Oct 8, 2026
Merged

trunk-io[bot] merged 1 commit into
mainfrom
claude/laughing-brown-pb0j33-git-fetch-branches

Conversation

@phil-opp

@phil-opp phil-opp commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Problem

When dora build reuses an existing git clone (ReuseOptions::CopyAndFetch / RenameAndFetch in libraries/core/src/build/git.rs), fetch_changes fetched only the remote's default branch. A fresh clone (NewClone) fetches every branch, so the two paths saw different commits.

For a node pinned to a non-default branch, the build fails:

- id: my-node
  git: https://example.com/org/repo
  branch: dev
  1. The first build clones the repo (all branches). This works.
  2. Someone pushes a new commit to dev, and you rebuild. The CLI resolves dev to the new hash. choose_clone_dir finds the previous clone of the same repo and picks RenameAndFetch (or CopyAndFetch).
  3. Only main is fetched, so the new dev commit is never downloaded, and checkout_tree fails with failed to parse ref.
  4. With RenameAndFetch, the previous clone has already been moved into the temp dir, and the failure cleanup deletes it.

rev: pins on commits that are only reachable from a non-default branch hit the same failure.

Fix

  • fetch_changes now calls remote.fetch(&[], ...). An empty refspec list makes libgit2 use the remote's configured refspecs (+refs/heads/*:refs/remotes/origin/*), so the reuse path sees the same commits as a fresh clone.
  • If the wanted commit is already in the clone, the fetch is skipped. Commit hashes are immutable, so this offsets the larger fetch and removes a network round-trip in the common case.
  • The refname parameter was always None, so it is removed.

Known edge case, not new to this PR: plain git fetch without --prune has the same limitation. If upstream deletes branch feature and creates feature/x, the stale refs/remotes/origin/feature in an old clone blocks the update ("cannot lock ref"). This only happens when the commit is missing locally, and it is much rarer than the failure fixed here.

Validation

Class: B (git build path in dora-core, not the descriptor)

  • New regression test build::git::tests::fetch_reuse_reaches_commits_on_non_default_branches. It covers both the Copy and Rename arms: a local file:// origin with a dev branch, a clone of it, then a new commit on dev.
    • Before the fix: called Result::unwrap() on an Err value: failed to parse ref.
    • After the fix: passes.
  • cargo test -p dora-core --features build --lib build::git: 24 passed.
  • cargo test -p dora-core --all-features: all pass except topics::tests::unreachable_peer_teardown_bounded. That test also fails on unmodified main in this sandbox, which has no IPv6 (tcp/[::]:0: address family not supported).
  • cargo clippy -p dora-core --all-targets --all-features -- -D warnings: clean. cargo fmt --check: clean.
  • The workspace-wide gates ran once on the combined diff of this run's PRs (see the summary comment).

🤖 Machine-generated PR. Opened by an automated Claude Code review run: Claude found the issue, wrote the fix, and ran the validation below. A human maintainer has not reviewed it yet; please review it like any other external contribution.

🤖 Generated with Claude Code

https://claude.ai/code/session_01D6z9PTSqfQtU5GZJVR2oYV


Generated by Claude Code

Updating an existing clone (`CopyAndFetch` / `RenameAndFetch`) fetched
only the remote's default branch, while a fresh clone fetches all of
them. For a node pinned with `branch: dev` (or a `rev:` that is only
reachable from a non-default branch), the commit to check out was never
downloaded, so the rebuild after a new push to that branch failed with
"failed to parse ref" -- and `RenameAndFetch` had already moved the
previous clone into a temp dir that the failure cleanup then deleted.

Fetch with the remote's configured refspecs instead, so the reuse path
sees the same commits as a new clone. Since that fetches more than
before, skip the fetch entirely when the wanted commit is already in the
clone (commit hashes are immutable). The `refname` parameter was always
`None` and is dropped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D6z9PTSqfQtU5GZJVR2oYV
@trunk-io

trunk-io Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

😎 Merged successfully - details.

phil-opp commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Automated codebase review run: summary (machine-generated by Claude Code; no human has reviewed these findings)

I scanned main @ 86327c1 for correctness, optimization, documentation and simplification issues. Five review agents covered the less-mined crates:

  • dora-core;
  • dora-message, arrow-convert, the C API and recording;
  • the Python and operator runtimes;
  • mavlink, download, coordinator-store, hub-client and the ros2-bridge-node;
  • the Rust and C++ node API.

I verified every finding against origin/main myself and checked the open PRs for duplicates.

PRs opened (5)

Correctness

Correctness / security hygiene (template)

Optimization / Documentation / Simplification: no standalone PR met the bar. Simplifications found while doing the above went into those PRs: the unused refname parameter was removed, the ros2 result timeout became one helper, and the Python node construction became Node::from_parts.

Validation on the combined diff:

  • fmt, workspace clippy -D warnings, cargo check --examples and make qa-test-python are green, and qa-breaking --fast reports no breaking changes.
  • cargo test --all passes apart from 21 zenoh-session tests. Those fail with tcp/[::]:0: Address family not supported because the sandbox has no IPv6, and the same tests fail on main there.

Rejected or deferred findings

  • Duplicate of fix: six independent correctness fixes (C/C++ APIs, record, ROS2 bridge, tensor-pool, type registry) #3660:
    • types/std.yml overriding the reserved std/ namespace.
    • ros2 run_multi_subscriber hanging on Stop when a topic is quiet.
    • free_dora_event(NULL) / free_dora_context(NULL) UB.
  • Duplicate of perf(node-api): don't ship a sliced array's whole parent through the IPC fast path #3653: the IPC fast path ships a sliced Utf8/List array's whole parent buffer.
  • Zenoh overlay rejects mode-dependent endpoints: {peer: [...]}: this is a deliberate limitation with a clear error message, not a bug.
  • DDS action client feedback/result threads outlive their goal and steal other goals' messages: plausible from reading ros2-client, but it needs a design change (a single demultiplexer) and a live ROS 2 setup to validate. Left for a focused issue.
  • SHM allocate for daemon-only outputs (encode to SHM, then copy out): a design-level perf change to SampleAllocator. Not a low-hanging fix.
  • Per-message schema re-hash and metadata clone in publish_schema_once: micro-optimization, unmeasured. Below the bar.
  • EventStream::is_empty() stays false after Stop: medium confidence, and only matters for an unusual polling pattern.
  • start_runtime holds the GIL for the runtime's lifetime: plausible starvation of Python background threads in operators, but unverified without a Python E2E run. Left for an issue.
  • Python try_recv swallows conversion errors (.ok()): real but low impact. It would change the API's failure mode.
  • runtime-api per-input OperatorId/DataId allocation: minor hot-path cost. Below the bar.
  • dora-message byte fields not using bulk_bytes: wire-compatible, but the payloads are small control messages. Below the bar.
  • mavlink UDP wake ignored with proto=v1/IPv6: costs only a 500 ms shutdown delay.
  • C API docs stale (non-UInt8 input "panics"; timestamp units): valid but minor docs. fix: use valid CLI flags in docs, smoke tests and scripts #3664 and docs: fix stale c++-dataflow README (#3637) #3665 are already working through doc accuracy.

Generated by Claude Code

@phil-opp phil-opp mentioned this pull request Oct 5, 2026

phil-opp commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review (fully automated Claude Code review; no human checked it)

I found no issues.

  • The fix: with an empty refspec list, libgit2 uses the remote's configured refspecs. For a clone that dora made, that is +refs/heads/*:refs/remotes/origin/* plus all tags, so CopyAndFetch/RenameAndFetch now reach the same commits a fresh clone_into would.
  • Skipping the fetch when the commit is already present is safe: clone_dir_path already limits commit_hash to hex, and a commit hash cannot point to different content later.
  • The test covers both reuse arms. I checked by reading that it fails on main: only the default branch is fetched there, so checkout_tree can't reach the dev commit. The build::git tests pass on the PR head.

Generated by Claude Code

@trunk-io
trunk-io Bot merged commit d7b644f into main Oct 8, 2026
19 checks passed
@trunk-io
trunk-io Bot deleted the claude/laughing-brown-pb0j33-git-fetch-branches branch October 8, 2026 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants