Skip to content

Commit 4d2f685

Browse files
Apollon77claude
Apollon77
and
claude
authored
fix(general): close env services before removing them (#3668)
* fix(general): close env services before removing them Environment.close removed the service from the env synchronously and only then awaited close(). Close-time consumers that transitively resolve the service via env.get() failed with "unsupported-dependency" — observed when the WAL final snapshot reached ClientNode.toString → store → env.get(ServerNodeStore) during ServerNodeStore.close. Reorder to close-then-delete via MaybePromise.finally so the service stays resolvable while close() runs and the local slot is cleared regardless of sync throw or async rejection. This matches the existing JSDoc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(general): split close() no-instance / shared paths Per PR review: previous comment "just block local inheritance" was misleading for the shared-service case where delete() is a no-op. Split into two branches: no-instance still nulls the local slot (preserves parent-inheritance blocking, e.g. ClientNode constructor's env.close(OccurrenceManager)); shared returns without delete since the no-op was meaningless. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 3b65344 commit 4d2f685

2 files changed

Lines changed: 104 additions & 9 deletions

File tree

‎packages/general/src/environment/Environment.ts‎

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -191,18 +191,27 @@ export class Environment implements ServiceProvider, Lifetime.Owner {
191191
close<T extends object>(
192192
type: Environmental.ServiceType<T>,
193193
): T extends { close: () => MaybePromise<void> } ? MaybePromise<void> : void {
194+
type Result = T extends { close: () => MaybePromise<void> } ? MaybePromise<void> : void;
195+
194196
const instance = this.maybeGet(type);
195-
this.delete(type, instance);
196-
if (instance !== undefined) {
197-
if (this.get(SharedServicesManager).has(type)) {
198-
// still in use
199-
return;
200-
}
201197

202-
return (instance as Partial<Destructable>).close?.() as T extends { close: () => MaybePromise<void> }
203-
? MaybePromise<void>
204-
: void;
198+
// No instance to close — still null the local slot to block future inheritance from parent.
199+
if (instance === undefined) {
200+
this.delete(type, instance);
201+
return undefined as Result;
202+
}
203+
204+
// Shared service still in use elsewhere; do not close or remove it here.
205+
if (this.get(SharedServicesManager).has(type)) {
206+
return undefined as Result;
205207
}
208+
209+
// Close before deleting so close-time env.get(type) still resolves; finally guarantees cleanup
210+
// even if close rejects.
211+
return MaybePromise.finally(
212+
() => (instance as Partial<Destructable>).close?.(),
213+
() => this.delete(type, instance),
214+
) as MaybePromise<void> as Result;
206215
}
207216

208217
/**

‎packages/general/test/environment/EnvironmentTest.ts‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,92 @@ describe("Environment", () => {
100100
});
101101
});
102102

103+
describe("close() ordering", () => {
104+
it("keeps service resolvable inside its own sync close", () => {
105+
let resolvedDuringClose: SelfRefService | undefined;
106+
107+
class SelfRefService {
108+
static [Environmental.create](environment: Environment) {
109+
return environment.get(SelfRefService);
110+
}
111+
constructor(public env: Environment) {}
112+
close() {
113+
resolvedDuringClose = this.env.get(SelfRefService);
114+
}
115+
}
116+
117+
const svc = new SelfRefService(env);
118+
env.set(SelfRefService, svc);
119+
120+
env.close(SelfRefService);
121+
122+
expect(resolvedDuringClose).to.equal(svc);
123+
expect(env.has(SelfRefService)).to.be.false;
124+
});
125+
126+
it("keeps service resolvable inside its own async close", async () => {
127+
let resolvedDuringClose: AsyncSelfRefService | undefined;
128+
129+
class AsyncSelfRefService {
130+
static [Environmental.create](environment: Environment) {
131+
return environment.get(AsyncSelfRefService);
132+
}
133+
constructor(public env: Environment) {}
134+
async close() {
135+
await Promise.resolve();
136+
resolvedDuringClose = this.env.get(AsyncSelfRefService);
137+
}
138+
}
139+
140+
const svc = new AsyncSelfRefService(env);
141+
env.set(AsyncSelfRefService, svc);
142+
143+
await env.close(AsyncSelfRefService);
144+
145+
expect(resolvedDuringClose).to.equal(svc);
146+
expect(env.has(AsyncSelfRefService)).to.be.false;
147+
});
148+
149+
it("removes service from env even when sync close throws", () => {
150+
class ThrowingService {
151+
static [Environmental.create](environment: Environment) {
152+
return environment.get(ThrowingService);
153+
}
154+
close() {
155+
throw new Error("close failed");
156+
}
157+
}
158+
159+
env.set(ThrowingService, new ThrowingService());
160+
161+
expect(() => env.close(ThrowingService)).to.throw("close failed");
162+
expect(env.has(ThrowingService)).to.be.false;
163+
});
164+
165+
it("removes service from env even when async close rejects", async () => {
166+
class RejectingService {
167+
static [Environmental.create](environment: Environment) {
168+
return environment.get(RejectingService);
169+
}
170+
async close() {
171+
throw new Error("close rejected");
172+
}
173+
}
174+
175+
env.set(RejectingService, new RejectingService());
176+
177+
let error: Error | undefined;
178+
try {
179+
await env.close(RejectingService);
180+
} catch (e) {
181+
error = e as Error;
182+
}
183+
184+
expect(error?.message).to.equal("close rejected");
185+
expect(env.has(RejectingService)).to.be.false;
186+
});
187+
});
188+
103189
describe("dependent tracking", () => {
104190
let dependent1: SharedEnvironmentServices;
105191
let dependent2: SharedEnvironmentServices;

0 commit comments

Comments
 (0)