[I] [Bug][Collector] Stateful collector silently discards the final page when ResponseParser returns items with ErrFinishCollect [devlake]

["michaelgriskified (via GitHub)" <gi...@apache.org>]

michaelgriskified opened a new issue, #9167:
URL: https://github.com/apache/devlake/issues/9167

   
   ### Description
   
   `api_collector_stateful.go`'s `ResponseParser` wrapper discards the records returned by the
   plugin's own parser whenever that parser signals `ErrFinishCollect`. The result is a **silent,
   page-aligned data loss** at the `timeAfter` boundary: the last page of a time-bounded collection
   is dropped in full, including the records on it that are inside the requested window.
   
   Nothing errors. The task reports `TASK_COMPLETED` with zero failed subtasks, and every
   internal consistency check passes, because the tool tables and the domain tables agree with
   each other — they are both derived from the same truncated raw data. The loss is only visible
   by comparing against the upstream API directly.
   
   ### Root cause
   
   `backend/helpers/pluginhelper/api/api_collector_stateful.go`:
   
   ```go
   ResponseParser: func(res *http.Response) ([]json.RawMessage, errors.Error) {
       items, err := args.CollectNewRecordsByList.ResponseParser(res)
       if err != nil {
           return nil, err          // <-- discards `items` even when err is ErrFinishCollect
       }
   ```
   
   Plugin parsers are written to return the valid prefix of a page *together with* the stop signal.
   For example `backend/plugins/circleci/tasks/pipeline_collector.go`:
   
   ```go
   for _, item := range data.Items {
       pipelineCreatedAt, err := extractCreatedAt(item)
       if err != nil { return nil, err }
       if pipelineCreatedAt.Before(*timeAfter) {
           return filteredItems, api.ErrFinishCollect   // in-window items + stop
       }
       filteredItems = append(filteredItems, item)
   }
   ```
   
   The wrapper replaces `filteredItems` with `nil`. `ApiCollector.fetchAsync` then takes the
   `count == 0` early return and never reaches `db.Create`, so the page is never persisted:
   
   ```go
   items, err := collector.args.ResponseParser(res)
   if err != nil {
       if errors.Is(err, ErrFinishCollect) {
           logger.Info("a fetch stop by parser, reqInput: #%s", reqData.Params)
           handler = nil
       } ...
   }
   count := len(items)
   if count == 0 {
       collector.args.Ctx.IncProgress(1)
       return nil          // <-- page silently dropped
   }
   ```
   
   Note `fetchAsync` itself handles this correctly — it persists whatever it is given before
   stopping pagination. The loss is introduced purely by the wrapper nulling the slice.
   
   This affects any plugin using `NewStatefulApiCollectorForFinalizableEntity` whose
   `ResponseParser` returns a partial page with `ErrFinishCollect`, not only circleci.
   
   ### Regression lineage
   
   The circleci parser gained its `return filteredItems, api.ErrFinishCollect` line in #7820
   (merged 2024-08-15), which fixed #7797 "CircleCI pipelines collected from before time range".
   
   That plugin-side change is correct on its own terms — it stops collection at the window
   boundary instead of running past it. But because the stateful wrapper nulls the slice whenever
   the parser returns an error, the fix effectively traded *collecting too much* for *silently
   collecting too little*. #7797's symptom was visible in the data; this one is not.
   
   ### Impact
   
   Loss is bounded by one page, so it is **proportionally worst for low-volume scopes** — the
   opposite of where it is likely to be noticed. Measured on three repositories with
   `timeAfter` = 90 days, `PageSize` = 20 (the CircleCI default):
   
   | repository | upstream API | collected | lost | loss |
   |---|---|---|---|---|
   | A (0.38 pipelines/day) | 34 | 20 | 14 | **41%** |
   | B (3.1 pipelines/day) | 276 | 260 | 16 | 5.8% |
   | C (6.3 pipelines/day) | 568 | 560* | 8 | 1.4% |
   
   \* predicted from the same model; C was collected after the workaround was applied and
   returned the full 568.
   
   ### Reproduction
   
   1. Configure a CircleCI connection and a project scope for a repository with more pipelines in
      the window than one page (>20), where the window boundary falls mid-page.
   2. Set a blueprint `timeAfter` such that the boundary page contains both in-window and
      out-of-window pipelines.
   3. Run the blueprint. It completes `TASK_COMPLETED`, zero failed subtasks.
   4. `SELECT count(*) FROM _tool_circleci_pipelines WHERE project_slug = '...'` returns an exact
      multiple of the page size.
   5. Compare with `GET /v2/project/{slug}/pipeline` counted directly over the same window — the
      difference is the in-window records on the discarded page.
   
   Confirming signals, all consistent:
   
   - `_raw_circleci_api_pipelines` holds the same truncated count, so the loss is at collection,
     not extraction.
   - Collected pipeline `number` values are perfectly contiguous — the tail is amputated, records
     are not dropped at random.
   - A `fullSync: true` re-run reproduces the identical count.
   - **In-band detector:** `collectPipelines.finishedRecords` counts pages fetched while
     `extractPipelines.finishedRecords` counts records kept; `pages - kept/PageSize` is the number
     of discarded pages. This agreed with the `a fetch stop by parser` log line on exactly the runs
     that lost data.
   
   ### Suggested fix
   
   Preserve the partial page when the stop signal is `ErrFinishCollect`:
   
   ```go
   items, err := args.CollectNewRecordsByList.ResponseParser(res)
   if err != nil {
       if errors.Is(err, ErrFinishCollect) {
           return items, err
       }
       return nil, err
   }
   ```
   
   `fetchAsync` already persists the returned items and then stops paginating, so no other change
   is needed.
   
   ### Workaround for operators
   
   Set `timeAfter` earlier than the window you actually need, so the discarded page falls in the
   buffer rather than in the data you rely on. The buffer must exceed **one page expressed in
   time**, which varies with each scope's rate: at `PageSize` 20 that is ~3 days for a repository
   at 6.3 pipelines/day but ~53 days for one at 0.38/day. A fixed buffer chosen for busy scopes
   will not protect quiet ones.
   
   ### Environment
   
   - DevLake `v1.0.3-beta17` (`8fe26f4`), Helm chart `1.0.3-beta9`
   - PostgreSQL backend
   - Plugins: circleci, github_graphql
   - `PIPELINE_MAX_PARALLEL=1`, `API_REQUESTS_PER_HOUR=3000`
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@devlake.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [Bug][Collector] Stateful collector silently discards the final page when ResponseParser returns items with ErrFinishCollect [devlake]

["klesh (via GitHub)" <gi...@apache.org>]

klesh closed issue #9167: [Bug][Collector] Stateful collector silently discards the final page when ResponseParser returns items with ErrFinishCollect
URL: https://github.com/apache/devlake/issues/9167


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [Bug][Collector] Stateful collector silently discards the final page when ResponseParser returns items with ErrFinishCollect [devlake]

["ashok-gk (via GitHub)" <gi...@apache.org>]

ashok-gk commented on issue #9167:
URL: https://github.com/apache/devlake/issues/9167#issuecomment-5848739251

   Hi, I’m interested in working on this issue. I reviewed the `ResponseParser `behavior in the stateful collector and understand the problem to be that items returned together with `ErrFinishCollect` can be discarded, causing the final page of an otherwise successful collection to be lost. Before I start, could you confirm that preserving the returned items while still terminating collection is the intended fix and that this issue is available for an external contribution? I would also plan to add regression coverage for the final-page boundary case.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org