Skip to content

fix(relay): Git pushes report and publish what actually landed - #9064

Closed
jmecom wants to merge 2 commits into
mainfrom
jm/git-push-path-fixes
Closed

jmecom wants to merge 2 commits into
mainfrom
jm/git-push-path-fixes

Conversation

@jmecom

@jmecom jmecom commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

This fixes three defects in the relay's Git push path. Each one made what a client sees differ from what the relay published. A Quint model of the push path found all three, and it is included as docs/spec/GitPushPath.qnt.

1. Partially accepted pushes published nothing. Without --atomic, git receive-pack applies each ref update independently. A stale old value, or a ref-name conflict such as feature vs feature/x, rejects one ref while the others land in the hydrated workspace and are reported ok. finalize_push treated any ng line as a rejected push: it skipped the CAS and returned git's report verbatim. The client then recorded ref updates that the relay had discarded. Now publication is skipped only when every ref is rejected, as with a pre-receive hook decline. cas_publish snapshots the workspace refs, so exactly the accepted updates are published.

2. Ref-state events could sort an older state first. kind:30618 events are stored side by side, and readers take the one with the greatest created_at, falling back to the lowest id. Each event was stamped with the emitting instance's clock. Two pushes in the same second therefore tied, and the older state won about half the time; a push on a slower clock sorted before its predecessor. Desktop then showed stale refs. Now each published manifest carries published_at, strictly greater than its parent's, and the event uses it as created_at. After an unstamped parent, such as the announce seed or an existing manifest, the stamp is now + 1. The field is omitted when absent, so the shared empty seed and existing manifests keep their bytes and digests. Relays without this change ignore the field when reading.

3. A landed CAS was reported as a failure when the serving lease lapsed after it. finalize_push and the default-branch setter ran the pointer CAS under ServingWriteGuard::protect. That discards a completed operation when the lease check after it fails, and a single missed heartbeat renewal is enough to fail it. The client got a 503 and no 30618 was emitted, although the pointer had moved. Both paths now use the existing protect_landed, keep the CAS outcome, and let the lease-guarded 30618 insert decide whether publication may continue. If deletion has actually fenced the community, that insert still fails and the response is the existing 500 "push committed but ref-state publication failed". A failed lease release after publication is logged instead of turning success into a 503.

Not changed: lease loss during the CAS future still drops it and answers 503, and the relay cannot tell whether the pointer PUT landed. The heartbeat still declares the lease lost on the first failed renewal. Both are noted in the spec as remaining limitations.

How the model was used

The existing TLA+ spec treats receive-pack as one atomic step over a single ref, so none of these defects can occur in it. GitPushPath.qnt follows the code instead:

  • info/refs and the POST hydrate separately.
  • Refs are applied one at a time, including the D/F pair.
  • The lease can be lost around the CAS.
  • Events are stamped with a per-instance clock.

Its invariants describe what clients observe: no false success, no false failure, and the selected 30618 matching the pointer once pushes settle. One module per defect pins each counterexample to that defect. Exhaustive checking to 8 steps with quint verify (Apalache) finds each defect within 3 to 7 steps. With all three fixes, the model passes 200,000 random 14-step simulations and the 8-step exhaustive check. These are bounded checks, not proofs.

Related issue

None found.

Testing

  • receive_pack_partial_rejection_applies_and_publishes_accepted_ref runs real git with a D/F conflict, plus report-scan unit tests.
  • finalize_push_lease_signal_after_landed_cas_still_publishes (Postgres + MinIO) fires the lease-loss signal as the CAS completes and expects a 200 and a 30618. On the previous code it fails with a 503.
  • Manifest tests cover pinned bytes with published_at, legacy manifests parsing unchanged, and next_published_at cases (same second, parent's clock ahead, unstamped parent). build_ref_state_event uses and signs the stamp. live_cas_publish_compacts_and_rehydrates asserts that the child's stamp follows a parent stamped by a clock that runs ahead.
  • cargo test -p buzz-relay --lib api::git -- --include-ignored passes against local Postgres, Redis and MinIO, including the default-branch integration suite.
  • Local MinIO intermittently rejected uploads with RequestTimeTooSkewed, though the host and container clocks agreed. Every default-branch integration test failed that way at least once, and each has also passed: the suite passed in full on two runs.
  • cargo fmt and cargo clippy -p buzz-relay --tests -D warnings are clean.
  • git_partial_push_publishes_accepted_refs was added to e2e_git.rs. It is ignored like the rest of that suite, and I did not run it against a live relay.

Without --atomic, git receive-pack applies each ref update independently.
A stale old value or a ref-name conflict rejects one ref while its
siblings land in the hydrated workspace and are reported ok to the client.
finalize_push treated any ng line as a rejected push, skipped the CAS, and
returned git's report verbatim, so the client recorded ref updates the
relay discarded.

Skip publication only when the report rejects every ref, as a pre-receive
hook decline does. cas_publish snapshots the workspace refs, so only the
accepted updates are published.

Signed-off-by: Jordan Mecom <[email protected]>
…tcomes

kind:30618 ref-state events are stored side by side, and readers take the
one with the greatest created_at. Each event was stamped with the emitting
instance's clock, so two pushes in one second tied (lowest id wins, so the
older state won about half the time), and a push on a slower clock sorted
before its predecessor. Each published manifest now carries published_at,
strictly greater than its parent's, and the derived event uses it as
created_at. The field is omitted when absent, so the shared announce seed
and existing manifests keep their digests.

finalize_push and the default-branch setter ran the pointer CAS under
ServingWriteGuard::protect, which discards a completed operation when the
lease check after it fails. A missed heartbeat renewal after a landed CAS
therefore answered 503 and skipped the 30618, although the pointer had
moved. Use protect_landed, keep the CAS outcome, and let the lease-guarded
30618 insert decide whether publication may continue. A failed lease
release after publication no longer turns success into 503 either.

Add docs/spec/GitPushPath.qnt, the Quint model of the push path that
found these defects and the partial-push one, with a module per defect.

Signed-off-by: Jordan Mecom <[email protected]>
@jmecom
jmecom requested a review from a team as a code owner November 4, 2026 17:14
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Nov 4, 2026
@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

  • Exact PR diff: 5b738eb647fb793057bcfe11638a5a1781b2dd76...f19781aad5c1b3d00715f17b3f3b3c235f66d475
  • Model: gpt-6.1-sol

💡 Click "edited" above to see earlier reviews for this PR.


Review Summary

Overall Risk: NONE

No concrete security, correctness, or reliability regressions found in the authorized PR range.

Findings

No concrete security, correctness, or reliability findings were identified.

Notes

  • Review used read-only source and history inspection. Builds and tests were not run, as requested.

Generated by Codex Security Review |
Requested by: @jmecom |
Workflow run

@joahg joahg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 Read the report scan, the published_at stamping (omitted when absent, so existing manifest digests are unchanged) and the protect_landed change in finalize_push and set_default_branch. Partial-accept publish and the post-CAS lease handling look correct.

@kazemek21 kazemek21 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Kyle reviewed the PR and the Nexus review-tracker notes and approved with the non-blocking comment below.

Posted by Codex (gpt-6.1-sol) on behalf of kylez

/// An unstamped parent (the announce-time seed, or a manifest written before
/// the field existed) had its kind:30618 stamped at or before `now`, possibly
/// in this same second, so the successor takes `now + 1` to sort after it.
pub fn next_published_at(parent: &Manifest, now: u64) -> u64 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] Gate stamped writes until legacy writers are drained

During the chart’s rolling update, an old replica can still handle a push after a new replica. Two new-replica pushes in second T produce stamps T+1 and T+2; an old replica pushing at T+1 drops published_at and emits T+1. The pointer advances, but useRepoRefs keeps displaying the older T+2 state.

Should we gate activation until all serving writers understand the stamp, with an explicit rollback constraint, or preserve ordering across mixed-version writes?

Posted by Codex (gpt-6.1-sol) on behalf of kylez

jmecom added a commit that referenced this pull request Nov 13, 2026
…ocol by refinement (#9425)

## Summary

This PR is stacked on #9064. It does two things to the
Git-on-object-storage code in `buzz-relay`. It replaces the TLA+ model
of the push protocol with one that states what the protocol is supposed
to implement and checks that it does. It also changes the rule by which
the relay decides whether to publish a push, because the new model shows
that the rule #9064 introduces still publishes requests in which git
accepted nothing.

### Relationship to #9064

The branch is `main`, then the two commits of #9064, then four commits
of this PR: the specification work, the publish-rule change, a review
fix merged from #9429, and a fix to `check.sh`. A merge of `main`
follows them; it was needed because `main` changed `Justfile`, which
#9429 also edits, after the branch point. The first two commits are
#9064's, replayed onto current `main` without conflicts and otherwise
unchanged. The base of this PR stays `main` because #9064's branch is
about 200 commits behind it. Once #9064 merges, this branch can be
rebased and the diff shrinks to the last four commits. Until then,
review the commits after the first two.

This PR keeps two of #9064's three changes as they are: the
`published_at` stamp that orders kind:30618 events by the manifest
chain, and `protect_landed`, which lets a landed conditional write stand
when the serving lease lapses after it. It replaces the third, the
publish rule.

### The publish rule

#9064 publishes when git exits 0 and the status report has an `ok` line
or no `ng` line. That handles a push in which one ref is updated and
another refused. The report, however, is not evidence of what git did.

- git exits 0 for a request that carries no commands, and for a push the
pre-receive hook declined. A decline is reported only to a client that
negotiated `report-status`. For a request with no commands, and for a
decline when the client did not negotiate a report, there is no `ng`
line. The relay then writes the pointer and emits a relay-signed
kind:30618 event that names the caller as pusher, although the push
policy approved nothing.
- The report is read from a stream whose framing the client's
capabilities determine, so the decision depends on parsing a
client-shaped stream.

The decision is now taken from the workspace. `push_disposition`
compares the refs after receive-pack with the refs the workspace was
hydrated from. Unchanged refs are answered with git's own output and
nothing is published. Changed refs are published if git exited normally.
Changed refs after an abnormal exit fail the request. The snapshot that
is compared is the one `cas_publish` publishes, so git's per-ref report
is true of the published state. The status-report scanner and its tests
are removed, and the ref-name conflict case they covered moves into the
table-driven test described below.

Three smaller changes follow. The ref-snapshot parser now fails closed:
output that was not UTF-8 used to read as an empty ref set and a
malformed line was skipped, and either would publish a manifest that
lacks refs. `manifest_changed` in `finalize_push` could not be false and
is removed. Taking the snapshot before the publish decision moved its
resource-limit error (snapshot larger than 4 MiB, or more refs than a
manifest may hold) onto a path that answered 500; #9429 restores the 413
the publish path gave before and adds
`snapshot_resource_limit_answers_413`.

### What the specification work adds

- `docs/spec/GitRefDB.tla` is the specification: a ref database in which
a push takes effect in one step, is applied only if authorized and only
against the old values it names, and is reported truthfully.
- `docs/spec/GitOnObjectStore.tla` is the protocol: several refs, an
object graph with reachability, packs, compaction, the pre-receive hook,
the publish decision, the default-branch setter, repository creation,
and a reader. `git receive-pack` is represented by a stated contract
rather than modeled internally.
- The correctness statement is `Refines`: every behavior of the protocol
is a behavior of the specification. TLC checks it together with the
store invariants (fence, reader completeness, closure of published
manifests).
- `docs/spec/check.sh` pins TLC 1.7.4 by SHA-256 and runs 28 checks.
Eight instances must pass. Nineteen seeded design errors must each be
detected with the violation named for them. One coverage check confirms
that a behavior the properties depend on (git reporting `ok` for a
command that changes nothing) is reachable. A name passed to `check.sh`
that is not a check is an error, so a misspelled selection cannot report
success.
- Both earlier publish rules are among the seeded errors: the original
rule (exit 0 and no `ng`) and #9064's status-report rule. For each, TLC
reports a publication the policy did not approve
(`PublishIsAuthorizedChange`) and a violation of `Refines`.
- `docs/spec/GitPushPath.qnt`, the Quint model from #9064 that covers
the serving lease and the event timestamps, is extended. The publish
rule becomes a parameter with three values, a request may carry no
commands or come from a client without a status report, and a new
invariant, `publicationChangesRefs`, states that a published manifest
differs in its refs from the one it replaces. The implemented rule
satisfies all four invariants; the status-report rule violates the new
one with a zero-command request.
- `docs/spec/lean/GitPointer.lean` is a Lean 4 proof, with no
dependencies, of the pointer discipline for any number of clients, refs
and objects: the fence, closure, that every pointer change is a legal
step from the value it replaces, that the length of the pointer's
predecessor chain equals the number of pointer writes, and that a
successful conditional write implies no intervening write.
- `.github/workflows/spec.yml` runs TLC and Lean when a file under
`docs/spec/` changes. On this PR's first run the TLC job took 10 minutes
19 seconds and the Lean job 22 seconds. Quint is not run in CI.
- `docs/git-on-object-storage.md` is revised to match. The push listing
follows the implemented order. The verification section separates
definitions, assumptions, results checked by TLC, results proved in
Lean, results checked with Quint, evidence about the implementation, and
limitations. The invariant names cited by code comments and by migration
0002 are kept as aliases in the TLA+ module.

### Assumptions and limitations

The TLC results hold for the bounded instances in `check.sh`, not for
arbitrary sizes. The Lean proof is unbounded but covers the pointer
discipline only, not the refinement. The Quint results are from random
simulation (2,000,000 runs of at most 14 steps) and a bounded symbolic
check (8 steps, Apalache); neither is a proof for unbounded runs. All of
them rest on the assumptions named in the document: the object store's
conditional-write semantics, ETag equality implying body equality, no
SHA-256 collision among the values that occur, the stated contract for
`git receive-pack`, and the policy endpoint deciding authorization. The
correspondence between the models and the Rust code is established by
reading the code and by the tests below; it is not mechanized.

The document states one assumption that #9064's ordering fix depends on
and that no model covers (Assumption C1). A manifest whose parent
carries no `published_at` (the empty manifest written when a repository
is announced, or a manifest written before the field existed) is stamped
with the writing instance's current time plus one second. Its event is
selected over the parent's only if that value exceeds the parent event's
`created_at`, which was taken from the clock of the instance that
emitted it. This holds when one instance handles both, and otherwise
when the writing instance's clock is not behind the other's by more than
the time elapsed between the two events. The assumption concerns one
write per repository. The document lists its removal as a proposed
extension, for example by having the first push after an unstamped
manifest read the stored event's `created_at` and exceed it.

### Related issue

Related to #2621 (CI for the formal models). This adds the TLC job for
the git model and a Lean job; it does not cover the Tamarin models or
the NIP-PL scripts, so it should not close that issue.

### Testing

Run locally on the restacked branch:

- `docs/spec/check.sh`: 28 checks, each with the expected result (8
pass, 19 seeded errors violate the named property, 1 coverage witness
reached). About three minutes on 16 cores; the largest instance has
6,773,286 distinct states.
- `lake build` in `docs/spec/lean` (Lean 4.33.1): succeeds. The safety
theorems depend only on `propext`, `Classical.choice` and `Quot.sound`.
- Quint 0.33.0, `GitPushPath.qnt`: `quint run --main fixed --invariant
safety --max-steps 14 --max-samples 2000000 --seed 0x1` reports no
violation; `quint verify --main fixed --invariant safety --max-steps 8`
(Apalache 0.62.1) reports no violation; each single-defect module
produces the counterexample the document lists.
- `cargo fmt --all -- --check` and `cargo clippy -p buzz-relay
--all-targets -- -D warnings`: clean.
- `cargo nextest run -p buzz-relay --lib -E 'test(/^api::git::/) +
test(/side_effects/)'`: 158 passed.
- The storage-backed `finalize_push` tests
(`api::git::transport::track_c_tests::external_infra_minio_tests`, with
`--run-ignored ignored-only`) against local Postgres and MinIO: 5
passed.
- `e2e_git` against a local relay built from this branch, with Postgres,
Redis and MinIO: 3 passed (clone/push/fetch/force-push round trip,
concurrent push, partially refused push). The storage probe in that test
needs `BUZZ_E2E_GIT_COMMUNITY_ID` set to the local community's id.
- `just file-size-check` and `just security-review-check`: pass.

Two regression tests bind the changed decision to production code, and
both were confirmed to fail when the decision is forced back to the exit
status:

- `push_is_published_exactly_when_receive_pack_changed_refs` runs real
git through the production `run_git_at` and `observe_push` for seven
ways a push can end: a request with no commands, a push the hook
declines with and without a status report, an accepted push, an accepted
command that asks for the value the ref already has, a push in which one
ref is updated and another refused for its old value, and a push that
creates two refs whose names conflict, of which git creates one and
refuses the other. It needs no services. Which of the two conflicting
refs git creates depends on the git version (2.50 creates the first
command's ref, 2.56 on the flake-check runner creates the other), so the
test accepts either outcome and requires that each ref is in the
published snapshot exactly when git reported `ok` for it. The first
version of this case assumed one outcome and failed the Rust flake check
for that reason. Before #9429, `just test-unit` selected neither
`api::git::cas_publish::tests` nor `api::git::transport::track_c_tests`,
so this test ran only in the flake check; both modules are now in the
unit-test lane (45 tests, none needing services). It is Unix-only
because its hook fixture sets file modes; the first push of this PR
failed Windows clippy for that reason, and the test and its fixture are
now gated with `#[cfg(unix)]`. That gating has not been compiled for
Windows locally, so the Windows CI job is the check.
- `finalize_push_publishes_nothing_when_receive_pack_changed_no_ref`
calls `finalize_push` with an unchanged workspace and checks that the
pointer is not written and no kind:30618 event is stored. It needs
Postgres and an object store.

Not done, or not covered:

- No CI job selects the storage-backed `finalize_push` tests or
`e2e_git`, so the second regression test and the end-to-end tests are
evidence only when someone runs them. Adding them to CI needs a change
to the test fixture, which applies migrations itself, and is left as a
follow-up.
- `just ci` was not run in full.
- No human has tested the change. The human-testing step of the
readiness checklist in `AGENTS.md` is not met, and
`buzz-review-completed` is not claimed.

The local secret-scanning hook (`sadscan`) was skipped for the commits
and pushes that touch `crates/buzz-relay/src/api/git/settings_tests.rs`.
It scans whole files and flags the P-256 test key fixtures that file
already contains on `main`; this branch adds no key material.

---------

Signed-off-by: Jordan Mecom <[email protected]>
Signed-off-by: Tom Brow <[email protected]>
Co-authored-by: Tom Brow <[email protected]>
Co-authored-by: studio <dc2fc481343672df31a689840c09299972e704dac592e0da6f18486646a7ee3e@localhost>
@jmecom

jmecom commented Nov 13, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Closing as superseded by #9425, which is now merged (1fb33e4). #9425 carried both commits of this PR and kept two of its three changes as they are: the published_at stamp that orders kind:30618 events by the manifest chain, and protect_landed for a conditional write that landed before the serving lease lapsed. It replaced the third, the status-report publish rule, with a comparison of the workspace refs before and after receive-pack; the reason is in the description of #9425. e2e_git::git_partial_push_publishes_accepted_refs from this PR is on main unchanged.

The open review thread here about stamped writes during a rolling update with older replicas still applies to the code on main.

@jmecom jmecom closed this Nov 13, 2026

This branch was previously deployed

1 inactive deployment
codex-review — f19781aa Deployed Nov 4, 2026 by jmecom via Run Codex Security Review #12548
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex-security-review-current The posted Codex security review matches its recorded range.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants