You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
CCF's stderr does not consistently indicate fatal node failures, making fatal diagnostics difficult to capture separately from ordinary logs. Closes#8623
Implementation summary
Adds a shared, flushed fatal-diagnostic writer for CCF-controlled fatal paths across startup, join, and steady-state operation.
Recoverable logs and fatal-severity-only entries remain on stdout, and stderr diagnostics for join failures exclude response bodies and snapshot exception details.
Documents that direct application/dependency writes and runtime-generated diagnostics are outside the contract.
Safety and compatibility
No consensus, KV, wire-format, or application API changes.
The stderr diagnostics avoid join response bodies and snapshot exception details; existing detailed stdout logs are preserved.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The tests bypass the production console sink and omit aborting worker exceptions, leaving stream routing, stderr diagnostics, and SIGABRT termination unprotected against regressions.
Every report_fatal_error() call is already serialised by NodeState::lock and RuntimeControlImpl's mutex. Other write_fatal_diagnostic() callers can run concurrently, so each diagnostic is now emitted as a single stream insertion, which stdio's stream lock keeps whole. Add a regression test that writes from several threads to the real stderr and checks each line is intact.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db72aaa070234686ab3bcc336e728baf
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The new diagnostic mislabels all 4xx responses as “Bad Request,” while regression tests do not verify required stderr output or response-body redaction.
Verify join rejection diagnostics and response-body handling
src/node/node_state.h:1005
The join-rejection tests inspect stdout but discard the stderr validation result in tests/infra/network.py:1429, so removing this diagnostic or restoring the response body would still pass. Add a regression case with a distinctive response-body marker that asserts the fatal diagnostic is present on stderr, the marker is absent there, and the detailed stdout message remains available.
Report the actual HTTP status instead of always saying Bad Request
src/node/node_state.h:1005
This handler receives every 4xx response, not just HTTP 400. The new stderr diagnostic therefore describes responses such as 401 or 404 as "Bad Request", giving operators an incorrect failure reason. Use ccf::http_status_str(status), as handle_join_unexpected_status_unsafe() already does.
Assert the fatal diagnostic is written to stderr
tests/e2e_operations.py:3137
This suppresses the new stderr diagnostic without asserting that it was emitted. The subsequent check_log_for_error_message() reads only stdout (tests/infra/node.py:1215), so removing the fatal-diagnostic write in src/host/run.cpp:443 would still leave this startup regression test passing. Assert the expected fatal line in stderr before ignoring it during shutdown.
The test exercised libstdc++/glibc behaviour rather than CCF code.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db72aaa070234686ab3bcc336e728baf
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human review is needed because failed-join tests ignore the fatal stderr diagnostic, leaving response-body redaction and diagnostic preservation without regression coverage.
Restore the original frontend LOG_FATAL text, check console loggers only for stdout/stderr routing, and split the aborting task test so the stderr capture needs no manual restore.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db72aaa070234686ab3bcc336e728baf
- Report the actual HTTP status text in the join failure diagnostic instead of always saying Bad Request.
- Emit a fatal diagnostic before an enclave runtime thread exception escapes and terminates the process.
- Check that rejected joiners write the join diagnostic to stderr without the response body, and stop ignoring join diagnostics on shutdown: rejected joiners are already removed from the network before shutdown checks.
- Capitalise the changelog sentence.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db72aaa070234686ab3bcc336e728baf
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
An unrecognised 4xx response such as 471 aborts in the status-name lookup before fatal-error reporting, leaving stderr empty and bypassing graceful shutdown.
The reworded report_fatal_error() call in db3db9b was not clang-formatted, failing the C/C++ format check.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db72aaa070234686ab3bcc336e728baf
Just to check the counterfactual: why not extend the LOG_FAIL_FMT, and LOG_FATAL_FMT to write to stderr? and then use them consistently?
It would be just FATAL, rather than FAIL, which can happen more than once and which we should not log out to stderr. I don't have a strong reason not to do it, but I also don't see a strong reason to.
I envision stderr messages as shorter, higher guarantee-of-being-written-out strings, whereas LOG_FATAL_FMT can be more detailed, but goes through a more complicated sub-system with things like timestamps, configuration, and therefore higher chance of failure. Maybe it would be useful to draw a starker distinction and ensure stderr writing is always allocation-free.
But to recap, I think stderr writes should always, always happen on unprompted shutdown, and only then. Having them be a separate, as simple as possible path is attractive?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
CCF's stderr does not consistently indicate fatal node failures, making fatal diagnostics difficult to capture separately from ordinary logs.
Closes #8623
Implementation summary
Adds a shared, flushed fatal-diagnostic writer for CCF-controlled fatal paths across startup, join, and steady-state operation.
Recoverable logs and fatal-severity-only entries remain on stdout, and stderr diagnostics for join failures exclude response bodies and snapshot exception details.
Documents that direct application/dependency writes and runtime-generated diagnostics are outside the contract.
Safety and compatibility
No consensus, KV, wire-format, or application API changes.
The stderr diagnostics avoid join response bodies and snapshot exception details; existing detailed stdout logs are preserved.