Fix Contact Retry Failure to Restart (#541)

commit daea5128c0 (HEAD -> post-stable-fixes, origin/post-stable-fixes)
Author: Sarah Jamie Lewis <sarah@openprivacy.ca>
Date:   Tue Jan 2 12:45:39 2024 -0800

    Fixup Connection Test to check reconnecting status

commit 347ac3cf48
Author: Sarah Jamie Lewis <sarah@openprivacy.ca>
Date:   Tue Jan 2 12:33:31 2024 -0800

    Fixup Formatting and Quality Script

    ineffassign and misspell are no longer compatible with previous
    go workflows and the latest versions do not work. Commenting for
    now with intent to replace with better tooling.

commit d9ce7737cc
Author: Sarah Jamie Lewis <sarah@openprivacy.ca>
Date:   Tue Jan 2 12:24:33 2024 -0800

    Fix Contact Retry Failure to Restart

    When toggling between connected and disconnected, the Contact Retry plugin
    could find itself in a state where the new event would never get requeued.

    Also: Make the unsigned nature of limit in GetMessage* Apis explicit.

Reviewed-on: #541
Reviewed-by: Dan Ballard <dan@openprivacy.ca>

.drone.yml

@@ -5,27 +5,29 @@ name: linux-test
 
 steps:
   - name: fetch
-    image: golang:1.19.1
+    image: golang:1.21.5
     volumes:
       - name: deps
         path: /go
     commands:
       - go install honnef.co/go/tools/cmd/staticcheck@latest
-      - wget https://git.openprivacy.ca/openprivacy/buildfiles/raw/master/tor/tor
-      - wget https://git.openprivacy.ca/openprivacy/buildfiles/raw/master/tor/torrc
-      - chmod a+x tor
-      - go get -u golang.org/x/lint/golint
+      - go install go.uber.org/nilaway/cmd/nilaway@latest
+      - wget https://git.openprivacy.ca/openprivacy/buildfiles/raw/branch/master/tor/tor-0.4.8.9-linux-x86_64.tar.gz -O tor.tar.gz
+      - tar -xzf tor.tar.gz
+      - chmod a+x Tor/tor
+      - export PATH=$PWD/Tor/:$PATH
+      - export LD_LIBRARY_PATH=$PWD/Tor/
+      - tor --version
       - export GO111MODULE=on
-      - go mod vendor
   - name: quality
-    image: golang:1.19.1
+    image: golang:1.21.5
     volumes:
       - name: deps
         path: /go
     commands:
-      - staticcheck ./...
+      - ./testing/quality.sh
   - name: units-tests
-    image: golang:1.19.1
+    image: golang:1.21.5
     volumes:
       - name: deps
         path: /go
@@ -33,28 +35,32 @@ steps:
       - export PATH=`pwd`:$PATH
       - sh testing/tests.sh
   - name: integ-test
-    image: golang:1.19.1
+    image: golang:1.21.5
     volumes:
       - name: deps
         path: /go
     commands:
-      - export PATH=`pwd`:$PATH
+      - export PATH=$PWD/Tor/:$PATH
+      - export LD_LIBRARY_PATH=$PWD/Tor/
+      - tor --version
       - go test -timeout=30m -race -v cwtch.im/cwtch/testing/
   - name: filesharing-integ-test
-    image: golang:1.19.1
+    image: golang:1.21.5
     volumes:
       - name: deps
         path: /go
     commands:
-      - export PATH=`pwd`:$PATH
+      - export PATH=$PWD/Tor/:$PATH
+      - export LD_LIBRARY_PATH=$PWD/Tor/
       - go test -timeout=20m -race -v cwtch.im/cwtch/testing/filesharing
   - name: filesharing-autodownload-integ-test
-    image: golang:1.19.1
+    image: golang:1.21.5
     volumes:
       - name: deps
         path: /go
     commands:
-      - export PATH=`pwd`:$PATH
+      - export PATH=$PWD/Tor/:$PATH
+      - export LD_LIBRARY_PATH=$PWD/Tor/
       - go test -timeout=20m -race -v cwtch.im/cwtch/testing/autodownload
   - name: notify-gogs
     image: openpriv/drone-gogs

app/plugins/contactRetry.go

@@ -302,8 +302,7 @@ func (cr *contactRetry) run() {
 				cr.authorizedPeers.Store(id, true)
 				if c, ok := cr.connections.Load(id); ok {
 					contact := c.(*contact)
-					if contact.state == connections.DISCONNECTED && !contact.queued {
-
+					if contact.state == connections.DISCONNECTED {
 						// prioritize connections made in the last week
 						if time.Since(contact.lastSeen).Hours() < PriorityQueueTimeSinceQualifierHours {
 							cr.priorityQueue.insert(contact)
@@ -461,10 +460,10 @@ func (cr *contactRetry) addConnection(id string, state connections.ConnectionSta
 		cr.connections.Store(id, p)
 		return
 	} else {
-		// we have rerequested this connnection. Force set the queued parameter to true.
-		p, _ := cr.connections.Load(id)
-		if !p.(*contact).queued {
-			p.(*contact).queued = true
+		// we have rerequested this connnection, probably via an explicit ask, update it's state
+		if c, ok := cr.connections.Load(id); ok {
+			contact := c.(*contact)
+			contact.state = state
 		}
 	}
 }

app/plugins/contactRetry_test.go

@@ -46,9 +46,11 @@ func TestContactRetryQueue(t *testing.T) {
 		}
 	}
 
+	// We should very quickly become connecting...
+	time.Sleep(time.Second)
 	pinf, _ := cr.connections.Load(testOnion)
-	if pinf.(*contact).queued == false {
-		t.Fatalf("test connection should be queued, actually: %v", pinf.(*contact).queued)
+	if pinf.(*contact).state != 1 {
+		t.Fatalf("test connection should be in connecting after update, actually: %v", pinf.(*contact).state)
 	}
 
 	// Asset that "test" is authenticated
@@ -63,19 +65,12 @@ func TestContactRetryQueue(t *testing.T) {
 	// Publish an unrelated event to trigger the Plugin to go through a queuing cycle
 	// If we didn't do this we would have to wait 30 seconds for a check-in
 	bus.Publish(event.NewEvent(event.PeerStateChange, map[event.Field]string{event.RemotePeer: "test2", event.ConnectionState: "Disconnected"}))
+	bus.Publish(event.NewEvent(event.QueuePeerRequest, map[event.Field]string{event.RemotePeer: testOnion, event.LastSeen: time.Now().Format(time.RFC3339Nano)}))
+
 	time.Sleep(time.Second)
-	if pinf.(*contact).queued != false {
-		t.Fatalf("test connection should not be queued, actually: %v", pinf.(*contact).queued)
-	}
-
-	// Publish a  new peer request...
-	bus.Publish(event.NewEvent(event.QueuePeerRequest, map[event.Field]string{event.RemotePeer: testOnion}))
-	time.Sleep(time.Second) // yield for a second so the event can catch up...
-
-	// Peer test should be forced to queue....
 	pinf, _ = cr.connections.Load(testOnion)
-	if pinf.(*contact).queued != true {
-		t.Fatalf("test connection should be forced to queue after new queue peer request")
+	if pinf.(*contact).state != 1 {
+		t.Fatalf("test connection should be in connecting after update, actually: %v", pinf.(*contact).state)
 	}
 
 	cr.Shutdown()

go.mod

@@ -16,6 +16,7 @@ require (
 require (
 	filippo.io/edwards25519 v1.0.0 // indirect
 	git.openprivacy.ca/openprivacy/bine v0.0.5 // indirect
+	github.com/client9/misspell v0.3.4 // indirect
 	github.com/google/go-cmp v0.5.8 // indirect
 	github.com/gtank/merlin v0.1.1 // indirect
 	github.com/mimoo/StrobeGo v0.0.0-20220103164710-9a04d6ca976b // indirect

go.sum

@@ -8,6 +8,8 @@ git.openprivacy.ca/openprivacy/connectivity v1.11.0 h1:roASjaFtQLu+HdH5fa2wx6F00
 git.openprivacy.ca/openprivacy/connectivity v1.11.0/go.mod h1:OQO1+7OIz/jLxDrorEMzvZA6SEbpbDyLGpjoFqT3z1Y=
 git.openprivacy.ca/openprivacy/log v1.0.3 h1:E/PMm4LY+Q9s3aDpfySfEDq/vYQontlvNj/scrPaga0=
 git.openprivacy.ca/openprivacy/log v1.0.3/go.mod h1:gGYK8xHtndRLDymFtmjkG26GaMQNgyhioNS82m812Iw=
+github.com/client9/misspell v0.3.4 h1:ta993UF76GwbvJcIo3Y68y/M3WxlpEHPWIGDkJYwzJI=
+github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=

peer/cwtch_peer.go

@@ -94,7 +94,7 @@ func (cp *cwtchPeer) EnhancedImportBundle(importString string) string {
 	return cp.ImportBundle(importString).Error()
 }
 
-func (cp *cwtchPeer) EnhancedGetMessages(conversation int, index int, count int) string {
+func (cp *cwtchPeer) EnhancedGetMessages(conversation int, index int, count uint) string {
 	var emessages = make([]EnhancedMessage, count)
 
 	messages, err := cp.GetMostRecentMessages(conversation, 0, index, count)
@@ -888,7 +888,7 @@ func (cp *cwtchPeer) GetChannelMessageCount(conversation int, channel int) (int,
 }
 
 // GetMostRecentMessages returns a selection of messages, ordered by most recently inserted
-func (cp *cwtchPeer) GetMostRecentMessages(conversation int, channel int, offset int, limit int) ([]model.ConversationMessage, error) {
+func (cp *cwtchPeer) GetMostRecentMessages(conversation int, channel int, offset int, limit uint) ([]model.ConversationMessage, error) {
 	return cp.storage.GetMostRecentMessages(conversation, channel, offset, limit)
 }
 

peer/cwtchprofilestorage.go

@@ -781,7 +781,7 @@ func (cps *CwtchProfileStorage) SearchMessages(conversation int, channel int, pa
 }
 
 // GetMostRecentMessages returns the most recent messages in a channel up to a given limit at a given offset
-func (cps *CwtchProfileStorage) GetMostRecentMessages(conversation int, channel int, offset int, limit int) ([]model.ConversationMessage, error) {
+func (cps *CwtchProfileStorage) GetMostRecentMessages(conversation int, channel int, offset int, limit uint) ([]model.ConversationMessage, error) {
 	channelID := ChannelID{Conversation: conversation, Channel: channel}
 
 	cps.mutex.Lock()

peer/profile_interface.go

@@ -131,7 +131,7 @@ type CwtchPeer interface {
 	GetChannelMessage(conversation int, channel int, id int) (string, model.Attributes, error)
 	GetChannelMessageCount(conversation int, channel int) (int, error)
 	GetChannelMessageByContentHash(conversation int, channel int, contenthash string) (int, error)
-	GetMostRecentMessages(conversation int, channel int, offset int, limit int) ([]model.ConversationMessage, error)
+	GetMostRecentMessages(conversation int, channel int, offset int, limit uint) ([]model.ConversationMessage, error)
 	UpdateMessageAttribute(conversation int, channel int, id int, key string, value string) error
 	SearchConversations(pattern string) string
 
@@ -142,7 +142,7 @@ type CwtchPeer interface {
 	EnhancedGetMessageByContentHash(conversation int, hash string) string
 
 	// EnhancedGetMessages returns a set of json-encoded enhanced messages, suitable for rendering in a UI
-	EnhancedGetMessages(conversation int, index int, count int) string
+	EnhancedGetMessages(conversation int, index int, count uint) string
 
 	// Server Token APIS
 	// TODO move these to feature protected interfaces

testing/quality.sh

@@ -4,24 +4,25 @@ echo "Checking code quality (you want to see no output here)"
 echo ""
 
 echo ""
-echo "Linting:"
+echo "Running staticcheck..."
 
 staticcheck ./...
 
-# In the future we should enable this by default. However, there are a few false positives that make this
+# In the future we should remove include-pkgs. However, there are a few false positives in the overall go stdlib that make this
 # too noisy right now, specifically assigning nil to initialize slices (safe), and using go internal context channels assigned
 # nil (also safe).
 # We also have one file infinite_channel.go written in a way that static analysis cannot reason about easily. So it is explictly
 # ignored.
-# nilaway -exclude-file-docstrings="nolint:nilaway" ./...
+echo "Running nilaway..."
+nilaway -include-pkgs="cwtch.im/cwtch,cwtch.im/tapir,git.openprivacy.ca/openprivacy/connectivity" -exclude-file-docstrings="nolint:nilaway" ./...
 
 echo "Time to format"
 gofmt -l -s -w .
 
 # ineffassign (https://github.com/gordonklaus/ineffassign)
-echo "Checking for ineffectual assignment of errors (unchecked errors...)"
-ineffassign ./..
+# echo "Checking for ineffectual assignment of errors (unchecked errors...)"
+# ineffassign .
 
 # misspell (https://github.com/client9/misspell/cmd/misspell)
-echo "Checking for misspelled words..."
-misspell . | grep -v "testing/" | grep -v "vendor/" | grep -v "go.sum" | grep -v ".idea"
+# echo "Checking for misspelled words..."
+# misspell . | grep -v "testing/" | grep -v "vendor/" | grep -v "go.sum" | grep -v ".idea"