Skip to content

Bulk import boxes: imported-box tasks only work on their own import's boxes - #1858

Merged
JasonWildMe merged 5 commits into
mainfrom
fix/bbox-import-review-followups
Nov 13, 2026
Merged

JasonWildMe merged 5 commits into
mainfrom
fix/bbox-import-review-followups

Conversation

@JasonWildMe

Copy link
Copy Markdown
Collaborator

Follow-up to #1848 (bulk import boxes A), which merged before review was finished.

What

The extract-then-identify intake and the per-box extraction tasks added for bulk imports with user-supplied boxes now check what they are handed before changing anything. They only ever work on a box that the named import created.

How

  • Intake — IA.intakeImportedBoxes stages an annotation only when its feature carries the stamp of the import named on the request's root task. Anything else is logged and left untouched (no task, no status change). The recovery holder applies the same rule.
  • One shared rule — Annotation.isImportedBoxOf(importTaskId), used by the intake, the envelope builder (IAGateway.importedBoxAnnotationIds) and the processor.
  • Processor — MlServiceProcessor.process checks every job for an imported-box child: it must name the one box in the job the server stored on that child, and the box must carry the child's import stamp. Any other job is dropped before a write. The check uses strict lookups, so a datastore failure or unreadable stored task parameters (Task.hasUnreadableParameters) return an error and the job does not run.
  • Gateway — an imported-box child is never reused as a request's root task, and the imported-box marker is removed from incoming task parameters on both the enqueue path and the bulk-import hand-off.

No change for imports without boxes, for normal detection and identification, or for legitimate boxed imports: the importer already stamps every box it creates, and the server-built jobs (intake, startup replay, deferred matches, retries) all pass the check.

Testing

  • New and extended cases in ImportedBoxIntakeDbTest, MlServiceProcessorImportedBoxDbTest, IAGatewayRequestRootTest and IAGatewayImportEnvelopeTest: annotations that are not this import's boxes are left untouched (identification status, tasks and the asset's detection status all unchanged); jobs of every other shape for an imported-box child are dropped; a failed lookup or unreadable parameters is an error, not a pass; server-published deferred matches still run.
  • 310 tests across the 27 imported-box, processor, gateway and boxed-import test classes pass locally.

Not verified: the full suite was not run locally (CI will), and nothing was exercised on a running install.

Reviewed with Codex over five rounds until it reported no remaining findings.

🤖 Generated with Claude Code

The extract-then-identify intake and the per-box extraction children
added for bulk imports with user-supplied boxes now check what they are
handed before changing anything:

- IA.intakeImportedBoxes stages an annotation only when its feature
  carries the stamp of the import named on the request's root task
  (Annotation.isImportedBoxOf, also used by the envelope builder).
  Anything else is logged and left untouched; the recovery holder
  applies the same rule.
- MlServiceProcessor.process: an imported-box child accepts only a job
  that names the one box in the job the server stored on it, and that
  box must carry the child's import stamp. Any other job is dropped
  before a write. The check uses strict lookups, so a datastore failure
  or unreadable stored task parameters return an error rather than
  running the job.
- IAGateway: an imported-box child is never reused as a request's root,
  and the imported-box marker is removed from incoming task parameters
  on both the enqueue and the bulk-import hand-off paths.

Reviewed with Codex over five rounds.

Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@JasonWildMe JasonWildMe added this to the 11.1 milestone Nov 10, 2026
@codecov-commenter

codecov-commenter commented Nov 10, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.43%. Comparing base (809b1ee) to head (78b5336).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1858   +/-   ##
=======================================
  Coverage   59.43%   59.43%           
=======================================
  Files         320      320           
  Lines       13104    13104           
  Branches     4274     4274           
=======================================
  Hits         7788     7788           
  Misses       5033     5033           
  Partials      283      283           
Flag Coverage Δ
backend 59.43% <ø> (ø)
frontend 59.43% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@UrbanFercec UrbanFercec left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran it with #1860 on top as well, 279 tests green including the DbTests. Three things inline, the first one also hits ordinary detection jobs.

try {
mismatch = importedBoxJobMismatch(taskId, named);
} catch (Exception ex) {
return MlServiceJobOutcome.persistError("PERSIST", "imported-box check failed: " + ex);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch covers every job with a taskId, not only imported boxes. I ran a plain task with parameters {broken and a mediaAssetId job: ERROR_PERSIST, no ML call, task stays initiated, asset stays processing. On main the same job completes. Failing closed only when the raw parameters carry the marker, or calling markDetectionFailure here like the load path does, would avoid that.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good catch. Fixed in f413547: an unreadable-parameters task now fails closed only when something persisted says it could be an imported-box child (the marker text, a stored job that names an annotation, or a held imported box). A plain task with {broken parameters and a mediaAssetId job passes the check and runs as on main. New test unreadableParametersWithoutTheMarkerDoNotStopAnOrdinaryJob; the existing corruption test still covers a real imported-box child (including none, Unknown and {broken).

tparams.put("username", tuser.getUsername());
}
// the imported-box marker is only ever set by the server, on the children it creates
tparams.remove(Task.PARAM_IMPORTED_BOX);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This drops only the top-level key, but ImportedBoxReplay matches "importedBox":true anywhere in the stored parameters. Through gatewayTaskParams, {"x":{"importedBox":true}} is kept and the task gets replayed on every restart, and enough of them fill the 500 oldest slots. Re-checking Task.isImportedBoxTask(t) in fetchStaleResumeMessages would close it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f413547. The gateway now removes the marker at every depth of the task parameters (both gatewayTaskParams and handleBulkImport), and fetchStaleResumeMessages re-checks Task.isImportedBoxTask on each row. Candidates are read in pages, so rows that don't qualify never count toward the 500, and the rows examined per run are strictly bounded. Tests: gatewayTaskParamsDropTheMarkerAtEveryDepth, aNestedMarkerIsNotAnImportedBoxChild (including limit 1 with an older non-qualifying row), theScanBoundIsStrict.

" no longer exists; skipped");
continue;
}
if (!ann.isImportedBoxOf(importId)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When every box gets skipped here, nothing marks the root, so a client polling it sees a task that never finishes. rootWithoutAnImportStampExtractsNothing checks that nothing ran but not the root's status. Maybe mark the root as an invalid envelope when boxes were named and none were taken?

@JasonWildMe JasonWildMe Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f413547. When no named box is taken, the root now ends: error / INVALID_ENVELOPE if a named annotation exists but isn't a box of this import, completed if they were all deleted. A failed commit of that end goes through the existing staging recovery (root ends EXTRACTION_PERSIST). rootWithoutAnImportStampExtractsNothing now checks the root status; new tests rootWhoseBoxesAreAllGoneCompletesWithNothingToDo and aFailedCommitOfTheEmptyRootEndsItThroughRecovery. The import page's summary does not yet read this request's end for the all-deleted case; that is a follow-up on #1861, which owns the boxed-import completion rule.

JasonWildMe and others added 3 commits November 10, 2026 11:50
The submissions hand-off tests that arrived with #1851 mock an annotation.
The hand-off now selects an import's boxes through isImportedBoxOf, so the
mock has to answer it; without the stub the mocked box was left out and two
tests failed once main was merged in.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
From UrbanFercec's review of #1858:

- A task whose stored parameters do not parse no longer stops an ordinary
  job. The imported-box check fails closed only when something persisted
  says the task could be an imported-box child: the marker text, a stored
  job that names an annotation, or a held imported box. Detection jobs on
  such tasks run as before.
- The gateway removes the imported-box marker at every depth of
  client-supplied task parameters, and startup replay republishes only
  tasks whose parsed top-level flag is set. Candidates are read in pages,
  so rows that do not qualify never count toward the per-run limit, and
  the number of rows examined is strictly bounded.
- An extraction request whose named boxes were all skipped now ends: in
  error (INVALID_ENVELOPE) when a named annotation belongs to another
  import, completed when they were all deleted. A failed commit of that
  end goes through the existing staging recovery.

Reviewed with Codex over three rounds (converged).

Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@JasonWildMe

Copy link
Copy Markdown
Collaborator Author

Thanks @UrbanFercec! Lemme know of the fixes address your concerns.

@JasonWildMe
JasonWildMe merged commit 7b5bba1 into main Nov 13, 2026
1 check passed
@JasonWildMe
JasonWildMe deleted the fix/bbox-import-review-followups branch November 13, 2026 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants