fix(sink): always update requestActiveStartBlock on session init#792
Open
maoueh wants to merge 4 commits into
Open
fix(sink): always update requestActiveStartBlock on session init#792maoueh wants to merge 4 commits into
maoueh wants to merge 4 commits into
Conversation
The Response_Session case in (*Sinker).doRequest short-circuited with a bare `break` when the handler implemented SinkerSessionInitHandler, skipping the `s.requestActiveStartBlock = r.Session.ResolvedStartBlock` assignment. That field is consumed by the Response_ModulesProgress case to compute ProgressMessageLastContiguousBlock for production-mode mapper stages, so a custom session-init handler silently left the metric wrong. Remove the `break` entirely: the custom handler (when present) is still invoked, but the default log line and the requestActiveStartBlock assignment now run unconditionally. The case body is extracted into a documented private method (*Sinker).handleSessionInit for testability. # Conflicts: # docs/release-notes/change-log.md
Document on the SinkerSessionInitHandler interface that HandleSessionInit behaves as an interceptor and does not short-circuit the Sinker's normal session-init handling: the default "session initialized" log line and the internal resolved-start-block bookkeeping still run after the callback.
maoueh
force-pushed
the
bug/sinker-session-init-handler-break
branch
from
1df8f5f to
49c9cc5
Compare
Contributor
🔍 Vulnerabilities of
|
| digest | sha256:5b201c9d7b51d00e27f141a4f9609388f47441edc04dcb7c2134a8db51599942 |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 116 MB |
| packages | 353 |
📦 Base Image ubuntu:24.04
| also known as |
|
| digest | sha256:cdb5fd928fced577cfecf12c8966e830fcdf42ee481fb0b91904eeddc2fe5eff |
| vulnerabilities |   |
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
2 participants
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.
Summary
Fixes a latent bug in
(*Sinker).doRequest'sResponse_Sessionhandling. When the registered handler implementedSinkerSessionInitHandler, the case body did a barebreakright after invokingHandleSessionInit, which skipped the sinker-internal bookkeeping that follows — most importantlys.requestActiveStartBlock = session.ResolvedStartBlock.That field is consumed in the
Response_ModulesProgresscase to identify the contiguous completed range covering the user's resolved start block. WithrequestActiveStartBlockleft at zero, the predicate0 <= r.StartBlock && r.EndBlock >= 0is trivially true, causingProgressMessageLastContiguousBlockto be overwritten by every completed range of the last (mapper) stage in production mode instead of only the contiguous one. Impact is metric-only (no data correctness impact) and latent today since no SF repo currently implementsSinkerSessionInitHandler.Changes:
Response_Sessioncase body into(*Sinker).handleSessionInit. The customSinkerSessionInitHandler.HandleSessionInitcallback (when implemented) now runs as an interceptor: the default "session initialized with remote endpoint" log line and therequestActiveStartBlockassignment always run afterwards, regardless of whether a custom handler is installed.SinkerSessionInitHandlerinterface (sink/types.go) to make explicit that it behaves as an interceptor and does not short-circuit the sinker's default session handling.sink/sinker_test.go.## Unreleased.Test plan
go test ./sink/...passesgo vet ./sink/cleanProgressMessageLastContiguousBlockmetric stays correct for a production-mode mapper sink that implementsSinkerSessionInitHandler