diff --git a/docs/command-reference.md b/docs/command-reference.md index 8307b019..0ffd0d07 100644 --- a/docs/command-reference.md +++ b/docs/command-reference.md @@ -306,7 +306,8 @@ agents can react without launching a diagnostic chain on every blip: - Advanced command. Starts detached background sync by default; use `--foreground` for sync debugging, or explicit one-way sync (`up`/`down`). - By default each configured mapping syncs in its own `direction` (`spec.sync.paths[].direction`, falling back to `bi`); an explicit `--mode` forces that mode onto every mapping for the invocation. See the config manifest for choosing persistent directions — `down` makes the pod the authority so local writes can never clobber pod-generated results. - With multiple mappings, each becomes its own syncthing folder; the first (primary) mapping is the one shared to mesh receivers, and `sync reset-remote` clears only the primary remote. -- For default `--mode bi`, no-op when background sync is already active for the session. +- A mapping's local root may nest inside the primary root: okdev maintains a managed block in the primary root's `.stignore` excluding it from the primary folder (written before the sync daemon starts). Removing a mapping retains the entry as a tombstone so the subtree never silently joins the primary folder; sync start prints a notice and `status --details` lists active/retained excludes. +- Without an explicit `--mode`, no-op when background sync is already active for the session. - `--background`: explicitly request detached background mode. - `--reset`: check local-to-hub sync and mesh receiver health, then reset only what is broken. Skips the local sync teardown when the primary sync is already healthy. For sessions with mesh receivers, probes each receiver and re-runs mesh setup only when broken or disconnected receivers are found. - `--reset --force` / `--reset -f`: unconditionally reset without health checks. diff --git a/docs/config-manifest.md b/docs/config-manifest.md index 464c5560..4fb9f6ea 100644 --- a/docs/config-manifest.md +++ b/docs/config-manifest.md @@ -250,15 +250,17 @@ spec: ```yaml paths: - .:/workspace # primary: code, bidirectional - - local: ../collected # sibling of the repo — local roots must - remote: /data/results # not nest inside the primary mapping + - local: ./collected # nested inside the repo — allowed: + remote: /data/results # okdev excludes it from the primary folder direction: down # pod-generated results, pod is authority - - local: ../datasets + - local: ../datasets # disjoint roots work too remote: /data/datasets direction: up # dataset push, local is authority ``` -Note the disjoint rule applies to the **local** side too: a results directory *inside* the synced repo (e.g. `./collected` under `.`) is rejected — place it next to the repo instead, or narrow the primary mapping to a subdirectory. +**Nested local roots.** An additional mapping's local root may live *inside* the primary mapping's local root (`./collected` under `.`). okdev maintains a managed block in the primary root's `.stignore` (`// okdev:begin/end managed sync excludes`) so the subtree travels only through its own folder — the block is written before the sync daemon starts, so the exclusion is in force from the first scan. Any other local overlap (equal roots, nesting between additional mappings, an additional root containing the primary) is rejected, and **remote roots must always be disjoint**. + +**Removing a mapping never widens the primary folder.** When a nested mapping is removed, its folder is pruned from both syncthing instances and local files are kept, but the `.stignore` entry is *retained* as a tombstone (with an explanatory comment) — otherwise the subtree would suddenly bulk-sync through the code channel. To fold the directory back into the primary folder, delete its line from `.stignore`. `status --details` lists both active and retained excludes. **Volumes are provisioned automatically.** An additional mapping's remote root must be visible to the okdev sidecar (its syncthing serves the remote side), which means it must live on a volume — a root on the container's own filesystem cannot sync. You don't need to write any volume YAML: okdev auto-provisions an emptyDir at any extra remote root not already covered by a target-container volume mount, and mirrors the target container's mounts into the sidecar. Adding or removing a mapping changes the pod spec, so the next `okdev up` asks for `--reconcile` (pod recreate). Auto-provisioned emptyDirs don't survive pod recreation — fine for both directions (`up` re-pushes from local, `down` results are mirrored locally); mount your own PVC at the root if the data must persist, and okdev will use it instead. Sync startup still probes each extra root and fails with a clear error if it is not shared with the sidecar. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index a3f6373f..01013f04 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -22,18 +22,23 @@ ## Sync Failures - Validate workspace path exists and is writable in the container. -- Isolate directionality with one-way runs: `okdev sync --mode up`, `okdev sync --mode down`. +- Isolate directionality with one-way runs: `okdev sync --mode up`, `okdev sync --mode down` (an explicit `--mode` forces every mapping for that invocation). - `sync.engine=syncthing` requires local Syncthing bootstrap (handled by `okdev`) and sidecar image pull success. -- Current Syncthing implementation supports a single `local:remote` mapping. +- Multiple mappings are supported (each is its own syncthing folder with its own `direction`); remote roots must be disjoint, and a local root nested inside the primary root is excluded from the primary folder via the managed block in its `.stignore`. - If you see `ErrImagePull` / GHCR `403`, use a publicly pullable `spec.sidecar.image` or adjust registry permissions. - If `okdev status --details` shows stale local sync state or a stale PID file, run `okdev sync --reset`. For mesh sessions, `--reset` also probes receiver health and repairs any broken mesh connections. - `okdev status --details` shows live mesh health when receivers exist, including per-receiver connection and sync status. - If the session pod was recreated, rerun `okdev up` to re-bootstrap local sync against the new pod. - Re-run `okdev ports` if the sync connection depends on managed SSH forwarding that may have been interrupted. +Before treating "changes are not syncing" as a fault, rule out two by-design behaviors: + +- **Direction contract**: with `direction: up` pod-side writes never reach local; with `down` local writes never reach the pod (syncthing marks them as local additions). `okdev status --details` shows each mapping's direction arrow. +- **Managed excludes**: a directory that belongs to (or belonged to) its own nested mapping is excluded from the primary folder. `status --details` lists active excludes and tombstones retained after a mapping was removed; delete the entry from the primary root's `.stignore` to fold the directory back into the primary folder. + ## Recovering A File Overwritten By Sync -The managed folder is bidirectional in steady state, so a bad write on one side (e.g. an empty file from a failed `okdev exec ... > result.txt` redirect) can propagate and overwrite the real file on the other side. Versioning (on by default, `spec.sync.syncthing.versioningDays`) archives the previous version on the side that applied the incoming change: +With the default `direction: bi` a folder is bidirectional in steady state, so a bad write on one side (e.g. an empty file from a failed `okdev exec ... > result.txt` redirect) can propagate and overwrite the real file on the other side. (Directional mappings — `up`/`down` — are structurally immune: the non-authoritative side's writes are never propagated.) Versioning (on by default, `spec.sync.syncthing.versioningDays`) archives the previous version on the side that applied the incoming change: - On the pod: `/.stversions/` (e.g. `/workspace/.stversions/`) - Locally: `/.stversions/` diff --git a/internal/cli/status_details.go b/internal/cli/status_details.go index ffb5db9b..5d897198 100644 --- a/internal/cli/status_details.go +++ b/internal/cli/status_details.go @@ -113,8 +113,14 @@ type detailedStatusSSH struct { } type detailedStatusSync struct { - Engine string `json:"engine"` - Direction string `json:"direction,omitempty"` + Engine string `json:"engine"` + Direction string `json:"direction,omitempty"` + // ManagedExcludes are nested child mappings excluded from the primary + // folder; RetainedExcludes are tombstones kept after a mapping was + // removed (the subtree stays out of the primary folder until the user + // deletes the entry from .stignore). + ManagedExcludes []string `json:"managedExcludes,omitempty"` + RetainedExcludes []string `json:"retainedExcludes,omitempty"` Health string `json:"health,omitempty"` HealthDetail string `json:"healthDetail,omitempty"` ConfiguredPaths []string `json:"configuredPaths,omitempty"` @@ -422,13 +428,19 @@ func buildDetailedSync(sessionName string, cfg *config.DevEnvironment, cfgPath s engine = "syncthing" } direction := "" + var managedExcludes, retainedExcludes []string if pairs, err := syncengine.ParsePairs(cfg.Spec.Sync.Paths, cfg.EffectiveWorkspaceMountPath(cfgPath)); err == nil && len(pairs) > 0 { direction = pairs[0].Direction + if absPrimary, absErr := filepath.Abs(pairs[0].Local); absErr == nil { + managedExcludes, retainedExcludes = readManagedSTIgnoreBlock(absPrimary) + } } detail := detailedStatusSync{ - Engine: engine, - Direction: direction, - ConfiguredPaths: summarizeConfiguredSyncPaths(cfg, cfgPath), + Engine: engine, + Direction: direction, + ManagedExcludes: managedExcludes, + RetainedExcludes: retainedExcludes, + ConfiguredPaths: summarizeConfiguredSyncPaths(cfg, cfgPath), } if engine != "syncthing" { return detail @@ -602,6 +614,12 @@ func printDetailedStatus(w io.Writer, detail detailedStatus) { if detail.Sync.Direction != "" { fmt.Fprintf(w, "- direction: %s (%s)\n", detail.Sync.Direction, modeSymbol(detail.Sync.Direction)) } + if len(detail.Sync.ManagedExcludes) > 0 { + fmt.Fprintf(w, "- excluded from primary folder (own mappings): %s\n", strings.Join(detail.Sync.ManagedExcludes, ", ")) + } + if len(detail.Sync.RetainedExcludes) > 0 { + fmt.Fprintf(w, "- excluded from primary folder (retained after mapping removal): %s\n", strings.Join(detail.Sync.RetainedExcludes, ", ")) + } if detail.Sync.BackgroundStatus != "" { status := detail.Sync.BackgroundStatus if detail.Sync.BackgroundPID > 0 { diff --git a/internal/cli/status_details_test.go b/internal/cli/status_details_test.go index 10043044..4c2aca78 100644 --- a/internal/cli/status_details_test.go +++ b/internal/cli/status_details_test.go @@ -348,6 +348,8 @@ func TestPrintDetailedStatusIncludesSections(t *testing.T) { Sync: detailedStatusSync{ Engine: "syncthing", Direction: "down", + ManagedExcludes: []string{"/results"}, + RetainedExcludes: []string{"/old-datasets"}, BackgroundStatus: "running", BackgroundPID: 123, ConfiguredPaths: []string{"/tmp/repo -> /workspace"}, @@ -393,6 +395,8 @@ func TestPrintDetailedStatusIncludesSections(t *testing.T) { "managed forward: running", "Sync:", "direction: down (<-)", + "excluded from primary folder (own mappings): /results", + "excluded from primary folder (retained after mapping removal): /old-datasets", "background: running (pid 123)", "conflicts: 2", "./a.sync-conflict-1", diff --git a/internal/cli/stignore_managed_test.go b/internal/cli/stignore_managed_test.go new file mode 100644 index 00000000..053c1742 --- /dev/null +++ b/internal/cli/stignore_managed_test.go @@ -0,0 +1,99 @@ +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestMergeManagedSTIgnoreBlock(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, ".stignore") + + // Creating the block appends after existing user content. + if err := os.WriteFile(path, []byte("node_modules\n// user comment\n"), 0o644); err != nil { + t.Fatal(err) + } + retained, err := mergeManagedSTIgnoreBlock(dir, []string{"/results"}) + if err != nil || len(retained) != 0 { + t.Fatalf("merge: retained=%v err=%v", retained, err) + } + content, _ := os.ReadFile(path) + text := string(content) + if !strings.Contains(text, "node_modules\n") || !strings.Contains(text, "// user comment") { + t.Fatalf("user content must be preserved, got %q", text) + } + if !strings.Contains(text, managedSTIgnoreBegin+"\n/results\n"+managedSTIgnoreEnd) { + t.Fatalf("expected managed block with /results, got %q", text) + } + + // Idempotent: same input, same output. + if _, err := mergeManagedSTIgnoreBlock(dir, []string{"/results"}); err != nil { + t.Fatal(err) + } + again, _ := os.ReadFile(path) + if string(again) != text { + t.Fatalf("merge must be idempotent:\n%q\nvs\n%q", text, again) + } + + // Growing the active set keeps sorted patterns inside the block. + if _, err := mergeManagedSTIgnoreBlock(dir, []string{"/datasets", "/results"}); err != nil { + t.Fatal(err) + } + active, tombs := readManagedSTIgnoreBlock(dir) + if strings.Join(active, ",") != "/datasets,/results" || len(tombs) != 0 { + t.Fatalf("unexpected block state: active=%v tombs=%v", active, tombs) + } + + // Dropping a mapping tombstones its pattern instead of releasing the + // subtree into the primary folder. + retained, err = mergeManagedSTIgnoreBlock(dir, []string{"/datasets"}) + if err != nil { + t.Fatal(err) + } + if len(retained) != 1 || retained[0] != "/results" { + t.Fatalf("expected /results newly retained, got %v", retained) + } + active, tombs = readManagedSTIgnoreBlock(dir) + if strings.Join(active, ",") != "/datasets" || strings.Join(tombs, ",") != "/results" { + t.Fatalf("unexpected block state: active=%v tombs=%v", active, tombs) + } + content, _ = os.ReadFile(path) + if !strings.Contains(string(content), managedSTIgnoreTombstone+"\n/results\n") { + t.Fatalf("tombstone comment must precede the retained pattern, got %q", content) + } + + // A tombstoned pattern re-added to the config becomes active again and + // is reported as retained exactly once. + retained, err = mergeManagedSTIgnoreBlock(dir, []string{"/datasets", "/results"}) + if err != nil || len(retained) != 0 { + t.Fatalf("re-adding must not re-report retention: retained=%v err=%v", retained, err) + } + active, tombs = readManagedSTIgnoreBlock(dir) + if strings.Join(active, ",") != "/datasets,/results" || len(tombs) != 0 { + t.Fatalf("re-added pattern must leave tombstones, got active=%v tombs=%v", active, tombs) + } + + // User content after the block is preserved across rewrites. + if err := os.WriteFile(path, []byte(string(content)+"trailing-user-pattern\n"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := mergeManagedSTIgnoreBlock(dir, []string{"/datasets"}); err != nil { + t.Fatal(err) + } + final, _ := os.ReadFile(path) + if !strings.Contains(string(final), "trailing-user-pattern") { + t.Fatalf("content after block must survive, got %q", final) + } +} + +func TestMergeManagedSTIgnoreBlockNoopWithoutFileOrPatterns(t *testing.T) { + dir := t.TempDir() + if retained, err := mergeManagedSTIgnoreBlock(dir, nil); err != nil || retained != nil { + t.Fatalf("no file + no patterns must be a no-op, got %v %v", retained, err) + } + if _, err := os.Stat(filepath.Join(dir, ".stignore")); !os.IsNotExist(err) { + t.Fatal("no-op must not create .stignore") + } +} diff --git a/internal/cli/syncthing.go b/internal/cli/syncthing.go index ebcc9533..1f06bc85 100644 --- a/internal/cli/syncthing.go +++ b/internal/cli/syncthing.go @@ -195,6 +195,18 @@ func runSyncthingSync(cmd *cobra.Command, opts *Options, cfg *config.DevEnvironm } } primary := folders[0] + // Exclude nested child mappings from the primary folder BEFORE the + // local daemon starts: the daemon's first scan must already honor the + // managed block, or the child subtree leaks through the primary channel + // once. okdev owns the daemon lifecycle, so file-before-daemon ordering + // is a hard guarantee here. + newlyRetained, err := mergeManagedSTIgnoreBlock(primary.absLocal, cfg.NestedLocalSyncIgnorePatterns()) + if err != nil { + return fmt.Errorf("update primary folder ignore rules: %w", err) + } + for _, pattern := range newlyRetained { + fmt.Fprintf(cmd.OutOrStdout(), "Note: %s is no longer a sync mapping; its files are kept and it stays excluded from the primary folder. Delete its entry in %s to sync it via the primary folder.\n", pattern, filepath.Join(primary.absLocal, ".stignore")) + } localHome, err := localSyncthingHome(sessionName) if err != nil { return err @@ -642,6 +654,158 @@ func waitForSyncthingBootstrapComplete(ctx context.Context, sessionName string) } } +// Managed .stignore block markers. Syncthing's comment syntax is "//" +// ("#..." lines would be treated as patterns, and "#include" is special). +const ( + managedSTIgnoreBegin = "// okdev:begin managed sync excludes" + managedSTIgnoreEnd = "// okdev:end managed sync excludes" + managedSTIgnoreTombstone = "// okdev: retained after mapping removal - delete the next line to sync it via the primary folder" +) + +// mergeManagedSTIgnoreBlock rewrites the managed block in dir's .stignore: +// active patterns are excluded from the primary folder (nested sync +// mappings travel only through their own folders), and previously managed +// patterns that are no longer active become tombstones — removing a mapping +// must never silently widen the primary folder's scope. Content outside the +// block is preserved verbatim. Returns the newly tombstoned patterns so the +// caller can tell the user. +func mergeManagedSTIgnoreBlock(dir string, active []string) ([]string, error) { + path := filepath.Join(dir, ".stignore") + var lines []string + raw, err := os.ReadFile(path) + switch { + case err == nil: + lines = strings.Split(strings.TrimRight(string(raw), "\n"), "\n") + case os.IsNotExist(err): + if len(active) == 0 { + return nil, nil + } + default: + return nil, err + } + + var before, after []string + var existingActive, existingTombstones []string + inBlock, seenBlock, tombstoneNext := false, false, false + for _, line := range lines { + trimmed := strings.TrimSpace(line) + switch { + case trimmed == managedSTIgnoreBegin: + inBlock, seenBlock = true, true + case trimmed == managedSTIgnoreEnd: + inBlock = false + case inBlock: + if trimmed == managedSTIgnoreTombstone { + tombstoneNext = true + continue + } + if trimmed == "" || strings.HasPrefix(trimmed, "//") { + tombstoneNext = false + continue + } + if tombstoneNext { + existingTombstones = append(existingTombstones, trimmed) + } else { + existingActive = append(existingActive, trimmed) + } + tombstoneNext = false + case !seenBlock: + before = append(before, line) + default: + after = append(after, line) + } + } + + activeSet := make(map[string]bool, len(active)) + for _, p := range active { + activeSet[p] = true + } + tombstoneSet := make(map[string]bool) + for _, p := range existingTombstones { + if !activeSet[p] { + tombstoneSet[p] = true + } + } + var newlyRetained []string + for _, p := range existingActive { + if !activeSet[p] && !tombstoneSet[p] { + tombstoneSet[p] = true + newlyRetained = append(newlyRetained, p) + } + } + tombstones := make([]string, 0, len(tombstoneSet)) + for p := range tombstoneSet { + tombstones = append(tombstones, p) + } + sort.Strings(tombstones) + sort.Strings(newlyRetained) + + var b strings.Builder + for _, line := range before { + b.WriteString(line) + b.WriteString("\n") + } + if len(active) > 0 || len(tombstones) > 0 { + b.WriteString(managedSTIgnoreBegin) + b.WriteString("\n") + for _, p := range active { + b.WriteString(p) + b.WriteString("\n") + } + for _, p := range tombstones { + b.WriteString(managedSTIgnoreTombstone) + b.WriteString("\n") + b.WriteString(p) + b.WriteString("\n") + } + b.WriteString(managedSTIgnoreEnd) + b.WriteString("\n") + } + for _, line := range after { + b.WriteString(line) + b.WriteString("\n") + } + if err := os.WriteFile(path, []byte(b.String()), 0o644); err != nil { + return nil, err + } + return newlyRetained, nil +} + +// readManagedSTIgnoreBlock returns the active patterns and tombstones from +// dir's .stignore managed block (for status visibility). +func readManagedSTIgnoreBlock(dir string) (active, tombstones []string) { + raw, err := os.ReadFile(filepath.Join(dir, ".stignore")) + if err != nil { + return nil, nil + } + inBlock, tombstoneNext := false, false + for _, line := range strings.Split(string(raw), "\n") { + trimmed := strings.TrimSpace(line) + switch { + case trimmed == managedSTIgnoreBegin: + inBlock = true + case trimmed == managedSTIgnoreEnd: + return active, tombstones + case inBlock: + if trimmed == managedSTIgnoreTombstone { + tombstoneNext = true + continue + } + if trimmed == "" || strings.HasPrefix(trimmed, "//") { + tombstoneNext = false + continue + } + if tombstoneNext { + tombstones = append(tombstones, trimmed) + } else { + active = append(active, trimmed) + } + tombstoneNext = false + } + } + return active, tombstones +} + func writeSTIgnore(localPath string, excludes []string) error { content, ok := buildSTIgnoreContent(excludes) if !ok { diff --git a/internal/config/config.go b/internal/config/config.go index ce944897..c0e9ce72 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -162,13 +162,14 @@ type SyncSpec struct { // // paths: // - .:/workspace -// - local: ../collected +// - local: ./collected // remote: /data/results // direction: down // -// Mapping roots must be pairwise disjoint on both sides (validated): note -// "../collected" above — a results directory nested inside the primary -// mapping's local root would be rejected. +// Remote roots must be pairwise disjoint (validated). Local roots may nest +// inside the PRIMARY mapping's local root (okdev excludes the child from +// the primary folder via a managed .stignore block); any other local +// overlap is rejected. // // Direction sets the syncthing folder types for this mapping's folder: // @@ -704,25 +705,64 @@ func validateSyncPaths(paths []SyncPathSpec) error { return fmt.Errorf("spec.sync.paths entry %q direction must be one of %s, %s, %s", p.Local+":"+p.Remote, SyncDirectionBi, SyncDirectionUp, SyncDirectionDown) } } - // Mappings must be disjoint on both sides. Two syncthing folders on the - // same root are impossible (.stignore lives in the directory, so both - // folders would share ignore rules), and nested roots would sync the - // nested subtree through two folders at once — reintroducing the - // overwrite hazards per-path directions exist to prevent. The check is - // lexical (no symlink resolution). + // Remote roots must be pairwise disjoint: the sidecar's syncthing would + // otherwise sync a subtree through two folders at once. Local roots + // must be disjoint too, with ONE exception: an additional mapping's + // local root may nest inside the PRIMARY mapping's local root — okdev + // excludes it from the primary folder via a managed .stignore block, so + // the subtree still travels through exactly one channel. Equal roots + // are impossible in syncthing (.stignore lives in the directory, so two + // folders on one root share ignore rules). Checks are lexical (no + // symlink resolution). for i := 0; i < len(paths); i++ { for j := i + 1; j < len(paths); j++ { - if syncRootsOverlap(paths[i].Local, paths[j].Local) { - return fmt.Errorf("spec.sync.paths local roots %q and %q overlap; mappings must be disjoint", paths[i].Local, paths[j].Local) + if syncRootsOverlap(paths[i].Local, paths[j].Local) && !(i == 0 && syncRootNestedInside(paths[0].Local, paths[j].Local)) { + return fmt.Errorf("spec.sync.paths local roots %q and %q overlap; only nesting inside the primary (first) mapping's local root is supported", paths[i].Local, paths[j].Local) } if syncRootsOverlap(paths[i].Remote, paths[j].Remote) { - return fmt.Errorf("spec.sync.paths remote roots %q and %q overlap; mappings must be disjoint", paths[i].Remote, paths[j].Remote) + return fmt.Errorf("spec.sync.paths remote roots %q and %q overlap; remote roots must be disjoint", paths[i].Remote, paths[j].Remote) } } } return nil } +// syncRootNestedInside reports whether child is strictly inside parent +// (equal roots do not count). +func syncRootNestedInside(parent, child string) bool { + parent = filepath.Clean(strings.TrimSpace(parent)) + child = filepath.Clean(strings.TrimSpace(child)) + if parent == child { + return false + } + return pathContains(parent, child) +} + +// NestedLocalSyncIgnorePatterns returns the .stignore patterns the primary +// folder needs so additional mappings nested inside its local root travel +// only through their own folders: one "/rel/path" pattern per nested child, +// sorted for stable output. +func (d *DevEnvironment) NestedLocalSyncIgnorePatterns() []string { + if len(d.Spec.Sync.Paths) <= 1 { + return nil + } + primary := filepath.Clean(strings.TrimSpace(d.Spec.Sync.Paths[0].Local)) + var patterns []string + for _, p := range d.Spec.Sync.Paths[1:] { + child := filepath.Clean(strings.TrimSpace(p.Local)) + if !syncRootNestedInside(primary, child) { + continue + } + rel, err := filepath.Rel(primary, child) + if err != nil { + continue + } + patterns = append(patterns, "/"+filepath.ToSlash(rel)) + } + sort.Strings(patterns) + return patterns +} + // syncRootsOverlap reports whether two roots are equal or one contains the // other, comparing cleaned paths lexically. func syncRootsOverlap(a, b string) bool { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 2bc35358..47852e8b 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -440,10 +440,11 @@ func TestValidateRejectsOverlappingSyncPaths(t *testing.T) { paths []SyncPathSpec }{ {"equal locals", []SyncPathSpec{{Local: ".", Remote: "/a"}, {Local: "./", Remote: "/b"}}}, - {"nested locals", []SyncPathSpec{{Local: ".", Remote: "/a"}, {Local: "./results", Remote: "/b"}}}, - {"nested locals reversed", []SyncPathSpec{{Local: "./results/deep", Remote: "/a"}, {Local: "./results", Remote: "/b"}}}, + {"extra contains primary", []SyncPathSpec{{Local: "./results/deep", Remote: "/a"}, {Local: "./results", Remote: "/b"}}}, + {"extras nested in each other", []SyncPathSpec{{Local: ".", Remote: "/a"}, {Local: "./x", Remote: "/b"}, {Local: "./x/deep", Remote: "/c"}}}, {"equal remotes", []SyncPathSpec{{Local: "./a", Remote: "/workspace"}, {Local: "./b", Remote: "/workspace/"}}}, {"nested remotes", []SyncPathSpec{{Local: "./a", Remote: "/workspace"}, {Local: "./b", Remote: "/workspace/results"}}}, + {"nested remote under primary even with nested local", []SyncPathSpec{{Local: ".", Remote: "/workspace"}, {Local: "./results", Remote: "/workspace/results"}}}, } for _, tc := range cases { cfg := validConfig() @@ -455,6 +456,38 @@ func TestValidateRejectsOverlappingSyncPaths(t *testing.T) { } } +func TestValidateAllowsLocalsNestedInsidePrimary(t *testing.T) { + cfg := validConfig() + cfg.Spec.Sync.Paths = []SyncPathSpec{ + {Local: ".", Remote: "/workspace"}, + {Local: "./results", Remote: "/data/results", Direction: "down"}, + {Local: "./datasets", Remote: "/data/sets", Direction: "up"}, + } + cfg.SetDefaults() + if err := cfg.Validate(); err != nil { + t.Fatalf("locals nested inside the primary root should validate, got %v", err) + } +} + +func TestNestedLocalSyncIgnorePatterns(t *testing.T) { + cfg := validConfig() + cfg.Spec.Sync.Paths = []SyncPathSpec{ + {Local: ".", Remote: "/workspace"}, + {Local: "./results", Remote: "/data/results"}, + {Local: "./nested/deep", Remote: "/data/deep"}, + {Local: "../outside", Remote: "/data/outside"}, // disjoint — no pattern + } + got := cfg.NestedLocalSyncIgnorePatterns() + want := []string{"/nested/deep", "/results"} + if len(got) != 2 || got[0] != want[0] || got[1] != want[1] { + t.Fatalf("unexpected patterns: %v (want %v)", got, want) + } + cfg.Spec.Sync.Paths = cfg.Spec.Sync.Paths[:1] + if got := cfg.NestedLocalSyncIgnorePatterns(); len(got) != 0 { + t.Fatalf("single mapping must produce no patterns, got %v", got) + } +} + func TestSyncRootsOverlapLexicalEdges(t *testing.T) { // Sibling with a shared name prefix is NOT overlap; mixed abs/relative // roots cannot be compared lexically and are treated as disjoint. diff --git a/scripts/e2e_kind_smoke.sh b/scripts/e2e_kind_smoke.sh index b0c9d03d..1202bab8 100644 --- a/scripts/e2e_kind_smoke.sh +++ b/scripts/e2e_kind_smoke.sh @@ -600,19 +600,19 @@ if [[ "$REMOTE_RESULT_AFTER" != "result-data" ]]; then fi echo "sync direction down verified" -# --- Multiple sync mappings: disjoint folders with per-path directions ---- -# The session currently has one mapping (direction: down). Add a second -# disjoint mapping and verify: overlapping mappings are rejected by -# validation, the extra folder syncs in its own direction after `okdev up` -# (config-hash driven), and the primary mapping keeps working. +# --- Multiple sync mappings: per-path directions, nested local roots ------ +# The session currently has one mapping (direction: down). Verify remote +# overlap is rejected, then add a second mapping whose LOCAL root nests +# inside the primary root (managed .stignore exclusion) while the remote +# stays disjoint on an auto-provisioned volume. -echo "Testing overlapping sync mappings are rejected" +echo "Testing overlapping remote roots are rejected" cp "$CFG_PATH" "$CFG_PATH.bak" OVERLAP_ENTRY="- local: \"$SYNC_DIR\" remote: /workspace direction: down - - local: \"$SYNC_DIR/nested\" - remote: /data/nested" + - local: \"$WORKDIR/elsewhere\" + remote: /workspace/nested" replace_first_in_file "$CFG_PATH" "- local: \"$SYNC_DIR\" remote: /workspace direction: down" "$OVERLAP_ENTRY" @@ -628,12 +628,14 @@ fi cp "$CFG_PATH.bak" "$CFG_PATH" echo "overlap rejection verified" -echo "Adding a second disjoint mapping (code up, results down)" -COLLECT_DIR="$WORKDIR/collected" +echo "Adding a second mapping nested inside the primary local root (code up, results down)" +COLLECT_DIR="$SYNC_DIR/collected" mkdir -p "$COLLECT_DIR" -# No volume YAML needed: okdev auto-provisions an emptyDir at the extra -# mapping's remote root and mirrors it into the sidecar. Adding the mapping -# changes the pod spec, so plain `up` must first surface drift guidance. +# The local root nests inside the repo — okdev excludes it from the primary +# folder via a managed .stignore block. No volume YAML needed: okdev +# auto-provisions an emptyDir at the extra mapping's remote root and mirrors +# it into the sidecar. Adding the mapping changes the pod spec, so plain +# `up` must first surface drift guidance. MULTI_ENTRY="- local: \"$SYNC_DIR\" remote: /workspace direction: up @@ -693,6 +695,22 @@ if [[ "$MULTI_UP_OK" != true ]]; then exit 1 fi +echo "multi-mapping: nested local root is excluded from the primary folder" +# The primary folder (direction up) must NOT carry the nested subtree to +# the pod: the managed .stignore block excludes it, so /workspace/collected +# never appears even though local collected/ has content. +sleep 15 +NESTED_LEAK=$("$OKDEV_BIN" --config "$CFG_PATH" --session "$SESSION_NAME" exec --no-tty --no-prefix -- sh -lc 'test -e /workspace/collected && echo leaked || echo clean') +if [[ "$NESTED_LEAK" != *"clean"* ]]; then + echo "ERROR: nested local root leaked through the primary folder" >&2 + exit 1 +fi +if ! grep -q "okdev:begin managed sync excludes" "$SYNC_DIR/.stignore" || ! grep -q "^/collected$" "$SYNC_DIR/.stignore"; then + echo "ERROR: expected managed exclude for /collected in primary .stignore, got:" >&2 + cat "$SYNC_DIR/.stignore" >&2 + exit 1 +fi + echo "multi-mapping: status shows both mappings with their directions" MULTI_DETAILS=$("$OKDEV_BIN" --config "$CFG_PATH" --session "$SESSION_NAME" status --details) if [[ "$MULTI_DETAILS" != *"-> /workspace"* ]] || [[ "$MULTI_DETAILS" != *"<- /data/results"* ]]; then @@ -700,8 +718,48 @@ if [[ "$MULTI_DETAILS" != *"-> /workspace"* ]] || [[ "$MULTI_DETAILS" != *"<- /d echo "$MULTI_DETAILS" >&2 exit 1 fi +if [[ "$MULTI_DETAILS" != *"excluded from primary folder (own mappings): /collected"* ]]; then + echo "ERROR: expected managed exclude in status --details, got:" >&2 + echo "$MULTI_DETAILS" >&2 + exit 1 +fi echo "multiple sync mappings verified" +echo "Removing the nested mapping leaves a tombstone (no silent widening)" +SINGLE_ENTRY="- local: \"$SYNC_DIR\" + remote: /workspace + direction: up" +replace_first_in_file "$CFG_PATH" "- local: \"$SYNC_DIR\" + remote: /workspace + direction: up + - local: \"$COLLECT_DIR\" + remote: /data/results + direction: down" "$SINGLE_ENTRY" +"$OKDEV_BIN" --config "$CFG_PATH" --session "$SESSION_NAME" up --reconcile --wait-timeout 5m +if ! grep -q "retained after mapping removal" "$SYNC_DIR/.stignore"; then + echo "ERROR: expected tombstone comment in primary .stignore, got:" >&2 + cat "$SYNC_DIR/.stignore" >&2 + exit 1 +fi +if [[ "$(cat "$COLLECT_DIR/multi.txt" 2>/dev/null)" != "multi-result" ]]; then + echo "ERROR: local files of the removed mapping must be kept" >&2 + exit 1 +fi +echo "tombstone-probe" > "$COLLECT_DIR/tombstone-probe.txt" +sleep 15 +TOMB_LEAK=$("$OKDEV_BIN" --config "$CFG_PATH" --session "$SESSION_NAME" exec --no-tty --no-prefix -- sh -lc 'test -e /workspace/collected && echo leaked || echo clean') +if [[ "$TOMB_LEAK" != *"clean"* ]]; then + echo "ERROR: removed mapping's subtree leaked into the primary folder" >&2 + exit 1 +fi +TOMB_DETAILS=$("$OKDEV_BIN" --config "$CFG_PATH" --session "$SESSION_NAME" status --details) +if [[ "$TOMB_DETAILS" != *"retained after mapping removal): /collected"* ]]; then + echo "ERROR: expected retained exclude in status --details, got:" >&2 + echo "$TOMB_DETAILS" >&2 + exit 1 +fi +echo "nested mapping removal verified" + echo "Sync direction tests completed" # --- Detached jobs: runtime-volume logs, process-group stop, sidecar ------- diff --git a/skills/okdev/SKILL.md b/skills/okdev/SKILL.md index 9bb00a2c..38c61eef 100644 --- a/skills/okdev/SKILL.md +++ b/skills/okdev/SKILL.md @@ -50,6 +50,7 @@ Do not use this skill for: - Use `okdev up --reconcile` when workload-shaping config changed and the user wants those changes applied. - `okdev up` on a `job` or `pytorchjob` workload fails fast when a pod enters `PodFailed` before readiness. If that run also created or recreated the workload, okdev deletes the failed workload and clears local session state — the next `okdev up` starts fresh. Workloads that were reused as-is (no create/recreate this run) are left alone. For `pod` workloads okdev also returns early on `Failed` but does not auto-delete. - Use `okdev sync --reset` when local/background sync state is stale but the workload itself is still the intended one. +- Sync supports multiple mappings with per-path direction (`spec.sync.paths[].direction: bi|up|down`); `down` makes the pod the authority so local writes cannot clobber results. A mapping's local root may nest inside the primary root — okdev manages the exclusion and remote volumes automatically. "Not syncing" is often the configured direction or a managed exclude, not a fault; see `references/multipod.md` and `references/troubleshooting.md`. - `okdev ssh` is the okdev-managed interactive path; `ssh okdev-` is the plain SSH host alias path. The interactive shell can be bash or zsh, and the okdev tmux prefix is `ctrl-a`. - For sync, SSH, and port-forward issues, `okdev status --details` is usually the first useful diagnostic. - For commands across pods, prefer `okdev exec` fanout: `--group`/`--workers`/`--role`/`--label`/`--pod` to target, `--script` for non-trivial pipelines, and `--detach` + `okdev jobs list` for long background runs. diff --git a/skills/okdev/references/multipod.md b/skills/okdev/references/multipod.md index a7c071f6..67c4af51 100644 --- a/skills/okdev/references/multipod.md +++ b/skills/okdev/references/multipod.md @@ -44,7 +44,7 @@ Do not assume all pods independently sync from the local machine in the same way Direction: `spec.sync.paths[].direction: bi|up|down` sets the folder direction on the local↔hub edge for that mapping (`down` = pod is the authority, local writes can never clobber pod results). It applies to the mapping's whole folder — finer per-subpath direction is impossible in Syncthing. Mesh receiver pods are always receiveonly regardless. -Multiple mappings are supported when their roots are disjoint on both sides (each becomes its own folder with its own direction). Only the FIRST (primary) mapping fans out to mesh receivers; additional mappings are local↔hub only, so sync-based result collection covers the hub pod — fleet-wide results still go through `okdev jobs logs` / `okdev cp`. +Multiple mappings are supported: remote roots must be disjoint; a mapping's local root may nest inside the primary root (okdev excludes it from the primary folder via a managed .stignore block; removal leaves a tombstone so the subtree never silently joins the code channel). Only the FIRST (primary) mapping fans out to mesh receivers; additional mappings are local↔hub only, so sync-based result collection covers the hub pod — fleet-wide results still go through `okdev jobs logs` / `okdev cp`. ## PyTorchJob diff --git a/skills/okdev/references/troubleshooting.md b/skills/okdev/references/troubleshooting.md index 81df53af..37fbadff 100644 --- a/skills/okdev/references/troubleshooting.md +++ b/skills/okdev/references/troubleshooting.md @@ -34,6 +34,11 @@ Standard repair steps: If a real file was overwritten by a bad write syncing across from the other side (e.g. an empty local file clobbered a pod-side result), recover the previous version from `.stversions/` at the sync root on the side that had the good copy (pod: `/.stversions/`; local: `/.stversions/`). Versioning is on by default (`spec.sync.syncthing.versioningDays`, 30 days). +Before treating "changes are not syncing" as a fault, check two BY-DESIGN behaviors: + +- **Direction**: with `direction: up` the pod's writes never reach local; with `down` local writes never reach the pod (syncthing flags them as local additions). `okdev status --details` shows each mapping's direction arrow. This is the configured contract, not a sync failure. +- **Managed excludes**: a directory nested inside the primary sync root that belongs to (or belonged to) its own mapping is excluded from the primary folder via the managed block in the primary root's `.stignore`. `status --details` lists active excludes and tombstones retained after a mapping was removed; deleting the entry from `.stignore` is the explicit way to fold the directory back into the primary folder. + Use `docs/troubleshooting.md` for current sync-specific guidance. ## Session Reuse vs Fresh Workload diff --git a/skills/okdev/references/workflows.md b/skills/okdev/references/workflows.md index f1052172..30efedd6 100644 --- a/skills/okdev/references/workflows.md +++ b/skills/okdev/references/workflows.md @@ -70,6 +70,19 @@ Default guidance: - `okdev sync --foreground` is useful when the user needs live troubleshooting - `okdev sync --reset` is the standard repair path for stale local sync state +Result collection via sync: add a second mapping with its own direction — no volume YAML needed (okdev auto-provisions the remote volume; the next `up` asks for `--reconcile`): + +```yaml +sync: + paths: + - .:/workspace # code (bi or up) + - local: ./collected # may nest inside the repo + remote: /data/results + direction: down # pod is the authority; local writes can't clobber results +``` + +Point the job's output at the mapping's remote root (`/data/results` above). This covers the hub pod; fleet-wide collection stays on `okdev jobs logs` / `okdev cp`. + ## File Transfer Use: