fix(ui): report session persistence failures on shutdown - #338
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe storage writer now orders and coalesces session snapshots, tracks durable revisions, and reports unpersisted work during shutdown. The event loop propagates shutdown failures. Session deletion cleans orphaned metadata even when the primary log is missing. ChangesStorage persistence and shutdown
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RunLoop
participant EventLoop
participant StorageWriter
participant SessionStorage
RunLoop->>EventLoop: request shutdown
EventLoop->>StorageWriter: shut down with timeout
StorageWriter->>SessionStorage: persist generation-ordered snapshots
SessionStorage-->>StorageWriter: return durable or failed revisions
StorageWriter-->>EventLoop: return shutdown result
EventLoop-->>RunLoop: return shutdown report or preserve fatal error
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Criterion
Details
| Benchmark suite | Current: 8209b65 | Previous: d8320fa | Ratio |
|---|---|---|---|
fib/jit_mlua_hook |
5945728 ns/iter (± 44442) |
6479305 ns/iter (± 173328) |
0.92 |
fib/jit_watchdog |
1307811 ns/iter (± 42528) |
2444447 ns/iter (± 5352) |
0.54 |
fib/jit_none |
1307155 ns/iter (± 5668) |
2445376 ns/iter (± 16668) |
0.53 |
fib/interp_mlua_hook |
6642675 ns/iter (± 306313) |
7742986 ns/iter (± 15157) |
0.86 |
fib/interp_watchdog |
2422842 ns/iter (± 5662) |
3838489 ns/iter (± 134868) |
0.63 |
fib/interp_none |
2423091 ns/iter (± 5274) |
3769618 ns/iter (± 7737) |
0.64 |
buffer_rw/jit_mlua_hook |
670979 ns/iter (± 1023) |
553729 ns/iter (± 1099) |
1.21 |
buffer_rw/jit_watchdog |
75559 ns/iter (± 1616) |
167842 ns/iter (± 261) |
0.45 |
buffer_rw/jit_none |
75446 ns/iter (± 272) |
167775 ns/iter (± 226) |
0.45 |
buffer_rw/interp_mlua_hook |
903220 ns/iter (± 27614) |
1062421 ns/iter (± 11617) |
0.85 |
buffer_rw/interp_watchdog |
401456 ns/iter (± 10324) |
615755 ns/iter (± 7100) |
0.65 |
buffer_rw/interp_none |
401442 ns/iter (± 17607) |
615403 ns/iter (± 8963) |
0.65 |
splash_render_120x40 |
47024 ns/iter (± 942) |
56952 ns/iter (± 4752) |
0.83 |
splash_render_200x60 |
80730 ns/iter (± 7178) |
184924 ns/iter (± 3492) |
0.44 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@n00n-ui/src/app/mod.rs`:
- Line 221: Update the writer shutdown handling around wait_for_shutdown to
capture its typed error instead of converting it directly to false. Before
retaining the state directory, log the failure with an explicitly named fallback
and sanitized structured error details, while preserving the existing successful
shutdown behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 38dedf61-224d-42e6-ac75-37d9afced522
📒 Files selected for processing (5)
changelog.d/storage-shutdown-persistence.fixed.mdn00n-ui/src/app/mod.rsn00n-ui/src/app/tests.rsn00n-ui/src/event_loop.rsn00n-ui/src/storage_writer.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Do not add unsafe code, FFI, global mutable state,static mut, or unchecked transmute-like behavior without written review, an explicit lint exception, and a SAFETY comment where applicable.
Do not useunwrap,expect,panic!,todo!,unimplemented!, ordbg!in production Rust code; tests are exempt from the unwrap/expect/panic restriction.
Do not silently discard failures withunwrap_or,unwrap_or_default,.ok()onResult, or equivalent defaults; return typed errors, reject the operation, or use an explicitly named fallback with sanitized structured logging.
Use idiomatic Rust, descriptive names, minimal state, and avoid unnecessary comments, bloat, and magic numbers or strings.
Import types at the top of the file and use short imported names; keep constants immediately after imports.
UseResult<T, E>and explicit error handling instead of panics; usethiserrorfor library/domain errors andcolor-eyreat binary edges.
Use#[derive(Copy)]only for structs containing one primitive field.
Prefer structured logging with useful fields and provide helpful, sanitized error messages.
Place unit tests in the same file inside#[cfg(test)]modules; use#[test_case]and snake_case test names.
Propagate typed errors with?,ok_or_else, andmap_err; library crates usethiserrorand binaries usecolor-eyre.
Treat LLM and provider output as untrusted input; validate schemas, domain constraints, and source evidence before persistence or action.
Do not log raw provider payloads, prompts, credentials, or user session data, and never commit credentials, API keys, tokens, cookies, or auth headers.
Validate and authorize HTTP, file, queue, configuration/environment, LLM, and provider-callback inputs before mutation or persistence.
Tool execution requires allowlisted tools, scoped credentials, explicit user context, audit events, and refusal or denial tests.
Files:
n00n-ui/src/app/tests.rsn00n-ui/src/event_loop.rsn00n-ui/src/app/mod.rsn00n-ui/src/storage_writer.rs
🧠 Learnings (1)
📚 Learning: 2026-07-31T19:15:04.814Z
Learnt from: w0wl0lxd
Repo: w0wl0lxd/n00n PR: 206
File: changelog.d/orchestration-hardening.fixed.md:1-1
Timestamp: 2026-07-31T19:15:04.814Z
Learning: Files in changelog.d are changelog fragments intended for user-facing release notes and may begin directly with summary prose. Do not flag a missing Markdown H1 or require an H1 solely because Markdownlint MD041 reports it in these fragment files.
Applied to files:
changelog.d/storage-shutdown-persistence.fixed.md
🪛 markdownlint-cli2 (0.23.2)
changelog.d/storage-shutdown-persistence.fixed.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (4)
n00n-ui/src/storage_writer.rs (1)
30-43: LGTM!Also applies to: 67-125, 179-198, 232-261, 454-454, 484-498, 556-585, 642-642, 660-660, 688-688, 702-702
n00n-ui/src/event_loop.rs (1)
65-66: LGTM!Also applies to: 611-620, 1503-1503, 1525-1537
n00n-ui/src/app/tests.rs (1)
2554-2555: LGTM!Also applies to: 2734-2735, 2774-2775, 2837-2838, 2872-2873
changelog.d/storage-shutdown-persistence.fixed.md (1)
1-1: LGTM!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aef7e759e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@n00n-ui/src/storage_writer.rs`:
- Around line 550-556: Update the delete requeue logic around crossing_snapshot
so inserting into pending follows the same higher-revision replacement rule as
stage_snapshot. Preserve an existing pending snapshot when its revision exceeds
the latest_snapshots entry, and replace it only when the requeued snapshot has
the higher revision; keep crossing_target derived from the latest snapshot.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9543dd21-5206-4205-8736-a53d5766daf0
📒 Files selected for processing (3)
n00n-storage/src/sessions.rsn00n-ui/src/app/mod.rsn00n-ui/src/storage_writer.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (14)
- GitHub Check: MSRV (1.97)
- GitHub Check: Coverage
- GitHub Check: Build (Windows)
- GitHub Check: Test (macOS)
- GitHub Check: Rustdoc
- GitHub Check: Lint (Windows)
- GitHub Check: Test (Windows)
- GitHub Check: Lint (macOS)
- GitHub Check: Build
- GitHub Check: Docs
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Analyze (rust)
- GitHub Check: Criterion
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Do not add unsafe code, FFI, global mutable state,static mut, or unchecked transmute-like behavior without written review, an explicit lint exception, and a SAFETY comment where applicable.
Do not useunwrap,expect,panic!,todo!,unimplemented!, ordbg!in production Rust code; tests are exempt from the unwrap/expect/panic restriction.
Do not silently discard failures withunwrap_or,unwrap_or_default,.ok()onResult, or equivalent defaults; return typed errors, reject the operation, or use an explicitly named fallback with sanitized structured logging.
Use idiomatic Rust, descriptive names, minimal state, and avoid unnecessary comments, bloat, and magic numbers or strings.
Import types at the top of the file and use short imported names; keep constants immediately after imports.
UseResult<T, E>and explicit error handling instead of panics; usethiserrorfor library/domain errors andcolor-eyreat binary edges.
Use#[derive(Copy)]only for structs containing one primitive field.
Prefer structured logging with useful fields and provide helpful, sanitized error messages.
Place unit tests in the same file inside#[cfg(test)]modules; use#[test_case]and snake_case test names.
Propagate typed errors with?,ok_or_else, andmap_err; library crates usethiserrorand binaries usecolor-eyre.
Treat LLM and provider output as untrusted input; validate schemas, domain constraints, and source evidence before persistence or action.
Do not log raw provider payloads, prompts, credentials, or user session data, and never commit credentials, API keys, tokens, cookies, or auth headers.
Validate and authorize HTTP, file, queue, configuration/environment, LLM, and provider-callback inputs before mutation or persistence.
Tool execution requires allowlisted tools, scoped credentials, explicit user context, audit events, and refusal or denial tests.
Files:
n00n-ui/src/app/mod.rsn00n-storage/src/sessions.rsn00n-ui/src/storage_writer.rs
🔇 Additional comments (4)
n00n-ui/src/app/mod.rs (1)
221-227: LGTM!n00n-storage/src/sessions.rs (2)
3286-3297: LGTM!
4746-4769: LGTM!n00n-ui/src/storage_writer.rs (1)
3-157: LGTM!Also applies to: 169-242, 251-338, 341-465, 467-519, 521-549, 557-578, 581-653, 655-722, 736-780, 807-858, 870-1057, 1059-1253
1 participant
Summary
Root cause
When the operation channel disconnected, the writer performed one final flush but discarded its failure map and always signaled successful completion. Snapshots previously dropped after retry exhaustion were also forgotten, so graceful shutdown could claim success after losing the newest session state.
Validation
cargo fmt --all -- --checkgit diff --checkcargo clippy -p n00n-ui --tests -- -D warningscargo nextest run -p n00n-ui(1302 passed, 1 skipped)