feat: carry guaranteed bit rates from slice policy, and recognise GBR properly

[midwell]

What

Two changes, together because the first does nothing without the second, plus one consequence of
the second that had to come with it.

1. makeQosDesc carries the guaranteed rates. It built a QosData from the polled slice
policy with the 5QI, ARP and maximum bit rates, and dropped any guaranteed rate, so a slice policy
could express only a ceiling. It now carries GbrUl and GbrDl the same way it carries MaxBrUl
and MaxBrDl.

2. The PCF decided whether a flow is GBR with Var5qi <= 4, in seven places. That is not the
3GPP definition. TS 23.501 table 5.7.4-1 gives a GBR resource type to 5QI 1 to 4, 65 to 67 and 71
to 76, and a delay-critical GBR resource type to 82 to 90. So 5QI 66 or 82 are mishandled today,
silently: a guaranteed rate the AF requests is neither authorised on the QoS data nor budgeted
against the UE's aggregate. context.IsStandardisedGbr5QI reads the table instead.

3. util.MediaTypeTo5qiMap is now read checked. This is forced by 2 rather than chosen, and it
is the one place where behaviour changes for an input that is not a GBR 5QI, so it is worth stating
plainly. The map was read unchecked at both mapping sites, so a media type outside the enum yielded
a 5QI of 0 — and <= 4 accepted it, meaning an AF sending an unrecognised media type had its
requested rates applied under a value TS 23.501 does not assign. Reading the table instead would
have silently stopped applying them. So the lookup is checked and an unmapped media type keeps the
documented default of 9, which leaves no behaviour difference for any input the enum can produce.
TestMediaTypeTo5qiMapCoversEveryMediaType pins the map as total over
models.AllowedMediaTypeEnumValues, since that totality is the only reason the default was
unreachable for valid input.

Why all seven had to move together

Five of the seven are on the policy authorization path, where the 5QI is derived from the AF's
media type and is therefore always a standardised value — in practice always 1, 2 or 9. Three of
those five decide for a newly created PCC rule and two for an existing one: the same question
either side of a found-or-create branch. Fixing only the create arms would leave an AF's second
request for a rule answered differently from its first.

The remaining two are the aggregate budget's two directions, and they are a pair for the same
reason. DecreaseRemainGBR debits a requested guaranteed rate against the UE's aggregate;
IncreaseRemainGBR credits it back when RemovePccRule releases the rule. Moving only the debit
would leave the credit reading <= 4, so a rate debited for a 5QI of 66 — which is exactly what
this change starts doing — would never be given back, and the aggregate would shrink by that rate
once per rule, permanently. Neither gate fires for such a 5QI today, so the pair is consistent
before and after; it is splitting them that would introduce the leak. Thanks to the review that
caught this while the two were split.

Site	Decides
context/ue.go:373	whether to credit a released guaranteed rate back to the UE aggregate
context/ue.go:402	whether to budget a requested guaranteed rate against the UE aggregate
producer/policyauthorization.go:362	new PCC rule, postAppSessCtxProcedure
producer/policyauthorization.go:387	existing PCC rule, same procedure
producer/policyauthorization.go:655	new PCC rule, handleCombinedMediaSubComponents
producer/policyauthorization.go:1045	new PCC rule, ModAppSessionContextProcedure
producer/policyauthorization.go:1062	existing PCC rule, same procedure

Tests

Each test drives a call site rather than the helper, so what is under test is the decision and not
a table lookup. Each was checked by reverting the change under it and watching it fail, not only by
watching it pass:

• TestDecreaseRemainGBRBudgetsGbr5QIAboveFour — a 5QI of 66 returns the requested rates and
  decrements the aggregate budget. Reverted: empty rates and an untouched budget.
• TestRemovePccRuleRestoresBudgetForGbr5QIAboveFour — releasing that rule credits the same rate
  back. Reverted: the budget stays short by the released rate, for good.
• TestDecreaseThenRemoveLeavesAggregateUnchanged — the invariant the pair exists to keep, stated
  end to end: allocate then release leaves the aggregate where it started. It fails first if the
  debit never happened, so it cannot pass by both directions doing nothing.
• TestDecreaseRemainGBRLeavesNonGbr5QIUnbudgeted and
  TestRemovePccRuleLeavesBudgetAloneForNonGbr5QI — the mirror cases, so neither direction can
  later be widened into treating everything as guaranteed.
• TestHandleCombinedMediaSubComponentsAuthorisesGbr5QIAboveFour — a 5QI of 66 is authorised a
  guaranteed rate on the QoS data that is stored. Reverted: empty.
• TestMakeQosDescCarriesGuaranteedBitRates and TestMakeQosDescLeavesUnconfiguredGuaranteedRatesUnset
  — a configured guarantee is carried through, and an unconfigured one is not invented from the
  maximum.
• TestMediaTypeTo5qiMapCoversEveryMediaType — the totality that makes 3 a no-op for valid input;
  verified by deleting an entry and watching it fail.
• TestIsStandardisedGbr5QI asserts the full sets from the table, including the Non-GBR values that
  must stay out. That list now includes 5QI 10, the Rel-17 satellite value with an 1100 ms packet
  delay budget — the one standardised value the test previously left uncovered.

Gates run: make test (race and coverage, all nine packages green), golangci-lint v2.13.2 — the
version main.yml pins rather than the Makefile's latest default — 0 issues, and
make check-reuse compliant.

Dependency

nfConfigApi.PccQos.GbrUl/GbrDl come from openapi #176, released in v2.2.2. main already
requires v2.2.2, so this PR changes no pin.

Deliberately not included

Three of these are pre-existing defects in the same accounting. None is this change's, each wants
its own fix, but the first two widen slightly in reach here, so they are named rather than left
implicit.

• The modify path's rollback can erase the aggregate cap. On a failed modify,
  producer/smpolicy.go restores RemainGbrDL/RemainGbrUL from IncreaseRemainGBR's return
  values, which are nil whenever the previous rate was unset — and a nil budget reads downstream as
  no limit rather than as zero. The create arm has no rollback at all, so a request that passes on
  DL and fails on UL leaves DL debited. Both were unreachable for a 5QI above 4 before, because the
  old gate meant DecreaseRemainGBR could not fail for one.
• Slice-configured rules are credited but never debited. Nothing debits the aggregate for a
  rule that comes from the polled slice policy, but RemovePccRule credits whatever guaranteed
  rate the QoS data carries — which, after change 1, is no longer always empty. Where the
  subscription supplies an aggregate GBR, releasing such a rule can inflate the budget past it. The
  design question underneath is whether slice-configured guarantees should be budgeted at all,
  which is why it is not settled here.
• The existing-rule arms double-debit. Both re-debit an already-debited QoS without crediting
  it first, so an AF re-requesting a rule debits twice and only the last value is credited on
  release. Inert on this path, where the media type maps to a 5QI of 1, 2 or 9 only.
• A classifier for a dynamically assigned 5QI. Such a value sits outside table 5.7.4-1 and
  carries its own QoS characteristics, so what would settle it is the presence of guaranteed rates.
  Nothing in the PCF has to make that decision today: makeQosDesc carries the configured rates
  through without consulting the 5QI, and the SMF's CreateDedicatedQosQer keys off whether the
  rates are present rather than off the 5QI. An exported helper with no caller would only be a
  guess at the shape of that caller, so it is left for whichever change needs it.
• A warning when a guaranteed rate is configured against a Non-GBR 5QI. Worth having, but it is
  a configuration-validation change in its own right. Opened as feat: warn when a guaranteed rate is configured against a Non-GBR 5QI #391, stacked on this one.
• Anything SMF-side. Noted only because it bounds what this delivers end to end: the SMF sets
  GBR on the QER in CreateDedicatedQosQer, which the policy-update path reaches, while the
  establishment path's CreatePccRuleQer sets MBR only. A slice-configured guarantee therefore
  takes effect on a policy change and not at session establishment. That is an SMF gap, not this
  change's.

[midwell]

The producing end is webconsole#565, which adds app-gbr-uplink/app-gbr-downlink to a slice's application filtering rules and carries them into nfConfigApi.PccQos. The two are independent — this one carries whatever a policy provides, and that one is inert until something reads the fields — so neither gates the other.

[midwell]

Pushed 35626b9 — one behaviour change I had left unremarked, found reviewing the diff again rather than by a failing test.

util.MediaTypeTo5qiMap was read unchecked at both mapping sites, so a media type outside the enum yielded a 5QI of 0, and var5qi <= 4 accepted it: an AF sending an unrecognised media type had its requested rates applied under a value TS 23.501 does not assign. Reading the table instead silently stops applying them — strictly more correct about the 5QI, and a quiet loss of rate limiting on that input.

So the lookup is now checked and an unmapped media type keeps the documented default of 9, which leaves no behaviour difference for any input the enum can produce. TestMediaTypeTo5qiMapCoversEveryMediaType pins the map as total over models.AllowedMediaTypeEnumValues, since that totality is the only reason the default was unreachable for valid input; verified by deleting an entry and watching it fail.

Gates re-run on the pushed tree: make test green across all nine packages, golangci-lint v2.13.2 0 issues, make check-reuse compliant.

[midwell]

Follow-up opened as #391, stacked on this one: it warns when a policy configures a guaranteed rate against a standardised Non-GBR 5QI, which this PR's table makes checkable. It is the item this PR's description listed as deliberately not included, so it is separate rather than folded in here.

One thing from it is worth knowing here: it reads the Non-GBR block of TS 23.501 table 5.7.4-1 and that block includes 5QI 10, added for satellite access with an 1100 ms packet delay budget. IsStandardisedGbr5QI is correct for it — 10 is not in the GBR set — but the Non-GBR list in TestIsStandardisedGbr5QI here omits it, so nothing pinned the behaviour for the one standardised value a geostationary deployment actually runs. #391 asserts it on its own. Happy to add it to this PR's list instead if you would rather the coverage sat with the table.

[Copilot]

🟡 Changes recommended

context/ue.go still contains an IncreaseRemainGBR path that uses the old GetVar5qi() <= 4 check, which will keep GBR budgets inconsistent for standardised GBR 5QIs above 4.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR improves QoS/GBR handling in the PCF by (1) propagating guaranteed bit rates from slice PCC QoS configuration into generated QosData, and (2) replacing the incorrect 5QI <= 4 GBR classification with a standards-aligned helper (context.IsStandardisedGbr5QI) across policy authorization and UE GBR budgeting paths.

Changes:

• Extend makeQosDesc to carry GbrUl/GbrDl from slice policy into models.QosData.
• Replace 5QI <= 4 GBR detection with IsStandardisedGbr5QI in policy authorization and UE remaining-GBR decrease logic.
• Add targeted tests validating GBR classification (including 5QI 66) and guaranteed-rate propagation.

File summaries

File	Description
util/pcc_rule_test.go	Adds coverage to ensure MediaTypeTo5qiMap is total over the MediaType enum and never yields 5QI=0.
producer/policyauthorization.go	Uses checked media-type→5QI lookup and standards-based GBR classification for QoS/rate updates.
producer/policyauthorization_test.go	Adds a regression test ensuring GBR 5QI>4 (e.g., 66) gets guaranteed rates authorized into stored QoS data.
polling/pcc_policy_config.go	Propagates configured guaranteed bit rates (GbrUl/GbrDl) into generated QosData.
polling/pcc_policy_config_test.go	Adds tests verifying guaranteed bit rates are carried when configured and remain unset otherwise.
context/ue.go	Switches remaining-GBR decrease budgeting to standards-based GBR detection.
context/gbr.go	Introduces IsStandardisedGbr5QI helper based on TS 23.501 table 5.7.4-1 sets.
context/gbr_test.go	Adds tests asserting the helper’s full standardised GBR/non-GBR behavior and budgeting effects.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Two critical accounting issues remain unresolved: partial-debit rollback and slice-rule aggregate-credit handling.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 2
• Review effort level: Lite

[midwell]

Both accounting findings are right, and both are reachable because of this PR rather than in spite of it. Fixed in 1a94202, rebased onto your replay of the branch rather than pushed over it.

Credit without a debit. The credit read the guaranteed rates off the stored QosData. That was sound while only an application function's request put them there — modifyRemainBitRate writes back exactly what it managed to take, so stored rate and debited rate were the same number. Carrying slice policy's guaranteed rates into QosData is what breaks it: such a rule has rates but never went through DecreaseRemainGBR, and it still reaches RemovePccRule by the ordinary routes, a delete operation or an installation the SMF reports as failed. The budget would be credited a rate nobody took and rise above the aggregate the session started with.

The credit is now the mirror of a recorded debit, keyed by QoS data id, so a rule with no record is given back nothing. Tracing the takers is what made this more than a one-line change: modifyRemainBitRate debits directly without going through DecreaseRemainGBR, and it is the live AF path — recording only at the DecreaseRemainGBR call sites would have stranded those debits and left the budget permanently short, which is the same defect with the sign flipped.

Partial debit. Confirmed: the downlink is taken first, and on an uplink failure DecreaseRemainGBR returned with the downlink spent. The create arm returns on that error without restoring it, so the budget stayed short for the life of the session, once per refused request. The modify arm escaped it only because it happened to hold a snapshot. It is both directions or neither now, inside the function, so it is atomic for every caller rather than for the one that remembered.

Two adjacent defects in the same accounting are still not fixed and are named in the description rather than folded in: modifyRemainBitRate's own rollback dereferences a possibly-nil rate, and it restores only the uplink.

Both fixes are mutation-verified — reverting the credit to read stored rates fails the slice-rule test, and removing the downlink restore fails the atomicity test. Two earlier tests moved onto the recorded debit, since they had encoded the invariant this change replaces. Gates on the pushed head: go test ./... -race green across all packages, pre-commit run --all-files all hooks, golangci-lint v2.13.2 0 issues.

[midwell]

Rebased onto main at 0313d27, and pushed one further commit that came out of doing so.

#397 raises the pinned race gate from -count=1 to -count=20, and reading that against this diff rather than just rerunning it found something the gate would not have caught on its own: the debit ledger the previous commit added is reached from two HTTP handler goroutines for the same session — the application function path through modifyRemainBitRate, and the SM policy update path through RemovePccRule — and it had no lock.

The tempting reading is that it is no worse off than its neighbours, since RemainGbrUL and RemainGbrDL are written without a lock too. The failure modes are not the same size. A racy *float64 is a wrong number; a concurrent map write is a fatal runtime throw that takes the element down. So new shared state should not turn an existing correctness race into a crash, and the map now carries its own mutex — a leaf, nothing else acquired while it is held, so it cannot form a cycle with the PolicyMu #392 proposes.

The read and the delete are one critical section now, with the credit applied outside it. Two callers releasing the same rule would otherwise both read the entry before either removed it, and the budget would be credited twice for a debit taken once.

-count=20 -race passes on the unguarded code, which is the point worth recording: every test here was single-goroutine, so raising the count only ran tests that could not fail. The hazard had to be driven deliberately, and the new two-goroutine test reports DATA RACE and fails when the guard is removed.

Gates on d0c4bae: go test -race ./... -count=20 green across all nine packages, pre-commit run --all-files clean, golangci-lint v2.13.2 0 issues.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved rollback and concurrency issues remain, including a critical downlink-only panic.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1

Open (3)

• Guard UL rollback for downlink-only GBR requests · New
• These assignments make slice-configured GBR rates visible to IncreaseRemainGBR when the rule is…
• Restore prior ledger debit when replacement debit fails · New

Resolved since last review (1)

• This broadens DecreaseRemainGBR to 5QI 66/other GBR values, which makes the existing SM-policy…

[midwell]

Both right, and the first is a defect in the ledger I added in the previous round rather than one this branch merely exposes.

The failed modify path lost the ledger entry. The arm credited the rule's previous debit back, took the new one, and on failure restored the budget from the snapshot the credit returned. But the credit also removes the ledger entry, and nothing put it back — so the budget was correct again while the record of it was gone, and the next release of that rule found nothing to credit. The aggregate shrank for the life of the session, silently.

Fixing it exposed a second defect in the same three lines, which is the part I would not have found from the report alone. The restore assigned the snapshot pointers onto the session instead of writing through the ones it already held. Those snapshots are nil whenever there was nothing to credit, so the failure path installed a nil budget — read downstream as no limit at all, not as zero. The ledger makes that more reachable than it was before, because there are now more ways for the credit to find nothing.

Both are gone. ReplaceGbrDebit performs the whole exchange, restores through the session's own pointers and re-records the prior debit, so the call site has no opportunity to get either wrong — the same shape as making the debit atomic in the last round.

The rollback panic is real and "pre-existing" was not a good enough answer. I named it in an earlier round's description as a known adjacent defect; your point stands that widening the GBR path past 5QI 4 is what makes it reachable, and that is this branch. On a downlink-only request the uplink is never touched, qosData.GbrUl is unset, and an unset NullableString's Get() is nil — so an exhausted downlink budget panicked instead of returning the authorization error. Verified rather than assumed, then fixed to give back only the direction actually taken, through the accessor that answers "".

Three tests, each driven through the real path and each mutation-verified: the ledger survives a refused modification and the rule still credits back on release; the budget pointers are rewritten rather than replaced; and a downlink-only request against an exhausted budget returns a problem detail instead of panicking.

Worth recording about that verification: my first mutation run on the ledger fix passed, and it was a compile error — removing the restore left a variable unused, and the filter over the test output swallowed the build failure so an empty result read as green. Re-run building first and unfiltered, it fails as it should.

Gates: go test -race ./... -count=20 green across all nine packages, pre-commit run --all-files clean, golangci-lint v2.13.2 0 issues.

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical concurrency gaps can leave GBR budget debits and ledger entries inconsistent during concurrent modification or removal.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Release transaction allows concurrent ledger corruption · New
• Non-atomic debit and ledger update causes permanent budget loss · New

Resolved since last review (3)

• Guard UL rollback for downlink-only GBR requests
• These assignments make slice-configured GBR rates visible to IncreaseRemainGBR when the rule is…
• Restore prior ledger debit when replacement debit fails

[midwell]

Right, and the fix I reached for first was wrong in the way the finding predicts.

The transaction gap is real. The mutex guarded individual map accesses, not the sequence they belong to, so between ReplaceGbrDebit's credit and its record another handler could claim the same QoS data id — and the debit written afterwards belonged to a rule already released, leaving the aggregate short with nothing recorded to credit it back.

Holding the lock across the exchange alone was not enough, and the concurrency test written to prove the fix is what caught that. IncreaseRemainGBR still credited outside the lock while ReplaceGbrDebit mutated the same budget inside it, so the two raced on the *float64. A lock only the newest caller takes reads as protection while providing none. Every budget mutation in the package is under it now — IncreaseRemainGBR, DecreaseRemainGBR and the replace that composes them — with unexported variants for callers already holding it. Mutation-verified: reinstating the partial shape gives DATA RACE on the first iteration.

Worth saying that the test failed once for its own arithmetic before it failed for the code: the fixture recorded a debit while setting the budget as if nothing had been taken, so the invariant never held at the start. The comment in it now says so, because the next reader will build the same fixture.

What this does not cover, and I would rather name it than let a half-lock read as complete. modifyRemainBitRate changes the aggregate directly from the producer, outside this lock. I checked whether that gap is mine: it is not — upstream/main has no lock on the budget at all, and three paths mutate it, so the budget has never been transactional. My ledger added one more thing losable in that same window, and this commit closes the window this branch opened. Closing the other one properly is a per-session transaction over the whole budget, which is the shape #392 proposes for the policy decision; it belongs there or in its own change rather than as a second overlapping lock design here. Happy to take it either way if you would prefer it in this PR.

Gates on 8288358: go test -race ./... -count=20 green across nine packages, pre-commit run --all-files clean. One errcheck finding on the new test caught by the pinned lint before the commit landed, fixed.

[Copilot]

Copilot review overview

🟡 Changes recommended

GBR debit recording can race removal and record rates for directions that were not actually debited.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Record only GBR directions actually debited · New

Resolved since last review (2)

• Non-atomic debit and ledger update causes permanent budget loss
• Release transaction allows concurrent ledger corruption

[midwell]

Right, and reachable by a route worth spelling out: two of modifyRemainBitRate's five call sites read the QoS data straight out of the stored policy decision, which is exactly where a slice-derived entry with GBR rates and no debit lives. So recording both directions entered a rate nobody charged into the ledger, and releasing the rule credited it. That is the defect the ledger exists to prevent, arriving through the write path instead of the read.

Only the directions this call actually took are recorded now, and they are merged rather than replacing the entry. Replacing it would have been a second bug in the other direction: an earlier operation that debited only the uplink would have its record dropped by a later downlink-only one, and that debit would never be credited back.

The test written for it found a second defect, in code this branch added. IncreaseRemainGBR still returned early when the policy decision or its QosDecs map was absent. Those guards belonged to the version that read rates out of the decision; with the ledger as the record they mean a debit that was taken goes uncredited, silently and for the life of the session. Removed.

That one is reachable beyond the fixture: RemovePccRule sets decision.QosDecs = nil once the last entry is deleted, so a rule referencing two QoS data ids would skip the credit for the second.

Which points at something I am not changing here: RemovePccRule's own decision.QosDecs != nil guard around the credit has the same shape and the same consequence, and it is pre-existing rather than introduced by this branch. Fixing it means deciding whether the credit should be driven by the ledger alone rather than by the decision's shape — a change to that function's contract that I would rather not fold into a PR about carrying slice GBR rates. Say the word if you would prefer it here.

Both fixes mutation-verified: recording both directions again inflates the aggregate by the 5 Mbps that was never debited, and restoring the guards leaves the owed downlink credit unpaid.

Gates on 5a6cff5: go test -race ./... -count=20 green across nine packages, pre-commit run --all-files clean.