Skip to content

Commit 34412e1

Browse files
rahuls-dbIsaac
andcommitted
Address review: close fallback KernelBackend, test TTL expiry
- ThriftBackend: track the KernelBackend(s) created for Reyden (KP001) fallback and close them in close(), so the process-global log-bridge onLevelChange listener installed by connect() is released instead of leaking on every recovery. Add a createKernelBackend() seam so tests can inject a fake without the native binding, plus a test that close() releases the fallback backend. - ReydenWarehouseCache test: add a TTL-expiry test (sinon fake timers) covering the 6h boundary and opportunistic eviction on access. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
1 parent 7ca7f58 commit 34412e1

3 files changed

Lines changed: 70 additions & 4 deletions

File tree

lib/thrift-backend/ThriftBackend.ts

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,11 @@ export default class ThriftBackend implements IBackend {
3737

3838
private connectionOptions?: ConnectionOptions;
3939

40+
// KernelBackend(s) created for Reyden (KP001) fallback. Tracked so their
41+
// process-global log-bridge listeners are released on close() — otherwise each
42+
// fallback session would leak an onLevelChange listener for the process lifetime.
43+
private fallbackKernelBackends: KernelBackend[] = [];
44+
4045
constructor({ context, onConnectionEvent }: ThriftBackendOptions) {
4146
this.context = context;
4247
this.onConnectionEvent = onConnectionEvent;
@@ -184,14 +189,24 @@ export default class ThriftBackend implements IBackend {
184189
const logger = this.context.getLogger();
185190
logger.log(LogLevel.debug, 'Reyden: opening session via KernelBackend (SEA)');
186191

187-
// Create a new KernelBackend instance and connect/open
188-
const kernelBackend = new KernelBackend({ context: this.context });
192+
// Create a KernelBackend and connect/open. Track it so close() releases the
193+
// log-bridge listener that connect() installs.
194+
const kernelBackend = this.createKernelBackend();
195+
this.fallbackKernelBackends.push(kernelBackend);
189196
await kernelBackend.connect(this.connectionOptions);
190197
return kernelBackend.openSession(request);
191198
}
192199

200+
// Seam so tests can inject a fake KernelBackend without the native binding.
201+
protected createKernelBackend(): KernelBackend {
202+
return new KernelBackend({ context: this.context });
203+
}
204+
193205
public async close(): Promise<void> {
194-
// DBSQLClient owns the connection lifecycle and clears its own state
195-
// (connectionProvider, authProvider, thrift client) after this returns.
206+
// Release the process-global log-bridge listener(s) held by any Reyden-fallback
207+
// KernelBackend. DBSQLClient owns the rest of the connection lifecycle and clears
208+
// its own state (connectionProvider, authProvider, thrift client) after this returns.
209+
await Promise.all(this.fallbackKernelBackends.map((backend) => backend.close()));
210+
this.fallbackKernelBackends = [];
196211
}
197212
}

tests/unit/thrift-backend/ReydenThriftRecovery.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { expect } from 'chai';
2+
import sinon from 'sinon';
23
import reydenCache from '../../../lib/ReydenWarehouseCache';
34
import ThriftBackend from '../../../lib/thrift-backend/ThriftBackend';
45
import StatusError from '../../../lib/errors/StatusError';
@@ -97,6 +98,36 @@ describe('Reyden Warehouse Cache', () => {
9798
expect(reydenCache.isKnownReyden('host1.com', 'warehouse-1')).to.be.undefined;
9899
});
99100
});
101+
102+
describe('Cache TTL Expiry', () => {
103+
let clock: sinon.SinonFakeTimers;
104+
105+
beforeEach(() => {
106+
clock = sinon.useFakeTimers();
107+
});
108+
109+
afterEach(() => {
110+
clock.restore();
111+
});
112+
113+
it('keeps an entry until the 6h TTL, then evicts it on access', () => {
114+
const host = 'example.com';
115+
const warehouseId = 'warehouse-ttl';
116+
const sixHoursMs = 6 * 60 * 60 * 1000;
117+
118+
reydenCache.markReyden(host, warehouseId);
119+
expect(reydenCache.isKnownReyden(host, warehouseId)).to.be.true;
120+
121+
// At exactly the TTL boundary the entry is still valid (strict >).
122+
clock.tick(sixHoursMs);
123+
expect(reydenCache.isKnownReyden(host, warehouseId)).to.be.true;
124+
125+
// One tick past the TTL: expired, evicted on access.
126+
clock.tick(1);
127+
expect(reydenCache.isKnownReyden(host, warehouseId)).to.be.undefined;
128+
expect(reydenCache.size()).to.equal(0);
129+
});
130+
});
100131
});
101132

102133
describe('StatusError SQLSTATE Support', () => {

tests/unit/thrift-backend/ReydenThriftRecoveryOrchestration.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,4 +125,24 @@ describe('Reyden Thrift Auto-Recovery — Orchestration', () => {
125125
expect(thrown).to.equal(kernelError);
126126
expect(thrown.cause).to.equal(thriftError);
127127
});
128+
129+
it('closes the fallback KernelBackend on close(), releasing its log-bridge listener', async () => {
130+
const backend = makeBackend();
131+
const fakeKernel = {
132+
connect: sandbox.stub().resolves(),
133+
openSession: sandbox.stub().resolves({ marker: 'kernel-session' } as any),
134+
close: sandbox.stub().resolves(),
135+
};
136+
sandbox.stub(backend as any, 'openSessionWithThrift').rejects(kp001Error());
137+
// Inject the fake via the createKernelBackend seam and let the REAL
138+
// openSessionWithKernelBackend run (connect + track), so close() must release it.
139+
sandbox.stub(backend as any, 'createKernelBackend').returns(fakeKernel as any);
140+
141+
await backend.openSession({} as any);
142+
expect(fakeKernel.connect.calledOnce).to.be.true;
143+
expect(fakeKernel.close.called).to.be.false; // still open
144+
145+
await backend.close();
146+
expect(fakeKernel.close.calledOnce).to.be.true; // released on ThriftBackend.close()
147+
});
128148
});

0 commit comments

Comments
 (0)