Skip to content

Commit 77fa6f1

Browse files
committed
Close scoped disposal races
Skip scoped configurations that are already disposing when collecting retained resources for another scope. This lets the still-running disposer finish the shared resource cleanup instead of leaking it through mutual retention. Validate scoped logger shapes before compiling them, so malformed runtime inputs raise ConfigError rather than incidental TypeError exceptions. Also copy sync disposable sets before iterating so cleanup does not mutate the set being traversed. #188 (comment) #188 (comment) #188 (comment) Assisted-by: Codex:gpt-5.5
1 parent 8021cec commit 77fa6f1

3 files changed

Lines changed: 160 additions & 15 deletions

File tree

packages/logtape/src/config.test.ts

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,46 @@ test("withConfig() rejects invalid scoped configuration", async () => {
590590
}, () => {}),
591591
ConfigError,
592592
);
593+
await assert.rejects(
594+
() =>
595+
withConfig({
596+
sinks: {},
597+
loggers: [null],
598+
} as never, () => {}),
599+
ConfigError,
600+
);
601+
await assert.rejects(
602+
() =>
603+
withConfig({
604+
sinks: {},
605+
loggers: [{ category: 1 }],
606+
} as never, () => {}),
607+
ConfigError,
608+
);
609+
await assert.rejects(
610+
() =>
611+
withConfig({
612+
sinks: {},
613+
loggers: [{ category: ["app", 1] }],
614+
} as never, () => {}),
615+
ConfigError,
616+
);
617+
await assert.rejects(
618+
() =>
619+
withConfig({
620+
sinks: {},
621+
loggers: [{ category: "app", sinks: "sink" }],
622+
} as never, () => {}),
623+
ConfigError,
624+
);
625+
await assert.rejects(
626+
() =>
627+
withConfig({
628+
sinks: {},
629+
loggers: [{ category: "app", filters: "filter" }],
630+
} as never, () => {}),
631+
ConfigError,
632+
);
593633
} finally {
594634
await reset();
595635
}
@@ -951,6 +991,68 @@ test("withConfig() does not dispose resources still owned by sibling scopes", as
951991
}
952992
});
953993

994+
test("withConfig() disposes resources after overlapping scopes finish", async () => {
995+
const events: string[] = [];
996+
const sharedSink: Sink & AsyncDisposable = () => {};
997+
sharedSink[Symbol.asyncDispose] = async () => {
998+
await Promise.resolve();
999+
events.push("shared sink");
1000+
};
1001+
let releaseFirstDisposal!: () => void;
1002+
let markFirstDisposalStarted!: () => void;
1003+
const firstDisposalStarted = new Promise<void>((resolve) => {
1004+
markFirstDisposalStarted = resolve;
1005+
});
1006+
const firstDisposalRelease = new Promise<void>((resolve) => {
1007+
releaseFirstDisposal = resolve;
1008+
});
1009+
const firstFilter: Filter & AsyncDisposable = () => true;
1010+
firstFilter[Symbol.asyncDispose] = async () => {
1011+
markFirstDisposalStarted();
1012+
await firstDisposalRelease;
1013+
events.push("first filter");
1014+
};
1015+
1016+
await configure({
1017+
sinks: {},
1018+
loggers: [{ category: ["logtape", "meta"], sinks: [] }],
1019+
contextLocalStorage: new AsyncLocalStorage(),
1020+
reset: true,
1021+
});
1022+
1023+
try {
1024+
let finishFirst!: () => void;
1025+
const releaseFirst = new Promise<void>((resolve) => {
1026+
finishFirst = resolve;
1027+
});
1028+
const firstDone = withConfig({
1029+
sinks: { shared: sharedSink },
1030+
filters: { first: firstFilter },
1031+
loggers: [
1032+
{ category: "app", filters: ["first"], sinks: ["shared"] },
1033+
],
1034+
}, async () => {
1035+
await releaseFirst;
1036+
});
1037+
const secondDone = withConfig({
1038+
sinks: { shared: sharedSink },
1039+
loggers: [{ category: "app", sinks: ["shared"] }],
1040+
}, async () => {
1041+
finishFirst();
1042+
await firstDisposalStarted;
1043+
});
1044+
1045+
await secondDone;
1046+
assert.deepStrictEqual(events, ["shared sink"]);
1047+
releaseFirstDisposal();
1048+
await firstDone;
1049+
1050+
assert.deepStrictEqual(events, ["shared sink", "first filter"]);
1051+
} finally {
1052+
await reset();
1053+
}
1054+
});
1055+
9541056
test("withConfig() does not dispose resources still owned globally", async () => {
9551057
const records: LogRecord[] = [];
9561058
const events: string[] = [];

packages/logtape/src/config.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -459,7 +459,9 @@ function getRetainedDisposables(
459459
getGlobalDisposables(),
460460
);
461461
for (const activeScopedConfig of activeScopedConfigs) {
462-
if (activeScopedConfig === scopedConfig) continue;
462+
if (activeScopedConfig === scopedConfig || activeScopedConfig.disposed) {
463+
continue;
464+
}
463465
addScopedConfigDisposables(disposables, activeScopedConfig);
464466
}
465467
return disposables;

packages/logtape/src/scoped-config.ts

Lines changed: 55 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,38 @@ export function compileScopedConfig<
8181
const nodes = new Map<string, CompiledScopedLogger>();
8282
const configuredCategories = new Set<string>();
8383
for (const logger of config.loggers) {
84-
const category = normalizeCategory(logger.category);
84+
if (!isObjectLike(logger)) {
85+
throw createError("Logger configuration must be an object.");
86+
}
87+
const loggerConfig = logger as ScopedLoggerConfigLike<TSinkId, TFilterId>;
88+
const category = normalizeCategory(loggerConfig.category, createError);
89+
if (
90+
loggerConfig.sinks !== undefined && !Array.isArray(loggerConfig.sinks)
91+
) {
92+
throw createError("Logger sinks must be an array.");
93+
}
94+
if (
95+
loggerConfig.filters !== undefined &&
96+
!Array.isArray(loggerConfig.filters)
97+
) {
98+
throw createError("Logger filters must be an array.");
99+
}
100+
if (
101+
loggerConfig.parentSinks !== undefined &&
102+
loggerConfig.parentSinks !== "inherit" &&
103+
loggerConfig.parentSinks !== "override"
104+
) {
105+
throw createError(
106+
'Logger parentSinks must be "inherit" or "override".',
107+
);
108+
}
109+
if (
110+
loggerConfig.lowestLevel !== undefined &&
111+
loggerConfig.lowestLevel !== null &&
112+
!isLogLevel(loggerConfig.lowestLevel)
113+
) {
114+
throw createError("Logger lowestLevel must be a log level or null.");
115+
}
85116
const key = categoryKey(category);
86117
if (configuredCategories.has(key)) {
87118
throw createError(
@@ -92,7 +123,7 @@ export function compileScopedConfig<
92123
configuredCategories.add(key);
93124

94125
const sinks: Sink[] = [];
95-
const sinkIds: readonly TSinkId[] = logger.sinks ?? [];
126+
const sinkIds: readonly TSinkId[] = loggerConfig.sinks ?? [];
96127
for (const sinkId of sinkIds) {
97128
const sink = config.sinks[sinkId];
98129
if (!sink) throw createError(`Sink not found: ${sinkId}.`);
@@ -103,7 +134,7 @@ export function compileScopedConfig<
103134
}
104135

105136
const filters: ((record: LogRecord) => boolean)[] = [];
106-
const filterIds: readonly TFilterId[] = logger.filters ?? [];
137+
const filterIds: readonly TFilterId[] = loggerConfig.filters ?? [];
107138
for (const filterId of filterIds) {
108139
const filter = config.filters?.[filterId];
109140
if (filter === undefined) {
@@ -119,10 +150,10 @@ export function compileScopedConfig<
119150

120151
nodes.set(key, {
121152
filters,
122-
lowestLevel: logger.lowestLevel === undefined
153+
lowestLevel: loggerConfig.lowestLevel === undefined
123154
? "trace"
124-
: logger.lowestLevel,
125-
parentSinks: logger.parentSinks ?? "inherit",
155+
: loggerConfig.lowestLevel,
156+
parentSinks: loggerConfig.parentSinks ?? "inherit",
126157
sinks,
127158
});
128159
}
@@ -349,8 +380,18 @@ function addScopedDisposables(
349380
for (const disposable of scopedConfig.asyncSinks) disposables.add(disposable);
350381
}
351382

352-
function normalizeCategory(category: string | readonly string[]): string[] {
353-
return typeof category === "string" ? [category] : [...category];
383+
function normalizeCategory(
384+
category: unknown,
385+
createError: (message: string) => Error,
386+
): string[] {
387+
if (typeof category === "string") return [category];
388+
if (!Array.isArray(category)) {
389+
throw createError("Logger category must be a string or array of strings.");
390+
}
391+
if (category.some((part) => typeof part !== "string")) {
392+
throw createError("Logger category must only contain strings.");
393+
}
394+
return [...category];
354395
}
355396

356397
function categoryKey(category: readonly string[]): string {
@@ -443,17 +484,17 @@ function disposeSyncDisposables(
443484
disposables: Set<Disposable>,
444485
retainedDisposables: ReadonlySet<Disposable | AsyncDisposable>,
445486
): void {
487+
const disposableList = filterRetainedDisposables(
488+
disposables,
489+
retainedDisposables,
490+
);
446491
const errors: unknown[] = [];
447492
try {
448-
for (const disposable of disposables) {
493+
for (const disposable of disposableList) {
449494
try {
450-
if (!retainedDisposables.has(disposable)) {
451-
disposable[Symbol.dispose]();
452-
}
495+
disposable[Symbol.dispose]();
453496
} catch (error) {
454497
errors.push(error);
455-
} finally {
456-
disposables.delete(disposable);
457498
}
458499
}
459500
} finally {

0 commit comments

Comments
 (0)