Archived
docs(sessions): document background catalog refresh and contain offline env window
Finding 6: serialize createOfflineModelRuntime so overlapping calls cannot interleave their PI_OFFLINE save/restore pairs and leave the process offline, and name the process-wide visibility of that window in the docblock. Finding 7: assert the offline construction through the public refresh seam via reloadConfig() — the request path that regressed — instead of reading upstream's private modelNetworkEnabled field. Finding 8.4/8.5: document the background provider-catalog refresh in docs/config.md and docs/config.html (cadence, timeout, single retry, offline opt-out via PI_WEB_OFFLINE / PI_OFFLINE only), and update the changeset to match the behavior after the earlier fixes.
This commit is contained in:
@@ -471,14 +471,16 @@ describe("AuthService", () => {
|
||||
});
|
||||
|
||||
describe("createModelRuntimeForAgentDir", () => {
|
||||
it("disables runtime-owned network refreshes so request paths stay local", async () => {
|
||||
// The stall fix relies on this construction-time flag: reloadConfig(),
|
||||
// login(), and logout() refresh with allowNetwork = modelNetworkEnabled.
|
||||
// Asserting the private field is proportionate here because the flag is
|
||||
// exactly the contract this change depends on.
|
||||
it("keeps runtime-owned refreshes local so request paths cannot stall", async () => {
|
||||
// reloadConfig() is the request-path call site that regressed: it refreshes
|
||||
// with allowNetwork = the construction-time network flag and no abort signal.
|
||||
const agentDir = await tempAgentDir();
|
||||
const runtime = await createModelRuntimeForAgentDir(agentDir);
|
||||
expect(Reflect.get(runtime, "modelNetworkEnabled")).toBe(false);
|
||||
const refresh = vi.spyOn(runtime, "refresh");
|
||||
|
||||
await runtime.reloadConfig();
|
||||
|
||||
expect(refresh).toHaveBeenCalledWith({ allowNetwork: false });
|
||||
});
|
||||
|
||||
it("restores a previously set PI_OFFLINE after runtime creation", async () => {
|
||||
@@ -495,12 +497,36 @@ describe("createModelRuntimeForAgentDir", () => {
|
||||
try {
|
||||
const agentDir = await tempAgentDir();
|
||||
const runtime = await createModelRuntimeForAgentDir(agentDir);
|
||||
expect(Reflect.get(runtime, "modelNetworkEnabled")).toBe(false);
|
||||
const refresh = vi.spyOn(runtime, "refresh");
|
||||
|
||||
await runtime.reloadConfig();
|
||||
|
||||
expect(refresh).toHaveBeenCalledWith({ allowNetwork: false });
|
||||
expect(process.env["PI_OFFLINE"]).toBeUndefined();
|
||||
} finally {
|
||||
if (previous !== undefined) process.env["PI_OFFLINE"] = previous;
|
||||
}
|
||||
});
|
||||
|
||||
it("restores PI_OFFLINE when creations overlap, because the env windows are serialized", async () => {
|
||||
vi.unstubAllEnvs();
|
||||
const previous = process.env["PI_OFFLINE"];
|
||||
delete process.env["PI_OFFLINE"];
|
||||
try {
|
||||
const dirs = await Promise.all([tempAgentDir(), tempAgentDir(), tempAgentDir()]);
|
||||
const runtimes = await Promise.all(dirs.map((dir) => createModelRuntimeForAgentDir(dir)));
|
||||
|
||||
// Interleaved save/restore pairs would leave PI_OFFLINE set process-wide.
|
||||
expect(process.env["PI_OFFLINE"]).toBeUndefined();
|
||||
for (const runtime of runtimes) {
|
||||
const refresh = vi.spyOn(runtime, "refresh");
|
||||
await runtime.reloadConfig();
|
||||
expect(refresh).toHaveBeenCalledWith({ allowNetwork: false });
|
||||
}
|
||||
} finally {
|
||||
if (previous !== undefined) process.env["PI_OFFLINE"] = previous;
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
async function createAuthService(seed: Record<string, Credential> = {}, logger?: AuthServiceLogger) {
|
||||
|
||||
@@ -31,6 +31,14 @@ interface AuthChangeContext {
|
||||
|
||||
const noopLogger: AuthServiceLogger = { error() { /* no-op */ } };
|
||||
|
||||
/**
|
||||
* Serializes the `PI_OFFLINE` windows below, which is what keeps their
|
||||
* save/restore pairs properly nested. Two overlapping calls would otherwise both
|
||||
* capture the forced `"1"` and restore it, leaving the whole process offline
|
||||
* permanently. Owned by this module only; nothing else may mutate it.
|
||||
*/
|
||||
let offlineRuntimeCreations: Promise<unknown> = Promise.resolve();
|
||||
|
||||
/**
|
||||
* Create the shared model runtime with runtime-owned network refreshes disabled.
|
||||
*
|
||||
@@ -42,12 +50,33 @@ const noopLogger: AuthServiceLogger = { error() { /* no-op */ } };
|
||||
* fetch. Forcing `PI_OFFLINE` during construction makes every runtime-driven
|
||||
* refresh local-only; pi-web performs its own bounded catalog refreshes in the
|
||||
* background instead (see modelCatalogRefresher.ts).
|
||||
*
|
||||
* `modelNetworkEnabled` is computed once from the environment inside
|
||||
* `ModelRuntime.create()`, so the env var is the only lever upstream exposes.
|
||||
* Calls are queued so their env windows never overlap.
|
||||
*/
|
||||
async function createOfflineModelRuntime(options: CreateModelRuntimeOptions): Promise<ModelRuntime> {
|
||||
function createOfflineModelRuntime(options: CreateModelRuntimeOptions): Promise<ModelRuntime> {
|
||||
const created = offlineRuntimeCreations.then(() => forceOfflineWhile(() => ModelRuntime.create(options)));
|
||||
// A failed creation must not poison the queue; the caller still sees the rejection.
|
||||
offlineRuntimeCreations = created.then(() => undefined, () => undefined);
|
||||
return created;
|
||||
}
|
||||
|
||||
/**
|
||||
* Force `PI_OFFLINE` for the duration of `create`, then restore what was there.
|
||||
*
|
||||
* `process.env` is process-wide, so this window is a real global side effect:
|
||||
* anything reading `PI_OFFLINE` while `create` awaits observes offline mode,
|
||||
* including upstream's package manager, tools manager, and version check. That
|
||||
* is acceptable because pi-web only builds runtimes during daemon startup and in
|
||||
* tests, and the window is one `ModelRuntime.create()` call — but it is why
|
||||
* callers must stay serialized rather than run concurrently.
|
||||
*/
|
||||
async function forceOfflineWhile<T>(create: () => Promise<T>): Promise<T> {
|
||||
const previous = process.env["PI_OFFLINE"];
|
||||
process.env["PI_OFFLINE"] = "1";
|
||||
try {
|
||||
return await ModelRuntime.create(options);
|
||||
return await create();
|
||||
} finally {
|
||||
if (previous === undefined) delete process.env["PI_OFFLINE"];
|
||||
else process.env["PI_OFFLINE"] = previous;
|
||||
|
||||
Reference in New Issue
Block a user