Skip to content

Commit ef770d2

Browse files
committed
fix: simplify MetricsAggregator.close() flush race fix
Replace completeStatementForClose/flushForClose toggle pattern with a closing flag that suppresses batch-triggered fire-and-forget flushes. Set closing=true first so completeStatement works normally via addPendingMetric (closed is still false), then seal with closed=true and drain with a single awaited flush(false). Co-authored-by: samikshya-chand_data
1 parent d80f13e commit ef770d2

1 file changed

Lines changed: 8 additions & 36 deletions

File tree

lib/telemetry/MetricsAggregator.ts

Lines changed: 8 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -322,8 +322,6 @@ export default class MetricsAggregator {
322322
if (this.pendingMetrics.length >= this.batchSize && !this.closing) {
323323
// resetTimer=false so the periodic tail-drain keeps its cadence even
324324
// under sustained batch-size bursts.
325-
// Suppressed during close() so fire-and-forget promises don't race past
326-
// the single awaited flushForClose().
327325
const logger = this.context.getLogger();
328326
Promise.resolve(this.flush(false)).catch((err: any) => {
329327
logger.log(LogLevel.debug, `Batch-trigger flush failed: ${err?.message ?? err}`);
@@ -410,53 +408,27 @@ export default class MetricsAggregator {
410408

411409
async close(): Promise<void> {
412410
const logger = this.context.getLogger();
413-
this.closing = true;
414-
this.closed = true;
415411

416412
try {
413+
// Suppress batch-triggered fire-and-forget flushes from addPendingMetric
414+
// so no promises escape past the single awaited flush below.
415+
this.closing = true;
416+
417417
if (this.flushTimer) {
418418
clearInterval(this.flushTimer);
419419
this.flushTimer = null;
420420
}
421421

422-
// Snapshot keys — completeStatement mutates statementMetrics.
422+
// closed is still false here so completeStatement → addPendingMetric works normally.
423423
const remainingStatements = [...this.statementMetrics.keys()];
424424
for (const statementId of remainingStatements) {
425-
this.completeStatementForClose(statementId);
425+
this.completeStatement(statementId);
426426
}
427427

428-
await this.flushForClose();
429-
430-
// Belt-and-braces: something the above awaited could in principle
431-
// have resurrected a timer. Clear once more.
432-
if (this.flushTimer) {
433-
clearInterval(this.flushTimer);
434-
this.flushTimer = null;
435-
}
428+
this.closed = true;
429+
await this.flush(false);
436430
} catch (error: any) {
437431
logger.log(LogLevel.debug, `MetricsAggregator.close error: ${error.message}`);
438432
}
439433
}
440-
441-
/** completeStatement variant that bypasses the `closed` guard. */
442-
private completeStatementForClose(statementId: string): void {
443-
const prev = this.closed;
444-
this.closed = false;
445-
try {
446-
this.completeStatement(statementId);
447-
} finally {
448-
this.closed = prev;
449-
}
450-
}
451-
452-
/** flush variant that bypasses the `closed` guard on addPendingMetric. */
453-
private async flushForClose(): Promise<void> {
454-
const prev = this.closed;
455-
this.closed = false;
456-
try {
457-
await this.flush(false);
458-
} finally {
459-
this.closed = prev;
460-
}
461-
}
462434
}

0 commit comments

Comments
 (0)