Commit 4af7648

Eric Bower  ·  2026-06-17 22:50:21 -0400 EDT
parent 96bba54 
fix(rsync): properly delete `._pico_keep_dir` first so os.Remove suceeds
2 files changed,  +429, -3
M pkg/apps/pgs/uploader.go
A pkg/apps/pgs/uploader_test.go
M pkg/apps/pgs/uploader.go
+48, -3
......@@ -490,12 +490,57 @@ func (h *UploadAssetHandler) Delete(s *pssh.SSHServerConnSession, entry *senduti
490490 projectName := shared.GetProjectName(entry)
491491 logger = logger.With("project", projectName)
492492
493- if assetFilepath == filepath.Join("/", projectName, "._pico_keep_dir") {
494- return os.ErrPermission
493+ logger.Info("deleting file")
494+
495+ // Check if this path represents a directory (has a . _pico_keep_dir marker)
496+ keepDirPath := filepath.Join(assetFilepath, "._pico_keep_dir")
497+ keepDirReader, _, keepDirErr := h.Cfg.Storage.GetObject(bucket, keepDirPath)
498+ if keepDirReader != nil {
499+ defer func() {
500+ _ = keepDirReader.Close()
501+ }()
495502 }
496503
497- logger.Info("deleting file")
504+ if keepDirErr == nil {
505+ // This is a directory being deleted. We must delete all nested
506+ // . _pico_keep_dir files first, otherwise os.Remove() on the
507+ // directory will fail with "directory not empty".
508+ nested, err := h.Cfg.Storage.ListObjects(bucket, assetFilepath+"/", true)
509+ if err != nil {
510+ return err
511+ }
512+
513+ baseDepth := strings.Count(assetFilepath, string(os.PathSeparator))
514+ for _, nestedEntry := range nested {
515+ if filepath.Base(nestedEntry.Name()) != "._pico_keep_dir" {
516+ continue
517+ }
518+ // Only delete keep_dir files that are direct or nested children
519+ nestedDepth := strings.Count(nestedEntry.Name(), string(os.PathSeparator))
520+ if nestedDepth <= baseDepth {
521+ continue
522+ }
523+ nestedKeepDirPath := filepath.Join(assetFilepath, nestedEntry.Name())
524+ if delErr := h.Cfg.Storage.DeleteObject(bucket, nestedKeepDirPath); delErr != nil {
525+ return delErr
526+ }
527+ }
528+
529+ // Delete this directory's own . _pico_keep_dir
530+ err = h.Cfg.Storage.DeleteObject(bucket, keepDirPath)
531+ if err != nil {
532+ return err
533+ }
534+
535+ // Delete the directory itself (no-op for S3-style storage, removes the dir for fs storage)
536+ _ = h.Cfg.Storage.DeleteObject(bucket, assetFilepath)
537+
538+ surrogate := getSurrogateKey(user.Name, projectName)
539+ h.Cfg.CacheClearingQueue <- surrogate
540+ return nil
541+ }
498542
543+ // Regular file deletion: create . _pico_keep_dir if the directory becomes empty
499544 pathDir := filepath.Dir(assetFilepath)
500545 fileName := filepath.Base(assetFilepath)
501546
A pkg/apps/pgs/uploader_test.go
+381, -0
......@@ -0,0 +1,381 @@
1+package pgs
2+
3+import (
4+ "encoding/pem"
5+ "fmt"
6+ "io"
7+ "log/slog"
8+ "net"
9+ "os"
10+ "os/exec"
11+ "path/filepath"
12+ "strings"
13+ "testing"
14+ "time"
15+
16+ pgsdb "github.com/picosh/pico/pkg/apps/pgs/db"
17+ "github.com/picosh/pico/pkg/db"
18+ "github.com/picosh/pico/pkg/pssh"
19+ "github.com/picosh/pico/pkg/shared"
20+ "github.com/picosh/pico/pkg/storage"
21+ "github.com/pkg/sftp"
22+ "github.com/prometheus/client_golang/prometheus"
23+)
24+
25+// TestRsyncDeleteDirectoryWithKeepDir verifies that rsync --delete can
26+// successfully delete a directory that contains . _pico_keep_dir markers.
27+//
28+// Regression test for: "remove /storage/.../project/... directory not empty"
29+// The bug occurred because . _pico_keep_dir files were not cleaned up when
30+// their parent directory was explicitly deleted, causing os.Remove() to fail.
31+func TestRsyncDeleteDirectoryWithKeepDir(t *testing.T) {
32+ opts := &slog.HandlerOptions{
33+ AddSource: true,
34+ Level: slog.LevelDebug,
35+ }
36+ logger := slog.New(
37+ slog.NewTextHandler(os.Stdout, opts),
38+ )
39+ slog.SetDefault(logger)
40+ dbpool := pgsdb.NewDBMemory(logger)
41+ dbpool.SetupTestData()
42+
43+ // Use filesystem storage so the "directory not empty" bug manifests
44+ tmpDir, err := os.MkdirTemp("", "pgs-test-storage-*")
45+ if err != nil {
46+ t.Fatalf("failed to create temp dir: %v", err)
47+ }
48+ defer func() {
49+ _ = os.RemoveAll(tmpDir)
50+ }()
51+
52+ st, err := storage.NewStorageFS(logger, tmpDir)
53+ if err != nil {
54+ t.Fatalf("failed to create storage: %v", err)
55+ }
56+
57+ pubsub := NewPubsubChan()
58+ defer func() {
59+ _ = pubsub.Close()
60+ }()
61+
62+ _ = os.Setenv("PGS_SSH_PORT", "0")
63+ _ = os.Setenv("PGS_PROM_PORT", "0")
64+
65+ cfg := NewPgsConfig(logger, dbpool, st, pubsub)
66+ done := make(chan error)
67+ readyCh := make(chan *pssh.SSHServer)
68+ prometheus.DefaultRegisterer = prometheus.NewRegistry()
69+
70+ go StartSshServerForTesting(cfg, done, readyCh)
71+
72+ server := <-readyCh
73+ if server == nil {
74+ t.Fatal("failed to create ssh server")
75+ }
76+
77+ var actualAddr string
78+ for i := 0; i < 100; i++ {
79+ server.Mu.Lock()
80+ listener := server.Listener
81+ server.Mu.Unlock()
82+ if listener != nil {
83+ actualAddr = listener.Addr().String()
84+ break
85+ }
86+ time.Sleep(10 * time.Millisecond)
87+ }
88+ if actualAddr == "" {
89+ t.Fatal("server listener not ready")
90+ }
91+
92+ user := GenerateUser()
93+ dbpool.Pubkeys = append(dbpool.Pubkeys, &db.PublicKey{
94+ ID: "test-pubkey-keepdir",
95+ UserID: dbpool.Users[0].ID,
96+ Key: shared.KeyForKeyText(user.signer.PublicKey()),
97+ })
98+
99+ conn, err := user.NewClientAddr(actualAddr)
100+ if err != nil {
101+ t.Fatalf("failed to connect: %v", err)
102+ }
103+ defer func() {
104+ _ = conn.Close()
105+ }()
106+
107+ client, err := sftp.NewClient(conn)
108+ if err != nil {
109+ t.Fatalf("failed to create sftp client: %v", err)
110+ }
111+ defer func() {
112+ _ = client.Close()
113+ }()
114+
115+ // Create temp directory with nested structure
116+ name, err := os.MkdirTemp("", "rsync-dir-test-*")
117+ if err != nil {
118+ t.Fatalf("failed to create temp dir: %v", err)
119+ }
120+ defer func() {
121+ _ = os.RemoveAll(name)
122+ }()
123+
124+ // Create: project/subdir/nested/file.txt
125+ nestedDir := filepath.Join(name, "subdir", "nested")
126+ err = os.MkdirAll(nestedDir, 0755)
127+ if err != nil {
128+ t.Fatalf("failed to create nested dir: %v", err)
129+ }
130+ err = os.WriteFile(filepath.Join(nestedDir, "deep.txt"), []byte("deep content"), 0644)
131+ if err != nil {
132+ t.Fatalf("failed to write deep.txt: %v", err)
133+ }
134+
135+ // Create: project/subdir/file.txt
136+ err = os.WriteFile(filepath.Join(name, "subdir", "file.txt"), []byte("subdir content"), 0644)
137+ if err != nil {
138+ t.Fatalf("failed to write file.txt: %v", err)
139+ }
140+
141+ // Create: project/index.html (stays after delete)
142+ err = os.WriteFile(filepath.Join(name, "index.html"), []byte("index content"), 0644)
143+ if err != nil {
144+ t.Fatalf("failed to write index.html: %v", err)
145+ }
146+
147+ block := &pem.Block{
148+ Type: "OPENSSH PRIVATE KEY",
149+ Bytes: user.privateKey,
150+ }
151+ keyFile := filepath.Join(name, "id_ed25519")
152+ err = os.WriteFile(keyFile, pem.EncodeToMemory(block), 0600)
153+ if err != nil {
154+ t.Fatalf("failed to write key file: %v", err)
155+ }
156+
157+ _, port, err := net.SplitHostPort(actualAddr)
158+ if err != nil {
159+ t.Fatalf("failed to parse server address: %v", err)
160+ }
161+
162+ eCmd := fmt.Sprintf(
163+ "ssh -p %s -o IdentitiesOnly=yes -i %s -o StrictHostKeyChecking=no",
164+ port, keyFile,
165+ )
166+
167+ // Upload files including the subdir/ directory
168+ cmd := exec.Command("rsync", "-rv", "-e", eCmd, name+"/", "localhost:/testdir")
169+ result, err := cmd.CombinedOutput()
170+ if err != nil {
171+ t.Fatalf("rsync upload failed: %v\noutput: %s", err, result)
172+ }
173+
174+ // Verify files exist on the server
175+ _, err = client.Lstat("/testdir/subdir/file.txt")
176+ if err != nil {
177+ t.Fatalf("subdir/file.txt not found after upload: %v", err)
178+ }
179+ _, err = client.Lstat("/testdir/subdir/nested/deep.txt")
180+ if err != nil {
181+ t.Fatalf("subdir/nested/deep.txt not found after upload: %v", err)
182+ }
183+
184+ // Now remove the entire subdir/ from the local directory
185+ err = os.RemoveAll(filepath.Join(name, "subdir"))
186+ if err != nil {
187+ t.Fatalf("failed to remove local subdir: %v", err)
188+ }
189+
190+ // Run rsync --delete - this should delete the subdir/ directory
191+ // WITHOUT failing with "directory not empty"
192+ delCmd := exec.Command("rsync", "-rv", "--delete", "-e", eCmd, name+"/", "localhost:/testdir")
193+ result, err = delCmd.CombinedOutput()
194+ if err != nil {
195+ t.Fatalf("rsync --delete failed (this was the bug - 'directory not empty'): %v\noutput: %s", err, result)
196+ }
197+
198+ // Verify the subdir and its contents are gone
199+ _, err = client.Lstat("/testdir/subdir")
200+ if err == nil {
201+ t.Fatal("subdir/ should have been deleted but still exists")
202+ }
203+ // SFTP can return "no such file" or "file does not exist" depending on version
204+ if !strings.Contains(err.Error(), "no such file") && !strings.Contains(err.Error(), "does not exist") {
205+ t.Fatalf("expected 'not found' error, got: %v", err)
206+ }
207+
208+ // Verify the . _pico_keep_dir markers are also cleaned up
209+ _, err = client.Lstat("/testdir/subdir/._pico_keep_dir")
210+ if err == nil {
211+ t.Fatal("subdir/._pico_keep_dir should have been deleted but still exists")
212+ }
213+
214+ // Verify index.html still exists (it wasn't deleted)
215+ fi, err := client.Lstat("/testdir/index.html")
216+ if err != nil {
217+ t.Fatalf("index.html should still exist: %v", err)
218+ }
219+ if fi.Size() != 13 {
220+ t.Errorf("index.html has wrong size: got %d, want 13", fi.Size())
221+ }
222+
223+ close(done)
224+ time.Sleep(100 * time.Millisecond)
225+}
226+
227+// TestRsyncDeleteNestedEmptyDirectories verifies that rsync --delete handles
228+// deeply nested directory structures where intermediate directories become empty
229+// and need . _pico_keep_dir markers managed correctly.
230+func TestRsyncDeleteNestedEmptyDirectories(t *testing.T) {
231+ opts := &slog.HandlerOptions{
232+ AddSource: true,
233+ Level: slog.LevelDebug,
234+ }
235+ logger := slog.New(
236+ slog.NewTextHandler(io.Discard, opts),
237+ )
238+ slog.SetDefault(logger)
239+ dbpool := pgsdb.NewDBMemory(logger)
240+ dbpool.SetupTestData()
241+
242+ tmpDir, err := os.MkdirTemp("", "pgs-test-nested-*")
243+ if err != nil {
244+ t.Fatalf("failed to create temp dir: %v", err)
245+ }
246+ defer func() {
247+ _ = os.RemoveAll(tmpDir)
248+ }()
249+
250+ st, err := storage.NewStorageFS(logger, tmpDir)
251+ if err != nil {
252+ t.Fatalf("failed to create storage: %v", err)
253+ }
254+
255+ pubsub := NewPubsubChan()
256+ defer func() {
257+ _ = pubsub.Close()
258+ }()
259+
260+ _ = os.Setenv("PGS_SSH_PORT", "0")
261+ _ = os.Setenv("PGS_PROM_PORT", "0")
262+
263+ cfg := NewPgsConfig(logger, dbpool, st, pubsub)
264+ done := make(chan error)
265+ readyCh := make(chan *pssh.SSHServer)
266+ prometheus.DefaultRegisterer = prometheus.NewRegistry()
267+
268+ go StartSshServerForTesting(cfg, done, readyCh)
269+
270+ server := <-readyCh
271+ if server == nil {
272+ t.Fatal("failed to create ssh server")
273+ }
274+
275+ var actualAddr string
276+ for i := 0; i < 100; i++ {
277+ server.Mu.Lock()
278+ listener := server.Listener
279+ server.Mu.Unlock()
280+ if listener != nil {
281+ actualAddr = listener.Addr().String()
282+ break
283+ }
284+ time.Sleep(10 * time.Millisecond)
285+ }
286+ if actualAddr == "" {
287+ t.Fatal("server listener not ready")
288+ }
289+
290+ user := GenerateUser()
291+ dbpool.Pubkeys = append(dbpool.Pubkeys, &db.PublicKey{
292+ ID: "test-pubkey-nested",
293+ UserID: dbpool.Users[0].ID,
294+ Key: shared.KeyForKeyText(user.signer.PublicKey()),
295+ })
296+
297+ conn, err := user.NewClientAddr(actualAddr)
298+ if err != nil {
299+ t.Fatalf("failed to connect: %v", err)
300+ }
301+ defer func() {
302+ _ = conn.Close()
303+ }()
304+
305+ client, err := sftp.NewClient(conn)
306+ if err != nil {
307+ t.Fatalf("failed to create sftp client: %v", err)
308+ }
309+ defer func() {
310+ _ = client.Close()
311+ }()
312+
313+ // Create temp directory with deeply nested structure
314+ name, err := os.MkdirTemp("", "rsync-nested-test-*")
315+ if err != nil {
316+ t.Fatalf("failed to create temp dir: %v", err)
317+ }
318+ defer func() {
319+ _ = os.RemoveAll(name)
320+ }()
321+
322+ // Create: a/b/c/d/file.txt
323+ deepDir := filepath.Join(name, "a", "b", "c", "d")
324+ err = os.MkdirAll(deepDir, 0755)
325+ if err != nil {
326+ t.Fatalf("failed to create deep dir: %v", err)
327+ }
328+ err = os.WriteFile(filepath.Join(deepDir, "file.txt"), []byte("deep"), 0644)
329+ if err != nil {
330+ t.Fatalf("failed to write file: %v", err)
331+ }
332+
333+ block := &pem.Block{
334+ Type: "OPENSSH PRIVATE KEY",
335+ Bytes: user.privateKey,
336+ }
337+ keyFile := filepath.Join(name, "id_ed25519")
338+ err = os.WriteFile(keyFile, pem.EncodeToMemory(block), 0600)
339+ if err != nil {
340+ t.Fatalf("failed to write key file: %v", err)
341+ }
342+
343+ _, port, err := net.SplitHostPort(actualAddr)
344+ if err != nil {
345+ t.Fatalf("failed to parse server address: %v", err)
346+ }
347+
348+ eCmd := fmt.Sprintf(
349+ "ssh -p %s -o IdentitiesOnly=yes -i %s -o StrictHostKeyChecking=no",
350+ port, keyFile,
351+ )
352+
353+ // Upload
354+ cmd := exec.Command("rsync", "-rv", "-e", eCmd, name+"/", "localhost:/deep")
355+ result, err := cmd.CombinedOutput()
356+ if err != nil {
357+ t.Fatalf("rsync upload failed: %v\noutput: %s", err, result)
358+ }
359+
360+ // Remove the entire a/ directory locally
361+ err = os.RemoveAll(filepath.Join(name, "a"))
362+ if err != nil {
363+ t.Fatalf("failed to remove local a/: %v", err)
364+ }
365+
366+ // rsync --delete should handle the deeply nested structure
367+ delCmd := exec.Command("rsync", "-rv", "--delete", "-e", eCmd, name+"/", "localhost:/deep")
368+ result, err = delCmd.CombinedOutput()
369+ if err != nil {
370+ t.Fatalf("rsync --delete failed on nested dirs: %v\noutput: %s", err, result)
371+ }
372+
373+ // Verify the entire tree is gone
374+ _, err = client.Lstat("/deep/a")
375+ if err == nil {
376+ t.Fatal("/deep/a should have been deleted but still exists")
377+ }
378+
379+ close(done)
380+ time.Sleep(100 * time.Millisecond)
381+}