amadan
fix: age decides a stale import, not the fact of a boot
Third gpt-5.6-sol pass. It confirmed the roleFor fix complete, walking every route that decides access to a sheet, and found one new HIGH in the sweep I had just added. **"Stale by construction" was not.** sweepStaleImports discarded every filling sheet at startup, on the reasoning that the process which had been filling it was gone. That holds for a clean restart and fails everywhere two instances overlap — a rolling replacement whose old process is still draining, a duplicate start, or a test building a second App over one database. There, a starting instance deletes an import another one is still running, and the sheet vanishes from under the person doing it mid-write. My own test demonstrated the mechanism while asserting it was safe. So age decides it now: twice rastrillo's 15-minute jobTimeout, after which no live job can exist under any scheduling. An abandoned import lingers up to half an hour before being reclaimed, which costs nothing — it was already invisible — and erring towards leaving one alone is the right way to be wrong here. The test grew the case that matters: a second instance starting must NOT discard a young import. It fails if the age check goes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H6YuJ2QG6koRKjNf3ddr1Q
3 files changed,
+72
−28
internal/sheets/app.go+6 −4internal/sheets/handlers_import.go+27 −12internal/sheets/handlers_import_hidden_test.go+39 −12
diff --git a/internal/sheets/app.go b/internal/sheets/app.go| index ebac371..cbce2d0 100644 |
| --- a/internal/sheets/app.go |
| +++ b/internal/sheets/app.go |
| @@ -103,10 +103,12 @@ func New(cfg Config) *App { |
| panic("sheets: wiring job handlers: " + err.Error()) |
| } |
| a.jobPages = h |
| - // Any sheet still marked mid-import belongs to a process that is |
| - // gone, so this is the moment to reclaim them — before this one |
| - // starts an import of its own and the "stale by construction" |
| - // reasoning stops holding. See sweepStaleImports. |
| + // Reclaim imports abandoned by a process that is not coming back. |
| + // Only ones old enough to have no live job under any scheduling — |
| + // two instances CAN overlap (a draining rolling replacement, a |
| + // duplicate start), and a sweep that assumed otherwise would delete |
| + // a running import out from under the person doing it. See |
| + // sweepStaleImports. |
| a.sweepStaleImports() |
| return a |
| } |
diff --git a/internal/sheets/handlers_import.go b/internal/sheets/handlers_import.go| index 4ab6cb4..fa83cb5 100644 |
| --- a/internal/sheets/handlers_import.go |
| +++ b/internal/sheets/handlers_import.go |
| @@ -179,27 +179,42 @@ func (a *App) runImport(ctx context.Context, s SheetSummary, data []byte, progre |
| return nil |
| } |
| -// sweepStaleImports discards every sheet left mid-import by a previous |
| -// process. |
| +// staleImportAge is how long a sheet must have been filling before a |
| +// sweep will touch it. |
| // |
| -// At startup, a sheet still marked FillingSince is stale BY |
| -// CONSTRUCTION, with no timeout to guess at: the job that was filling |
| -// it was a goroutine in a process that is no longer running (the jobs |
| -// package is in-memory and unpersisted by design), so nothing is coming |
| -// back to finish it. That makes this the one sweep that cannot race |
| -// live work — within a running process every filling sheet has a live |
| -// job, and this only ever runs before there is one. |
| +// It is twice rastrillo's own jobTimeout (15 minutes), after which the |
| +// jobs registry marks a running job Failed and its context expires. So |
| +// an import older than this has no live job behind it under any |
| +// scheduling, and the doubling is slack for a machine that was asleep |
| +// or a clock that moved. |
| +// |
| +// An age check rather than "everything, at startup". The first version |
| +// of this swept unconditionally on the reasoning that a filling sheet |
| +// at process start belongs to a process that is gone — which is true of |
| +// a clean restart and false of every case where two instances overlap: |
| +// a rolling replacement where the old process is still draining, a |
| +// duplicate start, or a test that builds a second App over one |
| +// database. In those, an unconditional sweep deletes an import that is |
| +// still running, and the importer's own sheet vanishes from under them |
| +// mid-write. |
| +const staleImportAge = 30 * time.Minute |
| + |
| +// sweepStaleImports discards sheets left mid-import by a process that is |
| +// no longer coming back. |
| // |
| // Hidden was already the safe state, so this is reclamation rather than |
| // correctness: without it a deploy mid-import leaves the row, its |
| -// grants and its sealed cells behind for good. |
| +// grants and its sealed cells behind for good. That is also why erring |
| +// towards leaving one alone is the right way to be wrong. |
| func (a *App) sweepStaleImports() { |
| if a.db == nil { |
| return |
| } |
| + cutoff := time.Now().UTC().Add(-staleImportAge) |
| var stale []store.Sheet |
| - if err := a.db.Where("filling_since IS NOT NULL AND deleted_at IS NULL"). |
| - Find(&stale).Error; err != nil { |
| + if err := a.db.Where( |
| + "filling_since IS NOT NULL AND filling_since < ? AND deleted_at IS NULL", |
| + cutoff).Find(&stale).Error; err != nil { |
| return |
| } |
| for _, s := range stale { |
diff --git a/internal/sheets/handlers_import_hidden_test.go b/internal/sheets/handlers_import_hidden_test.go| index 7d7f4e9..4b689cc 100644 |
| --- a/internal/sheets/handlers_import_hidden_test.go |
| +++ b/internal/sheets/handlers_import_hidden_test.go |
| @@ -158,37 +158,64 @@ func TestAdmitsIgnoresGrantsToSheetsYouCannotOpen(t *testing.T) { |
| } |
| } |
| -// TestStaleImportsAreSweptAtBoot covers the reclamation path. A sheet |
| -// still marked filling when a process starts belongs to a process that |
| -// is gone — the jobs registry is in-memory — so nothing is coming back |
| -// to finish it. |
| +// TestStaleImportsAreSweptAtBoot covers the reclamation path, and the |
| +// line it must not cross. |
| +// |
| +// A second App over one database is what a restart looks like — and |
| +// also what a rolling replacement and a duplicate start look like, |
| +// which is the whole reason age is what decides this rather than the |
| +// mere fact of construction. An import too young to be abandoned has to |
| +// survive somebody else's boot. |
| func TestStaleImportsAreSweptAtBoot(t *testing.T) { |
| app := newTestApp(t) |
| member := testMember(t, app) |
| - s, err := app.createFillingSheet("interrupted by a deploy", member) |
| + old, err := app.createFillingSheet("interrupted by a deploy", member) |
| if err != nil { |
| t.Fatalf("createFillingSheet: %v", err) |
| } |
| + stale := time.Now().UTC().Add(-staleImportAge - time.Minute) |
| + if err := app.db.Model(&store.Sheet{}).Where("id = ?", old.ID). |
| + Update("filling_since", stale).Error; err != nil { |
| + t.Fatalf("backdate: %v", err) |
| + } |
| - // A second App over the same database is what a restart looks like. |
| + // This one started moments ago, so a live job may well still be |
| + // filling it in another instance. |
| + fresh, err := app.createFillingSheet("still going", member) |
| + if err != nil { |
| + t.Fatalf("createFillingSheet: %v", err) |
| + } |
| + |
| + // A second App over the same database. |
| restarted := New(Config{DB: app.db, Sealer: app.sealer, Admit: app.admit}) |
| - var sheet store.Sheet |
| - if err := restarted.db.Where("id = ?", s.ID).First(&sheet).Error; err != nil { |
| - t.Fatalf("reload sheet: %v", err) |
| - } |
| - if sheet.DeletedAt == nil { |
| + if deletedAt(t, restarted, old.ID) == nil { |
| t.Error("a stale import survived a restart undiscarded") |
| } |
| var grants int64 |
| - if err := restarted.db.Model(&Grant{}).Where("sheet_id = ?", s.ID). |
| + if err := restarted.db.Model(&Grant{}).Where("sheet_id = ?", old.ID). |
| Count(&grants).Error; err != nil { |
| t.Fatalf("count grants: %v", err) |
| } |
| if grants != 0 { |
| t.Errorf("a stale import kept %d grant(s) through a restart", grants) |
| } |
| + |
| + // The guard that matters: a second instance starting must not |
| + // delete an import the first one is still running. |
| + if deletedAt(t, restarted, fresh.ID) != nil { |
| + t.Error("a running import was discarded by another instance starting up") |
| + } |
| +} |
| + |
| +func deletedAt(t *testing.T, app *App, id uint64) *time.Time { |
| + t.Helper() |
| + var sheet store.Sheet |
| + if err := app.db.Where("id = ?", id).First(&sheet).Error; err != nil { |
| + t.Fatalf("reload sheet %d: %v", id, err) |
| + } |
| + return sheet.DeletedAt |
| } |
| // TestAFileOfExactlyTheAdvertisedSizeIsAccepted pins the limit the form |