Conversation
d3c0bee to
33a2aec
Compare
33a2aec to
94866c5
Compare
HatemMn
changed the title
8ac88da to
b51becd
Compare
GitHub Actions
Error
Manuthor
left a comment
Manuthor
left a comment
There was a problem hiding this comment.
Code review for PR #1190 (feat/audit-sink-generalize against feat/audit-sink-refactor):
The generalization of AuditStore over the async AuditSink trait is clean and sound. Moving recovery and exclusive lock handling into FileSink::resume() preserves the non-blocking startup contract. Below are specific in-scope suggestions.
| ) | ||
| .await; | ||
| } | ||
| Err(e) => { |
There was a problem hiding this comment.
In AuditFileStore::start_with_max_size(), if sink.resume().await returns an error (e.g. if an unforeseen non-recoverable error occurs in a third-party sink), the error is logged and the spawned task terminates. However, any callers who subsequently call enqueue() will have their events queued up to channel capacity and then dropped with channel full warnings. Consider exposing or tracking a sink failure state so enqueue() or health checks can distinguish between a busy channel and a dead writer task.
There was a problem hiding this comment.
Consider exposing or tracking a sink failure state so enqueue() or health checks can distinguish between a busy channel and a dead writer task.
Implementing that function wasn't hard but did you in an indirect manner suggest to modify health.rs by adding the audit as a dependency ? I see that database is already added as dependency
I will push my code to another branch, please tell me this is what your required or not because I am not so sure (without that, the 'addition' will become dead code)
Replace the private, file-only mock AuditSink trait with the public async cosmian_kms_interfaces::AuditSink defined in the previous commit: - FileSink: AuditSink wraps the file backend (lock+recover retry loop moves from writer_supervisor into FileSink::resume(), never returns Err in practice — preserves the always-start/self-heal behavior exactly). - write_failure_is_fatal() = false (preserves the file backend's historical log-and-continue behavior). - Added AuditSink::is_write_capacity_exceeded() (default false) so the generic writer loop can skip a write without an error-per-event log spam; FileSink is the only implementor that ever returns true (max_size_bytes cap). - writer_loop/write_draft_to_chain are now generic over any AuditSink and fully backend-agnostic (no file-specific cap/path knowledge left). - Added AuditEventDraft::finalize() (cosmian_kms_access) to share the draft->AuditEvent+row_hash computation between the async steady-state writer and the sync recovery-time sentinel writer (torn-write/reanchor), which runs inside spawn_blocking with no async runtime available. - AuditFileStore's public API (start/start_with_max_size/enqueue/flush/ new_disconnected) is unchanged; no consumer (middleware, kms/mod.rs, tests) needed changes. All 47 audit tests (access + core::audit + middleware + integration) green, clippy clean, no behavior change observed in file-backend tests.
update_log_index.py --check was failing on this branch: 2 stale entries (superseded by the resume() rewrite) and 4 new entries (from the earlier resume()/store.rs fixes) were undocumented. Not caused by the rebase — pre-existing gap from an earlier direct fix that never ran the doc-sync tool.
…unparsable Caught by the typos pre-commit hook (not installed until now, hence never enforced on this stack). Same misspelling already exists on develop's file_store.rs, unrelated to this rebase. The one other repo occurrence (crate/clients/clap/src/actions/audit.rs) is a static test fixture string, not coupled to this enum — left untouched.
b51becd to
90dce08
Compare
| ) | ||
| .await; | ||
| } | ||
| Err(e) => { |
This branch was successfully deployed
2 participants
Overview
feat/audit-sink-generalizeOne commit. This is where the file backend actually adapts to the new public
AuditSinktrait instead of a private mock one.Small diff but this is the one place old behavior could have silently shifted during the refactor.
Got its own PR instead of getting buried in the split commit before it.