Conversation
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="pkg/filesystem/vnode/tasks.go">
<violation number="1" location="pkg/filesystem/vnode/tasks.go:20">
P2: The fixed 10MB sentinel size can truncate task logs larger than 10MB because the file size advertised by Getattr/Readdir is smaller than the actual content returned by Read. Use a size that’s guaranteed to be >= the maximum possible log size (or compute the size from actual content) to avoid clients stopping early.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ) | ||
|
|
||
| const tasksCacheTTL = 5 * time.Second | ||
| const taskFileSentinelSize int64 = 10 << 20 // 10MB — must be large enough for FUSE/NFS to issue reads covering all content |
There was a problem hiding this comment.
P2: The fixed 10MB sentinel size can truncate task logs larger than 10MB because the file size advertised by Getattr/Readdir is smaller than the actual content returned by Read. Use a size that’s guaranteed to be >= the maximum possible log size (or compute the size from actual content) to avoid clients stopping early.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/filesystem/vnode/tasks.go, line 20:
<comment>The fixed 10MB sentinel size can truncate task logs larger than 10MB because the file size advertised by Getattr/Readdir is smaller than the actual content returned by Read. Use a size that’s guaranteed to be >= the maximum possible log size (or compute the size from actual content) to avoid clients stopping early.</comment>
<file context>
@@ -17,6 +17,7 @@ import (
)
const tasksCacheTTL = 5 * time.Second
+const taskFileSentinelSize int64 = 10 << 20 // 10MB — must be large enough for FUSE/NFS to issue reads covering all content
// TasksVNode provides /tasks directory listing tasks as files.
</file context>
There was a problem hiding this comment.
We may be able to (when listing metadata), get more accurate size values from s2 to avoid this hack. Worth a shot.
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
| ) | ||
|
|
||
| const tasksCacheTTL = 5 * time.Second | ||
| const taskFileSentinelSize int64 = 10 << 20 // 10MB — must be large enough for FUSE/NFS to issue reads covering all content |
There was a problem hiding this comment.
We may be able to (when listing metadata), get more accurate size values from s2 to avoid this hack. Worth a shot.
luke-lombardi
left a comment
There was a problem hiding this comment.
Overall: Good minimal fix — using a sentinel size unblocks FUSE/NFS reads for task files, and the comment explaining why is helpful.
Note on PR chain: This PR is the base for #50 (cg/tasks-files-cache), which layers on a real content cache and replaces the sentinel with accurate sizes. If the intent is to merge both together, the sentinel approach here is a reasonable stepping stone. If #49 is meant to land alone (and #50 separately), the 10MB lie in directory listings will be visible to users until #50 lands.
Minor concern: A 10MB reported size when most task files will be a few KB (or maybe a few MB of logs) means tools like ls -l, NFS file-size APIs, and progress bars will show inflated sizes. That said, it's a sentinel for correctness — if the alternative is zero (which blocks reads entirely), 10MB is the right pragmatic call.
Suggestion: The constant name taskFileSentinelSize and the comment are clear — no change needed there.
luke-lombardi
left a comment
There was a problem hiding this comment.
PR Review: Make Task Contents Available In Mounted Drive
Summary: Clean, minimal fix. The sentinel size approach (10MB) is a reasonable workaround for the FUSE/NFS zero-size issue — reporting a non-zero size convinces clients to issue a full read.
Observations:
- The constant is well-named and commented, making the intent clear.
- Both
GetattrandReaddirare updated consistently, which is correct. - The sentinel value (10MB) is hard-coded. Worth documenting somewhere that task output should not approach 10MB, or this could cause clients to issue reads that stop mid-content.
Note: PR #88 appears to build on top of this by adding a post-read size cache for terminal tasks, which solves the permanent-sentinel problem. If #88 is ready to merge, it may be cleaner to merge #88 directly (it already includes this sentinel logic). Consider whether #49 should be merged alone or closed in favor of #88.
Verdict: Looks good — the core fix is correct. Recommend coordinating merge order with #88.
luke-lombardi
left a comment
There was a problem hiding this comment.
Overall: Simple, targeted fix to unblock FUSE/NFS reads on task files. The approach is correct — FUSE clients skip reads on zero-size files, so reporting a sentinel size is necessary.
Notes:
-
Superseded by PR #88 — PR #88 (
cg/tasks-files-3) adds the sametaskFileSentinelSizeconstant and sentinel logic, plus a per-task size cache that replaces the sentinel with the real size after first read. These two PRs will conflict ontasks.go. If #88 is ready, this PR can be closed in its favor. -
Sentinel value is conservative but unbounded — 10MB is large enough for most task payloads, but if any task content exceeds 10MB, the client will issue a short read and silently truncate. Worth documenting this upper bound expectation or validating task content size at write time.
-
The comment on the constant is clear and self-documenting. No other concerns with the change itself.
luke-lombardi
left a comment
There was a problem hiding this comment.
PR #49: Make Task Contents Available In Mounted Drive
Small, targeted fix that unblocks FUSE/NFS reads by reporting a non-zero file size in Getattr and Readdir. Previously, zero-size files caused clients to skip issuing read calls entirely.
What looks good:
- Minimal diff — exactly the right scope for the problem being solved.
- The sentinel constant is well-named and the comment explains the FUSE/NFS constraint.
- Both
GetattrandReaddirare updated consistently.
Observations:
- The 10MB sentinel is a heuristic. If a task file can legitimately exceed 10MB (e.g. large log output), reads will be truncated at the client side before the full content is returned. Worth documenting or enforcing an upper bound at the read layer.
- PR #50 and #88 both build on this and replace the sentinel with actual cached sizes — this PR appears to be a prerequisite step for those. The relationship between the three PRs should be clear in the merge order.
No blocking issues. Clean fix.
Summary by cubic
Make task contents readable in the mounted /tasks directory by reporting a 10MB sentinel size for each task file. This prompts FUSE/NFS clients to issue reads and return the task body.
Written for commit 392129f. Summary will update on new commits.