release fails on cordova/capacitor apps

[minomikula]

Hi,

We started using your code-push-server and code-push-cli with capacitor plugin https://github.com/mapiacompany/capacitor-codepush and we found some problems. I believe that these problems are not present when used with the react-native release/plugins.

Most serious one is that second release will fail all the time if the output folder is not named CodePush. So "partial" release functionality is broken.

Steps to reproduce:

1. prepare code push: code-push app add test-android android cordova
2. create new folder and file with some content: echo "version 1" > ./test_folder/test_file
3. create release: code-push release test-android ./test_folder "*"
4. change file: echo "version 2" > ./test_folder/test_file
5. release it again: code-push release test-android ./test_folder "*"

there is error on the server, and server instance will stop! (we are not using pm2, just simple node process)

Error: ENOENT: no such file or directory, stat 'C:\....\code-push-demo\workDir\codepush_Ye3hArNTWWsMkBtBAsGeLMEV04BID9dM\e592ede40edcc1a367b8c19d67de83811848f9345b682c39a2efff199a2f8a37\dataCenter\CodePush\test_file'
Emitted 'error' event on ZipFile instance at:
    at C:\...\code-push-demo\node_modules\yazl\index.js:31:26

I tried to investigate and found two problems:

First, this error should not kill whole node process. This is caused by improper error handling on zip file creation: on https://github.com/shm-open/code-push-server/blob/master/src/core/services/package-manager.ts#L233 it is not sufficient to add error handler to zipFile.outputStream.on('error'...).

If we check how ZipFile is throwing exeption https://github.com/thejoshwolfe/yazl/blob/master/index.js
we can see it used like: if (err) return self.emit("error", err); and util.inherits(ZipFile, EventEmitter);. So to catch these exeptions we need to add error handler on the zipFile instance: zipFile.on('error', (error)=>{ reject(error);}).

After this, the error will not kill whole server. Also after restart of the server everything looks ok-ish, because even if partial release failed to be created, the full release is used as fallback. So if someone is using for example pm2 this error could be unnoticed.

Second problem is error itself. The problem is that on https://github.com/shm-open/code-push-server/blob/master/src/core/utils/security.ts#L186-L189, when you are creating manifest file, the root folder is renamed to CodePush. So in my example of test_folder/test_file, the created manifest would contain something like this:

{
  'CodePush/test_file': 'b03d44cd60d71de68a4aca7808c6f768802f6d6c414430ff8ccea10c1aa57b4c'  
}

Then on the https://github.com/shm-open/code-push-server/blob/master/src/core/services/package-manager.ts#L287 in the generateOneDiffPackage function there is comparison of old and new manifest files. The result in my example run is ['CodePush/test_file']. Then this path is used to create zip file containing only changed files. On line https://github.com/shm-open/code-push-server/blob/master/src/core/services/package-manager.ts#L241 there is zipFile.addFile(path.join(baseDirectoryPath, file), slash(file)); but this file does not exist. In my example path is C:\....\code-push-demo\workDir\codepush_Ye3hArNTWWsMkBtBAsGeLMEV04BID9dM\e592ede40edcc1a367b8c19d67de83811848f9345b682c39a2efff199a2f8a37\dataCenter\CodePush\test_file. But without renaming it would be: C:\....\code-push-demo\workDir\codepush_Ye3hArNTWWsMkBtBAsGeLMEV04BID9dM\e592ede40edcc1a367b8c19d67de83811848f9345b682c39a2efff199a2f8a37\dataCenter\test_folder\test_file which exists.

I think if works for you only because during code-push release-react you are writing files to the folder named CodePush. From cli console: node node_modules\react-native\local-cli\cli.js bundle --assets-dest C:\***\Temp\CodePush\CodePush --bundle-output C:\***\Temp\CodePush\CodePush\index.android.bundle .....

But also react native apps do not need the renaming, in that case the renaming is: CodePush->CodePush (if code-push release-react is used or bundle/asset outpus is in CodePush folder). I think the correct fix should be to remove lines https://github.com/shm-open/code-push-server/blob/master/src/core/utils/security.ts#L186-L189.

In real word scenario the cordova builds have builded files in www folder and ionic capacitor .../public folder. In both cases the rename of the folder to CodePush in the manifest paths will result in error on second release.

I found this issue mentioned few times:
#75 (comment) They "fixed" issue by replacing rename.
the same thing here: lisong#319

The rename code originate from this PR: https://github-com.translate.goog/lisong/code-push-server/pull/236?_x_tr_sl=auto&_x_tr_tl=en&_x_tr_hl=en&_x_tr_pto=wapp
Where they created react native build, but not in CodePush folder. I think the proper solution was either to change output folder, or fix hardcoded CodePush folder in the ios plugin. But still I do not understand how it was possible that they did not get the same error that we have now.

Just to double check if the partial update feature was not destroyed after I removed rename code. I tried it and it works without problems on our capacitor app: (note only one src_app_pages_profile_profile_module_ts file was changed)

VM3:192 result Filesystem.readdir (#100067870)
VM3:203 Objectfiles: (2) ['src_app_pages_profile_profile_module_ts.js', 'src_app_pages_profile_profile_module_ts.js.map'][[Prototype]]: Object
VM3:218 native Filesystem.stat (#100067871)
VM3:225 ObjectcallbackId: "100067871"methodName: "stat"options: {directory: 'DATA', path: 'codepush/download/unzipped/public/src_app_pages_profile_profile_module_ts.js'}pluginId: "Filesystem"[[Prototype]]: Object
VM3:192 result Filesystem.stat (#100067871)
....
VM3:192 result Filesystem.readFile (#100067875)
src_app_pages_profile_profile_module_ts.js:1088 [CodePush] Applying diff update

Thanks!
(I can create pull request if you accept, but I don't know how to test react-native/ios part)

[kbrownlees]

As a data point: I am successfully using this package for cordova and have done multiple releases. My only thought is that I would usually alternative between ios / android release, perhaps the change clears something else which makes this work?

My release commands looks something like:

code-push release -d staging --description "abc" app-android build/bundle/www "*"

I also don't appear to be putting a relative link on my folder (build/bundle/www vs ./build/bundle/www).

[rocwind]

@minomikula
Thank you for the investigation!
The proposed fix for the first problem (zipfile) sounds looks good to me, I'd happy to accept a PR for it.

Regarding the second problem (diff package) issue, I have read through the discussion thread(lisong#236), the rename of folder seems necessary to me to get it to work if someone prefer to not using the automated workflow(code-push release-react) but build + upload (react-native bundle +code-push release), where it makes sure the uploaded files matches the expected CodePush/* path at client side and eventually getting the same hash between the server and client.

If the fix for cordova case mentioned in (#75 (comment), lisong#319 (comment)) works, I would like to suggest a safer temporay solution: check current app type and then choose to rename to CodePush or www, it should keep the react-native workflow untouched while fixes cordova issue (maybe ionic capacitor need a seperated app type as its folder structure is different from cordova?). what do you think?

[minomikula]

Hi,

Thanks for the quick response!

I will create PR for the zipfile error handling.

For the folder problem: I understand that that you want to keep possibility of the "manual" workflow for the react native app. But I think that also this workflow is not possible at the moment. I tried to replicate the lisnong#236 but now the server will fail on such request.

> react-native -- bundle --entry-file index.js --platform ios --dev false --bundle-output "/Users/.../Desktop/bundle/main.jsbundle" --assets-dest "/Users/.../Desktop/bundle"
> code-push -- release "reactNative-ios" "/Users/.../Desktop/bundle" "*" -d "Staging" 
(make some changes)
> react-native -- bundle --entry-file index.js --platform ios --dev false --bundle-output "/Users/.../Desktop/bundle/main.jsbundle" --assets-dest "/Users/.../Desktop/bundle"
> code-push -- release "reactNative-ios" "/Users/.../Desktop/bundle" "*" -d "Staging" 

Then server will fail with:

Error: ENOENT: no such file or directory, stat 'C:\...\code-push-demo\workDir\codepush_2TEKlbQ4aQk6uzEobgCQ44fGsMIVNKZX\ddfbef5b9c5be7bba8d959e6dc585feec6ca79dafe8b06c0eab3d897affaceff\dataCenter\CodePush\main.jsbundle'
Emitted 'error' event on ZipFile instance at:
    at C:\...\code-push-demo\node_modules\yazl\index.js:31:26
    at FSReqCallback.oncomplete (node:fs:198:21) {
  errno: -4058,

I think that their rename fix was working in the past, but something has changed in the creation of the zip files, which broke this functionality. Right now every release has to be from folder called 'CodePush', otherwise it will fail.

Keep in mind, that this "fail" of server can be unnoticed for long time.

First this is fail of the "advanced" partial release release. The normal full release is working even if server fails. Second, probably most of the users use some king of automatic restarting of node process. And third the failure is not reported to user who is calling the code-push cli. From their perspective everything is working.

kbrownlees: Could you double check if the "partial release" is really working? In the case that on app there is some code-push release installed, and later there is only one file changed in next release, then only this one file should be transfered. (It is possible to check it in the console, as I showed in my first comment). Also, are you sure that the server did not crashed?

So it is hard to choose what is the correct way to fix this problem. I think that removal of the renaming "fix" and maybe adding documentation for the react-native/ios that folder of the release must be called 'CodePush'... (if the react-native/ios plugin has hard requirement for that folder name)

But in general I do not like the fact, that not only content of the folder containing the release, but also the folder name is used. For example I can build app for the first time in folder 'dist1', release it, then upload it to 'dist2', release it, and there will be mess in the manifests...

[kbrownlees]

@minomikula apologies, it does look like it is failing to handle partial releases correctly. We are using pm2 so it just restarts after writing the main package so it 'works'.

[rocwind]

@minomikula
Thanks for the detailed clarification!
You are right, that "manual" workflow is broken due to the same issue we are discussing here. Please go ahead with PR. I see no reason to reject it.