Repository navigation
fix(notify): prepare private cache file before transaction budget - #332
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (12)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughCache claims now prepare temporary files before starting the locked transaction. Platform cache implementations manage preparation and cleanup, and Windows writes publish through a single-use prepared handle. The documentation and tests reflect the updated timing and failure reporting. ChangesObservation cache claim and publication
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Caller
participant RecentCacheClaim
participant windowsCache
participant PrivateCacheRoot
participant PrivateCacheWrite
Caller->>RecentCacheClaim: Claim(ctx)
RecentCacheClaim->>windowsCache: Prepare(ctx)
windowsCache->>PrivateCacheRoot: PrepareWrite(ctx, name)
PrivateCacheRoot-->>windowsCache: prepared write
RecentCacheClaim->>windowsCache: Write(data)
windowsCache->>PrivateCacheWrite: WriteContext(ctx, data)
PrivateCacheWrite-->>windowsCache: publication result
windowsCache-->>RecentCacheClaim: write result
RecentCacheClaim-->>Caller: claim result
Merge Risk: ⚪ Minimal · up to The change moves private cache file preparation ahead of the 250 ms transaction budget, so total claim time can exceed that budget. No merge-blocking issue was identified. Normal current-head CI should still finish before merge. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected changes preserve fail-closed admission, serialized attempt tracking, and private-file publication controls. Preparation now occurs before the transaction timeout, so complete claims can take longer and duplicates also require temporary-file allocation. No introduced security defect was established, but incomplete failure and interference coverage leaves bounded uncertainty. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 10 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 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. I’m a rabbit with a cache to tend, Comment |
|
Independent source review accepted exact d255ef3 against 9fb6035 with no actionable findings. Internal code matches accepted and native-tested 10806b8 exactly. The internal diff SHA256 is 95821a099ad9d07d82304642f0f85824974ed1ad87fe063e9de9f44e4bc5ee1d. Documentation preserves recorder limits and explicitly defines preflight, inherited caller deadlines, the 250ms transaction budget and eager duplicate preparation. Native Windows Go 1.25/1.26 canary 37195341892 passed on the first attempt. Normal exact-head CI is pending; this source acceptance does not claim faster I/O or a known driver cause. |
Retain bounded named GeneralProfile/FileIO/Minifilter recordings from original Windows qualification failures without changing strict outcomes. Independent review and all 12 current-head checks passed; native event/stack decoding verified recorder coverage. Preserve original head ancestry for dependent PR #332. This records evidence and does not claim an availability cure. Refs #320
Preserve the concurrent Claude branding changes and qualify their composition with the notification cache fix. Refs #320
|
Independent read-only composition review accepted exact head eab71f7 against main 8d5f814. No blocking findings. The previously reviewed internal fix is unchanged (diff SHA256 95821a099ad9d07d82304642f0f85824974ed1ad87fe063e9de9f44e4bc5ee1d). The concurrent Claude public-label changes do not alter cache identity, deadlines, admission or cleanup. Transaction budget, inherited caller context, private ACL/root pinning, locking, rename, checked close and cleanup contracts remain intact. Current exact-head CI must pass before merge. |
|
Independent read-only review accepted merge composition: head eab71f7, current main 75b7acd, conflict-free merge tree 3fa9bc826cc9e1c4e13f7876e7cc88b5749f830b. Differences from the accepted and CI-qualified head are limited to 14 separately merged landing/workflow/PNG paths from #333 and #334. Go production code, tests, dependencies, scripts and Go qualification workflows are byte-identical. No integration blocker; no repeat of unaffected Go checks is needed. All pending exact-head checks must still pass before merge. Final Windows Go 1.25 and 1.26 full jobs passed on attempt 1 in run 37197800947. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #332 +/- ##
==========================================
+ Coverage 65.94% 65.96% +0.01%
==========================================
Files 264 264
Lines 29929 29956 +27
==========================================
+ Hits 19737 19759 +22
- Misses 7240 7241 +1
- Partials 2952 2956 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Released in v1.47.1, an opt-in Linux/Windows prerelease. Draft and public package checks passed on Linux amd64/arm64 and Windows amd64. macOS is skipped; latest stable and automatic updates remain on v1.46.1. |
Windows first valid claims can fail because private temporary-file creation alone exceeds the 250ms transaction allowance. Original failed traces measured 288-791ms in NtCreateFile, including 537.766ms after the publication guard merged.
Prepare one empty private temporary file before starting the existing 250ms locked read/publication context, as root preflight already precedes that context. The original caller context still governs the complete claim and is checked before and after allocation; complete claim duration can exceed 250ms. This is an explicit budget-boundary change selected under the owner's delegated decision, not a claim that Windows I/O became faster.
All history and clock decisions remain under the permanent lock. Windows revalidates its pinned root and the replacement target during publication. Initial protected private ACL, atomic rename, checked close before admission, pre-rename deadline guard, post-publication check, fixed 60-second window and no automatic retry remain intact. Unused preparations are removed on every early exit. Eager preparation also makes duplicate lookup require a writable private root; allocation failure still denies admission. Existing standalone runtime writes remain thin prepare/publish wrappers.
Validation: Independent source and composition review accepted final eab71f7 against current main 8d5f814. The internal fix diff remains unchanged after integrating the concurrent Claude-label PR #331. Internal code is byte-identical to accepted 10806b8. Four focused macOS scenarios pass. Real-file tests cover canceled/expired preflight, document preservation, duplicate cleanup, Windows ACL changes after preparation and one-shot publication. The held-lock gate still requires lock/deadline denial, and bounds transaction BudgetElapsed rather than total preflight+claim time. Native Windows Go 1.25/1.26 qualification passed on the first attempt, including the complete shared cache/Gemini packages, private-file lifecycle and ACL contracts, and 20 unchanged strict consent repetitions.
https://github.com/777genius/agent-notifications/actions/runs/37195341892
#330 is merged and its original-attempt OS recorder accompanies normal final-head qualification. This PR now targets current main and preserves #331. Independent composition review also accepted merge tree 3fa9bc826cc9e1c4e13f7876e7cc88b5749f830b against main 75b7acd: the later #333/#334 changes are limited to landing/workflow/PNG files, with all Go code and qualification paths byte-identical to the CI head. All 25 final-head checks passed, including full Windows Go 1.25 and 1.26 on attempt 1. Passing controls do not prove faster syscalls or identify the driver.
Refs #320