mirror of
https://github.com/lancedb/lancedb.git
synced 2026-11-14 06:36:54 +00:00
fix: prevent unsafe write retries and bound Blob uploads (#4977)
Ambiguous gateway failures can occur after a write commits, so replaying the operation can append duplicate rows. Non-idempotent writes now retry only known precommit failures. Multipart retries reuse the same upload ID, part ID, and encoded bytes; ambiguous completion returns an unknown-outcome error without abort or replay. A definite 409/429 completion rejection triggers best-effort cleanup using the call's retry budget. Replayable parts and the shared byte-based partition target default to 64 MiB. Dictionary-bearing batches are compacted before size estimation and again when actual compressed IPC framing requires further splitting: keys are remapped without changing their types or schema, view backing buffers are collected, and ListView/LargeListView child ranges are compacted as an interval union so overlapping views retain sharing. Parts are prepared on demand; a single row and its schema can still exceed the target. Multipart progress starts at zero and counts each acknowledged IPC body once. Rust write builders and Python/TypeScript `add` expose per-call retry overrides. Atomic `create_table` pre-encodes to a temporary file and rejects compressed IPC over 256 MiB before sending any request; the schema-first import guidance now includes Python, TypeScript, and Rust examples. The shared partition target also affects local automatic partitioning. At `cb2a79473bd1b51b950c9fa3e9f247f929c8ac3b`, 70 targeted Rust cases pass on source verified against the committed four-file patch. With a 16 KiB target and 128 unique 1 KiB dictionary values, the original ordinary dictionary reproducer drops from 128 requests / 16,942,080 bytes to 16 / 143,488; ListView and LargeListView drop from 128 / 17,056,768 to 16 / 149,632. Encoded HTTP bodies are decoded to check values, nulls, slices, metadata, changing dictionaries, and overlapping views; additional cases cover framing-driven splitting, oversized rows, stable retry bytes/IDs, acknowledged progress, and atomic create. Real HTTP fault tests commit Blob payloads to a local Lance table and verify byte equality, no duplicate append, and zero abort/replay after ambiguous completion. The earlier 811 remote and 7 progress cases at `f36d217110a25f6b501c0255df9aaa60e3c8000a`, Linux Python remote database/options results (437 cases plus 8 fixture consumers), Node retry-binding evidence, and Rust/TypeScript documentation example checks remain historical validation. Python/Node extensions were not rebuilt for the dictionary follow-up; these results do not establish current binding integration or fresh cross-platform/minimum-dependency CI acceptance. A separate real Query Node lifecycle experiment used SDK `b8efe7d2d63647d4962789ca5fc042b59efe073d` and Sophon `58b7895c7c287810fff4e89f13226fc5fa60d9ef`. After the actual listener forced a connection closed, a surviving QN accepted a retry with identical upload ID, part ID, and IPC bytes. Ordinary insert and complete response-loss scenarios sent no replay or abort; fresh reads observed exactly one commit and exact Blob bytes in all three scenarios. This local experiment used a 1-second drain grace, two 32 KiB payloads, and one distinct staged part. It validates acknowledgement-loss recovery at those revisions, not concurrent overlapping attempts, large uploads, physical orphan cleanup, the later dictionary encoding change, or the original sixth-insert EOF. The part target is not a process memory limit: source/ancestor buffers and temporary compaction allocations may coexist. Struct/FixedSizeList parent nulls can retain hidden child keys; this is not complete logical-reachability GC for every Arrow combination, and retained hidden values may affect wire size. The current local Lance 15.0.0-beta.17 rejects ListView/LargeListView persistence; the tests establish remote client IPC encoding behavior, not native or Cloud persistence support. Live Cloud, full workspace/Clippy, the original ordinary-insert disconnect root cause, and Sophon's asynchronous transaction cleanup behavior are not established. Cancelling during staging retains the existing server-session cleanup limitation. Function reader delivery is separate.
This commit is contained in:
@@ -483,6 +483,65 @@ describe("remote connection", () => {
|
||||
expect(insertRequests).toBe(2);
|
||||
});
|
||||
|
||||
it.each([
|
||||
{ status: 429, retries: undefined, attempts: 2 },
|
||||
{ status: 429, retries: 0, attempts: 1 },
|
||||
{ status: 504, retries: undefined, attempts: 1 },
|
||||
{ status: 504, retries: 0, attempts: 1 },
|
||||
])("applies per-call add retries: %j", async (scenario) => {
|
||||
let attempts = 0;
|
||||
await withMockDatabase(
|
||||
(req, res) => {
|
||||
if (req.url?.endsWith("/describe/")) {
|
||||
res.writeHead(200).end(
|
||||
JSON.stringify({
|
||||
version: 1,
|
||||
schema: {
|
||||
fields: [
|
||||
{ name: "id", type: { type: "int64" }, nullable: true },
|
||||
],
|
||||
},
|
||||
}),
|
||||
);
|
||||
return;
|
||||
}
|
||||
expect(req.url).toContain("/insert/");
|
||||
req.resume();
|
||||
req.on("end", () => {
|
||||
attempts++;
|
||||
res
|
||||
.writeHead(attempts === 1 ? scenario.status : 200)
|
||||
.end('{"version":2}');
|
||||
});
|
||||
},
|
||||
async (db) => {
|
||||
const table = await db.openTable("items");
|
||||
const result = table.add([{ id: 1n }], { retries: scenario.retries });
|
||||
if (scenario.attempts === 2) {
|
||||
await expect(result).resolves.toMatchObject({ version: 2 });
|
||||
} else {
|
||||
await expect(result).rejects.toThrow();
|
||||
}
|
||||
for (const retries of [-1, 256, 1.5]) {
|
||||
await expect(table.add([{ id: 2n }], { retries })).rejects.toThrow(
|
||||
"retries must be an integer between 0 and 255",
|
||||
);
|
||||
}
|
||||
},
|
||||
{
|
||||
clientConfig: {
|
||||
retryConfig: {
|
||||
retries: 3,
|
||||
statuses: [429, 504],
|
||||
backoffFactor: 0,
|
||||
backoffJitter: 0,
|
||||
},
|
||||
},
|
||||
},
|
||||
);
|
||||
expect(attempts).toBe(scenario.attempts);
|
||||
});
|
||||
|
||||
it("should accept partial connection options", async () => {
|
||||
await connect("db://test", {
|
||||
apiKey: "fake",
|
||||
|
||||
@@ -137,6 +137,13 @@ export interface AddDataOptions {
|
||||
* when the blob is read later.
|
||||
*/
|
||||
allowExternalBlobOutsideBases?: boolean;
|
||||
|
||||
/**
|
||||
* Override the remote retry budget for this call (0 to 255). Zero disables
|
||||
* retries, including multipart parts. Ambiguous commits are never replayed.
|
||||
* Ignored by LanceDB Embedded.
|
||||
*/
|
||||
retries?: number;
|
||||
}
|
||||
|
||||
export interface UpdateOptions {
|
||||
@@ -1320,6 +1327,14 @@ export class LocalTable extends Table {
|
||||
}
|
||||
|
||||
async add(data: Data, options?: Partial<AddDataOptions>): Promise<AddResult> {
|
||||
if (
|
||||
options?.retries !== undefined &&
|
||||
(!Number.isInteger(options.retries) ||
|
||||
options.retries < 0 ||
|
||||
options.retries > 255)
|
||||
) {
|
||||
throw new Error("retries must be an integer between 0 and 255");
|
||||
}
|
||||
const mode = options?.mode ?? "append";
|
||||
const schema = await this.schema();
|
||||
|
||||
@@ -1342,6 +1357,7 @@ export class LocalTable extends Table {
|
||||
mode,
|
||||
progress,
|
||||
options?.allowExternalBlobOutsideBases,
|
||||
options?.retries,
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
+7
-1
@@ -134,7 +134,7 @@ impl Table {
|
||||
|
||||
#[napi(
|
||||
catch_unwind,
|
||||
ts_args_type = "buf: Buffer, mode: string, progressCallback?: (progress: WriteProgressInfo) => void, allowExternalBlobOutsideBases?: boolean"
|
||||
ts_args_type = "buf: Buffer, mode: string, progressCallback?: (progress: WriteProgressInfo) => void, allowExternalBlobOutsideBases?: boolean, retries?: number"
|
||||
)]
|
||||
pub async fn add(
|
||||
&self,
|
||||
@@ -142,6 +142,7 @@ impl Table {
|
||||
mode: String,
|
||||
progress_callback: Option<ProgressFn>,
|
||||
allow_external_blob_outside_bases: Option<bool>,
|
||||
retries: Option<u32>,
|
||||
) -> napi::Result<AddResult> {
|
||||
let batches = ipc_file_to_batches(buf.to_vec())
|
||||
.map_err(|e| napi::Error::from_reason(format!("Failed to read IPC file: {}", e)))?;
|
||||
@@ -157,6 +158,11 @@ impl Table {
|
||||
})
|
||||
.collect::<Result<Vec<_>>>()?;
|
||||
let mut op = self.inner_ref()?.add(batches);
|
||||
if let Some(retries) = retries {
|
||||
let retries = u8::try_from(retries)
|
||||
.map_err(|_| napi::Error::from_reason("retries must be between 0 and 255"))?;
|
||||
op = op.retries(retries);
|
||||
}
|
||||
|
||||
op = if mode == "append" {
|
||||
op.mode(AddDataMode::Append)
|
||||
|
||||
Reference in New Issue
Block a user