Skip to content

Commit 1a2506d

Browse files
authored
fix(logging): ALS-safe mixin, quieter errors, matrix login creds (#16)
* fix(logging): ALS-safe mixin, quieter errors, matrix login creds ## Fixed - Clone AsyncLocalStorage context in pino mixin so merge strategy does not mutate stored poll/chat context across log lines. - Drop duplicate failure logs from withDurationMs and multi-insight sink; engine logs delivery outcome once and skips dedupe on post failure. - Install Matrix access token and user id after password login so requests are not sent as M_MISSING_TOKEN. ## Changed - Emit service/env/version only on the startup log line; README notes Loki query fields accordingly. * refactor(logging): tighten comments and multi-sink fan-out
1 parent aa60660 commit 1a2506d

9 files changed

Lines changed: 98 additions & 63 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ Agent tools include flow search, rankings, fan-in, rare-destination / port-scan
154154

155155
Logging verbosity, redaction, and the `env` field use fixed defaults and are not configurable via environment variables.
156156

157-
In Kubernetes with Loki or VictoriaLogs, query on stable fields such as `service="kaytoo"`, `component`, `level`, `pollId`, `eventId`, and `msg`.
157+
In Kubernetes with Loki or VictoriaLogs, query on stable fields such as `component`, `level`, `pollId`, `eventId`, and `msg`. Service/env/version are emitted once on the `kaytoo starting` line; rely on pod labels elsewhere.
158158

159159
### Chat adapters
160160

src/chat/adapters/matrix.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,12 +50,14 @@ export async function startMatrixAdapter(opts: {
5050
});
5151

5252
if ('user' in opts.auth) {
53-
await client.loginRequest({
53+
const resp = await client.loginRequest({
5454
type: 'm.login.password',
5555
identifier: loginIdentifier(opts.auth.user),
5656
password: opts.auth.password,
5757
initial_device_display_name: 'kaytoo',
5858
});
59+
client.setAccessToken(resp.access_token);
60+
client.credentials = { userId: resp.user_id };
5961
}
6062

6163
const onTimeline = async (

src/insights/engine.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,13 @@ export async function startInsightEngine(opts: { config: KaytooConfig; insightSi
200200
return formatFindingsFallback(novel);
201201
});
202202

203-
await opts.insightSink.postInsight(text);
203+
try {
204+
await opts.insightSink.postInsight(text);
205+
} catch (e) {
206+
// Notifier already logged the cause; record outcome and skip dedupe so the next poll retries.
207+
log.warn({ findingCount: novel.length, output: config.output, ...logErr(e) }, 'post findings failed');
208+
return;
209+
}
204210
for (const f of novel) dedupe.mark(f.id);
205211
log.info({ findingCount: novel.length, output: config.output }, 'posted findings');
206212
}

src/logging/logger.ts

Lines changed: 16 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,14 @@ import { getLogContext } from './context.js';
66

77
const __dirname = dirname(fileURLToPath(import.meta.url));
88

9+
export function getServiceMetadata(): { service: 'kaytoo'; env: string; version: string } {
10+
return {
11+
service: 'kaytoo',
12+
env: process.env.NODE_ENV ?? 'development',
13+
version: readPackageVersion(),
14+
};
15+
}
16+
917
const defaultRedactPaths = [
1018
'password',
1119
'*.token',
@@ -40,14 +48,13 @@ export function initLogging(opts: LoggingInit): PinoLogger {
4048
const redact = [...defaultRedactPaths, ...opts.redactPaths];
4149
const baseOpts = {
4250
level: opts.level,
43-
base: {
44-
service: 'kaytoo',
45-
env: process.env.NODE_ENV ?? 'development',
46-
version: readPackageVersion(),
47-
},
51+
base: null,
4852
redact: { paths: redact, censor: '[Redacted]' },
4953
mixin() {
50-
return getLogContext() ?? {};
54+
// Pino merges per-call fields into the returned object in place; copy so
55+
// the AsyncLocalStorage store isn't mutated across log calls.
56+
const ctx = getLogContext();
57+
return ctx ? { ...ctx } : {};
5158
},
5259
};
5360
root.current = opts.destination ? pino(baseOpts, opts.destination) : pino(baseOpts);
@@ -102,18 +109,7 @@ export function logErr(e: unknown): { err: { name: string; message: string; stac
102109

103110
export async function withDurationMs<T>(log: PinoLogger, msg: string, fn: () => Promise<T>): Promise<T> {
104111
const t0 = Date.now();
105-
try {
106-
const out = await fn();
107-
log.debug({ durationMs: Date.now() - t0 }, msg);
108-
return out;
109-
} catch (e) {
110-
log.warn(
111-
{
112-
durationMs: Date.now() - t0,
113-
...(e instanceof Error ? { err: e } : logErr(e)),
114-
},
115-
`${msg} failed`,
116-
);
117-
throw e;
118-
}
112+
const out = await fn();
113+
log.debug({ durationMs: Date.now() - t0 }, msg);
114+
return out;
119115
}

src/main.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { getConfig } from './config.js';
33
import { createConsoleInsightSink } from './notify/consoleInsightSink.js';
44
import { createMultiInsightSink, createPlatformInsightSink, type InsightSink } from './notify/insightSink.js';
55
import { startInsightEngine } from './insights/engine.js';
6-
import { getLogger, initLogging } from './logging/logger.js';
6+
import { getLogger, getServiceMetadata, initLogging } from './logging/logger.js';
77
import { startE2eHttpChatServer } from './e2eHttpChatServer.js';
88
import type { ChatRouter as ChatRouterT } from './chat/router.js';
99
import type { Notifier } from './notify/notifier.js';
@@ -43,7 +43,12 @@ const { level, redactPaths } = config.logging;
4343
initLogging({ level, redactPaths });
4444
const log = getLogger({ component: 'main' });
4545
log.info(
46-
{ indexPattern: config.search.indexPattern, pollSeconds: config.behavior.pollIntervalSeconds, output: config.output },
46+
{
47+
...getServiceMetadata(),
48+
indexPattern: config.search.indexPattern,
49+
pollSeconds: config.behavior.pollIntervalSeconds,
50+
output: config.output,
51+
},
4752
'kaytoo starting',
4853
);
4954

@@ -168,7 +173,6 @@ if (config.output === 'console') {
168173
{ platform: 'matrix', notifier: matrixNotifier, channelId: config.matrix?.defaultRoomId },
169174
{ platform: 'mattermost', notifier: mattermostNotifier, channelId: config.mattermost?.channelId },
170175
]),
171-
log: getLogger({ component: 'insights.out' }),
172176
});
173177

174178
const agent = await createAgentRuntime({ config });

src/notify/insightSink.ts

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
import type { Logger } from 'pino';
2-
import { logErr } from '../logging/logger.js';
31
import type { ChatAddress } from '../chat/types.js';
42
import type { Notifier } from './notifier.js';
53

@@ -13,18 +11,17 @@ export function createPlatformInsightSink(notifier: Notifier, address: ChatAddre
1311
};
1412
}
1513

16-
export function createMultiInsightSink(opts: {
17-
sinks: readonly InsightSink[];
18-
log: Logger;
19-
}): InsightSink {
20-
const { sinks, log } = opts;
14+
// Resolves if any sink succeeds; rejects only when every sink fails. Inner
15+
// notifiers own per-sink failure logging.
16+
export function createMultiInsightSink(opts: { sinks: readonly InsightSink[] }): InsightSink {
17+
const { sinks } = opts;
2118
return {
2219
async postInsight(text: string): Promise<void> {
2320
if (sinks.length === 0) return;
2421
const results = await Promise.allSettled(sinks.map((s) => s.postInsight(text)));
25-
for (const [i, r] of results.entries()) {
26-
if (r.status === 'rejected') log.warn({ ...logErr(r.reason), sinkIndex: i }, 'insight sink failed');
27-
}
22+
const reasons = results.flatMap((r) => (r.status === 'rejected' ? [r.reason] : []));
23+
if (reasons.length !== sinks.length) return;
24+
throw reasons.length === 1 ? reasons[0] : new AggregateError(reasons, 'all insight sinks failed');
2825
},
2926
};
3027
}

test/insightSink.test.ts

Lines changed: 18 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import { describe, expect, it, vi } from 'vitest';
2-
import type { Logger } from 'pino';
32
import type { ChatAddress, ChatPost } from '../src/chat/types.js';
43
import type { Notifier } from '../src/notify/notifier.js';
54
import { createMultiInsightSink, createPlatformInsightSink } from '../src/notify/insightSink.js';
@@ -14,18 +13,6 @@ function recordingNotifier(): Notifier & { calls: ChatPost[] } {
1413
};
1514
}
1615

17-
function silentLogger(): Logger & { warns: Array<{ obj: unknown; msg: string }> } {
18-
const warns: Array<{ obj: unknown; msg: string }> = [];
19-
const log = {
20-
warns,
21-
warn: (obj: unknown, msg: string) => warns.push({ obj, msg }),
22-
info: () => {},
23-
error: () => {},
24-
debug: () => {},
25-
} as unknown as Logger & { warns: Array<{ obj: unknown; msg: string }> };
26-
return log;
27-
}
28-
2916
describe('createPlatformInsightSink', () => {
3017
it('delegates to the notifier with a fixed ChatAddress', async () => {
3118
const notifier = recordingNotifier();
@@ -52,30 +39,39 @@ describe('createMultiInsightSink', () => {
5239
it('fans out to every sink', async () => {
5340
const a = { postInsight: vi.fn().mockResolvedValue(undefined) };
5441
const b = { postInsight: vi.fn().mockResolvedValue(undefined) };
55-
const sink = createMultiInsightSink({ sinks: [a, b], log: silentLogger() });
42+
const sink = createMultiInsightSink({ sinks: [a, b] });
5643

5744
await sink.postInsight('msg');
5845

5946
expect(a.postInsight).toHaveBeenCalledWith('msg');
6047
expect(b.postInsight).toHaveBeenCalledWith('msg');
6148
});
6249

63-
it('continues past a failing sink and logs the rejection', async () => {
50+
it('resolves when at least one sink succeeds (inner notifier owns failure logging)', async () => {
6451
const ok = { postInsight: vi.fn().mockResolvedValue(undefined) };
6552
const fail = { postInsight: vi.fn().mockRejectedValue(new Error('matrix down')) };
66-
const log = silentLogger();
67-
const sink = createMultiInsightSink({ sinks: [fail, ok], log });
53+
const sink = createMultiInsightSink({ sinks: [fail, ok] });
6854

6955
await expect(sink.postInsight('msg')).resolves.toBeUndefined();
7056
expect(ok.postInsight).toHaveBeenCalled();
71-
expect(log.warns).toHaveLength(1);
72-
expect(log.warns[0]?.msg).toBe('insight sink failed');
57+
});
58+
59+
it('rethrows the single reason when only one sink is configured and it fails', async () => {
60+
const fail = { postInsight: vi.fn().mockRejectedValue(new Error('matrix down')) };
61+
const sink = createMultiInsightSink({ sinks: [fail] });
62+
await expect(sink.postInsight('msg')).rejects.toThrow('matrix down');
63+
});
64+
65+
it('rejects with AggregateError when every sink fails', async () => {
66+
const a = { postInsight: vi.fn().mockRejectedValue(new Error('a down')) };
67+
const b = { postInsight: vi.fn().mockRejectedValue(new Error('b down')) };
68+
const sink = createMultiInsightSink({ sinks: [a, b] });
69+
70+
await expect(sink.postInsight('msg')).rejects.toBeInstanceOf(AggregateError);
7371
});
7472

7573
it('is a no-op with no sinks', async () => {
76-
const log = silentLogger();
77-
const sink = createMultiInsightSink({ sinks: [], log });
74+
const sink = createMultiInsightSink({ sinks: [] });
7875
await expect(sink.postInsight('msg')).resolves.toBeUndefined();
79-
expect(log.warns).toHaveLength(0);
8076
});
8177
});

test/logging.test.ts

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { tmpdir } from 'node:os';
44
import pino from 'pino';
55
import { describe, expect, it, beforeEach } from 'vitest';
66
import { runWithLogContext } from '../src/logging/context.js';
7-
import { getLogger, initLogging, resetLogging } from '../src/logging/logger.js';
7+
import { getLogger, getServiceMetadata, initLogging, resetLogging } from '../src/logging/logger.js';
88

99
describe('logging', () => {
1010
beforeEach(() => {
@@ -26,16 +26,19 @@ describe('logging', () => {
2626
};
2727
}
2828

29-
it('emits JSON with service and redacts password', () => {
29+
it('emits JSON without process metadata on each line and redacts password', () => {
3030
const { dest, dir, readLines } = makeSyncFileDest();
3131
initLogging({ level: 'info', redactPaths: [], destination: dest });
3232
getLogger({ component: 'test' }).info({ password: 'secret', ok: true }, 'hello');
3333
const lines = readLines();
3434
rmSync(dir, { recursive: true, force: true });
3535
expect(lines.length).toBeGreaterThan(0);
3636
const row = JSON.parse(lines[0]!) as Record<string, unknown>;
37-
expect(row.service).toBe('kaytoo');
38-
expect(row.env).toBe('test');
37+
expect(row.service).toBeUndefined();
38+
expect(row.env).toBeUndefined();
39+
expect(row.version).toBeUndefined();
40+
expect(row.pid).toBeUndefined();
41+
expect(row.hostname).toBeUndefined();
3942
expect(row.component).toBe('test');
4043
expect(row.msg).toBe('hello');
4144
expect(row.password).toBe('[Redacted]');
@@ -55,6 +58,31 @@ describe('logging', () => {
5558
expect(row.component).toBe('insights');
5659
});
5760

61+
it('does not leak per-call fields from one log into later logs in the same context', () => {
62+
const { dest, dir, readLines } = makeSyncFileDest();
63+
initLogging({ level: 'info', redactPaths: [], destination: dest });
64+
runWithLogContext({ pollId: 'pid-leak' }, () => {
65+
const log = getLogger({ component: 'insights' });
66+
log.warn({ degradedKey: 'alerting', degradedMsg: 'boom' }, 'insights degraded');
67+
log.info({ findingCount: 3 }, 'second');
68+
});
69+
const lines = readLines();
70+
rmSync(dir, { recursive: true, force: true });
71+
const rows = lines.map((l) => JSON.parse(l) as Record<string, unknown>);
72+
expect(rows[0]?.degradedKey).toBe('alerting');
73+
expect(rows[1]?.degradedKey).toBeUndefined();
74+
expect(rows[1]?.degradedMsg).toBeUndefined();
75+
expect(rows[1]?.findingCount).toBe(3);
76+
expect(rows[1]?.pollId).toBe('pid-leak');
77+
});
78+
79+
it('exposes static service metadata for one-time startup logging', () => {
80+
const meta = getServiceMetadata();
81+
expect(meta.service).toBe('kaytoo');
82+
expect(meta.env).toBe('test');
83+
expect(typeof meta.version).toBe('string');
84+
});
85+
5886
it('supports custom redact paths', () => {
5987
const { dest, dir, readLines } = makeSyncFileDest();
6088
initLogging({ level: 'info', redactPaths: ['customSecret'], destination: dest });

test/matrixAdapter.test.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ vi.mock('matrix-js-sdk', () => ({
1818
joinRoom: vi.fn().mockResolvedValue({}),
1919
getUserId: vi.fn().mockReturnValue('@bot:hs'),
2020
loginRequest: vi.fn().mockResolvedValue({ access_token: 'srv-tok', user_id: '@bot:hs' }),
21+
setAccessToken: vi.fn(),
22+
credentials: undefined as unknown,
2123
})),
2224
}));
2325

@@ -254,7 +256,7 @@ describe('startMatrixAdapter', () => {
254256
await stop();
255257
});
256258

257-
it('uses matrix-js-sdk login with m.login.password and no token in createClient', async () => {
259+
it('uses matrix-js-sdk login with m.login.password and installs returned credentials', async () => {
258260
const { stop, client } = await startMatrixAdapter({
259261
homeserverUrl: 'https://hs',
260262
auth: { user: 'kaytoo', password: 'pw' },
@@ -273,6 +275,8 @@ describe('startMatrixAdapter', () => {
273275
password: 'pw',
274276
initial_device_display_name: 'kaytoo',
275277
});
278+
expect(client.setAccessToken).toHaveBeenCalledWith('srv-tok');
279+
expect((client as unknown as { credentials: { userId: string } }).credentials).toEqual({ userId: '@bot:hs' });
276280
await stop();
277281
});
278282

@@ -303,6 +307,8 @@ describe('startMatrixAdapter', () => {
303307
joinRoom: vi.fn().mockResolvedValue({}),
304308
getUserId: vi.fn().mockReturnValue('@bot:hs'),
305309
loginRequest: vi.fn().mockRejectedValue(new Error('M_FORBIDDEN: Invalid password')),
310+
setAccessToken: vi.fn(),
311+
credentials: undefined as unknown,
306312
}) as unknown as MatrixClient,
307313
);
308314
await expect(

0 commit comments

Comments
 (0)