NO-ISSUE: fix - preserve billing context through transient states and treat DELETING as billing-ending by omer-vishlitzky · Pull Request #176 · osac-project/osac

omer-vishlitzky · 2026-08-06T09:45:06Z

Summary

Fixes a billing data integrity bug where transient states (STOPPING,
STARTING) destroyed billing context, causing suspended.v1 events to
have null duration_seconds and wrong previous_state.

Bug

When a RUNNING VM is stopped, the state machine goes:
RUNNING → STOPPING → STOPPED

The Watch Consumer emitted:

updated.v1 for STOPPING — overwrote IsBillable=false, BillableSince=nil
suspended.v1 for STOPPED — duration_seconds=null, previous_state=STOPPING

The design doc (line 341) says suspended.v1 should carry
duration_seconds for billing closure. Adapters watching for
suspended.v1 to close billing intervals got no duration.

Root cause

resolveUpdatedEventType mapped STOPPING to updated.v1. The
projection update overwrote IsBillable=false and BillableSince=nil.
When STOPPED arrived, billing context was gone.

Fix

STOPPING and STARTING now return ErrTransientState. The Watch
Consumer catches this and updates only FulfillmentVersion +
TransitionTime in the projection — CurrentState, IsBillable,
and BillableSince are all preserved. No CloudEvent is published.

When the final state arrives (STOPPED), the projection still shows
CurrentState=RUNNING, so suspended.v1 has previous_state=RUNNING
and correct duration_seconds. This matches the design exactly.

DELETING emits suspended.v1 (billing closure), same as
STOPPED/PAUSED/FAILED — it is a billing-ending state, not transient.

Unknown states return an error (fail fast). All supported state
transitions are explicit in the switch.

Before/After

Transition	Before	After
RUNNING→STOPPING	`updated.v1` (destroyed billing context)	No event (projection: version+time only)
STOPPING→STOPPED	`suspended.v1` (duration=null, prev=STOPPING)	`suspended.v1` (duration=3600, prev=RUNNING)
RUNNING→DELETING	`updated.v1` (consumed billing context)	`suspended.v1` (duration=3600, billing closed)
Unknown state	`updated.v1` (silent)	Error (fail fast)

Test plan

ginkgo run -r internal — 127 specs pass
RUNNING→STOPPING→STOPPED: no event for STOPPING,
suspended.v1 has previous_state=RUNNING and duration_seconds ≈ 3600
(fixed timestamps prove BillableSince preserved, not reset)
STOPPING/STARTING return ErrTransientState
DELETING emits suspended.v1
Unknown state (proto value 9999) returns error, not ErrTransientState
Pre-commit passes

References

Design doc line 341: "suspended.v1 — Billing interval closes with exact duration_seconds"
Design doc line 416: "STOPPING → STOPPED → osac.resource.suspended.v1"
E2E test failure: OSAC-3434: Add VMaaS metering lifecycle E2E tests osac-test-infra#301

Assisted-by: Claude Code noreply@anthropic.com

coderabbitai · 2026-08-06T09:45:28Z

Walkthrough

The change classifies STARTING and STOPPING as transient states and DELETING as suspended. The watch consumer updates projections for transient states without emitting CloudEvents. Unknown states and correction reasons now return descriptive errors.

Changes

Compute Instance State Handling

Layer / File(s)	Summary
State mapping and validation `osac-metering/metering-service/internal/events/compute_instance.go`, `osac-metering/metering-service/internal/events/mapper.go`, `osac-metering/metering-service/internal/events/mapper_test.go`	The resolver returns `ErrTransientState` for `STARTING` and `STOPPING`, maps `DELETING` to a suspended event, preserves existing mappings, and reports unknown states.
Transient projection updates `osac-metering/metering-service/internal/watch/consumer.go`, `osac-metering/metering-service/internal/watch/consumer_test.go`	The consumer updates transient projection metadata without changing billing fields or publishing CloudEvents. Tests cover the `RUNNING` → `STOPPING` → `STOPPED` sequence.
Correction error description `osac-metering/metering-service/internal/reconciliation/correction.go`	Unknown correction reasons now include the reason in a formatted error description.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant WatchConsumer
  participant CloudEventType
  participant ProjectionStore
  participant CloudEventPublisher

  WatchConsumer->>CloudEventType: Resolve compute-instance state
  CloudEventType-->>WatchConsumer: Return ErrTransientState
  WatchConsumer->>ProjectionStore: Update transient projection metadata
  ProjectionStore-->>WatchConsumer: Persist projection
  WatchConsumer->>CloudEventPublisher: Publish terminal-state CloudEvent

Possibly related PRs

osac-project/osac#135: Extends the related compute-instance projection and event-mapping flow.
osac-project/osac#172: Overlaps in compute-instance state classification and error propagation.

Suggested reviewers: akshaynadkarni, danmanor, tzvatot

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

Ignore

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
No-Sensitive-Data-In-Logs	❌ Error	New transient-state logs include the customer resource identifier from mapper.ResourceID(); the new error path also propagates that identifier into the logged stream error.	Do not log raw resource IDs. Use a non-identifying correlation value or a documented, approved hash, and keep transient-state errors free of customer identifiers.

✅ Passed checks (10 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets	✅ Passed	The six changed files add no credential identifiers, private-key material, credential URLs, or secret-shaped encoded blobs; added literals are event/state names and error text.
No-Weak-Crypto	✅ Passed	The PR changes state mapping, errors, reconciliation text, and projection handling; added-line and import scans found no weak crypto, custom crypto, or secret/token comparisons.
No-Injection-Vectors	✅ Passed	The forward PR diff adds no SQL, shell, eval/exec, pickle, YAML, or dangerouslySetInnerHTML sink. Its only formatting call builds a CloudEvent description.
Container-Privileges	✅ Passed	The PR changes only Go source and tests; no container/Kubernetes manifests or added privilege settings such as privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEscalation were...
Ai-Attribution	✅ Passed	AI use is disclosed in the PR and both feature commits with `Assisted-by: Claude Code`; no `Co-Authored-By` trailer is present.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes preserving billing context through transient states and treating DELETING as billing-ending.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@osac-metering/metering-service/internal/watch/consumer_test.go`:
- Around line 713-781: The RUNNING→STOPPING→STOPPED test only verifies a
positive duration and does not prove BillableSince was preserved. Update the
test around “preserves billing context through RUNNING→STOPPING→STOPPED
sequence” to use fixed transition timestamps for the original billable start and
STOPPED event, then assert duration_seconds equals the full expected elapsed
duration from the original BillableSince to STOPPED rather than merely being
greater than zero.

In `@osac-metering/metering-service/internal/watch/consumer.go`:
- Around line 153-155: Remove mapper.ResourceID() and any customer resource
identifier from the log fields and error text in the logging paths around
handleTransientState and lines 196-224. Preserve the existing log behavior and
other non-sensitive context while ensuring no customer compute resource IDs are
emitted.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 614ba480-9cd8-49cd-9a02-a33175a2ac06

📥 Commits

Reviewing files that changed from the base of the PR and between 946697d and 9745102.

📒 Files selected for processing (5)

osac-metering/metering-service/internal/events/compute_instance.go
osac-metering/metering-service/internal/events/mapper.go
osac-metering/metering-service/internal/events/mapper_test.go
osac-metering/metering-service/internal/watch/consumer.go
osac-metering/metering-service/internal/watch/consumer_test.go

coderabbitai · 2026-08-06T09:47:43Z

+		if errors.Is(err, events.ErrTransientState) {
+			return c.handleTransientState(ctx, mapper, existing, version, currentState, transitionTime)
+		}


🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Remove customer resource identifiers from the new logs.

mapper.ResourceID() identifies the customer compute resource. Lines 216-224 write it to logs. Remove it from the log fields and error text.

Proposed fix

- return c.handleTransientState(ctx, mapper, existing, version, currentState, transitionTime) + return c.handleTransientState(ctx, existing, version, currentState, transitionTime) func (c *Consumer) handleTransientState( ctx context.Context, - mapper events.ResourceMapper, existing *projection.ResourceState, version int32, currentState string, @@ - c.logger.Info("stale version during transient state update, skipping", - "resource_id", mapper.ResourceID(), "state", currentState) + c.logger.Info("stale version during transient state update, skipping", + "state", currentState) return nil } - return fmt.Errorf("upserting transient state for %s: %w", mapper.ResourceID(), err) + return fmt.Errorf("upserting transient state: %w", err) } c.logger.V(1).Info("transient state updated (no CloudEvent)", - "resource_id", mapper.ResourceID(), "state", currentState) + "state", currentState)

As per coding guidelines, "Do not log passwords, tokens, API keys, PII ... or customer data."

Also applies to: 196-224

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@osac-metering/metering-service/internal/watch/consumer.go` around lines 153 - 155, Remove mapper.ResourceID() and any customer resource identifier from the log fields and error text in the logging paths around handleTransientState and lines 196-224. Preserve the existing log behavior and other non-sensitive context while ensuring no customer compute resource IDs are emitted.

Source: Coding guidelines

…TING/DELETING) Bug: When a RUNNING VM is stopped, the state machine goes RUNNING→STOPPING→STOPPED. The Watch Consumer was emitting updated.v1 for STOPPING (with duration_seconds) and suspended.v1 for STOPPED (with null duration_seconds and previous_state=STOPPING). The design says suspended.v1 should carry duration_seconds for billing closure. Root cause: resolveUpdatedEventType mapped STOPPING to updated.v1, causing the projection to overwrite IsBillable=false and BillableSince=nil at the STOPPING step. By the time STOPPED arrived, the billing context was gone. Fix: STOPPING, STARTING, and DELETING are now transient states that return ErrTransientState from CloudEventType. The Watch Consumer catches this error and updates only CurrentState + FulfillmentVersion in the projection — IsBillable and BillableSince are preserved. No CloudEvent is published for transient states. When the final state arrives (STOPPED, RUNNING, DELETED), the billing context is intact: buildStateContext sees IsBillable=true and computes duration_seconds correctly. The suspended.v1 event has previous_state=STOPPING (the actual previous state from the projection) and duration_seconds > 0 (the correct billing interval). Tests: - New: RUNNING→STOPPING→STOPPED two-step sequence through Watch Consumer — verifies no CloudEvent for STOPPING, suspended.v1 has duration_seconds > 0 - Updated: STOPPING/STARTING/DELETING now expect ErrTransientState instead of updated.v1 Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>

DELETING emits suspended.v1 to close the billing interval. Unlike STOPPING (which always leads to STOPPED where suspended.v1 fires), DELETING leads to OBJECT_DELETED (deleted.v1) which is audit-only. Making DELETING transient would lose billing closure for direct deletes (RUNNING → DELETING → DELETED). Also strengthens the RUNNING→STOPPING→STOPPED test to use fixed timestamps and assert exact duration_seconds (3600s), catching any corruption of BillableSince during transient state handling. Signed-off-by: omer-vishlitzky <ovishliz@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>

coderabbitai · 2026-08-06T10:57:27Z

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@osac-metering/metering-service/internal/events/mapper_test.go`:
- Around line 259-271: Update the unknown-state test around mapEvent to set
ci.PreviousState to a known state, then assert the returned error contains both
the previous state and the unknown current state values in addition to the
existing transition message. Keep the ErrTransientState assertion unchanged.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f8ad3806-c423-46ee-a988-763bc992fdf3

📥 Commits

Reviewing files that changed from the base of the PR and between b6da7f2 and a71bb89.

📒 Files selected for processing (6)

osac-metering/metering-service/internal/events/compute_instance.go
osac-metering/metering-service/internal/events/mapper.go
osac-metering/metering-service/internal/events/mapper_test.go
osac-metering/metering-service/internal/reconciliation/correction.go
osac-metering/metering-service/internal/watch/consumer.go
osac-metering/metering-service/internal/watch/consumer_test.go

🚧 Files skipped from review as they are similar to previous changes (4)

osac-metering/metering-service/internal/events/mapper.go
osac-metering/metering-service/internal/watch/consumer_test.go
osac-metering/metering-service/internal/events/compute_instance.go
osac-metering/metering-service/internal/watch/consumer.go

coderabbitai · 2026-08-06T10:59:31Z

+		It("returns error for unknown state (default branch)", func() {
+			ci.Status.State = privatev1.ComputeInstanceState(9999)

 			event := &privatev1.Event{
-				Id:      "evt-unspecified",
+				Id:      "evt-unknown-state",
 				Type:    privatev1.EventType_EVENT_TYPE_OBJECT_UPDATED,
 				Payload: &privatev1.Event_ComputeInstance{ComputeInstance: ci},
 			}

-			ce, err := mapEvent(event, &events.StateContext{})
-			Expect(err).NotTo(HaveOccurred())
-			Expect(ce.Type()).To(Equal("osac.resource.updated.v1"))
+			_, err := mapEvent(event, &events.StateContext{})
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("unexpected compute instance state transition"))
+			Expect(errors.Is(err, events.ErrTransientState)).To(BeFalse())


🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the complete unknown transition.

The test checks only the static error prefix. A generic error without the previous or unknown current state would still pass. Set a known PreviousState and assert that the error includes both transition values.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@osac-metering/metering-service/internal/events/mapper_test.go` around lines 259 - 271, Update the unknown-state test around mapEvent to set ci.PreviousState to a known state, then assert the returned error contains both the previous state and the unknown current state values in addition to the existing transition message. Keep the ErrTransientState assertion unchanged.

handleTransientState now updates only FulfillmentVersion and TransitionTime. CurrentState stays as-is (e.g., RUNNING) so when the final state arrives (STOPPED), previous_state=RUNNING on suspended.v1 — matching the design exactly. Signed-off-by: omer-vishlitzky <ovishliz@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>

openshift-ci-robot · 2026-08-06T11:19:17Z

@omer-vishlitzky: This pull request explicitly references no jira issue.

Details

In response to this:

Summary

Fixes a billing data integrity bug where transient states (STOPPING,
STARTING) destroyed billing context, causing suspended.v1 events to
have null duration_seconds and wrong previous_state.

Bug

When a RUNNING VM is stopped, the state machine goes:
RUNNING → STOPPING → STOPPED

The Watch Consumer emitted:

updated.v1 for STOPPING — overwrote IsBillable=false, BillableSince=nil

suspended.v1 for STOPPED — duration_seconds=null, previous_state=STOPPING

The design doc (line 341) says suspended.v1 should carry
duration_seconds for billing closure. Adapters watching for
suspended.v1 to close billing intervals got no duration.

Root cause

resolveUpdatedEventType mapped STOPPING to updated.v1. The
projection update overwrote IsBillable=false and BillableSince=nil.
When STOPPED arrived, billing context was gone.

Fix

STOPPING and STARTING now return ErrTransientState. The Watch
Consumer catches this and updates only FulfillmentVersion +
TransitionTime in the projection — CurrentState, IsBillable,
and BillableSince are all preserved. No CloudEvent is published.

When the final state arrives (STOPPED), the projection still shows
CurrentState=RUNNING, so suspended.v1 has previous_state=RUNNING
and correct duration_seconds. This matches the design exactly.

DELETING emits suspended.v1 (billing closure), same as
STOPPED/PAUSED/FAILED — it is a billing-ending state, not transient.

Unknown states return an error (fail fast). All supported state
transitions are explicit in the switch.

Before/After

Transition Before After

RUNNING→STOPPING updated.v1 (destroyed billing context) No event (projection: version+time only)

STOPPING→STOPPED suspended.v1 (duration=null, prev=STOPPING) suspended.v1 (duration=3600, prev=RUNNING)

RUNNING→DELETING updated.v1 (consumed billing context) suspended.v1 (duration=3600, billing closed)

Unknown state updated.v1 (silent) Error (fail fast)

Test plan

ginkgo run -r internal — 127 specs pass

RUNNING→STOPPING→STOPPED: no event for STOPPING,
suspended.v1 has previous_state=RUNNING and duration_seconds ≈ 3600
(fixed timestamps prove BillableSince preserved, not reset)

STOPPING/STARTING return ErrTransientState

DELETING emits suspended.v1

Unknown state (proto value 9999) returns error, not ErrTransientState

Pre-commit passes

References

Design doc line 341: "suspended.v1 — Billing interval closes with exact duration_seconds"

Design doc line 416: "STOPPING → STOPPED → osac.resource.suspended.v1"

E2E test failure: OSAC-3434: Add VMaaS metering lifecycle E2E tests osac-test-infra#301

Assisted-by: Claude Code noreply@anthropic.com

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2026-08-06T11:19:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: masayag, omer-vishlitzky

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [masayag,omer-vishlitzky]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

masayag · 2026-08-06T11:20:11Z

/lgtm

OSAC-4497: Admin bare metal instance type create and edit page

openshift-ci Bot requested review from akshaynadkarni and danmanor August 6, 2026 09:45

openshift-ci Bot added the approved label Aug 6, 2026

omer-vishlitzky had a problem deploying to e2e-test August 6, 2026 09:45 — with GitHub Actions Error

coderabbitai Bot requested changes Aug 6, 2026

View reviewed changes

omer-vishlitzky force-pushed the fix/metering-transient-state-billing branch from 9745102 to 552cf7c Compare August 6, 2026 09:52

omer-vishlitzky had a problem deploying to e2e-test August 6, 2026 09:53 — with GitHub Actions Error

omer-vishlitzky force-pushed the fix/metering-transient-state-billing branch from 552cf7c to 1c20a34 Compare August 6, 2026 09:54

omer-vishlitzky had a problem deploying to e2e-test August 6, 2026 09:55 — with GitHub Actions Error

omer-vishlitzky force-pushed the fix/metering-transient-state-billing branch from 1c20a34 to 2017848 Compare August 6, 2026 09:58

omer-vishlitzky had a problem deploying to e2e-test August 6, 2026 09:59 — with GitHub Actions Error

omer-vishlitzky had a problem deploying to e2e-test August 6, 2026 10:33 — with GitHub Actions Error

Merge branch 'main' into fix/metering-transient-state-billing

a71bb89

omer-vishlitzky had a problem deploying to e2e-test August 6, 2026 10:58 — with GitHub Actions Error

omer-vishlitzky changed the title ~~fix: preserve billing context through transient states (STOPPING/STARTING/DELETING)~~ fix: preserve billing context through transient states and treat DELETING as billing-ending Aug 6, 2026

coderabbitai Bot requested changes Aug 6, 2026

View reviewed changes

omer-vishlitzky temporarily deployed to e2e-test August 6, 2026 11:10 — with GitHub Actions Inactive

omer-vishlitzky changed the title ~~fix: preserve billing context through transient states and treat DELETING as billing-ending~~ NO-ISSUE: fix - preserve billing context through transient states and treat DELETING as billing-ending Aug 6, 2026

openshift-ci-robot added the jira/valid-reference label Aug 6, 2026

masayag approved these changes Aug 6, 2026

View reviewed changes

openshift-ci Bot assigned masayag Aug 6, 2026

openshift-ci Bot added the lgtm label Aug 6, 2026

omer-vishlitzky merged commit 846101b into osac-project:main Aug 6, 2026
24 of 25 checks passed

omer-vishlitzky mentioned this pull request Aug 6, 2026

OSAC-3410: Add CaaS lifecycle metering with N+1 per-component billing #172

Merged

8 of 9 tasks

eliorerz pushed a commit that referenced this pull request Sep 18, 2026

Merge pull request #176 from rawagner/OSAC_4497

f6cd9c2

OSAC-4497: Admin bare metal instance type create and edit page

Conversation

omer-vishlitzky commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Bug

Root cause

Fix

Before/After

Test plan

References

Uh oh!

coderabbitai Bot commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Pre-merge checks failed

❌ Failed checks (1 error)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Aug 6, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Aug 6, 2026

Summary

Bug

Root cause

Fix

Before/After

Test plan

References

Uh oh!

openshift-ci Bot commented Aug 6, 2026

Uh oh!

masayag commented Aug 6, 2026

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

omer-vishlitzky commented Aug 6, 2026 •

edited

Loading

coderabbitai Bot commented Aug 6, 2026 •

edited

Loading