Skip to content

Conversation

@icecrasher321
Copy link
Collaborator

Summary

  • Even if log fails to write to db, we should correctly increment cost
  • Massive logs killing the write with error

Type of Change

  • Bug fix

Testing

Testing with @aadamgough

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Dec 20, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Review Updated (UTC)
docs Skipped Skipped Dec 20, 2025 1:38am

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 20, 2025

Greptile Summary

This PR fixes a critical bug where execution costs were not captured when logs failed to write to the database due to size constraints. The implementation adds three key improvements:

  • Enhanced filterForDisplay to truncate strings to 100K characters and remove null bytes that PostgreSQL JSONB doesn't support
  • Added completeWithCostOnlyLog fallback mechanism that stores execution costs with empty trace spans when the full log write fails
  • Added optional level parameter override to support the fallback scenario

Key Issue Found:

  • The fallback mechanism in completeWithCostOnlyLog calls calculateCostSummary(params.traceSpans) on potentially oversized trace spans that caused the original failure. If these spans are large enough to cause memory issues during traversal, the fallback itself will fail and costs won't be captured.

Recommendation:

  • Wrap the calculateCostSummary call in a try-catch block that falls back to BASE_EXECUTION_CHARGE if cost calculation fails

Confidence Score: 3/5

  • This PR is generally safe but has a potential edge case where the fallback mechanism could fail
  • Score reflects good intent and mostly solid implementation, but the fallback in completeWithCostOnlyLog could fail if calculateCostSummary() crashes while traversing oversized trace spans. The display filters are well-implemented with null byte removal and truncation.
  • apps/sim/lib/logs/execution/logging-session.ts needs attention - the cost calculation fallback should handle the case where trace spans are too large to traverse

Important Files Changed

Filename Overview
apps/sim/lib/core/utils/display-filters.ts Significantly enhanced to handle massive logs by truncating strings to 100K chars, removing null bytes, and safely handling edge cases like circular references and non-JSON-serializable types
apps/sim/lib/logs/execution/logging-session.ts Added cost-only fallback mechanism that ensures costs are captured even when trace spans are too large to store, potential issue with oversized spans causing fallback to fail
apps/sim/lib/logs/execution/logger.ts Added optional level override parameter to support cost-only fallback scenarios, straightforward and safe change

Sequence Diagram

sequenceDiagram
    participant WE as Workflow Executor
    participant LS as LoggingSession
    participant EL as ExecutionLogger
    participant DF as DisplayFilters
    participant DB as Database

    WE->>LS: safeComplete(traceSpans)
    LS->>LS: complete(traceSpans)
    LS->>EL: completeWorkflowExecution(traceSpans)
    EL->>DF: filterForDisplay(traceSpans)
    Note over DF: Truncate strings to 100K<br/>Remove null bytes<br/>Handle circular refs
    DF-->>EL: filtered traceSpans
    EL->>DB: Store execution log
    
    alt Database write fails (too large)
        DB-->>EL: Error
        EL-->>LS: Error
        LS->>LS: completeWithCostOnlyLog()
        LS->>LS: calculateCostSummary(traceSpans)
        Note over LS: Extract costs from<br/>oversized traceSpans
        LS->>EL: completeWorkflowExecution(empty traceSpans)
        Note over LS: Pass empty array to avoid<br/>size issues
        EL->>DF: filterForDisplay([])
        DF-->>EL: []
        EL->>DB: Store cost-only log
        DB-->>EL: Success
        Note over DB: Cost captured,<br/>traceSpans empty
    end
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

@icecrasher321
Copy link
Collaborator Author

@greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@icecrasher321 icecrasher321 merged commit 50c1c67 into staging Dec 20, 2025
6 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/logs-truncate branch December 20, 2025 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants