GV-14: Add BillingAccount storage admission and external-contribution limits - #700
GV-14: Add BillingAccount storage admission and external-contribution limits#700ScottArbeit wants to merge 4 commits into
Conversation
CI failure routed to fresh validation workerGitHub Actions The build step succeeded. The failure is in
Representative error: I assigned fresh validation-fix worker Avicenna ( |
![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: 1fb153740a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Grace/src/Grace.Actors/PromotionSet.Actor.fs
Lines 98 to 100 in 429bf6a
Allow same-correlation apply transfer retries
If TransferAcceptedContentOwnershipForSteps fails after the Applied event has been persisted, retrying the same apply request with the same correlation id is rejected here before the new succeeded-state transfer retry path can run. That leaves recovery dependent on callers inventing a new correlation id, even though this change explicitly relies on re-entering Apply after success to finish or replay the ownership transfer.
ℹ️ 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".
Second-cycle stabilization pass: content ownership ledger durability and replay closurePR #700 received a fresh current-head Codex review on Because this PR touches actor behavior, storage, persisted state, retries, idempotency, and side-effect ordering, this is the second substantive high-risk review cycle. I am starting the stabilization pass before assigning another worker. Review timeline
Missing invariant familyThe ledger needs durability/replay closure across every place where ownership state crosses an actor or storage boundary:
Proof obligation for the stabilization workerThe next worker must keep scope to #653 and prove:
No maintainer product decision is required for this pass because the findings refine #653's already-accepted ledger, transfer, persisted-state, retry, and retention responsibilities. If the worker discovers that a new public contract, durable domain concept beyond |
Second-cycle stabilization fix pushedThe PR #700 second-cycle stabilization fix was pushed as Resolved current-head review threads from Codex review
The top-level review-body finding, Validation evidence from Halley:
The earlier current-head Validate run |
CI failure routed to fresh validation workerGitHub Actions This is not the hosted-runner acquisition failure seen on run
Representative error: I assigned fresh validation-fix worker Newton ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa89600388
ℹ️ 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".
| (metadata: EventMetadata) | ||
| : Result<ContentOwnershipLedgerDecision, GraceError> | ||
| = | ||
| let command = commandWithDurableAcceptedOwner events command |
There was a problem hiding this comment.
Fresh evidence beyond the earlier Reference metadata replay issue: this rewrites contributor-owned add commands before tryFindAppliedOperation checks the operation id. After an accepted transfer exists, retrying the original reference:<id>:... add for a reference that was first recorded as contributor-owned is rewritten to repository-owned, so the existing ActiveUsageAdded event no longer matches and the otherwise idempotent Reference create replay is rejected as a different payload. Only apply the accepted-owner rewrite after confirming the add operation id has not already been persisted.
Useful? React with 👍 / 👎.
| command | ||
| | _ -> command | ||
|
|
||
| let decideCommandForKey |
There was a problem hiding this comment.
src/AGENTS.md requires new F# modules, types, functions, methods, members, and meaningful helper functions to have concise /// XML documentation. This new exported module function is the primary-key-scoped decision entry point used by the actor and tests, but it is missing the required XML doc while the wrapper below is documented, so future maintainers lose the intended invariant at the public boundary.
Useful? React with 👍 / 👎.
| | ContentOwnershipOwnerScope.ContributorOwned _ when | ||
| activeUsageOwners.Values | ||
| |> Seq.contains repositoryOwnerScope | ||
| -> | ||
| repositoryOwnerScope |
There was a problem hiding this comment.
When any repository-owned active usage already exists for the manifest, this branch converts every new contributor-owned add into repository-owned usage even if there is no accepted transfer for that contributor. That violates docs/Contributor-owned accounting design.md lines 66-67, where abandoned/rejected private work remains contributor-owned and same-content contributor scopes stay separate; for example, a private save that reuses content already present on a repository-owned branch would be billed to the repository without being accepted.
Useful? React with 👍 / 👎.
Validation worker assignment could not startThe attempted fresh validation-fix worker Newton ( Current actionable state remains:
A fresh validation-fix worker still needs to be assigned when Codex worker capacity is available, or the maintainer needs to explicitly approve the main orchestrator deviating from the Grace rule that coding/fix work is delegated to fresh workers. |
Current-head hard stop: ledger replay and owner-normalization orderingPR #700 received a fresh current-head Codex review on This PR is now stopped under the repeated-review rules. The content ownership ledger is a high-risk actor, storage, retry, idempotency, persisted-shape, and side-effect-ordering surface. The latest review contains another substantive ledger/retry finding and another substantive billing-ownership finding, so this is not routine patch churn. Current state
Review timeline
Suspected missing invariant familyThe ledger needs a clearer separation between three ideas that currently overlap in the implementation:
The CI timeout may be another symptom of the same actor/replay/ledger boundary. The next approved pass should prove the ledger rules and the failing server setup path together unless the maintainer explicitly wants those split. Maintainer decisions neededI generated a local unblock page with longer explanations and clickable choices:
Please approve or revise these decisions before another #653 coding/fix worker is assigned:
Proof obligation for the approved fix
No additional #653 coding or fix worker should be assigned until the maintainer approves or revises these decisions. Also, the attempted fresh worker Newton failed immediately because the Codex usage limit was reached, so no code work began after |
ScottArbeit
changed the title
Maintainer Approval: Replace The Payer LedgerThe maintainer approved replacing #653 and PR #700's contributor payer ledger with the revised BillingAccount Accepted decisions:
The three prior substantive review cycles belong to the superseded Egress remains deferred. The newly recorded concern is that a public repository can continue generating anonymous One fresh GPT-5.6-Sol Medium worker is approved within the revised issue scope. |
Replacement Worker AssignedFresh worker Ohm ( The worker owns replacement of the current payer ledger with the revised #653 BillingAccount storage-admission tracer The worker is explicitly prohibited from implementing egress/public-delivery policy, mutating durable repository Status file: |
Coding Stop: Reservation-To-Branch Binding Needs ApprovalFresh worker Ohm stopped before commit or push. The rejected payer ledger is inverted in the local #653 worktree, and a Code-grounded blocker:
The smallest candidate binding is to add the future Additional decisions are required before a continuation worker can safely wire the lifecycle:
Local decision page:
No replacement commit was created, no push occurred, Fast validation was intentionally not run, and the PR remains at |
Maintainer Correction: Branch Creation Is Not A Storage BoundaryThe previous unblock packet incorrectly treated branch creation as the event that settles upload reservations. The The code-grounded lifecycle is:
Upload finalization proves valid content but remains unattached. Persisting a detached directory graph also does not Maintainer dispositions:
One contract decision remains.
Coding remains stopped until that binding is approved. The recommended shape is for |
Maintainer Resolution: Upload Evidence Belongs In Event PropertiesThe remaining reservation-binding question is resolved with a command-independent rule:
Code-grounded implementation interpretation:
This uses the existing event extensibility construct and does not add a new payer ledger, named per-command upload field, |
Event-Properties Continuation Worker AssignedFresh worker Mencius ( The worker must preserve the prior uncommitted old-ledger inverse and BillingAccount prototype, complete the revised Primary prevention targets:
Status file: |
Fresh Continuation Worker AssignedWorker Mencius completed an intermediate uncommitted slice but stopped without a product blocker and before server Fresh GPT-5.6-Sol Medium worker Avicenna ( Status remains |
BillingAccount Replacement PushedThe revised #653 implementation is committed and pushed on PR #700:
Implemented replacement:
Local evidence:
Local gate limitation:
Current blocking gates:
Residual implementation risks to evaluate against CI/review:
|
Current-Head CI Failure ClassifiedGitHub What is now proven:
Fresh failures:
This is a fresh latest-head CI result against the replacement BillingAccount model, not a recurrence of the old payer |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b965fa2a8
ℹ️ 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".
| @@ -1626,6 +1626,9 @@ module Application = | |||
| route "/setAllowsLargeFiles" Repository.SetAllowsLargeFiles | |||
| |> addMetadata typeof<Repository.SetAllowsLargeFilesParameters> | |||
|
|
|||
| route "/setAllowExternalContributions" Repository.SetAllowExternalContributions | |||
There was a problem hiding this comment.
This new route is registered without requireRepositoryWriteOrAdmin or requireRepositoryAdmin; the endpoint manifest marks it only Authenticated, and the handler validations only check that the repository is not deleted. In a private repository, any authenticated caller who can name the repository can flip AllowExternalContributions, enabling the storage path that was supposed to stay owner-controlled.
Useful? React with 👍 / 👎.
| match validateRepositoryExistsForStorageRequest session.RepositoryId repositoryDto correlationId with | ||
| | Error error -> return! context |> result400BadRequest error | ||
| | Ok () -> | ||
| match! reserveUploadCapacity context repositoryDto session correlationId with |
There was a problem hiding this comment.
reserveUploadCapacity is only called from the content-block upload-URI path (checked repo-wide references), so retained uploads that never request a block URI—whole-file fallback and fully deduped manifest sessions—can finalize and be saved with no BillingAccount reservation or file-size admission. That lets repeated small/whole-file or deduped retained content bypass account capacity.
Useful? React with 👍 / 👎.
| let! settlement = | ||
| match tryGetRetainingDirectoryId cmd with | ||
| | Some directoryId -> settleRetainedUploadSessions graceIds correlationId directoryId retainedUploadSessions metadata |
There was a problem hiding this comment.
Settlement runs only after actorProxy.Handle has already accepted the retaining branch command, so a later BillingAccountCommand.Settle rejection leaves the save/promotion reference durable even though the request returns 500. For example, two external branches can both reserve while the branch limit has room; after the first settle consumes the last branch slot, the second settle fails, but its branch reference has already been written and retains content without settled branch accounting.
Useful? React with 👍 / 👎.
| billingAccount.Handle | ||
| (Grace.Types.BillingAccount.BillingAccountCommand.Settle(settlementId, reservationId, graceIds.RepositoryId, graceIds.BranchId)) |
There was a problem hiding this comment.
This settlement path creates retained BillingAccount usage, but the change adds no matching release for branch/reference logical deletion or save/checkpoint expiry; I checked all BillingAccountCommand.Release call sites and only upload-session abandon/expire invokes it. Once a saved reference with UploadSessionIds expires or is deleted, UsedBytes/ExternalBytes remain consumed permanently even though the content is no longer retained by that reference.
Useful? React with 👍 / 👎.
| let private tryGetRetainingDirectoryId (command: BranchCommand) = | ||
| match command with | ||
| | BranchCommand.Assign (directoryId, _, _, _) | ||
| | BranchCommand.Promote (directoryId, _, _, _) |
There was a problem hiding this comment.
Promotion is treated as a retaining command here, but the settlement path only issues BillingAccountCommand.Settle, and a repo-wide search finds BillingAccountCommand.Promote only in unit tests. When an external branch's saved manifest is promoted to repository-owned usage, its reservation remains ExternalContribution, so ExternalBytes and ActiveExternalBranches stay consumed after acceptance instead of freeing the protected budget.
Useful? React with 👍 / 👎.
| elif storageClass = ExternalContribution | ||
| && account.ActiveExternalBranches | ||
| >= account.ExternalBranchLimit then |
There was a problem hiding this comment.
At reservation time the command has no retaining branch, so this check rejects every external upload whenever ActiveExternalBranches is at ExternalBranchLimit. That includes adding another file to an already-counted external branch—a case Settle handles with ExternalBranchReservationCounts without incrementing branch count—so a branch at the limit becomes unable to update even though it would not consume a new branch slot.
Useful? React with 👍 / 👎.
| let private validateRetainedUploadSessions (graceIds: GraceIds) correlationId (properties: Dictionary<string, string>) command = | ||
| task { | ||
| match properties.TryGetValue UploadSessionIdsProperty with | ||
| | false, _ -> return Ok [] |
There was a problem hiding this comment.
If a client omits UploadSessionIds, this branch accepts the retaining command with no settlement attempt. For a manifest upload that did reserve capacity, the saved reference can then retain content while ActiveExternalBranches never increments, so custom or older clients can bypass the external branch sub-budget simply by leaving out the property.
Useful? React with 👍 / 👎.
| static member public SetAllowExternalContributions(parameters: SetAllowExternalContributionsParameters) = | ||
| postServer<SetAllowExternalContributionsParameters, String> ( | ||
| parameters |> ensureCorrelationIdIsSet, | ||
| $"repository/{nameof (Repository.SetAllowExternalContributions)}" |
There was a problem hiding this comment.
SetAllowExternalContributions adds a public SDK call and server route, but rg for setAllowExternalContributions/SetAllowExternalContributions found no path or parameter schema in src/OpenAPI/Grace.OpenAPI*.yaml or generated SDKs, only RouteClassification. Non-.NET generated clients therefore cannot call the new setting even though the server exposes it.
Useful? React with 👍 / 👎.
| | UploadSessionCommand.Abandon operationId | ||
| | UploadSessionCommand.Expire operationId -> |
There was a problem hiding this comment.
Reservations are released only for Abandon and Expire, but a successfully finalized upload that is never attached moves to RetentionPending and is later cleaned up through DeletePhysicalState; that path compacts/deactivates without releasing an unsettled reservation. In the common failed-or-never-retried save case after a manifest finalize, UsedBytes/ExternalBytes stay consumed forever even though the upload coordination state has been deleted.
Useful? React with 👍 / 👎.
Replacement-Model Stabilization Stop: Retained Storage Must Be Server-AuthoritativeCodex completed review Review And CI AuditSuperseded findings:
Fresh routine findings within the approved replacement contract:
Fresh structural findings:
Current-head CI separately proves that the old Missing Invariant Family
Proposed invariant family:
This is a new durable pending-state and materialization-domain decision. It is not safe to infer from the existing Decision PageThe local unblock page explains four choices with practical tradeoffs:
No fix worker is assigned. After maintainer approval, revise the #653 issue body and #638 stabilization ledger, then |
1 participant
Pull Request
Linked Issue
Related to #653 and parent epic #638.
Summary
This pull request is being revised in place to replace its rejected contributor payer ledger with the approved
BillingAccount storage-admission tracer bullet.
The approved replacement model is:
BillingAccountpays for all retained repository storage, including private branches.AllowExternalContributionscontrols private contributions from users without repository write access.AllowsLargeFilesbecomes a bounded server-enforced file-size tier: one MiB when disabled and a finite configuredmaximum when enabled.
durable reference event produced by the composite command first retains the finalized upload, reclassifies without a
total-byte change on promotion, and releases on rejection, abandonment, expiry, or logical deletion.
Current head
0b965fa2a841ef8654188c005df4db6c97f50a56removes the supersededContentOwnershipLedgerimplementation andimplements the approved BillingAccount storage-admission/event-properties replacement.
Scope
In scope:
hooks.
AllowExternalContributionsand existingAllowsLargeFilesat the appropriate server boundaries.repository-create regression.
Out of scope:
Validation Status
git diff --check: passed.pwsh ./scripts/validate.ps1 -Fast: complete solution build passed with zero warnings and errors; CLI 697/697, Types174/174, Authorization 53/53, and Operations 26/26 passed. Server.Unit did not finish cleanly in the worker shell
because existing static server modules initialized with missing storage configuration and then a null logger factory.
/repository/create: current-head CI created the owner, organization, and three repositories successfully. The oldtimeout is fixed.
Validate / fullrun29148036443, job86532826457, failed after build and fixturesetup. Grace.Server.Tests passed 257/259; failures are BillingAccount reservation operation-id collision and a
large-binary test that did not enable
AllowsLargeFiles. Grace.Server.Unit.Tests had 31 static initialization failuresbecause the unit-test process had no storage account key.
Validate / full, explicit/repository/createproof from that run, and afresh current-head Codex Code Review Bot result.
Review Status
ContentOwnershipLedgerpayer/replay invariantfamily, followed by a maintainer hard stop.
implementation is removed. Do not resolve them as individually fixed before replacement code is pushed.
permission-change accounting is rejected; settlement recovery is client-retry-only; usage meters logical finalized
bytes even when physical content is deduplicated.
UploadSessionIdsvalue inthe resulting event's generic
Properties, not in a primary named field on Branch.Save, Commit, Checkpoint,FileVersion, DirectoryVersion, or each command-specific DTO/event.
Propertiesdictionary to common command parameters; rejectattempts to set protected server-owned metadata; canonicalize and validate
UploadSessionIdsbefore actor invocation.0b965fa2a841ef8654188c005df4db6c97f50a56; worktree isclean and remote-aligned.
4677429660completed on0b965fa2with nine fresh findings./repository/createregression is green.UploadSessionIdscannot be the billing source of truth, post-event settlement can retain unaccounted content, andlogical release/promotion/branch-slot lifecycle requires a server-owned retention invariant.
RetentionClaim; see PRand issue comment
653-retention-stabilization-stop.mdcontent and the local unblock page.threads remain superseded.
C:\Users\scott\AppData\Local\Temp\grace-638-orchestration\grace-653-unblock-current.html.result.
Review/Fix Prevention
The revised issue names the missing invariant family directly: storage admission belongs to a BillingAccount aggregate,
not a per-file payer-transfer ledger. The implementation must use bounded aggregate reads and atomic updates; it must
not scan content history or recreate payer-transfer semantics under another name.
Egress Deferral
Storage exhaustion does not by itself bound public download cost. That concern is explicitly deferred for a separate
design session. This worker must not mutate durable repository visibility at cutoff. A future design must cover a
distinct public-delivery state, anonymous routes, issued SAS URLs, caches/CDNs, authorized access, recovery, and the
relationship between storage and egress budgets.
Docs Impact
Update nearby documentation only where this tracer bullet changes a currently documented repository setting or upload
behavior. Pricing, payment, and egress documentation remain deferred.