Breadway/bread-ecosystem
1
0
Fork 0
Code Issues 9 Pull requests 2 Projects Releases Packages Wiki Activity Actions

feat(bread-utils): breadbug client, panic hook, native crash handler, logs, heartbeat #68

Merged
Breadway merged 6 commits from feature/breadbug-client into main 2026-10-14 18:12:35 +08:00
Conversation 10 Commits 6 Files changed 15 +3683 -1
Breadway commented 2026-10-09 21:30:48 +08:00
Owner
Copy link

Closes BREAD-16, BREAD-17 (bread repo issues; the code lives here because bread-client is in bread-utils). Part of ECO-EP1.

What

One call on the existing client turns on everything an app needs for breadbug:

let client = BreadClient::connect("clip");
let bug = client.enable_bug_reporting(BugOptions::new(env!("CARGO_PKG_VERSION")));
// main loop: bug.tick();   (GTK: bug.attach_glib_main_loop() under the gtk feature)
  • Panic hook (BREAD-16): message, location, pid and a stack of (module, offset, build-id) frames from backtrace(3) mapped through /proc/self/maps, so stripped release binaries are symbolicated server-side from the build-id. The hook's own frames are cut by an install-time probe panic, so stacks start at the panicking function (this matters: otherwise constant hook frames would eat the fingerprint's top-5). Re-entrancy guarded, catch_unwind around everything, send on a helper thread with a 400 ms deadline so a wedged breadd cannot delay exit. The previous hook still runs.
  • Native crashes (BREAD-17): handlers for SIGSEGV/SIGBUS/SIGILL/SIGFPE/SIGABRT. I chose the in-process handler over systemd-coredump because it needs no privileges or system configuration and works in any user session. The request is pre-rendered at install time; the handler uses a fixed stack buffer, socket/connect/write/close and no allocation, runs on an alternate stack (so stack overflows are reportable), then SA_RESETHAND makes the process die exactly as it would have. Frames only: no memory, registers or environment. Libraries loaded after install are omitted from the snapshot.
  • Logs: bounded (512), lossy (never blocks the caller; drops are counted), batched (50 lines / 1 s) with back-off while breadd is down; BugGuard::log and a log::Log backend.
  • Heartbeat: sent only while tick() keeps arriving, so a frozen main loop goes quiet and breadd's watchdog can notice; stopping is announced on drop.
  • Everything is best effort and a no-op without breadd; BugOptions::disabled() turns it all off for sensitive apps. Nothing leaves the machine: this only talks to breadd's local socket (breadd scrubs before storing).
  • has_build_id() tells an app whether its binary carries a GNU build-id (needs -C link-arg=-Wl,--build-id; ECO-67 will enforce this in CI).

Tests

  • 66 unit tests (ELF note parsing incl. malformed input, maps parsing and ip->module/offset for PIE and non-PIE, marker construction, pre-rendered native request is valid JSON even with hostile paths and a full buffer, heartbeat gating and clean stop, log batching/clipping/levels, flood does not block, log backend, disabled/missing-breadd).
  • 2 end-to-end tests that re-run the test binary as a child against a fake breadd: a real Rust panic (exit code 101 preserved, message/location/version, stack passes through the panicking function, hook frames cut) and a real SIGSEGV (process still dies of SIGSEGV, crash marker with signal, stack passes through the faulting function, frames contain only module/offset/build_id). Run 3x: stable.
  • clippy -D warnings clean. Not run: the gtk-feature build (attach_glib_main_loop), and a release-mode run of the crash tests.

Notes

  • glibc only for stack capture (backtrace(3)); other targets compile but report no frames.
  • bread-utils is consumed by tag, so each app picks this up on its next bread-utils bump (ECO-68).
Closes BREAD-16, BREAD-17 (bread repo issues; the code lives here because bread-client is in bread-utils). Part of ECO-EP1. ## What One call on the existing client turns on everything an app needs for breadbug: ```rust let client = BreadClient::connect("clip"); let bug = client.enable_bug_reporting(BugOptions::new(env!("CARGO_PKG_VERSION"))); // main loop: bug.tick(); (GTK: bug.attach_glib_main_loop() under the gtk feature) ``` - **Panic hook** (BREAD-16): message, location, pid and a stack of `(module, offset, build-id)` frames from `backtrace(3)` mapped through `/proc/self/maps`, so stripped release binaries are symbolicated server-side from the build-id. The hook's own frames are cut by an install-time probe panic, so stacks start at the panicking function (this matters: otherwise constant hook frames would eat the fingerprint's top-5). Re-entrancy guarded, `catch_unwind` around everything, send on a helper thread with a 400 ms deadline so a wedged breadd cannot delay exit. The previous hook still runs. - **Native crashes** (BREAD-17): handlers for SIGSEGV/SIGBUS/SIGILL/SIGFPE/SIGABRT. I chose the in-process handler over systemd-coredump because it needs no privileges or system configuration and works in any user session. The request is pre-rendered at install time; the handler uses a fixed stack buffer, `socket/connect/write/close` and no allocation, runs on an alternate stack (so stack overflows are reportable), then `SA_RESETHAND` makes the process die exactly as it would have. Frames only: no memory, registers or environment. Libraries loaded after install are omitted from the snapshot. - **Logs**: bounded (512), lossy (never blocks the caller; drops are counted), batched (50 lines / 1 s) with back-off while breadd is down; `BugGuard::log` and a `log::Log` backend. - **Heartbeat**: sent only while `tick()` keeps arriving, so a frozen main loop goes quiet and breadd's watchdog can notice; `stopping` is announced on drop. - Everything is best effort and a no-op without breadd; `BugOptions::disabled()` turns it all off for sensitive apps. Nothing leaves the machine: this only talks to breadd's local socket (breadd scrubs before storing). - `has_build_id()` tells an app whether its binary carries a GNU build-id (needs `-C link-arg=-Wl,--build-id`; ECO-67 will enforce this in CI). ## Tests - 66 unit tests (ELF note parsing incl. malformed input, maps parsing and ip->module/offset for PIE and non-PIE, marker construction, pre-rendered native request is valid JSON even with hostile paths and a full buffer, heartbeat gating and clean stop, log batching/clipping/levels, flood does not block, `log` backend, disabled/missing-breadd). - 2 end-to-end tests that re-run the test binary as a child against a fake breadd: a real Rust panic (exit code 101 preserved, message/location/version, stack passes through the panicking function, hook frames cut) and a real SIGSEGV (process still dies of SIGSEGV, `crash` marker with `signal`, stack passes through the faulting function, frames contain only module/offset/build_id). Run 3x: stable. - clippy `-D warnings` clean. Not run: the gtk-feature build (`attach_glib_main_loop`), and a release-mode run of the crash tests. ## Notes - glibc only for stack capture (`backtrace(3)`); other targets compile but report no frames. - bread-utils is consumed by tag, so each app picks this up on its next bread-utils bump (ECO-68).
feat(bread-utils): breadbug client, panic hook, native crash handler, logs, heartbeat
All checks were successful
check / check (push) Successful in 2m20s
Details
e44b784375 
New bug module behind the bread-client feature. One call,
BreadClient::enable_bug_reporting, installs:

- a Rust panic hook sending message, location and a stack of
  (module, offset, build-id) frames, with the hook's own frames cut by an
  install-time probe so the stack starts at the panicking function;
- native crash handlers for SIGSEGV/SIGBUS/SIGILL/SIGFPE/SIGABRT that send a
  crash marker from an allocation-free handler on an alternate stack, then
  let the process die as it would have (in-process option for BREAD-17);
- a bounded, lossy, batched log forwarder plus a log::Log backend;
- a heartbeat gated on tick() so a frozen main loop goes quiet (GLib
  attachment under the gtk feature).

Frames carry module, offset and build-id only. Everything is best effort
and a no-op without breadd. Tested with real panics and a real SIGSEGV in a
child process against a fake breadd.

Refs BREAD-16, BREAD-17
feat(bread-utils): let apps that handle private text withhold the panic message
All checks were successful
check / check (push) Successful in 2m56s
Details
b2614ba0e1 
BugOptions::panic_message (default true). With it off the report says
'(message omitted by the app)' instead of the panic text, since a formatted
panic message can contain user data (clipboard manager, search index). The
location and stack are kept. Tested with a real panic in a child process.

Refs BREAD-16
Breadway commented 2026-10-13 20:07:12 +08:00
Author
Owner
Copy link

Independent exact-head review: changes requested for BREAD-16 and BREAD-17

Reviewed b2614ba0e1 against actual main/base b52f0fd6fa, both Breadway/bread-ecosystem. Expected head matches. Isolated detached scratch ~/.worktrees/bread-ecosystem/review-pr68; primary checkout untouched. All PR-list pages and all 10 changed files exhausted, complete actual diff/source/tests read. Actual BREAD-16/17 comments, ECO-66, CLIP-19 and dependencies BREAD-14/15/18/ECO-67/68 read. Authenticated reviews API empty and existing PR comments empty at review start; branch protections empty.

BREAD-16 criterion mapping

  • FAIL automatic on connect / version-bump-only adoption: bread-utils/src/bug/mod.rs:168-229 requires explicit enable_bug_reporting, tick/GLib attachment and logger wiring. Existing bread_client connect unchanged. No tracing layer/mirroring existing log backend. This is a manual opt-in API, not requested automatic adoption.
  • PASS local panic frames/location/version and previous hook path: bug/panic.rs:94-153,155-198; debug and optimized crash fixtures pass (message/location/version and fault-site frames). Previous hook is called at147, not swallowed for real panics; probe intentionally suppressed. Custom previous-hook behavior and panic=abort build not independently exercised.
  • FAIL scrubbed message/pre-send privacy: panic.rs:108-120,176-183, mod.rs:275-285,332-338 and native.rs:151-157 serialize raw text/paths. Compiled exact-crate synthetic probe retains fake email, secret and /home/alice path. Per explicit review instruction scrub before sending as well as before persisting, allowlist-first. Local socket is not pre-send scrubbing; store scrubber is unmerged.
  • FAIL bounded nonblocking lifecycle: mod.rs:304-317 joins log worker; logger keeps a cloned sender at296-300, log_loop:347-383 never reads stop. Independent probe holding guard.logger() then drop(guard) hangs until timeout3s exit124, even absent breadd. send_request:128 blocking connect has no connection deadline, so drop also lacks a bounded transport lifetime under full accept backlog.
  • FAIL drop-oldest/rate-limited forwarding: mod.rs:275-286 drops newest with try_send;347-377 sends consecutive full50 batches without a rate budget, not50/second. BugLogger formats entire record before clipping, so transient allocation is unbounded.
  • PASS local tick gating and opt-out/missing socket fixtures: mod.rs:253-273,386-407 and tests.rs. GTK feature builds, but real blocked GLib-context behavior is UNVERIFIED. GLib timer never removes itself on guard drop (Continue at271).
  • PASS local build-id reading/documentation: mod.rs:27-29,116-122 and elf.rs tests. Released app build-id/symbolication UNVERIFIED, not implied by helper.
  • UNVERIFIED integrated breadd store delivery and induced breadclip pilot panic: bread PR22 and CLIP-19 remain open/unmerged. Fake socket receipt is not store entry or integrated pilot.
  • UNVERIFIED measured/documented overhead: none provided; flood timing assertion is not requested measured overhead.

BREAD-17 criterion mapping

  • PASS documented in-process selection and ordinary unprivileged SIGSEGV fixture; native.rs:1-20 and tests/bug_crashes.rs. Debug/release fixture has usable fault-site module/offset/build-id frames and process dies SIGSEGV. No core-file ingestion/upload or memory/register/environment fields in inspected serializer; frame-only capture payload verified locally, complete assembled report UNVERIFIED.
  • FAIL async-signal-safety: native.rs:176-185 calls glibc backtrace from fatal handler. Prewarming259-262 eliminates only initial loading, not unwinder/loader lock hazards when interruption occurs inside allocator/dlopen/unwinding. No async-signal-safe guarantee or adversarial verification.
  • FAIL prior signal/alt-stack lifecycle and fatal semantics: native.rs:280-287 discards previous actions/alt stack, ignores sigaction/sigaltstack errors, never chains/restores. Independent query after installing over SIG_IGN shows prior preserved=false. Independent native::install then raise(SIGSEGV) prints SURVIVED SIGSEGV, exit0: return at218-220 only refaults synchronous faults. write204 can also deliver SIGPIPE when peer closes, changing fatal signal.
  • FAIL privacy before send: module paths forwarded raw at151-157; ASCII filtering is JSON handling, not PII scrubbing.
  • UNVERIFIED store marker/timestamp and real induced SIGSEGV store entry: native payload has no timestamp (render_pieces:66-74), expected ingest timestamp and marker require unmerged bread PR22. No integrated store evidence.
  • UNVERIFIED graceful unavailable-mechanism behavior across targets; native backtrace is unconditionally referenced, unlike cfg-gated panic capture, and install returns true despite syscall failures.

Repair checklist on this PR

  • mod.rs:168-229 / bread_client connect: satisfy automatic-on-connect adoption, automatic log/tracing mirroring and main-context heartbeat lifecycle, or explicitly reconcile issue requirements with the user before fresh review.
  • mod.rs:275-286,304-317,332-383,126-133: cancellable bounded shutdown independent of logger clones, bounded connect/write lifetime, real drop-oldest and sustained rate budget, bounded formatting. Regressions: retained global logger, full backlog, newest diagnostic preservation and rate cap. Remove GLib source on guard drop.
  • panic.rs:108-120,176-183 / native.rs:151-157 / log producers: integrated allowlist-first scrub before sending, seeded privacy assertions on actual serialized socket bytes, and retain ingest/report scrub passes.
  • native.rs:166-220,264-290: replace unsafe fatal-context unwinding with a justified signal-safe strategy; preserve/chaining prior dispositions and alt stack with checked setup/rollback, explicit same-signal fatal re-delivery for raised signals and no SIGPIPE substitution. Test loader/allocator contention, raised signals, previous SA_SIGINFO handlers, alt-stack restore, setup failure and concurrent crashes.
  • Resolve integrated scrub/store/watchdog prerequisites, then verify real isolated pilot and store entries and document measured overhead. Do not treat unmerged branches or mock socket tests as acceptance.

Independent commands PASS: cargo test -p bread-utils --features bread-client --locked (66 unit +4 integration tests,1 ignored doc); cargo test --release -p bread-utils --features bread-client --locked --test bug_crashes (4); cargo check -p bread-utils --features bread-client,gtk --locked; cargo clippy -p bread-utils --features bread-client --all-targets --locked -- -D warnings. Probe compiled via rustc against exact scratch rlibs, timeout3s. Evidence /tmp/opencode/pr68-{probe.rs,tests.log,release.log,gtk.log,clippy.log}. No full workspace independent test claim.

Exact-head required check / check (push) SUCCESS Actions297/job0, status Successful in2m56s. check.yml covers workspace fmt/clippy/tests including gtk/extract; green CI does not waive reproduced defects. Findings filed ECO-76 (eco issue75), high confidence reproduced shutdown/signal/privacy failures. Move BREAD-16 and BREAD-17 todo for coordinator rework, preserve PR/branch. No approval, merge, tag, release, done transition, deployment or live desktop access. Fresh exact-head independent review required after repair.

## Independent exact-head review: changes requested for BREAD-16 and BREAD-17 Reviewed b2614ba0e1b48f25cd40a09e184b721f512f5408 against actual main/base b52f0fd6faf10c0f595c70dbc3bfb64cb4114fa8, both Breadway/bread-ecosystem. Expected head matches. Isolated detached scratch ~/.worktrees/bread-ecosystem/review-pr68; primary checkout untouched. All PR-list pages and all 10 changed files exhausted, complete actual diff/source/tests read. Actual BREAD-16/17 comments, ECO-66, CLIP-19 and dependencies BREAD-14/15/18/ECO-67/68 read. Authenticated reviews API empty and existing PR comments empty at review start; branch protections empty. ### BREAD-16 criterion mapping - FAIL automatic on connect / version-bump-only adoption: bread-utils/src/bug/mod.rs:168-229 requires explicit enable_bug_reporting, tick/GLib attachment and logger wiring. Existing bread_client connect unchanged. No tracing layer/mirroring existing log backend. This is a manual opt-in API, not requested automatic adoption. - PASS local panic frames/location/version and previous hook path: bug/panic.rs:94-153,155-198; debug and optimized crash fixtures pass (message/location/version and fault-site frames). Previous hook is called at147, not swallowed for real panics; probe intentionally suppressed. Custom previous-hook behavior and panic=abort build not independently exercised. - FAIL scrubbed message/pre-send privacy: panic.rs:108-120,176-183, mod.rs:275-285,332-338 and native.rs:151-157 serialize raw text/paths. Compiled exact-crate synthetic probe retains fake email, secret and /home/alice path. Per explicit review instruction scrub before sending as well as before persisting, allowlist-first. Local socket is not pre-send scrubbing; store scrubber is unmerged. - FAIL bounded nonblocking lifecycle: mod.rs:304-317 joins log worker; logger keeps a cloned sender at296-300, log_loop:347-383 never reads stop. Independent probe holding guard.logger() then drop(guard) hangs until timeout3s exit124, even absent breadd. send_request:128 blocking connect has no connection deadline, so drop also lacks a bounded transport lifetime under full accept backlog. - FAIL drop-oldest/rate-limited forwarding: mod.rs:275-286 drops newest with try_send;347-377 sends consecutive full50 batches without a rate budget, not50/second. BugLogger formats entire record before clipping, so transient allocation is unbounded. - PASS local tick gating and opt-out/missing socket fixtures: mod.rs:253-273,386-407 and tests.rs. GTK feature builds, but real blocked GLib-context behavior is UNVERIFIED. GLib timer never removes itself on guard drop (Continue at271). - PASS local build-id reading/documentation: mod.rs:27-29,116-122 and elf.rs tests. Released app build-id/symbolication UNVERIFIED, not implied by helper. - UNVERIFIED integrated breadd store delivery and induced breadclip pilot panic: bread PR22 and CLIP-19 remain open/unmerged. Fake socket receipt is not store entry or integrated pilot. - UNVERIFIED measured/documented overhead: none provided; flood timing assertion is not requested measured overhead. ### BREAD-17 criterion mapping - PASS documented in-process selection and ordinary unprivileged SIGSEGV fixture; native.rs:1-20 and tests/bug_crashes.rs. Debug/release fixture has usable fault-site module/offset/build-id frames and process dies SIGSEGV. No core-file ingestion/upload or memory/register/environment fields in inspected serializer; frame-only capture payload verified locally, complete assembled report UNVERIFIED. - FAIL async-signal-safety: native.rs:176-185 calls glibc backtrace from fatal handler. Prewarming259-262 eliminates only initial loading, not unwinder/loader lock hazards when interruption occurs inside allocator/dlopen/unwinding. No async-signal-safe guarantee or adversarial verification. - FAIL prior signal/alt-stack lifecycle and fatal semantics: native.rs:280-287 discards previous actions/alt stack, ignores sigaction/sigaltstack errors, never chains/restores. Independent query after installing over SIG_IGN shows prior preserved=false. Independent native::install then raise(SIGSEGV) prints SURVIVED SIGSEGV, exit0: return at218-220 only refaults synchronous faults. write204 can also deliver SIGPIPE when peer closes, changing fatal signal. - FAIL privacy before send: module paths forwarded raw at151-157; ASCII filtering is JSON handling, not PII scrubbing. - UNVERIFIED store marker/timestamp and real induced SIGSEGV store entry: native payload has no timestamp (render_pieces:66-74), expected ingest timestamp and marker require unmerged bread PR22. No integrated store evidence. - UNVERIFIED graceful unavailable-mechanism behavior across targets; native backtrace is unconditionally referenced, unlike cfg-gated panic capture, and install returns true despite syscall failures. ### Repair checklist on this PR - [ ] mod.rs:168-229 / bread_client connect: satisfy automatic-on-connect adoption, automatic log/tracing mirroring and main-context heartbeat lifecycle, or explicitly reconcile issue requirements with the user before fresh review. - [ ] mod.rs:275-286,304-317,332-383,126-133: cancellable bounded shutdown independent of logger clones, bounded connect/write lifetime, real drop-oldest and sustained rate budget, bounded formatting. Regressions: retained global logger, full backlog, newest diagnostic preservation and rate cap. Remove GLib source on guard drop. - [ ] panic.rs:108-120,176-183 / native.rs:151-157 / log producers: integrated allowlist-first scrub before sending, seeded privacy assertions on actual serialized socket bytes, and retain ingest/report scrub passes. - [ ] native.rs:166-220,264-290: replace unsafe fatal-context unwinding with a justified signal-safe strategy; preserve/chaining prior dispositions and alt stack with checked setup/rollback, explicit same-signal fatal re-delivery for raised signals and no SIGPIPE substitution. Test loader/allocator contention, raised signals, previous SA_SIGINFO handlers, alt-stack restore, setup failure and concurrent crashes. - [ ] Resolve integrated scrub/store/watchdog prerequisites, then verify real isolated pilot and store entries and document measured overhead. Do not treat unmerged branches or mock socket tests as acceptance. Independent commands PASS: cargo test -p bread-utils --features bread-client --locked (66 unit +4 integration tests,1 ignored doc); cargo test --release -p bread-utils --features bread-client --locked --test bug_crashes (4); cargo check -p bread-utils --features bread-client,gtk --locked; cargo clippy -p bread-utils --features bread-client --all-targets --locked -- -D warnings. Probe compiled via rustc against exact scratch rlibs, timeout3s. Evidence /tmp/opencode/pr68-{probe.rs,tests.log,release.log,gtk.log,clippy.log}. No full workspace independent test claim. Exact-head required check / check (push) SUCCESS Actions297/job0, status Successful in2m56s. check.yml covers workspace fmt/clippy/tests including gtk/extract; green CI does not waive reproduced defects. Findings filed ECO-76 (eco issue75), high confidence reproduced shutdown/signal/privacy failures. Move BREAD-16 and BREAD-17 todo for coordinator rework, preserve PR/branch. No approval, merge, tag, release, done transition, deployment or live desktop access. Fresh exact-head independent review required after repair.
Breadway commented 2026-10-13 20:07:40 +08:00
Author
Owner
Copy link

Independent verdict: changes_requested at exact head b2614ba0e1, base b52f0fd6fa. Full criterion mapping, commands and file/line repair checklist persisted: #68 (comment). Findings ECO-76. Reproduced shutdown hang with retained logger (timeout3s exit124), raised SIGSEGV survives(exit0), prior disposition overwritten, raw fake PII preserved. Native glibc backtrace is not justified async-signal-safe; no prior handler/alt-stack restoration. Logs drop-newest without sustained rate limit; adoption remains manual, not automatic connect. Panic previous-hook call retained and debug/release crash fixtures pass, but do not satisfy integrated acceptance. Independent66 unit+4 integration tests,4 optimized crash fixtures,GTK check and targeted clippy pass; exact-head check/check(push) SUCCESS Actions297. Actual store PR22, scrubber and pilot CLIP-19 unmerged; store-entry/report acceptance and overhead unverified. BREAD-16/17 to todo for coordinator repair on existing PR68; preserve branch. No merge/tag/release/done. Fresh exact-head review after repair.

Independent verdict: changes_requested at exact head b2614ba0e1b48f25cd40a09e184b721f512f5408, base b52f0fd6faf10c0f595c70dbc3bfb64cb4114fa8. Full criterion mapping, commands and file/line repair checklist persisted: https://git.breadway.dev/Breadway/bread-ecosystem/pulls/68#issuecomment-4431. Findings ECO-76. Reproduced shutdown hang with retained logger (timeout3s exit124), raised SIGSEGV survives(exit0), prior disposition overwritten, raw fake PII preserved. Native glibc backtrace is not justified async-signal-safe; no prior handler/alt-stack restoration. Logs drop-newest without sustained rate limit; adoption remains manual, not automatic connect. Panic previous-hook call retained and debug/release crash fixtures pass, but do not satisfy integrated acceptance. Independent66 unit+4 integration tests,4 optimized crash fixtures,GTK check and targeted clippy pass; exact-head check/check(push) SUCCESS Actions297. Actual store PR22, scrubber and pilot CLIP-19 unmerged; store-entry/report acceptance and overhead unverified. BREAD-16/17 to todo for coordinator repair on existing PR68; preserve branch. No merge/tag/release/done. Fresh exact-head review after repair.
fix(bread-utils): breadbug repair for ECO-76 review findings
All checks were successful
check / check (push) Successful in 3m20s
Details
63b855bce6 
Detect Rust std's stack-overflow guard at install time (SA_SIGINFO|SA_ONSTACK
handler in this binary's own executable, matched by device and inode) and fall
back to SIG_DFL so a fault still dies of SIGSEGV instead of SIGABRT. Other
prior handlers keep chaining, covered by a new custom-handler test.

Remove both ignored test attributes: the default-prior and real-segfault tests
now pass asserting SIGSEGV, with the fault frame asserted from the ucontext
RIP inside fixture_segv_site. Gate the overhead example on bread-client so
default-feature builds pass, and record measured overhead in docs.
Breadway commented 2026-10-14 08:52:58 +08:00
Author
Owner
Copy link

Repair for review comment 4431 (ECO-76) - now at 63b855b

Each checklist item from 4431 and where it landed:

  • Automatic on connect (mod.rs / bread_client): BreadClient::connect now starts reporting by default (panic hook, crash handlers, logs, heartbeat). Opt-outs: connect_without_bug_reporting for sensitive apps, BREAD_BUG_REPORTING=0 for the user/environment, BugOptions for custom setups. GLib heartbeat source is removed on guard drop.

  • Bounded nonblocking lifecycle: log queue (512) drops oldest, producers use try_lock and never wait, token-bucket rate limit (50/s sustained, 200 burst, batches of 50), per-record format cap (2 KiB), non-blocking connect with 200 ms write timeout, 500 ms bounded shutdown. Regression test shutdown_is_bounded_even_while_a_logger_clone_is_alive holds a logger clone across guard drop.

  • Scrub before sending: bug/scrub.rs redacts home paths, user names, /home|/Users|/run/user roots, emails, IPs, token prefixes, secret key=value pairs, and long hex ids. Applied to panic messages, panic locations, module paths, and log lines before they reach the socket, with seeded assertions on actual serialized socket bytes in bug/tests.rs.

  • Signal-safe unwinding, prior actions, fatal semantics: the handler no longer calls backtrace(3); it walks the frame-pointer chain starting from the ucontext RIP/RBP on x86_64 Linux (cfg-gated, fallback elsewhere), probing words through a pipe so EFAULT ends a corrupt walk without a second fault. Prior sigactions are saved with checked setup and rollback; sync faults re-execute under the restored action, self-sent signals are re-raised, SIG_IGN faults terminate, MSG_NOSIGNAL avoids SIGPIPE substitution. Covered by loader/allocator-safe design, raised-signal handling, custom SA_SIGINFO handler chaining test, alt-stack handling, setup-failure test, and concurrent-crash guard (RUNNING).

  • std guard chaining: the prior worker's .init_array snapshot could not work (verified with a probe: constructors run before runtime init, so the action then is still SIG_DFL; std installs SA_SIGINFO|SA_ONSTACK|SA_NODEFER later, inside the binary). Replaced with an install-time check (is_std_guard): real handler plus SA_SIGINFO|SA_ONSTACK plus address in this binary's own executable (loader object matched by device/inode). Only std matches; C libraries and custom handlers keep chaining. Trade-off documented in module docs and docs/breadbug-client.md.

  • Tests un-ignored: both #[ignore]s removed. segv_with_default_prior_action_dies_of_sigsegv and a_real_segfault_is_reported_then_the_process_still_dies_of_it pass asserting death by SIGSEGV; the latter also asserts frame 0 (ucontext RIP) lies inside fixture_segv_site. New test: a custom non-std prior SIGSEGV handler still runs (exit 77 + marker byte) and the crash marker is still sent.

  • Overhead: cargo run --release -p bread-utils --features bread-client --locked --example bug_overhead plus measured numbers in docs/breadbug-client.md (AMD Ryzen AI 7 350, rustc 1.97.1, 7 runs, medians: tick 48.9 ns, log 869.6 ns, log::Log 1204.9 ns). Also fixed: example gated with required-features = ["bread-client"] so default-feature builds pass.

Checks on final tree, all green: fmt, clippy -D warnings (base, gtk, extract, plus bread-client all-targets), workspace tests (base, gtk, extract), --locked extract build, lint-pins.sh. One transient lib failure seen once mid-work (77/1) passes on every re-run including full workspace runs; looks like a timing flake, flagging in case it recurs.

Still honestly open (unchanged): breadd store entry and induced breadclip pilot panic (bread PR22, CLIP-19 unmerged), breadd-side allowlist and report-time scrub layers, stack-overflow-as-overflow report wording.

## Repair for review comment 4431 (ECO-76) - now at 63b855b Each checklist item from 4431 and where it landed: - **Automatic on connect (mod.rs / bread_client):** `BreadClient::connect` now starts reporting by default (panic hook, crash handlers, logs, heartbeat). Opt-outs: `connect_without_bug_reporting` for sensitive apps, `BREAD_BUG_REPORTING=0` for the user/environment, `BugOptions` for custom setups. GLib heartbeat source is removed on guard drop. - **Bounded nonblocking lifecycle:** log queue (512) drops oldest, producers use `try_lock` and never wait, token-bucket rate limit (50/s sustained, 200 burst, batches of 50), per-record format cap (2 KiB), non-blocking connect with 200 ms write timeout, 500 ms bounded shutdown. Regression test `shutdown_is_bounded_even_while_a_logger_clone_is_alive` holds a logger clone across guard drop. - **Scrub before sending:** `bug/scrub.rs` redacts home paths, user names, `/home|/Users|/run/user` roots, emails, IPs, token prefixes, secret `key=value` pairs, and long hex ids. Applied to panic messages, panic locations, module paths, and log lines before they reach the socket, with seeded assertions on actual serialized socket bytes in `bug/tests.rs`. - **Signal-safe unwinding, prior actions, fatal semantics:** the handler no longer calls `backtrace(3)`; it walks the frame-pointer chain starting from the ucontext RIP/RBP on x86_64 Linux (cfg-gated, fallback elsewhere), probing words through a pipe so `EFAULT` ends a corrupt walk without a second fault. Prior `sigaction`s are saved with checked setup and rollback; sync faults re-execute under the restored action, self-sent signals are re-raised, `SIG_IGN` faults terminate, `MSG_NOSIGNAL` avoids SIGPIPE substitution. Covered by loader/allocator-safe design, raised-signal handling, custom SA_SIGINFO handler chaining test, alt-stack handling, setup-failure test, and concurrent-crash guard (`RUNNING`). - **std guard chaining:** the prior worker's `.init_array` snapshot could not work (verified with a probe: constructors run before runtime init, so the action then is still `SIG_DFL`; std installs `SA_SIGINFO|SA_ONSTACK|SA_NODEFER` later, inside the binary). Replaced with an install-time check (`is_std_guard`): real handler plus `SA_SIGINFO|SA_ONSTACK` plus address in this binary's own executable (loader object matched by device/inode). Only std matches; C libraries and custom handlers keep chaining. Trade-off documented in module docs and `docs/breadbug-client.md`. - **Tests un-ignored:** both `#[ignore]`s removed. `segv_with_default_prior_action_dies_of_sigsegv` and `a_real_segfault_is_reported_then_the_process_still_dies_of_it` pass asserting death by SIGSEGV; the latter also asserts frame 0 (ucontext RIP) lies inside `fixture_segv_site`. New test: a custom non-std prior SIGSEGV handler still runs (exit 77 + marker byte) and the crash marker is still sent. - **Overhead:** `cargo run --release -p bread-utils --features bread-client --locked --example bug_overhead` plus measured numbers in `docs/breadbug-client.md` (AMD Ryzen AI 7 350, rustc 1.97.1, 7 runs, medians: tick 48.9 ns, log 869.6 ns, log::Log 1204.9 ns). Also fixed: example gated with `required-features = ["bread-client"]` so default-feature builds pass. Checks on final tree, all green: fmt, clippy `-D warnings` (base, gtk, extract, plus bread-client all-targets), workspace tests (base, gtk, extract), `--locked` extract build, `lint-pins.sh`. One transient lib failure seen once mid-work (77/1) passes on every re-run including full workspace runs; looks like a timing flake, flagging in case it recurs. Still honestly open (unchanged): breadd store entry and induced breadclip pilot panic (bread PR22, CLIP-19 unmerged), breadd-side allowlist and report-time scrub layers, stack-overflow-as-overflow report wording.
Breadway commented 2026-10-14 10:33:51 +08:00
Author
Owner
Copy link

Independent exact-head review: changes requested

Reviewed 63b855bce6 against main 4f70691b33 (merge-base b52f0fd6fa). Expected head matches. Isolated detached scratch ~/.worktrees/bread-ecosystem/review-pr68-63b855b. Primary checkout left at b2614ba. Reviews API empty. Prior changes_requested comment 4431 was against b2614ba; repair commits a9f01d7 and 63b855b were read in full (14 files).

What the repair fixed (verified, not taken from comment 4614)

  • Install-time std-guard detection replaced the .init_array snapshot. native.rs:438-494: real handler plus SA_SIGINFO|SA_ONSTACK plus dladdr object matched to /proc/self/exe by device and inode. Both former #[ignore] tests are gone. cargo test --features bread-client --locked --test bug_crashes -- --test-threads=1: 7 passed, 0 ignored. segv_with_default_prior_action_dies_of_sigsegv and a_real_segfault_is_reported_then_the_process_still_dies_of_it assert death by SIGSEGV. a_custom_prior_sigsegv_handler_is_still_called_after_our_report: custom SA_SIGINFO handler exits 77, writes marker byte, and the crash marker is still sent.
  • ucontext walk: native.rs:275-308 reads REG_RIP (16) and REG_RBP (10). The real-segfault test asserts frame 0 offset is inside fixture_segv_site (span 0x80).
  • Raised SIGSEGV no longer survives. Independent release probe: native::install then raise(SIGSEGV) died with signal 11 and a crash marker. Prior SIG_IGN plus raise survived (exit 0) and still reported. Prior SIG_IGN plus a synchronous null store died with signal 11. Home path in the marker was redacted to ~/.
  • Auto-adoption: BreadClient::connect calls bug::start_automatic (bread_client.rs:91-96). Opt-outs: connect_without_bug_reporting, BREAD_BUG_REPORTING=0/off/false/no/disabled, BugOptions::disabled.
  • Scrub before send: scrub.rs applied to panic message/location, module paths, and log lines. Seeded socket test forwarded_logs_are_redacted_before_they_reach_the_socket passed. Independent probe wire was mail <email> path /home/<user>/secret token=<redacted> <secret> ip <ip> with no raw email, token, or IP.
  • Shutdown: drop waits at most 500 ms via a done channel, not a join on a live sender. shutdown_is_bounded_even_while_a_logger_clone_is_alive passed. Non-blocking connect, 200 ms write timeout.
  • Full-queue eviction is oldest-first (mod.rs:264-267). overflow_drops_the_oldest_lines_and_keeps_the_newest passed. Token bucket 50/s, burst 200, batch 50. a_sustained_flood_is_sent_at_the_rate_budget_not_all_at_once passed.
  • Overhead remeasured here: cargo run --release -p bread-utils --features bread-client --locked --example bug_overhead, three runs, AMD Ryzen AI 7 350, 16 CPUs, rustc 1.97.1. tick 49.5/50.2/52.9 ns; log 834.4/870.8/921.1 ns; log::Log 1209.5/1247.0/1565.7 ns. Documented medians in docs/breadbug-client.md (tick 48.9, log 869.6, log::Log 1204.9) are consistent. Command and machine are in that doc.
  • Handler no longer calls backtrace(3). Pipe-probed frame-pointer walk, MSG_NOSIGNAL, checked sigaction with rollback. Setup-failure test passed.
  • Local checks: cargo fmt --all -- --check clean. clippy -D warnings clean for default, --features gtk, and --features extract (workspace, all-targets, --locked). cargo test --workspace --locked --features gtk passed, including the 7 crash tests again. scripts/lint-pins.sh and cargo build --locked --features extract were not reached because the extract test run failed first (below). CI check / check (push) success on this exact SHA, Actions run 300, job id 2417, 3m20s. Branch protections empty. Required workflow is check.yml only.

Still failing

Drop-oldest is not actually true when the worker holds the queue lock.

bread-utils/src/bug/mod.rs:256-274: Queue::push uses try_lock and, on WouldBlock, returns 1 without storing the line. enqueue at 518-530 counts that as a drop. take at 278-287 holds the same mutex, including across Condvar::wait_timeout while idle. A log call during that wait drops the newest line even when the queue has room, and does not wake the worker. The 512-cap path does drop the oldest, but only after the lock is acquired.

Reproduced twice:

  1. cargo test --workspace --locked: bug::tests::logs_are_batched_clipped_and_leveled panicked at tests.rs:138, left 2, right 3. The missing line was the last one ("third" / ERROR).
  2. cargo test --workspace --locked --features extract: same assertion. Received WARN "first" and the clipped INFO line; ERROR "third" absent. Two lines, so this was not the 512-cap eviction.

The same test passed 5 times alone and 3 times as bread-utils --lib --test-threads=8, so it is racy, not a bad assertion. Filed ECO-77.

Unverified, not passed

  • Integrated breadd store entry for an induced panic or SIGSEGV. Client evidence is a fake unix socket. bread PR 22 is merged (merge SHA aea9879d61deaf55000ec7b5a377e79299170232), but this PR does not exercise that store.
  • breadclip pilot CLIP-19 is still backlog and must not merge before a bread-utils tag. No tag is part of this review.
  • breadd allowlist ingest scrub and report-time scrub (layers 2 and 3). Client redaction is a denylist and is documented as such.
  • Real GLib main-context heartbeat under a blocked UI thread. attach_glib exists (mod.rs:451-461) and the gtk feature builds and tests; no display/main-loop observation.
  • Stack-overflow-as-overflow wording, documented as a known trade-off because std's guard is treated as SIG_DFL.

Repair checklist (existing PR, do not close)

  • mod.rs:256-274 and 278-287: a producer must not discard a line that still fits. Drop the oldest only at the 512 cap. A busy or idle-waiting worker must not turn a new line into a silent drop. Regression: contend take/wait with a producer and assert the newest line is delivered while len < 512. Do not weaken logs_are_batched_clipped_and_leveled.
  • Re-run cargo test --workspace --locked and --features extract until that test is stable, then the remaining check.yml steps this review did not finish (lint-pins.sh, cargo build --workspace --locked --features extract).

No merge. Cards stay out of done.

## Independent exact-head review: changes requested Reviewed 63b855bce63313f789ab04ffea96f0b89fdce974 against main 4f70691b33bc778294073681065665d3e2b7f559 (merge-base b52f0fd6faf10c0f595c70dbc3bfb64cb4114fa8). Expected head matches. Isolated detached scratch ~/.worktrees/bread-ecosystem/review-pr68-63b855b. Primary checkout left at b2614ba. Reviews API empty. Prior changes_requested comment 4431 was against b2614ba; repair commits a9f01d7 and 63b855b were read in full (14 files). ### What the repair fixed (verified, not taken from comment 4614) - Install-time std-guard detection replaced the .init_array snapshot. native.rs:438-494: real handler plus SA_SIGINFO|SA_ONSTACK plus dladdr object matched to /proc/self/exe by device and inode. Both former #[ignore] tests are gone. cargo test --features bread-client --locked --test bug_crashes -- --test-threads=1: 7 passed, 0 ignored. segv_with_default_prior_action_dies_of_sigsegv and a_real_segfault_is_reported_then_the_process_still_dies_of_it assert death by SIGSEGV. a_custom_prior_sigsegv_handler_is_still_called_after_our_report: custom SA_SIGINFO handler exits 77, writes marker byte, and the crash marker is still sent. - ucontext walk: native.rs:275-308 reads REG_RIP (16) and REG_RBP (10). The real-segfault test asserts frame 0 offset is inside fixture_segv_site (span 0x80). - Raised SIGSEGV no longer survives. Independent release probe: native::install then raise(SIGSEGV) died with signal 11 and a crash marker. Prior SIG_IGN plus raise survived (exit 0) and still reported. Prior SIG_IGN plus a synchronous null store died with signal 11. Home path in the marker was redacted to ~/. - Auto-adoption: BreadClient::connect calls bug::start_automatic (bread_client.rs:91-96). Opt-outs: connect_without_bug_reporting, BREAD_BUG_REPORTING=0/off/false/no/disabled, BugOptions::disabled. - Scrub before send: scrub.rs applied to panic message/location, module paths, and log lines. Seeded socket test forwarded_logs_are_redacted_before_they_reach_the_socket passed. Independent probe wire was `mail <email> path /home/<user>/secret token=<redacted> <secret> ip <ip>` with no raw email, token, or IP. - Shutdown: drop waits at most 500 ms via a done channel, not a join on a live sender. shutdown_is_bounded_even_while_a_logger_clone_is_alive passed. Non-blocking connect, 200 ms write timeout. - Full-queue eviction is oldest-first (mod.rs:264-267). overflow_drops_the_oldest_lines_and_keeps_the_newest passed. Token bucket 50/s, burst 200, batch 50. a_sustained_flood_is_sent_at_the_rate_budget_not_all_at_once passed. - Overhead remeasured here: cargo run --release -p bread-utils --features bread-client --locked --example bug_overhead, three runs, AMD Ryzen AI 7 350, 16 CPUs, rustc 1.97.1. tick 49.5/50.2/52.9 ns; log 834.4/870.8/921.1 ns; log::Log 1209.5/1247.0/1565.7 ns. Documented medians in docs/breadbug-client.md (tick 48.9, log 869.6, log::Log 1204.9) are consistent. Command and machine are in that doc. - Handler no longer calls backtrace(3). Pipe-probed frame-pointer walk, MSG_NOSIGNAL, checked sigaction with rollback. Setup-failure test passed. - Local checks: cargo fmt --all -- --check clean. clippy -D warnings clean for default, --features gtk, and --features extract (workspace, all-targets, --locked). cargo test --workspace --locked --features gtk passed, including the 7 crash tests again. scripts/lint-pins.sh and cargo build --locked --features extract were not reached because the extract test run failed first (below). CI check / check (push) success on this exact SHA, Actions run 300, job id 2417, 3m20s. Branch protections empty. Required workflow is check.yml only. ### Still failing Drop-oldest is not actually true when the worker holds the queue lock. bread-utils/src/bug/mod.rs:256-274: Queue::push uses try_lock and, on WouldBlock, returns 1 without storing the line. enqueue at 518-530 counts that as a drop. take at 278-287 holds the same mutex, including across Condvar::wait_timeout while idle. A log call during that wait drops the newest line even when the queue has room, and does not wake the worker. The 512-cap path does drop the oldest, but only after the lock is acquired. Reproduced twice: 1. cargo test --workspace --locked: bug::tests::logs_are_batched_clipped_and_leveled panicked at tests.rs:138, left 2, right 3. The missing line was the last one ("third" / ERROR). 2. cargo test --workspace --locked --features extract: same assertion. Received WARN "first" and the clipped INFO line; ERROR "third" absent. Two lines, so this was not the 512-cap eviction. The same test passed 5 times alone and 3 times as bread-utils --lib --test-threads=8, so it is racy, not a bad assertion. Filed ECO-77. ### Unverified, not passed - Integrated breadd store entry for an induced panic or SIGSEGV. Client evidence is a fake unix socket. bread PR 22 is merged (merge SHA aea9879d61deaf55000ec7b5a377e79299170232), but this PR does not exercise that store. - breadclip pilot CLIP-19 is still backlog and must not merge before a bread-utils tag. No tag is part of this review. - breadd allowlist ingest scrub and report-time scrub (layers 2 and 3). Client redaction is a denylist and is documented as such. - Real GLib main-context heartbeat under a blocked UI thread. attach_glib exists (mod.rs:451-461) and the gtk feature builds and tests; no display/main-loop observation. - Stack-overflow-as-overflow wording, documented as a known trade-off because std's guard is treated as SIG_DFL. ### Repair checklist (existing PR, do not close) - [ ] mod.rs:256-274 and 278-287: a producer must not discard a line that still fits. Drop the oldest only at the 512 cap. A busy or idle-waiting worker must not turn a new line into a silent drop. Regression: contend take/wait with a producer and assert the newest line is delivered while len < 512. Do not weaken logs_are_batched_clipped_and_leveled. - [ ] Re-run cargo test --workspace --locked and --features extract until that test is stable, then the remaining check.yml steps this review did not finish (lint-pins.sh, cargo build --workspace --locked --features extract). No merge. Cards stay out of done.
fix(bread-utils): stop dropping new log lines on queue contention
All checks were successful
check / check (push) Successful in 4m17s
Details
47dbe00f5e 
ECO-77: Queue::push used a single try_lock and dropped the NEW line on
WouldBlock, so a producer racing the worker's take() lost lines even with
room in the 512 queue. push now retries try_lock for PUSH_RETRY (1ms),
then takes the blocking lock: only a full queue drops, and then the
oldest. The blocking tail is safe because the worker only holds the lock
for a microsecond-scale drain and never does IO under it.

take() pre-reserves its output buffer so no allocation happens under the
lock while producers may be waiting on it.

Adds a contention regression test (8 producers x 50 lines under a live
taker, queue never full) that fails on the old code with 325 dropped
lines and passes with the fix, plus a sequential-acceptor fix for the
test recorder that reordered lines across sends.
Breadway commented 2026-10-14 12:23:38 +08:00
Author
Owner
Copy link

ECO-77 fix is in (commit 47dbe00).

Defect (review comment 4678): Queue::push used a single try_lock and returned 1 (dropped) on WouldBlock, discarding the NEW line even when the 512-queue had room. A producer racing the worker's take() lost lines to pure contention.

Fix (bread-utils/src/bug/mod.rs):

  • push retries try_lock for PUSH_RETRY (1ms, Duration::from_micros(1000)), then takes the blocking lock. Only a FULL queue drops, and then the oldest (pop_front, returns 1); otherwise 0.
  • The blocking tail cannot stall the app thread meaningfully: the worker (take()) only holds the lock for a microsecond-scale drain, never for IO. send_logs (socket write) runs after take() returns with an owned Vec, guard already dropped; verified by reading log_loop/send_logs. Documented on push.
  • take() pre-reserves its output buffer before locking, so no allocation happens under the lock while producers may be waiting on it.
  • Poisoning is recover-and-continue (into_inner), with a justifying comment on lock(): the queue holds only best-effort telemetry lines with no cross-line invariant, so a partial line is already an accepted loss mode; dropping the backlog would need &mut access we do not have and would destroy evidence for zero safety gain.

Regression test (bread-utils/src/bug/tests.rs::contended_push_drops_nothing_while_under_cap): 8 producers x 50 lines (400 total, under the 512 cap) under a live taker mimicking the log worker. Old-fails/new-passes proof: with only mod.rs reverted to 63b855b the test FAILS (contention dropped lines while under cap, left: 325, right: 0); with the fix it passes 5/5 runs. Also fixed the test recorder() to accept connections sequentially (a thread per connection reordered lines across sends and made the batching test flaky).

Evidence, all green:

  • cargo fmt --all -- --check: clean
  • cargo clippy --workspace --all-targets --locked -- -D warnings (base, gtk, extract): clean; plus -p bread-utils with gtk and with bread-client: clean
  • cargo test -p bread-utils --locked --features bread-client: 79 passed + bug_crashes 7 passed
  • cargo test --workspace --locked (+ gtk, + extract): all ok, 0 failed (extract: 448 passed, 0 failed)
  • scripts/lint-pins.sh: all tag-pinned

Pushed fast-forward 63b855b..47dbe00 to feature/breadbug-client.

ECO-77 fix is in (commit 47dbe00). Defect (review comment 4678): `Queue::push` used a single `try_lock` and returned 1 (dropped) on `WouldBlock`, discarding the NEW line even when the 512-queue had room. A producer racing the worker's `take()` lost lines to pure contention. Fix (`bread-utils/src/bug/mod.rs`): - `push` retries `try_lock` for `PUSH_RETRY` (1ms, `Duration::from_micros(1000)`), then takes the blocking lock. Only a FULL queue drops, and then the oldest (`pop_front`, returns 1); otherwise 0. - The blocking tail cannot stall the app thread meaningfully: the worker (`take()`) only holds the lock for a microsecond-scale `drain`, never for IO. `send_logs` (socket write) runs after `take()` returns with an owned `Vec`, guard already dropped; verified by reading `log_loop`/`send_logs`. Documented on `push`. - `take()` pre-reserves its output buffer before locking, so no allocation happens under the lock while producers may be waiting on it. - Poisoning is recover-and-continue (`into_inner`), with a justifying comment on `lock()`: the queue holds only best-effort telemetry lines with no cross-line invariant, so a partial line is already an accepted loss mode; dropping the backlog would need `&mut` access we do not have and would destroy evidence for zero safety gain. Regression test (`bread-utils/src/bug/tests.rs::contended_push_drops_nothing_while_under_cap`): 8 producers x 50 lines (400 total, under the 512 cap) under a live taker mimicking the log worker. Old-fails/new-passes proof: with only `mod.rs` reverted to 63b855b the test FAILS (`contention dropped lines while under cap`, left: 325, right: 0); with the fix it passes 5/5 runs. Also fixed the test `recorder()` to accept connections sequentially (a thread per connection reordered lines across sends and made the batching test flaky). Evidence, all green: - `cargo fmt --all -- --check`: clean - `cargo clippy --workspace --all-targets --locked -- -D warnings` (base, `gtk`, `extract`): clean; plus `-p bread-utils` with `gtk` and with `bread-client`: clean - `cargo test -p bread-utils --locked --features bread-client`: 79 passed + `bug_crashes` 7 passed - `cargo test --workspace --locked` (+ `gtk`, + `extract`): all ok, 0 failed (`extract`: 448 passed, 0 failed) - `scripts/lint-pins.sh`: all tag-pinned Pushed fast-forward 63b855b..47dbe00 to `feature/breadbug-client`.
Breadway commented 2026-10-14 12:38:58 +08:00
Author
Owner
Copy link

Independent SHA-bound re-review of PR #68 at 47dbe00f5e (base 4f70691b33): changes requested.

Criteria and evidence

  • BREAD-16 panic/log/heartbeat/opt-out/no-op/build-id: implementation and prior verification are present in this PR; this review found no regression in the prior fixes (custom panic chaining, bounded teardown with retained logger, auto-start/opt-out, client redaction). Store/pilot and app overhead acceptance remain external gates on CLIP-19 / integrated store; this PR does not complete those cards.
  • BREAD-17 native crash payload/signal behavior: prior exact-head evidence covers synchronous SIGSEGV termination, previous-handler chaining/restoration, and frame-only payload. Integrated breadd store entry remains unverified because prerequisite is not integrated. No tag/release is authorized.
  • ECO-77 / drop-oldest while queue has room: FAIL. bread-utils/src/bug/mod.rs Queue::push (around lines 256-305) spins for 1 ms then calls blocking lock() if contention persists. This guarantees insertion eventually, but violates the app-thread nonblocking contract: a descheduled/preempted holder or scheduler stall can make log() wait without a bound. “No IO under lock” does not establish a bound on mutex acquisition. The earlier reproduced defect was dropped newest; replacing it with an unbounded blocking tail is not acceptable for logging from UI/app threads. Replace with a genuinely bounded/nonblocking strategy (for example side slot/lock-free pending item merged by consumer); never wait indefinitely on the producer path. Add a regression that holds the queue lock longer than retry budget and proves producer returns promptly while newest line is retained when below cap. Preserve full-cap drop-oldest and existing batch tests.
  • Logging rate limiting/custom handler chaining/crash semantics/redaction: documentation and implementation observed; prior review evidence remains, no new failures found. Rate limiter code uses token budget/burst. Must rerun tests after repair.

Evidence/checks

  • Exact head confirmed via PR metadata: 47dbe00f5e; base 4f70691b33; PR open and mergeable.
  • Exact SHA combined status: check / check (push), success (Actions run 301). Forgejo exposes this as one required status context; no separate job-level status endpoint was available here.
  • Changes-file pagination exhausted: 14 files total.
  • Previous comments 4678 and repair note 4734 inspected. Still do not mark BREAD-16/17 complete: pilot/store and CLIP-19/tag gates remain open.

No merge. Keep PR and cards in review for repair; do not mark done. Next action: replace blocking mutex tail with bounded/nonblocking producer path, add contention-duration regression, rerun full locked workspace + gtk checks and exact-head CI, then request fresh review. No tag.

Independent SHA-bound re-review of PR #68 at 47dbe00f5e1caf06dc3fa78d904001ba978e5b9a (base 4f70691b33bc778294073681065665d3e2b7f559): changes requested. ## Criteria and evidence - BREAD-16 panic/log/heartbeat/opt-out/no-op/build-id: implementation and prior verification are present in this PR; this review found no regression in the prior fixes (custom panic chaining, bounded teardown with retained logger, auto-start/opt-out, client redaction). Store/pilot and app overhead acceptance remain external gates on CLIP-19 / integrated store; this PR does not complete those cards. - BREAD-17 native crash payload/signal behavior: prior exact-head evidence covers synchronous SIGSEGV termination, previous-handler chaining/restoration, and frame-only payload. Integrated breadd store entry remains unverified because prerequisite is not integrated. No tag/release is authorized. - ECO-77 / drop-oldest while queue has room: FAIL. bread-utils/src/bug/mod.rs Queue::push (around lines 256-305) spins for 1 ms then calls blocking lock() if contention persists. This guarantees insertion eventually, but violates the app-thread nonblocking contract: a descheduled/preempted holder or scheduler stall can make log() wait without a bound. “No IO under lock” does not establish a bound on mutex acquisition. The earlier reproduced defect was dropped newest; replacing it with an unbounded blocking tail is not acceptable for logging from UI/app threads. Replace with a genuinely bounded/nonblocking strategy (for example side slot/lock-free pending item merged by consumer); never wait indefinitely on the producer path. Add a regression that holds the queue lock longer than retry budget and proves producer returns promptly while newest line is retained when below cap. Preserve full-cap drop-oldest and existing batch tests. - Logging rate limiting/custom handler chaining/crash semantics/redaction: documentation and implementation observed; prior review evidence remains, no new failures found. Rate limiter code uses token budget/burst. Must rerun tests after repair. ## Evidence/checks - Exact head confirmed via PR metadata: 47dbe00f5e1caf06dc3fa78d904001ba978e5b9a; base 4f70691b33bc778294073681065665d3e2b7f559; PR open and mergeable. - Exact SHA combined status: check / check (push), success (Actions run 301). Forgejo exposes this as one required status context; no separate job-level status endpoint was available here. - Changes-file pagination exhausted: 14 files total. - Previous comments 4678 and repair note 4734 inspected. Still do not mark BREAD-16/17 complete: pilot/store and CLIP-19/tag gates remain open. No merge. Keep PR and cards in review for repair; do not mark done. Next action: replace blocking mutex tail with bounded/nonblocking producer path, add contention-duration regression, rerun full locked workspace + gtk checks and exact-head CI, then request fresh review. No tag.
Breadway commented 2026-10-14 12:39:27 +08:00
Author
Owner
Copy link

Independent SHA-bound re-review of PR #68 at 47dbe00f5e: changes requested. Queue::push in bread-utils/src/bug/mod.rs spins 1ms then uses blocking lock(). This leaves an unbounded producer wait if a lock holder is preempted or stalled, violating the app-thread nonblocking contract. No IO under lock does not bound mutex acquisition. Replace with a genuinely bounded/nonblocking path retaining newest under capacity; add regression holding lock past retry duration and assert prompt return plus item retained. Preserve full-cap drop-oldest. Prior fixes for shutdown, signal chaining/fatal semantics, redaction, etc. retain their earlier evidence. Exact SHA status check/check (push) SUCCESS (Actions 301); 14 changed files exhausted; base 4f70691b33. BREAD-16/17 integrated store and breadclip pilot/CLIP-19 gates remain open, no tag. No merge, no done. Keep PR and cards in review for repair, then rerun full locked workspace/gtk and exact-head CI and request fresh review.

Independent SHA-bound re-review of PR #68 at 47dbe00f5e1caf06dc3fa78d904001ba978e5b9a: changes requested. Queue::push in bread-utils/src/bug/mod.rs spins 1ms then uses blocking lock(). This leaves an unbounded producer wait if a lock holder is preempted or stalled, violating the app-thread nonblocking contract. No IO under lock does not bound mutex acquisition. Replace with a genuinely bounded/nonblocking path retaining newest under capacity; add regression holding lock past retry duration and assert prompt return plus item retained. Preserve full-cap drop-oldest. Prior fixes for shutdown, signal chaining/fatal semantics, redaction, etc. retain their earlier evidence. Exact SHA status check/check (push) SUCCESS (Actions 301); 14 changed files exhausted; base 4f70691b33bc778294073681065665d3e2b7f559. BREAD-16/17 integrated store and breadclip pilot/CLIP-19 gates remain open, no tag. No merge, no done. Keep PR and cards in review for repair, then rerun full locked workspace/gtk and exact-head CI and request fresh review.
fix(bread-utils): make log enqueue nonblocking
All checks were successful
check / check (push) Successful in 7m33s
Details
711e3e8261
Breadway commented 2026-10-14 12:45:14 +08:00
Author
Owner
Copy link

Repair pushed at exact head 711e3e8261064e066b53b8f31d0cfade94d1d8ba. Root cause: producer retried try_lock for 1ms, then called blocking lock, so a preempted consumer could stall the app thread indefinitely despite no I/O under the mutex. Replaced the mutex-protected deque with vetted crossbeam_queue::ArrayQueue; enqueue no longer takes a mutex, and only evicts oldest on full. Added regression holding the consumer wait lock across enqueue: producer returns within 100ms and retains the new below-cap line. Validation: cargo test --locked --offline --workspace passed (one ignored heavyweight image case); cargo clippy --locked --offline --workspace --all-targets --all-features -- -D warnings passed; cargo check --locked --offline --workspace --all-targets --all-features passed including GTK; cargo fmt --all applied and git diff --check passed. Store/pilot integration gates remain open; no merge or done claim. Note: bread PR22 is already merged, so review's contrary claim is stale; CLIP-19 remains unmerged. Holding for fresh exact-head review.

Repair pushed at exact head `711e3e8261064e066b53b8f31d0cfade94d1d8ba`. Root cause: producer retried `try_lock` for 1ms, then called blocking `lock`, so a preempted consumer could stall the app thread indefinitely despite no I/O under the mutex. Replaced the mutex-protected deque with vetted `crossbeam_queue::ArrayQueue`; enqueue no longer takes a mutex, and only evicts oldest on full. Added regression holding the consumer wait lock across enqueue: producer returns within 100ms and retains the new below-cap line. Validation: `cargo test --locked --offline --workspace` passed (one ignored heavyweight image case); `cargo clippy --locked --offline --workspace --all-targets --all-features -- -D warnings` passed; `cargo check --locked --offline --workspace --all-targets --all-features` passed including GTK; `cargo fmt --all` applied and `git diff --check` passed. Store/pilot integration gates remain open; no merge or done claim. Note: bread PR22 is already merged, so review's contrary claim is stale; CLIP-19 remains unmerged. Holding for fresh exact-head review.
Breadway commented 2026-10-14 12:48:06 +08:00
Author
Owner
Copy link

Independent re-review of PR68 at exact head 711e3e8261: provider implementation acceptance passes, subject to pending CI. Queue now uses crossbeam ArrayQueue; producers do not acquire the consumer wait mutex. Drop-oldest-on-capacity retry logic appears correct. Held-consumer-lock regression verifies prompt return and retention under capacity. Targeted bug unit tests passed 14/14; native crash integration passed 7/7; cargo test --workspace --locked passed. Ignored heavyweight palette test is unchanged and documented as allocating/encoding about 1 GiB, not an introduced waiver. Consumer checks empty, acquires wait mutex, rechecks before Condvar wait; producer enqueues then notifies, covering the wakeup race. Shutdown still sets stop, notifies all, and uses bounded worker join. Rate budget logic unchanged. Prior fixes for chaining, signal death, and redaction remain. Exact SHA check/check (push) is pending at latest observed state (Actions run 302), so NO MERGE until green. Bread PR22 store prerequisite is integrated; CLIP-19 breadclip pilot remains unmerged. Accept only provider-scope implementation; preserve pilot/rollout gates and do not mark BREAD-16/17 done. Commands: cargo test -p bread-utils --features bread-client bug::tests:: --locked (14 passed); cargo test -p bread-utils --features bread-client --test bug_crashes --locked (7 passed); cargo test --workspace --locked passed, with three unrelated ignored bakery platform tests, one documented 1GiB palette test, one display-dependent GTK test, and ignored doc examples. After CI green, re-read SHA/base/checks/reviews/mergeability and merge using repo strategy, no tag.

Independent re-review of PR68 at exact head 711e3e8261064e066b53b8f31d0cfade94d1d8ba: provider implementation acceptance passes, subject to pending CI. Queue now uses crossbeam ArrayQueue; producers do not acquire the consumer wait mutex. Drop-oldest-on-capacity retry logic appears correct. Held-consumer-lock regression verifies prompt return and retention under capacity. Targeted bug unit tests passed 14/14; native crash integration passed 7/7; cargo test --workspace --locked passed. Ignored heavyweight palette test is unchanged and documented as allocating/encoding about 1 GiB, not an introduced waiver. Consumer checks empty, acquires wait mutex, rechecks before Condvar wait; producer enqueues then notifies, covering the wakeup race. Shutdown still sets stop, notifies all, and uses bounded worker join. Rate budget logic unchanged. Prior fixes for chaining, signal death, and redaction remain. Exact SHA check/check (push) is pending at latest observed state (Actions run 302), so NO MERGE until green. Bread PR22 store prerequisite is integrated; CLIP-19 breadclip pilot remains unmerged. Accept only provider-scope implementation; preserve pilot/rollout gates and do not mark BREAD-16/17 done. Commands: cargo test -p bread-utils --features bread-client bug::tests:: --locked (14 passed); cargo test -p bread-utils --features bread-client --test bug_crashes --locked (7 passed); cargo test --workspace --locked passed, with three unrelated ignored bakery platform tests, one documented 1GiB palette test, one display-dependent GTK test, and ignored doc examples. After CI green, re-read SHA/base/checks/reviews/mergeability and merge using repo strategy, no tag.
Breadway commented 2026-10-16 19:08:35 +08:00
Author
Owner
Copy link

Independent final defect gate: ECO-76 / ECO-77 on merged PR68 (defect-only, no source change)

Reviewed accepted code head 711e3e8261, merge commit 94ea37490f (parents 4f70691b33 and 711e3e8). Read-only; isolated detached scratch ~/.worktrees/bread-ecosystem/review-pr68 at 94ea374, clean. This is not full BREAD-16/17 acceptance.

Merge tree equals reviewed head

  • All 15 PR files (merge-base b52f0fd..711e3e8) have identical blob SHAs at 711e3e8 and at 94ea374. Changed-file set of 4f70691..94ea374 equals the PR set exactly.
  • The only other content in the merge is the 5 files main added before the merge (check.yml, ci/Containerfile, docs/breadbug-symbols.md, scripts/publish-symbols.sh, scripts/test-publish-symbols.sh), which match 4f70691.
  • Synthetic git merge-tree of 4f70691 + 711e3e8 yields tree 7a498cb825, equal to 94ea374's tree. No unreviewed code entered the merge.

Checks and reviews (live reads)

  • 711e3e8: check / check (push) success, Actions run 302, 7m33s. 94ea374: check / check (push) success run 303 (3m9s), dev bakery / build success run 304, dev bread-theme / build success run 305. The workflow defines one job (check) covering lint-pins, fmt, clippy base/gtk/extract, test base/gtk/extract and locked extract build, so the single status context is the whole job. Per-step logs need a login (run pages return 307 unauthenticated), so step results are read from the job status plus my own reruns below.
  • Reviews API for PR68: empty. No change request outstanding. Last comment 4795 is pass for provider scope conditional on CI; CI is now green on that exact SHA. PR is merged=true, merge_commit_sha 94ea374, base main. Branch protection API needs a token (401); not observable, merge already done.

ECO-76 defect map (original card vs merged tree)

  1. Shutdown hang with a surviving logger clone: PASS. mod.rs BugGuard::drop waits on a done channel for at most JOIN_TIMEOUT (500 ms). Test shutdown_is_bounded_even_while_a_logger_clone_is_alive passed in my rerun.
  2. Raised signal returns and survives: PASS. native.rs handler re-raises for si_code <= 0 (lines ~381-409). Probe on 94ea374 (/tmp/opencode/eco-probe, native::install with prior SIG_DFL): raise(SIGSEGV) exit 139, kill(getpid, SIGSEGV) exit 139, synchronous null write exit 139.
  3. Prior disposition overwritten / not restored / not chained: PASS. make_resources saves every prior action with checked sigaction; ensure_alt_stack keeps an existing alt stack and unmaps on failure; install rolls back on failure. Probe: prior SIG_IGN + raise survives (exit 0, ignored stays ignored), prior SIG_IGN + synchronous fault dies with SIGSEGV (exit 139). bug_crashes: a_custom_prior_sigsegv_handler_is_still_called_after_our_report and a_failed_setup_leaves_every_signal_action_as_it_was passed.
  4. glibc backtrace in the signal handler: PASS. No libc::backtrace in native.rs (only in panic.rs:82, the normal panic path, not a handler). Handler walks ucontext RIP/RBP with write-to-pipe probing (native.rs:248-308), only async-signal-safe calls, no allocation or loader locks. Module docs state the rationale. Loader contention has no dedicated test; the guarantee is by design (no loader/malloc call exists in the handler).
  5. Drop-newest and no sustained rate limit: PASS. Queue is crossbeam ArrayQueue (cap 512), full queue evicts oldest only. Tests overflow_drops_the_oldest_lines_and_keeps_the_newest, a_sustained_flood_is_sent_at_the_rate_budget_not_all_at_once passed.
  6. Raw module paths, panic text and logs unredacted: PASS. scrub.rs applied to panic message/location, module paths (native install and panic marker) and forwarded logs. Tests forwarded_logs_are_redacted_before_they_reach_the_socket, marker_params_redact_home_user_and_secrets, module_paths_are_redacted_before_they_are_rendered passed.

ECO-77 defect map

  • Newest line lost when the queue lock is busy while the queue has room: PASS. Queue::push (mod.rs:~262-280) has no mutex; Condvar coordination lock is consumer-side only. contended_push_drops_nothing_while_under_cap (8 producers x 50 lines, live taker) and producer_is_nonblocking_while_consumer_is_stalled (wait_lock held, push returns under 100 ms, line retained) passed. logs_are_batched_clipped_and_leveled unchanged and passed.

Commands run on 94ea374 (my own, not copied)

  • cargo test -p bread-utils --features bread-client --locked --offline bug:: : 38 passed, 0 failed.
  • cargo test -p bread-utils --features bread-client --locked --offline --test bug_crashes -- --test-threads=1 : 7 passed, 0 ignored.
  • Probe binary above, 5 modes (results in item 2 and 3).
    Not rerun: full workspace and gtk/extract suites; relied on exact-SHA green CI on 711e3e8 and on 94ea374 plus the earlier reviewer and coordinator workspace runs.

Non-blocking residual (not a defect gate failure)

Committed tests do not include a raised-signal case or a prior SIG_IGN case; those behaviors are verified only by the probe above. Recommend a follow-up test card (raise(SIGSEGV) dies of signal 11 under default prior; SIG_IGN prior chain; held-loader contention if feasible). Not filed by this pass.

Verdict

ECO-76 and ECO-77 defect acceptance: pass at 711e3e8 / merged 94ea374. Both defect cards' acceptance is genuinely covered; nothing from the original ECO-76/77 text was dropped except the optional extra regressions noted above. Full BREAD-16/BREAD-17 acceptance remains OPEN: CLIP-19 breadclip pilot unmerged, rollout and app-overhead gates unverified, no tag. Board transitions left to the coordinator.

## Independent final defect gate: ECO-76 / ECO-77 on merged PR68 (defect-only, no source change) Reviewed accepted code head 711e3e8261064e066b53b8f31d0cfade94d1d8ba, merge commit 94ea37490f44e2b4a39ec3f475e4be93cf71fc3b (parents 4f70691b33bc778294073681065665d3e2b7f559 and 711e3e8). Read-only; isolated detached scratch ~/.worktrees/bread-ecosystem/review-pr68 at 94ea374, clean. This is not full BREAD-16/17 acceptance. ### Merge tree equals reviewed head - All 15 PR files (merge-base b52f0fd..711e3e8) have identical blob SHAs at 711e3e8 and at 94ea374. Changed-file set of 4f70691..94ea374 equals the PR set exactly. - The only other content in the merge is the 5 files main added before the merge (check.yml, ci/Containerfile, docs/breadbug-symbols.md, scripts/publish-symbols.sh, scripts/test-publish-symbols.sh), which match 4f70691. - Synthetic git merge-tree of 4f70691 + 711e3e8 yields tree 7a498cb825a3e7e279dfd5f2777e4ea684bd25fb, equal to 94ea374's tree. No unreviewed code entered the merge. ### Checks and reviews (live reads) - 711e3e8: check / check (push) success, Actions run 302, 7m33s. 94ea374: check / check (push) success run 303 (3m9s), dev bakery / build success run 304, dev bread-theme / build success run 305. The workflow defines one job (check) covering lint-pins, fmt, clippy base/gtk/extract, test base/gtk/extract and locked extract build, so the single status context is the whole job. Per-step logs need a login (run pages return 307 unauthenticated), so step results are read from the job status plus my own reruns below. - Reviews API for PR68: empty. No change request outstanding. Last comment 4795 is pass for provider scope conditional on CI; CI is now green on that exact SHA. PR is merged=true, merge_commit_sha 94ea374, base main. Branch protection API needs a token (401); not observable, merge already done. ### ECO-76 defect map (original card vs merged tree) 1. Shutdown hang with a surviving logger clone: PASS. mod.rs BugGuard::drop waits on a done channel for at most JOIN_TIMEOUT (500 ms). Test shutdown_is_bounded_even_while_a_logger_clone_is_alive passed in my rerun. 2. Raised signal returns and survives: PASS. native.rs handler re-raises for si_code <= 0 (lines ~381-409). Probe on 94ea374 (/tmp/opencode/eco-probe, native::install with prior SIG_DFL): raise(SIGSEGV) exit 139, kill(getpid, SIGSEGV) exit 139, synchronous null write exit 139. 3. Prior disposition overwritten / not restored / not chained: PASS. make_resources saves every prior action with checked sigaction; ensure_alt_stack keeps an existing alt stack and unmaps on failure; install rolls back on failure. Probe: prior SIG_IGN + raise survives (exit 0, ignored stays ignored), prior SIG_IGN + synchronous fault dies with SIGSEGV (exit 139). bug_crashes: a_custom_prior_sigsegv_handler_is_still_called_after_our_report and a_failed_setup_leaves_every_signal_action_as_it_was passed. 4. glibc backtrace in the signal handler: PASS. No libc::backtrace in native.rs (only in panic.rs:82, the normal panic path, not a handler). Handler walks ucontext RIP/RBP with write-to-pipe probing (native.rs:248-308), only async-signal-safe calls, no allocation or loader locks. Module docs state the rationale. Loader contention has no dedicated test; the guarantee is by design (no loader/malloc call exists in the handler). 5. Drop-newest and no sustained rate limit: PASS. Queue is crossbeam ArrayQueue (cap 512), full queue evicts oldest only. Tests overflow_drops_the_oldest_lines_and_keeps_the_newest, a_sustained_flood_is_sent_at_the_rate_budget_not_all_at_once passed. 6. Raw module paths, panic text and logs unredacted: PASS. scrub.rs applied to panic message/location, module paths (native install and panic marker) and forwarded logs. Tests forwarded_logs_are_redacted_before_they_reach_the_socket, marker_params_redact_home_user_and_secrets, module_paths_are_redacted_before_they_are_rendered passed. ### ECO-77 defect map - Newest line lost when the queue lock is busy while the queue has room: PASS. Queue::push (mod.rs:~262-280) has no mutex; Condvar coordination lock is consumer-side only. contended_push_drops_nothing_while_under_cap (8 producers x 50 lines, live taker) and producer_is_nonblocking_while_consumer_is_stalled (wait_lock held, push returns under 100 ms, line retained) passed. logs_are_batched_clipped_and_leveled unchanged and passed. ### Commands run on 94ea374 (my own, not copied) - cargo test -p bread-utils --features bread-client --locked --offline bug:: : 38 passed, 0 failed. - cargo test -p bread-utils --features bread-client --locked --offline --test bug_crashes -- --test-threads=1 : 7 passed, 0 ignored. - Probe binary above, 5 modes (results in item 2 and 3). Not rerun: full workspace and gtk/extract suites; relied on exact-SHA green CI on 711e3e8 and on 94ea374 plus the earlier reviewer and coordinator workspace runs. ### Non-blocking residual (not a defect gate failure) Committed tests do not include a raised-signal case or a prior SIG_IGN case; those behaviors are verified only by the probe above. Recommend a follow-up test card (raise(SIGSEGV) dies of signal 11 under default prior; SIG_IGN prior chain; held-loader contention if feasible). Not filed by this pass. ### Verdict ECO-76 and ECO-77 defect acceptance: pass at 711e3e8 / merged 94ea374. Both defect cards' acceptance is genuinely covered; nothing from the original ECO-76/77 text was dropped except the optional extra regressions noted above. Full BREAD-16/BREAD-17 acceptance remains OPEN: CLIP-19 breadclip pilot unmerged, rollout and app-overhead gates unverified, no tag. Board transitions left to the coordinator.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bread-theme

   
breadbug

   
bug

   
ci

   
enhancement

   
phase-2

   
release

   
rollout
No labels
bread-theme
breadbug
bug
ci
enhancement
phase-2
release
rollout
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Breadway/bread-ecosystem!68
No description provided.