Skip to content

Commit 2d5f130

Browse files
committed
Add file-meta GC coverage
1 parent 066245f commit 2d5f130

3 files changed

Lines changed: 161 additions & 29 deletions

File tree

docs/file-meta-gc-pr2-plan.md

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
# Plan: File-meta GC + reference counting (PR 2)
2+
3+
## Background
4+
5+
- FileDef instances already live in `CardStoreWithGarbageCollection`, but GC and reference counting only consider cards.
6+
- Requirement: file-meta instances should participate in GC and reference counting.
7+
- This PR is scoped to GC/reference behavior only; explicit store read types remain in PR 1.
8+
9+
## Goals
10+
11+
- Keep FileDef instances alive when they have active references.
12+
- GC FileDef instances when their references drop to zero.
13+
- Avoid changing public Store read APIs in this PR.
14+
- Keep file-meta read-only behavior (no save/auto-save).
15+
16+
## Non-goals
17+
18+
- No `StoreReadType` changes or call-site updates (handled in PR 1).
19+
- No changes to FileMetaResource or operator-mode wiring.
20+
- No change to file-meta error storage strategy (can remain in StoreService for now).
21+
22+
## Test-first plan
23+
24+
1. Add failing tests in `packages/host/tests/integration/store-test.gts`:
25+
- `file-meta instances are retained when referenced`:
26+
- Write a file (e.g., `hero.png`) into the test realm.
27+
- Load the FileDef via existing behavior (`storeService.get(fileUrl)` on main).
28+
- Add a file-meta reference (via a new internal helper or direct map access).
29+
- Run `forceGC()` and assert the FileDef is still in the store.
30+
- `file-meta instances are GC'd when references drop`:
31+
- Drop the file-meta reference.
32+
- Run `forceGC()` and assert the FileDef is removed from the store.
33+
34+
2. Run the tests to confirm they fail before implementation.
35+
36+
## Implementation plan (after tests)
37+
38+
1. Expand `CardStoreWithGarbageCollection` to understand file-meta references:
39+
- Accept a second reference-count map for file-meta (or a unified typed map).
40+
- Update `hasReferences()` to include both card and file-meta counts.
41+
- In `sweep()`, treat FileDef instances as GC candidates and roots when referenced.
42+
- Ensure `delete()` and `reset()` clear FileDef instances consistently.
43+
44+
2. Wire StoreService to pass file-meta reference counts into CardStore:
45+
- Add `fileMetaReferenceCount` on StoreService (private).
46+
- Provide a minimal internal way to increment/decrement file-meta references
47+
(used by tests in this PR; public API stays unchanged).
48+
- Pass both reference maps into `CardStoreWithGarbageCollection`.
49+
50+
3. Re-run tests and confirm pass.
51+
52+
## Target files
53+
54+
- `packages/host/app/lib/gc-card-store.ts`
55+
- `packages/host/app/services/store.ts`
56+
- `packages/host/tests/integration/store-test.gts`
57+
58+
## Testing notes
59+
60+
- `cd packages/host && pnpm lint`
61+
- `cd packages/host && ember test --path dist --filter "file-meta"`

packages/host/app/lib/gc-card-store.ts

Lines changed: 62 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -334,20 +334,26 @@ export default class CardStoreWithGarbageCollection implements CardStore {
334334
sweep(api: typeof CardAPI) {
335335
let dependencyGraph = this.makeDependencyGraph(api);
336336
let reachable = new Set<string>();
337-
let visited = new WeakSet<CardDef>();
337+
let visited = new WeakSet<StoredInstance>();
338338
let rootLocalIds: string[] = [];
339339

340340
for (let instance of this.#cards.values()) {
341-
if (!instance || !isCardInstance(instance)) {
341+
if (!instance || visited.has(instance)) {
342342
continue;
343343
}
344-
if (visited.has(instance)) {
344+
visited.add(instance);
345+
if (isCardInstance(instance)) {
346+
let localId = instance[localIdSymbol];
347+
if (this.hasReferences(localId)) {
348+
rootLocalIds.push(localId);
349+
}
345350
continue;
346351
}
347-
visited.add(instance);
348-
let localId = instance[localIdSymbol];
349-
if (this.hasReferences(localId)) {
350-
rootLocalIds.push(localId);
352+
if (isFileDefInstance(instance)) {
353+
let fileId = instance.id;
354+
if (fileId && this.hasReferences(fileId)) {
355+
reachable.add(fileId);
356+
}
351357
}
352358
}
353359

@@ -367,32 +373,51 @@ export default class CardStoreWithGarbageCollection implements CardStore {
367373
}
368374
}
369375

370-
visited = new WeakSet<CardDef>();
376+
visited = new WeakSet<StoredInstance>();
371377
for (let instance of this.#cards.values()) {
372-
if (!instance || !isCardInstance(instance)) {
378+
if (!instance || visited.has(instance)) {
373379
continue;
374380
}
375-
if (visited.has(instance)) {
381+
visited.add(instance);
382+
if (isCardInstance(instance)) {
383+
let localId = instance[localIdSymbol];
384+
if (!reachable.has(localId)) {
385+
if (this.#gcCandidates.has(localId)) {
386+
destroy(instance);
387+
// brand the instance to make it easier for debugging
388+
(instance as unknown as any)[
389+
Symbol.for('__instance_detached_from_store')
390+
] = true;
391+
this.delete(localId);
392+
if (instance.id) {
393+
this.delete(instance.id);
394+
}
395+
} else {
396+
this.#gcCandidates.add(localId);
397+
}
398+
} else {
399+
this.#gcCandidates.delete(localId);
400+
}
376401
continue;
377402
}
378-
visited.add(instance);
379-
let localId = instance[localIdSymbol];
380-
if (!reachable.has(localId)) {
381-
if (this.#gcCandidates.has(localId)) {
382-
destroy(instance);
383-
// brand the instance to make it easier for debugging
384-
(instance as unknown as any)[
385-
Symbol.for('__instance_detached_from_store')
386-
] = true;
387-
this.delete(localId);
388-
if (instance.id) {
389-
this.delete(instance.id);
403+
if (isFileDefInstance(instance)) {
404+
let fileId = instance.id;
405+
if (!fileId) {
406+
continue;
407+
}
408+
if (!reachable.has(fileId)) {
409+
if (this.#gcCandidates.has(fileId)) {
410+
destroy(instance);
411+
(instance as unknown as any)[
412+
Symbol.for('__instance_detached_from_store')
413+
] = true;
414+
this.delete(fileId);
415+
} else {
416+
this.#gcCandidates.add(fileId);
390417
}
391418
} else {
392-
this.#gcCandidates.add(localId);
419+
this.#gcCandidates.delete(fileId);
393420
}
394-
} else {
395-
this.#gcCandidates.delete(localId);
396421
}
397422
}
398423
}
@@ -569,10 +594,18 @@ export default class CardStoreWithGarbageCollection implements CardStore {
569594
}
570595
}
571596

572-
private hasReferences(localId: string): boolean {
573-
let referenceCount = this.#referenceCount.get(localId) ?? 0;
574-
for (let remoteId of this.#idResolver.getRemoteIds(localId)) {
575-
referenceCount += this.#referenceCount.get(remoteId) ?? 0;
597+
private hasReferences(id: string): boolean {
598+
let idsToCheck = new Set<string>([id]);
599+
let localId = isLocalId(id) ? id : this.#idResolver.getLocalId(id);
600+
if (localId) {
601+
idsToCheck.add(localId);
602+
for (let remoteId of this.#idResolver.getRemoteIds(localId)) {
603+
idsToCheck.add(remoteId);
604+
}
605+
}
606+
let referenceCount = 0;
607+
for (let refId of idsToCheck) {
608+
referenceCount += this.#referenceCount.get(refId) ?? 0;
576609
}
577610
return referenceCount > 0;
578611
}

packages/host/tests/integration/store-test.gts

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -377,6 +377,44 @@ module('Integration | Store', function (hooks) {
377377
);
378378
});
379379

380+
test('file-meta instance is retained when referenced', async function (assert) {
381+
await testRealm.write('hero.png', 'mock hero image');
382+
let fileUrl = `${testRealmURL}hero.png`;
383+
384+
let fileInstance = await storeService.get(fileUrl);
385+
assert.ok(fileInstance, 'file meta instance is loaded');
386+
assert.ok(
387+
(fileInstance as any).constructor?.isFileDef,
388+
'file meta instance is a FileDef',
389+
);
390+
391+
storeService.addReference(fileUrl);
392+
forceGC();
393+
394+
let peekedInstance = cardStore.get(fileUrl) as unknown;
395+
assert.strictEqual(
396+
peekedInstance,
397+
fileInstance,
398+
'file meta instance is retained after GC with reference',
399+
);
400+
});
401+
402+
test('file-meta instance is garbage collected when references drop', async function (assert) {
403+
await testRealm.write('hero.png', 'mock hero image');
404+
let fileUrl = `${testRealmURL}hero.png`;
405+
406+
await storeService.get(fileUrl);
407+
storeService.addReference(fileUrl);
408+
storeService.dropReference(fileUrl);
409+
forceGC();
410+
411+
assert.strictEqual(
412+
cardStore.get(fileUrl),
413+
undefined,
414+
'file meta instance is garbage collected after reference drop',
415+
);
416+
});
417+
380418
test('garbage collects cards that only consume each other', async function (assert) {
381419
let alpha = new PersonDef({ name: 'Alpha' });
382420
let beta = new PersonDef({ name: 'Beta' });

0 commit comments

Comments
 (0)