fix(automation)!: getSuspendedScreen reads the durable store, not just the hot cache (#4515) (#4566)

`AutomationEngine.getSuspendedScreen` was synchronous, so it could only ever
read the in-memory hot cache — it structurally could not consult the
suspended-run store. But `SuspendedRun.screen` IS persisted
(`sys_automation_run.screen_json`) and `resume()` cold-reads it back via
`loadSuspendedRun` on a cache miss. The result, for a durably suspended screen
run after a process restart: `POST …/runs/:runId/resume` worked while
`GET …/runs/:runId/screen` returned 404 "No pending screen for run" — the
refresh-safe re-fetch failing in exactly the situation it exists for (page
refresh, another device). That is the rendering half of ADR-0019's
durable-suspend promise, missing while the resuming half shipped.

BREAKING: `IAutomationService.getSuspendedScreen(runId)` now returns
`Promise<ScreenSpec | null>`. No sync variant remains on the contract; every
consumer is migrated in this change (the runtime automation domain route, the
contract-checked http-dispatcher mock, three engine test call sites).

The engine keeps the hot cache as its fast path and falls through to the store
via the same `loadSuspendedRun` that `resume` rehydrates from — one loader, two
callers, no duplicated rehydration logic. A run that does not exist, is no
longer suspended, or paused at a non-screen node still resolves to `null`, so
the route keeps 404-ing for genuinely absent runs. A store outage reads as
`null` (this backs a 404); `hasSuspendedRun` remains the strict variant that
throws for callers who must tell "gone" from "unknown".

Tests: `suspended-screen-durability.test.ts` pins the hot path, the cold-boot
cache-miss (the bug), the absent-run and no-screen null cases, the store-outage
degradation and the no-store behaviour. `flow-durable-suspend.dogfood.test.ts`
adds the end-to-end assertion over a real `stop()` → cold `bootStack`: the
second kernel re-fetches the persisted screen with its field contract intact,
without consuming the pause. Both fail with the fallback removed.

Co-authored-by: Claude <support@objectstack.ai>

Author: os-zhuang

.changeset/durable-suspended-screen-refetch.md

@@ -0,0 +1,25 @@
+---
+'@objectstack/spec': minor
+'@objectstack/service-automation': minor
+'@objectstack/runtime': minor
+---
+
+**BREAKING**: `IAutomationService.getSuspendedScreen(runId)` is now **async** — it returns `Promise<ScreenSpec | null>` instead of `ScreenSpec | null` (#4515).
+
+FROM → TO for anyone calling or implementing it:
+
+```ts
+// caller
+- const screen = automationService.getSuspendedScreen(runId);
++ const screen = await automationService.getSuspendedScreen(runId);
+
+// implementer
+- getSuspendedScreen(runId: string): ScreenSpec | null
++ async getSuspendedScreen(runId: string): Promise<ScreenSpec | null>
+```
+
+One-line fix: `await` the call (the enclosing function is almost certainly already `async`), and make any test double resolve rather than return (`mockResolvedValue`, not `mockReturnValue`).
+
+Why it had to change: the method could only ever read the engine's in-memory hot cache, because a synchronous signature cannot consult the durable suspended-run store. `SuspendedRun.screen` *is* persisted (`sys_automation_run.screen_json`) and `resume()` cold-reads it back, so after a process restart a still-suspended screen run could be resumed (`POST …/runs/:runId/resume` → 200) while `GET …/runs/:runId/screen` returned 404 “No pending screen for run” — the refresh-safe re-fetch failing in exactly the situation it exists for (page refresh, another device), and the rendering half of ADR-0019's durable-suspend promise missing while the resuming half shipped.
+
+`AutomationEngine.getSuspendedScreen` now takes the hot cache as its fast path and falls through to the store via the same loader `resume()` rehydrates from. A run that does not exist, is no longer suspended, or paused at a non-screen node still resolves to `null`, so `GET …/runs/:runId/screen` keeps returning 404 for genuinely absent runs. No sync variant of the method remains on the contract.

docs/design/screen-flow-runtime.md

@@ -26,7 +26,7 @@ interface ScreenSpec { nodeId: string; title?: string; description?: string; fie
 - **screen executor**: suspend when `waitForInput === true` **or** (`config.fields` non-empty **and** `waitForInput !== false`). When suspending, return `{ success:true, suspend:true, screen: { nodeId, title, description, fields } }` built from `node.config`.
 - **suspend plumbing**: `NodeExecutionResult.screen` → `FlowSuspendSignal.screen` → `SuspendedRun.screen` → paused `AutomationResult.screen`.
 - **resume**: apply `signal.variables` as **bare** variables (`variables.set(name, value)`) in addition to the existing `signal.output` (`${nodeId}.key`). If the continuation suspends at another screen, return that screen (multi-screen wizards).
-- `getSuspendedScreen(runId)` getter so HTTP can re-fetch the current screen.
+- `async getSuspendedScreen(runId)` getter so HTTP can re-fetch the current screen. Durable (#4515): hot cache first, then the `SuspendedRunStore` via the same loader `resume` rehydrates from, so a screen run that survived a restart renders as well as it resumes.
 
 ### HTTP (`runtime/http-dispatcher.ts` `handleAutomation`)
 - **Launch**: existing `POST /api/v1/automation/:name/trigger` — when the run pauses at a screen, the response includes `{ status:'paused', runId, screen }`.

packages/qa/dogfood/test/flow-durable-suspend.dogfood.test.ts

@@ -291,6 +291,42 @@ describe('objectstack verify FLOW: a suspended run survives a real cold boot (#4
     expect(JSON.parse(String(rec.variables_json)).noteId).toBe(noteId);
   });
 
+  it('the cold kernel RE-FETCHES the screen — refresh-safe rendering, not just resuming (#4515)', async () => {
+    // The rendering half of the same promise, and the half that was missing.
+    // `GET …/runs/:runId/screen` exists so a user who refreshes the page — or
+    // picks the flow up on another device — gets the form back. It was backed
+    // by `AutomationEngine.getSuspendedScreen`, which was SYNCHRONOUS and so
+    // structurally could only read the in-memory hot cache: after this cold
+    // boot the run was resumable (the test below) yet its screen 404'd, i.e.
+    // the route failed in exactly the situation it was built for. Fixed by
+    // making the contract method async and falling through to the same
+    // suspended-run store `resume` rehydrates from.
+    const res = await cold!.apiAs(coldToken, 'GET', `/automation/flow_durable_suspend/runs/${runId}/screen`);
+    expect(res.status, `screen re-fetch after cold boot: ${await res.clone().text()}`).toBe(200);
+    const body = (await res.json()) as any;
+    const payload = body.data ?? body;
+    expect(payload.runId).toBe(runId);
+
+    // The screen served by a kernel that never rendered it must be the screen
+    // the flow declared — read back out of `screen_json`, field contract intact
+    // (that contract is what the resume below is validated against, #4477).
+    const screen = payload.screen;
+    expect(screen.nodeId).toBe('ask');
+    expect(screen.fields.map((f: any) => f.name)).toContain('resolution');
+    expect(screen.fields.find((f: any) => f.name === 'resolution').required).toBe(true);
+
+    // Read-only: re-fetching must not consume the pause, or a refresh would
+    // destroy the very run it is trying to display.
+    const again = await cold!.apiAs(coldToken, 'GET', `/automation/flow_durable_suspend/runs/${runId}/screen`);
+    expect(again.status).toBe(200);
+    const row = await cold!.apiAs(coldToken, 'GET', `/data/sys_automation_run/${runId}`);
+    expect(row.status).toBe(200);
+
+    // A genuinely absent run still 404s — durable ≠ credulous.
+    const missing = await cold!.apiAs(coldToken, 'GET', '/automation/flow_durable_suspend/runs/run_nope/screen');
+    expect(missing.status).toBe(404);
+  });
+
   it('the cold kernel RESUMES the run and takes the right branch', async () => {
     // The one assertion #4470 was written for. The second kernel never
     // executed a node of this run: it has to rebuild the continuation from
@@ -313,6 +349,12 @@ describe('objectstack verify FLOW: a suspended run survives a real cold boot (#4
     const history = await cold!.apiAs(coldToken, 'GET', `/data/sys_automation_run/run_${runId}`);
     expect(history.status).toBe(200);
     expect((((await history.json()) as any).record ?? {}).status).toBe('completed');
+
+    // …and with the suspension consumed there is no screen left to render:
+    // the durable fallback answers for runs that are SUSPENDED, not for every
+    // id that was ever suspended (#4515).
+    const screen = await cold!.apiAs(coldToken, 'GET', `/automation/flow_durable_suspend/runs/${runId}/screen`);
+    expect(screen.status).toBe(404);
   });
 
   it('the resumed result is itself durable — a THIRD boot still reads it', async () => {

packages/runtime/src/domains/automation.ts

@@ -370,7 +370,7 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str
         // (refresh-safe re-fetch for the UI flow-runner).
         if (parts[1] === 'runs' && parts[2] && parts[3] === 'screen' && m === 'GET') {
             if (typeof automationService.getSuspendedScreen === 'function') {
-                const screen = automationService.getSuspendedScreen(parts[2]);
+                const screen = await automationService.getSuspendedScreen(parts[2]);
                 if (!screen) return { handled: true, response: deps.error('No pending screen for run', 404) };
                 return { handled: true, response: deps.success({ runId: parts[2], screen }) };
             }

packages/runtime/src/http-dispatcher.test.ts

@@ -188,8 +188,11 @@ describe('HttpDispatcher', () => {
                 listRuns: vi.fn().mockResolvedValue([{ id: 'run_1', status: 'completed' }]),
                 getRun: vi.fn().mockResolvedValue({ id: 'run_1', status: 'completed' }),
                 resume: vi.fn().mockResolvedValue({ success: true, output: {}, durationMs: 7 }),
-                // Sync per IAutomationService — `ScreenSpec | null`, not a promise.
-                getSuspendedScreen: vi.fn().mockReturnValue({ nodeId: 'collect', fields: [] }),
+                // ASYNC per IAutomationService (#4515) — `Promise<ScreenSpec | null>`.
+                // It has to be: a screen re-fetch answers for any genuinely
+                // suspended run, which after a restart means reading the
+                // durable suspended-run store, not just the hot cache.
+                getSuspendedScreen: vi.fn().mockResolvedValue({ nodeId: 'collect', fields: [] }),
                 getActionDescriptors: vi.fn().mockReturnValue([
                     { type: 'decision', name: 'Decision', category: 'logic', paradigms: ['flow'], source: 'builtin' },
                     { type: 'http_request', name: 'HTTP Request', category: 'io', paradigms: ['flow', 'approval'], source: 'builtin' },
@@ -444,7 +447,7 @@ describe('HttpDispatcher', () => {
         });
 
         it('should return 404 when the run is not awaiting a screen', async () => {
-            mockAutomationService.getSuspendedScreen.mockReturnValue(null);
+            mockAutomationService.getSuspendedScreen.mockResolvedValue(null);
             const result = await dispatcher.handleAutomation('flow_a/runs/run_1/screen', 'GET', {}, { request: {} });
             expect(result.handled).toBe(true);
             expect(result.response?.status).toBe(404);

packages/services/service-automation/src/builtin/screen-resume-validation.test.ts

@@ -140,7 +140,7 @@ describe('screen resume validation (#4477)', () => {
         const bad = await engine.resume(runId, { variables: {} });
         expect(bad.success).toBe(false);
         // The screen is still fetchable…
-        expect(engine.getSuspendedScreen(runId)?.nodeId).toBe('ask');
+        expect((await engine.getSuspendedScreen(runId))?.nodeId).toBe('ask');
         // …and the legitimate submission still lands.
         const good = await engine.resume(runId, { variables: { kind: 'normal' } });
         expect(good.success).toBe(true);

packages/services/service-automation/src/builtin/subflow-node.test.ts

@@ -245,7 +245,7 @@ describe('subflow node executor', () => {
     expect(r2.status).toBe('paused');
     expect(r2.runId).toBe(parentRunId); // UI keeps one stable run id
     expect(r2.screen).toEqual(s2); // next wizard screen
-    expect(engine.getSuspendedScreen(parentRunId)).toEqual(s2); // refresh-safe re-fetch
+    expect(await engine.getSuspendedScreen(parentRunId)).toEqual(s2); // refresh-safe re-fetch
 
     const r3 = await engine.resume(parentRunId, { variables: { other: 'x' } });
     expect(r3.success).toBe(true);

packages/services/service-automation/src/engine.test.ts

@@ -581,13 +581,13 @@ describe('AutomationEngine', () => {
             expect(paused.screen!.fields[0]).toMatchObject({ name: 'new_assignee', required: true, type: 'text' });
             expect(captured).toBe('UNSET'); // downstream not run yet
             // Re-fetchable for a refreshed client.
-            expect(engine.getSuspendedScreen(paused.runId!)).toMatchObject({ nodeId: 'collect' });
+            expect(await engine.getSuspendedScreen(paused.runId!)).toMatchObject({ nodeId: 'collect' });
 
             const done = await engine.resume(paused.runId!, { variables: { new_assignee: 'ada@example.com' } });
             expect(done.success).toBe(true);
             expect(done.status).toBeUndefined();
             expect(captured).toBe('ada@example.com'); // bare var set on resume → downstream read it
-            expect(engine.getSuspendedScreen(paused.runId!)).toBeNull();
+            expect(await engine.getSuspendedScreen(paused.runId!)).toBeNull();
         });
 
         it('passes a field-less screen straight through (no pause)', async () => {

packages/services/service-automation/src/engine.ts

@@ -3186,9 +3186,20 @@ export class AutomationEngine implements IAutomationService {
      * The screen a paused run is currently waiting on (screen-flow runtime), or
      * `null` if the run isn't suspended / didn't pause at a screen node. Lets a
      * UI flow-runner re-fetch the form after a refresh.
+     *
+     * Durable (#4515): the hot cache is the fast path, and a miss falls through
+     * to the {@link SuspendedRunStore} via the same {@link loadSuspendedRun}
+     * that {@link resume} rehydrates from — one loader, two callers. Without
+     * that fallback a screen run that survived a restart could be *resumed* but
+     * not *rendered*, which is precisely when a refresh-safe re-fetch matters.
+     *
+     * Best-effort by design: a store outage reads as "no such run" (`null`),
+     * matching the 404 this backs. A caller that must distinguish "gone" from
+     * "unknown" before writing anything wants {@link hasSuspendedRun}, which
+     * throws instead.
      */
-    getSuspendedScreen(runId: string): ScreenSpec | null {
-        return this.suspendedRuns.get(runId)?.screen ?? null;
+    async getSuspendedScreen(runId: string): Promise<ScreenSpec | null> {
+        return (await this.loadSuspendedRun(runId))?.screen ?? null;
     }
 
     // ── DAG Traversal Core ──────────────────────────────────