-
Notifications
You must be signed in to change notification settings - Fork 0
Update testing-git-remote skill with preview perf-testing lessons #72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
b623bab
5fb77b3
3d42756
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -81,11 +81,59 @@ curl -s -o /dev/null -w "%{http_code}" "http://127.0.0.1:8080/git/evil.com/org/r | |||||||||||||||||
| curl -s -o /dev/null -w "%{http_code}" "http://127.0.0.1:8080/git/github.com/octocat/Hello-World.git/info/refs?service=git-receive-pack" | ||||||||||||||||||
| ``` | ||||||||||||||||||
|
|
||||||||||||||||||
| ## Cold-Cache Perf Testing on an AWS Preview Stack | ||||||||||||||||||
|
|
||||||||||||||||||
| A fresh preview stack (`scripts/aws/deploy-preview.sh <ref>`) guarantees a cold EBS | ||||||||||||||||||
| cache (isolated stack + S3 prefix), making it the right environment for read-through | ||||||||||||||||||
| perf measurements. | ||||||||||||||||||
|
|
||||||||||||||||||
| - **Opt out of proxy-on-miss or you measure the proxy, not the cache.** Since | ||||||||||||||||||
| proxy-on-miss became the default, read-through benchmarks must send a falsey header: | ||||||||||||||||||
| ```bash | ||||||||||||||||||
| git clone -c http.extraHeader="git-cache-use-proxy-on-miss: 0" \ | ||||||||||||||||||
| http://<ALB>/v/<VERSION_ID>/git/github.com/<owner>/<repo>.git | ||||||||||||||||||
| ``` | ||||||||||||||||||
| - **Time the clone yourself** (`S=$(date +%s.%N); git clone ...; date +%s.%N`). | ||||||||||||||||||
| `/usr/bin/time` may not be installed. | ||||||||||||||||||
| - **Assert on server logs, not just wall time.** Tail CloudWatch | ||||||||||||||||||
| (`aws logs tail /ecs/gmc-p-<VERSION_ID>/ec2-api --region us-west-2 --since 20m --format short`) | ||||||||||||||||||
| and check: | ||||||||||||||||||
| - one `direct git batched read-through fetch for wanted commits` line with | ||||||||||||||||||
| `refspec_count` ≈ the repo's advertised ref count (700+ for astral repos); | ||||||||||||||||||
| - upstream `git command finished args=["fetch", ...]` lines: should be ≤ 3 per cold | ||||||||||||||||||
| clone, not one per want; | ||||||||||||||||||
| - exactly one `queued direct git background fsck` per upload-pack request. | ||||||||||||||||||
| - **Reference numbers** (m8g.2xlarge preview, full clone of astral-sh/ruff, ~727 refs): | ||||||||||||||||||
| batched cold ≈ 37s, warm ≈ 9s, GitHub direct ≈ 9s. If cold is in the hundreds of | ||||||||||||||||||
| seconds, the per-want fetch storm may have regressed. | ||||||||||||||||||
| - A fresh `git clone` wants the tip of EVERY advertised ref, so many-ref repos | ||||||||||||||||||
| (astral-sh/ruff, astral-sh/uv, llvm) are the right stress tests; few-ref repos | ||||||||||||||||||
| (octocat/Hello-World) won't exercise batching. | ||||||||||||||||||
|
|
||||||||||||||||||
| ### Cross-build + preview deploy gotchas | ||||||||||||||||||
|
|
||||||||||||||||||
| - `scripts/aws/build-image-cross.sh` tags the ECR image with the SHORT commit sha, | ||||||||||||||||||
| but `deploy-preview.sh` defaults `IMAGE_TAG` to the 12-char VERSION_ID. To reuse a | ||||||||||||||||||
| cross-built image, pass it explicitly: | ||||||||||||||||||
| ```bash | ||||||||||||||||||
| IMAGE_TAG=$(git rev-parse --short HEAD) ECS_SKIP_DOCKER_BUILD_IF_IMAGE_EXISTS=true \ | ||||||||||||||||||
| AWS_REGION=us-west-2 scripts/aws/deploy-preview.sh $(git rev-parse HEAD) | ||||||||||||||||||
| ``` | ||||||||||||||||||
| Otherwise the deploy falls back to a slow QEMU docker build on the box. | ||||||||||||||||||
| - The cross build needs `gcc-aarch64-linux-gnu`; if `apt-get install` 404s, run | ||||||||||||||||||
| `sudo apt-get update` first (stale package index). | ||||||||||||||||||
| - Export AWS creds in EVERY shell that runs AWS scripts (they don't inherit across | ||||||||||||||||||
| shell tabs). | ||||||||||||||||||
| - A killed/partial preview deploy still leaves billable resources (EC2 instance, ALB | ||||||||||||||||||
| rule); clean up with `VERSION_ID=<id> scripts/aws/destroy-preview.sh`. | ||||||||||||||||||
|
|
||||||||||||||||||
| ## Known Limitations | ||||||||||||||||||
|
|
||||||||||||||||||
| - **Large repos may timeout on first access**: The default `git_timeout_seconds = 120` might not be enough for the initial full fetch of very large repos (e.g., astral-sh/uv). The Python integration tests use `--depth 1` shallow clones to handle this. Use small repos like `octocat/Hello-World` for quick smoke tests. | ||||||||||||||||||
| - **`upstream_root` vs real GitHub**: With `upstream_root` set (as in `local.example.toml`), the server uses local filesystem paths as upstreams. Remove it to proxy to real GitHub. | ||||||||||||||||||
| - **Integration tests use `multi_thread` tokio runtime**: The Rust integration tests require `#[tokio::test(flavor = "multi_thread")]` because single-threaded runtime deadlocks when blocking git CLI calls starve the Axum server. | ||||||||||||||||||
| - **Very large repos (llvm-scale) may still hit the server 1h git timeout** during | ||||||||||||||||||
| pack-objects on full-ref blobless clones; this is independent of fetch batching. | ||||||||||||||||||
|
|
||||||||||||||||||
| ## What to Verify After Validation Changes | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -95,7 +143,10 @@ When modifying input validators (`reject_remote_url`, `reject_refspec`, `reject_ | |||||||||||||||||
| 2. Start live server with `RUST_LOG=debug` and verify git command args in logs show `--` separators | ||||||||||||||||||
| 3. Verify `git clone` through the proxy still works (validators not too strict) | ||||||||||||||||||
| 4. Verify disallowed hosts return 4xx (validators not too loose) | ||||||||||||||||||
| 5. Upstream-derived strings (e.g. branch names from ls-remote) interpolated into | ||||||||||||||||||
| refspecs must be validated per-component (e.g. `branch_cache_refspec`) — `reject_refspec` | ||||||||||||||||||
| alone allows `:` because refspecs legitimately contain it | ||||||||||||||||||
|
|
||||||||||||||||||
| ## Secrets Needed | ||||||||||||||||||
| ## Devin Secrets Needed | ||||||||||||||||||
|
|
||||||||||||||||||
| None — all testing uses public GitHub repos and local infrastructure. | ||||||||||||||||||
| - `AWS_Access_Key` — only for live preview-stack testing (format: `ACCESS_KEY_ID,SECRET_ACCESS_KEY` on the last line). Local testing needs no secrets. | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 AWS secret listed in AGENTS.md lines 10-12 state that
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 5fb77b3 — reverted to "Secrets Needed: None" with a pointer to |
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 AWS preview-stack instructions placed in
.agents/skills/violate AGENTS.md local-only runbook ruleAGENTS.md lines 10-12 mandate that runbooks under
.agents/skills/"require no secrets or live-infrastructure access and should be runnable by any agent with the local repo, Rust toolchain, and Git." Content requiring AWS credentials, live infrastructure (ECR, CloudWatch, ECS, ALB), and preview-stack deployment belongs under.devin/skills/per AGENTS.md lines 13-15. The new "Cold-Cache Perf Testing on an AWS Preview Stack" section and "Cross-build + preview deploy gotchas" subsection referencescripts/aws/deploy-preview.sh,aws logs tail, ECR image management, andscripts/aws/destroy-preview.sh— all requiring live AWS access and credentials. This content should live in.devin/skills/gitmirrorcache-deploy/SKILL.md(which already owns AWS deployment procedures) or a new.devin/skills/runbook, not in.agents/skills/testing-git-remote/SKILL.md. The file's own header at line 9 still claims "no secrets; no live-infrastructure access," creating an internal contradiction.Prompt for agents
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 5fb77b3. The AWS-specific content (preview deploy, CloudWatch log tailing, IMAGE_TAG mismatch, cross-build prereqs, teardown) moved to
.devin/skills/gitmirrorcache-deploy/SKILL.mdunder the preview-deployments section. The.agentsrunbook keeps only the infrastructure-agnostic perf guidance (proxy opt-out header, expected log lines, many-ref repo selection) plus a cross-reference to the privileged runbook, restoring the local-only contract.