Skip to content

Commit d6b0067

Browse files
committed
avoid holding locks during Export
Signed-off-by: Connor Braa <connor@dagger.io>
1 parent 57d84de commit d6b0067

File tree

2 files changed

+42
-29
lines changed

2 files changed

+42
-29
lines changed

repository/git.go

Lines changed: 32 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,7 @@ func (r *Repository) propagateToWorktree(ctx context.Context, env *environment.E
181181
"err", rerr)
182182
}()
183183

184+
// Export environment without holding any git locks - this only writes to filesystem
184185
if err := r.exportEnvironment(ctx, env); err != nil {
185186
return err
186187
}
@@ -189,23 +190,31 @@ func (r *Repository) propagateToWorktree(ctx context.Context, env *environment.E
189190
if err != nil {
190191
return fmt.Errorf("failed to get worktree path: %w", err)
191192
}
193+
192194
// Protect commitWorktreeChanges to prevent concurrent writes to .git/worktrees/*/logs/HEAD
193195
if err := r.lockManager.WithLock(ctx, LockTypeWorktree, func() error {
194196
return r.commitWorktreeChanges(ctx, worktreePath, explanation)
195197
}); err != nil {
196198
return fmt.Errorf("failed to commit worktree changes: %w", err)
197199
}
198200

199-
if err := r.saveState(ctx, env); err != nil {
200-
return fmt.Errorf("failed to add notes: %w", err)
201-
}
201+
// Use git notes lock for all git notes operations
202+
if err := r.lockManager.WithLock(ctx, LockTypeGitNotes, func() error {
203+
if err := r.saveState(ctx, env); err != nil {
204+
return fmt.Errorf("failed to add notes: %w", err)
205+
}
202206

203-
slog.Info("Fetching container-use remote in source repository")
204-
if _, err := RunGitCommand(ctx, r.userRepoPath, "fetch", containerUseRemote, env.ID); err != nil {
205-
return err
206-
}
207+
slog.Info("Fetching container-use remote in source repository")
208+
if _, err := RunGitCommand(ctx, r.userRepoPath, "fetch", containerUseRemote, env.ID); err != nil {
209+
return err
210+
}
207211

208-
if err := r.propagateGitNotes(ctx, gitNotesStateRef); err != nil {
212+
if err := r.propagateGitNotesUnlocked(ctx, gitNotesStateRef); err != nil {
213+
return err
214+
}
215+
216+
return nil
217+
}); err != nil {
209218
return err
210219
}
211220

@@ -234,23 +243,27 @@ func (r *Repository) exportEnvironment(ctx context.Context, env *environment.Env
234243
return nil
235244
}
236245
func (r *Repository) propagateGitNotes(ctx context.Context, ref string) error {
246+
return r.lockManager.WithLock(ctx, LockTypeGitNotes, func() error {
247+
return r.propagateGitNotesUnlocked(ctx, ref)
248+
})
249+
}
250+
251+
func (r *Repository) propagateGitNotesUnlocked(ctx context.Context, ref string) error {
237252
fullRef := fmt.Sprintf("refs/notes/%s", ref)
238253
fetch := func() error {
239254
_, err := RunGitCommand(ctx, r.userRepoPath, "fetch", containerUseRemote, fullRef+":"+fullRef)
240255
return err
241256
}
242257

243-
return r.lockManager.WithLock(ctx, LockTypeGitNotes, func() error {
244-
if err := fetch(); err != nil {
245-
if strings.Contains(err.Error(), "[rejected]") {
246-
if _, err := RunGitCommand(ctx, r.userRepoPath, "update-ref", "-d", fullRef); err == nil {
247-
return fetch()
248-
}
258+
if err := fetch(); err != nil {
259+
if strings.Contains(err.Error(), "[rejected]") {
260+
if _, err := RunGitCommand(ctx, r.userRepoPath, "update-ref", "-d", fullRef); err == nil {
261+
return fetch()
249262
}
250-
return err
251263
}
252-
return nil
253-
})
264+
return err
265+
}
266+
return nil
254267
}
255268

256269
func (r *Repository) saveState(ctx context.Context, env *environment.Environment) error {
@@ -272,10 +285,8 @@ func (r *Repository) saveState(ctx context.Context, env *environment.Environment
272285
return err
273286
}
274287

275-
return r.lockManager.WithLock(ctx, LockTypeGitNotes, func() error {
276-
_, err = RunGitCommand(ctx, worktreePath, "notes", "--ref", gitNotesStateRef, "add", "-f", "-F", f.Name())
277-
return err
278-
})
288+
_, err = RunGitCommand(ctx, worktreePath, "notes", "--ref", gitNotesStateRef, "add", "-f", "-F", f.Name())
289+
return err
279290
}
280291

281292
func (r *Repository) loadState(ctx context.Context, worktreePath string) ([]byte, error) {

repository/repository.go

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -359,15 +359,17 @@ func (r *Repository) isDescendantOfCommit(ctx context.Context, ancestorCommit, e
359359
// Update saves the provided environment to the repository.
360360
// Writes configuration and source code changes to the worktree and history + state to git notes.
361361
func (r *Repository) Update(ctx context.Context, env *environment.Environment, explanation string) error {
362-
return r.lockManager.WithLock(ctx, LockTypeGitNotes, func() error {
363-
if err := r.propagateToWorktree(ctx, env, explanation); err != nil {
364-
return err
365-
}
366-
if note := env.Notes.Pop(); note != "" {
362+
if err := r.propagateToWorktree(ctx, env, explanation); err != nil {
363+
return err
364+
}
365+
366+
// Handle git notes outside the main propagation to avoid holding locks during export
367+
if note := env.Notes.Pop(); note != "" {
368+
return r.lockManager.WithLock(ctx, LockTypeGitNotes, func() error {
367369
return r.addGitNote(ctx, env, note)
368-
}
369-
return nil
370-
})
370+
})
371+
}
372+
return nil
371373
}
372374

373375
// Delete removes an environment from the repository.

0 commit comments

Comments
 (0)