diff --git a/backend/__tests__/unit/models/pg/message.activityHint.test.js b/backend/__tests__/unit/models/pg/message.activityHint.test.js index 2e15b00266..ac8694540a 100644 --- a/backend/__tests__/unit/models/pg/message.activityHint.test.js +++ b/backend/__tests__/unit/models/pg/message.activityHint.test.js @@ -63,6 +63,25 @@ describe('Message.findActivityHint', () => { it('a query failure degrades to a zero hint rather than throwing', async () => { // The heartbeat's pod-selection pass must not die because the hint did. pool.query.mockRejectedValueOnce(new Error('connection terminated')); - await expect(Message.findActivityHint(POD, SINCE)).resolves.toEqual({ count: 0, lastAt: null }); + await expect(Message.findActivityHint(POD, SINCE)) + .resolves.toEqual({ count: 0, lastAt: null, unavailable: true }); + }); + + // TASK-099. Degrading is right; degrading INDISTINGUISHABLY is the defect. + // `count: 0` is exactly what a genuinely quiet pod returns, so without a + // discriminator the caller — and, through the heartbeat prompt, every agent — + // reads a Postgres outage as "nothing happened here". + it('a real zero and a failed zero are distinguishable', async () => { + pool.query.mockResolvedValueOnce({ rows: [{ count: '0', last_at: null }] }); + const quiet = await Message.findActivityHint(POD, SINCE); + + pool.query.mockRejectedValueOnce(new Error('connection terminated')); + const broken = await Message.findActivityHint(POD, SINCE); + + expect(quiet.count).toBe(0); + expect(broken.count).toBe(0); + expect(quiet.unavailable).toBeUndefined(); + expect(broken.unavailable).toBe(true); + expect(quiet).not.toEqual(broken); }); }); diff --git a/backend/__tests__/unit/routes/agentsRuntime.contextBudget.test.js b/backend/__tests__/unit/routes/agentsRuntime.contextBudget.test.js new file mode 100644 index 0000000000..2f639fb753 --- /dev/null +++ b/backend/__tests__/unit/routes/agentsRuntime.contextBudget.test.js @@ -0,0 +1,113 @@ +/** + * TASK-099 site 9 — `maxContextTokens: 0` means UNCAPPED to PodContextService, + * so it must not also be a failure value. + * + * These close a gap @sprint-review found by mutation on #1519: the new 400 and + * the new `contextBudget` field shipped with no test, so deleting either left + * the suite fully green. That is exactly the defect the PR is about — a + * fallback nobody can observe — one level up, in the coverage rather than the + * code. Each case below pairs the failure with the success value it collided + * with, the same shape as the service tests. + */ + +jest.mock('jsonwebtoken', () => ({ sign: jest.fn(), verify: jest.fn(), decode: jest.fn() })); + +jest.mock('../../../middleware/agentRuntimeAuth', () => (req, res, next) => { + req.agentUser = { _id: 'bot-1' }; + req.agentInstallations = [{ podId: 'pod-1', status: 'active', agentName: 'a', instanceId: 'i' }]; + req.agentAuthorizedPodIds = ['pod-1']; + next(); +}); +jest.mock('../../../middleware/auth', () => (req, res, next) => next()); +jest.mock('../../../middleware/apiTokenScopes', () => ({ + requireApiTokenScopes: () => (req, res, next) => next(), +})); + +jest.mock('../../../services/agentEventService', () => ({})); +jest.mock('../../../services/agentIdentityService', () => ({ + buildAgentUsername: jest.fn((a) => a), + getOrCreateAgentUser: jest.fn().mockResolvedValue({ _id: 'agent-user-1' }), + ensureAgentInPod: jest.fn().mockResolvedValue(undefined), +})); +jest.mock('../../../services/agentMessageService', () => ({ getRecentMessages: jest.fn() })); +jest.mock('../../../services/agentThreadService', () => ({})); +jest.mock('../../../services/podContextService', () => ({ + getPodContext: jest.fn().mockResolvedValue({ _status: 'success', stats: {} }), +})); +jest.mock('../../../services/globalModelConfigService', () => ({ getConfig: jest.fn() })); +jest.mock('../../../services/socialPolicyService', () => ({})); +jest.mock('../../../integrations', () => ({ get: jest.fn() })); +jest.mock('../../../models/Activity', () => ({})); +jest.mock('../../../models/User', () => ({ findById: jest.fn() })); +jest.mock('../../../models/Post', () => ({ findById: jest.fn() })); +jest.mock('../../../models/Pod', () => ({ find: jest.fn() })); +jest.mock('../../../services/dmService', () => ({ getOrCreateAgentDM: jest.fn() })); +jest.mock('../../../models/Integration', () => ({ find: jest.fn(), findOne: jest.fn() })); +jest.mock('../../../models/AgentRegistry', () => ({ + AgentInstallation: { findOne: jest.fn(), find: jest.fn() }, +})); + +const express = require('express'); +const request = require('supertest'); +const GlobalModelConfigService = require('../../../services/globalModelConfigService'); +const PodContextService = require('../../../services/podContextService'); +const router = require('../../../routes/agentsRuntime'); + +const app = express(); +app.use(express.json()); +app.use('/api/agents/runtime', router); + +const get = (qs = '') => request(app).get(`/api/agents/runtime/pods/pod-1/context${qs}`); +const budgetPassedToService = () => PodContextService.getPodContext.mock.calls[0][0].maxContextTokens; + +describe('GET /pods/:podId/context — the context budget (TASK-099 site 9)', () => { + beforeEach(() => { + jest.clearAllMocks(); + PodContextService.getPodContext.mockResolvedValue({ _status: 'success', stats: {} }); + }); + + it('a malformed maxContextTokens is a 400, not a silent uncap', async () => { + // parseLimit's NaN fallback is 0, and 0 means UNCAPPED downstream — so the + // pre-fix behaviour was to honour a typo by removing the budget entirely. + const res = await get('?maxContextTokens=abc'); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/maxContextTokens/); + expect(PodContextService.getPodContext).not.toHaveBeenCalled(); + }); + + it('a well-formed maxContextTokens is honoured and attributed to the query', async () => { + const res = await get('?maxContextTokens=8000'); + expect(res.status).toBe(200); + expect(budgetPassedToService()).toBe(8000); + expect(res.body.contextBudget).toEqual({ applied: true, source: 'query' }); + }); + + it('a configured contextLimit reserves 25% and says so', async () => { + GlobalModelConfigService.getConfig.mockResolvedValue({ llmService: { contextLimit: 100000 } }); + const res = await get(); + expect(res.status).toBe(200); + expect(budgetPassedToService()).toBe(75000); + expect(res.body.contextBudget).toEqual({ applied: true, source: 'model-config' }); + }); + + it('an UNCONFIGURED contextLimit is uncapped and named as unconfigured', async () => { + GlobalModelConfigService.getConfig.mockResolvedValue({ llmService: {} }); + const res = await get(); + expect(budgetPassedToService()).toBe(0); + expect(res.body.contextBudget).toEqual({ applied: false, source: 'unconfigured' }); + }); + + it('a config-store OUTAGE is uncapped too, and is distinguishable from unconfigured', async () => { + // Both send `maxContextTokens: 0`, which is the whole problem: the response + // field is the only thing that can tell an operator which one happened. + GlobalModelConfigService.getConfig.mockRejectedValue(new Error('mongo down')); + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + + const res = await get(); + expect(budgetPassedToService()).toBe(0); + expect(res.body.contextBudget).toEqual({ applied: false, source: 'config-unavailable' }); + expect(warn).toHaveBeenCalled(); + + warn.mockRestore(); + }); +}); diff --git a/backend/__tests__/unit/services/agentInstallationCleanup.failureCount.test.js b/backend/__tests__/unit/services/agentInstallationCleanup.failureCount.test.js new file mode 100644 index 0000000000..6967f4e7d3 --- /dev/null +++ b/backend/__tests__/unit/services/agentInstallationCleanup.failureCount.test.js @@ -0,0 +1,91 @@ +/** + * TASK-099 site 3 — `{ marked: 0 }` must not mean both "nothing was stale" and + * "every staleness check threw". + * + * The per-item catch logs and continues, so the returned count is a LOWER BOUND + * reported as a total. That is loud at the log and silent at the return value, + * and the return value is what the runner prints and what a caller could gate on. + * + * Closes a gap @sprint-review found by mutation on #1519: stopping the failure + * counter left the suite green, because the existing staleness suite only ever + * destructures `marked`. + */ + +jest.mock('node-cron', () => ({ schedule: jest.fn() })); + +jest.mock('../../../models/AgentRegistry', () => ({ + AgentInstallation: { find: jest.fn(), updateMany: jest.fn(), deleteMany: jest.fn() }, +})); +jest.mock('../../../models/AgentEvent', () => ({ findOne: jest.fn() })); +jest.mock('../../../models/User', () => ({ findOne: jest.fn() })); +jest.mock('../../../models/Pod', () => ({ updateOne: jest.fn() })); +jest.mock('../../../services/agentIdentityService', () => ({ + buildAgentUsername: (name, instanceId) => `${name}__${instanceId}`, +})); + +const { AgentInstallation } = require('../../../models/AgentRegistry'); +const AgentEvent = require('../../../models/AgentEvent'); +const User = require('../../../models/User'); +const { markStaleInstallations } = require('../../../services/agentInstallationCleanupService'); + +const givenActiveInstalls = (n) => { + const rows = Array.from({ length: n }, (_, i) => ({ + _id: `i${i}`, agentName: 'telegram-app', instanceId: `inst-${i}`, podId: 'p1', + })); + AgentInstallation.find.mockReturnValue({ select: () => ({ lean: async () => rows }) }); +}; + +// A live token: unexpired, so the pair is spared and nothing is marked. This is +// the SUCCESS TWIN of the failure case — both return `marked: 0`. +const givenLiveToken = () => { + User.findOne.mockReturnValue({ + select: () => ({ lean: async () => ({ agentRuntimeTokens: [{ expiresAt: new Date(Date.now() + 864e5) }] }) }), + }); +}; + +beforeEach(() => { + jest.clearAllMocks(); + AgentInstallation.updateMany.mockResolvedValue({ modifiedCount: 0 }); + AgentEvent.findOne.mockReturnValue({ select: () => ({ sort: () => ({ lean: async () => null }) }) }); +}); + +describe('markStaleInstallations — a failed sweep differs from a clean one', () => { + it('a clean sweep with nothing stale reports zero failures', async () => { + givenActiveInstalls(2); + givenLiveToken(); + const result = await markStaleInstallations(7); + expect(result).toEqual({ marked: 0, evaluationFailures: 0 }); + }); + + it('a sweep that threw on every pair reports the same marked:0 AND its failures', async () => { + givenActiveInstalls(2); + const err = jest.spyOn(console, 'error').mockImplementation(() => {}); + User.findOne.mockImplementation(() => { throw new Error('mongo down'); }); + + const result = await markStaleInstallations(7); + + // The count alone is indistinguishable from the clean sweep above — which is + // why the second field has to exist. + expect(result.marked).toBe(0); + expect(result.evaluationFailures).toBe(2); + expect(err).toHaveBeenCalledTimes(2); + + err.mockRestore(); + }); + + it('a PARTIAL failure is reported as partial, not rounded to either end', async () => { + givenActiveInstalls(2); + const err = jest.spyOn(console, 'error').mockImplementation(() => {}); + let call = 0; + User.findOne.mockImplementation(() => { + call += 1; + if (call === 1) throw new Error('mongo down'); + return { select: () => ({ lean: async () => ({ agentRuntimeTokens: [{ expiresAt: new Date(Date.now() + 864e5) }] }) }) }; + }); + + const result = await markStaleInstallations(7); + + expect(result.evaluationFailures).toBe(1); + err.mockRestore(); + }); +}); diff --git a/backend/__tests__/unit/services/silentFailure.distinguishable.test.js b/backend/__tests__/unit/services/silentFailure.distinguishable.test.js new file mode 100644 index 0000000000..8c4155e0d0 --- /dev/null +++ b/backend/__tests__/unit/services/silentFailure.distinguishable.test.js @@ -0,0 +1,160 @@ +/** + * TASK-099 — silent-failure sweep. + * + * The row's rule: every fallback that hides an error fails LOUD or fails + * CLOSED with a log, never a template. + * + * The discriminator these tests encode, one level sharper than "does it + * log": a fallback is a SILENT FAILURE iff the value it returns is reachable + * on the SUCCESS path without being the documented fallback. `count: 0`, + * `items: []`, `marked: 0`, `null` and `maxContextTokens: 0` are all ordinary + * success values somewhere in this codebase, so returning one on failure + * makes the failure unobservable by construction — no amount of logging at + * the throw site changes what the CALLER can see. + * + * So each case below pairs the failure with its success TWIN and asserts the + * two differ. A test that only exercised the failure arm would pass against + * the very code these fixes replace. + */ + +const path = require('path'); + +describe('TASK-099 — a failed fallback is distinguishable from its success twin', () => { + describe('skillsCatalogService.loadCatalog', () => { + const CATALOG = '/tmp/task099-catalog.json'; + let fs; + let loadCatalog; + let invalidateCache; + + beforeEach(() => { + jest.resetModules(); + jest.doMock('fs', () => ({ + existsSync: jest.fn(), + statSync: jest.fn(), + readFileSync: jest.fn(), + })); + process.env.SKILLS_CATALOG_PATH = CATALOG; + fs = require('fs'); + ({ loadCatalog, invalidateCache } = require('../../../services/skillsCatalogService')); + invalidateCache(); + }); + + afterEach(() => { + delete process.env.SKILLS_CATALOG_PATH; + jest.dontMock('fs'); + }); + + it('returns an empty catalog when the file is legitimately absent', () => { + fs.existsSync.mockReturnValue(false); + expect(loadCatalog()).toEqual({ source: 'awesome', updatedAt: null, items: [] }); + }); + + it('THROWS on an unreadable file instead of returning that same empty catalog', () => { + fs.existsSync.mockReturnValue(true); + fs.statSync.mockReturnValue({ mtimeMs: 1 }); + fs.readFileSync.mockImplementation(() => { throw new Error('EACCES'); }); + // Previously this returned `{ items: [] }` and GET /api/skills/catalog + // answered 200 "no skills" for a catalog nobody could read. + expect(() => loadCatalog()).toThrow(/unreadable/i); + }); + + it('THROWS on malformed JSON too — a parse failure is not an empty catalog', () => { + fs.existsSync.mockReturnValue(true); + fs.statSync.mockReturnValue({ mtimeMs: 2 }); + fs.readFileSync.mockReturnValue('{ not json'); + expect(() => loadCatalog()).toThrow(/unreadable/i); + }); + }); + + describe('telegramBridgeService.findLiveIntegration', () => { + it('logs when the lookup THROWS, because null also means "no bridge here"', async () => { + jest.resetModules(); + const Integration = { findOne: jest.fn(), findByIdAndUpdate: jest.fn() }; + jest.doMock('../../../models/Integration', () => Integration); + jest.doMock('../../../services/telegramService', () => ({ sendMessage: jest.fn() })); + + const bridge = require('../../../services/telegramBridgeService'); + const errSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + + // Success twin: no live bridge for this pod. Must stay quiet. + Integration.findOne.mockReturnValue({ lean: () => Promise.resolve(null) }); + await bridge.relayAgentMessageToTelegram({ + podId: 'p1', agentUsername: 'a', displayName: 'A', content: '[DECISION] x', + }); + const quietCalls = errSpy.mock.calls.filter((c) => String(c[0]).includes('findLiveIntegration')); + expect(quietCalls).toHaveLength(0); + + // Failure arm: the store threw. Same `null`, but it must be audible. + Integration.findOne.mockReturnValue({ lean: () => Promise.reject(new Error('mongo down')) }); + await bridge.relayAgentMessageToTelegram({ + podId: 'p1', agentUsername: 'a', displayName: 'A', content: '[DECISION] x', + }); + const loudCalls = errSpy.mock.calls.filter((c) => String(c[0]).includes('findLiveIntegration')); + expect(loudCalls).toHaveLength(1); + + errSpy.mockRestore(); + }); + }); + + describe('agentAvatarService.parseDesignDescription', () => { + it('tags the default design so a parse failure is not reported as a clean SVG', () => { + jest.resetModules(); + const AgentAvatarService = require('../../../services/agentAvatarService'); + const svc = AgentAvatarService.default || AgentAvatarService; + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + + const parsed = svc.parseDesignDescription('noise {"style":"abstract","colors":["#111"]} tail'); + expect(parsed.fallbackReason).toBeUndefined(); + expect(warn).not.toHaveBeenCalled(); + + const fell = svc.parseDesignDescription('there is no json here at all'); + expect(fell.fallbackReason).toBe('design-parse-failed'); + expect(warn).toHaveBeenCalled(); + + warn.mockRestore(); + }); + }); + + describe('schedulerService.buildHeartbeatActivityHint', () => { + // This hint is shipped VERBATIM into the heartbeat payload the agent reads, + // so the pg arm's failure value does not stop at the service boundary — it + // becomes a sentence in a prompt. `hasRecentActivity: false` asserted from a + // failed read is the platform lying to every agent in the pod. + const buildHint = ({ pgHint, posts = [] }) => { + jest.resetModules(); + jest.doMock('../../../services/agentEventService', () => ({ enqueue: jest.fn() })); + jest.doMock('../../../models/pg/Message', () => ({ + findActivityHint: jest.fn().mockResolvedValue(pgHint), + })); + jest.doMock('../../../models/Post', () => ({ + aggregate: jest.fn().mockResolvedValue(posts), + })); + const instance = require('../../../services/schedulerService'); + const SchedulerService = instance.constructor; + return SchedulerService.buildHeartbeatActivityHint({ podId: 'p1', now: new Date() }); + }; + + it('a genuinely quiet pod reads false', async () => { + const hint = await buildHint({ pgHint: { count: 0, lastAt: null } }); + expect(hint.hasRecentActivity).toBe(false); + expect(hint.messageCountUnavailable).toBe(false); + }); + + it('an unreadable message store reads null — unknown, not quiet', async () => { + const hint = await buildHint({ pgHint: { count: 0, lastAt: null, unavailable: true } }); + expect(hint.hasRecentActivity).toBeNull(); + expect(hint.messageCountUnavailable).toBe(true); + }); + + it('a positive signal from the OTHER arm still reads true despite the failure', async () => { + // Only a zero is unknowable. Posts answered, so activity is established + // even though the message count is missing. + const hint = await buildHint({ + pgHint: { count: 0, lastAt: null, unavailable: true }, + posts: [{ _id: null, count: 3, lastAt: new Date() }], + }); + expect(hint.hasRecentActivity).toBe(true); + expect(hint.messageCountUnavailable).toBe(true); + }); + }); +}); diff --git a/backend/models/pg/Message.ts b/backend/models/pg/Message.ts index 0778026edc..cce0d065a8 100644 --- a/backend/models/pg/Message.ts +++ b/backend/models/pg/Message.ts @@ -41,6 +41,13 @@ interface FormattedMessage extends MessageRow { interface ActivityHintResult { count: number; lastAt: unknown; + /** + * True when the query FAILED and `count: 0` is therefore not a measurement. + * Without this the caller cannot tell a Postgres outage from a quiet pod — + * they return byte-identical values, and the quiet-pod reading is shipped + * into every agent's heartbeat prompt (schedulerService.buildHeartbeatActivityHint). + */ + unavailable?: boolean; } interface PodActivityEntry { @@ -492,7 +499,7 @@ class Message { } catch (error) { const e = error as { message?: string }; console.error('Error in findActivityHint:', e.message); - return { count: 0, lastAt: null }; + return { count: 0, lastAt: null, unavailable: true }; } } diff --git a/backend/routes/agentsRuntime.ts b/backend/routes/agentsRuntime.ts index 1dfb469e56..fe0f01d50b 100644 --- a/backend/routes/agentsRuntime.ts +++ b/backend/routes/agentsRuntime.ts @@ -1491,16 +1491,42 @@ router.get('/pods/:podId/context', agentRuntimeAuth, async (req: any, res: any) return clamp(parsed, 1, max); }; - // Resolve context token budget from model config + // Resolve context token budget from model config. + // + // `maxContextTokens: 0` means UNCAPPED to PodContextService (its guard is + // `if (maxContextTokens > 0)`), so 0 must never also be a failure value — + // it previously had three producers that all landed there and all meant + // something different: a config-store outage, an unconfigured contextLimit, + // and a malformed `?maxContextTokens=`. Each removed the budget entirely + // and returned the whole context untrimmed, which is fail-OPEN. let maxContextTokens = 0; + let contextBudgetSource: 'query' | 'model-config' | 'unconfigured' | 'config-unavailable' = 'unconfigured'; if (req.query.maxContextTokens) { + // A caller who ASKED for a budget and mistyped it must not silently get + // no budget at all. parseLimit's NaN fallback is 0 = uncapped here. + const requested = Number.parseInt(req.query.maxContextTokens as string, 10); + if (Number.isNaN(requested)) { + return res.status(400).json({ error: 'maxContextTokens must be an integer' }); + } maxContextTokens = parseLimit(req.query.maxContextTokens, 0, 200000); + contextBudgetSource = 'query'; } else { - const modelConfig = await GlobalModelConfigService.getConfig().catch(() => null); - const contextLimit = modelConfig?.llmService?.contextLimit || 0; - // Reserve 25% of model context for system prompt + output - if (contextLimit > 0) { - maxContextTokens = Math.floor(contextLimit * 0.75); + const modelConfig = await GlobalModelConfigService.getConfig().catch((configErr: Error) => { + // Mirrors llmService.generateText, which logs the identical failure of + // the identical call. Silent here meant an outage was indistinguishable + // from "no contextLimit configured". + console.warn('[agents-runtime] Failed to load model config for context budget:', configErr?.message); + return null; + }); + if (!modelConfig) { + contextBudgetSource = 'config-unavailable'; + } else { + const contextLimit = modelConfig?.llmService?.contextLimit || 0; + // Reserve 25% of model context for system prompt + output + if (contextLimit > 0) { + maxContextTokens = Math.floor(contextLimit * 0.75); + contextBudgetSource = 'model-config'; + } } } @@ -1518,7 +1544,13 @@ router.get('/pods/:podId/context', agentRuntimeAuth, async (req: any, res: any) maxContextTokens, }); - return res.json(context); + // PodContextService attaches tokenEstimate/tokenBudget to `stats` only when + // a budget applied, so an absent budget was invisible to the caller AND + // unattributable. Say which of the four states produced it. + return res.json({ + ...(context as Record), + contextBudget: { applied: maxContextTokens > 0, source: contextBudgetSource }, + }); } catch (error: any) { let statusCode = 500; if (error.status) statusCode = error.status; diff --git a/backend/services/agentAvatarService.ts b/backend/services/agentAvatarService.ts index 137dca52aa..719c2153f2 100644 --- a/backend/services/agentAvatarService.ts +++ b/backend/services/agentAvatarService.ts @@ -159,6 +159,9 @@ class AgentAvatarService { source: 'svg', model: null, fallbackUsed: true, + // null when the model's design actually parsed. `fallbackUsed` alone + // could not distinguish these: it is set to true on this path either way. + designFallbackReason: (avatarDesign as { fallbackReason?: string })?.fallbackReason || null, }, }; } catch (error: any) { @@ -306,7 +309,7 @@ class AgentAvatarService { return this.parseDesignDescription(designDescription); } catch (error) { console.error('Error generating avatar design:', error); - return this.getDefaultDesign(style, colorScheme); + return { ...this.getDefaultDesign(style, colorScheme), fallbackReason: 'design-generation-failed' }; } } @@ -411,8 +414,16 @@ Keep it suitable for a clean vector avatar (flat shapes, gradients, and simple d } throw new Error('No JSON found in response'); } catch (error) { - // Fallback to default design - return this.getDefaultDesign('banana', 'vibrant'); + // Fallback to default design. This catch was previously the only fully + // silent one in this file: it never logged, and its throw never reached + // the outer catch at generateAvatar, so a hardcoded banana was returned + // under `metadata.fallbackUsed: true` — the same value a successful SVG + // generation sets. Log it, and TAG the design so the caller can say which. + console.warn( + '[agent-avatar] could not parse design description, using default:', + (error as Error).message, + ); + return { ...this.getDefaultDesign('banana', 'vibrant'), fallbackReason: 'design-parse-failed' }; } } diff --git a/backend/services/agentInstallationCleanupService.ts b/backend/services/agentInstallationCleanupService.ts index ac9b0c5e3f..b70aae6fec 100644 --- a/backend/services/agentInstallationCleanupService.ts +++ b/backend/services/agentInstallationCleanupService.ts @@ -107,13 +107,13 @@ function hasRecentTokenUse( */ export async function markStaleInstallations( daysSinceLastEvent: number = DEFAULT_STALENESS_EVENT_DAYS, -): Promise<{ marked: number }> { +): Promise<{ marked: number; evaluationFailures: number }> { if (!Number.isFinite(daysSinceLastEvent) || daysSinceLastEvent <= 0) { console.warn( '[installation-cleanup] invalid daysSinceLastEvent, skipping mark step (value=%s)', daysSinceLastEvent, ); - return { marked: 0 }; + return { marked: 0, evaluationFailures: 0 }; } const cutoff = new Date(Date.now() - daysSinceLastEvent * 24 * 60 * 60 * 1000); @@ -125,7 +125,7 @@ export async function markStaleInstallations( .lean(); if (!activeInstalls.length) { - return { marked: 0 }; + return { marked: 0, evaluationFailures: 0 }; } // Dedup the (agentName, instanceId) pairs so we only do one User lookup and @@ -141,6 +141,10 @@ export async function markStaleInstallations( } const stalePairs = new Set(); + // A per-item swallow makes `marked` a LOWER BOUND reported as a total: a run + // that failed to evaluate every pair returns `{ marked: 0 }`, byte-identical + // to a clean run with nothing to do. Count the misses so the two differ. + let evaluationFailures = 0; for (const { agentName, instanceId } of uniquePairs.values()) { try { @@ -178,11 +182,12 @@ export async function markStaleInstallations( instanceId, (err as Error).message, ); + evaluationFailures += 1; } } if (!stalePairs.size) { - return { marked: 0 }; + return { marked: 0, evaluationFailures }; } // Build the OR filter once and updateMany — touches all pods of each stale @@ -207,7 +212,7 @@ export async function markStaleInstallations( ); const marked = result?.modifiedCount ?? result?.nModified ?? 0; - return { marked }; + return { marked, evaluationFailures }; } /** @@ -217,13 +222,13 @@ export async function markStaleInstallations( */ export async function pruneStaleInstallations( minStaleAgeDays: number = DEFAULT_PRUNE_AFTER_STALE_DAYS, -): Promise<{ deleted: number }> { +): Promise<{ deleted: number; failures: number }> { if (!Number.isFinite(minStaleAgeDays) || minStaleAgeDays <= 0) { console.warn( '[installation-cleanup] invalid minStaleAgeDays, skipping prune step (value=%s)', minStaleAgeDays, ); - return { deleted: 0 }; + return { deleted: 0, failures: 0 }; } const cutoff = new Date(Date.now() - minStaleAgeDays * 24 * 60 * 60 * 1000); @@ -236,10 +241,13 @@ export async function pruneStaleInstallations( .lean(); if (!prunable.length) { - return { deleted: 0 }; + return { deleted: 0, failures: 0 }; } let deleted = 0; + // Same reason as markStaleInstallations: `deleted: 0` must not mean both + // "nothing to prune" and "every prune threw". + let failures = 0; for (const inst of prunable) { try { // Remove the agent user from the pod's members array so it stops @@ -261,10 +269,11 @@ export async function pruneStaleInstallations( inst.instanceId, (err as Error).message, ); + failures += 1; } } - return { deleted }; + return { deleted, failures }; } export async function runCleanup(): Promise { @@ -291,9 +300,15 @@ export async function runCleanup(): Promise { `[installation-cleanup] running: mark stale after ${eventDays}d inactivity, prune after ${pruneDays}d stale`, ); const markResult = await markStaleInstallations(eventDays); - console.log(`[installation-cleanup] mark done: marked ${markResult.marked} install(s) stale`); + console.log( + `[installation-cleanup] mark done: marked ${markResult.marked} install(s) stale` + + `, ${markResult.evaluationFailures} pair(s) could not be evaluated`, + ); const pruneResult = await pruneStaleInstallations(pruneDays); - console.log(`[installation-cleanup] prune done: deleted ${pruneResult.deleted} stale install(s)`); + console.log( + `[installation-cleanup] prune done: deleted ${pruneResult.deleted} stale install(s)` + + `, ${pruneResult.failures} failed`, + ); } catch (err) { // Swallow so cron keeps running — never crash the host process from a // cleanup failure. Next run will retry. diff --git a/backend/services/schedulerService.ts b/backend/services/schedulerService.ts index 55e6e50348..9f1eda3714 100644 --- a/backend/services/schedulerService.ts +++ b/backend/services/schedulerService.ts @@ -75,7 +75,15 @@ interface HeartbeatActivityHint { messageCount: number; postCount: number; totalSignals: number; - hasRecentActivity: boolean; + /** + * `null` means WE DO NOT KNOW, not "no activity". The chat-message arm reads + * Postgres and returns `count: 0` on failure, which is the same value a quiet + * pod returns; this hint is shipped verbatim into the heartbeat payload the + * agent reads, so a store outage used to tell every agent the pod was quiet. + */ + hasRecentActivity: boolean | null; + /** True when the message-count arm failed, so `messageCount` is not a measurement. */ + messageCountUnavailable: boolean; lastMessageAt: string | null; lastPostAt: string | null; generatedAt: string; @@ -884,7 +892,7 @@ class SchedulerService { const since = new Date(now.getTime() - (lookbackMinutes * 60 * 1000)); const [pgMsgHint, postStats]: [ - { count: number; lastAt?: string | Date }, + { count: number; lastAt?: string | Date; unavailable?: boolean }, Array<{ _id: unknown; count: number; lastAt?: Date }> ] = await Promise.all([ PGMessage.findActivityHint(podId, since), @@ -895,16 +903,24 @@ class SchedulerService { ]); const messageCount = pgMsgHint.count; + const messageCountUnavailable = pgMsgHint.unavailable === true; const postCount = Number(postStats?.[0]?.count || 0); const totalSignals = messageCount + postCount; + // A positive signal is still positive even if the other arm failed; only a + // ZERO is unknowable, because we did not manage to look. + const hasRecentActivity = totalSignals > 0 + ? true + : (messageCountUnavailable ? null : false); + return { lookbackMinutes, since: since.toISOString(), messageCount, postCount, totalSignals, - hasRecentActivity: totalSignals > 0, + hasRecentActivity, + messageCountUnavailable, lastMessageAt: pgMsgHint.lastAt ? new Date(pgMsgHint.lastAt).toISOString() : null, lastPostAt: postStats?.[0]?.lastAt ? new Date(postStats[0].lastAt).toISOString() : null, generatedAt: now.toISOString(), diff --git a/backend/services/skillsCatalogService.ts b/backend/services/skillsCatalogService.ts index 20bd569779..56d9f91efa 100644 --- a/backend/services/skillsCatalogService.ts +++ b/backend/services/skillsCatalogService.ts @@ -97,8 +97,15 @@ export const loadCatalog = (source = DEFAULT_SOURCE): Catalog => { }); return catalog; } catch (error) { - console.warn(`[skills-catalog] Failed to read ${catalogPath}:`, (error as Error).message); - return { source, updatedAt: null, items: [] }; + // FAIL LOUD. `{ items: [] }` is exactly what a legitimately empty catalog + // returns (and what the two guards above return for "no path configured" + // and "file absent"), so swallowing here answered `GET /api/skills/catalog` + // with 200 "no skills" for an unreadable or malformed catalog file. The + // route's existing catch turns this throw into a 500, which is the honest + // answer: we do not know what skills exist. + const message = (error as Error).message; + console.error(`[skills-catalog] Failed to read ${catalogPath}:`, message); + throw new Error(`skills catalog unreadable at ${catalogPath}: ${message}`); } }; @@ -108,8 +115,15 @@ export const loadCatalog = (source = DEFAULT_SOURCE): Catalog => { * "Last updated X minutes ago" indicator on the frontend. */ export const getLastRefreshedAt = (source = DEFAULT_SOURCE): { localRefreshedAt: string | null; upstreamRefreshedAt: string | null } => { - // Force a cache warm-up so we pick up the current on-disk values. - loadCatalog(source); + // Force a cache warm-up so we pick up the current on-disk values. This is a + // best-effort refresh of a display timestamp, not the catalog read itself — + // loadCatalog now throws on an unreadable file and that must surface from the + // CATALOG call, not from here, or a broken file would be reported twice. + try { + loadCatalog(source); + } catch { + // already logged inside loadCatalog; fall through to whatever is cached + } const cached = catalogCache.get(source); if (!cached) { return { localRefreshedAt: null, upstreamRefreshedAt: null }; diff --git a/backend/services/telegramBridgeService.ts b/backend/services/telegramBridgeService.ts index 603e03f7c9..2ad26a776d 100644 --- a/backend/services/telegramBridgeService.ts +++ b/backend/services/telegramBridgeService.ts @@ -112,7 +112,13 @@ const findLiveIntegration = async (podId: unknown): Promise