fix: don't fork the consolidation chain on duplicate report rows - #361
Conversation
consolidateContinuousTimeSeries walks rows by matching j's PreviousReportTimestamp against the next row's ReportTimestamp. Two rows that instead share the exact same (previousReportTimestamp, reportTimestamp) pair - same logical report, split into two physical objects - don't match that check, so they get treated as a fork instead of one link. The series never collapses to a single row, and updateProfileStatus's completion check (len == 1) never fires. Add a case for the duplicate-pair situation so those rows count as one link instead of two. Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Traced the new switch in consolidateContinuousTimeSeries through several scenarios (dup pair alone, dup pair chained to a real predecessor, 3-way duplicate collapse, duplicate immediately followed by a genuine fork) — all collapse/link correctly, and the new case is mutually exclusive with the existing chain-link case so no regression to the normal walk.
Confirmed the dropped duplicate row doesn't lose data: mergeTimeSeriesData iterates the original unconsolidated slice (independent of consolidateContinuousTimeSeries), so both physical rows' TS profile blobs still get merged into the profile and both get queued for deletion via deleteTimeSeries; consolidateContinuousTimeSeries only decides what's written back as the surviving chain state.
Regression test fails on main / passes with the fix, matches the described bug precisely. No blockers — approving.
2 participants
Found via #352, filed off the node-agent#866 review discussion.
`consolidateContinuousTimeSeries` walks time series rows in reverse chronological order and links row `j` to row `i+1` when `j.PreviousReportTimestamp == (i+1).ReportTimestamp`. That's correct for real chain links, but two rows can also share the exact same `(previousReportTimestamp, reportTimestamp)` pair when a single logical report got split into two physical objects client-side (e.g. a chunk that came back over the queue's size limit and got resent as two halves). Neither row's timestamps chain to the other, so the walk treats them as a fork and `newTimeSeries` ends up with two rows for what should be one. `updateProfileStatus`'s completion check only fires when `len(newTimeSeries) == 1`, so the profile just... never completes.
Added a case that recognizes the duplicate-pair situation and treats it as one link instead of a fork.
Test: `TestConsolidateContinuousTimeSeries_DuplicateReportRowsCollapse`, fails on main (asserts 1 result, gets 2), passes with the fix. Ran the rest of the `Consolidate*` suite too, nothing regressed.