diff --git a/docs/TODO.md b/docs/TODO.md index 893a395b..af65918b 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -7,7 +7,13 @@ should land before the next tag is cut (the last release is v0.55.0). Checked ag ## Before the next release -Nothing open. +- [ ] **Test the failure boundaries of concurrent commit windows** (stage 3 of + [concurrent-author-windows.md](design/concurrent-author-windows.md#stages)). Stage 2 ships with no + flag: a branch worker keeps a window per author whenever their footprints are proven disjoint. + Before it is released, the reference model (`internal/git/window_model_test.go`) must cover both + close orders, conflicting writes, delete and recreate, `attach_next`, zero-duration windows, a + resync, shutdown, failed publication and replay, and buffer pressure, each with several windows + open. ## Applier claims and bi-directional editing diff --git a/docs/configuration.md b/docs/configuration.md index fd44a76d..3453bdd0 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1450,11 +1450,22 @@ editing them affects windows that open afterward, never one already running. A [`CommitRequest`](#the-request-window-specwindow) that attaches to a window replaces both timers for that window. -One branch worker holds one live window, bound to an author and target. Interleaved authors or -targets split a burst. Atomic writes, a resync, buffer limits, shutdown, an `attach: Next` request, -a push notice from the [receiver](#gitreceiver), and the -[flush](bi-directional.md#flush-then-apply) before an apply can close a window early. Push cooldown -is independent: a zero timer does not promise an immediate remote push. See the +A branch worker holds a window per author of one target, side by side, while it can prove their +writes change different files: each object alone in its own file, at the built-in path or where its +document already lives, with no kustomization governing it and no `placement.byType` or +`placement.default` template for a new one. Anything it cannot prove stays serial, one window at a +time: an object in a file with other documents, anything under a kustomization or with +`useKustomize` on, a new document placed by a template, an encrypted type. Such a write, a write to +a file another author's open window holds, and a write of another target close the windows it +cannot sit beside first, so those still split a burst. **Commits land in the order their windows +close**, not the order their first writes arrived: with Alice's window open since 10:00 and Bob's +since 10:01, Bob's commit can come first. No file ever goes back in time, but `git log` is no +longer one timeline of edits. + +Atomic writes, a resync, buffer limits, shutdown, an `attach: Next` request, a push notice from the +[receiver](#gitreceiver), and the [flush](bi-directional.md#flush-then-apply) before an apply close +the open windows early. Push cooldown is independent: a zero timer does not promise an immediate +remote push. See the [complete applier claim rules](spec/commit-window-refactor.md). Both values are **Go duration strings with a mandatory unit**, at most `24h`, checked by the API diff --git a/docs/design/concurrent-author-windows.md b/docs/design/concurrent-author-windows.md index 70816f87..a5ab1a8e 100644 --- a/docs/design/concurrent-author-windows.md +++ b/docs/design/concurrent-author-windows.md @@ -1,6 +1,9 @@ # Design: one commit window per author, open at the same time -Status: **proposal, not built.** Raised by gitops-api's review of the source-side save (PR #456): +Status: **stages 1 and 2 built** (`internal/git/window_set.go`, `footprint.go`, +`window_admission.go`; see [what stage 2 built](#what-stage-2-built)). Stage 2 ships without the +startup flag of stage 4, so stage 3 lands before the next release (`docs/TODO.md`). Raised by +gitops-api's review of the source-side save (PR #456): with saves created straight from a browser, a second person's edit splits the first person's change into two commits. This page sets out the change that fixes that, the rules it must keep, how to test it, and the stages to ship it in. @@ -300,6 +303,28 @@ window, so they are proven against today's behavior before they are used to judg measurements. Field ownership and the apply hold are separate changes, each with its own state-machine review. +### What stage 2 built + +The rules above, with these choices where the page left room: + +- **A window is proven once it has company.** A window that never sat beside another has no + footprint ("unproven") and its commit is not checked; proving costs a scan of the folder per + revision, and a lone window needs no proof. From the first write admitted beside it, a window's + footprint covers every write it collects. +- **The proof also needs the folder's bootstrap files committed**, the checkout to hold every decided + write, a whole object (not a field patch) and a type that is not encrypted. Anything else is + unknown. The revision is the count of the worker's resets and own commits, with the generation. +- **The check before the commit uses a proof of the commit's own tree.** The windows a write's + admission closes commit before it and can move what it changes (a delete committed first turns + its update into a create), so the decided write is proven again once the checkout holds every + write before it. After a reset or a `GitTarget` change the proof is of another tree, and the + commit is not checked. A refusal fails the write as terminal: the worktree is undone, its save + fails with the paths, the worker logs `SingleWindowFallback` and holds one window at a time until + it restarts, and a resync of the `GitTarget` is asked for. +- **A request's own author's window stands in no one's way.** A save waiting while another author's + window is open records a mismatch only when no window of its own author is open. +- **The window ID** (`window`) is in the open, close and finalized log lines, with the branch. + ## What it costs - **The loop's state.** The single `openWindow` becomes a set with an opening order, the close timer diff --git a/docs/interpreting-metrics.md b/docs/interpreting-metrics.md index bcbbe616..8691e221 100644 --- a/docs/interpreting-metrics.md +++ b/docs/interpreting-metrics.md @@ -206,7 +206,7 @@ boundary, the commit, the push. Background: | `git_queue_refusals_total` | counter | `gitbranch_namespace`, `gitbranch_name`, `provider_namespace`, `provider_name`, `kind` | Work refused at enqueue: the queue was full, intake was paused through an outage, or the worker was stopping. `kind` is `write` / `attach` / `resync` / `refresh`. The producer keeps a refused item and offers it again: the watch delivers a write again from its cursor, the controller re-sends a save, a resync is gathered again, and the next reconcile asks for a refresh. What it can cost, when its watch cursor expires before the write comes back, is the write's own commit and, under `prune.mode: OnEvent`, a deleted object's removal from Git. | | `git_commit_failures_total` | counter | `gitbranch_namespace`, `gitbranch_name`, `provider_namespace`, `provider_name`, `reason` | A live commit window that died between routing and pushing; a resync's failure is `git_resync_failures_total`. `reason` is `refused` (a Git path a human must fix) / `error`. Every increment is a window's events lost until the next resync. An unreachable remote does not count: a decided window waits for the publication retry instead. | | `git_writes_dropped_total` | counter | `gitbranch_namespace`, `gitbranch_name`, `provider_namespace`, `provider_name`, `reason` | A live event the branch worker dropped because its `GitTarget`'s render fidelity was not established. `reason` is `render_fidelity_diverged`: a rendered value does not match the live object, and every write is dropped until a fresh replay measures the folder clean. Or it is `render_fidelity_pending`: a collection of the `GitTarget` has not finished its replay, which closes every collection of that target until it does. A live event that meets a pending gate is held, not dropped, and committed with its author once the gate opens; it is counted here only when the hold gives up, 10 minutes after the first one was held or once the branch holds more than its retained-byte budget, and the worker then asks for a fresh snapshot, which commits the current state as the operator (a deletion comes back only under prune mode `Always`). Nothing replays a dropped write, so each one is an edit missing from Git until the object changes again or its collection replays. The worker logs the first drop under each gate status at Info, naming the collections the gate waits for, and each hold, release and give-up at Info. | -| `git_commit_windows_total` | counter | `gittarget_namespace`, `gittarget_name`, `close_reason`, `timer_source` | One per closed commit window, whether or not it then produced a commit. `close_reason` is `idle_timeout` / `max_duration` / `attach_next` / `identity_change` / `buffer_limit` / `resync_before_apply` / `foreign_commit` (a Git host said the branch may have moved, so the window is published now) / `apply_due` (an apply came due for an applier on the branch, so the window's edits are published before the apply is sent) / `shutdown`; `timer_source` is `target` or `commit_request`. See [tuning commit windows](#tuning-commit-windows). | +| `git_commit_windows_total` | counter | `gittarget_namespace`, `gittarget_name`, `close_reason`, `timer_source` | One per closed commit window, whether or not it then produced a commit. `close_reason` is `idle_timeout` / `max_duration` / `attach_next` / `identity_change` / `path_conflict` / `buffer_limit` / `resync_before_apply` / `foreign_commit` (a Git host said the branch may have moved, so the window is published now) / `apply_due` (an apply came due for an applier on the branch, so the window's edits are published before the apply is sent) / `shutdown`; `timer_source` is `target` or `commit_request`. See [tuning commit windows](#tuning-commit-windows). | | `git_commit_window_duration_seconds` | histogram | `gittarget_namespace`, `gittarget_name` | How long each window collected: from the write that opened it to the start of its finalize. Its count equals `git_commit_windows_total` summed over `close_reason` and `timer_source`. | | `git_queue_depth` | gauge | `gitbranch_namespace`, `gitbranch_name`, `provider_namespace`, `provider_name` | Pending + in-flight + committed-but-unpushed. Read at scrape time. | | `git_retained_bytes` | gauge | `gitbranch_namespace`, `gitbranch_name`, `provider_namespace`, `provider_name` | What the worker holds until a push lands, in the units of the retained-byte budget: the open window, the retained writes and waiting saves, each with a 1 KiB per-item charge. Serialized bytes, not heap. | @@ -645,9 +645,13 @@ picture toward `max_duration`. writes kept arriving closer together than `idleTimeout`; raise `maxDuration` if those changes belong in one commit. It does not prove continuous activity: an `idleTimeout` close to `maxDuration` ends quiet windows this way too. -- `identity_change` means a write arrived that could not join the open window: a different author, - a different GitTarget on the same branch, or a different attribution state. A high share means - commits are being split by interleaving rather than by time, and no timer changes that. +- `identity_change` means a write arrived that could not join an open window, nor be proven to change + other files than it: a different GitTarget on the same branch, or a different author or + attribution state writing where the worker cannot prove a file of its own (a file with other + documents, a kustomize folder, a placement template). A high share means commits are being split + by interleaving rather than by time, and no timer changes that. +- `path_conflict` means another author's write changes a file the window's writes change, so the + window committed first. Authors editing different files keep a window each. - `attach_next` is a save with `attach: Next` closing the author's open window before it waits. - `buffer_limit` means the worker's retained-write budget was reached, and the window was flushed to stay within it. Writes committed but not yet pushed count toward that budget, so this reads diff --git a/internal/git/applier_claim.go b/internal/git/applier_claim.go index eb9d2c06..523e32ec 100644 --- a/internal/git/applier_claim.go +++ b/internal/git/applier_claim.go @@ -374,7 +374,7 @@ func (l *branchWorkerEventLoop) closeWindowForPushHint() { if !hinted || l.w.applyRequester == nil { return } - for _, window := range l.commitWindows.all() { + for _, window := range l.windows.all() { if window.pendingCR != nil { continue } diff --git a/internal/git/applier_claim_test.go b/internal/git/applier_claim_test.go index 77ae71de..4714d9fb 100644 --- a/internal/git/applier_claim_test.go +++ b/internal/git/applier_claim_test.go @@ -370,12 +370,12 @@ func TestCloseWindowForPushHint_PublishesTheOpenWindowNow(t *testing.T) { loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("edited", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow, "an hour-long window is still open") + require.NotNil(t, loop.windows.only(), "an hour-long window is still open") loop.handleRefreshRequest(&RefreshRequest{PushNotice: true}) loop.closeWindowForPushHint() - assert.Nil(t, loop.openWindow) + assert.Nil(t, loop.windows.only()) assert.Len(t, loop.pendingWrites, 1, "the window's write is decided, to be published") } @@ -391,14 +391,14 @@ func TestCloseWindowForPushHint_LeavesASavesWindowToTheSave(t *testing.T) { loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("edited", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow, "an hour-long window is still open") + require.NotNil(t, loop.windows.only(), "an hour-long window is still open") id := commitRequestID{Namespace: "team-a", Name: "save", UID: "uid-save"} - loop.openWindow.pendingCR = &id + loop.windows.only().pendingCR = &id loop.handleRefreshRequest(&RefreshRequest{PushNotice: true}) loop.closeWindowForPushHint() - assert.NotNil(t, loop.openWindow, "the save's window is still open") + assert.NotNil(t, loop.windows.only(), "the save's window is still open") assert.Empty(t, loop.pendingWrites) } diff --git a/internal/git/applier_echoes_test.go b/internal/git/applier_echoes_test.go index c5a21016..9f54cc83 100644 --- a/internal/git/applier_echoes_test.go +++ b/internal/git/applier_echoes_test.go @@ -35,6 +35,15 @@ type echoFixture struct { } func newEchoFixture(t *testing.T, slug string, prune configv1alpha3.PruneMode, docs map[string]string) *echoFixture { + t.Helper() + return newEchoFixtureWith(t, slug, prune, docs, nil) +} + +// newEchoFixtureWith is newEchoFixture with files, by path, committed from outside after the documents +// and before the worker clones, so the worker starts on them as on anything it has applied. +func newEchoFixtureWith( + t *testing.T, slug string, prune configv1alpha3.PruneMode, docs, files map[string]string, +) *echoFixture { t.Helper() f := newLedgerFixture(t, slug, true) f.worker.mapper = configMapMapper() @@ -47,6 +56,16 @@ func newEchoFixture(t *testing.T, slug string, prune configv1alpha3.PruneMode, d for name, value := range docs { e.pushFromOutside(name, value) } + if len(files) > 0 { + for path, content := range files { + full := filepath.Join(clone, path) + require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o750)) + require.NoError(t, os.WriteFile(full, []byte(content), 0o600)) + gitIn(t, clone, "add", path) + } + gitIn(t, clone, "commit", "--quiet", "-m", "outside: files") + gitIn(t, clone, "push", "--quiet", "origin", "HEAD:main") + } require.NoError(t, f.worker.ensureRepositoryInitialized(f.worker.ctx)) e.loop = newBranchWorkerEventLoop(f.worker, 0) t.Cleanup(e.loop.stopTimers) @@ -317,17 +336,17 @@ func TestApplierEchoes_FlushThenApply(t *testing.T) { holdPushes(f.loop) // the push cooldown runs: a flush does not wait for it f.writeWindow(echoEvent("x", "x2")) // E0, in the open window when the apply comes due - require.NotNil(t, f.loop.openWindow) + require.NotNil(t, f.loop.windows.only()) restore := failPushes(t, f.worker) f.echoApplier().repair = true f.loop.endWake(0) - assert.Nil(t, f.loop.openWindow, "the apply closed the open window") + assert.Nil(t, f.loop.windows.only(), "the apply closed the open window") assert.Empty(t, requester.taken(), "the writes up to the mark are not published yet") require.NotNil(t, f.loop.flushEdits, "edits are recorded from the mark") f.writeWindow(echoEvent("y", "y2")) // E1, decided after the mark - require.NotNil(t, f.loop.openWindow, "it opens a window of its own") + require.NotNil(t, f.loop.windows.only(), "it opens a window of its own") restore() f.loop.retry.due = time.Now().Add(-time.Second) diff --git a/internal/git/apply_attempt_test.go b/internal/git/apply_attempt_test.go index 4a6059ac..e1f87b6f 100644 --- a/internal/git/apply_attempt_test.go +++ b/internal/git/apply_attempt_test.go @@ -152,7 +152,7 @@ func TestApplier_ARecoveredRefusalIsNotRetried(t *testing.T) { // Requests carry the uid, and the worker drops what was pending for an incarnation that is gone. func TestApplier_ARecreatedTargetInheritsNothing(t *testing.T) { loop, requester := manualLoop(t, applierTestTarget("alpha", true, reapplying)) - loop.openWindow = &openWindow{} + loop.windows.setOnly(&openWindow{}) addAlphaUnsentRefusal(loop) old := &configv1alpha3.GitTarget{} @@ -165,7 +165,7 @@ func TestApplier_ARecreatedTargetInheritsNothing(t *testing.T) { recreated.UID = "uid-alpha-recreated" require.NoError(t, loop.w.Client.Create(context.Background(), recreated)) - loop.openWindow = nil + loop.windows.setOnly(nil) loop.serviceAppliers() assert.Empty(t, requester.taken(), "the refusal was the old incarnation's") assert.Empty(t, loop.unsentRefusals) @@ -340,7 +340,7 @@ func TestPushNotice_TheTipAlreadySeenDoesNotCutAWindow(t *testing.T) { Spec: configv1alpha3.GitProviderSpec{URL: "https://github.com/acme/platform"}, }) loop.w.recordRemoteObservation("ours", ObservedByPush) - loop.openWindow = &openWindow{} + loop.windows.setOnly(&openWindow{}) loop.handleRefreshRequest(&RefreshRequest{PushNotice: true, After: tc.after}) diff --git a/internal/git/apply_catchup_test.go b/internal/git/apply_catchup_test.go index d68e93b1..69433a79 100644 --- a/internal/git/apply_catchup_test.go +++ b/internal/git/apply_catchup_test.go @@ -82,14 +82,14 @@ func TestCatchUp_AnAPICreateIsAdoptedEvenAtTheTip(t *testing.T) { owned := createdEvent("alpha", "kubectl-create") owned.AppliedBy = AppliedBy{Kind: configv1alpha3.ApplierKindKustomization, Namespace: "x", Name: "other"} loop.handleLiveEvents(&WriteRequest{Events: []Event{owned}}) - loop.openWindow = nil + loop.windows.setOnly(nil) loop.serviceAppliers() require.Empty(t, requester.taken(), "neither is an API create of the folder's own objects") loop.handleLiveEvents(&WriteRequest{ Events: []Event{createdEvent("alpha", "kubectl-create")}, }) - loop.openWindow = nil + loop.windows.setOnly(nil) loop.serviceAppliers() adopt := requester.taken() require.Len(t, adopt, 1, "adopted, although the applier is at the tip") diff --git a/internal/git/apply_converge_test.go b/internal/git/apply_converge_test.go index edc3973c..66bb9b69 100644 --- a/internal/git/apply_converge_test.go +++ b/internal/git/apply_converge_test.go @@ -144,7 +144,7 @@ func TestCatchUpAfter_YieldsToAnEditBeforeItsResume(t *testing.T) { assert.False(t, sent[0].Current(), "an edit arrived during the fetch") report(sent[0], ApplyStale) - loop.openWindow = nil + loop.windows.setOnly(nil) loop.serviceAppliers() assert.Empty(t, requester.taken(), "not again at once: a fresh interval") state := loop.appliers[applierRef("alpha")] diff --git a/internal/git/apply_ownership_test.go b/internal/git/apply_ownership_test.go index 8a1fd896..4e00dd18 100644 --- a/internal/git/apply_ownership_test.go +++ b/internal/git/apply_ownership_test.go @@ -92,7 +92,7 @@ func TestOwnership_ALiveWriteOfAnotherAppliersObjectIsNotMirrored(t *testing.T) event := windowEvent("alpha", "2", "kubectl-edit") event.AppliedBy = other loop.handleLiveEvents(&WriteRequest{Events: []Event{event}}) - assert.Nil(t, loop.openWindow, "the object another applier owns opened no window") + assert.Nil(t, loop.windows.only(), "the object another applier owns opened no window") } // TestOwnership_ASnapshotObjectOfAnotherApplierIsNeitherUpsertedNorSwept: the resync drops the object diff --git a/internal/git/apply_window_carry_test.go b/internal/git/apply_window_carry_test.go index 969c9a10..3a6089f7 100644 --- a/internal/git/apply_window_carry_test.go +++ b/internal/git/apply_window_carry_test.go @@ -91,7 +91,7 @@ func carryKey() string { return objectKey(carryEdit("alice", "", "").Identifier) func TestApplyWindow_AWithheldApplyIsNotCommittedUnderTheNextEdit(t *testing.T) { t.Run("in the window that holds the edit", func(t *testing.T) { f := carryFlushOverAnEdit(t, "carry-same-window") - require.NotNil(t, f.loop.openWindow, "E1 is collecting in alice's window") + require.NotNil(t, f.loop.windows.only(), "E1 is collecting in alice's window") f.writeWindow(carryApply("a1", "b1")) require.True(t, f.echoApplier().repair, "the apply over E1 is withheld, and a Repair is pending") diff --git a/internal/git/apply_window_test.go b/internal/git/apply_window_test.go index 38086a9b..39b2ea24 100644 --- a/internal/git/apply_window_test.go +++ b/internal/git/apply_window_test.go @@ -126,9 +126,9 @@ func TestApplyWindow_OnlyAWithheldWriteIsKeptFromTheCommitWindow(t *testing.T) { windowEvent("alpha", "2", FluxFieldManager), }}) - require.NotNil(t, loop.openWindow) - require.Len(t, loop.openWindow.pathToEvent, 1) - for _, event := range loop.openWindow.pathToEvent { + require.NotNil(t, loop.windows.only()) + require.Len(t, loop.windows.only().pathToEvent, 1) + for _, event := range loop.windows.only().pathToEvent { replicas, _, _ := unstructured.NestedString(event.Object.Object, "spec", "replicas") assert.Equal(t, "3", replicas, "Git keeps the API's value") } diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index 39b0f1a3..318490bc 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -255,6 +255,11 @@ type BranchWorker struct { // // See docs/design/gittarget-branch-worker-pending-writes.md. checkoutApplied atomic.Int64 + // checkoutResets and checkoutCommits count the resets of the checkout and the commits the worker + // made on it. With the GitTarget's generation they name the revision a window's footprint is + // proven against (footprint.go). + checkoutResets atomic.Uint64 + checkoutCommits atomic.Uint64 // lastReapply records when this worker last decided a Refusal request for a GitTarget's applier, keyed // by "namespace/name", so refusalReapplyInterval can floor the rate. It is guarded by its own @@ -431,6 +436,10 @@ const ( windowFinalizeReasonShutdown windowFinalizeReason = "shutdown" windowFinalizeReasonForeignCommit windowFinalizeReason = "foreign-commit" windowFinalizeReasonApplyDue windowFinalizeReason = "apply-due" + // windowFinalizeReasonPathConflict closes a window whose footprint a write, or a proof against a + // newer revision, finds overlapping another window's: it commits before the write that overlaps + // it (footprint.go). + windowFinalizeReasonPathConflict windowFinalizeReason = "path-conflict" ) // NewBranchWorker creates a worker for a (provider, branch) combination. A zero @@ -987,8 +996,8 @@ func (w *BranchWorker) queueDepth() int64 { // processEvents is the main event processing loop. // -// The loop owns one live commit-shaped event window. A commit-window timer -// detects silence for that open window, and a one-shot push timer enforces the +// The loop owns the live commit-shaped event windows. One commit-window timer +// wakes it for the earliest of their deadlines, and a one-shot push timer enforces the // PushCooldown between successful pushes. Commit and push are independent: // local commits accumulate from retained pending writes and feed // replay-on-conflict; only a successful push clears that retained queue. @@ -1014,11 +1023,16 @@ func (w *BranchWorker) processEvents() { // goroutine running run() may touch these fields, so no extra synchronisation // is required. type branchWorkerEventLoop struct { - // commitWindows holds the live commit-shaped event windows and what they are charged. A window is - // finalized when its idle or max-duration timer runs out, and eagerly on author/target changes, - // resync arrivals, byte-cap trips, an attach: Next, and shutdown. Embedded, so loop.openWindow - // names the one window it holds. - commitWindows + // windows holds the live commit-shaped event windows and what they are charged. A window is + // finalized when its idle or max-duration timer runs out, and eagerly when a write cannot be + // admitted beside it (another GitTarget, an unproven or overlapping footprint), on resync + // arrivals, byte-cap trips, an attach: Next, and shutdown. + windows commitWindows + // footprints are the placement indexes footprints are proven with, by GitTarget, and + // singleWindow is set once a commit changed a file outside its window's footprint: from then on + // the worker holds one window at a time (footprint.go). + footprints map[itypes.ResourceReference]*footprintIndex + singleWindow bool w *BranchWorker @@ -1244,7 +1258,7 @@ func (l *branchWorkerEventLoop) endWake(released int64) { // publishing the flag second would let both read empty while a window is open, and a scrape landing // there would report a drained queue for a worker that has work. func (l *branchWorkerEventLoop) publishLoopState(released int64) { - l.w.hasUnpushedWork.Store(l.commitWindows.anyOpen() || len(l.pendingWrites) > 0 || len(l.held) > 0) + l.w.hasUnpushedWork.Store(l.windows.anyOpen() || len(l.pendingWrites) > 0 || len(l.held) > 0) l.syncAdmission(released) } @@ -1271,7 +1285,7 @@ func (l *branchWorkerEventLoop) timerChannels() ( // the open windows plus the locally-committed-but-not-yet-pushed events that // we keep around for replay-on-conflict. func (l *branchWorkerEventLoop) totalRetainedBytes() int64 { - return l.commitWindows.windowBytes + l.pendingWritesBytes + return l.windows.bytes() + l.pendingWritesBytes } // dispatch hands one item to its handler, and returns the intake charge the item was admitted with. @@ -1323,30 +1337,21 @@ func (l *branchWorkerEventLoop) handleLiveEvents(request *WriteRequest) { // finalizing it as identity, the byte cap or a zero window require. func (l *branchWorkerEventLoop) appendLiveEvent(event Event) { { - for _, other := range l.commitWindows.blocking(event) { - // Log the identity that broke the window so an unexpected split is - // diagnosable: the common cause is an incoming event whose author is empty - // (an unattributed update, e.g. a /status change that carries no audit fact) - // arriving against a non-empty window author, which forces a finalize and - // can split a CommitRequest collect window. windowAuthor vs eventAuthor makes - // that visible at a glance. - l.w.Log.Info("Window identity change forces finalize before appending event", - "windowAuthor", other.Author, - "eventAuthor", event.UserInfo.Username, - "operation", event.Operation, - "resource", event.Identifier.String(), - "eventTarget", event.GitTargetNamespace+"/"+event.GitTargetName) - l.finalizeWindow(other, windowFinalizeReasonIdentityChange) - } + // Closes the windows the write cannot sit beside first (window_admission.go). + proven := l.admitWrite(event) - window := l.commitWindows.forWrite(event) + window := l.windows.forWrite(event) opened := false if window == nil { + window = l.windows.open(event, l.w.contentWriter, l.w.now()) l.w.Log.Info("Opening commit window", + "window", window.seq, + "branch", l.w.Branch, "author", event.UserInfo.Username, "gitTarget", event.GitTargetNamespace+"/"+event.GitTargetName, - "resource", event.Identifier.String()) - window = l.commitWindows.open(event, l.w.contentWriter, l.w.now()) + "resource", event.Identifier.String(), + "footprint", footprintString(proven), + "decision", decisionWindowOpened) // One window, one GitTarget: snapshot that target's timers now, so a later edit to the // target never moves a deadline already running. timers := l.w.commitWindowFor( @@ -1356,13 +1361,21 @@ func (l *branchWorkerEventLoop) appendLiveEvent(event Event) { maxAt: l.w.now().Add(timers.maxDuration), } opened = true + } else { + l.w.Log.V(1).Info("Write joined open commit window", + "window", window.seq, + "branch", l.w.Branch, + "resource", event.Identifier.String(), + "footprint", footprintString(proven), + "decision", decisionWriteJoinedWindow) } + window.footprint = proven // The window keeps one event per path, so a replaced one stops being charged. if replaced, ok := window.add(event); ok { - l.commitWindows.charge(window, -eventRetainedCharge(&replaced)) + l.windows.charge(window, -eventRetainedCharge(&replaced)) } window.lastWriteAt = l.w.now() - l.commitWindows.charge(window, eventRetainedCharge(&event)) + l.windows.charge(window, eventRetainedCharge(&event)) if opened { // A waiting CommitRequest gets the new window BEFORE anything can close it: before the @@ -1372,7 +1385,7 @@ func (l *branchWorkerEventLoop) appendLiveEvent(event Event) { // waiting on another author, never see the window that refuses it. l.noteForeignWindow() l.attachWaitingCommitRequests() - if !l.commitWindows.isOpen(window) { + if !l.windows.isOpen(window) { return // the attached request's maxDuration: 0s closed it already } } @@ -1433,14 +1446,20 @@ func (l *branchWorkerEventLoop) mirrors(event *Event) bool { // closes the window in the same step that collected the write. func (l *branchWorkerEventLoop) closeOrArmWindow() { now := l.w.now() - var next time.Time - for _, window := range l.commitWindows.all() { - closeAt, reason := window.closeAt() - if !closeAt.After(now) { + for _, window := range l.windows.all() { + if closeAt, reason := window.closeAt(); !closeAt.After(now) { l.finalizeWindow(window, reason) - continue } - if next.IsZero() || closeAt.Before(next) { + } + l.armCommitTimer() +} + +// armCommitTimer arms the commit timer for the earliest deadline of the open windows, or stops it +// when none is open. Every window has its own deadlines; the one timer wakes the loop for the first. +func (l *branchWorkerEventLoop) armCommitTimer() { + var next time.Time + for _, window := range l.windows.all() { + if closeAt, _ := window.closeAt(); next.IsZero() || closeAt.Before(next) { next = closeAt } } @@ -1448,7 +1467,7 @@ func (l *branchWorkerEventLoop) closeOrArmWindow() { l.stopCommitTimer() return } - l.resetCommitTimer(next.Sub(now)) + l.resetCommitTimer(next.Sub(l.w.now())) } func (l *branchWorkerEventLoop) handleShutdown() { @@ -1528,7 +1547,7 @@ func (l *branchWorkerEventLoop) resetCommitTimer(delay time.Duration) { // finalizeOpenWindowWithReason closes every open window, oldest first: the triggers that are about // the branch rather than about one window (a resync, an apply flush, the byte cap, shutdown). func (l *branchWorkerEventLoop) finalizeOpenWindowWithReason(reason windowFinalizeReason) { - for _, window := range l.commitWindows.all() { + for _, window := range l.windows.all() { l.finalizeWindow(window, reason) } } @@ -1540,7 +1559,8 @@ func (l *branchWorkerEventLoop) finalizeOpenWindowWithReason(reason windowFinali // for the publication retry, and its CommitRequest held. Only a failure of the write itself // drops it, and resolves that request Failed (settleFailed). func (l *branchWorkerEventLoop) finalizeWindow(window *openWindow, reason windowFinalizeReason) { - l.stopCommitTimer() + // The windows left open keep their own deadlines: the timer is armed again for theirs. + defer l.armCommitTimer() // Counted here, before anything can fail or find no diff, so every window is counted once and // its collection time never includes the commit. l.w.recordWindowClosed(window, reason) @@ -1564,11 +1584,14 @@ func (l *branchWorkerEventLoop) finalizeWindow(window *openWindow, reason window "reason", string(reason), "windowAuthor", windowAuthor, "windowTarget", windowTarget, + "window", window.seq, + "branch", l.w.Branch, "events", len(events), "windowBytes", window.bytes, "pendingWrites", len(l.pendingWrites), "messageOverride", effectiveMessage != "", - "attachedCR", pendingCR != nil) + "attachedCR", pendingCR != nil, + "decision", decisionWindowClosed) pendingWrite, err := l.w.buildGroupedPendingWrite(l.w.ctx, events) if err != nil { @@ -1587,18 +1610,33 @@ func (l *branchWorkerEventLoop) finalizeWindow(window *openWindow, reason window return } l.carryRequestOnWrite(pendingWrite, pendingCR) + pendingWrite.footprint = l.closingFootprint(window) - l.commitWindows.remove(window) + l.windows.remove(window) l.decide(*pendingWrite) l.w.Log.Info("Open commit window finalized", "reason", string(reason), "windowAuthor", windowAuthor, "windowTarget", windowTarget, + "window", window.seq, + "branch", l.w.Branch, + "footprint", footprintString(pendingWrite.footprint), "events", len(events), "pendingWrites", len(l.pendingWrites)) } +// closingFootprint marks the closing window's write for the check before its commit when the window +// was ever open beside another: its footprint, proven again on the tree the commit is planned on +// (proveBeforeCommit). Nil for a window never proven. +func (l *branchWorkerEventLoop) closingFootprint(window *openWindow) *footprint { + if window.footprint == nil { + return nil + } + f := *window.footprint + return &f +} + // carryRequestOnWrite carries the attached CommitRequest onto the write so its result follows the // data: it is resolved Committed once this write is pushed (ยง6.5). The request's // whenNothingToCommit rides along, so a window that changed nothing can still record its message. @@ -1640,7 +1678,7 @@ func (l *branchWorkerEventLoop) gateBuiltWindow( // dropWindow discards a window whose finalize failed before it was decided, resolving any // attached CommitRequest as Failed so the controller does not poll forever. func (l *branchWorkerEventLoop) dropWindow(window *openWindow, pendingCR *commitRequestID, cause error) { - l.commitWindows.remove(window) + l.windows.remove(window) if pendingCR != nil { l.resolveCommitRequest(*pendingCR, FinalizeResult{Branch: l.w.Branch, Err: cause}) } @@ -1903,7 +1941,10 @@ func (w *BranchWorker) markWorktreeDirty(reason string) { // well as after every reset, because a reset moves the branch ref and then rewrites the worktree, // so it can fail with the ref already moved and the commits behind the retained writes already // unreachable. Marking too eagerly costs one replay. -func (w *BranchWorker) markCheckoutReset() { w.checkoutApplied.Store(0) } +func (w *BranchWorker) markCheckoutReset() { + w.checkoutApplied.Store(0) + w.checkoutResets.Add(1) +} // markCheckoutPublished records that a push published every retained write: unless a failed write // left the worktree dirty, the checkout is its root again. @@ -2929,6 +2970,8 @@ func (r windowFinalizeReason) metricLabel() string { return "foreign_commit" case windowFinalizeReasonApplyDue: return "apply_due" + case windowFinalizeReasonPathConflict: + return "path_conflict" case windowFinalizeReasonUnspecified: return "unspecified" default: diff --git a/internal/git/branch_worker_loop_test.go b/internal/git/branch_worker_loop_test.go index 4a06f792..3aa399b7 100644 --- a/internal/git/branch_worker_loop_test.go +++ b/internal/git/branch_worker_loop_test.go @@ -123,7 +123,7 @@ func TestEventLoop_TotalRetainedBytes(t *testing.T) { assert.Equal(t, int64(0), loop.totalRetainedBytes()) - loop.windowBytes = 100 + loop.windows.windowBytes = 100 loop.pendingWritesBytes = 250 assert.Equal(t, int64(350), loop.totalRetainedBytes(), "cap is enforced against the open window + pendingWrites combined") diff --git a/internal/git/branch_worker_new_branch_test.go b/internal/git/branch_worker_new_branch_test.go index dae1f33c..eb597b33 100644 --- a/internal/git/branch_worker_new_branch_test.go +++ b/internal/git/branch_worker_new_branch_test.go @@ -412,7 +412,7 @@ func TestBranchWorker_ParentMovesWhileTheWindowIsOpen(t *testing.T) { f.warmOnParent() liveWrite(loop, "cm1") - require.NotNil(t, loop.openWindow, "the window is still collecting") + require.NotNil(t, loop.windows.only(), "the window is still collecting") hashB := f.pushToMain(map[string]string{"LATEST.md": "from main at B\n"}) require.True(t, loop.finalizeOpenWindow()) diff --git a/internal/git/branch_worker_queue_depth_test.go b/internal/git/branch_worker_queue_depth_test.go index f76f03aa..c7d1e313 100644 --- a/internal/git/branch_worker_queue_depth_test.go +++ b/internal/git/branch_worker_queue_depth_test.go @@ -33,7 +33,7 @@ func TestHandledItem_PublishesRetainedWorkBeforeReleasingInflight(t *testing.T) loop := newBranchWorkerEventLoop(w, time.Second) // The state handleQueueItem leaves behind for an event that opened a commit window. - loop.openWindow = &openWindow{} + loop.windows.setOnly(&openWindow{}) require.Positive(t, w.queueDepth(), "the in-flight count covers the item before the release") loop.endWake(0) diff --git a/internal/git/branch_worker_split_test.go b/internal/git/branch_worker_split_test.go index 61464cf3..6e2ec6aa 100644 --- a/internal/git/branch_worker_split_test.go +++ b/internal/git/branch_worker_split_test.go @@ -778,7 +778,7 @@ func TestEventLoop_CommitWindowZero_HonestPerEvent(t *testing.T) { }, }}) - assert.Nil(t, loop.openWindow, "commitWindow=0 must finalize the open window on every event") + assert.Nil(t, loop.windows.only(), "commitWindow=0 must finalize the open window on every event") assert.Len(t, loop.pendingWrites, 2, "events land in pendingWrites immediately while the cooldown defers the push") assert.NotNil(t, loop.pushTimer, @@ -816,8 +816,8 @@ func TestEventLoop_AuthorChangeFinalizesOpenWindow(t *testing.T) { require.Len(t, loop.pendingWrites, 1) assert.Equal(t, "alice", loop.pendingWrites[0].Events[0].UserInfo.Username) - require.NotNil(t, loop.openWindow) - assert.Equal(t, "bob", loop.openWindow.Author) + require.NotNil(t, loop.windows.only()) + assert.Equal(t, "bob", loop.windows.only().Author) loop.stopTimers() } @@ -839,8 +839,8 @@ func TestEventLoop_TargetChangeFinalizesOpenWindow(t *testing.T) { require.Len(t, loop.pendingWrites, 1) assert.Equal(t, "team-a", loop.pendingWrites[0].Events[0].GitTargetName) - require.NotNil(t, loop.openWindow) - assert.Equal(t, "team-b", loop.openWindow.GitTarget) + require.NotNil(t, loop.windows.only()) + assert.Equal(t, "team-b", loop.windows.only().GitTarget) loop.stopTimers() } @@ -859,8 +859,8 @@ func TestEventLoop_RepeatedPathEditsCollapseInOpenWindow(t *testing.T) { Events: []Event{first, second, configMapEvent("other", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow) - events := loop.openWindow.orderedEvents() + require.NotNil(t, loop.windows.only()) + events := loop.windows.only().orderedEvents() require.Len(t, events, 2) assert.Equal(t, "same", events[0].Identifier.Name) assert.Equal(t, "latest", events[0].Object.Object["data"].(map[string]interface{})["key"]) @@ -883,7 +883,7 @@ func TestEventLoop_ByteCapFinalizesIncludingTrippingEvent(t *testing.T) { Events: []Event{configMapEvent("oversized", "alice", "team-a")}, }}) - assert.Nil(t, loop.openWindow) + assert.Nil(t, loop.windows.only()) require.Len(t, loop.pendingWrites, 1) require.Len(t, loop.pendingWrites[0].Events, 1) assert.Equal(t, "oversized", loop.pendingWrites[0].Events[0].Identifier.Name) @@ -908,8 +908,8 @@ func TestEventLoop_AuthorChangeStartsFreshCommitTimer(t *testing.T) { require.NotNil(t, loop.commitTimer) assert.NotSame(t, firstTimer, loop.commitTimer) - require.NotNil(t, loop.openWindow) - assert.Equal(t, "bob", loop.openWindow.Author) + require.NotNil(t, loop.windows.only()) + assert.Equal(t, "bob", loop.windows.only().Author) loop.stopTimers() } diff --git a/internal/git/commit_executor.go b/internal/git/commit_executor.go index f892c207..5b9c4bf6 100644 --- a/internal/git/commit_executor.go +++ b/internal/git/commit_executor.go @@ -110,6 +110,10 @@ func (w *BranchWorker) runPendingWrites( // requestTemplate render failed back here at commit time. pendingWrites[i].committedMessageSource = source commitsCreated += created + // A new revision: every footprint proven on the tree before it is proven again (footprint.go). + if created > 0 { + w.checkoutCommits.Add(1) + } } return commitsCreated, refusals, nil @@ -397,7 +401,8 @@ func (w *BranchWorker) executeEventWrite( if err != nil { return 0, plumbing.ZeroHash, messageResolutionUnsupported, err } - if !pendingWrite.actionsDecided { + first := !pendingWrite.actionsDecided + if first { pendingWrite.actionsDecided = true if !pendingWrite.keepsEveryObject() { pendingWrite.Events = eventsThatChanged(pendingWrite.Events, changedBy) @@ -407,6 +412,12 @@ func (w *BranchWorker) executeEventWrite( if !anyChanges && !recordEmpty { return 0, plumbing.ZeroHash, messageResolutionUnsupported, nil } + // Before the commit, not after it: a window that changes a file outside its footprint may have + // been open beside one that changes that file too, and committing it could put an older value + // over a newer one. The caller undoes the worktree. + if err := w.checkFootprint(worktree, pendingWrite, first); err != nil { + return 0, plumbing.ZeroHash, messageResolutionUnsupported, err + } commitMessage, commitOptions, source, err := pendingWrite.commitMetadata() if err != nil { diff --git a/internal/git/commit_request_attach_loop.go b/internal/git/commit_request_attach_loop.go index 85c8acf1..20aedc68 100644 --- a/internal/git/commit_request_attach_loop.go +++ b/internal/git/commit_request_attach_loop.go @@ -236,14 +236,14 @@ func (l *branchWorkerEventLoop) handleAttachCommitRequest(req *AttachCommitReque "attach", string(req.Attach), "attachTimeout", req.AttachTimeout.String()) - if l.commitWindows.foreignTo(pcr) != nil { + if l.windows.foreignTo(pcr) != nil { // Noted here and not left to noteForeignWindow, which skips a request whose deadline has // passed: with attachTimeout: 0s that is every request, so a foreign window open at // registration would go unseen and the request would resolve NoWindow โ€” and, with // CommitEmpty, record an empty commit while another author's work is in flight. pcr.sawForeignWindow = true } - window := l.commitWindows.forRequest(pcr) + window := l.windows.forRequest(pcr) if window == nil { return } @@ -277,7 +277,7 @@ func (l *branchWorkerEventLoop) saveInProgress( if earlier.waiting(now) { return earlier } - collecting := earlier.attached && l.commitWindows.carrying(earlier.id) != nil + collecting := earlier.attached && l.windows.carrying(earlier.id) != nil if collecting && attach != v1alpha3.AttachNext { return earlier } @@ -328,7 +328,7 @@ func (l *branchWorkerEventLoop) noteForeignWindow() { if !pcr.waiting(now) || pcr.sawForeignWindow { continue } - foreign := l.commitWindows.foreignTo(pcr) + foreign := l.windows.foreignTo(pcr) if foreign == nil { continue } @@ -347,8 +347,9 @@ func (l *branchWorkerEventLoop) noteForeignWindow() { // a second waits for the next window, and a request whose deadline has passed takes none. func (l *branchWorkerEventLoop) attachWaitingCommitRequests() { now := l.w.now() - for _, window := range l.commitWindows.all() { - if window.pendingCR != nil { + for _, window := range l.windows.all() { + // An attach closes the windows that are due (closeOrArmWindow), so a later one may be gone. + if window.pendingCR != nil || !l.windows.isOpen(window) { continue } var first *pendingCommitRequest @@ -376,7 +377,7 @@ func (l *branchWorkerEventLoop) attachToWindow(window *openWindow, pcr *pendingC id := pcr.id window.pendingCR = &id pcr.attached = true - l.commitWindows.charge(window, attachCharge(pcr.message)) + l.windows.charge(window, attachCharge(pcr.message)) timers := windowTimers{noIdle: pcr.idleTimeout == nil, maxAt: now.Add(pcr.maxDuration)} if pcr.idleTimeout != nil { timers.idle = *pcr.idleTimeout diff --git a/internal/git/commit_request_attach_test.go b/internal/git/commit_request_attach_test.go index 74b9e926..ef29f4ad 100644 --- a/internal/git/commit_request_attach_test.go +++ b/internal/git/commit_request_attach_test.go @@ -62,8 +62,8 @@ func forceDue(loop *branchWorkerEventLoop) { if pcr := loop.pendingCRs[id]; pcr != nil && !pcr.attached { pcr.attachDeadline = past } - if loop.openWindow != nil && loop.openWindow.pendingCR != nil && *loop.openWindow.pendingCR == id { - loop.openWindow.timers.maxAt = past + if loop.windows.only() != nil && loop.windows.only().pendingCR != nil && *loop.windows.only().pendingCR == id { + loop.windows.only().timers.maxAt = past loop.closeOrArmWindow() loop.endWake(0) } @@ -145,7 +145,7 @@ func TestAttach_CommitsOpenWindow(t *testing.T) { configMapTargetEvent("explicit-b", "alice", "team-a"), }, }}) - require.NotNil(t, loop.openWindow, "events should accumulate in an open window") + require.NotNil(t, loop.windows.only(), "events should accumulate in an open window") const message = "save: increase checkout API memory" req := attachReq("alice", 0) @@ -157,7 +157,7 @@ func TestAttach_CommitsOpenWindow(t *testing.T) { require.NoError(t, res.Err) assert.Equal(t, FinalizeCommitted, res.Outcome) require.NotEmpty(t, res.Commit) - assert.Nil(t, loop.openWindow, "the open window must be finalized") + assert.Nil(t, loop.windows.only(), "the open window must be finalized") assert.Empty(t, loop.pendingWrites, "the pushed write is cleared on success") // The reported SHA must match local HEAD (Committed means on the remote) and @@ -185,7 +185,7 @@ func TestAttach_EmptyMessageUsesGeneratedMessage(t *testing.T) { loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("solo", "bob", "team-a")}, }}) - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) serviceAttach(loop, attachReq("bob", 0)) // no Message @@ -226,9 +226,9 @@ func TestAttach_CollectGraceJoinsLaterWindow(t *testing.T) { Events: []Event{configMapTargetEvent("late", "alice", "team-a")}, }}) loop.endWake(0) - require.NotNil(t, loop.openWindow) - require.NotNil(t, loop.openWindow.pendingCR, "the opened window must carry the attached request") - assert.Equal(t, "bundle save", loop.openWindow.pendingMessage) + require.NotNil(t, loop.windows.only()) + require.NotNil(t, loop.windows.only().pendingCR, "the opened window must carry the attached request") + assert.Equal(t, "bundle save", loop.windows.only().pendingMessage) // Grace elapses โ†’ the collected window is finalized as one commit. forceDue(loop) @@ -238,7 +238,7 @@ func TestAttach_CollectGraceJoinsLaterWindow(t *testing.T) { require.True(t, ok) require.NoError(t, res.Err) assert.Equal(t, FinalizeCommitted, res.Outcome) - assert.Nil(t, loop.openWindow) + assert.Nil(t, loop.windows.only()) } // TestAttach_TheAttachStartsTheRequestsMaxDuration pins that the time a request spent waiting for @@ -261,12 +261,12 @@ func TestAttach_TheAttachStartsTheRequestsMaxDuration(t *testing.T) { loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("late", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow) - require.NotNil(t, loop.openWindow.pendingCR, "precondition: the request attached to the window") + require.NotNil(t, loop.windows.only()) + require.NotNil(t, loop.windows.only().pendingCR, "precondition: the request attached to the window") - assert.False(t, loop.openWindow.timers.maxAt.Before(attached.Add(maxDuration)), + assert.False(t, loop.windows.only().timers.maxAt.Before(attached.Add(maxDuration)), "the attach must start the request's maxDuration") - assert.True(t, loop.openWindow.timers.noIdle, + assert.True(t, loop.windows.only().timers.noIdle, "a request that set no idleTimeout replaces the target's idle close with none") } @@ -295,9 +295,9 @@ func TestAttach_AnExpiredWaitNeverTakesALaterWindow(t *testing.T) { require.True(t, ok, "the overdue request must resolve in this pass") require.NoError(t, res.Err) assert.Equal(t, FinalizeNoOpenWindow, res.Outcome) - require.NotNil(t, loop.openWindow, "the late write keeps its own window") - assert.Nil(t, loop.openWindow.pendingCR, "the expired request must not take the later window") - assert.Empty(t, loop.openWindow.pendingMessage, "so the late write never carries its message") + require.NotNil(t, loop.windows.only(), "the late write keeps its own window") + assert.Nil(t, loop.windows.only().pendingCR, "the expired request must not take the later window") + assert.Empty(t, loop.windows.only().pendingMessage, "so the late write never carries its message") } // TestAttach_WritesAfterTheClaimJoinOneCommit pins the save the claim restart exists for: a request @@ -361,10 +361,10 @@ func TestAttach_ForeignWindowIsNotStolen(t *testing.T) { loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("cm", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) serviceAttach(loop, attachReq("bob", 60*time.Second)) // bob, not alice - require.Nil(t, loop.openWindow.pendingCR, "bob's attach must not claim alice's window") + require.Nil(t, loop.windows.only().pendingCR, "bob's attach must not claim alice's window") forceDue(loop) loop.endWake(0) @@ -375,8 +375,8 @@ func TestAttach_ForeignWindowIsNotStolen(t *testing.T) { assert.Equal(t, FinalizeWindowMismatch, res.Outcome, "bob waited out his grace on a window that was alice's the whole time; that is a refusal a "+ "human can see, not the benign 'nothing was pending'") - require.NotNil(t, loop.openWindow, "alice's window must be left open") - assert.Equal(t, "alice", loop.openWindow.Author) + require.NotNil(t, loop.windows.only(), "alice's window must be left open") + assert.Equal(t, "alice", loop.windows.only().Author) } // The two refusals must stay distinguishable, which is the whole reason WindowMismatch exists: @@ -390,7 +390,7 @@ func TestAttach_NoWindowAtAllIsNotAMismatch(t *testing.T) { defer loop.stopTimers() serviceAttach(loop, attachReq("bob", 60*time.Second)) - require.Nil(t, loop.openWindow, "precondition: nothing is open") + require.Nil(t, loop.windows.only(), "precondition: nothing is open") forceDue(loop) loop.endWake(0) @@ -417,13 +417,13 @@ func TestAttach_ForeignWindowClosingBeforeExpiryIsStillAMismatch(t *testing.T) { loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("cm", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) serviceAttach(loop, attachReq("bob", 60*time.Second)) // Alice's window is finalized on its own account, long before bob's grace elapses. loop.finalizeOpenWindowWithReason(windowFinalizeReasonIdleTimeout) - require.Nil(t, loop.openWindow, "precondition: nothing is open when bob's grace runs out") + require.Nil(t, loop.windows.only(), "precondition: nothing is open when bob's grace runs out") forceDue(loop) loop.endWake(0) @@ -460,7 +460,7 @@ func TestAttach_ASecondSaveOfTheSameAuthorIsRefusedWhileTheFirstIsInProgress(t * Events: []Event{configMapTargetEvent("cm", "alice", "team-a")}, }}) serviceAttach(loop, attachReq("alice", 60*time.Second)) - require.NotNil(t, loop.openWindow.pendingCR, "precondition: the first save collects the window") + require.NotNil(t, loop.windows.only().pendingCR, "precondition: the first save collects the window") serviceAttach(loop, second()) @@ -510,7 +510,7 @@ func TestAttach_ASecondSaveOfTheSameAuthorIsRefusedWhileTheFirstIsInProgress(t * Events: []Event{configMapTargetEvent("cm", "alice", "team-a")}, }}) serviceAttach(loop, attachReq("alice", 60*time.Second)) - require.NotNil(t, loop.openWindow.pendingCR, "precondition: the first save collects the window") + require.NotNil(t, loop.windows.only().pendingCR, "precondition: the first save collects the window") next := second() next.Attach = configv1alpha3.AttachNext @@ -518,7 +518,7 @@ func TestAttach_ASecondSaveOfTheSameAuthorIsRefusedWhileTheFirstIsInProgress(t * _, resolved := secondOutcome(t, worker) assert.False(t, resolved, "it waits for the next window") - assert.Nil(t, loop.openWindow, "the first save's window was closed under its own message") + assert.Nil(t, loop.windows.only(), "the first save's window was closed under its own message") }) } @@ -537,14 +537,14 @@ func TestAttach_ForeignWindowOpeningAfterExpiryIsNotAMismatch(t *testing.T) { // Bob parks with nothing open, and his grace runs out before anyone else starts work. serviceAttach(loop, attachReq("bob", 60*time.Second)) - require.Nil(t, loop.openWindow, "precondition: nothing was open during bob's grace") + require.Nil(t, loop.windows.only(), "precondition: nothing was open during bob's grace") forceDue(loop) // Only now does alice's work arrive and open a window. loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("cm", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) loop.endWake(0) res, ok := outcome(t, worker) @@ -612,14 +612,14 @@ func TestAttach_AnUnreachableRemoteHoldsTheRequest(t *testing.T) { loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("cm", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) serviceAttach(loop, attachReq("alice", 0)) _, resolved := outcome(t, worker) assert.False(t, resolved, "a request whose write can still land is not failed") assert.Equal(t, PhaseWaitingForPush, worker.LookupCommitRequestPhase("default", crName, "uid-"+crName)) - assert.Nil(t, loop.openWindow, "the window closed on its own timers") + assert.Nil(t, loop.windows.only(), "the window closed on its own timers") require.Len(t, loop.pendingWrites, 1, "the decided window stays pending") assert.False(t, loop.pendingWrites[0].committedOnce, "nothing could commit it yet") assert.True(t, loop.retry.pending(), "the publication retry owns the next attempt") @@ -640,10 +640,10 @@ func TestFinalizeOpenWindow_ReturnsCommittedFlag(t *testing.T) { loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("cm", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) assert.True(t, loop.finalizeOpenWindow(), "open window finalized โ†’ true") - assert.Nil(t, loop.openWindow) + assert.Nil(t, loop.windows.only()) } // TestAttach_NoDiffResolvesAlreadyPresentOnceTheRemoteConfirms pins the corrected contract: a @@ -683,7 +683,7 @@ func TestAttach_NoDiffResolvesAlreadyPresentOnceTheRemoteConfirms(t *testing.T) loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("present", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) serviceAttach(loop, attachReq("alice", 0)) _, ok := outcome(t, worker) @@ -732,7 +732,7 @@ func TestAttach_ResyncCutOffCarriesMessageAndResolvesOnPush(t *testing.T) { loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("held", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) // Attach with a distinctive message and a long grace, so the window is Attached // but its finalize deadline has not fired. @@ -740,7 +740,7 @@ func TestAttach_ResyncCutOffCarriesMessageAndResolvesOnPush(t *testing.T) { req := attachReq("alice", 300*time.Second) req.Message = message serviceAttach(loop, req) - require.NotNil(t, loop.openWindow.pendingCR, "the window must be attached") + require.NotNil(t, loop.windows.only().pendingCR, "the window must be attached") _, resolved := outcome(t, worker) require.False(t, resolved, "the request must not resolve before its window is finalized") @@ -971,7 +971,7 @@ func TestAttach_ReSentAttachDuringPushCooldownDoesNotResolve(t *testing.T) { loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("held", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) serviceAttach(loop, attachReq("alice", 0)) require.Len(t, loop.pendingWrites, 1, "the window committed locally and is retained") @@ -1072,12 +1072,12 @@ func TestAttach_ACommittedRequestNeverClaimsAnotherWindow(t *testing.T) { loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("someone-elses-save", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) loop.endWake(0) - assert.Nil(t, loop.openWindow.pendingCR, + assert.Nil(t, loop.windows.only().pendingCR, "a request whose window is already committed must not claim a second window") - assert.Empty(t, loop.openWindow.pendingMessage, + assert.Empty(t, loop.windows.only().pendingMessage, "and must not lend its message to a commit its author did not make") } diff --git a/internal/git/commit_request_empty_test.go b/internal/git/commit_request_empty_test.go index 881bbfe9..16b3da41 100644 --- a/internal/git/commit_request_empty_test.go +++ b/internal/git/commit_request_empty_test.go @@ -379,7 +379,7 @@ func TestCommitEmpty_AZeroAttachTimeoutStillSeesAnotherAuthorsWindow(t *testing. defer loop.stopTimers() writeTo(loop, "alices-edit") - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) req := commitEmptyReq("bob", "bob's save") req.AttachTimeout = 0 @@ -391,8 +391,8 @@ func TestCommitEmpty_AZeroAttachTimeoutStillSeesAnotherAuthorsWindow(t *testing. assert.Equal(t, FinalizeWindowMismatch, res.Outcome) assert.Empty(t, res.Commit) assert.Empty(t, loop.pendingWrites, "no record was made") - require.NotNil(t, loop.openWindow, "and alice's window is left alone") - assert.Nil(t, loop.openWindow.pendingCR) + require.NotNil(t, loop.windows.only(), "and alice's window is left alone") + assert.Nil(t, loop.windows.only().pendingCR) }) } } @@ -442,7 +442,7 @@ func TestCommitEmpty_ABufferFlushStillServesTheWaitingSaves(t *testing.T) { req.Attach = configv1alpha3.AttachNext serviceAttach(loop, req) writeTo(loop, "oversized") - assert.Nil(t, loop.openWindow, "the buffer limit flushed the window") + assert.Nil(t, loop.windows.only(), "the buffer limit flushed the window") loop.endWake(0) loop.pushPending() diff --git a/internal/git/commit_request_withdraw_test.go b/internal/git/commit_request_withdraw_test.go index f2ffa423..75c259d1 100644 --- a/internal/git/commit_request_withdraw_test.go +++ b/internal/git/commit_request_withdraw_test.go @@ -42,8 +42,8 @@ func TestWithdraw_AWaitingRequestIsCancelledForGood(t *testing.T) { loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("late", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow) - assert.Nil(t, loop.openWindow.pendingCR, "a withdrawn request never attaches") + require.NotNil(t, loop.windows.only()) + assert.Nil(t, loop.windows.only().pendingCR, "a withdrawn request never attaches") assert.Empty(t, loop.pendingCRs) res, _ = outcome(t, worker) require.ErrorIs(t, res.Err, ErrCommitRequestWithdrawn) diff --git a/internal/git/commit_window_clock_test.go b/internal/git/commit_window_clock_test.go index f9b44657..207e3683 100644 --- a/internal/git/commit_window_clock_test.go +++ b/internal/git/commit_window_clock_test.go @@ -38,21 +38,21 @@ func TestWindowClock_TheIdleTimeoutIsMeasuredOnTheWorkersClock(t *testing.T) { holdPushes(loop) // hold the push, so the local commit stays inspectable writeTo(loop, "first") - require.NotNil(t, loop.openWindow) - assert.Equal(t, clock.Now(), loop.openWindow.openedAt, "the window opens at the clock's time") + require.NotNil(t, loop.windows.only()) + assert.Equal(t, clock.Now(), loop.windows.only().openedAt, "the window opens at the clock's time") clock.advance(9 * time.Second) timerWake(loop) - require.NotNil(t, loop.openWindow, "a second short of the idle timeout") + require.NotNil(t, loop.windows.only(), "a second short of the idle timeout") writeTo(loop, "second") // restarts the idle interval clock.advance(9 * time.Second) timerWake(loop) - require.NotNil(t, loop.openWindow, "the second write restarted it") + require.NotNil(t, loop.windows.only(), "the second write restarted it") clock.advance(time.Second) timerWake(loop) - assert.Nil(t, loop.openWindow, "idle for the whole timeout") + assert.Nil(t, loop.windows.only(), "idle for the whole timeout") require.Len(t, loop.pendingWrites, 1) assert.Len(t, loop.pendingWrites[0].Events, 2) } @@ -68,11 +68,11 @@ func TestWindowClock_MaxDurationIsMeasuredOnTheWorkersClock(t *testing.T) { for i, name := range []string{"a", "b", "c"} { writeTo(loop, name) - require.NotNil(t, loop.openWindow, "write %d keeps the window busy", i) + require.NotNil(t, loop.windows.only(), "write %d keeps the window busy", i) clock.advance(9 * time.Second) timerWake(loop) } - assert.Nil(t, loop.openWindow, "27s after it opened, past its 25s maxDuration") + assert.Nil(t, loop.windows.only(), "27s after it opened, past its 25s maxDuration") require.Len(t, loop.pendingWrites, 1) assert.Len(t, loop.pendingWrites[0].Events, 3) } @@ -89,16 +89,16 @@ func TestWindowClock_AnAttachedRequestsTimersRunFromTheAttachOnTheWorkersClock(t req := attachReq("alice", time.Hour) req.MaxDuration = 30 * time.Second serviceAttach(loop, req) - require.NotNil(t, loop.openWindow.pendingCR, "precondition: attached") - assert.Equal(t, clock.Now().Add(30*time.Second), loop.openWindow.timers.maxAt) + require.NotNil(t, loop.windows.only().pendingCR, "precondition: attached") + assert.Equal(t, clock.Now().Add(30*time.Second), loop.windows.only().timers.maxAt) clock.advance(29 * time.Second) timerWake(loop) - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) clock.advance(time.Second) timerWake(loop) - assert.Nil(t, loop.openWindow, "the request's maxDuration ran out") + assert.Nil(t, loop.windows.only(), "the request's maxDuration ran out") res, ok := outcome(t, worker) require.True(t, ok) assert.Equal(t, FinalizeCommitted, res.Outcome) diff --git a/internal/git/commit_window_metrics_test.go b/internal/git/commit_window_metrics_test.go index 17eb389d..3defc365 100644 --- a/internal/git/commit_window_metrics_test.go +++ b/internal/git/commit_window_metrics_test.go @@ -62,7 +62,7 @@ func TestCommitWindowMetrics_AZeroTimerClosesAndCountsTheWindowAtOnce(t *testing writeTo(loop, "first") - assert.Nil(t, loop.openWindow) + assert.Nil(t, loop.windows.only()) assert.Equal(t, int64(1), windowsClosed(t, reader, "idle_timeout", windowTimerSourceTarget)) requireOneObservationPerWindow(t, reader, 1) } @@ -75,23 +75,31 @@ func TestCommitWindowMetrics_EachCloseReasonIsCountedUnderItsOwnName(t *testing. // max_duration: the target's maximum runs out. writeTo(loop, "a") - loop.openWindow.timers.maxAt = time.Now().Add(-time.Millisecond) + loop.windows.only().timers.maxAt = time.Now().Add(-time.Millisecond) loop.closeOrArmWindow() loop.endWake(0) - // identity_change: another author's write arrives while alice's window is open. + // path_conflict: another author writes the file alice's open window holds. writeTo(loop, "b") loop.handleQueueItem(WorkItem{Request: &WriteRequest{ - Events: []Event{configMapTargetEvent("c", "bob", "team-a")}, + Events: []Event{configMapTargetEvent("b", "bob", "team-a")}, }}) - // shutdown: bob's window is open when the worker stops. + // identity_change: once the worker holds one window at a time, another author's write closes + // bob's window whatever it writes. + loop.singleWindow = true + loop.handleQueueItem(WorkItem{Request: &WriteRequest{ + Events: []Event{configMapTargetEvent("c", "carol", "team-a")}, + }}) + + // shutdown: carol's window is open when the worker stops. loop.handleShutdown() assert.Equal(t, int64(1), windowsClosed(t, reader, "max_duration", windowTimerSourceTarget)) + assert.Equal(t, int64(1), windowsClosed(t, reader, "path_conflict", windowTimerSourceTarget)) assert.Equal(t, int64(1), windowsClosed(t, reader, "identity_change", windowTimerSourceTarget)) assert.Equal(t, int64(1), windowsClosed(t, reader, "shutdown", windowTimerSourceTarget)) - requireOneObservationPerWindow(t, reader, 3) + requireOneObservationPerWindow(t, reader, 4) } // TestCommitWindowMetrics_AnAttachKeepsTheOpeningTimeAndCountsTheRequestsTimers pins the two @@ -105,11 +113,11 @@ func TestCommitWindowMetrics_AnAttachKeepsTheOpeningTimeAndCountsTheRequestsTime writeTo(loop, "first") opened := time.Now().Add(-10 * time.Second) // as if the write had arrived ten seconds ago - loop.openWindow.openedAt = opened + loop.windows.only().openedAt = opened serviceAttach(loop, attachReq("alice", time.Hour)) - require.NotNil(t, loop.openWindow.pendingCR) - assert.Equal(t, opened, loop.openWindow.openedAt, "the attach restarts the timers, not the opening time") + require.NotNil(t, loop.windows.only().pendingCR) + assert.Equal(t, opened, loop.windows.only().openedAt, "the attach restarts the timers, not the opening time") forceDue(loop) assert.Equal(t, int64(1), windowsClosed(t, reader, "max_duration", windowTimerSourceCommitRequest)) diff --git a/internal/git/commit_window_timers_test.go b/internal/git/commit_window_timers_test.go index c6bbdd4e..0de0408f 100644 --- a/internal/git/commit_window_timers_test.go +++ b/internal/git/commit_window_timers_test.go @@ -43,16 +43,16 @@ func TestWindowTimers_ContinuousActivityStopsAtTheTargetsMaxDuration(t *testing. holdPushes(loop) // hold the push, so the local commit stays inspectable writeTo(loop, "first") - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) // Rewind the deadline rather than sleeping past a short one: a 20ms maxDuration raced the // opening write itself, which under a loaded test run took longer than 20ms and closed the // window before the assertion above could see it. - require.WithinDuration(t, time.Now().Add(time.Hour), loop.openWindow.timers.maxAt, time.Minute, + require.WithinDuration(t, time.Now().Add(time.Hour), loop.windows.only().timers.maxAt, time.Minute, "the window takes the target's maxDuration when it opens") - loop.openWindow.timers.maxAt = time.Now().Add(-time.Millisecond) + loop.windows.only().timers.maxAt = time.Now().Add(-time.Millisecond) writeTo(loop, "second") // restarts idle, but maxDuration has already passed - assert.Nil(t, loop.openWindow, "a window closes at maxDuration however much keeps arriving") + assert.Nil(t, loop.windows.only(), "a window closes at maxDuration however much keeps arriving") require.Len(t, loop.pendingWrites, 1, "both writes are in the one window it closed") assert.Len(t, loop.pendingWrites[0].Events, 2) } @@ -68,7 +68,7 @@ func TestWindowTimers_ATargetMaxDurationOfZeroCommitsEveryWrite(t *testing.T) { writeTo(loop, "first") writeTo(loop, "second") - assert.Nil(t, loop.openWindow) + assert.Nil(t, loop.windows.only()) assert.Len(t, loop.pendingWrites, 2, "maxDuration 0s closes the window right after the write that opened it") } @@ -82,13 +82,13 @@ func TestWindowTimers_ARequestIdleTimeoutLongerThanTheTargetsKeepsTheWindowOpen( req.IdleTimeout = durationPtr(time.Hour) serviceAttach(loop, req) writeTo(loop, "first") - require.NotNil(t, loop.openWindow.pendingCR, "precondition: attached") + require.NotNil(t, loop.windows.only().pendingCR, "precondition: attached") time.Sleep(20 * time.Millisecond) loop.closeOrArmWindow() // what the commit timer does when it fires loop.endWake(0) - assert.NotNil(t, loop.openWindow, "the request's longer idleTimeout replaces the target's") + assert.NotNil(t, loop.windows.only(), "the request's longer idleTimeout replaces the target's") } func TestWindowTimers_ARequestIdleTimeoutShorterThanTheTargetsClosesSooner(t *testing.T) { @@ -101,13 +101,13 @@ func TestWindowTimers_ARequestIdleTimeoutShorterThanTheTargetsClosesSooner(t *te req.IdleTimeout = durationPtr(5 * time.Millisecond) serviceAttach(loop, req) writeTo(loop, "first") - require.NotNil(t, loop.openWindow.pendingCR, "precondition: attached") + require.NotNil(t, loop.windows.only().pendingCR, "precondition: attached") time.Sleep(20 * time.Millisecond) loop.closeOrArmWindow() loop.endWake(0) - assert.Nil(t, loop.openWindow, "the request's shorter idleTimeout replaces the target's hour") + assert.Nil(t, loop.windows.only(), "the request's shorter idleTimeout replaces the target's hour") } func TestWindowTimers_ARequestWithOnlyMaxDurationIgnoresTheTargetsIdleTimer(t *testing.T) { @@ -122,7 +122,7 @@ func TestWindowTimers_ARequestWithOnlyMaxDurationIgnoresTheTargetsIdleTimer(t *t loop.closeOrArmWindow() loop.endWake(0) - assert.NotNil(t, loop.openWindow, "no idle close: maxDuration alone ends the collection") + assert.NotNil(t, loop.windows.only(), "no idle close: maxDuration alone ends the collection") } func TestWindowTimers_AWaitingRequestAttachesBeforeATargetIdleOfZeroCloses(t *testing.T) { @@ -136,9 +136,9 @@ func TestWindowTimers_AWaitingRequestAttachesBeforeATargetIdleOfZeroCloses(t *te serviceAttach(loop, req) writeTo(loop, "first") - require.NotNil(t, loop.openWindow, + require.NotNil(t, loop.windows.only(), "the request attaches in the step the write opens the window, before the zero timer closes it") - assert.Equal(t, "save", loop.openWindow.pendingMessage) + assert.Equal(t, "save", loop.windows.only().pendingMessage) } func TestWindowTimers_AZeroAttachTimeoutStillCollectsForMaxDuration(t *testing.T) { @@ -152,8 +152,8 @@ func TestWindowTimers_AZeroAttachTimeoutStillCollectsForMaxDuration(t *testing.T req.AttachTimeout = 0 serviceAttach(loop, req) - require.NotNil(t, loop.openWindow, "attachTimeout 0s never forces a zero collection") - assert.NotNil(t, loop.openWindow.pendingCR, "it attached to the window open at registration") + require.NotNil(t, loop.windows.only(), "attachTimeout 0s never forces a zero collection") + assert.NotNil(t, loop.windows.only().pendingCR, "it attached to the window open at registration") _, resolved := outcome(t, worker) assert.False(t, resolved) } @@ -169,7 +169,7 @@ func TestWindowTimers_ARequestMaxDurationOfZeroFinalizesRightAfterTheAttach(t *t req.MaxDuration = 0 serviceAttach(loop, req) - assert.Nil(t, loop.openWindow) + assert.Nil(t, loop.windows.only()) res, ok := outcome(t, worker) require.True(t, ok) assert.Equal(t, FinalizeCommitted, res.Outcome) @@ -186,8 +186,8 @@ func TestAttachSelection_CompetingRequestsAreServedFirstComeFirstServed(t *testi serviceAttach(loop, namedAttachReq("late", time.Minute)) writeTo(loop, "first") - require.NotNil(t, loop.openWindow.pendingCR) - assert.Equal(t, "early", loop.openWindow.pendingCR.Name) + require.NotNil(t, loop.windows.only().pendingCR) + assert.Equal(t, "early", loop.windows.only().pendingCR.Name) } func TestAttachSelection_NextClosesTheOpenWindowUnderItsOwnMessage(t *testing.T) { @@ -201,26 +201,26 @@ func TestAttachSelection_NextClosesTheOpenWindowUnderItsOwnMessage(t *testing.T) current := namedAttachReq("current", time.Hour) current.Message = "the earlier save" serviceAttach(loop, current) - require.NotNil(t, loop.openWindow.pendingCR, "precondition: the earlier save is attached") + require.NotNil(t, loop.windows.only().pendingCR, "precondition: the earlier save is attached") next := namedAttachReq("next", time.Hour) next.Attach = configv1alpha3.AttachNext next.Message = "the new save" serviceAttach(loop, next) - assert.Nil(t, loop.openWindow, "Next closes the author's open window") + assert.Nil(t, loop.windows.only(), "Next closes the author's open window") require.Len(t, loop.pendingWrites, 1) assert.Equal(t, "the earlier save", loop.pendingWrites[0].CommitMessage, "the closed window keeps the message another request attached to it") writeTo(loop, "after") - require.NotNil(t, loop.openWindow) - require.NotNil(t, loop.openWindow.pendingCR) - assert.Equal(t, "next", loop.openWindow.pendingCR.Name, "Next attaches to the window the next write opens") + require.NotNil(t, loop.windows.only()) + require.NotNil(t, loop.windows.only().pendingCR) + assert.Equal(t, "next", loop.windows.only().pendingCR.Name, "Next attaches to the window the next write opens") // A re-send of the same request closes nothing: the close is keyed to its registration. serviceAttach(loop, next) - assert.NotNil(t, loop.openWindow, "a re-send must not close the window it attached to") + assert.NotNil(t, loop.windows.only(), "a re-send must not close the window it attached to") assert.Len(t, loop.pendingWrites, 1) } @@ -265,18 +265,18 @@ func TestWindowTimers_AWaitingRequestWithMaxDurationZeroCommitsExactlyTheNextWri req.MaxDuration = 0 req.Message = "save: exactly the next change" serviceAttach(loop, req) - require.Nil(t, loop.openWindow, "the request waits; it opens nothing") + require.Nil(t, loop.windows.only(), "the request waits; it opens nothing") assert.Equal(t, PhaseWaitingForWindow, worker.LookupCommitRequestPhase("default", crName, "uid-"+crName)) writeTo(loop, "the-change") loop.endWake(0) - assert.Nil(t, loop.openWindow, "the write opened a window, the request attached, and maxDuration: 0s closed it") + assert.Nil(t, loop.windows.only(), "the write opened a window, the request attached, and maxDuration: 0s closed it") require.Len(t, loop.pendingWrites, 1) assert.Equal(t, req.Message, loop.pendingWrites[0].CommitMessage) require.Len(t, loop.pendingWrites[0].Events, 1, "the commit holds that one write") writeTo(loop, "the-next-change") - require.NotNil(t, loop.openWindow, "a later write opens an ordinary window under the target's timers") - assert.Nil(t, loop.openWindow.pendingCR) - assert.Empty(t, loop.openWindow.pendingMessage) + require.NotNil(t, loop.windows.only(), "a later write opens an ordinary window under the target's timers") + assert.Nil(t, loop.windows.only().pendingCR) + assert.Empty(t, loop.windows.only().pendingMessage) } diff --git a/internal/git/footprint.go b/internal/git/footprint.go new file mode 100644 index 00000000..a3d532b3 --- /dev/null +++ b/internal/git/footprint.go @@ -0,0 +1,514 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "errors" + "fmt" + "os" + "path" + "path/filepath" + "slices" + "strings" + + gogit "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing/object" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + itypes "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// A window's footprint is the set of Git files its writes may change. Two windows may be open side +// by side only while their footprints are proven not to overlap: otherwise the one that closes last +// could put an older value of a file over a newer one. See docs/design/concurrent-author-windows.md. +// +// A footprint is proven only where that is cheap and certain, and is unknown everywhere else; unknown +// overlaps everything, so those writes stay serial, one window at a time, as before. A write is +// proven when all of these hold, on the checkout the next commit is planned on: +// +// - the GitTarget declares no placement.default and no useKustomize, and no placement.byType +// template for the object's kind: a template can bundle several objects into one file; +// - its folder reads nothing outside spec.path, feeds no configMapGenerator, and holds no +// kustomization the writer does not model; +// - the object's file is governed by no kustomization (none in its directory or above, none +// naming it), so the write registers nothing in a kustomization.yaml and creates none; +// - the object's document is alone in its file, or the object has none and the built-in path is +// free; the built-in path has no API version, so two versions of one object share it; +// - the folder's bootstrap files are in place, so the commit adds none; +// - the write is a whole object (not a field patch) of a type that is not encrypted. +// +// A proof holds for one revision: the checkout, named by how many resets and own commits it has +// seen, and the GitTarget's generation. Before a write is admitted beside other windows, every open +// window is proven again against the current revision (FootprintReproved). And before a proven +// window's write is committed, every path the commit would change must be inside its footprint: a +// commit that changes more is a footprint bug, and is refused (footprintViolationError). + +// footprint is the set of Git files a window's writes may change, as proven against one revision. +type footprint struct { + // paths are the files, slash-separated and relative to the worktree root, sorted and distinct. + paths []string + // unknown says why the footprint could not be proven; empty when it is. + unknown string + // revision is what the footprint was proven against. + revision footprintRevision +} + +// footprintRevision is what a footprint is proven against: the checkout, named by how many resets +// and own commits the worker has made, and the generation of the window's GitTarget. +type footprintRevision struct { + resets uint64 + commits uint64 + generation int64 +} + +// known reports whether the footprint is proven. +func (f footprint) known() bool { return f.unknown == "" } + +// overlaps reports whether the two footprints may change one file: when either is unknown, or they +// share a path. +func (f footprint) overlaps(other footprint) bool { + if !f.known() || !other.known() { + return true + } + for _, p := range f.paths { + if _, found := slices.BinarySearch(other.paths, p); found { + return true + } + } + return false +} + +// union is every path of both, proven against f's revision; unknown when either is. +func (f footprint) union(other footprint) footprint { + if !f.known() { + return f + } + if !other.known() { + return footprint{unknown: other.unknown, revision: f.revision} + } + paths := slices.Concat(f.paths, other.paths) + slices.Sort(paths) + return footprint{paths: slices.Compact(paths), revision: f.revision} +} + +// outside returns the paths that are not in the footprint. +func (f footprint) outside(paths []string) []string { + var out []string + for _, p := range paths { + if _, found := slices.BinarySearch(f.paths, p); !found { + out = append(out, p) + } + } + return out +} + +// String is the footprint for a log line. +func (f footprint) String() string { + if !f.known() { + return "unknown: " + f.unknown + } + return "[" + strings.Join(f.paths, " ") + "]" +} + +// String is the revision for a log line. +func (r footprintRevision) String() string { + return fmt.Sprintf("resets=%d commits=%d generation=%d", r.resets, r.commits, r.generation) +} + +// footprintString is a window's footprint for a log line: "unproven" for a window that never had +// company, and so never needed one. +func footprintString(f *footprint) string { + if f == nil { + return "unproven" + } + return f.String() +} + +// footprintIndex is what proving a GitTarget's writes needs to know about its folder at one revision. +// The loop keeps one per GitTarget, and builds it again once the revision moves. +type footprintIndex struct { + revision footprintRevision + // unknown says why no write of the GitTarget can be proven at this revision; empty when they can. + unknown string + // base is the GitTarget's folder, slash-relative to the worktree root. + base string + root string + store *manifestanalyzer.ManifestStore + docLoc map[*manifestanalyzer.DocumentModel]manifestanalyzer.RecordRef + policy *manifestanalyzer.PlacementPolicy + // content is every scanned YAML file's bytes, by its path relative to base. + content map[string][]byte + writer eventContentWriter +} + +// currentRevision is the revision a footprint of the GitTarget is proven against now, and whether +// it could be read. +func (l *branchWorkerEventLoop) currentRevision(name, namespace string) (footprintRevision, bool) { + target, err := l.w.getGitTarget(l.w.ctx, name, namespace) + if err != nil { + return footprintRevision{}, false + } + return footprintRevision{ + resets: l.w.checkoutResets.Load(), + commits: l.w.checkoutCommits.Load(), + generation: target.Generation, + }, true +} + +// proveEvents proves the footprint of events, all of one GitTarget, against the current revision, for +// a write the loop admits now: the next commit is planned on the checkout once it holds every decided +// write. +func (l *branchWorkerEventLoop) proveEvents(name, namespace string, events []Event) footprint { + return l.proveOn(name, namespace, events, l.checkoutCurrent() && !l.w.worktreeDirty()) +} + +// proveOn proves the footprint of events against the current revision. planned says whether the +// checkout is the tree their commit is planned on; a proof of any other tree is unknown. +func (l *branchWorkerEventLoop) proveOn(name, namespace string, events []Event, planned bool) footprint { + rev, ok := l.currentRevision(name, namespace) + if !ok { + return footprint{unknown: "the GitTarget cannot be read"} + } + if !planned { + return footprint{unknown: "the checkout does not hold every decided write yet", revision: rev} + } + ix := l.footprintIndexFor(itypes.NewResourceReference(name, namespace), rev, events) + f := footprint{revision: rev} + if ix.unknown != "" { + f.unknown = ix.unknown + return f + } + for i := range events { + file, why := ix.fileFor(events[i]) + if why != "" { + return footprint{unknown: why, revision: rev} + } + if file != "" { + f = f.union(footprint{paths: []string{file}, revision: rev}) + } + } + return f +} + +// proveWindow proves the footprint of every write window holds, against the current revision. +func (l *branchWorkerEventLoop) proveWindow(window *openWindow) footprint { + return l.proveEvents(window.GitTarget, window.GitTargetNamespace, window.orderedEvents()) +} + +// footprintIndexFor returns the GitTarget's index at rev, building it when the one kept is of another +// revision. +func (l *branchWorkerEventLoop) footprintIndexFor( + target itypes.ResourceReference, rev footprintRevision, events []Event, +) *footprintIndex { + if ix := l.footprints[target]; ix != nil && ix.revision == rev { + return ix + } + ix := l.buildFootprintIndex(target, rev, events) + if l.footprints == nil { + l.footprints = map[itypes.ResourceReference]*footprintIndex{} + } + l.footprints[target] = ix + return ix +} + +// buildFootprintIndex scans the GitTarget's folder as the write plan does, and records what decides +// whether a write there can be proven. +func (l *branchWorkerEventLoop) buildFootprintIndex( + target itypes.ResourceReference, rev footprintRevision, events []Event, +) *footprintIndex { + ix := &footprintIndex{revision: rev, writer: l.w.contentWriter} + md, err := l.w.resolveTargetMetadata(l.w.ctx, target.Name, target.Namespace) + if err != nil { + ix.unknown = "the GitTarget cannot be resolved" + return ix + } + ix.policy = md.Placement + if why := wholeTargetUnproven(md.Placement); why != "" { + ix.unknown = why + return ix + } + ix.root, ix.base = l.w.repoPath(), sanitizePath(md.Path) + if _, err := os.Stat(filepath.Join(ix.root, ".git")); err != nil { + ix.unknown = "the repository is not checked out" + return ix + } + if missing := missingBootstrapFile(ix.root, ix.base, md.BootstrapOptions); missing != "" { + ix.unknown = "the folder's bootstrap file " + missing + " is not in Git yet" + return ix + } + + l.w.repoMu.Lock() + scoped, err := scanRenderScope(ix.root, ix.base) + l.w.repoMu.Unlock() + switch { + case err != nil: + ix.unknown = "the folder cannot be scanned" + return ix + case scoped.writeSubdir != "": + ix.unknown = "the folder reads files outside spec.path" + return ix + case len(scoped.scan.InputFiles) > 0: + ix.unknown = "the folder feeds a configMapGenerator" + return ix + } + mapper := l.w.mapperForCluster(clusterIDForEvents(events)) + ix.store = manifestanalyzer.BuildStoreFromScan(l.w.ctx, scoped.scan, mapper, manifestanalyzer.WriterAllowlist(), + manifestanalyzer.WithDeclaredNamespace(md.Namespaces.declaredNamespace())) + for _, k := range ix.store.Kustomizations { + if k.Unsupported { + ix.unknown = "the folder holds a kustomization the writer does not model" + return ix + } + } + ix.docLoc = ix.store.DocumentLocations() + ix.content = make(map[string][]byte, len(scoped.scan.YAMLFiles)) + for _, f := range scoped.scan.YAMLFiles { + ix.content[f.Path] = f.Content + } + return ix +} + +// wholeTargetUnproven says why no write of a GitTarget with this placement can be proven, or "". +func wholeTargetUnproven(policy *manifestanalyzer.PlacementPolicy) string { + switch { + case policy == nil: + return "" + case policy.UseKustomize: + return "placement.useKustomize is on" + case policy.Default != "": + return "placement.default is set" + default: + return "" + } +} + +// missingBootstrapFile names a bootstrap file the next commit to the folder would add, or "". It reads +// the commit HEAD names, not the disk: a write that committed nothing leaves its bootstrap files +// staged, and the next commit adds them. +func missingBootstrapFile(root, base string, options pathBootstrapOptions) string { + if !options.Enabled { + return "" + } + entries, err := bootstrapTemplateFS.ReadDir(bootstrapTemplateDir) + if err != nil { + return bootstrapTemplateDir + } + tree, err := headTree(root) + if err != nil { + return "(HEAD cannot be read)" + } + for _, entry := range entries { + if shouldSkipBootstrapEntry(entry, options) { + continue + } + if _, err := tree.File(path.Join(base, entry.Name())); err != nil { + return entry.Name() + } + } + return "" +} + +// headTree is the tree of the commit HEAD names in the repository at root. +func headTree(root string) (*object.Tree, error) { + repo, err := gogit.PlainOpen(root) + if err != nil { + return nil, err + } + head, err := repo.Head() + if err != nil { + return nil, err + } + commit, err := repo.CommitObject(head.Hash()) + if err != nil { + return nil, err + } + return commit.Tree() +} + +// fileFor is the one file, slash-relative to the worktree root, a write of the event may change: "" +// when it changes none, with why it cannot be proven when it cannot. +func (ix *footprintIndex) fileFor(event Event) (string, string) { + switch { + case event.IsFieldPatch(): + return "", "a field patch" + case ix.writer.isSensitiveIdentifier(event.Identifier): + return "", "an encrypted type" + case event.Operation == string(itypes.OperationDelete): + return ix.deleteFile(event) + default: + return ix.upsertFile(event) + } +} + +// upsertFile is fileFor for a create or an update: the file holding the object's document, or the +// built-in path for an object with none. +func (ix *footprintIndex) upsertFile(event Event) (string, string) { + id, ok := manifestIdentity(event.Object) + if !ok { + return "", "an object without a manifest identity" + } + if dm := ix.store.ByManifestIdentity[id]; dm != nil { + return ix.aloneIn(ix.docLoc[dm].FilePath) + } + if len(ix.store.GeneratedObjectsFor(event.Object)) > 0 { + return "", "an object a configMapGenerator renders" + } + kind := event.Object.GetKind() + if ix.policy != nil && ix.policy.ByType[kind] != "" { + return "", "a placement.byType template for " + kind + } + placement, err := manifestanalyzer.LocateNew(ix.store, ix.policy, manifestanalyzer.PlacementRequest{ + Identifier: event.Identifier, + Kind: kind, + Labels: event.Object.GetLabels(), + }) + switch { + case err != nil: + return "", "a placement that cannot be resolved" + case placement.Source != manifestanalyzer.PlacementSourceBuiltin || placement.Append || + placement.Kustomization != nil: + return "", "a new document placed beside others" + case ix.governed(placement.Path): + return "", "a new document under a kustomization" + } + destination := filepath.Join(ix.root, filepath.FromSlash(ix.base), filepath.FromSlash(placement.Path)) + if _, err := os.Stat(destination); !errors.Is(err, os.ErrNotExist) { + return "", "a new document at a path that is taken" + } + return ix.repoPath(placement.Path), "" +} + +// deleteFile is fileFor for a delete: the file holding the object's document, or none when Git holds +// no document of it. +func (ix *footprintIndex) deleteFile(event Event) (string, string) { + if id, ok := manifestIdentity(event.Object); ok { + if dm := ix.store.ByManifestIdentity[id]; dm != nil { + return ix.aloneIn(ix.docLoc[dm].FilePath) + } + } + if action, found := manifestanalyzer.PlanDelete(ix.store, event.Identifier); found { + return ix.aloneIn(action.Ref.FilePath) + } + if len(ix.store.Kustomizations) > 0 { + // A delete of an object no document holds may still remove a generator's entry. + return "", "a delete of an object no document holds, in a folder with a kustomization" + } + return "", "" +} + +// aloneIn is file, which holds the object's document, when that document is alone in it and no +// kustomization governs it. +func (ix *footprintIndex) aloneIn(file string) (string, string) { + if ix.governed(file) { + return "", "a document under a kustomization" + } + inventory, diags := manifestedit.IndexFile(file, ix.content[file]) + if len(inventory.Records) != 1 { + return "", "a document in a file with others" + } + for _, d := range diags { + if d.Reason == manifestedit.ReasonNotKRM || d.Reason == manifestedit.ReasonInvalidYAML { + return "", "a document in a file with others" + } + } + return ix.repoPath(file), "" +} + +// governed reports whether a kustomization governs file, relative to base: one in its directory or +// above, or one naming it, or a directory above it, among its resources or patches. +func (ix *footprintIndex) governed(file string) bool { + within := func(dir string) bool { return dir == "." || dir == "" || strings.HasPrefix(file, dir+"/") } + for dir, k := range ix.store.Kustomizations { + if within(dir) || slices.Contains(k.Patches, file) { + return true + } + for _, entry := range k.Resources { + if named := path.Clean(path.Join(path.Dir(k.Path), entry)); named == file || within(named) { + return true + } + } + } + return false +} + +// repoPath is file, relative to base, relative to the worktree root. +func (ix *footprintIndex) repoPath(file string) string { + if ix.base == "" { + return file + } + return ix.base + "/" + file +} + +// footprintViolationError refuses a commit that would change files outside its window's footprint. +type footprintViolationError struct { + outside []string + footprint footprint +} + +func (e *footprintViolationError) Error() string { + return fmt.Sprintf("the commit would change %s, outside its window's footprint %s", + strings.Join(e.outside, ", "), e.footprint) +} + +// proveBeforeCommit proves the footprint of the decided write at i again on the tree its first commit +// is planned on, once the checkout holds every write before it: the windows a write's admission +// closed commit before it, and can move what it changes (a delete committed first turns its update +// into a create). The check before the commit is then of a proof of that very tree. +func (l *branchWorkerEventLoop) proveBeforeCommit(i int) { + pendingWrite := &l.pendingWrites[i] + if pendingWrite.footprint == nil || pendingWrite.committedOnce || len(pendingWrite.Events) == 0 { + return + } + name, namespace := pendingWrite.windowTarget() + planned := l.w.checkoutHolds(i) && !l.w.rootParentStale() && !l.w.worktreeDirty() + proven := l.proveOn(name, namespace, pendingWrite.Events, planned) + pendingWrite.footprint = &proven +} + +// checkFootprint refuses the commit about to be made from the staged changes when it changes a file +// outside the write's footprint. It checks a write's first commit only, against a footprint proven +// on the tree the commit is planned on (proveBeforeCommit). A proof of another tree, after a reset +// or a change of the GitTarget, says nothing about this one, and the commit is not checked. +func (w *BranchWorker) checkFootprint(worktree *gogit.Worktree, pendingWrite *PendingWrite, first bool) error { + f := pendingWrite.footprint + if f == nil || !f.known() || !first { + return nil + } + if !w.provenOnCheckout(pendingWrite.Target(), f.revision) { + w.Log.V(1).Info("Not checking a commit against its window's footprint: the proof is of another tree", + "footprint", f.String(), "revision", f.revision.String()) + return nil + } + changed, err := stagedPaths(worktree) + if err != nil { + return fmt.Errorf("read the staged changes: %w", err) + } + if outside := f.outside(changed); len(outside) > 0 { + return &footprintViolationError{outside: outside, footprint: *f} + } + return nil +} + +// provenOnCheckout reports whether rev is the revision the checkout is at now. +func (w *BranchWorker) provenOnCheckout(target ResolvedTargetMetadata, rev footprintRevision) bool { + current, err := w.getGitTarget(w.ctx, target.Name, target.Namespace) + return err == nil && current.Generation == rev.generation && w.checkoutResets.Load() == rev.resets && + w.checkoutCommits.Load() == rev.commits +} + +// stagedPaths is every path the next commit changes: the index against HEAD, sorted. +func stagedPaths(worktree *gogit.Worktree) ([]string, error) { + status, err := worktree.Status() + if err != nil { + return nil, err + } + var paths []string + for p, s := range status { + if s.Staging != gogit.Unmodified && s.Staging != gogit.Untracked { + paths = append(paths, p) + } + } + slices.Sort(paths) + return paths, nil +} diff --git a/internal/git/intake.go b/internal/git/intake.go index 6340596a..e23c0bcf 100644 --- a/internal/git/intake.go +++ b/internal/git/intake.go @@ -173,7 +173,7 @@ func (r *ResyncRequest) charge() int64 { return r.payloadSize() + pendingWriteOv // attached save is in the charge of the window it is attached to, and then of the write that window // became. func (l *branchWorkerEventLoop) heldBytes() int64 { - held := l.commitWindows.windowBytes + l.pendingWritesBytes + l.heldEventBytes + held := l.windows.bytes() + l.pendingWritesBytes + l.heldEventBytes for _, pcr := range l.pendingCRs { if !pcr.attached { held += attachCharge(pcr.message) diff --git a/internal/git/intake_test.go b/internal/git/intake_test.go index 7a7854fa..8f458f76 100644 --- a/internal/git/intake_test.go +++ b/internal/git/intake_test.go @@ -84,7 +84,7 @@ func TestIntake_EveryKindOfHeldWorkCounts(t *testing.T) { "a registered save is charged for what it keeps") held = loop.heldBytes() - loop.windowBytes = 2048 + loop.windows.windowBytes = 2048 assert.Equal(t, held+2048, loop.heldBytes(), "and so is the open window") } @@ -233,10 +233,10 @@ func TestIntake_AWindowChargesAReplacedEventOnce(t *testing.T) { }}) } edit() - require.NotNil(t, loop.openWindow) - once := loop.windowBytes + require.NotNil(t, loop.windows.only()) + once := loop.windows.bytes() edit() - assert.Equal(t, once, loop.windowBytes) + assert.Equal(t, once, loop.windows.bytes()) } // deleteEvent is a live delete for the ledger target, which carries no object. @@ -260,8 +260,8 @@ func TestIntake_AnAttachedSaveStaysCharged(t *testing.T) { require.True(t, f.worker.Enqueue(configMapTargetEvent("opens-window", "alice", ledgerTargetName))) loop.handleQueueItem(<-f.worker.eventQueue) - require.NotNil(t, loop.openWindow) - require.NotNil(t, loop.openWindow.pendingCR, "the save attached to the window") + require.NotNil(t, loop.windows.only()) + require.NotNil(t, loop.windows.only().pendingCR, "the save attached to the window") assert.Greater(t, loop.heldBytes(), waiting, "the window holds the save and its event") require.True(t, loop.finalizeOpenWindow()) diff --git a/internal/git/open_window.go b/internal/git/open_window.go index 5e90fb4e..18dd434b 100644 --- a/internal/git/open_window.go +++ b/internal/git/open_window.go @@ -57,8 +57,13 @@ type openWindow struct { // attach of a CommitRequest. lastWriteAt time.Time - // seq is the window's place in the order windows opened (commitWindows.windowSeq). + // seq is the window's place in the order windows opened (commitWindows.windowSeq): its ID in the + // log lines, unique for the life of the worker and never a metric label. seq uint64 + // footprint is the Git files the window's writes may change, as last proven (footprint.go). Nil + // until the window is first open beside another: a window that never had company needs no proof. + // Once proven it covers every write the window collects, so its commit can be checked. + footprint *footprint // bytes is what the window is charged: its events, and its attached request's message. The set's // windowBytes is the sum of every open window's. bytes int64 diff --git a/internal/git/pending_writes_loop.go b/internal/git/pending_writes_loop.go index 68cdb6c5..a4687852 100644 --- a/internal/git/pending_writes_loop.go +++ b/internal/git/pending_writes_loop.go @@ -177,6 +177,7 @@ func (l *branchWorkerEventLoop) materialize() error { l.w.recordMaterializationFailure(err) return err } + l.proveBeforeCommit(i) err := l.w.commitPendingWrites(l.pendingWrites[i : i+1]) switch { case err == nil: @@ -442,6 +443,10 @@ func (l *branchWorkerEventLoop) settleDropped(pendingWrite PendingWrite, err err func (l *branchWorkerEventLoop) settleFailedChange(pendingWrite PendingWrite, err error) { name, namespace := pendingWrite.windowTarget() collection := sourceCollectionForEvents(pendingWrite.Events) + var violation *footprintViolationError + if errors.As(err, &violation) { + l.fallBackToSingleWindow(pendingWrite, violation) + } if isRefusal, refused := l.w.reportPathRefusal(err, name, namespace, collection); isRefusal { l.w.recordCommitFailure(commitFailureRefused) l.reapplyForRefusal(name, namespace, err.Error(), refused, diff --git a/internal/git/push_notification_test.go b/internal/git/push_notification_test.go index f7ca39cb..f552aa76 100644 --- a/internal/git/push_notification_test.go +++ b/internal/git/push_notification_test.go @@ -48,7 +48,7 @@ func TestPushNotice_ARunOfReportsQueuesOneLook(t *testing.T) { Spec: configv1alpha3.GitProviderSpec{URL: "https://github.com/acme/platform"}, }) loop.w.recordRemoteObservation("ours", ObservedByPush) - loop.openWindow = &openWindow{} + loop.windows.setOnly(&openWindow{}) for _, after := range tc.reports { loop.w.EnqueuePushNotification(after) diff --git a/internal/git/refresh.go b/internal/git/refresh.go index 4c14330d..2de729b6 100644 --- a/internal/git/refresh.go +++ b/internal/git/refresh.go @@ -131,7 +131,7 @@ func (l *branchWorkerEventLoop) handleRefreshRequest(req *RefreshRequest) { // commits, and the worktree may hold a partial write โ€” which is also why this is the one exit // that does not re-read the folder: a layout resolved from a half-written tree is worse than // a slightly old one. A target mid-cycle is writing, and a write publishes its own layout. - if len(l.pendingWrites) > 0 || l.commitWindows.anyOpen() || w.worktreeDirty() { + if len(l.pendingWrites) > 0 || l.windows.anyOpen() || w.worktreeDirty() { l.noteBusyPushNotice(req, observed, known) w.Log.V(1).Info("Skipping refresh: this branch is mid-cycle", "branch", w.Branch, "gitTarget", req.Target.String()) diff --git a/internal/git/render_fidelity_drops_test.go b/internal/git/render_fidelity_drops_test.go index 96c4ba9a..a2215f9e 100644 --- a/internal/git/render_fidelity_drops_test.go +++ b/internal/git/render_fidelity_drops_test.go @@ -94,7 +94,7 @@ func TestRenderFidelity_ALiveEventQueuedBehindAReplayResyncIsKept(t *testing.T) // Nothing has read req.Result: the watch layer's drain, on its own goroutine, is still behind. f.liveEvent("created-after-the-replay") - assert.NotNil(t, f.loop.openWindow, "the event queued behind the replay is kept") + assert.NotNil(t, f.loop.windows.only(), "the event queued behind the replay is kept") assert.True(t, f.gate.AllowsWrites(f.target), "the loop recorded the scope before answering") assert.Zero(t, writesDropped(t, reader, writeDropRenderFidelityPending)) require.NoError(t, (<-req.Result).Err) @@ -153,7 +153,7 @@ func TestRenderFidelity_ADroppedWriteIsCountedAndLoggedOncePerGateStatus(t *test f.liveEvent("first") f.liveEvent("second") - assert.Nil(t, f.loop.openWindow, "a pending scope closes the GitTarget") + assert.Nil(t, f.loop.windows.only(), "a pending scope closes the GitTarget") require.NotNil(t, f.loop.held[f.target]) assert.Len(t, f.loop.held[f.target].events, 2, "its live writes are held, not dropped") assert.Len(t, logged.matching("Holding the GitTarget's live writes"), 1, "the hold is said once") @@ -167,7 +167,7 @@ func TestRenderFidelity_ADroppedWriteIsCountedAndLoggedOncePerGateStatus(t *test assert.Nil(t, f.loop.held[f.target], "a divergence drops what was held") assert.Equal(t, int64(3), writesDropped(t, reader, writeDropRenderFidelityDiverged)) assert.Len(t, logged.matching("Dropped a write"), 1, "the drops under one gate status log once") - assert.Nil(t, f.loop.openWindow) + assert.Nil(t, f.loop.windows.only()) } // infoLines collects the worker's Info-level log lines. diff --git a/internal/git/render_fidelity_gate_test.go b/internal/git/render_fidelity_gate_test.go index 3c47ba6d..8ad1e333 100644 --- a/internal/git/render_fidelity_gate_test.go +++ b/internal/git/render_fidelity_gate_test.go @@ -105,12 +105,12 @@ func TestRenderFidelityGate_PendingScopeCannotOpenLiveWindow(t *testing.T) { event := Event{GitTargetName: target.Name, GitTargetNamespace: target.Namespace, Operation: "UPDATE"} loop.handleQueueItem(WorkItem{Request: &WriteRequest{Events: []Event{event}}}) - assert.Nil(t, loop.openWindow, "Unknown must block a new live window") + assert.Nil(t, loop.windows.only(), "Unknown must block a new live window") _, applied := gate.RecordScopeClean(target, revisions[scope], scope) require.True(t, applied) loop.handleQueueItem(WorkItem{Request: &WriteRequest{Events: []Event{event}}}) - assert.NotNil(t, loop.openWindow, "every scope reporting clean reopens normal writes") + assert.NotNil(t, loop.windows.only(), "every scope reporting clean reopens normal writes") } // restartAll is the whole-plan reconcile: every scope is restarted, which is what a forced diff --git a/internal/git/render_fidelity_hold_test.go b/internal/git/render_fidelity_hold_test.go index aca6f7e9..b3981da2 100644 --- a/internal/git/render_fidelity_hold_test.go +++ b/internal/git/render_fidelity_hold_test.go @@ -36,11 +36,11 @@ func (f *replayFixture) send(events ...Event) { // windowNames is the objects the open window holds, in the order they were added. func (f *replayFixture) windowNames() []string { - if f.loop.openWindow == nil { + if f.loop.windows.only() == nil { return nil } var names []string - for _, event := range f.loop.openWindow.orderedEvents() { + for _, event := range f.loop.windows.only().orderedEvents() { names = append(names, event.Identifier.Name) } return names @@ -57,7 +57,7 @@ func TestRenderFidelityHold_AnEditMadeWhileAnotherCollectionReplaysIsCommittedBy f := newReplayFixture(t, "render-fidelity-hold-window-b") f.send(f.streamEvent("edited", "alice", itypes.OperationUpdate)) - require.Nil(t, f.loop.openWindow) + require.Nil(t, f.loop.windows.only()) require.NotNil(t, f.loop.holdTimer, "the held GitTarget's gate is looked at again") assert.Positive(t, f.loop.heldBytes(), "held writes count against the intake budget") @@ -66,7 +66,7 @@ func TestRenderFidelityHold_AnEditMadeWhileAnotherCollectionReplaysIsCommittedBy f.loop.endWake(0) // the recheck assert.Equal(t, []string{"edited"}, f.windowNames(), "released once the gate opened") - assert.Equal(t, "alice", f.loop.openWindow.Author, "with its author") + assert.Equal(t, "alice", f.loop.windows.only().Author, "with its author") assert.Empty(t, f.loop.held) assert.Zero(t, f.loop.heldEventBytes) assert.Zero(t, writesDropped(t, reader, writeDropRenderFidelityPending)) @@ -82,10 +82,14 @@ func TestRenderFidelityHold_HeldEditsGoBeforeLaterOnes(t *testing.T) { f.send(f.streamEvent("second", "bob", itypes.OperationUpdate)) - require.NotNil(t, f.loop.openWindow) - assert.Equal(t, "bob", f.loop.openWindow.Author, - "alice's held edit went first, and bob's closed her window") - assert.Equal(t, []string{"second"}, f.windowNames()) + // The two edits change different files, so their windows stay open side by side, in the order + // the edits were admitted. + open := f.loop.windows.all() + require.Len(t, open, 2) + assert.Equal(t, "alice", open[0].Author, "alice's held edit went first") + assert.Equal(t, "first", open[0].orderedEvents()[0].Identifier.Name) + assert.Equal(t, "bob", open[1].Author) + assert.Equal(t, "second", open[1].orderedEvents()[0].Identifier.Name) } // TestRenderFidelityHold_ASnapshotSupersedesTheEditsOfItsCollectionHeldBeforeIt: the replay resync @@ -144,7 +148,7 @@ func TestRenderFidelityHold_ACreateHeldIsAdoptedOnceItIsInAWrite(t *testing.T) { created := configMapTargetEvent("created", "alice", echoTarget) created.SourceCollection = echoScope().Collection f.writeWindow(created) - require.Nil(t, f.loop.openWindow, "held") + require.Nil(t, f.loop.windows.only(), "held") require.Empty(t, requester.taken(), "nothing to adopt yet: the create is in no commit") return f, requester, gate, revisions[echoScope().Collection] } @@ -358,6 +362,6 @@ func TestRenderFidelityHold_ADivergenceAfterAReplayDropsWhatWasHeld(t *testing.T f.loop.endWake(0) assert.Empty(t, f.loop.held) - assert.Nil(t, f.loop.openWindow) + assert.Nil(t, f.loop.windows.only()) assert.Equal(t, int64(1), writesDropped(t, reader, writeDropRenderFidelityDiverged)) } diff --git a/internal/git/replay_failure_test.go b/internal/git/replay_failure_test.go index 86bd04c8..fae347fa 100644 --- a/internal/git/replay_failure_test.go +++ b/internal/git/replay_failure_test.go @@ -128,7 +128,7 @@ func TestReplayFailure_DoesNotResolveACommitRequestThatNeverLanded(t *testing.T) loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("saved-by-hand", "alice", ledgerTargetName)}, }}) - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) req := attachReq("alice", 0) req.GitTargetName = ledgerTargetName req.Message = "save: this must not be reported as committed" diff --git a/internal/git/replay_refusal_test.go b/internal/git/replay_refusal_test.go index 85acb01b..f91b693e 100644 --- a/internal/git/replay_refusal_test.go +++ b/internal/git/replay_refusal_test.go @@ -53,7 +53,7 @@ func retainTwoWrites(t *testing.T, loop *branchWorkerEventLoop) { loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("refused-on-replay", "alice", ledgerTargetName)}, }}) - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) req := attachReq("alice", 0) req.GitTargetName = ledgerTargetName req.Message = "save: lands in a folder that is refused before it is pushed" diff --git a/internal/git/resync_flush.go b/internal/git/resync_flush.go index 066aaba1..3395f190 100644 --- a/internal/git/resync_flush.go +++ b/internal/git/resync_flush.go @@ -45,7 +45,7 @@ func (l *branchWorkerEventLoop) applyResync(req *ResyncRequest) { "resourceVersion", req.ResourceVersion, "scope", req.Scope.String(), "gitTarget", req.GitTargetNamespace+"/"+req.GitTargetName, - "openWindow", l.commitWindows.anyOpen(), + "openWindow", l.windows.anyOpen(), "pendingWrites", len(l.pendingWrites)) // A resync force-finalizes every open window to preserve arrival order before its mark-and-sweep. l.finalizeOpenWindowWithReason(windowFinalizeReasonResyncBeforeApply) diff --git a/internal/git/resync_push_test.go b/internal/git/resync_push_test.go index 03e24b21..8ebe49d6 100644 --- a/internal/git/resync_push_test.go +++ b/internal/git/resync_push_test.go @@ -328,7 +328,7 @@ func TestHandleResyncRequest_ClosedWindowIsPushedEvenWhenNoOpResync(t *testing.T loop.handleQueueItem(WorkItem{Request: &WriteRequest{ Events: []Event{configMapTargetEvent("held", "alice", "team-a")}, }}) - require.NotNil(t, loop.openWindow, "the edit must open a window") + require.NotNil(t, loop.windows.only(), "the edit must open a window") // A type-scoped resync for a DIFFERENT type with an empty desired set: it closes // the open window (resync-before-apply) but its own mark-and-sweep โ€” scoped to a diff --git a/internal/git/types.go b/internal/git/types.go index 392fc6ad..f34b01e5 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -433,6 +433,11 @@ type PendingWrite struct { decision uint64 // origin is what the write's outcome is settled against. See writeOrigin. origin writeOrigin + // footprint is the footprint of the window the write closed, when that window was ever open + // beside another, proven again on the tree its first commit is planned on (proveBeforeCommit): + // that commit may change no file outside it (footprint.go). Nil for every other write. + // Loop-goroutine only. + footprint *footprint // CommitSHA is the hash of the commit this write created, captured in // executePendingWrite and refreshed when the write is re-executed on a diff --git a/internal/git/window_admission.go b/internal/git/window_admission.go new file mode 100644 index 00000000..f86fbe0f --- /dev/null +++ b/internal/git/window_admission.go @@ -0,0 +1,196 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + itypes "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// This file is how a live write is admitted to the open commit windows: which windows it joins, +// opens beside, or closes first. See docs/design/concurrent-author-windows.md. +// +// A write joins the open window of its author, attribution and GitTarget, or opens one. Windows of +// other authors stay open beside it while every footprint is proven and none overlaps (footprint.go). +// Otherwise the windows it cannot sit beside close first, oldest first, so their commits land before +// its own: +// +// - every other window, with close reason identity_change, when the write is of another GitTarget, +// when its footprint or theirs cannot be proven, or once the worker fell back to one window; +// - the windows whose footprint overlaps the write's, with close reason path_conflict. +// +// Before a write is admitted beside others, every open window is proven against the current revision +// (FootprintReproved); windows whose new proofs overlap close first, with path_conflict. +// +// The decisions have names in the log lines, and the reference model reads them: WindowOpened, +// WriteJoinedWindow, FootprintReproved, WindowClosed (with its reason), and SingleWindowFallback, +// which is about the worker rather than a window. They name diagnostics, not a journal. + +const ( + decisionWindowOpened = "WindowOpened" + decisionWriteJoinedWindow = "WriteJoinedWindow" + decisionFootprintReproved = "FootprintReproved" + decisionWindowClosed = "WindowClosed" + decisionSingleWindowFallback = "SingleWindowFallback" +) + +// admitWrite closes every open window the write cannot be admitted beside, and returns the footprint +// the window it then joins or opens takes: nil for a window that has never had company. +func (l *branchWorkerEventLoop) admitWrite(event Event) *footprint { + own := l.windows.forWrite(event) + others := l.windows.others(own) + if len(others) == 0 { + return l.extendedFootprint(own, event) + } + if why := l.serialAdmission(event, others); why != "" { + for _, other := range others { + l.closeBeforeWrite(other, event, windowFinalizeReasonIdentityChange, why) + } + return l.extendedFootprint(own, event) + } + + // Every proof first, while the checkout still holds every decided write: closing a window below + // decides another. + write := l.proveEvents(event.GitTargetName, event.GitTargetNamespace, []Event{event}) + l.reproveOpenWindows(write.revision) + l.closeConflictingWindows() + + own = l.windows.forWrite(event) + for _, other := range l.windows.others(own) { + switch { + case !write.known(): + l.closeBeforeWrite(other, event, windowFinalizeReasonIdentityChange, + "the write's footprint is "+write.String()) + case !other.footprint.known(): + l.closeBeforeWrite(other, event, windowFinalizeReasonIdentityChange, + "the window's footprint is "+other.footprint.String()) + case other.footprint.overlaps(write): + l.closeBeforeWrite(other, event, windowFinalizeReasonPathConflict, + "the write's footprint "+write.String()+" overlaps the window's "+other.footprint.String()) + } + } + if own == nil { + return &write + } + joined := own.footprint.union(write) + return &joined +} + +// serialAdmission says why a write cannot be admitted beside other windows whatever their +// footprints, or "". +func (l *branchWorkerEventLoop) serialAdmission(event Event, others []*openWindow) string { + switch { + case l.singleWindow: + return "the worker holds one window at a time" + case others[0].GitTarget != event.GitTargetName || others[0].GitTargetNamespace != event.GitTargetNamespace: + // Every open window is of one GitTarget: per-target encryption and bootstrap stay serial. + return "the write is of another GitTarget" + default: + return "" + } +} + +// extendedFootprint is the footprint own takes with the write, while no other window is open: a +// window once proven keeps covering every write it collects, so its commit can be checked. A window +// never proven, and a new one, stay unproven. +func (l *branchWorkerEventLoop) extendedFootprint(own *openWindow, event Event) *footprint { + if own == nil || own.footprint == nil { + return nil + } + write := l.proveEvents(event.GitTargetName, event.GitTargetNamespace, []Event{event}) + if own.footprint.revision != write.revision { + l.reprove(own, write.revision) + } + joined := own.footprint.union(write) + return &joined +} + +// reproveOpenWindows proves every open window again whose footprint is not proven against rev. +func (l *branchWorkerEventLoop) reproveOpenWindows(rev footprintRevision) { + for _, window := range l.windows.all() { + if window.footprint == nil || window.footprint.revision != rev { + l.reprove(window, rev) + } + } +} + +// reprove proves window's footprint again, from every write it holds. +func (l *branchWorkerEventLoop) reprove(window *openWindow, rev footprintRevision) { + proven := l.proveWindow(window) + window.footprint = &proven + l.w.Log.Info("Commit window footprint proven against a new revision", + "window", window.seq, + "branch", l.w.Branch, + "revision", rev.String(), + "footprint", proven.String(), + "decision", decisionFootprintReproved) +} + +// closeConflictingWindows closes, oldest first, every open window whose footprint overlaps another +// open window's, once more than one is open. +func (l *branchWorkerEventLoop) closeConflictingWindows() { + open := l.windows.all() + if len(open) <= 1 { + return + } + involved := map[*openWindow]bool{} + for i, a := range open { + for _, b := range open[i+1:] { + if a.footprint.overlaps(*b.footprint) { + involved[a], involved[b] = true, true + } + } + } + for _, window := range open { + if !involved[window] { + continue + } + l.w.Log.Info("Commit window footprint overlaps another open window's; finalizing it", + "window", window.seq, + "branch", l.w.Branch, + "windowAuthor", window.Author, + "footprint", window.footprint.String()) + l.finalizeWindow(window, windowFinalizeReasonPathConflict) + } +} + +// closeBeforeWrite closes a window the write cannot be admitted beside. +func (l *branchWorkerEventLoop) closeBeforeWrite( + window *openWindow, event Event, reason windowFinalizeReason, why string, +) { + if !l.windows.isOpen(window) { + return + } + // The identities and why make an unexpected split diagnosable. The common cause used to be an + // incoming event whose author is empty (an unattributed update, e.g. a /status change that + // carries no audit fact) arriving against a non-empty window author. + l.w.Log.Info("A write cannot be admitted beside an open commit window; finalizing it first", + "window", window.seq, + "branch", l.w.Branch, + "reason", string(reason), + "why", why, + "windowAuthor", window.Author, + "eventAuthor", event.UserInfo.Username, + "operation", event.Operation, + "resource", event.Identifier.String(), + "eventTarget", event.GitTargetNamespace+"/"+event.GitTargetName) + l.finalizeWindow(window, reason) +} + +// fallBackToSingleWindow is the answer to a commit refused for changing a file outside its window's +// footprint: the footprint rules are wrong somewhere, so the worker stops trusting them and holds one +// window at a time for the rest of its life, and a resync of the GitTarget brings Git back to the +// cluster whatever order the windows closed in. +func (l *branchWorkerEventLoop) fallBackToSingleWindow(pendingWrite PendingWrite, violation *footprintViolationError) { + name, namespace := pendingWrite.windowTarget() + l.singleWindow = true + l.w.Log.Error(violation, "A commit would change files outside its window's footprint; refused it, holding one "+ + "commit window at a time from now on, and asking for a resync of the GitTarget", + "branch", l.w.Branch, + "gitTarget", namespace+"/"+name, + "outside", violation.outside, + "footprint", violation.footprint.String(), + "decision", decisionSingleWindowFallback) + if l.w.resyncRecheck != nil { + l.w.resyncRecheck(itypes.NewResourceReference(name, namespace)) + } +} diff --git a/internal/git/window_admission_test.go b/internal/git/window_admission_test.go new file mode 100644 index 00000000..fc2f75bb --- /dev/null +++ b/internal/git/window_admission_test.go @@ -0,0 +1,195 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + itypes "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// The tests here pin docs/design/concurrent-author-windows.md: a window per author while their +// footprints are proven not to overlap, and serial everywhere else. + +// newAdmissionFixture is an echo fixture whose folder the worker has bootstrapped with a commit of its +// own, so writes there can be proven, and whose windows stay open for an hour unless something closes +// them. Pushes are held. +func newAdmissionFixture(t *testing.T, slug string, files map[string]string) *echoFixture { + t.Helper() + f := newEchoFixtureWith(t, slug, configv1alpha3.PruneOnEvent, + map[string]string{"x": "x1", "z": "z1", "q": "q1"}, files) + holdPushes(f.loop) + f.writeWindow(echoEvent("q", "q2")) // the zero window commits at once, with the bootstrap files + require.Empty(t, f.loop.windows.all()) + f.loop.defaultWindow = commitWindowDefaults{idleTimeout: time.Hour, maxDuration: time.Hour} + return f +} + +func (f *echoFixture) writeBy(author, name, value string) { + event := echoEvent(name, value) + event.UserInfo.Username = author + f.loop.handleQueueItem(WorkItem{Request: &WriteRequest{Events: []Event{event}}}) +} + +func authorsOf(windows []*openWindow) []string { + var authors []string + for _, w := range windows { + authors = append(authors, w.Author) + } + return authors +} + +func TestWindowAdmission_AuthorsOfDisjointFilesKeepAWindowEach(t *testing.T) { + f := newAdmissionFixture(t, "admission-disjoint", nil) + + f.writeBy("alice", "x", "x2") + f.writeBy("bob", "z", "z2") + f.writeBy("alice", "x", "x3") + + open := f.loop.windows.all() + require.Equal(t, []string{"alice", "bob"}, authorsOf(open), "oldest first") + require.NotNil(t, open[0].footprint) + assert.Equal(t, []string{echoFile("x")}, open[0].footprint.paths) + assert.Equal(t, []string{echoFile("z")}, open[1].footprint.paths) + assert.Less(t, open[0].seq, open[1].seq) + + // Bob's window closes first; commits land in close order. + open[1].timers.maxAt = time.Now().Add(-time.Millisecond) + f.loop.closeOrArmWindow() + f.loop.endWake(0) + require.Equal(t, []string{"alice"}, authorsOf(f.loop.windows.all())) + f.loop.finalizeOpenWindowWithReason(windowFinalizeReasonShutdown) + f.loop.endWake(0) + f.loop.pushPending() + + assert.Equal( + t, + []string{"bob", "alice"}, + splitLines(gitIn(t, f.repoDir, "log", "--reverse", "--format=%an", "-2", "main")), + ) + assert.Equal(t, echoDocument("x", "x3"), f.onMain(echoFile("x"))) + assert.Equal(t, echoDocument("z", "z2"), f.onMain(echoFile("z"))) +} + +func TestWindowAdmission_AWriteToAFileAnotherWindowHoldsClosesThatWindowFirst(t *testing.T) { + f := newAdmissionFixture(t, "admission-conflict", nil) + reader := captureInfo(f.worker) + + f.writeBy("alice", "x", "x2") + f.writeBy("bob", "z", "z2") + f.writeBy("carol", "x", "x3") + + assert.Equal(t, []string{"bob", "carol"}, authorsOf(f.loop.windows.all())) + require.Len(t, f.loop.pendingWrites, 2, "the bootstrap write, and alice's window") + assert.Equal(t, "alice", f.loop.pendingWrites[1].Author(), "decided before carol's write opened a window") + assert.Len( + t, + reader.matching(`"msg"="Finalizing open commit window" "reason"="path-conflict" "windowAuthor"="alice"`), + 1, + ) +} + +func TestWindowAdmission_AWriteThatCannotBeProvenStaysSerial(t *testing.T) { + shared := echoDocument("s1", "a") + "---\n" + echoDocument("s2", "b") + f := newAdmissionFixture(t, "admission-unproven", + map[string]string{echoTarget + "/default/configmaps/shared.yaml": shared}) + + f.writeBy("alice", "x", "x2") + f.writeBy("bob", "s1", "c") + + open := f.loop.windows.all() + require.Equal(t, []string{"bob"}, authorsOf(open), "a document in a shared file closes every other window") + require.NotNil(t, open[0].footprint) + assert.False(t, open[0].footprint.known()) + assert.Equal(t, "alice", f.loop.pendingWrites[len(f.loop.pendingWrites)-1].Author()) + + // And an unknown window closes before a write is admitted beside it. + f.writeBy("carol", "z", "z2") + assert.Equal(t, []string{"carol"}, authorsOf(f.loop.windows.all())) +} + +func TestWindowAdmission_AWorkerThatFellBackHoldsOneWindow(t *testing.T) { + f := newAdmissionFixture(t, "admission-single", nil) + f.loop.singleWindow = true + + f.writeBy("alice", "x", "x2") + f.writeBy("bob", "z", "z2") + + assert.Equal(t, []string{"bob"}, authorsOf(f.loop.windows.all())) +} + +func TestWindowAdmission_ClosingOneWindowArmsTheTimerForTheOthers(t *testing.T) { + f := newAdmissionFixture(t, "admission-rearm", nil) + clock := useTestClock(f.worker) + + f.writeBy("alice", "x", "x2") + clock.advance(time.Minute) + f.writeBy("bob", "z", "z2") + open := f.loop.windows.all() + require.Len(t, open, 2) + aliceAt, _ := open[0].closeAt() + bobAt, _ := open[1].closeAt() + require.Equal(t, aliceAt, f.loop.commitTimerAt, "armed for the earliest deadline") + + f.loop.finalizeWindow(open[0], windowFinalizeReasonAttachNext) + + require.NotNil(t, f.loop.commitTimer, "bob's window still has a deadline to keep") + assert.Equal(t, bobAt, f.loop.commitTimerAt) +} + +func TestWindowAdmission_ACommitOutsideItsFootprintIsRefusedAndTheWorkerFallsBack(t *testing.T) { + f := newAdmissionFixture(t, "admission-violation", nil) + var asked []itypes.ResourceReference + f.worker.resyncRecheck = func(target itypes.ResourceReference) { asked = append(asked, target) } + head := gitIn(t, f.worker.repoPath(), "rev-parse", "HEAD") + + pendingWrite, err := f.worker.buildGroupedPendingWrite(f.worker.ctx, []Event{echoEvent("x", "x9")}) + require.NoError(t, err) + rev, ok := f.loop.currentRevision(echoTarget, "default") + require.True(t, ok) + // A proof that names the wrong file, as a footprint bug would. + pendingWrite.footprint = &footprint{paths: []string{echoFile("z")}, revision: rev} + id := commitRequestID{Namespace: "default", Name: crName, UID: "uid-" + crName} + pendingWrite.CommitRequest = &id + + err = f.worker.commitPendingWrites([]PendingWrite{*pendingWrite}) + var violation *footprintViolationError + require.ErrorAs(t, err, &violation) + assert.Equal(t, []string{echoFile("x")}, violation.outside) + require.ErrorIs(t, err, errWriteFailed, "terminal for the write") + assert.Equal(t, head, gitIn(t, f.worker.repoPath(), "rev-parse", "HEAD"), "nothing was committed") + assert.Empty(t, gitIn(t, f.worker.repoPath(), "status", "--porcelain"), "and the worktree was undone") + + f.loop.pendingWrites = []PendingWrite{*pendingWrite} + f.loop.settleFailed(0, err) + + assert.True(t, f.loop.singleWindow, "one window at a time for the rest of the worker's life") + assert.Equal(t, []itypes.ResourceReference{itypes.NewResourceReference(echoTarget, "default")}, asked, + "a resync brings Git back to the cluster") + res, resolved := outcome(t, f.worker) + require.True(t, resolved) + require.Error(t, res.Err) + assert.Contains(t, res.Err.Error(), echoFile("x"), "the save says which path broke the footprint") +} + +func TestFootprint_OverlapUnionAndOutside(t *testing.T) { + a := footprint{paths: []string{"a", "b"}} + b := footprint{paths: []string{"c"}} + unknown := footprint{unknown: "why"} + + assert.False(t, a.overlaps(b)) + assert.True(t, a.overlaps(footprint{paths: []string{"b"}})) + assert.True(t, a.overlaps(unknown), "unknown overlaps everything") + assert.True(t, unknown.overlaps(footprint{})) + assert.Equal(t, []string{"a", "b", "c"}, a.union(b).paths) + assert.False(t, a.union(unknown).known()) + assert.Equal(t, []string{"d"}, a.outside([]string{"a", "d"})) + assert.Equal(t, "[a b]", a.String()) + assert.Equal(t, "unknown: why", unknown.String()) + assert.Equal(t, "unproven", footprintString(nil)) +} diff --git a/internal/git/window_model_test.go b/internal/git/window_model_test.go index 01abdc6e..7c40d71f 100644 --- a/internal/git/window_model_test.go +++ b/internal/git/window_model_test.go @@ -3,10 +3,14 @@ package git import ( + "bytes" + "errors" "fmt" + "io" "maps" "math/rand" "os" + "path/filepath" "reflect" "regexp" "slices" @@ -15,6 +19,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" "gopkg.in/yaml.v3" configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" @@ -22,26 +27,36 @@ import ( ) // The reference model of docs/design/concurrent-author-windows.md ("How to test it"): a small, -// independent implementation of today's single-window rules, fed the same random inputs as a branch -// worker on a real Git server, with the worker checked against it. +// independent implementation of the window rules, fed the same random inputs as a branch worker on a +// real Git server, with the worker checked against it. // -// - After every step: the open window (author, objects, attached save, deadline, charge), the timers -// armed for it and for the waiting saves, the registered saves (attached or waiting, and until -// when), the retained-byte charge, the close reasons so far, and every save's outcome are the -// model's. +// - After every step: the open windows, oldest first (each with its number, author, objects, +// attached save, deadline, charge and footprint), the timers armed for them and for the waiting +// saves, the registered saves (attached or waiting, and until when), the retained-byte charge, +// the close reasons so far, and every save's outcome are the model's. // - Every commit the worker publishes is the one the model's closing window makes: its author, its -// message, every path it changes, and each document whole. Per document, the writes that land do -// so in the order they were accepted, so an older write never lands after a newer one. +// message, every path it changes, and each file whole, its documents parsed. Per document, the +// writes that land do so in the order they were accepted, so an older write never lands after a +// newer one. A commit of a window that was open beside another changes no path outside the +// window's footprint. // - Every save settles exactly once, or is given back at shutdown. // - Drained, Git holds the model's tree, path for path, and nothing is charged. // +// The folder holds objects in three shapes: x, w and z each alone in its own file at the built-in +// path; k in a kustomize root (kust/, which also takes every object created anew, since it is the +// folder's one render root); and s1 and s2 in one shared file. The model proves footprints by its +// own rules: an object alone in a file no kustomization governs has that file; anything else, a +// create included, is unknown, and never opens beside another window. There is no object y: kustomize +// reads a bare y as a boolean, so the render of the root refuses a ConfigMap named y once it lands +// there. +// // The model shares no window code with the worker: only the event fixtures, the per-event charge // (eventRetainedCharge, attachCharge) and the close-reason names. The worker's timers are real; the // driver steps the worker's clock to the time each armed timer is due and delivers that timer's wake, // so a deadline the worker armed no timer for is never delivered. // // WINDOW_MODEL_SEEDS and WINDOW_MODEL_STEPS raise the search; a failure names its seed, and -// WINDOW_MODEL_SEED replays one. +// WINDOW_MODEL_SEED replays one. WINDOW_MODEL_LOGS prints the worker's log with a failure. const ( // modelIdle and modelMax are the commit window the GitTarget declares none of, so the loop's default. @@ -50,13 +65,159 @@ const ( ) var ( - modelObjects = []string{"x", "y", "z"} + modelObjects = []string{"x", "w", "z", "k", "s1", "s2"} modelAuthors = []string{"alice", "bob", "carol"} // modelBootstrap is what the worker adds to the folder, from its template, with the first commit // it makes there. The GitTarget encrypts nothing, so there is no .sops.yaml. modelBootstrap = []string{gitTargetIgnoreFileName, "README.md"} + // modelKustDir is the folder's one kustomize root, and modelKustFile its kustomization. + modelKustDir = echoTarget + "/kust" + modelKustFile = modelKustDir + "/kustomization.yaml" + // modelSharedFile holds s1 and s2. + modelSharedFile = echoTarget + "/default/configmaps/shared.yaml" ) +// modelTree is a Git tree of the folder: each object's value, the files and the objects each holds in +// document order, and the kustomize root's resources: entries. +type modelTree struct { + values map[string]string + files map[string][]string + kust []string +} + +func (t *modelTree) clone() modelTree { + files := make(map[string][]string, len(t.files)) + for p, objs := range t.files { + files[p] = slices.Clone(objs) + } + return modelTree{values: maps.Clone(t.values), files: files, kust: slices.Clone(t.kust)} +} + +// loc is the file holding obj's document, or "". +func (t *modelTree) loc(obj string) string { + for p, objs := range t.files { + if slices.Contains(objs, obj) { + return p + } + } + return "" +} + +// set is what a write of obj with value (a delete when "") does to the tree, as the worker places it: +// an existing document is edited where it lives; a document removed from the kustomize root leaves its +// resources: entry with it; a new object goes into the root and is registered there. It returns the +// paths it changes. +func (t *modelTree) set(obj, value string) []string { + at := t.loc(obj) + switch { + case value == "" && at == "": + return nil + case value == "": + delete(t.values, obj) + t.files[at] = slices.DeleteFunc(t.files[at], func(o string) bool { return o == obj }) + if len(t.files[at]) > 0 { + return []string{at} + } + delete(t.files, at) + if entry := strings.TrimPrefix(at, modelKustDir+"/"); entry != at { + t.kust = slices.DeleteFunc(t.kust, func(e string) bool { return e == entry }) + return []string{at, modelKustFile} + } + return []string{at} + case at != "": + if t.values[obj] == value { + return nil + } + t.values[obj] = value + return []string{at} + default: + at = modelKustDir + "/" + obj + ".yaml" + t.values[obj] = value + t.files[at] = []string{obj} + t.kust = append(t.kust, obj+".yaml") + return []string{at, modelKustFile} + } +} + +// setOutside is a write from another clone: it edits a document where it lives, or adds the object at +// its built-in path, unregistered. +func (t *modelTree) setOutside(obj, value string) string { + at := t.loc(obj) + if at == "" { + at = echoFile(obj) + t.files[at] = []string{obj} + } + t.values[obj] = value + return at +} + +// render is the bytes of the file at path. +func (t *modelTree) render(path string) string { + if path == modelKustFile { + var b strings.Builder + b.WriteString("apiVersion: kustomize.config.k8s.io/v1beta1\nkind: Kustomization\nresources:\n") + for _, entry := range t.kust { + b.WriteString("- " + entry + "\n") + } + return b.String() + } + var docs []string + for _, obj := range t.files[path] { + docs = append(docs, echoDocument(obj, t.values[obj])) + } + return strings.Join(docs, "---\n") +} + +// file is what the tree holds at path. +func (t *modelTree) file(path string) modelFile { + if path == modelKustFile { + return modelFile{kustomization: true, entries: slices.Clone(t.kust)} + } + var f modelFile + for _, obj := range t.files[path] { + f.docs = append(f.docs, [2]string{obj, t.values[obj]}) + } + return f +} + +// modelFootprint is a window's footprint by the model's rules: nil is a window never proven. +type modelFootprint struct { + paths []string + unknown bool +} + +func (f *modelFootprint) String() string { + switch { + case f == nil: + return "unproven" + case f.unknown: + return "unknown" + default: + return "[" + strings.Join(f.paths, " ") + "]" + } +} + +func (f *modelFootprint) overlaps(other modelFootprint) bool { + if f.unknown || other.unknown { + return true + } + for _, p := range f.paths { + if slices.Contains(other.paths, p) { + return true + } + } + return false +} + +func (f *modelFootprint) union(other modelFootprint) modelFootprint { + if f.unknown || other.unknown { + return modelFootprint{unknown: true} + } + paths := slices.Concat(f.paths, other.paths) + slices.Sort(paths) + return modelFootprint{paths: slices.Compact(paths)} +} + // modelEdit is one accepted write: seq is the order the worker accepted it in, and value is the // document's key, empty for a delete. type modelEdit struct { @@ -79,6 +240,7 @@ type modelSave struct { } type modelWindow struct { + seq int author string objs []string latest map[string]modelEdit @@ -88,6 +250,7 @@ type modelWindow struct { maxAt time.Time lastWriteAt time.Time charge int64 + footprint *modelFootprint } func (w *modelWindow) closeAt() (time.Time, windowFinalizeReason) { @@ -97,35 +260,50 @@ func (w *modelWindow) closeAt() (time.Time, windowFinalizeReason) { return w.maxAt, windowFinalizeReasonMaxDuration } -// modelPending is a decided write the next push publishes. +// modelPending is a decided write the next push publishes. footprint is the proven footprint of the +// window it closed, when that window had company. type modelPending struct { - save *modelSave - author string - message string - resync bool - changes []modelEdit + save *modelSave + author string + message string + resync bool + changes []modelEdit + footprint *modelFootprint +} + +// modelFile is what a commit leaves at one path: the kustomization's entries, the documents, or +// nothing. +type modelFile struct { + kustomization bool + entries []string + docs [][2]string } // modelCommit is a commit the worker must publish. A resync's message is not the model's to say. -// bootstrap is the folder's first commit, which also adds modelBootstrap. +// bootstrap is the folder's first commit, which also adds modelBootstrap. footprint, when set, holds +// every path the commit changes. type modelCommit struct { author string message string resync bool bootstrap bool - files map[string]string + paths []string + files map[string]modelFile seqs map[string]int + footprint *modelFootprint } type windowModel struct { now time.Time - window *modelWindow + windows []*modelWindow + seq int saves map[string]*modelSave saveSeq int editSeq int // remote is Git, known what the worker's checkout holds of it, cluster the live objects. - remote, known, cluster map[string]string + remote, known modelTree + cluster map[string]string // history is the values each object held on the cluster, for undos. history map[string][]string @@ -142,47 +320,139 @@ type windowModel struct { landed map[string]int } -func newWindowModel(now time.Time, docs map[string]string) *windowModel { +func newWindowModel(now time.Time, tree modelTree) *windowModel { m := &windowModel{ now: now, saves: map[string]*modelSave{}, - remote: map[string]string{}, known: map[string]string{}, cluster: map[string]string{}, + remote: tree.clone(), known: tree.clone(), cluster: maps.Clone(tree.values), history: map[string][]string{}, outcomes: map[string]FinalizeOutcome{}, given: map[string]bool{}, landed: map[string]int{}, } - for obj, value := range docs { - m.remote[obj], m.known[obj], m.cluster[obj] = value, value, value + for obj, value := range tree.values { m.history[obj] = []string{value} } return m } func (m *windowModel) windowCharge() int64 { - if m.window == nil { - return 0 + var charge int64 + for _, w := range m.windows { + charge += w.charge } - return m.window.charge + return charge } -// describeWindow is the open window: its author, how many objects it holds, the save it carries, when -// and why it closes, and what it is charged. -func (m *windowModel) describeWindow() string { - w := m.window - if w == nil { +// describeWindows is the open windows, oldest first: each one's number, author, how many objects it +// holds, the save it carries, when and why it closes, what it is charged, and its footprint. +func (m *windowModel) describeWindows() string { + var out []string + for _, w := range m.windows { + saved := "" + if w.save != nil { + saved = w.save.name + } + at, reason := w.closeAt() + out = append(out, fmt.Sprintf("#%d %s objs=%d save=%q closes in %s (%s) charge %d footprint %s", w.seq, + w.author, len(w.objs), saved, at.Sub(m.now), reason, w.charge, w.footprint)) + } + if len(out) == 0 { return "none" } - saved := "" - if w.save != nil { - saved = w.save.name + return strings.Join(out, "; ") +} + +func (m *windowModel) windowOf(author string) *modelWindow { + for _, w := range m.windows { + if w.author == author { + return w + } } - at, reason := w.closeAt() - return fmt.Sprintf("%s objs=%d save=%q closes in %s (%s) charge %d", w.author, len(w.objs), saved, - at.Sub(m.now), reason, w.charge) + return nil +} + +func (m *windowModel) isOpen(w *modelWindow) bool { return slices.Contains(m.windows, w) } + +func (m *windowModel) others(own *modelWindow) []*modelWindow { + var out []*modelWindow + for _, w := range m.windows { + if w != own { + out = append(out, w) + } + } + return out } func (m *windowModel) waiting(s *modelSave) bool { return !s.attached && s.deadline.After(m.now) } -func (m *windowModel) matches(s *modelSave) bool { - return m.window != nil && m.window.author == s.author +// prove is the footprint of a write of obj on the tree the worker's checkout holds. Before the folder +// is bootstrapped every write adds its bootstrap files. +func (m *windowModel) prove(obj string) modelFootprint { + at := m.known.loc(obj) + switch { + case !m.bootstrapped, at == "", strings.HasPrefix(at, modelKustDir+"/"), len(m.known.files[at]) > 1: + return modelFootprint{unknown: true} + default: + return modelFootprint{paths: []string{at}} + } +} + +func (m *windowModel) proveWindow(w *modelWindow) modelFootprint { + f := modelFootprint{} + for _, obj := range w.objs { + f = f.union(m.prove(obj)) + } + return f +} + +// admit closes the windows a write of obj by author cannot sit beside, and returns the footprint its +// window takes. +func (m *windowModel) admit(author, obj string) *modelFootprint { + own := m.windowOf(author) + if len(m.others(own)) == 0 { + if own == nil || own.footprint == nil { + return nil + } + f := m.proveWindow(own) + f = f.union(m.prove(obj)) + return &f + } + write := m.prove(obj) + m.reprove() + own = m.windowOf(author) + for _, other := range m.others(own) { + switch { + case write.unknown || other.footprint.unknown: + m.close(other, windowFinalizeReasonIdentityChange) + case write.overlaps(*other.footprint): + m.close(other, windowFinalizeReasonPathConflict) + } + } + if own == nil { + return &write + } + f := own.footprint.union(write) + return &f +} + +// reprove proves every open window again, and closes, oldest first, the windows whose footprints +// overlap. +func (m *windowModel) reprove() { + for _, w := range m.windows { + f := m.proveWindow(w) + w.footprint = &f + } + involved := map[*modelWindow]bool{} + for i, a := range m.windows { + for _, b := range m.windows[i+1:] { + if a.footprint.overlaps(*b.footprint) { + involved[a], involved[b] = true, true + } + } + } + for _, w := range slices.Clone(m.windows) { + if involved[w] { + m.close(w, windowFinalizeReasonPathConflict) + } + } } // write is appendLiveEvent's rules for one accepted write. @@ -193,17 +463,18 @@ func (m *windowModel) write(author string, edit modelEdit, pressure bool) { if edit.value != "" { m.history[edit.obj] = append(m.history[edit.obj], edit.value) } - if m.window != nil && m.window.author != author { - m.close(windowFinalizeReasonIdentityChange) - } - opened := m.window == nil + footprint := m.admit(author, edit.obj) + w := m.windowOf(author) + opened := w == nil if opened { - m.window = &modelWindow{ - author: author, latest: map[string]modelEdit{}, + m.seq++ + w = &modelWindow{ + seq: m.seq, author: author, latest: map[string]modelEdit{}, idle: modelIdle, maxAt: m.now.Add(modelMax), } + m.windows = append(m.windows, w) } - w := m.window + w.footprint = footprint if old, ok := w.latest[edit.obj]; ok { w.charge -= old.charge } else { @@ -215,12 +486,12 @@ func (m *windowModel) write(author string, edit modelEdit, pressure bool) { if opened { m.noteForeign() m.attachWaiting() - if m.window != w { + if !m.isOpen(w) { return } } if pressure { - m.close(windowFinalizeReasonBufferLimit) + m.closeAll(windowFinalizeReasonBufferLimit) return } m.closeOrArm() @@ -232,7 +503,10 @@ func (m *windowModel) register(s *modelSave, attachTimeout time.Duration) { if earlier.author != s.author { continue } - collecting := earlier.attached && m.window != nil && m.window.save == earlier + collecting := false + for _, w := range m.windows { + collecting = collecting || (earlier.attached && w.save == earlier) + } if m.waiting(earlier) || (collecting && !s.next) { m.outcomes[s.name] = FinalizeSaveInProgress return @@ -242,24 +516,24 @@ func (m *windowModel) register(s *modelSave, attachTimeout time.Duration) { s.seq = m.saveSeq s.deadline = m.now.Add(attachTimeout) m.saves[s.name] = s - if m.window == nil { + if len(m.windows) == 0 { return } - if !m.matches(s) { + own := m.windowOf(s.author) + if own == nil { s.sawForeign = true return } if s.next { - m.close(windowFinalizeReasonAttachNext) + m.close(own, windowFinalizeReasonAttachNext) return } - if m.window.save == nil { - m.attach(s) + if own.save == nil { + m.attach(own, s) } } -func (m *windowModel) attach(s *modelSave) { - w := m.window +func (m *windowModel) attach(w *modelWindow, s *modelSave) { w.save, s.attached = s, true w.charge += attachCharge(s.message) w.noIdle = s.idle == nil @@ -272,34 +546,41 @@ func (m *windowModel) attach(s *modelSave) { } func (m *windowModel) closeOrArm() { - if m.window == nil { - return + for _, w := range slices.Clone(m.windows) { + if at, reason := w.closeAt(); !at.After(m.now) { + m.close(w, reason) + } } - if at, reason := m.window.closeAt(); !at.After(m.now) { - m.close(reason) +} + +func (m *windowModel) closeAll(reason windowFinalizeReason) { + for _, w := range slices.Clone(m.windows) { + m.close(w, reason) } } func (m *windowModel) noteForeign() { for _, s := range m.saves { - if m.waiting(s) && m.window != nil && !m.matches(s) { + if m.waiting(s) && len(m.windows) > 0 && m.windowOf(s.author) == nil { s.sawForeign = true } } } func (m *windowModel) attachWaiting() { - if m.window == nil || m.window.save != nil { - return - } - var first *modelSave - for _, s := range m.saves { - if m.waiting(s) && m.matches(s) && (first == nil || s.seq < first.seq) { - first = s + for _, w := range slices.Clone(m.windows) { + if w.save != nil || !m.isOpen(w) { + continue + } + var first *modelSave + for _, s := range m.saves { + if m.waiting(s) && s.author == w.author && (first == nil || s.seq < first.seq) { + first = s + } + } + if first != nil { + m.attach(w, first) } - } - if first != nil { - m.attach(first) } } @@ -321,25 +602,31 @@ func (m *windowModel) resolve(s *modelSave, outcome FinalizeOutcome) { m.outcomes[s.name] = outcome } -// close decides the open window: the documents whose latest write differs from the checkout, in the -// order the window first saw them. A window that changes nothing and carries no save leaves no trace. -// A save is never trimmed: it decides every document it captured, so a replay onto a push the worker -// had not seen re-asserts them (API wins for a save). -func (m *windowModel) close(reason windowFinalizeReason) { - w := m.window - m.window = nil - m.closes = append(m.closes, string(reason)+" "+w.author) +// close decides a window: the documents whose latest write differs from the checkout, in the order the +// window first saw them. A window that changes nothing and carries no save leaves no trace. A save is +// never trimmed: it decides every document it captured, so a replay onto a push the worker had not +// seen re-asserts them (API wins for a save). A window once proven carries its footprint, proven again +// on the checkout it commits on. +func (m *windowModel) close(w *modelWindow, reason windowFinalizeReason) { + m.windows = slices.DeleteFunc(m.windows, func(o *modelWindow) bool { return o == w }) + m.closes = append(m.closes, fmt.Sprintf("%s %s #%d", reason, w.author, w.seq)) + var footprint *modelFootprint + if w.footprint != nil { + if f := m.proveWindow(w); !f.unknown { + footprint = &f + } + } var changes []modelEdit for _, obj := range w.objs { - if edit := w.latest[obj]; edit.value != m.known[obj] || w.save != nil { + if edit := w.latest[obj]; edit.value != m.known.values[obj] || w.save != nil { changes = append(changes, edit) - m.known[obj] = edit.value + m.known.set(obj, edit.value) } } if len(changes) == 0 && w.save == nil { return } - p := modelPending{save: w.save, author: w.author, changes: changes} + p := modelPending{save: w.save, author: w.author, changes: changes, footprint: footprint} if w.save != nil { p.message = w.save.message } else { @@ -363,27 +650,37 @@ func generatedMessage(changes []modelEdit) string { // push publishes the pending writes. A remote that moved behind the worker's back is found by the // push, which replays the decided changes onto it, and every applier is due for the foreign commit. +// The footprint of a replayed write was proven on another tree, so it holds nothing about the commit. func (m *windowModel) push() { if len(m.pending) == 0 { return } for _, p := range m.pending { commit := modelCommit{author: p.author, message: p.message, resync: p.resync, - files: map[string]string{}, seqs: map[string]int{}} + files: map[string]modelFile{}, seqs: map[string]int{}} + if !m.foreignUnseen { + commit.footprint = p.footprint + } + var paths []string for _, edit := range p.changes { - if edit.value == m.remote[edit.obj] { + changed := m.remote.set(edit.obj, edit.value) + if len(changed) == 0 { continue } - m.remote[edit.obj] = edit.value - commit.files[edit.obj] = edit.value + paths = append(paths, changed...) commit.seqs[edit.obj] = edit.seq } - if len(commit.files) == 0 { + if len(paths) == 0 { if p.save != nil { m.resolve(p.save, FinalizeAlreadyPresent) } continue } + slices.Sort(paths) + commit.paths = slices.Compact(paths) + for _, path := range commit.paths { + commit.files[path] = m.remote.file(path) + } commit.bootstrap = !m.bootstrapped m.bootstrapped = true m.commits = append(m.commits, commit) @@ -395,7 +692,7 @@ func (m *windowModel) push() { if m.foreignUnseen { m.foreignUnseen, m.applyDue = false, true } - m.known = maps.Clone(m.remote) + m.known = m.remote.clone() } // endWake is the loop's end of every wake. It reports whether an apply was sent: the driver's applier @@ -408,40 +705,38 @@ func (m *windowModel) endWake() bool { } if m.hinted { m.hinted = false - if m.window != nil && m.window.save == nil { - m.close(windowFinalizeReasonForeignCommit) + for _, w := range slices.Clone(m.windows) { + if w.save == nil { + m.close(w, windowFinalizeReasonForeignCommit) + } } } m.push() if !m.applyDue || len(m.saves) > 0 { return false } - if m.window != nil { - m.close(windowFinalizeReasonApplyDue) - } + m.closeAll(windowFinalizeReasonApplyDue) m.push() m.applyDue = false // The apply is of the tip the worker knows: a push it has not found yet is not in it. - m.cluster = maps.Clone(m.known) + m.cluster = maps.Clone(m.known.values) return true } -// resync closes the window, then commits, as the operator, every live object Git does not hold as -// it is. prune.mode is onEvent, so nothing is swept. An object it adds is adopted: an apply is due. +// resync closes the windows, then commits, as the operator, every live object Git does not hold as it +// is. prune.mode is onEvent, so nothing is swept. An object it adds is adopted: an apply is due. func (m *windowModel) resync() { - if m.window != nil { - m.close(windowFinalizeReasonResyncBeforeApply) - } + m.closeAll(windowFinalizeReasonResyncBeforeApply) var changes []modelEdit for _, obj := range modelObjects { value := m.cluster[obj] - if value == "" || value == m.known[obj] { + if value == "" || value == m.known.values[obj] { continue } - if m.known[obj] == "" { + if m.known.values[obj] == "" { m.applyDue = true } - m.known[obj] = value + m.known.set(obj, value) changes = append(changes, modelEdit{obj: obj, value: value}) } if len(changes) > 0 { @@ -449,11 +744,9 @@ func (m *windowModel) resync() { } } -// shutdown closes the window and publishes; a save still waiting for a window is given back. +// shutdown closes the windows and publishes; a save still waiting for a window is given back. func (m *windowModel) shutdown() { - if m.window != nil { - m.close(windowFinalizeReasonShutdown) - } + m.closeAll(windowFinalizeReasonShutdown) m.push() for name := range m.saves { m.given[name] = true @@ -479,12 +772,26 @@ type windowModelRun struct { exited bool values int saves int + // concurrent is how many steps ended with more than one window open. + concurrent int } func newWindowModelRun(t *testing.T, seed int64) *windowModelRun { t.Helper() - docs := map[string]string{"x": "x0", "y": "y0", "z": "z0"} - f := newEchoFixture(t, "window-model-"+strconv.FormatInt(seed, 10), configv1alpha3.PruneOnEvent, docs) + tree := modelTree{ + values: map[string]string{"x": "x0", "w": "w0", "z": "z0", "k": "k0", "s1": "s10", "s2": "s20"}, + files: map[string][]string{ + echoFile("x"): {"x"}, echoFile("w"): {"w"}, echoFile("z"): {"z"}, + modelKustDir + "/k.yaml": {"k"}, modelSharedFile: {"s1", "s2"}, + }, + kust: []string{"k.yaml"}, + } + layout := map[string]string{} + for _, path := range []string{modelKustFile, modelKustDir + "/k.yaml", modelSharedFile} { + layout[path] = tree.render(path) + } + f := newEchoFixtureWith(t, "window-model-"+strconv.FormatInt(seed, 10), configv1alpha3.PruneOnEvent, + map[string]string{"x": "x0", "w": "w0", "z": "z0"}, layout) requester := f.withClaim() r := &windowModelRun{t: t, seed: seed, f: f, requester: requester, rng: rand.New(rand.NewSource(seed))} r.clock = useTestClock(f.worker) @@ -493,13 +800,34 @@ func newWindowModelRun(t *testing.T, seed int64) *windowModelRun { r.base = gitIn(t, f.repoDir, "rev-parse", "main") r.knownTip = r.base r.start = r.clock.Now() - r.m = newWindowModel(r.start, docs) + r.m = newWindowModel(r.start, tree) r.m.bootstrapped = gitIn(t, f.repoDir, "ls-tree", "--name-only", r.base, echoTarget+"/README.md") != "" return r } +// pushFromOutside commits the files at paths, as tree holds them, from the outside clone. +func (r *windowModelRun) pushFromOutside(tree modelTree, paths []string, message string) { + r.t.Helper() + clone := r.f.clone + gitIn(r.t, clone, "pull", "--quiet", "--ff-only", "origin", "main") + for _, path := range paths { + full := filepath.Join(clone, path) + require.NoError(r.t, os.MkdirAll(filepath.Dir(full), 0o750)) + require.NoError(r.t, os.WriteFile(full, []byte(tree.render(path)), 0o600)) + gitIn(r.t, clone, "add", path) + } + gitIn(r.t, clone, "commit", "--quiet", "-m", message) + gitIn(r.t, clone, "push", "--quiet", "origin", "HEAD:main") +} + func (r *windowModelRun) failf(format string, args ...any) { r.t.Helper() + if os.Getenv("WINDOW_MODEL_LOGS") != "" { + r.t.Log("the worker's log:\n" + strings.Join(r.logs.matching(""), "\n")) + for _, p := range splitLines(gitIn(r.t, r.f.repoDir, "ls-tree", "-r", "--name-only", "main")) { + r.t.Log("DEBUGFILE " + p + ":\n" + gitIn(r.t, r.f.repoDir, "show", "main:"+p)) + } + } tail := r.trace if len(tail) > 40 { tail = tail[len(tail)-40:] @@ -612,7 +940,7 @@ func (r *windowModelRun) advance(d time.Duration) { func (r *windowModelRun) freshValue(obj string) string { r.values++ - return obj + strconv.Itoa(r.values) + return obj + "v" + strconv.Itoa(r.values) } func (r *windowModelRun) event(author, obj, value string) Event { @@ -628,10 +956,15 @@ func (r *windowModelRun) event(author, obj, value string) Event { } // write is one accepted live write: a new value, an undo to an earlier one, the value the object -// already holds, or a delete. +// already holds, or a delete. Each author mostly edits an object of its own, so their windows can stay +// open side by side, and sometimes any other. func (r *windowModelRun) write(pressure bool) { - author := modelAuthors[r.rng.Intn(len(modelAuthors))] + who := r.rng.Intn(len(modelAuthors)) + author := modelAuthors[who] obj := modelObjects[r.rng.Intn(len(modelObjects))] + if r.rng.Intn(3) > 0 { + obj = modelObjects[who] + } current := r.m.cluster[obj] value, kind := r.freshValue(obj), "write" switch roll := r.rng.Intn(20); { @@ -639,7 +972,7 @@ func (r *windowModelRun) write(pressure bool) { value, kind = r.m.history[obj][r.rng.Intn(len(r.m.history[obj])-1)], "undo" case roll < 7 && current != "": value, kind = current, "no-op" - case roll < 10 && current != "": + case roll < 9 && current != "": value, kind = "", "delete" } event := r.event(author, obj, value) @@ -691,8 +1024,8 @@ func (r *windowModelRun) foreign() { r.values++ value := "outside" + strconv.Itoa(r.values) r.notef("outside push %s=%q, hinted", obj, value) - r.f.pushFromOutside(obj, value) - r.m.remote[obj] = value + path := r.m.remote.setOutside(obj, value) + r.pushFromOutside(r.m.remote, []string{path}, "outside: "+obj+"="+value) r.m.foreignUnseen = true r.f.loop.pushHinted = true r.wake(nil, func() { r.m.hinted = true }) @@ -756,6 +1089,9 @@ func (r *windowModelRun) run(steps int) { for range steps { r.step() r.check("step") + if len(r.m.windows) > 1 { + r.concurrent++ + } } if r.rng.Intn(2) == 0 { r.shutdown() @@ -766,28 +1102,32 @@ func (r *windowModelRun) run(steps int) { r.drained() } -var closeLine = regexp.MustCompile(`"msg"="Finalizing open commit window" "reason"="([^"]+)" "windowAuthor"="([^"]*)"`) +var closeLine = regexp.MustCompile(`"msg"="Finalizing open commit window" "reason"="([^"]+)" ` + + `"windowAuthor"="([^"]*)" "windowTarget"="[^"]*" "window"=(\d+)`) // check compares the worker with the model after a step. func (r *windowModelRun) check(what string) { r.t.Helper() - r.checkWindow(what) + r.checkWindows(what) r.checkTimers(what) r.checkSaves(what) r.checkCloses(what) r.checkCommits(what) + if lines := r.logs.matching(decisionSingleWindowFallback); len(lines) > 0 { + r.failf("%s: the worker refused a commit outside its footprint: %s", what, lines[0]) + } } -// checkWindow compares the open window and what is retained. -func (r *windowModelRun) checkWindow(what string) { +// checkWindows compares the open windows and what is retained. +func (r *windowModelRun) checkWindows(what string) { r.t.Helper() loop, m := r.f.loop, r.m - if got, want := r.workerWindow(), m.describeWindow(); got != want { - r.failf("%s: the open window differs:\n worker %s\n model %s", what, got, want) + if got, want := r.workerWindows(), m.describeWindows(); got != want { + r.failf("%s: the open windows differ:\n worker %s\n model %s", what, got, want) } - if got, want := loop.totalRetainedBytes(), m.windowCharge(); got != want || loop.commitWindows.windowBytes != want { - r.failf("%s: retained %d (window %d), the model's window charge is %d", what, got, - loop.commitWindows.windowBytes, want) + if got, want := loop.totalRetainedBytes(), m.windowCharge(); got != want || loop.windows.bytes() != want { + r.failf("%s: retained %d (windows %d), the model's window charge is %d", what, got, + loop.windows.bytes(), want) } if len(loop.pendingWrites) != 0 { r.failf("%s: %d writes still pending after a wake that pushed at once", what, len(loop.pendingWrites)) @@ -795,7 +1135,8 @@ func (r *windowModelRun) checkWindow(what string) { } // checkTimers compares the timers the worker has armed with the model's deadlines: the commit timer -// for the open window's close, the attach timer for the earliest deadline of a save still waiting. +// for the earliest close of an open window, the attach timer for the earliest deadline of a save still +// waiting. func (r *windowModelRun) checkTimers(what string) { r.t.Helper() if r.exited { @@ -803,8 +1144,10 @@ func (r *windowModelRun) checkTimers(what string) { } loop, m := r.f.loop, r.m var commitAt, attachAt time.Time - if m.window != nil { - commitAt, _ = m.window.closeAt() + for _, w := range m.windows { + if at, _ := w.closeAt(); commitAt.IsZero() || at.Before(commitAt) { + commitAt = at + } } for _, s := range m.saves { if !s.attached && (attachAt.IsZero() || s.deadline.Before(attachAt)) { @@ -826,19 +1169,27 @@ func (r *windowModelRun) describeTimer(armed bool, at time.Time) string { return "in " + at.Sub(r.m.now).String() } -// workerWindow describes the worker's open window as describeWindow does the model's. -func (r *windowModelRun) workerWindow() string { - w := r.f.loop.openWindow - if w == nil { - return "none" +// workerWindows describes the worker's open windows as describeWindows does the model's. An unknown +// footprint's reason is the worker's to word. +func (r *windowModelRun) workerWindows() string { + var out []string + for _, w := range r.f.loop.windows.all() { + saved := "" + if w.pendingCR != nil { + saved = w.pendingCR.Name + } + footprint := footprintString(w.footprint) + if w.footprint != nil && !w.footprint.known() { + footprint = "unknown" + } + at, reason := w.closeAt() + out = append(out, fmt.Sprintf("#%d %s objs=%d save=%q closes in %s (%s) charge %d footprint %s", w.seq, + w.Author, len(w.pathOrder), saved, at.Sub(r.m.now), reason, w.bytes, footprint)) } - saved := "" - if w.pendingCR != nil { - saved = w.pendingCR.Name + if len(out) == 0 { + return "none" } - at, reason := w.closeAt() - return fmt.Sprintf("%s objs=%d save=%q closes in %s (%s) charge %d", w.Author, len(w.pathOrder), saved, - at.Sub(r.m.now), reason, w.bytes) + return strings.Join(out, "; ") } // checkSaves compares the registered saves, and every save's outcome and how often it settled. @@ -874,7 +1225,7 @@ func (r *windowModelRun) checkSaves(what string) { } } -// checkCloses compares the windows closed so far, by reason and author, in order. +// checkCloses compares the windows closed so far, by reason, author and number, in order. func (r *windowModelRun) checkCloses(what string) { r.t.Helper() var closes []string @@ -883,7 +1234,7 @@ func (r *windowModelRun) checkCloses(what string) { if match == nil { r.failf("unparsed close line %s", line) } - closes = append(closes, match[1]+" "+match[2]) + closes = append(closes, match[1]+" "+match[2]+" #"+match[3]) } if !slices.Equal(closes, r.m.closes) { r.failf("%s: close reasons differ:\n worker %v\n model %v", what, closes, r.m.closes) @@ -931,10 +1282,7 @@ func (r *windowModelRun) checkCommit(what, hash string, want modelCommit) { want.message) } paths := splitLines(gitIn(r.t, r.f.repoDir, "diff-tree", "-r", "--name-only", "--no-commit-id", hash)) - var wantPaths []string - for obj := range want.files { - wantPaths = append(wantPaths, echoFile(obj)) - } + wantPaths := slices.Clone(want.paths) if want.bootstrap { for _, name := range modelBootstrap { wantPaths = append(wantPaths, echoTarget+"/"+name) @@ -946,8 +1294,16 @@ func (r *windowModelRun) checkCommit(what, hash string, want modelCommit) { if !slices.Equal(paths, wantPaths) { r.failf("%s: commit %s by %s changes %v; the model's changes %v", what, hash[:8], author, paths, wantPaths) } - for obj, value := range want.files { - r.checkDocument(what, hash, obj, value) + if f := want.footprint; f != nil { + for _, path := range paths { + if !slices.Contains(f.paths, path) { + r.failf("%s: commit %s by %s changes %s, outside its window's footprint %s", what, hash[:8], + author, path, f) + } + } + } + for path, file := range want.files { + r.checkFile(what, hash, path, file) } for obj, seq := range want.seqs { if seq <= r.m.landed[obj] { @@ -957,26 +1313,65 @@ func (r *windowModelRun) checkCommit(what, hash string, want modelCommit) { } } -// checkDocument compares the document obj at rev, parsed, with the ConfigMap obj holding value; value -// "" is no document. YAML 1.2, so a bare y is the name y. -func (r *windowModelRun) checkDocument(what, rev, obj, value string) { +// checkFile compares the file at path in rev, parsed, with what the model says it holds: the +// kustomization's resources: entries, or each document whole, in order. YAML 1.2, so a bare y is the +// name y. +func (r *windowModelRun) checkFile(what, rev, path string, want modelFile) { r.t.Helper() - var got, want map[string]any - listed := gitIn(r.t, r.f.repoDir, "ls-tree", "--name-only", rev, echoFile(obj)) - if listed != "" { - if err := yaml.Unmarshal([]byte(gitIn(r.t, r.f.repoDir, "show", rev+":"+echoFile(obj))), &got); err != nil { - r.failf("%s: %s at %s: %v", what, obj, rev, err) + var content []byte + if gitIn(r.t, r.f.repoDir, "ls-tree", "--name-only", rev, path) != "" { + content = []byte(gitIn(r.t, r.f.repoDir, "show", rev+":"+path)) + } + docs, err := parseDocuments(content) + if err != nil { + r.failf("%s: %s at %s: %v", what, path, rev, err) + } + if want.kustomization { + var entries []string + if len(docs) == 1 { + for _, entry := range asSlice(docs[0]["resources"]) { + entries = append(entries, fmt.Sprint(entry)) + } + } + if len(docs) != 1 || !slices.Equal(entries, want.entries) { + r.failf("%s: %s at %s lists %v; the model's %v", what, path, rev, entries, want.entries) } + return } - if value != "" { - want = map[string]any{ + var wantDocs []map[string]any + for _, doc := range want.docs { + wantDocs = append(wantDocs, map[string]any{ "apiVersion": "v1", "kind": "ConfigMap", - "metadata": map[string]any{"name": obj, "namespace": "default"}, - "data": map[string]any{"key": value}, - } + "metadata": map[string]any{"name": doc[0], "namespace": "default"}, + "data": map[string]any{"key": doc[1]}, + }) } - if !reflect.DeepEqual(got, want) { - r.failf("%s: %s at %s is %v; the model's is %v", what, obj, rev, got, want) + if !reflect.DeepEqual(docs, wantDocs) { + r.failf("%s: %s at %s is %v; the model's is %v", what, path, rev, docs, wantDocs) + } +} + +func asSlice(v any) []any { + s, _ := v.([]any) + return s +} + +// parseDocuments parses every document of a YAML stream with yaml.v3. +func parseDocuments(content []byte) ([]map[string]any, error) { + var docs []map[string]any + decoder := yaml.NewDecoder(bytes.NewReader(content)) + for { + var doc map[string]any + err := decoder.Decode(&doc) + if errors.Is(err, io.EOF) { + return docs, nil + } + if err != nil { + return nil, err + } + if doc != nil { + docs = append(docs, doc) + } } } @@ -996,7 +1391,7 @@ func (r *windowModelRun) checkBootstrap(what, rev, name string) { func (r *windowModelRun) drained() { r.t.Helper() r.check("drained") - if r.m.window != nil || len(r.m.saves) != 0 || len(r.m.pending) != 0 { + if len(r.m.windows) != 0 || len(r.m.saves) != 0 || len(r.m.pending) != 0 { r.failf("the model is not drained") } // An exited loop is dropped with the saves it gave back still counted in heldBytes; what it retains @@ -1019,37 +1414,33 @@ func (r *windowModelRun) drained() { } } -// checkTree checks that Git is the base tree with the model's documents and, once the folder is -// bootstrapped, the template's files: nothing else was added, changed or removed. +// checkTree checks that Git is the base tree outside the folder, and in it the model's files and, +// once the folder is bootstrapped, the template's: nothing else was added, changed or removed. func (r *windowModelRun) checkTree() { r.t.Helper() - present := map[string]bool{} + var want []string for _, path := range splitLines(gitIn(r.t, r.f.repoDir, "ls-tree", "-r", "--name-only", r.base)) { - present[path] = true - } - for _, obj := range modelObjects { - present[echoFile(obj)] = r.m.remote[obj] != "" - r.checkDocument("drained", "main", obj, r.m.remote[obj]) - } - others := []string{"diff", "--name-only", r.base, "main", "--", ".", - ":(exclude)" + echoTarget + "/default/configmaps"} - for _, name := range modelBootstrap { - others = append(others, ":(exclude)"+echoTarget+"/"+name) - if r.m.bootstrapped { - present[echoTarget+"/"+name] = true - r.checkBootstrap("drained", "main", name) + if !strings.HasPrefix(path, echoTarget+"/") { + want = append(want, path) } } - var want []string - for path, ok := range present { - if ok { - want = append(want, path) + want = append(want, modelKustFile) + r.checkFile("drained", "main", modelKustFile, r.m.remote.file(modelKustFile)) + for path := range r.m.remote.files { + want = append(want, path) + r.checkFile("drained", "main", path, r.m.remote.file(path)) + } + if r.m.bootstrapped { + for _, name := range modelBootstrap { + want = append(want, echoTarget+"/"+name) + r.checkBootstrap("drained", "main", name) } } slices.Sort(want) if tree := splitLines(gitIn(r.t, r.f.repoDir, "ls-tree", "-r", "--name-only", "main")); !slices.Equal(tree, want) { r.failf("drained, Git holds %v; the model %v", tree, want) } + others := []string{"diff", "--name-only", r.base, "main", "--", ".", ":(exclude)" + echoTarget} if changed := gitIn(r.t, r.f.repoDir, others...); changed != "" { r.failf("drained, Git changed the base's %s", changed) } @@ -1063,7 +1454,8 @@ func envInt(name string, fallback int) int { } // TestWindowModel_TheWorkerFollowsTheReferenceModel runs random input sequences through a worker and -// the model, and checks every step. +// the model, and checks every step. Across the seeds, some steps must end with several windows open, +// or the search says nothing about them. func TestWindowModel_TheWorkerFollowsTheReferenceModel(t *testing.T) { seeds, steps := envInt("WINDOW_MODEL_SEEDS", 6), envInt("WINDOW_MODEL_STEPS", 40) if testing.Short() { @@ -1073,9 +1465,16 @@ func TestWindowModel_TheWorkerFollowsTheReferenceModel(t *testing.T) { if seed := envInt("WINDOW_MODEL_SEED", 0); seed > 0 { first, seeds = int64(seed), 1 } + concurrent := 0 for seed := first; seed < first+int64(seeds); seed++ { t.Run("seed-"+strconv.FormatInt(seed, 10), func(t *testing.T) { - newWindowModelRun(t, seed).run(steps) + run := newWindowModelRun(t, seed) + run.run(steps) + concurrent += run.concurrent }) } + t.Logf("%d steps ended with several windows open", concurrent) + if seeds > 1 && !t.Failed() { + require.Positive(t, concurrent, "no step ended with several windows open") + } } diff --git a/internal/git/window_set.go b/internal/git/window_set.go index 71c8aa44..b935a8a3 100644 --- a/internal/git/window_set.go +++ b/internal/git/window_set.go @@ -2,60 +2,69 @@ package git -import "time" +import ( + "slices" + "time" +) // commitWindows is the set of commit windows a branch worker holds open, and what they are charged. // Every path that reads a window says which one it means, or all of them. // -// It allows one window. A write that cannot join the open window closes it first, with close reason -// identity_change, before it opens its own, so commits land in the order their windows opened. -// docs/design/concurrent-author-windows.md lets several stay open side by side; windows that one -// trigger closes then close oldest first, by the sequence number each took when it opened, never by -// map order or by time. -// -// The loop embeds the set, so loop.openWindow still names the one window it holds. +// Several windows may be open side by side, one per author on the GitTarget, while their footprints +// are proven not to overlap (footprint.go, docs/design/concurrent-author-windows.md). A write that +// cannot be admitted beside a window closes that window first (blocking). Windows that one trigger +// closes close oldest first, by the sequence number each took when it opened, never by map order or +// by time. type commitWindows struct { - // openWindow is the open window, or nil. - openWindow *openWindow + // windows are the open windows, in the order they opened. + windows []*openWindow // windowBytes is what the open windows are charged: their events, and the message of the request // attached to each. It is the sum of every open window's charge. windowBytes int64 - // windowSeq numbers the windows in the order they opened. + // windowSeq numbers the windows in the order they opened. It resets with the worker. windowSeq uint64 } -// all returns the open windows, oldest first. +// all returns the open windows, oldest first. The slice is the caller's: closing a window while +// ranging over it is safe. func (s *commitWindows) all() []*openWindow { - if s.openWindow == nil { - return nil - } - return []*openWindow{s.openWindow} + return slices.Clone(s.windows) } // anyOpen reports whether a window is open. func (s *commitWindows) anyOpen() bool { - return s.openWindow != nil + return len(s.windows) > 0 +} + +// count is how many windows are open. +func (s *commitWindows) count() int { + return len(s.windows) +} + +// bytes is what the open windows are charged. +func (s *commitWindows) bytes() int64 { + return s.windowBytes } // isOpen reports whether w is still one of the open windows. func (s *commitWindows) isOpen(w *openWindow) bool { - return w != nil && s.openWindow == w + return w != nil && slices.Contains(s.windows, w) } // open opens a window for the write e at now, numbered after every window opened before it. The -// caller has closed every window e cannot be admitted beside (blocking), so the set is empty here. +// caller has closed every window e cannot be admitted beside (blocking). func (s *commitWindows) open(e Event, writer eventContentWriter, now time.Time) *openWindow { s.windowSeq++ w := newOpenWindow(e, writer) w.seq = s.windowSeq w.openedAt = now - s.openWindow = w + s.windows = append(s.windows, w) return w } // forWrite returns the open window the write e joins, or nil when it must open one. func (s *commitWindows) forWrite(e Event) *openWindow { - for _, w := range s.all() { + for _, w := range s.windows { if w.canAppend(e) { return w } @@ -63,13 +72,12 @@ func (s *commitWindows) forWrite(e Event) *openWindow { return nil } -// blocking returns the open windows the write e cannot be admitted beside, oldest first: every -// window it cannot join. -func (s *commitWindows) blocking(e Event) []*openWindow { +// others returns the open windows other than w, oldest first. +func (s *commitWindows) others(w *openWindow) []*openWindow { var out []*openWindow - for _, w := range s.all() { - if !w.canAppend(e) { - out = append(out, w) + for _, other := range s.windows { + if other != w { + out = append(out, other) } } return out @@ -77,7 +85,7 @@ func (s *commitWindows) blocking(e Event) []*openWindow { // forRequest returns the open window the request may attach to, or nil. func (s *commitWindows) forRequest(p *pendingCommitRequest) *openWindow { - for _, w := range s.all() { + for _, w := range s.windows { if p.matchesWindow(w) { return w } @@ -85,9 +93,13 @@ func (s *commitWindows) forRequest(p *pendingCommitRequest) *openWindow { return nil } -// foreignTo returns the oldest open window the request may not attach to, or nil. +// foreignTo returns the oldest open window the request may not attach to, while none it may attach +// to is open, or nil. A window of the request's own author stands in no other author's way. func (s *commitWindows) foreignTo(p *pendingCommitRequest) *openWindow { - for _, w := range s.all() { + if s.forRequest(p) != nil { + return nil + } + for _, w := range s.windows { if !p.matchesWindow(w) { return w } @@ -97,7 +109,7 @@ func (s *commitWindows) foreignTo(p *pendingCommitRequest) *openWindow { // carrying returns the open window the request is attached to, or nil. func (s *commitWindows) carrying(id commitRequestID) *openWindow { - for _, w := range s.all() { + for _, w := range s.windows { if w.pendingCR != nil && *w.pendingCR == id { return w } @@ -113,9 +125,10 @@ func (s *commitWindows) charge(w *openWindow, n int64) { // remove forgets w, and releases what it was charged. func (s *commitWindows) remove(w *openWindow) { - if !s.isOpen(w) { + i := slices.Index(s.windows, w) + if i < 0 { return } - s.openWindow = nil + s.windows = slices.Delete(s.windows, i, i+1) s.windowBytes -= w.bytes } diff --git a/internal/git/window_set_test.go b/internal/git/window_set_test.go index 9c84a4a1..6c30c53d 100644 --- a/internal/git/window_set_test.go +++ b/internal/git/window_set_test.go @@ -3,6 +3,7 @@ package git import ( + "fmt" "testing" "time" @@ -10,6 +11,28 @@ import ( "github.com/stretchr/testify/require" ) +// only is the one open window, or nil, for a test of a worker that holds one window at a time. It +// panics when several are open: such a test asks which one it means. +func (s *commitWindows) only() *openWindow { + switch len(s.windows) { + case 0: + return nil + case 1: + return s.windows[0] + default: + panic(fmt.Sprintf("%d commit windows are open; the test reads one", len(s.windows))) + } +} + +// setOnly makes w the one open window, or leaves none open when w is nil, for a test that fakes one. +// What the windows are charged is left alone. +func (s *commitWindows) setOnly(w *openWindow) { + s.windows = nil + if w != nil { + s.windows = []*openWindow{w} + } +} + func TestCommitWindows_WindowsAreNumberedInTheOrderTheyOpen(t *testing.T) { var set commitWindows opened := time.Now() @@ -17,11 +40,15 @@ func TestCommitWindows_WindowsAreNumberedInTheOrderTheyOpen(t *testing.T) { first := set.open(configMapTargetEvent("a", "alice", "team-a"), nil, opened) assert.Equal(t, opened, first.openedAt) assert.Equal(t, []*openWindow{first}, set.all()) - set.remove(first) - assert.Empty(t, set.all()) - second := set.open(configMapTargetEvent("b", "bob", "team-a"), nil, opened) assert.Greater(t, second.seq, first.seq, "a window opened later is numbered after it") + assert.Equal(t, []*openWindow{first, second}, set.all(), "oldest first") + + set.remove(first) + assert.Equal(t, []*openWindow{second}, set.all()) + third := set.open(configMapTargetEvent("c", "carol", "team-a"), nil, opened) + assert.Equal(t, []*openWindow{second, third}, set.all(), "a close keeps the others in their order") + assert.Equal(t, 2, set.count()) } func TestCommitWindows_ASetSaysWhichWindowAWriteOrARequestBelongsTo(t *testing.T) { @@ -31,9 +58,9 @@ func TestCommitWindows_ASetSaysWhichWindowAWriteOrARequestBelongsTo(t *testing.T alice := configMapTargetEvent("b", "alice", "team-a") bob := configMapTargetEvent("b", "bob", "team-a") assert.Same(t, window, set.forWrite(alice)) - assert.Empty(t, set.blocking(alice)) + assert.Empty(t, set.others(set.forWrite(alice))) assert.Nil(t, set.forWrite(bob)) - assert.Equal(t, []*openWindow{window}, set.blocking(bob), "bob's write must close alice's window first") + assert.Equal(t, []*openWindow{window}, set.others(set.forWrite(bob))) aliceSave := &pendingCommitRequest{author: "alice", gitTargetName: "team-a", gitTargetNamespace: "default"} bobSave := &pendingCommitRequest{author: "bob", gitTargetName: "team-a", gitTargetNamespace: "default"} @@ -42,24 +69,36 @@ func TestCommitWindows_ASetSaysWhichWindowAWriteOrARequestBelongsTo(t *testing.T assert.Nil(t, set.forRequest(bobSave)) assert.Same(t, window, set.foreignTo(bobSave)) + bobs := set.open(bob, nil, time.Now()) + assert.Same(t, bobs, set.forRequest(bobSave)) + assert.Nil(t, set.foreignTo(bobSave), "a window of the request's own author is open: nobody stands in its way") + carolSave := &pendingCommitRequest{author: "carol", gitTargetName: "team-a", gitTargetNamespace: "default"} + assert.Same(t, window, set.foreignTo(carolSave), "the oldest window it cannot attach to") + id := commitRequestID{Namespace: "default", Name: "save", UID: "uid-save"} assert.Nil(t, set.carrying(id)) - window.pendingCR = &id - assert.Same(t, window, set.carrying(id)) + bobs.pendingCR = &id + assert.Same(t, bobs, set.carrying(id)) } func TestCommitWindows_ClosingAWindowReleasesExactlyItsCharge(t *testing.T) { var set commitWindows window := set.open(configMapTargetEvent("a", "alice", "team-a"), nil, time.Now()) + other := set.open(configMapTargetEvent("b", "bob", "team-a"), nil, time.Now()) set.charge(window, 100) set.charge(window, -30) + set.charge(other, 5) require.Equal(t, int64(70), window.bytes) - require.Equal(t, int64(70), set.windowBytes) + require.Equal(t, int64(75), set.bytes()) set.remove(window) - assert.False(t, set.anyOpen()) - assert.Zero(t, set.windowBytes) + assert.True(t, set.isOpen(other)) + assert.False(t, set.isOpen(window)) + assert.Equal(t, int64(5), set.bytes()) set.remove(window) - assert.Zero(t, set.windowBytes, "a window already closed has nothing left to release") + assert.Equal(t, int64(5), set.bytes(), "a window already closed has nothing left to release") + set.remove(other) + assert.False(t, set.anyOpen()) + assert.Zero(t, set.bytes()) } diff --git a/internal/git/write_boundary_precondition_test.go b/internal/git/write_boundary_precondition_test.go index ed4a2e38..88f73ef4 100644 --- a/internal/git/write_boundary_precondition_test.go +++ b/internal/git/write_boundary_precondition_test.go @@ -247,11 +247,11 @@ func TestEventLoop_LiveFanInRefusal_FailsGitTargetAndCommitsNothing(t *testing.T event.GitTargetName = "podinfo-test" event.GitTargetNamespace = "default" loop.handleQueueItem(WorkItem{Request: &WriteRequest{Events: []Event{event}}}) - require.NotNil(t, loop.openWindow, "the live event must open a commit window") + require.NotNil(t, loop.windows.only(), "the live event must open a commit window") assert.False(t, loop.finalizeOpenWindow(), "a refused write plan must not produce a pending write") assert.Empty(t, loop.pendingWrites, "a refused flush must retain nothing to push") - assert.Nil(t, loop.openWindow, "the refused window must be dropped, not retried forever") + assert.Nil(t, loop.windows.only(), "the refused window must be dropped, not retried forever") require.Len(t, refusals, 1, "a live write-boundary refusal must be reported exactly once") assert.Equal(t, types.NewResourceReference("podinfo-test", "default"), refusedTargets[0], diff --git a/internal/git/write_gate_test.go b/internal/git/write_gate_test.go index 49f01db2..23c58525 100644 --- a/internal/git/write_gate_test.go +++ b/internal/git/write_gate_test.go @@ -140,7 +140,7 @@ func TestWriteGate_ARequestOnARefusingTargetFails(t *testing.T) { closeGate func(), req *AttachCommitRequest) { serviceAttach(loop, req) writeTo(loop, "new") - require.NotNil(t, loop.openWindow, "the request is attached to an open window") + require.NotNil(t, loop.windows.only(), "the request is attached to an open window") closeGate() forceDueNamed(loop, req) loop.pushPending() @@ -149,7 +149,7 @@ func TestWriteGate_ARequestOnARefusingTargetFails(t *testing.T) { closeGate func(), req *AttachCommitRequest) { writeTo(loop, "existing") serviceAttach(loop, req) - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) closeGate() forceDueNamed(loop, req) loop.pushPending() @@ -185,8 +185,8 @@ func TestWriteGate_ARequestOnARefusingTargetFails(t *testing.T) { // forceDueNamed closes req's attached window at its maxDuration, as forceDue does for crName. func forceDueNamed(loop *branchWorkerEventLoop, req *AttachCommitRequest) { id := req.id() - if loop.openWindow != nil && loop.openWindow.pendingCR != nil && *loop.openWindow.pendingCR == id { - loop.openWindow.timers.maxAt = time.Now().Add(-time.Millisecond) + if loop.windows.only() != nil && loop.windows.only().pendingCR != nil && *loop.windows.only().pendingCR == id { + loop.windows.only().timers.maxAt = time.Now().Add(-time.Millisecond) loop.closeOrArmWindow() loop.endWake(0) } @@ -203,7 +203,7 @@ func TestWriteGate_ASaveWhoseWritesTheRenderGateDroppedSaysSo(t *testing.T) { serviceAttach(loop, req) writeTo(loop, "dropped") - require.Nil(t, loop.openWindow, "the gate dropped the write, so no window opened") + require.Nil(t, loop.windows.only(), "the gate dropped the write, so no window opened") expire(loop) loop.pushPending() @@ -349,7 +349,7 @@ func TestWriteGate_ARenderGateClosingDuringFinalizeStopsTheWholeWindow(t *testin serviceAttach(loop, req) } writeTo(loop, "new") - require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.windows.only()) require.True(t, worker.normalWritesAllowed(crTarget, "default"), "the first look finds the gate open") // Close the gate from inside the finalize, at the GitTarget read that resolves its metadata. @@ -372,7 +372,7 @@ func TestWriteGate_ARenderGateClosingDuringFinalizeStopsTheWholeWindow(t *testin } loop.pushPending() - assert.Nil(t, loop.openWindow) + assert.Nil(t, loop.windows.only()) assert.Empty(t, loop.pendingWrites, "nothing is retained") assert.Equal(t, before, remoteHead(t, serverRepo), "nothing reached the remote") if withRequest { diff --git a/internal/telemetry/exporter.go b/internal/telemetry/exporter.go index 7bde4072..1cfcee50 100644 --- a/internal/telemetry/exporter.go +++ b/internal/telemetry/exporter.go @@ -254,9 +254,9 @@ var ( GitWritesDroppedTotal metric.Int64Counter // GitCommitWindowsTotal counts closed commit windows, labelled by {gittarget_namespace, // gittarget_name, close_reason, timer_source}. close_reason says what ended the window - // (idle_timeout, max_duration, attach_next, identity_change, buffer_limit, resync_before_apply, - // shutdown, foreign_commit for a Git host's push notice, apply_due for the - // flush before an apply); timer_source says whose timers were in effect: the GitTarget's, + // (idle_timeout, max_duration, attach_next, identity_change, path_conflict, buffer_limit, + // resync_before_apply, shutdown, foreign_commit for a Git host's push notice, apply_due for + // the flush before an apply); timer_source says whose timers were in effect: the GitTarget's, // or a CommitRequest's that attached and replaced them. Every window is counted exactly once, at // the start of its finalize, so a window that then produces no diff or fails to commit is counted // too. A push replay rebuilds commits and closes no window, and a CommitEmpty record has none.