feat: hypercore11 migration support - #214
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
gmaclennan
left a comment
There was a problem hiding this comment.
This requires quite a few changes before we can safely merge this.
The retry behaviour currently is broken - it just falls through to forceFallback: true. I also don't understand the reasoning for only allowing a single retry - a user would want to keep freeing up space and retrying until they have enough space.
There is no API for the user to skip the retry and just use the old comapeo (fallback) - at least, not if we fix retry to actually retry instead of being "use fallback".
We can remove the iOS migration code and path - we haven't released anything on iOS so no need to release and maintain that.
See my note about testing - my oversight never plugging backend tests into CI, and it's really important that this code has some CI tests that are required before merge. The tests you added I don't think are running from any test script due to the filename glob.
There is no partial migration state handling, e.g. if migration throws an error before completing (e.g. because disk space runs out, despite our attempts to ensure there is enough space). This code allows a user to go into the fallback in this state (partially migrated) which would result in data corruption. Also if migration does fail due to disk space, this isn't exposed as a low-space error, just a generic backend error. I think this needs a fix in core too - we should detect the partial state in core and forbid opening the v7 core. Also low_space needs to indicate whether it is possible to ignore / delay until later, so the UX can decide whether to show the button that results in the fallback.
| "@sentry/core": "^10.53.0", | ||
| "@sentry/node-core": "^10.53.0", | ||
| "@sentry/opentelemetry": "^10.53.0", | ||
| "comapeo-core-old": "npm:@comapeo/core@^7.1.0", |
There was a problem hiding this comment.
let's pin this as we do other deps.
| * accept fallback) to allow the migration to proceed. A positive value | ||
| * means the migration needs more space than is currently available. | ||
| */ | ||
| spaceNeeded?: number; |
There was a problem hiding this comment.
Nit: This would be easier to work with on the front-end if it's a discriminate union, e.g.
type StateChangeEventPayload = {
state: Exclude<ComapeoState, 'ERROR' | 'LOW_SPACE'>;
} | {
state: 'ERROR';
errorPhase: string;
errorMessage: string;
} | {
state: 'LOW_SPACE';
spaceNeeded: number;
}| * frame (subsequent retries after the first are no-ops in the backend). | ||
| */ | ||
| export function sendRetry(): void { | ||
| nativeModule.sendRetry?.(); |
There was a problem hiding this comment.
Why the nullish optional chaining? Wouldn't it be better to throw rather than do nothing?
| comapeoServicesClient, | ||
| getNotificationPermissionsAsync, | ||
| requestNotificationPermissionsAsync, | ||
| sendRetry, |
There was a problem hiding this comment.
nit: This is confusing naming. Retry what? "send" to where? Maybe call it retryMigration()?
| }); | ||
| // Old and new MapeoManager have incompatible #private fields; cast | ||
| // to the newer type. Both share the same public API surface. | ||
| comapeoManager = /** @type {import("@comapeo/core").MapeoManager} */ ( |
There was a problem hiding this comment.
Rather than cast the type, either redefine the type of comapeoManager to be either the old or the new type, or use this helper:
type Public<T> = { [K in keyof T]: T[K] }
let comapeoManager: Public<MapeoManager>There was a problem hiding this comment.
This is a really useful trick, I've been running into this with mocks in other codebases. 🙇
There was a problem hiding this comment.
not enough because other places ask for the full manager class. Maybe we can change the type exported in core?
| is ControlFrame.Migrating -> setState(JsState.MIGRATING) | ||
| is ControlFrame.LowSpace -> setState( | ||
| JsState.LOW_SPACE, | ||
| mapOf("spaceNeeded" to frame.spaceNeeded.toString()), |
There was a problem hiding this comment.
This is sent as a string, but in JS it's typed as a number, so I don't think this will work as-is?
| * Safe to call from any state — if not parked, the backend ignores the | ||
| * frame (subsequent retries after the first are no-ops in the backend). |
There was a problem hiding this comment.
Is this true? if sent before parked then I think it would break things. Maybe gate this on being parked?
| // boot after a `LOW_SPACE` park. | ||
| Function("sendRetry") { | ||
| val availableDiskSpace = | ||
| java.nio.file.Files.getFileStore(java.nio.file.Paths.get(appContext.persistentFilesDirectory.path)).usableSpace |
There was a problem hiding this comment.
This could throw an IOException - i guess ok since that just bubbles to RN JS?
There was a problem hiding this comment.
Yes, I think we should bubble it
| SentryConfig.readApplicationMetaDataString(this, META_DEFAULT_ONLINE_STYLE_URL) ?: "" | ||
| // 6th positional: available disk space in bytes for migration decision. | ||
| val availableDiskSpace = | ||
| java.nio.file.Files.getFileStore(java.nio.file.Paths.get(dataDir)).usableSpace |
There was a problem hiding this comment.
This can throw a IOException and crash the FGS
There was a problem hiding this comment.
This won't work in Android API < 26, and we support API >= 24.
You can use File(dataDir).usableSpace (java.io.File) but it returns 0 on failure (rather than an error). There is also StorageManager.getAllocatableBytes(storageUuid) which is slightly different because it includes "bytes that could be written" including data Android might evict from other apps' cache folders, but it's API 26+ - we could use this with a fallback.
What do you think should be the failure mode on a device where we can't read free disk space for some reason?
|
|
||
| /** | ||
| * One-shot retry gate: resolves on first `retry` frame from native. | ||
| * Only fires once so a misbehaving native side can't loop the boot. |
There was a problem hiding this comment.
This doesn't make sense to me, is this an agent copying from the comment below on line 103? Why would we only want to allow retry once? A user would want to keep retrying as they keep trying to free up enough space.
|
Going to totally redo how retries work and how we do migration to align it more with how we did it in pre core-react-native comapeo-mobile. |
Based on the existing work we did in mobile