fix(sync): hold the ref and record forward intent before a REST write's CAS by ThomasK33 · Pull Request #737 · coder/openstorage

ThomasK33 · 2026-11-06T14:39:21Z

Why

Every accepted write to a forward_on_push repository on the local schedule (SQLite and S3-WAL, ADR-50/51) must reach the upstream. #678 made git pushes record their forward intent before the authority CAS. REST ref writes still recorded it only after the CAS, in events.RefStore's sync poke. Serve dying between the two, or a failed RequestPush, left an accepted write that no run ever forwards: nothing polls the upstream. This is verify-lane-schedule finding 1 for REST writes (the model's WritesComplete assumption and its probe NoCompletionAssumption.cfg).

What changes

A REST write holds the ref and records its intent before the CAS. On the local schedule, serve's events.RefStore decorator now sets the new HeldBeforeCAS option to restIntentBeforeCAS. For a forward_on_push repository and a caller that may forward (forwardDeniedFromContext), it takes the ref's push hold in serve's cache (new githttp.HoldRef), then runs RequestPush. The hold ends when the inner CAS returns. A run that claims the intent waits in its publication wait until then, so it cannot snapshot the ref before the CAS. A failed read, hold or intent write refuses the write. The forward gate comes first, so a caller that may not forward is never refused by this step, the same rule fix(serve): refuse an intent-recording ref update when the hook cannot wire its eventing #724 gave the hook. The post-CAS poke stays. PostgreSQL is unchanged (fix(sync): never lose a PostgreSQL forward between the ref CAS and its push run #679 records the intent in the CAS transaction).
HeldBeforeCAS gets the ref name and returns a release. events.RefStore runs it after BeforeCAS. It defers the release before the inner CAS and also calls it right after the CAS, before the poke. runBeforeCAS wraps the release in sync.OnceFunc, so it runs once. A CAS that panics, which serve's API recovers, still ends the hold. Without that, the hold's refresher would keep it live until serve restarts, and every later write to the ref, and every push run of the repository, would fail. The hook's BeforeCAS and wireHookEventing are unchanged.
A refused push run emits no event. RunPush emitted repo.push_upstream.started before its loopback fetch. A local run whose fetch the server refuses with 423 (a stale held ref) then restored its request with no terminal event, so a webhook consumer saw a run that never ends. started now goes out once the fetch has its snapshot (or when the run ends otherwise). This path gets more frequent with this PR, because REST writes now hold refs too. On PostgreSQL the started event now also goes out after the loopback fetch, not before it.
The model describes REST writers. RReserve (hold), RMark (intent), RCas, RRelease, RPoke, RAbort, RCrashRest, behind the new code fact RestMarkBeforeCas. RCrashRest is always allowed now, so WritesComplete and NoCompletionAssumption.cfg are gone. MarkBeforeCas follows the new runBeforeCAS call, which now returns the release.
A write that gives up on a held ref gets 503. reservePushHolds marks both of its give-ups (the reservation lock and a conflicting hold, after pushHoldWaitMax) with the new githttp.ErrRefHeld, unless the caller's own context ended. The messages that a pushing git client shows stay the same. The API's 500 writers (internalErr, wcInternal, wmInternal) answer such an error with 503, Retry-After: 1 and ref is busy: a concurrent push or ref write holds it; retry shortly.
Docs. The ADR-50 amendment adds the REST write's hold and intent, names the meta.ErrNotFound exception (no upstream link), and loses an odd line break. docs/reference/http-api.md says when an outbound run emits no event, that repo.push_upstream.started goes out after the loopback fetch on every backend (also in webhooks.md), or right before failed for a run that fails before or at that fetch, how a forwarding REST write waits for the same ref or a directory/file relative (10 s, then 503 with Retry-After: 1) and when it answers a plain 500, that a failed read of the upstream link refuses a ref write on any repository (SQLite and S3WAL, for a token that passes the forward gates), that a write that loses its CAS still costs one forward run, and lists the new 503. docs/reference/webhooks.md names the push runs that send no event (held back by a write in progress) or no terminal event (cancelled, or claim taken over).

Behaviour a user can see

A REST ref write on SQLite or S3-WAL to a forward_on_push repository by a caller that may forward now waits for a concurrent push that holds the same ref, up to pushHoldWaitMax (10 s), as two pushes do. After that wait it fails with 503 and Retry-After: 1. Push holds normally last milliseconds.
On the local schedule, a REST ref write by a caller that may forward, whose upstream-row read fails, is now refused with 500. Before, the write landed and its poke was lost.
Every non-ephemeral REST ref write on the local schedule by a caller that may forward reads the upstream row once more, before the CAS. With forward_on_push, the intent write moves before the CAS, and the post-CAS RequestPush finds it already set. This is read from the code, not measured.

Known doc gaps, fixed in the E PR

Two doc gaps that a claim check found at this head. Both are fixed in the follow-up PR (branch fix-pg-strict-fetch-2):

docs/reference/webhooks.md ("with these exceptions") misses one path: when the metadata write at the end of a push run fails (finish push ..., internal/sync/worker_push.go), the run also ends with started and no succeeded or failed.
docs/reference/http-api.md ("Ref writes that forward") does not exempt ephemeral-namespace writes from the failed-link-read 500. Those writes skip the hold and the intent (EphemeralUpdate in RefStore.runBeforeCAS, internal/events/refstore.go), so a failed link read does not refuse them.

Verification

Red first, each test committed before its fix:

TestServeRestIntentBeforeCAS (cmd/openstorage, 6d0f15e6) writes through buildServeEventing's decorator on SQLite and S3-WAL, and reads the row and the ref's hold at the instant the CAS commits. Old code: at the authority CAS push_pending = 0, ref held = false; want the intent recorded and the ref held, and with a closed SQLite store the write was accepted.
The TestLocalSchedulePublicationFence case stale held snapshot restores force and attempt (internal/sync, 9d38dda8) expects no event from the refused run. Old code: got 3 events ... want 2 ([repo.push_upstream.started repo.push_upstream.failed]).

More tests: TestBeforeCASRunsFirstAndRefuses covers both options, the ref name and the release order. TestServeRestIntentBeforeCAS also checks that a write-only caller's write lands when the store fails. TestLocalRESTWriteForwards (internal/githttp) runs a real serve node on SQLite and S3-WAL: a branch and a tag created through the API on a forward_on_push repository reach the upstream through the real handlers. TestHeldBeforeCASReleasesOnPanic (internal/events) and the panicking_cas case of TestServeRestIntentBeforeCAS check that a panicking CAS ends the hold. Before f92bc9d4 they failed with 0 releases and with a retried write refused by a concurrent push holds a ref conflicting with refs/heads/main. The held_elsewhere case refuses a write while another writer holds the ref, with an error that matches githttp.ErrRefHeld. TestRefHeldIsServiceUnavailable (internal/api) checks the 503 from all three 500 writers. TestReservePushHolds_* check the mark, and that a canceled caller and unreadable holds do not get it. Removing the deferred release, the sync.OnceFunc wrap, the mark, the canceled-caller rule or any writer's mapping turns one of these tests red. TestRestIntentBeforeCASPaths (cmd/openstorage) drives the intent step through each answer of the store and the cache: no upstream link, a failed row read, no forward_on_push, a failed hold, a failed intent write (the hold ends), a link gone before the intent write, and a plain forward. TestServeRestIntentBeforeCAS also asserts a plain (not forced) intent, a write with no upstream link, and a lost CAS that keeps its intent. TestHoldRefHoldsItsRefUntilRelease and TestHoldRefReleasesWhenItCannotReserve (internal/githttp) pin the held ref name, the refresher stop and the release on a failed reservation. TestPushRunStartedPrecedesUpstreamPush (internal/sync, River path) checks that repo.push_upstream.started goes out before the upstream push. A verifier changed each of these answers in the code once, and the tests then stayed green. Each change now turns a test red. TestHoldRefRefreshesPastItsTTL checks that a hold outlives its TTL while the write is in flight, TestHeldBeforeCASNilReleaseAsserts that a HeldBeforeCAS without a release stops before the CAS, and TestReservePushHolds_LockOpenFailureIsNotRefHeld that a reservation lock that cannot be opened stays a plain error (500), not a 503. The tests' "not held" checks wait 5 s instead of 1 s or 200 ms (holdCheckBound): the publication wait checks its context before its first poll, so a stall past a short bound read an unheld ref as held.

verify-lane-schedule/check.sh --record at 2c7c0fdd: RESULT: PASS. 12 facts TRUE. LaneSchedule 3,535,660 states, OtherWriters 56,705,945 (two REST writers now hold and mark), Liveness 348,735, NoLeaseAssumption.cfg fails as expected. mutate.sh breaks each fact in the code: COUNT: 12/12 at this head (2e7cfc8d). mutate.sh --model: COUNT: 12/12 at 2c7c0fdd (the model has not changed since).
go test -race, with -tags=openstorage_viewcheck except on the local backend, as in CI, at 2c7c0fdd (the head differs only in mutate.sh): cmd/openstorage, internal/events, internal/sync, internal/api, internal/jobs, internal/gitread and internal/simtest pass on PostgreSQL, local and S3-WAL, and internal/githttp passes on all three.
Round 3 then merged main (af601025, with fix(githttp): keep the hook's store errors off the pusher's screen and give them to the operator #742, fix(api): preview merge conflicts at paths longer than one git argument #739 and perf(api): resume a recursive files page at its cursor #715) and recorded again: check.sh --record at 916473e2 gives RESULT: PASS with the same 12 facts and state counts, and mutate.sh gives COUNT: 12/12 there. At 916473e2, cmd/openstorage, internal/events, internal/sync, internal/api, internal/jobs, internal/gitread and internal/simtest pass on PostgreSQL, local and S3-WAL, internal/githttp passes on local, and lint, vet, links and both fingerprint checks pass. Each new test's guard was removed once, and its test turned red.
golangci-lint 2.15.0: 0 issues. go vet ./... clean. Doc links: 0 broken. Both --fingerprints-only checks pass.

Size

1,381 changed lines: 213 of production Go, 851 of tests, and 317 of model, skill and docs. The model change has to ship with the code: the lane-schedule-fingerprints CI job fails while covered code and the recorded model disagree.

Notes for reviewers

This branch was cut from #678's head, so its commit list repeats #675 and #678. Its own commits start at 6d0f15e6. 65189135 merges main with the strategy ours after a135e2a3 merged #718: git diff 1996cc32 65189135 shows only this branch's change. 065dad8a merges main with #724, 58e3cab5 with #726 and af601025 with #742, all normally. The hook's code (wireHookEventing, intentlessRefStore) is unchanged by this PR. Round 2 (584a39eb onward) adds the panic-safe release, the 503, the tests that a verifier's code changes showed missing, and the docs. Round 3 (ff2472ac onward) adds three more guard tests, the longer "not held" bound and the docs.

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: xhigh

…h-lane schedule Model-checks the ADR-50 push lane (SQLite and S3-WAL) against git pushes, push-upstream calls, runs, steals, retries, restores, crashes and shutdown. Nine code facts tie the model to both backends and the worker; fingerprints in the lane-schedule-fingerprints CI job flag covered-code changes. The NoCompletionAssumption probe records two findings: a push that dies after its authority CAS can lose its forward. Change-Id: I07baac11eba4d51333064aa0fb5e90dd84e48653 Signed-off-by: Thomas Kosiewski <tk@coder.com>

…e model variables Change-Id: Iafc903e2e9a284d33bd0cb076b93f5c621837eb2 Signed-off-by: Thomas Kosiewski <tk@coder.com>

…print the push-hold path Codex round 1 on #675: fingerprint the push-hold lifecycle, both refspec vars and LocalScheduler.Start; model pushes that record no intent; state that ref deletions are not forwarded by design. The model now counts a write as owed at its CAS, adds REST writes (OtherWriters.cfg) and a HeldRefsFresh probe that fails with no fault. Change-Id: I863914ac2567ec75ba1b8f92a9b68663b3816c6d Signed-off-by: Thomas Kosiewski <tk@coder.com>

…model forward-denied REST writes Codex round 2 on #675: fingerprint every function of events/refstore.go and githttp/publication.go and every push-hold helper in hydrate.go; add QuietRestWriters; state that rewrites are out of scope (plain forwards of a non-fast-forward fail as FailRejected by design). Change-Id: I6a4da0c6e58ec9300fa36353932fc34ae10d74a7 Signed-off-by: Thomas Kosiewski <tk@coder.com>

Two failing tests for the verify-lane-schedule findings: - TestLocalHookIntentAuthorizationAndFailures now checks the row at the instant the hook's authority CAS commits (finding 1): push_pending is still 0 there, so a hook killed right after the CAS loses the forward. A metadata store that fails must refuse the update; today the CAS is accepted with no intent. - TestLocalForwardHeldStaleRefRestores replays finding 2 on the real Hydrate, push holds and Worker.RunPush: a REST write, a run past its publication wait, then a push reserving its hold. The loopback fetch forwards the stale held ref and the lane ends done. Change-Id: Ia48fdc669feae8e67b3483b379c606b7ee5a888b Signed-off-by: Thomas Kosiewski <tk@coder.com>

…held snapshots On the local schedule (SQLite, S3WAL; ADR-50) two interleavings lost an accepted write's forward to the upstream (verify-lane-schedule findings 1 and 2). - The update hook records RequestPush before its authority CAS, behind the forward poke's own gates, after the ref's push hold is in place. A failed intent write refuses the update. The new events.RefStoreOptions.BeforeCAS step carries it; only the local hook wiring sets it, and the hook's one upstream-row read is shared with the post-CAS poke, so no push pays an extra S3 round trip. - A local push run's loopback fetch (upload-pack with a sync claim on a StrictSyncFetch server) fails with 423 Locked when hydration would skip a push-held ref whose cached value is older than the authority. RunPush then restores its request, like a failed publication wait, instead of forwarding the stale ref and ending done. ADR-50 gets an amendment for the new hook order, and the HTTP API reference says when an outbound run ends without a terminal event. Change-Id: I1b367a2151cd027696df6a114df3d7a6da21d479 Signed-off-by: Thomas Kosiewski <tk@coder.com>

…he local schedule A sync-claim fetch of a ref that a push holds while the cache lags the authority gets 423 Locked from a SQLite or S3WAL node, and still reads on PostgreSQL, whose runs have no restore. An ordinary fetch reads the cached value on every backend. Change-Id: Ia58b43b1b747e0014852f0cf61ab2a1b67656d85 Signed-off-by: Thomas Kosiewski <tk@coder.com>

…rd-git Change-Id: I967eee2110d0e994f0185e16b981f9e4f744421a Signed-off-by: Thomas Kosiewski <tk@coder.com>

…eld snapshot The verify-lane-schedule model now describes the fixed code: - A git push reserves its hold (the cache may be older than the authority), records its intent (WMark, behind MarkBeforeCas), wins its CAS, pokes again (WPoke), and publishes. It can die after its CAS. - A run's snapshot of a held ref that the cache has not caught up with restores the run (RSnapHeld, behind StrictSnapshot). HeldRefsFresh and its probe go: the code no longer relies on it. WritesComplete now covers REST writes only, whose intent still follows their CAS; its probe runs one REST write. Two new code facts guard the new steps, and each was broken once in the real code (COUNT: 11/11). Change-Id: I59c6b14d264013191bfdcc5f6518c1b988cd8b0c Signed-off-by: Thomas Kosiewski <tk@coder.com>

Change-Id: Ife43711474301d0a26ca7ea352ee7aeaaff4fbf9 Signed-off-by: Thomas Kosiewski <tk@coder.com>

… strict snapshot Change-Id: I0b5aadcbb4631b4a842188e0144cb9dfc6ed7bfd Signed-off-by: Thomas Kosiewski <tk@coder.com>

…ok's update The forward intent now goes in the row before the CAS, so a hook that cannot read the S3WAL metadata document can no longer accept the push and drop its intent: the format check still fails open, and the intent step refuses the update. PostgreSQL keeps failing open. Change-Id: I996a8017a42c0ef42c91cce8191e101d4b53b0ef Signed-off-by: Thomas Kosiewski <tk@coder.com>

Change-Id: I5cff827bead0971223101323611f67c97909a392 Signed-off-by: Thomas Kosiewski <tk@coder.com>

Change-Id: Ife03e411e8a9cbeb738af3512ce56535a56baba8 Signed-off-by: Thomas Kosiewski <tk@coder.com>

Change-Id: I7770390f42967ab4702133ec5ffac4b2e7acecf4 Signed-off-by: Thomas Kosiewski <tk@coder.com>

The hook's intent step before the CAS and its sync poke after it shared one upstream-row read. A pull that finished between that read and the CAS then suppressed the pull poke with a stale Syncing, though it could not have seen the write (Codex P2 on #678). The poke now reuses the read only for a missing link and for a forwarding link, whose forward poke reads nothing else; a linked repository that does not forward reads the row again. Change-Id: Iaf86367919bd2e04ab416321548a4fc7c7382848 Signed-off-by: Thomas Kosiewski <tk@coder.com>

Change-Id: I3e3a99160caa83cd7c6aec758d996402268ba6c5 Signed-off-by: Thomas Kosiewski <tk@coder.com>

…s not open On SQLite and S3WAL the hook records the push's forward intent before the CAS. When the metadata store does not open, it used to accept the update with no events and no intent. It now refuses the update, like a failed read; PostgreSQL keeps failing open. Also fix the publication-wait comment in buildLocalSyncQueue, which still said hooks record intent after the CAS. Change-Id: I9abe5de79a3c3670a4563bf6ed3c04fc1de1b76d Signed-off-by: Thomas Kosiewski <tk@coder.com>

Change-Id: Ic043387eaaca6c4cbc7e1390971b25cf604bb9ad Signed-off-by: Thomas Kosiewski <tk@coder.com>

Change-Id: Ic6f8b8f12f24460b5a5fda0bb29e1c8cabc6c1a5 Signed-off-by: Thomas Kosiewski <tk@coder.com>

…uracy changes Change-Id: I20aac425fbcb188297405276954e6cce7843342f Signed-off-by: Thomas Kosiewski <tk@coder.com>

…ta store does not open The hook refused every ref update when the SQLite or S3WAL metadata store did not open, ephemeral ones included, which main accepts. It now refuses only updates that would record forward intent, like a failed read: ephemeral updates and a sync run's own loopback push keep failing open (the loopback push's claim fence refuses it when the store is down, as on main). The test covers all four cases and fails without the fix. Change-Id: Ib42ddab6de05cb169a50f644f0cd2acf9238c554 Signed-off-by: Thomas Kosiewski <tk@coder.com>

