fix(scheduling): close seven ledger and authoring follow-ups from the MVP review - #1255
Merged
Merged
Conversation
…authoring An offering's requiresCapabilities and prerequisites are satisfied from the admission request, and the request edge refuses either list past MAXIMUM_COLLECTION_ENTRIES. The authoring check bounded reminders, services, offerings, holiday sets, openings, hooks, and windows, but not these two, so an offering declaring more requirements than a request may carry passed authoring and startup, published, and then refused every admissible request as request.invalid: a permanently unbookable offering with no authoring signal. Closes #1253 Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…ty I/O availability and explain each checked the offering selector for emptiness alone, while every hold and appointment route validated its identifier against the runtime's own bounded grammar. A caller could spend a round trip sending a URL the runtime would refuse, and receive a transport or intermediary failure rather than the caller-side InvalidRequest the client promises for that class of input. Both now share one validate_offering, so explain no longer carries its own copy of the check and there is a single definition of a valid selector. The selector travels as a query parameter, not a path segment, so no route detour was reachable through it. Closes #1254 Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
close_claim, which release and cancellation both go through, sets revision=$4 + 1 and its callers write the history event at that new value. The expiry sweeper ran its own update and omitted revision from the SET list, so the RETURNING value was the revision the row already carried and the expired event landed at the same revision as the held event that opened the claim. Nothing caught it at write time: scheduling_history has no unique constraint on (claim_id, revision). Capacity was never affected, because the snapshot query discounts expired holds by hold_expires_at without waiting for the sweep. Closes #1252 Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
The duplicate-active guard counted every active booking regardless of its time, and no transition the party can reach ever closes one: there is no completion route, and cancellation is refused at the offering's cutoff. A party who kept an appointment was therefore refused that offering forever. Bound the guard by time in both places that answer it, the offering-wide read and the snapshot predicate, so the key means one live booking per party per offering. A booking still consumes capacity for overlap after it has passed; only its hold on the key is released. Closes #1251 Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…he claim Release and cancel resolved the offering from the current policy before anything reached the stored receipt, so once publication retired an offering whose claims were all closed, the retry each receipt exists to serve answered service.unavailable for the rest of the retention window. A caller cannot tell that from an outage and keeps retrying. Read the offering from the policy revision the claim names when the current policy no longer carries it. The receipt is still reached through an authorized request: the grant is matched against the offering as it stood when the claim was committed, rather than the check being skipped to reach the replay. Closes #1250 Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…e database The combined validation of a policy against the published window records it governs ran only in the authoring tooling, against the files on an operator's disk. Neither database write repeated it, so an operator who edited the policy and restarted could deploy a pool that staffs a window and also backs an exact-time offering. The two modes lock different supply anchors, so their capacity transactions never serialize and the ledger cannot observe that it sold the same staffing twice. Both writes now re-run the same check under the locks they already hold: publication loads the deployed window records, replacement loads the retained document of the deployed policy revision. The authoring check stays the single implementation; the writes narrow its findings to the contradictions, excluding the two reasons that name an absence ordinary operator sequencing depends on. SCHEDULING-DEF-07 is promoted to the enforced SCHEDULING-SEC-26. Closes #1249 Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
A resource pool and a published window anchor their capacity transactions on the same row of scheduling_supply, keyed by the identifier, and the two namespaces are authored separately. A collision aborted the records write on the primary key and was silently skipped by the policy write, which left a pool anchored on a row marked window that the next records replacement deleted, refusing every commitment against that pool until the policy was republished. Refuse the collision by name from both sides of authoring, and give the two anchor writes one insert that returns the kind the row settled on so a standing anchor of the other kind is named rather than swallowed. lock_supply now names the identifiers it could not find, which is what SCHEDULING-SEC-02 already promised the operator's diagnostics would say. Closes #1242 Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2466ed21c4
ℹ️ 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 selector bound added for #1254 reused the route-identifier check, which admits the alphabet a minted document id travels in: uppercase letters, underscores, dots, and up to 128 bytes. The runtime validates the offering query parameter with the policy grammar instead, which admits lowercase letters, digits and hyphens up to 64 bytes and must begin with a letter. So selectors such as Registry_Update or a 65-byte lowercase name still passed both availability entry points, spent the round trip, and came back as a server-side refusal rather than the caller-side InvalidRequest the bound exists to give. Validate the selector with the runtime's own valid_identifier. Route identifiers keep the wider alphabet: they carry ids the runtime minted, not identifiers a policy author wrote. Refs #1254 Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…dentifier The fix for #1250 read the policy revision a claim names only when the current policy no longer carried its offering. Retirement is not the only change a closed claim outlives: once nothing on an offering is live, publication may also keep the identifier and sell it under a different service or location, because only a live claim pins those. A release or cancellation retry was then authorized against terms its claim never had. The caller holding the grant that committed the claim was refused the receipt that grant had already earned, and a grant over whatever the identifier sells today could reach a receipt written under something else. Resolve the offering from the revision the claim names unless the claim still pins it, which is the sentence publication itself selects on when it refuses to move an offering: an active booking, or a hold not yet past its expiry. For such a claim the live policy and the retained revision carry the same service, location, mode and supply, so the live one still answers and nothing about a standing claim changes. Refs #1250 Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c34c21e4b0
ℹ️ 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".
A booking stays active once its time has passed, so the active claims carrying one party's duplicate key accumulate for the life of the deployment. The guard filters on the key, the offering, and the booking's end, but the schema version 1 index carried the key alone and left the other two to a recheck over that accumulating set. Schema version 7 replaces it with an index over all three columns under the same partial predicate. Refs #1251 Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51de118f76
ℹ️ 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".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Seven follow-ups from the Scheduling MVP review, one commit per ticket, in
the order they build on each other. Every one of them was filed rather than
fixed during that review because none of them was claimed by an
enforcedrow of the security invariant matrix; each is closed here with the test that
holds it and, where it adds an invariant, the matrix row that now claims it.
The changes
#1253, offering capability and prerequisite lists. An offering's
requiresCapabilitiesandprerequisitesare satisfied from the admissionrequest, and the request edge refuses either list past
MAXIMUM_COLLECTION_ENTRIES. The authoring check bounded every othercollection but not these two, so an offering declaring more requirements than
a request may carry passed authoring and startup, published, and then refused
every admissible request as
request.invalid: permanently unbookable, with noauthoring signal. The authoring check now bounds both at the edge's own limit.
#1254, the client's offering selector.
availabilityandexplainchecked the selector for emptiness alone, while every hold and appointment
route validated its identifier against the runtime's bounded grammar. A caller
could spend a round trip on a selector the runtime refuses and receive a
transport failure rather than the caller-side
InvalidRequestthe clientpromises for that class of input. Both now share one
validate_offering. Theselector travels as a query parameter, not a path segment, so no route detour
was reachable through it.
#1252, hold expiry and the claim revision.
close_claim, which releaseand cancellation both go through, advances the revision, and its callers write
the history event at the new value. The expiry sweeper ran its own update and
omitted the revision, so the expired event landed at the same revision as the
held event that opened the claim, and nothing caught it:
scheduling_historyhas no unique constraint on
(claim_id, revision). Capacity was neveraffected, because the snapshot query discounts expired holds by
hold_expires_atwithout waiting for the sweep.#1251, duplicate keys. The duplicate-active guard counted every active
booking regardless of its time, and no transition the party can reach closes
one: there is no completion route, and cancellation is refused at the
offering's cutoff. A party who kept an appointment was refused that offering
forever. Both places that answer the guard are now bounded by time, so the key
means one live booking per party per offering. A booking still consumes
capacity for overlap after it has passed; only its hold on the key is released.
#1250, close retries. Release and cancel resolved the offering from the
current policy before anything reached the stored receipt, so once publication
retired an offering whose claims were all closed, the retry each receipt
exists to serve answered
service.unavailablefor the rest of the retentionwindow, which a caller cannot tell from an outage. The offering is now read
from the policy revision the claim names when the current policy no longer
carries it. The receipt is still reached through an authorized request: the
grant is matched against the offering as it stood when the claim was
committed, rather than the check being skipped to reach the replay.
#1249, the combined invariants at the database.
check_window_recordsisthe canonical validation of a policy against the window records it governs,
and it refuses the consequential case: a pool that staffs a published window
while also backing an exact-time offering, whose two modes anchor on different
supply rows and therefore never serialize. It ran only in
schedulingctl,against the files on an operator's disk. Both database writes now re-run it
under the locks they already hold, excluding the two findings that name an
absence rather than a contradiction so that ordinary operator sequencing in
either order stays possible. New matrix row SCHEDULING-SEC-26; the deferral
SCHEDULING-DEF-07 that recorded the gap is removed.
#1242, supply identifiers. A resource pool and a published window anchor
their capacity transactions on one row of
scheduling_supplykeyed by theidentifier, and the two namespaces are authored separately. A collision
aborted the records write on the primary key and was silently skipped by the
policy write, leaving a pool anchored on a row marked
windowthat the nextrecords replacement deleted, which refused every commitment against that pool
until the policy was republished. Authoring refuses the collision by name from
both sides, and the two anchor writes share one insert that returns the kind
the row settled on.
lock_supplynames the identifiers it could not findrather than reporting a corrupt ledger, which is what SCHEDULING-SEC-02
already promised the operator's diagnostics would say. New matrix row
SCHEDULING-SEC-27.
The last two are sequenced: #1249 puts the canonical combined check behind
both database writes, and #1242's window-side collision rule reaches those
writes through it.
Review fixes
Three commits answer the automated review, each correcting a commit above
rather than rewriting it.
fix(scheduling-client): hold the offering selector to the policy grammarcorrects #1254. The bound it added reused the route-identifier check, which
admits the alphabet a minted document id travels in: uppercase letters,
underscores, dots, 128 bytes. The runtime validates the offering query
parameter with the policy grammar instead, so
Registry_Updateor a 65-bytelowercase name still spent the round trip the bound exists to save. The
selector now uses
valid_identifier, the same predicate the edge applies.Route identifiers keep the wider alphabet, because they carry ids the runtime
minted rather than identifiers a policy author wrote.
fix(scheduling): read a close retry's terms from the claim, not the identifiercorrects #1250. That commit read the retained revision only whenthe current policy no longer carried the offering, but retirement is not the
only change a closed claim outlives: publication pins an offering's service,
location, mode and supply only while a claim on it is live, so the identifier
may also stay and be sold under different terms. The retry was then authorized
against terms its claim never had, in both directions: the caller holding the
grant that committed the claim lost the receipt it had earned, and a grant over
whatever the identifier sells today could reach a receipt written under
something else. The offering now comes from the revision the claim names unless
the claim still pins it, which is the sentence
apply_policyitself selects on(an active booking, or a hold not yet past its expiry) and which also covers
the expired hold a state check alone would miss. New Postgres test, traced
under SCHEDULING-SEC-09, whose promise to replay the stored first answer for
the same key and body is what the gap was breaking.
fix(scheduling): index the whole predicate the duplicate guard filterscorrects #1251. The bound that commit put on the duplicate guard, counting only
a booking whose time has not passed, filters on three columns: the key, the
offering, and the booking's end. The version 1 index carried the key alone, and
nothing in the ledger closes an elapsed booking (no completion transition,
appointment retention deferred), so the active claims behind one party's key
accumulate for the life of the deployment and the guard rechecked all of them.
Schema version 7 indexes
(duplicate_key, offering, occupied_end)under thesame partial predicate and drops the narrower index.
0001_scheduling.sqlstays as written, so a migrated deployment and a fresh one converge on the same
schema. New Postgres test asserting the index the guard needs.
The review's other finding, a missing DCO sign-off, names a commit SHA that
resolves in neither this repository nor the GitHub API; all ten commits carry
the trailer.
Not in this PR
#1240 and #1241 stay open. #1240 wires Scheduling binaries and installers
through the release pipeline, which touches release provenance and wants its
own reviewed change. #1241 adds the client to the three unified facades: it is
purely additive, spans the Node.js and Python packaging, and belongs with a
facade change rather than here.
Verification
cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --locked --workspace --no-fail-fast(the failures in that runare the
registry-bregtargets that requireBREG_TEST_DATABASE_URL, plusone
registry-platform-httputildeadline test that passes on its own underno load; neither depends on any crate this branch touches)
products/scheduling/scripts/check-checkpoint.shproducts/scheduling/scripts/check-contracts.shpython3 products/identifiers/scripts/generate.py --check-referencescargo test --locked -p registry-scheduling --features postgres-test --test postgres_commitmentsagainst a disposable PostgreSQL 17 database