feat: update refill workflow for identity-based credits#4194
feat: update refill workflow for identity-based credits#4194Flo4604 wants to merge 6 commits intofeat/identity-credits-go-servicesfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe refill_keys workflow is refactored to support dual data paths: modern credits table entries and legacy key entries. Each path applies date-based refill logic with existence checks, updates respective database records in a transaction, and generates corresponding audit logs with resource-specific event types. The return structure changes to report refilled credits and legacy keys separately. Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant Database
participant Transaction
participant AuditLog
participant Return
Workflow->>Database: Fetch credits with date/existence filters
Workflow->>Database: Fetch legacy keys (no credits match)
rect rgb(240, 248, 255)
Note over Workflow,Transaction: Process both data paths
Workflow->>Workflow: Merge credits and legacy items
Workflow->>Workflow: Calculate refill amounts (date logic)
end
Workflow->>Transaction: Begin transaction
Transaction->>Database: Update credits (remaining, refilledAt, updatedAt)
Transaction->>Database: Update legacy keys (remaining)
Transaction->>AuditLog: Log dynamic audit events<br/>(by resource type & target)
AuditLog-->>Transaction: Confirm
Transaction-->>Workflow: Commit
Workflow->>Return: Return refillCreditIds & refillLegacyKeyIds
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
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 |
6668e81 to
8a07fb8
Compare
2d83e5c to
d434978
Compare
d434978 to
6dbce05
Compare
8a07fb8 to
82018c9
Compare
82018c9 to
723c836
Compare
6dbce05 to
68ae12d
Compare
723c836 to
6e6a1ec
Compare
68ae12d to
8097e24
Compare
8097e24 to
a5ab7f7
Compare
abb3650 to
6240d77
Compare
4f072a6 to
5d3d6f6
Compare
6240d77 to
ac969b1
Compare
5d3d6f6 to
922ec04
Compare
922ec04 to
3856684
Compare
ac969b1 to
2819cf3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workflows/src/workflows/refill_keys.ts (1)
20-22: Fix timezone inconsistency in date calculations.
lastDayOfMonthis calculated using local timezone methods (getMonth(),getFullYear(),getDate()), whiletodayuses UTC (getUTCDate()). This inconsistency could cause incorrect refill behavior when comparing these values, especially at timezone boundaries.Apply this diff to make both calculations use UTC:
- const lastDayOfMonth = new Date(now.getFullYear(), now.getMonth() + 1, 0).getDate(); + const lastDayOfMonth = new Date(Date.UTC(now.getUTCFullYear(), now.getUTCMonth() + 1, 0)).getUTCDate(); const today = now.getUTCDate();
🧹 Nitpick comments (1)
apps/workflows/src/workflows/refill_keys.ts (1)
150-151: Consider adding a defensive check for resource identification.While the queries ensure either
keyIdoridentityIdexists, adding a defensive check or comment would improve code robustness and clarity.Consider adding a check:
for (const credit of allToRefill) { + if (!credit.keyId && !credit.identityId) { + console.error(`Credit ${credit.id} has neither keyId nor identityId, skipping`); + continue; + } const resourceType = credit.keyId ? "key" : "identity";Or at minimum, add a comment explaining the assumption:
+ // Each credit has either keyId or identityId (ensured by queries) const resourceType = credit.keyId ? "key" : "identity";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/workflows/src/workflows/refill_keys.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.710Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
📚 Learning: 2025-10-30T15:10:52.710Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.710Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Applied to files:
apps/workflows/src/workflows/refill_keys.ts
🔇 Additional comments (6)
apps/workflows/src/workflows/refill_keys.ts (6)
31-97: Well-structured credits query with comprehensive filtering.The query correctly uses left joins to validate related entities at the database level and filters out deleted keys/identities. The date-based refill logic properly handles end-of-month edge cases.
100-145: Excellent double-refill prevention for legacy keys.The query correctly filters out keys that already have a credits entry (line 133) to prevent double refills. The mapping structure maintains consistency with the credits query results.
154-173: Correct handling of legacy and modern credit updates.The conditional logic appropriately updates the respective tables with the correct IDs and field types. The differences in field names (
lastRefillAtvsrefilledAt) and date formats (Date object vs timestamp) are appropriate for the respective schemas.
176-223: Well-designed dynamic audit logging.The audit log construction correctly handles different resource types with appropriate targets and event types. The defensive approach of conditionally adding both key and identity targets provides flexibility.
149-227: Verify error handling strategy for individual refills.The current implementation will stop the entire workflow if any single refill fails. Consider whether partial completion (refilling successful items even if some fail) would be more appropriate for a scheduled background job.
If partial completion is desired, consider adding error handling:
for (const credit of allToRefill) { const resourceType = credit.keyId ? "key" : "identity"; const resourceId = credit.keyId || credit.identityId; await step.do(`refilling ${credit.id} (${resourceType})`, async () => { try { await db.transaction(async (tx) => { // ... existing transaction logic }); return { creditId: credit.id, resourceType, resourceId }; } catch (error) { console.error(`Failed to refill ${credit.id}:`, error); return { creditId: credit.id, error: error.message, skipped: true }; } }); }Otherwise, if fail-fast is intentional, consider adding a comment explaining this behavior.
234-236: Clear and informative return structure.Separating refilled credits and legacy keys in the return value provides good visibility into the workflow execution and helps distinguish between the two data paths.
- Update refill_keys workflow to use new credits table - Refactor to work with credit-based refill configuration - Simplify workflow logic by leveraging credits table structure

What does this PR do?
Updates the workflow we use for refilling the credits to use the new credits table if key uses it otherwise does the old refill.
Type of change
How should this be tested?
Make sure credits get refilled for both keys and identities.
cd apps/workflowsnpx wrangler dev --test-scheduledcurl "http://localhost:8787/__scheduled?cron=0+0+*+*+*"This should trigger the refill.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated