ReviewOS
Reload

also looking at this

stacks/uplink

fix(app): report a crash instead of vanishing, and cap the log

#55
Merged glennmichael123 wants to merge fix/log-a-crash-and-cap-the-log into main
4 files +252 -5
app/Desktop/launcher.tsmodified+25-5
@@ -11,14 +11,14 @@
1111 * Run from source for development with `bun app/Desktop/launcher.ts`; Craft
1212 * then comes from CRAFT_BIN or PATH (pantry installs it).
1313 */
14import { appendFileSync, existsSync, mkdirSync } from 'node:fs'
14import { existsSync } from 'node:fs'
1515import { dirname, join } from 'node:path'
1616import process from 'node:process'
1717import { format } from 'node:util'
1818import pkg from '../../package.json'
1919import { agentPath } from '../Uplink/agent-path'
2020import { acquireSingleInstance, startDesktopAgent } from '../Uplink/desktop-agent'
21import { LOG_PATH } from '../Uplink/settings'
21import { installCrashHandlers, writeLog } from '../Uplink/log'
2222
2323// Launched from Finder or from its own LaunchAgent, this process gets the bare
2424// launchd PATH, and everything below it inherits that: the engines, their
@@ -28,14 +28,31 @@ import { LOG_PATH } from '../Uplink/settings'
2828process.env.PATH = agentPath()
2929
3030// Launched from Finder or at login there is no terminal, so keep a log where
31// Console.app looks for one.
31// Console.app looks for one. From a terminal the developer keeps stderr.
3232if (!process.stdout.isTTY) {
33 mkdirSync(dirname(LOG_PATH), { recursive: true })
3433 for (const level of ['log', 'warn', 'error'] as const) {
35 console[level] = (...args: unknown[]) => appendFileSync(LOG_PATH, `${new Date().toISOString()} ${format(...args)}\n`)
34 console[level] = (...args: unknown[]) => writeLog(format(...args))
3635 }
3736}
3837
38/**
39 * Whether the things `shutdown` closes over exist yet. A crash during startup
40 * is both the one most worth reporting and the one where calling `shutdown`
41 * would itself throw, so that case exits directly. The single instance lock is
42 * reaped by the next start.
43 */
44let running = false
45
46installCrashHandlers({
47 shutdown: (code) => {
48 if (!running)
49 process.exit(code)
50 // A hung agent.stop() must not keep a half dead process alive.
51 setTimeout(() => process.exit(code), 3000).unref()
52 void shutdown(code)
53 },
54})
55
3956if (!acquireSingleInstance()) {
4057 console.log('[uplink] already running')
4158 process.exit(0)
@@ -84,6 +101,9 @@ void new Response(craft.stderr).text().then((stderr) => {
84101
85102// The point is answering while you are away, so hold off idle sleep.
86103const keepAwake = Bun.spawn(['/usr/bin/caffeinate', '-i', '-w', String(process.pid)], { stdio: ['ignore', 'ignore', 'ignore'] })
104// Everything `shutdown` touches now exists, so a crash from here on can go
105// through it rather than exiting on the spot.
106running = true
87107
88108let stopping = false
89109async function shutdown(code: number): Promise<never> {
app/Uplink/log.tsadded+92-0
@@ -0,0 +1,92 @@
1import { appendFileSync, mkdirSync, renameSync, statSync } from 'node:fs'
2import { dirname } from 'node:path'
3import process from 'node:process'
4import { LOG_MAX_BYTES, LOG_PATH } from './settings'
5
6/**
7 * ~/Library/Logs/Uplink.log, which is the whole support story for a
8 * downloadable app: no crash reporting, no diagnostics bundle, and stdout that
9 * nobody is attached to.
10 *
11 * So this file has two jobs the previous inline version did not do. It stays
12 * within a size, because a watcher polling every two seconds appends forever.
13 * And it survives a crash, because the failure most worth explaining was the
14 * one the file could not describe: an uncaught throw went to a stderr the
15 * LaunchAgent does not capture, and the process vanished with the log's last
16 * line being whatever succeeded before it.
17 */
18
19// Checked on a counter rather than on every append: the watcher writes on a
20// two second poll, and a stat per line would be most of the work.
21const ROTATE_EVERY = 200
22let writes = 0
23
24/**
25 * One timestamped line. Never throws: `~/Library/Logs` can be missing or
26 * unwritable, and a crash reporter that crashes is worse than none.
27 */
28export function writeLog(line: string, path: string = LOG_PATH, maxBytes: number = LOG_MAX_BYTES): void {
29 try {
30 // Every write, not only on the rotation check: a directory that goes away
31 // mid-run should not cost 200 lines before anyone notices.
32 mkdirSync(dirname(path), { recursive: true })
33 if (writes++ % ROTATE_EVERY === 0)
34 rotate(path, maxBytes)
35 appendFileSync(path, `${new Date().toISOString()} ${line}\n`)
36 }
37 catch {
38 // Nothing useful left to do: the place errors are reported is the thing
39 // that failed.
40 }
41}
42
43/**
44 * One rotation, keeping `Uplink.log.1`. Kept rather than deleted because the
45 * lines leading up to a crash are the ones worth reading, and a rotation
46 * triggered by that crash's own output would otherwise throw them away.
47 */
48export function rotate(path: string = LOG_PATH, maxBytes: number = LOG_MAX_BYTES): void {
49 if ((statSync(path, { throwIfNoEntry: false })?.size ?? 0) > maxBytes)
50 renameSync(path, `${path}.1`)
51}
52
53/**
54 * Name, message and stack, and nothing else. A stack can carry a frame
55 * argument in some runtimes, and a text somebody sent is not ours to write to
56 * a file that outlives the run.
57 */
58export function describeFatal(error: unknown): string {
59 return error instanceof Error
60 ? `${error.name}: ${error.message}\n${error.stack ?? '(no stack)'}`
61 : String(error)
62}
63
64/**
65 * Report an uncaught throw or a rejected promise, then stop.
66 *
67 * Stopping is the point. A handler that swallows the error leaves the agent
68 * half alive: the watcher is gone and the loopback server is not, so the
69 * menubar reports a healthy app that answers nothing.
70 */
71export function installCrashHandlers(options: { path?: string, shutdown?: (code: number) => void } = {}): void {
72 const shutdown = options.shutdown ?? ((code: number) => process.exit(code))
73 let fatal = false
74
75 const report = (kind: string, error: unknown): void => {
76 // Running twice turns one crash into two, and the second is never the
77 // interesting one.
78 if (fatal)
79 return
80 fatal = true
81 const described = `[uplink] ${kind}: ${describeFatal(error)}`
82 // Whatever the TTY says. From a terminal the developer also gets it on
83 // stderr; the file gets the same entry either way.
84 writeLog(described, options.path)
85 if (process.stdout.isTTY)
86 console.error(described)
87 shutdown(1)
88 }
89
90 process.on('uncaughtException', error => report('uncaught exception', error))
91 process.on('unhandledRejection', reason => report('unhandled rejection', reason))
92}
app/Uplink/settings.tsmodified+8-0
@@ -17,6 +17,14 @@ export const DATA_DIR = join(homedir(), 'Library', 'Application Support', 'Uplin
1717export const SETTINGS_PATH = join(DATA_DIR, 'settings.json')
1818export const DATABASE_PATH = join(DATA_DIR, 'uplink.sqlite')
1919export const LOG_PATH = join(homedir(), 'Library', 'Logs', 'Uplink.log')
20/**
21 * Rotate past this. The watcher polls every two seconds and appends whatever
22 * it finds, so the file is unbounded on a Mac that is simply left running.
23 * One rotation is kept as `Uplink.log.1`, because the lines leading up to a
24 * crash are the ones worth reading and a rotation triggered by that crash's
25 * own output would otherwise throw them away.
26 */
27export const LOG_MAX_BYTES = 5 * 1024 * 1024
2028
2129const KEYCHAIN_SERVICE = 'com.stacksjs.uplink'
2230// Claude Code's token. Codex keeps its own credentials in CODEX_HOME via
tests/unit/uplink/log.test.tsadded+127-0
@@ -0,0 +1,127 @@
1import { afterEach, describe, expect, it } from 'bun:test'
2import { appendFileSync, existsSync, mkdtempSync, readFileSync, rmSync, statSync } from 'node:fs'
3import { tmpdir } from 'node:os'
4import { join } from 'node:path'
5import process from 'node:process'
6import { describeFatal, rotate, writeLog } from '../../../app/Uplink/log'
7
8const dirs: string[] = []
9function tempLog(): string {
10 const dir = mkdtempSync(join(tmpdir(), 'uplink-log-'))
11 dirs.push(dir)
12 return join(dir, 'Uplink.log')
13}
14afterEach(() => {
15 for (const dir of dirs.splice(0))
16 rmSync(dir, { recursive: true, force: true })
17})
18
19describe('the log file', () => {
20 it('writes a timestamped line', () => {
21 const path = tempLog()
22 writeLog('hello', path)
23 expect(readFileSync(path, 'utf8')).toMatch(/^\d{4}-\d{2}-\d{2}T[\d:.]+Z hello\n$/)
24 })
25
26 it('makes the directory rather than failing on a first run', () => {
27 const path = join(tempLog(), '..', 'nested', 'Uplink.log')
28 writeLog('first', path)
29 expect(readFileSync(path, 'utf8')).toContain('first')
30 })
31
32 /**
33 * The lines leading up to a crash are the ones worth reading, and a rotation
34 * triggered by that crash's own output would otherwise throw them away. So
35 * one generation is kept rather than truncated.
36 */
37 it('rotates once past the limit and keeps the previous contents', () => {
38 const path = tempLog()
39 appendFileSync(path, 'x'.repeat(2048))
40 rotate(path, 1024)
41
42 expect(existsSync(path)).toBe(false)
43 expect(readFileSync(`${path}.1`, 'utf8')).toHaveLength(2048)
44
45 // And it keeps logging afterwards.
46 writeLog('after the rotation', path, 1024)
47 expect(readFileSync(path, 'utf8')).toContain('after the rotation')
48 expect(statSync(path).size).toBeLessThan(1024)
49 })
50
51 it('leaves a file that is under the limit alone', () => {
52 const path = tempLog()
53 appendFileSync(path, 'small')
54 rotate(path, 1024)
55 expect(readFileSync(path, 'utf8')).toBe('small')
56 expect(existsSync(`${path}.1`)).toBe(false)
57 })
58
59 it('replaces an older rotation rather than accumulating them', () => {
60 const path = tempLog()
61 appendFileSync(`${path}.1`, 'the generation before last')
62 appendFileSync(path, 'y'.repeat(2048))
63 rotate(path, 1024)
64 expect(readFileSync(`${path}.1`, 'utf8')).toHaveLength(2048)
65 expect(existsSync(`${path}.2`)).toBe(false)
66 })
67
68 it('does not throw when the log cannot be written', () => {
69 // A path whose parent is a file, so both mkdir and append fail.
70 const blocked = tempLog()
71 appendFileSync(blocked, 'i am a file')
72 expect(() => writeLog('into the void', join(blocked, 'Uplink.log'))).not.toThrow()
73 })
74})
75
76describe('describeFatal', () => {
77 it('gives the name, message and stack', () => {
78 const described = describeFatal(new TypeError('null is not an object'))
79 expect(described).toContain('TypeError: null is not an object')
80 expect(described).toContain('log.test.ts')
81 })
82
83 it('handles something thrown that is not an Error', () => {
84 expect(describeFatal('just a string')).toBe('just a string')
85 expect(describeFatal(undefined)).toBe('undefined')
86 })
87})
88
89/**
90 * The failure this exists for. An uncaught throw used to go to a stderr the
91 * LaunchAgent does not capture, and the process vanished with the log's last
92 * line being whatever succeeded before it.
93 */
94describe('crash handlers', () => {
95 function crash(how: string): { exitCode: number, log: string } {
96 const path = tempLog()
97 const root = join(import.meta.dir, '..', '..', '..')
98 const result = Bun.spawnSync(['bun', '-e', `
99 import { installCrashHandlers } from ${JSON.stringify(join(root, 'app/Uplink/log.ts'))}
100 installCrashHandlers({ path: ${JSON.stringify(path)} })
101 ${how}
102 `], { stderr: 'pipe', stdout: 'pipe', env: { ...process.env } })
103 return { exitCode: result.exitCode, log: existsSync(path) ? readFileSync(path, 'utf8') : '' }
104 }
105
106 it('records an uncaught throw and exits non-zero', () => {
107 const { exitCode, log } = crash(`setTimeout(() => { throw new Error('boom') }, 0)`)
108 expect(exitCode).toBe(1)
109 expect(log).toContain('uncaught exception')
110 expect(log).toContain('Error: boom')
111 // The stack, which is the part worth having.
112 expect(log).toContain('at ')
113 })
114
115 it('records a rejected promise with no catch', () => {
116 const { exitCode, log } = crash(`Promise.reject(new Error('nobody caught me'))`)
117 expect(exitCode).toBe(1)
118 expect(log).toContain('unhandled rejection')
119 expect(log).toContain('nobody caught me')
120 })
121
122 it('reports one crash, not a cascade', () => {
123 const { log } = crash(`setTimeout(() => { throw new Error('first') }, 0); setTimeout(() => { throw new Error('second') }, 1)`)
124 expect(log).toContain('first')
125 expect(log).not.toContain('second')
126 })
127})