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

feat(shuttle): Revert unsafe "Support deleting messages on DB that are missing on hub" #2341 #2422

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

tybook
Copy link
Contributor

@tybook tybook commented Dec 9, 2024

Reverts #2341

We found this to be a fundamentally flawed action to take on messages missing in the hub. Reverting the change to not mislead other Shuttle users.

Why is this unsafe? Imagine a simple Shuttle-powered app with a links table written to and deleted from by its handleMessageMerge method. Then imagine this series of Farcaster messages submitted to the network:

  1. LINK_ADD: FID 123 follows FID 234
  2. LINK_REMOVE: FID 123 unfollows FID 234
  3. LINK_ADD: FID 123 refollows FID 234

Obviously the correct final links table state should be FID 123 following FID 234. But with my changes that I'm reverting here, this won't be the case. The hubs will eventually delete message 1, meaning Shuttle's reverse reconciliation would ultimately run handleMessageMerge() with a state arg of "deleted" on message 1. This means the links table would have its row deleted that signifies FID 123 following FID 234, regardless of message 3.

I think it might be safe in general to have this onDbMessage() callback do something like hard delete missing-in-hub messages from the messages table to keep its disk usage in check, but calling into the app to have it perform all downstream deletion processing of the message like this isn't safe.


PR-Codex overview

This PR focuses on reverting a previous change that allowed for the deletion of messages from the database if they were missing on the hub. It simplifies the handleMissingMessage method by removing the conditional check for missingInHub.

Detailed summary

  • Reverted feature: "Support deleting messages on DB that are missing on hub".
  • Modified handleMissingMessage method in hubEventProcessor.ts to remove the missingInHub parameter.
  • Updated calls to handleMissingMessage in app.ts to reflect the removal of the missingInHub argument.

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

Copy link

changeset-bot bot commented Dec 9, 2024

🦋 Changeset detected

Latest commit: 64e5635

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@farcaster/shuttle Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Dec 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hub-monorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 9, 2024 8:30pm

@sanjayprabhu
Copy link
Contributor

Could you please add a changeset?

@tybook tybook changed the title Revert "feat(shuttle): Support deleting messages on DB that are missing on hub" feat(shuttle): Revert unsafe "Support deleting messages on DB that are missing on hub" #2341 Dec 9, 2024
@sanjayprabhu sanjayprabhu merged commit a302bcd into farcasterxyz:main Dec 9, 2024
7 checks passed
Copy link

codecov bot commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (1880070) to head (64e5635).
Report is 435 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #2422       +/-   ##
==========================================
- Coverage   74.05%       0   -74.06%     
==========================================
  Files          99       0       -99     
  Lines        9425       0     -9425     
  Branches     2097       0     -2097     
==========================================
- Hits         6980       0     -6980     
+ Misses       2327       0     -2327     
+ Partials      118       0      -118     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants