Mirrored from https://github.com/247TaxiDriver/QOSami-colab-cli.git
Synced Nov 5, 23:30 UTC
fix: use cross-platform filelock instead of POSIX-only fcntl (#53)
fcntl is unavailable on Windows, so _LockedFileStore crashed on import there. Switch to the filelock library's ReadWriteLock, preserving the LOCK_SH/LOCK_EX distinction (shared reads via read_lock, exclusive writes via write_lock). Construct the lock with is_singleton=False so two StateStore instances for the same path in one process don't collapse into a single reentrant lock, whose reentrancy guard would otherwise raise RuntimeError under multi-threaded write contention. Add regression tests for cross-process exclusion, concurrent readers, and multi-thread writes, and document the locking design in docs/01_session_management.md.
Tyler committed
Jun 10, 2026 at 12:07 UTC
65a3d0248b540dffbbe2aa6638e886dfb7e7d385
5 files changed
+247
-11
docs/01_session_management.md
+16
@@ -1,3 +1,8 @@
1
+---
2
+log:
3
+2026-06-10: Replaced the POSIX-only `fcntl.flock` file locking in `_LockedFileStore` with the cross-platform `filelock` library (reported broken on Windows). Reads use `ReadWriteLock.read_lock()` (shared) and writes use `write_lock()` (exclusive), preserving the original `LOCK_SH`/`LOCK_EX` semantics. The lock is constructed with `is_singleton=False` so two `StateStore` instances for the same path in one process don't collapse into a single reentrant lock (which would raise `RuntimeError` on multi-threaded write contention). Added shared-read, cross-process exclusion, and multi-thread/multi-process regression tests.
4
+---
5
+
6
# Design: Session Management (`new`, `status`, `stop`, `sessions`)
7
8
## Overview
@@ -88,6 +93,12 @@ To prevent Colab VMs from being deleted due to idle timeouts (standard is ~90 mi
93
- Use `requests` for robust HTTP interactions and `pydantic` for schema validation.
94
- Handle authentication headers (likely `Authorization: Bearer <token>` or cookies).
95
96
+### State Persistence & File Locking
97
+- **Stores**: `StateStore` (`sessions.json`) and `SettingsStore` (`settings.json`) both derive from `_LockedFileStore`, which guards concurrent access across independent `colab` invocations and the detached keep-alive daemon.
98
+- **Cross-platform locking**: Locking uses the [`filelock`](https://pypi.org/project/filelock/) library rather than `fcntl.flock`. `fcntl` is POSIX-only and is unavailable on Windows, so the original implementation crashed on import there. `filelock` provides the same advisory cross-process locking on Linux, macOS, and Windows.
99
+- **Shared vs. exclusive**: Each store owns a `filelock.ReadWriteLock` bound to a sidecar file (`<path>.lock`). Reads acquire `read_lock()` (shared — multiple concurrent readers allowed) and writes acquire `write_lock()` (exclusive). This preserves the `LOCK_SH`/`LOCK_EX` distinction of the previous `fcntl` implementation.
100
+- **`is_singleton=False`**: The `ReadWriteLock` is created with `is_singleton=False`. With `filelock`'s default (`True`), two `ReadWriteLock` objects for the same path *within a single process* are deduplicated into one reentrant lock; its reentrancy guard then raises `RuntimeError` when two threads each construct their own `StateStore` and contend for the write lock. Disabling the singleton registry makes each store's lock independent so they serialize via the underlying file lock instead.
101
+
102
## Testing Strategy
103
TDD is mandatory for all session management features.
104
@@ -100,3 +111,8 @@ TDD is mandatory for all session management features.
111
### 2. State Store Validation
112
- **Test Case**: Verify `StateStore` correctly handles file locking and multiple concurrent reads/writes.
113
- **Test Case**: Verify `--config` override correctly directs all operations to the specified file path.
114
+- **Test Case (cross-platform locking)**: Verify the store locks via `filelock.ReadWriteLock` on the `<path>.lock` sidecar and does not import the POSIX-only `fcntl`.
115
+- **Test Case (shared/exclusive semantics)**: Verify reads go through `read_lock()` and writes through `write_lock()`.
116
+- **Test Case (cross-process exclusion)**: Hold the write lock from a separate process and confirm the store's in-process write blocks until release.
117
+- **Test Case (concurrent readers)**: Hold a read lock from a separate process and confirm the store can still complete a read concurrently.
118
+- **Test Case (multi-thread regression)**: Two `StateStore` instances writing from different threads must serialize without raising `RuntimeError` (guards the `is_singleton=False` choice).
pyproject.toml
+1
@@ -24,6 +24,7 @@ classifiers = [
24
]
25
dependencies = [
26
"click>=8.0",
27
+ "filelock>=3.29.2",
28
"google-auth>=2.49.1",
29
"google-auth-oauthlib>=1.3.0",
30
"jupyter-kernel-client",
src/colab_cli/state.py
+15
-11
@@ -15,9 +15,10 @@
15
import contextlib
16
import json
17
import os
18
-import fcntl
18
from datetime import datetime
19
from typing import Dict, Optional, Tuple, Iterator, IO
20
+
21
+import filelock
22
from pydantic import BaseModel
23
24
@@ -46,6 +47,15 @@ class Settings(BaseModel):
47
class _LockedFileStore:
48
def __init__(self, path: str):
49
self.path = path
50
+ self.lock_path = "%s.lock" % self.path
51
+ # ReadWriteLock gives us shared (concurrent) readers and exclusive
52
+ # writers -- the cross-platform equivalent of fcntl LOCK_SH/LOCK_EX.
53
+ # is_singleton=False keeps each store's lock independent: with the
54
+ # default (True), two StateStore instances for the same path in one
55
+ # process are merged into a single reentrant lock, whose reentrancy
56
+ # guard then raises RuntimeError when two threads contend for the write
57
+ # lock. We want them to actually serialize via the underlying file lock.
58
+ self._rwlock = filelock.ReadWriteLock(self.lock_path, is_singleton=False)
59
self._ensure_dir()
60
61
def _ensure_dir(self):
@@ -63,21 +73,15 @@ class _LockedFileStore:
73
if not os.path.exists(self.path):
74
yield None
75
return
66
- with open(self.path, "r") as f:
67
- fcntl.flock(f, fcntl.LOCK_SH)
68
- try:
76
+ with self._rwlock.read_lock():
77
+ with open(self.path, "r") as f:
78
yield f
70
- finally:
71
- fcntl.flock(f, fcntl.LOCK_UN)
79
80
@contextlib.contextmanager
81
def _lock_exclusive(self) -> Iterator[IO]:
75
- with open(self.path, "a+") as f:
76
- fcntl.flock(f, fcntl.LOCK_EX)
77
- try:
82
+ with self._rwlock.write_lock():
83
+ with open(self.path, "a+") as f:
84
yield f
79
- finally:
80
- fcntl.flock(f, fcntl.LOCK_UN)
85
86
87
class SettingsStore(_LockedFileStore):
tests/test_state.py
+204
@@ -12,11 +12,15 @@
12
# See the License for the specific language governing permissions and
13
# limitations under the License.
14
15
+import multiprocessing
16
import os
17
import pytest
18
import tempfile
19
import threading
20
from datetime import datetime
21
+
22
+import filelock
23
+
24
from colab_cli.state import StateStore, SessionState, SettingsStore, Settings
25
26
@@ -121,3 +125,203 @@ def test_settings_store_save_load(temp_config):
125
loaded = SettingsStore(temp_config).load()
126
assert loaded.enable_update_check is False
127
assert loaded.last_check == datetime(2026, 1, 1)
128
+
129
+
130
+# --- Cross-platform locking (filelock ReadWriteLock) behavior ---------------
131
+
132
+
133
+def test_lock_path_is_derived_from_path(temp_config):
134
+ """The lock sidecar file lives next to the data file with a .lock suffix."""
135
+ store = StateStore(temp_config)
136
+ assert store.lock_path == temp_config + ".lock"
137
+
138
+
139
+def test_store_uses_readwrite_lock_on_sidecar(temp_config):
140
+ """The store holds a ReadWriteLock bound to the sidecar path."""
141
+ store = StateStore(temp_config)
142
+ assert isinstance(store._rwlock, filelock.ReadWriteLock)
143
+ assert store._rwlock.lock_file == temp_config + ".lock"
144
+
145
+
146
+def test_separate_store_instances_can_write_from_threads(temp_config):
147
+ """Two StateStore instances writing from different threads must not crash.
148
+
149
+ Regression guard for filelock's is_singleton=True default: it merges two
150
+ same-path ReadWriteLock objects into one reentrant lock whose reentrancy
151
+ guard raises RuntimeError when two threads contend for the write lock. We
152
+ pass is_singleton=False so they serialize via the underlying file lock.
153
+ """
154
+ errors = []
155
+
156
+ def writer(start):
157
+ store = StateStore(temp_config)
158
+ try:
159
+ for i in range(start, start + 25):
160
+ store.add(SessionState(name=f"n{i}", token="t", url="u", endpoint="e"))
161
+ except Exception as exc: # pragma: no cover - failure path
162
+ errors.append(repr(exc))
163
+
164
+ threads = [threading.Thread(target=writer, args=(s,)) for s in (0, 100)]
165
+ for t in threads:
166
+ t.start()
167
+ for t in threads:
168
+ t.join()
169
+
170
+ assert errors == []
171
+ assert len(StateStore(temp_config).list()) == 50
172
+
173
+
174
+def test_write_acquires_write_lock(temp_config, mocker):
175
+ """Writes go through the ReadWriteLock's exclusive write_lock()."""
176
+ store = StateStore(temp_config)
177
+ spy = mocker.spy(store._rwlock, "write_lock")
178
+ store.add(SessionState(name="s", token="t", url="u", endpoint="e"))
179
+ assert spy.call_count >= 1
180
+ assert store.get("s") is not None
181
+
182
+
183
+def test_read_acquires_read_lock(temp_config, mocker):
184
+ """Reads go through the ReadWriteLock's shared read_lock()."""
185
+ store = StateStore(temp_config)
186
+ store.add(SessionState(name="s", token="t", url="u", endpoint="e"))
187
+
188
+ spy = mocker.spy(store._rwlock, "read_lock")
189
+ assert store.get("s") is not None
190
+ assert spy.call_count >= 1
191
+
192
+
193
+def test_uses_filelock_not_fcntl(temp_config):
194
+ """The store must lock via the platform-independent filelock library.
195
+
196
+ Guards against a regression back to the POSIX-only fcntl.flock approach
197
+ that broke Windows users.
198
+ """
199
+ store = StateStore(temp_config)
200
+ assert store._rwlock.__class__.__module__.startswith("filelock")
201
+
202
+
203
+def test_no_fcntl_import_in_state_module():
204
+ """The state module must not depend on the POSIX-only fcntl module."""
205
+ import colab_cli.state as state_module
206
+
207
+ assert not hasattr(state_module, "fcntl")
208
+
209
+
210
+def _mp_hold_write_lock(lock_path, hold_for, acquired_evt, release_evt):
211
+ rw = filelock.ReadWriteLock(lock_path)
212
+ with rw.write_lock():
213
+ acquired_evt.set()
214
+ release_evt.wait(timeout=hold_for)
215
+
216
+
217
+def test_write_lock_blocks_across_processes(temp_config):
218
+ """A write lock held by another process must block the store's write section.
219
+
220
+ The cross-process guarantee is the whole point of the fcntl->filelock
221
+ switch, so we hold the write lock from a separate process and confirm the
222
+ in-process writer cannot proceed until it's released.
223
+ """
224
+ store = StateStore(temp_config)
225
+ # Pre-create the data file so add() doesn't race on first creation.
226
+ store.add(SessionState(name="seed", token="t", url="u", endpoint="e"))
227
+
228
+ ctx = multiprocessing.get_context("spawn")
229
+ acquired = ctx.Event()
230
+ release = ctx.Event()
231
+ holder = ctx.Process(
232
+ target=_mp_hold_write_lock, args=(store.lock_path, 10, acquired, release)
233
+ )
234
+ holder.start()
235
+ try:
236
+ assert acquired.wait(timeout=5), "holder process never acquired the lock"
237
+
238
+ finished = threading.Event()
239
+
240
+ def writer():
241
+ store.add(SessionState(name="blocked", token="t", url="u", endpoint="e"))
242
+ finished.set()
243
+
244
+ t = threading.Thread(target=writer)
245
+ t.start()
246
+
247
+ # While the external process holds the write lock, the writer can't finish.
248
+ assert not finished.wait(timeout=0.75)
249
+
250
+ release.set()
251
+ assert finished.wait(timeout=5)
252
+ t.join()
253
+ assert store.get("blocked") is not None
254
+ finally:
255
+ release.set()
256
+ holder.join(timeout=5)
257
+
258
+
259
+def _mp_hold_read_lock(lock_path, acquired_evt, release_evt):
260
+ rw = filelock.ReadWriteLock(lock_path)
261
+ with rw.read_lock():
262
+ acquired_evt.set()
263
+ release_evt.wait(timeout=10)
264
+
265
+
266
+def test_readers_are_concurrent_across_processes(temp_config):
267
+ """Shared read locks must allow concurrent readers (the LOCK_SH semantics).
268
+
269
+ This is the capability ReadWriteLock buys us over a plain exclusive
270
+ FileLock: while one process holds a read lock, this process can still read.
271
+ """
272
+ store = StateStore(temp_config)
273
+ store.add(SessionState(name="s", token="t", url="u", endpoint="e"))
274
+
275
+ ctx = multiprocessing.get_context("spawn")
276
+ acquired = ctx.Event()
277
+ release = ctx.Event()
278
+ holder = ctx.Process(
279
+ target=_mp_hold_read_lock, args=(store.lock_path, acquired, release)
280
+ )
281
+ holder.start()
282
+ try:
283
+ assert acquired.wait(timeout=5), "holder process never acquired the read lock"
284
+
285
+ done = threading.Event()
286
+ result = {}
287
+
288
+ def reader():
289
+ result["value"] = store.get("s")
290
+ done.set()
291
+
292
+ t = threading.Thread(target=reader)
293
+ t.start()
294
+
295
+ # The read must complete even though another process holds a read lock.
296
+ assert done.wait(timeout=3), "concurrent read was blocked by another reader"
297
+ t.join()
298
+ assert result["value"] is not None
299
+ finally:
300
+ release.set()
301
+ holder.join(timeout=5)
302
+
303
+
304
+def _mp_add_sessions(path, start, count):
305
+ store = StateStore(path)
306
+ for i in range(start, start + count):
307
+ store.add(SessionState(name=f"p{i}", token="t", url="u", endpoint="e"))
308
+
309
+
310
+def test_state_store_multiprocess_concurrency(temp_config):
311
+ """filelock must serialize writes across separate processes (not just threads).
312
+
313
+ fcntl.flock is per-open-file-description and advisory; this test exercises
314
+ the cross-process guarantee that motivated the switch.
315
+ """
316
+ ctx = multiprocessing.get_context("spawn")
317
+ p1 = ctx.Process(target=_mp_add_sessions, args=(temp_config, 0, 40))
318
+ p2 = ctx.Process(target=_mp_add_sessions, args=(temp_config, 40, 40))
319
+
320
+ p1.start()
321
+ p2.start()
322
+ p1.join()
323
+ p2.join()
324
+
325
+ assert p1.exitcode == 0
326
+ assert p2.exitcode == 0
327
+ assert len(StateStore(temp_config).list()) == 80
uv.lock
+11
@@ -276,6 +276,15 @@ wheels = [
276
{ url = "https://files.pythonhosted.org/packages/cb/a8/20d0723294217e47de6d9e2e40fd4a9d2f7c4b6ef974babd482a59743694/fastjsonschema-2.21.2-py3-none-any.whl", hash = "sha256:1c797122d0a86c5cace2e54bf4e819c36223b552017172f32c5c024a6b77e463", size = 24024, upload-time = "2025-08-14T18:49:34.776Z" },
277
]
278
279
+[[package]]
280
+name = "filelock"
281
+version = "3.29.2"
282
+source = { registry = "https://pypi.org/simple" }
283
+sdist = { url = "https://files.pythonhosted.org/packages/ec/fa/b7dc8a63243aafe57bf53611bd26fa3e0dee2d613590656d44e0a59b7828/filelock-3.29.2.tar.gz", hash = "sha256:779d2f5443b584750c6b90457abffd49235bfb0e66ce82ef5a680867e518ca1c", size = 61449, upload-time = "2026-06-10T14:45:23.072Z" }
284
+wheels = [
285
+ { url = "https://files.pythonhosted.org/packages/45/c1/91b0faf8de6938ae72da425e0c6f322d270c044e5ffc372e6f0f89ed1564/filelock-3.29.2-py3-none-any.whl", hash = "sha256:f5d3feb44b2b8824832587543af5226822fe86baf086678ede47aa177fe47ca5", size = 42129, upload-time = "2026-06-10T14:45:21.896Z" },
286
+]
287
+
288
[[package]]
289
name = "google-auth"
290
version = "2.49.1"
@@ -307,6 +316,7 @@ name = "google-colab-cli"
316
source = { editable = "." }
317
dependencies = [
318
{ name = "click" },
319
+ { name = "filelock" },
320
{ name = "google-auth" },
321
{ name = "google-auth-oauthlib" },
322
{ name = "jupyter-kernel-client" },
@@ -333,6 +343,7 @@ dev = [
343
[package.metadata]
344
requires-dist = [
345
{ name = "click", specifier = ">=8.0" },
346
+ { name = "filelock", specifier = ">=3.29.2" },
347
{ name = "google-auth", specifier = ">=2.49.1" },
348
{ name = "google-auth-oauthlib", specifier = ">=1.3.0" },
349
{ name = "jupyter-kernel-client", git = "https://github.com/googlecolab/jupyter-kernel-client.git" },