Skip to content

fix(git): close the gaps the second review of concurrent windows found - #470

Open
sunib wants to merge 7 commits into
fix/commit-window-stack-roundupfrom
fix/concurrent-windows-review-2
Open

sunib wants to merge 7 commits into
fix/commit-window-stack-roundupfrom
fix/concurrent-windows-review-2

Conversation

@sunib

@sunib sunib commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Stacked on #469. It answers the second review of the concurrent-window stack (#466 to #469), and adds the flagship e2e.

Fixes

  • Withheld-field record per GitTarget. The record and the last accepted versions were keyed by object alone, so an apply withheld for team-a set team-b's field of a same-named object back, or kept team-b's object out of Git. Both are now keyed by {GitTarget, object}, and a recorded edit is looked up only in the apply windows of appliers covering that GitTarget. A GitTarget reads one source cluster, so this also separates clusters.
  • Queued writes at shutdown. A clean shutdown pushed the open windows, then drained the FIFO unhandled. That dropped live writes Enqueue had already accepted (a watch may have moved its cursor past them). Shutdown now decides the writes on the FIFO first, each into its author's window, then closes the windows and pushes. It only takes what the FIFO held when shutdown began; resyncs, refreshes and attaches drain as before.
  • Model evidence for a footprint check. The model matched FootprintChecked by tree alone, so an unchecked commit returning to a checked tree from another parent passed. The log line now carries parentTree, and the model matches both.

Visibility

git_commit_windows_open and git_commit_windows_single, both per branch and read at scrape time: how many windows are open side by side, and whether a worker has fallen back to one at a time until restart. The second one gets an alert in the metrics guide.

Tests

  • test/e2e/concurrent_authors_e2e_test.go, label concurrent-authors. Alice and bob are impersonated and interleave creates and an update on a GitTarget with a 120s window. Nothing is committed while they work. Each then saves, and each save must be exactly one commit with their author, their message and exactly their files. The branch is seeded first, because a second window only opens once the folder's bootstrap files are committed. Passed locally on its own.
  • Unit tests for the cross-target record (both reverted-field and made-again cases), the footprint witness, and queued writes at shutdown, all mutation-checked. Also gauge tests, and three cases the review checked by hand: windows stay side by side through the push cooldown, a failed push keeps its writes beside open windows, and an existing document at a custom path gets a footprint.

Docs

The architecture, the current-behaviour commit-window spec, the pipeline diagram and the configuration guide no longer describe one window at a time. The design now says:

  • what it promises: commit grouping, not a transaction;
  • that an existing document qualifies wherever it lives;
  • that stage 4's flag was decided against, with the gauges in its place.

TODO records the footprint-index measurements and the memory the byte budget doesn't charge.

Not here

Stage 3's model work stays the release gate in docs/TODO.md: retained failures across wakes, replay, buffer pressure, reach floors and target changes.

🤖 Generated with Claude Code

sunib and others added 7 commits October 29, 2026 07:39
The withheld-field record and the last accepted versions were keyed by
the object alone. Two GitTargets on one branch can mirror objects of one
name, from two clusters or into two folders, so an apply withheld for
team-a set team-b's field back to team-a's accepted value, or kept
team-b's object out of Git altogether.

Key both by GitTarget and object, and look for a recorded edit only in
the apply windows of appliers that cover that GitTarget.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The model took the worker's FootprintChecked line as evidence for any
commit with the same tree. A later commit that returns to a checked tree
from another parent, made with the check skipped, passed as checked.

Log the parent's tree with the check, and have the model match the tree
and the parent's tree together: a rebuild onto the same parent still
matches, another change does not.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A clean shutdown closed the open windows and pushed them, then drained
the FIFO without handling it. A live write Enqueue had already accepted,
and a watch had moved its cursor past, was thrown away: only a later
resync could bring the edit back, without its author or its window.

Decide the writes on the FIFO when the shutdown begins, before the
windows close, so the last push carries each in its author's window.
Only what the FIFO held then is taken, so a producer still sending
cannot keep the shutdown going. Every other item, which may read the
remote or bind a save, is drained as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…to one

Concurrent windows are on with no flag, and nothing but log lines said
whether they deliver anything, or whether a worker had fallen back to
one window at a time after a footprint violation.

Add two scrape-time gauges per branch worker: git_commit_windows_open,
the windows open side by side, and git_commit_windows_single, 1 once the
worker holds one window at a time until it restarts. The second gets an
alert in the metrics guide.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The architecture, the commit-window spec and the event pipeline still
said a branch worker holds one window at a time, so reviewers kept
reading two different implementations.

- the spec, architecture and pipeline diagram describe a window per
  author while footprints are proven disjoint, and serial otherwise;
- the design says what it promises (commit grouping, not a transaction:
  a deadline can still split a save, and a footprint proves files are
  independent, not objects consistent), that an existing document
  qualifies wherever it lives, and that stage 4's flag was decided
  against, with the gauges that replace its measurement period;
- the configuration guide says which edits a graceful stop commits;
- TODO records the footprint-index measurements and the memory the
  retained-byte budget does not charge.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… a custom path

Three cases a review checked by hand: a decided write is committed while
the cooldown holds its push, so later writes are still proven and open
beside the others; a failed push keeps its write committed and pending
while the other windows collect, and the retry publishes everything; an
existing document outside the built-in path is proven where it lives.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The flagship of concurrent commit windows, through the real path: alice
and bob, impersonated, interleave creates and an update on one GitTarget
whose window is two minutes long, and nothing is committed while they
work. Each then saves. Each save must come out as exactly one commit:
their author, their message, exactly their files, and alice's update
made after bob's writes inside hers.

On one window at a time, bob's first write closed alice's window, so
both the quiet wait and the commit contents would fail.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0667b426-3d41-4c2b-bb03-cf186a76b448
Commits

Reviewing files that changed from the base of the PR and between 8507025 and 93c589b.

Files selected for processing: 18
  • docs/TODO.md
  • docs/architecture.md
  • docs/configuration.md
  • docs/design/concurrent-author-windows.md
  • docs/design/event-pipeline-overview.md
  • docs/interpreting-metrics.md
  • docs/spec/commit-window-refactor.md
  • internal/git/apply_window_carry_test.go
  • internal/git/branch_worker.go
  • internal/git/branch_worker_metrics_test.go
  • internal/git/footprint.go
  • internal/git/publication.go
  • internal/git/window_admission_test.go
  • internal/git/window_model_test.go
  • internal/git/withheld_fields.go
  • internal/telemetry/exporter.go
  • internal/telemetry/gauges.go
  • test/e2e/concurrent_authors_e2e_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Oct 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.36364% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/git/branch_worker.go 90.4% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@sunib
sunib added this pull request to stack #464 October 29, 2026 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant