Skip to content

Fix warm cache symlink merges - #52

Merged
crohr merged 2 commits into
mainfrom
auto/fix-warm-cache-symlink-merge
Aug 3, 2026
Merged

Fix warm cache symlink merges#52
crohr merged 2 commits into
mainfrom
auto/fix-warm-cache-symlink-merge

Conversation

@crohr

@crohr crohr commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes the unresolved P1 review feedback from #43 (discussion_r3702181296).

Warm user-owned cache merges remove colliding destination symlinks and entry-type conflicts before copying. Root-owned apt merges build the same conflict view through sudo find, so unreadable apt directories and stale file/directory layouts are both handled. Linux keeps GNU cp --remove-destination protection for changes between conflict cleanup and copy.

Adds regressions for writable external file symlinks, file/directory layout changes, directory symlinks, and unreadable root-owned apt contents. Regenerates the shipped Linux amd64, Linux arm64, and Windows amd64 binaries.

Validation

  • go test ./...
  • go test -race ./internal/stickydisk
  • go vet ./internal/stickydisk
  • GOOS=windows GOARCH=amd64 go test -c ./internal/stickydisk
  • upx -t main-linux-amd64 main-linux-arm64 main-windows-amd64.exe
  • git diff --check

@crohr
crohr force-pushed the auto/fix-warm-cache-symlink-merge branch from 8b2aac2 to b35c006 Compare August 3, 2026 10:03
@chatgpt-codex-connector

Copy link
Copy Markdown

💡 Codex Review

copyArgs := []string{"cp", "-a"}
if runtime.GOOS == "linux" {
copyArgs = append(copyArgs, "--remove-destination")

P2 Badge Replace restored directories before copying non-directories

When a warm cache contains a directory at a path that is now a symlink or regular file in the current target, GNU cp -a --remove-destination still exits with cannot overwrite directory ... with non-directory; the flag removes destination files but does not replace directories. In the Linux cache workflow this makes mergeTargetIntoCache fail, after which cacheMount deletes the entire warm source and skips mounting it. Explicitly remove every colliding destination entry, including directories, before copying so current cache layouts can replace stale directory layouts.


func restoreAptAt(action *githubactions.Action, configDir string) error {
if err := runLogged(action, "sudo", "rm", "-f", "--", filepath.Join(configDir, aptKeepArchivesName)); err != nil {
return err

P2 Badge Restore apt configuration only after successful setup

When sticky_cache: apt is requested but Configure fails before configureApt runs—for example because the sticky disk is absent or readiness times out—the action's post step still dispatches restoreApt based solely on the input. This unconditional removal then deletes any pre-existing /etc/apt/apt.conf.d/99runs-on-keep-archives file even though this invocation never created it. Save setup ownership in action state and remove the file only when apt setup actually installed it.


log.Info("git proxy shutting down")
shutdownCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
return srv.Shutdown(shutdownCtx)

P2 Badge Terminate active proxy children when graceful shutdown expires

If a Git request is still active when the post step stops the proxy, Shutdown returns after this 10-second deadline and the proxy process exits while the request's git http-backend, clone, or fetch subprocess can remain orphaned in the detached process group. The parent cleanup sees the proxy PID disappear before its 12-second escalation deadline and therefore never sends the process-group SIGKILL, allowing the child to keep writing to the sticky disk while it is snapshotted. On shutdown timeout, explicitly close active handlers and terminate or join every Git child before allowing the proxy process to exit.


substitute := !isReceivePack(t, r)
if substitute && strings.Contains(t.rest, "/info/lfs/") {
substitute = false
if strings.HasSuffix(t.rest, "/info/lfs/objects/batch") && r.Header.Get("Content-Encoding") == "" {

P2 Badge Substitute credentials for read-only LFS endpoints

When a Git LFS client sends a read-only request such as GET .../info/lfs/locks through the rewritten GitHub remote, its proxy-scoped Authorization header contains the opaque client token and must be exchanged for the upstream token. This branch disables substitution for every LFS path except a parseable download batch, so lock listing and other authenticated read-only LFS endpoints forward the opaque token to GitHub and receive an authentication failure. Classify LFS requests by operation or HTTP method so read-only endpoints use upstreamAuth while uploads, lock creation, and unlocks retain the caller's own credential.

ℹ️ 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".

@crohr
crohr force-pushed the auto/fix-warm-cache-symlink-merge branch from b35c006 to ba29475 Compare August 3, 2026 12:16

crohr commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ba29475e54

ℹ️ 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".

Comment thread internal/stickydisk/mount_unix.go Outdated
@crohr
crohr force-pushed the auto/fix-warm-cache-symlink-merge branch from ba29475 to bb73f35 Compare August 3, 2026 12:27

crohr commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: bb73f35ece

ℹ️ 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".

@crohr
crohr merged commit d5c5d9f into main Aug 3, 2026
9 checks passed
@crohr
crohr deleted the auto/fix-warm-cache-symlink-merge branch August 3, 2026 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant