Skip to content

Commit a90165a

Browse files
committed
fix: guard close race and error overwrite in cache-only init
Two subtle bugs in the cache-only exhaustion branch: 1. Close race: if close() runs before runInitializers starts, the loop was skipped entirely and the exhaustion branch fired VALID and marked the start() promise resolved, where it should reject with "closed before initialization completed." Add an early return when closed is set before the exhaustion branch. 2. VALID-over-error: the branch unconditionally requested VALID on cache miss. Today's default CacheInitializer never emits error statuses, but a custom cache-marked factory could. Track errorReportedDuringInit from the interrupted/terminal_error cases and skip the VALID request when an error was reported. Adds regression tests for both paths: close-before-init rejects and emits no VALID, and a cache-only factory emitting interrupted leaves the error status intact.
1 parent c7959e8 commit a90165a

2 files changed

Lines changed: 76 additions & 4 deletions

File tree

packages/shared/sdk-client/__tests__/datasource/fdv2/FDv2DataSource.test.ts

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,62 @@ it('does not overwrite an error status when a later initializer fails after data
193193
ds.close();
194194
});
195195

196+
it('rejects start() when close() is called before cache-only initialization runs', async () => {
197+
// Race: close() happens before the runInitializers microtask starts.
198+
// The cache-only success path must not fire a spurious VALID or resolve
199+
// the start() promise; it must reject with "closed before initialization".
200+
const dataCallback = jest.fn();
201+
const statusManager = makeStatusManager();
202+
const nonePayload = makePayload({ type: 'none' });
203+
204+
const ds = createFDv2DataSource({
205+
initializerFactories: [
206+
makeCacheInitFactory(makeMockInitializer(changeSet(nonePayload, false))),
207+
],
208+
synchronizerSlots: [],
209+
dataCallback,
210+
statusManager,
211+
selectorGetter: noSelector,
212+
});
213+
214+
const startPromise = ds.start().catch((e) => e);
215+
ds.close();
216+
const error = await startPromise;
217+
218+
expect(error).toBeInstanceOf(Error);
219+
expect((error as Error).message).toContain('closed before initialization');
220+
expect(statusManager.requestStateUpdate).not.toHaveBeenCalledWith('VALID');
221+
});
222+
223+
it('does not overwrite error status when a cache-only initializer reports interrupted', async () => {
224+
// Latent guard: even though the default CacheInitializer never emits
225+
// interrupted/terminal_error, a custom cache-marked factory could.
226+
// The cache-only exhaustion branch must not overwrite the reported
227+
// error status with VALID.
228+
const dataCallback = jest.fn();
229+
const statusManager = makeStatusManager();
230+
const logger = makeLogger();
231+
232+
const ds = createFDv2DataSource({
233+
initializerFactories: [
234+
makeCacheInitFactory(makeMockInitializer(interrupted(makeErrorInfo(), false))),
235+
],
236+
synchronizerSlots: [],
237+
dataCallback,
238+
statusManager,
239+
selectorGetter: noSelector,
240+
logger,
241+
});
242+
243+
// Initialization still completes (cache-only mode is always ready) but
244+
// without overriding the reported error status.
245+
await ds.start();
246+
247+
expect(statusManager.reportError).toHaveBeenCalled();
248+
expect(statusManager.requestStateUpdate).not.toHaveBeenCalledWith('VALID');
249+
ds.close();
250+
});
251+
196252
it('rejects when a cache initializer is followed by a non-cache initializer and neither delivers data', async () => {
197253
// Cache initializer misses (transfer-none) and a non-cache initializer
198254
// also returns transfer-none. Because the chain includes a non-cache

packages/shared/sdk-client/src/datasource/fdv2/FDv2DataSource.ts

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,11 @@ export function createFDv2DataSource(config: FDv2DataSourceConfig): FDv2DataSour
145145
// The orchestration loops intentionally use await-in-loop for sequential
146146
// state machine processing — one result at a time.
147147
async function runInitializers(): Promise<void> {
148+
// Tracks whether any initializer reported interrupted/terminal_error.
149+
// Used below so the cache-only exhaustion branch does not overwrite
150+
// that error status with VALID.
151+
let errorReportedDuringInit = false;
152+
148153
while (!closed) {
149154
const initializer = sourceManager.getNextInitializerAndSetActive();
150155
if (initializer === undefined) {
@@ -181,6 +186,7 @@ export function createFDv2DataSource(config: FDv2DataSourceConfig): FDv2DataSour
181186
case 'terminal_error':
182187
logger?.warn(`Initializer failed: ${result.errorInfo?.message ?? 'unknown error'}`);
183188
reportStatusError(result);
189+
errorReportedDuringInit = true;
184190
break;
185191
case 'shutdown':
186192
return;
@@ -194,13 +200,23 @@ export function createFDv2DataSource(config: FDv2DataSourceConfig): FDv2DataSour
194200
}
195201
}
196202

203+
// close() between the last loop iteration and the exhaustion branch.
204+
// Exit without marking initialized or emitting a spurious VALID; the
205+
// start() promise will be rejected by the post-orchestration handler
206+
// with "closed before initialization completed."
207+
if (closed) {
208+
return;
209+
}
210+
197211
// All initializers exhausted.
198212
if (cacheOnlyDataSystem) {
199213
// Cache-only data system with no synchronizer to produce a VALID
200-
// status on its own. On a cache miss, nothing else has asserted
201-
// VALID yet, so do it here. On a cache hit, applyChangeSet already
202-
// asserted VALID -- skip the redundant call for idempotence.
203-
if (!dataReceived) {
214+
// status on its own. On a cache miss with no errors, nothing else
215+
// has asserted VALID yet, so do it here. Skip the update if:
216+
// - dataReceived (cache hit): applyChangeSet already asserted VALID.
217+
// - errorReportedDuringInit: reportError set an error status that
218+
// must not be silently overwritten.
219+
if (!dataReceived && !errorReportedDuringInit) {
204220
statusManager.requestStateUpdate('VALID');
205221
}
206222
markInitialized();

0 commit comments

Comments
 (0)