fix(github): refreshed stale issue and pr views
Refresh soft-expired issue and PR view cache rows synchronously before returning content, while keeping PR diff rows on stale-first refresh semantics. Add stale fallback warnings when a live refresh fails and cover the cache/protocol behavior in tests. Fixes #2684
This commit is contained in:
@@ -26,6 +26,7 @@
|
||||
- Fixed advisor context handling to automatically maintain its token budget by promoting the advisor model or compacting/restarting advisor context when needed, preventing advice from degrading on long sessions
|
||||
- Fixed `startup.quiet` leaving MCP and LSP startup status events visible during launch ([#2639](https://github.com/can1357/oh-my-pi/issues/2639)).
|
||||
- Registered the `Advisor` group in the `model` settings tab so advisor settings render correctly in the settings panel.
|
||||
- Fixed soft-expired `issue://` and `pr://` reads to refresh live before returning stale state, with an explicit stale warning when the live refresh fails ([#2684](https://github.com/can1357/oh-my-pi/issues/2684)).
|
||||
|
||||
## [15.13.3] - 2026-06-15
|
||||
|
||||
|
||||
@@ -28,7 +28,7 @@ import {
|
||||
parsePositiveDecimalInt,
|
||||
resolveDefaultRepoMemoized,
|
||||
} from "../tools/gh";
|
||||
import { formatFreshnessNote } from "../tools/github-cache";
|
||||
import { type CacheStatus, formatFreshnessNote } from "../tools/github-cache";
|
||||
import * as git from "../utils/git";
|
||||
import type { InternalResource, InternalUrl, ProtocolHandler, ResolveContext } from "./types";
|
||||
|
||||
@@ -355,7 +355,7 @@ interface BuildSingleArgs {
|
||||
scheme: Scheme;
|
||||
parsed: ParsedSingle;
|
||||
rendered: string;
|
||||
status: "miss" | "fresh" | "stale" | "disabled";
|
||||
status: CacheStatus;
|
||||
fetchedAt: number;
|
||||
/** Resolved repo (post short-form expansion) — used for the PR-only diff hint. */
|
||||
repo?: string;
|
||||
@@ -377,11 +377,15 @@ function buildSingleResource({
|
||||
const diffUrl = repoSegment ? `pr://${repoSegment}/${parsed.number}/diff` : `pr://${parsed.number}/diff`;
|
||||
notes.push(`Diff: ${diffUrl}`);
|
||||
}
|
||||
const content =
|
||||
status === "stale"
|
||||
? `> WARNING: Live GitHub refresh failed; this ${scheme} content is cached and may be stale.\n\n${rendered}`
|
||||
: rendered;
|
||||
return {
|
||||
url: url.href,
|
||||
content: rendered,
|
||||
content,
|
||||
contentType: "text/markdown",
|
||||
size: Buffer.byteLength(rendered, "utf-8"),
|
||||
size: Buffer.byteLength(content, "utf-8"),
|
||||
notes,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -8,10 +8,11 @@
|
||||
* helpers swallow open/IO failures and degrade to "no cache" so a corrupt or
|
||||
* unreadable DB never blocks a `gh` call.
|
||||
*
|
||||
* TTL:
|
||||
* Soft TTL → return cached row directly.
|
||||
* Past soft TTL but within hard TTL → return cached row AND schedule a
|
||||
* background refresh (errors logged, never thrown).
|
||||
* Stateful issue/PR rows past soft TTL but within hard TTL → refresh
|
||||
* synchronously, falling back to the cached row if the live fetch fails.
|
||||
* Expensive PR diff rows past soft TTL but within hard TTL → return cached
|
||||
* row AND schedule a background refresh (errors logged, never thrown).
|
||||
* Past hard TTL → treat as miss and fetch fresh.
|
||||
*/
|
||||
|
||||
@@ -449,7 +450,7 @@ export interface CacheLookupOptions<T> {
|
||||
now?: number;
|
||||
}
|
||||
|
||||
export type CacheStatus = "miss" | "fresh" | "stale" | "disabled";
|
||||
export type CacheStatus = "miss" | "fresh" | "refreshed" | "stale" | "disabled";
|
||||
|
||||
export interface CacheLookupResult<T> {
|
||||
rendered: string;
|
||||
@@ -595,7 +596,7 @@ export async function getOrFetchView<T>(options: CacheLookupOptions<T>): Promise
|
||||
status: "fresh",
|
||||
fetchedAt: cached.fetchedAt,
|
||||
};
|
||||
} else {
|
||||
} else if (options.kind === "pr-diff") {
|
||||
scheduleBackgroundRefresh(
|
||||
authKey,
|
||||
options.repo,
|
||||
@@ -611,6 +612,27 @@ export async function getOrFetchView<T>(options: CacheLookupOptions<T>): Promise
|
||||
status: "stale",
|
||||
fetchedAt: cached.fetchedAt,
|
||||
};
|
||||
} else {
|
||||
try {
|
||||
const fresh = await options.fetchFresh();
|
||||
const fetchedAt = Date.now();
|
||||
storeResult(authKey, options.repo, options.kind, options.number, options.includeComments, fresh, fetchedAt);
|
||||
return { ...fresh, status: "refreshed", fetchedAt };
|
||||
} catch (err) {
|
||||
logger.debug("github cache: synchronous refresh failed; returning stale view", {
|
||||
err: String(err),
|
||||
repo: options.repo,
|
||||
kind: options.kind,
|
||||
number: options.number,
|
||||
});
|
||||
return {
|
||||
rendered: cached.rendered,
|
||||
sourceUrl: cached.sourceUrl,
|
||||
payload: cached.payload,
|
||||
status: "stale",
|
||||
fetchedAt: cached.fetchedAt,
|
||||
};
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -624,7 +646,7 @@ export async function getOrFetchView<T>(options: CacheLookupOptions<T>): Promise
|
||||
* Human-friendly freshness note for protocol-handler `notes[]` rendering.
|
||||
*/
|
||||
export function formatFreshnessNote(status: CacheStatus, fetchedAtMs: number, now: number = Date.now()): string {
|
||||
if (status === "miss") return "Fetched live";
|
||||
if (status === "miss" || status === "refreshed") return "Fetched live";
|
||||
if (status === "disabled") return "Cache disabled; fetched live";
|
||||
const ageSec = Math.max(0, Math.round((now - fetchedAtMs) / 1000));
|
||||
const human =
|
||||
@@ -633,6 +655,7 @@ export function formatFreshnessNote(status: CacheStatus, fetchedAtMs: number, no
|
||||
: ageSec < 3600
|
||||
? `${Math.round(ageSec / 60)}m ago`
|
||||
: `${Math.round(ageSec / 3600)}h ago`;
|
||||
if (status === "stale") return `Cached: ${human} (refreshing in background)`;
|
||||
if (status === "stale")
|
||||
return `WARNING: showing cached content from ${human}; live refresh failed or is still running`;
|
||||
return `Cached: ${human}`;
|
||||
}
|
||||
|
||||
@@ -9,6 +9,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test";
|
||||
import * as fs from "node:fs/promises";
|
||||
import * as os from "node:os";
|
||||
import * as path from "node:path";
|
||||
import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
|
||||
import { InternalUrlRouter } from "@oh-my-pi/pi-coding-agent/internal-urls";
|
||||
import { resetForTests as resetCacheForTests } from "@oh-my-pi/pi-coding-agent/tools/github-cache";
|
||||
import * as git from "@oh-my-pi/pi-coding-agent/utils/git";
|
||||
@@ -173,6 +174,27 @@ describe("issue:// protocol handler", () => {
|
||||
expect(spy).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("marks soft-expired issue fallback content as stale when live refresh fails", async () => {
|
||||
const spy = vi.spyOn(git.github, "json").mockResolvedValue(issuePayload(43, "cached body") as never);
|
||||
const settings = Settings.isolated({
|
||||
"github.cache.softTtlSec": 0,
|
||||
"github.cache.hardTtlSec": 86400,
|
||||
});
|
||||
|
||||
const router = InternalUrlRouter.instance();
|
||||
await router.resolve("issue://owner/example/43");
|
||||
await Bun.sleep(1);
|
||||
spy.mockImplementation(async () => {
|
||||
throw new Error("offline");
|
||||
});
|
||||
|
||||
const resource = await router.resolve("issue://owner/example/43", { settings });
|
||||
expect(resource.content.startsWith("> WARNING: Live GitHub refresh failed")).toBe(true);
|
||||
expect(resource.notes?.[0]).toMatch(/^WARNING: showing cached content/);
|
||||
expect(resource.content).toContain("cached body");
|
||||
expect(spy).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it("retries issue://owner/repo/<n> without stateReason when gh does not support it", async () => {
|
||||
const spy = vi.spyOn(git.github, "json").mockImplementation(async (_cwd, args) => {
|
||||
if (requestedJsonFields(args).has("stateReason")) {
|
||||
|
||||
@@ -299,7 +299,7 @@ describe("getOrFetchView (TTL semantics)", () => {
|
||||
expect(fetchFresh).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("returns cached row AND schedules a background refresh past soft TTL", async () => {
|
||||
it("refreshes issue rows synchronously past soft TTL", async () => {
|
||||
const settings = Settings.isolated({
|
||||
"github.cache.softTtlSec": 60,
|
||||
"github.cache.hardTtlSec": 86400,
|
||||
@@ -326,21 +326,94 @@ describe("getOrFetchView (TTL semantics)", () => {
|
||||
fetchFresh,
|
||||
settings,
|
||||
});
|
||||
expect(result.status).toBe("stale");
|
||||
expect(result.rendered).toBe("old");
|
||||
|
||||
// Background refresh runs on a microtask; flush + give the write a tick
|
||||
// to land before asserting.
|
||||
await Promise.resolve();
|
||||
// Wait for the chained .then() (writes) to settle.
|
||||
await new Promise<void>(resolve => setTimeout(resolve, 5));
|
||||
|
||||
expect(result.status).toBe("refreshed");
|
||||
expect(result.rendered).toBe("refreshed");
|
||||
expect(fetchFresh).toHaveBeenCalledTimes(1);
|
||||
|
||||
const updated = getCached<{ refreshed: boolean }>(TEST_REPO, "issue", 50, true);
|
||||
expect(updated?.rendered).toBe("refreshed");
|
||||
expect(updated?.payload.refreshed).toBe(true);
|
||||
});
|
||||
|
||||
it("falls back to stale issue rows when a soft-expired synchronous refresh fails", async () => {
|
||||
const settings = Settings.isolated({
|
||||
"github.cache.softTtlSec": 60,
|
||||
"github.cache.hardTtlSec": 86400,
|
||||
});
|
||||
const fetchFresh = vi.fn(
|
||||
async (): Promise<{
|
||||
rendered: string;
|
||||
sourceUrl: undefined;
|
||||
payload: { number: number; refreshed: boolean };
|
||||
}> => {
|
||||
throw new Error("offline");
|
||||
},
|
||||
);
|
||||
putCached({
|
||||
repo: TEST_REPO,
|
||||
kind: "issue",
|
||||
number: 51,
|
||||
includeComments: true,
|
||||
payload: { number: 51, refreshed: false },
|
||||
rendered: "old",
|
||||
fetchedAt: Date.now() - 5 * 60_000,
|
||||
});
|
||||
|
||||
const result = await getOrFetchView({
|
||||
repo: TEST_REPO,
|
||||
kind: "issue",
|
||||
number: 51,
|
||||
includeComments: true,
|
||||
fetchFresh,
|
||||
settings,
|
||||
});
|
||||
|
||||
expect(result.status).toBe("stale");
|
||||
expect(result.rendered).toBe("old");
|
||||
expect(fetchFresh).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("keeps PR diff rows stale-first past soft TTL", async () => {
|
||||
const settings = Settings.isolated({
|
||||
"github.cache.softTtlSec": 60,
|
||||
"github.cache.hardTtlSec": 86400,
|
||||
});
|
||||
const fetchFresh = vi.fn(async () => ({
|
||||
rendered: "refreshed-diff",
|
||||
sourceUrl: undefined,
|
||||
payload: { files: [], refreshed: true },
|
||||
}));
|
||||
putCached({
|
||||
repo: TEST_REPO,
|
||||
kind: "pr-diff",
|
||||
number: 52,
|
||||
includeComments: false,
|
||||
payload: { files: [], refreshed: false },
|
||||
rendered: "old-diff",
|
||||
fetchedAt: Date.now() - 5 * 60_000,
|
||||
});
|
||||
|
||||
const result = await getOrFetchView({
|
||||
repo: TEST_REPO,
|
||||
kind: "pr-diff",
|
||||
number: 52,
|
||||
includeComments: false,
|
||||
fetchFresh,
|
||||
settings,
|
||||
});
|
||||
|
||||
expect(result.status).toBe("stale");
|
||||
expect(result.rendered).toBe("old-diff");
|
||||
|
||||
await Promise.resolve();
|
||||
await Bun.sleep(5);
|
||||
|
||||
expect(fetchFresh).toHaveBeenCalledTimes(1);
|
||||
const updated = getCached<{ refreshed: boolean }>(TEST_REPO, "pr-diff", 52, false);
|
||||
expect(updated?.rendered).toBe("refreshed-diff");
|
||||
expect(updated?.payload.refreshed).toBe(true);
|
||||
});
|
||||
|
||||
it("treats past-hard-TTL rows as cache misses (fetcher runs)", async () => {
|
||||
const settings = Settings.isolated({
|
||||
"github.cache.softTtlSec": 60,
|
||||
|
||||
Reference in New Issue
Block a user