Skip to content

refactor(slack-bridge): mtime-based cache for load_allowed/load_tier_map (closes #894) - #1186

Closed
bassilkhilo-ag2 wants to merge 1 commit into
sonichi:mainfrom
bassilkhilo-ag2:fix/slack-bridge-access-cache-894
Closed

bassilkhilo-ag2 wants to merge 1 commit into
sonichi:mainfrom
bassilkhilo-ag2:fix/slack-bridge-access-cache-894

Conversation

@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator

Summary

  • load_allowed() and load_tier_map() both read access.json on every _write_task call — O(N) I/O for N Slack events in a long-running daemon
  • Introduce _load_access_json(): checks stat().st_mtime before re-parsing; only re-reads when the file changes
  • Both helpers become thin wrappers returning [0] / [1] from the cached tuple
  • The None vs set() TOFU contract is preserved (allowed = None when file absent, in the shared helper)

Tests

Test plan

  • python3 tests/slack-bridge-tier-map.test.py → 19 passed
  • python3 tests/slack-bridge-access.test.py → 1 passed

🤖 Generated with Claude Code

…map (closes sonichi#894)

Both helpers read access.json on every _write_task invocation — wasteful
for a long-running daemon even at low Slack volume. Introduce _load_access_json()
that checks stat().st_mtime before re-parsing; load_allowed() and load_tier_map()
are now thin wrappers. The None-vs-set TOFU contract is preserved (file absent
→ allowed=None in the shared helper).

Tests: 19/19 pass in slack-bridge-tier-map.test.py (5 new cache cases);
slack-bridge-access.test.py updated to accept both the pre-sonichi#894 direct-return
shape and the new helper-delegation shape.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@VasiliyRad VasiliyRad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cache logic is correct. The stat/read outside the lock is safe — a race between stat and read at best causes one extra parse on a file-changed boundary, which is harmless and self-correcting on the next call. TOFU contract preserved: allowed=None on missing file flows through correctly. Tests cover the four meaningful states (cache hit, mtime bump, file deleted). Scope is clean — only src/slack-bridge.py and its two test files.

@VasiliyRad VasiliyRad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Clean refactor. _load_access_json() reads + parses once per mtime tick; both load_allowed() and load_tier_map() are thin delegates now. Thread-safety via _access_cache_lock, OSError handling preserved, and the load-bearing None-vs-empty-set distinction for TOFU eligibility is intact (file-missing → None, parse-error → empty set). Tests updated to match the new shape. 3 files only, no bundled changes.

LGTM 👍

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