[swupdate][PATCH] feat: recover from interrupted range requests in delta handler

[Caleb Hensley]

With the channel_get_file fix submitted previous to this patch, interrupted
range requests now return an error instead of being resumed, so the delta
handler can recover from it depending on the error type. The chunks downloader
knows how many bytes it forwarded, but the delta handler knows which of them
were turned into a verified chunk and written to the chained handler.

The downloader keeps a retry budget, taken from retry and retrywait of the delta
section, and reports a retryable transfer error as RANGE_RESTART rather than
RANGE_ERROR. It sleeps after the handler has been notified, so the next request
is already prepared when it wakes up, and resets the count on every completed
request so the budget covers one outage instead of capping the interruptions of
a long download.

The handler answers RANGE_RESTART by dropping the state of the running request
and asking for ranges again. network_process_data() only advances priv->chunk
after copyfile() verified the chunk hash, so priv->chunk is the first chunk that
was not written yet and zchunk_get_missing_range() computes the remaining ranges
from it. The partially filled buffer is discarded: it never reached copyfile(),
so nothing of it reached the chained handler. Chunks available in the source are
checked and copied first: they can be next if the transfer stopped between two parts.

The answer to the new request is a new multipart body, so the boundary is
dropped with the rest of the state. Headers arriving while data is expected are
now ignored instead of being collected as payload.

Also included is a fix to the channel settings parsed type for retrywait.

---
corelib/server_utils.c | 6 ++--
handlers/delta_downloader.c | 43 ++++++++++++++++++++++++-
handlers/delta_handler.c | 63 +++++++++++++++++++++++++++++++++++--
handlers/delta_handler.h | 3 +-
4 files changed, 107 insertions(+), 8 deletions(-)

diff --git a/corelib/server_utils.c b/corelib/server_utils.c
index 13923d50..de91a831 100644
--- a/corelib/server_utils.c
+++ b/corelib/server_utils.c
@@ -24,6 +24,8 @@ int channel_settings(void *elem, void *data)

GET_FIELD_INT(LIBCFG_PARSER, elem, "retry",
(int *)&chan->retries);
+ GET_FIELD_INT(LIBCFG_PARSER, elem, "retrywait",
+ (unsigned int *)&chan->retry_sleep);

GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "max-download-speed", tmp);
if (strlen(tmp)) {
@@ -32,10 +34,6 @@ int channel_settings(void *elem, void *data)
WARN("max-download-speed setting %s: ustrtoull failed", tmp);
}

- GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "retrywait", tmp);
- if (strlen(tmp))
- chan->retry_sleep =
- (unsigned int)strtoul(tmp, NULL, 10);
GET_FIELD_BOOL(LIBCFG_PARSER, elem, "nocheckcert", &tmp_bool);
chan->strictssl = !tmp_bool;
GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "cafile", tmp);
diff --git a/handlers/delta_downloader.c b/handlers/delta_downloader.c
index fb8493a6..2a9458e9 100644
--- a/handlers/delta_downloader.c
+++ b/handlers/delta_downloader.c
@@ -71,6 +71,16 @@ static channel_data_t channel_data_defaults = {
.received_headers = NULL
};

+/*
+ * Errors worth asking for the ranges again: when the connection dropped or was
+ * never established. Anything else (not found, ranges not supported, bad
+ * credentials, etc.) does not get better by repeating the request.
+ */
+static bool range_request_retryable(channel_op_res_t result)
+{
+ return result == CHANNEL_EAGAIN || result == CHANNEL_ENONET;
+}
+
/*
* Data callback: takes the buffer, surrounded with IPC meta data
* and send to the process that reqeusted the download
@@ -219,6 +229,9 @@ int start_delta_downloader(const char __attribute__ ((__unused__)) *fname,
channel_settoken("TargetToken", dwldata.targettoken, &channel_data);
channel_settoken("GatewayToken", dwldata.gatewaytoken, &channel_data);

+ /* Track consecutive failures per range request */
+ unsigned int failed_attempts = 0;
+
for (;;) {
ret = read(sw_sockfd, req, sizeof(range_request_t));
if (ret < 0) {
@@ -256,13 +269,41 @@ int start_delta_downloader(const char __attribute__ ((__unused__)) *fname,
}

answer->id = req->id;
- answer->type = (result == CHANNEL_OK) ? RANGE_COMPLETED : RANGE_ERROR;
answer->len = 0;
+
+ /*
+ * An interrupted range request cannot be resumed in the curl handler.
+ * see channel_get_file(). The delta handler knows which chunks it
+ * already passed to the chained handler for disk write, so let the
+ * delta handler compute a new set of ranges instead of failing the
+ * whole update.
+ */
+ if (result == CHANNEL_OK) {
+ failed_attempts = 0;
+ answer->type = RANGE_COMPLETED;
+ } else if (range_request_retryable(result) &&
+ failed_attempts < channel_data.retries) {
+ failed_attempts++;
+ WARN("Range request interrupted, asking for new ranges in %u "
+ "seconds (attempt %u of %u)", channel_data.retry_sleep,
+ failed_attempts, channel_data.retries);
+ answer->type = RANGE_RESTART;
+ } else {
+ if (range_request_retryable(result))
+ ERROR("Range request still failing after %u "
+ "attempts, giving up", failed_attempts);
+ answer->type = RANGE_ERROR;
+ }
+
if (write(sw_sockfd, answer, sizeof(*answer)) != sizeof(*answer)) {
ERROR("Answer cannot be sent back, maybe deadlock !!");
}

(void)channel->close(channel);
+
+ /* retry intervals */
+ if (answer->type == RANGE_RESTART && channel_data.retry_sleep)
+ sleep(channel_data.retry_sleep);
}

exit (EXIT_SUCCESS);
diff --git a/handlers/delta_handler.c b/handlers/delta_handler.c
index 665ac844..f6909b59 100644
--- a/handlers/delta_handler.c
+++ b/handlers/delta_handler.c
@@ -115,6 +115,7 @@ struct hnd_priv {
size_t rangestart; /* Value from Content-range header */
bool content_range_received; /* Flag to indicate that last header is content-range */
bool error_in_parser; /* Flag to report if an error occurred */
+ bool restart_requested; /* Downloader asks for a new set of ranges */
multipart_parser *parser; /* pointer to parser, allocated at any download */
/* Some nice statistics */
size_t bytes_to_be_reused;
@@ -595,6 +596,26 @@ static void dwl_cleanup(struct hnd_priv *priv)
priv->parser = NULL;
}

+/*
+ * Drop everything collected for the running request, so that a new one can be
+ * started from scratch. The chunk being filled is discarded: it never reached
+ * copyfile(), and never reached the chained handler.
+ */
+static void reset_dwl_state(struct hnd_priv *priv)
+{
+ free(priv->current.buf);
+ priv->current.buf = NULL;
+ priv->current.nbytes = 0;
+ priv->current.chunksize = 0;
+ dwl_cleanup(priv);
+ priv->boundary[0] = '\0';
+ priv->range_type = NONE_RANGE;
+ priv->content_range_received = false;
+ priv->error_in_parser = false;
+ priv->dwlrunning = false;
+ priv->dwlstate = NOTRUNNING;
+}
+
static bool read_and_validate_package(struct hnd_priv *priv)
{
ssize_t nbytes = sizeof(range_answer_t);
@@ -623,10 +644,19 @@ static bool read_and_validate_package(struct hnd_priv *priv)
} while (answer->id != priv->reqid);


+ /*
+ * The downloader decided that another request is needed, and it is sleeping
+ * before serving it. The state machine restarts from the first chunk that
+ * was not written yet.
+ */
+ if (answer->type == RANGE_RESTART) {
+ priv->restart_requested = true;
+ return true;
+ }
+
if (answer->type == RANGE_ERROR) {
ERROR("Transfer was unsuccessful, aborting...");
- priv->dwlrunning = false;
- dwl_cleanup(priv);
+ reset_dwl_state(priv);
return false;
}

@@ -758,7 +788,30 @@ static bool copy_network_chunks(zckChunk **dstChunk, struct hnd_priv *priv)

priv->chunk = *dstChunk;
priv->error_in_parser = false;
+ priv->restart_requested = false;
while (1) {
+ if (priv->restart_requested) {
+ priv->restart_requested = false;
+ WARN("Restarting download from chunk %ld",
+ priv->chunk ? (long)zck_get_chunk_number(priv->chunk) : -1);
+
+ reset_dwl_state(priv);
+
+ /*
+ * The chunks following the last one written can be available in
+ * the source. Copy them before asking for ranges.
+ */
+ if (!copy_existing_chunks(&priv->chunk, priv))
+ return false;
+
+ /* Nothing left to download, the rest came from source */
+ if (!priv->chunk) {
+ *dstChunk = priv->chunk;
+ return true;
+ }
+ /* dwlstate is NOTRUNNING : new ranges are requested below */
+ }
+
switch (priv->dwlstate) {
case NOTRUNNING:
if (!trigger_download(priv))
@@ -769,6 +822,8 @@ static bool copy_network_chunks(zckChunk **dstChunk, struct hnd_priv *priv)
if (!read_and_validate_package(priv))
return false;
answer = priv->answer;
+ if (priv->restart_requested)
+ break;
if (answer->type == RANGE_HEADERS) {
if (!parse_headers(priv)) {
return false;
@@ -803,8 +858,12 @@ static bool copy_network_chunks(zckChunk **dstChunk, struct hnd_priv *priv)
if (!read_and_validate_package(priv))
return false;
answer = priv->answer;
+ if (priv->restart_requested)
+ break;
if (answer->type == RANGE_COMPLETED) {
priv->dwlstate = END_TRANSFER;
+ } else if (answer->type == RANGE_HEADERS) {
+ /* no payload to collect */
} else if (!fill_buffers_list(priv))
return false;
break;
diff --git a/handlers/delta_handler.h b/handlers/delta_handler.h
index 9aba1f1c..6200a2d3 100644
--- a/handlers/delta_handler.h
+++ b/handlers/delta_handler.h
@@ -16,7 +16,8 @@ typedef enum {
RANGE_HEADERS,
RANGE_DATA,
RANGE_COMPLETED,
- RANGE_ERROR
+ RANGE_ERROR,
+ RANGE_RESTART /* Have caller reconstruct another range request. */
} request_type;

typedef struct {
--
2.50.1 (Apple Git-155)

[Caleb Hensley]

With the channel_get_file fix submitted previous to this patch, interrupted
range requests now return an error instead of being resumed, so the delta
handler can recover from it depending on the error type. The chunks downloader
knows how many bytes it forwarded, but the delta handler knows which of them
were turned into a verified chunk and written to the chained handler.

The downloader keeps a retry budget, taken from retry and retrywait of the delta
section, and reports a retryable transfer error as RANGE_RESTART rather than
RANGE_ERROR. It sleeps after the handler has been notified, so the next request
is already prepared when it wakes up, and resets the count on every completed
request so the budget covers one outage instead of capping the interruptions of
a long download.

The handler answers RANGE_RESTART by dropping the state of the running request
and asking for ranges again. network_process_data() only advances priv->chunk
after copyfile() verified the chunk hash, so priv->chunk is the first chunk that
was not written yet and zchunk_get_missing_range() computes the remaining ranges
from it. The partially filled buffer is discarded: it never reached copyfile(),
so nothing of it reached the chained handler. Chunks available in the source are
checked and copied first: they can be next if the transfer stopped between two parts.

The answer to the new request is a new multipart body, so the boundary is
dropped with the rest of the state. Headers arriving while data is expected are
now ignored instead of being collected as payload.

Also included is a fix to the channel settings parsed type for retrywait.

Signed-off-by: Caleb Hensley <caleb....@lvt.com>

index 665ac844..e006c393 100644