Skip to content

GUACAMOLE-2180: Improve SFTP upload performance #629

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.

Sign up for GitHub

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

Open
alailsonko wants to merge 2 commits into
base: main
Choose a base branch
from
Open

GUACAMOLE-2180: Improve SFTP upload performance #629

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2706080
GUACAMOLE-2180: Improve SFTP upload performance by implementing buffe…
alailsonko Dec 15, 2025
40a3726
GUACAMOLE-2180: Document SFTP upload state structure and flush function.
alailsonko Jan 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
184 changes: 161 additions & 23 deletions src/common-ssh/sftp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,68 @@
#include <stdlib.h>
#include <string.h>

/*
* Size of the per-upload buffer, in bytes.
*
* SFTP packets carry up to 32KB of payload, but libssh2_sftp_write() can
* accept larger buffers and will internally split them across multiple SFTP
* packets.
*/
#define GUAC_COMMON_SSH_SFTP_UPLOAD_BUFFER_SIZE (256 * 1024)

typedef struct guac_common_ssh_sftp_upload_state {

@necouchman necouchman Jan 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide overall documentation for the data structure.

@alailsonko alailsonko Jan 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation added on commit 40a3726

LIBSSH2_SFTP_HANDLE* file; /* Underlying SFTP file handle */
char buffer[GUAC_COMMON_SSH_SFTP_UPLOAD_BUFFER_SIZE]; /* Buffered upload data */
int buffered_length; /* Bytes currently buffered */
int error; /* Non-zero if a write error occurred */
} guac_common_ssh_sftp_upload_state;

/**
* Flushes any buffered upload data to the remote SFTP file.
*
* Partial writes are retried until complete or an error occurs. On error, the
* upload state is marked so that subsequent blobs can report failure.
*
* NOTE: libssh2_sftp_write() may return a short count even when no error has
* occurred. A short return is still progress. Continue calling until the
* entire buffer has been consumed before reusing or modifying it.
*
* @param user
* The user receiving the upload.
*
* @param state
* Per-stream upload state containing the SFTP handle and buffered data.
*
* @return
* Nothing.
*/
static void guac_common_ssh_sftp_upload_flush(guac_user* user,
guac_common_ssh_sftp_upload_state* state) {

if (state->error || state->buffered_length <= 0 || state->file == NULL)
return;

int offset = 0;
while (offset < state->buffered_length) {
ssize_t written = libssh2_sftp_write(state->file,
state->buffer + offset, state->buffered_length - offset);
if (written <= 0) {
guac_user_log(user, GUAC_LOG_INFO,
"Buffered SFTP write failed after %d bytes (attempt returned %zd)",
offset, written);
state->error = 1;
break;
}
offset += (int)written;
}

if (!state->error)
guac_user_log(user, GUAC_LOG_DEBUG,
"Flushed %d buffered bytes to remote file", state->buffered_length);

state->buffered_length = 0;
}
Comment on lines +122 to +177

@necouchman necouchman Jan 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some documentation throughout this function would be nice.

@alailsonko alailsonko Jan 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation added on commit 40a3726


int guac_common_ssh_sftp_normalize_path(char* fullpath,
const char* path) {

Expand Down Expand Up @@ -307,26 +369,69 @@ static int guac_ssh_append_path(char* fullpath, const char* path_a,
*/
static int guac_common_ssh_sftp_blob_handler(guac_user* user,
guac_stream* stream, void* data, int length) {
guac_common_ssh_sftp_upload_state* state =
(guac_common_ssh_sftp_upload_state*) stream->data;

/* Pull file from stream */
LIBSSH2_SFTP_HANDLE* file = (LIBSSH2_SFTP_HANDLE*) stream->data;
if (state == NULL) {
guac_user_log(user, GUAC_LOG_WARNING, "SFTP upload blob received with missing state");
guac_protocol_send_ack(user->socket, stream, "SFTP: Invalid state",
GUAC_PROTOCOL_STATUS_SERVER_ERROR);
guac_socket_flush(user->socket);
return 0;
}

/* Attempt write */
if (libssh2_sftp_write(file, data, length) == length) {
guac_user_log(user, GUAC_LOG_DEBUG, "%i bytes written", length);
guac_protocol_send_ack(user->socket, stream, "SFTP: OK",
GUAC_PROTOCOL_STATUS_SUCCESS);
/* If previous flush failed, report failure immediately */
if (state->error) {
guac_user_log(user, GUAC_LOG_WARNING, "SFTP upload blob received after previous write failure");
guac_protocol_send_ack(user->socket, stream, "SFTP: Previous write failed",
GUAC_PROTOCOL_STATUS_SERVER_ERROR);
guac_socket_flush(user->socket);
return 0;
}

const char* incoming = (const char*) data;
int remaining = length;

/*
* Copy the incoming blob into the upload buffer. We always flush when the
* buffer is full so that subsequent blobs start with space available and
* the buffer can be reused without reallocations.
*/
while (remaining > 0 && !state->error) {
/* If the buffer is already full, flush before copying more data. */
if (state->buffered_length == GUAC_COMMON_SSH_SFTP_UPLOAD_BUFFER_SIZE) {
guac_common_ssh_sftp_upload_flush(user, state);
/* Defensive: bail if error occurred during flush. */
if (state->error)
break;

/* Defensive: bail if flush did not reset buffered_length. */
if (state->buffered_length == GUAC_COMMON_SSH_SFTP_UPLOAD_BUFFER_SIZE)
break;
}

/* Copy as much of the remaining blob as will fit in the buffer. */
int space = GUAC_COMMON_SSH_SFTP_UPLOAD_BUFFER_SIZE - state->buffered_length;
int to_copy = remaining < space ? remaining : space;

memcpy(state->buffer + state->buffered_length, incoming, to_copy);
state->buffered_length += to_copy;
incoming += to_copy;
remaining -= to_copy;
}

/* Inform of any errors */
/* ACK immediately after buffering (fast path) */
if (!state->error) {
guac_user_log(user, GUAC_LOG_DEBUG, "SFTP upload buffered %d bytes (ack success)", length);
guac_protocol_send_ack(user->socket, stream, "SFTP: OK",
GUAC_PROTOCOL_STATUS_SUCCESS);
}
else {
guac_user_log(user, GUAC_LOG_INFO, "Unable to write to file");
guac_user_log(user, GUAC_LOG_WARNING, "SFTP upload blob failed after buffering attempt");
guac_protocol_send_ack(user->socket, stream, "SFTP: Write failed",
GUAC_PROTOCOL_STATUS_SERVER_ERROR);
guac_socket_flush(user->socket);
}

guac_socket_flush(user->socket);
return 0;

}
Expand All @@ -348,24 +453,47 @@ static int guac_common_ssh_sftp_blob_handler(guac_user* user,
*/
static int guac_common_ssh_sftp_end_handler(guac_user* user,
guac_stream* stream) {
guac_common_ssh_sftp_upload_state* state =
(guac_common_ssh_sftp_upload_state*) stream->data;

/* Pull file from stream */
LIBSSH2_SFTP_HANDLE* file = (LIBSSH2_SFTP_HANDLE*) stream->data;
if (state == NULL) {
guac_protocol_send_ack(user->socket, stream, "SFTP: Invalid state",
GUAC_PROTOCOL_STATUS_SERVER_ERROR);
guac_socket_flush(user->socket);
return 0;
}

/* Flush any remaining buffered data */
guac_common_ssh_sftp_upload_flush(user, state);

/* Attempt to close file */
if (libssh2_sftp_close(file) == 0) {
guac_user_log(user, GUAC_LOG_DEBUG, "File closed");
int close_ok = 0;
if (state->file != NULL && !state->error) {
if (libssh2_sftp_close(state->file) == 0) {
guac_user_log(user, GUAC_LOG_DEBUG, "File closed");
close_ok = 1;
}
else {
guac_user_log(user, GUAC_LOG_INFO, "Unable to close file");
}
}

if (close_ok && !state->error) {
guac_protocol_send_ack(user->socket, stream, "SFTP: OK",
GUAC_PROTOCOL_STATUS_SUCCESS);
guac_socket_flush(user->socket);
}
else if (state->error) {
guac_protocol_send_ack(user->socket, stream, "SFTP: Write failed (flush)",
GUAC_PROTOCOL_STATUS_SERVER_ERROR);
}
else {
guac_user_log(user, GUAC_LOG_INFO, "Unable to close file");
guac_protocol_send_ack(user->socket, stream, "SFTP: Close failed",
GUAC_PROTOCOL_STATUS_SERVER_ERROR);
guac_socket_flush(user->socket);
}
guac_socket_flush(user->socket);

/* Free state */
guac_mem_free(state);
stream->data = NULL;
return 0;

}
Expand Down Expand Up @@ -434,8 +562,13 @@ int guac_common_ssh_sftp_handle_file_stream(
stream->blob_handler = guac_common_ssh_sftp_blob_handler;
stream->end_handler = guac_common_ssh_sftp_end_handler;

/* Store file within stream */
stream->data = file;
/* Allocate and initialize upload state */
guac_common_ssh_sftp_upload_state* state =
guac_mem_alloc(sizeof(guac_common_ssh_sftp_upload_state));
state->file = file;
state->buffered_length = 0;
state->error = (file == NULL) ? 1 : 0; /* Mark error if file open failed */
stream->data = state;
return 0;

}
Expand Down Expand Up @@ -917,8 +1050,13 @@ static int guac_common_ssh_sftp_put_handler(guac_user* user,
stream->blob_handler = guac_common_ssh_sftp_blob_handler;
stream->end_handler = guac_common_ssh_sftp_end_handler;

/* Store file within stream */
stream->data = file;
/* Allocate and initialize upload state */
guac_common_ssh_sftp_upload_state* state =
guac_mem_alloc(sizeof(guac_common_ssh_sftp_upload_state));
state->file = file;
state->buffered_length = 0;
state->error = (file == NULL) ? 1 : 0;
stream->data = state;

guac_socket_flush(user->socket);
return 0;
Expand Down