Change-Id: I1a453ad0ba3f3e7c31004db6239837bd4b191f66 Signed-off-by: Thomas Kosiewski <tk@coder.com>

the WaitBeforeSnapshot break no longer matched and mutate.sh counted 10/11. Also correct why PostgreSQL leaves StrictSyncFetch off: a River run now restores after a failed publication wait, but not on a 423. Change-Id: I71774bab023c5ac19841c4464b820d5cf79bca10 Signed-off-by: Thomas Kosiewski <tk@coder.com>

Change-Id: Ib57fdfb972853f6f778efdd69495112629eb8181 Signed-off-by: Thomas Kosiewski <tk@coder.com>

Change-Id: Icca467bf7789d43b33f58983ac5ed916ffac93f0 Signed-off-by: Thomas Kosiewski <tk@coder.com>

…hedule On SQLite and S3-WAL a REST ref write records its forward intent only after the authority CAS, in events.RefStore's sync poke. Serve dying between the two, or a failed RequestPush, leaves an accepted write that no run forwards (verify-lane-schedule finding 1, open for REST writes). TestServeRestIntentBeforeCAS writes through buildServeEventing's decorator and reads the row and the ref's push hold at the instant the CAS commits. It fails on this commit: push_pending is 0 and nothing holds the ref, and a write whose metadata store fails is accepted. Change-Id: Ife5779cd13adfe994641b9be9315f974755adaaa Signed-off-by: Thomas Kosiewski <tk@coder.com>

…'s CAS On SQLite and S3-WAL a REST ref write recorded its forward intent only in events.RefStore's sync poke after the authority CAS. Serve dying between the two, or a failed RequestPush, left an accepted write that no run forwards (verify-lane-schedule finding 1, which #678 fixed for git pushes only). Serve's decorator now sets BeforeCAS on the local schedule: restIntentBeforeCAS reads the upstream row and, for a forward_on_push repository and a caller that may forward, takes the ref's push hold in the cache (githttp.HoldRef), then runs RequestPush. The hold ends when the inner CAS returns. A run that claims the intent waits in its publication wait until then, so it cannot snapshot the ref before the CAS. A failed read, hold or intent write refuses the write. BeforeCAS now gets the ref name and returns a release, which RefStore calls right after the inner CAS. The hook's BeforeCAS returns a no-op release. The model gets the REST writer's hold and pre-CAS mark (RReserve, RMark, RCas, RRelease, RPoke, RAbort, RCrashRest) behind the new code fact RestMarkBeforeCas. RCrashRest is now always allowed, so the WritesComplete assumption and its probe NoCompletionAssumption.cfg go. The ADR-50 amendment covers REST writes and states that a failed read refuses every non-ephemeral update. Change-Id: Ic220b1210a4e600381168d8b83ee731352436eec Signed-off-by: Thomas Kosiewski <tk@coder.com>

…vent A local push run whose loopback fetch the server refuses with 423 (a push holds a ref the cache has not caught up with) restores its request, but it already emitted repo.push_upstream.started. No terminal event follows for that run, so a webhook consumer sees a run that never ends. The C-harness subtest "stale held snapshot restores force and attempt" now expects no event from the refused run. It fails on this commit with a third, unterminated started event. Change-Id: I1a21cbf259906aa4af14e86601de03ea71e189c9 Signed-off-by: Thomas Kosiewski <tk@coder.com>

…s its snapshot RunPush emitted repo.push_upstream.started before transferPush. A local run whose fetch is refused for a stale held ref then restored its request with no terminal event. transferPush now calls back once the loopback fetch succeeded, and RunPush emits started there, or after transferPush returns for every other outcome. The refused local run emits nothing, like a failed publication wait. Event order and payloads stay the same for all other runs. docs/reference/http-api.md: such a run emits no event, and REST writes can hold a ref too. Change-Id: Ic10fa05473c288c696e48e81dcef9b8a28fc403f Signed-off-by: Thomas Kosiewski <tk@coder.com>

A verifier removed transferPush's fetched() call, so the started event went out only when the run ended, after the upstream push, and every test stayed green. TestPushRunStartedPrecedesUpstreamPush runs a River push (PostgreSQL's path) and records whether the upstream already has main when the started event goes out. Without the fetched() call it fails: "repo.push_upstream.started went out after the upstream push". Change-Id: Ibeb8ccb5e836f75e2ac9ca73e60f97c9c373f81b Signed-off-by: Thomas Kosiewski <tk@coder.com>

…e waits RunPush's comment and the HTTP API reference say that, on every backend, repo.push_upstream.started goes out once the loopback fetch has its snapshot, before the upstream push. On PostgreSQL it went out before the fetch until this branch. The reference gets a "Ref writes that forward" paragraph: on SQLite and S3WAL such a write holds the ref and records the forward request before it changes the ref, waits up to 10 seconds for a push or write that holds the same ref, then fails with 503 and Retry-After: 1, and a write that loses its compare-and-swap (409) still costs one forward run. The ADR-50 amendment names the meta.ErrNotFound exception (no upstream link) to the refusal. Change-Id: I4cff9eb00575fdaa5e2a0c3ca112b81eac3f8490 Signed-off-by: Thomas Kosiewski <tk@coder.com>

check.sh --record: LaneSchedule.cfg PASS (3535660 distinct states) OtherWriters.cfg PASS (56705945 distinct states) Liveness.cfg PASS (348735 distinct states) NoLeaseAssumption.cfg fails as expected RESULT: PASS Change-Id: I7487027b2357aac274ddfa718ce7462c6bb8f363 Signed-off-by: Thomas Kosiewski <tk@coder.com>

mutate.sh's MarkBeforeCas mutation no longer matched RefStore.CASRefGen after the deferred hold release, so it reported an error instead of breaking the fact. It now moves runBeforeCAS after the inner CAS and keeps a deferred and a direct release(), so only MarkBeforeCas turns FALSE. Change-Id: Ia02ac49ad0256aa4de4a9ae9dd00d1366a2cbc3c Signed-off-by: Thomas Kosiewski <tk@coder.com>

ThomasK33 · 2026-11-07T03:54:43Z

@codex review

ThomasK33 · 2026-11-07T03:54:49Z

@codex security review

chatgpt-codex-connector · 2026-11-07T03:58:08Z

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: 2e7cfc8d93

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-11-07T03:58:39Z

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 2e7cfc8d93

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

You comment "@codex security review"
A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

TestRestIntentBeforeCASPaths decided "not held" with a 1 s publication wait, and TestServeRestIntentBeforeCAS with 200 ms at the CAS. The wait checks its context before its first poll, so a host that stalls the test past the bound reads a ref that nothing holds as held: a scratch check that sleeps 1.1 s between the deadline and the wait got "context deadline exceeded" for an unheld ref. holdCheckBound keeps the short bound where a hold is expected (the wait times out by design) and uses the tests' other 5 s bound where none is, which returns at the first poll. Change-Id: I7cc7edd7bda5bb9b3919fe4b710d072a7f16cf20 Signed-off-by: Thomas Kosiewski <tk@coder.com>

…k-open failure A verifier dropped three guards of the REST hold and every test stayed green: - HoldRef's refresher: a hold then lapses after about one and a half TTLs while the write is still in flight. TestHoldRefRefreshesPastItsTTL holds a ref under a 2 s TTL and checks that the publication wait still blocks 4.5 s later. - events.RefStore's assertion that HeldBeforeCAS returns a release. TestHeldBeforeCASNilReleaseAsserts expects the assertion's panic, and no inner CAS. - A reservation lock that cannot be opened marked as ErrRefHeld (a 503 for a failure a retry does not fix). TestReservePushHolds_LockOpenFailureIsNotRefHeld puts a directory at the lock path and expects a plain error. Each guard's removal now turns its test red. Change-Id: Ica6a17fb0db4136fa1e8e8c8fdc6b58dcdd91dce Signed-off-by: Thomas Kosiewski <tk@coder.com>

…d timing The "Ref writes that forward" paragraph and the 5xx row now say that a directory/file relative of the ref (refs/heads/a and refs/heads/a/b) also makes a forwarding REST write wait and then answer 503, and that a failed upstream-link read, another hold failure or a failed write of the forward request answers a plain 500 with the ref unchanged. The reference and webhooks.md say when repo.push_upstream.started goes out, and that a run that fails before or at its loopback fetch sends it right before its failed event. Change-Id: Ia427fb422d85b8db85f271854716e5be10bbc7ba Signed-off-by: Thomas Kosiewski <tk@coder.com>

Change-Id: I74a2522592401c528f63519916f492fcaa8ffdfc Signed-off-by: Thomas Kosiewski <tk@coder.com>

check.sh --record: LaneSchedule.cfg PASS (3535660 distinct states) OtherWriters.cfg PASS (56705945 distinct states) Liveness.cfg PASS (348735 distinct states) NoLeaseAssumption.cfg fails as expected RESULT: PASS Change-Id: I684e7e45d28a1b9e5579bb46fd9bfdc200d7e8b7 Signed-off-by: Thomas Kosiewski <tk@coder.com>

ThomasK33 · 2026-11-07T08:33:29Z

@codex review

ThomasK33 · 2026-11-07T08:33:36Z

@codex security review

chatgpt-codex-connector · 2026-11-07T08:36:14Z

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 916473e29c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-11-07T08:43:24Z

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 916473e29c

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

You comment "@codex security review"
A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

http-api.md put the plain 500 for a failed upstream-link read under "Ref writes that forward", but the write reads the link before it knows whether the repository forwards: on SQLite and S3WAL the read's failure refuses a ref write on any repository for a token that passes the forward gates. webhooks.md said that a push run that fails before or at its loopback fetch sends started right before failed. A run that a write in progress holds back sends no event, and a cancelled run or one whose claim another worker took over ends with no terminal event. Name those exceptions and point to "Events and cancellation". Change-Id: Iad1d6e0ae3c3bead60354541585e879eec5798fc Signed-off-by: Thomas Kosiewski <tk@coder.com>

ThomasK33 · 2026-11-07T09:38:25Z

@codex review

ThomasK33 · 2026-11-07T09:38:32Z

@codex security review

chatgpt-codex-connector · 2026-11-07T09:41:40Z

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: c89a9f93b5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-11-07T09:43:29Z

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: c89a9f93b5

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

You comment "@codex security review"
A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

ThomasK33 added 30 commits November 5, 2026 08:33

docs(skills): explain probe UNEXPECTED lines, FAIL over STALE, and th…

90dea59

…e model variables Change-Id: Iafc903e2e9a284d33bd0cb076b93f5c621837eb2 Signed-off-by: Thomas Kosiewski <tk@coder.com>

Merge verify-lane-schedule (PR #675 head 7201eb1) into fix-lane-forwa…

24b2f49

…rd-git Change-Id: I967eee2110d0e994f0185e16b981f9e4f744421a Signed-off-by: Thomas Kosiewski <tk@coder.com>

docs(skills): update the verify-lane-schedule sample state counts

849b9a2

Change-Id: Ife43711474301d0a26ca7ea352ee7aeaaff4fbf9 Signed-off-by: Thomas Kosiewski <tk@coder.com>

docs(skills): record verify-lane-schedule at the fixed hook order and…

5a59261

… strict snapshot Change-Id: I0b5aadcbb4631b4a842188e0144cb9dfc6ed7bfd Signed-off-by: Thomas Kosiewski <tk@coder.com>

Merge remote-tracking branch 'origin/main' into fix-lane-forward-git

356645f

Change-Id: I5cff827bead0971223101323611f67c97909a392 Signed-off-by: Thomas Kosiewski <tk@coder.com>

docs(skills): record verify-lane-schedule after merging main

bf583ae

Change-Id: Ife03e411e8a9cbeb738af3512ce56535a56baba8 Signed-off-by: Thomas Kosiewski <tk@coder.com>

Merge remote-tracking branch 'origin/main' into fix-lane-forward-git

5268405

Change-Id: I7770390f42967ab4702133ec5ffac4b2e7acecf4 Signed-off-by: Thomas Kosiewski <tk@coder.com>

docs(skills): record verify-lane-schedule at the pull-poke fix

37f65bc

Change-Id: I3e3a99160caa83cd7c6aec758d996402268ba6c5 Signed-off-by: Thomas Kosiewski <tk@coder.com>

docs(skills): record verify-lane-schedule at the open-failure refusal

e083a03

Change-Id: Ic043387eaaca6c4cbc7e1390971b25cf604bb9ad Signed-off-by: Thomas Kosiewski <tk@coder.com>

Merge remote-tracking branch 'origin/main' into fix-lane-forward-git

9327c6e

Change-Id: Ic6f8b8f12f24460b5a5fda0bb29e1c8cabc6c1a5 Signed-off-by: Thomas Kosiewski <tk@coder.com>

docs(skills): record verify-lane-schedule after merging the skill-acc…

902ea1b

…uracy changes Change-Id: I20aac425fbcb188297405276954e6cce7843342f Signed-off-by: Thomas Kosiewski <tk@coder.com>

Merge remote-tracking branch 'origin/main' into fix-lane-forward-git

a3223fb

Change-Id: I1a453ad0ba3f3e7c31004db6239837bd4b191f66 Signed-off-by: Thomas Kosiewski <tk@coder.com>

docs(skills): record verify-lane-schedule after merging #679 and #689

745c7fb

Change-Id: Ib57fdfb972853f6f778efdd69495112629eb8181 Signed-off-by: Thomas Kosiewski <tk@coder.com>

Merge remote-tracking branch 'origin/main' into fix-lane-forward-git

3e02f92

Change-Id: Icca467bf7789d43b33f58983ac5ed916ffac93f0 Signed-off-by: Thomas Kosiewski <tk@coder.com>

ThomasK33 added 4 commits November 6, 2026 23:42

ThomasK33 added 5 commits November 7, 2026 06:28

Merge remote-tracking branch 'origin/main' into fix-rest-forward-intent

af60102

Change-Id: I74a2522592401c528f63519916f492fcaa8ffdfc Signed-off-by: Thomas Kosiewski <tk@coder.com>

ThomasK33 added this pull request to the merge queue Nov 7, 2026

Merged via the queue into main with commit dc0dcfe Nov 7, 2026
12 checks passed

ThomasK33 deleted the fix-rest-forward-intent branch November 7, 2026 11:27

github-actions Bot mentioned this pull request Nov 7, 2026

chore(main): release 0.20.0 #653

Merged

This was referenced Nov 7, 2026

fix(api): clip long client values in git error names and log lines #757

Merged

fix(githttp): show the pusher only refusals from a hook #755

Merged

fix(sync): refuse stale held snapshots on PostgreSQL and restore River push runs #762

Merged

Navigation Menu

Repository navigation

fix(sync): hold the ref and record forward intent before a REST write's CAS - #737

fix(sync): hold the ref and record forward intent before a REST write's CAS#737
ThomasK33 merged 66 commits into
mainfrom
fix-rest-forward-intent

ThomasK33 commented Nov 6, 2026 •

edited

Loading

Uh oh!

ThomasK33 commented Nov 7, 2026

Uh oh!

ThomasK33 commented Nov 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026

Uh oh!

ThomasK33 commented Nov 7, 2026

Uh oh!

ThomasK33 commented Nov 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026

Uh oh!

ThomasK33 commented Nov 7, 2026

Uh oh!

ThomasK33 commented Nov 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ThomasK33 commented Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changes

Behaviour a user can see

Known doc gaps, fixed in the E PR

Verification

Size

Notes for reviewers

Uh oh!

ThomasK33 commented Nov 7, 2026

Uh oh!

ThomasK33 commented Nov 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026

🛡️ Codex Security Review

Uh oh!

ThomasK33 commented Nov 7, 2026

Uh oh!

ThomasK33 commented Nov 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026

🛡️ Codex Security Review

Uh oh!

ThomasK33 commented Nov 7, 2026

Uh oh!

ThomasK33 commented Nov 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026

🛡️ Codex Security Review

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ThomasK33 commented Nov 6, 2026 •

edited

Loading