[codex] Force async advancement for invoice collection#4626
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThis PR adds a ChangesForceAsyncAdvance feature flag propagation
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Collector as Collect Worker / SubscriptionSync
participant BillingService as billing.Service
participant StateMachine as Invoice State Machine
participant EventBus as Event Publisher
Collector->>BillingService: InvoicePendingLines(ForceAsyncAdvance: true)
BillingService->>BillingService: CreateStandardInvoiceFromGatheringLines(ForceAsyncAdvance)
BillingService->>StateMachine: advanceUntilStateStable(sm, forceAsync=true)
alt forceAsync or QueuedAdvancementStrategy
StateMachine->>EventBus: publish AdvanceStandardInvoiceEvent
else synchronous strategy
StateMachine->>StateMachine: activate & validate state machine
end
BillingService-->>Collector: draft invoice(s)
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryThis PR adds forced async advancement for invoice pending-line collection. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "Force async advancement for invoice coll..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
turip
force-pushed
the
feat/force-async-advance
branch
from
16820a7 to
d092e11
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/config/billing.go (1)
54-54: 🩺 Stability & Availability | 🔵 TrivialHeads up: shipping this default as
trueneeds solid confidence.This flips subscription-sync's invoice pending-lines advancement to async by default for everyone, not just an opt-in cohort. The PR description mentions the
test/billingintegration test wasn't actually run locally (no Postgres/Docker), so this default is going out without the full end-to-end validation that would normally cover the state-machine transitions it affects.Worth double-checking CI actually ran the
test/billingsuite (with-tags=dynamic) before merge, given this touches the invoicing critical path by default.🤖 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 `@app/config/billing.go` at line 54, The billing config default for subscription-sync is being switched to async for all users, so verify the change is backed by full end-to-end coverage before keeping it on by default. Recheck that the `test/billing` integration suite with `-tags=dynamic` actually ran in CI, and if it did not, gate the new default behind a safer rollout mechanism or revert the `billing.featureSwitches.subscriptionSyncForceAsyncAdvance` default in `config/billing.go` until the state-machine transitions are validated.openmeter/billing/invoice.go (1)
457-463: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a short doc comment on
ForceAsyncAdvance.The field itself is fine, but its effect isn't obvious from the name alone: setting it
truecauses the invoice to be left indraft.createdand advancement to be deferred to an async published event (seeadvanceUntilStateStableinopenmeter/billing/service/invoice.go) rather than happening synchronously in this call. A one-liner here (and on the mirrored field instdinvoice.go) would help future readers avoid surprises.As per coding guidelines: "Add docstrings to domain helpers when the name compresses important business semantics, and describe the observable business contract rather than implementation mechanics."
✏️ Suggested doc comment
IncludePendingLines mo.Option[[]string] AsOf *time.Time + // ForceAsyncAdvance defers invoice advancement to the async AdvanceStandardInvoiceEvent + // flow instead of advancing synchronously in this call; the returned invoice(s) will + // remain in draft.created until the async advancement is processed. ForceAsyncAdvance bool🤖 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 `@openmeter/billing/invoice.go` around lines 457 - 463, Add a short doc comment for ForceAsyncAdvance in InvoicePendingLinesInput, and mirror the same comment on the equivalent field in stdinvoice.go. Clarify the observable business behavior: when true, the invoice stays in draft.created and advancement is deferred to an async published event instead of completing synchronously in the current call. Use the field names ForceAsyncAdvance and InvoicePendingLinesInput (and the mirrored stdinvoice struct) to locate the spots.Source: Coding guidelines
openmeter/billing/worker/collect/collect.go (1)
105-114: 🩺 Stability & Availability | 🔵 TrivialOperational note: collection now always relies on the async advancement consumer.
Forcing
ForceAsyncAdvance: trueunconditionally here (unlike the subscription-sync path, which is gated bySubscriptionSyncForceAsyncAdvance) means every collected invoice now depends on theAdvanceStandardInvoiceEventconsumer actually running to leavedraft.created. Worth confirming that consumer's throughput/alerting is solid, since a stuck or lagging consumer would now silently stall all collection-created invoices with no config-level fallback to synchronous advancement.🤖 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 `@openmeter/billing/worker/collect/collect.go` around lines 105 - 114, The collect flow in collect.go is now hard-wired to async advancement via InvoicePendingLines with ForceAsyncAdvance set to true, so collected invoices fully depend on the AdvanceStandardInvoiceEvent consumer. Update this path to follow the same guarded behavior as the subscription-sync flow, using a configurable or fallback mechanism instead of always forcing async advancement, and keep the existing billing.WithPartialInvoiceLinesDisabled usage intact while adjusting the collect logic around InvoicePendingLines and params handling.
🤖 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.
Nitpick comments:
In `@app/config/billing.go`:
- Line 54: The billing config default for subscription-sync is being switched to
async for all users, so verify the change is backed by full end-to-end coverage
before keeping it on by default. Recheck that the `test/billing` integration
suite with `-tags=dynamic` actually ran in CI, and if it did not, gate the new
default behind a safer rollout mechanism or revert the
`billing.featureSwitches.subscriptionSyncForceAsyncAdvance` default in
`config/billing.go` until the state-machine transitions are validated.
In `@openmeter/billing/invoice.go`:
- Around line 457-463: Add a short doc comment for ForceAsyncAdvance in
InvoicePendingLinesInput, and mirror the same comment on the equivalent field in
stdinvoice.go. Clarify the observable business behavior: when true, the invoice
stays in draft.created and advancement is deferred to an async published event
instead of completing synchronously in the current call. Use the field names
ForceAsyncAdvance and InvoicePendingLinesInput (and the mirrored stdinvoice
struct) to locate the spots.
In `@openmeter/billing/worker/collect/collect.go`:
- Around line 105-114: The collect flow in collect.go is now hard-wired to async
advancement via InvoicePendingLines with ForceAsyncAdvance set to true, so
collected invoices fully depend on the AdvanceStandardInvoiceEvent consumer.
Update this path to follow the same guarded behavior as the subscription-sync
flow, using a configurable or fallback mechanism instead of always forcing async
advancement, and keep the existing billing.WithPartialInvoiceLinesDisabled usage
intact while adjusting the collect logic around InvoicePendingLines and params
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 784de61f-883a-4803-b9dd-edc2b56d765e
📒 Files selected for processing (14)
app/common/billing.goapp/config/billing.goapp/config/config_test.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.goconfig.example.yamlopenmeter/billing/invoice.goopenmeter/billing/service/gatheringinvoicependinglines.goopenmeter/billing/service/invoice.goopenmeter/billing/stdinvoice.goopenmeter/billing/worker/collect/collect.goopenmeter/billing/worker/subscriptionsync/service/service.goopenmeter/billing/worker/subscriptionsync/service/sync.gotest/billing/invoice_test.go
2 participants
Summary
Validation
Note: the focused billing integration test was not run because local Postgres/Docker was unavailable in this environment.
Summary by CodeRabbit
New Features
Tests