-
Notifications
You must be signed in to change notification settings - Fork 0
feat(daemon): coalesce workspace.toml auto-sync commits via push cooldown #47
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
Changes from all commits
fa23da6
9380fd5
70e35fc
19c5ca9
7d8ea67
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 |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| package daemon_test | ||
|
|
||
| import ( | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/kuchmenko/workspace/internal/daemon" | ||
| ) | ||
|
|
||
| func TestResolvedPushCooldown(t *testing.T) { | ||
| cases := []struct { | ||
| name string | ||
| in string | ||
| want time.Duration | ||
| }{ | ||
| // Empty → default. The common case: a workspace entry written before | ||
| // push_cooldown existed must get the new coalescing behavior for free. | ||
| {"unset", "", daemon.DefaultPushCooldown}, | ||
|
|
||
| // Literal "0" disables coalescing. time.ParseDuration rejects a bare | ||
| // "0" (no unit) so this is the dedicated short-circuit — the | ||
| // regression that motivated this test. | ||
| {"bare zero", "0", 0}, | ||
| {"zero seconds", "0s", 0}, | ||
|
|
||
| {"valid", "30m", 30 * time.Minute}, | ||
| {"valid hours", "2h", 2 * time.Hour}, | ||
|
|
||
| // Garbage falls back to the default rather than silently dropping | ||
| // the daemon to no-coalesce. Surprises here would be hard to debug | ||
| // (the symptom is just "history is noisy again"). | ||
| {"garbage", "not-a-duration", daemon.DefaultPushCooldown}, | ||
| } | ||
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| got := daemon.WorkspaceEntry{PushCooldown: tc.in}.ResolvedPushCooldown() | ||
| if got != tc.want { | ||
| t.Fatalf("ResolvedPushCooldown(%q) = %s, want %s", tc.in, got, tc.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import ( | |
| "encoding/json" | ||
| "fmt" | ||
| "path/filepath" | ||
| "time" | ||
|
|
||
| "github.com/kuchmenko/workspace/internal/conflict" | ||
| "github.com/kuchmenko/workspace/internal/git" | ||
|
|
@@ -63,17 +64,37 @@ func (r *Reconciler) syncTOML() (bool, error) { | |
| } | ||
|
|
||
| // Commit dirty changes first so the rest of the matrix only deals with | ||
| // committed state. | ||
| // committed state. When HEAD is already an unpushed auto-sync commit from | ||
| // this host, amend into it instead of stacking another one — see the | ||
| // pushCooldown design note in reconciler.go. | ||
| autoSyncMsg := fmt.Sprintf("ws: auto-sync workspace.toml from %s", machineHostname()) | ||
| if localDirty { | ||
| if err := git.Add(repoRoot, relFile); err != nil { | ||
| return false, fmt.Errorf("git add: %w", err) | ||
| } | ||
| host := machineHostname() | ||
| msg := fmt.Sprintf("ws: auto-sync workspace.toml from %s", host) | ||
| if err := git.Commit(repoRoot, msg); err != nil { | ||
| return false, fmt.Errorf("git commit: %w", err) | ||
| headMsg, _ := git.LastCommitMessage(repoRoot) | ||
| if ahead > 0 && headMsg == autoSyncMsg { | ||
| // If the staged tree now matches HEAD's parent, the held commit's | ||
| // net change has been undone (e.g. a favorite toggled on then off | ||
| // inside the cooldown). git refuses an amend that produces an | ||
| // empty diff vs parent; without this branch we'd return an error | ||
| // every subsequent tick and leave workspace.toml staged forever. | ||
| // Drop the held commit instead — the right history outcome is | ||
| // "no commit at all". | ||
| if err := runIn(repoRoot, "git", "diff", "--cached", "--quiet", "HEAD~1"); err == nil { | ||
| if err := runIn(repoRoot, "git", "reset", "--mixed", "HEAD~1"); err != nil { | ||
| return false, fmt.Errorf("drop empty held auto-sync: %w", err) | ||
| } | ||
| ahead-- | ||
| } else if err := runIn(repoRoot, "git", "commit", "--amend", "--no-edit"); err != nil { | ||
| return false, fmt.Errorf("git commit --amend: %w", err) | ||
|
Comment on lines
+76
to
+90
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.
When a held auto-sync edit is undone before the cooldown expires (for example toggling a favorite on and then off), this amend attempts to make HEAD's tree identical to its parent. Git rejects that case with Useful? React with 👍 / 👎. |
||
| } | ||
| } else { | ||
| if err := git.Commit(repoRoot, autoSyncMsg); err != nil { | ||
| return false, fmt.Errorf("git commit: %w", err) | ||
| } | ||
| ahead++ | ||
| } | ||
| ahead++ | ||
| } | ||
|
|
||
| // Re-evaluate behind in case fetch happened pre-commit. | ||
|
|
@@ -88,9 +109,14 @@ func (r *Reconciler) syncTOML() (bool, error) { | |
| _ = r.clearTOMLConflicts() | ||
| } | ||
|
|
||
| // Push if anything to push. | ||
| // Push if anything to push — unless the pushCooldown gate is holding our | ||
| // auto-sync commit open for further amending. The held commit will be | ||
| // pushed on a later tick once its age exceeds the cooldown, or sooner if | ||
| // a non-auto-sync commit lands on top of it. | ||
| if ahead > 0 || behind > 0 { | ||
| if err := git.Push(repoRoot); err != nil { | ||
| if r.shouldHoldPush(repoRoot, autoSyncMsg, ahead) { | ||
| r.logger.Printf("reconciler: %s holding auto-sync commit for amend (cooldown %s)", repoRoot, r.pushCooldown) | ||
| } else if err := git.Push(repoRoot); err != nil { | ||
| // One retry: fetch + rebase + push, mirror of the legacy syncer. | ||
| if perr := runIn(repoRoot, "git", "pull", "--rebase"); perr != nil { | ||
| r.recordTOMLConflict(repoRoot, conflict.KindTOMLMerge, perr) | ||
|
|
@@ -107,6 +133,38 @@ func (r *Reconciler) syncTOML() (bool, error) { | |
| return newHead != originalHead, nil | ||
| } | ||
|
|
||
| // shouldHoldPush reports whether HEAD is our own auto-sync commit that is | ||
| // still young enough to absorb further amends. Zero pushCooldown disables | ||
| // the gate entirely (the historical behavior, kept for `ws sync`). | ||
| // | ||
| // The age check uses the author date — preserved by `git commit --amend | ||
| // --no-edit` — so continuous activity that keeps amending into the held | ||
| // commit cannot indefinitely defer the push. The committer date would | ||
| // refresh on every amend and silently turn the cooldown into "never push | ||
| // while busy", which is the failure mode this gate exists to prevent. | ||
| // | ||
| // The ahead==1 guard prevents the gate from withholding a user's manual | ||
| // commit that sits below the auto-sync: in that case `git push` would | ||
| // publish *both* commits, and the cooldown is only entitled to defer the | ||
| // auto-sync one. When ahead > 1 we always push. | ||
| func (r *Reconciler) shouldHoldPush(repoRoot, autoSyncMsg string, ahead int) bool { | ||
| if r.pushCooldown <= 0 { | ||
| return false | ||
| } | ||
| if ahead != 1 { | ||
| return false | ||
| } | ||
| headMsg, _ := git.LastCommitMessage(repoRoot) | ||
| if headMsg != autoSyncMsg { | ||
| return false | ||
| } | ||
| t, err := git.LastCommitAuthorTime(repoRoot) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return time.Since(t) < r.pushCooldown | ||
| } | ||
|
|
||
| func (r *Reconciler) recordTOMLConflict(workspace string, kind conflict.Kind, cause error) { | ||
| if r.store == nil { | ||
| return | ||
|
|
||
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.
The new config comments document
push_cooldown = "0"as disabling coalescing, buttime.ParseDuration("0")fails because it has no unit, so this falls into the error branch and silently re-enables the default one-hour cooldown. A user trying to restore immediate daemon pushes with the documented value will still have auto-sync commits deferred; handle the literal"0"before parsing or document/use"0s".Useful? React with 👍 / 👎.