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

nixos/syncthing: add missing --fail to curl #390742

Merged
merged 2 commits into from
Mar 18, 2025
Merged

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Mar 17, 2025

This is to address a problem I've found when debugging: #326704 (comment)

Without --fail, curl does not seem to set exit code, and I'm under suspicion all the other retry flags etc. are doing nothing as well.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 17, 2025
@dpc dpc requested a review from bjornfor March 17, 2025 19:58
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Mar 17, 2025
Copy link
Contributor

@bjornfor bjornfor left a comment

Choose a reason for hiding this comment

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

LGTM, but please update commit message to have "nixos/syncthing:" prefix.

In case of an incorrect ID, the behavior of this script
is quite tricky. See:

NixOS#326704 (comment)

for more info.
@dpc
Copy link
Contributor Author

dpc commented Mar 17, 2025

@bjornfor

I've added one more change to help with debugging. If something's wrong with please let me know, I can always remove.

@bjornfor bjornfor changed the title fix(syncthing): missing --fail to curl nixos/syncthing: add missing --fail to curl Mar 17, 2025
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 17, 2025
@bjornfor bjornfor merged commit 07a3793 into NixOS:master Mar 18, 2025
37 checks passed
@74k1
Copy link
Contributor

74k1 commented Mar 22, 2025

Hey @dpc

Wanted to chime in and say, that a rebuild would fail because of this when having the following options set to false:

services.syncthing.settings = {
  overrideDevice = false;
  overrideFolder = false;
}

I get an error for the syncthing-init.service.

Console Output of manually running the service command
env RUNTIME_DIRECTORY=/tmp bash -x /nix/store/qngqpx8bmwh98hdmkmxipqjyyl6l3lqn-merge-syncthing-config
+ set -efu
+ umask 0077
+ curl -X PUT -d true 127.0.0.1:8384/rest/config/relaysEnabled
+ /nix/store/ag4wrj6qc542q7l67ld6i1pwbcp1cjq4-libxml2-2.13.6-bin/bin/xmllint --xpath 'string(configuration/gui/apikey)' /home/taki/.config/syncthing/config.xml
+ printf 'X-API-Key: '
+ cat /tmp/api_key
+ /nix/store/ipyzblpplw889rdvmm7sw8r3kf24p6c2-curl-8.12.1-bin/bin/curl --fail -sSLk -H @/tmp/headers --retry 1000 --retry-delay 1 --retry-all-errors -X PUT -d true 127.0.0.1:8384/rest/config/relaysEnabled
curl: (22) The requested URL returned error: 404
curl: (22) The requested URL returned error: 404
curl: (22) The requested URL returned error: 404

I'm not entirely sure if it's really related to my syncthing settings, as I wouldn't want my current devices and folders to get removed by testing it on true.

At least removing the --fail flag on that curl worked for me.

It could be that it's just me. If that is the case, please let me know.

@bjornfor
Copy link
Contributor

I'm seeing the same in my local integration tests (repeated curl: (22) The requested URL returned error: 404, and eventually timeout). The NixOS tests don't have/see a problem though. I think we should revert until this is better understodd.

@bjornfor
Copy link
Contributor

Revert here: #392178. Please review.

drupol pushed a commit that referenced this pull request Mar 23, 2025
This reverts commit a8b8f8f.

It introduced a failure in the syncthing service, where it hangs at the
curl step, repeatedly printing this:

  l3ijkvb20h5nnffg5q25i4nmcsbf7glx-merge-syncthing-config[1458]: curl: (22) The requested URL returned error: 404
  l3ijkvb20h5nnffg5q25i4nmcsbf7glx-merge-syncthing-config[1458]: curl: (22) The requested URL returned error: 404
  l3ijkvb20h5nnffg5q25i4nmcsbf7glx-merge-syncthing-config[1458]: curl: (22) The requested URL returned error: 404
  [...]

This is unfortunately not detected by `nix-build -A syncthing.tests`.

Ref #390742
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants