Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 5488793 in 2 minutes and 22 seconds. Click for details.
- Reviewed
103lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. Makefile:1
- Draft comment:
PHONY list changed: 'package' target removed. Confirm if this removal is intentional. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. Makefile:30
- Draft comment:
Review installation paths and systemctl invocation for the service file. Ensure user vs system service behavior is as intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure that the behavior is intended, which violates the rules. It does not provide a specific suggestion or point out a specific issue with the code.
3. aw-watcher-window-wayland.service:37
- Draft comment:
Use an absolute path in ExecStart to ensure the binary is located correctly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This comment is suggesting a best practice (using absolute paths in systemd service files), which is generally good advice. However, I need to consider: 1) Is this clearly wrong as-is? No, systemd will find binaries in PATH. 2) Is this actionable? Somewhat, but it's unclear what the absolute path should be since it depends on installation location (could be ~/.local/bin/ or /usr/local/bin/). 3) The installation instructions mention "make install" to ~/.local/, but the exact path isn't specified. 4) This feels more like a "best practice" suggestion rather than a clear bug. The rules say comments should be about clear code changes required, not optional improvements unless they're very actionable. The comment could be valid - systemd documentation does recommend absolute paths for security and clarity. Using PATH lookup can be less predictable. However, many systemd services use binary names without absolute paths successfully, especially for user services where the binary is in the user's PATH. While absolute paths are a best practice, this isn't a clear bug - the service will work as written if the binary is in PATH (which it should be after installation). The comment doesn't specify what the absolute path should be, making it less actionable. This feels more like optional advice rather than a required change. This comment is suggesting a best practice improvement rather than fixing a clear bug. Since the service will work as-is (systemd searches PATH), and the comment doesn't provide a specific absolute path to use, it's not sufficiently actionable. According to the rules, I should only keep comments that clearly require a code change.
Workflow ID: wflow_Llfuxp6ZAIynCSde
You can customize 
This comment was marked as outdated.
This comment was marked as outdated.
Author
|
I did a poor testing job earlier today. This seems a bit more complicated than what I had wished for. The WAYLAND_DISPLAY environment variable may differ (wayland-0 vs wayland-1). Claude suggests to export the variable from the session to systemd. I think it should be possible to do some auto-detection here as a fallback. Or perhaps it's just easier to use aw-qt :-) I'll probably follow up this one in some few days. |
Author
|
I forgot to follow up on this one. I'm running both the server and many watchers through systemd (under the In aw-watcher-afk-prompt Claude set up a Makefile target An alternative could be to have some logic guessing on the correct I will push the |
2 tasks
Add systemd user service file with proper dependencies on aw-server. Includes port readiness check to ensure server is listening before starting the watcher. Note: This is an alternative to running aw-qt (recommended approach). Disclaimer: QA and testing done utilizing real stupidity. Except for that, everything is stiched together using artificial intelligence. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add 60-second timeout to server readiness check to prevent infinite wait - Fix WAYLAND_DISPLAY to use %t (XDG_RUNTIME_DIR) instead of %E (XDG_CONFIG_HOME) Wayland sockets are in $XDG_RUNTIME_DIR, not config directory 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add help target showing all available make targets - Add enable-service target to enable and start systemd service - Add disable-service target to stop and disable service - Add setup-wayland target that auto-detects Sway/Hyprland configs and adds WAYLAND_DISPLAY environment import for systemd services Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
With Restart=on-failure, the watcher would not restart after being stopped cleanly (exit 0). This happened when aw-server stopped and pulled the watcher down via Requires=, then aw-server came back up but the watcher stayed dead — causing multi-hour gaps in recording. Restart=always ensures the watcher comes back regardless of whether it exited cleanly or with an error. The ExecStartPre check already handles waiting for aw-server to be ready. Co-Authored-By: Claude <noreply@anthropic.com>
v3 of both actions is deprecated and now auto-fails on GitHub. Co-Authored-By: Claude <noreply@anthropic.com>
When aw-server is temporarily down, the watcher is stopped by systemd (via Requires=) and then restarted. Previously there was no start-limit configuration, so the default burst limit could cause systemd to stop retrying. Now set StartLimitIntervalSec=7200 / StartLimitBurst=60 (up to 60 attempts in 2 hours) and RestartSec=120 (2-minute gap between attempts) to survive multi-hour aw-server outages. prompt: The window watcher, run from systemd, stops whenever the aw-server is down (even if it's just down for a short while). Can we set up systemd to do auto-restarts? A couple of minutes between each retry is OK, but it should retry restarts for at least an hour or two. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Requires=aw-server.service caused systemd to stop the watcher whenever aw-server went down, even briefly. With RestartSec=120 this meant every aw-server hiccup produced a minimum 2-minute tracking gap. Switch to Wants= so aw-server going down does not stop the watcher — the watcher's existing heartbeat retry loop (outer loop retries on every timer tick, ~5 s) handles runtime outages transparently. Also: - Remove the 60-iteration cap from ExecStartPre so it waits indefinitely for aw-server at startup (add TimeoutStartSec=infinity to match). - Drop RestartSec from 120 s to 10 s, since fast restart is now safe (the watcher only restarts for Wayland/crash reasons, not aw-server). prompt: There seems to be at least one local commit here 0f12b8e to deal with cases where the aw-server is down for a shorter period, but still I have problems that the watcher stops working when the aw server is temporarily down Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Reviewed-by: Tobias Brox <tobias@redpill-linpro.com>
On my laptop I'm starting the compositor by hand. Yesterday it was
started relatively long after booting up and logging in. Without a
compositor, the watcher doesn't want to start. The
RestartSec=10/StartLimitBurst=60 was exhausted roughly ten minutes into
the session - causing a watcher outage for 21 hours (I should set up
some monitoring on it).
A second ExecStartPre has been added, it blocks until the compositor is
usable.
Other approaches considered:
* Waiting for the socket alone would still race: a compositor creates
its socket and only then runs `systemctl --user import-environment
WAYLAND_DISPLAY`, and starting in that window leaves connect_to_env()
with nothing (the hardcoded `wayland-0` fallback does not help on a
machine whose display is `wayland-1`). The gate therefore requires both
the imported WAYLAND_DISPLAY and the socket it names.
* Tuning RestartSec/StartLimitBurst upward was the alternative, but any
number is a guess at how long the user takes to start sway, and a unit
reporting `activating (start-pre)` describes the situation more
honestly than one cycling between `activating (auto-restart)` and
`failed`.
`$$` is systemd's documented escape for a literal `$`. It is not
strictly required here -- systemd expands `${VAR}` inside a quoted
word, but bare `$VAR` only when it forms a whole argv element, so the
bare forms used in this line survive unescaped -- but writing the
escape keeps the line correct if a `${...}` is ever added.
Because the unit now waits, a plain `systemctl --user start` blocks
until a compositor exists, so give the Makefile's start and restart
calls --no-block. That matters most for setup-wayland, which depends on
enable-service and exists to write the very import-environment line the
service is waiting for: without --no-block it deadlocks before writing
it, on exactly the machine that needs it.
Also correct the comment above After=graphical-session.target, which
claimed to ensure a graphical session. The target is inactive on setups
whose compositor never activates it, making the ordering a no-op there.
Adds tests/wait_for_wayland_test.sh, which extracts the wait expression
from the shipped unit and runs it against a faked user environment and
a fake `systemctl` that refuses anything but `--user show-environment`.
It must block with nothing present, with the display imported but no
socket, and -- the case that tells this gate apart from one that merely
waits for a socket -- with the socket present but the display not yet
imported; then return once both are there, and again when a compositor
appears late. Verified by mutation: a socket-only gate passes every
other case and fails that third one. The test cannot catch a missing
`$$`, since escaped and unescaped forms reach /bin/sh identically; its
header says so.
This is the first test in the repository, so it also adds a `test:`
target. A test exits 77 when its prerequisites are missing, which make
treats as failure, so the target maps 77 back to success.
Not addressed: after a SIGKILLed compositor, WAYLAND_DISPLAY stays in
the manager environment and the socket inode can survive, so the gate
would pass against a dead socket and the watcher would fail-loop as
before. Reaching that needs a compositor killed without cleanup, which
is not what happened here.
prompt: Why did not the service run? [+ pasted `systemctl --user status aw-watcher-window-wayland.service` output showing Result: start-limit-hit and ExecStart status=101]
followup-prompt: Sway is started manually on this computer, and this morning I spent some time before sway was started. I think it's better that the unit appears to be failed or not started than that it appears to be running while it's constantly retrying connecting to the compositor, so I believe the fix should go into the systemd unit. I think it should probably retry with longer intervals and many more times. Or maybe it should wait for the socket. Please fix something.
followup-prompts: [Claude trying to ask if I wanted to merge it into existing pull requests or create a new one]
Co-authored-by: Claude Opus 5 (via Claude Code) <noreply@anthropic.com>
Reviewed-by: Tobias Brox <tobias@redpill-linpro.com>
tobixen
added a commit
to tobixen/aw-watcher-window-wayland
that referenced
this pull request
Aug 26, 2026
The service is WantedBy=default.target, so on a machine where the
compositor is started by hand it is launched at login, potentially long
before any compositor exists. get_wl_display() then panics (exit 101),
and with RestartSec=10/StartLimitBurst=60 the unit exhausts its restart
budget roughly ten minutes into the session.
Observed 2026-08-24: boot at 10:36:54, restart limit hit at 10:47:12,
sway's socket created at 10:53:02 -- six minutes after systemd had
given up. The watcher stayed dead for the next 21 hours.
Add a second ExecStartPre that blocks until the compositor is usable.
Waiting for the socket alone would still race: a compositor creates its
socket and only then runs `systemctl --user import-environment
WAYLAND_DISPLAY`, and starting in that window leaves connect_to_env()
with nothing (the hardcoded `wayland-0` fallback does not help on a
machine whose display is `wayland-1`). The gate therefore requires both
the imported WAYLAND_DISPLAY and the socket it names.
Tuning RestartSec/StartLimitBurst upward was the alternative, but any
number is a guess at how long the user takes to start sway, and a unit
reporting `activating (start-pre)` describes the situation more
honestly than one cycling between `activating (auto-restart)` and
`failed`.
`$$` is systemd's documented escape for a literal `$`. It is not
strictly required here -- systemd expands `${VAR}` inside a quoted
word, but bare `$VAR` only when it forms a whole argv element, so the
bare forms used in this line survive unescaped -- but writing the
escape keeps the line correct if a `${...}` is ever added.
Because the unit now waits, a plain `systemctl --user start` blocks
until a compositor exists, so give the Makefile's start and restart
calls --no-block. That matters most for setup-wayland, which depends on
enable-service and exists to write the very import-environment line the
service is waiting for: without --no-block it deadlocks before writing
it, on exactly the machine that needs it.
Also correct the comment above After=graphical-session.target, which
claimed to ensure a graphical session. The target is inactive on setups
whose compositor never activates it, making the ordering a no-op there.
Adds tests/wait_for_wayland_test.sh, which extracts the wait expression
from the shipped unit and runs it against a faked user environment and
a fake `systemctl` that refuses anything but `--user show-environment`.
It must block with nothing present, with the display imported but no
socket, and -- the case that tells this gate apart from one that merely
waits for a socket -- with the socket present but the display not yet
imported; then return once both are there, and again when a compositor
appears late. Verified by mutation: a socket-only gate passes every
other case and fails that third one. The test cannot catch a missing
`$$`, since escaped and unescaped forms reach /bin/sh identically; its
header says so.
A test exits 77 when its prerequisites are missing, which make treats
as failure, so `make test` now maps 77 back to success rather than
letting one skipped test stop the rest from running.
Not addressed: after a SIGKILLed compositor, WAYLAND_DISPLAY stays in
the manager environment and the socket inode can survive, so the gate
would pass against a dead socket and the watcher would fail-loop as
before. Reaching that needs a compositor killed without cleanup, which
is not what happened here.
prompt: Why did not the service run? [+ pasted `systemctl --user status aw-watcher-window-wayland.service` output showing Result: start-limit-hit and ExecStart status=101]
followup-prompt: Sway is started manually on this computer, and this morning I spent some time before sway was started. I think it's better that the unit appears to be failed or not started than that it appears to be running while it's constantly retrying connecting to the compositor, so I believe the fix should go into the systemd unit. I think it should probably retry with longer intervals and many more times. Or maybe it should wait for the socket. Please fix something.
followup-prompt: pull from upstream, merge together, /review-and-push and create a new PR
followup-prompt: (answer to a clarifying question) Findings 1-7, surface 8 [= fix everything the clean-context review found except the SIGKILLed-compositor hole, which this message documents instead]
followup-prompt: (answer to a clarifying question) No new PR -- extend the existing pull requests [= route this change onto ActivityWatch#43 rather than opening a new one; ActivityWatch#50 turned out to be the wrong home for any of it and is left untouched]
Co-authored-by: Claude Opus 5 (via Claude Code) <noreply@anthropic.com>
tobixen
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add systemd user service file with proper dependencies on aw-server.
Includes port readiness check to ensure server is listening before
starting the watcher.
Note: This is an alternative to running aw-qt (recommended approach).
Note/update: more changes added to this pull request - hardening against outages on the aw-server, as well as late start-ups of the compositor, and even introducing some test code.
Disclaimer: Reviews*), QA and testing done utilizing real stupidity. Except for that, most of this pull request was done using artificial intelligence. (Claude Opus, Claude Sonnet, some of the commit messages contain details, including prompts used).
*) Claude is complaining that I'm stealing the credits here - I have been looking through it, but Claude has done a more thorough review than what I was able to do.
Important
Adds systemd service for
aw-watcher-window-waylandand updatesMakefilefor conditional installation and build modes.aw-watcher-window-wayland.serviceto run the watcher withoutaw-qt.aw-serveris ready on port 5600 before starting the watcher.PREFIXbased onSUDO_USERfor user or system-wide installation.installtarget to copy binary and service file to appropriate locations.CARGO_FLAGSandTARGET_DIR.Makefileincludescleantarget to runcargo clean.This description was created by
for 5488793. You can customize this summary. It will automatically update as commits are pushed.