syncthing feat(model): announce the state of a folder becoming out of space (fixes #9962)

Purpose

Describe the purpose of this change. If there is an existing issue that is resolved by this pull request, ensure that the commit subject is on the form Some short description (fixes #1234) where 1234 is the issue number.

Testing

Describe what testing has been done, and how the reviewer can test the change if new tests are not included.

Screenshots

If this is a GUI change, include screenshots of the change. If not, please feel free to just delete this section.

Documentation

If this is a user visible change (including API and protocol changes), add a link here to the corresponding pull request on https://github.com/syncthing/docs or describe the documentation changes necessary.

Authorship

Your name and email will be added automatically to the AUTHORS file based on the commit metadata.

May 23 '25 18:05 hazemKrimi

I don't think we currently trigger sending a cluster config when the folder state changes. However if we do send this information, we need such a hook. Otherwise we only ever send that info if we happen to send a cluster config for any other reason. That means most of the time the information will not be accurate/out-of-date.

May 24 '25 20:05 imsodin

@imsodin where do I need to add a hook to update the cluster config whenever a local state is changed?

May 31 '25 01:05 hazemKrimi

@imsodin where do I need to add a hook to update the cluster config whenever a local state is changed?

There's already an event generated for it in the folder stateTracker, I'd probably act on that in model.serve. Or potentially also don't pretend we need to decouple that much here, and instead add an explicit (internal) method to trigger a cluster config in model. Could be just a literal method that's called from folder (resp its stateTracker), it already holds a reference to model for other purposes.

"Short" version on overall design (original, much longer version at the end):

Imo it's worth to get consensus on the overall/protocol design before investing much more time into implementation:

To be consistent in user notification, I'd like to have an indication of folder being stopped/unhealthy/errored regardless of cause. I believe that's best done with an enum based approach, as calmh already brought up in a comment on bep.proto I'd just add a catch-all value for stopped/error to always be able to notify the user of problems. And suggest different naming, as the enum signals more than just a reason for a stopped folder. E.g. naming the field state with the enum like this (though I'd prefer something less generic than state (or status) but can't think of anything, maybe an added qualifier like executionState, but can't say that's convincing - naming... :) ):

enum FolderState {
    running (or healthy)
    paused
    stopped_other (or unhealthy and maybe drop _other)
    out_of_space
}

Original post with "a bit" more context/details aka much longer

I do think this feature should first get some higher-level review/consensus on the design, before investing much (more) time into it. Otherwise there's a risk that the effort is spent in wain, as (large) parts may need redoing or otherwise can't be used as is.
Disclaimer: I have a tendency to get quite verbous in such messages, sorry about that (obviously there's no need to match that, brief is anyway usually better). Plus if I bring up related aspects here, that does not mean I am asking you to cover them/"introduce feature creep", it's just to reason about resp. explain my comments.

One aspect is what the intended usage of this information on the receiving device is. The ticket is mainly asking to have a user-visible indication that there's something amiss on the other device, concretely that it ran out of space. However as mentioned on that ticket, that's not necessarily the only use-case: That information could also help with index handling or syncing. However that's a bit more granular than just a boolean: If a folder is stopped because of the folder is out of space, we can probably continue to exchange index info and even request data from that device. However if it's stopped because the folder disappeared, or there's not enough space for the DB, there's no point in doing either. Again I am not saying this PR needs to provide the building blocks for that, but it might inform how the protocol changes are done.

And as mentioned, the state "folder is out of space" isn't special or unique, there are other reasons a folder may be (partially) stopped or unhealthy. And if we want to indicate to the user on a remote that a folder isn't making progress, then imo that's useful info whether the folder is out of space or whether it's any other reason. I am not asking to specifically handle all/more cases, that would be an unreasonable ask/feature creep (plus I have my doubts it would even be worthwhile), more the contrary: Having a general, non-specific way of signalling a folder is stopped/unhealthy. And "folder out of space" may potentially remain as a special case of that. As could any other reason, if in the future we also want to differentiate that in the user indication in the future.

Imo both of these concerns lead to something like calmh already brought up in a comment on bep.proto: Add the new info about a folder being stopped as an enum. That is extensible, as we can add more specific cases if/when we want to address them. And we can add a "catch-all" value, such that we can always provide a user indication that something is amiss and might be inhibiting sync, without the (imo unreasonable, if even feasible) effort of explicitly handing every possible reason. I'd suggest a different naming than calmh, as the enum signals more than just a reason for a stopped folder in this case. E.g. naming the field state with the enum like this (though I'd prefer something less generic than state (or status) but can't think of anything, maybe an added qualifier like executionState, but can't say that's convincing - naming... :) ):

enum FolderState {
    running (or healthy)
    paused
    stopped_other (or unhealthy and maybe drop _other)
    out_of_space
}

Slight downside of the enum approach, resp. something to be careful of in my opinion: Those enum values should be as widely applicable and self-explaining as possible, not just map 1-to-1 to whatever we use inside syncthing. They should at least potentially be usable/applicable to other BEP implementations as well. E.g. if we handled the case of a missing folder marker, the enum value imo should be something like "data unavailable", not "missing folder marker".

We'll also have to keep the paused bool for BEP compatibility, but that's not an issue/simple.

And unimportant explanation of an opinionated detail: I put the generic stopped enum value before the specific out of space, as any future specific values will have to come after (just because proto) - so I'd rather have them consistently after :)

May 31 '25 08:05 imsodin

I agree to using an enum instead of a bunch of booleans. I suggest naming it something like FolderHealthStatus since all the options will be an indication of the health of a specific folder. And the stateTracker.setError should explicitly set the health status depending on the received error.

There's already an event generated for it in the folder stateTracker, I'd probably act on that in model.serve. Or potentially also don't pretend we need to decouple that much here, and instead add an explicit (internal) method to trigger a cluster config in model. Could be just a literal method that's called from folder (resp its stateTracker), it already holds a reference to model for other purposes.

@imsodin do you mean in the above that I should have a separate method in FolderConfiguration that creates a new ClusterConfig? I don't really understand what should be the correct implementation to to send a new config to a remote device in this case.

Jun 03 '25 17:06 hazemKrimi

There's already an event generated for it in the folder stateTracker, I'd probably act on that in model.serve. Or potentially also don't pretend we need to decouple that much here, and instead add an explicit (internal) method to trigger a cluster config in model. Could be just a literal method that's called from folder (resp its stateTracker), it already holds a reference to model for other purposes.

@imsodin do you mean in the above that I should have a separate method in FolderConfiguration that creates a new ClusterConfig? I don't really understand what should be the correct implementation to to send a new config to a remote device in this case.

Nope, I don't see FolderConfiguration being involved here at all (but I might just miss how it should be). What I am saying is that whenever the folder state changes, that should trigger sending a cluster config. It's not about a new way of sending the cluster config, what we do now is just fine ( (model.generateClusterConfig and then sending it - of course you need to add the new enum). Doing that needs to be triggered by a change in folder state though.

Jun 13 '25 21:06 imsodin

I will create a new PR for this.

Jun 16 '25 07:06 hazemKrimi

Just to make the link, as it's likely relevant when this is picked up again: There's a PR open that already introduces the enum for folder stopped reason that came up here. It doesn't introduce the new states, but would facilitate it: https://github.com/syncthing/syncthing/pull/10202

Jun 28 '25 12:06 imsodin