Repository navigation
fix(state): lock staterc and replace it atomically #2195
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
Changes from all commits
ae80756
bc6f7e9
b201bcf
3450783
2e60d96
b7fe006
9a38b2e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| #!/usr/bin/env bash | ||
| ## | ||
| # Locked, atomic writer for $XDG_STATE_HOME/hyde/staterc. | ||
| # | ||
| # Usage: staterc.sh set KEY VALUE | ||
| # | ||
| # waybar.py, set_conf() and the Lua selectors all rewrite staterc. Unlocked | ||
| # and truncated in place, two of them running at once could wipe it down to | ||
| # one line, since a writer that read it mid-rewrite saw an empty file | ||
| # (HyDE-Project/HyDE#2194). Here the read-modify-write runs under an flock | ||
| # on staterc.lock, which waybar.py takes too (fcntl.flock is the same lock), | ||
| # and the result replaces staterc with a rename, so a script sourcing it | ||
| # never sees half a file. | ||
| # | ||
| # The value is written literally: it reaches awk through the environment, | ||
| # not through sed's c command, which ate backslashes ("a\nb" became two | ||
| # lines). A key present twice is collapsed instead of appended again. | ||
| # | ||
| # Do not delete staterc.lock while it is held: the lock lives on the open | ||
| # file, so a second writer would lock a new file at the same path and run | ||
| # concurrently (same as wallpaper_acquire_lock in globalcontrol.sh). | ||
| # | ||
| # Exit status: 0 written, 1 write failed, 2 usage error. | ||
| ## | ||
|
|
||
| if [ "$#" -ne 3 ] || [ "$1" != "set" ]; then | ||
| echo "usage: staterc.sh set KEY VALUE" >&2 | ||
| exit 2 | ||
| fi | ||
| key=$2 | ||
| if [[ ! "$key" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then | ||
| echo "staterc.sh: invalid key '$key'" >&2 | ||
| exit 2 | ||
| fi | ||
|
|
||
| state_dir="${XDG_STATE_HOME:-$HOME/.local/state}/hyde" | ||
| staterc="$state_dir/staterc" | ||
| mkdir -p "$state_dir" || exit 1 | ||
|
|
||
| # A writer that can't get the lock in time still writes: losing the setting | ||
| # is worse than the rare race it would otherwise wait out. The braces keep | ||
| # 2>/dev/null to the open: on a bare exec it would silence the whole script. | ||
| lock_wait=${STATERC_LOCK_WAIT:-10} | ||
| if { exec {lock_fd}>"$state_dir/staterc.lock"; } 2>/dev/null; then | ||
| flock -w "$lock_wait" "$lock_fd" || | ||
| echo "staterc.sh: staterc is still locked after ${lock_wait}s, writing without the lock" >&2 | ||
| else | ||
| echo "staterc.sh: cannot open staterc.lock, writing without the lock" >&2 | ||
| fi | ||
|
|
||
| tmp=$(mktemp "$state_dir/.staterc.XXXXXX" 2>/dev/null) || exit 1 | ||
| trap 'rm -f "$tmp"' EXIT | ||
|
|
||
| # Replace the first KEY= line, drop later duplicates, append if absent. | ||
| src=/dev/null | ||
| [ -f "$staterc" ] && src=$staterc | ||
| STATERC_VALUE=$3 awk -v key="$key" ' | ||
| BEGIN { line = key "=\"" ENVIRON["STATERC_VALUE"] "\"" } | ||
| index($0, key "=") == 1 { if (!done) { print line; done = 1 }; next } | ||
| { print } | ||
| END { if (!done) print line } | ||
| ' "$src" >"$tmp" || exit 1 | ||
|
|
||
| # mktemp creates 0600: keep the old file's mode, or give a new one what a | ||
| # plain redirect would (0666 minus the umask). | ||
| if [ -f "$staterc" ]; then | ||
| chmod --reference="$staterc" "$tmp" | ||
| else | ||
| chmod "$(printf '%o' $((0666 & ~0$(umask))))" "$tmp" | ||
| fi || exit 1 | ||
|
|
||
| mv -f "$tmp" "$staterc" || exit 1 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| #!/usr/bin/env python3 | ||
| # TODO:Refactor. This what happens when you just want it to work lol. | ||
| import contextlib | ||
| import fcntl | ||
| import json | ||
| import os | ||
| import glob | ||
|
|
@@ -11,6 +13,7 @@ | |
| import sys | ||
| import hashlib | ||
| import signal | ||
| import tempfile | ||
|
|
||
| from pathlib import Path | ||
|
|
||
|
|
@@ -152,35 +155,98 @@ def get_config_value(key, default=None): | |
| return default | ||
|
|
||
|
|
||
| def set_state_value(key, value): | ||
| """Set or update a value in the state file, removing any duplicates.""" | ||
| @contextlib.contextmanager | ||
| def _staterc_lock(): | ||
| """Hold the flock on staterc.lock that staterc.sh (set_conf, Lua) takes too. | ||
|
|
||
| Unlocked, two writers rewriting staterc at once could wipe it down to one | ||
| line (HyDE-Project/HyDE#2194). fcntl.flock is the same lock as the | ||
| flock command. A writer still waiting after 10s writes anyway: losing the | ||
| setting is worse than the rare race. Never delete staterc.lock. | ||
| """ | ||
| STATE_FILE.parent.mkdir(parents=True, exist_ok=True) | ||
| try: | ||
| lock = open(STATE_FILE.with_name("staterc.lock"), "a") | ||
| except OSError as exc: | ||
| # e.g. left root-owned by a sudo run; staterc.sh writes on too. | ||
| logger.warning(f"cannot open staterc.lock ({exc}), writing without the lock") | ||
| yield | ||
| return | ||
| with lock: | ||
| deadline = time.monotonic() + 10 | ||
| while True: | ||
| try: | ||
| fcntl.flock(lock, fcntl.LOCK_EX | fcntl.LOCK_NB) | ||
| break | ||
| except BlockingIOError: | ||
| if time.monotonic() >= deadline: | ||
| logger.warning("staterc is still locked after 10s, writing without the lock") | ||
| break | ||
| time.sleep(0.05) | ||
| except OSError as exc: | ||
| logger.warning(f"cannot lock staterc.lock ({exc}), writing without the lock") | ||
| break | ||
| yield | ||
|
|
||
| if not STATE_FILE.exists(): | ||
| with open(STATE_FILE, "w") as file: | ||
| file.write(f"{key}={value}\n") | ||
| return True | ||
|
|
||
| existing_lines = [] | ||
| seen_keys = set() | ||
| if os.path.exists(STATE_FILE): | ||
| with open(STATE_FILE, "r") as file: | ||
| for line in file: | ||
| line = line.strip() | ||
| if not line: | ||
| continue | ||
| try: | ||
| current_key = line.split("=", 1)[0] | ||
| if current_key not in seen_keys and current_key != key: | ||
| existing_lines.append(line) | ||
| seen_keys.add(current_key) | ||
| except Exception: | ||
| existing_lines.append(line) | ||
| def _replace_state_file(text): | ||
| """Replace staterc with text in one step, so no reader sees it half written.""" | ||
| fd, tmp_path = tempfile.mkstemp(dir=STATE_FILE.parent, prefix=".staterc.") | ||
| try: | ||
| with os.fdopen(fd, "w") as file: | ||
| file.write(text) | ||
| # mkstemp creates the file as 0600; keep the old mode, or give a new | ||
| # file what open(..., "w") would (0666 minus the umask). | ||
| if STATE_FILE.exists(): | ||
| shutil.copymode(STATE_FILE, tmp_path) | ||
| else: | ||
| umask = os.umask(0) | ||
| os.umask(umask) | ||
| os.chmod(tmp_path, 0o666 & ~umask) | ||
| os.replace(tmp_path, STATE_FILE) | ||
| except BaseException: | ||
| if os.path.exists(tmp_path): | ||
| os.unlink(tmp_path) | ||
| raise | ||
|
|
||
|
|
||
| def _add_missing_state_values(values): | ||
| """Add the keys of values that staterc still lacks, in one locked step. | ||
|
|
||
| Returns the keys added. A key another writer set in the meantime is kept. | ||
| """ | ||
| with _staterc_lock(): | ||
| text = STATE_FILE.read_text() if STATE_FILE.exists() else "" | ||
| present = {line.split("=", 1)[0] for line in text.splitlines() if "=" in line} | ||
| added = [key for key in values if key not in present] | ||
| if added: | ||
| if text and not text.endswith("\n"): | ||
| text += "\n" | ||
| _replace_state_file(text + "".join(f"{key}={values[key]}\n" for key in added)) | ||
|
Comment on lines
+221
to
+225
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Recheck the layout path when adding derived values. If 🤖 Prompt for AI Agents |
||
| return added | ||
|
|
||
|
|
||
| existing_lines.append(f"{key}={value}") | ||
| def set_state_value(key, value): | ||
| """Set or update a value in the state file, removing any duplicates.""" | ||
| with _staterc_lock(): | ||
| existing_lines = [] | ||
| seen_keys = set() | ||
| if STATE_FILE.exists(): | ||
| with open(STATE_FILE, "r") as file: | ||
| for line in file: | ||
| line = line.strip() | ||
| if not line: | ||
| continue | ||
| try: | ||
| current_key = line.split("=", 1)[0] | ||
| if current_key not in seen_keys and current_key != key: | ||
| existing_lines.append(line) | ||
| seen_keys.add(current_key) | ||
| except Exception: | ||
| existing_lines.append(line) | ||
|
|
||
| with open(STATE_FILE, "w") as file: | ||
| file.write("\n".join(existing_lines) + "\n") | ||
| existing_lines.append(f"{key}={value}") | ||
| _replace_state_file("\n".join(existing_lines) + "\n") | ||
|
|
||
| return True | ||
|
|
||
|
|
@@ -255,53 +321,50 @@ def get_current_layout_from_config(): | |
|
|
||
|
|
||
| def ensure_state_file(): | ||
| """Ensure the state file has the necessary entries.""" | ||
| """Ensure the state file has the necessary entries. | ||
|
|
||
| The values are worked out first, then added in one locked step that skips | ||
| any key another writer set in the meantime. Holding the lock while working | ||
| them out would deadlock: get_current_layout_from_config() writes staterc | ||
| too (HyDE-Project/HyDE#2194). | ||
| """ | ||
| STATE_FILE.parent.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| logger.debug(f"Ensuring state file exists at: {STATE_FILE}") | ||
|
|
||
| keys = ("WAYBAR_LAYOUT_PATH", "WAYBAR_LAYOUT_NAME", "WAYBAR_STYLE_PATH") | ||
| if not STATE_FILE.exists(): | ||
| logger.debug("State file does not exist, creating it") | ||
| current_layout = get_current_layout_from_config() | ||
| layout_name = ( | ||
| os.path.basename(current_layout).replace(".jsonc", "") if current_layout else "" | ||
| if not current_layout: | ||
| STATE_FILE.touch() | ||
| logger.warning("No layout found to write to state file") | ||
| return | ||
| missing = set(keys) | ||
| else: | ||
| with open(STATE_FILE, "r") as file: | ||
| lines = file.readlines() | ||
| missing = {key for key in keys if not any(line.startswith(f"{key}=") for line in lines)} | ||
| if not missing: | ||
| return | ||
| logger.debug("State file is missing entries, updating it") | ||
| # A layout already in staterc is the one to describe; look it up only | ||
| # when the path itself is missing. | ||
| current_layout = ( | ||
| get_current_layout_from_config() | ||
| if "WAYBAR_LAYOUT_PATH" in missing | ||
| else get_state_value("WAYBAR_LAYOUT_PATH") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Normalize the stored layout path before deriving missing entries. When 🤖 Prompt for AI Agents |
||
| ) | ||
| style_path = resolve_style_path(current_layout) if current_layout else "" | ||
|
|
||
| with open(STATE_FILE, "w") as file: | ||
| if current_layout: | ||
| file.write(f"WAYBAR_LAYOUT_PATH={current_layout}\n") | ||
| file.write(f"WAYBAR_LAYOUT_NAME={layout_name}\n") | ||
| file.write(f"WAYBAR_STYLE_PATH={style_path}\n") | ||
| logger.debug(f"Created state file with layout: {current_layout}") | ||
| else: | ||
| logger.warning("No layout found to write to state file") | ||
| return | ||
|
|
||
| with open(STATE_FILE, "r") as file: | ||
| lines = file.readlines() | ||
|
|
||
| layout_path_exists = any(line.startswith("WAYBAR_LAYOUT_PATH=") for line in lines) | ||
| layout_name_exists = any(line.startswith("WAYBAR_LAYOUT_NAME=") for line in lines) | ||
| style_path_exists = any(line.startswith("WAYBAR_STYLE_PATH=") for line in lines) | ||
| if not current_layout: | ||
| return | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| if not layout_path_exists or not layout_name_exists or not style_path_exists: | ||
| logger.debug("State file is missing entries, updating it") | ||
| current_layout = get_current_layout_from_config() if not layout_path_exists else None | ||
| if current_layout: | ||
| layout_name = os.path.basename(current_layout).replace(".jsonc", "") | ||
| style_path = resolve_style_path(current_layout) | ||
|
|
||
| with open(STATE_FILE, "a") as file: | ||
| if not layout_path_exists: | ||
| file.write(f"WAYBAR_LAYOUT_PATH={current_layout}\n") | ||
| logger.debug(f"Added WAYBAR_LAYOUT_PATH={current_layout}") | ||
| if not layout_name_exists: | ||
| file.write(f"WAYBAR_LAYOUT_NAME={layout_name}\n") | ||
| logger.debug(f"Added WAYBAR_LAYOUT_NAME={layout_name}") | ||
| if not style_path_exists: | ||
| file.write(f"WAYBAR_STYLE_PATH={style_path}\n") | ||
| logger.debug(f"Added WAYBAR_STYLE_PATH={style_path}") | ||
| values = { | ||
| "WAYBAR_LAYOUT_PATH": current_layout, | ||
| "WAYBAR_LAYOUT_NAME": os.path.basename(current_layout).replace(".jsonc", ""), | ||
| "WAYBAR_STYLE_PATH": resolve_style_path(current_layout), | ||
| } | ||
| for key in _add_missing_state_values({key: values[key] for key in keys if key in missing}): | ||
| logger.debug(f"Added {key}={values[key]}") | ||
|
|
||
|
|
||
| def resolve_style_path(layout_path): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make accepted values safe to source.
The helper accepts
a"band writesKEY="a"b". Sourcing that assignment fails, although the write succeeds. Other accepted values, including$()expressions, can change meaning when scripts sourcestaterc. Encode values as shell assignments and keepS.staterc_get()compatible with that encoding, or reject values that the current format cannot represent safely. The linked issue confirms that HyDE scripts source this file. (github.com)🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that this is a real problem, but it predates this PR: the old
set_conf()(sed ... c/>>), Lua'sstaterc_set()and Python'sset_state_value()all wrote the value unescaped in the same format, soa"bor$(...)already brokesourcebefore. A proper fix escapes\,",$and backticks on write and unescapes them in Lua'sstaterc_get()and the Python readers, which changes the read side in three languages. To keep this PR focused on the locking and atomic replace, I'll send that as a separate follow-up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EnzoItalianoThanks for clarifying. A separate follow-up is reasonable for this pre-existing issue. This PR can stay focused on locking and atomic replacement.The encoding fix must cover both writers and readers so values remain literal across Bash, Lua, and Python. The finding remains open because the fix is deferred.
Would you like me to open a follow-up issue?
✏️ Learnings added
You are interacting with an AI system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up for this is in #2198 (issue #2197).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EnzoItalianoThanks for linking the follow-up in#2198and issue#2197. No additional issue is needed. This PR can remain focused on locking and atomic replacement.🧠 Learnings used
You are interacting with an AI system.