Gate set_last_save_timestamp on save script exit code (fixes #162)#163
Open
pleasedodisturb wants to merge 1 commit into
Open
Gate set_last_save_timestamp on save script exit code (fixes #162)#163pleasedodisturb wants to merge 1 commit into
pleasedodisturb wants to merge 1 commit into
Conversation
`fetch_and_run_tmux_resurrect_save_script` previously backgrounded the save script with `&` and called `set_last_save_timestamp` unconditionally. When the configured save script silently failed (wrong path, missing file, exec error swallowed by `>/dev/null 2>&1`), the last-save timestamp still advanced — making `@continuum-save-last-timestamp` appear healthy while nothing was being written to disk. The same anti-pattern as a backup system without restore drills: the system actively lies about its health, the user has no signal, and saves are silently lost across reboots. This is the root cause behind reports like: - tmux-plugins#22 — "automatic saving not working when tmux status is off" - tmux-plugins#94 — saved files contain incomplete session data - numerous downstream wrappers users have written to compensate Change: run the save script synchronously and only call `set_last_save_timestamp` when it returns exit 0. On failure the timestamp stays put, so the next save tick (5s status interval, gated to every save-interval minutes) will retry instead of skipping ahead. Behavioural impact: - Synchronous save adds at most one save-script duration to one status refresh per save-interval (default 15 min). Tested locally with `@resurrect-capture-pane-contents 'on'` on 10 panes: ~80ms. Status bar pause is invisible at that scale. - The existing `acquire_lock` trap now correctly holds the lock for the duration of the save (previously the trap fired on the wrapper exit while the backgrounded save was still running — the lock was effectively a no-op for save serialization). Verified with a probe save script in 4 cases (test scaffolding at `fetch_and_run_tmux_resurrect_save_script` level, with `get_tmux_option` and `set_last_save_timestamp` stubbed): | Case | Before | After | |----------------------------|--------|--------| | probe exits 0 | set | set ✓ | | probe exits 1 | set ✗ | NOT ✓ | | save script path missing | set ✗ | NOT ✓ | | @resurrect-save-script-path empty | NOT | NOT ✓ | Related: tmux-plugins#159 (boot-grace race that clobbers the `last` symlink) is a separate but adjacent bug in the same file — both stem from assumptions about backgrounded operations completing successfully. This patch is intentionally minimal and does not touch tmux-plugins#159's scope. Reviewed-by: claude-opus-4-7 (code+security)
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.
Fixes #162.
Summary
fetch_and_run_tmux_resurrect_save_scriptpreviously backgrounded the save with&and calledset_last_save_timestampunconditionally, so silent save failures (wrong path, missing file, non-zero exit, error swallowed by>/dev/null 2>&1) still advanced@continuum-save-last-timestamp. Result:tmux_resurrect_*.txtfiles stop being written to disk while continuum keeps reporting healthy saves indefinitely.This is the issue tracked in #162, with deeper symptoms in #22 (open since 2016) and #94. Independent of #159 (boot-grace race in the same file) but both fixes can land.
Change
fetch_and_run_tmux_resurrect_save_script() { local resurrect_save_script_path="$(get_tmux_option "$resurrect_save_path_option" "")" if [ -n "$resurrect_save_script_path" ]; then - "$resurrect_save_script_path" "quiet" >/dev/null 2>&1 & - set_last_save_timestamp + if "$resurrect_save_script_path" "quiet" >/dev/null 2>&1; then + set_last_save_timestamp + fi fi }Two changes:
&— run the save script synchronously so the caller sees the exit code.set_last_save_timestampon success. On failure the timestamp stays put and the next tick (every status interval, gated to every save-interval minutes) will retry.Behavioural impact
In practice the synchronous save adds ~80ms with
@resurrect-capture-pane-contents 'on'and 10 panes — invisible at that scale, and only once every save-interval (default 15 min). Users with extremely large pane scrollback may see a longer pause; this is the cost of correctness.Side benefit: the existing
acquire_locktrap now correctly holds the lock for the duration of the save. Previously the trap fired on wrapper exit while the backgrounded save was still running, making the lock effectively a no-op for save serialization.Verification
Targeted test at the function level (sources the function with stubbed
get_tmux_option/set_last_save_timestamp, exercises each branch). Same harness run before and after to demonstrate the bug and the fix:@resurrect-save-script-pathunset(✗ = bug present; ✓ = correct behaviour.)
Workaround pointer
For users hitting this in the wild who want a fix today without waiting on this PR, a wrapper script and a real-mtime status-bar widget are at https://github.com/pleasedodisturb/terminal-craft/pull/43 (
scripts/continuum-save-guard.sh+scripts/save-status.sh). The widget reads file mtime directly instead of trusting@continuum-save-last-timestamp— the lying timestamp this PR fixes.Test plan