Skip to content

perf: resolve decofile state/revision synchronously when already resolved #1089

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hugo-ccabral wants to merge 1 commit into
base: main
Choose a base branch
from
+56 −25
Open

perf: resolve decofile state/revision synchronously when already resolved #1089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions engine/decofile/fetcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,26 +225,46 @@ export const fromEndpoint = (endpoint: string): DecofileProvider => {
return r;
},
);

let resolvedProvider: DecofileProvider | null = null;
decofileProviderPromise.then((r) => {
Copy link

@cubic-dev-ai cubic-dev-ai bot Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: resolvedProvider can be repopulated after dispose() if the promise resolves later, leaving a stale disposed provider cached. Guard the .then assignment with a disposed flag (or clear in the dispose promise) so the cached provider cannot be restored after disposal.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At engine/decofile/fetcher.ts, line 230:

<comment>`resolvedProvider` can be repopulated after `dispose()` if the promise resolves later, leaving a stale disposed provider cached. Guard the `.then` assignment with a disposed flag (or clear in the dispose promise) so the cached provider cannot be restored after disposal.</comment>

<file context>
@@ -225,26 +225,46 @@ export const fromEndpoint = (endpoint: string): DecofileProvider => {
       );
+
+  let resolvedProvider: DecofileProvider | null = null;
+  decofileProviderPromise.then((r) => {
+    resolvedProvider = r;
+  });
</file context>
Fix with Cubic

resolvedProvider = r;
});

return {
set(state, revision) {
if (resolvedProvider) return resolvedProvider.set?.(state, revision) ?? Promise.resolve();
return decofileProviderPromise.then((r) => r?.set?.(state, revision));
},
notify() {
if (resolvedProvider) return resolvedProvider.notify?.() ?? Promise.resolve();
return decofileProviderPromise.then((r) =>
r?.notify?.() ?? Promise.resolve()
);
},
state: (options) => decofileProviderPromise.then((r) => r.state(options)),
state: (options) => {
if (resolvedProvider) return resolvedProvider.state(options);
return decofileProviderPromise.then((r) => r.state(options));
},
onChange: (cb) => {
if (resolvedProvider) return resolvedProvider.onChange(cb);
decofileProviderPromise.then((r) => r.onChange(cb));
return {
[Symbol.dispose]: () => {
},
};
},
revision: () => decofileProviderPromise.then((r) => r.revision()),
revision: () => {
if (resolvedProvider) return resolvedProvider.revision();
return decofileProviderPromise.then((r) => r.revision());
},
dispose: () => {
decofileProviderPromise.then((r) => r?.dispose?.());
if (resolvedProvider) {
resolvedProvider.dispose?.();
} else {
decofileProviderPromise.then((r) => r?.dispose?.());
}
resolvedProvider = null;
delete decofileCache[endpoint];
Comment on lines +229 to 268
Copy link

@coderabbitai coderabbitai bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

dispose() before promise resolution resurrects a disposed resolvedProvider.

The .then that populates resolvedProvider (line 230) is registered before dispose() can register its cleanup .then. Microtask ordering therefore guarantees:

  1. resolvedProvider = r fires → live reference is cached.
  2. r?.dispose?.() fires → provider is disposed.

resolvedProvider is left pointing to the disposed provider, so any call to state(), revision(), set(), etc. on the returned object after dispose() will route through the disposed instance.

🐛 Proposed fix — guard the cache-setter against post-dispose execution 
  let resolvedProvider: DecofileProvider | null = null;
+ let disposed = false;
  decofileProviderPromise.then((r) => {
-   resolvedProvider = r;
+   if (!disposed) {
+     resolvedProvider = r;
+   }
  });

  // … inside dispose: …
  dispose: () => {
+   disposed = true;
    if (resolvedProvider) {
      resolvedProvider.dispose?.();
    } else {
      decofileProviderPromise.then((r) => r?.dispose?.());
    }
    resolvedProvider = null;
    delete decofileCache[endpoint];
  },
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@engine/decofile/fetcher.ts` around lines 229 - 268, The promise resolution
currently assigns resolvedProvider unconditionally which can resurrect a
provider after dispose; add a local boolean flag (e.g., isDisposed = false) and
in the decofileProviderPromise.then callback check isDisposed before assigning
resolvedProvider—if isDisposed then call r?.dispose?.() and skip caching; in the
returned dispose() implementation set isDisposed = true, null out
resolvedProvider and perform the existing cleanup so the then-handler will not
cache a disposed provider. Ensure references to
resolvedProvider/state/revision/set/onChange continue to work with this guard.

},
};
Expand Down
55 changes: 33 additions & 22 deletions engine/decofile/fs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,21 +69,27 @@ export const newFsProviderFromPath = (
const onChangeCbs: OnChangeCallback[] = [];
let previousState: unknown = null;

let resolvedState: Decofile | null = null;
let resolvedRevision: string | null = null;

const setResolved = (versioned: VersionedDecofile) => {
resolvedState = versioned.state;
resolvedRevision = versioned.revision;
};

const doUpdateState = async () => {
const state = await readAndDecompressFile(fullPath)
.catch((_e) => null);
if (state === null) {
return;
}
// Only update and notify if the state has actually changed
if (equal(state, previousState)) {
return;
}
previousState = state;
decofile = Promise.resolve({
state,
revision: hashState(state),
});
const versioned = { state, revision: hashState(state) };
decofile = Promise.resolve(versioned);
setResolved(versioned);
for (const cb of onChangeCbs) {
cb();
}
Expand Down Expand Up @@ -114,6 +120,8 @@ export const newFsProviderFromPath = (
};
},
).then((result) => {
setResolved(result);

(async () => {
const watcher = Deno.watchFs(fullPath);
for await (const _event of watcher) {
Expand All @@ -124,27 +132,25 @@ export const newFsProviderFromPath = (
return result;
});

const state = async (options: ReadOptions | undefined) => {
if (options?.forceFresh) {
return readAndDecompressFile(fullPath);
}

return await decofile.then((r) => r.state);
};

return {
state,
state: (options: ReadOptions | undefined) => {
if (options?.forceFresh) {
return readAndDecompressFile(fullPath);
}
if (resolvedState !== null) {
return Promise.resolve(resolvedState);
}
return decofile.then((r) => r.state);
},
notify: async () => {
// Directly check for updates without debounce
// This is important for environments where Deno.watchFs doesn't work (e.g., tempfs)
await doUpdateState();
},
set: (state, rev) => {
set: (state: Decofile, rev?: string) => {
previousState = state;
decofile = Promise.resolve({
state,
revision: rev ?? hashState(state),
});
const revision = rev ?? hashState(state);
const versioned = { state, revision };
decofile = Promise.resolve(versioned);
setResolved(versioned);
for (const cb of onChangeCbs) {
cb();
}
Expand All @@ -158,7 +164,12 @@ export const newFsProviderFromPath = (
},
};
},
revision: () => decofile.then((r) => r.revision),
revision: () => {
if (resolvedRevision !== null) {
return Promise.resolve(resolvedRevision);
}
return decofile.then((r) => r.revision);
},
};
};
export const newFsProvider = (
Expand Down
Loading