diff --git a/backend/__tests__/unit/routes/summaries.allPostsFailure.test.js b/backend/__tests__/unit/routes/summaries.allPostsFailure.test.js new file mode 100644 index 000000000..80774bd11 --- /dev/null +++ b/backend/__tests__/unit/routes/summaries.allPostsFailure.test.js @@ -0,0 +1,134 @@ +// TASK-099 site 4 — follow-up to #1501. +// +// #1501 stopped `GET /api/summaries/all-posts` fabricating a filler summary +// and made the handler fail closed with 503. It did so UNCONDITIONALLY, so +// every throw out of `summarizeAllPosts` — a Mongo error, a TypeError in the +// post mapping, anything — reported as a transient outage. A 503 is an +// instruction to retry, and retrying a code defect never succeeds. +// +// These two cases differ only in the error the service throws. Before the +// fix both answered 503; the second is the one that must not. + +jest.mock('../../../middleware/auth', () => (req, res, next) => { + req.user = { id: 'user1' }; + req.userId = 'user1'; + next(); +}); + +const mockSummarizeAllPosts = jest.fn(); +jest.mock('../../../services/summarizerService', () => { + // Mirror the real module: it exports the singleton AND the marker constant + // the route matches on. A suite that mocks only the method leaves the + // constant undefined, which is exactly the regression the route's `||` + // fallback exists to stop — covered below. + class SummaryUnavailableError extends Error { + constructor(message) { + super(message); + this.name = 'SummaryUnavailableError'; + this.code = 'summary_unavailable'; + } + } + return { + summarizeAllPosts: (...args) => mockSummarizeAllPosts(...args), + SUMMARY_UNAVAILABLE: 'summary_unavailable', + SummaryUnavailableError, + constructor: { getRecentSummaries: jest.fn(), garbageCollectForDigest: jest.fn() }, + }; +}); + +jest.mock('../../../services/chatSummarizerService', () => ({ + getMultiplePodSummaries: jest.fn(), + summarizePodMessages: jest.fn(), + constructor: { getRecentChatSummariesByPodType: jest.fn(), getLatestPodSummary: jest.fn() }, +})); +jest.mock('../../../services/schedulerService', () => ({ + getStatus: jest.fn(), + constructor: { triggerSummarizer: jest.fn(), summarizeIntegrationBuffers: jest.fn(), dispatchPodSummaryRequests: jest.fn() }, +})); +jest.mock('../../../services/dailyDigestService', () => ({ + generateUserDailyDigest: jest.fn(), + generateAllDailyDigests: jest.fn().mockResolvedValue([]), +})); +jest.mock('../../../services/agentEventService', () => ({ enqueue: jest.fn() })); +jest.mock('../../../models/AgentRegistry', () => ({ + AgentInstallation: { find: jest.fn().mockReturnValue({ select: jest.fn().mockReturnValue({ lean: jest.fn().mockResolvedValue([]) }) }) }, +})); +jest.mock('../../../services/dmService', () => ({ canViewPod: jest.fn() })); +jest.mock('../../../models/Pod'); +jest.mock('../../../models/User'); +jest.mock('../../../models/Summary'); + +const request = require('supertest'); +const express = require('express'); + +const { SummaryUnavailableError } = require('../../../services/summarizerService'); +const routes = require('../../../routes/summaries'); + +describe('GET /api/summaries/all-posts — 503 means unavailable, not broken', () => { + let app; + let errorSpy; + + beforeEach(() => { + app = express(); + app.use(express.json()); + app.use('/api/summaries', routes); + jest.clearAllMocks(); + errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => errorSpy.mockRestore()); + + it('answers 503 when the LLM is unavailable', async () => { + mockSummarizeAllPosts.mockRejectedValue( + new SummaryUnavailableError('All-posts summary generation failed at the LLM: connect ECONNREFUSED'), + ); + + const res = await request(app).get('/api/summaries/all-posts'); + + expect(res.status).toBe(503); + expect(res.body).toEqual({ error: 'summary_unavailable' }); + }); + + it('answers 503 when the rate-limit cooldown is armed', async () => { + mockSummarizeAllPosts.mockRejectedValue( + new SummaryUnavailableError('All-posts summary generation is cooling down after an LLM rate limit'), + ); + + const res = await request(app).get('/api/summaries/all-posts'); + + expect(res.status).toBe(503); + }); + + it('answers 500 — not 503 — for a code defect inside the summarizer', async () => { + // The measured case: injecting `(undefined).boom()` at the top of + // summarizeAllPosts used to return `503 summary_unavailable`. + mockSummarizeAllPosts.mockRejectedValue( + new TypeError("Cannot read properties of undefined (reading 'boom')"), + ); + + const res = await request(app).get('/api/summaries/all-posts'); + + expect(res.status).toBe(500); + expect(res.body).not.toEqual({ error: 'summary_unavailable' }); + }); + + it('answers 500 for a datastore failure, which is not this endpoint being unavailable', async () => { + // A Post.find() failure is a dependency outage, but it is not the one the + // 503 sentence claims, and it is not something the caller fixes by + // retrying this route. Fail closed loudly rather than mislabel it. + mockSummarizeAllPosts.mockRejectedValue(new Error('MongoNetworkError: connection 3 to db timed out')); + + const res = await request(app).get('/api/summaries/all-posts'); + + expect(res.status).toBe(500); + }); + + it('still returns the summary on the success path', async () => { + mockSummarizeAllPosts.mockResolvedValue({ title: 'Community Overview • 3 recent posts', content: 'x', metadata: {} }); + + const res = await request(app).get('/api/summaries/all-posts'); + + expect(res.status).toBe(200); + expect(res.body.title).toBe('Community Overview • 3 recent posts'); + }); +}); diff --git a/backend/__tests__/unit/services/llmFallbacks.failClosed.test.ts b/backend/__tests__/unit/services/llmFallbacks.failClosed.test.ts index 56f8af375..518d5b7dc 100644 --- a/backend/__tests__/unit/services/llmFallbacks.failClosed.test.ts +++ b/backend/__tests__/unit/services/llmFallbacks.failClosed.test.ts @@ -58,3 +58,53 @@ describe('LLM summaries fail closed', () => { )).rejects.toThrow('Enhanced chat summary generation returned invalid JSON'); }); }); + +// TASK-099 site 4. Failing closed is the first half; saying WHICH failure is +// the second. `GET /api/summaries/all-posts` turns a throw from here into a +// 503, and 503 is an instruction to retry — correct for an LLM outage, +// actively wrong for a defect in this method. The route reads `.code`, so the +// tag has to be applied at the throw site, and only at the two causes the +// route's own comment names. +describe('summarizeAllPosts tags the causes that are genuinely unavailability', () => { + const postsQuery = (result) => { + const query = { + populate: jest.fn(), + sort: jest.fn(), + limit: jest.fn(), + lean: jest.fn(), + }; + query.populate.mockReturnValue(query); + query.sort.mockReturnValue(query); + query.limit.mockReturnValue(query); + if (result instanceof Error) query.lean.mockRejectedValue(result); + else query.lean.mockResolvedValue(result); + return query; + }; + const onePost = [{ content: 'A real post', tags: [], userId: { username: 'lily' } }]; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it("tags an LLM failure with code 'summary_unavailable'", async () => { + Post.find.mockReturnValueOnce(postsQuery(onePost)); + generateText.mockRejectedValueOnce(new Error('connect ECONNREFUSED')); + + const err = await summarizerService.summarizeAllPosts().catch((e) => e); + + expect(err.code).toBe('summary_unavailable'); + // The original message survives inside the wrapper: the outer catch still + // scans it for '429' / 'Resource exhausted' to arm the cooldown, so + // replacing the text rather than interpolating it would disarm that. + expect(err.message).toContain('connect ECONNREFUSED'); + }); + + it('leaves a datastore failure untagged, so the route reports it as a server error', async () => { + Post.find.mockReturnValueOnce(postsQuery(new Error('MongoNetworkError: connection 3 to db timed out'))); + + const err = await summarizerService.summarizeAllPosts().catch((e) => e); + + expect(err.message).toContain('MongoNetworkError'); + expect(err.code).toBeUndefined(); + }); +}); diff --git a/backend/routes/summaries.ts b/backend/routes/summaries.ts index 71354bc35..4ae5ec4d1 100644 --- a/backend/routes/summaries.ts +++ b/backend/routes/summaries.ts @@ -2,6 +2,12 @@ const express = require('express'); // eslint-disable-next-line global-require const summarizerService = require('../services/summarizerService'); +// The `|| ...` is not defensive noise: several suites mock +// `services/summarizerService` wholesale, and a destructure of an absent +// export yields `undefined` — which would then match every error that carries +// no `code` and restore the exact unconditional-503 behaviour this replaces. +// eslint-disable-next-line global-require +const SUMMARY_UNAVAILABLE = require('../services/summarizerService').SUMMARY_UNAVAILABLE || 'summary_unavailable'; // eslint-disable-next-line global-require const Summary = require('../models/Summary'); // eslint-disable-next-line global-require @@ -261,10 +267,23 @@ router.get('/all-posts', summariesReadRateLimit, auth, async (_req: AuthReq, res const allPostsSummary = await summarizerService.summarizeAllPosts(); res.json(allPostsSummary); } catch (error) { - // Fail closed: the summarizer no longer fabricates filler when the LLM is - // unavailable, so this is a service-unavailable, not a server bug. - console.error('All posts summary unavailable:', (error as Error).message); - res.status(503).json({ error: 'summary_unavailable' }); + // Fail closed, and say WHICH failure. #1501 stopped the summarizer + // fabricating filler and made this a 503 — but unconditionally, so a + // defect inside `summarizeAllPosts` (a Mongo error, a TypeError in the + // post mapping) reported as a transient outage and told the caller to + // retry an endpoint that is permanently broken. 503 now means only what + // the sentence above claims: the LLM is unavailable, or the rate-limit + // cooldown is armed. Both are tagged at the throw site with + // `code: 'summary_unavailable'`. Anything else is a server bug. TASK-099 + // site 4, follow-up to #1501. + const code = (error as { code?: string })?.code; + if (code === SUMMARY_UNAVAILABLE) { + console.error('All posts summary unavailable:', (error as Error).message); + res.status(503).json({ error: 'summary_unavailable' }); + return; + } + console.error('Error generating all posts summary:', error); + res.status(500).json({ error: 'Failed to generate all posts summary' }); } }); diff --git a/backend/services/summarizerService.ts b/backend/services/summarizerService.ts index c673b34a3..043d595bb 100644 --- a/backend/services/summarizerService.ts +++ b/backend/services/summarizerService.ts @@ -38,6 +38,32 @@ interface AllPostsSummary { }; } +// TASK-099 site 4. `GET /api/summaries/all-posts` reported EVERY throw out of +// `summarizeAllPosts` as `503 summary_unavailable`, so a code defect in this +// method was indistinguishable from the LLM being down — and 503 tells the +// caller to retry an endpoint that is permanently broken. Only the two causes +// the route's fail-closed comment actually names carry this marker: the +// rate-limit cooldown, and a `generateText` failure. Everything else (a Mongo +// error, a TypeError in the post mapping) stays an ordinary throw and the +// route answers 500. +// +// The route matches on `.code`, not `instanceof`: this module ships through +// the `module.exports = exports.default` CJS-compat shim at the foot of the +// file, and a duplicated module registration would break identity while a +// string property survives it. +export const SUMMARY_UNAVAILABLE = 'summary_unavailable'; + +export class SummaryUnavailableError extends Error { + code: string; + + constructor(message: string, options?: { cause?: unknown }) { + super(message); + this.name = 'SummaryUnavailableError'; + this.code = SUMMARY_UNAVAILABLE; + if (options && 'cause' in options) (this as { cause?: unknown }).cause = options.cause; + } +} + interface AllPostsCache { summary: AllPostsSummary | null; createdAt: number; @@ -399,10 +425,20 @@ Please create an engaging 3-4 sentence community overview that: This is for new visitors to understand what the community is all about. Focus on the content and conversations, not just statistics.`; if (now < cache.cooldownUntil) { - throw new Error('All-posts summary generation is cooling down after an LLM rate limit'); + throw new SummaryUnavailableError('All-posts summary generation is cooling down after an LLM rate limit'); } console.log('Generating all-posts summary with LLM...'); - const summaryText = await generateText(prompt, { temperature: 0.4 }) as string; + let summaryText: string; + try { + summaryText = await generateText(prompt, { temperature: 0.4 }) as string; + } catch (llmError) { + // Message is interpolated, not replaced: the outer catch below still + // scans it for '429'/'Resource exhausted' to arm the cooldown. + throw new SummaryUnavailableError( + `All-posts summary generation failed at the LLM: ${(llmError as Error)?.message}`, + { cause: llmError }, + ); + } console.log( `✓ LLM returned all-posts summary: "${summaryText.substring(0, 100)}..."`, );