fix(api): keep open usage periods in step with their boxes - #1104
fix(api): keep open usage periods in step with their boxes#1104ltstriker wants to merge 1 commit into
Conversation
📦 BoxLite review — couldn't completepowered by BoxLite |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughUsage billing now derives open-period shapes from box state. Rollover refreshes resources from current box data. A locked reconciliation job repairs missing, stale, and orphaned periods across runner shards. Tests cover lifecycle states, drift, locking, grace windows, metrics, and idempotency. ChangesUsage period reconciliation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UsageService
participant RunnerRepository
participant BoxRepository
participant UsagePeriodRepository
UsageService->>RunnerRepository: scan runner shards
UsageService->>BoxRepository: load current box state
UsageService->>UsagePeriodRepository: identify missing, stale, or orphaned periods
UsageService->>UsagePeriodRepository: repair periods under locks and transactions
Possibly related issues
Possibly related PRs
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 |
End to end, in one pictureThe two passes stay side by side because their blind spots are opposite:
|
Design challenge — duplication, performance, styleBare line numbers are Duplication1. The two passes disagree about which boxes are in scope. 2. The state rule is written twice. 3. "Agrees with the box" means three different things. 4. Two ways to close a period, inside one method. 5. Performance6. Eight of the nine selected columns are never read. 7. The lock TTL equals the cron interval — no margin. 8. The cap is per shard, not per pass. 9. One query per runner even when nothing is wrong. 10. Style11. 12. 13. 14. "provably too short" ( 15. Holds up under challenge
If only four things change
|
f983773 to
2227901
Compare
a98687e to
5fa3c43
Compare
7dcddd3 to
0fc7290
Compare
0fc7290 to
7dcddd3
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens the API’s usage ledger so a box’s open usage period stays consistent with the box’s current state even when fire-and-forget state events are missed (process crash/throw mid-transition).
Changes:
- Centralizes the state → billable-period rule in
expectedOpenPeriod(box)and reuses it across event handling, daily roll-over, and reconciliation. - Updates the daily roll-over to re-derive billed resources from the current box row instead of copying forward from the closing period.
- Adds a 5-minute reconcile cron pass that scans from the box side (sharded by runner) to repair missing/orphan/stale open periods, with integration coverage.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/src/usage/usage.module.ts | Registers Runner with TypeORM so the usage service can shard reconciliation by runner. |
| apps/api/src/usage/usage.module.spec.ts | Updates module wiring assertions for the new Runner repository dependency. |
| apps/api/src/usage/services/usage.service.ts | Adds expectedOpenPeriod usage, fixes roll-over drift, and introduces the reconcile cron pass + drift metrics. |
| apps/api/src/usage/services/usage.service.spec.ts | Updates unit test scaffolding to account for the new runner repository dependency. |
| apps/api/src/usage/services/usage.service.integration.spec.ts | Expands integration coverage to validate reconcile + updated roll-over behavior against real Postgres/Redis. |
| apps/api/src/usage/services/expected-usage-period.ts | Introduces the shared “expected open period” rule and shape comparison helper. |
| apps/api/src/usage/services/expected-usage-period.spec.ts | Adds unit coverage asserting state exhaustiveness and intended divergence from quota semantics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
apps/api/src/usage/services/usage.service.ts (3)
400-444: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe state rule now lives in two places, which the SQL can silently narrow.
findDriftCandidatesencodes the state-to-shape rule in SQL.repairDriftre-derives it throughexpectedOpenPeriod. A state added toexpectedOpenPeriodbut not to the SQL predicate produces a box that is never selected and therefore never repaired. The failure mode is silent under-billing, not an error.The integration spec pins the two together for every
BoxState, which covers the current states. Consider extracting the predicate into one builder next toexpectedOpenPeriodso the SQL cannot drift on its own.Also note that
boxcan benullat Line 406 when the box is deleted between the scan and the repair. Theexpected === nullbranch handles that case correctly, so no guard is required today, but the dependency is implicit.🤖 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 `@apps/api/src/usage/services/usage.service.ts` around lines 400 - 444, Consolidate the state-to-shape rule used by findDriftCandidates and repairDrift into a shared predicate or builder located beside expectedOpenPeriod, then reuse it for the SQL filtering and repair logic so every BoxState remains covered consistently. Preserve the existing expected === null handling for deleted boxes and avoid adding a separate guard.
55-66: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSelect only the box id, and drop the unused candidate columns.
repairDriftusescandidate.box_idonly. It re-reads the box throughboxRepository.findOneand re-derives the shape. The other eight columns are transferred for every candidate and never read. TheDriftCandidateinterface also uses snake_case fields, which does not match the rest of the codebase.Reduce the projection to
b.idand type the result as{ box_id: string }[], or alias the column toboxId.Also applies to: 343-353
🤖 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 `@apps/api/src/usage/services/usage.service.ts` around lines 55 - 66, Update the drift-candidate query and DriftCandidate type used by repairDrift to select and retain only the box identifier, using the codebase’s naming convention (prefer an alias such as boxId, or consistently type the result as { box_id: string }[]). Remove the unused state, resource, organization, region, and period fields from the projection and interface.
50-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCache the counter lazily, without changing the meter name.
recordDriftcallscreateCounteron every repair. Cache the counter after first use. Keepmetrics.getMeter(''); existing TypeScript instrumentation uses the empty meter name. Do not create the counter at module scope becauseAppModuleloads beforeotelSdk.start().🤖 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 `@apps/api/src/usage/services/usage.service.ts` around lines 50 - 53, Update getDriftCounter to lazily cache and reuse the counter after its first creation, while preserving metrics.getMeter('') and delaying initialization until the function is called rather than creating it at module scope; ensure recordDrift continues using this cached counter.apps/api/src/box/services/box.service.start-reconciliation.spec.ts (2)
45-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for a box with no
runnerId.
findStalledStartupJobreturnsnullwhenbox.runnerIdis unset. No test covers that branch. A box inCREATINGwithout a runner must not query the job repository and must not change state.🤖 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 `@apps/api/src/box/services/box.service.start-reconciliation.spec.ts` around lines 45 - 65, Add a test for the start-reconciliation flow using a CREATING box whose runnerId is unset. Assert that findStalledStartupJob returns null without calling the job repository, and that the box remains unchanged with no updateJobStatus or updateWhere calls.
81-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe assertion does not cover the stall window.
expect.objectContainingomitsstartedAt. A regression that drops theLessThan(claimedBefore)predicate, or that ignoresboxSync.startConfirmationStallSeconds, still passes this test. The stall window is the core rule of this feature. Assert thestartedAtbound.💚 Add the stall-window assertion
expect(jobFindOne).toHaveBeenCalledWith( expect.objectContaining({ where: expect.objectContaining({ runnerId: 'runner-1', resourceType: ResourceType.BOX, resourceId: box.id, type: JobType.CREATE_BOX, status: JobStatus.IN_PROGRESS, + startedAt: LessThan(expect.any(Date)), }), }), ) + + const claimedBefore = jobFindOne.mock.calls[0][0].where.startedAt.value as Date + expect(Date.now() - claimedBefore.getTime()).toBeGreaterThanOrEqual(STALL_SECONDS * 1000)Import
LessThanfromtypeorm.🤖 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 `@apps/api/src/box/services/box.service.start-reconciliation.spec.ts` around lines 81 - 91, Update the job query assertion in the reconciliation test to include the startedAt stall-window predicate, using TypeORM’s LessThan with a bound derived from boxSync.startConfirmationStallSeconds. Import LessThan from typeorm and assert it alongside runnerId, resourceType, resourceId, type, and status so regressions removing or misconfiguring the bound fail.apps/api/src/box/services/job.service.transaction.spec.ts (1)
41-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe commit-ordering assertion can be swallowed.
updateJobStatuscallshandleJobCompletion(...).catch(...)and logs the error. If theexpectinside this mock throws, the service catches it and the test still passes. Record the flag instead and assert it after the call.♻️ Record the ordering, then assert
+ let committedAtCompletion: boolean | null = null const jobStateHandlerService = { handleJobCompletion: jest.fn(() => { - expect(transactionCommitted).toBe(true) + committedAtCompletion = transactionCommitted return Promise.resolve() }), } const service = new JobService(jobRepository as any, {} as any, jobStateHandlerService as any) - return { entityManager, jobRepository, jobStateHandlerService, service } + return { entityManager, jobRepository, jobStateHandlerService, service, committedAtCompletion: () => committedAtCompletion }Then assert
expect(committedAtCompletion()).toBe(true)in the first test.🤖 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 `@apps/api/src/box/services/job.service.transaction.spec.ts` around lines 41 - 46, Update the jobStateHandlerService.handleJobCompletion mock to record transactionCommitted through a flag or committedAtCompletion helper instead of asserting inside the callback, then assert that recorded value after updateJobStatus completes in the first test. This keeps the commit-ordering assertion outside the service’s caught error path.apps/api/src/box/services/job.service.claim.spec.ts (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
returningCalledis never asserted.The helper tracks and exposes
returningCalled, but no test reads it. Either assert thatreturningwas called in the first test, or remove the tracking.Also applies to: 44-47, 71-71
🤖 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 `@apps/api/src/box/services/job.service.claim.spec.ts` at line 31, Update the tests using the returningCalled helper to assert that returning was invoked in the relevant first test, or remove the unused tracking variable and assignments if that behavior is not under test. Ensure no unasserted returningCalled state remains.apps/api/src/box/services/job.service.ts (1)
489-511: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider one conditional UPDATE for the whole batch.
The loop issues one round trip per job. With the default
limitof 10 each poll costs up to 10 sequential statements, and every poll from every runner pays this. A single statement keeps the same compare-and-swap semantics:♻️ Batch the claim
- for (const job of jobs) { - const claim = await this.jobRepository - .createQueryBuilder() - .update(Job) - .set({ status: JobStatus.IN_PROGRESS, startedAt: now, updatedAt: now }) - .where('id = :id', { id: job.id }) - .andWhere('status = :pending', { pending: JobStatus.PENDING }) - .returning('*') - .execute() - - const claimedRow = (claim.raw as Job[])[0] - if (!claim.affected || !claimedRow) { - this.logger.debug(`Job ${job.id} was already claimed by a concurrent poll`) - continue - } - - claimedJobs.push(new JobDto(this.jobRepository.create(claimedRow))) - } + const claim = await this.jobRepository + .createQueryBuilder() + .update(Job) + .set({ status: JobStatus.IN_PROGRESS, startedAt: now, updatedAt: now }) + .where('id IN (:...ids)', { ids: jobs.map((job) => job.id) }) + .andWhere('status = :pending', { pending: JobStatus.PENDING }) + .returning('*') + .execute() + + for (const row of (claim.raw as Job[]) ?? []) { + claimedJobs.push(new JobDto(this.jobRepository.create(row))) + }🤖 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 `@apps/api/src/box/services/job.service.ts` around lines 489 - 511, Replace the per-job update loop in the job polling method with one batch conditional UPDATE that claims all eligible jobs in a single round trip, preserving the pending-status compare-and-swap semantics and limiting claims to the requested batch. Build JobDto instances from the returned rows, hydrating each through this.jobRepository.create before adding them to claimedJobs; retain concurrency-safe handling for rows not claimed.
🤖 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 `@apps/api/src/box/services/box.service.ts`:
- Around line 1323-1347: Serialize stalled-startup reconciliation per box around
findStalledStartupJob and updateJobStatus, using an ownership-safe box lock so
concurrent runner callbacks cannot complete the same job or release another
operation’s lock. Ensure only the lock holder proceeds and always releases only
its own lock, preserving the existing completion-handler flow.
In `@apps/api/src/config/configuration.ts`:
- Around line 336-339: Validate BOX_SYNC_START_CONFIRMATION_STALL_SECONDS in the
configuration before assigning startConfirmationStallSeconds, falling back to 60
when the value is non-numeric, zero, or negative. Reuse an existing shared
numeric environment helper if available, and preserve the parsed positive value
for valid input.
In `@apps/api/src/usage/services/usage.service.ts`:
- Around line 293-328: Prevent overlapping reconciliation passes in
reconcileUsagePeriods by ensuring the lock remains valid for the entire
shard-and-repair loop: either extend the lock TTL beyond the worst-case runtime
or add a deadline check that stops processing before expiration. Preserve the
existing unlock behavior and shard processing for work completed within the
valid lock period.
- Around line 338-363: Update findDriftCandidates to exclude boxes whose
organization is BOX_WARM_POOL_UNASSIGNED_ORGANIZATION, matching the exclusion
used by closeAndReopenUsagePeriods. Add this predicate to the query so
repairDrift cannot create billable periods for warm-pool boxes.
In `@apps/runner/pkg/boxlite/client.go`:
- Around line 176-178: Update NewClient’s home-directory resolution so
Client.homeDir always matches the directory passed to boxlite.WithHomeDir:
either propagate the os.UserHomeDir() resolution error, or store the exact
fallback directory selected by NewRuntime. Ensure BoxSyncService can use the
populated Client.homeDir to run readStartedRecord during startup reconciliation.
---
Nitpick comments:
In `@apps/api/src/box/services/box.service.start-reconciliation.spec.ts`:
- Around line 45-65: Add a test for the start-reconciliation flow using a
CREATING box whose runnerId is unset. Assert that findStalledStartupJob returns
null without calling the job repository, and that the box remains unchanged with
no updateJobStatus or updateWhere calls.
- Around line 81-91: Update the job query assertion in the reconciliation test
to include the startedAt stall-window predicate, using TypeORM’s LessThan with a
bound derived from boxSync.startConfirmationStallSeconds. Import LessThan from
typeorm and assert it alongside runnerId, resourceType, resourceId, type, and
status so regressions removing or misconfiguring the bound fail.
In `@apps/api/src/box/services/job.service.claim.spec.ts`:
- Line 31: Update the tests using the returningCalled helper to assert that
returning was invoked in the relevant first test, or remove the unused tracking
variable and assignments if that behavior is not under test. Ensure no
unasserted returningCalled state remains.
In `@apps/api/src/box/services/job.service.transaction.spec.ts`:
- Around line 41-46: Update the jobStateHandlerService.handleJobCompletion mock
to record transactionCommitted through a flag or committedAtCompletion helper
instead of asserting inside the callback, then assert that recorded value after
updateJobStatus completes in the first test. This keeps the commit-ordering
assertion outside the service’s caught error path.
In `@apps/api/src/box/services/job.service.ts`:
- Around line 489-511: Replace the per-job update loop in the job polling method
with one batch conditional UPDATE that claims all eligible jobs in a single
round trip, preserving the pending-status compare-and-swap semantics and
limiting claims to the requested batch. Build JobDto instances from the returned
rows, hydrating each through this.jobRepository.create before adding them to
claimedJobs; retain concurrency-safe handling for rows not claimed.
In `@apps/api/src/usage/services/usage.service.ts`:
- Around line 400-444: Consolidate the state-to-shape rule used by
findDriftCandidates and repairDrift into a shared predicate or builder located
beside expectedOpenPeriod, then reuse it for the SQL filtering and repair logic
so every BoxState remains covered consistently. Preserve the existing expected
=== null handling for deleted boxes and avoid adding a separate guard.
- Around line 55-66: Update the drift-candidate query and DriftCandidate type
used by repairDrift to select and retain only the box identifier, using the
codebase’s naming convention (prefer an alias such as boxId, or consistently
type the result as { box_id: string }[]). Remove the unused state, resource,
organization, region, and period fields from the projection and interface.
- Around line 50-53: Update getDriftCounter to lazily cache and reuse the
counter after its first creation, while preserving metrics.getMeter('') and
delaying initialization until the function is called rather than creating it at
module scope; ensure recordDrift continues using this cached counter.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: febbaac1-d922-4178-9ae9-325de286285c
📒 Files selected for processing (20)
apps/api/src/box/services/box.service.spec.tsapps/api/src/box/services/box.service.start-reconciliation.spec.tsapps/api/src/box/services/box.service.tsapps/api/src/box/services/job.service.claim.spec.tsapps/api/src/box/services/job.service.transaction.spec.tsapps/api/src/box/services/job.service.tsapps/api/src/config/configuration.tsapps/api/src/usage/services/expected-usage-period.spec.tsapps/api/src/usage/services/expected-usage-period.tsapps/api/src/usage/services/usage.service.integration.spec.tsapps/api/src/usage/services/usage.service.spec.tsapps/api/src/usage/services/usage.service.tsapps/api/src/usage/usage.module.tsapps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/create_invariant_test.goapps/runner/pkg/services/box_sync.goapps/runner/pkg/services/box_sync_test.gosrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/runtime/layout.rssrc/boxlite/tests/container_start_record.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
apps/api/src/usage/services/usage.service.ts:53
getDriftCounter()currently callscreateCounter()every time drift is recorded, which can register duplicate instruments and add unnecessary overhead on hot paths. Cache the Counter instance so repeated repairs just calladd()on the same instrument.
const getDriftCounter = () =>
metrics.getMeter('').createCounter('usage_period_drift_repaired', {
description: 'Open usage periods brought back in step with the box they bill for',
})
apps/api/src/usage/services/usage.service.ts:283
- The PR description says the diff here should only include the usage files (stacked on #1091), but this PR also contains non-usage changes (BoxLite Rust runtime/tests, runner BoxSync, JobService concurrency). Please retarget/rebase or update the PR description/scope so reviewers know what they are approving.
/**
* Brings open periods back in step with the boxes they bill for.
*
* The ledger is maintained by in-process events, which are fire-and-forget: a
* handler that dies, throws, or loses its process leaves the box and its period
7dcddd3 to
0fc7290
Compare
|
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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/usage/services/usage.service.integration.spec.ts (1)
477-494: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe parity test pins only one direction.
it.eachalways starts from a box with no open period. It therefore checks that the SQL predicate creates the shapeexpectedOpenPeriodreturns. It never checks the opposite direction: a box that already holds an open period whose state maps tonull. The test at lines 443-456 shows thatSTARTINGdiverges in exactly that direction. Consider adding a secondit.eachthat seeds an open period first and asserts closure whenexpectedOpenPeriodreturnsnull, withSTARTINGand other in-flight states excluded explicitly.🤖 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 `@apps/api/src/usage/services/usage.service.integration.spec.ts` around lines 477 - 494, Extend the parity coverage near the existing “agrees with expectedOpenPeriod” test with a second it.each that seeds an open period before reconciliation, then asserts it is closed when expectedOpenPeriod({ ...box, state }) returns null. Exclude STARTING and the other in-flight states explicitly, while preserving the current test’s coverage for creating open periods from an initially closed box.
🤖 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 `@apps/api/src/usage/services/usage.service.integration.spec.ts`:
- Around line 477-494: Extend the parity coverage near the existing “agrees with
expectedOpenPeriod” test with a second it.each that seeds an open period before
reconciliation, then asserts it is closed when expectedOpenPeriod({ ...box,
state }) returns null. Exclude STARTING and the other in-flight states
explicitly, while preserving the current test’s coverage for creating open
periods from an initially closed box.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a9cc6dbe-314a-4210-9e3b-96bf2f4ddf45
📒 Files selected for processing (7)
apps/api/src/usage/services/expected-usage-period.spec.tsapps/api/src/usage/services/expected-usage-period.tsapps/api/src/usage/services/usage.service.integration.spec.tsapps/api/src/usage/services/usage.service.spec.tsapps/api/src/usage/services/usage.service.tsapps/api/src/usage/usage.module.spec.tsapps/api/src/usage/usage.module.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/api/src/usage/services/usage.service.spec.ts
- apps/api/src/usage/usage.module.ts
- apps/api/src/usage/services/expected-usage-period.spec.ts
- apps/api/src/usage/services/expected-usage-period.ts
- apps/api/src/usage/services/usage.service.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
apps/api/src/usage/services/usage.service.ts:451
recordDrift()logs a WARN per repaired box. With up toRECONCILE_BATCH_SIZErepairs per shard and many shards per run, this can generate very high log volume during catch-up, impacting logging/alerting noise. Consider logging per-run summaries at WARN/INFO and keeping per-box detail at DEBUG.
private recordDrift(kind: 'missing' | 'orphan' | 'stale_shape', boxId: string): void {
getDriftCounter().add(1, { kind })
this.logger.warn(`Repaired ${kind} usage period drift for box ${boxId}`)
}
apps/api/src/usage/services/usage.service.ts:302
- The cron-level Redis lock for
reconcileUsagePeriods()is set to 300s, but the work per run scales with the number of runner shards (up toRECONCILE_BATCH_SIZErepairs per shard). If the run exceeds 300s, the lock can expire mid-run and another instance can start overlapping reconciliation, increasing contention and potentially duplicating repairs.
This issue also appears on line 448 of the same file.
async reconcileUsagePeriods() {
const lockKey = 'reconcile-usage-periods'
if (!(await this.redisLockProvider.lock(lockKey, 300))) {
return
apps/api/src/usage/services/usage.service.ts:404
- The per-box lock helper is named
aquireLock(missing the second 'c'), and the new reconciliation path calls it. This typo makes the locking API harder to scan/search and easy to retype incorrectly; consider renaming toacquireLockand updating call sites + the helper definition.
private async repairDrift(candidate: DriftCandidate): Promise<void> {
if (!(await this.aquireLock(candidate.box_id))) {
return
0fc7290 to
ba64941
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
apps/api/src/usage/services/usage.service.ts:54
getDriftCounter()creates a new OpenTelemetry counter on every call. In OTel, instruments are expected to be created once and reused; repeatedly creating them can add unnecessary overhead and may lead to duplicate-instrument behavior depending on the SDK/exporter. Memoize the counter instance and only callcreateCounterthe first time.
const getDriftCounter = () =>
metrics.getMeter('').createCounter('usage_period_drift_repaired', {
description: 'Open usage periods brought back in step with the box they bill for',
})
apps/api/src/usage/services/expected-usage-period.ts:51
- This comment says cpu/gpu/mem/disk are "double precision on both sides", but the box resource columns are currently
int(seeBoxentity) while usage periods are stored as floats. The rationale forRESOURCE_EPSILONstill makes sense (JS numbers + float storage for periods), but the wording is misleading; tightening it will prevent future confusion about which side can introduce rounding noise.
// cpu/gpu/mem/disk are double precision on both sides, so a figure that has
// round-tripped through Postgres can differ from the box's by float noise.
// Without a tolerance the reconcile pass would rewrite an already-correct
// period on every run, fragmenting the ledger into unbillable slivers.
The ledger is maintained by fire-and-forget in-process events, so a handler that throws or loses its process leaves a box and its open period disagreeing, with nothing to notice. Two gaps followed: - The daily roll-over copied the closing period's resources into the new one, so drift was re-copied every day: a period charging no cpu for a running box stayed wrong forever, and a disk resize that landed while the box was stopped never reached the ledger at all. - Nothing scanned from the box side. The roll-over walks the period table, so a box that never got a period was invisible to it. `expectedOpenPeriod` becomes the single source of truth for the state -> period rule, shared by the event handler, the roll-over and a new reconcile pass. The roll-over now re-derives resources from the box instead of copying them forward. The reconcile pass scans boxes per runner shard every five minutes, re-checks each candidate under the per-box lock, and repairs missing, stale and orphaned periods, counting each repair on `usage_period_drift_repaired`. It deliberately does not reuse `BOX_STATES_CONSUMING_COMPUTE`: quota counts CREATING and STARTING because the runner has pinned the resources, while billing does not charge for a box the tenant cannot use yet. Corrections start now and are never backdated — the window a box spent mis-billed cannot be reconstructed, and guessing it would replace a known gap with an invented charge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ba64941 to
3a17efd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
apps/api/src/usage/services/usage.service.ts:462
- In the drift repair transaction,
open.endAtis set withnew Date(), thencreateUsagePeriodsetsstartAtwith a separatenew Date(). That can introduce a small gap/overlap between periods (unlike the roll-over path which uses a singlecloseTime). Capture one timestamp and reuse it for both the close and the reopened period’sstartAtto keep the ledger contiguous.
open.endAt = new Date()
await transactionalEntityManager.save(open)
}
signal.throwIfAborted()
await this.createUsagePeriod(box, expected, transactionalEntityManager)
apps/api/src/usage/services/expected-usage-period.ts:39
- This comment says RESTORING is a state “no writer in this repository ever assigns”, but the runner adapter converts runner state
BoxStateRestoringintoBoxState.RESTORING(apps/api/src/box/runner-adapter/runnerAdapter.v0.ts:126–148). Please adjust the comment so it only claims “no writer” for the states that are truly unassigned (e.g. ARCHIVING/RESIZING) while keeping RESTORING in the “bills nothing” group.
// Every remaining state is in neither list and bills nothing, the same answer a
// terminal box gets: CREATING, STARTING and UNKNOWN, plus RESTORING, RESIZING
// and ARCHIVING, which no writer in this repository ever assigns.
Problem
A box's open usage period is maintained by in-process events, which are
fire-and-forget. A handler that throws, or whose process dies mid-transition,
leaves the box and its period disagreeing and nothing notices. Two gaps let that
disagreement persist indefinitely:
The daily roll-over preserved drift instead of correcting it. It copied the
closing period's resources into the replacement, so a period charging no cpu for
a running box was re-copied every day, forever. A disk resize that landed while
the box was stopped never reached the ledger at all.
Nothing ever scanned from the box side. The roll-over walks the period
table, so a box that never got a period is invisible to it — there is no row to
find. Its one-day cutoff also lets a wrong period bill for a further day.
Approach
expectedOpenPeriod(box)becomes the single source of truth for thestate → period rule — full compute while running, disk alone once stopped,
nothing otherwise — and the event handler, the roll-over and the new reconcile
pass all answer the question the same way.
Roll-over re-derives resources from the box rather than copying them
forward. A box that is gone or terminal yields no shape and so is not reopened,
which is what stops a deleted box from accruing.
Reconcile pass (every 5 min) scans from the box side, which is why both are
needed — their blind spots are opposite. The roll-over is the only thing that
can see a period whose box row was deleted outright; this is the only thing that
can see a box that never got a period.
box_runnerid_idxrather than readingthe box table end to end. Measured on 20k boxes across 50 runners: bitmap
index scan, ~400 rows rechecked, ~1.7 ms. The trailing
runnerId IS NULLshard is not optional — a box that reached DESTROYED or ARCHIVED has had its
runnerId cleared, and those are exactly the periods that must be closed.
60 s, so a handler can legitimately take a full minute to reach the ledger;
reconciling inside that window would race it and collide on the
one-open-period index. Anything under 60 s is provably too short.
expectedOpenPeriod, so one the event handler fixed in the meantime is leftalone. The SQL is a deliberately wide filter, not the authority.
usage_period_drift_repaired{kind=missing|orphan|stale_shape}.Decisions worth challenging
BOX_STATES_CONSUMING_COMPUTEcounts CREATING and STARTING because the runner has already pinned the
resources; billing does not charge for a box the tenant cannot use yet.
Divergence here is a pricing decision, not a bug —
expected-usage-period.spec.tsasserts it so nobody "fixes" it by accident.
mis-billed cannot be reconstructed (its
updatedAthas moved on for unrelatedreasons), and guessing it would replace a known gap with an invented charge.
p.cpu <> b.cpuispermanently true for every stopped box — a stopped period should charge no
cpu — and those false positives would fill each page and starve real drift out
of the batch forever.
precision on both sides; without
RESOURCE_EPSILONthe pass would rewrite analready-correct period on every run, fragmenting the ledger into unbillable
slivers.
Pricing a state the product does not have yet would be inventing a rule.
No database migration, no API surface change, no client regeneration.
Verification
Ran against a real Postgres 16 + Redis (
DB_*/REDIS_*pointed at a disposabledatabase; the integration spec builds its schema by running the migrations, so
it exercises the DDL that ships, and skips when no database is reachable).
tsc -p api/tsconfig.spec.json --noEmiteslint api/src/usage --max-warnings=0prettier --checkTwo-side verified. With the roll-over hunk reverted and everything else
intact, three roll-over assertions fail on the bug itself — a running box's
rolled-over period comes back
cpu: 0, gpu: 0, mem: 0instead of2/1/4:Restoring the hunk turns all three green. A full revert of every production file
is red too, but only as a compile error, since the reconcile pass is new surface
its specs cannot load without — so the roll-over hunk is the isolation that
carries real signal. Stated plainly: the reconcile tests demonstrate new
behaviour, they do not reproduce a prior bug.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests