Skip to content

Conversation

@FreyreCorona
Copy link
Contributor

@FreyreCorona FreyreCorona commented Dec 8, 2025

This change refines the internal logic used by the Windows Service module to report state updates to the Service Control Manager (SCM).

A new Status(name string) function was added to map human-readable state names (“running”, “stopped”, etc.) into the corresponding svc.State values. This centralizes state translation and avoids duplicated logic.
The Error function was also updated to send a StopPending status through the global status channel before returning, ensuring the SCM is consistently notified when the service fails.

Overall, the goal is to improve communication with the SCM and avoid inconsistent state transitions during service shutdown or failure.

Motivation:
The current implementation does not exist, and te big TODO reaveal this. Consolidating state mapping and making error reporting explicit improves maintainability and reduces the risk of state desynchronization.

How it was tested:
– Cross-compilation for Windows (GOOS=windows) to ensure the module builds correctly.
– Manually tested on CI (github Actions) to ensure the functions are work as expected (now)

The modifications only affect internal state reporting logic on Windows systems.

The code was generated by me(human) and the text of this PR was generated by ChatGPT

@CLAassistant
Copy link

CLAassistant commented Dec 8, 2025

CLA assistant check
All committers have signed the CLA.

@francislavoie
Copy link
Member

Thanks!

How it was tested:
– Cross-compilation for Windows (GOOS=windows) to ensure the module builds correctly.

Did you test it on Windows to make sure it does what you expect? Or did you just compile it?

@francislavoie francislavoie changed the title implement service status and error notifications notify: implement windows service status and error notifications Dec 8, 2025
@FreyreCorona
Copy link
Contributor Author

FreyreCorona commented Dec 8, 2025

Did you test it on Windows to make sure it does what you expect? Or did you just compile it?

I just compile it , if the other previous functions already works, because are on main branch , these work too because are the same code as well , these are basic operations , anyways today i want to test on a windows machine because using CI from github actions
Unit test passed normally with both operations

@FreyreCorona FreyreCorona force-pushed the notify-windows-TODO branch 2 times, most recently from 89089cc to 3515ae9 Compare December 8, 2025 14:42
@FreyreCorona
Copy link
Contributor Author

I already run a lot of tests this morning on github Actions , that's why have the force pushes , i tested , update the code and delete the commits with the test.yml used for testing
it's my first time contributing with a open source repository , sorry for the confusion.
Now already the code was tested and i am sure it work as expected. :)

Copy link
Contributor Author

@FreyreCorona FreyreCorona left a comment

Choose a reason for hiding this comment

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

I already test this code and it work as expected , but , in anywhere is called the Status function , it's because are not implemented or other some reason ?

@francislavoie
Copy link
Member

I don't understand the question. Can you clarify what you mean?

@francislavoie
Copy link
Member

FYI @WingLim since you originally contributed the notify stuff, if you'd like to take a look

@FreyreCorona
Copy link
Contributor Author

FreyreCorona commented Dec 8, 2025

I don't understand the question. Can you clarify what you mean?

I Search on all LSP references for the function Status , and did not found any call 0 incoming calls
Maybe on my Neovim i cannot found anyone, it was only a observation
The others functions are called , Error , Ready, Realoading, etc..

@francislavoie
Copy link
Member

francislavoie commented Dec 8, 2025

Ah, I think it was just to cover the systemd APIs on linux (i.e. it has a STATUS=msg that can be notified), i.e. left in, just in case we do at some point realize we need to trigger a status for whatever reason. And since we have OS-specific files, the windows one also needs that one declared even if it doesn't necessarily do anything.

@FreyreCorona
Copy link
Contributor Author

Hi @WingLim , all the revision changes are applied

@FreyreCorona
Copy link
Contributor Author

@mohammed90 , i think it's correct now , i tried to avoid create a new error object for pass arguments as zap.Error() , instead i used a zap.String() for simplicity , i be used caddy.Log().Warn() instead o Error() ??

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on this! Looks good to me, though I don't understand the Windows APIs personally.

@FreyreCorona
Copy link
Contributor Author

FreyreCorona commented Dec 11, 2025

@mohammed90 , The linters are failing because import caddy.Log on notify make a circular dependency with import notify on caddy module , thats why the first time i use log directly , after make a change for fix that , i wanna ask. There is another way to do this ? like importing zap directly and calling default logger on it ? or simply use log library ? Ignore them like @WingLim says?
I ask for avoid a commit for fix and another commit for fix the fixed code

@mholt
Copy link
Member

mholt commented Dec 11, 2025

Can notify's files go into the caddy package instead?

The other option is to revert to using just std lib log -- we capture it into a structured log anyway -- I think what you originally had.

Actually yeah maybe just using std lib log is better than moving the whole package files.

@FreyreCorona
Copy link
Contributor Author

I understand , just go backward on the commit for these change , i wait for responses for other members or just rollback the commit ?

@mohammed90
Copy link
Member

Yes, since we're blocked with import cycle, we can revert to stdlib log.

@FreyreCorona FreyreCorona requested a review from mholt December 11, 2025 22:32
@francislavoie
Copy link
Member

Could we move notify into internal to get around the cycle?

@FreyreCorona
Copy link
Contributor Author

FreyreCorona commented Dec 12, 2025

🤷‍♂️ if you import caddy module on notify and notify on module caddy you always get the error , you wanna delete the notify module ?

@mholt
Copy link
Member

mholt commented Dec 12, 2025

Nope, I don't think that will work. Still an import cycle.

@FreyreCorona
Copy link
Contributor Author

Why will use Zap library on the first place ? it's not better an simple use log/slog of the std library ????

@francislavoie
Copy link
Member

We use zap because Caddy v2 was designed before slog existed. We have a compatibility layer for slog for plugins to use though.

But anyway, I think it's fine that we just plainly log in this case because it should never actually happen unless we had bad code which uses an unsupported status.

@francislavoie francislavoie merged commit 409a072 into caddyserver:master Dec 12, 2025
54 checks passed
@FreyreCorona
Copy link
Contributor Author

But anyway, I think it's fine that we just plainly log in this case because it should never actually happen unless we had bad code which uses an unsupported status.

I think the same , if you pass a unsupported status , the function will be a NO-OP , just log for give it some debug information

@github-actions github-actions bot mentioned this pull request Jan 6, 2026
4 tasks
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.

6 participants