Code review

[tomasmach]

Summary by CodeRabbit

• Bug Fixes

  • Improved position management with enhanced validation to prevent invalid orders and duplicate positions.
  • Enhanced message publishing reliability with better error handling and state consistency.
  • Fixed startup state consistency to ensure reliable background task execution.

• Improvements

  • Strengthened data integrity checks during position and statistics loading.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request introduces defensive programming enhancements across three modules: reorganizes the startup sequence in main.py to ensure consistent state, adds atomic-state loading patterns and validation checks in paper_trading.py to prevent inconsistent engine state, and implements race condition prevention with a boolean return type in mqtt_publisher.py.

Changes

Cohort / File(s)	Summary
Startup Sequence main.py	Renamed position value from "locked" to "invested_amount" in balance update loop; reorganized initialization to set _running flag immediately after successful MQTT connection for consistent background task state.
Position Management src/core/paper_trading.py	Added early-exit validation in _OPEN_POSITION_ for missing/invalid order prices and duplicate position warnings; introduced atomic-state loading pattern in load_positions_ using temporary structures to prevent partial state updates; implemented atomic statistics assignment after validation.
MQTT Communication src/utils/mqtt_publisher.py	Changed _do_publish return type from None to bool; added atomic client capture with None check to prevent time-of-check-time-of-use race condition; returns True on successful publish, False if client is unavailable.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• add atomic position persistence for crash recovery #4: Modifies position loading and opening logic in src/core/paper_trading.py—this PR adds atomic-state loading and validation enhancements while the related PR introduces JSON persistence and position serialization methods, creating direct functional overlap.

Poem

🐰 A rabbit hops through code so fine,
Atomic states now all align,
No race conditions dare to peek,
While validations stand so sleek,
Investment amounts proudly shine! 💫

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Code review' is vague and generic, providing no meaningful information about what was actually changed in the pull request.	Use a descriptive title that clearly summarizes the main changes, such as 'Refactor position loading with atomic state patterns and add MQTT client race condition fix' or similar.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)

main.py (2)

255-266: Startup correctness: _running = True can be set even if Polymarket connect fails.

If await self.client.connect() fails, the instance may remain “running” (and shutdown() doesn’t clear _running), which can confuse background loops/state checks and future restarts.

Safer sequencing (set running after all required connects succeed)

         # Connect MQTT if enabled
         if self.mqtt_publisher:
             logger.info("connecting_to_mqtt_broker")
             await self.mqtt_publisher.connect()
 
-        # Set running flag immediately after connections succeed
-        # This ensures the flag is consistent before any background tasks start
-        self._running = True
-
         # Connect to Polymarket
         logger.info("connecting_to_polymarket")
         await self.client.connect()
+
+        # Mark running only after all startup connects succeed
+        self._running = True

---

202-225: Multiple MQTT payload fields changed without backward compatibility—breaking downstream consumers.

The renaming of "position_value" to "invested_amount" is confirmed, but this change is part of a larger breaking update: locked_in_positions and unrealized_pnl were removed, and update_reason changed from a string to a list. The outdated test file (test_mqtt.py) still reflects the old payload structure, confirming the risk.

Without versioning or a compatibility period, any external consumer (e.g., the Discord bot mentioned in README) will break. Consider publishing both old and new field names for one release while external integrations migrate.

Proposed compatibility approach

                     await self.mqtt_publisher.publish_balance_update({
                         "balance": current_balance,
                         "equity": total_equity,
                         "available_balance": available,
                         "total_pnl": self.paper_engine.total_pnl,
                         "position_count": len(self.paper_engine.positions),
                         "invested_amount": invested_amount,
+                        "position_value": invested_amount,  # Backward compat (deprecated, remove after migration)
                         "update_reason": update_reason
                     })

Also update test_mqtt.py to reflect current payload structure.

src/core/paper_trading.py (1)

360-389: Inconsistent state risk: invalid price returns early, but the order is still “FILLED”.

With the new guard, a BUY can produce a PaperOrder(status=FILLED) that never opens a position (and create_order() still logs "paper_order_filled"), which can silently desync stats/UX. Prefer validating current_price before constructing a FILLED order (or downgrade the order to REJECTED and avoid “filled” logging/return).

src/utils/mqtt_publisher.py (1)

250-279: _do_publish() returns bool but result is ignored; disconnect() can race with publish() since it doesn't hold _publish_lock.

The return value (line 278) is never used, so callers have no way to detect failure. More critically, disconnect() does not acquire _publish_lock, allowing it to call loop_stop() and disconnect() while a publish is in-flight on another thread. The atomic client capture prevents None dereference but not concurrent use of a disconnecting client. Also, info.wait_for_publish() timeout is ignored—if it returns False, the code logs success anyway.

Fix: (1) capture and check the boolean return from _do_publish(), (2) have disconnect() acquire _publish_lock to serialize against in-flight publishes, (3) check wait_for_publish() return value and log/return on timeout.

🧹 Nitpick comments (1)

src/core/paper_trading.py (1)

631-686: Atomic load looks good; consider clarifying return semantics for “empty but valid” files.

The temp-parse + atomic assignment pattern is solid, but return len(loaded_position_ids) > 0 or failed_positions == 0 makes an empty positions file return True, which may cause misleading “positions_restored” logs in main.py. Consider returning True only when the file is successfully parsed/applied, and let callers decide whether “restored” means > 0 positions.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bbced6b and 9ea7c62.

📒 Files selected for processing (3)

• main.py
• src/core/paper_trading.py
• src/utils/mqtt_publisher.py

🧰 Additional context used

🧬 Code graph analysis (1)

src/utils/mqtt_publisher.py (1)

src/core/client.py (1)

• client (142-148)