Repository navigation
fix(serve): refuse an intent-recording ref update when the hook cannot wire its eventing - #724
Conversation
…its forward intent When the hook's meta.Open (or its secrets codec) fails on PostgreSQL, wireHookEventing falls back to the eventless store. That store records no forward intent in the CAS, so a forward_on_push push lands with no intent and no events. Change-Id: If642ce461c7bea3b58d07107f66fdc0e5b74937f Signed-off-by: Thomas Kosiewski <tk@coder.com>
…its eventing On PostgreSQL the ref CAS records a forward_on_push repository's forward intent, and a failed meta.Open does not prove the CAS fails too: the database can be back by then. The hook's eventless fallback then accepted the push with no intent and no events. A hook whose secrets codec, meta.Open or River insert-only client fails now refuses the ref update before its CAS on PostgreSQL, and the client can push again. SQLite and S3-WAL keep failing open. ADR-33 and ADR-44 get amendments, and the serve README states the new rule. Change-Id: I8dbf5b106e0df5064dfb640b8b3e38dd1eb54555 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…fusal on PostgreSQL Change-Id: I4f88970108351e63faeccb6aa8b4e77fe50cd8cb Signed-off-by: Thomas Kosiewski <tk@coder.com>
…back Change-Id: Ife7e65a1328120e16be1cb57582ecca7703191a4 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…back every update that would record forward intent (intentlessRefStore), and let ephemeral updates and a sync run's loopback push fail open. The disabled helper now applies the same rule on every backend for every failed wiring step (secrets codec, meta.Open, River client), instead of refusing all updates on PostgreSQL only. The PostgreSQL fail-open test and its helper go; TestHookEventingRefusesIntentWithoutEventing covers both backends, ephemeral updates and the loopback push. Change-Id: I2c6060d2056019d735f45eeb85eaa6e5c1685691 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I1fd9c385e65039187215901f1c51b84328255e40 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d35594ad02
ℹ️ 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".
…the hook refuses intent The refusing store wrapped the hook's raw store, so a fail-open ephemeral update lost ref.IdempotentCAS's recovery of an ambiguous CAS outcome. It now wraps IdempotentCAS, as the eventless default does. A caller whose token may not forward records no intent even with eventing, so its updates now fail open instead of being refused. #678's separate local-schedule refusal for a failed meta.Open goes through the same disabled helper, so both backends follow one rule. Change-Id: If3db34488697ffcf59b012c8338dd712bd89e8de Signed-off-by: Thomas Kosiewski <tk@coder.com>
…tail for the pusher - The hook's local-schedule BeforeCAS no longer runs for a caller whose token may not forward: it records no intent, so a failed read of the upstream link cannot refuse that caller's update. - The refusal and the hook's log name only the failed step. git relays the hook's stderr and its error to the pusher, and a store error names the database host, user and name, or the S3 endpoint. The same holds for BeforeCAS's refusal and the format-check warning. - newHookRiver lets a test reach the River client's failure. - Tests: every shape (persistent, secrets failure, ephemeral namespace and key, sync loopback, write-only token) on PostgreSQL and SQLite, through CASRef and CASRefGen, over a store that commits and then reports an EOF; a canary database URL that the pusher's output must not name; the River failure and its cleanup on a real PostgreSQL. - ADR-33, ADR-44, ADR-50, the serve README and the WrapHookRefStore doc say what the hook does now. Change-Id: I49051297eed7ed15e0890d5184739affba9242b1 Signed-off-by: Thomas Kosiewski <tk@coder.com>
A down fake bucket answers 503, whose error names no host, so a warning that logged the read error passed. An unresolvable endpoint's error names the host. Change-Id: I64b02ada00c005d64f49a77a1a6a8fcfa0ac6dce Signed-off-by: Thomas Kosiewski <tk@coder.com>
…error where it is wired The lane-schedule fact MarkBeforeCas reads the local-schedule branch's beforeCAS = enqueueSyncOrForward(...) assignment. quietBeforeCAS now hides the store error where the options are built. Change-Id: I2c35fc57702e7aa476255c4d32cf72be47429293 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…k refusal rule Change-Id: Id7a2d27b8643eeb744d4aa354ad4282fcb0ea540 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…back Change-Id: I61315965de08eee34ccd9e7432dcec99586d7e9d Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…back Change-Id: I0561bf38eaf94bd9e0de8cdcd25b61758e582ead Signed-off-by: Thomas Kosiewski <tk@coder.com>
…e, and say eventing is unavailable - The table now creates a branch and a tag, updates and deletes a ref, on PostgreSQL and SQLite, so a rule that accepted any of them fails. - The refusals' "push again later" suffix and BeforeCAS's log line are pinned. - The SQLite rows open a canary-named path, with a control that meta.Open's error names it. - The wiring's log line no longer says the hook refuses updates, because it still accepts the ones that record no intent. Change-Id: I8f6078e5b2901562290244db9779a17bc9798af8 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…'s connection fails ADR-33's amendment and the disabled helper's comment said serve logs its own store errors. serve logs a store error only when its own connection fails, so when only the hook's connection fails, neither the pusher nor the operator sees the cause. Change-Id: Ia9eed12d6266fa033807d6f61ad655ebe5eab1da Signed-off-by: Thomas Kosiewski <tk@coder.com>
…comment fix Change-Id: I6ac584aa23065a4dfb5f7b686dce9951e3aa854d Signed-off-by: Thomas Kosiewski <tk@coder.com>
…back Change-Id: I6aff803447c09f1fe94348567cdb48815e9b6156 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…nd the REST gate order check.sh --record: 12 facts TRUE, LaneSchedule.cfg 3535660, OtherWriters.cfg 56705945 and Liveness.cfg 348735 distinct states pass, NoLeaseAssumption.cfg fails as expected. Change-Id: I379a587269e49a62c4cfad785535848fd5e83a5f Signed-off-by: Thomas Kosiewski <tk@coder.com>
Why
Every accepted write to a
forward_on_pushrepository must record its forward intent with the ref update. On PostgreSQL the ref CAS records it (#679). On the local schedule (SQLite and S3-WAL) the hook records it before the CAS (#678). When a git hook could not set up its eventing (its secrets codec,meta.Open, or its River insert-only client failed), it fell back to the eventless ref store, which records no intent. The push then landed with no forward intent and no events. A failedmeta.Opendoes not prove that the CAS fails too: the database can be back a moment later, and the CAS then commits the push alone. #678 closed this for a local-schedule store that does not open. The secrets-codec path still had the gap on every backend, and every path had it on PostgreSQL.This PR is the one place for the hook's refusal rule on both backends.
Fix
A hook whose eventing wiring fails refuses every update that would record forward intent, before its CAS, on every backend. The client sees the push fail and can push again.
+ephemeralrepository key or the ephemeral ref namespace), a sync run's own loopback push (OPENSTORAGE_SYNC_CLAIM; on main a refused loopback push fails the inbound run withFailPush), and a caller whose token may not forward (hookCallerDeniedForward: nogit:read, or ref-policy ops).wireHookEventingends every failed wiring stage through onedisabledhelper. It closes what the wiring opened, and it wraps the ref store in fix(sync): record git forward intent before the CAS and refuse stale held snapshots #678'sintentlessRefStore. fix(sync): record git forward intent before the CAS and refuse stale held snapshots #678's separate refusal for a local-schedule store that does not open now goes through the same helper.intentlessRefStorepasses the updates it accepts toref.IdempotentCAS, as the eventless default does, so a CAS that commits and then loses its reply still wins. It overridesCASRefandCASRefGenalike.refusing the ref update: hook eventing unavailable (open meta store); push again later). The hook logshook eventing unavailable: open meta store; refusing only updates that would record forward intent, with no store error. The same holds for fix(sync): record git forward intent before the CAS and refuse stale held snapshots #678'sBeforeCASrefusal (hook could not record the forward intent; push again later) and the format-check warning.push event lostandsync poke lostlog lines. A follow-up PR gives those paths a fixed message and gives the cause back to the operator through a channel that only serve reads.BeforeCASno longer runs for a caller whose token may not forward. It records nothing for such a caller, so a failed read of the upstream link no longer refuses that caller's update.WrapHookRefStoredoc comment state the rule.Tests
TestHookEventingRefusesIntentWithoutEventing(cmd/openstorage) runs each shape on PostgreSQL (an unresolvable canary host, someta.Openfails) and on SQLite (a canary-named database path under a file). Each shape creates a ref, updates an existing ref and deletes one, throughCASRefandCASRefGen, over a ref store that commits each create or update and then reports an EOF:The main column comes from a run of a branch creation. On main, both the eventless default and #678's refusal look only at the repository key and the ref name, so a tag, an update and a delete take the same path (I read the code and did not run them on main).
The test also checks that the hook's log and errors name no canary part: the PostgreSQL URL's user, password, host and database, or the SQLite path (match counts of 0). A control first checks that
meta.Open's own error names each part except the password, so a count of 0 proves something.TestHookEventingRefusesUnreadableLocalScheduleadds a write-only token (accepted) and an unresolvable S3 endpoint, whose host must not appear in the hook's output. It pinsBeforeCAS's refusal,push again latersuffix included, and its log linehook refuses ref updates: record forward intent.TestHookEventingRiverFailureRefusesAndCleansUp(real PostgreSQL) replaces the River constructor with a failing one (newHookRiver): the update is refused, and the metadata store's connection is closed (pg_stat_activity).Twenty-two source mutations each turn at least one of these tests red: no refusal; a refusal of the loopback push, of every update, of forward-denied callers, or on PostgreSQL only; the old eventless return for the secrets codec,
meta.Openor the River client; nocleanup(); the refusing store over the raw ref store; the store error in the refusal or in the log line;BeforeCASfor forward-denied callers;BeforeCAS's raw error;CASRefpassing intent updates through; the store error in the format-check warning; a refusing store that accepts deletes, tags or updates of existing refs; nopush again latersuffix on either refusal; and noBeforeCASlog line.The
verify-lane-schedulemodel does not describe the hook's failure handling.check.shpasses, andcheck.sh --recordrecorded the new sync commit, becausewireHookEventingchanged.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh