Skip to content

fix(sync-workspace): isolate test lock dir + fix test 27 init guard - #2026

Merged
qingyun-wu merged 2 commits into
sonichi:mainfrom
john-the-dev:fix/sync-workspace-test-lock-isolation
Jul 9, 2026
Merged

qingyun-wu merged 2 commits into
sonichi:mainfrom
john-the-dev:fix/sync-workspace-test-lock-isolation

Conversation

@john-the-dev

Copy link
Copy Markdown
Collaborator

Closes

Resolves the remaining entry in shell-ci-known-failures.txt (tracked in PR #1948).

Summary

Two independent root causes for CI failures in tests/sync-workspace.test.sh:

1. Lock contention (tests 8, 9, 10)
The default lock dir /tmp/sync-workspace.lock.d is shared between test invocations and the live sync-workspace cron. When the cron fires mid-test it steals the lock; subsequent test calls exit immediately with "another instance is running, skipping" causing assertion failures.

Fix: add SUTANDO_SYNC_LOCK_DIR env hook to sync-workspace.sh (same pattern as SUTANDO_WS_ID_OVERRIDE, SUTANDO_TEST_MODE) and set it to a temp-dir-scoped path in the test COMMON_ENV.

2. Test 27 init guard (silent set-e abort)
Test 27 creates a pre-wsId workspace (flat host/mighost branch) to test the migration path. But it doesn't write the .git/info/exclude marker that _assert_sync_initialized() requires. The script exits 1, set -euo pipefail in the test aborts silently (no FAIL printed), and tests 27–28 never run — so the test exits with fail=1 from this abort.

Fix: write # Generated by sync-workspace.sh — do not edit by hand. to .git/info/exclude in the test 27 setup subshell. A real pre-wsId workspace would have this marker since sync-workspace.sh has always written it.

Checklist

  • Confirmed no other open PR closes the same issue
  • Git author email matches CLA-signed email
  • Single concern per PR — two fixes but both are for the same test file / same CI entry
  • Confirmed failures reproduce on upstream/main before fix
  • All 88 assertions pass locally after fix

Test plan

bash tests/sync-workspace.test.sh
# Total: 88 — pass: 88, fail: 0

Run several times to confirm no timing-dependent failures (lock isolation eliminates the race).

Two independent root causes for CI failures in tests/sync-workspace.test.sh:

1. Lock contention (tests 8, 9, 10): the default lock dir
   `/tmp/sync-workspace.lock.d` was shared between test invocations and
   the live `sync-workspace` cron. When the cron fired mid-test it
   grabbed the lock and subsequent tests exited with "another instance
   is running, skipping."  Fix: `LOCK_DIR="${SUTANDO_SYNC_LOCK_DIR:-…}"`
   in the script + `SUTANDO_SYNC_LOCK_DIR="$TEST_ROOT/sync.lock.d"` in
   the test COMMON_ENV.

2. Test 27 init guard (exit via set -e): test 27 creates a pre-wsId
   workspace (flat `host/mighost` branch) without the `.git/info/exclude`
   marker that `_assert_sync_initialized()` requires.  The script exits 1,
   `set -euo pipefail` in the test aborts silently without printing FAIL,
   and tests 27-28 never run.  Fix: write the "Generated by
   sync-workspace.sh" marker during test 27 setup to simulate a
   legitimately-initialized-but-old workspace (which would have had it).

All 88 assertions now pass on the first try locally.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EL2XBcV3RsWhevzTPtW4jw
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Related: PR #1997 (fix(sync-workspace): allow pre-wsId flat branch past init guard) takes a complementary approach to the test 27 fix here — it adds a third sentinel in _assert_sync_initialized() (flat host/<hostname> HEAD passes) rather than writing the exclude marker in the test setup. Both work; they fix the same test 27 failure via different mechanisms.

The lock contention fix (SUTANDO_SYNC_LOCK_DIR env hook in sync-workspace.sh + isolated lock dir in test COMMON_ENV) is unique to this PR and not covered by #1997.

Suggested merge order: #1997 first, then this PR — or drop the test-27 exclude-marker hunk from this PR if #1997 lands first (the lock contention fix stands on its own).

@john-the-dev
john-the-dev marked this pull request as ready for review July 8, 2026 17:27

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed — LGTM. Parameterizing LOCK_DIR (default unchanged) is the minimal correct fix for test cross-talk, and the test-27 change is right: a legitimately-initialized pre-wsId workspace would carry the "Generated by sync-workspace.sh" marker in .git/info/exclude, so writing it in the fixture is simulating reality, not dodging the guard. CI green.

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@cla-assistant check

@qingyun-wu
qingyun-wu merged commit dd9c8d5 into sonichi:main Jul 9, 2026
7 checks passed
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.

2 participants