fix(config): keep comment-only values empty and poll when watching fails
A value that is only a comment (key = # note) is empty again, as before kvconf. When native watching fails, for example on inotify limits, the watcher keeps the current config, warns once and polls every 2 seconds instead of stopping live reload. Watch tests write through a rename so a reload never sees a truncated file, and the repeated-error test now checks that a load error is reported once, cleared by a good load and reported again. The docs describe live reload, comments and the file limits.
- commit
- fcb004a0c28d86574759e4109da3c652fec44e0b
- author
- brice · · 2026-10-10 08:35 UTC
- parent
- 4f752f1
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
3 files · +128 -43
M docs/configuration.md +2 -1
| @@ -3,7 +3,8 @@ | |||
| 3 | 3 | vev reads `~/.config/vev/config` (`$XDG_CONFIG_HOME` is respected). | |
| 4 | 4 | ||
| 5 | 5 | - No file means defaults. | |
| 6 | - | - Changes apply live within a couple of seconds. | |
| 6 | + | - Changes apply live as soon as the file is saved. | |
| 7 | + | - `#` starts a comment at the start of a line or after a space, except inside a quoted word. The file must be UTF-8 and at most 1 MiB; otherwise vev logs a warning and uses the defaults. | |
| 7 | 8 | - Exception: `web.listen` and `web.origin` need a gateway restart. See [browser terminal](web-terminal.md#listener-and-public-origin). | |
| 8 | 9 | - Invalid values log a warning and fall back to the default. | |
| 9 | 10 | ||
| @@ -3,7 +3,8 @@ | |||||
| 3 | vev reads `~/.config/vev/config` (`$XDG_CONFIG_HOME` is respected). | 3 | vev reads `~/.config/vev/config` (`$XDG_CONFIG_HOME` is respected). | ||
| 4 | 4 | ||||
| 5 | - No file means defaults. | 5 | - No file means defaults. | ||
| 6 | - | - Changes apply live within a couple of seconds. | 6 | + | - Changes apply live as soon as the file is saved. |
| 7 | + | - `#` starts a comment at the start of a line or after a space, except inside a quoted word. The file must be UTF-8 and at most 1 MiB; otherwise vev logs a warning and uses the defaults. | |||
| 7 | - Exception: `web.listen` and `web.origin` need a gateway restart. See [browser terminal](web-terminal.md#listener-and-public-origin). | 8 | - Exception: `web.listen` and `web.origin` need a gateway restart. See [browser terminal](web-terminal.md#listener-and-public-origin). | ||
| 8 | - Invalid values log a warning and fall back to the default. | 9 | - Invalid values log a warning and fall back to the default. | ||
| 9 | 10 | ||||
M internal/adapters/config/config.go +24 -5
| @@ -54,6 +54,11 @@ func Parse(r io.Reader) (domain.Config, []domain.Warning, error) { | |||
| 54 | 54 | pending = pending[1:] | |
| 55 | 55 | } | |
| 56 | 56 | lineNo, key, value := entry.Line, entry.Key, entry.Value | |
| 57 | + | // "key = # note" has an empty value: kvconf only strips a '#' that | |
| 58 | + | // follows other text. | |
| 59 | + | if strings.HasPrefix(value, "#") { | |
| 60 | + | value = "" | |
| 61 | + | } | |
| 57 | 62 | ||
| 58 | 63 | switch { | |
| 59 | 64 | case key == "web.listen" || key == "web.origin": | |
| @@ -307,6 +312,12 @@ func Watch(ctx context.Context, path string, onChange func(domain.Config, []doma | |||
| 307 | 312 | return watch(ctx, path, kvconf.WatchOptions{}, onChange) | |
| 308 | 313 | } | |
| 309 | 314 | ||
| 315 | + | // fallbackPoll is the polling interval used when native watching fails. | |
| 316 | + | const fallbackPoll = 2 * time.Second | |
| 317 | + | ||
| 318 | + | // kvWatch is kvconf.Watch; tests replace it to make native watching fail. | |
| 319 | + | var kvWatch = kvconf.Watch | |
| 320 | + | ||
| 310 | 321 | func watch(ctx context.Context, path string, opts kvconf.WatchOptions, onChange func(domain.Config, []domain.Warning)) error { | |
| 311 | 322 | lastErrMsg := "" | |
| 312 | 323 | reportErr := func(err error) { | |
| @@ -319,16 +330,13 @@ func watch(ctx context.Context, path string, opts kvconf.WatchOptions, onChange | |||
| 319 | 330 | reportErr(err) | |
| 320 | 331 | } | |
| 321 | 332 | ||
| 322 | - | watchCtx, cancel := context.WithCancel(ctx) | |
| 323 | - | defer cancel() | |
| 324 | 333 | changed := make(chan struct{}, 1) | |
| 325 | 334 | watchErr := make(chan error, 1) | |
| 326 | - | go func() { watchErr <- kvconf.Watch(watchCtx, path, opts, changed) }() | |
| 335 | + | go func() { watchErr <- kvWatch(ctx, path, opts, changed) }() | |
| 327 | 336 | ||
| 328 | 337 | for { | |
| 329 | 338 | select { | |
| 330 | 339 | case <-ctx.Done(): | |
| 331 | - | cancel() | |
| 332 | 340 | <-watchErr | |
| 333 | 341 | return ctx.Err() | |
| 334 | 342 | case err := <-watchErr: | |
| @@ -338,7 +346,18 @@ func watch(ctx context.Context, path string, opts kvconf.WatchOptions, onChange | |||
| 338 | 346 | if err == nil { | |
| 339 | 347 | err = errors.New("config watcher stopped") | |
| 340 | 348 | } | |
| 341 | - | return err | |
| 349 | + | if opts.Poll > 0 { | |
| 350 | + | return err | |
| 351 | + | } | |
| 352 | + | // The OS ran out of watches (inotify limits): poll like before | |
| 353 | + | // rather than lose live reload until the daemon restarts. The | |
| 354 | + | // warning rides on the current config, which must not reset. | |
| 355 | + | opts.Poll = fallbackPoll | |
| 356 | + | go func() { watchErr <- kvWatch(ctx, path, opts, changed) }() | |
| 357 | + | if cfg, warnings, err2 := Load(path); err2 == nil { | |
| 358 | + | lastErrMsg = "" | |
| 359 | + | onChange(cfg, append(warnings, domain.Warning{Msg: fmt.Sprintf("config watcher: %v; polling every %s", err, fallbackPoll)})) | |
| 360 | + | } | |
| 342 | 361 | case <-changed: | |
| 343 | 362 | cfg, warnings, err := Load(path) | |
| 344 | 363 | if err != nil { | |
| @@ -54,6 +54,11 @@ func Parse(r io.Reader) (domain.Config, []domain.Warning, error) { | |||||
| 54 | pending = pending[1:] | 54 | pending = pending[1:] | ||
| 55 | } | 55 | } | ||
| 56 | lineNo, key, value := entry.Line, entry.Key, entry.Value | 56 | lineNo, key, value := entry.Line, entry.Key, entry.Value | ||
| 57 | + | // "key = # note" has an empty value: kvconf only strips a '#' that | |||
| 58 | + | // follows other text. | |||
| 59 | + | if strings.HasPrefix(value, "#") { | |||
| 60 | + | value = "" | |||
| 61 | + | } | |||
| 57 | 62 | ||||
| 58 | switch { | 63 | switch { | ||
| 59 | case key == "web.listen" || key == "web.origin": | 64 | case key == "web.listen" || key == "web.origin": | ||
| @@ -307,6 +312,12 @@ func Watch(ctx context.Context, path string, onChange func(domain.Config, []doma | |||||
| 307 | return watch(ctx, path, kvconf.WatchOptions{}, onChange) | 312 | return watch(ctx, path, kvconf.WatchOptions{}, onChange) | ||
| 308 | } | 313 | } | ||
| 309 | 314 | ||||
| 315 | + | // fallbackPoll is the polling interval used when native watching fails. | |||
| 316 | + | const fallbackPoll = 2 * time.Second | |||
| 317 | + | ||||
| 318 | + | // kvWatch is kvconf.Watch; tests replace it to make native watching fail. | |||
| 319 | + | var kvWatch = kvconf.Watch | |||
| 320 | + | ||||
| 310 | func watch(ctx context.Context, path string, opts kvconf.WatchOptions, onChange func(domain.Config, []domain.Warning)) error { | 321 | func watch(ctx context.Context, path string, opts kvconf.WatchOptions, onChange func(domain.Config, []domain.Warning)) error { | ||
| 311 | lastErrMsg := "" | 322 | lastErrMsg := "" | ||
| 312 | reportErr := func(err error) { | 323 | reportErr := func(err error) { | ||
| @@ -319,16 +330,13 @@ func watch(ctx context.Context, path string, opts kvconf.WatchOptions, onChange | |||||
| 319 | reportErr(err) | 330 | reportErr(err) | ||
| 320 | } | 331 | } | ||
| 321 | 332 | ||||
| 322 | - | watchCtx, cancel := context.WithCancel(ctx) | |||
| 323 | - | defer cancel() | |||
| 324 | changed := make(chan struct{}, 1) | 333 | changed := make(chan struct{}, 1) | ||
| 325 | watchErr := make(chan error, 1) | 334 | watchErr := make(chan error, 1) | ||
| 326 | - | go func() { watchErr <- kvconf.Watch(watchCtx, path, opts, changed) }() | 335 | + | go func() { watchErr <- kvWatch(ctx, path, opts, changed) }() |
| 327 | 336 | ||||
| 328 | for { | 337 | for { | ||
| 329 | select { | 338 | select { | ||
| 330 | case <-ctx.Done(): | 339 | case <-ctx.Done(): | ||
| 331 | - | cancel() | |||
| 332 | <-watchErr | 340 | <-watchErr | ||
| 333 | return ctx.Err() | 341 | return ctx.Err() | ||
| 334 | case err := <-watchErr: | 342 | case err := <-watchErr: | ||
| @@ -338,7 +346,18 @@ func watch(ctx context.Context, path string, opts kvconf.WatchOptions, onChange | |||||
| 338 | if err == nil { | 346 | if err == nil { | ||
| 339 | err = errors.New("config watcher stopped") | 347 | err = errors.New("config watcher stopped") | ||
| 340 | } | 348 | } | ||
| 341 | - | return err | 349 | + | if opts.Poll > 0 { |
| 350 | + | return err | |||
| 351 | + | } | |||
| 352 | + | // The OS ran out of watches (inotify limits): poll like before | |||
| 353 | + | // rather than lose live reload until the daemon restarts. The | |||
| 354 | + | // warning rides on the current config, which must not reset. | |||
| 355 | + | opts.Poll = fallbackPoll | |||
| 356 | + | go func() { watchErr <- kvWatch(ctx, path, opts, changed) }() | |||
| 357 | + | if cfg, warnings, err2 := Load(path); err2 == nil { | |||
| 358 | + | lastErrMsg = "" | |||
| 359 | + | onChange(cfg, append(warnings, domain.Warning{Msg: fmt.Sprintf("config watcher: %v; polling every %s", err, fallbackPoll)})) | |||
| 360 | + | } | |||
| 342 | case <-changed: | 361 | case <-changed: | ||
| 343 | cfg, warnings, err := Load(path) | 362 | cfg, warnings, err := Load(path) | ||
| 344 | if err != nil { | 363 | if err != nil { | ||
M internal/adapters/config/config_test.go +102 -37
| @@ -2,10 +2,12 @@ package config | |||
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | + | "errors" | |
| 5 | 6 | "os" | |
| 6 | 7 | "path/filepath" | |
| 7 | 8 | "reflect" | |
| 8 | 9 | "strings" | |
| 10 | + | "sync/atomic" | |
| 9 | 11 | "testing" | |
| 10 | 12 | "time" | |
| 11 | 13 | ||
| @@ -123,6 +125,20 @@ func TestParse(t *testing.T) { | |||
| 123 | 125 | }, | |
| 124 | 126 | wantWarnings: []domain.Warning{{Line: 2, Msg: "duplicate key \"floating.command\""}}, | |
| 125 | 127 | }, | |
| 128 | + | { | |
| 129 | + | name: "a value that is only a comment is empty", | |
| 130 | + | input: "floating.command = echo ready\nfloating.command = # default shell\n", | |
| 131 | + | want: domain.Config{ | |
| 132 | + | Theme: domain.ThemeAuto, | |
| 133 | + | Floating: domain.FloatingConfig{ | |
| 134 | + | Width: 80, | |
| 135 | + | Height: 80, | |
| 136 | + | }, | |
| 137 | + | BindingEntries: []domain.ConfigEntry{}, | |
| 138 | + | Codes: map[string]string{}, | |
| 139 | + | }, | |
| 140 | + | wantWarnings: []domain.Warning{{Line: 2, Msg: "duplicate key \"floating.command\""}}, | |
| 141 | + | }, | |
| 126 | 142 | { | |
| 127 | 143 | name: "floating percentage endpoints", | |
| 128 | 144 | input: "floating.width = 1%\nfloating.height = 100%\n", | |
| @@ -872,6 +888,15 @@ func startWatch(t *testing.T, path string) (<-chan watchChange, func()) { | |||
| 872 | 888 | } | |
| 873 | 889 | } | |
| 874 | 890 | ||
| 891 | + | // writeAtomic replaces path in one step, like most editors: the watcher | |
| 892 | + | // never sees a truncated file, whatever the scheduling. | |
| 893 | + | func writeAtomic(t *testing.T, path string, data []byte) { | |
| 894 | + | t.Helper() | |
| 895 | + | tmp := path + ".tmp" | |
| 896 | + | require.NoError(t, os.WriteFile(tmp, data, 0o600)) | |
| 897 | + | require.NoError(t, os.Rename(tmp, path)) | |
| 898 | + | } | |
| 899 | + | ||
| 875 | 900 | func nextChange(t *testing.T, changes <-chan watchChange) watchChange { | |
| 876 | 901 | t.Helper() | |
| 877 | 902 | select { | |
| @@ -901,7 +926,7 @@ func TestWatchReloadsOnChange(t *testing.T) { | |||
| 901 | 926 | name: "changed file reloads with parse warnings", | |
| 902 | 927 | initial: ptr("theme = dark\n"), | |
| 903 | 928 | write: func(t *testing.T, path string) { | |
| 904 | - | require.NoError(t, os.WriteFile(path, []byte("theme = light\nbad line\n"), 0o600)) | |
| 929 | + | writeAtomic(t, path, []byte("theme = light\nbad line\n")) | |
| 905 | 930 | }, | |
| 906 | 931 | wantConfig: light, | |
| 907 | 932 | wantWarnings: []domain.Warning{{Line: 2, Msg: "missing '='"}}, | |
| @@ -910,7 +935,7 @@ func TestWatchReloadsOnChange(t *testing.T) { | |||
| 910 | 935 | name: "created file loads", | |
| 911 | 936 | initial: nil, | |
| 912 | 937 | write: func(t *testing.T, path string) { | |
| 913 | - | require.NoError(t, os.WriteFile(path, []byte("theme = light\n"), 0o600)) | |
| 938 | + | writeAtomic(t, path, []byte("theme = light\n")) | |
| 914 | 939 | }, | |
| 915 | 940 | wantConfig: light, | |
| 916 | 941 | }, | |
| @@ -927,7 +952,7 @@ func TestWatchReloadsOnChange(t *testing.T) { | |||
| 927 | 952 | initial: ptr("theme = light\n"), | |
| 928 | 953 | write: func(t *testing.T, path string) { | |
| 929 | 954 | // Larger than kvconf.DefaultMaxBytes. | |
| 930 | - | require.NoError(t, os.WriteFile(path, []byte(strings.Repeat("# x\n", kvconf.DefaultMaxBytes/4+1)), 0o600)) | |
| 955 | + | writeAtomic(t, path, []byte(strings.Repeat("# x\n", kvconf.DefaultMaxBytes/4+1))) | |
| 931 | 956 | }, | |
| 932 | 957 | wantConfig: domain.Defaults(), | |
| 933 | 958 | wantAnyWarn: true, | |
| @@ -936,7 +961,7 @@ func TestWatchReloadsOnChange(t *testing.T) { | |||
| 936 | 961 | name: "invalid UTF-8 reports defaults and a warning", | |
| 937 | 962 | initial: ptr("theme = light\n"), | |
| 938 | 963 | write: func(t *testing.T, path string) { | |
| 939 | - | require.NoError(t, os.WriteFile(path, []byte("theme = \xff\n"), 0o600)) | |
| 964 | + | writeAtomic(t, path, []byte("theme = \xff\n")) | |
| 940 | 965 | }, | |
| 941 | 966 | wantConfig: domain.Defaults(), | |
| 942 | 967 | wantAnyWarn: true, | |
| @@ -948,7 +973,7 @@ func TestWatchReloadsOnChange(t *testing.T) { | |||
| 948 | 973 | ||
| 949 | 974 | path := filepath.Join(t.TempDir(), "config") | |
| 950 | 975 | if tt.initial != nil { | |
| 951 | - | require.NoError(t, os.WriteFile(path, []byte(*tt.initial), 0o600)) | |
| 976 | + | writeAtomic(t, path, []byte(*tt.initial)) | |
| 952 | 977 | } | |
| 953 | 978 | changes, stop := startWatch(t, path) | |
| 954 | 979 | tt.write(t, path) | |
| @@ -965,51 +990,91 @@ func TestWatchReloadsOnChange(t *testing.T) { | |||
| 965 | 990 | } | |
| 966 | 991 | } | |
| 967 | 992 | ||
| 968 | - | func TestWatchReportsStatErrorOnce(t *testing.T) { | |
| 993 | + | // A load error is reported once until a good load, then again. | |
| 994 | + | func TestWatchReportsLoadErrorOnce(t *testing.T) { | |
| 995 | + | t.Parallel() | |
| 996 | + | ||
| 997 | + | path := filepath.Join(t.TempDir(), "config") | |
| 998 | + | writeAtomic(t, path, []byte("theme = light\n")) | |
| 999 | + | changes, stop := startWatch(t, path) | |
| 1000 | + | invalid := []byte("theme = \xff\n") | |
| 1001 | + | ||
| 1002 | + | writeAtomic(t, path, invalid) | |
| 1003 | + | got := nextChange(t, changes) | |
| 1004 | + | require.Equal(t, domain.Defaults(), got.cfg) | |
| 1005 | + | require.Len(t, got.warnings, 1) | |
| 1006 | + | ||
| 1007 | + | // The same failure again is not reported; the next good load is. | |
| 1008 | + | writeAtomic(t, path, invalid) | |
| 1009 | + | writeAtomic(t, path, []byte("theme = dark\n")) | |
| 1010 | + | got = nextChange(t, changes) | |
| 1011 | + | require.Equal(t, domain.ThemeDark, got.cfg.Theme) | |
| 1012 | + | require.Empty(t, got.warnings) | |
| 1013 | + | ||
| 1014 | + | // After a good load, the failure is news again. | |
| 1015 | + | writeAtomic(t, path, invalid) | |
| 1016 | + | got = nextChange(t, changes) | |
| 1017 | + | require.Len(t, got.warnings, 1) | |
| 1018 | + | stop() | |
| 1019 | + | } | |
| 1020 | + | ||
| 1021 | + | // A stat error at startup other than a missing file is reported. | |
| 1022 | + | func TestWatchReportsStartupStatError(t *testing.T) { | |
| 969 | 1023 | t.Parallel() | |
| 970 | 1024 | ||
| 971 | - | // A name longer than NAME_MAX makes every stat fail with the same error. | |
| 1025 | + | // A name longer than NAME_MAX makes every stat fail. | |
| 972 | 1026 | path := filepath.Join(t.TempDir(), strings.Repeat("x", 5000)) | |
| 973 | 1027 | ctx, cancel := context.WithCancel(context.Background()) | |
| 974 | 1028 | defer cancel() | |
| 975 | - | ||
| 976 | - | changes := make(chan []domain.Warning, 4) | |
| 977 | - | ready := make(chan struct{}) | |
| 1029 | + | changes := make(chan watchChange, 4) | |
| 978 | 1030 | done := make(chan error, 1) | |
| 979 | 1031 | go func() { | |
| 980 | - | done <- watch(ctx, path, kvconf.WatchOptions{Debounce: 10 * time.Millisecond, Ready: ready}, | |
| 981 | - | func(cfg domain.Config, warnings []domain.Warning) { | |
| 982 | - | require.Equal(t, domain.Defaults(), cfg) | |
| 983 | - | changes <- warnings | |
| 984 | - | }) | |
| 1032 | + | done <- watch(ctx, path, kvconf.WatchOptions{Poll: time.Hour}, func(cfg domain.Config, warnings []domain.Warning) { | |
| 1033 | + | changes <- watchChange{cfg: cfg, warnings: warnings} | |
| 1034 | + | }) | |
| 985 | 1035 | }() | |
| 986 | - | ||
| 987 | - | select { | |
| 988 | - | case first := <-changes: | |
| 989 | - | require.Len(t, first, 1) | |
| 990 | - | case <-time.After(5 * time.Second): | |
| 991 | - | t.Fatal("timed out waiting for the initial stat error") | |
| 992 | - | } | |
| 993 | - | select { | |
| 994 | - | case extra := <-changes: | |
| 995 | - | t.Fatalf("watch() repeated persistent stat error: %#v", extra) | |
| 996 | - | case <-done: | |
| 997 | - | // The watcher may refuse such a path; the error is still reported once. | |
| 998 | - | case <-time.After(300 * time.Millisecond): | |
| 999 | - | } | |
| 1000 | - | ||
| 1036 | + | got := nextChange(t, changes) | |
| 1037 | + | require.Equal(t, domain.Defaults(), got.cfg) | |
| 1038 | + | require.Len(t, got.warnings, 1) | |
| 1001 | 1039 | cancel() | |
| 1002 | 1040 | select { | |
| 1003 | - | case err := <-done: | |
| 1004 | - | _ = err | |
| 1041 | + | case <-done: | |
| 1005 | 1042 | case <-time.After(5 * time.Second): | |
| 1006 | 1043 | t.Fatal("timed out waiting for watch() to return") | |
| 1007 | 1044 | } | |
| 1008 | - | select { | |
| 1009 | - | case extra := <-changes: | |
| 1010 | - | t.Fatalf("watch() repeated persistent stat error: %#v", extra) | |
| 1011 | - | default: | |
| 1012 | - | } | |
| 1013 | 1045 | } | |
| 1014 | 1046 | ||
| 1015 | 1047 | func ptr[T any](v T) *T { return &v } | |
| 1048 | + | ||
| 1049 | + | // When native watching fails (inotify limits), watch keeps the current | |
| 1050 | + | // config, warns once and polls instead of giving up. | |
| 1051 | + | func TestWatchFallsBackToPolling(t *testing.T) { //nolint:paralleltest // replaces kvWatch | |
| 1052 | + | path := filepath.Join(t.TempDir(), "config") | |
| 1053 | + | writeAtomic(t, path, []byte("theme = light\n")) | |
| 1054 | + | var polled atomic.Bool | |
| 1055 | + | orig := kvWatch | |
| 1056 | + | t.Cleanup(func() { kvWatch = orig }) | |
| 1057 | + | kvWatch = func(ctx context.Context, path string, opts kvconf.WatchOptions, out chan<- struct{}) error { | |
| 1058 | + | if opts.Poll == 0 { | |
| 1059 | + | return errors.New("too many open files") | |
| 1060 | + | } | |
| 1061 | + | polled.Store(true) | |
| 1062 | + | <-ctx.Done() | |
| 1063 | + | return ctx.Err() | |
| 1064 | + | } | |
| 1065 | + | ctx, cancel := context.WithCancel(context.Background()) | |
| 1066 | + | changes := make(chan watchChange, 4) | |
| 1067 | + | done := make(chan error, 1) | |
| 1068 | + | go func() { | |
| 1069 | + | done <- watch(ctx, path, kvconf.WatchOptions{}, func(cfg domain.Config, warnings []domain.Warning) { | |
| 1070 | + | changes <- watchChange{cfg: cfg, warnings: warnings} | |
| 1071 | + | }) | |
| 1072 | + | }() | |
| 1073 | + | got := nextChange(t, changes) | |
| 1074 | + | require.Equal(t, domain.ThemeLight, got.cfg.Theme, "the current config is kept") | |
| 1075 | + | require.Len(t, got.warnings, 1) | |
| 1076 | + | require.Contains(t, got.warnings[0].Msg, "polling every") | |
| 1077 | + | cancel() | |
| 1078 | + | require.ErrorIs(t, <-done, context.Canceled) | |
| 1079 | + | require.True(t, polled.Load()) | |
| 1080 | + | } | |
| @@ -2,10 +2,12 @@ package config | |||||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | + | "errors" | |||
| 5 | "os" | 6 | "os" | ||
| 6 | "path/filepath" | 7 | "path/filepath" | ||
| 7 | "reflect" | 8 | "reflect" | ||
| 8 | "strings" | 9 | "strings" | ||
| 10 | + | "sync/atomic" | |||
| 9 | "testing" | 11 | "testing" | ||
| 10 | "time" | 12 | "time" | ||
| 11 | 13 | ||||
| @@ -123,6 +125,20 @@ func TestParse(t *testing.T) { | |||||
| 123 | }, | 125 | }, | ||
| 124 | wantWarnings: []domain.Warning{{Line: 2, Msg: "duplicate key \"floating.command\""}}, | 126 | wantWarnings: []domain.Warning{{Line: 2, Msg: "duplicate key \"floating.command\""}}, | ||
| 125 | }, | 127 | }, | ||
| 128 | + | { | |||
| 129 | + | name: "a value that is only a comment is empty", | |||
| 130 | + | input: "floating.command = echo ready\nfloating.command = # default shell\n", | |||
| 131 | + | want: domain.Config{ | |||
| 132 | + | Theme: domain.ThemeAuto, | |||
| 133 | + | Floating: domain.FloatingConfig{ | |||
| 134 | + | Width: 80, | |||
| 135 | + | Height: 80, | |||
| 136 | + | }, | |||
| 137 | + | BindingEntries: []domain.ConfigEntry{}, | |||
| 138 | + | Codes: map[string]string{}, | |||
| 139 | + | }, | |||
| 140 | + | wantWarnings: []domain.Warning{{Line: 2, Msg: "duplicate key \"floating.command\""}}, | |||
| 141 | + | }, | |||
| 126 | { | 142 | { | ||
| 127 | name: "floating percentage endpoints", | 143 | name: "floating percentage endpoints", | ||
| 128 | input: "floating.width = 1%\nfloating.height = 100%\n", | 144 | input: "floating.width = 1%\nfloating.height = 100%\n", | ||
| @@ -872,6 +888,15 @@ func startWatch(t *testing.T, path string) (<-chan watchChange, func()) { | |||||
| 872 | } | 888 | } | ||
| 873 | } | 889 | } | ||
| 874 | 890 | ||||
| 891 | + | // writeAtomic replaces path in one step, like most editors: the watcher | |||
| 892 | + | // never sees a truncated file, whatever the scheduling. | |||
| 893 | + | func writeAtomic(t *testing.T, path string, data []byte) { | |||
| 894 | + | t.Helper() | |||
| 895 | + | tmp := path + ".tmp" | |||
| 896 | + | require.NoError(t, os.WriteFile(tmp, data, 0o600)) | |||
| 897 | + | require.NoError(t, os.Rename(tmp, path)) | |||
| 898 | + | } | |||
| 899 | + | ||||
| 875 | func nextChange(t *testing.T, changes <-chan watchChange) watchChange { | 900 | func nextChange(t *testing.T, changes <-chan watchChange) watchChange { | ||
| 876 | t.Helper() | 901 | t.Helper() | ||
| 877 | select { | 902 | select { | ||
| @@ -901,7 +926,7 @@ func TestWatchReloadsOnChange(t *testing.T) { | |||||
| 901 | name: "changed file reloads with parse warnings", | 926 | name: "changed file reloads with parse warnings", | ||
| 902 | initial: ptr("theme = dark\n"), | 927 | initial: ptr("theme = dark\n"), | ||
| 903 | write: func(t *testing.T, path string) { | 928 | write: func(t *testing.T, path string) { | ||
| 904 | - | require.NoError(t, os.WriteFile(path, []byte("theme = light\nbad line\n"), 0o600)) | 929 | + | writeAtomic(t, path, []byte("theme = light\nbad line\n")) |
| 905 | }, | 930 | }, | ||
| 906 | wantConfig: light, | 931 | wantConfig: light, | ||
| 907 | wantWarnings: []domain.Warning{{Line: 2, Msg: "missing '='"}}, | 932 | wantWarnings: []domain.Warning{{Line: 2, Msg: "missing '='"}}, | ||
| @@ -910,7 +935,7 @@ func TestWatchReloadsOnChange(t *testing.T) { | |||||
| 910 | name: "created file loads", | 935 | name: "created file loads", | ||
| 911 | initial: nil, | 936 | initial: nil, | ||
| 912 | write: func(t *testing.T, path string) { | 937 | write: func(t *testing.T, path string) { | ||
| 913 | - | require.NoError(t, os.WriteFile(path, []byte("theme = light\n"), 0o600)) | 938 | + | writeAtomic(t, path, []byte("theme = light\n")) |
| 914 | }, | 939 | }, | ||
| 915 | wantConfig: light, | 940 | wantConfig: light, | ||
| 916 | }, | 941 | }, | ||
| @@ -927,7 +952,7 @@ func TestWatchReloadsOnChange(t *testing.T) { | |||||
| 927 | initial: ptr("theme = light\n"), | 952 | initial: ptr("theme = light\n"), | ||
| 928 | write: func(t *testing.T, path string) { | 953 | write: func(t *testing.T, path string) { | ||
| 929 | // Larger than kvconf.DefaultMaxBytes. | 954 | // Larger than kvconf.DefaultMaxBytes. | ||
| 930 | - | require.NoError(t, os.WriteFile(path, []byte(strings.Repeat("# x\n", kvconf.DefaultMaxBytes/4+1)), 0o600)) | 955 | + | writeAtomic(t, path, []byte(strings.Repeat("# x\n", kvconf.DefaultMaxBytes/4+1))) |
| 931 | }, | 956 | }, | ||
| 932 | wantConfig: domain.Defaults(), | 957 | wantConfig: domain.Defaults(), | ||
| 933 | wantAnyWarn: true, | 958 | wantAnyWarn: true, | ||
| @@ -936,7 +961,7 @@ func TestWatchReloadsOnChange(t *testing.T) { | |||||
| 936 | name: "invalid UTF-8 reports defaults and a warning", | 961 | name: "invalid UTF-8 reports defaults and a warning", | ||
| 937 | initial: ptr("theme = light\n"), | 962 | initial: ptr("theme = light\n"), | ||
| 938 | write: func(t *testing.T, path string) { | 963 | write: func(t *testing.T, path string) { | ||
| 939 | - | require.NoError(t, os.WriteFile(path, []byte("theme = \xff\n"), 0o600)) | 964 | + | writeAtomic(t, path, []byte("theme = \xff\n")) |
| 940 | }, | 965 | }, | ||
| 941 | wantConfig: domain.Defaults(), | 966 | wantConfig: domain.Defaults(), | ||
| 942 | wantAnyWarn: true, | 967 | wantAnyWarn: true, | ||
| @@ -948,7 +973,7 @@ func TestWatchReloadsOnChange(t *testing.T) { | |||||
| 948 | 973 | ||||
| 949 | path := filepath.Join(t.TempDir(), "config") | 974 | path := filepath.Join(t.TempDir(), "config") | ||
| 950 | if tt.initial != nil { | 975 | if tt.initial != nil { | ||
| 951 | - | require.NoError(t, os.WriteFile(path, []byte(*tt.initial), 0o600)) | 976 | + | writeAtomic(t, path, []byte(*tt.initial)) |
| 952 | } | 977 | } | ||
| 953 | changes, stop := startWatch(t, path) | 978 | changes, stop := startWatch(t, path) | ||
| 954 | tt.write(t, path) | 979 | tt.write(t, path) | ||
| @@ -965,51 +990,91 @@ func TestWatchReloadsOnChange(t *testing.T) { | |||||
| 965 | } | 990 | } | ||
| 966 | } | 991 | } | ||
| 967 | 992 | ||||
| 968 | - | func TestWatchReportsStatErrorOnce(t *testing.T) { | 993 | + | // A load error is reported once until a good load, then again. |
| 994 | + | func TestWatchReportsLoadErrorOnce(t *testing.T) { | |||
| 995 | + | t.Parallel() | |||
| 996 | + | ||||
| 997 | + | path := filepath.Join(t.TempDir(), "config") | |||
| 998 | + | writeAtomic(t, path, []byte("theme = light\n")) | |||
| 999 | + | changes, stop := startWatch(t, path) | |||
| 1000 | + | invalid := []byte("theme = \xff\n") | |||
| 1001 | + | ||||
| 1002 | + | writeAtomic(t, path, invalid) | |||
| 1003 | + | got := nextChange(t, changes) | |||
| 1004 | + | require.Equal(t, domain.Defaults(), got.cfg) | |||
| 1005 | + | require.Len(t, got.warnings, 1) | |||
| 1006 | + | ||||
| 1007 | + | // The same failure again is not reported; the next good load is. | |||
| 1008 | + | writeAtomic(t, path, invalid) | |||
| 1009 | + | writeAtomic(t, path, []byte("theme = dark\n")) | |||
| 1010 | + | got = nextChange(t, changes) | |||
| 1011 | + | require.Equal(t, domain.ThemeDark, got.cfg.Theme) | |||
| 1012 | + | require.Empty(t, got.warnings) | |||
| 1013 | + | ||||
| 1014 | + | // After a good load, the failure is news again. | |||
| 1015 | + | writeAtomic(t, path, invalid) | |||
| 1016 | + | got = nextChange(t, changes) | |||
| 1017 | + | require.Len(t, got.warnings, 1) | |||
| 1018 | + | stop() | |||
| 1019 | + | } | |||
| 1020 | + | ||||
| 1021 | + | // A stat error at startup other than a missing file is reported. | |||
| 1022 | + | func TestWatchReportsStartupStatError(t *testing.T) { | |||
| 969 | t.Parallel() | 1023 | t.Parallel() | ||
| 970 | 1024 | ||||
| 971 | - | // A name longer than NAME_MAX makes every stat fail with the same error. | 1025 | + | // A name longer than NAME_MAX makes every stat fail. |
| 972 | path := filepath.Join(t.TempDir(), strings.Repeat("x", 5000)) | 1026 | path := filepath.Join(t.TempDir(), strings.Repeat("x", 5000)) | ||
| 973 | ctx, cancel := context.WithCancel(context.Background()) | 1027 | ctx, cancel := context.WithCancel(context.Background()) | ||
| 974 | defer cancel() | 1028 | defer cancel() | ||
| 975 | - | 1029 | + | changes := make(chan watchChange, 4) | |
| 976 | - | changes := make(chan []domain.Warning, 4) | |||
| 977 | - | ready := make(chan struct{}) | |||
| 978 | done := make(chan error, 1) | 1030 | done := make(chan error, 1) | ||
| 979 | go func() { | 1031 | go func() { | ||
| 980 | - | done <- watch(ctx, path, kvconf.WatchOptions{Debounce: 10 * time.Millisecond, Ready: ready}, | 1032 | + | done <- watch(ctx, path, kvconf.WatchOptions{Poll: time.Hour}, func(cfg domain.Config, warnings []domain.Warning) { |
| 981 | - | func(cfg domain.Config, warnings []domain.Warning) { | 1033 | + | changes <- watchChange{cfg: cfg, warnings: warnings} |
| 982 | - | require.Equal(t, domain.Defaults(), cfg) | 1034 | + | }) |
| 983 | - | changes <- warnings | |||
| 984 | - | }) | |||
| 985 | }() | 1035 | }() | ||
| 986 | - | 1036 | + | got := nextChange(t, changes) | |
| 987 | - | select { | 1037 | + | require.Equal(t, domain.Defaults(), got.cfg) |
| 988 | - | case first := <-changes: | 1038 | + | require.Len(t, got.warnings, 1) |
| 989 | - | require.Len(t, first, 1) | |||
| 990 | - | case <-time.After(5 * time.Second): | |||
| 991 | - | t.Fatal("timed out waiting for the initial stat error") | |||
| 992 | - | } | |||
| 993 | - | select { | |||
| 994 | - | case extra := <-changes: | |||
| 995 | - | t.Fatalf("watch() repeated persistent stat error: %#v", extra) | |||
| 996 | - | case <-done: | |||
| 997 | - | // The watcher may refuse such a path; the error is still reported once. | |||
| 998 | - | case <-time.After(300 * time.Millisecond): | |||
| 999 | - | } | |||
| 1000 | - | ||||
| 1001 | cancel() | 1039 | cancel() | ||
| 1002 | select { | 1040 | select { | ||
| 1003 | - | case err := <-done: | 1041 | + | case <-done: |
| 1004 | - | _ = err | |||
| 1005 | case <-time.After(5 * time.Second): | 1042 | case <-time.After(5 * time.Second): | ||
| 1006 | t.Fatal("timed out waiting for watch() to return") | 1043 | t.Fatal("timed out waiting for watch() to return") | ||
| 1007 | } | 1044 | } | ||
| 1008 | - | select { | |||
| 1009 | - | case extra := <-changes: | |||
| 1010 | - | t.Fatalf("watch() repeated persistent stat error: %#v", extra) | |||
| 1011 | - | default: | |||
| 1012 | - | } | |||
| 1013 | } | 1045 | } | ||
| 1014 | 1046 | ||||
| 1015 | func ptr[T any](v T) *T { return &v } | 1047 | func ptr[T any](v T) *T { return &v } | ||
| 1048 | + | ||||
| 1049 | + | // When native watching fails (inotify limits), watch keeps the current | |||
| 1050 | + | // config, warns once and polls instead of giving up. | |||
| 1051 | + | func TestWatchFallsBackToPolling(t *testing.T) { //nolint:paralleltest // replaces kvWatch | |||
| 1052 | + | path := filepath.Join(t.TempDir(), "config") | |||
| 1053 | + | writeAtomic(t, path, []byte("theme = light\n")) | |||
| 1054 | + | var polled atomic.Bool | |||
| 1055 | + | orig := kvWatch | |||
| 1056 | + | t.Cleanup(func() { kvWatch = orig }) | |||
| 1057 | + | kvWatch = func(ctx context.Context, path string, opts kvconf.WatchOptions, out chan<- struct{}) error { | |||
| 1058 | + | if opts.Poll == 0 { | |||
| 1059 | + | return errors.New("too many open files") | |||
| 1060 | + | } | |||
| 1061 | + | polled.Store(true) | |||
| 1062 | + | <-ctx.Done() | |||
| 1063 | + | return ctx.Err() | |||
| 1064 | + | } | |||
| 1065 | + | ctx, cancel := context.WithCancel(context.Background()) | |||
| 1066 | + | changes := make(chan watchChange, 4) | |||
| 1067 | + | done := make(chan error, 1) | |||
| 1068 | + | go func() { | |||
| 1069 | + | done <- watch(ctx, path, kvconf.WatchOptions{}, func(cfg domain.Config, warnings []domain.Warning) { | |||
| 1070 | + | changes <- watchChange{cfg: cfg, warnings: warnings} | |||
| 1071 | + | }) | |||
| 1072 | + | }() | |||
| 1073 | + | got := nextChange(t, changes) | |||
| 1074 | + | require.Equal(t, domain.ThemeLight, got.cfg.Theme, "the current config is kept") | |||
| 1075 | + | require.Len(t, got.warnings, 1) | |||
| 1076 | + | require.Contains(t, got.warnings[0].Msg, "polling every") | |||
| 1077 | + | cancel() | |||
| 1078 | + | require.ErrorIs(t, <-done, context.Canceled) | |||
| 1079 | + | require.True(t, polled.Load()) | |||
| 1080 | + | } | |||