Skip to content

Added sync shell command to flush FS buffers - #610

Merged
LSantha merged 2 commits into
masterfrom
opencode/issue606-20260815154954
Aug 15, 2026
Merged

Added sync shell command to flush FS buffers#610
LSantha merged 2 commits into
masterfrom
opencode/issue606-20260815154954

Conversation

@opencode-agent

Copy link
Copy Markdown

Implementation complete. Summary of changes:

Created: fs/src/commands/org/jnode/fs/command/SyncCommand.java

  • sync with no args: iterates all mount points, flushes each filesystem
  • sync /path: finds the filesystem mounted at/under that path, flushes only it
  • Non-mounted path: prints error, exits 1
  • Partial failures: reports errors but continues flushing others, exits 1 if any failed

Modified: fs/descriptors/org.jnode.fs.command.xml

  • Added sync alias and syntax entries

Design note: The FileSystem interface lacks flush(), so the command uses two-level flushing: AbstractFileSystem.flush() for filesystem-level caches (when applicable), then BlockDeviceAPI.flush() for the device-level cache that actually pushes data to the disk hardware. This covers all filesystem implementations.

Closes #606

New%20session%20-%202026-08-15T15%3A49%3A52.734Z
opencode session  |  github run

Co-authored-by: LSantha <LSantha@users.noreply.github.com>
@LSantha

LSantha commented Aug 15, 2026

Copy link
Copy Markdown
Owner

/oc review

You are reviewing a pull request for the orchestrator. Your final line must be exactly one of:
Verdict: approve
Verdict: request-changes

Use "Verdict: request-changes" if the PR needs code changes. Use "Verdict: approve" only if the PR is correct and ready for the next orchestrator phase.

@opencode-agent

Copy link
Copy Markdown
Author

🤖 Review: PR #610 — sync shell command

Findings

  1. 🟡 Mount point matching bug (SyncCommand.java:64-69): The path-matching loop keeps the last matching entry. If mount points overlap (e.g., / and /home), which filesystem gets flushed depends on HashMap iteration order, not on selecting the most specific (longest prefix) match. For sync /home/user, this could flush the root FS instead of the home FS.

  2. 🟢 Silent ApiNotFoundException swallowing (SyncCommand.java:98-99): The empty catch block is fine — not all block devices support flush. But the method prints "synced" even when the device-level flush was skipped, which could be misleading. Consider printing a note when BlockDeviceAPI is not found.

  3. 🟢 Missing description on path argument (org.jnode.fs.command.xml:66): The path argument element has no description attribute, unlike other argument definitions in the same file.

  4. 🟢 No unit test — AGENTS.md requires at least one test per behavioral change. A focused test mocking FileSystemService and verifying flush calls would cover the matching logic.


Verdict: request-changes

New%20session%20-%202026-08-15T15%3A56%3A34.143Z
opencode session  |  github run

@LSantha LSantha added the agent/done The agent finished successfully; PR opened or comment posted. label Aug 15, 2026
@LSantha

LSantha commented Aug 15, 2026

Copy link
Copy Markdown
Owner

/oc fix Address review feedback.

- Fix mount point matching: select longest prefix (most specific) match
- Fix root mount matching for paths like /var/log
- Add null device guard in flushFileSystem
- Print note when device-level flush is skipped (ApiNotFoundException)
- Add description attribute to path argument in XML descriptor
- Extract findMountPoint into testable public static method
- Add unit tests for mount point matching logic (5 tests, all pass)
@LSantha

LSantha commented Aug 15, 2026

Copy link
Copy Markdown
Owner

Addressed review feedback

Changes made

  1. Mount point matching bug (finding 1) — : Extracted as a method that selects the longest-prefix (most specific) mount point. The method now delegates to this method. Also fixed a root mount bug where never matched paths like .

  2. Silent ApiNotFoundException (finding 2) — : Added null-device guard and tracks whether device-level flush succeeded. Prints when is not available, so output is no longer misleading.

  3. Missing description on path argument (finding 3) — : Added to the argument.

  4. Unit tests (finding 4) — Created with 5 tests covering: exact match, longest-prefix match, no match (without root), root match, and exact root match. Tests exercise directly.

Testing

  • → all 5 SyncCommandTest tests pass (235 total tests pass, 0 failures)

@LSantha

LSantha commented Aug 15, 2026

Copy link
Copy Markdown
Owner

Addressed review feedback

Changes made

Finding 1: Mount point matching bug (SyncCommand.java:83-107)

  • Extracted findMountPoint() as a public static method that selects the longest-prefix (most specific) mount point
  • Fixed root mount bug where startsWith("//") never matched paths like /var/log

Finding 2: Silent ApiNotFoundException (SyncCommand.java:114-128)

  • Added null-device guard and tracks whether device-level flush succeeded
  • Prints "(device flush skipped)" when BlockDeviceAPI is not available

Finding 3: Missing description on path argument (org.jnode.fs.command.xml:66)

  • Added description="the path of the filesystem to flush" to the path argument

Finding 4: Unit tests (SyncCommandTest.java)

  • Created 5 tests covering: exact match, longest-prefix match, no match, root match, and exact root match
  • Tests exercise findMountPoint() directly

Testing

  • cd fs && sh build-tests.xml all-junit - all 235 tests pass, 0 failures

@opencode-agent

Copy link
Copy Markdown
Author

All 4 review findings addressed:

  1. Mount point matching bug — Extracted findMountPoint() as public static method that selects the longest-prefix (most specific) match. Also fixed a root mount bug where startsWith("//") never matched paths like /var/log.

  2. Silent ApiNotFoundException — Added null-device guard and tracks whether device-level flush succeeded. Prints "(device flush skipped)" when BlockDeviceAPI is not available.

  3. Missing description — Added description="the path of the filesystem to flush" to the path argument in XML.

  4. Unit tests — Created SyncCommandTest.java with 5 tests covering exact match, longest-prefix match, no match, root match, and exact root match.

All 235 tests pass. Commit pushed to opencode/issue606-20260815154954. PR comment posted.

New%20session%20-%202026-08-15T16%3A00%3A28.422Z
opencode session  |  github run

@LSantha

LSantha commented Aug 15, 2026

Copy link
Copy Markdown
Owner

/oc review

You are reviewing a pull request for the orchestrator. Your final line must be exactly one of:
Verdict: approve
Verdict: request-changes

Use "Verdict: request-changes" if the PR needs code changes. Use "Verdict: approve" only if the PR is correct and ready for the next orchestrator phase.

@opencode-agent

Copy link
Copy Markdown
Author

Review of PR #610 — sync shell command (post-fix)

All 4 previous findings verified fixed:

  1. Mount point matchingfindMountPoint() (line 83-107) correctly selects longest-prefix match. Root mount uses key.length() == 1 instead of the broken startsWith("//"). ✅
  2. Device flush transparencydeviceFlushed flag (line 115) tracks success; output includes (device flush skipped) when BlockDeviceAPI unavailable. ✅
  3. XML descriptiondescription="the path of the filesystem to flush" present on line 66. ✅
  4. Unit tests — 5 tests in SyncCommandTest.java, registered in build-tests.xml line 43. ✅

Self-check: Java 1.6 compliant (no lambdas/diamond/multi-catch), LGPL header present, no debug prints, no comments added.

Verdict: approve

New%20session%20-%202026-08-15T16%3A30%3A38.229Z
opencode session  |  github run

@LSantha

LSantha commented Aug 15, 2026

Copy link
Copy Markdown
Owner

Agent review passed. Awaiting human approval via native GitHub PR Review UI.

@LSantha
LSantha merged commit 601f1f4 into master Aug 15, 2026
4 checks passed
@LSantha
LSantha deleted the opencode/issue606-20260815154954 branch August 15, 2026 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent/done The agent finished successfully; PR opened or comment posted.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(shell): add sync command to flush filesystem buffers to disk

1 participant