fix: distinguish UNIT_MISMATCH from BUDGET_NOT_FOUND on reserve/event/decide (v0.1.25.6)#80
Merged
Merged
Conversation
… paths reserve.lua and event.lua now probe the fixed UnitEnum set when no budgeted scope is found for the requested unit. If any alternate-unit budget exists at any affected scope, the scripts return UNIT_MISMATCH (400) with scope, requested_unit, and available_units in the error payload so the client can self-correct. BUDGET_NOT_FOUND (404) remains the response only when the scope has no budget in ANY unit. Addresses runcycles/cycles-client-rust#8 where a TOKENS reservation against a USD_MICROCENTS budget returned 404 BUDGET_NOT_FOUND with no hint toward the fix. event.lua had the same latent bug. Changes: - reserve.lua: new ARGV[15] units_csv; alternate-unit probe added to the #budgeted_scopes == 0 branch; affected_scopes now start at ARGV[16] - event.lua: new ARGV[14] units_csv; symmetric probe; scopes at ARGV[15] - RedisReservationRepository: UNIT_CSV constant derived once from Enums.UnitEnum.values(); passed into createReservation and createEvent - evaluateDryRun: Java mirrors the probe and throws UNIT_MISMATCH (400) instead of silently returning DENY with reason_code=BUDGET_NOT_FOUND - handleScriptError: extracts scope/requested_unit/available_units when present; falls back to the legacy no-detail factory for commit.lua's UNIT_MISMATCH responses which do not carry details - CyclesProtocolException: unitMismatch(scope, requestedUnit, availableUnits) overload populating details - Integration tests: ReservationLifecycleIntegrationTest and DecisionAndEventIntegrationTest now assert 400 UNIT_MISMATCH with details; added regression guards for the truly-missing case (jedis.del the budget, expect 404 BUDGET_NOT_FOUND) - Unit tests: CyclesProtocolExceptionTest and RedisReservationCoreOpsTest cover both the detail-bearing and legacy forms - AUDIT.md + cycles-protocol-service/README.md updated - Version bump 0.1.25.5 -> 0.1.25.6 Cascade semantics preserved: scopes without a budget at the requested unit are still silently skipped during the main validation loop; the probe only fires when every affected scope misses. No hot-path change.
…d to expected_units Addresses the remaining gaps from #79: 1. /v1/decide had the same latent bug as reserve/event: when the requested estimate.unit didn't match any budget at the derived scopes, it silently returned 200 DENY with reason_code=BUDGET_NOT_FOUND, with no way for the caller to tell "wrong unit" from "truly missing". `decide()` now runs the symmetric probe and throws UNIT_MISMATCH (400) with scope/requested_unit/expected_units details when an alternate unit is found. Spec line 1131-1134's "MUST NOT return 409" rule applies only to debt/overdraft conditions on /decide; 400 for a request-validity error (wrong unit) is permitted and consistent across all four entry points (reserve, reserve dry_run, event, decide). 2. The detail field is renamed from `available_units` to `expected_units` across the stack (Lua, Java factory, handleScriptError parser, all tests, docs) to match the field name called out in issue #79's acceptance criteria (AC 2). 3. A shared private helper `probeAlternateUnits(jedis, scope, requestedUnit)` in RedisReservationRepository is now used by both `evaluateDryRun` and `decide`, avoiding duplication between the two Java-side probes. New integration tests in DecisionAndEventIntegrationTest cover both /decide paths: wrong-unit → 400 UNIT_MISMATCH with details, and truly-missing → 200 DENY with reason_code=BUDGET_NOT_FOUND (regression guard preserving decide's "no 4xx for budget-state" convention for the no-budget case). Build: 291 tests, 0 failures, jacoco coverage checks met.
Updates the stale 0.1.25.5 jar filename / image tag examples in the top-level README.md and cycles-protocol-service/README.md to match the pom.xml revision bumped in 6224e7e. Historical references in BENCHMARKS.md and the AUDIT.md changelog sections stay as-is.
5 tasks
…t probe's field shape
The internal-consistency branch at event.lua:108-114 was emitting
`{error, scope, expected, actual}` — a different field-name shape than
the new cross-unit probe at event.lua:133-160 which emits
`{error, scope, requested_unit, expected_units}`. Java's
handleScriptError only extracts details from the new shape, so if the
defensive branch ever fired, the `expected`/`actual` fields would
silently drop through to the no-detail factory.
This branch is dead code in practice — it fires only when
`budget:<scope>:<unit>` exists AND the stored `unit` field inside the
hash disagrees with the key's unit suffix (an internal data-integrity
inconsistency that shouldn't happen under normal operation). Rather
than delete it, keep it as a defensive safety net but emit the same
shape as the cross-unit probe, so:
1. If the dead branch ever fires due to a data-corruption incident,
clients get a structured UNIT_MISMATCH with populated details
instead of a no-detail fallback.
2. Future readers only see one UNIT_MISMATCH response shape in
event.lua, not two.
3. handleScriptError's detail extraction works uniformly for both
code paths.
Wraps the single stored unit as a one-element `expected_units` array
to match the cross-unit probe's array shape.
Added a comment explaining why the two branches coexist and why this
one is a safety net rather than a hot path.
Review feedback nit N1 from #80.
amavashev
added a commit
to runcycles/cycles-protocol
that referenced
this pull request
Apr 10, 2026
Review feedback nit N2 from runcycles/cycles-server#80: the broadened UNIT_MISMATCH list in cycles-protocol-v0.yaml line 56 enumerated (a) reserve, (b) commit, (c) event — but the reference server also throws 400 UNIT_MISMATCH on /v1/decide when estimate.unit does not match any budget stored for the derived scopes. This is spec-compatible because /v1/decide's response list already declares 400, and the "MUST NOT return 409" clause in the /decide description is specific to budget-state conditions (debt, overdraft, insufficient remaining), not request-validity errors. But the normative UNIT_MISMATCH list was incomplete — a reader of the spec alone wouldn't know /decide can 400 UNIT_MISMATCH. Add (d) decide — estimate.unit does not match any budget stored for the derived scopes … with an explanatory note that this is an exception to /decide's general "return 200 DENY without 4xx" pattern, which applies only to budget-state conditions, not request-validity errors. No schema change — ErrorResponse already supports arbitrary details and /v1/decide's response list already declares 400. Normative text lives in info.description which the merge script replaces with a generated stub, so merged/cycles-openapi-protocol-merged.yaml is unchanged (idempotent merge confirmed via python scripts/merge_specs.py). Companion server change (nit N1 fix) lands in runcycles/cycles-server#80.
Collaborator
Author
|
Review feedback addressed in e42ecad — event.lua defensive UNIT_MISMATCH branch now emits the same |
amavashev
added a commit
that referenced
this pull request
Apr 10, 2026
Write-path (vs v0.1.25.5, 0.1.25.6 in parens): Reserve 7.2ms → 6.0ms p50 Commit 5.8ms → 5.0ms p50 Release 6.0ms → 4.8ms p50 Extend 9.0ms → 7.5ms p50 Decide 6.3ms → 5.9ms p50 Event 6.2ms → 5.0ms p50 Reserve+Commit 16.7ms → 14.3ms p50 Reserve+Release 14.2ms → 12.2ms p50 Read-path: within ±0.4ms noise across GET reservation / GET balances / LIST reservations / Decide (pipelined). Concurrent (Reserve+Commit lifecycle): 8 threads: 752.8 → 789.4 ops/s (+4.9%) 16 threads: 1063.2 → 1122.4 ops/s (+5.6%) 32 threads: 2519.8 → 2624.0 ops/s (+4.1%) Zero errors at all concurrency levels. All deltas are run-to-run environmental variance. The UNIT_MISMATCH probe added to reserve.lua / event.lua only fires when #budgeted_scopes == 0 — an error path that no benchmark workload exercises. The extra units_csv ARGV string per request is parsed only if the probe fires, so the hot path ignores it. No hot-path change, no measurable regression. Environment: Windows 11 Pro for Workstations, AMD Ryzen Threadripper 3990X 64-Core, Java 21, Docker + Redis 7 (Testcontainers). Base commit 89d2651 (post-merge of #80). 200 measured iterations after 50 warmup, via mvn test -Pbenchmark.
Merged
6 tasks
amavashev
added a commit
that referenced
this pull request
Apr 11, 2026
…de as Enums.ReasonCode Follow-up to v0.1.25.6 and the companion spec enum enhancement in runcycles/cycles-protocol#26. The spec now declares DecisionResponse.reason_code and ReservationCreateResponse.reason_code as a closed enum (DecisionReasonCode with 6 values); this commit promotes the Java side from free-form String to a typed enum so the compiler catches drift against the spec at build time instead of at runtime. Changes: - Enums.java: added ReasonCode enum with the same 6 values as the spec's DecisionReasonCode schema: BUDGET_EXCEEDED, BUDGET_FROZEN, BUDGET_CLOSED, BUDGET_NOT_FOUND, OVERDRAFT_LIMIT_EXCEEDED, DEBT_OUTSTANDING Includes a class-level javadoc explaining the distinction from ErrorCode (same labels overlap, different response types — reason codes appear on 200 DENY, error codes on 4xx/5xx). - DecisionResponse.reasonCode / ReservationCreateResponse.reasonCode: type changed from `@Size(max=128) String` to `Enums.ReasonCode`. Jackson's default enum serialization produces the enum's name() string in JSON, so the wire format is byte-identical to v0.1.25.6 — no client impact, no version bump needed. - RedisReservationRepository.java: 10 .reasonCode(...) call sites updated to pass Enums.ReasonCode constants instead of string literals. The two dynamic "BUDGET_" + budgetStatus concatenations become conditional ternaries: "FROZEN".equals(budgetStatus) ? Enums.ReasonCode.BUDGET_FROZEN : Enums.ReasonCode.BUDGET_CLOSED The preceding `if ("FROZEN".equals(...) || "CLOSED".equals(...))` check guarantees those are the only two branches reached. - Controller boundary (DecisionController, ReservationController): two `.reasonCode(response.getReasonCode())` call sites that pass the value into EventDataReservationDenied (a webhook event payload model, which keeps its String-typed reasonCode as that's its own wire contract) updated to explicitly convert via .name(), with a null guard. This preserves the webhook payload shape exactly. - Test assertions: 11 unit tests in RedisReservationCrudTest and RedisReservationDecideEventTest that compared `.getReasonCode()` against string literals updated to compare against Enums.ReasonCode constants. Integration tests that read `resp.getBody().get("reason_code")` from parsed JSON (as a String) are unaffected because Jackson serializes the enum to its name. - Test fixture: ReservationControllerTest.denyResponse() builder updated to use Enums.ReasonCode.BUDGET_EXCEEDED instead of the string literal. Not touched: - EventDataReservationDenied.reasonCode stays String-typed. That field is the webhook event payload contract (admin plane / webhook spec), which is wire-independent from DecisionResponse and has its own serialization target (webhook POST bodies). Changing it would affect webhook consumers. If desired, a follow-up could tighten that field too, but it's not part of the runtime plane enum refinement. - Protocol version stays at 0.1.25.6. No wire format change, purely internal compile-time tightening. - cycles-protocol-service/pom.xml <revision> unchanged. Verified: - mvn verify on the full reactor: all modules SUCCESS, all tests green (including Testcontainers integration tests exercising the wire format), jacoco coverage checks met. Cross-refs: - Companion spec enum: runcycles/cycles-protocol#26 (fix/spec-not-found-naming) - Original v0.1.25.6 context: #79, #80
This was referenced Apr 11, 2026
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.
Summary
Closes #79. Addresses runcycles/cycles-client-rust#8.
POST /v1/reservationsreturned 404BUDGET_NOT_FOUNDwhen the requested unit didn't match any budget stored at the derived scopes, even when the scope had a budget in a different unit./v1/eventsand/v1/decidehad the same latent bug. Clients couldn't distinguish "no budget at scope" from "wrong unit" and had no hint toward the fix.This PR implements Option A from #79 (the preferred approach): on the empty-budgeted-scopes error path only, probe the fixed `UnitEnum` set (`USD_MICROCENTS`, `TOKENS`, `CREDITS`, `RISK_POINTS`) via `EXISTS budget::<unit_alt>` for each affected scope. If any alternate-unit budget exists, return `UNIT_MISMATCH` (400) with `scope`, `requested_unit`, and `expected_units` in `details` so the client can self-correct. Otherwise fall through to the existing `BUDGET_NOT_FOUND` (404).
Cascade semantics preserved — the probe only fires when every affected scope missed, so scopes without a budget at the requested unit are still silently skipped during the main validation loop. No hot-path change; the cost is paid once on the error path only.
What changed
Core fix (reserve.lua + event.lua):
Symmetric Java probes (non-Lua paths had the same bug):
Exception + handler plumbing:
Tests (+ integration regression guards):
Version + docs:
Companion spec change
Spec side lands in runcycles/cycles-protocol on the same branch name (`fix/reserve-unit-mismatch`): broadens the normative UNIT_MISMATCH wording in `cycles-protocol-v0.yaml` line 56 to cover reserve, and adds `"404": BUDGET_NOT_FOUND` response blocks to `/v1/reservations` POST and `/v1/events` POST (closes the conformance gap called out in #79). `/v1/decide` response list unchanged because it still returns 200 DENY for the no-budget case.
Acceptance criteria (#79)
Test plan
Rust client
No SDK change required — the structured error is enough for the call-site in runcycles/cycles-client-rust#8 to self-correct. The defensive workaround in cycles-client-rust v0.2.3 remains forward-compatible.