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

Several sources of truth about contest start #178

Open
kunyavskiy opened this issue Dec 9, 2024 · 12 comments
Open

Several sources of truth about contest start #178

kunyavskiy opened this issue Dec 9, 2024 · 12 comments

Comments

@kunyavskiy
Copy link

There are currently two ways of implementing check if the contest has started:

  • You can check if start_time reported by contest endpoint already passed
  • You can check if started in state is not null.

It's unclear what it means if they are inconsistent, e.g. :

  • state is still null, but start_time already passed
  • state shows some point in the future
  • both are not null but show different points in the past.

Maybe it would be better to unify it into being part of the only the state (but not contest). We can also consider moving hold_time.

In that case there would be two separated concepts:

  • contest is the description of permanent contest properties (length, freeze length, rules, names, etc.)
  • state is the description of changing properties (start_time can be rescheduled, held time is temporary, etc.)

The may disadvantage I see is that some clients can now need both state and contest, while before only the contest would be enough.

@kunyavskiy
Copy link
Author

Why it's a problem:
during nov/dec 24 we have several streams on different testing systems, which didn't deliver state for some reason. Having the single source of truth would help to make such bugs more obvious to both clients and ccs implementors.

@niemela
Copy link
Member

niemela commented Dec 10, 2024

According to the spec contests.start_time is the "scheduled" start time, and state.started is when the contest "actually started". That said, the spec also says that when set state.started must be equal to contests.start_time (i.e. when you start the contest you implicitly schedule it to start now).

This means the following:

  • if state.started is null and contests.start_time is in the future, then the contest has not started (as expected because the scheduled start time is in the future).
  • if state.started is null and contests.start_time is in the past, then the contest has not started, but the scheduled start time has not yet been updated.
  • if state.started is not null and equal to contests.start_time in the past, then the contest has started at that time (This is normal.)
  • if state.started is not null and equal to contests.start_time in the future, then the upstream system is not compliant. There is no reasonable interpretation of this (but the future will become the past eventually...)
  • if state.started is not null and different from contests.start_time, then the upstream system is not compliant, state.started is the time the contest started not contests.start_time.

In other words, state.started is always the source of truth for when and if the contest started (but if it is, then it must be equal to contests.start_time).

What you describe sounds like bugs in the systems. I suggest we close this issue as there is IMO nothing to fix in the spec.

@eldering
Copy link
Collaborator

eldering commented Dec 10, 2024

  • if state.started is null and contests.start_time is in the past, then the contest has not started, but the scheduled start time has not yet been updated.

I think this is/should be actually a bug: once contest.start_time has passed, then the contest should be considered to have started and therefore state.started must be set and equal to it.

In other words, state.started is always the source of truth for when and if the contest started (but if it is, then it must be equal to contests.start_time).

The only practical issue I can see is that for a brief amount of time after contest start there's a discrepancy, or the event feed delivering the state change event is behind. But this should not take more than milliseconds normally.

What you describe sounds like bugs in the systems. I suggest we close this issue as there is IMO nothing to fix in the spec.

I agree. Just wondering if we should say anything about state.start_time being authoritative and e.g. maximum delay in event delivery that servers should comply to.

@niemela
Copy link
Member

niemela commented Dec 10, 2024

  • if state.started is null and contests.start_time is in the past, then the contest has not started, but the scheduled start time has not yet been updated.

I think this is/should be actually a bug: once contest.start_time has passed, then the contest should be considered to have started and therefore state.started must be set and equal to it.

I disagree that this is a bug as the spec is currently written. contest.start_time is clearly only the "scheduled" start time, and state.started is clearly the only actual start time.

Maybe an argument could be made that we should specify that the "scheduled" start time can't be set in the past? This would imply that when the current time reaches contests.start_time a system must either set state.started to the same value as contests.start_time (i.e. "start") or set contests.start_time to null or some future time (i.e. explicitly "not start").

Just wondering if we should say anything about state.start_time being authoritative

Agreed, that is bot a bad idea to clarify.

and e.g. maximum delay in event delivery that servers should comply to.

I definitely think we should not go there. Event delivery really can't be controlled by the server (at all).

