amadan

oficina / sheets Public

fix: a reveal cannot resurrect a swept sheet, and boot is not the only sweep

Fourth gpt-5.6-sol pass. Two findings, both fair, both cheap.

**The sweep race was narrowed, not closed.** A job's goroutine cannot
be killed — rastrillo's jobs package says so itself; the timeout stops a
job counting, not running — so an import pathologically slower than
staleImportAge can still be writing when another instance's sweep
discards its sheet. markFilled would then clear filling_since on a
soft-deleted row, un-hiding it, and send the importer to a sheet no
read path serves.

markFilled now only reveals a sheet that is not deleted, and says so
when it cannot. The sweep wins, the import reports failure, and whatever
the orphaned goroutine goes on writing lands in a row nothing can reach.

**Boot was the only sweep, and that left a gap with no second chance.**
A process dying during a YOUNG import is replaced by one that starts
before the threshold, finds nothing old enough, and never looks again —
so the hidden row, its cells and its grant sit there until some later
restart happens to fall the right side of the age check. Import now
sweeps too. It is one indexed query, and anybody affected by an
abandoned import is by definition somebody who imports.

Both have tests. The file splits are the 300-line cap, nothing more:
handlers_import.go keeps the request, importlifecycle.go takes what
happens to the sheet afterwards.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YuJ2QG6koRKjNf3ddr1Q
Paul Campbell pushed by paul@keymail.dev 4af9e0d9fa8d1c6f8d9808b8721bf4580168f490 parent 8ca7d2c
4 files changed, +343 −243
  • internal/sheets/handlers_import.go +11 −105
  • internal/sheets/handlers_import_hidden_test.go +0 −138
  • internal/sheets/importlifecycle.go +134 −0
  • internal/sheets/importlifecycle_test.go +198 −0

CI passed — run details

Intent

Attributed to this commit: 425 g CO₂e (49916647 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on feat/xlsx-drop-import, never backward from it. A turn that produced more than one commit books all of its tokens to the last one, and a concurrent session on this branch can add to the same figure. Some of it was priced using the shared default rate, because no model-specific factor row exists yet for at least one model involved. The Carbon view has the repo total, which is never derived from this attribution.

full ledger: feat/xlsx-drop-import

diff --git a/internal/sheets/handlers_import.go b/internal/sheets/handlers_import.go
index fa83cb5..09ba94f 100644
--- a/internal/sheets/handlers_import.go
+++ b/internal/sheets/handlers_import.go
@@ -6,12 +6,10 @@ import (
"errors"
"net/http"
"strconv"
- "time"
"github.com/carlosframework/rastrillo/jobs"
"github.com/carlosframework/rastrillo/sessions"
- "amadan.net/oficina/sheets/internal/sheets/store"
"amadan.net/oficina/sheets/internal/sheets/xlsx"
)
@@ -100,6 +98,17 @@ func (a *App) handleSheetImport(w http.ResponseWriter, r *http.Request) {
return
}
+ // Reclaim here as well as at boot. Boot alone leaves a gap with no
+ // second chance in it: a process that dies during a YOUNG import is
+ // replaced by one that starts before the threshold, finds nothing
+ // old enough to sweep, and never looks again — so the hidden row,
+ // its cells and its grant sit there until some later restart
+ // happens to fall the right side of the age check. Importing is the
+ // natural moment to look, it is one indexed query, and anybody
+ // affected by an abandoned import is by definition someone who
+ // imports.
+ a.sweepStaleImports()
+
data, filename, refusal := readUpload(w, r)
if refusal != "" {
a.renderSheetList(w, r, http.StatusBadRequest, refusal)
@@ -178,106 +187,3 @@ func (a *App) runImport(ctx context.Context, s SheetSummary, data []byte, progre
a.touchSheet(s.ID)
return nil
}
-
-// staleImportAge is how long a sheet must have been filling before a
-// sweep will touch it.
-//
-// 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. 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 filling_since < ? AND deleted_at IS NULL",
- cutoff).Find(&stale).Error; err != nil {
- return
- }
- for _, s := range stale {
- a.discardSheet(s.ID)
- }
-}
-
-// markFilled reveals a sheet that finished filling.
-//
-// Its error is NOT dropped, unlike touchSheet's: a sheet whose flag
-// will not clear is invisible, so swallowing this would report a
-// successful import and then 404 the person on the sheet it made.
-func (a *App) markFilled(id uint64) error {
- if a.db == nil {
- return errNoDB
- }
- return a.db.Model(&store.Sheet{}).Where("id = ?", id).
- Update("filling_since", nil).Error
-}
-
-// importFailure turns an import error into text safe to show and safe
-// to log.
-//
-// Safe to LOG is the binding half and the easy one to miss: rastrillo's
-// job runner slogs the error this function's result becomes
-// (jobs.run's "jobs: failed" line), and `carlos logs` is platform-held
-// storage this app cannot grep or redact. So this never wraps the
-// original — it replaces it. A coordinate and a code are what an error
-// here is allowed to carry (AGENTS.md), and xlsx.CellError is what
-// makes the coordinate available without reading it back out of a
-// string.
-func importFailure(err error) error {
- if errors.Is(err, store.ErrTabTooLarge) {
- return errors.New("that workbook holds more cells than one sheet can take")
- }
- var cell *xlsx.CellError
- if errors.As(err, &cell) {
- return errors.New("the import stopped at cell " + cell.Ref)
- }
- return errors.New("that file could not be read as a spreadsheet")
-}
-
-// discardSheet throws away a sheet whose import did not finish: it
-// soft-deletes the row and removes the grants that were minted with it.
-//
-// The grants matter on their own. Admits counts a member's grants to
-// decide whether an address may sign in at all, so an owner grant left
-// behind by a failed import keeps admitting a guest whose every real
-// grant has since been revoked. Deleting them here keeps that from
-// being something a failed upload can create. (The wider gap — Admits
-// not excluding grants to already-deleted sheets — is not this
-// function's to close, and is reported separately.)
-//
-// Soft-deleting the sheet is belt and braces rather than the mechanism:
-// the sheet is still marked FillingSince, so it is already invisible on
-// every read path, and an error here therefore cannot expose anything.
-// That is why this returns nothing — there is no failure mode left
-// worth reporting over the import failure that caused it.
-func (a *App) discardSheet(id uint64) {
- if a.db == nil {
- return
- }
- now := time.Now().UTC()
- a.db.Model(&store.Sheet{}).Where("id = ?", id).Update("deleted_at", now)
- a.db.Where("sheet_id = ?", id).Delete(&Grant{})
-}
diff --git a/internal/sheets/handlers_import_hidden_test.go b/internal/sheets/handlers_import_hidden_test.go
index 4b689cc..429aba2 100644
--- a/internal/sheets/handlers_import_hidden_test.go
+++ b/internal/sheets/handlers_import_hidden_test.go
@@ -1,9 +1,6 @@
package sheets
import (
- "net/http"
- "net/http/httptest"
- "strings"
"testing"
"time"
@@ -157,138 +154,3 @@ func TestAdmitsIgnoresGrantsToSheetsYouCannotOpen(t *testing.T) {
t.Error("a grant to a sheet still being filled admits its holder")
}
}
-
-// 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)
-
- 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)
- }
-
- // 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})
-
- 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 = ?", 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
-// promises. MaxBytesReader bounds the whole multipart body, so without
-// slack for the framing a file of exactly 20 MB — the number printed on
-// the page — is refused.
-//
-// The payload is not a workbook, so this stops at the extension check;
-// what matters is that it gets that far rather than dying as "too
-// large".
-func TestAFileOfExactlyTheAdvertisedSizeIsAccepted(t *testing.T) {
- app, cookies := signIn(t)
-
- rec := do(t, app, as(uploadRequest(t, "big.bin", make([]byte, maxImportBytes)), cookies))
- if rec.Code != http.StatusBadRequest {
- t.Fatalf("answered %d, want 400", rec.Code)
- }
- body := rec.Body.String()
- if strings.Contains(body, refusalTooLarge) {
- t.Error("a file of exactly the advertised size was refused as too large")
- }
- if !strings.Contains(body, refusalNotWorkbook) {
- t.Errorf("want the extension refusal, got: %q", firstRefusal(body))
- }
-}
-
-// TestAFileOverTheAdvertisedSizeIsRefused is the other side of it: the
-// slack exists for the framing, not to raise the limit.
-func TestAFileOverTheAdvertisedSizeIsRefused(t *testing.T) {
- app, cookies := signIn(t)
-
- rec := do(t, app, as(uploadRequest(t, "big.xlsx", make([]byte, maxImportBytes+1)), cookies))
- if rec.Code != http.StatusBadRequest {
- t.Fatalf("answered %d, want 400", rec.Code)
- }
- if !strings.Contains(rec.Body.String(), refusalTooLarge) {
- t.Error("a file over the advertised size was not refused as too large")
- }
-}
-
-// firstRefusal pulls whichever refusal a page is carrying, for a
-// failure message that says what actually happened.
-func firstRefusal(body string) string {
- for _, r := range []string{
- refusalNoFile, refusalNotWorkbook, refusalTooLarge,
- refusalBusy, refusalUnreadable,
- } {
- if strings.Contains(body, r) {
- return r
- }
- }
- return "no refusal on the page"
-}
-
-// TestImportedSheetIsNotReachableByLinkWhileFilling closes the last
-// read path: share links resolve a sheet by id of their own accord, so
-// they need the same filter as the list and the page.
-func TestImportedSheetIsNotReachableByLinkWhileFilling(t *testing.T) {
- app := newTestApp(t)
- member := testMember(t, app)
-
- s, err := app.createFillingSheet("half a workbook", member)
- if err != nil {
- t.Fatalf("createFillingSheet: %v", err)
- }
- token, err := app.createLink(s.ID, SheetViewer, member)
- if err != nil {
- // Minting a link for a filling sheet may itself be refused,
- // which is a perfectly good answer to this question.
- return
- }
- rec := do(t, app, httptest.NewRequest(http.MethodGet, "/l/"+token, nil))
- if rec.Code == http.StatusOK {
- t.Error("a share link opens a sheet that is still being filled")
- }
-}
diff --git a/internal/sheets/importlifecycle.go b/internal/sheets/importlifecycle.go
new file mode 100644
index 0000000..a318eb4
--- /dev/null
+++ b/internal/sheets/importlifecycle.go
@@ -0,0 +1,134 @@
+// The life of an imported sheet after the upload: revealed, discarded,
+// or reclaimed later by whoever notices it was abandoned. Split from
+// handlers_import.go only because that file reached the 300-line cap.
+package sheets
+
+import (
+ "errors"
+ "time"
+
+ "amadan.net/oficina/sheets/internal/sheets/store"
+ "amadan.net/oficina/sheets/internal/sheets/xlsx"
+)
+
+// staleImportAge is how long a sheet must have been filling before a
+// sweep will touch it.
+//
+// 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. 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 filling_since < ? AND deleted_at IS NULL",
+ cutoff).Find(&stale).Error; err != nil {
+ return
+ }
+ for _, s := range stale {
+ a.discardSheet(s.ID)
+ }
+}
+
+// markFilled reveals a sheet that finished filling, unless it has been
+// discarded in the meantime.
+//
+// The deleted_at condition is what makes this safe against the one race
+// the age threshold narrows but cannot close: a job's goroutine cannot
+// be killed (rastrillo's jobs package says so itself — the timeout
+// stops the job counting, not running), so an import pathologically
+// slower than staleImportAge can still be writing when another
+// instance's sweep discards its sheet. Without the condition this would
+// then un-hide a deleted row and send the importer to a sheet no read
+// path will serve. With it, the sweep wins, the import reports failure,
+// and the writes the goroutine goes on making land in a row nothing can
+// reach.
+//
+// Its error is NOT dropped, unlike touchSheet's: a sheet whose flag
+// will not clear is invisible, so swallowing this would report a
+// successful import and then 404 the person on the sheet it made.
+func (a *App) markFilled(id uint64) error {
+ if a.db == nil {
+ return errNoDB
+ }
+ res := a.db.Model(&store.Sheet{}).Where("id = ? AND deleted_at IS NULL", id).
+ Update("filling_since", nil)
+ if res.Error != nil {
+ return res.Error
+ }
+ if res.RowsAffected == 0 {
+ return errors.New("sheets: that sheet was discarded before its import finished")
+ }
+ return nil
+}
+
+// importFailure turns an import error into text safe to show and safe
+// to log.
+//
+// Safe to LOG is the binding half and the easy one to miss: rastrillo's
+// job runner slogs the error this function's result becomes
+// (jobs.run's "jobs: failed" line), and `carlos logs` is platform-held
+// storage this app cannot grep or redact. So this never wraps the
+// original — it replaces it. A coordinate and a code are what an error
+// here is allowed to carry (AGENTS.md), and xlsx.CellError is what
+// makes the coordinate available without reading it back out of a
+// string.
+func importFailure(err error) error {
+ if errors.Is(err, store.ErrTabTooLarge) {
+ return errors.New("that workbook holds more cells than one sheet can take")
+ }
+ var cell *xlsx.CellError
+ if errors.As(err, &cell) {
+ return errors.New("the import stopped at cell " + cell.Ref)
+ }
+ return errors.New("that file could not be read as a spreadsheet")
+}
+
+// discardSheet throws away a sheet whose import did not finish: it
+// soft-deletes the row and removes the grants that were minted with it.
+//
+// The grants matter on their own. Admits counts a member's grants to
+// decide whether an address may sign in at all, so an owner grant left
+// behind by a failed import keeps admitting a guest whose every real
+// grant has since been revoked. Deleting them here keeps that from
+// being something a failed upload can create. (The wider gap — Admits
+// not excluding grants to already-deleted sheets — is not this
+// function's to close, and is reported separately.)
+//
+// Soft-deleting the sheet is belt and braces rather than the mechanism:
+// the sheet is still marked FillingSince, so it is already invisible on
+// every read path, and an error here therefore cannot expose anything.
+// That is why this returns nothing — there is no failure mode left
+// worth reporting over the import failure that caused it.
+func (a *App) discardSheet(id uint64) {
+ if a.db == nil {
+ return
+ }
+ now := time.Now().UTC()
+ a.db.Model(&store.Sheet{}).Where("id = ?", id).Update("deleted_at", now)
+ a.db.Where("sheet_id = ?", id).Delete(&Grant{})
+}
diff --git a/internal/sheets/importlifecycle_test.go b/internal/sheets/importlifecycle_test.go
new file mode 100644
index 0000000..9fa9689
--- /dev/null
+++ b/internal/sheets/importlifecycle_test.go
@@ -0,0 +1,198 @@
+package sheets
+
+// The reclamation half of import: what happens to a sheet whose import
+// never finished, and the line a sweep must not cross.
+
+import (
+ "net/http"
+ "net/http/httptest"
+ "strings"
+ "testing"
+ "time"
+
+ "amadan.net/oficina/sheets/internal/sheets/store"
+)
+
+// 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)
+
+ 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)
+ }
+
+ // 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})
+
+ 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 = ?", 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")
+ }
+}
+
+// TestARevealRefusesADiscardedSheet closes the last corner of the
+// sweep race. A job's goroutine cannot be killed, so an import slower
+// than staleImportAge can still be running when its sheet is swept —
+// and revealing it then would un-hide a deleted row and send the
+// importer to a sheet no read path serves.
+func TestARevealRefusesADiscardedSheet(t *testing.T) {
+ app := newTestApp(t)
+ member := testMember(t, app)
+
+ s, err := app.createFillingSheet("swept mid-import", member)
+ if err != nil {
+ t.Fatalf("createFillingSheet: %v", err)
+ }
+ app.discardSheet(s.ID)
+
+ if err := app.markFilled(s.ID); err == nil {
+ t.Fatal("a discarded sheet was revealed as a finished import")
+ }
+ // And it stays gone rather than being resurrected invisible-but-live.
+ if deletedAt(t, app, s.ID) == nil {
+ t.Error("the discarded sheet came back")
+ }
+}
+
+// TestImportingReclaimsStaleImports covers the gap a boot-only sweep
+// leaves: a process that dies during a young import is replaced before
+// the threshold, so its replacement's startup sweep finds nothing.
+func TestImportingReclaimsStaleImports(t *testing.T) {
+ app, cookies := signIn(t)
+ member := signedInMember(t, app)
+
+ abandoned, err := app.createFillingSheet("nobody is coming back", 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 = ?", abandoned.ID).
+ Update("filling_since", stale).Error; err != nil {
+ t.Fatalf("backdate: %v", err)
+ }
+
+ // Any import at all, even one that is refused, is a look.
+ do(t, app, as(uploadRequest(t, "notes.pdf", []byte("%PDF")), cookies))
+
+ if deletedAt(t, app, abandoned.ID) == nil {
+ t.Error("an abandoned import survived a later import untouched")
+ }
+}
+
+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
+// promises. MaxBytesReader bounds the whole multipart body, so without
+// slack for the framing a file of exactly 20 MB — the number printed on
+// the page — is refused.
+//
+// The payload is not a workbook, so this stops at the extension check;
+// what matters is that it gets that far rather than dying as "too
+// large".
+func TestAFileOfExactlyTheAdvertisedSizeIsAccepted(t *testing.T) {
+ app, cookies := signIn(t)
+
+ rec := do(t, app, as(uploadRequest(t, "big.bin", make([]byte, maxImportBytes)), cookies))
+ if rec.Code != http.StatusBadRequest {
+ t.Fatalf("answered %d, want 400", rec.Code)
+ }
+ body := rec.Body.String()
+ if strings.Contains(body, refusalTooLarge) {
+ t.Error("a file of exactly the advertised size was refused as too large")
+ }
+ if !strings.Contains(body, refusalNotWorkbook) {
+ t.Errorf("want the extension refusal, got: %q", firstRefusal(body))
+ }
+}
+
+// TestAFileOverTheAdvertisedSizeIsRefused is the other side of it: the
+// slack exists for the framing, not to raise the limit.
+func TestAFileOverTheAdvertisedSizeIsRefused(t *testing.T) {
+ app, cookies := signIn(t)
+
+ rec := do(t, app, as(uploadRequest(t, "big.xlsx", make([]byte, maxImportBytes+1)), cookies))
+ if rec.Code != http.StatusBadRequest {
+ t.Fatalf("answered %d, want 400", rec.Code)
+ }
+ if !strings.Contains(rec.Body.String(), refusalTooLarge) {
+ t.Error("a file over the advertised size was not refused as too large")
+ }
+}
+
+// firstRefusal pulls whichever refusal a page is carrying, for a
+// failure message that says what actually happened.
+func firstRefusal(body string) string {
+ for _, r := range []string{
+ refusalNoFile, refusalNotWorkbook, refusalTooLarge,
+ refusalBusy, refusalUnreadable,
+ } {
+ if strings.Contains(body, r) {
+ return r
+ }
+ }
+ return "no refusal on the page"
+}
+
+// TestImportedSheetIsNotReachableByLinkWhileFilling closes the last
+// read path: share links resolve a sheet by id of their own accord, so
+// they need the same filter as the list and the page.
+func TestImportedSheetIsNotReachableByLinkWhileFilling(t *testing.T) {
+ app := newTestApp(t)
+ member := testMember(t, app)
+
+ s, err := app.createFillingSheet("half a workbook", member)
+ if err != nil {
+ t.Fatalf("createFillingSheet: %v", err)
+ }
+ token, err := app.createLink(s.ID, SheetViewer, member)
+ if err != nil {
+ // Minting a link for a filling sheet may itself be refused,
+ // which is a perfectly good answer to this question.
+ return
+ }
+ rec := do(t, app, httptest.NewRequest(http.MethodGet, "/l/"+token, nil))
+ if rec.Code == http.StatusOK {
+ t.Error("a share link opens a sheet that is still being filled")
+ }
+}