Skip to content

fix: make ChronoTerm singleton lazy to avoid import-time crash#17

Closed
michaelxer wants to merge 2 commits into
TruFoundation:mainfrom
michaelxer:fix-chrono-singleton-import-crash
Closed

fix: make ChronoTerm singleton lazy to avoid import-time crash#17
michaelxer wants to merge 2 commits into
TruFoundation:mainfrom
michaelxer:fix-chrono-singleton-import-crash

Conversation

@michaelxer
Copy link
Copy Markdown

@michaelxer michaelxer commented Jun 3, 2026

Summary

Fixes #9

The module-level chrono = ChronoTerm() at the bottom of shell.py runs at import time, causing several problems:

  1. Just importing the module in a test spins up a real background thread
  2. If the state file is corrupted or the directory isn't writable, the entire import fails
  3. It's impossible to mock the ChronoTerm initializer in tests

Changes

trushell/chronoterm/shell.py — Replace module-level singleton with lazy initialization:

# Before
chrono = ChronoTerm()

# After
_chrono: ChronoTerm | None = None

def _get_chrono() -> ChronoTerm:
    """Lazy singleton: create ChronoTerm on first access, not at import time."""
    global _chrono
    if _chrono is None:
        _chrono = ChronoTerm()
    return _chrono

All function bodies now call _get_chrono() instead of using the module-level chrono variable.

Impact

  • Tests can now import the module without triggering side effects
  • StateStore errors no longer crash the import
  • Mocking is possible by setting _chrono before calling functions
  • No behavioral change for normal usage — the singleton is still created on first access

Test plan

  • Run pytest tests/ and verify all tests pass
  • Verify the module can be imported without side effects
  • Verify the ChronoTerm singleton is created on first access

Fixes TruFoundation#9

The module-level  at the bottom of shell.py
runs at import time: it calls StateStore().load(), creates a Stopwatch,
creates an AlarmManager, and immediately calls start_scheduler() which
spawns a background daemon thread.

This means:
- Just importing the module in a test spins up a real background thread
- If the state file is corrupted or the directory isn't writable, the
  entire import fails
- It's impossible to mock the ChronoTerm initializer in tests

Replace with a lazy singleton pattern:  creates the
instance on first access, not at import time.

Co-authored-by: Codex <codex@openai.com>
@AkshajSinghal
Copy link
Copy Markdown
Collaborator

Thanks for the PR. A few blockers before we can merge:

  • Thread safety: Need a lock around the lazy init to prevent race conditions.
  • Tests: Please add specific tests verifying the lazy behavior (not created on import, created on first access, singleton preserved).
  • Error handling: Wrap the instantiation in try/except so we don't trade an import crash for a confusing runtime error.
  • Commit message: Needs more context, what issue does this solve?

Let me know if you have questions!

@michaelxer
Copy link
Copy Markdown
Author

michaelxer commented Jun 3, 2026

Closing this for now because I don't have time to finish the review follow-up properly. Sorry for the churn, and thank you for the detailed review.

@michaelxer michaelxer closed this Jun 3, 2026
@AkshajSinghal
Copy link
Copy Markdown
Collaborator

@michaelxer don't bail on this. the logic was solid, you just missed the lock. That’s a 5 min fix, not a reason to nuke the PR.
I've reopened it, slap a threading.Lock on the init, add one test to prove it doesn’t race, and I’ll merge it.
Don’t let the nitpicking kill the contribution. give it another shot. 🐧

@AkshajSinghal AkshajSinghal reopened this Jun 3, 2026
@AkshajSinghal AkshajSinghal marked this pull request as ready for review June 3, 2026 12:20
The lazy singleton refactor removed the module-level 'chrono' name,
but chronoterm/__init__.py still imports it. Add a __getattr__ hook
so 'from trushell.chronoterm.shell import chrono' works via lazy
instantiation.

All 40 tests pass locally.
@AkshajSinghal
Copy link
Copy Markdown
Collaborator

Fix this:

  1. Thread Safety: Your current if _chrono is None check isn’t atomic. If two threads hit it at once, you get two instances. Wrap the creation in a threading.Lock (double-check pattern).
  2. Proof: Add one test that asserts _chrono is None before calling _get_chrono() and an instance after.

@michaelxer
Copy link
Copy Markdown
Author

michaelxer commented Jun 3, 2026

Closing this for now because I don't have time to finish the thread-safety and test changes properly. Sorry for the churn, and thank you for the detailed review.

@michaelxer michaelxer closed this Jun 3, 2026
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.

shell.py creates a module-level ChronoTerm singleton which crashes on import in tests

2 participants