Skip to content

fix(harness): report event log write failures - #542

Merged
milind-soni merged 1 commit into
milind-soni:mainfrom
rahul-vanyar:fix/event-log-write-failures
Aug 28, 2026
Merged

fix(harness): report event log write failures#542
milind-soni merged 1 commit into
milind-soni:mainfrom
rahul-vanyar:fix/event-log-write-failures

Conversation

@rahul-vanyar

@rahul-vanyar rahul-vanyar commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep live event delivery working when the canonical NDJSON log cannot be written
  • emit one visible runtime.error per affected thread until logging recovers
  • persist an incomplete-history marker before the first recovered event without recursively publishing the warning

Validation

  • pnpm typecheck
  • pnpm exec vitest run server/harness/bus.test.ts (8 passed)
  • affected server-start suites plus bus: 161 passed, 1 skipped
  • git diff --check
  • full pnpm test reaches 2,273 counted tests; its only remaining local failure is server/drivers/codex-catalog.test.ts, reproduced unchanged on upstream/main

Summary by CodeRabbit

  • Bug Fixes
    • Added clear runtime warnings when event history cannot be written.
    • Ensured subscribers continue receiving live events during logging failures.
    • Recorded a single warning when logging recovers, before subsequent events are saved.
    • Prevented duplicate history-incomplete warnings for the same thread.

@vercel

vercel Bot commented Aug 28, 2026

Copy link
Copy Markdown

@rahul-vanyar is attempting to deploy a commit to the SupaMaus Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3707267c-d179-4f3e-9e5f-4b0b28673c9f

📥 Commits

Reviewing files that changed from the base of the PR and between 7983055 and 44abb43.

📒 Files selected for processing (2)
  • server/harness/bus.test.ts
  • server/harness/bus.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

EventBus now reports incomplete canonical logs through one runtime.error event per thread and persists the warning before the first event written after recovery. Tests inject logger failures and verify delivery and recovery behavior.

Changes

Event log recovery

Layer / File(s) Summary
Publish failure and recovery handling
server/harness/bus.ts
EventBus accepts an injectable appendFileSync implementation. publish tracks pending per-thread warnings, delivers one runtime.error event for a logging outage, and persists the warning before the next successful event.
Failure and recovery test coverage
server/harness/bus.test.ts
Tests verify incomplete-log reporting, continued live-event delivery, and persistence of the warning before the first recovered event.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🟡 Moderate · up to 44abb

Live event delivery continues during log-write failures, but recovery can lose its incomplete-history marker after a restart, mishandle partial writes, or persist unsanitized identity metadata. These bounded reliability and security risks should be fixed or explicitly accepted before merging.

Sequence Diagram(s)

sequenceDiagram
  participant Publisher
  participant EventBus
  participant appendFileSync
  participant Subscriber
  Publisher->>EventBus: publish event
  EventBus->>appendFileSync: write redacted event
  appendFileSync-->>EventBus: write failure
  EventBus->>Subscriber: deliver runtime.error warning
  Publisher->>EventBus: publish recovered event
  EventBus->>appendFileSync: write pending warning and recovered event
  EventBus->>Subscriber: deliver recovered event
Loading

Suggested reviewers: milind-soni, aivsomkar

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: reporting event log write failures in the harness.
Description check ✅ Passed The description clearly states the behavior changes and verification results. It uses non-template headings and omits the checklist, but it provides the main required information, and screenshots are …
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description clearly states the behavior changes and verification results. It uses non-template headings and omits the checklist, but it provides the main required information, and screenshots are not applicable because this is not a UI change.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files.

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@milind-soni
milind-soni merged commit 5bd79bc into milind-soni:main Aug 28, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants