feat(services): shadow liveness in UserMeter - #1119
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
# Conflicts: # packages/worker/src/email/inbound.ts # packages/worker/src/email/outbound.ts # packages/worker/worker-configuration.d.ts Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
# Conflicts: # docs/contributing/architecture/data-storage.md # packages/worker/src/account/export.node.test.ts # packages/worker/src/account/export.ts Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThe PR adds schema v5 package-service liveness shadows to UserMeter. It adds state operations, pagination, counting, bootstrap, export, purge, and RPC support. Package-service lifecycle projections mirror D1 state through non-blocking UserMeter writes. ChangesPackage-service shadow lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PackageService
participant D1
participant UserMeter
participant DurableObject
PackageService->>D1: Update package_service_states
PackageService->>UserMeter: Schedule shadow upsert
UserMeter-->>PackageService: Apply or log failure
PackageService->>UserMeter: Schedule shadow deletion during purge
PackageService->>DurableObject: Delete durable package state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
# Conflicts: # docs/contributing/architecture/data-storage.md Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
@coderabbitai review |
|
🔎 Preview deployed: https://kody-pr-1119.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/worker/src/package-runtime/package-service.node.test.ts (1)
982-1000: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for shadow write ordering during purge.
This block drains the purge tasks in scheduling order, so it cannot detect a reordered delete.
handlePurgeRequestschedules astoppedshadow upsert and then a shadow delete as two independentwaitUntiltasks. Add a test that settles the delete before the upsert and asserts the shadow row is absent. See the related comment onpackages/worker/src/package-runtime/package-service.tslines 461-469.Do you want me to generate this 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 `@packages/worker/src/package-runtime/package-service.node.test.ts` around lines 982 - 1000, Extend the purge test around handlePurgeRequest to settle the shadow-delete waitUntil task before the stopped-shadow-upsert task, rather than draining tasks in scheduling order. Assert that packageServiceShadowRow remains undefined after this reordered settlement and retain the existing delete assertion.packages/worker/src/entitlements/user-meter-do.ts (2)
1171-1191: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the bootstrap batch size.
bootstrapPackageServiceStatesiterates the wholeinput.statesarray and performs one read plus one write per entry. The input size is not capped. A large caller batch can exhaust the Durable Object request CPU budget and leave the shadow table partially seeded. Add an explicit maximum batch size and reject larger inputs, so callers page the cold bootstrap.♻️ Proposed cap
+const maxPackageServiceBootstrapStates = 500 + /** Cutover-support bulk seed; same monotonic guard as upsert. */ async bootstrapPackageServiceStates(input: { states: ReadonlyArray<{ @@ }): Promise<UserMeterPackageServiceBootstrapResult> { + if (input.states.length > maxPackageServiceBootstrapStates) { + throw new Error( + `UserMeter bootstrapPackageServiceStates accepts at most ${maxPackageServiceBootstrapStates} states per call.`, + ) + } let applied = 0🤖 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 `@packages/worker/src/entitlements/user-meter-do.ts` around lines 1171 - 1191, Update bootstrapPackageServiceStates to enforce an explicit maximum size for input.states before processing any entries, rejecting batches above that limit and preserving the existing applied/skipped upsert behavior for valid batches. Define or reuse a named cap near the method rather than silently truncating the input.
1262-1269: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the first-page shadow inventory.
listAllPackageServiceRowsreads every shadow row and returns them in one export page. UnlikestorageBytesShadow, this inventory grows with the number of package services for the user.countUserMeterExportEntriesinpackages/worker/src/account/export.ts(lines 211-219) adds that full length to the section item count, so one page can exceed the requestedpageSizeby an unbounded amount. Consider reading the inventory with a hardLIMITand reporting truncation, or paging it withlistPackageServiceStates.🤖 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 `@packages/worker/src/entitlements/user-meter-do.ts` around lines 1262 - 1269, The first-page shadow inventory in the export flow can exceed the requested pageSize because listAllPackageServiceRows returns every package-service row at once. Update the shadow handling around includeShadows and packageServiceStatesShadow to bound or page these rows using the existing listPackageServiceStates path, and ensure countUserMeterExportEntries reports the resulting truncation or pagination consistently with the emitted entries.packages/worker/src/package-runtime/package-service.ts (1)
490-492: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse distinct log labels for the upsert and delete failures.
Both handlers log
package-service-user-meter-shadow-failed. An operator cannot tell a failed mirror write from a failed mirror delete. A failed delete leaves an orphan shadow row, so it needs its own label. Also includepackageIdandserviceNamein the log so the failed row is identifiable.Also applies to: 519-521
🤖 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 `@packages/worker/src/package-runtime/package-service.ts` around lines 490 - 492, Update the error handlers around the upsert and delete operations to use distinct log labels, with the delete path indicating a shadow-delete failure rather than reusing the upsert label. Include packageId and serviceName in both console.warn calls so the affected shadow row is identifiable.packages/worker/src/test-support/user-meter.ts (1)
236-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMirror the Durable Object input validation in the stub.
The stub validates
statusonly. The realUserMeterBasealso rejects empty or over-longpackageIdandserviceNamethroughassertPackageServiceId, and rejects an emptysourceUpdatedAtthroughassertSourceUpdatedAt. Node tests that use this stub therefore pass with inputs that the production Durable Object rejects. Add the same identifier and timestamp checks so the double keeps parity.🤖 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 `@packages/worker/src/test-support/user-meter.ts` around lines 236 - 286, Update upsertPackageServiceState to mirror UserMeterBase validation by calling the existing assertPackageServiceId checks for packageId and serviceName and assertSourceUpdatedAt for sourceUpdatedAt before status processing or state mutation. Preserve the existing status validation and upsert behavior while ensuring empty and over-long identifiers and empty timestamps are rejected consistently.docs/contributing/architecture/data-storage.md (1)
267-269: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winQualify the store in both authority statements.
package_service_stateshas two stores. The D1 table is authoritative. The UserMeter schema-v5 table is a shadow. The text at Lines 267-269 does not repeat theUserMeterqualifier. Line 531 saysD1 remains sole authority for readswithout limitingreadsto package-service liveness. UserMeterdaily_countersremain authoritative, so this wording can mislead future cutover work.Use explicit store and read-scope names. This preserves the authority split stated in
docs/contributing/architecture/entitlements.mdLines 127-132 and 233-267.Proposed wording
- UserMeter `storage_bytes_state` (schema v4) and `package_service_states` (schema v5) are + UserMeter `storage_bytes_state` (schema v4) and UserMeter `package_service_states` (schema v5) are - D1 remains sole authority for reads, running counts, discovery, and + D1 remains sole authority for package-service liveness reads, running counts, discovery, andAlso applies to: 529-532
🤖 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 `@docs/contributing/architecture/data-storage.md` around lines 267 - 269, Qualify both authority statements in the architecture documentation: identify the UserMeter schema-v5 `package_service_states` table as a shadow while keeping the D1 `package_service_states` table authoritative, and limit “D1 remains sole authority for reads” to package-service liveness reads. Preserve UserMeter `daily_counters` as authoritative and align the wording with the authority split in the Entitlements documentation.
🤖 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 `@packages/worker/src/account/export.ts`:
- Around line 302-306: Update the documentation for packageServiceStatesShadow
in the UserMeter export type to state that continuation user_meter pages return
null rather than omitting the field, matching the existing branch behavior and
export.node.test.ts expectation. Do not change the implementation or test.
In `@packages/worker/src/entitlements/user-meter-do.ts`:
- Around line 288-295: Update assertSourceUpdatedAt to validate that
sourceUpdatedAt is a canonical ISO-8601 UTC timestamp, not merely a non-empty
string, so values used by upsertPackageServiceState and
countRunningPackageServices preserve lexicographic ordering. Reject malformed,
offset-based, or non-UTC inputs while continuing to return valid new
Date().toISOString()-style values.
In `@packages/worker/src/package-runtime/package-service.ts`:
- Around line 461-469: Serialize shadow upserts and deletes per instance instead
of starting independent waitUntil tasks. Update
schedulePackageServiceStateShadow and schedulePackageServiceShadowDelete to
append each shadow operation to a shared promise chain in scheduling order,
while retaining the existing userMeterNamespace guard and waitUntil lifecycle
handling.
---
Nitpick comments:
In `@docs/contributing/architecture/data-storage.md`:
- Around line 267-269: Qualify both authority statements in the architecture
documentation: identify the UserMeter schema-v5 `package_service_states` table
as a shadow while keeping the D1 `package_service_states` table authoritative,
and limit “D1 remains sole authority for reads” to package-service liveness
reads. Preserve UserMeter `daily_counters` as authoritative and align the
wording with the authority split in the Entitlements documentation.
In `@packages/worker/src/entitlements/user-meter-do.ts`:
- Around line 1171-1191: Update bootstrapPackageServiceStates to enforce an
explicit maximum size for input.states before processing any entries, rejecting
batches above that limit and preserving the existing applied/skipped upsert
behavior for valid batches. Define or reuse a named cap near the method rather
than silently truncating the input.
- Around line 1262-1269: The first-page shadow inventory in the export flow can
exceed the requested pageSize because listAllPackageServiceRows returns every
package-service row at once. Update the shadow handling around includeShadows
and packageServiceStatesShadow to bound or page these rows using the existing
listPackageServiceStates path, and ensure countUserMeterExportEntries reports
the resulting truncation or pagination consistently with the emitted entries.
In `@packages/worker/src/package-runtime/package-service.node.test.ts`:
- Around line 982-1000: Extend the purge test around handlePurgeRequest to
settle the shadow-delete waitUntil task before the stopped-shadow-upsert task,
rather than draining tasks in scheduling order. Assert that
packageServiceShadowRow remains undefined after this reordered settlement and
retain the existing delete assertion.
In `@packages/worker/src/package-runtime/package-service.ts`:
- Around line 490-492: Update the error handlers around the upsert and delete
operations to use distinct log labels, with the delete path indicating a
shadow-delete failure rather than reusing the upsert label. Include packageId
and serviceName in both console.warn calls so the affected shadow row is
identifiable.
In `@packages/worker/src/test-support/user-meter.ts`:
- Around line 236-286: Update upsertPackageServiceState to mirror UserMeterBase
validation by calling the existing assertPackageServiceId checks for packageId
and serviceName and assertSourceUpdatedAt for sourceUpdatedAt before status
processing or state mutation. Preserve the existing status validation and upsert
behavior while ensuring empty and over-long identifiers and empty timestamps are
rejected consistently.
🪄 Autofix (Beta)
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: 8a26de87-65f0-4277-9847-bf059fbb5b03
📒 Files selected for processing (11)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/entitlements.mddocs/contributing/architecture/primitives.yamlpackages/worker/src/account/export.node.test.tspackages/worker/src/account/export.tspackages/worker/src/account/user-owned-surfaces.tspackages/worker/src/entitlements/user-meter-do.tspackages/worker/src/entitlements/user-meter.workers.test.tspackages/worker/src/package-runtime/package-service.node.test.tspackages/worker/src/package-runtime/package-service.tspackages/worker/src/test-support/user-meter.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
# Conflicts: # docs/contributing/architecture/data-storage.md Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
3 participants
Summary
ctx.waitUntilpackage_service_statesas sole count, discovery, andservice_startauthorityValidation
Deployment notes
Phase A expand only. The authority flip remains a separate high-risk PR after a full 24-hour stale-window soak, parity review, and cold-bootstrap validation.
System recap — extends User meter and package services (medium risk)
Mode: recap · Base:
main@6a849eaf· Head:3a0d41d8Classification: extends — adds non-authoritative package-service liveness shadowing while preserving D1 enforcement authority.
Primitives touched
user-meterpackage-servicesaccount-exportSystem map
PackageServiceInstance keeps D1 authoritative and shadows ordered lifecycle state into UserMeter.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Invariants
Conductor report
Summary by CodeRabbit
New Features
Documentation
Tests