fix(topograph): return HTTP 202 for requests in progress

[dmitsh]

No description provided.

[codecov]

Codecov Report

❌ Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.82%. Comparing base (f392302) to head (20afd9b).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/server/trailing_delay_queue.go	57.14%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #210      +/-   ##
==========================================
- Coverage   65.88%   65.82%   -0.06%     
==========================================
  Files          82       82              
  Lines        4482     4483       +1     
==========================================
- Hits         2953     2951       -2     
- Misses       1416     1419       +3     
  Partials      113      113              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[greptile-apps]

Greptile Overview

Greptile Summary

This PR implements proper HTTP 202 (Accepted) status handling for asynchronous topology requests that are still being processed. Previously, requests in progress returned inconsistent responses; now they immediately store and return HTTP 202 responses when first submitted.

Key changes:

• TrailingDelayQueue.Submit() now immediately stores an HTTP 202 response in the LRU cache when generating a new request ID, ensuring clients can query the status before processing completes
• TrailingDelayQueue.Get() logic simplified by removing the redundant check for current UID, since HTTP 202 responses are now pre-stored
• Integration test updated to properly handle HTTP 202 responses in a loop (continue polling) vs other status codes (break and return)
• Renamed baseDelay to backOff across packages for consistency
• Converted backOff to a package-level variable in engine.go to allow test overrides without changing function signatures

Confidence Score: 5/5

• This PR is safe to merge with minimal risk
• The changes are well-tested with comprehensive integration tests, implement proper HTTP semantics for async operations, and include appropriate test setup/teardown. The refactoring improves code organization without changing core business logic, and the HTTP 202 handling follows REST best practices for long-running operations.
• No files require special attention

Important Files Changed

Filename	Overview
pkg/server/engine.go	Converted backOff to package-level variable with init function, removed delay parameter from processRequestWithRetries
pkg/server/trailing_delay_queue.go	Immediately stores HTTP 202 response when new request ID is created, simplified Get() logic by removing current UID check
pkg/server/integration_test.go	Added backOff override for faster tests, improved retry logic to properly handle HTTP 202 responses with clearer control flow

Sequence Diagram

sequenceDiagram
    participant Client
    participant HttpServer
    participant TrailingDelayQueue
    participant ProcessRequest
    participant LRUCache

    Client->>HttpServer: POST /v1/generate (topology request)
    HttpServer->>TrailingDelayQueue: Submit(request)
    
    alt First submission (new UID)
        TrailingDelayQueue->>TrailingDelayQueue: Generate new UUID
        TrailingDelayQueue->>LRUCache: Store {Status: 202, Message: "request ID xxx not completed"}
        TrailingDelayQueue-->>HttpServer: Return UID
    else Subsequent submission (existing UID)
        TrailingDelayQueue->>TrailingDelayQueue: Update item and lastTime
        TrailingDelayQueue-->>HttpServer: Return existing UID
    end
    
    HttpServer-->>Client: HTTP 202 + UID
    
    Note over TrailingDelayQueue: Background processing after delay
    TrailingDelayQueue->>ProcessRequest: handle(item)
    ProcessRequest->>ProcessRequest: processRequestWithRetries
    
    alt Success
        ProcessRequest-->>TrailingDelayQueue: {Ret: data, Status: 200}
        TrailingDelayQueue->>LRUCache: Update {Ret: data, Status: 200}
    else Error
        ProcessRequest-->>TrailingDelayQueue: {Message: error, Status: 5xx}
        TrailingDelayQueue->>LRUCache: Update {Message: error, Status: 5xx}
    end
    
    Client->>HttpServer: GET /v1/topology?uid=xxx
    HttpServer->>TrailingDelayQueue: Get(uid)
    TrailingDelayQueue->>LRUCache: Get(uid)
    
    alt Request still in progress
        LRUCache-->>TrailingDelayQueue: {Status: 202, Message: "not completed"}
        TrailingDelayQueue-->>HttpServer: Completion{Status: 202}
        HttpServer-->>Client: HTTP 202 + Message
    else Request completed successfully
        LRUCache-->>TrailingDelayQueue: {Status: 200, Ret: data}
        TrailingDelayQueue-->>HttpServer: Completion{Status: 200, Ret: data}
        HttpServer-->>Client: HTTP 200 + topology data
    else Request failed
        LRUCache-->>TrailingDelayQueue: {Status: 5xx, Message: error}
        TrailingDelayQueue-->>HttpServer: Completion{Status: 5xx}
        HttpServer-->>Client: HTTP 5xx + error message
    else UID not found
        LRUCache-->>TrailingDelayQueue: nil
        TrailingDelayQueue-->>HttpServer: Completion{Status: 404}
        HttpServer-->>Client: HTTP 404 + "not found"
    end

Loading

[greptile-apps]

_{3 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{3 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{5 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (2)

pkg/server/engine.go
Global mutable backoff

BackOff is a package-level variable that’s mutated by TestServerIntegration and read by processRequest, so running tests in parallel (or multiple servers in the same process) can race and also leak configuration across tests. Since this is effectively config, it should live on the server/queue instance (or be passed into NewTrailingDelayQueue / processRequestWithRetries) rather than a global var.

---

pkg/server/integration_test.go
Test mutates global state

This test sets the global server.BackOff (BackOff = 100 * time.Millisecond), which can interfere with other tests and will race if tests are ever run with -race and/or in parallel. Prefer wiring backoff through config.Config / initHttpServer so the integration test can set it per server instance without global mutation.

[greptile-apps]

_{3 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{3 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}