Skip to content

fix: leave a record when the process panics, and harden the paths that can panic unattended - #703

Open
Adam-Dalloul wants to merge 4 commits into
xintaofei:mainfrom
Adam-Dalloul:feat/panic-hook-and-idle-hardening
Open

fix: leave a record when the process panics, and harden the paths that can panic unattended#703
Adam-Dalloul wants to merge 4 commits into
xintaofei:mainfrom
Adam-Dalloul:feat/panic-hook-and-idle-hardening

Conversation

@Adam-Dalloul

Copy link
Copy Markdown
Contributor

A codeg 0.30.6 desktop process died on Windows with exception 0xc0000409
(STATUS_STACK_BUFFER_OVERRUN), faulting module codeg.exe, and a matching
Windows Error Reporting BEX64 bucket. It had logged nothing: ~/.codeg/logs/
held a codeg.2026-09-05.log with content, a zero-byte codeg.2026-09-06.log,
no file at all for 09-07 or 09-08, and a codeg.2026-09-09.log that begins at
19:26:25 UTC with "Applying all pending migrations", which is the relaunch a
few seconds after the crash. So the process ran for days, sat idle, and
vanished without a message.

0xc0000409 on Windows is what Rust's abort() raises, and panic = "abort"
is not set here, so the likely shapes are a panic unwinding across an
extern "system" boundary (the tao/wry event loop, a WndProc, a WebView2
callback) or a stack overflow. I have not proven which, and this PR does not
claim to have found the panic site. It makes the next one leave evidence, and
fixes the unattended paths I could show were reachable.

The hook

logging::panic_hook installs a std::panic::set_hook from build_subscriber,
so all five entry points get it (desktop, codeg-server, codeg-mcp, the
--supervise supervisor, the credential helper) before any of them does real
work. It records the payload, the thread name and id, file:line:column, the
app version, and a backtrace from Backtrace::force_capture() rather than
RUST_BACKTRACE, which nobody filing a crash report has set. It then chains to
the previous hook, so the standard thread '...' panicked at ... line still
prints.

Getting that to disk needed more than tracing::error!. The file sink is a
tracing_appender::non_blocking writer, so an event is only pushed onto a
crossbeam channel for a separate worker thread. That writer's
io::Write::flush is a no-op, the sender is lossy by default (try_send,
dropped when the queue is full), and the only real flush is
WorkerGuard::drop, which the hook must not do, because tokio catches panics
in spawned tasks and tearing the log writer down would blind everything after.
A process that aborts before the worker is next scheduled loses the line. So
the hook appends the record to today's rolling file itself, synchronously, as
one JSON line in the same shape the file sink writes, and only then emits the
tracing event for stderr, the ring buffer and the Logs viewer.

Two details worth a review eye. The event uses an explicit target of
codeg_lib::panic, because TARGET_BACKSTOPS pins codeg_lib::logging to
off and a record inheriting the module path would be filtered out before any
sink saw it; there is a test for that. And the daily filename now comes from
one shared budget::daily_file_name, so the budget's resume measurement and
the hook's append cannot drift apart.

Note this catches a panic, including one that later aborts at an FFI boundary,
and a panic tokio swallows in a spawned task. It does not catch a stack
overflow, where Windows raises the same exception code with no panic involved.

The idle-path fixes

strip_bot_mention found @botname in text.to_lowercase() and then sliced
the original text at that offset. Lowercasing is not length preserving, so a
single character before the mention desynchronises the two: U+0130 grows from
two bytes to three, the Kelvin sign U+212A shrinks from three to one. The slice
lands mid character or past the end and panics, and this text comes from any
member of a bound group chat, parsed by the unattended getUpdates long-poll
loop. Of the five cases in the new test, four panicked and the fifth silently
stripped the wrong bytes.

Both chat-channel config caches forced their first refresh with
Instant::now() - Duration::from_secs(TTL + 1). That is a checked_sub plus
expect, not a saturating subtraction, and on Windows an Instant is the QPC
reading measured from system boot, so starting inside the first 31 seconds of a
boot panicked there. codeg ships an autostart plugin, so login start is an
ordinary case. last_refresh is now Option<Instant>.

background_keepalive_max_age built its chrono::Duration with
Duration::seconds, an expect over try_seconds that panics past
i64::MAX / 1000. It is read on the 60-second ACP idle sweep and on every
background-watch tick, so an out-of-range
CODEG_ACP_BACKGROUND_KEEPALIVE_MAX_SECS aborted from a timer. Out of range
now takes the documented default.

LaunchSeq and DispatchSignal in the work-task engine, and
TerminalManager::kill_by_owner_window / kill_all / the PTY read loop's final
removal, used panicking locks. None guards a value that can be left
half-written. The terminal ones matter most: they are called from inside
on_window_event and RunEvent::ExitRequested, on the main thread inside the
platform event loop, where a panic is an abort rather than a failed operation.
They now use the poison-tolerant form already used in office_watch and
background_watch.

Not changed, but worth knowing

spawn_pet_hover_watcher polls outer_position(), outer_size() and
cursor_position() every 80 ms for as long as the pet window is open. Each of
those is a round trip to the main thread, where tauri-runtime-wry's
handle_user_message answers with tx.send(..).unwrap(). If the requesting
task is gone by the time the main thread gets there, that unwrap panics on the
main thread inside the message pump, which is an abort. That is roughly a
million round trips a day with nobody present, and it is the only thing in the
process polling the windowing layer on a timer. Its own doc comment says the
polling exists because macOS does not deliver mouse events to non-key windows,
yet it runs everywhere. I did not touch it because I cannot test the hover
behaviour on each platform, but if the hook comes back pointing at
handle_user_message, that loop is where to look. It may also be relevant to
#393.

Main has not shipped anything in this area: 0.30.6 is still the latest release,
there is no set_hook anywhere in the tree, and I found no existing crash
reporting design to fit into. Happy to reshape any of this if you have one in
mind.

Verification

Rust tests are new unit tests in the existing style: the hook's target,
summary, JSON line, truncation and file append, a real panic driven through
catch_unwind to check payload, location and backtrace capture, the shared
daily filename, and the Telegram mention cases. Extracted pure-logic versions
of the hook's formatting and the mention matcher were compiled and run locally
against stable 1.98; the full suite and clippy are left to CI. No frontend
changes.

A Rust panic currently leaves nothing behind. On Windows the runtime's
abort() raises STATUS_STACK_BUFFER_OVERRUN (0xc0000409), so all a user
has is a Windows Error Reporting BEX64 bucket naming codeg.exe and a
rolling log that simply stops: no message, no location, no backtrace,
nothing that names the code that failed.

logging::panic_hook installs a hook from build_subscriber, so all five
entry points get it (desktop, codeg-server, codeg-mcp, the --supervise
supervisor, the credential helper) and it is live before any of them does
real work. It records the panic payload, the thread name and id,
file:line:column, the app version, and a backtrace taken with
Backtrace::force_capture() rather than left to RUST_BACKTRACE, which
nobody reporting a crash has set. Then it chains to the previous hook, so
the standard "thread '...' panicked at ..." line still prints.

The record is written twice, and the order matters. tracing::error! alone
does not reach disk: the file sink is a tracing_appender::non_blocking
writer, so an event is only pushed onto a crossbeam channel for a
separate worker thread; that writer's io::Write::flush is a no-op, the
sender is lossy by default (try_send, dropped when full), and the only
real flush is WorkerGuard::drop, which the hook must not do because tokio
catches panics in spawned tasks and tearing the log writer down would
blind everything after. So the hook appends the record to today's rolling
file itself, synchronously, as one JSON line in the same shape the file
sink writes, and only then emits the tracing event for stderr, the ring
buffer and the Logs viewer's live tail.

The event carries an explicit target of codeg_lib::panic rather than its
module path, because TARGET_BACKSTOPS pins codeg_lib::logging to off and
a record emitted from codeg_lib::logging::panic_hook would be filtered
out before it reached any sink. A test pins that.

The daily filename now comes from one shared budget::daily_file_name, so
the budget's resume measurement and the hook's append cannot drift from
the appender or from each other.

Nothing in the hook may panic, since a panic inside a panic hook aborts
at once: every fallible step is best effort, both strings are truncated
on a character boundary, and there is no unwrap on the path.
strip_bot_mention located @botName in text.to_lowercase() and then sliced
the original text at that offset. Lowercasing is not length preserving,
so one character before the mention desynchronises the two: U+0130 (I
with dot above) goes from two bytes to three, the Kelvin sign U+212A from
three to one, U+1E9E from three to two. The slice then lands in the
middle of a character or past the end of the string and panics. The
second slice compounded it, adding the original-case length to an offset
found with the lowercased needle.

This is not exotic input. It arrives from any member of a bound group
chat and is parsed by the getUpdates long-poll loop, which runs
unattended. Of the five cases the new test covers, four panicked and the
fifth silently stripped the wrong bytes: "\u{130}@codeg_bot hi" came back
as "\u{130}@hi".

find_bot_mention scans the original string and compares with
eq_ignore_ascii_case, which is the whole comparison a Telegram bot
username needs, and uses str::get, so an end offset that is out of range
or off a character boundary answers None instead of panicking.
telegram_should_process_text_message now shares that matcher, so a
mention it accepts is one that will actually be stripped, and neither
allocates a lowercased copy of every group message.
Both config caches forced their first refresh with
Instant::now() - Duration::from_secs(TTL + 1). That subtraction is not
saturating: Sub<Duration> for Instant is a checked_sub plus expect. On
Windows an Instant is the QPC reading, which is measured from system
boot, so a codeg started inside the first 31 seconds of a boot panicked
right here. codeg ships an autostart plugin, so launching at login is an
ordinary case, and both caches are constructed inside long-lived spawned
tasks with nobody at the keyboard.

last_refresh becomes Option<Instant>, where None means "never refreshed"
and is itself what forces the first refresh. Same behaviour, no
arithmetic to get wrong.
None of these has been observed firing. They are the ones a timer or a
platform event-loop callback reaches with nobody present, where a panic
is either invisible or fatal.

background_keepalive_max_age built its chrono::Duration with
Duration::seconds, which is an expect over try_seconds and panics past
i64::MAX / 1000. It is read on the 60-second ACP idle sweep and on every
background-watch tick, so an out-of-range
CODEG_ACP_BACKGROUND_KEEPALIVE_MAX_SECS aborted the process from a timer.
Out of range now takes the documented default, like any other invalid
value.

LaunchSeq and DispatchSignal in the work-task engine used
.lock().expect(..) on std mutexes the schedule tick reaches. Neither
guards a value that can be left half-written, so poisoning by an
unrelated panic has nothing to protect and only turns that panic into a
permanent failure of every later launch. They now match the
poison-tolerant locks in office_watch and background_watch.

TerminalManager::kill_by_owner_window and kill_all are called from inside
Tauri's on_window_event and RunEvent::ExitRequested, which run on the
main thread inside the platform event loop, where a panic unwinds across
an extern "system" boundary and Rust turns it into an immediate abort.
Same treatment, plus the matching lock at the end of the PTY read loop,
which is the inconsistent sibling of the poison-tolerant scrollback lock
a few lines above it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant