Skip to content

Commit 01f6d25

Browse files
andinuxclaude
andcommitted
fix(network): track lastOptimisticVersion latest-valid, not monotonic max
network_sync_state_update_from_response kept the maximum optimistic/confirmed version seen across a multi-chunk send. But the server can move lastOptimisticVersion BACKWARD when a later chunk fails (rollback), and that value becomes the durable send checkpoint (CLOUDSYNC_KEY_SEND_DBVERSION) — so masking a decrease advanced the checkpoint past the rolled-back changes, which were then never re-sent (silent data loss). Take the latest valid (>= 0) value instead, matching how `gaps` is already handled; a missing/unparseable field (-1) still leaves the value untouched. Pre-existing since the chunked-payload transport work, unreleased. Add a network-layer unit test harness (the layer had none): network_unit.c is built with networking compiled in (T_CFLAGS minus OMIT_NETWORK, linked against curl) so the internal parsing/state functions can be exercised on in-memory NETWORK_RESULT buffers with no server. `make network-unittest` runs it (also wired into `make test`). The new test reproduces the rollback (50→100→50 must end at 50, not 100) and covers the missing-field no-op and network_compute_status; network_sync_state_update_from_response / network_compute_status are de-static'd for it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 5ee59f2 commit 01f6d25

4 files changed

Lines changed: 157 additions & 14 deletions

File tree

Makefile

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,14 +78,28 @@ FI_SRC = $(FI_DIR)/fractional_indexing.c
7878
# Combined for SQLite extension build
7979
SRC_FILES = $(CORE_SRC) $(SQLITE_SRC) $(FI_SRC)
8080

81-
TEST_SRC = $(wildcard $(TEST_DIR)/*.c)
81+
# network_unit.c is built separately with networking ENABLED (see network-unittest),
82+
# so keep it out of the default OMIT_NETWORK test build.
83+
TEST_SRC = $(filter-out $(TEST_DIR)/network_unit.c,$(wildcard $(TEST_DIR)/*.c))
8284
TEST_FILES = $(SRC_FILES) $(TEST_SRC) $(wildcard $(SQLITE_DIR)/*.c)
8385
RELEASE_OBJ = $(patsubst %.c, $(BUILD_RELEASE)/%.o, $(notdir $(SRC_FILES)))
8486
TEST_OBJ = $(patsubst %.c, $(BUILD_TEST)/%.o, $(notdir $(TEST_FILES)))
8587
COV_FILES = $(filter-out $(SRC_DIR)/lz4.c $(NETWORK_DIR)/network.c $(SQLITE_IMPL_DIR)/sql_sqlite.c $(POSTGRES_IMPL_DIR)/database_postgresql.c $(FI_SRC), $(SRC_FILES))
8688
CURL_LIB = $(CURL_DIR)/$(PLATFORM)/libcurl.a
8789
TEST_TARGET = $(patsubst %.c,$(DIST_DIR)/%$(EXE), $(notdir $(TEST_SRC)))
8890

91+
# Network-enabled unit tests: rebuild the codebase with networking ON (T_CFLAGS
92+
# minus OMIT_NETWORK) and link curl, so network.c's internal functions can be
93+
# tested directly on in-memory buffers. NT_LDFLAGS reuses the platform LDFLAGS
94+
# (which carries -lcurl) minus the shared-library-only flags, plus the test link
95+
# libs. -undefined dynamic_lookup is kept: the test never opens a connection, so
96+
# curl's transport symbols are linked but never invoked.
97+
BUILD_NETTEST = build/nettest
98+
NT_CFLAGS = $(filter-out -DCLOUDSYNC_OMIT_NETWORK,$(T_CFLAGS))
99+
NT_LDFLAGS = $(filter-out -dynamiclib -headerpad_max_install_names,$(LDFLAGS)) $(T_LDFLAGS)
100+
NT_SRC = $(SRC_FILES) $(SQLITE_DIR)/sqlite3.c $(TEST_DIR)/network_unit.c
101+
NT_OBJ = $(patsubst %.c,$(BUILD_NETTEST)/%.o,$(notdir $(NT_SRC)))
102+
89103
# Build curl hermetically: neutralize the developer's ambient build env so
90104
# curl's ./configure compile tests aren't broken by overrides leaking in
91105
# (e.g. exported LDFLAGS/CPPFLAGS/LIBS pointing at Homebrew). Build flags for
@@ -261,8 +275,16 @@ $(BUILD_TEST)/sqlite3.o: $(SQLITE_DIR)/sqlite3.c
261275
$(BUILD_TEST)/%.o: %.c
262276
$(CC) $(T_CFLAGS) -c $< -o $@
263277

278+
# Network-enabled object files (networking ON, for network-unittest)
279+
$(BUILD_NETTEST):
280+
mkdir -p $(BUILD_NETTEST)
281+
$(BUILD_NETTEST)/sqlite3.o: $(SQLITE_DIR)/sqlite3.c | $(BUILD_NETTEST)
282+
$(CC) $(CFLAGS) -DSQLITE_DQS=0 -DSQLITE_CORE -c $< -o $@
283+
$(BUILD_NETTEST)/%.o: %.c | $(BUILD_NETTEST)
284+
$(CC) $(NT_CFLAGS) -c $< -o $@
285+
264286
# Run code coverage (--css-file $(CUSTOM_CSS))
265-
test: $(TARGET) $(TEST_TARGET) unittest e2e
287+
test: $(TARGET) $(TEST_TARGET) unittest network-unittest e2e
266288
set -e; $(SQLITE3) ":memory:" -cmd ".bail on" ".load ./$<" "SELECT cloudsync_version();"
267289
ifneq ($(COVERAGE),false)
268290
mkdir -p $(COV_DIR)
@@ -274,6 +296,11 @@ endif
274296
unittest: $(TARGET) $(DIST_DIR)/unit$(EXE)
275297
@./$(DIST_DIR)/unit$(EXE)
276298

299+
# Run the network-layer unit tests (networking compiled in, no server)
300+
network-unittest: $(CURL_LIB) $(NT_OBJ)
301+
$(CC) $(NT_OBJ) -o $(DIST_DIR)/network_unit$(EXE) $(NT_LDFLAGS)
302+
@./$(DIST_DIR)/network_unit$(EXE)
303+
277304
# Run end-to-end integration tests
278305
e2e: $(TARGET) $(DIST_DIR)/integration$(EXE)
279306
@if [ -f .env ]; then \

src/network/network.c

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1601,18 +1601,22 @@ static int network_send_payload_to_apply(sqlite3_context *context, network_data
16011601
return SQLITE_OK;
16021602
}
16031603

1604-
static void network_sync_state_update_from_response(NETWORK_RESULT *res,
1605-
int64_t *last_optimistic_version,
1606-
int64_t *last_confirmed_version,
1607-
int *gaps_size,
1608-
char **apply_failure_json,
1609-
char **check_failure_json) {
1604+
void network_sync_state_update_from_response(NETWORK_RESULT *res,
1605+
int64_t *last_optimistic_version,
1606+
int64_t *last_confirmed_version,
1607+
int *gaps_size,
1608+
char **apply_failure_json,
1609+
char **check_failure_json) {
16101610
if (!res || res->code != CLOUDSYNC_NETWORK_BUFFER || !res->buffer) return;
16111611

1612-
int64_t parsed_version = json_extract_int(res->buffer, res->blen, "lastOptimisticVersion", -1);
1613-
if (parsed_version > *last_optimistic_version) *last_optimistic_version = parsed_version;
1614-
parsed_version = json_extract_int(res->buffer, res->blen, "lastConfirmedVersion", -1);
1615-
if (parsed_version > *last_confirmed_version) *last_confirmed_version = parsed_version;
1612+
// Take the latest valid (>= 0) value, not the max: the server can move these
1613+
// BACKWARD on a rollback when a later send chunk fails, and lastOptimisticVersion
1614+
// becomes the durable send checkpoint — masking a decrease would advance the
1615+
// checkpoint past the rolled-back changes and silently drop them.
1616+
int64_t parsed_optimistic = json_extract_int(res->buffer, res->blen, "lastOptimisticVersion", -1);
1617+
if (parsed_optimistic >= 0) *last_optimistic_version = parsed_optimistic;
1618+
int64_t parsed_confirmed = json_extract_int(res->buffer, res->blen, "lastConfirmedVersion", -1);
1619+
if (parsed_confirmed >= 0) *last_confirmed_version = parsed_confirmed;
16161620
int parsed_gaps_size = json_extract_array_size(res->buffer, res->blen, "gaps");
16171621
if (parsed_gaps_size >= 0) *gaps_size = parsed_gaps_size;
16181622

@@ -1627,10 +1631,16 @@ static void network_sync_state_update_from_response(NETWORK_RESULT *res,
16271631
if (*check_failure_json) cloudsync_memory_free(*check_failure_json);
16281632
*check_failure_json = check_failure;
16291633
}
1634+
1635+
#ifdef CLOUDSYNC_NETWORK_TRACE
1636+
// Full endpoint response body that the sync-state fields above were parsed from.
1637+
// The buffer is not guaranteed NUL-terminated, so bound the print with its length.
1638+
fprintf(stderr, "[cloudsync-network] sync_state response=%.*s\n", (int)res->blen, res->buffer);
1639+
#endif
16301640
}
16311641

1632-
static const char *network_compute_status(int64_t last_optimistic, int64_t last_confirmed,
1633-
int gaps_size, int64_t local_version) {
1642+
const char *network_compute_status(int64_t last_optimistic, int64_t last_confirmed,
1643+
int gaps_size, int64_t local_version) {
16341644
if (last_optimistic < 0 || last_confirmed < 0) return "error";
16351645
if (gaps_size > 0 || last_optimistic < local_version) return "out-of-sync";
16361646
if (last_optimistic == last_confirmed) return "synced";

src/network/network_private.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@
88
#ifndef __CLOUDSYNC_NETWORK_PRIVATE__
99
#define __CLOUDSYNC_NETWORK_PRIVATE__
1010

11+
#include <stdint.h>
12+
#include <stddef.h>
13+
1114
#define CLOUDSYNC_DEFAULT_ADDRESS "https://cloudsync.sqlite.ai"
1215
#define CLOUDSYNC_ENDPOINT_PREFIX "v2/cloudsync/databases"
1316
#define CLOUDSYNC_ENDPOINT_UPLOAD "upload"
@@ -46,6 +49,10 @@ bool network_data_set_endpoints (network_data *data, char *auth, char *check, ch
4649
bool network_send_buffer(network_data *data, const char *endpoint, const char *authentication, const void *blob, int blob_size);
4750
NETWORK_RESULT network_receive_buffer (network_data *data, const char *endpoint, const char *authentication, bool zero_terminated, bool is_post_request, char *json_payload, const char **extra_headers, int nextra_headers);
4851

52+
// Exposed (non-static) for the network unit test; otherwise internal to network.c.
53+
void network_sync_state_update_from_response(NETWORK_RESULT *res, int64_t *last_optimistic_version, int64_t *last_confirmed_version, int *gaps_size, char **apply_failure_json, char **check_failure_json);
54+
const char *network_compute_status(int64_t last_optimistic, int64_t last_confirmed, int gaps_size, int64_t local_version);
55+
4956
#ifdef CLOUDSYNC_NETWORK_TRACE
5057
const char *network_trace_endpoint_name(network_data *data, const char *endpoint);
5158
const char *network_trace_result_name(int code);

test/network_unit.c

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
//
2+
// network_unit.c
3+
// cloudsync
4+
//
5+
// Unit tests for the network layer's pure response-handling logic. Built with
6+
// networking ENABLED (unlike dist/unit, which is -DCLOUDSYNC_OMIT_NETWORK), so it
7+
// can call the internal functions directly on crafted in-memory NETWORK_RESULT
8+
// buffers — no server, no sockets.
9+
//
10+
11+
#include <stdio.h>
12+
#include <string.h>
13+
#include <stdbool.h>
14+
#include <stdint.h>
15+
#include <inttypes.h>
16+
#include "network_private.h"
17+
18+
static int failures = 0;
19+
20+
static void check(const char *name, bool ok) {
21+
printf("%-64s %s\n", name, ok ? "OK" : "FAIL");
22+
if (!ok) failures++;
23+
}
24+
25+
static NETWORK_RESULT json_buffer(char *json) {
26+
NETWORK_RESULT r = {0};
27+
r.code = CLOUDSYNC_NETWORK_BUFFER;
28+
r.buffer = json;
29+
r.blen = strlen(json);
30+
return r;
31+
}
32+
33+
// Regression: lastOptimisticVersion must track the LATEST valid value, including a
34+
// decrease. The server can roll the optimistic version back when a later send chunk
35+
// fails; since it becomes the durable send checkpoint, a monotonic "max" would mask
36+
// the rollback and skip the rolled-back changes on the next send.
37+
static bool test_optimistic_version_rollback(void) {
38+
int64_t optimistic = -1, confirmed = -1;
39+
int gaps = -1;
40+
char *apply = NULL, *check_fail = NULL;
41+
bool ok = true;
42+
43+
char j1[] = "{\"lastOptimisticVersion\":50,\"lastConfirmedVersion\":10}";
44+
NETWORK_RESULT r1 = json_buffer(j1);
45+
network_sync_state_update_from_response(&r1, &optimistic, &confirmed, &gaps, &apply, &check_fail);
46+
ok = ok && optimistic == 50 && confirmed == 10;
47+
48+
char j2[] = "{\"lastOptimisticVersion\":100,\"lastConfirmedVersion\":20}";
49+
NETWORK_RESULT r2 = json_buffer(j2);
50+
network_sync_state_update_from_response(&r2, &optimistic, &confirmed, &gaps, &apply, &check_fail);
51+
ok = ok && optimistic == 100 && confirmed == 20;
52+
53+
// Server rolls back on a later chunk error: the value must DECREASE to 50.
54+
char j3[] = "{\"lastOptimisticVersion\":50,\"lastConfirmedVersion\":20}";
55+
NETWORK_RESULT r3 = json_buffer(j3);
56+
network_sync_state_update_from_response(&r3, &optimistic, &confirmed, &gaps, &apply, &check_fail);
57+
ok = ok && optimistic == 50;
58+
59+
// A response missing the field (parsed -1) must NOT clobber the current value.
60+
char j4[] = "{\"lastConfirmedVersion\":20}";
61+
NETWORK_RESULT r4 = json_buffer(j4);
62+
network_sync_state_update_from_response(&r4, &optimistic, &confirmed, &gaps, &apply, &check_fail);
63+
ok = ok && optimistic == 50;
64+
65+
ok = ok && apply == NULL && check_fail == NULL; // no failures object in these responses
66+
return ok;
67+
}
68+
69+
// A non-BUFFER result (or NULL buffer) must leave the accumulators untouched.
70+
static bool test_non_buffer_is_noop(void) {
71+
int64_t optimistic = 7, confirmed = 3;
72+
int gaps = 0;
73+
char *apply = NULL, *check_fail = NULL;
74+
75+
NETWORK_RESULT err = {0};
76+
err.code = CLOUDSYNC_NETWORK_ERROR;
77+
network_sync_state_update_from_response(&err, &optimistic, &confirmed, &gaps, &apply, &check_fail);
78+
return optimistic == 7 && confirmed == 3 && gaps == 0;
79+
}
80+
81+
static bool test_compute_status(void) {
82+
bool ok = true;
83+
ok = ok && strcmp(network_compute_status(100, 100, 0, 100), "synced") == 0;
84+
ok = ok && strcmp(network_compute_status(100, 50, 0, 100), "syncing") == 0;
85+
ok = ok && strcmp(network_compute_status(100, 100, 1, 100), "out-of-sync") == 0; // gaps
86+
ok = ok && strcmp(network_compute_status(90, 90, 0, 100), "out-of-sync") == 0; // behind local
87+
ok = ok && strcmp(network_compute_status(-1, 100, 0, 100), "error") == 0; // unparsed
88+
return ok;
89+
}
90+
91+
int main(void) {
92+
printf("\nNetwork unit tests\n");
93+
check("optimistic/confirmed version folds latest-valid (allows rollback):", test_optimistic_version_rollback());
94+
check("non-buffer response is a no-op:", test_non_buffer_is_noop());
95+
check("network_compute_status:", test_compute_status());
96+
if (failures) { printf("\n%d test(s) FAILED\n", failures); return 1; }
97+
printf("\nAll network unit tests passed\n");
98+
return 0;
99+
}

0 commit comments

Comments
 (0)