Skip to content

Dev/kirisamejin/support file list and get - #32

Merged
dxyinme merged 7 commits into
mainfrom
dev/kirisamejin/support-file-list-and-get
Sep 3, 2025
Merged

Dev/kirisamejin/support file list and get#32
dxyinme merged 7 commits into
mainfrom
dev/kirisamejin/support-file-list-and-get

Conversation

@kirisamejin

Copy link
Copy Markdown
Contributor

Closes #22

@kirisamejin

Copy link
Copy Markdown
Contributor Author

应该还得在agent和agentcentral加上test

@codecov

codecov Bot commented Jul 22, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 10.96939% with 349 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.55%. Comparing base (ea305af) to head (a0a9e68).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...rvices/agentcentral/internal/logic/agents/agent.go 0.00% 94 Missing ⚠️
services/agentcentral/apis/agents.go 0.00% 86 Missing ⚠️
agent/internal/handler/files.go 0.00% 85 Missing ⚠️
...ices/agentcentral/internal/logic/agents/manager.go 0.00% 49 Missing ⚠️
agent/internal/filemanager/mgr.go 64.58% 12 Missing and 5 partials ⚠️
agent/internal/filemanager/mock_mgr.go 0.00% 12 Missing ⚠️
agent/internal/handler/handler.go 62.50% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #32      +/-   ##
==========================================
- Coverage   41.96%   37.55%   -4.41%     
==========================================
  Files          88       89       +1     
  Lines        2414     2809     +395     
==========================================
+ Hits         1013     1055      +42     
- Misses       1339     1684     +345     
- Partials       62       70       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread tools/tools.md
--fileservicefe, --no-fileservicefe
build fileservicefe (default: False)

./tools/testscripts/devstart.sh

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

这个最好新开一个section,然后写个什么 dev env 启动之类的title

Comment thread protocol/models/heartbeat.go Outdated

type FileListRequest struct {
OperationId string
Node string `json:"nodes"`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

json:"node"就行了,不要nodes

ah.responsesMutex.Lock()
delete(ah.pendingFileGetOps, operationId)
ah.responsesMutex.Unlock()
return nil, fmt.Errorf("timeout waiting for response")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

return 之前 可以打个log


func (ah *agentHolder) AddFileListRequest(operationId, path string) error {
ah.fileRequestsMutex.Lock()
defer ah.fileRequestsMutex.Unlock()

@dxyinme dxyinme Jul 23, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

这里是不是可以不用defer, append成功就可以unlock了

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

我想了一下(虽然这个代码也是AI写的),如果append之后unlock,这个时候pendingFileListOps还没加上,就可以addRequestToHB,然后再下一次HB返回的时候如果还没加上,就会找不到对应的pendingFileListOps。但应该不会出现这种情况。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

确实有可能你说的这种情况

// Add file operation request queues
fileListRequests []models.FileListRequest
fileGetRequests []models.FileGetRequest
fileRequestsMutex sync.Mutex

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

fileListRequests和fileGetRequests为啥共用同一个锁

@dxyinme

dxyinme commented Jul 23, 2025

Copy link
Copy Markdown
Member

应该还得在agent和agentcentral加上test

确实得加,不加coverage太低了

Comment thread agent/internal/handler/files.go
s.fakeFileServer.AddFileContent(path.Join("TestPublicFileDownload", "a.txt"), content)
url := fakefileserver.BuildDownloadURL(s.fakeFileServer, path.Join("TestPublicFileDownload", "a.txt"))
// Use forward slashes for URL paths, not path.Join which uses OS-specific separators
serverPath := "TestPublicFileDownload/a.txt"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

我个人建议用filepath去join

@dxyinme

dxyinme commented Aug 24, 2025

Copy link
Copy Markdown
Member

其他都OK,就加点test就行

@dxyinme
dxyinme merged commit 0e444d3 into main Sep 3, 2025
3 of 5 checks passed
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.

Support get/list file APIs from node

2 participants