revert: Remove remote Git mirror changes - #4173
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf01df6d9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if strings.HasPrefix(envar, "BUILDKITE") || strings.HasPrefix(envar, "CI") || strings.HasPrefix(envar, "PATH") { | ||
| e.shell.Printf("%s", strings.ReplaceAll(envar, "\n", "\\n")) |
There was a problem hiding this comment.
Preserve mirror-URL credential redaction in debug logs
When a job still contains BUILDKITE_GIT_REMOTE_MIRROR_URL—for example during a staged control-plane rollback or when a pipeline sets it—createEnvironment copies it through unchanged, and this generic debug branch now writes the complete credentialed URL to the uploaded job log. Removing mirror checkout support does not make the environment value safe to print; retain the URL-credential redaction independently of the reverted feature.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
BUILDKITE_GIT_REMOTE_MIRROR_URL never contain credentials.
| helper := fmt.Sprintf(`%s git-credentials-helper`, self.Path(ctx)) | ||
| err = e.shell.Command("git", "config", "--global", "credential.helper", helper).Run(ctx, shell.ShowPrompt(false)) |
There was a problem hiding this comment.
Keep quoting the credential-helper executable path
When the agent executable is installed under a path containing spaces or shell metacharacters and an HTTPS repository requires Buildkite-provided credentials, this unquoted helper string is split by Git's shell invocation—for example, /opt/Buildkite Agent/buildkite-agent is attempted as /opt/Buildkite. The credential helper then cannot run and checkout fails, so the quoted leading-! helper construction should remain even though remote-mirror support is being reverted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I believe this was pre-existing (this is a revert PR) so not something we'll change in this PR.
|
I've merged this branch into |
There was a problem hiding this comment.
The revert patches line up with the five merge commits, and the control-plane OTLP changes remain intact. I have one question about rollout state persisted by v3.136.0.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 11658, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
About buildsworth
Model: gpt-5.6-sol with xhigh thinking.
How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.
Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. To let it approve, grant L2 approval by mentioning @buildsworth-bk (see L2 approval grant):
- L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
- L2 — Standard risk (new UI, additive API fields, refactors). Approved only with an L2 grant; otherwise comment-only.
- L3 — High risk (auth, migrations, payments, secrets, perf-critical paths). Human review always required.
| // flags — the caller decides based on sparse-checkout state and user-supplied | ||
| // filters. | ||
| func (e *Executor) fetchSource(ctx context.Context, addBloblessFilter bool, attempt *remoteMirrorAttempt) (retErr error) { | ||
| func (e *Executor) fetchSource(ctx context.Context, addBloblessFilter bool) (retErr error) { |
There was a problem hiding this comment.
Question: Did any v3.136.0 agents run mirror-enabled checkouts before this rollback? If a process died after starting a filtered mirror fetch, it could leave the buildkite.remote-mirror-promisor marker and a URL-named promisor section behind; #4162 intentionally repaired that state on the next fetch even after the mirror URL disappeared. Reverting this call and its helpers means later lazy object reads can still contact the removed mirror without the scoped credentials or timeout. Was the rollout constrained such that this on-disk state cannot exist?
There was a problem hiding this comment.
Still open — could you confirm whether any mirror-enabled v3.136.0 checkout could have left this marker/promisor state behind?
There was a problem hiding this comment.
Still open — the 3.136-stable note does not confirm whether an earlier v3.136.0 checkout could have persisted this state.
bf01df6 to
b0b6e5a
Compare
|
Rebased the revert branch onto |
There was a problem hiding this comment.
The rollout-state question remains open, and this revision also introduces a merge conflict with main in agent/job_runner.go.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 11664, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
| } | ||
| } | ||
|
|
||
| // When in KubernetesExec mode, filter out the Kubernetes plugin, |
There was a problem hiding this comment.
Blocking: This branch does not merge into current main: both sides edit this point in createEnvironment, and Git reports a content conflict in agent/job_runner.go. Please rebase onto main and resolve this hunk while retaining the control-plane OTLP destination block added by #4165; otherwise this PR cannot be merged as-is.
|
This is already merged into a stable branch and being released as https://github.com/buildkite/agent/releases/tag/v3.136.1 I'm thinking I'll close it here, and fix forwards on main prior to the next release instead. |
Description
Reverts the remote Git mirror rollout because local mirror reachability checking introduced a significant performance regression. This restores the checkout behavior from before the rollout while the regression is investigated separately.
The revert commits are based directly on
75683492d8057fe3eb0d4ac78ceaed2b1e1930a6, before the current tip ofmain.Context
Reverts:
Changes
Reverts the five merge commits in reverse merge order.
Testing
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go tool gofumpt -extra -w .)The affected package tests (
agent,internal/job,clicommand,env, andinternal/shell) pass after the rebase, and the agent builds successfully. The full test suite is deferred to keep the urgent revert moving.Affiliation (optional, external contributors)
Buildkite.
Disclosures / Credits
Cursor generated and applied the revert commits, including the rebase conflict resolution.