Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: warn-cacao-near-exp #3164

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

AaronGoldman
Copy link
Contributor

log a warning when a cacao is verified with an expiration within 12h of the covering time event.

Description

Include a summary of the change or link to the issue it addresses.

Include relevant motivation, context, brief description and impact of the change(s). List follow-up tasks here.

How Has This Been Tested?

Describe the tests that you ran to verify your changes. Provide instructions for reproduction.

  • Test A (e.g. Test A - New test that ... ran in local, docker, and dev unstable.)
  • Test B

PR checklist

Before submitting this PR, please make sure:

  • I have tagged the relevant reviewers and interested parties
  • I have updated the READMEs of affected packages
  • I have made corresponding changes to the documentation

References:

Please list relevant documentation (e.g. tech specs, articles, related work etc.) relevant to this change, and note if the documentation has been updated.

log a warning when a cacao is verified with an expiration within 12h of the covering time event.
@@ -366,6 +367,12 @@ export class StreamUtils {
logEntry.expirationTime
}. Loading the stream with 'sync: SyncOptions.ALWAYS_SYNC' will restore the stream to a usable state, by discarding the invalid commits (this means losing the data from those invalid writes!)`
)
} else if (logEntry.expirationTime && logEntry.expirationTime - timestamp < 12 * 60 * 60) {
logger.warn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since timestamp can come from the anchor commit time OR from "now" if the stream isn't actually anchored yet, this log message is a bit misleading, since the message implies the stream is already anchored in all cases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anchorTime=${logEntry.timestamp} will be null if there is no anchor but I see what you mean about the wording.

@@ -366,6 +367,12 @@ export class StreamUtils {
logEntry.expirationTime
}. Loading the stream with 'sync: SyncOptions.ALWAYS_SYNC' will restore the stream to a usable state, by discarding the invalid commits (this means losing the data from those invalid writes!)`
)
} else if (logEntry.expirationTime && logEntry.expirationTime - timestamp < 12 * 60 * 60) {
logger.warn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking about this more, I'm actually not sure how useful this log message is, and I'm afraid it might be quite noisy. This is because there's nothing that prevents us from doing a write with a CACAO right before it's about to expire. A standard CACAO session is valid for a week by default, but if the user logs in once, then logs in again 6 days, 23 hours, and 59 minutes later, the app will still let them re-use their old session without signing in again. So the mere fact that the CACAO is close to expiring doesn't actually tell us that the stream took longer than expected to be anchored.

I think this log message really needs to be based on the timestamp of when the write was performed, which we only have in the ComposeDB index database.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we have the 24 hour grace period logging the fact that it is expired tells us basicly the same thing but it is more clear that a client is not refreshing it CACAO sessions aggressively enugh.

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