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

Publishing existing package overrides existing package to 0 bytes file #1191

Closed
pavels555 opened this issue Jan 15, 2019 · 9 comments · Fixed by #1240
Closed

Publishing existing package overrides existing package to 0 bytes file #1191

pavels555 opened this issue Jan 15, 2019 · 9 comments · Fixed by #1240

Comments

@pavels555
Copy link

pavels555 commented Jan 15, 2019

Describe the bug
When trying to publish a package that already exists it says "npm ERR! publish fail Cannot publish over existing version."
But at the same time, something fails at the server and the existing file turns into a 0 bytes size file.

It doesn't happen all the time, but it happens.

To Reproduce
Steps to reproduce the behavior:

  1. Try publishing a bunch of tgz packed packages with the command "npm publish xxxx.tgz" . (that already exist). I added a file at the bottom with some tgzs and script to execute it fast
  2. get inside the server's storage place and execute command "find -iname *.tgz -type f -size -1c".
    It will show you all tgz files under 1 byte.
  3. Try installing the package you found and you'll get integrity error (0 bytes).

Expected behavior
Expected the package to not get overwritten.

Screenshots
Found files that are 0 bytes.
Let's look at the debug package.
terminal

Found in the logs a error that appers only on package that have been destroyed. It is marked
log

Docker || Kubernetes (please complete the following information):

  • Openshift
  • Docker - verdaccio:3.10.1

Configuration File (cat ~/.config/verdaccio/config.yaml)

# path to a directory with all packages
storage: ./storage

auth:
  htpasswd:
    file: ./htpasswd
    # Maximum amount of users allowed to register, defaults to "+inf".
    # You can set this to -1 to disable registration.
    #max_users: 1000

# a list of other known repositories we can talk to



packages:
  '@*/*':
    # scoped packages
    access: $all
    publish: $all

  '**':
    # allow all users (including non-authenticated users) to read and
    # publish all packages
    #
    # you can specify usernames/groupnames (depending on your auth plugin)
    # and three keywords: "$all", "$anonymous", "$authenticated"
    access: $all

    # allow all known users to publish packages
    # (anyone can register by default, remember?)
    publish: $all

    # if package is not available locally, proxy requests to 'npmjs' registry


# To use `npm audit` uncomment the following section
middlewares:
  audit:
    enabled: true

# log settings
logs:
  - {type: stdout, format: pretty, level: http}
  #- {type: file, path: verdaccio.log, level: info}

Debugging output
Logs at the pictures.

Additional context
adding a file with some packages for you to try.
packages-demo.zip

  1. Upload them all with the script.bat inside
  2. Run the script.bat again
  3. Look inside the server for 0 bytes packages.
@mlamothe
Copy link

I've seen this issue on one of our Windows VMs as well (with our own packages, not using @pavels555's demo files. Sometimes it creates an empty file and sometimes it deletes the file.

@mlamothe
Copy link

mlamothe commented Feb 22, 2019

One thing I wanted to note is the server where we don't see this issue is running Windows Server 2008 R2. The one with the issue uses Windows Server 2016.

Poking around the code, it looks like Verdaccio moves the existing tarball out of the way, then tries to put it back on error (I'm not 100% sure, I'm having a hard time following the code, and can't find a way to attach a debugger running through IIS).

My best guess is that this code is what corrupts the file:
Package: @verdaccio\local-storage\lib\local-fs.js
Function: renameTmp

  // but it seem to be able to rename it
  const tmp = tempFile(dst);
  _fs2.default.rename(dst, tmp, function (err) {
    _fs2.default.rename(src, dst, cb);
    if (!err) {
      _fs2.default.unlink(tmp, () => {});
    }
  });

My error file is identical to @pavels555 - including the Write After End message.

@juanpicado
Copy link
Member

juanpicado commented Feb 23, 2019

thanks @pavels555 for diving into it, effectively the issue is the following. If there is an error on fs.exists. If the tarball exist for some reason we are trying to get a temp file, but in that very moment the error emit close the uploadStream and having in account the uploadStream.pipe(file); is piped with file, the open event is emitted, it fails because cannot be opened ERR_STREAM_WRITE_AFTER_END since the stream has already ended.

So, my solution (or what I have on mind) is don't create tmp file and stop the execution there if the file does not exist.

Source of the issue #892
It works fine until v3.5.1

@juanpicado
Copy link
Member

juanpicado commented Feb 24, 2019

I found a solution, it seems at #892 we forgot something essential, the issue is at the local storage plugin. If the resource exists we should not open a file create the stream, it just does not make sense.

I'll publish here a patched version, I'd like you guys to try it in your side, if works fine, I'll merge the patch.

juanpicado added a commit to verdaccio/local-storage that referenced this issue Feb 24, 2019
@juanpicado
Copy link
Member

npm install -g verdaccio@fix-1191 --registry https://registry.verdaccio.org

juanpicado added a commit that referenced this issue Feb 24, 2019
Publishing existing package overrides existing package to 0 bytes file
@mlamothe
Copy link

I installed the patch and it looks like the bug is fixed - thank you!!!

@juanpicado
Copy link
Member

Perfect ! I will release it ! Thanks for the quick test

juanpicado added a commit that referenced this issue Feb 25, 2019
fix: overrides existing package to zero bytes file #1191
@juanpicado
Copy link
Member

[email protected]

@lock
Copy link

lock bot commented Jun 29, 2019

🤖This thread has been automatically locked 🔒 since there has not been any recent activity after it was closed.
We lock tickets after 90 days with the idea to encourage you to open a ticket with new fresh data and to provide you better feedback 🤝and better visibility 👀.
If you consider, can attach this ticket 📨to the new one as a reference for better context.
Thanks for being a part of the Verdaccio community! 💘

@lock lock bot added the outdated label Jun 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants