mscdex / mscdex/ssh2

sftp.readdir (and other high-level SFTP methods) silently drop callback on connection loss

Open Beginner friendly
#1,490 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
5.8k
Forks
734
PR merge metrics
No merged PRs in 30d

Description

When the remote server crashes or the TCP connection drops abruptly, high-level SFTP methods such as sftp.readdir(path, cb) never invoke their
callback — neither with a result nor with an error. The calling code stalls silently with no way to detect the failure.

Reproducer (https://github.com/jeffrson/ssh2-missing-cb)

  sftp.readdir('/some/path', (err, list) => {
    // never called when server dies mid-request
    console.log(err, list);
  });

  // simulate server crash while readdir is in flight
  setTimeout(() => process.kill(serverPid), 50);

Root cause

cleanupRequests() in SFTP.js is designed to drain all pending _requests with a 'No response from server' error when the channel closes. It is called
correctly from push(null) on connection loss.

The problem is that high-level methods like readdir (string form, lines 886–910) call further SFTP operations inside their error handlers:

  this.readdir(handle, opts, (err, list) => {
    if (err && !eof)
      return this.close(handle, () => cb(err));  // ← adds a new _requests entry
    ...
  });

The sequence on connection drop:

  1. cleanupRequests fires → calls the pending READDIR callback with err
  2. That callback synchronously calls this.close(handle, () => cb(err))
  3. close() registers a new entry in _requests
  4. cleanupRequests has already returned; channel.readable is now false → push(null) will never fire again
  5. The new close request hangs forever → user callback is never called

This affects every high-level SFTP method that calls a further SFTP operation in its error or EOF handler. readdir with a string path is the most
common case, but the pattern recurs elsewhere.

Fix

cleanupRequests must loop until _requests is stably empty, so that any requests added synchronously by cleanup callbacks are also flushed:

  function cleanupRequests(sftp) {
    const err = new Error('No response from server');
    while (true) {
      const keys = Object.keys(sftp._requests);
      if (keys.length === 0)
        return;
      const reqs = sftp._requests;
      sftp._requests = {};
      for (let i = 0; i < keys.length; ++i) {
        const req = reqs[keys[i]];
        if (typeof req.cb === 'function')
          req.cb(err);
      }
    }
  }

Each iteration processes requests added by the previous round's callbacks. The loop terminates because callback chains are finite in depth (e.g. readdir → close → cb → done, depth 2).

Edit: I'm sorry I missed my older report (it's rather old so that's why...): https://github.com/mscdex/ssh2/issues/1175 - however, this time the report contains deeper analysis and has a PR.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in SFTP.js at cleanupRequests() and the high-level readdir implementation around lines 886–910. Run the linked ssh2-missing-cb reproducer, then verify that connection loss drains requests added by error or EOF callbacks and that the user callback receives an error instead of hanging.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
networking
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.