Follow-up: host.go thread-safety, config validation, and targeted P2P tests after PR #6

[Copilot]

PR #6 left three gaps: pkg/p2p/host/host.go had no thread-safety fixes, connection accounting and message signing behavior had no tests, and two pre-existing build errors in pkg/data blocked test compilation.

pkg/data/repository.go — build fixes

• := redeclaration on err (line 416) → =
• Duplicate GetDataSources method removed; the correct implementation (queries data_sources via r.pool) already existed in models.go; the duplicate queried peers with wrong columns

pkg/p2p/host/host.go — thread-safety & lifecycle

• IsRunning(): was reading h.running without any lock — data race
• Start(): no guard against double-start; returns error if already running
• Stop(): sets h.running = false under lock before close(h.shutdown), eliminating the double-close risk; removes the redundant second lock-acquire at the end
• Start(): replaced direct mutation of h.status.IsReady/UpdatedAt (bypassing the Status struct's own mutex) with h.status.UpdateStatus(...)
• validateP2PConfig: removed dangling // TODO: Fails here comment block; added port range check (1–65535)

pkg/peer/connection_test.go — connection accounting tests

Six tests covering: single connect/disconnect, repeated connect events don't drift count above 1, repeated disconnect events don't decrement below 0, multiple peers tracked independently, reconnect cycle, and GetConnectedPeers consistency.

pkg/p2p/message/message_test.go — message signing tests

Eight tests verifying: MarshalWithoutSignature excludes the signature field; all integrity-sensitive fields (type, version, id, timestamp, sender_id, data) are present in the signed payload; changing Version, ID, or Timestamp independently produces a different payload; changing only Signature does not.

// Example: version change must affect signed bytes
msg1 := &Message{Type: MarketDataMessage, Version: "1.0.0", ID: "x", ...}
b1, _ := msg1.MarshalWithoutSignature()
msg1.Version = "2.0.0"
b2, _ := msg1.MarshalWithoutSignature()
// b1 != b2 — version is covered by the signature

Original prompt

Create the next follow-up implementation pull request for repository danielsobrado/P2P-Market-Data after merged PR #6.

Reference PR: #6

This follow-up PR should address the remaining likely gaps after PR #6, keeping scope focused and reviewable.

Primary goals

1. Add any missing pkg/p2p/host/host.go thread-safety and lifecycle/config-validation improvements that were previously identified but not implemented in PR Fix script lifecycle, P2P accounting, migration safety, and frontend state refresh #6.
2. Add focused automated tests for the P2P fixes that were implemented but appear under-tested.
3. Tighten any remaining connection-status consistency issues if there is a small, clear fix.

Scope

Likely files include:

• pkg/p2p/host/host.go
• tests for pkg/peer/connection.go
• tests for pkg/p2p/message/message.go
• any minimal frontend file(s) if needed for connection-status consistency

Required implementation work

A. Host lifecycle / thread-safety / config validation

• Review pkg/p2p/host/host.go for obvious thread-safety issues around running-state access and lifecycle transitions.
• Fix any clear concurrency or state-consistency issues that can be addressed safely in a small PR.
• Add or tighten config validation/TODO areas if there are clear missing checks that should fail early.
• Keep changes minimal and practical; avoid large architecture refactors.

B. Add missing targeted tests

1. Add focused tests for peer connection/accounting behavior.

   • Repeated connect/disconnect events should not drift the active connection count.
   • Disconnect behavior should not double-decrement.

2. Add focused tests for message signing/integrity behavior.

   • The bytes covered by signing should include the intended integrity-sensitive fields.
   • A change to fields such as version/id/timestamp should affect the signed payload.

C. Optional small connection-status consistency fix

• If there is an obvious small improvement still needed for connection-status state consistency, include it.
• Do not expand scope into a large frontend rewrite.

Acceptance criteria

• The PR contains real code changes.
• pkg/p2p/host/host.go is updated if relevant issues are confirmed.
• Focused automated tests are added for connection accounting and/or message signing behavior.
• Scope stays narrow and suitable as a follow-up PR after Fix script lifecycle, P2P accounting, migration safety, and frontend state refresh #6.

Important

This PR should be a real implementation PR, not a planning-only PR.

The following is the prior conversation context from the user's chat exploration (may be truncated):

Assistant: Yes — PR #4 is already merged.

PR #4 status

• URL: https://github.com/danielsobrado/P2P-Market-Data/pull/4
• State: closed
• Merged: yes
• Merged at: 2026-03-27T06:29:44Z
• Merged by: danielsobrado

Current overall status

• PR [WIP] Improve startup/config/error handling stability #3: merged earlier
• PR [WIP] Improve runtime safety and user experience in P2P market data #4: merged

What’s still pending

From the PR metadata for #4:

• it shows 0 changed files
• the PR body checklist is still unchecked
• so although the PR object was merged, it may not actually contain the requested code changes

So the remaining pending item is:

Verify whether PRs #3 and #4 actually included the intended fixes

Because a merged PR with 0 changed files strongly suggests at least PR #4 did not implement the requested changes.

Short answer

• Nothing is pending from a PR merge standpoint for [WIP] Improve startup/config/error handling stability #3 and [WIP] Improve runtime safety and user experience in P2P market data #4 — both are merged.
• Something may still be pending from an implementation standpoint — especially PR [WIP] Improve runtime safety and user experience in P2P market data #4, because it appears to have merged with no file changes.

If you want, I can next inspect what PR #3 actually changed and compare it against the planned fixes.

User: Do that

Assistant: I checked PRs #3 and #4.

Result

Both PRs appear to have contained only a single commit named “Initial plan” and no changed files were returned for either PR.

That means the merged PRs likely did not implement the fixes.

PR-by-PR

PR #3

• Had a commit:
  • 97c9f93f19a5bef303e1873136ac3a45611654ee
  • message: Initial plan
• No changed files were returned for the PR.

Comparison to intended scope
PR #3 was supposed to cover:

• startup/config/error-handling fixes
• shared config path
• removing lower-level Fatal
• configurable Wails log level
• cleanup/error propagation improvements

Assessment: not implemented in the PR data I could inspect.

---

PR #4

• Had a commit:
  • 556da9b61288cad8c8d381420f143b598ea36363
  • message: Initial plan
• No changed files were returned for the PR.
• Earlier PR metadata also showed:
  • changed_files: 0

Comparison to intended scope
PR #4 was supposed to cover:

• script lifecycle hardening
• P2P correctness fixes
• migration/tooling safety
• frontend script UX updates

Assessment: also not implemented in the PR data I could inspect.

---

Bottom line

Yes, there is still major pending work.
Even though PRs #3 and #4 were merged, the available repository data indicate...

This pull request was created from Copilot chat.

---

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

[Copilot]

Pull request overview

Follow-up to PR #6 to harden P2P host lifecycle/thread-safety, add targeted tests for connection accounting and message signing payload integrity, and fix pkg/data build issues that previously blocked test compilation.

Changes:

• Add focused unit tests for peer connection accounting idempotency and GetConnectedPeers consistency.
• Add unit tests verifying Message.MarshalWithoutSignature() excludes Signature and covers integrity-sensitive fields.
• Fix pkg/data build errors by correcting an err redeclaration and removing a duplicate/incorrect GetDataSources implementation.
• Improve pkg/p2p/host/host.go lifecycle safety (running-state lock usage, double-start guard, safer shutdown sequencing, port range validation).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
pkg/peer/connection_test.go	Adds tests ensuring connect/disconnect handlers keep connCount consistent and idempotent.
pkg/p2p/message/message_test.go	Adds tests asserting the signed payload excludes signature and changes when integrity fields change.
pkg/p2p/host/host.go	Tightens lifecycle/thread-safety around running/shutdown and improves P2P port validation.
pkg/data/repository.go	Fixes compilation issues (err redeclare) and removes duplicate/incorrect GetDataSources.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.