@nickygerritsen
Copy link
Collaborator

  • if state.started is null and contests.start_time is in the past, then the contest has not started, but the scheduled start time has not yet been updated.

I think this is/should be actually a bug: once contest.start_time has passed, then the contest should be considered to have started and therefore state.started must be set and equal to it.

I disagree that this is a bug as the spec is currently written. contest.start_time is clearly only the "scheduled" start time, and state.started is clearly the only actual start time.

But we do say:

may be null if the start time is unknown or the countdown is paused.

I know it says may, but I find this a bit inconsistent. But we could easily clarify this.

@kunyavskiy
Copy link
Author

The main reason I think it's worth fixing on the spec level is the general principle of making less illegal state representable.

Even if we say that such states are a bug in ccs, the client still needs to be able to handle them because non-atomic updates between different endpoints are non-atomic, so you can observe such states.

Another problem is that if there is a ccs bug, it's very hard for the client's end-user to understand, especially if they run into such a problem the first time, because some random parts, which don't depend on "if the contest is running," are still working.

So I would say, while the specification is consistent and unambiguous here, maybe we still should change it, to make both server and client less error prone.

@niemela
Copy link
Member

niemela commented Dec 10, 2024

I know it says may, but I find this a bit inconsistent. But we could easily clarify this.

It really is not "inconsistent". Inconsistent would imply that it's overspecified, and it's possibly a bit underspecified. Specifically, if the contest does not actually start when scheduled, the current spec allows for either null or the old scheduled time. The advantage of the current (arguably maybe unspecified) requirements is that it is possible to implement. If we fix this by requiring that contest.start_time is reset immediately, that would be much harder to implement...

@niemela
Copy link
Member

niemela commented Dec 10, 2024

So I would say, while the specification is consistent and unambiguous here, maybe we still should change it, to make both server and client less error prone.

What change are you suggesting?

@kunyavskiy
Copy link
Author

What change are you suggesting?

Move contest.start_time and contest.countdown_pause_time into state, to make updating it atomic operation. This would make it hard to forget the state, as there is no other source of information.

It would still force some clients to deal with inconsistencies (e.g. timer should probably start contest on scheduled time optimistically, even without state event, as it just looks better). But in much more predictable way.

@niemela
Copy link
Member

niemela commented Dec 10, 2024

Move contest.start_time and contest.countdown_pause_time into state, to make updating it atomic operation. This would make it hard to forget the state, as there is no other source of information.

I agree that it would have the benefit you claim.

I really don't like it. It will break our categorization of data. /contests is a configuration endpoint, and /state is a live data endpoint.

I would much rather make it much more clear that /state is the authoritative source for when the contest started. Somewhat counterintuitive, maybe it would be more clear if we removed the requirement that contest.start_time is set to state.started?

It would still force some clients to deal with inconsistencies (e.g. timer should probably start contest on scheduled time optimistically, even without state event, as it just looks better). But in much more predictable way.

How is that an inconsistency?

(I feel there is a lot of misuse of the term "inconsistent" in this thread)

@deboer-tim
Copy link
Member

FWIW, I think the spec is fine in what it actually enforces today and I don't think we should remove or change the requirements of either field. The distinction between configuration and state is an important one. But I do think we could improve the descriptions to make it clearer for both implementers and clients.

From previous discussions, the intent was that contest.start_time is when the contest is scheduled to start, whereas state is, well, the state of when it actually did. A contest might be scheduled to start at 9:00 and then actually start at 9:01, might not have a formal scheduled start time, might be started when someone hits a button, or whatever. When it does start you set the state, and then the requirement is to update the contest.start_time to match is really to avoid future confusion more than anything else.

When you look at it this way it's easy to see that something like a schedule or countdown should be based on contest.start_time, but a contest clock should use state. I think that actually makes sense, but could be explained better.

@kunyavskiy
Copy link
Author

Ah, I see.

That division of data (configurable vs unconfigurable) also makes sense. The one I was mostly thinking about is "can/can't reasonably change after contest was set up". Let's argree on just clarifying some parts suggested above, without changing required semantics.

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

No branches or pull requests

5 participants