Skip to content

Commit dc8fa30

Browse files
authored
Merge pull request #2064 from HarperFast/fix/2049-reject-table-scoped-rocksdb-log-purge
`delete_transaction_logs_before` with a table on RocksDB now returns an error instead of deleting the entire database's transaction log
2 parents 2976e21 + 4bd7817 commit dc8fa30

6 files changed

Lines changed: 177 additions & 16 deletions

File tree

DESIGN.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -479,6 +479,20 @@ corrupt by verify). This closes the "healthy-looking but incomplete" and concurr
479479
the remaining engine/blob point-in-time skew (a blob unlinked between the engine cut and the blob
480480
walk) is the documented best-effort limitation above.
481481

482+
## RocksDB transaction log purges are database-wide only (`ResourceBridge.deleteTransactionLogsBefore`)
483+
484+
On RocksDB, every table in a database writes to one shared set of transaction logs (partitioned per
485+
origin node, not per table), and `purgeLogs()` deletes whole log files — rocksdb-js has no table
486+
filter, and adding one would mean rewriting files instead of deleting them. So a table-scoped
487+
`delete_transaction_logs_before` is unimplementable at the storage layer; the bridge rejects
488+
`table` on RocksDB with a 400 rather than silently purging every sibling table's history
489+
(harper#2049 — the original code did exactly that, and a _typo'd_ table name did too, because a
490+
missing table fell through to the no-table branch; that now 404s). Two consequences to preserve:
491+
the deprecated `delete_audit_logs_before` op _requires_ `table`, so it always errors on RocksDB
492+
(the message steers callers to the new op without `table`); and the table/no-table checks in the
493+
bridge use `!= null` presence, not truthiness, so a table named `"0"` addressed numerically stays
494+
table-scoped instead of widening to a database purge.
495+
482496
## Scheduler: cluster-once execution without a consensus primitive (`resources/scheduler/`)
483497

484498
The built-in `scheduler` plugin (#951) runs config-declared jobs "exactly once per cluster." The

dataLayer/harperBridge/ResourceBridge.ts

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -492,12 +492,25 @@ export class ResourceBridge extends BridgeMethods {
492492
: typeof deleteObj.timestamp === 'string'
493493
? Number.parseInt(deleteObj.timestamp)
494494
: deleteObj.timestamp;
495+
const databaseName = deleteObj.database || deleteObj.schema || DEFAULT_DATABASE;
495496
const table = getTable(deleteObj);
497+
// A nonexistent table must not fall through to the no-table branch below — on RocksDB that
498+
// widens a table-scoped request (e.g. a typo) into a whole-database log purge (#2049).
499+
// Presence check, not truthiness: a table named "0" addressed numerically is still table-scoped.
500+
if (deleteObj.table != null && !table)
501+
throw handleHDBError(
502+
new Error(),
503+
HDB_ERROR_MSGS.TABLE_NOT_FOUND(databaseName, deleteObj.table),
504+
404,
505+
undefined,
506+
undefined,
507+
true
508+
);
496509
if (!table) {
497510
// no table, check if any of the tables are RocksDB
498511
// since all tables share the same transaction log store, we break after the first
499512
// RocksDB table is found
500-
const tables = getDatabases()[deleteObj.database];
513+
const tables = getDatabases()[databaseName];
501514
if (tables) {
502515
for (const table of Object.values(tables)) {
503516
if (table.primaryStore instanceof RocksDatabase) {
@@ -509,9 +522,16 @@ export class ResourceBridge extends BridgeMethods {
509522
}
510523
}
511524
} else if (table.primaryStore instanceof RocksDatabase) {
512-
const deleted = table.primaryStore.purgeLogs({ before, includeEntryCounts: true });
513-
totalResults.log_files_deleted += deleted.length;
514-
totalResults.entries_deleted += deleted.reduce((acc, file) => acc + file.entries, 0);
525+
// All tables in a RocksDB database share one transaction log with no per-table purge
526+
// granularity; honoring `table` here would silently purge every sibling table's log (#2049).
527+
throw handleHDBError(
528+
new Error(),
529+
`Table-level transaction log deletion is not supported for RocksDB tables because all tables in a database share one transaction log; to delete the transaction logs for the entire '${databaseName}' database, use delete_transaction_logs_before with only 'database' and 'timestamp'`,
530+
400,
531+
undefined,
532+
undefined,
533+
true
534+
);
515535
} else {
516536
totalResults.entries_deleted += await table.deleteHistory(before, deleteObj.cleanup_deleted_records);
517537
}
@@ -698,7 +718,8 @@ function getTable(operationObject: { database?: string; schema?: string; table?:
698718
const databaseName = operationObject.database || operationObject.schema || DEFAULT_DATABASE;
699719
const tables = getDatabases()[databaseName];
700720
if (!tables) throw handleHDBError(new Error(), HDB_ERROR_MSGS.SCHEMA_NOT_FOUND(databaseName), 404);
701-
return operationObject.table ? tables[operationObject.table] : undefined;
721+
// Presence check, not truthiness, so a table named "0" resolves when addressed numerically.
722+
return operationObject.table != null ? tables[operationObject.table] : undefined;
702723
}
703724

704725
/**

integrationTests/apiTests/terminology.test.mjs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -452,6 +452,10 @@ suite('Terminology aliases (database / primary_key)', (ctx) => {
452452
await awaitJobCompleted(client, r.body.job_id, { timeoutSeconds: JOB_TIMEOUT_SECONDS });
453453
});
454454

455+
// The two delete_audit_logs_before jobs below start fine but ERROR on RocksDB:
456+
// the deprecated op requires `table`, and table-scoped transaction log deletion
457+
// is rejected there because all tables in a database share one log (harper#2049).
458+
// The database/schema param handling under test still resolves before that error.
455459
test('delete_audit_logs_before with database param starts job', async () => {
456460
const r = await client
457461
.req()
@@ -463,7 +467,11 @@ suite('Terminology aliases (database / primary_key)', (ctx) => {
463467
})
464468
.expect((r) => assert.ok(r.body.message.includes('Starting job with id'), r.text))
465469
.expect(200);
466-
await awaitJobCompleted(client, r.body.job_id, { timeoutSeconds: JOB_TIMEOUT_SECONDS });
470+
await awaitJobCompleted(client, r.body.job_id, {
471+
timeoutSeconds: JOB_TIMEOUT_SECONDS,
472+
// the error echoes the resolved database, proving the `database` param was honored
473+
expectedError: "transaction logs for the entire 'job_guy' database",
474+
});
467475
});
468476

469477
test('delete_audit_logs_before without database starts job', async () => {
@@ -472,7 +480,11 @@ suite('Terminology aliases (database / primary_key)', (ctx) => {
472480
.send({ operation: 'delete_audit_logs_before', table: 'friends', timestamp: 1690553291764 })
473481
.expect((r) => assert.ok(r.body.message.includes('Starting job with id'), r.text))
474482
.expect(200);
475-
await awaitJobCompleted(client, r.body.job_id, { timeoutSeconds: JOB_TIMEOUT_SECONDS });
483+
await awaitJobCompleted(client, r.body.job_id, {
484+
timeoutSeconds: JOB_TIMEOUT_SECONDS,
485+
// no database param resolves to the default 'data' database
486+
expectedError: "transaction logs for the entire 'data' database",
487+
});
476488
});
477489

478490
test('csv_file_load with database param starts job', async () => {

integrationTests/apiTests/transaction-logs.test.mjs

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,14 +113,14 @@ suite('Transaction Logs', (ctx) => {
113113
test('delete_transaction_logs_before suiteStart deletes no files', async () => {
114114
// `suiteStart` is from before any inserts happened — no log files should
115115
// have been rotated out yet, so the job should run to completion with
116-
// `log_files_deleted: 0` and `entries_deleted: 0`.
116+
// `log_files_deleted: 0` and `entries_deleted: 0`. No `table`: on RocksDB
117+
// the operation is database-wide only (harper#2049).
117118
const response = await client
118119
.req()
119120
.send({
120121
operation: 'delete_transaction_logs_before',
121122
timestamp: `${suiteStart}`,
122123
schema: SCHEMA,
123-
table: TABLE,
124124
})
125125
.expect(200);
126126

@@ -130,6 +130,26 @@ suite('Transaction Logs', (ctx) => {
130130
assert.equal(jobResponse.body[0].result.entries_deleted, 0, jobResponse.text);
131131
});
132132

133+
test('delete_transaction_logs_before with a table is rejected on RocksDB', async () => {
134+
// All tables in a RocksDB database share one transaction log, so a
135+
// table-scoped delete used to silently purge every sibling table's log
136+
// (harper#2049). It must fail instead of widening the scope.
137+
const response = await client
138+
.req()
139+
.send({
140+
operation: 'delete_transaction_logs_before',
141+
timestamp: `${suiteStart}`,
142+
schema: SCHEMA,
143+
table: TABLE,
144+
})
145+
.expect(200);
146+
147+
const jobId = getJobId(response.body);
148+
const jobResponse = await awaitJob(client, jobId, 15);
149+
assert.equal(jobResponse.body[0].status, 'ERROR', jobResponse.text);
150+
assert.ok(JSON.stringify(jobResponse.body[0].message).includes('not supported for RocksDB'), jobResponse.text);
151+
});
152+
133153
test('drop test_logs table', async () => {
134154
await client.req().send({ operation: 'drop_table', schema: SCHEMA, table: TABLE }).expect(200);
135155
});

integrationTests/apiTests/transactions.test.mjs

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,12 @@ suite('Transactions / audit log', (ctx) => {
8989
.expect(200);
9090
});
9191

92-
test('delete_audit_logs_before returns deprecation hint', async () => {
92+
test('delete_audit_logs_before is rejected on RocksDB', async () => {
93+
// The deprecated op requires `table`, and on RocksDB table-scoped
94+
// transaction log deletion is rejected because all tables in a database
95+
// share one log (harper#2049) — so on RocksDB this op always fails with a
96+
// message steering callers to delete_transaction_logs_before without a
97+
// table. The success path (and its `deprecated` hint) is LMDB-only.
9398
const response = await client
9499
.req()
95100
.send({
@@ -102,12 +107,8 @@ suite('Transactions / audit log', (ctx) => {
102107

103108
const jobId = getJobId(response.body);
104109
const jobResponse = await awaitJob(client, jobId, 15);
105-
assert.ok(jobResponse.body[0].message.includes('Successfully completed'), jobResponse.text);
106-
assert.equal(
107-
jobResponse.body[0].result?.deprecated,
108-
'Please use delete_transaction_logs_before instead',
109-
jobResponse.text
110-
);
110+
assert.equal(jobResponse.body[0].status, 'ERROR', jobResponse.text);
111+
assert.ok(JSON.stringify(jobResponse.body[0].message).includes('not supported for RocksDB'), jobResponse.text);
111112
});
112113

113114
test('create test_read table', async () => {
Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
/**
2+
* Regression guards for harper#2049: on RocksDB, all tables in a database share one
3+
* transaction log with no per-table purge granularity, so a table-scoped
4+
* delete_transaction_logs_before used to silently purge EVERY table's log in the
5+
* database. A nonexistent table name (e.g. a typo) fell through to the same
6+
* whole-database purge. Both must be rejected before any purge happens.
7+
*/
8+
require('../testUtils');
9+
const assert = require('node:assert');
10+
const { setupTestDBPath } = require('../testUtils');
11+
const { table } = require('#src/resources/databases');
12+
const { setMainIsWorker } = require('#js/server/threads/manageThreads');
13+
const harperBridge = require('#src/dataLayer/harperBridge/harperBridge').default;
14+
15+
describe('deleteTransactionLogsBefore on RocksDB (harper#2049)', () => {
16+
if (process.env.HARPER_STORAGE_ENGINE === 'lmdb') return;
17+
18+
const DB = 'txnLogPurgeScope';
19+
let TableA, TableB;
20+
21+
before(async () => {
22+
setupTestDBPath();
23+
setMainIsWorker(true);
24+
const attributes = [{ name: 'id', isPrimaryKey: true }, { name: 'name' }];
25+
TableA = table({ table: 'TableA', database: DB, attributes });
26+
TableB = table({ table: 'TableB', database: DB, attributes });
27+
await TableA.put(1, { name: 'a1' });
28+
await TableA.put(2, { name: 'a2' });
29+
await TableB.put(1, { name: 'b1' });
30+
});
31+
32+
async function historyCount(tbl) {
33+
let count = 0;
34+
for await (const _entry of tbl.getHistory()) count++;
35+
return count;
36+
}
37+
38+
it('rejects a table-scoped delete with a 400 and purges nothing', async () => {
39+
await assert.rejects(
40+
harperBridge.deleteTransactionLogsBefore({ database: DB, table: 'TableA', timestamp: Date.now() + 1000 }),
41+
(error) => {
42+
assert.strictEqual(error.statusCode, 400);
43+
assert.match(error.message, /not supported for RocksDB/);
44+
return true;
45+
}
46+
);
47+
assert.ok((await historyCount(TableA)) >= 2, 'TableA history should be untouched');
48+
assert.ok((await historyCount(TableB)) >= 1, 'TableB history should be untouched');
49+
});
50+
51+
it('rejects a nonexistent table with a 404 instead of purging the whole database', async () => {
52+
await assert.rejects(
53+
harperBridge.deleteTransactionLogsBefore({ database: DB, table: 'NoSuchTable', timestamp: Date.now() + 1000 }),
54+
(error) => {
55+
assert.strictEqual(error.statusCode, 404);
56+
return true;
57+
}
58+
);
59+
assert.ok((await historyCount(TableA)) >= 2, 'TableA history should be untouched');
60+
assert.ok((await historyCount(TableB)) >= 1, 'TableB history should be untouched');
61+
});
62+
63+
it('treats a falsy table name as table-scoped rather than database-wide', async () => {
64+
// A table named "0" addressed numerically must not fall into the no-table
65+
// branch (which would purge the whole database's log on RocksDB).
66+
table({ table: '0', database: DB, attributes: [{ name: 'id', isPrimaryKey: true }] });
67+
await assert.rejects(
68+
harperBridge.deleteTransactionLogsBefore({ database: DB, table: 0, timestamp: Date.now() + 1000 }),
69+
(error) => {
70+
assert.strictEqual(error.statusCode, 400);
71+
assert.match(error.message, /not supported for RocksDB/);
72+
return true;
73+
}
74+
);
75+
});
76+
77+
it('rejects a nonexistent database with a 404', async () => {
78+
await assert.rejects(
79+
harperBridge.deleteTransactionLogsBefore({ database: 'NoSuchDatabase', timestamp: Date.now() + 1000 }),
80+
(error) => {
81+
assert.strictEqual(error.statusCode, 404);
82+
return true;
83+
}
84+
);
85+
});
86+
87+
it('still performs the database-wide purge when no table is given', async () => {
88+
const results = await harperBridge.deleteTransactionLogsBefore({ database: DB, timestamp: Date.now() + 1000 });
89+
assert.ok(results, 'should return results rather than throw');
90+
assert.strictEqual(typeof results.entries_deleted, 'number');
91+
assert.strictEqual(typeof results.log_files_deleted, 'number');
92+
});
93+
});

0 commit comments

Comments
 (0)