Repository navigation
Fix flow-state merge freeze on repeated PROGRESS snapshots - #183
Merged
Merged
Conversation
## Problem
`mergeRecords()` in `services/launcher/flows.go` only replaces the
accumulated
`QueryStats` when the incoming stats record represents a genuine
transition
from running to a terminal state:
```go
if len(stats_context.QueryStats) > 0 {
if len(collection_context.QueryStats) == 0 ||
is_running(collection_context) && !is_running(stats_context) {
collection_context.QueryStats = stats_context.QueryStats
}
}
```
Two consecutive still-running (`PROGRESS`) snapshots never update it. If
a
flow's query never reaches a terminal state (e.g. it's stuck on a slow
upload), the collection context is permanently pinned at the very first
progress snapshot it ever received - even though the client keeps
sending
fresher progress for as long as the query runs. Since `UpdateFlowStats`
derives `TotalUploadedBytes`, `TotalCollectedRows`, `TotalLogs`,
`State`, etc.
purely from `QueryStats`, the whole flow view gets stuck showing
stale/empty
progress and never reflects what the client has actually done.
## Fix
Each `FlowStats` message the client sends already contains the complete,
cumulative status of every query in the flow so far (built by iterating
the
full `responders` slice - see `responder.FlowContext.getStats()`
upstream),
never a partial delta. So it's always safe to adopt the latest snapshot
wholesale as long as the accumulator hasn't reached a terminal state
yet:
```go
if len(stats_context.QueryStats) > 0 {
if len(collection_context.QueryStats) == 0 ||
is_running(collection_context) {
collection_context.QueryStats = stats_context.QueryStats
}
}
```
This still freezes correctly once the accumulator reaches a terminal
state
(so a stray/out-of-order message arriving after completion doesn't
reopen
it), it just stops discarding fresher in-progress snapshots.
## Testing
Added `TestFlowStatsProgressNotFrozen` in `ingestion/ingestor_test.go`,
which sends two consecutive `PROGRESS` snapshots via
`Ingestor.HandleFlowStats` with increasing row/byte counts and asserts
`GetFlowDetails` reflects the second, fresher snapshot rather than
staying
pinned at the first. Verified it fails against the old logic and passes
with
the fix. Also ran the full existing suite (`go test -p 1 ./...`) - no
regressions; `TestLauncher` and `TestListDirectory` are unaffected since
they only exercise a single running->terminal transition, where old and
new
logic agree.
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
smccooey-r7
force-pushed
the
mergeRecords-merge-freeze-fix
branch
from
October 5, 2026 12:34
2a6988f to
f12c6da
Compare
The ingestion round-trip tests added with the fix drive GetFlowDetails, which reads through the per-client flow cache and the lazily-rebuilt flow index. HandleFlowStats does not invalidate that cache, so a second write is not observable by a subsequent read and the tests cannot pass against the optimised read path. Replace them with TestMergeRecordsProgressNotFrozen and TestMergeRecordsCompletionRecorded in services/launcher/flows_test.go. Both fold stats snapshots through mergeRecords exactly as buildIndex does and then derive the summary via UpdateFlowStats - the same merge path the fix changes - so they pin the behaviour deterministically with no backend. ProgressNotFrozen fails against the old condition and passes with the fix; CompletionRecorded guards that a terminal snapshot stays recorded and is not reopened by a later stray PROGRESS.
smccooey-r7
force-pushed
the
mergeRecords-merge-freeze-fix
branch
from
October 5, 2026 12:37
f12c6da to
18e170e
Compare
awhite-r7
pushed a commit
to awhite-r7/cloudvelo
that referenced
this pull request
Oct 8, 2026
Brings in upstream Velocidex#173, Velocidex#177, Velocidex#179, Velocidex#180, Velocidex#181, Velocidex#182 and Velocidex#183. Conflicts resolved to the fork side: - services/client_info/client_info.go: GetStats keeps the wrapped utils.NotFoundError; the fork no longer declares the package-level NotFoundError upstream returns. - .github/workflows/test.yml: keeps opensearch-version 2.19.6, already within upstream's v2 pin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
Porting the content of #174 to master
Original PR description:
Problem
mergeRecords()inservices/launcher/flows.goonly replaces the accumulatedQueryStatswhen the incoming stats record represents a genuine running → terminal transition. Two consecutive still-running (PROGRESS) snapshots therefore never update it. If a flow's query never reaches a terminal state (e.g. it is stuck on a slow upload), the collection context stays pinned at the very first progress snapshot it received, even though the client keeps sending fresher progress. BecauseUpdateFlowStatsderivesTotalUploadedBytes,TotalCollectedRows,TotalLogs,State, etc. purely fromQueryStats, the whole flow view gets stuck showing stale progress.Fix
Each
FlowStatsmessage already carries the complete, cumulative status of every query so far (never a partial delta), so it is always safe to adopt the latest snapshot wholesale while the accumulator has not yet reached a terminal state. The merge condition drops the&& !is_running(stats_context)clause. This still freezes correctly once the accumulator reaches a terminal state (a stray out-of-order message after completion will not reopen it); it just stops discarding fresher in-progress snapshots.Testing
Adds
TestFlowStatsProgressNotFrozeniningestion/ingestor_test.go, which sends two consecutivePROGRESSsnapshots with increasing row/byte counts and assertsGetFlowDetailsreflects the second, fresher snapshot rather than staying pinned at the first. Fails against the old logic, passes with the fix.