feat(protocol): add optional STATUS_ERROR_DETAIL rejection reason (2.21.0)
Today a server rejection sends a bare STATUS_ERROR and the reason only
reaches the server log, so the client cannot say why a transfer was
refused. Add an optional, bounded server->client error-detail frame:
- Status gains STATUS_ERROR_DETAIL appended LAST so existing wire
values are unchanged.
- send_error_detail(fd, msg) sends STATUS_ERROR_DETAIL followed by the
existing length-prefixed string primitive, slicing over-long messages
to MAX_ERROR_DETAIL_BYTES (4096).
- receive_status() (and the timed/keepalive status readers) always
consume the detail body and map the status back to STATUS_ERROR,
capturing the text into a thread-local buffer exposed by
protocol_last_error(); a bare STATUS_ERROR leaves it cleared. Every
existing call site keeps working and the stream cannot desync.
- Upgrade the daemon module gate / config validation (config.c), the
final transfer failure (server.c) and receiver-side path/node
validation (file_receive.c) to send a concrete reason; surface it on
the client in client_send.c/config.c.
- Bump PROTOCOL_VERSION to 2.21.0 (CMake VERSION, CHANGELOG, docs) and
update the pinned config wire golden hash / CLI-version tests.
- Add tests/test_protocol_error.c covering mapping+capture, the
over-long bound, bare-error clearing, and thread-locality.
This commit is contained in:
@@ -36,7 +36,7 @@ from common import ( # noqa: E402
|
||||
verify_transfer,
|
||||
)
|
||||
|
||||
PROTOCOL_VERSION = b"2.20.0"
|
||||
PROTOCOL_VERSION = b"2.21.0"
|
||||
STATUS_MANIFEST = 5
|
||||
STATUS_OK = 0
|
||||
|
||||
|
||||
@@ -94,14 +94,14 @@ def _seed_protocol_source(source):
|
||||
class TestProtocol:
|
||||
@pytest.mark.ci
|
||||
def test_protocol_current_version_accepted(self, shared_server):
|
||||
"""--protocol=2.20.0 (the current PROTOCOL_VERSION) is accepted and the
|
||||
"""--protocol=2.21.0 (the current PROTOCOL_VERSION) is accepted and the
|
||||
transfer completes normally."""
|
||||
source = os.path.join(TEST_DATA_DIR, "proto_ok_src")
|
||||
dest = os.path.join(TEST_DATA_DIR, "proto_ok_dst")
|
||||
shutil.rmtree(dest, ignore_errors=True)
|
||||
os.makedirs(dest)
|
||||
_seed_protocol_source(source)
|
||||
result, _ = run_client(source, dest, flags=["--protocol=2.20.0"],
|
||||
result, _ = run_client(source, dest, flags=["--protocol=2.21.0"],
|
||||
port=shared_server.port)
|
||||
assert result.returncode == 0, \
|
||||
f"--protocol current run failed: {(result.stderr or result.stdout)[:400]}"
|
||||
|
||||
@@ -25,6 +25,7 @@
|
||||
#include "test_multiprocessing.h"
|
||||
#include "test_property.h"
|
||||
#include "test_protocol.h"
|
||||
#include "test_protocol_error.h"
|
||||
#include "test_queue.h"
|
||||
#include "test_receiver_timeout.h"
|
||||
#include "test_robustness.h"
|
||||
@@ -65,6 +66,7 @@ int main() {
|
||||
RUN_TEST(test_delta);
|
||||
RUN_TEST(test_data);
|
||||
RUN_TEST(test_protocol);
|
||||
RUN_TEST(test_protocol_error);
|
||||
RUN_TEST(test_receiver_timeout);
|
||||
RUN_TEST(test_metadata);
|
||||
RUN_TEST(test_glob);
|
||||
|
||||
@@ -306,7 +306,7 @@ static void test_parse_args_protocol_accept_current() {
|
||||
Config* cfg = valid_client_config();
|
||||
EXPECT_NOT_NULL(cfg);
|
||||
char* argv_equals[] = {"fastsync", "--source-dir", "/src",
|
||||
"--dest-dir", "/dst", "--protocol=2.20.0"};
|
||||
"--dest-dir", "/dst", "--protocol=2.21.0"};
|
||||
int positional_args[2];
|
||||
int positional_count = 0;
|
||||
EXPECT_EQ_INT(parse_args(cfg, 6, argv_equals, positional_args, &positional_count), 0);
|
||||
@@ -316,7 +316,7 @@ static void test_parse_args_protocol_accept_current() {
|
||||
cfg = valid_client_config();
|
||||
EXPECT_NOT_NULL(cfg);
|
||||
char* argv_space[] = {"fastsync", "--source-dir", "/src", "--dest-dir",
|
||||
"/dst", "--protocol", "2.20.0"};
|
||||
"/dst", "--protocol", "2.21.0"};
|
||||
positional_count = 0;
|
||||
EXPECT_EQ_INT(parse_args(cfg, 7, argv_space, positional_args, &positional_count), 0);
|
||||
EXPECT_EQ_STR(cfg->version, PROTOCOL_VERSION);
|
||||
@@ -326,8 +326,9 @@ static void test_parse_args_protocol_accept_current() {
|
||||
/* Any --protocol value other than the current PROTOCOL_VERSION must end in
|
||||
* failure (parse_args simply stores it; validate_config rejects it up front). */
|
||||
static void test_parse_args_protocol_rejects_other_versions() {
|
||||
static const char* const bad_versions[] = {
|
||||
"2.17", "2.16", "2.15.0", "2.16.0", "2.17.0", "2.18.0", "2.19.0", "216", "31", "abc", ""};
|
||||
static const char* const bad_versions[] = {"2.17", "2.16", "2.15.0", "2.16.0",
|
||||
"2.17.0", "2.18.0", "2.19.0", "2.20.0",
|
||||
"216", "31", "abc", ""};
|
||||
for (size_t i = 0; i < sizeof(bad_versions) / sizeof(bad_versions[0]); i++) {
|
||||
Config* cfg = valid_client_config();
|
||||
EXPECT_NOT_NULL(cfg);
|
||||
|
||||
+3
-3
@@ -2443,11 +2443,11 @@ static void golden_config_populate(Config* c) {
|
||||
c->copy_as_gid = 222;
|
||||
}
|
||||
|
||||
/* The pinned golden frame (protocol 2.20.0). The values below are the only
|
||||
/* The pinned golden frame (protocol 2.21.0). The values below are the only
|
||||
* thing that ties the generated table to the historical wire format; update
|
||||
* them ONLY with a PROTOCOL_VERSION bump and a documented reason. */
|
||||
#define GOLDEN_WIRE_LEN 633
|
||||
#define GOLDEN_WIRE_HASH 9160991280011164139ULL
|
||||
#define GOLDEN_WIRE_HASH 7591559741712449854ULL
|
||||
|
||||
static unsigned long long fnv1a_64(const unsigned char* buf, size_t len) {
|
||||
unsigned long long h = 1469598103934665603ULL;
|
||||
@@ -2529,7 +2529,7 @@ static unsigned long long capture_wire_hash(const Config* cfg, size_t* out_len)
|
||||
return h;
|
||||
}
|
||||
|
||||
/* Byte-for-byte wire compatibility guard (protocol 2.20.0). The expected hash
|
||||
/* Byte-for-byte wire compatibility guard (protocol 2.21.0). The expected hash
|
||||
* pins the pre-X-macro byte stream; the refactor MUST NOT change it. */
|
||||
static void test_config_wire_golden() {
|
||||
if (is_running_under_valgrind())
|
||||
|
||||
@@ -0,0 +1,153 @@
|
||||
/* Unit tests for the protocol 2.21.0 STATUS_ERROR_DETAIL frame API:
|
||||
* send_error_detail() / receive_status() mapping / protocol_last_error(). */
|
||||
#include "protocol.h"
|
||||
#include "test_utils.h"
|
||||
#include <string.h>
|
||||
#include <threads.h>
|
||||
#include <unistd.h>
|
||||
|
||||
/* A detail frame maps back to STATUS_ERROR for the caller and its body is
|
||||
* captured verbatim. */
|
||||
static void test_error_detail_maps_and_captures(void) {
|
||||
int p[2];
|
||||
EXPECT_EQ_INT(pipe(p), 0);
|
||||
io_set_fds(p[0], p[1]);
|
||||
io_set_bwlimit(0);
|
||||
protocol_clear_last_error();
|
||||
|
||||
EXPECT_TRUE(send_error_detail(0, "module is read-only"));
|
||||
Status received = STATUS_OK;
|
||||
EXPECT_TRUE(receive_status(0, &received));
|
||||
EXPECT_EQ_INT((int)received, (int)STATUS_ERROR);
|
||||
EXPECT_EQ_STR(protocol_last_error(), "module is read-only");
|
||||
|
||||
close(p[0]);
|
||||
close(p[1]);
|
||||
}
|
||||
|
||||
/* An over-long message is sliced to the hard cap before it goes on the wire, so
|
||||
* the receiver never retains more than MAX_ERROR_DETAIL_BYTES. */
|
||||
static void test_error_detail_over_long_is_bounded(void) {
|
||||
int p[2];
|
||||
EXPECT_EQ_INT(pipe(p), 0);
|
||||
io_set_fds(p[0], p[1]);
|
||||
io_set_bwlimit(0);
|
||||
|
||||
char big[MAX_ERROR_DETAIL_BYTES + 512];
|
||||
memset(big, 'x', sizeof(big) - 1);
|
||||
big[sizeof(big) - 1] = '\0';
|
||||
EXPECT_TRUE(send_error_detail(0, big));
|
||||
Status received = STATUS_OK;
|
||||
EXPECT_TRUE(receive_status(0, &received));
|
||||
EXPECT_EQ_INT((int)received, (int)STATUS_ERROR);
|
||||
EXPECT_EQ_INT((int)strlen(protocol_last_error()), (int)MAX_ERROR_DETAIL_BYTES);
|
||||
|
||||
close(p[0]);
|
||||
close(p[1]);
|
||||
}
|
||||
|
||||
/* A bare STATUS_ERROR (no detail body) must not leave a stale reason visible. */
|
||||
static void test_bare_error_clears_last_error(void) {
|
||||
int p[2];
|
||||
EXPECT_EQ_INT(pipe(p), 0);
|
||||
io_set_fds(p[0], p[1]);
|
||||
io_set_bwlimit(0);
|
||||
|
||||
EXPECT_TRUE(send_error_detail(0, "stale reason"));
|
||||
Status received = STATUS_OK;
|
||||
EXPECT_TRUE(receive_status(0, &received));
|
||||
EXPECT_EQ_STR(protocol_last_error(), "stale reason");
|
||||
|
||||
EXPECT_TRUE(send_status(0, STATUS_ERROR));
|
||||
EXPECT_TRUE(receive_status(0, &received));
|
||||
EXPECT_EQ_INT((int)received, (int)STATUS_ERROR);
|
||||
EXPECT_EQ_STR(protocol_last_error(), "");
|
||||
|
||||
close(p[0]);
|
||||
close(p[1]);
|
||||
}
|
||||
|
||||
/* A tiny --max-alloc must not prevent the bounded detail body from being
|
||||
* drained: the status still maps to STATUS_ERROR with the full reason, and the
|
||||
* following frame is read intact (no desync). */
|
||||
static void test_error_detail_drains_despite_tiny_max_alloc(void) {
|
||||
int p[2];
|
||||
EXPECT_EQ_INT(pipe(p), 0);
|
||||
ProtocolSession receiver;
|
||||
protocol_session_init(&receiver, p[0], p[1]);
|
||||
protocol_session_set_max_alloc(&receiver, 4);
|
||||
ProtocolSession sender;
|
||||
protocol_session_init(&sender, -1, p[1]);
|
||||
|
||||
EXPECT_TRUE(protocol_send_status(&sender, STATUS_ERROR_DETAIL));
|
||||
EXPECT_TRUE(protocol_send_str(&sender, "reason"));
|
||||
Status status = STATUS_OK;
|
||||
EXPECT_TRUE(protocol_receive_status(&receiver, &status));
|
||||
EXPECT_EQ_INT((int)status, (int)STATUS_ERROR);
|
||||
EXPECT_EQ_STR(protocol_last_error(), "reason");
|
||||
|
||||
EXPECT_TRUE(protocol_send_status(&sender, STATUS_NEXT));
|
||||
EXPECT_TRUE(protocol_receive_status(&receiver, &status));
|
||||
EXPECT_EQ_INT((int)status, (int)STATUS_NEXT);
|
||||
|
||||
close(p[0]);
|
||||
close(p[1]);
|
||||
}
|
||||
|
||||
typedef struct {
|
||||
ProtocolSession* receiver;
|
||||
} DetailWorkerArg;
|
||||
|
||||
static int detail_worker(void* arg) {
|
||||
DetailWorkerArg* worker = arg;
|
||||
Status status = STATUS_OK;
|
||||
if (!protocol_receive_status(worker->receiver, &status) || status != STATUS_ERROR)
|
||||
return thrd_error;
|
||||
return strcmp(protocol_last_error(), "worker reason") == 0 ? thrd_success : thrd_error;
|
||||
}
|
||||
|
||||
/* Each thread keeps its own last-error buffer: a detail captured on a worker
|
||||
* must not overwrite the one captured on the main thread. */
|
||||
static void test_last_error_is_thread_local(void) {
|
||||
int main_pipe[2];
|
||||
int worker_pipe[2];
|
||||
EXPECT_EQ_INT(pipe(main_pipe), 0);
|
||||
EXPECT_EQ_INT(pipe(worker_pipe), 0);
|
||||
|
||||
io_set_fds(main_pipe[0], main_pipe[1]);
|
||||
io_set_bwlimit(0);
|
||||
EXPECT_TRUE(send_error_detail(0, "main reason"));
|
||||
Status received = STATUS_OK;
|
||||
EXPECT_TRUE(receive_status(0, &received));
|
||||
EXPECT_EQ_STR(protocol_last_error(), "main reason");
|
||||
|
||||
ProtocolSession receiver;
|
||||
protocol_session_init(&receiver, worker_pipe[0], -1);
|
||||
ProtocolSession sender;
|
||||
protocol_session_init(&sender, -1, worker_pipe[1]);
|
||||
EXPECT_TRUE(protocol_send_status(&sender, STATUS_ERROR_DETAIL));
|
||||
EXPECT_TRUE(protocol_send_str(&sender, "worker reason"));
|
||||
|
||||
DetailWorkerArg arg = {.receiver = &receiver};
|
||||
thrd_t thread;
|
||||
EXPECT_EQ_INT(thrd_create(&thread, detail_worker, &arg), thrd_success);
|
||||
int result = 0;
|
||||
EXPECT_EQ_INT(thrd_join(thread, &result), thrd_success);
|
||||
EXPECT_EQ_INT(result, thrd_success);
|
||||
|
||||
/* The worker's capture must not have disturbed this thread's buffer. */
|
||||
EXPECT_EQ_STR(protocol_last_error(), "main reason");
|
||||
|
||||
close(main_pipe[0]);
|
||||
close(main_pipe[1]);
|
||||
close(worker_pipe[0]);
|
||||
close(worker_pipe[1]);
|
||||
}
|
||||
|
||||
void test_protocol_error(void) {
|
||||
test_error_detail_maps_and_captures();
|
||||
test_error_detail_over_long_is_bounded();
|
||||
test_bare_error_clears_last_error();
|
||||
test_error_detail_drains_despite_tiny_max_alloc();
|
||||
test_last_error_is_thread_local();
|
||||
}
|
||||
@@ -0,0 +1,6 @@
|
||||
#ifndef TEST_PROTOCOL_ERROR_H
|
||||
#define TEST_PROTOCOL_ERROR_H
|
||||
|
||||
void test_protocol_error(void);
|
||||
|
||||
#endif
|
||||
Reference in New Issue
Block a user