[REF-1476] fix: defer PTC child billing until parent execution settles#2267
Closed
nettee wants to merge 1 commit into
Closed
[REF-1476] fix: defer PTC child billing until parent execution settles#2267nettee wants to merge 1 commit into
nettee wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds deferred PTC billing: child-tool PTC charges are enqueued and persisted, then settled or discarded when the parent Changes
Sequence Diagram(s)sequenceDiagram
participant SkillInvoker as SkillInvokerService
participant Billing as BillingService
participant Redis as Redis
participant Tool as ToolExecutor
participant DB as Prisma/DB
rect rgba(200,200,255,0.5)
SkillInvoker->>Tool: start parent execute_code
Tool-->>SkillInvoker: parent toolCallId (parent)
end
rect rgba(200,255,200,0.5)
Note over Tool,Billing: Child PTC tool calls occur during parent execution
Tool->>Billing: report child PTC success + bill payload (child)
Billing->>Redis: enqueue deferred entry keyed by parentCallId
Billing-->>Tool: ack (deferred)
end
rect rgba(255,220,200,0.5)
SkillInvoker->>Billing: on parent terminal (success/failure/timeout) settleOrDiscard(parentCallId)
Billing->>Redis: lock parent key
Billing->>Redis: get deferred entry ids
loop per deferred entry
Billing->>DB: check existing tool usage / idempotency
alt parent succeeded and no existing usage
Billing->>Billing: settleBillingCharge(entry) -> charge via immediate path
else
Billing->>Redis: discard entry (remove)
end
end
Billing->>Redis: remove processed ids and release lock
Billing-->>SkillInvoker: done
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Deploying refly-branch-test with
|
| Latest commit: |
f954daa
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://58766d1d.refly-branch-test.pages.dev |
| Branch Preview URL: | https://ptc-timeout-billing-pr.refly-branch-test.pages.dev |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/modules/tool/billing/billing.service.spec.ts (1)
115-201:⚠️ Potential issue | 🟠 MajorThe new Redis-backed billing path still has no direct test coverage.
These mocks wire
toolCallResult,creditUsage, and Redis state for deferred billing, but the suite still never exercisessettleOrDiscardDeferredPtcBilling(), duplicate settlement/discard triggers, or the existing-charge guard. For a billing change, that leaves the new parent-terminal flow able to regress without a failing test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/tool/billing/billing.service.spec.ts` around lines 115 - 201, Tests don't exercise the new Redis-backed billing path—specifically settleOrDiscardDeferredPtcBilling and the duplicate-settlement/discard and existing-charge guard—so add unit tests that invoke BillingService.settleOrDiscardDeferredPtcBilling (and any exposed parent-terminal flow entrypoints) using the provided mocked dependencies (mockRedis, mockPrisma toolCallResult/creditUsage, and mockCreditService), simulate scenarios for: successful settle (flush > 0 via mockClient.eval), remainder-only (flush == 0), duplicate replayed events, and pre-existing charge present (creditUsage.findFirst returns a record) and assert that the correct Redis methods (sadd/srem/scard/eval/getJSON/setJSON/del) are called and syncToolCreditUsage is/n't invoked accordingly and that duplicate/discard paths short-circuit as expected.
🧹 Nitpick comments (4)
apps/api/src/modules/tool/billing/billing.service.ts (4)
1049-1049: Optional: Consider reducing cron frequency.Running reconciliation every minute is fairly aggressive. Since inline settlement handles most cases and this is just a backup for transient failures, consider reducing to every 5 minutes (
'*/5 * * * *') to reduce overhead while still providing timely recovery.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/tool/billing/billing.service.ts` at line 1049, The `@Cron`('*/1 * * * *') decorator on the reconciliation job in billing.service.ts is too frequent; update the cron expression to run every 5 minutes (e.g., '*/5 * * * *') on the reconciliation method in the BillingService (the method decorated with `@Cron`) so the backup reconciliation runs less aggressively while still providing timely recovery.
752-762: Silent catch blocks obscure Redis command failures.The catch blocks at lines 756 and 769/790 silently swallow Redis errors when falling back to JSON-based storage. This makes debugging production issues difficult. Consider logging these errors at debug/warn level.
📝 Proposed fix to add error logging
try { const client = this.redis.getClient(); await client.sadd(parentKey, entry.entryId); await client.expire(parentKey, DEFERRED_PTC_BILLING_TTL_SECONDS); - } catch { + } catch (error) { + this.logger.debug( + `Redis SADD failed, using JSON fallback: ${error instanceof Error ? error.message : String(error)}`, + ); const currentEntryIds = (await this.redis.getJSON<string[]>(parentKey)) ?? [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/tool/billing/billing.service.ts` around lines 752 - 762, The catch blocks that fallback from Redis commands to JSON storage are silently swallowing errors; update the catch handlers around this.redis.getClient()/client.sadd and the later similar catch to log the caught error (include context: parentKey and entry.entryId) at debug or warn level before proceeding with the JSON fallback; ensure logs reference DEFERRED_PTC_BILLING_TTL_SECONDS and the operation (sadd/expire or setJSON) and use the existing logger instance or a passed-in logger to avoid throwing from the catch.
503-510: Remove redundant?? undefinedexpressions.The trailing
?? undefinedon lines 507-509 is redundant sincegetToolCallId(),getResultId(), andgetResultVersion()already returnundefinedwhen no value is available.♻️ Suggested simplification
idempotencyKey: this.buildDefaultIdempotencyKey({ uid, toolName, toolsetKey, - toolCallId: options.toolCallId ?? getToolCallId() ?? undefined, - resultId: options.resultId ?? getResultId() ?? undefined, - version: options.version ?? getResultVersion() ?? undefined, + toolCallId: options.toolCallId ?? getToolCallId(), + resultId: options.resultId ?? getResultId(), + version: options.version ?? getResultVersion(), }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/tool/billing/billing.service.ts` around lines 503 - 510, The idempotency key construction uses redundant "?? undefined" fallbacks; remove the unnecessary "?? undefined" on the toolCallId, resultId, and version properties so they simply use options.toolCallId ?? getToolCallId(), options.resultId ?? getResultId(), and options.version ?? getResultVersion() when building the idempotency key in buildDefaultIdempotencyKey (refer to the idempotencyKey object and the helper functions getToolCallId, getResultId, getResultVersion).
1069-1085: Consider batching parent status queries to reduce DB load.The reconciliation job queries the database individually for each parent call (N+1 query pattern). With many deferred entries, this could create significant database load, especially running every minute.
Consider batching the queries:
⚡ Proposed optimization using batch query
+ // Batch query parent statuses to avoid N+1 + const parentCallIds = parentKeys + .map((key) => key.replace(DEFERRED_PTC_PARENT_KEY_PREFIX, '')) + .filter(Boolean); + + if (parentCallIds.length === 0) { + return; + } + + const parentCalls = await this.prisma.toolCallResult.findMany({ + where: { callId: { in: parentCallIds } }, + select: { callId: true, status: true }, + }); + + const statusMap = new Map(parentCalls.map((p) => [p.callId, p.status])); + - for (const parentKey of parentKeys) { - const parentCallId = parentKey.replace(DEFERRED_PTC_PARENT_KEY_PREFIX, ''); - if (!parentCallId) { - continue; - } - - const parentCall = await this.prisma.toolCallResult.findUnique({ - where: { callId: parentCallId }, - select: { status: true }, - }); - - if (parentCall?.status === 'completed') { + for (const parentCallId of parentCallIds) { + const status = statusMap.get(parentCallId); + + if (status === 'completed') { await this.settleOrDiscardDeferredPtcBilling(parentCallId, true); - } else if (parentCall?.status === 'failed') { + } else if (status === 'failed') { await this.settleOrDiscardDeferredPtcBilling(parentCallId, false); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/tool/billing/billing.service.ts` around lines 1069 - 1085, The loop creates an N+1 query by calling prisma.toolCallResult.findUnique for each parentKey; instead, collect parentCallIds by stripping DEFERRED_PTC_PARENT_KEY_PREFIX from parentKeys, batch-query prisma.toolCallResult.findMany with a where: { callId: { in: parentCallIds } } selecting callId and status, build a map from callId->status, then iterate parentKeys and call settleOrDiscardDeferredPtcBilling(parentCallId, true/false) based on the mapped status; for large parent lists, perform the IN queries in chunks to avoid extremely large IN clauses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/skill/skill-invoker.service.ts`:
- Around line 1219-1225: The deferred billing discard currently runs only in the
execute_code tool callbacks (on_tool_end/on_tool_error) and can be skipped if
control jumps to the outer catch/finally (or if ptcPollerManager.stop throws);
add a parent-level fallback by tracking active execute_code toolCallId values
(e.g., maintain a Set pendingExecuteCodeCalls: add in the execute_code start
path and delete after successful billing in the tool callbacks) and in the outer
catch/finally call Promise.allSettled([...pendingExecuteCodeCalls].map(id =>
this.billingService.settleOrDiscardDeferredPtcBilling(id, false))) to ensure any
remaining deferred PTC billing is settled/discarded; update places that
start/finish execute_code and ensure pendingExecuteCodeCalls is referenced from
the same service/context as billingService.settleOrDiscardDeferredPtcBilling to
avoid races.
In `@apps/api/src/modules/tool/billing/billing.service.ts`:
- Around line 597-599: The code currently logs a warning when lock acquisition
fails (releaseLock is falsy) but continues processing, creating a TOCTOU race
between hasExistingToolUsage and settleBillingCharge; change the behavior so
that when releaseLock is not acquired you log and return early (abort
processing) to let reconciliation retry later; update the block that contains
the releaseLock check (the logger.warn using parentCallId) to return immediately
instead of proceeding to hasExistingToolUsage/settleBillingCharge, or
alternatively replace the ad-hoc lock logic with a stricter locking strategy so
processing only continues when a valid lock exists.
- Around line 637-645: The loop currently calls settleBillingCharge(entry) but
always marks the entry for deletion (keysToDelete/entryIds) regardless of
result; modify the logic inside the loop (around hasExistingToolUsage,
settleBillingCharge, keysToDelete and entryIds) so you await
settleBillingCharge(entry), inspect its returned result (e.g., success flag or
error), and only push the entry key into keysToDelete and entryIds when the
settlement succeeded; on failure, log the failure with context (toolCallId/entry
id) and do not include the entry for
redis.delMany/removeProcessedDeferredPtcEntryIds so it can be retried by
reconciliation.
In `@specs/current/ptc/scripts/ptc_common.py`:
- Around line 575-589: This warning logic flags any completed child with zero
credits as "unbilled_success" regardless of parent outcome; move this block so
it runs after parent linkage and add a guard that skips children whose parent
finished unsuccessfully or was explicitly discarded (e.g., check the linked
parent call's status for failed/timed_out/discarded or an explicit discarded
flag) before appending the warning; keep the existing checks (status ==
"completed", (credits_charged or 0) == 0, not is_non_billable) but require the
parent's success condition as well to avoid flagging intentionally
discarded/failed-parent children (use the parent object obtained during linkage
to inspect its status).
- Around line 309-323: The print_runtime_config_info function currently prints
raw runtime values from get_runtime_config_snapshot (snapshot -> values), which
may leak secrets (notably keys like PTC_*, SANDBOX_S3LIB_*, *_KEY, *_SECRET,
PASSWORD, TOKEN); change the values-loop in print_runtime_config_info to detect
secret-like keys (match prefixes PTC_, SANDBOX_S3LIB_ or suffixes _KEY, _SECRET,
PASSWORD, TOKEN, and any key in a configured secrets list) and redact them
before printing (e.g., print "<redacted>" or only indicate "set"/"unset" or show
a short masked prefix) while leaving non-sensitive keys unchanged; update
references to snapshot, values and DB_URL_ENV_BY_TARGET accordingly so the
function prints presence or masked values only.
---
Outside diff comments:
In `@apps/api/src/modules/tool/billing/billing.service.spec.ts`:
- Around line 115-201: Tests don't exercise the new Redis-backed billing
path—specifically settleOrDiscardDeferredPtcBilling and the
duplicate-settlement/discard and existing-charge guard—so add unit tests that
invoke BillingService.settleOrDiscardDeferredPtcBilling (and any exposed
parent-terminal flow entrypoints) using the provided mocked dependencies
(mockRedis, mockPrisma toolCallResult/creditUsage, and mockCreditService),
simulate scenarios for: successful settle (flush > 0 via mockClient.eval),
remainder-only (flush == 0), duplicate replayed events, and pre-existing charge
present (creditUsage.findFirst returns a record) and assert that the correct
Redis methods (sadd/srem/scard/eval/getJSON/setJSON/del) are called and
syncToolCreditUsage is/n't invoked accordingly and that duplicate/discard paths
short-circuit as expected.
---
Nitpick comments:
In `@apps/api/src/modules/tool/billing/billing.service.ts`:
- Line 1049: The `@Cron`('*/1 * * * *') decorator on the reconciliation job in
billing.service.ts is too frequent; update the cron expression to run every 5
minutes (e.g., '*/5 * * * *') on the reconciliation method in the BillingService
(the method decorated with `@Cron`) so the backup reconciliation runs less
aggressively while still providing timely recovery.
- Around line 752-762: The catch blocks that fallback from Redis commands to
JSON storage are silently swallowing errors; update the catch handlers around
this.redis.getClient()/client.sadd and the later similar catch to log the caught
error (include context: parentKey and entry.entryId) at debug or warn level
before proceeding with the JSON fallback; ensure logs reference
DEFERRED_PTC_BILLING_TTL_SECONDS and the operation (sadd/expire or setJSON) and
use the existing logger instance or a passed-in logger to avoid throwing from
the catch.
- Around line 503-510: The idempotency key construction uses redundant "??
undefined" fallbacks; remove the unnecessary "?? undefined" on the toolCallId,
resultId, and version properties so they simply use options.toolCallId ??
getToolCallId(), options.resultId ?? getResultId(), and options.version ??
getResultVersion() when building the idempotency key in
buildDefaultIdempotencyKey (refer to the idempotencyKey object and the helper
functions getToolCallId, getResultId, getResultVersion).
- Around line 1069-1085: The loop creates an N+1 query by calling
prisma.toolCallResult.findUnique for each parentKey; instead, collect
parentCallIds by stripping DEFERRED_PTC_PARENT_KEY_PREFIX from parentKeys,
batch-query prisma.toolCallResult.findMany with a where: { callId: { in:
parentCallIds } } selecting callId and status, build a map from callId->status,
then iterate parentKeys and call settleOrDiscardDeferredPtcBilling(parentCallId,
true/false) based on the mapped status; for large parent lists, perform the IN
queries in chunks to avoid extremely large IN clauses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37fee165-99d9-44a3-b299-35489dd7da6b
📒 Files selected for processing (9)
apps/api/src/modules/skill/skill-invoker.service.tsapps/api/src/modules/skill/skill.module.tsapps/api/src/modules/tool/billing/billing.service.spec.tsapps/api/src/modules/tool/billing/billing.service.tsspecs/change/20260310-ptc-timeout-billing-root-fix/spec.mdspecs/current/ptc/scripts/ptc_common.pyspecs/current/ptc/scripts/ptc_debug_billing.pyspecs/current/ptc/scripts/ptc_debug_calling.pyspecs/current/ptc/scripts/ptc_verify.py
Contributor
Author
|
/cr 修复 PTC 子调用在父级 execute_code 超时/失败后仍可能计费的问题，改为父调用结束后统一结算或丢弃 |
|
✅ CR summary sent to Slack channel #code-reviews. |
- Route PTC child-call charges through deferred Redis entries and settle/discard them based on parent execute_code terminal status to prevent timeout-related overbilling. - Integrate BillingService with skill invoker terminal handling and add periodic reconciliation plus idempotent safeguards for deferred settlements. - Update billing tests and PTC debug scripts/spec docs to support environment-aware diagnostics and runtime config visibility.
nettee
force-pushed
the
ptc-timeout-billing-pr
branch
from
0648ea7 to
f954daa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
specs/current/ptc/scripts/ptc_common.py (2)
559-573:⚠️ Potential issue | 🟠 MajorThis discarded-parent branch never matches as written.
should_warn_unbilled_success()checksparent.get("discarded")/parent.get("is_discarded"), but those fields are never populated whenrowis built above. If the deferred billing flow represents discards via a boolean flag instead ofstatus = 'discarded', these children will still fall through to the warning path. Either carry the discard marker intorow, or delete the dead branch and rely solely on status.Also applies to: 584-603
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/current/ptc/scripts/ptc_common.py` around lines 559 - 573, The code builds a dictionary named row that lacks any discarded flag, but should_warn_unbilled_success() later checks parent.get("discarded") / parent.get("is_discarded") so the branch never matches; fix by adding the discard marker(s) into row (e.g., include "discarded": parent_discarded_value and/or "is_discarded": parent_is_discarded_value when constructing row in the block that sets call_id/tool_name/type/status/.../is_non_billable), or if the system uses only status to indicate discard, remove the dead discarded/is_discarded checks in should_warn_unbilled_success() so the logic relies solely on status consistently (ensure to apply the same change to the other occurrence referenced around lines 584-603).
320-323:⚠️ Potential issue | 🟠 MajorRedact runtime config values before printing them.
get_runtime_config_snapshot()now pulls from local.envfiles and Kubernetes secrets, and this loop prints every matchingPTC_*/SANDBOX_S3LIB_*value verbatim. Because the three CLI scripts call this helper by default, running them will leak credentials into terminal history and captured logs. Mask secret-like keys or print presence only.🔒 Suggested hardening
+def _redact_runtime_value(key, value): + upper_key = key.upper() + if any(token in upper_key for token in ("KEY", "SECRET", "TOKEN", "PASSWORD")): + return "<redacted>" + return value + def print_runtime_config_info(env_name): @@ values = snapshot.get("values", {}) if values: for key in sorted(values): - print(f" {key}: {values[key]}") + print(f" {key}: {_redact_runtime_value(key, values[key])}") else: print(" (no SANDBOX_S3LIB_* / PTC_* values found)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/current/ptc/scripts/ptc_common.py` around lines 320 - 323, The loop that prints runtime config values in ptc_common.py (where snapshot is built by get_runtime_config_snapshot()) may leak secrets; update the for-loop over values (the block starting with for key in sorted(values):) to redact secret-like keys—detect keys containing substrings such as "PTC_", "SANDBOX_S3LIB_", "SECRET", "KEY", "TOKEN", "PASSWORD" or any key matching a configurable secret pattern—and instead of printing the raw value either print a presence indicator like "<redacted>" or a masked form (e.g., show only first/last 2 chars with the middle replaced by asterisks). Preserve non-secret keys' plain output. Ensure this logic is applied consistently wherever get_runtime_config_snapshot() is consumed.
🧹 Nitpick comments (2)
specs/change/20260310-ptc-timeout-billing-root-fix/spec.md (1)
48-102: Specify TTL value and settlement failure handling.The design section is comprehensive, but two operational details would strengthen it:
TTL value: Line 52 mentions persisting deferred payloads in Redis, and line 100 mentions "keep deferred entries until TTL," but the actual TTL duration is not specified. This is important for understanding orphaned entry cleanup behavior.
Settlement failure handling: The design doesn't address what happens if the settlement/discard process itself fails (e.g., Redis unavailable during
settleOrDiscardDeferredPtcBillingexecution). Consider documenting retry logic or alerting requirements.📋 Suggested additions to Design section
Consider adding a subsection under "Architecture Overview" or "Edge Cases":
- TTL Configuration - Deferred entries: 24-hour TTL (configurable via DEFERRED_PTC_BILLING_TTL_SECONDS) - Lock keys: 10-second TTL as shown in code - Settlement Failure Handling - Redis unavailable: log error, rely on TTL-based cleanup - Lock acquisition failure: skip settlement with warning (noThrow: true) - Partial settlement failure: continue processing remaining entries🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/change/20260310-ptc-timeout-billing-root-fix/spec.md` around lines 48 - 102, Add explicit TTL and failure-handling details to the BillingService design and implementation: define a config constant (e.g., DEFERRED_PTC_BILLING_TTL_SECONDS defaulting to 24*3600) used by the Redis enqueue helpers and document the lock TTL (e.g., 10s) in the design; update settleOrDiscardDeferredPtcBilling in BillingService to handle Redis/unexpected failures by retrying a bounded number of times, logging/alerting on persistent failure, and continuing processing on partial failures (ensure idempotency safeguards still apply); also document behavior for lock-acquisition failure (skip with warning) and reliance on TTL-based cleanup in the spec and tests.apps/api/src/modules/tool/billing/billing.service.spec.ts (1)
115-202: Please add coverage for the deferred PTC lifecycle itself.These mock additions are useful, but the spec still only exercises
processBilling. The risky behavior added in this PR is “defer child billing, then settle on parent success / discard on failure / retry safely after transient errors”, and none of that is asserted here. A couple of focused tests around that flow would make these billing regressions much harder to reintroduce.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/tool/billing/billing.service.spec.ts` around lines 115 - 202, Add unit tests in billing.service.spec.ts that directly exercise the deferred PTC lifecycle around processBilling: write tests that (1) simulate deferring a child billing (use the existing mockRedis/getClient().eval and redisStore helpers to return flush/remainder/replayed values) and then assert that when the parent billing succeeds the deferred child is settled (e.g., mockCreditService.syncToolCreditUsage is called and Redis entries removed via mockRedis.del or mockClient.srem), (2) simulate parent failure and assert the deferred child is discarded (no syncToolCreditUsage call and deferred keys removed), and (3) simulate a transient error path (use eval returning a non‑zero “replayed” or throw on first attempt then succeed) and assert safe retry behavior (no double-settle, eventual syncToolCreditUsage called once). Target the processBilling invocation and the Redis mock methods (getClient().eval, smembers, srem, del) and mockCreditService.syncToolCreditUsage to locate the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/skill/skill-invoker.service.ts`:
- Around line 1694-1710: The current fallback unconditionally calls
billingService.settleOrDiscardDeferredPtcBilling(toolCallId, false) for every id
in pendingExecuteCodeCalls; change this to use the actual terminal outcome for
each call instead of hardcoding false by storing per-call terminal state when a
parent finishes (e.g., maintain a Map<string, boolean> or attach status to
pendingExecuteCodeCalls entries) and then, in the cleanup block that builds
pendingCallIds and calls settleOrDiscardDeferredPtcBilling, pass the recorded
parentSucceeded value for each toolCallId; ensure you look up the recorded
outcome (by toolCallId) when creating the Promise for each
settleOrDiscardDeferredPtcBilling call and fall back to a sensible default only
if no recorded outcome exists, while still clearing pendingExecuteCodeCalls
afterwards.
In `@apps/api/src/modules/tool/billing/billing.service.ts`:
- Around line 741-756: The current enqueue logic uses setIfNotExists(entryKey,
...) then returns early if not inserted, which can leave the parent index
(getDeferredPtcParentKey) missing the entryId; modify the flow so the parent set
is always updated: after calling setIfNotExists, always obtain client =
this.redis.getClient() and call client.sadd(parentKey, entry.entryId) and
client.expire(parentKey, DEFERRED_PTC_BILLING_TTL_SECONDS) regardless of
inserted, or make the two writes atomic by using a Redis MULTI/EXEC or a Lua
script to perform SETNX + SADD + EXPIRE in one transaction; ensure you keep the
existing keys (entryKey, parentKey) and TTL constants
(DEFERRED_PTC_BILLING_TTL_SECONDS) and preserve error handling around the redis
operations (the methods: setIfNotExists, getDeferredPtcParentKey,
redis.getClient().sadd, client.expire).
---
Duplicate comments:
In `@specs/current/ptc/scripts/ptc_common.py`:
- Around line 559-573: The code builds a dictionary named row that lacks any
discarded flag, but should_warn_unbilled_success() later checks
parent.get("discarded") / parent.get("is_discarded") so the branch never
matches; fix by adding the discard marker(s) into row (e.g., include
"discarded": parent_discarded_value and/or "is_discarded":
parent_is_discarded_value when constructing row in the block that sets
call_id/tool_name/type/status/.../is_non_billable), or if the system uses only
status to indicate discard, remove the dead discarded/is_discarded checks in
should_warn_unbilled_success() so the logic relies solely on status consistently
(ensure to apply the same change to the other occurrence referenced around lines
584-603).
- Around line 320-323: The loop that prints runtime config values in
ptc_common.py (where snapshot is built by get_runtime_config_snapshot()) may
leak secrets; update the for-loop over values (the block starting with for key
in sorted(values):) to redact secret-like keys—detect keys containing substrings
such as "PTC_", "SANDBOX_S3LIB_", "SECRET", "KEY", "TOKEN", "PASSWORD" or any
key matching a configurable secret pattern—and instead of printing the raw value
either print a presence indicator like "<redacted>" or a masked form (e.g., show
only first/last 2 chars with the middle replaced by asterisks). Preserve
non-secret keys' plain output. Ensure this logic is applied consistently
wherever get_runtime_config_snapshot() is consumed.
---
Nitpick comments:
In `@apps/api/src/modules/tool/billing/billing.service.spec.ts`:
- Around line 115-202: Add unit tests in billing.service.spec.ts that directly
exercise the deferred PTC lifecycle around processBilling: write tests that (1)
simulate deferring a child billing (use the existing mockRedis/getClient().eval
and redisStore helpers to return flush/remainder/replayed values) and then
assert that when the parent billing succeeds the deferred child is settled
(e.g., mockCreditService.syncToolCreditUsage is called and Redis entries removed
via mockRedis.del or mockClient.srem), (2) simulate parent failure and assert
the deferred child is discarded (no syncToolCreditUsage call and deferred keys
removed), and (3) simulate a transient error path (use eval returning a non‑zero
“replayed” or throw on first attempt then succeed) and assert safe retry
behavior (no double-settle, eventual syncToolCreditUsage called once). Target
the processBilling invocation and the Redis mock methods (getClient().eval,
smembers, srem, del) and mockCreditService.syncToolCreditUsage to locate the
code under test.
In `@specs/change/20260310-ptc-timeout-billing-root-fix/spec.md`:
- Around line 48-102: Add explicit TTL and failure-handling details to the
BillingService design and implementation: define a config constant (e.g.,
DEFERRED_PTC_BILLING_TTL_SECONDS defaulting to 24*3600) used by the Redis
enqueue helpers and document the lock TTL (e.g., 10s) in the design; update
settleOrDiscardDeferredPtcBilling in BillingService to handle Redis/unexpected
failures by retrying a bounded number of times, logging/alerting on persistent
failure, and continuing processing on partial failures (ensure idempotency
safeguards still apply); also document behavior for lock-acquisition failure
(skip with warning) and reliance on TTL-based cleanup in the spec and tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ccee4048-a7a9-4fa4-961e-be3e4c957725
📒 Files selected for processing (9)
apps/api/src/modules/skill/skill-invoker.service.tsapps/api/src/modules/skill/skill.module.tsapps/api/src/modules/tool/billing/billing.service.spec.tsapps/api/src/modules/tool/billing/billing.service.tsspecs/change/20260310-ptc-timeout-billing-root-fix/spec.mdspecs/current/ptc/scripts/ptc_common.pyspecs/current/ptc/scripts/ptc_debug_billing.pyspecs/current/ptc/scripts/ptc_debug_calling.pyspecs/current/ptc/scripts/ptc_verify.py
Comment on lines
+1694
to
+1710
| if (pendingExecuteCodeCalls.size > 0) { | ||
| const pendingCallIds = [...pendingExecuteCodeCalls]; | ||
| const fallbackResults = await Promise.allSettled( | ||
| pendingCallIds.map((toolCallId) => | ||
| this.billingService.settleOrDiscardDeferredPtcBilling(toolCallId, false), | ||
| ), | ||
| ); | ||
|
|
||
| fallbackResults.forEach((result, index) => { | ||
| if (result.status === 'rejected') { | ||
| this.logger.error( | ||
| `Deferred PTC billing fallback discard failed for ${pendingCallIds[index]}: ${result.reason instanceof Error ? result.reason.message : String(result.reason)}`, | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| pendingExecuteCodeCalls.clear(); |
There was a problem hiding this comment.
Don't always retry pending execute_code calls as failures.
Anything left in pendingExecuteCodeCalls is retried here with parentSucceeded = false. That includes successful parents when ptcPollerManager.stop() or the earlier success-path settlement throws, so a transient cleanup/billing error turns into a permanent discard of valid child charges. Track the terminal outcome per call and use that value in the fallback instead of hardcoding false.
Suggested fix
- const pendingExecuteCodeCalls: Set<string> = new Set();
+ const pendingExecuteCodeCalls = new Map<string, boolean>();
...
- pendingExecuteCodeCalls.add(toolCallId);
+ pendingExecuteCodeCalls.set(toolCallId, false);
...
+ pendingExecuteCodeCalls.set(toolCallId, !isErrorStatus);
await this.billingService.settleOrDiscardDeferredPtcBilling(
toolCallId,
!isErrorStatus,
);
pendingExecuteCodeCalls.delete(toolCallId);
...
- const pendingCallIds = [...pendingExecuteCodeCalls];
+ const pendingCalls = [...pendingExecuteCodeCalls.entries()];
const fallbackResults = await Promise.allSettled(
- pendingCallIds.map((toolCallId) =>
- this.billingService.settleOrDiscardDeferredPtcBilling(toolCallId, false),
+ pendingCalls.map(([toolCallId, parentSucceeded]) =>
+ this.billingService.settleOrDiscardDeferredPtcBilling(toolCallId, parentSucceeded),
),
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/modules/skill/skill-invoker.service.ts` around lines 1694 -
1710, The current fallback unconditionally calls
billingService.settleOrDiscardDeferredPtcBilling(toolCallId, false) for every id
in pendingExecuteCodeCalls; change this to use the actual terminal outcome for
each call instead of hardcoding false by storing per-call terminal state when a
parent finishes (e.g., maintain a Map<string, boolean> or attach status to
pendingExecuteCodeCalls entries) and then, in the cleanup block that builds
pendingCallIds and calls settleOrDiscardDeferredPtcBilling, pass the recorded
parentSucceeded value for each toolCallId; ensure you look up the recorded
outcome (by toolCallId) when creating the Promise for each
settleOrDiscardDeferredPtcBilling call and fall back to a sensible default only
if no recorded outcome exists, while still clearing pendingExecuteCodeCalls
afterwards.
Comment on lines
+741
to
+756
| const inserted = await this.redis.setIfNotExists( | ||
| entryKey, | ||
| JSON.stringify(entry), | ||
| DEFERRED_PTC_BILLING_TTL_SECONDS, | ||
| ); | ||
|
|
||
| if (!inserted) { | ||
| return; | ||
| } | ||
|
|
||
| const parentKey = this.getDeferredPtcParentKey(entry.parentCallId); | ||
|
|
||
| try { | ||
| const client = this.redis.getClient(); | ||
| await client.sadd(parentKey, entry.entryId); | ||
| await client.expire(parentKey, DEFERRED_PTC_BILLING_TTL_SECONDS); |
There was a problem hiding this comment.
Repair the parent index even when the entry already exists.
This enqueue path can orphan deferred charges. If the process dies after setIfNotExists(entryKey, ...) succeeds but before sadd(parentKey, entryId), the retry on Line 747 returns early and never re-links the entry to its parent, so reconciliation will never see it. Please make the two writes atomic, or at minimum always re-add entryId to parentKey when the entry already exists.
Suggested fix
private async enqueueDeferredPtcBillingEntry(entry: DeferredPtcBillingEntry): Promise<void> {
const entryKey = this.getDeferredPtcEntryKey(entry.entryId);
- const inserted = await this.redis.setIfNotExists(
+ await this.redis.setIfNotExists(
entryKey,
JSON.stringify(entry),
DEFERRED_PTC_BILLING_TTL_SECONDS,
);
-
- if (!inserted) {
- return;
- }
const parentKey = this.getDeferredPtcParentKey(entry.parentCallId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/modules/tool/billing/billing.service.ts` around lines 741 - 756,
The current enqueue logic uses setIfNotExists(entryKey, ...) then returns early
if not inserted, which can leave the parent index (getDeferredPtcParentKey)
missing the entryId; modify the flow so the parent set is always updated: after
calling setIfNotExists, always obtain client = this.redis.getClient() and call
client.sadd(parentKey, entry.entryId) and client.expire(parentKey,
DEFERRED_PTC_BILLING_TTL_SECONDS) regardless of inserted, or make the two writes
atomic by using a Redis MULTI/EXEC or a Lua script to perform SETNX + SADD +
EXPIRE in one transaction; ensure you keep the existing keys (entryKey,
parentKey) and TTL constants (DEFERRED_PTC_BILLING_TTL_SECONDS) and preserve
error handling around the redis operations (the methods: setIfNotExists,
getDeferredPtcParentKey, redis.getClient().sadd, client.expire).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes timeout-related PTC billing inconsistencies by deferring child-call charges until the parent
execute_codecall reaches a terminal state.Changes
BillingServicethat queues child-call charges and settles or discards them based on parent success/failureSkillInvokerServiceterminal handling to trigger deferred billing settlement/discard forexecute_codeSummary by CodeRabbit
New Features
Tests
Chores