Skip to content

Commit

Permalink
fix: add request length limit for getAllMessagesBySyncIds (#2347)
Browse files Browse the repository at this point in the history
We've observed hub memory usage and disk reads spike over the last
couple days. We'd like to put a size bound on `getAllMessagesBySyncIds`
because it currently accepts an unlimited number of sync ids and could
end up exhausting hub resources.

## Merge Checklist

_Choose all relevant options below by adding an `x` now or at any time
before submitting for review_

- [x] PR title adheres to the [conventional
commits](https://www.conventionalcommits.org/en/v1.0.0/) standard
- [x] PR has a
[changeset](https://github.com/farcasterxyz/hub-monorepo/blob/main/CONTRIBUTING.md#35-adding-changesets)
- [x] PR has been tagged with a change label(s) (i.e. documentation,
feature, bugfix, or chore)
- [ ] PR includes
[documentation](https://github.com/farcasterxyz/hub-monorepo/blob/main/CONTRIBUTING.md#32-writing-docs)
if necessary.

<!-- start pr-codex -->

---

## PR-Codex overview
This PR focuses on adding a request length limit for the
`getAllMessagesBySyncIds` function in the `server.ts` file and
refactoring the message retrieval logic in `syncHealth.ts` to handle
multiple requests efficiently.

### Detailed summary
- Added a check for the length of `syncIds` in `getAllMessagesBySyncIds`
to enforce a maximum limit.
- Changed the message retrieval logic in `syncHealth.ts` to accumulate
messages from multiple requests into `allMessages`.
- Updated the error handling to ensure consistent error responses.

> ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your
question}`

<!-- end pr-codex -->
  • Loading branch information
aditiharini authored Oct 2, 2024
1 parent dbf1f15 commit cda909b
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 4 deletions.
5 changes: 5 additions & 0 deletions .changeset/three-zoos-mix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@farcaster/hubble": patch
---

fix: add request length limit for getAllMessagesBySyncIds
4 changes: 4 additions & 0 deletions apps/hubble/src/rpc/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,10 @@ export default class Server {
}

public async getAllMessagesBySyncIds(request: SyncIds) {
if (request.syncIds.length > MAX_VALUES_RETURNED_PER_SYNC_ID_REQUEST) {
return err(new HubError("bad_request.validation_failure", "Too many sync ids provided"));
}

const syncIds = request.syncIds.map((syncId) => SyncId.fromBytes(syncId));
const messagesResult = await this.syncEngine?.getAllMessagesBySyncIds(syncIds);
if (messagesResult?.isErr()) {
Expand Down
15 changes: 11 additions & 4 deletions apps/hubble/src/utils/syncHealth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -497,14 +497,21 @@ export class SyncHealthProbe {
return ok([]);
}

const messages = await metadataRetrieverWithMessages.getAllMessagesBySyncIds(missingSyncIds);
const allMessages = [];
for (let i = 0; i < missingSyncIds.length; i += this._maxValuesReturnedPerSyncIdRequest) {
const messages = await metadataRetrieverWithMessages.getAllMessagesBySyncIds(
missingSyncIds.slice(i, i + this._maxValuesReturnedPerSyncIdRequest),
);

if (messages.isErr()) {
return err(messages.error);
}

if (messages.isErr()) {
return err(messages.error);
allMessages.push(...messages.value.messages);
}

const results = [];
for (const message of messages.value.messages) {
for (const message of allMessages) {
const result = await metadataRetrieverMissingMessages.submitMessage(message);
const augmentedResult = result.mapErr((err) => {
return { hubError: err, originalMessage: message };
Expand Down

0 comments on commit cda909b

Please sign in to comment.