Skip to content

fix(privacy): merge token vault writes across instances; thread-safe SQLite vault - #400

Merged
kevincostner17 merged 1 commit into
mainfrom
fix/token-vault-concurrency
Sep 15, 2026
Merged

kevincostner17 merged 1 commit into
mainfrom
fix/token-vault-concurrency

Conversation

@kevincostner17

@kevincostner17 kevincostner17 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes two problems with sharing the persistent token vaults.

  • JsonTokenVault merges and serialises writes. The vault used to load its file once and rewrite it from that in-memory copy on every put, so two instances on the same path overwrote each other's entries.
    • Each put/save now opens the file and takes an exclusive lock: fcntl.flock on POSIX, msvcrt.locking on byte 0 on Windows, or only the per-instance thread lock when neither is available.
    • Under that lock it always re-reads the file, merges it with the in-memory map, rewrites the file in place, and calls flush() + os.fsync() before unlocking.
    • A put whose entry is already on disk skips the rewrite.
    • __init__ reads under a shared lock, get reloads on a miss when the file has changed, and an empty file loads as an empty vault.
    • The docstring notes that a crash mid-write can leave the file incomplete.
  • SqliteTokenVault is thread-safe. It connects with check_same_thread=False, timeout=30.0, and an RLock serialises get, put, __len__ and close. A vault created in the main thread can therefore be passed to apply_privacy_policy running in a worker pool.

Only the two vault classes (plus a small lock helper next to them) change; tokenization, masking and key handling are untouched. The on-disk format is unchanged.

Behaviour changes:

  • An empty JSON vault file loads as an empty vault instead of raising JSONDecodeError.
  • save() keeps entries that other instances wrote.

Limitation: CI has no Windows job, so the msvcrt path is covered only by a stub-module test.

Rebased onto #399 (merged); the only overlap was the privacy.py import block.

Tests

New tests/test_token_vault_concurrency.py (17 tests, well under a second, tmp_path only):

  • JSON vault sharing:
    • Issue repro: two instances on one path keep both mappings.
    • Interleaved puts from two instances.
    • Instance B's get sees A's entry.
    • 8 threads, each with its own JsonTokenVault, and 8 threads sharing one instance: all entries present.
  • JSON vault behaviour:
    • An empty file loads.
    • Nothing is written before put.
    • A repeated put skips the rewrite.
    • save() keeps other instances' entries.
    • Tokenize round trip through two instances.
  • Locking:
    • Exclusive lock for writes and shared lock for reads (flock).
    • Windows byte-0 locking via a stubbed msvcrt.
    • Fallback with no lock module.
  • SQLite vault:
    • Issue repro: used from a ThreadPoolExecutor worker.
    • get/put/len from another thread.
    • 8 threads sharing one vault.
    • Close twice.

12 of the new tests fail on main, including both issue repros. The new file passed 5 runs in a row on each interpreter.

Verification

Closes #279

@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: cd4a222c-900a-45f1-844e-b90fdc258fb2


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

FreshData benchmark report — performance

  • freshdata: ?
  • python: ?
  • platform: ?
fixture n_rows n_cols p50 s p95 s peak MB repair % false-repair % preserve % trust monotonic export %

Authored-code reduction (Metric 6)

…SQLite vault

Root cause:
- JsonTokenVault loaded its file once in __init__ and every put rewrote the
  whole file from that in-memory copy, so two instances sharing a path
  overwrote each other's entries and those tokens could no longer be
  detokenized.
- SqliteTokenVault opened its connection with sqlite's default
  check_same_thread=True, so a vault created in one thread raised
  ProgrammingError when apply_privacy_policy ran it from a worker thread.

Fix:
- JsonTokenVault merges and serialises writes. put/save open the file with
  open(path, "a+"), take an exclusive fcntl.flock (msvcrt.locking on byte 0
  on Windows; the per-instance thread lock alone when neither module is
  importable), always re-read the file inside the lock, merge it with the
  in-memory map, rewrite it in place, then flush and fsync before unlocking.
  A put whose entry is already on disk skips the rewrite. __init__ reads
  under a shared lock, get reloads on a miss when the file changed, and an
  empty file loads as an empty vault. The docstring notes that a crash
  mid-write can leave the file incomplete.
- SqliteTokenVault connects with check_same_thread=False and timeout=30.0,
  and an RLock serialises get, put, __len__ and close.

Closes #279
@kevincostner17
kevincostner17 force-pushed the fix/token-vault-concurrency branch from f6f24ce to f418b00 Compare September 15, 2026 11:03
@kevincostner17
kevincostner17 merged commit 33985af into main Sep 15, 2026
32 of 33 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.

Token vaults are unsafe to share: JSON vault loses mappings, SQLite vault fails in threads

1 participant