Audit of warn! and error! by JasonAtClockwork · Pull Request #5640 · clockworklabs/SpacetimeDB

JasonAtClockwork · 2026-07-31T20:37:50Z

Description of Changes

Reviewed all warn! and error! lines for host side issues to get ready for alerting.

API and ABI breaking changes

No breaking changes.

Expected complexity level and risk

1 - Only changes to logging

Testing

Ran cargo test against impacted crates

JasonAtClockwork · 2026-07-31T22:56:22Z

+            // TODO: May need to split this from module errors vs host errors
+            log::warn!("Internal error while invoking procedure {procedure}: {e:#}");
+            StatusCode::INTERNAL_SERVER_ERROR


Could we preserve whether this failure came from module code or the host? Module errors should remain low-level and eventually go to module logs, while internal host failures may need to stay at error! for alerting.

Created #5660 . For now, let's make this info.

JasonAtClockwork · 2026-07-31T22:58:10Z

            if ws_version == WsVersion::V1
                && let MessageHandleError::Execution(err) = e
            {
+                // TODO: Review log level after guest/client execution errors can be distinguished from internal failures.
                log::error!("{err:#}");
                // If the send task has exited, also exit this recv task.
                if unordered_tx.send(err.into()).is_err() {


This looks to combine module errors with possible host errors, that leads me to lean error! for now

If we can't distinguish, I think we should prefer warn for now. I don't have context on where this error comes from or what we'd be splitting up; can you create a follow-up ticket under the "Alerting" header and mark it P2?

JasonAtClockwork · 2026-07-31T22:58:50Z

        .map_err(|e| {
+            // TODO: Review log level after user SQL errors can be distinguished from internal database failures.
            log::warn!("{e}");
            (StatusCode::BAD_REQUEST, e.to_string())


Same as other areas we might want to split this up so we can use error! if there is a database failure.

Can you create a follow-up ticket?

JasonAtClockwork · 2026-07-31T22:59:39Z

+// TODO: Review each caller and split this helper by the appropriate log severity.
 pub fn log_and_500(e: impl std::fmt::Display) -> ErrorResponse {
    log::error!("internal error: {e:#}");
    (StatusCode::INTERNAL_SERVER_ERROR, format!("{e:#}")).into()


This seems to be very broadly used which makes it hard to know if it should be reduced from error! my gut reaction is to change it to warn! and slowly clean up it's use?

Please change to warn for now, and any place you're already editing that has a log_and_500 directly next to its own more specific log line, remove the log_and_500 call and replace with directly returning the error response. You may want to move the error response into a new helper function.

JasonAtClockwork · 2026-07-31T23:14:31Z

+            // TODO: Review log level after configured issuers can be distinguished from arbitrary token issuers.
            log::warn!("Error fetching public key for issuer {raw_issuer}: {e:?}");


This one I'm not 100% sure if we need to split up or count everything as warn!

I have no clue. At this stage I'm unconcerned.

JasonAtClockwork · 2026-07-31T23:15:12Z

+                // TODO: Review log level after migration/user errors can be distinguished from internal database failures.
                log::warn!("Database update failed: {} @ {}", e, stdb.database_identity());
                system_logger.warn(&format!("Database update failed: {e}"));
                let (_, tx_metrics, reducer) = stdb.rollback_mut_tx(tx);


Another spot where it looks like real failures can be mixed in with user issues.

Make a follow-up?

JasonAtClockwork · 2026-07-31T23:15:37Z

+                        // TODO: Review log level after guest view errors can be distinguished from materialization failures.
                        log::error!("Error materializing view `{view_name}`: {err:?}");
                        ViewOutcome::Failed(format!("Error materializing view `{view_name}`: {err}"))


This one looked similar as above but I'm not certain if it's true that it can be mixed here.

Another follow-up ticket? And, can @joshua-spacetime weigh in as to whether user errors can happen here, which I assume would be if a view function panics.

bfops · 2026-08-03T08:02:38Z

FYI #5320 is about to merge which tweaks some logs as well

Signed-off-by: Jason Larabie <jason@clockworklabs.io>

gefjon · 2026-08-04T16:26:50Z

+            // TODO: May need to split this from module errors vs host errors
+            log::warn!("Internal error while invoking procedure {procedure}: {e:#}");
+            StatusCode::INTERNAL_SERVER_ERROR


Created #5660 . For now, let's make this info.

gefjon · 2026-08-04T16:28:30Z

            if ws_version == WsVersion::V1
                && let MessageHandleError::Execution(err) = e
            {
+                // TODO: Review log level after guest/client execution errors can be distinguished from internal failures.
                log::error!("{err:#}");
                // If the send task has exited, also exit this recv task.
                if unordered_tx.send(err.into()).is_err() {


If we can't distinguish, I think we should prefer warn for now. I don't have context on where this error comes from or what we'd be splitting up; can you create a follow-up ticket under the "Alerting" header and mark it P2?

gefjon · 2026-08-04T16:29:33Z

        .map_err(|e| {
+            // TODO: Review log level after user SQL errors can be distinguished from internal database failures.
            log::warn!("{e}");
            (StatusCode::BAD_REQUEST, e.to_string())


Can you create a follow-up ticket?

gefjon · 2026-08-04T16:31:06Z

+// TODO: Review each caller and split this helper by the appropriate log severity.
 pub fn log_and_500(e: impl std::fmt::Display) -> ErrorResponse {
    log::error!("internal error: {e:#}");
    (StatusCode::INTERNAL_SERVER_ERROR, format!("{e:#}")).into()


Please change to warn for now, and any place you're already editing that has a log_and_500 directly next to its own more specific log line, remove the log_and_500 call and replace with directly returning the error response. You may want to move the error response into a new helper function.

gefjon · 2026-08-04T16:31:57Z

+            // TODO: Review log level after configured issuers can be distinguished from arbitrary token issuers.
            log::warn!("Error fetching public key for issuer {raw_issuer}: {e:?}");


I have no clue. At this stage I'm unconcerned.

gefjon · 2026-08-04T16:42:53Z

            Some(ptr) => self.delete(ST_CLIENT_ID, ptr).map(drop)?,
            _ => {
-                log::error!(
+                log::warn!(


@joshua-spacetime how can this (and the other previously error! logs in this file) happen, and how bad is it?

gefjon · 2026-08-04T16:43:29Z

            }
            Err(e) => {
+                // TODO: Review log level after retryable cleanup errors can be distinguished from internal database failures.
                log::error!(


What user errors can get mixed in here?

gefjon · 2026-08-04T16:44:03Z

                    let res = self
                        .maybe_take_snapshot(database_state.as_ref())
                        .await
+                        // TODO: Review log level after lifecycle errors can be distinguished from snapshot creation failures.


Please create a follow-up ticket.

gefjon · 2026-08-04T16:45:01Z

+            // TODO: Review log level after client SQL errors can be distinguished from internal database failures.
            log::error!("PG: Error for database {database}: {err}");


Downgrade to warn, at least until the TODO is done. Please make a follow-up ticket, but mark it P3.

gefjon · 2026-08-04T16:45:30Z

+                        // TODO: Do not log the supplied password/token; then classify credential errors separately from provider failures.
                        log::error!(


Please downgrade to warn, and make a P3 follow-up, as above.

JasonAtClockwork added 2 commits July 31, 2026 11:24

Audit of warn! and error!

205216b

Areas of concern for alerts

724a927

JasonAtClockwork commented Jul 31, 2026

View reviewed changes

JasonAtClockwork marked this pull request as ready for review July 31, 2026 23:19

Merge branch 'master' into jlarabie/audit-logs

faad664

Signed-off-by: Jason Larabie <jason@clockworklabs.io>

JasonAtClockwork requested review from gefjon and joshua-spacetime August 4, 2026 14:58

gefjon mentioned this pull request Aug 4, 2026

ProcedureCallError: preserve distinction between guest and host errors #5660

Open

gefjon reviewed Aug 4, 2026

View reviewed changes

		// TODO: Review log level after configured issuers can be distinguished from arbitrary token issuers.
		log::warn!("Error fetching public key for issuer {raw_issuer}: {e:?}");

		// TODO: Review log level after client SQL errors can be distinguished from internal database failures.
		log::error!("PG: Error for database {database}: {err}");

		// TODO: Do not log the supplied password/token; then classify credential errors separately from provider failures.
		log::error!(

Uh oh!

Conversation

JasonAtClockwork commented Jul 31, 2026

Description of Changes

API and ABI breaking changes

Expected complexity level and risk

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bfops commented Aug 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants