Skip to content

Commit 26e1462

Browse files
revettrevett
authored
Storage → Test connection verifies conditional writes (#144)
Resolves [#141](#141) ## Problem - `testConnection` reported success the moment a provider accepted the credentials, never checking the conditional writes sync's compare-and-swap depends on - Providers that reject conditional writes, or accept `If-None-Match: *` and silently ignore it, passed the test then either refused to sync or let concurrent edits overwrite each other with no error ## Changes - Added a conditional write probe that runs after the credential check, writing and rereading a throwaway object under the reserved prefix to confirm the provider enforces `If-None-Match: *` and returns an `ETag` - Failed the connection test with a clear message when a provider ignores, rejects, or cannot support the writes sync needs - Covered each failure mode with unit tests, with the happy path exercised against real storage ## Why - A connection test that passes for providers that lose edits is worse than none; this makes it answer the question it claims to, before a vault ever trusts the bucket <!-- greptile_comment --> <h3>Greptile Summary</h3> Adds a storage compatibility probe that: - Verifies providers enforce conditional object creation. - Confirms successful reads return an ETag. - Deletes the temporary probe object without allowing cleanup rejection to replace the probe verdict. - Adds unit coverage for supported providers and each compatibility failure mode. <h3>Confidence Score: 5/5</h3> The PR appears safe to merge. No blocking failure remains. <h3>Important Files Changed</h3> | Filename | Overview | |----------|----------| | src/storage/storage.ts | Adds the conditional-write probe to connection testing and safely suppresses rejected best-effort cleanup. | | src/storage/storage.test.ts | Covers successful probing, ignored or rejected conditions, missing ETags, reserved-prefix cleanup, and cleanup rejection. | <sub>Reviews (2): Last reviewed commit: ["Address comment around error handling"](f8ace30) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=47667341)</sub> <!-- /greptile_comment -->
1 parent 45b94e8 commit 26e1462

2 files changed

Lines changed: 184 additions & 4 deletions

File tree

‎src/storage/storage.test.ts‎

Lines changed: 121 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,41 @@
11
import assert from "node:assert/strict";
22
import { test } from "node:test";
33
import { DEFAULT_SETTINGS, type GeodeSettings } from "../settings/settings.ts";
4-
import { testConnection } from "./storage.ts";
4+
import { probeConditionalWrites, type StorageClient, testConnection } from "./storage.ts";
55
import { parseListObjectsXml } from "./xml.ts";
66

7+
// honouringPut is a putObject that enforces ifAbsent the way a correct S3 server does: the first
8+
// write to a key lands, a later ifAbsent write to the same key is rejected as a conflict.
9+
function honouringPut(): StorageClient["putObject"] {
10+
const seen = new Set<string>();
11+
return async (key, _body, condition) => {
12+
if (condition !== undefined && condition.kind === "ifAbsent" && seen.has(key)) {
13+
return { ok: false, status: "conflict", message: "Storage rejected the write (412)" };
14+
}
15+
seen.add(key);
16+
return { ok: true, status: "ok", message: "" };
17+
};
18+
}
19+
20+
// probeStub returns a StorageClient whose methods succeed with an etag by default, so each test
21+
// overrides only the behaviour it exercises.
22+
function probeStub(over: Partial<StorageClient>): StorageClient {
23+
const base: StorageClient = {
24+
putObject: async () => ({ ok: true, status: "ok", message: "" }),
25+
getObject: async () => ({
26+
ok: true,
27+
status: "ok",
28+
message: "",
29+
body: new Uint8Array(),
30+
etag: '"probe"',
31+
}),
32+
copyObject: async () => ({ ok: true, status: "ok", message: "" }),
33+
deleteObject: async () => ({ ok: true, status: "ok", message: "" }),
34+
listObjects: async () => ({ ok: true, status: "ok", message: "", objects: [] }),
35+
};
36+
return { ...base, ...over };
37+
}
38+
739
const missingFieldCases: {
840
name: string;
941
settings: GeodeSettings;
@@ -87,6 +119,94 @@ test("testConnection: R2 with empty endpoint and region passes field check", asy
87119
assert.ok(!result.message.startsWith("Fill in"));
88120
});
89121

122+
test("probeConditionalWrites: passes when the provider honours conditional writes", async () => {
123+
const client = probeStub({ putObject: honouringPut() });
124+
125+
const result = await probeConditionalWrites(client);
126+
127+
assert.equal(result.ok, true);
128+
assert.equal(result.status, "ok");
129+
});
130+
131+
test("probeConditionalWrites: fails when the provider silently ignores the condition", async () => {
132+
// Google Cloud Storage's S3 interop accepts If-None-Match: * and does nothing with it, so the
133+
// second write clobbers the first instead of conflicting.
134+
const client = probeStub({
135+
putObject: async () => ({ ok: true, status: "ok", message: "" }),
136+
});
137+
138+
const result = await probeConditionalWrites(client);
139+
140+
assert.equal(result.ok, false);
141+
assert.equal(result.status, "client");
142+
assert.match(result.message, /concurrent edits/);
143+
});
144+
145+
test("probeConditionalWrites: fails when the provider rejects the conditional write", async () => {
146+
// Backblaze B2, Wasabi, and Garage reject the precondition outright rather than honour it.
147+
const client = probeStub({
148+
putObject: async () => ({
149+
ok: false,
150+
status: "server",
151+
message: "Storage rejected the write (501)",
152+
}),
153+
});
154+
155+
const result = await probeConditionalWrites(client);
156+
157+
assert.equal(result.ok, false);
158+
assert.equal(result.status, "server");
159+
assert.equal(result.message, "Storage rejected the write (501)");
160+
});
161+
162+
test("probeConditionalWrites: fails when the provider returns no etag", async () => {
163+
const client = probeStub({
164+
putObject: honouringPut(),
165+
getObject: async () => ({
166+
ok: true,
167+
status: "ok",
168+
message: "",
169+
body: new Uint8Array(),
170+
etag: null,
171+
}),
172+
});
173+
174+
const result = await probeConditionalWrites(client);
175+
176+
assert.equal(result.ok, false);
177+
assert.equal(result.status, "client");
178+
assert.match(result.message, /ETag/);
179+
});
180+
181+
test("probeConditionalWrites: deletes its probe object under the reserved prefix", async () => {
182+
let deleted = "";
183+
const client = probeStub({
184+
putObject: honouringPut(),
185+
deleteObject: async (key) => {
186+
deleted = key;
187+
return { ok: true, status: "ok", message: "" };
188+
},
189+
});
190+
191+
await probeConditionalWrites(client);
192+
193+
assert.ok(deleted.startsWith(".geode/"));
194+
});
195+
196+
test("probeConditionalWrites: a rejecting cleanup does not mask a passing probe", async () => {
197+
const client = probeStub({
198+
putObject: honouringPut(),
199+
deleteObject: async () => {
200+
throw new Error("network blip during cleanup");
201+
},
202+
});
203+
204+
const result = await probeConditionalWrites(client);
205+
206+
assert.equal(result.ok, true);
207+
assert.equal(result.status, "ok");
208+
});
209+
90210
test("parseListObjectsXml decodes XML entities in object keys", () => {
91211
const xml = `<?xml version="1.0" encoding="UTF-8"?>
92212
<ListBucketResult>

‎src/storage/storage.ts‎

Lines changed: 63 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,11 @@ import { encodeComponent, encodeKey } from "./encode.ts";
44
import { messageFor, statusForHttp } from "./errors.ts";
55
import { parseListObjectsXml } from "./xml.ts";
66

7+
// PROBE_KEY_PREFIX namespaces testConnection's throwaway probe object under geode's reserved bucket
8+
// prefix (sync's RESERVED_PREFIX). A probe left behind by a failed cleanup therefore sits where
9+
// sync ignores it, never pulled to every device as a phantom vault file.
10+
const PROBE_KEY_PREFIX = ".geode/connection-probe-";
11+
712
// ConnectionResult reports whether a storage provider accepted a test request. Message is the
813
// empty string when ok is true.
914
export type ConnectionResult = {
@@ -110,8 +115,62 @@ export function createS3Client(settings: GeodeSettings, secretAccessKey: string)
110115
};
111116
}
112117

113-
// testConnection sends a signed HEAD request for the configured bucket and reports whether the
114-
// provider accepted the credentials.
118+
// probeConditionalWrites confirms the provider actually honours the compare-and-swap sync is built
119+
// on, not merely that it accepts the credentials. It writes a throwaway object with If-None-Match:
120+
// *, then issues a second If-None-Match: * write that must be rejected: a provider with no
121+
// conditional-write support (Backblaze B2, Wasabi, Garage) fails the first write, and one that
122+
// accepts the header but ignores it (Google Cloud Storage's S3 interop) lets the second write
123+
// clobber the first, the exact silent data loss the conditional puts exist to prevent. It also
124+
// checks the read hands back an ETag, which sync needs to make later updates conditional. The probe
125+
// object is always deleted, best effort.
126+
export async function probeConditionalWrites(client: StorageClient): Promise<ConnectionResult> {
127+
const key = `${PROBE_KEY_PREFIX}${crypto.randomUUID()}`;
128+
const body = new TextEncoder().encode("geode connection probe");
129+
130+
try {
131+
const first = await client.putObject(key, body, { kind: "ifAbsent" });
132+
if (!first.ok) {
133+
return { ok: false, status: first.status, message: first.message };
134+
}
135+
136+
const second = await client.putObject(key, body, { kind: "ifAbsent" });
137+
if (second.ok) {
138+
return {
139+
ok: false,
140+
status: "client",
141+
message: "Storage ignored a conditional write, so concurrent edits can be lost",
142+
};
143+
}
144+
if (second.status !== "conflict") {
145+
return { ok: false, status: second.status, message: second.message };
146+
}
147+
148+
const read = await client.getObject(key);
149+
if (!read.ok) {
150+
return { ok: false, status: read.status, message: read.message };
151+
}
152+
if (read.etag === null) {
153+
return {
154+
ok: false,
155+
status: "client",
156+
message: "Storage did not return an ETag, which sync needs for conditional writes",
157+
};
158+
}
159+
160+
return { ok: true, status: "ok", message: "" };
161+
} finally {
162+
// Best effort: the probe already proved what it needed to, and a leftover object lives under
163+
// the reserved prefix where sync ignores it. A cleanup that rejects would escape the finally
164+
// and replace the probe's verdict, so the rejection is swallowed rather than allowed to mask
165+
// an otherwise successful test.
166+
await client.deleteObject(key).catch(() => undefined);
167+
}
168+
}
169+
170+
// testConnection reports whether a storage provider is usable for sync: it accepts the credentials
171+
// (a signed HEAD for the bucket) and honours the conditional writes sync's compare-and-swap depends
172+
// on (probeConditionalWrites). Reporting ok on the HEAD alone would green-light providers that
173+
// authenticate fine but silently lose edits under concurrency.
115174
export async function testConnection(
116175
settings: GeodeSettings,
117176
secretAccessKey: string,
@@ -143,7 +202,8 @@ export async function testConnection(
143202
message: `Storage rejected the request (${response.status})`,
144203
};
145204
}
146-
return { ok: true, status: "ok", message: "" };
205+
206+
return probeConditionalWrites(createS3Client(settings, secretAccessKey));
147207
}
148208

149209
// conditionHeaders converts a PutCondition into the HTTP precondition headers an S3 compatible

0 commit comments

Comments
 (0)