Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Firehose consumers do not receive root block in case of too big seq commit #2893

Closed
MarshalX opened this issue Oct 19, 2024 · 5 comments · Fixed by #2894
Closed

Firehose consumers do not receive root block in case of too big seq commit #2893

MarshalX opened this issue Oct 19, 2024 · 5 comments · Fixed by #2894
Labels
bug Something isn't working

Comments

@MarshalX
Copy link
Contributor

MarshalX commented Oct 19, 2024

Describe the bug

Firehose consumers do not receive root block in case of too big seq commit.

To Reproduce

I wrote a corresponding unit test in the linked PR that reproduces the issue

Actual behavior

car.blocks are always empty

Expected behavior

car.blocks must be always one (the root one)

Additional context

Line no. 29 is so suspicious:

// max 200 ops or 1MB of data
if (writes.length > 200 || commitData.newBlocks.byteSize > 1000000) {
tooBig = true
const justRoot = new BlockMap()
justRoot.add(commitData.newBlocks.get(commitData.cid))
carSlice = await blocksToCarFile(commitData.cid, justRoot)
} else {

Here is why:

  • .add of BlockMap returns a promise and no one waits for resolve (forgotten await)
  • input arg of .add is typed asLexValue which goes through dataToCborBlock(lexToIpld(value)). That does not make sense since we just move the block from one BlockMap to another one. newBlocks.get returns bytes already.

P.S. I dug into this because I got reports from my lovely Python devs that commit.blocks received from the firehose message frame could be missed. But it is marked as a required field in the lexicon. From our observation, it is missed always in combination with tooBig=true. I will continue my journey, but if you have any ideas I would appreciate it

@MarshalX
Copy link
Contributor Author

Hi @dholms! I am pinging you here since you are the last one in the git blame for this file :)

@MarshalX
Copy link
Contributor Author

bump :(

@bnewbold
Copy link
Collaborator

The intention going forward is that the commit block should be included as the root block. It is also possible it isn't getting passed through the relay? We'll need to investigate.

@dholms
Copy link
Collaborator

dholms commented Oct 29, 2024

Hey @MarshalX thanks for flagging this!

I put up a fix for the PDS codebase (#2923). But we seldom if ever output tooBig events on the PDS. I'll flag this in the relay codebase as well which I think is likely the culprit of these missing commits

@dholms
Copy link
Collaborator

dholms commented Oct 29, 2024

Oh rip I didn't see your PR on this 😅

Let me take a closer look, I might prefer yours

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants